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,
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,
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.

Signed-off-by: Babu Moger <babu.moger@amd.com>
---
v15: Updated patch to add arch calls resctrl_arch_cntr_read() and resctrl_arch_reset_cntr()
     with mbm_event mode.
     https://lore.kernel.org/lkml/b4b14670-9cb0-4f65-abd5-39db996e8da9@intel.com/

v14: Updated the context in changelog. Added text in imperative tone.
     Added WARN_ON_ONCE() when cntr_id < 0.
     Improved code documentation in include/linux/resctrl.h.
     Added the check in mbm_update() to skip overflow handler when counter is unassigned.

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  | 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.
+	 */
+	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, void *ignored)
+{
+	u64 msr_val;
+	int ret;
+
+	resctrl_arch_rmid_read_context_check();
+
+	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.
-- 
2.34.1

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:".

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

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

> 
> Signed-off-by: Babu Moger <babu.moger@amd.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)

> +	 */
> +	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?

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

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@amd.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

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?


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

Reinette

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