| Message ID | 20221203003745.1475584-2-seanjc@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/cpu: KVM: Make SGX and VMX depend on FEAT_CTL | expand |
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); + } +}
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(+)