[PATCH 0/6] Improve KVM's interaction with CPU hotplug

[PATCH 0/6] Improve KVM's interaction with CPU hotplug

[Chao Gao]

KVM registers its CPU hotplug callback to CPU starting section. And in the
callback, KVM enables hardware virtualization on hotplugged CPUs if any VM
is running on existing CPUs.

There are two problems in the process:
1. KVM doesn't do compatibility checks before enabling hardware
virtualization on hotplugged CPUs. This may cause #GP if VMX isn't
supported or vmentry failure if some in-use VMX features are missing on
hotplugged CPUs. Both break running VMs.
2. Callbacks in CPU STARTING section cannot fail. So, even if KVM finds
some incompatible CPUs, its callback cannot block CPU hotplug.

This series improves KVM's interaction with CPU hotplug to avoid
incompatible CPUs breaking running VMs. Following changes are made:

1. move KVM's CPU hotplug callback to ONLINE section (suggested by Thomas)
2. do compatibility checks on hotplugged CPUs.
3. abort onlining incompatible CPUs if there is a running VM.

This series is a follow-up to the discussion about KVM and CPU hotplug
https://lore.kernel.org/lkml/3d3296f0-9245-40f9-1b5a-efffdb082de9@redhat.com/T/

Note: this series is tested only on Intel systems.

Chao Gao (6):
  KVM: x86: Move check_processor_compatibility from init ops to runtime
    ops
  KVM: x86: Use kvm_x86_ops in kvm_arch_check_processor_compat
  KVM: Remove opaque from kvm_arch_check_processor_compat
  KVM: Rename and move CPUHP_AP_KVM_STARTING to ONLINE section
  KVM: x86: Remove WARN_ON in kvm_arch_check_processor_compat
  KVM: Do compatibility checks on hotplugged CPUs

 arch/arm64/kvm/arm.c            |  2 +-
 arch/mips/kvm/mips.c            |  2 +-
 arch/powerpc/kvm/powerpc.c      |  2 +-
 arch/riscv/kvm/main.c           |  2 +-
 arch/s390/kvm/kvm-s390.c        |  2 +-
 arch/x86/include/asm/kvm_host.h |  2 +-
 arch/x86/kvm/svm/svm.c          |  4 +-
 arch/x86/kvm/vmx/evmcs.c        |  2 +-
 arch/x86/kvm/vmx/evmcs.h        |  2 +-
 arch/x86/kvm/vmx/vmx.c          | 12 +++---
 arch/x86/kvm/x86.c              |  7 +---
 include/linux/cpuhotplug.h      |  2 +-
 include/linux/kvm_host.h        |  2 +-
 virt/kvm/kvm_main.c             | 74 ++++++++++++++++++++++++---------
 14 files changed, 74 insertions(+), 43 deletions(-)

-- 
2.25.1

[PATCH 1/6] KVM: x86: Move check_processor_compatibility from init ops to runtime ops

[Chao Gao]

so that KVM can do compatibility checks on hotplugged CPUs. Drop __init
from check_processor_compatibility() and its callees.

Signed-off-by: Chao Gao <chao.gao@intel.com>
---
 arch/x86/include/asm/kvm_host.h |  2 +-
 arch/x86/kvm/svm/svm.c          |  4 ++--
 arch/x86/kvm/vmx/evmcs.c        |  2 +-
 arch/x86/kvm/vmx/evmcs.h        |  2 +-
 arch/x86/kvm/vmx/vmx.c          | 12 ++++++------
 arch/x86/kvm/x86.c              |  2 +-
 6 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 5d97f4adc1cb..0d5dc148090c 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -1313,6 +1313,7 @@ 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);
@@ -1513,7 +1514,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);
 
 	struct kvm_x86_ops *runtime_ops;
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 6cb38044a860..74c41a261633 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -3825,7 +3825,7 @@ svm_patch_hypercall(struct kvm_vcpu *vcpu, unsigned char *hypercall)
 	hypercall[2] = 0xd9;
 }
 
-static int __init svm_check_processor_compat(void)
+static int svm_check_processor_compat(void)
 {
 	return 0;
 }
@@ -4374,6 +4374,7 @@ static struct kvm_x86_ops svm_x86_ops __initdata = {
 	.name = "kvm_amd",
 
 	.hardware_unsetup = svm_hardware_teardown,
+	.check_processor_compatibility = svm_check_processor_compat,
 	.hardware_enable = svm_hardware_enable,
 	.hardware_disable = svm_hardware_disable,
 	.cpu_has_accelerated_tpr = svm_cpu_has_accelerated_tpr,
@@ -4743,7 +4744,6 @@ 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,
 
 	.runtime_ops = &svm_x86_ops,
 };
diff --git a/arch/x86/kvm/vmx/evmcs.c b/arch/x86/kvm/vmx/evmcs.c
index ba6f99f584ac..50f923e9917e 100644
--- a/arch/x86/kvm/vmx/evmcs.c
+++ b/arch/x86/kvm/vmx/evmcs.c
@@ -296,7 +296,7 @@ const struct evmcs_field vmcs_field_to_evmcs_1[] = {
 };
 const unsigned int nr_evmcs_1_fields = ARRAY_SIZE(vmcs_field_to_evmcs_1);
 
-__init void evmcs_sanitize_exec_ctrls(struct vmcs_config *vmcs_conf)
+void evmcs_sanitize_exec_ctrls(struct vmcs_config *vmcs_conf)
 {
 	vmcs_conf->pin_based_exec_ctrl &= ~EVMCS1_UNSUPPORTED_PINCTRL;
 	vmcs_conf->cpu_based_2nd_exec_ctrl &= ~EVMCS1_UNSUPPORTED_2NDEXEC;
diff --git a/arch/x86/kvm/vmx/evmcs.h b/arch/x86/kvm/vmx/evmcs.h
index 16731d2cf231..17a7c956396b 100644
--- a/arch/x86/kvm/vmx/evmcs.h
+++ b/arch/x86/kvm/vmx/evmcs.h
@@ -181,7 +181,7 @@ static inline void evmcs_load(u64 phys_addr)
 	vp_ap->enlighten_vmentry = 1;
 }
 
-__init void evmcs_sanitize_exec_ctrls(struct vmcs_config *vmcs_conf);
+void evmcs_sanitize_exec_ctrls(struct vmcs_config *vmcs_conf);
 #else /* !IS_ENABLED(CONFIG_HYPERV) */
 static __always_inline void evmcs_write64(unsigned long field, u64 value) {}
 static inline void evmcs_write32(unsigned long field, u32 value) {}
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index fe06b02994e6..de617a7bd966 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -2373,8 +2373,8 @@ static bool cpu_has_sgx(void)
 	return cpuid_eax(0) >= 0x12 && (cpuid_eax(0x12) & BIT(0));
 }
 
-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;
@@ -2392,8 +2392,8 @@ static __init int adjust_vmx_controls(u32 ctl_min, u32 ctl_opt,
 	return 0;
 }
 
-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 min, opt, min2, opt2;
@@ -6990,7 +6990,7 @@ static int vmx_vm_init(struct kvm *kvm)
 	return 0;
 }
 
-static int __init vmx_check_processor_compat(void)
+static int vmx_check_processor_compat(void)
 {
 	struct vmcs_config vmcs_conf;
 	struct vmx_capability vmx_cap;
@@ -7578,6 +7578,7 @@ static struct kvm_x86_ops vmx_x86_ops __initdata = {
 
 	.hardware_unsetup = hardware_unsetup,
 
+	.check_processor_compatibility = vmx_check_processor_compat,
 	.hardware_enable = hardware_enable,
 	.hardware_disable = hardware_disable,
 	.cpu_has_accelerated_tpr = report_flexpriority,
@@ -7911,7 +7912,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,
 
 	.runtime_ops = &vmx_x86_ops,
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index c194a8cbd25f..6411417b6871 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -11391,7 +11391,7 @@ int kvm_arch_check_processor_compat(void *opaque)
 	    __cr4_reserved_bits(cpu_has, &boot_cpu_data))
 		return -EIO;
 
-	return ops->check_processor_compatibility();
+	return ops->runtime_ops->check_processor_compatibility();
 }
 
 bool kvm_vcpu_is_reset_bsp(struct kvm_vcpu *vcpu)
-- 
2.25.1

Re: [PATCH 1/6] KVM: x86: Move check_processor_compatibility from init ops to runtime ops

[Sean Christopherson]

On Mon, Dec 27, 2021, Chao Gao wrote:
> so that KVM can do compatibility checks on hotplugged CPUs. Drop __init
> from check_processor_compatibility() and its callees.

Losing the __init annotation on all these helpers makes me a bit sad, more from a
documentation perspective than a "but we could shave a few bytes" perspective.
More than once I've wondered why some bit of code isn't __init, only to realize
its used for hotplug.

What if we added an __init_or_hotplug annotation that is a nop if HOTPLUG_CPU=y?
At a glance, KVM could use that if the guts of kvm_online_cpu() were #idef'd out
on !CONFIG_HOTPLUG_CPU.  That also give us a bit of test coverage for bots that
build with SMP=n.

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index a80e3b0c11a8..30bbcb4f4057 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -11380,7 +11380,7 @@ void kvm_arch_hardware_unsetup(void)
        static_call(kvm_x86_hardware_unsetup)();
 }

-int kvm_arch_check_processor_compat(void)
+int __init_or_hotplug kvm_arch_check_processor_compat(void)
 {
        struct cpuinfo_x86 *c = &cpu_data(smp_processor_id());

diff --git a/include/linux/init.h b/include/linux/init.h
index d82b4b2e1d25..33788b3c180a 100644
--- a/include/linux/init.h
+++ b/include/linux/init.h
@@ -53,6 +53,12 @@
 #define __exitdata     __section(".exit.data")
 #define __exit_call    __used __section(".exitcall.exit")

+#ifdef CONFIG_HOTPLUG_CPU
+#define __init_or_hotplug
+#else
+#define __init_or_hotplug __init
+#endif /* CONFIG_HOTPLUG_CPU
+
 /*
  * modpost check for section mismatches during the kernel build.
  * A section mismatch happens when there are references from a

Re: [PATCH 1/6] KVM: x86: Move check_processor_compatibility from init ops to runtime ops

[Chao Gao]

On Mon, Jan 10, 2022 at 11:27:09PM +0000, Sean Christopherson wrote:
>On Mon, Dec 27, 2021, Chao Gao wrote:
>> so that KVM can do compatibility checks on hotplugged CPUs. Drop __init
>> from check_processor_compatibility() and its callees.
>
>Losing the __init annotation on all these helpers makes me a bit sad, more from a
>documentation perspective than a "but we could shave a few bytes" perspective.

This makes sense.

>More than once I've wondered why some bit of code isn't __init, only to realize
>its used for hotplug.

Same problem to some global data structures which can be __initdata if hotplug
isn't supported.

>
>What if we added an __init_or_hotplug annotation that is a nop if HOTPLUG_CPU=y?

Personally __init_or_hotplug is a little long as an annotation. How about
__hotplug?

One concern is: is it acceptable to introduce a new annotation and use it in
new code but not fix all places that should use it in existing code.

I think the right process is
1. introduce a new annotation
2. fix existing code to use this annotation
3. add new code.

There is no doubt that #2 would take great effort. I'm not sure if it is really
worth it.

>At a glance, KVM could use that if the guts of kvm_online_cpu() were #idef'd out
>on !CONFIG_HOTPLUG_CPU.  That also give us a bit of test coverage for bots that
>build with SMP=n.

Will do with your suggested-by.

>
>diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>index a80e3b0c11a8..30bbcb4f4057 100644
>--- a/arch/x86/kvm/x86.c
>+++ b/arch/x86/kvm/x86.c
>@@ -11380,7 +11380,7 @@ void kvm_arch_hardware_unsetup(void)
>        static_call(kvm_x86_hardware_unsetup)();
> }
>
>-int kvm_arch_check_processor_compat(void)
>+int __init_or_hotplug kvm_arch_check_processor_compat(void)
> {
>        struct cpuinfo_x86 *c = &cpu_data(smp_processor_id());
>
>diff --git a/include/linux/init.h b/include/linux/init.h
>index d82b4b2e1d25..33788b3c180a 100644
>--- a/include/linux/init.h
>+++ b/include/linux/init.h
>@@ -53,6 +53,12 @@
> #define __exitdata     __section(".exit.data")
> #define __exit_call    __used __section(".exitcall.exit")
>
>+#ifdef CONFIG_HOTPLUG_CPU
>+#define __init_or_hotplug
>+#else
>+#define __init_or_hotplug __init
>+#endif /* CONFIG_HOTPLUG_CPU
>+
> /*
>  * modpost check for section mismatches during the kernel build.
>  * A section mismatch happens when there are references from a
>

Re: [PATCH 1/6] KVM: x86: Move check_processor_compatibility from init ops to runtime ops

[Sean Christopherson]

On Tue, Jan 11, 2022, Chao Gao wrote:
> On Mon, Jan 10, 2022 at 11:27:09PM +0000, Sean Christopherson wrote:
> >On Mon, Dec 27, 2021, Chao Gao wrote:
> >> so that KVM can do compatibility checks on hotplugged CPUs. Drop __init
> >> from check_processor_compatibility() and its callees.
> >
> >Losing the __init annotation on all these helpers makes me a bit sad, more from a
> >documentation perspective than a "but we could shave a few bytes" perspective.
> 
> This makes sense.
> 
> >More than once I've wondered why some bit of code isn't __init, only to realize
> >its used for hotplug.
> 
> Same problem to some global data structures which can be __initdata if hotplug
> isn't supported.
> 
> >
> >What if we added an __init_or_hotplug annotation that is a nop if HOTPLUG_CPU=y?
> 
> Personally __init_or_hotplug is a little long as an annotation. How about
> __hotplug?
> 
> One concern is: is it acceptable to introduce a new annotation and use it in
> new code but not fix all places that should use it in existing code.
> 
> I think the right process is
> 1. introduce a new annotation
> 2. fix existing code to use this annotation
> 3. add new code.
> 
> There is no doubt that #2 would take great effort. I'm not sure if it is really
> worth it.
> 
> >At a glance, KVM could use that if the guts of kvm_online_cpu() were #idef'd out
> >on !CONFIG_HOTPLUG_CPU.  That also give us a bit of test coverage for bots that
> >build with SMP=n.
> 
> Will do with your suggested-by.

I don't think you should try to add a new annotation in this series.  My question
really was just that, a question to others if there would be value in adding an
annotation to identify symbols that are !__init only because of hotplug.  Such new
functionality is certainly not required for fixing KVM's mishandling of hotplug,
and trying to cram it in here will bloat and slow down this series.

[PATCH 2/6] KVM: x86: Use kvm_x86_ops in kvm_arch_check_processor_compat

[Chao Gao]

check_processor_compatibility() is a "runtime" ops now. Use
kvm_x86_ops directly such that kvm_arch_check_processor_compat
can be called at runtime.

Signed-off-by: Chao Gao <chao.gao@intel.com>
---
 arch/x86/kvm/x86.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 6411417b6871..770b68e72391 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -11383,7 +11383,6 @@ void kvm_arch_hardware_unsetup(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());
 
@@ -11391,7 +11390,7 @@ int kvm_arch_check_processor_compat(void *opaque)
 	    __cr4_reserved_bits(cpu_has, &boot_cpu_data))
 		return -EIO;
 
-	return ops->runtime_ops->check_processor_compatibility();
+	return kvm_x86_ops.check_processor_compatibility();
 }
 
 bool kvm_vcpu_is_reset_bsp(struct kvm_vcpu *vcpu)
-- 
2.25.1

Re: [PATCH 2/6] KVM: x86: Use kvm_x86_ops in kvm_arch_check_processor_compat

[Sean Christopherson]

On Mon, Dec 27, 2021, Chao Gao wrote:
> check_processor_compatibility() is a "runtime" ops now. Use
> kvm_x86_ops directly such that kvm_arch_check_processor_compat
> can be called at runtime.
> 
> Signed-off-by: Chao Gao <chao.gao@intel.com>
> ---
>  arch/x86/kvm/x86.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 6411417b6871..770b68e72391 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -11383,7 +11383,6 @@ void kvm_arch_hardware_unsetup(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());
>  
> @@ -11391,7 +11390,7 @@ int kvm_arch_check_processor_compat(void *opaque)
>  	    __cr4_reserved_bits(cpu_has, &boot_cpu_data))
>  		return -EIO;
>  
> -	return ops->runtime_ops->check_processor_compatibility();
> +	return kvm_x86_ops.check_processor_compatibility();

I'd just squash this with patch 01.  And might as well make this a static_call().

>  }
>  
>  bool kvm_vcpu_is_reset_bsp(struct kvm_vcpu *vcpu)
> -- 
> 2.25.1
> 

Re: [PATCH 2/6] KVM: x86: Use kvm_x86_ops in kvm_arch_check_processor_compat

[Chao Gao]

On Mon, Jan 10, 2022 at 09:10:10PM +0000, Sean Christopherson wrote:
>On Mon, Dec 27, 2021, Chao Gao wrote:
>> check_processor_compatibility() is a "runtime" ops now. Use
>> kvm_x86_ops directly such that kvm_arch_check_processor_compat
>> can be called at runtime.
>> 
>> Signed-off-by: Chao Gao <chao.gao@intel.com>
>> ---
>>  arch/x86/kvm/x86.c | 3 +--
>>  1 file changed, 1 insertion(+), 2 deletions(-)
>> 
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index 6411417b6871..770b68e72391 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -11383,7 +11383,6 @@ void kvm_arch_hardware_unsetup(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());
>>  
>> @@ -11391,7 +11390,7 @@ int kvm_arch_check_processor_compat(void *opaque)
>>  	    __cr4_reserved_bits(cpu_has, &boot_cpu_data))
>>  		return -EIO;
>>  
>> -	return ops->runtime_ops->check_processor_compatibility();
>> +	return kvm_x86_ops.check_processor_compatibility();
>
>I'd just squash this with patch 01.  And might as well make this a static_call().

Will do

[PATCH 3/6] KVM: Remove opaque from kvm_arch_check_processor_compat

[Chao Gao]

No arch implementation uses this opaque now.

Signed-off-by: Chao Gao <chao.gao@intel.com>
---
 arch/arm64/kvm/arm.c       |  2 +-
 arch/mips/kvm/mips.c       |  2 +-
 arch/powerpc/kvm/powerpc.c |  2 +-
 arch/riscv/kvm/main.c      |  2 +-
 arch/s390/kvm/kvm-s390.c   |  2 +-
 arch/x86/kvm/x86.c         |  2 +-
 include/linux/kvm_host.h   |  2 +-
 virt/kvm/kvm_main.c        | 14 ++------------
 8 files changed, 9 insertions(+), 19 deletions(-)

diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
index 14106a7c75b5..c196f005a2d3 100644
--- a/arch/arm64/kvm/arm.c
+++ b/arch/arm64/kvm/arm.c
@@ -73,7 +73,7 @@ int kvm_arch_hardware_setup(void *opaque)
 	return 0;
 }
 
-int kvm_arch_check_processor_compat(void *opaque)
+int kvm_arch_check_processor_compat(void)
 {
 	return 0;
 }
diff --git a/arch/mips/kvm/mips.c b/arch/mips/kvm/mips.c
index e59cb6246f76..c5dc4fe53bfc 100644
--- a/arch/mips/kvm/mips.c
+++ b/arch/mips/kvm/mips.c
@@ -140,7 +140,7 @@ int kvm_arch_hardware_setup(void *opaque)
 	return 0;
 }
 
-int kvm_arch_check_processor_compat(void *opaque)
+int kvm_arch_check_processor_compat(void)
 {
 	return 0;
 }
diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
index 2ad0ccd202d5..30c817f3fa0c 100644
--- a/arch/powerpc/kvm/powerpc.c
+++ b/arch/powerpc/kvm/powerpc.c
@@ -423,7 +423,7 @@ int kvm_arch_hardware_setup(void *opaque)
 	return 0;
 }
 
-int kvm_arch_check_processor_compat(void *opaque)
+int kvm_arch_check_processor_compat(void)
 {
 	return kvmppc_core_check_processor_compat();
 }
diff --git a/arch/riscv/kvm/main.c b/arch/riscv/kvm/main.c
index 421ecf4e6360..3b0b104e443f 100644
--- a/arch/riscv/kvm/main.c
+++ b/arch/riscv/kvm/main.c
@@ -20,7 +20,7 @@ long kvm_arch_dev_ioctl(struct file *filp,
 	return -EINVAL;
 }
 
-int kvm_arch_check_processor_compat(void *opaque)
+int kvm_arch_check_processor_compat(void)
 {
 	return 0;
 }
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index 9c6d45d0d345..99c70d881cb6 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -252,7 +252,7 @@ int kvm_arch_hardware_enable(void)
 	return 0;
 }
 
-int kvm_arch_check_processor_compat(void *opaque)
+int kvm_arch_check_processor_compat(void)
 {
 	return 0;
 }
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 770b68e72391..aa09c8792134 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -11380,7 +11380,7 @@ void kvm_arch_hardware_unsetup(void)
 	static_call(kvm_x86_hardware_unsetup)();
 }
 
-int kvm_arch_check_processor_compat(void *opaque)
+int kvm_arch_check_processor_compat(void)
 {
 	struct cpuinfo_x86 *c = &cpu_data(smp_processor_id());
 
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 05862176df6a..630ffd2289c6 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -1307,7 +1307,7 @@ int kvm_arch_hardware_enable(void);
 void kvm_arch_hardware_disable(void);
 int kvm_arch_hardware_setup(void *opaque);
 void kvm_arch_hardware_unsetup(void);
-int kvm_arch_check_processor_compat(void *opaque);
+int kvm_arch_check_processor_compat(void);
 int kvm_arch_vcpu_runnable(struct kvm_vcpu *vcpu);
 bool kvm_arch_vcpu_in_kernel(struct kvm_vcpu *vcpu);
 int kvm_arch_vcpu_should_kick(struct kvm_vcpu *vcpu);
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 168d0ab93c88..4e1e7770e984 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -5600,22 +5600,14 @@ struct kvm_vcpu * __percpu *kvm_get_running_vcpus(void)
         return &kvm_running_vcpu;
 }
 
-struct kvm_cpu_compat_check {
-	void *opaque;
-	int *ret;
-};
-
 static void check_processor_compat(void *data)
 {
-	struct kvm_cpu_compat_check *c = data;
-
-	*c->ret = kvm_arch_check_processor_compat(c->opaque);
+	*(int *)data = kvm_arch_check_processor_compat();
 }
 
 int kvm_init(void *opaque, unsigned vcpu_size, unsigned vcpu_align,
 		  struct module *module)
 {
-	struct kvm_cpu_compat_check c;
 	int r;
 	int cpu;
 
@@ -5643,10 +5635,8 @@ int kvm_init(void *opaque, unsigned vcpu_size, unsigned vcpu_align,
 	if (r < 0)
 		goto out_free_1;
 
-	c.ret = &r;
-	c.opaque = opaque;
 	for_each_online_cpu(cpu) {
-		smp_call_function_single(cpu, check_processor_compat, &c, 1);
+		smp_call_function_single(cpu, check_processor_compat, &r, 1);
 		if (r < 0)
 			goto out_free_2;
 	}
-- 
2.25.1

Re: [PATCH 3/6] KVM: Remove opaque from kvm_arch_check_processor_compat

[Sean Christopherson]

On Mon, Dec 27, 2021, Chao Gao wrote:
> No arch implementation uses this opaque now.

Except for the RISC-V part, this can be a pure revert of commit b99040853738 ("KVM:
Pass kvm_init()'s opaque param to additional arch funcs").  I think it makes sense
to process it as a revert, with a short blurb in the changelog to note that RISC-V
is manually modified as RISC-V support came along in the interim.

Re: [PATCH 3/6] KVM: Remove opaque from kvm_arch_check_processor_compat

[Chao Gao]

On Mon, Jan 10, 2022 at 11:06:44PM +0000, Sean Christopherson wrote:
>On Mon, Dec 27, 2021, Chao Gao wrote:
>> No arch implementation uses this opaque now.
>
>Except for the RISC-V part, this can be a pure revert of commit b99040853738 ("KVM:
>Pass kvm_init()'s opaque param to additional arch funcs").  I think it makes sense
>to process it as a revert, with a short blurb in the changelog to note that RISC-V
>is manually modified as RISC-V support came along in the interim.

commit b99040853738 adds opaque param to kvm_arch_hardware_setup(), which isn't
reverted in this patch. I.e., this patch is a partial revert of b99040853738
plus manual changes to RISC-V. Given that, "process it as a revert" means
clearly say in changelog that this commit contains a partial revert of commit
b99040853738 ("KVM: Pass kvm_init()'s opaque param to additional arch funcs").

Right?

Re: [PATCH 3/6] KVM: Remove opaque from kvm_arch_check_processor_compat

[Sean Christopherson]

On Tue, Jan 11, 2022, Chao Gao wrote:
> On Mon, Jan 10, 2022 at 11:06:44PM +0000, Sean Christopherson wrote:
> >On Mon, Dec 27, 2021, Chao Gao wrote:
> >> No arch implementation uses this opaque now.
> >
> >Except for the RISC-V part, this can be a pure revert of commit b99040853738 ("KVM:
> >Pass kvm_init()'s opaque param to additional arch funcs").  I think it makes sense
> >to process it as a revert, with a short blurb in the changelog to note that RISC-V
> >is manually modified as RISC-V support came along in the interim.
> 
> commit b99040853738 adds opaque param to kvm_arch_hardware_setup(), which isn't
> reverted in this patch. I.e., this patch is a partial revert of b99040853738
> plus manual changes to RISC-V. Given that, "process it as a revert" means
> clearly say in changelog that this commit contains a partial revert of commit
> b99040853738 ("KVM: Pass kvm_init()'s opaque param to additional arch funcs").
> 
> Right?

What I meant is literally do

  git revert -s b99040853738

and then manually handle RISC-V.

Re: [PATCH 3/6] KVM: Remove opaque from kvm_arch_check_processor_compat

[Sean Christopherson]

On Wed, Jan 12, 2022, Sean Christopherson wrote:
> On Tue, Jan 11, 2022, Chao Gao wrote:
> > On Mon, Jan 10, 2022 at 11:06:44PM +0000, Sean Christopherson wrote:
> > >On Mon, Dec 27, 2021, Chao Gao wrote:
> > >> No arch implementation uses this opaque now.
> > >
> > >Except for the RISC-V part, this can be a pure revert of commit b99040853738 ("KVM:
> > >Pass kvm_init()'s opaque param to additional arch funcs").  I think it makes sense
> > >to process it as a revert, with a short blurb in the changelog to note that RISC-V
> > >is manually modified as RISC-V support came along in the interim.
> > 
> > commit b99040853738 adds opaque param to kvm_arch_hardware_setup(), which isn't
> > reverted in this patch. I.e., this patch is a partial revert of b99040853738
> > plus manual changes to RISC-V. Given that, "process it as a revert" means
> > clearly say in changelog that this commit contains a partial revert of commit
> > b99040853738 ("KVM: Pass kvm_init()'s opaque param to additional arch funcs").
> > 
> > Right?
> 
> What I meant is literally do
> 
>   git revert -s b99040853738
> 
> and then manually handle RISC-V.

Doh, to be clear, "manually handle RISC-V _in the same commit_".

[PATCH 4/6] KVM: Rename and move CPUHP_AP_KVM_STARTING to ONLINE section

[Chao Gao]

The CPU STARTING section doesn't allow callbacks to fail. Move KVM's
hotplug callback to ONLINE section so that it can abort onlining a CPU in
certain cases to avoid potentially breaking VMs running on existing CPUs.
For example, when kvm fails to enable hardware virtualization on the
hotplugged CPU.

Place KVM's hotplug state before CPUHP_AP_SCHED_WAIT_EMPTY as it ensures
when offlining a CPU, all user tasks and non-pinned kernel tasks have left
the CPU, i.e. there cannot be a vCPU task around. So, it is safe for KVM's
CPU offline callback to disable hardware virtualization at that point.
Likewise, KVM's online callback can enable hardware virtualization before
any vCPU task gets a chance to run on hotplugged CPUs.

KVM's CPU hotplug callbacks are renamed as well.

Suggested-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Chao Gao <chao.gao@intel.com>
---
 include/linux/cpuhotplug.h |  2 +-
 virt/kvm/kvm_main.c        | 28 ++++++++++++++++++++--------
 2 files changed, 21 insertions(+), 9 deletions(-)

diff --git a/include/linux/cpuhotplug.h b/include/linux/cpuhotplug.h
index 773c83730906..14d354c8ce35 100644
--- a/include/linux/cpuhotplug.h
+++ b/include/linux/cpuhotplug.h
@@ -182,7 +182,6 @@ enum cpuhp_state {
 	CPUHP_AP_CSKY_TIMER_STARTING,
 	CPUHP_AP_TI_GP_TIMER_STARTING,
 	CPUHP_AP_HYPERV_TIMER_STARTING,
-	CPUHP_AP_KVM_STARTING,
 	CPUHP_AP_KVM_ARM_VGIC_INIT_STARTING,
 	CPUHP_AP_KVM_ARM_VGIC_STARTING,
 	CPUHP_AP_KVM_ARM_TIMER_STARTING,
@@ -200,6 +199,7 @@ enum cpuhp_state {
 
 	/* Online section invoked on the hotplugged CPU from the hotplug thread */
 	CPUHP_AP_ONLINE_IDLE,
+	CPUHP_AP_KVM_ONLINE,
 	CPUHP_AP_SCHED_WAIT_EMPTY,
 	CPUHP_AP_SMPBOOT_THREADS,
 	CPUHP_AP_X86_VDSO_VMA_ONLINE,
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 4e1e7770e984..c1054604d1e8 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -4853,13 +4853,25 @@ static void hardware_enable_nolock(void *junk)
 	}
 }
 
