|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] [Xen-devel] [PATCH v5 2/4] x86/HVM: implement memory read caching for insn emulation
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 as use of CPU
registers goes (as those can't change without any other instruction
executing in between [1]), 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 read a memory operand successfully, any
subsequent pass needs to get to see the exact same value.
Introduce a cache 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); this is sufficient
for the immediate purpose of making re-execution an exact replay. The
cache also won't be used just yet for guest page walks; that'll be the
subject of a subsequent change.
With the cache being generally transparent to upper layers, but with it
having limited capacity yet being required for correctness, certain
users of hvm_copy_from_guest_*() need to disable caching temporarily,
without invalidating the cache. Note that the adjustments here to
hvm_hypercall() and hvm_task_switch() are benign at this point; they'll
become relevant once we start to be able to emulate respective insns
through the main emulator (and more changes will then likely be needed
to nested code).
As to the actual data page in a problamtic 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.
In __hvm_copy() also reduce p's scope and change its type to void *.
[1] Other than on actual hardware, actions like
XEN_DOMCTL_sethvmcontext, XEN_DOMCTL_setvcpucontext,
VCPUOP_initialise, INIT, or SIPI issued against the vCPU can occur
while the vCPU is blocked waiting for a device model to return data.
In such cases emulation now gets canceled, though, and hence re-
execution correctness is unaffected.
Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
---
TBD: In principle the caching here yields unnecessary the one used for
insn bytes (vio->mmio_insn{,_bytes}. However, to seed the cache
with the data SVM may have made available, we'd have to also know
the corresponding GPA. It's not safe, however, to re-walk the page
tables to find out, as the page tables may have changed in the
meantime. Therefore I guess we need to keep the duplicate
functionality for now. A possible solution to this could be to use
a physical-address-based cache for page table accesses (and looking
forward also e.g. SVM/VMX insn emulation), and a linear-address-
based one for all other reads.
---
v5: Re-arrange bitfield. Use domain_crash() in hvmemul_write_cache().
Move hvmemul_{read,write}_cache() stubs to later patch. Also adjust
hvmemul_cancel(). Add / extend comments. Re-base.
v4: Re-write for cache to become transparent to callers.
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
@@ -28,6 +28,19 @@
#include <asm/iocap.h>
#include <asm/vm_event.h>
+struct hvmemul_cache
+{
+ /* The cache is disabled as long as num_ents > max_ents. */
+ unsigned int num_ents;
+ unsigned int max_ents;
+ struct {
+ paddr_t gpa:PADDR_BITS;
+ unsigned int :BITS_PER_LONG - PADDR_BITS - 8;
+ unsigned int size:8;
+ unsigned long data;
+ } ents[];
+};
+
static void hvmtrace_io_assist(const ioreq_t *p)
{
unsigned int size, event;
@@ -136,6 +149,8 @@ void hvmemul_cancel(struct vcpu *v)
vio->mmio_access = (struct npfec){};
vio->mmio_retry = false;
vio->g2m_ioport = NULL;
+
+ hvmemul_cache_disable(v);
}
static int hvmemul_do_io(
@@ -1883,12 +1898,17 @@ static int hvmemul_rep_movs(
rc = HVMTRANS_okay;
}
else
+ {
+ unsigned int token = hvmemul_cache_disable(curr);
+
/*
* We do a modicum of checking here, just for paranoia's sake and to
* definitely avoid copying an unitialised buffer into guest address
* space.
*/
rc = hvm_copy_from_guest_phys(buf, sgpa, bytes);
+ hvmemul_cache_restore(curr, token);
+ }
if ( rc == HVMTRANS_okay )
rc = hvm_copy_to_guest_phys(dgpa, buf, bytes, curr);
@@ -2551,6 +2571,19 @@ static int _hvm_emulate_one(struct hvm_e
struct hvm_vcpu_io *vio = &curr->arch.hvm.hvm_io;
int rc;
+ /*
+ * Enable caching if it's currently disabled, but leave the cache
+ * untouched if it's already enabled, for re-execution to consume
+ * entries populated by an earlier pass.
+ */
+ if ( vio->cache->num_ents > vio->cache->max_ents )
+ {
+ ASSERT(vio->io_req.state == STATE_IOREQ_NONE);
+ vio->cache->num_ents = 0;
+ }
+ else
+ ASSERT(vio->io_req.state == STATE_IORESP_READY);
+
hvm_emulate_init_per_insn(hvmemul_ctxt, vio->mmio_insn,
vio->mmio_insn_bytes);
@@ -2564,6 +2597,7 @@ static int _hvm_emulate_one(struct hvm_e
{
vio->mmio_cache_count = 0;
vio->mmio_insn_bytes = 0;
+ hvmemul_cache_disable(curr);
}
else
{
@@ -2856,6 +2890,123 @@ void hvm_dump_emulation_state(const char
hvmemul_ctxt->insn_buf);
}
+int hvmemul_cache_init(struct vcpu *v)
+{
+ /*
+ * No insn can access more than 16 independent linear addresses (AVX512F
+ * scatters/gathers being the worst). Each such linear range can span a
+ * page boundary, i.e. may require two page walks. Account for each insn
+ * byte individually, for simplicity.
+ */
+ const unsigned int nents = (CONFIG_PAGING_LEVELS + 1) *
+ (MAX_INST_LEN + 16 * 2);
+ struct hvmemul_cache *cache = xmalloc_flex_struct(struct hvmemul_cache,
+ ents, nents);
+
+ if ( !cache )
+ return -ENOMEM;
+
+ /* Cache is disabled initially. */
+ cache->num_ents = nents + 1;
+ cache->max_ents = nents;
+
+ v->arch.hvm.hvm_io.cache = cache;
+
+ return 0;
+}
+
+unsigned int hvmemul_cache_disable(struct vcpu *v)
+{
+ struct hvmemul_cache *cache = v->arch.hvm.hvm_io.cache;
+ unsigned int token = cache->num_ents;
+
+ cache->num_ents = cache->max_ents + 1;
+
+ return token;
+}
+
+void hvmemul_cache_restore(struct vcpu *v, unsigned int token)
+{
+ struct hvmemul_cache *cache = v->arch.hvm.hvm_io.cache;
+
+ ASSERT(cache->num_ents > cache->max_ents);
+ cache->num_ents = token;
+}
+
+bool hvmemul_read_cache(const struct vcpu *v, paddr_t gpa,
+ void *buffer, unsigned int size)
+{
+ const struct hvmemul_cache *cache = v->arch.hvm.hvm_io.cache;
+ unsigned int i;
+
+ /* Cache unavailable? */
+ if ( cache->num_ents > cache->max_ents )
+ return false;
+
+ while ( size > sizeof(cache->ents->data) )
+ {
+ i = gpa & (sizeof(cache->ents->data) - 1)
+ ? -gpa & (sizeof(cache->ents->data) - 1)
+ : sizeof(cache->ents->data);
+ if ( !hvmemul_read_cache(v, gpa, buffer, i) )
+ return false;
+ gpa += i;
+ buffer += i;
+ size -= i;
+ }
+
+ for ( i = 0; i < cache->num_ents; ++i )
+ if ( cache->ents[i].gpa == gpa && cache->ents[i].size == size )
+ {
+ memcpy(buffer, &cache->ents[i].data, size);
+ return true;
+ }
+
+ return false;
+}
+
+void hvmemul_write_cache(const struct vcpu *v, paddr_t gpa,
+ const void *buffer, unsigned int size)
+{
+ struct hvmemul_cache *cache = v->arch.hvm.hvm_io.cache;
+ unsigned int i;
+
+ /* Cache unavailable? */
+ if ( cache->num_ents > cache->max_ents )
+ return;
+
+ while ( size > sizeof(cache->ents->data) )
+ {
+ i = gpa & (sizeof(cache->ents->data) - 1)
+ ? -gpa & (sizeof(cache->ents->data) - 1)
+ : sizeof(cache->ents->data);
+ hvmemul_write_cache(v, gpa, buffer, i);
+ gpa += i;
+ buffer += i;
+ size -= i;
+ }
+
+ for ( i = 0; i < cache->num_ents; ++i )
+ if ( cache->ents[i].gpa == gpa && cache->ents[i].size == size )
+ {
+ memcpy(&cache->ents[i].data, buffer, size);
+ return;
+ }
+
+ if ( unlikely(i >= cache->max_ents) )
+ {
+ domain_crash(v->domain);
+ return;
+ }
+
+ cache->ents[i].gpa = gpa;
+ cache->ents[i].size = size;
+
+ memcpy(&cache->ents[i].data, buffer, size);
+
+ cache->num_ents = i + 1;
+}
+
/*
* Local variables:
* mode: C
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -726,6 +726,8 @@ int hvm_domain_initialise(struct domain
/* This function and all its descendants need to be to be idempotent. */
void hvm_domain_relinquish_resources(struct domain *d)
{
+ struct vcpu *v;
+
if ( hvm_funcs.domain_relinquish_resources )
alternative_vcall(hvm_funcs.domain_relinquish_resources, d);
@@ -742,6 +744,9 @@ void hvm_domain_relinquish_resources(str
rtc_deinit(d);
pmtimer_deinit(d);
hpet_deinit(d);
+
+ for_each_vcpu ( d, v )
+ hvmemul_cache_destroy(v);
}
void hvm_domain_destroy(struct domain *d)
@@ -1549,6 +1554,10 @@ int hvm_vcpu_initialise(struct vcpu *v)
v->arch.hvm.inject_event.vector = HVM_EVENT_VECTOR_UNSET;
+ rc = hvmemul_cache_init(v);
+ if ( rc )
+ goto fail4;
+
rc = setup_compat_arg_xlat(v); /* teardown: free_compat_arg_xlat() */
if ( rc != 0 )
goto fail4;
@@ -1584,6 +1593,7 @@ int hvm_vcpu_initialise(struct vcpu *v)
fail5:
free_compat_arg_xlat(v);
fail4:
+ hvmemul_cache_destroy(v);
hvm_funcs.vcpu_destroy(v);
fail3:
vlapic_destroy(v);
@@ -2945,6 +2955,7 @@ void hvm_task_switch(
unsigned int eflags, new_cpl;
pagefault_info_t pfinfo;
int exn_raised, rc;
+ unsigned int token = hvmemul_cache_disable(v);
struct tss32 tss;
hvm_get_segment_register(v, x86_seg_gdtr, &gdt);
@@ -3152,6 +3163,8 @@ void hvm_task_switch(
out:
hvm_unmap_entry(optss_desc);
hvm_unmap_entry(nptss_desc);
+
+ hvmemul_cache_restore(v, token);
}
enum hvm_translation_result hvm_translate_get_page(
@@ -3242,7 +3255,6 @@ static enum hvm_translation_result __hvm
gfn_t gfn;
struct page_info *page;
p2m_type_t p2mt;
- char *p;
int count, todo = size;
ASSERT(is_hvm_vcpu(v));
@@ -3290,11 +3302,17 @@ static enum hvm_translation_result __hvm
return HVMTRANS_need_retry;
}
- p = __map_domain_page(page) + pgoff;
-
- if ( flags & HVMCOPY_to_guest )
+ if ( (flags & HVMCOPY_to_guest) ||
+ !hvmemul_read_cache(v, gfn_to_gaddr(gfn) | pgoff, buf, count) )
{
- if ( p2m_is_discard_write(p2mt) )
+ void *p = __map_domain_page(page) + pgoff;
+
+ if ( !(flags & HVMCOPY_to_guest) )
+ {
+ memcpy(buf, p, count);
+ hvmemul_write_cache(v, gfn_to_gaddr(gfn) | pgoff, buf, count);
+ }
+ else if ( p2m_is_discard_write(p2mt) )
{
static unsigned long lastpage;
@@ -3311,13 +3329,9 @@ static enum hvm_translation_result __hvm
memset(p, 0, count);
paging_mark_pfn_dirty(v->domain, _pfn(gfn_x(gfn)));
}
- }
- else
- {
- memcpy(buf, p, count);
- }
- unmap_domain_page(p);
+ unmap_domain_page(p);
+ }
addr += count;
if ( buf )
--- a/xen/arch/x86/hvm/hypercall.c
+++ b/xen/arch/x86/hvm/hypercall.c
@@ -22,6 +22,7 @@
#include <xen/hypercall.h>
#include <xen/nospec.h>
+#include <asm/hvm/emulate.h>
#include <asm/hvm/support.h>
static long hvm_memory_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
@@ -159,6 +160,7 @@ int hvm_hypercall(struct cpu_user_regs *
struct domain *currd = curr->domain;
int mode = hvm_guest_x86_mode(curr);
unsigned long eax = regs->eax;
+ unsigned int token;
switch ( mode )
{
@@ -183,7 +185,18 @@ int hvm_hypercall(struct cpu_user_regs *
}
if ( (eax & 0x80000000) && is_viridian_domain(currd) )
- return viridian_hypercall(regs);
+ {
+ int ret;
+
+ /* See comment below. */
+ token = hvmemul_cache_disable(curr);
+
+ ret = viridian_hypercall(regs);
+
+ hvmemul_cache_restore(curr, token);
+
+ return ret;
+ }
BUILD_BUG_ON(ARRAY_SIZE(hvm_hypercall_table) >
ARRAY_SIZE(hypercall_args_table));
@@ -202,6 +215,12 @@ int hvm_hypercall(struct cpu_user_regs *
return HVM_HCALL_completed;
}
+ /*
+ * Caching is intended for instruction emulation only. Disable it
+ * for any accesses by hypercall argument copy-in / copy-out.
+ */
+ token = hvmemul_cache_disable(curr);
+
curr->hcall_preempted = false;
if ( mode == 8 )
@@ -295,6 +314,8 @@ int hvm_hypercall(struct cpu_user_regs *
#endif
}
+ hvmemul_cache_restore(curr, token);
+
HVM_DBG_LOG(DBG_LEVEL_HCALL, "hcall%lu -> %lx", eax, regs->rax);
if ( curr->hcall_preempted )
--- a/xen/arch/x86/hvm/intercept.c
+++ b/xen/arch/x86/hvm/intercept.c
@@ -20,6 +20,7 @@
#include <xen/types.h>
#include <xen/sched.h>
#include <asm/regs.h>
+#include <asm/hvm/emulate.h>
#include <asm/hvm/hvm.h>
#include <asm/hvm/support.h>
#include <asm/hvm/domain.h>
@@ -163,6 +164,9 @@ int hvm_process_io_intercept(const struc
{
if ( p->data_is_ptr )
{
+ struct vcpu *curr = current;
+ unsigned int token = hvmemul_cache_disable(curr);
+
data = 0;
switch ( hvm_copy_from_guest_phys(&data, p->data + step * i,
p->size) )
@@ -179,9 +183,11 @@ int hvm_process_io_intercept(const struc
ASSERT_UNREACHABLE();
/* fall through */
default:
- domain_crash(current->domain);
+ domain_crash(curr->domain);
return X86EMUL_UNHANDLEABLE;
}
+
+ hvmemul_cache_restore(curr, token);
}
else
data = p->data;
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -1040,6 +1040,8 @@ void svm_vmenter_helper(const struct cpu
struct vcpu *curr = current;
struct vmcb_struct *vmcb = curr->arch.hvm.svm.vmcb;
+ ASSERT(hvmemul_cache_disabled(curr));
+
svm_asid_handle_vmrun();
if ( unlikely(tb_init_done) )
--- a/xen/arch/x86/hvm/vmsi.c
+++ b/xen/arch/x86/hvm/vmsi.c
@@ -35,6 +35,7 @@
#include <xen/irq.h>
#include <xen/vpci.h>
#include <public/hvm/ioreq.h>
+#include <asm/hvm/emulate.h>
#include <asm/hvm/io.h>
#include <asm/hvm/vpic.h>
#include <asm/hvm/vlapic.h>
@@ -607,6 +608,7 @@ void msix_write_completion(struct vcpu *
if ( !ctrl_address && snoop_addr &&
v->arch.hvm.hvm_io.msix_snoop_gpa )
{
+ unsigned int token = hvmemul_cache_disable(v);
const struct msi_desc *desc;
uint32_t data;
@@ -621,6 +623,8 @@ void msix_write_completion(struct vcpu *
sizeof(data)) == HVMTRANS_okay &&
!(data & PCI_MSIX_VECTOR_BITMASK) )
ctrl_address = snoop_addr;
+
+ hvmemul_cache_restore(v, token);
}
if ( !ctrl_address )
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -4362,6 +4362,8 @@ bool vmx_vmenter_helper(const struct cpu
struct hvm_vcpu_asid *p_asid;
bool_t need_flush;
+ ASSERT(hvmemul_cache_disabled(curr));
+
/* Shadow EPTP can't be updated here because irqs are disabled */
if ( nestedhvm_vcpu_in_guestmode(curr) && vcpu_nestedhvm(curr).stale_np2m
)
return false;
--- a/xen/include/asm-x86/hvm/emulate.h
+++ b/xen/include/asm-x86/hvm/emulate.h
@@ -13,6 +13,7 @@
#define __ASM_X86_HVM_EMULATE_H__
#include <xen/err.h>
+#include <xen/sched.h>
#include <asm/hvm/hvm.h>
#include <asm/x86_emulate.h>
@@ -97,6 +98,39 @@ int hvmemul_do_pio_buffer(uint16_t port,
uint8_t dir,
void *buffer);
+#ifdef CONFIG_HVM
+/*
+ * The cache controlled by the functions below is not like an ordinary CPU
+ * cache, i.e. aiming to help performance, but a "secret store" which is
+ * needed for correctness. The issue it helps addressing is the need for
+ * re-execution of an insn (after data was provided by a device model) to
+ * observe the exact same memory state, i.e. to specifically not observe any
+ * updates which may have occurred in the meantime by other agents.
+ * Therefore this cache gets
+ * - enabled when emulation of an insn starts,
+ * - disabled across processing secondary things like a hypercall resulting
+ * from insn emulation,
+ * - disabled again when an emulated insn is known to not require any
+ * further re-execution.
+ */
+int __must_check hvmemul_cache_init(struct vcpu *v);
+static inline void hvmemul_cache_destroy(struct vcpu *v)
+{
+ XFREE(v->arch.hvm.hvm_io.cache);
+}
+bool hvmemul_read_cache(const struct vcpu *, paddr_t gpa,
+ void *buffer, unsigned int size);
+void hvmemul_write_cache(const struct vcpu *, paddr_t gpa,
+ const void *buffer, unsigned int size);
+unsigned int hvmemul_cache_disable(struct vcpu *);
+void hvmemul_cache_restore(struct vcpu *, unsigned int token);
+/* For use in ASSERT()s only: */
+static inline bool hvmemul_cache_disabled(struct vcpu *v)
+{
+ return hvmemul_cache_disable(v) == hvmemul_cache_disable(v);
+}
+#endif
+
void hvm_dump_emulation_state(const char *loglvl, const char *prefix,
struct hvm_emulate_ctxt *hvmemul_ctxt, int rc);
--- a/xen/include/asm-x86/hvm/vcpu.h
+++ b/xen/include/asm-x86/hvm/vcpu.h
@@ -77,6 +77,8 @@ struct hvm_vcpu_io {
/* For retries we shouldn't re-fetch the instruction. */
unsigned int mmio_insn_bytes;
unsigned char mmio_insn[16];
+ struct hvmemul_cache *cache;
+
/*
* For string instruction emulation we need to be able to signal a
* necessary retry through other than function return codes.
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |