From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [PATCH v2] xen: arm: rework placement of fdt in initial dom0 memory map Date: Fri, 13 Sep 2013 17:20:11 +0100 Message-ID: <52333B3B.4030201@linaro.org> References: <1379082418-22871-1-git-send-email-ian.campbell@citrix.com> <52332CBA.5090205@linaro.org> <1379086980.19256.91.camel@kazak.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1379086980.19256.91.camel@kazak.uk.xensource.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ian Campbell Cc: stefano.stabellini@eu.citrix.com, tim@xen.org, xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org On 09/13/2013 04:43 PM, Ian Campbell wrote: > On Fri, 2013-09-13 at 16:18 +0100, Julien Grall wrote: >> On 09/13/2013 03:26 PM, Ian Campbell wrote: >>> The 32-bit Linux kernel uses its lowmem direct mapping to access the FDT. The >>> lowmem mapping is around 0.75GiB but varies depending on the kernel's .config. >>> Our current scheme of loading the FDT as high as 4GB therefore fails with >>> larger amounts of dom0 RAM. >>> >>> The upstream documentation has recently been update to provide more guidance >>> . In >>> accordance with this load the kernel just below 128MiB (aligned to 2MB) and >>> the FDT just above, or if there is less RAM available then as high as >>> possible. >>> >>> Signed-off-by: Ian Campbell >>> --- >>> v2: Use the kernels own reported size rather than the size of the module. This >>> is consistent with the amount we will actually copy (zimage.len) >>> v1: This is technically v2 of "xen: arm: load FDT below 0.5G" >>> --- >>> xen/arch/arm/domain_build.c | 16 +++++++++++----- >>> xen/arch/arm/kernel.c | 27 ++++++++++++++++++++++++--- >>> 2 files changed, 35 insertions(+), 8 deletions(-) >>> >>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c >>> index 77e2a61..172c985 100644 >>> --- a/xen/arch/arm/domain_build.c >>> +++ b/xen/arch/arm/domain_build.c >>> @@ -503,14 +503,20 @@ static int prepare_dtb(struct domain *d, struct kernel_info *kinfo) >>> if ( ret < 0 ) >>> goto err; >>> >>> + /* Actual new size */ >>> + new_size = fdt_totalsize(kinfo->fdt); >>> + >>> /* >>> - * DTB must be load below 4GiB and far enough from linux (Linux uses >>> - * the space after it to decompress) >>> - * Load the DTB at the end of the first bank, while ensuring it is >>> - * also below 4G >>> + * DTB must be loaded such that it does not conflict with the >>> + * kernel decompressor. For 32-bit Linux Documentation/arm/Booting >>> + * recommends just after the 128MB boundary while for 64-bit Linux >>> + * the recommendation in Documentation/arm64/booting.txt is below >>> + * 512MB. Place at 128MB, (or, if we have less RAM, as high as >>> + * possible) in order to satisfy both. >>> */ >>> end = kinfo->mem.bank[0].start + kinfo->mem.bank[0].size; >>> - end = MIN(1ull << 32, end); >>> + end = MIN(kinfo->mem.bank[0].start + (128<<20) + new_size, end); >>> + >>> kinfo->dtb_paddr = end - fdt_totalsize(kinfo->fdt); >> >> Can you use new_size here? > > Yes. > >> >>> /* Align the address to 2Mb. Linux only requires 4 byte alignment */ >>> kinfo->dtb_paddr &= ~((2 << 20) - 1); >> >> 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 > >> >>> 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; >>> >> >> > > -- Julien Grall