| Message ID | 20230511040857.6094-8-weijiang.yang@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Enable CET Virtualization | expand |
On Thu, May 11, 2023 at 12:08:43AM -0400, Yang Weijiang wrote: >Update CPUID(EAX=0DH,ECX=1) when the guest's XSS is modified. >CPUID(EAX=0DH,ECX=1).EBX reports current required storage size for all >features enabled via XCR0 | XSS so that guest can allocate correct xsave >buffer. > >Note, KVM does not yet support any XSS based features, i.e. supported_xss >is guaranteed to be zero at this time. > >Co-developed-by: Zhang Yi Z <yi.z.zhang@linux.intel.com> >Signed-off-by: Zhang Yi Z <yi.z.zhang@linux.intel.com> >Signed-off-by: Yang Weijiang <weijiang.yang@intel.com> >--- > arch/x86/kvm/cpuid.c | 7 +++++-- > arch/x86/kvm/x86.c | 6 ++++-- > 2 files changed, 9 insertions(+), 4 deletions(-) > >diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c >index 123bf8b97a4b..cbb1b8a65502 100644 >--- a/arch/x86/kvm/cpuid.c >+++ b/arch/x86/kvm/cpuid.c >@@ -277,8 +277,11 @@ static void __kvm_update_cpuid_runtime(struct kvm_vcpu *vcpu, struct kvm_cpuid_e > > best = cpuid_entry2_find(entries, nent, 0xD, 1); > if (best && (cpuid_entry_has(best, X86_FEATURE_XSAVES) || >- cpuid_entry_has(best, X86_FEATURE_XSAVEC))) >- best->ebx = xstate_required_size(vcpu->arch.xcr0, true); >+ cpuid_entry_has(best, X86_FEATURE_XSAVEC))) { Align indentation. if (best && (cpuid_entry_has(best, X86_FEATURE_XSAVES) || cpuid_entry_has(best, X86_FEATURE_XSAVEC))) { >+ u64 xstate = vcpu->arch.xcr0 | vcpu->arch.ia32_xss; >+ >+ best->ebx = xstate_required_size(xstate, true); >+ } > > best = __kvm_find_kvm_cpuid_features(vcpu, entries, nent); > if (kvm_hlt_in_guest(vcpu->kvm) && best && >diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c >index 33a780fe820b..ab3360a10933 100644 >--- 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. > return 1; >- vcpu->arch.ia32_xss = data; >- kvm_update_cpuid_runtime(vcpu); >+ if (vcpu->arch.ia32_xss != data) { >+ vcpu->arch.ia32_xss = data; >+ kvm_update_cpuid_runtime(vcpu); >+ } > break; > case MSR_SMI_COUNT: > if (!msr_info->host_initiated) >-- >2.27.0 >
On 5/25/2023 2:10 PM, Chao Gao wrote: > On Thu, May 11, 2023 at 12:08:43AM -0400, Yang Weijiang wrote: >> Update CPUID(EAX=0DH,ECX=1) when the guest's XSS is modified. >> CPUID(EAX=0DH,ECX=1).EBX reports current required storage size for all >> features enabled via XCR0 | XSS so that guest can allocate correct xsave >> buffer. >> >> Note, KVM does not yet support any XSS based features, i.e. supported_xss >> is guaranteed to be zero at this time. >> >> Co-developed-by: Zhang Yi Z <yi.z.zhang@linux.intel.com> >> Signed-off-by: Zhang Yi Z <yi.z.zhang@linux.intel.com> >> Signed-off-by: Yang Weijiang <weijiang.yang@intel.com> >> --- >> arch/x86/kvm/cpuid.c | 7 +++++-- >> arch/x86/kvm/x86.c | 6 ++++-- >> 2 files changed, 9 insertions(+), 4 deletions(-) >> >> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c >> index 123bf8b97a4b..cbb1b8a65502 100644 >> --- a/arch/x86/kvm/cpuid.c >> +++ b/arch/x86/kvm/cpuid.c >> @@ -277,8 +277,11 @@ static void __kvm_update_cpuid_runtime(struct kvm_vcpu *vcpu, struct kvm_cpuid_e >> >> best = cpuid_entry2_find(entries, nent, 0xD, 1); >> if (best && (cpuid_entry_has(best, X86_FEATURE_XSAVES) || >> - cpuid_entry_has(best, X86_FEATURE_XSAVEC))) >> - best->ebx = xstate_required_size(vcpu->arch.xcr0, true); >> + cpuid_entry_has(best, X86_FEATURE_XSAVEC))) { > Align indentation. OK. Thanks! > > if (best && (cpuid_entry_has(best, X86_FEATURE_XSAVES) || > cpuid_entry_has(best, X86_FEATURE_XSAVEC))) { > >> + u64 xstate = vcpu->arch.xcr0 | vcpu->arch.ia32_xss; >> + >> + best->ebx = xstate_required_size(xstate, true); >> + } >> >> best = __kvm_find_kvm_cpuid_features(vcpu, entries, nent); >> if (kvm_hlt_in_guest(vcpu->kvm) && best && >> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c >> index 33a780fe820b..ab3360a10933 100644 >> --- 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 all KVM supported bits can be accessed by guest. There's already another variable vcpu->arch.ia32_xss to play similar role. In future, if there's requirement to serve per VM control purpose, then will align it to XCR0 settings. > >> return 1; >> - vcpu->arch.ia32_xss = data; >> - kvm_update_cpuid_runtime(vcpu); >> + if (vcpu->arch.ia32_xss != data) { >> + vcpu->arch.ia32_xss = data; >> + kvm_update_cpuid_runtime(vcpu); >> + } >> break; >> case MSR_SMI_COUNT: >> if (!msr_info->host_initiated) >> -- >> 2.27.0 >>
>> > --- 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.
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!
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.
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?
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. [*] https://lore.kernel.org/all/20230607224520.4164598-1-aaronlewis@google.com
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
diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c index 123bf8b97a4b..cbb1b8a65502 100644 --- a/arch/x86/kvm/cpuid.c +++ b/arch/x86/kvm/cpuid.c @@ -277,8 +277,11 @@ static void __kvm_update_cpuid_runtime(struct kvm_vcpu *vcpu, struct kvm_cpuid_e best = cpuid_entry2_find(entries, nent, 0xD, 1); if (best && (cpuid_entry_has(best, X86_FEATURE_XSAVES) || - cpuid_entry_has(best, X86_FEATURE_XSAVEC))) - best->ebx = xstate_required_size(vcpu->arch.xcr0, true); + cpuid_entry_has(best, X86_FEATURE_XSAVEC))) { + u64 xstate = vcpu->arch.xcr0 | vcpu->arch.ia32_xss; + + best->ebx = xstate_required_size(xstate, true); + } best = __kvm_find_kvm_cpuid_features(vcpu, entries, nent); if (kvm_hlt_in_guest(vcpu->kvm) && best && diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index 33a780fe820b..ab3360a10933 100644 --- 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) return 1; - vcpu->arch.ia32_xss = data; - kvm_update_cpuid_runtime(vcpu); + if (vcpu->arch.ia32_xss != data) { + vcpu->arch.ia32_xss = data; + kvm_update_cpuid_runtime(vcpu); + } break; case MSR_SMI_COUNT: if (!msr_info->host_initiated)