If the BMEC (Bandwidth Monitoring Event Configuration) feature is
supported, the bandwidth events can be configured to track specific
events. The event configuration is domain specific. ABMC (Assignable
Bandwidth Monitoring Counters) feature needs event configuration
information to assign hardware counter to an RMID. Event configurations
are not stored in resctrl but instead always read from or written to
hardware directly when prompted by user space.

Read the event configuration from the hardware during the domain
initialization. Save the configuration information in the rdt_hw_domain,
so it can be used for counter assignment.

Signed-off-by: Babu Moger <babu.moger@amd.com>
---
v4: Read the configuration information from the hardware to initialize.
    Added few commit messages.
    Fixed the tab spaces.

v3: Minor changes related to rebase in mbm_config_write_domain.

v2: No changes.
---
 arch/x86/kernel/cpu/resctrl/core.c     |  2 ++
 arch/x86/kernel/cpu/resctrl/internal.h |  5 +++++
 arch/x86/kernel/cpu/resctrl/monitor.c  | 21 +++++++++++++++++++++
 3 files changed, 28 insertions(+)

diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
index ec93f6a50308..856c46d12177 100644
--- a/arch/x86/kernel/cpu/resctrl/core.c
+++ b/arch/x86/kernel/cpu/resctrl/core.c
@@ -542,6 +542,8 @@ static void domain_add_cpu(int cpu, struct rdt_resource *r)
 		return;
 	}
 
+	arch_domain_mbm_evt_config(hw_dom);
+
 	list_add_tail_rcu(&d->list, add_pos);
 
 	err = resctrl_online_domain(r, d);
diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
index 5e7e76cd512f..60a1ca0a11a7 100644
--- a/arch/x86/kernel/cpu/resctrl/internal.h
+++ b/arch/x86/kernel/cpu/resctrl/internal.h
@@ -373,6 +373,8 @@ struct arch_mbm_state {
  * @ctrl_val:	array of cache or mem ctrl values (indexed by CLOSID)
  * @arch_mbm_total:	arch private state for MBM total bandwidth
  * @arch_mbm_local:	arch private state for MBM local bandwidth
+ * @mbm_total_cfg:	MBM total bandwidth configuration
+ * @mbm_local_cfg:	MBM local bandwidth configuration
  *
  * Members of this structure are accessed via helpers that provide abstraction.
  */
@@ -381,6 +383,8 @@ struct rdt_hw_domain {
 	u32				*ctrl_val;
 	struct arch_mbm_state		*arch_mbm_total;
 	struct arch_mbm_state		*arch_mbm_local;
+	u32				mbm_total_cfg;
+	u32				mbm_local_cfg;
 };
 
 static inline struct rdt_hw_domain *resctrl_to_arch_dom(struct rdt_domain *r)
@@ -622,6 +626,7 @@ void __check_limbo(struct rdt_domain *d, bool force_free);
 void rdt_domain_reconfigure_cdp(struct rdt_resource *r);
 void __init resctrl_file_fflags_init(const char *config,
 				     unsigned long fflags);
+void arch_domain_mbm_evt_config(struct rdt_hw_domain *hw_dom);
 void rdt_staged_configs_clear(void);
 bool closid_allocated(unsigned int closid);
 int resctrl_find_cleanest_closid(void);
diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
index b1d002e5e93d..ab0f4bb49bd9 100644
--- a/arch/x86/kernel/cpu/resctrl/monitor.c
+++ b/arch/x86/kernel/cpu/resctrl/monitor.c
@@ -1093,6 +1093,27 @@ int __init rdt_get_mon_l3_config(struct rdt_resource *r)
 	return 0;
 }
 
