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

[Razvan Cojocaru <rcojocaru@bitdefender.com>]

On 04/13/2016 12:47 PM, 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?

Sure, I can return -ENXIO. I just thought -EINVAL reflects the case
well: it's not right to call this hypercall if you haven't subscribed
for vm_events beforehand (in which case d->arch.monitor_msr_bitmap is
NULL, because it's only allocated then, and de-allocated again when the
subscriber unsubscribes).

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

I've pretty much copied the code from the enabled msrs bitmap, so I
assume it was, but I'll change the code to follow Andrew Cooper's
suggestion which should make this go away.

>> +    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);
> 
> And for MSRs above 0xc0001fff it is OK to enable the interception?
> Or between 0x1fff and 0xc0000000?
> 
> 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?

I think the questions above have been answered by Andrew Cooper.

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

I'm happy to rename it, but I don't think it should - it has the exact
same semantics as before: monitor a MSR write.

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

I'll follow Andrew Cooper's requests here, which should address these
issues.

>> +    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/asm-x86/domain.h b/xen/include/asm-x86/domain.h
>> index d393ed2..d8d91c2 100644
>> --- a/xen/include/asm-x86/domain.h
>> +++ b/xen/include/asm-x86/domain.h
>> @@ -398,12 +398,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;
>>  
>> +    unsigned long *monitor_msr_bitmap;
>> +
>>      /* Mem_access emulation control */
>>      bool_t mem_access_emulate_each_rep;
>>  
>> diff --git a/xen/include/asm-x86/hvm/hvm.h b/xen/include/asm-x86/hvm/hvm.h
>> index 7b7ff3f..9d1c0ef 100644
>> --- a/xen/include/asm-x86/hvm/hvm.h
>> +++ b/xen/include/asm-x86/hvm/hvm.h
>> @@ -211,7 +211,7 @@ struct hvm_function_table {
>>                                    uint32_t *eax, uint32_t *ebx,
>>                                    uint32_t *ecx, uint32_t *edx);
>>  
>> -    void (*enable_msr_exit_interception)(struct domain *d);
>> +    void (*enable_msr_interception)(struct domain *d, uint32_t msr);
>>      bool_t (*is_singlestep_supported)(void);
>>      int (*set_mode)(struct vcpu *v, int mode);
>>  
>> @@ -565,11 +565,11 @@ static inline enum hvm_intblk nhvm_interrupt_blocked(struct vcpu *v)
>>      return hvm_funcs.nhvm_intr_blocked(v);
>>  }
>>  
>> -static inline bool_t hvm_enable_msr_exit_interception(struct domain *d)
>> +static inline bool_t hvm_enable_msr_interception(struct domain *d, uint32_t msr)
>>  {
>> -    if ( hvm_funcs.enable_msr_exit_interception )
>> +    if ( hvm_funcs.enable_msr_interception )
>>      {
>> -        hvm_funcs.enable_msr_exit_interception(d);
>> +        hvm_funcs.enable_msr_interception(d, msr);
>>          return 1;
>>      }
>>  
>> diff --git a/xen/include/asm-x86/hvm/vmx/vmcs.h b/xen/include/asm-x86/hvm/vmx/vmcs.h
>> index b54f52f..7bf5326 100644
>> --- a/xen/include/asm-x86/hvm/vmx/vmcs.h
>> +++ b/xen/include/asm-x86/hvm/vmx/vmcs.h
>> @@ -562,13 +562,6 @@ enum vmcs_field {
>>      HOST_RIP                        = 0x00006c16,
>>  };
>>  
>> -/*
>> - * A set of MSR-s that need to be enabled for memory introspection
>> - * to work.
>> - */
>> -extern const u32 vmx_introspection_force_enabled_msrs[];
>> -extern const unsigned int vmx_introspection_force_enabled_msrs_size;
>> -
>>  #define VMCS_VPID_WIDTH 16
>>  
>>  #define MSR_TYPE_R 1
>> diff --git a/xen/include/asm-x86/monitor.h b/xen/include/asm-x86/monitor.h
>> index 0954b59..74e5b1b 100644
>> --- a/xen/include/asm-x86/monitor.h
>> +++ b/xen/include/asm-x86/monitor.h
>> @@ -50,4 +50,6 @@ int arch_monitor_domctl_op(struct domain *d, struct xen_domctl_monitor_op *mop)
>>  int arch_monitor_domctl_event(struct domain *d,
>>                                struct xen_domctl_monitor_op *mop);
>>  
>> +bool_t arch_monitor_is_msr_enabled(const struct domain *d, u32 msr);
>> +
>>  #endif /* __ASM_X86_MONITOR_H__ */
>> 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?
> 
> Perhaps this should have a new struct / sub-ops? And the old
> 'mov_to_msr' will just re-use this new fangled code?

In addition to Andrew's comments, I think simply changing
VM_EVENT_INTERFACE_VERSION should be enough for xen-access-like clients
to figure out the incompatibility.


Thanks,
Razvan

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