Re: [PATCH v2 2/2] xen/arm: Restrict Kconfig configuration for LLC coloring

["Orzel, Michal" <michal.orzel@xxxxxxx>]

On 17/02/2025 14:15, Luca Fancellu wrote:
> 
> 
> Hi Michal,
> 
>> On 17 Feb 2025, at 12:55, Orzel, Michal <michal.orzel@xxxxxxx> 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@xxxxxxx>
>>> ---
>>> 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