[PATCH v1 1/1] x86/cpu: Fix boot on Intel Quark X1000

[PATCH v1 1/1] x86/cpu: Fix boot on Intel Quark X1000

[Andy Shevchenko]

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;
-- 
2.43.0.rc1.1336.g36b5255a03ac

Re: [PATCH v1 1/1] x86/cpu: Fix boot on Intel Quark X1000

[Dave Hansen]

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.

Re: [PATCH v1 1/1] x86/cpu: Fix boot on Intel Quark X1000

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko