Joe Epstein
2011-Jan-04 22:07 UTC
[Xen-devel] [PATCH 5 of 6] REDO: mem_access & mem_access 2: added INT3/CRx capture
* Allows a memory event listener to register for events on changes to
CR0, CR3, and CR4, as well as INT3 instructions, as a part of the
mem_access mechanism. These events can be either synchronous or
asynchronous.
* For INT3, the logic works independent of a debugger, and so both can
be supported.
* The presence and type of listener are stored and accessed through
HVM params.
* Changed the event mask handling to ensure that the right events are
captured based on the listeners.
* Added the ability to inject HW/SW traps into a VCPU when it next
resumes (rather than try to modify the existing IRQ injection
code paths). Only one trap to inject can be outstanding at a time.
Signed-off-by: Joe Epstein <jepstein98@gmail.com>
diff -r 3500724e8052 -r c1866aff7a5f xen/include/asm-x86/hvm/hvm.h
--- a/xen/include/asm-x86/hvm/hvm.h Tue Jan 04 12:34:06 2011 -0800
+++ b/xen/include/asm-x86/hvm/hvm.h Tue Jan 04 12:35:49 2011 -0800
@@ -370,4 +370,12 @@
int hvm_x2apic_msr_read(struct vcpu *v, unsigned int msr, uint64_t
*msr_content);
int hvm_x2apic_msr_write(struct vcpu *v, unsigned int msr, uint64_t
msr_content);
+/* Called for current VCPU on crX changes by guest */
+int hvm_memory_event_cr0(unsigned long value, unsigned long old);
+int hvm_memory_event_cr3(unsigned long value, unsigned long old);
+int hvm_memory_event_cr4(unsigned long value, unsigned long old);
+
+/* Called for current VCPU on int3 */
+int hvm_memory_event_int3(unsigned long gla);
+
#endif /* __ASM_X86_HVM_HVM_H__ */
diff -r 3500724e8052 -r c1866aff7a5f xen/include/asm-x86/hvm/vcpu.h
--- a/xen/include/asm-x86/hvm/vcpu.h Tue Jan 04 12:34:06 2011 -0800
+++ b/xen/include/asm-x86/hvm/vcpu.h Tue Jan 04 12:35:49 2011 -0800
@@ -114,6 +114,11 @@
/* We may write up to m128 as a number of device-model transactions. */
paddr_t mmio_large_write_pa;
unsigned int mmio_large_write_bytes;
+
+ /* Pending hw/sw interrupt */
+ int inject_trap; /* -1 for nothing to inject */
+ int inject_error_code;
+ unsigned long inject_cr2;
};
#endif /* __ASM_X86_HVM_VCPU_H__ */
diff -r 3500724e8052 -r c1866aff7a5f xen/include/public/hvm/hvm_op.h
--- a/xen/include/public/hvm/hvm_op.h Tue Jan 04 12:34:06 2011 -0800
+++ b/xen/include/public/hvm/hvm_op.h Tue Jan 04 12:35:49 2011 -0800
@@ -198,4 +198,23 @@
};
typedef struct xen_hvm_get_mem_access xen_hvm_get_mem_access_t;
DEFINE_XEN_GUEST_HANDLE(xen_hvm_get_mem_access_t);
+
+#define HVMOP_inject_trap 14
+/* Inject a trap into a VCPU, which will get taken up on the next
+ * scheduling of it */
+struct xen_hvm_inject_trap {
+ /* Domain to be queried. */
+ domid_t domid;
+ /* VCPU */
+ uint32_t vcpuid;
+ /* Trap number */
+ uint32_t trap;
+ /* Error code, or -1 to skip */
+ uint32_t error_code;
+ /* CR2 for page faults */
+ uint64_t cr2;
+};
+typedef struct xen_hvm_inject_trap xen_hvm_inject_trap_t;
+DEFINE_XEN_GUEST_HANDLE(xen_hvm_inject_trap_t);
+
#endif /* __XEN_PUBLIC_HVM_HVM_OP_H__ */
diff -r 3500724e8052 -r c1866aff7a5f xen/include/public/hvm/params.h
--- a/xen/include/public/hvm/params.h Tue Jan 04 12:34:06 2011 -0800
+++ b/xen/include/public/hvm/params.h Tue Jan 04 12:35:49 2011 -0800
@@ -124,6 +124,19 @@
*/
#define HVM_PARAM_ACPI_IOPORTS_LOCATION 19
-#define HVM_NR_PARAMS 20
+/* Enable blocking memory events, async or sync (pause vcpu until response)
+ * onchangeonly indicates messages only on a change of value */
+#define HVM_PARAM_MEMORY_EVENT_CR0 20
+#define HVM_PARAM_MEMORY_EVENT_CR3 21
+#define HVM_PARAM_MEMORY_EVENT_CR4 22
+#define HVM_PARAM_MEMORY_EVENT_INT3 23
+
+#define HVMPME_MODE_MASK (3 << 0)
+#define HVMPME_mode_disabled 0
+#define HVMPME_mode_async 1
+#define HVMPME_mode_sync 2
+#define HVMPME_onchangeonly (1 << 2)
+
+#define HVM_NR_PARAMS 24
#endif /* __XEN_PUBLIC_HVM_PARAMS_H__ */
diff -r 3500724e8052 -r c1866aff7a5f xen/include/public/mem_event.h
--- a/xen/include/public/mem_event.h Tue Jan 04 12:34:06 2011 -0800
+++ b/xen/include/public/mem_event.h Tue Jan 04 12:35:49 2011 -0800
@@ -37,6 +37,10 @@
/* Reasons for the memory event request */
#define MEM_EVENT_REASON_UNKNOWN 0 /* typical reason */
#define MEM_EVENT_REASON_VIOLATION 1 /* access violation, GFN is
address */
+#define MEM_EVENT_REASON_CR0 2 /* CR0 was hit: gfn is CR0 value */
+#define MEM_EVENT_REASON_CR3 3 /* CR3 was hit: gfn is CR3 value */
+#define MEM_EVENT_REASON_CR4 4 /* CR4 was hit: gfn is CR4 value */
+#define MEM_EVENT_REASON_INT3 5 /* int3 was hit: gla/gfn are RIP */
typedef struct mem_event_shared_page {
uint32_t port;
diff -r 3500724e8052 -r c1866aff7a5f xen/arch/x86/hvm/hvm.c
--- a/xen/arch/x86/hvm/hvm.c Tue Jan 04 12:34:06 2011 -0800
+++ b/xen/arch/x86/hvm/hvm.c Tue Jan 04 12:35:49 2011 -0800
@@ -309,6 +309,15 @@
return; /* bail */
}
}
+
+ /* Inject pending hw/sw trap */
+ if (v->arch.hvm_vcpu.inject_trap != -1)
+ {
+ hvm_inject_exception(v->arch.hvm_vcpu.inject_trap,
+ v->arch.hvm_vcpu.inject_error_code,
+ v->arch.hvm_vcpu.inject_cr2);
+ v->arch.hvm_vcpu.inject_trap = -1;
+ }
}
static void hvm_init_ioreq_page(
@@ -949,6 +958,8 @@
spin_lock_init(&v->arch.hvm_vcpu.tm_lock);
INIT_LIST_HEAD(&v->arch.hvm_vcpu.tm_list);
+ v->arch.hvm_vcpu.inject_trap = -1;
+
#ifdef CONFIG_COMPAT
rc = setup_compat_arg_xlat(v);
if ( rc != 0 )
@@ -3216,10 +3227,34 @@
case HVM_PARAM_ACPI_IOPORTS_LOCATION:
rc = pmtimer_change_ioport(d, a.value);
break;
+ case HVM_PARAM_MEMORY_EVENT_INT3:
+ if ( a.value & HVMPME_onchangeonly )
+ rc = -EINVAL;
+ break;
}
- if ( rc == 0 )
+ if ( rc == 0 )
+ {
d->arch.hvm_domain.params[a.index] = a.value;
+
+ switch( a.index )
+ {
+ case HVM_PARAM_MEMORY_EVENT_INT3:
+ {
+ domain_pause(d);
+ domain_unpause(d); /* Causes guest to latch new status */
+ break;
+ }
+ case HVM_PARAM_MEMORY_EVENT_CR3:
+ {
+ for_each_vcpu ( d, v )
+ hvm_funcs.update_guest_cr(v, 0); /* Latches
new CR3 mask through CR0 code */
+ break;
+ }
+ }
+
+ }
+
}
else
{
@@ -3631,6 +3666,41 @@
break;
}
+ case HVMOP_inject_trap:
+ {
+ xen_hvm_inject_trap_t tr;
+ struct domain *d;
+ struct vcpu *v;
+
+ if ( copy_from_guest(&tr, arg, 1 ) )
+ return -EFAULT;
+
+ rc = rcu_lock_target_domain_by_id(tr.domid, &d);
+ if ( rc != 0 )
+ return rc;
+
+ rc = -EINVAL;
+ if ( !is_hvm_domain(d) )
+ goto param_fail8;
+
+ rc = -ENOENT;
+ if ( tr.vcpuid >= d->max_vcpus || (v = d->vcpu[tr.vcpuid]) ==
NULL )
+ goto param_fail8;
+
+ if ( v->arch.hvm_vcpu.inject_trap != -1 )
+ rc = -EBUSY;
+ else
+ {
+ v->arch.hvm_vcpu.inject_trap = tr.trap;
+ v->arch.hvm_vcpu.inject_error_code = tr.error_code;
+ v->arch.hvm_vcpu.inject_cr2 = tr.cr2;
+ }
+
+ param_fail8:
+ rcu_unlock_domain(d);
+ break;
+ }
+
default:
{
gdprintk(XENLOG_WARNING, "Bad HVM op %ld.\n", op);
@@ -3671,6 +3741,161 @@
return rc;
}
+int hvm_memory_event_cr0(unsigned long value, unsigned long old) {
+ struct vcpu* v = current;
+ struct domain *d = v->domain;
+ mem_event_request_t req;
+
+ long p = d->arch.hvm_domain.params[HVM_PARAM_MEMORY_EVENT_CR0];
+
+ if ( p & HVMPME_MODE_MASK )
+ {
+ int rc;
+
+ if ( (p & HVMPME_onchangeonly) && value == old )
+ return 1;
+
+ rc = mem_event_check_ring(d);
+ if ( rc )
+ return rc;
+
+ memset(&req, 0, sizeof(req));
+ req.type = MEM_EVENT_TYPE_ACCESS;
+ req.reason = MEM_EVENT_REASON_CR0;
+
+ if ( (p & HVMPME_MODE_MASK) == HVMPME_mode_sync )
+ {
+ req.flags |= MEM_EVENT_FLAG_VCPU_PAUSED;
+ vcpu_pause_nosync(v);
+ }
+
+ req.gfn = value;
+ req.vcpu_id = v->vcpu_id;
+
+ mem_event_put_request(d, &req);
+
+ return 1;
+ }
+ return 0;
+}
+
+int hvm_memory_event_cr3(unsigned long value, unsigned long old) {
+ struct vcpu* v = current;
+ struct domain *d = v->domain;
+ mem_event_request_t req;
+
+ long p = d->arch.hvm_domain.params[HVM_PARAM_MEMORY_EVENT_CR3];
+
+ if ( p & HVMPME_MODE_MASK )
+ {
+ int rc;
+
+ if ( (p & HVMPME_onchangeonly) && value == old )
+ return 1;
+
+ rc = mem_event_check_ring(d);
+ if ( rc )
+ return rc;
+
+ memset(&req, 0, sizeof(req));
+ req.type = MEM_EVENT_TYPE_ACCESS;
+ req.reason = MEM_EVENT_REASON_CR3;
+
+ if ( (p & HVMPME_MODE_MASK) == HVMPME_mode_sync )
+ {
+ req.flags |= MEM_EVENT_FLAG_VCPU_PAUSED;
+ vcpu_pause_nosync(v);
+ }
+
+ req.gfn = value;
+ req.vcpu_id = v->vcpu_id;
+
+ mem_event_put_request(d, &req);
+
+ return 1;
+ }
+ return 0;
+}
+
+int hvm_memory_event_cr4(unsigned long value, unsigned long old) {
+ struct vcpu* v = current;
+ struct domain *d = v->domain;
+ mem_event_request_t req;
+
+ long p = d->arch.hvm_domain.params[HVM_PARAM_MEMORY_EVENT_CR4];
+
+ if ( p & HVMPME_MODE_MASK )
+ {
+ int rc;
+
+ if ( (p & HVMPME_onchangeonly) && value == old )
+ return 1;
+
+ rc = mem_event_check_ring(d);
+ if ( rc )
+ return rc;
+
+ memset(&req, 0, sizeof(req));
+ req.type = MEM_EVENT_TYPE_ACCESS;
+ req.reason = MEM_EVENT_REASON_CR4;
+
+ if ( (p & HVMPME_MODE_MASK) == HVMPME_mode_sync )
+ {
+ req.flags |= MEM_EVENT_FLAG_VCPU_PAUSED;
+ vcpu_pause_nosync(v);
+ }
+
+ req.gfn = value;
+ req.vcpu_id = v->vcpu_id;
+
+ mem_event_put_request(d, &req);
+
+ return 1;
+ }
+ return 0;
+}
+
+int hvm_memory_event_int3(unsigned long gla) {
+ struct vcpu* v = current;
+ struct domain *d = v->domain;
+ mem_event_request_t req;
+
+ long p = d->arch.hvm_domain.params[HVM_PARAM_MEMORY_EVENT_INT3];
+
+ if ( p & HVMPME_MODE_MASK )
+ {
+ uint32_t pfec = PFEC_page_present;
+ unsigned long gfn;
+ int rc;
+
+ rc = mem_event_check_ring(d);
+ if ( rc )
+ return rc;
+
+ gfn = paging_gva_to_gfn(current, gla, &pfec);
+
+ memset(&req, 0, sizeof(req));
+ req.type = MEM_EVENT_TYPE_ACCESS;
+ req.reason = MEM_EVENT_REASON_INT3;
+
+ if ( (p & HVMPME_MODE_MASK) == HVMPME_mode_sync )
+ {
+ req.flags |= MEM_EVENT_FLAG_VCPU_PAUSED;
+ vcpu_pause_nosync(v);
+ }
+
+ req.gfn = gfn;
+ req.offset = gla & ((1 << PAGE_SHIFT) - 1);
+ req.gla = gla;
+ req.gla_valid = 1;
+ req.vcpu_id = v->vcpu_id;
+
+ mem_event_put_request(d, &req);
+
+ return 1;
+ }
+ return 0;
+}
/*
* Local variables:
diff -r 3500724e8052 -r c1866aff7a5f xen/arch/x86/hvm/vmx/vmcs.c
--- a/xen/arch/x86/hvm/vmx/vmcs.c Tue Jan 04 12:34:06 2011 -0800
+++ b/xen/arch/x86/hvm/vmx/vmcs.c Tue Jan 04 12:35:49 2011 -0800
@@ -1082,7 +1082,9 @@
hvm_asid_flush_vcpu(v);
}
- debug_state = v->domain->debugger_attached;
+ debug_state = v->domain->debugger_attached
+ ||
v->domain->arch.hvm_domain.params[HVM_PARAM_MEMORY_EVENT_INT3];
+
if ( unlikely(v->arch.hvm_vcpu.debug_state_latch != debug_state) )
{
v->arch.hvm_vcpu.debug_state_latch = debug_state;
diff -r 3500724e8052 -r c1866aff7a5f xen/arch/x86/hvm/vmx/vmx.c
--- a/xen/arch/x86/hvm/vmx/vmx.c Tue Jan 04 12:34:06 2011 -0800
+++ b/xen/arch/x86/hvm/vmx/vmx.c Tue Jan 04 12:35:49 2011 -0800
@@ -1064,12 +1064,16 @@
if ( paging_mode_hap(v->domain) )
{
- /* We manage GUEST_CR3 when guest CR0.PE is zero. */
+ /* We manage GUEST_CR3 when guest CR0.PE is zero or when
cr3 memevents are on */
uint32_t cr3_ctls = (CPU_BASED_CR3_LOAD_EXITING |
CPU_BASED_CR3_STORE_EXITING);
v->arch.hvm_vmx.exec_control &= ~cr3_ctls;
if ( !hvm_paging_enabled(v) )
v->arch.hvm_vmx.exec_control |= cr3_ctls;
+
+ if (
v->domain->arch.hvm_domain.params[HVM_PARAM_MEMORY_EVENT_CR3] )
+ v->arch.hvm_vmx.exec_control |= CPU_BASED_CR3_LOAD_EXITING;
+
vmx_update_cpu_exec_control(v);
/* Changing CR0.PE can change some bits in real CR4. */
@@ -1252,9 +1256,12 @@
unsigned long intr_info = __vmread(VM_ENTRY_INTR_INFO);
struct vcpu *curr = current;
+ int type = X86_EVENTTYPE_HW_EXCEPTION;
+
switch ( trap )
{
case TRAP_debug:
+ type = X86_EVENTTYPE_SW_EXCEPTION;
if ( guest_cpu_user_regs()->eflags & X86_EFLAGS_TF )
{
__restore_debug_registers(curr);
@@ -1269,6 +1276,9 @@
domain_pause_for_debugger();
return;
}
+
+ type = X86_EVENTTYPE_SW_EXCEPTION;
+ __vmwrite(VM_ENTRY_INSTRUCTION_LEN, 1); /* int3 */
}
if ( unlikely(intr_info & INTR_INFO_VALID_MASK) &&
@@ -1279,7 +1289,7 @@
error_code = 0;
}
- __vmx_inject_exception(trap, X86_EVENTTYPE_HW_EXCEPTION, error_code);
+ __vmx_inject_exception(trap, type, error_code);
if ( trap == TRAP_page_fault )
HVMTRACE_LONG_2D(PF_INJECT, error_code,
@@ -1565,6 +1575,8 @@
unsigned long value;
struct vcpu *v = current;
struct vlapic *vlapic = vcpu_vlapic(v);
+ int rc = 0;
+ unsigned long old;
switch ( gp )
{
@@ -1589,13 +1601,25 @@
switch ( cr )
{
case 0:
- return !hvm_set_cr0(value);
+ old = v->arch.hvm_vcpu.guest_cr[0];
+ rc = !hvm_set_cr0(value);
+ if (rc)
+ hvm_memory_event_cr0(value, old);
+ return rc;
case 3:
- return !hvm_set_cr3(value);
+ old = v->arch.hvm_vcpu.guest_cr[3];
+ rc = !hvm_set_cr3(value);
+ if (rc)
+ hvm_memory_event_cr3(value, old);
+ return rc;
case 4:
- return !hvm_set_cr4(value);
+ old = v->arch.hvm_vcpu.guest_cr[4];
+ rc = !hvm_set_cr4(value);
+ if (rc)
+ hvm_memory_event_cr4(value, old);
+ return rc;
case 8:
vlapic_set_reg(vlapic, APIC_TASKPRI, ((value & 0x0F) << 4));
@@ -1676,11 +1700,17 @@
cr = exit_qualification & VMX_CONTROL_REG_ACCESS_NUM;
mov_from_cr(cr, gp, regs);
break;
- case VMX_CONTROL_REG_ACCESS_TYPE_CLTS:
+ case VMX_CONTROL_REG_ACCESS_TYPE_CLTS:
+ {
+ unsigned long old = v->arch.hvm_vcpu.guest_cr[0];
v->arch.hvm_vcpu.guest_cr[0] &= ~X86_CR0_TS;
vmx_update_guest_cr(v, 0);
+
+ hvm_memory_event_cr0(v->arch.hvm_vcpu.guest_cr[0], old);
+
HVMTRACE_0D(CLTS);
break;
+ }
case VMX_CONTROL_REG_ACCESS_TYPE_LMSW:
value = v->arch.hvm_vcpu.guest_cr[0];
/* LMSW can: (1) set bits 0-3; (2) clear bits 1-3. */
@@ -2350,13 +2380,29 @@
goto exit_and_crash;
domain_pause_for_debugger();
break;
- case TRAP_int3:
- if ( !v->domain->debugger_attached )
- goto exit_and_crash;
- update_guest_eip(); /* Safe: INT3 */
- current->arch.gdbsx_vcpu_event = TRAP_int3;
- domain_pause_for_debugger();
- break;
+ case TRAP_int3:
+ {
+ if ( v->domain->debugger_attached )
+ {
+ update_guest_eip(); /* Safe: INT3 */
+ current->arch.gdbsx_vcpu_event = TRAP_int3;
+ domain_pause_for_debugger();
+ break;
+ }
+ else {
+ int handled = hvm_memory_event_int3(regs->eip);
+
+ if ( handled < 0 )
+ {
+ vmx_inject_exception(TRAP_int3,
HVM_DELIVER_NO_ERROR_CODE, 0);
+ break;
+ }
+ else if ( handled )
+ break;
+ }
+
+ goto exit_and_crash;
+ }
case TRAP_no_device:
vmx_fpu_dirty_intercept();
break;
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Keir Fraser
2011-Jan-05 12:22 UTC
Re: [Xen-devel] [PATCH 5 of 6] REDO: mem_access & mem_access 2: added INT3/CRx capture
Code duplication in the hvm_memory_event_*() functions. That should be
abstracted out into a common function hvm_memory_event() taking sufficient
extra args so that it can be called from each of the (now very small)
specific hvm_memory_event_*() functions.
I also note some coding style issues:
- Opening brace for a function definition belongs on a new line
- The long if(...){....} in hvm_memory_event_*() can be better expressed as
if(!...) return 0; then the main meat of the function can follow, without
indentation within an if block.
Also your inline patches are being wordwrapped by your email client, so they
won''t apply. We will apply your patches as a series, rather than the
all-in-one attachment you also posted, so the inline patches do need to be
apply-able.
-- Keir
On 04/01/2011 22:07, "Joe Epstein" <jepstein98@gmail.com> wrote:
> * Allows a memory event listener to register for events on changes to
> CR0, CR3, and CR4, as well as INT3 instructions, as a part of the
> mem_access mechanism. These events can be either synchronous or
> asynchronous.
>
> * For INT3, the logic works independent of a debugger, and so both can
> be supported.
>
> * The presence and type of listener are stored and accessed through
> HVM params.
>
> * Changed the event mask handling to ensure that the right events are
> captured based on the listeners.
>
> * Added the ability to inject HW/SW traps into a VCPU when it next
> resumes (rather than try to modify the existing IRQ injection
> code paths). Only one trap to inject can be outstanding at a time.
>
> Signed-off-by: Joe Epstein <jepstein98@gmail.com>
>
> diff -r 3500724e8052 -r c1866aff7a5f xen/include/asm-x86/hvm/hvm.h
> --- a/xen/include/asm-x86/hvm/hvm.h Tue Jan 04 12:34:06 2011 -0800
> +++ b/xen/include/asm-x86/hvm/hvm.h Tue Jan 04 12:35:49 2011 -0800
> @@ -370,4 +370,12 @@
> int hvm_x2apic_msr_read(struct vcpu *v, unsigned int msr, uint64_t
> *msr_content);
> int hvm_x2apic_msr_write(struct vcpu *v, unsigned int msr, uint64_t
> msr_content);
>
> +/* Called for current VCPU on crX changes by guest */
> +int hvm_memory_event_cr0(unsigned long value, unsigned long old);
> +int hvm_memory_event_cr3(unsigned long value, unsigned long old);
> +int hvm_memory_event_cr4(unsigned long value, unsigned long old);
> +
> +/* Called for current VCPU on int3 */
> +int hvm_memory_event_int3(unsigned long gla);
> +
> #endif /* __ASM_X86_HVM_HVM_H__ */
> diff -r 3500724e8052 -r c1866aff7a5f xen/include/asm-x86/hvm/vcpu.h
> --- a/xen/include/asm-x86/hvm/vcpu.h Tue Jan 04 12:34:06 2011 -0800
> +++ b/xen/include/asm-x86/hvm/vcpu.h Tue Jan 04 12:35:49 2011 -0800
> @@ -114,6 +114,11 @@
> /* We may write up to m128 as a number of device-model transactions.
*/
> paddr_t mmio_large_write_pa;
> unsigned int mmio_large_write_bytes;
> +
> + /* Pending hw/sw interrupt */
> + int inject_trap; /* -1 for nothing to inject */
> + int inject_error_code;
> + unsigned long inject_cr2;
> };
>
> #endif /* __ASM_X86_HVM_VCPU_H__ */
> diff -r 3500724e8052 -r c1866aff7a5f xen/include/public/hvm/hvm_op.h
> --- a/xen/include/public/hvm/hvm_op.h Tue Jan 04 12:34:06 2011 -0800
> +++ b/xen/include/public/hvm/hvm_op.h Tue Jan 04 12:35:49 2011 -0800
> @@ -198,4 +198,23 @@
> };
> typedef struct xen_hvm_get_mem_access xen_hvm_get_mem_access_t;
> DEFINE_XEN_GUEST_HANDLE(xen_hvm_get_mem_access_t);
> +
> +#define HVMOP_inject_trap 14
> +/* Inject a trap into a VCPU, which will get taken up on the next
> + * scheduling of it */
> +struct xen_hvm_inject_trap {
> + /* Domain to be queried. */
> + domid_t domid;
> + /* VCPU */
> + uint32_t vcpuid;
> + /* Trap number */
> + uint32_t trap;
> + /* Error code, or -1 to skip */
> + uint32_t error_code;
> + /* CR2 for page faults */
> + uint64_t cr2;
> +};
> +typedef struct xen_hvm_inject_trap xen_hvm_inject_trap_t;
> +DEFINE_XEN_GUEST_HANDLE(xen_hvm_inject_trap_t);
> +
> #endif /* __XEN_PUBLIC_HVM_HVM_OP_H__ */
> diff -r 3500724e8052 -r c1866aff7a5f xen/include/public/hvm/params.h
> --- a/xen/include/public/hvm/params.h Tue Jan 04 12:34:06 2011 -0800
> +++ b/xen/include/public/hvm/params.h Tue Jan 04 12:35:49 2011 -0800
> @@ -124,6 +124,19 @@
> */
> #define HVM_PARAM_ACPI_IOPORTS_LOCATION 19
>
> -#define HVM_NR_PARAMS 20
> +/* Enable blocking memory events, async or sync (pause vcpu until
response)
> + * onchangeonly indicates messages only on a change of value */
> +#define HVM_PARAM_MEMORY_EVENT_CR0 20
> +#define HVM_PARAM_MEMORY_EVENT_CR3 21
> +#define HVM_PARAM_MEMORY_EVENT_CR4 22
> +#define HVM_PARAM_MEMORY_EVENT_INT3 23
> +
> +#define HVMPME_MODE_MASK (3 << 0)
> +#define HVMPME_mode_disabled 0
> +#define HVMPME_mode_async 1
> +#define HVMPME_mode_sync 2
> +#define HVMPME_onchangeonly (1 << 2)
> +
> +#define HVM_NR_PARAMS 24
>
> #endif /* __XEN_PUBLIC_HVM_PARAMS_H__ */
> diff -r 3500724e8052 -r c1866aff7a5f xen/include/public/mem_event.h
> --- a/xen/include/public/mem_event.h Tue Jan 04 12:34:06 2011 -0800
> +++ b/xen/include/public/mem_event.h Tue Jan 04 12:35:49 2011 -0800
> @@ -37,6 +37,10 @@
> /* Reasons for the memory event request */
> #define MEM_EVENT_REASON_UNKNOWN 0 /* typical reason */
> #define MEM_EVENT_REASON_VIOLATION 1 /* access violation, GFN is
> address */
> +#define MEM_EVENT_REASON_CR0 2 /* CR0 was hit: gfn is CR0 value
*/
> +#define MEM_EVENT_REASON_CR3 3 /* CR3 was hit: gfn is CR3 value
*/
> +#define MEM_EVENT_REASON_CR4 4 /* CR4 was hit: gfn is CR4 value
*/
> +#define MEM_EVENT_REASON_INT3 5 /* int3 was hit: gla/gfn are RIP
*/
>
> typedef struct mem_event_shared_page {
> uint32_t port;
> diff -r 3500724e8052 -r c1866aff7a5f xen/arch/x86/hvm/hvm.c
> --- a/xen/arch/x86/hvm/hvm.c Tue Jan 04 12:34:06 2011 -0800
> +++ b/xen/arch/x86/hvm/hvm.c Tue Jan 04 12:35:49 2011 -0800
> @@ -309,6 +309,15 @@
> return; /* bail */
> }
> }
> +
> + /* Inject pending hw/sw trap */
> + if (v->arch.hvm_vcpu.inject_trap != -1)
> + {
> + hvm_inject_exception(v->arch.hvm_vcpu.inject_trap,
> + v->arch.hvm_vcpu.inject_error_code,
> + v->arch.hvm_vcpu.inject_cr2);
> + v->arch.hvm_vcpu.inject_trap = -1;
> + }
> }
>
> static void hvm_init_ioreq_page(
> @@ -949,6 +958,8 @@
> spin_lock_init(&v->arch.hvm_vcpu.tm_lock);
> INIT_LIST_HEAD(&v->arch.hvm_vcpu.tm_list);
>
> + v->arch.hvm_vcpu.inject_trap = -1;
> +
> #ifdef CONFIG_COMPAT
> rc = setup_compat_arg_xlat(v);
> if ( rc != 0 )
> @@ -3216,10 +3227,34 @@
> case HVM_PARAM_ACPI_IOPORTS_LOCATION:
> rc = pmtimer_change_ioport(d, a.value);
> break;
> + case HVM_PARAM_MEMORY_EVENT_INT3:
> + if ( a.value & HVMPME_onchangeonly )
> + rc = -EINVAL;
> + break;
> }
>
> - if ( rc == 0 )
> + if ( rc == 0 )
> + {
> d->arch.hvm_domain.params[a.index] = a.value;
> +
> + switch( a.index )
> + {
> + case HVM_PARAM_MEMORY_EVENT_INT3:
> + {
> + domain_pause(d);
> + domain_unpause(d); /* Causes guest to latch new status
*/
> + break;
> + }
> + case HVM_PARAM_MEMORY_EVENT_CR3:
> + {
> + for_each_vcpu ( d, v )
> + hvm_funcs.update_guest_cr(v, 0); /* Latches
> new CR3 mask through CR0 code */
> + break;
> + }
> + }
> +
> + }
> +
> }
> else
> {
> @@ -3631,6 +3666,41 @@
> break;
> }
>
> + case HVMOP_inject_trap:
> + {
> + xen_hvm_inject_trap_t tr;
> + struct domain *d;
> + struct vcpu *v;
> +
> + if ( copy_from_guest(&tr, arg, 1 ) )
> + return -EFAULT;
> +
> + rc = rcu_lock_target_domain_by_id(tr.domid, &d);
> + if ( rc != 0 )
> + return rc;
> +
> + rc = -EINVAL;
> + if ( !is_hvm_domain(d) )
> + goto param_fail8;
> +
> + rc = -ENOENT;
> + if ( tr.vcpuid >= d->max_vcpus || (v =
d->vcpu[tr.vcpuid]) == NULL )
> + goto param_fail8;
> +
> + if ( v->arch.hvm_vcpu.inject_trap != -1 )
> + rc = -EBUSY;
> + else
> + {
> + v->arch.hvm_vcpu.inject_trap = tr.trap;
> + v->arch.hvm_vcpu.inject_error_code = tr.error_code;
> + v->arch.hvm_vcpu.inject_cr2 = tr.cr2;
> + }
> +
> + param_fail8:
> + rcu_unlock_domain(d);
> + break;
> + }
> +
> default:
> {
> gdprintk(XENLOG_WARNING, "Bad HVM op %ld.\n", op);
> @@ -3671,6 +3741,161 @@
> return rc;
> }
>
> +int hvm_memory_event_cr0(unsigned long value, unsigned long old) {
> + struct vcpu* v = current;
> + struct domain *d = v->domain;
> + mem_event_request_t req;
> +
> + long p = d->arch.hvm_domain.params[HVM_PARAM_MEMORY_EVENT_CR0];
> +
> + if ( p & HVMPME_MODE_MASK )
> + {
> + int rc;
> +
> + if ( (p & HVMPME_onchangeonly) && value == old )
> + return 1;
> +
> + rc = mem_event_check_ring(d);
> + if ( rc )
> + return rc;
> +
> + memset(&req, 0, sizeof(req));
> + req.type = MEM_EVENT_TYPE_ACCESS;
> + req.reason = MEM_EVENT_REASON_CR0;
> +
> + if ( (p & HVMPME_MODE_MASK) == HVMPME_mode_sync )
> + {
> + req.flags |= MEM_EVENT_FLAG_VCPU_PAUSED;
> + vcpu_pause_nosync(v);
> + }
> +
> + req.gfn = value;
> + req.vcpu_id = v->vcpu_id;
> +
> + mem_event_put_request(d, &req);
> +
> + return 1;
> + }
> + return 0;
> +}
> +
> +int hvm_memory_event_cr3(unsigned long value, unsigned long old) {
> + struct vcpu* v = current;
> + struct domain *d = v->domain;
> + mem_event_request_t req;
> +
> + long p = d->arch.hvm_domain.params[HVM_PARAM_MEMORY_EVENT_CR3];
> +
> + if ( p & HVMPME_MODE_MASK )
> + {
> + int rc;
> +
> + if ( (p & HVMPME_onchangeonly) && value == old )
> + return 1;
> +
> + rc = mem_event_check_ring(d);
> + if ( rc )
> + return rc;
> +
> + memset(&req, 0, sizeof(req));
> + req.type = MEM_EVENT_TYPE_ACCESS;
> + req.reason = MEM_EVENT_REASON_CR3;
> +
> + if ( (p & HVMPME_MODE_MASK) == HVMPME_mode_sync )
> + {
> + req.flags |= MEM_EVENT_FLAG_VCPU_PAUSED;
> + vcpu_pause_nosync(v);
> + }
> +
> + req.gfn = value;
> + req.vcpu_id = v->vcpu_id;
> +
> + mem_event_put_request(d, &req);
> +
> + return 1;
> + }
> + return 0;
> +}
> +
> +int hvm_memory_event_cr4(unsigned long value, unsigned long old) {
> + struct vcpu* v = current;
> + struct domain *d = v->domain;
> + mem_event_request_t req;
> +
> + long p = d->arch.hvm_domain.params[HVM_PARAM_MEMORY_EVENT_CR4];
> +
> + if ( p & HVMPME_MODE_MASK )
> + {
> + int rc;
> +
> + if ( (p & HVMPME_onchangeonly) && value == old )
> + return 1;
> +
> + rc = mem_event_check_ring(d);
> + if ( rc )
> + return rc;
> +
> + memset(&req, 0, sizeof(req));
> + req.type = MEM_EVENT_TYPE_ACCESS;
> + req.reason = MEM_EVENT_REASON_CR4;
> +
> + if ( (p & HVMPME_MODE_MASK) == HVMPME_mode_sync )
> + {
> + req.flags |= MEM_EVENT_FLAG_VCPU_PAUSED;
> + vcpu_pause_nosync(v);
> + }
> +
> + req.gfn = value;
> + req.vcpu_id = v->vcpu_id;
> +
> + mem_event_put_request(d, &req);
> +
> + return 1;
> + }
> + return 0;
> +}
> +
> +int hvm_memory_event_int3(unsigned long gla) {
> + struct vcpu* v = current;
> + struct domain *d = v->domain;
> + mem_event_request_t req;
> +
> + long p = d->arch.hvm_domain.params[HVM_PARAM_MEMORY_EVENT_INT3];
> +
> + if ( p & HVMPME_MODE_MASK )
> + {
> + uint32_t pfec = PFEC_page_present;
> + unsigned long gfn;
> + int rc;
> +
> + rc = mem_event_check_ring(d);
> + if ( rc )
> + return rc;
> +
> + gfn = paging_gva_to_gfn(current, gla, &pfec);
> +
> + memset(&req, 0, sizeof(req));
> + req.type = MEM_EVENT_TYPE_ACCESS;
> + req.reason = MEM_EVENT_REASON_INT3;
> +
> + if ( (p & HVMPME_MODE_MASK) == HVMPME_mode_sync )
> + {
> + req.flags |= MEM_EVENT_FLAG_VCPU_PAUSED;
> + vcpu_pause_nosync(v);
> + }
> +
> + req.gfn = gfn;
> + req.offset = gla & ((1 << PAGE_SHIFT) - 1);
> + req.gla = gla;
> + req.gla_valid = 1;
> + req.vcpu_id = v->vcpu_id;
> +
> + mem_event_put_request(d, &req);
> +
> + return 1;
> + }
> + return 0;
> +}
>
> /*
> * Local variables:
> diff -r 3500724e8052 -r c1866aff7a5f xen/arch/x86/hvm/vmx/vmcs.c
> --- a/xen/arch/x86/hvm/vmx/vmcs.c Tue Jan 04 12:34:06 2011 -0800
> +++ b/xen/arch/x86/hvm/vmx/vmcs.c Tue Jan 04 12:35:49 2011 -0800
> @@ -1082,7 +1082,9 @@
> hvm_asid_flush_vcpu(v);
> }
>
> - debug_state = v->domain->debugger_attached;
> + debug_state = v->domain->debugger_attached
> + ||
> v->domain->arch.hvm_domain.params[HVM_PARAM_MEMORY_EVENT_INT3];
> +
> if ( unlikely(v->arch.hvm_vcpu.debug_state_latch != debug_state) )
> {
> v->arch.hvm_vcpu.debug_state_latch = debug_state;
> diff -r 3500724e8052 -r c1866aff7a5f xen/arch/x86/hvm/vmx/vmx.c
> --- a/xen/arch/x86/hvm/vmx/vmx.c Tue Jan 04 12:34:06 2011 -0800
> +++ b/xen/arch/x86/hvm/vmx/vmx.c Tue Jan 04 12:35:49 2011 -0800
> @@ -1064,12 +1064,16 @@
>
> if ( paging_mode_hap(v->domain) )
> {
> - /* We manage GUEST_CR3 when guest CR0.PE is zero. */
> + /* We manage GUEST_CR3 when guest CR0.PE is zero or when
> cr3 memevents are on */
> uint32_t cr3_ctls = (CPU_BASED_CR3_LOAD_EXITING |
> CPU_BASED_CR3_STORE_EXITING);
> v->arch.hvm_vmx.exec_control &= ~cr3_ctls;
> if ( !hvm_paging_enabled(v) )
> v->arch.hvm_vmx.exec_control |= cr3_ctls;
> +
> + if (
> v->domain->arch.hvm_domain.params[HVM_PARAM_MEMORY_EVENT_CR3] )
> + v->arch.hvm_vmx.exec_control |=
CPU_BASED_CR3_LOAD_EXITING;
> +
> vmx_update_cpu_exec_control(v);
>
> /* Changing CR0.PE can change some bits in real CR4. */
> @@ -1252,9 +1256,12 @@
> unsigned long intr_info = __vmread(VM_ENTRY_INTR_INFO);
> struct vcpu *curr = current;
>
> + int type = X86_EVENTTYPE_HW_EXCEPTION;
> +
> switch ( trap )
> {
> case TRAP_debug:
> + type = X86_EVENTTYPE_SW_EXCEPTION;
> if ( guest_cpu_user_regs()->eflags & X86_EFLAGS_TF )
> {
> __restore_debug_registers(curr);
> @@ -1269,6 +1276,9 @@
> domain_pause_for_debugger();
> return;
> }
> +
> + type = X86_EVENTTYPE_SW_EXCEPTION;
> + __vmwrite(VM_ENTRY_INSTRUCTION_LEN, 1); /* int3 */
> }
>
> if ( unlikely(intr_info & INTR_INFO_VALID_MASK) &&
> @@ -1279,7 +1289,7 @@
> error_code = 0;
> }
>
> - __vmx_inject_exception(trap, X86_EVENTTYPE_HW_EXCEPTION, error_code);
> + __vmx_inject_exception(trap, type, error_code);
>
> if ( trap == TRAP_page_fault )
> HVMTRACE_LONG_2D(PF_INJECT, error_code,
> @@ -1565,6 +1575,8 @@
> unsigned long value;
> struct vcpu *v = current;
> struct vlapic *vlapic = vcpu_vlapic(v);
> + int rc = 0;
> + unsigned long old;
>
> switch ( gp )
> {
> @@ -1589,13 +1601,25 @@
> switch ( cr )
> {
> case 0:
> - return !hvm_set_cr0(value);
> + old = v->arch.hvm_vcpu.guest_cr[0];
> + rc = !hvm_set_cr0(value);
> + if (rc)
> + hvm_memory_event_cr0(value, old);
> + return rc;
>
> case 3:
> - return !hvm_set_cr3(value);
> + old = v->arch.hvm_vcpu.guest_cr[3];
> + rc = !hvm_set_cr3(value);
> + if (rc)
> + hvm_memory_event_cr3(value, old);
> + return rc;
>
> case 4:
> - return !hvm_set_cr4(value);
> + old = v->arch.hvm_vcpu.guest_cr[4];
> + rc = !hvm_set_cr4(value);
> + if (rc)
> + hvm_memory_event_cr4(value, old);
> + return rc;
>
> case 8:
> vlapic_set_reg(vlapic, APIC_TASKPRI, ((value & 0x0F) <<
4));
> @@ -1676,11 +1700,17 @@
> cr = exit_qualification & VMX_CONTROL_REG_ACCESS_NUM;
> mov_from_cr(cr, gp, regs);
> break;
> - case VMX_CONTROL_REG_ACCESS_TYPE_CLTS:
> + case VMX_CONTROL_REG_ACCESS_TYPE_CLTS:
> + {
> + unsigned long old = v->arch.hvm_vcpu.guest_cr[0];
> v->arch.hvm_vcpu.guest_cr[0] &= ~X86_CR0_TS;
> vmx_update_guest_cr(v, 0);
> +
> + hvm_memory_event_cr0(v->arch.hvm_vcpu.guest_cr[0], old);
> +
> HVMTRACE_0D(CLTS);
> break;
> + }
> case VMX_CONTROL_REG_ACCESS_TYPE_LMSW:
> value = v->arch.hvm_vcpu.guest_cr[0];
> /* LMSW can: (1) set bits 0-3; (2) clear bits 1-3. */
> @@ -2350,13 +2380,29 @@
> goto exit_and_crash;
> domain_pause_for_debugger();
> break;
> - case TRAP_int3:
> - if ( !v->domain->debugger_attached )
> - goto exit_and_crash;
> - update_guest_eip(); /* Safe: INT3 */
> - current->arch.gdbsx_vcpu_event = TRAP_int3;
> - domain_pause_for_debugger();
> - break;
> + case TRAP_int3:
> + {
> + if ( v->domain->debugger_attached )
> + {
> + update_guest_eip(); /* Safe: INT3 */
> + current->arch.gdbsx_vcpu_event = TRAP_int3;
> + domain_pause_for_debugger();
> + break;
> + }
> + else {
> + int handled = hvm_memory_event_int3(regs->eip);
> +
> + if ( handled < 0 )
> + {
> + vmx_inject_exception(TRAP_int3,
> HVM_DELIVER_NO_ERROR_CODE, 0);
> + break;
> + }
> + else if ( handled )
> + break;
> + }
> +
> + goto exit_and_crash;
> + }
> case TRAP_no_device:
> vmx_fpu_dirty_intercept();
> break;
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Tim Deegan
2011-Jan-05 14:39 UTC
Re: [Xen-devel] [PATCH 5 of 6] REDO: mem_access & mem_access 2: added INT3/CRx capture
Hi, At 22:07 +0000 on 04 Jan (1294178845), Joe Epstein wrote:> diff -r 3500724e8052 -r c1866aff7a5f xen/arch/x86/hvm/hvm.c > --- a/xen/arch/x86/hvm/hvm.c Tue Jan 04 12:34:06 2011 -0800 > +++ b/xen/arch/x86/hvm/hvm.c Tue Jan 04 12:35:49 2011 -0800 > @@ -309,6 +309,15 @@ > return; /* bail */ > } > } > + > + /* Inject pending hw/sw trap */ > + if (v->arch.hvm_vcpu.inject_trap != -1) > + { > + hvm_inject_exception(v->arch.hvm_vcpu.inject_trap, > + v->arch.hvm_vcpu.inject_error_code, > + v->arch.hvm_vcpu.inject_cr2); > + v->arch.hvm_vcpu.inject_trap = -1; > + }What if the guest already has a trap pending for some reason? This could turn it into a double-fault, which is probably not what the caller wanted.> } > > static void hvm_init_ioreq_page( > @@ -949,6 +958,8 @@ > spin_lock_init(&v->arch.hvm_vcpu.tm_lock); > INIT_LIST_HEAD(&v->arch.hvm_vcpu.tm_list); > > + v->arch.hvm_vcpu.inject_trap = -1; > + > #ifdef CONFIG_COMPAT > rc = setup_compat_arg_xlat(v); > if ( rc != 0 ) > @@ -3216,10 +3227,34 @@ > case HVM_PARAM_ACPI_IOPORTS_LOCATION: > rc = pmtimer_change_ioport(d, a.value); > break; > + case HVM_PARAM_MEMORY_EVENT_INT3: > + if ( a.value & HVMPME_onchangeonly ) > + rc = -EINVAL; > + break; > } > > - if ( rc == 0 ) > + if ( rc == 0 ) > + { > d->arch.hvm_domain.params[a.index] = a.value; > + > + switch( a.index ) > + { > + case HVM_PARAM_MEMORY_EVENT_INT3: > + { > + domain_pause(d);You need to make sure a VM can''t set this param on itself, or else this domain_pause() can deadlock Xen. Also, you probably don''t want the daomin turning these intercepts off anyway. :) Now that I think of it, it''s worth checking your other hypercalls to check that a VM can''t change its own p2m mapping types. [...]> @@ -1589,13 +1601,25 @@ > switch ( cr ) > { > case 0: > - return !hvm_set_cr0(value); > + old = v->arch.hvm_vcpu.guest_cr[0]; > + rc = !hvm_set_cr0(value); > + if (rc) > + hvm_memory_event_cr0(value, old);You defined the hvm_memory_event_foo() functions to return error codes but you don''t handle them in the caller. Please either handle them or arrange for the functions to return void. Cheers, Tim. -- Tim Deegan <Tim.Deegan@citrix.com> Principal Software Engineer, Xen Platform Team Citrix Systems UK Ltd. (Company #02937203, SL9 0BG) _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Joe Epstein
2011-Jan-05 15:18 UTC
Re: [Xen-devel] [PATCH 5 of 6] REDO: mem_access & mem_access 2: added INT3/CRx capture
One question in line. Thanks On Wed, Jan 5, 2011 at 6:39 AM, Tim Deegan <Tim.Deegan@citrix.com> wrote:> Hi, > > At 22:07 +0000 on 04 Jan (1294178845), Joe Epstein wrote: > > diff -r 3500724e8052 -r c1866aff7a5f xen/arch/x86/hvm/hvm.c > > --- a/xen/arch/x86/hvm/hvm.c Tue Jan 04 12:34:06 2011 -0800 > > +++ b/xen/arch/x86/hvm/hvm.c Tue Jan 04 12:35:49 2011 -0800 > > @@ -309,6 +309,15 @@ > > return; /* bail */ > > } > > } > > + > > + /* Inject pending hw/sw trap */ > > + if (v->arch.hvm_vcpu.inject_trap != -1) > > + { > > + hvm_inject_exception(v->arch.hvm_vcpu.inject_trap, > > + v->arch.hvm_vcpu.inject_error_code, > > + v->arch.hvm_vcpu.inject_cr2); > > + v->arch.hvm_vcpu.inject_trap = -1; > > + } > > What if the guest already has a trap pending for some reason? This > could turn it into a double-fault, which is probably not what the caller > wanted. >Hmm...it doesn''t seem like there can be two different injected events, as this will overwrite whatever else was set. If the guest was trying to service a trap and took a VM exit, then injection should be the same as if it happened natively. So I think I''m missing something here. Also, because the injection only takes effect on the next entry, the odds are good that the caller knows the VCPU was paused, and might have a better sense of the state. The hypercall is most useful on a memory event handler registered for INT3, having to inject the INT3 to whatever debugger is running in the guest that needs to handle it.> > } > > > > static void hvm_init_ioreq_page( > > @@ -949,6 +958,8 @@ > > spin_lock_init(&v->arch.hvm_vcpu.tm_lock); > > INIT_LIST_HEAD(&v->arch.hvm_vcpu.tm_list); > > > > + v->arch.hvm_vcpu.inject_trap = -1; > > + > > #ifdef CONFIG_COMPAT > > rc = setup_compat_arg_xlat(v); > > if ( rc != 0 ) > > @@ -3216,10 +3227,34 @@ > > case HVM_PARAM_ACPI_IOPORTS_LOCATION: > > rc = pmtimer_change_ioport(d, a.value); > > break; > > + case HVM_PARAM_MEMORY_EVENT_INT3: > > + if ( a.value & HVMPME_onchangeonly ) > > + rc = -EINVAL; > > + break; > > } > > > > - if ( rc == 0 ) > > + if ( rc == 0 ) > > + { > > d->arch.hvm_domain.params[a.index] = a.value; > > + > > + switch( a.index ) > > + { > > + case HVM_PARAM_MEMORY_EVENT_INT3: > > + { > > + domain_pause(d); > > You need to make sure a VM can''t set this param on itself, or else this > domain_pause() can deadlock Xen. Also, you probably don''t want the > daomin turning these intercepts off anyway. :) > > Now that I think of it, it''s worth checking your other hypercalls to > check that a VM can''t change its own p2m mapping types. > > [...] > > > @@ -1589,13 +1601,25 @@ > > switch ( cr ) > > { > > case 0: > > - return !hvm_set_cr0(value); > > + old = v->arch.hvm_vcpu.guest_cr[0]; > > + rc = !hvm_set_cr0(value); > > + if (rc) > > + hvm_memory_event_cr0(value, old); > > You defined the hvm_memory_event_foo() functions to return error codes > but you don''t handle them in the caller. Please either handle them or > arrange for the functions to return void. > > Cheers, > > Tim. > > -- > Tim Deegan <Tim.Deegan@citrix.com> > Principal Software Engineer, Xen Platform Team > Citrix Systems UK Ltd. (Company #02937203, SL9 0BG) >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Tim Deegan
2011-Jan-05 15:55 UTC
Re: [Xen-devel] [PATCH 5 of 6] REDO: mem_access & mem_access 2: added INT3/CRx capture
At 15:18 +0000 on 05 Jan (1294240690), Joe Epstein wrote:>> What if the guest already has a trap pending for some reason? This >> could turn it into a double-fault, which is probably not what the caller >> wanted. > > Hmm...it doesn''t seem like there can be two different injected events, > as this will overwrite whatever else was set. If the guest was trying > to service a trap and took a VM exit, then injection should be the > same as if it happened natively. So I think I''m missing something > here. > > Also, because the injection only takes effect on the next entry, the > odds are good that the caller knows the VCPU was paused, and might > have a better sense of the state. The hypercall is most useful on a > memory event handler registered for INT3, having to inject the INT3 to > whatever debugger is running in the guest that needs to handle it.Yes, I can see why it''s useful - just as long as the caller knows that there''s a risk (in some circumstances) that the injected trap will collide with another one and cause a double fault. In the INT3 case, the caller can be sure that it''s the only injection going on, but that''s not always the case (e.g. if the guest trapped on a write to its stack during a pagefault). Cheers, Tim. -- Tim Deegan <Tim.Deegan@citrix.com> Principal Software Engineer, Xen Platform Team Citrix Systems UK Ltd. (Company #02937203, SL9 0BG) _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel