| Message ID | 1399336859-7227-4-git-send-email-bsd@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Il 06/05/2014 02:40, Bandan Das ha scritto: > On every instruction fetch, kvm_read_guest_virt_helper > does the gva to gpa translation followed by searching for the > memslot. Store the gva hva mapping so that if there's a match > we can directly call __copy_from_user() > > Suggested-by: Paolo Bonzini <pbonzini@redhat.com> > Signed-off-by: Bandan Das <bsd@redhat.com> > --- > arch/x86/include/asm/kvm_emulate.h | 7 ++++++- > arch/x86/kvm/x86.c | 33 +++++++++++++++++++++++---------- > 2 files changed, 29 insertions(+), 11 deletions(-) > > diff --git a/arch/x86/include/asm/kvm_emulate.h b/arch/x86/include/asm/kvm_emulate.h > index 085d688..20ccde4 100644 > --- a/arch/x86/include/asm/kvm_emulate.h > +++ b/arch/x86/include/asm/kvm_emulate.h > @@ -323,10 +323,11 @@ struct x86_emulate_ctxt { > int (*execute)(struct x86_emulate_ctxt *ctxt); > int (*check_perm)(struct x86_emulate_ctxt *ctxt); > /* > - * The following five fields are cleared together, > + * The following six fields are cleared together, > * the rest are initialized unconditionally in x86_decode_insn > * or elsewhere > */ > + bool addr_cache_valid; You can just set gfn to -1 to invalidate the fetch. > u8 rex_prefix; > u8 lock_prefix; > u8 rep_prefix; > @@ -348,6 +349,10 @@ struct x86_emulate_ctxt { > struct fetch_cache fetch; > struct read_cache io_read; > struct read_cache mem_read; > + struct { > + gfn_t gfn; > + unsigned long uaddr; > + } addr_cache; This is not used by emulate.c, so it should be directly in struct kvm_vcpu. You could invalidate it in init_emulate_ctxt, together with emulate_regs_need_sync_from_vcpu. In the big picture, however, what we really want is to persist the cache across multiple instructions, and also cache the linearization of the address (where you add RIP and CS.base). This would be a bigger source of improvement. If you do that, the cache has to be indeed in x86_emulate_ctxt, but on the other hand the implementation needs to be in emulate.c. > }; > > /* Repeat String Operation Prefix */ > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index cf69e3b..7afcfc7 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -4072,26 +4072,38 @@ static int kvm_read_guest_virt_helper(gva_t addr, void *val, unsigned int bytes, > unsigned toread = min(bytes, (unsigned)PAGE_SIZE - offset); > int ret; > unsigned long uaddr; > + gfn_t gfn = addr >> PAGE_SHIFT; > > - ret = ctxt->ops->memory_prepare(ctxt, addr, toread, > - exception, false, > - NULL, &uaddr); > - if (ret != X86EMUL_CONTINUE) > - return ret; > + if (ctxt->addr_cache_valid && > + (ctxt->addr_cache.gfn == gfn)) > + uaddr = (ctxt->addr_cache.uaddr << PAGE_SHIFT) + > + offset_in_page(addr); > + else { > + ret = ctxt->ops->memory_prepare(ctxt, addr, toread, > + exception, false, > + NULL, &uaddr); Did you measure the hit rate, and the speedup after every patch? My reading of the code is that the fetch is done only once per page, with the speedup coming from the microoptimization that patch 2 provides for single-page reads. Single-page reads are indeed a very common case for the emulator. I suggest to start with making patch 2 ready for inclusion. Paolo > + if (ret != X86EMUL_CONTINUE) > + return ret; > + > + if (unlikely(kvm_is_error_hva(uaddr))) { > + r = X86EMUL_PROPAGATE_FAULT; > + return r; > + } > > - if (unlikely(kvm_is_error_hva(uaddr))) { > - r = X86EMUL_PROPAGATE_FAULT; > - return r; > + /* Cache gfn and hva */ > + ctxt->addr_cache.gfn = addr >> PAGE_SHIFT; > + ctxt->addr_cache.uaddr = uaddr >> PAGE_SHIFT; > + ctxt->addr_cache_valid = true; > } > > ret = __copy_from_user(data, (void __user *)uaddr, toread); > if (ret < 0) { > r = X86EMUL_IO_NEEDED; > + /* Where else should we invalidate cache ? */ > + ctxt->ops->memory_finish(ctxt, NULL, uaddr); > return r; > } > > - ctxt->ops->memory_finish(ctxt, NULL, uaddr); > - > bytes -= toread; > data += toread; > addr += toread; > @@ -4339,6 +4351,7 @@ static void emulator_memory_finish(struct x86_emulate_ctxt *ctxt, > struct kvm_memory_slot *memslot; > gfn_t gfn; > > + ctxt->addr_cache_valid = false; > if (!opaque) > return; > > -- 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
Paolo Bonzini <pbonzini@redhat.com> writes: > Il 06/05/2014 02:40, Bandan Das ha scritto: >> On every instruction fetch, kvm_read_guest_virt_helper >> does the gva to gpa translation followed by searching for the >> memslot. Store the gva hva mapping so that if there's a match >> we can directly call __copy_from_user() >> >> Suggested-by: Paolo Bonzini <pbonzini@redhat.com> >> Signed-off-by: Bandan Das <bsd@redhat.com> >> --- >> arch/x86/include/asm/kvm_emulate.h | 7 ++++++- >> arch/x86/kvm/x86.c | 33 +++++++++++++++++++++++---------- >> 2 files changed, 29 insertions(+), 11 deletions(-) >> >> diff --git a/arch/x86/include/asm/kvm_emulate.h b/arch/x86/include/asm/kvm_emulate.h >> index 085d688..20ccde4 100644 >> --- a/arch/x86/include/asm/kvm_emulate.h >> +++ b/arch/x86/include/asm/kvm_emulate.h >> @@ -323,10 +323,11 @@ struct x86_emulate_ctxt { >> int (*execute)(struct x86_emulate_ctxt *ctxt); >> int (*check_perm)(struct x86_emulate_ctxt *ctxt); >> /* >> - * The following five fields are cleared together, >> + * The following six fields are cleared together, >> * the rest are initialized unconditionally in x86_decode_insn >> * or elsewhere >> */ >> + bool addr_cache_valid; > > You can just set gfn to -1 to invalidate the fetch. > >> u8 rex_prefix; >> u8 lock_prefix; >> u8 rep_prefix; >> @@ -348,6 +349,10 @@ struct x86_emulate_ctxt { >> struct fetch_cache fetch; >> struct read_cache io_read; >> struct read_cache mem_read; >> + struct { >> + gfn_t gfn; >> + unsigned long uaddr; >> + } addr_cache; > > This is not used by emulate.c, so it should be directly in struct > kvm_vcpu. You could invalidate it in init_emulate_ctxt, together with > emulate_regs_need_sync_from_vcpu. Ok. > In the big picture, however, what we really want is to persist the > cache across multiple instructions, and also cache the linearization > of the address (where you add RIP and CS.base). This would be a > bigger source of improvement. If you do that, the cache has to be > indeed in x86_emulate_ctxt, but on the other hand the implementation > needs to be in emulate.c. > >> }; >> >> /* Repeat String Operation Prefix */ >> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c >> index cf69e3b..7afcfc7 100644 >> --- a/arch/x86/kvm/x86.c >> +++ b/arch/x86/kvm/x86.c >> @@ -4072,26 +4072,38 @@ static int kvm_read_guest_virt_helper(gva_t addr, void *val, unsigned int bytes, >> unsigned toread = min(bytes, (unsigned)PAGE_SIZE - offset); >> int ret; >> unsigned long uaddr; >> + gfn_t gfn = addr >> PAGE_SHIFT; >> >> - ret = ctxt->ops->memory_prepare(ctxt, addr, toread, >> - exception, false, >> - NULL, &uaddr); >> - if (ret != X86EMUL_CONTINUE) >> - return ret; >> + if (ctxt->addr_cache_valid && >> + (ctxt->addr_cache.gfn == gfn)) >> + uaddr = (ctxt->addr_cache.uaddr << PAGE_SHIFT) + >> + offset_in_page(addr); >> + else { >> + ret = ctxt->ops->memory_prepare(ctxt, addr, toread, >> + exception, false, >> + NULL, &uaddr); > > Did you measure the hit rate, and the speedup after every patch? My > reading of the code is that the fetch is done only once per page, with Yes, I did. IIRC, patch 3 actually improved the speedup compared to 2. A rough estimate for jmp - patch 2 reduced it to the low 600s, I guess around 610, but I could get a fresh set of numbers. So, not sure where the speed up is coming from, I will try to find out. > the speedup coming from the microoptimization that patch 2 provides > for single-page reads. Single-page reads are indeed a very common > case for the emulator. > > I suggest to start with making patch 2 ready for inclusion. > > Paolo > >> + if (ret != X86EMUL_CONTINUE) >> + return ret; >> + >> + if (unlikely(kvm_is_error_hva(uaddr))) { >> + r = X86EMUL_PROPAGATE_FAULT; >> + return r; >> + } >> >> - if (unlikely(kvm_is_error_hva(uaddr))) { >> - r = X86EMUL_PROPAGATE_FAULT; >> - return r; >> + /* Cache gfn and hva */ >> + ctxt->addr_cache.gfn = addr >> PAGE_SHIFT; >> + ctxt->addr_cache.uaddr = uaddr >> PAGE_SHIFT; >> + ctxt->addr_cache_valid = true; >> } >> >> ret = __copy_from_user(data, (void __user *)uaddr, toread); >> if (ret < 0) { >> r = X86EMUL_IO_NEEDED; >> + /* Where else should we invalidate cache ? */ >> + ctxt->ops->memory_finish(ctxt, NULL, uaddr); >> return r; >> } >> >> - ctxt->ops->memory_finish(ctxt, NULL, uaddr); >> - >> bytes -= toread; >> data += toread; >> addr += toread; >> @@ -4339,6 +4351,7 @@ static void emulator_memory_finish(struct x86_emulate_ctxt *ctxt, >> struct kvm_memory_slot *memslot; >> gfn_t gfn; >> >> + ctxt->addr_cache_valid = false; >> if (!opaque) >> return; >> >> -- 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 085d688..20ccde4 100644 --- a/arch/x86/include/asm/kvm_emulate.h +++ b/arch/x86/include/asm/kvm_emulate.h @@ -323,10 +323,11 @@ struct x86_emulate_ctxt { int (*execute)(struct x86_emulate_ctxt *ctxt); int (*check_perm)(struct x86_emulate_ctxt *ctxt); /* - * The following five fields are cleared together, + * The following six fields are cleared together, * the rest are initialized unconditionally in x86_decode_insn * or elsewhere */ + bool addr_cache_valid; u8 rex_prefix; u8 lock_prefix; u8 rep_prefix; @@ -348,6 +349,10 @@ struct x86_emulate_ctxt { struct fetch_cache fetch; struct read_cache io_read; struct read_cache mem_read; + struct { + gfn_t gfn; + unsigned long uaddr; + } addr_cache; }; /* Repeat String Operation Prefix */ diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index cf69e3b..7afcfc7 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -4072,26 +4072,38 @@ static int kvm_read_guest_virt_helper(gva_t addr, void *val, unsigned int bytes, unsigned toread = min(bytes, (unsigned)PAGE_SIZE - offset); int ret; unsigned long uaddr; + gfn_t gfn = addr >> PAGE_SHIFT; - ret = ctxt->ops->memory_prepare(ctxt, addr, toread, - exception, false, - NULL, &uaddr); - if (ret != X86EMUL_CONTINUE) - return ret; + if (ctxt->addr_cache_valid && + (ctxt->addr_cache.gfn == gfn)) + uaddr = (ctxt->addr_cache.uaddr << PAGE_SHIFT) + + offset_in_page(addr); + else { + ret = ctxt->ops->memory_prepare(ctxt, addr, toread, + exception, false, + NULL, &uaddr); + if (ret != X86EMUL_CONTINUE) + return ret; + + if (unlikely(kvm_is_error_hva(uaddr))) { + r = X86EMUL_PROPAGATE_FAULT; + return r; + } - if (unlikely(kvm_is_error_hva(uaddr))) { - r = X86EMUL_PROPAGATE_FAULT; - return r; + /* Cache gfn and hva */ + ctxt->addr_cache.gfn = addr >> PAGE_SHIFT; + ctxt->addr_cache.uaddr = uaddr >> PAGE_SHIFT; + ctxt->addr_cache_valid = true; } ret = __copy_from_user(data, (void __user *)uaddr, toread); if (ret < 0) { r = X86EMUL_IO_NEEDED; + /* Where else should we invalidate cache ? */ + ctxt->ops->memory_finish(ctxt, NULL, uaddr); return r; } - ctxt->ops->memory_finish(ctxt, NULL, uaddr); - bytes -= toread; data += toread; addr += toread; @@ -4339,6 +4351,7 @@ static void emulator_memory_finish(struct x86_emulate_ctxt *ctxt, struct kvm_memory_slot *memslot; gfn_t gfn; + ctxt->addr_cache_valid = false; if (!opaque) return;
On every instruction fetch, kvm_read_guest_virt_helper does the gva to gpa translation followed by searching for the memslot. Store the gva hva mapping so that if there's a match we can directly call __copy_from_user() Suggested-by: Paolo Bonzini <pbonzini@redhat.com> Signed-off-by: Bandan Das <bsd@redhat.com> --- arch/x86/include/asm/kvm_emulate.h | 7 ++++++- arch/x86/kvm/x86.c | 33 +++++++++++++++++++++++---------- 2 files changed, 29 insertions(+), 11 deletions(-)