Re: [PATCH] Improve FreeDOS direct loading support compatibility.

[Vladimir 'φ-coder/phcoder' Serbinenko]

On 08.07.2012 10:28, C. Masloch wrote:

> 
> Thanks for the help. The attached patch includes spaces around all
> operators, doesn't include the void cast, and doesn't mention (E)DR-DOS
> any longer. I recently learned that the dl register requirement is
> present in prior releases of the FreeDOS kernel too, still called DOS-C.
> These DOS-C releases already are under GPLv2+ so I hope it is okay to
> mention them in the comment, as the attached patch does.
> 
> Regards,
> Chris
> 
> improve-freedos-compatibility-2.diff
> 
> 
> Subject: [PATCH] Improve FreeDOS direct loading support compatibility
> Date: Sun, 08 Jul 2012 10:08:29 +0200
> From: "C. Masloch" <address@hidden>
> 
>  ChangeLog                          |  15 ++++++
>  grub-core/lib/i386/relocator.c     |   2 +
>  grub-core/lib/i386/relocator16.S   |   5 ++
>  grub-core/loader/i386/pc/freedos.c |  82 
> +++++++++++++++++++++++++++++++++++--
>  include/grub/i386/relocator.h      |   1 +
>  5 files changed, 100 insertions(+), 5 deletions(-)
> 
> # HG changeset patch
> # User C. Masloch <address@hidden>
> # Date 1341734778 -7200
> # Node ID 3c587f7d6d1e80c73ec0c4a664feebb788f00ed1
> # Parent  73948d0f69334ef6b60e60c16389742c402c2f3e
> Improve FreeDOS direct loading support compatibility.
> 
> diff --git a/ChangeLog b/ChangeLog
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,3 +1,18 @@
> +2012-06-30  C. Masloch  <address@hidden>
> +
> +     Improve FreeDOS direct loading support compatibility.
> +
> +     * include/grub/i386/relocator.h (grub_relocator16_state):
> +     New member ebp.
> +     * grub-core/lib/i386/relocator.c (grub_relocator16_ebp): New extern
> +     variable.
> +     (grub_relocator16_boot): Handle %ebp.
> +     * grub-core/lib/i386/relocator16.S: Likewise.
> +     * grub-core/loader/i386/pc/freedos.c:
> +     Load BPB to pass kernel which partition to load from.
> +     Check that kernel file is not too large.
> +     Set register dl to BIOS unit number as well.
> +
>  2012-06-27  Vladimir Serbinenko  <address@hidden>
>  
>       * configure.ac: Bump version to 2.00.
> diff --git a/grub-core/lib/i386/relocator.c b/grub-core/lib/i386/relocator.c
> --- a/grub-core/lib/i386/relocator.c
> +++ b/grub-core/lib/i386/relocator.c
> @@ -54,6 +54,7 @@
>  extern grub_uint32_t grub_relocator16_edx;
>  extern grub_uint32_t grub_relocator16_ebx;
>  extern grub_uint32_t grub_relocator16_esi;
> +extern grub_uint32_t grub_relocator16_ebp;
>  
>  extern grub_uint16_t grub_relocator16_keep_a20_enabled;
>  
> @@ -225,6 +226,7 @@
>    grub_relocator16_ss = state.ss;
>    grub_relocator16_sp = state.sp;
>  
> +  grub_relocator16_ebp = state.ebp;
>    grub_relocator16_ebx = state.ebx;
>    grub_relocator16_edx = state.edx;
>    grub_relocator16_esi = state.esi;
> diff --git a/grub-core/lib/i386/relocator16.S 
> b/grub-core/lib/i386/relocator16.S
> --- a/grub-core/lib/i386/relocator16.S
> +++ b/grub-core/lib/i386/relocator16.S
> @@ -259,6 +259,11 @@
>  VARIABLE(grub_relocator16_ebx)
>       .long   0
>  
> +     /* movl imm32, %ebp.  */
> +     .byte   0x66, 0xbd
> +VARIABLE(grub_relocator16_ebp)
> +     .long   0
> +
>       /* Cleared direction flag is of no problem with any current
>          payload and makes this implementation easier.  */
>       cld
> diff --git a/grub-core/loader/i386/pc/freedos.c 
> b/grub-core/loader/i386/pc/freedos.c
> --- a/grub-core/loader/i386/pc/freedos.c
> +++ b/grub-core/loader/i386/pc/freedos.c
> @@ -32,6 +32,7 @@
>  #include <grub/video.h>
>  #include <grub/mm.h>
>  #include <grub/cpu/relocator.h>
> +#include <grub/machine/chainloader.h>
>  
>  GRUB_MOD_LICENSE ("GPLv3+");
>  
> @@ -40,8 +41,23 @@
>  static grub_uint32_t ebx = 0xffffffff;
>  
>  #define GRUB_FREEDOS_SEGMENT         0x60
> +#define GRUB_FREEDOS_ADDR            (GRUB_FREEDOS_SEGMENT << 4)
>  #define GRUB_FREEDOS_STACK_SEGMENT         0x1fe0
> -#define GRUB_FREEDOS_STACK_POINTER         0x8000
> +#define GRUB_FREEDOS_STACK_BPB_POINTER     0x7c00
> +#define GRUB_FREEDOS_BPB_ADDR        ((GRUB_FREEDOS_STACK_SEGMENT << 4) \
> +                                       + GRUB_FREEDOS_STACK_BPB_POINTER)
> +
> +/* FreeDOS boot.asm passes register sp as exactly this. Importantly,
> +   it must point below the BPB (to avoid overwriting any of it). */
> +#define GRUB_FREEDOS_STACK_POINTER         (GRUB_FREEDOS_STACK_BPB_POINTER \
> +                                             - 0x60)
> +
> +/* In this, the additional 8192 bytes are the stack reservation; the
> +   remaining parts trivially give the maximum allowed size. */
> +#define GRUB_FREEDOS_MAX_SIZE        ((GRUB_FREEDOS_STACK_SEGMENT << 4) \
> +                                       + GRUB_FREEDOS_STACK_POINTER \
> +                                       - GRUB_FREEDOS_ADDR \
> +                                       - 8192)
>  
>  static grub_err_t
>  grub_freedos_boot (void)
> @@ -49,14 +65,29 @@
>    struct grub_relocator16_state state = { 
>      .cs = GRUB_FREEDOS_SEGMENT,
>      .ip = 0,
> -    .ds = 0,
> +
> +    /* This is not strictly necessary for the current FreeDOS kernel
> +       but improves potential compatibility with others.
> +       There is no harm in setting this. */
> +    .ds = GRUB_FREEDOS_STACK_SEGMENT,
>      .es = 0,
>      .fs = 0,
>      .gs = 0,
>      .ss = GRUB_FREEDOS_STACK_SEGMENT,
>      .sp = GRUB_FREEDOS_STACK_POINTER,
> +    .ebp = GRUB_FREEDOS_STACK_BPB_POINTER,
>      .ebx = ebx,
> -    .edx = 0,
> +
> +    /* This is not strictly necessary for the current FreeDOS kernel
> +       but is crucial for potential compatibility with the load
> +       protocols of other DOS-like kernels and loaders (including
> +       the older FreeDOS kernel releases called DOS-C).
> +       (Among those, FreeDOS's new load protocol must be considered
> +       a special case in that it doesn't require register dl to pass
> +       the unit number. Incidentally, the current FreeDOS boot.asm
> +       does pass it in both registers.)
> +       There is no harm in setting this. */
> +    .edx = ebx,
>      .a20 = 1
>    };
>    grub_video_set_mode ("text", 0, 0);
> @@ -79,8 +110,9 @@
>  {
>    grub_file_t file = 0;
>    grub_err_t err;
> -  void *kernelsys;
> +  void *bs, *kernelsys;
>    grub_size_t kernelsyssize;
> +  grub_device_t dev;
>  
>    if (argc == 0)
>      return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("filename expected"));
> @@ -95,12 +127,52 @@
>    if (! file)
>      goto fail;
>  
> +  {
> +    grub_relocator_chunk_t ch;
> +    err = grub_relocator_alloc_chunk_addr (rel, &ch, GRUB_FREEDOS_BPB_ADDR,
> +                                        GRUB_DISK_SECTOR_SIZE);

I don't understand this. Shouldn't it be at 0x7c00 ?
Do you plan to contribute more? If so I'd prefer to make you sign a
copyright assignment, otherwise this patch can go in, once it's clear
with addresses.
-- 
Regards
Vladimir 'φ-coder/phcoder' Serbinenko

signature.asc
Description: OpenPGP digital signature