Re: [Xen-devel] [PATCH 3/7] xen/vm-events: Move monitor_domctl to common-side.

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

On Mon, Feb 8, 2016 at 9:57 AM, Corneliu ZUZU <czuzu@xxxxxxxxxxxxxxx> wrote:

1. Kconfig:
 * Added Kconfigs for common monitor vm-events:
 # see files: common/Kconfig, x86/Kconfig
  HAS_VM_EVENT_WRITE_CTRLREG
  HAS_VM_EVENT_SINGLESTEP
  HAS_VM_EVENT_SOFTWARE_BREAKPOINT
  HAS_VM_EVENT_GUEST_REQUEST

2. Moved monitor_domctl from arch-side to common-side
 2.1. Moved arch/x86/monitor.c to common/monitor.c
  # see files: arch/x86/Makefile, xen/common/Makefile, xen/common/monitor.c
  # changes:
    - removed status_check (we would have had to duplicate it in X86
      arch_monitor_domctl_event otherwise)
    - moved get_capabilities to arch-side (arch_monitor_get_capabilities)
    - moved XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_REP to arch-side (see
      arch_monitor_domctl_op)
    - put XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR to x86-side (see
      arch_monitor_domctl_event)
    - surrounded switch cases w/ CONFIG_HAS_VM_EVENT_*

 2.2. Moved asm-x86/monitor.h to xen/monitor.h
  # see files: arch/x86/hvm/event.c, arch/x86/hvm/hvm.c,
        Âarch/x86/hvm/vmx/vmx.c, xen/common/domctl.c

 2.3. Removed asm-arm/monitor.h (no longer needed)

3. Added x86/monitor_x86.c => will rename in next commit to monitor.c (not done
in this commit to avoid git seeing this as being the modified old monitor.c =>
keeping the same name would have rendered an unnecessarily bulky diff)
  # see files: arch/x86/Makefile
  # implements X86-side arch_monitor_domctl_event

4. Added asm-x86/monitor_arch.h, asm-arm/monitor_arch.h (renamed to
monitor.h in next commit, reason is the same as @ (3.).
  # define/implement: arch_monitor_get_capabilities, arch_monitor_domctl_op
    and arch_monitor_domctl_event

Signed-off-by: Corneliu ZUZU <czuzu@xxxxxxxxxxxxxxx>
---
Âxen/arch/x86/Kconfig       Â| Â4 +
Âxen/arch/x86/Makefile       | Â2 +-
Âxen/arch/x86/hvm/event.c     Â| Â2 +-
Âxen/arch/x86/hvm/hvm.c      Â| Â2 +-
Âxen/arch/x86/hvm/vmx/vmx.c    Â| Â2 +-
Âxen/arch/x86/monitor.c      Â| 228 -------------------------------------
Âxen/arch/x86/monitor_x86.c    Â| 72 ++++++++++++
Âxen/common/Kconfig        Â| 20 ++++
Âxen/common/Makefile        | Â1 +
Âxen/common/domctl.c        | Â2 +-
Âxen/common/monitor.c       Â| 203 +++++++++++++++++++++++++++++++++
Âxen/include/asm-arm/monitor.h   | 33 ------
Âxen/include/asm-arm/monitor_arch.h |Â 53 +++++++++
Âxen/include/asm-x86/monitor.h   | 31 -----
Âxen/include/asm-x86/monitor_arch.h |Â 74 ++++++++++++
Âxen/include/xen/monitor.h     | 36 ++++++
Â16 files changed, 468 insertions(+), 297 deletions(-)
Âdelete mode 100644 xen/arch/x86/monitor.c
Âcreate mode 100644 xen/arch/x86/monitor_x86.c
Âcreate mode 100644 xen/common/monitor.c
Âdelete mode 100644 xen/include/asm-arm/monitor.h
Âcreate mode 100644 xen/include/asm-arm/monitor_arch.h
Âdelete mode 100644 xen/include/asm-x86/monitor.h
Âcreate mode 100644 xen/include/asm-x86/monitor_arch.h
Âcreate mode 100644 xen/include/xen/monitor.h

diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
index 3a90f47..e46be1b 100644
--- a/xen/arch/x86/Kconfig
+++ b/xen/arch/x86/Kconfig
@@ -14,6 +14,10 @@ config X86
    select HAS_MEM_ACCESS
    select HAS_MEM_PAGING
    select HAS_MEM_SHARING
+Â Â Â Âselect HAS_VM_EVENT_WRITE_CTRLREG
+Â Â Â Âselect HAS_VM_EVENT_SINGLESTEP
+Â Â Â Âselect HAS_VM_EVENT_SOFTWARE_BREAKPOINT
+Â Â Â Âselect HAS_VM_EVENT_GUEST_REQUEST
    select HAS_NS16550
    select HAS_PASSTHROUGH
    select HAS_PCI
diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
index 8e6e901..6e80cf0 100644
--- a/xen/arch/x86/Makefile
+++ b/xen/arch/x86/Makefile
@@ -36,7 +36,7 @@ obj-y += microcode_intel.o
Â# This must come after the vendor specific files.
Âobj-y += microcode.o
Âobj-y += mm.o x86_64/mm.o
-obj-y += monitor.o
+obj-y += monitor_x86.o
Âobj-y += mpparse.o
Âobj-y += nmi.o
Âobj-y += numa.o
diff --git a/xen/arch/x86/hvm/event.c b/xen/arch/x86/hvm/event.c
index 9dc533b..5ffc485 100644
--- a/xen/arch/x86/hvm/event.c
+++ b/xen/arch/x86/hvm/event.c
@@ -20,8 +20,8 @@

Â#include <xen/vm_event.h>
Â#include <xen/paging.h>
+#include <xen/monitor.h>
Â#include <asm/hvm/event.h>
-#include <asm/monitor.h>
Â#include <asm/altp2m.h>
Â#include <public/vm_event.h>

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 35ec6c9..9063eb5 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -32,6 +32,7 @@
Â#include <xen/guest_access.h>
Â#include <xen/event.h>
Â#include <xen/paging.h>
+#include <xen/monitor.h>
Â#include <xen/cpu.h>
Â#include <xen/wait.h>
Â#include <xen/mem_access.h>
@@ -51,7 +52,6 @@
Â#include <asm/traps.h>
Â#include <asm/mc146818rtc.h>
Â#include <asm/mce.h>
-#include <asm/monitor.h>
Â#include <asm/hvm/hvm.h>
Â#include <asm/hvm/vpt.h>
Â#include <asm/hvm/support.h>
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 1a24788..f7708fe 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -25,6 +25,7 @@
Â#include <xen/domain_page.h>
Â#include <xen/hypercall.h>
Â#include <xen/perfc.h>
+#include <xen/monitor.h>
Â#include <asm/current.h>
Â#include <asm/io.h>
Â#include <asm/iocap.h>
@@ -57,7 +58,6 @@
Â#include <asm/hvm/nestedhvm.h>
Â#include <asm/altp2m.h>
Â#include <asm/event.h>
-#include <asm/monitor.h>
Â#include <public/arch-x86/cpuid.h>

Âstatic bool_t __initdata opt_force_ept;
diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c
deleted file mode 100644
index 1d43880..0000000
--- a/xen/arch/x86/monitor.c
+++ /dev/null
@@ -1,228 +0,0 @@
-/*
- * arch/x86/monitor.c
- *
- * Architecture-specific monitor_op domctl handler.
- *
- * Copyright (c) 2015 Tamas K Lengyel (tamas@xxxxxxxxxxxxx)
- *
- * 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/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>
-
-/*
- * 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 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;
-
-Â Â switch ( mop->event )
-Â Â {
-Â Â case XEN_DOMCTL_MONITOR_EVENT_WRITE_CTRLREG:
-Â Â {
-Â Â Â Â unsigned int ctrlreg_bitmask =
-Â Â Â Â Â Â monitor_ctrlreg_bitmask(mop->u.mov_to_cr.index);
-Â Â Â Â bool_t status =
-Â Â Â Â Â Â !!(ad->monitor.write_ctrlreg_enabled & ctrlreg_bitmask);
-Â Â Â Â struct vcpu *v;
-
-Â Â Â Â rc = status_check(mop, status);
-Â Â Â Â if ( rc )
-Â Â Â Â Â Â return rc;
-
-Â Â Â Â domain_pause(d);
-
-Â Â Â Â if ( mop->u.mov_to_cr.sync )
-Â Â Â Â Â Â ad->monitor.write_ctrlreg_sync |= ctrlreg_bitmask;
-Â Â Â Â else
-Â Â Â Â Â Â ad->monitor.write_ctrlreg_sync &= ~ctrlreg_bitmask;
-
-Â Â Â Â if ( mop->u.mov_to_cr.onchangeonly )
-Â Â Â Â Â Â ad->monitor.write_ctrlreg_onchangeonly |= ctrlreg_bitmask;
-Â Â Â Â else
-Â Â Â Â Â Â ad->monitor.write_ctrlreg_onchangeonly &= ~ctrlreg_bitmask;
-
-Â Â Â Â if ( !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 */
-Â Â Â Â Â Â for_each_vcpu ( d, v )
-Â Â Â Â Â Â Â Â hvm_update_guest_cr(v, 0);
-
-Â Â Â Â domain_unpause(d);
-
-Â Â Â Â break;
-Â Â }
-
-Â Â case XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR:
-Â Â {
-Â Â Â Â bool_t status = ad->monitor.mov_to_msr_enabled;
-
-Â Â Â Â rc = status_check(mop, status);
-Â Â Â Â if ( rc )
-Â Â Â Â Â Â return rc;
-
-Â Â Â Â if ( mop->op == XEN_DOMCTL_MONITOR_OP_ENABLE &&
-Â Â Â Â Â Â Â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;
-Â Â Â Â else
-Â Â Â Â Â Â ad->monitor.mov_to_msr_extended = 0;
-
-Â Â Â Â ad->monitor.mov_to_msr_enabled = !status;
-Â Â Â Â domain_unpause(d);
-Â Â Â Â break;
-Â Â }
-
-Â Â case XEN_DOMCTL_MONITOR_EVENT_SINGLESTEP:
-Â Â {
-Â Â Â Â bool_t status = ad->monitor.singlestep_enabled;
-
-Â Â Â Â rc = status_check(mop, status);
-Â Â Â Â if ( rc )
-Â Â Â Â Â Â return rc;
-
-Â Â Â Â domain_pause(d);
-Â Â Â Â ad->monitor.singlestep_enabled = !status;
-Â Â Â Â domain_unpause(d);
-Â Â Â Â break;
-Â Â }
-
-Â Â case XEN_DOMCTL_MONITOR_EVENT_SOFTWARE_BREAKPOINT:
-Â Â {
-Â Â Â Â bool_t status = ad->monitor.software_breakpoint_enabled;
-
-Â Â Â Â rc = status_check(mop, status);
-Â Â Â Â if ( rc )
-Â Â Â Â Â Â return rc;
-
-Â Â Â Â domain_pause(d);
-Â Â Â Â ad->monitor.software_breakpoint_enabled = !status;
-Â Â Â Â domain_unpause(d);
-Â Â Â Â break;
-Â Â }
-
-Â Â case XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST:
-Â Â {
-Â Â Â Â bool_t status = ad->monitor.guest_request_enabled;
-
-Â Â Â Â rc = status_check(mop, status);
-Â Â Â Â if ( rc )
-Â Â Â Â Â Â return rc;
-
-Â Â Â Â domain_pause(d);
-Â Â Â Â ad->monitor.guest_request_sync = mop->u.guest_request.sync;
-Â Â Â Â ad->monitor.guest_request_enabled = !status;
-Â Â Â Â domain_unpause(d);
-Â Â Â Â break;
-Â Â }
-
-Â Â default:
-Â Â Â Â return -EOPNOTSUPP;
-
-Â Â };
-
-Â Â return 0;
-}
-
-/*
- * Local variables:
- * mode: C
- * c-file-style: "BSD"
- * c-basic-offset: 4
- * indent-tabs-mode: nil
- * End:
- */
diff --git a/xen/arch/x86/monitor_x86.c b/xen/arch/x86/monitor_x86.c
new file mode 100644
index 0000000..d19fd15
--- /dev/null
+++ b/xen/arch/x86/monitor_x86.c
@@ -0,0 +1,72 @@
+/*
+ * arch/x86/monitor_x86.c
+ *
+ * Arch-specific monitor_op domctl handler.
+ *
+ * Copyright (c) 2015 Tamas K Lengyel (tamas@xxxxxxxxxxxxx)
+ * 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 <asm/monitor_arch.h>
+
+bool_t arch_monitor_domctl_event(struct domain *d,
+Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âstruct xen_domctl_monitor_op *mop,
+Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âint *rc)
+{
+Â Â struct arch_domain *ad = &d->arch;
+Â Â bool_t requested_status = (XEN_DOMCTL_MONITOR_OP_ENABLE == mop->op);
+
+Â Â switch ( mop->event )
+Â Â {
+Â Â Â Â case XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR:
+Â Â Â Â {
+Â Â Â Â Â Â bool_t old_status = ad->monitor.mov_to_msr_enabled;
+
+Â Â Â Â Â Â if ( unlikely(old_status == requested_status) )
+Â Â Â Â Â Â Â Â return -EEXIST;
+
+Â Â Â Â Â Â if ( XEN_DOMCTL_MONITOR_OP_ENABLE == mop->op &&
+Â Â Â Â Â Â Â Â Âmop->u.mov_to_msr.extended_capture &&
+Â Â Â Â Â Â Â Â Â!hvm_enable_msr_exit_interception(d) )
+Â Â Â Â Â Â Â Â return -EOPNOTSUPP;
+
+Â Â Â Â Â Â domain_pause(d);
+
+Â Â Â Â Â Â if ( XEN_DOMCTL_MONITOR_OP_ENABLE == mop->op &&
+Â Â Â Â Â Â Â Â Â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 = !old_status;
+Â Â Â Â Â Â domain_unpause(d);
+Â Â Â Â Â Â break;
+Â Â Â Â }
+
+Â Â Â Â default:
+Â Â Â Â Â Â return 0;
+Â Â }
+
+Â Â return 1;
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/common/Kconfig b/xen/common/Kconfig
index 6f404b4..172da13 100644
--- a/xen/common/Kconfig
+++ b/xen/common/Kconfig
@@ -36,6 +36,26 @@ config HAS_MEM_PAGING
Âconfig HAS_MEM_SHARING
    bool

+config HAS_VM_EVENT_WRITE_CTRLREG
+Â Â Â Âbool
+Â Â Â Â---help---
+Â Â Â Â ÂSelect if ctrl-reg write monitor vm-events are supported
+
+config HAS_VM_EVENT_SINGLESTEP
+Â Â Â Âbool
+Â Â Â Â---help---
+Â Â Â Â ÂSelect if single-step monitor vm-events are supported
+
+config HAS_VM_EVENT_SOFTWARE_BREAKPOINT
+Â Â Â Âbool
+Â Â Â Â---help---
+Â Â Â Â ÂSelect if software-breakpoint monitor vm-events are supported
+
+config HAS_VM_EVENT_GUEST_REQUEST
+Â Â Â Âbool
+Â Â Â Â---help---
+Â Â Â Â ÂSelect if guest-request monitor vm-events are supported
+
Â# Select HAS_PDX if PDX is supported
Âconfig HAS_PDX
    bool
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 121a34a..4b1dec1 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..7bbeba5
--- /dev/null
+++ b/xen/common/monitor.c
@@ -0,0 +1,203 @@
+/*
+ * xen/common/monitor.c
+ *
+ * Common monitor_op domctl handler.
+ *
+ * Copyright (c) 2015 Tamas K Lengyel (tamas@xxxxxxxxxxxxx)
+ * 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>Â Â Â Â Â Â Â /* for domain_pause, ... */
+#include <xen/config.h>Â Â Â Â Â Â Â/* for XENLOG_WARNING */
+#include <xsm/xsm.h>
+#include <public/domctl.h>
+
+#include <asm/monitor_arch.h>Â Â Â Â/* for monitor_arch_# */
+
+#if CONFIG_X86

