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

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

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