[PATCH 0/3] x86/resctrl: Non-contiguous bitmasks in Intel CAT

[PATCH 0/3] x86/resctrl: Non-contiguous bitmasks in Intel CAT

[Wieczor-Retman, Maciej]

Until recently Intel CPUs didn't support using non-contiguous 1s
in Cache Allocation Technology (CAT). Writing a bitmask with
non-contiguous 1s to the resctrl schemata file would fail.

Intel CPUs that support non-contiguous 1s can be identified through a
CPUID leaf mentioned in the "Intel® Architecture Instruction Set
Extensions Programming Reference" document available at:
https://www.intel.com/content/www/us/en/developer/articles/technical/intel-sdm.html

Add kernel support for detecting if non-contiguous 1s in Cache
Allocation Technology (CAT) are supported by the hardware. Also add a
new resctrl FS file to output this information to the userspace.
Keep the hardcoded value for Haswell CPUs only since they do not have
CPUID enumeration support for Cache allocation.

Since the selftests/resctrl files are going through many rewrites and
cleanups the appropriate selftest is still a work in progress. For
basic selftesting capabilities use the bash script attached below this
paragraph. It checks whether various bitmasks written into resctrl FS
generate output consistent with reported feature support.

#!/bin/bash
# must be run as root, depends on a recent cpuid tool (20230406 or later)
# variables
RESCTRL_INFO="/sys/fs/resctrl/info"
L3_NON_CONT_VAL="${RESCTRL_INFO}/L3/sparse_bitmaps"
L2_NON_CONT_VAL="${RESCTRL_INFO}/L2/sparse_bitmaps"
L3_NON_CONT_CBM="${RESCTRL_INFO}/L3/cbm_mask"
L2_NON_CONT_CBM="${RESCTRL_INFO}/L2/cbm_mask"
L3_CPUID_CMD="cpuid -1 -l 0x10 -s 0x01"
L2_CPUID_CMD="cpuid -1 -l 0x10 -s 0x02"
PASSED_TESTS=0
L3_SUPPORT=0
L2_SUPPORT=0
TESTS=0

