x86/resctrl : Support AMD Assignable Bandwidth Monitoring Counters (ABMC)

Hi Babu,

Could you please change shortlog prefix to be "x86/cpufeatures:" to make it obvious where this patch is headed?

Consolidate all the data related to monitoring into separate structure for better readability.

Hi Babu,

Quoted 2 lines · Babu Moger

Consolidate all the data related to monitoring into separate structure for better readability.

This needs a better motivation because "better readability" is subjective. You can motivate this work by noting that this is re-organization in preparation
for more monitoring specific properties that will clobber the existing resource
struct more. To support this organization the pattern of the cache allocation and memory bandwidth allocation features that consolidate the feature specific
properties into a struct is followed.

ABMC feature details are reported via CPUID Fn8000_0020_EBX_x5. Bits Description
15:0 MAX_ABMC Maximum Supported Assignable Bandwidth
Monitoring Counter ID + 1

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.3.3.3 Assignable Bandwidth Monitoring (ABMC).

Link https://bugzilla.kernel.org/show_bug.cgi?id=206537

Signed-off-by Babu Moger <[email protected]>

---
v4: Removed resctrl_arch_has_abmc(). Added all the code inline. We dont
need to separate this as arch code.

v3: Removed changes related to mon_features.
Moved rdt_cpu_has to core.c and added new function resctrl_arch_has_abmc. Also moved the fields mbm_assign_capable and mbm_assign_cntrs to rdt_resource. (James)

v2: Changed the field name to mbm_assign_capable from abmc_capable.

---
  arch/x86/kernel/cpu/resctrl/monitor.c | 14 ++++++++++++++
  include/linux/resctrl.h               |  4 ++++
  2 files changed, 18 insertions(+)

arch/x86/kernel/cpu/resctrl/monitor.c +1 -0

index b35d04fc761b..1602b58ba23d 100644
@@ -1066,6 +1066,20 @@ int __init rdt_get_mon_l3_config(struct rdt_resource *r)
  			mbm_local_event.configurable = true;
  			mbm_config_rftype_init("mbm_local_bytes_config");
  		}
+

arch/x86/kernel/cpu/resctrl/monitor.c +3 -0

+				r->mon.num_cntrs = 64;
+			}
+		}
  	}
 
  	l3_mon_evt_init(r);

include/linux/resctrl.h +0 -0

index bf99eb9c6ce4..24087e6efbb6 100644

include/linux/resctrl.h +2 -0

   * @evt_list:		List of monitoring events
   */
  struct resctrl_mon {
  	int			num_rmid;
+	int			num_cntrs;
  	struct list_head	evt_list;
  };
 

@@ -177,6 +179,7 @@ struct resctrl_mon {
   * @parse_ctrlval:	Per resource function pointer to parse control values
   * @fflags:		flags to choose base and info files
   * @cdp_capable:	Is the CDP feature available on this resource
+ * @abmc_capable:	Is system capable of supporting monitor assignment?
   */
  struct rdt_resource {
  	int			rid;

Hi Babu,

Quoted 43 lines · Babu Moger

ABMC feature details are reported via CPUID Fn8000_0020_EBX_x5. Bits Description
15:0 MAX_ABMC Maximum Supported Assignable Bandwidth Monitoring Counter ID + 1

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.3.3.3 Assignable Bandwidth Monitoring (ABMC).

Link https://bugzilla.kernel.org/show_bug.cgi?id=206537

Signed-off-by Babu Moger <[email protected]>

---
v4: Removed resctrl_arch_has_abmc(). Added all the code inline. We dont need to separate this as arch code.

v3: Removed changes related to mon_features.
     Moved rdt_cpu_has to core.c and added new function resctrl_arch_has_abmc.
     Also moved the fields mbm_assign_capable and mbm_assign_cntrs to rdt_resource. (James)

v2: Changed the field name to mbm_assign_capable from abmc_capable.
---
  arch/x86/kernel/cpu/resctrl/monitor.c | 14 ++++++++++++++ include/linux/resctrl.h               |  4 ++++

  2 files changed, 18 insertions(+)

arch/x86/kernel/cpu/resctrl/monitor.c +5 -0

index b35d04fc761b..1602b58ba23d 100644
@@ -1066,6 +1066,20 @@ int __init rdt_get_mon_l3_config(struct
 rdt_resource *r)
               mbm_local_event.configurable = true;
               mbm_config_rftype_init("mbm_local_bytes_config");
           }
+
+        if (rdt_cpu_has(X86_FEATURE_ABMC)) {
+            r->abmc_capable = true;
+            /*
+             * Query CPUID_Fn80000020_EBX_x05 for number of

arch/x86/kernel/cpu/resctrl/monitor.c int __init rdt_get_mon_l3_config(struct rdt_resource *r) · line 1079

Quoted 6 lines

arch/x86/kernel/cpu/resctrl/monitor.c +6 -0

+             * ABMC counters
+             */
+            cpuid_count(0x80000020, 5, &eax, &ebx, &ecx, &edx);
+            r->mon.num_cntrs = (ebx & 0xFFFF) + 1;
+            if (r->mon.num_cntrs > 64) {
+                WARN(1, "Cannot support more than 64 ABMC counters\n");

if (WARN_ON(...))

Quoted 10 lines

arch/x86/kernel/cpu/resctrl/monitor.c +3 -0

+                r->mon.num_cntrs = 64;
+            }
+        }
       }
         l3_mon_evt_init(r);

include/linux/resctrl.h +0 -0

index bf99eb9c6ce4..24087e6efbb6 100644

include/linux/resctrl.h struct resctrl_schema; · line 156

Quoted 5 lines

include/linux/resctrl.h +1 -0

