Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1538593 > unrolled thread

Re: [PATCH v2 3/3] kvm: svm: Use the hardware provided GPA instead of page walk

Started byPaolo Bonzini <pbonzini@redhat.com>
First post2016-12-08 16:00 +0100
Last post2016-12-09 16:50 +0100
Articles 2 — 1 participant

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v2 3/3] kvm: svm: Use the hardware provided GPA instead of  page walk Paolo Bonzini <pbonzini@redhat.com> - 2016-12-08 16:00 +0100
    Re: [PATCH v2 3/3] kvm: svm: Use the hardware provided GPA instead  of page walk Paolo Bonzini <pbonzini@redhat.com> - 2016-12-09 16:50 +0100

#1538593 — Re: [PATCH v2 3/3] kvm: svm: Use the hardware provided GPA instead of page walk

FromPaolo Bonzini <pbonzini@redhat.com>
Date2016-12-08 16:00 +0100
SubjectRe: [PATCH v2 3/3] kvm: svm: Use the hardware provided GPA instead of page walk
Message-ID<sM8kW-7mt-9@gated-at.bofh.it>

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>

Tom, Brijesh,

I would like to confirm that you have run kvm-unit-tests with this patch?
I haven't yet tried AMD (will do before sending the pull request to Linus),
but a similar patch for Intel gives me these 4 failures on emulator.flat:

FAIL: push mem
FAIL: pop mem
FAIL: cross-page mmio read
FAIL: cross-page mmio write

The VMX patch to set gpa_available is just this:

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 25d48380c312..5d7b60d4795b 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -6393,6 +6393,7 @@ static int handle_ept_violation(struct kvm_vcpu *vcpu)
 	/* ept page table is present? */
 	error_code |= (exit_qualification & 0x38) != 0;
 
+	vcpu->arch.gpa_available = true;
 	vcpu->arch.exit_qualification = exit_qualification;
 
 	return kvm_mmu_page_fault(vcpu, gpa, error_code, NULL, 0);
@@ -6410,6 +6411,7 @@ static int handle_ept_misconfig(struct kvm_vcpu *vcpu)
 	}
 
 	ret = handle_mmio_page_fault(vcpu, gpa, true);
+	vcpu->arch.gpa_available = true;
 	if (likely(ret == RET_MMIO_PF_EMULATE))
 		return x86_emulate_instruction(vcpu, gpa, 0, NULL, 0) ==
 					      EMULATE_DONE;
@@ -8524,6 +8526,7 @@ static int vmx_handle_exit(struct kvm_vcpu *vcpu)
 	u32 vectoring_info = vmx->idt_vectoring_info;
 
 	trace_kvm_exit(exit_reason, vcpu, KVM_ISA_VMX);
+	vcpu->arch.gpa_available = false;
 
 	/*
 	 * Flush logged GPAs PML buffer, this will make dirty_bitmap more

Thanks,

Paolo

> ---
>  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)
> +
>  /* 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)
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

[toc] | [next] | [standalone]

#1539449 — Re: [PATCH v2 3/3] kvm: svm: Use the hardware provided GPA instead of page walk

FromPaolo Bonzini <pbonzini@redhat.com>
Date2016-12-09 16:50 +0100
SubjectRe: [PATCH v2 3/3] kvm: svm: Use the hardware provided GPA instead of page walk
Message-ID<sMvAR-522-9@gated-at.bofh.it>
In reply to#1538593
> I am able to reproduce it on AMD HW using kvm-unit-tests. Looking at
> test, the initial thought is "push mem" has two operands (the memory
> being pushed and the stack pointer). The provided GPA could be either
> one of those.

Aha, this makes sense---and it's easy to handle too, since you can just
add a flag to the decode table and extend the string op case to cover
that flag too.  Detecting cross-page MMIO is more problematic, I don't
have an idea offhand of how to solve it.

> If we can detect those cases, we should not set the gpa_available on
> them (like what we do with string move).

It would forbid usage of "push/pop mem" instructions with MMIO for SEV,
right?  It probably doesn't happen much in practice, but it's unfortunate.

Paolo

> We probably haven't hit this case in guest booting. Will investigate bit
> further and provide a updated patch to handle it.
> 
> > -Brijesh
> >> The VMX patch to set gpa_available is just this:
> >>
> >> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> >> index 25d48380c312..5d7b60d4795b 100644
> >> --- a/arch/x86/kvm/vmx.c
> >> +++ b/arch/x86/kvm/vmx.c
> >> @@ -6393,6 +6393,7 @@ static int handle_ept_violation(struct kvm_vcpu
> >> *vcpu)
> >>      /* ept page table is present? */
> >>      error_code |= (exit_qualification & 0x38) != 0;
> >>
> >> +    vcpu->arch.gpa_available = true;
> >>      vcpu->arch.exit_qualification = exit_qualification;
> >>
> >>      return kvm_mmu_page_fault(vcpu, gpa, error_code, NULL, 0);
> >> @@ -6410,6 +6411,7 @@ static int handle_ept_misconfig(struct kvm_vcpu
> >> *vcpu)
> >>      }
> >>
> >>      ret = handle_mmio_page_fault(vcpu, gpa, true);
> >> +    vcpu->arch.gpa_available = true;
> >>      if (likely(ret == RET_MMIO_PF_EMULATE))
> >>          return x86_emulate_instruction(vcpu, gpa, 0, NULL, 0) ==
> >>                            EMULATE_DONE;
> >> @@ -8524,6 +8526,7 @@ static int vmx_handle_exit(struct kvm_vcpu *vcpu)
> >>      u32 vectoring_info = vmx->idt_vectoring_info;
> >>
> >>      trace_kvm_exit(exit_reason, vcpu, KVM_ISA_VMX);
> >> +    vcpu->arch.gpa_available = false;
> >>
> >>      /*
> >>       * Flush logged GPAs PML buffer, this will make dirty_bitmap more
> >>
> >> Thanks,
> >>
> >> Paolo
> >>
> >>> ---
> >>>  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)
> >>> +
> >>>  /* 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)
> >>>
> >>> --
> >>> To unsubscribe from this list: send the line "unsubscribe kvm" in
> >>> the body of a message to majordomo@vger.kernel.org
> >>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >>>
>

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web