| Message ID | 147992052008.27638.18095073174935903705.stgit@brijesh-build-machine (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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) > -- 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
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)