linux-kernel - Re: [PATCH v15 22/34] x86/resctrl: Implement resctrl_arch_reset_cntr() and resctrl_arch_cntr

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <cb0201d7-5f88-4f0f-b335-246dc684fae1@amd.com>
Date: Wed, 23 Jul 2025 11:48:24 -0500
From: "Moger, Babu" <babu.moger@....com>
To: Reinette Chatre <reinette.chatre@...el.com>, corbet@....net,
 tony.luck@...el.com, james.morse@....com, tglx@...utronix.de,
 mingo@...hat.com, bp@...en8.de, dave.hansen@...ux.intel.com
Cc: Dave.Martin@....com, x86@...nel.org, hpa@...or.com,
 akpm@...ux-foundation.org, paulmck@...nel.org, rostedt@...dmis.org,
 Neeraj.Upadhyay@....com, david@...hat.com, arnd@...db.de, fvdl@...gle.com,
 seanjc@...gle.com, jpoimboe@...nel.org, pawan.kumar.gupta@...ux.intel.com,
 xin@...or.com, manali.shukla@....com, tao1.su@...ux.intel.com,
 sohil.mehta@...el.com, kai.huang@...el.com, xiaoyao.li@...el.com,
 peterz@...radead.org, xin3.li@...el.com, kan.liang@...ux.intel.com,
 mario.limonciello@....com, thomas.lendacky@....com, perry.yuan@....com,
 gautham.shenoy@....com, chang.seok.bae@...el.com, linux-doc@...r.kernel.org,
 linux-kernel@...r.kernel.org, peternewman@...gle.com, eranian@...gle.com
Subject: Re: [PATCH v15 22/34] x86/resctrl: Implement
 resctrl_arch_reset_cntr() and resctrl_arch_cntr_read()

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