Re: [PATCH] Drivemap module

[Robert Millan]

On Wed, Aug 13, 2008 at 04:28:24PM +0200, Javier Martín wrote:
> > 
> > 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";

Yes, but I'd merge this change separately from your drivemap patch (either
before or after, as you prefer), for the whole of the codebase.  Doesn't make
sense to do it in some places but not in others, IMHO.

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

Fair enough.  The addr=0 assumption sounds troubling, though.  Why not use
sidt instead?

> > > +/* 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.  

Ok.

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

For consistency (I'm so predictable).  We do the same everywhere else.  Also,
I think some versions of binutils reject instructions that don't have size
qualifiers.

And it's more readable for people used to gas (I know, it's also less readable
for people used to tasm or so).

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

Having a small kernel is highly desireable for most users.  If the kernel is
too big, it won't fit and then either we have to use blocklists (which are
unreliable), or we have to abort the install.

Please, try to find a way that doesn't increase kernel size significantly.

If the kernel interfaces are not extensible enough, you could try to readjust
them for your needs.  This approach works well most of the time (although I
haven't studied this particular problem in detail).

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

Actually, I just spotted it by chance.  But don't you give me ideas ;-)

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

Okay.  Looking forward to a very high quality drivemap patch from you :-)

-- 
Robert Millan

  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
  how) you may access your data; but nobody's threatening your freedom: we
  still allow you to remove your data and not access it at all."