diff mbox

[14/16] x86/monitor: clarify separation between monitor subsys and vm-event as a whole

Message ID 1468038143-7071-1-git-send-email-czuzu@bitdefender.com (mailing list archive)
State New, archived
Headers show

Commit Message

Corneliu ZUZU July 9, 2016, 4:22 a.m. UTC
Move fields in arch_vm_event in a structure called arch_vm_event_monitor and
refactor arch_vm_event to only hold a pointer to an instance of the
aforementioned.

Done for a number of reasons:

* to make the separation of monitor resources from other vm-event resources
  clearly visible

* to be able to selectively allocate/free monitor-only resources in monitor
  init & cleanup stubs

* did _not_ do a v->arch.vm_event -to- v->arch.monitor transformation because we
  want the guarantee of not occupying more than 1 pointer in arch_vcpu, i.e. if
  in the future e.g. paging vm-events would need per-vcpu resources, one would
  make an arch_vm_event_paging structure and add a pointer to an instance of it
  as a arch_vm_event.paging field, in which case we will still have only 1
  pointer (arch_vm_event) consuming memory in arch_vcpu for vm-event resources

Note also that this breaks the old implication 'vm-event initialized -> monitor
resources (which were in arch_vm_event) initialized', so 2 extra checks on
monitor_domain_initialised() had to be added:

i) in vm_event_resume() before calling monitor_ctrlreg_write_resume(),
    also an ASSERT in monitor_ctrlreg_write_resume()
ii) in vm_event_resume() before calling mem_access_resume(),
    also an ASSERT on that code-path in p2m_mem_access_emulate_check()

Finally, also note that the definition of arch_vm_event had to be moved to
asm-x86/domain.h due to a cyclic header dependency it caused between
asm-x86/vm_event.h and asm-x86/monitor.h.

Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com>
---
 xen/arch/x86/hvm/emulate.c     |  8 +++---
 xen/arch/x86/hvm/hvm.c         | 31 ++++++++++++------------
 xen/arch/x86/mm/p2m.c          |  7 ++++--
 xen/arch/x86/monitor.c         | 55 ++++++++++++++++++++++++++++++++++++------
 xen/arch/x86/vm_event.c        | 17 +++++++++++--
 xen/common/vm_event.c          | 17 +++++++++++++
 xen/include/asm-x86/domain.h   | 15 ++++++++++++
 xen/include/asm-x86/monitor.h  |  7 ++++++
 xen/include/asm-x86/vm_event.h | 13 +++-------
 9 files changed, 128 insertions(+), 42 deletions(-)

Comments

Tamas K Lengyel July 9, 2016, 6:26 p.m. UTC | #1
> diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
> index ae1dcb4..7663da2 100644
> --- a/xen/include/asm-x86/domain.h
> +++ b/xen/include/asm-x86/domain.h
> @@ -9,6 +9,7 @@
>  #include <asm/e820.h>
>  #include <asm/mce.h>
>  #include <public/vcpu.h>
> +#include <public/vm_event.h>
>  #include <public/hvm/hvm_info_table.h>
>
>  #define has_32bit_shinfo(d)    ((d)->arch.has_32bit_shinfo)
> @@ -503,6 +504,20 @@ typedef enum __packed {
>      SMAP_CHECK_DISABLED,        /* disable the check */
>  } smap_check_policy_t;
>
> +/*
> + * Should we emulate the next matching instruction on VCPU resume
> + * after a vm_event?
> + */
> +struct arch_vm_event_monitor {

This should be named struct arch_vcpu_monitor.

> +    uint32_t emulate_flags;
> +    struct vm_event_emul_read_data emul_read_data;

This should probably get renamed as well at some point to struct
monitor_emul_read_data.

> +    struct monitor_write_data write_data;
> +};
> +
> +struct arch_vm_event {
> +    struct arch_vm_event_monitor *monitor;
> +};

IMHO there is not much point in defining struct arch_vm_event this
way, we could just as well store the pointer to the arch_monitor
directly in arch_vcpu as we do right now.

