[PATCH 0/7] x86/resctrl : Support L3 Smart Data Cache Injection Allocation Enforcement (SDCIAE)

[PATCH 0/7] x86/resctrl : Support L3 Smart Data Cache Injection Allocation Enforcement (SDCIAE)

[Babu Moger]

This series adds the support for L3 Smart Data Cache Injection Allocation
Enforcement (SDCIAE) to resctrl infrastructure. 

Upcoming AMD hardware implements Smart Data Cache Injection (SDCI).
Smart Data Cache Injection (SDCI) is a mechanism that enables direct
insertion of data from I/O devices into the L3 cache. By directly caching
data from I/O devices rather than first storing the I/O data in DRAM, SDCI
reduces demands on DRAM bandwidth and reduces latency to the processor
consuming the I/O data. The SDCIAE (SDCI Allocation Enforcement) PQE
feature allows system software to limit the portion of the L3 cache used
for SDCI.

The feature details are documented in the APM listed below [1].
[1] AMD64 Architecture Programmer's Manual Volume 2: System Programming
Publication # 24593 Revision 3.41 section 19.4.7 L3 Smart Data Cache
Injection Allocation Enforcement (SDCIAE)
Link: https://bugzilla.kernel.org/show_bug.cgi?id=206537

The feature requires linux support of TPH (TLP Processing Hints).
The support is ongoing and patches are currently under review.
https://lore.kernel.org/lkml/20240717205511.2541693-2-wei.huang2@amd.com/

The patches are based on top of commit
ad1b4b6ed19f (tip/master) Merge branch into tip/master: 'x86/timers'

Babu Moger (7):
  x86/cpufeatures: Add support for L3 Smart Data Cache Injection
    Allocation Enforcement
  x86/resctrl: Add SDCIAE feature in the command line options
  x86/resctrl: Introduce sdciae_capable in rdt_resource
  x86/resctrl: Implement SDCIAE enable/disable
  x86/resctrl: Add interface to enable/disable SDCIAE
  x86/resctrl: Introduce interface to display SDCIAE Capacity Bit Masks
  x86/resctrl: Introduce interface to modify SDCIAE Capacity Bit Masks

 .../admin-guide/kernel-parameters.txt         |   2 +-
 Documentation/arch/x86/resctrl.rst            |  29 ++
 arch/x86/include/asm/cpufeatures.h            |   1 +
 arch/x86/include/asm/msr-index.h              |   1 +
 arch/x86/kernel/cpu/cpuid-deps.c              |   1 +
 arch/x86/kernel/cpu/resctrl/core.c            |  10 +
 arch/x86/kernel/cpu/resctrl/ctrlmondata.c     |   4 +-
 arch/x86/kernel/cpu/resctrl/internal.h        |  15 +
 arch/x86/kernel/cpu/resctrl/rdtgroup.c        | 298 ++++++++++++++++++
 arch/x86/kernel/cpu/scattered.c               |   1 +
 include/linux/resctrl.h                       |   2 +
 11 files changed, 361 insertions(+), 3 deletions(-)

-- 
2.34.1

[PATCH 1/7] x86/cpufeatures: Add support for L3 Smart Data Cache Injection Allocation Enforcement

[Babu Moger]

Smart Data Cache Injection (SDCI) is a mechanism that enables direct
insertion of data from I/O devices into the L3 cache. By directly caching
data from I/O devices rather than first storing the I/O data in DRAM,
SDCI reduces demands on DRAM bandwidth and reduces latency to the processor
consuming the I/O data.

The SDCIAE (SDCI Allocation Enforcement) PQE feature allows system software
to limit the portion of the L3 cache used for SDCI.

When enabled, SDCIAE forces all SDCI lines to be placed into the L3 cache
partitions identified by the highest-supported L3_MASK_n register where n
maximum supported CLOSID.

Add CPUID feature bit that can be used to configure SDCIAE.

The feature details are documented in APM listed below [1].
[1] AMD64 Architecture Programmer's Manual Volume 2: System Programming
Publication # 24593 Revision 3.41 section 19.4.7 L3 Smart Data Cache
Injection Allocation Enforcement (SDCIAE)

Link: https://bugzilla.kernel.org/show_bug.cgi?id=206537
Signed-off-by: Babu Moger <babu.moger@amd.com>
---
 arch/x86/include/asm/cpufeatures.h | 1 +
 arch/x86/kernel/cpu/cpuid-deps.c   | 1 +
 arch/x86/kernel/cpu/scattered.c    | 1 +
 3 files changed, 3 insertions(+)

diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
index dd4682857c12..5ca39431d423 100644
--- a/arch/x86/include/asm/cpufeatures.h
+++ b/arch/x86/include/asm/cpufeatures.h
@@ -473,6 +473,7 @@
 #define X86_FEATURE_CLEAR_BHB_HW	(21*32+ 3) /* BHI_DIS_S HW control enabled */
 #define X86_FEATURE_CLEAR_BHB_LOOP_ON_VMEXIT (21*32+ 4) /* Clear branch history at vmexit using SW loop */
 #define X86_FEATURE_FAST_CPPC		(21*32 + 5) /* AMD Fast CPPC */
