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

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

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