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

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

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

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);
> +    }
> +
> +    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?

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

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


>          } mov_to_msr;
>  
>          struct {
> -- 
> 1.9.1
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

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