x86: cpuid: improve support for broken CPUID configurations

PATCH Applied Maxim Levitsky 8 participants 31 messages Jul 18, 2022 – Nov 7, 2022

Subsystems (4)

x86 KVM perf events Crypto API

lore

Share & tools

lore

Maxim Levitsky

author

2022-07-18 14:11 UTC #0 lore raw

v2 cover x86: cpuid: improve support for broken CPUID configurations

This patch series aims to harden the cpuid code against the case when the hypervisor exposes a broken CPUID configuration to the guest, in the form of having a feature disabled but not features that depend on it.

This is the more generic way to fix kernel panic in aes-ni kernel driver, which was triggered by CPUID configuration in which AVX is disabled but not AVX2.

https://lore.kernel.org/all/[email protected]/T/

This was tested by booting a guest with AVX disabled and not AVX2, and observing that both a warning is now printed in dmesg, and that avx2 is gone from /proc/cpuinfo.

V2:

I hopefully addressed all the (very good) review feedback.

Best regards,
Maxim Levitsky

Maxim Levitsky (5):
perf/x86/intel/lbr: use setup_clear_cpu_cap instead of clear_cpu_cap x86/cpuid: refactor setup_clear_cpu_cap()/clear_cpu_cap() x86/cpuid: move filter_cpuid_features to cpuid-deps.c x86/cpuid: remove 'warn' parameter from filter_cpuid_features x86/cpuid: check for dependencies violations in CPUID and attempt to
fix them

 arch/x86/events/intel/lbr.c       |  2 +-
 arch/x86/include/asm/cpufeature.h |  1 +
 arch/x86/kernel/cpu/common.c      | 51 +-------------------
 arch/x86/kernel/cpu/cpuid-deps.c  | 80 +++++++++++++++++++++++++++----
 4 files changed, 74 insertions(+), 60 deletions(-)

Signature · 1 line

2.34.3

Maxim Levitsky

author

2022-07-18 14:11 UTC #1 lore raw

PATCH v2 1/5 perf/x86/intel/lbr: use setup_clear_cpu_cap instead of clear_cpu_cap

clear_cpu_cap(&boot_cpu_data) is very similar to setup_clear_cpu_cap except that the latter also sets a bit in 'cpu_caps_cleared' which later clears the same cap in secondary cpus, which is likely what is meant here.

Fixes 47125db27e47 ("perf/x86/intel/lbr: Support Architectural LBR")

Reviewed-by Kan Liang <[email protected]>

Signed-off-by Maxim Levitsky <[email protected]>

---
 arch/x86/events/intel/lbr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

arch/x86/events/intel/lbr.c +1 -1

index 13179f31fe10fa..b08715172309a7 100644
@@ -1860,7 +1860,7 @@ void __init intel_pmu_arch_lbr_init(void)
 	return;
 
 clear_arch_lbr:
