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

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

On 10/07/2013 05:39 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 seaprate phase and then fill in the memory nodes in the

separate

> DTB based on the result while generating the DTB.
> 
> This allows us to move kernel parsing before DTB setup.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>  xen/arch/arm/domain_build.c |   94 ++++++++++++++++++++++++++++++-------------
>  1 file changed, 65 insertions(+), 29 deletions(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index fb1fa56..1287934 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -63,11 +63,8 @@ struct vcpu *__init alloc_dom0_vcpu0(void)
>      return alloc_vcpu(dom0, 0, 0);
>  }
>  
> -static int set_memory_reg_11(struct domain *d, struct kernel_info *kinfo,
> -                             const struct dt_property *pp,
> -                             const struct dt_device_node *np, __be32 *new_cell)
> +static int allocate_memory_11(struct domain *d, struct kernel_info *kinfo)

This function always return 0 or panic if an error occurred. Perhaps you
can move the return type to void?

>  {
> -    int reg_size = dt_cells_to_size(dt_n_addr_cells(np) + dt_n_size_cells(np));
>      paddr_t start;
>      paddr_t size;
>      struct page_info *pg;
> @@ -91,40 +88,53 @@ static int set_memory_reg_11(struct domain *d, struct kernel_info *kinfo,
>      if ( res )
>          panic("Unable to add pages in DOM0: %d\n", res);
>  
> -    dt_set_range(&new_cell, np, start, size);
> -
>      kinfo->mem.bank[0].start = start;
>      kinfo->mem.bank[0].size = size;
>      kinfo->mem.nr_banks = 1;
>  
> -    return reg_size;
> +    kinfo->unassigned_mem -= size;
> +
> +    return 0;
>  }
>  
> -static int set_memory_reg(struct domain *d, struct kernel_info *kinfo,
> -                          const struct dt_property *pp,
> -                          const struct dt_device_node *np, __be32 *new_cell)
> +static int allocate_memory(struct domain *d, struct kernel_info *kinfo)

Same here.

>  {
> -    int reg_size = dt_cells_to_size(dt_n_addr_cells(np) + dt_n_size_cells(np));
> -    int l = 0;
> +
> +    const struct dt_device_node *memory;
> +    const void *reg;
> +    u32 reg_len, reg_size;
> +    int l = 0, ret;
>      unsigned int bank = 0;
> -    u64 start;
> -    u64 size;
> -    int ret;
>  
>      if ( platform_has_quirk(PLATFORM_QUIRK_DOM0_MAPPING_11) )
> -        return set_memory_reg_11(d, kinfo, pp, np, new_cell);
> +        return allocate_memory_11(d, kinfo);
> +
> +    memory = dt_find_node_by_type(NULL, "memory");

Can we try to have the same way to retrieve the memory node in each place?

- common/device_tree.c: looking by memory@unit
- arch/arm/domain_build.c:write_properties: looking only the exact node
name "memory"
- here: looking by type "memory"

Furthermore, your are assuming that there is only one memory node in DTS
tree. Perhaps a loop is better here?

-- 
Julien Grall