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

[Carlo Nonato <carlo.nonato@xxxxxxxxxxxxxxx>]

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).

Thanks.