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

[Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>]

On 04/12/16 20:49, Tamas K Lengyel wrote:
>     diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c
>     index 1fec412..1be058a 100644
>     --- a/xen/arch/x86/monitor.c
>     +++ b/xen/arch/x86/monitor.c
>     @@ -20,6 +20,7 @@
>       */
> 
>      #include <asm/monitor.h>
>     +#include <asm/vm_event.h>
>      #include <public/vm_event.h>
> 
>      int arch_monitor_domctl_event(struct domain *d,
>     @@ -77,25 +78,25 @@ int arch_monitor_domctl_event(struct domain *d,
> 
>          case XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR:
>          {
>     -        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 = vm_event_is_msr_enabled(d, msr);
> 
>     -        domain_pause(d);
>     +        if ( unlikely(old_status == requested_status) )
>     +            return -EEXIST;
> 
> 
> Moving this test after the domain gets paused will leave the guest
> paused on error condition. Any reason why this rearrangement is necessary?

The rearrangement is necessary because vm_event_is_msr_enabled() uses
the per-domain bitmap to do its job, so I thought having the domain
paused when checking it would work best.

Leaving the domain paused there is an oversight, I need to correct that,
so thanks for noticing!

>     -        if ( requested_status && mop->u.mov_to_msr.extended_capture )
>     -            ad->monitor.mov_to_msr_extended = 1;
>     +        if ( requested_status )
>     +            rc = vm_event_enable_msr(d, msr);
>              else
>     -            ad->monitor.mov_to_msr_extended = 0;
>     +            rc = vm_event_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..b851e39 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();
>     +
>     +    if ( !d->arch.monitor_msr_bitmap )
>     +        return -ENOMEM;
>     +
>     +    memset(d->arch.monitor_msr_bitmap, 0, PAGE_SIZE);
>     +
>          for_each_vcpu ( d, v )
>          {
>              if ( v->arch.vm_event )
>     @@ -55,11 +62,66 @@ void vm_event_cleanup_domain(struct domain *d)
>              v->arch.vm_event = NULL;
>          }
> 
>     +    free_xenheap_page(d->arch.monitor_msr_bitmap);
>     +    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));
>      }
> 
>     +int vm_event_enable_msr(struct domain *d, u32 msr)
> 
> 
> This function..
>  
> 
>     +{
>     +    if ( !d->arch.monitor_msr_bitmap )
>     +        return -EINVAL;
>     +
>     +    if ( msr <= 0x1fff )
>     +        set_bit(msr, d->arch.monitor_msr_bitmap +
>     0x000/BYTES_PER_LONG);
>     +    else if ( (msr >= 0xc0000000) && (msr <= 0xc0001fff) )
>     +    {
>     +        msr &= 0x1fff;
>     +        set_bit(msr, d->arch.monitor_msr_bitmap +
>     0x400/BYTES_PER_LONG);
>     +    }
>     +
>     +    hvm_enable_msr_interception(d, msr);
>     +
>     +    return 0;
>     +}
>     +
>     +int vm_event_disable_msr(struct domain *d, u32 msr)
> 
> 
> ..and this..
>  
> 
>     +{
>     +    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 vm_event_is_msr_enabled(const struct domain *d, u32 msr)
> 
>  
> ..and this one has nothing to do with vm_event really. These belong in
> the monitor.c side.
>  
> 
>     +{
>     +    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);
>     +    }
>     +
>     +    return rc;
>     +}
>     +
>      void vm_event_toggle_singlestep(struct domain *d, struct vcpu *v)
>      {
>          if ( !is_hvm_domain(d) || !atomic_read(&v->vm_event_pause_count) )

Fair enough. Will move them.


Thanks,
Razvan


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