On 01/04/2025 14:57, Bertrand Marquis wrote:
>
>
> 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 ?
Do you mean from hwdom or domU path? In the former it is expected. In the latter
it would be a bug given that there are several dozens of things that operate on
this node being a /chosen/domU@X node before we pass node to process_shm().
> - isn't there a NULL case to be handled if dt_find_node_by_path does not find
> a result ?
It wasn't validated before this change. It would be catched in early boot code
so no worries.
>
> Couldn't the condition also check for the domain to be the hwdom ?
I could although we have so many /chosen and hwdom asserts in the tree in the
dom0 creation that I find it not necessary.
~Michal