Wouldn't it be safe to include these headers on ARM as well?

Â

+#include <public/vm_event.h>Â Â Â Â /* for VM_EVENT_X86_CR3 */
+#include <asm/hvm/hvm.h>Â Â Â Â Â Â /* for hvm_update_guest_cr, ... */
+#endif
+
+int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *mop)
+{
+Â Â int rc;
+Â Â bool_t requested_status;
+
+Â Â 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;
+
+Â Â if ( unlikely(mop->op != XEN_DOMCTL_MONITOR_OP_ENABLE &&
+Â Â Â Â Â Â Â Â Â mop->op != XEN_DOMCTL_MONITOR_OP_DISABLE) )
+Â Â {

Please make a switch on mop->op instead where the default case is to call arch_monitor_domctl. It would be a bit easier to follow.

+Â Â Â Â if ( XEN_DOMCTL_MONITOR_OP_GET_CAPABILITIES == mop->op )
+Â Â Â Â {
+Â Â Â Â Â Â mop->event = arch_monitor_get_capabilities(d);
+Â Â Â Â Â Â return 0;
+Â Â Â Â }
+

"proly"->probably?

Â

+Â Â Â Â /* The monitor op is proly handled on the arch-side. */
+Â Â Â Â if ( likely(arch_monitor_domctl_op(d, mop, &rc)) )
+Â Â Â Â Â Â return rc;
+
+Â Â Â Â /* unrecognized op */
+Â Â Â Â return -EOPNOTSUPP;
+Â Â }
+
+Â Â /* Check if event type is available. */
+Â Â if ( unlikely(!(arch_monitor_get_capabilities(d) & (1 << mop->event))) )
+Â Â Â Â return -EOPNOTSUPP;
+
+Â Â requested_status = (XEN_DOMCTL_MONITOR_OP_ENABLE == mop->op);
+
+Â Â switch ( mop->event )
+Â Â {

Â

Empty line not needed.

+
+#if CONFIG_HAS_VM_EVENT_WRITE_CTRLREG

Emtpy line not needed.

+
+Â Â case XEN_DOMCTL_MONITOR_EVENT_WRITE_CTRLREG:
+Â Â {
+Â Â Â Â struct arch_domain *ad = &d->arch;
+Â Â Â Â unsigned int ctrlreg_bitmask =
+Â Â Â Â Â Â monitor_ctrlreg_bitmask(mop->u.mov_to_cr.index);
+Â Â Â Â bool_t old_status =
+Â Â Â Â Â Â !!(ad->monitor.write_ctrlreg_enabled & ctrlreg_bitmask);
+
+Â Â Â Â if ( unlikely(old_status == requested_status) )
+Â Â Â Â Â Â return -EEXIST;
+
+Â Â Â Â domain_pause(d);
+
+Â Â Â Â if ( mop->u.mov_to_cr.sync )
+Â Â Â Â Â Â ad->monitor.write_ctrlreg_sync |= ctrlreg_bitmask;
+Â Â Â Â else
+Â Â Â Â Â Â ad->monitor.write_ctrlreg_sync &= ~ctrlreg_bitmask;
+
+Â Â Â Â if ( mop->u.mov_to_cr.onchangeonly )
+Â Â Â Â Â Â ad->monitor.write_ctrlreg_onchangeonly |= ctrlreg_bitmask;
+Â Â Â Â else
+Â Â Â Â Â Â ad->monitor.write_ctrlreg_onchangeonly &= ~ctrlreg_bitmask;
+
+Â Â Â Â if ( !old_status )
+Â Â Â Â Â Â ad->monitor.write_ctrlreg_enabled |= ctrlreg_bitmask;
+Â Â Â Â else
+Â Â Â Â Â Â ad->monitor.write_ctrlreg_enabled &= ~ctrlreg_bitmask;
+

So I don't particularly like this #if check here. IMHO this should be done in arch-specific function that you call from here that is just empty for ARM. It could be a static inline function as it's rather small.
Â

+#if CONFIG_X86

+Â Â Â Â 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);
+Â Â Â Â }
+#endif
+
+Â Â Â Â domain_unpause(d);
+
+Â Â Â Â break;
+Â Â }
+
+#endif // HAS_VM_EVENT_WRITE_CTRLREG
+
+#if CONFIG_HAS_VM_EVENT_SINGLESTEP

