Re: [PATCH] Drivemap module

[Javier Martín]

El mié, 13-08-2008 a las 15:00 +0200, Robert Millan escribió:
> Hi,
> 
> Marco asked me to review this.
So he finally got fed up of me... Understandable ^^

>   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.
Ok, so following your [1], what about replacing the define with... ?

static const char[] MODNAME = "drivemap";
> 
> > +/* 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).

It _could_ be made generic, but the function as it is currently designed
installs a TSR-like assembly routine (more properly a bundle formed by a
routine and its data) in conventional memory that it has previously
reserved. Furthermore, it accesses the real-mode IVT at its "standard"
location of 0, which could be a weakness since from the 286 on even the
realmode IVT can be relocated with lidt.

Nevertheless, I don't think this functionality is so badly needed on its
own that it would be good to delay the implementation of "drivemap" to
wait for the re-engineering of this function.

> 
> > +  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.
Done.  

> 
> > +/* 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).
Sorry, it was a leftover from early development, in which I had to debug
the installing code to see whether the pointer to the old int13 was
installer: a pointer of "beef:dead" was a clue that it didn't work.
Removed and replaced with 32 bits of zeros.  

> 
> > +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).
What for? the operands are clearly unambiguous.  As you can see with the
"xchgw %ax, -4(%bp)" line, I only use them for disambiguation.  Assembly
language is cluttered enough - and AT&T syntax is twisted enough as it
is.  In fact, given that the code is specific for i386, I'd like to
rewrite this code in GAS-Intel syntax so that memory references are not
insane: -4(%bp)? please... we can have a simpler [bp - 4].  

> 
> > 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?
Most likely not, and the discussion on this particular piece of the code
died out long time (months) ago without reaching any decision.  It's a
way I've found for a fully out-kernel module like drivemap to set a
just-before-boot hook in order to install its int13 handler: installing
it earlier could cause havoc with the biosdisk driver, as drives in GRUB
would suddenly reverse, duplicate, disappear, etc.  

This "solution" is the lightest and most scalable I've found that does
not introduce drivemap-specific code in the kernel, because this
infrastructure could be used by other modules.  

> 
> > +/* 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.
What about "grub_asmsymbol_t"?

> 
> > 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?
For the reasons discussed above in the loader.h snippet, I don't think
so: the only "lighter" solution would be to just put a drivemap_hook
variable that would be called before boot, but as I mentioned before,
this solution can be employed by other modules as well.

Besides (and I realize this is not a great defense) it's not _that_ much
code: just a simple linked-list implementation with add and delete
operations, and the iteration of it on loader_boot. I did not check how
many bytes does this patch add by itself, but I can run some simulations
(I totally _had_ to say that ^^) if you want.

El mié, 13-08-2008 a las 15:01 +0200, Robert Millan escribió:
> On Wed, Aug 13, 2008 at 02:16:25PM +0200, Javier Martín wrote:
> > +static grub_err_t
> > +revparse_biosdisk(const grub_uint8_t dnum, const char **output)
> 
> Ah, and please separate function names from parenthesis ;-)
Done.  Do you and/or Marco perform any automated search (grep & friends)
for these thingies? It's either that or the robot theory... ¬¬

Well, I'm feeling lazy enough today not to attach a new version of the
patch for just five cosmetic changes (unless you're going to tell me
that it's ripe for commit :D) so we can continue discussion on the other
issues on the table.

-Habbit

signature.asc
Description: Esta parte del mensaje está firmada digitalmente