Hi Michal,
> On 1 Apr 2025, at 11:09, Michal Orzel <michal.or...@amd.com> wrote:
>
> There's no benefit in having process_shm_chosen() next to process_shm().
> The former is just a helper to pass "/chosen" node to the latter for
> hwdom case. Drop process_shm_chosen() and instead use process_shm()
> passing NULL as node parameter, which will result in searching for and
> using /chosen to find shm node (the DT full path search is done in
> process_shm() to avoid expensive lookup if !CONFIG_STATIC_SHM). This
> will simplify future handling of hw/control domain separation.
>
> Signed-off-by: Michal Orzel <michal.or...@amd.com>
> ---
> xen/arch/arm/domain_build.c | 2 +-
> xen/arch/arm/include/asm/static-shmem.h | 14 --------------
> xen/arch/arm/static-shmem.c | 4 ++++
> 3 files changed, 5 insertions(+), 15 deletions(-)
>
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 2b5b4331834f..7f9e17e1de4d 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -2325,7 +2325,7 @@ int __init construct_hwdom(struct kernel_info *kinfo)
> else
> allocate_memory(d, kinfo);
>
> - rc = process_shm_chosen(d, kinfo);
> + rc = process_shm(d, kinfo, NULL);
> if ( rc < 0 )
> return rc;
>
> diff --git a/xen/arch/arm/include/asm/static-shmem.h
> b/xen/arch/arm/include/asm/static-shmem.h
> index fd0867c4f26b..94eaa9d500f9 100644
> --- a/xen/arch/arm/include/asm/static-shmem.h
> +++ b/xen/arch/arm/include/asm/static-shmem.h
> @@ -18,14 +18,6 @@ int make_resv_memory_node(const struct kernel_info *kinfo,
> int addrcells,
> int process_shm(struct domain *d, struct kernel_info *kinfo,
> const struct dt_device_node *node);
>
> -static inline int process_shm_chosen(struct domain *d,
> - struct kernel_info *kinfo)
> -{
> - const struct dt_device_node *node = dt_find_node_by_path("/chosen");
> -
> - return process_shm(d, kinfo, node);
> -}
> -
> int process_shm_node(const void *fdt, int node, uint32_t address_cells,
> uint32_t size_cells);
>
> @@ -74,12 +66,6 @@ static inline int process_shm(struct domain *d, struct
> kernel_info *kinfo,
> return 0;
> }
>
> -static inline int process_shm_chosen(struct domain *d,
> - struct kernel_info *kinfo)
> -{
> - return 0;
> -}
> -
> static inline void init_sharedmem_pages(void) {};
>
> static inline int remove_shm_from_rangeset(const struct kernel_info *kinfo,
> diff --git a/xen/arch/arm/static-shmem.c b/xen/arch/arm/static-shmem.c
> index c74fa13d4847..cda90105923d 100644
> --- a/xen/arch/arm/static-shmem.c
> +++ b/xen/arch/arm/static-shmem.c
> @@ -297,6 +297,10 @@ int __init process_shm(struct domain *d, struct
> kernel_info *kinfo,
> {
> struct dt_device_node *shm_node;
>
> + /* Hwdom case - shm node under /chosen */
> + if ( !node )
> + node = dt_find_node_by_path("/chosen");
> +
I would have 2 questions here:
- what if a NULL pointer is passed, wouldn't you wrongly look in the main
device tree ?
- isn't there a NULL case to be handled if dt_find_node_by_path does not find a
result ?
Couldn't the condition also check for the domain to be the hwdom ?
Cheers
Bertrand
> dt_for_each_child_node(node, shm_node)
> {
> const struct membank *boot_shm_bank;
> --
> 2.25.1
>
