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

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

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

Cheers,
Luca

> 
> ~Michal
>