On Thu, Dec 12, 2019 at 01:43:27AM +0100, Paolo Bonzini wrote: > On 02/12/19 22:21, Sean Christopherson wrote: > > As for the original code, arguably it *should* do a full WARN and not > > simply log the error, as kvm_set_msr() should never fail if > > VM_EXIT_LOAD_IA32_PERF_GLOBAL_CTRL was exposed to L1, unlike the above two > > cases where KVM is processing an L1-controlled MSR list, e.g.: > > > > if (vmcs12->vm_exit_controls & VM_EXIT_LOAD_IA32_PERF_GLOBAL_CTRL) > > WARN_ON_ONCE(kvm_set_msr(vcpu, MSR_CORE_PERF_GLOBAL_CTRL, > > vmcs12->host_ia32_perf_global_ctrl)); > > > > Back to this patch, this isn't simply consolidating code, it's promoting > > L1-controlled messages from pr_debug() to pr_warn(). > > > > What if you add a patch to remove SET_MSR_OR_WARN() and instead manually > > do the WARN_ON_ONCE() as above, and then introduce a new macro to > > consolidate the pr_debug_ratelimited() stuff in this patch? Sean, Thank you for the detailed review of this patch (as well as the last one that I snuck past you :-P). I'm in complete agreement with your sentiments, a follow-up is in order. I'll get that out soon. > Should go without saying (Sean is a Certified Reviewer according to > MAINTAINERS :)) but I agree. > > Paolo Sean has been a great help in providing detailed reviews -- well deserving of the designation! Thank you for pinging this thread, Paolo. -- Best, Oliver
