Re: [PATCH v3 1/3] x86/resctrl: Fix ABMC counter programming

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

Hi Reinette,

Thanks for the quick review.

On 10/6/26 12:47, Reinette Chatre wrote:
> Hi Babu,
> 
> On 10/2/26 2:26 PM, Babu Moger wrote:
>> AMD's Assignable Bandwidth Monitoring Counters (ABMC) are configured via
>> MSR_IA32_L3_QOS_ABMC_CFG. The architecture [1] received an update that
>> expands the counter ID field (l3_qos_abmc_cfg.split.cntr_id) from 5 to 12
> 
> This describes an architecture change as a code change (l3_qos_abmc_cfg)?
> 
> The subject also claims a "fix" but above establishes this just as an update because
> of architecture change without making clear what the problem needing fixing is.
> using the tip guidance when writing changelog will ensure that problem description
> is always clear.

Ack.>
>> bits.
>>
> 
>> Use the updated field width. The number of supported counters is enumerated
> 
> The second sentence of above paragraph seems out of place as it seems to add
> in a bit of additional context as part of the rest of the paragraph that describes
> the fix.
> 
>> separately. Mark this as a fix to the original ABMC support to avoid
> 
> Tag can speak for itself, just focus on describing the problem being fixed.
> 
> The problem and fix are fused together making it difficult to see what is being
> fixed here. Placing problem in its own paragraph helps to make this clear.
> 
>> misconfigurations caused by truncating counter IDs on hardware that
>> supports a large number of counters.
>>
>> Also limit the number of supported counters to the maximum value that can
>> be represented by the 12-bit cntr_id field if hardware reports more than
>> 12 bits.
> 
> This is the actual fix though but is added as an afterthought?
> 

Yea.

>>
>> The AMD64 Architecture Programmer's Manual [1], available at [2], will be
>> updated in a future revision to document the expanded cntr_id field.
> 
> 
> Consider an example changelog like below that aims to follow the x86 changelog
> guidelines:
> 
> AMD's Assignable Bandwidth Monitoring Counters (ABMC) are configured via
> MSR_IA32_L3_QOS_ABMC_CFG; MSR_IA32_L3_QOS_ABMC_CFG.cntr_id selects which counter
> the configuration applies to. The number of counters a platform supports (the
> number of possible values written to MSR_IA32_L3_QOS_ABMC_CFG.cntr_id) is
> enumerated separately via CPUID.
> 
> On a platform that enumerates more than 32 counters, writing a counter ID
> through the current 5-bit encoding truncates the ID and misprograms ABMC.
> 
> The AMD64 Architecture Programmer's Manual [1], available from [2], has been
> updated to widen MSR_IA32_L3_QOS_ABMC_CFG.cntr_id from 5 bits to 12 bits (the
> published revision 3.41 does not yet reflect this; a future revision will). The
> CPUID enumeration reports the maximum counter ID in a 16-bit field and can
> still report more counters than a 12-bit MSR_IA32_L3_QOS_ABMC_CFG.cntr_id
> can address.
> 
> Widen MSR_IA32_L3_QOS_ABMC_CFG.cntr_id to 12 bits to match the architecture. Cap
> the enumerated counter count at BIT(12) so every counter ID resctrl uses can be
> written to MSR_IA32_L3_QOS_ABMC_CFG.cntr_id without truncation.
> 
> 

Looks good.

>>
>> [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")
>> Signed-off-by: Babu Moger <babu.moger@amd.com>
>> Cc: stable@vger.kernel.org
>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=206537 # [2]
>> ---
> Patch looks good to me.
  Thank you.

Babu