Re: [PATCH] Drivemap module

[Marco Gerards]

Hi,

Javier Martín <address@hidden> writes:

> 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 ^^

No, but I am not as qualified regarding the BIOS as Robert is, except
for general remarks ;-)

>>   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";

static const char[] modname = "drivemap";

We don't capatilize variables ;)
 
>> > +/* 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.

This can go in and we can change it, I think.
 
>> > +  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].  

I'll leave that to Robert :-)

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

Perhaps Bean should get involved in the discussion ;-).  He talked
about this in another thread.

> 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"?

How about a grub_drivemap_ prefix?  Actually, I forgot to check you
used a prefix for exported symbols and in headerfiles.  Did you? :)

>> 
>> > 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... ¬¬

It might be a good idea to make some script that tests patches
automatically.  Do you want to volunteer? :P

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

Like the argument syntax I proposed?  map --grub (hd0) --os (hd1) and
alike?

--
Marco