Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 6 Jun 2023 16:40:40 -0700
In-Reply-To: <8AE71A62-1BE3-4D6F-B57C-B5FCEA93169F@nutanix.com>
Mime-Version: 1.0
References: <20230530200152.18961-1-jon@nutanix.com> <2a6502e3-ba87-0355-af09-825e8467b81f@intel.com>
 <F4AFC5EE-9967-4117-BA85-ED82C106575C@nutanix.com> <ZHd2P6D142rCByrm@google.com>
 <E17EFDD7-C54A-4532-B1D3-D567557FC54B@nutanix.com> <ZHermsSGQBcDD07R@google.com>
 <CBFC095A-10D1-4925-9F28-DEDEBBB38EF8@nutanix.com> <8AE71A62-1BE3-4D6F-B57C-B5FCEA93169F@nutanix.com>
Message-ID: <ZH/D+JP2OGFtCUBy@google.com>
Subject: Re: [PATCH] x86/fpu/xstate: clear XSAVE features if DISABLED_MASK set
From:   Sean Christopherson <seanjc@google.com>
To:     Jon Kohler <jon@nutanix.com>
Cc:     Dave Hansen <dave.hansen@intel.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
        Dave Hansen <dave.hansen@linux.intel.com>,
        "x86@kernel.org" <x86@kernel.org>,
        "H. Peter Anvin" <hpa@zytor.com>,
        "Chang S. Bae" <chang.seok.bae@intel.com>,
        Kyle Huey <me@kylehuey.com>,
        "neelnatu@google.com" <neelnatu@google.com>,
        Andrew Cooper <andrew.cooper3@citrix.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "kvm@vger.kernel.org" <kvm@vger.kernel.org>,
        Paolo Bonzini <pbonzini@redhat.com>
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Mon, Jun 05, 2023, Jon Kohler wrote:
> > On May 31, 2023, at 5:09 PM, Jon Kohler <jon@nutanix.com> wrote:
> >> The CPUID bits that enumerate support for a feature are independent fr=
om the CPUID
> >> bits that enumerate what XCR0 bits are supported, i.e. what features c=
an be saved
> >> and restored via XSAVE/XRSTOR.
> >>=20
> >> KVM does mostly account for host XCR0, but in a very ad hoc way.  E.g.=
 MPX is
> >> handled by manually checking host XCR0.
> >>=20
> >> 	if (kvm_mpx_supported())
> >> 		kvm_cpu_cap_check_and_set(X86_FEATURE_MPX);
> >>=20
> >> PKU manually checks too, but indirectly by looking at whether or not t=
he kernel
> >> has enabled CR4.OSPKE.
> >>=20
> >> 	if (!tdp_enabled || !boot_cpu_has(X86_FEATURE_OSPKE))
> >> 		kvm_cpu_cap_clear(X86_FEATURE_PKU);
> >>=20
> >> But unless I'm missing something, the various AVX and AMX bits rely so=
lely on
> >> boot_cpu_data, i.e. would break if someone added CONFIG_X86_AVX or CON=
FIG_X86_AMX.
> >=20
> > What if we simply moved static unsigned short xsave_cpuid_features[] =
=E2=80=A6 into
> > xstate.h, which is already included in arch/x86/kvm/cpuid.c, and do
> > something similar to what I=E2=80=99m proposing in this patch already
> >=20
> > This would future proof such breakages I=E2=80=99d imagine?
> >=20
> > void kvm_set_cpu_caps(void)
> > {
> > ...
> >    /*
> >     * Clear CPUID for XSAVE features that are disabled.
> >     */
> >    for (i =3D 0; i < ARRAY_SIZE(xsave_cpuid_features); i++) {
> >        unsigned short cid =3D xsave_cpuid_features[i];
> >=20
> >        /* Careful: X86_FEATURE_FPU is 0! */
> >        if ((i !=3D XFEATURE_FP && !cid) || !boot_cpu_has(cid) ||
> >            !cpu_feature_enabled(cid))
> >            kvm_cpu_cap_clear(cid);
> >    }
> > =E2=80=A6
> > }
> >=20
>=20
> Sean - following up on this rough idea code above, wanted to validate tha=
t
> this was the direction you were thinking of having kvm_set_cpu_caps() cle=
ar
> caps when a particular xsave feature was disabled?

Ya, more or or less.  But for KVM, that should be kvm_cpu_cap_has(), not bo=
ot_cpu_has().
And then I think KVM could actually WARN on a feature being disabled, i.e. =
put up
a tripwire to detect if things change in the future and the kernel lets the=
 user
disable a feature that KVM wants to expose to a guest.

Side topic, I find the "cid" nomenclature super confusing, and the establis=
hed
name in KVM is x86_feature.

Something like this?

	for (i =3D 0; i < ARRAY_SIZE(xsave_cpuid_features); i++) {
		unsigned int x86_feature =3D xsave_cpuid_features[i];

		if (i !=3D XFEATURE_FP && !x86_feature)
			continue;

		if (!kvm_cpu_cap_has(x86_feature))
			continue;

		if (WARN_ON_ONCE(!cpu_feature_enabled(x86_feature)))
			kvm_cpu_cap_clear(x86_feature);
	}