Re: [Xen-devel] [PATCH V5 07/12] xen: Introduce monitor_op domctl

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 13.02.15 at 17:33, <tamas.lengyel@xxxxxxxxxxxx> wrote:
> --- a/xen/arch/x86/hvm/emulate.c
> +++ b/xen/arch/x86/hvm/emulate.c
> @@ -411,7 +411,8 @@ static int hvmemul_virtual_to_linear(
>       * being triggered for repeated writes to a whole page.
>       */
>      *reps = min_t(unsigned long, *reps,
> -                  
> unlikely(current->domain->arch.hvm_domain.introspection_enabled)
> +                  unlikely(current->domain->arch
> +                            .monitor_options.mov_to_msr.extended_capture)
>                             ? 1 : 4096);

This makes no sense (especially not to a reader in a year or two):
There's no connection between mov-to-msr and the repeat count
capping done here. Please wrap this in a suitably named is_...() or
has_...() or introspection_enabled() helper, with a comment at its
definition site making the connection explicit.

> @@ -79,7 +76,7 @@ static int hvm_event_traps(uint64_t parameters, 
> vm_event_request_t *req)
>          return rc;
>      };
>  
> -    if ( (parameters & HVMPME_MODE_MASK) == HVMPME_mode_sync )
> +    if ( sync )

Looks like this function parameter wants to be bool_t.

> +#define DISABLE_OPTION(option)              \
> +    do {                                    \
> +        if ( !option->enabled )             \
> +            return -EFAULT;                 \
> +        domain_pause(d);                    \
> +        option->enabled = 0;                \
> +        domain_unpause(d);                  \
> +    } while (0)
> +
> +#define ENABLE_OPTION(option)               \
> +    do {                                    \
> +        domain_pause(d);                    \
> +        option->enabled = 1;                \
> +        domain_unpause(d);                  \
> +    } while (0)

If you decide not to follow Andrew's suggestion, then at the very
least these macros need to be properly parenthesized, have all
their parameters made explicit, and all their arguments evaluated
exactly once.

> +int monitor_domctl(struct xen_domctl_monitor_op *domctl, struct domain *d)
> +{
> +    /*
> +     * At the moment only Intel HVM domains are supported. However, event
> +     * delivery could be extended to AMD and PV domains. See comments below.
> +     */
> +    if ( !is_hvm_domain(d) || !cpu_has_vmx)
> +        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_cr *options = &d->arch.monitor_options.mov_to_cr0;

As three of the cases need a variable of this type, please consider
declaring it in the scope of switch() itself.

> +
> +        if ( domctl->op == XEN_DOMCTL_MONITOR_OP_ENABLE )
> +        {
> +            if ( options->enabled )
> +                return -EBUSY;
> +
> +            options->sync = domctl->u.mov_to_cr.sync;
> +            options->onchangeonly = domctl->u.mov_to_cr.onchangeonly;
> +            ENABLE_OPTION(options);
> +        }
> +        else
> +        {
> +            DISABLE_OPTION(options);
> +        }

Pointless braces.

> +        break;
> +    }
> +
> +    case XEN_DOMCTL_MONITOR_SUBOP_MOV_TO_CR3:

Here you properly add a blank line between cases - please do so
too further down.

> --- a/xen/common/vm_event.c
> +++ b/xen/common/vm_event.c
> @@ -621,32 +621,17 @@ int vm_event_domctl(struct domain *d, 
> xen_domctl_vm_event_op_t *vec,
>          switch( vec->op )
>          {
>          case XEN_VM_EVENT_MONITOR_ENABLE:
> -        case XEN_VM_EVENT_MONITOR_ENABLE_INTROSPECTION:
> -        {
> -            rc = -ENODEV;
> -            if ( !p2m_vm_event_sanity_check(d) )
> -                break;

Other than the revision log says, this doesn't get moved but dropped,
which seems wrong (or would need mentioning in the description).

>              rc = vm_event_enable(d, vec, ved, _VPF_mem_access,
> -                                    HVM_PARAM_MONITOR_RING_PFN,
> -                                    mem_access_notification);
> -
> -            if ( vec->op == XEN_VM_EVENT_MONITOR_ENABLE_INTROSPECTION
> -                 && !rc )
> -                p2m_setup_introspection(d);
> -
> -        }
> -        break;
> +                                 HVM_PARAM_MONITOR_RING_PFN,
> +                                 mem_access_notification);

I don't see what changes for these two lines. If it's indentation, it
should be done right when the code gets added.

> +            break;

This indentation change should not be necessary - the braces you
remove here shouldn't get added in the first place when the new
file gets introduced. Same further down.

> --- /dev/null
> +++ b/xen/include/asm-arm/monitor.h
> @@ -0,0 +1,13 @@
> +#ifndef __ASM_ARM_MONITOR_H__
> +#define __ASM_ARM_MONITOR_H__
> +
> +#include <xen/config.h>

This is pointless and should be dropped (I seem to recall having made
the same statement before on an earlier version - please apply such
to all of the patches in a series).

> +#include <public/domctl.h>
> +
> +static inline
> +int monitor_domctl(struct xen_domctl_monitor_op *op, struct domain *d)

The includes above are insufficient for the types used, or you should
forward declare _both_ structs and not have any includes.

> --- a/xen/include/asm-x86/domain.h
> +++ b/xen/include/asm-x86/domain.h
> @@ -241,6 +241,24 @@ struct time_scale {
>      u32 mul_frac;
>  };
>  
> +/************************************************/
> +/*            monitor event options             */
> +/************************************************/
> +struct mov_to_cr {
> +    uint8_t enabled;
> +    uint8_t sync;
> +    uint8_t onchangeonly;
> +};
> +
> +struct mov_to_msr {
> +    uint8_t enabled;
> +    uint8_t extended_capture;
> +};
> +
> +struct debug_event {
> +    uint8_t enabled;
> +};

These are all internal structures - is there anything wrong with using
bitfields here?

> --- /dev/null
> +++ b/xen/include/asm-x86/monitor.h
> @@ -0,0 +1,30 @@
> +/*
> + * include/asm-x86/monitor.h
> + *
> + * 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, write to the
> + * Free Software Foundation, Inc., 59 Temple Place - Suite 330,
> + * Boston, MA 021110-1307, USA.
> + */
> +
> +#ifndef __ASM_X86_MONITOR_H__
> +#define __ASM_X86_MONITOR_H__
> +
> +#include <public/domctl.h>
> +
> +int monitor_domctl(struct xen_domctl_monitor_op *op, struct domain *d);

Same as for the ARM variant.

Jan

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