@@ -153,10 +153,12 @@ struct resctrl_schema;
   /**
    * struct resctrl_mon - Monitoring related data
    * @num_rmid:        Number of RMIDs available
+ * @num_cntrs:        Maximum number of abmc counters

Maximum implies some limit and it is not clear what limit this applies to. Can this just be "Number of monitoring counters".

Thinking ahead ... when switching between different "MBM modes" it may be that one mode has a different number of counters than the other. Since "ABMC" is just one mode it does not seem appropriate to connect the "num_cntrs" property to ABMC.

Quoted 17 lines

include/linux/resctrl.h +1 -0

    * @evt_list:        List of monitoring events
    */
   struct resctrl_mon {
       int            num_rmid;
+    int            num_cntrs;
       struct list_head    evt_list;
   };
   @@ -177,6 +179,7 @@ struct resctrl_mon {

   * @parse_ctrlval:    Per resource function pointer to parse control values
   * @fflags:        flags to choose base and info files
   * @cdp_capable:    Is the CDP feature available on this resource
+ * @abmc_capable:    Is system capable of supporting monitor assignment? */
  struct rdt_resource {

include/linux/resctrl.h struct rdt_resource { · line 202

Quoted 6 lines

      int            rid;

include/linux/resctrl.h +1 -0

@@ -196,6 +199,7 @@ struct rdt_resource {
                            struct rdt_domain *d);
       unsigned long        fflags;
       bool            cdp_capable;
+    bool            abmc_capable;

Shouldn't abmc_capable be a property of the new struct resctrl_mon?

include/linux/resctrl.h struct rdt_resource { · line 204

Quoted 3 lines

include/linux/resctrl.h +0 -0

   };
     /**

Reinette

The functions thread_throttle_mode_init() and mbm_config_rftype_init()

both initialize fflags for resctrl files. Adding the new files will

involve adding another function to initialize the fflags. All of this

can be simplified by adding a new function resctrl_file_fflags_init() and passing the file name and flags to be initialized.

Consolidate multiple fflags initialization into resctrl_file_fflags_init() and remove thread_throttle_mode_init() and mbm_config_rftype_init().

Signed-off-by Babu Moger <[email protected]>

---

Hi Babu,

Shortlog: "Introduce resctrl_file_fflags_init() ..."

Quoted 1 line

The functions thread_throttle_mode_init() and mbm_config_rftype_init()

You can drop "The functions".

Quoted 2 lines

both initialize fflags for resctrl files. Adding the new files will

"Adding the new files" -> "Adding new files"? (this should be in new paragraph)

Quoted 2 lines

involve adding another function to initialize the fflags. All of this

Solution should be in new paragraph and written in imperative mood.

Quoted 9 lines

can be simplified by adding a new function resctrl_file_fflags_init() and passing the file name and flags to be initialized.

Consolidate multiple fflags initialization into resctrl_file_fflags_init() and remove thread_throttle_mode_init() and mbm_config_rftype_init().

Signed-off-by Babu Moger <[email protected]>

---

Patch looks good.

The ABMC feature provides an option to the user to assign a hardware counter to an RMID and monitor the bandwidth as long as the counter is assigned. Number of assignments depend on number of ABMC counters available.

Provide the interface to display the number of ABMC counters supported.

Signed-off-by Babu Moger <[email protected]>

---
v4: Changed the counter name to num_cntrs. And few text changes.

v3: Changed the field name to mbm_assign_cntrs.

v2: Changed the field name to mbm_assignable_counters from abmc_counters.

---
  Documentation/arch/x86/resctrl.rst     |  4 ++++
  arch/x86/kernel/cpu/resctrl/monitor.c  |  1 +
  arch/x86/kernel/cpu/resctrl/rdtgroup.c | 16 ++++++++++++++++
  3 files changed, 21 insertions(+)

Documentation/arch/x86/resctrl.rst +0 -0

index 02790efaabcc..7ab8172ef208 100644

Hi Babu,

Quoted 4 lines · Babu Moger

The ABMC feature provides an option to the user to assign a hardware counter to an RMID and monitor the bandwidth as long as the counter is assigned. Number of assignments depend on number of ABMC counters available.

Take care that this interface will not just be used by ABMC. I assumed that when the user switches to "soft-RMID" then the value of "num_cntrs" will change?

Quoted 26 lines

Provide the interface to display the number of ABMC counters supported.

Signed-off-by Babu Moger <[email protected]>

---
v4: Changed the counter name to num_cntrs. And few text changes.

v3: Changed the field name to mbm_assign_cntrs.

v2: Changed the field name to mbm_assignable_counters from abmc_counters.
---
  Documentation/arch/x86/resctrl.rst     |  4 ++++ arch/x86/kernel/cpu/resctrl/monitor.c  |  1 + arch/x86/kernel/cpu/resctrl/rdtgroup.c | 16 ++++++++++++++++

  3 files changed, 21 insertions(+)

Documentation/arch/x86/resctrl.rst +0 -0

index 02790efaabcc..7ab8172ef208 100644
@@ -257,6 +257,10 @@ with the following files:
           # cat /sys/fs/resctrl/info/L3_MON/mbm_local_bytes_config

          0=0x30;1=0x30;3=0x15;4=0x15
  +"num_cntrs":

Quoted 2 lines

+    Available when ABMC feature is supported. The number of ABMC counters +    available for configuration.

This can only be understood by folks already familiar with AMD's ABMC feature. There is
no information about what "ABMC feature" is, what an "ABMC counter" is and what
"configuration" can be done with it.

Do you think this num_cntrs will only be used by ABMC? What will happen when user
enables "soft-RMID" or some other mode?

Reinette

Add the functionality to enable/disable ABMC feature.

ABMC feature is enabled by setting enabled bit(0) in MSR L3_QOS_EXT_CFG. When the state of ABMC is changed, the MSR needs to be updated on all the logical processors in the QOS Domain.

Hardware counters will reset when ABMC state is changed. Kernel internal counters need to be reset to avoid overflow condition in the next update.

The ABMC 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.3.3.3 Assignable Bandwidth Monitoring (ABMC).

Signed-off-by Babu Moger <[email protected]>

Link https://bugzilla.kernel.org/show_bug.cgi?id=206537

---
v4: Removed resctrl_arch_set_abmc_enabled and resctrl_arch_get_abmc_enabled.
Directly calling resctrl_abmc_enable and resctrl_abmc_disable. Renamed couple of functions.
resctrl_abmc_msrwrite() -> resctrl_abmc_set_one() resctrl_abmc_setup() -> resctrl_abmc_set_all()
Added rdtgroup_mutex lockdep asserts.
Updated commit log and code comments.

v3: No changes.

v2: Few text changes in commit message.

---
  arch/x86/include/asm/msr-index.h       |  1 +
  arch/x86/kernel/cpu/resctrl/internal.h |  8 ++++
  arch/x86/kernel/cpu/resctrl/rdtgroup.c | 62 ++++++++++++++++++++++++++
  3 files changed, 71 insertions(+)

arch/x86/include/asm/msr-index.h +1 -0

index e022e6eb766c..5f9a0139e98c 100644
@@ -1171,6 +1171,7 @@
  #define MSR_IA32_MBA_BW_BASE		0xc0000200
  #define MSR_IA32_SMBA_BW_BASE		0xc0000280
  #define MSR_IA32_EVT_CFG_BASE		0xc0000400
+#define MSR_IA32_L3_QOS_EXT_CFG		0xc00003ff
 
  /* MSR_IA32_VMX_MISC bits */
  #define MSR_IA32_VMX_MISC_INTEL_PT                 (1ULL << 14)

arch/x86/kernel/cpu/resctrl/internal.h +4 -0

index d566251094b2..fabe40304798 100644
@@ -97,6 +97,9 @@ cpumask_any_housekeeping(const struct cpumask *mask, int exclude_cpu)
  	return cpu;
  }
 
+/* Setting bit 0 in L3_QOS_EXT_CFG enables the ABMC feature */
+#define ABMC_ENABLE			BIT(0)
+
  struct rdt_fs_context {
  	struct kernfs_fs_context	kfc;
  	bool				enable_cdpl2;

@@ -436,6 +439,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
+ * @abmc_enabled:	ABMC 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.

arch/x86/kernel/cpu/resctrl/internal.h +1 -0

+
  /*
   * To return the common struct rdt_resource, which is contained in struct
   * rdt_hw_resource, walk the resctrl member of struct rdt_hw_resource.

arch/x86/kernel/cpu/resctrl/rdtgroup.c +6 -0

index ca692712b393..9148d1234ede 100644
@@ -2416,6 +2416,68 @@ int resctrl_arch_set_cdp_enabled(enum resctrl_res_level l, bool enable)
  	return 0;
  }
 
+static void resctrl_abmc_set_one(void *arg)
+{
+	bool *enable = arg;
+	u64 msrval;
+
+	rdmsrl(MSR_IA32_L3_QOS_EXT_CFG, msrval);

arch/x86/kernel/cpu/resctrl/rdtgroup.c +10 -0

+	 */
+	list_for_each_entry(d, &r->domains, list) {
+		on_each_cpu_mask(&d->cpu_mask, resctrl_abmc_set_one, &enable, 1);
+		resctrl_arch_reset_rmid_all(r, d);
+	}
+
+	return 0;
+}
+
+int resctrl_abmc_enable(enum resctrl_res_level l)

arch/x86/kernel/cpu/resctrl/rdtgroup.c +10 -0

+	}
+
+	return ret;
+}
+
+void resctrl_abmc_disable(enum resctrl_res_level l)
+{
+	struct rdt_hw_resource *hw_res = &rdt_resources_all[l];
+
+	lockdep_assert_held(&rdtgroup_mutex);

Hi Babu,

Quoted 8 lines · Babu Moger

Add the functionality to enable/disable ABMC feature.

ABMC feature is enabled by setting enabled bit(0) in MSR L3_QOS_EXT_CFG. When the state of ABMC is changed, the MSR needs to be updated on all the logical processors in the QOS Domain.

Hardware counters will reset when ABMC state is changed. Kernel internal counters need to be reset to avoid overflow condition in the next update.

Please note that there are two "ABMC" features introduced in this series. First, there is the "abmc" resctrl fs feature that just happens to have the same name as AMD's "ABMC" (which may be a good motivation to change this name). Second, there is the architecture (AMD) specific "ABMC" feature that is enabled in response to user's request to enable the resctrl "abmc" feature.

Other architectures need to support resctrl fs "abmc" feature with something entirely different.

Please consider this distinction during this series because it is often blurred.

Quoted 69 lines

The ABMC 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.3.3.3 Assignable Bandwidth Monitoring (ABMC).

Signed-off-by Babu Moger <[email protected]>

Link https://bugzilla.kernel.org/show_bug.cgi?id=206537

---
v4: Removed resctrl_arch_set_abmc_enabled and resctrl_arch_get_abmc_enabled.
     Directly calling resctrl_abmc_enable and resctrl_abmc_disable. Renamed couple of functions. resctrl_abmc_msrwrite() -> resctrl_abmc_set_one() resctrl_abmc_setup() -> resctrl_abmc_set_all() Added rdtgroup_mutex lockdep asserts.
     Updated commit log and code comments.

v3: No changes.

v2: Few text changes in commit message.
---
  arch/x86/include/asm/msr-index.h       |  1 + arch/x86/kernel/cpu/resctrl/internal.h |  8 ++++ arch/x86/kernel/cpu/resctrl/rdtgroup.c | 62 ++++++++++++++++++++++++++

  3 files changed, 71 insertions(+)

arch/x86/include/asm/msr-index.h +1 -0

index e022e6eb766c..5f9a0139e98c 100644
@@ -1171,6 +1171,7 @@
   #define MSR_IA32_MBA_BW_BASE        0xc0000200
   #define MSR_IA32_SMBA_BW_BASE        0xc0000280
   #define MSR_IA32_EVT_CFG_BASE        0xc0000400
+#define MSR_IA32_L3_QOS_EXT_CFG        0xc00003ff
     /* MSR_IA32_VMX_MISC bits */
   #define MSR_IA32_VMX_MISC_INTEL_PT                 (1ULL << 14)

arch/x86/kernel/cpu/resctrl/internal.h +0 -0

index d566251094b2..fabe40304798 100644
@@ -97,6 +97,9 @@ cpumask_any_housekeeping(const struct cpumask *mask,
 int exclude_cpu)
       return cpu;
   }

  +/* Setting bit 0 in L3_QOS_EXT_CFG enables the ABMC feature */

arch/x86/kernel/cpu/resctrl/internal.h +2 -0

