All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] Drivemap module
@ 2008-07-04  2:12 Javier Martín
  2008-07-05 11:04 ` Marco Gerards
  2008-07-20 19:40 ` Marco Gerards
  0 siblings, 2 replies; 47+ messages in thread
From: Javier Martín @ 2008-07-04  2:12 UTC (permalink / raw)
  To: The development of GRUB 2


[-- Attachment #1.1: Type: text/plain, Size: 911 bytes --]

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.

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.

[-- Attachment #1.2: drivemap.patch --]
[-- Type: text/x-patch, Size: 24356 bytes --]

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
+
+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 */
+
+/* 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);
+
+void EXPORT_FUNC (grub_drivemap_int13_handler) (void);
+
+/* ASSEMBLY SYMBOLS/VARS/FUNCS - end */
+
+static grub_preboot_hookid insthandler_hook = 0;
+
+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;
+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. */
+{
+  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;
+  else  /* Create a new mapping and add it to the head of the list */
+    {
+      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. */
+{
+  drivemap_node_t *mapping = 0, *search = drivemap, *previous = 0;
+  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 */
+      grub_free (mapping);
+    }
+}
+
+static grub_err_t
+parse_biosdisk (const char *name, grub_uint8_t *disknum)
+{
+  if (!name) return GRUB_ERR_BAD_ARGUMENT;
+  if (*name == '(')
+    name++; /* Skip the first ( in (hd0) - disk_open wants just the name! */
+  grub_disk_t disk = grub_disk_open (name);
+  if (!disk)
+    return GRUB_ERR_UNKNOWN_DEVICE;
+  else
+    {
+      enum grub_disk_dev_id id = disk->dev->id;
+      if (disknum)
+        *disknum = disk->id;   /* Only valid, of course if it's a biosdisk */
+      grub_disk_close (disk);
+      return (GRUB_DISK_DEVICE_BIOSDISK_ID != id) ?
+          GRUB_ERR_BAD_DEVICE : GRUB_ERR_NONE;
+    }
+}
+
+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)
+          {
+            found = 1;
+            if (output)
+              *output = name;
+            ret = GRUB_ERR_NONE;
+          }
+        grub_disk_close (disk);
+        return found;
+      }
+  }
+
+  grub_disk_dev_iterate (&find);
+  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 */
+      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 */
+    {
+      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");
+          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;
+      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;
+        }
+      
+      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);
+        }
+    }
+
+  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 */
+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 */
+    {
+      entries++;
+      curentry = curentry->next;
+    }
+  if (0 == 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");
+}
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.
+ */
+
+/*
+ *  This is the area for all of the special variables.
+ */
+
+#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;
+};
+
+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)
+    {
+      grub_error (GRUB_ERR_BAD_ARGUMENT, "preboot hoook must not be 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;
+  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();
+      if (possible_error != GRUB_ERR_NONE && entry->abort_on_error)
+        return possible_error;
+      entry = entry->next;
+    }
 
   if (grub_loader_noreturn)
     grub_machine_fini ();

[-- Attachment #2: Esta parte del mensaje está firmada digitalmente --]
[-- Type: application/pgp-signature, Size: 827 bytes --]

^ permalink raw reply	[flat|nested] 47+ messages in thread
* Re: [PATCH] Drivemap module
@ 2008-08-08 13:43 Viswesh S
  0 siblings, 0 replies; 47+ messages in thread
From: Viswesh S @ 2008-08-08 13:43 UTC (permalink / raw)
  To: The development of GRUB 2

[-- Attachment #1: Type: text/plain, Size: 1707 bytes --]





----- Original Message ----
From: Felix Zielcke <fzielcke@z-51.de>
To: The development of GRUB 2 <grub-devel@gnu.org>
Sent: Friday, 8 August, 2008 6:50:12 PM
Subject: Re: [PATCH] Drivemap module

Am Mittwoch, den 06.08.2008, 19:31 +0200 schrieb Javier Martín:

> This will work on Windows, because no matter where it tries to boot from
> it will find the same disk: both the first and second BIOS disks point
> to hd1 now. 
> ...
> This is the setup I have
> extensively tested in QEMU, while the Windows boot has been tested in
> both QEMU (with ReactOS) and some random computers including mine (with
> Windows XP Home and Windows XP Pro x64).
> 

Even though I don't need your drivemap at all (see one of my replies to
Marco's ATA topic) I was so kind to try this out with my Vista x64
SP1 ;)

I haven't remembered exactly the 2 menuentrys you wrote in your mail so
I used a mixture of both :)

drivemap (hd1) (hd0)
set root=(hd0)
chainloader (hd1,1)+1

This booted fine for me, GRUB loaded from IDE disk and Vista chainloaded
from my SATA RAID 0.

I really hope that you get this commited, then there's one feature less
missing from grub-legacy ;)

Hi,
Nice to know that the patch is working..for others also.meanwhile I am not too much sure,why the same is not happening to me.
The only difference in this case is that , my linux is in sata and my windows is in IDE.
Viswesh

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel



      Unlimited freedom, unlimited storage. Get it now, on http://help.yahoo.com/l/in/yahoo/mail/yahoomail/tools/tools-08.html/

[-- Attachment #2: Type: text/html, Size: 2897 bytes --]

^ permalink raw reply	[flat|nested] 47+ messages in thread
* Re: [PATCH] Drivemap module
@ 2008-08-07 12:15 Viswesh S
  0 siblings, 0 replies; 47+ messages in thread
From: Viswesh S @ 2008-08-07 12:15 UTC (permalink / raw)
  To: The development of GRUB 2

[-- Attachment #1: Type: text/plain, Size: 4793 bytes --]





From: Javier Martín <lordhabbit@gmail.com>
To: The development of GRUB 2 <grub-devel@gnu.org>
Sent: Wednesday, 6 August, 2008 11:01:19 PM
Subject: Re: [PATCH] Drivemap module

El mié, 06-08-2008 a las 07:43 -0700, Viswesh S escribió:
> Hi,
> 
>  
> 
> Could you let all know how to use your drivemap command.Is it similar
> to the map command in legacy grub.

Hi there. Information about the use of a command is better placed in the
wiki than in this list, however this command is not merged in yet, and I
reckon its "help" feature could be quite better.

Currently, the "safe" GRUB2 commands for booting from (hd1,1) is:

drivemap (hd0) (hd1)
drivemap (hd1) (hd0)
set root=(hd0)
chainloader (hd1,1)+1
boot

Maybe an explanation of what "drivemap" does would put a bit of light
here:

    * The BIOS hard disks are numbered from 0 to 128 (actually from 0x80 to
0xFF because the lower numbers are reserved for floppy disks). These
numbers are used to select the target drive when issuing I/O requests
through the BIOS routines (INT 13h)
    * When the BIOS loads a boot sector and transfers control to it, the DL
register contains the "boot disk number", i.e. the disk from which the
bootsector was loaded. This allows the bootsector to load its OS from
the same disk it was run, instead of having to probe every single disk
in the system.
    * The "chainloader" command works like the BIOS: it loads a bootsector
into memory and jumps to it. In this case, the value in DL corresponds
to the disk that was set as "root drive" to GRUB. If no root drive is
set, the OS receives 0xFF, which should be recognized as an impossible
drive. Some OSes will just trust this and fail (i.e. FreeDOS) while
others will try to boot from the first hard disk (0x80).
    * The "drivemap" command acts as the old TSRs from the DOS times: when
the "boot" command is issued, it installs its own handler for INT 13h
requests, which performs the requested mappings and then call the old
(usually BIOS) handler with the changed parameters. Thus, an OS
accessing the drive through the BIOS routines will see the drives moved,
duplicated or whatever the mappings provided. Again: drivemap does NOT
modify the "live" drive mappings within GRUB; its changes only affect
the booted OS.

The strange root=(hd0) line that appears to contradict the chainloader
line is there because drivemap has no communication with the particular
loader. If you set root to (hd1,1) and then issue chainloader and boot,
the OS will receive 0x81 in DL because hd1 was the second hard drive
when chainloader was issued (remember that drivemap doesn't act until
"boot"). Thus, the target OS will try to boot from what _it_ sees as the
second hard disk, which will now be the old hd0 - wrong.

If the target OS does not need to access the old hd0 or it only uses the
BIOS routines for the boot process (i.e. it later uses ATA drivers and
will redetect everything from scratch), you can leave the second
drivemap line out and use a more compact script like this:

drivemap (hd0) (hd1)
set root=(hd1,1)
chainloader +1
boot

This will work on Windows, because no matter where it tries to boot from
it will find the same disk: both the first and second BIOS disks point
to hd1 now. However, if you use a DOS-like system which uses the BIOS
routines exclusively (i.e. FreeDOS) then your hd0 disk would have
disappeared to it and you'd have D: to be a mirror of C:. In order to
have hd0 as D:, hd1 as C: and everything working, you need the first
script I posted, which makes the complete swap and then makes FreeDOS
load from the "first" hard drive (i.e. hd1). This is the setup I have
extensively tested in QEMU, while the Windows boot has been tested in
both QEMU (with ReactOS) and some random computers including mine (with
Windows XP Home and Windows XP Pro x64).

I'm looking forward to streamline the drivemap syntax, so that the two
drivemap lines can be fused into one like "drivemap --swap (hd0) (hd1)".
However, given that it's not a bug and that GRUB is still in heavy
development (and thus syntax changes are acceptable), I'd prefer to have
the patch integrated as it is - so we have the functionality - and then
modify what's needed - so we have it pretty.

-Habbit

Hi,
I have tried the same and different combinations,but still the machine is going for reset after I execute the boot command.
As nothing gets displayed also,I am not sure where exactly this is going wrong also.
What all might be the details in which I might have to look on, if I have to boot windows using grub2 only.
Viswesh


      Connect with friends all over the world. Get Yahoo! India Messenger at http://in.messenger.yahoo.com/?wm=n/

[-- Attachment #2: Type: text/html, Size: 6319 bytes --]

^ permalink raw reply	[flat|nested] 47+ messages in thread
* Re: [PATCH] Drivemap module
@ 2008-08-06 14:43 Viswesh S
  2008-08-06 17:31 ` Javier Martín
  0 siblings, 1 reply; 47+ messages in thread
From: Viswesh S @ 2008-08-06 14:43 UTC (permalink / raw)
  To: The development of GRUB 2

[-- Attachment #1: Type: text/plain, Size: 52629 bytes --]

Hi,
Could you let all know how to use your drivemap command.Is it similar to the map command in legacy grub.
drivemap (hd0) (hd1)
drivemap (hd1) (hd0)
with the grub.cfg to be 
menuentry "Linux" {
linux ....
initrd ...
}
menuentry "Windows" {
        set root=(hd1,1)
        chainloader +1
}
Viswesh



----- Original Message ----
From: Javier Martín <lordhabbit@gmail.com>
To: The development of GRUB 2 <grub-devel@gnu.org>
Sent: Tuesday, 5 August, 2008 10:09:41 PM
Subject: Re: [PATCH] Drivemap module

Ok, I will reply to your last three messages on a single post:
Hi there!

El mar, 05-08-2008 a las 13:31 +0200, Marco Gerards escribió: 
> Hi,
> 
> Javier Martín <lordhabbit@gmail.com> writes:
> 
> >> Anyway, since "they" are more likely to maintain the code in 
> >> the long run than you, in general, the question is whether 
> >> the code is more likely to become buggy by their hacking on 
> >> it, if it follows project coding style or someone else's 
> >> (your) "safer" coding style.  Likely it's safer if using a 
> >> consistent programming style.  Although I personally don't 
> >> see that it's very helpful to have a style that makes things 
> >> down to the order of "==" arguments be consistent within the 
> >> project; haphazard only slows reading the tiniest bit, and I 
> >> don't think it makes a different what order the arguments are...
> > 
> > Hmm... I was partially expecting a flamefest to start. Pity ^^
> > Well, let's spill a little napalm: the GNU style bracing is extremely
> > silly!! Why the hell are the "if" and "else" blocks indented differently
> > in this example?
> >  if (condition)
> >    return 0;
> >  else
> >    {
> >      return -1;
> >    }
> > Nah, I'm not really bringing that issue, I was just joking, and in fact
> > I'm reconsidering my objections to the operator== arguments order rule,
> > even though I still consider my style safer and more sensible. If
> > someone else wants to express their opinion on that issue, do it fast
> > before I completely submit to Master Marco's will :D
> 
> Please don't be sarcastic, start flame wars or call names.  It will not
> help anyone.

Ok, sorry, picturing you as a whip-cracking dominatrix was really not proper ^^. I was just joking about how fast can flamewars be started.

> 
> --
> Marco
> 
> 
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel

El mar, 05-08-2008 a las 13:23 +0200, Marco Gerards escribió:
> Hi Javier,
> 
> Javier Martín <lordhabbit@gmail.com> writes:
> 
> > El lun, 04-08-2008 a las 22:51 +0200, Marco Gerards escribió:
> >> Javier Martín <lordhabbit@gmail.com> writes:
> >> 
> >> > After your latest replay, I "reevaluated" my stubbornness WRT some of
> >> > your advices, and I've changed a few things:
> >> >
> >> > - Variables are now declared (and, if possible, initialized) before
> >> > precondition checks, even simple ones. The install_int13_handler
> >> > function has not been modified, however, since I find it a bit
> >> > nonsensical to put bunch of declarations without an obvious meaning just
> >> > after the "else" line:
> >> >      grub_uint32_t *ivtslot;
> >> >      grub_uint16_t *bpa_freekb;
> >> >      grub_size_t total_size;
> >> >      grub_uint16_t payload_sizekb;
> >> >      grub_uint8_t *handler_base;
> >> >      int13map_node_t *handler_map;
> >> >      grub_uint32_t ivtentry;
> >> 
> >> Please understand me correctly.  Code just has to be written according
> >> to our coding standards before it can and will be included.  We can
> >> discuss things endlessly but we will simply stick to the GNU Coding
> >> Standards as you might expect.
> >> 
> >> I hope you can appreciate that all code of GRUB has the same coding
> >> style, if you like this style or not.
> > Big sigh. Function modified to conform to your preciousss coding style.
> > I might look a bit childish or arrogant saying this, but what is the
> > point upholding a coding style that, no matter how consistent it is,
> > hinders readability. With so much local variables, they are hard to keep
> > track of no matter the coding style, but with the old ("mixed, heretic")
> > style there was not need for a comment before each declaration: they
> > were declared and used when needed instead of at the start of a block,
> > because that function is inherently linear, not block-structured.
> 
> In your opinion it is not a good coding style.  I just avoid
> discussions about it because such discussions just waste my time.
> Even if you are able to convince me, GRUB is a GNU project and will
> remain to use the GCS.

Which is fine, but the order of the arguments to == is specified nowhere in the GCS: the order you defend only appears in examples less than five times. Thus, your insistence is a matter of internal consistence within GRUB and is only based on examples in the GCS. Additionally, your previous suggestion to change checks line "entries == 0 " to "!entries" are _against_ the examples of the GCS, even for pointers.

Then, given that the _examples_ in the GCS are non-authoritative, I was advocating a simple change for better resilience against future changes. You want to keep consistency with the current sources, and that I understand, so I will perform the required trivial changes in my code to use the style used in other GRUB code, but I still think it is worse and more error-prone.

> 
> 
> >> > - Only one declaration per line: even though C is a bit absurd in not
> >> > recognizing T* as a first class type and instead thinking of * as a
> >> > qualifier to the variable name; and even though my code does not incur
> >> > into such ambiguities.
> >> > - Comments moved as you required, reworded as needed
> >> > - Extensive printf showing quasi-help in the "show" mode trimmed down to
> >> > just the first sentence.
> >> > - Internal helper functions now use the standard error handling, i.e.
> >> > return grub_error (err, fmt, ...)
> >> > - Comment about the strange "void" type instead of "void*" rephrased to
> >> > be clearer
> >> 
> >> Thanks a lot!
> >> 
> >> > There is, however, one point in which I keep my objection: comparisons
> >> > between a variable and a constant should be of the form CONSTANT ==
> >> > variable and not in the reverse order, since an erroneous but quite
> >> > possible change of == for = results in a compile-time error instead of a
> >> > _extremely_ difficult to trace runtime bug. Such kind of bugs are quite
> >> > excruciating to find in userspace applications within an IDE, so I can't
> >> > even consider the pain to debug them in a bootloader.
> >> 
> >> I understand your concern, nevertheless, can you please change it?
> > You understand my concern, but seemingly do not understand that in order
> > to conform to the Holy Coding Style you are asking me to write code that
> > can become buggy (and with a very hard to trace bug) with a simple
> > deltion! (point: did you notice that last word _without_ a spelling
> > checker? Now try to do so in a multithousand-line program).
> 
> Yes, I did notice it immediately.
> 
> The coding style is not holy in any way.  Everyone has its own coding
> style.  You must understand I do not want to fully discuss it every
> time someone sends in a patch, especially since it will not change
> anyways.

By the way, I just noticed that you write _normal_ text with two spaces after a dot, just like comments in the code! o_O
> 
> > While tools like `svn diff' can help in these kind of problems, their
> > utility is greatly diminished if the offending change is part of a
> > multi-line change. Besides, sometimes checks like "if (a=b)", or more
> > frequently "if (a=f())" are intentionally used in C, so the error might
> > become even more difficult to spot and correct. I ask for a deep
> > reflection on this issue: maybe I'm dead wrong and an arrogant brat in
> > my attempt to tackle the Holy GNU Coding Standards, but I'd like to ask
> > input from more people on this.
> 
> I will refuse patches with "if (a = f())", if that makes you sleep
> better ;-)
In fact the GCS discourages that use, but allows the same in loop checks, particularly "while" loops.
>  
> >> > WRT accepting raw BIOS disk numbers, I agree with you in principle, but
> >> > I'm keeping the functionality for now, since I don't quite like the
> >> > "drivemap (hd0) (hd1)" syntax - which device is which?. I'd rather have
> >> > something akin to "drivemap (hd0) (bios:hd1)", but I want to hear the
> >> > opinions of people in this list.
> >> 
> >> I personally do not care a lot if BIOS disk numbers are used.
> >> Although I do not see why it is useful.
> >> 
> >> As for the syntax, I would prefer something more GRUB2ish, like:
> >> 
> >> map --bios=(hd0) --os=(hd1)
> > I agree. What about "grub" for the source drive instead of "os", with
> > "bios" being the target drive? I mean:
> >     drivemap --grub=(hd1) --bios=(hd0)
> > Would map 0x80 in the BIOS to grub's (hd1) drive which will most likely
> > be BIOS drive 0x81.
> 
> It will not change the functionality which GRUB is active.  But it
> will change it for the OS that is loaded.  So I do not think --grub=
> is a good idea because of this.  As for GRUB Legacy, it confused a lot
> of people, so making people explicitly say that it is changed for the
> OS, this confusion might go away :-)
> 
> > However, I prefer not to change it right now. Maybe when there are no
> > other issues WRT to this patch we can set on to tackle this one.
> 
> So first I'll review this patch, then you will send in a new one?

Of course. I meant that I prefer to smash all "syntactic" changes in the patch before introducing a functionality change that could lead to new syntactic issues.
>  
> >> Or so, perhaps the long argument names can be chosen in a more clever
> >> way :-)
> >> > The new version of the patch is attached, and here is my suggested CLog:
> >> >
> >> > 2008-08-XX  Javier Martin  <lordhabbit@gmail.com>
> >> 
> >> (newline)
> >> 
> >> >        * commands/i386/pc/drivemap.c : New file.
> >> >        * commands/i386/pc/drivemap_int13h.S : New file.
> >> >        * conf/i386-pc.rmk (pkglib_MODULES) : Added drivemap.mod
> >> >        (drivemap_mod_SOURCES) : New variable
> >> >        (drivemap_mod_ASFLAGS) : Likewise
> >> >        (drivemap_mod_CFLAGS) : Likewise
> >> >        (drivemap_mod_LDFLAGS) : Likewise
> >> >        * include/grub/loader.h (grub_loader_register_preboot) : New
> >> >        function prototype. Register a new pre-boot handler
> >> 
> >> No need how the change is used or why it was added.
> >> 
> >> >        (grub_loader_unregister_preboot) : Likewise. Unregister handler
> >> 
> >> Same here.
> >> 
> >> >        (grub_preboot_hookid) : New typedef. Registered hook "handle"
> >> 
> >> Same here.
> >> 
> >> >        * kern/loader.c (grub_loader_register_preboot) : New function.
> >> >        (grub_loader_unregister_preboot) : Likewise.
> >> >        (preboot_hooks) : New variable. Linked list of preboot hooks
> >> 
> >> Same here.
> >> 
> >> >        (grub_loader_boot) : Call the list of preboot-hooks before the
> >> >        actual loader
> >> 
> >> What's the `'?
> > The what? o_O
> 
> I see some weird character in your text.  My font shows it as a block
> before every `*'.
I see nothing: "What's the `'?". Maybe it's some kind of tab?
> 
> >> Please do not add a space before the ":" 
> > Ok, everything corrected. New CL entry:
> >
> > 2008-08-XX  Javier Martin  <lordhabbit@gmail.com>
> >
> >        * commands/i386/pc/drivemap.c: New file.
> >        * commands/i386/pc/drivemap_int13h.S: New file.
> >        * conf/i386-pc.rmk (pkglib_MODULES): Added drivemap.mod
> >        (drivemap_mod_SOURCES): New variable
> >        (drivemap_mod_ASFLAGS): Likewise
> >        (drivemap_mod_CFLAGS): Likewise
> >        (drivemap_mod_LDFLAGS): Likewise
> >        * include/grub/loader.h (grub_loader_register_preboot): New
> >        function prototype.
> >        (grub_loader_unregister_preboot): Likewise.
> >        (grub_preboot_hookid): New typedef.
> >        * kern/loader.c (grub_loader_register_preboot): New function.
> >        (grub_loader_unregister_preboot): Likewise.
> >        (preboot_hooks): New variable.
> >        (grub_loader_boot): Call the list of preboot-hooks before the
> >        actual loader
> 
> Please add a `.' after "New variable" and "Likewise", same for the
> third and the last sentence.  Sometimes you did it right :-).
> 
Done. New CL entry will go at the end of this neverending post.

> 
> >> Some comments can be found below.  Can you please fix the code mention
> >> in the review and similar code?  I really want the code to be just
> >> like any other GRUB code.  Please understand that we have to maintain
> >> this in the future.  If everyone would use his own codingstyle, GRUB
> >> would become unmaintainable.
> >> 
> >> > Index: commands/i386/pc/drivemap.c
> >> > ===================================================================
> >> > --- commands/i386/pc/drivemap.c    (revisión: 0)
> >> > +++ commands/i386/pc/drivemap.c    (revisión: 0)
> >> > @@ -0,0 +1,417 @@
> >> > +/* 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>
> >> > +
> >> > +#define MODNAME "drivemap"
> >> > +
> >> > +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}
> >> > +};
> >> > +
> >> > +/* Syms/vars/funcs exported from drivemap_int13h.S - start.  */
> >> > +
> >> > +/* Realmode far ptr = 2 * 16b */
> >> > +extern grub_uint32_t grub_drivemap_int13_oldhandler;
> >> > +/* Size of the section to be copied */
> >> > +extern grub_uint16_t grub_drivemap_int13_size;
> >> > +
> >> > +/* This type is used for imported assembly labels, takes no storage and is only
> >> > +  used to take the symbol address with &label.  Do NOT put void* here.  */
> >> > +typedef void grub_symbol_t;
> >> > +extern grub_symbol_t grub_drivemap_int13_handler_base;
> >> > +extern grub_symbol_t grub_drivemap_int13_mapstart;
> >> > +
> >> > +void grub_drivemap_int13_handler (void);
> >> 
> >> The lines above belong in a header file.
> > True, but they are used in a single file in the whole project and thus I
> > see it pointless to extract an unneeded header, which would become one
> > more SVN object to track. However, if you insist I will split the header
> > file at once. In particular, I think the grub_symbol_t typedef should go
> > into the "standard" GRUB headers somehow (but not in symbol.h, which is
> > included from assembly files).
> 
> Please do so.
> 
> Other people might want to comment on the symbol change.  They will
> most likely miss it if we keep discussing it here ;-).  Can you please
> send that in as a separate change to give other the opportunity to
> react on it?
Ok, so in a first stage I will put the discussed block in a drivemap.h file in include/, then when that's committed start a discussion about moving grub_symbol_t from drivemap.h to another header file.
> 
> >> > +/* Syms/vars/funcs exported from drivemap_int13h.S - end.  */
> >> > +
> >> > +
> >> > +static grub_preboot_hookid insthandler_hook;
> >> > +
> >> > +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;
> >> 
> >> No need to set this to zero.
> > Yes, you said so already, but I wanted to make the initial state very
> > explicit to a future developer, since that variable is checked against
> > zero in several points. Given that the added source size is four bytes
> > and the added binary size is _zero_, is all the fuss really necessary?
> > Notice that I changed the other variable in which you pointed out this
> > issue, because it is not checked against zero anywhere.
> 
> Please do so anyways.
> 
> >> > +static grub_err_t install_int13_handler (void);
> >> > +
> >> > +/* Puts the specified mapping into the table, replacing an existing mapping
> >> > +  for newdrive or adding a new one if required.  */
> >> > +static grub_err_t
> >> > +drivemap_set (grub_uint8_t newdrive, grub_uint8_t redirto)
> >> > +
> >> 
> >> Please do not add a newline here.
> > Oops, sorry. I forgot to remove it when moving the comment
> 
> :-)
> 
> >> > +  drivemap_node_t *mapping = 0;
> >> > +  drivemap_node_t *search = drivemap;
> >> > +  while (search)
> >> > +    {
> >> > +      if (search->newdrive == newdrive)
> >> > +        {
> >> > +          mapping = search;
> >> > +          break;
> >> > +        }
> >> > +      search = search->next;
> >> > +    }
> >> > +
> >> > +  
> >> > +  /* Check for pre-existing mappings to modify before creating a new one.  */
> >> > +  if (mapping)
> >> > +    mapping->redirto = redirto;
> >> > +  else 
> >> > +    {
> >> > +      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;
> >> > +}
> >> > +
> >> > +/* Removes the mapping for newdrive from the table.  If there is no mapping,
> >> > +  then this function behaves like a no-op on the map.  */
> >> > +static void
> >> > +drivemap_remove (grub_uint8_t newdrive)
> >> > +{
> >> > +  drivemap_node_t *mapping = 0;
> >> > +  drivemap_node_t *search = drivemap;
> >> > +  drivemap_node_t *previous = 0;
> >> > +  while (search)
> >> > +    {
> >> > +      if (search->newdrive == newdrive)
> >> > +        {
> >> > +          mapping = search;
> >> > +          break;
> >> > +        }
> >> > +      previous = search;
> >> > +      search = search->next;
> >> > +    }
> >> > +  if (mapping) /* Found.  */
> >> 
> >> You forgot one.
> > Corrected. Sorry.
> >> 
> >> > +    {
> >> > +      if (previous)
> >> > +        previous->next = mapping->next;
> >> > +      else /* Entry was head of list.  */
> >> > +        drivemap = mapping->next;
> >> > +      grub_free (mapping);
> >> > +    }
> >> > +}
> >> > +
> >> > +/* Given a device name, resolves its BIOS disk number and stores it in the
> >> > +  passed location, which should only be trusted if ERR_NONE is returned.  */
> >> > +static grub_err_t
> >> > +parse_biosdisk (const char *name, grub_uint8_t *disknum)
> >> > +{
> >> > +  grub_disk_t disk;
> >> > +  if (!name || 0 == *name)
> >> > +    return grub_error (GRUB_ERR_BAD_ARGUMENT, "device name empty");
> >> > +  /* Skip the first ( in (hd0) - disk_open wants just the name.  */
> >> > +  if (*name == '(')
> >> > +    name++;
> >> > +  
> >> > +  disk = grub_disk_open (name);
> >> > +  if (!disk)
> >> > +    return grub_error (GRUB_ERR_UNKNOWN_DEVICE, "unknown device \"%s\"", name);
> >> > +  else
> >> > +    {
> >> > +      const enum grub_disk_dev_id id = disk->dev->id;
> >> > +      /* The following assignment is only sound if the device is indeed a
> >> > +        biosdisk.  The caller must check the return value.  */
> >> > +      if (disknum)
> >> > +        *disknum = disk->id;
> >> > +      grub_disk_close (disk);
> >> > +      if (GRUB_DISK_DEVICE_BIOSDISK_ID == id)
> >> > +        return GRUB_ERR_NONE;
> >> > +      else return grub_error (GRUB_ERR_BAD_DEVICE, "%s is not a BIOS disk", name);
> >> > +    }
> >> > +}
> >> > +
> >> > +/* Given a BIOS disk number, returns its GRUB device name if it exists.
> >> > +  For nonexisting BIOS dnums, this function returns ERR_UNKNOWN_DEVICE.  */
> >> 
> >> This is GRUB_ERR_UNKNOWN_DEVICE
> > I know, I consciously left the GRUB_ part out because 1) it would
> > require the line to be split and 2) that prefix is all over the place.
> > Corrected, however.
> >> 
> >> > +static grub_err_t
> >> > +revparse_biosdisk(const grub_uint8_t dnum, const char **output)
> >> > +{
> >> > +  int found = 0;
> >> > +  auto int find (const char *name);
> >> > +  int find (const char *name)
> >> > +  {
> >> > +    const grub_disk_t disk = grub_disk_open (name);
> >> > +    if (!disk)
> >> > +      return 0;
> >> > +    else
> >> > +      {
> >> > +        if (disk->id == dnum && GRUB_DISK_DEVICE_BIOSDISK_ID == disk->dev->id)
> >> > +          {
> >> > +            found = 1;
> >> > +            if (output)
> >> > +              *output = name;
> >> > +          }
> >> > +        grub_disk_close (disk);
> >> > +        return found;
> >> > +      }
> >> > +  }
> >> > +
> >> > +  grub_disk_dev_iterate (find);
> >> > +  if (found)
> >> > +    return GRUB_ERR_NONE;
> >> > +  else return grub_error (GRUB_ERR_UNKNOWN_DEVICE, "BIOS disk %d not found", dnum);
> >> 
> >> Please change this.
> > Em... to what? What is the problem? Do you want me to reverse the
> > comparison? i.e. if (!found) { return error; } else { return ok; }
> 
> The return is on the same line as the else.
> 

Corrected.

> 
> >> > +/* Given a GRUB-like device name and a convenient location, stores the related
> >> > +  BIOS disk number.  Accepts devices like \((f|h)dN\), with 0 <= N < 128.  */
> >> > +static grub_err_t
> >> > +tryparse_diskstring (const char *str, grub_uint8_t *output)
> >> > +{
> >> > +  if (!str || 0 == *str)
> >> > +    goto fail;
> >> > +  /* Skip opening paren in order to allow both (hd0) and hd0.  */
> >> > +  if (*str == '(')
> >> > +    str++;
> >> > +  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, 0, 0);
> >> > +      if (grub_errno != GRUB_ERR_NONE || drivenum > 127)
> >> > +        /* N not a number or out of range */
> >> > +        goto fail;
> >> 
> >> Can you put this between braces, now comment was added.
> > Done.
> >> 
> >> > +      else
> >> > +        {
> >> > +          bios_num |= drivenum;
> >> > +          if (output)
> >> > +            *output = bios_num;
> >> > +          return GRUB_ERR_NONE;
> >> > +        }
> >> > +    }
> >> > +  else goto fail;
> >> 
> >> ...
> > What's the problem here? The lack of braces? The goto (as used in the
> > ext2 code)?
> 
> goto is on the same line as the else.

Corrected, though I find this change less logic than the last one (splitting the else retun grub_error(...)) line because this generates two _extremely_ short lines;
> 
> >> > +fail:
> >> > +  return grub_error (GRUB_ERR_BAD_ARGUMENT, "device format \"%s\" invalid: must"
> >> > +                    "be (f|h)dN, with 0 <= N < 128", str);
> >> > +}
> >> > +
> >> > +static grub_err_t
> >> > +grub_cmd_drivemap (struct grub_arg_list *state, int argc, char **args)
> >> > +{
> >> > +  if (state[0].set)
> >> > +    {
> >> > +      /* Show: list mappings.  */
> >> > +      if (!drivemap)
> >> > +        grub_printf ("No drives have been remapped");
> >> > +      else
> >> > +        {
> >> > +          grub_printf ("Showing only remapped drives.\n");
> >> > +          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, freeing their memory.  */
> >> > +      drivemap_node_t *curnode = drivemap;
> >> > +      drivemap_node_t *prevnode = 0;
> >> > +      while (curnode)
> >> > +        {
> >> > +          prevnode = curnode;
> >> > +          curnode = curnode->next;
> >> > +          grub_free (prevnode);
> >> > +        }
> >> > +      drivemap = 0;
> >> > +    }
> >> > +  else
> >> > +    {
> >> > +      /* Neither flag: put mapping */
> >> 
> >> ".  */
> > Done
> >> 
> >> > +      grub_uint8_t mapfrom = 0;
> >> > +      grub_uint8_t mapto = 0xFF;
> >> > +      grub_err_t err;
> >> > +      
> >> > +      if (argc != 2)
> >> > +        return grub_error (GRUB_ERR_BAD_ARGUMENT, "two arguments required");
> >> > +
> >> > +      err = parse_biosdisk (args[0], &mapfrom);
> >> > +      if (err != GRUB_ERR_NONE)
> >> > +        return err;
> >> > +
> >> > +      err = tryparse_diskstring (args[1], &mapto);
> >> > +      if (err != GRUB_ERR_NONE) /* Not a disk string. Maybe a raw num then?  */
> >> 
> >> Please move this up.
> > Done.
> >> 
> >> > +        {    
> >> > +          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;
> >> > +        }
> >> > +      
> >> > +      if (mapto == mapfrom)  /* Reset to default.  */
> >> 
> >> Same here.
> > Done.
> >> 
> >> > +        {
> >> > +          grub_dprintf (MODNAME, "Removing the mapping for %s (%02x)", args[0], mapfrom);
> >> > +          drivemap_remove (mapfrom);
> >> > +        }
> >> > +      else  /* Map.  */
> >> 
> >> Please move the comment inside the braces below.
> > Done, and reworded.
> >> 
> >> > +        {
> >> > +          grub_dprintf (MODNAME, "Mapping %s (%02x) to %02x\n", args[0], mapfrom, mapto);
> >> > +          return drivemap_set ((grub_uint8_t)mapto, mapfrom);
> >> > +        }
> >> > +    }
> >> > +
> >> > +  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 apart from identity paging, since it accesses realmode
> >> > +  structures by their absolute addresses, like the IVT at 0 or the BDA at
> >> > +  0x400; and transforms a pmode pointer into a rmode seg:off far ptr.  */
> >> > +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.  */
> >> 
> >> ...
> > Comment moved up.
> >> 
> >> > +    {
> >> > +      entries++;
> >> > +      curentry = curentry->next;
> >> > +    }
> >> > +  if (0 == entries)
> >> 
> >> I know this is what you prefer, but can you change this nevertheless?
> > I refer to my objection near the top of the post.
> 
> I know you object, but did you change it?
Done.
> 
> >> > +      grub_dprintf (MODNAME, "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
> >> > +    {
> >> > +      grub_dprintf (MODNAME, "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;
> >> > +      grub_dprintf (MODNAME, "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;
> >> > +      grub_dprintf (MODNAME, "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");
> >> > +      grub_dprintf (MODNAME, "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);
> >> > +      grub_dprintf (MODNAME, "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);
> >> > +      grub_dprintf (MODNAME, "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;
> >> > +          grub_dprintf (MODNAME, "\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;
> >> > +      grub_dprintf (MODNAME, "\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);
> >> > +      grub_dprintf (MODNAME, "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;            /* Stop warning.  */
> >> > +  grub_register_command (MODNAME, grub_cmd_drivemap,
> >> > +                        GRUB_COMMAND_FLAG_BOTH,
> >> > +                              MODNAME " -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 (MODNAME);
> >> > +}
> >> > +
> >> > Index: commands/i386/pc/drivemap_int13h.S
> >> > ===================================================================
> >> > --- commands/i386/pc/drivemap_int13h.S    (revisión: 0)
> >> > +++ commands/i386/pc/drivemap_int13h.S    (revisión: 0)
> >> > @@ -0,0 +1,118 @@
> >> > +/*
> >> > + *  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.
> >> > + */
> >> > +
> >> > +#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.  IP-relative
> >> > +  addressing like on amd64 would make life way 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
> >> > ===================================================================
> >> > --- conf/i386-pc.rmk    (revisión: 1766)
> >> > +++ conf/i386-pc.rmk    (copia de trabajo)
> >> > @@ -158,7 +158,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
> >> > @@ -325,4 +325,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
> >> > ===================================================================
> >> > --- include/grub/loader.h    (revisión: 1766)
> >> > +++ include/grub/loader.h    (copia de trabajo)
> >> > @@ -37,6 +37,22 @@
> >> >  /* 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. on module unload).  If
> >> > +  abort_on_error is set, the boot sequence will abort if any of the registered
> >> > +  functions return anything else than GRUB_ERR_NONE.
> >> > +  On error, the return value will compare equal to 0 and the error information
> >> > +  will be available in errno and errmsg.  However, if the call is successful
> >> > +  those variables are _not_ modified. */
> >> 
> >> No need to mention errmsg, it's internal to GRUB.  As for errno (which
> >> is grub_errno, actually) it does not need to be mentioned, otherwise
> >> we would have to do so everywhere.  Please capitalize HOOK and
> >> ABORT_ON_ERROR in the comments above.
> > Done. "hook" removed because it referred to the loader module boot
> > function.
> >> 
> >> > +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.  */
> >> 
> >> Nitpick: "loader.  This..."
> > Are you a bot? ¬¬ Corrected
> 
> It would make like much simpler if I were ;-).  What makes you think
> so?
Your ability to spot these kind of smallish things, like the "deltion" word and the lack of a double space. You even write normal text with double spaces!

> 
> >> >  grub_err_t EXPORT_FUNC(grub_loader_boot) (void);
> >> > Index: kern/loader.c
> >> > ===================================================================
> >> > --- kern/loader.c    (revisión: 1766)
> >> > +++ kern/loader.c    (copia de trabajo)
> >> > @@ -61,11 +61,82 @@
> >> >    grub_loader_loaded = 0;
> >> >  }
> >> >  
> >> > +struct hooklist_node
> >> > +{
> >> > +  grub_err_t (*hook) (void);
> >> > +  int abort_on_error;
> >> > +  struct hooklist_node *next;
> >> > +};
> >> > +
> >> > +static struct hooklist_node *preboot_hooks = 0;
> >> > +
> >> > +grub_preboot_hookid
> >> > +grub_loader_register_preboot(grub_err_t (*hook) (void), int abort_on_error)
> >> > +{
> >> > +  if (!hook)
> >> > +    {
> >> > +      grub_error (GRUB_ERR_BAD_ARGUMENT, "preboot hook must not be NULL");
> >> > +      return 0;
> >> > +    }
> >> > +  grub_preboot_hookid newentry = grub_malloc (sizeof (struct hooklist_node));
> >> 
> >> Mixed declarations/code.
> > Oops, sorry. I put most of my attention on drivemap.c (and even then
> > many comments slipped through). Corrected.
> 
> Please re-check them, I might have missed things this time...
> 
> >> > +  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)
> >> 
> >> "preboot (grub"
> > Corrected on both functions ;)
> >> 
> >> > +{
> >> > +  grub_preboot_hookid entry = 0;
> >> > +  grub_preboot_hookid search = preboot_hooks;
> >> > +  grub_preboot_hookid previous = 0;
> >> > +
> >> > +  if (0 == id)
> >> > +    return;
> >> 
> >> ...
> > ... xD
> >> 
> >> > +  while (search)
> >> > +    {
> >> > +      if (search == id)
> >> > +        {
> >> > +          entry = search;
> >> > +          break;
> >> > +        }
> >> > +      previous = search;
> >> > +      search = search->next;
> >> > +    }
> >> > +  if (entry) /* Found */
> >> 
> >> ...
> > Comment removed, was unnecessary.
> >> 
> >> > +    {
> >> > +      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;
> >> 
> >> Mixed declarations/code.
> > Moved the whole line up.
> >> 
> >> > +  while (entry)
> >> > +    {
> >> > +      grub_err_t possible_error = entry->hook();
> >> > +      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
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel

El mar, 05-08-2008 a las 13:28 +0200, Marco Gerards escribió:
> Hi,
> 
> Javier Martín <lordhabbit@gmail.com> writes:
> 
> >> > There is, however, one point in which I keep my objection: comparisons
> >> > between a variable and a constant should be of the form CONSTANT ==
> >> > variable and not in the reverse order, since an erroneous but quite
> >> > possible change of == for = results in a compile-time error instead of a
> >> > _extremely_ difficult to trace runtime bug. Such kind of bugs are quite
> >> > excruciating to find in userspace applications within an IDE, so I can't
> >> > even consider the pain to debug them in a bootloader.
> >> 
> >> I understand your concern, nevertheless, can you please change it?
> > You understand my concern, but seemingly do not understand that in order
> > to conform to the Holy Coding Style you are asking me to write code that
> > can become buggy (and with a very hard to trace bug) with a simple
> > deltion! (point: did you notice that last word _without_ a spelling
> > checker? Now try to do so in a multithousand-line program).
> 
> BTW, your patch still contains this, can you please change it before I
> go over it again?
> 
> I know people who claim that this code will become buggy because we
> write it in C.  Should we start accepting patches to rewrite GRUB in
> Haskell or whatever? :-)
What about Ada? The stock GCC has Ada support ^^
> 
> Really, as a maintainer I should set some standards and stick to it.
> Of course not everyone will like me and sometimes I have to act like a
> jerk.  But I rather be a jerk, than committing code that do not meet
> my expectations.  But please understand, this contribution is highly
> appreciated.  However, we want to have something maintainable for the
> far future as well :-)
I understand these kind of concerns, particularly seeing how GRUB Legacy
ended - tangled, unscalable spaghetti code. You're not a jerk, just a
bit obsessive, but that's fine when trying to handle herds of us
all-important devs which think all we do is The Right Thing (tm) and
others are heretics to The Truth.
> 
> --
> Marco
> 
> 
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel

Ok, so here is the new version of the patch, with a new drivemap.h
header and the == arguments put in the Holy Ordering ^^. I hope I didn't
forget anything this time. Here is the new CL entry:

2008-08-XX  Javier Martin  <lordhabbit@gmail.com>

        * commands/i386/pc/drivemap.c: New file.
        * commands/i386/pc/drivemap_int13h.S: New file.
        * conf/i386-pc.rmk (pkglib_MODULES): Added drivemap.mod.
        (drivemap_mod_SOURCES): New variable.
        (drivemap_mod_ASFLAGS): Likewise.
        (drivemap_mod_CFLAGS): Likewise.
        (drivemap_mod_LDFLAGS): Likewise.
        * include/grub/loader.h (grub_loader_register_preboot): New
        function prototype.
        (grub_loader_unregister_preboot): Likewise.
        (grub_preboot_hookid): New typedef.
        * kern/loader.c (grub_loader_register_preboot): New function.
        (grub_loader_unregister_preboot): Likewise.
        (preboot_hooks): New variable.
        (grub_loader_boot): Call the list of preboot-hooks before the
        actual loader.

By the way, is there anything I can do to make `svn up' updates less
traumatic? I don't want to search each "C" file for "<<<<<< mine" lines
and correct them: is there any tool to do this with a workflow not
unlike that of Gentoo's `etc-update'?

Habbit



      Add more friends to your messenger and enjoy! Go to http://in.messenger.yahoo.com/invite/

[-- Attachment #2: Type: text/html, Size: 75159 bytes --]

^ permalink raw reply	[flat|nested] 47+ messages in thread
* [PATCH] Drivemap module
@ 2008-06-10 20:09 Javier Martín
  2008-06-10 20:23 ` Vesa Jääskeläinen
  2008-06-11 14:44 ` Marco Gerards
  0 siblings, 2 replies; 47+ messages in thread
From: Javier Martín @ 2008-06-10 20:09 UTC (permalink / raw)
  To: grub-devel


[-- Attachment #1.1: Type: text/plain, Size: 1956 bytes --]

Since the last posts in the "reimplementing the legacy map command"
thread seemingly died silently, I'm sending the improved version of the
patch here. If the death of the thread was intentional, please just tell
me (either nicely or rudely) and I'll be off.

Recapitulating the other thread, this is a new module for x86-pc that
provides functionality similar to the legacy "map" command, i.e. allows
BIOS drives to be remapped and have the second HD appear as 0x80 (the
first), which is necessary because some OSes refuse to boot otherwise,
ignoring the "boot drive" passed to their loaders in DL (Windows XP and
other NTs); or can only boot with limited functionality (FreeDOS). The
syntax of this new command is similar to the old "map", though the
second argument must be the target BIOS drive number instead of a GRUB
device, though a hack-patch parsing them would be simple.

Regarding the objections raised by Vesa, I followed most of the advices,
removing chatter in the source and completely separating drivemap from
the kernel. However, I've left the "abort_on_error" part of the new
preboot-hook interface intact, mostly because I don't know how to
replace it. As drivemap is the only module using it, the problem should
be harmless for now, but a solution should be devised - not by me,
though, finals are here (literally, I had one today).

I hope I didn't leave anything out this time. This code has been tested
on the following situations:
 * Emulators: Bochs & QEMU, booting FreeDOS in (hd1) with another fat
partition in (hd0) - works wonderfully, and even allows access to both
drives if they are crossmapped and a faux root=(hd0) is set.
 * Real hardware: an Athlon64 (939) CPU on a Gigabyte motherboard,
booting Windows XP Pro x64 in (hd1) with Ubuntu in (hd0) - works too,
with no hacks required at all (by the way, booting XP was my reason to
do this).

Well, here it is. Have a nice day!

Habbit

[-- Attachment #1.2: drivemap.patch --]
[-- Type: text/x-patch, Size: 23117 bytes --]

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	10 Jun 2008 20:02:19 -0000
@@ -0,0 +1,348 @@
+/* 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
+
+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 */
+
+/* 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);
+
+void EXPORT_FUNC(grub_drivemap_int13_handler)(void);
+
+/* ASSEMBLY SYMBOLS/VARS/FUNCS - end */
+
+static grub_preboot_hookid insthandler_hook = 0;
+
+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;
+static grub_err_t drivemap_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. */
+{
+  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;
+  else  /* Create a new mapping and add it to the head of the list */
+    {
+      mapping = grub_malloc (sizeof (drivemap_node_t));
+      if (!mapping)
+        return grub_error (GRUB_ERR_OUT_OF_MEMORY, "cannot allocate map entry");
+      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. */
+{
+  drivemap_node_t *mapping = 0, *search = drivemap, *previous = 0;
+  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 */
+      grub_free (mapping);
+    }
+}
+
+static grub_err_t parse_biosdisk (const char *name, grub_uint8_t *disknum)
+{
+  if (!name) return GRUB_ERR_BAD_ARGUMENT;
+  if (name[0] == '(')
+    name++; /* Skip the first ( in (hd0) - disk_open wants just the name! */
+  grub_disk_t disk = grub_disk_open (name);
+  if (!disk)
+    return GRUB_ERR_UNKNOWN_DEVICE;
+  else
+    {
+      enum grub_disk_dev_id id = disk->dev->id;
+      if (disknum)
+        *disknum = disk->id;   /* Only valid, of course if it's a biosdisk */
+      grub_disk_close (disk);
+      return (GRUB_DISK_DEVICE_BIOSDISK_ID != id) ?
+          GRUB_ERR_BAD_DEVICE : GRUB_ERR_NONE;
+    }
+}
+
+static grub_err_t revparse_biosdisk(const grub_uint8_t dnum, const char **output)
+{
+  grub_err_t retval = 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)
+          {
+            found = 1;
+            *output = name;
+            retval = GRUB_ERR_NONE;
+          }
+        grub_disk_close (disk);
+        return found;
+      }
+  }
+
+  grub_disk_dev_iterate (&find);
+  return retval;
+}
+
+static grub_err_t
+grub_cmd_drivemap (struct grub_arg_list *state, int argc, char **args)
+{
+  if (state[0].set) /* Show: list mappings */
+    {
+      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");
+          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;
+      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_errno = GRUB_ERR_NONE;
+      unsigned long mapto = grub_strtoul (args[1], 0, 0);
+      if (grub_errno != GRUB_ERR_NONE)
+        return grub_error (grub_errno,
+                          "BIOS disk number must be between 0 and 255");
+      else 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);
+        }
+    }
+
+  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 */
+static grub_err_t drivemap_install_int13_handler(void)
+{
+  grub_size_t entries = 0;
+  drivemap_node_t *curentry = drivemap;
+  while (curentry)  /* Count entries to prepare a contiguous map block */
+    {
+      entries++;
+      curentry = curentry->next;
+    }
+  if (0 == entries)
+    {
+      DBG_PRINTF ("drivemap: No drives remapped, int13h handler not installed");
+      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 *bpaMemInKb = (grub_uint16_t*)0x00000413;
+      DBG_PRINTF ("Top of conventional memory: %u KiB\n", *bpaMemInKb);
+      grub_size_t totalSize = grub_drivemap_int13_size
+                            + (entries + 1) * sizeof(int13map_node_t);
+      grub_uint16_t payloadSizeKb = (totalSize >> 10) +
+                                    (((totalSize % 1024) == 0) ? 0 : 1);
+      if ((*bpaMemInKb - payloadSizeKb) < 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", totalSize, payloadSizeKb);
+      *bpaMemInKb -= payloadSizeKb;
+
+      /* Copy int13h handler chunk to reserved area */
+      grub_uint8_t *handler_base = (grub_uint8_t*)(*bpaMemInKb << 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;
+    }
+}
+#undef INT13H_TONEWADDR
+#undef INT13H_REBASE
+#undef INT13H_OFFSET
+
+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 (&drivemap_install_int13_handler, 1);
+}
+
+GRUB_MOD_FINI(drivemap)
+{
+  grub_loader_unregister_preboot (insthandler_hook);
+  insthandler_hook = 0;
+  grub_unregister_command ("drivemap");
+}
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	10 Jun 2008 20:02:19 -0000
@@ -0,0 +1,128 @@
+/*
+ *  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.
+ */
+
+/*
+ *  This is the area for all of the special variables.
+ */
+
+#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
+#  push %es
+#  pushal
+#  mov $0, %di
+#  mov $0xb800, %bx
+#  mov %bx, %es
+#  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)
+  .space 0
+.code32 /* Copy stops here */
+VARIABLE(grub_drivemap_int13_size)
+  .word GRUB_DRIVEMAP_INT13H_OFFSET(grub_drivemap_int13_size)
+#undef GRUB_DRIVEMAP_INT13H_OFFSET
Index: conf/i386-pc.rmk
===================================================================
RCS file: /sources/grub/grub2/conf/i386-pc.rmk,v
retrieving revision 1.119
diff -u -p -r1.119 i386-pc.rmk
--- conf/i386-pc.rmk	30 May 2008 04:20:47 -0000	1.119
+++ conf/i386-pc.rmk	10 Jun 2008 20:02:26 -0000
@@ -152,7 +152,7 @@ pkglib_MODULES = biosdisk.mod _chain.mod
 	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
@@ -319,4 +319,11 @@ bsd_mod_SOURCES = loader/i386/bsd_normal
 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 -p -r1.9 loader.h
--- include/grub/loader.h	21 Jul 2007 23:32:22 -0000	1.9
+++ include/grub/loader.h	10 Jun 2008 20:02:26 -0000
@@ -37,6 +37,19 @@ void EXPORT_FUNC(grub_loader_set) (grub_
 /* 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 -p -r1.9 loader.c
--- kern/loader.c	21 Jul 2007 23:32:26 -0000	1.9
+++ kern/loader.c	10 Jun 2008 20:02:27 -0000
@@ -61,11 +61,78 @@ grub_loader_unset(void)
   grub_loader_loaded = 0;
 }
 
+struct hooklist_node
+{
+  grub_err_t (*hook) (void);
+  int abort_on_error;
+  struct hooklist_node *next;
+};
+
+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)
+    {
+      grub_error (GRUB_ERR_BAD_ARGUMENT, "preboot hoook must not be 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;
+  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 retVal = entry->hook();
+      if (retVal != GRUB_ERR_NONE && entry->abort_on_error)
+        return retVal;
+      entry = entry->next;
+    }
 
   if (grub_loader_noreturn)
     grub_machine_fini ();

[-- Attachment #2: Esta parte del mensaje está firmada digitalmente --]
[-- Type: application/pgp-signature, Size: 827 bytes --]

^ permalink raw reply	[flat|nested] 47+ messages in thread

end of thread, other threads:[~2008-08-14 22:16 UTC | newest]

Thread overview: 47+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2008-07-04  2:12 [PATCH] Drivemap module Javier Martín
2008-07-05 11:04 ` Marco Gerards
2008-07-16 15:39   ` Javier Martín
2008-07-20 19:40 ` Marco Gerards
2008-07-21  0:55   ` Javier Martín
2008-07-21 11:07     ` Javier Martín
2008-07-22 21:32     ` Robert Millan
2008-07-31 19:01     ` Marco Gerards
2008-08-03 23:29       ` Javier Martín
2008-08-04 20:51         ` Marco Gerards
2008-08-04 23:10           ` Javier Martín
2008-08-05  0:50             ` Isaac Dupree
2008-08-05  2:38               ` Javier Martín
2008-08-05 11:31                 ` Marco Gerards
2008-08-05 11:23             ` Marco Gerards
2008-08-05 17:18               ` Colin D Bennett
2008-08-05 11:28             ` Marco Gerards
2008-08-05 16:39               ` Javier Martín
2008-08-09 15:33                 ` Javier Martín
2008-08-13 10:13                   ` Marco Gerards
2008-08-13 12:16                     ` Javier Martín
2008-08-13 13:00                       ` Robert Millan
2008-08-13 14:28                         ` Javier Martín
2008-08-13 14:51                           ` Marco Gerards
2008-08-13 15:14                           ` Robert Millan
2008-08-13 15:57                             ` Marco Gerards
2008-08-13 22:38                               ` Javier Martín
2008-08-14 17:15                                 ` Marco Gerards
2008-08-14 22:17                                   ` Javier Martín
2008-08-13 13:01                       ` Robert Millan
2008-08-05 17:15             ` Colin D Bennett
  -- strict thread matches above, loose matches on Subject: below --
2008-08-08 13:43 Viswesh S
2008-08-07 12:15 Viswesh S
2008-08-06 14:43 Viswesh S
2008-08-06 17:31 ` Javier Martín
2008-08-08 13:20   ` Felix Zielcke
2008-06-10 20:09 Javier Martín
2008-06-10 20:23 ` Vesa Jääskeläinen
2008-06-10 21:31   ` Javier Martín
2008-06-12 20:31     ` Pavel Roskin
2008-06-12 22:43       ` Javier Martín
2008-06-12 22:58         ` Colin D Bennett
2008-06-13  1:00           ` Pavel Roskin
2008-06-13  4:09             ` Colin D Bennett
2008-06-13  1:37         ` Pavel Roskin
2008-06-13  2:29           ` Javier Martín
2008-06-11 14:44 ` Marco Gerards

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.