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

[Babu Moger]

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

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