+#define ABMC_ENABLE            BIT(0)
+

  struct rdt_fs_context {
      struct kernfs_fs_context    kfc;
      bool                enable_cdpl2;

arch/x86/kernel/cpu/resctrl/internal.h +2 -0

@@ -436,6 +439,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
+ * @abmc_enabled:    ABMC 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.

@@ -450,6 +454,7 @@ struct rdt_hw_resource {
       unsigned int        mbm_width;
       unsigned int        mbm_cfg_mask;
       bool            cdp_enabled;
+    bool            abmc_enabled;
   };

Quoted 1 line

ok, so here by making "abmc_enabled" a member of struct rdt_hw_resource this is
an architecture specific property. This is reasonable since every architecture
will look different. What is _not_ ok is that this causes the rest of the

series to change resctrl fs to reach into the architecture code. For example, this work causes mbm_config_show() to now need to peek into struct rdt_hw_resource
to see this value. That is not ok. All of the interactions between this field and resctrl fs needs to be via arch helpers:
resctrl_arch_abmc_enable()/
resctrl_arch_abmc_disable() and resctrl_arch_get_abmc_enabled()/resctrl_arch_set_abmc_enabled().

Quoted 9 lines

  static inline struct rdt_hw_resource *resctrl_to_arch_res(struct rdt_resource *r)

arch/x86/kernel/cpu/resctrl/internal.h +0 -0

@@ -493,6 +498,9 @@ static inline bool resctrl_arch_get_cdp_enabled(enum
 resctrl_res_level l)

    int resctrl_arch_set_cdp_enabled(enum resctrl_res_level l, bool enable);
  +int resctrl_abmc_enable(enum resctrl_res_level l); +void resctrl_abmc_disable(enum resctrl_res_level l);

Why do these need enum resctrl_res_level parameter?

Quoted 26 lines

+
  /*
   * To return the common struct rdt_resource, which is contained in struct
   * rdt_hw_resource, walk the resctrl member of struct rdt_hw_resource.

arch/x86/kernel/cpu/resctrl/rdtgroup.c +11 -0

index ca692712b393..9148d1234ede 100644
@@ -2416,6 +2416,68 @@ int resctrl_arch_set_cdp_enabled(enum
 resctrl_res_level l, bool enable)
       return 0;
   }
   +static void resctrl_abmc_set_one(void *arg)
+{
+    bool *enable = arg;
+    u64 msrval;
+
+    rdmsrl(MSR_IA32_L3_QOS_EXT_CFG, msrval);
+
+    if (*enable)
+        msrval |= ABMC_ENABLE;
+    else
+        msrval &= ~ABMC_ENABLE;
+

arch/x86/kernel/cpu/resctrl/rdtgroup.c int resctrl_arch_set_cdp_enabled(enum resctrl_res_level l, bool enable) · line 2434

Quoted 4 lines

arch/x86/kernel/cpu/resctrl/rdtgroup.c +4 -0

+    wrmsrl(MSR_IA32_L3_QOS_EXT_CFG, msrval);
+}
+
+static int resctrl_abmc_set_all(enum resctrl_res_level l, bool enable)

Should this function and resctrl_abmc_set_one() perhaps have "amd" in the name just to enforce that this is not filesystem code at all and specific and unique to AMD.

arch/x86/kernel/cpu/resctrl/rdtgroup.c int resctrl_arch_set_cdp_enabled(enum resctrl_res_level l, bool enable) · line 2440

Quoted 7 lines

arch/x86/kernel/cpu/resctrl/rdtgroup.c +6 -0

+{
+    struct rdt_resource *r = &rdt_resources_all[l].r_resctrl;
+    struct rdt_domain *d;
+
+    /*
+     * Update QOS_CFG MSR on all the CPUs associated with the resource

end of sentence needs "."

This comment about the specific register seems more appropriate to resctrl_abmc_set_one() though. This function is a higher level enable/disable of the hardware feature.

arch/x86/kernel/cpu/resctrl/rdtgroup.c int resctrl_arch_set_cdp_enabled(enum resctrl_res_level l, bool enable) · line 2441

Quoted 2 lines

arch/x86/kernel/cpu/resctrl/rdtgroup.c +1 -0

+     * Hardware counters will reset after switching the monotor mode.

monotor -> monitor

arch/x86/kernel/cpu/resctrl/rdtgroup.c int resctrl_arch_set_cdp_enabled(enum resctrl_res_level l, bool enable) · line 2443

Quoted 3 lines

arch/x86/kernel/cpu/resctrl/rdtgroup.c +2 -0

+     * Reset the internal counters so that it is not considered as
+     * an overflow in next update.

For the first time the term "internal counters" is introduced. What does it mean?

Quoted 19 lines

arch/x86/kernel/cpu/resctrl/rdtgroup.c +18 -0

+     */
+    list_for_each_entry(d, &r->domains, list) {
+        on_each_cpu_mask(&d->cpu_mask, resctrl_abmc_set_one, &enable, 1);
+        resctrl_arch_reset_rmid_all(r, d);
+    }
+
+    return 0;
+}
+
+int resctrl_abmc_enable(enum resctrl_res_level l)
+{
+    struct rdt_hw_resource *hw_res = &rdt_resources_all[l];
+    int ret = 0;
+
+    lockdep_assert_held(&rdtgroup_mutex);
+
+    if (!hw_res->abmc_enabled) {
+        ret = resctrl_abmc_set_all(l, true);

arch/x86/kernel/cpu/resctrl/rdtgroup.c int resctrl_arch_set_cdp_enabled(enum resctrl_res_level l, bool enable) · line 2463

Quoted 2 lines

arch/x86/kernel/cpu/resctrl/rdtgroup.c +2 -0

+        if (!ret)
+            hw_res->abmc_enabled = true;

This error handling seems useless since resctrl_abmc_set_all() always returns 0 ... perhaps it should return void instead and this error handling dropped? With that this function can never fail either and it can just return void, but this is probably not what we want as the architecture call since other architectures may fail.

Quoted 20 lines

arch/x86/kernel/cpu/resctrl/rdtgroup.c +17 -0

+    }
+
+    return ret;
+}
+
+void resctrl_abmc_disable(enum resctrl_res_level l)
+{
+    struct rdt_hw_resource *hw_res = &rdt_resources_all[l];
+
+    lockdep_assert_held(&rdtgroup_mutex);
+
+    if (hw_res->abmc_enabled) {
+        resctrl_abmc_set_all(l, false);
+        hw_res->abmc_enabled = false;
+    }
+}
+
   /*
    * We don't allow rdtgroup directories to be created anywhere

Quoted 1 line

   * except the root directory. Thus when looking for the rdtgroup

Reinette

Quoted 7 lines · Reinette Chatre

Quoted 2 lines · Babu Moger

+     * Reset the internal counters so that it is not considered as +     * an overflow in next update.

For the first time the term "internal counters" is introduced. What does it mean?

How about "mbm counters"?

Hi Babu,

Quoted 9 lines · Babu

Quoted 7 lines · Reinette Chatre

Quoted 2 lines · Babu Moger

+     * Reset the internal counters so that it is not considered as +     * an overflow in next update.

For the first time the term "internal counters" is introduced. What does it mean?

Quoted 1 line

How about "mbm counters"?

I think it refers to the state resctrl maintains to be able to catch overflows, so how about just "architectural state" with matching change of something like:

    Reset the architectural state so that reading of hardware counter is not considered as an overflow in next update.

The ABMC feature provides an option to the user to assign a hardware counter to an RMID and monitor the bandwidth as long as it is assigned. ABMC mode is enabled by default when supported. System can be one mode at a time (Legacy monitor mode or ABMC mode).

Provide an interface to display the monitor mode on the system.

     $cat /sys/fs/resctrl/info/L3_MON/mbm_assign
     [abmc]
     legacy

Signed-off-by Babu Moger <[email protected]>

---
v4: Fixed the checks for legacy and abmc mode. Default it ABMC.

v3: New patch to display ABMC capability.

---
  Documentation/arch/x86/resctrl.rst     | 10 ++++++++++
  arch/x86/kernel/cpu/resctrl/monitor.c  |  1 +
  arch/x86/kernel/cpu/resctrl/rdtgroup.c | 23 +++++++++++++++++++++++
  3 files changed, 34 insertions(+)

Documentation/arch/x86/resctrl.rst +0 -0

index 7ab8172ef208..ab3cde61a124 100644

Documentation/arch/x86/resctrl.rst +8 -0

+	monitoring features supported. The enclosed brackets indicate which
+	feature is enabled.
+	::
+
+	  cat /sys/fs/resctrl/info/L3_MON/mbm_assign
+	  [abmc]
+	  mbm_legacy
+
  "max_threshold_occupancy":
  		Read/write file provides the largest value (in
  		bytes) at which a previously used LLC_occupancy

arch/x86/kernel/cpu/resctrl/monitor.c +1 -0

index e75a6146068b..b1d002e5e93d 100644
@@ -1071,6 +1071,7 @@ int __init rdt_get_mon_l3_config(struct rdt_resource *r)
 
  		if (rdt_cpu_has(X86_FEATURE_ABMC)) {
  			r->abmc_capable = true;
+			resctrl_file_fflags_init("mbm_assign", RFTYPE_MON_INFO);
  			/*
  			 * Query CPUID_Fn80000020_EBX_x05 for number of
  			 * ABMC counters

arch/x86/kernel/cpu/resctrl/rdtgroup.c +0 -0

index 9148d1234ede..3071bbb7a15e 100644

arch/x86/kernel/cpu/resctrl/rdtgroup.c +12 -0

+
+	if (hw_res->abmc_enabled) {
+		seq_puts(s, "[abmc]\n");
+		seq_puts(s, "mbm_legacy\n");
+	} else {
+		seq_puts(s, "abmc\n");
+		seq_puts(s, "[mbm_legacy]\n");
+	}
+
+	return 0;
+}
+
  #ifdef CONFIG_PROC_CPU_RESCTRL
 
  /*

@@ -1913,6 +1930,12 @@ static struct rftype res_common_files[] = {
  		.seq_show	= mbm_local_bytes_config_show,
  		.write		= mbm_local_bytes_config_write,

Hi Babu,

Quoted 5 lines · Babu Moger

The ABMC feature provides an option to the user to assign a hardware counter to an RMID and monitor the bandwidth as long as it is assigned. ABMC mode is enabled by default when supported. System can be one mode at a time (Legacy monitor mode or ABMC mode).

Quoted 4 lines

Provide an interface to display the monitor mode on the system. $cat /sys/fs/resctrl/info/L3_MON/mbm_assign [abmc]
     legacy

Output is different from cover letter and what this patch implements.

Quoted 13 lines

Signed-off-by Babu Moger <[email protected]>

---
v4: Fixed the checks for legacy and abmc mode. Default it ABMC.

v3: New patch to display ABMC capability.
---
  Documentation/arch/x86/resctrl.rst     | 10 ++++++++++ arch/x86/kernel/cpu/resctrl/monitor.c  |  1 + arch/x86/kernel/cpu/resctrl/rdtgroup.c | 23 +++++++++++++++++++++++

  3 files changed, 34 insertions(+)

Quoted 9 lines

Documentation/arch/x86/resctrl.rst +0 -0

index 7ab8172ef208..ab3cde61a124 100644
@@ -261,6 +261,16 @@ with the following files:

      Available when ABMC feature is supported. The number of ABMC counters available for configuration.
  +"mbm_assign":

This name is not ideal but I am having trouble finding a better one ... I have
seen you use "monitor mode" a couple of times (even in shortlog), so maybe that
could be the start of a more generic name? "mbm_mode"?

Quoted 3 lines

+    Available when ABMC feature is supported. Reports the list of assignable

Why not always make this file available? At least it will display that legacy mode is supported and it gives user space an always present file to query to
determine support.

Quoted 34 lines

+    monitoring features supported. The enclosed brackets indicate which +    feature is enabled.
+    ::

Documentation/arch/x86/resctrl.rst +5 -0

+
+      cat /sys/fs/resctrl/info/L3_MON/mbm_assign
+      [abmc]
+      mbm_legacy
+

  "max_threshold_occupancy":
          Read/write file provides the largest value (in bytes) at which a previously used LLC_occupancy

arch/x86/kernel/cpu/resctrl/monitor.c +1 -0

index e75a6146068b..b1d002e5e93d 100644
@@ -1071,6 +1071,7 @@ int __init rdt_get_mon_l3_config(struct
 rdt_resource *r)
             if (rdt_cpu_has(X86_FEATURE_ABMC)) {
               r->abmc_capable = true;
+            resctrl_file_fflags_init("mbm_assign", RFTYPE_MON_INFO);
               /*
                * Query CPUID_Fn80000020_EBX_x05 for number of
                * ABMC counters

arch/x86/kernel/cpu/resctrl/rdtgroup.c +0 -0

index 9148d1234ede..3071bbb7a15e 100644
@@ -856,6 +856,23 @@ static int rdtgroup_num_cntrs_show(struct
 kernfs_open_file *of,
       return 0;
   }

arch/x86/kernel/cpu/resctrl/rdtgroup.c static int rdtgroup_num_cntrs_show(struct kernfs_open_file *of, · line 863

Quoted 5 lines

arch/x86/kernel/cpu/resctrl/rdtgroup.c +4 -0

   +static int rdtgroup_mbm_assign_show(struct kernfs_open_file *of,
+                    struct seq_file *s, void *v)
+{
+    struct rdt_resource *r = of->kn->parent->priv;
+    struct rdt_hw_resource *hw_res = resctrl_to_arch_res(r);

Please use arch helper to just get abmc state instead of fs code digging into arch structures.

Quoted 18 lines

arch/x86/kernel/cpu/resctrl/rdtgroup.c +12 -0

+
+    if (hw_res->abmc_enabled) {
+        seq_puts(s, "[abmc]\n");
+        seq_puts(s, "mbm_legacy\n");
+    } else {
+        seq_puts(s, "abmc\n");
+        seq_puts(s, "[mbm_legacy]\n");
+    }
+
+    return 0;
+}
+
   #ifdef CONFIG_PROC_CPU_RESCTRL
     /*

@@ -1913,6 +1930,12 @@ static struct rftype res_common_files[] = {
           .seq_show    = mbm_local_bytes_config_show,
           .write        = mbm_local_bytes_config_write,

arch/x86/kernel/cpu/resctrl/rdtgroup.c static struct rftype res_common_files[] = { · line 1941

Quoted 10 lines

arch/x86/kernel/cpu/resctrl/rdtgroup.c +6 -0

       },
+    {
+        .name        = "mbm_assign",
+        .mode        = 0444,
+        .kf_ops        = &rdtgroup_kf_single_ops,
+        .seq_show    = rdtgroup_mbm_assign_show,
+    },
       {
           .name        = "cpus",
           .mode        = 0644,

Reinette

Quoted 14 lines · Reinette Chatre

Documentation/arch/x86/resctrl.rst with the following files: · line 263

Quoted 6 lines · Babu Moger

Documentation/arch/x86/resctrl.rst +0 -0

@@ -261,6 +261,16 @@ with the following files:
       Available when ABMC feature is supported. The number of ABMC counters
       available for configuration.
   +"mbm_assign":

This name is not ideal but I am having trouble finding a better one ... I have
seen you use "monitor mode" a couple of times (even in shortlog), so maybe that
could be the start of a more generic name? "mbm_mode"?

mbm_mode sounds good. Like this.

$cat /sys/fs/resctrl/info/L3_MON/mbm_mode
[abmc]
legacy

Keeping just "legacy" vs mbm_legacy.

Hi Babu,

Quoted 18 lines · Babu

Quoted 14 lines · Reinette Chatre

Quoted 6 lines · Babu Moger

Documentation/arch/x86/resctrl.rst +0 -0

@@ -261,6 +261,16 @@ with the following files:

       Available when ABMC feature is supported. The number of ABMC counters available for configuration.
   +"mbm_assign":

This name is not ideal but I am having trouble finding a better one ... I have
seen you use "monitor mode" a couple of times (even in shortlog), so maybe that
could be the start of a more generic name? "mbm_mode"?

mbm_mode sounds good. Like this.

Quoted 6 lines

$cat /sys/fs/resctrl/info/L3_MON/mbm_mode
[abmc]
legacy

Keeping just "legacy" vs mbm_legacy.

Looks good to me. This sounds generic enough to build on.

Hardware provides a set of counters when the ABMC feature is supported. These counters are used for enabling the events in resctrl group when the feature is supported.

Introduce num_cntrs_free_map bitmap to track available and free counters.

Signed-off-by Babu Moger <[email protected]>

---
v4: Changed the name to num_cntrs where applicable.
Used bitmap apis.
Added more comments for the globals.

v3: Changed the bitmap name to assign_cntrs_free_map. Removed abmc
from the name.

v2: Changed the bitmap name to assignable_counter_free_map from
abmc_counter_free_map.

---
  arch/x86/kernel/cpu/resctrl/rdtgroup.c | 23 +++++++++++++++++++++++
  1 file changed, 23 insertions(+)

arch/x86/kernel/cpu/resctrl/rdtgroup.c +0 -0

index 3071bbb7a15e..400ae405e10e 100644

Hi Babu,

Quoted 31 lines · Babu Moger

Hardware provides a set of counters when the ABMC feature is supported. These counters are used for enabling the events in resctrl group when the feature is supported.

Introduce num_cntrs_free_map bitmap to track available and free counters.

Signed-off-by Babu Moger <[email protected]>

---
v4: Changed the name to num_cntrs where applicable. Used bitmap apis.
     Added more comments for the globals.

v3: Changed the bitmap name to assign_cntrs_free_map. Removed abmc from the name.

v2: Changed the bitmap name to assignable_counter_free_map from abmc_counter_free_map.
---
  arch/x86/kernel/cpu/resctrl/rdtgroup.c | 23 +++++++++++++++++++++++

  1 file changed, 23 insertions(+)

arch/x86/kernel/cpu/resctrl/rdtgroup.c +0 -0

index 3071bbb7a15e..400ae405e10e 100644
@@ -186,6 +186,23 @@ bool closid_allocated(unsigned int closid)
       return !test_bit(closid, &closid_free_map);
   }

  +/*
+ * ABMC Counter bitmap and length for tracking available counters.

Quoted 5 lines

+ * ABMC feature provides set of hardware counters for enabling events.
+ * Each event takes one hardware counter. Kernel needs to keep track
+ * of number of available counters.
+ */
+static unsigned long num_cntrs_free_map;

