Re: [RFC][PATCH] kvm: x86: Improve emulation of CPUID leaves 0BH and 1FH

[Xiaoyao Li <xiaoyao.li@intel.com>]

On 9/19/2019 2:41 AM, Jim Mattson wrote:
> On Wed, Sep 18, 2019 at 11:22 AM Jim Mattson <jmattson@google.com> wrote:
[...]>>>
>>> If I'm reading the code correctly, this is fragile and subtle.  The order
>>> of cpuid entries is controlled by userspace, which means that clearing
>>> KVM_CPUID_FLAG_SIGNIFCANT_INDEX depends on this entry being kept after all
>>> other entries for this function.  In practice I'm guessing userspaces
>>> naturally sort entries with the same function by ascending index, but it
>>> seems like avoidable issue.
>>
>> Though not documented, the CPUID leaf matching code has always
>> depended on ordering.
>>
>>> Also, won't matching the last entry generate the wrong values for EAX, EBX
>>> and ECX, i.e. the valid values for the last index instead of zeroes?
>>
>> This entry has CH==0. According to the SDM, "For sub-leaves that
>> return an invalid level-type of 0 in ECX[15:8]; EAX and EBX will
>> return 0."
>> ECX[7:0] will be wrong, but that's fixed up by the flag below.
>> ECX[31:16] are reserved and perhaps should be cleared here, but I'm
>> not sure how I would interpret it if those bits started being non-zero
>> for the first leaf with CH==0.
>>
>>>> +                     entry[i - 1].flags |= KVM_CPUID_FLAG_CL_IS_PASSTHROUGH;
>>>
>>> Lastly, do we actually need to enumerate this silliness to userspace?
>>> What if we handle this as a one-off case in CPUID emulation and avoid the
>>> ABI breakage that way?  E.g.:
>>>
>>> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
>>> index dd5985eb61b4..aaf5cdcb88c9 100644
>>> --- a/arch/x86/kvm/cpuid.c
>>> +++ b/arch/x86/kvm/cpuid.c
>>> @@ -1001,6 +1001,16 @@ bool kvm_cpuid(struct kvm_vcpu *vcpu, u32 *eax, u32 *ebx,
>>>          }
>>>
>>>   out:
>>> +       if (!best && (function == 0xb || function == 0x1f)) {
>>> +               best = check_cpuid_limit(vcpu, function, 0);
>>> +               if (best) {
>>> +                       *eax = 0;
>>> +                       *ebx = 0;
>>> +                       *ecx &= 0xff;
>>> +                       *edx = *best->edx;
>>> +               }
>>> +       }
>>> +
>>

Hi Sean,
your proposal above seems not work. check_cpuid_limit(vcpu, 0xb/01f, 0) 
always return NULL, if there are valid 0xb/0x1f leaves in 
vcpu->arch.cpuid_entries[] (maxBaiscleaf >= 0xb/0x1f)

>> Aside from the fact that one should never call check_cpuid_limit on
>> AMD systems (they don't do the "last basic leaf" nonsense), an
>> approach like this should work.
> 
> The above proposal doesn't correctly handle a leaf outside of ([0,
> maxBasicLeaf] union [80000000H, maxExtendedLeaf]) , where maxBasicLeaf
> == (0BH or 1FH) on Intel hardware...but it could be fixed up to do so,
> if hard-coding this behavior in kvm_cpuid() is preferable to the API
> breakage.
> 

I vote for Sean's one-off case, how about something like this:

diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index 22c2720cd948..6af5febf7b12 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -976,11 +976,23 @@ bool kvm_cpuid(struct kvm_vcpu *vcpu, u32 *eax, 
u32 *ebx,
                u32 *ecx, u32 *edx, bool check_limit)
  {
         u32 function = *eax, index = *ecx;
-       struct kvm_cpuid_entry2 *best;
+       struct kvm_cpuid_entry2 *best, tmp;
         bool entry_found = true;

         best = kvm_find_cpuid_entry(vcpu, function, index);

+       if (!best && (fuction == 0xb || function == 0x1f) && index > 0) {
+               best = kvm_find_cpuid_entry(vcpu, function, 0);
+               if (best) {
+                       tmp.eax = 0;
+                       tmp.ebx = 0;
+                       tmp.ecx = index & 0xff;
+                       tmp.edx = best->edx;
+                       best = &tmp;
+                       goto out;
+               }
+       }
+