| Message ID | 20250401090937.105187-1-michal.orzel@amd.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/arm: Drop process_shm_chosen() | expand |
Hi Michal, > On 1 Apr 2025, at 10:09, Michal Orzel <michal.orzel@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.orzel@amd.com> > --- Looks good to me: Reviewed-by: Luca Fancellu <luca.fancellu@arm.com> I’ve also tested both configuration CONFIG_STATIC_SHM and !CONFIG_STATIC_SHM, with Dom0 and with only DomU (Dom0less started): Tested-by: Luca Fancellu <luca.fancellu@arm.com> Cheers, Luca
Hi Michal, > On 1 Apr 2025, at 11:09, Michal Orzel <michal.orzel@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.orzel@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 ? - isn't there a NULL case to be handled if dt_find_node_by_path does not find a result ? Couldn't the condition also check for the domain to be the hwdom ? Cheers Bertrand > dt_for_each_child_node(node, shm_node) > { > const struct membank *boot_shm_bank; > -- > 2.25.1 >
On 01/04/2025 14:57, Bertrand Marquis wrote: > > > Hi Michal, > >> On 1 Apr 2025, at 11:09, Michal Orzel <michal.orzel@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.orzel@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. > > 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. ~Michal
Hi, > On 1 Apr 2025, at 16:22, Orzel, Michal <Michal.Orzel@amd.com> wrote: > > > > On 01/04/2025 14:57, Bertrand Marquis wrote: >> >> >> Hi Michal, >> >>> On 1 Apr 2025, at 11:09, Michal Orzel <michal.orzel@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.orzel@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. > >> >> 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. Cheers Bertrand > > ~Michal
On 01/04/2025 16:49, Bertrand Marquis wrote: > > > Hi, > >> On 1 Apr 2025, at 16:22, Orzel, Michal <Michal.Orzel@amd.com> wrote: >> >> >> >> On 01/04/2025 14:57, Bertrand Marquis wrote: >>> >>> >>> Hi Michal, >>> >>>> On 1 Apr 2025, at 11:09, Michal Orzel <michal.orzel@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.orzel@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. /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
Hi Michal, > On 1 Apr 2025, at 17:21, Orzel, Michal <michal.orzel@amd.com> wrote: > > > > On 01/04/2025 16:49, Bertrand Marquis wrote: >> >> >> Hi, >> >>> On 1 Apr 2025, at 16:22, Orzel, Michal <Michal.Orzel@amd.com> wrote: >>> >>> >>> >>> On 01/04/2025 14:57, Bertrand Marquis wrote: >>>> >>>> >>>> Hi Michal, >>>> >>>>> On 1 Apr 2025, at 11:09, Michal Orzel <michal.orzel@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.orzel@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. Cheers Bertrand > ~Michal
On 01/04/2025 17:53, Bertrand Marquis wrote: > > > Hi Michal, > >> On 1 Apr 2025, at 17:21, Orzel, Michal <michal.orzel@amd.com> wrote: >> >> >> >> On 01/04/2025 16:49, Bertrand Marquis wrote: >>> >>> >>> Hi, >>> >>>> On 1 Apr 2025, at 16:22, Orzel, Michal <Michal.Orzel@amd.com> wrote: >>>> >>>> >>>> >>>> On 01/04/2025 14:57, Bertrand Marquis wrote: >>>>> >>>>> >>>>> Hi Michal, >>>>> >>>>>> On 1 Apr 2025, at 11:09, Michal Orzel <michal.orzel@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.orzel@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
Hi Michal, > On 1 Apr 2025, at 18:42, Orzel, Michal <Michal.Orzel@amd.com> wrote: > > > > On 01/04/2025 17:53, Bertrand Marquis wrote: >> >> >> Hi Michal, >> >>> On 1 Apr 2025, at 17:21, Orzel, Michal <michal.orzel@amd.com> wrote: >>> >>> >>> >>> On 01/04/2025 16:49, Bertrand Marquis wrote: >>>> >>>> >>>> Hi, >>>> >>>>> On 1 Apr 2025, at 16:22, Orzel, Michal <Michal.Orzel@amd.com> wrote: >>>>> >>>>> >>>>> >>>>> On 01/04/2025 14:57, Bertrand Marquis wrote: >>>>>> >>>>>> >>>>>> Hi Michal, >>>>>> >>>>>>> On 1 Apr 2025, at 11:09, Michal Orzel <michal.orzel@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.orzel@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
On 02/04/2025 08:18, Bertrand Marquis wrote: > > > Hi Michal, > >> On 1 Apr 2025, at 18:42, Orzel, Michal <Michal.Orzel@amd.com> wrote: >> >> >> >> On 01/04/2025 17:53, Bertrand Marquis wrote: >>> >>> >>> Hi Michal, >>> >>>> On 1 Apr 2025, at 17:21, Orzel, Michal <michal.orzel@amd.com> wrote: >>>> >>>> >>>> >>>> On 01/04/2025 16:49, Bertrand Marquis wrote: >>>>> >>>>> >>>>> Hi, >>>>> >>>>>> On 1 Apr 2025, at 16:22, Orzel, Michal <Michal.Orzel@amd.com> wrote: >>>>>> >>>>>> >>>>>> >>>>>> On 01/04/2025 14:57, Bertrand Marquis wrote: >>>>>>> >>>>>>> >>>>>>> Hi Michal, >>>>>>> >>>>>>>> On 1 Apr 2025, at 11:09, Michal Orzel <michal.orzel@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.orzel@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. 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
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"); + dt_for_each_child_node(node, shm_node) { const struct membank *boot_shm_bank;
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@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(-)