| Message ID | 20221102231911.3107438-34-seanjc@google.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | KVM: Rework kvm_init() and hardware enabling | expand |
| Context | Check | Description |
|---|---|---|
| conchuod/apply | fail | Patch does not apply to for-next |
| conchuod/tree_selection | success | Guessing tree name failed |
On 11/3/22 00:19, Sean Christopherson wrote: > + if (!boot_cpu_has(X86_FEATURE_MSR_IA32_FEAT_CTL) || > + !boot_cpu_has(X86_FEATURE_VMX)) { > + pr_err("VMX not enabled in MSR_IA32_FEAT_CTL\n"); > + return false; I think the reference to the BIOS should remain in these messages and in svm.c (even though these days it's much less common for vendors to default to disabled virtualization in the system setup). The check for X86_FEATURE_MSR_IA32_FEAT_CTL is not needed because init_ia32_feat_ctl() will clear X86_FEATURE_VMX if the rdmsr fail (and not set X86_FEATURE_MSR_IA32_FEAT_CTL). Paolo
On Thu, Nov 03, 2022, Paolo Bonzini wrote: > On 11/3/22 00:19, Sean Christopherson wrote: > > + if (!boot_cpu_has(X86_FEATURE_MSR_IA32_FEAT_CTL) || > > + !boot_cpu_has(X86_FEATURE_VMX)) { > > + pr_err("VMX not enabled in MSR_IA32_FEAT_CTL\n"); > > + return false; > > I think the reference to the BIOS should remain in these messages and in > svm.c (even though these days it's much less common for vendors to default > to disabled virtualization in the system setup). Ya, I'll figure out a way to mention BIOS/firmware. > The check for X86_FEATURE_MSR_IA32_FEAT_CTL is not needed because > init_ia32_feat_ctl() will clear X86_FEATURE_VMX if the rdmsr fail (and not > set X86_FEATURE_MSR_IA32_FEAT_CTL). It's technically required. IA32_FEAT_CTL and thus KVM_INTEL depends on any of 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_*. 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. If VMX isn't enabled in MSR_IA32_FEAT_CTL, KVM will get unexpected #UDs when trying to enable VMX.
On 11/3/22 19:35, Sean Christopherson wrote: > It's technically required. IA32_FEAT_CTL and thus KVM_INTEL depends on any of > 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_*. > > 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. If VMX isn't enabled in MSR_IA32_FEAT_CTL, KVM > will get unexpected #UDs when trying to enable VMX. Oh, I see. Perhaps X86_FEATURE_VMX and X86_FEATURE_SGX should be moved to one of the software words instead of using cpuid. Nothing that you should care about for this series though. Paolo
On Thu, Nov 03, 2022, Paolo Bonzini wrote: > On 11/3/22 19:35, Sean Christopherson wrote: > > It's technically required. IA32_FEAT_CTL and thus KVM_INTEL depends on any of > > 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_*. > > > > 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. If VMX isn't enabled in MSR_IA32_FEAT_CTL, KVM > > will get unexpected #UDs when trying to enable VMX. > > Oh, I see. Perhaps X86_FEATURE_VMX and X86_FEATURE_SGX should be moved to > one of the software words instead of using cpuid. Nothing that you should > care about for this series though. Or maybe something like this? diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c index 3e508f239098..ebe617ab0b37 100644 --- a/arch/x86/kernel/cpu/common.c +++ b/arch/x86/kernel/cpu/common.c @@ -191,6 +191,8 @@ static void default_init(struct cpuinfo_x86 *c) strcpy(c->x86_model_id, "386"); } #endif + + clear_cpu_cap(c, X86_FEATURE_MSR_IA32_FEAT_CTL); } static const struct cpu_dev default_cpu = { diff --git a/arch/x86/kernel/cpu/cpuid-deps.c b/arch/x86/kernel/cpu/cpuid-deps.c index c881bcafba7d..3a7ae67f5a5e 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 },
On 11/3/22 19:58, Sean Christopherson wrote: > > diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c > index 3e508f239098..ebe617ab0b37 100644 > --- a/arch/x86/kernel/cpu/common.c > +++ b/arch/x86/kernel/cpu/common.c > @@ -191,6 +191,8 @@ static void default_init(struct cpuinfo_x86 *c) > strcpy(c->x86_model_id, "386"); > } > #endif > + > + clear_cpu_cap(c, X86_FEATURE_MSR_IA32_FEAT_CTL); > } > > static const struct cpu_dev default_cpu = { Not needed I think? default_init does not call init_ia32_feat_ctl. > diff --git a/arch/x86/kernel/cpu/cpuid-deps.c b/arch/x86/kernel/cpu/cpuid-deps.c > index c881bcafba7d..3a7ae67f5a5e 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 }, > Yes, good idea. Paolo
On Fri, Nov 04, 2022, Paolo Bonzini wrote: > On 11/3/22 19:58, Sean Christopherson wrote: > > > > diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c > > index 3e508f239098..ebe617ab0b37 100644 > > --- a/arch/x86/kernel/cpu/common.c > > +++ b/arch/x86/kernel/cpu/common.c > > @@ -191,6 +191,8 @@ static void default_init(struct cpuinfo_x86 *c) > > strcpy(c->x86_model_id, "386"); > > } > > #endif > > + > > + clear_cpu_cap(c, X86_FEATURE_MSR_IA32_FEAT_CTL); > > } > > static const struct cpu_dev default_cpu = { > > Not needed I think? default_init does not call init_ia32_feat_ctl. cpuid_deps is only processed by do_clear_cpu_cap(), so unless there's an explicit "clear" action, the dependencies will not be updated. It kinda makes sense since hardware-based features shouldn't end up with scenarios where a dependent feature exists but the base feature does not (barring bad KVM setups :-) ). That said, this seems like a bug waiting to happen, and unless I'm missing something it's quite straightforward to process all dependencies during setup. Time to find out if Boris and co. agree :-) 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 3e508f239098..28ce31dadd7f 100644 --- a/arch/x86/kernel/cpu/common.c +++ b/arch/x86/kernel/cpu/common.c @@ -1884,6 +1884,8 @@ static void identify_cpu(struct cpuinfo_x86 *c) c->x86_capability[i] |= boot_cpu_data.x86_capability[i]; } + apply_cpuid_deps(c); + ppin_init(c); /* Init Machine Check Exception if available. */ diff --git a/arch/x86/kernel/cpu/cpuid-deps.c b/arch/x86/kernel/cpu/cpuid-deps.c index c881bcafba7d..7e91e97973ca 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->feature)) + clear_cpu_cap(c, d->feature); + } +}
On Wed, 2022-11-02 at 23:19 +0000, Sean Christopherson wrote: > +static bool __init kvm_is_vmx_supported(void) > +{ > + if (!cpu_has_vmx()) { > + pr_err("CPU doesn't support VMX\n"); > + return false; > + } > + > + if (!boot_cpu_has(X86_FEATURE_MSR_IA32_FEAT_CTL) || > + !boot_cpu_has(X86_FEATURE_VMX)) { > + pr_err("VMX not enabled in MSR_IA32_FEAT_CTL\n"); > + return false; > + } > + > + return true; > +} > + > 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)) { > - pr_err("VMX is disabled on CPU %d\n", smp_processor_id()); > + if (!kvm_is_vmx_supported()) > return -EIO; > - } > Looks there's a functional change here -- the old code checks local cpu's feature bits but the new code always checks bsp's feature bits. Should have no problem I think, though.
On Tue, Nov 15, 2022, Huang, Kai wrote: > On Wed, 2022-11-02 at 23:19 +0000, Sean Christopherson wrote: > > +static bool __init kvm_is_vmx_supported(void) > > +{ > > + if (!cpu_has_vmx()) { > > + pr_err("CPU doesn't support VMX\n"); > > + return false; > > + } > > + > > + if (!boot_cpu_has(X86_FEATURE_MSR_IA32_FEAT_CTL) || > > + !boot_cpu_has(X86_FEATURE_VMX)) { > > + pr_err("VMX not enabled in MSR_IA32_FEAT_CTL\n"); > > + return false; > > + } > > + > > + return true; > > +} > > + > > 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)) { > > - pr_err("VMX is disabled on CPU %d\n", smp_processor_id()); > > + if (!kvm_is_vmx_supported()) > > return -EIO; > > - } > > > > Looks there's a functional change here -- the old code checks local cpu's > feature bits but the new code always checks bsp's feature bits. Should have no > problem I think, though. Ouch. The bad check will defeat the purpose of doing compat checks. Nice catch!
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h index 58a7cb8d8e96..f223c845ed6e 100644 --- a/arch/x86/include/asm/kvm_host.h +++ b/arch/x86/include/asm/kvm_host.h @@ -1666,8 +1666,6 @@ struct kvm_x86_nested_ops { }; struct kvm_x86_init_ops { - int (*cpu_has_kvm_support)(void); - int (*disabled_by_bios)(void); int (*check_processor_compatibility)(void); int (*hardware_setup)(void); unsigned int (*handle_intel_pt_intr)(void); diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c index 3c48fb837302..3523d24d004b 100644 --- a/arch/x86/kvm/svm/svm.c +++ b/arch/x86/kvm/svm/svm.c @@ -525,21 +525,28 @@ static void svm_init_osvw(struct kvm_vcpu *vcpu) vcpu->arch.osvw.status |= 1; } -static int has_svm(void) +static bool kvm_is_svm_supported(void) { const char *msg; + u64 vm_cr; if (!cpu_has_svm(&msg)) { - printk(KERN_INFO "has_svm: %s\n", msg); - return 0; + pr_err("SVM not supported, %s\n", msg); + return false; } if (cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT)) { pr_info("KVM is unsupported when running as an SEV guest\n"); - return 0; + return false; } - return 1; + rdmsrl(MSR_VM_CR, vm_cr); + if (vm_cr & (1 << SVM_VM_CR_SVM_DISABLE)) { + pr_err("SVM disabled in MSR_VM_CR\n"); + return false; + } + + return true; } void __svm_write_tsc_multiplier(u64 multiplier) @@ -578,10 +585,9 @@ static int svm_hardware_enable(void) if (efer & EFER_SVME) return -EBUSY; - if (!has_svm()) { - pr_err("%s: err EOPNOTSUPP on %d\n", __func__, me); + if (!kvm_is_svm_supported()) return -EINVAL; - } + sd = per_cpu(svm_data, me); if (!sd) { pr_err("%s: svm_data is NULL on %d\n", __func__, me); @@ -4112,17 +4118,6 @@ static void svm_load_mmu_pgd(struct kvm_vcpu *vcpu, hpa_t root_hpa, vmcb_mark_dirty(svm->vmcb, VMCB_CR); } -static int is_disabled(void) -{ - u64 vm_cr; - - rdmsrl(MSR_VM_CR, vm_cr); - if (vm_cr & (1 << SVM_VM_CR_SVM_DISABLE)) - return 1; - - return 0; -} - static void svm_patch_hypercall(struct kvm_vcpu *vcpu, unsigned char *hypercall) { @@ -5121,8 +5116,6 @@ static __init int svm_hardware_setup(void) static struct kvm_x86_init_ops svm_init_ops __initdata = { - .cpu_has_kvm_support = has_svm, - .disabled_by_bios = is_disabled, .hardware_setup = svm_hardware_setup, .check_processor_compatibility = svm_check_processor_compat, @@ -5136,6 +5129,9 @@ static int __init svm_init(void) __unused_size_checks(); + if (!kvm_is_svm_supported()) + return -EOPNOTSUPP; + r = kvm_x86_vendor_init(&svm_init_ops); if (r) return r; diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c index 1b645f52cd8d..2a7e62d0707d 100644 --- a/arch/x86/kvm/vmx/vmx.c +++ b/arch/x86/kvm/vmx/vmx.c @@ -2485,17 +2485,6 @@ static void vmx_cache_reg(struct kvm_vcpu *vcpu, enum kvm_reg reg) } } -static __init int cpu_has_kvm_support(void) -{ - return cpu_has_vmx(); -} - -static __init int vmx_disabled_by_bios(void) -{ - return !boot_cpu_has(X86_FEATURE_MSR_IA32_FEAT_CTL) || - !boot_cpu_has(X86_FEATURE_VMX); -} - static int kvm_cpu_vmxon(u64 vmxon_pointer) { u64 msr; @@ -7477,16 +7466,29 @@ static int vmx_vm_init(struct kvm *kvm) return 0; } +static bool __init kvm_is_vmx_supported(void) +{ + if (!cpu_has_vmx()) { + pr_err("CPU doesn't support VMX\n"); + return false; + } + + if (!boot_cpu_has(X86_FEATURE_MSR_IA32_FEAT_CTL) || + !boot_cpu_has(X86_FEATURE_VMX)) { + pr_err("VMX not enabled in MSR_IA32_FEAT_CTL\n"); + return false; + } + + return true; +} + 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)) { - pr_err("VMX is disabled on CPU %d\n", smp_processor_id()); + if (!kvm_is_vmx_supported()) return -EIO; - } if (setup_vmcs_config(&vmcs_conf, &vmx_cap) < 0) return -EIO; @@ -8471,8 +8473,6 @@ static __init int hardware_setup(void) } static struct kvm_x86_init_ops vmx_init_ops __initdata = { - .cpu_has_kvm_support = cpu_has_kvm_support, - .disabled_by_bios = vmx_disabled_by_bios, .check_processor_compatibility = vmx_check_processor_compat, .hardware_setup = hardware_setup, .handle_intel_pt_intr = NULL, @@ -8517,6 +8517,9 @@ static int __init vmx_init(void) { int r, cpu; + if (!kvm_is_vmx_supported()) + return -EOPNOTSUPP; + hv_setup_evmcs(); r = kvm_x86_vendor_init(&vmx_init_ops); diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index 39675b9662d7..0c1778f3308a 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -9309,17 +9309,6 @@ static int __kvm_x86_vendor_init(struct kvm_x86_init_ops *ops) return -EEXIST; } - if (!ops->cpu_has_kvm_support()) { - pr_err_ratelimited("no hardware support for '%s'\n", - ops->runtime_ops->name); - return -EOPNOTSUPP; - } - if (ops->disabled_by_bios()) { - pr_err_ratelimited("support for '%s' disabled by bios\n", - ops->runtime_ops->name); - return -EOPNOTSUPP; - } - /* * KVM explicitly assumes that the guest has an FPU and * FXSAVE/FXRSTOR. For example, the KVM_GET_FPU explicitly casts the
Do basic VMX/SVM support checks directly in vendor code instead of implementing them via kvm_x86_ops hooks. Beyond the superficial benefit of providing common messages, which isn't even clearly a net positive since vendor code can provide more precise/detailed messages, there's zero advantage to bouncing through common x86 code. Consolidating the checks will also simplify performing the checks across all CPUs (in a future patch). Signed-off-by: Sean Christopherson <seanjc@google.com> --- arch/x86/include/asm/kvm_host.h | 2 -- arch/x86/kvm/svm/svm.c | 38 +++++++++++++++------------------ arch/x86/kvm/vmx/vmx.c | 37 +++++++++++++++++--------------- arch/x86/kvm/x86.c | 11 ---------- 4 files changed, 37 insertions(+), 51 deletions(-)