Re: [PATCH v3 03/14] xen: arm: allocate dom0 memory separately from preparing the dtb

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

On 11/07/2013 08:44 AM, 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.

Your solution to create the memory node won't work in some case. From 
the EPAR, memory nodes can be everywhere. So we can have a device tree 
like that:

/ {
   motherboard
   {
      #address-cells = 2
      #size-cells = 2
      memory {
         device_type = "memory";
         reg = < ... >
      }
   }
}

Here, the root (/) has #address-cells = 2 and #size-cells = 1, that is 
the default value. As you will create the memory node in slash, you will 
loose 1 cell of the size.

> This allows us to move kernel parsing before DTB setup.

Why do you want to move the kernel parsing earlier? Xen don't use 
d->arch.type during dom0 building.

> 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 |  198 ++++++++++++++++++++++---------------------
>   xen/arch/arm/kernel.c       |   80 +++++++++++------
>   xen/arch/arm/kernel.h       |    2 -
>   3 files changed, 155 insertions(+), 125 deletions(-)
>

[..]

>   static void kernel_elf_load(struct kernel_info *info)
>   {
> +    place_modules(info,
> +                  info->elf.parms.virt_kstart,
> +                  info->elf.parms.virt_kend);
> +
>       printk("Loading ELF image into guest memory\n");
>       info->elf.elf.dest_base = (void*)(unsigned long)info->elf.parms.virt_kstart;
>       info->elf.elf.dest_size =
>            info->elf.parms.virt_kend - info->elf.parms.virt_kstart;
> +

spurious line?

>       elf_load_binary(&info->elf.elf);
>
>       printk("Free temporary kernel buffer\n");
> @@ -321,7 +348,6 @@ static int kernel_try_elf_prepare(struct kernel_info *info,
>        */
>       info->entry = info->elf.parms.virt_entry;
>       info->load = kernel_elf_load;
> -    info->check_overlap = NULL;
>
>       if ( elf_check_broken(&info->elf.elf) )
>           printk("Xen: warning: ELF kernel broken: %s\n",
> diff --git a/xen/arch/arm/kernel.h b/xen/arch/arm/kernel.h
> index debf590..b48c2c9 100644
> --- a/xen/arch/arm/kernel.h
> +++ b/xen/arch/arm/kernel.h
> @@ -40,8 +40,6 @@ struct kernel_info {
>       };
>
>       void (*load)(struct kernel_info *info);
> -    /* Callback to check overlap between the kernel and the device tree */
> -    void (*check_overlap)(struct kernel_info *kinfo);
>       int load_attr;
>   };
>
>

-- 
Julien Grall