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

[Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>]

On 04/29/16 05:44, Tian, Kevin wrote:
>> From: Razvan Cojocaru [mailto:rcojocaru@xxxxxxxxxxxxxxx]
>> Sent: Wednesday, April 27, 2016 3:48 PM
>> +
>> +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;
> 
> I think we shouldn't change '*msr' directly here. Since later, 
> 
>> +
>> +    case 0xc0000000 ... 0xc0001fff:
>> +        BUILD_BUG_ON(ARRAY_SIZE(d->arch.monitor_msr_bitmap->high) * 8 <
>> 0x1fff);
>> +        *msr &= 0x1fff;
>> +        return &d->arch.monitor_msr_bitmap->high;
>> +
>> +    default:
>> +        return NULL;
>> +    }
>> +}
>> +
>> +static int monitor_enable_msr(struct domain *d, u32 msr)
>> +{
>> +    u32 *bitmap;
>> +
>> +    if ( !d->arch.monitor_msr_bitmap )
>> +        return -ENXIO;
>> +
>> +    bitmap = monitor_bitmap_for_msr(d, &msr);
>> +
>> +    if ( !bitmap )
>> +        return -EINVAL;
>> +
>> +    __set_bit(msr, bitmap);
>> +
>> +    hvm_enable_msr_interception(d, msr);
> 
> Here the original msr value is expected.

Thanks for catching that. You're right. I didn't catch that while
testing because the only interesting MSR-s (for introspection) which
could also be disabled (and thus needed the explicit interception
enabling) were below 0x1fff, and so no problems occured (although of
course the patch was also enabling interception for a few other MSR-s
below 0x1fff which shouldn't have been intercepted).

I'll leave monitor_bitmap_for_msr() as it is, but will cache the old
value there and pass that to hvm_enable_msr_interception().


Thanks,
Razvan

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