KVM Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] KVM: x86: Only print persistent reasons for kvm disabled once
@ 2019-08-26 18:23 Tony Luck
  2019-08-27  6:27 ` Vitaly Kuznetsov
  0 siblings, 1 reply; 6+ messages in thread
From: Tony Luck @ 2019-08-26 18:23 UTC (permalink / raw)
  To: kvm
  Cc: Tony Luck, Paolo Bonzini, Radim Krčmář,
	Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
	Joerg Roedel

When I boot my server I'm treated to a console log with:

[   40.520510] kvm: disabled by bios
[   40.551234] kvm: disabled by bios
[   40.607987] kvm: disabled by bios
[   40.659701] kvm: disabled by bios
[   40.691224] kvm: disabled by bios
[   40.718786] kvm: disabled by bios
[   40.750122] kvm: disabled by bios
[   40.797170] kvm: disabled by bios
[   40.828408] kvm: disabled by bios

 ... many, many more lines, one for every logical CPU

Since it isn't likely that BIOS is going to suddenly enable
KVM between bringing up one CPU and the next, we might as
well just print this once.

Same for a few other unchanging reasons that might keep
kvm from being initialized.

Signed-off-by: Tony Luck <tony.luck@intel.com>
---
 arch/x86/kvm/x86.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 93b0bd45ac73..56d4a43dd2db 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -7007,18 +7007,18 @@ int kvm_arch_init(void *opaque)
 	struct kvm_x86_ops *ops = opaque;
 
 	if (kvm_x86_ops) {
-		printk(KERN_ERR "kvm: already loaded the other module\n");
+		pr_err_once("kvm: already loaded the other module\n");
 		r = -EEXIST;
 		goto out;
 	}
 
 	if (!ops->cpu_has_kvm_support()) {
-		printk(KERN_ERR "kvm: no hardware support\n");
+		pr_err_once("kvm: no hardware support\n");
 		r = -EOPNOTSUPP;
 		goto out;
 	}
 	if (ops->disabled_by_bios()) {
-		printk(KERN_ERR "kvm: disabled by bios\n");
+		pr_err_once("kvm: disabled by bios\n");
 		r = -EOPNOTSUPP;
 		goto out;
 	}
@@ -7029,7 +7029,7 @@ int kvm_arch_init(void *opaque)
 	 * vCPU's FPU state as a fxregs_state struct.
 	 */
 	if (!boot_cpu_has(X86_FEATURE_FPU) || !boot_cpu_has(X86_FEATURE_FXSR)) {
-		printk(KERN_ERR "kvm: inadequate fpu\n");
+		pr_err_once("kvm: inadequate fpu\n");
 		r = -EOPNOTSUPP;
 		goto out;
 	}
-- 
2.20.1


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] KVM: x86: Only print persistent reasons for kvm disabled once
  2019-08-26 18:23 [PATCH] KVM: x86: Only print persistent reasons for kvm disabled once Tony Luck
@ 2019-08-27  6:27 ` Vitaly Kuznetsov
  2019-08-27 17:50   ` Luck, Tony
  2019-08-27 19:08   ` Radim Krčmář
  0 siblings, 2 replies; 6+ messages in thread
From: Vitaly Kuznetsov @ 2019-08-27  6:27 UTC (permalink / raw)
  To: Tony Luck, kvm
  Cc: Paolo Bonzini, Radim Krčmář,
	Sean Christopherson, Wanpeng Li, Jim Mattson, Joerg Roedel

Tony Luck <tony.luck@intel.com> writes:

> When I boot my server I'm treated to a console log with:
>
> [   40.520510] kvm: disabled by bios
> [   40.551234] kvm: disabled by bios
> [   40.607987] kvm: disabled by bios
> [   40.659701] kvm: disabled by bios
> [   40.691224] kvm: disabled by bios
> [   40.718786] kvm: disabled by bios
> [   40.750122] kvm: disabled by bios
> [   40.797170] kvm: disabled by bios
> [   40.828408] kvm: disabled by bios
>
>  ... many, many more lines, one for every logical CPU

(If I didn't miss anything) we have the following code:

__init vmx_init()
        kvm_init();
            kvm_arch_init()

and we bail on first error so there should be only 1 message per module
load attempt. The question I have is who (and why) is trying to load
kvm-intel (or kvm-amd which is not any different) for each CPU? Is it
udev? Can this be changed?

In particular, I'm worried about eVMCS enablement in vmx_init(), we will
also get a bunch of "KVM: vmx: using Hyper-V Enlightened VMCS" messages
if the consequent kvm_arch_init() fails.

>
> Since it isn't likely that BIOS is going to suddenly enable
> KVM between bringing up one CPU and the next, we might as
> well just print this once.
>
> Same for a few other unchanging reasons that might keep
> kvm from being initialized.
>
> Signed-off-by: Tony Luck <tony.luck@intel.com>
> ---
>  arch/x86/kvm/x86.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 93b0bd45ac73..56d4a43dd2db 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -7007,18 +7007,18 @@ int kvm_arch_init(void *opaque)
>  	struct kvm_x86_ops *ops = opaque;
>  
>  	if (kvm_x86_ops) {
> -		printk(KERN_ERR "kvm: already loaded the other module\n");
> +		pr_err_once("kvm: already loaded the other module\n");
>  		r = -EEXIST;
>  		goto out;
>  	}
>  
>  	if (!ops->cpu_has_kvm_support()) {
> -		printk(KERN_ERR "kvm: no hardware support\n");
> +		pr_err_once("kvm: no hardware support\n");
>  		r = -EOPNOTSUPP;
>  		goto out;
>  	}
>  	if (ops->disabled_by_bios()) {
> -		printk(KERN_ERR "kvm: disabled by bios\n");
> +		pr_err_once("kvm: disabled by bios\n");
>  		r = -EOPNOTSUPP;
>  		goto out;
>  	}
> @@ -7029,7 +7029,7 @@ int kvm_arch_init(void *opaque)
>  	 * vCPU's FPU state as a fxregs_state struct.
>  	 */
>  	if (!boot_cpu_has(X86_FEATURE_FPU) || !boot_cpu_has(X86_FEATURE_FXSR)) {
> -		printk(KERN_ERR "kvm: inadequate fpu\n");
> +		pr_err_once("kvm: inadequate fpu\n");
>  		r = -EOPNOTSUPP;
>  		goto out;
>  	}

The messages themselves may need some love but this is irrelevant to the
patch, so

Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com>

-- 
Vitaly

^ permalink raw reply	[flat|nested] 6+ messages in thread

* RE: [PATCH] KVM: x86: Only print persistent reasons for kvm disabled once
  2019-08-27  6:27 ` Vitaly Kuznetsov
@ 2019-08-27 17:50   ` Luck, Tony
  2019-08-27 19:08   ` Radim Krčmář
  1 sibling, 0 replies; 6+ messages in thread
From: Luck, Tony @ 2019-08-27 17:50 UTC (permalink / raw)
  To: Vitaly Kuznetsov, kvm
  Cc: Paolo Bonzini, Radim Krcmár, Christopherson, Sean J,
	Wanpeng Li, Jim Mattson, Joerg Roedel

> and we bail on first error so there should be only 1 message per module
> load attempt. The question I have is who (and why) is trying to load
> kvm-intel (or kvm-amd which is not any different) for each CPU? Is it
> udev? Can this be changed?

No idea where to look.  The system has a RHEL8 user space install. Kernel
is latest from Linus (v5.3-rc6).

-Tony

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] KVM: x86: Only print persistent reasons for kvm disabled once
  2019-08-27  6:27 ` Vitaly Kuznetsov
  2019-08-27 17:50   ` Luck, Tony
@ 2019-08-27 19:08   ` Radim Krčmář
  2019-08-27 19:24     ` Sean Christopherson
  1 sibling, 1 reply; 6+ messages in thread
From: Radim Krčmář @ 2019-08-27 19:08 UTC (permalink / raw)
  To: Vitaly Kuznetsov
  Cc: Tony Luck, kvm, Paolo Bonzini, Sean Christopherson, Wanpeng Li,
	Jim Mattson, Joerg Roedel

2019-08-27 08:27+0200, Vitaly Kuznetsov:
> Tony Luck <tony.luck@intel.com> writes:
> 
> > When I boot my server I'm treated to a console log with:
> >
> > [   40.520510] kvm: disabled by bios
> > [   40.551234] kvm: disabled by bios
> > [   40.607987] kvm: disabled by bios
> > [   40.659701] kvm: disabled by bios
> > [   40.691224] kvm: disabled by bios
> > [   40.718786] kvm: disabled by bios
> > [   40.750122] kvm: disabled by bios
> > [   40.797170] kvm: disabled by bios
> > [   40.828408] kvm: disabled by bios
> >
> >  ... many, many more lines, one for every logical CPU
> 
> (If I didn't miss anything) we have the following code:
> 
> __init vmx_init()
>         kvm_init();
>             kvm_arch_init()
> 
> and we bail on first error so there should be only 1 message per module
> load attempt. The question I have is who (and why) is trying to load
> kvm-intel (or kvm-amd which is not any different) for each CPU? Is it
> udev? Can this be changed?

I agree that this is a highly suspicious behavior.

It would be really helpful if we found out what is causing it.
So far, this patch seems to be working around a userspace bug.

> In particular, I'm worried about eVMCS enablement in vmx_init(), we will
> also get a bunch of "KVM: vmx: using Hyper-V Enlightened VMCS" messages
> if the consequent kvm_arch_init() fails.

And we can't get rid of this through the printk_once trick, because this
code lives in kvm_intel module and therefore gets unloaded on every
failure.

I am also not inclined to apply the patch as we will likely merge the
kvm and kvm_{svm,intel} modules in the future to take full advantage of
link time optimizations and this patch would stop working after that.

Thanks.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] KVM: x86: Only print persistent reasons for kvm disabled once
  2019-08-27 19:08   ` Radim Krčmář
@ 2019-08-27 19:24     ` Sean Christopherson
  2019-09-11 15:30       ` Paolo Bonzini
  0 siblings, 1 reply; 6+ messages in thread
From: Sean Christopherson @ 2019-08-27 19:24 UTC (permalink / raw)
  To: Radim Krčmář
  Cc: Vitaly Kuznetsov, Tony Luck, kvm, Paolo Bonzini, Wanpeng Li,
	Jim Mattson, Joerg Roedel

On Tue, Aug 27, 2019 at 09:08:10PM +0200, Radim Krčmář wrote:
> I am also not inclined to apply the patch as we will likely merge the
> kvm and kvm_{svm,intel} modules in the future to take full advantage of
> link time optimizations and this patch would stop working after that.

Any chance you can provide additional details on the plan for merging
modules?  E.g. I assume there would still be kvm_intel and kvm_svm, just
no vanilla kvm?

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] KVM: x86: Only print persistent reasons for kvm disabled once
  2019-08-27 19:24     ` Sean Christopherson
@ 2019-09-11 15:30       ` Paolo Bonzini
  0 siblings, 0 replies; 6+ messages in thread
From: Paolo Bonzini @ 2019-09-11 15:30 UTC (permalink / raw)
  To: Sean Christopherson, Radim Krčmář
  Cc: Vitaly Kuznetsov, Tony Luck, kvm, Wanpeng Li, Jim Mattson, Joerg Roedel

On 27/08/19 21:24, Sean Christopherson wrote:
> On Tue, Aug 27, 2019 at 09:08:10PM +0200, Radim Krčmář wrote:
>> I am also not inclined to apply the patch as we will likely merge the
>> kvm and kvm_{svm,intel} modules in the future to take full advantage of
>> link time optimizations and this patch would stop working after that.
> 
> Any chance you can provide additional details on the plan for merging
> modules?  E.g. I assume there would still be kvm_intel and kvm_svm, just
> no vanilla kvm?

Yes, that is the idea.  Basically trade disk space for performance,
since kvm+kvm_intel+kvm_amd are never loaded together and kvm never has
>1 user.

Paolo


^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, back to index

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-26 18:23 [PATCH] KVM: x86: Only print persistent reasons for kvm disabled once Tony Luck
2019-08-27  6:27 ` Vitaly Kuznetsov
2019-08-27 17:50   ` Luck, Tony
2019-08-27 19:08   ` Radim Krčmář
2019-08-27 19:24     ` Sean Christopherson
2019-09-11 15:30       ` Paolo Bonzini

KVM Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/kvm/0 kvm/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 kvm kvm/ https://lore.kernel.org/kvm \
		kvm@vger.kernel.org kvm@archiver.kernel.org
	public-inbox-index kvm


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.kvm


AGPL code for this site: git clone https://public-inbox.org/ public-inbox