The initial change to set x86_virt_bits to the correct value straight away broke boot on Intel Quark X1000 CPUs (which are family 5, model 9, stepping 0)
With deeper investigation it appears that the Quark doesn't have the bit 19 set in 0x01 CPUID leaf, which means it doesn't provide any clflush instructions and hence the cache alignment is set to 0. The actual cache line size is 16 bytes, hence we may set the alignment accordingly. At the same time the physical and virtual address bits are retrieved via 0x80000008 CPUID leaf.
Note, we don't really care about the value of x86_clflush_size as it is either used with a proper check for the instruction to be present, or, like in PCI case, it assumes 32 bytes for all supported 32-bit CPUs that have actually smaller cache line sizes and don't advertise it.
The commit fbf6449f84bf ("x86/sev-es: Set x86_virt_bits to the correct value straight away, instead of a two-phase approach") basically revealed the issue that has been present from day 1 of introducing the Quark support.
Fixes: aece118e487a ("x86: Add cpu_detect_cache_sizes to init_intel() add Quark legacy_cache()") Cc: stable@vger.kernel.org Signed-off-by: Andy Shevchenko andriy.shevchenko@linux.intel.com --- arch/x86/kernel/cpu/intel.c | 9 +++++++++ 1 file changed, 9 insertions(+)
diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c index be30d7fa2e66..2bffae158dd5 100644 --- a/arch/x86/kernel/cpu/intel.c +++ b/arch/x86/kernel/cpu/intel.c @@ -321,6 +321,15 @@ static void early_init_intel(struct cpuinfo_x86 *c) #ifdef CONFIG_X86_64 set_cpu_cap(c, X86_FEATURE_SYSENTER32); #else + /* + * The Quark doesn't have bit 19 set in 0x01 CPUID leaf, which means + * it doesn't provide any clflush instructions and hence the cache + * alignment is set to 0. The actual cache line size is 16 bytes, + * hence set the alignment accordingly. At the same time the physical + * and virtual address bits are retrieved via 0x80000008 CPUID leaf. + */ + if (c->x86 == 5 && c->x86_model == 9) + c->x86_cache_alignment = 16; /* Netburst reports 64 bytes clflush size, but does IO in 128 bytes */ if (c->x86 == 15 && c->x86_cache_alignment == 64) c->x86_cache_alignment = 128;
On 5/16/24 10:39, Andy Shevchenko wrote:
The initial change to set x86_virt_bits to the correct value straight away broke boot on Intel Quark X1000 CPUs (which are family 5, model 9, stepping 0)
Do you know what _actually_ broke? Like was there a crash somewhere?
With deeper investigation it appears that the Quark doesn't have the bit 19 set in 0x01 CPUID leaf, which means it doesn't provide any clflush instructions and hence the cache alignment is set to 0. The actual cache line size is 16 bytes, hence we may set the alignment accordingly. At the same time the physical and virtual address bits are retrieved via 0x80000008 CPUID leaf.
This seems to be saying that ->x86_clflush_size must come from CPUID. But there _are_ CPUID-independent defaults set in identify_cpu(). How do those fit in?
Note, we don't really care about the value of x86_clflush_size as it is either used with a proper check for the instruction to be present, or, like in PCI case, it assumes 32 bytes for all supported 32-bit CPUs that have actually smaller cache line sizes and don't advertise it.
Are you trying to say that having ->x86_clflush_size==0 is not fatal while having ->x86_cache_alignment==0 _is_ fatal?
The commit fbf6449f84bf ("x86/sev-es: Set x86_virt_bits to the correct value straight away, instead of a two-phase approach") basically revealed the issue that has been present from day 1 of introducing the Quark support.
How did it do that, exactly? It's still not crystal clear.
diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c index be30d7fa2e66..2bffae158dd5 100644 --- a/arch/x86/kernel/cpu/intel.c +++ b/arch/x86/kernel/cpu/intel.c @@ -321,6 +321,15 @@ static void early_init_intel(struct cpuinfo_x86 *c) #ifdef CONFIG_X86_64 set_cpu_cap(c, X86_FEATURE_SYSENTER32); #else
- /*
* The Quark doesn't have bit 19 set in 0x01 CPUID leaf, which means* it doesn't provide any clflush instructions and hence the cache* alignment is set to 0. The actual cache line size is 16 bytes,* hence set the alignment accordingly. At the same time the physical* and virtual address bits are retrieved via 0x80000008 CPUID leaf.*/- if (c->x86 == 5 && c->x86_model == 9)
c->x86_cache_alignment = 16;
What are the odds that another CPU has this issue? I'm thinking we should just set a default in addition to hacking around this for Quark.
On Thu, May 16, 2024 at 11:21:13AM -0700, Dave Hansen wrote:
On 5/16/24 10:39, Andy Shevchenko wrote:
The initial change to set x86_virt_bits to the correct value straight away broke boot on Intel Quark X1000 CPUs (which are family 5, model 9, stepping 0)
Do you know what _actually_ broke? Like was there a crash somewhere?
Nope, I have no a single character to tell anything about this. Note, earlyprintk may not be helpful as this SoC doesn't have legacy UARTs.
With deeper investigation it appears that the Quark doesn't have the bit 19 set in 0x01 CPUID leaf, which means it doesn't provide any clflush instructions and hence the cache alignment is set to 0. The actual cache line size is 16 bytes, hence we may set the alignment accordingly. At the same time the physical and virtual address bits are retrieved via 0x80000008 CPUID leaf.
This seems to be saying that ->x86_clflush_size must come from CPUID. But there _are_ CPUID-independent defaults set in identify_cpu(). How do those fit in?
Where? The mentioned fbf6449f84bf dropped those for this case.
Note, we don't really care about the value of x86_clflush_size as it is either used with a proper check for the instruction to be present, or, like in PCI case, it assumes 32 bytes for all supported 32-bit CPUs that have actually smaller cache line sizes and don't advertise it.
Are you trying to say that having ->x86_clflush_size==0 is not fatal while having ->x86_cache_alignment==0 _is_ fatal?
I'm only saying that clflush is not implemented there and having a dangled value is not a problem.
But it indeed implies what you are saying.
The commit fbf6449f84bf ("x86/sev-es: Set x86_virt_bits to the correct value straight away, instead of a two-phase approach") basically revealed the issue that has been present from day 1 of introducing the Quark support.
How did it do that, exactly? It's still not crystal clear.
See above, it removes, like you said, the CPUID independent defaults which were set unconditionally and implied that cache alignment should come from CPUID (clflush bits).
...
- /*
* The Quark doesn't have bit 19 set in 0x01 CPUID leaf, which means* it doesn't provide any clflush instructions and hence the cache* alignment is set to 0. The actual cache line size is 16 bytes,* hence set the alignment accordingly. At the same time the physical* and virtual address bits are retrieved via 0x80000008 CPUID leaf.*/- if (c->x86 == 5 && c->x86_model == 9)
c->x86_cache_alignment = 16;What are the odds that another CPU has this issue?
I'm not sure I follow.
I'm thinking we should just set a default in addition to hacking around this for Quark.
linux-stable-mirror@lists.linaro.org
-
Andy Shevchenko -
Dave Hansen