Messages in this thread | | | Date | Mon, 26 Jun 2023 17:24:36 +0800 | Subject | Re: [PATCH v3 07/21] KVM:x86: Refresh CPUID on write to guest MSR_IA32_XSS | From | "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@google.com
| |