Re: [PATCH 5/7] xen/arm: Rework heap page allocation outside allocate_bank_memory

[Michal Orzel <michal.orzel@xxxxxxx>]

Hi Luca,

On 23/04/2024 10:25, Luca Fancellu wrote:
> 
> 
> The function allocate_bank_memory allocates pages from the heap and
> map them to the guest using guest_physmap_add_page.
s/map/maps

> 
> As a preparation work to support static shared memory bank when the
> host physical address is not provided, Xen needs to allocate memory
> from the heap, so rework allocate_bank_memory moving out the page
> allocation in a new function called allocate_domheap_memory.
> 
> The function allocate_domheap_memory takes a callback function and
> a pointer to some extra information passed to the callback and this
> function will be called for every page allocated, until a defined
for every region given that you pass sgfn and order

> size is reached.
> 
> In order to keep allocate_bank_memory functionality, the callback
> passed to allocate_domheap_memory is a wrapper for
> guest_physmap_add_page.
> 
> Let allocate_domheap_memory be externally visible, in order to use
> it in the future from the static shared memory module.
> 
> Take the opportunity to change the signature of allocate_bank_memory
> and remove the 'struct domain' parameter, which can be retrieved from
> 'struct kernel_info'.
> 
> No functional changes is intended.
> 
> Signed-off-by: Luca Fancellu <luca.fancellu@xxxxxxx>
> ---
>  xen/arch/arm/dom0less-build.c           |  4 +-
>  xen/arch/arm/domain_build.c             | 77 +++++++++++++++++--------
>  xen/arch/arm/include/asm/domain_build.h |  9 ++-
>  3 files changed, 62 insertions(+), 28 deletions(-)
> 
> diff --git a/xen/arch/arm/dom0less-build.c b/xen/arch/arm/dom0less-build.c
> index 74f053c242f4..20ddf6f8f250 100644
> --- a/xen/arch/arm/dom0less-build.c
> +++ b/xen/arch/arm/dom0less-build.c
> @@ -60,12 +60,12 @@ static void __init allocate_memory(struct domain *d, 
> struct kernel_info *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),
> +    if ( !allocate_bank_memory(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),
> +    if ( !allocate_bank_memory(kinfo, gaddr_to_gfn(GUEST_RAM1_BASE),
>                                 bank_size) )
>          goto fail;
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 0784e4c5e315..148db06b8ca3 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -417,26 +417,13 @@ static void __init allocate_memory_11(struct domain *d,
>  }
> 
>  #ifdef CONFIG_DOM0LESS_BOOT
> -bool __init allocate_bank_memory(struct domain *d, struct kernel_info *kinfo,
> -                                 gfn_t sgfn, paddr_t tot_size)
> +bool __init allocate_domheap_memory(struct domain *d, paddr_t tot_size,
> +                                    alloc_domheap_mem_cb cb, void *extra)
>  {
> -    struct membanks *mem = kernel_info_get_mem(kinfo);
> -    int res;
> +    unsigned int max_order = UINT_MAX;
>      struct page_info *pg;
You can limit the visibility of these variables by moving them into loop

> -    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 = &mem->bank[mem->nr_banks];
> -    bank->start = gfn_to_gaddr(sgfn);
> -    bank->size = tot_size;
> +    BUG_ON(!cb);
No need for that

> 
>      while ( tot_size > 0 )
>      {
> @@ -463,17 +450,61 @@ bool __init allocate_bank_memory(struct domain *d, 
> struct kernel_info *kinfo,
>              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 ( cb(d, pg, order, extra) )
>              return false;
> -        }
> 
> -        sgfn = gfn_add(sgfn, 1UL << order);
>          tot_size -= (1ULL << (PAGE_SHIFT + order));
>      }
> 
> +    return true;
> +}
> +
> +static int __init guest_map_pages(struct domain *d, struct page_info *pg,
Does it make sense to return int if it is not taken into account by the user?

> +                                  unsigned int order, void *extra)
> +{
> +    gfn_t *sgfn = (gfn_t *)extra;
> +    int res;
> +
> +    BUG_ON(!sgfn);
> +    res = guest_physmap_add_page(d, *sgfn, page_to_mfn(pg), order);
> +    if ( res )
> +    {
> +        dprintk(XENLOG_ERR, "Failed map pages to DOMU: %d", res);
> +        return res;
> +    }
> +
> +    *sgfn = gfn_add(*sgfn, 1UL << order);
> +
> +    return 0;
> +}
> +
> +bool __init allocate_bank_memory(struct kernel_info *kinfo, gfn_t sgfn,
> +                                 paddr_t tot_size)
> +{
> +    struct membanks *mem = kernel_info_get_mem(kinfo);
> +    struct domain *d = kinfo->d;
> +    struct membank *bank;
> +
> +    /*
> +     * 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 = &mem->bank[mem->nr_banks];
> +    bank->start = gfn_to_gaddr(sgfn);
> +    bank->size = tot_size;
> +
> +    /*
> +     * Allocate pages from the heap until tot_size and map them to the guest
until tot_size is 0

> +     * using guest_map_pages, passing the starting gfn as extra parameter for
> +     * the map operation.
> +     */
> +    if ( !allocate_domheap_memory(d, tot_size, guest_map_pages, &sgfn) )
> +        return false;
> +
>      mem->nr_banks++;
>      kinfo->unassigned_mem -= bank->size;
> 
> diff --git a/xen/arch/arm/include/asm/domain_build.h 
> b/xen/arch/arm/include/asm/domain_build.h
> index 45936212ca21..9eeb5839f6ed 100644
> --- a/xen/arch/arm/include/asm/domain_build.h
> +++ b/xen/arch/arm/include/asm/domain_build.h
> @@ -5,9 +5,12 @@
>  #include <asm/kernel.h>
> 
>  typedef __be32 gic_interrupt_t[3];
> -
> -bool allocate_bank_memory(struct domain *d, struct kernel_info *kinfo,
> -                          gfn_t sgfn, paddr_t tot_size);
> +typedef int (*alloc_domheap_mem_cb)(struct domain *d, struct page_info *pg,
> +                                    unsigned int order, void *extra);
> +bool allocate_domheap_memory(struct domain *d, paddr_t tot_size,
> +                             alloc_domheap_mem_cb cb, void *extra);
> +bool allocate_bank_memory(struct kernel_info *kinfo, gfn_t sgfn,
> +                          paddr_t tot_size);
>  int construct_domain(struct domain *d, struct kernel_info *kinfo);
>  int domain_fdt_begin_node(void *fdt, const char *name, uint64_t unit);
>  int make_chosen_node(const struct kernel_info *kinfo);
> --
> 2.34.1
> 

~Michal