Re: [PATCH] Drivemap module

[Marco Gerards]

Javier Martín <address@hidden> writes:

> Just an updated version of the patch that adds support for device-like
> names instead of raw BIOS disk numbers, i.e. this is now supported:
>       grub> drivemap (hd0) (hd1)
> In addition to the already supported:
>       grub> drivemap (hd0) 0x81
> The effect is the same: the second BIOS hard drive will map to (hd0)
> through the installed int13h routine. The new syntax does not require
> the target device name to exist (hd1 need not exist in my example), and
> the parsing is very simple: it accepts names like (fdN) and (hdN) with
> and without parenthesis, and with N ranging from 0 to 127, thus allowing
> the full 0x00-0xFF range even though most BIOS-probing OSes don't bother
> going any further than fd7 and hd7 respectively.

Did you use code from other people or projects?

> For newcomers, full info on the patch is available on the list archives
> - it was proposed on June and its discussion deferred for "two or three
> weeks" because the developers were busy.


I have copied the changelog entry from your other e-mail:

        * commands/i386/pc/drivemap.c : New file, main part of the new
        drivemap module allowing BIOS drive remapping not unlike the
        legacy "map" command. This allows to boot OSes with boot-time
        dependencies on the particular ordering of BIOS drives or
        trusting their own to be 0x80, like Windows XP, with
        non-standard boot configurations.

"New file." would be sufficient

        * commands/i386/pc/drivemap_int13h.S : New file, INT 13h handler
        for the drivemap module. Installed as a TSR routine by
        drivemap.c, performs the actual redirection of BIOS drives.

Same here.

        * conf/i386-pc.rmk : Added the new module

Please say which variables you added in this file.  You can find some
examples on how to do this in the ChangeLog file.

        * include/grub/loader.h : Added a "just-before-boot" callback
        infrastructure used by drivemap.mod to install the INT13 handler
        only when the "boot" command has been issued.

Please describe changes, not effects.  So which prototypes and macros
did you add?

        * kern/loader.c : Implement the preboot-hook described

Which functions did you change and how?  Please describe actual changes.

The header is missing, please include it.  Also newlines between the
files make it easier to read.


Here follows a review.  Sorry I kept you waiting for this long, this
feature and your work is really appreciated!  Perhaps I can spot some
more problems after you fixed it and supplied an updated changelog
entry.  There are quite some comments, but please do not let this
demotivate you, it is mainly coding style related :-)

> Index: commands/i386/pc/drivemap.c
> ===================================================================
> RCS file: commands/i386/pc/drivemap.c
> diff -N commands/i386/pc/drivemap.c
> --- /dev/null 1 Jan 1970 00:00:00 -0000
> +++ commands/i386/pc/drivemap.c       2 Jul 2008 01:12:36 -0000
> @@ -0,0 +1,391 @@
> +/* drivemap.c - command to manage the BIOS drive mappings.  */
> +/*
> + *  GRUB  --  GRand Unified Bootloader
> + *  Copyright (C) 2008  Free Software Foundation, Inc.
> + *
> + *  GRUB is free software: you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License as published by
> + *  the Free Software Foundation, either version 3 of the License, or
> + *  (at your option) any later version.
> + *
> + *  GRUB is distributed in the hope that it will be useful,
> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + *  GNU General Public License for more details.
> + *
> + *  You should have received a copy of the GNU General Public License
> + *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <grub/normal.h>
> +#include <grub/dl.h>
> +#include <grub/mm.h>
> +#include <grub/misc.h>
> +#include <grub/disk.h>
> +#include <grub/loader.h>
> +#include <grub/machine/loader.h>
> +#include <grub/machine/biosdisk.h>
> +
> +/* Uncomment the following line to enable debugging output */
> +/* #define DRIVEMAP_DEBUG */
> +
> +#ifdef DRIVEMAP_DEBUG
> +# define DBG_PRINTF(...) grub_printf(__VA_ARGS__)
> +#else
> +# define DBG_PRINTF(...)
> +#endif

Please use the grub_dprintf infrastructure.

> +static const struct grub_arg_option options[] = {
> +  {"list", 'l', 0, "show the current mappings", 0, 0},
> +  {"reset", 'r', 0, "reset all mappings to the default values", 0, 0},
> +  {0, 0, 0, 0, 0, 0}
> +};
> +
> +/* ASSEMBLY SYMBOLS/VARS/FUNCS - start */

Useless comment

> +/* realmode far ptr = 2 * 16b */
> +extern grub_uint32_t EXPORT_VAR (grub_drivemap_int13_oldhandler);
> +/* Size of the section to be copied */
> +extern grub_uint16_t EXPORT_VAR (grub_drivemap_int13_size);
> +
> +/* NOT a typo - just need the symbol's address with &symbol */
> +typedef void grub_symbol_t;
> +extern grub_symbol_t EXPORT_VAR (grub_drivemap_int13_handler_base);
> +extern grub_symbol_t EXPORT_VAR (grub_drivemap_int13_mapstart);

Please export stuff in header files, that's the normal practise in
this file as well, right?  What's not a typo?

> +void EXPORT_FUNC (grub_drivemap_int13_handler) (void);
> +
> +/* ASSEMBLY SYMBOLS/VARS/FUNCS - end */

Like before.

> +static grub_preboot_hookid insthandler_hook = 0;

You can leave the "= 0" away.

> +typedef struct drivemap_node
> +{
> +  grub_uint8_t newdrive;
> +  grub_uint8_t redirto;
> +  struct drivemap_node *next;
> +} drivemap_node_t;
> +
> +static drivemap_node_t *drivemap = 0;

Same here :-)

> +static grub_err_t install_int13_handler (void);
> +
> +static grub_err_t
> +drivemap_set (grub_uint8_t newdrive, grub_uint8_t redirto)
> +  /* Puts the specified mapping into the table, replacing an existing mapping
> +   * for newdrive or adding a new one if required. */

Please place the comments before the function.

> +{
> +  drivemap_node_t *mapping = 0, *search = drivemap;
> +  while (search)
> +    {
> +      if (search->newdrive == newdrive)
> +        {
> +          mapping = search;
> +          break;
> +        }
> +      search = search->next;
> +    }
> +  if (mapping)  /* There was a mapping already in place, modify it */
> +    mapping->redirto = redirto;

Please place the comment before the if statement.  Can you format the
comment such that it begins with a capital and ends with ".  */" (two
spaces)?  Can you do this for the other comments you added as well?

> +  else  /* Create a new mapping and add it to the head of the list */
> +    {

I would move the comment inside the braces.

> +      mapping = grub_malloc (sizeof (drivemap_node_t));
> +      if (!mapping)
> +        return grub_error (GRUB_ERR_OUT_OF_MEMORY,
> +                           "cannot allocate map entry, not enough memory");
> +      mapping->newdrive = newdrive;
> +      mapping->redirto = redirto;
> +      mapping->next = drivemap;
> +      drivemap = mapping;
> +    }
> +  return GRUB_ERR_NONE;
> +}
> +
> +static void
> +drivemap_remove (grub_uint8_t newdrive)
> +  /* Removes the mapping for newdrive from the table. If there is no mapping,
> +   * then this function behaves like a no-op on the map. */

Can you move the comments to before the function?  Please do not place
*'s on each line.  Without the second * it would be fine.  After a
".", please insert two spaces.

