| 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: <00daaafe-3ead-497e-b441-ce1da8f4adc4@intel.com>
Date: Thu, 22 May 2025 16:31:14 -0700
From: Reinette Chatre <reinette.chatre@...el.com>
To: Babu Moger <babu.moger@....com>, <corbet@....net>, <tony.luck@...el.com>,
<tglx@...utronix.de>, <mingo@...hat.com>, <bp@...en8.de>,
<dave.hansen@...ux.intel.com>
CC: <james.morse@....com>, <dave.martin@....com>, <fenghuay@...dia.com>,
<x86@...nel.org>, <hpa@...or.com>, <paulmck@...nel.org>,
<akpm@...ux-foundation.org>, <thuth@...hat.com>, <rostedt@...dmis.org>,
<ardb@...nel.org>, <gregkh@...uxfoundation.org>,
<daniel.sneddon@...ux.intel.com>, <jpoimboe@...nel.org>,
<alexandre.chartre@...cle.com>, <pawan.kumar.gupta@...ux.intel.com>,
<thomas.lendacky@....com>, <perry.yuan@....com>, <seanjc@...gle.com>,
<kai.huang@...el.com>, <xiaoyao.li@...el.com>, <kan.liang@...ux.intel.com>,
<xin3.li@...el.com>, <ebiggers@...gle.com>, <xin@...or.com>,
<sohil.mehta@...el.com>, <andrew.cooper3@...rix.com>,
<mario.limonciello@....com>, <linux-doc@...r.kernel.org>,
<linux-kernel@...r.kernel.org>, <peternewman@...gle.com>,
<maciej.wieczor-retman@...el.com>, <eranian@...gle.com>,
<Xiaojian.Du@....com>, <gautham.shenoy@....com>
Subject: Re: [PATCH v13 17/27] x86/resctrl: Add the support for reading ABMC
counters
Hi Babu,
On 5/15/25 3:52 PM, Babu Moger wrote:
> Software can read the assignable counters using the QM_EVTSEL and QM_CTR
> register pair.
Please append with more context on how register pair is used to support the
changes in this patch.
>
> 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, 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.
Please rewrite above in imperative tone.
>
> Introduce __cntr_id_read_phys() to read the counter ID event data.
>
> Link: https://www.amd.com/content/dam/amd/en/documents/processor-tech-docs/programmer-references/40332.pdf
> Signed-off-by: Babu Moger <babu.moger@....com>
> ---
> v13: Split the patch into 2. First one to handle the passing of rdtgroup structure to few
> functions( __mon_event_count and mbm_update(). Second one to handle ABMC counter reading.
> Added new function __cntr_id_read_phys() to handle ABMC event reading.
> Updated kernel doc for resctrl_arch_reset_rmid() and resctrl_arch_rmid_read().
> Resolved conflicts caused by the recent FS/ARCH code restructure.
> The monitor.c file has now been split between the FS and ARCH directories.
>
> v12: New patch to support extended event mode when ABMC is enabled.
> ---
> arch/x86/kernel/cpu/resctrl/internal.h | 6 +++
> arch/x86/kernel/cpu/resctrl/monitor.c | 66 ++++++++++++++++++++++----
> fs/resctrl/monitor.c | 14 ++++--
> include/linux/resctrl.h | 9 ++--
> 4 files changed, 80 insertions(+), 15 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
> index db6b0c28ee6b..3b0cdb5520c7 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
>
> +/*
> + * ABMC Qos Event Identifiers.
QoS?
> + */
> +#define ABMC_EXTENDED_EVT_ID BIT(31)
> +#define ABMC_EVT_ID 1
Please use BIT(0) to be consistent.
> +
> /**
> * 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 e31084f7babd..36a03dae6d8e 100644
> --- a/arch/x86/kernel/cpu/resctrl/monitor.c
> +++ b/arch/x86/kernel/cpu/resctrl/monitor.c
> @@ -161,6 +161,41 @@ static int __rmid_read_phys(u32 prmid, enum resctrl_event_id eventid, u64 *val)
> return 0;
> }
>
> +static int __cntr_id_read_phys(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.
> + */
> + 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;
> +
> + *val = msr_val;
> + return 0;
> +}
> +
> static struct arch_mbm_state *get_arch_mbm_state(struct rdt_hw_mon_domain *hw_dom,
> u32 rmid,
> enum resctrl_event_id eventid)
> @@ -180,7 +215,7 @@ static struct arch_mbm_state *get_arch_mbm_state(struct rdt_hw_mon_domain *hw_do
> }
>
> void resctrl_arch_reset_rmid(struct rdt_resource *r, struct rdt_mon_domain *d,
> - u32 unused, u32 rmid,
> + 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);
> @@ -192,9 +227,16 @@ void resctrl_arch_reset_rmid(struct rdt_resource *r, struct rdt_mon_domain *d,
> if (am) {
> memset(am, 0, sizeof(*am));
>
> - prmid = logical_rmid_to_physical_rmid(cpu, rmid);
> - /* Record any initial, non-zero count value. */
> - __rmid_read_phys(prmid, eventid, &am->prev_msr);
> + if (resctrl_arch_mbm_cntr_assign_enabled(r) &&
> + resctrl_is_mbm_event(eventid)) {
> + if (cntr_id < 0)
This would be a bug, no? how about WARN_ON_ONCE()?
> + return;
> + __cntr_id_read_phys(cntr_id, &am->prev_msr);
> + } else {
> + prmid = logical_rmid_to_physical_rmid(cpu, rmid);
> + /* Record any initial, non-zero count value. */
> + __rmid_read_phys(prmid, eventid, &am->prev_msr);
> + }
> }
> }
>
> @@ -224,8 +266,8 @@ static u64 mbm_overflow_count(u64 prev_msr, u64 cur_msr, unsigned int width)
> }
>
> int resctrl_arch_rmid_read(struct rdt_resource *r, struct rdt_mon_domain *d,
> - u32 unused, u32 rmid, enum resctrl_event_id eventid,
> - u64 *val, void *ignored)
> + u32 unused, u32 rmid, int cntr_id,
> + enum resctrl_event_id eventid, u64 *val, void *ignored)
> {
> struct rdt_hw_mon_domain *hw_dom = resctrl_to_arch_mon_dom(d);
> struct rdt_hw_resource *hw_res = resctrl_to_arch_res(r);
> @@ -237,8 +279,16 @@ int resctrl_arch_rmid_read(struct rdt_resource *r, struct rdt_mon_domain *d,
>
> resctrl_arch_rmid_read_context_check();
>
> - prmid = logical_rmid_to_physical_rmid(cpu, rmid);
> - ret = __rmid_read_phys(prmid, eventid, &msr_val);
> + if (resctrl_arch_mbm_cntr_assign_enabled(r) &&
> + resctrl_is_mbm_event(eventid)) {
> + if (cntr_id < 0)
WARN_ON_ONCE()?
> + return cntr_id;
> + ret = __cntr_id_read_phys(cntr_id, &msr_val);
> + } else {
> + prmid = logical_rmid_to_physical_rmid(cpu, rmid);
> + ret = __rmid_read_phys(prmid, eventid, &msr_val);
> + }
> +
> if (ret)
> return ret;
>
> diff --git a/fs/resctrl/monitor.c b/fs/resctrl/monitor.c
> index a477be9cdb66..72f3dfb5b903 100644
> --- a/fs/resctrl/monitor.c
> +++ b/fs/resctrl/monitor.c
> @@ -159,7 +159,11 @@ void __check_limbo(struct rdt_mon_domain *d, bool force_free)
> break;
>
> entry = __rmid_entry(idx);
> - if (resctrl_arch_rmid_read(r, d, entry->closid, entry->rmid,
> + /*
> + * cntr_id is not relevant for QOS_L3_OCCUP_EVENT_ID.
> + * Pass dummy value -1.
> + */
> + if (resctrl_arch_rmid_read(r, d, entry->closid, entry->rmid, -1,
> QOS_L3_OCCUP_EVENT_ID, &val,
> arch_mon_ctx)) {
> rmid_dirty = true;
> @@ -359,6 +363,7 @@ static struct mbm_state *get_mbm_state(struct rdt_mon_domain *d, u32 closid,
>
> static int __mon_event_count(struct rdtgroup *rdtgrp, struct rmid_read *rr)
> {
> + int cntr_id = mbm_cntr_get(rr->r, rr->d, rdtgrp, rr->evtid);
So mbm_cntr_get() is called on *all* events (even non MBM) whether assignable counters
are supported or not. I assume it relies on num_mbm_cntrs to be zero on non-ABMC systems
but I think this needs to be explicit that mbm_cntr_get() returns -ENOENT in these cases.
Any developer attempting to modify mbm_cntr_get() needs to be aware of this usage.
This is quite subtle that resctrl_arch_reset_rmid() and resctrl_arch_rmid_read()
can be called with a negative counter ID. To help with code health this needs to
be highlighted (more later).
> int cpu = smp_processor_id();
> u32 closid = rdtgrp->closid;
> u32 rmid = rdtgrp->mon.rmid;
> @@ -368,7 +373,7 @@ static int __mon_event_count(struct rdtgroup *rdtgrp, struct rmid_read *rr)
> u64 tval = 0;
>
> if (rr->first) {
> - resctrl_arch_reset_rmid(rr->r, rr->d, closid, rmid, rr->evtid);
> + resctrl_arch_reset_rmid(rr->r, rr->d, closid, rmid, cntr_id, rr->evtid);
> m = get_mbm_state(rr->d, closid, rmid, rr->evtid);
> if (m)
> memset(m, 0, sizeof(struct mbm_state));
> @@ -379,7 +384,7 @@ static int __mon_event_count(struct rdtgroup *rdtgrp, struct rmid_read *rr)
> /* Reading a single domain, must be on a CPU in that domain. */
> if (!cpumask_test_cpu(cpu, &rr->d->hdr.cpu_mask))
> return -EINVAL;
> - rr->err = resctrl_arch_rmid_read(rr->r, rr->d, closid, rmid,
> + rr->err = resctrl_arch_rmid_read(rr->r, rr->d, closid, rmid, cntr_id,
> rr->evtid, &tval, rr->arch_mon_ctx);
> if (rr->err)
> return rr->err;
> @@ -404,7 +409,8 @@ static int __mon_event_count(struct rdtgroup *rdtgrp, struct rmid_read *rr)
> list_for_each_entry(d, &rr->r->mon_domains, hdr.list) {
> if (d->ci->id != rr->ci->id)
> continue;
> - err = resctrl_arch_rmid_read(rr->r, d, closid, rmid,
> + cntr_id = mbm_cntr_get(rr->r, d, rdtgrp, rr->evtid);
> + err = resctrl_arch_rmid_read(rr->r, d, closid, rmid, cntr_id,
> rr->evtid, &tval, rr->arch_mon_ctx);
> if (!err) {
> rr->val += tval;
> diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
> index f78b6064230c..cd24d1577e0a 100644
> --- a/include/linux/resctrl.h
> +++ b/include/linux/resctrl.h
> @@ -473,6 +473,7 @@ void resctrl_offline_cpu(unsigned int cpu);
> * counter may match traffic of both @closid and @rmid, or @rmid
> * only.
> * @rmid: rmid of the counter to read.
> + * @cntr_id: cntr_id to read MBM events with mbm_cntr_assign mode.
"Counter ID used to read MBM events in mbm_cntr_evt_assign mode. Only valid when
mbm_cntr_evt_assign mode is enabled and @eventid is an MBM event. Can be negative
when invalid." (Please feel free to improve)
> * @eventid: eventid to read, e.g. L3 occupancy.
> * @val: result of the counter read in bytes.
> * @arch_mon_ctx: An architecture specific value from
> @@ -490,8 +491,9 @@ void resctrl_offline_cpu(unsigned int cpu);
> * 0 on success, or -EIO, -EINVAL etc on error.
> */
> int resctrl_arch_rmid_read(struct rdt_resource *r, struct rdt_mon_domain *d,
> - u32 closid, u32 rmid, enum resctrl_event_id eventid,
> - u64 *val, void *arch_mon_ctx);
> + u32 closid, u32 rmid, int cntr_id,
> + enum resctrl_event_id eventid, u64 *val,
> + void *arch_mon_ctx);
>
> /**
> * resctrl_arch_rmid_read_context_check() - warn about invalid contexts
> @@ -532,12 +534,13 @@ struct rdt_domain_hdr *resctrl_find_domain(struct list_head *h, int id,
> * @closid: closid that matches the rmid. Depending on the architecture, the
> * counter may match traffic of both @closid and @rmid, or @rmid only.
> * @rmid: The rmid whose counter values should be reset.
> + * @cntr_id: The cntr_id to read MBM events with mbm_cntr_assign mode.
Same as above.
> * @eventid: The eventid whose counter values should be reset.
> *
> * This can be called from any CPU.
> */
> void resctrl_arch_reset_rmid(struct rdt_resource *r, struct rdt_mon_domain *d,
> - u32 closid, u32 rmid,
> + u32 closid, u32 rmid, int cntr_id,
> enum resctrl_event_id eventid);
>
> /**
Reinette
Powered by blists - more mailing lists