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

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 13/04/16 10:47, Konrad Rzeszutek Wilk wrote:
>> diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c
>> index 1fec412..4c96968 100644
>> --- a/xen/arch/x86/monitor.c
>> +++ b/xen/arch/x86/monitor.c
>> @@ -22,6 +22,58 @@
>>  #include <asm/monitor.h>
>>  #include <public/vm_event.h>
>>  
>> +static int arch_monitor_enable_msr(struct domain *d, u32 msr)
>> +{
>> +    if ( !d->arch.monitor_msr_bitmap )
>> +        return -EINVAL;
> I this was not set wouldn't we fail in vm_event_enable with -ENOMEM?
>
> I presume the user can still make this hypercall..  Ah yes.
>
> Perhaps -ENXIO?
>> +
>> +    if ( msr <= 0x1fff )
>> +        set_bit(msr, d->arch.monitor_msr_bitmap + 0x000/BYTES_PER_LONG);

(It might help to read the following review before coming back here...)

It might be clearer to express monitor_msr_bitmap as a pointer to

struct monitor_msr_bitmap {
    uint8_t low[1024];
    uint8_t hypervisor[1024];
    uint8_t high[1024];
};

which avoids the odd pointer arithmetic.

> The 0x000/BYTER_PER_LONG looks odd. Is it even needed?
>
>> +    else if ( (msr >= 0xc0000000) && (msr <= 0xc0001fff) )
>> +    {
>> +        msr &= 0x1fff;
>> +        set_bit(msr, d->arch.monitor_msr_bitmap + 0x400/BYTES_PER_LONG);

__set_bit().  I don't think you need a LOCK here.

>> +    }
>> +
>> +    hvm_enable_msr_interception(d, msr);
> And for MSRs above 0xc0001fff it is OK to enable the interception?
> Or between 0x1fff and 0xc0000000?

No real MSRs exist outside the [0..1fff] and [0xc0000000..0xc0001fff]
ranges, so will suffer a #GP.  This is even reflected in how both VT-x
and SVM do their MSR interception bitmap, which is why I specifically
suggested using the same here.

However, this case wants a range between [0x40000000..0x40001fff]

>
> No need to filter them out? Or error on them?
>> +
>> +    return 0;
>> +}
>> +
>> +static int arch_monitor_disable_msr(struct domain *d, u32 msr)
>> +{
>> +    if ( !d->arch.monitor_msr_bitmap )
>> +        return -EINVAL;
>> +
>> +    if ( msr <= 0x1fff )
>> +        clear_bit(msr, d->arch.monitor_msr_bitmap + 0x000/BYTES_PER_LONG);
>> +    else if ( (msr >= 0xc0000000) && (msr <= 0xc0001fff) )
>> +    {
>> +        msr &= 0x1fff;
>> +        clear_bit(msr, d->arch.monitor_msr_bitmap + 0x400/BYTES_PER_LONG);
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +bool_t arch_monitor_is_msr_enabled(const struct domain *d, u32 msr)
>> +{
>> +    bool_t rc = 0;
>> +
>> +    if ( !d->arch.monitor_msr_bitmap )
>> +        return 0;
>> +
>> +    if ( msr <= 0x1fff )
>> +        rc = test_bit(msr, d->arch.monitor_msr_bitmap + 0x000/BYTES_PER_LONG);
>> +    else if ( (msr >= 0xc0000000) && (msr <= 0xc0001fff) )
>> +    {
>> +        msr &= 0x1fff;
>> +        rc = test_bit(msr, d->arch.monitor_msr_bitmap + 0x400/BYTES_PER_LONG);
>> +    }
> And what if msr requested is above 0xc0001fff ? What then?
>
>> +
>> +    return rc;
>> +}
>> +
>>  int arch_monitor_domctl_event(struct domain *d,
>>                                struct xen_domctl_monitor_op *mop)
>>  {
>> @@ -77,25 +129,28 @@ int arch_monitor_domctl_event(struct domain *d,
>>  
>>      case XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR:
> Should this be renamed?
>>      {
>> -        bool_t old_status = ad->monitor.mov_to_msr_enabled;
>> +        bool_t old_status;
>> +        int rc;
>> +        u32 msr = mop->u.mov_to_msr.msr;
>>  
>> -        if ( unlikely(old_status == requested_status) )
>> -            return -EEXIST;
>> +        domain_pause(d);
>>  
>> -        if ( requested_status && mop->u.mov_to_msr.extended_capture &&
>> -             !hvm_enable_msr_exit_interception(d) )
>> -            return -EOPNOTSUPP;
>> +        old_status = arch_monitor_is_msr_enabled(d, msr);
>>  
>> -        domain_pause(d);
>> +        if ( unlikely(old_status == requested_status) )
>> +        {
>> +            domain_unpause(d);
>> +            return -EEXIST;
>> +        }
>>  
>> -        if ( requested_status && mop->u.mov_to_msr.extended_capture )
>> -            ad->monitor.mov_to_msr_extended = 1;
>> +        if ( requested_status )
>> +            rc = arch_monitor_enable_msr(d, msr);
>>          else
>> -            ad->monitor.mov_to_msr_extended = 0;
>> +            rc = arch_monitor_disable_msr(d, msr);
>>  
>> -        ad->monitor.mov_to_msr_enabled = requested_status;
>>          domain_unpause(d);
>> -        break;
>> +
>> +        return rc;
>>      }
>>  
>>      case XEN_DOMCTL_MONITOR_EVENT_SINGLESTEP:
>> diff --git a/xen/arch/x86/vm_event.c b/xen/arch/x86/vm_event.c
>> index 5635603..9b4267e 100644
>> --- a/xen/arch/x86/vm_event.c
>> +++ b/xen/arch/x86/vm_event.c
>> @@ -27,6 +27,13 @@ int vm_event_init_domain(struct domain *d)
>>  {
>>      struct vcpu *v;
>>  
>> +    d->arch.monitor_msr_bitmap = alloc_xenheap_page();
> How about using vzalloc?

vmap space is far more limited than general xenheap space.  vmap()
should only be used when you need >4K allocations contiguously in
virtual address space.

>> +
>> +    if ( !d->arch.monitor_msr_bitmap )
>> +        return -ENOMEM;
>> +
>> +    memset(d->arch.monitor_msr_bitmap, 0, PAGE_SIZE);
> Then you don't have to do that.

clear_page()

>
>> +
>>      for_each_vcpu ( d, v )
>>      {
>>          if ( v->arch.vm_event )
>> @@ -55,6 +62,9 @@ void vm_event_cleanup_domain(struct domain *d)
>>          v->arch.vm_event = NULL;
>>      }
>>  
>> +    free_xenheap_page(d->arch.monitor_msr_bitmap);
> And this would be vfree.
>
>> +    d->arch.monitor_msr_bitmap = NULL;
>> +
>>      d->arch.mem_access_emulate_each_rep = 0;
>>      memset(&d->arch.monitor, 0, sizeof(d->arch.monitor));
>>      memset(&d->monitor, 0, sizeof(d->monitor));
>> diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
>> index 2457698..875c09a 100644
>> --- a/xen/include/public/domctl.h
>> +++ b/xen/include/public/domctl.h
>> @@ -1107,8 +1107,7 @@ struct xen_domctl_monitor_op {
>>          } mov_to_cr;
>>  
>>          struct {
>> -            /* Enable the capture of an extended set of MSRs */
>> -            uint8_t extended_capture;
>> +            uint32_t msr;
> Whoa there. Isn't it expanding the structure? Will this be backwards
> compatible? What if somebody is using an older version of xen-access
> against this hypervisor? Will they work?

Its a domctl.  This is perfectly fine (within the rules) to do.

~Andrew

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