Re: [PATCH v5 18/20] x86/resctrl: Enable AMD ABMC feature by default when supported

[Reinette Chatre <reinette.chatre@xxxxxxxxx>]

Hi Babu,

On 7/3/24 2:48 PM, Babu Moger wrote:
> Enable ABMC by default when supported during the boot up.
>
> Users will not see any difference in the behavior when resctrl is
> mounted. With automatic assignment everything will work as running
> in the legacy monitor mode.
>
> Signed-off-by: Babu Moger <babu.moger@xxxxxxx>
> ---
> v5: New patch to enable ABMC by default.
> ---
>   arch/x86/kernel/cpu/resctrl/core.c     |  2 ++
>   arch/x86/kernel/cpu/resctrl/internal.h |  1 +
>   arch/x86/kernel/cpu/resctrl/rdtgroup.c | 17 +++++++++++++++++
>   3 files changed, 20 insertions(+)
>
> diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
> index 6265ef8b610f..b69b2650bde3 100644
> --- a/arch/x86/kernel/cpu/resctrl/core.c
> +++ b/arch/x86/kernel/cpu/resctrl/core.c
> @@ -599,6 +599,7 @@ static void domain_add_cpu_mon(int cpu, struct rdt_resource *r)
>   		d = container_of(hdr, struct rdt_mon_domain, hdr);
>   
>   		cpumask_set_cpu(cpu, &d->hdr.cpu_mask);
> +		resctrl_arch_configure_abmc();
>   		return;
>   	}
>   
> @@ -620,6 +621,7 @@ static void domain_add_cpu_mon(int cpu, struct rdt_resource *r)
>   	arch_mon_domain_online(r, d);
>   
>   	resctrl_arch_mbm_evt_config(hw_dom);
> +	resctrl_arch_configure_abmc();
>   
>   	if (arch_domain_mbm_alloc(r->mon.num_rmid, hw_dom)) {
>   		mon_domain_free(hw_dom);
> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
> index beb005775fe4..0f858cff8ab1 100644
> --- a/arch/x86/kernel/cpu/resctrl/internal.h
> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
> @@ -707,6 +707,7 @@ void rdt_domain_reconfigure_cdp(struct rdt_resource *r);
>   void __init resctrl_file_fflags_init(const char *config,
>   				     unsigned long fflags);
>   void resctrl_arch_mbm_evt_config(struct rdt_hw_mon_domain *hw_dom);
> +void resctrl_arch_configure_abmc(void);
>   unsigned int mon_event_config_index_get(u32 evtid);
>   int resctrl_arch_assign_cntr(struct rdt_mon_domain *d, u32 evtid, u32 rmid,
>   			     u32 cntr_id, u32 closid, bool enable);
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index 531233779f8d..d978668c8865 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -2733,6 +2733,23 @@ void resctrl_arch_abmc_disable(void)
>   	}
>   }
>   
> +void resctrl_arch_configure_abmc(void)
> +{
> +	struct rdt_resource *r = &rdt_resources_all[RDT_RESOURCE_L3].r_resctrl;
> +	struct rdt_hw_resource *hw_res = resctrl_to_arch_res(r);
> +	bool enable = true;
> +
> +	mutex_lock(&rdtgroup_mutex);
> +
> +	if (r->mon.abmc_capable) {
> +		if (!hw_res->abmc_enabled)
> +			hw_res->abmc_enabled = true;
> +		resctrl_abmc_set_one_amd(&enable);
> +	}

This does not look right. It is not architecture code that needs to
decide if this feature is enabled or not, right? The feature is enabled
via fs (for example when user writes to mbm_mode). If the default is
enabled then it should be set by fs. resctrl_arch_configure_abmc()
then checks if feature is capable and enabled before it configures
it on the CPU.

> +
> +	mutex_unlock(&rdtgroup_mutex);
> +}
> +
>   /*
>    * We don't allow rdtgroup directories to be created anywhere
>    * except the root directory. Thus when looking for the rdtgroup

Reinette