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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Thu, 4 Jan 2024, Michal Orzel wrote:
> 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.
 
Yeah I think that would be fine