* [PATCH v2 1/2] vm_event: Sanitize vm_event response handling
@ 2016-09-15 16:51 Tamas K Lengyel
2016-09-15 16:51 ` [PATCH v2 2/2] x86/vm_event: Allow overwriting Xen's i-cache used for emulation Tamas K Lengyel
2016-09-17 16:36 ` [PATCH v2 1/2] vm_event: Sanitize vm_event response handling Razvan Cojocaru
0 siblings, 2 replies; 19+ messages in thread
From: Tamas K Lengyel @ 2016-09-15 16:51 UTC (permalink / raw)
To: xen-devel
Cc: Stefano Stabellini, Razvan Cojocaru, Tamas K Lengyel,
Julien Grall, Jan Beulich, Andrew Cooper
Setting response flags in vm_event are only ever safe if the vCPUs are paused.
To reflect this we move all checks within the if block that already checks
whether this is the case. Checks that are only supported on one architecture
we relocate the bitmask operations to the arch-specific handlers to avoid
the overhead on architectures that don't support it.
Furthermore, we clean-up the emulation checks so it more clearly represents the
decision-logic when emulation should take place. As part of this we also
set the stage to allow emulation in response to other types of events, not just
mem_access violations.
Signed-off-by: Tamas K Lengyel <tamas.lengyel@zentific.com>
Acked-by: George Dunlap <george.dunlap@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Razvan Cojocaru <rcojocaru@bitdefender.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Julien Grall <julien.grall@arm.com>
v2: use bool instead of bool_t
---
xen/arch/x86/mm/p2m.c | 79 +++++++++++++++++++-----------------------
xen/arch/x86/vm_event.c | 35 ++++++++++++++++++-
xen/common/vm_event.c | 53 ++++++++++++++--------------
xen/include/asm-arm/p2m.h | 3 +-
xen/include/asm-arm/vm_event.h | 9 ++++-
xen/include/asm-x86/p2m.h | 2 +-
xen/include/asm-x86/vm_event.h | 5 ++-
xen/include/xen/mem_access.h | 12 -------
8 files changed, 111 insertions(+), 87 deletions(-)
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index 7d14c3b..faffc2a 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -1588,62 +1588,55 @@ void p2m_mem_paging_resume(struct domain *d, vm_event_response_t *rsp)
}
}
-void p2m_mem_access_emulate_check(struct vcpu *v,
+bool p2m_mem_access_emulate_check(struct vcpu *v,
const vm_event_response_t *rsp)
{
- /* Mark vcpu for skipping one instruction upon rescheduling. */
- if ( rsp->flags & VM_EVENT_FLAG_EMULATE )
- {
- xenmem_access_t access;
- bool_t violation = 1;
- const struct vm_event_mem_access *data = &rsp->u.mem_access;
+ xenmem_access_t access;
+ bool violation = 1;
+ const struct vm_event_mem_access *data = &rsp->u.mem_access;
- if ( p2m_get_mem_access(v->domain, _gfn(data->gfn), &access) == 0 )
+ if ( p2m_get_mem_access(v->domain, _gfn(data->gfn), &access) == 0 )
+ {
+ switch ( access )
{
- switch ( access )
- {
- case XENMEM_access_n:
- case XENMEM_access_n2rwx:
- default:
- violation = data->flags & MEM_ACCESS_RWX;
- break;
+ case XENMEM_access_n:
+ case XENMEM_access_n2rwx:
+ default:
+ violation = data->flags & MEM_ACCESS_RWX;
+ break;
- case XENMEM_access_r:
- violation = data->flags & MEM_ACCESS_WX;
- break;
+ case XENMEM_access_r:
+ violation = data->flags & MEM_ACCESS_WX;
+ break;
- case XENMEM_access_w:
- violation = data->flags & MEM_ACCESS_RX;
- break;
+ case XENMEM_access_w:
+ violation = data->flags & MEM_ACCESS_RX;
+ break;
- case XENMEM_access_x:
- violation = data->flags & MEM_ACCESS_RW;
- break;
+ case XENMEM_access_x:
+ violation = data->flags & MEM_ACCESS_RW;
+ break;
- case XENMEM_access_rx:
- case XENMEM_access_rx2rw:
- violation = data->flags & MEM_ACCESS_W;
- break;
+ case XENMEM_access_rx:
+ case XENMEM_access_rx2rw:
+ violation = data->flags & MEM_ACCESS_W;
+ break;
- case XENMEM_access_wx:
- violation = data->flags & MEM_ACCESS_R;
- break;
+ case XENMEM_access_wx:
+ violation = data->flags & MEM_ACCESS_R;
+ break;
- case XENMEM_access_rw:
- violation = data->flags & MEM_ACCESS_X;
- break;
+ case XENMEM_access_rw:
+ violation = data->flags & MEM_ACCESS_X;
+ break;
- case XENMEM_access_rwx:
- violation = 0;
- break;
- }
+ case XENMEM_access_rwx:
+ violation = 0;
+ break;
}
-
- v->arch.vm_event->emulate_flags = violation ? rsp->flags : 0;
-
- if ( (rsp->flags & VM_EVENT_FLAG_SET_EMUL_READ_DATA) )
- v->arch.vm_event->emul_read_data = rsp->data.emul_read_data;
}
+
+ return violation;
}
void p2m_altp2m_check(struct vcpu *v, uint16_t idx)
diff --git a/xen/arch/x86/vm_event.c b/xen/arch/x86/vm_event.c
index e938ca3..343b9c8 100644
--- a/xen/arch/x86/vm_event.c
+++ b/xen/arch/x86/vm_event.c
@@ -18,6 +18,7 @@
* License along with this program; If not, see <http://www.gnu.org/licenses/>.
*/
+#include <asm/p2m.h>
#include <asm/vm_event.h>
/* Implicitly serialized by the domctl lock. */
@@ -56,8 +57,12 @@ void vm_event_cleanup_domain(struct domain *d)
d->arch.mem_access_emulate_each_rep = 0;
}
-void vm_event_toggle_singlestep(struct domain *d, struct vcpu *v)
+void vm_event_toggle_singlestep(struct domain *d, struct vcpu *v,
+ vm_event_response_t *rsp)
{
+ if ( !(rsp->flags & VM_EVENT_FLAG_TOGGLE_SINGLESTEP) )
+ return;
+
if ( !is_hvm_domain(d) )
return;
@@ -186,6 +191,34 @@ void vm_event_fill_regs(vm_event_request_t *req)
req->data.regs.x86.cs_arbytes = seg.attr.bytes;
}
+void vm_event_emulate_check(struct vcpu *v, vm_event_response_t *rsp)
+{
+ if ( !(rsp->flags & VM_EVENT_FLAG_EMULATE) )
+ {
+ v->arch.vm_event->emulate_flags = 0;
+ return;
+ }
+
+ switch ( rsp->reason )
+ {
+ case VM_EVENT_REASON_MEM_ACCESS:
+ /*
+ * Emulate iff this is a response to a mem_access violation and there
+ * are still conflicting mem_access permissions in-place.
+ */
+ if ( p2m_mem_access_emulate_check(v, rsp) )
+ {
+ if ( rsp->flags & VM_EVENT_FLAG_SET_EMUL_READ_DATA )
+ v->arch.vm_event->emul_read_data = rsp->data.emul_read_data;
+
+ v->arch.vm_event->emulate_flags = rsp->flags;
+ }
+ break;
+ default:
+ break;
+ };
+}
+
/*
* Local variables:
* mode: C
diff --git a/xen/common/vm_event.c b/xen/common/vm_event.c
index 8398af7..907ab40 100644
--- a/xen/common/vm_event.c
+++ b/xen/common/vm_event.c
@@ -398,42 +398,41 @@ void vm_event_resume(struct domain *d, struct vm_event_domain *ved)
* In some cases the response type needs extra handling, so here
* we call the appropriate handlers.
*/
- switch ( rsp.reason )
- {
-#ifdef CONFIG_X86
- case VM_EVENT_REASON_MOV_TO_MSR:
-#endif
- case VM_EVENT_REASON_WRITE_CTRLREG:
- vm_event_register_write_resume(v, &rsp);
- break;
-
-#ifdef CONFIG_HAS_MEM_ACCESS
- case VM_EVENT_REASON_MEM_ACCESS:
- mem_access_resume(v, &rsp);
- break;
-#endif
+ /* Check flags which apply only when the vCPU is paused */
+ if ( atomic_read(&v->vm_event_pause_count) )
+ {
#ifdef CONFIG_HAS_MEM_PAGING
- case VM_EVENT_REASON_MEM_PAGING:
- p2m_mem_paging_resume(d, &rsp);
- break;
+ if ( rsp.reason == VM_EVENT_REASON_MEM_PAGING )
+ p2m_mem_paging_resume(d, &rsp);
#endif
- };
+ /*
+ * Check emulation flags in the arch-specific handler only, as it
+ * has to set arch-specific flags when supported, and to avoid
+ * bitmask overhead when it isn't supported.
+ */
+ vm_event_emulate_check(v, &rsp);
+
+ /*
+ * Check in arch-specific handler to avoid bitmask overhead when
+ * not supported.
+ */
+ vm_event_register_write_resume(v, &rsp);
- /* Check for altp2m switch */
- if ( rsp.flags & VM_EVENT_FLAG_ALTERNATE_P2M )
- p2m_altp2m_check(v, rsp.altp2m_idx);
+ /*
+ * Check in arch-specific handler to avoid bitmask overhead when
+ * not supported.
+ */
+ vm_event_toggle_singlestep(d, v, &rsp);
+
+ /* Check for altp2m switch */
+ if ( rsp.flags & VM_EVENT_FLAG_ALTERNATE_P2M )
+ p2m_altp2m_check(v, rsp.altp2m_idx);
- /* Check flags which apply only when the vCPU is paused */
- if ( atomic_read(&v->vm_event_pause_count) )
- {
if ( rsp.flags & VM_EVENT_FLAG_SET_REGISTERS )
vm_event_set_registers(v, &rsp);
- if ( rsp.flags & VM_EVENT_FLAG_TOGGLE_SINGLESTEP )
- vm_event_toggle_singlestep(d, v);
-
if ( rsp.flags & VM_EVENT_FLAG_VCPU_PAUSED )
vm_event_vcpu_unpause(v);
}
diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
index 53c4d78..6251b37 100644
--- a/xen/include/asm-arm/p2m.h
+++ b/xen/include/asm-arm/p2m.h
@@ -121,10 +121,11 @@ typedef enum {
p2m_to_mask(p2m_map_foreign)))
static inline
-void p2m_mem_access_emulate_check(struct vcpu *v,
+bool p2m_mem_access_emulate_check(struct vcpu *v,
const vm_event_response_t *rsp)
{
/* Not supported on ARM. */
+ return 0;
}
static inline
diff --git a/xen/include/asm-arm/vm_event.h b/xen/include/asm-arm/vm_event.h
index 9482636..66f2474 100644
--- a/xen/include/asm-arm/vm_event.h
+++ b/xen/include/asm-arm/vm_event.h
@@ -34,7 +34,8 @@ static inline void vm_event_cleanup_domain(struct domain *d)
memset(&d->monitor, 0, sizeof(d->monitor));
}
-static inline void vm_event_toggle_singlestep(struct domain *d, struct vcpu *v)
+static inline void vm_event_toggle_singlestep(struct domain *d, struct vcpu *v,
+ vm_event_response_t *rsp)
{
/* Not supported on ARM. */
}
@@ -45,4 +46,10 @@ void vm_event_register_write_resume(struct vcpu *v, vm_event_response_t *rsp)
/* Not supported on ARM. */
}
+static inline
+void vm_event_emulate_check(struct vcpu *v, vm_event_response_t *rsp)
+{
+ /* Not supported on ARM. */
+}
+
#endif /* __ASM_ARM_VM_EVENT_H__ */
diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
index 9fc9ead..7035860 100644
--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -677,7 +677,7 @@ bool_t p2m_mem_access_check(paddr_t gpa, unsigned long gla,
/* Check for emulation and mark vcpu for skipping one instruction
* upon rescheduling if required. */
-void p2m_mem_access_emulate_check(struct vcpu *v,
+bool p2m_mem_access_emulate_check(struct vcpu *v,
const vm_event_response_t *rsp);
/* Sanity check for mem_access hardware support */
diff --git a/xen/include/asm-x86/vm_event.h b/xen/include/asm-x86/vm_event.h
index 294def6..ebb5d88 100644
--- a/xen/include/asm-x86/vm_event.h
+++ b/xen/include/asm-x86/vm_event.h
@@ -35,8 +35,11 @@ int vm_event_init_domain(struct domain *d);
void vm_event_cleanup_domain(struct domain *d);
-void vm_event_toggle_singlestep(struct domain *d, struct vcpu *v);
+void vm_event_toggle_singlestep(struct domain *d, struct vcpu *v,
+ vm_event_response_t *rsp);
void vm_event_register_write_resume(struct vcpu *v, vm_event_response_t *rsp);
+void vm_event_emulate_check(struct vcpu *v, vm_event_response_t *rsp);
+
#endif /* __ASM_X86_VM_EVENT_H__ */
diff --git a/xen/include/xen/mem_access.h b/xen/include/xen/mem_access.h
index 3d054e0..da36e07 100644
--- a/xen/include/xen/mem_access.h
+++ b/xen/include/xen/mem_access.h
@@ -30,12 +30,6 @@
int mem_access_memop(unsigned long cmd,
XEN_GUEST_HANDLE_PARAM(xen_mem_access_op_t) arg);
-static inline
-void mem_access_resume(struct vcpu *v, vm_event_response_t *rsp)
-{
- p2m_mem_access_emulate_check(v, rsp);
-}
-
#else
static inline
@@ -45,12 +39,6 @@ int mem_access_memop(unsigned long cmd,
return -ENOSYS;
}
-static inline
-void mem_access_resume(struct vcpu *vcpu, vm_event_response_t *rsp)
-{
- /* Nothing to do. */
-}
-
#endif /* HAS_MEM_ACCESS */
#endif /* _XEN_ASM_MEM_ACCESS_H */
--
2.9.3
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH v2 2/2] x86/vm_event: Allow overwriting Xen's i-cache used for emulation
2016-09-15 16:51 [PATCH v2 1/2] vm_event: Sanitize vm_event response handling Tamas K Lengyel
@ 2016-09-15 16:51 ` Tamas K Lengyel
2016-09-17 16:40 ` Razvan Cojocaru
2016-09-19 8:19 ` Jan Beulich
2016-09-17 16:36 ` [PATCH v2 1/2] vm_event: Sanitize vm_event response handling Razvan Cojocaru
1 sibling, 2 replies; 19+ messages in thread
From: Tamas K Lengyel @ 2016-09-15 16:51 UTC (permalink / raw)
To: xen-devel
Cc: Kevin Tian, Stefano Stabellini, Jan Beulich, Razvan Cojocaru,
George Dunlap, Tamas K Lengyel, Julien Grall, Paul Durrant,
Jun Nakajima, Andrew Cooper
When emulating instructions Xen's emulator maintains a small i-cache fetched
from the guest memory. This patch extends the vm_event interface to allow
overwriting this i-cache via a buffer returned in the vm_event response.
When responding to a SOFTWARE_BREAKPOINT event (INT3) the monitor subscriber
normally has to remove the INT3 from memory - singlestep - place back INT3
to allow the guest to continue execution. This routine however is susceptible
to a race-condition on multi-vCPU guests. By allowing the subscriber to return
the i-cache to be used for emulation it can side-step the problem by returning
a clean buffer without the INT3 present.
As part of this patch we rename hvm_mem_access_emulate_one to
hvm_emulate_one_vm_event to better reflect that it is used in various vm_event
scenarios now, not just in response to mem_access events.
Signed-off-by: Tamas K Lengyel <tamas.lengyel@zentific.com>
---
Cc: Paul Durrant <paul.durrant@citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Jun Nakajima <jun.nakajima@intel.com>
Cc: Kevin Tian <kevin.tian@intel.com>
Cc: George Dunlap <george.dunlap@eu.citrix.com>
Cc: Razvan Cojocaru <rcojocaru@bitdefender.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Julien Grall <julien.grall@arm.com>
v2: rework hvm_mem_access_emulate_one switch statement
add BUILD_BUG_ON to ensure internal and vm_event buffer sizes match
Note: this patch has now been fully tested and works as intended
---
xen/arch/x86/hvm/emulate.c | 37 ++++++++++++++++++++++++-------------
xen/arch/x86/hvm/hvm.c | 9 ++++++---
xen/arch/x86/hvm/vmx/vmx.c | 1 +
xen/arch/x86/vm_event.c | 9 ++++++++-
xen/common/vm_event.c | 1 -
xen/include/asm-x86/hvm/emulate.h | 8 +++++---
xen/include/asm-x86/vm_event.h | 5 ++++-
xen/include/public/vm_event.h | 16 +++++++++++++++-
8 files changed, 63 insertions(+), 23 deletions(-)
diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
index cc25676..acae998 100644
--- a/xen/arch/x86/hvm/emulate.c
+++ b/xen/arch/x86/hvm/emulate.c
@@ -76,9 +76,9 @@ static int set_context_data(void *buffer, unsigned int size)
if ( curr->arch.vm_event )
{
unsigned int safe_size =
- min(size, curr->arch.vm_event->emul_read_data.size);
+ min(size, curr->arch.vm_event->emul.read.size);
- memcpy(buffer, curr->arch.vm_event->emul_read_data.data, safe_size);
+ memcpy(buffer, curr->arch.vm_event->emul.read.data, safe_size);
memset(buffer + safe_size, 0, size - safe_size);
return X86EMUL_OKAY;
}
@@ -827,7 +827,7 @@ static int hvmemul_read(
struct hvm_emulate_ctxt *hvmemul_ctxt =
container_of(ctxt, struct hvm_emulate_ctxt, ctxt);
- if ( unlikely(hvmemul_ctxt->set_context) )
+ if ( unlikely(hvmemul_ctxt->set_context_data) )
return set_context_data(p_data, bytes);
return __hvmemul_read(
@@ -1029,7 +1029,7 @@ static int hvmemul_cmpxchg(
struct hvm_emulate_ctxt *hvmemul_ctxt =
container_of(ctxt, struct hvm_emulate_ctxt, ctxt);
- if ( unlikely(hvmemul_ctxt->set_context) )
+ if ( unlikely(hvmemul_ctxt->set_context_data) )
{
int rc = set_context_data(p_new, bytes);
@@ -1122,7 +1122,7 @@ static int hvmemul_rep_outs(
p2m_type_t p2mt;
int rc;
- if ( unlikely(hvmemul_ctxt->set_context) )
+ if ( unlikely(hvmemul_ctxt->set_context_data) )
return hvmemul_rep_outs_set_context(src_seg, src_offset, dst_port,
bytes_per_rep, reps, ctxt);
@@ -1264,7 +1264,7 @@ static int hvmemul_rep_movs(
if ( buf == NULL )
return X86EMUL_UNHANDLEABLE;
- if ( unlikely(hvmemul_ctxt->set_context) )
+ if ( unlikely(hvmemul_ctxt->set_context_data) )
{
rc = set_context_data(buf, bytes);
@@ -1470,7 +1470,7 @@ static int hvmemul_read_io(
*val = 0;
- if ( unlikely(hvmemul_ctxt->set_context) )
+ if ( unlikely(hvmemul_ctxt->set_context_data) )
return set_context_data(val, bytes);
return hvmemul_do_pio_buffer(port, bytes, IOREQ_READ, val);
@@ -1793,7 +1793,17 @@ static int _hvm_emulate_one(struct hvm_emulate_ctxt *hvmemul_ctxt,
pfec |= PFEC_user_mode;
hvmemul_ctxt->insn_buf_eip = regs->eip;
- if ( !vio->mmio_insn_bytes )
+
+ if ( unlikely(hvmemul_ctxt->set_context_insn) && curr->arch.vm_event )
+ {
+ BUILD_BUG_ON(sizeof(hvmemul_ctxt->insn_buf_bytes) ==
+ sizeof(curr->arch.vm_event->emul.insn));
+
+ hvmemul_ctxt->insn_buf_bytes = sizeof(curr->arch.vm_event->emul.insn);
+ memcpy(hvmemul_ctxt->insn_buf, &curr->arch.vm_event->emul.insn,
+ hvmemul_ctxt->insn_buf_bytes);
+ }
+ else if ( !vio->mmio_insn_bytes )
{
hvmemul_ctxt->insn_buf_bytes =
hvm_get_insn_bytes(curr, hvmemul_ctxt->insn_buf) ?:
@@ -1931,7 +1941,7 @@ int hvm_emulate_one_mmio(unsigned long mfn, unsigned long gla)
return rc;
}
-void hvm_mem_access_emulate_one(enum emul_kind kind, unsigned int trapnr,
+void hvm_emulate_one_vm_event(enum emul_kind kind, unsigned int trapnr,
unsigned int errcode)
{
struct hvm_emulate_ctxt ctx = {{ 0 }};
@@ -1944,11 +1954,11 @@ void hvm_mem_access_emulate_one(enum emul_kind kind, unsigned int trapnr,
case EMUL_KIND_NOWRITE:
rc = hvm_emulate_one_no_write(&ctx);
break;
- case EMUL_KIND_SET_CONTEXT:
- ctx.set_context = 1;
- /* Intentional fall-through. */
default:
+ ctx.set_context_data = (kind == EMUL_KIND_SET_CONTEXT_DATA);
+ ctx.set_context_insn = (kind == EMUL_KIND_SET_CONTEXT_INSN);
rc = hvm_emulate_one(&ctx);
+ break;
}
switch ( rc )
@@ -1983,7 +1993,8 @@ void hvm_emulate_prepare(
hvmemul_ctxt->ctxt.force_writeback = 1;
hvmemul_ctxt->seg_reg_accessed = 0;
hvmemul_ctxt->seg_reg_dirty = 0;
- hvmemul_ctxt->set_context = 0;
+ hvmemul_ctxt->set_context_data = 0;
+ hvmemul_ctxt->set_context_insn = 0;
hvmemul_get_seg_reg(x86_seg_cs, hvmemul_ctxt);
hvmemul_get_seg_reg(x86_seg_ss, hvmemul_ctxt);
}
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index ca96643..7462794 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -489,13 +489,16 @@ void hvm_do_resume(struct vcpu *v)
if ( v->arch.vm_event->emulate_flags &
VM_EVENT_FLAG_SET_EMUL_READ_DATA )
- kind = EMUL_KIND_SET_CONTEXT;
+ kind = EMUL_KIND_SET_CONTEXT_DATA;
else if ( v->arch.vm_event->emulate_flags &
VM_EVENT_FLAG_EMULATE_NOWRITE )
kind = EMUL_KIND_NOWRITE;
+ else if ( v->arch.vm_event->emulate_flags &
+ VM_EVENT_FLAG_SET_EMUL_INSN_DATA )
+ kind = EMUL_KIND_SET_CONTEXT_INSN;
- hvm_mem_access_emulate_one(kind, TRAP_invalid_op,
- HVM_DELIVER_NO_ERROR_CODE);
+ hvm_emulate_one_vm_event(kind, TRAP_invalid_op,
+ HVM_DELIVER_NO_ERROR_CODE);
v->arch.vm_event->emulate_flags = 0;
}
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 2759e6f..d214716 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -57,6 +57,7 @@
#include <asm/altp2m.h>
#include <asm/event.h>
#include <asm/monitor.h>
+#include <asm/vm_event.h>
#include <public/arch-x86/cpuid.h>
static bool_t __initdata opt_force_ept;
diff --git a/xen/arch/x86/vm_event.c b/xen/arch/x86/vm_event.c
index 343b9c8..ca5d515 100644
--- a/xen/arch/x86/vm_event.c
+++ b/xen/arch/x86/vm_event.c
@@ -209,11 +209,18 @@ void vm_event_emulate_check(struct vcpu *v, vm_event_response_t *rsp)
if ( p2m_mem_access_emulate_check(v, rsp) )
{
if ( rsp->flags & VM_EVENT_FLAG_SET_EMUL_READ_DATA )
- v->arch.vm_event->emul_read_data = rsp->data.emul_read_data;
+ v->arch.vm_event->emul.read = rsp->data.emul.read;
v->arch.vm_event->emulate_flags = rsp->flags;
}
break;
+ case VM_EVENT_REASON_SOFTWARE_BREAKPOINT:
+ if ( rsp->flags & VM_EVENT_FLAG_SET_EMUL_INSN_DATA )
+ {
+ v->arch.vm_event->emul.insn = rsp->data.emul.insn;
+ v->arch.vm_event->emulate_flags = rsp->flags;
+ }
+ break;
default:
break;
};
diff --git a/xen/common/vm_event.c b/xen/common/vm_event.c
index 907ab40..d8ee7f3 100644
--- a/xen/common/vm_event.c
+++ b/xen/common/vm_event.c
@@ -398,7 +398,6 @@ void vm_event_resume(struct domain *d, struct vm_event_domain *ved)
* In some cases the response type needs extra handling, so here
* we call the appropriate handlers.
*/
-
/* Check flags which apply only when the vCPU is paused */
if ( atomic_read(&v->vm_event_pause_count) )
{
diff --git a/xen/include/asm-x86/hvm/emulate.h b/xen/include/asm-x86/hvm/emulate.h
index 3aabcbe..b52f99e 100644
--- a/xen/include/asm-x86/hvm/emulate.h
+++ b/xen/include/asm-x86/hvm/emulate.h
@@ -34,20 +34,22 @@ struct hvm_emulate_ctxt {
uint32_t intr_shadow;
- bool_t set_context;
+ bool_t set_context_data;
+ bool_t set_context_insn;
};
enum emul_kind {
EMUL_KIND_NORMAL,
EMUL_KIND_NOWRITE,
- EMUL_KIND_SET_CONTEXT
+ EMUL_KIND_SET_CONTEXT_DATA,
+ EMUL_KIND_SET_CONTEXT_INSN
};
int hvm_emulate_one(
struct hvm_emulate_ctxt *hvmemul_ctxt);
int hvm_emulate_one_no_write(
struct hvm_emulate_ctxt *hvmemul_ctxt);
-void hvm_mem_access_emulate_one(enum emul_kind kind,
+void hvm_emulate_one_vm_event(enum emul_kind kind,
unsigned int trapnr,
unsigned int errcode);
void hvm_emulate_prepare(
diff --git a/xen/include/asm-x86/vm_event.h b/xen/include/asm-x86/vm_event.h
index ebb5d88..ca73f99 100644
--- a/xen/include/asm-x86/vm_event.h
+++ b/xen/include/asm-x86/vm_event.h
@@ -27,7 +27,10 @@
*/
struct arch_vm_event {
uint32_t emulate_flags;
- struct vm_event_emul_read_data emul_read_data;
+ union {
+ struct vm_event_emul_read_data read;
+ struct vm_event_emul_insn_data insn;
+ } emul;
struct monitor_write_data write_data;
};
diff --git a/xen/include/public/vm_event.h b/xen/include/public/vm_event.h
index f756126..ef62932 100644
--- a/xen/include/public/vm_event.h
+++ b/xen/include/public/vm_event.h
@@ -97,6 +97,13 @@
* Requires the vCPU to be paused already (synchronous events only).
*/
#define VM_EVENT_FLAG_SET_REGISTERS (1 << 8)
+/*
+ * Instruction cache is being sent back to the hypervisor in the event response
+ * to be used by the emulator. This flag is only useful when combined with
+ * VM_EVENT_FLAG_EMULATE and is incompatible with also setting
+ * VM_EVENT_FLAG_EMULATE_NOWRITE or VM_EVENT_FLAG_SET_EMUL_READ_DATA.
+ */
+#define VM_EVENT_FLAG_SET_EMUL_INSN_DATA (1 << 9)
/*
* Reasons for the vm event request
@@ -265,6 +272,10 @@ struct vm_event_emul_read_data {
uint8_t data[sizeof(struct vm_event_regs_x86) - sizeof(uint32_t)];
};
+struct vm_event_emul_insn_data {
+ uint8_t data[16]; /* Has to be completely filled */
+};
+
typedef struct vm_event_st {
uint32_t version; /* VM_EVENT_INTERFACE_VERSION */
uint32_t flags; /* VM_EVENT_FLAG_* */
@@ -291,7 +302,10 @@ typedef struct vm_event_st {
struct vm_event_regs_arm arm;
} regs;
- struct vm_event_emul_read_data emul_read_data;
+ union {
+ struct vm_event_emul_read_data read;
+ struct vm_event_emul_insn_data insn;
+ } emul;
} data;
} vm_event_request_t, vm_event_response_t;
--
2.9.3
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH v2 1/2] vm_event: Sanitize vm_event response handling
2016-09-15 16:51 [PATCH v2 1/2] vm_event: Sanitize vm_event response handling Tamas K Lengyel
2016-09-15 16:51 ` [PATCH v2 2/2] x86/vm_event: Allow overwriting Xen's i-cache used for emulation Tamas K Lengyel
@ 2016-09-17 16:36 ` Razvan Cojocaru
1 sibling, 0 replies; 19+ messages in thread
From: Razvan Cojocaru @ 2016-09-17 16:36 UTC (permalink / raw)
To: Tamas K Lengyel, xen-devel
Cc: Andrew Cooper, Julien Grall, Stefano Stabellini, Jan Beulich
On 09/15/16 19:51, Tamas K Lengyel wrote:
> Setting response flags in vm_event are only ever safe if the vCPUs are paused.
> To reflect this we move all checks within the if block that already checks
> whether this is the case. Checks that are only supported on one architecture
> we relocate the bitmask operations to the arch-specific handlers to avoid
> the overhead on architectures that don't support it.
>
> Furthermore, we clean-up the emulation checks so it more clearly represents the
> decision-logic when emulation should take place. As part of this we also
> set the stage to allow emulation in response to other types of events, not just
> mem_access violations.
>
> Signed-off-by: Tamas K Lengyel <tamas.lengyel@zentific.com>
> Acked-by: George Dunlap <george.dunlap@citrix.com>
> ---
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: Razvan Cojocaru <rcojocaru@bitdefender.com>
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Julien Grall <julien.grall@arm.com>
>
> v2: use bool instead of bool_t
Acked-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
Thanks,
Razvan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 2/2] x86/vm_event: Allow overwriting Xen's i-cache used for emulation
2016-09-15 16:51 ` [PATCH v2 2/2] x86/vm_event: Allow overwriting Xen's i-cache used for emulation Tamas K Lengyel
@ 2016-09-17 16:40 ` Razvan Cojocaru
2016-09-17 16:54 ` Tamas K Lengyel
2016-09-19 8:19 ` Jan Beulich
1 sibling, 1 reply; 19+ messages in thread
From: Razvan Cojocaru @ 2016-09-17 16:40 UTC (permalink / raw)
To: Tamas K Lengyel, xen-devel
Cc: Kevin Tian, Stefano Stabellini, Jan Beulich, George Dunlap,
Andrew Cooper, Julien Grall, Paul Durrant, Jun Nakajima
On 09/15/16 19:51, Tamas K Lengyel wrote:
> When emulating instructions Xen's emulator maintains a small i-cache fetched
> from the guest memory. This patch extends the vm_event interface to allow
> overwriting this i-cache via a buffer returned in the vm_event response.
>
> When responding to a SOFTWARE_BREAKPOINT event (INT3) the monitor subscriber
> normally has to remove the INT3 from memory - singlestep - place back INT3
> to allow the guest to continue execution. This routine however is susceptible
> to a race-condition on multi-vCPU guests. By allowing the subscriber to return
> the i-cache to be used for emulation it can side-step the problem by returning
> a clean buffer without the INT3 present.
>
> As part of this patch we rename hvm_mem_access_emulate_one to
> hvm_emulate_one_vm_event to better reflect that it is used in various vm_event
> scenarios now, not just in response to mem_access events.
>
> Signed-off-by: Tamas K Lengyel <tamas.lengyel@zentific.com>
> ---
> Cc: Paul Durrant <paul.durrant@citrix.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: Jun Nakajima <jun.nakajima@intel.com>
> Cc: Kevin Tian <kevin.tian@intel.com>
> Cc: George Dunlap <george.dunlap@eu.citrix.com>
> Cc: Razvan Cojocaru <rcojocaru@bitdefender.com>
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Julien Grall <julien.grall@arm.com>
>
> v2: rework hvm_mem_access_emulate_one switch statement
> add BUILD_BUG_ON to ensure internal and vm_event buffer sizes match
>
> Note: this patch has now been fully tested and works as intended
Acked-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
On a side note, I see that you're using an email address that's
different from the one in MAINTAINERS. Should we update the MAINTAINERS
file?
Thanks,
Razvan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 2/2] x86/vm_event: Allow overwriting Xen's i-cache used for emulation
2016-09-17 16:40 ` Razvan Cojocaru
@ 2016-09-17 16:54 ` Tamas K Lengyel
0 siblings, 0 replies; 19+ messages in thread
From: Tamas K Lengyel @ 2016-09-17 16:54 UTC (permalink / raw)
To: Razvan Cojocaru
Cc: Kevin Tian, Stefano Stabellini, Jan Beulich, George Dunlap,
Andrew Cooper, Julien Grall, Paul Durrant, Jun Nakajima,
xen-devel
On Sat, Sep 17, 2016 at 10:40 AM, Razvan Cojocaru
<rcojocaru@bitdefender.com> wrote:
> On 09/15/16 19:51, Tamas K Lengyel wrote:
>> When emulating instructions Xen's emulator maintains a small i-cache fetched
>> from the guest memory. This patch extends the vm_event interface to allow
>> overwriting this i-cache via a buffer returned in the vm_event response.
>>
>> When responding to a SOFTWARE_BREAKPOINT event (INT3) the monitor subscriber
>> normally has to remove the INT3 from memory - singlestep - place back INT3
>> to allow the guest to continue execution. This routine however is susceptible
>> to a race-condition on multi-vCPU guests. By allowing the subscriber to return
>> the i-cache to be used for emulation it can side-step the problem by returning
>> a clean buffer without the INT3 present.
>>
>> As part of this patch we rename hvm_mem_access_emulate_one to
>> hvm_emulate_one_vm_event to better reflect that it is used in various vm_event
>> scenarios now, not just in response to mem_access events.
>>
>> Signed-off-by: Tamas K Lengyel <tamas.lengyel@zentific.com>
>> ---
>> Cc: Paul Durrant <paul.durrant@citrix.com>
>> Cc: Jan Beulich <jbeulich@suse.com>
>> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
>> Cc: Jun Nakajima <jun.nakajima@intel.com>
>> Cc: Kevin Tian <kevin.tian@intel.com>
>> Cc: George Dunlap <george.dunlap@eu.citrix.com>
>> Cc: Razvan Cojocaru <rcojocaru@bitdefender.com>
>> Cc: Stefano Stabellini <sstabellini@kernel.org>
>> Cc: Julien Grall <julien.grall@arm.com>
>>
>> v2: rework hvm_mem_access_emulate_one switch statement
>> add BUILD_BUG_ON to ensure internal and vm_event buffer sizes match
>>
>> Note: this patch has now been fully tested and works as intended
>
> Acked-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
>
> On a side note, I see that you're using an email address that's
> different from the one in MAINTAINERS. Should we update the MAINTAINERS
> file?
It's fine for now (both go to the same place at the end anyway).
Tamas
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 2/2] x86/vm_event: Allow overwriting Xen's i-cache used for emulation
2016-09-15 16:51 ` [PATCH v2 2/2] x86/vm_event: Allow overwriting Xen's i-cache used for emulation Tamas K Lengyel
2016-09-17 16:40 ` Razvan Cojocaru
@ 2016-09-19 8:19 ` Jan Beulich
2016-09-19 16:40 ` Tamas K Lengyel
2016-09-19 18:27 ` Tamas K Lengyel
1 sibling, 2 replies; 19+ messages in thread
From: Jan Beulich @ 2016-09-19 8:19 UTC (permalink / raw)
To: Tamas K Lengyel
Cc: Kevin Tian, Stefano Stabellini, Razvan Cojocaru, George Dunlap,
Andrew Cooper, Julien Grall, Paul Durrant, Jun Nakajima,
xen-devel
>>> On 15.09.16 at 18:51, <tamas.lengyel@zentific.com> wrote:
> @@ -1793,7 +1793,17 @@ static int _hvm_emulate_one(struct hvm_emulate_ctxt *hvmemul_ctxt,
> pfec |= PFEC_user_mode;
>
> hvmemul_ctxt->insn_buf_eip = regs->eip;
> - if ( !vio->mmio_insn_bytes )
> +
> + if ( unlikely(hvmemul_ctxt->set_context_insn) && curr->arch.vm_event )
> + {
> + BUILD_BUG_ON(sizeof(hvmemul_ctxt->insn_buf_bytes) ==
> + sizeof(curr->arch.vm_event->emul.insn));
This should quite clearly be !=, and I think it builds only because you
use the wrong operand in the first sizeof().
> + hvmemul_ctxt->insn_buf_bytes = sizeof(curr->arch.vm_event->emul.insn);
> + memcpy(hvmemul_ctxt->insn_buf, &curr->arch.vm_event->emul.insn,
> + hvmemul_ctxt->insn_buf_bytes);
This memcpy()s between dissimilar types. Please omit the & and
properly add .data on the second argument (and this .data
addition should then also be mirrored in the BUILD_BUG_ON()).
> + }
> + else if ( !vio->mmio_insn_bytes )
And then - I'm sorry for not having thought of this before - I think
this would better not live here, or have an effect more explicitly
only when coming here through hvm_emulate_one_vm_event().
Since the former seems impractical, I think giving _hvm_emulate_one()
one or two extra parameters would be the most straightforward
approach.
> @@ -1944,11 +1954,11 @@ void hvm_mem_access_emulate_one(enum emul_kind kind, unsigned int trapnr,
> case EMUL_KIND_NOWRITE:
> rc = hvm_emulate_one_no_write(&ctx);
> break;
> - case EMUL_KIND_SET_CONTEXT:
> - ctx.set_context = 1;
> - /* Intentional fall-through. */
> default:
> + ctx.set_context_data = (kind == EMUL_KIND_SET_CONTEXT_DATA);
> + ctx.set_context_insn = (kind == EMUL_KIND_SET_CONTEXT_INSN);
> rc = hvm_emulate_one(&ctx);
> + break;
> }
Thanks - this is a lot better readable now!
> @@ -1983,7 +1993,8 @@ void hvm_emulate_prepare(
> hvmemul_ctxt->ctxt.force_writeback = 1;
> hvmemul_ctxt->seg_reg_accessed = 0;
> hvmemul_ctxt->seg_reg_dirty = 0;
> - hvmemul_ctxt->set_context = 0;
> + hvmemul_ctxt->set_context_data = 0;
> + hvmemul_ctxt->set_context_insn = 0;
I think you can almost guess the comment here and on the declaration
of these fields: Please use false here and plain bool there.
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -489,13 +489,16 @@ void hvm_do_resume(struct vcpu *v)
>
> if ( v->arch.vm_event->emulate_flags &
> VM_EVENT_FLAG_SET_EMUL_READ_DATA )
> - kind = EMUL_KIND_SET_CONTEXT;
> + kind = EMUL_KIND_SET_CONTEXT_DATA;
> else if ( v->arch.vm_event->emulate_flags &
> VM_EVENT_FLAG_EMULATE_NOWRITE )
> kind = EMUL_KIND_NOWRITE;
> + else if ( v->arch.vm_event->emulate_flags &
> + VM_EVENT_FLAG_SET_EMUL_INSN_DATA )
> + kind = EMUL_KIND_SET_CONTEXT_INSN;
The header talking about incompatibilities between these flags -
wouldn't it be a good idea to actually -EINVAL such combinations?
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -57,6 +57,7 @@
> #include <asm/altp2m.h>
> #include <asm/event.h>
> #include <asm/monitor.h>
> +#include <asm/vm_event.h>
> #include <public/arch-x86/cpuid.h>
I have to admit that looking through the header changes you do I
can't figure why this adjustment is necessary.
> --- a/xen/arch/x86/vm_event.c
> +++ b/xen/arch/x86/vm_event.c
> @@ -209,11 +209,18 @@ void vm_event_emulate_check(struct vcpu *v, vm_event_response_t *rsp)
> if ( p2m_mem_access_emulate_check(v, rsp) )
> {
> if ( rsp->flags & VM_EVENT_FLAG_SET_EMUL_READ_DATA )
> - v->arch.vm_event->emul_read_data = rsp->data.emul_read_data;
> + v->arch.vm_event->emul.read = rsp->data.emul.read;
>
> v->arch.vm_event->emulate_flags = rsp->flags;
> }
> break;
> + case VM_EVENT_REASON_SOFTWARE_BREAKPOINT:
> + if ( rsp->flags & VM_EVENT_FLAG_SET_EMUL_INSN_DATA )
> + {
> + v->arch.vm_event->emul.insn = rsp->data.emul.insn;
> + v->arch.vm_event->emulate_flags = rsp->flags;
> + }
> + break;
> default:
Blank lines between non-fall-through case statements please (part
of me thinks I've said so before).
> --- a/xen/common/vm_event.c
> +++ b/xen/common/vm_event.c
> @@ -398,7 +398,6 @@ void vm_event_resume(struct domain *d, struct vm_event_domain *ved)
> * In some cases the response type needs extra handling, so here
> * we call the appropriate handlers.
> */
> -
> /* Check flags which apply only when the vCPU is paused */
Stray change.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 2/2] x86/vm_event: Allow overwriting Xen's i-cache used for emulation
2016-09-19 8:19 ` Jan Beulich
@ 2016-09-19 16:40 ` Tamas K Lengyel
2016-09-20 0:36 ` Tian, Kevin
2016-09-19 18:27 ` Tamas K Lengyel
1 sibling, 1 reply; 19+ messages in thread
From: Tamas K Lengyel @ 2016-09-19 16:40 UTC (permalink / raw)
To: Jan Beulich
Cc: Kevin Tian, Stefano Stabellini, Razvan Cojocaru, George Dunlap,
Andrew Cooper, Julien Grall, Paul Durrant, Jun Nakajima,
xen-devel
On Mon, Sep 19, 2016 at 2:19 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 15.09.16 at 18:51, <tamas.lengyel@zentific.com> wrote:
>> @@ -1793,7 +1793,17 @@ static int _hvm_emulate_one(struct hvm_emulate_ctxt *hvmemul_ctxt,
>> pfec |= PFEC_user_mode;
>>
>> hvmemul_ctxt->insn_buf_eip = regs->eip;
>> - if ( !vio->mmio_insn_bytes )
>> +
>> + if ( unlikely(hvmemul_ctxt->set_context_insn) && curr->arch.vm_event )
>> + {
>> + BUILD_BUG_ON(sizeof(hvmemul_ctxt->insn_buf_bytes) ==
>> + sizeof(curr->arch.vm_event->emul.insn));
>
> This should quite clearly be !=, and I think it builds only because you
> use the wrong operand in the first sizeof().
Yea.. I don't know what I was thinking =)
>
>> + hvmemul_ctxt->insn_buf_bytes = sizeof(curr->arch.vm_event->emul.insn);
>> + memcpy(hvmemul_ctxt->insn_buf, &curr->arch.vm_event->emul.insn,
>> + hvmemul_ctxt->insn_buf_bytes);
>
> This memcpy()s between dissimilar types. Please omit the & and
> properly add .data on the second argument (and this .data
> addition should then also be mirrored in the BUILD_BUG_ON()).
Ack.
>
>> + }
>> + else if ( !vio->mmio_insn_bytes )
>
> And then - I'm sorry for not having thought of this before - I think
> this would better not live here, or have an effect more explicitly
> only when coming here through hvm_emulate_one_vm_event().
> Since the former seems impractical, I think giving _hvm_emulate_one()
> one or two extra parameters would be the most straightforward
> approach.
>
>> @@ -1944,11 +1954,11 @@ void hvm_mem_access_emulate_one(enum emul_kind kind, unsigned int trapnr,
>> case EMUL_KIND_NOWRITE:
>> rc = hvm_emulate_one_no_write(&ctx);
>> break;
>> - case EMUL_KIND_SET_CONTEXT:
>> - ctx.set_context = 1;
>> - /* Intentional fall-through. */
>> default:
>> + ctx.set_context_data = (kind == EMUL_KIND_SET_CONTEXT_DATA);
>> + ctx.set_context_insn = (kind == EMUL_KIND_SET_CONTEXT_INSN);
>> rc = hvm_emulate_one(&ctx);
>> + break;
>> }
>
> Thanks - this is a lot better readable now!
>
>> @@ -1983,7 +1993,8 @@ void hvm_emulate_prepare(
>> hvmemul_ctxt->ctxt.force_writeback = 1;
>> hvmemul_ctxt->seg_reg_accessed = 0;
>> hvmemul_ctxt->seg_reg_dirty = 0;
>> - hvmemul_ctxt->set_context = 0;
>> + hvmemul_ctxt->set_context_data = 0;
>> + hvmemul_ctxt->set_context_insn = 0;
>
> I think you can almost guess the comment here and on the declaration
> of these fields: Please use false here and plain bool there.
>
>> --- a/xen/arch/x86/hvm/hvm.c
>> +++ b/xen/arch/x86/hvm/hvm.c
>> @@ -489,13 +489,16 @@ void hvm_do_resume(struct vcpu *v)
>>
>> if ( v->arch.vm_event->emulate_flags &
>> VM_EVENT_FLAG_SET_EMUL_READ_DATA )
>> - kind = EMUL_KIND_SET_CONTEXT;
>> + kind = EMUL_KIND_SET_CONTEXT_DATA;
>> else if ( v->arch.vm_event->emulate_flags &
>> VM_EVENT_FLAG_EMULATE_NOWRITE )
>> kind = EMUL_KIND_NOWRITE;
>> + else if ( v->arch.vm_event->emulate_flags &
>> + VM_EVENT_FLAG_SET_EMUL_INSN_DATA )
>> + kind = EMUL_KIND_SET_CONTEXT_INSN;
>
> The header talking about incompatibilities between these flags -
> wouldn't it be a good idea to actually -EINVAL such combinations?
The header just points out that setting all these flags at the same
time won't have a "combined" effect - evident from the if-else
treatment above. There is really no point to do an error, the error
would never reach the user. Setting response flags through vm_event
doesn't have an error-path back.
>> --- a/xen/arch/x86/hvm/vmx/vmx.c
>> +++ b/xen/arch/x86/hvm/vmx/vmx.c
>> @@ -57,6 +57,7 @@
>> #include <asm/altp2m.h>
>> #include <asm/event.h>
>> #include <asm/monitor.h>
>> +#include <asm/vm_event.h>
>> #include <public/arch-x86/cpuid.h>
>
> I have to admit that looking through the header changes you do I
> can't figure why this adjustment is necessary.
Yea, it's just a residue (the patch was first made when this header
was still included).
>
>> --- a/xen/arch/x86/vm_event.c
>> +++ b/xen/arch/x86/vm_event.c
>> @@ -209,11 +209,18 @@ void vm_event_emulate_check(struct vcpu *v, vm_event_response_t *rsp)
>> if ( p2m_mem_access_emulate_check(v, rsp) )
>> {
>> if ( rsp->flags & VM_EVENT_FLAG_SET_EMUL_READ_DATA )
>> - v->arch.vm_event->emul_read_data = rsp->data.emul_read_data;
>> + v->arch.vm_event->emul.read = rsp->data.emul.read;
>>
>> v->arch.vm_event->emulate_flags = rsp->flags;
>> }
>> break;
>> + case VM_EVENT_REASON_SOFTWARE_BREAKPOINT:
>> + if ( rsp->flags & VM_EVENT_FLAG_SET_EMUL_INSN_DATA )
>> + {
>> + v->arch.vm_event->emul.insn = rsp->data.emul.insn;
>> + v->arch.vm_event->emulate_flags = rsp->flags;
>> + }
>> + break;
>> default:
>
> Blank lines between non-fall-through case statements please (part
> of me thinks I've said so before).
Ack.
>
>> --- a/xen/common/vm_event.c
>> +++ b/xen/common/vm_event.c
>> @@ -398,7 +398,6 @@ void vm_event_resume(struct domain *d, struct vm_event_domain *ved)
>> * In some cases the response type needs extra handling, so here
>> * we call the appropriate handlers.
>> */
>> -
>> /* Check flags which apply only when the vCPU is paused */
>
> Stray change.
>
> Jan
Thanks!
Tamas
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 2/2] x86/vm_event: Allow overwriting Xen's i-cache used for emulation
2016-09-19 8:19 ` Jan Beulich
2016-09-19 16:40 ` Tamas K Lengyel
@ 2016-09-19 18:27 ` Tamas K Lengyel
2016-09-20 7:26 ` Jan Beulich
1 sibling, 1 reply; 19+ messages in thread
From: Tamas K Lengyel @ 2016-09-19 18:27 UTC (permalink / raw)
To: Jan Beulich
Cc: Kevin Tian, Stefano Stabellini, Razvan Cojocaru, George Dunlap,
Andrew Cooper, Julien Grall, Paul Durrant, Jun Nakajima,
xen-devel
On Mon, Sep 19, 2016 at 2:19 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 15.09.16 at 18:51, <tamas.lengyel@zentific.com> wrote:
>> @@ -1793,7 +1793,17 @@ static int _hvm_emulate_one(struct hvm_emulate_ctxt *hvmemul_ctxt,
>> pfec |= PFEC_user_mode;
>>
>> hvmemul_ctxt->insn_buf_eip = regs->eip;
>> - if ( !vio->mmio_insn_bytes )
>> +
>> + if ( unlikely(hvmemul_ctxt->set_context_insn) && curr->arch.vm_event )
>> + {
>> + BUILD_BUG_ON(sizeof(hvmemul_ctxt->insn_buf_bytes) ==
>> + sizeof(curr->arch.vm_event->emul.insn));
>
> This should quite clearly be !=, and I think it builds only because you
> use the wrong operand in the first sizeof().
>
>> + hvmemul_ctxt->insn_buf_bytes = sizeof(curr->arch.vm_event->emul.insn);
>> + memcpy(hvmemul_ctxt->insn_buf, &curr->arch.vm_event->emul.insn,
>> + hvmemul_ctxt->insn_buf_bytes);
>
> This memcpy()s between dissimilar types. Please omit the & and
> properly add .data on the second argument (and this .data
> addition should then also be mirrored in the BUILD_BUG_ON()).
>
>> + }
>> + else if ( !vio->mmio_insn_bytes )
>
> And then - I'm sorry for not having thought of this before - I think
> this would better not live here, or have an effect more explicitly
> only when coming here through hvm_emulate_one_vm_event().
> Since the former seems impractical, I think giving _hvm_emulate_one()
> one or two extra parameters would be the most straightforward
> approach.
So this is the spot where the mmio insn buffer is getting copied as
well instead of fetching the instructions from the guest memory. So
having the vm_event buffer getting copied here too makes the most
sense. Having the vm_event insn buffer getting copied in somewhere
else, while the mmio insn buffer getting copied here, IMHO just
fragments the flow even more making it harder to see what is actually
happening. How about adjusting the if-else here to be:
if ( !vio->mmio_insn_bytes && !hvmemul_ctxt->set_context_insn )
...
else if ( vio->mmio_insn_bytes )
...
else if ( unlikely(hvmemul_ctxt->set_context_insn) && curr->arch.vm_event )
?
Tamas
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 2/2] x86/vm_event: Allow overwriting Xen's i-cache used for emulation
2016-09-19 16:40 ` Tamas K Lengyel
@ 2016-09-20 0:36 ` Tian, Kevin
2016-09-20 15:32 ` Tamas K Lengyel
0 siblings, 1 reply; 19+ messages in thread
From: Tian, Kevin @ 2016-09-20 0:36 UTC (permalink / raw)
To: Tamas K Lengyel, Jan Beulich
Cc: Stefano Stabellini, Razvan Cojocaru, George Dunlap,
Andrew Cooper, Julien Grall, Paul Durrant, Nakajima, Jun,
xen-devel
> From: Tamas K Lengyel
> Sent: Tuesday, September 20, 2016 12:40 AM
>
> >> --- a/xen/arch/x86/hvm/hvm.c
> >> +++ b/xen/arch/x86/hvm/hvm.c
> >> @@ -489,13 +489,16 @@ void hvm_do_resume(struct vcpu *v)
> >>
> >> if ( v->arch.vm_event->emulate_flags &
> >> VM_EVENT_FLAG_SET_EMUL_READ_DATA )
> >> - kind = EMUL_KIND_SET_CONTEXT;
> >> + kind = EMUL_KIND_SET_CONTEXT_DATA;
> >> else if ( v->arch.vm_event->emulate_flags &
> >> VM_EVENT_FLAG_EMULATE_NOWRITE )
> >> kind = EMUL_KIND_NOWRITE;
> >> + else if ( v->arch.vm_event->emulate_flags &
> >> + VM_EVENT_FLAG_SET_EMUL_INSN_DATA )
> >> + kind = EMUL_KIND_SET_CONTEXT_INSN;
> >
> > The header talking about incompatibilities between these flags -
> > wouldn't it be a good idea to actually -EINVAL such combinations?
>
> The header just points out that setting all these flags at the same
> time won't have a "combined" effect - evident from the if-else
> treatment above. There is really no point to do an error, the error
> would never reach the user. Setting response flags through vm_event
> doesn't have an error-path back.
>
Maybe you can have an explicit comment on priority of those flags,
so consistent behavior can be expected moving forward. e.g. above
code implies read_data>nowrite>insn_data. w/o a proper guidance
here, future code changes by others may break that sequence...
Thanks
Kevin
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 2/2] x86/vm_event: Allow overwriting Xen's i-cache used for emulation
2016-09-19 18:27 ` Tamas K Lengyel
@ 2016-09-20 7:26 ` Jan Beulich
2016-09-20 14:56 ` Tamas K Lengyel
0 siblings, 1 reply; 19+ messages in thread
From: Jan Beulich @ 2016-09-20 7:26 UTC (permalink / raw)
To: Tamas K Lengyel
Cc: Kevin Tian, Stefano Stabellini, Razvan Cojocaru, George Dunlap,
Andrew Cooper, Julien Grall, Paul Durrant, Jun Nakajima,
xen-devel
>>> On 19.09.16 at 20:27, <tamas.lengyel@zentific.com> wrote:
> On Mon, Sep 19, 2016 at 2:19 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>> On 15.09.16 at 18:51, <tamas.lengyel@zentific.com> wrote:
>>> @@ -1793,7 +1793,17 @@ static int _hvm_emulate_one(struct hvm_emulate_ctxt
> *hvmemul_ctxt,
>>> pfec |= PFEC_user_mode;
>>>
>>> hvmemul_ctxt->insn_buf_eip = regs->eip;
>>> - if ( !vio->mmio_insn_bytes )
>>> +
>>> + if ( unlikely(hvmemul_ctxt->set_context_insn) && curr->arch.vm_event )
>>> + {
>>> + BUILD_BUG_ON(sizeof(hvmemul_ctxt->insn_buf_bytes) ==
>>> + sizeof(curr->arch.vm_event->emul.insn));
>>
>> This should quite clearly be !=, and I think it builds only because you
>> use the wrong operand in the first sizeof().
>>
>>> + hvmemul_ctxt->insn_buf_bytes = sizeof(curr->arch.vm_event->emul.insn);
>>> + memcpy(hvmemul_ctxt->insn_buf, &curr->arch.vm_event->emul.insn,
>>> + hvmemul_ctxt->insn_buf_bytes);
>>
>> This memcpy()s between dissimilar types. Please omit the & and
>> properly add .data on the second argument (and this .data
>> addition should then also be mirrored in the BUILD_BUG_ON()).
>>
>>> + }
>>> + else if ( !vio->mmio_insn_bytes )
>>
>> And then - I'm sorry for not having thought of this before - I think
>> this would better not live here, or have an effect more explicitly
>> only when coming here through hvm_emulate_one_vm_event().
>> Since the former seems impractical, I think giving _hvm_emulate_one()
>> one or two extra parameters would be the most straightforward
>> approach.
>
> So this is the spot where the mmio insn buffer is getting copied as
> well instead of fetching the instructions from the guest memory. So
> having the vm_event buffer getting copied here too makes the most
> sense. Having the vm_event insn buffer getting copied in somewhere
> else, while the mmio insn buffer getting copied here, IMHO just
> fragments the flow even more making it harder to see what is actually
> happening.
And I didn't unconditionally ask to move the copying elsewhere.
The alternative - passing the override in as function argument(s),
which would then be NULL/zero for all cases except the VM event
one, would be as suitable. It is in particular ...
> How about adjusting the if-else here to be:
>
> if ( !vio->mmio_insn_bytes && !hvmemul_ctxt->set_context_insn )
> ...
> else if ( vio->mmio_insn_bytes )
> ...
> else if ( unlikely(hvmemul_ctxt->set_context_insn) && curr->arch.vm_event )
... this curr->arch.vm_event reference which I'd like to see gone
from this specific code path. The ordering in your original patch,
otoh, would then be fine (check for the override first with unlikely(),
else do what is being done today). Such a code structure would
then also ease a possible second way of overriding the insn by
some other party, without having to touch the code here again.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 2/2] x86/vm_event: Allow overwriting Xen's i-cache used for emulation
2016-09-20 7:26 ` Jan Beulich
@ 2016-09-20 14:56 ` Tamas K Lengyel
2016-09-20 15:09 ` Razvan Cojocaru
2016-09-20 15:12 ` Jan Beulich
0 siblings, 2 replies; 19+ messages in thread
From: Tamas K Lengyel @ 2016-09-20 14:56 UTC (permalink / raw)
To: Jan Beulich
Cc: Kevin Tian, Stefano Stabellini, Razvan Cojocaru, George Dunlap,
Andrew Cooper, Julien Grall, Paul Durrant, Jun Nakajima,
xen-devel
On Tue, Sep 20, 2016 at 1:26 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 19.09.16 at 20:27, <tamas.lengyel@zentific.com> wrote:
>> On Mon, Sep 19, 2016 at 2:19 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>> On 15.09.16 at 18:51, <tamas.lengyel@zentific.com> wrote:
>>>> @@ -1793,7 +1793,17 @@ static int _hvm_emulate_one(struct hvm_emulate_ctxt
>> *hvmemul_ctxt,
>>>> pfec |= PFEC_user_mode;
>>>>
>>>> hvmemul_ctxt->insn_buf_eip = regs->eip;
>>>> - if ( !vio->mmio_insn_bytes )
>>>> +
>>>> + if ( unlikely(hvmemul_ctxt->set_context_insn) && curr->arch.vm_event )
>>>> + {
>>>> + BUILD_BUG_ON(sizeof(hvmemul_ctxt->insn_buf_bytes) ==
>>>> + sizeof(curr->arch.vm_event->emul.insn));
>>>
>>> This should quite clearly be !=, and I think it builds only because you
>>> use the wrong operand in the first sizeof().
>>>
>>>> + hvmemul_ctxt->insn_buf_bytes = sizeof(curr->arch.vm_event->emul.insn);
>>>> + memcpy(hvmemul_ctxt->insn_buf, &curr->arch.vm_event->emul.insn,
>>>> + hvmemul_ctxt->insn_buf_bytes);
>>>
>>> This memcpy()s between dissimilar types. Please omit the & and
>>> properly add .data on the second argument (and this .data
>>> addition should then also be mirrored in the BUILD_BUG_ON()).
>>>
>>>> + }
>>>> + else if ( !vio->mmio_insn_bytes )
>>>
>>> And then - I'm sorry for not having thought of this before - I think
>>> this would better not live here, or have an effect more explicitly
>>> only when coming here through hvm_emulate_one_vm_event().
>>> Since the former seems impractical, I think giving _hvm_emulate_one()
>>> one or two extra parameters would be the most straightforward
>>> approach.
>>
>> So this is the spot where the mmio insn buffer is getting copied as
>> well instead of fetching the instructions from the guest memory. So
>> having the vm_event buffer getting copied here too makes the most
>> sense. Having the vm_event insn buffer getting copied in somewhere
>> else, while the mmio insn buffer getting copied here, IMHO just
>> fragments the flow even more making it harder to see what is actually
>> happening.
>
> And I didn't unconditionally ask to move the copying elsewhere.
> The alternative - passing the override in as function argument(s),
> which would then be NULL/zero for all cases except the VM event
> one, would be as suitable. It is in particular ...
>
>> How about adjusting the if-else here to be:
>>
>> if ( !vio->mmio_insn_bytes && !hvmemul_ctxt->set_context_insn )
>> ...
>> else if ( vio->mmio_insn_bytes )
>> ...
>> else if ( unlikely(hvmemul_ctxt->set_context_insn) && curr->arch.vm_event )
>
> ... this curr->arch.vm_event reference which I'd like to see gone
> from this specific code path. The ordering in your original patch,
> otoh, would then be fine (check for the override first with unlikely(),
> else do what is being done today). Such a code structure would
> then also ease a possible second way of overriding the insn by
> some other party, without having to touch the code here again.
>
So that check is one that Razvan asked to be added. I think it is
necessary too as there seems to be a race-condition if vm_event gets
shutdown after the response flag is set but before this emulation path
takes place. Effectively set_context_insn may be set but the
arch.vm_event already gotten freed. Razvan, is that correct?
Tamas
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 2/2] x86/vm_event: Allow overwriting Xen's i-cache used for emulation
2016-09-20 14:56 ` Tamas K Lengyel
@ 2016-09-20 15:09 ` Razvan Cojocaru
2016-09-20 15:12 ` Jan Beulich
1 sibling, 0 replies; 19+ messages in thread
From: Razvan Cojocaru @ 2016-09-20 15:09 UTC (permalink / raw)
To: Tamas K Lengyel, Jan Beulich
Cc: Kevin Tian, Stefano Stabellini, George Dunlap, Andrew Cooper,
Julien Grall, Paul Durrant, Jun Nakajima, xen-devel
On 09/20/2016 05:56 PM, Tamas K Lengyel wrote:
> On Tue, Sep 20, 2016 at 1:26 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>> On 19.09.16 at 20:27, <tamas.lengyel@zentific.com> wrote:
>>> On Mon, Sep 19, 2016 at 2:19 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>>> On 15.09.16 at 18:51, <tamas.lengyel@zentific.com> wrote:
>>>>> @@ -1793,7 +1793,17 @@ static int _hvm_emulate_one(struct hvm_emulate_ctxt
>>> *hvmemul_ctxt,
>>>>> pfec |= PFEC_user_mode;
>>>>>
>>>>> hvmemul_ctxt->insn_buf_eip = regs->eip;
>>>>> - if ( !vio->mmio_insn_bytes )
>>>>> +
>>>>> + if ( unlikely(hvmemul_ctxt->set_context_insn) && curr->arch.vm_event )
>>>>> + {
>>>>> + BUILD_BUG_ON(sizeof(hvmemul_ctxt->insn_buf_bytes) ==
>>>>> + sizeof(curr->arch.vm_event->emul.insn));
>>>>
>>>> This should quite clearly be !=, and I think it builds only because you
>>>> use the wrong operand in the first sizeof().
>>>>
>>>>> + hvmemul_ctxt->insn_buf_bytes = sizeof(curr->arch.vm_event->emul.insn);
>>>>> + memcpy(hvmemul_ctxt->insn_buf, &curr->arch.vm_event->emul.insn,
>>>>> + hvmemul_ctxt->insn_buf_bytes);
>>>>
>>>> This memcpy()s between dissimilar types. Please omit the & and
>>>> properly add .data on the second argument (and this .data
>>>> addition should then also be mirrored in the BUILD_BUG_ON()).
>>>>
>>>>> + }
>>>>> + else if ( !vio->mmio_insn_bytes )
>>>>
>>>> And then - I'm sorry for not having thought of this before - I think
>>>> this would better not live here, or have an effect more explicitly
>>>> only when coming here through hvm_emulate_one_vm_event().
>>>> Since the former seems impractical, I think giving _hvm_emulate_one()
>>>> one or two extra parameters would be the most straightforward
>>>> approach.
>>>
>>> So this is the spot where the mmio insn buffer is getting copied as
>>> well instead of fetching the instructions from the guest memory. So
>>> having the vm_event buffer getting copied here too makes the most
>>> sense. Having the vm_event insn buffer getting copied in somewhere
>>> else, while the mmio insn buffer getting copied here, IMHO just
>>> fragments the flow even more making it harder to see what is actually
>>> happening.
>>
>> And I didn't unconditionally ask to move the copying elsewhere.
>> The alternative - passing the override in as function argument(s),
>> which would then be NULL/zero for all cases except the VM event
>> one, would be as suitable. It is in particular ...
>>
>>> How about adjusting the if-else here to be:
>>>
>>> if ( !vio->mmio_insn_bytes && !hvmemul_ctxt->set_context_insn )
>>> ...
>>> else if ( vio->mmio_insn_bytes )
>>> ...
>>> else if ( unlikely(hvmemul_ctxt->set_context_insn) && curr->arch.vm_event )
>>
>> ... this curr->arch.vm_event reference which I'd like to see gone
>> from this specific code path. The ordering in your original patch,
>> otoh, would then be fine (check for the override first with unlikely(),
>> else do what is being done today). Such a code structure would
>> then also ease a possible second way of overriding the insn by
>> some other party, without having to touch the code here again.
>>
>
> So that check is one that Razvan asked to be added. I think it is
> necessary too as there seems to be a race-condition if vm_event gets
> shutdown after the response flag is set but before this emulation path
> takes place. Effectively set_context_insn may be set but the
> arch.vm_event already gotten freed. Razvan, is that correct?
Yes, that's the general idea, but there's also already a check in
hvm_do_resume() right before calling hvm_mem_access_emulate_one(), so as
long as that's the only code path leading here it should be alright to
remove the check. The check adds extra safety but it's not strictly
necessary here, so if Jan prefers it taken out it should, theoretically,
be fine for now.
Thanks,
Razvan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 2/2] x86/vm_event: Allow overwriting Xen's i-cache used for emulation
2016-09-20 14:56 ` Tamas K Lengyel
2016-09-20 15:09 ` Razvan Cojocaru
@ 2016-09-20 15:12 ` Jan Beulich
2016-09-20 15:14 ` Tamas K Lengyel
1 sibling, 1 reply; 19+ messages in thread
From: Jan Beulich @ 2016-09-20 15:12 UTC (permalink / raw)
To: Tamas K Lengyel
Cc: Kevin Tian, Stefano Stabellini, Razvan Cojocaru, George Dunlap,
Andrew Cooper, Julien Grall, Paul Durrant, Jun Nakajima,
xen-devel
>>> On 20.09.16 at 16:56, <tamas.lengyel@zentific.com> wrote:
> On Tue, Sep 20, 2016 at 1:26 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>> On 19.09.16 at 20:27, <tamas.lengyel@zentific.com> wrote:
>>> On Mon, Sep 19, 2016 at 2:19 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>>> On 15.09.16 at 18:51, <tamas.lengyel@zentific.com> wrote:
>>>>> @@ -1793,7 +1793,17 @@ static int _hvm_emulate_one(struct hvm_emulate_ctxt
>>> *hvmemul_ctxt,
>>>>> pfec |= PFEC_user_mode;
>>>>>
>>>>> hvmemul_ctxt->insn_buf_eip = regs->eip;
>>>>> - if ( !vio->mmio_insn_bytes )
>>>>> +
>>>>> + if ( unlikely(hvmemul_ctxt->set_context_insn) && curr->arch.vm_event )
>>>>> + {
>>>>> + BUILD_BUG_ON(sizeof(hvmemul_ctxt->insn_buf_bytes) ==
>>>>> + sizeof(curr->arch.vm_event->emul.insn));
>>>>
>>>> This should quite clearly be !=, and I think it builds only because you
>>>> use the wrong operand in the first sizeof().
>>>>
>>>>> + hvmemul_ctxt->insn_buf_bytes = sizeof(curr->arch.vm_event->emul.insn);
>>>>> + memcpy(hvmemul_ctxt->insn_buf, &curr->arch.vm_event->emul.insn,
>>>>> + hvmemul_ctxt->insn_buf_bytes);
>>>>
>>>> This memcpy()s between dissimilar types. Please omit the & and
>>>> properly add .data on the second argument (and this .data
>>>> addition should then also be mirrored in the BUILD_BUG_ON()).
>>>>
>>>>> + }
>>>>> + else if ( !vio->mmio_insn_bytes )
>>>>
>>>> And then - I'm sorry for not having thought of this before - I think
>>>> this would better not live here, or have an effect more explicitly
>>>> only when coming here through hvm_emulate_one_vm_event().
>>>> Since the former seems impractical, I think giving _hvm_emulate_one()
>>>> one or two extra parameters would be the most straightforward
>>>> approach.
>>>
>>> So this is the spot where the mmio insn buffer is getting copied as
>>> well instead of fetching the instructions from the guest memory. So
>>> having the vm_event buffer getting copied here too makes the most
>>> sense. Having the vm_event insn buffer getting copied in somewhere
>>> else, while the mmio insn buffer getting copied here, IMHO just
>>> fragments the flow even more making it harder to see what is actually
>>> happening.
>>
>> And I didn't unconditionally ask to move the copying elsewhere.
>> The alternative - passing the override in as function argument(s),
>> which would then be NULL/zero for all cases except the VM event
>> one, would be as suitable. It is in particular ...
>>
>>> How about adjusting the if-else here to be:
>>>
>>> if ( !vio->mmio_insn_bytes && !hvmemul_ctxt->set_context_insn )
>>> ...
>>> else if ( vio->mmio_insn_bytes )
>>> ...
>>> else if ( unlikely(hvmemul_ctxt->set_context_insn) && curr->arch.vm_event )
>>
>> ... this curr->arch.vm_event reference which I'd like to see gone
>> from this specific code path. The ordering in your original patch,
>> otoh, would then be fine (check for the override first with unlikely(),
>> else do what is being done today). Such a code structure would
>> then also ease a possible second way of overriding the insn by
>> some other party, without having to touch the code here again.
>
> So that check is one that Razvan asked to be added. I think it is
> necessary too as there seems to be a race-condition if vm_event gets
> shutdown after the response flag is set but before this emulation path
> takes place. Effectively set_context_insn may be set but the
> arch.vm_event already gotten freed. Razvan, is that correct?
Well, in case you misunderstood: I didn't ask for the check to be
_removed_, but for it to be _moved elsewhere_.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 2/2] x86/vm_event: Allow overwriting Xen's i-cache used for emulation
2016-09-20 15:12 ` Jan Beulich
@ 2016-09-20 15:14 ` Tamas K Lengyel
2016-09-20 15:39 ` Jan Beulich
0 siblings, 1 reply; 19+ messages in thread
From: Tamas K Lengyel @ 2016-09-20 15:14 UTC (permalink / raw)
To: Jan Beulich
Cc: Kevin Tian, Stefano Stabellini, Razvan Cojocaru, George Dunlap,
Andrew Cooper, Julien Grall, Paul Durrant, Jun Nakajima,
xen-devel
On Tue, Sep 20, 2016 at 9:12 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 20.09.16 at 16:56, <tamas.lengyel@zentific.com> wrote:
>> On Tue, Sep 20, 2016 at 1:26 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>> On 19.09.16 at 20:27, <tamas.lengyel@zentific.com> wrote:
>>>> On Mon, Sep 19, 2016 at 2:19 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>>>> On 15.09.16 at 18:51, <tamas.lengyel@zentific.com> wrote:
>>>>>> @@ -1793,7 +1793,17 @@ static int _hvm_emulate_one(struct hvm_emulate_ctxt
>>>> *hvmemul_ctxt,
>>>>>> pfec |= PFEC_user_mode;
>>>>>>
>>>>>> hvmemul_ctxt->insn_buf_eip = regs->eip;
>>>>>> - if ( !vio->mmio_insn_bytes )
>>>>>> +
>>>>>> + if ( unlikely(hvmemul_ctxt->set_context_insn) && curr->arch.vm_event )
>>>>>> + {
>>>>>> + BUILD_BUG_ON(sizeof(hvmemul_ctxt->insn_buf_bytes) ==
>>>>>> + sizeof(curr->arch.vm_event->emul.insn));
>>>>>
>>>>> This should quite clearly be !=, and I think it builds only because you
>>>>> use the wrong operand in the first sizeof().
>>>>>
>>>>>> + hvmemul_ctxt->insn_buf_bytes = sizeof(curr->arch.vm_event->emul.insn);
>>>>>> + memcpy(hvmemul_ctxt->insn_buf, &curr->arch.vm_event->emul.insn,
>>>>>> + hvmemul_ctxt->insn_buf_bytes);
>>>>>
>>>>> This memcpy()s between dissimilar types. Please omit the & and
>>>>> properly add .data on the second argument (and this .data
>>>>> addition should then also be mirrored in the BUILD_BUG_ON()).
>>>>>
>>>>>> + }
>>>>>> + else if ( !vio->mmio_insn_bytes )
>>>>>
>>>>> And then - I'm sorry for not having thought of this before - I think
>>>>> this would better not live here, or have an effect more explicitly
>>>>> only when coming here through hvm_emulate_one_vm_event().
>>>>> Since the former seems impractical, I think giving _hvm_emulate_one()
>>>>> one or two extra parameters would be the most straightforward
>>>>> approach.
>>>>
>>>> So this is the spot where the mmio insn buffer is getting copied as
>>>> well instead of fetching the instructions from the guest memory. So
>>>> having the vm_event buffer getting copied here too makes the most
>>>> sense. Having the vm_event insn buffer getting copied in somewhere
>>>> else, while the mmio insn buffer getting copied here, IMHO just
>>>> fragments the flow even more making it harder to see what is actually
>>>> happening.
>>>
>>> And I didn't unconditionally ask to move the copying elsewhere.
>>> The alternative - passing the override in as function argument(s),
>>> which would then be NULL/zero for all cases except the VM event
>>> one, would be as suitable. It is in particular ...
>>>
>>>> How about adjusting the if-else here to be:
>>>>
>>>> if ( !vio->mmio_insn_bytes && !hvmemul_ctxt->set_context_insn )
>>>> ...
>>>> else if ( vio->mmio_insn_bytes )
>>>> ...
>>>> else if ( unlikely(hvmemul_ctxt->set_context_insn) && curr->arch.vm_event )
>>>
>>> ... this curr->arch.vm_event reference which I'd like to see gone
>>> from this specific code path. The ordering in your original patch,
>>> otoh, would then be fine (check for the override first with unlikely(),
>>> else do what is being done today). Such a code structure would
>>> then also ease a possible second way of overriding the insn by
>>> some other party, without having to touch the code here again.
>>
>> So that check is one that Razvan asked to be added. I think it is
>> necessary too as there seems to be a race-condition if vm_event gets
>> shutdown after the response flag is set but before this emulation path
>> takes place. Effectively set_context_insn may be set but the
>> arch.vm_event already gotten freed. Razvan, is that correct?
>
> Well, in case you misunderstood: I didn't ask for the check to be
> _removed_, but for it to be _moved elsewhere_.
>
So as Razvan pointed out, there is a check already in hvm_do_resume
for exactly the same effect, so then what you are asking for is
already done.
Tamas
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 2/2] x86/vm_event: Allow overwriting Xen's i-cache used for emulation
2016-09-20 0:36 ` Tian, Kevin
@ 2016-09-20 15:32 ` Tamas K Lengyel
0 siblings, 0 replies; 19+ messages in thread
From: Tamas K Lengyel @ 2016-09-20 15:32 UTC (permalink / raw)
To: Tian, Kevin
Cc: Stefano Stabellini, Nakajima, Jun, Razvan Cojocaru,
George Dunlap, Andrew Cooper, Julien Grall, Paul Durrant,
Jan Beulich, xen-devel
On Mon, Sep 19, 2016 at 6:36 PM, Tian, Kevin <kevin.tian@intel.com> wrote:
>> From: Tamas K Lengyel
>> Sent: Tuesday, September 20, 2016 12:40 AM
>>
>> >> --- a/xen/arch/x86/hvm/hvm.c
>> >> +++ b/xen/arch/x86/hvm/hvm.c
>> >> @@ -489,13 +489,16 @@ void hvm_do_resume(struct vcpu *v)
>> >>
>> >> if ( v->arch.vm_event->emulate_flags &
>> >> VM_EVENT_FLAG_SET_EMUL_READ_DATA )
>> >> - kind = EMUL_KIND_SET_CONTEXT;
>> >> + kind = EMUL_KIND_SET_CONTEXT_DATA;
>> >> else if ( v->arch.vm_event->emulate_flags &
>> >> VM_EVENT_FLAG_EMULATE_NOWRITE )
>> >> kind = EMUL_KIND_NOWRITE;
>> >> + else if ( v->arch.vm_event->emulate_flags &
>> >> + VM_EVENT_FLAG_SET_EMUL_INSN_DATA )
>> >> + kind = EMUL_KIND_SET_CONTEXT_INSN;
>> >
>> > The header talking about incompatibilities between these flags -
>> > wouldn't it be a good idea to actually -EINVAL such combinations?
>>
>> The header just points out that setting all these flags at the same
>> time won't have a "combined" effect - evident from the if-else
>> treatment above. There is really no point to do an error, the error
>> would never reach the user. Setting response flags through vm_event
>> doesn't have an error-path back.
>>
>
> Maybe you can have an explicit comment on priority of those flags,
> so consistent behavior can be expected moving forward. e.g. above
> code implies read_data>nowrite>insn_data. w/o a proper guidance
> here, future code changes by others may break that sequence...
>
Fair point, will do that.
Thanks,
Tamas
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 2/2] x86/vm_event: Allow overwriting Xen's i-cache used for emulation
2016-09-20 15:14 ` Tamas K Lengyel
@ 2016-09-20 15:39 ` Jan Beulich
2016-09-20 15:54 ` Tamas K Lengyel
0 siblings, 1 reply; 19+ messages in thread
From: Jan Beulich @ 2016-09-20 15:39 UTC (permalink / raw)
To: Tamas K Lengyel
Cc: Kevin Tian, Stefano Stabellini, Razvan Cojocaru, George Dunlap,
Andrew Cooper, Julien Grall, Paul Durrant, Jun Nakajima,
xen-devel
>>> On 20.09.16 at 17:14, <tamas.lengyel@zentific.com> wrote:
> On Tue, Sep 20, 2016 at 9:12 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>> On 20.09.16 at 16:56, <tamas.lengyel@zentific.com> wrote:
>>> On Tue, Sep 20, 2016 at 1:26 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>>> On 19.09.16 at 20:27, <tamas.lengyel@zentific.com> wrote:
>>>>> On Mon, Sep 19, 2016 at 2:19 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>>>>> On 15.09.16 at 18:51, <tamas.lengyel@zentific.com> wrote:
>>>>>>> @@ -1793,7 +1793,17 @@ static int _hvm_emulate_one(struct hvm_emulate_ctxt
>>>>> *hvmemul_ctxt,
>>>>>>> pfec |= PFEC_user_mode;
>>>>>>>
>>>>>>> hvmemul_ctxt->insn_buf_eip = regs->eip;
>>>>>>> - if ( !vio->mmio_insn_bytes )
>>>>>>> +
>>>>>>> + if ( unlikely(hvmemul_ctxt->set_context_insn) && curr->arch.vm_event )
>>>>>>> + {
>>>>>>> + BUILD_BUG_ON(sizeof(hvmemul_ctxt->insn_buf_bytes) ==
>>>>>>> + sizeof(curr->arch.vm_event->emul.insn));
>>>>>>
>>>>>> This should quite clearly be !=, and I think it builds only because you
>>>>>> use the wrong operand in the first sizeof().
>>>>>>
>>>>>>> + hvmemul_ctxt->insn_buf_bytes = sizeof(curr->arch.vm_event->emul.insn);
>>>>>>> + memcpy(hvmemul_ctxt->insn_buf, &curr->arch.vm_event->emul.insn,
>>>>>>> + hvmemul_ctxt->insn_buf_bytes);
>>>>>>
>>>>>> This memcpy()s between dissimilar types. Please omit the & and
>>>>>> properly add .data on the second argument (and this .data
>>>>>> addition should then also be mirrored in the BUILD_BUG_ON()).
>>>>>>
>>>>>>> + }
>>>>>>> + else if ( !vio->mmio_insn_bytes )
>>>>>>
>>>>>> And then - I'm sorry for not having thought of this before - I think
>>>>>> this would better not live here, or have an effect more explicitly
>>>>>> only when coming here through hvm_emulate_one_vm_event().
>>>>>> Since the former seems impractical, I think giving _hvm_emulate_one()
>>>>>> one or two extra parameters would be the most straightforward
>>>>>> approach.
>>>>>
>>>>> So this is the spot where the mmio insn buffer is getting copied as
>>>>> well instead of fetching the instructions from the guest memory. So
>>>>> having the vm_event buffer getting copied here too makes the most
>>>>> sense. Having the vm_event insn buffer getting copied in somewhere
>>>>> else, while the mmio insn buffer getting copied here, IMHO just
>>>>> fragments the flow even more making it harder to see what is actually
>>>>> happening.
>>>>
>>>> And I didn't unconditionally ask to move the copying elsewhere.
>>>> The alternative - passing the override in as function argument(s),
>>>> which would then be NULL/zero for all cases except the VM event
>>>> one, would be as suitable. It is in particular ...
>>>>
>>>>> How about adjusting the if-else here to be:
>>>>>
>>>>> if ( !vio->mmio_insn_bytes && !hvmemul_ctxt->set_context_insn )
>>>>> ...
>>>>> else if ( vio->mmio_insn_bytes )
>>>>> ...
>>>>> else if ( unlikely(hvmemul_ctxt->set_context_insn) && curr->arch.vm_event )
>>>>
>>>> ... this curr->arch.vm_event reference which I'd like to see gone
>>>> from this specific code path. The ordering in your original patch,
>>>> otoh, would then be fine (check for the override first with unlikely(),
>>>> else do what is being done today). Such a code structure would
>>>> then also ease a possible second way of overriding the insn by
>>>> some other party, without having to touch the code here again.
>>>
>>> So that check is one that Razvan asked to be added. I think it is
>>> necessary too as there seems to be a race-condition if vm_event gets
>>> shutdown after the response flag is set but before this emulation path
>>> takes place. Effectively set_context_insn may be set but the
>>> arch.vm_event already gotten freed. Razvan, is that correct?
>>
>> Well, in case you misunderstood: I didn't ask for the check to be
>> _removed_, but for it to be _moved elsewhere_.
>>
>
> So as Razvan pointed out, there is a check already in hvm_do_resume
> for exactly the same effect, so then what you are asking for is
> already done.
Partly - I really meant all curr->arch.vm_event uses to go away from
that path. The other part (passing in the override buffer instead of
special casing vm-event handling here) still would need to be done.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 2/2] x86/vm_event: Allow overwriting Xen's i-cache used for emulation
2016-09-20 15:39 ` Jan Beulich
@ 2016-09-20 15:54 ` Tamas K Lengyel
2016-09-21 9:03 ` Jan Beulich
0 siblings, 1 reply; 19+ messages in thread
From: Tamas K Lengyel @ 2016-09-20 15:54 UTC (permalink / raw)
To: Jan Beulich
Cc: Kevin Tian, Stefano Stabellini, Razvan Cojocaru, George Dunlap,
Andrew Cooper, Julien Grall, Paul Durrant, Jun Nakajima,
xen-devel
On Tue, Sep 20, 2016 at 9:39 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 20.09.16 at 17:14, <tamas.lengyel@zentific.com> wrote:
>> On Tue, Sep 20, 2016 at 9:12 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>> On 20.09.16 at 16:56, <tamas.lengyel@zentific.com> wrote:
>>>> On Tue, Sep 20, 2016 at 1:26 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>>>> On 19.09.16 at 20:27, <tamas.lengyel@zentific.com> wrote:
>>>>>> On Mon, Sep 19, 2016 at 2:19 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>>>>>> On 15.09.16 at 18:51, <tamas.lengyel@zentific.com> wrote:
>>>>>>>> @@ -1793,7 +1793,17 @@ static int _hvm_emulate_one(struct hvm_emulate_ctxt
>>>>>> *hvmemul_ctxt,
>>>>>>>> pfec |= PFEC_user_mode;
>>>>>>>>
>>>>>>>> hvmemul_ctxt->insn_buf_eip = regs->eip;
>>>>>>>> - if ( !vio->mmio_insn_bytes )
>>>>>>>> +
>>>>>>>> + if ( unlikely(hvmemul_ctxt->set_context_insn) && curr->arch.vm_event )
>>>>>>>> + {
>>>>>>>> + BUILD_BUG_ON(sizeof(hvmemul_ctxt->insn_buf_bytes) ==
>>>>>>>> + sizeof(curr->arch.vm_event->emul.insn));
>>>>>>>
>>>>>>> This should quite clearly be !=, and I think it builds only because you
>>>>>>> use the wrong operand in the first sizeof().
>>>>>>>
>>>>>>>> + hvmemul_ctxt->insn_buf_bytes = sizeof(curr->arch.vm_event->emul.insn);
>>>>>>>> + memcpy(hvmemul_ctxt->insn_buf, &curr->arch.vm_event->emul.insn,
>>>>>>>> + hvmemul_ctxt->insn_buf_bytes);
>>>>>>>
>>>>>>> This memcpy()s between dissimilar types. Please omit the & and
>>>>>>> properly add .data on the second argument (and this .data
>>>>>>> addition should then also be mirrored in the BUILD_BUG_ON()).
>>>>>>>
>>>>>>>> + }
>>>>>>>> + else if ( !vio->mmio_insn_bytes )
>>>>>>>
>>>>>>> And then - I'm sorry for not having thought of this before - I think
>>>>>>> this would better not live here, or have an effect more explicitly
>>>>>>> only when coming here through hvm_emulate_one_vm_event().
>>>>>>> Since the former seems impractical, I think giving _hvm_emulate_one()
>>>>>>> one or two extra parameters would be the most straightforward
>>>>>>> approach.
>>>>>>
>>>>>> So this is the spot where the mmio insn buffer is getting copied as
>>>>>> well instead of fetching the instructions from the guest memory. So
>>>>>> having the vm_event buffer getting copied here too makes the most
>>>>>> sense. Having the vm_event insn buffer getting copied in somewhere
>>>>>> else, while the mmio insn buffer getting copied here, IMHO just
>>>>>> fragments the flow even more making it harder to see what is actually
>>>>>> happening.
>>>>>
>>>>> And I didn't unconditionally ask to move the copying elsewhere.
>>>>> The alternative - passing the override in as function argument(s),
>>>>> which would then be NULL/zero for all cases except the VM event
>>>>> one, would be as suitable. It is in particular ...
>>>>>
>>>>>> How about adjusting the if-else here to be:
>>>>>>
>>>>>> if ( !vio->mmio_insn_bytes && !hvmemul_ctxt->set_context_insn )
>>>>>> ...
>>>>>> else if ( vio->mmio_insn_bytes )
>>>>>> ...
>>>>>> else if ( unlikely(hvmemul_ctxt->set_context_insn) && curr->arch.vm_event )
>>>>>
>>>>> ... this curr->arch.vm_event reference which I'd like to see gone
>>>>> from this specific code path. The ordering in your original patch,
>>>>> otoh, would then be fine (check for the override first with unlikely(),
>>>>> else do what is being done today). Such a code structure would
>>>>> then also ease a possible second way of overriding the insn by
>>>>> some other party, without having to touch the code here again.
>>>>
>>>> So that check is one that Razvan asked to be added. I think it is
>>>> necessary too as there seems to be a race-condition if vm_event gets
>>>> shutdown after the response flag is set but before this emulation path
>>>> takes place. Effectively set_context_insn may be set but the
>>>> arch.vm_event already gotten freed. Razvan, is that correct?
>>>
>>> Well, in case you misunderstood: I didn't ask for the check to be
>>> _removed_, but for it to be _moved elsewhere_.
>>>
>>
>> So as Razvan pointed out, there is a check already in hvm_do_resume
>> for exactly the same effect, so then what you are asking for is
>> already done.
>
> Partly - I really meant all curr->arch.vm_event uses to go away from
> that path. The other part (passing in the override buffer instead of
> special casing vm-event handling here) still would need to be done.
>
I don't really follow what exactly you are looking for. You want the
buffer to be sent in as an input? We can do that but I mean the mmio
case doesn't do that either.. And what do you mean not "special casing
vm_event handling"? We need to handle it in an if-statement because by
default the buffer is fetched from memory. We don't want to do that,
just as the mmio case doesn't want that either. So I think if we want
to be consistent we do what the mmio case is doing, fetching the
buffer from curr->arch.hvm_vcpu.hvm_io, only we fetch it from
curr->arch.vm_event.
Tamas
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 2/2] x86/vm_event: Allow overwriting Xen's i-cache used for emulation
2016-09-20 15:54 ` Tamas K Lengyel
@ 2016-09-21 9:03 ` Jan Beulich
2016-09-21 14:22 ` Tamas K Lengyel
0 siblings, 1 reply; 19+ messages in thread
From: Jan Beulich @ 2016-09-21 9:03 UTC (permalink / raw)
To: Tamas K Lengyel
Cc: Kevin Tian, Stefano Stabellini, Razvan Cojocaru, George Dunlap,
Andrew Cooper, Julien Grall, Paul Durrant, Jun Nakajima,
xen-devel
>>> On 20.09.16 at 17:54, <tamas.lengyel@zentific.com> wrote:
> On Tue, Sep 20, 2016 at 9:39 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>> On 20.09.16 at 17:14, <tamas.lengyel@zentific.com> wrote:
>>> On Tue, Sep 20, 2016 at 9:12 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>>> On 20.09.16 at 16:56, <tamas.lengyel@zentific.com> wrote:
>>>>> On Tue, Sep 20, 2016 at 1:26 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>>>>> On 19.09.16 at 20:27, <tamas.lengyel@zentific.com> wrote:
>>>>>>> On Mon, Sep 19, 2016 at 2:19 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>>>>>>> On 15.09.16 at 18:51, <tamas.lengyel@zentific.com> wrote:
>>>>>>>>> @@ -1793,7 +1793,17 @@ static int _hvm_emulate_one(struct hvm_emulate_ctxt
>>>>>>> *hvmemul_ctxt,
>>>>>>>>> pfec |= PFEC_user_mode;
>>>>>>>>>
>>>>>>>>> hvmemul_ctxt->insn_buf_eip = regs->eip;
>>>>>>>>> - if ( !vio->mmio_insn_bytes )
>>>>>>>>> +
>>>>>>>>> + if ( unlikely(hvmemul_ctxt->set_context_insn) && curr->arch.vm_event )
>>>>>>>>> + {
>>>>>>>>> + BUILD_BUG_ON(sizeof(hvmemul_ctxt->insn_buf_bytes) ==
>>>>>>>>> + sizeof(curr->arch.vm_event->emul.insn));
>>>>>>>>
>>>>>>>> This should quite clearly be !=, and I think it builds only because you
>>>>>>>> use the wrong operand in the first sizeof().
>>>>>>>>
>>>>>>>>> + hvmemul_ctxt->insn_buf_bytes = sizeof(curr->arch.vm_event->emul.insn);
>>>>>>>>> + memcpy(hvmemul_ctxt->insn_buf, &curr->arch.vm_event->emul.insn,
>>>>>>>>> + hvmemul_ctxt->insn_buf_bytes);
>>>>>>>>
>>>>>>>> This memcpy()s between dissimilar types. Please omit the & and
>>>>>>>> properly add .data on the second argument (and this .data
>>>>>>>> addition should then also be mirrored in the BUILD_BUG_ON()).
>>>>>>>>
>>>>>>>>> + }
>>>>>>>>> + else if ( !vio->mmio_insn_bytes )
>>>>>>>>
>>>>>>>> And then - I'm sorry for not having thought of this before - I think
>>>>>>>> this would better not live here, or have an effect more explicitly
>>>>>>>> only when coming here through hvm_emulate_one_vm_event().
>>>>>>>> Since the former seems impractical, I think giving _hvm_emulate_one()
>>>>>>>> one or two extra parameters would be the most straightforward
>>>>>>>> approach.
>>>>>>>
>>>>>>> So this is the spot where the mmio insn buffer is getting copied as
>>>>>>> well instead of fetching the instructions from the guest memory. So
>>>>>>> having the vm_event buffer getting copied here too makes the most
>>>>>>> sense. Having the vm_event insn buffer getting copied in somewhere
>>>>>>> else, while the mmio insn buffer getting copied here, IMHO just
>>>>>>> fragments the flow even more making it harder to see what is actually
>>>>>>> happening.
>>>>>>
>>>>>> And I didn't unconditionally ask to move the copying elsewhere.
>>>>>> The alternative - passing the override in as function argument(s),
>>>>>> which would then be NULL/zero for all cases except the VM event
>>>>>> one, would be as suitable. It is in particular ...
>>>>>>
>>>>>>> How about adjusting the if-else here to be:
>>>>>>>
>>>>>>> if ( !vio->mmio_insn_bytes && !hvmemul_ctxt->set_context_insn )
>>>>>>> ...
>>>>>>> else if ( vio->mmio_insn_bytes )
>>>>>>> ...
>>>>>>> else if ( unlikely(hvmemul_ctxt->set_context_insn) && curr->arch.vm_event )
>>>>>>
>>>>>> ... this curr->arch.vm_event reference which I'd like to see gone
>>>>>> from this specific code path. The ordering in your original patch,
>>>>>> otoh, would then be fine (check for the override first with unlikely(),
>>>>>> else do what is being done today). Such a code structure would
>>>>>> then also ease a possible second way of overriding the insn by
>>>>>> some other party, without having to touch the code here again.
>>>>>
>>>>> So that check is one that Razvan asked to be added. I think it is
>>>>> necessary too as there seems to be a race-condition if vm_event gets
>>>>> shutdown after the response flag is set but before this emulation path
>>>>> takes place. Effectively set_context_insn may be set but the
>>>>> arch.vm_event already gotten freed. Razvan, is that correct?
>>>>
>>>> Well, in case you misunderstood: I didn't ask for the check to be
>>>> _removed_, but for it to be _moved elsewhere_.
>>>>
>>>
>>> So as Razvan pointed out, there is a check already in hvm_do_resume
>>> for exactly the same effect, so then what you are asking for is
>>> already done.
>>
>> Partly - I really meant all curr->arch.vm_event uses to go away from
>> that path. The other part (passing in the override buffer instead of
>> special casing vm-event handling here) still would need to be done.
>>
>
> I don't really follow what exactly you are looking for. You want the
> buffer to be sent in as an input? We can do that but I mean the mmio
> case doesn't do that either.. And what do you mean not "special casing
> vm_event handling"? We need to handle it in an if-statement because by
> default the buffer is fetched from memory. We don't want to do that,
> just as the mmio case doesn't want that either. So I think if we want
> to be consistent we do what the mmio case is doing, fetching the
> buffer from curr->arch.hvm_vcpu.hvm_io, only we fetch it from
> curr->arch.vm_event.
No. Please look back at my original reply (still visible in context
above). You're comparing apples and oranges - the existing override
is an integral part of the emulation logic, while yours is an add-on.
And btw., see how
https://lists.xenproject.org/archives/html/xen-devel/2016-09/msg00897.html
even factors out that part.
It might even be an option to simply copy your override data right
into vio->mmio_insn{,_bytes}, in the vm-event specific function,
allowing all other code to remain untouched.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 2/2] x86/vm_event: Allow overwriting Xen's i-cache used for emulation
2016-09-21 9:03 ` Jan Beulich
@ 2016-09-21 14:22 ` Tamas K Lengyel
0 siblings, 0 replies; 19+ messages in thread
From: Tamas K Lengyel @ 2016-09-21 14:22 UTC (permalink / raw)
To: Jan Beulich
Cc: Kevin Tian, Stefano Stabellini, Razvan Cojocaru, George Dunlap,
Andrew Cooper, Julien Grall, Paul Durrant, Jun Nakajima,
xen-devel
On Wed, Sep 21, 2016 at 3:03 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 20.09.16 at 17:54, <tamas.lengyel@zentific.com> wrote:
>> On Tue, Sep 20, 2016 at 9:39 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>> On 20.09.16 at 17:14, <tamas.lengyel@zentific.com> wrote:
>>>> On Tue, Sep 20, 2016 at 9:12 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>>>> On 20.09.16 at 16:56, <tamas.lengyel@zentific.com> wrote:
>>>>>> On Tue, Sep 20, 2016 at 1:26 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>>>>>> On 19.09.16 at 20:27, <tamas.lengyel@zentific.com> wrote:
>>>>>>>> On Mon, Sep 19, 2016 at 2:19 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>>>>>>>> On 15.09.16 at 18:51, <tamas.lengyel@zentific.com> wrote:
>>>>>>>>>> @@ -1793,7 +1793,17 @@ static int _hvm_emulate_one(struct hvm_emulate_ctxt
>>>>>>>> *hvmemul_ctxt,
>>>>>>>>>> pfec |= PFEC_user_mode;
>>>>>>>>>>
>>>>>>>>>> hvmemul_ctxt->insn_buf_eip = regs->eip;
>>>>>>>>>> - if ( !vio->mmio_insn_bytes )
>>>>>>>>>> +
>>>>>>>>>> + if ( unlikely(hvmemul_ctxt->set_context_insn) && curr->arch.vm_event )
>>>>>>>>>> + {
>>>>>>>>>> + BUILD_BUG_ON(sizeof(hvmemul_ctxt->insn_buf_bytes) ==
>>>>>>>>>> + sizeof(curr->arch.vm_event->emul.insn));
>>>>>>>>>
>>>>>>>>> This should quite clearly be !=, and I think it builds only because you
>>>>>>>>> use the wrong operand in the first sizeof().
>>>>>>>>>
>>>>>>>>>> + hvmemul_ctxt->insn_buf_bytes = sizeof(curr->arch.vm_event->emul.insn);
>>>>>>>>>> + memcpy(hvmemul_ctxt->insn_buf, &curr->arch.vm_event->emul.insn,
>>>>>>>>>> + hvmemul_ctxt->insn_buf_bytes);
>>>>>>>>>
>>>>>>>>> This memcpy()s between dissimilar types. Please omit the & and
>>>>>>>>> properly add .data on the second argument (and this .data
>>>>>>>>> addition should then also be mirrored in the BUILD_BUG_ON()).
>>>>>>>>>
>>>>>>>>>> + }
>>>>>>>>>> + else if ( !vio->mmio_insn_bytes )
>>>>>>>>>
>>>>>>>>> And then - I'm sorry for not having thought of this before - I think
>>>>>>>>> this would better not live here, or have an effect more explicitly
>>>>>>>>> only when coming here through hvm_emulate_one_vm_event().
>>>>>>>>> Since the former seems impractical, I think giving _hvm_emulate_one()
>>>>>>>>> one or two extra parameters would be the most straightforward
>>>>>>>>> approach.
>>>>>>>>
>>>>>>>> So this is the spot where the mmio insn buffer is getting copied as
>>>>>>>> well instead of fetching the instructions from the guest memory. So
>>>>>>>> having the vm_event buffer getting copied here too makes the most
>>>>>>>> sense. Having the vm_event insn buffer getting copied in somewhere
>>>>>>>> else, while the mmio insn buffer getting copied here, IMHO just
>>>>>>>> fragments the flow even more making it harder to see what is actually
>>>>>>>> happening.
>>>>>>>
>>>>>>> And I didn't unconditionally ask to move the copying elsewhere.
>>>>>>> The alternative - passing the override in as function argument(s),
>>>>>>> which would then be NULL/zero for all cases except the VM event
>>>>>>> one, would be as suitable. It is in particular ...
>>>>>>>
>>>>>>>> How about adjusting the if-else here to be:
>>>>>>>>
>>>>>>>> if ( !vio->mmio_insn_bytes && !hvmemul_ctxt->set_context_insn )
>>>>>>>> ...
>>>>>>>> else if ( vio->mmio_insn_bytes )
>>>>>>>> ...
>>>>>>>> else if ( unlikely(hvmemul_ctxt->set_context_insn) && curr->arch.vm_event )
>>>>>>>
>>>>>>> ... this curr->arch.vm_event reference which I'd like to see gone
>>>>>>> from this specific code path. The ordering in your original patch,
>>>>>>> otoh, would then be fine (check for the override first with unlikely(),
>>>>>>> else do what is being done today). Such a code structure would
>>>>>>> then also ease a possible second way of overriding the insn by
>>>>>>> some other party, without having to touch the code here again.
>>>>>>
>>>>>> So that check is one that Razvan asked to be added. I think it is
>>>>>> necessary too as there seems to be a race-condition if vm_event gets
>>>>>> shutdown after the response flag is set but before this emulation path
>>>>>> takes place. Effectively set_context_insn may be set but the
>>>>>> arch.vm_event already gotten freed. Razvan, is that correct?
>>>>>
>>>>> Well, in case you misunderstood: I didn't ask for the check to be
>>>>> _removed_, but for it to be _moved elsewhere_.
>>>>>
>>>>
>>>> So as Razvan pointed out, there is a check already in hvm_do_resume
>>>> for exactly the same effect, so then what you are asking for is
>>>> already done.
>>>
>>> Partly - I really meant all curr->arch.vm_event uses to go away from
>>> that path. The other part (passing in the override buffer instead of
>>> special casing vm-event handling here) still would need to be done.
>>>
>>
>> I don't really follow what exactly you are looking for. You want the
>> buffer to be sent in as an input? We can do that but I mean the mmio
>> case doesn't do that either.. And what do you mean not "special casing
>> vm_event handling"? We need to handle it in an if-statement because by
>> default the buffer is fetched from memory. We don't want to do that,
>> just as the mmio case doesn't want that either. So I think if we want
>> to be consistent we do what the mmio case is doing, fetching the
>> buffer from curr->arch.hvm_vcpu.hvm_io, only we fetch it from
>> curr->arch.vm_event.
>
> No. Please look back at my original reply (still visible in context
> above). You're comparing apples and oranges - the existing override
> is an integral part of the emulation logic, while yours is an add-on.
> And btw., see how
> https://lists.xenproject.org/archives/html/xen-devel/2016-09/msg00897.html
> even factors out that part.
>
> It might even be an option to simply copy your override data right
> into vio->mmio_insn{,_bytes}, in the vm-event specific function,
> allowing all other code to remain untouched.
We can do that, though that seems to be a bit hack-ish. OTOH it would
not require any other code-changes, as you say, so if that's really an
option, I'm fine with it.
Tamas
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2016-09-21 14:22 UTC | newest]
Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-09-15 16:51 [PATCH v2 1/2] vm_event: Sanitize vm_event response handling Tamas K Lengyel
2016-09-15 16:51 ` [PATCH v2 2/2] x86/vm_event: Allow overwriting Xen's i-cache used for emulation Tamas K Lengyel
2016-09-17 16:40 ` Razvan Cojocaru
2016-09-17 16:54 ` Tamas K Lengyel
2016-09-19 8:19 ` Jan Beulich
2016-09-19 16:40 ` Tamas K Lengyel
2016-09-20 0:36 ` Tian, Kevin
2016-09-20 15:32 ` Tamas K Lengyel
2016-09-19 18:27 ` Tamas K Lengyel
2016-09-20 7:26 ` Jan Beulich
2016-09-20 14:56 ` Tamas K Lengyel
2016-09-20 15:09 ` Razvan Cojocaru
2016-09-20 15:12 ` Jan Beulich
2016-09-20 15:14 ` Tamas K Lengyel
2016-09-20 15:39 ` Jan Beulich
2016-09-20 15:54 ` Tamas K Lengyel
2016-09-21 9:03 ` Jan Beulich
2016-09-21 14:22 ` Tamas K Lengyel
2016-09-17 16:36 ` [PATCH v2 1/2] vm_event: Sanitize vm_event response handling Razvan Cojocaru
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.