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

[Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>]

On 05/04/2016 05:45 PM, Jan Beulich wrote:
>>>> 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.

No problem.

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

I'll change it to 0x2000, and switch to sizeof().

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

Understood.

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

I'll move it.


Thanks,
Razvan


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