diff mbox series

[RFC,v1,1/4] x86/cpufeatures: Add CPUID feature bit for the Bus Lock Threshold

Message ID 20240709175145.9986-2-manali.shukla@amd.com (mailing list archive)
State New
Headers show
Series Add support for the Bus Lock Threshold | expand

Commit Message

Manali Shukla July 9, 2024, 5:51 p.m. UTC 
Malicious guests can cause bus locks to degrade the performance of
a system. Non-WB(write-back) and misaligned locked
RMW(read-modify-write) instructions are referred to as "bus locks" and
require system wide synchronization among all processors to guarantee
atomicity.  The bus locks may incur significant performance penalties
for all processors in the system.

The Bus Lock Threshold feature proves beneficial for hypervisors
seeking to restrict guests' ability to initiate numerous bus locks,
thereby preventing system slowdowns that affect all tenants.

Presence of the Bus Lock threshold feature is indicated via CPUID
function 0x8000000A_EDX[29]

Signed-off-by: Manali Shukla <manali.shukla@amd.com>
---
 arch/x86/include/asm/cpufeatures.h | 1 +
 1 file changed, 1 insertion(+)


base-commit: 704ec48fc2fbd4e41ec982662ad5bf1eee33eeb2

Comments

Sean Christopherson Aug. 16, 2024, 7:37 p.m. UTC | #1 
On Tue, Jul 09, 2024, Manali Shukla wrote:
> Malicious guests can cause bus locks to degrade the performance of

I would say "misbehaving", I bet the overwhelming majority of bus locks in practice
are due to legacy/crusty software, not malicious software.

> a system. Non-WB(write-back) and misaligned locked
> RMW(read-modify-write) instructions are referred to as "bus locks" and
> require system wide synchronization among all processors to guarantee
> atomicity.  The bus locks may incur significant performance penalties
> for all processors in the system.
> 
> The Bus Lock Threshold feature proves beneficial for hypervisors
> seeking to restrict guests' ability to initiate numerous bus locks,
> thereby preventing system slowdowns that affect all tenants.

None of this actually says what the feature does.

> Presence of the Bus Lock threshold feature is indicated via CPUID
> function 0x8000000A_EDX[29]
> 
> Signed-off-by: Manali Shukla <manali.shukla@amd.com>
> ---
>  arch/x86/include/asm/cpufeatures.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
> index 3c7434329661..10f397873790 100644
> --- a/arch/x86/include/asm/cpufeatures.h
> +++ b/arch/x86/include/asm/cpufeatures.h
> @@ -381,6 +381,7 @@
>  #define X86_FEATURE_V_SPEC_CTRL		(15*32+20) /* Virtual SPEC_CTRL */
>  #define X86_FEATURE_VNMI		(15*32+25) /* Virtual NMI */
>  #define X86_FEATURE_SVME_ADDR_CHK	(15*32+28) /* "" SVME addr check */
> +#define X86_FEATURE_BUS_LOCK_THRESHOLD	(15*32+29) /* "" Bus lock threshold */

I would strongly prefer to enumerate this in /proc/cpuinfo, having to manually
query CPUID to see if a CPU supports a feature I want to test is beyond annoying.

>  /* Intel-defined CPU features, CPUID level 0x00000007:0 (ECX), word 16 */
>  #define X86_FEATURE_AVX512VBMI		(16*32+ 1) /* AVX512 Vector Bit Manipulation instructions*/
> 
> base-commit: 704ec48fc2fbd4e41ec982662ad5bf1eee33eeb2
> -- 
> 2.34.1
>
Manali Shukla Aug. 22, 2024, 9:43 a.m. UTC | #2 
Hi Sean,

Thank you for reviewing my patches.

On 8/17/2024 1:07 AM, Sean Christopherson wrote:
> On Tue, Jul 09, 2024, Manali Shukla wrote:
>> Malicious guests can cause bus locks to degrade the performance of
> 
> I would say "misbehaving", I bet the overwhelming majority of bus locks in practice
> are due to legacy/crusty software, not malicious software.
> 

Ack.

>> a system. Non-WB(write-back) and misaligned locked
>> RMW(read-modify-write) instructions are referred to as "bus locks" and
>> require system wide synchronization among all processors to guarantee
>> atomicity.  The bus locks may incur significant performance penalties
>> for all processors in the system.
>>
>> The Bus Lock Threshold feature proves beneficial for hypervisors
>> seeking to restrict guests' ability to initiate numerous bus locks,
>> thereby preventing system slowdowns that affect all tenants.
> 
> None of this actually says what the feature does.
> 

Sure I will rewrite the commit message. 

