Re: [PATCH v3 12/12] xen/arm: List static shared memory regions as /memory nodes

[Michal Orzel <michal.orzel@xxxxxxx>]

Hi Luca,

On 18/04/2024 09:36, Luca Fancellu wrote:
> 
> 
> Currently Xen is not exporting the static shared memory regions
> to the device tree as /memory node, this commit is fixing this
> issue.
> 
> Given that now make_memory_node needs a parameter 'struct kernel_info'
> in order to call the new function shm_mem_node_fill_reg_range,
> take the occasion to remove the unused struct domain parameter.
> 
> Signed-off-by: Luca Fancellu <luca.fancellu@xxxxxxx>
> ---
> v3:
>  - removed previous patch that was merging the intervals, rebase
>    changes.
> v2:
>  - try to use make_memory_node, don't add overlapping ranges of
>    memory, commit message changed.
> v1:
>  - new patch
> ---
> ---
>  xen/arch/arm/dom0less-build.c           |  2 +-
>  xen/arch/arm/domain_build.c             | 34 +++++++++++++++++--------
>  xen/arch/arm/include/asm/domain_build.h |  2 +-
>  xen/arch/arm/include/asm/static-shmem.h | 15 +++++++++++
>  xen/arch/arm/static-shmem.c             | 23 +++++++++++++++++
>  5 files changed, 63 insertions(+), 13 deletions(-)
> 
> diff --git a/xen/arch/arm/dom0less-build.c b/xen/arch/arm/dom0less-build.c
> index 51cf03221d56..74f053c242f4 100644
> --- a/xen/arch/arm/dom0less-build.c
> +++ b/xen/arch/arm/dom0less-build.c
> @@ -642,7 +642,7 @@ static int __init prepare_dtb_domU(struct domain *d, 
> struct kernel_info *kinfo)
>      if ( ret )
>          goto err;
> 
> -    ret = make_memory_node(d, kinfo->fdt, addrcells, sizecells,
> +    ret = make_memory_node(kinfo, addrcells, sizecells,
>                             kernel_info_get_mem(kinfo));
>      if ( ret )
>          goto err;
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 68532ddc084c..15f888169c4e 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -756,15 +756,14 @@ int __init domain_fdt_begin_node(void *fdt, const char 
> *name, uint64_t unit)
>      return fdt_begin_node(fdt, buf);
>  }
> 
> -int __init make_memory_node(const struct domain *d,
> -                            void *fdt,
> -                            int addrcells, int sizecells,
> -                            const struct membanks *mem)
> +int __init make_memory_node(const struct kernel_info *kinfo, int addrcells,
> +                            int sizecells, const struct membanks *mem)
>  {
> +    void *fdt = kinfo->fdt;
>      unsigned int i;
>      int res, reg_size = addrcells + sizecells;
>      int nr_cells = 0;
> -    __be32 reg[NR_MEM_BANKS * 4 /* Worst case addrcells + sizecells */];
> +    __be32 reg[DT_MEM_NODE_REG_RANGE_SIZE];
>      __be32 *cells;
> 
>      if ( mem->nr_banks == 0 )
> @@ -797,14 +796,28 @@ int __init make_memory_node(const struct domain *d,
>          if ( mem->bank[i].type == MEMBANK_STATIC_DOMAIN )
>              continue;
> 
> -        dt_dprintk("  Bank %d: %#"PRIx64"->%#"PRIx64"\n",
> -                   i, start, start + size);
> -
>          nr_cells += reg_size;
>          BUG_ON(nr_cells >= ARRAY_SIZE(reg));
>          dt_child_set_range(&cells, addrcells, sizecells, start, size);
>      }
> 
> +    /*
> +     * static shared memory banks need to be listed as /memory node, so when
> +     * this function is handling the normal memory, add the banks.
> +     */
> +    if ( mem == kernel_info_get_mem(kinfo) )
> +        shm_mem_node_fill_reg_range(kinfo, reg, &nr_cells, addrcells,
> +                                    sizecells);
> +
> +    for ( cells = reg, i = 0; cells < reg + nr_cells; i++, cells += reg_size 
> )
> +    {
> +        u64 start = dt_read_number(cells, addrcells);
We should no longer use Linux derived types like u64. Use uint64_t.

> +        u64 size = dt_read_number(cells + addrcells, sizecells);
> +
> +        dt_dprintk("  Bank %d: %#"PRIx64"->%#"PRIx64"\n",
> +                   i, start, start + size);
i is unsigned so the correct format specifier should be %u

> +    }
> +
>      dt_dprintk("(reg size %d, nr cells %d)\n", reg_size, nr_cells);
> 
>      res = fdt_property(fdt, "reg", reg, nr_cells * sizeof(*reg));
> @@ -1783,7 +1796,7 @@ static int __init handle_node(struct domain *d, struct 
> kernel_info *kinfo,
>          if ( res )
>              return res;
> 
> -        res = make_memory_node(d, kinfo->fdt, addrcells, sizecells,
> +        res = make_memory_node(kinfo, addrcells, sizecells,
>                                 kernel_info_get_mem(kinfo));
>          if ( res )
>              return res;
> @@ -1794,8 +1807,7 @@ static int __init handle_node(struct domain *d, struct 
> kernel_info *kinfo,
>           */
>          if ( reserved_mem->nr_banks > 0 )
>          {
> -            res = make_memory_node(d, kinfo->fdt, addrcells, sizecells,
> -                                   reserved_mem);
> +            res = make_memory_node(kinfo, addrcells, sizecells, 
> reserved_mem);
>              if ( res )
>                  return res;
>          }
> diff --git a/xen/arch/arm/include/asm/domain_build.h 
> b/xen/arch/arm/include/asm/domain_build.h
> index 026d975da28e..45936212ca21 100644
> --- a/xen/arch/arm/include/asm/domain_build.h
> +++ b/xen/arch/arm/include/asm/domain_build.h
> @@ -14,7 +14,7 @@ int make_chosen_node(const struct kernel_info *kinfo);
>  int make_cpus_node(const struct domain *d, void *fdt);
>  int make_hypervisor_node(struct domain *d, const struct kernel_info *kinfo,
>                           int addrcells, int sizecells);
> -int make_memory_node(const struct domain *d, void *fdt, int addrcells,
> +int make_memory_node(const struct kernel_info *kinfo, int addrcells,
>                       int sizecells, const struct membanks *mem);
>  int make_psci_node(void *fdt);
>  int make_timer_node(const struct kernel_info *kinfo);
> diff --git a/xen/arch/arm/include/asm/static-shmem.h 
> b/xen/arch/arm/include/asm/static-shmem.h
> index 7495a91e7a31..3b6569e5703f 100644
> --- a/xen/arch/arm/include/asm/static-shmem.h
> +++ b/xen/arch/arm/include/asm/static-shmem.h
> @@ -3,10 +3,15 @@
>  #ifndef __ASM_STATIC_SHMEM_H_
>  #define __ASM_STATIC_SHMEM_H_
> 
> +#include <xen/types.h>
>  #include <asm/kernel.h>
> +#include <asm/setup.h>
> 
>  #ifdef CONFIG_STATIC_SHM
> 
> +/* Worst case /memory node reg element: (addrcells + sizecells) */
> +#define DT_MEM_NODE_REG_RANGE_SIZE ((NR_MEM_BANKS + NR_SHMEM_BANKS) * 4)
> +
>  int make_resv_memory_node(const struct kernel_info *kinfo, int addrcells,
>                            int sizecells);
> 
> @@ -37,8 +42,14 @@ int remove_shm_holes_for_domU(const struct kernel_info 
> *kinfo,
>  int make_shm_resv_memory_node(const struct kernel_info *kinfo, int addrcells,
>                                int sizecells);
> 
> +void shm_mem_node_fill_reg_range(const struct kernel_info *kinfo, __be32 
> *reg,
> +                                 int *nr_cells, int addrcells, int 
> sizecells);
> +
>  #else /* !CONFIG_STATIC_SHM */
> 
> +/* Worst case /memory node reg element: (addrcells + sizecells) */
> +#define DT_MEM_NODE_REG_RANGE_SIZE (NR_MEM_BANKS * 4)
> +
>  static inline int make_resv_memory_node(const struct kernel_info *kinfo,
>                                          int addrcells, int sizecells)
>  {
> @@ -86,6 +97,10 @@ static inline int make_shm_resv_memory_node(const struct 
> kernel_info *kinfo,
>      return 0;
>  }
> 
> +static inline void shm_mem_node_fill_reg_range(const struct kernel_info 
> *kinfo,
> +                                               __be32 *reg, int *nr_cells,
> +                                               int addrcells, int sizecells) 
> {};
> +
>  #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 c85f60dd1bf7..07c93a820508 100644
> --- a/xen/arch/arm/static-shmem.c
> +++ b/xen/arch/arm/static-shmem.c
> @@ -1,5 +1,6 @@
>  /* SPDX-License-Identifier: GPL-2.0-only */
> 
> +#include <xen/device_tree.h>
>  #include <xen/libfdt/libfdt.h>
>  #include <xen/rangeset.h>
>  #include <xen/sched.h>
> @@ -668,6 +669,28 @@ int __init remove_shm_holes_for_domU(const struct 
> kernel_info *kinfo,
>      return res;
>  }
> 
> +void __init shm_mem_node_fill_reg_range(const struct kernel_info *kinfo,
> +                                        __be32 *reg, int *nr_cells,
> +                                        int addrcells, int sizecells)
> +{
> +    const struct membanks *mem = &kinfo->shm_mem.common;
> +    unsigned int i;
> +    __be32 *cells;
> +
> +    BUG_ON(!nr_cells || !reg);
> +
> +    cells = &reg[*nr_cells];
> +    for ( i = 0; i < mem->nr_banks; i++ )
> +    {
> +        u64 start = mem->bank[i].start;
ditto

Rest LGTM:
Reviewed-by: Michal Orzel <michal.orzel@xxxxxxx>

~Michal