Re: [PATCH 1/2] KVM: x86: Add vendor name to kvm_x86_ops, use it for error messages

[Vitaly Kuznetsov <vkuznets@redhat.com>]

Sean Christopherson <seanjc@google.com> writes:

> Paul pointed out the error messages when KVM fails to load are unhelpful
> in understanding exactly what went wrong if userspace probes the "wrong"
> module.
>
> Add a mandatory kvm_x86_ops field to track vendor module names, kvm_intel
> and kvm_amd, and use the name for relevant error message when KVM fails
> to load so that the user knows which module failed to load.
>
> Opportunistically tweak the "disabled by bios" error message to clarify
> that _support_ was disabled, not that the module itself was magically
> disabled by BIOS.
>

...

> Suggested-by: Paul Menzel <pmenzel@molgen.mpg.de>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  arch/x86/include/asm/kvm_host.h | 2 ++
>  arch/x86/kvm/svm/svm.c          | 2 ++
>  arch/x86/kvm/vmx/vmx.c          | 2 ++
>  arch/x86/kvm/x86.c              | 8 +++++---
>  4 files changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 80f4b8a9233c..b05bfcc72042 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -1302,6 +1302,8 @@ static inline u16 kvm_lapic_irq_dest_mode(bool dest_mode_logical)
>  }
>  
>  struct kvm_x86_ops {
> +	const char *name;
> +
>  	int (*hardware_enable)(void);
>  	void (*hardware_disable)(void);
>  	void (*hardware_unsetup)(void);
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index 89077160d463..cee4915d2ce3 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -4580,6 +4580,8 @@ static int svm_vm_init(struct kvm *kvm)
>  }
>  
>  static struct kvm_x86_ops svm_x86_ops __initdata = {
> +	.name = "kvm_amd",
> +
>  	.hardware_unsetup = svm_hardware_teardown,
>  	.hardware_enable = svm_hardware_enable,
>  	.hardware_disable = svm_hardware_disable,
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 1c8b2b6e7ed9..c147438eaafc 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -7568,6 +7568,8 @@ static bool vmx_check_apicv_inhibit_reasons(ulong bit)
>  }
>  
>  static struct kvm_x86_ops vmx_x86_ops __initdata = {
> +	.name = "kvm_intel",
> +
>  	.hardware_unsetup = hardware_unsetup,
>  
>  	.hardware_enable = hardware_enable,
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index c59b63c56af9..e966e9cdd805 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -8539,18 +8539,20 @@ int kvm_arch_init(void *opaque)
>  	int r;
>  
>  	if (kvm_x86_ops.hardware_enable) {
> -		printk(KERN_ERR "kvm: already loaded the other module\n");
> +		pr_err("kvm: already loaded vendor module '%s'\n", kvm_x86_ops.name);
>  		r = -EEXIST;
>  		goto out;
>  	}
>  
>  	if (!ops->cpu_has_kvm_support()) {
> -		pr_err_ratelimited("kvm: no hardware support\n");
> +		pr_err_ratelimited("kvm: no hardware support for '%s'\n",
> +				   ops->runtime_ops->name);
>  		r = -EOPNOTSUPP;
>  		goto out;
>  	}
>  	if (ops->disabled_by_bios()) {
> -		pr_err_ratelimited("kvm: disabled by bios\n");
> +		pr_err_ratelimited("kvm: support for '%s' disabled by bios\n",
> +				   ops->runtime_ops->name);


I'd suggest we change this to 

		pr_err_ratelimited("kvm: %s: virtualization disabled in BIOS\n",
				   ops->runtime_ops->name);

or something like that as generally, it makes little sense to search for
'KVM' in BIOS settings. You need too look for either 'Virtualization' or
VT-x/AMD-v.

>  		r = -EOPNOTSUPP;
>  		goto out;
>  	}

-- 
Vitaly