+void arch_domain_mbm_evt_config(struct rdt_hw_domain *hw_dom)
+{
+	u64 msrval;
+
+	/*
+	 * Read the configuration registers QOS_EVT_CFG_n, where <n> is
+	 * the BMEC event number (EvtID).
+	 * 0 for evtid == QOS_L3_MBM_TOTAL_EVENT_ID
+	 * 1 for evtid == QOS_L3_MBM_LOCAL_EVENT_ID
+	 */
+	if (mbm_total_event.configurable) {
+		rdmsrl(MSR_IA32_EVT_CFG_BASE, msrval);
+		hw_dom->mbm_total_cfg = msrval & MAX_EVT_CONFIG_BITS;
+	}
+
+	if (mbm_local_event.configurable) {
+		rdmsrl(MSR_IA32_EVT_CFG_BASE + 1, msrval);
+		hw_dom->mbm_local_cfg = msrval & MAX_EVT_CONFIG_BITS;
+	}
+}
+
 void __exit rdt_put_mon_l3_config(void)
 {
 	dom_data_exit();
-- 
2.34.1

Hi Babu,

On 5/24/24 5:23 AM, Babu Moger wrote:
> If the BMEC (Bandwidth Monitoring Event Configuration) feature is
> supported, the bandwidth events can be configured to track specific
> events. The event configuration is domain specific. ABMC (Assignable
> Bandwidth Monitoring Counters) feature needs event configuration
> information to assign hardware counter to an RMID. Event configurations
> are not stored in resctrl but instead always read from or written to
> hardware directly when prompted by user space.
> 
> Read the event configuration from the hardware during the domain
> initialization. Save the configuration information in the rdt_hw_domain,
> so it can be used for counter assignment.
> 
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---
> v4: Read the configuration information from the hardware to initialize.
>      Added few commit messages.
>      Fixed the tab spaces.
> 
> v3: Minor changes related to rebase in mbm_config_write_domain.
> 
> v2: No changes.
> ---
>   arch/x86/kernel/cpu/resctrl/core.c     |  2 ++
>   arch/x86/kernel/cpu/resctrl/internal.h |  5 +++++
>   arch/x86/kernel/cpu/resctrl/monitor.c  | 21 +++++++++++++++++++++
>   3 files changed, 28 insertions(+)
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
> index ec93f6a50308..856c46d12177 100644
> --- a/arch/x86/kernel/cpu/resctrl/core.c
> +++ b/arch/x86/kernel/cpu/resctrl/core.c
> @@ -542,6 +542,8 @@ static void domain_add_cpu(int cpu, struct rdt_resource *r)
>   		return;
>   	}
>   
> +	arch_domain_mbm_evt_config(hw_dom);
> +
>   	list_add_tail_rcu(&d->list, add_pos);
>   
>   	err = resctrl_online_domain(r, d);
> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
> index 5e7e76cd512f..60a1ca0a11a7 100644
> --- a/arch/x86/kernel/cpu/resctrl/internal.h
> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
> @@ -373,6 +373,8 @@ struct arch_mbm_state {
>    * @ctrl_val:	array of cache or mem ctrl values (indexed by CLOSID)
>    * @arch_mbm_total:	arch private state for MBM total bandwidth
>    * @arch_mbm_local:	arch private state for MBM local bandwidth
> + * @mbm_total_cfg:	MBM total bandwidth configuration
> + * @mbm_local_cfg:	MBM local bandwidth configuration
>    *
>    * Members of this structure are accessed via helpers that provide abstraction.
>    */
> @@ -381,6 +383,8 @@ struct rdt_hw_domain {
>   	u32				*ctrl_val;
>   	struct arch_mbm_state		*arch_mbm_total;
>   	struct arch_mbm_state		*arch_mbm_local;
> +	u32				mbm_total_cfg;
> +	u32				mbm_local_cfg;
>   };

Similar to the abmc_enabled member of rdt_hw_resource, these new
members of rdt_hw_domain are architecture specific and should never be
touched directly by resctrl fs code, for example, from mbm_config_show().

>   
>   static inline struct rdt_hw_domain *resctrl_to_arch_dom(struct rdt_domain *r)
> @@ -622,6 +626,7 @@ void __check_limbo(struct rdt_domain *d, bool force_free);
>   void rdt_domain_reconfigure_cdp(struct rdt_resource *r);
>   void __init resctrl_file_fflags_init(const char *config,
>   				     unsigned long fflags);
> +void arch_domain_mbm_evt_config(struct rdt_hw_domain *hw_dom);
>   void rdt_staged_configs_clear(void);
>   bool closid_allocated(unsigned int closid);
>   int resctrl_find_cleanest_closid(void);
> diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
> index b1d002e5e93d..ab0f4bb49bd9 100644
> --- a/arch/x86/kernel/cpu/resctrl/monitor.c
> +++ b/arch/x86/kernel/cpu/resctrl/monitor.c
> @@ -1093,6 +1093,27 @@ int __init rdt_get_mon_l3_config(struct rdt_resource *r)
>   	return 0;
>   }
>   
> +void arch_domain_mbm_evt_config(struct rdt_hw_domain *hw_dom)
> +{
> +	u64 msrval;
> +
> +	/*
> +	 * Read the configuration registers QOS_EVT_CFG_n, where <n> is
> +	 * the BMEC event number (EvtID).
> +	 * 0 for evtid == QOS_L3_MBM_TOTAL_EVENT_ID
> +	 * 1 for evtid == QOS_L3_MBM_LOCAL_EVENT_ID

This is what mon_event_config_index_get() is for, no?

> +	 */
> +	if (mbm_total_event.configurable) {
> +		rdmsrl(MSR_IA32_EVT_CFG_BASE, msrval);
> +		hw_dom->mbm_total_cfg = msrval & MAX_EVT_CONFIG_BITS;
> +	}
> +
> +	if (mbm_local_event.configurable) {
> +		rdmsrl(MSR_IA32_EVT_CFG_BASE + 1, msrval);
> +		hw_dom->mbm_local_cfg = msrval & MAX_EVT_CONFIG_BITS;
> +	}
> +}
> +
>   void __exit rdt_put_mon_l3_config(void)
>   {
>   	dom_data_exit();

Reinette

Hi Reinette,

On 6/13/24 20:43, Reinette Chatre wrote:
> Hi Babu,
> 
> On 5/24/24 5:23 AM, Babu Moger wrote:
>> If the BMEC (Bandwidth Monitoring Event Configuration) feature is
>> supported, the bandwidth events can be configured to track specific
>> events. The event configuration is domain specific. ABMC (Assignable
>> Bandwidth Monitoring Counters) feature needs event configuration
>> information to assign hardware counter to an RMID. Event configurations
>> are not stored in resctrl but instead always read from or written to
>> hardware directly when prompted by user space.
>>
>> Read the event configuration from the hardware during the domain
>> initialization. Save the configuration information in the rdt_hw_domain,
>> so it can be used for counter assignment.
>>
>> Signed-off-by: Babu Moger <babu.moger@amd.com>
>> ---
>> v4: Read the configuration information from the hardware to initialize.
>>      Added few commit messages.
>>      Fixed the tab spaces.
>>
>> v3: Minor changes related to rebase in mbm_config_write_domain.
>>
>> v2: No changes.
>> ---
>>   arch/x86/kernel/cpu/resctrl/core.c     |  2 ++
>>   arch/x86/kernel/cpu/resctrl/internal.h |  5 +++++
>>   arch/x86/kernel/cpu/resctrl/monitor.c  | 21 +++++++++++++++++++++
>>   3 files changed, 28 insertions(+)
>>
>> diff --git a/arch/x86/kernel/cpu/resctrl/core.c
>> b/arch/x86/kernel/cpu/resctrl/core.c
>> index ec93f6a50308..856c46d12177 100644
>> --- a/arch/x86/kernel/cpu/resctrl/core.c
>> +++ b/arch/x86/kernel/cpu/resctrl/core.c
>> @@ -542,6 +542,8 @@ static void domain_add_cpu(int cpu, struct
>> rdt_resource *r)
>>           return;
>>       }
>>   +    arch_domain_mbm_evt_config(hw_dom);
>> +
>>       list_add_tail_rcu(&d->list, add_pos);
>>         err = resctrl_online_domain(r, d);
>> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h
>> b/arch/x86/kernel/cpu/resctrl/internal.h
>> index 5e7e76cd512f..60a1ca0a11a7 100644
>> --- a/arch/x86/kernel/cpu/resctrl/internal.h
>> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
>> @@ -373,6 +373,8 @@ struct arch_mbm_state {
>>    * @ctrl_val:    array of cache or mem ctrl values (indexed by CLOSID)
>>    * @arch_mbm_total:    arch private state for MBM total bandwidth
>>    * @arch_mbm_local:    arch private state for MBM local bandwidth
>> + * @mbm_total_cfg:    MBM total bandwidth configuration
>> + * @mbm_local_cfg:    MBM local bandwidth configuration
>>    *
>>    * Members of this structure are accessed via helpers that provide
>> abstraction.
>>    */
>> @@ -381,6 +383,8 @@ struct rdt_hw_domain {
>>       u32                *ctrl_val;
>>       struct arch_mbm_state        *arch_mbm_total;
>>       struct arch_mbm_state        *arch_mbm_local;
>> +    u32                mbm_total_cfg;
>> +    u32                mbm_local_cfg;
>>   };
> 
> Similar to the abmc_enabled member of rdt_hw_resource, these new
> members of rdt_hw_domain are architecture specific and should never be
> touched directly by resctrl fs code, for example, from mbm_config_show().

Need some clarification here.

I am thinking you want to introduce architecture specific routines to get
and set mbm_total_config/mbm_local_config for the domain.
Something like this.

+int arch_get_mbm_evt_cfg(struct rdt_domain *d, enum resctrl_event_id eventid)
+{
+       struct rdt_hw_domain *hw_dom = resctrl_to_arch_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 -1;
+}
+
+void arch_set_mbm_evt_cfg(struct rdt_domain *d,
+                         enum resctrl_event_id eventid, u32 val)
+{
+       struct rdt_hw_domain *hw_dom = resctrl_to_arch_dom(d);
+
+       switch (eventid) {
+       case QOS_L3_OCCUP_EVENT_ID:
+               break;
+       case QOS_L3_MBM_TOTAL_EVENT_ID:
+               hw_dom->mbm_total_cfg = val;
+               break;
+       case QOS_L3_MBM_LOCAL_EVENT_ID:
+               hw_dom->mbm_local_cfg = val;
+       }
+
+       return;
+}
+

I have added the functions in rdtgroup.c and prototypes in internal.h.
This will be called in mbm)config_show() and mbm_config_write_domain()

