On 17/02/2025 14:15, Luca Fancellu wrote:
>
>
> Hi Michal,
>
>> On 17 Feb 2025, at 12:55, Orzel, Michal <[email protected]> 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 <[email protected]>
>>> ---
>>> 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
