Message ID | 20230914063325.85503-10-weijiang.yang@intel.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a05:612c:172:b0:3f2:4152:657d with SMTP id h50csp226530vqi; Thu, 14 Sep 2023 02:42:19 -0700 (PDT) X-Google-Smtp-Source: AGHT+IE8aMAN85CGHnG5MQTTofreWU6aGIoEqEIvvvvm2LusSUEzFa/NZRaGKFghU9x2ABSI7k/m X-Received: by 2002:a05:6a00:14ca:b0:682:537f:2cb8 with SMTP id w10-20020a056a0014ca00b00682537f2cb8mr5831967pfu.26.1694684539384; Thu, 14 Sep 2023 02:42:19 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1694684539; cv=none; d=google.com; s=arc-20160816; b=xfLlyUxV9FSXft0z9MBXABxjGlz2slD1SJQ3LenFv0F0plVNG+mdbpwvAEYnvv36K2 4dVAMvvjqwtJUWJkSl/cIdg12BLd1ohzoOwrqfGpiPW997lzoh3lYIlJTxnsnOmIK+q8 vDEs+/8jRhr+P5vpT56stNN4UFUSlQBqp5PX+Vm5SuN0vy1bsb3Eha74sSiL3huGLwOY uBApKHgtGR8XufJ3beUUwIrK5Q5jJV9GZNoSHJCfFqIhH1b/aU/Kzb3AapBXlKCHQrrE KCbR3azIRbMBiPgMeyGk1Pdv9PUAnbsihn18nflY8jZ9/RK3mB+stB2MRsOS0xT3n8sp LXoQ== 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=txcO8Bm5yl30eYH3vWHyMFDGNKeHYAw5vDra+9kQT7g=; fh=jQqUhNQZPAXcZ44u72Wu3jv2pQizzn2Be2T8mkd1RwU=; b=S1aLLKl6cROJ3dDgGzb3N11g3nQte8IKeDiTMbF1o/fXvOuv36q5BXEq+i6sM9FbqJ JI0e/tsnkl1h9L5TEhk9P+1bhVqasxv7meLSecUKE8Nlk8d8JN/tvmkB8+g7oXFMzxzr yVHFwFFM6Ce0cR3gjb5ARoK4j8HGpS8ytSxFgMFznrYi6t/nm3LOez7buLwHzd/BbMuq d98weKWZi+XiIoUsdNWVKg0QO1ggUSBg2n2Clm5OueM8jAAOlmFpP+hV5wpwc5crCGCI SM04EL9ZBHqxhI8RJEuzVz1nPQ2khnCv3mzZHarPebDiN0kEvpFfy8d1ur1R68wcPrR1 XBOw== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@intel.com header.s=Intel header.b=KfGyQVvH; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.31 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 morse.vger.email (morse.vger.email. [23.128.96.31]) by mx.google.com with ESMTPS id f20-20020a056a0022d400b0068a68d71b68si1260257pfj.216.2023.09.14.02.42.18 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 14 Sep 2023 02:42:19 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.31 as permitted sender) client-ip=23.128.96.31; Authentication-Results: mx.google.com; dkim=pass header.i=@intel.com header.s=Intel header.b=KfGyQVvH; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.31 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 (depot.vger.email [IPv6:2620:137:e000::3:0]) by morse.vger.email (Postfix) with ESMTP id 61B6F83328ED; Thu, 14 Sep 2023 02:39:01 -0700 (PDT) X-Virus-Status: Clean X-Virus-Scanned: clamav-milter 0.103.10 at morse.vger.email Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S237735AbjINJiw (ORCPT <rfc822;chrisfriedt@gmail.com> + 35 others); Thu, 14 Sep 2023 05:38:52 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:49822 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S237270AbjINJiX (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Thu, 14 Sep 2023 05:38:23 -0400 Received: from mgamail.intel.com (mgamail.intel.com [192.55.52.88]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 423791BFF; Thu, 14 Sep 2023 02:38:19 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1694684299; x=1726220299; h=from:to:cc:subject:date:message-id:in-reply-to: references:mime-version:content-transfer-encoding; bh=fmKaQuGnM9SpdEUHMKDlC8GkY5+N9nPibVd+P2rGn30=; b=KfGyQVvH5gvUg9pKIOpTEgtYI2AmdhU9qnyMvEGGL1yPEehDE3BIOFVo DPdZukVNeY86oYz61N/+ucSZpkvHM5DIs6cIu2CtrIpb6ApkpUnRRexiz Tp99laEBJsP3C5POiodv2GfLQyd41XpklYSfyBvffXEYuBmvrncSnHdfo 3SOBvJL1eJwt7e+yZeIqZ+zztponf7dpyTRXo6C6NP2XosJEfiU5ifokQ FCGf46ZfltqaAlA6dwamv53p8O9HPko8ERZwJGUHgwTiYIj+yjlLjF+qc VTFtwJReA9YxfqfgNcsZTb3yzl+w7ZAF0VhUL6/V2HplQS0VcIXjAJY0f Q==; X-IronPort-AV: E=McAfee;i="6600,9927,10832"; a="409857351" X-IronPort-AV: E=Sophos;i="6.02,145,1688454000"; d="scan'208";a="409857351" Received: from fmsmga007.fm.intel.com ([10.253.24.52]) by fmsmga101.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 14 Sep 2023 02:38:18 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6600,9927,10832"; a="747656240" X-IronPort-AV: E=Sophos;i="6.02,145,1688454000"; d="scan'208";a="747656240" Received: from embargo.jf.intel.com ([10.165.9.183]) by fmsmga007-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 14 Sep 2023 02:38:18 -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: dave.hansen@intel.com, peterz@infradead.org, chao.gao@intel.com, rick.p.edgecombe@intel.com, weijiang.yang@intel.com, john.allen@amd.com Subject: [PATCH v6 09/25] KVM: x86: Rework cpuid_get_supported_xcr0() to operate on vCPU data Date: Thu, 14 Sep 2023 02:33:09 -0400 Message-Id: <20230914063325.85503-10-weijiang.yang@intel.com> X-Mailer: git-send-email 2.27.0 In-Reply-To: <20230914063325.85503-1-weijiang.yang@intel.com> References: <20230914063325.85503-1-weijiang.yang@intel.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (morse.vger.email [0.0.0.0]); Thu, 14 Sep 2023 02:39:01 -0700 (PDT) X-Spam-Status: No, score=0.2 required=5.0 tests=DATE_IN_PAST_03_06, DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS autolearn=no autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on morse.vger.email X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1777005535297641678 X-GMAIL-MSGID: 1777005535297641678 |
Series |
Enable CET Virtualization
|
|
Commit Message
Yang, Weijiang
Sept. 14, 2023, 6:33 a.m. UTC
From: Sean Christopherson <seanjc@google.com> Rework and rename cpuid_get_supported_xcr0() to explicitly operate on vCPU state, i.e. on a vCPU's CPUID state. Prior to commit 275a87244ec8 ("KVM: x86: Don't adjust guest's CPUID.0x12.1 (allowed SGX enclave XFRM)"), KVM incorrectly fudged guest CPUID at runtime, which in turn necessitated massaging the incoming CPUID state for KVM_SET_CPUID{2} so as not to run afoul of kvm_cpuid_check_equal(). Opportunistically move the helper below kvm_update_cpuid_runtime() to make it harder to repeat the mistake of querying supported XCR0 for runtime updates. No functional change intended. Signed-off-by: Sean Christopherson <seanjc@google.com> Signed-off-by: Yang Weijiang <weijiang.yang@intel.com> --- arch/x86/kvm/cpuid.c | 33 ++++++++++++++++----------------- 1 file changed, 16 insertions(+), 17 deletions(-)
Comments
On Thu, 2023-09-14 at 02:33 -0400, Yang Weijiang wrote: > From: Sean Christopherson <seanjc@google.com> > > Rework and rename cpuid_get_supported_xcr0() to explicitly operate on vCPU > state, i.e. on a vCPU's CPUID state. Prior to commit 275a87244ec8 ("KVM: > x86: Don't adjust guest's CPUID.0x12.1 (allowed SGX enclave XFRM)"), KVM > incorrectly fudged guest CPUID at runtime, Can you explain how commit 275a87244ec8 relates to this patch? > which in turn necessitated > massaging the incoming CPUID state for KVM_SET_CPUID{2} so as not to run > afoul of kvm_cpuid_check_equal(). Can you link the commit that added this 'massaging' and explain on how this relates to this patch? Can you explain what is the problem that this patch is trying to solve? Is it really allowed in x86 spec to have different supported mask of XCR0 bits on different CPUs (assuming all CPUs of the same type)? If true, does KVM supports it? Assuming that the answer to the above questions is no, won't this patch make it easier to break this rule and thus make it easier to introduce a bug? Best regards, Maxim Levitsky > > Opportunistically move the helper below kvm_update_cpuid_runtime() to make > it harder to repeat the mistake of querying supported XCR0 for runtime > updates. > > No functional change intended. > > Signed-off-by: Sean Christopherson <seanjc@google.com> > Signed-off-by: Yang Weijiang <weijiang.yang@intel.com> > --- > arch/x86/kvm/cpuid.c | 33 ++++++++++++++++----------------- > 1 file changed, 16 insertions(+), 17 deletions(-) > > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c > index 0544e30b4946..7c3e4a550ca7 100644 > --- a/arch/x86/kvm/cpuid.c > +++ b/arch/x86/kvm/cpuid.c > @@ -247,21 +247,6 @@ void kvm_update_pv_runtime(struct kvm_vcpu *vcpu) > vcpu->arch.pv_cpuid.features = best->eax; > } > > -/* > - * Calculate guest's supported XCR0 taking into account guest CPUID data and > - * KVM's supported XCR0 (comprised of host's XCR0 and KVM_SUPPORTED_XCR0). > - */ > -static u64 cpuid_get_supported_xcr0(struct kvm_cpuid_entry2 *entries, int nent) > -{ > - struct kvm_cpuid_entry2 *best; > - > - best = cpuid_entry2_find(entries, nent, 0xd, 0); > - if (!best) > - return 0; > - > - return (best->eax | ((u64)best->edx << 32)) & kvm_caps.supported_xcr0; > -} > - > static void __kvm_update_cpuid_runtime(struct kvm_vcpu *vcpu, struct kvm_cpuid_entry2 *entries, > int nent) > { > @@ -312,6 +297,21 @@ void kvm_update_cpuid_runtime(struct kvm_vcpu *vcpu) > } > EXPORT_SYMBOL_GPL(kvm_update_cpuid_runtime); > > +/* > + * Calculate guest's supported XCR0 taking into account guest CPUID data and > + * KVM's supported XCR0 (comprised of host's XCR0 and KVM_SUPPORTED_XCR0). > + */ > +static u64 vcpu_get_supported_xcr0(struct kvm_vcpu *vcpu) > +{ > + struct kvm_cpuid_entry2 *best; > + > + best = kvm_find_cpuid_entry_index(vcpu, 0xd, 0); > + if (!best) > + return 0; > + > + return (best->eax | ((u64)best->edx << 32)) & kvm_caps.supported_xcr0; > +} > + > static bool kvm_cpuid_has_hyperv(struct kvm_cpuid_entry2 *entries, int nent) > { > struct kvm_cpuid_entry2 *entry; > @@ -357,8 +357,7 @@ static void kvm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu) > kvm_apic_set_version(vcpu); > } > > - vcpu->arch.guest_supported_xcr0 = > - cpuid_get_supported_xcr0(vcpu->arch.cpuid_entries, vcpu->arch.cpuid_nent); > + vcpu->arch.guest_supported_xcr0 = vcpu_get_supported_xcr0(vcpu); > > /* > * FP+SSE can always be saved/restored via KVM_{G,S}ET_XSAVE, even if
On Tue, Oct 31, 2023, Maxim Levitsky wrote: > On Thu, 2023-09-14 at 02:33 -0400, Yang Weijiang wrote: > > From: Sean Christopherson <seanjc@google.com> > > > > Rework and rename cpuid_get_supported_xcr0() to explicitly operate on vCPU > > state, i.e. on a vCPU's CPUID state. Prior to commit 275a87244ec8 ("KVM: > > x86: Don't adjust guest's CPUID.0x12.1 (allowed SGX enclave XFRM)"), KVM > > incorrectly fudged guest CPUID at runtime, > Can you explain how commit 275a87244ec8 relates to this patch? > > > which in turn necessitated massaging the incoming CPUID state for > > KVM_SET_CPUID{2} so as not to run afoul of kvm_cpuid_check_equal(). > > Can you link the commit that added this 'massaging' and explain on how this > relates to this patch? It's commit 275a87244ec8, which is right above. I think the missing part is an explicit call out that the massaging used cpuid_get_supported_xcr0() with the incoming "struct kvm_cpuid_entry2", i.e. without a "struct kvm_vcpu". > Can you explain what is the problem that this patch is trying to solve? Is this better? -- Rework and rename cpuid_get_supported_xcr0() to explicitly operate on vCPU state, i.e. on a vCPU's CPUID state, now that the only usage of the helper is to retrieve a vCPU's already-set CPUID. Prior to commit 275a87244ec8 ("KVM: x86: Don't adjust guest's CPUID.0x12.1 (allowed SGX enclave XFRM)"), KVM incorrectly fudged guest CPUID at runtime, which in turn necessitated massaging the incoming CPUID state for KVM_SET_CPUID{2} so as not to run afoul of kvm_cpuid_check_equal(). I.e. KVM also invoked cpuid_get_supported_xcr0() with the incoming CPUID state, and thus without an explicit vCPU object. -- > Is it really allowed in x86 spec to have different supported mask of XCR0 bits > on different CPUs (assuming all CPUs of the same type)? Yes, nothing in the SDM explicitly states that all cores in have identical feature sets. And "assuming all CPUs of the same type" isn't really a valid constraint because it's very doable to put different SKUs into a multi-socket system. Intel even (somewhat inadvertantly) kinda sorta shipped such CPUs, as Alder Lake P-cores support AVX512 but E-cores do not, and IIRC early (pre-production?) BIOS didn't disable AVX512 on the P-Cores, i.e. software could observe cores with and without AVX512. That quickly got fixed because it confused software, but until Intel squashed AVX512 entirely with a microcode update, disabling E-Cores in BIOS would effectively enable AVX512 on the remaining P-Cores. And it's not XCR0-related, but PMUs on Alder Lake (and all Intel hybrid CPUs) are truly heterogenous. It's a mess for virtualization, but concrete proof that there are no architectural guarantees regarding homogeneity of feature sets. > If true, does KVM supports it? Yes. Whether or not that's a good thing is definitely debatle, bug KVM's ABI for a very long time has allowed userspace to expose whatever it wants via KVM_SET_CPUID. Getting (guest) software to play nice is an entirely different matter, but exposing heterogenous vCPUs isn't an architectural violation.
On Wed, 2023-11-01 at 07:41 -0700, Sean Christopherson wrote: > On Tue, Oct 31, 2023, Maxim Levitsky wrote: > > On Thu, 2023-09-14 at 02:33 -0400, Yang Weijiang wrote: > > > From: Sean Christopherson <seanjc@google.com> > > > > > > Rework and rename cpuid_get_supported_xcr0() to explicitly operate on vCPU > > > state, i.e. on a vCPU's CPUID state. Prior to commit 275a87244ec8 ("KVM: > > > x86: Don't adjust guest's CPUID.0x12.1 (allowed SGX enclave XFRM)"), KVM > > > incorrectly fudged guest CPUID at runtime, > > Can you explain how commit 275a87244ec8 relates to this patch? > > > > > which in turn necessitated massaging the incoming CPUID state for > > > KVM_SET_CPUID{2} so as not to run afoul of kvm_cpuid_check_equal(). > > > > Can you link the commit that added this 'massaging' and explain on how this > > relates to this patch? > > It's commit 275a87244ec8, which is right above. I think the missing part is an > explicit call out that the massaging used cpuid_get_supported_xcr0() with the > incoming "struct kvm_cpuid_entry2", i.e. without a "struct kvm_vcpu". > > > Can you explain what is the problem that this patch is trying to solve? > > Is this better? > > -- > Rework and rename cpuid_get_supported_xcr0() to explicitly operate on vCPU > state, i.e. on a vCPU's CPUID state, now that the only usage of the helper > is to retrieve a vCPU's already-set CPUID. > > Prior to commit 275a87244ec8 ("KVM: x86: Don't adjust guest's CPUID.0x12.1 > (allowed SGX enclave XFRM)"), KVM incorrectly fudged guest CPUID at > runtime, which in turn necessitated massaging the incoming CPUID state for > KVM_SET_CPUID{2} so as not to run afoul of kvm_cpuid_check_equal(). I.e. > KVM also invoked cpuid_get_supported_xcr0() with the incoming CPUID state, > and thus without an explicit vCPU object. Ah, I understand you. I incorrectly assumed that KVM doesn't allow different CPUID on different vCPUs, while the actual restriction that was recently placed was to not allow changing a vCPU's CPUID, once a vCPU was in a guest mode once. I also understand what you mean in regard to the commit 275a87244ec8 but IMHO this part of the commit message is only adding to the confusion. I think that this will be a better commit message: "Rework and rename cpuid_get_supported_xcr0() to explicitly operate on vCPU state i.e. on a vCPU's CPUID state. This is needed because KVM permits different vCPUs to have different CPUIDs, and thus it is valid for each vCPU to have different set of supported bits in the XCR0" > -- > > > Is it really allowed in x86 spec to have different supported mask of XCR0 bits > > on different CPUs (assuming all CPUs of the same type)? > > Yes, nothing in the SDM explicitly states that all cores in have identical feature > sets. And "assuming all CPUs of the same type" isn't really a valid constraint > because it's very doable to put different SKUs into a multi-socket system. > > Intel even (somewhat inadvertantly) kinda sorta shipped such CPUs, as Alder Lake > P-cores support AVX512 but E-cores do not, and IIRC early (pre-production?) BIOS > didn't disable AVX512 on the P-Cores, i.e. software could observe cores with and > without AVX512. That quickly got fixed because it confused software, but until > Intel squashed AVX512 entirely with a microcode update, disabling E-Cores in BIOS > would effectively enable AVX512 on the remaining P-Cores. Yea, sure I know about this, that's why I said "same type". I just was under the impression that KVM doesn't 'officially' support heterogeneous vCPUs and recently added a check to ensure that all vCPUs have the same CPUID. Now I understand. > > And it's not XCR0-related, but PMUs on Alder Lake (and all Intel hybrid CPUs) are > truly heterogenous. It's a mess for virtualization, but concrete proof that there > are no architectural guarantees regarding homogeneity of feature sets. > > > If true, does KVM supports it? > > Yes. Whether or not that's a good thing is definitely debatle, bug KVM's ABI for > a very long time has allowed userspace to expose whatever it wants via KVM_SET_CPUID. > > Getting (guest) software to play nice is an entirely different matter, but exposing > heterogenous vCPUs isn't an architectural violation. > Best regards, Maxim Levitsky
diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c index 0544e30b4946..7c3e4a550ca7 100644 --- a/arch/x86/kvm/cpuid.c +++ b/arch/x86/kvm/cpuid.c @@ -247,21 +247,6 @@ void kvm_update_pv_runtime(struct kvm_vcpu *vcpu) vcpu->arch.pv_cpuid.features = best->eax; } -/* - * Calculate guest's supported XCR0 taking into account guest CPUID data and - * KVM's supported XCR0 (comprised of host's XCR0 and KVM_SUPPORTED_XCR0). - */ -static u64 cpuid_get_supported_xcr0(struct kvm_cpuid_entry2 *entries, int nent) -{ - struct kvm_cpuid_entry2 *best; - - best = cpuid_entry2_find(entries, nent, 0xd, 0); - if (!best) - return 0; - - return (best->eax | ((u64)best->edx << 32)) & kvm_caps.supported_xcr0; -} - static void __kvm_update_cpuid_runtime(struct kvm_vcpu *vcpu, struct kvm_cpuid_entry2 *entries, int nent) { @@ -312,6 +297,21 @@ void kvm_update_cpuid_runtime(struct kvm_vcpu *vcpu) } EXPORT_SYMBOL_GPL(kvm_update_cpuid_runtime); +/* + * Calculate guest's supported XCR0 taking into account guest CPUID data and + * KVM's supported XCR0 (comprised of host's XCR0 and KVM_SUPPORTED_XCR0). + */ +static u64 vcpu_get_supported_xcr0(struct kvm_vcpu *vcpu) +{ + struct kvm_cpuid_entry2 *best; + + best = kvm_find_cpuid_entry_index(vcpu, 0xd, 0); + if (!best) + return 0; + + return (best->eax | ((u64)best->edx << 32)) & kvm_caps.supported_xcr0; +} + static bool kvm_cpuid_has_hyperv(struct kvm_cpuid_entry2 *entries, int nent) { struct kvm_cpuid_entry2 *entry; @@ -357,8 +357,7 @@ static void kvm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu) kvm_apic_set_version(vcpu); } - vcpu->arch.guest_supported_xcr0 = - cpuid_get_supported_xcr0(vcpu->arch.cpuid_entries, vcpu->arch.cpuid_nent); + vcpu->arch.guest_supported_xcr0 = vcpu_get_supported_xcr0(vcpu); /* * FP+SSE can always be saved/restored via KVM_{G,S}ET_XSAVE, even if