linux-kernel - Re: [PATCH 1/2] KVM: x86: Add vendor name to kvm_x86

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87k0i6x0jk.fsf@vitty.brq.redhat.com>
Date:   Thu, 21 Oct 2021 10:08:31 +0200
From:   Vitaly Kuznetsov <vkuznets@...hat.com>
To:     Sean Christopherson <seanjc@...gle.com>,
        Paolo Bonzini <pbonzini@...hat.com>
Cc:     Wanpeng Li <wanpengli@...cent.com>,
        Jim Mattson <jmattson@...gle.com>,
        Joerg Roedel <joro@...tes.org>, kvm@...r.kernel.org,
        linux-kernel@...r.kernel.org, Paul Menzel <pmenzel@...gen.mpg.de>,
        "Maciej S . Szmigiero" <mail@...iej.szmigiero.name>
Subject: Re: [PATCH 1/2] KVM: x86: Add vendor name to kvm_x86_ops, use it
 for error messages

Sean Christopherson <seanjc@...gle.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@...gen.mpg.de>
> Signed-off-by: Sean Christopherson <seanjc@...gle.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