Re: [PATCH v8 14/25] x86/resctrl: Add data structures and definitions for ABMC assignment

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

Hi Babu,

On 10/17/24 11:52 AM, Moger, Babu wrote:
> On 10/15/24 22:21, Reinette Chatre wrote:
>> On 10/9/24 10:39 AM, Babu Moger wrote:

>>> + *			  tracked by the @cntr_id.
>>> + * @bw_src		: Bandwidth source (RMID or CLOSID).
>>> + * @reserved1		: Reserved.
>>> + * @is_clos		: @bw_src field is a CLOSID (not an RMID).
>>> + * @cntr_id		: Counter identifier.
>>> + * @reserved		: Reserved.
>>> + * @cntr_en		: Counting enable bit.
>>> + * @cfg_en		: Configuration enable bit.
>>> + *
>>> + * Configuration and counting:
>>> + * cfg_en=0,            : No configuration changes applied.
>>
>> Can this be expanded? (sidenote: It is taking a long time to get clarity on how
>> to interact with hardware. These incremental cryptic fragments make it difficult
>> to know how to interact with the hardware.)
>>
>> For example, "No configuration changes applied. Counter can be configured across
>> multiple writes to MSR while @cfg_en=0. Configuration applied when @cfg_en=1."
>>
>>> + * cfg_en=1, cntr_en=0  : Configure cntr_id and but no counting the events.
>>
>> hmmm ... still the same (""but no counting the events") strange language I
>> highlighted in V7 ...
>>
>> I think it will make things easier to understand if similar language is used
>> between the descriptions of the different fields.
>>
>> "Apply @cntr_id configuration but do not count events." 
>>  
>>> + * cfg_en=1, cntr_en=1  : Configure cntr_id and start counting the events.
>>
>> "Apply @cntr_id configuration and start counting events." 
>>
>> Can it be added here which of these settings (or combination of settings) result
>> in counters being reset?
> 
> Any change in the configuration will reset the counters.
> 
> Little bit lost here. Let me summarize. How about this?
> 
> Configuration and counting:
> Counter can be configured across multiple writes to MSR. Configuration
> is applied only when @cfg_en = 1. The event counters will reset when any
> of the configuration is changed.

Is is not clear to me what is meant with "when any of the configuration is changed".
Are event counters reset with every write to the MSR, whether @cfg_en is set or
not? If counters are only reset when @cfg_en is set then I think it should read
"Counter @cntr_id is reset when the configuration is applied." Note this is
also made specific to be related to *just* the counter being configured, not all
event counters that "The event counters will reset ..." implies.

> cfg_en = 1, cntr_en = 0 : Apply @cntr_id configuration but do not count
> events.
> cfg_en = 1, cntr_en = 1 : Apply @cntr_id configuration and start counting
> events.
> 

Please use the @ prefix when referring to union members.

Thank you

Reinette