Re: [Qemu-devel] [PATCH] x86-KVM: Supply TSC and APIC clock rates to guest like VMWare

[Paolo Bonzini <pbonzini@redhat.com>]

On 18/01/2017 17:02, Phil Dennis-Jordan wrote:
> 
>     One issue is that the TSC frequency can change, for example on
>     migration.  Telling the guest about the TSC frequency makes little sense
>     if it can change.
> 
> That makes sense. Darwin can't handle changing TSC frequencies in any
> case, regardless of cpuid leaf 0x40000010. Do I deduce correctly from
> the following code (lines 967~977) that this bit inhibits migration
> intrinsically, so other than depending on it, I don't need to
> specifically disable migration for this option?

Correct.

>     So the leaf should be conditional on the INVTSC feature
>     (CPUID[0x80000007].EDX bit 8).  You can enable this unconditionally for
>     new machine types (i.e. making it true here, and turning it off in
>     include/hw/i386/pc.h's PC_COMPAT_2_8 macro), but only expose it if that
>     bit is also set.
> 
> Sorry, you've lost me here. Would you mind explaining in a little more
> detail? What would I be enabling unconditionally? (I'm getting lost on
> what the various 'this'/'that'/'it' are referring to.)

You enable vmware-cpuid-freq unconditionally.  But then you actually
publish 0x40000010 only if INVTSC is set.
> 
>     > +    if (cpu->vmware_clock_rates) {
> 
>     ^^ Here is where you should also check invtsc.
> 
>     > +        if (cpu->expose_kvm
> 
>     I think this should not depend on cpu->expose_kvm.  This is not a KVM
>     leaf, it's a vmware leaf; if it were a KVM leaf, it would obey kvm_base.
>      Of course checking kvm_base is still a good idea, to avoid stomping on
>     Hyper-V's CPUID space.
> 
> Hmm, my thinking here is that leaf 0x40000000 only is published if kvm
> or Hyper-V is exposed. Without 0x40000000, Darwin won't find 0x40000010.

Of course you're right, but please add a comment like this:

	/* Guests depend on 0x40000000 to detect this, so do not expose
	 * it unless that leaf is present.
	 */

Paolo