Message ID | 20230511040857.6094-8-weijiang.yang@intel.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:b0ea:0:b0:3b6:4342:cba0 with SMTP id b10csp4171456vqo; Thu, 11 May 2023 00:15:46 -0700 (PDT) X-Google-Smtp-Source: ACHHUZ6wXOW4/k4CC2CugbOwtiJfFjpx2j65nhM5JbiONS50v5ziubykPPquWMtJnzumevbKeb20 X-Received: by 2002:a17:902:ec87:b0:19c:3d78:6a54 with SMTP id x7-20020a170902ec8700b0019c3d786a54mr23476228plg.14.1683789346273; Thu, 11 May 2023 00:15:46 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1683789346; cv=none; d=google.com; s=arc-20160816; b=kPJVwuMt0DraAtr3oHWwpk4cfr87bnKh0+1cJsg8R1VIgMbGC+4r5j8bgg00CsctRS 62xfkAeXRktZm7q/HKsgjho6Gr84qnqBu8yD0aJDqIO2V6FluuowkHRIOjZtYeMiIcb9 aqG6R6ZaXNR23D5CJIzF0bDwPeTW2MeVbuMtcE/drsta/M8SfPtd9+CvrsYtpKx9SDcr ZQ58i4od4nBMZdaAN7pWf6llcpl0o2kQuorFswldLzXj/nYyB7f+G/AcE2gbdilS2XF5 F9TSVP7EJ/nFKXBvvlHS7N5mENkzQHFbA5yVdrWQfBj2gVurYIjXZmG0zs1HpJzl3Ndk 555w== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :references:in-reply-to:message-id:date:subject:cc:to:from :dkim-signature; bh=0B+dqCkCpN6XOhB3tX9m1joZY2xlyAAMP1JcHMAzbc8=; b=m6SoNIyzopX9eeR/GiryWOcbczmWEYy7nay8HBEIdlyqRV24uD15a4wTspxkIIh7/5 xM76PwcadYl/X2Ec3kgtyaopHLbY8111JQT4OKakKGW5t82PYrrdIDtVr3BkgQecqaU0 +POvVFWCj2jb+CZylRenNEWwC2JeXQopRzTTN6jcUrz0KrBjQgvTMUXUbONt4Mb5E8QB TAXElQeS0Zn/hdybocY/4JFyuN2CAm2zHfh2aJf5Dhx1+YRCHGcf9BIOhhkxVP7uNL7S ot89nHDZsmwo8tx/fYwwVOYMhHPZ/h71Y84fWbyxUgwncqvV6Zsdkz+kakgqk6MFM0XO hGgw== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@intel.com header.s=Intel header.b=ZXxOZbgg; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=intel.com Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id bf10-20020a170902b90a00b001a66c369e0fsi5765802plb.510.2023.05.11.00.15.31; Thu, 11 May 2023 00:15:46 -0700 (PDT) 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; Authentication-Results: mx.google.com; dkim=pass header.i=@intel.com header.s=Intel header.b=ZXxOZbgg; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=intel.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S237437AbjEKHNq (ORCPT <rfc822;peekingduck44@gmail.com> + 99 others); Thu, 11 May 2023 03:13:46 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:47414 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S237174AbjEKHNk (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Thu, 11 May 2023 03:13:40 -0400 Received: from mga18.intel.com (mga18.intel.com [134.134.136.126]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 3DC0D2D6A; Thu, 11 May 2023 00:13:32 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1683789212; x=1715325212; h=from:to:cc:subject:date:message-id:in-reply-to: references:mime-version:content-transfer-encoding; bh=7J0BAjKpZeee0YTws2bhg1lRumDrGh+SHYT9zVcLiQI=; b=ZXxOZbgg6h0AcatxizMAgzefIy8sFAe70lc+skijsFs03JCXvTFYHpJk aNSk99IPDb6H5rC7DW/X+zjairDBnxqmi+l0a9M4GROUw/gSXxqx5HjhV uHSxBhnXZW5Wblo+pL5j8K8sGFBHmt2V1yz0mWSgJ9XGLfkT5fQoRlpHa VV4OohyJkxImzYEy89uTt6w+Dt34gZ6P1JsY8+C3U7uRBq9e9fWMJVbIB 1gBm04D+2uZkKNVm1v8Rp2wFiIWgDHn0kuOCqFF0l9Ee2Rgdvo0ZbKR1K Zr0cM9bldTFMgATKvvLRSNj+o5OojdWGiBVHLiwsy7ORArutN/jFMlpxr w==; X-IronPort-AV: E=McAfee;i="6600,9927,10706"; a="334896562" X-IronPort-AV: E=Sophos;i="5.99,266,1677571200"; d="scan'208";a="334896562" Received: from fmsmga005.fm.intel.com ([10.253.24.32]) by orsmga106.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 11 May 2023 00:13:31 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6600,9927,10706"; a="1029512362" X-IronPort-AV: E=Sophos;i="5.99,266,1677571200"; d="scan'208";a="1029512362" Received: from embargo.jf.intel.com ([10.165.9.183]) by fmsmga005-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 11 May 2023 00:13:23 -0700 From: Yang Weijiang <weijiang.yang@intel.com> To: seanjc@google.com, pbonzini@redhat.com, kvm@vger.kernel.org, linux-kernel@vger.kernel.org Cc: peterz@infradead.org, rppt@kernel.org, binbin.wu@linux.intel.com, rick.p.edgecombe@intel.com, weijiang.yang@intel.com, john.allen@amd.com, Zhang Yi Z <yi.z.zhang@linux.intel.com> Subject: [PATCH v3 07/21] KVM:x86: Refresh CPUID on write to guest MSR_IA32_XSS Date: Thu, 11 May 2023 00:08:43 -0400 Message-Id: <20230511040857.6094-8-weijiang.yang@intel.com> X-Mailer: git-send-email 2.27.0 In-Reply-To: <20230511040857.6094-1-weijiang.yang@intel.com> References: <20230511040857.6094-1-weijiang.yang@intel.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-0.5 required=5.0 tests=BAYES_00,DATE_IN_PAST_03_06, DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF, SPF_HELO_NONE,SPF_NONE,T_SCC_BODY_TEXT_LINE autolearn=no autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1765581097427698220?= X-GMAIL-MSGID: =?utf-8?q?1765581097427698220?= |
Series |
Enable CET Virtualization
|
|
Commit Message
Yang, Weijiang
May 11, 2023, 4:08 a.m. UTC
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(-)
Comments
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)