-	clear_cpu_cap(&boot_cpu_data, X86_FEATURE_ARCH_LBR);
+	setup_clear_cpu_cap(X86_FEATURE_ARCH_LBR);
 }
 
 /**

Signature · 1 line

2.34.3

Borislav Petkov

maintainer

2022-09-19 14:31 UTC #2 lore raw

Re: [PATCH v2 1/5] perf/x86/intel/lbr: use setup_clear_cpu_cap instead of clear_cpu_cap

Quoted 18 lines · Maxim Levitsky

clear_cpu_cap(&boot_cpu_data) is very similar to setup_clear_cpu_cap except that the latter also sets a bit in 'cpu_caps_cleared' which later clears the same cap in secondary cpus, which is likely what is meant here.
Fixes 47125db27e47 ("perf/x86/intel/lbr: Support Architectural LBR")
Reviewed-by Kan Liang <[email protected]>
Signed-off-by Maxim Levitsky <[email protected]>
---
 arch/x86/events/intel/lbr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
arch/x86/events/intel/lbr.c +0 -0
index 13179f31fe10fa..b08715172309a7 100644

arch/x86/events/intel/lbr.c void __init intel_pmu_arch_lbr_init(void) · line 1863

Quoted 5 lines

arch/x86/events/intel/lbr.c +1 -1

@@ -1860,7 +1860,7 @@ void __init intel_pmu_arch_lbr_init(void)
 	return;
 
 clear_arch_lbr:
-	clear_cpu_cap(&boot_cpu_data, X86_FEATURE_ARCH_LBR);
+	setup_clear_cpu_cap(X86_FEATURE_ARCH_LBR);

setup_clear_cpu_cap() has a very specific purpose - see apply_forced_caps().

This whole call sequence is an early_initcall() which is way after the whole CPU features picking apart happens.

So what is actually this fixing?

Signature · 4 lines

Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Maxim Levitsky

author

2022-09-20 08:23 UTC #3 lore raw

Re: [PATCH v2 1/5] perf/x86/intel/lbr: use setup_clear_cpu_cap instead of clear_cpu_cap

Quoted 28 lines · Borislav Petkov

Quoted 19 lines · Maxim Levitsky
clear_cpu_cap(&boot_cpu_data) is very similar to setup_clear_cpu_cap except that the latter also sets a bit in 'cpu_caps_cleared' which later clears the same cap in secondary cpus, which is likely what is meant here.
Fixes 47125db27e47 ("perf/x86/intel/lbr: Support Architectural LBR")
Reviewed-by Kan Liang <[email protected]>
Signed-off-by Maxim Levitsky <[email protected]>
---
arch/x86/events/intel/lbr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
arch/x86/events/intel/lbr.c +0 -0
index 13179f31fe10fa..b08715172309a7 100644
@@ -1860,7 +1860,7 @@ void __init intel_pmu_arch_lbr_init(void)
         return;
Quoted 4 lines

clear_arch_lbr:
- clear_cpu_cap(&boot_cpu_data, X86_FEATURE_ARCH_LBR); + setup_clear_cpu_cap(X86_FEATURE_ARCH_LBR);
setup_clear_cpu_cap() has a very specific purpose - see apply_forced_caps().

Quoted 5 lines

This whole call sequence is an early_initcall() which is way after the whole CPU features picking apart happens.
So what is actually this fixing?

If I understand that correctly, the difference between clear_cpu_cap and setup_clear_cpu_cap is that setup_clear_cpu_cap should be called early when only the boot cpu is running and it

1. works on 'boot_cpu_data' which represents the boot cpu.
2. sets a bit in 'cpu_caps_cleared' which are later applied to all CPUs, including these that are hotplugged.

On the other hand the clear_cpu_cap just affects the given 'struct cpuinfo_x86'.

Call of 'clear_cpu_cap(&boot_cpu_data, X86_FEATURE_ARCH_LBR)' is weird since it still affects 'boot_cpu_data' but doesn't affect 'cpu_caps_cleared'

I assumed that this was a mistake and the intention was to disable the feature on all CPUs.

I need this patch because in the next patch, I change the clear_cpu_cap such as it detects being called on boot_cpu_data and in this case also clears bits in 'cpu_caps_cleared', thus while this patch does introduce a functional change, the next patch doesn't since this is the only place where clear_cpu_cap is called explicitly on 'boot_cpu_data'

I do now notice that initcalls are run after smp is initialized, which means that this code doesn't really disable the CPUID feature on all CPUs at all.

Maybe we can drop the call instead, which does seem to be wrong?

Best regards,
Maxim Levitsky

Borislav Petkov

maintainer

2022-09-26 14:48 UTC #4 lore raw

Re: [PATCH v2 1/5] perf/x86/intel/lbr: use setup_clear_cpu_cap instead of clear_cpu_cap

Quoted 5 lines · Maxim Levitsky

If I understand that correctly, the difference between clear_cpu_cap and setup_clear_cpu_cap is that setup_clear_cpu_cap should be called early when only the boot cpu is running and it
1. works on 'boot_cpu_data' which represents the boot cpu.
2. sets a bit in 'cpu_caps_cleared' which are later applied to all CPUs, including these that are hotplugged.

Yes.

Quoted 1 line

On the other hand the clear_cpu_cap just affects the given 'struct cpuinfo_x86'.

Yes.

Quoted 2 lines

Call of 'clear_cpu_cap(&boot_cpu_data, X86_FEATURE_ARCH_LBR)' is weird since it still affects 'boot_cpu_data' but doesn't affect 'cpu_caps_cleared'

Yes.

Quoted 1 line

I assumed that this was a mistake and the intention was to disable the feature on all CPUs.

peterz says yes.

Quoted 4 lines

I need this patch because in the next patch, I change the clear_cpu_cap such as it detects being called on boot_cpu_data and in this case also clears bits in 'cpu_caps_cleared', thus while this patch does introduce a functional change, the next patch doesn't since this is the only place where clear_cpu_cap is called explicitly on 'boot_cpu_data'

This is not needed - this patch doing setup_clear_cpu_cap() should suffice.

But, there must be something you're fixing with this. Which is it? Some weird virt config?

Quoted 3 lines

I do now notice that initcalls are run after smp is initialized, which means that this code doesn't really disable the CPUID feature on all CPUs at all.

Well, not exactly. There's do_pre_smp_calls() which is where the early_initcall() thing is run.

So setup_clear_cpu_cap() will make sure that the feature bit is cleared when the APs come online.

Do you have a virt configuration where you can test this case where the feature flag is clear on all CPUs when it fails?

I.e., "arch_lbr" will disappear in /proc/cpuinfo completely.

Thx.

Signature · 4 lines

Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Maxim Levitsky

author

2022-09-28 10:52 UTC #5 lore raw

Re: [PATCH v2 1/5] perf/x86/intel/lbr: use setup_clear_cpu_cap instead of clear_cpu_cap

Quoted 27 lines · Borislav Petkov

Quoted 5 lines · Maxim Levitsky
If I understand that correctly, the difference between clear_cpu_cap and setup_clear_cpu_cap is that setup_clear_cpu_cap should be called early when only the boot cpu is running and it
1. works on 'boot_cpu_data' which represents the boot cpu.
2. sets a bit in 'cpu_caps_cleared' which are later applied to all CPUs, including these that are hotplugged.
Yes.
Quoted 1 line
On the other hand the clear_cpu_cap just affects the given 'struct cpuinfo_x86'.
Yes.
Quoted 2 lines
Call of 'clear_cpu_cap(&boot_cpu_data, X86_FEATURE_ARCH_LBR)' is weird since it still affects 'boot_cpu_data' but doesn't affect 'cpu_caps_cleared'
Yes.
Quoted 1 line
I assumed that this was a mistake and the intention was to disable the feature on all CPUs.
peterz says yes.
Quoted 4 lines
I need this patch because in the next patch, I change the clear_cpu_cap such as it detects being called on boot_cpu_data and in this case also clears bits in 'cpu_caps_cleared', thus while this patch does introduce a functional change, the next patch doesn't since this is the only place where clear_cpu_cap is called explicitly on 'boot_cpu_data'

Quoted 4 lines

This is not needed - this patch doing setup_clear_cpu_cap() should suffice.
But, there must be something you're fixing with this. Which is it? Some weird virt config?

Patches 1-3 don't fix anything - these are just refactoring to make the code simplier.

This particular patch is done to enable the refactoring in the next patch by removing an (hopefully broken) outlier.

Patch 4 is small fix in the sense that it allows the warning from the current cpuid filtering code to be seen (it is supressed in early code, but then it doesn't usually happen again, so no warning is printed at all)

Patch 5 is the main fix - it makes the kernel to be tolerant to a broken CPUID config (coming hopefully from hypervisor),
where you have a feature (AVX2 in my case) but not a feature on which this feature depends (AVX).

Quoted 7 lines

Quoted 4 lines
I do now notice that initcalls are run after smp is initialized, which means that this code doesn't really disable the CPUID feature on all CPUs at all.
Well, not exactly. There's do_pre_smp_calls() which is where the early_initcall() thing is run.

Aha! I was reading the 'do_initcalls()' code and thought that it goes over all initcalls.

note that it turns out that this function is called 'do_pre_smp_initcalls()'.

Quoted 6 lines

So setup_clear_cpu_cap() will make sure that the feature bit is cleared when the APs come online.
Do you have a virt configuration where you can test this case where the feature flag is clear on all CPUs when it fails?

This needs the arch lbrs which aren't yet supported by KVM (there are patches on the mailing list), plus I need a hardware which supportes them, of which I don't know even if intel released any yet.

I can hack the code/KVM though to simulate enough of it to see if this failback happens.

Besides that, anything else I should do to to see that patch series merged?

Thanks,
Best regards,
Maxim Levitsky

Quoted 5 lines

I.e., "arch_lbr" will disappear in /proc/cpuinfo completely.
Thx.

Borislav Petkov

maintainer

2022-10-20 09:00 UTC #6 lore raw

Re: [PATCH v2 1/5] perf/x86/intel/lbr: use setup_clear_cpu_cap instead of clear_cpu_cap

Quoted 4 lines · Maxim Levitsky

Patch 5 is the main fix - it makes the kernel to be tolerant to a broken CPUID config (coming hopefully from hypervisor), where you have a feature (AVX2 in my case) but not a feature on which this feature depends (AVX).

I really really don't like it when people are fixing the wrong thing.

Why does the kernel need to get fixed when something else can't get its CPUID dependencies straight? I don't even want to know why something would set AVX2 without AVX?!?!

Srsly.

Some of your other bits look sensible and I'd take a deeper look but this does not make any sense. This is a hypervisor problem - not a kernel one.

Thx.

Signature · 4 lines

Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Herbert Xu

maintainer

2022-10-20 09:06 UTC #7 lore raw

Re: [PATCH v2 1/5] perf/x86/intel/lbr: use setup_clear_cpu_cap instead of clear_cpu_cap

Quoted 6 lines · Borislav Petkov

I really really don't like it when people are fixing the wrong thing.
Why does the kernel need to get fixed when something else can't get its CPUID dependencies straight? I don't even want to know why something would set AVX2 without AVX?!?!

That's exactly what I said when this was first reported to me as a crypto bug :)

Cheers,
--
Email: Herbert Xu <[email protected]> Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Maxim Levitsky

author

2022-10-20 10:24 UTC #8 lore raw

Re: [PATCH v2 1/5] perf/x86/intel/lbr: use setup_clear_cpu_cap instead of clear_cpu_cap

Quoted 9 lines · Herbert Xu

Quoted 5 lines · Borislav Petkov
I really really don't like it when people are fixing the wrong thing.
Why does the kernel need to get fixed when something else can't get its CPUID dependencies straight? I don't even want to know why something would set AVX2 without AVX?!?!
That's exactly what I said when this was first reported to me as a crypto bug :)

I agree with you, however this patch series is just refactoring/hardening of the kernel - if the kernel can avoid crashing - why not.

Of course the hypervisor should not present such broken configurations to the guest - in fact the guest kernel can't fix this - guest userspace will still see wrong CPUID and can still crash.

TL;DR - this patch series is not intended to workaround a broken hypervisor and such, it is just a hardening against misconfiguration.

Best regards,
Maxim Levitsky

Quoted 7 lines

Cheers,
--
Email: Herbert Xu <[email protected]> Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Borislav Petkov

maintainer

2022-10-20 11:13 UTC #9 lore raw

Re: [PATCH v2 1/5] perf/x86/intel/lbr: use setup_clear_cpu_cap instead of clear_cpu_cap

Quoted 3 lines · Maxim Levitsky

I agree with you, however this patch series is just refactoring/hardening of the kernel - if the kernel can avoid crashing
- why not.

Because we're already drowning in patches which are trying to fix real problems. If we open the floodgates on alleged hardening just because some other part of the stack is misbehaving, there'll be no end of it.

Thx.

Signature · 4 lines

Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

H. Peter Anvin

reviewer

2022-10-22 21:10 UTC #10 lore raw

Re: [PATCH v2 1/5] perf/x86/intel/lbr: use setup_clear_cpu_cap instead of clear_cpu_cap

In reply to Borislav Petkov

Quoted 12 lines · Borislav Petkov

Quoted 4 lines · Maxim Levitsky
Patch 5 is the main fix - it makes the kernel to be tolerant to a broken CPUID config (coming hopefully from hypervisor), where you have a feature (AVX2 in my case) but not a feature on which this feature depends (AVX).
I really really don't like it when people are fixing the wrong thing.
Why does the kernel need to get fixed when something else can't get its CPUID dependencies straight? I don't even want to know why something would set AVX2 without AVX?!?!

Quoted 8 lines

Srsly.
Some of your other bits look sensible and I'd take a deeper look but this does not make any sense. This is a hypervisor problem - not a kernel one.
Thx.

Yes, this is utterly nonsensical and it will break user space applications left, right, and center.

Paolo Bonzini

maintainer

2022-11-02 13:41 UTC #11 lore raw

Re: [PATCH v2 1/5] perf/x86/intel/lbr: use setup_clear_cpu_cap instead of clear_cpu_cap

In reply to Borislav Petkov

Quoted 11 lines · Borislav Petkov

Quoted 4 lines · Maxim Levitsky
Patch 5 is the main fix - it makes the kernel to be tolerant to a broken CPUID config (coming hopefully from hypervisor), where you have a feature (AVX2 in my case) but not a feature on which this feature depends (AVX).
I really really don't like it when people are fixing the wrong thing.
Why does the kernel need to get fixed when something else can't get its CPUID dependencies straight? I don't even want to know why something would set AVX2 without AVX?!?!

Users do so because they just "disable AVX" (e.g. in QEMU -cpu host,-avx) and that removes the AVX bit. Userspace didn't bother to implement the whole set of CPUID bit dependencies for AVX because:

1) Intel is adding AVX features every other week and probably half the time people would forget to add the dependency

2) anyway you absolutely need to check XCR0 before using AVX, which in the kernel is done using cpu_has_xfeatures(XFEATURE_MASK_YMM), and userspace *does* remove the XSAVE state from 0Dh leaf if you remove AVX.

(2) in particular holds even on bare metal. The kernel bug here is that X86_FEATURE_AVX only tells you if the instructions are _present_, not if they are _usable_. Indeed, the XCR0 check is present for all other files in arch/x86/crypto, either instead or in addition to boot_cpu_has(X86_FEATURE_AVX).

Maxim had sent a patch about a year ago to do it in aesni-intel-glue.c but Dave told him to fix the dependencies instead (https://lore.kernel.org/all/[email protected]/).
What do you think of applying that patch instead?

Thanks,

Paolo

Elliott, Robert (Servers)

2022-11-02 14:29 UTC #12 lore raw

RE: [PATCH v2 1/5] perf/x86/intel/lbr: use setup_clear_cpu_cap instead of clear_cpu_cap

Quoted 2 lines

From: Paolo Bonzini <[email protected]>

...

Quoted 11 lines

(2) in particular holds even on bare metal. The kernel bug here is that X86_FEATURE_AVX only tells you if the instructions are _present_, not if
they are _usable_. Indeed, the XCR0 check is present for all other
files in arch/x86/crypto, either instead or in addition to boot_cpu_has(X86_FEATURE_AVX).
Maxim had sent a patch about a year ago to do it in aesni-intel-glue.c but Dave told him to fix the dependencies instead
(https://lore.kernel.org/all/20211103124614.499580-1-
[email protected]/).
What do you think of applying that patch instead?

Most of the x86 crypto modules using X86_FEATURE_AVX do check
cpu_has_xfeatures(XFEATURE_MASK_YMM, ...

so it's probably prudent to add it to the rest (or remove it everywhere if it is not needed).

1. Currently checking XSAVE YMM:
aria_aesni_avx_glue
blake2s-glue
camellia_aesni_avx2_glue camellia_aesni_avx_glue
cast5_avx_glue cast6_avx_glue
chacha_glue
poly1305_glue
serpent_avx2_glue serpent_avx_glue
sha1_ssse3_glue sha256_ssse3_glue sha512_ssse3_glue
sm3_avx_glue
sm4_aesni_avx2_glue sm4_aesni_avx_glue
twofish_avx_glue

Currently not checking XSAVE YMM:
aesni-intel_glue
curve25519-x86_64
nhpoly1305-avx2-glue
polyval-clmulni_glue

2. Similarly, modules using X86_FEATURE_AVX512F, X86_FEATURE_AVXX512VL and/or X86_FEATURE_AVX512BW probably need to check XFEATURE_MASK_AVX512:

Currently checking XSAVE AVX512:
blake2s-glue
poly1305_glue

Currently not checking XSAVE AVX512:
chacha_glue

3. Similarly, modules using X86_FEATURE_XMM2 probably need to check XFEATURE_MASK_SSE:

Currently checking XSAVE SSE:
aegis128-aesni-glue

Current not checking XSAVE SSE:
nhpoly1305-sse2_glue
serpent_sse2_glue

H. Peter Anvin

reviewer

2022-11-02 16:29 UTC #13 lore raw

RE: [PATCH v2 1/5] perf/x86/intel/lbr: use setup_clear_cpu_cap instead of clear_cpu_cap

Quoted 53 lines · Robert (Servers)

Quoted 2 lines
From: Paolo Bonzini <[email protected]>
...
Quoted 11 lines
(2) in particular holds even on bare metal. The kernel bug here is that X86_FEATURE_AVX only tells you if the instructions are _present_, not if
they are _usable_. Indeed, the XCR0 check is present for all other
files in arch/x86/crypto, either instead or in addition to boot_cpu_has(X86_FEATURE_AVX).
Maxim had sent a patch about a year ago to do it in aesni-intel-glue.c but Dave told him to fix the dependencies instead
(https://lore.kernel.org/all/20211103124614.499580-1-
[email protected]/).
What do you think of applying that patch instead?
Most of the x86 crypto modules using X86_FEATURE_AVX do check
cpu_has_xfeatures(XFEATURE_MASK_YMM, ...
so it's probably prudent to add it to the rest (or remove it everywhere if it is not needed).
1. Currently checking XSAVE YMM:
aria_aesni_avx_glue
blake2s-glue
camellia_aesni_avx2_glue camellia_aesni_avx_glue
cast5_avx_glue cast6_avx_glue
chacha_glue
poly1305_glue
serpent_avx2_glue serpent_avx_glue
sha1_ssse3_glue sha256_ssse3_glue sha512_ssse3_glue
sm3_avx_glue
sm4_aesni_avx2_glue sm4_aesni_avx_glue
twofish_avx_glue
Currently not checking XSAVE YMM:
aesni-intel_glue
curve25519-x86_64
nhpoly1305-avx2-glue
polyval-clmulni_glue
2. Similarly, modules using X86_FEATURE_AVX512F, X86_FEATURE_AVXX512VL and/or X86_FEATURE_AVX512BW probably need to check XFEATURE_MASK_AVX512:
Currently checking XSAVE AVX512:
blake2s-glue
poly1305_glue
Currently not checking XSAVE AVX512:
chacha_glue
3. Similarly, modules using X86_FEATURE_XMM2 probably need to check XFEATURE_MASK_SSE:

Quoted 9 lines

Currently checking XSAVE SSE:
aegis128-aesni-glue
Current not checking XSAVE SSE:
nhpoly1305-sse2_glue
serpent_sse2_glue

We have a dependency system for CPUID features. If you are going to do this (as opposed to "fixing" this in Qemu or just saying "don't do that, it isn't a valid hardware configuration."

H. Peter Anvin

reviewer

2022-11-02 18:19 UTC #14 lore raw

RE: [PATCH v2 1/5] perf/x86/intel/lbr: use setup_clear_cpu_cap instead of clear_cpu_cap

Quoted 57 lines · H. Peter Anvin

Quoted 56 lines · Robert (Servers)
Quoted 2 lines
From: Paolo Bonzini <[email protected]>
...
Quoted 11 lines
(2) in particular holds even on bare metal. The kernel bug here is that X86_FEATURE_AVX only tells you if the instructions are _present_, not if
they are _usable_. Indeed, the XCR0 check is present for all other
files in arch/x86/crypto, either instead or in addition to boot_cpu_has(X86_FEATURE_AVX).
Maxim had sent a patch about a year ago to do it in aesni-intel-glue.c but Dave told him to fix the dependencies instead
(https://lore.kernel.org/all/20211103124614.499580-1-
[email protected]/).
What do you think of applying that patch instead?
Most of the x86 crypto modules using X86_FEATURE_AVX do check
cpu_has_xfeatures(XFEATURE_MASK_YMM, ...
so it's probably prudent to add it to the rest (or remove it everywhere if it is not needed).
1. Currently checking XSAVE YMM:
aria_aesni_avx_glue
blake2s-glue
camellia_aesni_avx2_glue camellia_aesni_avx_glue
cast5_avx_glue cast6_avx_glue
chacha_glue
poly1305_glue
serpent_avx2_glue serpent_avx_glue
sha1_ssse3_glue sha256_ssse3_glue sha512_ssse3_glue
sm3_avx_glue
sm4_aesni_avx2_glue sm4_aesni_avx_glue
twofish_avx_glue
Currently not checking XSAVE YMM:
aesni-intel_glue
curve25519-x86_64
nhpoly1305-avx2-glue
polyval-clmulni_glue
2. Similarly, modules using X86_FEATURE_AVX512F, X86_FEATURE_AVXX512VL and/or X86_FEATURE_AVX512BW probably need to check XFEATURE_MASK_AVX512:
Currently checking XSAVE AVX512:
blake2s-glue
poly1305_glue
Currently not checking XSAVE AVX512:
chacha_glue
3. Similarly, modules using X86_FEATURE_XMM2 probably need to check XFEATURE_MASK_SSE:
Currently checking XSAVE SSE:
aegis128-aesni-glue

Quoted 8 lines

Quoted 6 lines
Current not checking XSAVE SSE:
nhpoly1305-sse2_glue
serpent_sse2_glue
We have a dependency system for CPUID features. If you are going to do this (as opposed to "fixing" this in Qemu or just saying "don't do that, it isn't a valid hardware configuration."

One more thing: for obvious reasons, this doesn't fix user space if user space calls CPUID directly as opposed to reading /proc/cpuinfo or looking in sysfs. Unfortunately this is the rule rather than the exception, although for some features like AVX user space is also supposed to check XCR0, in which case it will work properly anyway.

Elliott, Robert (Servers)

2022-11-02 19:16 UTC #15 lore raw

RE: [PATCH v2 1/5] perf/x86/intel/lbr: use setup_clear_cpu_cap instead of clear_cpu_cap

Quoted 9 lines

Quoted 2 lines
We have a dependency system for CPUID features. If you are going to do
this (as opposed to "fixing" this in Qemu or just saying "don't do that, it isn't a valid hardware configuration."
One more thing: for obvious reasons, this doesn't fix user space if user space calls CPUID directly as opposed to reading /proc/cpuinfo or looking in sysfs. Unfortunately this is the rule rather than the exception, although for some features like AVX user space is also supposed to check XCR0, in which case it will work properly anyway.

The x86 crypto modules use boot_cpu_has() to check features before using them.

If that (or some other function that we change them to use) returned false if the necessary XSAVE bits were not set, then they could drop the cpu_has_xfeatures() calls.

arch/x86/kernel/fpu/xstate.c, which provides cpu_has_xfeatures(), and also has an xsave_cpu_features table listing the features needed be each xfeature. Perhaps that should provide a cpu_feature_usable() function that calls boot_cpu_has() and confirms the associated xfeatures are present. That way the logic would be in one place rather than replicated in 20+ crypto modules.

Paolo Bonzini

maintainer

2022-11-03 13:27 UTC #16 lore raw

Re: [PATCH v2 1/5] perf/x86/intel/lbr: use setup_clear_cpu_cap instead of clear_cpu_cap

In reply to H. Peter Anvin

Quoted 3 lines · H. Peter Anvin

We have a dependency system for CPUID features. If you are going to do this (as opposed to "fixing" this in Qemu or just saying "don't do that, it isn't a valid hardware configuration."

I didn't check Robert's full list, but at least in the case of aesni-intel_glue this is not about AVX2-depends-on-AVX or AVX-depends-on-XSAVE (and it is not about QEMU at all). It's just that checking AVX or AVX2 only tells you about presence and is not enough to say whether the instructions are _usable_. Likewise for AVX512.

What would the dependency be?

Paolo

Quoted 41 lines

1. Currently checking XSAVE YMM:
aria_aesni_avx_glue
blake2s-glue
camellia_aesni_avx2_glue camellia_aesni_avx_glue
cast5_avx_glue cast6_avx_glue
chacha_glue
poly1305_glue
serpent_avx2_glue serpent_avx_glue
sha1_ssse3_glue sha256_ssse3_glue sha512_ssse3_glue
sm3_avx_glue
sm4_aesni_avx2_glue sm4_aesni_avx_glue
twofish_avx_glue
Currently not checking XSAVE YMM:
aesni-intel_glue
curve25519-x86_64
nhpoly1305-avx2-glue
polyval-clmulni_glue
2. Similarly, modules using X86_FEATURE_AVX512F, X86_FEATURE_AVXX512VL and/or X86_FEATURE_AVX512BW probably need to check XFEATURE_MASK_AVX512:
Currently checking XSAVE AVX512:
blake2s-glue
poly1305_glue
Currently not checking XSAVE AVX512:
chacha_glue
3. Similarly, modules using X86_FEATURE_XMM2 probably need to check XFEATURE_MASK_SSE:
Currently checking XSAVE SSE:
aegis128-aesni-glue
Current not checking XSAVE SSE:
nhpoly1305-sse2_glue
serpent_sse2_glue

Paolo Bonzini

maintainer

2022-11-03 13:32 UTC #17 lore raw

Re: [PATCH v2 1/5] perf/x86/intel/lbr: use setup_clear_cpu_cap instead of clear_cpu_cap

In reply to Elliott, Robert (Servers)

Quoted 10 lines · Robert (Servers)

3. Similarly, modules using X86_FEATURE_XMM2 probably need to check XFEATURE_MASK_SSE:
Currently checking XSAVE SSE:
   aegis128-aesni-glue
Current not checking XSAVE SSE:
   nhpoly1305-sse2_glue
   serpent_sse2_glue

These should check boot_cpu_has(X86_FEATURE_FXSR). Checking XFEATURE_MASK_SSE will fail on systems without XSAVE, because fpu_kernel_cfg.max_features is zero there (see fpu__init_system_xstate() in arch/x86/kernel/fpu/xstate.c).

It happens to work for aegis128-aesni-glue because AES instructions only exist on new-enough parts, but it should probably be changed as well.

Paolo

tip-bot2 for Maxim Levitsky

merge bot Applied

2022-10-20 15:19 UTC #18 lore raw

[tip: x86/urgent] perf/x86/intel/lbr: Use setup_clear_cpu_cap() instead of clear_cpu_cap()

In reply to Maxim Levitsky

The following commit has been merged into the x86/urgent branch of tip:

Commit-ID:     b329f5ddc9ce4b622d9c7aaf5c6df4de52caf91a
Gitweb:        https://git.kernel.org/tip/b329f5ddc9ce4b622d9c7aaf5c6df4de52caf91a
Author:        Maxim Levitsky <[email protected]>
AuthorDate:    Mon, 18 Jul 2022 17:11:19 +03:00
Committer:     Peter Zijlstra <[email protected]>
CommitterDate: Thu, 20 Oct 2022 17:10:28 +02:00

perf/x86/intel/lbr: Use setup_clear_cpu_cap() instead of clear_cpu_cap()

clear_cpu_cap(&boot_cpu_data) is very similar to setup_clear_cpu_cap() except that the latter also sets a bit in 'cpu_caps_cleared' which later clears the same cap in secondary cpus, which is likely what is meant here.

Fixes 47125db27e47 ("perf/x86/intel/lbr: Support Architectural LBR")

Signed-off-by Maxim Levitsky <[email protected]>

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

Reviewed-by Kan Liang <[email protected]>

Link https://lkml.kernel.org/r/[email protected] open here

---
 arch/x86/events/intel/lbr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

arch/x86/events/intel/lbr.c +1 -1

index 4fce1a4..8259d72 100644
@@ -1596,7 +1596,7 @@ void __init intel_pmu_arch_lbr_init(void)
 	return;
 
 clear_arch_lbr:
-	clear_cpu_cap(&boot_cpu_data, X86_FEATURE_ARCH_LBR);
+	setup_clear_cpu_cap(X86_FEATURE_ARCH_LBR);
 }
 
 /**

Maxim Levitsky

author

2022-07-18 14:11 UTC #19 lore raw

PATCH v2 2/5 x86/cpuid: refactor setup_clear_cpu_cap()/clear_cpu_cap()

In reply to Maxim Levitsky

Currently setup_clear_cpu_cap passes NULL 'struct cpuinfo_x86*'
to clear_cpu_cap to indicate that capability should be cleared from boot_cpu_data.

Later that is used in clear_feature to do recursive call to clear_cpu_cap together with clearing the feature bit from 'cpu_caps_cleared'

Remove that code and just call the do_clear_cpu_cap on boot_cpu_data directly from the setup_clear_cpu_cap.

The only functional change this introduces is that now calling clear_cpu_cap explicitly on boot_cpu_data also sets the bits in cpu_caps_cleared, which is the only thing that makes sense anyway.

All callers of both functions were checked for this and fixed.

Signed-off-by Maxim Levitsky <[email protected]>

---
 arch/x86/kernel/cpu/cpuid-deps.c | 17 +++++------------
 1 file changed, 5 insertions(+), 12 deletions(-)

arch/x86/kernel/cpu/cpuid-deps.c +5 ~~-12~~

index c881bcafba7d70..d6777d07ba3302 100644
@@ -88,18 +88,16 @@ static inline void clear_feature(struct cpuinfo_x86 *c, unsigned int feature)
 	 * rest of the cpufeature code uses atomics as well, so keep it for
 	 * consistency. Cleanup all of it separately.
 	 */