Emtpy line not needed.

+
+Â Â case XEN_DOMCTL_MONITOR_EVENT_SINGLESTEP:
+Â Â {
+Â Â Â Â struct arch_domain *ad = &d->arch;
+Â Â Â Â bool_t old_status = ad->monitor.singlestep_enabled;
+
+Â Â Â Â if ( unlikely(old_status == requested_status) )
+Â Â Â Â Â Â return -EEXIST;
+
+Â Â Â Â domain_pause(d);
+Â Â Â Â ad->monitor.singlestep_enabled = !old_status;
+Â Â Â Â domain_unpause(d);
+Â Â Â Â break;
+Â Â }
+
+#endif // HAS_VM_EVENT_SINGLESTEP
+
+#if CONFIG_HAS_VM_EVENT_SOFTWARE_BREAKPOINT

Emtpy line not needed.

+
+Â Â case XEN_DOMCTL_MONITOR_EVENT_SOFTWARE_BREAKPOINT:
+Â Â {
+Â Â Â Â struct arch_domain *ad = &d->arch;
+Â Â Â Â bool_t old_status = ad->monitor.software_breakpoint_enabled;
+
+Â Â Â Â if ( unlikely(old_status == requested_status) )
+Â Â Â Â Â Â return -EEXIST;
+
+Â Â Â Â domain_pause(d);
+Â Â Â Â ad->monitor.software_breakpoint_enabled = !old_status;
+Â Â Â Â domain_unpause(d);
+Â Â Â Â break;
+Â Â }
+
+#endif // HAS_VM_EVENT_SOFTWARE_BREAKPOINT
+
+#if CONFIG_HAS_VM_EVENT_GUEST_REQUEST

Emtpy line not needed.

+
+Â Â case XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST:
+Â Â {
+Â Â Â Â struct arch_domain *ad = &d->arch;
+Â Â Â Â bool_t old_status = ad->monitor.guest_request_enabled;
+
+Â Â Â Â 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 = !old_status;
+Â Â Â Â domain_unpause(d);
+Â Â Â Â break;
+Â Â }

Looks good otherwise.

Thanks,

Tamas

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel