Re: [Xen-devel] [PATCH v2] xen: arm: rework placement of fdt in initial dom0 memory map

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

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
> > <http://www.arm.linux.org.uk/developer/patches/viewpatch.php?id=7824/1>. 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 <ian.campbell@xxxxxxxxxx>
> > ---
> > 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?

> 
> > 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;
> > 
> 
> 



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel