Message ID | 20221203003745.1475584-2-seanjc@google.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:adf:f944:0:0:0:0:0 with SMTP id q4csp1142546wrr; Fri, 2 Dec 2022 16:41:18 -0800 (PST) X-Google-Smtp-Source: AA0mqf6T6TIx2VR/o1csF6S9UKjpcM6Dft4CmLDPyXxjH0/31MynxN4w8bS9pMbm+gI0WMuhVPvZ X-Received: by 2002:a17:906:a408:b0:7c0:b4ae:d802 with SMTP id l8-20020a170906a40800b007c0b4aed802mr7826572ejz.283.1670028078657; Fri, 02 Dec 2022 16:41:18 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1670028078; cv=none; d=google.com; s=arc-20160816; b=sDygcr7EuCOBkWoQnFT86nDWPhmXGMBsR7s9wlywWeMkEV1IDDNToTcwmUC2parYTI 8MSWEm18CQOsnzUr7oTtoCHfx8ST6FwsDJoiAFFuNgsbpy/533VUL8WgMpHMJIdPNaAk 2ZKVRnlrDlKZBmI/7bPYT5K8q7skVT9oOaKs8HK4nNUuqbz+8lcC2cKMEAIy6tJYX8uJ XxEFuAiUtjDFx978kK+7/i4dM7GlGdJxzZhHQJTcRvaqeB/19gYcyUr0pA2/omsYdsSy 6IT46G3c4d5RsPMdKP8k9sCgxlRwX5bwkYtzHz4aoh2zHIgTQn6HfMl7vW7Z/6cVD0AC 1I9A== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:cc:to:from:subject:message-id:references :mime-version:in-reply-to:date:reply-to:dkim-signature; bh=MQ2DipcqCIIlOynGgQG74Sc/21NET4A4uu5IriUFE+o=; b=hflASkXgROTWoQiO7F3u3R25qLXGxoq3qY1k76Fshmox5CNUII1/AEnMZBjol2hzyl knB3x0YNvvoT9s1mxRvPHMDKY8wopwuyaIx9648XIplAjnz/okVywCj2NJWfZGDH0LZL 8MEA4+aBRWF37h//4P1gsCoDGW+75isBBMBMoB31rk/xbd6P/dBDWWRQMxtPuzV65KeS XJfxiykp/xcwB1zlD/t7anBY5FA1sXwSzCo1BFRQ+nAIo56Tp4k5LHDFv061Ga6KFVnU zfjKljeilMZZ/Mm/0WSVqzadXgV51bMPUVDQJEmiLFWYi7Ib34PMC9hdQjXoQJ5J0Mtk JaTw== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@google.com header.s=20210112 header.b=SxXUc63M; 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=REJECT sp=REJECT dis=NONE) header.from=google.com Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id b2-20020aa7d482000000b0046b0dcc24dasi6877625edr.403.2022.12.02.16.40.53; Fri, 02 Dec 2022 16:41:18 -0800 (PST) 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=@google.com header.s=20210112 header.b=SxXUc63M; 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=REJECT sp=REJECT dis=NONE) header.from=google.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S235045AbiLCAkc (ORCPT <rfc822;lhua1029@gmail.com> + 99 others); Fri, 2 Dec 2022 19:40:32 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:41318 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S234997AbiLCAkF (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Fri, 2 Dec 2022 19:40:05 -0500 Received: from mail-pf1-x449.google.com (mail-pf1-x449.google.com [IPv6:2607:f8b0:4864:20::449]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 30A071FCE2 for <linux-kernel@vger.kernel.org>; Fri, 2 Dec 2022 16:37:51 -0800 (PST) Received: by mail-pf1-x449.google.com with SMTP id d3-20020a056a0010c300b005728633819aso6231897pfu.8 for <linux-kernel@vger.kernel.org>; Fri, 02 Dec 2022 16:37:51 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=cc:to:from:subject:message-id:references:mime-version:in-reply-to :date:reply-to:from:to:cc:subject:date:message-id:reply-to; bh=MQ2DipcqCIIlOynGgQG74Sc/21NET4A4uu5IriUFE+o=; b=SxXUc63M6tJlnEa9G90sZBb/Ga5eyEFP62Gfiw9NU6XZfZ69koBRG2/Frc3Q8OO1Ir epCvPCR+Lwsf5k+Wh+9lsvuoCwEZ+NpFtEklIm5YibOyYi0oUg09BqcOqma9relrrOLi EXF2fSH3KYgb8w8AILUxXYy1mSxUcGwKEXfu9hMDmuhLLpFfVstM87mNxi16PLYjKpmd AlQ46jDB/4NYtyjm3Mg8LMVw09cCwYGC4dHCyNib2PcemJai46WlGhkOljYFlwvoH7Kh kMr79+aA+eGhiD9LwpFnZAOOq28GPIxB2reEE/YXVMGrdCSjj0bxIOaB7A2MJyAx7jCB ZRBQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:from:subject:message-id:references:mime-version:in-reply-to :date:reply-to:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=MQ2DipcqCIIlOynGgQG74Sc/21NET4A4uu5IriUFE+o=; b=yYOWv08F2UFCiMF6MzRXxZlqMaGLyOsDoV3rQ6HfQlYFL1ahFcqC7w9ul9qZciDq+F lV918pCik6vhF2/Q8fiyAGk1n3ObjJC18nxx1tzA+ZA15wBUYl4kMaNm0L4xKXVRzOrX 5XLF+K90xT6D5DI6jXzJ/zO5i4aS4K9ri9fZCgxLzTnGsX/pnF7QwHKiJXzJNid5nHkU eiyt0CMGMsdKZuac9QjuSv86Es705eYYN3fO5aZ9AAuq/cdUtQWcp2PtC8SgsBQkuGqy 7noi3IW0Ov6zQmFIiElpXohUPOEJyBP1cUzeZNs8RyOwjVWFqheLQ0u46f/qPHb2IRoo 12SA== X-Gm-Message-State: ANoB5pmC9dMNXaAomqv+AuI2GLckfG8f8LqCJo5X/jD4+IxZLY+ZYuBB tvon//PxLD+G3qR9vvGqSSPXFQ3O5IQ= X-Received: from zagreus.c.googlers.com ([fda3:e722:ac3:cc00:7f:e700:c0a8:5c37]) (user=seanjc job=sendgmr) by 2002:aa7:8dd3:0:b0:573:4ae5:e475 with SMTP id j19-20020aa78dd3000000b005734ae5e475mr54315834pfr.64.1670027870646; Fri, 02 Dec 2022 16:37:50 -0800 (PST) Reply-To: Sean Christopherson <seanjc@google.com> Date: Sat, 3 Dec 2022 00:37:43 +0000 In-Reply-To: <20221203003745.1475584-1-seanjc@google.com> Mime-Version: 1.0 References: <20221203003745.1475584-1-seanjc@google.com> X-Mailer: git-send-email 2.39.0.rc0.267.gcb52ba06e7-goog Message-ID: <20221203003745.1475584-2-seanjc@google.com> Subject: [PATCH 1/3] x86/cpu: Process all CPUID dependencies after identifying CPU info From: Sean Christopherson <seanjc@google.com> To: Thomas Gleixner <tglx@linutronix.de>, Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>, Dave Hansen <dave.hansen@linux.intel.com>, x86@kernel.org, Sean Christopherson <seanjc@google.com>, Paolo Bonzini <pbonzini@redhat.com> Cc: "H. Peter Anvin" <hpa@zytor.com>, linux-kernel@vger.kernel.org, kvm@vger.kernel.org Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-9.6 required=5.0 tests=BAYES_00,DKIMWL_WL_MED, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_NONE, SPF_HELO_NONE,SPF_PASS,USER_IN_DEF_DKIM_WL autolearn=unavailable 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?1751151362550320738?= X-GMAIL-MSGID: =?utf-8?q?1751151362550320738?= |
Series |
x86/cpu: KVM: Make SGX and VMX depend on FEAT_CTL
|
|
Commit Message
Sean Christopherson
Dec. 3, 2022, 12:37 a.m. UTC
Process all CPUID dependencies to ensure that a dependent is disabled if
one or more of its parent features is unsupported. As is, cpuid_deps is
processed if an only if a feature is explicitly disabled via
clear_cpu_cap(), which makes it annoying/dangerous to use cpuid_deps for
features whose parent(s) do not always have explicit processing.
E.g. VMX and SGX depend on the synthetic X86_FEATURE_MSR_IA32_FEAT_CTL,
but there is no common location to clear MSR_IA32_FEAT_CTL, and so
consumers of VMX and SGX are forced to check MSR_IA32_FEAT_CTL on top
of the dependent feature.
Manually clearing X86_FEATURE_MSR_IA32_FEAT_CTL is the obvious
alternative, but it's subtly more difficult that updating
init_ia32_feat_ctl(). CONFIG_IA32_FEAT_CTL depends on any of
CONFIG_CPU_SUP_{INTEL,CENATUR,ZHAOXIN}, but init_ia32_feat_ctl() is
invoked if and only if the actual CPU type matches one of the
aforementioned CPU_SUP_* types. E.g. running a kernel built with
CONFIG_CPU_SUP_INTEL=y
CONFIG_CPU_SUP_AMD=y
# CONFIG_CPU_SUP_HYGON is not set
# CONFIG_CPU_SUP_CENTAUR is not set
# CONFIG_CPU_SUP_ZHAOXIN is not set
on a Cenatur or Zhaoxin CPU will leave X86_FEATURE_VMX set but not set
X86_FEATURE_MSR_IA32_FEAT_CTL, and will never call init_ia32_feat_ctl()
to give the kernel a convenient opportunity to clear
X86_FEATURE_MSR_IA32_FEAT_CTL.
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
arch/x86/include/asm/cpufeature.h | 1 +
arch/x86/kernel/cpu/common.c | 6 ++++++
arch/x86/kernel/cpu/cpuid-deps.c | 10 ++++++++++
3 files changed, 17 insertions(+)
Comments
On Sat, Dec 03, 2022 at 12:37:43AM +0000, Sean Christopherson wrote: > Process all CPUID dependencies to ensure that a dependent is disabled if > one or more of its parent features is unsupported. Just out of curiosity: this is some weird guest configuration, right? Not addressing a real hw issue... > As is, cpuid_deps is > processed if an only if a feature is explicitly disabled via > clear_cpu_cap(), which makes it annoying/dangerous to use cpuid_deps for > features whose parent(s) do not always have explicit processing. > > E.g. VMX and SGX depend on the synthetic X86_FEATURE_MSR_IA32_FEAT_CTL, > but there is no common location to clear MSR_IA32_FEAT_CTL, and so > consumers of VMX and SGX are forced to check MSR_IA32_FEAT_CTL on top > of the dependent feature. > > Manually clearing X86_FEATURE_MSR_IA32_FEAT_CTL is the obvious > alternative, but it's subtly more difficult that updating > init_ia32_feat_ctl(). CONFIG_IA32_FEAT_CTL depends on any of > CONFIG_CPU_SUP_{INTEL,CENATUR,ZHAOXIN}, but init_ia32_feat_ctl() is > invoked if and only if the actual CPU type matches one of the > aforementioned CPU_SUP_* types. E.g. running a kernel built with > > CONFIG_CPU_SUP_INTEL=y > CONFIG_CPU_SUP_AMD=y > # CONFIG_CPU_SUP_HYGON is not set > # CONFIG_CPU_SUP_CENTAUR is not set > # CONFIG_CPU_SUP_ZHAOXIN is not set > > on a Cenatur or Zhaoxin CPU will leave X86_FEATURE_VMX set but not set Typo fix for the committer: Centaur > X86_FEATURE_MSR_IA32_FEAT_CTL, and will never call init_ia32_feat_ctl() > to give the kernel a convenient opportunity to clear > X86_FEATURE_MSR_IA32_FEAT_CTL. > > Signed-off-by: Sean Christopherson <seanjc@google.com> > --- > arch/x86/include/asm/cpufeature.h | 1 + > arch/x86/kernel/cpu/common.c | 6 ++++++ > arch/x86/kernel/cpu/cpuid-deps.c | 10 ++++++++++ > 3 files changed, 17 insertions(+) ... > diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c > index bf4ac1cb93d7..094fc69dba63 100644 > --- a/arch/x86/kernel/cpu/common.c > +++ b/arch/x86/kernel/cpu/common.c > @@ -1887,6 +1887,12 @@ static void identify_cpu(struct cpuinfo_x86 *c) > > ppin_init(c); > > + /* > + * Apply CPUID dependencies to ensure dependent features are disabled > + * if a parent feature is unsupported but wasn't explicitly disabled. > + */ > + apply_cpuid_deps(c); I'd probably call that resolve_cpuid_deps()... But yeah, that init path would need cleaning up at some point - all kinds of init detection functions have been hastily slapped there over the years...
On Thu, Dec 08, 2022, Borislav Petkov wrote: > On Sat, Dec 03, 2022 at 12:37:43AM +0000, Sean Christopherson wrote: > > Process all CPUID dependencies to ensure that a dependent is disabled if > > one or more of its parent features is unsupported. > > Just out of curiosity: this is some weird guest configuration, right? No, it's also relevant for bare metal. > Not addressing a real hw issue... But it's not really a hardware issue either. More like an admin/user issue. The problem is that if a kernel is built for subset of CPU types, e.g. just Intel or just Centaur, and then booted on an "unsupported" CPU type, init_ia32_feat_ctl() will never be invoked because ->c_init() will point a default_init(), and so the kernel never checks MSR_IA32_FEAT_CTL to see if VMX and/or SGX are fully enabled. E.g. if someone booted an "unsupported" kernel and also disabled VMX in BIOS, then the CPU will enumerate support for VMX in CPUID, but attempting to actually enable VMX will fail due to VMX being disabled in MSR_IA32_FEAT_CTL. > > diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c > > index bf4ac1cb93d7..094fc69dba63 100644 > > --- a/arch/x86/kernel/cpu/common.c > > +++ b/arch/x86/kernel/cpu/common.c > > @@ -1887,6 +1887,12 @@ static void identify_cpu(struct cpuinfo_x86 *c) > > > > ppin_init(c); > > > > + /* > > + * Apply CPUID dependencies to ensure dependent features are disabled > > + * if a parent feature is unsupported but wasn't explicitly disabled. > > + */ > > + apply_cpuid_deps(c); > > I'd probably call that resolve_cpuid_deps()... "resolve" works for me.
On Thu, Dec 08, 2022 at 04:26:29PM +0000, Sean Christopherson wrote: > But it's not really a hardware issue either. More like an admin/user issue. > > The problem is that if a kernel is built for subset of CPU types, e.g. just Intel > or just Centaur, and then booted on an "unsupported" CPU type, init_ia32_feat_ctl() > will never be invoked because ->c_init() will point a default_init(), and so the > kernel never checks MSR_IA32_FEAT_CTL to see if VMX and/or SGX are fully enabled. Yeah, you called it an "edge case". I'm wondering whether we should even worry about that case... I mean, the majority of Linuxes out there are allmodconfig-like kernels and booting on unsupported CPU type doesn't happen. Hell, I'd even say that if you attempt booting on unsupported CPU type, we should simply fail that boot attempt. I.e., what validate_cpu() does in some cases. IOW, I don't mind what you're doing but I wonder whether we should even go the trouble to do so or simply deny that by saying "Well, don't do that then". Hmmm.
On Thu, Dec 08, 2022, Borislav Petkov wrote: > On Thu, Dec 08, 2022 at 04:26:29PM +0000, Sean Christopherson wrote: > > But it's not really a hardware issue either. More like an admin/user issue. > > > > The problem is that if a kernel is built for subset of CPU types, e.g. just Intel > > or just Centaur, and then booted on an "unsupported" CPU type, init_ia32_feat_ctl() > > will never be invoked because ->c_init() will point a default_init(), and so the > > kernel never checks MSR_IA32_FEAT_CTL to see if VMX and/or SGX are fully enabled. > > Yeah, you called it an "edge case". I'm wondering whether we should even > worry about that case... > > I mean, the majority of Linuxes out there are allmodconfig-like kernels > and booting on unsupported CPU type doesn't happen. > > Hell, I'd even say that if you attempt booting on unsupported CPU type, > we should simply fail that boot attempt. > > I.e., what validate_cpu() does in some cases. > > IOW, I don't mind what you're doing but I wonder whether we should even > go the trouble to do so or simply deny that by saying "Well, don't do > that then". I agree with the "don't do that" sentiment, but IMO refusing to boot is too much. Unlike the validate_cpu() cases, the kernel can likely boot and run just fine, albeit with limited feature enabling. And there's a non-zero chance we'd end up with a kernel param to allow booting unknown CPUs, e.g. for people doing weird things with VMs or running old, esoteric hardware. At that point we'd end up with a more complex implementation than processing dependencies on synthetic flags, especially if there's ever a more legitimate need to process such dependencies.
On Wed, Jan 04, 2023 at 09:02:04PM +0000, Sean Christopherson wrote: > And there's a non-zero chance we'd end up with a kernel param to allow booting > unknown CPUs, e.g. for people doing weird things with VMs or running old, esoteric > hardware. At that point we'd end up with a more complex implementation than > processing dependencies on synthetic flags, especially if there's ever a more > legitimate need to process such dependencies. I'm sorry but I'm still unclear on what actual use care are we even fixing here? If it is about people who'd like to tinker with old hw or doing weird VM things, they can just as well adjust their kernel .configs and rebuild. Peeking around your patchset, if all this is about dropping the X86_FEATURE_MSR_IA32_FEAT_CTL check and checking only X86_FEATURE_VMX and in order to do that, you want to cover those obscure cases where init_ia32_feat_ctl() won't get run, then sure, I guess - changes look simple enough. :)
On Wed, Jan 04, 2023, Borislav Petkov wrote: > On Wed, Jan 04, 2023 at 09:02:04PM +0000, Sean Christopherson wrote: > > And there's a non-zero chance we'd end up with a kernel param to allow booting > > unknown CPUs, e.g. for people doing weird things with VMs or running old, esoteric > > hardware. At that point we'd end up with a more complex implementation than > > processing dependencies on synthetic flags, especially if there's ever a more > > legitimate need to process such dependencies. > > I'm sorry but I'm still unclear on what actual use care are we even fixing here? There's no fix. What I was trying to say is that modifying the kernel to refuse to boot on unknown CPUs is opening a can of worms for very little benefit. > If it is about people who'd like to tinker with old hw or doing weird VM things, > they can just as well adjust their kernel .configs and rebuild. > > Peeking around your patchset, if all this is about dropping the > X86_FEATURE_MSR_IA32_FEAT_CTL check and checking only X86_FEATURE_VMX and in > order to do that, you want to cover those obscure cases where > init_ia32_feat_ctl() won't get run, then sure, I guess - changes look simple > enough. :) Yes, this is purely to drop the explicit X86_FEATURE_MSR_IA32_FEAT_CTL checks. Alternatively, we could just drop the checks without processing the dependency, i.e. take the stance that running KVM with a funky .config is a user error, but that feels unnecessarily hostile since it's quite easy to play nice. Or I guess do nothing and carry the explicit checks.
On Wed, Jan 04, 2023 at 11:18:31PM +0000, Sean Christopherson wrote:
> Yes, this is purely to drop the explicit X86_FEATURE_MSR_IA32_FEAT_CTL checks.
Yeah, we can do that as it is simple enough.
Btw, we already resolve deps - or forced features but whatever, it is similar -
in apply_forced_caps(). And we call it right before we "sync" the feature bit
arrays with boot_cpu_data's in identify_cpu().
So I'm thinking this all, including your change, should be carved out in a
separate function and all the CPU flags massaging should be concentrated there.
And that should happen last in identify_cpu() - that ppin_init() thing sets and
clears cpu caps too. ;-\
But I can do that ontop, so how do you wanna merge this?
I take it or I review it and you take it or... ?
diff --git a/arch/x86/include/asm/cpufeature.h b/arch/x86/include/asm/cpufeature.h index 1a85e1fb0922..c4408d03b180 100644 --- a/arch/x86/include/asm/cpufeature.h +++ b/arch/x86/include/asm/cpufeature.h @@ -147,6 +147,7 @@ extern const char * const x86_bug_flags[NBUGINTS*32]; extern void setup_clear_cpu_cap(unsigned int bit); extern void clear_cpu_cap(struct cpuinfo_x86 *c, unsigned int bit); +extern void apply_cpuid_deps(struct cpuinfo_x86 *c); #define setup_force_cpu_cap(bit) do { \ set_cpu_cap(&boot_cpu_data, bit); \ diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c index bf4ac1cb93d7..094fc69dba63 100644 --- a/arch/x86/kernel/cpu/common.c +++ b/arch/x86/kernel/cpu/common.c @@ -1887,6 +1887,12 @@ static void identify_cpu(struct cpuinfo_x86 *c) ppin_init(c); + /* + * Apply CPUID dependencies to ensure dependent features are disabled + * if a parent feature is unsupported but wasn't explicitly disabled. + */ + apply_cpuid_deps(c); + /* Init Machine Check Exception if available. */ mcheck_cpu_init(c); diff --git a/arch/x86/kernel/cpu/cpuid-deps.c b/arch/x86/kernel/cpu/cpuid-deps.c index c881bcafba7d..68e26d4c8063 100644 --- a/arch/x86/kernel/cpu/cpuid-deps.c +++ b/arch/x86/kernel/cpu/cpuid-deps.c @@ -138,3 +138,13 @@ void setup_clear_cpu_cap(unsigned int feature) { do_clear_cpu_cap(NULL, feature); } + +void apply_cpuid_deps(struct cpuinfo_x86 *c) +{ + const struct cpuid_dep *d; + + for (d = cpuid_deps; d->feature; d++) { + if (!cpu_has(c, d->depends)) + clear_cpu_cap(c, d->feature); + } +}