Re: [Xen-devel] [PATCH v4 2/2] xen/vm-events: Move parts of monitor_domctl code to common-side.

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

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@xxxxxxxxxxxxxxx>

For the ARM part

Acked-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>


> 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@xxxxxxxxxxxxx>
>  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@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
> @@ -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@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>
> +#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@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
> @@ -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@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
> @@ -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@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/>.
> + */
> +
> +#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
> 

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