Why does variable have "num" in its name? That seems strange. How about just "mon_cntrs_free_map

Quoted 2 lines

+static u32 num_cntrs_free_map_len;

Same comment about "num" ... also, any special reason why u32 is needed?

arch/x86/kernel/cpu/resctrl/rdtgroup.c bool closid_allocated(unsigned int closid) · line 198

Quoted 3 lines

arch/x86/kernel/cpu/resctrl/rdtgroup.c +2 -0

+
+static void num_cntrs_init(void)

mon_cntrs_init() ?

Quoted 10 lines

+{
+    struct rdt_resource *r =
&rdt_resources_all[RDT_RESOURCE_L3].r_resctrl;

arch/x86/kernel/cpu/resctrl/rdtgroup.c +5 -0

+
+    bitmap_fill(&num_cntrs_free_map, r->mon.num_cntrs);
+    num_cntrs_free_map_len = r->mon.num_cntrs;
+}
+

  /**

arch/x86/kernel/cpu/resctrl/rdtgroup.c static int resctrl_abmc_set_all(enum · line 2476

Quoted 4 lines

   * rdtgroup_mode_by_closid - Return mode of resource group with closid
   * @closid: closid if the resource group

arch/x86/kernel/cpu/resctrl/rdtgroup.c +0 -0

@@ -2459,6 +2476,12 @@ static int resctrl_abmc_set_all(enum
 resctrl_res_level l, bool enable)

resctrl_abmc_set_all() was initially created as a complement of resctrl_abmc_set_one() ... but with more initialization added to resctrl_abmc_set_all() this relationship becomes vague.

Quoted 6 lines

arch/x86/kernel/cpu/resctrl/rdtgroup.c +0 -0

       struct rdt_resource *r = &rdt_resources_all[l].r_resctrl;
       struct rdt_domain *d;

  +    /*
+     * The bitmap num_cntrs_free_map needs to be reset when switching +     * the monitor mode.

This comment puts in words what can be understood by just looking at the code.
What would be useful would be to explain _why_ this needs to be done ... what happens to the previously assigned counters?

Quoted 8 lines

+     */
+    num_cntrs_init();
+
      /*
       * Update QOS_CFG MSR on all the CPUs associated with the resource
       * Hardware counters will reset after switching the monotor mode.

Reinette

Quoted 1 line · Reinette Chatre

On 5/24/24 5:23 AM, Babu Moger wrote:

Quoted 10 lines

Quoted 10 lines

arch/x86/kernel/cpu/resctrl/rdtgroup.c +1 -0

index 3071bbb7a15e..400ae405e10e 100644
@@ -186,6 +186,23 @@ bool closid_allocated(unsigned int closid)
       return !test_bit(closid, &closid_free_map);
   }
   +/*
+ * ABMC Counter bitmap and length for tracking available counters.

Quoted 8 lines

arch/x86/kernel/cpu/resctrl/rdtgroup.c bool closid_allocated(unsigned int closid) · line 195

Quoted 5 lines

arch/x86/kernel/cpu/resctrl/rdtgroup.c +5 -0

+ * ABMC feature provides set of hardware counters for enabling events.
+ * Each event takes one hardware counter. Kernel needs to keep track
+ * of number of available counters.
+ */
+static unsigned long num_cntrs_free_map;

