[V4] vm_event: Allow subscribing to write events for specific MSR-s

Commit Message

Razvan Cojocaru April 17, 2016, 7:15 p.m. UTC

Previously, subscribing to MSR write events was an all-or-none
approach, with special cases for introspection MSR-s. This patch
allows the vm_event consumer to specify exactly what MSR-s it is
interested in, and as a side-effect gets rid of the
vmx_introspection_force_enabled_msrs[] special case.
This replaces the previously posted "xen: Filter out MSR write
events" patch.

Signed-off-by: Razvan Cojocaru <[e-mail]>
Acked-by: Wei Liu <[e-mail]>

---
Changes since V3:
 - Fixed high MSR range.
 - Added comment about where to find MSR indices to xenctrl.h.
 - Added Wei Liu's ack.
---
 tools/libxc/include/xenctrl.h      |  9 +++-
 tools/libxc/xc_monitor.c           |  6 +--
 xen/arch/x86/hvm/event.c           |  3 +-
 xen/arch/x86/hvm/hvm.c             |  3 +-
 xen/arch/x86/hvm/vmx/vmcs.c        | 26 ++---------
 xen/arch/x86/hvm/vmx/vmx.c         | 10 ++--
 xen/arch/x86/monitor.c             | 95 +++++++++++++++++++++++++++++++++-----
 xen/arch/x86/vm_event.c            |  9 ++++
 xen/include/asm-x86/domain.h       |  4 +-
 xen/include/asm-x86/hvm/hvm.h      |  8 ++--
 xen/include/asm-x86/hvm/vmx/vmcs.h |  7 ---
 xen/include/asm-x86/monitor.h      |  8 ++++
 xen/include/public/domctl.h        |  5 +-
 13 files changed, 126 insertions(+), 67 deletions(-)

Comments

Tamas K Lengyel April 17, 2016, 7:29 p.m. UTC | #1

On Sun, Apr 17, 2016 at 1:15 PM, Razvan Cojocaru <[e-mail]>
wrote:

> Previously, subscribing to MSR write events was an all-or-none
> approach, with special cases for introspection MSR-s. This patch
> allows the vm_event consumer to specify exactly what MSR-s it is
> interested in, and as a side-effect gets rid of the
> vmx_introspection_force_enabled_msrs[] special case.
> This replaces the previously posted "xen: Filter out MSR write
> events" patch.
>
> Signed-off-by: Razvan Cojocaru <[e-mail]>
> Acked-by: Wei Liu <[e-mail]>
>

Acked-by: Tamas K Lengyel <[e-mail]>

Tian, Kevin April 19, 2016, 5:09 a.m. UTC | #2

> From: Razvan Cojocaru [mailto:[e-mail]]
> Sent: Monday, April 18, 2016 3:16 AM
> 
> Previously, subscribing to MSR write events was an all-or-none
> approach, with special cases for introspection MSR-s. This patch
> allows the vm_event consumer to specify exactly what MSR-s it is
> interested in, and as a side-effect gets rid of the
> vmx_introspection_force_enabled_msrs[] special case.
> This replaces the previously posted "xen: Filter out MSR write
> events" patch.
> 
> Signed-off-by: Razvan Cojocaru <[e-mail]>
> Acked-by: Wei Liu <[e-mail]>
> 

Acked-by: Kevin Tian <[e-mail]>

Andrew Cooper April 22, 2016, 6:07 p.m. UTC | #3

