Re: [PATCH v5 07/19] xen: arm: allocate dom0 memory separately from preparing the dtb

[Ian Campbell <Ian.Campbell@citrix.com>]

On Thu, 2013-11-14 at 00:52 +0000, Julien Grall wrote:
> 
> On 11/13/2013 06:11 PM, Ian Campbell wrote:
> > Mixing these two together is a pain, it forces us to prepare the dtb before
> > processing the kernel which means we don't know whether the guest is 32- or
> > 64-bit while we construct its DTB.
> >
> > Instead split out the memory allocation (including 1:1 workaround handling)
> > and p2m setup into a separate phase and then create a memory node in the DTB
> > based on the result.
> >
> > This allows us to move kernel parsing before DTB setup.
> >
> > As part of this it was also necessary to rework where the decision regarding
> > the placement of the DTB and initrd in RAM was made. It is now made when
> > loading the kernel, which allows it to make use of the zImage/ELF specific
> > information and therefore to make decisions based on complete knowledge and do
> > it right rather than guessing in prepare_dtb and relying on a later check to
> > see if things worked.
> >
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > ---
> > v3: Also rework module placement, v2 broke boot because dtb_paddr wasn't set
> >      soon enough. This ends up cleaner anyway.
> > v2: Fixed typo in the commit log
> >      Handle multiple memory nodes as well as individual nodes with several
> >      entries in them.
> >      Strip the original memory node and recreate rather than trying to modify.
> > ---
> >   xen/arch/arm/domain_build.c |  203 ++++++++++++++++++++++---------------------
> >   xen/arch/arm/kernel.c       |   80 +++++++++++------
> >   xen/arch/arm/kernel.h       |    2 -
> >   3 files changed, 158 insertions(+), 127 deletions(-)
> 
> [..]
> 
> > @@ -236,17 +267,9 @@ static int kernel_try_zimage32_prepare(struct kernel_info *info,
> >           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;
> > +        load_end = MIN(info->mem.bank[0].start + MB(128), load_end);
> > +
> > +        load_end += MB(2);
> 
> I didn't notice this line during my previous review, why do you add MB(2)?

I've no idea... Perhaps I thought I was aligning it, but that's clearly
wrong. Or maybe it is left over debug from trying to force an error
condition etc. I'll drop it...

Ian.