* [PATCH v4 0/2] Vm-events: move monitor vm-events code to common-side.
@ 2016-02-16 7:07 Corneliu ZUZU
2016-02-16 7:07 ` [PATCH v4 1/2] xen/arm: fix file comments Corneliu ZUZU
2016-02-16 7:08 ` [PATCH v4 2/2] xen/vm-events: Move parts of monitor_domctl code to common-side Corneliu ZUZU
0 siblings, 2 replies; 13+ messages in thread
From: Corneliu ZUZU @ 2016-02-16 7:07 UTC (permalink / raw)
To: xen-devel; +Cc: Stefano Stabellini, Ian Campbell
This patch series is an attempt to move some of the monitor vm-events code to
the common-side. Done to make it easier to move additional parts that can be
moved to common when ARM-side implementations are to be added.
Patches summary:
1. Fix file comment
Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
2. Move monitor_domctl to common-side.
Note: ARM support for guest-request, control-register write monitor vm-events
will follow after review of this patch-series.
---
Changed since v3:
1/2: nothing changed
2/2: only minor changes
* sanity check mop->event range to avoid left-shift undefined behavior
* remove unused requested_status
* use ASSERT_UNREACHABLE() instead of bug warning
* replace includes w/ structs forward-declare, fix #ifndef
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v4 1/2] xen/arm: fix file comments
2016-02-16 7:07 [PATCH v4 0/2] Vm-events: move monitor vm-events code to common-side Corneliu ZUZU
@ 2016-02-16 7:07 ` Corneliu ZUZU
2016-02-16 7:08 ` [PATCH v4 2/2] xen/vm-events: Move parts of monitor_domctl code to common-side Corneliu ZUZU
1 sibling, 0 replies; 13+ messages in thread
From: Corneliu ZUZU @ 2016-02-16 7:07 UTC (permalink / raw)
To: xen-devel; +Cc: Stefano Stabellini, Ian Campbell
Add file header comment and local variable block @ EOF
of xen/arch/arm/hvm.c.
Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com>
Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
Changed since v2 (mistakenly not included in v3): <nothing changed>
---
xen/arch/arm/hvm.c | 29 ++++++++++++++++++++++++++++-
1 file changed, 28 insertions(+), 1 deletion(-)
diff --git a/xen/arch/arm/hvm.c b/xen/arch/arm/hvm.c
index 5fd0753..056db1a 100644
--- a/xen/arch/arm/hvm.c
+++ b/xen/arch/arm/hvm.c
@@ -1,3 +1,21 @@
+/*
+ * arch/arm/hvm.c
+ *
+ * Arch-specific hardware virtual machine abstractions.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program; If not, see <http://www.gnu.org/licenses/>.
+ */
+
#include <xen/config.h>
#include <xen/init.h>
#include <xen/lib.h>
@@ -14,7 +32,6 @@
#include <asm/hypercall.h>
long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
-
{
long rc = 0;
@@ -65,3 +82,13 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
return rc;
}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
--
2.5.0
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v4 2/2] xen/vm-events: Move parts of monitor_domctl code to common-side.
2016-02-16 7:07 [PATCH v4 0/2] Vm-events: move monitor vm-events code to common-side Corneliu ZUZU
2016-02-16 7:07 ` [PATCH v4 1/2] xen/arm: fix file comments Corneliu ZUZU
@ 2016-02-16 7:08 ` Corneliu ZUZU
2016-02-16 8:13 ` Corneliu ZUZU
` (3 more replies)
1 sibling, 4 replies; 13+ messages in thread
From: Corneliu ZUZU @ 2016-02-16 7:08 UTC (permalink / raw)
To: xen-devel
Cc: Tamas K Lengyel, Keir Fraser, Ian Campbell, Razvan Cojocaru,
Andrew Cooper, Stefano Stabellini, Jan Beulich
This patch moves monitor_domctl to common-side.
Purpose: move what's common to common, prepare for implementation
of such vm-events on ARM.
* move get_capabilities to arch-side => arch_monitor_get_capabilities.
* add arch-side monitor op handling function => arch_monitor_domctl_op.
e.g. X86-side handles XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_REP op
* add arch-side monitor event handling function => arch_monitor_domctl_event.
e.g. X86-side handles XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR event enable/disable
* remove status_check
Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com>
---
Changed since v3:
* monitor_domctl @ common/monitor.c:
- remove unused requested_status
- sanity check mop->event range to avoid left-shift undefined behavior
* arch_monitor_domctl_event: use ASSERT_UNREACHABLE() instead of bug warning
* xen/monitor.h: replace includes w/ structs forward-declare, fix #ifndef
---
MAINTAINERS | 1 +
xen/arch/x86/monitor.c | 153 +++++++++++-------------------------------
xen/common/Makefile | 1 +
xen/common/domctl.c | 2 +-
xen/common/monitor.c | 69 +++++++++++++++++++
xen/include/asm-arm/monitor.h | 30 +++++++--
xen/include/asm-x86/monitor.h | 53 +++++++++++++--
xen/include/xen/monitor.h | 30 +++++++++
8 files changed, 217 insertions(+), 122 deletions(-)
create mode 100644 xen/common/monitor.c
create mode 100644 xen/include/xen/monitor.h
diff --git a/MAINTAINERS b/MAINTAINERS
index f07384c..5cbb1dc 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -355,6 +355,7 @@ M: Tamas K Lengyel <tamas@tklengyel.com>
S: Supported
F: xen/common/vm_event.c
F: xen/common/mem_access.c
+F: xen/common/monitor.c
F: xen/arch/x86/hvm/event.c
F: xen/arch/x86/monitor.c
F: xen/arch/*/vm_event.c
diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c
index 1d43880..660b92c 100644
--- a/xen/arch/x86/monitor.c
+++ b/xen/arch/x86/monitor.c
@@ -1,9 +1,10 @@
/*
* arch/x86/monitor.c
*
- * Architecture-specific monitor_op domctl handler.
+ * Arch-specific monitor_op domctl handler.
*
* Copyright (c) 2015 Tamas K Lengyel (tamas@tklengyel.com)
+ * Copyright (c) 2016, Bitdefender S.R.L.
*
* This program is free software; you can redistribute it and/or
* modify it under the terms of the GNU General Public
@@ -18,87 +19,14 @@
* License along with this program; If not, see <http://www.gnu.org/licenses/>.
*/
-#include <xen/config.h>
-#include <xen/sched.h>
-#include <xen/mm.h>
-#include <asm/domain.h>
#include <asm/monitor.h>
-#include <public/domctl.h>
-#include <xsm/xsm.h>
+#include <public/vm_event.h>
-/*
- * Sanity check whether option is already enabled/disabled
- */
-static inline
-int status_check(struct xen_domctl_monitor_op *mop, bool_t status)
-{
- bool_t requested_status = (mop->op == XEN_DOMCTL_MONITOR_OP_ENABLE);
-
- if ( status == requested_status )
- return -EEXIST;
-
- return 0;
-}
-
-static inline uint32_t get_capabilities(struct domain *d)
-{
- uint32_t capabilities = 0;
-
- /*
- * At the moment only Intel HVM domains are supported. However, event
- * delivery could be extended to AMD and PV domains.
- */
- if ( !is_hvm_domain(d) || !cpu_has_vmx )
- return capabilities;
-
- capabilities = (1 << XEN_DOMCTL_MONITOR_EVENT_WRITE_CTRLREG) |
- (1 << XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR) |
- (1 << XEN_DOMCTL_MONITOR_EVENT_SOFTWARE_BREAKPOINT) |
- (1 << XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST);
-
- /* Since we know this is on VMX, we can just call the hvm func */
- if ( hvm_is_singlestep_supported() )
- capabilities |= (1 << XEN_DOMCTL_MONITOR_EVENT_SINGLESTEP);
-
- return capabilities;
-}
-
-int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *mop)
+int arch_monitor_domctl_event(struct domain *d,
+ struct xen_domctl_monitor_op *mop)
{
- int rc;
struct arch_domain *ad = &d->arch;
- uint32_t capabilities = get_capabilities(d);
-
- if ( current->domain == d ) /* no domain_pause() */
- return -EPERM;
-
- rc = xsm_vm_event_control(XSM_PRIV, d, mop->op, mop->event);
- if ( rc )
- return rc;
-
- switch ( mop->op )
- {
- case XEN_DOMCTL_MONITOR_OP_GET_CAPABILITIES:
- mop->event = capabilities;
- return 0;
-
- case XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_REP:
- domain_pause(d);
- ad->mem_access_emulate_each_rep = !!mop->event;
- domain_unpause(d);
- return 0;
- }
-
- /*
- * Sanity check
- */
- if ( mop->op != XEN_DOMCTL_MONITOR_OP_ENABLE &&
- mop->op != XEN_DOMCTL_MONITOR_OP_DISABLE )
- return -EOPNOTSUPP;
-
- /* Check if event type is available. */
- if ( !(capabilities & (1 << mop->event)) )
- return -EOPNOTSUPP;
+ bool_t requested_status = (XEN_DOMCTL_MONITOR_OP_ENABLE == mop->op);
switch ( mop->event )
{
@@ -106,13 +34,11 @@ int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *mop)
{
unsigned int ctrlreg_bitmask =
monitor_ctrlreg_bitmask(mop->u.mov_to_cr.index);
- bool_t status =
+ bool_t old_status =
!!(ad->monitor.write_ctrlreg_enabled & ctrlreg_bitmask);
- struct vcpu *v;
- rc = status_check(mop, status);
- if ( rc )
- return rc;
+ if ( unlikely(old_status == requested_status) )
+ return -EEXIST;
domain_pause(d);
@@ -126,15 +52,18 @@ int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *mop)
else
ad->monitor.write_ctrlreg_onchangeonly &= ~ctrlreg_bitmask;
- if ( !status )
+ if ( !old_status )
ad->monitor.write_ctrlreg_enabled |= ctrlreg_bitmask;
else
ad->monitor.write_ctrlreg_enabled &= ~ctrlreg_bitmask;
- if ( mop->u.mov_to_cr.index == VM_EVENT_X86_CR3 )
- /* Latches new CR3 mask through CR0 code */
+ if ( VM_EVENT_X86_CR3 == mop->u.mov_to_cr.index )
+ {
+ struct vcpu *v;
+ /* Latches new CR3 mask through CR0 code. */
for_each_vcpu ( d, v )
hvm_update_guest_cr(v, 0);
+ }
domain_unpause(d);
@@ -143,77 +72,75 @@ int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *mop)
case XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR:
{
- bool_t status = ad->monitor.mov_to_msr_enabled;
+ bool_t old_status = ad->monitor.mov_to_msr_enabled;
- rc = status_check(mop, status);
- if ( rc )
- return rc;
+ if ( unlikely(old_status == requested_status) )
+ return -EEXIST;
- if ( mop->op == XEN_DOMCTL_MONITOR_OP_ENABLE &&
- mop->u.mov_to_msr.extended_capture &&
+ if ( requested_status && mop->u.mov_to_msr.extended_capture &&
!hvm_enable_msr_exit_interception(d) )
return -EOPNOTSUPP;
domain_pause(d);
- if ( mop->op == XEN_DOMCTL_MONITOR_OP_ENABLE &&
- mop->u.mov_to_msr.extended_capture )
- ad->monitor.mov_to_msr_extended = 1;
+ if ( requested_status && mop->u.mov_to_msr.extended_capture )
+ ad->monitor.mov_to_msr_extended = 1;
else
ad->monitor.mov_to_msr_extended = 0;
- ad->monitor.mov_to_msr_enabled = !status;
+ ad->monitor.mov_to_msr_enabled = !old_status;
domain_unpause(d);
break;
}
case XEN_DOMCTL_MONITOR_EVENT_SINGLESTEP:
{
- bool_t status = ad->monitor.singlestep_enabled;
+ bool_t old_status = ad->monitor.singlestep_enabled;
- rc = status_check(mop, status);
- if ( rc )
- return rc;
+ if ( unlikely(old_status == requested_status) )
+ return -EEXIST;
domain_pause(d);
- ad->monitor.singlestep_enabled = !status;
+ ad->monitor.singlestep_enabled = !old_status;
domain_unpause(d);
break;
}
case XEN_DOMCTL_MONITOR_EVENT_SOFTWARE_BREAKPOINT:
{
- bool_t status = ad->monitor.software_breakpoint_enabled;
+ bool_t old_status = ad->monitor.software_breakpoint_enabled;
- rc = status_check(mop, status);
- if ( rc )
- return rc;
+ if ( unlikely(old_status == requested_status) )
+ return -EEXIST;
domain_pause(d);
- ad->monitor.software_breakpoint_enabled = !status;
+ ad->monitor.software_breakpoint_enabled = !old_status;
domain_unpause(d);
break;
}
case XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST:
{
- bool_t status = ad->monitor.guest_request_enabled;
+ bool_t old_status = ad->monitor.guest_request_enabled;
- rc = status_check(mop, status);
- if ( rc )
- return rc;
+ if ( unlikely(old_status == requested_status) )
+ return -EEXIST;
domain_pause(d);
ad->monitor.guest_request_sync = mop->u.guest_request.sync;
- ad->monitor.guest_request_enabled = !status;
+ ad->monitor.guest_request_enabled = !old_status;
domain_unpause(d);
break;
}
default:
+ /*
+ * Should not be reached unless arch_monitor_get_capabilities() is not
+ * properly implemented.
+ */
+ ASSERT_UNREACHABLE();
return -EOPNOTSUPP;
-
- };
+ }
return 0;
}
diff --git a/xen/common/Makefile b/xen/common/Makefile
index 6e82b33..0d76efe 100644
--- a/xen/common/Makefile
+++ b/xen/common/Makefile
@@ -20,6 +20,7 @@ obj-y += lib.o
obj-y += lzo.o
obj-$(CONFIG_HAS_MEM_ACCESS) += mem_access.o
obj-y += memory.o
+obj-y += monitor.o
obj-y += multicall.o
obj-y += notifier.o
obj-y += page_alloc.o
diff --git a/xen/common/domctl.c b/xen/common/domctl.c
index 22fa5d5..b34c0a1 100644
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -25,11 +25,11 @@
#include <xen/paging.h>
#include <xen/hypercall.h>
#include <xen/vm_event.h>
+#include <xen/monitor.h>
#include <asm/current.h>
#include <asm/irq.h>
#include <asm/page.h>
#include <asm/p2m.h>
-#include <asm/monitor.h>
#include <public/domctl.h>
#include <xsm/xsm.h>
diff --git a/xen/common/monitor.c b/xen/common/monitor.c
new file mode 100644
index 0000000..2a99a04
--- /dev/null
+++ b/xen/common/monitor.c
@@ -0,0 +1,69 @@
+/*
+ * xen/common/monitor.c
+ *
+ * Common monitor_op domctl handler.
+ *
+ * Copyright (c) 2015 Tamas K Lengyel (tamas@tklengyel.com)
+ * Copyright (c) 2016, Bitdefender S.R.L.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public
+ * License v2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public
+ * License along with this program; If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <xen/monitor.h>
+#include <xen/sched.h>
+#include <xsm/xsm.h>
+#include <public/domctl.h>
+#include <asm/monitor.h>
+
+int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *mop)
+{
+ int rc;
+
+ if ( unlikely(current->domain == d) ) /* no domain_pause() */
+ return -EPERM;
+
+ rc = xsm_vm_event_control(XSM_PRIV, d, mop->op, mop->event);
+ if ( unlikely(rc) )
+ return rc;
+
+ switch ( mop->op )
+ {
+ case XEN_DOMCTL_MONITOR_OP_ENABLE:
+ case XEN_DOMCTL_MONITOR_OP_DISABLE:
+ /* Check if event type is available. */
+ /* sanity check: avoid left-shift undefined behavior */
+ if ( unlikely(mop->event > 31) )
+ return -EINVAL;
+ if ( unlikely(!(arch_monitor_get_capabilities(d) & (1U << mop->event))) )
+ return -EOPNOTSUPP;
+ /* Arch-side handles enable/disable ops. */
+ return arch_monitor_domctl_event(d, mop);
+
+ case XEN_DOMCTL_MONITOR_OP_GET_CAPABILITIES:
+ mop->event = arch_monitor_get_capabilities(d);
+ return 0;
+
+ default:
+ /* The monitor op is probably handled on the arch-side. */
+ return arch_monitor_domctl_op(d, mop);
+ }
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/include/asm-arm/monitor.h b/xen/include/asm-arm/monitor.h
index a3a9703..82a4a66 100644
--- a/xen/include/asm-arm/monitor.h
+++ b/xen/include/asm-arm/monitor.h
@@ -1,9 +1,10 @@
/*
* include/asm-arm/monitor.h
*
- * Architecture-specific monitor_op domctl handler.
+ * Arch-specific monitor_op domctl handler.
*
* Copyright (c) 2015 Tamas K Lengyel (tamas@tklengyel.com)
+ * Copyright (c) 2016, Bitdefender S.R.L.
*
* This program is free software; you can redistribute it and/or
* modify it under the terms of the GNU General Public
@@ -24,10 +25,31 @@
#include <xen/sched.h>
#include <public/domctl.h>
+static inline uint32_t arch_monitor_get_capabilities(struct domain *d)
+{
+ /* No monitor vm-events implemented on ARM. */
+ return 0;
+}
+
+static inline
+int arch_monitor_domctl_op(struct domain *d, struct xen_domctl_monitor_op *mop)
+{
+ /* No arch-specific monitor ops on ARM. */
+ return -EOPNOTSUPP;
+}
+
static inline
-int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *op)
+int arch_monitor_domctl_event(struct domain *d,
+ struct xen_domctl_monitor_op *mop)
{
- return -ENOSYS;
+ /*
+ * No arch-specific monitor vm-events on ARM.
+ *
+ * Should not be reached unless arch_monitor_get_capabilities() is not
+ * properly implemented.
+ */
+ ASSERT_UNREACHABLE();
+ return -EOPNOTSUPP;
}
-#endif /* __ASM_X86_MONITOR_H__ */
+#endif /* __ASM_ARM_MONITOR_H__ */
diff --git a/xen/include/asm-x86/monitor.h b/xen/include/asm-x86/monitor.h
index 7c8280b..c789f71 100644
--- a/xen/include/asm-x86/monitor.h
+++ b/xen/include/asm-x86/monitor.h
@@ -1,9 +1,10 @@
/*
* include/asm-x86/monitor.h
*
- * Architecture-specific monitor_op domctl handler.
+ * Arch-specific monitor_op domctl handler.
*
* Copyright (c) 2015 Tamas K Lengyel (tamas@tklengyel.com)
+ * Copyright (c) 2016, Bitdefender S.R.L.
*
* This program is free software; you can redistribute it and/or
* modify it under the terms of the GNU General Public
@@ -21,11 +22,55 @@
#ifndef __ASM_X86_MONITOR_H__
#define __ASM_X86_MONITOR_H__
-struct domain;
-struct xen_domctl_monitor_op;
+#include <xen/sched.h>
+#include <public/domctl.h>
+#include <asm/cpufeature.h>
+#include <asm/hvm/hvm.h>
#define monitor_ctrlreg_bitmask(ctrlreg_index) (1U << (ctrlreg_index))
-int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *op);
+static inline uint32_t arch_monitor_get_capabilities(struct domain *d)
+{
+ uint32_t capabilities = 0;
+
+ /*
+ * At the moment only Intel HVM domains are supported. However, event
+ * delivery could be extended to AMD and PV domains.
+ */
+ if ( !is_hvm_domain(d) || !cpu_has_vmx )
+ return capabilities;
+
+ capabilities = (1 << XEN_DOMCTL_MONITOR_EVENT_WRITE_CTRLREG) |
+ (1 << XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR) |
+ (1 << XEN_DOMCTL_MONITOR_EVENT_SOFTWARE_BREAKPOINT) |
+ (1 << XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST);
+
+ /* Since we know this is on VMX, we can just call the hvm func */
+ if ( hvm_is_singlestep_supported() )
+ capabilities |= (1 << XEN_DOMCTL_MONITOR_EVENT_SINGLESTEP);
+
+ return capabilities;
+}
+
+static inline
+int arch_monitor_domctl_op(struct domain *d, struct xen_domctl_monitor_op *mop)
+{
+ switch ( mop->op )
+ {
+ case XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_REP:
+ domain_pause(d);
+ d->arch.mem_access_emulate_each_rep = !!mop->event;
+ domain_unpause(d);
+ break;
+
+ default:
+ return -EOPNOTSUPP;
+ }
+
+ return 0;
+}
+
+int arch_monitor_domctl_event(struct domain *d,
+ struct xen_domctl_monitor_op *mop);
#endif /* __ASM_X86_MONITOR_H__ */
diff --git a/xen/include/xen/monitor.h b/xen/include/xen/monitor.h
new file mode 100644
index 0000000..7015e6d
--- /dev/null
+++ b/xen/include/xen/monitor.h
@@ -0,0 +1,30 @@
+/*
+ * include/xen/monitor.h
+ *
+ * Common monitor_op domctl handler.
+ *
+ * Copyright (c) 2015 Tamas K Lengyel (tamas@tklengyel.com)
+ * Copyright (c) 2016, Bitdefender S.R.L.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public
+ * License v2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public
+ * License along with this program; If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef __XEN_MONITOR_H__
+#define __XEN_MONITOR_H__
+
+struct domain;
+struct xen_domctl_monitor_op;
+
+int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *op);
+
+#endif /* __XEN_MONITOR_H__ */
--
2.5.0
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v4 2/2] xen/vm-events: Move parts of monitor_domctl code to common-side.
2016-02-16 7:08 ` [PATCH v4 2/2] xen/vm-events: Move parts of monitor_domctl code to common-side Corneliu ZUZU
@ 2016-02-16 8:13 ` Corneliu ZUZU
2016-02-16 10:45 ` Jan Beulich
2016-02-16 10:54 ` Stefano Stabellini
` (2 subsequent siblings)
3 siblings, 1 reply; 13+ messages in thread
From: Corneliu ZUZU @ 2016-02-16 8:13 UTC (permalink / raw)
To: xen-devel
Cc: Tamas K Lengyel, Keir Fraser, Ian Campbell, Razvan Cojocaru,
Andrew Cooper, Stefano Stabellini, Jan Beulich
On 2/16/2016 9:08 AM, Corneliu ZUZU wrote:
> This patch moves monitor_domctl to common-side.
> Purpose: move what's common to common, prepare for implementation
> of such vm-events on ARM.
>
> * move get_capabilities to arch-side => arch_monitor_get_capabilities.
> * add arch-side monitor op handling function => arch_monitor_domctl_op.
> e.g. X86-side handles XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_REP op
> * add arch-side monitor event handling function => arch_monitor_domctl_event.
> e.g. X86-side handles XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR event enable/disable
> * remove status_check
>
> Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com>
>
> ---
> Changed since v3:
> * monitor_domctl @ common/monitor.c:
> - remove unused requested_status
> - sanity check mop->event range to avoid left-shift undefined behavior
Due to left-shift undefined behavior situations, shouldn't I also:
* in X86 arch_monitor_get_capabilities: replace '1 <<' w/ '1U <<'
* in X86 arch_monitor_domctl_event,
XEN_DOMCTL_MONITOR_EVENT_WRITE_CTRLREG case
add a sanity check of mop->u.mov_to_cr.index before:
unsigned int ctrlreg_bitmask =
monitor_ctrlreg_bitmask(mop->u.mov_to_cr.index);
, which basically translates to:
unsigned int ctrlreg_bitmask = (1U << mop->u.mov_to_cr.index);
? (especially since mop->u.mov_to_cr.index is set by the caller).
Would have been good if I'd thought of that before sending this patch
series :).
Thanks,
Corneliu.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v4 2/2] xen/vm-events: Move parts of monitor_domctl code to common-side.
2016-02-16 8:13 ` Corneliu ZUZU
@ 2016-02-16 10:45 ` Jan Beulich
2016-02-16 11:20 ` Corneliu ZUZU
0 siblings, 1 reply; 13+ messages in thread
From: Jan Beulich @ 2016-02-16 10:45 UTC (permalink / raw)
To: Corneliu ZUZU
Cc: Tamas K Lengyel, Keir Fraser, Ian Campbell, Razvan Cojocaru,
Andrew Cooper, xen-devel, Stefano Stabellini
>>> On 16.02.16 at 09:13, <czuzu@bitdefender.com> wrote:
> On 2/16/2016 9:08 AM, Corneliu ZUZU wrote:
>> This patch moves monitor_domctl to common-side.
>> Purpose: move what's common to common, prepare for implementation
>> of such vm-events on ARM.
>>
>> * move get_capabilities to arch-side => arch_monitor_get_capabilities.
>> * add arch-side monitor op handling function => arch_monitor_domctl_op.
>> e.g. X86-side handles XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_REP op
>> * add arch-side monitor event handling function => arch_monitor_domctl_event.
>> e.g. X86-side handles XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR event
> enable/disable
>> * remove status_check
>>
>> Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com>
>>
>> ---
>> Changed since v3:
>> * monitor_domctl @ common/monitor.c:
>> - remove unused requested_status
>> - sanity check mop->event range to avoid left-shift undefined behavior
>
> Due to left-shift undefined behavior situations, shouldn't I also:
>
> * in X86 arch_monitor_get_capabilities: replace '1 <<' w/ '1U <<'
There's no undefinedness there, since the right side operands of
<< are all constant. Using 1U here would be okay, but is not
strictly needed.
> * in X86 arch_monitor_domctl_event,
> XEN_DOMCTL_MONITOR_EVENT_WRITE_CTRLREG case
> add a sanity check of mop->u.mov_to_cr.index before:
> unsigned int ctrlreg_bitmask =
> monitor_ctrlreg_bitmask(mop->u.mov_to_cr.index);
> , which basically translates to:
> unsigned int ctrlreg_bitmask = (1U << mop->u.mov_to_cr.index);
>
> ? (especially since mop->u.mov_to_cr.index is set by the caller).
Yes, there a range check would be needed, but preferably as a
separate patch (as this has nothing to do with the code motion
you perform here).
Jan
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v4 2/2] xen/vm-events: Move parts of monitor_domctl code to common-side.
2016-02-16 7:08 ` [PATCH v4 2/2] xen/vm-events: Move parts of monitor_domctl code to common-side Corneliu ZUZU
2016-02-16 8:13 ` Corneliu ZUZU
@ 2016-02-16 10:54 ` Stefano Stabellini
2016-02-16 14:10 ` Razvan Cojocaru
2016-02-16 16:02 ` Tamas K Lengyel
3 siblings, 0 replies; 13+ messages in thread
From: Stefano Stabellini @ 2016-02-16 10:54 UTC (permalink / raw)
To: Corneliu ZUZU
Cc: Tamas K Lengyel, Keir Fraser, Ian Campbell, Razvan Cojocaru,
Andrew Cooper, xen-devel, Stefano Stabellini, Jan Beulich
On Tue, 16 Feb 2016, Corneliu ZUZU wrote:
> This patch moves monitor_domctl to common-side.
> Purpose: move what's common to common, prepare for implementation
> of such vm-events on ARM.
>
> * move get_capabilities to arch-side => arch_monitor_get_capabilities.
> * add arch-side monitor op handling function => arch_monitor_domctl_op.
> e.g. X86-side handles XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_REP op
> * add arch-side monitor event handling function => arch_monitor_domctl_event.
> e.g. X86-side handles XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR event enable/disable
> * remove status_check
>
> Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com>
For the ARM part
Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Changed since v3:
> * monitor_domctl @ common/monitor.c:
> - remove unused requested_status
> - sanity check mop->event range to avoid left-shift undefined behavior
> * arch_monitor_domctl_event: use ASSERT_UNREACHABLE() instead of bug warning
> * xen/monitor.h: replace includes w/ structs forward-declare, fix #ifndef
> ---
> MAINTAINERS | 1 +
> xen/arch/x86/monitor.c | 153 +++++++++++-------------------------------
> xen/common/Makefile | 1 +
> xen/common/domctl.c | 2 +-
> xen/common/monitor.c | 69 +++++++++++++++++++
> xen/include/asm-arm/monitor.h | 30 +++++++--
> xen/include/asm-x86/monitor.h | 53 +++++++++++++--
> xen/include/xen/monitor.h | 30 +++++++++
> 8 files changed, 217 insertions(+), 122 deletions(-)
> create mode 100644 xen/common/monitor.c
> create mode 100644 xen/include/xen/monitor.h
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index f07384c..5cbb1dc 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -355,6 +355,7 @@ M: Tamas K Lengyel <tamas@tklengyel.com>
> S: Supported
> F: xen/common/vm_event.c
> F: xen/common/mem_access.c
> +F: xen/common/monitor.c
> F: xen/arch/x86/hvm/event.c
> F: xen/arch/x86/monitor.c
> F: xen/arch/*/vm_event.c
> diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c
> index 1d43880..660b92c 100644
> --- a/xen/arch/x86/monitor.c
> +++ b/xen/arch/x86/monitor.c
> @@ -1,9 +1,10 @@
> /*
> * arch/x86/monitor.c
> *
> - * Architecture-specific monitor_op domctl handler.
> + * Arch-specific monitor_op domctl handler.
> *
> * Copyright (c) 2015 Tamas K Lengyel (tamas@tklengyel.com)
> + * Copyright (c) 2016, Bitdefender S.R.L.
> *
> * This program is free software; you can redistribute it and/or
> * modify it under the terms of the GNU General Public
> @@ -18,87 +19,14 @@
> * License along with this program; If not, see <http://www.gnu.org/licenses/>.
> */
>
> -#include <xen/config.h>
> -#include <xen/sched.h>
> -#include <xen/mm.h>
> -#include <asm/domain.h>
> #include <asm/monitor.h>
> -#include <public/domctl.h>
> -#include <xsm/xsm.h>
> +#include <public/vm_event.h>
>
> -/*
> - * Sanity check whether option is already enabled/disabled
> - */
> -static inline
> -int status_check(struct xen_domctl_monitor_op *mop, bool_t status)
> -{
> - bool_t requested_status = (mop->op == XEN_DOMCTL_MONITOR_OP_ENABLE);
> -
> - if ( status == requested_status )
> - return -EEXIST;
> -
> - return 0;
> -}
> -
> -static inline uint32_t get_capabilities(struct domain *d)
> -{
> - uint32_t capabilities = 0;
> -
> - /*
> - * At the moment only Intel HVM domains are supported. However, event
> - * delivery could be extended to AMD and PV domains.
> - */
> - if ( !is_hvm_domain(d) || !cpu_has_vmx )
> - return capabilities;
> -
> - capabilities = (1 << XEN_DOMCTL_MONITOR_EVENT_WRITE_CTRLREG) |
> - (1 << XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR) |
> - (1 << XEN_DOMCTL_MONITOR_EVENT_SOFTWARE_BREAKPOINT) |
> - (1 << XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST);
> -
> - /* Since we know this is on VMX, we can just call the hvm func */
> - if ( hvm_is_singlestep_supported() )
> - capabilities |= (1 << XEN_DOMCTL_MONITOR_EVENT_SINGLESTEP);
> -
> - return capabilities;
> -}
> -
> -int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *mop)
> +int arch_monitor_domctl_event(struct domain *d,
> + struct xen_domctl_monitor_op *mop)
> {
> - int rc;
> struct arch_domain *ad = &d->arch;
> - uint32_t capabilities = get_capabilities(d);
> -
> - if ( current->domain == d ) /* no domain_pause() */
> - return -EPERM;
> -
> - rc = xsm_vm_event_control(XSM_PRIV, d, mop->op, mop->event);
> - if ( rc )
> - return rc;
> -
> - switch ( mop->op )
> - {
> - case XEN_DOMCTL_MONITOR_OP_GET_CAPABILITIES:
> - mop->event = capabilities;
> - return 0;
> -
> - case XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_REP:
> - domain_pause(d);
> - ad->mem_access_emulate_each_rep = !!mop->event;
> - domain_unpause(d);
> - return 0;
> - }
> -
> - /*
> - * Sanity check
> - */
> - if ( mop->op != XEN_DOMCTL_MONITOR_OP_ENABLE &&
> - mop->op != XEN_DOMCTL_MONITOR_OP_DISABLE )
> - return -EOPNOTSUPP;
> -
> - /* Check if event type is available. */
> - if ( !(capabilities & (1 << mop->event)) )
> - return -EOPNOTSUPP;
> + bool_t requested_status = (XEN_DOMCTL_MONITOR_OP_ENABLE == mop->op);
>
> switch ( mop->event )
> {
> @@ -106,13 +34,11 @@ int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *mop)
> {
> unsigned int ctrlreg_bitmask =
> monitor_ctrlreg_bitmask(mop->u.mov_to_cr.index);
> - bool_t status =
> + bool_t old_status =
> !!(ad->monitor.write_ctrlreg_enabled & ctrlreg_bitmask);
> - struct vcpu *v;
>
> - rc = status_check(mop, status);
> - if ( rc )
> - return rc;
> + if ( unlikely(old_status == requested_status) )
> + return -EEXIST;
>
> domain_pause(d);
>
> @@ -126,15 +52,18 @@ int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *mop)
> else
> ad->monitor.write_ctrlreg_onchangeonly &= ~ctrlreg_bitmask;
>
> - if ( !status )
> + if ( !old_status )
> ad->monitor.write_ctrlreg_enabled |= ctrlreg_bitmask;
> else
> ad->monitor.write_ctrlreg_enabled &= ~ctrlreg_bitmask;
>
> - if ( mop->u.mov_to_cr.index == VM_EVENT_X86_CR3 )
> - /* Latches new CR3 mask through CR0 code */
> + if ( VM_EVENT_X86_CR3 == mop->u.mov_to_cr.index )
> + {
> + struct vcpu *v;
> + /* Latches new CR3 mask through CR0 code. */
> for_each_vcpu ( d, v )
> hvm_update_guest_cr(v, 0);
> + }
>
> domain_unpause(d);
>
> @@ -143,77 +72,75 @@ int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *mop)
>
> case XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR:
> {
> - bool_t status = ad->monitor.mov_to_msr_enabled;
> + bool_t old_status = ad->monitor.mov_to_msr_enabled;
>
> - rc = status_check(mop, status);
> - if ( rc )
> - return rc;
> + if ( unlikely(old_status == requested_status) )
> + return -EEXIST;
>
> - if ( mop->op == XEN_DOMCTL_MONITOR_OP_ENABLE &&
> - mop->u.mov_to_msr.extended_capture &&
> + if ( requested_status && mop->u.mov_to_msr.extended_capture &&
> !hvm_enable_msr_exit_interception(d) )
> return -EOPNOTSUPP;
>
> domain_pause(d);
>
> - if ( mop->op == XEN_DOMCTL_MONITOR_OP_ENABLE &&
> - mop->u.mov_to_msr.extended_capture )
> - ad->monitor.mov_to_msr_extended = 1;
> + if ( requested_status && mop->u.mov_to_msr.extended_capture )
> + ad->monitor.mov_to_msr_extended = 1;
> else
> ad->monitor.mov_to_msr_extended = 0;
>
> - ad->monitor.mov_to_msr_enabled = !status;
> + ad->monitor.mov_to_msr_enabled = !old_status;
> domain_unpause(d);
> break;
> }
>
> case XEN_DOMCTL_MONITOR_EVENT_SINGLESTEP:
> {
> - bool_t status = ad->monitor.singlestep_enabled;
> + bool_t old_status = ad->monitor.singlestep_enabled;
>
> - rc = status_check(mop, status);
> - if ( rc )
> - return rc;
> + if ( unlikely(old_status == requested_status) )
> + return -EEXIST;
>
> domain_pause(d);
> - ad->monitor.singlestep_enabled = !status;
> + ad->monitor.singlestep_enabled = !old_status;
> domain_unpause(d);
> break;
> }
>
> case XEN_DOMCTL_MONITOR_EVENT_SOFTWARE_BREAKPOINT:
> {
> - bool_t status = ad->monitor.software_breakpoint_enabled;
> + bool_t old_status = ad->monitor.software_breakpoint_enabled;
>
> - rc = status_check(mop, status);
> - if ( rc )
> - return rc;
> + if ( unlikely(old_status == requested_status) )
> + return -EEXIST;
>
> domain_pause(d);
> - ad->monitor.software_breakpoint_enabled = !status;
> + ad->monitor.software_breakpoint_enabled = !old_status;
> domain_unpause(d);
> break;
> }
>
> case XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST:
> {
> - bool_t status = ad->monitor.guest_request_enabled;
> + bool_t old_status = ad->monitor.guest_request_enabled;
>
> - rc = status_check(mop, status);
> - if ( rc )
> - return rc;
> + if ( unlikely(old_status == requested_status) )
> + return -EEXIST;
>
> domain_pause(d);
> ad->monitor.guest_request_sync = mop->u.guest_request.sync;
> - ad->monitor.guest_request_enabled = !status;
> + ad->monitor.guest_request_enabled = !old_status;
> domain_unpause(d);
> break;
> }
>
> default:
> + /*
> + * Should not be reached unless arch_monitor_get_capabilities() is not
> + * properly implemented.
> + */
> + ASSERT_UNREACHABLE();
> return -EOPNOTSUPP;
> -
> - };
> + }
>
> return 0;
> }
> diff --git a/xen/common/Makefile b/xen/common/Makefile
> index 6e82b33..0d76efe 100644
> --- a/xen/common/Makefile
> +++ b/xen/common/Makefile
> @@ -20,6 +20,7 @@ obj-y += lib.o
> obj-y += lzo.o
> obj-$(CONFIG_HAS_MEM_ACCESS) += mem_access.o
> obj-y += memory.o
> +obj-y += monitor.o
> obj-y += multicall.o
> obj-y += notifier.o
> obj-y += page_alloc.o
> diff --git a/xen/common/domctl.c b/xen/common/domctl.c
> index 22fa5d5..b34c0a1 100644
> --- a/xen/common/domctl.c
> +++ b/xen/common/domctl.c
> @@ -25,11 +25,11 @@
> #include <xen/paging.h>
> #include <xen/hypercall.h>
> #include <xen/vm_event.h>
> +#include <xen/monitor.h>
> #include <asm/current.h>
> #include <asm/irq.h>
> #include <asm/page.h>
> #include <asm/p2m.h>
> -#include <asm/monitor.h>
> #include <public/domctl.h>
> #include <xsm/xsm.h>
>
> diff --git a/xen/common/monitor.c b/xen/common/monitor.c
> new file mode 100644
> index 0000000..2a99a04
> --- /dev/null
> +++ b/xen/common/monitor.c
> @@ -0,0 +1,69 @@
> +/*
> + * xen/common/monitor.c
> + *
> + * Common monitor_op domctl handler.
> + *
> + * Copyright (c) 2015 Tamas K Lengyel (tamas@tklengyel.com)
> + * Copyright (c) 2016, Bitdefender S.R.L.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public
> + * License v2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public
> + * License along with this program; If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <xen/monitor.h>
> +#include <xen/sched.h>
> +#include <xsm/xsm.h>
> +#include <public/domctl.h>
> +#include <asm/monitor.h>
> +
> +int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *mop)
> +{
> + int rc;
> +
> + if ( unlikely(current->domain == d) ) /* no domain_pause() */
> + return -EPERM;
> +
> + rc = xsm_vm_event_control(XSM_PRIV, d, mop->op, mop->event);
> + if ( unlikely(rc) )
> + return rc;
> +
> + switch ( mop->op )
> + {
> + case XEN_DOMCTL_MONITOR_OP_ENABLE:
> + case XEN_DOMCTL_MONITOR_OP_DISABLE:
> + /* Check if event type is available. */
> + /* sanity check: avoid left-shift undefined behavior */
> + if ( unlikely(mop->event > 31) )
> + return -EINVAL;
> + if ( unlikely(!(arch_monitor_get_capabilities(d) & (1U << mop->event))) )
> + return -EOPNOTSUPP;
> + /* Arch-side handles enable/disable ops. */
> + return arch_monitor_domctl_event(d, mop);
> +
> + case XEN_DOMCTL_MONITOR_OP_GET_CAPABILITIES:
> + mop->event = arch_monitor_get_capabilities(d);
> + return 0;
> +
> + default:
> + /* The monitor op is probably handled on the arch-side. */
> + return arch_monitor_domctl_op(d, mop);
> + }
> +}
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/xen/include/asm-arm/monitor.h b/xen/include/asm-arm/monitor.h
> index a3a9703..82a4a66 100644
> --- a/xen/include/asm-arm/monitor.h
> +++ b/xen/include/asm-arm/monitor.h
> @@ -1,9 +1,10 @@
> /*
> * include/asm-arm/monitor.h
> *
> - * Architecture-specific monitor_op domctl handler.
> + * Arch-specific monitor_op domctl handler.
> *
> * Copyright (c) 2015 Tamas K Lengyel (tamas@tklengyel.com)
> + * Copyright (c) 2016, Bitdefender S.R.L.
> *
> * This program is free software; you can redistribute it and/or
> * modify it under the terms of the GNU General Public
> @@ -24,10 +25,31 @@
> #include <xen/sched.h>
> #include <public/domctl.h>
>
> +static inline uint32_t arch_monitor_get_capabilities(struct domain *d)
> +{
> + /* No monitor vm-events implemented on ARM. */
> + return 0;
> +}
> +
> +static inline
> +int arch_monitor_domctl_op(struct domain *d, struct xen_domctl_monitor_op *mop)
> +{
> + /* No arch-specific monitor ops on ARM. */
> + return -EOPNOTSUPP;
> +}
> +
> static inline
> -int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *op)
> +int arch_monitor_domctl_event(struct domain *d,
> + struct xen_domctl_monitor_op *mop)
> {
> - return -ENOSYS;
> + /*
> + * No arch-specific monitor vm-events on ARM.
> + *
> + * Should not be reached unless arch_monitor_get_capabilities() is not
> + * properly implemented.
> + */
> + ASSERT_UNREACHABLE();
> + return -EOPNOTSUPP;
> }
>
> -#endif /* __ASM_X86_MONITOR_H__ */
> +#endif /* __ASM_ARM_MONITOR_H__ */
> diff --git a/xen/include/asm-x86/monitor.h b/xen/include/asm-x86/monitor.h
> index 7c8280b..c789f71 100644
> --- a/xen/include/asm-x86/monitor.h
> +++ b/xen/include/asm-x86/monitor.h
> @@ -1,9 +1,10 @@
> /*
> * include/asm-x86/monitor.h
> *
> - * Architecture-specific monitor_op domctl handler.
> + * Arch-specific monitor_op domctl handler.
> *
> * Copyright (c) 2015 Tamas K Lengyel (tamas@tklengyel.com)
> + * Copyright (c) 2016, Bitdefender S.R.L.
> *
> * This program is free software; you can redistribute it and/or
> * modify it under the terms of the GNU General Public
> @@ -21,11 +22,55 @@
> #ifndef __ASM_X86_MONITOR_H__
> #define __ASM_X86_MONITOR_H__
>
> -struct domain;
> -struct xen_domctl_monitor_op;
> +#include <xen/sched.h>
> +#include <public/domctl.h>
> +#include <asm/cpufeature.h>
> +#include <asm/hvm/hvm.h>
>
> #define monitor_ctrlreg_bitmask(ctrlreg_index) (1U << (ctrlreg_index))
>
> -int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *op);
> +static inline uint32_t arch_monitor_get_capabilities(struct domain *d)
> +{
> + uint32_t capabilities = 0;
> +
> + /*
> + * At the moment only Intel HVM domains are supported. However, event
> + * delivery could be extended to AMD and PV domains.
> + */
> + if ( !is_hvm_domain(d) || !cpu_has_vmx )
> + return capabilities;
> +
> + capabilities = (1 << XEN_DOMCTL_MONITOR_EVENT_WRITE_CTRLREG) |
> + (1 << XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR) |
> + (1 << XEN_DOMCTL_MONITOR_EVENT_SOFTWARE_BREAKPOINT) |
> + (1 << XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST);
> +
> + /* Since we know this is on VMX, we can just call the hvm func */
> + if ( hvm_is_singlestep_supported() )
> + capabilities |= (1 << XEN_DOMCTL_MONITOR_EVENT_SINGLESTEP);
> +
> + return capabilities;
> +}
> +
> +static inline
> +int arch_monitor_domctl_op(struct domain *d, struct xen_domctl_monitor_op *mop)
> +{
> + switch ( mop->op )
> + {
> + case XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_REP:
> + domain_pause(d);
> + d->arch.mem_access_emulate_each_rep = !!mop->event;
> + domain_unpause(d);
> + break;
> +
> + default:
> + return -EOPNOTSUPP;
> + }
> +
> + return 0;
> +}
> +
> +int arch_monitor_domctl_event(struct domain *d,
> + struct xen_domctl_monitor_op *mop);
>
> #endif /* __ASM_X86_MONITOR_H__ */
> diff --git a/xen/include/xen/monitor.h b/xen/include/xen/monitor.h
> new file mode 100644
> index 0000000..7015e6d
> --- /dev/null
> +++ b/xen/include/xen/monitor.h
> @@ -0,0 +1,30 @@
> +/*
> + * include/xen/monitor.h
> + *
> + * Common monitor_op domctl handler.
> + *
> + * Copyright (c) 2015 Tamas K Lengyel (tamas@tklengyel.com)
> + * Copyright (c) 2016, Bitdefender S.R.L.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public
> + * License v2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public
> + * License along with this program; If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#ifndef __XEN_MONITOR_H__
> +#define __XEN_MONITOR_H__
> +
> +struct domain;
> +struct xen_domctl_monitor_op;
> +
> +int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *op);
> +
> +#endif /* __XEN_MONITOR_H__ */
> --
> 2.5.0
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v4 2/2] xen/vm-events: Move parts of monitor_domctl code to common-side.
2016-02-16 10:45 ` Jan Beulich
@ 2016-02-16 11:20 ` Corneliu ZUZU
2016-02-16 12:34 ` Jan Beulich
0 siblings, 1 reply; 13+ messages in thread
From: Corneliu ZUZU @ 2016-02-16 11:20 UTC (permalink / raw)
To: Jan Beulich
Cc: Tamas K Lengyel, Keir Fraser, Ian Campbell, Razvan Cojocaru,
Andrew Cooper, xen-devel, Stefano Stabellini
On 2/16/2016 12:45 PM, Jan Beulich wrote:
>>>> On 16.02.16 at 09:13, <czuzu@bitdefender.com> wrote:
>> On 2/16/2016 9:08 AM, Corneliu ZUZU wrote:
>>> This patch moves monitor_domctl to common-side.
>>> Purpose: move what's common to common, prepare for implementation
>>> of such vm-events on ARM.
>>>
>>> * move get_capabilities to arch-side => arch_monitor_get_capabilities.
>>> * add arch-side monitor op handling function => arch_monitor_domctl_op.
>>> e.g. X86-side handles XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_REP op
>>> * add arch-side monitor event handling function => arch_monitor_domctl_event.
>>> e.g. X86-side handles XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR event
>> enable/disable
>>> * remove status_check
>>>
>>> Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com>
>>>
>>> ---
>>> Changed since v3:
>>> * monitor_domctl @ common/monitor.c:
>>> - remove unused requested_status
>>> - sanity check mop->event range to avoid left-shift undefined behavior
>> Due to left-shift undefined behavior situations, shouldn't I also:
>>
>> * in X86 arch_monitor_get_capabilities: replace '1 <<' w/ '1U <<'
> There's no undefinedness there, since the right side operands of
> << are all constant. Using 1U here would be okay, but is not
> strictly needed.
I reasoned based on this ISO C99 quote:
[for an E1 << E2 operation, ]
"If E1 has a signed type and nonnegative value, and E1 × 2^E2 is
representable in the result type, then that is the resulting value;
otherwise, the behavior is undefined."
I inferred that this means that code such as '(1 << 31)' would render
undefined behavior, since (1 x 2^31) is not representable on 'int'.
The standard doesn't seem to mention different behavior if the operands
are constants.
This would render undefined behavior if bit 31 of capabilities would be
used @ some point, i.e. if one day someone would e.g. unknowingly:
#define XEN_DOMCTL_MONITOR_EVENT_GRAVITATIONAL_WAVE 31
Have I misinterpreted the 'representable in the result type' part?
>
>> * in X86 arch_monitor_domctl_event,
>> XEN_DOMCTL_MONITOR_EVENT_WRITE_CTRLREG case
>> add a sanity check of mop->u.mov_to_cr.index before:
>> unsigned int ctrlreg_bitmask =
>> monitor_ctrlreg_bitmask(mop->u.mov_to_cr.index);
>> , which basically translates to:
>> unsigned int ctrlreg_bitmask = (1U << mop->u.mov_to_cr.index);
>>
>> ? (especially since mop->u.mov_to_cr.index is set by the caller).
> Yes, there a range check would be needed, but preferably as a
> separate patch (as this has nothing to do with the code motion
> you perform here).
>
> Jan
>
>
Great, I'll do these changes in a separate patch then.
Let me know if you have any other comments on this.
Thanks,
Corneliu.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v4 2/2] xen/vm-events: Move parts of monitor_domctl code to common-side.
2016-02-16 11:20 ` Corneliu ZUZU
@ 2016-02-16 12:34 ` Jan Beulich
2016-02-16 13:03 ` Corneliu ZUZU
0 siblings, 1 reply; 13+ messages in thread
From: Jan Beulich @ 2016-02-16 12:34 UTC (permalink / raw)
To: Corneliu ZUZU
Cc: Tamas K Lengyel, Keir Fraser, Ian Campbell, Razvan Cojocaru,
Andrew Cooper, xen-devel, Stefano Stabellini
>>> On 16.02.16 at 12:20, <czuzu@bitdefender.com> wrote:
> On 2/16/2016 12:45 PM, Jan Beulich wrote:
>>>>> On 16.02.16 at 09:13, <czuzu@bitdefender.com> wrote:
>>> On 2/16/2016 9:08 AM, Corneliu ZUZU wrote:
>>>> This patch moves monitor_domctl to common-side.
>>>> Purpose: move what's common to common, prepare for implementation
>>>> of such vm-events on ARM.
>>>>
>>>> * move get_capabilities to arch-side => arch_monitor_get_capabilities.
>>>> * add arch-side monitor op handling function => arch_monitor_domctl_op.
>>>> e.g. X86-side handles XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_REP op
>>>> * add arch-side monitor event handling function => arch_monitor_domctl_event.
>>>> e.g. X86-side handles XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR event
>>> enable/disable
>>>> * remove status_check
>>>>
>>>> Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com>
>>>>
>>>> ---
>>>> Changed since v3:
>>>> * monitor_domctl @ common/monitor.c:
>>>> - remove unused requested_status
>>>> - sanity check mop->event range to avoid left-shift undefined behavior
>>> Due to left-shift undefined behavior situations, shouldn't I also:
>>>
>>> * in X86 arch_monitor_get_capabilities: replace '1 <<' w/ '1U <<'
>> There's no undefinedness there, since the right side operands of
>> << are all constant. Using 1U here would be okay, but is not
>> strictly needed.
>
> I reasoned based on this ISO C99 quote:
> [for an E1 << E2 operation, ]
> "If E1 has a signed type and nonnegative value, and E1 × 2^E2 is
> representable in the result type, then that is the resulting value;
> otherwise, the behavior is undefined."
>
> I inferred that this means that code such as '(1 << 31)' would render
> undefined behavior, since (1 x 2^31) is not representable on 'int'.
> The standard doesn't seem to mention different behavior if the operands
> are constants.
> This would render undefined behavior if bit 31 of capabilities would be
> used @ some point, i.e. if one day someone would e.g. unknowingly:
> #define XEN_DOMCTL_MONITOR_EVENT_GRAVITATIONAL_WAVE 31
> Have I misinterpreted the 'representable in the result type' part?
No, that's all correct. It's just that right now no
XEN_DOMCTL_MONITOR_EVENT_* has value 31, and hence
there's only a very minor latent issue here (someone blindly
copying the existing 1 << ... without adding the necessary U at
that point; one might hope the compiler would then point this out
though).
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v4 2/2] xen/vm-events: Move parts of monitor_domctl code to common-side.
2016-02-16 12:34 ` Jan Beulich
@ 2016-02-16 13:03 ` Corneliu ZUZU
0 siblings, 0 replies; 13+ messages in thread
From: Corneliu ZUZU @ 2016-02-16 13:03 UTC (permalink / raw)
To: Jan Beulich
Cc: Tamas K Lengyel, Keir Fraser, Ian Campbell, Razvan Cojocaru,
Andrew Cooper, xen-devel, Stefano Stabellini
On 2/16/2016 2:34 PM, Jan Beulich wrote:
>>>> On 16.02.16 at 12:20, <czuzu@bitdefender.com> wrote:
>> On 2/16/2016 12:45 PM, Jan Beulich wrote:
>>>>>> On 16.02.16 at 09:13, <czuzu@bitdefender.com> wrote:
>>>> On 2/16/2016 9:08 AM, Corneliu ZUZU wrote:
>>>>> This patch moves monitor_domctl to common-side.
>>>>> Purpose: move what's common to common, prepare for implementation
>>>>> of such vm-events on ARM.
>>>>>
>>>>> * move get_capabilities to arch-side => arch_monitor_get_capabilities.
>>>>> * add arch-side monitor op handling function => arch_monitor_domctl_op.
>>>>> e.g. X86-side handles XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_REP op
>>>>> * add arch-side monitor event handling function => arch_monitor_domctl_event.
>>>>> e.g. X86-side handles XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR event
>>>> enable/disable
>>>>> * remove status_check
>>>>>
>>>>> Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com>
>>>>>
>>>>> ---
>>>>> Changed since v3:
>>>>> * monitor_domctl @ common/monitor.c:
>>>>> - remove unused requested_status
>>>>> - sanity check mop->event range to avoid left-shift undefined behavior
>>>> Due to left-shift undefined behavior situations, shouldn't I also:
>>>>
>>>> * in X86 arch_monitor_get_capabilities: replace '1 <<' w/ '1U <<'
>>> There's no undefinedness there, since the right side operands of
>>> << are all constant. Using 1U here would be okay, but is not
>>> strictly needed.
>> I reasoned based on this ISO C99 quote:
>> [for an E1 << E2 operation, ]
>> "If E1 has a signed type and nonnegative value, and E1 × 2^E2 is
>> representable in the result type, then that is the resulting value;
>> otherwise, the behavior is undefined."
>>
>> I inferred that this means that code such as '(1 << 31)' would render
>> undefined behavior, since (1 x 2^31) is not representable on 'int'.
>> The standard doesn't seem to mention different behavior if the operands
>> are constants.
>> This would render undefined behavior if bit 31 of capabilities would be
>> used @ some point, i.e. if one day someone would e.g. unknowingly:
>> #define XEN_DOMCTL_MONITOR_EVENT_GRAVITATIONAL_WAVE 31
>> Have I misinterpreted the 'representable in the result type' part?
> No, that's all correct. It's just that right now no
> XEN_DOMCTL_MONITOR_EVENT_* has value 31, and hence
> there's only a very minor latent issue here (someone blindly
> copying the existing 1 << ... without adding the necessary U at
> that point; one might hope the compiler would then point this out
> though).
>
> Jan
>
Ah, I see. Did a fast test earlier w/ GCC 5.1, unfortunately I think the
compiler doesn't
issue any warning in this situation, would have preferred a heads-up too
(couldn't even force it to do so).
(it would be nice if Xen shipped w/ a gravitational-waves detector
though....)
Corneliu.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v4 2/2] xen/vm-events: Move parts of monitor_domctl code to common-side.
2016-02-16 7:08 ` [PATCH v4 2/2] xen/vm-events: Move parts of monitor_domctl code to common-side Corneliu ZUZU
2016-02-16 8:13 ` Corneliu ZUZU
2016-02-16 10:54 ` Stefano Stabellini
@ 2016-02-16 14:10 ` Razvan Cojocaru
2016-02-16 16:02 ` Tamas K Lengyel
3 siblings, 0 replies; 13+ messages in thread
From: Razvan Cojocaru @ 2016-02-16 14:10 UTC (permalink / raw)
To: Corneliu ZUZU, xen-devel
Cc: Tamas K Lengyel, Keir Fraser, Ian Campbell, Andrew Cooper,
Stefano Stabellini, Jan Beulich
On 02/16/2016 09:08 AM, Corneliu ZUZU wrote:
> This patch moves monitor_domctl to common-side.
> Purpose: move what's common to common, prepare for implementation
> of such vm-events on ARM.
>
> * move get_capabilities to arch-side => arch_monitor_get_capabilities.
> * add arch-side monitor op handling function => arch_monitor_domctl_op.
> e.g. X86-side handles XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_REP op
> * add arch-side monitor event handling function => arch_monitor_domctl_event.
> e.g. X86-side handles XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR event enable/disable
> * remove status_check
>
> Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com>
>
> ---
> Changed since v3:
> * monitor_domctl @ common/monitor.c:
> - remove unused requested_status
> - sanity check mop->event range to avoid left-shift undefined behavior
> * arch_monitor_domctl_event: use ASSERT_UNREACHABLE() instead of bug warning
> * xen/monitor.h: replace includes w/ structs forward-declare, fix #ifndef
> ---
> MAINTAINERS | 1 +
> xen/arch/x86/monitor.c | 153 +++++++++++-------------------------------
> xen/common/Makefile | 1 +
> xen/common/domctl.c | 2 +-
> xen/common/monitor.c | 69 +++++++++++++++++++
> xen/include/asm-arm/monitor.h | 30 +++++++--
> xen/include/asm-x86/monitor.h | 53 +++++++++++++--
> xen/include/xen/monitor.h | 30 +++++++++
> 8 files changed, 217 insertions(+), 122 deletions(-)
> create mode 100644 xen/common/monitor.c
> create mode 100644 xen/include/xen/monitor.h
Fair enough.
Acked-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
Thanks,
Razvan
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v4 2/2] xen/vm-events: Move parts of monitor_domctl code to common-side.
2016-02-16 7:08 ` [PATCH v4 2/2] xen/vm-events: Move parts of monitor_domctl code to common-side Corneliu ZUZU
` (2 preceding siblings ...)
2016-02-16 14:10 ` Razvan Cojocaru
@ 2016-02-16 16:02 ` Tamas K Lengyel
2016-02-16 17:48 ` Corneliu ZUZU
3 siblings, 1 reply; 13+ messages in thread
From: Tamas K Lengyel @ 2016-02-16 16:02 UTC (permalink / raw)
To: Corneliu ZUZU
Cc: Keir Fraser, Ian Campbell, Razvan Cojocaru, Andrew Cooper,
Xen-devel, Stefano Stabellini, Jan Beulich
[-- Attachment #1.1: Type: text/plain, Size: 2663 bytes --]
>
>
> @@ -143,77 +72,75 @@ int monitor_domctl(struct domain *d, struct
> xen_domctl_monitor_op *mop)
>
> case XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR:
> {
>
So since we will now have two separate booleans, requested_status and
old_status and then manually verify they are opposite..
> - bool_t status = ad->monitor.mov_to_msr_enabled;
> + bool_t old_status = ad->monitor.mov_to_msr_enabled;
>
...here we should set the field to requested_status, not !old_status. While
they are technically equivalent, the code would read better to other way
around.
>
> - ad->monitor.mov_to_msr_enabled = !status;
> + ad->monitor.mov_to_msr_enabled = !old_status;
domain_unpause(d);
> break;
> }
>
> case XEN_DOMCTL_MONITOR_EVENT_SINGLESTEP:
> {
> - bool_t status = ad->monitor.singlestep_enabled;
> + bool_t old_status = ad->monitor.singlestep_enabled;
>
> - rc = status_check(mop, status);
> - if ( rc )
> - return rc;
> + if ( unlikely(old_status == requested_status) )
> + return -EEXIST;
>
> domain_pause(d);
>
Here as well..
> - ad->monitor.singlestep_enabled = !status;
> + ad->monitor.singlestep_enabled = !old_status;
> domain_unpause(d);
> break;
> }
>
> case XEN_DOMCTL_MONITOR_EVENT_SOFTWARE_BREAKPOINT:
> {
> - bool_t status = ad->monitor.software_breakpoint_enabled;
> + bool_t old_status = ad->monitor.software_breakpoint_enabled;
>
> - rc = status_check(mop, status);
> - if ( rc )
> - return rc;
> + if ( unlikely(old_status == requested_status) )
> + return -EEXIST;
>
> domain_pause(d);
>
..and here..
> - ad->monitor.software_breakpoint_enabled = !status;
> + ad->monitor.software_breakpoint_enabled = !old_status;
> domain_unpause(d);
> break;
> }
>
> case XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST:
> {
> - bool_t status = ad->monitor.guest_request_enabled;
> + bool_t old_status = ad->monitor.guest_request_enabled;
>
> - rc = status_check(mop, status);
> - if ( rc )
> - return rc;
> + if ( unlikely(old_status == requested_status) )
> + return -EEXIST;
>
> domain_pause(d);
> ad->monitor.guest_request_sync = mop->u.guest_request.sync;
>
..and here.
> - ad->monitor.guest_request_enabled = !status;
> + ad->monitor.guest_request_enabled = !old_status;
> domain_unpause(d);
> break;
> }
>
Otherwise the patch looks good.
Thanks,
Tamas
[-- Attachment #1.2: Type: text/html, Size: 4344 bytes --]
[-- Attachment #2: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v4 2/2] xen/vm-events: Move parts of monitor_domctl code to common-side.
2016-02-16 16:02 ` Tamas K Lengyel
@ 2016-02-16 17:48 ` Corneliu ZUZU
2016-02-16 17:53 ` Tamas K Lengyel
0 siblings, 1 reply; 13+ messages in thread
From: Corneliu ZUZU @ 2016-02-16 17:48 UTC (permalink / raw)
To: Tamas K Lengyel
Cc: Keir Fraser, Ian Campbell, Razvan Cojocaru, Andrew Cooper,
Xen-devel, Stefano Stabellini, Jan Beulich
[-- Attachment #1.1: Type: text/plain, Size: 3190 bytes --]
On 2/16/2016 6:02 PM, Tamas K Lengyel wrote:
>
>
> @@ -143,77 +72,75 @@ int monitor_domctl(struct domain *d, struct
> xen_domctl_monitor_op *mop)
>
> case XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR:
> {
>
>
> So since we will now have two separate booleans, requested_status and
> old_status and then manually verify they are opposite..
>
> - bool_t status = ad->monitor.mov_to_msr_enabled;
> + bool_t old_status = ad->monitor.mov_to_msr_enabled;
>
>
> ...here we should set the field to requested_status, not !old_status.
> While they are technically equivalent, the code would read better to
> other way around.
>
>
> - ad->monitor.mov_to_msr_enabled = !status;
> + ad->monitor.mov_to_msr_enabled = !old_status;
>
> domain_unpause(d);
> break;
> }
>
> case XEN_DOMCTL_MONITOR_EVENT_SINGLESTEP:
> {
> - bool_t status = ad->monitor.singlestep_enabled;
> + bool_t old_status = ad->monitor.singlestep_enabled;
>
> - rc = status_check(mop, status);
> - if ( rc )
> - return rc;
> + if ( unlikely(old_status == requested_status) )
> + return -EEXIST;
>
> domain_pause(d);
>
>
> Here as well..
>
> - ad->monitor.singlestep_enabled = !status;
> + ad->monitor.singlestep_enabled = !old_status;
> domain_unpause(d);
> break;
> }
>
> case XEN_DOMCTL_MONITOR_EVENT_SOFTWARE_BREAKPOINT:
> {
> - bool_t status = ad->monitor.software_breakpoint_enabled;
> + bool_t old_status = ad->monitor.software_breakpoint_enabled;
>
> - rc = status_check(mop, status);
> - if ( rc )
> - return rc;
> + if ( unlikely(old_status == requested_status) )
> + return -EEXIST;
>
> domain_pause(d);
>
>
> ..and here..
>
> - ad->monitor.software_breakpoint_enabled = !status;
> + ad->monitor.software_breakpoint_enabled = !old_status;
> domain_unpause(d);
> break;
> }
>
> case XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST:
> {
> - bool_t status = ad->monitor.guest_request_enabled;
> + bool_t old_status = ad->monitor.guest_request_enabled;
>
> - rc = status_check(mop, status);
> - if ( rc )
> - return rc;
> + if ( unlikely(old_status == requested_status) )
> + return -EEXIST;
>
> domain_pause(d);
> ad->monitor.guest_request_sync = mop->u.guest_request.sync;
>
>
> ..and here.
>
> - ad->monitor.guest_request_enabled = !status;
> + ad->monitor.guest_request_enabled = !old_status;
> domain_unpause(d);
> break;
> }
>
>
> Otherwise the patch looks good.
>
> Thanks,
> Tamas
>
Oh, right, that would look better. Shall I send a v5 then with that
change? (and if yes I guess it won't hurt if I also include the
left-shift sanity checks I mentioned I should have added (?))
Thanks,
Corneliu.
[-- Attachment #1.2: Type: text/html, Size: 7049 bytes --]
[-- Attachment #2: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v4 2/2] xen/vm-events: Move parts of monitor_domctl code to common-side.
2016-02-16 17:48 ` Corneliu ZUZU
@ 2016-02-16 17:53 ` Tamas K Lengyel
0 siblings, 0 replies; 13+ messages in thread
From: Tamas K Lengyel @ 2016-02-16 17:53 UTC (permalink / raw)
To: Corneliu ZUZU
Cc: Keir Fraser, Ian Campbell, Razvan Cojocaru, Andrew Cooper,
Xen-devel, Stefano Stabellini, Jan Beulich
[-- Attachment #1.1: Type: text/plain, Size: 3270 bytes --]
On Tue, Feb 16, 2016 at 10:48 AM, Corneliu ZUZU <czuzu@bitdefender.com>
wrote:
> On 2/16/2016 6:02 PM, Tamas K Lengyel wrote:
>
>
>> @@ -143,77 +72,75 @@ int monitor_domctl(struct domain *d, struct
>> xen_domctl_monitor_op *mop)
>>
>> case XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR:
>> {
>>
>
> So since we will now have two separate booleans, requested_status and
> old_status and then manually verify they are opposite..
>
>
>> - bool_t status = ad->monitor.mov_to_msr_enabled;
>> + bool_t old_status = ad->monitor.mov_to_msr_enabled;
>>
>
> ...here we should set the field to requested_status, not !old_status.
> While they are technically equivalent, the code would read better to other
> way around.
>
>
>>
>> - ad->monitor.mov_to_msr_enabled = !status;
>> + ad->monitor.mov_to_msr_enabled = !old_status;
>
> domain_unpause(d);
>> break;
>> }
>>
>> case XEN_DOMCTL_MONITOR_EVENT_SINGLESTEP:
>> {
>> - bool_t status = ad->monitor.singlestep_enabled;
>> + bool_t old_status = ad->monitor.singlestep_enabled;
>>
>> - rc = status_check(mop, status);
>> - if ( rc )
>> - return rc;
>> + if ( unlikely(old_status == requested_status) )
>> + return -EEXIST;
>>
>> domain_pause(d);
>>
>
> Here as well..
>
>
>> - ad->monitor.singlestep_enabled = !status;
>> + ad->monitor.singlestep_enabled = !old_status;
>> domain_unpause(d);
>> break;
>> }
>>
>> case XEN_DOMCTL_MONITOR_EVENT_SOFTWARE_BREAKPOINT:
>> {
>> - bool_t status = ad->monitor.software_breakpoint_enabled;
>> + bool_t old_status = ad->monitor.software_breakpoint_enabled;
>>
>> - rc = status_check(mop, status);
>> - if ( rc )
>> - return rc;
>> + if ( unlikely(old_status == requested_status) )
>> + return -EEXIST;
>>
>> domain_pause(d);
>>
>
> ..and here..
>
>
>> - ad->monitor.software_breakpoint_enabled = !status;
>> + ad->monitor.software_breakpoint_enabled = !old_status;
>> domain_unpause(d);
>> break;
>> }
>>
>> case XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST:
>> {
>> - bool_t status = ad->monitor.guest_request_enabled;
>> + bool_t old_status = ad->monitor.guest_request_enabled;
>>
>> - rc = status_check(mop, status);
>> - if ( rc )
>> - return rc;
>> + if ( unlikely(old_status == requested_status) )
>> + return -EEXIST;
>>
>> domain_pause(d);
>> ad->monitor.guest_request_sync = mop->u.guest_request.sync;
>>
>
> ..and here.
>
>
>> - ad->monitor.guest_request_enabled = !status;
>> + ad->monitor.guest_request_enabled = !old_status;
>> domain_unpause(d);
>> break;
>> }
>>
>
> Otherwise the patch looks good.
>
> Thanks,
> Tamas
>
>
> Oh, right, that would look better. Shall I send a v5 then with that
> change? (and if yes I guess it won't hurt if I also include the left-shift
> sanity checks I mentioned I should have added (?))
>
Please do send another revision with these changes. As I understood Jan
prefers the sanity-checks to be added as a separate patch, so do that.
Thanks,
Tamas
[-- Attachment #1.2: Type: text/html, Size: 7308 bytes --]
[-- Attachment #2: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2016-02-16 17:53 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-02-16 7:07 [PATCH v4 0/2] Vm-events: move monitor vm-events code to common-side Corneliu ZUZU
2016-02-16 7:07 ` [PATCH v4 1/2] xen/arm: fix file comments Corneliu ZUZU
2016-02-16 7:08 ` [PATCH v4 2/2] xen/vm-events: Move parts of monitor_domctl code to common-side Corneliu ZUZU
2016-02-16 8:13 ` Corneliu ZUZU
2016-02-16 10:45 ` Jan Beulich
2016-02-16 11:20 ` Corneliu ZUZU
2016-02-16 12:34 ` Jan Beulich
2016-02-16 13:03 ` Corneliu ZUZU
2016-02-16 10:54 ` Stefano Stabellini
2016-02-16 14:10 ` Razvan Cojocaru
2016-02-16 16:02 ` Tamas K Lengyel
2016-02-16 17:48 ` Corneliu ZUZU
2016-02-16 17:53 ` Tamas K Lengyel
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.