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

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

Hi Babu,

On 7/25/25 11:29 AM, Babu Moger wrote:
> System software can read resctrl event data for a particular resource by

"can read" -> "reads"

> 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

"can be read" -> "is read"

> setting the following fields: QM_EVTSEL.ExtendedEvtID = 1, QM_EVTSEL.EvtID
> = L3CacheABMC (=1) and setting [RMID] to the desired counter ID. Reading

"[RMID]" -> "QM_EVTSEL.RMID"

> QM_CTR will then return the contents of the specified counter ID. The

"will then return" -> "then returns"

> RMID_VAL_ERROR bit will be set if the counter configuration was invalid, or

"will be set" -> "is set"
"was invalid" -> "is invalid"

> if an invalid counter ID was set in the QM_EVTSEL[RMID] field. If the

"was set" -> "is set"

"in the QM_EVTSEL[RMID] field" -> "in QM_EVTSEL.RMID"


> counter data is currently unavailable, the RMID_VAL_UNAVAIL bit will be
> set.

"The RMID_VAL_UNAVAIL bit is set if the counter data is unavailable."

Please review after changes that all is coherent and in imperative tone and make
same adjustments to duplicate text in patch.

> 
> Introduce resctrl_arch_reset_cntr() and resctrl_arch_cntr_read() to reset
> and read event data for a specific counter.
> 
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---

...

> ---
>  arch/x86/kernel/cpu/resctrl/internal.h |  6 +++
>  arch/x86/kernel/cpu/resctrl/monitor.c  | 68 ++++++++++++++++++++++++++
>  2 files changed, 74 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 1f77fd58e707..57c8409a8247 100644
> --- a/arch/x86/kernel/cpu/resctrl/monitor.c
> +++ b/arch/x86/kernel/cpu/resctrl/monitor.c
> @@ -259,6 +259,74 @@ 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

ExtEvtID vs ExtendedEvtID ... either the definition or the text should change to
use same names.
Can description of RMID be expanded to note that it may
contain RMID or counter ID?

> +	 * 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 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. If the counter
> +	 * data is currently unavailable, the RMID_VAL_UNAVAIL bit will be set.
> +	 */
> +	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;
> +}
> +
> +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)
> +{
> +	u64 msr_val;
> +	int ret;
> +
> +	ret = __cntr_id_read(cntr_id, &msr_val);
> +	if (ret)
> +		return ret;
> +
> +	*val = get_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.

code looks good.

Reinette