Re: [PATCH 10/11] xen/arm: fix duplicate /reserved-memory node in Dom0

[Michal Orzel <michal.orzel@amd.com>]

Hi Luca,

On 12/03/2024 14:03, Luca Fancellu wrote:
> 
> 
> From: Penny Zheng <Penny.Zheng@arm.com>
> 
> In case there is a /reserved-memory node already present in the host dtb,
> current Xen codes would create yet another /reserved-memory node when
> the static shared memory feature is enabled and static shared memory
> region are present, this would result in an incorrect device tree
s/region/regions, please add full stop after present.

> generation and guest would not be able to detect the static shared memory
s/guest/hwdom to make it clear when the issue can be observed

> region.
> 
> Avoid this issue checking the presence of the /reserved-memory node and
by checking

> appending the nodes instead of generating a duplicate /reserved-memory.
> 
> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
> ---
> v1:
>  - Rework of https://patchwork.kernel.org/project/xen-devel/patch/20231206090623.1932275-11-Penny.Zheng@arm.com/
> ---
>  xen/arch/arm/domain_build.c             | 23 ++++++++++++++++++++---
>  xen/arch/arm/include/asm/static-shmem.h | 11 +++++++++++
>  xen/arch/arm/static-shmem.c             | 12 +++++++-----
>  3 files changed, 38 insertions(+), 8 deletions(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 740c483ea2db..575e906d81a6 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -1620,6 +1620,7 @@ static int __init handle_node(struct domain *d, struct kernel_info *kinfo,
>          DT_MATCH_PATH("/hypervisor"),
>          { /* sentinel */ },
>      };
> +    static __initdata bool res_mem_node_found = false;
>      struct dt_device_node *child;
>      int res, i, nirq, irq_id;
>      const char *name;
> @@ -1714,6 +1715,19 @@ static int __init handle_node(struct domain *d, struct kernel_info *kinfo,
>      if ( res )
>          return res;
> 
> +    if ( dt_node_path_is_equal(node, "/reserved-memory") )
> +    {
> +        res_mem_node_found = true;
> +        /*
> +         * Avoid duplicate /reserved-memory nodes in Device Tree, so list the
s/list/add

> +         * static shared memory nodes there.
> +         */
> +        res = make_shm_memory_node(d, kinfo, dt_n_addr_cells(node),
> +                                   dt_n_size_cells(node));
> +        if ( res )
> +            return res;
> +    }
> +
>      for ( child = node->child; child != NULL; child = child->sibling )
>      {
>          res = handle_node(d, kinfo, child, p2mt);
> @@ -1766,9 +1780,12 @@ static int __init handle_node(struct domain *d, struct kernel_info *kinfo,
>                  return res;
>          }
> 
> -        res = make_resv_memory_node(d, kinfo, addrcells, sizecells);
> -        if ( res )
> -            return res;
> +        if ( !res_mem_node_found )
> +        {
> +            res = make_resv_memory_node(d, kinfo, addrcells, sizecells);
> +            if ( res )
> +                return res;
> +        }
>      }
> 
>      res = fdt_end_node(kinfo->fdt);
> diff --git a/xen/arch/arm/include/asm/static-shmem.h b/xen/arch/arm/include/asm/static-shmem.h
> index 2f70aed53ac7..d28b9540d49b 100644
> --- a/xen/arch/arm/include/asm/static-shmem.h
> +++ b/xen/arch/arm/include/asm/static-shmem.h
> @@ -35,6 +35,10 @@ int remove_shm_from_rangeset(const struct kernel_info *kinfo,
>  int remove_shm_holes_for_domU(const struct kernel_info *kinfo,
>                                struct membanks *ext_regions);
> 
> +int make_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,
> @@ -79,6 +83,13 @@ static inline int remove_shm_holes_for_domU(const struct kernel_info *kinfo,
>      return 0;
>  }
> 
> +static inline int make_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 b3e2105dd3f2..67d5fa3b5d25 100644
> --- a/xen/arch/arm/static-shmem.c
> +++ b/xen/arch/arm/static-shmem.c
> @@ -287,15 +287,17 @@ int __init process_shm(struct domain *d, struct kernel_info *kinfo,
>      return 0;
>  }
> 
> -static int __init make_shm_memory_node(const struct domain *d, void *fdt,
> -                                       int addrcells, int sizecells,
> -                                       const struct membanks *mem)
> +int __init make_shm_memory_node(const struct domain *d,
> +                                const struct kernel_info *kinfo, int addrcells,
> +                                int sizecells)
Strictly speaking, changing the function signature is not mandatory for this change, so I would
at least mention it in the commit msg.

Other than that:
Reviewed-by: Michal Orzel <michal.orzel@amd.com>

~Michal