| Message ID | 20250217102732.2343729-3-luca.fancellu@arm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Prerequisite patches for Arm64 MPU build | expand |
On 17/02/2025 11:27, Luca Fancellu wrote: > > > LLC coloring can be used only on MMU system, move the code > that selects it from ARM_64 to MMU and add the ARM_64 > dependency. > > While there, add a clarification comment in the startup > code related to the LLC coloring, because boot_fdt_info() > is required to be called before llc_coloring_init(), because > it parses the memory banks from the DT, but to discover that > the developer needs to dig into the function. Well, if at all such requirement would better be expressed using ASSERT in get_xen_paddr(). The reason is ... > > Signed-off-by: Luca Fancellu <luca.fancellu@arm.com> > --- > v2 changes: > - dropped part of the v1 code, now this one is simpler, I will > discuss better how to design a common boot flow for MPU and > implement on another patch. > > --- > --- > xen/arch/arm/Kconfig | 2 +- > xen/arch/arm/setup.c | 1 + > 2 files changed, 2 insertions(+), 1 deletion(-) > > diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig > index a26d3e11827c..ffdff1f0a36c 100644 > --- a/xen/arch/arm/Kconfig > +++ b/xen/arch/arm/Kconfig > @@ -8,7 +8,6 @@ config ARM_64 > depends on !ARM_32 > select 64BIT > select HAS_FAST_MULTIPLY > - select HAS_LLC_COLORING if !NUMA > > config ARM > def_bool y > @@ -76,6 +75,7 @@ choice > > config MMU > bool "MMU" > + select HAS_LLC_COLORING if !NUMA && ARM_64 > select HAS_PMAP > select HAS_VMAP > select HAS_PASSTHROUGH > diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c > index c1f2d1b89d43..91fa579e73e5 100644 > --- a/xen/arch/arm/setup.c > +++ b/xen/arch/arm/setup.c > @@ -328,6 +328,7 @@ void asmlinkage __init start_xen(unsigned long fdt_paddr) > (paddr_t)(uintptr_t)(_end - _start), false); > BUG_ON(!xen_bootmodule); > > + /* This parses memory banks needed for LLC coloring */ this comment is confusing. It reads as if boot_fdt_info was here only for LLC coloring. Moreover, if you add such comment here, why not adding a comment above boot_fdt_cmdline and cmdline_parse which are hard dependency for LLC coloring code to read LLC cmdline options parsed by llc_coloring_init? ~Michal
Hi Michal, > On 17 Feb 2025, at 12:55, Orzel, Michal <michal.orzel@amd.com> wrote: > > > > On 17/02/2025 11:27, Luca Fancellu wrote: >> >> >> LLC coloring can be used only on MMU system, move the code >> that selects it from ARM_64 to MMU and add the ARM_64 >> dependency. >> >> While there, add a clarification comment in the startup >> code related to the LLC coloring, because boot_fdt_info() >> is required to be called before llc_coloring_init(), because >> it parses the memory banks from the DT, but to discover that >> the developer needs to dig into the function. > Well, if at all such requirement would better be expressed using ASSERT in > get_xen_paddr(). Ok, you mean asserting that mem ( bootinfo_get_mem() ) is not empty? > The reason is ... > >> >> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com> >> --- >> v2 changes: >> - dropped part of the v1 code, now this one is simpler, I will >> discuss better how to design a common boot flow for MPU and >> implement on another patch. >> >> --- >> --- >> xen/arch/arm/Kconfig | 2 +- >> xen/arch/arm/setup.c | 1 + >> 2 files changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig >> index a26d3e11827c..ffdff1f0a36c 100644 >> --- a/xen/arch/arm/Kconfig >> +++ b/xen/arch/arm/Kconfig >> @@ -8,7 +8,6 @@ config ARM_64 >> depends on !ARM_32 >> select 64BIT >> select HAS_FAST_MULTIPLY >> - select HAS_LLC_COLORING if !NUMA >> >> config ARM >> def_bool y >> @@ -76,6 +75,7 @@ choice >> >> config MMU >> bool "MMU" >> + select HAS_LLC_COLORING if !NUMA && ARM_64 >> select HAS_PMAP >> select HAS_VMAP >> select HAS_PASSTHROUGH >> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c >> index c1f2d1b89d43..91fa579e73e5 100644 >> --- a/xen/arch/arm/setup.c >> +++ b/xen/arch/arm/setup.c >> @@ -328,6 +328,7 @@ void asmlinkage __init start_xen(unsigned long fdt_paddr) >> (paddr_t)(uintptr_t)(_end - _start), false); >> BUG_ON(!xen_bootmodule); >> >> + /* This parses memory banks needed for LLC coloring */ > this comment is confusing. It reads as if boot_fdt_info was here only for LLC > coloring. Moreover, if you add such comment here, why not adding a comment above > boot_fdt_cmdline and cmdline_parse which are hard dependency for LLC coloring > code to read LLC cmdline options parsed by llc_coloring_init? Yeah I get your point, do you think we should just go with the assert or maybe add a comment on top of llc_coloring_init() to say it needs to be called after boot_fdt_info and boot_fdt_cmdline in order to work? Also because the assert in get_xen_paddr (llc-coloring.c) won’t be compiled on a setup not having cache coloring Cheers, Luca > > ~Michal >
On 17/02/2025 14:15, Luca Fancellu wrote: > > > Hi Michal, > >> On 17 Feb 2025, at 12:55, Orzel, Michal <michal.orzel@amd.com> wrote: >> >> >> >> On 17/02/2025 11:27, Luca Fancellu wrote: >>> >>> >>> LLC coloring can be used only on MMU system, move the code >>> that selects it from ARM_64 to MMU and add the ARM_64 >>> dependency. >>> >>> While there, add a clarification comment in the startup >>> code related to the LLC coloring, because boot_fdt_info() >>> is required to be called before llc_coloring_init(), because >>> it parses the memory banks from the DT, but to discover that >>> the developer needs to dig into the function. >> Well, if at all such requirement would better be expressed using ASSERT in >> get_xen_paddr(). > > Ok, you mean asserting that mem ( bootinfo_get_mem() ) is not empty? > >> The reason is ... >> >>> >>> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com> >>> --- >>> v2 changes: >>> - dropped part of the v1 code, now this one is simpler, I will >>> discuss better how to design a common boot flow for MPU and >>> implement on another patch. >>> >>> --- >>> --- >>> xen/arch/arm/Kconfig | 2 +- >>> xen/arch/arm/setup.c | 1 + >>> 2 files changed, 2 insertions(+), 1 deletion(-) >>> >>> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig >>> index a26d3e11827c..ffdff1f0a36c 100644 >>> --- a/xen/arch/arm/Kconfig >>> +++ b/xen/arch/arm/Kconfig >>> @@ -8,7 +8,6 @@ config ARM_64 >>> depends on !ARM_32 >>> select 64BIT >>> select HAS_FAST_MULTIPLY >>> - select HAS_LLC_COLORING if !NUMA >>> >>> config ARM >>> def_bool y >>> @@ -76,6 +75,7 @@ choice >>> >>> config MMU >>> bool "MMU" >>> + select HAS_LLC_COLORING if !NUMA && ARM_64 >>> select HAS_PMAP >>> select HAS_VMAP >>> select HAS_PASSTHROUGH >>> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c >>> index c1f2d1b89d43..91fa579e73e5 100644 >>> --- a/xen/arch/arm/setup.c >>> +++ b/xen/arch/arm/setup.c >>> @@ -328,6 +328,7 @@ void asmlinkage __init start_xen(unsigned long fdt_paddr) >>> (paddr_t)(uintptr_t)(_end - _start), false); >>> BUG_ON(!xen_bootmodule); >>> >>> + /* This parses memory banks needed for LLC coloring */ >> this comment is confusing. It reads as if boot_fdt_info was here only for LLC >> coloring. Moreover, if you add such comment here, why not adding a comment above >> boot_fdt_cmdline and cmdline_parse which are hard dependency for LLC coloring >> code to read LLC cmdline options parsed by llc_coloring_init? > > Yeah I get your point, do you think we should just go with the assert or maybe add a comment > on top of llc_coloring_init() to say it needs to be called after boot_fdt_info and boot_fdt_cmdline > in order to work? Also because the assert in get_xen_paddr (llc-coloring.c) won’t be compiled on > a setup not having cache coloring TBH I would not do anything. I assume such comment would target developers. Then why are we special casing LLC coloring and not for example boot_fdt_cmdline that also needs to be called after boot_fdt_info to parse legacy location for cmdline? There are dozens of examples in start_xen where we rely on a specific order and developer always needs to check if rearranging is possible. Adding a single comment for LLC would not improve the situation and would just result in inconsistency leading to confusion. That's why I would only consider adding an ASSERT but in this case, there are more things than memory bank that LLC init relies on. ~Michal
>>>> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c >>>> index c1f2d1b89d43..91fa579e73e5 100644 >>>> --- a/xen/arch/arm/setup.c >>>> +++ b/xen/arch/arm/setup.c >>>> @@ -328,6 +328,7 @@ void asmlinkage __init start_xen(unsigned long fdt_paddr) >>>> (paddr_t)(uintptr_t)(_end - _start), false); >>>> BUG_ON(!xen_bootmodule); >>>> >>>> + /* This parses memory banks needed for LLC coloring */ >>> this comment is confusing. It reads as if boot_fdt_info was here only for LLC >>> coloring. Moreover, if you add such comment here, why not adding a comment above >>> boot_fdt_cmdline and cmdline_parse which are hard dependency for LLC coloring >>> code to read LLC cmdline options parsed by llc_coloring_init? >> >> Yeah I get your point, do you think we should just go with the assert or maybe add a comment >> on top of llc_coloring_init() to say it needs to be called after boot_fdt_info and boot_fdt_cmdline >> in order to work? Also because the assert in get_xen_paddr (llc-coloring.c) won’t be compiled on >> a setup not having cache coloring > TBH I would not do anything. I assume such comment would target developers. Then > why are we special casing LLC coloring and not for example boot_fdt_cmdline that > also needs to be called after boot_fdt_info to parse legacy location for > cmdline? There are dozens of examples in start_xen where we rely on a specific > order and developer always needs to check if rearranging is possible. Adding a > single comment for LLC would not improve the situation and would just result in > inconsistency leading to confusion. That's why I would only consider adding an > ASSERT but in this case, there are more things than memory bank that LLC init > relies on. ok, yes of course there are a lot of things that rely on the right order of calls, however I felt that an optional feature like LLC would benefit for the extra documentation. In other cases rearranging calls could lead to Xen not booting, but in this case (as it happened to me) it was not immediately clear. Anyway if that’s your preference I will drop the comment, I would not add the assert in this commit as it feels not the right place, but I can see that in get_xen_paddr if mem is empty we would not touch paddr and have a panic later, so we would notice if something happen. Cheers, Luca
diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig index a26d3e11827c..ffdff1f0a36c 100644 --- a/xen/arch/arm/Kconfig +++ b/xen/arch/arm/Kconfig @@ -8,7 +8,6 @@ config ARM_64 depends on !ARM_32 select 64BIT select HAS_FAST_MULTIPLY - select HAS_LLC_COLORING if !NUMA config ARM def_bool y @@ -76,6 +75,7 @@ choice config MMU bool "MMU" + select HAS_LLC_COLORING if !NUMA && ARM_64 select HAS_PMAP select HAS_VMAP select HAS_PASSTHROUGH diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c index c1f2d1b89d43..91fa579e73e5 100644 --- a/xen/arch/arm/setup.c +++ b/xen/arch/arm/setup.c @@ -328,6 +328,7 @@ void asmlinkage __init start_xen(unsigned long fdt_paddr) (paddr_t)(uintptr_t)(_end - _start), false); BUG_ON(!xen_bootmodule); + /* This parses memory banks needed for LLC coloring */ fdt_size = boot_fdt_info(device_tree_flattened, fdt_paddr); cmdline = boot_fdt_cmdline(device_tree_flattened);
LLC coloring can be used only on MMU system, move the code that selects it from ARM_64 to MMU and add the ARM_64 dependency. While there, add a clarification comment in the startup code related to the LLC coloring, because boot_fdt_info() is required to be called before llc_coloring_init(), because it parses the memory banks from the DT, but to discover that the developer needs to dig into the function. Signed-off-by: Luca Fancellu <luca.fancellu@arm.com> --- v2 changes: - dropped part of the v1 code, now this one is simpler, I will discuss better how to design a common boot flow for MPU and implement on another patch. --- --- xen/arch/arm/Kconfig | 2 +- xen/arch/arm/setup.c | 1 + 2 files changed, 2 insertions(+), 1 deletion(-)