From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with archive (Exim 4.43) id 1KTHKL-0001Uw-HD for mharc-grub-devel@gnu.org; Wed, 13 Aug 2008 10:27:53 -0400 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1KTHKJ-0001Uo-MB for grub-devel@gnu.org; Wed, 13 Aug 2008 10:27:51 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1KTHKI-0001Uc-VN for grub-devel@gnu.org; Wed, 13 Aug 2008 10:27:51 -0400 Received: from [199.232.76.173] (port=52103 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1KTHKI-0001UZ-R8 for grub-devel@gnu.org; Wed, 13 Aug 2008 10:27:50 -0400 Received: from ik-out-1112.google.com ([66.249.90.176]:48213) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1KTHKF-0001iZ-CB for grub-devel@gnu.org; Wed, 13 Aug 2008 10:27:47 -0400 Received: by ik-out-1112.google.com with SMTP id c21so9696ika.2 for ; Wed, 13 Aug 2008 07:27:44 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:received:received:subject:from:to:in-reply-to :references:content-type:date:message-id:mime-version:x-mailer; bh=sNL/6ztXjBJbQcXCRvtkCmuUqwpCm262UTbVMZqbtUc=; b=C0loyp/UlS9uJ4oBqasEtbfCR6WrxvtrJvyHTdj6bNn5QgnHcH4dOip5h3yA8+MWGr rqWGfkHOxQcV2Amssv06+2KX/XxdulnQBfERQ67NUXxgNCRW1EqmL0xfs7owbtWyZ5oY ICtq6LNCQokO6CbCLzNt1CHSqRv41mrCj8Gtc= DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=subject:from:to:in-reply-to:references:content-type:date:message-id :mime-version:x-mailer; b=BWtRKLftpbBbXTcGuA5EiSoXYqSYiXdD4LksXOyUloZ5pONpZPgaAfnmC1L6YoBiCL 0FCx0n6ocHpwr6ftSgYxtLUWH53kpFrxeNHBrJ1rXyOQ9VRK4560tv04CK+y2LAXtlvC 9KOS18Tw69n9Yh/eeIVrQ3FaLZZmg8QJTUobY= Received: by 10.210.125.7 with SMTP id x7mr11984392ebc.45.1218637664441; Wed, 13 Aug 2008 07:27:44 -0700 (PDT) Received: from ?192.168.1.100? ( [213.37.137.93]) by mx.google.com with ESMTPS id z37sm436524ikz.6.2008.08.13.07.27.41 (version=SSLv3 cipher=RC4-MD5); Wed, 13 Aug 2008 07:27:43 -0700 (PDT) From: Javier =?ISO-8859-1?Q?Mart=EDn?= To: The development of GRUB 2 In-Reply-To: <20080813130022.GB26618@thorin> 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> Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="=-GfQkjYRjaCuDLgzkD2A7" Date: Wed, 13 Aug 2008 16:28:24 +0200 Message-Id: <1218637704.8757.60.camel@localhost> Mime-Version: 1.0 X-Mailer: Evolution 2.22.3.1 X-detected-kernel: by monty-python.gnu.org: Linux 2.6 (newer, 2) 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:27:52 -0000 --=-GfQkjYRjaCuDLgzkD2A7 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable El mi=C3=A9, 13-08-2008 a las 15:00 +0200, Robert Millan escribi=C3=B3: > Hi, >=20 > 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. >=20 > On Wed, Aug 13, 2008 at 02:16:25PM +0200, Javier Mart=C3=ADn 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 w= e > are to do it, IMHO this should really be done globally for consistency, a= nd > 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"; >=20 > > +/* Int13h handler installer - reserves conventional memory for the han= dler, > > + copies it over and sets the IVT entry for int13h. =20 > > + This code rests on the assumption that GRUB does not activate any k= ind of > > + memory mapping apart from identity paging, since it accesses realmo= de > > + structures by their absolute addresses, like the IVT at 0 or the BD= A 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 pro= bably > 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 suitabl= e 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. >=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 > > +/* 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 > > +FUNCTION(grub_drivemap_int13_handler) > > + push %bp > > + mov %sp, %bp > > + push %ax /* We'll need it later to determine the used BIOS function= . */ >=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 > > 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=C3=B3n: 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 > > -/* 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 mod= ule > > + 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 in= formation > > + will be available in grub_errno. However, if the call is successfu= l 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_pre= boot. =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 discussio= n; > 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 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 > > +/* This type is used for imported assembly labels, takes no storage an= d is only > > + used to take the symbol address with &label. Do NOT put void* here= . */ > > +typedef void grub_symbol_t; >=20 > I think this name is too generic for such an specific purpose. What about "grub_asmsymbol_t"? >=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=C3=B3n: 1802) > > +++ kern/loader.c (copia de trabajo) > > @@ -61,11 +61,88 @@ > > grub_loader_loaded =3D 0; > > } > > =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 highl= y > 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=C3=A9, 13-08-2008 a las 15:01 +0200, Robert Millan escribi=C3=B3: > On Wed, Aug 13, 2008 at 02:16:25PM +0200, Javier Mart=C3=ADn 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... =C2=AC=C2=AC 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 --=-GfQkjYRjaCuDLgzkD2A7 Content-Type: application/pgp-signature; name=signature.asc Content-Description: Esta parte del mensaje =?ISO-8859-1?Q?est=E1?= firmada digitalmente -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.6 (GNU/Linux) iQIVAwUASKLviKSl+Fbdeo72AQIpVhAAmCyjrrgwhLAVXIX25jumUqGAqbMEtaKy HQBm55pOer+hcwAAwcBOxu3B3m6SLtD79qSxCDIbj2oKZspPikwk9FOif9GOxyFe VJRuLF2U1XjGM2mDFiw0utzJTN5q5e9hWQ/BobanCsTMl39P++zzsrQmtQaK1dtU mO2GH2NTA6xUdXf8lCZbdAmZv6l0MCr6K7zShpqZYG5MpZaX9ld3AoB5S/snQGRv oAKKwYPEqtUqIO1KeAsOQHrT2F7m9SFR1/geRvf/kY2vvBEA6ieW86J8Vjt3GVUQ B1vE1MCSAHa8uKKFplh+B4uraDX4QJBNWohZc0TK8KCQiMLiDNNv7u0mKKEyE4Uz 99eIkfN7cKnpHSzqxhGHnMGE0wLGhvoJb1Lqz4yN6PBGms1NVJQ/WXHTTnGXt9Q9 hiFCbuyZm/bJ0++WNaIUvyK/7y6pUkI5f+2ImsLokgXas5gP3sPf4pE8W8T2SIDf vWD6+A+Lf5IFl3IkAaopwlNLrKs7slUENvs0VLf+lYvtfYGd/g6fMbClBvEjeyBV Yj94u031j8HiX9nI494E7VevBEbIkv4jCNU8VlR3eMiZEADYR0AxdJWJMpdudIkW RHOnc8hH1Nc59jTJ14cEYckA4aeJebQUlUdfb+QPM6plJRwE9MH9C1nCCJGXZDQ4 3EzOn/2lAqc= =IRQs -----END PGP SIGNATURE----- --=-GfQkjYRjaCuDLgzkD2A7--