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

[Tamas K Lengyel <tamas@tklengyel.com>]

[-- Attachment #1.1: Type: text/plain, Size: 23268 bytes --]

On Mon, Feb 8, 2016 at 9:57 AM, Corneliu ZUZU <czuzu@bitdefender.com> 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@bitdefender.com>
> ---
>  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@tklengyel.com)
> - *
> - * 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@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 <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@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>              /* 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

[-- Attachment #1.2: Type: text/html, Size: 30100 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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