From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ian Campbell Subject: Re: [PATCH v2] xen: arm: rework placement of fdt in initial dom0 memory map Date: Tue, 17 Sep 2013 16:23:23 +0100 Message-ID: <1379431403.11304.139.camel@hastur.hellion.org.uk> References: <1379082418-22871-1-git-send-email-ian.campbell@citrix.com> <52332CBA.5090205@linaro.org> <1379086980.19256.91.camel@kazak.uk.xensource.com> <52333B3B.4030201@linaro.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <52333B3B.4030201@linaro.org> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Julien Grall Cc: stefano.stabellini@eu.citrix.com, tim@xen.org, xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org On Fri, 2013-09-13 at 17:20 +0100, Julien Grall wrote: > >> I have noticed that the check below is wrong > >> if ( fdt_totalsize(...) > end ) > >> > >> Can you fix the check in this patch? > > > > What's wrong with it? > > > > if ( fdt_totalsize(...) > end ) then the dtb_paddr will have underflowed > > and we panic. Or is that not what you are referring to? > > end is an absolute address and fdt_totalsize(...) is relative. > > I think the check should be > (fdt_totalsize(...) + mem.bank[0].start) > end I think we also want dtb_paddr < mem.bank[0].start to cope with underflow. > > > > >> > >>> diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c > >>> index f12f895..e4c0981 100644 > >>> --- a/xen/arch/arm/kernel.c > >>> +++ b/xen/arch/arm/kernel.c > >>> @@ -211,11 +211,32 @@ static int kernel_try_zimage32_prepare(struct kernel_info *info, > >>> info->zimage.kernel_addr = addr; > >>> > >>> /* > >>> - * If start is zero, the zImage is position independent -- load it > >>> - * at 32k from start of RAM. > >>> + * If start is zero, the zImage is position independent, in this > >>> + * case Documentation/arm/Booting recommends loading below 128MiB > >>> + * and above 32MiB. Load it as high as possible within these > >>> + * constraints, while also avoiding the DTB. > >>> */ > >>> if (start == 0) > >>> - info->zimage.load_addr = info->mem.bank[0].start + 0x8000; > >>> + { > >>> + paddr_t load_end; > >>> + > >>> + load_end = info->mem.bank[0].start + info->mem.bank[0].size; > >>> + load_end = MIN(info->mem.bank[0].start + (128<<20), load_end); > >>> + > >>> + /* > >>> + * FDT is loaded above 128M or as high as possible, so the > >>> + * only way we can clash is if we have <=128MB, in which case > >>> + * FDT will be right at the end and so dtb_paddr will be below > >>> + * the proposed kernel load address. Move the kernel down if > >>> + * necessary. > >>> + */ > >>> + if ( load_end >= info->dtb_paddr ) > >>> + load_end = info->dtb_paddr; > >>> + > >>> + info->zimage.load_addr = load_end - end + start; > >> > >> Actually start is always equals to 0, so you don't need to add it. > > > > Oh yes. > > > >> > >> In the future, we will need some check here to verify the kernel belongs > >> to bank 0 and start won't. > >> > >>> + /* Align to 2MB */ > >>> + info->zimage.load_addr &= ~((2 << 20) - 1); > >>> + } > >>> else > >>> info->zimage.load_addr = start; > >>> info->zimage.len = end - start; > >>> > >> > >> > > > > > >