[36/44] KVM: x86: Do compatibility checks when onlining CPU
Commit Message
From: Chao Gao <chao.gao@intel.com>
Do compatibility checks when enabling hardware to effectively add
compatibility checks when onlining a CPU. Abort enabling, i.e. the
online process, if the (hotplugged) CPU is incompatible with the known
good setup.
At init time, KVM does compatibility checks to ensure that all online
CPUs support hardware virtualization and a common set of features. But
KVM uses hotplugged CPUs without such compatibility checks. On Intel
CPUs, this leads to #GP if the hotplugged CPU doesn't support VMX, or
VM-Entry failure if the hotplugged CPU doesn't support all features
enabled by KVM.
Note, this is little more than a NOP on SVM, as SVM already checks for
full SVM support during hardware enabling.
Opportunistically add a pr_err() if setup_vmcs_config() fails, and
tweak all error messages to output which CPU failed.
Signed-off-by: Chao Gao <chao.gao@intel.com>
Co-developed-by: Sean Christopherson <seanjc@google.com>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
arch/x86/include/asm/kvm_host.h | 2 +-
arch/x86/kvm/svm/svm.c | 20 +++++++++++---------
arch/x86/kvm/vmx/vmx.c | 33 +++++++++++++++++++--------------
arch/x86/kvm/x86.c | 5 +++--
4 files changed, 34 insertions(+), 26 deletions(-)
Comments
On 11/3/22 00:19, Sean Christopherson wrote:
> From: Chao Gao<chao.gao@intel.com>
>
> Do compatibility checks when enabling hardware to effectively add
> compatibility checks when onlining a CPU. Abort enabling, i.e. the
> online process, if the (hotplugged) CPU is incompatible with the known
> good setup.
This paragraph is not true with this patch being before "KVM: Rename and
move CPUHP_AP_KVM_STARTING to ONLINE section".
Paolo
On Thu, Nov 03, 2022, Paolo Bonzini wrote:
> On 11/3/22 00:19, Sean Christopherson wrote:
> > From: Chao Gao<chao.gao@intel.com>
> >
> > Do compatibility checks when enabling hardware to effectively add
> > compatibility checks when onlining a CPU. Abort enabling, i.e. the
> > online process, if the (hotplugged) CPU is incompatible with the known
> > good setup.
>
> This paragraph is not true with this patch being before "KVM: Rename and
> move CPUHP_AP_KVM_STARTING to ONLINE section".
Argh, good eyes. Getting the ordering correct in this series has been quite the
struggle. Assuming there are no subtle dependencies between x86 and common KVM,
the ordering should be something like this:
KVM: Opt out of generic hardware enabling on s390 and PPC
KVM: Register syscore (suspend/resume) ops early in kvm_init()
KVM: x86: Do compatibility checks when onlining CPU
KVM: SVM: Check for SVM support in CPU compatibility checks
KVM: VMX: Shuffle support checks and hardware enabling code around
KVM: x86: Do VMX/SVM support checks directly in vendor code
KVM: x86: Unify pr_fmt to use module name for all KVM modules
KVM: x86: Use KBUILD_MODNAME to specify vendor module name
KVM: Make hardware_enable_failed a local variable in the "enable all" path
KVM: Use a per-CPU variable to track which CPUs have enabled virtualization
KVM: Remove on_each_cpu(hardware_disable_nolock) in kvm_exit()
KVM: Drop kvm_count_lock and instead protect kvm_usage_count with kvm_lock
KVM: Disable CPU hotplug during hardware enabling
KVM: Rename and move CPUHP_AP_KVM_STARTING to ONLINE section
KVM: Drop kvm_arch_check_processor_compat() hook
On 11/3/22 18:44, Sean Christopherson wrote:
>>> Do compatibility checks when enabling hardware to effectively add
>>> compatibility checks when onlining a CPU. Abort enabling, i.e. the
>>> online process, if the (hotplugged) CPU is incompatible with the known
>>> good setup.
>>
>> This paragraph is not true with this patch being before "KVM: Rename and
>> move CPUHP_AP_KVM_STARTING to ONLINE section".
>
> Argh, good eyes. Getting the ordering correct in this series has been quite the
> struggle. Assuming there are no subtle dependencies between x86 and common KVM,
> the ordering should be something like this:
It's not a problem to keep the ordering in this v1, just fix the commit
message like "Do compatibility checks when enabling hardware to
effectively add compatibility checks on CPU hotplug. For now KVM is
using a STARTING hook, which makes it impossible to abort the hotplug if
the new CPU is incompatible with the known good setup; switching to an
ONLINE hook will fix this."
Paolo
> KVM: Opt out of generic hardware enabling on s390 and PPC
> KVM: Register syscore (suspend/resume) ops early in kvm_init()
> KVM: x86: Do compatibility checks when onlining CPU
> KVM: SVM: Check for SVM support in CPU compatibility checks
> KVM: VMX: Shuffle support checks and hardware enabling code around
> KVM: x86: Do VMX/SVM support checks directly in vendor code
> KVM: x86: Unify pr_fmt to use module name for all KVM modules
> KVM: x86: Use KBUILD_MODNAME to specify vendor module name
> KVM: Make hardware_enable_failed a local variable in the "enable all" path
> KVM: Use a per-CPU variable to track which CPUs have enabled virtualization
> KVM: Remove on_each_cpu(hardware_disable_nolock) in kvm_exit()
> KVM: Drop kvm_count_lock and instead protect kvm_usage_count with kvm_lock
> KVM: Disable CPU hotplug during hardware enabling
> KVM: Rename and move CPUHP_AP_KVM_STARTING to ONLINE section
> KVM: Drop kvm_arch_check_processor_compat() hook
>
On Wed, Nov 02, 2022 at 11:19:03PM +0000,
Sean Christopherson <seanjc@google.com> wrote:
> From: Chao Gao <chao.gao@intel.com>
>
> Do compatibility checks when enabling hardware to effectively add
> compatibility checks when onlining a CPU. Abort enabling, i.e. the
> online process, if the (hotplugged) CPU is incompatible with the known
> good setup.
>
> At init time, KVM does compatibility checks to ensure that all online
> CPUs support hardware virtualization and a common set of features. But
> KVM uses hotplugged CPUs without such compatibility checks. On Intel
> CPUs, this leads to #GP if the hotplugged CPU doesn't support VMX, or
> VM-Entry failure if the hotplugged CPU doesn't support all features
> enabled by KVM.
>
> Note, this is little more than a NOP on SVM, as SVM already checks for
> full SVM support during hardware enabling.
>
> Opportunistically add a pr_err() if setup_vmcs_config() fails, and
> tweak all error messages to output which CPU failed.
>
> Signed-off-by: Chao Gao <chao.gao@intel.com>
> Co-developed-by: Sean Christopherson <seanjc@google.com>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
> arch/x86/include/asm/kvm_host.h | 2 +-
> arch/x86/kvm/svm/svm.c | 20 +++++++++++---------
> arch/x86/kvm/vmx/vmx.c | 33 +++++++++++++++++++--------------
> arch/x86/kvm/x86.c | 5 +++--
> 4 files changed, 34 insertions(+), 26 deletions(-)
>
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index f223c845ed6e..c99222b71fcc 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -1666,7 +1666,7 @@ struct kvm_x86_nested_ops {
> };
>
> struct kvm_x86_init_ops {
> - int (*check_processor_compatibility)(void);
> + int (*check_processor_compatibility)(int cpu);
Is this cpu argument used only for error message to include cpu number
with avoiding repeating raw_smp_processor_id() in pr_err()?
The actual check is done on the current executing cpu.
If cpu != raw_smp_processor_id(), cpu is wrong. Although the function is called
in non-preemptive context, it's a bit confusing. So voting to remove it and
to use.
Thanks,
On Thu, Nov 03, 2022, Isaku Yamahata wrote:
> On Wed, Nov 02, 2022 at 11:19:03PM +0000,
> Sean Christopherson <seanjc@google.com> wrote:
> > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> > index f223c845ed6e..c99222b71fcc 100644
> > --- a/arch/x86/include/asm/kvm_host.h
> > +++ b/arch/x86/include/asm/kvm_host.h
> > @@ -1666,7 +1666,7 @@ struct kvm_x86_nested_ops {
> > };
> >
> > struct kvm_x86_init_ops {
> > - int (*check_processor_compatibility)(void);
> > + int (*check_processor_compatibility)(int cpu);
>
> Is this cpu argument used only for error message to include cpu number
> with avoiding repeating raw_smp_processor_id() in pr_err()?
Yep.
> The actual check is done on the current executing cpu.
>
> If cpu != raw_smp_processor_id(), cpu is wrong. Although the function is called
> in non-preemptive context, it's a bit confusing. So voting to remove it and
> to use.
What if I rename the param is this_cpu? I 100% agree the argument is confusing
as-is, but forcing all the helpers to manually grab the cpu is quite annoying.
On Thu, Nov 03, 2022 at 10:34:10PM +0000,
Sean Christopherson <seanjc@google.com> wrote:
> On Thu, Nov 03, 2022, Isaku Yamahata wrote:
> > On Wed, Nov 02, 2022 at 11:19:03PM +0000,
> > Sean Christopherson <seanjc@google.com> wrote:
> > > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> > > index f223c845ed6e..c99222b71fcc 100644
> > > --- a/arch/x86/include/asm/kvm_host.h
> > > +++ b/arch/x86/include/asm/kvm_host.h
> > > @@ -1666,7 +1666,7 @@ struct kvm_x86_nested_ops {
> > > };
> > >
> > > struct kvm_x86_init_ops {
> > > - int (*check_processor_compatibility)(void);
> > > + int (*check_processor_compatibility)(int cpu);
> >
> > Is this cpu argument used only for error message to include cpu number
> > with avoiding repeating raw_smp_processor_id() in pr_err()?
>
> Yep.
>
> > The actual check is done on the current executing cpu.
> >
> > If cpu != raw_smp_processor_id(), cpu is wrong. Although the function is called
> > in non-preemptive context, it's a bit confusing. So voting to remove it and
> > to use.
>
> What if I rename the param is this_cpu? I 100% agree the argument is confusing
> as-is, but forcing all the helpers to manually grab the cpu is quite annoying.
Makes sense. Let's settle it with this_cpu.
On Fri, Nov 04, 2022, Isaku Yamahata wrote:
> On Thu, Nov 03, 2022 at 10:34:10PM +0000,
> Sean Christopherson <seanjc@google.com> wrote:
>
> > On Thu, Nov 03, 2022, Isaku Yamahata wrote:
> > > On Wed, Nov 02, 2022 at 11:19:03PM +0000,
> > > Sean Christopherson <seanjc@google.com> wrote:
> > > > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> > > > index f223c845ed6e..c99222b71fcc 100644
> > > > --- a/arch/x86/include/asm/kvm_host.h
> > > > +++ b/arch/x86/include/asm/kvm_host.h
> > > > @@ -1666,7 +1666,7 @@ struct kvm_x86_nested_ops {
> > > > };
> > > >
> > > > struct kvm_x86_init_ops {
> > > > - int (*check_processor_compatibility)(void);
> > > > + int (*check_processor_compatibility)(int cpu);
> > >
> > > Is this cpu argument used only for error message to include cpu number
> > > with avoiding repeating raw_smp_processor_id() in pr_err()?
> >
> > Yep.
> >
> > > The actual check is done on the current executing cpu.
> > >
> > > If cpu != raw_smp_processor_id(), cpu is wrong. Although the function is called
> > > in non-preemptive context, it's a bit confusing. So voting to remove it and
> > > to use.
> >
> > What if I rename the param is this_cpu? I 100% agree the argument is confusing
> > as-is, but forcing all the helpers to manually grab the cpu is quite annoying.
>
> Makes sense. Let's settle it with this_cpu.
Finally got to actually change the code, and am not a fan of passing "this_cpu"
everywhere. It's not terrible, but it's not clearly better than just grabbing
the CPU on-demand. And while manually grabbing the CPU in the helpers is annoying,
in at least two cases the pain is just shifted to the caller.
I'm going with your original suggestion of just grabbing raw_smp_processor_id()
in the helpers that print the error message.
@@ -1666,7 +1666,7 @@ struct kvm_x86_nested_ops {
};
struct kvm_x86_init_ops {
- int (*check_processor_compatibility)(void);
+ int (*check_processor_compatibility)(int cpu);
int (*hardware_setup)(void);
unsigned int (*handle_intel_pt_intr)(void);
@@ -525,13 +525,13 @@ static void svm_init_osvw(struct kvm_vcpu *vcpu)
vcpu->arch.osvw.status |= 1;
}
-static bool kvm_is_svm_supported(void)
+static bool kvm_is_svm_supported(int cpu)
{
const char *msg;
u64 vm_cr;
if (!cpu_has_svm(&msg)) {
- pr_err("SVM not supported, %s\n", msg);
+ pr_err("SVM not supported by CPU %d, %s\n", cpu, msg);
return false;
}
@@ -542,16 +542,16 @@ static bool kvm_is_svm_supported(void)
rdmsrl(MSR_VM_CR, vm_cr);
if (vm_cr & (1 << SVM_VM_CR_SVM_DISABLE)) {
- pr_err("SVM disabled in MSR_VM_CR\n");
+ pr_err("SVM disabled in MSR_VM_CR on CPU %d\n", cpu);
return false;
}
return true;
}
-static int __init svm_check_processor_compat(void)
+static int svm_check_processor_compat(int cpu)
{
- if (!kvm_is_svm_supported())
+ if (!kvm_is_svm_supported(cpu))
return -EIO;
return 0;
@@ -588,14 +588,16 @@ static int svm_hardware_enable(void)
uint64_t efer;
struct desc_struct *gdt;
int me = raw_smp_processor_id();
+ int r;
+
+ r = svm_check_processor_compat(me);
+ if (r)
+ return r;
rdmsrl(MSR_EFER, efer);
if (efer & EFER_SVME)
return -EBUSY;
- 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);
@@ -5132,7 +5134,7 @@ static int __init svm_init(void)
__unused_size_checks();
- if (!kvm_is_svm_supported())
+ if (!kvm_is_svm_supported(raw_smp_processor_id()))
return -EOPNOTSUPP;
r = kvm_x86_vendor_init(&svm_init_ops);
@@ -2520,8 +2520,7 @@ static bool cpu_has_perf_global_ctrl_bug(void)
return false;
}
-static __init int adjust_vmx_controls(u32 ctl_min, u32 ctl_opt,
- u32 msr, u32 *result)
+static int adjust_vmx_controls(u32 ctl_min, u32 ctl_opt, u32 msr, u32 *result)
{
u32 vmx_msr_low, vmx_msr_high;
u32 ctl = ctl_min | ctl_opt;
@@ -2539,7 +2538,7 @@ static __init int adjust_vmx_controls(u32 ctl_min, u32 ctl_opt,
return 0;
}
-static __init u64 adjust_vmx_controls64(u64 ctl_opt, u32 msr)
+static u64 adjust_vmx_controls64(u64 ctl_opt, u32 msr)
{
u64 allowed;
@@ -2548,8 +2547,8 @@ static __init u64 adjust_vmx_controls64(u64 ctl_opt, u32 msr)
return ctl_opt & allowed;
}
-static __init int setup_vmcs_config(struct vmcs_config *vmcs_conf,
- struct vmx_capability *vmx_cap)
+static int setup_vmcs_config(struct vmcs_config *vmcs_conf,
+ struct vmx_capability *vmx_cap)
{
u32 vmx_msr_low, vmx_msr_high;
u32 _pin_based_exec_control = 0;
@@ -2710,36 +2709,38 @@ static __init int setup_vmcs_config(struct vmcs_config *vmcs_conf,
return 0;
}
-static bool __init kvm_is_vmx_supported(void)
+static bool kvm_is_vmx_supported(int cpu)
{
if (!cpu_has_vmx()) {
- pr_err("CPU doesn't support VMX\n");
+ pr_err("VMX not supported by CPU %d\n", cpu);
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");
+ pr_err("VMX not enabled in MSR_IA32_FEAT_CTL on CPU %d\n", cpu);
return false;
}
return true;
}
-static int __init vmx_check_processor_compat(void)
+static int vmx_check_processor_compat(int cpu)
{
struct vmcs_config vmcs_conf;
struct vmx_capability vmx_cap;
- if (!kvm_is_vmx_supported())
+ if (!kvm_is_vmx_supported(cpu))
return -EIO;
- if (setup_vmcs_config(&vmcs_conf, &vmx_cap) < 0)
+ if (setup_vmcs_config(&vmcs_conf, &vmx_cap) < 0) {
+ pr_err("Failed to setup VMCS config on CPU %d\n", cpu);
return -EIO;
+ }
if (nested)
nested_vmx_setup_ctls_msrs(&vmcs_conf, vmx_cap.ept);
- if (memcmp(&vmcs_config, &vmcs_conf, sizeof(struct vmcs_config)) != 0) {
- pr_err("CPU %d feature inconsistency!\n", smp_processor_id());
+ if (memcmp(&vmcs_config, &vmcs_conf, sizeof(struct vmcs_config))) {
+ pr_err("Inconsistent VMCS config on CPU %d\n", cpu);
return -EIO;
}
return 0;
@@ -2771,6 +2772,10 @@ static int vmx_hardware_enable(void)
u64 phys_addr = __pa(per_cpu(vmxarea, cpu));
int r;
+ r = vmx_check_processor_compat(cpu);
+ if (r)
+ return r;
+
if (cr4_read_shadow() & X86_CR4_VMXE)
return -EBUSY;
@@ -8517,7 +8522,7 @@ static int __init vmx_init(void)
{
int r, cpu;
- if (!kvm_is_vmx_supported())
+ if (!kvm_is_vmx_supported(raw_smp_processor_id()))
return -EOPNOTSUPP;
hv_setup_evmcs();
@@ -9280,7 +9280,8 @@ struct kvm_cpu_compat_check {
static int kvm_x86_check_processor_compatibility(struct kvm_x86_init_ops *ops)
{
- struct cpuinfo_x86 *c = &cpu_data(smp_processor_id());
+ int cpu = smp_processor_id();
+ struct cpuinfo_x86 *c = &cpu_data(cpu);
WARN_ON(!irqs_disabled());
@@ -9288,7 +9289,7 @@ static int kvm_x86_check_processor_compatibility(struct kvm_x86_init_ops *ops)
__cr4_reserved_bits(cpu_has, &boot_cpu_data))
return -EIO;
- return ops->check_processor_compatibility();
+ return ops->check_processor_compatibility(cpu);
}
static void kvm_x86_check_cpu_compat(void *data)