From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with archive (Exim 4.43) id 1LiG9b-00073k-C6 for mharc-grub-devel@gnu.org; Fri, 13 Mar 2009 18:46:59 -0400 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1LiG9Z-00070U-2w for grub-devel@gnu.org; Fri, 13 Mar 2009 18:46:57 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1LiG9T-0006yF-K7 for grub-devel@gnu.org; Fri, 13 Mar 2009 18:46:55 -0400 Received: from [199.232.76.173] (port=48019 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1LiG9T-0006yC-GE for grub-devel@gnu.org; Fri, 13 Mar 2009 18:46:51 -0400 Received: from mx20.gnu.org ([199.232.41.8]:4728) by monty-python.gnu.org with esmtps (TLS-1.0:RSA_AES_256_CBC_SHA1:32) (Exim 4.60) (envelope-from ) id 1LiG9S-00034M-Nr for grub-devel@gnu.org; Fri, 13 Mar 2009 18:46:50 -0400 Received: from aybabtu.com ([69.60.117.155]) by mx20.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1LiG9S-0004CP-4s for grub-devel@gnu.org; Fri, 13 Mar 2009 18:46:50 -0400 Received: from [192.168.10.10] (helo=thorin) by aybabtu.com with esmtp (Exim 4.69) (envelope-from ) id 1LiG0m-0000Nt-Iq for grub-devel@gnu.org; Fri, 13 Mar 2009 23:37:54 +0100 Received: from rmh by thorin with local (Exim 4.69) (envelope-from ) id 1LiG97-0000DP-Us for grub-devel@gnu.org; Fri, 13 Mar 2009 23:46:29 +0100 Date: Fri, 13 Mar 2009 23:46:29 +0100 From: Robert Millan To: The development of GRUB 2 Message-ID: <20090313224629.GA781@thorin> References: <49AB29BA.6040609@gmail.com> <20090311211541.GC29956@thorin> <49B82B65.3080506@gmail.com> <20090313191442.GC17068@thorin> <49BAC506.2030006@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <49BAC506.2030006@gmail.com> 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-kernel: by mx20.gnu.org: Genre and OS details not recognized. X-detected-operating-system: by monty-python.gnu.org: GNU/Linux 2.6, seldom 2.4 (older, 4) Subject: Re: ELF bugfixes 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: Fri, 13 Mar 2009 22:46:57 -0000 On Fri, Mar 13, 2009 at 09:41:42PM +0100, phcoder wrote: > Robert Millan wrote: >> On Wed, Mar 11, 2009 at 10:21:41PM +0100, phcoder wrote: >>> Robert Millan wrote: >>>> On Mon, Mar 02, 2009 at 01:35:06AM +0100, phcoder wrote: >>>>> + * include/grub/elf.h: added missing attributes >>>> This should be a bit more descriptive. >>>> >>>>> for (i = 0; i < ehdr->e_phnum; i++) >>>>> if (phdr(i)->p_type == PT_LOAD && phdr(i)->p_filesz != 0) >>>>> { >>>>> - if (phdr(i)->p_paddr < phdr(lowest_segment)->p_paddr) >>>>> + if (lowest_segment == -1 + || phdr(i)->p_paddr < >>>>> phdr(lowest_segment)->p_paddr) >>>>> lowest_segment = i; >>>>> - if (phdr(i)->p_paddr > phdr(highest_segment)->p_paddr) >>>>> + if (highest_segment == -1 >>>>> + || phdr(i)->p_paddr > phdr(highest_segment)->p_paddr) >>>>> highest_segment = i; >>>>> } >>>> Why? >>> Because if first segment doesn't have the PT_LOAD attribute set then >>> it should be considered in this comparison >> >> But you didn't remove the PT_LOAD check. And in the routine below that >> does the actual segment load, we still check for PT_LOAD. Those should be >> consistent, right? >> > > No I expressed myself badly. Original code assumed that first segment > has PT_LOAD always set (lowest_segment is 0 initally). I removed this > assumption Why do we care about non-PT_LOAD segments? >>>>> - grub_multiboot_payload_entry_offset = ehdr->e_entry - phdr(lowest_segment)->p_vaddr; >>>>> + grub_multiboot_payload_entry_offset = ehdr->e_entry - phdr(lowest_segment)->p_paddr; >>>> Are you sure about this? IIRC e_entry is in the virtual address space. I >>>> think we had some trouble with this (with NetBSD?), which lead to the current >>>> use of p_vaddr in this line. >>>> >>> Actually now thinking I see that the problem is more deep. The >>> section which is loaded at the lowest address isn't necessarily the >>> section which contains entry point. I'll fix this part cleanly and >>> will resubmit the patch >> >> No, but AFAICT the entry point is defined relative to that address, regardless >> of which segment contains it. >> > Actually our segment table is also our table for transforming between > virtual and physical address. I don't see why entry point would be > defined against virtual address of lowest physical segement I think entry point is supposed to be defined in virtual address space. As to why do we check for physical addresses earlier, I'm not entirely sure. I think the idea was that we store the entry point as an offset, so that it can be applied to physical addresses, despite the fact that we obtained it by comparing e_entry with a virtual address. ISTR this being an issue for NetBSD. We should be certain what we do before changing it. In particular, the following commit seem relevant: 2008-02-05 Bean * loader/i386/pc/multiboot.c (grub_multiboot_load_elf32): Get physical address of entry. I'd also recommend testing your changes with NetBSD's kernel. -- 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."