On Mon, Aug 03, 2009 at 02:06:03PM +0200, Vladimir 'phcoder' Serbinenko wrote: > Hello. As discussed on IRC it would be preferable if all loaders use > flexible technics for loading kernels using relocators as currently > multiboot and xnu does.
Well, as for what I said on IRC, I'm not sure if it'd be preferable. I think it *could* be, if it makes things simpler & more maintainable (which greatly depends on how the patches for multiboot.c/linux.c look like). Anyway, I'll comment some things on this patch: > +#ifdef __x86_64__ > +extern grub_uint64_t grub_relocator32_backward_src; > +#else > +extern grub_uint32_t grub_relocator32_backward_src; > +#endif You could make this a pointer, or grub_uintptr_t (the latter we don't yet have, it seems like a good excuse to add it if a pointer is not suitable :-)). > +#ifndef __x86_64__ > + /* mov imm32, %eax */ > + .byte 0xb8 > +RELOCATOR_VARIABLE(dest) > + .long 0 > + mov %eax, %edi Please use size qualifiers (e.g. movl). Also, remember to indent the first parameter like we do elsewhere (e.g. ".long\t0", "movl\t%eax, %edi", etc). > + /* Backward movsl is implicitly off-by-four. compensate that. */ > + subl $4, %esi > + subl $4, %edi > + > + /* Backward copy. */ > + std > + addl %ecx, %esi > + addl %ecx, %edi > + > + rep > + movsl Are you sure moving to movsl is a good idea? Maybe the payload size is not 4-aligned. > +#ifdef APPLE_CC > + add $(cont0 - base), %eax > +#else > + add $(cont0 - base), %rax > +#endif What's the issue at hand? Apple assembler [1] can't add an inmediate to %rax ? Truncating it seems like it could be problematic if %eax overflows. [1] btw, APPLE_CC macro for assembler checks is really confusing > + /* Update %cs. Thanks to David Miller for pointing this mistake out. */ > + ljmp *(jump_vector - base) (%esi,1) Comments ought to be for useful information. For praise/acknoledgements (and I think David deserves it) we have the THANKS file. > + /* Disable paging. */ > + mov %cr0, %eax > + and $0x7fffffff, %eax > + mov %eax, %cr0 > + > + /* Disable amd64. */ > + mov $0xc0000080, %ecx > + rdmsr > + and $0xfffffeff, %eax > + wrmsr > + > + /* Turn off PAE. */ > + movl %cr4, %eax > + and $0xffffffcf, %eax > + mov %eax, %cr4 Please use macros for such things (see GRUB_MEMORY_MACHINE_CR0_PE_ON). -- 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." _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
