The event configuration is domain specific and initialized during domain
initialization. The values are stored in struct rdt_hw_mon_domain.

It is not required to read the configuration register every time user asks
for it. Use the value stored in struct rdt_hw_mon_domain instead.

Introduce resctrl_arch_mon_event_config_get() and
resctrl_arch_mon_event_config_set() to get/set architecture domain specific
mbm_total_cfg/mbm_local_cfg values.

Signed-off-by: Babu Moger <babu.moger@amd.com>
---
v11: Moved the mon_config_info structure definition to internal.h.
     Moved resctrl_arch_mon_event_config_get() and resctrl_arch_mon_event_config_set()
     to monitor.c file.
     Renamed local variable from val to config_val.

v10: Moved the mon_config_info structure definition to resctrl.h.

v9: Removed QOS_L3_OCCUP_EVENT_ID switch case in resctrl_arch_mon_event_config_set.
    Fixed a unnecessary space.

v8: Renamed
    resctrl_arch_event_config_get() to resctrl_arch_mon_event_config_get().
    resctrl_arch_event_config_set() to resctrl_arch_mon_event_config_set().

v7: Removed check if (val == INVALID_CONFIG_VALUE) as resctrl_arch_event_config_get
    already prints warning.
    Kept the Event config value definitions as is.

v6: Fixed inconstancy with types. Made all the types to u32 for config
    value.
    Removed few rdt_last_cmd_puts as it is not necessary.
    Removed unused config value definitions.
    Few more updates to commit message.

v5: Introduced resctrl_arch_event_config_get and
    resctrl_arch_event_config_get() based on our discussion.
    https://lore.kernel.org/lkml/68e861f9-245d-4496-a72e-46fc57d19c62@amd.com/

v4: New patch.
---
 arch/x86/kernel/cpu/resctrl/internal.h | 15 +++++++
 arch/x86/kernel/cpu/resctrl/monitor.c  | 46 +++++++++++++++++++
 arch/x86/kernel/cpu/resctrl/rdtgroup.c | 61 +++++---------------------
 3 files changed, 72 insertions(+), 50 deletions(-)

diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
index ab28b9340ee7..cfaea20145d0 100644
--- a/arch/x86/kernel/cpu/resctrl/internal.h
+++ b/arch/x86/kernel/cpu/resctrl/internal.h
@@ -605,6 +605,18 @@ union cpuid_0x10_x_edx {
 	unsigned int full;
 };
 
+/**
+ * struct mon_config_info - Monitoring event configuratiin details
+ * @d:			Domain for the event
+ * @evtid:		Event type
+ * @mon_config:		Event configuration value
+ */
+struct mon_config_info {
+	struct rdt_mon_domain *d;
+	enum resctrl_event_id evtid;
+	u32 mon_config;
+};
+
 void rdt_last_cmd_clear(void);
 void rdt_last_cmd_puts(const char *s);
 __printf(1, 2)
@@ -674,4 +686,7 @@ int resctrl_arch_mbm_cntr_assign_set(struct rdt_resource *r, bool enable);
 bool resctrl_arch_mbm_cntr_assign_enabled(struct rdt_resource *r);
 void arch_mbm_evt_config_init(struct rdt_hw_mon_domain *hw_dom);
 unsigned int mon_event_config_index_get(u32 evtid);
+void resctrl_arch_mon_event_config_set(void *info);
+u32 resctrl_arch_mon_event_config_get(struct rdt_mon_domain *d,
+				      enum resctrl_event_id eventid);
 #endif /* _ASM_X86_RESCTRL_INTERNAL_H */
diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
index 8917c7261680..6fe9e610e9a0 100644
--- a/arch/x86/kernel/cpu/resctrl/monitor.c
+++ b/arch/x86/kernel/cpu/resctrl/monitor.c
@@ -1324,3 +1324,49 @@ int resctrl_arch_mbm_cntr_assign_set(struct rdt_resource *r, bool enable)
 
 	return 0;
 }
+
+u32 resctrl_arch_mon_event_config_get(struct rdt_mon_domain *d,
+				      enum resctrl_event_id eventid)
+{
+	struct rdt_hw_mon_domain *hw_dom = resctrl_to_arch_mon_dom(d);
+
+	switch (eventid) {
+	case QOS_L3_OCCUP_EVENT_ID:
+		break;
+	case QOS_L3_MBM_TOTAL_EVENT_ID:
+		return hw_dom->mbm_total_cfg;
+	case QOS_L3_MBM_LOCAL_EVENT_ID:
+		return hw_dom->mbm_local_cfg;
+	}
+
+	/* Never expect to get here */
+	WARN_ON_ONCE(1);
+
+	return INVALID_CONFIG_VALUE;
+}
+
+void resctrl_arch_mon_event_config_set(void *info)
+{
+	struct mon_config_info *mon_info = info;
+	struct rdt_hw_mon_domain *hw_dom;
+	unsigned int index;
+
+	index = mon_event_config_index_get(mon_info->evtid);
+	if (index == INVALID_CONFIG_INDEX)
+		return;
+
+	wrmsr(MSR_IA32_EVT_CFG_BASE + index, mon_info->mon_config, 0);
+
+	hw_dom = resctrl_to_arch_mon_dom(mon_info->d);
+
+	switch (mon_info->evtid) {
+	case QOS_L3_MBM_TOTAL_EVENT_ID:
+		hw_dom->mbm_total_cfg = mon_info->mon_config;
+		break;
+	case QOS_L3_MBM_LOCAL_EVENT_ID:
+		hw_dom->mbm_local_cfg = mon_info->mon_config;
+		break;
+	default:
+		break;
+	}
+}
diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index ddecaa51cec4..18110a1afb6d 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -1586,11 +1586,6 @@ static int rdtgroup_size_show(struct kernfs_open_file *of,
 	return ret;
 }
 
