Re: [PATCH] Drivemap module

[Javier Martín]

Ok, making a mixup reply...

El mié, 13-08-2008 a las 17:14 +0200, Robert Millan escribió: 
> 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.
Urgh... It's already been implemented in drivemap, why not put it in
with it, then change everything else? Or should I just keep the #define
in the meantime?

> 
> > 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?
Well, so is the assumption that GRUB does not enable any kind of paging
or memory remapping for that matter. WRT sidt, I think that could be
better implemented as a function in kern/i386/pc called
grub_machine_get_rmove_ivt() or SLT, because it requires dropping to
rmode, executing sidt, going back to pmode and then returning the
address, modified to be valid in pmode as required.

This approach would cost a few bytes in kernel (I can hear you screaming
already), but it would be extensible for the future "interrupt
installer" you envisioned, and it would take care of the paging/mappings
if there ever were any. Same for a function to get the address of the
BDA or other machine-specific addresses.

> 
> > > > +/* 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.
Version 0.1 (alpha 2) maybe? IIRC, it's been long since the size
qualifiers inference is available in GAS.  
> 
> And it's more readable for people used to gas (I know, it's also less readable
> for people used to tasm or so).
TASM? Don't even name the devil.  I like GAS and its directives (even
better and NASM and YASM), but many of the conventions of the AT&T
syntax are at best broken (i.e. src, dest looks good, but breaks on FP
instructions), and memory references are a royal PITA.  Given that the
code is _not_ platform-portable and that a PPC dev will not understand
it even in AT&T syntax, why bother with it...  

However, I take it that your advertised predictability means that I
should consider the "int13h in intel syntax" request denied.
Nevertheless, I've attached the two versions of the asm file, so you can
check which one is less of a mind boggler. Both assemble to _exactly_
the same machine code (checked with gcc + objdump).


El mié, 13-08-2008 a las 17:57 +0200, Marco Gerards escribió:
> Robert Millan <address@hidden> writes:
> 
> [...]
> 
> >> > 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).
> 
> Like discussed before.  Bring up such modifications like hooks up in a
> *separate* thread.  I already said that not everyone reads this
> discussion.  I will not accept a patch that changes the kernel if it
> is part of a bigger patch that not many people read.
> 
> Please don't discuss this over with Robert and me, you know that it
> was pointed out that this has to be a patch in a separate thread.
> Furthermore, this is a way to get some feedback from Bean who wants
> something similar, IIRC.

I know I will be regretting saying this, but it is _very_ rude to review
some five versions of the patch, spotting mostly coding-style errors on
each, and then, on version 8, tell me that "you won't accept a patch
that contains blah" (with "blah" being essential for the patch to work).
Quite the proverbial slap in the face to me.

There was a discussion with other people sometime near May and June,
true, and many people were mildly opposed to the current design, but I
think I had proved that while the implementation could change (e.g. to a
fixed-size array), the interface is pretty much as simple as it can get.
Besides, and given that it is (and will be for the foreseeable future)
only used by drivemap, changes to it should be pretty manageable and
havoc-less.

WRT Bean's hook proposal, even the BOOT_PRELOAD I've seen discussed
works before the _high-level_ "boot" command is issued, while my patch
acts from within grub_loader_boot, before grub_X_real_boot is called.
However, given that the only actual difference in that timing is that my
code works after loader.c has checked whether a kernel has been loaded
and that can be checked from "userland" (i.e. modules), I could
eventually use Bean's hooks for drivemap. I've searched the archives and
haven't found any code, though. Could I get a cookie, er... pointer?

-Habbit

> 
> --
> Marco
> 
> 
> 
> _______________________________________________
> Grub-devel mailing list
> address@hidden
> http://lists.gnu.org/mailman/listinfo/grub-devel

drivemap_int13h.S
Description: Text Data

drivemap_int13h_intel.S
Description: Text Data

signature.asc
Description: Esta parte del mensaje está firmada digitalmente