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

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

Hi Reinette,

Thanks for the quick response.

On 9/11/26 17:03, Reinette Chatre wrote:
> 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

Sure.

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


You're right. The spec overlooked this scenario. I've notified the 
concerned team.


Do you want me add this check?

-               r->mon.num_mbm_cntrs = (ebx & GENMASK(15, 0)) + 1;
+               r->mon.num_mbm_cntrs = (ebx & GENMASK(12, 0)) + 1;


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

Two fields(ctrl_id and bw_src) in here are changing for this register. I 
dont know about other changes yet.

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

The RMID field width is expected to change. We can revisit this when 
that happens.

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

Yes. I agree and support that approach. Looks like all the maintainers 
are inline with dropping 32. Will drop this change.

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

Looks good. Thanks. Will update if required.

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

Yes. It is. Will add it in next revision.

Thanks
Babu