On 17/04/16 20:15, Razvan Cojocaru wrote:
> diff --git a/xen/arch/x86/hvm/event.c b/xen/arch/x86/hvm/event.c
> index 56c5514..9c17f37 100644
> --- a/xen/arch/x86/hvm/event.c
> +++ b/xen/arch/x86/hvm/event.c
> @@ -57,9 +57,8 @@ bool_t hvm_event_cr(unsigned int index, unsigned long value, unsigned long old)
>  void hvm_event_msr(unsigned int msr, uint64_t value)
>  {
>      struct vcpu *curr = current;
> -    struct arch_domain *ad = &curr->domain->arch;
>  
> -    if ( ad->monitor.mov_to_msr_enabled )
> +    if ( monitor_is_msr_enabled(curr->domain, msr) )

"enabled" can take several meanings with MSRs.  This function name would
be clearer as is_msr_monitored(), or perhaps monitored_msr() if you want
to keep the monitor prefix.

> diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
> index 8284281..24ad906 100644
> --- a/xen/arch/x86/hvm/vmx/vmcs.c
> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
> @@ -37,6 +37,7 @@
>  #include <asm/hvm/vmx/vvmx.h>
>  #include <asm/hvm/vmx/vmcs.h>
>  #include <asm/flushtlb.h>
> +#include <asm/monitor.h>
>  #include <asm/shadow.h>
>  #include <asm/tboot.h>
>  #include <asm/apic.h>
> @@ -108,18 +109,6 @@ u64 vmx_ept_vpid_cap __read_mostly;
>  u64 vmx_vmfunc __read_mostly;
>  bool_t vmx_virt_exception __read_mostly;
>  
> -const u32 vmx_introspection_force_enabled_msrs[] = {
> -    MSR_IA32_SYSENTER_EIP,
> -    MSR_IA32_SYSENTER_ESP,
> -    MSR_IA32_SYSENTER_CS,
> -    MSR_IA32_MC0_CTL,
> -    MSR_STAR,
> -    MSR_LSTAR
> -};

What takes care of enabling monitoring for these MSRs, or are you
expecting the introspection engine to now explicitly register for each
MSR it wants?  Either is fine, but I suspect the latter, which is a
behavioural change in the interface.

> diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c
> index 1fec412..2bc05ed 100644
> --- a/xen/arch/x86/monitor.c
> +++ b/xen/arch/x86/monitor.c
> @@ -22,6 +22,74 @@
>  #include <asm/monitor.h>
>  #include <public/vm_event.h>
>  
> +static int monitor_enable_msr(struct domain *d, u32 msr)
> +{
> +    if ( !d->arch.monitor_msr_bitmap )
> +        return -ENXIO;
> +
> +    if ( msr <= 0x1fff )
> +        __set_bit(msr, &d->arch.monitor_msr_bitmap->low);
> +    else if ( (msr >= 0x40000000) && (msr <= 0x40001fff) )
> +    {
> +        msr &= 0x1fff;
> +        __set_bit(msr, &d->arch.monitor_msr_bitmap->hypervisor);
> +    }
> +    else if ( (msr >= 0xc0000000) && (msr <= 0xc0001fff) )
> +    {
> +        msr &= 0x1fff;
> +        __set_bit(msr, &d->arch.monitor_msr_bitmap->high);
> +    }
> +
> +    hvm_enable_msr_interception(d, msr);
> +
> +    return 0;
> +}
> +
> +static int monitor_disable_msr(struct domain *d, u32 msr)
> +{
> +    if ( !d->arch.monitor_msr_bitmap )
> +        return -ENXIO;
> +
> +    if ( msr <= 0x1fff )
> +        clear_bit(msr, &d->arch.monitor_msr_bitmap->low);
> +    else if ( (msr >= 0x40000000) && (msr <= 0x40001fff) )
> +    {
> +        msr &= 0x1fff;
> +        clear_bit(msr, &d->arch.monitor_msr_bitmap->hypervisor);
> +    }
> +    else if ( (msr >= 0xc0000000) && (msr <= 0xc0001fff) )
> +    {
> +        msr &= 0x1fff;
> +        clear_bit(msr, &d->arch.monitor_msr_bitmap->high);

__clear_bit()

> +    }
> +

What about disabling MSR interception, to mirror the enable side?  (This
is starting to get complicated - should we start refcounting how many
entities want an MSR intercepted?  This sounds suboptimal).

> +
> +    return 0;
> +}

You should also return -EINVAL for a bad MSR index, and BUILD_BUG_ON()
if the size of the bitmaps change.  You can also combine these
functions, to reduce the code duplication and risk of divergence.  e.g.
make a helper which looks like this

static void *monitor_bitmap_for_msr(struct domain *d, u32 *msr)
{
    ASSERT(d->arch.monitor_msr_bitmap);

    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;

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

To reduce the complexity of the other functions.

> diff --git a/xen/arch/x86/vm_event.c b/xen/arch/x86/vm_event.c
> index 5635603..22819c5 100644
> --- a/xen/arch/x86/vm_event.c
> +++ b/xen/arch/x86/vm_event.c
> @@ -20,6 +20,7 @@
>  
>  #include <xen/sched.h>
>  #include <asm/hvm/hvm.h>
> +#include <asm/monitor.h>
>  #include <asm/vm_event.h>
>  
>  /* Implicitly serialized by the domctl lock. */
> @@ -27,6 +28,11 @@ int vm_event_init_domain(struct domain *d)
>  {
>      struct vcpu *v;
>  
> +    d->arch.monitor_msr_bitmap = xzalloc(struct monitor_msr_bitmap);
> +
> +    if ( !d->arch.monitor_msr_bitmap )
> +        return -ENOMEM;

Shouldn't we in principle have a monitor_domain_init() function for
this, or are monitor and vm_event too intertwined in practice?

~Andrew

Razvan Cojocaru April 22, 2016, 6:36 p.m. UTC | #4

On 04/22/16 21:07, Andrew Cooper wrote:
> On 17/04/16 20:15, Razvan Cojocaru wrote:
>> diff --git a/xen/arch/x86/hvm/event.c b/xen/arch/x86/hvm/event.c
>> index 56c5514..9c17f37 100644
>> --- a/xen/arch/x86/hvm/event.c
>> +++ b/xen/arch/x86/hvm/event.c
>> @@ -57,9 +57,8 @@ bool_t hvm_event_cr(unsigned int index, unsigned long value, unsigned long old)
>>  void hvm_event_msr(unsigned int msr, uint64_t value)
>>  {
>>      struct vcpu *curr = current;
>> -    struct arch_domain *ad = &curr->domain->arch;
>>  
>> -    if ( ad->monitor.mov_to_msr_enabled )
>> +    if ( monitor_is_msr_enabled(curr->domain, msr) )
> 
> "enabled" can take several meanings with MSRs.  This function name would
> be clearer as is_msr_monitored(), or perhaps monitored_msr() if you want
> to keep the monitor prefix.

I'll pick the latter if nobody minds.

>> diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
>> index 8284281..24ad906 100644
>> --- a/xen/arch/x86/hvm/vmx/vmcs.c
>> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
>> @@ -37,6 +37,7 @@
>>  #include <asm/hvm/vmx/vvmx.h>
>>  #include <asm/hvm/vmx/vmcs.h>
>>  #include <asm/flushtlb.h>
>> +#include <asm/monitor.h>
>>  #include <asm/shadow.h>
>>  #include <asm/tboot.h>
>>  #include <asm/apic.h>
>> @@ -108,18 +109,6 @@ u64 vmx_ept_vpid_cap __read_mostly;
>>  u64 vmx_vmfunc __read_mostly;
>>  bool_t vmx_virt_exception __read_mostly;
>>  
>> -const u32 vmx_introspection_force_enabled_msrs[] = {
>> -    MSR_IA32_SYSENTER_EIP,
>> -    MSR_IA32_SYSENTER_ESP,
>> -    MSR_IA32_SYSENTER_CS,
>> -    MSR_IA32_MC0_CTL,
>> -    MSR_STAR,
>> -    MSR_LSTAR
>> -};
> 
> What takes care of enabling monitoring for these MSRs, or are you
> expecting the introspection engine to now explicitly register for each
> MSR it wants?  Either is fine, but I suspect the latter, which is a
> behavioural change in the interface.

Yes, that's the idea - the consumer now needs to explicitly register for
each MSR it wants write events for (and is able to unsubscribe
individually to MSRs that are no longer interesting). This provides
fine-grained control over MSR vm_events and can be a performance
optimization for an introspection application, since no vm_event exits
will occur for MSRs that are no longer interesting.

>> diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c
>> index 1fec412..2bc05ed 100644
>> --- a/xen/arch/x86/monitor.c
>> +++ b/xen/arch/x86/monitor.c
>> @@ -22,6 +22,74 @@
>>  #include <asm/monitor.h>
>>  #include <public/vm_event.h>
>>  
>> +static int monitor_enable_msr(struct domain *d, u32 msr)
>> +{
>> +    if ( !d->arch.monitor_msr_bitmap )
>> +        return -ENXIO;
>> +
>> +    if ( msr <= 0x1fff )
>> +        __set_bit(msr, &d->arch.monitor_msr_bitmap->low);
>> +    else if ( (msr >= 0x40000000) && (msr <= 0x40001fff) )
>> +    {
>> +        msr &= 0x1fff;
>> +        __set_bit(msr, &d->arch.monitor_msr_bitmap->hypervisor);
>> +    }
>> +    else if ( (msr >= 0xc0000000) && (msr <= 0xc0001fff) )
>> +    {
>> +        msr &= 0x1fff;
>> +        __set_bit(msr, &d->arch.monitor_msr_bitmap->high);
>> +    }
>> +
>> +    hvm_enable_msr_interception(d, msr);
>> +
>> +    return 0;
>> +}
>> +
>> +static int monitor_disable_msr(struct domain *d, u32 msr)
>> +{
>> +    if ( !d->arch.monitor_msr_bitmap )
>> +        return -ENXIO;
>> +
>> +    if ( msr <= 0x1fff )
>> +        clear_bit(msr, &d->arch.monitor_msr_bitmap->low);
>> +    else if ( (msr >= 0x40000000) && (msr <= 0x40001fff) )
>> +    {
>> +        msr &= 0x1fff;
>> +        clear_bit(msr, &d->arch.monitor_msr_bitmap->hypervisor);
>> +    }
>> +    else if ( (msr >= 0xc0000000) && (msr <= 0xc0001fff) )
>> +    {
>> +        msr &= 0x1fff;
>> +        clear_bit(msr, &d->arch.monitor_msr_bitmap->high);
> 
> __clear_bit()

Fair enough, will change it.

>> +    }
>> +
> 
> What about disabling MSR interception, to mirror the enable side?  (This
> is starting to get complicated - should we start refcounting how many
> entities want an MSR intercepted?  This sounds suboptimal).

Yes, that's why I've just left them on. This would indeed require
reference counting for each MSR to work properly, which would again
require more memory to store the refcount.

>> +
>> +    return 0;
>> +}
> 
> You should also return -EINVAL for a bad MSR index, and BUILD_BUG_ON()
> if the size of the bitmaps change.  You can also combine these
> functions, to reduce the code duplication and risk of divergence.  e.g.
> make a helper which looks like this
> 
> static void *monitor_bitmap_for_msr(struct domain *d, u32 *msr)
> {
>     ASSERT(d->arch.monitor_msr_bitmap);
> 
>     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;
> 
>     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;
>     }
> }
> 
> To reduce the complexity of the other functions.

That's a great suggestion, I'll make that change.

>> diff --git a/xen/arch/x86/vm_event.c b/xen/arch/x86/vm_event.c
>> index 5635603..22819c5 100644
>> --- a/xen/arch/x86/vm_event.c
>> +++ b/xen/arch/x86/vm_event.c
>> @@ -20,6 +20,7 @@
>>  
>>  #include <xen/sched.h>
>>  #include <asm/hvm/hvm.h>
>> +#include <asm/monitor.h>
>>  #include <asm/vm_event.h>
>>  
>>  /* Implicitly serialized by the domctl lock. */
>> @@ -27,6 +28,11 @@ int vm_event_init_domain(struct domain *d)
>>  {
>>      struct vcpu *v;
>>  
>> +    d->arch.monitor_msr_bitmap = xzalloc(struct monitor_msr_bitmap);
>> +
>> +    if ( !d->arch.monitor_msr_bitmap )
>> +        return -ENOMEM;
> 
> Shouldn't we in principle have a monitor_domain_init() function for
> this, or are monitor and vm_event too intertwined in practice?

I think they are (it doesn't really make sense to try to monitor MSR
writes without initializing the vm_event subsystem). Maybe Tamas has
different ideas?


Thanks,
Razvan

Tamas K Lengyel April 22, 2016, 6:57 p.m. UTC | #5

>
>
> >> diff --git a/xen/arch/x86/vm_event.c b/xen/arch/x86/vm_event.c
> >> index 5635603..22819c5 100644
> >> --- a/xen/arch/x86/vm_event.c
> >> +++ b/xen/arch/x86/vm_event.c
> >> @@ -20,6 +20,7 @@
> >>
> >>  #include <xen/sched.h>
> >>  #include <asm/hvm/hvm.h>
> >> +#include <asm/monitor.h>
> >>  #include <asm/vm_event.h>
> >>
> >>  /* Implicitly serialized by the domctl lock. */
> >> @@ -27,6 +28,11 @@ int vm_event_init_domain(struct domain *d)
> >>  {
> >>      struct vcpu *v;
> >>
> >> +    d->arch.monitor_msr_bitmap = xzalloc(struct monitor_msr_bitmap);
> >> +
> >> +    if ( !d->arch.monitor_msr_bitmap )
> >> +        return -ENOMEM;
> >
> > Shouldn't we in principle have a monitor_domain_init() function for
> > this, or are monitor and vm_event too intertwined in practice?
>
> I think they are (it doesn't really make sense to try to monitor MSR
> writes without initializing the vm_event subsystem). Maybe Tamas has
> different ideas?
>

There is certainly dependency between the two, but it's entirely possible
to use vm_event without the monitor subsystem (like for mem_paging and
mem_sharing). So +1 for making a monitor_domain_init that get's called only
when enabling the MONITOR ring (XEN_DOMCTL_VM_EVENT_OP_MONITOR).

Tamas

Patch

diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index f5a034a..3216e48 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -2183,8 +2183,13 @@  int xc_monitor_get_capabilities(xc_interface *xch, domid_t domain_id,
 int xc_monitor_write_ctrlreg(xc_interface *xch, domid_t domain_id,
                              uint16_t index, bool enable, bool sync,
                              bool onchangeonly);
-int xc_monitor_mov_to_msr(xc_interface *xch, domid_t domain_id, bool enable,
-                          bool extended_capture);
+/*
+ * A list of MSR indices can usually be found in /usr/include/asm/msr-index.h.
+ * Please consult the Intel/AMD manuals for more information on
+ * non-architectural indices.
+ */
+int xc_monitor_mov_to_msr(xc_interface *xch, domid_t domain_id, uint32_t msr,
+                          bool enable);
 int xc_monitor_singlestep(xc_interface *xch, domid_t domain_id, bool enable);
 int xc_monitor_software_breakpoint(xc_interface *xch, domid_t domain_id,
                                    bool enable);
diff --git a/tools/libxc/xc_monitor.c b/tools/libxc/xc_monitor.c
index b1705dd..78131b2 100644
--- a/tools/libxc/xc_monitor.c
+++ b/tools/libxc/xc_monitor.c
@@ -86,8 +86,8 @@  int xc_monitor_write_ctrlreg(xc_interface *xch, domid_t domain_id,
     return do_domctl(xch, &domctl);
 }
 
-int xc_monitor_mov_to_msr(xc_interface *xch, domid_t domain_id, bool enable,
-                          bool extended_capture)
+int xc_monitor_mov_to_msr(xc_interface *xch, domid_t domain_id, uint32_t msr,
+                          bool enable)
 {
     DECLARE_DOMCTL;
 
@@ -96,7 +96,7 @@  int xc_monitor_mov_to_msr(xc_interface *xch, domid_t domain_id, bool enable,
     domctl.u.monitor_op.op = enable ? XEN_DOMCTL_MONITOR_OP_ENABLE
                                     : XEN_DOMCTL_MONITOR_OP_DISABLE;
     domctl.u.monitor_op.event = XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR;
-    domctl.u.monitor_op.u.mov_to_msr.extended_capture = extended_capture;
+    domctl.u.monitor_op.u.mov_to_msr.msr = msr;
 
     return do_domctl(xch, &domctl);
 }
diff --git a/xen/arch/x86/hvm/event.c b/xen/arch/x86/hvm/event.c
index 56c5514..9c17f37 100644
--- a/xen/arch/x86/hvm/event.c
+++ b/xen/arch/x86/hvm/event.c
@@ -57,9 +57,8 @@  bool_t hvm_event_cr(unsigned int index, unsigned long value, unsigned long old)
 void hvm_event_msr(unsigned int msr, uint64_t value)
 {
     struct vcpu *curr = current;
-    struct arch_domain *ad = &curr->domain->arch;
 
-    if ( ad->monitor.mov_to_msr_enabled )
+    if ( monitor_is_msr_enabled(curr->domain, msr) )
     {
         vm_event_request_t req = {
             .reason = VM_EVENT_REASON_MOV_TO_MSR,
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index f24126d..447e3c2 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -3695,7 +3695,6 @@  int hvm_msr_write_intercept(unsigned int msr, uint64_t msr_content,
     bool_t mtrr;
     unsigned int edx, index;
     int ret = X86EMUL_OKAY;
-    struct arch_domain *currad = &current->domain->arch;
 
     HVMTRACE_3D(MSR_WRITE, msr,
                (uint32_t)msr_content, (uint32_t)(msr_content >> 32));
@@ -3703,7 +3702,7 @@  int hvm_msr_write_intercept(unsigned int msr, uint64_t msr_content,
     hvm_cpuid(1, NULL, NULL, NULL, &edx);
     mtrr = !!(edx & cpufeat_mask(X86_FEATURE_MTRR));
 
-    if ( may_defer && unlikely(currad->monitor.mov_to_msr_enabled) )
+    if ( may_defer && unlikely(monitor_is_msr_enabled(v->domain, msr)) )
     {
         ASSERT(v->arch.vm_event);
 
diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
index 8284281..24ad906 100644
--- a/xen/arch/x86/hvm/vmx/vmcs.c
+++ b/xen/arch/x86/hvm/vmx/vmcs.c
@@ -37,6 +37,7 @@ 
 #include <asm/hvm/vmx/vvmx.h>
 #include <asm/hvm/vmx/vmcs.h>
 #include <asm/flushtlb.h>
+#include <asm/monitor.h>
 #include <asm/shadow.h>
 #include <asm/tboot.h>
 #include <asm/apic.h>
@@ -108,18 +109,6 @@  u64 vmx_ept_vpid_cap __read_mostly;
 u64 vmx_vmfunc __read_mostly;
 bool_t vmx_virt_exception __read_mostly;
 
-const u32 vmx_introspection_force_enabled_msrs[] = {
-    MSR_IA32_SYSENTER_EIP,
-    MSR_IA32_SYSENTER_ESP,
-    MSR_IA32_SYSENTER_CS,
-    MSR_IA32_MC0_CTL,
-    MSR_STAR,
-    MSR_LSTAR
-};
-
-const unsigned int vmx_introspection_force_enabled_msrs_size =
-    ARRAY_SIZE(vmx_introspection_force_enabled_msrs);
-
 static DEFINE_PER_CPU_READ_MOSTLY(paddr_t, vmxon_region);
 static DEFINE_PER_CPU(paddr_t, current_vmcs);
 static DEFINE_PER_CPU(struct list_head, active_vmcs_list);
@@ -810,17 +799,8 @@  void vmx_disable_intercept_for_msr(struct vcpu *v, u32 msr, int type)
     if ( msr_bitmap == NULL )
         return;
 
-    if ( unlikely(d->arch.monitor.mov_to_msr_enabled &&
-                  d->arch.monitor.mov_to_msr_extended) &&
-         vm_event_check_ring(&d->vm_event->monitor) )
-    {
-        unsigned int i;
-
-        /* Filter out MSR-s needed for memory introspection */
-        for ( i = 0; i < vmx_introspection_force_enabled_msrs_size; i++ )
-            if ( msr == vmx_introspection_force_enabled_msrs[i] )
-                return;
-    }
+    if ( unlikely(monitor_is_msr_enabled(d, msr)) )
+        return;
 
     /*
      * See Intel PRM Vol. 3, 20.6.9 (MSR-Bitmap Address). Early manuals
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index bc4410f..9135441 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -1958,16 +1958,12 @@  void vmx_hypervisor_cpuid_leaf(uint32_t sub_idx,
         *eax |= XEN_HVM_CPUID_X2APIC_VIRT;
 }
 
-static void vmx_enable_msr_exit_interception(struct domain *d)
+static void vmx_enable_msr_interception(struct domain *d, uint32_t msr)
 {
     struct vcpu *v;
-    unsigned int i;
 
-    /* Enable interception for MSRs needed for memory introspection. */
     for_each_vcpu ( d, v )
-        for ( i = 0; i < vmx_introspection_force_enabled_msrs_size; i++ )
-            vmx_enable_intercept_for_msr(v, vmx_introspection_force_enabled_msrs[i],
-                                         MSR_TYPE_W);
+        vmx_enable_intercept_for_msr(v, msr, MSR_TYPE_W);
 }
 
 static bool_t vmx_is_singlestep_supported(void)
@@ -2166,7 +2162,7 @@  static struct hvm_function_table __initdata vmx_function_table = {
     .handle_eoi           = vmx_handle_eoi,
     .nhvm_hap_walk_L1_p2m = nvmx_hap_walk_L1_p2m,
     .hypervisor_cpuid_leaf = vmx_hypervisor_cpuid_leaf,
-    .enable_msr_exit_interception = vmx_enable_msr_exit_interception,
+    .enable_msr_interception = vmx_enable_msr_interception,
     .is_singlestep_supported = vmx_is_singlestep_supported,
     .set_mode = vmx_set_mode,
     .altp2m_vcpu_update_p2m = vmx_vcpu_update_eptp,
diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c
index 1fec412..2bc05ed 100644
--- a/xen/arch/x86/monitor.c
+++ b/xen/arch/x86/monitor.c
@@ -22,6 +22,74 @@ 
 #include <asm/monitor.h>
 #include <public/vm_event.h>
 
+static int monitor_enable_msr(struct domain *d, u32 msr)
+{
+    if ( !d->arch.monitor_msr_bitmap )
+        return -ENXIO;
+
+    if ( msr <= 0x1fff )
+        __set_bit(msr, &d->arch.monitor_msr_bitmap->low);
+    else if ( (msr >= 0x40000000) && (msr <= 0x40001fff) )
+    {
+        msr &= 0x1fff;
+        __set_bit(msr, &d->arch.monitor_msr_bitmap->hypervisor);
+    }
+    else if ( (msr >= 0xc0000000) && (msr <= 0xc0001fff) )
+    {
+        msr &= 0x1fff;
+        __set_bit(msr, &d->arch.monitor_msr_bitmap->high);
+    }
+
+    hvm_enable_msr_interception(d, msr);
+
+    return 0;
+}
+
+static int monitor_disable_msr(struct domain *d, u32 msr)
+{
+    if ( !d->arch.monitor_msr_bitmap )
+        return -ENXIO;
+
+    if ( msr <= 0x1fff )
+        clear_bit(msr, &d->arch.monitor_msr_bitmap->low);
+    else if ( (msr >= 0x40000000) && (msr <= 0x40001fff) )
+    {
+        msr &= 0x1fff;
+        clear_bit(msr, &d->arch.monitor_msr_bitmap->hypervisor);
+    }
+    else if ( (msr >= 0xc0000000) && (msr <= 0xc0001fff) )
+    {
+        msr &= 0x1fff;
+        clear_bit(msr, &d->arch.monitor_msr_bitmap->high);
+    }
+
+
+    return 0;
+}
+
+bool_t 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->low);
+    else if ( (msr >= 0x40000000) && (msr <= 0x40001fff) )
+    {
+        msr &= 0x1fff;
+        rc = test_bit(msr, &d->arch.monitor_msr_bitmap->hypervisor);
+    }
+    else if ( (msr >= 0xc0000000) && (msr <= 0xc0001fff) )
+    {
+        msr &= 0x1fff;
+        rc = test_bit(msr, &d->arch.monitor_msr_bitmap->high);
+    }
+
+    return rc;
+}
+
 int arch_monitor_domctl_event(struct domain *d,
                               struct xen_domctl_monitor_op *mop)
 {
@@ -77,25 +145,28 @@  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 = 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 = monitor_enable_msr(d, msr);
         else
-            ad->monitor.mov_to_msr_extended = 0;
+            rc = 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..22819c5 100644
--- a/xen/arch/x86/vm_event.c
+++ b/xen/arch/x86/vm_event.c
@@ -20,6 +20,7 @@ 
 
 #include <xen/sched.h>
 #include <asm/hvm/hvm.h>
+#include <asm/monitor.h>
 #include <asm/vm_event.h>
 
 /* Implicitly serialized by the domctl lock. */
@@ -27,6 +28,11 @@  int vm_event_init_domain(struct domain *d)
 {
     struct vcpu *v;
 
+    d->arch.monitor_msr_bitmap = xzalloc(struct monitor_msr_bitmap);
+
+    if ( !d->arch.monitor_msr_bitmap )
+        return -ENOMEM;
+
     for_each_vcpu ( d, v )
     {
         if ( v->arch.vm_event )
@@ -55,6 +61,9 @@  void vm_event_cleanup_domain(struct domain *d)
         v->arch.vm_event = NULL;
     }
 
+    xfree(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));
diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
index d393ed2..591ef26 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;
 
+    struct monitor_msr_bitmap *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..5056452 100644
--- a/xen/include/asm-x86/monitor.h
+++ b/xen/include/asm-x86/monitor.h
@@ -29,6 +29,12 @@ 
 
 #define monitor_ctrlreg_bitmask(ctrlreg_index) (1U << (ctrlreg_index))
 
+struct monitor_msr_bitmap {
+    uint8_t low[1024];
+    uint8_t hypervisor[1024];
+    uint8_t high[1024];
+};
+
 static inline
 int arch_monitor_domctl_op(struct domain *d, struct xen_domctl_monitor_op *mop)
 {
@@ -50,4 +56,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 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..7be3924 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -37,7 +37,7 @@ 
 #include "hvm/save.h"
 #include "memory.h"
 
-#define XEN_DOMCTL_INTERFACE_VERSION 0x0000000b
+#define XEN_DOMCTL_INTERFACE_VERSION 0x0000000c
 
 /*
  * NB. xen_domctl.domain is an IN/OUT parameter for this operation.
@@ -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;
         } mov_to_msr;
 
         struct {