Re: [PATCH v15 16/34] x86,fs/resctrl: Implement resctrl_arch_config_cntr() to assign a counter with ABMC

["Moger, Babu" <babu.moger@amd.com>]

Hi Reinette,


On 7/17/25 13:49, Reinette Chatre wrote:
> Hi Babu,
> 
> On 7/8/25 3:17 PM, Babu Moger wrote:
>> The ABMC feature allows users to assign a hardware counter to an RMID,
>> event pair and monitor bandwidth usage as long as it is assigned. The
>> hardware continues to track the assigned counter until it is explicitly
>> unassigned by the user.
>>
>> Implement an architecture-specific handler to assign and unassign the
>> counter. Configure counters by writing to the L3_QOS_ABMC_CFG MSR,
>> specifying the counter ID, bandwidth source (RMID), and event
>> configuration.
> 
>>From above description it is not obvious to me how configuring a counter
> is connected to assign/unassign/configure. How about something like:
> 
>   Implement an x86 architecture-specific handler to configure a counter. This
>   architecture specific handler is called by resctrl fs when a counter
>   is assigned or unassigned as well as when an already assigned counter's
>   configuration should be updated. Configure counters by writing to the
>   L3_QOS_ABMC_CFG MSR, specifying the counter ID, bandwidth source (RMID),
>   and event configuration.

Looks good.

> 
> (please feel free to improve)
> 
>>
>> The feature details are documented in the APM listed below [1].
>> [1] AMD64 Architecture Programmer's Manual Volume 2: System Programming
>>     Publication # 24593 Revision 3.41 section 19.3.3.3 Assignable Bandwidth
>>     Monitoring (ABMC).
>>
>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=206537
>> Signed-off-by: Babu Moger <babu.moger@amd.com>
>> ---
> 
> ...
> 
>> +/*
>> + * Send an IPI to the domain to assign the counter to RMID, event pair.
>> + */
>> +void resctrl_arch_config_cntr(struct rdt_resource *r, struct rdt_mon_domain *d,
>> +			      enum resctrl_event_id evtid, u32 rmid, u32 closid,
>> +			      u32 cntr_id, bool assign)
>> +{
>> +	struct rdt_hw_mon_domain *hw_dom = resctrl_to_arch_mon_dom(d);
>> +	union l3_qos_abmc_cfg abmc_cfg = { 0 };
>> +	struct arch_mbm_state *am;
>> +
>> +	abmc_cfg.split.cfg_en = 1;
>> +	abmc_cfg.split.cntr_en = assign ? 1 : 0;
>> +	abmc_cfg.split.cntr_id = cntr_id;
>> +	abmc_cfg.split.bw_src = rmid;
>> +	if (assign)
>> +		abmc_cfg.split.bw_type = resctrl_get_mon_evt_cfg(evtid);
>> +
>> +	smp_call_function_any(&d->hdr.cpu_mask, resctrl_abmc_config_one_amd, &abmc_cfg, 1);
>> +
>> +	/*
>> +	 * The hardware counter is reset (because cfg_en == 1) so there is no
>> +	 * need to record initial non-zero counts.
>> +	 */
>> +	if (assign) {
> 
> We learned from patch #14 that the counter is reset whenever cfg_en = 1. Since this
> is always the case it is not clear to me why the architectural state is not always
> reset to match what hardware does. Even more, looking how this function is later used in
> resctrl_config_cntr(), the caller resctrl_config_cntr() always resets non-architectural
> state, why not always reset architectural state here?
> 

We initially had reset logic for both assign and unassign cases, but later
thought it might not be necessary during unassign. However, it's better to
keep it in both cases, since resctrl_config_cntr() resets the
non-architectural state regardless."


-- 
Thanks
Babu Moger