linux-kernel - Re: [RFC PATCH v3 07/17] x86/resctrl: Add support to enable/disable ABMC feature

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: <a873f6ba-904e-4317-b177-5645c3a8526d@intel.com>
Date: Tue, 7 May 2024 13:32:56 -0700
From: Reinette Chatre <reinette.chatre@...el.com>
To: <babu.moger@....com>, <corbet@....net>, <fenghua.yu@...el.com>,
	<tglx@...utronix.de>, <mingo@...hat.com>, <bp@...en8.de>,
	<dave.hansen@...ux.intel.com>
CC: <x86@...nel.org>, <hpa@...or.com>, <paulmck@...nel.org>,
	<rdunlap@...radead.org>, <tj@...nel.org>, <peterz@...radead.org>,
	<yanjiewtw@...il.com>, <kim.phillips@....com>, <lukas.bulwahn@...il.com>,
	<seanjc@...gle.com>, <jmattson@...gle.com>, <leitao@...ian.org>,
	<jpoimboe@...nel.org>, <rick.p.edgecombe@...el.com>,
	<kirill.shutemov@...ux.intel.com>, <jithu.joseph@...el.com>,
	<kai.huang@...el.com>, <kan.liang@...ux.intel.com>,
	<daniel.sneddon@...ux.intel.com>, <pbonzini@...hat.com>,
	<sandipan.das@....com>, <ilpo.jarvinen@...ux.intel.com>,
	<peternewman@...gle.com>, <maciej.wieczor-retman@...el.com>,
	<linux-doc@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
	<eranian@...gle.com>, <james.morse@....com>
Subject: Re: [RFC PATCH v3 07/17] x86/resctrl: Add support to enable/disable
 ABMC feature

Hi Babu,

On 5/7/2024 12:12 PM, Moger, Babu wrote:
> On 5/3/24 18:30, Reinette Chatre wrote:
>> On 3/28/2024 6:06 PM, Babu Moger wrote:

..

>>>  /* MSR_IA32_VMX_MISC bits */
>>>  #define MSR_IA32_VMX_MISC_INTEL_PT                 (1ULL << 14)
>>> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
>>> index 722388621403..8238ee437369 100644
>>> --- a/arch/x86/kernel/cpu/resctrl/internal.h
>>> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
>>> @@ -96,6 +96,9 @@ cpumask_any_housekeeping(const struct cpumask *mask, int exclude_cpu)
>>>  	return cpu;
>>>  }
>>>  
>>> +/* ABMC ENABLE */
>>
>> Can this comment be made more useful?
> 
> How about?
> /* Setting bit 0 in L3_QOS_EXT_CFG enables ABMC features */

Regarding "ABMC features" - are there several features connected to
"ABMC"?

> 
> Or I can remove it totally.
> 
>>

..

>>> +int resctrl_arch_set_abmc_enabled(enum resctrl_res_level l, bool enable)
>>> +{
>>> +	struct rdt_hw_resource *hw_res = &rdt_resources_all[l];
>>> +
>>> +	if (!hw_res->r_resctrl.mbm_assign_capable)
>>> +		return -EINVAL;
>>> +
>>> +	if (enable)
>>> +		return resctrl_abmc_enable(l);
>>> +
>>> +	resctrl_abmc_disable(l);
>>> +
>>> +	return 0;
>>> +}
>>
>> Why is resctrl_arch_set_abmc_enabled() necessary? It seem to add an unnecessary
>> layer of abstraction.
>>
> 
> I feel it is better to keep it that way. It is consistent with definition
> of resctrl_arch_set_cdp_enabled. It handles both enable and disable.
> Otherwise we have add those checks from the caller.

Caller needs to know anyway whether to provide "true" or "false" to this function ...
caller may as well just call the appropriate _enable()/_disable() variant, no?

Reinette