| Message ID | 148044249174.29544.4194514184644021170.stgit@brijesh-build-machine (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 29/11/2016 19:01, 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 | 1 + > arch/x86/include/asm/kvm_host.h | 3 ++ > arch/x86/kvm/emulate.c | 8 +++++++ > arch/x86/kvm/svm.c | 2 ++ > arch/x86/kvm/x86.c | 44 ++++++++++++++++++++++++++++-------- > 5 files changed, 48 insertions(+), 10 deletions(-) > > diff --git a/arch/x86/include/asm/kvm_emulate.h b/arch/x86/include/asm/kvm_emulate.h > index e9cd7be..777eea2 100644 > --- a/arch/x86/include/asm/kvm_emulate.h > +++ b/arch/x86/include/asm/kvm_emulate.h > @@ -441,5 +441,6 @@ int emulator_task_switch(struct x86_emulate_ctxt *ctxt, > int emulate_int_real(struct x86_emulate_ctxt *ctxt, int irq); > void emulator_invalidate_register_cache(struct x86_emulate_ctxt *ctxt); > void emulator_writeback_register_cache(struct x86_emulate_ctxt *ctxt); > +bool emulator_is_string_op(struct x86_emulate_ctxt *ctxt); > > #endif /* _ASM_X86_KVM_X86_EMULATE_H */ > 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/emulate.c b/arch/x86/kvm/emulate.c > index a3ce9d2..0ea543e 100644 > --- a/arch/x86/kvm/emulate.c > +++ b/arch/x86/kvm/emulate.c > @@ -5483,3 +5483,11 @@ void emulator_writeback_register_cache(struct x86_emulate_ctxt *ctxt) > { > writeback_registers(ctxt); > } > + > +bool emulator_is_string_op(struct x86_emulate_ctxt *ctxt) > +{ > + if (ctxt->d & String) > + return true; > + > + return false; > +} > diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c > index 5e64e656..e9b3555 100644 > --- a/arch/x86/kvm/svm.c > +++ b/arch/x86/kvm/svm.c > @@ -4188,6 +4188,8 @@ static int handle_exit(struct kvm_vcpu *vcpu) > > trace_kvm_exit(exit_code, vcpu, KVM_ISA_SVM); > > + vcpu->arch.gpa_available = (exit_code == SVM_EXIT_NPF); > + > 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 > index c30f62dc..507c75c 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -4420,6 +4420,21 @@ int kvm_write_guest_virt_system(struct x86_emulate_ctxt *ctxt, > } > EXPORT_SYMBOL_GPL(kvm_write_guest_virt_system); > > +static int vcpu_is_mmio_gpa(struct kvm_vcpu *vcpu, unsigned long gva, > + gpa_t gpa, bool write) > +{ > + /* For APIC access vmexit */ > + if ((gpa & PAGE_MASK) == APIC_DEFAULT_PHYS_BASE) > + return 1; > + > + if (vcpu_match_mmio_gpa(vcpu, gpa)) { > + trace_vcpu_match_mmio(gva, gpa, write, true); > + return 1; > + } > + > + return 0; > +} > + > static int vcpu_mmio_gva_to_gpa(struct kvm_vcpu *vcpu, unsigned long gva, > gpa_t *gpa, struct x86_exception *exception, > bool write) > @@ -4446,16 +4461,7 @@ static int vcpu_mmio_gva_to_gpa(struct kvm_vcpu *vcpu, unsigned long gva, > if (*gpa == UNMAPPED_GVA) > return -1; > > - /* For APIC access vmexit */ > - if ((*gpa & PAGE_MASK) == APIC_DEFAULT_PHYS_BASE) > - return 1; > - > - if (vcpu_match_mmio_gpa(vcpu, *gpa)) { > - trace_vcpu_match_mmio(gva, *gpa, write, true); > - return 1; > - } > - > - return 0; > + return vcpu_is_mmio_gpa(vcpu, gva, *gpa, write); > } > > int emulator_write_phys(struct kvm_vcpu *vcpu, gpa_t gpa, > @@ -4552,6 +4558,21 @@ static int emulator_read_write_onepage(unsigned long addr, void *val, > int handled, ret; > bool write = ops->write; > struct kvm_mmio_fragment *frag; > + struct x86_emulate_ctxt *ctxt = &vcpu->arch.emulate_ctxt; > + > + /* > + * 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 the NPF > + * occurred. > + */ > + if (vcpu->arch.gpa_available && > + !emulator_is_string_op(ctxt) && > + vcpu_is_mmio_gpa(vcpu, addr, exception->address, write)) { > + gpa = exception->address; > + goto mmio; > + } > > ret = vcpu_mmio_gva_to_gpa(vcpu, addr, &gpa, exception, write); > > @@ -5563,6 +5584,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) > Reviewed-by: Paolo Bonzini <pbonzini@redhat.com> Thanks! Paolo -- 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
On Tue, 29 Nov 2016, Brijesh Singh wrote: > --- a/arch/x86/kvm/emulate.c > +++ b/arch/x86/kvm/emulate.c > @@ -5483,3 +5483,11 @@ void emulator_writeback_register_cache(struct x86_emulate_ctxt *ctxt) > { > writeback_registers(ctxt); > } > + > +bool emulator_is_string_op(struct x86_emulate_ctxt *ctxt) > +{ > + if (ctxt->d & String) > + return true; > + > + return false; > +} Do we really need a full function call for this simple thing? Just because this horrible CamelCase constant is in emulate.c? What's wrong with moving that thing into a header and make it a trivial inline: #define INS_STRING (1 << 13) static inline bool emulator_is_string_op(struct x86_emulate_ctxt *ctxt) { return ctxt->d & INS_STRING; } Hmm? tglx -- 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
On 11/29/2016 12:20 PM, Thomas Gleixner wrote: > On Tue, 29 Nov 2016, Brijesh Singh wrote: >> --- a/arch/x86/kvm/emulate.c >> +++ b/arch/x86/kvm/emulate.c >> @@ -5483,3 +5483,11 @@ void emulator_writeback_register_cache(struct x86_emulate_ctxt *ctxt) >> { >> writeback_registers(ctxt); >> } >> + >> +bool emulator_is_string_op(struct x86_emulate_ctxt *ctxt) >> +{ >> + if (ctxt->d & String) >> + return true; >> + >> + return false; >> +} > > Do we really need a full function call for this simple thing? Just because > this horrible CamelCase constant is in emulate.c? > > What's wrong with moving that thing into a header and make it a trivial > inline: > > #define INS_STRING (1 << 13) > > static inline bool emulator_is_string_op(struct x86_emulate_ctxt *ctxt) > { > return ctxt->d & INS_STRING; > } > > Hmm? One of the recommendation from previous review feedback was to move the check inside emulator.c. I am fine with inlining it into kvm_emulate.h Hi Paolo, Do you want me to spin a new version ? -Brijesh > > tglx > -- 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
On 29/11/2016 20:38, Brijesh Singh wrote: > > > On 11/29/2016 12:20 PM, Thomas Gleixner wrote: >> On Tue, 29 Nov 2016, Brijesh Singh wrote: >>> --- a/arch/x86/kvm/emulate.c >>> +++ b/arch/x86/kvm/emulate.c >>> @@ -5483,3 +5483,11 @@ void emulator_writeback_register_cache(struct >>> x86_emulate_ctxt *ctxt) >>> { >>> writeback_registers(ctxt); >>> } >>> + >>> +bool emulator_is_string_op(struct x86_emulate_ctxt *ctxt) >>> +{ >>> + if (ctxt->d & String) >>> + return true; >>> + >>> + return false; >>> +} >> >> Do we really need a full function call for this simple thing? Just >> because >> this horrible CamelCase constant is in emulate.c? >> >> What's wrong with moving that thing into a header and make it a trivial >> inline: >> >> #define INS_STRING (1 << 13) >> >> static inline bool emulator_is_string_op(struct x86_emulate_ctxt *ctxt) >> { >> return ctxt->d & INS_STRING; >> } >> >> Hmm? > > One of the recommendation from previous review feedback was to move the > check inside emulator.c. I am fine with inlining it into kvm_emulate.h > > Hi Paolo, > > Do you want me to spin a new version ? No, it's not that much of a fast path, so it wouldn't change anything in practice. Paolo -- 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..777eea2 100644 --- a/arch/x86/include/asm/kvm_emulate.h +++ b/arch/x86/include/asm/kvm_emulate.h @@ -441,5 +441,6 @@ int emulator_task_switch(struct x86_emulate_ctxt *ctxt, int emulate_int_real(struct x86_emulate_ctxt *ctxt, int irq); void emulator_invalidate_register_cache(struct x86_emulate_ctxt *ctxt); void emulator_writeback_register_cache(struct x86_emulate_ctxt *ctxt); +bool emulator_is_string_op(struct x86_emulate_ctxt *ctxt); #endif /* _ASM_X86_KVM_X86_EMULATE_H */ 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/emulate.c b/arch/x86/kvm/emulate.c index a3ce9d2..0ea543e 100644 --- a/arch/x86/kvm/emulate.c +++ b/arch/x86/kvm/emulate.c @@ -5483,3 +5483,11 @@ void emulator_writeback_register_cache(struct x86_emulate_ctxt *ctxt) { writeback_registers(ctxt); } + +bool emulator_is_string_op(struct x86_emulate_ctxt *ctxt) +{ + if (ctxt->d & String) + return true; + + return false; +} diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c index 5e64e656..e9b3555 100644 --- a/arch/x86/kvm/svm.c +++ b/arch/x86/kvm/svm.c @@ -4188,6 +4188,8 @@ static int handle_exit(struct kvm_vcpu *vcpu) trace_kvm_exit(exit_code, vcpu, KVM_ISA_SVM); + vcpu->arch.gpa_available = (exit_code == SVM_EXIT_NPF); + 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 index c30f62dc..507c75c 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -4420,6 +4420,21 @@ int kvm_write_guest_virt_system(struct x86_emulate_ctxt *ctxt, } EXPORT_SYMBOL_GPL(kvm_write_guest_virt_system); +static int vcpu_is_mmio_gpa(struct kvm_vcpu *vcpu, unsigned long gva, + gpa_t gpa, bool write) +{ + /* For APIC access vmexit */ + if ((gpa & PAGE_MASK) == APIC_DEFAULT_PHYS_BASE) + return 1; + + if (vcpu_match_mmio_gpa(vcpu, gpa)) { + trace_vcpu_match_mmio(gva, gpa, write, true); + return 1; + } + + return 0; +} + static int vcpu_mmio_gva_to_gpa(struct kvm_vcpu *vcpu, unsigned long gva, gpa_t *gpa, struct x86_exception *exception, bool write) @@ -4446,16 +4461,7 @@ static int vcpu_mmio_gva_to_gpa(struct kvm_vcpu *vcpu, unsigned long gva, if (*gpa == UNMAPPED_GVA) return -1; - /* For APIC access vmexit */ - if ((*gpa & PAGE_MASK) == APIC_DEFAULT_PHYS_BASE) - return 1; - - if (vcpu_match_mmio_gpa(vcpu, *gpa)) { - trace_vcpu_match_mmio(gva, *gpa, write, true); - return 1; - } - - return 0; + return vcpu_is_mmio_gpa(vcpu, gva, *gpa, write); } int emulator_write_phys(struct kvm_vcpu *vcpu, gpa_t gpa, @@ -4552,6 +4558,21 @@ static int emulator_read_write_onepage(unsigned long addr, void *val, int handled, ret; bool write = ops->write; struct kvm_mmio_fragment *frag; + struct x86_emulate_ctxt *ctxt = &vcpu->arch.emulate_ctxt; + + /* + * 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 the NPF + * occurred. + */ + if (vcpu->arch.gpa_available && + !emulator_is_string_op(ctxt) && + vcpu_is_mmio_gpa(vcpu, addr, exception->address, write)) { + gpa = exception->address; + goto mmio; + } ret = vcpu_mmio_gva_to_gpa(vcpu, addr, &gpa, exception, write); @@ -5563,6 +5584,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)