Re: [PATCH 02/15] x86/resctrl: Remove hard-coded memory bandwidth event configuration

[Moger, Babu]

Hi Reinette,

On 12/5/23 17:21, Reinette Chatre wrote:
> Hi Babu,
> 
> On 11/30/2023 4:57 PM, Babu Moger wrote:
>> If the BMEC (Bandwidth Monitoring Event Configuration) feature is
>> supported, the bandwidth events can be configured. Currently, the maximum
>> supported event bitmask is hard-coded. This information can be detected by
>> the CPUID_Fn80000020_ECX_x03.
> 
> Be careful here ... the original meaning is the maximum length of the bitmask
> and the new meaning is the maximum valid bitmask. Looking at the AMD spec
> it looks to me that the original meaning is still valid, the number of
> bits available in register to configure the bandwidth types is still the same.
> 
> There is additional information about which of those bits are actually supported
> by the hardware.

There is no change in the definition. Definition is still the same. I just
wanted to remove hard-coding.

Will make that clear.

> 
>>
>> CPUID_Fn80000020_ECX_x03 [Platform QoS Monitoring Bandwidth Event
>> Configuration] Read-only. Reset: 0000_007Fh.
>> Bits	Description
>> 31:7	Reserved
>>  6:0	Identifies the bandwidth sources that can be tracked.
> 
> So above means that only bits 0 to 6 can be used for config of event type? This
> is done in current implementation and I do not think this should change.
> Learning and using the supported events from hardware is new.

That is correct. Right now bits 0 to 6 are only used. New hardware can add
more bits here to count new type of event.

> 
>>
>> Remove the hardcoded value and detect using CPUID command.
>>
>> The CPUID details are documentation in the PPR listed below [1].
>> [1] Processor Programming Reference (PPR) Vol 1.1 for AMD Family 19h Model
>> 11h B1 - 55901 Rev 0.25.
>>
>> Fixes: dc2a3e857981 ("x86/resctrl: Add interface to read mbm_total_bytes_config")
> 
> Please highlight the impact here to understand if this is

This is just to make sure not to change code for new hardware adds new
event. There is not much impact currently.


> stable material. This also does not seem part of this series.

Agree. This is not specific to this series.

> 
>> Signed-off-by: Babu Moger <babu.moger@xxxxxxx>
>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=206537
>> ---
>>  arch/x86/kernel/cpu/resctrl/internal.h |  4 +---
>>  arch/x86/kernel/cpu/resctrl/monitor.c  | 11 +++++++++++
>>  arch/x86/kernel/cpu/resctrl/rdtgroup.c |  4 ++--
>>  3 files changed, 14 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
>> index d2979748fae4..524d8bec1439 100644
>> --- a/arch/x86/kernel/cpu/resctrl/internal.h
>> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
>> @@ -50,9 +50,6 @@
>>  /* Dirty Victims to All Types of Memory */
>>  #define DIRTY_VICTIMS_TO_ALL_MEM	BIT(6)
>>  
>> -/* Max event bits supported */
>> -#define MAX_EVT_CONFIG_BITS		GENMASK(6, 0)
>> -
>>  struct rdt_fs_context {
>>  	struct kernfs_fs_context	kfc;
>>  	bool				enable_cdpl2;
>> @@ -117,6 +114,7 @@ extern bool rdt_alloc_capable;
>>  extern bool rdt_mon_capable;
>>  extern unsigned int rdt_mon_features;
>>  extern struct list_head resctrl_schema_all;
>> +extern unsigned int resctrl_max_evt_bitmask;
>>  
> 
> Why is this global and not resource specific like other monitoring
> properties?

It does not have to be global. I can add it part rdt_hw_resource.

> 
>>  enum rdt_group_type {
>>  	RDTCTRL_GROUP = 0,
>> diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
>> index f136ac046851..c611b16ba259 100644
>> --- a/arch/x86/kernel/cpu/resctrl/monitor.c
>> +++ b/arch/x86/kernel/cpu/resctrl/monitor.c
>> @@ -127,6 +127,11 @@ static const struct mbm_correction_factor_table {
>>  static u32 mbm_cf_rmidthreshold __read_mostly = UINT_MAX;
>>  static u64 mbm_cf __read_mostly;
>>  
>> +/*
>> + * Identifies the list of QoS Bandwidth Sources to track
>> + */
>> +unsigned int resctrl_max_evt_bitmask;
>> +
>>  static inline u64 get_corrected_mbm_count(u32 rmid, unsigned long val)
>>  {
>>  	/* Correct MBM value. */
>> @@ -813,6 +818,12 @@ int __init rdt_get_mon_l3_config(struct rdt_resource *r)
>>  		return ret;
>>  
>>  	if (rdt_cpu_has(X86_FEATURE_BMEC)) {
>> +		u32 eax, ebx, ecx, edx;
>> +
>> +		/* Detect list of bandwidth sources that can be tracked */
>> +		cpuid_count(0x80000020, 3, &eax, &ebx, &ecx, &edx);
>> +		resctrl_max_evt_bitmask = ecx;
>> +
>>  		if (rdt_cpu_has(X86_FEATURE_CQM_MBM_TOTAL)) {
>>  			mbm_total_event.configurable = true;
>>  			mbm_config_rftype_init("mbm_total_bytes_config");
>> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> index 69a1de92384a..6c22718dbaa2 100644
>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> @@ -1547,7 +1547,7 @@ static void mon_event_config_read(void *info)
>>  	rdmsrl(MSR_IA32_EVT_CFG_BASE + index, msrval);
>>  
>>  	/* Report only the valid event configuration bits */
>> -	mon_info->mon_config = msrval & MAX_EVT_CONFIG_BITS;
>> +	mon_info->mon_config = msrval & resctrl_max_evt_bitmask;
> 
> Original code still looks correct to me. Just like before the first seven
> bits of  MSR_IA32_EVT_CFG_BASE contains the event configuration.

Original code still works.

> 
> Comparing with supported bits would be an additional check, but what does
> that imply? Would it be possible for hardware to have a bit set that is
> not supported? Would that mean it is actually supported or a hardware bug?

No. Hardware supports all the bits reported here. Like i said before
wanted to remove the hard-coded value.

> 
>>  }
>>  
>>  static void mondata_config_read(struct rdt_domain *d, struct mon_config_info *mon_info)
>> @@ -1621,7 +1621,7 @@ static int mbm_config_write_domain(struct rdt_resource *r,
>>  	int ret = 0;
>>  
>>  	/* mon_config cannot be more than the supported set of events */
>> -	if (val > MAX_EVT_CONFIG_BITS) {
>> +	if (val > resctrl_max_evt_bitmask) {
>>  		rdt_last_cmd_puts("Invalid event configuration\n");
>>  		return -EINVAL;
>>  	}
> 
> This does not look right. resctrl_max_evt_bitmask contains the supported
> types. A user may set a value that is less than resctrl_max_evt_bitmask but
> yet have an unsupported bit set, no?

I think I have to make this clear in the patch. There is no difference in
the definition. Hardware supports all the events reported by the cpuid.
-- 
Thanks
Babu Moger