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

Radim Krčmář
2017-04-24T20:52:36Z

2017-04-24 11:52-0400, Brijesh Singh:
> On AMD hardware when a guest causes a NFP 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>
> ---
> arch/x86/include/asm/kvm_host.h | 1 +
> arch/x86/kvm/svm.c | 4 ++++
> arch/x86/kvm/x86.c | 26 +++++++++++++++-----------
> 3 files changed, 20 insertions(+), 11 deletions(-)
>
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 74ef58c..491326d 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -677,6 +677,7 @@ struct kvm_vcpu_arch {
>
> /* GPA available (AMD only) */
> bool gpa_available;
> + gpa_t gpa_val;

Can't we pass this information through function parameters?

(I'd rather avoid intractable variables.)

> };
>
> struct kvm_lpage_info {
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index 5fba706..8827e4b 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -4159,6 +4159,10 @@ static int handle_exit(struct kvm_vcpu *vcpu)
>
> vcpu->arch.gpa_available = (exit_code == SVM_EXIT_NPF);
>
> + /* On #NPF, exit_info_2 contain a valid GPA */
> + if (vcpu->arch.gpa_available)
> + vcpu->arch.gpa_val = svm->vmcb->control.exit_info_2;

How is vcpu->arch.gpa_val used between here and the NPF handler?

> +
> if (!is_cr_intercept(svm, INTERCEPT_CR0_WRITE))
> vcpu->arch.cr0 = svm->vmcb->save.cr0;
> if (npt_enabled)
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> @@ -4653,18 +4653,16 @@ static int emulator_read_write_onepage(unsigned long addr, void *val,
> * occurred.
> */
> 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)) {
> + emulator_can_use_gpa(ctxt) &&
> + (addr & ~PAGE_MASK) == (exception->address & ~PAGE_MASK)) {
> gpa = exception->address;
> - goto mmio;
> + ret = vcpu_is_mmio_gpa(vcpu, addr, gpa, write);
> + } else {
> + ret = vcpu_mmio_gva_to_gpa(vcpu, addr, &gpa, exception, write);
> + if (ret < 0)
> + return X86EMUL_PROPAGATE_FAULT;
> }
>
> - ret = vcpu_mmio_gva_to_gpa(vcpu, addr, &gpa, exception, write);
> -
> - if (ret < 0)
> - return X86EMUL_PROPAGATE_FAULT;
> -
> /* For APIC access vmexit */
> if (ret)
> goto mmio;
> @@ -5675,8 +5673,14 @@ int x86_emulate_instruction(struct kvm_vcpu *vcpu,
> }
>
> restart:
> - /* Save the faulting GPA (cr2) in the address field */
> - ctxt->exception.address = cr2;
> + /*
> + * Save the faulting GPA (cr2) in the address field
> + * NOTE: If gpa_available is set then gpa_val will contain a valid GPA
> + */
> + if (vcpu->arch.gpa_available)
> + ctxt->exception.address = vcpu->arch.gpa_val;
> + else
> + ctxt->exception.address = cr2;

And related, shouldn't vcpu->arch.gpa_val be in cr2?

Thanks.