* RFC: silencing kvm unimplemented msr spew. @ 2016-07-15 19:27 Dave Jones 2016-07-18 15:26 ` Paolo Bonzini 0 siblings, 1 reply; 11+ messages in thread From: Dave Jones @ 2016-07-15 19:27 UTC (permalink / raw) To: Linux Kernel; +Cc: Paolo Bonzini, Radim Krčmář 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 ? 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; } ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: RFC: silencing kvm unimplemented msr spew. 2016-07-15 19:27 RFC: silencing kvm unimplemented msr spew Dave Jones @ 2016-07-18 15:26 ` Paolo Bonzini 2016-07-19 19:58 ` Dave Jones 0 siblings, 1 reply; 11+ messages in thread From: Paolo Bonzini @ 2016-07-18 15:26 UTC (permalink / raw) To: Dave Jones; +Cc: Linux Kernel, Radim Krčmář, kvm > 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; > } > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: RFC: silencing kvm unimplemented msr spew. 2016-07-18 15:26 ` Paolo Bonzini @ 2016-07-19 19:58 ` Dave Jones 2016-07-19 20:24 ` Bandan Das 2016-07-19 22:12 ` Paolo Bonzini 0 siblings, 2 replies; 11+ messages in thread From: Dave Jones @ 2016-07-19 19:58 UTC (permalink / raw) To: Paolo Bonzini; +Cc: Linux Kernel, Radim Krčmář, kvm 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... 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: RFC: silencing kvm unimplemented msr spew. 2016-07-19 19:58 ` Dave Jones @ 2016-07-19 20:24 ` Bandan Das 2016-07-21 20:24 ` Dave Jones 2016-07-19 22:12 ` Paolo Bonzini 1 sibling, 1 reply; 11+ messages in thread From: Bandan Das @ 2016-07-19 20:24 UTC (permalink / raw) To: Dave Jones; +Cc: Paolo Bonzini, Linux Kernel, Radim Krčmář, kvm 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: RFC: silencing kvm unimplemented msr spew. 2016-07-19 20:24 ` Bandan Das @ 2016-07-21 20:24 ` Dave Jones 2016-07-21 20:41 ` Bandan Das 2016-07-22 8:48 ` Paolo Bonzini 0 siblings, 2 replies; 11+ messages in thread From: Dave Jones @ 2016-07-21 20:24 UTC (permalink / raw) To: Bandan Das; +Cc: Paolo Bonzini, Linux Kernel, Radim Krčmář, kvm 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: RFC: silencing kvm unimplemented msr spew. 2016-07-21 20:24 ` Dave Jones @ 2016-07-21 20:41 ` Bandan Das 2016-07-22 8:48 ` Paolo Bonzini 1 sibling, 0 replies; 11+ messages in thread From: Bandan Das @ 2016-07-21 20:41 UTC (permalink / raw) To: Dave Jones; +Cc: Paolo Bonzini, Linux Kernel, Radim Krčmář, kvm 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: RFC: silencing kvm unimplemented msr spew. 2016-07-21 20:24 ` Dave Jones 2016-07-21 20:41 ` Bandan Das @ 2016-07-22 8:48 ` Paolo Bonzini 2016-07-22 16:04 ` Bandan Das 1 sibling, 1 reply; 11+ messages in thread From: Paolo Bonzini @ 2016-07-22 8:48 UTC (permalink / raw) To: Dave Jones; +Cc: Bandan Das, Linux Kernel, Radim Krčmář, kvm > 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: RFC: silencing kvm unimplemented msr spew. 2016-07-22 8:48 ` Paolo Bonzini @ 2016-07-22 16:04 ` Bandan Das 2016-07-22 16:20 ` Paolo Bonzini 0 siblings, 1 reply; 11+ messages in thread From: Bandan Das @ 2016-07-22 16:04 UTC (permalink / raw) To: Paolo Bonzini; +Cc: Dave Jones, Linux Kernel, Radim Krčmář, kvm 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: RFC: silencing kvm unimplemented msr spew. 2016-07-22 16:04 ` Bandan Das @ 2016-07-22 16:20 ` Paolo Bonzini 2016-07-22 16:37 ` Bandan Das 0 siblings, 1 reply; 11+ messages in thread From: Paolo Bonzini @ 2016-07-22 16:20 UTC (permalink / raw) To: Bandan Das; +Cc: Dave Jones, Linux Kernel, Radim Krčmář, kvm > >> 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: RFC: silencing kvm unimplemented msr spew. 2016-07-22 16:20 ` Paolo Bonzini @ 2016-07-22 16:37 ` Bandan Das 0 siblings, 0 replies; 11+ messages in thread From: Bandan Das @ 2016-07-22 16:37 UTC (permalink / raw) To: Paolo Bonzini; +Cc: Dave Jones, Linux Kernel, Radim Krčmář, kvm 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: RFC: silencing kvm unimplemented msr spew. 2016-07-19 19:58 ` Dave Jones 2016-07-19 20:24 ` Bandan Das @ 2016-07-19 22:12 ` Paolo Bonzini 1 sibling, 0 replies; 11+ messages in thread From: Paolo Bonzini @ 2016-07-19 22:12 UTC (permalink / raw) To: Dave Jones; +Cc: Linux Kernel, Radim Krčmář, kvm > 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2016-07-22 16:38 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-07-15 19:27 RFC: silencing kvm unimplemented msr spew Dave Jones 2016-07-18 15:26 ` Paolo Bonzini 2016-07-19 19:58 ` Dave Jones 2016-07-19 20:24 ` Bandan Das 2016-07-21 20:24 ` Dave Jones 2016-07-21 20:41 ` Bandan Das 2016-07-22 8:48 ` Paolo Bonzini 2016-07-22 16:04 ` Bandan Das 2016-07-22 16:20 ` Paolo Bonzini 2016-07-22 16:37 ` Bandan Das 2016-07-19 22:12 ` Paolo Bonzini
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).