DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 4E00CD64B3
Subject: Re: [PATCH] KVM: nVMX: INVPCID support
To: Paolo Bonzini <pbonzini@redhat.com>, linux-kernel@vger.kernel.org,
        kvm@vger.kernel.org
References: <1501161639-9707-1-git-send-email-pbonzini@redhat.com>
From: David Hildenbrand <david@redhat.com>
Organization: Red Hat GmbH
Message-ID: <0b82ca85-7b3e-9dc4-a102-24d0e9033dcd@redhat.com>
Date: Thu, 27 Jul 2017 20:29:21 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.1.0
MIME-Version: 1.0
In-Reply-To: <1501161639-9707-1-git-send-email-pbonzini@redhat.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3794
Lines: 123

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