Re: [PATCH v5 11/11] xen/arm: create another /memory node for static shm

[Michal Orzel <michal.orzel@xxxxxxx>]

Hi Penny,

On 06/12/2023 10:06, Penny Zheng wrote:
> 
> 
> Static shared memory region shall be described both under /memory and
> /reserved-memory.
> 
> We introduce export_shm_memory_node() to create another /memory node to
> contain the static shared memory ranges.
> 
> Signed-off-by: Penny Zheng <penny.zheng@xxxxxxx>
> 
> ---
> v3 -> v4:
> new commit
> ---
> v4 -> v5:
> rebase and no changes
> ---
>  xen/arch/arm/dom0less-build.c           |  8 ++++++++
>  xen/arch/arm/domain_build.c             |  8 ++++++++
>  xen/arch/arm/include/asm/static-shmem.h | 10 ++++++++++
>  xen/arch/arm/static-shmem.c             | 19 +++++++++++++++++++
>  4 files changed, 45 insertions(+)
> 
> diff --git a/xen/arch/arm/dom0less-build.c b/xen/arch/arm/dom0less-build.c
> index ac096fa3fa..870b8a553f 100644
> --- a/xen/arch/arm/dom0less-build.c
> +++ b/xen/arch/arm/dom0less-build.c
> @@ -645,6 +645,14 @@ static int __init prepare_dtb_domU(struct domain *d, 
> struct kernel_info *kinfo)
>      if ( ret )
>          goto err;
> 
> +    /* Create a memory node to store the static shared memory regions */
> +    if ( kinfo->shminfo.nr_banks != 0 )
There is no need for this check to be repeated every time 
export_shm_memory_node is used.
When the feature is disabled, export_shm_memory_node will return 0 anyway.
Furthermore, there is no need for kinfo->shminfo exposure. Please move the 
check to the function itself.

Also, I think both this and previous patch could be moved towards the beginning 
of the series.
They are not related to other things you do in the series.


> +    {
> +        ret = export_shm_memory_node(d, kinfo, addrcells, sizecells);
> +        if ( ret )
> +            goto err;
> +    }
> +
>      ret = make_resv_memory_node(d, kinfo->fdt, addrcells, sizecells, kinfo);
>      if ( ret )
>          goto err;
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index f098678ea3..4e450cb4c7 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -1873,6 +1873,14 @@ static int __init handle_node(struct domain *d, struct 
> kernel_info *kinfo,
>                  return res;
>          }
> 
> +        /* Create a memory node to store the static shared memory regions */
> +        if ( kinfo->shminfo.nr_banks != 0 )
> +        {
> +            res = export_shm_memory_node(d, kinfo, addrcells, sizecells);
> +            if ( res )
> +                return res;
> +        }
> +
>          /* Avoid duplicate /reserved-memory nodes in Device Tree */
>          if ( !kinfo->resv_mem )
>          {
> diff --git a/xen/arch/arm/include/asm/static-shmem.h 
> b/xen/arch/arm/include/asm/static-shmem.h
> index 6cb4ef9646..385fd24c17 100644
> --- a/xen/arch/arm/include/asm/static-shmem.h
> +++ b/xen/arch/arm/include/asm/static-shmem.h
> @@ -38,6 +38,10 @@ int make_shm_memory_node(const struct domain *d,
>                           void *fdt,
>                           int addrcells, int sizecells,
>                           const struct kernel_info *kinfo);
> +
> +int export_shm_memory_node(const struct domain *d,
> +                           const struct kernel_info *kinfo,
> +                           int addrcells, int sizecells);
>  #else /* !CONFIG_STATIC_SHM */
> 
>  static inline int make_resv_memory_node(const struct domain *d, void *fdt,
> @@ -86,6 +90,12 @@ static inline int make_shm_memory_node(const struct domain 
> *d,
>      return 0;
>  }
> 
> +static inline int export_shm_memory_node(const struct domain *d,
> +                                         const struct kernel_info *kinfo,
> +                                         int addrcells, int sizecells)
> +{
> +    return 0;
> +}
>  #endif /* CONFIG_STATIC_SHM */
> 
>  #endif /* __ASM_STATIC_SHMEM_H_ */
> diff --git a/xen/arch/arm/static-shmem.c b/xen/arch/arm/static-shmem.c
> index bfce5bbad0..e583aae685 100644
> --- a/xen/arch/arm/static-shmem.c
> +++ b/xen/arch/arm/static-shmem.c
> @@ -505,6 +505,25 @@ int __init process_shm(struct domain *d, struct 
> kernel_info *kinfo,
>      return 0;
>  }
> 
> +int __init export_shm_memory_node(const struct domain *d,
> +                                  const struct kernel_info *kinfo,
> +                                  int addrcells, int sizecells)
> +{
> +    unsigned int i = 0;
> +    struct meminfo shm_meminfo;
> +
> +    /* Extract meminfo from kinfo.shminfo */
> +    for ( ; i < kinfo->shminfo.nr_banks; i++ )
> +    {
> +        shm_meminfo.bank[i].start = kinfo->shminfo.bank[i].membank.start;
> +        shm_meminfo.bank[i].size = kinfo->shminfo.bank[i].membank.size;
> +        shm_meminfo.bank[i].type = MEMBANK_DEFAULT;
Is all of this really needed? This series introduces so many structures to 
avoid using
meminfo but at the end we still need to use it. The amount of meminfo like 
structures copying
done in this series worries me a bit.

~Michal