[PATCH 0/2] KVM: x86: Enhance vendor module error messages

[PATCH 0/2] KVM: x86: Enhance vendor module error messages

[Sean Christopherson]

Paul Menzel encountered the bad userspace behavior of spamming KVM module
loading on all CPUs, which was mitigated by commit ef935c25fd64 ("kvm: x86:
Limit the number of "kvm: disabled by bios" messages"), except this time
userspace is extra "clever" and spams both kvm_intel and kvm_amd.  Because
the "already loaded the other module" message isn't ratelimited, the bogus
module load managed to spam the kernel log.

Patch 1 addresses another suggestion from Paul by incorporating the vendor
module name into the error messages.

Patch 2 addresses the original report by prioritizing the hardware/bios
support messages over the "already loaded" error.  In additional to
reducing spam, doing so also ensures consistent messaging if a vendor
module isn't supported regardless of what other modules may be loaded.

[*] https://lkml.kernel.org/r/20210818114956.7171-1-pmenzel@molgen.mpg.de

Sean Christopherson (2):
  KVM: x86: Add vendor name to kvm_x86_ops, use it for error messages
  KVM: x86: Defer "already loaded" check until after basic support
    checks

 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              | 17 +++++++++--------
 4 files changed, 15 insertions(+), 8 deletions(-)

-- 
2.33.0.1079.g6e70778dc9-goog

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

[Sean Christopherson]

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);
 		r = -EOPNOTSUPP;
 		goto out;
 	}
-- 
2.33.0.1079.g6e70778dc9-goog

[PATCH 2/2] KVM: x86: Defer "already loaded" check until after basic support checks

[Sean Christopherson]

Move KVM's check for a vendor module already being loaded below the basic
functionality checks so that attempting to load an unsupported module
provides the same result regardless of whether or not a supported vendor
module is already loaded.

Intentionally keep the err non-ratelimited; if userspace is probing two
different modules for the same vendor on all CPUs, it deserves the spam.

Reported-by: Paul Menzel <pmenzel@molgen.mpg.de>
Cc: Maciej S. Szmigiero <mail@maciej.szmigiero.name>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/x86.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index e966e9cdd805..f67da77be267 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -8538,12 +8538,6 @@ int kvm_arch_init(void *opaque)
 	struct kvm_x86_init_ops *ops = opaque;
 	int r;
 
-	if (kvm_x86_ops.hardware_enable) {
-		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 for '%s'\n",
 				   ops->runtime_ops->name);
@@ -8556,6 +8550,11 @@ int kvm_arch_init(void *opaque)
 		r = -EOPNOTSUPP;
 		goto out;
 	}
+	if (kvm_x86_ops.hardware_enable) {
+		pr_err("kvm: already loaded vendor module '%s'\n", kvm_x86_ops.name);
+		r = -EEXIST;
+		goto out;
+	}
 
 	/*
 	 * KVM explicitly assumes that the guest has an FPU and
-- 
2.33.0.1079.g6e70778dc9-goog

Re: [PATCH 0/2] KVM: x86: Enhance vendor module error messages

[Paolo Bonzini]

On 18/10/21 20:39, Sean Christopherson wrote:
> Paul Menzel encountered the bad userspace behavior of spamming KVM module
> loading on all CPUs, which was mitigated by commit ef935c25fd64 ("kvm: x86:
> Limit the number of "kvm: disabled by bios" messages"), except this time
> userspace is extra "clever" and spams both kvm_intel and kvm_amd.  Because
> the "already loaded the other module" message isn't ratelimited, the bogus
> module load managed to spam the kernel log.
> 
> Patch 1 addresses another suggestion from Paul by incorporating the vendor
> module name into the error messages.
> 
> Patch 2 addresses the original report by prioritizing the hardware/bios
> support messages over the "already loaded" error.  In additional to
> reducing spam, doing so also ensures consistent messaging if a vendor
> module isn't supported regardless of what other modules may be loaded.
> 
> [*] https://lkml.kernel.org/r/20210818114956.7171-1-pmenzel@molgen.mpg.de
> 
> Sean Christopherson (2):
>    KVM: x86: Add vendor name to kvm_x86_ops, use it for error messages
>    KVM: x86: Defer "already loaded" check until after basic support
>      checks
> 
>   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              | 17 +++++++++--------
>   4 files changed, 15 insertions(+), 8 deletions(-)
> 

Queued, thanks.  But I kept the ratelimiting; given the recent "clashes" 
with the chief penguin, I am also ratelimiting the snark in the commit 
messages.

Paolo

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

[Vitaly Kuznetsov]

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

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

[Sean Christopherson]

On Thu, Oct 21, 2021, Vitaly Kuznetsov wrote:
> >  	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.

I'd prefer to avoid VT-x/AMD-v so as not to speculate on the module being loaded
or the underlying hardware, e.g. I've no idea what Hygon, Zhaoxin, etc... use for
"code" names.

What about

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

to add the virtualization flavor but still make it clear that error is coming
from the base kvm module.

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

[Vitaly Kuznetsov]

Sean Christopherson <seanjc@google.com> writes:

> On Thu, Oct 21, 2021, Vitaly Kuznetsov wrote:
>> >  	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.
>
> I'd prefer to avoid VT-x/AMD-v so as not to speculate on the module being loaded
> or the underlying hardware, e.g. I've no idea what Hygon, Zhaoxin, etc... use for
> "code" names.
>
> What about
>
> 		pr_err_ratelimited("kvm: virtualization support for '%s' disabled by BIOS\n",
> 				   ops->runtime_ops->name);
>
> to add the virtualization flavor but still make it clear that error is coming
> from the base kvm module.

Works for me, thanks! I just want to make sure people know what to do
when they see the message.

-- 
Vitaly