From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with archive (Exim 4.43) id 1KTHdq-0008Ey-NK for mharc-grub-devel@gnu.org; Wed, 13 Aug 2008 10:48:02 -0400 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1KTHdn-0008CV-TW for grub-devel@gnu.org; Wed, 13 Aug 2008 10:48:00 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1KTHdk-00088h-HP for grub-devel@gnu.org; Wed, 13 Aug 2008 10:47:59 -0400 Received: from [199.232.76.173] (port=42698 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1KTHdk-00088D-9f for grub-devel@gnu.org; Wed, 13 Aug 2008 10:47:56 -0400 Received: from smtp-vbr7.xs4all.nl ([194.109.24.27]:4465) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1KTHdj-0000nR-Up for grub-devel@gnu.org; Wed, 13 Aug 2008 10:47:56 -0400 Received: from localhost.localdomain (249-174.surfsnel.dsl.internl.net [145.99.174.249]) by smtp-vbr7.xs4all.nl (8.13.8/8.13.8) with ESMTP id m7DElr7Y080792 for ; Wed, 13 Aug 2008 16:47:54 +0200 (CEST) (envelope-from mgerards@xs4all.nl) From: Marco Gerards To: The development of GRUB 2 References: <1216601741.8334.122.camel@localhost> <877ib1khe9.fsf@xs4all.nl> <1217806150.9634.24.camel@localhost> <87iqug33m9.fsf@xs4all.nl> <1217891426.15145.38.camel@localhost> <87vdyfem5k.fsf@xs4all.nl> <1217954381.14674.31.camel@localhost> <1218296029.20937.10.camel@localhost> <87k5el9q99.fsf@xs4all.nl> <1218629785.8757.30.camel@localhost> <20080813130022.GB26618@thorin> <1218637704.8757.60.camel@localhost> Mail-Copies-To: mgerards@xs4all.nl Date: Wed, 13 Aug 2008 16:51:31 +0200 In-Reply-To: <1218637704.8757.60.camel@localhost> (Javier =?iso-8859-1?Q?M?= =?iso-8859-1?Q?art=EDn's?= message of "Wed, 13 Aug 2008 16:28:24 +0200") Message-ID: <87tzdp6k98.fsf@xs4all.nl> User-Agent: Gnus/5.110006 (No Gnus v0.6) Emacs/21.4 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: quoted-printable X-Virus-Scanned: by XS4ALL Virus Scanner X-detected-kernel: by monty-python.gnu.org: FreeBSD 4.6-4.9 Subject: Re: [PATCH] Drivemap module X-BeenThere: grub-devel@gnu.org X-Mailman-Version: 2.1.5 Precedence: list Reply-To: The development of GRUB 2 List-Id: The development of GRUB 2 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 13 Aug 2008 14:48:00 -0000 Hi, Javier Mart=EDn writes: > El mi=E9, 13-08-2008 a las 15:00 +0200, Robert Millan escribi=F3: >> Hi, >>=20 >> 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. >>=20 >> On Wed, Aug 13, 2008 at 02:16:25PM +0200, Javier Mart=EDn wrote: >> > + >> > +#define MODNAME "drivemap" >> > + >> > [...] >> > + grub_dprintf (MODNAME, "Removing the mapping for %s (%02x)"= , args[0], mapfrom); >>=20 >> 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. >>=20 >> [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! >>=20 >> [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 =3D "drivemap"; static const char[] modname =3D "drivemap"; We don't capatilize variables ;) =20 >> > +/* Int13h handler installer - reserves conventional memory for the ha= ndler, >> > + copies it over and sets the IVT entry for int13h.=20=20 >> > + This code rests on the assumption that GRUB does not activate any = kind of >> > + memory mapping apart from identity paging, since it accesses realm= ode >> > + structures by their absolute addresses, like the IVT at 0 or the B= DA at >> > + 0x400; and transforms a pmode pointer into a rmode seg:off far ptr= . */ >> > +static grub_err_t >> > +install_int13_handler (void) >> > +{ >>=20 >> Can this be made generic? Like "install_int_handler (int n)". We're pr= obably >> going to need interrupts for other things later on. >>=20 >> Or is this code suitable for i8086 mode interrupts only? >>=20 >> Anyway, if it's made generic, remember the handler itself becomes suitab= le 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. =20 >> > + drivemap_node_t *curentry =3D drivemap; >>=20 >> 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.=20=20 > >>=20 >> > +/* 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 >>=20 >> Is this a signature? Then a macro would be preferred, so that it can be= shared >> with whoever checks for it. >>=20 >> 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.=20=20 > >>=20 >> > +FUNCTION(grub_drivemap_int13_handler) >> > + push %bp >> > + mov %sp, %bp >> > + push %ax /* We'll need it later to determine the used BIOS functio= n. */ >>=20 >> 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].=20=20 I'll leave that to Robert :-) >> > Index: include/grub/loader.h >> > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D >> > --- include/grub/loader.h (revisi=F3n: 1802) >> > +++ include/grub/loader.h (copia de trabajo) >> > @@ -37,7 +37,23 @@ >> > /* Unset current loader, if any. */ >> > void EXPORT_FUNC(grub_loader_unset) (void); >> >=20=20 >> > -/* 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 mo= dule >> > + unload). If ABORT_ON_ERROR is set, the boot sequence will abort i= f 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 i= nformation >> > + will be available in grub_errno. However, if the call is successf= ul 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_pr= eboot.=20=20 >> > + 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); >>=20 >> This interface is added for all platforms. I didn't follow the discussi= on; >> 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.=20=20 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.=20=20 > >>=20 >> > +/* This type is used for imported assembly labels, takes no storage a= nd is only >> > + used to take the symbol address with &label. Do NOT put void* her= e. */ >> > +typedef void grub_symbol_t; >>=20 >> 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? :) >>=20 >> > Index: kern/loader.c >> > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D >> > --- kern/loader.c (revisi=F3n: 1802) >> > +++ kern/loader.c (copia de trabajo) >> > @@ -61,11 +61,88 @@ >> > grub_loader_loaded =3D 0; >> > } >> >=20=20 >> > +struct hooklist_node >> > +{ >> > + grub_err_t (*hook) (void); >> > + int abort_on_error; >> > + struct hooklist_node *next; >> > +}; >> > + >> > +static struct hooklist_node *preboot_hooks =3D 0; >> > + >> > +grub_preboot_hookid >> > +grub_loader_register_preboot (grub_err_t (*hook) (void), int abort_on= _error) >> > +{ >> > [...] >>=20 >> This is a lot of code being added to kernel, and space in kernel is high= ly >> valuable. >>=20 >> 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=E9, 13-08-2008 a las 15:01 +0200, Robert Millan escribi=F3: >> On Wed, Aug 13, 2008 at 02:16:25PM +0200, Javier Mart=EDn wrote: >> > +static grub_err_t >> > +revparse_biosdisk(const grub_uint8_t dnum, const char **output) >>=20 >> 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... =AC=AC 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