Re: [PATCH] Drivemap module

[Robert Millan]

Hi,

Marco asked me to review this.  I haven't followed the earlier discussion,
so if I say or ask something that was discussed before, please bear with me
and just point me to that.

On Wed, Aug 13, 2008 at 02:16:25PM +0200, Javier Martín wrote:
> +
> +#define MODNAME "drivemap"
> +
> [...]
> +          grub_dprintf (MODNAME, "Removing the mapping for %s (%02x)", 
> args[0], mapfrom);

I don't think this MODNAME approach is a bad idea per se [1][2], but if we
are to do it, IMHO this should really be done globally for consistency, and
preferably separately from this patch.

[1] But I'd use a const char[] instead of a macro to save space.  Maybe
    significant space can be saved when doing this throurough the code!

[2] In fact, I think it's a nice idea.

> +/* 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)
> +{

Can this be made generic?  Like "install_int_handler (int n)".  We're probably
going to need interrupts for other things later on.

Or is this code suitable for i8086 mode interrupts only?

Anyway, if it's made generic, remember the handler itself becomes suitable for
all i386 ports, not just i386-pc (for directory placement).

> +  drivemap_node_t *curentry = drivemap;

We use 'curr' as a handle for 'current' in lots of places throurough the code
(rgrep for curr[^e]).  I think it's better to be consistent with that.

> +/* Far pointer to the old handler.  Stored as a CS:IP in the style of 
> real-mode
> +   IVT entries (thus PI:SC in mem).  */
> +VARIABLE(grub_drivemap_int13_oldhandler)
> +  .word 0xdead, 0xbeef

Is this a signature?  Then a macro would be preferred, so that it can be shared
with whoever checks for it.

What is it used for, anyway?  In general, I like to be careful when using
signatures because they introduce a non-deterministic factor (e.g. GRUB
might have a 1/64k possibility to missbehave).

> +FUNCTION(grub_drivemap_int13_handler)
> +  push %bp
> +  mov %sp, %bp
> +  push %ax  /* We'll need it later to determine the used BIOS function.  */

Please use size modifiers (pushw, movw, etc).

> Index: include/grub/loader.h
> ===================================================================
> --- include/grub/loader.h     (revisión: 1802)
> +++ include/grub/loader.h     (copia de trabajo)
> @@ -37,7 +37,23 @@
>  /* Unset current loader, if any.  */
>  void EXPORT_FUNC(grub_loader_unset) (void);
>  
> -/* Call the boot hook in current loader. This may or may not return,
> +typedef struct hooklist_node *grub_preboot_hookid;
> +
> +/* Register a function to be called before the loader "boot" function.  
> 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 grub_errno.  However, if the call is successful that
> +   variable is _not_ modified. */
> +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);

This interface is added for all platforms.  I didn't follow the discussion;
has it been considered that it will be useful elsewhere then i386-pc?

> +/* 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;

I think this name is too generic for such an specific purpose.

> Index: kern/loader.c
> ===================================================================
> --- kern/loader.c     (revisión: 1802)
> +++ kern/loader.c     (copia de trabajo)
> @@ -61,11 +61,88 @@
>    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)
> +{
> [...]

This is a lot of code being added to kernel, and space in kernel is highly
valuable.

Would the same functionality work if put inside a module?

-- 
Robert Millan

  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
  how) you may access your data; but nobody's threatening your freedom: we
  still allow you to remove your data and not access it at all."