Subject: Re: [PATCH v2 3/3] kvm: svm: Use the hardware provided GPA instead of
 page walk
To: Brijesh Singh <brijesh.singh@amd.com>, kvm@vger.kernel.org
References: <147992048887.27638.17559991037474542240.stgit@brijesh-build-machine>
 <147992052008.27638.18095073174935903705.stgit@brijesh-build-machine>
Cc: thomas.lendacky@amd.com, rkrcmar@redhat.com, joro@8bytes.org,
        x86@kernel.org, linux-kernel@vger.kernel.org, mingo@redhat.com,
        hpa@zytor.com, tglx@linutronix.de, bp@suse.de
From: Paolo Bonzini <pbonzini@redhat.com>
Message-ID: <80ed2179-277d-cbd3-6441-582d733ff760@redhat.com>
Date: Wed, 23 Nov 2016 22:53:35 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101
 Thunderbird/45.4.0
MIME-Version: 1.0
In-Reply-To: <147992052008.27638.18095073174935903705.stgit@brijesh-build-machine>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 8bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 4686
Lines: 138


On 23/11/2016 18:02, Brijesh Singh wrote:
> From: Tom Lendacky <thomas.lendacky@amd.com>
> 
> When a guest causes a NPF which requires emulation, KVM sometimes walks
> the guest page tables to translate the GVA to a GPA. This is unnecessary
> most of the time on AMD hardware since the hardware provides the GPA in
> EXITINFO2.
> 
> The only exception cases involve string operations involving rep or
> operations that use two memory locations. With rep, the GPA will only be
> the value of the initial NPF and with dual memory locations we won't know
> which memory address was translated into EXITINFO2.
> 
> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
> Reviewed-by: Borislav Petkov <bp@suse.de>
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
>  arch/x86/include/asm/kvm_emulate.h |    3 +++
>  arch/x86/include/asm/kvm_host.h    |    3 +++
>  arch/x86/kvm/svm.c                 |    2 ++
>  arch/x86/kvm/x86.c                 |   17 ++++++++++++++++-
>  4 files changed, 24 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/include/asm/kvm_emulate.h b/arch/x86/include/asm/kvm_emulate.h
> index e9cd7be..2d1ac09 100644
> --- a/arch/x86/include/asm/kvm_emulate.h
> +++ b/arch/x86/include/asm/kvm_emulate.h
> @@ -344,6 +344,9 @@ struct x86_emulate_ctxt {
>  	struct read_cache mem_read;
>  };
>  
> +/* String operation identifier (matches the definition in emulate.c) */
> +#define CTXT_STRING_OP	(1 << 13)

I'm not sure how I missed this in v1, so this is my fault, but it is
wrong in many ways:

- it doesn't say *what* it matches (it matches "String")

- it doesn't include a check that it actually does match the definition

- it doesn't say with what field you're supposed to use it.

- it is not clear from the code that vcpu->arch.emulate_ctxt is valid at
all in vcpu_mmio_gva_to_gpa.
	
So please make a non-static function in emulate.c that takes the
emulation context pointer and returns a bool.

Also, in fact, now that I look at it, this path is relatively unlikely
to ever trigger, necause vcpu_match_mmio_gva probably matches first
anyway.  But it _is_ cheaper, since it doesn't need to test
permission_fault, so it is indeed a worthwhile patch to have.

However, please place it straight in emulator_read_write_onepage, like

	if (vcpu->arch.gpa_available &&
	    ... test CTXT_STRING_OP ... &&
	    vcpu_is_mmio_gpa(vcpu, exception->address)) {
		gpa = exception->address;
		goto mmio;
	}

where vcpu_is_mmio_gpa is essentially the APIC_DEFAULT_PHYS_BASE +
vcpu_match_mmio_gpa pair of checks, extracted from vcpu_mmio_gva_to_gpa.
	
Thanks, and sorry for the bad earlier review.

Paolo

>  /* Repeat String Operation Prefix */
>  #define REPE_PREFIX	0xf3
>  #define REPNE_PREFIX	0xf2
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 77cb3f9..fd5b1c8 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -668,6 +668,9 @@ struct kvm_vcpu_arch {
>  
>  	int pending_ioapic_eoi;
>  	int pending_external_vector;
> +
> +	/* GPA available (AMD only) */
> +	bool gpa_available;
>  };
>  
>  struct kvm_lpage_info {
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index 5e64e656..1bbd04c 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -4246,6 +4246,8 @@ static int handle_exit(struct kvm_vcpu *vcpu)
>  		return 1;
>  	}
>  
> +	vcpu->arch.gpa_available = (exit_code == SVM_EXIT_NPF);
> +
>  	return svm_exit_handlers[exit_code](svm);
>  }
>  
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index c30f62dc..5002eea 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -4441,7 +4441,19 @@ static int vcpu_mmio_gva_to_gpa(struct kvm_vcpu *vcpu, unsigned long gva,
>  		return 1;
>  	}
>  
> -	*gpa = vcpu->arch.walk_mmu->gva_to_gpa(vcpu, gva, access, exception);
> +	/*
> +	 * If the exit was due to a NPF we may already have a GPA.
> +	 * If the GPA is present, use it to avoid the GVA to GPA table
> +	 * walk. Note, this cannot be used on string operations since
> +	 * string operation using rep will only have the initial GPA
> +	 * from when the NPF occurred.
> +	 */
> +	if (vcpu->arch.gpa_available &&
> +	    !(vcpu->arch.emulate_ctxt.d & CTXT_STRING_OP))
> +		*gpa = exception->address;
> +	else
> +		*gpa = vcpu->arch.walk_mmu->gva_to_gpa(vcpu, gva, access,
> +						       exception);
>  
>  	if (*gpa == UNMAPPED_GVA)
>  		return -1;
> @@ -5563,6 +5575,9 @@ int x86_emulate_instruction(struct kvm_vcpu *vcpu,
>  	}
>  
>  restart:
> +	/* Save the faulting GPA (cr2) in the address field */
> +	ctxt->exception.address = cr2;
> +
>  	r = x86_emulate_insn(ctxt);
>  
>  	if (r == EMULATION_INTERCEPTED)
>