Did I understand this correctly?

Thanks
Babu

Hi Babu,

On 6/27/24 11:51 AM, Moger, Babu wrote:
> Hi Reinette,
> 
> On 6/13/24 20:43, Reinette Chatre wrote:
>> Hi Babu,
>>
>> On 5/24/24 5:23 AM, Babu Moger wrote:
>>> If the BMEC (Bandwidth Monitoring Event Configuration) feature is
>>> supported, the bandwidth events can be configured to track specific
>>> events. The event configuration is domain specific. ABMC (Assignable
>>> Bandwidth Monitoring Counters) feature needs event configuration
>>> information to assign hardware counter to an RMID. Event configurations
>>> are not stored in resctrl but instead always read from or written to
>>> hardware directly when prompted by user space.
>>>
>>> Read the event configuration from the hardware during the domain
>>> initialization. Save the configuration information in the rdt_hw_domain,
>>> so it can be used for counter assignment.
>>>
>>> Signed-off-by: Babu Moger <babu.moger@amd.com>
>>> ---
>>> v4: Read the configuration information from the hardware to initialize.
>>>       Added few commit messages.
>>>       Fixed the tab spaces.
>>>
>>> v3: Minor changes related to rebase in mbm_config_write_domain.
>>>
>>> v2: No changes.
>>> ---
>>>    arch/x86/kernel/cpu/resctrl/core.c     |  2 ++
>>>    arch/x86/kernel/cpu/resctrl/internal.h |  5 +++++
>>>    arch/x86/kernel/cpu/resctrl/monitor.c  | 21 +++++++++++++++++++++
>>>    3 files changed, 28 insertions(+)
>>>
>>> diff --git a/arch/x86/kernel/cpu/resctrl/core.c
>>> b/arch/x86/kernel/cpu/resctrl/core.c
>>> index ec93f6a50308..856c46d12177 100644
>>> --- a/arch/x86/kernel/cpu/resctrl/core.c
>>> +++ b/arch/x86/kernel/cpu/resctrl/core.c
>>> @@ -542,6 +542,8 @@ static void domain_add_cpu(int cpu, struct
>>> rdt_resource *r)
>>>            return;
>>>        }
>>>    +    arch_domain_mbm_evt_config(hw_dom);
>>> +
>>>        list_add_tail_rcu(&d->list, add_pos);
>>>          err = resctrl_online_domain(r, d);
>>> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h
>>> b/arch/x86/kernel/cpu/resctrl/internal.h
>>> index 5e7e76cd512f..60a1ca0a11a7 100644
>>> --- a/arch/x86/kernel/cpu/resctrl/internal.h
>>> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
>>> @@ -373,6 +373,8 @@ struct arch_mbm_state {
>>>     * @ctrl_val:    array of cache or mem ctrl values (indexed by CLOSID)
>>>     * @arch_mbm_total:    arch private state for MBM total bandwidth
>>>     * @arch_mbm_local:    arch private state for MBM local bandwidth
>>> + * @mbm_total_cfg:    MBM total bandwidth configuration
>>> + * @mbm_local_cfg:    MBM local bandwidth configuration
>>>     *
>>>     * Members of this structure are accessed via helpers that provide
>>> abstraction.
>>>     */
>>> @@ -381,6 +383,8 @@ struct rdt_hw_domain {
>>>        u32                *ctrl_val;
>>>        struct arch_mbm_state        *arch_mbm_total;
>>>        struct arch_mbm_state        *arch_mbm_local;
>>> +    u32                mbm_total_cfg;
>>> +    u32                mbm_local_cfg;
>>>    };
>>
>> Similar to the abmc_enabled member of rdt_hw_resource, these new
>> members of rdt_hw_domain are architecture specific and should never be
>> touched directly by resctrl fs code, for example, from mbm_config_show().
> 
> Need some clarification here.
> 
> I am thinking you want to introduce architecture specific routines to get
> and set mbm_total_config/mbm_local_config for the domain.
> Something like this.
> 
> +int arch_get_mbm_evt_cfg(struct rdt_domain *d, enum resctrl_event_id eventid)

The prefix for arch specific calls converged to "resctrl_arch_".

> +{
> +       struct rdt_hw_domain *hw_dom = resctrl_to_arch_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 -1;
> +}
> +
> +void arch_set_mbm_evt_cfg(struct rdt_domain *d,
> +                         enum resctrl_event_id eventid, u32 val)
> +{
> +       struct rdt_hw_domain *hw_dom = resctrl_to_arch_dom(d);
> +
> +       switch (eventid) {
> +       case QOS_L3_OCCUP_EVENT_ID:
> +               break;
> +       case QOS_L3_MBM_TOTAL_EVENT_ID:
> +               hw_dom->mbm_total_cfg = val;
> +               break;
> +       case QOS_L3_MBM_LOCAL_EVENT_ID:
> +               hw_dom->mbm_local_cfg = val;
> +       }
> +
> +       return;
> +}
> +

I expected that a call to set the event configuration on an architecture to
also interact with the hardware. Essentially what mon_event_config_write()
does today, but in addition also sets the cached mbm_total_cfg/mbm_local_cfg.

Part of this is done by the new MPAM portion [1]. With that the post-ABMC
implementation of resctrl_arch_mon_event_config_read() may be what you
have as arch_get_mbm_evt_cfg() above and resctrl_arch_mon_event_config_write()
does the same as in [1] but with addition of updating the cached
mbm_local_cfg/mbm_total_cfg within struct rdt_hw_domain. Would this work
for ABMC?
  
Reinette

[1] https://lore.kernel.org/lkml/20240614150033.10454-21-james.morse@arm.com/

Hi Reinette,

On 6/27/2024 3:56 PM, Reinette Chatre wrote:
> Hi Babu,
> 
> On 6/27/24 11:51 AM, Moger, Babu wrote:
>> Hi Reinette,
>>
>> On 6/13/24 20:43, Reinette Chatre wrote:
>>> Hi Babu,
>>>
>>> On 5/24/24 5:23 AM, Babu Moger wrote:
>>>> If the BMEC (Bandwidth Monitoring Event Configuration) feature is
>>>> supported, the bandwidth events can be configured to track specific
>>>> events. The event configuration is domain specific. ABMC (Assignable
>>>> Bandwidth Monitoring Counters) feature needs event configuration
>>>> information to assign hardware counter to an RMID. Event configurations
>>>> are not stored in resctrl but instead always read from or written to
>>>> hardware directly when prompted by user space.
>>>>
>>>> Read the event configuration from the hardware during the domain
>>>> initialization. Save the configuration information in the 
>>>> rdt_hw_domain,
>>>> so it can be used for counter assignment.
>>>>
>>>> Signed-off-by: Babu Moger <babu.moger@amd.com>
>>>> ---
>>>> v4: Read the configuration information from the hardware to initialize.
>>>>       Added few commit messages.
>>>>       Fixed the tab spaces.
>>>>
>>>> v3: Minor changes related to rebase in mbm_config_write_domain.
>>>>
>>>> v2: No changes.
>>>> ---
>>>>    arch/x86/kernel/cpu/resctrl/core.c     |  2 ++
>>>>    arch/x86/kernel/cpu/resctrl/internal.h |  5 +++++
>>>>    arch/x86/kernel/cpu/resctrl/monitor.c  | 21 +++++++++++++++++++++
>>>>    3 files changed, 28 insertions(+)
>>>>
>>>> diff --git a/arch/x86/kernel/cpu/resctrl/core.c
>>>> b/arch/x86/kernel/cpu/resctrl/core.c
>>>> index ec93f6a50308..856c46d12177 100644
>>>> --- a/arch/x86/kernel/cpu/resctrl/core.c
>>>> +++ b/arch/x86/kernel/cpu/resctrl/core.c
>>>> @@ -542,6 +542,8 @@ static void domain_add_cpu(int cpu, struct
>>>> rdt_resource *r)
>>>>            return;
>>>>        }
>>>>    +    arch_domain_mbm_evt_config(hw_dom);
>>>> +
>>>>        list_add_tail_rcu(&d->list, add_pos);
>>>>          err = resctrl_online_domain(r, d);
>>>> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h
>>>> b/arch/x86/kernel/cpu/resctrl/internal.h
>>>> index 5e7e76cd512f..60a1ca0a11a7 100644
>>>> --- a/arch/x86/kernel/cpu/resctrl/internal.h
>>>> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
>>>> @@ -373,6 +373,8 @@ struct arch_mbm_state {
>>>>     * @ctrl_val:    array of cache or mem ctrl values (indexed by 
>>>> CLOSID)
>>>>     * @arch_mbm_total:    arch private state for MBM total bandwidth
>>>>     * @arch_mbm_local:    arch private state for MBM local bandwidth
>>>> + * @mbm_total_cfg:    MBM total bandwidth configuration
>>>> + * @mbm_local_cfg:    MBM local bandwidth configuration
>>>>     *
>>>>     * Members of this structure are accessed via helpers that provide
>>>> abstraction.
>>>>     */
>>>> @@ -381,6 +383,8 @@ struct rdt_hw_domain {
>>>>        u32                *ctrl_val;
>>>>        struct arch_mbm_state        *arch_mbm_total;
>>>>        struct arch_mbm_state        *arch_mbm_local;
>>>> +    u32                mbm_total_cfg;
>>>> +    u32                mbm_local_cfg;
>>>>    };
>>>
>>> Similar to the abmc_enabled member of rdt_hw_resource, these new
>>> members of rdt_hw_domain are architecture specific and should never be
>>> touched directly by resctrl fs code, for example, from 
>>> mbm_config_show().
>>
>> Need some clarification here.
>>
>> I am thinking you want to introduce architecture specific routines to get
>> and set mbm_total_config/mbm_local_config for the domain.
>> Something like this.
>>
>> +int arch_get_mbm_evt_cfg(struct rdt_domain *d, enum resctrl_event_id 
>> eventid)
> 
> The prefix for arch specific calls converged to "resctrl_arch_".
> 
>> +{
>> +       struct rdt_hw_domain *hw_dom = resctrl_to_arch_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 -1;
>> +}
>> +
>> +void arch_set_mbm_evt_cfg(struct rdt_domain *d,
>> +                         enum resctrl_event_id eventid, u32 val)
>> +{
>> +       struct rdt_hw_domain *hw_dom = resctrl_to_arch_dom(d);
>> +
>> +       switch (eventid) {
>> +       case QOS_L3_OCCUP_EVENT_ID:
>> +               break;
>> +       case QOS_L3_MBM_TOTAL_EVENT_ID:
>> +               hw_dom->mbm_total_cfg = val;
>> +               break;
>> +       case QOS_L3_MBM_LOCAL_EVENT_ID:
>> +               hw_dom->mbm_local_cfg = val;
>> +       }
>> +
>> +       return;
>> +}
>> +
> 
> I expected that a call to set the event configuration on an architecture to
> also interact with the hardware. Essentially what mon_event_config_write()
> does today, but in addition also sets the cached 
> mbm_total_cfg/mbm_local_cfg.

Yea. Understood.
> 
> Part of this is done by the new MPAM portion [1]. With that the post-ABMC
> implementation of resctrl_arch_mon_event_config_read() may be what you
> have as arch_get_mbm_evt_cfg() above and 
> resctrl_arch_mon_event_config_write()
> does the same as in [1] but with addition of updating the cached
> mbm_local_cfg/mbm_total_cfg within struct rdt_hw_domain. Would this work
> for ABMC?


Yes. It does work. I can keep the same name as well.
Thanks for the clarifications.

> 
> Reinette
> 
> [1] 
> https://lore.kernel.org/lkml/20240614150033.10454-21-james.morse@arm.com/
> 
> 

-- 
- Babu Moger

Hi Reinette,

On 6/13/24 20:43, Reinette Chatre wrote:
> Hi Babu,
> 
> On 5/24/24 5:23 AM, Babu Moger wrote:
>> If the BMEC (Bandwidth Monitoring Event Configuration) feature is
>> supported, the bandwidth events can be configured to track specific
>> events. The event configuration is domain specific. ABMC (Assignable
>> Bandwidth Monitoring Counters) feature needs event configuration
>> information to assign hardware counter to an RMID. Event configurations
>> are not stored in resctrl but instead always read from or written to
>> hardware directly when prompted by user space.
>>
>> Read the event configuration from the hardware during the domain
>> initialization. Save the configuration information in the rdt_hw_domain,
>> so it can be used for counter assignment.
>>
>> Signed-off-by: Babu Moger <babu.moger@amd.com>
>> ---
>> v4: Read the configuration information from the hardware to initialize.
>>      Added few commit messages.
>>      Fixed the tab spaces.
>>
>> v3: Minor changes related to rebase in mbm_config_write_domain.
>>
>> v2: No changes.
>> ---
>>   arch/x86/kernel/cpu/resctrl/core.c     |  2 ++
>>   arch/x86/kernel/cpu/resctrl/internal.h |  5 +++++
>>   arch/x86/kernel/cpu/resctrl/monitor.c  | 21 +++++++++++++++++++++
>>   3 files changed, 28 insertions(+)
>>
>> diff --git a/arch/x86/kernel/cpu/resctrl/core.c
>> b/arch/x86/kernel/cpu/resctrl/core.c
>> index ec93f6a50308..856c46d12177 100644
>> --- a/arch/x86/kernel/cpu/resctrl/core.c
>> +++ b/arch/x86/kernel/cpu/resctrl/core.c
>> @@ -542,6 +542,8 @@ static void domain_add_cpu(int cpu, struct
>> rdt_resource *r)
>>           return;
>>       }
>>   +    arch_domain_mbm_evt_config(hw_dom);
>> +
>>       list_add_tail_rcu(&d->list, add_pos);
>>         err = resctrl_online_domain(r, d);
>> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h
>> b/arch/x86/kernel/cpu/resctrl/internal.h
>> index 5e7e76cd512f..60a1ca0a11a7 100644
>> --- a/arch/x86/kernel/cpu/resctrl/internal.h
>> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
>> @@ -373,6 +373,8 @@ struct arch_mbm_state {
>>    * @ctrl_val:    array of cache or mem ctrl values (indexed by CLOSID)
>>    * @arch_mbm_total:    arch private state for MBM total bandwidth
>>    * @arch_mbm_local:    arch private state for MBM local bandwidth
>> + * @mbm_total_cfg:    MBM total bandwidth configuration
>> + * @mbm_local_cfg:    MBM local bandwidth configuration
>>    *
>>    * Members of this structure are accessed via helpers that provide
>> abstraction.
>>    */
>> @@ -381,6 +383,8 @@ struct rdt_hw_domain {
>>       u32                *ctrl_val;
>>       struct arch_mbm_state        *arch_mbm_total;
>>       struct arch_mbm_state        *arch_mbm_local;
>> +    u32                mbm_total_cfg;
>> +    u32                mbm_local_cfg;
>>   };
> 
> Similar to the abmc_enabled member of rdt_hw_resource, these new
> members of rdt_hw_domain are architecture specific and should never be
> touched directly by resctrl fs code, for example, from mbm_config_show().

Yes. Understood. Will check all the accesses.

> 
>>     static inline struct rdt_hw_domain *resctrl_to_arch_dom(struct
>> rdt_domain *r)
>> @@ -622,6 +626,7 @@ void __check_limbo(struct rdt_domain *d, bool
>> force_free);
>>   void rdt_domain_reconfigure_cdp(struct rdt_resource *r);
>>   void __init resctrl_file_fflags_init(const char *config,
>>                        unsigned long fflags);
>> +void arch_domain_mbm_evt_config(struct rdt_hw_domain *hw_dom);
>>   void rdt_staged_configs_clear(void);
>>   bool closid_allocated(unsigned int closid);
>>   int resctrl_find_cleanest_closid(void);
>> diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c
>> b/arch/x86/kernel/cpu/resctrl/monitor.c
>> index b1d002e5e93d..ab0f4bb49bd9 100644
>> --- a/arch/x86/kernel/cpu/resctrl/monitor.c
>> +++ b/arch/x86/kernel/cpu/resctrl/monitor.c
>> @@ -1093,6 +1093,27 @@ int __init rdt_get_mon_l3_config(struct
>> rdt_resource *r)
>>       return 0;
>>   }
>>   +void arch_domain_mbm_evt_config(struct rdt_hw_domain *hw_dom)
>> +{
>> +    u64 msrval;
>> +
>> +    /*
>> +     * Read the configuration registers QOS_EVT_CFG_n, where <n> is
>> +     * the BMEC event number (EvtID).
>> +     * 0 for evtid == QOS_L3_MBM_TOTAL_EVENT_ID
>> +     * 1 for evtid == QOS_L3_MBM_LOCAL_EVENT_ID
> 
> This is what mon_event_config_index_get() is for, no?

Yes, it is. I can change it.

> 
>> +     */
>> +    if (mbm_total_event.configurable) {
>> +        rdmsrl(MSR_IA32_EVT_CFG_BASE, msrval);
>> +        hw_dom->mbm_total_cfg = msrval & MAX_EVT_CONFIG_BITS;
>> +    }
>> +
>> +    if (mbm_local_event.configurable) {
>> +        rdmsrl(MSR_IA32_EVT_CFG_BASE + 1, msrval);
>> +        hw_dom->mbm_local_cfg = msrval & MAX_EVT_CONFIG_BITS;
>> +    }
>> +}
>> +
>>   void __exit rdt_put_mon_l3_config(void)
>>   {
>>       dom_data_exit();
> 
> Reinette
> 

-- 
Thanks
Babu Moger