> +
>  struct arch_vcpu
>  {
>      /*

Tamas
Corneliu ZUZU July 9, 2016, 6:57 p.m. UTC | #2
On 7/9/2016 9:26 PM, Tamas K Lengyel wrote:
>> diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
>> index ae1dcb4..7663da2 100644
>> --- a/xen/include/asm-x86/domain.h
>> +++ b/xen/include/asm-x86/domain.h
>> @@ -9,6 +9,7 @@
>>   #include <asm/e820.h>
>>   #include <asm/mce.h>
>>   #include <public/vcpu.h>
>> +#include <public/vm_event.h>
>>   #include <public/hvm/hvm_info_table.h>
>>
>>   #define has_32bit_shinfo(d)    ((d)->arch.has_32bit_shinfo)
>> @@ -503,6 +504,20 @@ typedef enum __packed {
>>       SMAP_CHECK_DISABLED,        /* disable the check */
>>   } smap_check_policy_t;
>>
>> +/*
>> + * Should we emulate the next matching instruction on VCPU resume
>> + * after a vm_event?
>> + */
>> +struct arch_vm_event_monitor {
> This should be named struct arch_vcpu_monitor.

Good idea.

>
>> +    uint32_t emulate_flags;
>> +    struct vm_event_emul_read_data emul_read_data;
> This should probably get renamed as well at some point to struct
> monitor_emul_read_data.

Ack.

>> +    struct monitor_write_data write_data;
>> +};
>> +
>> +struct arch_vm_event {
>> +    struct arch_vm_event_monitor *monitor;
>> +};
> IMHO there is not much point in defining struct arch_vm_event this
> way, we could just as well store the pointer to the arch_monitor
> directly in arch_vcpu as we do right now.
>

I stated the reason for that in the commit message (see 3rd '*'), Jan 
insists it would be preferable to occupy just one pointer in arch_vcpu 
(which would still hold if we do as you suggest but I was wondering how 
probable would it be in the future for one of the paging/share vm-event 
subsystems to need per-vCPU resources). But personally, yeah, I would 
have preferred what you suggest as well..

Zuzu.
Corneliu ZUZU July 13, 2016, 4:26 a.m. UTC | #3
On 7/9/2016 9:57 PM, Corneliu ZUZU wrote:
> On 7/9/2016 9:26 PM, Tamas K Lengyel wrote:
>>> diff --git a/xen/include/asm-x86/domain.h 
>>> b/xen/include/asm-x86/domain.h
>>> index ae1dcb4..7663da2 100644
>>> --- a/xen/include/asm-x86/domain.h
>>> +++ b/xen/include/asm-x86/domain.h
>>> @@ -9,6 +9,7 @@
>>>   #include <asm/e820.h>
>>>   #include <asm/mce.h>
>>>   #include <public/vcpu.h>
>>> +#include <public/vm_event.h>
>>>   #include <public/hvm/hvm_info_table.h>
>>>
>>>   #define has_32bit_shinfo(d) ((d)->arch.has_32bit_shinfo)
>>> @@ -503,6 +504,20 @@ typedef enum __packed {
>>>       SMAP_CHECK_DISABLED,        /* disable the check */
>>>   } smap_check_policy_t;
>>>
>>> +/*
>>> + * Should we emulate the next matching instruction on VCPU resume
>>> + * after a vm_event?
>>> + */
>>> +struct arch_vm_event_monitor {
>> This should be named struct arch_vcpu_monitor.
>
> Good idea.
>
>>
>>> +    uint32_t emulate_flags;
>>> +    struct vm_event_emul_read_data emul_read_data;
>> This should probably get renamed as well at some point to struct
>> monitor_emul_read_data.
>
> Ack.
>
>>> +    struct monitor_write_data write_data;
>>> +};
>>> +
>>> +struct arch_vm_event {
>>> +    struct arch_vm_event_monitor *monitor;
>>> +};
>> IMHO there is not much point in defining struct arch_vm_event this
>> way, we could just as well store the pointer to the arch_monitor
>> directly in arch_vcpu as we do right now.
>>
>
> I stated the reason for that in the commit message (see 3rd '*'), Jan 
> insists it would be preferable to occupy just one pointer in arch_vcpu 
> (which would still hold if we do as you suggest but I was wondering 
> how probable would it be in the future for one of the paging/share 
> vm-event subsystems to need per-vCPU resources). But personally, yeah, 
> I would have preferred what you suggest as well..
>
> Zuzu.

Tamas, any update on this? Do you think we should think now about 
per-vCPU resources being occupied in the future by paging/share or leave 
that for later and do what you suggested?

Thanks,
Corneliu.
Tamas K Lengyel July 13, 2016, 6:56 p.m. UTC | #4
On Tue, Jul 12, 2016 at 10:26 PM, Corneliu ZUZU <czuzu@bitdefender.com> wrote:
> On 7/9/2016 9:57 PM, Corneliu ZUZU wrote:
>>
>> On 7/9/2016 9:26 PM, Tamas K Lengyel wrote:
>>>>
>>>> diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
>>>> index ae1dcb4..7663da2 100644
>>>> --- a/xen/include/asm-x86/domain.h
>>>> +++ b/xen/include/asm-x86/domain.h
>>>> @@ -9,6 +9,7 @@
>>>>   #include <asm/e820.h>
>>>>   #include <asm/mce.h>
>>>>   #include <public/vcpu.h>
>>>> +#include <public/vm_event.h>
>>>>   #include <public/hvm/hvm_info_table.h>
>>>>
>>>>   #define has_32bit_shinfo(d) ((d)->arch.has_32bit_shinfo)
>>>> @@ -503,6 +504,20 @@ typedef enum __packed {
>>>>       SMAP_CHECK_DISABLED,        /* disable the check */
>>>>   } smap_check_policy_t;
>>>>
>>>> +/*
>>>> + * Should we emulate the next matching instruction on VCPU resume
>>>> + * after a vm_event?
>>>> + */
>>>> +struct arch_vm_event_monitor {
>>>
>>> This should be named struct arch_vcpu_monitor.
>>
>>
>> Good idea.
>>
>>>
>>>> +    uint32_t emulate_flags;
>>>> +    struct vm_event_emul_read_data emul_read_data;
>>>
>>> This should probably get renamed as well at some point to struct
>>> monitor_emul_read_data.
>>
>>
>> Ack.
>>
>>>> +    struct monitor_write_data write_data;
>>>> +};
>>>> +
>>>> +struct arch_vm_event {
>>>> +    struct arch_vm_event_monitor *monitor;
>>>> +};
>>>
>>> IMHO there is not much point in defining struct arch_vm_event this
>>> way, we could just as well store the pointer to the arch_monitor
>>> directly in arch_vcpu as we do right now.
>>>
>>
>> I stated the reason for that in the commit message (see 3rd '*'), Jan
>> insists it would be preferable to occupy just one pointer in arch_vcpu
>> (which would still hold if we do as you suggest but I was wondering how
>> probable would it be in the future for one of the paging/share vm-event
>> subsystems to need per-vCPU resources). But personally, yeah, I would have
>> preferred what you suggest as well..
>>
>> Zuzu.
>
>
> Tamas, any update on this? Do you think we should think now about per-vCPU
> resources being occupied in the future by paging/share or leave that for
> later and do what you suggested?

It's highly unlikely that either paging or sharing will have any use
of this struct in the near future provided but subsystems are pretty
much orphaned at this point. If we ever going to need it it's an easy
enough adjustment but for now it's not needed.

Tamas
diff mbox

Patch

diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
index a0094e9..7a9c6af 100644
--- a/xen/arch/x86/hvm/emulate.c
+++ b/xen/arch/x86/hvm/emulate.c
@@ -76,10 +76,10 @@  static int set_context_data(void *buffer, unsigned int size)
 
     if ( unlikely(monitor_domain_initialised(curr->domain)) )
     {
-        unsigned int safe_size =
-            min(size, curr->arch.vm_event->emul_read_data.size);
+        struct arch_vm_event_monitor *vem = curr->arch.vm_event->monitor;
+        unsigned int safe_size = min(size, vem->emul_read_data.size);
 
-        memcpy(buffer, curr->arch.vm_event->emul_read_data.data, safe_size);
+        memcpy(buffer, vem->emul_read_data.data, safe_size);
         memset(buffer + safe_size, 0, size - safe_size);
         return X86EMUL_OKAY;
     }
