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

Users can create as many monitor groups as RMIDs supported by the hardware. However, bandwidth monitoring feature on AMD system only guarantees that RMIDs currently assigned to a processor will be tracked by hardware. The counters of any other RMIDs which are no longer being tracked will be reset to zero. The MBM event counters return "Unavailable" for the RMIDs that are not tracked by hardware. So, there can be only limited number of groups that can give guaranteed monitoring numbers. With ever changing configurations there is no way to definitely know which of these groups are being tracked for certain point of time. Users do not have the option to monitor a group or set of groups for certain period of time without worrying about RMID being reset in between.

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. The assigned RMID will be tracked by the hardware until the user unassigns it manually. There is no need to worry about counters being reset during this period. Additionally, the user can specify a bitmask identifying the specific bandwidth types from the given source to track with the counter.

Without ABMC enabled, monitoring will work in current mode without assignment option.

Linux resctrl subsystem provides the interface to count maximum of two memory bandwidth events per group, from a combination of available total and local events. Keeping the current interface, users can enable a maximum of 2 ABMC counters per group. User will also have the option to enable only one counter to the group. If the system runs out of assignable ABMC counters, kernel will display an error. Users need to disable an already enabled counter to make space for new assignments.

The feature can be detected via CPUID_Fn80000020_EBX_x00 bit 5. Bits Description
5 ABMC (Assignable Bandwidth Monitoring Counters)

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).

Note: Checkpatch checks/warnings are ignored to maintain coding style.

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

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

---

Hi Babu,

Quoted 34 lines · Babu Moger

Users can create as many monitor groups as RMIDs supported by the hardware. However, bandwidth monitoring feature on AMD system only guarantees that RMIDs currently assigned to a processor will be tracked by hardware. The counters of any other RMIDs which are no longer being tracked will be reset to zero. The MBM event counters return "Unavailable" for the RMIDs that are not tracked by hardware. So, there can be only limited number of groups that can give guaranteed monitoring numbers. With ever changing configurations there is no way to definitely know which of these groups are being tracked for certain point of time. Users do not have the option to monitor a group or set of groups for certain period of time without worrying about RMID being reset in between.

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. The assigned RMID will be tracked by the hardware until the user unassigns it manually. There is no need to worry about counters being reset during this period. Additionally, the user can specify a bitmask identifying the specific bandwidth types from the given source to track with the counter.

Without ABMC enabled, monitoring will work in current mode without assignment option.

Linux resctrl subsystem provides the interface to count maximum of two memory bandwidth events per group, from a combination of available total and local events. Keeping the current interface, users can enable a maximum of 2 ABMC counters per group. User will also have the option to enable only one counter to the group. If the system runs out of assignable ABMC counters, kernel will display an error. Users need to disable an already enabled counter to make space for new assignments.

The feature can be detected via CPUID_Fn80000020_EBX_x00 bit 5. Bits Description
5    ABMC (Assignable Bandwidth Monitoring Counters)

Quoted 6 lines

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).

Note: Checkpatch checks/warnings are ignored to maintain coding style.

This note may be more appropriate below the '---' separator line.

Quoted 5 lines

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

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

---

Reviewed-by Reinette Chatre <[email protected]>

The cache allocation and memory bandwidth allocation feature properties are consolidated into cache and membw structures respectively. In

preparation for more monitoring properties that will clobber the existing resource struct more, re-organize the monitoring specific properties into separate structure.

Suggested-by Reinette Chatre <[email protected]>

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

---

include/linux/resctrl.h +11 -1

index b0875b99e811..e43fc5bb5a3a 100644
@@ -182,6 +182,16 @@ enum resctrl_scope {
  	RESCTRL_L3_NODE,
  };
 
+/**
+ * struct resctrl_mon - Monitoring related data
+ * @num_rmid:		Number of RMIDs available
+ * @evt_list:		List of monitoring events
+ */
+struct resctrl_mon {
+	int			num_rmid;
+	struct list_head	evt_list;
+};
+
  /**
   * struct rdt_resource - attributes of a resctrl resource
   * @rid:		The index of the resource

@@ -207,11 +217,11 @@ struct rdt_resource {
  	int			rid;
  	bool			alloc_capable;
  	bool			mon_capable;
-	int			num_rmid;
  	enum resctrl_scope	ctrl_scope;
  	enum resctrl_scope	mon_scope;
  	struct resctrl_cache	cache;
  	struct resctrl_membw	membw;
+	struct resctrl_mon	mon;
  	struct list_head	ctrl_domains;
  	struct list_head	mon_domains;
  	char			*name;

Hi Babu,

Quoted 2 lines · Babu Moger

The cache allocation and memory bandwidth allocation feature properties are consolidated into cache and membw structures respectively. In

Let "In preparation ... " start a new paragraph.

Quoting Documentation/process/maintainer-tip.rst:
    It's also useful to structure the changelog into several paragraphs and not lump everything together into a single one. A good structure is to explain the context, the problem and the solution in separate paragraphs and this order.

Quoted 4 lines

preparation for more monitoring properties that will clobber the existing resource struct more, re-organize the monitoring specific properties into separate structure.

"re-organize the monitoring specific properties into separate structure" -> "re-organize the monitoring specific properties to also be in a separate structure."

Quoted 5 lines

Suggested-by Reinette Chatre <[email protected]>

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

---

...

Quoted 37 lines

include/linux/resctrl.h +0 -0

index b0875b99e811..e43fc5bb5a3a 100644
@@ -182,6 +182,16 @@ enum resctrl_scope {
       RESCTRL_L3_NODE,
   };

  +/**
+ * struct resctrl_mon - Monitoring related data
+ * @num_rmid:        Number of RMIDs available
+ * @evt_list:        List of monitoring events
+ */

include/linux/resctrl.h +5 -0

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

  /**
   * struct rdt_resource - attributes of a resctrl resource
   * @rid:        The index of the resource

include/linux/resctrl.h +1 -1

@@ -207,11 +217,11 @@ struct rdt_resource {
       int            rid;
       bool            alloc_capable;
       bool            mon_capable;
-    int            num_rmid;
       enum resctrl_scope    ctrl_scope;
       enum resctrl_scope    mon_scope;
       struct resctrl_cache    cache;
       struct resctrl_membw    membw;
+    struct resctrl_mon    mon;
       struct list_head    ctrl_domains;
       struct list_head    mon_domains;
       char            *name;

@@ -221,7 +231,6 @@ struct rdt_resource {
       int            (*parse_ctrlval)(struct rdt_parse_data *data,
                            struct resctrl_schema *s,
                            struct rdt_ctrl_domain *d);

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

Quoted 4 lines

include/linux/resctrl.h +0 -1

-    struct list_head    evt_list;
       unsigned long        fflags;
       bool            cdp_capable;
   };

struct rdt_resource's kernel-doc still refers to the members removed in this patch. Its kernel-doc also needs an update for the new member added.

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]>

---
v5: Name change num_cntrs to num_mbm_cntrs.
Moved abmc_capable to resctrl_mon.

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 | 12 ++++++++++++
  include/linux/resctrl.h               |  4 ++++
  2 files changed, 16 insertions(+)

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

index 795fe91a8feb..87d40f149ebc 100644
@@ -1229,6 +1229,18 @@ 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->mon.abmc_capable = true;
+			/*
+			 * Query CPUID_Fn80000020_EBX_x05 for number of
+			 * ABMC counters
+			 */
+			cpuid_count(0x80000020, 5, &eax, &ebx, &ecx, &edx);
+			r->mon.num_mbm_cntrs = (ebx & 0xFFFF) + 1;
+			if (WARN_ON(r->mon.num_mbm_cntrs > 64))
+				r->mon.num_mbm_cntrs = 64;
+		}
  	}
 
  	l3_mon_evt_init(r);

include/linux/resctrl.h +1 -0

index e43fc5bb5a3a..62f0f002ef41 100644
@@ -185,10 +185,14 @@ enum resctrl_scope {
  /**
   * struct resctrl_mon - Monitoring related data
   * @num_rmid:		Number of RMIDs available
+ * @num_mbm_cntrs:	Number of monitoring counters

Hi Babu,

Quoted 5 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

Quoted 4 lines

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).

<insert snippet about what the patch does>

Quoted 63 lines

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

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

---
v5: Name change num_cntrs to num_mbm_cntrs.
     Moved abmc_capable to resctrl_mon.

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 | 12 ++++++++++++ include/linux/resctrl.h               |  4 ++++

  2 files changed, 16 insertions(+)

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

index 795fe91a8feb..87d40f149ebc 100644
@@ -1229,6 +1229,18 @@ 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->mon.abmc_capable = true;
+            /*
+             * Query CPUID_Fn80000020_EBX_x05 for number of
+             * ABMC counters
+             */
+            cpuid_count(0x80000020, 5, &eax, &ebx, &ecx, &edx);
+            r->mon.num_mbm_cntrs = (ebx & 0xFFFF) + 1;
+            if (WARN_ON(r->mon.num_mbm_cntrs > 64))
+                r->mon.num_mbm_cntrs = 64;
+        }
       }
         l3_mon_evt_init(r);

include/linux/resctrl.h +4 -0

