Re: RFC: silencing kvm unimplemented msr spew.

[Paolo Bonzini <pbonzini@redhat.com>]

> 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