From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with archive (Exim 4.43) id 1MYR0S-0004rp-Ep for mharc-grub-devel@gnu.org; Tue, 04 Aug 2009 16:53:12 -0400 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1MYR0Q-0004re-AF for grub-devel@gnu.org; Tue, 04 Aug 2009 16:53:10 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1MYR0L-0004mv-PL for grub-devel@gnu.org; Tue, 04 Aug 2009 16:53:10 -0400 Received: from [199.232.76.173] (port=40441 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1MYR0L-0004ms-NZ for grub-devel@gnu.org; Tue, 04 Aug 2009 16:53:05 -0400 Received: from xvm-190-8.ghst.net ([217.70.190.8]:46267 helo=aybabtu.com) by monty-python.gnu.org with esmtps (TLS-1.0:RSA_AES_256_CBC_SHA1:32) (Exim 4.60) (envelope-from ) id 1MYR0L-0003xx-6L for grub-devel@gnu.org; Tue, 04 Aug 2009 16:53:05 -0400 Received: from [192.168.10.10] (helo=thorin) by aybabtu.com with esmtp (Exim 4.69) (envelope-from ) id 1MYR0F-000571-3W for grub-devel@gnu.org; Tue, 04 Aug 2009 22:53:01 +0200 Received: from rmh by thorin with local (Exim 4.69) (envelope-from ) id 1MYR0C-0004Dk-B2 for grub-devel@gnu.org; Tue, 04 Aug 2009 22:52:56 +0200 Date: Tue, 4 Aug 2009 22:52:56 +0200 From: Robert Millan To: The development of GRUB 2 Message-ID: <20090804205256.GE15811@thorin> References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Organization: free as in freedom X-Message-Flag: Worried about Outlook viruses? Switch to Thunderbird! www.mozilla.com/thunderbird X-Debbugs-No-Ack: true User-Agent: Mutt/1.5.18 (2008-05-17) X-detected-operating-system: by monty-python.gnu.org: GNU/Linux 2.6 (newer, 3) Subject: Re: [PATCH 1/2] Relocator framework 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: Tue, 04 Aug 2009 20:53:10 -0000 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."