Re: [PATCH v5 04/11] xen/arm: introduce allocate_domheap_memory and guest_physmap_memory

[Michal Orzel <michal.orzel@xxxxxxx>]

Hi Penny,

On 06/12/2023 10:06, Penny Zheng wrote:
> 
> 
> We split the code 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.
> 
> We also define a set of MACRO helpers to access common fields in data
> structure of "meminfo" type, e.g. "struct meminfo" is one of them, and
> later new "struct shm_meminfo" is also one of them.
> This kind of structures must have the following characteristics:
> - an array of "struct membank"
> - a member called "nr_banks" indicating current array size
> - a field indicating the maximum array size
> When introducing a new data structure, according callbacks with function type
> "retrieve_fn" shall be defined for using MACRO helpers.
> This commit defines callback "retrieve_meminfo" for data structure
> "struct meminfo".
This patch introduces quite a bit of complexity with all these helpers, so 
adding a rationale is crucial.
AFAIU, all of this is because we don't want to reuse struct meminfo where 
NR_MEM_BANKS is defined as 256,
which is a lot more than we need for shmem. Am I right?

I would like others to share the opinion here as well.

If we decide to go with this approach, what about static inline helpers instead 
of macros here for type checking and overall
protection such as when there is no retriever function defined (at the moment 
it would most probably result
in some trap).