run_test() {
        # L2 or L3
        CACHE_LEVEL=$1
        CACHE_LEVEL_SUPPORT="${CACHE_LEVEL}_SUPPORT"
        echo "Checking ${RESCTRL_INFO}/${CACHE_LEVEL}..."
        if [[ -d "${RESCTRL_INFO}/${CACHE_LEVEL}" ]]; then
                eval "${CACHE_LEVEL_SUPPORT}=1"
                echo "${CACHE_LEVEL} CAT Feature is supported"
        else
                echo "${CACHE_LEVEL} CAT Feature is not supported"
        fi

        if [[ ${!CACHE_LEVEL_SUPPORT} -eq 1 ]]; then
                echo " --- Running tests for ${CACHE_LEVEL} CAT ---"

                # read sysfs entries
                # are non-contiguous cbm supported? (driver sysfs)
                eval "NON_CONT_VAL=${CACHE_LEVEL}_NON_CONT_VAL"
                eval "NON_CONT_FEAT=$( cat ${!NON_CONT_VAL} )"

                # are non-contiguous cbm supported? (cpuid)
                CACHE_CPUID_CMD="${CACHE_LEVEL}_CPUID_CMD"
                NONCONT_CPUID=$(${!CACHE_CPUID_CMD} | grep non-contiguous | grep true)
                NONCONT_CPUID_RET=$(( !$? ))

                # what is the mask size?
                eval "NON_CONT_CBM=${CACHE_LEVEL}_NON_CONT_CBM"
                MAX_MASK=$(( 16#$( cat ${!NON_CONT_CBM} ) ))

                # prepare contiguous and non-contiguous masks for tests
                BC_STRING="l(${MAX_MASK})/l(2)"
                MAX_MASK_BIT_COUNT=$(echo ${BC_STRING} | bc -l)
                MAX_MASK_BIT_COUNT=$(printf "%.0f" "$MAX_MASK_BIT_COUNT")
                BITSHIFT=$(( $MAX_MASK_BIT_COUNT/2 - ($MAX_MASK_BIT_COUNT/2 % 4) ))
                CONT_MASK=$(( $MAX_MASK >> $BITSHIFT ))
                NONCONT_MASK=$(( ~( $MAX_MASK & ( 15<<$BITSHIFT) ) ))
                NONCONT_MASK=$(( $NONCONT_MASK & $MAX_MASK ))

                # test if cpuid reported support matches the sysfs one
                echo " * Testing if CPUID matches ${CACHE_LEVEL}/sparse_bitmaps..."
                TESTS=$((TESTS + 1))
                if [[ $NONCONT_CPUID_RET -eq $NON_CONT_FEAT ]]; then
                        PASSED_TESTS=$((PASSED_TESTS + 1))
                        echo "There is a match!"
                else
                        echo "Error - no match!"
                fi

                # test by writing CBMs to the schemata
                printf " * Writing 0x%x mask to the schemata...\n" ${CONT_MASK}
                TESTS=$((TESTS + 1))
                SCHEMATA=$(printf "${CACHE_LEVEL}:0=%x" $CONT_MASK)
                echo "$SCHEMATA" > /sys/fs/resctrl/schemata
                if [[ $? -eq 0 ]]; then
                        PASSED_TESTS=$((PASSED_TESTS + 1))
                        echo "Contiguous ${CACHE_LEVEL} write correct!"
                else
                        echo "Contiguous ${CACHE_LEVEL} write ERROR!"
                fi

                printf " * Writing 0x%x mask to the schemata...\n" ${NONCONT_MASK}
                TESTS=$((TESTS + 1))
                SCHEMATA=$(printf "${CACHE_LEVEL}:0=%x" $NONCONT_MASK)
                echo "$SCHEMATA" > /sys/fs/resctrl/schemata
                if [[ (($? -eq 0) && ($NON_CONT_FEAT -eq 1)) || \
                        (($? -ne 0) && ($NON_CONT_FEAT -eq 0)) ]]; then
                        PASSED_TESTS=$((PASSED_TESTS + 1))
                        echo "Non-contiguous ${CACHE_LEVEL} write correct!"
                else
                        echo "Non-contiguous ${CACHE_LEVEL} write ERROR!"
                fi
        fi
}

# mount resctrl
mount -t resctrl resctrl /sys/fs/resctrl

run_test L3
run_test L2

echo "TESTS PASSED / ALL TESTS : ${PASSED_TESTS} / ${TESTS}"

# unmount resctrl
umount /sys/fs/resctrl

Fenghua Yu (2):
  x86/resctrl: Add sparse_bitmaps file in info
  Documentation/x86: Document resctrl's new sparse_bitmaps

Wieczor-Retman, Maciej (1):
  x86/resctrl: Enable non-contiguous bits in Intel CAT

 Documentation/arch/x86/resctrl.rst        | 16 ++++++++++++----
 arch/x86/kernel/cpu/resctrl/core.c        |  9 ++++++---
 arch/x86/kernel/cpu/resctrl/ctrlmondata.c | 11 +++++++----
 arch/x86/kernel/cpu/resctrl/internal.h    |  9 +++++++++
 arch/x86/kernel/cpu/resctrl/rdtgroup.c    | 18 ++++++++++++++++++
 5 files changed, 52 insertions(+), 11 deletions(-)

-- 
2.42.0

[PATCH 1/3] x86/resctrl: Enable non-contiguous bits in Intel CAT

[Wieczor-Retman, Maciej]

The setting for non-contiguous 1s support in Intel CAT is
hardcoded to false. On these systems, writing non-contiguous
1s into the schemata file will fail before resctrl passes
the value to the hardware.

In Intel CAT CPUID.0x10.1:ECX[3] and CPUID.0x10.2:ECX[3] stopped
being reserved and now carry information about non-contiguous 1s
value support for L3 and L2 cache respectively. The CAT
capacity bitmask (CBM) supports a non-contiguous 1s value if
the bit is set.

Replace the hardcoded non-contiguous support value with
the support learned from the hardware. Add hardcoded non-contiguous
support value to Haswell probe since it can't make use of CPUID for
Cache allocation.

Originally-by: Fenghua Yu <fenghua.yu@intel.com>
Signed-off-by: Wieczor-Retman, Maciej <maciej.wieczor-retman@intel.com>
---
 arch/x86/kernel/cpu/resctrl/core.c        |  9 ++++++---
 arch/x86/kernel/cpu/resctrl/ctrlmondata.c | 11 +++++++----
 arch/x86/kernel/cpu/resctrl/internal.h    |  9 +++++++++
 3 files changed, 22 insertions(+), 7 deletions(-)

diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
index 030d3b409768..c783a873147c 100644
--- a/arch/x86/kernel/cpu/resctrl/core.c
+++ b/arch/x86/kernel/cpu/resctrl/core.c
@@ -152,6 +152,7 @@ static inline void cache_alloc_hsw_probe(void)
 	r->cache.cbm_len = 20;
 	r->cache.shareable_bits = 0xc0000;
 	r->cache.min_cbm_bits = 2;
+	r->cache.arch_has_sparse_bitmaps = false;
 	r->alloc_capable = true;
 
 	rdt_alloc_capable = true;
@@ -267,15 +268,18 @@ static void rdt_get_cache_alloc_cfg(int idx, struct rdt_resource *r)
 {
 	struct rdt_hw_resource *hw_res = resctrl_to_arch_res(r);
 	union cpuid_0x10_1_eax eax;
+	union cpuid_0x10_x_ecx ecx;
 	union cpuid_0x10_x_edx edx;
-	u32 ebx, ecx;
+	u32 ebx;
 
-	cpuid_count(0x00000010, idx, &eax.full, &ebx, &ecx, &edx.full);
+	cpuid_count(0x00000010, idx, &eax.full, &ebx, &ecx.full, &edx.full);
 	hw_res->num_closid = edx.split.cos_max + 1;
 	r->cache.cbm_len = eax.split.cbm_len + 1;
 	r->default_ctrl = BIT_MASK(eax.split.cbm_len + 1) - 1;
 	r->cache.shareable_bits = ebx & r->default_ctrl;
 	r->data_width = (r->cache.cbm_len + 3) / 4;
+	if (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL)
+		r->cache.arch_has_sparse_bitmaps = ecx.split.noncont;
 	r->alloc_capable = true;
 }
 
@@ -872,7 +876,6 @@ static __init void rdt_init_res_defs_intel(void)
 
 		if (r->rid == RDT_RESOURCE_L3 ||
 		    r->rid == RDT_RESOURCE_L2) {
-			r->cache.arch_has_sparse_bitmaps = false;
 			r->cache.arch_has_per_cpu_cfg = false;
 			r->cache.min_cbm_bits = 1;
 		} else if (r->rid == RDT_RESOURCE_MBA) {
diff --git a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
index b44c487727d4..782e2700290b 100644
--- a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
+++ b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
@@ -87,10 +87,13 @@ int parse_bw(struct rdt_parse_data *data, struct resctrl_schema *s,
 
 /*
  * Check whether a cache bit mask is valid.
- * For Intel the SDM says:
- *	Please note that all (and only) contiguous '1' combinations
- *	are allowed (e.g. FFFFH, 0FF0H, 003CH, etc.).
- * Additionally Haswell requires at least two bits set.
+ * On Intel CPUs, non-contiguous 1s value support is indicated by CPUID:
+ *   - CPUID.0x10.1:ECX[3]: L3 non-contiguous 1s value supported if 1
+ *   - CPUID.0x10.2:ECX[3]: L2 non-contiguous 1s value supported if 1
+ *
+ * Additionally Haswell requires at least two bits set. Since it does not
+ * have CPUID enumeration support for Cache allocation the non-contiguous 1s
+ * value support is disabled.
  * AMD allows non-contiguous bitmasks.
  */
 static bool cbm_validate(char *buf, u32 *data, struct rdt_resource *r)
diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
index 85ceaf9a31ac..c47ef2f13e8e 100644
--- a/arch/x86/kernel/cpu/resctrl/internal.h
+++ b/arch/x86/kernel/cpu/resctrl/internal.h
@@ -492,6 +492,15 @@ union cpuid_0x10_3_eax {
 	unsigned int full;
 };
 
+/* CPUID.(EAX=10H, ECX=ResID).ECX */
+union cpuid_0x10_x_ecx {
+	struct {
+		unsigned int reserved:3;
+		unsigned int noncont:1;
+	} split;
+	unsigned int full;
+};
+
 /* CPUID.(EAX=10H, ECX=ResID).EDX */
 union cpuid_0x10_x_edx {
 	struct {
-- 
2.42.0

[PATCH 2/3] x86/resctrl: Add sparse_bitmaps file in info

[Wieczor-Retman, Maciej]

From: Fenghua Yu <fenghua.yu@intel.com>

Add the interface in resctrl FS to show if sparse CAT bitmaps are
supported on the platform. Reading the file returns either a "1" if
non-contiguous 1s are supported and "0" otherwise. The file path is
/sys/fs/resctrl/info/{resource}/sparse_bitmaps, where {resource} can be
either "L2" or "L3" depending on their support in the CAT feature.

Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
Signed-off-by: Wieczor-Retman, Maciej <maciej.wieczor-retman@intel.com>
---
 arch/x86/kernel/cpu/resctrl/rdtgroup.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index 725344048f85..4d27354f3f30 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -895,6 +895,17 @@ static int rdt_shareable_bits_show(struct kernfs_open_file *of,
 	return 0;
 }
 
+static int rdt_has_sparse_bitmaps_show(struct kernfs_open_file *of,
+				       struct seq_file *seq, void *v)
+{
+	struct resctrl_schema *s = of->kn->parent->priv;
+	struct rdt_resource *r = s->res;
+
+	seq_printf(seq, "%u\n", r->cache.arch_has_sparse_bitmaps);
+
+	return 0;
+}
+
 /**
  * rdt_bit_usage_show - Display current usage of resources
  *
@@ -1839,6 +1850,13 @@ static struct rftype res_common_files[] = {
 		.seq_show	= rdtgroup_size_show,
 		.fflags		= RF_CTRL_BASE,
 	},
+	{
+		.name		= "sparse_bitmaps",
+		.mode		= 0444,
+		.kf_ops		= &rdtgroup_kf_single_ops,
+		.seq_show	= rdt_has_sparse_bitmaps_show,
+		.fflags		= RF_CTRL_INFO | RFTYPE_RES_CACHE,
+	},
 
 };
 
-- 
2.42.0

[PATCH 3/3] Documentation/x86: Document resctrl's new sparse_bitmaps

[Wieczor-Retman, Maciej]

From: Fenghua Yu <fenghua.yu@intel.com>

The documentation mentions that non-contiguous bitmasks are not
supported in Intel Cache Allocation Technology (CAT).

Update the documentation on how to determine if sparse bitmasks are
allowed in L2 and L3 CAT.

Mention the file with feature support information is located in
the /sys/fs/resctrl/info/{resource}/ directories and enumerate what
are the possible outputs on file read operation.

Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
Signed-off-by: Wieczor-Retman, Maciej <maciej.wieczor-retman@intel.com>
---
 Documentation/arch/x86/resctrl.rst | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/Documentation/arch/x86/resctrl.rst b/Documentation/arch/x86/resctrl.rst
index cb05d90111b4..38694fc5800c 100644
--- a/Documentation/arch/x86/resctrl.rst
+++ b/Documentation/arch/x86/resctrl.rst
@@ -124,6 +124,13 @@ related to allocation:
 			"P":
 			      Corresponding region is pseudo-locked. No
 			      sharing allowed.
+"sparse_bitmaps":
+		Indicates if non-contiguous 1s value in CBM is supported.
+
+			"0":
+			      Only contiguous 1s value in CBM is supported.
+			"1":
+			      Non-contiguous 1s value in CBM is supported.
 
 Memory bandwidth(MB) subdirectory contains the following files
 with respect to allocation:
@@ -445,12 +452,13 @@ For cache resources we describe the portion of the cache that is available
 for allocation using a bitmask. The maximum value of the mask is defined
 by each cpu model (and may be different for different cache levels). It
 is found using CPUID, but is also provided in the "info" directory of
-the resctrl file system in "info/{resource}/cbm_mask". Intel hardware
+the resctrl file system in "info/{resource}/cbm_mask". Some Intel hardware
 requires that these masks have all the '1' bits in a contiguous block. So
 0x3, 0x6 and 0xC are legal 4-bit masks with two bits set, but 0x5, 0x9
-and 0xA are not.  On a system with a 20-bit mask each bit represents 5%
-of the capacity of the cache. You could partition the cache into four
-equal parts with masks: 0x1f, 0x3e0, 0x7c00, 0xf8000.
+and 0xA are not. Check /sys/fs/resctrl/info/{resource}/sparse_bitmaps
+if non-contiguous 1s value is supported. On a system with a 20-bit mask
+each bit represents 5% of the capacity of the cache. You could partition
+the cache into four equal parts with masks: 0x1f, 0x3e0, 0x7c00, 0xf8000.
 
 Memory bandwidth Allocation and monitoring
 ==========================================
-- 
2.42.0

Re: [PATCH 1/3] x86/resctrl: Enable non-contiguous bits in Intel CAT

[Reinette Chatre]

Hi Maciej,

On 9/1/2023 1:55 AM, Wieczor-Retman, Maciej wrote:
> The setting for non-contiguous 1s support in Intel CAT is
> hardcoded to false. On these systems, writing non-contiguous
> 1s into the schemata file will fail before resctrl passes
> the value to the hardware.
> 
> In Intel CAT CPUID.0x10.1:ECX[3] and CPUID.0x10.2:ECX[3] stopped
> being reserved and now carry information about non-contiguous 1s
> value support for L3 and L2 cache respectively. The CAT
> capacity bitmask (CBM) supports a non-contiguous 1s value if
> the bit is set.
> 
> Replace the hardcoded non-contiguous support value with
> the support learned from the hardware. Add hardcoded non-contiguous
> support value to Haswell probe since it can't make use of CPUID for
> Cache allocation.
> 
> Originally-by: Fenghua Yu <fenghua.yu@intel.com>
> Signed-off-by: Wieczor-Retman, Maciej <maciej.wieczor-retman@intel.com>
> ---
>  arch/x86/kernel/cpu/resctrl/core.c        |  9 ++++++---
>  arch/x86/kernel/cpu/resctrl/ctrlmondata.c | 11 +++++++----
>  arch/x86/kernel/cpu/resctrl/internal.h    |  9 +++++++++
>  3 files changed, 22 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
> index 030d3b409768..c783a873147c 100644
> --- a/arch/x86/kernel/cpu/resctrl/core.c
> +++ b/arch/x86/kernel/cpu/resctrl/core.c
> @@ -152,6 +152,7 @@ static inline void cache_alloc_hsw_probe(void)
>  	r->cache.cbm_len = 20;
>  	r->cache.shareable_bits = 0xc0000;
>  	r->cache.min_cbm_bits = 2;
> +	r->cache.arch_has_sparse_bitmaps = false;
>  	r->alloc_capable = true;
>  
>  	rdt_alloc_capable = true;
> @@ -267,15 +268,18 @@ static void rdt_get_cache_alloc_cfg(int idx, struct rdt_resource *r)
>  {
>  	struct rdt_hw_resource *hw_res = resctrl_to_arch_res(r);
>  	union cpuid_0x10_1_eax eax;
> +	union cpuid_0x10_x_ecx ecx;
>  	union cpuid_0x10_x_edx edx;
> -	u32 ebx, ecx;
> +	u32 ebx;
>  
> -	cpuid_count(0x00000010, idx, &eax.full, &ebx, &ecx, &edx.full);
> +	cpuid_count(0x00000010, idx, &eax.full, &ebx, &ecx.full, &edx.full);
>  	hw_res->num_closid = edx.split.cos_max + 1;
>  	r->cache.cbm_len = eax.split.cbm_len + 1;
>  	r->default_ctrl = BIT_MASK(eax.split.cbm_len + 1) - 1;
>  	r->cache.shareable_bits = ebx & r->default_ctrl;
>  	r->data_width = (r->cache.cbm_len + 3) / 4;
> +	if (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL)
> +		r->cache.arch_has_sparse_bitmaps = ecx.split.noncont;
>  	r->alloc_capable = true;
>  }
>  
> @@ -872,7 +876,6 @@ static __init void rdt_init_res_defs_intel(void)
>  
>  		if (r->rid == RDT_RESOURCE_L3 ||
>  		    r->rid == RDT_RESOURCE_L2) {
> -			r->cache.arch_has_sparse_bitmaps = false;
>  			r->cache.arch_has_per_cpu_cfg = false;
>  			r->cache.min_cbm_bits = 1;
>  		} else if (r->rid == RDT_RESOURCE_MBA) {
> diff --git a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
> index b44c487727d4..782e2700290b 100644
> --- a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
> +++ b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
> @@ -87,10 +87,13 @@ int parse_bw(struct rdt_parse_data *data, struct resctrl_schema *s,
>  
>  /*
>   * Check whether a cache bit mask is valid.
> - * For Intel the SDM says:
> - *	Please note that all (and only) contiguous '1' combinations
> - *	are allowed (e.g. FFFFH, 0FF0H, 003CH, etc.).
> - * Additionally Haswell requires at least two bits set.
> + * On Intel CPUs, non-contiguous 1s value support is indicated by CPUID:
> + *   - CPUID.0x10.1:ECX[3]: L3 non-contiguous 1s value supported if 1
> + *   - CPUID.0x10.2:ECX[3]: L2 non-contiguous 1s value supported if 1
> + *
> + * Additionally Haswell requires at least two bits set. Since it does not
> + * have CPUID enumeration support for Cache allocation the non-contiguous 1s
> + * value support is disabled.

The second sentence can be misinterpreted. non-contiguous 1s value support is
disabled on Haswell because Haswell does not support non-contiguous 1s value,
not because it does not have CPUID enumeration for RDT.

>   * AMD allows non-contiguous bitmasks.

(connecting to comments in following patch on this topic ... notice how the above
comments use the term "bit mask" and "bitmasks")

>   */
>  static bool cbm_validate(char *buf, u32 *data, struct rdt_resource *r)
> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
> index 85ceaf9a31ac..c47ef2f13e8e 100644
> --- a/arch/x86/kernel/cpu/resctrl/internal.h
> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
> @@ -492,6 +492,15 @@ union cpuid_0x10_3_eax {
>  	unsigned int full;
>  };
>  
> +/* CPUID.(EAX=10H, ECX=ResID).ECX */
> +union cpuid_0x10_x_ecx {
> +	struct {
> +		unsigned int reserved:3;
> +		unsigned int noncont:1;
> +	} split;
> +	unsigned int full;
> +};
> +
>  /* CPUID.(EAX=10H, ECX=ResID).EDX */
>  union cpuid_0x10_x_edx {
>  	struct {

The core of the change looks good to me.

Reinette

Re: [PATCH 2/3] x86/resctrl: Add sparse_bitmaps file in info

[Reinette Chatre]

Hi Maciej,

On 9/1/2023 1:55 AM, Wieczor-Retman, Maciej wrote:
> From: Fenghua Yu <fenghua.yu@intel.com>
> 
> Add the interface in resctrl FS to show if sparse CAT bitmaps are

resctrl is intended to be a generic interface so modifying it
with a vendor specific change is not ok. This is not what the
patch is doing though so the changelog can just be modified to
not create the impression. Perhaps just:
 "sparse CAT bitmaps" -> "sparse cache allocation bit masks"

> supported on the platform. Reading the file returns either a "1" if
> non-contiguous 1s are supported and "0" otherwise. The file path is
> /sys/fs/resctrl/info/{resource}/sparse_bitmaps, where {resource} can be
> either "L2" or "L3" depending on their support in the CAT feature.

No CAT here. L2 and L3 are the hardcoded cache allocation resources
so "depending on their support in the CAT feature" can just be removed.

> 
> Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
> Signed-off-by: Wieczor-Retman, Maciej <maciej.wieczor-retman@intel.com>
> ---
>  arch/x86/kernel/cpu/resctrl/rdtgroup.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index 725344048f85..4d27354f3f30 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -895,6 +895,17 @@ static int rdt_shareable_bits_show(struct kernfs_open_file *of,
>  	return 0;
>  }
>  
> +static int rdt_has_sparse_bitmaps_show(struct kernfs_open_file *of,
> +				       struct seq_file *seq, void *v)
> +{
> +	struct resctrl_schema *s = of->kn->parent->priv;
> +	struct rdt_resource *r = s->res;
> +
> +	seq_printf(seq, "%u\n", r->cache.arch_has_sparse_bitmaps);
> +
> +	return 0;
> +}
> +
>  /**
>   * rdt_bit_usage_show - Display current usage of resources
>   *
> @@ -1839,6 +1850,13 @@ static struct rftype res_common_files[] = {
>  		.seq_show	= rdtgroup_size_show,
>  		.fflags		= RF_CTRL_BASE,
>  	},
> +	{
> +		.name		= "sparse_bitmaps",
> +		.mode		= 0444,
> +		.kf_ops		= &rdtgroup_kf_single_ops,
> +		.seq_show	= rdt_has_sparse_bitmaps_show,
> +		.fflags		= RF_CTRL_INFO | RFTYPE_RES_CACHE,
> +	},
>  
>  };
>  

I think it is essential to use consistent terminology. To help with
this I reviewed the resctrl documentation and found no mention of
"bitmap" ... yet many instances of bit mask and even a clear official
term of "Cache Bit Masks (CBM)". The user interface is thus already
established and using the term "bit mask". Looking through the
AMD and Intel specs I also only see "bit mask". I think "bitmap"
sneaked in via an Arm contribution as motivated by their spec's
use of the term "cache portion bitmap". Since "bit mask" is already
in the user documentation and also in the interface via "cbm_mask"
I'd prefer that we stick with "bit mask" in user interface instead
of creating fragmentation with a new term.

Considering this I'd like to propose "sparse_masks" to match existing
"cbm_mask".

Please review this series to be consistent in this regard. Note that
patch 2 refers to "bitmaps" and then patch 3 switches to "bitmasks"
... patch 3 already uses the term "sparse bitmasks".

I think that it may also help to add a patch to this series that
renames arch_has_sparse_bitmaps to arch_has_sparse_bitmasks.

Reinette

Re: [PATCH 1/3] x86/resctrl: Enable non-contiguous bits in Intel CAT

[Maciej Wieczór-Retman]

Hello,

On 2023-09-11 at 13:05:16 -0700, Reinette Chatre wrote:
>Hi Maciej,
>> diff --git a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
>> index b44c487727d4..782e2700290b 100644
>> --- a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
>> +++ b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
>> @@ -87,10 +87,13 @@ int parse_bw(struct rdt_parse_data *data, struct resctrl_schema *s,
>>  
>>  /*
>>   * Check whether a cache bit mask is valid.
>> - * For Intel the SDM says:
>> - *	Please note that all (and only) contiguous '1' combinations
>> - *	are allowed (e.g. FFFFH, 0FF0H, 003CH, etc.).
>> - * Additionally Haswell requires at least two bits set.
>> + * On Intel CPUs, non-contiguous 1s value support is indicated by CPUID:
>> + *   - CPUID.0x10.1:ECX[3]: L3 non-contiguous 1s value supported if 1
>> + *   - CPUID.0x10.2:ECX[3]: L2 non-contiguous 1s value supported if 1
>> + *
>> + * Additionally Haswell requires at least two bits set. Since it does not
>> + * have CPUID enumeration support for Cache allocation the non-contiguous 1s
>> + * value support is disabled.
>
>The second sentence can be misinterpreted. non-contiguous 1s value support is
>disabled on Haswell because Haswell does not support non-contiguous 1s value,
>not because it does not have CPUID enumeration for RDT.

Would phrasing it like this:

+ * Additionally Haswell requires at least two bits set. Since it does not
+ * allow the non-contiguous 1s for cache allocation the support is
+ * hardcoded to disabled.

be more sensible?

>>   * AMD allows non-contiguous bitmasks.
>
>(connecting to comments in following patch on this topic ... notice how the above
>comments use the term "bit mask" and "bitmasks")

I'll change it so the naming is unified, thanks.

>>   */
>>  static bool cbm_validate(char *buf, u32 *data, struct rdt_resource *r)
>> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
>> index 85ceaf9a31ac..c47ef2f13e8e 100644
>> --- a/arch/x86/kernel/cpu/resctrl/internal.h
>> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
>> @@ -492,6 +492,15 @@ union cpuid_0x10_3_eax {
>>  	unsigned int full;
>>  };
>>  
>> +/* CPUID.(EAX=10H, ECX=ResID).ECX */
>> +union cpuid_0x10_x_ecx {
>> +	struct {
>> +		unsigned int reserved:3;
>> +		unsigned int noncont:1;
>> +	} split;
>> +	unsigned int full;
>> +};
>> +
>>  /* CPUID.(EAX=10H, ECX=ResID).EDX */
>>  union cpuid_0x10_x_edx {
>>  	struct {
>
>The core of the change looks good to me.
>
>Reinette

Thanks for reviewing!

-- 
Kind regards
Maciej Wieczór-Retman

Re: [PATCH 2/3] x86/resctrl: Add sparse_bitmaps file in info

[Maciej Wieczór-Retman]

Hello,

On 2023-09-11 at 13:05:30 -0700, Reinette Chatre wrote:
>Hi Maciej,
>
>On 9/1/2023 1:55 AM, Wieczor-Retman, Maciej wrote:
>> From: Fenghua Yu <fenghua.yu@intel.com>
>> 
>> Add the interface in resctrl FS to show if sparse CAT bitmaps are
>
>resctrl is intended to be a generic interface so modifying it
>with a vendor specific change is not ok. This is not what the
>patch is doing though so the changelog can just be modified to
>not create the impression. Perhaps just:
> "sparse CAT bitmaps" -> "sparse cache allocation bit masks"

Okay, I'll change it, thanks.

>> supported on the platform. Reading the file returns either a "1" if
>> non-contiguous 1s are supported and "0" otherwise. The file path is
>> /sys/fs/resctrl/info/{resource}/sparse_bitmaps, where {resource} can be
>> either "L2" or "L3" depending on their support in the CAT feature.
>
>No CAT here. L2 and L3 are the hardcoded cache allocation resources
>so "depending on their support in the CAT feature" can just be removed.

I'll change it.

>> 
>> Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
>> Signed-off-by: Wieczor-Retman, Maciej <maciej.wieczor-retman@intel.com>
>> ---
>>  arch/x86/kernel/cpu/resctrl/rdtgroup.c | 18 ++++++++++++++++++
>>  1 file changed, 18 insertions(+)
>> 
>> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> index 725344048f85..4d27354f3f30 100644
>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> @@ -895,6 +895,17 @@ static int rdt_shareable_bits_show(struct kernfs_open_file *of,
>>  	return 0;
>>  }
>>  
>> +static int rdt_has_sparse_bitmaps_show(struct kernfs_open_file *of,
>> +				       struct seq_file *seq, void *v)
>> +{
>> +	struct resctrl_schema *s = of->kn->parent->priv;
>> +	struct rdt_resource *r = s->res;
>> +
>> +	seq_printf(seq, "%u\n", r->cache.arch_has_sparse_bitmaps);
>> +
>> +	return 0;
>> +}
>> +
>>  /**
>>   * rdt_bit_usage_show - Display current usage of resources
>>   *
>> @@ -1839,6 +1850,13 @@ static struct rftype res_common_files[] = {
>>  		.seq_show	= rdtgroup_size_show,
>>  		.fflags		= RF_CTRL_BASE,
>>  	},
>> +	{
>> +		.name		= "sparse_bitmaps",
>> +		.mode		= 0444,
>> +		.kf_ops		= &rdtgroup_kf_single_ops,
>> +		.seq_show	= rdt_has_sparse_bitmaps_show,
>> +		.fflags		= RF_CTRL_INFO | RFTYPE_RES_CACHE,
>> +	},
>>  
>>  };
>>  
>
>I think it is essential to use consistent terminology. To help with
>this I reviewed the resctrl documentation and found no mention of
>"bitmap" ... yet many instances of bit mask and even a clear official
>term of "Cache Bit Masks (CBM)". The user interface is thus already
>established and using the term "bit mask". Looking through the
>AMD and Intel specs I also only see "bit mask". I think "bitmap"
>sneaked in via an Arm contribution as motivated by their spec's
>use of the term "cache portion bitmap". Since "bit mask" is already
>in the user documentation and also in the interface via "cbm_mask"
>I'd prefer that we stick with "bit mask" in user interface instead
>of creating fragmentation with a new term.
>
>Considering this I'd like to propose "sparse_masks" to match existing
>"cbm_mask".
>
>Please review this series to be consistent in this regard. Note that
>patch 2 refers to "bitmaps" and then patch 3 switches to "bitmasks"
>... patch 3 already uses the term "sparse bitmasks".
>
>I think that it may also help to add a patch to this series that
>renames arch_has_sparse_bitmaps to arch_has_sparse_bitmasks.

Okay, I'll go through the patches and unify the names to "bit mask"
rather than "bitmap" etc.

>Reinette

-- 
Kind regards
Maciej Wieczór-Retman

Re: [PATCH 1/3] x86/resctrl: Enable non-contiguous bits in Intel CAT

[Reinette Chatre]

Hi Maciej,

On 9/11/2023 11:53 PM, Maciej Wieczór-Retman wrote:
> On 2023-09-11 at 13:05:16 -0700, Reinette Chatre wrote:
>> Hi Maciej,
>>> diff --git a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
>>> index b44c487727d4..782e2700290b 100644
>>> --- a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
>>> +++ b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
>>> @@ -87,10 +87,13 @@ int parse_bw(struct rdt_parse_data *data, struct resctrl_schema *s,
>>>  
>>>  /*
>>>   * Check whether a cache bit mask is valid.
>>> - * For Intel the SDM says:
>>> - *	Please note that all (and only) contiguous '1' combinations
>>> - *	are allowed (e.g. FFFFH, 0FF0H, 003CH, etc.).
>>> - * Additionally Haswell requires at least two bits set.
>>> + * On Intel CPUs, non-contiguous 1s value support is indicated by CPUID:
>>> + *   - CPUID.0x10.1:ECX[3]: L3 non-contiguous 1s value supported if 1
>>> + *   - CPUID.0x10.2:ECX[3]: L2 non-contiguous 1s value supported if 1
>>> + *
>>> + * Additionally Haswell requires at least two bits set. Since it does not
>>> + * have CPUID enumeration support for Cache allocation the non-contiguous 1s
>>> + * value support is disabled.
>>
>> The second sentence can be misinterpreted. non-contiguous 1s value support is
>> disabled on Haswell because Haswell does not support non-contiguous 1s value,
>> not because it does not have CPUID enumeration for RDT.
> 
> Would phrasing it like this:
> 
> + * Additionally Haswell requires at least two bits set. Since it does not
> + * allow the non-contiguous 1s for cache allocation the support is
> + * hardcoded to disabled.
> 
> be more sensible?
> 

I think the above would be sensible as a comment of cache_alloc_hsw_probe()
where the support is hardcoded but here the comment is just used to summarize
what the hardware supports.
Maybe just something like:
	Haswell does not support a non-contiguous 1s value and additionally
	requires at least two bits set.

Reinette