Add the functionality to enable/disable ABMC feature.
ABMC is enabled by setting enabled bit(0) in MSR L3_QOS_EXT_CFG. When the
state of ABMC is changed, it must be changed to the updated value on all
logical processors in the QOS Domain.
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
---
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 | 12 ++++
arch/x86/kernel/cpu/resctrl/rdtgroup.c | 76 +++++++++++++++++++++++++-
3 files changed, 88 insertions(+), 1 deletion(-)
diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
index 05956bd8bacf..f16ee50b1a23 100644
--- a/arch/x86/include/asm/msr-index.h
+++ b/arch/x86/include/asm/msr-index.h
@@ -1165,6 +1165,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 722388621403..8238ee437369 100644
--- a/arch/x86/kernel/cpu/resctrl/internal.h
+++ b/arch/x86/kernel/cpu/resctrl/internal.h
@@ -96,6 +96,9 @@ cpumask_any_housekeeping(const struct cpumask *mask, int exclude_cpu)
return cpu;
}
+/* ABMC ENABLE */
+#define ABMC_ENABLE BIT(0)
+
struct rdt_fs_context {
struct kernfs_fs_context kfc;
bool enable_cdpl2;
@@ -433,6 +436,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.
@@ -448,6 +452,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)
@@ -491,6 +496,13 @@ static inline bool resctrl_arch_get_cdp_enabled(enum resctrl_res_level l)
int resctrl_arch_set_cdp_enabled(enum resctrl_res_level l, bool enable);
+static inline bool resctrl_arch_get_abmc_enabled(enum resctrl_res_level l)
+{
+ return rdt_resources_all[l].abmc_enabled;
+}
+
+int resctrl_arch_set_abmc_enabled(enum resctrl_res_level l, bool enable);
+
/*
* To return the common struct rdt_resource, which is contained in struct
* rdt_hw_resource, walk the resctrl member of struct rdt_hw_resource.
diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index 05f551bc316e..f49073c86884 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -850,9 +850,15 @@ static int rdtgroup_mbm_assign_show(struct kernfs_open_file *of,
struct seq_file *s, void *v)
{
struct rdt_resource *r = of->kn->parent->priv;
+ struct rdt_hw_resource *hw_res = resctrl_to_arch_res(r);
- if (r->mbm_assign_capable)
+ if (r->mbm_assign_capable && hw_res->abmc_enabled) {
+ seq_puts(s, "[abmc]\n");
+ seq_puts(s, "legacy_mbm\n");
+ } else if (r->mbm_assign_capable) {
seq_puts(s, "abmc\n");
+ seq_puts(s, "[legacy_mbm]\n");
+ }
return 0;
}
@@ -2433,6 +2439,74 @@ int resctrl_arch_set_cdp_enabled(enum resctrl_res_level l, bool enable)
return 0;
}
+static void resctrl_abmc_msrwrite(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_setup(enum resctrl_res_level l, bool enable)
+{
+ struct rdt_resource *r = &rdt_resources_all[l].r_resctrl;
+ struct rdt_domain *d;
+
+ /* Update QOS_CFG MSR on all the CPUs in cpu_mask */
+ list_for_each_entry(d, &r->domains, list) {
+ on_each_cpu_mask(&d->cpu_mask, resctrl_abmc_msrwrite, &enable, 1);
+ resctrl_arch_reset_rmid_all(r, d);
+ }
+
+ return 0;
+}
+
+static int resctrl_abmc_enable(enum resctrl_res_level l)
+{
+ struct rdt_hw_resource *hw_res = &rdt_resources_all[l];
+ int ret = 0;
+
+ if (!hw_res->abmc_enabled) {
+ ret = resctrl_abmc_setup(l, true);
+ if (!ret)
+ hw_res->abmc_enabled = true;
+ }
+
+ return ret;
+}
+
+static void resctrl_abmc_disable(enum resctrl_res_level l)
+{
+ struct rdt_hw_resource *hw_res = &rdt_resources_all[l];
+
+ if (hw_res->abmc_enabled) {
+ resctrl_abmc_setup(l, false);
+ hw_res->abmc_enabled = false;
+ }
+}
+
+int resctrl_arch_set_abmc_enabled(enum resctrl_res_level l, bool enable)
+{
+ struct rdt_hw_resource *hw_res = &rdt_resources_all[l];
+
+ if (!hw_res->r_resctrl.mbm_assign_capable)
+ return -EINVAL;
+
+ if (enable)
+ return resctrl_abmc_enable(l);
+
+ resctrl_abmc_disable(l);
+
+ return 0;
+}
+
/*
* 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 3/28/2024 6:06 PM, Babu Moger wrote:
> Add the functionality to enable/disable ABMC feature.
>
> ABMC is enabled by setting enabled bit(0) in MSR L3_QOS_EXT_CFG. When the
> state of ABMC is changed, it must be changed to the updated value on all
> logical processors in the QOS Domain.
This patch does much more than enable what is mentioned above. There is little
information about what this patch aims to accomplish. Without this it makes
review difficult.
>
> 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
> ---
> 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 | 12 ++++
> arch/x86/kernel/cpu/resctrl/rdtgroup.c | 76 +++++++++++++++++++++++++-
> 3 files changed, 88 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
> index 05956bd8bacf..f16ee50b1a23 100644
> --- a/arch/x86/include/asm/msr-index.h
> +++ b/arch/x86/include/asm/msr-index.h
> @@ -1165,6 +1165,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 722388621403..8238ee437369 100644
> --- a/arch/x86/kernel/cpu/resctrl/internal.h
> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
> @@ -96,6 +96,9 @@ cpumask_any_housekeeping(const struct cpumask *mask, int exclude_cpu)
> return cpu;
> }
>
> +/* ABMC ENABLE */
Can this comment be made more useful?
> +#define ABMC_ENABLE BIT(0)
> +
> struct rdt_fs_context {
> struct kernfs_fs_context kfc;
> bool enable_cdpl2;
> @@ -433,6 +436,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.
> @@ -448,6 +452,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)
> @@ -491,6 +496,13 @@ static inline bool resctrl_arch_get_cdp_enabled(enum resctrl_res_level l)
>
> int resctrl_arch_set_cdp_enabled(enum resctrl_res_level l, bool enable);
>
> +static inline bool resctrl_arch_get_abmc_enabled(enum resctrl_res_level l)
> +{
> + return rdt_resources_all[l].abmc_enabled;
> +}
> +
> +int resctrl_arch_set_abmc_enabled(enum resctrl_res_level l, bool enable);
> +
> /*
> * To return the common struct rdt_resource, which is contained in struct
> * rdt_hw_resource, walk the resctrl member of struct rdt_hw_resource.
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index 05f551bc316e..f49073c86884 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -850,9 +850,15 @@ static int rdtgroup_mbm_assign_show(struct kernfs_open_file *of,
> struct seq_file *s, void *v)
> {
> struct rdt_resource *r = of->kn->parent->priv;
> + struct rdt_hw_resource *hw_res = resctrl_to_arch_res(r);
>
> - if (r->mbm_assign_capable)
> + if (r->mbm_assign_capable && hw_res->abmc_enabled) {
> + seq_puts(s, "[abmc]\n");
> + seq_puts(s, "legacy_mbm\n");
> + } else if (r->mbm_assign_capable) {
> seq_puts(s, "abmc\n");
> + seq_puts(s, "[legacy_mbm]\n");
> + }
>
> return 0;
> }
> @@ -2433,6 +2439,74 @@ int resctrl_arch_set_cdp_enabled(enum resctrl_res_level l, bool enable)
> return 0;
> }
>
> +static void resctrl_abmc_msrwrite(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_setup(enum resctrl_res_level l, bool enable)
> +{
> + struct rdt_resource *r = &rdt_resources_all[l].r_resctrl;
> + struct rdt_domain *d;
> +
> + /* Update QOS_CFG MSR on all the CPUs in cpu_mask */
"all the CPUs in cpu_mask" -> "all the CPUs associated with the resource"?
> + list_for_each_entry(d, &r->domains, list) {
> + on_each_cpu_mask(&d->cpu_mask, resctrl_abmc_msrwrite, &enable, 1);
> + resctrl_arch_reset_rmid_all(r, d);
Could the changelog please explain why this is needed and what the impact of
this is?
> + }
> +
> + return 0;
> +}
I think the naming can be changed to make these easier to understand. For example,
resctrl_abmc_msrwrite() -> resctrl_abmc_set_one()
resctrl_abmc_setup() -> resctrl_abmc_set_all()
> +
> +static int resctrl_abmc_enable(enum resctrl_res_level l)
> +{
> + struct rdt_hw_resource *hw_res = &rdt_resources_all[l];
> + int ret = 0;
> +
> + if (!hw_res->abmc_enabled) {
> + ret = resctrl_abmc_setup(l, true);
> + if (!ret)
> + hw_res->abmc_enabled = true;
> + }
> +
> + return ret;
> +}
> +
> +static void resctrl_abmc_disable(enum resctrl_res_level l)
> +{
> + struct rdt_hw_resource *hw_res = &rdt_resources_all[l];
> +
> + if (hw_res->abmc_enabled) {
> + resctrl_abmc_setup(l, false);
> + hw_res->abmc_enabled = false;
> + }
> +}
> +
> +int resctrl_arch_set_abmc_enabled(enum resctrl_res_level l, bool enable)
> +{
> + struct rdt_hw_resource *hw_res = &rdt_resources_all[l];
> +
> + if (!hw_res->r_resctrl.mbm_assign_capable)
> + return -EINVAL;
> +
> + if (enable)
> + return resctrl_abmc_enable(l);
> +
> + resctrl_abmc_disable(l);
> +
> + return 0;
> +}
Why is resctrl_arch_set_abmc_enabled() necessary? It seem to add an unnecessary
layer of abstraction.
> +
> /*
> * We don't allow rdtgroup directories to be created anywhere
> * except the root directory. Thus when looking for the rdtgroup
Reinette
Hi Reinette,
On 5/3/24 18:30, Reinette Chatre wrote:
> Hi Babu,
>
> On 3/28/2024 6:06 PM, Babu Moger wrote:
>> Add the functionality to enable/disable ABMC feature.
>>
>> ABMC is enabled by setting enabled bit(0) in MSR L3_QOS_EXT_CFG. When the
>> state of ABMC is changed, it must be changed to the updated value on all
>> logical processors in the QOS Domain.
>
> This patch does much more than enable what is mentioned above. There is little
> information about what this patch aims to accomplish. Without this it makes
> review difficult.
Sure. Also I need to add details about why resctrl_arch_reset_rmid_all()
is required. Will do.
>
>>
>> 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
>> ---
>> 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 | 12 ++++
>> arch/x86/kernel/cpu/resctrl/rdtgroup.c | 76 +++++++++++++++++++++++++-
>> 3 files changed, 88 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
>> index 05956bd8bacf..f16ee50b1a23 100644
>> --- a/arch/x86/include/asm/msr-index.h
>> +++ b/arch/x86/include/asm/msr-index.h
>> @@ -1165,6 +1165,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 722388621403..8238ee437369 100644
>> --- a/arch/x86/kernel/cpu/resctrl/internal.h
>> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
>> @@ -96,6 +96,9 @@ cpumask_any_housekeeping(const struct cpumask *mask, int exclude_cpu)
>> return cpu;
>> }
>>
>> +/* ABMC ENABLE */
>
> Can this comment be made more useful?
How about?
/* Setting bit 0 in L3_QOS_EXT_CFG enables ABMC features */
Or I can remove it totally.
>
>> +#define ABMC_ENABLE BIT(0)
>> +
>> struct rdt_fs_context {
>> struct kernfs_fs_context kfc;
>> bool enable_cdpl2;
>> @@ -433,6 +436,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.
>> @@ -448,6 +452,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)
>> @@ -491,6 +496,13 @@ static inline bool resctrl_arch_get_cdp_enabled(enum resctrl_res_level l)
>>
>> int resctrl_arch_set_cdp_enabled(enum resctrl_res_level l, bool enable);
>>
>> +static inline bool resctrl_arch_get_abmc_enabled(enum resctrl_res_level l)
>> +{
>> + return rdt_resources_all[l].abmc_enabled;
>> +}
>> +
>> +int resctrl_arch_set_abmc_enabled(enum resctrl_res_level l, bool enable);
>> +
>> /*
>> * To return the common struct rdt_resource, which is contained in struct
>> * rdt_hw_resource, walk the resctrl member of struct rdt_hw_resource.
>> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> index 05f551bc316e..f49073c86884 100644
>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> @@ -850,9 +850,15 @@ static int rdtgroup_mbm_assign_show(struct kernfs_open_file *of,
>> struct seq_file *s, void *v)
>> {
>> struct rdt_resource *r = of->kn->parent->priv;
>> + struct rdt_hw_resource *hw_res = resctrl_to_arch_res(r);
>>
>> - if (r->mbm_assign_capable)
>> + if (r->mbm_assign_capable && hw_res->abmc_enabled) {
>> + seq_puts(s, "[abmc]\n");
>> + seq_puts(s, "legacy_mbm\n");
>> + } else if (r->mbm_assign_capable) {
>> seq_puts(s, "abmc\n");
>> + seq_puts(s, "[legacy_mbm]\n");
>> + }
>>
>> return 0;
>> }
>> @@ -2433,6 +2439,74 @@ int resctrl_arch_set_cdp_enabled(enum resctrl_res_level l, bool enable)
>> return 0;
>> }
>>
>> +static void resctrl_abmc_msrwrite(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_setup(enum resctrl_res_level l, bool enable)
>> +{
>> + struct rdt_resource *r = &rdt_resources_all[l].r_resctrl;
>> + struct rdt_domain *d;
>> +
>> + /* Update QOS_CFG MSR on all the CPUs in cpu_mask */
>
> "all the CPUs in cpu_mask" -> "all the CPUs associated with the resource"?
Sure.
>
>> + list_for_each_entry(d, &r->domains, list) {
>> + on_each_cpu_mask(&d->cpu_mask, resctrl_abmc_msrwrite, &enable, 1);
>> + resctrl_arch_reset_rmid_all(r, d);
>
> Could the changelog please explain why this is needed and what the impact of
> this is?
Sure.
>
>> + }
>> +
>> + return 0;
>> +}
>
> I think the naming can be changed to make these easier to understand. For example,
> resctrl_abmc_msrwrite() -> resctrl_abmc_set_one()
> resctrl_abmc_setup() -> resctrl_abmc_set_all()
Sure.
>
>> +
>> +static int resctrl_abmc_enable(enum resctrl_res_level l)
>> +{
>> + struct rdt_hw_resource *hw_res = &rdt_resources_all[l];
>> + int ret = 0;
>> +
>> + if (!hw_res->abmc_enabled) {
>> + ret = resctrl_abmc_setup(l, true);
>> + if (!ret)
>> + hw_res->abmc_enabled = true;
>> + }
>> +
>> + return ret;
>> +}
>> +
>> +static void resctrl_abmc_disable(enum resctrl_res_level l)
>> +{
>> + struct rdt_hw_resource *hw_res = &rdt_resources_all[l];
>> +
>> + if (hw_res->abmc_enabled) {
>> + resctrl_abmc_setup(l, false);
>> + hw_res->abmc_enabled = false;
>> + }
>> +}
>> +
>> +int resctrl_arch_set_abmc_enabled(enum resctrl_res_level l, bool enable)
>> +{
>> + struct rdt_hw_resource *hw_res = &rdt_resources_all[l];
>> +
>> + if (!hw_res->r_resctrl.mbm_assign_capable)
>> + return -EINVAL;
>> +
>> + if (enable)
>> + return resctrl_abmc_enable(l);
>> +
>> + resctrl_abmc_disable(l);
>> +
>> + return 0;
>> +}
>
> Why is resctrl_arch_set_abmc_enabled() necessary? It seem to add an unnecessary
> layer of abstraction.
>
I feel it is better to keep it that way. It is consistent with definition
of resctrl_arch_set_cdp_enabled. It handles both enable and disable.
Otherwise we have add those checks from the caller.
--
Thanks
Babu Moger
Hi Babu,
On 5/7/2024 12:12 PM, Moger, Babu wrote:
> On 5/3/24 18:30, Reinette Chatre wrote:
>> On 3/28/2024 6:06 PM, Babu Moger wrote:
...
>>> /* 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 722388621403..8238ee437369 100644
>>> --- a/arch/x86/kernel/cpu/resctrl/internal.h
>>> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
>>> @@ -96,6 +96,9 @@ cpumask_any_housekeeping(const struct cpumask *mask, int exclude_cpu)
>>> return cpu;
>>> }
>>>
>>> +/* ABMC ENABLE */
>>
>> Can this comment be made more useful?
>
> How about?
> /* Setting bit 0 in L3_QOS_EXT_CFG enables ABMC features */
Regarding "ABMC features" - are there several features connected to
"ABMC"?
>
> Or I can remove it totally.
>
>>
...
>>> +int resctrl_arch_set_abmc_enabled(enum resctrl_res_level l, bool enable)
>>> +{
>>> + struct rdt_hw_resource *hw_res = &rdt_resources_all[l];
>>> +
>>> + if (!hw_res->r_resctrl.mbm_assign_capable)
>>> + return -EINVAL;
>>> +
>>> + if (enable)
>>> + return resctrl_abmc_enable(l);
>>> +
>>> + resctrl_abmc_disable(l);
>>> +
>>> + return 0;
>>> +}
>>
>> Why is resctrl_arch_set_abmc_enabled() necessary? It seem to add an unnecessary
>> layer of abstraction.
>>
>
> I feel it is better to keep it that way. It is consistent with definition
> of resctrl_arch_set_cdp_enabled. It handles both enable and disable.
> Otherwise we have add those checks from the caller.
Caller needs to know anyway whether to provide "true" or "false" to this function ...
caller may as well just call the appropriate _enable()/_disable() variant, no?
Reinette
Hi Reinette,
On 5/7/24 15:32, Reinette Chatre wrote:
> Hi Babu,
>
> On 5/7/2024 12:12 PM, Moger, Babu wrote:
>> On 5/3/24 18:30, Reinette Chatre wrote:
>>> On 3/28/2024 6:06 PM, Babu Moger wrote:
>
> ...
>
>>>> /* 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 722388621403..8238ee437369 100644
>>>> --- a/arch/x86/kernel/cpu/resctrl/internal.h
>>>> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
>>>> @@ -96,6 +96,9 @@ cpumask_any_housekeeping(const struct cpumask *mask, int exclude_cpu)
>>>> return cpu;
>>>> }
>>>>
>>>> +/* ABMC ENABLE */
>>>
>>> Can this comment be made more useful?
>>
>> How about?
>> /* Setting bit 0 in L3_QOS_EXT_CFG enables ABMC features */
>
> Regarding "ABMC features" - are there several features connected to
> "ABMC"?
>
No. It should have been "ABMC feature". Will correct it.
>>
>> Or I can remove it totally.
>>
>>>
>
> ...
>
>>>> +int resctrl_arch_set_abmc_enabled(enum resctrl_res_level l, bool enable)
>>>> +{
>>>> + struct rdt_hw_resource *hw_res = &rdt_resources_all[l];
>>>> +
>>>> + if (!hw_res->r_resctrl.mbm_assign_capable)
>>>> + return -EINVAL;
>>>> +
>>>> + if (enable)
>>>> + return resctrl_abmc_enable(l);
>>>> +
>>>> + resctrl_abmc_disable(l);
>>>> +
>>>> + return 0;
>>>> +}
>>>
>>> Why is resctrl_arch_set_abmc_enabled() necessary? It seem to add an unnecessary
>>> layer of abstraction.
>>>
>>
>> I feel it is better to keep it that way. It is consistent with definition
>> of resctrl_arch_set_cdp_enabled. It handles both enable and disable.
>> Otherwise we have add those checks from the caller.
>
> Caller needs to know anyway whether to provide "true" or "false" to this function ...
> caller may as well just call the appropriate _enable()/_disable() variant, no?
Yes. We can call resctrl_abmc_enable() and resctrl_abmc_disable() directly.
Thanks
Babu Moger
Hi Babu,
On Thu, Mar 28, 2024 at 6:07 PM Babu Moger <babu.moger@amd.com> wrote:
> struct rdt_fs_context {
> struct kernfs_fs_context kfc;
> bool enable_cdpl2;
> @@ -433,6 +436,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.
> @@ -448,6 +452,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)
> @@ -491,6 +496,13 @@ static inline bool resctrl_arch_get_cdp_enabled(enum resctrl_res_level l)
>
> int resctrl_arch_set_cdp_enabled(enum resctrl_res_level l, bool enable);
>
> +static inline bool resctrl_arch_get_abmc_enabled(enum resctrl_res_level l)
> +{
> + return rdt_resources_all[l].abmc_enabled;
> +}
This inline definition will not work in either this file or
fs/resctrl/internal.h, following James's change[1] moving the code.
resctrl_arch-definitions are either declared in linux/resctrl.h or
defined inline in a file like asm/resctrl.h.
> +
> +int resctrl_arch_set_abmc_enabled(enum resctrl_res_level l, bool enable);
> +
> /*
> * To return the common struct rdt_resource, which is contained in struct
> * rdt_hw_resource, walk the resctrl member of struct rdt_hw_resource.
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index 05f551bc316e..f49073c86884 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -850,9 +850,15 @@ static int rdtgroup_mbm_assign_show(struct kernfs_open_file *of,
> struct seq_file *s, void *v)
> {
> struct rdt_resource *r = of->kn->parent->priv;
> + struct rdt_hw_resource *hw_res = resctrl_to_arch_res(r);
>
> - if (r->mbm_assign_capable)
> + if (r->mbm_assign_capable && hw_res->abmc_enabled) {
> + seq_puts(s, "[abmc]\n");
> + seq_puts(s, "legacy_mbm\n");
> + } else if (r->mbm_assign_capable) {
> seq_puts(s, "abmc\n");
> + seq_puts(s, "[legacy_mbm]\n");
> + }
This looks like it would move to fs/resctrl/rdtgroup.c where it's not
possible to dereference an rdt_hw_resource struct.
It might be helpful to try building your changes on top of James's
change[1] to get an idea of how this would fit in post-refactoring.
I'll stop pointing out inconsistencies with his portability scheme
now.
>
> return 0;
> }
> @@ -2433,6 +2439,74 @@ int resctrl_arch_set_cdp_enabled(enum resctrl_res_level l, bool enable)
> return 0;
> }
>
> +static void resctrl_abmc_msrwrite(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_setup(enum resctrl_res_level l, bool enable)
> +{
> + struct rdt_resource *r = &rdt_resources_all[l].r_resctrl;
> + struct rdt_domain *d;
> +
> + /* Update QOS_CFG MSR on all the CPUs in cpu_mask */
> + list_for_each_entry(d, &r->domains, list) {
> + on_each_cpu_mask(&d->cpu_mask, resctrl_abmc_msrwrite, &enable, 1);
> + resctrl_arch_reset_rmid_all(r, d);
> + }
> +
> + return 0;
> +}
> +
> +static int resctrl_abmc_enable(enum resctrl_res_level l)
> +{
> + struct rdt_hw_resource *hw_res = &rdt_resources_all[l];
> + int ret = 0;
> +
> + if (!hw_res->abmc_enabled) {
> + ret = resctrl_abmc_setup(l, true);
> + if (!ret)
> + hw_res->abmc_enabled = true;
Presumably this would be called holding the rdtgroup_mutex? Perhaps a
lockdep assertion somewhere would be appropriate?
Thanks!
-Peter
[1] https://lore.kernel.org/lkml/20240321165106.31602-32-james.morse@arm.com/
Hi Peter,
While working on v4, found few things. Just wanted you to know (mostly FYI.).
On 4/3/24 19:30, Peter Newman wrote:
> Hi Babu,
>
> On Thu, Mar 28, 2024 at 6:07 PM Babu Moger <babu.moger@amd.com> wrote:
>> struct rdt_fs_context {
>> struct kernfs_fs_context kfc;
>> bool enable_cdpl2;
>> @@ -433,6 +436,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.
>> @@ -448,6 +452,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)
>> @@ -491,6 +496,13 @@ static inline bool resctrl_arch_get_cdp_enabled(enum resctrl_res_level l)
>>
>> int resctrl_arch_set_cdp_enabled(enum resctrl_res_level l, bool enable);
>>
>> +static inline bool resctrl_arch_get_abmc_enabled(enum resctrl_res_level l)
>> +{
>> + return rdt_resources_all[l].abmc_enabled;
>> +}
>
> This inline definition will not work in either this file or
> fs/resctrl/internal.h, following James's change[1] moving the code.
>
> resctrl_arch-definitions are either declared in linux/resctrl.h or
> defined inline in a file like asm/resctrl.h.
Yes, These definitions need to moved to asm/resctrl.h.
Moving that will involve moving few data structure as well.
It is better it is done during fs and arch restructure.
>
>
>> +
>> +int resctrl_arch_set_abmc_enabled(enum resctrl_res_level l, bool enable);
>> +
>> /*
>> * To return the common struct rdt_resource, which is contained in struct
>> * rdt_hw_resource, walk the resctrl member of struct rdt_hw_resource.
>> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> index 05f551bc316e..f49073c86884 100644
>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> @@ -850,9 +850,15 @@ static int rdtgroup_mbm_assign_show(struct kernfs_open_file *of,
>> struct seq_file *s, void *v)
>> {
>> struct rdt_resource *r = of->kn->parent->priv;
>> + struct rdt_hw_resource *hw_res = resctrl_to_arch_res(r);
>>
>> - if (r->mbm_assign_capable)
>> + if (r->mbm_assign_capable && hw_res->abmc_enabled) {
>> + seq_puts(s, "[abmc]\n");
>> + seq_puts(s, "legacy_mbm\n");
>> + } else if (r->mbm_assign_capable) {
>> seq_puts(s, "abmc\n");
>> + seq_puts(s, "[legacy_mbm]\n");
>> + }
>
> This looks like it would move to fs/resctrl/rdtgroup.c where it's not
> possible to dereference an rdt_hw_resource struct.
There are two rdtgroup.c files.
0 arch/x86/kernel/cpu/resctrl/rdtgroup.c
1 fs/resctrl/rdtgroup.c
I think this should move to arch rdtgroup.c. It is better it is done
during fs and arch restructure.
>
> It might be helpful to try building your changes on top of James's
> change[1] to get an idea of how this would fit in post-refactoring.
> I'll stop pointing out inconsistencies with his portability scheme
> now.
>
>>
>> return 0;
>> }
>> @@ -2433,6 +2439,74 @@ int resctrl_arch_set_cdp_enabled(enum resctrl_res_level l, bool enable)
>> return 0;
>> }
>>
>> +static void resctrl_abmc_msrwrite(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_setup(enum resctrl_res_level l, bool enable)
>> +{
>> + struct rdt_resource *r = &rdt_resources_all[l].r_resctrl;
>> + struct rdt_domain *d;
>> +
>> + /* Update QOS_CFG MSR on all the CPUs in cpu_mask */
>> + list_for_each_entry(d, &r->domains, list) {
>> + on_each_cpu_mask(&d->cpu_mask, resctrl_abmc_msrwrite, &enable, 1);
>> + resctrl_arch_reset_rmid_all(r, d);
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int resctrl_abmc_enable(enum resctrl_res_level l)
>> +{
>> + struct rdt_hw_resource *hw_res = &rdt_resources_all[l];
>> + int ret = 0;
>> +
>> + if (!hw_res->abmc_enabled) {
>> + ret = resctrl_abmc_setup(l, true);
>> + if (!ret)
>> + hw_res->abmc_enabled = true;
>
> Presumably this would be called holding the rdtgroup_mutex? Perhaps a
> lockdep assertion somewhere would be appropriate?
Yes. I have taken care of this.
>
> Thanks!
> -Peter
>
> [1] https://lore.kernel.org/lkml/20240321165106.31602-32-james.morse@arm.com/
>
--
Thanks
Babu Moger
Hi Peter, On 4/3/2024 5:30 PM, Peter Newman wrote: ... > > Presumably this would be called holding the rdtgroup_mutex? Perhaps a > lockdep assertion somewhere would be appropriate? > Considering that you are digging into the implementation already, can it be assumed that you approve (while considering how "soft RMID" may build on this) of the new interface as described in the cover letter? Reinette
Hi Reinette, On Thu, Apr 4, 2024 at 11:43 AM Reinette Chatre <reinette.chatre@intel.com> wrote: > > Hi Peter, > > On 4/3/2024 5:30 PM, Peter Newman wrote: > > ... > > > > Presumably this would be called holding the rdtgroup_mutex? Perhaps a > > lockdep assertion somewhere would be appropriate? > > > > Considering that you are digging into the implementation already, can > it be assumed that you approve (while considering how "soft RMID" may > build on this) of the new interface as described in the cover letter? Yes, I believe we came back to an agreement when discussing the last series. I'll look over the cover letter in this series just to make sure everything is there. -Peter
Hi Peter,
On 4/3/24 19:30, Peter Newman wrote:
> Hi Babu,
>
> On Thu, Mar 28, 2024 at 6:07 PM Babu Moger <babu.moger@amd.com> wrote:
>> struct rdt_fs_context {
>> struct kernfs_fs_context kfc;
>> bool enable_cdpl2;
>> @@ -433,6 +436,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.
>> @@ -448,6 +452,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)
>> @@ -491,6 +496,13 @@ static inline bool resctrl_arch_get_cdp_enabled(enum resctrl_res_level l)
>>
>> int resctrl_arch_set_cdp_enabled(enum resctrl_res_level l, bool enable);
>>
>> +static inline bool resctrl_arch_get_abmc_enabled(enum resctrl_res_level l)
>> +{
>> + return rdt_resources_all[l].abmc_enabled;
>> +}
>
> This inline definition will not work in either this file or
> fs/resctrl/internal.h, following James's change[1] moving the code.
Yea. I see..
>
> resctrl_arch-definitions are either declared in linux/resctrl.h or
> defined inline in a file like asm/resctrl.h.
ok.
>
>
>> +
>> +int resctrl_arch_set_abmc_enabled(enum resctrl_res_level l, bool enable);
>> +
>> /*
>> * To return the common struct rdt_resource, which is contained in struct
>> * rdt_hw_resource, walk the resctrl member of struct rdt_hw_resource.
>> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> index 05f551bc316e..f49073c86884 100644
>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> @@ -850,9 +850,15 @@ static int rdtgroup_mbm_assign_show(struct kernfs_open_file *of,
>> struct seq_file *s, void *v)
>> {
>> struct rdt_resource *r = of->kn->parent->priv;
>> + struct rdt_hw_resource *hw_res = resctrl_to_arch_res(r);
>>
>> - if (r->mbm_assign_capable)
>> + if (r->mbm_assign_capable && hw_res->abmc_enabled) {
>> + seq_puts(s, "[abmc]\n");
>> + seq_puts(s, "legacy_mbm\n");
>> + } else if (r->mbm_assign_capable) {
>> seq_puts(s, "abmc\n");
>> + seq_puts(s, "[legacy_mbm]\n");
>> + }
>
> This looks like it would move to fs/resctrl/rdtgroup.c where it's not
> possible to dereference an rdt_hw_resource struct.
>
> It might be helpful to try building your changes on top of James's
> change[1] to get an idea of how this would fit in post-refactoring.
> I'll stop pointing out inconsistencies with his portability scheme
> now.
Considering the complexity of James changes, I was hoping my series will
go first. It would be difficult for me to make changes based on transient
patch series. I would think it would be best to base the patches based on
tip/master.
>
>>
>> return 0;
>> }
>> @@ -2433,6 +2439,74 @@ int resctrl_arch_set_cdp_enabled(enum resctrl_res_level l, bool enable)
>> return 0;
>> }
>>
>> +static void resctrl_abmc_msrwrite(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_setup(enum resctrl_res_level l, bool enable)
>> +{
>> + struct rdt_resource *r = &rdt_resources_all[l].r_resctrl;
>> + struct rdt_domain *d;
>> +
>> + /* Update QOS_CFG MSR on all the CPUs in cpu_mask */
>> + list_for_each_entry(d, &r->domains, list) {
>> + on_each_cpu_mask(&d->cpu_mask, resctrl_abmc_msrwrite, &enable, 1);
>> + resctrl_arch_reset_rmid_all(r, d);
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int resctrl_abmc_enable(enum resctrl_res_level l)
>> +{
>> + struct rdt_hw_resource *hw_res = &rdt_resources_all[l];
>> + int ret = 0;
>> +
>> + if (!hw_res->abmc_enabled) {
>> + ret = resctrl_abmc_setup(l, true);
>> + if (!ret)
>> + hw_res->abmc_enabled = true;
>
> Presumably this would be called holding the rdtgroup_mutex? Perhaps a
> lockdep assertion somewhere would be appropriate?
Yes. Sure. Will add it next revision.
>
> Thanks!
> -Peter
>
> [1] https://lore.kernel.org/lkml/20240321165106.31602-32-james.morse@arm.com/
--
Thanks
Babu Moger
Hi Babu, On Thu, Apr 4, 2024 at 8:16 AM Moger, Babu <babu.moger@amd.com> wrote: > On 4/3/24 19:30, Peter Newman wrote: > > This looks like it would move to fs/resctrl/rdtgroup.c where it's not > > possible to dereference an rdt_hw_resource struct. > > > > It might be helpful to try building your changes on top of James's > > change[1] to get an idea of how this would fit in post-refactoring. > > I'll stop pointing out inconsistencies with his portability scheme > > now. > > Considering the complexity of James changes, I was hoping my series will > go first. It would be difficult for me to make changes based on transient > patch series. I would think it would be best to base the patches based on > tip/master. I don't need you to push the patches to the mailing list based on James's series. I was just asking you to try building locally on top of the refactoring changes. You are putting in the effort trying to make this code portable (i.e., inventing new resctrl_arch_-interfaces), so it would be sensible to check your work locally. However, I am the main stakeholder who cares about MPAM and ABMC working in the same kernel, so I can continue to give feedback on portability as I compose the series' together. -Peter
Hi Peter, On 4/4/24 12:36, Peter Newman wrote: > Hi Babu, > > On Thu, Apr 4, 2024 at 8:16 AM Moger, Babu <babu.moger@amd.com> wrote: > >> On 4/3/24 19:30, Peter Newman wrote: >>> This looks like it would move to fs/resctrl/rdtgroup.c where it's not >>> possible to dereference an rdt_hw_resource struct. >>> >>> It might be helpful to try building your changes on top of James's >>> change[1] to get an idea of how this would fit in post-refactoring. >>> I'll stop pointing out inconsistencies with his portability scheme >>> now. >> >> Considering the complexity of James changes, I was hoping my series will >> go first. It would be difficult for me to make changes based on transient >> patch series. I would think it would be best to base the patches based on >> tip/master. > > I don't need you to push the patches to the mailing list based on > James's series. I was just asking you to try building locally on top > of the refactoring changes. You are putting in the effort trying to > make this code portable (i.e., inventing new > resctrl_arch_-interfaces), so it would be sensible to check your work > locally. I am really no focusing much on portability in this series. I named it to match it with resctrl_arch_set_cdp_enabled. Yes. I got your concerns. I will plan check against James changes in next revision. > > However, I am the main stakeholder who cares about MPAM and ABMC > working in the same kernel, so I can continue to give feedback on > portability as I compose the series' together. Agree. Please continue your feedback. -- Thanks Babu Moger
© 2016 - 2026 Red Hat, Inc.