@@ -524,7 +524,7 @@  static int hvmemul_virtual_to_linear(
      * vm_event being triggered for repeated writes to a whole page.
      */
     if ( unlikely(current->domain->arch.mem_access_emulate_each_rep) &&
-         current->arch.vm_event->emulate_flags != 0 )
+         current->arch.vm_event->monitor->emulate_flags != 0 )
        max_reps = 1;
 
     /*
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 46fed33..1bfd9d4 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -65,7 +65,6 @@ 
 #include <asm/altp2m.h>
 #include <asm/mtrr.h>
 #include <asm/apic.h>
-#include <asm/vm_event.h>
 #include <public/sched.h>
 #include <public/hvm/ioreq.h>
 #include <public/version.h>
@@ -473,21 +472,21 @@  void hvm_do_resume(struct vcpu *v)
 
     if ( unlikely(monitor_domain_initialised(v->domain)) )
     {
-        if ( unlikely(v->arch.vm_event->emulate_flags) )
+        struct arch_vm_event_monitor *vem = v->arch.vm_event->monitor;
+
+        if ( unlikely(vem->emulate_flags) )
         {
             enum emul_kind kind = EMUL_KIND_NORMAL;
 
-            if ( v->arch.vm_event->emulate_flags &
-                 VM_EVENT_FLAG_SET_EMUL_READ_DATA )
+            if ( vem->emulate_flags & VM_EVENT_FLAG_SET_EMUL_READ_DATA )
                 kind = EMUL_KIND_SET_CONTEXT;
-            else if ( v->arch.vm_event->emulate_flags &
-                      VM_EVENT_FLAG_EMULATE_NOWRITE )
+            else if ( vem->emulate_flags & VM_EVENT_FLAG_EMULATE_NOWRITE )
                 kind = EMUL_KIND_NOWRITE;
 
             hvm_mem_access_emulate_one(kind, TRAP_invalid_op,
                                        HVM_DELIVER_NO_ERROR_CODE);
 
-            v->arch.vm_event->emulate_flags = 0;
+            vem->emulate_flags = 0;
         }
 
         monitor_ctrlreg_write_data(v);
@@ -2184,8 +2183,8 @@  int hvm_set_cr0(unsigned long value, bool_t may_defer)
              * The actual write will occur in monitor_ctrlreg_write_data(), if
              * permitted.
              */
-            v->arch.vm_event->write_data.do_write.cr0 = 1;
-            v->arch.vm_event->write_data.cr0 = value;
+            v->arch.vm_event->monitor->write_data.do_write.cr0 = 1;
+            v->arch.vm_event->monitor->write_data.cr0 = value;
 
             return X86EMUL_OKAY;
         }
@@ -2289,8 +2288,8 @@  int hvm_set_cr3(unsigned long value, bool_t may_defer)
              * The actual write will occur in monitor_ctrlreg_write_data(), if
              * permitted.
              */
-            v->arch.vm_event->write_data.do_write.cr3 = 1;
-            v->arch.vm_event->write_data.cr3 = value;
+            v->arch.vm_event->monitor->write_data.do_write.cr3 = 1;
+            v->arch.vm_event->monitor->write_data.cr3 = value;
 
             return X86EMUL_OKAY;
         }
@@ -2372,8 +2371,8 @@  int hvm_set_cr4(unsigned long value, bool_t may_defer)
              * The actual write will occur in monitor_ctrlreg_write_data(), if
              * permitted.
              */
-            v->arch.vm_event->write_data.do_write.cr4 = 1;
-            v->arch.vm_event->write_data.cr4 = value;
+            v->arch.vm_event->monitor->write_data.do_write.cr4 = 1;
+            v->arch.vm_event->monitor->write_data.cr4 = value;
 
             return X86EMUL_OKAY;
         }
