Re: [PATCH v5 06/20] x86/resctrl: Add support to enable/disable AMD ABMC feature

["Moger, Babu" <babu.moger@amd.com>]

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