Re: [PATCH v2 1/3] x86/resctrl: Fix ABMC counter programming for extended counter ranges

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

Hi Babu,

What does the "for extended counter ranges" in subject refer to? As I understand
"extended events" is another term for ABMC so this seems redundant? 
"counter ranges" also just seems to refer to one "part" of this patch so perhaps
it could just be:
	x86/resctrl: Fix ABMC counter programming

On 9/4/26 11:06 AM, Babu Moger wrote:
> Memory Bandwidth Monitoring (MBM) can report incorrect values when ABMC is
> enabled on systems supporting more than 32 ABMC counters. As the number of
> active monitoring groups increases beyond the range supported by the
> existing counter ID encoding, programming an ABMC counter may inadvertently
> affect a different counter, resulting in unexpected counter resets and
> abnormally large MBM readings.
> 
> The issue originates from the ABMC counter programming interface in the
> L3_QOS_ABMC_CFG MSR. The counter ID field is currently defined as 5 bits,
> which limits the addressable counter range to 32 counters. On systems
> implementing more than 32 ABMC counters, counter IDs above 31 cannot be
> encoded correctly. Consequently, programming a counter ID beyond the
> supported range may target an unintended counter and reset bandwidth
> statistics associated with another monitoring group.
> 
> While updating this logic, it was also observed that the bw_src field,
> which encodes the RMID, is currently at its 12-bit limit with support for
> 4096 RMIDs. This field also needs to be updated for future expansion.
> 
> Also found one more pre-existing issue. This union structure can truncate
> data on 32-bit x86 systems when unsigned long is used.
> 
> Fix the issues with the following changes:
> 
> 1. Update the cntr_id field handling to support the full hardware ABMC
> counter range and ensure that counter programming does not interfere with
> unrelated counters.

Sashiko's assessment that this statement is not accurate looks correct to me.

Looks like the enumeration needs a check to limit the number of supported
counters if the hardware supports more than what can be configured? It seems
awkward that such hardware could exist and unclear why the spec has a mismatch in
the number of bits here. Even if cntr_id expands further to use the remaining
reserved bits it would still not be sufficient to configure all the numbers
that hardware may claim to support. Are there perhaps more field width changes
in this upcoming spec update?

> 
> 2. Expand the bw_src field to 15 bits.

Do the comments describing the RMID field width when reading the monitoring
data (__cntr_id_read() and __rmid_read_phys()) need an update also? 

 
> 3. Change "unsigned long" to u64 to fix truncation on 32-bit x86.

Sashiko found that this change by itself is not sufficient to address issues
with 32-bit. What do you think of dropping this part of the patch and instead
adding your support to:

https://lore.kernel.org/lkml/20260831174421.13921-22-tony.luck@intel.com/ ?

> 
> The AMD64 Architecture Programmer's Manual [1] available at [2] will be
> updated accordingly in a future revision to document the expanded cntr_id
> and bw_src field definitions.

This changelog uses a lot of text to describe one of these spec updates and then,
seemingly as an afterthought, describe two more changes in a way that hints that
these should be separate patches.

Could the changelog be simplified (after dropping the 64-bit change) to something
like:

	AMD's Assignable Bandwidth Monitoring Counters (ABMC) are configured via        
	MSR_IA32_L3_QOS_ABMC_CFG. The architecture [1] received an update that          
	increases the width of two of the MSR's fields:                                 
                                                                                
	1. The counter ID (represented by l3_qos_abmc_cfg.split.cntr_id) increases
	   from 5 to 12 bits.
                                                                                
	2. The bandwidth source (represented by l3_qos_abmc_cfg.split.bw_src), used
	   for the RMID, increases from 12 to 15 bits.                                  
                                                                                
	Use the new field widths. The number of supported counters and RMID are
	enumerated separately. Designate this update as a fix to original enabling
	to avoid misconfigurations resulting from truncating the counter ID and RMID             
	on hardware that support a large number of these IDs.   

The changelog is just a suggestion based on its current form - other potential changes
like the comments and enumeration checks are not captured by it, but should be if/when
they are added.

> 
> [1] AMD64 Architecture Programmer's Manual Volume 2: System Programming,
>     Publication #24593, Revision 3.41, Section 19.3.3.3 "Assignable
>     Bandwidth Monitoring (ABMC)"
> 
> Fixes: 84ecefb76674 ("x86/resctrl: Add data structures and definitions for ABMC assignment")

Is this a stable candidate?

> Signed-off-by: Babu Moger <babu.moger@amd.com>
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=206537 # [2]
> ---
Reinette