Re: [RFC PATCH V3 10/12] xen: Introduce monitor_op domctl

["Jan Beulich" <JBeulich@suse.com>]

>>> On 29.01.15 at 22:46, <tamas.lengyel@zentific.com> wrote:
> --- /dev/null
> +++ b/xen/arch/x86/monitor.c
> @@ -0,0 +1,197 @@
> +/*
> + * 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, write to the
> + * Free Software Foundation, Inc., 59 Temple Place - Suite 330,
> + * Boston, MA 021110-1307, USA.
> + */
> +
> +#include <xen/config.h>
> +#include <xen/sched.h>
> +#include <xen/mm.h>
> +#include <asm/domain.h>
> +
> +#define DISABLE_OPTION(option)          \
> +    if ( !option->enabled )             \
> +        return -EFAULT;                 \
> +    memset(option, 0, sizeof(*option))

This needs to be wrapped in either ({ }) or do { } while (0), allowing
you to drop currently necessary (but suggested to be omitted by
./CODING_STYLE) braces around (some of) its uses below.

> +int monitor_domctl(struct xen_domctl_monitor_op *domctl, struct domain *d)
> +{
> +    /*
> +     * At the moment only HVM domains are supported. However, event delivery
> +     * could be extended to PV domains. See comments below.
> +     */
> +    if ( !is_hvm_domain(d) )
> +        return -ENOSYS;
> +
> +    if ( domctl->op != XEN_DOMCTL_MONITOR_OP_ENABLE &&
> +         domctl->op != XEN_DOMCTL_MONITOR_OP_DISABLE )
> +        return -EFAULT;
> +
> +    switch ( domctl->subop )
> +    {
> +    case XEN_DOMCTL_MONITOR_SUBOP_MOV_TO_CR0:
> +    {
> +        /* Note: could be supported on PV domains. */
> +        struct mov_to_cr0 *options = &d->arch.monitor_options.mov_to_cr0;
> +
> +        if ( domctl->op == XEN_DOMCTL_MONITOR_OP_ENABLE )
> +        {
> +            if ( options->enabled )
> +                return -EBUSY;
> +
> +            options->enabled = 1;
> +            options->sync = domctl->options.mov_to_cr0.sync;
> +            options->onchangeonly = domctl->options.mov_to_cr0.onchangeonly;

Shouldn't you set "->enabled" last, after a suitable barrier (and with
the consuming sides using suitable barriers too)? Or are you missing
a domain_pause() here?

> +        }
> +        else
> +        {
> +            DISABLE_OPTION(options);
> +        }
> +        break;
> +    }
> +    case XEN_DOMCTL_MONITOR_SUBOP_MOV_TO_CR3:

Please consistently have blank lines between individual, not falling
through cases.

> +    default:
> +        return -EFAULT;

Certainly not.

> @@ -1179,6 +1180,16 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>      }
>      break;
>  
> +    case XEN_DOMCTL_monitor_op:
> +    {
> +        ret = -EPERM;
> +        if ( current->domain == d )
> +            break;
> +
> +        ret = monitor_domctl(&op->u.monitor_op, d);
> +    }
> +    break;

Pointless braces.

> --- a/xen/common/vm_event.c
> +++ b/xen/common/vm_event.c
> @@ -617,16 +617,10 @@ int vm_event_domctl(struct domain *d, xen_domctl_vm_event_op_t *vec,
>          switch( vec->op )
>          {
>          case XEN_DOMCTL_VM_EVENT_OP_MONITOR_ENABLE:
> -        case XEN_DOMCTL_VM_EVENT_OP_MONITOR_ENABLE_INTROSPECTION:
>          {
>              rc = vm_event_enable(d, vec, ved, _VPF_mem_access,
>                                      HVM_PARAM_MONITOR_RING_PFN,
>                                      mem_access_notification);
> -
> -            if ( vec->op == XEN_DOMCTL_VM_EVENT_OP_MONITOR_ENABLE_INTROSPECTION
> -                 && !rc )
> -                p2m_setup_introspection(d);
> -
>          }
>          break;

The braces should now be removed too.

> @@ -635,7 +629,6 @@ int vm_event_domctl(struct domain *d, xen_domctl_vm_event_op_t *vec,
>              if ( ved->ring_page )
>              {
>                  rc = vm_event_disable(d, ved);
> -                d->arch.hvm_domain.introspection_enabled = 0;
>              }

And again. Please go through all of the modification the series makes
and adjust for coding style.

> --- a/xen/include/asm-x86/domain.h
> +++ b/xen/include/asm-x86/domain.h
> @@ -236,6 +236,41 @@ struct time_scale {
>      u32 mul_frac;
>  };
>  
> +/************************************************/
> +/*            monitor event options             */
> +/************************************************/
> +struct mov_to_cr0 {
> +    uint8_t enabled;
> +    uint8_t sync;
> +    uint8_t onchangeonly;
> +};
> +
> +struct mov_to_cr3 {
> +    uint8_t enabled;
> +    uint8_t sync;
> +    uint8_t onchangeonly;
> +};
> +
> +struct mov_to_cr4 {
> +    uint8_t enabled;
> +    uint8_t sync;
> +    uint8_t onchangeonly;
> +};

As long as they're identical, these should just be a single
struct mov_to_cr, allowing (afaict) further abstraction elsewhere.

> +struct mov_to_msr {
> +    uint8_t enabled;
> +    uint8_t extended_capture;
> +};
> +
> +struct singlestep {
> +    uint8_t enabled;
> +};
> +
> +struct software_breakpoint {
> +    uint8_t enabled;
> +};

These may also better be a common struct debug_event.

> +
> +
>  struct pv_domain

Just a single blank line please.

> --- /dev/null
> +++ b/xen/include/asm-x86/monitor.h
> @@ -0,0 +1,9 @@
> +#ifndef __ASM_X86_MONITOR_H__
> +#define __ASM_X86_MONITOR_H__
> +
> +#include <xen/config.h>

Not needed anywhere. Please drop such inclusions from the series.

> @@ -1001,6 +1000,58 @@ struct xen_domctl_psr_cmt_op {
>  typedef struct xen_domctl_psr_cmt_op xen_domctl_psr_cmt_op_t;
>  DEFINE_XEN_GUEST_HANDLE(xen_domctl_psr_cmt_op_t);
>  
> +/*  XEN_DOMCTL_MONITOR_*
> + *
> + * Enable/disable monitoring various VM events.
> + * This domctl configures what events will be reported to helper apps
> + * via the ring buffer "MONITOR". The ring has to be first enabled
> + * with the domctl XEN_DOMCTL_VM_EVENT_OP_MONITOR.
> + *
> + * NOTICE: mem_access events are also delivered via the "MONITOR" ring buffer;
> + * however, enabling/disabling those events is performed with the use of
> + * memory_op hypercalls!
> + *
> + */

Stray blank comment line.

> +struct xen_domctl_monitor_op {
> +    uint32_t op; /* XEN_DOMCTL_MONITOR_OP_* */
> +    uint32_t subop; /* XEN_DOMCTL_MONITOR_SUBOP_* */
> +
> +    /*
> +     * Further options when issuing XEN_DOMCTL_MONITOR_OP_ENABLE.
> +     */
> +    union {
> +        struct {
> +            uint8_t sync; /* Pause vCPU until response */
> +            uint8_t onchangeonly; /* Send event only on a change of value */
> +            uint8_t pad[6];
> +        } mov_to_cr0, mov_to_cr3, mov_to_cr4;
> +
> +        /* Enable the capture of msr events on
> +            MSR_IA32_SYSENTER_EIP
> +            MSR_IA32_SYSENTER_ESP
> +            MSR_IA32_SYSENTER_CS
> +            MSR_IA32_MC0_CTL
> +            MSR_STAR
> +            MSR_LSTAR */

Coding style.

> --- a/xen/include/public/hvm/params.h
> +++ b/xen/include/public/hvm/params.h
> @@ -162,21 +162,6 @@
>   */
>  #define HVM_PARAM_ACPI_IOPORTS_LOCATION 19
>  
> -/* Enable blocking memory events, async or sync (pause vcpu until response) 
> - * onchangeonly indicates messages only on a change of value */
> -#define HVM_PARAM_MEMORY_EVENT_CR0          20
> -#define HVM_PARAM_MEMORY_EVENT_CR3          21
> -#define HVM_PARAM_MEMORY_EVENT_CR4          22
> -#define HVM_PARAM_MEMORY_EVENT_INT3         23
> -#define HVM_PARAM_MEMORY_EVENT_SINGLE_STEP  25
> -#define HVM_PARAM_MEMORY_EVENT_MSR          30

I'm not sure if HVM param slots can be re-used. If they can, leaving a
note that the deleted numbers are available for re-sue would be nice.
If they can't, leaving a note that they shouldn't be re-used would
seem mandatory.

Jan