Re: [PATCH v6 13/22] x86/resctrl: Add data structures and definitions for ABMC assignment

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

Hi Reinette,


On 8/16/24 16:38, Reinette Chatre wrote:
> Hi Babu,
> 
> This patch now only introduces one data structure so the subject could
> be made more specific.

How about?

x86/resctrl: Add data structures for L3_QOS_ABMC_CFG MSR

> 
> On 8/6/24 3:00 PM, 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.
>>
>>
> 
> (extra empty line)

Sure.
> 
>> The ABMC feature implements a pair of MSRs, L3_QOS_ABMC_CFG (C000_03FDh)
>> and L3_QOS_ABMC_DSC (C000_3FEh). The counters are configured by writing
>> to MSR L3_QOS_ABMC_CFG. Configuration is done by setting the counter id,
>> bandwidth source (RMID) and bandwidth configuration supported by BMEC
>> (Bandwidth Monitoring Event Configuration).
>>
>> L3_QOS_ABMC_DSC is a read-only MSR. Reading L3_QOS_ABMC_DSC returns the
>> configuration of the counter id specified in L3_QOS_ABMC_CFG.cntr_id
>> with rmid(bw_src) and event configuration(bw_type).
>>
>> Attempts to read or write these MSRs when ABMC is not enabled will result
>> in a #GP(0) exception.
>>
>> Introduce data structures and definitions for ABMC MSRs.
>>
>> MSR L3_QOS_ABMC_CFG (0xC000_03FDh) and L3_QOS_ABMC_DSC (0xC000_03FEh)
>> details.
> 
> The changelog and patch introduce L3_QOS_ABMC_DSC but I cannot see that it is
> used in this series.

Yes. I was using it in v5 to read the configuration back. It is not
required anymore. I will remove it.

> 
>> =========================================================================
>> Bits     Mnemonic    Description            Access Reset
>>                             Type   Value
>> =========================================================================
>> 63     CfgEn         Configuration Enable         R/W     0
>>
>> 62     CtrEn         Enable/disable Tracking        R/W     0
>>
>> 61:53     –         Reserved             MBZ     0
>>
>> 52:48     CtrID         Counter Identifier        R/W    0
>>
>> 47     IsCOS        BwSrc field is a CLOSID        R/W    0
>>             (not an RMID)
>>
>> 46:44     –        Reserved            MBZ    0
>>
>> 43:32    BwSrc        Bandwidth Source        R/W    0
>>             (RMID or CLOSID)
>>
>> 31:0    BwType        Bandwidth configuration        R/W    0
>>             to track for this counter
>> ==========================================================================
>>
>> Configuration and tracking:
>> CfgEn=1,CtrEn=0 : Configure CtrID and but no tracking the events yet.
>> CfgEn=1,CtrEn=1 : Configure CtrID and start tracking events.
> 
> Could you please add the above snippet noting field combinations to the
> kernel-doc of the union?

Sure.
> 
>>
>> 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).
>>
>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=206537
>> Signed-off-by: Babu Moger <babu.moger@amd.com>
>> ---
>> v6: Removed all the fs related changes.
>>      Added note on CfgEn,CtrEn.
>>      Removed the definitions which are not used.
>>      Removed cntr_id initialization.
>>
>> v5: Moved assignment flags here (path 10/19 of v4).
>>      Added MON_CNTR_UNSET definition to initialize cntr_id's.
>>      More details in commit log.
>>      Renamed few fields in l3_qos_abmc_cfg for readability.
>>
>> 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 | 26 ++++++++++++++++++++++++++
>>   2 files changed, 28 insertions(+)
>>
>> diff --git a/arch/x86/include/asm/msr-index.h
>> b/arch/x86/include/asm/msr-index.h
>> index d86469bf5d41..5b3931a59d5a 100644
>> --- a/arch/x86/include/asm/msr-index.h
>> +++ b/arch/x86/include/asm/msr-index.h
>> @@ -1183,6 +1183,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 1021227d8c7e..af3efa35a62e 100644
>> --- a/arch/x86/kernel/cpu/resctrl/internal.h
>> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
>> @@ -589,6 +589,32 @@ union cpuid_0x10_x_edx {
>>       unsigned int full;
>>   };
>>   +/*
>> + * ABMC counters can be configured by writing to L3_QOS_ABMC_CFG.
>> + * @bw_type        : Bandwidth configuration(supported by BMEC)
>> + *              tracked by the @cntr_id.
>> + * @bw_src        : Bandwidth source (RMID or CLOSID).
>> + * @reserved1        : Reserved.
>> + * @is_clos        : @bw_src field is a CLOSID (not an RMID).
>> + * @cntr_id        : Counter identifier.
>> + * @reserved        : Reserved.
>> + * @cntr_en        : Tracking enable bit.
>> + * @cfg_en        : Configuration enable bit.
>> + */
>> +union l3_qos_abmc_cfg {
>> +    struct {
>> +        unsigned long    bw_type    :32,
>> +                bw_src    :12,
>> +                reserved1: 3,
>> +                is_clos    : 1,
>> +                cntr_id    : 5,
>> +                reserved : 9,
>> +                cntr_en    : 1,
>> +                cfg_en    : 1;
>> +    } split;
>> +    unsigned long full;
>> +};
>> +
> 
> This data structure still uses tabs that seem to have goal of aligning
> members
> but the tabs are used inconsistently and members are not lining up either.

Sorry. I always have issues with these tabs. Will address it next revision.

> 
>>   void rdt_last_cmd_clear(void);
>>   void rdt_last_cmd_puts(const char *s);
>>   __printf(1, 2)
> 
> Reinette
> 

-- 
Thanks
Babu Moger