Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1445588 > unrolled thread

Re: RFC: silencing kvm unimplemented msr spew.

Started byPaolo Bonzini <pbonzini@redhat.com>
First post2016-07-18 17:30 +0200
Last post2016-07-20 00:20 +0200
Articles 8 — 2 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: RFC: silencing kvm unimplemented msr spew. Paolo Bonzini <pbonzini@redhat.com> - 2016-07-18 17:30 +0200
    Re: RFC: silencing kvm unimplemented msr spew. Bandan Das <bsd@redhat.com> - 2016-07-19 22:30 +0200
      Re: RFC: silencing kvm unimplemented msr spew. Bandan Das <bsd@redhat.com> - 2016-07-21 22:50 +0200
      Re: RFC: silencing kvm unimplemented msr spew. Paolo Bonzini <pbonzini@redhat.com> - 2016-07-22 10:50 +0200
        Re: RFC: silencing kvm unimplemented msr spew. Bandan Das <bsd@redhat.com> - 2016-07-22 18:10 +0200
          Re: RFC: silencing kvm unimplemented msr spew. Paolo Bonzini <pbonzini@redhat.com> - 2016-07-22 18:30 +0200
            Re: RFC: silencing kvm unimplemented msr spew. Bandan Das <bsd@redhat.com> - 2016-07-22 18:40 +0200
    Re: RFC: silencing kvm unimplemented msr spew. Paolo Bonzini <pbonzini@redhat.com> - 2016-07-20 00:20 +0200

#1445588 — Re: RFC: silencing kvm unimplemented msr spew.

FromPaolo Bonzini <pbonzini@redhat.com>
Date2016-07-18 17:30 +0200
SubjectRe: RFC: silencing kvm unimplemented msr spew.
Message-ID<rWiEy-7XO-11@gated-at.bofh.it>
> kvm is pretty noisy when you have guests poking at MSRs that the kernel
> doesn't implement.  The conveniently named 'ignore_msrs' option initially
> seemed
> like it was what I was looking for, but it changes the printk instead
> of eliding it.
> 
> Untested patch below converts ignore_msrs to a bitmask and adds an option to
> be
> completely silent. The idea being if after testing, things still work and you
> don't care about those messages, you can deploy in production with the
> silence option.
> 
> Would something like this be acceptable ?

Indeed, ignore_msrs does a completely different thing.  It suppresses
general protection faults in the guest.  It is related to behavior that
KVM injects in the guests, not to the things that KVM spews in the host.

What about just downgrading the printf to KERN_DEBUG?  You could simply
change from vcpu_unimpl to vcpu_debug, but it's probably a good idea to
keep the ratelimiting; there's a kvm_pr_unimpl, so maybe add a new
kvm_pr_debug and vcpu_pr_debug.

Thanks,

Paolo

> 
> 	Dave
> 
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 7da5dd2057a9..f9db287118d0 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -96,8 +96,12 @@ static void __kvm_set_rflags(struct kvm_vcpu *vcpu,
> unsigned long rflags);
>  struct kvm_x86_ops *kvm_x86_ops __read_mostly;
>  EXPORT_SYMBOL_GPL(kvm_x86_ops);
>  
> -static bool __read_mostly ignore_msrs = 0;
> -module_param(ignore_msrs, bool, S_IRUGO | S_IWUSR);
> +#define UNHANDLED_UNIMPL_MSR 0
> +#define IGNORE_UNIMPL_MSR    1
> +#define SILENT_UNHANDLED_MSR 2
> +#define SILENT_IGNORED_MSR   3
> +static int __read_mostly ignore_msrs = UNHANDLED_UNIMPL_MSR;
> +module_param(ignore_msrs, int, S_IRUGO | S_IWUSR);
>  
>  unsigned int min_timer_period_us = 500;
>  module_param(min_timer_period_us, uint, S_IRUGO | S_IWUSR);
> @@ -2228,15 +2232,22 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct
> msr_data *msr_info)
>  			return xen_hvm_config(vcpu, data);
>  		if (kvm_pmu_is_valid_msr(vcpu, msr))
>  			return kvm_pmu_set_msr(vcpu, msr_info);
> -		if (!ignore_msrs) {
> +
> +		switch (ignore_msrs) {
> +		case UNHANDLED_UNIMPL_MSR:
>  			vcpu_unimpl(vcpu, "unhandled wrmsr: 0x%x data %llx\n",
> -				    msr, data);
> +				msr, data);
> +			/* fallthrough */
> +		case SILENT_UNHANDLED_MSR:
>  			return 1;
> -		} else {
> +		case IGNORE_UNIMPL_MSR:
>  			vcpu_unimpl(vcpu, "ignored wrmsr: 0x%x data %llx\n",
> -				    msr, data);
> +				msr, data);
> +			/* fallthrough */
> +		case SILENT_IGNORED_MSR:
>  			break;
>  		}
> +		break;
>  	}
>  	return 0;
>  }
>

[toc] | [next] | [standalone]

#1446709

FromBandan Das <bsd@redhat.com>
Date2016-07-19 22:30 +0200
Message-ID<rWJOq-ca-21@gated-at.bofh.it>
In reply to#1445588
Dave Jones <dsj@fb.com> writes:

> On Mon, Jul 18, 2016 at 11:26:50AM -0400, Paolo Bonzini wrote:
>  > 
>  > > kvm is pretty noisy when you have guests poking at MSRs that the kernel
>  > > doesn't implement.  The conveniently named 'ignore_msrs' option initially
>  > > seemed
>  > > like it was what I was looking for, but it changes the printk instead
>  > > of eliding it.
>  > > 
>  > > Untested patch below converts ignore_msrs to a bitmask and adds an option to
>  > > be
>  > > completely silent. The idea being if after testing, things still work and you
>  > > don't care about those messages, you can deploy in production with the
>  > > silence option.
>  > > 
>  > > Would something like this be acceptable ?
>  > 
>  > Indeed, ignore_msrs does a completely different thing.  It suppresses
>  > general protection faults in the guest.  It is related to behavior that
>  > KVM injects in the guests, not to the things that KVM spews in the host.
>  > 
>  > What about just downgrading the printf to KERN_DEBUG?  You could simply
>  > change from vcpu_unimpl to vcpu_debug, but it's probably a good idea to
>  > keep the ratelimiting; there's a kvm_pr_unimpl, so maybe add a new
>  > kvm_pr_debug and vcpu_pr_debug.
>
> Hm, we've certainly got a lot of options in terms of print primitives these days.
>
> We could just do this...

Heh, actually after speaking about this to Paolo a while back, I had this sleeping
in my local branch for a while. Same as what you suggested (without the ratelimiting)

diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
index def97b3..c6e6f64 100644
--- a/arch/x86/kvm/mmu.c
+++ b/arch/x86/kvm/mmu.c
@@ -4952,7 +4952,7 @@ void kvm_mmu_invalidate_mmio_sptes(struct kvm *kvm, struct kvm_memslots *slots)
         * zap all shadow pages.
         */
        if (unlikely((slots->generation & MMIO_GEN_MASK) == 0)) {
-               printk_ratelimited(KERN_DEBUG "kvm: zapping shadow pages for mmio generation wraparound\n");
+               kvm_debug("zapping shadow pages for mmio generation wraparound\n");
                kvm_mmu_invalidate_zap_all_pages(kvm);
        }
 }
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 7da5dd2..02d09f9 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -2229,7 +2229,7 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
                if (kvm_pmu_is_valid_msr(vcpu, msr))
                        return kvm_pmu_set_msr(vcpu, msr_info);
                if (!ignore_msrs) {
-                       vcpu_unimpl(vcpu, "unhandled wrmsr: 0x%x data %llx\n",
+                       vcpu_debug(vcpu, "unhandled wrmsr: 0x%x data %llx\n",
                                    msr, data);
                        return 1;
                } else {
@@ -2441,7 +2441,7 @@ int kvm_get_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
                if (kvm_pmu_is_valid_msr(vcpu, msr_info->index))
                        return kvm_pmu_get_msr(vcpu, msr_info->index, &msr_info->data);
                if (!ignore_msrs) {
-                       vcpu_unimpl(vcpu, "unhandled rdmsr: 0x%x\n", msr_info->index);
+                       vcpu_debug(vcpu, "unhandled rdmsr: 0x%x\n", msr_info->index);
                        return 1;
                } else {
                        vcpu_unimpl(vcpu, "ignored rdmsr: 0x%x\n", msr_info->index);

I had the same reasoning regarding  dynamic debugging which I think is
enabled by default on most builds anyway.

Bandan

> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index 1c9c973a7dd9..a80b9a0a5f8c 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -423,7 +423,7 @@ struct kvm {
>  #define kvm_debug(fmt, ...) \
>         pr_debug("kvm [%i]: " fmt, task_pid_nr(current), ## __VA_ARGS__)
>  #define kvm_pr_unimpl(fmt, ...) \
> -       pr_err_ratelimited("kvm [%i]: " fmt, \
> +       pr_debug_ratelimited("kvm [%i]: " fmt, \
>                            task_tgid_nr(current), ## __VA_ARGS__)
>  
>  /* The guest did something we don't support. */
>
>
> Which I think would have the desired effect, and also gets us dynamic debug
> support for free.
>
> Thoughts ?
>
> 	Dave
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[toc] | [prev] | [next] | [standalone]

#1448130

FromBandan Das <bsd@redhat.com>
Date2016-07-21 22:50 +0200
Message-ID<rXt4R-47W-7@gated-at.bofh.it>
In reply to#1446709
Hi Dave,

Dave Jones <dsj@fb.com> writes:

> On Tue, Jul 19, 2016 at 04:24:31PM -0400, Bandan Das wrote:
>
>  > Heh, actually after speaking about this to Paolo a while back, I had this sleeping
>  > in my local branch for a while. Same as what you suggested (without the ratelimiting)
>  > 
>  > diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
>  > index def97b3..c6e6f64 100644
>  > --- a/arch/x86/kvm/mmu.c
>  > +++ b/arch/x86/kvm/mmu.c
>  > @@ -4952,7 +4952,7 @@ void kvm_mmu_invalidate_mmio_sptes(struct kvm *kvm, struct kvm_memslots *slots)
>  >          * zap all shadow pages.
>  >          */
>  >         if (unlikely((slots->generation & MMIO_GEN_MASK) == 0)) {
>  > -               printk_ratelimited(KERN_DEBUG "kvm: zapping shadow pages for mmio generation wraparound\n");
>  > +               kvm_debug("zapping shadow pages for mmio generation wraparound\n");
>  >                 kvm_mmu_invalidate_zap_all_pages(kvm);
>  >         }
>  >  }
>  > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>  > index 7da5dd2..02d09f9 100644
>  > --- a/arch/x86/kvm/x86.c
>  > +++ b/arch/x86/kvm/x86.c
>  > @@ -2229,7 +2229,7 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>  >                 if (kvm_pmu_is_valid_msr(vcpu, msr))
>  >                         return kvm_pmu_set_msr(vcpu, msr_info);
>  >                 if (!ignore_msrs) {
>  > -                       vcpu_unimpl(vcpu, "unhandled wrmsr: 0x%x data %llx\n",
>  > +                       vcpu_debug(vcpu, "unhandled wrmsr: 0x%x data %llx\n",
>  >                                     msr, data);
>  >                         return 1;
>  >                 } else {
>  > @@ -2441,7 +2441,7 @@ int kvm_get_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>  >                 if (kvm_pmu_is_valid_msr(vcpu, msr_info->index))
>  >                         return kvm_pmu_get_msr(vcpu, msr_info->index, &msr_info->data);
>  >                 if (!ignore_msrs) {
>  > -                       vcpu_unimpl(vcpu, "unhandled rdmsr: 0x%x\n", msr_info->index);
>  > +                       vcpu_debug(vcpu, "unhandled rdmsr: 0x%x\n", msr_info->index);
>  >                         return 1;
>  >                 } else {
>  >                         vcpu_unimpl(vcpu, "ignored rdmsr: 0x%x\n", msr_info->index);
>  > 
>  > I had the same reasoning regarding  dynamic debugging which I think is
>  > enabled by default on most builds anyway.
>
> Yeah, that's close. Though I would have done the same for the other side of the if's too.
> (Still evaluating which mode is actually more useful for us).

My reasoning was:

When debugging guest runs/unimplemented msrs accesses, it makes sense to use the
original behavior of printing out the accesses. So, vcpu_unimpl() remains
unchanged and is used for that case and vcpu_debug_ratelimited becomes the
default. 

Bandan

> Paolo, would you prefer this, or the other approach you already ack'd ?
>
> 	Dave
>
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[toc] | [prev] | [next] | [standalone]

#1448473

FromPaolo Bonzini <pbonzini@redhat.com>
Date2016-07-22 10:50 +0200
Message-ID<rXEjE-3pq-13@gated-at.bofh.it>
In reply to#1446709
> On Tue, Jul 19, 2016 at 04:24:31PM -0400, Bandan Das wrote:
> 
>  > Heh, actually after speaking about this to Paolo a while back, I had this
>  > sleeping
>  > in my local branch for a while. Same as what you suggested (without the
>  > ratelimiting)
>  > 
>  > diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
>  > index def97b3..c6e6f64 100644
>  > --- a/arch/x86/kvm/mmu.c
>  > +++ b/arch/x86/kvm/mmu.c
>  > @@ -4952,7 +4952,7 @@ void kvm_mmu_invalidate_mmio_sptes(struct kvm *kvm,
>  > struct kvm_memslots *slots)
>  >          * zap all shadow pages.
>  >          */
>  >         if (unlikely((slots->generation & MMIO_GEN_MASK) == 0)) {
>  > -               printk_ratelimited(KERN_DEBUG "kvm: zapping shadow pages
>  > for mmio generation wraparound\n");
>  > +               kvm_debug("zapping shadow pages for mmio generation
>  > wraparound\n");
>  >                 kvm_mmu_invalidate_zap_all_pages(kvm);
>  >         }
>  >  }
>  > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>  > index 7da5dd2..02d09f9 100644
>  > --- a/arch/x86/kvm/x86.c
>  > +++ b/arch/x86/kvm/x86.c
>  > @@ -2229,7 +2229,7 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct
>  > msr_data *msr_info)
>  >                 if (kvm_pmu_is_valid_msr(vcpu, msr))
>  >                         return kvm_pmu_set_msr(vcpu, msr_info);
>  >                 if (!ignore_msrs) {
>  > -                       vcpu_unimpl(vcpu, "unhandled wrmsr: 0x%x data
>  > %llx\n",
>  > +                       vcpu_debug(vcpu, "unhandled wrmsr: 0x%x data
>  > %llx\n",
>  >                                     msr, data);
>  >                         return 1;
>  >                 } else {
>  > @@ -2441,7 +2441,7 @@ int kvm_get_msr_common(struct kvm_vcpu *vcpu, struct
>  > msr_data *msr_info)
>  >                 if (kvm_pmu_is_valid_msr(vcpu, msr_info->index))
>  >                         return kvm_pmu_get_msr(vcpu, msr_info->index,
>  >                         &msr_info->data);
>  >                 if (!ignore_msrs) {
>  > -                       vcpu_unimpl(vcpu, "unhandled rdmsr: 0x%x\n", msr_info->index);
>  > +                       vcpu_debug(vcpu, "unhandled rdmsr: 0x%x\n", msr_info->index);
>  >                         return 1;
>  >                 } else {
>  >                         vcpu_unimpl(vcpu, "ignored rdmsr: 0x%x\n",
>  >                         msr_info->index);
>  > 
>  > I had the same reasoning regarding  dynamic debugging which I think is
>  > enabled by default on most builds anyway.
> 
> Yeah, that's close. Though I would have done the same for the other side of
> the if's too.
> (Still evaluating which mode is actually more useful for us).

For Linux guests, there should be no reason to use ignore_msrs.  Linux
is pretty resilient to "missing" MSRs (especially because they are already
ignored if the kernel is compiled with CONFIG_PARAVIRT=y!).  The option is
really more for Windows, because it doesn't have anything like CONFIG_PARAVIRT
and because drivers are sometimes less vetted (and sometimes do RDMSR
themselves for whatever reason).  In general we try to look at beta versions
of Windows and add any required MSRs well before the final release date,
but if you're using old kernels you might be stuck with ignore_msrs.

IOW, if there was a really common reason to use ignore_msrs it would be
the default. ;)

> Paolo, would you prefer this, or the other approach you already ack'd ?

I think I prefer the other, because vcpu_debug is not ratelimited.
If the guest can trigger a printk it should always be ratelimited.

Paolo

[toc] | [prev] | [next] | [standalone]

#1448659

FromBandan Das <bsd@redhat.com>
Date2016-07-22 18:10 +0200
Message-ID<rXLbr-80C-9@gated-at.bofh.it>
In reply to#1448473
Paolo Bonzini <pbonzini@redhat.com> writes:
...
>> Yeah, that's close. Though I would have done the same for the other side of
>> the if's too.
>> (Still evaluating which mode is actually more useful for us).
>
> For Linux guests, there should be no reason to use ignore_msrs.  Linux
> is pretty resilient to "missing" MSRs (especially because they are already
> ignored if the kernel is compiled with CONFIG_PARAVIRT=y!).  The option is
> really more for Windows, because it doesn't have anything like CONFIG_PARAVIRT
> and because drivers are sometimes less vetted (and sometimes do RDMSR
> themselves for whatever reason).  In general we try to look at beta versions
> of Windows and add any required MSRs well before the final release date,
> but if you're using old kernels you might be stuck with ignore_msrs.
>
> IOW, if there was a really common reason to use ignore_msrs it would be
> the default. ;)
>
>> Paolo, would you prefer this, or the other approach you already ack'd ?
>
> I think I prefer the other, because vcpu_debug is not ratelimited.
> If the guest can trigger a printk it should always be ratelimited.

Agree with rate limiting, but making this the default for everything doesn't sound
right IMO, especially for ignore_msrs=1. vcpu_unimpl is already rate limited.

Or is this change specifically to suppress messages on ignore_msrs=1 ?

> Paolo
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[toc] | [prev] | [next] | [standalone]

#1448668

FromPaolo Bonzini <pbonzini@redhat.com>
Date2016-07-22 18:30 +0200
Message-ID<rXLuN-86Y-21@gated-at.bofh.it>
In reply to#1448659
> >> Paolo, would you prefer this, or the other approach you already ack'd ?
> >
> > I think I prefer the other, because vcpu_debug is not ratelimited.
> > If the guest can trigger a printk it should always be ratelimited.
> 
> Agree with rate limiting, but making this the default for everything doesn't
> sound right IMO, especially for ignore_msrs=1. vcpu_unimpl is already rate
> limited.

The problem is that your patch removes rate limiting whenever it now
uses vcpu_debug.

Paolo

[toc] | [prev] | [next] | [standalone]

#1448669

FromBandan Das <bsd@redhat.com>
Date2016-07-22 18:40 +0200
Message-ID<rXLEu-8aa-31@gated-at.bofh.it>
In reply to#1448668
Paolo Bonzini <pbonzini@redhat.com> writes:

>> >> Paolo, would you prefer this, or the other approach you already ack'd ?
>> >
>> > I think I prefer the other, because vcpu_debug is not ratelimited.
>> > If the guest can trigger a printk it should always be ratelimited.
>> 
>> Agree with rate limiting, but making this the default for everything doesn't
>> sound right IMO, especially for ignore_msrs=1. vcpu_unimpl is already rate
>> limited.
>
> The problem is that your patch removes rate limiting whenever it now
> uses vcpu_debug.

I was suggesting Dave to do something like what I posted but with a new
vcpu_debug_ratelimited.

> Paolo

[toc] | [prev] | [next] | [standalone]

#1446752

FromPaolo Bonzini <pbonzini@redhat.com>
Date2016-07-20 00:20 +0200
Message-ID<rWLwR-1l8-13@gated-at.bofh.it>
In reply to#1445588
> On Mon, Jul 18, 2016 at 11:26:50AM -0400, Paolo Bonzini wrote:
> > Indeed, ignore_msrs does a completely different thing.  It suppresses
> > general protection faults in the guest.  It is related to behavior that
> > KVM injects in the guests, not to the things that KVM spews in the host.
> > 
> > What about just downgrading the printf to KERN_DEBUG?  You could simply
> > change from vcpu_unimpl to vcpu_debug, but it's probably a good idea to
> > keep the ratelimiting; there's a kvm_pr_unimpl, so maybe add a new
> > kvm_pr_debug and vcpu_pr_debug.
>
> Hm, we've certainly got a lot of options in terms of print primitives these
> days.
> 
> We could just do this...
> 
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index 1c9c973a7dd9..a80b9a0a5f8c 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -423,7 +423,7 @@ struct kvm {
>  #define kvm_debug(fmt, ...) \
>         pr_debug("kvm [%i]: " fmt, task_pid_nr(current), ## __VA_ARGS__)
>  #define kvm_pr_unimpl(fmt, ...) \
> -       pr_err_ratelimited("kvm [%i]: " fmt, \
> +       pr_debug_ratelimited("kvm [%i]: " fmt, \
>                            task_tgid_nr(current), ## __VA_ARGS__)
>  
>  /* The guest did something we don't support. */
> 
> Which I think would have the desired effect, and also gets us dynamic debug
> support for free.

There are some "unimplemented" messages that would be affected and really
should be errors (or even WARNs), but I guess this patch is okay.  Can you
submit it with SoB and all that?

Thanks,

Paolo

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web