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?
~Michal
