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(-)
>
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 8645aa1..edfcf14 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -81,11 +81,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 void allocate_memory_11(struct domain *d, struct kernel_info *kinfo)
>   {
> -    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 = NULL;
> @@ -116,53 +113,61 @@ 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;
>   }
>
> -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 void allocate_memory(struct domain *d, struct kernel_info *kinfo)
>   {
> -    int reg_size = dt_cells_to_size(dt_n_addr_cells(np) + dt_n_size_cells(np));
> -    int l = 0;
> +
> +    struct dt_device_node *memory = NULL;
> +    const void *reg;
> +    u32 reg_len, reg_size;
>       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);
>
> -    while ( kinfo->unassigned_mem > 0 && l + reg_size <= pp->length
> -            && kinfo->mem.nr_banks < NR_MEM_BANKS )
> +    while ( (memory = dt_find_node_by_type(memory, "memory")) )
>       {
> -        ret = dt_device_get_address(np, bank, &start, &size);
> -        if ( ret )
> -            panic("Unable to retrieve the bank %u for %s\n",
> -                  bank, dt_node_full_name(np));
> -
> -        if ( size > kinfo->unassigned_mem )
> -            size = kinfo->unassigned_mem;
> -        dt_set_range(&new_cell, np, start, size);
> -
> -        printk("Populate P2M %#"PRIx64"->%#"PRIx64"\n", start, start + size);
> -        if ( p2m_populate_ram(d, start, start + size) < 0 )
> -            panic("Failed to populate P2M\n");
> -        kinfo->mem.bank[kinfo->mem.nr_banks].start = start;
> -        kinfo->mem.bank[kinfo->mem.nr_banks].size = size;
> -        kinfo->mem.nr_banks++;
> -        kinfo->unassigned_mem -= size;
> -
> -        l += reg_size;
> -    }
> +        int l;
> +
> +        DPRINT("memory node\n");
> +
> +        reg_size = dt_cells_to_size(dt_n_addr_cells(memory) + dt_n_size_cells(memory));
> +
> +        reg = dt_get_property(memory, "reg", &reg_len);
> +        if ( reg == NULL )
> +            panic("Memory node has no reg property!\n");
> +
> +        for ( l = 0;
> +              kinfo->unassigned_mem > 0 && l + reg_size <= reg_len
> +                  && kinfo->mem.nr_banks < NR_MEM_BANKS;
> +              l += reg_size )
> +        {
> +            paddr_t start, size;
>
> -    return l;
> +            if ( dt_device_get_address(memory, bank, &start, &size) )
> +                panic("Unable to retrieve the bank %u for %s\n",
> +                      bank, dt_node_full_name(memory));
> +
> +            if ( size > kinfo->unassigned_mem )
> +                size = kinfo->unassigned_mem;
> +
> +            printk("Populate P2M %#"PRIx64"->%#"PRIx64"\n",
> +                   start, start + size);
> +            if ( p2m_populate_ram(d, start, start + size) < 0 )
> +                panic("Failed to populate P2M\n");
> +            kinfo->mem.bank[kinfo->mem.nr_banks].start = start;
> +            kinfo->mem.bank[kinfo->mem.nr_banks].size = size;
> +            kinfo->mem.nr_banks++;
> +
> +            kinfo->unassigned_mem -= size;
> +        }
> +    }
>   }
>
>   static int write_properties(struct domain *d, struct kernel_info *kinfo,
> @@ -211,23 +216,6 @@ static int write_properties(struct domain *d, struct kernel_info *kinfo,
>                   continue;
>               }
>           }
> -        /*
> -         * In a memory node: adjust reg property.
> -         * TODO: handle properly memory node (ie: device_type = "memory")
> -         */
> -        else if ( dt_node_name_is_equal(np, "memory") )
> -        {
> -            if ( dt_property_name_is_equal(pp, "reg") )
> -            {
> -                new_data = xzalloc_bytes(pp->length);
> -                if ( new_data  == NULL )
> -                    return -FDT_ERR_XEN(ENOMEM);
> -
> -                prop_len = set_memory_reg(d, kinfo, pp, np,
> -                                          (__be32 *)new_data);
> -                prop_data = new_data;
> -            }
> -        }
>
>           res = fdt_property(kinfo->fdt, pp->name, prop_data, prop_len);
>
> @@ -304,6 +292,46 @@ static int fdt_property_interrupts(void *fdt, gic_interrupt_t *intr,
>       return res;
>   }
>
> +static int make_memory_node(const struct domain *d,
> +                            void *fdt,
> +                            const struct kernel_info *kinfo)
> +{
> +    int res, i;
> +    int nr_cells = XEN_FDT_NODE_REG_SIZE*kinfo->mem.nr_banks;

What about xzalloc? I don't think it's safe to allocate an uncontrol 
size (we don't know the size of nr_banks, even if now it's hardcoded).

-- 
Julien Grall