Re: [RFC PATCH 02/15] x86/apic: Fix smp init delay for extended Intel families

[Sohil Mehta <sohil.mehta@intel.com>]

On 12/20/2024 3:20 PM, Dave Hansen wrote:
> On 12/20/24 13:36, Sohil Mehta wrote:
>> The MP specification version 1.4 references the i486 and early Pentium
>> processors in family 5. 
> 
> Can you please elaborate on how this reference is relevant to the patch
> at hand?
> 

The comment above UDELAY_10MS_DEFAULT which references the MP
specification seems to be the basis of all the cpu_init_delay quirks.

I wanted to use that reference to justify that family 15 doesn't need
the 10 msec delay since it is not explicitly mentioned there. I realize
now that it's moot since the Pentium 4 wasn't released until 3 years
after it's publication.

I referred the latest MP initialization specification but that doesn't
say explicitly when the delay is needed vs not needed. However it does
say that later Family 6 models and Family 15 models have similar
initialization protocols.

"The selection of the BSP and APs is handled through a special system
bus cycle, without using BIPI and FIPI message arbitration (see Section
9.4.3, “MP Initialization Protocol Algorithm for MP Systems”). These
processor generations have CPUID signatures of family=0FH with
(model=0H, stepping>=0AH) or (model >0, all steppings); or family=06H,
extended_model=0, model>=0EH."

>> However, all processors starting with family 6 likely do not need the
>> 10 msec INIT delay. The omission of the Pentium 4s (family 15) seems
>> like an oversight in the original check.
>>
>> With some risk, choose a simpler check and extend the quirk to all
>> recent and upcoming Intel processors.
> 
> I'm struggling to follow this a bit.
> 

Because my interpretation of the code and the related comments is
opposite to yours. The usage of "quirk" in this context seems to be
inverted due to how this check has evolved over time.

> I think these are the facts that matter:
> 

The code says this:

/* if modern processor, use no delay */
	init_udelay = 0;
/* else, use legacy delay */
	init_udelay = UDELAY_10MS_DEFAULT;


The legacy/default delay is 10 msec and then the quirk applies to the
modern processors to remove the delay. This is the comment above
UDELAY_10MS_DEFAULT:

 * Modern processor families are quirked to remove the delay entirely.

>  * init_udelay=0 means "no quirk"
>  * Modern CPUs don't have the quirk
>  * The current check says "only family 6 is modern"
>  * Family 15 is _probably_ modern and just forgotten about
> 
> And this is what you're doing in the end:
> 
> Consider everything PPro and later to be modern, including all of
> families 6, 15 and the new 18/19 CPUs.
> 

That's right. We consider these CPUs to be modern and do not apply the
10 msec delay but the usage of "quirk" is confusing here.

I'll clarify the changelog without using "quirk" to avoid confusion.

Am I interpreting this wrong?