-	if (!c) {
-		clear_cpu_cap(&boot_cpu_data, feature);
+	clear_bit(feature, (unsigned long *)c->x86_capability);
+
+	if (c == &boot_cpu_data)
 		set_bit(feature, (unsigned long *)cpu_caps_cleared);
-	} else {
-		clear_bit(feature, (unsigned long *)c->x86_capability);
-	}
 }
 
 /* Take the capabilities and the BUG bits into account */
 #define MAX_FEATURE_BITS ((NCAPINTS + NBUGINTS) * sizeof(u32) * 8)
 
-static void do_clear_cpu_cap(struct cpuinfo_x86 *c, unsigned int feature)
+void clear_cpu_cap(struct cpuinfo_x86 *c, unsigned int feature)
 {
 	DECLARE_BITMAP(disable, MAX_FEATURE_BITS);
 	const struct cpuid_dep *d;

@@ -129,12 +127,7 @@ static void do_clear_cpu_cap(struct cpuinfo_x86 *c, unsigned int feature)
 	} while (changed);
 }
 
-void clear_cpu_cap(struct cpuinfo_x86 *c, unsigned int feature)
-{
-	do_clear_cpu_cap(c, feature);
-}
-
 void setup_clear_cpu_cap(unsigned int feature)
 {
-	do_clear_cpu_cap(NULL, feature);
+	clear_cpu_cap(&boot_cpu_data, feature);
 }

