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

[Julien Grall <julien.grall@linaro.org>]

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)?

-- 
Julien Grall