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

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

Hi Reinette,

On 7/22/25 18:27, Reinette Chatre wrote:
> 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?

Checked again here. Yes. RMID_VAL_UNAVAIL is also a possibility. I should
have added the text before.

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

Added the text now.

"The RMID_VAL_UNAVAIL bit will be set if the counter data is not currently
available."


-- 
Thanks
Babu Moger