Signature · 1 line

2.34.3

Borislav Petkov

maintainer

2022-10-21 16:20 UTC #20 lore raw

Re: [PATCH v2 2/5] x86/cpuid: refactor setup_clear_cpu_cap()/clear_cpu_cap()

Quoted 6 lines · Maxim Levitsky

Currently setup_clear_cpu_cap passes NULL 'struct cpuinfo_x86*'
to clear_cpu_cap to indicate that capability should be cleared from boot_cpu_data.
Later that is used in clear_feature to do recursive call to clear_cpu_cap together with clearing the feature bit from 'cpu_caps_cleared'

Quoted 8 lines

Remove that code and just call the do_clear_cpu_cap on boot_cpu_data directly from the setup_clear_cpu_cap.
The only functional change this introduces is that now calling clear_cpu_cap explicitly on boot_cpu_data also sets the bits in cpu_caps_cleared, which is the only thing that makes sense anyway.
All callers of both functions were checked for this and fixed.

Change looks ok. What I can't grok is this sentence: what was checked and fixed where?

What does need fixing and why?

Thx.

Signature · 4 lines

Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Borislav Petkov

maintainer

2022-11-07 19:11 UTC #21 lore raw

Re: [PATCH v2 2/5] x86/cpuid: refactor setup_clear_cpu_cap()/clear_cpu_cap()

Quoted 10 lines · Borislav Petkov

Quoted 9 lines · Maxim Levitsky
Currently setup_clear_cpu_cap passes NULL 'struct cpuinfo_x86*'
to clear_cpu_cap to indicate that capability should be cleared from boot_cpu_data.
Later that is used in clear_feature to do recursive call to clear_cpu_cap together with clearing the feature bit from 'cpu_caps_cleared'
Remove that code and just call the do_clear_cpu_cap on boot_cpu_data directly from the setup_clear_cpu_cap.

Quoted 8 lines

Quoted 5 lines
The only functional change this introduces is that now calling clear_cpu_cap explicitly on boot_cpu_data also sets the bits in cpu_caps_cleared, which is the only thing that makes sense anyway.
All callers of both functions were checked for this and fixed.
Change looks ok. What I can't grok is this sentence: what was checked and fixed where?

Ok, I think I know what you mean. That:

git grep -E "clear_cpu_cap.*boot"
arch/x86/events/intel/lbr.c:1599: clear_cpu_cap(&boot_cpu_data, X86_FEATURE_ARCH_LBR);
arch/x86/kernel/alternative.c:746: clear_cpu_cap(&boot_cpu_data, X86_FEATURE_UP);

Right, so here's the difference:

When you call setup_clear_cpu_cap(), it basically means, to disable the cap on *every* CPU. This is done with cpu_caps_cleared which gets ANDed in in apply_forced_caps().

clear_cpu_cap() clears the bit *only* in the first parameter supplied.

Now, that first parameter can be boot_cpu_data too but then, strictly speaking, clear_cpu_cap() would really do what you want it to do - to clear it only in its first param.

If you really want to enforce that bit cleared everywhere, you need to use the setup_* variant.

So this patch is actually incorrect but I admit, the CPU caps handling are kinda subtle and probably need cleaning.

Lemme document it so that it is at least clear. Who knows, we might end up improving it in the process.

:-)

Thx.

Signature · 4 lines

Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Borislav Petkov

maintainer

2022-11-07 21:58 UTC #22 lore raw

Re: [PATCH v2 2/5] x86/cpuid: refactor setup_clear_cpu_cap()/clear_cpu_cap()

Quoted 2 lines · Borislav Petkov

Lemme document it so that it is at least clear. Who knows, we might end up improving it in the process.

IOW, something like this. It's a start at least...

https://lore.kernel.org/r/[email protected]

Signature · 4 lines

Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Maxim Levitsky

author

2022-07-18 14:11 UTC #23 lore raw

PATCH v2 3/5 x86/cpuid: move filter_cpuid_features to cpuid-deps.c

In reply to Maxim Levitsky

filter_cpuid_features performs a sanity check on CPU/hypervisor provided CPUID in regard to having all required leaves which some CPUID feature bits require.

Soon this sanity check will be extended to also disable CPUID features which were erronsly enabled in CPUID and depend on features that are marked as disabled in the CPUID.

It thus makes sense to have both checks in one file.

No functional change intended.

Signed-off-by Maxim Levitsky <[email protected]>

---
 arch/x86/include/asm/cpufeature.h |  1 +
 arch/x86/kernel/cpu/common.c      | 47 -------------------------------
 arch/x86/kernel/cpu/cpuid-deps.c  | 47 +++++++++++++++++++++++++++++++
 3 files changed, 48 insertions(+), 47 deletions(-)

arch/x86/include/asm/cpufeature.h +1 -0

