Re: [PATCH v4 13/19] x86/resctrl: Add data structures for ABMC assignment

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

Hi Reinette,

On 6/13/24 20:44, Reinette Chatre wrote:
> Hi Babu,
> 
> On 5/24/24 5:23 AM, Babu Moger wrote:
>> The ABMC feature provides an option to the user to assign a hardware
>> counter to an RMID and monitor the bandwidth as long as the counter
>> is assigned. The bandwidth events will be tracked by the hardware until
>> the user changes the configuration. Each resctrl group can configure
>> maximum two counters, one for total event and one for local event.
>>
>> The counters are configured by writing to MSR L3_QOS_ABMC_CFG.
>> Configuration is done by setting the counter id, bandwidth source (RMID),
>> bandwidth types. Reading L3_QOS_ABMC_DSC returns the configuration of
> 
> What is a "bandwidth type" here?

Its bandwidth configuration(reads, writes etc). It is
mbm_total_bytes_config", "mbm_local_bytes_config" in
https://www.kernel.org/doc/Documentation/x86/resctrl.rst

I will add that text.

> 
>> the counter id specified in L3_QOS_ABMC_CFG.
>>
>> Attempts to read or write these MSRs when ABMC is not enabled will result
>> in a #GP(0) exception.
>>
>> MSR L3_QOS_ABMC_CFG (0xC000_03FDh) and L3_QOS_ABMC_DSC (0xC000_03FEh)
>> details.
>> ==========================================================================
>> Bits     Mnemonic    Description        Access Type   Reset Value
>> ==========================================================================
>> 63     CfgEn         Configuration Enable     R/W         0
>>
>> 62     CtrEn         Counter Enable         R/W         0
>>
>> 61:53     –         Reserved         MBZ         0
>>
>> 52:48     CtrID         Counter Identifier    R/W        0
>>
>> 47     IsCOS        BwSrc field is a COS    R/W        0
>>             (not an RMID)
>>
>> 46:44     –        Reserved        MBZ        0
>>
>> 43:32    BwSrc        Bandwidth Source    R/W        0
>>             (RMID or COS)
>>
>> 31:0    BwType        Bandwidth types to    R/W        0
>>             track for this counter
>> ==========================================================================
>>
> 
> This is copy&paste from AMD spec but needs some translation to map it to
> resctrl.

Yes. Sure. Will elaborate.

> 
>> The feature details are documented in the APM listed below [1].
>> [1] AMD64 Architecture Programmer's Manual Volume 2: System Programming
>> Publication # 24593 Revision 3.41 section 19.3.3.3 Assignable Bandwidth
>> Monitoring (ABMC).
>>
>> Signed-off-by: Babu Moger <babu.moger@amd.com>
>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=206537
>> ---
>> v4: Added more descriptions.
>>      Changed the name abmc_ctr_id to ctr_id.
>>      Added L3_QOS_ABMC_DSC. Used for reading the configuration.
>>
>> v3: No changes.
>>
>> v2: No changes.
>> ---
>>   arch/x86/include/asm/msr-index.h       |  2 ++
>>   arch/x86/kernel/cpu/resctrl/internal.h | 30 ++++++++++++++++++++++++++
>>   2 files changed, 32 insertions(+)
>>
>> diff --git a/arch/x86/include/asm/msr-index.h
>> b/arch/x86/include/asm/msr-index.h
>> index 5f9a0139e98c..6d2fe39ac68f 100644
>> --- a/arch/x86/include/asm/msr-index.h
>> +++ b/arch/x86/include/asm/msr-index.h
>> @@ -1172,6 +1172,8 @@
>>   #define MSR_IA32_SMBA_BW_BASE        0xc0000280
>>   #define MSR_IA32_EVT_CFG_BASE        0xc0000400
>>   #define MSR_IA32_L3_QOS_EXT_CFG        0xc00003ff
>> +#define MSR_IA32_L3_QOS_ABMC_CFG    0xc00003fd
>> +#define MSR_IA32_L3_QOS_ABMC_DSC    0xc00003fe
>>     /* MSR_IA32_VMX_MISC bits */
>>   #define MSR_IA32_VMX_MISC_INTEL_PT                 (1ULL << 14)
>> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h
>> b/arch/x86/kernel/cpu/resctrl/internal.h
>> index 60a1ca0a11a7..45ed33f4f0ff 100644
>> --- a/arch/x86/kernel/cpu/resctrl/internal.h
>> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
>> @@ -107,6 +107,9 @@ cpumask_any_housekeeping(const struct cpumask *mask,
>> int exclude_cpu)
>>   #define ASSIGN_TOTAL            BIT(QOS_L3_MBM_TOTAL_EVENT_ID)
>>   #define ASSIGN_LOCAL            BIT(QOS_L3_MBM_LOCAL_EVENT_ID)
>>   +/* Maximum assignable counters per resctrl group */
>> +#define MAX_CNTRS        2
>> +
>>   struct rdt_fs_context {
>>       struct kernfs_fs_context    kfc;
>>       bool                enable_cdpl2;
>> @@ -211,6 +214,7 @@ enum rdtgrp_mode {
>>    * @crdtgrp_list:        child rdtgroup node list
>>    * @rmid:            rmid for this rdtgroup
>>    * @mon_state:            Assignment state of the group
>> + * @ctr_id:            ABMC counter ids assigned to this group
>>    */
>>   struct mongroup {
>>       struct kernfs_node    *mon_data_kn;
>> @@ -218,6 +222,7 @@ struct mongroup {
>>       struct list_head    crdtgrp_list;
>>       u32            rmid;
>>       u32            mon_state;
>> +    u32            ctr_id[MAX_CNTRS];
> 
> To match "num_cntrs" it seems "cntr_id" may be more appropriate?

Yes. Will change appropriately.

> 
>>   };
>>     /**
>> @@ -568,6 +573,31 @@ union cpuid_0x10_x_edx {
>>       unsigned int full;
>>   };
>>   +/*
>> + * ABMC counters can be configured  by writing to L3_QOS_ABMC_CFG.
> 
> extra space

ok

> 
>> + * @bw_type        : Bandwidth types to track for this counter
> 
> What is a "Bandwidth type"? What are possible values?

Its bandwidth configuration(reads, writes etc). It is
mbm_total_bytes_config", "mbm_local_bytes_config" in
https://www.kernel.org/doc/Documentation/x86/resctrl.rst
It is a bitmap. Will clarify  that.


> 
>> + * @bw_src        : Bandwidth Source (RMID or CLOSID)
>> + * @reserved1        : Reserved
>> + * @is_cos        : BwSrc field is a COS (not an RMID)
> 
> Please be consistent wrt "CLOS"/"CLOSID" in comment and member name

Sure. Will change it to CLOSID.

> 
>> + * @ctr_id        : Counter Identifier
> 
> Please be consistent with "cntr" vs "ctr". Earlier the series
> introduces "num_cntrs" so cntr seems appropriate?

Sure.

> 
>> + * @reserved        : Reserved
>> + * @ctr_en        : Counter Enable bit
>> + * @cfg_en        : Configuration Enable bit
> 
> Why are there two enable bits? How are they used?

I am setting both these bits when configuring, I need some clarifications
on these bits with h/w team. I need to know how different combinations
changes the behavior. I will confirm about this.

> 
> (please match other kernel doc wrt : usage)
> 
>> + */
>> +union l3_qos_abmc_cfg {
>> +    struct {
>> +        unsigned long  bw_type    :32,
>> +                   bw_src    :12,
>> +                   reserved1: 3,
>> +                   is_cos    : 1,
>> +                   ctr_id    : 5,
>> +                   reserved    : 9,
>> +                   ctr_en    : 1,
>> +                   cfg_en    : 1;
>> +    } split;
>> +    unsigned long full;
>> +};
>> +
>>   void rdt_last_cmd_clear(void);
>>   void rdt_last_cmd_puts(const char *s);
>>   __printf(1, 2)
> 
> Reinette
> 

-- 
Thanks
Babu Moger