Re: [PATCH v1 01/13] xen/arm: re-arrange the static shared memory region

[Julien Grall <julien@xxxxxxx>]

Hi Penny,

On 15/11/2022 02:52, Penny Zheng wrote:
> This commit re-arranges the static shared memory regions into a separate array
> shm_meminfo. And static shared memory region now would have its own structure
> 'shm_membank' to hold all shm-related members, like shm_id, etc, and a pointer
> to the normal memory bank 'membank'. This will avoid continuing to grow
> 'membank'.
>
> Signed-off-by: Penny Zheng <penny.zheng@xxxxxxx>
> ---
>   xen/arch/arm/bootfdt.c            | 40 +++++++++++++++++++------------
>   xen/arch/arm/domain_build.c       | 35 ++++++++++++++++-----------
>   xen/arch/arm/include/asm/kernel.h |  2 +-
>   xen/arch/arm/include/asm/setup.h  | 16 +++++++++----
>   4 files changed, 59 insertions(+), 34 deletions(-)
>
> diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
> index 6014c0f852..ccf281cd37 100644
> --- a/xen/arch/arm/bootfdt.c
> +++ b/xen/arch/arm/bootfdt.c
> @@ -384,6 +384,7 @@ static int __init process_shm_node(const void *fdt, int 
> node,
>       const __be32 *cell;
>       paddr_t paddr, gaddr, size;
>       struct meminfo *mem = &bootinfo.reserved_mem;
After this patch, 'mem' is barely going to be used. So I would recommend 
to remove it or restrict the scope.
This will make easier to confirm that most of the use of 'mem' have been 
replaced with 'shm_mem' and reduce the risk of confusion between the two 
(the name are quite similar).
[...]

> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index bd30d3798c..c0fd13f6ed 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -757,20 +757,20 @@ static int __init acquire_nr_borrower_domain(struct 
> domain *d,
>   {
>       unsigned int bank;
>   
> -    /* Iterate reserved memory to find requested shm bank. */
> -    for ( bank = 0 ; bank < bootinfo.reserved_mem.nr_banks; bank++ )
> +    /* Iterate static shared memory to find requested shm bank. */
> +    for ( bank = 0 ; bank < bootinfo.shm_mem.nr_banks; bank++ )
>       {
> -        paddr_t bank_start = bootinfo.reserved_mem.bank[bank].start;
> -        paddr_t bank_size = bootinfo.reserved_mem.bank[bank].size;
> +        paddr_t bank_start = bootinfo.shm_mem.bank[bank].membank->start;
> +        paddr_t bank_size = bootinfo.shm_mem.bank[bank].membank->size;
I was expecting a "if (type == MEMBANK_STATIC_DOMAIN) ..."  to be 
removed. But it looks like there was none. I guess that was a mistake in 
the existing code?
>   
>           if ( (pbase == bank_start) && (psize == bank_size) )
>               break;
>       }
>   
> -    if ( bank == bootinfo.reserved_mem.nr_banks )
> +    if ( bank == bootinfo.shm_mem.nr_banks )
>           return -ENOENT;
>   
> -    *nr_borrowers = bootinfo.reserved_mem.bank[bank].nr_shm_borrowers;
> +    *nr_borrowers = bootinfo.shm_mem.bank[bank].nr_shm_borrowers;
>   
>       return 0;
>   }
> @@ -907,11 +907,18 @@ static int __init append_shm_bank_to_domain(struct 
> kernel_info *kinfo,
>                                               paddr_t start, paddr_t size,
>                                               const char *shm_id)
>   {
> +    struct membank *membank;
> +
>       if ( kinfo->shm_mem.nr_banks >= NR_MEM_BANKS )
>           return -ENOMEM;
>   
> -    kinfo->shm_mem.bank[kinfo->shm_mem.nr_banks].start = start;
> -    kinfo->shm_mem.bank[kinfo->shm_mem.nr_banks].size = size;
> +    membank = xmalloc_bytes(sizeof(struct membank));
You allocate memory but never free it. However, I think it would be 
better to avoid the dynamic allocation. So I would consider to not use 
the structure shm_membank and instead create a specific one for the 
domain construction.
> +    if ( membank == NULL )
> +        return -ENOMEM;
> +
> +    kinfo->shm_mem.bank[kinfo->shm_mem.nr_banks].membank = membank;
> +    kinfo->shm_mem.bank[kinfo->shm_mem.nr_banks].membank->start = start;
> +    kinfo->shm_mem.bank[kinfo->shm_mem.nr_banks].membank->size = size;
The last two could be replaced with:

membank->start = start;
membank->size = size;

This would make the code more readable. Also, while you are modifying 
the code, I would consider to introduce a local variable that points to
kinfo->shm_mem.bank[kinfo->shm_mem.nr_banks].

[...]

>   struct meminfo {
> @@ -61,6 +57,17 @@ struct meminfo {
>       struct membank bank[NR_MEM_BANKS];
>   };
>   
> +struct shm_membank {
> +    char shm_id[MAX_SHM_ID_LENGTH];
> +    unsigned int nr_shm_borrowers;
> +    struct membank *membank;
After the change I suggest above, I would expect that the field of 
membank will not be updated. So I would add const here.
Cheers,

--
Julien Grall