index ea34cc31b0474f..3eb5fe0d654e63 100644
@@ -147,6 +147,7 @@ extern const char * const x86_bug_flags[NBUGINTS*32];
 
 extern void setup_clear_cpu_cap(unsigned int bit);
 extern void clear_cpu_cap(struct cpuinfo_x86 *c, unsigned int bit);
+extern void filter_cpuid_features(struct cpuinfo_x86 *c, bool warn);
 
 #define setup_force_cpu_cap(bit) do { \
 	set_cpu_cap(&boot_cpu_data, bit);	\

arch/x86/kernel/cpu/common.c +0 ~~-47~~

53 lines not on this page · raw message

arch/x86/kernel/cpu/cpuid-deps.c +47 -0

50 lines not on this page · raw message

Signature · 1 line

2.34.3

Maxim Levitsky

author

2022-07-18 14:12 UTC #24 lore raw

PATCH v2 4/5 x86/cpuid: remove 'warn' parameter from filter_cpuid_features

In reply to Maxim Levitsky

This parameter suppresses the warning if issue is found when this function called from early boot cpu identification code, which doesn't seem to be useful, except that it actually completely hides the warning, because next time this code is called for each CPU and when the warning is printed, the issue is already fixed, and so no warning is printed at all.

Tested by using a broken CPUID with missing leaf and observing no warning printed, and with the patch a warning is printed once.

Signed-off-by Maxim Levitsky <[email protected]>

---
 arch/x86/include/asm/cpufeature.h | 2 +-
 arch/x86/kernel/cpu/common.c      | 4 ++--
 arch/x86/kernel/cpu/cpuid-deps.c  | 4 +---
 3 files changed, 4 insertions(+), 6 deletions(-)

arch/x86/include/asm/cpufeature.h +1 -1

index 3eb5fe0d654e63..86d7fbb3f2b592 100644
@@ -147,7 +147,7 @@ extern const char * const x86_bug_flags[NBUGINTS*32];
 
 extern void setup_clear_cpu_cap(unsigned int bit);
 extern void clear_cpu_cap(struct cpuinfo_x86 *c, unsigned int bit);
-extern void filter_cpuid_features(struct cpuinfo_x86 *c, bool warn);
+extern void filter_cpuid_features(struct cpuinfo_x86 *c);
 
 #define setup_force_cpu_cap(bit) do { \
 	set_cpu_cap(&boot_cpu_data, bit);	\

arch/x86/kernel/cpu/common.c +2 -2

index beaea42c1b47e1..1a2f4e83e2f312 100644
@@ -1485,7 +1485,7 @@ static void __init early_identify_cpu(struct cpuinfo_x86 *c)
 			this_cpu->c_early_init(c);
 
 		c->cpu_index = 0;
-		filter_cpuid_features(c, false);
+		filter_cpuid_features(c);
 
 		if (this_cpu->c_bsp_init)
 			this_cpu->c_bsp_init(c);

@@ -1773,7 +1773,7 @@ static void identify_cpu(struct cpuinfo_x86 *c)
 	 */
 
 	/* Filter out anything that depends on CPUID levels we don't have */
