[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] [Xen-devel] Ping: [PATCH v3 3/4] x86/HVM: implement memory read caching
>>> On 25.09.18 at 16:25, <JBeulich@xxxxxxxx> wrote: > Emulation requiring device model assistance uses a form of instruction > re-execution, assuming that the second (and any further) pass takes > exactly the same path. This is a valid assumption as far use of CPU > registers goes (as those can't change without any other instruction > executing in between), but is wrong for memory accesses. In particular > it has been observed that Windows might page out buffers underneath an > instruction currently under emulation (hitting between two passes). If > the first pass translated a linear address successfully, any subsequent > pass needs to do so too, yielding the exact same translation. > > Introduce a cache (used by just guest page table accesses for now) to > make sure above described assumption holds. This is a very simplistic > implementation for now: Only exact matches are satisfied (no overlaps or > partial reads or anything). > > As to the actual data page in this scenario, there are a couple of > aspects to take into consideration: > - We must be talking about an insn accessing two locations (two memory > ones, one of which is MMIO, or a memory and an I/O one). > - If the non I/O / MMIO side is being read, the re-read (if it occurs at > all) is having its result discarded, by taking the shortcut through > the first switch()'s STATE_IORESP_READY case in hvmemul_do_io(). Note > how, among all the re-issue sanity checks there, we avoid comparing > the actual data. > - If the non I/O / MMIO side is being written, it is the OSes > responsibility to avoid actually moving page contents to disk while > there might still be a write access in flight - this is no different > in behavior from bare hardware. > - Read-modify-write accesses are, as always, complicated, and while we > deal with them better nowadays than we did in the past, we're still > not quite there to guarantee hardware like behavior in all cases > anyway. Nothing is getting worse by the changes made here, afaict. > > Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx> > Acked-by: Tim Deegan <tim@xxxxxxx> > Reviewed-by: Paul Durrant <paul.durrant@xxxxxxxxxx> SVM and VMX maintainers? Thanks, Jan > --- > v3: Add text about the actual data page to the description. > v2: Re-base. > > --- a/xen/arch/x86/hvm/emulate.c > +++ b/xen/arch/x86/hvm/emulate.c > @@ -27,6 +27,18 @@ > #include <asm/hvm/svm/svm.h> > #include <asm/vm_event.h> > > +struct hvmemul_cache > +{ > + unsigned int max_ents; > + unsigned int num_ents; > + struct { > + paddr_t gpa:PADDR_BITS; > + unsigned int size:(BITS_PER_LONG - PADDR_BITS) / 2; > + unsigned int level:(BITS_PER_LONG - PADDR_BITS) / 2; > + unsigned long data; > + } ents[]; > +}; > + > static void hvmtrace_io_assist(const ioreq_t *p) > { > unsigned int size, event; > @@ -541,7 +553,7 @@ static int hvmemul_do_mmio_addr(paddr_t > */ > static void *hvmemul_map_linear_addr( > unsigned long linear, unsigned int bytes, uint32_t pfec, > - struct hvm_emulate_ctxt *hvmemul_ctxt) > + struct hvm_emulate_ctxt *hvmemul_ctxt, struct hvmemul_cache *cache) > { > struct vcpu *curr = current; > void *err, *mapping; > @@ -586,7 +598,7 @@ static void *hvmemul_map_linear_addr( > ASSERT(mfn_x(*mfn) == 0); > > res = hvm_translate_get_page(curr, addr, true, pfec, > - &pfinfo, &page, NULL, &p2mt); > + &pfinfo, &page, NULL, &p2mt, cache); > > switch ( res ) > { > @@ -702,6 +714,8 @@ static int hvmemul_linear_to_phys( > gfn_t gfn, ngfn; > unsigned long done, todo, i, offset = addr & ~PAGE_MASK; > int reverse; > + struct hvmemul_cache *cache = pfec & PFEC_insn_fetch > + ? NULL : curr->arch.hvm.data_cache; > > /* > * Clip repetitions to a sensible maximum. This avoids extensive > looping in > @@ -731,7 +745,7 @@ static int hvmemul_linear_to_phys( > return rc; > gfn = gaddr_to_gfn(gaddr); > } > - else if ( gfn_eq(gfn = paging_gla_to_gfn(curr, addr, &pfec, NULL), > + else if ( gfn_eq(gfn = paging_gla_to_gfn(curr, addr, &pfec, cache), > INVALID_GFN) ) > { > if ( pfec & (PFEC_page_paged | PFEC_page_shared) ) > @@ -747,7 +761,7 @@ static int hvmemul_linear_to_phys( > { > /* Get the next PFN in the range. */ > addr += reverse ? -PAGE_SIZE : PAGE_SIZE; > - ngfn = paging_gla_to_gfn(curr, addr, &pfec, NULL); > + ngfn = paging_gla_to_gfn(curr, addr, &pfec, cache); > > /* Is it contiguous with the preceding PFNs? If not then we're > done. */ > if ( gfn_eq(ngfn, INVALID_GFN) || > @@ -1073,7 +1087,10 @@ static int linear_read(unsigned long add > uint32_t pfec, struct hvm_emulate_ctxt > *hvmemul_ctxt) > { > pagefault_info_t pfinfo; > - int rc = hvm_copy_from_guest_linear(p_data, addr, bytes, pfec, &pfinfo); > + int rc = hvm_copy_from_guest_linear(p_data, addr, bytes, pfec, &pfinfo, > + (pfec & PFEC_insn_fetch > + ? NULL > + : current->arch.hvm.data_cache)); > > switch ( rc ) > { > @@ -1270,7 +1287,8 @@ static int hvmemul_write( > > if ( !known_gla(addr, bytes, pfec) ) > { > - mapping = hvmemul_map_linear_addr(addr, bytes, pfec, hvmemul_ctxt); > + mapping = hvmemul_map_linear_addr(addr, bytes, pfec, hvmemul_ctxt, > + current->arch.hvm.data_cache); > if ( IS_ERR(mapping) ) > return ~PTR_ERR(mapping); > } > @@ -1312,7 +1330,8 @@ static int hvmemul_rmw( > > if ( !known_gla(addr, bytes, pfec) ) > { > - mapping = hvmemul_map_linear_addr(addr, bytes, pfec, hvmemul_ctxt); > + mapping = hvmemul_map_linear_addr(addr, bytes, pfec, hvmemul_ctxt, > + current->arch.hvm.data_cache); > if ( IS_ERR(mapping) ) > return ~PTR_ERR(mapping); > } > @@ -1466,7 +1485,8 @@ static int hvmemul_cmpxchg( > else if ( hvmemul_ctxt->seg_reg[x86_seg_ss].dpl == 3 ) > pfec |= PFEC_user_mode; > > - mapping = hvmemul_map_linear_addr(addr, bytes, pfec, hvmemul_ctxt); > + mapping = hvmemul_map_linear_addr(addr, bytes, pfec, hvmemul_ctxt, > + curr->arch.hvm.data_cache); > if ( IS_ERR(mapping) ) > return ~PTR_ERR(mapping); > > @@ -2373,6 +2393,7 @@ static int _hvm_emulate_one(struct hvm_e > { > vio->mmio_cache_count = 0; > vio->mmio_insn_bytes = 0; > + curr->arch.hvm.data_cache->num_ents = 0; > } > else > { > @@ -2591,7 +2612,7 @@ void hvm_emulate_init_per_insn( > &addr) && > hvm_copy_from_guest_linear(hvmemul_ctxt->insn_buf, addr, > sizeof(hvmemul_ctxt->insn_buf), > - pfec | PFEC_insn_fetch, > + pfec | PFEC_insn_fetch, NULL, > NULL) == HVMTRANS_okay) ? > sizeof(hvmemul_ctxt->insn_buf) : 0; > } > @@ -2664,9 +2685,35 @@ void hvm_dump_emulation_state(const char > hvmemul_ctxt->insn_buf); > } > > +struct hvmemul_cache *hvmemul_cache_init(unsigned int nents) > +{ > + struct hvmemul_cache *cache = xmalloc_bytes(offsetof(struct > hvmemul_cache, > + ents[nents])); > + > + if ( cache ) > + { > + cache->num_ents = 0; > + cache->max_ents = nents; > + } > + > + return cache; > +} > + > bool hvmemul_read_cache(const struct hvmemul_cache *cache, paddr_t gpa, > unsigned int level, void *buffer, unsigned int > size) > { > + unsigned int i; > + > + ASSERT(size <= sizeof(cache->ents->data)); > + > + for ( i = 0; i < cache->num_ents; ++i ) > + if ( cache->ents[i].level == level && cache->ents[i].gpa == gpa && > + cache->ents[i].size == size ) > + { > + memcpy(buffer, &cache->ents[i].data, size); > + return true; > + } > + > return false; > } > > @@ -2674,6 +2721,35 @@ void hvmemul_write_cache(struct hvmemul_ > unsigned int level, const void *buffer, > unsigned int size) > { > + unsigned int i; > + > + if ( size > sizeof(cache->ents->data) ) > + { > + ASSERT_UNREACHABLE(); > + return; > + } > + > + for ( i = 0; i < cache->num_ents; ++i ) > + if ( cache->ents[i].level == level && cache->ents[i].gpa == gpa && > + cache->ents[i].size == size ) > + { > + memcpy(&cache->ents[i].data, buffer, size); > + return; > + } > + > + if ( unlikely(i >= cache->max_ents) ) > + { > + ASSERT_UNREACHABLE(); > + return; > + } > + > + cache->ents[i].level = level; > + cache->ents[i].gpa = gpa; > + cache->ents[i].size = size; > + > + memcpy(&cache->ents[i].data, buffer, size); > + > + cache->num_ents = i + 1; > } > > /* > --- a/xen/arch/x86/hvm/hvm.c > +++ b/xen/arch/x86/hvm/hvm.c > @@ -1498,6 +1498,17 @@ int hvm_vcpu_initialise(struct vcpu *v) > > v->arch.hvm.inject_event.vector = HVM_EVENT_VECTOR_UNSET; > > + /* > + * Leaving aside the insn fetch, for which we don't use this cache, no > + * insn can access more than 8 independent linear addresses (AVX2 > + * gathers being the worst). Each such linear range can span a page > + * boundary, i.e. require two page walks. > + */ > + v->arch.hvm.data_cache = hvmemul_cache_init(CONFIG_PAGING_LEVELS * 8 * > 2); > + rc = -ENOMEM; > + if ( !v->arch.hvm.data_cache ) > + goto fail4; > + > rc = setup_compat_arg_xlat(v); /* teardown: free_compat_arg_xlat() */ > if ( rc != 0 ) > goto fail4; > @@ -1527,6 +1538,7 @@ int hvm_vcpu_initialise(struct vcpu *v) > fail5: > free_compat_arg_xlat(v); > fail4: > + hvmemul_cache_destroy(v->arch.hvm.data_cache); > hvm_funcs.vcpu_destroy(v); > fail3: > vlapic_destroy(v); > @@ -1549,6 +1561,8 @@ void hvm_vcpu_destroy(struct vcpu *v) > > free_compat_arg_xlat(v); > > + hvmemul_cache_destroy(v->arch.hvm.data_cache); > + > tasklet_kill(&v->arch.hvm.assert_evtchn_irq_tasklet); > hvm_funcs.vcpu_destroy(v); > > @@ -2923,7 +2937,7 @@ void hvm_task_switch( > } > > rc = hvm_copy_from_guest_linear( > - &tss, prev_tr.base, sizeof(tss), PFEC_page_present, &pfinfo); > + &tss, prev_tr.base, sizeof(tss), PFEC_page_present, &pfinfo, NULL); > if ( rc == HVMTRANS_bad_linear_to_gfn ) > hvm_inject_page_fault(pfinfo.ec, pfinfo.linear); > if ( rc != HVMTRANS_okay ) > @@ -2970,7 +2984,7 @@ void hvm_task_switch( > goto out; > > rc = hvm_copy_from_guest_linear( > - &tss, tr.base, sizeof(tss), PFEC_page_present, &pfinfo); > + &tss, tr.base, sizeof(tss), PFEC_page_present, &pfinfo, NULL); > if ( rc == HVMTRANS_bad_linear_to_gfn ) > hvm_inject_page_fault(pfinfo.ec, pfinfo.linear); > /* > @@ -3081,7 +3095,7 @@ void hvm_task_switch( > enum hvm_translation_result hvm_translate_get_page( > struct vcpu *v, unsigned long addr, bool linear, uint32_t pfec, > pagefault_info_t *pfinfo, struct page_info **page_p, > - gfn_t *gfn_p, p2m_type_t *p2mt_p) > + gfn_t *gfn_p, p2m_type_t *p2mt_p, struct hvmemul_cache *cache) > { > struct page_info *page; > p2m_type_t p2mt; > @@ -3089,7 +3103,7 @@ enum hvm_translation_result hvm_translat > > if ( linear ) > { > - gfn = paging_gla_to_gfn(v, addr, &pfec, NULL); > + gfn = paging_gla_to_gfn(v, addr, &pfec, cache); > > if ( gfn_eq(gfn, INVALID_GFN) ) > { > @@ -3161,7 +3175,7 @@ enum hvm_translation_result hvm_translat > #define HVMCOPY_linear (1u<<2) > static enum hvm_translation_result __hvm_copy( > void *buf, paddr_t addr, int size, struct vcpu *v, unsigned int flags, > - uint32_t pfec, pagefault_info_t *pfinfo) > + uint32_t pfec, pagefault_info_t *pfinfo, struct hvmemul_cache *cache) > { > gfn_t gfn; > struct page_info *page; > @@ -3194,8 +3208,8 @@ static enum hvm_translation_result __hvm > > count = min_t(int, PAGE_SIZE - gpa, todo); > > - res = hvm_translate_get_page(v, addr, flags & HVMCOPY_linear, > - pfec, pfinfo, &page, &gfn, &p2mt); > + res = hvm_translate_get_page(v, addr, flags & HVMCOPY_linear, pfec, > + pfinfo, &page, &gfn, &p2mt, cache); > if ( res != HVMTRANS_okay ) > return res; > > @@ -3242,14 +3256,14 @@ enum hvm_translation_result hvm_copy_to_ > paddr_t paddr, void *buf, int size, struct vcpu *v) > { > return __hvm_copy(buf, paddr, size, v, > - HVMCOPY_to_guest | HVMCOPY_phys, 0, NULL); > + HVMCOPY_to_guest | HVMCOPY_phys, 0, NULL, NULL); > } > > enum hvm_translation_result hvm_copy_from_guest_phys( > void *buf, paddr_t paddr, int size) > { > return __hvm_copy(buf, paddr, size, current, > - HVMCOPY_from_guest | HVMCOPY_phys, 0, NULL); > + HVMCOPY_from_guest | HVMCOPY_phys, 0, NULL, NULL); > } > > enum hvm_translation_result hvm_copy_to_guest_linear( > @@ -3258,16 +3272,17 @@ enum hvm_translation_result hvm_copy_to_ > { > return __hvm_copy(buf, addr, size, current, > HVMCOPY_to_guest | HVMCOPY_linear, > - PFEC_page_present | PFEC_write_access | pfec, > pfinfo); > + PFEC_page_present | PFEC_write_access | pfec, > + pfinfo, NULL); > } > > enum hvm_translation_result hvm_copy_from_guest_linear( > void *buf, unsigned long addr, int size, uint32_t pfec, > - pagefault_info_t *pfinfo) > + pagefault_info_t *pfinfo, struct hvmemul_cache *cache) > { > return __hvm_copy(buf, addr, size, current, > HVMCOPY_from_guest | HVMCOPY_linear, > - PFEC_page_present | pfec, pfinfo); > + PFEC_page_present | pfec, pfinfo, cache); > } > > unsigned long copy_to_user_hvm(void *to, const void *from, unsigned int > len) > @@ -3308,7 +3323,8 @@ unsigned long copy_from_user_hvm(void *t > return 0; > } > > - rc = hvm_copy_from_guest_linear(to, (unsigned long)from, len, 0, NULL); > + rc = hvm_copy_from_guest_linear(to, (unsigned long)from, len, > + 0, NULL, NULL); > return rc ? len : 0; /* fake a copy_from_user() return code */ > } > > @@ -3724,7 +3740,7 @@ void hvm_ud_intercept(struct cpu_user_re > sizeof(sig), hvm_access_insn_fetch, > cs, &addr) && > (hvm_copy_from_guest_linear(sig, addr, sizeof(sig), > - walk, NULL) == HVMTRANS_okay) && > + walk, NULL, NULL) == > HVMTRANS_okay) && > (memcmp(sig, "\xf\xbxen", sizeof(sig)) == 0) ) > { > regs->rip += sizeof(sig); > --- a/xen/arch/x86/hvm/svm/svm.c > +++ b/xen/arch/x86/hvm/svm/svm.c > @@ -1358,7 +1358,7 @@ static void svm_emul_swint_injection(str > goto raise_exception; > > rc = hvm_copy_from_guest_linear(&idte, idte_linear_addr, idte_size, > - PFEC_implicit, &pfinfo); > + PFEC_implicit, &pfinfo, NULL); > if ( rc ) > { > if ( rc == HVMTRANS_bad_linear_to_gfn ) > --- a/xen/arch/x86/hvm/vmx/vvmx.c > +++ b/xen/arch/x86/hvm/vmx/vvmx.c > @@ -475,7 +475,7 @@ static int decode_vmx_inst(struct cpu_us > { > pagefault_info_t pfinfo; > int rc = hvm_copy_from_guest_linear(poperandS, base, size, > - 0, &pfinfo); > + 0, &pfinfo, NULL); > > if ( rc == HVMTRANS_bad_linear_to_gfn ) > hvm_inject_page_fault(pfinfo.ec, pfinfo.linear); > --- a/xen/arch/x86/mm/shadow/common.c > +++ b/xen/arch/x86/mm/shadow/common.c > @@ -166,7 +166,7 @@ const struct x86_emulate_ops *shadow_ini > hvm_access_insn_fetch, sh_ctxt, &addr) && > !hvm_copy_from_guest_linear( > sh_ctxt->insn_buf, addr, sizeof(sh_ctxt->insn_buf), > - PFEC_insn_fetch, NULL)) > + PFEC_insn_fetch, NULL, NULL)) > ? sizeof(sh_ctxt->insn_buf) : 0; > > return &hvm_shadow_emulator_ops; > @@ -201,7 +201,7 @@ void shadow_continue_emulation(struct sh > hvm_access_insn_fetch, sh_ctxt, &addr) && > !hvm_copy_from_guest_linear( > sh_ctxt->insn_buf, addr, sizeof(sh_ctxt->insn_buf), > - PFEC_insn_fetch, NULL)) > + PFEC_insn_fetch, NULL, NULL)) > ? sizeof(sh_ctxt->insn_buf) : 0; > sh_ctxt->insn_buf_eip = regs->rip; > } > --- a/xen/arch/x86/mm/shadow/hvm.c > +++ b/xen/arch/x86/mm/shadow/hvm.c > @@ -125,7 +125,7 @@ hvm_read(enum x86_segment seg, > rc = hvm_copy_from_guest_linear(p_data, addr, bytes, > (access_type == hvm_access_insn_fetch > ? PFEC_insn_fetch : 0), > - &pfinfo); > + &pfinfo, NULL); > > switch ( rc ) > { > --- a/xen/include/asm-x86/hvm/emulate.h > +++ b/xen/include/asm-x86/hvm/emulate.h > @@ -99,6 +99,11 @@ int hvmemul_do_pio_buffer(uint16_t port, > void *buffer); > > struct hvmemul_cache; > +struct hvmemul_cache *hvmemul_cache_init(unsigned int nents); > +static inline void hvmemul_cache_destroy(struct hvmemul_cache *cache) > +{ > + xfree(cache); > +} > bool hvmemul_read_cache(const struct hvmemul_cache *, paddr_t gpa, > unsigned int level, void *buffer, unsigned int > size); > void hvmemul_write_cache(struct hvmemul_cache *, paddr_t gpa, > --- a/xen/include/asm-x86/hvm/support.h > +++ b/xen/include/asm-x86/hvm/support.h > @@ -99,7 +99,7 @@ enum hvm_translation_result hvm_copy_to_ > pagefault_info_t *pfinfo); > enum hvm_translation_result hvm_copy_from_guest_linear( > void *buf, unsigned long addr, int size, uint32_t pfec, > - pagefault_info_t *pfinfo); > + pagefault_info_t *pfinfo, struct hvmemul_cache *cache); > > /* > * Get a reference on the page under an HVM physical or linear address. If > @@ -110,7 +110,7 @@ enum hvm_translation_result hvm_copy_fro > enum hvm_translation_result hvm_translate_get_page( > struct vcpu *v, unsigned long addr, bool linear, uint32_t pfec, > pagefault_info_t *pfinfo, struct page_info **page_p, > - gfn_t *gfn_p, p2m_type_t *p2mt_p); > + gfn_t *gfn_p, p2m_type_t *p2mt_p, struct hvmemul_cache *cache); > > #define HVM_HCALL_completed 0 /* hypercall completed - no further action > */ > #define HVM_HCALL_preempted 1 /* hypercall preempted - re-execute VMCALL > */ > --- a/xen/include/asm-x86/hvm/vcpu.h > +++ b/xen/include/asm-x86/hvm/vcpu.h > @@ -53,8 +53,6 @@ struct hvm_mmio_cache { > uint8_t buffer[32]; > }; > > -struct hvmemul_cache; > - > struct hvm_vcpu_io { > /* I/O request in flight to device model. */ > enum hvm_io_completion io_completion; > @@ -200,6 +198,7 @@ struct hvm_vcpu { > u8 cache_mode; > > struct hvm_vcpu_io hvm_io; > + struct hvmemul_cache *data_cache; > > /* Pending hw/sw interrupt (.vector = -1 means nothing pending). */ > struct x86_event inject_event; > > > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@xxxxxxxxxxxxxxxxxxxx > https://lists.xenproject.org/mailman/listinfo/xen-devel _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxxxxxxxxx https://lists.xenproject.org/mailman/listinfo/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |