Re: [PATCH v2 40/50] KVM: x86: Do compatibility checks when onlining CPU

[Huang, Kai]

On Wed, 2022-11-30 at 23:09 +0000, Sean Christopherson wrote:
> From: Chao Gao <chao.gao@xxxxxxxxx>
> 
> Do compatibility checks when enabling hardware to effectively add
> compatibility checks when onlining a CPU.  Abort enabling, i.e. the
> online process, if the (hotplugged) CPU is incompatible with the known
> good setup.
> 
> At init time, KVM does compatibility checks to ensure that all online
> CPUs support hardware virtualization and a common set of features. But
> KVM uses hotplugged CPUs without such compatibility checks. On Intel
> CPUs, this leads to #GP if the hotplugged CPU doesn't support VMX, or
> VM-Entry failure if the hotplugged CPU doesn't support all features
> enabled by KVM.
> 
> Note, this is little more than a NOP on SVM, as SVM already checks for
> full SVM support during hardware enabling.
> 
> Opportunistically add a pr_err() if setup_vmcs_config() fails, and
> tweak all error messages to output which CPU failed.
> 
> Signed-off-by: Chao Gao <chao.gao@xxxxxxxxx>
> Co-developed-by: Sean Christopherson <seanjc@xxxxxxxxxx>
> Signed-off-by: Sean Christopherson <seanjc@xxxxxxxxxx>

For VMX:

Acked-by: Kai Huang <kai.huang@xxxxxxxxx>

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