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

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

On 01/04/2025 17:53, Bertrand Marquis wrote:
> 
> 
> Hi Michal,
> 
>> On 1 Apr 2025, at 17:21, Orzel, Michal <michal.orzel@xxxxxxx> wrote:
>>
>>
>>
>> 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.
> 
> I am not quite sure why you do not want an assert here.
> Reading the code the first that comes to mind is "what if this is still NULL 
> after"
> which would be clearly something no expected if someone sees an assert.
> 
> Seeing the place where it is, that would not impact performance in any way.
> So why not adding it ?
> 
>>
>> /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().
>>
> 
> Sure you might be right, what if something bypass this due to efi boot or 
> acpi boot and the
> code comes down here ?
> 
> Even it might be true now, this would make sure that no change in the code is 
> changing this.
> 
> Anyway i will not argue on that for to long as it is kind of a matter of 
> taste.
> 
> So feel free to put my acked-by without the assert.
You gave me a reason to scan the code and I realized that in ACPI case, if
STATIC_SHM is enabled, it triggers a bug in process_shm_chosen. So, you were
right and we found a latent bug that is not related to this series. But maybe it
would want to be handled as separate fix before the process_shm_chosen drop?

~Michal