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 <babu.moger@amd.com>
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(+)
diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
index 01342963011e..263b2d9d00ed 100644
--- a/arch/x86/include/asm/msr-index.h
+++ b/arch/x86/include/asm/msr-index.h
@@ -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)
diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
index 2bd207624eec..0ce9797f80fe 100644
--- a/arch/x86/kernel/cpu/resctrl/internal.h
+++ b/arch/x86/kernel/cpu/resctrl/internal.h
@@ -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;
@@ -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;
+ bool abmc_enabled;
};
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.
diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index 7e76f8d839fc..471fc0dbd7c3 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -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;
+
+ rdmsrl(MSR_IA32_L3_QOS_EXT_CFG, msrval);
+
+ if (*enable)
+ msrval |= ABMC_ENABLE;
+ else
+ msrval &= ~ABMC_ENABLE;
+
+ wrmsrl(MSR_IA32_L3_QOS_EXT_CFG, msrval);
+}
+
+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,
+ resctrl_abmc_set_one_amd, &enable, 1);
+ resctrl_arch_reset_rmid_all(r, d);
+ }
+
+ return 0;
+}
+
+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);
+ if (!ret)
+ hw_res->abmc_enabled = true;
+ }
+
+ return ret;
+}
+
+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);
+
+ if (hw_res->abmc_enabled) {
+ _resctrl_abmc_enable(r, false);
+ hw_res->abmc_enabled = false;
+ }
+}
+
/*
* We don't allow rdtgroup directories to be created anywhere
* except the root directory. Thus when looking for the rdtgroup
--
2.34.1Hi Babu,
On 7/3/24 2:48 PM, Babu Moger wrote:
> 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 <babu.moger@amd.com>
> 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(+)
>
> diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
> index 01342963011e..263b2d9d00ed 100644
> --- a/arch/x86/include/asm/msr-index.h
> +++ b/arch/x86/include/asm/msr-index.h
> @@ -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)
> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
> index 2bd207624eec..0ce9797f80fe 100644
> --- a/arch/x86/kernel/cpu/resctrl/internal.h
> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
> @@ -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.
> +#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;
> + 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.
>
> 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.
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index 7e76f8d839fc..471fc0dbd7c3 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -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;
> +
> + rdmsrl(MSR_IA32_L3_QOS_EXT_CFG, msrval);
> +
> + if (*enable)
> + msrval |= ABMC_ENABLE;
> + else
> + msrval &= ~ABMC_ENABLE;
> +
> + wrmsrl(MSR_IA32_L3_QOS_EXT_CFG, msrval);
> +}
msr_set_bit() and msr_clear_bit() can be used here.
> +
> +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,
> + resctrl_abmc_set_one_amd, &enable, 1);
> + resctrl_arch_reset_rmid_all(r, d);
> + }
> +
> + return 0;
> +}
Seems like _resctrl_abmc_enable() can just return void.
> +
> +int resctrl_arch_abmc_enable(void)
resctrl_arch_mbm_cntr_enable()? I'll no longer point all these out.
> +{
> + 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);
> + if (!ret)
> + hw_res->abmc_enabled = true;
The above error handling seems unnecessary.
> + }
> +
> + 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.
> +}
> +
> +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);
> +
> + if (hw_res->abmc_enabled) {
> + _resctrl_abmc_enable(r, false);
> + hw_res->abmc_enabled = false;
> + }
> +}
> +
> /*
> * We don't allow rdtgroup directories to be created anywhere
> * except the root directory. Thus when looking for the rdtgroup
Reinette
Hello!
On 12/07/2024 23:05, Reinette Chatre wrote:
> On 7/3/24 2:48 PM, Babu Moger wrote:
>> 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).
>> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> index 7e76f8d839fc..471fc0dbd7c3 100644
>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> @@ -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);
>> + 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.
Just as a datapoint on this: arm64 does indeed need to be able to return an error here.
This helper gets used to allocate all the monitors (and an array to hold them) which can fail.
Thanks,
James
Hi Reinette,
On 7/12/24 17:05, Reinette Chatre wrote:
> Hi Babu,
>
> On 7/3/24 2:48 PM, Babu Moger wrote:
>> 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 <babu.moger@amd.com>
>> 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(+)
>>
>> diff --git a/arch/x86/include/asm/msr-index.h
>> b/arch/x86/include/asm/msr-index.h
>> index 01342963011e..263b2d9d00ed 100644
>> --- a/arch/x86/include/asm/msr-index.h
>> +++ b/arch/x86/include/asm/msr-index.h
>> @@ -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)
>> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h
>> b/arch/x86/kernel/cpu/resctrl/internal.h
>> index 2bd207624eec..0ce9797f80fe 100644
>> --- a/arch/x86/kernel/cpu/resctrl/internal.h
>> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
>> @@ -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.
>
>> +#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;
>> + 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" ?
>
>> 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.
>> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> index 7e76f8d839fc..471fc0dbd7c3 100644
>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> @@ -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;
>> +
>> + rdmsrl(MSR_IA32_L3_QOS_EXT_CFG, msrval);
>> +
>> + if (*enable)
>> + msrval |= ABMC_ENABLE;
>> + else
>> + msrval &= ~ABMC_ENABLE;
>> +
>> + wrmsrl(MSR_IA32_L3_QOS_EXT_CFG, msrval);
>> +}
>
> msr_set_bit() and msr_clear_bit() can be used here.
Sure.
>
>> +
>> +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,
>> + resctrl_abmc_set_one_amd, &enable, 1);
>> + resctrl_arch_reset_rmid_all(r, d);
>> + }
>> +
>> + return 0;
>> +}
>
> Seems like _resctrl_abmc_enable() can just return void.
Sure.
>
>> +
>> +int resctrl_arch_abmc_enable(void)
>
> resctrl_arch_mbm_cntr_enable()? I'll no longer point all these out.
Sure.
>
>> +{
>> + 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);
>> + if (!ret)
>> + hw_res->abmc_enabled = true;
>
> The above error handling seems unnecessary.
Sure.
>
>> + }
>> +
>> + 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.
Yea. Sure.
>
>> +}
>> +
>> +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);
>> +
>> + if (hw_res->abmc_enabled) {
>> + _resctrl_abmc_enable(r, false);
>> + hw_res->abmc_enabled = false;
>> + }
>> +}
>> +
>> /*
>> * We don't allow rdtgroup directories to be created anywhere
>> * except the root directory. Thus when looking for the rdtgroup
>
> Reinette
>
--
Thanks
Babu Moger
Hi Babu,
On 7/16/24 8:13 AM, Moger, Babu wrote:
> On 7/12/24 17:05, Reinette Chatre wrote:
>> On 7/3/24 2:48 PM, Babu Moger wrote:
>>> 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 <babu.moger@amd.com>
>>> 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(+)
>>>
>>> diff --git a/arch/x86/include/asm/msr-index.h
>>> b/arch/x86/include/asm/msr-index.h
>>> index 01342963011e..263b2d9d00ed 100644
>>> --- a/arch/x86/include/asm/msr-index.h
>>> +++ b/arch/x86/include/asm/msr-index.h
>>> @@ -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)
>>> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h
>>> b/arch/x86/kernel/cpu/resctrl/internal.h
>>> index 2bd207624eec..0ce9797f80fe 100644
>>> --- a/arch/x86/kernel/cpu/resctrl/internal.h
>>> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
>>> @@ -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.
>
>>
>>> +#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;
>>> + 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" ?
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.
Reinette
Hi Reinette,
On 7/16/24 12:51, Reinette Chatre wrote:
> Hi Babu,
>
> On 7/16/24 8:13 AM, Moger, Babu wrote:
>> On 7/12/24 17:05, Reinette Chatre wrote:
>>> On 7/3/24 2:48 PM, Babu Moger wrote:
>>>> 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 <babu.moger@amd.com>
>>>> 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(+)
>>>>
>>>> diff --git a/arch/x86/include/asm/msr-index.h
>>>> b/arch/x86/include/asm/msr-index.h
>>>> index 01342963011e..263b2d9d00ed 100644
>>>> --- a/arch/x86/include/asm/msr-index.h
>>>> +++ b/arch/x86/include/asm/msr-index.h
>>>> @@ -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)
>>>> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h
>>>> b/arch/x86/kernel/cpu/resctrl/internal.h
>>>> index 2bd207624eec..0ce9797f80fe 100644
>>>> --- a/arch/x86/kernel/cpu/resctrl/internal.h
>>>> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
>>>> @@ -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.
>>
>>>
>>>> +#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;
>>>> + 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" ?
>
> 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.
>
> 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.
Thanks for the heads up.
Thanks
Babu Moger
Hi Babu,
On 7/16/24 11:48 AM, Moger, Babu wrote:
> On 7/16/24 12:51, Reinette Chatre wrote:
>> On 7/16/24 8:13 AM, Moger, Babu wrote:
>>> On 7/12/24 17:05, Reinette Chatre wrote:
>>>> On 7/3/24 2:48 PM, Babu Moger wrote:
>>>>> @@ -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" ?
>>
>> 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.
ok. It is not clear to me how this will turn out. I'm afraid the length may
start to be cumbersome, but we can see how it turns out.
Reinette
© 2016 - 2026 Red Hat, Inc.