Re: [PATCH 2/6] KVM: x86: Fix CPUID range check for Centaur and Hypervisor ranges

From: Jim Mattson <jmattson@google.com>
To: Sean Christopherson <sean.j.christopherson@intel.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>,
	Vitaly Kuznetsov <vkuznets@redhat.com>,
	Wanpeng Li <wanpengli@tencent.com>,
	Joerg Roedel <joro@8bytes.org>, kvm list <kvm@vger.kernel.org>,
	LKML <linux-kernel@vger.kernel.org>,
	Jan Kiszka <jan.kiszka@siemens.com>,
	Xiaoyao Li <xiaoyao.li@intel.com>
Subject: Re: [PATCH 2/6] KVM: x86: Fix CPUID range check for Centaur and Hypervisor ranges
Date: Tue, 3 Mar 2020 09:42:42 -0800	[thread overview]
Message-ID: <CALMp9eSYZKUBko4ZViNbasRGJs2bAO2fREHX9maDbLrYj8yDhQ@mail.gmail.com> (raw)
In-Reply-To: <20200303045838.GF27842@linux.intel.com>

Unfathomable was the wrong word. I can see what you're trying to do. I
just don't think it's defensible. I suspect that Intel CPU architects
will be surprised and disappointed to find that the maximum effective
value of CPUID.0H:EAX is now 255, and that they have to define
CPUID.100H:EAX as the "maximum leaf between 100H and 1FFH" if they
want to define any leaves between 100H and 1FFH.

Furthermore, AMD has only ceded 4000_0000h through 4000_00FFh to
hypervisors, so kvm's use of 40000100H through 400001FFH appears to be
a land grab, akin to VIA's unilateral grab of the C0000000H leaves.
Admittedly, one could argue that the 40000000H leaves are not AMD's to
apportion, since AMD and Intel appear to have reached a detente by
splitting the available space down the middle. Intel, who seems to be
the recognized authority for this range, declares the entire range
from 40000000H through 4FFFFFFFH to be invalid. Make of that what you
will.

In any event, no one has ever documented what's supposed to happen if
you leave gaps in the 4xxxxxxxH range when defining synthesized CPUID
leaves under kvm.

On Mon, Mar 2, 2020 at 8:58 PM Sean Christopherson
<sean.j.christopherson@intel.com> wrote:
>
> On Mon, Mar 02, 2020 at 08:25:31PM -0800, Jim Mattson wrote:
> > On Mon, Mar 2, 2020 at 7:25 PM Jim Mattson <jmattson@google.com> wrote:
> > >
> > > On Mon, Mar 2, 2020 at 11:57 AM Sean Christopherson
> > > <sean.j.christopherson@intel.com> wrote:
> > >
> > > > The bad behavior can be visually confirmed by dumping CPUID output in
> > > > the guest when running Qemu with a stable TSC, as Qemu extends the limit
> > > > of range 0x40000000 to 0x40000010 to advertise VMware's cpuid_freq,
> > > > without defining zeroed entries for 0x40000002 - 0x4000000f.
> > >
> > > I think it could be reasonably argued that this is a userspace bug.
> > > Clearly, when userspace explicitly supplies the results for a leaf,
> > > those results override the default CPUID values for that leaf. But I
> > > haven't seen it documented anywhere that leaves *not* explicitly
> > > supplied by userspace will override the default CPUID values, just
> > > because they happen to appear in some magic range.
> >
> > In fact, the more I think about it, the original change is correct, at
> > least in this regard. Your "fix" introduces undocumented and
> > unfathomable behavior.
>
> Heh, the takeaway from this is that whatever we decide on needs to be
> documented somewhere :-)
>
> I wouldn't say it's unfathomable, conceptually it seems like the intent
> of the hypervisor range was to mimic the basic and extended ranges.  The
> whole thing is arbitrary behavior.  Of course if Intel CPUs would just
> return 0s on undefined leafs it would be a lot less arbitrary :-)
>
> Anyways, I don't have a strong opinion on whether this patch stays or goes.