public inbox for kvm@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/3] x86/cpu: KVM: Make SGX and VMX depend on FEAT_CTL
@ 2022-12-03  0:37 Sean Christopherson
  2022-12-03  0:37 ` [PATCH 1/3] x86/cpu: Process all CPUID dependencies after identifying CPU info Sean Christopherson
                   ` (2 more replies)
  0 siblings, 3 replies; 11+ messages in thread
From: Sean Christopherson @ 2022-12-03  0:37 UTC (permalink / raw)
  To: Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86,
	Sean Christopherson, Paolo Bonzini
  Cc: H. Peter Anvin, linux-kernel, kvm

"Officially" make SGX and VMX depend on X86_FEATURE_MSR_IA32_FEAT_CTL,
and drop manual checks on X86_FEATURE_MSR_IA32_FEAT_CTL when querying
VMX support.

To make dependencies on MSR_IA32_FEAT_CTL work as expected, process all
CPUID dependencies at the end of CPU indentification.  Because
MSR_IA32_FEAT_CTL is a synthetic flag, it is effectively off-by-default,
and thus may never be unset via clear_cpu_cap(), i.e. never triggers
processing of its dependents.

The obvious alternative would be to explicitly clear MSR_IA32_FEAT_CTL if
the MSR is unsupported, but that ends up being rather ugly as it would
require clearing the flag in default_init() to handle the scenario where
hardware supports the MSR, but the kernel was built without support for the
CPU vendor.  E.g. running on an Intel CPU with CPU_SUP_INTEL=n.  This edge
case is also why the existing manual checks in KVM are necessary; KVM_INTEL
effectively depends on any of CPU_SUP_{INTEL,CENATUR,ZHAOXIN}.

Processing all dependencies also seems like the correct thing to do across
the board, e.g. if the kernel ends up with more synthetic features with
dependents.

The placement of the call to apply_cpuid_deps() isn't super scientific.  I
placed it after, AFAICT, the overwhelming majority of cpu cap updates had
already been done, but before anything was likely to want the dependencies
to be processed.  Specifically, I couldn't find any set_cpu_caps() in the
machine check code, but there are definitely cpu_has() calls under
mcheck_cpu_init().

Last thought, patch 3 will conflict with at least one in-flight KVM series[*].
The conflict should be straightfoward to resolve, but at the same time this
is far from urgent, i.e. kicking this series down the road until KVM settles
down is totally ok.

[*] https://lore.kernel.org/all/20221130230934.1014142-1-seanjc@google.com

Sean Christopherson (3):
  x86/cpu: Process all CPUID dependencies after identifying CPU info
  x86/cpu: Mark SGX and VMX as being dependent on MSR_IA32_FEAT_CTL
  KVM: VMX: Drop manual checks on X86_FEATURE_MSR_IA32_FEAT_CTL

 arch/x86/include/asm/cpufeature.h |  1 +
 arch/x86/kernel/cpu/bugs.c        |  3 +--
 arch/x86/kernel/cpu/common.c      |  6 ++++++
 arch/x86/kernel/cpu/cpuid-deps.c  | 12 ++++++++++++
 arch/x86/kernel/cpu/feat_ctl.c    |  3 +--
 arch/x86/kvm/vmx/vmx.c            |  6 ++----
 6 files changed, 23 insertions(+), 8 deletions(-)


base-commit: d800169041c0e035160c8b81f30d4b7e8f8ef777
-- 
2.39.0.rc0.267.gcb52ba06e7-goog


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH 1/3] x86/cpu: Process all CPUID dependencies after identifying CPU info
  2022-12-03  0:37 [PATCH 0/3] x86/cpu: KVM: Make SGX and VMX depend on FEAT_CTL Sean Christopherson
@ 2022-12-03  0:37 ` Sean Christopherson
  2022-12-08 16:14   ` Borislav Petkov
  2022-12-03  0:37 ` [PATCH 2/3] x86/cpu: Mark SGX and VMX as being dependent on MSR_IA32_FEAT_CTL Sean Christopherson
  2022-12-03  0:37 ` [PATCH 3/3] KVM: VMX: Drop manual checks on X86_FEATURE_MSR_IA32_FEAT_CTL Sean Christopherson
  2 siblings, 1 reply; 11+ messages in thread
From: Sean Christopherson @ 2022-12-03  0:37 UTC (permalink / raw)
  To: Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86,
	Sean Christopherson, Paolo Bonzini
  Cc: H. Peter Anvin, linux-kernel, kvm

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(+)

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);
+	}
+}
-- 
2.39.0.rc0.267.gcb52ba06e7-goog


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 2/3] x86/cpu: Mark SGX and VMX as being dependent on MSR_IA32_FEAT_CTL
  2022-12-03  0:37 [PATCH 0/3] x86/cpu: KVM: Make SGX and VMX depend on FEAT_CTL Sean Christopherson
  2022-12-03  0:37 ` [PATCH 1/3] x86/cpu: Process all CPUID dependencies after identifying CPU info Sean Christopherson
@ 2022-12-03  0:37 ` Sean Christopherson
  2022-12-03  0:37 ` [PATCH 3/3] KVM: VMX: Drop manual checks on X86_FEATURE_MSR_IA32_FEAT_CTL Sean Christopherson
  2 siblings, 0 replies; 11+ messages in thread
From: Sean Christopherson @ 2022-12-03  0:37 UTC (permalink / raw)
  To: Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86,
	Sean Christopherson, Paolo Bonzini
  Cc: H. Peter Anvin, linux-kernel, kvm

Mark SGX and VMX as being dependent on MSR_IA32_FEAT_CTL now that CPUID
dependencies are processed unconditionally, i.e. now that SGX and VMX
will be disabled if MSR_IA32_FEAT_CTL is unsupported even if the kernel
never explicitly disables MSR_IA32_FEAT_CTL.  Since init_ia32_feat_ctl()
is invoked if and only if the CPU might possibly support the MSR and the
kernel was built with the necessary CPU_SUP_*=y, it's possible for a CPU
that supports VMX and/or SGX to run on a kernel that never sets the
feature flag.

Explicitly clear MSR_IA32_FEAT_CTL if reading the MSR faults to handle
the extremely unlikely edge case where the RDMSR fails when restoring CPU
state after suspend, hibernate, kexec, etc.

Capturing the SGX and VMX dependencies will allow dropping manual checks
on X86_FEATURE_MSR_IA32_FEAT_CTL for code that just wants to detect if
SGX or VMX is fully supported.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kernel/cpu/cpuid-deps.c | 2 ++
 arch/x86/kernel/cpu/feat_ctl.c   | 3 +--
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/cpu/cpuid-deps.c b/arch/x86/kernel/cpu/cpuid-deps.c
index 68e26d4c8063..37abdb6fb4ea 100644
--- a/arch/x86/kernel/cpu/cpuid-deps.c
+++ b/arch/x86/kernel/cpu/cpuid-deps.c
@@ -72,6 +72,8 @@ static const struct cpuid_dep cpuid_deps[] = {
 	{ X86_FEATURE_AVX512_FP16,		X86_FEATURE_AVX512BW  },
 	{ X86_FEATURE_ENQCMD,			X86_FEATURE_XSAVES    },
 	{ X86_FEATURE_PER_THREAD_MBA,		X86_FEATURE_MBA       },
+	{ X86_FEATURE_VMX,			X86_FEATURE_MSR_IA32_FEAT_CTL },
+	{ X86_FEATURE_SGX,			X86_FEATURE_MSR_IA32_FEAT_CTL },
 	{ X86_FEATURE_SGX_LC,			X86_FEATURE_SGX	      },
 	{ X86_FEATURE_SGX1,			X86_FEATURE_SGX       },
 	{ X86_FEATURE_SGX2,			X86_FEATURE_SGX1      },
diff --git a/arch/x86/kernel/cpu/feat_ctl.c b/arch/x86/kernel/cpu/feat_ctl.c
index 03851240c3e3..0b7186d9ba05 100644
--- a/arch/x86/kernel/cpu/feat_ctl.c
+++ b/arch/x86/kernel/cpu/feat_ctl.c
@@ -117,8 +117,7 @@ void init_ia32_feat_ctl(struct cpuinfo_x86 *c)
 	u64 msr;
 
 	if (rdmsrl_safe(MSR_IA32_FEAT_CTL, &msr)) {
-		clear_cpu_cap(c, X86_FEATURE_VMX);
-		clear_cpu_cap(c, X86_FEATURE_SGX);
+		clear_cpu_cap(c, X86_FEATURE_MSR_IA32_FEAT_CTL);
 		return;
 	}
 
-- 
2.39.0.rc0.267.gcb52ba06e7-goog


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 3/3] KVM: VMX: Drop manual checks on X86_FEATURE_MSR_IA32_FEAT_CTL
  2022-12-03  0:37 [PATCH 0/3] x86/cpu: KVM: Make SGX and VMX depend on FEAT_CTL Sean Christopherson
  2022-12-03  0:37 ` [PATCH 1/3] x86/cpu: Process all CPUID dependencies after identifying CPU info Sean Christopherson
  2022-12-03  0:37 ` [PATCH 2/3] x86/cpu: Mark SGX and VMX as being dependent on MSR_IA32_FEAT_CTL Sean Christopherson
@ 2022-12-03  0:37 ` Sean Christopherson
  2 siblings, 0 replies; 11+ messages in thread
