[PATCH v2] KVM: x86: Avoid guest page table walk when gpa_available is set

[PATCH v2] KVM: x86: Avoid guest page table walk when gpa_available is set

[Brijesh Singh]

From: Brijesh Singh <brijesh.singh@amd.com>

On AMD hardware when a guest causes a NPF which requires emulation,
the vcpu->arch.gpa_available flag is set to indicate that cr2 contains
a valid GPA.

Currently, emulator_read_write_onepage() makes use of gpa_available flag
to avoid a guest page walk for a known MMIO regions. Lets not limit
the gpa_available optimization to just MMIO region. The patch extends
the check to avoid page walk whenever gpa_available flag is set.

Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
v1: http://marc.info/?l=kvm&m=149304930814202&w=2

Changes in v2:
 - move gpa_val setting in pf_interception

 arch/x86/include/asm/kvm_host.h |  1 +
 arch/x86/kvm/svm.c              |  4 ++++
 arch/x86/kvm/x86.c              | 14 +++++++-------
 3 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 695605e..cc87e00 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -678,6 +678,7 @@ struct kvm_vcpu_arch {
 
 	/* GPA available (AMD only) */
 	bool gpa_available;
+	gpa_t gpa_val;
 };
 
 struct kvm_lpage_info {
diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index c27ac69..27fb563 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -2070,9 +2070,13 @@ static void svm_set_dr7(struct kvm_vcpu *vcpu, unsigned long value)
 static int pf_interception(struct vcpu_svm *svm)
 {
 	u64 fault_address = svm->vmcb->control.exit_info_2;
+	struct kvm_vcpu *vcpu = &svm->vcpu;
 	u64 error_code;
 	int r = 1;
 
+	/* On #NPF, exit_info_2 contains a valid GPA */
+	vcpu->arch.gpa_val = fault_address;
+
 	switch (svm->apf_reason) {
 	default:
 		error_code = svm->vmcb->control.exit_info_1;
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index b54125b..d2d88ed 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -4634,16 +4634,16 @@ static int emulator_read_write_onepage(unsigned long addr, void *val,
 	 */
 	if (vcpu->arch.gpa_available &&
 	    emulator_can_use_gpa(ctxt) &&
-	    vcpu_is_mmio_gpa(vcpu, addr, exception->address, write) &&
 	    (addr & ~PAGE_MASK) == (exception->address & ~PAGE_MASK)) {
-		gpa = exception->address;
-		goto mmio;
-	}
+		gpa = vcpu->arch.gpa_val;
+		ret = vcpu_is_mmio_gpa(vcpu, addr, gpa, write);
+	} else {
 
-	ret = vcpu_mmio_gva_to_gpa(vcpu, addr, &gpa, exception, write);
+		ret = vcpu_mmio_gva_to_gpa(vcpu, addr, &gpa, exception, write);
 
-	if (ret < 0)
-		return X86EMUL_PROPAGATE_FAULT;
+		if (ret < 0)
+			return X86EMUL_PROPAGATE_FAULT;
+	}
 
 	/* For APIC access vmexit */
 	if (ret)
-- 
2.7.4

Re: [PATCH v2] KVM: x86: Avoid guest page table walk when gpa_available is set

[Joerg Roedel]

On Fri, May 19, 2017 at 11:12:29AM -0400, Brijesh Singh wrote:
> From: Brijesh Singh <brijesh.singh@amd.com>
> 
> On AMD hardware when a guest causes a NPF which requires emulation,
> the vcpu->arch.gpa_available flag is set to indicate that cr2 contains
> a valid GPA.
> 
> Currently, emulator_read_write_onepage() makes use of gpa_available flag
> to avoid a guest page walk for a known MMIO regions. Lets not limit
> the gpa_available optimization to just MMIO region. The patch extends
> the check to avoid page walk whenever gpa_available flag is set.
> 
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
> v1: http://marc.info/?l=kvm&m=149304930814202&w=2
> 
> Changes in v2:
>  - move gpa_val setting in pf_interception
> 
>  arch/x86/include/asm/kvm_host.h |  1 +
>  arch/x86/kvm/svm.c              |  4 ++++
>  arch/x86/kvm/x86.c              | 14 +++++++-------
>  3 files changed, 12 insertions(+), 7 deletions(-)

I havn't checked in detail, but maybe you have: Does that take nesting
into account, where we might run on a shadow nested page-table and we
actually get a nested gpa (which still needs translation) instead of a
normal gpa?


	Joerg

Re: [PATCH v2] KVM: x86: Avoid guest page table walk when gpa_available is set

[Brijesh Singh]

Hi Jorge,

On 05/30/2017 04:05 AM, Joerg Roedel wrote:
> On Fri, May 19, 2017 at 11:12:29AM -0400, Brijesh Singh wrote:
>> From: Brijesh Singh <brijesh.singh@amd.com>
>>
>> On AMD hardware when a guest causes a NPF which requires emulation,
>> the vcpu->arch.gpa_available flag is set to indicate that cr2 contains
>> a valid GPA.
>>
>> Currently, emulator_read_write_onepage() makes use of gpa_available flag
>> to avoid a guest page walk for a known MMIO regions. Lets not limit
>> the gpa_available optimization to just MMIO region. The patch extends
>> the check to avoid page walk whenever gpa_available flag is set.
>>
>> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
>> ---
>> v1: http://marc.info/?l=kvm&m=149304930814202&w=2
>>
>> Changes in v2:
>>   - move gpa_val setting in pf_interception
>>
>>   arch/x86/include/asm/kvm_host.h |  1 +
>>   arch/x86/kvm/svm.c              |  4 ++++
>>   arch/x86/kvm/x86.c              | 14 +++++++-------
>>   3 files changed, 12 insertions(+), 7 deletions(-)
> 
> I havn't checked in detail, but maybe you have: Does that take nesting
> into account, where we might run on a shadow nested page-table and we
> actually get a nested gpa (which still needs translation) instead of a
> normal gpa?
> 

nesting should not be an issue. all we're doing is avoiding the GVA->GPA
translation for the lowest level guest. The nested case should still use
the original code path and perform the nested page walk. I have verified
the nesting support and it all seem to work just fine. You can find the
original gpa_available patch here [1]

[1] https://git.kernel.org/pub/scm/virt/kvm/kvm.git/commit/?id=0f89b207b04a1a399e19d35293658e3a571da3d7

-Brijesh

Re: [PATCH v2] KVM: x86: Avoid guest page table walk when gpa_available is set

[Brijesh Singh]

Hi Paolo and Radim

Any comments on this patch, I could not find it in 4.13-2 branch.

Please let me know if you want to fix something, or want me to
refresh and resend the patch.

- Brijesh

On 05/19/2017 10:12 AM, Brijesh Singh wrote:
> From: Brijesh Singh <brijesh.singh@amd.com>
> 
> On AMD hardware when a guest causes a NPF which requires emulation,
> the vcpu->arch.gpa_available flag is set to indicate that cr2 contains
> a valid GPA.
> 
> Currently, emulator_read_write_onepage() makes use of gpa_available flag
> to avoid a guest page walk for a known MMIO regions. Lets not limit
> the gpa_available optimization to just MMIO region. The patch extends
> the check to avoid page walk whenever gpa_available flag is set.
> 
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
> v1: http://marc.info/?l=kvm&m=149304930814202&w=2
> 
> Changes in v2:
>   - move gpa_val setting in pf_interception
> 
>   arch/x86/include/asm/kvm_host.h |  1 +
>   arch/x86/kvm/svm.c              |  4 ++++
>   arch/x86/kvm/x86.c              | 14 +++++++-------
>   3 files changed, 12 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 695605e..cc87e00 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -678,6 +678,7 @@ struct kvm_vcpu_arch {
>   
>   	/* GPA available (AMD only) */
>   	bool gpa_available;
> +	gpa_t gpa_val;
>   };
>   
>   struct kvm_lpage_info {
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index c27ac69..27fb563 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -2070,9 +2070,13 @@ static void svm_set_dr7(struct kvm_vcpu *vcpu, unsigned long value)
>   static int pf_interception(struct vcpu_svm *svm)
>   {
>   	u64 fault_address = svm->vmcb->control.exit_info_2;
> +	struct kvm_vcpu *vcpu = &svm->vcpu;
>   	u64 error_code;
>   	int r = 1;
>   
> +	/* On #NPF, exit_info_2 contains a valid GPA */
> +	vcpu->arch.gpa_val = fault_address;
> +
>   	switch (svm->apf_reason) {
>   	default:
>   		error_code = svm->vmcb->control.exit_info_1;
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index b54125b..d2d88ed 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -4634,16 +4634,16 @@ static int emulator_read_write_onepage(unsigned long addr, void *val,
>   	 */
>   	if (vcpu->arch.gpa_available &&
>   	    emulator_can_use_gpa(ctxt) &&
> -	    vcpu_is_mmio_gpa(vcpu, addr, exception->address, write) &&
>   	    (addr & ~PAGE_MASK) == (exception->address & ~PAGE_MASK)) {
> -		gpa = exception->address;
> -		goto mmio;
> -	}
> +		gpa = vcpu->arch.gpa_val;
> +		ret = vcpu_is_mmio_gpa(vcpu, addr, gpa, write);
> +	} else {
>   
> -	ret = vcpu_mmio_gva_to_gpa(vcpu, addr, &gpa, exception, write);
> +		ret = vcpu_mmio_gva_to_gpa(vcpu, addr, &gpa, exception, write);
>   
> -	if (ret < 0)
> -		return X86EMUL_PROPAGATE_FAULT;
> +		if (ret < 0)
> +			return X86EMUL_PROPAGATE_FAULT;
> +	}
>   
>   	/* For APIC access vmexit */
>   	if (ret)
> 

Re: [PATCH v2] KVM: x86: Avoid guest page table walk when gpa_available is set

[Radim Krčmář]

2017-07-17 16:32-0500, Brijesh Singh:
> Hi Paolo and Radim
> 
> Any comments on this patch, I could not find it in 4.13-2 branch.
> 
> Please let me know if you want to fix something, or want me to
> refresh and resend the patch.

Sorry, I tried it during the merge window, but it didn't pass tests on
VMX and I got distracted by other bugs before looking into the cause.

Can you reproduce the fail?

Re: [PATCH v2] KVM: x86: Avoid guest page table walk when gpa_available is set

[Brijesh Singh]

On 07/19/2017 06:19 AM, Radim Krčmář wrote:
> 2017-07-17 16:32-0500, Brijesh Singh:
>> Hi Paolo and Radim
>>
>> Any comments on this patch, I could not find it in 4.13-2 branch.
>>
>> Please let me know if you want to fix something, or want me to
>> refresh and resend the patch.
> 
> Sorry, I tried it during the merge window, but it didn't pass tests on
> VMX and I got distracted by other bugs before looking into the cause.
> 
> Can you reproduce the fail?
> 

No worries, thanks.

I can try to reproduce it, are you running kvm-unittest or something different?

IIRC, VMX does not set the gpa_available flag hence I am wondering what did I miss
in the patch to trigger the failure. I will debug it and let you know.

-Brijesh

Re: [PATCH v2] KVM: x86: Avoid guest page table walk when gpa_available is set

[Radim Krčmář]

2017-07-19 08:35-0500, Brijesh Singh:
> On 07/19/2017 06:19 AM, Radim Krčmář wrote:
> > 2017-07-17 16:32-0500, Brijesh Singh:
> > > Hi Paolo and Radim
> > > 
> > > Any comments on this patch, I could not find it in 4.13-2 branch.
> > > 
> > > Please let me know if you want to fix something, or want me to
> > > refresh and resend the patch.
> > 
> > Sorry, I tried it during the merge window, but it didn't pass tests on
> > VMX and I got distracted by other bugs before looking into the cause.
> > 
> > Can you reproduce the fail?
> > 
> 
> No worries, thanks.
> 
> I can try to reproduce it, are you running kvm-unittest or something different?

I noticed that a linux guest hung in early boot, but at least (io)apic
kvm-unit-tests failed as well, IIRC.

> IIRC, VMX does not set the gpa_available flag hence I am wondering what did I miss
> in the patch to trigger the failure. I will debug it and let you know.

It does now, in ept_violation and ept_misconfig,

thanks.

Re: [PATCH v2] KVM: x86: Avoid guest page table walk when gpa_available is set

[Brijesh Singh]

Hi Radim,


On 07/20/2017 02:43 AM, Radim Krčmář wrote:
> 2017-07-19 08:35-0500, Brijesh Singh:
>> On 07/19/2017 06:19 AM, Radim Krčmář wrote:
>>> 2017-07-17 16:32-0500, Brijesh Singh:
>>>> Hi Paolo and Radim
>>>>
>>>> Any comments on this patch, I could not find it in 4.13-2 branch.
>>>>
>>>> Please let me know if you want to fix something, or want me to
>>>> refresh and resend the patch.
>>>
>>> Sorry, I tried it during the merge window, but it didn't pass tests on
>>> VMX and I got distracted by other bugs before looking into the cause.
>>>
>>> Can you reproduce the fail?
>>>
>>
>> No worries, thanks.
>>
>> I can try to reproduce it, are you running kvm-unittest or something different?
> 
> I noticed that a linux guest hung in early boot, but at least (io)apic
> kvm-unit-tests failed as well, IIRC.
> 
>> IIRC, VMX does not set the gpa_available flag hence I am wondering what did I miss
>> in the patch to trigger the failure. I will debug it and let you know.
> 
> It does now, in ept_violation and ept_misconfig,
> 

I am able to reproduce the issue on VMX, Sorry it took a bit longer to verify
it.

I was not aware that VMX is also making use of gpa_available flag hence I missed
updating the vmx.c to set the gpa_val. After applying the below small patch I am
able to boot the guest on Intel Xeon E5-2665.

Additionally, there was one issue in current patch pointed by Paolo [1]. If patch
was using vcpu->arch.gpa_val check as pointed by Paolo then on VMX we will silently
fallback to guest page table walk (even when gpa_available is set). I guess since I
have testing my code on SVM platform hence never caught the error. I will soon send
updated patch.

[1] http://marc.info/?l=kvm&m=150116338725964&w=2

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index b5e0b02..9309fbb 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -6309,6 +6309,7 @@ static int handle_ept_violation(struct kvm_vcpu *vcpu)
                       ? PFERR_PRESENT_MASK : 0;
  
         vcpu->arch.gpa_available = true;
+       vcpu->arch.gpa_val = gpa;
         vcpu->arch.exit_qualification = exit_qualification;
  
         return kvm_mmu_page_fault(vcpu, gpa, error_code, NULL, 0);
@@ -6326,6 +6327,7 @@ static int handle_ept_misconfig(struct kvm_vcpu *vcpu)
         }
  
         ret = handle_mmio_page_fault(vcpu, gpa, true);
+       vcpu->arch.gpa_val = gpa;
         vcpu->arch.gpa_available = true;
         if (likely(ret == RET_MMIO_PF_EMULATE))
                 return x86_emulate_instruction(vcpu, gpa, 0, NULL, 0) ==

Re: [PATCH v2] KVM: x86: Avoid guest page table walk when gpa_available is set

[Paolo Bonzini]

On 19/05/2017 17:12, Brijesh Singh wrote:
> -	    vcpu_is_mmio_gpa(vcpu, addr, exception->address, write) &&
>  	    (addr & ~PAGE_MASK) == (exception->address & ~PAGE_MASK)) {
                                    ^^^^^^^^^^^^^^^^^^

I think this should be vcpu->arch.gpa_val.

Paolo

> -		gpa = exception->address;
> -		goto mmio;
> -	}
> +		gpa = vcpu->arch.gpa_val;
> +		ret = vcpu_is_mmio_gpa(vcpu, addr, gpa, write);
> +	} else {
>