Why does variable have "num" in its name? That seems strange. How about just "mon_cntrs_free_map

It came from patch 4/19.

struct resctrl_mon {
         int                     num_rmid;
+       int                     num_cntrs;
         struct list_head        evt_list;

};

num_cntrs_free_map is a bitmap representing num_cntrs. Kept the matching name for better understanding. Renaming it will loose that connection.

If I rename then I will have to rename both.

How about mbm_cntrs and mbm_cntrs_free_map?

Quoted 3 lines

arch/x86/kernel/cpu/resctrl/rdtgroup.c bool closid_allocated(unsigned int closid) · line 196

Quoted 1 line

arch/x86/kernel/cpu/resctrl/rdtgroup.c +1 -0

+static u32 num_cntrs_free_map_len;

Same comment about "num" ... also, any special reason why u32 is needed?

Only reason is, it is supposed to be unsigned. I can change it "unsigned int".

Quoted 1 line

Quoted 4 lines

arch/x86/kernel/cpu/resctrl/rdtgroup.c bool closid_allocated(unsigned int closid) · line 198

Quoted 2 lines

arch/x86/kernel/cpu/resctrl/rdtgroup.c +2 -0

+
+static void num_cntrs_init(void)

mon_cntrs_init() ?

mbm_cntrs_init?

Quoted 11 lines

Quoted 11 lines

arch/x86/kernel/cpu/resctrl/rdtgroup.c +7 -0

+{
+    struct rdt_resource *r =
 &rdt_resources_all[RDT_RESOURCE_L3].r_resctrl;
+
+    bitmap_fill(&num_cntrs_free_map, r->mon.num_cntrs);
+    num_cntrs_free_map_len = r->mon.num_cntrs;
+}
+
   /**

Quoted 8 lines

arch/x86/kernel/cpu/resctrl/rdtgroup.c static int resctrl_abmc_set_all(enum · line 2476

Quoted 4 lines

arch/x86/kernel/cpu/resctrl/rdtgroup.c +0 -0

    * rdtgroup_mode_by_closid - Return mode of resource group with closid
    * @closid: closid if the resource group

@@ -2459,6 +2476,12 @@ static int resctrl_abmc_set_all(enum
 resctrl_res_level l, bool enable)

resctrl_abmc_set_all() was initially created as a complement of resctrl_abmc_set_one() ... but with more initialization added to resctrl_abmc_set_all() this relationship becomes vague.

Yes. Understood. Let me know if want me to change anything here.

Hi Babu,

Quoted 2 lines · Babu

Quoted 1 line · Reinette Chatre

On 5/24/24 5:23 AM, Babu Moger wrote:

...

Quoted 21 lines

Quoted 10 lines

Quoted 10 lines

arch/x86/kernel/cpu/resctrl/rdtgroup.c +0 -0

index 3071bbb7a15e..400ae405e10e 100644
@@ -186,6 +186,23 @@ bool closid_allocated(unsigned int closid)
        return !test_bit(closid, &closid_free_map);
    }

   +/*
+ * ABMC Counter bitmap and length for tracking available counters.

Quoted 8 lines

Quoted 5 lines

+ * ABMC feature provides set of hardware counters for enabling events.
+ * Each event takes one hardware counter. Kernel needs to keep track
+ * of number of available counters.
+ */
+static unsigned long num_cntrs_free_map;

Why does variable have "num" in its name? That seems strange. How about just "mon_cntrs_free_map

It came from patch 4/19.

Quoted 8 lines

struct resctrl_mon {
         int                     num_rmid;
+       int                     num_cntrs;
         struct list_head        evt_list;
  };

num_cntrs_free_map is a bitmap representing num_cntrs. Kept the matching name for better understanding. Renaming it will loose that connection.

I disagree. The "num" in "num_cntrs" indicates that this variable stores the _number_ of a particular entity. In this case "cntrs" or ... counters. This is just like how resctrl uses "num_closid" to indicate how many closid are available and then have a separate "closid_free_map" to actually track now closids are used ... it is _not_ "num_closid_free_map". Similarly, "num_rmid" indicates how many RMID are available and the "rmid_free_lru" tracks how RMID are used ... it is _not_ "num_rmid_free_lru".

Quoted 1 line

If I rename then I will have to rename both.

No, you do not.

Quoted 2 lines

How about mbm_cntrs and mbm_cntrs_free_map?

"mbm_cntrs" does sound good. It is more specific than "cntrs". I would suggest that use "num_mbm_cntrs" to match with "num_rmid" and "num_closid" and then you can introduce "mbm_cntrs_free_map".

Quoted 9 lines

Quoted 4 lines

Quoted 2 lines

+static u32 num_cntrs_free_map_len;

Same comment about "num" ... also, any special reason why u32 is needed?

Only reason is, it is supposed to be unsigned.  I can change it "unsigned int".

Quoted 1 line

Quoted 6 lines

Quoted 4 lines

arch/x86/kernel/cpu/resctrl/rdtgroup.c bool closid_allocated(unsigned int closid) · line 198

Quoted 2 lines

arch/x86/kernel/cpu/resctrl/rdtgroup.c +2 -0

+
+static void num_cntrs_init(void)

mon_cntrs_init() ?

mbm_cntrs_init?

Sounds good.

Quoted 21 lines

Quoted 20 lines

Quoted 15 lines

+{
+    struct rdt_resource *r =
&rdt_resources_all[RDT_RESOURCE_L3].r_resctrl;

arch/x86/kernel/cpu/resctrl/rdtgroup.c +5 -0

+
+    bitmap_fill(&num_cntrs_free_map, r->mon.num_cntrs);
+    num_cntrs_free_map_len = r->mon.num_cntrs;
+}
+

   /**
    * rdtgroup_mode_by_closid - Return mode of resource group with closid
    * @closid: closid if the resource group

arch/x86/kernel/cpu/resctrl/rdtgroup.c static int resctrl_abmc_set_all(enum · line 2476

Quoted 1 line

arch/x86/kernel/cpu/resctrl/rdtgroup.c +0 -0

@@ -2459,6 +2476,12 @@ static int resctrl_abmc_set_all(enum
 resctrl_res_level l, bool enable)

resctrl_abmc_set_all() was initially created as a complement of resctrl_abmc_set_one() ... but with more initialization added to resctrl_abmc_set_all() this relationship becomes vague.

Quoted 1 line

Yes. Understood. Let me know if want me to change anything here.

How about renaming resctrl_abmc_set_all() to _resctrl_abmc_enable()?

If the BMEC (Bandwidth Monitoring Event Configuration) feature is supported, the bandwidth events can be configured to track specific events. The event configuration is domain specific. ABMC (Assignable Bandwidth Monitoring Counters) feature needs event configuration information to assign hardware counter to an RMID. Event configurations are not stored in resctrl but instead always read from or written to hardware directly when prompted by user space.

Read the event configuration from the hardware during the domain initialization. Save the configuration information in the rdt_hw_domain, so it can be used for counter assignment.

Signed-off-by Babu Moger <[email protected]>

---
v4: Read the configuration information from the hardware to initialize.
Added few commit messages.
Fixed the tab spaces.

v3: Minor changes related to rebase in mbm_config_write_domain.

v2: No changes.

---
  arch/x86/kernel/cpu/resctrl/core.c     |  2 ++
  arch/x86/kernel/cpu/resctrl/internal.h |  5 +++++
  arch/x86/kernel/cpu/resctrl/monitor.c  | 21 +++++++++++++++++++++
  3 files changed, 28 insertions(+)

arch/x86/kernel/cpu/resctrl/core.c +2 -0

index ec93f6a50308..856c46d12177 100644
@@ -542,6 +542,8 @@ static void domain_add_cpu(int cpu, struct rdt_resource *r)
  		return;
  	}
 
+	arch_domain_mbm_evt_config(hw_dom);
+
  	list_add_tail_rcu(&d->list, add_pos);
 
  	err = resctrl_online_domain(r, d);

arch/x86/kernel/cpu/resctrl/internal.h +2 -0

index 5e7e76cd512f..60a1ca0a11a7 100644
@@ -373,6 +373,8 @@ struct arch_mbm_state {
   * @ctrl_val:	array of cache or mem ctrl values (indexed by CLOSID)
   * @arch_mbm_total:	arch private state for MBM total bandwidth
   * @arch_mbm_local:	arch private state for MBM local bandwidth
+ * @mbm_total_cfg:	MBM total bandwidth configuration
+ * @mbm_local_cfg:	MBM local bandwidth configuration
   *
   * Members of this structure are accessed via helpers that provide abstraction.
   */

arch/x86/kernel/cpu/resctrl/internal.h +1 -0

  static inline struct rdt_hw_domain *resctrl_to_arch_dom(struct rdt_domain *r)

@@ -622,6 +626,7 @@ void __check_limbo(struct rdt_domain *d, bool force_free);
  void rdt_domain_reconfigure_cdp(struct rdt_resource *r);
  void __init resctrl_file_fflags_init(const char *config,
  				     unsigned long fflags);
+void arch_domain_mbm_evt_config(struct rdt_hw_domain *hw_dom);
  void rdt_staged_configs_clear(void);
  bool closid_allocated(unsigned int closid);
  int resctrl_find_cleanest_closid(void);

arch/x86/kernel/cpu/resctrl/monitor.c +0 -0

index b1d002e5e93d..ab0f4bb49bd9 100644
@@ -1093,6 +1093,27 @@ int __init rdt_get_mon_l3_config(struct rdt_resource *r)
  	return 0;
  }

arch/x86/kernel/cpu/resctrl/monitor.c +5 -0

+	 */
+	if (mbm_total_event.configurable) {
+		rdmsrl(MSR_IA32_EVT_CFG_BASE, msrval);
+		hw_dom->mbm_total_cfg = msrval & MAX_EVT_CONFIG_BITS;
+	}

Hi Babu,

Quoted 58 lines · Babu Moger

If the BMEC (Bandwidth Monitoring Event Configuration) feature is supported, the bandwidth events can be configured to track specific events. The event configuration is domain specific. ABMC (Assignable Bandwidth Monitoring Counters) feature needs event configuration information to assign hardware counter to an RMID. Event configurations are not stored in resctrl but instead always read from or written to hardware directly when prompted by user space.

Read the event configuration from the hardware during the domain initialization. Save the configuration information in the rdt_hw_domain, so it can be used for counter assignment.

Signed-off-by Babu Moger <[email protected]>

---
v4: Read the configuration information from the hardware to initialize. Added few commit messages.
     Fixed the tab spaces.

v3: Minor changes related to rebase in mbm_config_write_domain.

v2: No changes.
---
  arch/x86/kernel/cpu/resctrl/core.c     |  2 ++ arch/x86/kernel/cpu/resctrl/internal.h |  5 +++++ arch/x86/kernel/cpu/resctrl/monitor.c  | 21 +++++++++++++++++++++

  3 files changed, 28 insertions(+)

arch/x86/kernel/cpu/resctrl/core.c +1 -0

index ec93f6a50308..856c46d12177 100644
@@ -542,6 +542,8 @@ static void domain_add_cpu(int cpu, struct
 rdt_resource *r)
           return;
       }
   +    arch_domain_mbm_evt_config(hw_dom);
+
       list_add_tail_rcu(&d->list, add_pos);
         err = resctrl_online_domain(r, d);

arch/x86/kernel/cpu/resctrl/internal.h +2 -0

index 5e7e76cd512f..60a1ca0a11a7 100644
@@ -373,6 +373,8 @@ struct arch_mbm_state {
    * @ctrl_val:    array of cache or mem ctrl values (indexed by CLOSID)
    * @arch_mbm_total:    arch private state for MBM total bandwidth
    * @arch_mbm_local:    arch private state for MBM local bandwidth
+ * @mbm_total_cfg:    MBM total bandwidth configuration
+ * @mbm_local_cfg:    MBM local bandwidth configuration
    *
    * Members of this structure are accessed via helpers that provide

abstraction.
   */

arch/x86/kernel/cpu/resctrl/internal.h +0 -0

@@ -381,6 +383,8 @@ struct rdt_hw_domain {
       u32                *ctrl_val;
       struct arch_mbm_state        *arch_mbm_total;

arch/x86/kernel/cpu/resctrl/internal.h struct rdt_hw_domain { · line 388

Quoted 4 lines

arch/x86/kernel/cpu/resctrl/internal.h +2 -0

       struct arch_mbm_state        *arch_mbm_local;
+    u32                mbm_total_cfg;
+    u32                mbm_local_cfg;
   };

Similar to the abmc_enabled member of rdt_hw_resource, these new members of rdt_hw_domain are architecture specific and should never be touched directly by resctrl fs code, for example, from mbm_config_show().

Quoted 24 lines

    static inline struct rdt_hw_domain *resctrl_to_arch_dom(struct rdt_domain *r)

arch/x86/kernel/cpu/resctrl/internal.h +1 -0

@@ -622,6 +626,7 @@ void __check_limbo(struct rdt_domain *d, bool
 force_free);
   void rdt_domain_reconfigure_cdp(struct rdt_resource *r);
   void __init resctrl_file_fflags_init(const char *config,
                        unsigned long fflags);
+void arch_domain_mbm_evt_config(struct rdt_hw_domain *hw_dom);
   void rdt_staged_configs_clear(void);
   bool closid_allocated(unsigned int closid);

  int resctrl_find_cleanest_closid(void);

arch/x86/kernel/cpu/resctrl/monitor.c +2 -0

index b1d002e5e93d..ab0f4bb49bd9 100644
@@ -1093,6 +1093,27 @@ int __init rdt_get_mon_l3_config(struct
 rdt_resource *r)
       return 0;
   }
   +void arch_domain_mbm_evt_config(struct rdt_hw_domain *hw_dom)
+{
+    u64 msrval;

arch/x86/kernel/cpu/resctrl/monitor.c int __init rdt_get_mon_l3_config(struct rdt_resource *r) · line 1104

Quoted 6 lines

arch/x86/kernel/cpu/resctrl/monitor.c +6 -0

+
+    /*
+     * Read the configuration registers QOS_EVT_CFG_n, where <n> is
+     * the BMEC event number (EvtID).
+     * 0 for evtid == QOS_L3_MBM_TOTAL_EVENT_ID
+     * 1 for evtid == QOS_L3_MBM_LOCAL_EVENT_ID

This is what mon_event_config_index_get() is for, no?

Quoted 15 lines

arch/x86/kernel/cpu/resctrl/monitor.c +12 -0

+     */
+    if (mbm_total_event.configurable) {
+        rdmsrl(MSR_IA32_EVT_CFG_BASE, msrval);
+        hw_dom->mbm_total_cfg = msrval & MAX_EVT_CONFIG_BITS;
+    }
+
+    if (mbm_local_event.configurable) {
+        rdmsrl(MSR_IA32_EVT_CFG_BASE + 1, msrval);
+        hw_dom->mbm_local_cfg = msrval & MAX_EVT_CONFIG_BITS;
+    }
+}
+
   void __exit rdt_put_mon_l3_config(void)
   {

Quoted 1 line

      dom_data_exit();

Reinette

Hi Babu,

Quoted 58 lines · Babu Moger

If the BMEC (Bandwidth Monitoring Event Configuration) feature is supported, the bandwidth events can be configured to track specific events. The event configuration is domain specific. ABMC (Assignable Bandwidth Monitoring Counters) feature needs event configuration information to assign hardware counter to an RMID. Event configurations are not stored in resctrl but instead always read from or written to hardware directly when prompted by user space.

Read the event configuration from the hardware during the domain initialization. Save the configuration information in the rdt_hw_domain, so it can be used for counter assignment.

Signed-off-by Babu Moger <[email protected]>

---
v4: Read the configuration information from the hardware to initialize. Added few commit messages.
     Fixed the tab spaces.

v3: Minor changes related to rebase in mbm_config_write_domain.

v2: No changes.
---
  arch/x86/kernel/cpu/resctrl/core.c     |  2 ++ arch/x86/kernel/cpu/resctrl/internal.h |  5 +++++ arch/x86/kernel/cpu/resctrl/monitor.c  | 21 +++++++++++++++++++++

  3 files changed, 28 insertions(+)

arch/x86/kernel/cpu/resctrl/core.c +1 -0

index ec93f6a50308..856c46d12177 100644
@@ -542,6 +542,8 @@ static void domain_add_cpu(int cpu, struct
 rdt_resource *r)
           return;
       }
   +    arch_domain_mbm_evt_config(hw_dom);
+
       list_add_tail_rcu(&d->list, add_pos);
         err = resctrl_online_domain(r, d);

arch/x86/kernel/cpu/resctrl/internal.h +2 -0

index 5e7e76cd512f..60a1ca0a11a7 100644
@@ -373,6 +373,8 @@ struct arch_mbm_state {
    * @ctrl_val:    array of cache or mem ctrl values (indexed by CLOSID)
    * @arch_mbm_total:    arch private state for MBM total bandwidth
    * @arch_mbm_local:    arch private state for MBM local bandwidth
+ * @mbm_total_cfg:    MBM total bandwidth configuration
+ * @mbm_local_cfg:    MBM local bandwidth configuration
    *
    * Members of this structure are accessed via helpers that provide

abstraction.
   */

arch/x86/kernel/cpu/resctrl/internal.h +0 -0

@@ -381,6 +383,8 @@ struct rdt_hw_domain {
       u32                *ctrl_val;
       struct arch_mbm_state        *arch_mbm_total;

arch/x86/kernel/cpu/resctrl/internal.h struct rdt_hw_domain { · line 388

Quoted 4 lines

arch/x86/kernel/cpu/resctrl/internal.h +2 -0

       struct arch_mbm_state        *arch_mbm_local;
+    u32                mbm_total_cfg;
+    u32                mbm_local_cfg;
   };

Similar to the abmc_enabled member of rdt_hw_resource, these new members of rdt_hw_domain are architecture specific and should never be touched directly by resctrl fs code, for example, from mbm_config_show().

Hi Reinette,

Quoted 61 lines · Reinette Chatre

Hi Babu,

Quoted 58 lines · Babu Moger

If the BMEC (Bandwidth Monitoring Event Configuration) feature is supported, the bandwidth events can be configured to track specific events. The event configuration is domain specific. ABMC (Assignable Bandwidth Monitoring Counters) feature needs event configuration information to assign hardware counter to an RMID. Event configurations are not stored in resctrl but instead always read from or written to hardware directly when prompted by user space.

Read the event configuration from the hardware during the domain initialization. Save the configuration information in the rdt_hw_domain, so it can be used for counter assignment.

Signed-off-by Babu Moger <[email protected]>

---
v4: Read the configuration information from the hardware to initialize.
     Added few commit messages.
     Fixed the tab spaces.

v3: Minor changes related to rebase in mbm_config_write_domain.

v2: No changes.

---
   arch/x86/kernel/cpu/resctrl/core.c     |  2 ++
   arch/x86/kernel/cpu/resctrl/internal.h |  5 +++++
   arch/x86/kernel/cpu/resctrl/monitor.c  | 21 +++++++++++++++++++++
   3 files changed, 28 insertions(+)

arch/x86/kernel/cpu/resctrl/core.c +1 -0

index ec93f6a50308..856c46d12177 100644
@@ -542,6 +542,8 @@ static void domain_add_cpu(int cpu, struct
 rdt_resource *r)
           return;
       }
   +    arch_domain_mbm_evt_config(hw_dom);
+
       list_add_tail_rcu(&d->list, add_pos);
         err = resctrl_online_domain(r, d);

arch/x86/kernel/cpu/resctrl/internal.h +2 -0

index 5e7e76cd512f..60a1ca0a11a7 100644
@@ -373,6 +373,8 @@ struct arch_mbm_state {
    * @ctrl_val:    array of cache or mem ctrl values (indexed by CLOSID)
    * @arch_mbm_total:    arch private state for MBM total bandwidth
    * @arch_mbm_local:    arch private state for MBM local bandwidth
+ * @mbm_total_cfg:    MBM total bandwidth configuration
+ * @mbm_local_cfg:    MBM local bandwidth configuration
    *
    * Members of this structure are accessed via helpers that provide

abstraction.
   */

arch/x86/kernel/cpu/resctrl/internal.h +0 -0

@@ -381,6 +383,8 @@ struct rdt_hw_domain {
       u32                *ctrl_val;
       struct arch_mbm_state        *arch_mbm_total;

Quoted 8 lines

arch/x86/kernel/cpu/resctrl/internal.h struct rdt_hw_domain { · line 388

Quoted 4 lines

arch/x86/kernel/cpu/resctrl/internal.h +2 -0

       struct arch_mbm_state        *arch_mbm_local;
+    u32                mbm_total_cfg;
+    u32                mbm_local_cfg;
   };

Similar to the abmc_enabled member of rdt_hw_resource, these new members of rdt_hw_domain are architecture specific and should never be touched directly by resctrl fs code, for example, from mbm_config_show().

Need some clarification here.

I am thinking you want to introduce architecture specific routines to get and set mbm_total_config/mbm_local_config for the domain. Something like this.

+int arch_get_mbm_evt_cfg(struct rdt_domain *d, enum resctrl_event_id eventid)

(quoted diff) +26 -0

+{
+       struct rdt_hw_domain *hw_dom = resctrl_to_arch_dom(d);
+
+       switch (eventid) {
+       case QOS_L3_OCCUP_EVENT_ID:
+               break;
+       case QOS_L3_MBM_TOTAL_EVENT_ID:
+               return hw_dom->mbm_total_cfg;
+       case QOS_L3_MBM_LOCAL_EVENT_ID:
+               return hw_dom->mbm_local_cfg;
+       }
+
+       /* Never expect to get here */
+       WARN_ON_ONCE(1);
+
+       return -1;
+}
+
+void arch_set_mbm_evt_cfg(struct rdt_domain *d,
+                         enum resctrl_event_id eventid, u32 val)
+{
+       struct rdt_hw_domain *hw_dom = resctrl_to_arch_dom(d);
+
+       switch (eventid) {
+       case QOS_L3_OCCUP_EVENT_ID:
+               break;

Hi Babu,

Quoted 73 lines · Babu

Hi Reinette,

Quoted 61 lines · Reinette Chatre

Hi Babu,

Quoted 58 lines · Babu Moger

If the BMEC (Bandwidth Monitoring Event Configuration) feature is supported, the bandwidth events can be configured to track specific events. The event configuration is domain specific. ABMC (Assignable Bandwidth Monitoring Counters) feature needs event configuration information to assign hardware counter to an RMID. Event configurations are not stored in resctrl but instead always read from or written to hardware directly when prompted by user space.

Read the event configuration from the hardware during the domain initialization. Save the configuration information in the rdt_hw_domain, so it can be used for counter assignment.

Signed-off-by Babu Moger <[email protected]>

---
v4: Read the configuration information from the hardware to initialize. Added few commit messages.
      Fixed the tab spaces.

v3: Minor changes related to rebase in mbm_config_write_domain.

v2: No changes.
---
   arch/x86/kernel/cpu/resctrl/core.c     |  2 ++ arch/x86/kernel/cpu/resctrl/internal.h |  5 +++++ arch/x86/kernel/cpu/resctrl/monitor.c  | 21 +++++++++++++++++++++

   3 files changed, 28 insertions(+)

arch/x86/kernel/cpu/resctrl/core.c +1 -0

index ec93f6a50308..856c46d12177 100644
@@ -542,6 +542,8 @@ static void domain_add_cpu(int cpu, struct
 rdt_resource *r)
            return;
        }
    +    arch_domain_mbm_evt_config(hw_dom);
+
        list_add_tail_rcu(&d->list, add_pos);
          err = resctrl_online_domain(r, d);

arch/x86/kernel/cpu/resctrl/internal.h +2 -0

index 5e7e76cd512f..60a1ca0a11a7 100644
@@ -373,6 +373,8 @@ struct arch_mbm_state {
     * @ctrl_val:    array of cache or mem ctrl values (indexed by CLOSID)
     * @arch_mbm_total:    arch private state for MBM total bandwidth
     * @arch_mbm_local:    arch private state for MBM local bandwidth
+ * @mbm_total_cfg:    MBM total bandwidth configuration
+ * @mbm_local_cfg:    MBM local bandwidth configuration
     *
     * Members of this structure are accessed via helpers that provide

abstraction.
    */

arch/x86/kernel/cpu/resctrl/internal.h +0 -0

@@ -381,6 +383,8 @@ struct rdt_hw_domain {
        u32                *ctrl_val;
        struct arch_mbm_state        *arch_mbm_total;

Quoted 8 lines

arch/x86/kernel/cpu/resctrl/internal.h struct rdt_hw_domain { · line 388

Quoted 4 lines

arch/x86/kernel/cpu/resctrl/internal.h +2 -0

        struct arch_mbm_state        *arch_mbm_local;
+    u32                mbm_total_cfg;
+    u32                mbm_local_cfg;
    };

Similar to the abmc_enabled member of rdt_hw_resource, these new members of rdt_hw_domain are architecture specific and should never be touched directly by resctrl fs code, for example, from mbm_config_show().

Quoted 8 lines

Need some clarification here.

I am thinking you want to introduce architecture specific routines to get and set mbm_total_config/mbm_local_config for the domain. Something like this.

+int arch_get_mbm_evt_cfg(struct rdt_domain *d, enum resctrl_event_id eventid)

The prefix for arch specific calls converged to "resctrl_arch_".

Quoted 32 lines

(quoted diff) +32 -0

+{
+       struct rdt_hw_domain *hw_dom = resctrl_to_arch_dom(d);
+
+       switch (eventid) {
+       case QOS_L3_OCCUP_EVENT_ID:
+               break;
+       case QOS_L3_MBM_TOTAL_EVENT_ID:
+               return hw_dom->mbm_total_cfg;
+       case QOS_L3_MBM_LOCAL_EVENT_ID:
+               return hw_dom->mbm_local_cfg;
+       }
+
+       /* Never expect to get here */
+       WARN_ON_ONCE(1);
+
+       return -1;
+}
+
+void arch_set_mbm_evt_cfg(struct rdt_domain *d,
+                         enum resctrl_event_id eventid, u32 val)
+{
+       struct rdt_hw_domain *hw_dom = resctrl_to_arch_dom(d);
+
+       switch (eventid) {
+       case QOS_L3_OCCUP_EVENT_ID:
+               break;
+       case QOS_L3_MBM_TOTAL_EVENT_ID:
+               hw_dom->mbm_total_cfg = val;
+               break;
+       case QOS_L3_MBM_LOCAL_EVENT_ID:
+               hw_dom->mbm_local_cfg = val;
+       }

arch/x86/kernel/cpu/resctrl/monitor.c int __init rdt_get_mon_l3_config(struct rdt_resource *r) · line 1110

Quoted 4 lines

arch/x86/kernel/cpu/resctrl/monitor.c +4 -0

+
+       return;
+}
+

I expected that a call to set the event configuration on an architecture to also interact with the hardware. Essentially what mon_event_config_write() does today, but in addition also sets the cached mbm_total_cfg/mbm_local_cfg.

Part of this is done by the new MPAM portion [1]. With that the post-ABMC implementation of resctrl_arch_mon_event_config_read() may be what you have as arch_get_mbm_evt_cfg() above and resctrl_arch_mon_event_config_write() does the same as in [1] but with addition of updating the cached mbm_local_cfg/mbm_total_cfg within struct rdt_hw_domain. Would this work for ABMC?

Reinette

[1] https://lore.kernel.org/lkml/[email protected]/

The ABMC feature provides an option to the user to assign a hardware counter to an RMID and monitor the bandwidth as long as the counter is assigned. The bandwidth events will be tracked by the hardware until the user changes the configuration. Each resctrl group can configure maximum two counters, one for total event and one for local event.

The counters are configured by writing to MSR L3_QOS_ABMC_CFG. Configuration is done by setting the counter id, bandwidth source (RMID), bandwidth types. Reading L3_QOS_ABMC_DSC returns the configuration of

the counter id specified in L3_QOS_ABMC_CFG.

Attempts to read or write these MSRs when ABMC is not enabled will result in a #GP(0) exception.

MSR L3_QOS_ABMC_CFG (0xC000_03FDh) and L3_QOS_ABMC_DSC (0xC000_03FEh) details.
==========================================================================
Bits Mnemonic Description Access Type Reset Value
==========================================================================
63 CfgEn Configuration Enable R/W 0

62 CtrEn Counter Enable R/W 0

61:53 – Reserved MBZ 0

52:48 CtrID Counter Identifier R/W 0

47 IsCOS BwSrc field is a COS R/W 0
(not an RMID)

46:44 – Reserved MBZ 0

43:32 BwSrc Bandwidth Source R/W 0
(RMID or COS)

31:0 BwType Bandwidth types to R/W 0
track for this counter
==========================================================================

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.3.3.3 Assignable Bandwidth Monitoring (ABMC).

Signed-off-by Babu Moger <[email protected]>

Link https://bugzilla.kernel.org/show_bug.cgi?id=206537

---
v4: Added more descriptions.
Changed the name abmc_ctr_id to ctr_id.
Added L3_QOS_ABMC_DSC. Used for reading the configuration.

v3: No changes.

v2: No changes.

---
  arch/x86/include/asm/msr-index.h       |  2 ++
  arch/x86/kernel/cpu/resctrl/internal.h | 30 ++++++++++++++++++++++++++
  2 files changed, 32 insertions(+)

arch/x86/include/asm/msr-index.h +2 -0

index 5f9a0139e98c..6d2fe39ac68f 100644
@@ -1172,6 +1172,8 @@
  #define MSR_IA32_SMBA_BW_BASE		0xc0000280
  #define MSR_IA32_EVT_CFG_BASE		0xc0000400
  #define MSR_IA32_L3_QOS_EXT_CFG		0xc00003ff
+#define MSR_IA32_L3_QOS_ABMC_CFG	0xc00003fd
+#define MSR_IA32_L3_QOS_ABMC_DSC	0xc00003fe
 
  /* MSR_IA32_VMX_MISC bits */
  #define MSR_IA32_VMX_MISC_INTEL_PT                 (1ULL << 14)

arch/x86/kernel/cpu/resctrl/internal.h +4 -0

index 60a1ca0a11a7..45ed33f4f0ff 100644
@@ -107,6 +107,9 @@ cpumask_any_housekeeping(const struct cpumask *mask, int exclude_cpu)
  #define ASSIGN_TOTAL			BIT(QOS_L3_MBM_TOTAL_EVENT_ID)
  #define ASSIGN_LOCAL			BIT(QOS_L3_MBM_LOCAL_EVENT_ID)
 
+/* Maximum assignable counters per resctrl group */
+#define MAX_CNTRS		2
+
  struct rdt_fs_context {
  	struct kernfs_fs_context	kfc;
  	bool				enable_cdpl2;

@@ -211,6 +214,7 @@ enum rdtgrp_mode {
   * @crdtgrp_list:		child rdtgroup node list
   * @rmid:			rmid for this rdtgroup
   * @mon_state:			Assignment state of the group
+ * @ctr_id:			ABMC counter ids assigned to this group
   */
  struct mongroup {
  	struct kernfs_node	*mon_data_kn;

arch/x86/kernel/cpu/resctrl/internal.h +8 -0

+ */
+union l3_qos_abmc_cfg {
+	struct {
+		unsigned long  bw_type	:32,
+			       bw_src	:12,
+			       reserved1: 3,
+			       is_cos	: 1,
+			       ctr_id	: 5,

Hi Babu,

Quoted 6 lines · Babu Moger

The ABMC feature provides an option to the user to assign a hardware counter to an RMID and monitor the bandwidth as long as the counter is assigned. The bandwidth events will be tracked by the hardware until the user changes the configuration. Each resctrl group can configure maximum two counters, one for total event and one for local event.

Quoted 3 lines

The counters are configured by writing to MSR L3_QOS_ABMC_CFG. Configuration is done by setting the counter id, bandwidth source (RMID), bandwidth types. Reading L3_QOS_ABMC_DSC returns the configuration of

What is a "bandwidth type" here?

Quoted 27 lines

the counter id specified in L3_QOS_ABMC_CFG.

Attempts to read or write these MSRs when ABMC is not enabled will result in a #GP(0) exception.

MSR L3_QOS_ABMC_CFG (0xC000_03FDh) and L3_QOS_ABMC_DSC (0xC000_03FEh) details.
==========================================================================
Bits     Mnemonic    Description        Access Type   Reset Value
==========================================================================
63     CfgEn         Configuration Enable     R/W         0

62     CtrEn         Counter Enable         R/W         0

61:53     –         Reserved         MBZ         0

52:48     CtrID         Counter Identifier    R/W        0

47     IsCOS        BwSrc field is a COS    R/W        0 (not an RMID)

46:44     –        Reserved        MBZ        0

43:32    BwSrc        Bandwidth Source    R/W        0 (RMID or COS)

Quoted 4 lines

31:0    BwType        Bandwidth types to    R/W        0 track for this counter
==========================================================================

This is copy&paste from AMD spec but needs some translation to map it to resctrl.

Quoted 56 lines

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.3.3.3 Assignable Bandwidth Monitoring (ABMC).

Signed-off-by Babu Moger <[email protected]>

Link https://bugzilla.kernel.org/show_bug.cgi?id=206537

---
v4: Added more descriptions.
     Changed the name abmc_ctr_id to ctr_id.
     Added L3_QOS_ABMC_DSC. Used for reading the configuration.

v3: No changes.

v2: No changes.
---
  arch/x86/include/asm/msr-index.h       |  2 ++ arch/x86/kernel/cpu/resctrl/internal.h | 30 ++++++++++++++++++++++++++

  2 files changed, 32 insertions(+)

arch/x86/include/asm/msr-index.h +2 -0

index 5f9a0139e98c..6d2fe39ac68f 100644
@@ -1172,6 +1172,8 @@
   #define MSR_IA32_SMBA_BW_BASE        0xc0000280
   #define MSR_IA32_EVT_CFG_BASE        0xc0000400
   #define MSR_IA32_L3_QOS_EXT_CFG        0xc00003ff
+#define MSR_IA32_L3_QOS_ABMC_CFG    0xc00003fd
+#define MSR_IA32_L3_QOS_ABMC_DSC    0xc00003fe
     /* MSR_IA32_VMX_MISC bits */
   #define MSR_IA32_VMX_MISC_INTEL_PT                 (1ULL << 14)

arch/x86/kernel/cpu/resctrl/internal.h +0 -0

index 60a1ca0a11a7..45ed33f4f0ff 100644
@@ -107,6 +107,9 @@ cpumask_any_housekeeping(const struct cpumask *mask,
 int exclude_cpu)
   #define ASSIGN_TOTAL            BIT(QOS_L3_MBM_TOTAL_EVENT_ID)
   #define ASSIGN_LOCAL            BIT(QOS_L3_MBM_LOCAL_EVENT_ID)

  +/* Maximum assignable counters per resctrl group */

arch/x86/kernel/cpu/resctrl/internal.h +2 -0

+#define MAX_CNTRS        2
+

  struct rdt_fs_context {
      struct kernfs_fs_context    kfc;
      bool                enable_cdpl2;

arch/x86/kernel/cpu/resctrl/internal.h +1 -0

@@ -211,6 +214,7 @@ enum rdtgrp_mode {
    * @crdtgrp_list:        child rdtgroup node list
    * @rmid:            rmid for this rdtgroup
    * @mon_state:            Assignment state of the group
+ * @ctr_id:            ABMC counter ids assigned to this group
    */
   struct mongroup {

arch/x86/kernel/cpu/resctrl/internal.h struct mongroup { · line 225

Quoted 6 lines

arch/x86/kernel/cpu/resctrl/internal.h +1 -0

       struct kernfs_node    *mon_data_kn;

@@ -218,6 +222,7 @@ struct mongroup {
       struct list_head    crdtgrp_list;
       u32            rmid;
       u32            mon_state;
+    u32            ctr_id[MAX_CNTRS];

To match "num_cntrs" it seems "cntr_id" may be more appropriate?

Quoted 8 lines

arch/x86/kernel/cpu/resctrl/internal.h +0 -0

   };
     /**

@@ -568,6 +573,31 @@ union cpuid_0x10_x_edx {
       unsigned int full;
   };

  +/*
+ * ABMC counters can be configured  by writing to L3_QOS_ABMC_CFG.

extra space

Quoted 2 lines

+ * @bw_type        : Bandwidth types to track for this counter

What is a "Bandwidth type"? What are possible values?

Quoted 4 lines

+ * @bw_src        : Bandwidth Source (RMID or CLOSID)
+ * @reserved1        : Reserved
+ * @is_cos        : BwSrc field is a COS (not an RMID)

Please be consistent wrt "CLOS"/"CLOSID" in comment and member name

Quoted 2 lines

+ * @ctr_id        : Counter Identifier

Please be consistent with "cntr" vs "ctr". Earlier the series introduces "num_cntrs" so cntr seems appropriate?

Quoted 4 lines

+ * @reserved        : Reserved
+ * @ctr_en        : Counter Enable bit
+ * @cfg_en        : Configuration Enable bit

Why are there two enable bits? How are they used?

(please match other kernel doc wrt : usage)

Quoted 15 lines

+ */

arch/x86/kernel/cpu/resctrl/internal.h +14 -0

+union l3_qos_abmc_cfg {
+    struct {
+        unsigned long  bw_type    :32,
+                   bw_src    :12,
+                   reserved1: 3,
+                   is_cos    : 1,
+                   ctr_id    : 5,
+                   reserved    : 9,
+                   ctr_en    : 1,
+                   cfg_en    : 1;
+    } split;
+    unsigned long full;
+};
+

Quoted 3 lines

  void rdt_last_cmd_clear(void);
  void rdt_last_cmd_puts(const char *s);
  __printf(1, 2)

Reinette