* [PATCH] xen/vm_event: Clean up control-register-write vm_events
@ 2015-05-12 14:58 Razvan Cojocaru
2015-05-12 15:35 ` Jan Beulich
0 siblings, 1 reply; 9+ messages in thread
From: Razvan Cojocaru @ 2015-05-12 14:58 UTC (permalink / raw)
To: xen-devel
Cc: kevin.tian, wei.liu2, ian.campbell, Razvan Cojocaru,
stefano.stabellini, jun.nakajima, ian.jackson, tim, eddie.dong,
jbeulich, andrew.cooper3, keir
As suggested by Andrew Cooper, this patch attempts to remove
some redundancy and allow for an easier time when adding vm_events
for new control registers in the future, by having a single
VM_EVENT_REASON_MOV_TO_CR vm_event type, meant to serve CR0,
CR3 and CR4. The actual control register will be deduced by
the new .index field in vm_event_write_ctrlreg (renamed from
vm_event_mov_to_cr). The patch has also modified the xen-access.c
test - it is now able to log CR3 events.
Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
---
tools/libxc/include/xenctrl.h | 8 ++---
tools/libxc/xc_monitor.c | 39 +++--------------------
tools/tests/xen-access/xen-access.c | 30 ++++++++++++++++--
xen/arch/x86/hvm/event.c | 50 +++++++----------------------
xen/arch/x86/hvm/hvm.c | 6 ++--
xen/arch/x86/hvm/vmx/vmx.c | 4 +--
xen/arch/x86/monitor.c | 60 ++++++++++++-----------------------
xen/include/asm-x86/domain.h | 20 ++++--------
xen/include/asm-x86/hvm/event.h | 4 +--
xen/include/public/domctl.h | 12 +++----
xen/include/public/vm_event.h | 27 +++++++++-------
11 files changed, 100 insertions(+), 160 deletions(-)
diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index a689caf..7025027 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -2355,12 +2355,8 @@ int xc_mem_access_disable_emulate(xc_interface *xch, domid_t domain_id);
void *xc_monitor_enable(xc_interface *xch, domid_t domain_id, uint32_t *port);
int xc_monitor_disable(xc_interface *xch, domid_t domain_id);
int xc_monitor_resume(xc_interface *xch, domid_t domain_id);
-int xc_monitor_mov_to_cr0(xc_interface *xch, domid_t domain_id, bool enable,
- bool sync, bool onchangeonly);
-int xc_monitor_mov_to_cr3(xc_interface *xch, domid_t domain_id, bool enable,
- bool sync, bool onchangeonly);
-int xc_monitor_mov_to_cr4(xc_interface *xch, domid_t domain_id, bool enable,
- bool sync, bool onchangeonly);
+int xc_monitor_mov_to_cr(xc_interface *xch, domid_t domain_id, uint16_t index,
+ bool enable, bool sync, bool onchangeonly);
int xc_monitor_mov_to_msr(xc_interface *xch, domid_t domain_id, bool enable,
bool extended_capture);
int xc_monitor_singlestep(xc_interface *xch, domid_t domain_id, bool enable);
diff --git a/tools/libxc/xc_monitor.c b/tools/libxc/xc_monitor.c
index 87ad968..8aba93f 100644
--- a/tools/libxc/xc_monitor.c
+++ b/tools/libxc/xc_monitor.c
@@ -45,8 +45,8 @@ int xc_monitor_resume(xc_interface *xch, domid_t domain_id)
NULL);
}
-int xc_monitor_mov_to_cr0(xc_interface *xch, domid_t domain_id, bool enable,
- bool sync, bool onchangeonly)
+int xc_monitor_mov_to_cr(xc_interface *xch, domid_t domain_id, uint16_t index,
+ bool enable, bool sync, bool onchangeonly)
{
DECLARE_DOMCTL;
@@ -54,39 +54,8 @@ int xc_monitor_mov_to_cr0(xc_interface *xch, domid_t domain_id, bool enable,
domctl.domain = domain_id;
domctl.u.monitor_op.op = enable ? XEN_DOMCTL_MONITOR_OP_ENABLE
: XEN_DOMCTL_MONITOR_OP_DISABLE;
- domctl.u.monitor_op.event = XEN_DOMCTL_MONITOR_EVENT_MOV_TO_CR0;
- domctl.u.monitor_op.u.mov_to_cr.sync = sync;
- domctl.u.monitor_op.u.mov_to_cr.onchangeonly = onchangeonly;
-
- return do_domctl(xch, &domctl);
-}
-
-int xc_monitor_mov_to_cr3(xc_interface *xch, domid_t domain_id, bool enable,
- bool sync, bool onchangeonly)
-{
- DECLARE_DOMCTL;
-
- domctl.cmd = XEN_DOMCTL_monitor_op;
- domctl.domain = domain_id;
- domctl.u.monitor_op.op = enable ? XEN_DOMCTL_MONITOR_OP_ENABLE
- : XEN_DOMCTL_MONITOR_OP_DISABLE;
- domctl.u.monitor_op.event = XEN_DOMCTL_MONITOR_EVENT_MOV_TO_CR3;
- domctl.u.monitor_op.u.mov_to_cr.sync = sync;
- domctl.u.monitor_op.u.mov_to_cr.onchangeonly = onchangeonly;
-
- return do_domctl(xch, &domctl);
-}
-
-int xc_monitor_mov_to_cr4(xc_interface *xch, domid_t domain_id, bool enable,
- bool sync, bool onchangeonly)
-{
- DECLARE_DOMCTL;
-
- domctl.cmd = XEN_DOMCTL_monitor_op;
- domctl.domain = domain_id;
- domctl.u.monitor_op.op = enable ? XEN_DOMCTL_MONITOR_OP_ENABLE
- : XEN_DOMCTL_MONITOR_OP_DISABLE;
- domctl.u.monitor_op.event = XEN_DOMCTL_MONITOR_EVENT_MOV_TO_CR4;
+ domctl.u.monitor_op.event = XEN_DOMCTL_MONITOR_EVENT_MOV_TO_CR;
+ domctl.u.monitor_op.u.mov_to_cr.index = index;
domctl.u.monitor_op.u.mov_to_cr.sync = sync;
domctl.u.monitor_op.u.mov_to_cr.onchangeonly = onchangeonly;
diff --git a/tools/tests/xen-access/xen-access.c b/tools/tests/xen-access/xen-access.c
index 12ab921..c0b474b 100644
--- a/tools/tests/xen-access/xen-access.c
+++ b/tools/tests/xen-access/xen-access.c
@@ -318,9 +318,9 @@ static void put_response(vm_event_t *vm_event, vm_event_response_t *rsp)
void usage(char* progname)
{
fprintf(stderr,
- "Usage: %s [-m] <domain_id> write|exec|breakpoint\n"
+ "Usage: %s [-m] <domain_id> write|exec|breakpoint|cr3\n"
"\n"
- "Logs first page writes, execs, or breakpoint traps that occur on the domain.\n"
+ "Logs first page writes, execs, CR3 writes or breakpoint traps that occur on the domain.\n"
"\n"
"-m requires this program to run, or else the domain may pause\n",
progname);
@@ -341,6 +341,7 @@ int main(int argc, char *argv[])
int required = 0;
int breakpoint = 0;
int shutting_down = 0;
+ int cr3 = 0;
char* progname = argv[0];
argv++;
@@ -383,6 +384,10 @@ int main(int argc, char *argv[])
{
breakpoint = 1;
}
+ else if ( !strcmp(argv[0], "cr3") )
+ {
+ cr3 = 1;
+ }
else
{
usage(argv[0]);
@@ -443,6 +448,16 @@ int main(int argc, char *argv[])
}
}
+ if ( cr3 )
+ {
+ rc = xc_monitor_mov_to_cr(xch, domain_id, X86_CR3, 1, 1, 1);
+ if ( rc < 0 )
+ {
+ ERROR("Error %d requesting CR3 monitoring with vm_event\n", rc);
+ goto exit;
+ }
+ }
+
/* Wait for access */
for (;;)
{
@@ -455,6 +470,7 @@ int main(int argc, char *argv[])
rc = xc_set_mem_access(xch, domain_id, XENMEM_access_rwx, START_PFN,
(xenaccess->max_gpfn - START_PFN) );
rc = xc_monitor_software_breakpoint(xch, domain_id, 0);
+ rc = xc_monitor_mov_to_cr(xch, domain_id, X86_CR3, 0, 0, 0);
shutting_down = 1;
}
@@ -546,6 +562,16 @@ int main(int argc, char *argv[])
}
break;
+ case VM_EVENT_REASON_MOV_TO_CR:
+ if ( req.u.write_ctrlreg.index == X86_CR3 )
+ printf("CR3: old 0x%016lx new 0x%016lx\n",
+ req.u.write_ctrlreg.new_value,
+ req.u.write_ctrlreg.old_value);
+ else
+ printf("Non-CR3 control register write event: 0x%08lx\n",
+ req.u.write_ctrlreg.index);
+
+ break;
default:
fprintf(stderr, "UNKNOWN REASON CODE %d\n", req.reason);
}
diff --git a/xen/arch/x86/hvm/event.c b/xen/arch/x86/hvm/event.c
index 9d5f9f3..7565aa1 100644
--- a/xen/arch/x86/hvm/event.c
+++ b/xen/arch/x86/hvm/event.c
@@ -88,55 +88,29 @@ static int hvm_event_traps(uint8_t sync, vm_event_request_t *req)
return 1;
}
-static inline
-void hvm_event_cr(uint32_t reason, unsigned long value,
- unsigned long old, bool_t onchangeonly, bool_t sync)
+void hvm_event_cr(unsigned short index, unsigned long value, unsigned long old)
{
- if ( onchangeonly && value == old )
+ struct arch_domain *currad = ¤t->domain->arch;
+
+ if ( !(currad->monitor.write_ctrlreg_enabled & index) )
+ return;
+
+ if ( (currad->monitor.write_ctrlreg_onchangeonly & index) && value == old )
return;
else
{
vm_event_request_t req = {
- .reason = reason,
+ .reason = VM_EVENT_REASON_MOV_TO_CR,
.vcpu_id = current->vcpu_id,
- .u.mov_to_cr.new_value = value,
- .u.mov_to_cr.old_value = old
+ .u.write_ctrlreg.index = index,
+ .u.write_ctrlreg.new_value = value,
+ .u.write_ctrlreg.old_value = old
};
- hvm_event_traps(sync, &req);
+ hvm_event_traps(currad->monitor.write_ctrlreg_sync & index, &req);
}
}
-void hvm_event_cr0(unsigned long value, unsigned long old)
-{
- struct arch_domain *currad = ¤t->domain->arch;
-
- if ( currad->monitor.mov_to_cr0_enabled )
- hvm_event_cr(VM_EVENT_REASON_MOV_TO_CR0, value, old,
- currad->monitor.mov_to_cr0_onchangeonly,
- currad->monitor.mov_to_cr0_sync);
-}
-
-void hvm_event_cr3(unsigned long value, unsigned long old)
-{
- struct arch_domain *currad = ¤t->domain->arch;
-
- if ( currad->monitor.mov_to_cr3_enabled )
- hvm_event_cr(VM_EVENT_REASON_MOV_TO_CR3, value, old,
- currad->monitor.mov_to_cr3_onchangeonly,
- currad->monitor.mov_to_cr3_sync);
-}
-
-void hvm_event_cr4(unsigned long value, unsigned long old)
-{
- struct arch_domain *currad = ¤t->domain->arch;
-
- if ( currad->monitor.mov_to_cr4_enabled )
- hvm_event_cr(VM_EVENT_REASON_MOV_TO_CR4, value, old,
- currad->monitor.mov_to_cr4_onchangeonly,
- currad->monitor.mov_to_cr4_sync);
-}
-
void hvm_event_msr(unsigned int msr, uint64_t value)
{
struct vcpu *curr = current;
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 689e402..f1f20bc 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -3267,7 +3267,7 @@ int hvm_set_cr0(unsigned long value)
hvm_funcs.handle_cd(v, value);
hvm_update_cr(v, 0, value);
- hvm_event_cr0(value, old_value);
+ hvm_event_cr(X86_CR0, value, old_value);
if ( (value ^ old_value) & X86_CR0_PG ) {
if ( !nestedhvm_vmswitch_in_progress(v) && nestedhvm_vcpu_in_guestmode(v) )
@@ -3308,7 +3308,7 @@ int hvm_set_cr3(unsigned long value)
old=v->arch.hvm_vcpu.guest_cr[3];
v->arch.hvm_vcpu.guest_cr[3] = value;
paging_update_cr3(v);
- hvm_event_cr3(value, old);
+ hvm_event_cr(X86_CR3, value, old);
return X86EMUL_OKAY;
bad_cr3:
@@ -3349,7 +3349,7 @@ int hvm_set_cr4(unsigned long value)
}
hvm_update_cr(v, 4, value);
- hvm_event_cr4(value, old_cr);
+ hvm_event_cr(X86_CR4, value, old_cr);
/*
* Modifying CR4.{PSE,PAE,PGE,SMEP}, or clearing CR4.PCIDE
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 74f563f..56651ce 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -1262,7 +1262,7 @@ static void vmx_update_guest_cr(struct vcpu *v, unsigned int cr)
v->arch.hvm_vmx.exec_control |= cr3_ctls;
/* Trap CR3 updates if CR3 memory events are enabled. */
- if ( v->domain->arch.monitor.mov_to_cr3_enabled )
+ if ( v->domain->arch.monitor.write_ctrlreg_enabled & X86_CR3 )
v->arch.hvm_vmx.exec_control |= CPU_BASED_CR3_LOAD_EXITING;
vmx_update_cpu_exec_control(v);
@@ -2010,7 +2010,7 @@ static int vmx_cr_access(unsigned long exit_qualification)
unsigned long old = curr->arch.hvm_vcpu.guest_cr[0];
curr->arch.hvm_vcpu.guest_cr[0] &= ~X86_CR0_TS;
vmx_update_guest_cr(curr, 0);
- hvm_event_cr0(curr->arch.hvm_vcpu.guest_cr[0], old);
+ hvm_event_cr(X86_CR0, curr->arch.hvm_vcpu.guest_cr[0], old);
HVMTRACE_0D(CLTS);
break;
}
diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c
index d7b1c18..155ad14 100644
--- a/xen/arch/x86/monitor.c
+++ b/xen/arch/x86/monitor.c
@@ -67,59 +67,39 @@ int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *mop)
switch ( mop->event )
{
- case XEN_DOMCTL_MONITOR_EVENT_MOV_TO_CR0:
+ case XEN_DOMCTL_MONITOR_EVENT_MOV_TO_CR:
{
- bool_t status = ad->monitor.mov_to_cr0_enabled;
-
- rc = status_check(mop, status);
- if ( rc )
- return rc;
-
- ad->monitor.mov_to_cr0_sync = mop->u.mov_to_cr.sync;
- ad->monitor.mov_to_cr0_onchangeonly = mop->u.mov_to_cr.onchangeonly;
-
- domain_pause(d);
- ad->monitor.mov_to_cr0_enabled = !status;
- domain_unpause(d);
- break;
- }
-
- case XEN_DOMCTL_MONITOR_EVENT_MOV_TO_CR3:
- {
- bool_t status = ad->monitor.mov_to_cr3_enabled;
+ bool_t status = ad->monitor.write_ctrlreg_enabled & mop->u.mov_to_cr.index;
struct vcpu *v;
rc = status_check(mop, status);
if ( rc )
return rc;
- ad->monitor.mov_to_cr3_sync = mop->u.mov_to_cr.sync;
- ad->monitor.mov_to_cr3_onchangeonly = mop->u.mov_to_cr.onchangeonly;
+ if ( mop->u.mov_to_cr.sync )
+ ad->monitor.write_ctrlreg_sync |= mop->u.mov_to_cr.index;
+ else
+ ad->monitor.write_ctrlreg_sync &= ~mop->u.mov_to_cr.index;
- domain_pause(d);
- ad->monitor.mov_to_cr3_enabled = !status;
- domain_unpause(d);
+ if ( mop->u.mov_to_cr.onchangeonly )
+ ad->monitor.write_ctrlreg_onchangeonly |= mop->u.mov_to_cr.index;
+ else
+ ad->monitor.write_ctrlreg_onchangeonly &= mop->u.mov_to_cr.index;
- /* Latches new CR3 mask through CR0 code */
- for_each_vcpu ( d, v )
- hvm_funcs.update_guest_cr(v, 0);
- break;
- }
+ domain_pause(d);
- case XEN_DOMCTL_MONITOR_EVENT_MOV_TO_CR4:
- {
- bool_t status = ad->monitor.mov_to_cr4_enabled;
+ if ( !status )
+ ad->monitor.write_ctrlreg_enabled |= mop->u.mov_to_cr.index;
+ else
+ ad->monitor.write_ctrlreg_enabled &= ~mop->u.mov_to_cr.index;
- rc = status_check(mop, status);
- if ( rc )
- return rc;
+ domain_unpause(d);
- ad->monitor.mov_to_cr4_sync = mop->u.mov_to_cr.sync;
- ad->monitor.mov_to_cr4_onchangeonly = mop->u.mov_to_cr.onchangeonly;
+ if ( mop->u.mov_to_cr.index == X86_CR3 )
+ /* Latches new CR3 mask through CR0 code */
+ for_each_vcpu ( d, v )
+ hvm_funcs.update_guest_cr(v, 0);
- domain_pause(d);
- ad->monitor.mov_to_cr4_enabled = !status;
- domain_unpause(d);
break;
}
diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
index 45b5283..1dd49dd 100644
--- a/xen/include/asm-x86/domain.h
+++ b/xen/include/asm-x86/domain.h
@@ -341,19 +341,13 @@ struct arch_domain
/* Monitor options */
struct {
- uint16_t mov_to_cr0_enabled : 1;
- uint16_t mov_to_cr0_sync : 1;
- uint16_t mov_to_cr0_onchangeonly : 1;
- uint16_t mov_to_cr3_enabled : 1;
- uint16_t mov_to_cr3_sync : 1;
- uint16_t mov_to_cr3_onchangeonly : 1;
- uint16_t mov_to_cr4_enabled : 1;
- uint16_t mov_to_cr4_sync : 1;
- uint16_t mov_to_cr4_onchangeonly : 1;
- uint16_t mov_to_msr_enabled : 1;
- uint16_t mov_to_msr_extended : 1;
- uint16_t singlestep_enabled : 1;
- uint16_t software_breakpoint_enabled : 1;
+ uint32_t write_ctrlreg_enabled : 8;
+ uint32_t write_ctrlreg_sync : 8;
+ uint32_t write_ctrlreg_onchangeonly : 8;
+ uint32_t mov_to_msr_enabled : 1;
+ uint32_t mov_to_msr_extended : 1;
+ uint32_t singlestep_enabled : 1;
+ uint32_t software_breakpoint_enabled : 1;
} monitor;
/* Mem_access emulation control */
diff --git a/xen/include/asm-x86/hvm/event.h b/xen/include/asm-x86/hvm/event.h
index bb757a1..7d327f4 100644
--- a/xen/include/asm-x86/hvm/event.h
+++ b/xen/include/asm-x86/hvm/event.h
@@ -19,9 +19,7 @@
#define __ASM_X86_HVM_EVENT_H__
/* Called for current VCPU on crX/MSR changes by guest */
-void hvm_event_cr0(unsigned long value, unsigned long old);
-void hvm_event_cr3(unsigned long value, unsigned long old);
-void hvm_event_cr4(unsigned long value, unsigned long old);
+void hvm_event_cr(unsigned short index, unsigned long value, unsigned long old);
void hvm_event_msr(unsigned int msr, uint64_t value);
/* Called for current VCPU: returns -1 if no listener */
int hvm_event_int3(unsigned long gla);
diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index 0c0ea4a..5bdc56f 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -1034,12 +1034,10 @@ DEFINE_XEN_GUEST_HANDLE(xen_domctl_psr_cmt_op_t);
#define XEN_DOMCTL_MONITOR_OP_ENABLE 0
#define XEN_DOMCTL_MONITOR_OP_DISABLE 1
-#define XEN_DOMCTL_MONITOR_EVENT_MOV_TO_CR0 0
-#define XEN_DOMCTL_MONITOR_EVENT_MOV_TO_CR3 1
-#define XEN_DOMCTL_MONITOR_EVENT_MOV_TO_CR4 2
-#define XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR 3
-#define XEN_DOMCTL_MONITOR_EVENT_SINGLESTEP 4
-#define XEN_DOMCTL_MONITOR_EVENT_SOFTWARE_BREAKPOINT 5
+#define XEN_DOMCTL_MONITOR_EVENT_MOV_TO_CR 0
+#define XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR 1
+#define XEN_DOMCTL_MONITOR_EVENT_SINGLESTEP 2
+#define XEN_DOMCTL_MONITOR_EVENT_SOFTWARE_BREAKPOINT 3
struct xen_domctl_monitor_op {
uint32_t op; /* XEN_DOMCTL_MONITOR_OP_* */
@@ -1050,6 +1048,8 @@ struct xen_domctl_monitor_op {
*/
union {
struct {
+ /* Which control register */
+ uint8_t index;
/* Pause vCPU until response */
uint8_t sync;
/* Send event only on a change of value */
diff --git a/xen/include/public/vm_event.h b/xen/include/public/vm_event.h
index c7426de..3ae59be 100644
--- a/xen/include/public/vm_event.h
+++ b/xen/include/public/vm_event.h
@@ -60,22 +60,24 @@
#define VM_EVENT_REASON_MEM_SHARING 2
/* Memory paging event */
#define VM_EVENT_REASON_MEM_PAGING 3
-/* CR0 was updated */
-#define VM_EVENT_REASON_MOV_TO_CR0 4
-/* CR3 was updated */
-#define VM_EVENT_REASON_MOV_TO_CR3 5
-/* CR4 was updated */
-#define VM_EVENT_REASON_MOV_TO_CR4 6
+/* A control register was updated */
+#define VM_EVENT_REASON_MOV_TO_CR 4
/* An MSR was updated. */
-#define VM_EVENT_REASON_MOV_TO_MSR 7
+#define VM_EVENT_REASON_MOV_TO_MSR 5
/* Debug operation executed (e.g. int3) */
-#define VM_EVENT_REASON_SOFTWARE_BREAKPOINT 8
+#define VM_EVENT_REASON_SOFTWARE_BREAKPOINT 6
/* Single-step (e.g. MTF) */
-#define VM_EVENT_REASON_SINGLESTEP 9
+#define VM_EVENT_REASON_SINGLESTEP 7
+
+/* Supported values for the vm_event_write_ctrlreg index. */
+#define X86_CR0 (1 << 0)
+#define X86_CR3 (1 << 1)
+#define X86_CR4 (1 << 2)
+#define X86_XCR0 (1 << 3)
/*
* Using a custom struct (not hvm_hw_cpu) so as to not fill
- * the mem_event ring buffer too quickly.
+ * the vm_event ring buffer too quickly.
*/
struct vm_event_regs_x86 {
uint64_t rax;
@@ -156,7 +158,8 @@ struct vm_event_mem_access {
uint32_t _pad;
};
-struct vm_event_mov_to_cr {
+struct vm_event_write_ctrlreg {
+ uint64_t index;
uint64_t new_value;
uint64_t old_value;
};
@@ -196,7 +199,7 @@ typedef struct vm_event_st {
struct vm_event_paging mem_paging;
struct vm_event_sharing mem_sharing;
struct vm_event_mem_access mem_access;
- struct vm_event_mov_to_cr mov_to_cr;
+ struct vm_event_write_ctrlreg write_ctrlreg;
struct vm_event_mov_to_msr mov_to_msr;
struct vm_event_debug software_breakpoint;
struct vm_event_debug singlestep;
--
1.7.9.5
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH] xen/vm_event: Clean up control-register-write vm_events
2015-05-12 14:58 [PATCH] xen/vm_event: Clean up control-register-write vm_events Razvan Cojocaru
@ 2015-05-12 15:35 ` Jan Beulich
2015-05-12 15:49 ` Razvan Cojocaru
0 siblings, 1 reply; 9+ messages in thread
From: Jan Beulich @ 2015-05-12 15:35 UTC (permalink / raw)
To: Razvan Cojocaru
Cc: tim, kevin.tian, wei.liu2, eddie.dong, stefano.stabellini,
andrew.cooper3, ian.jackson, xen-devel, jun.nakajima, keir,
ian.campbell
>>> On 12.05.15 at 16:58, <rcojocaru@bitdefender.com> wrote:
> +/* Supported values for the vm_event_write_ctrlreg index. */
> +#define X86_CR0 (1 << 0)
> +#define X86_CR3 (1 << 1)
> +#define X86_CR4 (1 << 2)
> +#define X86_XCR0 (1 << 3)
These names, being put in the public interface, are way too generic.
Jan
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] xen/vm_event: Clean up control-register-write vm_events
2015-05-12 15:35 ` Jan Beulich
@ 2015-05-12 15:49 ` Razvan Cojocaru
2015-05-12 15:50 ` Andrew Cooper
0 siblings, 1 reply; 9+ messages in thread
From: Razvan Cojocaru @ 2015-05-12 15:49 UTC (permalink / raw)
To: Jan Beulich
Cc: kevin.tian, wei.liu2, eddie.dong, stefano.stabellini,
andrew.cooper3, tim, xen-devel, jun.nakajima, keir, ian.jackson,
ian.campbell
On 05/12/2015 06:35 PM, Jan Beulich wrote:
>>>> On 12.05.15 at 16:58, <rcojocaru@bitdefender.com> wrote:
>> +/* Supported values for the vm_event_write_ctrlreg index. */
>> +#define X86_CR0 (1 << 0)
>> +#define X86_CR3 (1 << 1)
>> +#define X86_CR4 (1 << 2)
>> +#define X86_XCR0 (1 << 3)
>
> These names, being put in the public interface, are way too generic.
I've copied them from Andrew Cooper's suggestion in the previous thread,
hopefully he'll chime in. I'm happy to rename / move them if so desired.
Thanks,
Razvan
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] xen/vm_event: Clean up control-register-write vm_events
2015-05-12 15:49 ` Razvan Cojocaru
@ 2015-05-12 15:50 ` Andrew Cooper
2015-05-12 15:53 ` Andrew Cooper
0 siblings, 1 reply; 9+ messages in thread
From: Andrew Cooper @ 2015-05-12 15:50 UTC (permalink / raw)
To: Razvan Cojocaru, Jan Beulich
Cc: kevin.tian, wei.liu2, ian.campbell, stefano.stabellini, tim,
eddie.dong, xen-devel, jun.nakajima, keir, ian.jackson
On 12/05/15 16:49, Razvan Cojocaru wrote:
> On 05/12/2015 06:35 PM, Jan Beulich wrote:
>>>>> On 12.05.15 at 16:58, <rcojocaru@bitdefender.com> wrote:
>>> +/* Supported values for the vm_event_write_ctrlreg index. */
>>> +#define X86_CR0 (1 << 0)
>>> +#define X86_CR3 (1 << 1)
>>> +#define X86_CR4 (1 << 2)
>>> +#define X86_XCR0 (1 << 3)
>> These names, being put in the public interface, are way too generic.
> I've copied them from Andrew Cooper's suggestion in the previous thread,
> hopefully he'll chime in. I'm happy to rename / move them if so desired.
I intended something like VM_EVENT_ARCH_X86_CR0 etc.
~Andrew
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] xen/vm_event: Clean up control-register-write vm_events
2015-05-12 15:50 ` Andrew Cooper
@ 2015-05-12 15:53 ` Andrew Cooper
2015-05-12 16:52 ` Razvan Cojocaru
0 siblings, 1 reply; 9+ messages in thread
From: Andrew Cooper @ 2015-05-12 15:53 UTC (permalink / raw)
To: Razvan Cojocaru, Jan Beulich
Cc: kevin.tian, wei.liu2, ian.campbell, stefano.stabellini,
eddie.dong, tim, xen-devel, jun.nakajima, keir, ian.jackson
On 12/05/15 16:50, Andrew Cooper wrote:
> On 12/05/15 16:49, Razvan Cojocaru wrote:
>> On 05/12/2015 06:35 PM, Jan Beulich wrote:
>>>>>> On 12.05.15 at 16:58, <rcojocaru@bitdefender.com> wrote:
>>>> +/* Supported values for the vm_event_write_ctrlreg index. */
>>>> +#define X86_CR0 (1 << 0)
>>>> +#define X86_CR3 (1 << 1)
>>>> +#define X86_CR4 (1 << 2)
>>>> +#define X86_XCR0 (1 << 3)
>>> These names, being put in the public interface, are way too generic.
>> I've copied them from Andrew Cooper's suggestion in the previous thread,
>> hopefully he'll chime in. I'm happy to rename / move them if so desired.
> I intended something like VM_EVENT_ARCH_X86_CR0 etc.
or perhaps slightly more specific to ctrl_reg read/write.
~Andrew
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] xen/vm_event: Clean up control-register-write vm_events
2015-05-12 15:53 ` Andrew Cooper
@ 2015-05-12 16:52 ` Razvan Cojocaru
2015-05-12 17:54 ` Andrew Cooper
0 siblings, 1 reply; 9+ messages in thread
From: Razvan Cojocaru @ 2015-05-12 16:52 UTC (permalink / raw)
To: Andrew Cooper, Jan Beulich
Cc: kevin.tian, wei.liu2, ian.campbell, stefano.stabellini,
eddie.dong, tim, xen-devel, jun.nakajima, keir, ian.jackson
On 05/12/2015 06:53 PM, Andrew Cooper wrote:
> On 12/05/15 16:50, Andrew Cooper wrote:
>> On 12/05/15 16:49, Razvan Cojocaru wrote:
>>> On 05/12/2015 06:35 PM, Jan Beulich wrote:
>>>>>>> On 12.05.15 at 16:58, <rcojocaru@bitdefender.com> wrote:
>>>>> +/* Supported values for the vm_event_write_ctrlreg index. */
>>>>> +#define X86_CR0 (1 << 0)
>>>>> +#define X86_CR3 (1 << 1)
>>>>> +#define X86_CR4 (1 << 2)
>>>>> +#define X86_XCR0 (1 << 3)
>>>> These names, being put in the public interface, are way too generic.
>>> I've copied them from Andrew Cooper's suggestion in the previous thread,
>>> hopefully he'll chime in. I'm happy to rename / move them if so desired.
>> I intended something like VM_EVENT_ARCH_X86_CR0 etc.
>
> or perhaps slightly more specific to ctrl_reg read/write.
VM_EVENT_ARCH_X86_MOV_TO_CR0? Or maybe VM_EVENT_WRITE_ARCH_X86_CR0?
There's something else that should also be clarified: I've renamed the
vm_event.h struct to vm_event_write_ctrlreg, and changed
monitor.mov_to_cr* to monitor.write_ctrlreg*, but left
xc_monitor_mov_to_cr() in libxc, and mov_to_cr in the domctl part (I
thought it fit with mov_to_msr(), etc.). Should I change that to
write_ctrlreg as well? On the one hand, mov_to_cr seems to be consistent
with the previous interface, and on the other it's not consistent with
the name changes the patch has made.
Thanks,
Razvan
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] xen/vm_event: Clean up control-register-write vm_events
2015-05-12 16:52 ` Razvan Cojocaru
@ 2015-05-12 17:54 ` Andrew Cooper
2015-05-12 18:38 ` Razvan Cojocaru
0 siblings, 1 reply; 9+ messages in thread
From: Andrew Cooper @ 2015-05-12 17:54 UTC (permalink / raw)
To: Razvan Cojocaru, Jan Beulich
Cc: kevin.tian, wei.liu2, ian.campbell, stefano.stabellini, tim,
eddie.dong, xen-devel, jun.nakajima, keir, ian.jackson
On 12/05/15 17:52, Razvan Cojocaru wrote:
> On 05/12/2015 06:53 PM, Andrew Cooper wrote:
>> On 12/05/15 16:50, Andrew Cooper wrote:
>>> On 12/05/15 16:49, Razvan Cojocaru wrote:
>>>> On 05/12/2015 06:35 PM, Jan Beulich wrote:
>>>>>>>> On 12.05.15 at 16:58, <rcojocaru@bitdefender.com> wrote:
>>>>>> +/* Supported values for the vm_event_write_ctrlreg index. */
>>>>>> +#define X86_CR0 (1 << 0)
>>>>>> +#define X86_CR3 (1 << 1)
>>>>>> +#define X86_CR4 (1 << 2)
>>>>>> +#define X86_XCR0 (1 << 3)
>>>>> These names, being put in the public interface, are way too generic.
>>>> I've copied them from Andrew Cooper's suggestion in the previous thread,
>>>> hopefully he'll chime in. I'm happy to rename / move them if so desired.
>>> I intended something like VM_EVENT_ARCH_X86_CR0 etc.
>> or perhaps slightly more specific to ctrl_reg read/write.
> VM_EVENT_ARCH_X86_MOV_TO_CR0? Or maybe VM_EVENT_WRITE_ARCH_X86_CR0?
Are there any other events which could plausibly use the same indicies?
Monitoring ctrl_reg reads perhaps ?
>
> There's something else that should also be clarified: I've renamed the
> vm_event.h struct to vm_event_write_ctrlreg, and changed
> monitor.mov_to_cr* to monitor.write_ctrlreg*, but left
> xc_monitor_mov_to_cr() in libxc, and mov_to_cr in the domctl part (I
> thought it fit with mov_to_msr(), etc.). Should I change that to
> write_ctrlreg as well? On the one hand, mov_to_cr seems to be consistent
> with the previous interface, and on the other it's not consistent with
> the name changes the patch has made.
Any API/ABI changes in this area are fine until 4.6 is released, and a
consistent interface is much preferred.
~Andrew
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] xen/vm_event: Clean up control-register-write vm_events
2015-05-12 17:54 ` Andrew Cooper
@ 2015-05-12 18:38 ` Razvan Cojocaru
2015-05-13 6:06 ` Jan Beulich
0 siblings, 1 reply; 9+ messages in thread
From: Razvan Cojocaru @ 2015-05-12 18:38 UTC (permalink / raw)
To: Andrew Cooper, Jan Beulich
Cc: kevin.tian, wei.liu2, ian.campbell, stefano.stabellini,
eddie.dong, tim, xen-devel, jun.nakajima, keir, ian.jackson
On 05/12/2015 08:54 PM, Andrew Cooper wrote:
> On 12/05/15 17:52, Razvan Cojocaru wrote:
>> On 05/12/2015 06:53 PM, Andrew Cooper wrote:
>>> On 12/05/15 16:50, Andrew Cooper wrote:
>>>> On 12/05/15 16:49, Razvan Cojocaru wrote:
>>>>> On 05/12/2015 06:35 PM, Jan Beulich wrote:
>>>>>>>>> On 12.05.15 at 16:58, <rcojocaru@bitdefender.com> wrote:
>>>>>>> +/* Supported values for the vm_event_write_ctrlreg index. */
>>>>>>> +#define X86_CR0 (1 << 0)
>>>>>>> +#define X86_CR3 (1 << 1)
>>>>>>> +#define X86_CR4 (1 << 2)
>>>>>>> +#define X86_XCR0 (1 << 3)
>>>>>> These names, being put in the public interface, are way too generic.
>>>>> I've copied them from Andrew Cooper's suggestion in the previous thread,
>>>>> hopefully he'll chime in. I'm happy to rename / move them if so desired.
>>>> I intended something like VM_EVENT_ARCH_X86_CR0 etc.
>>> or perhaps slightly more specific to ctrl_reg read/write.
>> VM_EVENT_ARCH_X86_MOV_TO_CR0? Or maybe VM_EVENT_WRITE_ARCH_X86_CR0?
>
> Are there any other events which could plausibly use the same indicies?
> Monitoring ctrl_reg reads perhaps ?
I see what you mean, those could be reused for reads and result in less
verbosity in the vm_event.h header.
I'll stick to VM_EVENT_ARCH_X86_CR0 and so on (unless requested
otherwise), I don't have a better naming convention (I guess if we need
to emphasize that something is being done to said control register we
could call them VM_EVENT_ACCESS_ARCH_X86_CR0, but that's more verbose
and IMHO not much better).
>> There's something else that should also be clarified: I've renamed the
>> vm_event.h struct to vm_event_write_ctrlreg, and changed
>> monitor.mov_to_cr* to monitor.write_ctrlreg*, but left
>> xc_monitor_mov_to_cr() in libxc, and mov_to_cr in the domctl part (I
>> thought it fit with mov_to_msr(), etc.). Should I change that to
>> write_ctrlreg as well? On the one hand, mov_to_cr seems to be consistent
>> with the previous interface, and on the other it's not consistent with
>> the name changes the patch has made.
>
> Any API/ABI changes in this area are fine until 4.6 is released, and a
> consistent interface is much preferred.
Understood. Then I'll rename xc_monitor_mov_to_cr() to
xc_monitor_write_ctrlreg(), since vm_event_write_ctrlreg has been
previously requested. I'll also rename
XEN_DOMCTL_MONITOR_EVENT_MOV_TO_CR to
XEN_DOMCTL_MONITOR_EVENT_WRITE_CTRLREG, and VM_EVENT_REASON_MOV_TO_CR to
VM_EVENT_REASON_WRITE_CTRLREG.
I'm reading your reply to mean that I should also rename
xc_monitor_mov_to_msr() to xc_monitor_write_msr(), and do similar things
to the respective vm_event struct, VM_EVENT_REASON_MOV_TO_MSR and so on.
Is that correct?
Thanks,
Razvan
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] xen/vm_event: Clean up control-register-write vm_events
2015-05-12 18:38 ` Razvan Cojocaru
@ 2015-05-13 6:06 ` Jan Beulich
0 siblings, 0 replies; 9+ messages in thread
From: Jan Beulich @ 2015-05-13 6:06 UTC (permalink / raw)
To: Razvan Cojocaru, Andrew Cooper
Cc: tim, kevin.tian, wei.liu2, ian.campbell, stefano.stabellini,
eddie.dong, ian.jackson, xen-devel, jun.nakajima, keir
>>> On 12.05.15 at 20:38, <rcojocaru@bitdefender.com> wrote:
> On 05/12/2015 08:54 PM, Andrew Cooper wrote:
>> On 12/05/15 17:52, Razvan Cojocaru wrote:
>>> On 05/12/2015 06:53 PM, Andrew Cooper wrote:
>>>> On 12/05/15 16:50, Andrew Cooper wrote:
>>>>> On 12/05/15 16:49, Razvan Cojocaru wrote:
>>>>>> On 05/12/2015 06:35 PM, Jan Beulich wrote:
>>>>>>>>>> On 12.05.15 at 16:58, <rcojocaru@bitdefender.com> wrote:
>>>>>>>> +/* Supported values for the vm_event_write_ctrlreg index. */
>>>>>>>> +#define X86_CR0 (1 << 0)
>>>>>>>> +#define X86_CR3 (1 << 1)
>>>>>>>> +#define X86_CR4 (1 << 2)
>>>>>>>> +#define X86_XCR0 (1 << 3)
>>>>>>> These names, being put in the public interface, are way too generic.
>>>>>> I've copied them from Andrew Cooper's suggestion in the previous thread,
>>>>>> hopefully he'll chime in. I'm happy to rename / move them if so desired.
>>>>> I intended something like VM_EVENT_ARCH_X86_CR0 etc.
>>>> or perhaps slightly more specific to ctrl_reg read/write.
>>> VM_EVENT_ARCH_X86_MOV_TO_CR0? Or maybe VM_EVENT_WRITE_ARCH_X86_CR0?
>>
>> Are there any other events which could plausibly use the same indicies?
>> Monitoring ctrl_reg reads perhaps ?
>
> I see what you mean, those could be reused for reads and result in less
> verbosity in the vm_event.h header.
>
> I'll stick to VM_EVENT_ARCH_X86_CR0 and so on (unless requested
> otherwise),
Yes - I'd say this one, or even with the _ARCH part dropped.
Jan
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2015-05-13 6:06 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-05-12 14:58 [PATCH] xen/vm_event: Clean up control-register-write vm_events Razvan Cojocaru
2015-05-12 15:35 ` Jan Beulich
2015-05-12 15:49 ` Razvan Cojocaru
2015-05-12 15:50 ` Andrew Cooper
2015-05-12 15:53 ` Andrew Cooper
2015-05-12 16:52 ` Razvan Cojocaru
2015-05-12 17:54 ` Andrew Cooper
2015-05-12 18:38 ` Razvan Cojocaru
2015-05-13 6:06 ` Jan Beulich
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.