Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1697986 > unrolled thread

[PATCH] KVM: nVMX: INVPCID support

Started byPaolo Bonzini <pbonzini@redhat.com>
First post2017-07-27 15:30 +0200
Last post2017-07-27 21:10 +0200
Articles 3 — 2 participants

Back to article view | Back to linux.kernel

Contents

  [PATCH] KVM: nVMX: INVPCID support Paolo Bonzini <pbonzini@redhat.com> - 2017-07-27 15:30 +0200
    Re: [PATCH] KVM: nVMX: INVPCID support David Hildenbrand <david@redhat.com> - 2017-07-27 20:30 +0200
      Re: [PATCH] KVM: nVMX: INVPCID support Paolo Bonzini <pbonzini@redhat.com> - 2017-07-27 21:10 +0200

#1697986 — [PATCH] KVM: nVMX: INVPCID support

FromPaolo Bonzini <pbonzini@redhat.com>
Date2017-07-27 15:30 +0200
Subject[PATCH] KVM: nVMX: INVPCID support
Message-ID<u7R1x-6CD-25@gated-at.bofh.it>
Expose the "Enable INVPCID" secondary execution control to the guest
and properly reflect the exit reason.

In addition, before this patch the guest was always running with
INVPCID enabled, causing pcid.flat's "Test on INVPCID when disabled"
test to fail.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch/x86/kvm/vmx.c | 34 +++++++++++++++++++++++++---------
 1 file changed, 25 insertions(+), 9 deletions(-)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index ed43fd824264..9c3c6c524430 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -8189,6 +8189,10 @@ static bool nested_vmx_exit_reflected(struct kvm_vcpu *vcpu, u32 exit_reason)
 		 * table is L0's fault.
 		 */
 		return false;
+	case EXIT_REASON_INVPCID:
+		return
+			nested_cpu_has2(vmcs12, SECONDARY_EXEC_ENABLE_INVPCID) &&
+			nested_cpu_has(vmcs12, CPU_BASED_INVLPG_EXITING);
 	case EXIT_REASON_WBINVD:
 		return nested_cpu_has2(vmcs12, SECONDARY_EXEC_WBINVD_EXITING);
 	case EXIT_REASON_XSETBV:
@@ -9440,7 +9444,6 @@ static void nested_vmx_cr_fixed1_bits_update(struct kvm_vcpu *vcpu)
 
 static void vmx_cpuid_update(struct kvm_vcpu *vcpu)
 {
-	struct kvm_cpuid_entry2 *best;
 	struct vcpu_vmx *vmx = to_vmx(vcpu);
 	u32 secondary_exec_ctl = vmx_secondary_exec_control(vmx);
 
@@ -9459,15 +9462,27 @@ static void vmx_cpuid_update(struct kvm_vcpu *vcpu)
 		}
 	}
 
-	/* Exposing INVPCID only when PCID is exposed */
-	best = kvm_find_cpuid_entry(vcpu, 0x7, 0);
-	if (vmx_invpcid_supported() &&
-	    (!best || !(best->ebx & bit(X86_FEATURE_INVPCID)) ||
-	    !guest_cpuid_has_pcid(vcpu))) {
-		secondary_exec_ctl &= ~SECONDARY_EXEC_ENABLE_INVPCID;
+	if (vmx_invpcid_supported()) {
+		/* Exposing INVPCID only when PCID is exposed */
+		struct kvm_cpuid_entry2 *best = kvm_find_cpuid_entry(vcpu, 0x7, 0);
+		bool invpcid_enabled =
+			best && best->ebx & bit(X86_FEATURE_INVPCID) &&
+			guest_cpuid_has_pcid(vcpu);
 
-		if (best)
-			best->ebx &= ~bit(X86_FEATURE_INVPCID);
+		if (!invpcid_enabled) {
+			secondary_exec_ctl &= ~SECONDARY_EXEC_ENABLE_INVPCID;
+			if (best)
+				best->ebx &= ~bit(X86_FEATURE_INVPCID);
+		}
+
+		if (nested) {
+			if (invpcid_enabled)
+				vmx->nested.nested_vmx_secondary_ctls_high |=
+					SECONDARY_EXEC_ENABLE_INVPCID;
+			else
+				vmx->nested.nested_vmx_secondary_ctls_high &=
+					~SECONDARY_EXEC_ENABLE_INVPCID;
+		}
 	}
 
 	if (cpu_has_secondary_exec_ctrls())
@@ -10175,6 +10190,7 @@ static int prepare_vmcs02(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12,
 
 		/* Take the following fields only from vmcs12 */
 		exec_control &= ~(SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES |
+				  SECONDARY_EXEC_ENABLE_INVPCID |
 				  SECONDARY_EXEC_RDTSCP |
 				  SECONDARY_EXEC_VIRTUAL_INTR_DELIVERY |
 				  SECONDARY_EXEC_APIC_REGISTER_VIRT);
-- 
1.8.3.1

[toc] | [next] | [standalone]

#1698233

FromDavid Hildenbrand <david@redhat.com>
Date2017-07-27 20:30 +0200
Message-ID<u7VHP-15C-1@gated-at.bofh.it>
In reply to#1697986
On 27.07.2017 15:20, Paolo Bonzini wrote:
> Expose the "Enable INVPCID" secondary execution control to the guest
> and properly reflect the exit reason.
> 
> In addition, before this patch the guest was always running with
> INVPCID enabled, causing pcid.flat's "Test on INVPCID when disabled"
> test to fail.

Did you wanted to send "KVM: nVMX: do not fill vm_exit_intr_error_code
in prepare_vmcs12" I can spot on kvm/queue? (but sent this patch twice
instead?)

> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  arch/x86/kvm/vmx.c | 34 +++++++++++++++++++++++++---------
>  1 file changed, 25 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index ed43fd824264..9c3c6c524430 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -8189,6 +8189,10 @@ static bool nested_vmx_exit_reflected(struct kvm_vcpu *vcpu, u32 exit_reason)
>  		 * table is L0's fault.
>  		 */
>  		return false;
> +	case EXIT_REASON_INVPCID:
> +		return
> +			nested_cpu_has2(vmcs12, SECONDARY_EXEC_ENABLE_INVPCID) &&
> +			nested_cpu_has(vmcs12, CPU_BASED_INVLPG_EXITING);

(why the extra line after the return ?)

>  	case EXIT_REASON_WBINVD:
>  		return nested_cpu_has2(vmcs12, SECONDARY_EXEC_WBINVD_EXITING);
>  	case EXIT_REASON_XSETBV:
> @@ -9440,7 +9444,6 @@ static void nested_vmx_cr_fixed1_bits_update(struct kvm_vcpu *vcpu)
>  
>  static void vmx_cpuid_update(struct kvm_vcpu *vcpu)
>  {
> -	struct kvm_cpuid_entry2 *best;
>  	struct vcpu_vmx *vmx = to_vmx(vcpu);
>  	u32 secondary_exec_ctl = vmx_secondary_exec_control(vmx);
>  
> @@ -9459,15 +9462,27 @@ static void vmx_cpuid_update(struct kvm_vcpu *vcpu)
>  		}
>  	}
>  
> -	/* Exposing INVPCID only when PCID is exposed */
> -	best = kvm_find_cpuid_entry(vcpu, 0x7, 0);
> -	if (vmx_invpcid_supported() &&
> -	    (!best || !(best->ebx & bit(X86_FEATURE_INVPCID)) ||
> -	    !guest_cpuid_has_pcid(vcpu))) {
> -		secondary_exec_ctl &= ~SECONDARY_EXEC_ENABLE_INVPCID;
> +	if (vmx_invpcid_supported()) {
> +		/* Exposing INVPCID only when PCID is exposed */
> +		struct kvm_cpuid_entry2 *best = kvm_find_cpuid_entry(vcpu, 0x7, 0);
> +		bool invpcid_enabled =
> +			best && best->ebx & bit(X86_FEATURE_INVPCID) &&

I thought parentheses are recommended around &, but I am usually wrong
about these things :)

> +			guest_cpuid_has_pcid(vcpu);
>  
> -		if (best)
> -			best->ebx &= ~bit(X86_FEATURE_INVPCID);
> +		if (!invpcid_enabled) {



> +			secondary_exec_ctl &= ~SECONDARY_EXEC_ENABLE_INVPCID;
> +			if (best)

(thinking loud: only relevant if !guest_cpuid_has_pcid(vcpu))

> +				best->ebx &= ~bit(X86_FEATURE_INVPCID);
> +		}

Can't we rewrite that a little bit, avoiding that "best" handling
(introducing guest_cpuid_disable_invpcid() and guest_cpuid_has_invpcid())

bool invpcid_enabled = guest_cpuid_has_pcid(vcpu) &&
		       guest_cpuid_has_invpcid();

if (!invpcid_enabled) {
	secondary_exec_ctl &= ~SECONDARY_EXEC_ENABLE_INVPCID;
	/* make sure there is no no INVPCID without PCID */
	guest_cpuid_disable_invpcid(vcpu);
}

if (nested) {
	...

> +
> +		if (nested) {
> +			if (invpcid_enabled)
> +				vmx->nested.nested_vmx_secondary_ctls_high |=
> +					SECONDARY_EXEC_ENABLE_INVPCID;
> +			else
> +				vmx->nested.nested_vmx_secondary_ctls_high &=
> +					~SECONDARY_EXEC_ENABLE_INVPCID;
> +		}
>  	}
>  
>  	if (cpu_has_secondary_exec_ctrls())
> @@ -10175,6 +10190,7 @@ static int prepare_vmcs02(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12,
>  
>  		/* Take the following fields only from vmcs12 */
>  		exec_control &= ~(SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES |
> +				  SECONDARY_EXEC_ENABLE_INVPCID |
>  				  SECONDARY_EXEC_RDTSCP |
>  				  SECONDARY_EXEC_VIRTUAL_INTR_DELIVERY |
>  				  SECONDARY_EXEC_APIC_REGISTER_VIRT);
> 

Makes sense to me!

-- 

Thanks,

David

[toc] | [prev] | [next] | [standalone]

#1698253

FromPaolo Bonzini <pbonzini@redhat.com>
Date2017-07-27 21:10 +0200
Message-ID<u7Wkx-1Bk-1@gated-at.bofh.it>
In reply to#1698233

----- Original Message -----
> From: "David Hildenbrand" <david@redhat.com>
> To: "Paolo Bonzini" <pbonzini@redhat.com>, linux-kernel@vger.kernel.org, kvm@vger.kernel.org
> Sent: Thursday, July 27, 2017 8:29:21 PM
> Subject: Re: [PATCH] KVM: nVMX: INVPCID support
> 
> On 27.07.2017 15:20, Paolo Bonzini wrote:
> > Expose the "Enable INVPCID" secondary execution control to the guest
> > and properly reflect the exit reason.
> > 
> > In addition, before this patch the guest was always running with
> > INVPCID enabled, causing pcid.flat's "Test on INVPCID when disabled"
> > test to fail.
> 
> Did you wanted to send "KVM: nVMX: do not fill vm_exit_intr_error_code
> in prepare_vmcs12" I can spot on kvm/queue? (but sent this patch twice
> instead?)

Yes, but it was two separate mistakes not one. :)