>> Presence of the Bus Lock threshold feature is indicated via CPUID
>> function 0x8000000A_EDX[29]
>>
>> Signed-off-by: Manali Shukla <manali.shukla@amd.com>
>> ---
>>  arch/x86/include/asm/cpufeatures.h | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
>> index 3c7434329661..10f397873790 100644
>> --- a/arch/x86/include/asm/cpufeatures.h
>> +++ b/arch/x86/include/asm/cpufeatures.h
>> @@ -381,6 +381,7 @@
>>  #define X86_FEATURE_V_SPEC_CTRL		(15*32+20) /* Virtual SPEC_CTRL */
>>  #define X86_FEATURE_VNMI		(15*32+25) /* Virtual NMI */
>>  #define X86_FEATURE_SVME_ADDR_CHK	(15*32+28) /* "" SVME addr check */
>> +#define X86_FEATURE_BUS_LOCK_THRESHOLD	(15*32+29) /* "" Bus lock threshold */
> 
> I would strongly prefer to enumerate this in /proc/cpuinfo, having to manually
> query CPUID to see if a CPU supports a feature I want to test is beyond annoying.

I will do the modifications accordingly.

> 
>>  /* Intel-defined CPU features, CPUID level 0x00000007:0 (ECX), word 16 */
>>  #define X86_FEATURE_AVX512VBMI		(16*32+ 1) /* AVX512 Vector Bit Manipulation instructions*/
>>
>> base-commit: 704ec48fc2fbd4e41ec982662ad5bf1eee33eeb2
>> -- 
>> 2.34.1
>>
 - Manali
Borislav Petkov Aug. 29, 2024, 6:48 a.m. UTC | #3 
On Fri, Aug 16, 2024 at 12:37:52PM -0700, Sean Christopherson wrote:
> I would strongly prefer to enumerate this in /proc/cpuinfo, having to manually
> query CPUID to see if a CPU supports a feature I want to test is beyond annoying.

Why?

We have tools/arch/x86/kcpuid/kcpuid.c for that.
Sean Christopherson Aug. 30, 2024, 4:42 a.m. UTC | #4 
On Thu, Aug 29, 2024, Borislav Petkov wrote:
> On Fri, Aug 16, 2024 at 12:37:52PM -0700, Sean Christopherson wrote:
> > I would strongly prefer to enumerate this in /proc/cpuinfo, having to manually
> > query CPUID to see if a CPU supports a feature I want to test is beyond annoying.
> 
> Why?
> 
> We have tools/arch/x86/kcpuid/kcpuid.c for that.

Ah, sorry, if the platform+kernel supports the feature, not just raw CPU.  And
because that utility is not available by default on most targets I care about,
and having to build and copy over a binary is annoying (though this is a minor
gripe).

That said, what I really want in most cases is to know if _KVM_ supports a
feature.  I'll think more on this, I have a few vague ideas for getting a pile
of information out of KVM without needing to add more uABI.
Borislav Petkov Aug. 30, 2024, 8:21 a.m. UTC | #5 
On Thu, Aug 29, 2024 at 09:42:40PM -0700, Sean Christopherson wrote:
> Ah, sorry, if the platform+kernel supports the feature, not just raw CPU.

Yeah, that's not always trivial, as I'm sure you know. Especially if it is
a complicated feature like, SNP, for example, which needs fw and platform to
be configured properly and so on.

> And because that utility is not available by default on most targets I care
> about, and having to build and copy over a binary is annoying (though this
> is a minor gripe).

I'm keeping that thing as simple as possible on purpose. So if you wanna make
it available on such targets, I'm all ears.
 
> That said, what I really want in most cases is to know if _KVM_ supports
> a feature.  I'll think more on this, I have a few vague ideas for getting
> a pile of information out of KVM without needing to add more uABI.

That's exactly my pet peeve - making it a uABI and then supporting it foreva.

We have tried to explain what cpuinfo should be:

Documentation/arch/x86/cpuinfo.rst

The gist of it is:

"So, the current use of /proc/cpuinfo is to show features which the kernel has
*enabled* and *supports*. As in: the CPUID feature flag is there, there's an
additional setup which the kernel has done while booting and the functionality
is ready to use. A perfect example for that is "user_shstk" where additional
code enablement is present in the kernel to support shadow stack for user
programs."

So if it is something that has been enabled and is actively supported, then
sure, ofc. What I don't want to have there is a partial mirror of every
possible CPUID flag which is going to be a senseless and useless madness.

Dunno, I guess if we had a

"virt: ..."

