Re: [PATCH v2 1/2] KVM: CPUID: Check limit first when emulating CPUID instruction

[Jim Mattson <jmattson@google.com>]

On Tue, Sep 10, 2019 at 3:42 AM Xiaoyao Li <xiaoyao.li@intel.com> wrote:
>
> When limit checking is required, it should be executed first, which is
> consistent with the CPUID specification.
>
> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
> ---
> v2:
>   - correctly set entry_found in no limit checking case.
>
> ---
>  arch/x86/kvm/cpuid.c | 51 ++++++++++++++++++++++++++------------------
>  1 file changed, 30 insertions(+), 21 deletions(-)
>
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index 22c2720cd948..67fa44ab87af 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -952,23 +952,36 @@ struct kvm_cpuid_entry2 *kvm_find_cpuid_entry(struct kvm_vcpu *vcpu,
>  EXPORT_SYMBOL_GPL(kvm_find_cpuid_entry);
>
>  /*
> - * If no match is found, check whether we exceed the vCPU's limit
> - * and return the content of the highest valid _standard_ leaf instead.
> - * This is to satisfy the CPUID specification.
> + * Based on CPUID specification, if leaf number exceeds the vCPU's limit,
> + * it should return the content of the highest valid _standard_ leaf instead.
> + * Note: *found is set true only means the queried leaf number doesn't exceed
> + * the maximum leaf number of basic or extented leaf.

Nit: "extented" should be "extended."

A more serious problem is that the CPUID specification you quote is
Intel's specification. AMD CPUs return zeroes in EAX, EBX, ECX, and
EDX for all undefined leaves, whatever the input value for EAX. This
code is supposed to be vendor-agnostic, right?

>   */
> -static struct kvm_cpuid_entry2* check_cpuid_limit(struct kvm_vcpu *vcpu,
> -                                                  u32 function, u32 index)
> +static struct kvm_cpuid_entry2* cpuid_check_limit(struct kvm_vcpu *vcpu,
> +                                                  u32 function, u32 index,
> +                                                 bool *found)
>  {
>         struct kvm_cpuid_entry2 *maxlevel;
>
> +       if (found)
> +               *found = false;
> +
>         maxlevel = kvm_find_cpuid_entry(vcpu, function & 0x80000000, 0);
> -       if (!maxlevel || maxlevel->eax >= function)
> +       if (!maxlevel)
>                 return NULL;
> -       if (function & 0x80000000) {
> -               maxlevel = kvm_find_cpuid_entry(vcpu, 0, 0);
> -               if (!maxlevel)
> -                       return NULL;
> +
> +       if (maxlevel->eax >= function) {
> +               if (found)
> +                       *found = true;
> +               return kvm_find_cpuid_entry(vcpu, function, index);
>         }
> +
> +       if (function & 0x80000000)
> +               maxlevel = kvm_find_cpuid_entry(vcpu, 0, 0);
> +
> +       if (!maxlevel)
> +               return NULL;
> +
>         return kvm_find_cpuid_entry(vcpu, maxlevel->eax, index);
>  }
>
> @@ -979,24 +992,20 @@ bool kvm_cpuid(struct kvm_vcpu *vcpu, u32 *eax, u32 *ebx,
>         struct kvm_cpuid_entry2 *best;
>         bool entry_found = true;
>
> -       best = kvm_find_cpuid_entry(vcpu, function, index);
> -
> -       if (!best) {
> -               entry_found = false;
> -               if (!check_limit)
> -                       goto out;
> -
> -               best = check_cpuid_limit(vcpu, function, index);
> -       }
> +       if (check_limit)
> +               best = cpuid_check_limit(vcpu, function, index, &entry_found);
> +       else
> +               best = kvm_find_cpuid_entry(vcpu, function, index);
>
> -out:
>         if (best) {
>                 *eax = best->eax;
>                 *ebx = best->ebx;
>                 *ecx = best->ecx;
>                 *edx = best->edx;
> -       } else
> +       } else {
> +               entry_found = false;
>                 *eax = *ebx = *ecx = *edx = 0;
> +       }
>         trace_kvm_cpuid(function, *eax, *ebx, *ecx, *edx, entry_found);
>         return entry_found;
>  }
> --
> 2.19.1
>