linux-kernel - Re: [PATCH v11 20/23] x86/resctrl: Configure mbm_cntr

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <3fb20135-1b8a-42df-b670-5fbf29acfa85@intel.com>
Date: Mon, 24 Feb 2025 09:01:59 -0800
From: Reinette Chatre <reinette.chatre@...el.com>
To: <babu.moger@....com>, James Morse <james.morse@....com>, <corbet@....net>,
	<tglx@...utronix.de>, <mingo@...hat.com>, <bp@...en8.de>,
	<dave.hansen@...ux.intel.com>, <tony.luck@...el.com>,
	<peternewman@...gle.com>
CC: <x86@...nel.org>, <hpa@...or.com>, <paulmck@...nel.org>,
	<akpm@...ux-foundation.org>, <thuth@...hat.com>, <rostedt@...dmis.org>,
	<xiongwei.song@...driver.com>, <pawan.kumar.gupta@...ux.intel.com>,
	<daniel.sneddon@...ux.intel.com>, <jpoimboe@...nel.org>,
	<perry.yuan@....com>, <sandipan.das@....com>, <kai.huang@...el.com>,
	<xiaoyao.li@...el.com>, <seanjc@...gle.com>, <xin3.li@...el.com>,
	<andrew.cooper3@...rix.com>, <ebiggers@...gle.com>,
	<mario.limonciello@....com>, <tan.shaopeng@...itsu.com>,
	<linux-doc@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
	<maciej.wieczor-retman@...el.com>, <eranian@...gle.com>
Subject: Re: [PATCH v11 20/23] x86/resctrl: Configure mbm_cntr_assign mode if
 supported

Hi James and Babu,

On 2/24/25 7:49 AM, Moger, Babu wrote:
> Hi James,
> 
> On 2/21/25 12:06, James Morse wrote:
>> Hi Babu,
>>
>> On 22/01/2025 20:20, Babu Moger wrote:

>> This sequence has me confused:
>>
>>> diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
>>> index 3d748fdbcb5f..a9a5dc626a1e 100644
>>> --- a/arch/x86/kernel/cpu/resctrl/monitor.c
>>> +++ b/arch/x86/kernel/cpu/resctrl/monitor.c
>>> @@ -1233,6 +1233,7 @@ int __init rdt_get_mon_l3_config(struct rdt_resource *r)
>>>  			r->mon.mbm_cntr_assignable = true;
>>>  			cpuid_count(0x80000020, 5, &eax, &ebx, &ecx, &edx);
>>>  			r->mon.num_mbm_cntrs = (ebx & GENMASK(15, 0)) + 1;
>>
>>> +			hw_res->mbm_cntr_assign_enabled = true;
>>
>> Here the arch code sets ABMC to be enabled by default at boot.
>>
>>
>>> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>>> index 6922173c4f8f..515969c5f64f 100644
>>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>>> @@ -4302,9 +4302,13 @@ int resctrl_online_mon_domain(struct rdt_resource *r, struct rdt_mon_domain *d)
>>>  
>>>  void resctrl_online_cpu(unsigned int cpu)
>>>  {
>>> +	struct rdt_resource *r = &rdt_resources_all[RDT_RESOURCE_L3].r_resctrl;
>>> +
>>>  	mutex_lock(&rdtgroup_mutex);
>>>  	/* The CPU is set in default rdtgroup after online. */
>>>  	cpumask_set_cpu(cpu, &rdtgroup_default.cpu_mask);
>>> +	if (r->mon_capable && r->mon.mbm_cntr_assignable)
>>> +		resctrl_arch_mbm_cntr_assign_set_one(r);
>>>  	mutex_unlock(&rdtgroup_mutex);
>>>  }
>>
>> But here, resctrl has to call back to the arch code to make sure the hardware is in the
>> same state as hw_res->mbm_cntr_assign_enabled.

Another scenario needing to be supported by this flow is when CPUs come online later ...
after resctrl is mounted and potentially after the user modified the assignable counter
mode.

>>
>> Could this be done in resctrl_arch_online_cpu() instead? That way resctrl doesn't get CPUs
>> in an inconsistent state that it has to fix up...

Could you please elaborate the inconsistent state that would need to be fixed up?

>>
> 
> Sure. Here is the diff.
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/core.c
> b/arch/x86/kernel/cpu/resctrl/core.c
> index 22399f19810f..f48b298413bc 100644
> --- a/arch/x86/kernel/cpu/resctrl/core.c
> +++ b/arch/x86/kernel/cpu/resctrl/core.c
> @@ -771,6 +771,12 @@ static int resctrl_arch_online_cpu(unsigned int cpu)
>                 domain_add_cpu(cpu, r);
>         mutex_unlock(&domain_list_lock);
> 
> +       r = &rdt_resources_all[RDT_RESOURCE_L3].r_resctrl;
> +       mutex_lock(&rdtgroup_mutex);
> +       if (r->mon_capable && r->mon.mbm_cntr_assignable)
> +               resctrl_arch_mbm_cntr_assign_set_one(r);
> +       mutex_unlock(&rdtgroup_mutex);
> +
>         clear_closid_rmid(cpu);
>         resctrl_online_cpu(cpu);

This would require every architecture to duplicate the above, no?

Also, please note there is more appropriate domain_add_cpu_mon().

Reinette