Re: [PATCH] xen/arm: Drop process_shm_chosen()

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi,

> On 1 Apr 2025, at 16:22, Orzel, Michal <Michal.Orzel@xxxxxxx> wrote:
> 
> 
> 
> On 01/04/2025 14:57, Bertrand Marquis wrote:
>> 
>> 
>> Hi Michal,
>> 
>>> On 1 Apr 2025, at 11:09, Michal Orzel <michal.orzel@xxxxxxx> 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.orzel@xxxxxxx>
>>> ---
>>> 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.

Then an ASSERT on NULL would be good.

> 
>> 
>> 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.

There are never to many asserts but ok :-)

With an ASSERT added for the NULL case you can add my R-b.

Cheers
Bertrand

> 
> ~Michal