Re: [PATCH v15 22/34] x86/resctrl: Implement resctrl_arch_reset_cntr() and resctrl_arch_cntr_read()

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

Hi Babu,

On 7/22/25 8:51 AM, Moger, Babu wrote:
> Hi Reinette,
> 
> On 7/17/25 22:51, Reinette Chatre wrote:
>> Hi Babu,
>>
>> On 7/8/25 3:17 PM, Babu Moger wrote:
>>> System software can read resctrl event data for a particular resource by
>>> writing the RMID and Event Identifier (EvtID) to the QM_EVTSEL register
>>> and then reading the event data from the QM_CTR register.
>>>
>>> In ABMC mode, the event data of a specific counter ID can be read by
>>> setting the following fields in QM_EVTSEL.ExtendedEvtID = 1,
>>
>> Seems easier to parse when "fields in" -> "fields:".
>>
> 
> Sure.
> 
> 
>>> QM_EVTSEL.EvtID = L3CacheABMC (=1) and setting [RMID] to the desired
>>> counter ID. Reading QM_CTR will then return the contents of the specified
>>> counter ID. The E bit will be set if the counter configuration was invalid,
>>
>> Where is "the E bit" defined?
> 
> QM_CTR MSRS bits
> 
> Bits        Mnemonic    Description.
> 63		E       Error on access to counter
> 62		U       Count for this event is currently unavailable
> 61-CNT_LEN   	_       Reserved, read as zero
> CNT_LEN-1:0  	CNT	Count of monitored resource or event
> 
> 
> The bit 63 is "E bit"  -> RMID_VAL_ERROR
> the bit 62 is "U bit"  -> RMID_VAL_UNAVAIL
> 
> How about?
> 
> The RMID_VAL_ERROR bit will be set if the counter configuration
> was invalid, or if an invalid counter ID was set in the QM_EVTSEL[RMID] field

It is not clear to me why the comments only mention one of the error bits handled
in the code.
The motivation for hardware counters is that reading of RMID may return
"Unavailable". Does this imply that reading a hardware counter should
never return "Unavailable"? Why is this error handled as possible when
reading a counter though?


>>> diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
>>> index a230d98e9d73..026c2e2d19d3 100644
>>> --- a/arch/x86/kernel/cpu/resctrl/monitor.c
>>> +++ b/arch/x86/kernel/cpu/resctrl/monitor.c
>>> @@ -259,6 +259,76 @@ int resctrl_arch_rmid_read(struct rdt_resource *r, struct rdt_mon_domain *d,
>>>  	return 0;
>>>  }
>>>  
>>> +static int __cntr_id_read(u32 cntr_id, u64 *val)
>>> +{
>>> +	u64 msr_val;
>>> +
>>> +	/*
>>> +	 * QM_EVTSEL Register definition:
>>> +	 * =======================================================
>>> +	 * Bits    Mnemonic        Description
>>> +	 * =======================================================
>>> +	 * 63:44   --              Reserved
>>> +	 * 43:32   RMID            Resource Monitoring Identifier
>>> +	 * 31      ExtEvtID        Extended Event Identifier
>>> +	 * 30:8    --              Reserved
>>> +	 * 7:0     EvtID           Event Identifier
>>> +	 * =======================================================
>>> +	 * The contents of a specific counter can be read by setting the
>>> +	 * following fields in QM_EVTSEL.ExtendedEvtID(=1) and
>>> +	 * QM_EVTSEL.EvtID = L3CacheABMC (=1) and setting [RMID] to the
>>> +	 * desired counter ID. Reading QM_CTR will then return the
>>> +	 * contents of the specified counter. The E bit will be set if the
>>> +	 * counter configuration was invalid, or if an invalid counter ID
>>> +	 * was set in the QM_EVTSEL[RMID] field.
>>
>> (same comments as changelog)
> 
> "The E bit" -> "The RMID_VAL_ERROR bit"

For comments to be accurate, per the handling that follows, RMID_VAL_UNAVAIL
is also a possibility, or is it not actually possible and just coded to match
RMID reading?

Reinette