Received-SPF: pass (google.com: domain of linux-kernel+bounces-131886-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Thu, 4 Apr 2024 10:28:14 -0700
In-Reply-To: <d1cc1f7c-9df0-4e3c-88ae-aedcd556ba95@citrix.com>
Precedence: bulk
Mime-Version: 1.0
References: <20240324190503.116160-1-xry111@xry111.site> <SN6PR02MB41576028614CB7FE9A11EBEBD4362@SN6PR02MB4157.namprd02.prod.outlook.com>
 <Zg7S5dk3J4Zc82nj@google.com> <d1cc1f7c-9df0-4e3c-88ae-aedcd556ba95@citrix.com>
Message-ID: <Zg7jLn-lV55Yh6mH@google.com>
Subject: Re: [PATCH v2] x86/mm: Don't disable INVLPG if "incomplete Global
 INVLPG flushes" is fixed by microcode
From: Sean Christopherson <seanjc@google.com>
To: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Michael Kelley <mhklinux@outlook.com>, Xi Ruoyao <xry111@xry111.site>, 
	Dave Hansen <dave.hansen@linux.intel.com>, Andy Lutomirski <luto@kernel.org>, 
	Peter Zijlstra <peterz@infradead.org>, Thomas Gleixner <tglx@linutronix.de>, 
	Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>, "H. Peter Anvin" <hpa@zytor.com>, 
	"x86@kernel.org" <x86@kernel.org>, 
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, Dexuan Cui <decui@microsoft.com>
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable

On Thu, Apr 04, 2024, Andrew Cooper wrote:
> On 04/04/2024 5:18 pm, Sean Christopherson wrote:
> > On Mon, Mar 25, 2024, Michael Kelley wrote:
> >>>  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 =3D 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.
> > Enumerating broken PCID support to a guest is very arguably a hyperviso=
r bug.
> > Hypervisors also lie to guest about FMS.  As KVM *user* with affected h=
ardware
> > (home box), I would want the kernel to assume PCID works if X86_FEATURE=
_HYPERVISOR
> > is present.
>=20
> I have very mixed feelings about all of this.
>=20
> The Gracemont INVLPG vs PCID bug was found in the field, so PCID will
> have been enumerated to guests at that point.=C2=A0 You can't blindly dro=
p
> PCID until the VM next reboots.
>=20
> A related example.=C2=A0 I wrote the patch to hide XSAVES to work around =
an
> AMD erratum where XSAVEC sufficed, and the consequences were so dire for
> some versions of Windows that there was a suggestion to simply revert
> the workaround to make VMs run again.=C2=A0 Windows intentionally asserts
> sanity (=3D=3D expectations) in what it can see; I have no idea whether i=
t
> would object in this case but hiding PCID is definitely playing with fire=
.

Yeah, KVM users got burned by that too.  d52734d00b8e ("KVM: x86: Give a hi=
nt when
Win2016 might fail to boot due to XSAVES erratum").

But practically speaking, that ship has sailed for KVM, as KVM advertises P=
CID
support if and only if the host kernel supports it, i.e. clearing X86_FEATU=
RE_PCID
in setup_pcid() means KVM stops advertising to userspace, which in turn mea=
ns
QEMU stops enumerating it VMs by default.

> I am frequently dismayed at how many FMS abuses there are in Linux, and
> I'm this >< close to talking a leaf out of HyperV's book and poisoning
> FMS to 0 or ~0 just like the microcode revision.=C2=A0=C2=A0 Any use of F=
MS for
> anything other than diagnostic purposes under virt is live migration bug
> waiting to happen.
>=20
> Xen's current TLB algorithms ensure never to have both PCID and PGE
> active together, so we managed to dodge this particular mess.=C2=A0 But a=
s a
> consequence, we've got no logic to spot it, or to consider changing PCID
> visibility.=C2=A0 That said, for better or worse, the ucode revision is
> visible (for now), and a guest which polls the revision will even spot
> the hypervisor doing a late ucode load.
>=20
> Sorry I don't have any better suggestions.=C2=A0 The only nice(ish) of fi=
xing
> this for the guest kernel is for Intel to allocate a $FOO_NO bit, and
> it's horrible in every other way.

Hmm, one crazy idea would be to carve out a hypervisor CPUID range for enum=
erating
(potentially) broken features.  Dealing with the Intel/AMD (and Centaur, LO=
L),
0 / 0x8000_0000 split would be annoying, but not hard.  E.g. use 0x4{0,8,C}=
01_xxxx
to enumerate broken features, and then the guest could do:

	support =3D CPUID(leaf).reg & ~CPUID(to_pv_broken(leaf)).reg;

It'd require a decent amount of churn for the initial support, but it would=
 give
hypervisors a way to inform guests that _any_ CPUID-based feature is broken=
,
without requiring guest changes (after the initial code is merged) or expli=
cit
action from hardware vendors.

And if we got Windows/Hyper-V in on the game, it would allow them to keep t=
heir
sanity checks while (hopefully) degrading gracefully if a feature is enumer=
ated
as broken.