line in /proc/cpuinfo which has flags of what the hypervisor has enabled as
a feature, it might not be such a wrong idea... with the above caveats, ofc.
I don't think you want a flurry of patches setting all possible flags just
because.

Or maybe somewhere else where you can query it conveniently...
Manali Shukla Sept. 20, 2024, 5:53 a.m. UTC | #6 
On 8/30/2024 1:51 PM, Borislav Petkov wrote:
> On Thu, Aug 29, 2024 at 09:42:40PM -0700, Sean Christopherson wrote:
>> Ah, sorry, if the platform+kernel supports the feature, not just raw CPU.
> 
> Yeah, that's not always trivial, as I'm sure you know. Especially if it is
> a complicated feature like, SNP, for example, which needs fw and platform to
> be configured properly and so on.
> 
>> And because that utility is not available by default on most targets I care
>> about, and having to build and copy over a binary is annoying (though this
>> is a minor gripe).
> 
> I'm keeping that thing as simple as possible on purpose. So if you wanna make
> it available on such targets, I'm all ears.
>  
>> That said, what I really want in most cases is to know if _KVM_ supports
>> a feature.  I'll think more on this, I have a few vague ideas for getting
>> a pile of information out of KVM without needing to add more uABI.
> 
> That's exactly my pet peeve - making it a uABI and then supporting it foreva.
> 
> We have tried to explain what cpuinfo should be:
> 
> Documentation/arch/x86/cpuinfo.rst
> 
> The gist of it is:
> 
> "So, the current use of /proc/cpuinfo is to show features which the kernel has
> *enabled* and *supports*. As in: the CPUID feature flag is there, there's an
> additional setup which the kernel has done while booting and the functionality
> is ready to use. A perfect example for that is "user_shstk" where additional
> code enablement is present in the kernel to support shadow stack for user
> programs."
> 
> So if it is something that has been enabled and is actively supported, then
> sure, ofc. What I don't want to have there is a partial mirror of every
> possible CPUID flag which is going to be a senseless and useless madness.
> 
> Dunno, I guess if we had a
> 
> "virt: ..."
> 
> line in /proc/cpuinfo which has flags of what the hypervisor has enabled as
> a feature, it might not be such a wrong idea... with the above caveats, ofc.
> I don't think you want a flurry of patches setting all possible flags just
> because.
> 
> Or maybe somewhere else where you can query it conveniently...
> 

I came up with this patch. Does it look okay?

diff --git a/arch/x86/include/asm/cpufeature.h b/arch/x86/include/asm/cpufeature.h
index 0b9611da6c53..74c52bfd8cf2 100644
--- a/arch/x86/include/asm/cpufeature.h
+++ b/arch/x86/include/asm/cpufeature.h
@@ -41,6 +41,7 @@ enum cpuid_leafs
 #define x86_cap_flag_num(flag) ((flag) >> 5), ((flag) & 31)
 
 extern const char * const x86_cap_flags[NCAPINTS*32];
+extern const char * const x86_virt_flags[NCAPINTS*32];
 extern const char * const x86_power_flags[32];
 #define X86_CAP_FMT "%s"
 #define x86_cap_flag(flag) x86_cap_flags[flag]
diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
index 734940fdb6c1..20f389ee0079 100644
--- a/arch/x86/include/asm/cpufeatures.h
+++ b/arch/x86/include/asm/cpufeatures.h
@@ -382,7 +382,7 @@
 #define X86_FEATURE_V_SPEC_CTRL		(15*32+20) /* "v_spec_ctrl" Virtual SPEC_CTRL */
 #define X86_FEATURE_VNMI		(15*32+25) /* "vnmi" Virtual NMI */
 #define X86_FEATURE_SVME_ADDR_CHK	(15*32+28) /* SVME addr check */
-#define X86_FEATURE_BUS_LOCK_THRESHOLD	(15*32+29) /* "" Bus lock threshold */
+#define X86_VIRT_FEATURE_BUS_LOCK_THRESHOLD	(15*32+29) /* "buslock" Bus lock threshold */
 
 /* Intel-defined CPU features, CPUID level 0x00000007:0 (ECX), word 16 */
 #define X86_FEATURE_AVX512VBMI		(16*32+ 1) /* "avx512vbmi" AVX512 Vector Bit Manipulation instructions*/
diff --git a/arch/x86/kernel/cpu/mkcapflags.sh b/arch/x86/kernel/cpu/mkcapflags.sh
index 68f537347466..3671c7892c56 100644
--- a/arch/x86/kernel/cpu/mkcapflags.sh
+++ b/arch/x86/kernel/cpu/mkcapflags.sh
@@ -62,6 +62,9 @@ trap 'rm "$OUT"' EXIT
 	dump_array "x86_bug_flags" "NBUGINTS*32" "X86_BUG_" "NCAPINTS*32" $2
 	echo ""
 
