Re: [PATCH v5 00/13] Arm cache coloring

[Michal Orzel <michal.orzel@xxxxxxx>]

On 04/01/2024 10:37, Carlo Nonato wrote:
> 
> 
> Hi Stefano,
> 
> On Thu, Jan 4, 2024 at 2:55 AM Stefano Stabellini
> <sstabellini@xxxxxxxxxx> wrote:
>>
>> On Wed, 3 Jan 2024, Stefano Stabellini wrote:
>>> Also I tried this patch series on the "staging" branch and Xen failed to
>>> boot, no messages at all from Xen so it must be an early boot failure. I
>>> am passing this command line options to Xen and running it on QEMU:
>>>
>>> dom0_mem=1024M dom0_max_vcpus=1 xen-llc-colors=0 dom0-llc-colors=1-5 
>>> llc-way-size=65535 llc-coloring=true
>>
>> I managed to make it to work successfully with the following command
>> line:
>>
>> xen-llc-colors=0 dom0-llc-colors=1-5 llc-way-size=64K llc-coloring=on
>>
>> I think the problem was llc-way-size that needs to be rounded up (64K
>> instead of 65535).
>>
>> Also I found a few build issues when building for other architectures or
>> different kconfig options. This patch addresses those issues (however it
>> is not to be taken as is as the build issues should not be introduced in
>> the first place and there are probably better way to fix them, but I
>> hope it can help).
>>
>> Cheers,
>>
>> Stefano
>>
>>
>> diff --git a/xen/arch/arm/llc-coloring.c b/xen/arch/arm/llc-coloring.c
>> index f434efc45b..efe5cf3c23 100644
>> --- a/xen/arch/arm/llc-coloring.c
>> +++ b/xen/arch/arm/llc-coloring.c
>> @@ -39,7 +39,7 @@ static unsigned int __ro_after_init xen_num_colors;
>>
>>  #define mfn_color_mask              (nr_colors - 1)
>>  #define mfn_to_color(mfn)           (mfn_x(mfn) & mfn_color_mask)
>> -#define mfn_set_color(mfn, color)   ((mfn_x(mfn) & ~mfn_color_mask) | 
>> (color))
>> +#define mfn_set_color(mfn, color)   (_mfn((mfn_x(mfn) & ~mfn_color_mask) | 
>> (color)))
> 
> Thanks for spotting this.
> 
>>  /*
>>   * Parse the coloring configuration given in the buf string, following the
>> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
>> index 3bb0c9221f..bf16703e24 100644
>> --- a/xen/arch/arm/mm.c
>> +++ b/xen/arch/arm/mm.c
>> @@ -610,10 +610,10 @@ void __init setup_pagetables(unsigned long 
>> boot_phys_offset, paddr_t xen_paddr)
>>      pte.pt.table = 1;
>>      xen_second[second_table_offset(FIXMAP_ADDR(0))] = pte;
>>
>> +#ifdef CONFIG_ARM_64
>>      if ( llc_coloring_enabled )
>>          ttbr = virt_to_maddr(virt_to_reloc_virt(xen_pgtable));
>>      else
>> -#ifdef CONFIG_ARM_64
>>          ttbr = (uintptr_t) xen_pgtable + phys_offset;
>>  #else
>>          ttbr = (uintptr_t) cpu0_pgtable + phys_offset;
>> diff --git a/xen/include/xen/llc-coloring.h b/xen/include/xen/llc-coloring.h
>> index 7cd481e955..516139c4ff 100644
>> --- a/xen/include/xen/llc-coloring.h
>> +++ b/xen/include/xen/llc-coloring.h
>> @@ -21,7 +21,27 @@
>>  extern bool llc_coloring_enabled;
>>  #define llc_coloring_enabled (llc_coloring_enabled)
>>  #endif
>> -
>> +#else
>> +static inline void *xen_remap_colored(mfn_t xen_fn, paddr_t xen_size)
>> +{
>> +    return NULL;
>> +}
>> +static inline int domain_set_llc_colors_from_str(struct domain *d, const 
>> char *str)
>> +{
>> +    return -ENOSYS;
>> +}
>> +static inline int dom0_set_llc_colors(struct domain *d)
>> +{
>> +    return 0;
>> +}
>> +static inline bool llc_coloring_init(void)
>> +{
>> +    return false;
>> +}
>> +static inline paddr_t xen_colored_map_size(paddr_t size)
>> +{
>> +    return 0;
>> +}
>>  #endif
>>
>>  #ifndef llc_coloring_enabled
> 
> Sorry for the compilation mess.
> 
> This is definitely a solution, but I wonder if the best thing to do would be
> to move all signatures in the common header, without the stubs, relying again
> on DCE. This seems a little strange to me because users of some of those
> functions are only in arm code, and they always have to be protected with
> llc_coloring_enabled global variable/macro if, but it works (now I'm
> compiling also for arm32 and x86).
There are a lot of places in Xen relying on DCE, so I'd be ok with that (you 
can wait
for Stefano's opinion). Furthermore, you already rely on that in case of e.g. 
domain_set_llc_colors_domctl,
domain_llc_coloring_free.

~Michal