Re: [PATCH] KVM: nVMX: INVPCID support

[Paolo Bonzini]

----- Original Message -----
> From: "David Hildenbrand" <david@xxxxxxxxxx>
> To: "Paolo Bonzini" <pbonzini@xxxxxxxxxx>, linux-kernel@xxxxxxxxxxxxxxx, kvm@xxxxxxxxxxxxxxx
> 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@xxxxxxxxxx>
> > ---
> >  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
>