+	dump_array "x86_virt_flags" "NCAPINTS*32" "X86_VIRT_FEATURE_" "" $2
+	echo ""
+
 	echo "#ifdef CONFIG_X86_VMX_FEATURE_NAMES"
 	echo "#ifndef _ASM_X86_VMXFEATURES_H"
 	echo "#include <asm/vmxfeatures.h>"
diff --git a/arch/x86/kernel/cpu/proc.c b/arch/x86/kernel/cpu/proc.c
index e65fae63660e..3068b0a110e4 100644
--- a/arch/x86/kernel/cpu/proc.c
+++ b/arch/x86/kernel/cpu/proc.c
@@ -103,6 +103,11 @@ static int show_cpuinfo(struct seq_file *m, void *v)
 		if (cpu_has(c, i) && x86_cap_flags[i] != NULL)
 			seq_printf(m, " %s", x86_cap_flags[i]);
 
+	seq_puts(m, "\nvirt\t\t:");
+	for (i = 0; i < 32*NCAPINTS; i++)
+		if (cpu_has(c, i) && x86_virt_flags[i] != NULL)
+			seq_printf(m, " %s", x86_virt_flags[i]);
+
 #ifdef CONFIG_X86_VMX_FEATURE_NAMES
 	if (cpu_has(c, X86_FEATURE_VMX) && c->vmx_capability[0]) {
 		seq_puts(m, "\nvmx flags\t:");


Output for this patch from /proc/cpuinfo looks like below:

flags           : fpu vme de pse tsc msr pae mce cx8 apic sep mtrr pge mca cmov pat pse36 clflush mmx fxsr sse sse2 ht syscall nx mmxext fxsr_opt pdpe1gb rdtscp lm constant_tsc rep_good amd_lbr_v2 nopl xtopology nonstop_tsc cpuid extd_apicid aperfmperf rapl pni pclmulqdq monitor ssse3 fma cx16 pcid sse4_1 sse4_2 x2apic movbe popcnt aes xsave avx f16c rdrand lahf_lm cmp_legacy svm extapic cr8_legacy abm sse4a misalignsse 3dnowprefetch osvw ibs skinit wdt tce topoext perfctr_core perfctr_nb bpext perfctr_llc mwaitx cpb cat_l3 cdp_l3 hw_pstate ssbd mba perfmon_v2 ibrs ibpb stibp vmmcall fsgsbase tsc_adjust bmi1 avx2 smep bmi2 erms invpcid cqm rdt_a avx512f avx512dq rdseed adx smap avx512ifma clflushopt clwb avx512cd sha_ni avx512bw avx512vl xsaveopt xsavec xgetbv1 xsaves cqm_llc cqm_occup_llc cqm_mbm_total cqm_mbm_local avx_vnni avx512_bf16 clzero irperf xsaveerptr rdpru wbnoinvd amd_ppin cppc arat npt lbrv svm_lock nrip_save tsc_scale vmcb_clean flushbyasid decodeassists pausefilter pfthreshold avic v_vmsave_vmload vgif x2avic v_spec_ctrl vnmi avx512vbmi umip pku ospke avx512_vbmi2 gfni vaes vpclmulqdq avx512_vnni avx512_bitalg avx512_vpopcntdq la57 rdpid movdiri movdir64b overflow_recov succor smca fsrm avx512_vp2intersect flush_l1d sev sev_es sev_snp debug_swap amd_lbr_pmc_freeze
virt            : buslock
bugs            : sysret_ss_attrs spectre_v1 spectre_v2 spec_store_bypass

- Manali
diff mbox series

Patch

diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
index 3c7434329661..10f397873790 100644
--- a/arch/x86/include/asm/cpufeatures.h
+++ b/arch/x86/include/asm/cpufeatures.h
@@ -381,6 +381,7 @@ 
 #define X86_FEATURE_V_SPEC_CTRL		(15*32+20) /* Virtual SPEC_CTRL */
 #define X86_FEATURE_VNMI		(15*32+25) /* Virtual NMI */
 #define X86_FEATURE_SVME_ADDR_CHK	(15*32+28) /* "" SVME addr check */
+#define X86_FEATURE_BUS_LOCK_THRESHOLD	(15*32+29) /* "" Bus lock threshold */
 
 /* Intel-defined CPU features, CPUID level 0x00000007:0 (ECX), word 16 */
 #define X86_FEATURE_AVX512VBMI		(16*32+ 1) /* AVX512 Vector Bit Manipulation instructions*/