The following commit has been merged into the x86/urgent branch of tip:
Commit-ID: b9210e56d71d9deb1ad692e405f6b2394f7baa4d
Gitweb: https://git.kernel.org/tip/b9210e56d71d9deb1ad692e405f6b2394f7baa4d
Author: Dave Hansen <[email protected]>
AuthorDate: Fri, 17 May 2024 13:05:34 -07:00
Committer: Dave Hansen <[email protected]>
CommitterDate: Thu, 30 May 2024 07:14:27 -07:00
x86/cpu: Provide default cache line size if not enumerated
tl;dr: CPUs with CPUID.80000008H but without CPUID.01H:EDX[CLFSH]
will end up reporting cache_line_size()==0 and bad things happen.
Fill in a default on those to avoid the problem.
Long Story:
The kernel dies a horrible death if c->x86_cache_alignment (aka.
cache_line_size() is 0. Normally, this value is populated from
c->x86_clflush_size.
Right now the code is set up to get c->x86_clflush_size from two
places. First, modern CPUs get it from CPUID. Old CPUs that don't
have leaf 0x80000008 (or CPUID at all) just get some sane defaults
from the kernel in get_cpu_address_sizes().
The vast majority of CPUs that have leaf 0x80000008 also get
->x86_clflush_size from CPUID. But there are oddballs.
Intel Quark CPUs[1] and others[2] have leaf 0x80000008 but don't set
CPUID.01H:EDX[CLFSH], so they skip over filling in ->x86_clflush_size:
cpuid(0x00000001, &tfms, &misc, &junk, &cap0);
if (cap0 & (1<<19))
c->x86_clflush_size = ((misc >> 8) & 0xff) * 8;
So they: land in get_cpu_address_sizes() and see that CPUID has level
0x80000008 and jump into the side of the if() that does not fill in
c->x86_clflush_size. That assigns a 0 to c->x86_cache_alignment, and
hilarity ensues in code like:
buffer = kzalloc(ALIGN(sizeof(*buffer), cache_line_size()),
GFP_KERNEL);
To fix this, always provide a sane value for ->x86_clflush_size.
Big thanks to Andy Shevchenko for finding and reporting this and also
providing a first pass at a fix. But his fix was only partial and only
worked on the Quark CPUs. It would not, for instance, have worked on
the QEMU config.
1. https://raw.githubusercontent.com/InstLatx64/InstLatx64/master/GenuineIntel/GenuineIntel0000590_Clanton_03_CPUID.txt
2. You can also get this behavior if you use "-cpu 486,+clzero"
in QEMU.
[ dhansen: remove 'vp_bits_from_cpuid' reference in changelog
because bpetkov brutally murdered it recently. ]
Fixes: fbf6449f84bf ("x86/sev-es: Set x86_virt_bits to the correct value straight away, instead of a two-phase approach")
Reported-by: Andy Shevchenko <[email protected]>
Signed-off-by: Dave Hansen <[email protected]>
Tested-by: Andy Shevchenko <[email protected]>
Tested-by: Jörn Heusipp <[email protected]>
Cc: [email protected]
Link: https://lore.kernel.org/all/[email protected]/
Link: https://lore.kernel.org/lkml/[email protected]/
Link: https://lore.kernel.org/all/20240517200534.8EC5F33E%40davehans-spike.ostc.intel.com
---
arch/x86/kernel/cpu/common.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index 2b170da..373b16b 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -1070,6 +1070,10 @@ void get_cpu_address_sizes(struct cpuinfo_x86 *c)
cpu_has(c, X86_FEATURE_PSE36))
c->x86_phys_bits = 36;
}
+
+ /* Provide a sane default if not enumerated: */
+ if (!c->x86_clflush_size)
+ c->x86_clflush_size = 32;
} else {
cpuid(0x80000008, &eax, &ebx, &ecx, &edx);
Hello Dave!
On 30/05/2024 16:23, tip-bot2 for Dave Hansen wrote:
> The following commit has been merged into the x86/urgent branch of tip:
>
> Commit-ID: b9210e56d71d9deb1ad692e405f6b2394f7baa4d
> Gitweb: https://git.kernel.org/tip/b9210e56d71d9deb1ad692e405f6b2394f7baa4d
> Author: Dave Hansen <[email protected]>
> AuthorDate: Fri, 17 May 2024 13:05:34 -07:00
> Committer: Dave Hansen <[email protected]>
> CommitterDate: Thu, 30 May 2024 07:14:27 -07:00
> Tested-by: Jörn Heusipp <[email protected]>
> diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
> index 2b170da..373b16b 100644
> --- a/arch/x86/kernel/cpu/common.c
> +++ b/arch/x86/kernel/cpu/common.c
> @@ -1070,6 +1070,10 @@ void get_cpu_address_sizes(struct cpuinfo_x86 *c)
> cpu_has(c, X86_FEATURE_PSE36))
> c->x86_phys_bits = 36;
> }
> +
> + /* Provide a sane default if not enumerated: */
> + if (!c->x86_clflush_size)
> + c->x86_clflush_size = 32;
> } else {
> cpuid(0x80000008, &eax, &ebx, &ecx, &edx);
>
That honestly looks like a mis-merge to me. This is not what I did test.
x86_clflush_size needs to get set in the top-level else branch in which
it is not already set. commit 95bfb35269b2e85cff0dd2c957b2d42ebf95ae5f
had switched the if branches around.
Best regards,
Jörn
On 5/30/24 08:05, Jörn Heusipp wrote:
>>
>> + /* Provide a sane default if not enumerated: */
>> + if (!c->x86_clflush_size)
>> + c->x86_clflush_size = 32;
>> } else {
>> cpuid(0x80000008, &eax, &ebx, &ecx, &edx);
>>
>
> That honestly looks like a mis-merge to me. This is not what I did test.
> x86_clflush_size needs to get set in the top-level else branch in which
> it is not already set. commit 95bfb35269b2e85cff0dd2c957b2d42ebf95ae5f
> had switched the if branches around.
You're totally right. Thank you for catching that.
I think a variable removal flipped that code around and it confused me
when I did the rebase. It should be fixed up now.
Again, thanks a bunch for the report, the testing, and catching my
screwup quickly!