From: Sean Christopherson @ 2022-12-03  0:37 UTC (permalink / raw)
  To: Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86,
	Sean Christopherson, Paolo Bonzini
  Cc: H. Peter Anvin, linux-kernel, kvm

Drop manual checks on X86_FEATURE_MSR_IA32_FEAT_CTL and instead rely
solely on the X86_FEATURE_VMX check to detect VMX support now that VMX is
disabled via apply_cpuid_deps() if X86_FEATURE_MSR_IA32_FEAT_CTL isn't
supported.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kernel/cpu/bugs.c | 3 +--
 arch/x86/kvm/vmx/vmx.c     | 6 ++----
 2 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
index 9e84b685328f..3071e2a97f0d 100644
--- a/arch/x86/kernel/cpu/bugs.c
+++ b/arch/x86/kernel/cpu/bugs.c
@@ -2222,8 +2222,7 @@ static ssize_t l1tf_show_state(char *buf)
 
 static ssize_t itlb_multihit_show_state(char *buf)
 {
-	if (!boot_cpu_has(X86_FEATURE_MSR_IA32_FEAT_CTL) ||
-	    !boot_cpu_has(X86_FEATURE_VMX))
+	if (!boot_cpu_has(X86_FEATURE_VMX))
 		return sysfs_emit(buf, "KVM: Mitigation: VMX unsupported\n");
 	else if (!(cr4_read_shadow() & X86_CR4_VMXE))
 		return sysfs_emit(buf, "KVM: Mitigation: VMX disabled\n");
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 9dba04b6b019..de3fe3932ee8 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -2423,8 +2423,7 @@ static __init int cpu_has_kvm_support(void)
 
 static __init int vmx_disabled_by_bios(void)
 {
-	return !boot_cpu_has(X86_FEATURE_MSR_IA32_FEAT_CTL) ||
-	       !boot_cpu_has(X86_FEATURE_VMX);
+	return !boot_cpu_has(X86_FEATURE_VMX);
 }
 
 static int kvm_cpu_vmxon(u64 vmxon_pointer)
@@ -7413,8 +7412,7 @@ static int __init vmx_check_processor_compat(void)
 	struct vmcs_config vmcs_conf;
 	struct vmx_capability vmx_cap;
 
-	if (!this_cpu_has(X86_FEATURE_MSR_IA32_FEAT_CTL) ||
-	    !this_cpu_has(X86_FEATURE_VMX)) {
+	if (!this_cpu_has(X86_FEATURE_VMX)) {
 		pr_err("kvm: VMX is disabled on CPU %d\n", smp_processor_id());
 		return -EIO;
 	}
-- 
2.39.0.rc0.267.gcb52ba06e7-goog


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/3] x86/cpu: Process all CPUID dependencies after identifying CPU info
  2022-12-03  0:37 ` [PATCH 1/3] x86/cpu: Process all CPUID dependencies after identifying CPU info Sean Christopherson
@ 2022-12-08 16:14   ` Borislav Petkov
  2022-12-08 16:26     ` Sean Christopherson
  0 siblings, 1 reply; 11+ messages in thread
From: Borislav Petkov @ 2022-12-08 16:14 UTC (permalink / raw)
  To: Sean Christopherson
  Cc: Thomas Gleixner, Ingo Molnar, Dave Hansen, x86, Paolo Bonzini,
	H. Peter Anvin, linux-kernel, kvm

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...

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/3] x86/cpu: Process all CPUID dependencies after identifying CPU info
  2022-12-08 16:14   ` Borislav Petkov
@ 2022-12-08 16:26     ` Sean Christopherson
  2022-12-08 16:45       ` Borislav Petkov
  0 siblings, 1 reply; 11+ messages in thread
From: Sean Christopherson @ 2022-12-08 16:26 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: Thomas Gleixner, Ingo Molnar, Dave Hansen, x86, Paolo Bonzini,
	H. Peter Anvin, linux-kernel, kvm

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.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/3] x86/cpu: Process all CPUID dependencies after identifying CPU info
  2022-12-08 16:26     ` Sean Christopherson
@ 2022-12-08 16:45       ` Borislav Petkov
  2023-01-04 21:02         ` Sean Christopherson
  0 siblings, 1 reply; 11+ messages in thread
From: Borislav Petkov @ 2022-12-08 16:45 UTC (permalink / raw)
  To: Sean Christopherson
  Cc: Thomas Gleixner, Ingo Molnar, Dave Hansen, x86, Paolo Bonzini,
	H. Peter Anvin, linux-kernel, kvm

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.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/3] x86/cpu: Process all CPUID dependencies after identifying CPU info
  2022-12-08 16:45       ` Borislav Petkov
