|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v6 02/15] xen/arm: add initial support for LLC coloring on arm64
Hi Michal,
On Wed, Feb 14, 2024 at 11:14 AM Michal Orzel <michal.orzel@xxxxxxx> wrote:
>
> Hi Carlo,
>
> On 29/01/2024 18:17, Carlo Nonato wrote:
> >
> >
> > LLC coloring needs to know the last level cache layout in order to make the
> > best use of it. This can be probed by inspecting the CLIDR_EL1 register,
> > so the Last Level is defined as the last level visible by this register.
> > Note that this excludes system caches in some platforms.
> >
> > Static memory allocation and cache coloring are incompatible because static
> > memory can't be guaranteed to use only colors assigned to the domain.
> > Panic during DomUs creation when both are enabled.
> >
> > Based on original work from: Luca Miccio <lucmiccio@xxxxxxxxx>
> >
> > Signed-off-by: Carlo Nonato <carlo.nonato@xxxxxxxxxxxxxxx>
> > Signed-off-by: Marco Solieri <marco.solieri@xxxxxxxxxxxxxxx>
> > ---
> > v6:
> > - get_llc_way_size() now checks for at least separate I/D caches
> > v5:
> > - used - instead of _ for filenames
> > - moved static-mem check in this patch
> > - moved dom0 colors parsing in next patch
> > - moved color allocation and configuration in next patch
> > - moved check_colors() in next patch
> > - colors are now printed in short form
> > v4:
> > - added "llc-coloring" cmdline option for the boot-time switch
> > - dom0 colors are now checked during domain init as for any other domain
> > - fixed processor.h masks bit width
> > - check for overflow in parse_color_config()
> > - check_colors() now checks also that colors are sorted and unique
> > ---
> > docs/misc/cache-coloring.rst | 11 ++++
> > xen/arch/arm/Kconfig | 1 +
> > xen/arch/arm/Makefile | 1 +
> > xen/arch/arm/dom0less-build.c | 6 +++
> > xen/arch/arm/include/asm/processor.h | 16 ++++++
> > xen/arch/arm/llc-coloring.c | 75 ++++++++++++++++++++++++++++
> > xen/arch/arm/setup.c | 3 ++
> > 7 files changed, 113 insertions(+)
> > create mode 100644 xen/arch/arm/llc-coloring.c
> >
> > diff --git a/docs/misc/cache-coloring.rst b/docs/misc/cache-coloring.rst
> > index 9fe01e99e1..0535b5c656 100644
> > --- a/docs/misc/cache-coloring.rst
> > +++ b/docs/misc/cache-coloring.rst
> > @@ -85,3 +85,14 @@ More specific documentation is available at
> > `docs/misc/xen-command-line.pandoc`.
> > +----------------------+-------------------------------+
> > | ``llc-way-size`` | set the LLC way size |
> > +----------------------+-------------------------------+
> > +
> > +Known issues and limitations
> > +****************************
> > +
> > +"xen,static-mem" isn't supported when coloring is enabled
> > +#########################################################
> > +
> > +In the domain configuration, "xen,static-mem" allows memory to be
> > statically
> > +allocated to the domain. This isn't possible when LLC coloring is enabled,
> > +because that memory can't be guaranteed to use only colors assigned to the
> > +domain.
> > diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
> > index 50e9bfae1a..55143f86a9 100644
> > --- a/xen/arch/arm/Kconfig
> > +++ b/xen/arch/arm/Kconfig
> > @@ -8,6 +8,7 @@ config ARM_64
> > depends on !ARM_32
> > select 64BIT
> > select HAS_FAST_MULTIPLY
> > + select HAS_LLC_COLORING
> >
> > config ARM
> > def_bool y
> > diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
> > index 33c677672f..c9a1cd298d 100644
> > --- a/xen/arch/arm/Makefile
> > +++ b/xen/arch/arm/Makefile
> > @@ -37,6 +37,7 @@ obj-$(CONFIG_IOREQ_SERVER) += ioreq.o
> > obj-y += irq.o
> > obj-y += kernel.init.o
> > obj-$(CONFIG_LIVEPATCH) += livepatch.o
> > +obj-$(CONFIG_LLC_COLORING) += llc-coloring.o
> > obj-y += mem_access.o
> > obj-y += mm.o
> > obj-y += monitor.o
> > diff --git a/xen/arch/arm/dom0less-build.c b/xen/arch/arm/dom0less-build.c
> > index fb63ec6fd1..1142f7f74a 100644
> > --- a/xen/arch/arm/dom0less-build.c
> > +++ b/xen/arch/arm/dom0less-build.c
> > @@ -5,6 +5,7 @@
> > #include <xen/grant_table.h>
> > #include <xen/iocap.h>
> > #include <xen/libfdt/libfdt.h>
> > +#include <xen/llc-coloring.h>
> > #include <xen/sched.h>
> > #include <xen/serial.h>
> > #include <xen/sizes.h>
> > @@ -879,7 +880,12 @@ void __init create_domUs(void)
> > panic("No more domain IDs available\n");
> >
> > if ( dt_find_property(node, "xen,static-mem", NULL) )
> > + {
> > + if ( llc_coloring_enabled )
> > + panic("LLC coloring and static memory are incompatible\n");
> > +
> > flags |= CDF_staticmem;
> > + }
> >
> > if ( dt_property_read_bool(node, "direct-map") )
> > {
> > diff --git a/xen/arch/arm/include/asm/processor.h
> > b/xen/arch/arm/include/asm/processor.h
> > index 8e02410465..336933ee62 100644
> > --- a/xen/arch/arm/include/asm/processor.h
> > +++ b/xen/arch/arm/include/asm/processor.h
> > @@ -18,6 +18,22 @@
> > #define CTR_IDC_SHIFT 28
> > #define CTR_DIC_SHIFT 29
> >
> > +/* CCSIDR Current Cache Size ID Register */
> > +#define CCSIDR_LINESIZE_MASK _AC(0x7, ULL)
> Why ULL and not UL? ccsidr is of register_t type
Julien, while reviewing an earlier version:
> Please use ULL here otherwise someone using MASK << SHIFT will have the
> expected result.
https://patchew.org/Xen/20220826125111.152261-1-carlo.nonato@xxxxxxxxxxxxxxx/20220826125111.152261-2-carlo.nonato@xxxxxxxxxxxxxxx/#08956082-c194-8bae-cb25-44e4e3227689@xxxxxxx
>
> > +#define CCSIDR_NUMSETS_SHIFT 13
> > +#define CCSIDR_NUMSETS_MASK _AC(0x3fff, ULL)
> > +#define CCSIDR_NUMSETS_SHIFT_FEAT_CCIDX 32
> > +#define CCSIDR_NUMSETS_MASK_FEAT_CCIDX _AC(0xffffff, ULL)
> > +
> > +/* CSSELR Cache Size Selection Register */
> > +#define CSSELR_LEVEL_MASK _AC(0x7, UL)
> > +#define CSSELR_LEVEL_SHIFT 1
> > +
> > +/* CLIDR Cache Level ID Register */
> > +#define CLIDR_CTYPEn_SHIFT(n) (3 * (n - 1))
> n should be within parentheses
>
> > +#define CLIDR_CTYPEn_MASK _AC(0x7, UL)
> > +#define CLIDR_CTYPEn_LEVELS 7
> > +
> > #define ICACHE_POLICY_VPIPT 0
> > #define ICACHE_POLICY_AIVIVT 1
> > #define ICACHE_POLICY_VIPT 2
> > diff --git a/xen/arch/arm/llc-coloring.c b/xen/arch/arm/llc-coloring.c
> > new file mode 100644
> > index 0000000000..eee1e80e2d
> > --- /dev/null
> > +++ b/xen/arch/arm/llc-coloring.c
> > @@ -0,0 +1,75 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +/*
> > + * Last Level Cache (LLC) coloring support for ARM
> > + *
> > + * Copyright (C) 2022 Xilinx Inc.
> > + */
> > +#include <xen/llc-coloring.h>
> > +#include <xen/types.h>
> > +
> > +#include <asm/processor.h>
> > +#include <asm/sysregs.h>
> > +
> > +/* Return the LLC way size by probing the hardware */
> > +unsigned int __init get_llc_way_size(void)
> > +{
> > + register_t ccsidr_el1;
> > + register_t clidr_el1 = READ_SYSREG(CLIDR_EL1);
> > + register_t csselr_el1 = READ_SYSREG(CSSELR_EL1);
> > + register_t id_aa64mmfr2_el1 = READ_SYSREG(ID_AA64MMFR2_EL1);
> > + uint32_t ccsidr_numsets_shift = CCSIDR_NUMSETS_SHIFT;
> > + uint32_t ccsidr_numsets_mask = CCSIDR_NUMSETS_MASK;
> > + unsigned int n, line_size, num_sets;
> > +
> > + for ( n = CLIDR_CTYPEn_LEVELS; n != 0; n-- )
> > + {
> > + uint8_t ctype_n = (clidr_el1 >> CLIDR_CTYPEn_SHIFT(n)) &
> > + CLIDR_CTYPEn_MASK;
> > +
> > + /* At least separate I/D caches (see Arm ARM DDI 0487H.a D13.2.27)
> > */
> I'm a bit confused here given that this comment does not reflect the line
> below (also please refer to the latest spec).
> Since 0b011 is "Separate instruction and data caches" you would break only
> for Unified cache.
> That said, we care about last level cache that is visible to SW and I'm not
> aware of any Arm CPU where L2,L3 is not unified.
You're right, that should have been >=.
Anyway I can check more explicitly for == 0b100.
> > + if ( ctype_n > 0b011 )
> > + break;
> > + }
> > +
> > + if ( n == 0 )
> > + return 0;
> > +
> > + WRITE_SYSREG((n - 1) << CSSELR_LEVEL_SHIFT, CSSELR_EL1);
> > +
> no need for this empty line
>
> > + isb();
> > +
> > + ccsidr_el1 = READ_SYSREG(CCSIDR_EL1);
> > +
> > + /* Arm ARM: (Log2(Number of bytes in cache line)) - 4 */
> > + line_size = 1U << ((ccsidr_el1 & CCSIDR_LINESIZE_MASK) + 4);
> > +
> > + /* If FEAT_CCIDX is enabled, CCSIDR_EL1 has a different bit layout */
> > + if ( (id_aa64mmfr2_el1 >> ID_AA64MMFR2_CCIDX_SHIFT) & 0x7 )
> > + {
> > + ccsidr_numsets_shift = CCSIDR_NUMSETS_SHIFT_FEAT_CCIDX;
> > + ccsidr_numsets_mask = CCSIDR_NUMSETS_MASK_FEAT_CCIDX;
> > + }
> empty line here please
>
> > + /* Arm ARM: (Number of sets in cache) - 1 */
> > + num_sets = ((ccsidr_el1 >> ccsidr_numsets_shift) &
> > ccsidr_numsets_mask) + 1;
> > +
> > + printk(XENLOG_INFO "LLC found: L%u (line size: %u bytes, sets num:
> > %u)\n",
> > + n, line_size, num_sets);
> > +
> > + /* Restore value in CSSELR_EL1 */
> > + WRITE_SYSREG(csselr_el1, CSSELR_EL1);
> > + isb();
> > +
> > + return line_size * num_sets;
> > +}
> > +
> > +void __init arch_llc_coloring_init(void) {}
> > +
> > +/*
> > + * Local variables:
> > + * mode: C
> > + * c-file-style: "BSD"
> > + * c-basic-offset: 4
> > + * tab-width: 4
> > + * indent-tabs-mode: nil
> > + * End:
> > + */
> > diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> > index 59dd9bb25a..14cb023783 100644
> > --- a/xen/arch/arm/setup.c
> > +++ b/xen/arch/arm/setup.c
> > @@ -12,6 +12,7 @@
> > #include <xen/device_tree.h>
> > #include <xen/domain_page.h>
> > #include <xen/grant_table.h>
> > +#include <xen/llc-coloring.h>
> > #include <xen/types.h>
> > #include <xen/string.h>
> > #include <xen/serial.h>
> > @@ -746,6 +747,8 @@ void asmlinkage __init start_xen(unsigned long
> > boot_phys_offset,
> > printk("Command line: %s\n", cmdline);
> > cmdline_parse(cmdline);
> >
> > + llc_coloring_init();
> I think a check with llc_coloring_enabled is missing, given there is none in
> llc_coloring_init
You're right. It should have been in llc_coloring_init(), my bad.
Thanks.
> ~Michal
>
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |