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:   Fri, 23 Jun 2023 16:21:13 -0700
In-Reply-To: <f2708ad5-494c-c91e-cf5a-09f6e2d81e15@intel.com>
Mime-Version: 1.0
References: <20230511040857.6094-1-weijiang.yang@intel.com>
 <20230511040857.6094-8-weijiang.yang@intel.com> <ZG77wu4PyBn1z587@chao-email>
 <23169e5f-9793-9486-fd5f-287e3317b837@intel.com> <ZHXnP5bEvxPrDg3m@chao-email>
 <6e1e9de9-8def-46c3-a238-c1b4ef52e331@intel.com> <ZIuitcH7P1JpkzFH@google.com>
 <f2708ad5-494c-c91e-cf5a-09f6e2d81e15@intel.com>
Message-ID: <ZJYo6aDtt0DQ5Tjv@google.com>
Subject: Re: [PATCH v3 07/21] KVM:x86: Refresh CPUID on write to guest MSR_IA32_XSS
From:   Sean Christopherson <seanjc@google.com>
To:     Weijiang Yang <weijiang.yang@intel.com>
Cc:     Chao Gao <chao.gao@intel.com>, pbonzini@redhat.com,
        kvm@vger.kernel.org, linux-kernel@vger.kernel.org,
        peterz@infradead.org, rppt@kernel.org, binbin.wu@linux.intel.com,
        rick.p.edgecombe@intel.com, john.allen@amd.com,
        Zhang Yi Z <yi.z.zhang@linux.intel.com>
Content-Type: text/plain; charset="iso-8859-1"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Fri, Jun 16, 2023, Weijiang Yang wrote:
>=20
> On 6/16/2023 7:45 AM, Sean Christopherson wrote:
> > On Wed, May 31, 2023, Weijiang Yang wrote:
> > > On 5/30/2023 8:08 PM, Chao Gao wrote:
> > > > > > > --- a/arch/x86/kvm/x86.c
> > > > > > > +++ b/arch/x86/kvm/x86.c
> > > > > > > @@ -3776,8 +3776,10 @@ int kvm_set_msr_common(struct kvm_vcpu=
 *vcpu, struct msr_data *msr_info)
> > > > > > > 		 */
> > > > > > > 		if (data & ~kvm_caps.supported_xss)
> > > > > > Shouldn't we check against the supported value of _this_ guest?=
 similar to
> > > > > > guest_supported_xcr0.
> > > > > I don't think it requires an extra variable to serve per guest pu=
rpose.
> > > > >=20
> > > > > For guest XSS settings, now we don't add extra constraints like X=
CR0, thus
> > > > QEMU can impose constraints by configuring guest CPUID.0xd.1 to ind=
icate
> > > > certain supervisor state components cannot be managed by XSAVES, ev=
en
> > > > though KVM supports them. IOW, guests may differ in the supported v=
alues
> > > > for the IA32_XSS MSR.
> > > OK, will change this part to align with xcr0 settings. Thanks!
> > Please write KVM-Unit-Tests to verify KVM correctly handles the various=
 MSRs related
> > to CET, e.g. a test_cet_msrs() subtest in msr.c would do nicely.  Hmm, =
though testing
> > the combinations of CPUID bits will require multiple x86/unittests.cfg =
entries.
> > Might be time to split up msr.c into a library and then multiple tests.
>=20
> Since there's already a CET specific unit test app, do you mind adding al=
l
> CET related stuffs to the app to make it inclusive? e.g.,=EF=BF=BDvalidat=
e constraints
> between CET CPUIDs vs. CET/XSS MSRs?

Hmm, that will get a bit kludgy since the MSR testcases will want to toggle=
 IBT
and SHSTK on and off.

Actually, I take back my suggestion to add a KUT test.  Except for a few sp=
ecial
cases, e.g. 32-bit support, selftests is a better framework for testing MSR=
s than
KUT, as it's relatively easy to create a custom vCPU model in selftests, wh=
ereas
in KUT it requires handcoding an entry in unittests.cfg, and having corresp=
onding
code in the test itself.

The biggest gap in selftests was the lack of decent reporting in guest code=
, but
Aaron is working on closing that gap[*].

I'm thinking something like this as a framework. =20

	struct msr_data {
		const uint32_t idx;
		const char *name;
		const struct kvm_x86_cpu_feature feature1;
		const struct kvm_x86_cpu_feature feature2;
		const uint32_t nr_values;
		const uint64_t *values;
	};

	#define TEST_MSR2(msr, f1, f2) { .idx =3D msr, .name =3D #msr, .feature1 =
=3D f1, .feature2 =3D f2, .nr_values =3D ARRAY_SIZE(msr_VALUES), .values =
=3D msr_VALUES }
	#define TEST_MSR(msr, f) TEST_MSR2(msr, f, <a dummy value?>)
	#define TEST_MSR0(msr) TEST_MSR(msr, <a dummy value?>)

With CET usage looking like

	static const uint64_t MSR_IA32_S_CET_VALUES[] =3D {
		<super interesting values>
	};

	TEST_MSR2(MSR_IA32_S_CET, X86_FEATURE_IBT, X86_FEATURE_SHSTK);

Then the test could iterate over each entry and test the various combinatio=
ns of
features being enabled (if supported by KVM).  And it could also test ioctl=
s(),
which are all but impossible to test in KUT, e.g. verify that supported MSR=
s are
reported in KVM_GET_MSR_INDEX_LIST, verify that userspace can read/write MS=
Rs
regardless of guest CPUID, etc.  Ooh, and we can even test MSR filtering.

I don't know that we'd want to cram all of those things in a single test, b=
ut we
can worry about that later as it shouldn't be difficult to put the framewor=
k and
MSR definitions in common code.

[*] https://lore.kernel.org/all/20230607224520.4164598-1-aaronlewis@google.=
com