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 <[EMAIL PROTECTED]> 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 > Grub-devel@gnu.org > http://lists.gnu.org/mailman/listinfo/grub-devel
/* * GRUB -- GRand Unified Bootloader * Copyright (C) 1999,2000,2001,2002,2003,2005,2006,2007,2008 Free Software Foundation, Inc. * * GRUB is free software: you can redistribute it and/or modify * it under the terms of the GNU General Public License as published by * the Free Software Foundation, either version 3 of the License, or * (at your option) any later version. * * GRUB is distributed in the hope that it will be useful, * but WITHOUT ANY WARRANTY; without even the implied warranty of * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the * GNU General Public License for more details. * * You should have received a copy of the GNU General Public License * along with GRUB. If not, see <http://www.gnu.org/licenses/>. */ /* * Note: These functions defined in this file may be called from C. * Be careful of that you must not modify some registers. Quote * from gcc-2.95.2/gcc/config/i386/i386.h: 1 for registers not available across function calls. These must include the FIXED_REGISTERS and also any registers that can be used without being saved. The latter must include the registers where values are returned and the register where structure-value addresses are passed. Aside from that, you can include as many other registers as you like. ax,dx,cx,bx,si,di,bp,sp,st,st1,st2,st3,st4,st5,st6,st7,arg { 1, 1, 1, 0, 0, 0, 0, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1 } */ /* * Note: GRUB is compiled with the options -mrtd and -mregparm=3. * So the first three arguments are passed in %eax, %edx, and %ecx, * respectively, and if a function has a fixed number of arguments * and the number if greater than three, the function must return * with "ret $N" where N is ((the number of arguments) - 3) * 4. */ #include <grub/symbol.h> #define GRUB_DRIVEMAP_INT13H_OFFSET(x) ((x) - grub_drivemap_int13_handler_base) /* Copy starts here. When deployed, this label must be segment-aligned. */ VARIABLE(grub_drivemap_int13_handler_base) /* 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) .long 0 /* Drivemap module bundle - INT 13h handler - BIOS HD map */ /* We need to use relative addressing, and with CS to top it all, since we must make as few changes to the registers as possible. IP-relative addressing like on amd64 would make life way easier here. */ .code16 FUNCTION(grub_drivemap_int13_handler) push %bp mov %sp, %bp push %ax /* We'll need it later to determine the used BIOS function. */ /* Map the drive number (always in DL?). */ push %ax push %bx push %si mov $GRUB_DRIVEMAP_INT13H_OFFSET(grub_drivemap_int13_mapstart), %bx xor %si, %si 1:movw %cs:(%bx,%si), %ax cmp %ah, %al jz 3f /* DRV=DST => map end - drive not remapped, leave DL as-is. */ cmp %dl, %al jz 2f /* Found - drive remapped, modify DL. */ add $2, %si jmp 1b /* Not found, but more remaining, loop. */ 2:mov %ah, %dl 3:pop %si pop %bx xchgw %ax, -4(%bp) /* Recover the old AX and save the map entry for later. */ push %bp /* Simulate interrupt call: push flags and do a far call in order to set the stack the way the old handler expects it so that its iret works. */ push 6(%bp) movw (%bp), %bp /* Restore the caller BP (is this needed and/or sensible?). */ lcall *%cs:GRUB_DRIVEMAP_INT13H_OFFSET(grub_drivemap_int13_oldhandler) pop %bp /* The pushed flags were removed by iret. */ /* Set the saved flags to what the int13h handler returned. */ push %ax pushf pop %ax movw %ax, 6(%bp) pop %ax /* Reverse map any returned drive number if the data returned includes it. The only func that does this seems to be origAH = 0x08, but many BIOS refs say retDL = # of drives connected. However, the GRUB Legacy code treats this as the _drive number_ and "undoes" the remapping. Thus, this section has been disabled for testing if it's required. */ # cmpb $0x08, -1(%bp) /* Caller's AH. */ # jne 4f # xchgw %ax, -4(%bp) /* Map entry used. */ # cmp %ah, %al /* DRV=DST => drive not remapped. */ # je 4f # mov %ah, %dl /* Undo remap. */ 4:mov %bp, %sp pop %bp iret /* This label MUST be at the end of the copied block, since the installer code reserves additional space for mappings at runtime and copies them over it. */ .align 2 VARIABLE(grub_drivemap_int13_mapstart) /* Copy stops here. */ .code32 VARIABLE(grub_drivemap_int13_size) .word GRUB_DRIVEMAP_INT13H_OFFSET(grub_drivemap_int13_size)
/* * GRUB -- GRand Unified Bootloader * Copyright (C) 1999,2000,2001,2002,2003,2005,2006,2007,2008 Free Software Foundation, Inc. * * GRUB is free software: you can redistribute it and/or modify * it under the terms of the GNU General Public License as published by * the Free Software Foundation, either version 3 of the License, or * (at your option) any later version. * * GRUB is distributed in the hope that it will be useful, * but WITHOUT ANY WARRANTY; without even the implied warranty of * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the * GNU General Public License for more details. * * You should have received a copy of the GNU General Public License * along with GRUB. If not, see <http://www.gnu.org/licenses/>. */ /* * Note: These functions defined in this file may be called from C. * Be careful of that you must not modify some registers. Quote * from gcc-2.95.2/gcc/config/i386/i386.h: 1 for registers not available across function calls. These must include the FIXED_REGISTERS and also any registers that can be used without being saved. The latter must include the registers where values are returned and the register where structure-value addresses are passed. Aside from that, you can include as many other registers as you like. ax,dx,cx,bx,si,di,bp,sp,st,st1,st2,st3,st4,st5,st6,st7,arg { 1, 1, 1, 0, 0, 0, 0, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1 } */ /* * Note: GRUB is compiled with the options -mrtd and -mregparm=3. * So the first three arguments are passed in %eax, %edx, and %ecx, * respectively, and if a function has a fixed number of arguments * and the number if greater than three, the function must return * with "ret $N" where N is ((the number of arguments) - 3) * 4. */ #include <grub/symbol.h> .intel_syntax noprefix #define GRUB_DRIVEMAP_INT13H_OFFSET(x) ((x) - grub_drivemap_int13_handler_base) /* Copy starts here. When deployed, this label must be segment-aligned. */ VARIABLE(grub_drivemap_int13_handler_base) /* 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) .long 0 /* Drivemap module bundle - INT 13h handler - BIOS HD map */ /* We need to use relative addressing, and with CS to top it all, since we must make as few changes to the registers as possible. IP-relative addressing like on amd64 would make life way easier here. */ .code16 FUNCTION(grub_drivemap_int13_handler) push bp mov bp, sp push ax /* We'll need it later to determine the used BIOS function. */ /* Map the drive number (always in DL?). */ push ax push bx push si mov bx, offset GRUB_DRIVEMAP_INT13H_OFFSET(grub_drivemap_int13_mapstart) xor si, si 1:mov ax, word ptr cs:[bx + si] cmp al, ah jz 3f /* DRV=DST => map end - drive not remapped, leave DL as-is. */ cmp al, dl jz 2f /* Found - drive remapped, modify DL. */ add si, 2 jmp 1b /* Not found, but more remaining, loop. */ 2:mov dl, ah 3:pop si pop bx xchg ax, [bp - 4] /* Recover the old AX and save the map entry for later. */ push bp /* Simulate interrupt call: push flags and do a far call in order to set the stack the way the old handler expects it so that its iret works. */ push [bp + 6] mov bp, word ptr [bp] /* Restore the caller BP (is this needed and/or sensible?). */ call dword ptr cs:[GRUB_DRIVEMAP_INT13H_OFFSET(grub_drivemap_int13_oldhandler)] pop bp /* The pushed flags were removed by iret. */ /* Set the saved flags to what the int13h handler returned. */ push ax pushf pop ax mov word ptr [bp + 6], ax pop ax /* Reverse map any returned drive number if the data returned includes it. The only func that does this seems to be origAH = 0x08, but many BIOS refs say retDL = # of drives connected. However, the GRUB Legacy code treats this as the _drive number_ and "undoes" the remapping. Thus, this section has been disabled for testing if it's required. */ # cmp byte ptr [bp - 1], 0x08 /* Caller's AH. */ # jne 4f # xchg ax, [bp - 4] /* Map entry used. */ # cmp al, ah /* DRV=DST => drive not remapped. */ # je 4f # mov dl, ah /* Undo remap. */ 4:mov sp, bp pop bp iret /* This label MUST be at the end of the copied block, since the installer code reserves additional space for mappings at runtime and copies them over it. */ .align 2 VARIABLE(grub_drivemap_int13_mapstart) /* Copy stops here. */ .code32 VARIABLE(grub_drivemap_int13_size) .word GRUB_DRIVEMAP_INT13H_OFFSET(grub_drivemap_int13_size)
signature.asc
Description: Esta parte del mensaje está firmada digitalmente
_______________________________________________ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