index e43fc5bb5a3a..62f0f002ef41 100644
@@ -185,10 +185,14 @@ enum resctrl_scope {
   /**
    * struct resctrl_mon - Monitoring related data
    * @num_rmid:        Number of RMIDs available
+ * @num_mbm_cntrs:    Number of monitoring counters
+ * @abmc_capable:    Is system capable of supporting monitor assignment?
    * @evt_list:        List of monitoring events
    */
   struct resctrl_mon {
       int            num_rmid;
+    int            num_mbm_cntrs;
+    bool            abmc_capable;

Quoted 3 lines

include/linux/resctrl.h +0 -0

       struct list_head    evt_list;
   };

How about renaming "abmc_capable" to "mbm_cntr_capable? That would,
(a) connect the capability to the "num_mbm_cntrs" property, and (b) remove the AMD marketing name from the resctrl filesystem code that will be shared by all architectures.

Quoted 18 lines · Reinette Chatre

Quoted 16 lines · Babu Moger

include/linux/resctrl.h +4 -0

index e43fc5bb5a3a..62f0f002ef41 100644
@@ -185,10 +185,14 @@ enum resctrl_scope {
   /**
    * struct resctrl_mon - Monitoring related data
    * @num_rmid:        Number of RMIDs available
+ * @num_mbm_cntrs:    Number of monitoring counters
+ * @abmc_capable:    Is system capable of supporting monitor assignment?
    * @evt_list:        List of monitoring events
    */
   struct resctrl_mon {
       int            num_rmid;
+    int            num_mbm_cntrs;
+    bool            abmc_capable;

Quoted 8 lines

include/linux/resctrl.h enum resctrl_scope { · line 198

Quoted 3 lines

include/linux/resctrl.h +0 -0

       struct list_head    evt_list;
   };
 

How about renaming "abmc_capable" to "mbm_cntr_capable? That would,
(a) connect the capability to the "num_mbm_cntrs" property, and (b) remove the AMD marketing name from the resctrl filesystem code that will be shared by all architectures.

"mbm_cntr_capable" does not give full meaning of the feature.

How about "mbm_cntr_assignable"?

thread_throttle_mode_init() and mbm_config_rftype_init() both initialize fflags for resctrl files.

Adding new files will involve adding another function to initialize the fflags. This can be simplified by adding a new function resctrl_file_fflags_init() and passing the file name and flags to be initialized.

Consolidate 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]>

---

Add the functionality to enable/disable AMD ABMC feature.

AMD 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. Reset the architectural state so that reading of hardware counter is not considered as an overflow in 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

---
v5: Renamed resctrl_abmc_enable to resctrl_arch_abmc_enable.
Renamed resctrl_abmc_disable to resctrl_arch_abmc_disable. Introduced resctrl_arch_get_abmc_enabled to get abmc state from non-arch code.
Renamed resctrl_abmc_set_all to _resctrl_abmc_enable(). Modified commit log to make it clear about AMD ABMC feature.

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 | 13 +++++
  arch/x86/kernel/cpu/resctrl/rdtgroup.c | 66 ++++++++++++++++++++++++++
  3 files changed, 80 insertions(+)

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

index 01342963011e..263b2d9d00ed 100644
@@ -1174,6 +1174,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 2bd207624eec..0ce9797f80fe 100644

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

+#define ABMC_ENABLE			BIT(0)
+
  struct rdt_fs_context {
  	struct kernfs_fs_context	kfc;
  	bool				enable_cdpl2;

@@ -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
+ * @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 +8 -0

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

@@ -536,6 +541,14 @@ 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_abmc_enabled(void)
+{
+	return rdt_resources_all[RDT_RESOURCE_L3].abmc_enabled;
+}
+
+int resctrl_arch_abmc_enable(void);
+void resctrl_arch_abmc_disable(void);
+
  /*
   * 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 +7 -0

index 7e76f8d839fc..471fc0dbd7c3 100644
@@ -2402,6 +2402,72 @@ int resctrl_arch_set_cdp_enabled(enum resctrl_res_level l, bool enable)
  	return 0;
  }
 
+/*
+ * Update L3_QOS_EXT_CFG MSR on all the CPUs associated with the resource.
+ */
+static void resctrl_abmc_set_one_amd(void *arg)
+{
+	bool *enable = arg;
+	u64 msrval;

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

+
+static int _resctrl_abmc_enable(struct rdt_resource *r, bool enable)
+{
+	struct rdt_mon_domain *d;
+
+	/*
+	 * Hardware counters will reset after switching the monitor mode.
+	 * Reset the architectural state so that reading of hardware

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

+}
+
+void resctrl_arch_abmc_disable(void)
+{
+	struct rdt_resource *r = &rdt_resources_all[RDT_RESOURCE_L3].r_resctrl;
+	struct rdt_hw_resource *hw_res = resctrl_to_arch_res(r);
+
+	lockdep_assert_held(&rdtgroup_mutex);

Hi Babu,

Quoted 50 lines · Babu Moger

Add the functionality to enable/disable AMD ABMC feature.

AMD 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. Reset the architectural state so that reading of hardware counter is not considered as an overflow in 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

---
v5: Renamed resctrl_abmc_enable to resctrl_arch_abmc_enable. Renamed resctrl_abmc_disable to resctrl_arch_abmc_disable. Introduced resctrl_arch_get_abmc_enabled to get abmc state from non-arch code.
     Renamed resctrl_abmc_set_all to _resctrl_abmc_enable(). Modified commit log to make it clear about AMD ABMC feature.

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 | 13 +++++ arch/x86/kernel/cpu/resctrl/rdtgroup.c | 66 ++++++++++++++++++++++++++

  3 files changed, 80 insertions(+)

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

index 01342963011e..263b2d9d00ed 100644
@@ -1174,6 +1174,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 -1

index 2bd207624eec..0ce9797f80fe 100644
--- a/arch/x86/kernel/cpu/resctrl/internal.h

Quoted 6 lines

+++ b/arch/x86/kernel/cpu/resctrl/internal.h

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

@@ -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 */

Please be consistent throughout series to have sentences end with period.

Quoted 18 lines

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 +1 -0

@@ -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
+ * @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.

@@ -491,6 +495,7 @@ struct rdt_hw_resource {
       unsigned int        mbm_width;
       unsigned int        mbm_cfg_mask;
       bool            cdp_enabled;

arch/x86/kernel/cpu/resctrl/internal.h struct rdt_hw_resource { · line 499

Quoted 2 lines

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

+    bool            abmc_enabled;
   };

mbm_cntr_enabled? This is architecture specific code so there is more flexibility
here, but it may make implementation easier to understand if consistent naming is used
between fs and arch code.

Quoted 38 lines

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

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

@@ -536,6 +541,14 @@ 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_abmc_enabled(void)

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

+{
+    return rdt_resources_all[RDT_RESOURCE_L3].abmc_enabled;
+}
+
+int resctrl_arch_abmc_enable(void);
+void resctrl_arch_abmc_disable(void);
+

  /*
   * 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 +0 -0

index 7e76f8d839fc..471fc0dbd7c3 100644
@@ -2402,6 +2402,72 @@ int resctrl_arch_set_cdp_enabled(enum
 resctrl_res_level l, bool enable)
       return 0;
   }

  +/*
+ * Update L3_QOS_EXT_CFG MSR on all the CPUs associated with the resource.
+ */

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

+static void resctrl_abmc_set_one_amd(void *arg)
+{
+    bool *enable = arg;
+    u64 msrval;
+
+    rdmsrl(MSR_IA32_L3_QOS_EXT_CFG, msrval);
+
+    if (*enable)

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

Quoted 6 lines

+        msrval |= ABMC_ENABLE;
+    else
+        msrval &= ~ABMC_ENABLE;

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

+
+    wrmsrl(MSR_IA32_L3_QOS_EXT_CFG, msrval);
+}

msr_set_bit() and msr_clear_bit() can be used here.

Quoted 13 lines

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

+
+static int _resctrl_abmc_enable(struct rdt_resource *r, bool enable)
+{
+    struct rdt_mon_domain *d;
+
+    /*
+     * Hardware counters will reset after switching the monitor mode.
+     * Reset the architectural state so that reading of hardware
+     * counter is not considered as an overflow in the next update.
+     */
+    list_for_each_entry(d, &r->mon_domains, hdr.list) {
+        on_each_cpu_mask(&d->hdr.cpu_mask,

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

Quoted 6 lines

+                 resctrl_abmc_set_one_amd, &enable, 1); +        resctrl_arch_reset_rmid_all(r, d);
+    }

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

+
+    return 0;
+}

Seems like _resctrl_abmc_enable() can just return void.

arch/x86/kernel/cpu/resctrl/rdtgroup.c

Quoted 3 lines

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

+
+int resctrl_arch_abmc_enable(void)

resctrl_arch_mbm_cntr_enable()? I'll no longer point all these out.

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

Quoted 13 lines

+{
+    struct rdt_resource *r = &rdt_resources_all[RDT_RESOURCE_L3].r_resctrl;
+    struct rdt_hw_resource *hw_res = resctrl_to_arch_res(r); +    int ret = 0;

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

+
+    lockdep_assert_held(&rdtgroup_mutex);
+
+    if (r->mon.abmc_capable && !hw_res->abmc_enabled) {
+        ret = _resctrl_abmc_enable(r, true);
+        if (!ret)
+            hw_res->abmc_enabled = true;

The above error handling seems unnecessary.

Quoted 4 lines

+    }
+
+    return ret;

resctrl_arch_abmc_enable() should probably keep returning an int even though this implementation does not need it since other archs may indeed return error.

Quoted 17 lines

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

+}
+
+void resctrl_arch_abmc_disable(void)
+{
+    struct rdt_resource *r =

&rdt_resources_all[RDT_RESOURCE_L3].r_resctrl;
+    struct rdt_hw_resource *hw_res = resctrl_to_arch_res(r);

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

+
+    lockdep_assert_held(&rdtgroup_mutex);
+
+    if (hw_res->abmc_enabled) {
+        _resctrl_abmc_enable(r, false);
+        hw_res->abmc_enabled = false;
+    }
+}
+

Quoted 3 lines

  /*
   * We don't allow rdtgroup directories to be created anywhere
   * except the root directory. Thus when looking for the rdtgroup

Reinette

Quoted 51 lines · Reinette Chatre

Quoted 50 lines · Babu Moger

Add the functionality to enable/disable AMD ABMC feature.

AMD 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. Reset the architectural state so that reading of hardware counter is not considered as an overflow in 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

---
v5: Renamed resctrl_abmc_enable to resctrl_arch_abmc_enable.
     Renamed resctrl_abmc_disable to resctrl_arch_abmc_disable. Introduced resctrl_arch_get_abmc_enabled to get abmc state from non-arch code.
     Renamed resctrl_abmc_set_all to _resctrl_abmc_enable(). Modified commit log to make it clear about AMD ABMC feature.

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 | 13 +++++
   arch/x86/kernel/cpu/resctrl/rdtgroup.c | 66 ++++++++++++++++++++++++++
   3 files changed, 80 insertions(+)

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

index 01342963011e..263b2d9d00ed 100644
@@ -1174,6 +1174,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 -1

index 2bd207624eec..0ce9797f80fe 100644
--- a/arch/x86/kernel/cpu/resctrl/internal.h

Quoted 8 lines

arch/x86/kernel/cpu/resctrl/internal.h cpumask_any_housekeeping(const struct cpumask *mask, · line 100

Quoted 6 lines

+++ b/arch/x86/kernel/cpu/resctrl/internal.h

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

@@ -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 */

Please be consistent throughout series to have sentences end with period.

Sure.

Quoted 18 lines

Quoted 18 lines

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

+#define ABMC_ENABLE            BIT(0)
+
   struct rdt_fs_context {
       struct kernfs_fs_context    kfc;
       bool                enable_cdpl2;

@@ -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
+ * @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.

@@ -491,6 +495,7 @@ struct rdt_hw_resource {
       unsigned int        mbm_width;
       unsigned int        mbm_cfg_mask;
       bool            cdp_enabled;

Quoted 8 lines

arch/x86/kernel/cpu/resctrl/internal.h struct rdt_hw_resource { · line 499

Quoted 2 lines

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

+    bool            abmc_enabled;
   };

mbm_cntr_enabled? This is architecture specific code so there is more flexibility
here, but it may make implementation easier to understand if consistent naming is used
between fs and arch code.

How about "mbm_cntr_assign_enabled" or "cntr_assign_enabled" ?

Hi Babu,

Quoted 92 lines · Babu

Quoted 51 lines · Reinette Chatre

Quoted 50 lines · Babu Moger

Add the functionality to enable/disable AMD ABMC feature.

AMD 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. Reset the architectural state so that reading of hardware counter is not considered as an overflow in 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

---
v5: Renamed resctrl_abmc_enable to resctrl_arch_abmc_enable. Renamed resctrl_abmc_disable to resctrl_arch_abmc_disable. Introduced resctrl_arch_get_abmc_enabled to get abmc state from non-arch code.
      Renamed resctrl_abmc_set_all to _resctrl_abmc_enable(). Modified commit log to make it clear about AMD ABMC feature.

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 | 13 +++++ arch/x86/kernel/cpu/resctrl/rdtgroup.c | 66 ++++++++++++++++++++++++++

   3 files changed, 80 insertions(+)

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

index 01342963011e..263b2d9d00ed 100644
@@ -1174,6 +1174,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 -1

index 2bd207624eec..0ce9797f80fe 100644
--- a/arch/x86/kernel/cpu/resctrl/internal.h

Quoted 8 lines

Quoted 6 lines

+++ b/arch/x86/kernel/cpu/resctrl/internal.h

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

@@ -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 */

Please be consistent throughout series to have sentences end with period.

Sure.

Quoted 28 lines

Quoted 17 lines

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 +0 -0

@@ -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
+ * @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 struct rdt_hw_resource { · line 499

Quoted 5 lines

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

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

mbm_cntr_enabled? This is architecture specific code so there is more flexibility
here, but it may make implementation easier to understand if consistent naming is used
between fs and arch code.

Quoted 1 line

How about "mbm_cntr_assign_enabled" or "cntr_assign_enabled" ?

My preference is to keep the term "mbm_cntr" to be consistent with the other variables/struct members to help when reading the code. "mbm_cntr_assign_enabled" does seem to be getting long though. Are you planning to use it by assigning it to a local variable with shorter name?

As a sidenote, I will be offline for large portions of the next few weeks and thus unresponsive during this time. I'll be back to a regular schedule on August 12th.

Quoted 19 lines · Reinette Chatre

Quoted 17 lines · Babu

Quoted 14 lines · Reinette Chatre

arch/x86/kernel/cpu/resctrl/internal.h struct rdt_hw_resource { · line 499

Quoted 6 lines · Babu Moger

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

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

mbm_cntr_enabled? This is architecture specific code so there is more flexibility
here, but it may make implementation easier to understand if consistent naming is used
between fs and arch code.

How about "mbm_cntr_assign_enabled" or "cntr_assign_enabled" ?

Quoted 5 lines

My preference is to keep the term "mbm_cntr" to be consistent with the other variables/struct members to help when reading the code. "mbm_cntr_assign_enabled" does seem to be getting long though. Are you planning to use it by assigning it to a local variable with shorter name?

Yes. We can do that.

Hi Babu,

Quoted 92 lines · Babu

Quoted 51 lines · Reinette Chatre

Quoted 50 lines · Babu Moger

Add the functionality to enable/disable AMD ABMC feature.

AMD 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. Reset the architectural state so that reading of hardware counter is not considered as an overflow in 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

---
v5: Renamed resctrl_abmc_enable to resctrl_arch_abmc_enable. Renamed resctrl_abmc_disable to resctrl_arch_abmc_disable. Introduced resctrl_arch_get_abmc_enabled to get abmc state from non-arch code.
      Renamed resctrl_abmc_set_all to _resctrl_abmc_enable(). Modified commit log to make it clear about AMD ABMC feature.

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 | 13 +++++ arch/x86/kernel/cpu/resctrl/rdtgroup.c | 66 ++++++++++++++++++++++++++

   3 files changed, 80 insertions(+)

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

index 01342963011e..263b2d9d00ed 100644
@@ -1174,6 +1174,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 -1

index 2bd207624eec..0ce9797f80fe 100644
--- a/arch/x86/kernel/cpu/resctrl/internal.h

Quoted 8 lines

Quoted 6 lines

+++ b/arch/x86/kernel/cpu/resctrl/internal.h

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

@@ -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 */

Please be consistent throughout series to have sentences end with period.

Sure.

Quoted 20 lines

Quoted 20 lines

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 +0 -0

@@ -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
+ * @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 +0 -0

@@ -491,6 +495,7 @@ struct rdt_hw_resource {
        unsigned int        mbm_width;
        unsigned int        mbm_cfg_mask;
        bool            cdp_enabled;

Quoted 8 lines

arch/x86/kernel/cpu/resctrl/internal.h struct rdt_hw_resource { · line 499

Quoted 2 lines

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

+    bool            abmc_enabled;
    };

mbm_cntr_enabled? This is architecture specific code so there is more flexibility
here, but it may make implementation easier to understand if consistent naming is used
between fs and arch code.

Quoted 1 line

How about "mbm_cntr_assign_enabled" or "cntr_assign_enabled" ?

My preference is to keep the term "mbm_cntr" to be consistent with the other variables/struct members to help when reading the code. "mbm_cntr_assign_enabled" does seem to be getting long though. Are you planning to use it by assigning it to a local variable with shorter name?

As a sidenote, I will be offline for large portions of the next few weeks and thus unresponsive during this time. I'll be back to a regular schedule on August 12th.

Quoted 32 lines · Babu Moger

Add the functionality to enable/disable AMD ABMC feature.

AMD 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. Reset the architectural state so that reading of hardware counter is not considered as an overflow in 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).

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

index 7e76f8d839fc..471fc0dbd7c3 100644
@@ -2402,6 +2402,72 @@ int resctrl_arch_set_cdp_enabled(enum resctrl_res_level l, bool
 
+int resctrl_arch_abmc_enable(void)
+{
+    struct rdt_resource *r = &rdt_resources_all[RDT_RESOURCE_L3].r_resctrl;
+    struct rdt_hw_resource *hw_res = resctrl_to_arch_res(r);
+    int ret = 0;
+
+    lockdep_assert_held(&rdtgroup_mutex);
+
+    if (r->mon.abmc_capable && !hw_res->abmc_enabled) {
+        ret = _resctrl_abmc_enable(r, true);

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

Quoted 5 lines

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

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

resctrl_arch_abmc_enable() should probably keep returning an int even though this implementation does not need it since other archs may indeed return error.

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_mode
     [abmc]
     legacy

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

---
v5: Changed interface name to mbm_mode.
It will be always available even if ABMC feature is not supported. Added description in resctrl.rst about ABMC mode.
Fixed display abmc and legacy consistantly.

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

v3: New patch to display ABMC capability.

---
  Documentation/arch/x86/resctrl.rst     | 30 ++++++++++++++++++++++++++
  arch/x86/kernel/cpu/resctrl/monitor.c  |  2 ++
  arch/x86/kernel/cpu/resctrl/rdtgroup.c | 26 ++++++++++++++++++++++
  3 files changed, 58 insertions(+)

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

index 30586728a4cd..108e494fd7cc 100644
@@ -257,6 +257,36 @@ with the following files:
  	    # cat /sys/fs/resctrl/info/L3_MON/mbm_local_bytes_config
  	    0=0x30;1=0x30;3=0x15;4=0x15

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

+	The bandwidth monitoring feature on AMD system only guarantees that
+	RMIDs currently assigned to a processor will be tracked by hardware.
+	The counters of any other RMIDs which are no longer being tracked
+	will be reset to zero. The MBM event counters return "Unavailable"
+	for the RMIDs that are not tracked by hardware. So, there can be
+	only limited number of groups that can give guaranteed monitoring
+	numbers. With ever changing configurations there is no way to
+	definitely know which of these groups are being tracked for certain
+	point of time. Users do not have the option to monitor a group or
+	set of groups for certain period of time without worrying about

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

+
  "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 +0 -0

index 12793762ca24..6c4cb36b4b50 100644

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

  	l3_mon_evt_init(r);
 
  	r->mon_capable = true;

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

index 471fc0dbd7c3..3988d7b86817 100644
@@ -845,6 +845,26 @@ static int rdtgroup_rmid_show(struct kernfs_open_file *of,
  	return ret;
  }
 
+static int rdtgroup_mbm_mode_show(struct kernfs_open_file *of,
+				  struct seq_file *s, void *v)
+{
+	struct rdt_resource *r = of->kn->parent->priv;
+
+	if (r->mon.abmc_capable) {
+		if (resctrl_arch_get_abmc_enabled()) {
+			seq_puts(s, "[abmc]\n");
+			seq_puts(s, "legacy\n");
+		} else {
+			seq_puts(s, "abmc\n");
+			seq_puts(s, "[legacy]\n");
+		}
+	} else {
+		seq_puts(s, "[legacy]\n");
+	}
+
+	return 0;
+}
+
  #ifdef CONFIG_PROC_CPU_RESCTRL
 
  /*

@@ -1901,6 +1921,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_mode
     [abmc]
     legacy

<insert snippet about what happens when user switches from one mode to another>

Quoted 30 lines

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

---
v5: Changed interface name to mbm_mode.
     It will be always available even if ABMC feature is not supported. Added description in resctrl.rst about ABMC mode.
     Fixed display abmc and legacy consistantly.

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

v3: New patch to display ABMC capability.
---
  Documentation/arch/x86/resctrl.rst     | 30 ++++++++++++++++++++++++++ arch/x86/kernel/cpu/resctrl/monitor.c  |  2 ++ arch/x86/kernel/cpu/resctrl/rdtgroup.c | 26 ++++++++++++++++++++++

  3 files changed, 58 insertions(+)

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

index 30586728a4cd..108e494fd7cc 100644
@@ -257,6 +257,36 @@ with the following files:
           # cat /sys/fs/resctrl/info/L3_MON/mbm_local_bytes_config

          0=0x30;1=0x30;3=0x15;4=0x15
  +"mbm_mode":
+    Reports the list of assignable monitoring features supported. The +    enclosed brackets indicate which feature is enabled.
+    ::

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

Quoted 5 lines

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

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

"mbm_cntr" mode can be documented here with the details on how AMD's ABMC is one example of how it may be implemented on a system.

Quoted 15 lines

+    The bandwidth monitoring feature on AMD system only guarantees that +    RMIDs currently assigned to a processor will be tracked by hardware. +    The counters of any other RMIDs which are no longer being tracked +    will be reset to zero. The MBM event counters return "Unavailable" +    for the RMIDs that are not tracked by hardware. So, there can be +    only limited number of groups that can give guaranteed monitoring +    numbers. With ever changing configurations there is no way to +    definitely know which of these groups are being tracked for certain +    point of time. Users do not have the option to monitor a group or +    set of groups for certain period of time without worrying about +    RMID being reset in between.
+
+    The ABMC feature provides an option to the user to assign a +    hardware counter to an RMID and monitor the bandwidth as long as

Quoted 6 lines

+    it is assigned. The assigned RMID will be tracked by the hardware +    until the user unassigns it manually. There is no need to worry +    about counters being reset during this period.
+
+    Without ABMC enabled, monitoring will work in "legacy" mode +    without assignment option.

Let "legacy" be a distinct mode, instead of an alternative to ABMC.

Quoted 13 lines

+
  "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 +0 -0

index 12793762ca24..6c4cb36b4b50 100644
@@ -1245,6 +1245,8 @@ int __init rdt_get_mon_l3_config(struct
 rdt_resource *r)
           }

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

Quoted 3 lines

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

       }
   +    resctrl_file_fflags_init("mbm_mode", RFTYPE_MON_INFO);
+

Is this special flag assignment necessary? With file always visible I think it
can just be initialized in res_common_files below with the flag already assigned?

Quoted 37 lines

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

       l3_mon_evt_init(r);
         r->mon_capable = true;

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

index 471fc0dbd7c3..3988d7b86817 100644
@@ -845,6 +845,26 @@ static int rdtgroup_rmid_show(struct
 kernfs_open_file *of,
       return ret;
   }
   +static int rdtgroup_mbm_mode_show(struct kernfs_open_file *of,
+                  struct seq_file *s, void *v)
+{
+    struct rdt_resource *r = of->kn->parent->priv;
+
+    if (r->mon.abmc_capable) {
+        if (resctrl_arch_get_abmc_enabled()) {
+            seq_puts(s, "[abmc]\n");
+            seq_puts(s, "legacy\n");
+        } else {
+            seq_puts(s, "abmc\n");
+            seq_puts(s, "[legacy]\n");
+        }
+    } else {
+        seq_puts(s, "[legacy]\n");
+    }
+
+    return 0;
+}
+
   #ifdef CONFIG_PROC_CPU_RESCTRL
     /*

@@ -1901,6 +1921,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 1932

Quoted 10 lines

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

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

Reinette

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 enabled.

Introduce mbm_cntrs_free_map bitmap to track available and free counters.

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

---
v5:
Updated the comments and commit log.
Few renames
num_cntrs_free_map -> mbm_cntrs_free_map num_cntrs_init -> mbm_cntrs_init
Added initialization in rdt_get_tree because the default ABMC enablement happens during the init.

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 | 29 ++++++++++++++++++++++++--
  1 file changed, 27 insertions(+), 2 deletions(-)

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

index 4f47f52e01c2..b3d3fa048f15 100644

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

+static unsigned int mbm_cntrs_free_map_len;
+
+static void mbm_cntrs_init(void)
+{
+	struct rdt_resource *r = &rdt_resources_all[RDT_RESOURCE_L3].r_resctrl;
+
+	bitmap_fill(&mbm_cntrs_free_map, r->mon.num_mbm_cntrs);
+	mbm_cntrs_free_map_len = r->mon.num_mbm_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 +4 -1

  	/*
  	 * Hardware counters will reset after switching the monitor mode.
  	 * Reset the architectural state so that reading of hardware

@@ -2724,10 +2747,10 @@ static void schemata_list_destroy(void)
 
  static int rdt_get_tree(struct fs_context *fc)
  {
+	struct rdt_resource *r = &rdt_resources_all[RDT_RESOURCE_L3].r_resctrl;
  	struct rdt_fs_context *ctx = rdt_fc2context(fc);
  	unsigned long flags = RFTYPE_CTRL_BASE;
  	struct rdt_mon_domain *dom;
-	struct rdt_resource *r;
  	int ret;
 
  	cpus_read_lock();

@@ -2756,6 +2779,9 @@ static int rdt_get_tree(struct fs_context *fc)
 
  	closid_init();
 
+	if (r->mon.abmc_capable)
+		mbm_cntrs_init();
+
  	if (resctrl_arch_mon_capable())
  		flags |= RFTYPE_MON;
 

Hi Babu,

Quoted 38 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 enabled.

Introduce mbm_cntrs_free_map bitmap to track available and free counters.

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

---
v5:
   Updated the comments and commit log.
   Few renames
    num_cntrs_free_map -> mbm_cntrs_free_map num_cntrs_init -> mbm_cntrs_init
    Added initialization in rdt_get_tree because the default ABMC enablement happens during the init.

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 | 29 ++++++++++++++++++++++++--

  1 file changed, 27 insertions(+), 2 deletions(-)

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

index 4f47f52e01c2..b3d3fa048f15 100644
@@ -185,6 +185,23 @@ bool closid_allocated(unsigned int closid)
       return !test_bit(closid, &closid_free_map);
   }

  +/*

Quoted 3 lines

+ * Counter bitmap and its length for tracking available counters.
+ * ABMC feature provides set of hardware counters for enabling events.
+ * Each event takes one hardware counter. Kernel needs to keep track

What is meant with "Kernel" here? It looks to be the fs code but the implementation has both fs and arch code reaching into the counter management. This should not be the case, either the fs code or the arch code needs to manage the counters, not both.

Quoted 4 lines

+ * of number of available counters.
+ */
+static unsigned long mbm_cntrs_free_map;

With the lengths involved this needs a proper DECLARE_BITMAP()

Quoted 21 lines

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

+static unsigned int mbm_cntrs_free_map_len;
+
+static void mbm_cntrs_init(void)
+{
+    struct rdt_resource *r =

&rdt_resources_all[RDT_RESOURCE_L3].r_resctrl;

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

+
+    bitmap_fill(&mbm_cntrs_free_map, r->mon.num_mbm_cntrs);
+    mbm_cntrs_free_map_len = r->mon.num_mbm_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 +0 -0

@@ -2466,6 +2483,12 @@ static int _resctrl_abmc_enable(struct
 rdt_resource *r, bool enable)
   {
       struct rdt_mon_domain *d;

  +    /*
+     * Clear all the previous assignments while switching the monitor

Quoted 4 lines

+     * mode.
+     */
+    mbm_cntrs_init();
+

If the counters are managed by fs code then the arch code should not be doing this. If needed the fs code should init the counters before calling the arch helpers.

Quoted 24 lines

      /*
       * Hardware counters will reset after switching the monitor mode.
       * Reset the architectural state so that reading of hardware

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

@@ -2724,10 +2747,10 @@ static void schemata_list_destroy(void)
     static int rdt_get_tree(struct fs_context *fc)
   {
+    struct rdt_resource *r =
 &rdt_resources_all[RDT_RESOURCE_L3].r_resctrl;
       struct rdt_fs_context *ctx = rdt_fc2context(fc);
       unsigned long flags = RFTYPE_CTRL_BASE;
       struct rdt_mon_domain *dom;
-    struct rdt_resource *r;
       int ret;
         cpus_read_lock();

@@ -2756,6 +2779,9 @@ static int rdt_get_tree(struct fs_context *fc)
         closid_init();
   +    if (r->mon.abmc_capable)
+        mbm_cntrs_init();
+
       if (resctrl_arch_mon_capable())
           flags |= RFTYPE_MON;
   @@ -2800,7 +2826,6 @@ static int rdt_get_tree(struct fs_context *fc)
           resctrl_mounted = true;

Quoted 5 lines

        if (is_mbm_enabled()) {
-        r = &rdt_resources_all[RDT_RESOURCE_L3].r_resctrl; list_for_each_entry(dom, &r->mon_domains, hdr.list) mbm_setup_overflow_handler(dom, MBM_OVERFLOW_INTERVAL, RESCTRL_PICK_ANY_CPU);

Reinette

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

index 4f47f52e01c2..b3d3fa048f15 100644
@@ -185,6 +185,23 @@ bool closid_allocated(unsigned int closid)
        return !test_bit(closid, &closid_free_map);

Hi Babu,

Quoted 8 lines · Babu Moger

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

index 4f47f52e01c2..b3d3fa048f15 100644
@@ -185,6 +185,23 @@ bool closid_allocated(unsigned int closid)
         return !test_bit(closid, &closid_free_map);
  }
 

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

Quoted 8 lines

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

+/*
+ * Counter bitmap and its length for tracking available counters.
+ * 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 mbm_cntrs_free_map;
+static unsigned int mbm_cntrs_free_map_len;

If counter assignment is supported at a per-domain granularity, then counter id allocation needs to be done per-domain rather than globally. For example, if I free a counter from one group in a particular domain, it should be available to allocate to another group only in that domain.

When I attempt this using the current series, the resulting behavior is quite interesting. I noticed Reinette also commented on this later in the series, noticing that counters are only allocated permanently to groups and never move as a result of writing to mbm_control.

# grep 'g1[45]' info/L3_MON/mbm_control
test/g14/0=tl;1=tl;2=tl;3=tl;4=tl;5=tl;6=tl;7=tl;8=tl;9=tl;10=tl;11=tl;12=tl;13=tl;14=tl;15=tl;16=tl;17=tl;18=tl;19=tl;20=tl;21=tl;22=tl;23=tl;24=tl;25=tl;26=tl;27=tl;28=tl;29=tl;30=tl;31=tl;
test/g15/0=_;1=_;2=_;3=_;4=_;5=_;6=_;7=_;8=_;9=_;10=_;11=_;12=_;13=_;14=_;15=_;16=_;17=_;18=_;19=_;20=_;21=_;22=_;23=_;24=_;25=_;26=_;27=_;28=_;29=_;30=_;31=_;

[domains 2-31 omitted for clarity below]

# echo 'test/g14/1-t' > info/L3_MON/mbm_control
# grep 'g1[45]' info/L3_MON/mbm_control
test/g14/0=tl;1=l;
test/g15/0=_;1=_;

# echo "test/g15/1+t" > info/L3_MON/mbm_control
# grep 'g1[45]' info/L3_MON/mbm_control
test/g14/0=tl;1=_;
test/g15/0=_;1=_;

-Peter

Hi Babu,

Quoted 8 lines · Babu Moger

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

index 4f47f52e01c2..b3d3fa048f15 100644
@@ -185,6 +185,23 @@ bool closid_allocated(unsigned int closid)
         return !test_bit(closid, &closid_free_map);
  }
 

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

Quoted 8 lines

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

+/*
+ * Counter bitmap and its length for tracking available counters.
+ * 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 mbm_cntrs_free_map;
+static unsigned int mbm_cntrs_free_map_len;

If counter assignment is supported at a per-domain granularity, then counter id allocation needs to be done per-domain rather than globally. For example, if I free a counter from one group in a

particular domain, it should be available to allocate to another group only in that domain.

When I attempt this using the current series, the resulting behavior is quite interesting. I noticed Reinette also commented on this later in the series, noticing that counters are only allocated permanently to groups and never move as a result of writing to mbm_control.

# grep 'g1[45]' info/L3_MON/mbm_control
test/g14/0=tl;1=tl;2=tl;3=tl;4=tl;5=tl;6=tl;7=tl;8=tl;9=tl;10=tl;11=tl;12=tl;13=tl;14=tl;15=tl;16=tl;17=tl;18=tl;19=tl;20=tl;21=tl;22=tl;23=tl;24=tl;25=tl;26=tl;27=tl;28=tl;29=tl;30=tl;31=tl;
test/g15/0=_;1=_;2=_;3=_;4=_;5=_;6=_;7=_;8=_;9=_;10=_;11=_;12=_;13=_;14=_;15=_;16=_;17=_;18=_;19=_;20=_;21=_;22=_;23=_;24=_;25=_;26=_;27=_;28=_;29=_;30=_;31=_;

[domains 2-31 omitted for clarity below]

# echo 'test/g14/1-t' > info/L3_MON/mbm_control
# grep 'g1[45]' info/L3_MON/mbm_control
test/g14/0=tl;1=l;
test/g15/0=_;1=_;

# echo "test/g15/1+t" > info/L3_MON/mbm_control
# grep 'g1[45]' info/L3_MON/mbm_control
test/g14/0=tl;1=_;
test/g15/0=_;1=_;

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]>

---
v5: Exported mon_event_config_index_get.
Renamed arch_domain_mbm_evt_config to resctrl_arch_mbm_evt_config.

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 |  6 ++++++
  arch/x86/kernel/cpu/resctrl/monitor.c  | 22 ++++++++++++++++++++++
  arch/x86/kernel/cpu/resctrl/rdtgroup.c |  2 +-
  4 files changed, 31 insertions(+), 1 deletion(-)

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

index ff5cb693b396..6265ef8b610f 100644

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

  	if (arch_domain_mbm_alloc(r->mon.num_rmid, hw_dom)) {
  		mon_domain_free(hw_dom);
  		return;

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

index 0ce9797f80fe..4cb1a5d014a3 100644
@@ -401,6 +401,8 @@ struct rdt_hw_ctrl_domain {
   * @d_resctrl:	Properties exposed to the resctrl file system
   * @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.
   */

@@ -408,6 +410,8 @@ struct rdt_hw_mon_domain {
  	struct rdt_mon_domain		d_resctrl;
  	struct arch_mbm_state		*arch_mbm_total;
  	struct arch_mbm_state		*arch_mbm_local;
+	u32				mbm_total_cfg;
+	u32				mbm_local_cfg;
  };
 
  static inline struct rdt_hw_ctrl_domain *resctrl_to_arch_ctrl_dom(struct rdt_ctrl_domain *r)

@@ -662,6 +666,8 @@ void __check_limbo(struct rdt_mon_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 resctrl_arch_mbm_evt_config(struct rdt_hw_mon_domain *hw_dom);
+unsigned int mon_event_config_index_get(u32 evtid);
  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 7a93a6d2b2de..b96b0a8bd7d3 100644

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

+{
+	unsigned int index;
+	u64 msrval;
+
+	/*
+	 * Read the configuration registers QOS_EVT_CFG_n, where <n> is
+	 * the BMEC event number (EvtID).
+	 */
+	if (mbm_total_event.configurable) {
+		index = mon_event_config_index_get(QOS_L3_MBM_TOTAL_EVENT_ID);
+		rdmsrl(MSR_IA32_EVT_CFG_BASE + index, msrval);
+		hw_dom->mbm_total_cfg = msrval & MAX_EVT_CONFIG_BITS;
+	}
+
+	if (mbm_local_event.configurable) {
+		index = mon_event_config_index_get(QOS_L3_MBM_LOCAL_EVENT_ID);
+		rdmsrl(MSR_IA32_EVT_CFG_BASE + index, msrval);
+		hw_dom->mbm_local_cfg = msrval & MAX_EVT_CONFIG_BITS;
+	}
+}
+
  void __exit rdt_put_mon_l3_config(void)
  {
  	dom_data_exit();

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

index b3d3fa048f15..b2b751741dd8 100644

Hi Babu,

Quoted 8 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.

Quoted 2 lines

Read the event configuration from the hardware during the domain initialization. Save the configuration information in the rdt_hw_domain,

rdt_hw_domain -> rdt_hw_mon_domain

Quoted 22 lines

so it can be used for counter assignment.

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

---
v5: Exported mon_event_config_index_get.
     Renamed arch_domain_mbm_evt_config to resctrl_arch_mbm_evt_config.

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 |  6 ++++++ arch/x86/kernel/cpu/resctrl/monitor.c  | 22 ++++++++++++++++++++++ arch/x86/kernel/cpu/resctrl/rdtgroup.c |  2 +-

  4 files changed, 31 insertions(+), 1 deletion(-)

arch/x86/kernel/cpu/resctrl/core.c static void domain_add_cpu_mon(int cpu, struct · line 622

Quoted 10 lines

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

index ff5cb693b396..6265ef8b610f 100644
@@ -619,6 +619,8 @@ static void domain_add_cpu_mon(int cpu, struct
 rdt_resource *r)
         arch_mon_domain_online(r, d);
   +    resctrl_arch_mbm_evt_config(hw_dom);
+

This does not look to be an arch call called by the fs code so special naming does not seem to be required? If it _was_ an arch callback then

it cannot take a HW resource as parameter since the fs code does not have access to that.

Quoted 43 lines

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

       if (arch_domain_mbm_alloc(r->mon.num_rmid, hw_dom)) {
           mon_domain_free(hw_dom);
           return;

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

index 0ce9797f80fe..4cb1a5d014a3 100644
@@ -401,6 +401,8 @@ struct rdt_hw_ctrl_domain {
    * @d_resctrl:    Properties exposed to the resctrl file system
    * @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 +2 -0

@@ -408,6 +410,8 @@ struct rdt_hw_mon_domain {
       struct rdt_mon_domain        d_resctrl;
       struct arch_mbm_state        *arch_mbm_total;
       struct arch_mbm_state        *arch_mbm_local;
+    u32                mbm_total_cfg;
+    u32                mbm_local_cfg;
   };

    static inline struct rdt_hw_ctrl_domain *resctrl_to_arch_ctrl_dom(struct rdt_ctrl_domain *r)

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

@@ -662,6 +666,8 @@ void __check_limbo(struct rdt_mon_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 resctrl_arch_mbm_evt_config(struct rdt_hw_mon_domain *hw_dom);
+unsigned int mon_event_config_index_get(u32 evtid);
   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 7a93a6d2b2de..b96b0a8bd7d3 100644

arch/x86/kernel/cpu/resctrl/monitor.c int __init rdt_get_mon_l3_config(struct · line 1259

Quoted 4 lines

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

@@ -1256,6 +1256,28 @@ int __init rdt_get_mon_l3_config(struct
 rdt_resource *r)
       return 0;
   }
   +void resctrl_arch_mbm_evt_config(struct rdt_hw_mon_domain *hw_dom)

A function is expected to have a verb in its name and the verb here seems to be
"config", which does not seem appropriate and creates confusion with resctrl_arch_event_config_set(). How about resctrl_arch_mbm_evt_config_init() with proper initializer of the config values to also cover case when events are
not configurable (INVALID_CONFIG_VALUE introduced in next patch?) ?

Quoted 38 lines

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

+{
+    unsigned int index;
+    u64 msrval;
+
+    /*
+     * Read the configuration registers QOS_EVT_CFG_n, where <n> is
+     * the BMEC event number (EvtID).
+     */
+    if (mbm_total_event.configurable) {
+        index = mon_event_config_index_get(QOS_L3_MBM_TOTAL_EVENT_ID);
+        rdmsrl(MSR_IA32_EVT_CFG_BASE + index, msrval);
+        hw_dom->mbm_total_cfg = msrval & MAX_EVT_CONFIG_BITS;
+    }
+
+    if (mbm_local_event.configurable) {
+        index = mon_event_config_index_get(QOS_L3_MBM_LOCAL_EVENT_ID);
+        rdmsrl(MSR_IA32_EVT_CFG_BASE + index, msrval);
+        hw_dom->mbm_local_cfg = msrval & MAX_EVT_CONFIG_BITS;
+    }
+}
+
   void __exit rdt_put_mon_l3_config(void)
   {

      dom_data_exit();

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

index b3d3fa048f15..b2b751741dd8 100644
@@ -1606,7 +1606,7 @@ struct mon_config_info {
    *         1 for evtid == QOS_L3_MBM_LOCAL_EVENT_ID
    *         INVALID_CONFIG_INDEX for invalid evtid
    */
-static inline unsigned int mon_event_config_index_get(u32 evtid)
+unsigned int mon_event_config_index_get(u32 evtid)
   {
       switch (evtid) {

Quoted 1 line

      case QOS_L3_MBM_TOTAL_EVENT_ID:

Reinette

Quoted 23 lines · Reinette Chatre

Quoted 16 lines · Babu Moger

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

@@ -662,6 +666,8 @@ void __check_limbo(struct rdt_mon_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 resctrl_arch_mbm_evt_config(struct rdt_hw_mon_domain *hw_dom);
+unsigned int mon_event_config_index_get(u32 evtid);
   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 7a93a6d2b2de..b96b0a8bd7d3 100644

arch/x86/kernel/cpu/resctrl/monitor.c int __init rdt_get_mon_l3_config(struct · line 1259

Quoted 4 lines

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

@@ -1256,6 +1256,28 @@ int __init rdt_get_mon_l3_config(struct
 rdt_resource *r)
       return 0;
   }
   +void resctrl_arch_mbm_evt_config(struct rdt_hw_mon_domain *hw_dom)

Quoted 7 lines

A function is expected to have a verb in its name and the verb here seems to be
"config", which does not seem appropriate and creates confusion with resctrl_arch_event_config_set(). How about resctrl_arch_mbm_evt_config_init() with proper initializer of the config values to also cover case when events are
not configurable (INVALID_CONFIG_VALUE introduced in next patch?) ?

Sorry. I am not clear on this comment. Can you please elaborate?

Hi Babu,

Quoted 32 lines · Babu

Quoted 23 lines · Reinette Chatre

Quoted 16 lines · Babu Moger

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

@@ -662,6 +666,8 @@ void __check_limbo(struct rdt_mon_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 resctrl_arch_mbm_evt_config(struct rdt_hw_mon_domain *hw_dom);
+unsigned int mon_event_config_index_get(u32 evtid);
    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 7a93a6d2b2de..b96b0a8bd7d3 100644

arch/x86/kernel/cpu/resctrl/monitor.c int __init rdt_get_mon_l3_config(struct · line 1259

Quoted 4 lines

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

@@ -1256,6 +1256,28 @@ int __init rdt_get_mon_l3_config(struct
 rdt_resource *r)
        return 0;
    }
    +void resctrl_arch_mbm_evt_config(struct rdt_hw_mon_domain *hw_dom)

Quoted 7 lines

A function is expected to have a verb in its name and the verb here seems to be
"config", which does not seem appropriate and creates confusion with resctrl_arch_event_config_set(). How about resctrl_arch_mbm_evt_config_init() with proper initializer of the config values to also cover case when events are
not configurable (INVALID_CONFIG_VALUE introduced in next patch?) ?

Quoted 1 line

Sorry. I am not clear on this comment. Can you please elaborate?

This comment has two parts.

First, there is the naming of the function.
The name of the function should reflect what the function does and I believe that resctrl_arch_mbm_evt_config() is close enough to resctrl_arch_event_config_set() to cause confusion while also lacking an expected verb in the function name (since "config" should not be considered a verb here) . I proposed resctrl_arch_mbm_evt_config_init() as a new function name that has the "init" verb to indicate that this function "init"ializes the MBM config values.

Second, there is the work done by the function.
In this implementation the function initializes rdt_hw_mon_domain->mbm_total_cfg and rdt_hw_mon_domain->mbm_local_cfg when the events are configurable. I proposed that as an initializer the function can be expected to initialize rdt_hw_mon_domain->mbm_total_cfg and rdt_hw_mon_domain->mbm_local_cfg whether the events are configurable or not. In the latter case they can be initialized with INVALID_CONFIG_VALUE?

The event configuration is domain specific and initialized during domain initialization. It is not required to read the configuration register every time user asks for it. Use the value stored in rdt_mon_hw_domain

instead. Also update the configuration value when user writes it.

Introduce resctrl_arch_event_config_get() and resctrl_arch_event_config_set() to get/set architecture domain specific mbm_total_cfg/mbm_local_cfg values.

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

---
v5: Introduced resctrl_arch_event_config_get and
resctrl_arch_event_config_get() based on our discussion.
https://lore.kernel.org/lkml/[email protected]/

v4: New patch.

---
  arch/x86/kernel/cpu/resctrl/rdtgroup.c | 112 +++++++++++++++----------
  include/linux/resctrl.h                |   4 +
  2 files changed, 72 insertions(+), 44 deletions(-)

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

index b2b751741dd8..91c5d45ac367 100644

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

+
+unsigned int resctrl_arch_event_config_get(struct rdt_mon_domain *d,
+					   enum resctrl_event_id eventid)
+{
+	struct rdt_hw_mon_domain *hw_dom = resctrl_to_arch_mon_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 INVALID_CONFIG_VALUE;

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

+		pr_warn_once("Invalid event id %d\n", mon_info->evtid);
+		return;
+	}
+	wrmsr(MSR_IA32_EVT_CFG_BASE + index, mon_info->mon_config, 0);
+

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

+	}
+}
+
  #define INVALID_CONFIG_INDEX   UINT_MAX
 
  /**

@@ -1619,33 +1668,11 @@ unsigned int mon_event_config_index_get(u32 evtid)
  	}
  }
 
-static void mon_event_config_read(void *info)
-{
-	struct mon_config_info *mon_info = info;
-	unsigned int index;
-	u64 msrval;
-
-	index = mon_event_config_index_get(mon_info->evtid);
-	if (index == INVALID_CONFIG_INDEX) {
-		pr_warn_once("Invalid event id %d\n", mon_info->evtid);
-		return;
-	}
-	rdmsrl(MSR_IA32_EVT_CFG_BASE + index, msrval);
-
-	/* Report only the valid event configuration bits */
-	mon_info->mon_config = msrval & MAX_EVT_CONFIG_BITS;
-}
-
-static void mondata_config_read(struct rdt_mon_domain *d, struct mon_config_info *mon_info)
-{
-	smp_call_function_any(&d->hdr.cpu_mask, mon_event_config_read, mon_info, 1);
-}
-
  static int mbm_config_show(struct seq_file *s, struct rdt_resource *r, u32 evtid)
  {
-	struct mon_config_info mon_info = {0};
  	struct rdt_mon_domain *dom;
  	bool sep = false;
+	int val;
 
  	cpus_read_lock();
  	mutex_lock(&rdtgroup_mutex);

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

+			break;
+		}
 
-		seq_printf(s, "%d=0x%02x", dom->hdr.id, mon_info.mon_config);
+		seq_printf(s, "%d=0x%02x", dom->hdr.id, val);
  		sep = true;
  	}
  	seq_puts(s, "\n");

@@ -1689,33 +1718,27 @@ static int mbm_local_bytes_config_show(struct kernfs_open_file *of,
  	return 0;
  }
 
-static void mon_event_config_write(void *info)
-{
-	struct mon_config_info *mon_info = info;
-	unsigned int index;
-
-	index = mon_event_config_index_get(mon_info->evtid);
-	if (index == INVALID_CONFIG_INDEX) {
-		pr_warn_once("Invalid event id %d\n", mon_info->evtid);
-		return;
-	}
-	wrmsr(MSR_IA32_EVT_CFG_BASE + index, mon_info->mon_config, 0);
-}
 
  static void mbm_config_write_domain(struct rdt_resource *r,
  				    struct rdt_mon_domain *d, u32 evtid, u32 val)
  {
  	struct mon_config_info mon_info = {0};
+	int config_val;
 
  	/*