+#define X86_FEATURE_SDCIAE		(21*32 + 6) /* "" L3 Smart Data Cache Injection Allocation Enforcement */
 
 /*
  * BUG word(s)
diff --git a/arch/x86/kernel/cpu/cpuid-deps.c b/arch/x86/kernel/cpu/cpuid-deps.c
index b7d9f530ae16..1ef42cc4cc75 100644
--- a/arch/x86/kernel/cpu/cpuid-deps.c
+++ b/arch/x86/kernel/cpu/cpuid-deps.c
@@ -70,6 +70,7 @@ static const struct cpuid_dep cpuid_deps[] = {
 	{ X86_FEATURE_CQM_MBM_LOCAL,		X86_FEATURE_CQM_LLC   },
 	{ X86_FEATURE_BMEC,			X86_FEATURE_CQM_MBM_TOTAL   },
 	{ X86_FEATURE_BMEC,			X86_FEATURE_CQM_MBM_LOCAL   },
+	{ X86_FEATURE_SDCIAE,			X86_FEATURE_RDT_A     },
 	{ X86_FEATURE_AVX512_BF16,		X86_FEATURE_AVX512VL  },
 	{ X86_FEATURE_AVX512_FP16,		X86_FEATURE_AVX512BW  },
 	{ X86_FEATURE_ENQCMD,			X86_FEATURE_XSAVES    },
diff --git a/arch/x86/kernel/cpu/scattered.c b/arch/x86/kernel/cpu/scattered.c
index c84c30188fdf..88f00575c9ff 100644
--- a/arch/x86/kernel/cpu/scattered.c
+++ b/arch/x86/kernel/cpu/scattered.c
@@ -49,6 +49,7 @@ static const struct cpuid_bit cpuid_bits[] = {
 	{ X86_FEATURE_MBA,		CPUID_EBX,  6, 0x80000008, 0 },
 	{ X86_FEATURE_SMBA,		CPUID_EBX,  2, 0x80000020, 0 },
 	{ X86_FEATURE_BMEC,		CPUID_EBX,  3, 0x80000020, 0 },
+	{ X86_FEATURE_SDCIAE,		CPUID_EBX,  6, 0x80000020, 0 },
 	{ X86_FEATURE_PERFMON_V2,	CPUID_EAX,  0, 0x80000022, 0 },
 	{ X86_FEATURE_AMD_LBR_V2,	CPUID_EAX,  1, 0x80000022, 0 },
 	{ X86_FEATURE_AMD_LBR_PMC_FREEZE,	CPUID_EAX,  2, 0x80000022, 0 },
-- 
2.34.1

[PATCH 2/7] x86/resctrl: Add SDCIAE feature in the command line options

[Babu Moger]

Add the command line options to enable or disable the new resctrl feature
L3 Smart Data Cache Injection Allocation Enforcement (SDCIAE).

Signed-off-by: Babu Moger <babu.moger@amd.com>
---
 Documentation/admin-guide/kernel-parameters.txt | 2 +-
 arch/x86/kernel/cpu/resctrl/core.c              | 2 ++
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 09126bb8cc9f..63f17d23b8f4 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -5604,7 +5604,7 @@
 	rdt=		[HW,X86,RDT]
 			Turn on/off individual RDT features. List is:
 			cmt, mbmtotal, mbmlocal, l3cat, l3cdp, l2cat, l2cdp,
-			mba, smba, bmec.
+			mba, smba, bmec, sdciae.
 			E.g. to turn on cmt and turn off mba use:
 				rdt=cmt,!mba
 
diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
index 1930fce9dfe9..c4dfc768ddf5 100644
--- a/arch/x86/kernel/cpu/resctrl/core.c
+++ b/arch/x86/kernel/cpu/resctrl/core.c
@@ -801,6 +801,7 @@ enum {
 	RDT_FLAG_MBA,
 	RDT_FLAG_SMBA,
 	RDT_FLAG_BMEC,
+	RDT_FLAG_SDCIAE,
 };
 
 #define RDT_OPT(idx, n, f)	\
@@ -826,6 +827,7 @@ static struct rdt_options rdt_options[]  __initdata = {
 	RDT_OPT(RDT_FLAG_MBA,	    "mba",	X86_FEATURE_MBA),
 	RDT_OPT(RDT_FLAG_SMBA,	    "smba",	X86_FEATURE_SMBA),
 	RDT_OPT(RDT_FLAG_BMEC,	    "bmec",	X86_FEATURE_BMEC),
+	RDT_OPT(RDT_FLAG_SDCIAE,    "sdciae",	X86_FEATURE_SDCIAE),
 };
 #define NUM_RDT_OPTIONS ARRAY_SIZE(rdt_options)
 
-- 
2.34.1

[PATCH 3/7] x86/resctrl: Introduce sdciae_capable in rdt_resource

[Babu Moger]

Detect SDCIAE`(L3 Smart Data Cache Injection Allocation Enforcement)
feature and initialize sdciae_capable.

Signed-off-by: Babu Moger <babu.moger@amd.com>
---
 arch/x86/kernel/cpu/resctrl/core.c | 7 +++++++
 include/linux/resctrl.h            | 2 ++
 2 files changed, 9 insertions(+)

diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
index c4dfc768ddf5..e4381e3feb75 100644
--- a/arch/x86/kernel/cpu/resctrl/core.c
+++ b/arch/x86/kernel/cpu/resctrl/core.c
@@ -296,6 +296,11 @@ static void rdt_get_cdp_config(int level)
 	rdt_resources_all[level].r_resctrl.cdp_capable = true;
 }
 
+static void rdt_get_sdciae_alloc_cfg(struct rdt_resource *r)
+{
+	r->sdciae_capable = true;
+}
+
 static void rdt_get_cdp_l3_config(void)
 {
 	rdt_get_cdp_config(RDT_RESOURCE_L3);
@@ -921,6 +926,8 @@ static __init bool get_rdt_alloc_resources(void)
 		rdt_get_cache_alloc_cfg(1, r);
 		if (rdt_cpu_has(X86_FEATURE_CDP_L3))
 			rdt_get_cdp_l3_config();
+		if (rdt_cpu_has(X86_FEATURE_SDCIAE))
+			rdt_get_sdciae_alloc_cfg(r);
 		ret = true;
 	}
 	if (rdt_cpu_has(X86_FEATURE_CAT_L2)) {
diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
index b0875b99e811..281ba4fb8972 100644
--- a/include/linux/resctrl.h
+++ b/include/linux/resctrl.h
@@ -202,6 +202,7 @@ enum resctrl_scope {
  * @evt_list:		List of monitoring events
  * @fflags:		flags to choose base and info files
  * @cdp_capable:	Is the CDP feature available on this resource
+ * @sdciae_capable:	Is SDCIAE feature available on this resource
  */
 struct rdt_resource {
 	int			rid;
@@ -224,6 +225,7 @@ struct rdt_resource {
 	struct list_head	evt_list;
 	unsigned long		fflags;
 	bool			cdp_capable;
+	bool			sdciae_capable;
 };
 
 /**
-- 
2.34.1

[PATCH 4/7] x86/resctrl: Implement SDCIAE enable/disable

[Babu Moger]

SDCIAE feature can be enabled by setting bit 1 in MSR L3_QOS_EXT_CFG.
When the state of SDCIAE is changed, it must be changed to the updated
value on all logical processors in the QOS Domain. By default, the SDCIAE
feature is disabled.

Introduce arch handlers to detect and enable/disable the feature.

The SDCIAE feature details are available in APM listed below [1].
[1] AMD64 Architecture Programmer's Manual Volume 2: System Programming
Publication # 24593 Revision 3.41 section 19.4.7 L3 Smart Data Cache
Injection Allocation Enforcement (SDCIAE)

Signed-off-by: Babu Moger <babu.moger@amd.com>
Link: https://bugzilla.kernel.org/show_bug.cgi?id=206537
---
 arch/x86/include/asm/msr-index.h       |  1 +
 arch/x86/kernel/cpu/resctrl/internal.h | 12 +++++
 arch/x86/kernel/cpu/resctrl/rdtgroup.c | 61 ++++++++++++++++++++++++++
 3 files changed, 74 insertions(+)

diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
index 82c6a4d350e0..c78afed3c21f 100644
--- a/arch/x86/include/asm/msr-index.h
+++ b/arch/x86/include/asm/msr-index.h
@@ -1181,6 +1181,7 @@
 /* - AMD: */
 #define MSR_IA32_MBA_BW_BASE		0xc0000200
 #define MSR_IA32_SMBA_BW_BASE		0xc0000280
+#define MSR_IA32_L3_QOS_EXT_CFG		0xc00003ff
 #define MSR_IA32_EVT_CFG_BASE		0xc0000400
 
 /* MSR_IA32_VMX_MISC bits */
diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
index 955999aecfca..ceb0e8e1ed76 100644
--- a/arch/x86/kernel/cpu/resctrl/internal.h
+++ b/arch/x86/kernel/cpu/resctrl/internal.h
@@ -56,6 +56,9 @@
 /* Max event bits supported */
 #define MAX_EVT_CONFIG_BITS		GENMASK(6, 0)
 
+/* Setting bit 1 in L3_QOS_EXT_CFG enables the SDCIAE feature. */
+#define SDCIAE_ENABLE_BIT		1
+
 /**
  * cpumask_any_housekeeping() - Choose any CPU in @mask, preferring those that
  *			        aren't marked nohz_full
@@ -477,6 +480,7 @@ struct rdt_parse_data {
  * @mbm_cfg_mask:	Bandwidth sources that can be tracked when Bandwidth
  *			Monitoring Event Configuration (BMEC) is supported.
  * @cdp_enabled:	CDP state of this resource
+ * @sdciae_enabled:	SDCIAE feature is enabled
  *
  * Members of this structure are either private to the architecture
  * e.g. mbm_width, or accessed via helpers that provide abstraction. e.g.
@@ -491,6 +495,7 @@ struct rdt_hw_resource {
 	unsigned int		mbm_width;
 	unsigned int		mbm_cfg_mask;
 	bool			cdp_enabled;
+	bool			sdciae_enabled;
 };
 
 static inline struct rdt_hw_resource *resctrl_to_arch_res(struct rdt_resource *r)
@@ -536,6 +541,13 @@ int resctrl_arch_set_cdp_enabled(enum resctrl_res_level l, bool enable);
 
 void arch_mon_domain_online(struct rdt_resource *r, struct rdt_mon_domain *d);
 
+static inline bool resctrl_arch_get_sdciae_enabled(enum resctrl_res_level l)
+{
+	return rdt_resources_all[l].sdciae_enabled;
+}
+
+int resctrl_arch_set_sdciae_enabled(enum resctrl_res_level l, bool enable);
+
 /*
  * To return the common struct rdt_resource, which is contained in struct
  * rdt_hw_resource, walk the resctrl member of struct rdt_hw_resource.
diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index d7163b764c62..c62d6183bfe4 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -1789,6 +1789,67 @@ static ssize_t mbm_local_bytes_config_write(struct kernfs_open_file *of,
 	return ret ?: nbytes;
 }
 
+static void resctrl_sdciae_msrwrite(void *arg)
+{
+	bool *enable = arg;
+
+	if (*enable)
+		msr_set_bit(MSR_IA32_L3_QOS_EXT_CFG, SDCIAE_ENABLE_BIT);
+	else
+		msr_clear_bit(MSR_IA32_L3_QOS_EXT_CFG, SDCIAE_ENABLE_BIT);
+}
+
+static int resctrl_sdciae_setup(enum resctrl_res_level l, bool enable)
+{
+	struct rdt_resource *r = &rdt_resources_all[l].r_resctrl;
+	struct rdt_ctrl_domain *d;
+
+	/* Update  L3_QOS_EXT_CFG MSR on all the CPUs in all domains*/
+	list_for_each_entry(d, &r->ctrl_domains, hdr.list)
+		on_each_cpu_mask(&d->hdr.cpu_mask, resctrl_sdciae_msrwrite, &enable, 1);
+
+	return 0;
+}
+
+static int resctrl_sdciae_enable(enum resctrl_res_level l)
+{
+	struct rdt_hw_resource *hw_res = &rdt_resources_all[l];
+	int ret = 0;
+
+	if (!hw_res->sdciae_enabled) {
+		ret = resctrl_sdciae_setup(l, true);
+		if (!ret)
+			hw_res->sdciae_enabled = true;
+	}
+
+	return ret;
+}
+
+static void resctrl_sdciae_disable(enum resctrl_res_level l)
+{
+	struct rdt_hw_resource *hw_res = &rdt_resources_all[l];
+
+	if (hw_res->sdciae_enabled) {
+		resctrl_sdciae_setup(l, false);
+		hw_res->sdciae_enabled = false;
+	}
+}
+
+int resctrl_arch_set_sdciae_enabled(enum resctrl_res_level l, bool enable)
+{
+	struct rdt_hw_resource *hw_res = &rdt_resources_all[l];
+
+	if (!hw_res->r_resctrl.sdciae_capable)
+		return -EINVAL;
+
+	if (enable)
+		return resctrl_sdciae_enable(l);
+
+	resctrl_sdciae_disable(l);
+
+	return 0;
+}
+
 /* rdtgroup information files for one cache resource. */
 static struct rftype res_common_files[] = {
 	{
-- 
2.34.1

[PATCH 5/7] x86/resctrl: Add interface to enable/disable SDCIAE

[Babu Moger]

The SDCIAE (SDCI Allocation Enforcement) PQE feature allows system software
to configure the portion of the L3 cache used for SDCI.

When enabled, SDCIAE forces all SDCI lines to be placed into the L3 cache
partitions identified by the highest-supported L3_MASK_n register as
reported by CPUID Fn0000_0010_EDX_x1.MAX_COS. For example, if MAX_COS=15,
SDCI lines will be allocated into the L3 cache partitions determined by
the bitmask in the L3_MASK_15 register.

Introduce interface to enable/disable SDCIAE feature on user input.

Signed-off-by: Babu Moger <babu.moger@amd.com>
---
 Documentation/arch/x86/resctrl.rst     | 22 +++++++
 arch/x86/kernel/cpu/resctrl/core.c     |  1 +
 arch/x86/kernel/cpu/resctrl/internal.h |  1 +
 arch/x86/kernel/cpu/resctrl/rdtgroup.c | 88 ++++++++++++++++++++++++++
 4 files changed, 112 insertions(+)

diff --git a/Documentation/arch/x86/resctrl.rst b/Documentation/arch/x86/resctrl.rst
index a824affd741d..cb1532dd843f 100644
--- a/Documentation/arch/x86/resctrl.rst
+++ b/Documentation/arch/x86/resctrl.rst
@@ -135,6 +135,28 @@ related to allocation:
 			"1":
 			      Non-contiguous 1s value in CBM is supported.
 
+"sdciae":
+		Indicates if the system can support SDCIAE (L3 Smart Data Cache
+		Injection Allocation Enforcement) feature.
+
+		Smart Data Cache Injection (SDCI) is a mechanism that enables
+		direct insertion of data from I/O devices into the L3 cache.
+		By directly caching data from I/O devices rather than first
+		storing the I/O data in DRAM, SDCI reduces demands on DRAM
+		bandwidth and reduces latency to the processor consuming the
+		I/O data. The SDCIAE feature allows system software to configure
+		limit the portion of the L3 cache used for SDCI.
+
+			"0":
+			      Feature is not enabled.
+			"1":
+			      Feature is enabled.
+
+		Feature can be enabled/disabled by writing to the interface.
+		Example::
+
+			# echo 1 > /sys/fs/resctrl/info/L3/sdciae
+
 Memory bandwidth(MB) subdirectory contains the following files
 with respect to allocation:
 
diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
index e4381e3feb75..6a9512008a4a 100644
--- a/arch/x86/kernel/cpu/resctrl/core.c
+++ b/arch/x86/kernel/cpu/resctrl/core.c
@@ -299,6 +299,7 @@ static void rdt_get_cdp_config(int level)
 static void rdt_get_sdciae_alloc_cfg(struct rdt_resource *r)
 {
 	r->sdciae_capable = true;
+	resctrl_sdciae_rftype_init();
 }
 
 static void rdt_get_cdp_l3_config(void)
diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
index ceb0e8e1ed76..9a3da6d49144 100644
--- a/arch/x86/kernel/cpu/resctrl/internal.h
+++ b/arch/x86/kernel/cpu/resctrl/internal.h
@@ -662,6 +662,7 @@ void rdt_domain_reconfigure_cdp(struct rdt_resource *r);
 void __init thread_throttle_mode_init(void);
 void __init mbm_config_rftype_init(const char *config);
 void rdt_staged_configs_clear(void);
+void __init resctrl_sdciae_rftype_init(void);
 bool closid_allocated(unsigned int closid);
 int resctrl_find_cleanest_closid(void);
 
diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index c62d6183bfe4..58e4df195207 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -171,6 +171,27 @@ void closid_free(int closid)
 	__set_bit(closid, &closid_free_map);
 }
 
+/*
+ * SDCIAE feature uses max CLOSID to route the SDCI traffic.
+ * Get the max CLOSID number
+ */
+static u32 get_sdciae_closid(struct rdt_resource *r)
+{
+	return resctrl_arch_get_num_closid(r) - 1;
+}
+
+static int closid_alloc_sdciae(struct rdt_resource *r)
+{
+	u32 sdciae_closid = get_sdciae_closid(r);
+
+	if (closid_free_map & (1 << sdciae_closid)) {
+		closid_free_map &= ~(1 << sdciae_closid);
+		return sdciae_closid;
+	} else {
+		return -ENOSPC;
+	}
+}
+
 /**
  * closid_allocated - test if provided closid is in use
  * @closid: closid to be tested
@@ -1850,6 +1871,57 @@ int resctrl_arch_set_sdciae_enabled(enum resctrl_res_level l, bool enable)
 	return 0;
 }
 
+static int resctrl_sdciae_show(struct kernfs_open_file *of,
+			       struct seq_file *seq, void *v)
+{
+	seq_printf(seq, "%x\n", resctrl_arch_get_sdciae_enabled(RDT_RESOURCE_L3));
+	return 0;
+}
+
+static ssize_t resctrl_sdciae_write(struct kernfs_open_file *of, char *buf,
+				    size_t nbytes, loff_t off)
+{
+	struct resctrl_schema *s = of->kn->parent->priv;
+	struct rdt_resource *r = s->res;
+	unsigned int enable;
+	u32 sdciae_closid;
+	int ret;
+
+	if (!r->sdciae_capable)
+		return -EINVAL;
+
+	ret = kstrtouint(buf, 0, &enable);
+	if (ret)
+		return ret;
+
+	cpus_read_lock();
+	mutex_lock(&rdtgroup_mutex);
+
+	rdt_last_cmd_clear();
+
+	/* Update the MSR only when there is a change */
+	if (resctrl_arch_get_sdciae_enabled(RDT_RESOURCE_L3) != enable) {
+		if (enable) {
+			ret = closid_alloc_sdciae(r);
+			if (ret < 0) {
+				rdt_last_cmd_puts("SDCIAE CLOSID is not available\n");
+				goto out_sdciae;
+			}
+		} else {
+			sdciae_closid = get_sdciae_closid(r);
+			closid_free(sdciae_closid);
+		}
+
+		ret = resctrl_arch_set_sdciae_enabled(RDT_RESOURCE_L3, enable);
+	}
+
+out_sdciae:
+	mutex_unlock(&rdtgroup_mutex);
+	cpus_read_unlock();
+
+	return ret ?: nbytes;
+}
+
 /* rdtgroup information files for one cache resource. */
 static struct rftype res_common_files[] = {
 	{
@@ -2002,6 +2074,13 @@ static struct rftype res_common_files[] = {
 		.seq_show	= rdtgroup_schemata_show,
 		.fflags		= RFTYPE_CTRL_BASE,
 	},
+	{
+		.name		= "sdciae",
+		.mode		= 0644,
+		.kf_ops		= &rdtgroup_kf_single_ops,
+		.seq_show	= resctrl_sdciae_show,
+		.write		= resctrl_sdciae_write,
+	},
 	{
 		.name		= "mode",
 		.mode		= 0644,
@@ -2101,6 +2180,15 @@ void __init mbm_config_rftype_init(const char *config)
 		rft->fflags = RFTYPE_MON_INFO | RFTYPE_RES_CACHE;
 }
 
+void __init resctrl_sdciae_rftype_init(void)
+{
+	struct rftype *rft;
+
+	rft = rdtgroup_get_rftype_by_name("sdciae");
+	if (rft)
+		rft->fflags = RFTYPE_CTRL_INFO | RFTYPE_RES_CACHE;
+}
+
 /**
  * rdtgroup_kn_mode_restrict - Restrict user access to named resctrl file
  * @r: The resource group with which the file is associated.
-- 
2.34.1

[PATCH 6/7] x86/resctrl: Introduce interface to display SDCIAE Capacity Bit Masks

[Babu Moger]

When enabled, SDCIAE forces all SDCI lines to be placed into the L3 cache
partitions identified by the highest-supported L3_MASK_n register where
n is the maximum CLOSID supported.

Add the interface to display CBMs (Capacity Bit Mask) of the SDCIAE.

Signed-off-by: Babu Moger <babu.moger@amd.com>
---
 arch/x86/kernel/cpu/resctrl/ctrlmondata.c |  2 +-
 arch/x86/kernel/cpu/resctrl/internal.h    |  1 +
 arch/x86/kernel/cpu/resctrl/rdtgroup.c    | 29 +++++++++++++++++++++++
 3 files changed, 31 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
index 50fa1fe9a073..fc99f4d17e6c 100644
--- a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
+++ b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
@@ -439,7 +439,7 @@ u32 resctrl_arch_get_config(struct rdt_resource *r, struct rdt_ctrl_domain *d,
 	return hw_dom->ctrl_val[idx];
 }
 
-static void show_doms(struct seq_file *s, struct resctrl_schema *schema, int closid)
+void show_doms(struct seq_file *s, struct resctrl_schema *schema, int closid)
 {
 	struct rdt_resource *r = schema->res;
 	struct rdt_ctrl_domain *dom;
diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
index 9a3da6d49144..f2c87ca37b13 100644
--- a/arch/x86/kernel/cpu/resctrl/internal.h
+++ b/arch/x86/kernel/cpu/resctrl/internal.h
@@ -663,6 +663,7 @@ void __init thread_throttle_mode_init(void);
 void __init mbm_config_rftype_init(const char *config);
 void rdt_staged_configs_clear(void);
 void __init resctrl_sdciae_rftype_init(void);
+void show_doms(struct seq_file *s, struct resctrl_schema *schema, int closid);
 bool closid_allocated(unsigned int closid);
 int resctrl_find_cleanest_closid(void);
 
diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index 58e4df195207..51bc715bb6ae 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -1922,6 +1922,25 @@ static ssize_t resctrl_sdciae_write(struct kernfs_open_file *of, char *buf,
 	return ret ?: nbytes;
 }
 
+static int resctrl_sdciae_cbm_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;
+	u32 sdciae_closid;
+
+	if (!resctrl_arch_get_sdciae_enabled(RDT_RESOURCE_L3)) {
+		rdt_last_cmd_puts("SDCIAE is not enabled\n");
+		return -EINVAL;
+	}
+
+	sdciae_closid = get_sdciae_closid(r);
+
+	show_doms(seq, s, sdciae_closid);
+
+	return 0;
+}
+
 /* rdtgroup information files for one cache resource. */
 static struct rftype res_common_files[] = {
 	{
@@ -2081,6 +2100,12 @@ static struct rftype res_common_files[] = {
 		.seq_show	= resctrl_sdciae_show,
 		.write		= resctrl_sdciae_write,
 	},
+	{
+		.name		= "sdciae_cbm",
+		.mode		= 0444,
+		.kf_ops		= &rdtgroup_kf_single_ops,
+		.seq_show	= resctrl_sdciae_cbm_show,
+	},
 	{
 		.name		= "mode",
 		.mode		= 0644,
@@ -2187,6 +2212,10 @@ void __init resctrl_sdciae_rftype_init(void)
 	rft = rdtgroup_get_rftype_by_name("sdciae");
 	if (rft)
 		rft->fflags = RFTYPE_CTRL_INFO | RFTYPE_RES_CACHE;
+
+	rft = rdtgroup_get_rftype_by_name("sdciae_cbm");
+	if (rft)
+		rft->fflags = RFTYPE_CTRL_INFO | RFTYPE_RES_CACHE;
 }
 
 /**
-- 
2.34.1

[PATCH 7/7] x86/resctrl: Introduce interface to modify SDCIAE Capacity Bit Masks

[Babu Moger]

The SDCIAE (SDCI Allocation Enforcement) PQE feature allows system
software to limit the portion of the L3 cache used for SDCI.

Provide the interface to modify SDCIAE CBMs (Capacity Bit Masks).

Signed-off-by: Babu Moger <babu.moger@amd.com>
---
 Documentation/arch/x86/resctrl.rst        |   7 ++
 arch/x86/kernel/cpu/resctrl/ctrlmondata.c |   2 +-
 arch/x86/kernel/cpu/resctrl/internal.h    |   1 +
 arch/x86/kernel/cpu/resctrl/rdtgroup.c    | 122 +++++++++++++++++++++-
 4 files changed, 130 insertions(+), 2 deletions(-)

diff --git a/Documentation/arch/x86/resctrl.rst b/Documentation/arch/x86/resctrl.rst
index cb1532dd843f..33de17387980 100644
--- a/Documentation/arch/x86/resctrl.rst
+++ b/Documentation/arch/x86/resctrl.rst
@@ -157,6 +157,13 @@ related to allocation:
 
 			# echo 1 > /sys/fs/resctrl/info/L3/sdciae
 
+"sdciae_cbm":
+		Capacity Bit Mask (CBM) available to SDCIAE supported devices.
+		CBM can be configured by writing to the interface in the
+		following format::
+
+			L3:<cache_id0>=<cbm>;<cache_id1>=<cbm>;...
+
 Memory bandwidth(MB) subdirectory contains the following files
 with respect to allocation:
 
diff --git a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
index fc99f4d17e6c..cf39eee5fba9 100644
--- a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
+++ b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
@@ -97,7 +97,7 @@ int parse_bw(struct rdt_parse_data *data, struct resctrl_schema *s,
  * requires at least two bits set.
  * AMD allows non-contiguous bitmasks.
  */
-static bool cbm_validate(char *buf, u32 *data, struct rdt_resource *r)
+bool cbm_validate(char *buf, u32 *data, struct rdt_resource *r)
 {
 	unsigned long first_bit, zero_bit, val;
 	unsigned int cbm_len = r->cache.cbm_len;
diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
index f2c87ca37b13..66428950a326 100644
--- a/arch/x86/kernel/cpu/resctrl/internal.h
+++ b/arch/x86/kernel/cpu/resctrl/internal.h
@@ -664,6 +664,7 @@ void __init mbm_config_rftype_init(const char *config);
 void rdt_staged_configs_clear(void);
 void __init resctrl_sdciae_rftype_init(void);
 void show_doms(struct seq_file *s, struct resctrl_schema *schema, int closid);
+bool cbm_validate(char *buf, u32 *data, struct rdt_resource *r);
 bool closid_allocated(unsigned int closid);
 int resctrl_find_cleanest_closid(void);
 
diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index 51bc715bb6ae..247909461ab3 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -1941,6 +1941,125 @@ static int resctrl_sdciae_cbm_show(struct kernfs_open_file *of,
 	return 0;
 }
 
+/*
+ * Read the CBM and check the validity. Make sure CBM is not shared
+ * with any other exclusive resctrl groups.
+ */
+static int resctrl_parse_cbm(char *buf, struct resctrl_schema *s,
+			     struct rdt_ctrl_domain *d)
+{
+	struct resctrl_staged_config *cfg;
+	struct rdt_resource *r = s->res;
+	u32 sdciae_closid;
+	u32 cbm_val;
+
+	cfg = &d->staged_config[s->conf_type];
+	if (cfg->have_new_ctrl) {
+		rdt_last_cmd_printf("Duplicate domain %d\n", d->hdr.id);
+		return -EINVAL;
+	}
+
+	if (!cbm_validate(buf, &cbm_val, r))
+		return -EINVAL;
+
+	/*
+	 * The CBM may not overlap with other exclusive group.
+	 */
+	sdciae_closid = get_sdciae_closid(r);
+	if (rdtgroup_cbm_overlaps(s, d, cbm_val, sdciae_closid, true)) {
+		rdt_last_cmd_puts("Overlaps with exclusive group\n");
+		return -EINVAL;
+	}
+
+	cfg->new_ctrl = cbm_val;
+	cfg->have_new_ctrl = true;
+
+	return 0;
+}
+
+static int resctrl_parse_line(char *line,  struct rdt_resource *r,
+			      struct resctrl_schema *s)
+{
+	struct rdt_ctrl_domain *d;
+	char *dom = NULL, *id;
+	unsigned long dom_id;
+
+next:
+	if (!line || line[0] == '\0')
+		return 0;
+
+	dom = strsep(&line, ";");
+	id = strsep(&dom, "=");
+	if (!dom || kstrtoul(id, 10, &dom_id)) {
+		rdt_last_cmd_puts("Missing '=' or non-numeric domain\n");
+		return -EINVAL;
+	}
+
+	dom = strim(dom);
+	list_for_each_entry(d, &r->ctrl_domains, hdr.list) {
+		if (d->hdr.id == dom_id) {
+			if (resctrl_parse_cbm(dom, s, d))
+				return -EINVAL;
+			goto next;
+		}
+	}
+	return -EINVAL;
+}
+
+static ssize_t resctrl_sdciae_cbm_write(struct kernfs_open_file *of,
+					char *buf, size_t nbytes, loff_t off)
+{
+	struct resctrl_schema *s = of->kn->parent->priv;
+	struct rdt_resource *r = s->res;
+	u32 sdciae_closid;
+	char *resname;
+	int ret = 0;
+
+	/* Valid input requires a trailing newline */
+	if (nbytes == 0 || buf[nbytes - 1] != '\n')
+		return -EINVAL;
+
+	buf[nbytes - 1] = '\0';
+
+	cpus_read_lock();
+	mutex_lock(&rdtgroup_mutex);
+
+	rdt_last_cmd_clear();
+	rdt_staged_configs_clear();
+
+	resname = strim(strsep(&buf, ":"));
+	if (!buf) {
+		rdt_last_cmd_puts("Missing ':'\n");
+		ret = -EINVAL;
+		goto out;
+	}
+
+	if (strcmp(resname, "L3")) {
+		rdt_last_cmd_printf("Unsupported resource name '%s'\n", resname);
+		ret = -EINVAL;
+		goto out;
+	}
+
+	if (buf[0] == '\0') {
+		rdt_last_cmd_printf("Missing '%s' value\n", resname);
+		ret = -EINVAL;
+		goto out;
+	}
+
+	ret = resctrl_parse_line(buf, r, s);
+	if (ret)
+		goto out;
+
+	sdciae_closid = get_sdciae_closid(r);
+	ret = resctrl_arch_update_domains(r, sdciae_closid);
+
+out:
+	mutex_unlock(&rdtgroup_mutex);
+	cpus_read_unlock();
+
+	return ret ?: nbytes;
+}
+
 /* rdtgroup information files for one cache resource. */
 static struct rftype res_common_files[] = {
 	{
@@ -2102,9 +2221,10 @@ static struct rftype res_common_files[] = {
 	},
 	{
 		.name		= "sdciae_cbm",
-		.mode		= 0444,
+		.mode		= 0644,
 		.kf_ops		= &rdtgroup_kf_single_ops,
 		.seq_show	= resctrl_sdciae_cbm_show,
+		.write		= resctrl_sdciae_cbm_write,
 	},
 	{
 		.name		= "mode",
-- 
2.34.1

Re: [PATCH 1/7] x86/cpufeatures: Add support for L3 Smart Data Cache Injection Allocation Enforcement

[Borislav Petkov]

On Fri, Aug 16, 2024 at 11:16:18AM -0500, Babu Moger wrote:
> diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
> index dd4682857c12..5ca39431d423 100644
> --- a/arch/x86/include/asm/cpufeatures.h
> +++ b/arch/x86/include/asm/cpufeatures.h
> @@ -473,6 +473,7 @@
>  #define X86_FEATURE_CLEAR_BHB_HW	(21*32+ 3) /* BHI_DIS_S HW control enabled */
>  #define X86_FEATURE_CLEAR_BHB_LOOP_ON_VMEXIT (21*32+ 4) /* Clear branch history at vmexit using SW loop */
>  #define X86_FEATURE_FAST_CPPC		(21*32 + 5) /* AMD Fast CPPC */
> +#define X86_FEATURE_SDCIAE		(21*32 + 6) /* "" L3 Smart Data Cache Injection Allocation Enforcement */

The "" is not needed anymore - feature flags are automatically NOT added
to /proc/cpuinfo.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH 1/7] x86/cpufeatures: Add support for L3 Smart Data Cache Injection Allocation Enforcement

[Moger, Babu]

Hi Boris,

On 8/17/24 09:50, Borislav Petkov wrote:
> On Fri, Aug 16, 2024 at 11:16:18AM -0500, Babu Moger wrote:
>> diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
>> index dd4682857c12..5ca39431d423 100644
>> --- a/arch/x86/include/asm/cpufeatures.h
>> +++ b/arch/x86/include/asm/cpufeatures.h
>> @@ -473,6 +473,7 @@
>>  #define X86_FEATURE_CLEAR_BHB_HW	(21*32+ 3) /* BHI_DIS_S HW control enabled */
>>  #define X86_FEATURE_CLEAR_BHB_LOOP_ON_VMEXIT (21*32+ 4) /* Clear branch history at vmexit using SW loop */
>>  #define X86_FEATURE_FAST_CPPC		(21*32 + 5) /* AMD Fast CPPC */
>> +#define X86_FEATURE_SDCIAE		(21*32 + 6) /* "" L3 Smart Data Cache Injection Allocation Enforcement */
> 
> The "" is not needed anymore - feature flags are automatically NOT added
> to /proc/cpuinfo.

Sure. Will remove it in next revision.

-- 
Thanks
Babu Moger

Re: [PATCH 0/7] x86/resctrl : Support L3 Smart Data Cache Injection Allocation Enforcement (SDCIAE)

[Reinette Chatre]

Hi Babu,

On 8/16/24 9:16 AM, Babu Moger wrote:
> 
> This series adds the support for L3 Smart Data Cache Injection Allocation
> Enforcement (SDCIAE) to resctrl infrastructure.
> 
> Upcoming AMD hardware implements Smart Data Cache Injection (SDCI).
> Smart Data Cache Injection (SDCI) is a mechanism that enables direct
> insertion of data from I/O devices into the L3 cache. By directly caching
> data from I/O devices rather than first storing the I/O data in DRAM, SDCI
> reduces demands on DRAM bandwidth and reduces latency to the processor
> consuming the I/O data. The SDCIAE (SDCI Allocation Enforcement) PQE
> feature allows system software to limit the portion of the L3 cache used
> for SDCI.
> 

This series introduces new user interface. Could you please describe the
new user interface in the cover letter and how users are expected to interact
with this interface to be able to use this new feature? Please also describe
the impact on existing resctrl interfaces related to cache allocation from
I/O hardware, for example "shareable_bits", "bit_usage", etc. These existing
interfaces are used to communicate to user space how portions of cache are
used by I/O hardware but I cannot tell from this series how this work builds on
this.

How does this feature work with the existing "L3 Cache Allocation Sharing Mask"
that is enumerated as part of CAT feature?

> The feature details are documented in the APM listed below [1].
> [1] AMD64 Architecture Programmer's Manual Volume 2: System Programming
> Publication # 24593 Revision 3.41 section 19.4.7 L3 Smart Data Cache
> Injection Allocation Enforcement (SDCIAE)
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=206537
> 
> The feature requires linux support of TPH (TLP Processing Hints).
> The support is ongoing and patches are currently under review.
> https://lore.kernel.org/lkml/20240717205511.2541693-2-wei.huang2@amd.com/

Please note that the cover letter [1] of that series mentions "Cache Injection
allows PCIe endpoints to inject I/O Coherent DMA writes directly into an L2 ..."
while this series implements and refers to L3 only.

> 
> The patches are based on top of commit
> ad1b4b6ed19f (tip/master) Merge branch into tip/master: 'x86/timers'
> 

Reinette

[1] https://lore.kernel.org/lkml/20240717205511.2541693-1-wei.huang2@amd.com/

Re: [PATCH 1/7] x86/cpufeatures: Add support for L3 Smart Data Cache Injection Allocation Enforcement

[Reinette Chatre]

Hi Babu,

On 8/16/24 9:16 AM, Babu Moger wrote:
> Smart Data Cache Injection (SDCI) is a mechanism that enables direct
> insertion of data from I/O devices into the L3 cache. By directly caching
> data from I/O devices rather than first storing the I/O data in DRAM,
> SDCI reduces demands on DRAM bandwidth and reduces latency to the processor
> consuming the I/O data.
> 
> The SDCIAE (SDCI Allocation Enforcement) PQE feature allows system software
> to limit the portion of the L3 cache used for SDCI.
> 
> When enabled, SDCIAE forces all SDCI lines to be placed into the L3 cache
> partitions identified by the highest-supported L3_MASK_n register where n
> maximum supported CLOSID.

"where n maximum supported CLOSID" -> "where n is the maximum supported CLOSID" ?

> 
> Add CPUID feature bit that can be used to configure SDCIAE.
> 
> The feature details are documented in APM listed below [1].
> [1] AMD64 Architecture Programmer's Manual Volume 2: System Programming
> Publication # 24593 Revision 3.41 section 19.4.7 L3 Smart Data Cache
> Injection Allocation Enforcement (SDCIAE)
> 
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=206537
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---
>   arch/x86/include/asm/cpufeatures.h | 1 +
>   arch/x86/kernel/cpu/cpuid-deps.c   | 1 +
>   arch/x86/kernel/cpu/scattered.c    | 1 +
>   3 files changed, 3 insertions(+)
> 
> diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
> index dd4682857c12..5ca39431d423 100644
> --- a/arch/x86/include/asm/cpufeatures.h
> +++ b/arch/x86/include/asm/cpufeatures.h
> @@ -473,6 +473,7 @@
>   #define X86_FEATURE_CLEAR_BHB_HW	(21*32+ 3) /* BHI_DIS_S HW control enabled */
>   #define X86_FEATURE_CLEAR_BHB_LOOP_ON_VMEXIT (21*32+ 4) /* Clear branch history at vmexit using SW loop */
>   #define X86_FEATURE_FAST_CPPC		(21*32 + 5) /* AMD Fast CPPC */
> +#define X86_FEATURE_SDCIAE		(21*32 + 6) /* "" L3 Smart Data Cache Injection Allocation Enforcement */
>   
>   /*
>    * BUG word(s)
> diff --git a/arch/x86/kernel/cpu/cpuid-deps.c b/arch/x86/kernel/cpu/cpuid-deps.c
> index b7d9f530ae16..1ef42cc4cc75 100644
> --- a/arch/x86/kernel/cpu/cpuid-deps.c
> +++ b/arch/x86/kernel/cpu/cpuid-deps.c
> @@ -70,6 +70,7 @@ static const struct cpuid_dep cpuid_deps[] = {
>   	{ X86_FEATURE_CQM_MBM_LOCAL,		X86_FEATURE_CQM_LLC   },
>   	{ X86_FEATURE_BMEC,			X86_FEATURE_CQM_MBM_TOTAL   },
>   	{ X86_FEATURE_BMEC,			X86_FEATURE_CQM_MBM_LOCAL   },
> +	{ X86_FEATURE_SDCIAE,			X86_FEATURE_RDT_A     },

The need for this dependency is not clear to me. If there was a dependency
then I would have expected it to be X86_FEATURE_CAT_L3 but we have not
previously needed to do this. For example, X86_FEATURE_CDP_L3 does not depend
on X86_FEATURE_CAT_L3 and in turn X86_FEATURE_CAT_L3 does not depend on
X86_FEATURE_RDT_A. Could you please elaborate why this is needed?

>   	{ X86_FEATURE_AVX512_BF16,		X86_FEATURE_AVX512VL  },
>   	{ X86_FEATURE_AVX512_FP16,		X86_FEATURE_AVX512BW  },
>   	{ X86_FEATURE_ENQCMD,			X86_FEATURE_XSAVES    },
> diff --git a/arch/x86/kernel/cpu/scattered.c b/arch/x86/kernel/cpu/scattered.c
> index c84c30188fdf..88f00575c9ff 100644
> --- a/arch/x86/kernel/cpu/scattered.c
> +++ b/arch/x86/kernel/cpu/scattered.c
> @@ -49,6 +49,7 @@ static const struct cpuid_bit cpuid_bits[] = {
>   	{ X86_FEATURE_MBA,		CPUID_EBX,  6, 0x80000008, 0 },
>   	{ X86_FEATURE_SMBA,		CPUID_EBX,  2, 0x80000020, 0 },
>   	{ X86_FEATURE_BMEC,		CPUID_EBX,  3, 0x80000020, 0 },
> +	{ X86_FEATURE_SDCIAE,		CPUID_EBX,  6, 0x80000020, 0 },
>   	{ X86_FEATURE_PERFMON_V2,	CPUID_EAX,  0, 0x80000022, 0 },
>   	{ X86_FEATURE_AMD_LBR_V2,	CPUID_EAX,  1, 0x80000022, 0 },
>   	{ X86_FEATURE_AMD_LBR_PMC_FREEZE,	CPUID_EAX,  2, 0x80000022, 0 },

Reinette

Re: [PATCH 2/7] x86/resctrl: Add SDCIAE feature in the command line options

[Reinette Chatre]

Hi Babu,

On 8/16/24 9:16 AM, Babu Moger wrote:
> Add the command line options to enable or disable the new resctrl feature
> L3 Smart Data Cache Injection Allocation Enforcement (SDCIAE).
> 
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---
>   Documentation/admin-guide/kernel-parameters.txt | 2 +-
>   arch/x86/kernel/cpu/resctrl/core.c              | 2 ++
>   2 files changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 09126bb8cc9f..63f17d23b8f4 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -5604,7 +5604,7 @@
>   	rdt=		[HW,X86,RDT]
>   			Turn on/off individual RDT features. List is:
>   			cmt, mbmtotal, mbmlocal, l3cat, l3cdp, l2cat, l2cdp,
> -			mba, smba, bmec.
> +			mba, smba, bmec, sdciae.
>   			E.g. to turn on cmt and turn off mba use:
>   				rdt=cmt,!mba
>   
> diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
> index 1930fce9dfe9..c4dfc768ddf5 100644
> --- a/arch/x86/kernel/cpu/resctrl/core.c
> +++ b/arch/x86/kernel/cpu/resctrl/core.c
> @@ -801,6 +801,7 @@ enum {
>   	RDT_FLAG_MBA,
>   	RDT_FLAG_SMBA,
>   	RDT_FLAG_BMEC,
> +	RDT_FLAG_SDCIAE,
>   };
>   
>   #define RDT_OPT(idx, n, f)	\
> @@ -826,6 +827,7 @@ static struct rdt_options rdt_options[]  __initdata = {
>   	RDT_OPT(RDT_FLAG_MBA,	    "mba",	X86_FEATURE_MBA),
>   	RDT_OPT(RDT_FLAG_SMBA,	    "smba",	X86_FEATURE_SMBA),
>   	RDT_OPT(RDT_FLAG_BMEC,	    "bmec",	X86_FEATURE_BMEC),
> +	RDT_OPT(RDT_FLAG_SDCIAE,    "sdciae",	X86_FEATURE_SDCIAE),
>   };
>   #define NUM_RDT_OPTIONS ARRAY_SIZE(rdt_options)
>   

Why is this needed when patch #5 introduces an interface to enable/disable
this feature after mount?

Reinette

Re: [PATCH 3/7] x86/resctrl: Introduce sdciae_capable in rdt_resource

[Reinette Chatre]

Hi Babu,

On 8/16/24 9:16 AM, Babu Moger wrote:
> Detect SDCIAE`(L3 Smart Data Cache Injection Allocation Enforcement)

(stray ` char)

> feature and initialize sdciae_capable.

(This is a repeat of the discussion we had surrounding the ABMC feature.)

By adding "sdciae_capable" to struct rdt_resource the "sdciae" feature
becomes a resctrl fs feature. Any other architecture that has a "similar
but perhaps not identical feature to AMD's SDCIAE" will be forced to also
call it "sdciae" ... sdciae seems like a marketing name to me and resctrl
needs something generic that could later be built on (if needed) by other
architectures.

Reinette

Re: [PATCH 4/7] x86/resctrl: Implement SDCIAE enable/disable

[Reinette Chatre]

Hi Babu,

On 8/16/24 9:16 AM, Babu Moger wrote:
> SDCIAE feature can be enabled by setting bit 1 in MSR L3_QOS_EXT_CFG.
> When the state of SDCIAE is changed, it must be changed to the updated
> value on all logical processors in the QOS Domain. By default, the SDCIAE
> feature is disabled.
> 
> Introduce arch handlers to detect and enable/disable the feature.
> 
> The SDCIAE feature details are available in APM listed below [1].
> [1] AMD64 Architecture Programmer's Manual Volume 2: System Programming
> Publication # 24593 Revision 3.41 section 19.4.7 L3 Smart Data Cache
> Injection Allocation Enforcement (SDCIAE)
> 
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=206537
> ---
>   arch/x86/include/asm/msr-index.h       |  1 +
>   arch/x86/kernel/cpu/resctrl/internal.h | 12 +++++
>   arch/x86/kernel/cpu/resctrl/rdtgroup.c | 61 ++++++++++++++++++++++++++
>   3 files changed, 74 insertions(+)
> 
> diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
> index 82c6a4d350e0..c78afed3c21f 100644
> --- a/arch/x86/include/asm/msr-index.h
> +++ b/arch/x86/include/asm/msr-index.h
> @@ -1181,6 +1181,7 @@
>   /* - AMD: */
>   #define MSR_IA32_MBA_BW_BASE		0xc0000200
>   #define MSR_IA32_SMBA_BW_BASE		0xc0000280
> +#define MSR_IA32_L3_QOS_EXT_CFG		0xc00003ff
>   #define MSR_IA32_EVT_CFG_BASE		0xc0000400
>   
>   /* MSR_IA32_VMX_MISC bits */
> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
> index 955999aecfca..ceb0e8e1ed76 100644
> --- a/arch/x86/kernel/cpu/resctrl/internal.h
> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
> @@ -56,6 +56,9 @@
>   /* Max event bits supported */
>   #define MAX_EVT_CONFIG_BITS		GENMASK(6, 0)
>   
> +/* Setting bit 1 in L3_QOS_EXT_CFG enables the SDCIAE feature. */
> +#define SDCIAE_ENABLE_BIT		1
> +
>   /**
>    * cpumask_any_housekeeping() - Choose any CPU in @mask, preferring those that
>    *			        aren't marked nohz_full
> @@ -477,6 +480,7 @@ struct rdt_parse_data {
>    * @mbm_cfg_mask:	Bandwidth sources that can be tracked when Bandwidth
>    *			Monitoring Event Configuration (BMEC) is supported.
>    * @cdp_enabled:	CDP state of this resource
> + * @sdciae_enabled:	SDCIAE feature is enabled
>    *
>    * Members of this structure are either private to the architecture
>    * e.g. mbm_width, or accessed via helpers that provide abstraction. e.g.
> @@ -491,6 +495,7 @@ struct rdt_hw_resource {
>   	unsigned int		mbm_width;
>   	unsigned int		mbm_cfg_mask;
>   	bool			cdp_enabled;
> +	bool			sdciae_enabled;
>   };
>   
>   static inline struct rdt_hw_resource *resctrl_to_arch_res(struct rdt_resource *r)
> @@ -536,6 +541,13 @@ int resctrl_arch_set_cdp_enabled(enum resctrl_res_level l, bool enable);
>   
>   void arch_mon_domain_online(struct rdt_resource *r, struct rdt_mon_domain *d);
>   
> +static inline bool resctrl_arch_get_sdciae_enabled(enum resctrl_res_level l)
> +{
> +	return rdt_resources_all[l].sdciae_enabled;
> +}
> +
> +int resctrl_arch_set_sdciae_enabled(enum resctrl_res_level l, bool enable);
> +
>   /*
>    * To return the common struct rdt_resource, which is contained in struct
>    * rdt_hw_resource, walk the resctrl member of struct rdt_hw_resource.
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index d7163b764c62..c62d6183bfe4 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -1789,6 +1789,67 @@ static ssize_t mbm_local_bytes_config_write(struct kernfs_open_file *of,
>   	return ret ?: nbytes;
>   }
>   
> +static void resctrl_sdciae_msrwrite(void *arg)
> +{
> +	bool *enable = arg;
> +
> +	if (*enable)
> +		msr_set_bit(MSR_IA32_L3_QOS_EXT_CFG, SDCIAE_ENABLE_BIT);
> +	else
> +		msr_clear_bit(MSR_IA32_L3_QOS_EXT_CFG, SDCIAE_ENABLE_BIT);
> +}

(hmmm ... so there is an effort to make the rest of the code not be
resource specific ... but then the lowest level has L3 MSR hardcoded)

> +
> +static int resctrl_sdciae_setup(enum resctrl_res_level l, bool enable)
> +{
> +	struct rdt_resource *r = &rdt_resources_all[l].r_resctrl;
> +	struct rdt_ctrl_domain *d;
> +
> +	/* Update  L3_QOS_EXT_CFG MSR on all the CPUs in all domains*/

(please note some space issues above)

> +	list_for_each_entry(d, &r->ctrl_domains, hdr.list)
> +		on_each_cpu_mask(&d->hdr.cpu_mask, resctrl_sdciae_msrwrite, &enable, 1);
> +
> +	return 0;

It seems that this will be inside the arch specific code while
resctrl_arch_set_sdciae_enabled() will be called by resctrl fs code. It is
thus not clear why above returns an int, thus forcing callers to do
error code handling, when it will always just return 0.

> +}
> +
> +static int resctrl_sdciae_enable(enum resctrl_res_level l)
> +{
> +	struct rdt_hw_resource *hw_res = &rdt_resources_all[l];
> +	int ret = 0;
> +
> +	if (!hw_res->sdciae_enabled) {
> +		ret = resctrl_sdciae_setup(l, true);
> +		if (!ret)
> +			hw_res->sdciae_enabled = true;
> +	}
> +
> +	return ret;

Same here ... this will always return 0, no?

> +}
> +
> +static void resctrl_sdciae_disable(enum resctrl_res_level l)
> +{
> +	struct rdt_hw_resource *hw_res = &rdt_resources_all[l];
> +
> +	if (hw_res->sdciae_enabled) {
> +		resctrl_sdciae_setup(l, false);
> +		hw_res->sdciae_enabled = false;
> +	}
> +}
> +
> +int resctrl_arch_set_sdciae_enabled(enum resctrl_res_level l, bool enable)
> +{
> +	struct rdt_hw_resource *hw_res = &rdt_resources_all[l];
> +
> +	if (!hw_res->r_resctrl.sdciae_capable)
> +		return -EINVAL;
> +
> +	if (enable)
> +		return resctrl_sdciae_enable(l);
> +
> +	resctrl_sdciae_disable(l);
> +
> +	return 0;
> +}
> +
>   /* rdtgroup information files for one cache resource. */
>   static struct rftype res_common_files[] = {
>   	{

Reinette

Re: [PATCH 5/7] x86/resctrl: Add interface to enable/disable SDCIAE

[Reinette Chatre]

Hi Babu,

On 8/16/24 9:16 AM, Babu Moger wrote:
> The SDCIAE (SDCI Allocation Enforcement) PQE feature allows system software
> to configure the portion of the L3 cache used for SDCI.
> 
> When enabled, SDCIAE forces all SDCI lines to be placed into the L3 cache
> partitions identified by the highest-supported L3_MASK_n register as
> reported by CPUID Fn0000_0010_EDX_x1.MAX_COS. For example, if MAX_COS=15,
> SDCI lines will be allocated into the L3 cache partitions determined by
> the bitmask in the L3_MASK_15 register.
> 
> Introduce interface to enable/disable SDCIAE feature on user input.
> 
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---
>   Documentation/arch/x86/resctrl.rst     | 22 +++++++
>   arch/x86/kernel/cpu/resctrl/core.c     |  1 +
>   arch/x86/kernel/cpu/resctrl/internal.h |  1 +
>   arch/x86/kernel/cpu/resctrl/rdtgroup.c | 88 ++++++++++++++++++++++++++
>   4 files changed, 112 insertions(+)
> 
> diff --git a/Documentation/arch/x86/resctrl.rst b/Documentation/arch/x86/resctrl.rst
> index a824affd741d..cb1532dd843f 100644
> --- a/Documentation/arch/x86/resctrl.rst
> +++ b/Documentation/arch/x86/resctrl.rst
> @@ -135,6 +135,28 @@ related to allocation:
>   			"1":
>   			      Non-contiguous 1s value in CBM is supported.
>   
> +"sdciae":
> +		Indicates if the system can support SDCIAE (L3 Smart Data Cache
> +		Injection Allocation Enforcement) feature.
> +
> +		Smart Data Cache Injection (SDCI) is a mechanism that enables
> +		direct insertion of data from I/O devices into the L3 cache.
> +		By directly caching data from I/O devices rather than first
> +		storing the I/O data in DRAM, SDCI reduces demands on DRAM
> +		bandwidth and reduces latency to the processor consuming the
> +		I/O data. The SDCIAE feature allows system software to configure
> +		limit the portion of the L3 cache used for SDCI.

Above needs to change to a generic resctrl fs feature.

> +
> +			"0":
> +			      Feature is not enabled.
> +			"1":
> +			      Feature is enabled.

What does "feature is enabled" and "feature is not enabled" mean to the user?
What can the user expect to happen after enabling/disabling this feature?

> +
> +		Feature can be enabled/disabled by writing to the interface.
> +		Example::
> +
> +			# echo 1 > /sys/fs/resctrl/info/L3/sdciae
> +
>   Memory bandwidth(MB) subdirectory contains the following files
>   with respect to allocation:
>   
> diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
> index e4381e3feb75..6a9512008a4a 100644
> --- a/arch/x86/kernel/cpu/resctrl/core.c
> +++ b/arch/x86/kernel/cpu/resctrl/core.c
> @@ -299,6 +299,7 @@ static void rdt_get_cdp_config(int level)
>   static void rdt_get_sdciae_alloc_cfg(struct rdt_resource *r)
>   {
>   	r->sdciae_capable = true;
> +	resctrl_sdciae_rftype_init();
>   }
>   
>   static void rdt_get_cdp_l3_config(void)
> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
> index ceb0e8e1ed76..9a3da6d49144 100644
> --- a/arch/x86/kernel/cpu/resctrl/internal.h
> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
> @@ -662,6 +662,7 @@ void rdt_domain_reconfigure_cdp(struct rdt_resource *r);
>   void __init thread_throttle_mode_init(void);
>   void __init mbm_config_rftype_init(const char *config);
>   void rdt_staged_configs_clear(void);
> +void __init resctrl_sdciae_rftype_init(void);
>   bool closid_allocated(unsigned int closid);
>   int resctrl_find_cleanest_closid(void);
>   
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index c62d6183bfe4..58e4df195207 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -171,6 +171,27 @@ void closid_free(int closid)
>   	__set_bit(closid, &closid_free_map);
>   }
>   
> +/*
> + * SDCIAE feature uses max CLOSID to route the SDCI traffic.
> + * Get the max CLOSID number
> + */
> +static u32 get_sdciae_closid(struct rdt_resource *r)
> +{
> +	return resctrl_arch_get_num_closid(r) - 1;
> +}
> +
> +static int closid_alloc_sdciae(struct rdt_resource *r)
> +{
> +	u32 sdciae_closid = get_sdciae_closid(r);
> +
> +	if (closid_free_map & (1 << sdciae_closid)) {
> +		closid_free_map &= ~(1 << sdciae_closid);
> +		return sdciae_closid;
> +	} else {
> +		return -ENOSPC;
> +	}
> +}

How does this interact with CDP? It seems to me that the above would
cause overflow on a CDP system since the closid_free_map is sized to
half of what the hardware supports. This also seems to still allow
creating resource groups that may end up using the CLOSID dedicated
to SDCIAE here since the (when CDP enabled) resource groups use
software closid and then hardware configuration is done with the
hardware closid. When CDP is enabled it seems possible to still
create a resource group and modify the CBM of what is then intended to
be sdciae allocations?

Also, please be consistent with function naming, note how the above
two functions differ wrt namespace and verb. resctrl is surely not
consistent in this regard but it really helps to have partner functions
look similar. For example, this could be "feature_closid_get()" and
"feature_closid_alloc()".

Also, there looks to be opportunity to use bitops here ... perhaps
__test_and_clear_bit()?


> +
>   /**
>    * closid_allocated - test if provided closid is in use
>    * @closid: closid to be tested
> @@ -1850,6 +1871,57 @@ int resctrl_arch_set_sdciae_enabled(enum resctrl_res_level l, bool enable)
>   	return 0;
>   }
>   
> +static int resctrl_sdciae_show(struct kernfs_open_file *of,
> +			       struct seq_file *seq, void *v)
> +{
> +	seq_printf(seq, "%x\n", resctrl_arch_get_sdciae_enabled(RDT_RESOURCE_L3));
> +	return 0;
> +}

This does not look right ... this file has flags "RFTYPE_CTRL_INFO | RFTYPE_RES_CACHE"
which means that it will be created for all CAT resources. So, on a system that supports
L2 CAT, the "sdciae" file will be created for the L2 resource and it will show whether
"sdciae" is enabled on the *L3* resource?

> +
> +static ssize_t resctrl_sdciae_write(struct kernfs_open_file *of, char *buf,
> +				    size_t nbytes, loff_t off)
> +{
> +	struct resctrl_schema *s = of->kn->parent->priv;
> +	struct rdt_resource *r = s->res;
> +	unsigned int enable;
> +	u32 sdciae_closid;
> +	int ret;
> +
> +	if (!r->sdciae_capable)
> +		return -EINVAL;
> +
> +	ret = kstrtouint(buf, 0, &enable);

How about kstrtobool()? This will make things more consistent with
resctrl_arch_set_sdciae_enabled() expecting a bool. Or should we be looking ahead
at this file later possibly needing to support more integers to activate more capabilities
related to this feature? In that case this implementation needs to take care since instead
of supporting "0" and "1" it supports "0" and "anything but 0" that prevents any such
future enhancements.


> +	if (ret)
> +		return ret;
> +
> +	cpus_read_lock();
> +	mutex_lock(&rdtgroup_mutex);
> +
> +	rdt_last_cmd_clear();
> +
> +	/* Update the MSR only when there is a change */

The resctrl fs cannot make predictions on what arch code needs to do to enable feature.

> +	if (resctrl_arch_get_sdciae_enabled(RDT_RESOURCE_L3) != enable) {

(same issue with this file being present under L2 resource creating confusion)

> +		if (enable) {
> +			ret = closid_alloc_sdciae(r);
> +			if (ret < 0) {
> +				rdt_last_cmd_puts("SDCIAE CLOSID is not available\n");
> +				goto out_sdciae;
> +			}
> +		} else {
> +			sdciae_closid = get_sdciae_closid(r);
> +			closid_free(sdciae_closid);
> +		}


> +
> +		ret = resctrl_arch_set_sdciae_enabled(RDT_RESOURCE_L3, enable);

I assume that once SDCIAE is enabled the I/O traffic will start flowing to whatever
was the last CBM of the max CLOSID? Is this intended or should there be some default
CBM that this feature should start with?

> +	}
> +
> +out_sdciae:
> +	mutex_unlock(&rdtgroup_mutex);
> +	cpus_read_unlock();
> +
> +	return ret ?: nbytes;
> +}
> +
>   /* rdtgroup information files for one cache resource. */
>   static struct rftype res_common_files[] = {
>   	{
> @@ -2002,6 +2074,13 @@ static struct rftype res_common_files[] = {
>   		.seq_show	= rdtgroup_schemata_show,
>   		.fflags		= RFTYPE_CTRL_BASE,
>   	},
> +	{
> +		.name		= "sdciae",
> +		.mode		= 0644,
> +		.kf_ops		= &rdtgroup_kf_single_ops,
> +		.seq_show	= resctrl_sdciae_show,
> +		.write		= resctrl_sdciae_write,
> +	},
>   	{
>   		.name		= "mode",
>   		.mode		= 0644,
> @@ -2101,6 +2180,15 @@ void __init mbm_config_rftype_init(const char *config)
>   		rft->fflags = RFTYPE_MON_INFO | RFTYPE_RES_CACHE;
>   }
>   
> +void __init resctrl_sdciae_rftype_init(void)
> +{
> +	struct rftype *rft;
> +
> +	rft = rdtgroup_get_rftype_by_name("sdciae");
> +	if (rft)
> +		rft->fflags = RFTYPE_CTRL_INFO | RFTYPE_RES_CACHE;
> +}
> +

Another instance of the pattern that is impacted by the ABMC and MPAM work.

>   /**
>    * rdtgroup_kn_mode_restrict - Restrict user access to named resctrl file
>    * @r: The resource group with which the file is associated.

Reinette

Re: [PATCH 6/7] x86/resctrl: Introduce interface to display SDCIAE Capacity Bit Masks

[Reinette Chatre]

Hi Babu,

On 8/16/24 9:16 AM, Babu Moger wrote:
> When enabled, SDCIAE forces all SDCI lines to be placed into the L3 cache
> partitions identified by the highest-supported L3_MASK_n register where
> n is the maximum CLOSID supported.
> 
> Add the interface to display CBMs (Capacity Bit Mask) of the SDCIAE.
> 
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---
>   arch/x86/kernel/cpu/resctrl/ctrlmondata.c |  2 +-
>   arch/x86/kernel/cpu/resctrl/internal.h    |  1 +
>   arch/x86/kernel/cpu/resctrl/rdtgroup.c    | 29 +++++++++++++++++++++++
>   3 files changed, 31 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
> index 50fa1fe9a073..fc99f4d17e6c 100644
> --- a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
> +++ b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
> @@ -439,7 +439,7 @@ u32 resctrl_arch_get_config(struct rdt_resource *r, struct rdt_ctrl_domain *d,
>   	return hw_dom->ctrl_val[idx];
>   }
>   
> -static void show_doms(struct seq_file *s, struct resctrl_schema *schema, int closid)
> +void show_doms(struct seq_file *s, struct resctrl_schema *schema, int closid)
>   {
>   	struct rdt_resource *r = schema->res;
>   	struct rdt_ctrl_domain *dom;
> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
> index 9a3da6d49144..f2c87ca37b13 100644
> --- a/arch/x86/kernel/cpu/resctrl/internal.h
> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
> @@ -663,6 +663,7 @@ void __init thread_throttle_mode_init(void);
>   void __init mbm_config_rftype_init(const char *config);
>   void rdt_staged_configs_clear(void);
>   void __init resctrl_sdciae_rftype_init(void);
> +void show_doms(struct seq_file *s, struct resctrl_schema *schema, int closid);
>   bool closid_allocated(unsigned int closid);
>   int resctrl_find_cleanest_closid(void);
>   
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index 58e4df195207..51bc715bb6ae 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -1922,6 +1922,25 @@ static ssize_t resctrl_sdciae_write(struct kernfs_open_file *of, char *buf,
>   	return ret ?: nbytes;
>   }
>   
> +static int resctrl_sdciae_cbm_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;
> +	u32 sdciae_closid;
> +
> +	if (!resctrl_arch_get_sdciae_enabled(RDT_RESOURCE_L3)) {

(same issue with this file appearing in all cache resources, eg. L2)

> +		rdt_last_cmd_puts("SDCIAE is not enabled\n");
> +		return -EINVAL;
> +	}
> +
> +	sdciae_closid = get_sdciae_closid(r);
> +
> +	show_doms(seq, s, sdciae_closid);
> +
> +	return 0;
> +}

This really needs protection. (You should have encountered warnings
via lockdep_assert_held(&rdtgroup_mutex) and lockdep_assert_cpus_held()
found in the functions called.)

> +
>   /* rdtgroup information files for one cache resource. */
>   static struct rftype res_common_files[] = {
>   	{
> @@ -2081,6 +2100,12 @@ static struct rftype res_common_files[] = {
>   		.seq_show	= resctrl_sdciae_show,
>   		.write		= resctrl_sdciae_write,
>   	},
> +	{
> +		.name		= "sdciae_cbm",
> +		.mode		= 0444,
> +		.kf_ops		= &rdtgroup_kf_single_ops,
> +		.seq_show	= resctrl_sdciae_cbm_show,
> +	},
>   	{
>   		.name		= "mode",
>   		.mode		= 0644,
> @@ -2187,6 +2212,10 @@ void __init resctrl_sdciae_rftype_init(void)
>   	rft = rdtgroup_get_rftype_by_name("sdciae");
>   	if (rft)
>   		rft->fflags = RFTYPE_CTRL_INFO | RFTYPE_RES_CACHE;
> +
> +	rft = rdtgroup_get_rftype_by_name("sdciae_cbm");
> +	if (rft)
> +		rft->fflags = RFTYPE_CTRL_INFO | RFTYPE_RES_CACHE;
>   }
>   
>   /**

Reinette

Re: [PATCH 0/7] x86/resctrl : Support L3 Smart Data Cache Injection Allocation Enforcement (SDCIAE)

[Moger, Babu]

Hi Reinette,

Thanks for the feedback.

On 9/13/24 15:44, Reinette Chatre wrote:
> Hi Babu,
> 
> On 8/16/24 9:16 AM, Babu Moger wrote:
>>
>> This series adds the support for L3 Smart Data Cache Injection Allocation
>> Enforcement (SDCIAE) to resctrl infrastructure.
>>
>> Upcoming AMD hardware implements Smart Data Cache Injection (SDCI).
>> Smart Data Cache Injection (SDCI) is a mechanism that enables direct
>> insertion of data from I/O devices into the L3 cache. By directly caching
>> data from I/O devices rather than first storing the I/O data in DRAM, SDCI
>> reduces demands on DRAM bandwidth and reduces latency to the processor
>> consuming the I/O data. The SDCIAE (SDCI Allocation Enforcement) PQE
>> feature allows system software to limit the portion of the L3 cache used
>> for SDCI.
>>
> 
> This series introduces new user interface. Could you please describe the
> new user interface in the cover letter and how users are expected to interact
> with this interface to be able to use this new feature? Please also describe
> the impact on existing resctrl interfaces related to cache allocation from
> I/O hardware, for example "shareable_bits", "bit_usage", etc. These existing
> interfaces are used to communicate to user space how portions of cache are
> used by I/O hardware but I cannot tell from this series how this work
> builds on
> this.

Sure. Will add interface details and bit mask usage.

> 
> How does this feature work with the existing "L3 Cache Allocation Sharing
> Mask"
> that is enumerated as part of CAT feature?

Sharing is allowed as it is right now. SDCIAE cannot use the masks that
are exclusive.

> 
>> The feature details are documented in the APM listed below [1].
>> [1] AMD64 Architecture Programmer's Manual Volume 2: System Programming
>> Publication # 24593 Revision 3.41 section 19.4.7 L3 Smart Data Cache
>> Injection Allocation Enforcement (SDCIAE)
>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=206537
>>
>> The feature requires linux support of TPH (TLP Processing Hints).
>> The support is ongoing and patches are currently under review.
>> https://lore.kernel.org/lkml/20240717205511.2541693-2-wei.huang2@amd.com/
> 
> Please note that the cover letter [1] of that series mentions "Cache
> Injection
> allows PCIe endpoints to inject I/O Coherent DMA writes directly into an
> L2 ..."
> while this series implements and refers to L3 only.

SDCIAE only applies to L3 insertion. SDCIAE has no impact on SDCI
insertion into the L2.

SDCI lines will be inserted into the L3 after being evicted by L2 based on
the maximum CLOS(CLOSIS 15) allocation mask when SDCIAE is enabled. By
limiting the number of ways in L3, it will impact the SDCI cache occupancy
in L3.
-- 
Thanks
Babu Moger

Re: [PATCH 1/7] x86/cpufeatures: Add support for L3 Smart Data Cache Injection Allocation Enforcement

[Moger, Babu]

Hi Reinette,

On 9/13/2024 3:44 PM, Reinette Chatre wrote:
> Hi Babu,
> 
> On 8/16/24 9:16 AM, Babu Moger wrote:
>> Smart Data Cache Injection (SDCI) is a mechanism that enables direct
>> insertion of data from I/O devices into the L3 cache. By directly caching
>> data from I/O devices rather than first storing the I/O data in DRAM,
>> SDCI reduces demands on DRAM bandwidth and reduces latency to the 
>> processor
>> consuming the I/O data.
>>
>> The SDCIAE (SDCI Allocation Enforcement) PQE feature allows system 
>> software
>> to limit the portion of the L3 cache used for SDCI.
>>
>> When enabled, SDCIAE forces all SDCI lines to be placed into the L3 cache
>> partitions identified by the highest-supported L3_MASK_n register where n
>> maximum supported CLOSID.
> 
> "where n maximum supported CLOSID" -> "where n is the maximum supported 
> CLOSID" ?
> 
Sure.

>>
>> Add CPUID feature bit that can be used to configure SDCIAE.
>>
>> The feature details are documented in APM listed below [1].
>> [1] AMD64 Architecture Programmer's Manual Volume 2: System Programming
>> Publication # 24593 Revision 3.41 section 19.4.7 L3 Smart Data Cache
>> Injection Allocation Enforcement (SDCIAE)
>>
>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=206537
>> Signed-off-by: Babu Moger <babu.moger@amd.com>
>> ---
>>   arch/x86/include/asm/cpufeatures.h | 1 +
>>   arch/x86/kernel/cpu/cpuid-deps.c   | 1 +
>>   arch/x86/kernel/cpu/scattered.c    | 1 +
>>   3 files changed, 3 insertions(+)
>>
>> diff --git a/arch/x86/include/asm/cpufeatures.h 
>> b/arch/x86/include/asm/cpufeatures.h
>> index dd4682857c12..5ca39431d423 100644
>> --- a/arch/x86/include/asm/cpufeatures.h
>> +++ b/arch/x86/include/asm/cpufeatures.h
>> @@ -473,6 +473,7 @@
>>   #define X86_FEATURE_CLEAR_BHB_HW    (21*32+ 3) /* BHI_DIS_S HW 
>> control enabled */
>>   #define X86_FEATURE_CLEAR_BHB_LOOP_ON_VMEXIT (21*32+ 4) /* Clear 
>> branch history at vmexit using SW loop */
>>   #define X86_FEATURE_FAST_CPPC        (21*32 + 5) /* AMD Fast CPPC */
>> +#define X86_FEATURE_SDCIAE        (21*32 + 6) /* "" L3 Smart Data 
>> Cache Injection Allocation Enforcement */
>>   /*
>>    * BUG word(s)
>> diff --git a/arch/x86/kernel/cpu/cpuid-deps.c 
>> b/arch/x86/kernel/cpu/cpuid-deps.c
>> index b7d9f530ae16..1ef42cc4cc75 100644
>> --- a/arch/x86/kernel/cpu/cpuid-deps.c
>> +++ b/arch/x86/kernel/cpu/cpuid-deps.c
>> @@ -70,6 +70,7 @@ static const struct cpuid_dep cpuid_deps[] = {
>>       { X86_FEATURE_CQM_MBM_LOCAL,        X86_FEATURE_CQM_LLC   },
>>       { X86_FEATURE_BMEC,            X86_FEATURE_CQM_MBM_TOTAL   },
>>       { X86_FEATURE_BMEC,            X86_FEATURE_CQM_MBM_LOCAL   },
>> +    { X86_FEATURE_SDCIAE,            X86_FEATURE_RDT_A     },
> 
> The need for this dependency is not clear to me. If there was a dependency
> then I would have expected it to be X86_FEATURE_CAT_L3 but we have not
> previously needed to do this. For example, X86_FEATURE_CDP_L3 does not 
> depend
> on X86_FEATURE_CAT_L3 and in turn X86_FEATURE_CAT_L3 does not depend on
> X86_FEATURE_RDT_A. Could you please elaborate why this is needed?

SDCIAE is allocation feature. So, I added X86_FEATURE_RDT_A.
Yea, It may be appropriate to add dependency on X86_FEATURE_CAT_L3. 
Because it is CAT_L3 related feature. I can change that.

I don't know the history why we didn't have dependency on CDP and CAT_L3.

Thanks
- Babu Moger

Re: [PATCH 2/7] x86/resctrl: Add SDCIAE feature in the command line options

[Moger, Babu]

Hi Reinette,

On 9/13/24 15:45, Reinette Chatre wrote:
> Hi Babu,
> 
> On 8/16/24 9:16 AM, Babu Moger wrote:
>> Add the command line options to enable or disable the new resctrl feature
>> L3 Smart Data Cache Injection Allocation Enforcement (SDCIAE).
>>
>> Signed-off-by: Babu Moger <babu.moger@amd.com>
>> ---
>>   Documentation/admin-guide/kernel-parameters.txt | 2 +-
>>   arch/x86/kernel/cpu/resctrl/core.c              | 2 ++
>>   2 files changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/admin-guide/kernel-parameters.txt
>> b/Documentation/admin-guide/kernel-parameters.txt
>> index 09126bb8cc9f..63f17d23b8f4 100644
>> --- a/Documentation/admin-guide/kernel-parameters.txt
>> +++ b/Documentation/admin-guide/kernel-parameters.txt
>> @@ -5604,7 +5604,7 @@
>>       rdt=        [HW,X86,RDT]
>>               Turn on/off individual RDT features. List is:
>>               cmt, mbmtotal, mbmlocal, l3cat, l3cdp, l2cat, l2cdp,
>> -            mba, smba, bmec.
>> +            mba, smba, bmec, sdciae.
>>               E.g. to turn on cmt and turn off mba use:
>>                   rdt=cmt,!mba
>>   diff --git a/arch/x86/kernel/cpu/resctrl/core.c
>> b/arch/x86/kernel/cpu/resctrl/core.c
>> index 1930fce9dfe9..c4dfc768ddf5 100644
>> --- a/arch/x86/kernel/cpu/resctrl/core.c
>> +++ b/arch/x86/kernel/cpu/resctrl/core.c
>> @@ -801,6 +801,7 @@ enum {
>>       RDT_FLAG_MBA,
>>       RDT_FLAG_SMBA,
>>       RDT_FLAG_BMEC,
>> +    RDT_FLAG_SDCIAE,
>>   };
>>     #define RDT_OPT(idx, n, f)    \
>> @@ -826,6 +827,7 @@ static struct rdt_options rdt_options[]  __initdata = {
>>       RDT_OPT(RDT_FLAG_MBA,        "mba",    X86_FEATURE_MBA),
>>       RDT_OPT(RDT_FLAG_SMBA,        "smba",    X86_FEATURE_SMBA),
>>       RDT_OPT(RDT_FLAG_BMEC,        "bmec",    X86_FEATURE_BMEC),
>> +    RDT_OPT(RDT_FLAG_SDCIAE,    "sdciae",    X86_FEATURE_SDCIAE),
>>   };
>>   #define NUM_RDT_OPTIONS ARRAY_SIZE(rdt_options)
>>   
> 
> Why is this needed when patch #5 introduces an interface to enable/disable
> this feature after mount?

We have provided option to disable the RDT features on boot. To be
consistent, I have add this feature also to the list.

Yes. Feature can be enabled or disabled after mount.
Thanks
Babu Moger

Re: [PATCH 3/7] x86/resctrl: Introduce sdciae_capable in rdt_resource

[Moger, Babu]

Hi Reinette,

On 9/13/24 15:45, Reinette Chatre wrote:
> Hi Babu,
> 
> On 8/16/24 9:16 AM, Babu Moger wrote:
>> Detect SDCIAE`(L3 Smart Data Cache Injection Allocation Enforcement)
> 
> (stray ` char)

Sure.

> 
>> feature and initialize sdciae_capable.
> 
> (This is a repeat of the discussion we had surrounding the ABMC feature.)
> 
> By adding "sdciae_capable" to struct rdt_resource the "sdciae" feature
> becomes a resctrl fs feature. Any other architecture that has a "similar
> but perhaps not identical feature to AMD's SDCIAE" will be forced to also
> call it "sdciae" ... sdciae seems like a marketing name to me and resctrl
> needs something generic that could later be built on (if needed) by other
> architectures.

How about "cache_inject_capable" ?

This seems generic. I will change the description also.

-- 
Thanks
Babu Moger

Re: [PATCH 4/7] x86/resctrl: Implement SDCIAE enable/disable

[Moger, Babu]

Hi Reinette,

On 9/13/24 15:46, Reinette Chatre wrote:
> Hi Babu,
> 
> On 8/16/24 9:16 AM, Babu Moger wrote:
>> SDCIAE feature can be enabled by setting bit 1 in MSR L3_QOS_EXT_CFG.
>> When the state of SDCIAE is changed, it must be changed to the updated
>> value on all logical processors in the QOS Domain. By default, the SDCIAE
>> feature is disabled.
>>
>> Introduce arch handlers to detect and enable/disable the feature.
>>
>> The SDCIAE feature details are available in APM listed below [1].
>> [1] AMD64 Architecture Programmer's Manual Volume 2: System Programming
>> Publication # 24593 Revision 3.41 section 19.4.7 L3 Smart Data Cache
>> Injection Allocation Enforcement (SDCIAE)
>>
>> Signed-off-by: Babu Moger <babu.moger@amd.com>
>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=206537
>> ---
>>   arch/x86/include/asm/msr-index.h       |  1 +
>>   arch/x86/kernel/cpu/resctrl/internal.h | 12 +++++
>>   arch/x86/kernel/cpu/resctrl/rdtgroup.c | 61 ++++++++++++++++++++++++++
>>   3 files changed, 74 insertions(+)
>>
>> diff --git a/arch/x86/include/asm/msr-index.h
>> b/arch/x86/include/asm/msr-index.h
>> index 82c6a4d350e0..c78afed3c21f 100644
>> --- a/arch/x86/include/asm/msr-index.h
>> +++ b/arch/x86/include/asm/msr-index.h
>> @@ -1181,6 +1181,7 @@
>>   /* - AMD: */
>>   #define MSR_IA32_MBA_BW_BASE        0xc0000200
>>   #define MSR_IA32_SMBA_BW_BASE        0xc0000280
>> +#define MSR_IA32_L3_QOS_EXT_CFG        0xc00003ff
>>   #define MSR_IA32_EVT_CFG_BASE        0xc0000400
>>     /* MSR_IA32_VMX_MISC bits */
>> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h
>> b/arch/x86/kernel/cpu/resctrl/internal.h
>> index 955999aecfca..ceb0e8e1ed76 100644
>> --- a/arch/x86/kernel/cpu/resctrl/internal.h
>> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
>> @@ -56,6 +56,9 @@
>>   /* Max event bits supported */
>>   #define MAX_EVT_CONFIG_BITS        GENMASK(6, 0)
>>   +/* Setting bit 1 in L3_QOS_EXT_CFG enables the SDCIAE feature. */
>> +#define SDCIAE_ENABLE_BIT        1
>> +
>>   /**
>>    * cpumask_any_housekeeping() - Choose any CPU in @mask, preferring
>> those that
>>    *                    aren't marked nohz_full
>> @@ -477,6 +480,7 @@ struct rdt_parse_data {
>>    * @mbm_cfg_mask:    Bandwidth sources that can be tracked when Bandwidth
>>    *            Monitoring Event Configuration (BMEC) is supported.
>>    * @cdp_enabled:    CDP state of this resource
>> + * @sdciae_enabled:    SDCIAE feature is enabled
>>    *
>>    * Members of this structure are either private to the architecture
>>    * e.g. mbm_width, or accessed via helpers that provide abstraction. e.g.
>> @@ -491,6 +495,7 @@ struct rdt_hw_resource {
>>       unsigned int        mbm_width;
>>       unsigned int        mbm_cfg_mask;
>>       bool            cdp_enabled;
>> +    bool            sdciae_enabled;
>>   };
>>     static inline struct rdt_hw_resource *resctrl_to_arch_res(struct
>> rdt_resource *r)
>> @@ -536,6 +541,13 @@ int resctrl_arch_set_cdp_enabled(enum
>> resctrl_res_level l, bool enable);
>>     void arch_mon_domain_online(struct rdt_resource *r, struct
>> rdt_mon_domain *d);
>>   +static inline bool resctrl_arch_get_sdciae_enabled(enum
>> resctrl_res_level l)
>> +{
>> +    return rdt_resources_all[l].sdciae_enabled;
>> +}
>> +
>> +int resctrl_arch_set_sdciae_enabled(enum resctrl_res_level l, bool
>> enable);
>> +
>>   /*
>>    * To return the common struct rdt_resource, which is contained in struct
>>    * rdt_hw_resource, walk the resctrl member of struct rdt_hw_resource.
>> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> index d7163b764c62..c62d6183bfe4 100644
>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> @@ -1789,6 +1789,67 @@ static ssize_t
>> mbm_local_bytes_config_write(struct kernfs_open_file *of,
>>       return ret ?: nbytes;
>>   }
>>   +static void resctrl_sdciae_msrwrite(void *arg)
>> +{
>> +    bool *enable = arg;
>> +
>> +    if (*enable)
>> +        msr_set_bit(MSR_IA32_L3_QOS_EXT_CFG, SDCIAE_ENABLE_BIT);
>> +    else
>> +        msr_clear_bit(MSR_IA32_L3_QOS_EXT_CFG, SDCIAE_ENABLE_BIT);
>> +}
> 
> (hmmm ... so there is an effort to make the rest of the code not be
> resource specific ... but then the lowest level has L3 MSR hardcoded)

Not very clear on this.

I can separate the patch into two, one arch specific and the other FS
specific.

> 
>> +
>> +static int resctrl_sdciae_setup(enum resctrl_res_level l, bool enable)
>> +{
>> +    struct rdt_resource *r = &rdt_resources_all[l].r_resctrl;
>> +    struct rdt_ctrl_domain *d;
>> +
>> +    /* Update  L3_QOS_EXT_CFG MSR on all the CPUs in all domains*/
> 
> (please note some space issues above)

Sure.

> 
>> +    list_for_each_entry(d, &r->ctrl_domains, hdr.list)
>> +        on_each_cpu_mask(&d->hdr.cpu_mask, resctrl_sdciae_msrwrite,
>> &enable, 1);
>> +
>> +    return 0;
> 
> It seems that this will be inside the arch specific code while
> resctrl_arch_set_sdciae_enabled() will be called by resctrl fs code. It is
> thus not clear why above returns an int, thus forcing callers to do
> error code handling, when it will always just return 0.

Yes. It is returning 0 right now. Keeps the options open for other arch's
report error.  Looks like we heading to make this generic feature.


> 
>> +}
>> +
>> +static int resctrl_sdciae_enable(enum resctrl_res_level l)
>> +{
>> +    struct rdt_hw_resource *hw_res = &rdt_resources_all[l];
>> +    int ret = 0;
>> +
>> +    if (!hw_res->sdciae_enabled) {
>> +        ret = resctrl_sdciae_setup(l, true);
>> +        if (!ret)
>> +            hw_res->sdciae_enabled = true;
>> +    }
>> +
>> +    return ret;
> 
> Same here ... this will always return 0, no?

Yes. it is returns 0 always on AMD. Keeps the options open for other
arch's report error.

> 
>> +}
>> +
>> +static void resctrl_sdciae_disable(enum resctrl_res_level l)
>> +{
>> +    struct rdt_hw_resource *hw_res = &rdt_resources_all[l];
>> +
>> +    if (hw_res->sdciae_enabled) {
>> +        resctrl_sdciae_setup(l, false);
>> +        hw_res->sdciae_enabled = false;
>> +    }
>> +}
>> +
>> +int resctrl_arch_set_sdciae_enabled(enum resctrl_res_level l, bool enable)
>> +{
>> +    struct rdt_hw_resource *hw_res = &rdt_resources_all[l];
>> +
>> +    if (!hw_res->r_resctrl.sdciae_capable)
>> +        return -EINVAL;
>> +
>> +    if (enable)
>> +        return resctrl_sdciae_enable(l);
>> +
>> +    resctrl_sdciae_disable(l);
>> +
>> +    return 0;
>> +}
>> +
>>   /* rdtgroup information files for one cache resource. */
>>   static struct rftype res_common_files[] = {
>>       {
> 
> Reinette
> 

-- 
Thanks
Babu Moger

Re: [PATCH 3/7] x86/resctrl: Introduce sdciae_capable in rdt_resource

[Moger, Babu]

On 9/18/24 10:27, Moger, Babu wrote:
> Hi Reinette,
> 
> On 9/13/24 15:45, Reinette Chatre wrote:
>> Hi Babu,
>>
>> On 8/16/24 9:16 AM, Babu Moger wrote:
>>> Detect SDCIAE`(L3 Smart Data Cache Injection Allocation Enforcement)
>>
>> (stray ` char)
> 
> Sure.
> 
>>
>>> feature and initialize sdciae_capable.
>>
>> (This is a repeat of the discussion we had surrounding the ABMC feature.)
>>
>> By adding "sdciae_capable" to struct rdt_resource the "sdciae" feature
>> becomes a resctrl fs feature. Any other architecture that has a "similar
>> but perhaps not identical feature to AMD's SDCIAE" will be forced to also
>> call it "sdciae" ... sdciae seems like a marketing name to me and resctrl
>> needs something generic that could later be built on (if needed) by other
>> architectures.
> 
> How about "cache_inject_capable" ?
> 
> This seems generic. I will change the description also.
> 

Basically, this feature reserves specific CLOS for SDCI cache.

We can also name "clos_reserve_capable".
-- 
Thanks
Babu Moger

Re: [PATCH 5/7] x86/resctrl: Add interface to enable/disable SDCIAE

[Moger, Babu]

Hi Reinette,

On 9/13/24 15:51, Reinette Chatre wrote:
> Hi Babu,
> 
> On 8/16/24 9:16 AM, Babu Moger wrote:
>> The SDCIAE (SDCI Allocation Enforcement) PQE feature allows system software
>> to configure the portion of the L3 cache used for SDCI.
>>
>> When enabled, SDCIAE forces all SDCI lines to be placed into the L3 cache
>> partitions identified by the highest-supported L3_MASK_n register as
>> reported by CPUID Fn0000_0010_EDX_x1.MAX_COS. For example, if MAX_COS=15,
>> SDCI lines will be allocated into the L3 cache partitions determined by
>> the bitmask in the L3_MASK_15 register.
>>
>> Introduce interface to enable/disable SDCIAE feature on user input.
>>
>> Signed-off-by: Babu Moger <babu.moger@amd.com>
>> ---
>>   Documentation/arch/x86/resctrl.rst     | 22 +++++++
>>   arch/x86/kernel/cpu/resctrl/core.c     |  1 +
>>   arch/x86/kernel/cpu/resctrl/internal.h |  1 +
>>   arch/x86/kernel/cpu/resctrl/rdtgroup.c | 88 ++++++++++++++++++++++++++
>>   4 files changed, 112 insertions(+)
>>
>> diff --git a/Documentation/arch/x86/resctrl.rst
>> b/Documentation/arch/x86/resctrl.rst
>> index a824affd741d..cb1532dd843f 100644
>> --- a/Documentation/arch/x86/resctrl.rst
>> +++ b/Documentation/arch/x86/resctrl.rst
>> @@ -135,6 +135,28 @@ related to allocation:
>>               "1":
>>                     Non-contiguous 1s value in CBM is supported.
>>   +"sdciae":
>> +        Indicates if the system can support SDCIAE (L3 Smart Data Cache
>> +        Injection Allocation Enforcement) feature.
>> +
>> +        Smart Data Cache Injection (SDCI) is a mechanism that enables
>> +        direct insertion of data from I/O devices into the L3 cache.
>> +        By directly caching data from I/O devices rather than first
>> +        storing the I/O data in DRAM, SDCI reduces demands on DRAM
>> +        bandwidth and reduces latency to the processor consuming the
>> +        I/O data. The SDCIAE feature allows system software to configure
>> +        limit the portion of the L3 cache used for SDCI.
> 
> Above needs to change to a generic resctrl fs feature.

Agree. Will rephrase it

> 
>> +
>> +            "0":
>> +                  Feature is not enabled.
>> +            "1":
>> +                  Feature is enabled.
> 
> What does "feature is enabled" and "feature is not enabled" mean to the user?
> What can the user expect to happen after enabling/disabling this feature?

Ok. Will add few more details.

> 
>> +
>> +        Feature can be enabled/disabled by writing to the interface.
>> +        Example::
>> +
>> +            # echo 1 > /sys/fs/resctrl/info/L3/sdciae
>> +
>>   Memory bandwidth(MB) subdirectory contains the following files
>>   with respect to allocation:
>>   diff --git a/arch/x86/kernel/cpu/resctrl/core.c
>> b/arch/x86/kernel/cpu/resctrl/core.c
>> index e4381e3feb75..6a9512008a4a 100644
>> --- a/arch/x86/kernel/cpu/resctrl/core.c
>> +++ b/arch/x86/kernel/cpu/resctrl/core.c
>> @@ -299,6 +299,7 @@ static void rdt_get_cdp_config(int level)
>>   static void rdt_get_sdciae_alloc_cfg(struct rdt_resource *r)
>>   {
>>       r->sdciae_capable = true;
>> +    resctrl_sdciae_rftype_init();
>>   }
>>     static void rdt_get_cdp_l3_config(void)
>> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h
>> b/arch/x86/kernel/cpu/resctrl/internal.h
>> index ceb0e8e1ed76..9a3da6d49144 100644
>> --- a/arch/x86/kernel/cpu/resctrl/internal.h
>> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
>> @@ -662,6 +662,7 @@ void rdt_domain_reconfigure_cdp(struct rdt_resource
>> *r);
>>   void __init thread_throttle_mode_init(void);
>>   void __init mbm_config_rftype_init(const char *config);
>>   void rdt_staged_configs_clear(void);
>> +void __init resctrl_sdciae_rftype_init(void);
>>   bool closid_allocated(unsigned int closid);
>>   int resctrl_find_cleanest_closid(void);
>>   diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> index c62d6183bfe4..58e4df195207 100644
>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> @@ -171,6 +171,27 @@ void closid_free(int closid)
>>       __set_bit(closid, &closid_free_map);
>>   }
>>   +/*
>> + * SDCIAE feature uses max CLOSID to route the SDCI traffic.
>> + * Get the max CLOSID number
>> + */
>> +static u32 get_sdciae_closid(struct rdt_resource *r)
>> +{
>> +    return resctrl_arch_get_num_closid(r) - 1;
>> +}
>> +
>> +static int closid_alloc_sdciae(struct rdt_resource *r)
>> +{
>> +    u32 sdciae_closid = get_sdciae_closid(r);
>> +
>> +    if (closid_free_map & (1 << sdciae_closid)) {
>> +        closid_free_map &= ~(1 << sdciae_closid);
>> +        return sdciae_closid;
>> +    } else {
>> +        return -ENOSPC;
>> +    }
>> +}
> 
> How does this interact with CDP? It seems to me that the above would
> cause overflow on a CDP system since the closid_free_map is sized to
> half of what the hardware supports. This also seems to still allow
> creating resource groups that may end up using the CLOSID dedicated
> to SDCIAE here since the (when CDP enabled) resource groups use
> software closid and then hardware configuration is done with the
> hardware closid. When CDP is enabled it seems possible to still
> create a resource group and modify the CBM of what is then intended to
> be sdciae allocations?

Yes. This code is problematic when CDP is enabled.

Hardware techically supports both CDP and SDCIAE together. In that case
SDCIAE mask is shared with COS=7 Instruction mask.

We have couple of options.
1. Dont allow SDCIAE when CDP is enabled.
2. Modify the code to handle both CDP and SDCIAE. Add documentation to
clarify about sharing of SDCIAE and CDP masks.

I would think option 2 may be better.

> 
> Also, please be consistent with function naming, note how the above
> two functions differ wrt namespace and verb. resctrl is surely not
> consistent in this regard but it really helps to have partner functions
> look similar. For example, this could be "feature_closid_get()" and
> "feature_closid_alloc()".
> 

Sure.

> Also, there looks to be opportunity to use bitops here ... perhaps
> __test_and_clear_bit()?

Sure.

> 
> 
>> +
>>   /**
>>    * closid_allocated - test if provided closid is in use
>>    * @closid: closid to be tested
>> @@ -1850,6 +1871,57 @@ int resctrl_arch_set_sdciae_enabled(enum
>> resctrl_res_level l, bool enable)
>>       return 0;
>>   }
>>   +static int resctrl_sdciae_show(struct kernfs_open_file *of,
>> +                   struct seq_file *seq, void *v)
>> +{
>> +    seq_printf(seq, "%x\n",
>> resctrl_arch_get_sdciae_enabled(RDT_RESOURCE_L3));
>> +    return 0;
>> +}
> 
> This does not look right ... this file has flags "RFTYPE_CTRL_INFO |
> RFTYPE_RES_CACHE"
> which means that it will be created for all CAT resources. So, on a system
> that supports
> L2 CAT, the "sdciae" file will be created for the L2 resource and it will
> show whether
> "sdciae" is enabled on the *L3* resource?

Yes. I will have to get it from resctrl_schema. Good catch.

struct resctrl_schema *s = of->kn->parent->priv;
struct rdt_resource *r = s->res;

seq_printf(seq, "%x\n", resctrl_arch_get_sdciae_enabled(r->rid));
return 0;

> 
>> +
>> +static ssize_t resctrl_sdciae_write(struct kernfs_open_file *of, char
>> *buf,
>> +                    size_t nbytes, loff_t off)
>> +{
>> +    struct resctrl_schema *s = of->kn->parent->priv;
>> +    struct rdt_resource *r = s->res;
>> +    unsigned int enable;
>> +    u32 sdciae_closid;
>> +    int ret;
>> +
>> +    if (!r->sdciae_capable)
>> +        return -EINVAL;
>> +
>> +    ret = kstrtouint(buf, 0, &enable);
> 
> How about kstrtobool()? This will make things more consistent with
> resctrl_arch_set_sdciae_enabled() expecting a bool. Or should we be
> looking ahead
> at this file later possibly needing to support more integers to activate
> more capabilities
> related to this feature? In that case this implementation needs to take
> care since instead
> of supporting "0" and "1" it supports "0" and "anything but 0" that
> prevents any such
> future enhancements.

I will change it kstrtobool().

> 
> 
>> +    if (ret)
>> +        return ret;
>> +
>> +    cpus_read_lock();
>> +    mutex_lock(&rdtgroup_mutex);
>> +
>> +    rdt_last_cmd_clear();
>> +
>> +    /* Update the MSR only when there is a change */
> 
> The resctrl fs cannot make predictions on what arch code needs to do to
> enable feature.

Sure. Will remove it.

> 
>> +    if (resctrl_arch_get_sdciae_enabled(RDT_RESOURCE_L3) != enable) {
> 
> (same issue with this file being present under L2 resource creating
> confusion)

Ok. Will address it.

> 
>> +        if (enable) {
>> +            ret = closid_alloc_sdciae(r);
>> +            if (ret < 0) {
>> +                rdt_last_cmd_puts("SDCIAE CLOSID is not available\n");
>> +                goto out_sdciae;
>> +            }
>> +        } else {
>> +            sdciae_closid = get_sdciae_closid(r);
>> +            closid_free(sdciae_closid);
>> +        }
> 
> 
>> +
>> +        ret = resctrl_arch_set_sdciae_enabled(RDT_RESOURCE_L3, enable);
> 
> I assume that once SDCIAE is enabled the I/O traffic will start flowing to
> whatever
> was the last CBM of the max CLOSID? Is this intended or should there be
> some default
> CBM that this feature should start with?

It will start with whatever the last CBM for max CLOSID.

> 
>> +    }
>> +
>> +out_sdciae:
>> +    mutex_unlock(&rdtgroup_mutex);
>> +    cpus_read_unlock();
>> +
>> +    return ret ?: nbytes;
>> +}
>> +
>>   /* rdtgroup information files for one cache resource. */
>>   static struct rftype res_common_files[] = {
>>       {
>> @@ -2002,6 +2074,13 @@ static struct rftype res_common_files[] = {
>>           .seq_show    = rdtgroup_schemata_show,
>>           .fflags        = RFTYPE_CTRL_BASE,
>>       },
>> +    {
>> +        .name        = "sdciae",
>> +        .mode        = 0644,
>> +        .kf_ops        = &rdtgroup_kf_single_ops,
>> +        .seq_show    = resctrl_sdciae_show,
>> +        .write        = resctrl_sdciae_write,
>> +    },
>>       {
>>           .name        = "mode",
>>           .mode        = 0644,
>> @@ -2101,6 +2180,15 @@ void __init mbm_config_rftype_init(const char
>> *config)
>>           rft->fflags = RFTYPE_MON_INFO | RFTYPE_RES_CACHE;
>>   }
>>   +void __init resctrl_sdciae_rftype_init(void)
>> +{
>> +    struct rftype *rft;
>> +
>> +    rft = rdtgroup_get_rftype_by_name("sdciae");
>> +    if (rft)
>> +        rft->fflags = RFTYPE_CTRL_INFO | RFTYPE_RES_CACHE;
>> +}
>> +
> 
> Another instance of the pattern that is impacted by the ABMC and MPAM work.

Yes. Agree.

> 
>>   /**
>>    * rdtgroup_kn_mode_restrict - Restrict user access to named resctrl file
>>    * @r: The resource group with which the file is associated.
> 
> Reinette
> 

-- 
Thanks
Babu Moger

Re: [PATCH 6/7] x86/resctrl: Introduce interface to display SDCIAE Capacity Bit Masks

[Moger, Babu]

Hi Reinette,

On 9/13/24 15:52, Reinette Chatre wrote:
> Hi Babu,
> 
> On 8/16/24 9:16 AM, Babu Moger wrote:
>> When enabled, SDCIAE forces all SDCI lines to be placed into the L3 cache
>> partitions identified by the highest-supported L3_MASK_n register where
>> n is the maximum CLOSID supported.
>>
>> Add the interface to display CBMs (Capacity Bit Mask) of the SDCIAE.
>>
>> Signed-off-by: Babu Moger <babu.moger@amd.com>
>> ---
>>   arch/x86/kernel/cpu/resctrl/ctrlmondata.c |  2 +-
>>   arch/x86/kernel/cpu/resctrl/internal.h    |  1 +
>>   arch/x86/kernel/cpu/resctrl/rdtgroup.c    | 29 +++++++++++++++++++++++
>>   3 files changed, 31 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
>> b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
>> index 50fa1fe9a073..fc99f4d17e6c 100644
>> --- a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
>> +++ b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
>> @@ -439,7 +439,7 @@ u32 resctrl_arch_get_config(struct rdt_resource *r,
>> struct rdt_ctrl_domain *d,
>>       return hw_dom->ctrl_val[idx];
>>   }
>>   -static void show_doms(struct seq_file *s, struct resctrl_schema
>> *schema, int closid)
>> +void show_doms(struct seq_file *s, struct resctrl_schema *schema, int
>> closid)
>>   {
>>       struct rdt_resource *r = schema->res;
>>       struct rdt_ctrl_domain *dom;
>> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h
>> b/arch/x86/kernel/cpu/resctrl/internal.h
>> index 9a3da6d49144..f2c87ca37b13 100644
>> --- a/arch/x86/kernel/cpu/resctrl/internal.h
>> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
>> @@ -663,6 +663,7 @@ void __init thread_throttle_mode_init(void);
>>   void __init mbm_config_rftype_init(const char *config);
>>   void rdt_staged_configs_clear(void);
>>   void __init resctrl_sdciae_rftype_init(void);
>> +void show_doms(struct seq_file *s, struct resctrl_schema *schema, int
>> closid);
>>   bool closid_allocated(unsigned int closid);
>>   int resctrl_find_cleanest_closid(void);
>>   diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> index 58e4df195207..51bc715bb6ae 100644
>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> @@ -1922,6 +1922,25 @@ static ssize_t resctrl_sdciae_write(struct
>> kernfs_open_file *of, char *buf,
>>       return ret ?: nbytes;
>>   }
>>   +static int resctrl_sdciae_cbm_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;
>> +    u32 sdciae_closid;
>> +
>> +    if (!resctrl_arch_get_sdciae_enabled(RDT_RESOURCE_L3)) {
> 
> (same issue with this file appearing in all cache resources, eg. L2)

Sure. Will address it.

> 
>> +        rdt_last_cmd_puts("SDCIAE is not enabled\n");
>> +        return -EINVAL;
>> +    }
>> +
>> +    sdciae_closid = get_sdciae_closid(r);
>> +
>> +    show_doms(seq, s, sdciae_closid);
>> +
>> +    return 0;
>> +}
> 
> This really needs protection. (You should have encountered warnings
> via lockdep_assert_held(&rdtgroup_mutex) and lockdep_assert_cpus_held()
> found in the functions called.)


Sure. Will fix it.

>> +
>>   /* rdtgroup information files for one cache resource. */
>>   static struct rftype res_common_files[] = {
>>       {
>> @@ -2081,6 +2100,12 @@ static struct rftype res_common_files[] = {
>>           .seq_show    = resctrl_sdciae_show,
>>           .write        = resctrl_sdciae_write,
>>       },
>> +    {
>> +        .name        = "sdciae_cbm",
>> +        .mode        = 0444,
>> +        .kf_ops        = &rdtgroup_kf_single_ops,
>> +        .seq_show    = resctrl_sdciae_cbm_show,
>> +    },
>>       {
>>           .name        = "mode",
>>           .mode        = 0644,
>> @@ -2187,6 +2212,10 @@ void __init resctrl_sdciae_rftype_init(void)
>>       rft = rdtgroup_get_rftype_by_name("sdciae");
>>       if (rft)
>>           rft->fflags = RFTYPE_CTRL_INFO | RFTYPE_RES_CACHE;
>> +
>> +    rft = rdtgroup_get_rftype_by_name("sdciae_cbm");
>> +    if (rft)
>> +        rft->fflags = RFTYPE_CTRL_INFO | RFTYPE_RES_CACHE;
>>   }
>>     /**
> 
> Reinette
> 

-- 
Thanks
Babu Moger

Re: [PATCH 3/7] x86/resctrl: Introduce sdciae_capable in rdt_resource

[Reinette Chatre]

Hi Babu,

On 9/18/24 11:22 AM, Moger, Babu wrote:
> 
> 
> On 9/18/24 10:27, Moger, Babu wrote:
>> Hi Reinette,
>>
>> On 9/13/24 15:45, Reinette Chatre wrote:
>>> Hi Babu,
>>>
>>> On 8/16/24 9:16 AM, Babu Moger wrote:
>>>> Detect SDCIAE`(L3 Smart Data Cache Injection Allocation Enforcement)
>>>
>>> (stray ` char)
>>
>> Sure.
>>
>>>
>>>> feature and initialize sdciae_capable.
>>>
>>> (This is a repeat of the discussion we had surrounding the ABMC feature.)
>>>
>>> By adding "sdciae_capable" to struct rdt_resource the "sdciae" feature
>>> becomes a resctrl fs feature. Any other architecture that has a "similar
>>> but perhaps not identical feature to AMD's SDCIAE" will be forced to also
>>> call it "sdciae" ... sdciae seems like a marketing name to me and resctrl
>>> needs something generic that could later be built on (if needed) by other
>>> architectures.
>>
>> How about "cache_inject_capable" ?
>>
>> This seems generic. I will change the description also.
>>
> 
> Basically, this feature reserves specific CLOS for SDCI cache.
> 
> We can also name "clos_reserve_capable".

Naming is always complicated. I think we should try to stay away from
"clos" in a generic name since that creates problem when trying to
apply it to Arm and is very specific to how AMD implements this
feature. "cache_inject_capable" does sound much better to me ...
it also looks like this may be more appropriate as a property
of struct resctrl_cache?

Reinette

Re: [PATCH 4/7] x86/resctrl: Implement SDCIAE enable/disable

[Reinette Chatre]

Hi Babu,

On 9/18/24 9:26 AM, Moger, Babu wrote:
> On 9/13/24 15:46, Reinette Chatre wrote:
>> On 8/16/24 9:16 AM, Babu Moger wrote:
>>> SDCIAE feature can be enabled by setting bit 1 in MSR L3_QOS_EXT_CFG.
>>> When the state of SDCIAE is changed, it must be changed to the updated
>>> value on all logical processors in the QOS Domain. By default, the SDCIAE
>>> feature is disabled.
>>>
>>> Introduce arch handlers to detect and enable/disable the feature.
>>>
>>> The SDCIAE feature details are available in APM listed below [1].
>>> [1] AMD64 Architecture Programmer's Manual Volume 2: System Programming
>>> Publication # 24593 Revision 3.41 section 19.4.7 L3 Smart Data Cache
>>> Injection Allocation Enforcement (SDCIAE)
>>>
>>> Signed-off-by: Babu Moger <babu.moger@amd.com>
>>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=206537
>>> ---
>>>   arch/x86/include/asm/msr-index.h       |  1 +
>>>   arch/x86/kernel/cpu/resctrl/internal.h | 12 +++++
>>>   arch/x86/kernel/cpu/resctrl/rdtgroup.c | 61 ++++++++++++++++++++++++++
>>>   3 files changed, 74 insertions(+)
>>>
>>> diff --git a/arch/x86/include/asm/msr-index.h
>>> b/arch/x86/include/asm/msr-index.h
>>> index 82c6a4d350e0..c78afed3c21f 100644
>>> --- a/arch/x86/include/asm/msr-index.h
>>> +++ b/arch/x86/include/asm/msr-index.h
>>> @@ -1181,6 +1181,7 @@
>>>   /* - AMD: */
>>>   #define MSR_IA32_MBA_BW_BASE        0xc0000200
>>>   #define MSR_IA32_SMBA_BW_BASE        0xc0000280
>>> +#define MSR_IA32_L3_QOS_EXT_CFG        0xc00003ff
>>>   #define MSR_IA32_EVT_CFG_BASE        0xc0000400
>>>     /* MSR_IA32_VMX_MISC bits */
>>> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h
>>> b/arch/x86/kernel/cpu/resctrl/internal.h
>>> index 955999aecfca..ceb0e8e1ed76 100644
>>> --- a/arch/x86/kernel/cpu/resctrl/internal.h
>>> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
>>> @@ -56,6 +56,9 @@
>>>   /* Max event bits supported */
>>>   #define MAX_EVT_CONFIG_BITS        GENMASK(6, 0)
>>>   +/* Setting bit 1 in L3_QOS_EXT_CFG enables the SDCIAE feature. */
>>> +#define SDCIAE_ENABLE_BIT        1
>>> +
>>>   /**
>>>    * cpumask_any_housekeeping() - Choose any CPU in @mask, preferring
>>> those that
>>>    *                    aren't marked nohz_full
>>> @@ -477,6 +480,7 @@ struct rdt_parse_data {
>>>    * @mbm_cfg_mask:    Bandwidth sources that can be tracked when Bandwidth
>>>    *            Monitoring Event Configuration (BMEC) is supported.
>>>    * @cdp_enabled:    CDP state of this resource
>>> + * @sdciae_enabled:    SDCIAE feature is enabled
>>>    *
>>>    * Members of this structure are either private to the architecture
>>>    * e.g. mbm_width, or accessed via helpers that provide abstraction. e.g.
>>> @@ -491,6 +495,7 @@ struct rdt_hw_resource {
>>>       unsigned int        mbm_width;
>>>       unsigned int        mbm_cfg_mask;
>>>       bool            cdp_enabled;
>>> +    bool            sdciae_enabled;
>>>   };
>>>     static inline struct rdt_hw_resource *resctrl_to_arch_res(struct
>>> rdt_resource *r)
>>> @@ -536,6 +541,13 @@ int resctrl_arch_set_cdp_enabled(enum
>>> resctrl_res_level l, bool enable);
>>>     void arch_mon_domain_online(struct rdt_resource *r, struct
>>> rdt_mon_domain *d);
>>>   +static inline bool resctrl_arch_get_sdciae_enabled(enum
>>> resctrl_res_level l)
>>> +{
>>> +    return rdt_resources_all[l].sdciae_enabled;
>>> +}
>>> +
>>> +int resctrl_arch_set_sdciae_enabled(enum resctrl_res_level l, bool
>>> enable);
>>> +
>>>   /*
>>>    * To return the common struct rdt_resource, which is contained in struct
>>>    * rdt_hw_resource, walk the resctrl member of struct rdt_hw_resource.
>>> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>>> b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>>> index d7163b764c62..c62d6183bfe4 100644
>>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>>> @@ -1789,6 +1789,67 @@ static ssize_t
>>> mbm_local_bytes_config_write(struct kernfs_open_file *of,
>>>       return ret ?: nbytes;
>>>   }
>>>   +static void resctrl_sdciae_msrwrite(void *arg)
>>> +{
>>> +    bool *enable = arg;
>>> +
>>> +    if (*enable)
>>> +        msr_set_bit(MSR_IA32_L3_QOS_EXT_CFG, SDCIAE_ENABLE_BIT);
>>> +    else
>>> +        msr_clear_bit(MSR_IA32_L3_QOS_EXT_CFG, SDCIAE_ENABLE_BIT);
>>> +}
>>
>> (hmmm ... so there is an effort to make the rest of the code not be
>> resource specific ... but then the lowest level has L3 MSR hardcoded)
> 
> Not very clear on this.

The flow starts with functions called with L3 resource as parameter
while the final function hardcodes programming of L3 resource
specific MSR making an L3 resource the only supported parameter to begin with.

> 
> I can separate the patch into two, one arch specific and the other FS
> specific.
> 
>>
>>> +
>>> +static int resctrl_sdciae_setup(enum resctrl_res_level l, bool enable)
>>> +{
>>> +    struct rdt_resource *r = &rdt_resources_all[l].r_resctrl;
>>> +    struct rdt_ctrl_domain *d;
>>> +
>>> +    /* Update  L3_QOS_EXT_CFG MSR on all the CPUs in all domains*/
>>
>> (please note some space issues above)
> 
> Sure.
> 
>>
>>> +    list_for_each_entry(d, &r->ctrl_domains, hdr.list)
>>> +        on_each_cpu_mask(&d->hdr.cpu_mask, resctrl_sdciae_msrwrite,
>>> &enable, 1);
>>> +
>>> +    return 0;
>>
>> It seems that this will be inside the arch specific code while
>> resctrl_arch_set_sdciae_enabled() will be called by resctrl fs code. It is
>> thus not clear why above returns an int, thus forcing callers to do
>> error code handling, when it will always just return 0.
> 
> Yes. It is returning 0 right now. Keeps the options open for other arch's
> report error.  Looks like we heading to make this generic feature.

This is arch specific code though ... other archs will not call this function,
other archs call (to be renamed) resctrl_arch_set_sdciae_enabled(). As I
am looking at the ABMC work also this can benefit from using appropriate
namespaces ... only use "resctrl_" prefix for fs code and then this should
be more clear.

>>> +}
>>> +
>>> +static int resctrl_sdciae_enable(enum resctrl_res_level l)
>>> +{
>>> +    struct rdt_hw_resource *hw_res = &rdt_resources_all[l];
>>> +    int ret = 0;
>>> +
>>> +    if (!hw_res->sdciae_enabled) {
>>> +        ret = resctrl_sdciae_setup(l, true);
>>> +        if (!ret)
>>> +            hw_res->sdciae_enabled = true;
>>> +    }
>>> +
>>> +    return ret;
>>
>> Same here ... this will always return 0, no?
> 
> Yes. it is returns 0 always on AMD. Keeps the options open for other
> arch's report error.

This is the arch specific (note the access to struct rdt_hw_resource) AMD callback.
The function where the return code should be maintained in support of other archs is 
resctrl_arch_set_sdciae_enabled().

Reinette

Re: [PATCH 5/7] x86/resctrl: Add interface to enable/disable SDCIAE

[Reinette Chatre]

Hi Babu,

On 9/18/24 1:10 PM, Moger, Babu wrote:
> On 9/13/24 15:51, Reinette Chatre wrote:
>> On 8/16/24 9:16 AM, Babu Moger wrote:

...

>>> +        if (enable) {
>>> +            ret = closid_alloc_sdciae(r);
>>> +            if (ret < 0) {
>>> +                rdt_last_cmd_puts("SDCIAE CLOSID is not available\n");
>>> +                goto out_sdciae;
>>> +            }
>>> +        } else {
>>> +            sdciae_closid = get_sdciae_closid(r);
>>> +            closid_free(sdciae_closid);
>>> +        }
>>
>>
>>> +
>>> +        ret = resctrl_arch_set_sdciae_enabled(RDT_RESOURCE_L3, enable);
>>
>> I assume that once SDCIAE is enabled the I/O traffic will start flowing to
>> whatever
>> was the last CBM of the max CLOSID? Is this intended or should there be
>> some default
>> CBM that this feature should start with?
> 
> It will start with whatever the last CBM for max CLOSID.

This seems arbitrary based on whatever allocation the previous resource group
using the CLOSID has. When a new resource group is created resctrl ensures
that it is created with all usable allocations, see rdtgroup_init_cat().
Letting cache injection start with whatever allocation remnant programmed
in a register does not seem ideal. What if, for example, after that resource
group was removed, a new exclusive resource group was created that overlaps
with that allocation? 

Reinette

Re: [PATCH 3/7] x86/resctrl: Introduce sdciae_capable in rdt_resource

[Moger, Babu]

Hi Reinette,

On 9/19/2024 10:33 AM, Reinette Chatre wrote:
> Hi Babu,
> 
> On 9/18/24 11:22 AM, Moger, Babu wrote:
>>
>>
>> On 9/18/24 10:27, Moger, Babu wrote:
>>> Hi Reinette,
>>>
>>> On 9/13/24 15:45, Reinette Chatre wrote:
>>>> Hi Babu,
>>>>
>>>> On 8/16/24 9:16 AM, Babu Moger wrote:
>>>>> Detect SDCIAE`(L3 Smart Data Cache Injection Allocation Enforcement)
>>>>
>>>> (stray ` char)
>>>
>>> Sure.
>>>
>>>>
>>>>> feature and initialize sdciae_capable.
>>>>
>>>> (This is a repeat of the discussion we had surrounding the ABMC feature.)
>>>>
>>>> By adding "sdciae_capable" to struct rdt_resource the "sdciae" feature
>>>> becomes a resctrl fs feature. Any other architecture that has a "similar
>>>> but perhaps not identical feature to AMD's SDCIAE" will be forced to also
>>>> call it "sdciae" ... sdciae seems like a marketing name to me and resctrl
>>>> needs something generic that could later be built on (if needed) by other
>>>> architectures.
>>>
>>> How about "cache_inject_capable" ?
>>>
>>> This seems generic. I will change the description also.
>>>
>>
>> Basically, this feature reserves specific CLOS for SDCI cache.
>>
>> We can also name "clos_reserve_capable".
> 
> Naming is always complicated. I think we should try to stay away from
> "clos" in a generic name since that creates problem when trying to
> apply it to Arm and is very specific to how AMD implements this
> feature. "cache_inject_capable" does sound much better to me ...

ok, Sure.

> it also looks like this may be more appropriate as a property
> of struct resctrl_cache?
> 
Yes. Sure. We can do that.

-- 
- Babu Moger

Re: [PATCH 4/7] x86/resctrl: Implement SDCIAE enable/disable

[Moger, Babu]

Hi Reinette,

On 9/19/2024 10:34 AM, Reinette Chatre wrote:
> Hi Babu,
> 
> On 9/18/24 9:26 AM, Moger, Babu wrote:
>> On 9/13/24 15:46, Reinette Chatre wrote:
>>> On 8/16/24 9:16 AM, Babu Moger wrote:
>>>> SDCIAE feature can be enabled by setting bit 1 in MSR L3_QOS_EXT_CFG.
>>>> When the state of SDCIAE is changed, it must be changed to the updated
>>>> value on all logical processors in the QOS Domain. By default, the SDCIAE
>>>> feature is disabled.
>>>>
>>>> Introduce arch handlers to detect and enable/disable the feature.
>>>>
>>>> The SDCIAE feature details are available in APM listed below [1].
>>>> [1] AMD64 Architecture Programmer's Manual Volume 2: System Programming
>>>> Publication # 24593 Revision 3.41 section 19.4.7 L3 Smart Data Cache
>>>> Injection Allocation Enforcement (SDCIAE)
>>>>
>>>> Signed-off-by: Babu Moger <babu.moger@amd.com>
>>>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=206537
>>>> ---
>>>>    arch/x86/include/asm/msr-index.h       |  1 +
>>>>    arch/x86/kernel/cpu/resctrl/internal.h | 12 +++++
>>>>    arch/x86/kernel/cpu/resctrl/rdtgroup.c | 61 ++++++++++++++++++++++++++
>>>>    3 files changed, 74 insertions(+)
>>>>
>>>> diff --git a/arch/x86/include/asm/msr-index.h
>>>> b/arch/x86/include/asm/msr-index.h
>>>> index 82c6a4d350e0..c78afed3c21f 100644
>>>> --- a/arch/x86/include/asm/msr-index.h
>>>> +++ b/arch/x86/include/asm/msr-index.h
>>>> @@ -1181,6 +1181,7 @@
>>>>    /* - AMD: */
>>>>    #define MSR_IA32_MBA_BW_BASE        0xc0000200
>>>>    #define MSR_IA32_SMBA_BW_BASE        0xc0000280
>>>> +#define MSR_IA32_L3_QOS_EXT_CFG        0xc00003ff
>>>>    #define MSR_IA32_EVT_CFG_BASE        0xc0000400
>>>>      /* MSR_IA32_VMX_MISC bits */
>>>> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h
>>>> b/arch/x86/kernel/cpu/resctrl/internal.h
>>>> index 955999aecfca..ceb0e8e1ed76 100644
>>>> --- a/arch/x86/kernel/cpu/resctrl/internal.h
>>>> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
>>>> @@ -56,6 +56,9 @@
>>>>    /* Max event bits supported */
>>>>    #define MAX_EVT_CONFIG_BITS        GENMASK(6, 0)
>>>>    +/* Setting bit 1 in L3_QOS_EXT_CFG enables the SDCIAE feature. */
>>>> +#define SDCIAE_ENABLE_BIT        1
>>>> +
>>>>    /**
>>>>     * cpumask_any_housekeeping() - Choose any CPU in @mask, preferring
>>>> those that
>>>>     *                    aren't marked nohz_full
>>>> @@ -477,6 +480,7 @@ struct rdt_parse_data {
>>>>     * @mbm_cfg_mask:    Bandwidth sources that can be tracked when Bandwidth
>>>>     *            Monitoring Event Configuration (BMEC) is supported.
>>>>     * @cdp_enabled:    CDP state of this resource
>>>> + * @sdciae_enabled:    SDCIAE feature is enabled
>>>>     *
>>>>     * Members of this structure are either private to the architecture
>>>>     * e.g. mbm_width, or accessed via helpers that provide abstraction. e.g.
>>>> @@ -491,6 +495,7 @@ struct rdt_hw_resource {
>>>>        unsigned int        mbm_width;
>>>>        unsigned int        mbm_cfg_mask;
>>>>        bool            cdp_enabled;
>>>> +    bool            sdciae_enabled;
>>>>    };
>>>>      static inline struct rdt_hw_resource *resctrl_to_arch_res(struct
>>>> rdt_resource *r)
>>>> @@ -536,6 +541,13 @@ int resctrl_arch_set_cdp_enabled(enum
>>>> resctrl_res_level l, bool enable);
>>>>      void arch_mon_domain_online(struct rdt_resource *r, struct
>>>> rdt_mon_domain *d);
>>>>    +static inline bool resctrl_arch_get_sdciae_enabled(enum
>>>> resctrl_res_level l)
>>>> +{
>>>> +    return rdt_resources_all[l].sdciae_enabled;
>>>> +}
>>>> +
>>>> +int resctrl_arch_set_sdciae_enabled(enum resctrl_res_level l, bool
>>>> enable);
>>>> +
>>>>    /*
>>>>     * To return the common struct rdt_resource, which is contained in struct
>>>>     * rdt_hw_resource, walk the resctrl member of struct rdt_hw_resource.
>>>> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>>>> b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>>>> index d7163b764c62..c62d6183bfe4 100644
>>>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>>>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>>>> @@ -1789,6 +1789,67 @@ static ssize_t
>>>> mbm_local_bytes_config_write(struct kernfs_open_file *of,
>>>>        return ret ?: nbytes;
>>>>    }
>>>>    +static void resctrl_sdciae_msrwrite(void *arg)
>>>> +{
>>>> +    bool *enable = arg;
>>>> +
>>>> +    if (*enable)
>>>> +        msr_set_bit(MSR_IA32_L3_QOS_EXT_CFG, SDCIAE_ENABLE_BIT);
>>>> +    else
>>>> +        msr_clear_bit(MSR_IA32_L3_QOS_EXT_CFG, SDCIAE_ENABLE_BIT);
>>>> +}
>>>
>>> (hmmm ... so there is an effort to make the rest of the code not be
>>> resource specific ... but then the lowest level has L3 MSR hardcoded)
>>
>> Not very clear on this.
> 
> The flow starts with functions called with L3 resource as parameter
> while the final function hardcodes programming of L3 resource
> specific MSR making an L3 resource the only supported parameter to begin with.

Oh ok. Got it.

> 
>>
>> I can separate the patch into two, one arch specific and the other FS
>> specific.
>>
>>>
>>>> +
>>>> +static int resctrl_sdciae_setup(enum resctrl_res_level l, bool enable)
>>>> +{
>>>> +    struct rdt_resource *r = &rdt_resources_all[l].r_resctrl;
>>>> +    struct rdt_ctrl_domain *d;
>>>> +
>>>> +    /* Update  L3_QOS_EXT_CFG MSR on all the CPUs in all domains*/
>>>
>>> (please note some space issues above)
>>
>> Sure.
>>
>>>
>>>> +    list_for_each_entry(d, &r->ctrl_domains, hdr.list)
>>>> +        on_each_cpu_mask(&d->hdr.cpu_mask, resctrl_sdciae_msrwrite,
>>>> &enable, 1);
>>>> +
>>>> +    return 0;
>>>
>>> It seems that this will be inside the arch specific code while
>>> resctrl_arch_set_sdciae_enabled() will be called by resctrl fs code. It is
>>> thus not clear why above returns an int, thus forcing callers to do
>>> error code handling, when it will always just return 0.
>>
>> Yes. It is returning 0 right now. Keeps the options open for other arch's
>> report error.  Looks like we heading to make this generic feature.
> 
> This is arch specific code though ... other archs will not call this function,
> other archs call (to be renamed) resctrl_arch_set_sdciae_enabled(). As I
> am looking at the ABMC work also this can benefit from using appropriate
> namespaces ... only use "resctrl_" prefix for fs code and then this should
> be more clear.

Sure. I can make code inside the arch to return void.

> 
>>>> +}
>>>> +
>>>> +static int resctrl_sdciae_enable(enum resctrl_res_level l)
>>>> +{
>>>> +    struct rdt_hw_resource *hw_res = &rdt_resources_all[l];
>>>> +    int ret = 0;
>>>> +
>>>> +    if (!hw_res->sdciae_enabled) {
>>>> +        ret = resctrl_sdciae_setup(l, true);
>>>> +        if (!ret)
>>>> +            hw_res->sdciae_enabled = true;
>>>> +    }
>>>> +
>>>> +    return ret;
>>>
>>> Same here ... this will always return 0, no?
>>
>> Yes. it is returns 0 always on AMD. Keeps the options open for other
>> arch's report error.
> 
> This is the arch specific (note the access to struct rdt_hw_resource) AMD callback.
> The function where the return code should be maintained in support of other archs is
> resctrl_arch_set_sdciae_enabled().

Sure. Got it.
- Babu Moger

Re: [PATCH 5/7] x86/resctrl: Add interface to enable/disable SDCIAE

[Moger, Babu]

Hi Reinette,

Noticed I didn't respond to this comment.

On 9/19/24 10:35, Reinette Chatre wrote:
> Hi Babu,
> 
> On 9/18/24 1:10 PM, Moger, Babu wrote:
>> On 9/13/24 15:51, Reinette Chatre wrote:
>>> On 8/16/24 9:16 AM, Babu Moger wrote:
> 
> ...
> 
>>>> +        if (enable) {
>>>> +            ret = closid_alloc_sdciae(r);
>>>> +            if (ret < 0) {
>>>> +                rdt_last_cmd_puts("SDCIAE CLOSID is not available\n");
>>>> +                goto out_sdciae;
>>>> +            }
>>>> +        } else {
>>>> +            sdciae_closid = get_sdciae_closid(r);
>>>> +            closid_free(sdciae_closid);
>>>> +        }
>>>
>>>
>>>> +
>>>> +        ret = resctrl_arch_set_sdciae_enabled(RDT_RESOURCE_L3, enable);
>>>
>>> I assume that once SDCIAE is enabled the I/O traffic will start flowing to
>>> whatever
>>> was the last CBM of the max CLOSID? Is this intended or should there be
>>> some default
>>> CBM that this feature should start with?
>>
>> It will start with whatever the last CBM for max CLOSID.
> 
> This seems arbitrary based on whatever allocation the previous resource group
> using the CLOSID has. When a new resource group is created resctrl ensures
> that it is created with all usable allocations, see rdtgroup_init_cat().

Checked again with with the team here. When SDCIAE is enabled, it uses the
value in L3QosAllocMask15 (value in L3_MASK_15 MSR).  Enabling SDCIAE does
not change the value of L3QosAllocMask15.


> Letting cache injection start with whatever allocation remnant programmed
> in a register does not seem ideal. What if, for example, after that resource
> group was removed, a new exclusive resource group was created that overlaps
> with that allocation? 

In that case. it will share the bit mask with the exclusive group. We may
need to add a text about it.
-- 
Thanks
Babu Moger

Re: [PATCH 3/7] x86/resctrl: Introduce sdciae_capable in rdt_resource

[Moger, Babu]

Hi Reinette,

On 9/19/24 10:33, Reinette Chatre wrote:
> Hi Babu,
> 
> On 9/18/24 11:22 AM, Moger, Babu wrote:
>>
>>
>> On 9/18/24 10:27, Moger, Babu wrote:
>>> Hi Reinette,
>>>
>>> On 9/13/24 15:45, Reinette Chatre wrote:
>>>> Hi Babu,
>>>>
>>>> On 8/16/24 9:16 AM, Babu Moger wrote:
>>>>> Detect SDCIAE`(L3 Smart Data Cache Injection Allocation Enforcement)
>>>>
>>>> (stray ` char)
>>>
>>> Sure.
>>>
>>>>
>>>>> feature and initialize sdciae_capable.
>>>>
>>>> (This is a repeat of the discussion we had surrounding the ABMC feature.)
>>>>
>>>> By adding "sdciae_capable" to struct rdt_resource the "sdciae" feature
>>>> becomes a resctrl fs feature. Any other architecture that has a "similar
>>>> but perhaps not identical feature to AMD's SDCIAE" will be forced to also
>>>> call it "sdciae" ... sdciae seems like a marketing name to me and resctrl
>>>> needs something generic that could later be built on (if needed) by other
>>>> architectures.
>>>
>>> How about "cache_inject_capable" ?
>>>
>>> This seems generic. I will change the description also.
>>>
>>
>> Basically, this feature reserves specific CLOS for SDCI cache.
>>
>> We can also name "clos_reserve_capable".
> 
> Naming is always complicated. I think we should try to stay away from
> "clos" in a generic name since that creates problem when trying to
> apply it to Arm and is very specific to how AMD implements this
> feature. "cache_inject_capable" does sound much better to me ...
> it also looks like this may be more appropriate as a property
> of struct resctrl_cache?

Coming back to this again, I feel 'cache_inject_capable' is kind of very
generic. Cache injection term is used very generically everywhere.

Does  'cache_reserve_capable" sound good ?  This is inside the resctrl
subsystem. We know what it is referring to.

-- 
Thanks
Babu Moger

Re: [PATCH 5/7] x86/resctrl: Add interface to enable/disable SDCIAE

[Reinette Chatre]

Hi Babu,

On 10/15/24 12:25 PM, Moger, Babu wrote:
> Hi Reinette,
> 
> Noticed I didn't respond to this comment.
> 
> On 9/19/24 10:35, Reinette Chatre wrote:
>> Hi Babu,
>>
>> On 9/18/24 1:10 PM, Moger, Babu wrote:
>>> On 9/13/24 15:51, Reinette Chatre wrote:
>>>> On 8/16/24 9:16 AM, Babu Moger wrote:
>>
>> ...
>>
>>>>> +        if (enable) {
>>>>> +            ret = closid_alloc_sdciae(r);
>>>>> +            if (ret < 0) {
>>>>> +                rdt_last_cmd_puts("SDCIAE CLOSID is not available\n");
>>>>> +                goto out_sdciae;
>>>>> +            }
>>>>> +        } else {
>>>>> +            sdciae_closid = get_sdciae_closid(r);
>>>>> +            closid_free(sdciae_closid);
>>>>> +        }
>>>>
>>>>
>>>>> +
>>>>> +        ret = resctrl_arch_set_sdciae_enabled(RDT_RESOURCE_L3, enable);
>>>>
>>>> I assume that once SDCIAE is enabled the I/O traffic will start flowing to
>>>> whatever
>>>> was the last CBM of the max CLOSID? Is this intended or should there be
>>>> some default
>>>> CBM that this feature should start with?
>>>
>>> It will start with whatever the last CBM for max CLOSID.
>>
>> This seems arbitrary based on whatever allocation the previous resource group
>> using the CLOSID has. When a new resource group is created resctrl ensures
>> that it is created with all usable allocations, see rdtgroup_init_cat().
> 
> Checked again with with the team here. When SDCIAE is enabled, it uses the
> value in L3QosAllocMask15 (value in L3_MASK_15 MSR).  Enabling SDCIAE does
> not change the value of L3QosAllocMask15.

I see the issue as similar to how resource group allocations are managed.
Just like resctrl ensures that when a new resource group is created, it is done
with maximum allocations that the resource group may use ... not the allocations
left over from the previous resource group that used those MSRs.

I understand that the hardware uses L3QosAllocMask15 and does not change
L3QosAllocMask15 when SDCIAE is enabled, but resctrl is in a position to initialize
those registers at the time when SDCIAE is initialized to create a sane default
allocation, not an allocation of whatever happened to be in MSR at that time.

>> Letting cache injection start with whatever allocation remnant programmed
>> in a register does not seem ideal. What if, for example, after that resource
>> group was removed, a new exclusive resource group was created that overlaps
>> with that allocation? 
> 
> In that case. it will share the bit mask with the exclusive group. We may
> need to add a text about it.

No. This can be avoided entirely when resctrl initializes the MSR to a sane
default, no?

Reinette

Re: [PATCH 3/7] x86/resctrl: Introduce sdciae_capable in rdt_resource

[Reinette Chatre]

Hi Babu,

On 10/15/24 1:40 PM, Moger, Babu wrote:
> On 9/19/24 10:33, Reinette Chatre wrote:
>> On 9/18/24 11:22 AM, Moger, Babu wrote:
>>> On 9/18/24 10:27, Moger, Babu wrote:
>>>> On 9/13/24 15:45, Reinette Chatre wrote:
>>>>> On 8/16/24 9:16 AM, Babu Moger wrote:
>>>>>> Detect SDCIAE`(L3 Smart Data Cache Injection Allocation Enforcement)
>>>>>
>>>>> (stray ` char)
>>>>
>>>> Sure.
>>>>
>>>>>
>>>>>> feature and initialize sdciae_capable.
>>>>>
>>>>> (This is a repeat of the discussion we had surrounding the ABMC feature.)
>>>>>
>>>>> By adding "sdciae_capable" to struct rdt_resource the "sdciae" feature
>>>>> becomes a resctrl fs feature. Any other architecture that has a "similar
>>>>> but perhaps not identical feature to AMD's SDCIAE" will be forced to also
>>>>> call it "sdciae" ... sdciae seems like a marketing name to me and resctrl
>>>>> needs something generic that could later be built on (if needed) by other
>>>>> architectures.
>>>>
>>>> How about "cache_inject_capable" ?
>>>>
>>>> This seems generic. I will change the description also.
>>>>
>>>
>>> Basically, this feature reserves specific CLOS for SDCI cache.
>>>
>>> We can also name "clos_reserve_capable".
>>
>> Naming is always complicated. I think we should try to stay away from
>> "clos" in a generic name since that creates problem when trying to
>> apply it to Arm and is very specific to how AMD implements this
>> feature. "cache_inject_capable" does sound much better to me ...
>> it also looks like this may be more appropriate as a property
>> of struct resctrl_cache?
> 
> Coming back to this again, I feel 'cache_inject_capable' is kind of very
> generic. Cache injection term is used very generically everywhere.
> 
> Does  'cache_reserve_capable" sound good ?  This is inside the resctrl
> subsystem. We know what it is referring to.
> 

Since this is inside resctrl "cache_reserve_capable" sounds like existing
CAT to me. Could it help if the term "io" appears in the name? Something like
"io_reserve_capable"? When this is a member of struct resctrl_cache it should
be implicit that it refers to the cache.

Reinette

Re: [PATCH 3/7] x86/resctrl: Introduce sdciae_capable in rdt_resource

[Moger, Babu]

Hi Reinette,

On 10/16/24 10:54, Reinette Chatre wrote:
> Hi Babu,
> 
> On 10/15/24 1:40 PM, Moger, Babu wrote:
>> On 9/19/24 10:33, Reinette Chatre wrote:
>>> On 9/18/24 11:22 AM, Moger, Babu wrote:
>>>> On 9/18/24 10:27, Moger, Babu wrote:
>>>>> On 9/13/24 15:45, Reinette Chatre wrote:
>>>>>> On 8/16/24 9:16 AM, Babu Moger wrote:
>>>>>>> Detect SDCIAE`(L3 Smart Data Cache Injection Allocation Enforcement)
>>>>>>
>>>>>> (stray ` char)
>>>>>
>>>>> Sure.
>>>>>
>>>>>>
>>>>>>> feature and initialize sdciae_capable.
>>>>>>
>>>>>> (This is a repeat of the discussion we had surrounding the ABMC feature.)
>>>>>>
>>>>>> By adding "sdciae_capable" to struct rdt_resource the "sdciae" feature
>>>>>> becomes a resctrl fs feature. Any other architecture that has a "similar
>>>>>> but perhaps not identical feature to AMD's SDCIAE" will be forced to also
>>>>>> call it "sdciae" ... sdciae seems like a marketing name to me and resctrl
>>>>>> needs something generic that could later be built on (if needed) by other
>>>>>> architectures.
>>>>>
>>>>> How about "cache_inject_capable" ?
>>>>>
>>>>> This seems generic. I will change the description also.
>>>>>
>>>>
>>>> Basically, this feature reserves specific CLOS for SDCI cache.
>>>>
>>>> We can also name "clos_reserve_capable".
>>>
>>> Naming is always complicated. I think we should try to stay away from
>>> "clos" in a generic name since that creates problem when trying to
>>> apply it to Arm and is very specific to how AMD implements this
>>> feature. "cache_inject_capable" does sound much better to me ...
>>> it also looks like this may be more appropriate as a property
>>> of struct resctrl_cache?
>>
>> Coming back to this again, I feel 'cache_inject_capable' is kind of very
>> generic. Cache injection term is used very generically everywhere.
>>
>> Does  'cache_reserve_capable" sound good ?  This is inside the resctrl
>> subsystem. We know what it is referring to.
>>
> 
> Since this is inside resctrl "cache_reserve_capable" sounds like existing
> CAT to me. Could it help if the term "io" appears in the name? Something like
> "io_reserve_capable"? When this is a member of struct resctrl_cache it should
> be implicit that it refers to the cache.

Yea. Naming is difficult.

How about "io_alloc_capable"?

-- 
Thanks
Babu Moger

Re: [PATCH 5/7] x86/resctrl: Add interface to enable/disable SDCIAE

[Moger, Babu]

Hi Reinette,

On 10/16/24 10:53, Reinette Chatre wrote:
> Hi Babu,
> 
> On 10/15/24 12:25 PM, Moger, Babu wrote:
>> Hi Reinette,
>>
>> Noticed I didn't respond to this comment.
>>
>> On 9/19/24 10:35, Reinette Chatre wrote:
>>> Hi Babu,
>>>
>>> On 9/18/24 1:10 PM, Moger, Babu wrote:
>>>> On 9/13/24 15:51, Reinette Chatre wrote:
>>>>> On 8/16/24 9:16 AM, Babu Moger wrote:
>>>
>>> ...
>>>
>>>>>> +        if (enable) {
>>>>>> +            ret = closid_alloc_sdciae(r);
>>>>>> +            if (ret < 0) {
>>>>>> +                rdt_last_cmd_puts("SDCIAE CLOSID is not available\n");
>>>>>> +                goto out_sdciae;
>>>>>> +            }
>>>>>> +        } else {
>>>>>> +            sdciae_closid = get_sdciae_closid(r);
>>>>>> +            closid_free(sdciae_closid);
>>>>>> +        }
>>>>>
>>>>>
>>>>>> +
>>>>>> +        ret = resctrl_arch_set_sdciae_enabled(RDT_RESOURCE_L3, enable);
>>>>>
>>>>> I assume that once SDCIAE is enabled the I/O traffic will start flowing to
>>>>> whatever
>>>>> was the last CBM of the max CLOSID? Is this intended or should there be
>>>>> some default
>>>>> CBM that this feature should start with?
>>>>
>>>> It will start with whatever the last CBM for max CLOSID.
>>>
>>> This seems arbitrary based on whatever allocation the previous resource group
>>> using the CLOSID has. When a new resource group is created resctrl ensures
>>> that it is created with all usable allocations, see rdtgroup_init_cat().
>>
>> Checked again with with the team here. When SDCIAE is enabled, it uses the
>> value in L3QosAllocMask15 (value in L3_MASK_15 MSR).  Enabling SDCIAE does
>> not change the value of L3QosAllocMask15.
> 
> I see the issue as similar to how resource group allocations are managed.
> Just like resctrl ensures that when a new resource group is created, it is done
> with maximum allocations that the resource group may use ... not the allocations
> left over from the previous resource group that used those MSRs.
> 
> I understand that the hardware uses L3QosAllocMask15 and does not change
> L3QosAllocMask15 when SDCIAE is enabled, but resctrl is in a position to initialize
> those registers at the time when SDCIAE is initialized to create a sane default
> allocation, not an allocation of whatever happened to be in MSR at that time.

Yes. We can do that. Will add in next revision.

> 
>>> Letting cache injection start with whatever allocation remnant programmed
>>> in a register does not seem ideal. What if, for example, after that resource
>>> group was removed, a new exclusive resource group was created that overlaps
>>> with that allocation? 
>>
>> In that case. it will share the bit mask with the exclusive group. We may
>> need to add a text about it.
> 
> No. This can be avoided entirely when resctrl initializes the MSR to a sane
> default, no?

Sure. We can avoid it.

-- 
Thanks
Babu Moger

Re: [PATCH 3/7] x86/resctrl: Introduce sdciae_capable in rdt_resource

[Reinette Chatre]

Hi Babu,

On 10/16/24 9:46 AM, Moger, Babu wrote:
> On 10/16/24 10:54, Reinette Chatre wrote:
>> On 10/15/24 1:40 PM, Moger, Babu wrote:
>>> On 9/19/24 10:33, Reinette Chatre wrote:
>>>> On 9/18/24 11:22 AM, Moger, Babu wrote:
>>>>> On 9/18/24 10:27, Moger, Babu wrote:
>>>>>> On 9/13/24 15:45, Reinette Chatre wrote:
>>>>>>> On 8/16/24 9:16 AM, Babu Moger wrote:
>>>>>>>> Detect SDCIAE`(L3 Smart Data Cache Injection Allocation Enforcement)
>>>>>>>
>>>>>>> (stray ` char)
>>>>>>
>>>>>> Sure.
>>>>>>
>>>>>>>
>>>>>>>> feature and initialize sdciae_capable.
>>>>>>>
>>>>>>> (This is a repeat of the discussion we had surrounding the ABMC feature.)
>>>>>>>
>>>>>>> By adding "sdciae_capable" to struct rdt_resource the "sdciae" feature
>>>>>>> becomes a resctrl fs feature. Any other architecture that has a "similar
>>>>>>> but perhaps not identical feature to AMD's SDCIAE" will be forced to also
>>>>>>> call it "sdciae" ... sdciae seems like a marketing name to me and resctrl
>>>>>>> needs something generic that could later be built on (if needed) by other
>>>>>>> architectures.
>>>>>>
>>>>>> How about "cache_inject_capable" ?
>>>>>>
>>>>>> This seems generic. I will change the description also.
>>>>>>
>>>>>
>>>>> Basically, this feature reserves specific CLOS for SDCI cache.
>>>>>
>>>>> We can also name "clos_reserve_capable".
>>>>
>>>> Naming is always complicated. I think we should try to stay away from
>>>> "clos" in a generic name since that creates problem when trying to
>>>> apply it to Arm and is very specific to how AMD implements this
>>>> feature. "cache_inject_capable" does sound much better to me ...
>>>> it also looks like this may be more appropriate as a property
>>>> of struct resctrl_cache?
>>>
>>> Coming back to this again, I feel 'cache_inject_capable' is kind of very
>>> generic. Cache injection term is used very generically everywhere.
>>>
>>> Does  'cache_reserve_capable" sound good ?  This is inside the resctrl
>>> subsystem. We know what it is referring to.
>>>
>>
>> Since this is inside resctrl "cache_reserve_capable" sounds like existing
>> CAT to me. Could it help if the term "io" appears in the name? Something like
>> "io_reserve_capable"? When this is a member of struct resctrl_cache it should
>> be implicit that it refers to the cache.
> 
> Yea. Naming is difficult.
> 
> How about "io_alloc_capable"?
> 

Sounds good to me, thank you.

Reinette