Re: [Xen-devel] [PATCH V7] vm_event: Allow subscribing to write events for specific MSR-s

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 29.04.16 at 08:03, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
> --- a/xen/arch/x86/monitor.c
> +++ b/xen/arch/x86/monitor.c
> @@ -22,6 +22,104 @@
>  #include <asm/monitor.h>
>  #include <public/vm_event.h>
>  
> +int arch_monitor_init_domain(struct domain *d)
> +{
> +    if ( !d->arch.monitor_msr_bitmap )
> +        d->arch.monitor_msr_bitmap = xzalloc(struct monitor_msr_bitmap);
> +
> +    if ( !d->arch.monitor_msr_bitmap )
> +        return -ENOMEM;
> +
> +    memset(&d->arch.monitor, 0, sizeof(d->arch.monitor));
> +    memset(&d->monitor, 0, sizeof(d->monitor));
> +
> +    return 0;
> +}
> +
> +void arch_monitor_cleanup_domain(struct domain *d)
> +{
> +    xfree(d->arch.monitor_msr_bitmap);
> +    d->arch.monitor_msr_bitmap = NULL;
> +}

Considering that struct domain starts out zeroed, it would seem
slightly more efficient to move the memset()-s into the cleanup
function. After all that's how it has been before you moved the
code here.

> +static void *monitor_bitmap_for_msr(struct domain *d, u32 *msr)
> +{
> +    ASSERT(d->arch.monitor_msr_bitmap && msr);
> +
> +    switch ( *msr )
> +    {
> +    case 0 ... 0x1fff:
> +        BUILD_BUG_ON(ARRAY_SIZE(d->arch.monitor_msr_bitmap->low) * 8 < 
> 0x1fff);
> +        return &d->arch.monitor_msr_bitmap->low;
> +
> +    case 0x40000000 ... 0x40001fff:
> +        BUILD_BUG_ON(
> +            ARRAY_SIZE(d->arch.monitor_msr_bitmap->hypervisor) * 8 < 0x1fff);
> +        *msr &= 0x1fff;
> +        return &d->arch.monitor_msr_bitmap->hypervisor;
> +
> +    case 0xc0000000 ... 0xc0001fff:
> +        BUILD_BUG_ON(ARRAY_SIZE(d->arch.monitor_msr_bitmap->high) * 8 < 
> 0x1fff);

I really recommend using sizeof() instead of ARRAY_SIZE() in all of
these, so the code isn't dependent on the base type of those arrays
(which really ought to change to proper bitmaps anyway, due to
the __set_bit() and __clear_bit() the callers of this function do). Also
the comparison with 0x1fff is (in a benign way) off by one - I think
you really mean either <= or 0x2000.

> +        *msr &= 0x1fff;
> +        return &d->arch.monitor_msr_bitmap->high;
> +
> +    default:
> +        return NULL;
> +    }
> +}
> +
> +static int monitor_enable_msr(struct domain *d, u32 msr)
> +{
> +    u32 *bitmap;

Together with the above - unsigned long * please (and the helper
function's return type should then also be unsigned long *).

> --- a/xen/include/asm-x86/domain.h
> +++ b/xen/include/asm-x86/domain.h
> @@ -401,12 +401,12 @@ struct arch_domain
>          unsigned int write_ctrlreg_enabled       : 4;
>          unsigned int write_ctrlreg_sync          : 4;
>          unsigned int write_ctrlreg_onchangeonly  : 4;
> -        unsigned int mov_to_msr_enabled          : 1;
> -        unsigned int mov_to_msr_extended         : 1;
>          unsigned int singlestep_enabled          : 1;
>          unsigned int software_breakpoint_enabled : 1;
>      } monitor;
>  
> +    struct monitor_msr_bitmap *monitor_msr_bitmap;

Why is this not being made part of the immediately preceding
structure (omitting the monitor_ prefix)?

Jan


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