Hi Michal,
> On 1 Apr 2025, at 18:42, Orzel, Michal <michal.or...@amd.com> wrote:
>
>
>
> On 01/04/2025 17:53, Bertrand Marquis wrote:
>>
>>
>> Hi Michal,
>>
>>> On 1 Apr 2025, at 17:21, Orzel, Michal <michal.or...@amd.com> wrote:
>>>
>>>
>>>
>>> On 01/04/2025 16:49, Bertrand Marquis wrote:
>>>>
>>>>
>>>> Hi,
>>>>
>>>>> On 1 Apr 2025, at 16:22, Orzel, Michal <michal.or...@amd.com> wrote:
>>>>>
>>>>>
>>>>>
>>>>> 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.
>>>>
>>>> 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.
Cheers
Bertrand
>
> ~Michal
