Re: [PATCH v2 40/50] KVM: x86: Do compatibility checks when onlining CPU
On Wed, 2022-11-30 at 23:09 +, Sean Christopherson wrote: > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -11967,6 +11967,11 @@ int kvm_arch_hardware_enable(void) > bool stable, backwards_tsc = false; > > kvm_user_return_msr_cpu_online(); > + > + ret = kvm_x86_check_processor_compatibility(); > + if (ret) > + return ret; > + > ret = static_call(kvm_x86_hardware_enable)(); > if (ret != 0) > return ret; Thinking more, AFAICT, kvm_x86_vendor_init() so far still does the compatibility check on all online cpus. Since now kvm_arch_hardware_enable() also does the compatibility check, IIUC the compatibility check will be done twice -- one in kvm_x86_vendor_init() and one in hardware_enable_all() when creating the first VM. Do you think it's still worth to do compatibility check in vm_x86_vendor_init()? The behaviour difference should be "KVM module fail to load" vs "failing to create the first VM" IIUC. I don't know whether the former is better than the better, but it seems duplicated compatibility checking isn't needed? ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v2 41/50] KVM: Rename and move CPUHP_AP_KVM_STARTING to ONLINE section
On Wed, 2022-11-30 at 23:09 +, Sean Christopherson wrote: > From: Chao Gao > ... > > Suggested-by: Thomas Gleixner > Signed-off-by: Chao Gao > Signed-off-by: Isaku Yamahata Perhaps I am wrong, but I have memory that if someone has SoB but isn't the original author should also have a Co-developed-by? > Reviewed-by: Yuan Yao > [sean: drop WARN that IRQs are disabled] > Signed-off-by: Sean Christopherson ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v2 40/50] KVM: x86: Do compatibility checks when onlining CPU
On Wed, 2022-11-30 at 23:09 +, Sean Christopherson wrote:
> From: Chao Gao
>
> 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
> Co-developed-by: Sean Christopherson
> Signed-off-by: Sean Christopherson
For VMX:
Acked-by: Kai Huang
> ---
> arch/x86/kvm/svm/svm.c | 8 +++-
> arch/x86/kvm/vmx/vmx.c | 15 ++-
> arch/x86/kvm/x86.c | 5 +
> 3 files changed, 18 insertions(+), 10 deletions(-)
>
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index c2e95c0d9fd8..46b658d0f46e 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -521,11 +521,12 @@ static void svm_init_osvw(struct kvm_vcpu *vcpu)
>
> static bool kvm_is_svm_supported(void)
> {
> + int cpu = raw_smp_processor_id();
> 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;
> }
>
> @@ -536,7 +537,7 @@ 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 (by BIOS) in MSR_VM_CR\n");
> + pr_err("SVM disabled (by BIOS) in MSR_VM_CR on CPU %d\n", cpu);
> return false;
> }
>
> @@ -587,9 +588,6 @@ static int svm_hardware_enable(void)
> if (efer & EFER_SVME)
> return -EBUSY;
>
> - if (!kvm_is_svm_supported())
> - return -EINVAL;
> -
> sd = per_cpu_ptr(&svm_data, me);
> sd->asid_generation = 1;
> sd->max_asid = cpuid_ebx(SVM_CPUID_FUNC) - 1;
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 6416ed5b7f89..39dd3082fcd8 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -2711,14 +2711,16 @@ static int setup_vmcs_config(struct vmcs_config
> *vmcs_conf,
>
> static bool kvm_is_vmx_supported(void)
> {
> + int cpu = raw_smp_processor_id();
> +
> 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 (!this_cpu_has(X86_FEATURE_MSR_IA32_FEAT_CTL) ||
> !this_cpu_has(X86_FEATURE_VMX)) {
> - pr_err("VMX not enabled (by BIOS) in MSR_IA32_FEAT_CTL\n");
> + pr_err("VMX not enabled (by BIOS) in MSR_IA32_FEAT_CTL on CPU
> %d\n", cpu);
> return false;
> }
>
> @@ -2727,18 +2729,21 @@ static bool kvm_is_vmx_supported(void)
>
> static int vmx_check_processor_compat(void)
> {
> + int cpu = raw_smp_processor_id();
> struct vmcs_config vmcs_conf;
> struct vmx_capability vmx_cap;
>
> if (!kvm_is_vmx_supported())
> 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;
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index ee9af412ffd4..5a9e74cedbc6 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -11967,6 +11967,11 @@ int kvm_arch_hardware_enable(void)
> bool stable, backwards_tsc = false;
>
> kvm_user_return_msr_cpu_online();
> +
> + ret = kvm_x86_check_processor_compatibility();
> + if (ret)
> + return ret;
> +
> ret = static_call(kvm_x86_hardware_enable)();
> if (ret != 0)
> return ret;
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.c
Re: [PATCH v2 39/50] KVM: x86: Move CPU compat checks hook to kvm_x86_ops (from kvm_x86_init_ops)
On Wed, 2022-11-30 at 23:09 +, Sean Christopherson wrote:
> Move the .check_processor_compatibility() callback from kvm_x86_init_ops
> to kvm_x86_ops to allow a future patch to do compatibility checks during
> CPU hotplug.
>
> Do kvm_ops_update() before compat checks so that static_call() can be
> used during compat checks.
>
> Signed-off-by: Sean Christopherson
Reviewed-by: Kai Huang
> ---
> arch/x86/include/asm/kvm-x86-ops.h | 1 +
> arch/x86/include/asm/kvm_host.h| 3 ++-
> arch/x86/kvm/svm/svm.c | 5 +++--
> arch/x86/kvm/vmx/vmx.c | 16 +++
> arch/x86/kvm/x86.c | 31 +++---
> 5 files changed, 25 insertions(+), 31 deletions(-)
>
> diff --git a/arch/x86/include/asm/kvm-x86-ops.h
> b/arch/x86/include/asm/kvm-x86-ops.h
> index abccd51dcfca..dba2909e5ae2 100644
> --- a/arch/x86/include/asm/kvm-x86-ops.h
> +++ b/arch/x86/include/asm/kvm-x86-ops.h
> @@ -14,6 +14,7 @@ BUILD_BUG_ON(1)
> * to make a definition optional, but in this case the default will
> * be __static_call_return0.
> */
> +KVM_X86_OP(check_processor_compatibility)
> KVM_X86_OP(hardware_enable)
> KVM_X86_OP(hardware_disable)
> KVM_X86_OP(hardware_unsetup)
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index d79aedf70908..ba74fea6850b 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -1518,6 +1518,8 @@ static inline u16 kvm_lapic_irq_dest_mode(bool
> dest_mode_logical)
> struct kvm_x86_ops {
> const char *name;
>
> + int (*check_processor_compatibility)(void);
> +
> int (*hardware_enable)(void);
> void (*hardware_disable)(void);
> void (*hardware_unsetup)(void);
> @@ -1729,7 +1731,6 @@ struct kvm_x86_nested_ops {
> };
>
> struct kvm_x86_init_ops {
> - 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 9f94efcb9aa6..c2e95c0d9fd8 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -543,7 +543,7 @@ static bool kvm_is_svm_supported(void)
> return true;
> }
>
> -static int __init svm_check_processor_compat(void)
> +static int svm_check_processor_compat(void)
> {
> if (!kvm_is_svm_supported())
> return -EIO;
> @@ -4695,6 +4695,8 @@ static int svm_vm_init(struct kvm *kvm)
> static struct kvm_x86_ops svm_x86_ops __initdata = {
> .name = KBUILD_MODNAME,
>
> + .check_processor_compatibility = svm_check_processor_compat,
> +
> .hardware_unsetup = svm_hardware_unsetup,
> .hardware_enable = svm_hardware_enable,
> .hardware_disable = svm_hardware_disable,
> @@ -5079,7 +5081,6 @@ static __init int svm_hardware_setup(void)
>
> static struct kvm_x86_init_ops svm_init_ops __initdata = {
> .hardware_setup = svm_hardware_setup,
> - .check_processor_compatibility = svm_check_processor_compat,
>
> .runtime_ops = &svm_x86_ops,
> .pmu_ops = &amd_pmu_ops,
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 2a8a6e481c76..6416ed5b7f89 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -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,7 +2709,7 @@ 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(void)
> {
> if (!cpu_has_vmx()) {
> pr_err("CPU doesn't support VMX\n");
> @@ -2726,7 +2725,7 @@ static bool __init kvm_is_vmx_supported(void)
> return true;
> }
>
> -static int __init vmx_check_processor_compat(void)
> +static int vmx_check_processor_compat(void)
> {
> struct vmcs_config vmcs_conf;
> struct
Re: [PATCH v2 42/50] KVM: Disable CPU hotplug during hardware enabling/disabling
On Wed, 2022-11-30 at 23:09 +, Sean Christopherson wrote:
> From: Chao Gao
>
> Disable CPU hotplug when enabling/disabling hardware to prevent the
> corner case where if the following sequence occurs:
>
> 1. A hotplugged CPU marks itself online in cpu_online_mask
> 2. The hotplugged CPU enables interrupt before invoking KVM's ONLINE
> callback
> 3 hardware_{en,dis}able_all() is invoked on another CPU
>
> the hotplugged CPU will be included in on_each_cpu() and thus get sent
> through hardware_{en,dis}able_nolock() before kvm_online_cpu() is called.
Should we explicitly call out what is the consequence of such case, otherwise
it's hard to tell whether this truly is an issue?
IIUC, since now the compatibility check has already been moved to
kvm_arch_hardware_enable(), the consequence is hardware_enable_all() will fail
if the now online cpu isn't compatible, which will results in failing to create
the first VM. This isn't ideal since the incompatible cpu should be rejected to
go online instead.
>
> start_secondary { ...
> set_cpu_online(smp_processor_id(), true); <- 1
> ...
> local_irq_enable(); <- 2
> ...
> cpu_startup_entry(CPUHP_AP_ONLINE_IDLE); <- 3
> }
>
> KVM currently fudges around this race by keeping track of which CPUs have
> done hardware enabling (see commit 1b6c016818a5 "KVM: Keep track of which
> cpus have virtualization enabled"), but that's an inefficient, convoluted,
> and hacky solution.
>
> Signed-off-by: Chao Gao
> [sean: split to separate patch, write changelog]
> Signed-off-by: Sean Christopherson
> ---
> arch/x86/kvm/x86.c | 11 ++-
> virt/kvm/kvm_main.c | 12
> 2 files changed, 22 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index dad30097f0c3..d2ad383da998 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -9281,7 +9281,16 @@ static inline void kvm_ops_update(struct
> kvm_x86_init_ops *ops)
>
> static int kvm_x86_check_processor_compatibility(void)
> {
> - struct cpuinfo_x86 *c = &cpu_data(smp_processor_id());
> + int cpu = smp_processor_id();
> + struct cpuinfo_x86 *c = &cpu_data(cpu);
> +
> + /*
> + * Compatibility checks are done when loading KVM and when enabling
> + * hardware, e.g. during CPU hotplug, to ensure all online CPUs are
> + * compatible, i.e. KVM should never perform a compatibility check on
> + * an offline CPU.
> + */
> + WARN_ON(!cpu_online(cpu));
IMHO this chunk logically should belong to previous patch. IIUC disabling CPU
hotplug during hardware_enable_all() doesn't have relationship to this WARN().
>
> if (__cr4_reserved_bits(cpu_has, c) !=
> __cr4_reserved_bits(cpu_has, &boot_cpu_data))
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index f26ea779710a..d985b24c423b 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -5098,15 +5098,26 @@ static void hardware_disable_all_nolock(void)
>
> static void hardware_disable_all(void)
> {
> + cpus_read_lock();
> raw_spin_lock(&kvm_count_lock);
> hardware_disable_all_nolock();
> raw_spin_unlock(&kvm_count_lock);
> + cpus_read_unlock();
> }
>
> static int hardware_enable_all(void)
> {
> int r = 0;
>
> + /*
> + * When onlining a CPU, cpu_online_mask is set before kvm_online_cpu()
> + * is called, and so on_each_cpu() between them includes the CPU that
> + * is being onlined. As a result, hardware_enable_nolock() may get
> + * invoked before kvm_online_cpu(), which also enables hardware if the
> + * usage count is non-zero. Disable CPU hotplug to avoid attempting to
> + * enable hardware multiple times.
It won't enable hardware multiple times, right? Since hardware_enable_nolock()
has below check:
if (cpumask_test_cpu(cpu, cpus_hardware_enabled))
return;
cpumask_set_cpu(cpu, cpus_hardware_enabled);
IIUC the only issue is the one that I replied in the changelog.
Or perhaps I am missing something?
> + */
> + cpus_read_lock();
> raw_spin_lock(&kvm_count_lock);
>
> kvm_usage_count++;
> @@ -5121,6 +5132,7 @@ static int hardware_enable_all(void)
> }
>
> raw_spin_unlock(&kvm_count_lock);
> + cpus_read_unlock();
>
> return r;
> }
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v2 35/50] KVM: VMX: Use current CPU's info to perform "disabled by BIOS?" checks
On Wed, 2022-11-30 at 23:09 +, Sean Christopherson wrote:
> Use this_cpu_has() instead of boot_cpu_has() to perform the effective
> "disabled by BIOS?" checks for VMX. This will allow consolidating code
> between vmx_disabled_by_bios() and vmx_check_processor_compat().
>
> Checking the boot CPU isn't a strict requirement as any divergence in VMX
> enabling between the boot CPU and other CPUs will result in KVM refusing
> to load thanks to the aforementioned vmx_check_processor_compat().
>
> Furthermore, using the boot CPU was an unintentional change introduced by
> commit a4d0b2fdbcf7 ("KVM: VMX: Use VMX feature flag to query BIOS
> enabling"). Prior to using the feature flags, KVM checked the raw MSR
> value from the current CPU.
>
> Reported-by: Kai Huang
> Signed-off-by: Sean Christopherson
Reviewed-by: Kai Huang
> ---
> arch/x86/kvm/vmx/vmx.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index e859d2b7daa4..3f7d9f88b314 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -2492,8 +2492,8 @@ 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 !this_cpu_has(X86_FEATURE_MSR_IA32_FEAT_CTL) ||
> +!this_cpu_has(X86_FEATURE_VMX);
> }
>
> static int kvm_cpu_vmxon(u64 vmxon_pointer)
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v2 32/50] KVM: Drop kvm_arch_check_processor_compat() hook
On Wed, 2022-11-30 at 23:09 +, Sean Christopherson wrote:
> Drop kvm_arch_check_processor_compat() and its support code now that all
> architecture implementations are nops.
>
> Signed-off-by: Sean Christopherson
> Reviewed-by: Philippe Mathieu-Daudé
> Reviewed-by: Eric Farman# s390
> Acked-by: Anup Patel
For x86,
Reviewed-by: Kai Huang
> ---
> arch/arm64/kvm/arm.c | 7 +--
> arch/mips/kvm/mips.c | 7 +--
> arch/powerpc/kvm/book3s.c | 2 +-
> arch/powerpc/kvm/e500.c| 2 +-
> arch/powerpc/kvm/e500mc.c | 2 +-
> arch/powerpc/kvm/powerpc.c | 5 -
> arch/riscv/kvm/main.c | 7 +--
> arch/s390/kvm/kvm-s390.c | 7 +--
> arch/x86/kvm/svm/svm.c | 4 ++--
> arch/x86/kvm/vmx/vmx.c | 4 ++--
> arch/x86/kvm/x86.c | 5 -
> include/linux/kvm_host.h | 4 +---
> virt/kvm/kvm_main.c| 24 +---
> 13 files changed, 13 insertions(+), 67 deletions(-)
>
> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> index 936ef7d1ea94..e915b1d9f2cd 100644
> --- a/arch/arm64/kvm/arm.c
> +++ b/arch/arm64/kvm/arm.c
> @@ -63,11 +63,6 @@ int kvm_arch_vcpu_should_kick(struct kvm_vcpu *vcpu)
> return kvm_vcpu_exiting_guest_mode(vcpu) == IN_GUEST_MODE;
> }
>
> -int kvm_arch_check_processor_compat(void *opaque)
> -{
> - return 0;
> -}
> -
> int kvm_vm_ioctl_enable_cap(struct kvm *kvm,
> struct kvm_enable_cap *cap)
> {
> @@ -2273,7 +2268,7 @@ static __init int kvm_arm_init(void)
>* FIXME: Do something reasonable if kvm_init() fails after pKVM
>* hypervisor protection is finalized.
>*/
> - err = kvm_init(NULL, sizeof(struct kvm_vcpu), 0, THIS_MODULE);
> + err = kvm_init(sizeof(struct kvm_vcpu), 0, THIS_MODULE);
> if (err)
> goto out_subs;
>
> diff --git a/arch/mips/kvm/mips.c b/arch/mips/kvm/mips.c
> index 3cade648827a..36c8991b5d39 100644
> --- a/arch/mips/kvm/mips.c
> +++ b/arch/mips/kvm/mips.c
> @@ -135,11 +135,6 @@ void kvm_arch_hardware_disable(void)
> kvm_mips_callbacks->hardware_disable();
> }
>
> -int kvm_arch_check_processor_compat(void *opaque)
> -{
> - return 0;
> -}
> -
> extern void kvm_init_loongson_ipi(struct kvm *kvm);
>
> int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
> @@ -1636,7 +1631,7 @@ static int __init kvm_mips_init(void)
>
> register_die_notifier(&kvm_mips_csr_die_notifier);
>
> - ret = kvm_init(NULL, sizeof(struct kvm_vcpu), 0, THIS_MODULE);
> + ret = kvm_init(sizeof(struct kvm_vcpu), 0, THIS_MODULE);
> if (ret) {
> unregister_die_notifier(&kvm_mips_csr_die_notifier);
> return ret;
> diff --git a/arch/powerpc/kvm/book3s.c b/arch/powerpc/kvm/book3s.c
> index 87283a0e33d8..57f4e7896d67 100644
> --- a/arch/powerpc/kvm/book3s.c
> +++ b/arch/powerpc/kvm/book3s.c
> @@ -1052,7 +1052,7 @@ static int kvmppc_book3s_init(void)
> {
> int r;
>
> - r = kvm_init(NULL, sizeof(struct kvm_vcpu), 0, THIS_MODULE);
> + r = kvm_init(sizeof(struct kvm_vcpu), 0, THIS_MODULE);
> if (r)
> return r;
> #ifdef CONFIG_KVM_BOOK3S_32_HANDLER
> diff --git a/arch/powerpc/kvm/e500.c b/arch/powerpc/kvm/e500.c
> index 0ea61190ec04..b0f695428733 100644
> --- a/arch/powerpc/kvm/e500.c
> +++ b/arch/powerpc/kvm/e500.c
> @@ -531,7 +531,7 @@ static int __init kvmppc_e500_init(void)
> flush_icache_range(kvmppc_booke_handlers, kvmppc_booke_handlers +
> ivor[max_ivor] + handler_len);
>
> - r = kvm_init(NULL, sizeof(struct kvmppc_vcpu_e500), 0, THIS_MODULE);
> + r = kvm_init(sizeof(struct kvmppc_vcpu_e500), 0, THIS_MODULE);
> if (r)
> goto err_out;
> kvm_ops_e500.owner = THIS_MODULE;
> diff --git a/arch/powerpc/kvm/e500mc.c b/arch/powerpc/kvm/e500mc.c
> index 795667f7ebf0..611532a0dedc 100644
> --- a/arch/powerpc/kvm/e500mc.c
> +++ b/arch/powerpc/kvm/e500mc.c
> @@ -404,7 +404,7 @@ static int __init kvmppc_e500mc_init(void)
>*/
> kvmppc_init_lpid(KVMPPC_NR_LPIDS/threads_per_core);
>
> - r = kvm_init(NULL, sizeof(struct kvmppc_vcpu_e500), 0, THIS_MODULE);
> + r = kvm_init(sizeof(struct kvmppc_vcpu_e500), 0, THIS_MODULE);
> if (r)
> goto err_out;
> kvm_ops_e500mc.owner = THIS_MODULE;
> diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
> index 01d0f9935e6c..f5b4ff6bfc89 100644
> --- a/arch/powerpc/kvm/powerpc.c
> +++ b/arch/powerpc/kvm/powerpc.c
> @@ -440,11 +440,6 @@ int kvm_arch_hardware_enable(void)
> return 0;
> }
>
> -int kvm_arch_check_processor_compat(void *opaque)
> -{
> - return 0;
> -}
> -
> int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
> {
> struct kvmppc_ops *kvm_ops = NULL;
> diff --git a/arch/riscv/kvm/main.c b/arch/riscv/kvm/main.c
> index 4710a6751687..34c3dece6990 100644
> --- a/arch/riscv/kvm/main.c
> +++ b/arch/riscv/kvm/main.c
> @@ -20,11 +20,6 @@
Re: [PATCH v2 31/50] KVM: x86: Do CPU compatibility checks in x86 code
On Wed, 2022-11-30 at 23:09 +, Sean Christopherson wrote:
> Move the CPU compatibility checks to pure x86 code, i.e. drop x86's use
> of the common kvm_x86_check_cpu_compat() arch hook. x86 is the only
^
kvm_arch_check_processor_compat()
> architecture that "needs" to do per-CPU compatibility checks, moving
> the logic to x86 will allow dropping the common code, and will also
> give x86 more control over when/how the compatibility checks are
> performed, e.g. TDX will need to enable hardware (do VMXON) in order to
> perform compatibility checks.
>
> Signed-off-by: Sean Christopherson
Reviewed-by: Kai Huang
> ---
> arch/x86/kvm/svm/svm.c | 2 +-
> arch/x86/kvm/vmx/vmx.c | 2 +-
> arch/x86/kvm/x86.c | 49 --
> 3 files changed, 40 insertions(+), 13 deletions(-)
>
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index 19e81a99c58f..d7ea1c1175c2 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -5103,7 +5103,7 @@ static int __init svm_init(void)
>* Common KVM initialization _must_ come last, after this, /dev/kvm is
>* exposed to userspace!
>*/
> - r = kvm_init(&svm_init_ops, sizeof(struct vcpu_svm),
> + r = kvm_init(NULL, sizeof(struct vcpu_svm),
>__alignof__(struct vcpu_svm), THIS_MODULE);
> if (r)
> goto err_kvm_init;
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 654d81f781da..8deb1bd60c10 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -8592,7 +8592,7 @@ static int __init vmx_init(void)
>* Common KVM initialization _must_ come last, after this, /dev/kvm is
>* exposed to userspace!
>*/
> - r = kvm_init(&vmx_init_ops, sizeof(struct vcpu_vmx),
> + r = kvm_init(NULL, sizeof(struct vcpu_vmx),
>__alignof__(struct vcpu_vmx), THIS_MODULE);
> if (r)
> goto err_kvm_init;
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 66f16458aa97..3571bc968cf8 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -9277,10 +9277,36 @@ static inline void kvm_ops_update(struct
> kvm_x86_init_ops *ops)
> kvm_pmu_ops_update(ops->pmu_ops);
> }
>
> +struct kvm_cpu_compat_check {
> + struct kvm_x86_init_ops *ops;
> + int *ret;
> +};
> +
> +static int kvm_x86_check_processor_compatibility(struct kvm_x86_init_ops
> *ops)
> +{
> + struct cpuinfo_x86 *c = &cpu_data(smp_processor_id());
> +
> + WARN_ON(!irqs_disabled());
> +
> + if (__cr4_reserved_bits(cpu_has, c) !=
> + __cr4_reserved_bits(cpu_has, &boot_cpu_data))
> + return -EIO;
> +
> + return ops->check_processor_compatibility();
> +}
> +
> +static void kvm_x86_check_cpu_compat(void *data)
> +{
> + struct kvm_cpu_compat_check *c = data;
> +
> + *c->ret = kvm_x86_check_processor_compatibility(c->ops);
> +}
> +
> static int __kvm_x86_vendor_init(struct kvm_x86_init_ops *ops)
> {
> + struct kvm_cpu_compat_check c;
> u64 host_pat;
> - int r;
> + int r, cpu;
>
> if (kvm_x86_ops.hardware_enable) {
> pr_err("kvm: already loaded vendor module '%s'\n",
> kvm_x86_ops.name);
> @@ -9360,6 +9386,14 @@ static int __kvm_x86_vendor_init(struct
> kvm_x86_init_ops *ops)
> if (r != 0)
> goto out_mmu_exit;
>
> + c.ret = &r;
> + c.ops = ops;
> + for_each_online_cpu(cpu) {
> + smp_call_function_single(cpu, kvm_x86_check_cpu_compat, &c, 1);
> + if (r < 0)
> + goto out_hardware_unsetup;
> + }
> +
> /*
>* Point of no return! DO NOT add error paths below this point unless
>* absolutely necessary, as most operations from this point forward
> @@ -9402,6 +9436,8 @@ static int __kvm_x86_vendor_init(struct
> kvm_x86_init_ops *ops)
> kvm_init_msr_list();
> return 0;
>
> +out_hardware_unsetup:
> + ops->runtime_ops->hardware_unsetup();
> out_mmu_exit:
> kvm_mmu_vendor_module_exit();
> out_free_percpu:
> @@ -12037,16 +12073,7 @@ void kvm_arch_hardware_disable(void)
>
> int kvm_arch_check_processor_compat(void *opaque)
> {
> - struct cpuinfo_x86 *c = &cpu_data(smp_processor_id());
> - struct kvm_x86_init_ops *ops = opaque;
> -
> - WARN_ON(!irqs_disabled());
> -
> - if (__cr4_reserved_bits(cpu_has, c) !=
> - __cr4_reserved_bits(cpu_has, &boot_cpu_data))
> - return -EIO;
> -
> - return ops->check_processor_compatibility();
> + return 0;
> }
>
> bool kvm_vcpu_is_reset_bsp(struct kvm_vcpu *vcpu)
> --
> 2.38.1.584.g0f3c55d4c2-goog
>
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH 38/44] KVM: Disable CPU hotplug during hardware enabling
On Wed, 2022-11-16 at 17:11 +, Sean Christopherson wrote:
> On Wed, Nov 16, 2022, Huang, Kai wrote:
> > On Tue, 2022-11-15 at 20:16 +, Sean Christopherson wrote:
> > > On Thu, Nov 10, 2022, Huang, Kai wrote:
> > > > On Thu, 2022-11-10 at 01:33 +, Huang, Kai wrote:
> > > > Hmm.. I wasn't thinking thoroughly. I forgot CPU compatibility check
> > > > also
> > > > happens on all online cpus when loading KVM. For this case, IRQ is
> > > > disabled and
> > > > cpu_active() is true. For the hotplug case, IRQ is enabled but
> > > > cpu_active() is
> > > > false.
> > >
> > > Actually, you're right (and wrong). You're right in that the WARN is
> > > flawed. And
> > > the reason for that is because you're wrong about the hotplug case. In
> > > this version
> > > of things, the compatibility checks are routed through hardware enabling,
> > > i.e. this
> > > flow is used only when loading KVM. This helper should only be called
> > > via SMP function
> > > call, which means that IRQs should always be disabled.
> >
> > Did you mean below code change in later patch "[PATCH 39/44] KVM: Drop
> > kvm_count_lock and instead protect kvm_usage_count with kvm_lock"?
> >
> > /*
> > * Abort the CPU online process if hardware virtualization cannot
> > * be enabled. Otherwise running VMs would encounter unrecoverable
> > @@ -5039,13 +5039,16 @@ static int kvm_online_cpu(unsigned int cpu)
> > if (kvm_usage_count) {
> > WARN_ON_ONCE(atomic_read(&hardware_enable_failed));
> >
> > + local_irq_save(flags);
> > hardware_enable_nolock(NULL);
> > + local_irq_restore(flags);
>
> Sort of. What I was saying is that in this v1, the compatibility checks that
> are
> done during harware enabling are initiated from vendor code, i.e. VMX and SVM
> call
> {svm,vmx}_check_processor_compat() directly. As a result, the compat checks
> that
> are handled in common code:
>
> if (__cr4_reserved_bits(cpu_has, c) !=
> __cr4_reserved_bits(cpu_has, &boot_cpu_data))
> return -EIO;
>
> are skipped. And if that's fixed, then the above hardware_enable_nolock()
> call
> will bounce through kvm_x86_check_processor_compatibility() with IRQs enabled
> once the KVM hotplug hook is moved to the ONLINE section.
Oh I see. So you still want the kvm_x86_ops->check_processor_compatibility(),
in order to avoid duplicating the above code in SVM and VMX.
>
> As above, the simple "fix" would be to disable IRQs, but that's not actually
> necessary. The only requirement is that preemption is disabled so that the
> checks
> are done on the current CPU.
>
Probably even preemption is allowed, as long as the compatibility check is not
scheduled to another cpu.
> The "IRQs disabled" check was a deliberately
> agressive WARN that was added to guard against doing compatibility checks from
> the "wrong" location.
>
> E.g. this is what I ended up with for a changelog to drop the irqs_disabled()
> check and for the end code (though it's not tested yet...)
>
> Drop kvm_x86_check_processor_compatibility()'s WARN that IRQs are
> disabled, as the ONLINE section runs with IRQs disabled. The WARN wasn't
^
enabled.
> intended to be a requirement, e.g. disabling preemption is sufficient,
> the IRQ thing was purely an aggressive sanity check since the helper was
> only ever invoked via SMP function call.
>
>
> static int kvm_x86_check_processor_compatibility(void)
> {
> int cpu = smp_processor_id();
> struct cpuinfo_x86 *c = &cpu_data(cpu);
>
> /*
> * Compatibility checks are done when loading KVM and when enabling
> * hardware, e.g. during CPU hotplug, to ensure all online CPUs are
> * compatible, i.e. KVM should never perform a compatibility check on
> * an offline CPU.
> */
> WARN_ON(!cpu_online(cpu));
Looks good to me. Perhaps this also can be removed, though.
And IMHO the removing of WARN_ON(!irq_disabled()) should be folded to the patch
"[PATCH 37/44] KVM: Rename and move CPUHP_AP_KVM_STARTING to ONLINE section".
Because moving from STARTING section to ONLINE section changes the IRQ status
when the compatibility check is called.
>
> if (__cr4_reserved_bits(cpu_has,
Re: [PATCH 38/44] KVM: Disable CPU hotplug during hardware enabling
On Tue, 2022-11-15 at 20:16 +, Sean Christopherson wrote:
> On Thu, Nov 10, 2022, Huang, Kai wrote:
> > On Thu, 2022-11-10 at 01:33 +0000, Huang, Kai wrote:
> > > > @@ -9283,7 +9283,13 @@ static int
> > > > kvm_x86_check_processor_compatibility(struct kvm_x86_init_ops *ops)
> > > > int cpu = smp_processor_id();
> > > > struct cpuinfo_x86 *c = &cpu_data(cpu);
> > > >
> > > > - WARN_ON(!irqs_disabled());
> > > > + /*
> > > > +* Compatibility checks are done when loading KVM and when
> > > > enabling
> > > > +* hardware, e.g. during CPU hotplug, to ensure all online CPUs
> > > > are
> > > > +* compatible, i.e. KVM should never perform a compatibility
> > > > check
> > > > on
> > > > +* an offline CPU.
> > > > +*/
> > > > + WARN_ON(!irqs_disabled() && cpu_active(cpu));
> > > >
> > >
> > > Also, the logic of:
> > >
> > > !irqs_disabled() && cpu_active(cpu)
> > >
> > > is quite weird.
> > >
> > > The original "WARN(!irqs_disabled())" is reasonable because in STARTING
> > > section
> > > the IRQ is indeed disabled.
> > >
> > > But this doesn't make sense anymore after we move to ONLINE section, in
> > > which
> > > IRQ has already been enabled (see start_secondary()). IIUC the WARN_ON()
> > > doesn't get exploded is purely because there's an additional
> > > cpu_active(cpu)
> > > check.
> > >
> > > So, a more reasonable check should be something like:
> > >
> > > WARN_ON(irqs_disabled() || cpu_active(cpu) || !cpu_online(cpu));
> > >
> > > Or we can simply do:
> > >
> > > WARN_ON(!cpu_online(cpu) || cpu_active(cpu));
> > >
> > > (because I don't know whether it's possible IRQ can somehow get disabled
> > > in
> > > ONLINE section).
> > >
> > > Btw above is purely based on code analysis, but I haven't done any test.
> >
> > Hmm.. I wasn't thinking thoroughly. I forgot CPU compatibility check also
> > happens on all online cpus when loading KVM. For this case, IRQ is
> > disabled and
> > cpu_active() is true. For the hotplug case, IRQ is enabled but
> > cpu_active() is
> > false.
>
> Actually, you're right (and wrong). You're right in that the WARN is flawed.
> And
> the reason for that is because you're wrong about the hotplug case. In this
> version
> of things, the compatibility checks are routed through hardware enabling,
> i.e. this
> flow is used only when loading KVM. This helper should only be called via
> SMP function
> call, which means that IRQs should always be disabled.
Did you mean below code change in later patch "[PATCH 39/44] KVM: Drop
kvm_count_lock and instead protect kvm_usage_count with kvm_lock"?
/*
* Abort the CPU online process if hardware virtualization cannot
* be enabled. Otherwise running VMs would encounter unrecoverable
@@ -5039,13 +5039,16 @@ static int kvm_online_cpu(unsigned int cpu)
if (kvm_usage_count) {
WARN_ON_ONCE(atomic_read(&hardware_enable_failed));
+ local_irq_save(flags);
hardware_enable_nolock(NULL);
+ local_irq_restore(flags);
+
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH 13/44] KVM: x86: Serialize vendor module initialization (hardware setup)
On Wed, 2022-11-02 at 23:18 +, Sean Christopherson wrote:
> Acquire a new mutex, vendor_module_lock, in kvm_x86_vendor_init() while
> doing hardware setup to ensure that concurrent calls are fully serialized.
> KVM rejects attempts to load vendor modules if a different module has
> already been loaded, but doesn't handle the case where multiple vendor
> modules are loaded at the same time, and module_init() doesn't run under
> the global module_mutex.
>
> Note, in practice, this is likely a benign bug as no platform exists that
> supports both SVM and VMX, i.e. barring a weird VM setup, one of the
> vendor modules is guaranteed to fail a support check before modifying
> common KVM state.
>
> Alternatively, KVM could perform an atomic CMPXCHG on .hardware_enable,
> but that comes with its own ugliness as it would require setting
> .hardware_enable before success is guaranteed, e.g. attempting to load
> the "wrong" could result in spurious failure to load the "right" module.
>
> Introduce a new mutex as using kvm_lock is extremely deadlock prone due
> to kvm_lock being taken under cpus_write_lock(), and in the future, under
> under cpus_read_lock(). Any operation that takes cpus_read_lock() while
> holding kvm_lock would potentially deadlock, e.g. kvm_timer_init() takes
> cpus_read_lock() to register a callback. In theory, KVM could avoid
> such problematic paths, i.e. do less setup under kvm_lock, but avoiding
> all calls to cpus_read_lock() is subtly difficult and thus fragile. E.g.
> updating static calls also acquires cpus_read_lock().
>
> Inverting the lock ordering, i.e. always taking kvm_lock outside
> cpus_read_lock(), is not a viable option, e.g. kvm_online_cpu() takes
> kvm_lock and is called under cpus_write_lock().
"kvm_online_cpu() takes kvm_lock and is called under cpus_write_lock()" hasn't
happened yet.
>
> The lockdep splat below is dependent on future patches to take
> cpus_read_lock() in hardware_enable_all(), but as above, deadlock is
> already is already possible.
IIUC kvm_lock by design is supposed to protect vm_list, thus IMHO naturally it
doesn't fit to protect multiple vendor module loading.
Looks above argument is good enough. I am not sure whether we need additional
justification which comes from future patches. :)
Also, do you also want to update Documentation/virt/kvm/locking.rst" in this
patch?
>
>
> ==
> WARNING: possible circular locking dependency detected
> 6.0.0-smp--7ec93244f194-init2 #27 Tainted: G O
> --
> stable/251833 is trying to acquire lock:
> c097ea28 (kvm_lock){+.+.}-{3:3}, at: hardware_enable_all+0x1f/0xc0
> [kvm]
>
>but task is already holding lock:
> a2456828 (cpu_hotplug_lock){}-{0:0}, at:
> hardware_enable_all+0xf/0xc0 [kvm]
>
>which lock already depends on the new lock.
>
>the existing dependency chain (in reverse order) is:
>
>-> #1 (cpu_hotplug_lock){}-{0:0}:
> cpus_read_lock+0x2a/0xa0
> __cpuhp_setup_state+0x2b/0x60
> __kvm_x86_vendor_init+0x16a/0x1870 [kvm]
> kvm_x86_vendor_init+0x23/0x40 [kvm]
> 0xc0a4d02b
> do_one_initcall+0x110/0x200
> do_init_module+0x4f/0x250
> load_module+0x1730/0x18f0
> __se_sys_finit_module+0xca/0x100
> __x64_sys_finit_module+0x1d/0x20
> do_syscall_64+0x3d/0x80
> entry_SYSCALL_64_after_hwframe+0x63/0xcd
>
>-> #0 (kvm_lock){+.+.}-{3:3}:
> __lock_acquire+0x16f4/0x30d0
> lock_acquire+0xb2/0x190
> __mutex_lock+0x98/0x6f0
> mutex_lock_nested+0x1b/0x20
> hardware_enable_all+0x1f/0xc0 [kvm]
> kvm_dev_ioctl+0x45e/0x930 [kvm]
> __se_sys_ioctl+0x77/0xc0
> __x64_sys_ioctl+0x1d/0x20
> do_syscall_64+0x3d/0x80
> entry_SYSCALL_64_after_hwframe+0x63/0xcd
>
>other info that might help us debug this:
>
>Possible unsafe locking scenario:
>
> CPU0CPU1
>
> lock(cpu_hotplug_lock);
> lock(kvm_lock);
> lock(cpu_hotplug_lock);
> lock(kvm_lock);
>
> *** DEADLOCK ***
>
> 1 lock held by stable/251833:
>#0: a2456828 (cpu_hotplug_lock){}-{0:0}, at:
> hardware_enable_all+0xf/0xc0 [kvm]
>
> Signed-off-by: Sean Christopherson
> ---
> arch/x86/kvm/x86.c | 18 --
> 1 file changed, 16 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index a0ca401d3cdf..218707597bea 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -128,6 +128,7 @@ static int kvm_vcpu_do_singlestep(struct kvm_vcpu *vcpu);
> static int __set_sregs2(struct kvm_vcpu *vcpu,
Re: [PATCH 33/44] KVM: x86: Do VMX/SVM support checks directly in vendor code
On Wed, 2022-11-02 at 23:19 +, 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.
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH 38/44] KVM: Disable CPU hotplug during hardware enabling
On Thu, 2022-11-10 at 01:08 +, Huang, Kai wrote: > > - WARN_ON(!irqs_disabled()); > > + /* > > +* Compatibility checks are done when loading KVM and when enabling > > +* hardware, e.g. during CPU hotplug, to ensure all online CPUs are > > +* compatible, i.e. KVM should never perform a compatibility check > > on > > +* an offline CPU. > > +*/ > > + WARN_ON(!irqs_disabled() && cpu_active(cpu)); > > Comment doesn't match with the code? > > "KVM should never perform a compatibility check on on offline CPU" should be > something like: > > WARN_ON(!cpu_online(cpu)); > > So, should the comment be something like below? > > "KVM compatibility check happens before CPU is marked as active". Also ignore this one as I only thought about hotplug case. ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH 38/44] KVM: Disable CPU hotplug during hardware enabling
On Thu, 2022-11-10 at 01:33 +, Huang, Kai wrote: > > @@ -9283,7 +9283,13 @@ static int > > kvm_x86_check_processor_compatibility(struct kvm_x86_init_ops *ops) > > int cpu = smp_processor_id(); > > struct cpuinfo_x86 *c = &cpu_data(cpu); > > > > - WARN_ON(!irqs_disabled()); > > + /* > > +* Compatibility checks are done when loading KVM and when enabling > > +* hardware, e.g. during CPU hotplug, to ensure all online CPUs are > > +* compatible, i.e. KVM should never perform a compatibility check > > on > > +* an offline CPU. > > +*/ > > + WARN_ON(!irqs_disabled() && cpu_active(cpu)); > > > > Also, the logic of: > > !irqs_disabled() && cpu_active(cpu) > > is quite weird. > > The original "WARN(!irqs_disabled())" is reasonable because in STARTING > section > the IRQ is indeed disabled. > > But this doesn't make sense anymore after we move to ONLINE section, in which > IRQ has already been enabled (see start_secondary()). IIUC the WARN_ON() > doesn't get exploded is purely because there's an additional cpu_active(cpu) > check. > > So, a more reasonable check should be something like: > > WARN_ON(irqs_disabled() || cpu_active(cpu) || !cpu_online(cpu)); > > Or we can simply do: > > WARN_ON(!cpu_online(cpu) || cpu_active(cpu)); > > (because I don't know whether it's possible IRQ can somehow get disabled in > ONLINE section). > > Btw above is purely based on code analysis, but I haven't done any test. Hmm.. I wasn't thinking thoroughly. I forgot CPU compatibility check also happens on all online cpus when loading KVM. For this case, IRQ is disabled and cpu_active() is true. For the hotplug case, IRQ is enabled but cpu_active() is false. So WARN_ON(!irqs_disabled() && cpu_active(cpu)) looks reasonable. Sorry for the noise. Just needed some time to connect the comment with the code. ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH 38/44] KVM: Disable CPU hotplug during hardware enabling
On Wed, 2022-11-02 at 23:19 +, Sean Christopherson wrote:
> From: Chao Gao
>
> Disable CPU hotplug during hardware_enable_all() to prevent the corner
> case where if the following sequence occurs:
>
> 1. A hotplugged CPU marks itself online in cpu_online_mask
> 2. The hotplugged CPU enables interrupt before invoking KVM's ONLINE
> callback
> 3 hardware_enable_all() is invoked on another CPU right
>
> the hotplugged CPU will be included in on_each_cpu() and thus get sent
> through hardware_enable_nolock() before kvm_online_cpu() is called.
>
> start_secondary { ...
> set_cpu_online(smp_processor_id(), true); <- 1
> ...
> local_irq_enable(); <- 2
> ...
> cpu_startup_entry(CPUHP_AP_ONLINE_IDLE); <- 3
> }
>
> KVM currently fudges around this race by keeping track of which CPUs have
> done hardware enabling (see commit 1b6c016818a5 "KVM: Keep track of which
> cpus have virtualization enabled"), but that's an inefficient, convoluted,
> and hacky solution.
>
> Signed-off-by: Chao Gao
> [sean: split to separate patch, write changelog]
> Signed-off-by: Sean Christopherson
> ---
> arch/x86/kvm/x86.c | 8 +++-
> virt/kvm/kvm_main.c | 10 ++
> 2 files changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index a7b1d916ecb2..a15e54ba0471 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -9283,7 +9283,13 @@ static int
> kvm_x86_check_processor_compatibility(struct kvm_x86_init_ops *ops)
> int cpu = smp_processor_id();
> struct cpuinfo_x86 *c = &cpu_data(cpu);
>
> - WARN_ON(!irqs_disabled());
> + /*
> + * Compatibility checks are done when loading KVM and when enabling
> + * hardware, e.g. during CPU hotplug, to ensure all online CPUs are
> + * compatible, i.e. KVM should never perform a compatibility check on
> + * an offline CPU.
> + */
> + WARN_ON(!irqs_disabled() && cpu_active(cpu));
Comment doesn't match with the code?
"KVM should never perform a compatibility check on on offline CPU" should be
something like:
WARN_ON(!cpu_online(cpu));
So, should the comment be something like below?
"KVM compatibility check happens before CPU is marked as active".
>
> if (__cr4_reserved_bits(cpu_has, c) !=
> __cr4_reserved_bits(cpu_has, &boot_cpu_data))
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index fd9e39c85549..4e765ef9f4bd 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -5088,6 +5088,15 @@ static int hardware_enable_all(void)
> {
> int r = 0;
>
> + /*
> + * When onlining a CPU, cpu_online_mask is set before kvm_online_cpu()
> + * is called, and so on_each_cpu() between them includes the CPU that
> + * is being onlined. As a result, hardware_enable_nolock() may get
> + * invoked before kvm_online_cpu().
> + *
> + * Disable CPU hotplug to prevent scenarios where KVM sees
> + */
The above sentence is broken.
I think below comment Quoted from Isaku's series should be OK?
/*
* During onlining a CPU, cpu_online_mask is set before
kvm_online_cpu()
* is called. on_each_cpu() between them includes the CPU. As a result,
* hardware_enable_nolock() may get invoked before kvm_online_cpu().
* This would enable hardware virtualization on that cpu without
* compatibility checks, which can potentially crash system or break
* running VMs.
*
* Disable CPU hotplug to prevent this case from happening.
*/
> + cpus_read_lock();
> raw_spin_lock(&kvm_count_lock);
>
> kvm_usage_count++;
> @@ -5102,6 +5111,7 @@ static int hardware_enable_all(void)
> }
>
> raw_spin_unlock(&kvm_count_lock);
> + cpus_read_unlock();
>
> return r;
> }
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH 38/44] KVM: Disable CPU hotplug during hardware enabling
On Wed, 2022-11-02 at 23:19 +, Sean Christopherson wrote:
> From: Chao Gao
>
> Disable CPU hotplug during hardware_enable_all() to prevent the corner
> case where if the following sequence occurs:
>
> 1. A hotplugged CPU marks itself online in cpu_online_mask
> 2. The hotplugged CPU enables interrupt before invoking KVM's ONLINE
> callback
> 3 hardware_enable_all() is invoked on another CPU right
>
> the hotplugged CPU will be included in on_each_cpu() and thus get sent
> through hardware_enable_nolock() before kvm_online_cpu() is called.
>
> start_secondary { ...
> set_cpu_online(smp_processor_id(), true); <- 1
> ...
> local_irq_enable(); <- 2
> ...
> cpu_startup_entry(CPUHP_AP_ONLINE_IDLE); <- 3
> }
>
> KVM currently fudges around this race by keeping track of which CPUs have
> done hardware enabling (see commit 1b6c016818a5 "KVM: Keep track of which
> cpus have virtualization enabled"), but that's an inefficient, convoluted,
> and hacky solution.
>
> Signed-off-by: Chao Gao
> [sean: split to separate patch, write changelog]
> Signed-off-by: Sean Christopherson
> ---
> arch/x86/kvm/x86.c | 8 +++-
> virt/kvm/kvm_main.c | 10 ++
> 2 files changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index a7b1d916ecb2..a15e54ba0471 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -9283,7 +9283,13 @@ static int
> kvm_x86_check_processor_compatibility(struct kvm_x86_init_ops *ops)
> int cpu = smp_processor_id();
> struct cpuinfo_x86 *c = &cpu_data(cpu);
>
> - WARN_ON(!irqs_disabled());
> + /*
> + * Compatibility checks are done when loading KVM and when enabling
> + * hardware, e.g. during CPU hotplug, to ensure all online CPUs are
> + * compatible, i.e. KVM should never perform a compatibility check on
> + * an offline CPU.
> + */
> + WARN_ON(!irqs_disabled() && cpu_active(cpu));
>
Also, the logic of:
!irqs_disabled() && cpu_active(cpu)
is quite weird.
The original "WARN(!irqs_disabled())" is reasonable because in STARTING section
the IRQ is indeed disabled.
But this doesn't make sense anymore after we move to ONLINE section, in which
IRQ has already been enabled (see start_secondary()). IIUC the WARN_ON()
doesn't get exploded is purely because there's an additional cpu_active(cpu)
check.
So, a more reasonable check should be something like:
WARN_ON(irqs_disabled() || cpu_active(cpu) || !cpu_online(cpu));
Or we can simply do:
WARN_ON(!cpu_online(cpu) || cpu_active(cpu));
(because I don't know whether it's possible IRQ can somehow get disabled in
ONLINE section).
Btw above is purely based on code analysis, but I haven't done any test.
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH 00/44] KVM: Rework kvm_init() and hardware enabling
On Tue, 2022-11-08 at 08:56 +, Huang, Kai wrote: > On Mon, 2022-11-07 at 21:43 -0800, Isaku Yamahata wrote: > > On Tue, Nov 08, 2022 at 01:09:27AM +0000, > > "Huang, Kai" wrote: > > > > > On Mon, 2022-11-07 at 13:46 -0800, Isaku Yamahata wrote: > > > > > On Fri, Nov 04, 2022, Isaku Yamahata wrote: > > > > > > Thanks for the patch series. I the rebased TDX KVM patch series and > > > > > > it > > > > > > worked. > > > > > > Since cpu offline needs to be rejected in some cases(To keep at > > > > > > least one > > > > > > cpu > > > > > > on a package), arch hook for cpu offline is needed. > > > > > > > > > > I hate to bring this up because I doubt there's a real use case for > > > > > SUSPEND > > > > > with > > > > > TDX, but the CPU offline path isn't just for true offlining of CPUs. > > > > > When > > > > > the > > > > > system enters SUSPEND, only the initiating CPU goes through > > > > > kvm_suspend()+kvm_resume(), > > > > > all responding CPUs go through CPU offline+online. I.e. disallowing > > > > > all > > > > > CPUs from > > > > > going "offline" will prevent suspending the system. > > > > > > > > The current TDX KVM implementation disallows CPU package from offline > > > > only > > > > when > > > > TDs are running. If no TD is running, CPU offline is allowed. So > > > > before > > > > SUSPEND, TDs need to be killed via systemd or something. After killing > > > > TDs, > > > > the > > > > system can enter into SUSPEND state. > > > > > > This seems not correct. You need one cpu for each to be online in order > > > to > > > create TD as well, as TDH.MNG.KEY.CONFIG needs to be called on all > > > packages, > > > correct? > > > > That's correct. In such case, the creation of TD fails. TD creation checks > > if > > at least one cpu is online on all CPU packages. If no, error. > > I think we can just always refuse to offline the last cpu for each package > when > TDX is enabled. It's simpler I guess. Sorry I wasn't reading carefully. Please ignore. We need to support suspend :) ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH 00/44] KVM: Rework kvm_init() and hardware enabling
On Mon, 2022-11-07 at 21:43 -0800, Isaku Yamahata wrote: > On Tue, Nov 08, 2022 at 01:09:27AM +, > "Huang, Kai" wrote: > > > On Mon, 2022-11-07 at 13:46 -0800, Isaku Yamahata wrote: > > > > On Fri, Nov 04, 2022, Isaku Yamahata wrote: > > > > > Thanks for the patch series. I the rebased TDX KVM patch series and it > > > > > worked. > > > > > Since cpu offline needs to be rejected in some cases(To keep at least > > > > > one > > > > > cpu > > > > > on a package), arch hook for cpu offline is needed. > > > > > > > > I hate to bring this up because I doubt there's a real use case for > > > > SUSPEND > > > > with > > > > TDX, but the CPU offline path isn't just for true offlining of CPUs. > > > > When > > > > the > > > > system enters SUSPEND, only the initiating CPU goes through > > > > kvm_suspend()+kvm_resume(), > > > > all responding CPUs go through CPU offline+online. I.e. disallowing all > > > > CPUs from > > > > going "offline" will prevent suspending the system. > > > > > > The current TDX KVM implementation disallows CPU package from offline only > > > when > > > TDs are running. If no TD is running, CPU offline is allowed. So before > > > SUSPEND, TDs need to be killed via systemd or something. After killing > > > TDs, > > > the > > > system can enter into SUSPEND state. > > > > This seems not correct. You need one cpu for each to be online in order to > > create TD as well, as TDH.MNG.KEY.CONFIG needs to be called on all packages, > > correct? > > That's correct. In such case, the creation of TD fails. TD creation checks if > at least one cpu is online on all CPU packages. If no, error. I think we can just always refuse to offline the last cpu for each package when TDX is enabled. It's simpler I guess. ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH 00/44] KVM: Rework kvm_init() and hardware enabling
On Mon, 2022-11-07 at 13:46 -0800, Isaku Yamahata wrote: > > On Fri, Nov 04, 2022, Isaku Yamahata wrote: > > > Thanks for the patch series. I the rebased TDX KVM patch series and it > > > worked. > > > Since cpu offline needs to be rejected in some cases(To keep at least one > > > cpu > > > on a package), arch hook for cpu offline is needed. > > > > I hate to bring this up because I doubt there's a real use case for SUSPEND > > with > > TDX, but the CPU offline path isn't just for true offlining of CPUs. When > > the > > system enters SUSPEND, only the initiating CPU goes through > > kvm_suspend()+kvm_resume(), > > all responding CPUs go through CPU offline+online. I.e. disallowing all > > CPUs from > > going "offline" will prevent suspending the system. > > The current TDX KVM implementation disallows CPU package from offline only > when > TDs are running. If no TD is running, CPU offline is allowed. So before > SUSPEND, TDs need to be killed via systemd or something. After killing TDs, > the > system can enter into SUSPEND state. This seems not correct. You need one cpu for each to be online in order to create TD as well, as TDH.MNG.KEY.CONFIG needs to be called on all packages, correct? ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
