Re: [PATCH v5 21/40] x86/resctrl: Change mon_event_config_{read,write}() to be arch helpers

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

Hi James,

On 10/4/24 11:03 AM, James Morse wrote:
> @@ -315,6 +322,24 @@ int resctrl_arch_update_domains(struct rdt_resource *r, u32 closid);
>  
>  bool __init resctrl_arch_is_evt_configurable(enum resctrl_event_id evt);
>  
> +/**
> + * resctrl_arch_mon_event_config_write() - Write the config for a counter.

Please avoid the term "counter" for events ... the upcoming AMD work adds support
for counters*. 

> + * @info:	struct resctrl_mon_config_info describing the resource, domain
> + *		and event.

Expected "config" to appear as part of information about function
that claims to writes config?

> + *
> + * Must be called on a CPU that is a member of the specified domain.

I am not sure about this. Is this documentation intended to support authors of
arch code? In that case it may be instead useful to know that this function will be
called on a CPU that is a member of the specified domain to avoid confusion from arch
side whether it needs to take some action to ensure function is called on right CPU.

	Called on a CPU that is a member of the specified domain.

> + */
> +void resctrl_arch_mon_event_config_write(void *info);
> +
> +/**
> + * resctrl_arch_mon_event_config_read() - Read the config for a counter.

counter -> event

> + * @info:	struct resctrl_mon_config_info describing the resource, domain
> + *		and event.

Copy&paste? No information on how config is returned.

> + *
> + * Must be called on a CPU that is a member of the specified domain.

Same comment as above.

> + */
> +void resctrl_arch_mon_event_config_read(void *info);
> +
>  /*
>   * Update the ctrl_val and apply this config right now.
>   * Must be called on one of the domain's CPUs.

Reinette


* Awaiting Arm's feedback on that on whether it works for MPAM.