Re: Should KVM_GUEST stop depending on PARAVIRT?

[Peter Zijlstra <peterz@infradead.org>]

On Wed, Jul 29, 2015 at 11:24:09AM +0200, Paolo Bonzini wrote:
> 
> 
> 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.

Reliable CPUID enumeration would be nice indeed. Note that CPUID isn't
ideal either, for instance the CPUID HT bit doesn't indicate if the CPU
has hyperthreading enabled, just that it is able to report extended
topology information.

Determining if a CPU has hyperthreading enabled is extremely difficult,
and basically boils down to init all cpus, see if there are siblings
anywhere.

And then try and distinguish between the case where HT is disabled and
someone hot-unplugged all siblings :-)

> > 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?

Ah, do people really have nothing better to do? ;-) Seems like a petty
complaint.

Sure, it seems our check_hw_exists() does the same thing:

        printk("%sFailed to access perfctr msr (MSR %x is %Lx)\n",
                boot_cpu_has(X86_FEATURE_HYPERVISOR) ? KERN_INFO : KERN_ERR,
                reg, val_new);

so ERR for real hardware, INFO for hypervisor thingies.