-static int kvm_starting_cpu(unsigned int cpu)
+static int kvm_online_cpu(unsigned int cpu)
 {
+	int ret = 0;
+
 	raw_spin_lock(&kvm_count_lock);
-	if (kvm_usage_count)
+	/*
+	 * Abort the CPU online process if hardware virtualization cannot
+	 * be enabled. Otherwise running VMs would encounter unrecoverable
+	 * errors when scheduled to this CPU.
+	 */
+	if (kvm_usage_count) {
 		hardware_enable_nolock(NULL);
+		if (atomic_read(&hardware_enable_failed)) {
+			ret = -EIO;
+			pr_info("kvm: abort onlining CPU%d", cpu);
+		}
+	}
 	raw_spin_unlock(&kvm_count_lock);
-	return 0;
+	return ret;
 }
 
 static void hardware_disable_nolock(void *junk)
@@ -4872,7 +4884,7 @@ static void hardware_disable_nolock(void *junk)
 	kvm_arch_hardware_disable();
 }
 
-static int kvm_dying_cpu(unsigned int cpu)
+static int kvm_offline_cpu(unsigned int cpu)
 {
 	raw_spin_lock(&kvm_count_lock);
 	if (kvm_usage_count)
@@ -5641,8 +5653,8 @@ int kvm_init(void *opaque, unsigned vcpu_size, unsigned vcpu_align,
 			goto out_free_2;
 	}
 
-	r = cpuhp_setup_state_nocalls(CPUHP_AP_KVM_STARTING, "kvm/cpu:starting",
-				      kvm_starting_cpu, kvm_dying_cpu);
+	r = cpuhp_setup_state_nocalls(CPUHP_AP_KVM_ONLINE, "kvm/cpu:online",
+				      kvm_online_cpu, kvm_offline_cpu);
 	if (r)
 		goto out_free_2;
 	register_reboot_notifier(&kvm_reboot_notifier);
@@ -5705,7 +5717,7 @@ int kvm_init(void *opaque, unsigned vcpu_size, unsigned vcpu_align,
 	kmem_cache_destroy(kvm_vcpu_cache);
 out_free_3:
 	unregister_reboot_notifier(&kvm_reboot_notifier);
-	cpuhp_remove_state_nocalls(CPUHP_AP_KVM_STARTING);
+	cpuhp_remove_state_nocalls(CPUHP_AP_KVM_ONLINE);
 out_free_2:
 	kvm_arch_hardware_unsetup();
 out_free_1:
@@ -5731,7 +5743,7 @@ void kvm_exit(void)
 	kvm_async_pf_deinit();
 	unregister_syscore_ops(&kvm_syscore_ops);
 	unregister_reboot_notifier(&kvm_reboot_notifier);
-	cpuhp_remove_state_nocalls(CPUHP_AP_KVM_STARTING);
+	cpuhp_remove_state_nocalls(CPUHP_AP_KVM_ONLINE);
 	on_each_cpu(hardware_disable_nolock, NULL, 1);
 	kvm_arch_hardware_unsetup();
 	kvm_arch_exit();
-- 
2.25.1

[PATCH 5/6] KVM: x86: Remove WARN_ON in kvm_arch_check_processor_compat

[Chao Gao]

kvm_arch_check_processor_compat() needn't be called with interrupt
disabled, as it only reads some CRs/MSRs which won't be clobbered
by interrupt handlers or softirq.

What really needed is disabling preemption. No additional check is
added because if CONFIG_DEBUG_PREEMPT is enabled, smp_processor_id()
(right above the WARN_ON()) can help to detect any violation.

Signed-off-by: Chao Gao <chao.gao@intel.com>
---
 arch/x86/kvm/x86.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index aa09c8792134..a80e3b0c11a8 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -11384,8 +11384,6 @@ int kvm_arch_check_processor_compat(void)
 {
 	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;
-- 
2.25.1

Re: [PATCH 5/6] KVM: x86: Remove WARN_ON in kvm_arch_check_processor_compat

[Sean Christopherson]

On Mon, Dec 27, 2021, Chao Gao wrote:
> kvm_arch_check_processor_compat() needn't be called with interrupt
> disabled, as it only reads some CRs/MSRs which won't be clobbered
> by interrupt handlers or softirq.
> 
> What really needed is disabling preemption. No additional check is
> added because if CONFIG_DEBUG_PREEMPT is enabled, smp_processor_id()
> (right above the WARN_ON()) can help to detect any violation.

Hrm, IIRC, the assertion that IRQs are disabled was more about detecting improper
usage with respect to KVM doing hardware enabling than it was about ensuring the
current task isn't migrated.  E.g. as exhibited by patch 06, extra protections
(disabling of hotplug in that case) are needed if this helper is called outside
of the core KVM hardware enabling flow since hardware_enable_all() does its thing
via SMP function call.

Is there CPU onlining state/metadata that we could use to handle that specific case?
It'd be nice to preserve the paranoid check, but it's not a big deal if we can't.

If we can't preserve the WARN, can you rework the changelog to explain the motivation
for removing the WARN?

Thanks!

> Signed-off-by: Chao Gao <chao.gao@intel.com>
> ---
>  arch/x86/kvm/x86.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index aa09c8792134..a80e3b0c11a8 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -11384,8 +11384,6 @@ int kvm_arch_check_processor_compat(void)
>  {
>  	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;
> -- 
> 2.25.1
> 

RE: [PATCH 5/6] KVM: x86: Remove WARN_ON in kvm_arch_check_processor_compat

[Tian, Kevin]

> From: Sean Christopherson <seanjc@google.com>
> Sent: Tuesday, January 11, 2022 7:00 AM
> 
> On Mon, Dec 27, 2021, Chao Gao wrote:
> > kvm_arch_check_processor_compat() needn't be called with interrupt
> > disabled, as it only reads some CRs/MSRs which won't be clobbered
> > by interrupt handlers or softirq.
> >
> > What really needed is disabling preemption. No additional check is
> > added because if CONFIG_DEBUG_PREEMPT is enabled, smp_processor_id()
> > (right above the WARN_ON()) can help to detect any violation.
> 
> Hrm, IIRC, the assertion that IRQs are disabled was more about detecting
> improper
> usage with respect to KVM doing hardware enabling than it was about
> ensuring the
> current task isn't migrated.  E.g. as exhibited by patch 06, extra protections
> (disabling of hotplug in that case) are needed if this helper is called outside
> of the core KVM hardware enabling flow since hardware_enable_all() does
> its thing
> via SMP function call.

Looks the WARN_ON() was added by you. 😊

commit f1cdecf5807b1a91829a2dc4f254bfe6bafd4776
Author: Sean Christopherson <sean.j.christopherson@intel.com>
Date:   Tue Dec 10 14:44:14 2019 -0800

    KVM: x86: Ensure all logical CPUs have consistent reserved cr4 bits

    Check the current CPU's reserved cr4 bits against the mask calculated
    for the boot CPU to ensure consistent behavior across all CPUs.

    Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
    Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

But it's unclear to me how this WARN_ON() is related to what the commit
msg tries to explain. When I read this code it's more like a sanity check on
the assumption that it is currently called in SMP function call which runs 
the said function with interrupt disabled.

> 
> Is there CPU onlining state/metadata that we could use to handle that
> specific case?
> It'd be nice to preserve the paranoid check, but it's not a big deal if we can't.
> 
> If we can't preserve the WARN, can you rework the changelog to explain the
> motivation
> for removing the WARN?
> 
> Thanks!
> 
> > Signed-off-by: Chao Gao <chao.gao@intel.com>
> > ---
> >  arch/x86/kvm/x86.c | 2 --
> >  1 file changed, 2 deletions(-)
> >
> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > index aa09c8792134..a80e3b0c11a8 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -11384,8 +11384,6 @@ int kvm_arch_check_processor_compat(void)
> >  {
> >  	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;
> > --
> > 2.25.1
> >

Re: [PATCH 5/6] KVM: x86: Remove WARN_ON in kvm_arch_check_processor_compat

[Sean Christopherson]

On Tue, Jan 11, 2022, Tian, Kevin wrote:
> > From: Sean Christopherson <seanjc@google.com>
> > Sent: Tuesday, January 11, 2022 7:00 AM
> > 
> > On Mon, Dec 27, 2021, Chao Gao wrote:
> > > kvm_arch_check_processor_compat() needn't be called with interrupt
> > > disabled, as it only reads some CRs/MSRs which won't be clobbered
> > > by interrupt handlers or softirq.
> > >
> > > What really needed is disabling preemption. No additional check is
> > > added because if CONFIG_DEBUG_PREEMPT is enabled, smp_processor_id()
> > > (right above the WARN_ON()) can help to detect any violation.
> > 
> > Hrm, IIRC, the assertion that IRQs are disabled was more about detecting
> > improper usage with respect to KVM doing hardware enabling than it was
> > about ensuring the current task isn't migrated.  E.g. as exhibited by patch
> > 06, extra protections (disabling of hotplug in that case) are needed if
> > this helper is called outside of the core KVM hardware enabling flow since
> > hardware_enable_all() does its thing via SMP function call.
> 
> Looks the WARN_ON() was added by you. 😊

Yeah, past me owes current me a beer.

> commit f1cdecf5807b1a91829a2dc4f254bfe6bafd4776
> Author: Sean Christopherson <sean.j.christopherson@intel.com>
> Date:   Tue Dec 10 14:44:14 2019 -0800
> 
>     KVM: x86: Ensure all logical CPUs have consistent reserved cr4 bits
> 
>     Check the current CPU's reserved cr4 bits against the mask calculated
>     for the boot CPU to ensure consistent behavior across all CPUs.
> 
>     Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
>     Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> 
> But it's unclear to me how this WARN_ON() is related to what the commit
> msg tries to explain.

Ya, the changelog and lack of a comment is awful.

> When I read this code it's more like a sanity check on the assumption that it
> is currently called in SMP function call which runs the said function with
> interrupt disabled.

Yes, and as above, that assertion was more about the helper not really being safe
for general usage as opposed to wanting to detect use from preemptible context.
If we end up keeping the WARN_ON, I'll happily write a comment explaining the
point of the assertion.

Re: [PATCH 5/6] KVM: x86: Remove WARN_ON in kvm_arch_check_processor_compat

[Chao Gao]

On Tue, Jan 11, 2022 at 07:48:39PM +0000, Sean Christopherson wrote:
>On Tue, Jan 11, 2022, Tian, Kevin wrote:
>> > From: Sean Christopherson <seanjc@google.com>
>> > Sent: Tuesday, January 11, 2022 7:00 AM
>> > 
>> > On Mon, Dec 27, 2021, Chao Gao wrote:
>> > > kvm_arch_check_processor_compat() needn't be called with interrupt
>> > > disabled, as it only reads some CRs/MSRs which won't be clobbered
>> > > by interrupt handlers or softirq.
>> > >
>> > > What really needed is disabling preemption. No additional check is
>> > > added because if CONFIG_DEBUG_PREEMPT is enabled, smp_processor_id()
>> > > (right above the WARN_ON()) can help to detect any violation.
>> > 
>> > Hrm, IIRC, the assertion that IRQs are disabled was more about detecting
>> > improper usage with respect to KVM doing hardware enabling than it was
>> > about ensuring the current task isn't migrated.  E.g. as exhibited by patch
>> > 06, extra protections (disabling of hotplug in that case) are needed if
>> > this helper is called outside of the core KVM hardware enabling flow since
>> > hardware_enable_all() does its thing via SMP function call.
>> 
>> Looks the WARN_ON() was added by you. 😊
>
>Yeah, past me owes current me a beer.
>
>> commit f1cdecf5807b1a91829a2dc4f254bfe6bafd4776
>> Author: Sean Christopherson <sean.j.christopherson@intel.com>
>> Date:   Tue Dec 10 14:44:14 2019 -0800
>> 
>>     KVM: x86: Ensure all logical CPUs have consistent reserved cr4 bits
>> 
>>     Check the current CPU's reserved cr4 bits against the mask calculated
>>     for the boot CPU to ensure consistent behavior across all CPUs.
>> 
>>     Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
>>     Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> 
>> But it's unclear to me how this WARN_ON() is related to what the commit
>> msg tries to explain.
>
>Ya, the changelog and lack of a comment is awful.
>
>> When I read this code it's more like a sanity check on the assumption that it
>> is currently called in SMP function call which runs the said function with
>> interrupt disabled.
>
>Yes, and as above, that assertion was more about the helper not really being safe
>for general usage as opposed to wanting to detect use from preemptible context.
>If we end up keeping the WARN_ON, I'll happily write a comment explaining the
>point of the assertion.

OK. I will do following changes to keep the WARN_ON():
1. drop this patch
2. disable interrupt before the call site in patch 6.

Re: [PATCH 5/6] KVM: x86: Remove WARN_ON in kvm_arch_check_processor_compat

[Sean Christopherson]

On Wed, Jan 12, 2022, Chao Gao wrote:
> On Tue, Jan 11, 2022 at 07:48:39PM +0000, Sean Christopherson wrote:
> >On Tue, Jan 11, 2022, Tian, Kevin wrote:
> >> > From: Sean Christopherson <seanjc@google.com>
> >> > Sent: Tuesday, January 11, 2022 7:00 AM
> >> > 
> >> > On Mon, Dec 27, 2021, Chao Gao wrote:
> >> > > kvm_arch_check_processor_compat() needn't be called with interrupt
> >> > > disabled, as it only reads some CRs/MSRs which won't be clobbered
> >> > > by interrupt handlers or softirq.
> >> > >
> >> > > What really needed is disabling preemption. No additional check is
> >> > > added because if CONFIG_DEBUG_PREEMPT is enabled, smp_processor_id()
> >> > > (right above the WARN_ON()) can help to detect any violation.
> >> > 
> >> > Hrm, IIRC, the assertion that IRQs are disabled was more about detecting
> >> > improper usage with respect to KVM doing hardware enabling than it was
> >> > about ensuring the current task isn't migrated.  E.g. as exhibited by patch
> >> > 06, extra protections (disabling of hotplug in that case) are needed if
> >> > this helper is called outside of the core KVM hardware enabling flow since
> >> > hardware_enable_all() does its thing via SMP function call.
> >> 
> >> Looks the WARN_ON() was added by you. 😊
> >
> >Yeah, past me owes current me a beer.
> >
> >> commit f1cdecf5807b1a91829a2dc4f254bfe6bafd4776
> >> Author: Sean Christopherson <sean.j.christopherson@intel.com>
> >> Date:   Tue Dec 10 14:44:14 2019 -0800
> >> 
> >>     KVM: x86: Ensure all logical CPUs have consistent reserved cr4 bits
> >> 
> >>     Check the current CPU's reserved cr4 bits against the mask calculated
> >>     for the boot CPU to ensure consistent behavior across all CPUs.
> >> 
> >>     Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> >>     Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> >> 
> >> But it's unclear to me how this WARN_ON() is related to what the commit
> >> msg tries to explain.
> >
> >Ya, the changelog and lack of a comment is awful.
> >
> >> When I read this code it's more like a sanity check on the assumption that it
> >> is currently called in SMP function call which runs the said function with
> >> interrupt disabled.
> >
> >Yes, and as above, that assertion was more about the helper not really being safe
> >for general usage as opposed to wanting to detect use from preemptible context.
> >If we end up keeping the WARN_ON, I'll happily write a comment explaining the
> >point of the assertion.
> 
> OK. I will do following changes to keep the WARN_ON():
> 1. drop this patch
> 2. disable interrupt before the call site in patch 6.

No, we shouldn't sully other code just to keep this WARN.  Again, the point of
the WARN is/was to highlight that any use outside of the hardware enabling path
is suspect.  That's why I asked if there was a way this code could identify that
the CPU in question is being hotplugged, i.e. to convey that the helper is safe
to use only during hardware enabling _or_ hotplug.  If that's not feasible,
replacing the WARN with a scary comment is better than disabling IRQs.

Re: [PATCH 5/6] KVM: x86: Remove WARN_ON in kvm_arch_check_processor_compat

[Chao Gao]

On Wed, Jan 12, 2022 at 05:35:12PM +0000, Sean Christopherson wrote:
>On Wed, Jan 12, 2022, Chao Gao wrote:
>> On Tue, Jan 11, 2022 at 07:48:39PM +0000, Sean Christopherson wrote:
>> >On Tue, Jan 11, 2022, Tian, Kevin wrote:
>> >> > From: Sean Christopherson <seanjc@google.com>
>> >> > Sent: Tuesday, January 11, 2022 7:00 AM
>> >> > 
>> >> > On Mon, Dec 27, 2021, Chao Gao wrote:
>> >> > > kvm_arch_check_processor_compat() needn't be called with interrupt
>> >> > > disabled, as it only reads some CRs/MSRs which won't be clobbered
>> >> > > by interrupt handlers or softirq.
>> >> > >
>> >> > > What really needed is disabling preemption. No additional check is
>> >> > > added because if CONFIG_DEBUG_PREEMPT is enabled, smp_processor_id()
>> >> > > (right above the WARN_ON()) can help to detect any violation.
>> >> > 
>> >> > Hrm, IIRC, the assertion that IRQs are disabled was more about detecting
>> >> > improper usage with respect to KVM doing hardware enabling than it was
>> >> > about ensuring the current task isn't migrated.  E.g. as exhibited by patch
>> >> > 06, extra protections (disabling of hotplug in that case) are needed if
>> >> > this helper is called outside of the core KVM hardware enabling flow since
>> >> > hardware_enable_all() does its thing via SMP function call.
>> >> 
>> >> Looks the WARN_ON() was added by you. 😊
>> >
>> >Yeah, past me owes current me a beer.
>> >
>> >> commit f1cdecf5807b1a91829a2dc4f254bfe6bafd4776
>> >> Author: Sean Christopherson <sean.j.christopherson@intel.com>
>> >> Date:   Tue Dec 10 14:44:14 2019 -0800
>> >> 
>> >>     KVM: x86: Ensure all logical CPUs have consistent reserved cr4 bits
>> >> 
>> >>     Check the current CPU's reserved cr4 bits against the mask calculated
>> >>     for the boot CPU to ensure consistent behavior across all CPUs.
>> >> 
>> >>     Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
>> >>     Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> >> 
>> >> But it's unclear to me how this WARN_ON() is related to what the commit
>> >> msg tries to explain.
>> >
>> >Ya, the changelog and lack of a comment is awful.
>> >
>> >> When I read this code it's more like a sanity check on the assumption that it
>> >> is currently called in SMP function call which runs the said function with
>> >> interrupt disabled.
>> >
>> >Yes, and as above, that assertion was more about the helper not really being safe
>> >for general usage as opposed to wanting to detect use from preemptible context.
>> >If we end up keeping the WARN_ON, I'll happily write a comment explaining the
>> >point of the assertion.
>> 
>> OK. I will do following changes to keep the WARN_ON():
>> 1. drop this patch
>> 2. disable interrupt before the call site in patch 6.
>
>No, we shouldn't sully other code just to keep this WARN.  Again, the point of
>the WARN is/was to highlight that any use outside of the hardware enabling path
>is suspect.  That's why I asked if there was a way this code could identify that
>the CPU in question is being hotplugged, i.e. to convey that the helper is safe
>to use only during hardware enabling _or_ hotplug.  If that's not feasible,
>replacing the WARN with a scary comment is better than disabling IRQs.

OK. How about:

	/*
	 * Compatibility checks are done when loading KVM or in KVM's CPU
	 * hotplug callback. It ensures all online CPUs are compatible before
	 * running any vCPUs. For other cases, compatibility checks are
	 * unnecessary or even problematic. Try to detect improper usages here.
	 */
	WARN_ON(!irqs_disabled() && !cpu_active(smp_processor_id()));

a CPU is active when it reaches the CPUHP_AP_ACTIVE state (the last state before
CPUHP_ONLINE). So, if a cpu isn't active, it probably is being hotplugged. One
false positive is the CPU is dying, which I guess is fine.

And to help justify this change, I will merge it into patch 6.

Re: [PATCH 5/6] KVM: x86: Remove WARN_ON in kvm_arch_check_processor_compat

[Chao Gao]

On Mon, Jan 17, 2022 at 09:35:04PM +0800, Chao Gao wrote:
>On Wed, Jan 12, 2022 at 05:35:12PM +0000, Sean Christopherson wrote:
>>On Wed, Jan 12, 2022, Chao Gao wrote:
>>> On Tue, Jan 11, 2022 at 07:48:39PM +0000, Sean Christopherson wrote:
>>> >On Tue, Jan 11, 2022, Tian, Kevin wrote:
>>> >> > From: Sean Christopherson <seanjc@google.com>
>>> >> > Sent: Tuesday, January 11, 2022 7:00 AM
>>> >> > 
>>> >> > On Mon, Dec 27, 2021, Chao Gao wrote:
>>> >> > > kvm_arch_check_processor_compat() needn't be called with interrupt
>>> >> > > disabled, as it only reads some CRs/MSRs which won't be clobbered
>>> >> > > by interrupt handlers or softirq.
>>> >> > >
>>> >> > > What really needed is disabling preemption. No additional check is
>>> >> > > added because if CONFIG_DEBUG_PREEMPT is enabled, smp_processor_id()
>>> >> > > (right above the WARN_ON()) can help to detect any violation.
>>> >> > 
>>> >> > Hrm, IIRC, the assertion that IRQs are disabled was more about detecting
>>> >> > improper usage with respect to KVM doing hardware enabling than it was
>>> >> > about ensuring the current task isn't migrated.  E.g. as exhibited by patch
>>> >> > 06, extra protections (disabling of hotplug in that case) are needed if
>>> >> > this helper is called outside of the core KVM hardware enabling flow since
>>> >> > hardware_enable_all() does its thing via SMP function call.
>>> >> 
>>> >> Looks the WARN_ON() was added by you. 😊
>>> >
>>> >Yeah, past me owes current me a beer.
>>> >
>>> >> commit f1cdecf5807b1a91829a2dc4f254bfe6bafd4776
>>> >> Author: Sean Christopherson <sean.j.christopherson@intel.com>
>>> >> Date:   Tue Dec 10 14:44:14 2019 -0800
>>> >> 
>>> >>     KVM: x86: Ensure all logical CPUs have consistent reserved cr4 bits
>>> >> 
>>> >>     Check the current CPU's reserved cr4 bits against the mask calculated
>>> >>     for the boot CPU to ensure consistent behavior across all CPUs.
>>> >> 
>>> >>     Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
>>> >>     Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>>> >> 
>>> >> But it's unclear to me how this WARN_ON() is related to what the commit
>>> >> msg tries to explain.
>>> >
>>> >Ya, the changelog and lack of a comment is awful.
>>> >
>>> >> When I read this code it's more like a sanity check on the assumption that it
>>> >> is currently called in SMP function call which runs the said function with
>>> >> interrupt disabled.
>>> >
>>> >Yes, and as above, that assertion was more about the helper not really being safe
>>> >for general usage as opposed to wanting to detect use from preemptible context.
>>> >If we end up keeping the WARN_ON, I'll happily write a comment explaining the
>>> >point of the assertion.
>>> 
>>> OK. I will do following changes to keep the WARN_ON():
>>> 1. drop this patch
>>> 2. disable interrupt before the call site in patch 6.
>>
>>No, we shouldn't sully other code just to keep this WARN.  Again, the point of
>>the WARN is/was to highlight that any use outside of the hardware enabling path
>>is suspect.  That's why I asked if there was a way this code could identify that
>>the CPU in question is being hotplugged, i.e. to convey that the helper is safe
>>to use only during hardware enabling _or_ hotplug.  If that's not feasible,
>>replacing the WARN with a scary comment is better than disabling IRQs.
>
>OK. How about:
>
>	/*
>	 * Compatibility checks are done when loading KVM or in KVM's CPU
>	 * hotplug callback. It ensures all online CPUs are compatible before
>	 * running any vCPUs. For other cases, compatibility checks are
>	 * unnecessary or even problematic. Try to detect improper usages here.
>	 */
>	WARN_ON(!irqs_disabled() && !cpu_active(smp_processor_id()));

Sorry. It should be:
	WARN_ON(!irqs_disabled() && cpu_active(smp_processor_id()));

>
>a CPU is active when it reaches the CPUHP_AP_ACTIVE state (the last state before
>CPUHP_ONLINE). So, if a cpu isn't active, it probably is being hotplugged. One
>false positive is the CPU is dying, which I guess is fine.
>
>And to help justify this change, I will merge it into patch 6.

Re: [PATCH 5/6] KVM: x86: Remove WARN_ON in kvm_arch_check_processor_compat

[Sean Christopherson]

On Mon, Jan 17, 2022, Chao Gao wrote:
> On Mon, Jan 17, 2022 at 09:35:04PM +0800, Chao Gao wrote:
> >OK. How about:
> >
> >	/*
> >	 * Compatibility checks are done when loading KVM or in KVM's CPU
> >	 * hotplug callback. It ensures all online CPUs are compatible before
> >	 * running any vCPUs. For other cases, compatibility checks are
> >	 * unnecessary or even problematic. Try to detect improper usages here.
> >	 */
> >	WARN_ON(!irqs_disabled() && !cpu_active(smp_processor_id()));
> 
> Sorry. It should be:
> 	WARN_ON(!irqs_disabled() && cpu_active(smp_processor_id()));

Nice!  That's exactly what I was hoping we could do.

[PATCH 6/6] KVM: Do compatibility checks on hotplugged CPUs

[Chao Gao]

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
vmentry failure if the hotplugged CPU doesn't meet minimal feature
requirements.

Do compatibility checks when onlining a CPU. If any VM is running,
KVM hotplug callback returns an error to abort onlining incompatible
CPUs.

But if no VM is running, onlining incompatible CPUs is allowed. Instead,
KVM is prohibited from creating VMs similar to the policy for init-time
compatibility checks.

CPU hotplug is disabled during hardware_enable_all() to prevent the corner
case as shown below. A hotplugged CPU marks itself online in
cpu_online_mask (1) and enables interrupt (2) before invoking callbacks
registered in ONLINE section (3). So, if hardware_enable_all() is invoked
on another CPU right after (2), then on_each_cpu() in hardware_enable_all()
invokes hardware_enable_nolock() on the hotplugged CPU before
kvm_online_cpu() is called. This makes the CPU escape from compatibility
checks, which is risky.

	start_secondary { ...
		set_cpu_online(smp_processor_id(), true); <- 1
		...
		local_irq_enable();  <- 2
		...
		cpu_startup_entry(CPUHP_AP_ONLINE_IDLE); <- 3
	}

Keep compatibility checks at KVM init time. It can help to find
incompatibility issues earlier and refuse to load arch KVM module
(e.g., kvm-intel).

Signed-off-by: Chao Gao <chao.gao@intel.com>
---
 virt/kvm/kvm_main.c | 36 ++++++++++++++++++++++++++++++++++--
 1 file changed, 34 insertions(+), 2 deletions(-)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index c1054604d1e8..0ff80076d48d 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -106,6 +106,8 @@ LIST_HEAD(vm_list);
 static cpumask_var_t cpus_hardware_enabled;
 static int kvm_usage_count;
 static atomic_t hardware_enable_failed;
+/* Set if hardware becomes incompatible after CPU hotplug */
+static bool hardware_incompatible;
 
 static struct kmem_cache *kvm_vcpu_cache;
 
@@ -4855,20 +4857,32 @@ static void hardware_enable_nolock(void *junk)
 
 static int kvm_online_cpu(unsigned int cpu)
 {
-	int ret = 0;
+	int ret;
 
+	ret = kvm_arch_check_processor_compat();
 	raw_spin_lock(&kvm_count_lock);
 	/*
 	 * Abort the CPU online process if hardware virtualization cannot
 	 * be enabled. Otherwise running VMs would encounter unrecoverable
 	 * errors when scheduled to this CPU.
 	 */
-	if (kvm_usage_count) {
+	if (!ret && kvm_usage_count) {
 		hardware_enable_nolock(NULL);
 		if (atomic_read(&hardware_enable_failed)) {
 			ret = -EIO;
 			pr_info("kvm: abort onlining CPU%d", cpu);
 		}
+	} else if (ret && !kvm_usage_count) {
+		/*
+		 * Continue onlining an incompatible CPU if no VM is
+		 * running. KVM should reject creating any VM after this
+		 * point. Then this CPU can be still used to run non-VM
+		 * workload.
+		 */
+		ret = 0;
+		hardware_incompatible = true;
+		pr_info("kvm: prohibit VM creation due to incompatible CPU%d",
+			cpu);
 	}
 	raw_spin_unlock(&kvm_count_lock);
 	return ret;
@@ -4913,8 +4927,24 @@ static int hardware_enable_all(void)
 {
 	int r = 0;
 
+	/*
+	 * 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);
 
+	if (hardware_incompatible) {
+		r = -EIO;
+		goto unlock;
+	}
+
 	kvm_usage_count++;
 	if (kvm_usage_count == 1) {
 		atomic_set(&hardware_enable_failed, 0);
@@ -4926,7 +4956,9 @@ static int hardware_enable_all(void)
 		}
 	}
 
+unlock:
 	raw_spin_unlock(&kvm_count_lock);
+	cpus_read_unlock();
 
 	return r;
 }
-- 
2.25.1

Re: [PATCH 6/6] KVM: Do compatibility checks on hotplugged CPUs

[Sean Christopherson]

On Mon, Dec 27, 2021, Chao Gao wrote:
> 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
> vmentry failure if the hotplugged CPU doesn't meet minimal feature
> requirements.
> 
> Do compatibility checks when onlining a CPU. If any VM is running,
> KVM hotplug callback returns an error to abort onlining incompatible
> CPUs.
> 
> But if no VM is running, onlining incompatible CPUs is allowed. Instead,
> KVM is prohibited from creating VMs similar to the policy for init-time
> compatibility checks.

...

> ---
>  virt/kvm/kvm_main.c | 36 ++++++++++++++++++++++++++++++++++--
>  1 file changed, 34 insertions(+), 2 deletions(-)
> 
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index c1054604d1e8..0ff80076d48d 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -106,6 +106,8 @@ LIST_HEAD(vm_list);
>  static cpumask_var_t cpus_hardware_enabled;
>  static int kvm_usage_count;
>  static atomic_t hardware_enable_failed;
> +/* Set if hardware becomes incompatible after CPU hotplug */
> +static bool hardware_incompatible;
>  
>  static struct kmem_cache *kvm_vcpu_cache;
>  
> @@ -4855,20 +4857,32 @@ static void hardware_enable_nolock(void *junk)
>  
>  static int kvm_online_cpu(unsigned int cpu)
>  {
> -	int ret = 0;
> +	int ret;
>  
> +	ret = kvm_arch_check_processor_compat();
>  	raw_spin_lock(&kvm_count_lock);
>  	/*
>  	 * Abort the CPU online process if hardware virtualization cannot
>  	 * be enabled. Otherwise running VMs would encounter unrecoverable
>  	 * errors when scheduled to this CPU.
>  	 */
> -	if (kvm_usage_count) {
> +	if (!ret && kvm_usage_count) {
>  		hardware_enable_nolock(NULL);
>  		if (atomic_read(&hardware_enable_failed)) {
>  			ret = -EIO;
>  			pr_info("kvm: abort onlining CPU%d", cpu);
>  		}
> +	} else if (ret && !kvm_usage_count) {
> +		/*
> +		 * Continue onlining an incompatible CPU if no VM is
> +		 * running. KVM should reject creating any VM after this
> +		 * point. Then this CPU can be still used to run non-VM
> +		 * workload.
> +		 */
> +		ret = 0;
> +		hardware_incompatible = true;

This has a fairly big flaw in that it prevents KVM from creating VMs even if the
offending CPU is offlined.  That seems like a very reasonable thing to do, e.g.
admin sees that hotplugging a CPU broke KVM and removes the CPU to remedy the
problem.  And if KVM is built-in, reloading KVM to wipe hardware_incompatible
after offlining the CPU isn't an option.

To make this approach work, I think kvm_offline_cpu() would have to reevaluate
hardware_incompatible if the flag is set.

And should there be a KVM module param to let the admin opt in/out of this
behavior?  E.g. if the primary use case for a system is to run VMs, disabling
KVM just to online a CPU isn't very helpful.

That said, I'm not convinced that continuing with the hotplug in this scenario
is ever the right thing to do.  Either the CPU being hotplugged really is a different
CPU, or it's literally broken.  In both cases, odds are very, very good that running
on the dodgy CPU will hose the kernel sooner or later, i.e. KVM's compatibility checks
are just the canary in the coal mine.

TDX is a different beast as (a) that's purely a security restriction and (b) anyone
trying to run TDX guests darn well better know that TDX doesn't allow hotplug.
In other words, if TDX gets disabled due to hotplug, either someone majorly screwed
up and is going to be unhappy no matter what, or there's no intention of using TDX
and it's a complete don't care.

> +		pr_info("kvm: prohibit VM creation due to incompatible CPU%d",

pr_info() is a bit weak, this should be at least pr_warn() and maybe even pr_err().

> +			cpu);

Eh, I'd omit the newline and let that poke out.

>  	}
>  	raw_spin_unlock(&kvm_count_lock);
>  	return ret;
> @@ -4913,8 +4927,24 @@ static int hardware_enable_all(void)
>  {
>  	int r = 0;
>  
> +	/*
> +	 * 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);
>  
> +	if (hardware_incompatible) {

Another error message would likely be helpful here.  Even better would be if KVM
could provide some way for userspace to query which CPU(s) is bad.

> +		r = -EIO;
> +		goto unlock;
> +	}
> +
>  	kvm_usage_count++;
>  	if (kvm_usage_count == 1) {
>  		atomic_set(&hardware_enable_failed, 0);
> @@ -4926,7 +4956,9 @@ static int hardware_enable_all(void)
>  		}
>  	}
>  
> +unlock:
>  	raw_spin_unlock(&kvm_count_lock);
> +	cpus_read_unlock();
>  
>  	return r;
>  }
> -- 
> 2.25.1
> 

Re: [PATCH 6/6] KVM: Do compatibility checks on hotplugged CPUs

[Chao Gao]

On Tue, Jan 11, 2022 at 12:46:52AM +0000, Sean Christopherson wrote:
>On Mon, Dec 27, 2021, Chao Gao wrote:
>> 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
>> vmentry failure if the hotplugged CPU doesn't meet minimal feature
>> requirements.
>> 
>> Do compatibility checks when onlining a CPU. If any VM is running,
>> KVM hotplug callback returns an error to abort onlining incompatible
>> CPUs.
>> 
>> But if no VM is running, onlining incompatible CPUs is allowed. Instead,
>> KVM is prohibited from creating VMs similar to the policy for init-time
>> compatibility checks.
>
>...
>
>> ---
>>  virt/kvm/kvm_main.c | 36 ++++++++++++++++++++++++++++++++++--
>>  1 file changed, 34 insertions(+), 2 deletions(-)
>> 
>> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
>> index c1054604d1e8..0ff80076d48d 100644
>> --- a/virt/kvm/kvm_main.c
>> +++ b/virt/kvm/kvm_main.c
>> @@ -106,6 +106,8 @@ LIST_HEAD(vm_list);
>>  static cpumask_var_t cpus_hardware_enabled;
>>  static int kvm_usage_count;
>>  static atomic_t hardware_enable_failed;
>> +/* Set if hardware becomes incompatible after CPU hotplug */
>> +static bool hardware_incompatible;
>>  
>>  static struct kmem_cache *kvm_vcpu_cache;
>>  
>> @@ -4855,20 +4857,32 @@ static void hardware_enable_nolock(void *junk)
>>  
>>  static int kvm_online_cpu(unsigned int cpu)
>>  {
>> -	int ret = 0;
>> +	int ret;
>>  
>> +	ret = kvm_arch_check_processor_compat();
>>  	raw_spin_lock(&kvm_count_lock);
>>  	/*
>>  	 * Abort the CPU online process if hardware virtualization cannot
>>  	 * be enabled. Otherwise running VMs would encounter unrecoverable
>>  	 * errors when scheduled to this CPU.
>>  	 */
>> -	if (kvm_usage_count) {
>> +	if (!ret && kvm_usage_count) {
>>  		hardware_enable_nolock(NULL);
>>  		if (atomic_read(&hardware_enable_failed)) {
>>  			ret = -EIO;
>>  			pr_info("kvm: abort onlining CPU%d", cpu);
>>  		}
>> +	} else if (ret && !kvm_usage_count) {
>> +		/*
>> +		 * Continue onlining an incompatible CPU if no VM is
>> +		 * running. KVM should reject creating any VM after this
>> +		 * point. Then this CPU can be still used to run non-VM
>> +		 * workload.
>> +		 */
>> +		ret = 0;
>> +		hardware_incompatible = true;
>
>This has a fairly big flaw in that it prevents KVM from creating VMs even if the
>offending CPU is offlined.  That seems like a very reasonable thing to do, e.g.
>admin sees that hotplugging a CPU broke KVM and removes the CPU to remedy the
>problem.  And if KVM is built-in, reloading KVM to wipe hardware_incompatible
>after offlining the CPU isn't an option.

Ideally, yes, creation VMs should be allowed after offending CPUs are offlined.
But the problem is kind of foundamental: 

After kernel tries to online a CPU without VMX, boot_cpu_has(X86_FEATURE_VMX)
returns false. So, the current behavior is reloading KVM would fail if
kernel *tried* to bring up a CPU without VMX. So, it looks to me that
boot_cpu_has() doesn't do feature re-evalution either. Given that, I doubt
the value of making KVM able to create VM in this case.

>
>To make this approach work, I think kvm_offline_cpu() would have to reevaluate
>hardware_incompatible if the flag is set.
>
>And should there be a KVM module param to let the admin opt in/out of this
>behavior?  E.g. if the primary use case for a system is to run VMs, disabling
>KVM just to online a CPU isn't very helpful.
>
>That said, I'm not convinced that continuing with the hotplug in this scenario
>is ever the right thing to do.  Either the CPU being hotplugged really is a different
>CPU, or it's literally broken.  In both cases, odds are very, very good that running
>on the dodgy CPU will hose the kernel sooner or later, i.e. KVM's compatibility checks
>are just the canary in the coal mine.

Ok. Then here are two options:
1. KVM always prevents incompatible CPUs from being brought up regardless of running VMs
2. make "disabling KVM on incompatible CPUs" an opt-in feature.

Which one do you think is better?

And as said above, even with option 1, KVM reloading would fail due to
boot_cpu_has(X86_FEATURE_VMX). I suppose it isn't necessary to be fixed in this series.

>
>TDX is a different beast as (a) that's purely a security restriction and (b) anyone
>trying to run TDX guests darn well better know that TDX doesn't allow hotplug.
>In other words, if TDX gets disabled due to hotplug, either someone majorly screwed
>up and is going to be unhappy no matter what, or there's no intention of using TDX
>and it's a complete don't care.
>
>> +		pr_info("kvm: prohibit VM creation due to incompatible CPU%d",
>
>pr_info() is a bit weak, this should be at least pr_warn() and maybe even pr_err().
>
>> +			cpu);
>
>Eh, I'd omit the newline and let that poke out.

Will do.

>
>>  	}
>>  	raw_spin_unlock(&kvm_count_lock);
>>  	return ret;
>> @@ -4913,8 +4927,24 @@ static int hardware_enable_all(void)
>>  {
>>  	int r = 0;
>>  
>> +	/*
>> +	 * 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);
>>  
>> +	if (hardware_incompatible) {
>
>Another error message would likely be helpful here.  Even better would be if KVM
>could provide some way for userspace to query which CPU(s) is bad.

If option 1 is chosen, this check will be removed.

For option 2, will add an error message. And how about a debugfs tunable to provide
the list of bad CPUs?

Re: [PATCH 6/6] KVM: Do compatibility checks on hotplugged CPUs

[Sean Christopherson]

On Tue, Jan 11, 2022, Chao Gao wrote:
> On Tue, Jan 11, 2022 at 12:46:52AM +0000, Sean Christopherson wrote:
> >This has a fairly big flaw in that it prevents KVM from creating VMs even if the
> >offending CPU is offlined.  That seems like a very reasonable thing to do, e.g.
> >admin sees that hotplugging a CPU broke KVM and removes the CPU to remedy the
> >problem.  And if KVM is built-in, reloading KVM to wipe hardware_incompatible
> >after offlining the CPU isn't an option.

...

> >That said, I'm not convinced that continuing with the hotplug in this scenario
> >is ever the right thing to do.  Either the CPU being hotplugged really is a different
> >CPU, or it's literally broken.  In both cases, odds are very, very good that running
> >on the dodgy CPU will hose the kernel sooner or later, i.e. KVM's compatibility checks
> >are just the canary in the coal mine.
> 
> Ok. Then here are two options:
> 1. KVM always prevents incompatible CPUs from being brought up regardless of running VMs
> 2. make "disabling KVM on incompatible CPUs" an opt-in feature.
> 
> Which one do you think is better?

IMO, #1.  It's simpler to implement and document, and is less likely to surprise
the user.  We can always pivot to #2 _if_ anyone requests the ability to dynamically
disable KVM in order to bring up heterogenous CPUs and has a reasonable, sane use
case for doing so.  But that's a big "if" as I would be very surprised if it's even
possible to encounter such a setup without a hardware bug, firmware bug, and/or user
error.

Re: [PATCH 6/6] KVM: Do compatibility checks on hotplugged CPUs

[Jim Mattson]

On Wed, Jan 12, 2022 at 9:54 AM Sean Christopherson <seanjc@google.com> wrote:
>
> On Tue, Jan 11, 2022, Chao Gao wrote:
> > On Tue, Jan 11, 2022 at 12:46:52AM +0000, Sean Christopherson wrote:
> > >This has a fairly big flaw in that it prevents KVM from creating VMs even if the
> > >offending CPU is offlined.  That seems like a very reasonable thing to do, e.g.
> > >admin sees that hotplugging a CPU broke KVM and removes the CPU to remedy the
> > >problem.  And if KVM is built-in, reloading KVM to wipe hardware_incompatible
> > >after offlining the CPU isn't an option.
>
> ...
>
> > >That said, I'm not convinced that continuing with the hotplug in this scenario
> > >is ever the right thing to do.  Either the CPU being hotplugged really is a different
> > >CPU, or it's literally broken.  In both cases, odds are very, very good that running
> > >on the dodgy CPU will hose the kernel sooner or later, i.e. KVM's compatibility checks
> > >are just the canary in the coal mine.
> >
> > Ok. Then here are two options:
> > 1. KVM always prevents incompatible CPUs from being brought up regardless of running VMs
> > 2. make "disabling KVM on incompatible CPUs" an opt-in feature.
> >
> > Which one do you think is better?
>
> IMO, #1.  It's simpler to implement and document, and is less likely to surprise
> the user.  We can always pivot to #2 _if_ anyone requests the ability to dynamically
> disable KVM in order to bring up heterogenous CPUs and has a reasonable, sane use
> case for doing so.  But that's a big "if" as I would be very surprised if it's even
> possible to encounter such a setup without a hardware bug, firmware bug, and/or user
> error.

How quickly we forget the Woodcrest B/G fiasco.