Hi,
Javier Martín <[EMAIL PROTECTED]> 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
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel
