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

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

On 02/04/2025 08:18, Bertrand Marquis wrote:
> 
> 
> Hi Michal,
> 
>> On 1 Apr 2025, at 18:42, Orzel, Michal <Michal.Orzel@xxxxxxx> wrote:
>>
>>
>>
>> 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?
> 
> Nice at least this was useful, and it also means that there are never to much 
> asserts :-)
> 
> I would suggest to resubmit this patch on top of an other one fixing the 
> latent issue to
> make sure everything is merged in the right order but it is up to you as you 
> will probably
> be the one commiting both patches anyway.
Actually, I was a little bit imprecise. We don't need any ASSERT and my above
reasoning holds true. The problem I found is that when booting with ACPI we
don't protect call to process_shm_chosen with acpi_enabled like we do for cases
relying on DT. With ACPI enabled, we don't unflatten the host dt and host_dt is
always NULL, so it does not matter whether the host DT has or does not have
/chosen node. So we need the following fix I plan to submit separately later on
today:

-    rc = process_shm_chosen(d, kinfo);
-    if ( rc < 0 )
-        return rc;
+    if ( acpi_disabled )
+    {
+        rc = process_shm_chosen(d, kinfo);
+        if ( rc < 0 )
+            return rc;
+    }

~Michal