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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 12.03.15 at 18:58, <tamas.lengyel@xxxxxxxxxxxx> wrote:
> @@ -91,41 +88,55 @@ static int hvm_event_traps(uint64_t parameters, 
> vm_event_request_t *req)
>      return 1;
>  }
>  
> -static void hvm_event_cr(uint32_t reason, unsigned long value,
> -                         unsigned long old, uint64_t parameters)
> +static inline
> +void hvm_event_cr(uint32_t reason, unsigned long value,
> +                         unsigned long old, bool_t onchangeonly, bool_t sync)
>  {
> -    vm_event_request_t req = {
> -        .reason = reason,
> -        .vcpu_id = current->vcpu_id,
> -        .u.mov_to_cr.new_value = value,
> -        .u.mov_to_cr.old_value = old
> -    };
> -
> -    if ( (parameters & HVMPME_onchangeonly) && (value == old) )
> +    if ( onchangeonly && value == old )
> +    {
>          return;
> -
> -    hvm_event_traps(parameters, &req);
> +    }
> +    else
> +    {
> +        vm_event_request_t req = {
> +            .reason = reason,
> +            .vcpu_id = current->vcpu_id,
> +            .u.mov_to_cr.new_value = value,
> +            .u.mov_to_cr.old_value = old
> +        };
> +
> +        hvm_event_traps(sync, &req);
> +    }

... I'd really like to see such done without "else" (which would also
have resulted in a smaller change).

> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -5830,19 +5830,11 @@ long do_hvm_op(unsigned long op, 
> XEN_GUEST_HANDLE_PARAM(void) arg)
>              case HVM_PARAM_MEMORY_EVENT_CR0:
>              case HVM_PARAM_MEMORY_EVENT_CR3:
>              case HVM_PARAM_MEMORY_EVENT_CR4:
> -                if ( d == current->domain )
> -                    rc = -EPERM;
> -                break;
>              case HVM_PARAM_MEMORY_EVENT_INT3:
>              case HVM_PARAM_MEMORY_EVENT_SINGLE_STEP:
>              case HVM_PARAM_MEMORY_EVENT_MSR:
> -                if ( d == current->domain )
> -                {
> -                    rc = -EPERM;
> -                    break;
> -                }
> -                if ( a.value & HVMPME_onchangeonly )
> -                    rc = -EINVAL;
> +                /* Deprecated */
> +                rc = -EPERM;

-EOPNOTSUPP would have been better, but anyway.

> @@ -5902,29 +5894,10 @@ long do_hvm_op(unsigned long op, 
> XEN_GUEST_HANDLE_PARAM(void) arg)
>              }
>              }
>  
> -            if ( rc == 0 ) 
> +            if ( rc == 0 )
>              {
>                  d->arch.hvm_domain.params[a.index] = a.value;
> -
> -                switch( a.index )
> -                {
> -                case HVM_PARAM_MEMORY_EVENT_INT3:
> -                case HVM_PARAM_MEMORY_EVENT_SINGLE_STEP:
> -                {
> -                    domain_pause(d);
> -                    domain_unpause(d); /* Causes guest to latch new status */
> -                    break;
> -                }
> -                case HVM_PARAM_MEMORY_EVENT_CR3:
> -                {
> -                    for_each_vcpu ( d, v )
> -                        hvm_funcs.update_guest_cr(v, 0); /* Latches new CR3 
> mask through CR0 code */
> -                    break;
> -                }
> -                }
> -
>              }
> -

There's a now pointless pair of braces left in place.

> +int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *mop)
> +{
> +    int rc;
> +    struct arch_domain *ad = &d->arch;
> +
> +    rc = xsm_vm_event_control(XSM_PRIV, d, mop->op, mop->event);
> +    if ( rc )
> +        return rc;
> +
> +    /*
> +     * 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 -EOPNOTSUPP;
> +
> +    /*
> +     * Sanity check
> +     */
> +    if ( mop->op != XEN_DOMCTL_MONITOR_OP_ENABLE &&
> +         mop->op != XEN_DOMCTL_MONITOR_OP_DISABLE )
> +        return -ENOSYS;

Here and further down -EOPNOTSUPP would again be better - we
try to use -ENOSYS only for hypercalls that aren't implemented at
all, not for invalid sub-ops (but there may still be a few violators
in the tree).

>          case XEN_VM_EVENT_MONITOR_DISABLE:
>          {
>              if ( ved->ring_page )
>              {
>                  rc = vm_event_disable(d, ved);
> -                d->arch.hvm_domain.introspection_enabled = 0;
>              }

Again leaving in place a now pointless pair of braces.

> --- a/xen/include/asm-x86/domain.h
> +++ b/xen/include/asm-x86/domain.h
> @@ -333,12 +333,32 @@ struct arch_domain
>      struct e820entry *e820;
>      unsigned int nr_e820;
>  
> -    unsigned int psr_rmid; /* RMID assigned to the domain for CMT */
> -
>      /* Shared page for notifying that explicit PIRQ EOI is required. */
>      unsigned long *pirq_eoi_map;
>      unsigned long pirq_eoi_map_mfn;
> -};
> +
> +    unsigned int psr_rmid; /* RMID assigned to the domain for CMT */

I cannot see why this is being moved.

> --- /dev/null
> +++ b/xen/include/asm-x86/monitor.h
> @@ -0,0 +1,31 @@
> +/*
> + * 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 <xen/sched.h>
> +#include <public/domctl.h>
> +
> +int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *op);

Didn't we settle on the two includes above being pointless? We
should really aim at reducing the header dependency tree rather
than endlessly growing it (and having basically everyone include
everything).

> --- a/xen/include/asm-x86/p2m.h
> +++ b/xen/include/asm-x86/p2m.h
> @@ -607,24 +607,38 @@ long p2m_set_mem_access(struct domain *d, unsigned long 
> start_pfn, uint32_t nr,
>  int p2m_get_mem_access(struct domain *d, unsigned long pfn,
>                         xenmem_access_t *access);
>  
> -/* Check for emulation and mark vcpu for skipping one instruction
> - * upon rescheduling if required. */
> -void p2m_mem_access_emulate_check(struct vcpu *v,
> -                                  const vm_event_response_t *rsp);
> +/*
> + * Emulating a memory access requires custom handling
> + */
> +static inline
> +int p2m_mem_access_enable_emulate(struct domain *d)
> +{
> +    if ( d->arch.mem_access_emulate_enabled )
> +        return -EEXIST;
>  
> -/* Enable arch specific introspection options (such as MSR interception). */
> -void p2m_setup_introspection(struct domain *d);
> +    d->arch.mem_access_emulate_enabled = 1;
> +    return 0;
> +}
>  
> -/* Sanity check for vm_event hardware support */
> -static inline bool_t p2m_vm_event_sanity_check(struct domain *d)
> +static inline
> +int p2m_mem_access_disable_emulate(struct domain *d)
>  {
> -    return hap_enabled(d) && cpu_has_vmx;
> +    if ( !d->arch.mem_access_emulate_enabled )
> +        return -EEXIST;
> +
> +    d->arch.mem_access_emulate_enabled = 0;
> +    return 0;
>  }

The fact that these seemingly non-atomic checks/updates are safe
(due to being under domctl lock) should be stated in a comment.

Jan

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