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

["Orzel, Michal" <michal.orzel@xxxxxxx>]

On 01/04/2025 16:49, Bertrand Marquis wrote:
> 
> 
> 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.
See below.

> 
>>
>>>
>>> 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.
:(
So you still want to put ASSERT for a case where host DT does not have /chosen
node. I'd like to talk you to drop this idea. Normally I'm in favor of using
ASSERTs but not for so obvious violations like missing /chosen.

/chosen node is so crucial for Xen on Arm functioning that a lot of things would
simply fail a lot  earlier than a point where we call process_shm() at the end
(almost) of hwdom creation. There would be no modules, so the first example that
comes to my head is panic due to no kernel which happens way before 
process_shm().

~Michal