[PATCH v2 0/2] KVM: CPUID: emulation flow adjustment and one minor refinement when updating maxphyaddr

[PATCH v2 0/2] KVM: CPUID: emulation flow adjustment and one minor refinement when updating maxphyaddr

[Xiaoyao Li]

Patch 1 adjusts the execution flow of CPUID instruction emulation, which
checks the leaf number first per CPUID specification.

Patch 2 moves physical address width updating to where we check the virtual
address width in function kvm_update_cpuid() since they two use the same
cpuid leaf, which makes it more reasonable and no functional change.  

Xiaoyao Li (2):
  KVM: CPUID: Check limit first when emulating CPUID instruction
  KVM: CPUID: Put maxphyaddr updating together with virtual address
    width checking

 arch/x86/kvm/cpuid.c | 57 ++++++++++++++++++++++++++------------------
 1 file changed, 34 insertions(+), 23 deletions(-)

-- 
2.19.1

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

[Xiaoyao Li]

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.
  */
-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

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

[Jim Mattson]

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
>

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

[Xiaoyao Li]

On 9/11/2019 1:00 AM, Jim Mattson wrote:
> 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?
> 

I checked the AMD spec and I didn't find the statement about "AMD CPUs 
return zeroes in EAX, EBX, ECX, and EDX for all undefined leaves". I 
don't have AMD machine at hand so that I can't test it to verify it.

Assume what you said about AMD CPUs is true, then the current codes in 
KVM makes AMD guest act as Intel CPU that returns the highest valid 
standard leaf if input value of EAX exceeds the limit.

Anyway, I find we cannot check the limit first for guest, otherwise the 
leaves 0x4000XXXX will be not readable. So please just ignore this patch.

[PATCH v2 2/2] KVM: CPUID: Put maxphyaddr updating together with virtual address width checking

[Xiaoyao Li]

Since both of maxphyaddr updating and virtual address width checking
need to query the cpuid leaf 0x80000008. We can put them together.

Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
---
 arch/x86/kvm/cpuid.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index 67fa44ab87af..fd0a66079001 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -118,6 +118,7 @@ int kvm_update_cpuid(struct kvm_vcpu *vcpu)
 		best->ebx = xstate_required_size(vcpu->arch.xcr0, true);
 
 	/*
+	 * Update physical address width and check virtual address width.
 	 * The existing code assumes virtual address is 48-bit or 57-bit in the
 	 * canonical address checks; exit if it is ever changed.
 	 */
@@ -127,7 +128,10 @@ int kvm_update_cpuid(struct kvm_vcpu *vcpu)
 
 		if (vaddr_bits != 48 && vaddr_bits != 57 && vaddr_bits != 0)
 			return -EINVAL;
+
+		vcpu->arch.maxphyaddr = best->eax & 0xff;
 	}
+	vcpu->arch.maxphyaddr = 36;
 
 	best = kvm_find_cpuid_entry(vcpu, KVM_CPUID_FEATURES, 0);
 	if (kvm_hlt_in_guest(vcpu->kvm) && best &&
@@ -144,8 +148,6 @@ int kvm_update_cpuid(struct kvm_vcpu *vcpu)
 		}
 	}
 
-	/* Update physical-address width */
-	vcpu->arch.maxphyaddr = cpuid_query_maxphyaddr(vcpu);
 	kvm_mmu_reset_context(vcpu);
 
 	kvm_pmu_refresh(vcpu);
-- 
2.19.1

Re: [PATCH v2 2/2] KVM: CPUID: Put maxphyaddr updating together with virtual address width checking

[Jim Mattson]

On Tue, Sep 10, 2019 at 3:42 AM Xiaoyao Li <xiaoyao.li@intel.com> wrote:
>
> Since both of maxphyaddr updating and virtual address width checking
> need to query the cpuid leaf 0x80000008. We can put them together.
>
> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
> ---
>  arch/x86/kvm/cpuid.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index 67fa44ab87af..fd0a66079001 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -118,6 +118,7 @@ int kvm_update_cpuid(struct kvm_vcpu *vcpu)
>                 best->ebx = xstate_required_size(vcpu->arch.xcr0, true);
>
>         /*
> +        * Update physical address width and check virtual address width.
>          * The existing code assumes virtual address is 48-bit or 57-bit in the
>          * canonical address checks; exit if it is ever changed.
>          */
> @@ -127,7 +128,10 @@ int kvm_update_cpuid(struct kvm_vcpu *vcpu)
>
>                 if (vaddr_bits != 48 && vaddr_bits != 57 && vaddr_bits != 0)
>                         return -EINVAL;
> +
> +               vcpu->arch.maxphyaddr = best->eax & 0xff;
>         }
> +       vcpu->arch.maxphyaddr = 36;

Perhaps I'm missing something, but it looks to me like you always set
vcpu->arch.maxphyaddr to 36, regardless of what may be enumerated by
leaf 0x80000008.

Is there really much of an advantage to open-coding
cpuid_query_maxphyaddr() here?

>         best = kvm_find_cpuid_entry(vcpu, KVM_CPUID_FEATURES, 0);
>         if (kvm_hlt_in_guest(vcpu->kvm) && best &&
> @@ -144,8 +148,6 @@ int kvm_update_cpuid(struct kvm_vcpu *vcpu)
>                 }
>         }
>
> -       /* Update physical-address width */
> -       vcpu->arch.maxphyaddr = cpuid_query_maxphyaddr(vcpu);
>         kvm_mmu_reset_context(vcpu);
>
>         kvm_pmu_refresh(vcpu);
> --
> 2.19.1
>

Re: [PATCH v2 2/2] KVM: CPUID: Put maxphyaddr updating together with virtual address width checking

[Xiaoyao Li]

On Tue, 2019-09-10 at 10:13 -0700, Jim Mattson wrote:
> On Tue, Sep 10, 2019 at 3:42 AM Xiaoyao Li <xiaoyao.li@intel.com> wrote:
> > 
> > Since both of maxphyaddr updating and virtual address width checking
> > need to query the cpuid leaf 0x80000008. We can put them together.
> > 
> > Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
> > ---
> >  arch/x86/kvm/cpuid.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> > index 67fa44ab87af..fd0a66079001 100644
> > --- a/arch/x86/kvm/cpuid.c
> > +++ b/arch/x86/kvm/cpuid.c
> > @@ -118,6 +118,7 @@ int kvm_update_cpuid(struct kvm_vcpu *vcpu)
> >                 best->ebx = xstate_required_size(vcpu->arch.xcr0, true);
> > 
> >         /*
> > +        * Update physical address width and check virtual address width.
> >          * The existing code assumes virtual address is 48-bit or 57-bit in
> > the
> >          * canonical address checks; exit if it is ever changed.
> >          */
> > @@ -127,7 +128,10 @@ int kvm_update_cpuid(struct kvm_vcpu *vcpu)
> > 
> >                 if (vaddr_bits != 48 && vaddr_bits != 57 && vaddr_bits != 0)
> >                         return -EINVAL;
> > +
> > +               vcpu->arch.maxphyaddr = best->eax & 0xff;
> >         }
> > +       vcpu->arch.maxphyaddr = 36;
> 
> Perhaps I'm missing something, but it looks to me like you always set
> vcpu->arch.maxphyaddr to 36, regardless of what may be enumerated by
> leaf 0x80000008.

Oh, I made a stupid mistake. It should be included in the else case.
 
> 
> Is there really much of an advantage to open-coding
> cpuid_query_maxphyaddr() here?

Indeed not so much.
It can avoid two more kvm_find_cpuid_entry() calling that we don't handle leaf
0x80000008 twice in two place. 

> >         best = kvm_find_cpuid_entry(vcpu, KVM_CPUID_FEATURES, 0);
> >         if (kvm_hlt_in_guest(vcpu->kvm) && best &&
> > @@ -144,8 +148,6 @@ int kvm_update_cpuid(struct kvm_vcpu *vcpu)
> >                 }
> >         }
> > 
> > -       /* Update physical-address width */
> > -       vcpu->arch.maxphyaddr = cpuid_query_maxphyaddr(vcpu);
> >         kvm_mmu_reset_context(vcpu);
> > 
> >         kvm_pmu_refresh(vcpu);
> > --
> > 2.19.1
> > 

Re: [PATCH v2 2/2] KVM: CPUID: Put maxphyaddr updating together with virtual address width checking

[Jim Mattson]

On Tue, Sep 10, 2019 at 3:52 PM Xiaoyao Li <xiaoyao.li@intel.com> wrote:
>
> On Tue, 2019-09-10 at 10:13 -0700, Jim Mattson wrote:
> > On Tue, Sep 10, 2019 at 3:42 AM Xiaoyao Li <xiaoyao.li@intel.com> wrote:
> > >
> > > Since both of maxphyaddr updating and virtual address width checking
> > > need to query the cpuid leaf 0x80000008. We can put them together.
> > >
> > > Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
> > > ---
> > >  arch/x86/kvm/cpuid.c | 6 ++++--
> > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> > > index 67fa44ab87af..fd0a66079001 100644
> > > --- a/arch/x86/kvm/cpuid.c
> > > +++ b/arch/x86/kvm/cpuid.c
> > > @@ -118,6 +118,7 @@ int kvm_update_cpuid(struct kvm_vcpu *vcpu)
> > >                 best->ebx = xstate_required_size(vcpu->arch.xcr0, true);
> > >
> > >         /*
> > > +        * Update physical address width and check virtual address width.
> > >          * The existing code assumes virtual address is 48-bit or 57-bit in
> > > the
> > >          * canonical address checks; exit if it is ever changed.
> > >          */
> > > @@ -127,7 +128,10 @@ int kvm_update_cpuid(struct kvm_vcpu *vcpu)
> > >
> > >                 if (vaddr_bits != 48 && vaddr_bits != 57 && vaddr_bits != 0)
> > >                         return -EINVAL;
> > > +
> > > +               vcpu->arch.maxphyaddr = best->eax & 0xff;
> > >         }
> > > +       vcpu->arch.maxphyaddr = 36;
> >
> > Perhaps I'm missing something, but it looks to me like you always set
> > vcpu->arch.maxphyaddr to 36, regardless of what may be enumerated by
> > leaf 0x80000008.
>
> Oh, I made a stupid mistake. It should be included in the else case.
>
> >
> > Is there really much of an advantage to open-coding
> > cpuid_query_maxphyaddr() here?
>
> Indeed not so much.
> It can avoid two more kvm_find_cpuid_entry() calling that we don't handle leaf
> 0x80000008 twice in two place.

I'm inclined to leave things as they are. As your previous attempt
demonstrates, having the same logic replicated in multiple places is
prone to error. I can't imagine that this would be a
performance-sensitive path.

> > >         best = kvm_find_cpuid_entry(vcpu, KVM_CPUID_FEATURES, 0);
> > >         if (kvm_hlt_in_guest(vcpu->kvm) && best &&
> > > @@ -144,8 +148,6 @@ int kvm_update_cpuid(struct kvm_vcpu *vcpu)
> > >                 }
> > >         }
> > >
> > > -       /* Update physical-address width */
> > > -       vcpu->arch.maxphyaddr = cpuid_query_maxphyaddr(vcpu);
> > >         kvm_mmu_reset_context(vcpu);
> > >
> > >         kvm_pmu_refresh(vcpu);
> > > --
> > > 2.19.1
> > >
>