-	filter_cpuid_features(c, true);
+	filter_cpuid_features(c);
 
 	/* If the model name is still unset, do table lookup. */
 	if (!c->x86_model_id[0]) {

arch/x86/kernel/cpu/cpuid-deps.c +1 -3

index 306f285844aedc..e1b5f5c02c0106 100644
@@ -150,7 +150,7 @@ cpuid_dependent_features[] = {
 	{ 0, 0 }
 };
 
-void filter_cpuid_features(struct cpuinfo_x86 *c, bool warn)
+void filter_cpuid_features(struct cpuinfo_x86 *c)
 {
 	const struct cpuid_dependent_feature *df;

@@ -171,8 +171,6 @@ void filter_cpuid_features(struct cpuinfo_x86 *c, bool warn)
 			continue;
 
 		clear_cpu_cap(c, df->feature);
-		if (!warn)
-			continue;
 
 		pr_warn("CPU: CPU feature " X86_CAP_FMT " disabled, no CPUID level 0x%x\n",
 			x86_cap_flag(df->feature), df->level);

Signature · 1 line

2.34.3

Maxim Levitsky

author

2022-07-18 14:12 UTC #25 lore raw

PATCH v2 5/5 x86/cpuid: check for dependencies violations in CPUID and attempt to fix them

In reply to Maxim Levitsky

Due to configuration bugs, sometimes a CPU feature is disabled in CPUID, but not features that depend on it.

For example, when one attempts to disable AVX2 but not AVX in the guest's CPUID, the guest kernel crashes in aes-ni driver, when it is used.

While the aes-ni driver can also be fixed to be more eager to detect this kind of situation, it is simpler to fix this in a generic way since the kernel has all the required info in the form of a dependency table.

Signed-off-by Maxim Levitsky <[email protected]>

---
 arch/x86/kernel/cpu/cpuid-deps.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

arch/x86/kernel/cpu/cpuid-deps.c +22 -0

index e1b5f5c02c0106..376296c1f55ab2 100644
@@ -94,6 +94,11 @@ static inline void clear_feature(struct cpuinfo_x86 *c, unsigned int feature)
 		set_bit(feature, (unsigned long *)cpu_caps_cleared);
 }
 
+static inline bool test_feature(struct cpuinfo_x86 *c, unsigned int feature)
+{
+	return test_bit(feature, (unsigned long *)c->x86_capability);
+}
+
 /* Take the capabilities and the BUG bits into account */
 #define MAX_FEATURE_BITS ((NCAPINTS + NBUGINTS) * sizeof(u32) * 8)

@@ -136,6 +141,10 @@ void setup_clear_cpu_cap(unsigned int feature)
  * Some CPU features depend on higher CPUID levels, which may not always
  * be available due to CPUID level capping or broken virtualization
  * software.  Add those features to this table to auto-disable them.
+ *
+ * Also due to configuration bugs, some CPUID features might be present
+ * while CPUID features that they depend on are not present,
+ * e.g a AVX2 present but AVX is not present.
  */
 struct cpuid_dependent_feature {
 	u32 feature;

@@ -153,6 +162,7 @@ cpuid_dependent_features[] = {
 void filter_cpuid_features(struct cpuinfo_x86 *c)
 {
 	const struct cpuid_dependent_feature *df;
+	const struct cpuid_dep *d;
 
 	for (df = cpuid_dependent_features; df->feature; df++) {

@@ -175,4 +185,16 @@ void filter_cpuid_features(struct cpuinfo_x86 *c)
 		pr_warn("CPU: CPU feature " X86_CAP_FMT " disabled, no CPUID level 0x%x\n",
 			x86_cap_flag(df->feature), df->level);
 	}
+
+	for (d = cpuid_deps; d->feature; d++) {
+
+		if (!test_feature(c, d->feature) || test_feature(c, d->depends))
+			continue;
+
+		clear_cpu_cap(c, d->feature);
+
+		pr_warn("CPU: CPU feature " X86_CAP_FMT " disabled, because it depends on "
+			X86_CAP_FMT " which is not supported in CPUID\n",
+			x86_cap_flag(d->feature), x86_cap_flag(d->depends));
+	}
 }

Signature · 1 line

2.34.3

Maxim Levitsky

author

2022-07-28 07:30 UTC #26 lore raw

In reply to Maxim Levitsky

Quoted 29 lines · Maxim Levitsky

This patch series aims to harden the cpuid code against the case when the hypervisor exposes a broken CPUID configuration to the guest, in the form of having a feature disabled but not features that depend on it.
This is the more generic way to fix kernel panic in aes-ni kernel driver, which was triggered by CPUID configuration in which AVX is disabled but not AVX2.
https://lore.kernel.org/all/[email protected]/T/
This was tested by booting a guest with AVX disabled and not AVX2, and observing that both a warning is now printed in dmesg, and that avx2 is gone from /proc/cpuinfo.
V2:
I hopefully addressed all the (very good) review feedback.
Best regards,
Maxim Levitsky
Maxim Levitsky (5):
perf/x86/intel/lbr: use setup_clear_cpu_cap instead of clear_cpu_cap x86/cpuid: refactor setup_clear_cpu_cap()/clear_cpu_cap() x86/cpuid: move filter_cpuid_features to cpuid-deps.c x86/cpuid: remove 'warn' parameter from filter_cpuid_features x86/cpuid: check for dependencies violations in CPUID and attempt to
fix them

Quoted 10 lines

 arch/x86/events/intel/lbr.c       |  2 +-
 arch/x86/include/asm/cpufeature.h |  1 +
 arch/x86/kernel/cpu/common.c      | 51 +-------------------
 arch/x86/kernel/cpu/cpuid-deps.c  | 80 +++++++++++++++++++++++++++----
 4 files changed, 74 insertions(+), 60 deletions(-)

Signature · 1 line

2.34.3

A very kind ping on these patches.

Best regards,
Maxim Levitsky

Maxim Levitsky

author

2022-08-01 16:05 UTC #27 lore raw

Quoted 36 lines · Maxim Levitsky

Quoted 35 lines · Maxim Levitsky
This patch series aims to harden the cpuid code against the case when the hypervisor exposes a broken CPUID configuration to the guest, in the form of having a feature disabled but not features that depend on it.
This is the more generic way to fix kernel panic in aes-ni kernel driver, which was triggered by CPUID configuration in which AVX is disabled but not AVX2.
https://lore.kernel.org/all/[email protected]/T/
This was tested by booting a guest with AVX disabled and not AVX2, and observing that both a warning is now printed in dmesg, and that avx2 is gone from /proc/cpuinfo.
V2:
I hopefully addressed all the (very good) review feedback.
Best regards,
Maxim Levitsky
Maxim Levitsky (5):
perf/x86/intel/lbr: use setup_clear_cpu_cap instead of clear_cpu_cap x86/cpuid: refactor setup_clear_cpu_cap()/clear_cpu_cap() x86/cpuid: move filter_cpuid_features to cpuid-deps.c x86/cpuid: remove 'warn' parameter from filter_cpuid_features x86/cpuid: check for dependencies violations in CPUID and attempt to
fix them
 arch/x86/events/intel/lbr.c       |  2 +-
 arch/x86/include/asm/cpufeature.h |  1 +
 arch/x86/kernel/cpu/common.c      | 51 +-------------------
 arch/x86/kernel/cpu/cpuid-deps.c  | 80 +++++++++++++++++++++++++++----
 4 files changed, 74 insertions(+), 60 deletions(-)

Quoted 5 lines

Quoted 4 lines
Signature · 1 line
2.34.3
A very kind ping on these patches.

Another kind ping on these patches.

Best regards,
Maxim Levitsky

Quoted 3 lines

Best regards,
Maxim Levitsky

Dave Hansen

2022-08-01 16:31 UTC #28 lore raw

Quoted 2 lines · Maxim Levitsky

Quoted 1 line
A very kind ping on these patches.
Another kind ping on these patches.

Maxim,

This series is not forgotten. Its latest version was simply posted too close to the merge window. It'll get looked at in a week or two when things calm down.

Please be patient.

Maxim Levitsky

author

2022-08-01 16:41 UTC #29 lore raw

Quoted 4 lines · Dave Hansen

Quoted 2 lines · Maxim Levitsky
Quoted 1 line
A very kind ping on these patches.
Another kind ping on these patches.

Quoted 8 lines

Maxim,
This series is not forgotten. Its latest version was simply posted too close to the merge window. It'll get looked at in a week or two when things calm down.
Please be patient.

Thanks!

Best regards,
Maxim Levitsky

Maxim Levitsky

author

2022-09-19 13:43 UTC #30 lore raw

In reply to Maxim Levitsky

Quoted 37 lines · Maxim Levitsky

Quoted 36 lines · Maxim Levitsky
Quoted 35 lines · Maxim Levitsky
This patch series aims to harden the cpuid code against the case when the hypervisor exposes a broken CPUID configuration to the guest, in the form of having a feature disabled but not features that depend on it.
This is the more generic way to fix kernel panic in aes-ni kernel driver, which was triggered by CPUID configuration in which AVX is disabled but not AVX2.
https://lore.kernel.org/all/[email protected]/T/
This was tested by booting a guest with AVX disabled and not AVX2, and observing that both a warning is now printed in dmesg, and that avx2 is gone from /proc/cpuinfo.
V2:
I hopefully addressed all the (very good) review feedback.
Best regards,
Maxim Levitsky
Maxim Levitsky (5):
perf/x86/intel/lbr: use setup_clear_cpu_cap instead of clear_cpu_cap x86/cpuid: refactor setup_clear_cpu_cap()/clear_cpu_cap() x86/cpuid: move filter_cpuid_features to cpuid-deps.c x86/cpuid: remove 'warn' parameter from filter_cpuid_features x86/cpuid: check for dependencies violations in CPUID and attempt to
fix them
 arch/x86/events/intel/lbr.c       |  2 +-
 arch/x86/include/asm/cpufeature.h |  1 +
 arch/x86/kernel/cpu/common.c      | 51 +-------------------
 arch/x86/kernel/cpu/cpuid-deps.c  | 80 +++++++++++++++++++++++++++----
 4 files changed, 74 insertions(+), 60 deletions(-)

Quoted 7 lines

Quoted 5 lines
Quoted 4 lines
Signature · 1 line
2.34.3
A very kind ping on these patches.
Another kind ping on these patches.

Another very gentle ping on these patches.

Best regards,
Maxim Levitsky

Quoted 6 lines

Best regards,
Maxim Levitsky
Quoted 2 lines
Best regards,
Maxim Levitsky