@@ -3754,9 +3753,9 @@  int hvm_msr_write_intercept(unsigned int msr, uint64_t msr_content,
          * The actual write will occur in monitor_ctrlreg_write_data(), if
          * permitted.
          */
-        v->arch.vm_event->write_data.do_write.msr = 1;
-        v->arch.vm_event->write_data.msr = msr;
-        v->arch.vm_event->write_data.value = msr_content;
+        v->arch.vm_event->monitor->write_data.do_write.msr = 1;
+        v->arch.vm_event->monitor->write_data.msr = msr;
+        v->arch.vm_event->monitor->write_data.value = msr_content;
 
         hvm_monitor_msr(msr, msr_content);
         return X86EMUL_OKAY;
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index 16733a4..92c0576 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -1591,12 +1591,15 @@  void p2m_mem_paging_resume(struct domain *d, vm_event_response_t *rsp)
 void p2m_mem_access_emulate_check(struct vcpu *v,
                                   const vm_event_response_t *rsp)
 {
+    ASSERT(monitor_domain_initialised(v->domain));
+
     /* Mark vcpu for skipping one instruction upon rescheduling. */
     if ( rsp->flags & VM_EVENT_FLAG_EMULATE )
     {
         xenmem_access_t access;
         bool_t violation = 1;
         const struct vm_event_mem_access *data = &rsp->u.mem_access;
+        struct arch_vm_event_monitor *vem = v->arch.vm_event->monitor;
 
         if ( p2m_get_mem_access(v->domain, _gfn(data->gfn), &access) == 0 )
         {
@@ -1639,10 +1642,10 @@  void p2m_mem_access_emulate_check(struct vcpu *v,
             }
         }
 
-        v->arch.vm_event->emulate_flags = violation ? rsp->flags : 0;
+        vem->emulate_flags = violation ? rsp->flags : 0;
 
         if ( (rsp->flags & VM_EVENT_FLAG_SET_EMUL_READ_DATA) )
-            v->arch.vm_event->emul_read_data = rsp->data.emul_read_data;
+            vem->emul_read_data = rsp->data.emul_read_data;
     }
 }
 
diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c
index 0763ed7..c3123bc 100644
--- a/xen/arch/x86/monitor.c
+++ b/xen/arch/x86/monitor.c
@@ -21,7 +21,6 @@ 
 
 #include <asm/hvm/vmx/vmx.h>
 #include <asm/monitor.h>
-#include <asm/vm_event.h>
 #include <public/vm_event.h>
 
 static inline
@@ -55,16 +54,43 @@  static inline void monitor_ctrlreg_disable_traps(struct domain *d)
 /* Implicitly serialized by the domctl lock. */
 int monitor_init_domain(struct domain *d)
 {
-    if ( likely(!d->arch.monitor.msr_bitmap) )
+    int rc = 0;
+    struct vcpu *v;
+
+    /* Already initialised? */
+    if ( unlikely(monitor_domain_initialised(d)) )
+        return 0;
+
+    /* Per-vcpu initializations. */
+    for_each_vcpu(d, v)
+    {
+        ASSERT(v->arch.vm_event);
+        ASSERT(!v->arch.vm_event->monitor);
+
+        v->arch.vm_event->monitor = xzalloc(struct arch_vm_event_monitor);
+        if ( unlikely(!v->arch.vm_event->monitor) )
+        {
+            rc = -ENOMEM;
+            goto err;
+        }
+    }
+
+    ASSERT(!d->arch.monitor.msr_bitmap);
+    d->arch.monitor.msr_bitmap = xzalloc(struct monitor_msr_bitmap);
+    if ( unlikely(!d->arch.monitor.msr_bitmap) )
     {
-        d->arch.monitor.msr_bitmap = xzalloc(struct monitor_msr_bitmap);
-        if ( unlikely(!d->arch.monitor.msr_bitmap) )
-            return -ENOMEM;
+        rc = -ENOMEM;
+        goto err;
     }
 
     d->monitor.initialised = 1;
 
-    return 0;
+err:
+    if ( unlikely(rc) )
+        /* On fail cleanup whatever resources have been partially allocated. */
+        monitor_cleanup_domain(d);
+
+    return rc;
 }
 
 /*
@@ -73,9 +99,20 @@  int monitor_init_domain(struct domain *d)
  */
 void monitor_cleanup_domain(struct domain *d)
 {
+    struct vcpu *v;
+
     xfree(d->arch.monitor.msr_bitmap);
     d->arch.monitor.msr_bitmap = NULL;
 
+    for_each_vcpu(d, v)
+    {
+        if ( unlikely(!v->arch.vm_event || !v->arch.vm_event->monitor) )
+            continue;
+
+        xfree(v->arch.vm_event->monitor);
+        v->arch.vm_event->monitor = NULL;
+    }
+
     monitor_ctrlreg_disable_traps(d);
 
     memset(&d->arch.monitor, 0, sizeof(d->arch.monitor));
@@ -88,6 +125,8 @@  void monitor_cleanup_domain(struct domain *d)
 
 void monitor_ctrlreg_write_resume(struct vcpu *v, vm_event_response_t *rsp)
 {
+    ASSERT(monitor_domain_initialised(v->domain));
+
     if ( rsp->flags & VM_EVENT_FLAG_DENY )
     {
         struct monitor_write_data *w;
@@ -98,7 +137,7 @@  void monitor_ctrlreg_write_resume(struct vcpu *v, vm_event_response_t *rsp)
         if ( !atomic_read(&v->vm_event_pause_count) )
             return;
 
-        w = &v->arch.vm_event->write_data;
+        w = &v->arch.vm_event->monitor->write_data;
 
         switch ( rsp->reason )
         {
@@ -125,7 +164,7 @@  void monitor_ctrlreg_write_resume(struct vcpu *v, vm_event_response_t *rsp)
 
 void monitor_ctrlreg_write_data(struct vcpu *v)
 {
-    struct monitor_write_data *w = &v->arch.vm_event->write_data;
+    struct monitor_write_data *w = &v->arch.vm_event->monitor->write_data;
 
     if ( likely(!w->writes_pending) )
         return;
diff --git a/xen/arch/x86/vm_event.c b/xen/arch/x86/vm_event.c
index e2b258b..20cb1b0 100644
--- a/xen/arch/x86/vm_event.c
+++ b/xen/arch/x86/vm_event.c
@@ -65,9 +65,22 @@  void vm_event_cleanup_domain(struct domain *d, struct vm_event_domain *ved)
     if ( &d->vm_event->monitor == ved )
         monitor_cleanup_domain(d);
 
-    /* Per-vcpu uninitializations. */
+    /*
+     * Per-vCPU: if all resources of all vm-event subsystems were freed,
+     * also free v->arch.vm_event.
+     */
     for_each_vcpu ( d, v )
-        vm_event_cleanup_vcpu_destroy(v);
+    {
+        if ( unlikely(!v->arch.vm_event) )
+            continue;
+
+        /* Are there resources of vm-event subsystems left unfreed? */
+        if ( unlikely(v->arch.vm_event->monitor) )
+            continue;
+
+        xfree(v->arch.vm_event);
+        v->arch.vm_event = NULL;
+    }
 }
 
 void vm_event_toggle_singlestep(struct domain *d, struct vcpu *v)
diff --git a/xen/common/vm_event.c b/xen/common/vm_event.c
index dafe7bf..c409b80 100644
--- a/xen/common/vm_event.c
+++ b/xen/common/vm_event.c
@@ -22,6 +22,7 @@ 
 
 #include <xen/sched.h>
 #include <xen/event.h>
+#include <xen/monitor.h>
 #include <xen/wait.h>
 #include <xen/vm_event.h>
 #include <xen/mem_access.h>
@@ -397,11 +398,27 @@  void vm_event_resume(struct domain *d, struct vm_event_domain *ved)
         case VM_EVENT_REASON_MOV_TO_MSR:
 #endif
         case VM_EVENT_REASON_WRITE_CTRLREG:
+            if ( unlikely(!monitor_domain_initialised(d)) )
+            {
+                printk(XENLOG_G_WARNING "Vm-event monitor subsystem not"
+                                        "initialized, cr-write ring response"
+                                        "ignored (hint: have you called"
+                                        "xc_monitor_enable()?).\n");
+                continue;
+            }
             monitor_ctrlreg_write_resume(v, &rsp);
             break;
 
 #ifdef CONFIG_HAS_MEM_ACCESS
         case VM_EVENT_REASON_MEM_ACCESS:
+            if ( unlikely(!monitor_domain_initialised(d)) )
+            {
+                printk(XENLOG_G_WARNING "Vm-event monitor subsystem not"
+                                        "initialized, mem-access ring response"
+                                        "ignored (hint: have you called"
+                                        "xc_monitor_enable()?).\n");
+                continue;
+            }
             mem_access_resume(v, &rsp);
             break;
 #endif
diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
index ae1dcb4..7663da2 100644
--- a/xen/include/asm-x86/domain.h
+++ b/xen/include/asm-x86/domain.h
@@ -9,6 +9,7 @@ 
 #include <asm/e820.h>
 #include <asm/mce.h>
 #include <public/vcpu.h>
+#include <public/vm_event.h>
 #include <public/hvm/hvm_info_table.h>
 
 #define has_32bit_shinfo(d)    ((d)->arch.has_32bit_shinfo)
@@ -503,6 +504,20 @@  typedef enum __packed {
     SMAP_CHECK_DISABLED,        /* disable the check */
 } smap_check_policy_t;
 
+/*
+ * Should we emulate the next matching instruction on VCPU resume
+ * after a vm_event?
+ */
+struct arch_vm_event_monitor {
+    uint32_t emulate_flags;
+    struct vm_event_emul_read_data emul_read_data;
+    struct monitor_write_data write_data;
+};
+
+struct arch_vm_event {
+    struct arch_vm_event_monitor *monitor;
+};
+
 struct arch_vcpu
 {
     /*
diff --git a/xen/include/asm-x86/monitor.h b/xen/include/asm-x86/monitor.h
index 4014f8d..11497ef 100644
--- a/xen/include/asm-x86/monitor.h
+++ b/xen/include/asm-x86/monitor.h
@@ -90,6 +90,13 @@  int monitor_init_domain(struct domain *d);
 
 void monitor_cleanup_domain(struct domain *d);
 
+/* Called on vCPU destroy. */
+static inline void monitor_cleanup_vcpu_destroy(struct vcpu *v)
+{
+    ASSERT(v->arch.vm_event);
+    xfree(v->arch.vm_event->monitor);
+}
+
 void monitor_ctrlreg_write_resume(struct vcpu *v, vm_event_response_t *rsp);
 
 void monitor_ctrlreg_write_data(struct vcpu *v);
diff --git a/xen/include/asm-x86/vm_event.h b/xen/include/asm-x86/vm_event.h
index c1b82ab..3c14d4f 100644
--- a/xen/include/asm-x86/vm_event.h
+++ b/xen/include/asm-x86/vm_event.h
@@ -20,16 +20,7 @@ 
 #define __ASM_X86_VM_EVENT_H__
 
 #include <xen/sched.h>
-
-/*
- * Should we emulate the next matching instruction on VCPU resume
- * after a vm_event?
- */
-struct arch_vm_event {
-    uint32_t emulate_flags;
-    struct vm_event_emul_read_data emul_read_data;
-    struct monitor_write_data write_data;
-};
+#include <asm/monitor.h>
 
 int vm_event_init_domain(struct domain *d, struct vm_event_domain *ved);
 
@@ -41,6 +32,8 @@  static inline void vm_event_cleanup_vcpu_destroy(struct vcpu *v)
     if ( likely(!v->arch.vm_event) )
         return;
 
+    monitor_cleanup_vcpu_destroy(v);
+
     xfree(v->arch.vm_event);
     v->arch.vm_event = NULL;
 }