Re: [Xen-devel] [PATCH v6 12/26] xen/arm: introduce allocate_memory

[Julien Grall <julien.grall@xxxxxxx>]

Hi Stefano,

Most of the code is mine, so it is hard to review it :). Although, I have a few 
comments below.

On 02/11/2018 23:45, Stefano Stabellini wrote:
> Introduce an allocate_memory function able to allocate memory for DomUs
> and map it at the right guest addresses, according to the guest memory
> map: GUEST_RAM0_BASE and GUEST_RAM1_BASE.
>
> This is under #if 0 as not used for now.
>
> Signed-off-by: Julien Grall <julien.grall@xxxxxxx>
> Signed-off-by: Stefano Stabellini <stefanos@xxxxxxxxxx>
> ---
> Changes in v6:
> - turn dprintks into printk
> - use panic instead of printk+BUG_ON
> - use Julien's implementation of allocate_bank_memory
>
> Changes in v5:
> - improve commit message
> - code style
> - remove unneeded local var
> - while loop in allocate_bank_memory to allocate memory so that it
>    doesn't have to be contiguos
> - combile while loops in allocate_memory
>
> Changes in v4:
> - move earlier, add #if 0
> - introduce allocate_bank_memory, remove insert_bank
> - allocate_bank_memory allocate memory and inserts the bank, while
>    allocate_memory specifies where to do that
>
> Changes in v3:
> - new patch
> ---
>   xen/arch/arm/domain_build.c | 102 ++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 102 insertions(+)
>
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 66a258a..86abcc6 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -368,6 +368,108 @@ static void __init allocate_memory_11(struct domain *d,
>       }
>   }
>   
> +#if 0
> +static bool __init allocate_bank_memory(struct domain *d,
> +                                       struct kernel_info *kinfo,
> +                                       gfn_t sgfn,
> +                                       unsigned long tot_size)
> +{
> +    int res;
> +    struct page_info *pg;
> +    struct membank *bank;
> +    gfn_t gfn = sgfn;
> +    unsigned long size = tot_size;
The introduction for those 2 variables can be avoided if you pre-populate the 
bank before hand (see more below). I think this would avoid to confuse between 
the different variables.
> +    unsigned int max_order = ~0;
> +
> +    while ( size > 0 )
> +    {
> +        unsigned int order = get_allocation_size(size);
> +
> +        order = min(max_order, order);
> +
> +        pg = alloc_domheap_pages(d, order, 0);
> +        if ( !pg )
> +        {
> +            /*
> +             * If we can't allocate one page, then it is unlikely to
> +             * succeed in the next iteration. So bail out.
> +             */
> +            if ( !order )
> +                return false;
> +
> +            /*
> +             * If we can't allocate memory with order, then it is
> +             * unlikely to succeed in the next iteration.
> +             * Record the order - 1 to avoid re-trying.
> +             */
> +            max_order = order - 1;
> +            continue;
> +        }
> +
> +        res = guest_physmap_add_page(d, gfn, page_to_mfn(pg), order);
> +        if ( res )
> +        {
> +            dprintk(XENLOG_ERR, "Failed map pages to DOMU: %d", res);
> +            return false;
> +        }
> +
> +        gfn = gfn_add(gfn, 1UL << order);
> +        size -= (1ULL << (PAGE_SHIFT + order));
> +    }
> +
> +    bank = &kinfo->mem.bank[kinfo->mem.nr_banks];
> +    bank->start = gfn_to_gaddr(sgfn);
> +    bank->size = tot_size;
In relation with my comment on the variables, this is the 3 lines that could be 
moved earlier.
> +    kinfo->mem.nr_banks++;
> +    kinfo->unassigned_mem -= tot_size;
> +
> +    return true;
> +}
> +
> +static void __init allocate_memory(struct domain *d, struct kernel_info *kinfo)
> +{
> +    unsigned int i;
> +    unsigned long bank_size;
> +
> +    printk(XENLOG_INFO "Allocating mappings totalling %ldMB for dom%d:\n",
NIT: You can use the recently %pd to print the domain.

> +           /* Don't want format this as PRIpaddr (16 digit hex) */
> +           (unsigned long)(kinfo->unassigned_mem >> 20), d->domain_id);
> +
> +    kinfo->mem.nr_banks = 0;
> +    bank_size = MIN(GUEST_RAM0_SIZE, kinfo->unassigned_mem);
> +    if ( !allocate_bank_memory(d, kinfo, gaddr_to_gfn(GUEST_RAM0_BASE),
> +                               bank_size) )
> +        goto fail;
> +
> +    bank_size = MIN(GUEST_RAM1_SIZE, kinfo->unassigned_mem);
> +    if ( !allocate_bank_memory(d, kinfo, gaddr_to_gfn(GUEST_RAM1_BASE),
> +                               bank_size) )
> +        goto fail;
> +
> +    if ( kinfo->unassigned_mem )
> +        goto fail;
> +
> +    for( i = 0; i < kinfo->mem.nr_banks; i++ )
> +    {
> +        printk(XENLOG_INFO "Dom%d BANK[%d] %#"PRIpaddr"-%#"PRIpaddr" 
> (%ldMB)\n",
Same here.

> +               d->domain_id,
> +               i,
> +               kinfo->mem.bank[i].start,
> +               kinfo->mem.bank[i].start + kinfo->mem.bank[i].size,
> +               /* Don't want format this as PRIpaddr (16 digit hex) */
> +               (unsigned long)(kinfo->mem.bank[i].size >> 20));
> +    }
> +
> +    return;
> +
> +fail:
> +    panic("Failed to allocate requested domain memory."
> +          /* Don't want format this as PRIpaddr (16 digit hex) */
> +          " %ldKB unallocated. Fix the VMs configurations.\n",
> +          (unsigned long)kinfo->unassigned_mem >> 10);
> +}
> +#endif
> +
>   static int __init write_properties(struct domain *d, struct kernel_info 
> *kinfo,
>                                      const struct dt_device_node *node)
>   {
Cheers,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel