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

[Michael Kelley]

From: Xi Ruoyao <xry111@xxxxxxxxxxx> Sent: Monday, March 25, 2024 3:21 AM
> 
> 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.

I haven't done a complete census, but there don't seem to be
that many places in x86 code where the microcode version is
checked. Several (most?) already have some kind of "out" when
running on a hypervisor -- like bad_spectre_microcode(), and
apic_validate_deadline_timer().

I've commented above on what Hyper-V does, but I don't know
what other hypervisors do.  The comment in bad_spectre_microcode()
seems to imply that the problem is widespread, and perhaps
not just a Hyper-V thing.  But I don’t know that for sure.  If we
set boot_cpu_data.microcode to 0, we'll need to reason about
the implications because that will certainly flip the outcome of
any comparisons for Hyper-V guests and perhaps other
hypervisor guests as well.

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

I don't know.  At least for Hyper-V, that ship has sailed.  I don't
have a way to get Hyper-V to make a change.  Making a change
would also introduce a backward compatibility problem that can't
easily be handled.

Michael

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