I forgot I had sent this one already *and* I also forgot to send the other.

Paolo

> > 
> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > ---
> >  arch/x86/kvm/vmx.c | 34 +++++++++++++++++++++++++---------
> >  1 file changed, 25 insertions(+), 9 deletions(-)
> > 
> > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> > index ed43fd824264..9c3c6c524430 100644
> > --- a/arch/x86/kvm/vmx.c
> > +++ b/arch/x86/kvm/vmx.c
> > @@ -8189,6 +8189,10 @@ static bool nested_vmx_exit_reflected(struct
> > kvm_vcpu *vcpu, u32 exit_reason)
> >  		 * table is L0's fault.
> >  		 */
> >  		return false;
> > +	case EXIT_REASON_INVPCID:
> > +		return
> > +			nested_cpu_has2(vmcs12, SECONDARY_EXEC_ENABLE_INVPCID) &&
> > +			nested_cpu_has(vmcs12, CPU_BASED_INVLPG_EXITING);
> 
> (why the extra line after the return ?)
> 
> >  	case EXIT_REASON_WBINVD:
> >  		return nested_cpu_has2(vmcs12, SECONDARY_EXEC_WBINVD_EXITING);
> >  	case EXIT_REASON_XSETBV:
> > @@ -9440,7 +9444,6 @@ static void nested_vmx_cr_fixed1_bits_update(struct
> > kvm_vcpu *vcpu)
> >  
> >  static void vmx_cpuid_update(struct kvm_vcpu *vcpu)
> >  {
> > -	struct kvm_cpuid_entry2 *best;
> >  	struct vcpu_vmx *vmx = to_vmx(vcpu);
> >  	u32 secondary_exec_ctl = vmx_secondary_exec_control(vmx);
> >  
> > @@ -9459,15 +9462,27 @@ static void vmx_cpuid_update(struct kvm_vcpu *vcpu)
> >  		}
> >  	}
> >  
> > -	/* Exposing INVPCID only when PCID is exposed */
> > -	best = kvm_find_cpuid_entry(vcpu, 0x7, 0);
> > -	if (vmx_invpcid_supported() &&
> > -	    (!best || !(best->ebx & bit(X86_FEATURE_INVPCID)) ||
> > -	    !guest_cpuid_has_pcid(vcpu))) {
> > -		secondary_exec_ctl &= ~SECONDARY_EXEC_ENABLE_INVPCID;
> > +	if (vmx_invpcid_supported()) {
> > +		/* Exposing INVPCID only when PCID is exposed */
> > +		struct kvm_cpuid_entry2 *best = kvm_find_cpuid_entry(vcpu, 0x7, 0);
> > +		bool invpcid_enabled =
> > +			best && best->ebx & bit(X86_FEATURE_INVPCID) &&
> 
> I thought parentheses are recommended around &, but I am usually wrong
> about these things :)
> 
> > +			guest_cpuid_has_pcid(vcpu);
> >  
> > -		if (best)
> > -			best->ebx &= ~bit(X86_FEATURE_INVPCID);
> > +		if (!invpcid_enabled) {
> 
> 
> 
> > +			secondary_exec_ctl &= ~SECONDARY_EXEC_ENABLE_INVPCID;
> > +			if (best)
> 
> (thinking loud: only relevant if !guest_cpuid_has_pcid(vcpu))
> 
> > +				best->ebx &= ~bit(X86_FEATURE_INVPCID);
> > +		}
> 
> Can't we rewrite that a little bit, avoiding that "best" handling
> (introducing guest_cpuid_disable_invpcid() and guest_cpuid_has_invpcid())
> 
> bool invpcid_enabled = guest_cpuid_has_pcid(vcpu) &&
> 		       guest_cpuid_has_invpcid();
> 
> if (!invpcid_enabled) {
> 	secondary_exec_ctl &= ~SECONDARY_EXEC_ENABLE_INVPCID;
> 	/* make sure there is no no INVPCID without PCID */
> 	guest_cpuid_disable_invpcid(vcpu);
> }
> 
> if (nested) {
> 	...
> 
> > +
> > +		if (nested) {
> > +			if (invpcid_enabled)
> > +				vmx->nested.nested_vmx_secondary_ctls_high |=
> > +					SECONDARY_EXEC_ENABLE_INVPCID;
> > +			else
> > +				vmx->nested.nested_vmx_secondary_ctls_high &=
> > +					~SECONDARY_EXEC_ENABLE_INVPCID;
> > +		}
> >  	}
> >  
> >  	if (cpu_has_secondary_exec_ctrls())
> > @@ -10175,6 +10190,7 @@ static int prepare_vmcs02(struct kvm_vcpu *vcpu,
> > struct vmcs12 *vmcs12,
> >  
> >  		/* Take the following fields only from vmcs12 */
> >  		exec_control &= ~(SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES |
> > +				  SECONDARY_EXEC_ENABLE_INVPCID |
> >  				  SECONDARY_EXEC_RDTSCP |
> >  				  SECONDARY_EXEC_VIRTUAL_INTR_DELIVERY |
> >  				  SECONDARY_EXEC_APIC_REGISTER_VIRT);
> > 
> 
> Makes sense to me!
> 
> --
> 
> Thanks,
> 
> David
>

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web