> +{
> +  drivemap_node_t *mapping = 0, *search = drivemap, *previous = 0;

Please define one variable per line, can you break this down to 3
lines?

> +  while (search)
> +    {
> +      if (search->newdrive == newdrive)
> +        {
> +          mapping = search;
> +          break;
> +        }
> +      previous = search;
> +      search = search->next;
> +    }
> +  if (mapping) /* Found */
> +    {
> +      if (previous)
> +        previous->next = mapping->next;
> +      else drivemap = mapping->next; /* Entry was head of list */

Please place the stuff after "else" on a new line.

> +      grub_free (mapping);
> +    }
> +}
> +
> +static grub_err_t
> +parse_biosdisk (const char *name, grub_uint8_t *disknum)
> +{
> +  if (!name) return GRUB_ERR_BAD_ARGUMENT;

Same here.  Please do not just return GRUB_ERR_BAD_ARGUMENT, use
grub_error so you can also define a string for the error.

> +  if (*name == '(')
> +    name++; /* Skip the first ( in (hd0) - disk_open wants just the name! */
> +  grub_disk_t disk = grub_disk_open (name);

Although mixed declarations and code are allowed, I personally would
avoid this.  It doesn't make the code easier to read, I think.

> +  if (!disk)
> +    return GRUB_ERR_UNKNOWN_DEVICE;

Use grub_error.  Or actually return grub_errno, since if
grub_disk_open failed it would have set this.

> +  else
> +    {
> +      enum grub_disk_dev_id id = disk->dev->id;
> +      if (disknum)
> +        *disknum = disk->id;   /* Only valid, of course if it's a biosdisk */

Please place the comment before the if.

> +      grub_disk_close (disk);
> +      return (GRUB_DISK_DEVICE_BIOSDISK_ID != id) ?
> +          GRUB_ERR_BAD_DEVICE : GRUB_ERR_NONE;

Please use grub_error.

> +    }
> +}
> +
> +static grub_err_t
> +revparse_biosdisk(const grub_uint8_t dnum, const char **output)
> +{
> +  grub_err_t ret = GRUB_ERR_UNKNOWN_DEVICE;
> +  auto int find (const char *name);
> +  int find (const char *name)
> +  {
> +    grub_disk_t disk = grub_disk_open (name);
> +    if (!disk)
> +      return 0;
> +    else
> +      {
> +        int found = 0;
> +        if (dnum == disk->id && GRUB_DISK_DEVICE_BIOSDISK_ID == 
> disk->dev->id)


This is not rally wrong, but doesn't feel right.  I would use:

disk->dev->id == GRUB_DISK_DEVICE_BIOSDISK_ID

I know, I am a pain in the ass right now :P

> +          {
> +            found = 1;
> +            if (output)
> +              *output = name;
> +            ret = GRUB_ERR_NONE;
> +          }
> +        grub_disk_close (disk);
> +        return found;
> +      }
> +  }
> +
> +  grub_disk_dev_iterate (&find);

No need for the &.  I even wonder if it would work this way...

> +  return ret;
> +}
> +
> +static grub_err_t
> +tryparse_diskstring (const char *str, grub_uint8_t *output)
> +{
> +  if (!str) return GRUB_ERR_BAD_ARGUMENT;
> +  if (*str == '(')
> +    str++;  /* Skip opening paren in order to allow both (hd0) and hd0 */
> +  if ((str[0] == 'f' || str[0] == 'h') && str[1] == 'd')
> +    {
> +      grub_uint8_t bios_num = (str[0] == 'h')? 0x80 : 0x00;
> +      grub_errno = GRUB_ERR_NONE;
> +      unsigned long drivenum = grub_strtoul (str + 2, &str, 0);
> +      if (grub_errno != GRUB_ERR_NONE || drivenum > 127)
> +        return GRUB_ERR_BAD_ARGUMENT; /* Could not convert, or num too high 
> for BIOS */

Use grub_error

> +      else
> +       {
> +          bios_num |= drivenum;
> +          if (output)
> +            *output = bios_num;
> +          return GRUB_ERR_NONE;
> +        }
> +    }
> +  else return GRUB_ERR_BAD_ARGUMENT;
> +}
> +
> +static grub_err_t
> +grub_cmd_drivemap (struct grub_arg_list *state, int argc, char **args)
> +{
> +  if (state[0].set) /* Show: list mappings */

Like the comments before

> +    {
> +      if (!drivemap)
> +        grub_printf ("No drives have been remapped");
> +      else
> +        {
> +          grub_printf ("Showing only remapped drives. Drives that have had "
> +                       "their slot assigned to another one and have not been 
> "
> +                       "themselves remapped will become inaccessible through 
> "
> +                       "the BIOS routines to the booted OS.\n\n");

I do not think this message is immediatly clear to the user.  Perhaps
we should even leave this out and say something in the to-be-written
manual? :-)

> +          grub_printf ("Mapped\tGRUB\n");
> +          drivemap_node_t *curnode = drivemap;
> +          while (curnode)
> +            {
> +              const char *dname = 0;
> +              grub_err_t err = revparse_biosdisk (curnode->redirto, &dname);
> +              if (err != GRUB_ERR_NONE)
> +                return grub_error (err, "invalid mapping: non-existent disk"
> +                                        "or not managed by the BIOS");
> +              grub_printf("0x%02x\t%4s\n", curnode->newdrive, dname);
> +              curnode = curnode->next;
> +            }
> +        }
> +    }
> +  else if (state[1].set) /* Reset: just delete all mappings */
> +    {
> +      if (drivemap)
> +        {
> +          drivemap_node_t *curnode = drivemap, *prevnode = 0;
> +          while (curnode)
> +            {
> +              prevnode = curnode;
> +              curnode = curnode->next;
> +              grub_free (prevnode);
> +            }
> +          drivemap = 0;
> +        }
> +    }
> +  else
> +    {
> +      if (argc != 2)
> +        return grub_error (GRUB_ERR_BAD_ARGUMENT, "two arguments required");
> +      grub_uint8_t mapfrom = 0;

Please do not use mixed code/declarations here.

> +      grub_err_t err = parse_biosdisk (args[0], &mapfrom);
> +      if (err != GRUB_ERR_NONE)
> +        return grub_error (err, "invalid disk or not managed by the BIOS");
> +
> +      grub_uint8_t mapto = 0xFF;
> +      err = tryparse_diskstring (args[1], &mapto);
> +      if (err != GRUB_ERR_NONE) /* Not a disk string. Maybe a raw num then? 
> */
> +        {    
> +          grub_errno = GRUB_ERR_NONE;
> +          unsigned long num = grub_strtoul (args[1], 0, 0);
> +          if (grub_errno != GRUB_ERR_NONE || num > 0xFF)  /* Not a raw num 
> or too high */
> +            return grub_error (grub_errno,
> +                              "Target specifier must be of the form (fdN) or 
> "
> +                              "(hdN), with 0 <= N < 128; or a plain dec/hex "
> +                              "number between 0 and 255");
> +          else mapto = (grub_uint8_t)num;
> +        }

Do we really want to support BIOS disk numbers here?  I do not think
it is useful.

> +      if (mapto == mapfrom)  /* Reset to default */
> +        {
> +          DBG_PRINTF ("Removing the mapping for %s (%02x)", args[0], 
> mapfrom);
> +          drivemap_remove (mapfrom);
> +        }
> +      else  /* Map */
> +        {
> +          DBG_PRINTF ("Mapping %s (%02x) to %02x\n", args[0], mapfrom, 
> mapto);
> +          return drivemap_set ((grub_uint8_t)mapto, mapfrom);
> +        }

Please use grub_dprintf

> +    }
> +
> +  return GRUB_ERR_NONE;
> +}
> +
> +typedef struct __attribute__ ((packed)) int13map_node
> +{
> +  grub_uint8_t disknum;
> +  grub_uint8_t mapto;
> +} int13map_node_t;
> +
> +/* The min amount of mem that must remain free after installing the handler.
> + * 32 KiB is just above 0x7C00-0x7E00, where the bootsector is loaded.  */
> +#define MIN_FREE_MEM_KB 32
> +#define INT13H_OFFSET(x) ( ((grub_uint8_t*)(x)) - 
> ((grub_uint8_t*)&grub_drivemap_int13_handler_base) )
> +#define INT13H_REBASE(x) ( (void*) (((grub_uint8_t*)handler_base) + (x)) )
> +#define INT13H_TONEWADDR(x) INT13H_REBASE( INT13H_OFFSET( x ) )
> +/* Int13h handler installer - reserves conventional memory for the handler,
> + * copies it over and sets the IVT entry for int13h.
> + * This code rests on the assumption that GRUB does not activate any kind of
> + * memory mapping, since it accesses realmode structures by their absolute
> + * addresses, like the IVT at 0 or the BDA at 0x400 */

Please do not use all these *'s

Can you add newlines between the #defines and the comments, this will
get quite messy like this.

> +static grub_err_t
> +install_int13_handler (void)
> +{
> +  grub_size_t entries = 0;
> +  drivemap_node_t *curentry = drivemap;
> +  while (curentry)  /* Count entries to prepare a contiguous map block */

Same like above.  Can you change the place of comments everywhere?


> +    {
> +      entries++;
> +      curentry = curentry->next;
> +    }
> +  if (0 == entries)

if (! entries)

> +    {
> +      DBG_PRINTF ("drivemap: No drives marked as remapped, installation of"
> +                  "an int13h handler is not required.");
> +      return GRUB_ERR_NONE;  /* No need to install the int13h handler */
> +    }
> +  else
> +    {
> +      DBG_PRINTF ("drivemap: Installing int13h handler...\n");
> +      grub_uint32_t *ivtslot = (grub_uint32_t*)0x0000004c;
> +      
> +      /* Save the pointer to the old int13h handler */
> +      grub_drivemap_int13_oldhandler = *ivtslot;
> +      DBG_PRINTF ("Old int13 handler at %04x:%04x\n",
> +                  (grub_drivemap_int13_oldhandler >> 16) & 0x0ffff,
> +                  grub_drivemap_int13_oldhandler & 0x0ffff);
> +
> +      /* Reserve a section of conventional memory as "BIOS memory" for 
> handler:
> +       * BDA offset 0x13 contains the top of such memory */
> +      grub_uint16_t *bpa_freekb = (grub_uint16_t*)0x00000413;
> +      DBG_PRINTF ("Top of conventional memory: %u KiB\n", *bpa_freekb);
> +      grub_size_t total_size = grub_drivemap_int13_size
> +                            + (entries + 1) * sizeof(int13map_node_t);
> +      grub_uint16_t payload_sizekb = (total_size >> 10) +
> +                                    (((total_size % 1024) == 0) ? 0 : 1);
> +      if ((*bpa_freekb - payload_sizekb) < MIN_FREE_MEM_KB)
> +        return grub_error (GRUB_ERR_OUT_OF_MEMORY, "refusing to install"
> +                           "int13 handler, not enough free memory after");
> +      DBG_PRINTF ("Payload is %u b long, reserving %u Kb\n", total_size,
> +                  payload_sizekb);
> +      *bpa_freekb -= payload_sizekb;
> +
> +      /* Copy int13h handler chunk to reserved area */
> +      grub_uint8_t *handler_base = (grub_uint8_t*)(*bpa_freekb << 10);
> +      DBG_PRINTF ("Copying int13 handler to: %p\n", handler_base);
> +      grub_memcpy (handler_base, &grub_drivemap_int13_handler_base,
> +                   grub_drivemap_int13_size);
> +
> +      /* Copy the mappings to the reserved area */
> +      curentry = drivemap;
> +      grub_size_t i;
> +      int13map_node_t *handler_map = (int13map_node_t*)
> +                      INT13H_TONEWADDR (&grub_drivemap_int13_mapstart);
> +      DBG_PRINTF ("Target map at %p, copying mappings...\n", handler_map);
> +      for (i = 0; i < entries && curentry; i++, curentry = curentry->next)
> +        {
> +          handler_map[i].disknum = curentry->newdrive;
> +          handler_map[i].mapto = curentry->redirto;
> +          DBG_PRINTF ("\t#%d: 0x%02x <- 0x%02x\n", i, handler_map[i].disknum,
> +                      handler_map[i].mapto);
> +        }
> +      /* Signal end-of-map */
> +      handler_map[i].disknum = 0;
> +      handler_map[i].mapto = 0;
> +      DBG_PRINTF ("\t#%d: 0x%02x <- 0x%02x (end)\n", i, 
> handler_map[i].disknum,
> +                  handler_map[i].mapto);
> +
> +      /* Install our function as the int13h handler in the IVT */
> +      grub_uint32_t ivtentry = ((grub_uint32_t)handler_base) << 12; /* 
> Segment address */
> +      ivtentry |= (grub_uint16_t) INT13H_OFFSET(grub_drivemap_int13_handler);
> +      DBG_PRINTF ("New int13 handler IVT pointer: %04x:%04x\n",
> +                  (ivtentry >> 16) & 0x0ffff, ivtentry & 0x0ffff);
> +      *ivtslot = ivtentry;
> +      
> +      return GRUB_ERR_NONE;
> +    }
> +}
> +
> +GRUB_MOD_INIT (drivemap)
> +{
> +  (void) mod;                        /* To stop warning. */
> +  grub_register_command ("drivemap", grub_cmd_drivemap,
> +                         GRUB_COMMAND_FLAG_BOTH,
> +                                        "drivemap -s | -r | (hdX) 
> newdrivenum",
> +                         "Manage the BIOS drive mappings", options);
> +  insthandler_hook = grub_loader_register_preboot (&install_int13_handler, 
> 1);
> +}
> +
> +GRUB_MOD_FINI (drivemap)
> +{
> +  grub_loader_unregister_preboot (insthandler_hook);
> +  insthandler_hook = 0;
> +  grub_unregister_command ("drivemap");
> +}

I wonder if this is not called, just before GRUB boots a kernel.  That
would cause a problem :-)

> Index: commands/i386/pc/drivemap_int13h.S
> ===================================================================
> RCS file: commands/i386/pc/drivemap_int13h.S
> diff -N commands/i386/pc/drivemap_int13h.S
> --- /dev/null 1 Jan 1970 00:00:00 -0000
> +++ commands/i386/pc/drivemap_int13h.S        2 Jul 2008 01:12:36 -0000
> @@ -0,0 +1,122 @@
> +/*
> + *  GRUB  --  GRand Unified Bootloader
> + *  Copyright (C) 1999,2000,2001,2002,2003,2005,2006,2007,2008 Free Software 
> Foundation, Inc.
> + *
> + *  GRUB is free software: you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License as published by
> + *  the Free Software Foundation, either version 3 of the License, or
> + *  (at your option) any later version.
> + *
> + *  GRUB is distributed in the hope that it will be useful,
> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + *  GNU General Public License for more details.
> + *
> + *  You should have received a copy of the GNU General Public License
> + *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +
> +/*
> + * Note: These functions defined in this file may be called from C.
> + *       Be careful of that you must not modify some registers. Quote
> + *       from gcc-2.95.2/gcc/config/i386/i386.h:
> +
> +   1 for registers not available across function calls.
> +   These must include the FIXED_REGISTERS and also any
> +   registers that can be used without being saved.
> +   The latter must include the registers where values are returned
> +   and the register where structure-value addresses are passed.
> +   Aside from that, you can include as many other registers as you like.
> +
> +  ax,dx,cx,bx,si,di,bp,sp,st,st1,st2,st3,st4,st5,st6,st7,arg
> +{  1, 1, 1, 0, 0, 0, 0, 1, 1,  1,  1,  1,  1,  1,  1,  1,  1 }
> + */
> +
> +/*
> + * Note: GRUB is compiled with the options -mrtd and -mregparm=3.
> + *       So the first three arguments are passed in %eax, %edx, and %ecx,
> + *       respectively, and if a function has a fixed number of arguments
> + *       and the number if greater than three, the function must return
> + *       with "ret $N" where N is ((the number of arguments) - 3) * 4.
> + */
>

I do not think this is required.  I mean, we know how GRUB is compiled
and how it deals with registers.

> +/*
> + *  This is the area for all of the special variables.
> + */

I don't think this comment is useful.

> +#include <grub/symbol.h>
> +
> +#define GRUB_DRIVEMAP_INT13H_OFFSET(x) ((x) - 
> grub_drivemap_int13_handler_base)
> +
> +/* Copy starts here. When deployed, this label must be segment-aligned */
> +VARIABLE(grub_drivemap_int13_handler_base)
> +
> +VARIABLE(grub_drivemap_int13_oldhandler)
> +  .word 0xdead, 0xbeef
> +/* Drivemap module - INT 13h handler - BIOS HD map */
> +/* We need to use relative addressing, and with CS to top it all, since we
> + * must make as few changes to the registers as possible. Pity we're not on
> + * amd64: rIP-relative addressing would make life easier here.
> + */
> +.code16
> +FUNCTION(grub_drivemap_int13_handler)
> +  push %bp
> +  mov %sp, %bp
> +  push %ax  /* We'll need it later to determine the used BIOS function */
> +
> +  /* Map the drive number (always in DL?) */
> +  push %ax
> +  push %bx
> +  push %si
> +  mov $GRUB_DRIVEMAP_INT13H_OFFSET(grub_drivemap_int13_mapstart), %bx
> +  xor %si, %si
> +1:movw %cs:(%bx,%si), %ax
> +  cmp %ah, %al
> +  jz 3f /* DRV=DST => map end - drive not remapped, leave DL as-is */
> +  cmp %dl, %al
> +  jz 2f /* Found - drive remapped, modify DL */
> +  add $2, %si
> +  jmp 1b /* Not found, but more remaining, loop  */
> +2:mov %ah, %dl
> +3:pop %si
> +  pop %bx
> +  xchgw %ax, -4(%bp) /* Recover the old AX and save the map entry for later 
> */
> +  
> +  push %bp
> +  /* Simulate interrupt call: push flags and do a far call in order to set
> +   * the stack the way the old handler expects it so that its iret works */
> +  push 6(%bp)
> +  movw (%bp), %bp  /* Restore the caller BP (is this needed and/or 
> sensible?) */
> +  lcall *%cs:GRUB_DRIVEMAP_INT13H_OFFSET(grub_drivemap_int13_oldhandler)
> +  pop %bp /* The pushed flags were removed by iret */
> +  /* Set the saved flags to what the int13h handler returned */
> +  push %ax
> +  pushf
> +  pop %ax
> +  movw %ax, 6(%bp)
> +  pop %ax
> +
> +  /* Reverse map any returned drive number if the data returned includes it.
> +   * The only func that does this seems to be origAH = 0x08, but many BIOS
> +   * refs say retDL = # of drives connected. However, the GRUB Legacy code
> +   * treats this as the _drive number_ and "undoes" the remapping. Thus,
> +   * this section has been disabled for testing if it's required */
> +#  cmpb $0x08, -1(%bp) /* Caller's AH */
> +#  jne 4f
> +#  xchgw %ax, -4(%bp) /* Map entry used */
> +#  cmp %ah, %al  /* DRV=DST => drive not remapped */
> +#  je 4f
> +#  mov %ah, %dl  /* Undo remap */
> +
> +4:mov %bp, %sp
> +  pop %bp
> +  iret
> +/* This label MUST be at the end of the copied block, since the installer 
> code
> + * reserves additional space for mappings at runtime and copies them over it 
> */
> +.align 2
> +VARIABLE(grub_drivemap_int13_mapstart)
> +/* Copy stops here */
> +.code32
> +VARIABLE(grub_drivemap_int13_size)
> +  .word GRUB_DRIVEMAP_INT13H_OFFSET(grub_drivemap_int13_size)
> Index: conf/i386-pc.rmk
> ===================================================================
> RCS file: /sources/grub/grub2/conf/i386-pc.rmk,v
> retrieving revision 1.120
> diff -u -r1.120 i386-pc.rmk
> --- conf/i386-pc.rmk  19 Jun 2008 05:14:15 -0000      1.120
> +++ conf/i386-pc.rmk  2 Jul 2008 01:12:44 -0000
> @@ -153,7 +153,7 @@
>       vbe.mod vbetest.mod vbeinfo.mod video.mod gfxterm.mod \
>       videotest.mod play.mod bitmap.mod tga.mod cpuid.mod serial.mod  \
>       ata.mod vga.mod memdisk.mod jpeg.mod png.mod pci.mod lspci.mod \
> -     aout.mod _bsd.mod bsd.mod
> +     aout.mod _bsd.mod bsd.mod drivemap.mod
>  
>  # For biosdisk.mod.
>  biosdisk_mod_SOURCES = disk/i386/pc/biosdisk.c
> @@ -320,4 +320,11 @@
>  bsd_mod_CFLAGS = $(COMMON_CFLAGS)
>  bsd_mod_LDFLAGS = $(COMMON_LDFLAGS)
>  
> +# For drivemap.mod.
> +drivemap_mod_SOURCES = commands/i386/pc/drivemap.c \
> +                       commands/i386/pc/drivemap_int13h.S
> +drivemap_mod_ASFLAGS = $(COMMON_ASFLAGS)
> +drivemap_mod_CFLAGS = $(COMMON_CFLAGS)
> +drivemap_mod_LDFLAGS = $(COMMON_LDFLAGS)
> +
>  include $(srcdir)/conf/common.mk
> Index: include/grub/loader.h
> ===================================================================
> RCS file: /sources/grub/grub2/include/grub/loader.h,v
> retrieving revision 1.9
> diff -u -r1.9 loader.h
> --- include/grub/loader.h     21 Jul 2007 23:32:22 -0000      1.9
> +++ include/grub/loader.h     2 Jul 2008 01:12:45 -0000
> @@ -37,6 +37,19 @@
>  /* Unset current loader, if any.  */
>  void EXPORT_FUNC(grub_loader_unset) (void);
>  
> +typedef struct hooklist_node *grub_preboot_hookid;
> +
> +/* Register a function to be called before the boot hook. Returns an id that
> +   can be later used to unregister the preboot (i.e. if module unloaded). If
> +   abort_on_error is set, the boot sequence will abort if any of the 
> registered
> +   functions return anything else than GRUB_ERR_NONE */
> +grub_preboot_hookid EXPORT_FUNC(grub_loader_register_preboot)
> +           (grub_err_t (*hook) (void), int abort_on_error);
> +
> +/* Unregister a preboot hook by the id returned by loader_register_preboot.
> +   This functions becomes a no-op if no such function is registered */
> +void EXPORT_FUNC(grub_loader_unregister_preboot) (grub_preboot_hookid id);
> +
>  /* Call the boot hook in current loader. This may or may not return,
>     depending on the setting by grub_loader_set.  */
>  grub_err_t EXPORT_FUNC(grub_loader_boot) (void);
> Index: kern/loader.c
> ===================================================================
> RCS file: /sources/grub/grub2/kern/loader.c,v
> retrieving revision 1.9
> diff -u -r1.9 loader.c
> --- kern/loader.c     21 Jul 2007 23:32:26 -0000      1.9
> +++ kern/loader.c     2 Jul 2008 01:12:45 -0000
> @@ -61,11 +61,78 @@
>    grub_loader_loaded = 0;
>  }
>  
> +struct hooklist_node
> +{
> +  grub_err_t (*hook) (void);
> +  int abort_on_error;
> +  struct hooklist_node *next;
> +};

