On 29/07/2015 11:19, Peter Zijlstra wrote: > On Tue, Jul 28, 2015 at 05:23:22PM -0700, Andy Lutomirski wrote: >> PeterZ, can we fix this for real instead of relying on >> CONFIG_PARAVIRT=y accidentally turning all msr accesses into "safe" >> accesses? We have the CPUID "hypervisor" bit, but I still don't fully >> understand the problem. > > So a whole bunch of PMU drivers already probe the MSRs at init time > using a combination of rdmsrl_safe() and wrmsrl_safe() to see if: > > 1) its there at all > 2) if its there, it 'works' like it ought to > > See for example: arch/x86/kernel/cpu/perf_event.c:check_hw_exists(). Great. Of course it's even better if you can probe it with CPUID (e.g. aperfmperf in intel_pstate.c), then there's no need for the _safe variant. > I have no problem using the _safe() methods to probe MSRs in init > routines and failing the init of the driver etc. > > What I do object to is sprinkling _safe() all over the driver itself and > creating horrid error paths all over (yes, people have proposed crazy > things like that). > > Another example is the LBR probing in > arch/x86/kernel/cpu/perf_event_intel.c:intel_pmu_init(). This is the one > people wanted to use _safe() crud for all throughout the driver, instead > of simply disabling the LBR at init time. I agree that check_msr is okay. > As to the 'bug' at hand, I really don't see the problem. The RAPL driver > says there's no valid RAPL domains, this is true, there aren't any on > the virtual machine, so what? Well, people have complained about it because it's KERN_ERR. Do you think it is okay to downgrade this (perhaps not even just on VMs) to info? Paolo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