@ 2023-01-04 21:02         ` Sean Christopherson
  2023-01-04 22:55           ` Borislav Petkov
  0 siblings, 1 reply; 11+ messages in thread
From: Sean Christopherson @ 2023-01-04 21:02 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: Thomas Gleixner, Ingo Molnar, Dave Hansen, x86, Paolo Bonzini,
	H. Peter Anvin, linux-kernel, kvm

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.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/3] x86/cpu: Process all CPUID dependencies after identifying CPU info
  2023-01-04 21:02         ` Sean Christopherson
@ 2023-01-04 22:55           ` Borislav Petkov
  2023-01-04 23:18             ` Sean Christopherson
  0 siblings, 1 reply; 11+ messages in thread
From: Borislav Petkov @ 2023-01-04 22:55 UTC (permalink / raw)
  To: Sean Christopherson
  Cc: Thomas Gleixner, Ingo Molnar, Dave Hansen, x86, Paolo Bonzini,
	H. Peter Anvin, linux-kernel, kvm

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. :)

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/3] x86/cpu: Process all CPUID dependencies after identifying CPU info
  2023-01-04 22:55           ` Borislav Petkov
@ 2023-01-04 23:18             ` Sean Christopherson
  2023-01-05 10:15               ` Borislav Petkov
  0 siblings, 1 reply; 11+ messages in thread
From: Sean Christopherson @ 2023-01-04 23:18 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: Thomas Gleixner, Ingo Molnar, Dave Hansen, x86, Paolo Bonzini,
	H. Peter Anvin, linux-kernel, kvm

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.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/3] x86/cpu: Process all CPUID dependencies after identifying CPU info
  2023-01-04 23:18             ` Sean Christopherson
@ 2023-01-05 10:15               ` Borislav Petkov
  0 siblings, 0 replies; 11+ messages in thread
From: Borislav Petkov @ 2023-01-05 10:15 UTC (permalink / raw)
  To: Sean Christopherson
  Cc: Thomas Gleixner, Ingo Molnar, Dave Hansen, x86, Paolo Bonzini,
	H. Peter Anvin, linux-kernel, kvm

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... ?

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2023-01-05 10:15 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-12-03  0:37 [PATCH 0/3] x86/cpu: KVM: Make SGX and VMX depend on FEAT_CTL Sean Christopherson
2022-12-03  0:37 ` [PATCH 1/3] x86/cpu: Process all CPUID dependencies after identifying CPU info Sean Christopherson
2022-12-08 16:14   ` Borislav Petkov
2022-12-08 16:26     ` Sean Christopherson
2022-12-08 16:45       ` Borislav Petkov
2023-01-04 21:02         ` Sean Christopherson
2023-01-04 22:55           ` Borislav Petkov
2023-01-04 23:18             ` Sean Christopherson
2023-01-05 10:15               ` Borislav Petkov
2022-12-03  0:37 ` [PATCH 2/3] x86/cpu: Mark SGX and VMX as being dependent on MSR_IA32_FEAT_CTL Sean Christopherson
2022-12-03  0:37 ` [PATCH 3/3] KVM: VMX: Drop manual checks on X86_FEATURE_MSR_IA32_FEAT_CTL Sean Christopherson

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox