RFC: silencing kvm unimplemented msr spew.

Commit Message

Bandan Das July 19, 2016, 8:24 p.m. UTC

Dave Jones <[e-mail]> 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)


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 [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Dave Jones July 21, 2016, 8:24 p.m. UTC | #1

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).

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 [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Bandan Das July 21, 2016, 8:41 p.m. UTC | #2

Hi Dave,

Dave Jones <[e-mail]> 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 [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Paolo Bonzini July 22, 2016, 8:48 a.m. UTC | #3

> 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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Bandan Das July 22, 2016, 4:04 p.m. UTC | #4

Paolo Bonzini <[e-mail]> 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 [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Paolo Bonzini July 22, 2016, 4:20 p.m. UTC | #5

> >> 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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Bandan Das July 22, 2016, 4:37 p.m. UTC | #6

Paolo Bonzini <[e-mail]> 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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

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);