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

[Reinette Chatre <reinette.chatre@intel.com>]

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.

> 
> 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.

> 
> 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
stable material. This also does not seem part of this series.

> Signed-off-by: Babu Moger <babu.moger@amd.com>
> 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?

>  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.

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?

>  }
>  
>  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?

Reinette