Isn't there something 

> +static struct hooklist_node *preboot_hooks = 0;
> +
> +grub_preboot_hookid
> +grub_loader_register_preboot(grub_err_t (*hook) (void), int abort_on_error)
> +{
> +  if (0 == hook)

if (! hook)

> +    {
> +      grub_error (GRUB_ERR_BAD_ARGUMENT, "preboot hoook must not be null");

hoook

null -> NULL

> +      return 0;
> +    }
> +  grub_preboot_hookid newentry = grub_malloc (sizeof (struct hooklist_node));
> +  if (!newentry)
> +    {
> +      grub_error (GRUB_ERR_OUT_OF_MEMORY, "cannot alloc a hookinfo 
> structure");
> +      return 0;
> +    }
> +  else
> +    {
> +      newentry->hook = hook;
> +      newentry->abort_on_error = abort_on_error;
> +      newentry->next = preboot_hooks;
> +      preboot_hooks = newentry;
> +      return newentry;
> +    }
> +}
> +
> +void
> +grub_loader_unregister_preboot(grub_preboot_hookid id)
> +{
> +  if (0 == id)
> +    return;
> +  grub_preboot_hookid entry = 0, search = preboot_hooks, previous = 0;
>
Can you move this up so this is not mixed.  Can you split this into 3 lines?

 +  while (search)
> +    {
> +      if (search == id)
> +        {
> +          entry = search;
> +          break;
> +        }
> +      previous = search;
> +      search = search->next;
> +    }
> +  if (entry) /* Found */
> +    {
> +      if (previous)
> +        previous->next = entry->next;
> +      else preboot_hooks = entry->next; /* Entry was head of list */
> +      grub_free (entry);
> +    }
> +}
> +
>  grub_err_t
>  grub_loader_boot (void)
>  {
>    if (! grub_loader_loaded)
>      return grub_error (GRUB_ERR_NO_KERNEL, "no loaded kernel");
> +  
> +  grub_preboot_hookid entry = preboot_hooks;
> +  while (entry)
> +    {
> +      grub_err_t possible_error = entry->hook();

Just use "err"

> +      if (possible_error != GRUB_ERR_NONE && entry->abort_on_error)
> +        return possible_error;
> +      entry = entry->next;
> +    }
>  
>    if (grub_loader_noreturn)
>      grub_machine_fini ();
> _______________________________________________
> Grub-devel mailing list
> address@hidden
> http://lists.gnu.org/mailman/listinfo/grub-devel