Re: [PATCH v2] x86/mm: Don't disable INVLPG if "incomplete Global INVLPG flushes" is fixed by microcode

[Xi Ruoyao]

On Mon, 2024-03-25 at 04:57 +0000, Michael Kelley wrote:
> From: Xi Ruoyao <xry111@xxxxxxxxxxx> Sent: Sunday, March 24, 2024 12:05 PM
> > 
> > Per the "Processor Specification Update" documentations referred by the
> > intel-microcode-20240312 release note, this microcode release has fixed
> > the issue for all affected models.
> > 
> > So don't disable INVLPG if the microcode is new enough.
> > 
> > Cc: Dave Hansen <dave.hansen@xxxxxxxxxxxxxxx>
> > Signed-off-by: Xi Ruoyao <xry111@xxxxxxxxxxx>
> > ---
> >  arch/x86/mm/init.c | 32 ++++++++++++++++++++------------
> >  1 file changed, 20 insertions(+), 12 deletions(-)
> > 
> > diff --git a/arch/x86/mm/init.c b/arch/x86/mm/init.c
> > index 679893ea5e68..c52be4e66e44 100644
> > --- a/arch/x86/mm/init.c
> > +++ b/arch/x86/mm/init.c
> > @@ -261,33 +261,41 @@ static void __init probe_page_size_mask(void)
> >  	}
> >  }
> > 
> > -#define INTEL_MATCH(_model) { .vendor  = X86_VENDOR_INTEL,	\
> > -			      .family  = 6,			\
> > -			      .model = _model,			\
> > -			    }
> > +#define INTEL_MATCH(_model, _fixed_microcode)	\
> > +    { .vendor		= X86_VENDOR_INTEL,	\
> > +      .family		= 6,			\
> > +      .model		= _model,		\
> > +      .driver_data	= _fixed_microcode,	\
> > +    }
> > +
> >  /*
> >   * INVLPG may not properly flush Global entries
> > - * on these CPUs when PCIDs are enabled.
> > + * on these CPUs when PCIDs are enabled and the
> > + * microcode is not updated to fix the issue.
> >   */
> >  static const struct x86_cpu_id invlpg_miss_ids[] = {
> > -	INTEL_MATCH(INTEL_FAM6_ALDERLAKE   ),
> > -	INTEL_MATCH(INTEL_FAM6_ALDERLAKE_L ),
> > -	INTEL_MATCH(INTEL_FAM6_ATOM_GRACEMONT ),
> > -	INTEL_MATCH(INTEL_FAM6_RAPTORLAKE  ),
> > -	INTEL_MATCH(INTEL_FAM6_RAPTORLAKE_P),
> > -	INTEL_MATCH(INTEL_FAM6_RAPTORLAKE_S),
> > +	INTEL_MATCH(INTEL_FAM6_ALDERLAKE,	0x34),
> > +	INTEL_MATCH(INTEL_FAM6_ALDERLAKE_L,	0x432),
> > +	INTEL_MATCH(INTEL_FAM6_ATOM_GRACEMONT,	0x15),
> > +	INTEL_MATCH(INTEL_FAM6_RAPTORLAKE,	0x122),
> > +	INTEL_MATCH(INTEL_FAM6_RAPTORLAKE_P,	0x4121),
> > +	INTEL_MATCH(INTEL_FAM6_RAPTORLAKE_S,	0x34),
> >  	{}
> >  };
> > 
> >  static void setup_pcid(void)
> >  {
> > +	const struct x86_cpu_id *invlpg_miss_match;
> > +
> >  	if (!IS_ENABLED(CONFIG_X86_64))
> >  		return;
> > 
> >  	if (!boot_cpu_has(X86_FEATURE_PCID))
> >  		return;
> > 
> > -	if (x86_match_cpu(invlpg_miss_ids)) {
> > +	invlpg_miss_match = x86_match_cpu(invlpg_miss_ids);
> > +	if (invlpg_miss_match &&
> > +	    invlpg_miss_match->driver_data > boot_cpu_data.microcode) {
> >  		pr_info("Incomplete global flushes, disabling PCID");
> >  		setup_clear_cpu_cap(X86_FEATURE_PCID);
> >  		return;
> 
> As noted in similar places where microcode versions are
> checked, hypervisors often lie to guests about microcode versions.
> For example, see comments in bad_spectre_microcode().  I
> know Hyper-V guests always see the microcode version as
> 0xFFFFFFFF (max u32 value).  So in a Hyper-V guest the above
> code will always leave PCID enabled.
> 
> Maybe the above should have a check for running on a
> hypervisor and always disable PCID without checking the
> microcode version.  That's the safe approach, though there are
> other similar cases like bad_spectre_microcode() that take
> the unsafe approach when running as a guest.  I don't know
> what's best here .....

Then generally maybe we should set boot_cpu_data.microcode to 0 if a
hypervisor is detected?  Or checking against hypervisors at every place
where we check the microcode revision will be nasty.

And I'd prefer they say 0 instead if 0xffffffffu if they must lie about
the microcode revision.

-- 
Xi Ruoyao <xry111@xxxxxxxxxxx>
School of Aerospace Science and Technology, Xidian University