Re: [PATCH v3 07/21] KVM:x86: Refresh CPUID on write to guest MSR_IA32_XSS

[Yang, Weijiang]

On 6/24/2023 7:21 AM, Sean Christopherson wrote:
> On Fri, Jun 16, 2023, Weijiang Yang wrote:
> > 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 purpose.
> > > > > >
> > > > > > For guest XSS settings, now we don't add extra constraints like XCR0, thus
> > > > > QEMU can impose constraints by configuring guest CPUID.0xd.1 to indicate
> > > > > certain supervisor state components cannot be managed by XSAVES, even
> > > > > though KVM supports them. IOW, guests may differ in the supported values
> > > > > 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.
> > Since there's already a CET specific unit test app, do you mind adding all
> > CET related stuffs to the app to make it inclusive? e.g.,�validate 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 special
> cases, e.g. 32-bit support, selftests is a better framework for testing MSRs than
> KUT, as it's relatively easy to create a custom vCPU model in selftests, whereas
> in KUT it requires handcoding an entry in unittests.cfg, and having corresponding
> 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.
>
> 	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 = msr, .name = #msr, .feature1 = f1, .feature2 = f2, .nr_values = ARRAY_SIZE(msr_VALUES), .values = 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[] = {
> 		<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 combinations of
> features being enabled (if supported by KVM).  And it could also test ioctls(),
> which are all but impossible to test in KUT, e.g. verify that supported MSRs are
> reported in KVM_GET_MSR_INDEX_LIST, verify that userspace can read/write MSRs
> 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, but we
> can worry about that later as it shouldn't be difficult to put the framework and
> MSR definitions in common code.

OK, I'll add a new selftest app which initially only includes CET MSRs 
testing but practice

the above ideas.

> [*] https://lore.kernel.org/all/20230607224520.4164598-1-aaronlewis@xxxxxxxxxx