> 
> Signed-off-by: Penny Zheng <penny.zheng@xxxxxxx>
> ---
> v1 -> v2:
> -  define a set of MACRO helpers to access common fields in data structure of
> "meminfo" type. "struct meminfo" is one of them, and according callback
> "retrieve_meminfo" is also introduced here.
> - typo of changing 1ULL to 1UL
> ---
> v2 -> v3
> - rebase and no changes
> ---
> v3 -> v4:
> rebase and no change
> ---
> v4 -> v5:
> rebase and no change
> ---
>  xen/arch/arm/domain_build.c      | 119 +++++++++++++++++++++++++------
>  xen/arch/arm/include/asm/setup.h |  33 +++++++++
>  2 files changed, 129 insertions(+), 23 deletions(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 64ae944431..a8bc78baa5 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -51,6 +51,28 @@ boolean_param("ext_regions", opt_ext_regions);
>  static u64 __initdata dom0_mem;
>  static bool __initdata dom0_mem_set;
> 
> +#ifdef CONFIG_DOM0LESS_BOOT
> +static void __init retrieve_meminfo(void *mem, unsigned int *max_mem_banks,
const void *mem

> +                                    struct membank **bank,
> +                                    unsigned int **nr_banks)
> +{
> +    struct meminfo *meminfo = (struct meminfo *)mem;
> +
> +    if ( max_mem_banks )
> +        *max_mem_banks = NR_MEM_BANKS;
> +
> +    if ( nr_banks )
> +        *nr_banks = &(meminfo->nr_banks);
> +
> +    if ( bank )
> +        *bank = meminfo->bank;
> +}
> +
> +retrieve_fn __initdata retrievers[MAX_MEMINFO_TYPE] = {
meminfo_retrievers

> +    [NORMAL_MEMINFO] = retrieve_meminfo,
> +};
> +#endif
> +
>  static int __init parse_dom0_mem(const char *s)
>  {
>      dom0_mem_set = true;
> @@ -415,32 +437,20 @@ 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)
> +static bool __init allocate_domheap_memory(struct domain *d,
> +                                           paddr_t tot_size,
> +                                           void *mem, enum meminfo_type type)
>  {
> -    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;
> +    unsigned int *nr_banks = GET_NR_BANKS(mem, type);
what if mem is NULL? You would end up dereferencing NULL pointer

> 
>      while ( tot_size > 0 )
>      {
>          unsigned int order = get_allocation_size(tot_size);
> +        struct membank *membank;
> 
>          order = min(max_order, order);
> -
>          pg = alloc_domheap_pages(d, order, 0);
>          if ( !pg )
>          {
> @@ -460,15 +470,78 @@ 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 ( *nr_banks == MAX_MEM_BANKS(type) )
shouldn't you move this check at the beginning of the loop before the 
allocation?
what if you enter this function with this condition being true.

>              return false;
> -        }
> +
The name of the function does not reflect that apart from allocation you will 
also
register the allocated memory regions.

> +        membank = GET_MEMBANK(mem, type, *nr_banks);
> +        membank->start = mfn_to_maddr(page_to_mfn(pg));
page_to_maddr

> +        membank->size = 1ULL << (PAGE_SHIFT + order);
> +        (*nr_banks)++;
> +        tot_size -= membank->size;
> +    }
> +
> +    return true;
> +}
> +
> +static int __init guest_physmap_memory(struct domain *d,
> +                                       void *mem, enum meminfo_type type,
> +                                       gfn_t sgfn)
> +{
> +    unsigned int i;
> +    int res;
> +    unsigned int *nr_banks = GET_NR_BANKS(mem, type);
what if mem is NULL? You would end up dereferencing NULL pointer

> +
> +    for ( i = 0; i < *nr_banks; i++ )
> +    {
> +        struct membank *membank = GET_MEMBANK(mem, type, i);
> +        paddr_t start = membank->start;
> +        paddr_t size = membank->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(start), order);
> +        if ( res )
> +            return res;
Here, you return a rc, but ...

> 
>          sgfn = gfn_add(sgfn, 1UL << order);
> -        tot_size -= (1ULL << (PAGE_SHIFT + order));
> +    }
> +
> +    return 0;
> +}
> +
> +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 };
> +
> +    /*
> +     * 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, (void *)&host, 
> NORMAL_MEMINFO) )
> +    {
> +        printk(XENLOG_ERR "Failed to allocate (%"PRIpaddr"MB) pages to 
> %pd\n",
> +               total_size >> 20, d);
> +        return false;
> +    }
> +
> +    if ( guest_physmap_memory(d, (void *)&host, NORMAL_MEMINFO, sgfn) )
... here you are not making use of it (to print the error code).

> +    {
> +        printk(XENLOG_ERR "Failed to map (%"PRIpaddr"MB) pages to %pd\n",
> +               total_size >> 20, d);
> +        return false;
>      }
> 
>      kinfo->mem.nr_banks++;
> diff --git a/xen/arch/arm/include/asm/setup.h 
> b/xen/arch/arm/include/asm/setup.h
> index 3a2b35ea46..bc5f08be97 100644
> --- a/xen/arch/arm/include/asm/setup.h
> +++ b/xen/arch/arm/include/asm/setup.h
> @@ -57,6 +57,39 @@ struct meminfo {
>      struct membank bank[NR_MEM_BANKS];
>  };
> 
A comment with a meaning of each value would be handy
> +enum meminfo_type {
> +    NORMAL_MEMINFO,
> +    MAX_MEMINFO_TYPE,
do you have a need for checking the max? If so, I would try to name the values 
in a similar, more informative way,
e.g. MEMINFO_TYPE_NORMAL

> +};
> +
> +/*
> + * Define a set of MACRO helpers to access meminfo_type, like "struct 
> meminfo"
> + * as type of NORMAL_MEMINFO, etc.
> + * This kind of structure must have a array of "struct membank",
> + * a member called nr_banks indicating the current array size, and also a 
> field
> + * indicating the maximum array size.
field? Looking at struct meminfo and then shm_meminfo you just have a macro to 
indicate max nr.

> + */
> +typedef void (*retrieve_fn)(void *, unsigned int *, struct membank **,
I think MISRA requires that parameters should be named.
void * can be const since there is no helper modifying the passed structure 
(here and in macros when casting)

> +                            unsigned int **);
> +
> +#define MAX_MEM_BANKS(type) ({                              \
> +    unsigned int _max_mem_banks;                            \
> +    retrievers[type](NULL, &_max_mem_banks, NULL, NULL);    \
> +    _max_mem_banks;                                         \
> +})
> +
> +#define GET_MEMBANK(mem, type, index) ({                    \
> +    struct membank *_bank;                                  \
> +    retrievers[type]((void *)(mem), NULL, &_bank, NULL);    \
> +    &(_bank[index]);                                        \
> +})
> +
> +#define GET_NR_BANKS(mem, type) ({                          \
> +    unsigned int *_nr_banks;                                \
> +    retrievers[type]((void *)mem, NULL, NULL, &_nr_banks);  \
> +    _nr_banks;                                              \
> +})
> +
>  /*
>   * The domU flag is set for kernels and ramdisks of "xen,domain" nodes.
>   * The purpose of the domU flag is to avoid getting confused in
> --
> 2.25.1
> 

~Michal