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

[Tamas K Lengyel <tamas.lengyel@zentific.com>]

On Tue, Feb 17, 2015 at 3:02 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 13.02.15 at 17:33, <tamas.lengyel@zentific.com> 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.

It took me a while to understand what "introspection_enabled" actually
represents and all it really does at the moment is enabling the
interception of an extended set of MSRs. If something, that is a bad
variable name. Since in this series "introspection_enabled" is
removed, here I just updated the variable to the one that holds the
same information. I don't actually know what the code here does as I
didn't touch it. If this indeed has no connection to mov-to-msr, it
should have its own option field with its own name actually describing
what it does. Maybe Razvan has some more information on what is going
on here and if another variable needs to be introduced that was just
latched onto "introspection_enabled".

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

I'm going with Andrew's suggestions.

>
>> +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).

The declaration of the check as a separate function is dropped. The
check itself is moved into the domctl handler.

>>              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.

Indentation can't be fixed in the code addition as it breaks git -M.
It reverts to the old format where it just removes the whole file and
adds the new one. I think its a waste to add a whole new separate
patch just to fix indentations so I just fix it here.

>> +            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.

See my previous comment.

>
>> --- /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).

Ack.

>
>> +#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.

Just including sched.h additionally should be enough IMHO.

>
>> --- 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?

The use if bitfields is not good performance-wise AFAIK. Would there
be any benefit that would offset that?

>
>> --- /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@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, 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.

Ack.

>
> Jan

Thanks,
Tamas