Re: [PATCH v1 03/13] xen/arm: introduce allocate_domheap_memory and guest_physmap_memory

[Stewart Hildebrand <stewart.hildebrand@xxxxxxx>]

Hi Penny,

On 11/14/22 21:52, Penny Zheng wrote:
> We split the codes of allocate_bank_memory into two parts,
> allocate_domheap_memory and guest_physmap_memory.
> 
> One is about allocating guest RAM from heap, which could be re-used later for
> allocating static shared memory from heap when host address is not provided.
> 
> The other is building up guest P2M mapping.
> 
> Signed-off-by: Penny Zheng <penny.zheng@xxxxxxx>
> ---
>  xen/arch/arm/domain_build.c | 93 +++++++++++++++++++++++++++----------
>  1 file changed, 68 insertions(+), 25 deletions(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index d2b9e60b5c..92763e96fc 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -390,34 +390,18 @@ static void __init allocate_memory_11(struct domain *d,
>      }
>  }
> 
> -static bool __init allocate_bank_memory(struct domain *d,
> -                                        struct kernel_info *kinfo,
> -                                        gfn_t sgfn,
> -                                        paddr_t tot_size)
> +static bool __init allocate_domheap_memory(struct domain *d,
> +                                           paddr_t tot_size,
> +                                           struct meminfo *mem)
>  {
> -    int res;
>      struct page_info *pg;
> -    struct membank *bank;
>      unsigned int max_order = ~0;
> 
> -    /*
> -     * allocate_bank_memory can be called with a tot_size of zero for
> -     * the second memory bank. It is not an error and we can safely
> -     * avoid creating a zero-size memory bank.
> -     */
> -    if ( tot_size == 0 )
> -        return true;
> -
> -    bank = &kinfo->mem.bank[kinfo->mem.nr_banks];
> -    bank->start = gfn_to_gaddr(sgfn);
> -    bank->size = tot_size;
> -
>      while ( tot_size > 0 )
>      {
>          unsigned int order = get_allocation_size(tot_size);
> 
>          order = min(max_order, order);
> -
>          pg = alloc_domheap_pages(d, order, 0);
>          if ( !pg )
>          {
> @@ -437,15 +421,74 @@ static bool __init allocate_bank_memory(struct domain 
> *d,
>              continue;
>          }
> 
> -        res = guest_physmap_add_page(d, sgfn, page_to_mfn(pg), order);
> -        if ( res )
> -        {
> -            dprintk(XENLOG_ERR, "Failed map pages to DOMU: %d", res);
> +        if ( mem->nr_banks == NR_MEM_BANKS )
>              return false;
> -        }
> +
> +        mem->bank[mem->nr_banks].start = mfn_to_maddr(page_to_mfn(pg));
> +        mem->bank[mem->nr_banks].size = 1UL << (PAGE_SHIFT + order);
> +        mem->nr_banks++;
> +        tot_size -= (1UL << (PAGE_SHIFT + order));

Why the change from 1ULL to 1UL?

> +    }
> +
> +    return true;
> +}
> +
> +static int __init guest_physmap_memory(struct domain *d,
> +                                       const struct meminfo *mem, gfn_t sgfn)
> +{
> +    unsigned int i;
> +    int res;
> +
> +    for ( i = 0; i < mem->nr_banks; i++ )
> +    {
> +        paddr_t size = mem->bank[i].size;
> +        unsigned int order = get_order_from_bytes(size);
> +
> +        /* Size must be power of two */
> +        BUG_ON(!size || (size & (size - 1)));
> +        res = guest_physmap_add_page(d, sgfn, 
> maddr_to_mfn(mem->bank[i].start),
> +                                     order);
> +        if ( res )
> +            return res;
> 
>          sgfn = gfn_add(sgfn, 1UL << order);
> -        tot_size -= (1ULL << (PAGE_SHIFT + order));
> +    }
> +
> +    return 0;
> +}
> +
> +static bool __init allocate_bank_memory(struct domain *d,
> +                                        struct kernel_info *kinfo,
> +                                        gfn_t sgfn,
> +                                        paddr_t total_size)
> +{
> +    struct membank *bank;
> +    struct meminfo host = {0};

This function uses 6k of stack, largely due to the sizeof struct meminfo.

arch/arm/domain_build.c: In function ‘allocate_bank_memory’:
arch/arm/domain_build.c:461:20: warning: stack usage is 6224 bytes 
[-Wstack-usage=]
  461 | static bool __init allocate_bank_memory(struct domain *d,
      |                    ^~~~~~~~~~~~~~~~~~~~

It may be fine for now, but I wanted to at least check if anyone else had an 
opinion on allocating "struct meminfo host" by some other means, such as 
xzalloc/free or by making it static:

static struct meminfo __initdata host;
memset(&host, 0, sizeof(struct meminfo));

Particularly if we ever plan on increasing NR_MEM_BANKS again in the future.

> +
> +    /*
> +     * allocate_bank_memory can be called with a total_size of zero for
> +     * the second memory bank. It is not an error and we can safely
> +     * avoid creating a zero-size memory bank.
> +     */
> +    if ( total_size == 0 )
> +        return true;
> +
> +    bank = &kinfo->mem.bank[kinfo->mem.nr_banks];
> +    bank->start = gfn_to_gaddr(sgfn);
> +    bank->size = total_size;
> +
> +    if ( !allocate_domheap_memory(d, total_size, &host) )
> +    {
> +        printk(XENLOG_ERR "Failed to allocate (%"PRIpaddr"MB) pages to 
> %pd\n",
> +               total_size >> 20, d);
> +        return false;
> +    }
> +
> +    if ( guest_physmap_memory(d, &host, sgfn) )
> +    {
> +        printk(XENLOG_ERR "Failed to map (%"PRIpaddr"MB) pages to %pd\n",
> +               total_size >> 20, d);
> +        return false;
>      }
> 
>      kinfo->mem.nr_banks++;
> --
> 2.25.1
> 
>