-struct mon_config_info {
-	u32 evtid;
-	u32 mon_config;
-};
-
 /**
  * mon_event_config_index_get - get the hardware index for the
  *                              configurable event
@@ -1613,33 +1608,11 @@ unsigned int mon_event_config_index_get(u32 evtid)
 	}
 }
 
-static void mon_event_config_read(void *info)
-{
-	struct mon_config_info *mon_info = info;
-	unsigned int index;
-	u64 msrval;
-
-	index = mon_event_config_index_get(mon_info->evtid);
-	if (index == INVALID_CONFIG_INDEX) {
-		pr_warn_once("Invalid event id %d\n", mon_info->evtid);
-		return;
-	}
-	rdmsrl(MSR_IA32_EVT_CFG_BASE + index, msrval);
-
-	/* Report only the valid event configuration bits */
-	mon_info->mon_config = msrval & MAX_EVT_CONFIG_BITS;
-}
-
-static void mondata_config_read(struct rdt_mon_domain *d, struct mon_config_info *mon_info)
-{
-	smp_call_function_any(&d->hdr.cpu_mask, mon_event_config_read, mon_info, 1);
-}
-
 static int mbm_config_show(struct seq_file *s, struct rdt_resource *r, u32 evtid)
 {
-	struct mon_config_info mon_info;
 	struct rdt_mon_domain *dom;
 	bool sep = false;
+	u32 config_val;
 
 	cpus_read_lock();
 	mutex_lock(&rdtgroup_mutex);
@@ -1648,11 +1621,8 @@ static int mbm_config_show(struct seq_file *s, struct rdt_resource *r, u32 evtid
 		if (sep)
 			seq_puts(s, ";");
 
-		memset(&mon_info, 0, sizeof(struct mon_config_info));
-		mon_info.evtid = evtid;
-		mondata_config_read(dom, &mon_info);
-
-		seq_printf(s, "%d=0x%02x", dom->hdr.id, mon_info.mon_config);
+		config_val = resctrl_arch_mon_event_config_get(dom, evtid);
+		seq_printf(s, "%d=0x%02x", dom->hdr.id, config_val);
 		sep = true;
 	}
 	seq_puts(s, "\n");
@@ -1683,33 +1653,23 @@ static int mbm_local_bytes_config_show(struct kernfs_open_file *of,
 	return 0;
 }
 
-static void mon_event_config_write(void *info)
-{
-	struct mon_config_info *mon_info = info;
-	unsigned int index;
-
-	index = mon_event_config_index_get(mon_info->evtid);
-	if (index == INVALID_CONFIG_INDEX) {
-		pr_warn_once("Invalid event id %d\n", mon_info->evtid);
-		return;
-	}
-	wrmsr(MSR_IA32_EVT_CFG_BASE + index, mon_info->mon_config, 0);
-}
 
 static void mbm_config_write_domain(struct rdt_resource *r,
 				    struct rdt_mon_domain *d, u32 evtid, u32 val)
 {
 	struct mon_config_info mon_info = {0};
+	u32 config_val;
 
 	/*
-	 * Read the current config value first. If both are the same then
+	 * Check the current config value first. If both are the same then
 	 * no need to write it again.
 	 */
-	mon_info.evtid = evtid;
-	mondata_config_read(d, &mon_info);
-	if (mon_info.mon_config == val)
+	config_val = resctrl_arch_mon_event_config_get(d, evtid);
+	if (config_val == INVALID_CONFIG_VALUE || config_val == val)
 		return;
 
+	mon_info.d = d;
+	mon_info.evtid = evtid;
 	mon_info.mon_config = val;
 
 	/*
@@ -1718,7 +1678,8 @@ static void mbm_config_write_domain(struct rdt_resource *r,
 	 * are scoped at the domain level. Writing any of these MSRs
 	 * on one CPU is observed by all the CPUs in the domain.
 	 */
-	smp_call_function_any(&d->hdr.cpu_mask, mon_event_config_write,
+	smp_call_function_any(&d->hdr.cpu_mask,
+			      resctrl_arch_mon_event_config_set,
 			      &mon_info, 1);
 
 	/*
-- 
2.34.1

On 1/22/2025 12:20 PM, Babu Moger wrote:
> diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
> index 8917c7261680..6fe9e610e9a0 100644
> --- a/arch/x86/kernel/cpu/resctrl/monitor.c
> +++ b/arch/x86/kernel/cpu/resctrl/monitor.c
> @@ -1324,3 +1324,49 @@ int resctrl_arch_mbm_cntr_assign_set(struct rdt_resource *r, bool enable)
>   
>   	return 0;
>   }
> +
> +u32 resctrl_arch_mon_event_config_get(struct rdt_mon_domain *d,
> +				      enum resctrl_event_id eventid)
> +{
> +	struct rdt_hw_mon_domain *hw_dom = resctrl_to_arch_mon_dom(d);
> +
> +	switch (eventid) {
> +	case QOS_L3_OCCUP_EVENT_ID:
> +		break;
> +	case QOS_L3_MBM_TOTAL_EVENT_ID:
> +		return hw_dom->mbm_total_cfg;
> +	case QOS_L3_MBM_LOCAL_EVENT_ID:
> +		return hw_dom->mbm_local_cfg;
> +	}
> +
> +	/* Never expect to get here */
> +	WARN_ON_ONCE(1);
> +
> +	return INVALID_CONFIG_VALUE;
> +}
> +
> +void resctrl_arch_mon_event_config_set(void *info)
> +{
> +	struct mon_config_info *mon_info = info;
> +	struct rdt_hw_mon_domain *hw_dom;
> +	unsigned int index;
> +
> +	index = mon_event_config_index_get(mon_info->evtid);
> +	if (index == INVALID_CONFIG_INDEX)
> +		return;
> +
> +	wrmsr(MSR_IA32_EVT_CFG_BASE + index, mon_info->mon_config, 0);

This is the existing code, however it would be better to use wrmsrl()
when the higher 32-bit are all 0s:

	wrmsrl(MSR_IA32_EVT_CFG_BASE + index, mon_info->mon_config);

Thanks!
     Xin

Hi Xin,

On 2/5/25 10:24 PM, Xin Li wrote:
> On 1/22/2025 12:20 PM, Babu Moger wrote:
>> diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
>> index 8917c7261680..6fe9e610e9a0 100644
>> --- a/arch/x86/kernel/cpu/resctrl/monitor.c
>> +++ b/arch/x86/kernel/cpu/resctrl/monitor.c
>> @@ -1324,3 +1324,49 @@ int resctrl_arch_mbm_cntr_assign_set(struct rdt_resource *r, bool enable)
>>         return 0;
>>   }
>> +
>> +u32 resctrl_arch_mon_event_config_get(struct rdt_mon_domain *d,
>> +                      enum resctrl_event_id eventid)
>> +{
>> +    struct rdt_hw_mon_domain *hw_dom = resctrl_to_arch_mon_dom(d);
>> +
>> +    switch (eventid) {
>> +    case QOS_L3_OCCUP_EVENT_ID:
>> +        break;
>> +    case QOS_L3_MBM_TOTAL_EVENT_ID:
>> +        return hw_dom->mbm_total_cfg;
>> +    case QOS_L3_MBM_LOCAL_EVENT_ID:
>> +        return hw_dom->mbm_local_cfg;
>> +    }
>> +
>> +    /* Never expect to get here */
>> +    WARN_ON_ONCE(1);
>> +
>> +    return INVALID_CONFIG_VALUE;
>> +}
>> +
>> +void resctrl_arch_mon_event_config_set(void *info)
>> +{
>> +    struct mon_config_info *mon_info = info;
>> +    struct rdt_hw_mon_domain *hw_dom;
>> +    unsigned int index;
>> +
>> +    index = mon_event_config_index_get(mon_info->evtid);
>> +    if (index == INVALID_CONFIG_INDEX)
>> +        return;
>> +
>> +    wrmsr(MSR_IA32_EVT_CFG_BASE + index, mon_info->mon_config, 0);
> 
> This is the existing code, however it would be better to use wrmsrl()
> when the higher 32-bit are all 0s:
> 
>     wrmsrl(MSR_IA32_EVT_CFG_BASE + index, mon_info->mon_config);
> 

Could you please elaborate what makes this change better?

Thank you!

Reinette

On 2/6/2025 8:17 AM, Reinette Chatre wrote:
>>> +    wrmsr(MSR_IA32_EVT_CFG_BASE + index, mon_info->mon_config, 0);
>> This is the existing code, however it would be better to use wrmsrl()
>> when the higher 32-bit are all 0s:
>>
>>      wrmsrl(MSR_IA32_EVT_CFG_BASE + index, mon_info->mon_config);
>>
> Could you please elaborate what makes this change better?

In short, it takes one less argument, and doesn't pass an argument of 0.

The longer story is that hpa and I are refactoring the MSR access APIs
to accommodate the immediate form of MSR access instructions.  And we
are not happy about that there are too many MSR access APIs and their
uses are *random*.  The native wrmsr() and wrmsrl() are essentially the 
same and the only difference is that wrmsr() passes a 64-bit value to be
written into a MSR in *2* u32 arguments.  But we already have struct msr
defined in asm/shared/msr.h as:
	struct msr {
         	union {
                 	struct {
                         	u32 l;
	                        u32 h;
         	        };
                 	u64 q;
	        };
	};

it's more natural to do the same job with this data structure in most
cases.  And we want to remove wrmsr() and only keep wrmsrl(), thus a
developer won't have to figure out which one is better to use :-P.

For that to happen, one cleanup is to replace wrmsr(msr, low, 0) with
wrmsrl(msr, low) (low is automatically converted to u64 from u32).

However, I'm fine if Babu wants to keep it as-is.

Thanks!
     Xin

Hi Xin,

On 2/7/25 04:07, Xin Li wrote:
> On 2/6/2025 8:17 AM, Reinette Chatre wrote:
>>>> +    wrmsr(MSR_IA32_EVT_CFG_BASE + index, mon_info->mon_config, 0);
>>> This is the existing code, however it would be better to use wrmsrl()
>>> when the higher 32-bit are all 0s:
>>>
>>>      wrmsrl(MSR_IA32_EVT_CFG_BASE + index, mon_info->mon_config);
>>>
>> Could you please elaborate what makes this change better?
> 
> In short, it takes one less argument, and doesn't pass an argument of 0.
> 
> The longer story is that hpa and I are refactoring the MSR access APIs
> to accommodate the immediate form of MSR access instructions.  And we
> are not happy about that there are too many MSR access APIs and their
> uses are *random*.  The native wrmsr() and wrmsrl() are essentially the
> same and the only difference is that wrmsr() passes a 64-bit value to be
> written into a MSR in *2* u32 arguments.  But we already have struct msr
> defined in asm/shared/msr.h as:
>     struct msr {
>             union {
>                     struct {
>                             u32 l;
>                             u32 h;
>                     };
>                     u64 q;
>             };
>     };
> 
> it's more natural to do the same job with this data structure in most
> cases.  And we want to remove wrmsr() and only keep wrmsrl(), thus a
> developer won't have to figure out which one is better to use :-P.
> 
> For that to happen, one cleanup is to replace wrmsr(msr, low, 0) with
> wrmsrl(msr, low) (low is automatically converted to u64 from u32).
> 
> However, I'm fine if Babu wants to keep it as-is.

Thanks for the explanation.  Changed it to use wrmsrl().

-- 
Thanks
Babu Moger

On 2/11/2025 11:44 AM, Moger, Babu wrote:
> Hi Xin,
> 
> On 2/7/25 04:07, Xin Li wrote:
>> On 2/6/2025 8:17 AM, Reinette Chatre wrote:
>>>>> +    wrmsr(MSR_IA32_EVT_CFG_BASE + index, mon_info->mon_config, 0);
>>>> This is the existing code, however it would be better to use wrmsrl()
>>>> when the higher 32-bit are all 0s:
>>>>
>>>>       wrmsrl(MSR_IA32_EVT_CFG_BASE + index, mon_info->mon_config);
>>>>
>>> Could you please elaborate what makes this change better?
>>
>> In short, it takes one less argument, and doesn't pass an argument of 0.
>>
>> The longer story is that hpa and I are refactoring the MSR access APIs
>> to accommodate the immediate form of MSR access instructions.  And we
>> are not happy about that there are too many MSR access APIs and their
>> uses are *random*.  The native wrmsr() and wrmsrl() are essentially the
>> same and the only difference is that wrmsr() passes a 64-bit value to be
>> written into a MSR in *2* u32 arguments.  But we already have struct msr
>> defined in asm/shared/msr.h as:
>>      struct msr {
>>              union {
>>                      struct {
>>                              u32 l;
>>                              u32 h;
>>                      };
>>                      u64 q;
>>              };
>>      };
>>
>> it's more natural to do the same job with this data structure in most
>> cases.  And we want to remove wrmsr() and only keep wrmsrl(), thus a
>> developer won't have to figure out which one is better to use :-P.
>>
>> For that to happen, one cleanup is to replace wrmsr(msr, low, 0) with
>> wrmsrl(msr, low) (low is automatically converted to u64 from u32).
>>
>> However, I'm fine if Babu wants to keep it as-is.
> 
> Thanks for the explanation.  Changed it to use wrmsrl().
> 

You're welcome.  And thanks for making the change.

Hi Babu,

On 1/22/25 12:20 PM, Babu Moger wrote:
> The event configuration is domain specific and initialized during domain
> initialization. The values are stored in struct rdt_hw_mon_domain.
> 
> It is not required to read the configuration register every time user asks
> for it. Use the value stored in struct rdt_hw_mon_domain instead.
> 
> Introduce resctrl_arch_mon_event_config_get() and
> resctrl_arch_mon_event_config_set() to get/set architecture domain specific
> mbm_total_cfg/mbm_local_cfg values.
> 
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---

> ---
>  arch/x86/kernel/cpu/resctrl/internal.h | 15 +++++++
>  arch/x86/kernel/cpu/resctrl/monitor.c  | 46 +++++++++++++++++++
>  arch/x86/kernel/cpu/resctrl/rdtgroup.c | 61 +++++---------------------
>  3 files changed, 72 insertions(+), 50 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
> index ab28b9340ee7..cfaea20145d0 100644
> --- a/arch/x86/kernel/cpu/resctrl/internal.h
> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
> @@ -605,6 +605,18 @@ union cpuid_0x10_x_edx {
>  	unsigned int full;
>  };
>  
> +/**
> + * struct mon_config_info - Monitoring event configuratiin details

Same typo as previous version. 

> + * @d:			Domain for the event
> + * @evtid:		Event type
> + * @mon_config:		Event configuration value
> + */
> +struct mon_config_info {
> +	struct rdt_mon_domain *d;
> +	enum resctrl_event_id evtid;
> +	u32 mon_config;
> +};
> +
>  void rdt_last_cmd_clear(void);
>  void rdt_last_cmd_puts(const char *s);
>  __printf(1, 2)
> @@ -674,4 +686,7 @@ int resctrl_arch_mbm_cntr_assign_set(struct rdt_resource *r, bool enable);
>  bool resctrl_arch_mbm_cntr_assign_enabled(struct rdt_resource *r);
>  void arch_mbm_evt_config_init(struct rdt_hw_mon_domain *hw_dom);
>  unsigned int mon_event_config_index_get(u32 evtid);
> +void resctrl_arch_mon_event_config_set(void *info);
> +u32 resctrl_arch_mon_event_config_get(struct rdt_mon_domain *d,
> +				      enum resctrl_event_id eventid);
>  #endif /* _ASM_X86_RESCTRL_INTERNAL_H */
> diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
> index 8917c7261680..6fe9e610e9a0 100644
> --- a/arch/x86/kernel/cpu/resctrl/monitor.c
> +++ b/arch/x86/kernel/cpu/resctrl/monitor.c
> @@ -1324,3 +1324,49 @@ int resctrl_arch_mbm_cntr_assign_set(struct rdt_resource *r, bool enable)
>  
>  	return 0;
>  }
> +
> +u32 resctrl_arch_mon_event_config_get(struct rdt_mon_domain *d,
> +				      enum resctrl_event_id eventid)
> +{
> +	struct rdt_hw_mon_domain *hw_dom = resctrl_to_arch_mon_dom(d);
> +
> +	switch (eventid) {
> +	case QOS_L3_OCCUP_EVENT_ID:
> +		break;
> +	case QOS_L3_MBM_TOTAL_EVENT_ID:
> +		return hw_dom->mbm_total_cfg;
> +	case QOS_L3_MBM_LOCAL_EVENT_ID:
> +		return hw_dom->mbm_local_cfg;
> +	}
> +
> +	/* Never expect to get here */
> +	WARN_ON_ONCE(1);
> +
> +	return INVALID_CONFIG_VALUE;
> +}
> +
> +void resctrl_arch_mon_event_config_set(void *info)
> +{
> +	struct mon_config_info *mon_info = info;
> +	struct rdt_hw_mon_domain *hw_dom;
> +	unsigned int index;
> +
> +	index = mon_event_config_index_get(mon_info->evtid);
> +	if (index == INVALID_CONFIG_INDEX)
> +		return;
> +
> +	wrmsr(MSR_IA32_EVT_CFG_BASE + index, mon_info->mon_config, 0);
> +
> +	hw_dom = resctrl_to_arch_mon_dom(mon_info->d);
> +
> +	switch (mon_info->evtid) {
> +	case QOS_L3_MBM_TOTAL_EVENT_ID:
> +		hw_dom->mbm_total_cfg = mon_info->mon_config;
> +		break;
> +	case QOS_L3_MBM_LOCAL_EVENT_ID:
> +		hw_dom->mbm_local_cfg = mon_info->mon_config;
> +		break;
> +	default:
> +		break;
> +	}
> +}

This new arch API has sharp corners because of asymmetry of where resctrl
runs the arch function. I do not think it is required to change this since we
can only speculate about how this may be used in the future but I do think
it will be helpful to add comments that highlight:

resctrl_arch_mon_event_config_get() ->  May run on CPU that does not belong to domain.
resctrl_arch_mon_event_config_set() ->  Runs on CPU that belongs to domain.

... 

> @@ -1683,33 +1653,23 @@ static int mbm_local_bytes_config_show(struct kernfs_open_file *of,
>  	return 0;
>  }
>  
> -static void mon_event_config_write(void *info)
> -{
> -	struct mon_config_info *mon_info = info;
> -	unsigned int index;
> -
> -	index = mon_event_config_index_get(mon_info->evtid);
> -	if (index == INVALID_CONFIG_INDEX) {
> -		pr_warn_once("Invalid event id %d\n", mon_info->evtid);
> -		return;
> -	}
> -	wrmsr(MSR_IA32_EVT_CFG_BASE + index, mon_info->mon_config, 0);
> -}
>  
>  static void mbm_config_write_domain(struct rdt_resource *r,
>  				    struct rdt_mon_domain *d, u32 evtid, u32 val)
>  {
>  	struct mon_config_info mon_info = {0};

As discussed in previous version it is unnecessary to explicitly initialize
the structure if it is fully initialized in the code. This avoids need for
future cleanups like commit 29eaa7958367 ("x86/resctrl: Slightly clean-up mbm_config_show()")

Reinette

Hi Reinette,

On 2/5/2025 5:58 PM, Reinette Chatre wrote:
> Hi Babu,
> 
> On 1/22/25 12:20 PM, Babu Moger wrote:
>> The event configuration is domain specific and initialized during domain
>> initialization. The values are stored in struct rdt_hw_mon_domain.
>>
>> It is not required to read the configuration register every time user asks
>> for it. Use the value stored in struct rdt_hw_mon_domain instead.
>>
>> Introduce resctrl_arch_mon_event_config_get() and
>> resctrl_arch_mon_event_config_set() to get/set architecture domain specific
>> mbm_total_cfg/mbm_local_cfg values.
>>
>> Signed-off-by: Babu Moger <babu.moger@amd.com>
>> ---
> 
>> ---
>>   arch/x86/kernel/cpu/resctrl/internal.h | 15 +++++++
>>   arch/x86/kernel/cpu/resctrl/monitor.c  | 46 +++++++++++++++++++
>>   arch/x86/kernel/cpu/resctrl/rdtgroup.c | 61 +++++---------------------
>>   3 files changed, 72 insertions(+), 50 deletions(-)
>>
>> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
>> index ab28b9340ee7..cfaea20145d0 100644
>> --- a/arch/x86/kernel/cpu/resctrl/internal.h
>> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
>> @@ -605,6 +605,18 @@ union cpuid_0x10_x_edx {
>>   	unsigned int full;
>>   };
>>   
>> +/**
>> + * struct mon_config_info - Monitoring event configuratiin details
> 
> Same typo as previous version.

I am really sorry about this.

> 
>> + * @d:			Domain for the event
>> + * @evtid:		Event type
>> + * @mon_config:		Event configuration value
>> + */
>> +struct mon_config_info {
>> +	struct rdt_mon_domain *d;
>> +	enum resctrl_event_id evtid;
>> +	u32 mon_config;
>> +};
>> +
>>   void rdt_last_cmd_clear(void);
>>   void rdt_last_cmd_puts(const char *s);
>>   __printf(1, 2)
>> @@ -674,4 +686,7 @@ int resctrl_arch_mbm_cntr_assign_set(struct rdt_resource *r, bool enable);
>>   bool resctrl_arch_mbm_cntr_assign_enabled(struct rdt_resource *r);
>>   void arch_mbm_evt_config_init(struct rdt_hw_mon_domain *hw_dom);
>>   unsigned int mon_event_config_index_get(u32 evtid);
>> +void resctrl_arch_mon_event_config_set(void *info);
>> +u32 resctrl_arch_mon_event_config_get(struct rdt_mon_domain *d,
>> +				      enum resctrl_event_id eventid);
>>   #endif /* _ASM_X86_RESCTRL_INTERNAL_H */
>> diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
>> index 8917c7261680..6fe9e610e9a0 100644
>> --- a/arch/x86/kernel/cpu/resctrl/monitor.c
>> +++ b/arch/x86/kernel/cpu/resctrl/monitor.c
>> @@ -1324,3 +1324,49 @@ int resctrl_arch_mbm_cntr_assign_set(struct rdt_resource *r, bool enable)
>>   
>>   	return 0;
>>   }
>> +
>> +u32 resctrl_arch_mon_event_config_get(struct rdt_mon_domain *d,
>> +				      enum resctrl_event_id eventid)
>> +{
>> +	struct rdt_hw_mon_domain *hw_dom = resctrl_to_arch_mon_dom(d);
>> +
>> +	switch (eventid) {
>> +	case QOS_L3_OCCUP_EVENT_ID:
>> +		break;
>> +	case QOS_L3_MBM_TOTAL_EVENT_ID:
>> +		return hw_dom->mbm_total_cfg;
>> +	case QOS_L3_MBM_LOCAL_EVENT_ID:
>> +		return hw_dom->mbm_local_cfg;
>> +	}
>> +
>> +	/* Never expect to get here */
>> +	WARN_ON_ONCE(1);
>> +
>> +	return INVALID_CONFIG_VALUE;
>> +}
>> +
>> +void resctrl_arch_mon_event_config_set(void *info)
>> +{
>> +	struct mon_config_info *mon_info = info;
>> +	struct rdt_hw_mon_domain *hw_dom;
>> +	unsigned int index;
>> +
>> +	index = mon_event_config_index_get(mon_info->evtid);
>> +	if (index == INVALID_CONFIG_INDEX)
>> +		return;
>> +
>> +	wrmsr(MSR_IA32_EVT_CFG_BASE + index, mon_info->mon_config, 0);
>> +
>> +	hw_dom = resctrl_to_arch_mon_dom(mon_info->d);
>> +
>> +	switch (mon_info->evtid) {
>> +	case QOS_L3_MBM_TOTAL_EVENT_ID:
>> +		hw_dom->mbm_total_cfg = mon_info->mon_config;
>> +		break;
>> +	case QOS_L3_MBM_LOCAL_EVENT_ID:
>> +		hw_dom->mbm_local_cfg = mon_info->mon_config;
>> +		break;
>> +	default:
>> +		break;
>> +	}
>> +}
> 
> This new arch API has sharp corners because of asymmetry of where resctrl
> runs the arch function. I do not think it is required to change this since we
> can only speculate about how this may be used in the future but I do think
> it will be helpful to add comments that highlight:
> 
> resctrl_arch_mon_event_config_get() ->  May run on CPU that does not belong to domain.
> resctrl_arch_mon_event_config_set() ->  Runs on CPU that belongs to domain.

Sure. will do.

> 
> ...
> 
>> @@ -1683,33 +1653,23 @@ static int mbm_local_bytes_config_show(struct kernfs_open_file *of,
>>   	return 0;
>>   }
>>   
>> -static void mon_event_config_write(void *info)
>> -{
>> -	struct mon_config_info *mon_info = info;
>> -	unsigned int index;
>> -
>> -	index = mon_event_config_index_get(mon_info->evtid);
>> -	if (index == INVALID_CONFIG_INDEX) {
>> -		pr_warn_once("Invalid event id %d\n", mon_info->evtid);
>> -		return;
>> -	}
>> -	wrmsr(MSR_IA32_EVT_CFG_BASE + index, mon_info->mon_config, 0);
>> -}
>>   
>>   static void mbm_config_write_domain(struct rdt_resource *r,
>>   				    struct rdt_mon_domain *d, u32 evtid, u32 val)
>>   {
>>   	struct mon_config_info mon_info = {0};
> 
> As discussed in previous version it is unnecessary to explicitly initialize
> the structure if it is fully initialized in the code. This avoids need for
> future cleanups like commit 29eaa7958367 ("x86/resctrl: Slightly clean-up mbm_config_show()")

Yes. Need to change it.

thanks
Babu

> This new arch API has sharp corners because of asymmetry of where resctrl
> runs the arch function. I do not think it is required to change this since we
> can only speculate about how this may be used in the future but I do think
> it will be helpful to add comments that highlight:
>
> resctrl_arch_mon_event_config_get() ->  May run on CPU that does not belong to domain.
> resctrl_arch_mon_event_config_set() ->  Runs on CPU that belongs to domain.

Here's a vague data point about the future to help with speculation.

I have something coming along the pipeline that also can run on any CPU.

I am contemplating a flag in the rdt_resource structure (in appropriate substructure
resctrl_cache/resctrl_membw) to indicate "domain" vs. "any" for operations.

Would something like that be useful here?

-Tony

Hi Tony,

On 2/5/25 4:51 PM, Luck, Tony wrote:
>> This new arch API has sharp corners because of asymmetry of where resctrl
>> runs the arch function. I do not think it is required to change this since we
>> can only speculate about how this may be used in the future but I do think
>> it will be helpful to add comments that highlight:
>>
>> resctrl_arch_mon_event_config_get() ->  May run on CPU that does not belong to domain.
>> resctrl_arch_mon_event_config_set() ->  Runs on CPU that belongs to domain.
> 
> Here's a vague data point about the future to help with speculation.
> 
> I have something coming along the pipeline that also can run on any CPU.
> 
> I am contemplating a flag in the rdt_resource structure (in appropriate substructure
> resctrl_cache/resctrl_membw) to indicate "domain" vs. "any" for operations.
> 
> Would something like that be useful here?

hmm ... I cannot envision how this may look. Could you please elaborate?

You mention "a" (singular) flag in rdt_resource while this scenario involves
different ops having different scope. This makes me think that this flag may
have to be per operation that in turn would need additional infrastructure to
manage and track operations.

These "arch" functions are evolving as the work to support MPAM is done and
so far I think it has been quite ad-hoc to just refactor arch specific code
into "arch" helpers instead of keeping track of which scope they are running in.
This currently requires any arch implementing an "arch" helper to be well aware 
of how resctrl will call it.

Reinette

On Wed, Feb 05, 2025 at 05:41:53PM -0800, Reinette Chatre wrote:
> Hi Tony,
> 
> On 2/5/25 4:51 PM, Luck, Tony wrote:
> >> This new arch API has sharp corners because of asymmetry of where resctrl
> >> runs the arch function. I do not think it is required to change this since we
> >> can only speculate about how this may be used in the future but I do think
> >> it will be helpful to add comments that highlight:
> >>
> >> resctrl_arch_mon_event_config_get() ->  May run on CPU that does not belong to domain.
> >> resctrl_arch_mon_event_config_set() ->  Runs on CPU that belongs to domain.
> > 
> > Here's a vague data point about the future to help with speculation.
> > 
> > I have something coming along the pipeline that also can run on any CPU.
> > 
> > I am contemplating a flag in the rdt_resource structure (in appropriate substructure
> > resctrl_cache/resctrl_membw) to indicate "domain" vs. "any" for operations.
> > 
> > Would something like that be useful here?
> 
> hmm ... I cannot envision how this may look. Could you please elaborate?
> 
> You mention "a" (singular) flag in rdt_resource while this scenario involves
> different ops having different scope. This makes me think that this flag may
> have to be per operation that in turn would need additional infrastructure to
> manage and track operations.
> 
> These "arch" functions are evolving as the work to support MPAM is done and
> so far I think it has been quite ad-hoc to just refactor arch specific code
> into "arch" helpers instead of keeping track of which scope they are running in.
> This currently requires any arch implementing an "arch" helper to be well aware 
> of how resctrl will call it.
> 
> Reinette

For MPAM, we must typically do all configuration access from a CPU in a
power domain that depends on the power domain of the relevant MPAM MSC,
including reads of the configuration.

In the MPAM case, the required topology knowledge is not necessarily
identical to the resctrl domain topology, so it doesn't feel right to
have the resctrl core code making the decisions.

So, in the interest of keeping the arch interface simple, should cross-
calling be delegated to the arch code, at least for now?

Cheers
---Dave

Hi Dave,

On 2/19/25 13:28, Dave Martin wrote:
> On Wed, Feb 05, 2025 at 05:41:53PM -0800, Reinette Chatre wrote:
>> On 2/5/25 4:51 PM, Luck, Tony wrote:
>>>> This new arch API has sharp corners because of asymmetry of where resctrl
>>>> runs the arch function. I do not think it is required to change this since we
>>>> can only speculate about how this may be used in the future but I do think
>>>> it will be helpful to add comments that highlight:
>>>>
>>>> resctrl_arch_mon_event_config_get() ->  May run on CPU that does not belong to domain.
>>>> resctrl_arch_mon_event_config_set() ->  Runs on CPU that belongs to domain.
>>>
>>> Here's a vague data point about the future to help with speculation.
>>>
>>> I have something coming along the pipeline that also can run on any CPU.
>>>
>>> I am contemplating a flag in the rdt_resource structure (in appropriate substructure
>>> resctrl_cache/resctrl_membw) to indicate "domain" vs. "any" for operations.
>>>
>>> Would something like that be useful here?
>>
>> hmm ... I cannot envision how this may look. Could you please elaborate?
>>
>> You mention "a" (singular) flag in rdt_resource while this scenario involves
>> different ops having different scope. This makes me think that this flag may
>> have to be per operation that in turn would need additional infrastructure to
>> manage and track operations.
>>
>> These "arch" functions are evolving as the work to support MPAM is done and
>> so far I think it has been quite ad-hoc to just refactor arch specific code
>> into "arch" helpers instead of keeping track of which scope they are running in.
>> This currently requires any arch implementing an "arch" helper to be well aware
>> of how resctrl will call it.

> For MPAM, we must typically do all configuration access from a CPU in a
> power domain that depends on the power domain of the relevant MPAM MSC,
> including reads of the configuration.
This is the worst case - but the firmware can describe an MSC as being globally accessible.


> In the MPAM case, the required topology knowledge is not necessarily
> identical to the resctrl domain topology, so it doesn't feel right to
> have the resctrl core code making the decisions.

This is up to the MPAM driver to hide. The upshot is that for some calls
resctrl needs to schedule work so that the MPAM driver can in turn IPI to get
the rest of the work done. The changes for this are already upstream.


> So, in the interest of keeping the arch interface simple, should cross-
> calling be delegated to the arch code, at least for now?


That's invasive on top of what we already have. Take smp_mon_event_count() as an
example, there is a bunch of work that resctrl can do on a CPU that can access
the hardware registers. If the cpumask was hidden, then we'd either need more IPI,
or to allocate a structure that can describe a long list of monitors to read.



Thanks,

James

> >> This new arch API has sharp corners because of asymmetry of where resctrl
> >> runs the arch function. I do not think it is required to change this since we
> >> can only speculate about how this may be used in the future but I do think
> >> it will be helpful to add comments that highlight:
> >>
> >> resctrl_arch_mon_event_config_get() ->  May run on CPU that does not belong to domain.
> >> resctrl_arch_mon_event_config_set() ->  Runs on CPU that belongs to domain.
> >
> > Here's a vague data point about the future to help with speculation.
> >
> > I have something coming along the pipeline that also can run on any CPU.
> >
> > I am contemplating a flag in the rdt_resource structure (in appropriate substructure
> > resctrl_cache/resctrl_membw) to indicate "domain" vs. "any" for operations.
> >
> > Would something like that be useful here?
>
> hmm ... I cannot envision how this may look. Could you please elaborate?
>
> You mention "a" (singular) flag in rdt_resource while this scenario involves
> different ops having different scope. This makes me think that this flag may
> have to be per operation that in turn would need additional infrastructure to
> manage and track operations.
>
> These "arch" functions are evolving as the work to support MPAM is done and
> so far I think it has been quite ad-hoc to just refactor arch specific code
> into "arch" helpers instead of keeping track of which scope they are running in.
> This currently requires any arch implementing an "arch" helper to be well aware
> of how resctrl will call it.

Reinette,

I haven't fleshed it out yet. One option would be to have a "choose_cpu_mask()"
function that takes resource and domain parameters (and given your comment
about this case an operation code). Then use that as the mask in an smp_call*().

Operations that can run anywhere would return a mask with just bit for the
current CPU. Those tied to a domain, a copy of the domain mask.

-Tony

Hi Tony, Reinette,

On 2/6/25 15:56, Luck, Tony wrote:
>>>> This new arch API has sharp corners because of asymmetry of where resctrl
>>>> runs the arch function. I do not think it is required to change this since we
>>>> can only speculate about how this may be used in the future but I do think
>>>> it will be helpful to add comments that highlight:
>>>>
>>>> resctrl_arch_mon_event_config_get() ->  May run on CPU that does not belong to domain.
>>>> resctrl_arch_mon_event_config_set() ->  Runs on CPU that belongs to domain.
>>>
>>> Here's a vague data point about the future to help with speculation.
>>>
>>> I have something coming along the pipeline that also can run on any CPU.

RISC-V has this - all their controls/monitors are accessible from any CPU.
Some MPAM platforms can do this too - but the code has to be structured for those
that need the IPI.

Having this be something resctrl can be told sounds like a great idea.

It sounds like all or nothing suits x86/riscv.
The MPAM driver has an accessibility cpumask for each thing it accesses that determines
if it needs to do an IPI.


>>> I am contemplating a flag in the rdt_resource structure (in appropriate substructure
>>> resctrl_cache/resctrl_membw) to indicate "domain" vs. "any" for operations.
>>>
>>> Would something like that be useful here?
>>
>> hmm ... I cannot envision how this may look. Could you please elaborate?
>>
>> You mention "a" (singular) flag in rdt_resource while this scenario involves
>> different ops having different scope. This makes me think that this flag may
>> have to be per operation that in turn would need additional infrastructure to
>> manage and track operations.
>>
>> These "arch" functions are evolving as the work to support MPAM is done and
>> so far I think it has been quite ad-hoc to just refactor arch specific code
>> into "arch" helpers instead of keeping track of which scope they are running in.
>> This currently requires any arch implementing an "arch" helper to be well aware
>> of how resctrl will call it.

This is how APIs in linux evolve - only the immediate problem needs solving.
Arch code being aware how resctrl uses a function shouldn't be surprising - it is the only user.


> I haven't fleshed it out yet. One option would be to have a "choose_cpu_mask()"
> function that takes resource and domain parameters (and given your comment
> about this case an operation code). Then use that as the mask in an smp_call*().
> 
> Operations that can run anywhere would return a mask with just bit for the
> current CPU.

That sounds like extra work. We already have a cpumask, if you set it to the cpu_possible mask
at boot, then smp_call_function() and friends will always prefer the current CPU, and it all falls
out in the wash.


Thanks,

James