| 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
| ||
|
Message-ID: <1e8b0a20-d12c-4f1b-9fc5-c4dfafb7c1dd@amd.com>
Date: Tue, 22 Jul 2025 10:51:52 -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/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
>> or if an invalid counter ID was set in the QM_EVTSEL[RMID] field.
>>
>> Introduce resctrl_arch_reset_cntr() and resctrl_arch_cntr_read() to read
>> event data for a specific counter.
>
> hmmm ... they cannot both be used to read the event data?
>
Yes. Corrected it.
>>
>> Signed-off-by: Babu Moger <babu.moger@....com>
>> ---
>
> ...
>
>> ---
>> arch/x86/kernel/cpu/resctrl/internal.h | 6 +++
>> arch/x86/kernel/cpu/resctrl/monitor.c | 70 ++++++++++++++++++++++++++
>> 2 files changed, 76 insertions(+)
>>
>> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
>> index 6bf6042f11b6..ae4003d44df4 100644
>> --- a/arch/x86/kernel/cpu/resctrl/internal.h
>> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
>> @@ -40,6 +40,12 @@ struct arch_mbm_state {
>> /* Setting bit 0 in L3_QOS_EXT_CFG enables the ABMC feature. */
>> #define ABMC_ENABLE_BIT 0
>>
>> +/*
>> + * Qos Event Identifiers.
>> + */
>> +#define ABMC_EXTENDED_EVT_ID BIT(31)
>> +#define ABMC_EVT_ID BIT(0)
>> +
>> /**
>> * struct rdt_hw_ctrl_domain - Arch private attributes of a set of CPUs that share
>> * a resource for a control function
>> 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"
>
>> + */
>> + wrmsr(MSR_IA32_QM_EVTSEL, ABMC_EXTENDED_EVT_ID | ABMC_EVT_ID, cntr_id);
>> + rdmsrl(MSR_IA32_QM_CTR, msr_val);
>> +
>> + if (msr_val & RMID_VAL_ERROR)
>> + return -EIO;
>> + if (msr_val & RMID_VAL_UNAVAIL)
>> + return -EINVAL;
>
> One of these must be the "E bit" (unclear which), leaving other undefined?
>
>> +
>> + *val = msr_val;
>> + return 0;
>> +}
>> +
>> +void resctrl_arch_reset_cntr(struct rdt_resource *r, struct rdt_mon_domain *d,
>> + u32 unused, u32 rmid, int cntr_id,
>> + enum resctrl_event_id eventid)
>> +{
>> + struct rdt_hw_mon_domain *hw_dom = resctrl_to_arch_mon_dom(d);
>> + struct arch_mbm_state *am;
>> +
>> + am = get_arch_mbm_state(hw_dom, rmid, eventid);
>> + if (am) {
>> + memset(am, 0, sizeof(*am));
>> +
>> + /* Record any initial, non-zero count value. */
>> + __cntr_id_read(cntr_id, &am->prev_msr);
>> + }
>> +}
>> +
>> +int resctrl_arch_cntr_read(struct rdt_resource *r, struct rdt_mon_domain *d,
>> + u32 unused, u32 rmid, int cntr_id,
>> + enum resctrl_event_id eventid,
>> + u64 *val, void *ignored)
>> +{
>> + u64 msr_val;
>> + int ret;
>> +
>> + resctrl_arch_rmid_read_context_check();
>
> Is this needed? I see this context check as related to the MPAM cache occupancy
> counters that need to be allocated when a counter is read (see comments of
> resctrl_arch_rmid_read() in include/linux/resctrl.h). To me this does not seem
> relevant to this work since all allocations should already have been done
> when the counter was allocated, no?
Yes. No required. Removed it.
>
>> +
>> + ret = __cntr_id_read(cntr_id, &msr_val);
>> + if (ret)
>> + return ret;
>> +
>> + *val = mbm_corrected_val(r, d, rmid, eventid, msr_val);
>> +
>> + return 0;
>> +}
>> +
>> /*
>> * The power-on reset value of MSR_RMID_SNC_CONFIG is 0x1
>> * which indicates that RMIDs are configured in legacy mode.
>
> Reinette
>
>
--
Thanks
Babu Moger
Powered by blists - more mailing lists