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

[Vladimir 'φ-coder/phcoder' Serbinenko]

Committed with minor problems fixed. Thanks.
On 08.07.2012 10:28, C. Masloch wrote:

> 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);
> +    if (err)
> +      goto fail;
> +    bs = get_virtual_current_address (ch);
> +  }
> +
>    ebx = grub_get_root_biosnumber ();
> +  dev = grub_device_open (0);
> +
> +  if (dev && dev->disk)
> +    {
> +      err = grub_disk_read (dev->disk, 0, 0, GRUB_DISK_SECTOR_SIZE, bs);
> +      if (err)
> +     {
> +       grub_device_close (dev);
> +       goto fail;
> +     }
> +      grub_chainloader_patch_bpb (bs, dev, ebx);
> +    }
> +
> +  if (dev)
> +    grub_device_close (dev);
>  
>    kernelsyssize = grub_file_size (file);
> +
> +  /* This check could be considered optional, but it provides a more
> +     specific error message than grub_relocator_alloc_chunk_addr would,
> +     and additionally it insures that a little is set aside for the
> +     initial stack as well.
> +     Quirkily, because of its size constraints FreeDOS's original loader
> +     doesn't perform such a check at all (and crashes instead). The file
> +     size limit is documented though. */
> +  if (kernelsyssize > GRUB_FREEDOS_MAX_SIZE)
> +    {
> +      grub_error (GRUB_ERR_BAD_OS,
> +               "file `%s' is too large for a valid"
> +               " FreeDOS kernel.sys", argv[0]);
> +      goto fail;
> +    }
> +
>    {
>      grub_relocator_chunk_t ch;
> -    err = grub_relocator_alloc_chunk_addr (rel, &ch, GRUB_FREEDOS_SEGMENT << 
> 4,
> +    err = grub_relocator_alloc_chunk_addr (rel, &ch, GRUB_FREEDOS_ADDR,
>                                          kernelsyssize);
>      if (err)
>        goto fail;
> diff --git a/include/grub/i386/relocator.h b/include/grub/i386/relocator.h
> --- a/include/grub/i386/relocator.h
> +++ b/include/grub/i386/relocator.h
> @@ -49,6 +49,7 @@
>    grub_uint32_t ebx;
>    grub_uint32_t edx;
>    grub_uint32_t esi;
> +  grub_uint32_t ebp;
>    int a20;
>  };
>  



-- 
Regards
Vladimir 'φ-coder/phcoder' Serbinenko

signature.asc
Description: OpenPGP digital signature