Message ID | 20220304174701.1453977-6-marco.solieri@minervasys.tech (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | Arm cache coloring | expand |
Hi, On 04/03/2022 17:46, Marco Solieri wrote: > From: Luca Miccio <lucmiccio@gmail.com> > > The size of the LLC way is a crucial parameter for the cache coloring > support, since it determines the maximum number of available colors on > the platform. This parameter can currently be retrieved only from > the way_size bootarg and it is prone to misconfiguration nullifying the > coloring mechanism and breaking cache isolation. Reading this sentence, I think the command line option should be introduced after this patch (assuming this is necessary). This will avoid undoing/fixing a "bug" that was introduced by the same series. > > Add an alternative and more safe method to retrieve the way size by > directly asking the hardware, namely using CCSIDR_EL1 and CSSELR_EL1 > registers. > > This method has to check also if at least L2 is implemented in the > hardware since there are scenarios where only L1 cache is availble, e.g, In the previous patch, the description for the Kconfig suggests that the cache coloring will only happen on L2. But here you are also adding L1. So I think the documentation needs to be updated. Typo: s/availble/available/ > QEMU. > > Signed-off-by: Luca Miccio <lucmiccio@gmail.com> > Signed-off-by: Marco Solieri <marco.solieri@minervasys.tech> > Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com> > --- > xen/arch/arm/coloring.c | 76 +++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 76 insertions(+) > > diff --git a/xen/arch/arm/coloring.c b/xen/arch/arm/coloring.c > index 8f1cff6efb..e3d490b453 100644 > --- a/xen/arch/arm/coloring.c > +++ b/xen/arch/arm/coloring.c > @@ -25,7 +25,10 @@ > #include <xen/lib.h> > #include <xen/errno.h> > #include <xen/param.h> > + NIT: I think this belongs to patch #4. > +#include <asm/sysregs.h> Please order the include alphabetically. > #include <asm/coloring.h> > +#include <asm/io.h> You don't seem to use read*/write* helper. So why do you need this? > > /* Number of color(s) assigned to Xen */ > static uint32_t xen_col_num; > @@ -39,6 +42,79 @@ static uint32_t dom0_col_mask[MAX_COLORS_CELLS]; > > static uint64_t way_size; > > +#define CTR_LINESIZE_MASK 0x7 > +#define CTR_SIZE_SHIFT 13 > +#define CTR_SIZE_MASK 0x3FFF > +#define CTR_SELECT_L2 1 << 1 > +#define CTR_SELECT_L3 1 << 2 > +#define CTR_CTYPEn_MASK 0x7 > +#define CTR_CTYPE2_SHIFT 3 > +#define CTR_CTYPE3_SHIFT 6 > +#define CTR_LLC_ON 1 << 2 > +#define CTR_LOC_SHIFT 24 > +#define CTR_LOC_MASK 0x7 > +#define CTR_LOC_L2 1 << 1 > +#define CTR_LOC_NOT_IMPLEMENTED 1 << 0 We already define some CTR_* in processor.h. Please any extra one there. > + > + > +/* Return the way size of last level cache by asking the hardware */ > +static uint64_t get_llc_way_size(void) This will break compilation as you are introducing get_llc_way_size() but not using it. I would suggest to fold this patch in the next one. > +{ > + uint32_t cache_sel = READ_SYSREG64(CSSELR_EL1); The return type for READ_SYSREG64() is uint64_t. That said, the equivalent register on 32bit is CSSELR which is 32-bit. So this should be READ_SYSREG() and the matching type is register_t. > + uint32_t cache_global_info = READ_SYSREG64(CLIDR_EL1); Same remark here. Except the matching register is CLIDR. > + uint32_t cache_info; > + uint32_t cache_line_size; > + uint32_t cache_set_num; > + uint32_t cache_sel_tmp; > + > + printk(XENLOG_INFO "Get information on LLC\n"); > + printk(XENLOG_INFO "Cache CLIDR_EL1: 0x%"PRIx32"\n", cache_global_info); > + > + /* Check if at least L2 is implemented */ > + if ( ((cache_global_info >> CTR_LOC_SHIFT) & CTR_LOC_MASK) This is a bit confusing. cache_global_info is storing CLIDR_* but you are using macro starting with CTR_*. Did you intend to name the macros CLIDR_*? The same remark goes for the other use of CTR_ below. The name of the macros should match the register they are meant to be used on. > + == CTR_LOC_NOT_IMPLEMENTED ) I am a bit confused this the check here. Shouln't you check that Ctype2 is notn 0 instead? > + { > + printk(XENLOG_ERR "ERROR: L2 Cache not implemented\n"); > + return 0; > + } > + > + /* Save old value of CSSELR_EL1 */ > + cache_sel_tmp = cache_sel; > + > + /* Get LLC index */ > + if ( ((cache_global_info >> CTR_CTYPE2_SHIFT) & CTR_CTYPEn_MASK) > + == CTR_LLC_ON ) I don't understand this check. You define CTR_LLC_ON to 1 << 2. So it would be 0b10. From the field you checked, this value mean "Data Cache Only". How is this indicating the which level to chose? But then in patch #4 you wrote we will do cache coloring on L2. So why are we selecting L3? > + cache_sel = CTR_SELECT_L2; > + else > + cache_sel = CTR_SELECT_L3; > + > + printk(XENLOG_INFO "LLC selection: %u\n", cache_sel); > + /* Select the correct LLC in CSSELR_EL1 */ > + WRITE_SYSREG64(cache_sel, CSSELR_EL1); This should be WRITE_SYSREG(). > + > + /* Ensure write */ > + isb(); > + > + /* Get info about the LLC */ > + cache_info = READ_SYSREG64(CCSIDR_EL1); > + > + /* ARM TRM: (Log2(Number of bytes in cache line)) - 4. */ From my understanding "TRM" in the Arm world refers to a specific processor. In this case we want to quote the spec. So we usually say "Arm Arm". > + cache_line_size = 1 << ((cache_info & CTR_LINESIZE_MASK) + 4); > + /* ARM TRM: (Number of sets in cache) - 1 */ > + cache_set_num = ((cache_info >> CTR_SIZE_SHIFT) & CTR_SIZE_MASK) + 1; The shifts here are assuming that FEAT_CCIDX is not implemented. I would be OK if we decide to not support cache coloring on such platform. However, we need to return an error if a user tries to use cache coloring on such platform. > + > + printk(XENLOG_INFO "Cache line size: %u bytes\n", cache_line_size); > + printk(XENLOG_INFO "Cache sets num: %u\n", cache_set_num); > + > + /* Restore value in CSSELR_EL1 */ > + WRITE_SYSREG64(cache_sel_tmp, CSSELR_EL1); > + > + /* Ensure write */ > + isb(); > + > + return (cache_line_size * cache_set_num); > +} > + > /************************* > * PARSING COLORING BOOTARGS > */ Cheers,
Hi Julien On 09/03/22 21:12, Julien Grall wrote: >> Signed-off-by: Luca Miccio <lucmiccio@gmail.com> >> Signed-off-by: Marco Solieri <marco.solieri@minervasys.tech> >> Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com> >> --- >> xen/arch/arm/coloring.c | 76 +++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 76 insertions(+) >> >> diff --git a/xen/arch/arm/coloring.c b/xen/arch/arm/coloring.c >> index 8f1cff6efb..e3d490b453 100644 >> --- a/xen/arch/arm/coloring.c >> +++ b/xen/arch/arm/coloring.c >> @@ -25,7 +25,10 @@ >> #include <xen/lib.h> >> #include <xen/errno.h> >> #include <xen/param.h> >> + > > > NIT: I think this belongs to patch #4. > >> +#include <asm/sysregs.h> > > Please order the include alphabetically. > >> #include <asm/coloring.h> > +#include <asm/io.h> > > You don't seem to use read*/write* helper. So why do you need this? >> /* Number of color(s) assigned to Xen */ >> static uint32_t xen_col_num; >> @@ -39,6 +42,79 @@ static uint32_t dom0_col_mask[MAX_COLORS_CELLS]; >> static uint64_t way_size; >> +#define CTR_LINESIZE_MASK 0x7 >> +#define CTR_SIZE_SHIFT 13 >> +#define CTR_SIZE_MASK 0x3FFF >> +#define CTR_SELECT_L2 1 << 1 >> +#define CTR_SELECT_L3 1 << 2 >> +#define CTR_CTYPEn_MASK 0x7 >> +#define CTR_CTYPE2_SHIFT 3 >> +#define CTR_CTYPE3_SHIFT 6 >> +#define CTR_LLC_ON 1 << 2 >> +#define CTR_LOC_SHIFT 24 >> +#define CTR_LOC_MASK 0x7 >> +#define CTR_LOC_L2 1 << 1 >> +#define CTR_LOC_NOT_IMPLEMENTED 1 << 0 > > We already define some CTR_* in processor.h. Please any extra one there. > >> + >> + >> +/* Return the way size of last level cache by asking the hardware */ >> +static uint64_t get_llc_way_size(void) > > This will break compilation as you are introducing get_llc_way_size() > but not using it. > > I would suggest to fold this patch in the next one. > >> +{ >> + uint32_t cache_sel = READ_SYSREG64(CSSELR_EL1); > > The return type for READ_SYSREG64() is uint64_t. That said, the > equivalent register on 32bit is CSSELR which is 32-bit. So this should > be READ_SYSREG() and the matching type is register_t. Since we don't want to support arm32, should I stick with READ_SYSREG64() or switch to the generic one you pointed me out? >> + uint32_t cache_global_info = READ_SYSREG64(CLIDR_EL1); > > Same remark here. Except the matching register is CLIDR. > >> + uint32_t cache_info; >> + uint32_t cache_line_size; >> + uint32_t cache_set_num; >> + uint32_t cache_sel_tmp; >> + >> + printk(XENLOG_INFO "Get information on LLC\n"); >> + printk(XENLOG_INFO "Cache CLIDR_EL1: 0x%"PRIx32"\n", >> cache_global_info); >> + >> + /* Check if at least L2 is implemented */ >> + if ( ((cache_global_info >> CTR_LOC_SHIFT) & CTR_LOC_MASK) > > This is a bit confusing. cache_global_info is storing CLIDR_* but you > are using macro starting with CTR_*. > > Did you intend to name the macros CLIDR_*? > > The same remark goes for the other use of CTR_ below. The name of the > macros should match the register they are meant to be used on. You are right for the naming mistakes. Should I add those defines in some specific file or can they stay here? >> + == CTR_LOC_NOT_IMPLEMENTED ) > > I am a bit confused this the check here. Shouln't you check that > Ctype2 is notn 0 instead? I should check a little bit better how this automatic probing thing actually works and we also have to clarify better what is the LLC for us, so that I know what we should really test for in this function. Probably you're right though. >> + { >> + printk(XENLOG_ERR "ERROR: L2 Cache not implemented\n"); >> + return 0; >> + } >> + >> + /* Save old value of CSSELR_EL1 */ >> + cache_sel_tmp = cache_sel; >> + >> + /* Get LLC index */ >> + if ( ((cache_global_info >> CTR_CTYPE2_SHIFT) & CTR_CTYPEn_MASK) >> + == CTR_LLC_ON ) > > I don't understand this check. You define CTR_LLC_ON to 1 << 2. So it > would be 0b10. From the field you checked, this value mean "Data Cache > Only". How is this indicating the which level to chose? > > But then in patch #4 you wrote we will do cache coloring on L2. So why > are we selecting L3? 1 << 2 is actually 0b100 which stands for "Unified cache". Still I don't know if this is the best way to test what we want. >> + cache_sel = CTR_SELECT_L2; >> + else >> + cache_sel = CTR_SELECT_L3; >> + >> + printk(XENLOG_INFO "LLC selection: %u\n", cache_sel); >> + /* Select the correct LLC in CSSELR_EL1 */ >> + WRITE_SYSREG64(cache_sel, CSSELR_EL1); > > This should be WRITE_SYSREG(). > >> + >> + /* Ensure write */ >> + isb(); >> + >> + /* Get info about the LLC */ >> + cache_info = READ_SYSREG64(CCSIDR_EL1); >> + >> + /* ARM TRM: (Log2(Number of bytes in cache line)) - 4. */ > > From my understanding "TRM" in the Arm world refers to a specific > processor. In this case we want to quote the spec. So we usually say > "Arm Arm". > >> + cache_line_size = 1 << ((cache_info & CTR_LINESIZE_MASK) + 4); >> + /* ARM TRM: (Number of sets in cache) - 1 */ >> + cache_set_num = ((cache_info >> CTR_SIZE_SHIFT) & CTR_SIZE_MASK) >> + 1; > > The shifts here are assuming that FEAT_CCIDX is not implemented. I > would be OK if we decide to not support cache coloring on such > platform. However, we need to return an error if a user tries to use > cache coloring on such platform. > In my understanding, if FEAT_CCIDX is implemented then CCSIDR_EL1 is a 64-bit register. So it's just a matter of probing for FEAT_CCIDX and in that case changing the way we access that register (since the layout changes too). Thanks. - Carlo Nonato
On 13/05/2022 15:34, Carlo Nonato wrote: > Hi Julien Hi, > On 09/03/22 21:12, Julien Grall wrote: >>> + >>> + >>> +/* Return the way size of last level cache by asking the hardware */ >>> +static uint64_t get_llc_way_size(void) >> >> This will break compilation as you are introducing get_llc_way_size() >> but not using it. >> >> I would suggest to fold this patch in the next one. >> >>> +{ >>> + uint32_t cache_sel = READ_SYSREG64(CSSELR_EL1); >> >> The return type for READ_SYSREG64() is uint64_t. That said, the >> equivalent register on 32bit is CSSELR which is 32-bit. So this should >> be READ_SYSREG() and the matching type is register_t. > Since we don't want to support arm32, should I stick with > READ_SYSREG64() or switch to the generic one you > pointed me out? If this code is meant to only work on 64-bit, then I would prefer if we use READ_SYSREG() and register_t (see more below about READ_SYSREG64 vs READ_SYSREG()). >>> + uint32_t cache_global_info = READ_SYSREG64(CLIDR_EL1); >> >> Same remark here. Except the matching register is CLIDR. >> >>> + uint32_t cache_info; >>> + uint32_t cache_line_size; >>> + uint32_t cache_set_num; >>> + uint32_t cache_sel_tmp; >>> + >>> + printk(XENLOG_INFO "Get information on LLC\n"); >>> + printk(XENLOG_INFO "Cache CLIDR_EL1: 0x%"PRIx32"\n", >>> cache_global_info); >>> + >>> + /* Check if at least L2 is implemented */ >>> + if ( ((cache_global_info >> CTR_LOC_SHIFT) & CTR_LOC_MASK) >> >> This is a bit confusing. cache_global_info is storing CLIDR_* but you >> are using macro starting with CTR_*. >> >> Did you intend to name the macros CLIDR_*? >> >> The same remark goes for the other use of CTR_ below. The name of the >> macros should match the register they are meant to be used on. > You are right for the naming mistakes. Should I add those defines in > some specific file or > can they stay here? I would define them in arch/arm/include/asm/processor.h where we already define all the mask for system registers. >>> + == CTR_LOC_NOT_IMPLEMENTED ) >> >> I am a bit confused this the check here. Shouln't you check that >> Ctype2 is notn 0 instead? > I should check a little bit better how this automatic probing thing > actually works > and we also have to clarify better what is the LLC for us, so that I > know what we > should really test for in this function. Probably you're right though. >>> + { >>> + printk(XENLOG_ERR "ERROR: L2 Cache not implemented\n"); >>> + return 0; >>> + } >>> + >>> + /* Save old value of CSSELR_EL1 */ >>> + cache_sel_tmp = cache_sel; >>> + >>> + /* Get LLC index */ >>> + if ( ((cache_global_info >> CTR_CTYPE2_SHIFT) & CTR_CTYPEn_MASK) >>> + == CTR_LLC_ON ) >> >> I don't understand this check. You define CTR_LLC_ON to 1 << 2. So it >> would be 0b10. From the field you checked, this value mean "Data Cache >> Only". How is this indicating the which level to chose? >> >> But then in patch #4 you wrote we will do cache coloring on L2. So why >> are we selecting L3? > 1 << 2 is actually 0b100 which stands for "Unified cache". Oh yes. Sorry, I miscalculated the field. > Still I don't > know if this is > the best way to test what we want. Would you be able to explain what you want to test? >>> + cache_sel = CTR_SELECT_L2; >>> + else >>> + cache_sel = CTR_SELECT_L3; >>> + >>> + printk(XENLOG_INFO "LLC selection: %u\n", cache_sel); >>> + /* Select the correct LLC in CSSELR_EL1 */ >>> + WRITE_SYSREG64(cache_sel, CSSELR_EL1); >> >> This should be WRITE_SYSREG(). >> >>> + >>> + /* Ensure write */ >>> + isb(); >>> + >>> + /* Get info about the LLC */ >>> + cache_info = READ_SYSREG64(CCSIDR_EL1); >>> + >>> + /* ARM TRM: (Log2(Number of bytes in cache line)) - 4. */ >> >> From my understanding "TRM" in the Arm world refers to a specific >> processor. In this case we want to quote the spec. So we usually say >> "Arm Arm". >> >>> + cache_line_size = 1 << ((cache_info & CTR_LINESIZE_MASK) + 4); >>> + /* ARM TRM: (Number of sets in cache) - 1 */ >>> + cache_set_num = ((cache_info >> CTR_SIZE_SHIFT) & CTR_SIZE_MASK) >>> + 1; >> >> The shifts here are assuming that FEAT_CCIDX is not implemented. I >> would be OK if we decide to not support cache coloring on such >> platform. However, we need to return an error if a user tries to use >> cache coloring on such platform. >> > In my understanding, if FEAT_CCIDX is implemented then CCSIDR_EL1 is a > 64-bit register. Technically all the system registers on arm64 are 64-bit registers. That said, earlier version of the Arm Arm suggested that some where 32-bit when in fact the top bits were RES0. In Xen, we should try to use register_t and READ_SYSREG() when using system register so we don't end up to mask the top by mistake (a future revision of the spec may define them). If the co-processor register is also 64-bit on 32-bit, then we should use register_t and READ_SYSREG64(). > So it's just a matter of probing for FEAT_CCIDX and in that case > changing the way we access > that register (since the layout changes too). Yes. I will review it if you want to implement it. But I am equally fine if you just want to add a check and return an error if FEAT_CCIDX is implemented. Cheers,
diff --git a/xen/arch/arm/coloring.c b/xen/arch/arm/coloring.c index 8f1cff6efb..e3d490b453 100644 --- a/xen/arch/arm/coloring.c +++ b/xen/arch/arm/coloring.c @@ -25,7 +25,10 @@ #include <xen/lib.h> #include <xen/errno.h> #include <xen/param.h> + +#include <asm/sysregs.h> #include <asm/coloring.h> +#include <asm/io.h> /* Number of color(s) assigned to Xen */ static uint32_t xen_col_num; @@ -39,6 +42,79 @@ static uint32_t dom0_col_mask[MAX_COLORS_CELLS]; static uint64_t way_size; +#define CTR_LINESIZE_MASK 0x7 +#define CTR_SIZE_SHIFT 13 +#define CTR_SIZE_MASK 0x3FFF +#define CTR_SELECT_L2 1 << 1 +#define CTR_SELECT_L3 1 << 2 +#define CTR_CTYPEn_MASK 0x7 +#define CTR_CTYPE2_SHIFT 3 +#define CTR_CTYPE3_SHIFT 6 +#define CTR_LLC_ON 1 << 2 +#define CTR_LOC_SHIFT 24 +#define CTR_LOC_MASK 0x7 +#define CTR_LOC_L2 1 << 1 +#define CTR_LOC_NOT_IMPLEMENTED 1 << 0 + + +/* Return the way size of last level cache by asking the hardware */ +static uint64_t get_llc_way_size(void) +{ + uint32_t cache_sel = READ_SYSREG64(CSSELR_EL1); + uint32_t cache_global_info = READ_SYSREG64(CLIDR_EL1); + uint32_t cache_info; + uint32_t cache_line_size; + uint32_t cache_set_num; + uint32_t cache_sel_tmp; + + printk(XENLOG_INFO "Get information on LLC\n"); + printk(XENLOG_INFO "Cache CLIDR_EL1: 0x%"PRIx32"\n", cache_global_info); + + /* Check if at least L2 is implemented */ + if ( ((cache_global_info >> CTR_LOC_SHIFT) & CTR_LOC_MASK) + == CTR_LOC_NOT_IMPLEMENTED ) + { + printk(XENLOG_ERR "ERROR: L2 Cache not implemented\n"); + return 0; + } + + /* Save old value of CSSELR_EL1 */ + cache_sel_tmp = cache_sel; + + /* Get LLC index */ + if ( ((cache_global_info >> CTR_CTYPE2_SHIFT) & CTR_CTYPEn_MASK) + == CTR_LLC_ON ) + cache_sel = CTR_SELECT_L2; + else + cache_sel = CTR_SELECT_L3; + + printk(XENLOG_INFO "LLC selection: %u\n", cache_sel); + /* Select the correct LLC in CSSELR_EL1 */ + WRITE_SYSREG64(cache_sel, CSSELR_EL1); + + /* Ensure write */ + isb(); + + /* Get info about the LLC */ + cache_info = READ_SYSREG64(CCSIDR_EL1); + + /* ARM TRM: (Log2(Number of bytes in cache line)) - 4. */ + cache_line_size = 1 << ((cache_info & CTR_LINESIZE_MASK) + 4); + /* ARM TRM: (Number of sets in cache) - 1 */ + cache_set_num = ((cache_info >> CTR_SIZE_SHIFT) & CTR_SIZE_MASK) + 1; + + printk(XENLOG_INFO "Cache line size: %u bytes\n", cache_line_size); + printk(XENLOG_INFO "Cache sets num: %u\n", cache_set_num); + + /* Restore value in CSSELR_EL1 */ + WRITE_SYSREG64(cache_sel_tmp, CSSELR_EL1); + + /* Ensure write */ + isb(); + + return (cache_line_size * cache_set_num); +} + /************************* * PARSING COLORING BOOTARGS */