[PATCH 0/5] Vm_event memory introspection helpers

[PATCH 0/5] Vm_event memory introspection helpers

[Razvan Cojocaru]

This series adds a number of useful tools for guest memory
introspection:

* XSETBV / XCR vm_events;
* support for memory-content hiding (sending data back via the
  vm_event response, to be used as data read when emulating an
  instruction;
* support for generic vm_events that the guest can request via
  VMCALLs;
* preemption for MSR writes;
* support for setting CR0, CR3 and CR4 from userspace.

Patches:

[PATCH 1/5] xen/vm_event: Added support for XSETBV events
[PATCH 2/5] xen/vm_access: Support for memory-content hiding
[PATCH 3/5] xen/vm_event: Support for guest-requested events
[PATCH 4/5] xen/vm_event: Deny MSR writes if refused by vm_event
[PATCH 5/5] xen: Write CR0, CR3 and CR4 in arch_set_info_guest()


Thanks,
Razvan Cojocaru

[PATCH 1/5] xen/vm_event: Added support for XSETBV events

[Razvan Cojocaru]

This patch adds XSETBV (XCR) vm_events.

Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
---
 tools/libxc/include/xenctrl.h   |    2 ++
 tools/libxc/xc_monitor.c        |   15 +++++++++++++++
 xen/arch/x86/hvm/event.c        |   16 ++++++++++++++++
 xen/arch/x86/hvm/hvm.c          |    2 ++
 xen/arch/x86/monitor.c          |   16 ++++++++++++++++
 xen/include/asm-x86/domain.h    |    2 ++
 xen/include/asm-x86/hvm/event.h |    1 +
 xen/include/public/domctl.h     |    6 ++++++
 xen/include/public/vm_event.h   |    8 ++++++++
 9 files changed, 68 insertions(+)

diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index 6994c51..1aa4f87 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -2337,6 +2337,8 @@ int xc_monitor_mov_to_msr(xc_interface *xch, domid_t domain_id, 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);
+int xc_monitor_xsetbv(xc_interface *xch, domid_t domain_id, bool enable,
+                      bool sync);
 
 /***
  * Memory sharing operations.
diff --git a/tools/libxc/xc_monitor.c b/tools/libxc/xc_monitor.c
index 87ad968..aec2f4a 100644
--- a/tools/libxc/xc_monitor.c
+++ b/tools/libxc/xc_monitor.c
@@ -135,3 +135,18 @@ int xc_monitor_singlestep(xc_interface *xch, domid_t domain_id,
 
     return do_domctl(xch, &domctl);
 }
+
+int xc_monitor_xsetbv(xc_interface *xch, domid_t domain_id, bool enable,
+                      bool sync)
+{
+    DECLARE_DOMCTL;
+
+    domctl.cmd = XEN_DOMCTL_monitor_op;
+    domctl.domain = domain_id;
+    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_XSETBV;
+    domctl.u.monitor_op.u.xsetbv.sync = sync;
+
+    return do_domctl(xch, &domctl);
+}
diff --git a/xen/arch/x86/hvm/event.c b/xen/arch/x86/hvm/event.c
index 9d5f9f3..5b869c8 100644
--- a/xen/arch/x86/hvm/event.c
+++ b/xen/arch/x86/hvm/event.c
@@ -151,6 +151,22 @@ void hvm_event_msr(unsigned int msr, uint64_t value)
         hvm_event_traps(1, &req);
 }
 
+void hvm_event_xsetbv(unsigned long xcr, uint64_t value)
+{
+    struct vcpu *curr = current;
+    struct arch_domain *currad = &current->domain->arch;
+
+    vm_event_request_t req = {
+        .reason = VM_EVENT_REASON_XSETBV,
+        .vcpu_id = curr->vcpu_id,
+        .u.xsetbv.xcr = xcr,
+        .u.xsetbv.value = value,
+    };
+
+    if ( currad->monitor.xsetbv_enabled )
+        hvm_event_traps(currad->monitor.xsetbv_sync, &req);
+}
+
 int hvm_event_int3(unsigned long gla)
 {
     int rc = 0;
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 3a09439..86f9885 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -2966,6 +2966,8 @@ int hvm_handle_xsetbv(u32 index, u64 new_bv)
 {
     struct segment_register sreg;
 
+    hvm_event_xsetbv(index, new_bv);
+
     hvm_get_segment_register(current, x86_seg_ss, &sreg);
     if ( sreg.attr.fields.dpl != 0 )
         goto err;
diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c
index d7b1c18..6823a84 100644
--- a/xen/arch/x86/monitor.c
+++ b/xen/arch/x86/monitor.c
@@ -178,6 +178,22 @@ int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *mop)
         break;
     }
 
+    case XEN_DOMCTL_MONITOR_EVENT_XSETBV:
+    {
+        bool_t status = ad->monitor.xsetbv_enabled;
+
+        rc = status_check(mop, status);
+        if ( rc )
+            return rc;
+
+        ad->monitor.xsetbv_sync = mop->u.xsetbv.sync;
+
+        domain_pause(d);
+        ad->monitor.xsetbv_enabled = !status;
+        domain_unpause(d);
+        break;
+    }
+
     default:
         return -EOPNOTSUPP;
 
diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
index 3f83e8b..452a9b3 100644
--- a/xen/include/asm-x86/domain.h
+++ b/xen/include/asm-x86/domain.h
@@ -354,6 +354,8 @@ struct arch_domain
         uint16_t mov_to_msr_extended         : 1;
         uint16_t singlestep_enabled          : 1;
         uint16_t software_breakpoint_enabled : 1;
+        uint16_t xsetbv_enabled              : 1;
+        uint16_t xsetbv_sync                 : 1;
     } monitor;
 
     /* Mem_access emulation control */
diff --git a/xen/include/asm-x86/hvm/event.h b/xen/include/asm-x86/hvm/event.h
index bb757a1..b2cf3bc 100644
--- a/xen/include/asm-x86/hvm/event.h
+++ b/xen/include/asm-x86/hvm/event.h
@@ -23,6 +23,7 @@ void hvm_event_cr0(unsigned long value, unsigned long old);
 void hvm_event_cr3(unsigned long value, unsigned long old);
 void hvm_event_cr4(unsigned long value, unsigned long old);
 void hvm_event_msr(unsigned int msr, uint64_t value);
+void hvm_event_xsetbv(unsigned long xcr, uint64_t value);
 /* Called for current VCPU: returns -1 if no listener */
 int hvm_event_int3(unsigned long gla);
 int hvm_event_single_step(unsigned long gla);
diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index 10b51ef..b866e33 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -1018,6 +1018,7 @@ DEFINE_XEN_GUEST_HANDLE(xen_domctl_psr_cmt_op_t);
 #define XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR            3
 #define XEN_DOMCTL_MONITOR_EVENT_SINGLESTEP            4
 #define XEN_DOMCTL_MONITOR_EVENT_SOFTWARE_BREAKPOINT   5
+#define XEN_DOMCTL_MONITOR_EVENT_XSETBV                6
 
 struct xen_domctl_monitor_op {
     uint32_t op; /* XEN_DOMCTL_MONITOR_OP_* */
@@ -1038,6 +1039,11 @@ struct xen_domctl_monitor_op {
             /* Enable the capture of an extended set of MSRs */
             uint8_t extended_capture;
         } mov_to_msr;
+
+        struct {
+            /* Pause vCPU until response */
+            uint8_t sync;
+        } xsetbv;
     } u;
 };
 typedef struct xen_domctl__op xen_domctl_monitor_op_t;
diff --git a/xen/include/public/vm_event.h b/xen/include/public/vm_event.h
index c7426de..71fe9ba 100644
--- a/xen/include/public/vm_event.h
+++ b/xen/include/public/vm_event.h
@@ -72,6 +72,8 @@
 #define VM_EVENT_REASON_SOFTWARE_BREAKPOINT     8
 /* Single-step (e.g. MTF) */
 #define VM_EVENT_REASON_SINGLESTEP              9
+/* An XCR was updated. */
+#define VM_EVENT_REASON_XSETBV                 10
 
 /*
  * Using a custom struct (not hvm_hw_cpu) so as to not fill
@@ -186,6 +188,11 @@ struct vm_event_sharing {
     uint32_t _pad;
 };
 
+struct vm_event_xsetbv {
+    uint64_t xcr;
+    uint64_t value;
+};
+
 typedef struct vm_event_st {
     uint32_t version;   /* VM_EVENT_INTERFACE_VERSION */
     uint32_t flags;     /* VM_EVENT_FLAG_* */
@@ -200,6 +207,7 @@ typedef struct vm_event_st {
         struct vm_event_mov_to_msr            mov_to_msr;
         struct vm_event_debug                 software_breakpoint;
         struct vm_event_debug                 singlestep;
+        struct vm_event_xsetbv                xsetbv;
     } u;
 
     union {
-- 
1.7.9.5

Re: [PATCH 1/5] xen/vm_event: Added support for XSETBV events

[Tim Deegan]

At 20:12 +0300 on 06 May (1430943148), Razvan Cojocaru wrote:
> This patch adds XSETBV (XCR) vm_events.
> 
> Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>

After making things more architecture-neutral in the API rework, it
seems a shame to have this called 'xsetbv'.  But since we already have
CR0 etc in this interface, I guess that's OK.  

Acked-by: Tim Deegan <tim@xen.org>

Re: [PATCH 1/5] xen/vm_event: Added support for XSETBV events

[Andrew Cooper]

On 07/05/15 16:43, Tim Deegan wrote:
> At 20:12 +0300 on 06 May (1430943148), Razvan Cojocaru wrote:
>> This patch adds XSETBV (XCR) vm_events.
>>
>> Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
> After making things more architecture-neutral in the API rework, it
> seems a shame to have this called 'xsetbv'.  But since we already have
> CR0 etc in this interface, I guess that's OK.  
>
> Acked-by: Tim Deegan <tim@xen.org>

While we are still in the period of ABI flux, might it be appropriate to
take the cleanup one step further and separate the common ABI/API from
architecture specific bits?

e.g. split public/vm_event.h to have arch-x86/vm_event.h as well, and
declare VM_EVENT_REASON_ARCH starting at 0x80000000 or so?  The x86
specific structures can become vm_event_x86_$foo.

It seems worthwhile, and we have until 4.6 is released to make changes
like this.

~Andrew

Re: [PATCH 1/5] xen/vm_event: Added support for XSETBV events

[Andrew Cooper]

On 06/05/15 18:12, Razvan Cojocaru wrote:
> This patch adds XSETBV (XCR) vm_events.
>
> Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
> ---
>  tools/libxc/include/xenctrl.h   |    2 ++
>  tools/libxc/xc_monitor.c        |   15 +++++++++++++++
>  xen/arch/x86/hvm/event.c        |   16 ++++++++++++++++
>  xen/arch/x86/hvm/hvm.c          |    2 ++
>  xen/arch/x86/monitor.c          |   16 ++++++++++++++++
>  xen/include/asm-x86/domain.h    |    2 ++
>  xen/include/asm-x86/hvm/event.h |    1 +
>  xen/include/public/domctl.h     |    6 ++++++
>  xen/include/public/vm_event.h   |    8 ++++++++
>  9 files changed, 68 insertions(+)
>
> diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
> index 6994c51..1aa4f87 100644
> --- a/tools/libxc/include/xenctrl.h
> +++ b/tools/libxc/include/xenctrl.h
> @@ -2337,6 +2337,8 @@ int xc_monitor_mov_to_msr(xc_interface *xch, domid_t domain_id, 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);
> +int xc_monitor_xsetbv(xc_interface *xch, domid_t domain_id, bool enable,
> +                      bool sync);
>  
>  /***
>   * Memory sharing operations.
> diff --git a/tools/libxc/xc_monitor.c b/tools/libxc/xc_monitor.c
> index 87ad968..aec2f4a 100644
> --- a/tools/libxc/xc_monitor.c
> +++ b/tools/libxc/xc_monitor.c
> @@ -135,3 +135,18 @@ int xc_monitor_singlestep(xc_interface *xch, domid_t domain_id,
>  
>      return do_domctl(xch, &domctl);
>  }
> +
> +int xc_monitor_xsetbv(xc_interface *xch, domid_t domain_id, bool enable,
> +                      bool sync)
> +{
> +    DECLARE_DOMCTL;
> +
> +    domctl.cmd = XEN_DOMCTL_monitor_op;
> +    domctl.domain = domain_id;
> +    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_XSETBV;
> +    domctl.u.monitor_op.u.xsetbv.sync = sync;
> +
> +    return do_domctl(xch, &domctl);
> +}
> diff --git a/xen/arch/x86/hvm/event.c b/xen/arch/x86/hvm/event.c
> index 9d5f9f3..5b869c8 100644
> --- a/xen/arch/x86/hvm/event.c
> +++ b/xen/arch/x86/hvm/event.c
> @@ -151,6 +151,22 @@ void hvm_event_msr(unsigned int msr, uint64_t value)
>          hvm_event_traps(1, &req);
>  }
>  
> +void hvm_event_xsetbv(unsigned long xcr, uint64_t value)

Architecturally, the XCR index is specified in ecx, making it only 32
bits wide. 

> +{
> +    struct vcpu *curr = current;
> +    struct arch_domain *currad = &current->domain->arch;
> +
> +    vm_event_request_t req = {
> +        .reason = VM_EVENT_REASON_XSETBV,
> +        .vcpu_id = curr->vcpu_id,
> +        .u.xsetbv.xcr = xcr,
> +        .u.xsetbv.value = value,
> +    };
> +
> +    if ( currad->monitor.xsetbv_enabled )
> +        hvm_event_traps(currad->monitor.xsetbv_sync, &req);
> +}
> +
>  int hvm_event_int3(unsigned long gla)
>  {
>      int rc = 0;
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 3a09439..86f9885 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -2966,6 +2966,8 @@ int hvm_handle_xsetbv(u32 index, u64 new_bv)
>  {
>      struct segment_register sreg;
>  
> +    hvm_event_xsetbv(index, new_bv);
> +
>      hvm_get_segment_register(current, x86_seg_ss, &sreg);
>      if ( sreg.attr.fields.dpl != 0 )
>          goto err;
> diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c
> index d7b1c18..6823a84 100644
> --- a/xen/arch/x86/monitor.c
> +++ b/xen/arch/x86/monitor.c
> @@ -178,6 +178,22 @@ int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *mop)
>          break;
>      }
>  
> +    case XEN_DOMCTL_MONITOR_EVENT_XSETBV:
> +    {
> +        bool_t status = ad->monitor.xsetbv_enabled;
> +
> +        rc = status_check(mop, status);
> +        if ( rc )
> +            return rc;
> +
> +        ad->monitor.xsetbv_sync = mop->u.xsetbv.sync;
> +
> +        domain_pause(d);
> +        ad->monitor.xsetbv_enabled = !status;
> +        domain_unpause(d);
> +        break;
> +    }
> +
>      default:
>          return -EOPNOTSUPP;
>  
> diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
> index 3f83e8b..452a9b3 100644
> --- a/xen/include/asm-x86/domain.h
> +++ b/xen/include/asm-x86/domain.h
> @@ -354,6 +354,8 @@ struct arch_domain
>          uint16_t mov_to_msr_extended         : 1;
>          uint16_t singlestep_enabled          : 1;
>          uint16_t software_breakpoint_enabled : 1;
> +        uint16_t xsetbv_enabled              : 1;
> +        uint16_t xsetbv_sync                 : 1;
>      } monitor;
>  
>      /* Mem_access emulation control */
> diff --git a/xen/include/asm-x86/hvm/event.h b/xen/include/asm-x86/hvm/event.h
> index bb757a1..b2cf3bc 100644
> --- a/xen/include/asm-x86/hvm/event.h
> +++ b/xen/include/asm-x86/hvm/event.h
> @@ -23,6 +23,7 @@ void hvm_event_cr0(unsigned long value, unsigned long old);
>  void hvm_event_cr3(unsigned long value, unsigned long old);
>  void hvm_event_cr4(unsigned long value, unsigned long old);
>  void hvm_event_msr(unsigned int msr, uint64_t value);
> +void hvm_event_xsetbv(unsigned long xcr, uint64_t value);
>  /* Called for current VCPU: returns -1 if no listener */
>  int hvm_event_int3(unsigned long gla);
>  int hvm_event_single_step(unsigned long gla);
> diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
> index 10b51ef..b866e33 100644
> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -1018,6 +1018,7 @@ DEFINE_XEN_GUEST_HANDLE(xen_domctl_psr_cmt_op_t);
>  #define XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR            3
>  #define XEN_DOMCTL_MONITOR_EVENT_SINGLESTEP            4
>  #define XEN_DOMCTL_MONITOR_EVENT_SOFTWARE_BREAKPOINT   5
> +#define XEN_DOMCTL_MONITOR_EVENT_XSETBV                6
>  
>  struct xen_domctl_monitor_op {
>      uint32_t op; /* XEN_DOMCTL_MONITOR_OP_* */
> @@ -1038,6 +1039,11 @@ struct xen_domctl_monitor_op {
>              /* Enable the capture of an extended set of MSRs */
>              uint8_t extended_capture;
>          } mov_to_msr;
> +
> +        struct {
> +            /* Pause vCPU until response */
> +            uint8_t sync;
> +        } xsetbv;
>      } u;
>  };
>  typedef struct xen_domctl__op xen_domctl_monitor_op_t;
> diff --git a/xen/include/public/vm_event.h b/xen/include/public/vm_event.h
> index c7426de..71fe9ba 100644
> --- a/xen/include/public/vm_event.h
> +++ b/xen/include/public/vm_event.h
> @@ -72,6 +72,8 @@
>  #define VM_EVENT_REASON_SOFTWARE_BREAKPOINT     8
>  /* Single-step (e.g. MTF) */
>  #define VM_EVENT_REASON_SINGLESTEP              9
> +/* An XCR was updated. */
> +#define VM_EVENT_REASON_XSETBV                 10
>  
>  /*
>   * Using a custom struct (not hvm_hw_cpu) so as to not fill
> @@ -186,6 +188,11 @@ struct vm_event_sharing {
>      uint32_t _pad;
>  };
>  
> +struct vm_event_xsetbv {
> +    uint64_t xcr;
> +    uint64_t value;
> +};
> +

In an effort to be architecture neutral, it might be an idea to have
something like

struct vm_event_write_cr {
    uint64_t index;
    uint64_t old_val, new_val;
};

And have a per-arch index of control registers, such as

X86_CR0
X86_CR3
X86_CR4
X86_XCR0
...
ARM32_$foo

(See also my reply to Tim on this thread)

~Andrew

>  typedef struct vm_event_st {
>      uint32_t version;   /* VM_EVENT_INTERFACE_VERSION */
>      uint32_t flags;     /* VM_EVENT_FLAG_* */
> @@ -200,6 +207,7 @@ typedef struct vm_event_st {
>          struct vm_event_mov_to_msr            mov_to_msr;
>          struct vm_event_debug                 software_breakpoint;
>          struct vm_event_debug                 singlestep;
> +        struct vm_event_xsetbv                xsetbv;
>      } u;
>  
>      union {

Re: [PATCH 1/5] xen/vm_event: Added support for XSETBV events

[Razvan Cojocaru]

On 05/07/2015 09:03 PM, Andrew Cooper wrote:
> In an effort to be architecture neutral, it might be an idea to have
> something like
> 
> struct vm_event_write_cr {
>     uint64_t index;
>     uint64_t old_val, new_val;
> };
> 
> And have a per-arch index of control registers, such as
> 
> X86_CR0
> X86_CR3
> X86_CR4
> X86_XCR0
> ...
> ARM32_$foo
> 
> (See also my reply to Tim on this thread)

Sure, if everyone thinks this is the way forward, I can submit a patch
for that as well (would you prefer it within, or outside the series?).


Thanks,
Razvan

Re: [PATCH 1/5] xen/vm_event: Added support for XSETBV events

[Jan Beulich]

>>> On 08.05.15 at 08:18, <rcojocaru@bitdefender.com> wrote:
> On 05/07/2015 09:03 PM, Andrew Cooper wrote:
>> In an effort to be architecture neutral, it might be an idea to have
>> something like
>> 
>> struct vm_event_write_cr {
>>     uint64_t index;
>>     uint64_t old_val, new_val;
>> };
>> 
>> And have a per-arch index of control registers, such as
>> 
>> X86_CR0
>> X86_CR3
>> X86_CR4
>> X86_XCR0
>> ...
>> ARM32_$foo
>> 
>> (See also my reply to Tim on this thread)
> 
> Sure, if everyone thinks this is the way forward, I can submit a patch
> for that as well

I would welcome this (and had tried to hint in this direction already
in past patch reviews).

> (would you prefer it within, or outside the series?).

Afaic either way is fine.

Jan

Re: [PATCH 1/5] xen/vm_event: Added support for XSETBV events

[Razvan Cojocaru]

On 05/07/2015 09:03 PM, Andrew Cooper wrote:
> In an effort to be architecture neutral, it might be an idea to have
> something like
> 
> struct vm_event_write_cr {
>     uint64_t index;
>     uint64_t old_val, new_val;
> };
> 
> And have a per-arch index of control registers, such as
> 
> X86_CR0
> X86_CR3
> X86_CR4
> X86_XCR0
> ...
> ARM32_$foo

Staging's vm_event.h looks like this now:

 63 /* CR0 was updated */
 64 #define VM_EVENT_REASON_MOV_TO_CR0              4
 65 /* CR3 was updated */
 66 #define VM_EVENT_REASON_MOV_TO_CR3              5
 67 /* CR4 was updated */
 68 #define VM_EVENT_REASON_MOV_TO_CR4              6
 69 /* An MSR was updated. */
 70 #define VM_EVENT_REASON_MOV_TO_MSR              7

Now, I can change VM_EVENT_REASON_MOV_TO_CR0 to
VM_EVENT_REASON_MOV_TO_CR and use that for all CR and XCR events (well,
only XCR0 now).

Should VM_EVENT_REASON_MOV_TO_MSR become 5, or should we keep backward
compatibility with prior clients and leave a gap?


Thanks,
Razvan Cojocaru

Re: [PATCH 1/5] xen/vm_event: Added support for XSETBV events

[Andrew Cooper]

On 08/05/15 10:06, Razvan Cojocaru wrote:
> On 05/07/2015 09:03 PM, Andrew Cooper wrote:
>> In an effort to be architecture neutral, it might be an idea to have
>> something like
>>
>> struct vm_event_write_cr {
>>     uint64_t index;
>>     uint64_t old_val, new_val;
>> };
>>
>> And have a per-arch index of control registers, such as
>>
>> X86_CR0
>> X86_CR3
>> X86_CR4
>> X86_XCR0
>> ...
>> ARM32_$foo
> Staging's vm_event.h looks like this now:
>
>  63 /* CR0 was updated */
>  64 #define VM_EVENT_REASON_MOV_TO_CR0              4
>  65 /* CR3 was updated */
>  66 #define VM_EVENT_REASON_MOV_TO_CR3              5
>  67 /* CR4 was updated */
>  68 #define VM_EVENT_REASON_MOV_TO_CR4              6
>  69 /* An MSR was updated. */
>  70 #define VM_EVENT_REASON_MOV_TO_MSR              7
>
> Now, I can change VM_EVENT_REASON_MOV_TO_CR0 to
> VM_EVENT_REASON_MOV_TO_CR and use that for all CR and XCR events (well,
> only XCR0 now).
>
> Should VM_EVENT_REASON_MOV_TO_MSR become 5, or should we keep backward
> compatibility with prior clients and leave a gap?

For Tamas' cleanup series, we accepted ABI/API incompatible changes to
try and get the interface in order.  Therefore, until 4.6 is released,
anything goes, and I am very much in favour of more changes to clean the
arch-specific bits out of the common bits.

~Andrew

Re: [PATCH 1/5] xen/vm_event: Added support for XSETBV events

[Tamas K Lengyel]

On Fri, May 8, 2015 at 12:55 PM, Andrew Cooper
<andrew.cooper3@citrix.com> wrote:
> On 08/05/15 11:53, Tamas K Lengyel wrote:
>> On Fri, May 8, 2015 at 11:10 AM, Andrew Cooper
>> <andrew.cooper3@citrix.com> wrote:
>>> On 08/05/15 10:06, Razvan Cojocaru wrote:
>>>> On 05/07/2015 09:03 PM, Andrew Cooper wrote:
>>>>> In an effort to be architecture neutral, it might be an idea to have
>>>>> something like
>>>>>
>>>>> struct vm_event_write_cr {
>>>>>     uint64_t index;
>>>>>     uint64_t old_val, new_val;
>>>>> };
>>>>>
>>>>> And have a per-arch index of control registers, such as
>>>>>
>>>>> X86_CR0
>>>>> X86_CR3
>>>>> X86_CR4
>>>>> X86_XCR0
>>>>> ...
>>>>> ARM32_$foo
>> On ARM there are no "cr" registers so IMHO it would be better to
>> rename the struct vm_event_write_register. Other than that this sounds
>> like a good addition to the interface.
>
> But there are surely the concept of "control registers" ?
>
> (I have no knowledge in this area)
>
> ~Andrew

(Re-adding xen-devel)

Certainly, they are just not (necessarily) called "CR". For example,
CR3 equivalent on ARM is TTBR1. So what I meant here is that naming
the struct should not be x86 specific.

Tamas

Re: [PATCH 1/5] xen/vm_event: Added support for XSETBV events

[Jan Beulich]

>>> On 08.05.15 at 13:05, <tamas.lengyel@zentific.com> wrote:
> On Fri, May 8, 2015 at 12:55 PM, Andrew Cooper
> <andrew.cooper3@citrix.com> wrote:
>> On 08/05/15 11:53, Tamas K Lengyel wrote:
>>> On Fri, May 8, 2015 at 11:10 AM, Andrew Cooper
>>> <andrew.cooper3@citrix.com> wrote:
>>>> On 08/05/15 10:06, Razvan Cojocaru wrote:
>>>>> On 05/07/2015 09:03 PM, Andrew Cooper wrote:
>>>>>> In an effort to be architecture neutral, it might be an idea to have
>>>>>> something like
>>>>>>
>>>>>> struct vm_event_write_cr {
>>>>>>     uint64_t index;
>>>>>>     uint64_t old_val, new_val;
>>>>>> };
>>>>>>
>>>>>> And have a per-arch index of control registers, such as
>>>>>>
>>>>>> X86_CR0
>>>>>> X86_CR3
>>>>>> X86_CR4
>>>>>> X86_XCR0
>>>>>> ...
>>>>>> ARM32_$foo
>>> On ARM there are no "cr" registers so IMHO it would be better to
>>> rename the struct vm_event_write_register. Other than that this sounds
>>> like a good addition to the interface.
>>
>> But there are surely the concept of "control registers" ?
>>
>> (I have no knowledge in this area)
>>
>> ~Andrew
> 
> (Re-adding xen-devel)
> 
> Certainly, they are just not (necessarily) called "CR". For example,
> CR3 equivalent on ARM is TTBR1. So what I meant here is that naming
> the struct should not be x86 specific.

In which case - vm_event_write_ctrlreg?

Jan

Re: [PATCH 1/5] xen/vm_event: Added support for XSETBV events

[Razvan Cojocaru]

On 05/08/2015 02:52 PM, Jan Beulich wrote:
>>>> On 08.05.15 at 13:05, <tamas.lengyel@zentific.com> wrote:
>> On Fri, May 8, 2015 at 12:55 PM, Andrew Cooper
>> <andrew.cooper3@citrix.com> wrote:
>>> On 08/05/15 11:53, Tamas K Lengyel wrote:
>>>> On Fri, May 8, 2015 at 11:10 AM, Andrew Cooper
>>>> <andrew.cooper3@citrix.com> wrote:
>>>>> On 08/05/15 10:06, Razvan Cojocaru wrote:
>>>>>> On 05/07/2015 09:03 PM, Andrew Cooper wrote:
>>>>>>> In an effort to be architecture neutral, it might be an idea to have
>>>>>>> something like
>>>>>>>
>>>>>>> struct vm_event_write_cr {
>>>>>>>     uint64_t index;
>>>>>>>     uint64_t old_val, new_val;
>>>>>>> };
>>>>>>>
>>>>>>> And have a per-arch index of control registers, such as
>>>>>>>
>>>>>>> X86_CR0
>>>>>>> X86_CR3
>>>>>>> X86_CR4
>>>>>>> X86_XCR0
>>>>>>> ...
>>>>>>> ARM32_$foo
>>>> On ARM there are no "cr" registers so IMHO it would be better to
>>>> rename the struct vm_event_write_register. Other than that this sounds
>>>> like a good addition to the interface.
>>>
>>> But there are surely the concept of "control registers" ?
>>>
>>> (I have no knowledge in this area)
>>>
>>> ~Andrew
>>
>> (Re-adding xen-devel)
>>
>> Certainly, they are just not (necessarily) called "CR". For example,
>> CR3 equivalent on ARM is TTBR1. So what I meant here is that naming
>> the struct should not be x86 specific.
> 
> In which case - vm_event_write_ctrlreg?

Looks good. Of course, the underlying footwork will need to stay just as
complicated - sync / enabled flags for each supported register, but the
interface will be cleaner and there will be less repetition for
xc_monitor_*() and hvm_event_*().


Thanks,
Razvan

Re: [PATCH 1/5] xen/vm_event: Added support for XSETBV events

[Tamas K Lengyel]

On Fri, May 8, 2015 at 2:09 PM, Razvan Cojocaru
<rcojocaru@bitdefender.com> wrote:
> On 05/08/2015 02:52 PM, Jan Beulich wrote:
>>>>> On 08.05.15 at 13:05, <tamas.lengyel@zentific.com> wrote:
>>> On Fri, May 8, 2015 at 12:55 PM, Andrew Cooper
>>> <andrew.cooper3@citrix.com> wrote:
>>>> On 08/05/15 11:53, Tamas K Lengyel wrote:
>>>>> On Fri, May 8, 2015 at 11:10 AM, Andrew Cooper
>>>>> <andrew.cooper3@citrix.com> wrote:
>>>>>> On 08/05/15 10:06, Razvan Cojocaru wrote:
>>>>>>> On 05/07/2015 09:03 PM, Andrew Cooper wrote:
>>>>>>>> In an effort to be architecture neutral, it might be an idea to have
>>>>>>>> something like
>>>>>>>>
>>>>>>>> struct vm_event_write_cr {
>>>>>>>>     uint64_t index;
>>>>>>>>     uint64_t old_val, new_val;
>>>>>>>> };
>>>>>>>>
>>>>>>>> And have a per-arch index of control registers, such as
>>>>>>>>
>>>>>>>> X86_CR0
>>>>>>>> X86_CR3
>>>>>>>> X86_CR4
>>>>>>>> X86_XCR0
>>>>>>>> ...
>>>>>>>> ARM32_$foo
>>>>> On ARM there are no "cr" registers so IMHO it would be better to
>>>>> rename the struct vm_event_write_register. Other than that this sounds
>>>>> like a good addition to the interface.
>>>>
>>>> But there are surely the concept of "control registers" ?
>>>>
>>>> (I have no knowledge in this area)
>>>>
>>>> ~Andrew
>>>
>>> (Re-adding xen-devel)
>>>
>>> Certainly, they are just not (necessarily) called "CR". For example,
>>> CR3 equivalent on ARM is TTBR1. So what I meant here is that naming
>>> the struct should not be x86 specific.
>>
>> In which case - vm_event_write_ctrlreg?
>
> Looks good. Of course, the underlying footwork will need to stay just as
> complicated - sync / enabled flags for each supported register, but the
> interface will be cleaner and there will be less repetition for
> xc_monitor_*() and hvm_event_*().
>
>
> Thanks,
> Razvan

Sounds good to me too.

Tamas

Re: [PATCH 1/5] xen/vm_event: Added support for XSETBV events

[Jan Beulich]

>>> On 06.05.15 at 19:12, <rcojocaru@bitdefender.com> wrote:
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -2966,6 +2966,8 @@ int hvm_handle_xsetbv(u32 index, u64 new_bv)
>  {
>      struct segment_register sreg;
>  
> +    hvm_event_xsetbv(index, new_bv);
> +
>      hvm_get_segment_register(current, x86_seg_ss, &sreg);
>      if ( sreg.attr.fields.dpl != 0 )
>          goto err;

I don't think the event should be sent before having done at least
the CPL check.

Jan

Re: [PATCH 1/5] xen/vm_event: Added support for XSETBV events

[Razvan Cojocaru]

On 05/08/2015 03:21 PM, Jan Beulich wrote:
>>>> On 06.05.15 at 19:12, <rcojocaru@bitdefender.com> wrote:
>> --- a/xen/arch/x86/hvm/hvm.c
>> +++ b/xen/arch/x86/hvm/hvm.c
>> @@ -2966,6 +2966,8 @@ int hvm_handle_xsetbv(u32 index, u64 new_bv)
>>  {
>>      struct segment_register sreg;
>>  
>> +    hvm_event_xsetbv(index, new_bv);
>> +
>>      hvm_get_segment_register(current, x86_seg_ss, &sreg);
>>      if ( sreg.attr.fields.dpl != 0 )
>>          goto err;
> 
> I don't think the event should be sent before having done at least
> the CPL check.

Ack.


Thanks,
Razvan

[PATCH 2/5] xen/vm_access: Support for memory-content hiding

[Razvan Cojocaru]

This patch adds support for memory-content hiding, by modifying the
value returned by emulated instructions that read certain memory
addresses that contain sensitive data. The patch only applies to
cases where MEM_ACCESS_EMULATE or MEM_ACCESS_EMULATE_NOWRITE have
been set to a vm_event response.

Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
---
 xen/arch/x86/hvm/emulate.c        |  155 ++++++++++++++++++++++++++++++++-----
 xen/arch/x86/mm/p2m.c             |   15 +++-
 xen/include/asm-x86/domain.h      |    2 +
 xen/include/asm-x86/hvm/emulate.h |   10 ++-
 xen/include/public/vm_event.h     |   20 ++++-
 5 files changed, 178 insertions(+), 24 deletions(-)

diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
index ac9c9d6..0058b15 100644
--- a/xen/arch/x86/hvm/emulate.c
+++ b/xen/arch/x86/hvm/emulate.c
@@ -578,6 +578,25 @@ static int hvmemul_read(
         container_of(ctxt, struct hvm_emulate_ctxt, ctxt));
 }
 
+static int hvmemul_read_set_context(
+    enum x86_segment seg,
+    unsigned long offset,
+    void *p_data,
+    unsigned int bytes,
+    struct x86_emulate_ctxt *ctxt)
+{
+    struct vcpu *curr = current;
+    unsigned int len =
+        (bytes > curr->arch.vm_event.emul_read_data.size ?
+            curr->arch.vm_event.emul_read_data.size : bytes);
+
+    if ( len )
+        memcpy(p_data, curr->arch.vm_event.emul_read_data.data,
+            curr->arch.vm_event.emul_read_data.size);
+
+    return X86EMUL_OKAY;
+}
+
 static int hvmemul_insn_fetch(
     enum x86_segment seg,
     unsigned long offset,
@@ -891,14 +910,15 @@ static int hvmemul_rep_outs(
                           !!(ctxt->regs->eflags & X86_EFLAGS_DF), NULL);
 }
 
-static int hvmemul_rep_movs(
+static int _hvmemul_rep_movs(
    enum x86_segment src_seg,
    unsigned long src_offset,
    enum x86_segment dst_seg,
    unsigned long dst_offset,
    unsigned int bytes_per_rep,
    unsigned long *reps,
-   struct x86_emulate_ctxt *ctxt)
+   struct x86_emulate_ctxt *ctxt,
+   bool_t set_context)
 {
     struct hvm_emulate_ctxt *hvmemul_ctxt =
         container_of(ctxt, struct hvm_emulate_ctxt, ctxt);
@@ -970,20 +990,38 @@ static int hvmemul_rep_movs(
     if ( df )
         dgpa -= bytes - bytes_per_rep;
 
-    /* Allocate temporary buffer. Fall back to slow emulation if this fails. */
-    buf = xmalloc_bytes(bytes);
-    if ( buf == NULL )
-        return X86EMUL_UNHANDLEABLE;
+    if ( unlikely(set_context) )
+    {
+        struct vcpu* curr = current;
 
-    /*
-     * We do a modicum of checking here, just for paranoia's sake and to
-     * definitely avoid copying an unitialised buffer into guest address space.
-     */
-    rc = hvm_copy_from_guest_phys(buf, sgpa, bytes);
-    if ( rc == HVMCOPY_okay )
-        rc = hvm_copy_to_guest_phys(dgpa, buf, bytes);
+        bytes = bytes < curr->arch.vm_event.emul_read_data.size ?
+            bytes : curr->arch.vm_event.emul_read_data.size;
 
-    xfree(buf);
+        rc = hvm_copy_to_guest_phys(dgpa,
+                                    curr->arch.vm_event.emul_read_data.data,
+                                    bytes);
+    }
+    else
+    {
+        /*
+         * Allocate temporary buffer. Fall back to slow emulation if this
+         * fails.
+         */
+        buf = xmalloc_bytes(bytes);
+        if ( buf == NULL )
+            return X86EMUL_UNHANDLEABLE;
+
+        /*
+         * We do a modicum of checking here, just for paranoia's sake and to
+         * definitely avoid copying an unitialised buffer into guest address
+         * space.
+         */
+        rc = hvm_copy_from_guest_phys(buf, sgpa, bytes);
+        if ( rc == HVMCOPY_okay )
+            rc = hvm_copy_to_guest_phys(dgpa, buf, bytes);
+
+        xfree(buf);
+    }
 
     if ( rc == HVMCOPY_gfn_paged_out )
         return X86EMUL_RETRY;
@@ -1000,6 +1038,32 @@ static int hvmemul_rep_movs(
     return X86EMUL_OKAY;
 }
 
+static int hvmemul_rep_movs(
+   enum x86_segment src_seg,
+   unsigned long src_offset,
+   enum x86_segment dst_seg,
+   unsigned long dst_offset,
+   unsigned int bytes_per_rep,
+   unsigned long *reps,
+   struct x86_emulate_ctxt *ctxt)
+{
+    return _hvmemul_rep_movs(src_seg, src_offset, dst_seg, dst_offset,
+                             bytes_per_rep, reps, ctxt, 0);
+}
+
+static int hvmemul_rep_movs_set_context(
+   enum x86_segment src_seg,
+   unsigned long src_offset,
+   enum x86_segment dst_seg,
+   unsigned long dst_offset,
+   unsigned int bytes_per_rep,
+   unsigned long *reps,
+   struct x86_emulate_ctxt *ctxt)
+{
+    return _hvmemul_rep_movs(src_seg, src_offset, dst_seg, dst_offset,
+                             bytes_per_rep, reps, ctxt, 1);
+}
+
 static int hvmemul_rep_stos(
     void *p_data,
     enum x86_segment seg,
@@ -1107,6 +1171,22 @@ static int hvmemul_rep_stos(
     }
 }
 
+static int hvmemul_rep_stos_set_context(
+    void *p_data,
+    enum x86_segment seg,
+    unsigned long offset,
+    unsigned int bytes_per_rep,
+    unsigned long *reps,
+    struct x86_emulate_ctxt *ctxt)
+{
+    struct vcpu *curr = current;
+    unsigned long local_reps = 1;
+
+    return hvmemul_rep_stos(curr->arch.vm_event.emul_read_data.data, seg,
+                            offset, curr->arch.vm_event.emul_read_data.size,
+                            &local_reps, ctxt);
+}
+
 static int hvmemul_read_segment(
     enum x86_segment seg,
     struct segment_register *reg,
@@ -1408,6 +1488,32 @@ static const struct x86_emulate_ops hvm_emulate_ops_no_write = {
     .invlpg        = hvmemul_invlpg
 };
 
+static const struct x86_emulate_ops hvm_emulate_ops_set_context = {
+    .read          = hvmemul_read_set_context,
+    .insn_fetch    = hvmemul_insn_fetch,
+    .write         = hvmemul_write,
+    .cmpxchg       = hvmemul_cmpxchg,
+    .rep_ins       = hvmemul_rep_ins,
+    .rep_outs      = hvmemul_rep_outs,
+    .rep_movs      = hvmemul_rep_movs_set_context,
+    .rep_stos      = hvmemul_rep_stos_set_context,
+    .read_segment  = hvmemul_read_segment,
+    .write_segment = hvmemul_write_segment,
+    .read_io       = hvmemul_read_io,
+    .write_io      = hvmemul_write_io,
+    .read_cr       = hvmemul_read_cr,
+    .write_cr      = hvmemul_write_cr,
+    .read_msr      = hvmemul_read_msr,
+    .write_msr     = hvmemul_write_msr,
+    .wbinvd        = hvmemul_wbinvd,
+    .cpuid         = hvmemul_cpuid,
+    .inject_hw_exception = hvmemul_inject_hw_exception,
+    .inject_sw_interrupt = hvmemul_inject_sw_interrupt,
+    .get_fpu       = hvmemul_get_fpu,
+    .put_fpu       = hvmemul_put_fpu,
+    .invlpg        = hvmemul_invlpg
+};
+
 static int _hvm_emulate_one(struct hvm_emulate_ctxt *hvmemul_ctxt,
     const struct x86_emulate_ops *ops)
 {
@@ -1528,18 +1634,31 @@ int hvm_emulate_one_no_write(
     return _hvm_emulate_one(hvmemul_ctxt, &hvm_emulate_ops_no_write);
 }
 
-void hvm_mem_access_emulate_one(bool_t nowrite, unsigned int trapnr,
+int hvm_emulate_one_set_context(
+    struct hvm_emulate_ctxt *hvmemul_ctxt)
+{
+    return _hvm_emulate_one(hvmemul_ctxt, &hvm_emulate_ops_set_context);
+}
+
+void hvm_mem_access_emulate_one(enum emul_kind kind, unsigned int trapnr,
     unsigned int errcode)
 {
     struct hvm_emulate_ctxt ctx = {{ 0 }};
-    int rc;
+    int rc = X86EMUL_UNHANDLEABLE;
 
     hvm_emulate_prepare(&ctx, guest_cpu_user_regs());
 
-    if ( nowrite )
+    switch ( kind ) {
+    case EMUL_KIND_NOWRITE:
         rc = hvm_emulate_one_no_write(&ctx);
-    else
+        break;
+    case EMUL_KIND_NORMAL:
         rc = hvm_emulate_one(&ctx);
+        break;
+    case EMUL_KIND_SET_CONTEXT:
+        rc = hvm_emulate_one_set_context(&ctx);
+        break;
+    }
 
     switch ( rc )
     {
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index 1fd1194..0b2f9a6 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -1466,6 +1466,9 @@ void p2m_mem_access_emulate_check(struct vcpu *v,
         }
 
         v->arch.vm_event.emulate_flags = violation ? rsp->flags : 0;
+
+        if ( rsp->flags & MEM_ACCESS_SET_EMUL_READ_DATA )
+            v->arch.vm_event.emul_read_data = rsp->emul_read_data;
     }
 }
 
@@ -1552,9 +1555,15 @@ bool_t p2m_mem_access_check(paddr_t gpa, unsigned long gla,
 
     if ( v->arch.vm_event.emulate_flags )
     {
-        hvm_mem_access_emulate_one((v->arch.vm_event.emulate_flags &
-                                    MEM_ACCESS_EMULATE_NOWRITE) != 0,
-                                   TRAP_invalid_op, HVM_DELIVER_NO_ERROR_CODE);
+        enum emul_kind kind = EMUL_KIND_NORMAL;
+
+        if ( v->arch.vm_event.emulate_flags & MEM_ACCESS_SET_EMUL_READ_DATA )
+            kind = EMUL_KIND_SET_CONTEXT;
+        else if ( v->arch.vm_event.emulate_flags & MEM_ACCESS_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;
         return 1;
diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
index 452a9b3..2b89182 100644
--- a/xen/include/asm-x86/domain.h
+++ b/xen/include/asm-x86/domain.h
@@ -10,6 +10,7 @@
 #include <asm/mce.h>
 #include <public/vcpu.h>
 #include <public/hvm/hvm_info_table.h>
+#include <public/vm_event.h>
 
 #define has_32bit_shinfo(d)    ((d)->arch.has_32bit_shinfo)
 #define is_pv_32bit_domain(d)  ((d)->arch.is_32bit_pv)
@@ -512,6 +513,7 @@ struct arch_vcpu
         uint32_t emulate_flags;
         unsigned long gpa;
         unsigned long eip;
+        struct vm_event_emul_read_data emul_read_data;
     } vm_event;
 
 };
diff --git a/xen/include/asm-x86/hvm/emulate.h b/xen/include/asm-x86/hvm/emulate.h
index b3971c8..65ccfd8 100644
--- a/xen/include/asm-x86/hvm/emulate.h
+++ b/xen/include/asm-x86/hvm/emulate.h
@@ -34,11 +34,19 @@ struct hvm_emulate_ctxt {
     uint32_t intr_shadow;
 };
 
+enum emul_kind {
+    EMUL_KIND_NORMAL,
+    EMUL_KIND_NOWRITE,
+    EMUL_KIND_SET_CONTEXT
+};
+
 int hvm_emulate_one(
     struct hvm_emulate_ctxt *hvmemul_ctxt);
 int hvm_emulate_one_no_write(
     struct hvm_emulate_ctxt *hvmemul_ctxt);
-void hvm_mem_access_emulate_one(bool_t nowrite,
+int hvm_emulate_one_set_context(
+    struct hvm_emulate_ctxt *hvmemul_ctxt);
+void hvm_mem_access_emulate_one(enum emul_kind kind,
     unsigned int trapnr,
     unsigned int errcode);
 void hvm_emulate_prepare(
diff --git a/xen/include/public/vm_event.h b/xen/include/public/vm_event.h
index 71fe9ba..bce3e3e 100644
--- a/xen/include/public/vm_event.h
+++ b/xen/include/public/vm_event.h
@@ -149,6 +149,13 @@ struct vm_event_regs_x86 {
  * potentially having side effects (like memory mapped or port I/O) disabled.
  */
 #define MEM_ACCESS_EMULATE_NOWRITE      (1 << 7)
+/*
+ * Data is being sent back to the hypervisor in the event response, to be
+ * returned by the read function when emulating an instruction.
+ * This flag is only useful when combined with MEM_ACCESS_EMULATE or
+ * MEM_ACCESS_EMULATE_NOWRITE.
+ */
+#define MEM_ACCESS_SET_EMUL_READ_DATA   (1 << 8)
 
 struct vm_event_mem_access {
     uint64_t gfn;
@@ -193,6 +200,11 @@ struct vm_event_xsetbv {
     uint64_t value;
 };
 
+struct vm_event_emul_read_data {
+    uint32_t size;
+    uint8_t  data[164];
+};
+
 typedef struct vm_event_st {
     uint32_t version;   /* VM_EVENT_INTERFACE_VERSION */
     uint32_t flags;     /* VM_EVENT_FLAG_* */
@@ -211,8 +223,12 @@ typedef struct vm_event_st {
     } u;
 
     union {
-        struct vm_event_regs_x86 x86;
-    } regs;
+        union {
+            struct vm_event_regs_x86 x86;
+        } regs;
+
+        struct vm_event_emul_read_data emul_read_data;
+    };
 } vm_event_request_t, vm_event_response_t;
 
 DEFINE_RING_TYPES(vm_event, vm_event_request_t, vm_event_response_t);
-- 
1.7.9.5

Re: [PATCH 2/5] xen/vm_access: Support for memory-content hiding

[Jan Beulich]

>>> On 06.05.15 at 19:12, <rcojocaru@bitdefender.com> wrote:
> --- a/xen/arch/x86/hvm/emulate.c
> +++ b/xen/arch/x86/hvm/emulate.c
> @@ -578,6 +578,25 @@ static int hvmemul_read(
>          container_of(ctxt, struct hvm_emulate_ctxt, ctxt));
>  }
>  
> +static int hvmemul_read_set_context(
> +    enum x86_segment seg,
> +    unsigned long offset,
> +    void *p_data,
> +    unsigned int bytes,
> +    struct x86_emulate_ctxt *ctxt)
> +{
> +    struct vcpu *curr = current;
> +    unsigned int len =
> +        (bytes > curr->arch.vm_event.emul_read_data.size ?
> +            curr->arch.vm_event.emul_read_data.size : bytes);

min()

> +    if ( len )
> +        memcpy(p_data, curr->arch.vm_event.emul_read_data.data,
> +            curr->arch.vm_event.emul_read_data.size);

Not len?

And what happens to the portion of the read beyond "bytes" (in
case that got reduced above)?

> @@ -970,20 +990,38 @@ static int hvmemul_rep_movs(
>      if ( df )
>          dgpa -= bytes - bytes_per_rep;
>  
> -    /* Allocate temporary buffer. Fall back to slow emulation if this fails. */
> -    buf = xmalloc_bytes(bytes);
> -    if ( buf == NULL )
> -        return X86EMUL_UNHANDLEABLE;
> +    if ( unlikely(set_context) )
> +    {
> +        struct vcpu* curr = current;

* and blank need to switch places.

> -    /*
> -     * We do a modicum of checking here, just for paranoia's sake and to
> -     * definitely avoid copying an unitialised buffer into guest address space.
> -     */
> -    rc = hvm_copy_from_guest_phys(buf, sgpa, bytes);
> -    if ( rc == HVMCOPY_okay )
> -        rc = hvm_copy_to_guest_phys(dgpa, buf, bytes);
> +        bytes = bytes < curr->arch.vm_event.emul_read_data.size ?
> +            bytes : curr->arch.vm_event.emul_read_data.size;

min() again.

> -    xfree(buf);
> +        rc = hvm_copy_to_guest_phys(dgpa,
> +                                    curr->arch.vm_event.emul_read_data.data,
> +                                    bytes);

Again - what happens to the portion of the MOVS beyond "bytes" (in
case that got reduced above)? I think you need to carefully update
*reps (without losing any bytes), or otherwise make this fall through
into what is the "else" branch below.

> @@ -1000,6 +1038,32 @@ static int hvmemul_rep_movs(
>      return X86EMUL_OKAY;
>  }
>  
> +static int hvmemul_rep_movs(
> +   enum x86_segment src_seg,
> +   unsigned long src_offset,
> +   enum x86_segment dst_seg,
> +   unsigned long dst_offset,
> +   unsigned int bytes_per_rep,
> +   unsigned long *reps,
> +   struct x86_emulate_ctxt *ctxt)
> +{
> +    return _hvmemul_rep_movs(src_seg, src_offset, dst_seg, dst_offset,
> +                             bytes_per_rep, reps, ctxt, 0);
> +}
> +
> +static int hvmemul_rep_movs_set_context(
> +   enum x86_segment src_seg,
> +   unsigned long src_offset,
> +   enum x86_segment dst_seg,
> +   unsigned long dst_offset,
> +   unsigned int bytes_per_rep,
> +   unsigned long *reps,
> +   struct x86_emulate_ctxt *ctxt)
> +{
> +    return _hvmemul_rep_movs(src_seg, src_offset, dst_seg, dst_offset,
> +                             bytes_per_rep, reps, ctxt, 1);
> +}

To be honest, these kinds of wrappers for very special, niche
purposes worry me.

> @@ -1107,6 +1171,22 @@ static int hvmemul_rep_stos(
>      }
>  }
>  
> +static int hvmemul_rep_stos_set_context(
> +    void *p_data,
> +    enum x86_segment seg,
> +    unsigned long offset,
> +    unsigned int bytes_per_rep,
> +    unsigned long *reps,
> +    struct x86_emulate_ctxt *ctxt)
> +{
> +    struct vcpu *curr = current;
> +    unsigned long local_reps = 1;
> +
> +    return hvmemul_rep_stos(curr->arch.vm_event.emul_read_data.data, seg,
> +                            offset, curr->arch.vm_event.emul_read_data.size,
> +                            &local_reps, ctxt);
> +}

How come here you can get away by simply reducing *reps to 1? And
considering this patch is about supplying read values - why is fiddling
with STOS emulation necessary in the first place?

> +static const struct x86_emulate_ops hvm_emulate_ops_set_context = {
> +    .read          = hvmemul_read_set_context,
> +    .insn_fetch    = hvmemul_insn_fetch,
> +    .write         = hvmemul_write,
> +    .cmpxchg       = hvmemul_cmpxchg,

How about this one?

> +    .rep_ins       = hvmemul_rep_ins,
> +    .rep_outs      = hvmemul_rep_outs,

And this?

> +    .rep_movs      = hvmemul_rep_movs_set_context,
> +    .rep_stos      = hvmemul_rep_stos_set_context,
> +    .read_segment  = hvmemul_read_segment,
> +    .write_segment = hvmemul_write_segment,
> +    .read_io       = hvmemul_read_io,

And this?

> +void hvm_mem_access_emulate_one(enum emul_kind kind, unsigned int trapnr,
>      unsigned int errcode)
>  {
>      struct hvm_emulate_ctxt ctx = {{ 0 }};
> -    int rc;
> +    int rc = X86EMUL_UNHANDLEABLE;
>  
>      hvm_emulate_prepare(&ctx, guest_cpu_user_regs());
>  
> -    if ( nowrite )
> +    switch ( kind ) {
> +    case EMUL_KIND_NOWRITE:
>          rc = hvm_emulate_one_no_write(&ctx);
> -    else
> +        break;
> +    case EMUL_KIND_NORMAL:
>          rc = hvm_emulate_one(&ctx);

Mind having "NORMAL" as the first case in the switch()?

> --- a/xen/include/asm-x86/domain.h
> +++ b/xen/include/asm-x86/domain.h
> @@ -512,6 +513,7 @@ struct arch_vcpu
>          uint32_t emulate_flags;
>          unsigned long gpa;
>          unsigned long eip;
> +        struct vm_event_emul_read_data emul_read_data;

Considering the size of this structure I don't think this should be
there for all vCPU-s of all guests. IOW - please allocate this
dynamically only on domains where it's needed.

> @@ -193,6 +200,11 @@ struct vm_event_xsetbv {
>      uint64_t value;
>  };
>  
> +struct vm_event_emul_read_data {
> +    uint32_t size;
> +    uint8_t  data[164];

This number needs an explanation.

Jan

Re: [PATCH 2/5] xen/vm_access: Support for memory-content hiding

[Razvan Cojocaru]

On 05/08/2015 07:07 PM, Jan Beulich wrote:
>>>> On 06.05.15 at 19:12, <rcojocaru@bitdefender.com> wrote:
>> --- a/xen/arch/x86/hvm/emulate.c
>> +++ b/xen/arch/x86/hvm/emulate.c
>> @@ -578,6 +578,25 @@ static int hvmemul_read(
>>          container_of(ctxt, struct hvm_emulate_ctxt, ctxt));
>>  }
>>  
>> +static int hvmemul_read_set_context(
>> +    enum x86_segment seg,
>> +    unsigned long offset,
>> +    void *p_data,
>> +    unsigned int bytes,
>> +    struct x86_emulate_ctxt *ctxt)
>> +{
>> +    struct vcpu *curr = current;
>> +    unsigned int len =
>> +        (bytes > curr->arch.vm_event.emul_read_data.size ?
>> +            curr->arch.vm_event.emul_read_data.size : bytes);
> 
> min()

Ack.

>> +    if ( len )
>> +        memcpy(p_data, curr->arch.vm_event.emul_read_data.data,
>> +            curr->arch.vm_event.emul_read_data.size);
> 
> Not len?
> 
> And what happens to the portion of the read beyond "bytes" (in
> case that got reduced above)?

Yes, it should be len. I've been porting these patches from an older
version of Xen and somehow lost that in translation. I'll correct it in V2.

>> @@ -970,20 +990,38 @@ static int hvmemul_rep_movs(
>>      if ( df )
>>          dgpa -= bytes - bytes_per_rep;
>>  
>> -    /* Allocate temporary buffer. Fall back to slow emulation if this fails. */
>> -    buf = xmalloc_bytes(bytes);
>> -    if ( buf == NULL )
>> -        return X86EMUL_UNHANDLEABLE;
>> +    if ( unlikely(set_context) )
>> +    {
>> +        struct vcpu* curr = current;
> 
> * and blank need to switch places.

Ack.

>> -    /*
>> -     * We do a modicum of checking here, just for paranoia's sake and to
>> -     * definitely avoid copying an unitialised buffer into guest address space.
>> -     */
>> -    rc = hvm_copy_from_guest_phys(buf, sgpa, bytes);
>> -    if ( rc == HVMCOPY_okay )
>> -        rc = hvm_copy_to_guest_phys(dgpa, buf, bytes);
>> +        bytes = bytes < curr->arch.vm_event.emul_read_data.size ?
>> +            bytes : curr->arch.vm_event.emul_read_data.size;
> 
> min() again.

Ack.

>> @@ -1107,6 +1171,22 @@ static int hvmemul_rep_stos(
>>      }
>>  }
>>  
>> +static int hvmemul_rep_stos_set_context(
>> +    void *p_data,
>> +    enum x86_segment seg,
>> +    unsigned long offset,
>> +    unsigned int bytes_per_rep,
>> +    unsigned long *reps,
>> +    struct x86_emulate_ctxt *ctxt)
>> +{
>> +    struct vcpu *curr = current;
>> +    unsigned long local_reps = 1;
>> +
>> +    return hvmemul_rep_stos(curr->arch.vm_event.emul_read_data.data, seg,
>> +                            offset, curr->arch.vm_event.emul_read_data.size,
>> +                            &local_reps, ctxt);
>> +}
> 
> How come here you can get away by simply reducing *reps to 1? And
> considering this patch is about supplying read values - why is fiddling
> with STOS emulation necessary in the first place?

I got away with it because *reps is always one in our test scenario -
emulating instructions with the REP part disabled, as done here:

http://lists.xen.org/archives/html/xen-devel/2014-11/msg00243.html

But it's indeed not reasonable to assume that that is the case with all
users, so I'll have to rethink that.

>> +static const struct x86_emulate_ops hvm_emulate_ops_set_context = {
>> +    .read          = hvmemul_read_set_context,
>> +    .insn_fetch    = hvmemul_insn_fetch,
>> +    .write         = hvmemul_write,
>> +    .cmpxchg       = hvmemul_cmpxchg,
> 
> How about this one?
> 
>> +    .rep_ins       = hvmemul_rep_ins,
>> +    .rep_outs      = hvmemul_rep_outs,
> 
> And this?
> 
>> +    .rep_movs      = hvmemul_rep_movs_set_context,
>> +    .rep_stos      = hvmemul_rep_stos_set_context,
>> +    .read_segment  = hvmemul_read_segment,
>> +    .write_segment = hvmemul_write_segment,
>> +    .read_io       = hvmemul_read_io,
> 
> And this?

Yes, those require attention too. I thought those would use the read
function supplied, but I see now that I've been mislead by a comment
about __hvmemul_read() in hvmemul_cmpxchg().

>> +void hvm_mem_access_emulate_one(enum emul_kind kind, unsigned int trapnr,
>>      unsigned int errcode)
>>  {
>>      struct hvm_emulate_ctxt ctx = {{ 0 }};
>> -    int rc;
>> +    int rc = X86EMUL_UNHANDLEABLE;
>>  
>>      hvm_emulate_prepare(&ctx, guest_cpu_user_regs());
>>  
>> -    if ( nowrite )
>> +    switch ( kind ) {
>> +    case EMUL_KIND_NOWRITE:
>>          rc = hvm_emulate_one_no_write(&ctx);
>> -    else
>> +        break;
>> +    case EMUL_KIND_NORMAL:
>>          rc = hvm_emulate_one(&ctx);
> 
> Mind having "NORMAL" as the first case in the switch()?

No problem, I'll change it.

>> --- a/xen/include/asm-x86/domain.h
>> +++ b/xen/include/asm-x86/domain.h
>> @@ -512,6 +513,7 @@ struct arch_vcpu
>>          uint32_t emulate_flags;
>>          unsigned long gpa;
>>          unsigned long eip;
>> +        struct vm_event_emul_read_data emul_read_data;
> 
> Considering the size of this structure I don't think this should be
> there for all vCPU-s of all guests. IOW - please allocate this
> dynamically only on domains where it's needed.

Ack.

>> @@ -193,6 +200,11 @@ struct vm_event_xsetbv {
>>      uint64_t value;
>>  };
>>  
>> +struct vm_event_emul_read_data {
>> +    uint32_t size;
>> +    uint8_t  data[164];
> 
> This number needs an explanation.

It's less than the size of the x86_regs and enough for all the cases
we've tested so far...


Thanks,
Razvan

Re: [PATCH 2/5] xen/vm_access: Support for memory-content hiding

[Tamas K Lengyel]

On Fri, May 8, 2015 at 6:49 PM, Razvan Cojocaru
<rcojocaru@bitdefender.com> wrote:
> On 05/08/2015 07:07 PM, Jan Beulich wrote:
>>>>> On 06.05.15 at 19:12, <rcojocaru@bitdefender.com> wrote:
>>> --- a/xen/arch/x86/hvm/emulate.c
>>> +++ b/xen/arch/x86/hvm/emulate.c
>>> @@ -578,6 +578,25 @@ static int hvmemul_read(
>>>          container_of(ctxt, struct hvm_emulate_ctxt, ctxt));
>>>  }
>>>
>>> +static int hvmemul_read_set_context(
>>> +    enum x86_segment seg,
>>> +    unsigned long offset,
>>> +    void *p_data,
>>> +    unsigned int bytes,
>>> +    struct x86_emulate_ctxt *ctxt)
>>> +{
>>> +    struct vcpu *curr = current;
>>> +    unsigned int len =
>>> +        (bytes > curr->arch.vm_event.emul_read_data.size ?
>>> +            curr->arch.vm_event.emul_read_data.size : bytes);
>>
>> min()
>
> Ack.
>
>>> +    if ( len )
>>> +        memcpy(p_data, curr->arch.vm_event.emul_read_data.data,
>>> +            curr->arch.vm_event.emul_read_data.size);
>>
>> Not len?
>>
>> And what happens to the portion of the read beyond "bytes" (in
>> case that got reduced above)?
>
> Yes, it should be len. I've been porting these patches from an older
> version of Xen and somehow lost that in translation. I'll correct it in V2.
>
>>> @@ -970,20 +990,38 @@ static int hvmemul_rep_movs(
>>>      if ( df )
>>>          dgpa -= bytes - bytes_per_rep;
>>>
>>> -    /* Allocate temporary buffer. Fall back to slow emulation if this fails. */
>>> -    buf = xmalloc_bytes(bytes);
>>> -    if ( buf == NULL )
>>> -        return X86EMUL_UNHANDLEABLE;
>>> +    if ( unlikely(set_context) )
>>> +    {
>>> +        struct vcpu* curr = current;
>>
>> * and blank need to switch places.
>
> Ack.
>
>>> -    /*
>>> -     * We do a modicum of checking here, just for paranoia's sake and to
>>> -     * definitely avoid copying an unitialised buffer into guest address space.
>>> -     */
>>> -    rc = hvm_copy_from_guest_phys(buf, sgpa, bytes);
>>> -    if ( rc == HVMCOPY_okay )
>>> -        rc = hvm_copy_to_guest_phys(dgpa, buf, bytes);
>>> +        bytes = bytes < curr->arch.vm_event.emul_read_data.size ?
>>> +            bytes : curr->arch.vm_event.emul_read_data.size;
>>
>> min() again.
>
> Ack.
>
>>> @@ -1107,6 +1171,22 @@ static int hvmemul_rep_stos(
>>>      }
>>>  }
>>>
>>> +static int hvmemul_rep_stos_set_context(
>>> +    void *p_data,
>>> +    enum x86_segment seg,
>>> +    unsigned long offset,
>>> +    unsigned int bytes_per_rep,
>>> +    unsigned long *reps,
>>> +    struct x86_emulate_ctxt *ctxt)
>>> +{
>>> +    struct vcpu *curr = current;
>>> +    unsigned long local_reps = 1;
>>> +
>>> +    return hvmemul_rep_stos(curr->arch.vm_event.emul_read_data.data, seg,
>>> +                            offset, curr->arch.vm_event.emul_read_data.size,
>>> +                            &local_reps, ctxt);
>>> +}
>>
>> How come here you can get away by simply reducing *reps to 1? And
>> considering this patch is about supplying read values - why is fiddling
>> with STOS emulation necessary in the first place?
>
> I got away with it because *reps is always one in our test scenario -
> emulating instructions with the REP part disabled, as done here:
>
> http://lists.xen.org/archives/html/xen-devel/2014-11/msg00243.html
>
> But it's indeed not reasonable to assume that that is the case with all
> users, so I'll have to rethink that.
>
>>> +static const struct x86_emulate_ops hvm_emulate_ops_set_context = {
>>> +    .read          = hvmemul_read_set_context,
>>> +    .insn_fetch    = hvmemul_insn_fetch,
>>> +    .write         = hvmemul_write,
>>> +    .cmpxchg       = hvmemul_cmpxchg,
>>
>> How about this one?
>>
>>> +    .rep_ins       = hvmemul_rep_ins,
>>> +    .rep_outs      = hvmemul_rep_outs,
>>
>> And this?
>>
>>> +    .rep_movs      = hvmemul_rep_movs_set_context,
>>> +    .rep_stos      = hvmemul_rep_stos_set_context,
>>> +    .read_segment  = hvmemul_read_segment,
>>> +    .write_segment = hvmemul_write_segment,
>>> +    .read_io       = hvmemul_read_io,
>>
>> And this?
>
> Yes, those require attention too. I thought those would use the read
> function supplied, but I see now that I've been mislead by a comment
> about __hvmemul_read() in hvmemul_cmpxchg().
>
>>> +void hvm_mem_access_emulate_one(enum emul_kind kind, unsigned int trapnr,
>>>      unsigned int errcode)
>>>  {
>>>      struct hvm_emulate_ctxt ctx = {{ 0 }};
>>> -    int rc;
>>> +    int rc = X86EMUL_UNHANDLEABLE;
>>>
>>>      hvm_emulate_prepare(&ctx, guest_cpu_user_regs());
>>>
>>> -    if ( nowrite )
>>> +    switch ( kind ) {
>>> +    case EMUL_KIND_NOWRITE:
>>>          rc = hvm_emulate_one_no_write(&ctx);
>>> -    else
>>> +        break;
>>> +    case EMUL_KIND_NORMAL:
>>>          rc = hvm_emulate_one(&ctx);
>>
>> Mind having "NORMAL" as the first case in the switch()?
>
> No problem, I'll change it.
>
>>> --- a/xen/include/asm-x86/domain.h
>>> +++ b/xen/include/asm-x86/domain.h
>>> @@ -512,6 +513,7 @@ struct arch_vcpu
>>>          uint32_t emulate_flags;
>>>          unsigned long gpa;
>>>          unsigned long eip;
>>> +        struct vm_event_emul_read_data emul_read_data;
>>
>> Considering the size of this structure I don't think this should be
>> there for all vCPU-s of all guests. IOW - please allocate this
>> dynamically only on domains where it's needed.
>
> Ack.
>
>>> @@ -193,6 +200,11 @@ struct vm_event_xsetbv {
>>>      uint64_t value;
>>>  };
>>>
>>> +struct vm_event_emul_read_data {
>>> +    uint32_t size;
>>> +    uint8_t  data[164];
>>
>> This number needs an explanation.
>
> It's less than the size of the x86_regs and enough for all the cases
> we've tested so far...
>
>
> Thanks,
> Razvan

I feel like 164 bytes is really wasteful for all vm_events considering
this would be useful only in a very specific subset of cases. Not sure
what would be the right way to get around that.. Maybe having another
hypercall (potentionally under memop?) place that buffer somewhere
where Xen can access it before the vm_event response is processed?
That would require two hypercalls to be issued by the monitoring
domain, one to place the buffer and one for the event channel
notification being sent to Xen to that the response is placed on the
ring, but it might save space on the ring buffer for all other
cases/users.

Tamas

Re: [PATCH 2/5] xen/vm_access: Support for memory-content hiding

[Razvan Cojocaru]

On 05/09/2015 02:34 AM, Tamas K Lengyel wrote:
>>>> @@ -193,6 +200,11 @@ struct vm_event_xsetbv {
>>>> >>>      uint64_t value;
>>>> >>>  };
>>>> >>>
>>>> >>> +struct vm_event_emul_read_data {
>>>> >>> +    uint32_t size;
>>>> >>> +    uint8_t  data[164];
>>> >>
>>> >> This number needs an explanation.
>> >
>> > It's less than the size of the x86_regs and enough for all the cases
>> > we've tested so far...
>> >
>> >
>> > Thanks,
>> > Razvan
> I feel like 164 bytes is really wasteful for all vm_events considering
> this would be useful only in a very specific subset of cases. Not sure
> what would be the right way to get around that.. Maybe having another
> hypercall (potentionally under memop?) place that buffer somewhere
> where Xen can access it before the vm_event response is processed?
> That would require two hypercalls to be issued by the monitoring
> domain, one to place the buffer and one for the event channel
> notification being sent to Xen to that the response is placed on the
> ring, but it might save space on the ring buffer for all other
> cases/users.

How is it wasteful? Those bytes are in a union with the x86 registers
that are already in each vm_event request and response, and the size of
that buffer is smaller than the size of the x86 registers struct.

The buffer is simply ignored (with no effect whatsoever - on the size of
the data involved or otherwise), except for the cases where it's needed,
when the client application fills it up and sets the flag in the response.

Am I missing something?


Thanks,
Razvan

Re: [PATCH 2/5] xen/vm_access: Support for memory-content hiding

[Tamas K Lengyel]

On Sat, May 9, 2015 at 8:55 AM, Razvan Cojocaru
<rcojocaru@bitdefender.com> wrote:
> On 05/09/2015 02:34 AM, Tamas K Lengyel wrote:
>>>>> @@ -193,6 +200,11 @@ struct vm_event_xsetbv {
>>>>> >>>      uint64_t value;
>>>>> >>>  };
>>>>> >>>
>>>>> >>> +struct vm_event_emul_read_data {
>>>>> >>> +    uint32_t size;
>>>>> >>> +    uint8_t  data[164];
>>>> >>
>>>> >> This number needs an explanation.
>>> >
>>> > It's less than the size of the x86_regs and enough for all the cases
>>> > we've tested so far...
>>> >
>>> >
>>> > Thanks,
>>> > Razvan
>> I feel like 164 bytes is really wasteful for all vm_events considering
>> this would be useful only in a very specific subset of cases. Not sure
>> what would be the right way to get around that.. Maybe having another
>> hypercall (potentionally under memop?) place that buffer somewhere
>> where Xen can access it before the vm_event response is processed?
>> That would require two hypercalls to be issued by the monitoring
>> domain, one to place the buffer and one for the event channel
>> notification being sent to Xen to that the response is placed on the
>> ring, but it might save space on the ring buffer for all other
>> cases/users.
>
> How is it wasteful? Those bytes are in a union with the x86 registers
> that are already in each vm_event request and response, and the size of
> that buffer is smaller than the size of the x86 registers struct.
>
> The buffer is simply ignored (with no effect whatsoever - on the size of
> the data involved or otherwise), except for the cases where it's needed,
> when the client application fills it up and sets the flag in the response.
>
> Am I missing something?
>
>
> Thanks,
> Razvan

Ah yes, I missed the fact that you put it into a union with regs. I
guess as long as it's the same size as regs it's fine. However, I
think the union will have to be named.

Thanks,
Tamas

Re: [PATCH 2/5] xen/vm_access: Support for memory-content hiding

[Razvan Cojocaru]

On 05/09/2015 11:33 AM, Tamas K Lengyel wrote:
> Ah yes, I missed the fact that you put it into a union with regs. I
> guess as long as it's the same size as regs it's fine. However, I
> think the union will have to be named.

Sure, I can name it. When I was working on it I was under the impression
that it's important to not disturb the API. I've since understood from
Andrew and Jan that that's not an issue.


Thanks,
Razvan

Re: [PATCH 2/5] xen/vm_access: Support for memory-content hiding

[Jan Beulich]

>>> On 09.05.15 at 08:55, <rcojocaru@bitdefender.com> wrote:
> On 05/09/2015 02:34 AM, Tamas K Lengyel wrote:
>>>>> @@ -193,6 +200,11 @@ struct vm_event_xsetbv {
>>>>> >>>      uint64_t value;
>>>>> >>>  };
>>>>> >>>
>>>>> >>> +struct vm_event_emul_read_data {
>>>>> >>> +    uint32_t size;
>>>>> >>> +    uint8_t  data[164];
>>>> >>
>>>> >> This number needs an explanation.
>>> >
>>> > It's less than the size of the x86_regs and enough for all the cases
>>> > we've tested so far...
>>> >
>>> >
>>> > Thanks,
>>> > Razvan
>> I feel like 164 bytes is really wasteful for all vm_events considering
>> this would be useful only in a very specific subset of cases. Not sure
>> what would be the right way to get around that.. Maybe having another
>> hypercall (potentionally under memop?) place that buffer somewhere
>> where Xen can access it before the vm_event response is processed?
>> That would require two hypercalls to be issued by the monitoring
>> domain, one to place the buffer and one for the event channel
>> notification being sent to Xen to that the response is placed on the
>> ring, but it might save space on the ring buffer for all other
>> cases/users.
> 
> How is it wasteful? Those bytes are in a union with the x86 registers
> that are already in each vm_event request and response, and the size of
> that buffer is smaller than the size of the x86 registers struct.

As said elsewhere already (but just to clarify things here too) -
the wastefulness isn't in the event structure, but in the addition
of a field of this type to struct arch_vcpu.

Jan

Re: [PATCH 2/5] xen/vm_access: Support for memory-content hiding

[Jan Beulich]

>>> On 08.05.15 at 18:49, <rcojocaru@bitdefender.com> wrote:
> On 05/08/2015 07:07 PM, Jan Beulich wrote:
>>>>> On 06.05.15 at 19:12, <rcojocaru@bitdefender.com> wrote:
>>> @@ -193,6 +200,11 @@ struct vm_event_xsetbv {
>>>      uint64_t value;
>>>  };
>>>  
>>> +struct vm_event_emul_read_data {
>>> +    uint32_t size;
>>> +    uint8_t  data[164];
>> 
>> This number needs an explanation.
> 
> It's less than the size of the x86_regs and enough for all the cases
> we've tested so far...

I was afraid of an explanation like that, which clearly cannot be
taken as a proper basis for a decision regarding a pubic interface.

Jan

Re: [PATCH 2/5] xen/vm_access: Support for memory-content hiding

[Razvan Cojocaru]

On 05/08/2015 07:07 PM, Jan Beulich wrote:
>> > --- a/xen/include/asm-x86/domain.h
>> > +++ b/xen/include/asm-x86/domain.h
>> > @@ -512,6 +513,7 @@ struct arch_vcpu
>> >          uint32_t emulate_flags;
>> >          unsigned long gpa;
>> >          unsigned long eip;
>> > +        struct vm_event_emul_read_data emul_read_data;
> Considering the size of this structure I don't think this should be
> there for all vCPU-s of all guests. IOW - please allocate this
> dynamically only on domains where it's needed.

Looking at the code, it's not immediately clear how to differentiate
between a domain where this is needed and one where it is not. At domain
init time we don't know, because it might become needed as soon as a
vm_event client becomes attached to the domain. Then again, even once a
vm_event client attaches to the domain, it might still not be necessary
to allocate that structure, as the client might never reply with
MEM_ACCESS_SET_EMUL_READ_DATA.

The only place where we know for sure that it's needed is in
p2m_mem_access_check(), when receiving a vm_event response with
MEM_ACCESS_SET_EMUL_READ_DATA (lazy allocation). Would this be alright?


Thanks,
Razvan

Re: [PATCH 2/5] xen/vm_access: Support for memory-content hiding

[Jan Beulich]

>>> On 08.06.15 at 12:02, <rcojocaru@bitdefender.com> wrote:
> On 05/08/2015 07:07 PM, Jan Beulich wrote:
>>> > --- a/xen/include/asm-x86/domain.h
>>> > +++ b/xen/include/asm-x86/domain.h
>>> > @@ -512,6 +513,7 @@ struct arch_vcpu
>>> >          uint32_t emulate_flags;
>>> >          unsigned long gpa;
>>> >          unsigned long eip;
>>> > +        struct vm_event_emul_read_data emul_read_data;
>> Considering the size of this structure I don't think this should be
>> there for all vCPU-s of all guests. IOW - please allocate this
>> dynamically only on domains where it's needed.
> 
> Looking at the code, it's not immediately clear how to differentiate
> between a domain where this is needed and one where it is not. At domain
> init time we don't know, because it might become needed as soon as a
> vm_event client becomes attached to the domain. Then again, even once a
> vm_event client attaches to the domain, it might still not be necessary
> to allocate that structure, as the client might never reply with
> MEM_ACCESS_SET_EMUL_READ_DATA.
> 
> The only place where we know for sure that it's needed is in
> p2m_mem_access_check(), when receiving a vm_event response with
> MEM_ACCESS_SET_EMUL_READ_DATA (lazy allocation). Would this be alright?

I can't really judge about that; the farther you can safely defer it,
the better. For the sake of addressing my complaint it would
probably suffice to defer it until a vm_event client attaches to the
guest.

Jan

[PATCH 3/5] xen/vm_event: Support for guest-requested events

[Razvan Cojocaru]

Added support for a new class of vm_events: VM_EVENT_REASON_REQUEST,
sent via HVMOP_request_vm_event. The guest can request that a
generic vm_event (containing only the vm_event-filled guest registers
as information) be sent to userspace by setting up the correct
registers and doing a VMCALL. For example, for a 64-bit guest, this
means: EAX = 34 (hvmop), EBX = 24 (HVMOP_request_vm_event).

Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
---
 tools/libxc/include/xenctrl.h   |    2 ++
 tools/libxc/xc_monitor.c        |   15 +++++++++++++++
 xen/arch/x86/hvm/event.c        |   14 ++++++++++++++
 xen/arch/x86/hvm/hvm.c          |    4 ++++
 xen/arch/x86/monitor.c          |   16 ++++++++++++++++
 xen/include/asm-x86/domain.h    |   32 +++++++++++++++++---------------
 xen/include/asm-x86/hvm/event.h |    1 +
 xen/include/public/domctl.h     |    6 ++++++
 xen/include/public/hvm/hvm_op.h |    4 ++++
 xen/include/public/vm_event.h   |    2 ++
 10 files changed, 81 insertions(+), 15 deletions(-)

diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index 1aa4f87..17a0bc8 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -2339,6 +2339,8 @@ int xc_monitor_software_breakpoint(xc_interface *xch, domid_t domain_id,
                                    bool enable);
 int xc_monitor_xsetbv(xc_interface *xch, domid_t domain_id, bool enable,
                       bool sync);
+int xc_monitor_requested(xc_interface *xch, domid_t domain_id, bool enable,
+                         bool sync);
 
 /***
  * Memory sharing operations.
diff --git a/tools/libxc/xc_monitor.c b/tools/libxc/xc_monitor.c
index aec2f4a..5d8f4f8 100644
--- a/tools/libxc/xc_monitor.c
+++ b/tools/libxc/xc_monitor.c
@@ -150,3 +150,18 @@ int xc_monitor_xsetbv(xc_interface *xch, domid_t domain_id, bool enable,
 
     return do_domctl(xch, &domctl);
 }
+
+int xc_monitor_requested(xc_interface *xch, domid_t domain_id, bool enable,
+                         bool sync)
+{
+    DECLARE_DOMCTL;
+
+    domctl.cmd = XEN_DOMCTL_monitor_op;
+    domctl.domain = domain_id;
+    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_REQUEST;
+    domctl.u.monitor_op.u.request.sync = sync;
+
+    return do_domctl(xch, &domctl);
+}
diff --git a/xen/arch/x86/hvm/event.c b/xen/arch/x86/hvm/event.c
index 5b869c8..b72ccbf 100644
--- a/xen/arch/x86/hvm/event.c
+++ b/xen/arch/x86/hvm/event.c
@@ -167,6 +167,20 @@ void hvm_event_xsetbv(unsigned long xcr, uint64_t value)
         hvm_event_traps(currad->monitor.xsetbv_sync, &req);
 }
 
+void hvm_event_requested(void)
+{
+    struct vcpu *curr = current;
+    struct arch_domain *currad = &current->domain->arch;
+
+    vm_event_request_t req = {
+        .reason = VM_EVENT_REASON_REQUEST,
+        .vcpu_id = curr->vcpu_id,
+    };
+
+    if ( currad->monitor.request_enabled )
+        hvm_event_traps(currad->monitor.request_sync, &req);
+}
+
 int hvm_event_int3(unsigned long gla)
 {
     int rc = 0;
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 86f9885..8ad03c6 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -6267,6 +6267,10 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
         break;
     }
 
+    case HVMOP_request_vm_event:
+        hvm_event_requested();
+        break;
+
     default:
     {
         gdprintk(XENLOG_DEBUG, "Bad HVM op %ld.\n", op);
diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c
index 6823a84..5176149 100644
--- a/xen/arch/x86/monitor.c
+++ b/xen/arch/x86/monitor.c
@@ -194,6 +194,22 @@ int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *mop)
         break;
     }
 
+    case XEN_DOMCTL_MONITOR_EVENT_REQUEST:
+    {
+        bool_t status = ad->monitor.request_enabled;
+
+        rc = status_check(mop, status);
+        if ( rc )
+            return rc;
+
+        ad->monitor.request_sync = mop->u.request.sync;
+
+        domain_pause(d);
+        ad->monitor.request_enabled = !status;
+        domain_unpause(d);
+        break;
+    }
+
     default:
         return -EOPNOTSUPP;
 
diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
index 2b89182..682ccc5 100644
--- a/xen/include/asm-x86/domain.h
+++ b/xen/include/asm-x86/domain.h
@@ -342,21 +342,23 @@ struct arch_domain
 
     /* Monitor options */
     struct {
-        uint16_t mov_to_cr0_enabled          : 1;
-        uint16_t mov_to_cr0_sync             : 1;
-        uint16_t mov_to_cr0_onchangeonly     : 1;
-        uint16_t mov_to_cr3_enabled          : 1;
-        uint16_t mov_to_cr3_sync             : 1;
-        uint16_t mov_to_cr3_onchangeonly     : 1;
-        uint16_t mov_to_cr4_enabled          : 1;
-        uint16_t mov_to_cr4_sync             : 1;
-        uint16_t mov_to_cr4_onchangeonly     : 1;
-        uint16_t mov_to_msr_enabled          : 1;
-        uint16_t mov_to_msr_extended         : 1;
-        uint16_t singlestep_enabled          : 1;
-        uint16_t software_breakpoint_enabled : 1;
-        uint16_t xsetbv_enabled              : 1;
-        uint16_t xsetbv_sync                 : 1;
+        uint32_t mov_to_cr0_enabled          : 1;
+        uint32_t mov_to_cr0_sync             : 1;
+        uint32_t mov_to_cr0_onchangeonly     : 1;
+        uint32_t mov_to_cr3_enabled          : 1;
+        uint32_t mov_to_cr3_sync             : 1;
+        uint32_t mov_to_cr3_onchangeonly     : 1;
+        uint32_t mov_to_cr4_enabled          : 1;
+        uint32_t mov_to_cr4_sync             : 1;
+        uint32_t mov_to_cr4_onchangeonly     : 1;
+        uint32_t mov_to_msr_enabled          : 1;
+        uint32_t mov_to_msr_extended         : 1;
+        uint32_t singlestep_enabled          : 1;
+        uint32_t software_breakpoint_enabled : 1;
+        uint32_t xsetbv_enabled              : 1;
+        uint32_t xsetbv_sync                 : 1;
+        uint32_t request_enabled             : 1;
+        uint32_t request_sync                : 1;
     } monitor;
 
     /* Mem_access emulation control */
diff --git a/xen/include/asm-x86/hvm/event.h b/xen/include/asm-x86/hvm/event.h
index b2cf3bc..ca63055 100644
--- a/xen/include/asm-x86/hvm/event.h
+++ b/xen/include/asm-x86/hvm/event.h
@@ -24,6 +24,7 @@ void hvm_event_cr3(unsigned long value, unsigned long old);
 void hvm_event_cr4(unsigned long value, unsigned long old);
 void hvm_event_msr(unsigned int msr, uint64_t value);
 void hvm_event_xsetbv(unsigned long xcr, uint64_t value);
+void hvm_event_requested(void);
 /* Called for current VCPU: returns -1 if no listener */
 int hvm_event_int3(unsigned long gla);
 int hvm_event_single_step(unsigned long gla);
diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index b866e33..a627360 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -1019,6 +1019,7 @@ DEFINE_XEN_GUEST_HANDLE(xen_domctl_psr_cmt_op_t);
 #define XEN_DOMCTL_MONITOR_EVENT_SINGLESTEP            4
 #define XEN_DOMCTL_MONITOR_EVENT_SOFTWARE_BREAKPOINT   5
 #define XEN_DOMCTL_MONITOR_EVENT_XSETBV                6
+#define XEN_DOMCTL_MONITOR_EVENT_REQUEST               7
 
 struct xen_domctl_monitor_op {
     uint32_t op; /* XEN_DOMCTL_MONITOR_OP_* */
@@ -1044,6 +1045,11 @@ struct xen_domctl_monitor_op {
             /* Pause vCPU until response */
             uint8_t sync;
         } xsetbv;
+
+        struct {
+            /* Pause vCPU until response */
+            uint8_t sync;
+        } request;
     } u;
 };
 typedef struct xen_domctl__op xen_domctl_monitor_op_t;
diff --git a/xen/include/public/hvm/hvm_op.h b/xen/include/public/hvm/hvm_op.h
index cde3571..cb5168a 100644
--- a/xen/include/public/hvm/hvm_op.h
+++ b/xen/include/public/hvm/hvm_op.h
@@ -389,6 +389,10 @@ DEFINE_XEN_GUEST_HANDLE(xen_hvm_evtchn_upcall_vector_t);
 
 #endif /* defined(__i386__) || defined(__x86_64__) */
 
+#if defined(__XEN__) || defined(__XEN_TOOLS__)
+#define HVMOP_request_vm_event 24
+#endif /* defined(__XEN__) || defined(__XEN_TOOLS__) */
+
 #endif /* __XEN_PUBLIC_HVM_HVM_OP_H__ */
 
 /*
diff --git a/xen/include/public/vm_event.h b/xen/include/public/vm_event.h
index bce3e3e..2913a85 100644
--- a/xen/include/public/vm_event.h
+++ b/xen/include/public/vm_event.h
@@ -74,6 +74,8 @@
 #define VM_EVENT_REASON_SINGLESTEP              9
 /* An XCR was updated. */
 #define VM_EVENT_REASON_XSETBV                 10
+/* An event has been requested via HVMOP_request_vm_event. */
+#define VM_EVENT_REASON_REQUEST                11
 
 /*
  * Using a custom struct (not hvm_hw_cpu) so as to not fill
-- 
1.7.9.5

Re: [PATCH 3/5] xen/vm_event: Support for guest-requested events

[Tamas K Lengyel]

Hi Razvan,
I guess I don't really see into the setup you envision using this
event-type with, but wouldn't it be possible to just use libxenvchan?

Tamas

On Wed, May 6, 2015 at 7:12 PM, Razvan Cojocaru
<rcojocaru@bitdefender.com> wrote:
> Added support for a new class of vm_events: VM_EVENT_REASON_REQUEST,
> sent via HVMOP_request_vm_event. The guest can request that a
> generic vm_event (containing only the vm_event-filled guest registers
> as information) be sent to userspace by setting up the correct
> registers and doing a VMCALL. For example, for a 64-bit guest, this
> means: EAX = 34 (hvmop), EBX = 24 (HVMOP_request_vm_event).
>
> Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
> ---
>  tools/libxc/include/xenctrl.h   |    2 ++
>  tools/libxc/xc_monitor.c        |   15 +++++++++++++++
>  xen/arch/x86/hvm/event.c        |   14 ++++++++++++++
>  xen/arch/x86/hvm/hvm.c          |    4 ++++
>  xen/arch/x86/monitor.c          |   16 ++++++++++++++++
>  xen/include/asm-x86/domain.h    |   32 +++++++++++++++++---------------
>  xen/include/asm-x86/hvm/event.h |    1 +
>  xen/include/public/domctl.h     |    6 ++++++
>  xen/include/public/hvm/hvm_op.h |    4 ++++
>  xen/include/public/vm_event.h   |    2 ++
>  10 files changed, 81 insertions(+), 15 deletions(-)
>
> diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
> index 1aa4f87..17a0bc8 100644
> --- a/tools/libxc/include/xenctrl.h
> +++ b/tools/libxc/include/xenctrl.h
> @@ -2339,6 +2339,8 @@ int xc_monitor_software_breakpoint(xc_interface *xch, domid_t domain_id,
>                                     bool enable);
>  int xc_monitor_xsetbv(xc_interface *xch, domid_t domain_id, bool enable,
>                        bool sync);
> +int xc_monitor_requested(xc_interface *xch, domid_t domain_id, bool enable,
> +                         bool sync);
>
>  /***
>   * Memory sharing operations.
> diff --git a/tools/libxc/xc_monitor.c b/tools/libxc/xc_monitor.c
> index aec2f4a..5d8f4f8 100644
> --- a/tools/libxc/xc_monitor.c
> +++ b/tools/libxc/xc_monitor.c
> @@ -150,3 +150,18 @@ int xc_monitor_xsetbv(xc_interface *xch, domid_t domain_id, bool enable,
>
>      return do_domctl(xch, &domctl);
>  }
> +
> +int xc_monitor_requested(xc_interface *xch, domid_t domain_id, bool enable,
> +                         bool sync)
> +{
> +    DECLARE_DOMCTL;
> +
> +    domctl.cmd = XEN_DOMCTL_monitor_op;
> +    domctl.domain = domain_id;
> +    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_REQUEST;
> +    domctl.u.monitor_op.u.request.sync = sync;
> +
> +    return do_domctl(xch, &domctl);
> +}
> diff --git a/xen/arch/x86/hvm/event.c b/xen/arch/x86/hvm/event.c
> index 5b869c8..b72ccbf 100644
> --- a/xen/arch/x86/hvm/event.c
> +++ b/xen/arch/x86/hvm/event.c
> @@ -167,6 +167,20 @@ void hvm_event_xsetbv(unsigned long xcr, uint64_t value)
>          hvm_event_traps(currad->monitor.xsetbv_sync, &req);
>  }
>
> +void hvm_event_requested(void)
> +{
> +    struct vcpu *curr = current;
> +    struct arch_domain *currad = &current->domain->arch;
> +
> +    vm_event_request_t req = {
> +        .reason = VM_EVENT_REASON_REQUEST,
> +        .vcpu_id = curr->vcpu_id,
> +    };
> +
> +    if ( currad->monitor.request_enabled )
> +        hvm_event_traps(currad->monitor.request_sync, &req);
> +}
> +
>  int hvm_event_int3(unsigned long gla)
>  {
>      int rc = 0;
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 86f9885..8ad03c6 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -6267,6 +6267,10 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
>          break;
>      }
>
> +    case HVMOP_request_vm_event:
> +        hvm_event_requested();
> +        break;
> +
>      default:
>      {
>          gdprintk(XENLOG_DEBUG, "Bad HVM op %ld.\n", op);
> diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c
> index 6823a84..5176149 100644
> --- a/xen/arch/x86/monitor.c
> +++ b/xen/arch/x86/monitor.c
> @@ -194,6 +194,22 @@ int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *mop)
>          break;
>      }
>
> +    case XEN_DOMCTL_MONITOR_EVENT_REQUEST:
> +    {
> +        bool_t status = ad->monitor.request_enabled;
> +
> +        rc = status_check(mop, status);
> +        if ( rc )
> +            return rc;
> +
> +        ad->monitor.request_sync = mop->u.request.sync;
> +
> +        domain_pause(d);
> +        ad->monitor.request_enabled = !status;
> +        domain_unpause(d);
> +        break;
> +    }
> +
>      default:
>          return -EOPNOTSUPP;
>
> diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
> index 2b89182..682ccc5 100644
> --- a/xen/include/asm-x86/domain.h
> +++ b/xen/include/asm-x86/domain.h
> @@ -342,21 +342,23 @@ struct arch_domain
>
>      /* Monitor options */
>      struct {
> -        uint16_t mov_to_cr0_enabled          : 1;
> -        uint16_t mov_to_cr0_sync             : 1;
> -        uint16_t mov_to_cr0_onchangeonly     : 1;
> -        uint16_t mov_to_cr3_enabled          : 1;
> -        uint16_t mov_to_cr3_sync             : 1;
> -        uint16_t mov_to_cr3_onchangeonly     : 1;
> -        uint16_t mov_to_cr4_enabled          : 1;
> -        uint16_t mov_to_cr4_sync             : 1;
> -        uint16_t mov_to_cr4_onchangeonly     : 1;
> -        uint16_t mov_to_msr_enabled          : 1;
> -        uint16_t mov_to_msr_extended         : 1;
> -        uint16_t singlestep_enabled          : 1;
> -        uint16_t software_breakpoint_enabled : 1;
> -        uint16_t xsetbv_enabled              : 1;
> -        uint16_t xsetbv_sync                 : 1;
> +        uint32_t mov_to_cr0_enabled          : 1;
> +        uint32_t mov_to_cr0_sync             : 1;
> +        uint32_t mov_to_cr0_onchangeonly     : 1;
> +        uint32_t mov_to_cr3_enabled          : 1;
> +        uint32_t mov_to_cr3_sync             : 1;
> +        uint32_t mov_to_cr3_onchangeonly     : 1;
> +        uint32_t mov_to_cr4_enabled          : 1;
> +        uint32_t mov_to_cr4_sync             : 1;
> +        uint32_t mov_to_cr4_onchangeonly     : 1;
> +        uint32_t mov_to_msr_enabled          : 1;
> +        uint32_t mov_to_msr_extended         : 1;
> +        uint32_t singlestep_enabled          : 1;
> +        uint32_t software_breakpoint_enabled : 1;
> +        uint32_t xsetbv_enabled              : 1;
> +        uint32_t xsetbv_sync                 : 1;
> +        uint32_t request_enabled             : 1;
> +        uint32_t request_sync                : 1;
>      } monitor;
>
>      /* Mem_access emulation control */
> diff --git a/xen/include/asm-x86/hvm/event.h b/xen/include/asm-x86/hvm/event.h
> index b2cf3bc..ca63055 100644
> --- a/xen/include/asm-x86/hvm/event.h
> +++ b/xen/include/asm-x86/hvm/event.h
> @@ -24,6 +24,7 @@ void hvm_event_cr3(unsigned long value, unsigned long old);
>  void hvm_event_cr4(unsigned long value, unsigned long old);
>  void hvm_event_msr(unsigned int msr, uint64_t value);
>  void hvm_event_xsetbv(unsigned long xcr, uint64_t value);
> +void hvm_event_requested(void);
>  /* Called for current VCPU: returns -1 if no listener */
>  int hvm_event_int3(unsigned long gla);
>  int hvm_event_single_step(unsigned long gla);
> diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
> index b866e33..a627360 100644
> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -1019,6 +1019,7 @@ DEFINE_XEN_GUEST_HANDLE(xen_domctl_psr_cmt_op_t);
>  #define XEN_DOMCTL_MONITOR_EVENT_SINGLESTEP            4
>  #define XEN_DOMCTL_MONITOR_EVENT_SOFTWARE_BREAKPOINT   5
>  #define XEN_DOMCTL_MONITOR_EVENT_XSETBV                6
> +#define XEN_DOMCTL_MONITOR_EVENT_REQUEST               7
>
>  struct xen_domctl_monitor_op {
>      uint32_t op; /* XEN_DOMCTL_MONITOR_OP_* */
> @@ -1044,6 +1045,11 @@ struct xen_domctl_monitor_op {
>              /* Pause vCPU until response */
>              uint8_t sync;
>          } xsetbv;
> +
> +        struct {
> +            /* Pause vCPU until response */
> +            uint8_t sync;
> +        } request;
>      } u;
>  };
>  typedef struct xen_domctl__op xen_domctl_monitor_op_t;
> diff --git a/xen/include/public/hvm/hvm_op.h b/xen/include/public/hvm/hvm_op.h
> index cde3571..cb5168a 100644
> --- a/xen/include/public/hvm/hvm_op.h
> +++ b/xen/include/public/hvm/hvm_op.h
> @@ -389,6 +389,10 @@ DEFINE_XEN_GUEST_HANDLE(xen_hvm_evtchn_upcall_vector_t);
>
>  #endif /* defined(__i386__) || defined(__x86_64__) */
>
> +#if defined(__XEN__) || defined(__XEN_TOOLS__)
> +#define HVMOP_request_vm_event 24
> +#endif /* defined(__XEN__) || defined(__XEN_TOOLS__) */
> +
>  #endif /* __XEN_PUBLIC_HVM_HVM_OP_H__ */
>
>  /*
> diff --git a/xen/include/public/vm_event.h b/xen/include/public/vm_event.h
> index bce3e3e..2913a85 100644
> --- a/xen/include/public/vm_event.h
> +++ b/xen/include/public/vm_event.h
> @@ -74,6 +74,8 @@
>  #define VM_EVENT_REASON_SINGLESTEP              9
>  /* An XCR was updated. */
>  #define VM_EVENT_REASON_XSETBV                 10
> +/* An event has been requested via HVMOP_request_vm_event. */
> +#define VM_EVENT_REASON_REQUEST                11
>
>  /*
>   * Using a custom struct (not hvm_hw_cpu) so as to not fill
> --
> 1.7.9.5
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH 3/5] xen/vm_event: Support for guest-requested events

[Razvan Cojocaru]

On 05/07/2015 08:05 PM, Tamas K Lengyel wrote:
> Hi Razvan,
> I guess I don't really see into the setup you envision using this
> event-type with, but wouldn't it be possible to just use libxenvchan?

Hello Tamas,

This patch evolved from "xen: Support for VMCALL mem_events":

http://lists.xen.org/archives/html/xen-devel/2014-07/msg00313.html

that I've sent in a previous series. Basically, it allows the guest to
trigger a vm_event that contains some of the guest's state. In the
current incarnation of the patch, the state consists of the x86
registers that have been filled in prior to sending out the vm_event.
So, just a way for the guest itself to be able to send out a generic
vm_event to any interested consumer. We thought that might be
interesting for everyone, since the event can be used for almost
anything and is not memory-introspection (or any particular thing) specific.

The main difference between the initial patch and this one (besides the
vm_event work that has taken place since) is that this is now a proper
hypercall and the magic numbers are gone. I have to thank Andrew Cooper
for the suggestion to do it this way.

To answer your question, I haven't really looked closely at libxenvchan
before, but our use case at least can't use it (we're injecting some
code in the guest on the fly). Am I wrong in assuming libxenvchan also
requires two PV domains talking to each other? The guests we're
monitoring are HVM guests, and _they_ are supposed to issue the
VMCALL-triggered events.


Thanks,
Razvan

Re: [PATCH 3/5] xen/vm_event: Support for guest-requested events

[Tamas K Lengyel]

> To answer your question, I haven't really looked closely at libxenvchan
> before, but our use case at least can't use it (we're injecting some
> code in the guest on the fly). Am I wrong in assuming libxenvchan also
> requires two PV domains talking to each other? The guests we're
> monitoring are HVM guests, and _they_ are supposed to issue the
> VMCALL-triggered events.

Libxenvchan is not PV specific AFAICT and it provides a really robust
interface to establish communication channels between domUs. It
usually is a better way to exchange lots of information instead of the
fairly limited vm_event ring page as libxenvchan can automatically
adjust the shared mem region to match the data being exchanged and do
it UDP or TCP style. I don't really see a downside in having this type
of event added, I was just wondering what the rationale is behind it
;)

Tamas

>
>
> Thanks,
> Razvan

Re: [PATCH 3/5] xen/vm_event: Support for guest-requested events

[Jan Beulich]

>>> On 06.05.15 at 19:12, <rcojocaru@bitdefender.com> wrote:
> Added support for a new class of vm_events: VM_EVENT_REASON_REQUEST,

Perhaps better VM_EVENT_REASON_GUEST_REQUEST?

> --- a/xen/arch/x86/hvm/event.c
> +++ b/xen/arch/x86/hvm/event.c
> @@ -167,6 +167,20 @@ void hvm_event_xsetbv(unsigned long xcr, uint64_t value)
>          hvm_event_traps(currad->monitor.xsetbv_sync, &req);
>  }
>  
> +void hvm_event_requested(void)
> +{
> +    struct vcpu *curr = current;
> +    struct arch_domain *currad = &current->domain->arch;

So you latched current into curr and then use current again?

Jan

Re: [PATCH 3/5] xen/vm_event: Support for guest-requested events

[Razvan Cojocaru]

On 05/08/2015 07:16 PM, Jan Beulich wrote:
>>>> On 06.05.15 at 19:12, <rcojocaru@bitdefender.com> wrote:
>> Added support for a new class of vm_events: VM_EVENT_REASON_REQUEST,
> 
> Perhaps better VM_EVENT_REASON_GUEST_REQUEST?

It does sound better, I'll change it.

>> --- a/xen/arch/x86/hvm/event.c
>> +++ b/xen/arch/x86/hvm/event.c
>> @@ -167,6 +167,20 @@ void hvm_event_xsetbv(unsigned long xcr, uint64_t value)
>>          hvm_event_traps(currad->monitor.xsetbv_sync, &req);
>>  }
>>  
>> +void hvm_event_requested(void)
>> +{
>> +    struct vcpu *curr = current;
>> +    struct arch_domain *currad = &current->domain->arch;
> 
> So you latched current into curr and then use current again?

Ack, will fix that.


Thanks,
Razvan

Re: [PATCH 3/5] xen/vm_event: Support for guest-requested events

[Andrew Cooper]

On 06/05/15 18:12, Razvan Cojocaru wrote:
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 86f9885..8ad03c6 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -6267,6 +6267,10 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
>          break;
>      }
>  
> +    case HVMOP_request_vm_event:
> +        hvm_event_requested();
> +        break;
> +

This is much better than the previous version, but there is nothing
HVM-specific about having an arbitrary signal to the vm_event subsystem.

It might be better to chose a different hypercall to hang a subop off. 
On the other hand, we already have things like HVMOP_xentrace, and there
is nothing actually preventing PV domains issuing a
HVMOP_request_vm_event.  Perhaps it doesn't matter too much.

~Andrew

Re: [PATCH 3/5] xen/vm_event: Support for guest-requested events

[Razvan Cojocaru]

On 05/08/2015 07:50 PM, Andrew Cooper wrote:
> On 06/05/15 18:12, Razvan Cojocaru wrote:
>> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
>> index 86f9885..8ad03c6 100644
>> --- a/xen/arch/x86/hvm/hvm.c
>> +++ b/xen/arch/x86/hvm/hvm.c
>> @@ -6267,6 +6267,10 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
>>          break;
>>      }
>>  
>> +    case HVMOP_request_vm_event:
>> +        hvm_event_requested();
>> +        break;
>> +
> 
> This is much better than the previous version, but there is nothing
> HVM-specific about having an arbitrary signal to the vm_event subsystem.
> 
> It might be better to chose a different hypercall to hang a subop off. 
> On the other hand, we already have things like HVMOP_xentrace, and there
> is nothing actually preventing PV domains issuing a
> HVMOP_request_vm_event.  Perhaps it doesn't matter too much.

Working on this again, I'm fine with changing the hypercall but looking
at xen.h none of the others strike me as particularly more intuitive
than __HYPERVISOR_hvm_op. Could you please suggest one that you think
fits the bill?


Thanks,
Razvan

[PATCH 4/5] xen/vm_event: Deny MSR writes if refused by vm_event reply

[Razvan Cojocaru]

Deny MSR writes if a vm_client subscribed to mov_to_msr events
forbids them.

Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
---
 xen/arch/x86/hvm/emulate.c        |    2 +-
 xen/arch/x86/hvm/hvm.c            |   24 ++++++++++++++++++++++--
 xen/arch/x86/hvm/svm/svm.c        |    2 +-
 xen/arch/x86/hvm/vmx/vmx.c        |    2 +-
 xen/arch/x86/hvm/vmx/vvmx.c       |    6 ++++--
 xen/arch/x86/mm/p2m.c             |    3 +++
 xen/common/vm_event.c             |    1 +
 xen/include/asm-x86/domain.h      |    5 +++++
 xen/include/asm-x86/hvm/support.h |    3 ++-
 xen/include/public/vm_event.h     |    5 +++++
 10 files changed, 45 insertions(+), 8 deletions(-)

diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
index 0058b15..566eee7 100644
--- a/xen/arch/x86/hvm/emulate.c
+++ b/xen/arch/x86/hvm/emulate.c
@@ -1293,7 +1293,7 @@ static int hvmemul_write_msr(
     uint64_t val,
     struct x86_emulate_ctxt *ctxt)
 {
-    return hvm_msr_write_intercept(reg, val);
+    return hvm_msr_write_intercept(reg, val, 1);
 }
 
 static int hvmemul_wbinvd(
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 8ad03c6..ed0ec9a 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -464,6 +464,15 @@ void hvm_do_resume(struct vcpu *v)
         }
     }
 
+    ASSERT(v == current);
+
+    if ( v->arch.msr_write.do_write )
+    {
+        hvm_msr_write_intercept(v->arch.msr_write.msr,
+                                v->arch.msr_write.value, 0);
+        v->arch.msr_write.do_write = 0;
+    }
+
     /* Inject pending hw/sw trap */
     if ( v->arch.hvm_vcpu.inject_trap.vector != -1 ) 
     {
@@ -4517,12 +4526,14 @@ int hvm_msr_read_intercept(unsigned int msr, uint64_t *msr_content)
     goto out;
 }
 
-int hvm_msr_write_intercept(unsigned int msr, uint64_t msr_content)
+int hvm_msr_write_intercept(unsigned int msr, uint64_t msr_content,
+                            bool_t event_only)
 {
     struct vcpu *v = current;
     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));
@@ -4530,7 +4541,16 @@ 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));
 
-    hvm_event_msr(msr, msr_content);
+    if ( event_only && unlikely(currad->monitor.mov_to_msr_enabled) )
+    {
+        /* The actual write will occur in hvm_do_resume() (if permitted). */
+        v->arch.msr_write.do_write = 1;
+        v->arch.msr_write.msr = msr;
+        v->arch.msr_write.value = msr_content;
+
+        hvm_event_msr(msr, msr_content);
+        return X86EMUL_OKAY;
+    }
 
     switch ( msr )
     {
diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index 6734fb6..b9b4791 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -1945,7 +1945,7 @@ static void svm_do_msr_access(struct cpu_user_regs *regs)
         if ( (inst_len = __get_instruction_length(v, INSTR_WRMSR)) == 0 )
             return;
         msr_content = ((uint64_t)regs->edx << 32) | (uint32_t)regs->eax;
-        rc = hvm_msr_write_intercept(regs->ecx, msr_content);
+        rc = hvm_msr_write_intercept(regs->ecx, msr_content, 1);
     }
 
     if ( rc == X86EMUL_OKAY )
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 74f563f..562507a 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -3044,7 +3044,7 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
     {
         uint64_t msr_content;
         msr_content = ((uint64_t)regs->edx << 32) | (uint32_t)regs->eax;
-        if ( hvm_msr_write_intercept(regs->ecx, msr_content) == X86EMUL_OKAY )
+        if ( hvm_msr_write_intercept(regs->ecx, msr_content, 1) == X86EMUL_OKAY )
             update_guest_eip(); /* Safe: WRMSR */
         break;
     }
diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
index ac6e3b3..01726d5 100644
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -1056,7 +1056,8 @@ static void load_shadow_guest_state(struct vcpu *v)
     if ( control & VM_ENTRY_LOAD_GUEST_PAT )
         hvm_set_guest_pat(v, __get_vvmcs(vvmcs, GUEST_PAT));
     if ( control & VM_ENTRY_LOAD_PERF_GLOBAL_CTRL )
-        hvm_msr_write_intercept(MSR_CORE_PERF_GLOBAL_CTRL, __get_vvmcs(vvmcs, GUEST_PERF_GLOBAL_CTRL));
+        hvm_msr_write_intercept(MSR_CORE_PERF_GLOBAL_CTRL,
+                                __get_vvmcs(vvmcs, GUEST_PERF_GLOBAL_CTRL), 1);
 
     hvm_funcs.set_tsc_offset(v, v->arch.hvm_vcpu.cache_tsc_offset, 0);
 
@@ -1257,7 +1258,8 @@ static void load_vvmcs_host_state(struct vcpu *v)
     if ( control & VM_EXIT_LOAD_HOST_PAT )
         hvm_set_guest_pat(v, __get_vvmcs(vvmcs, HOST_PAT));
     if ( control & VM_EXIT_LOAD_PERF_GLOBAL_CTRL )
-        hvm_msr_write_intercept(MSR_CORE_PERF_GLOBAL_CTRL, __get_vvmcs(vvmcs, HOST_PERF_GLOBAL_CTRL));
+        hvm_msr_write_intercept(MSR_CORE_PERF_GLOBAL_CTRL,
+                                __get_vvmcs(vvmcs, HOST_PERF_GLOBAL_CTRL), 1);
 
     hvm_funcs.set_tsc_offset(v, v->arch.hvm_vcpu.cache_tsc_offset, 0);
 
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index 0b2f9a6..1939d27 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -1417,6 +1417,9 @@ static void p2m_vm_event_fill_regs(vm_event_request_t *req)
 void p2m_mem_access_emulate_check(struct vcpu *v,
                                   const vm_event_response_t *rsp)
 {
+    if ( rsp->flags & MEM_ACCESS_SKIP_MSR_WRITE )
+        v->arch.msr_write.do_write = 0;
+
     /* Mark vcpu for skipping one instruction upon rescheduling. */
     if ( rsp->flags & MEM_ACCESS_EMULATE )
     {
diff --git a/xen/common/vm_event.c b/xen/common/vm_event.c
index 120a78a..844e892 100644
--- a/xen/common/vm_event.c
+++ b/xen/common/vm_event.c
@@ -387,6 +387,7 @@ void vm_event_resume(struct domain *d, struct vm_event_domain *ved)
 
 #ifdef HAS_MEM_ACCESS
         case VM_EVENT_REASON_MEM_ACCESS:
+        case VM_EVENT_REASON_MOV_TO_MSR:
             mem_access_resume(v, &rsp);
             break;
 #endif
diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
index 682ccc5..e70e2bc 100644
--- a/xen/include/asm-x86/domain.h
+++ b/xen/include/asm-x86/domain.h
@@ -518,6 +518,11 @@ struct arch_vcpu
         struct vm_event_emul_read_data emul_read_data;
     } vm_event;
 
+    struct {
+        bool_t do_write;
+        uint64_t msr;
+        uint64_t value;
+    } msr_write;
 };
 
 smap_check_policy_t smap_policy_change(struct vcpu *v,
diff --git a/xen/include/asm-x86/hvm/support.h b/xen/include/asm-x86/hvm/support.h
index 05ef5c5..f55373e 100644
--- a/xen/include/asm-x86/hvm/support.h
+++ b/xen/include/asm-x86/hvm/support.h
@@ -128,7 +128,8 @@ int hvm_set_cr0(unsigned long value);
 int hvm_set_cr3(unsigned long value);
 int hvm_set_cr4(unsigned long value);
 int hvm_msr_read_intercept(unsigned int msr, uint64_t *msr_content);
-int hvm_msr_write_intercept(unsigned int msr, uint64_t msr_content);
+int hvm_msr_write_intercept(
+    unsigned int msr, uint64_t msr_content, bool_t event_only);
 int hvm_mov_to_cr(unsigned int cr, unsigned int gpr);
 int hvm_mov_from_cr(unsigned int cr, unsigned int gpr);
 
diff --git a/xen/include/public/vm_event.h b/xen/include/public/vm_event.h
index 2913a85..36190cc 100644
--- a/xen/include/public/vm_event.h
+++ b/xen/include/public/vm_event.h
@@ -158,6 +158,11 @@ struct vm_event_regs_x86 {
  * MEM_ACCESS_EMULATE_NOWRITE.
  */
 #define MEM_ACCESS_SET_EMUL_READ_DATA   (1 << 8)
+ /*
+  * If mov_to_msr events are enabled, setting this flag in the vm_event
+  * response denies the MSR write that triggered the event.
+  */
+#define MEM_ACCESS_SKIP_MSR_WRITE       (1 << 9)
 
 struct vm_event_mem_access {
     uint64_t gfn;
-- 
1.7.9.5

Re: [PATCH 4/5] xen/vm_event: Deny MSR writes if refused by vm_event reply

[Jan Beulich]

>>> On 06.05.15 at 19:12, <rcojocaru@bitdefender.com> wrote:
> --- a/xen/include/asm-x86/domain.h
> +++ b/xen/include/asm-x86/domain.h
> @@ -518,6 +518,11 @@ struct arch_vcpu
>          struct vm_event_emul_read_data emul_read_data;
>      } vm_event;
>  
> +    struct {
> +        bool_t do_write;
> +        uint64_t msr;
> +        uint64_t value;
> +    } msr_write;
>  };

Again a growth of struct vcpu by 24 bytes for everyone even though
quite likely only very few VMs would actually need this. To be honest
I'd even be hesitant to accept a pointer addition here. Perhaps this
should be a suitably sized, dynamically allocated array hanging off of
struct domain?

> --- a/xen/include/public/vm_event.h
> +++ b/xen/include/public/vm_event.h
> @@ -158,6 +158,11 @@ struct vm_event_regs_x86 {
>   * MEM_ACCESS_EMULATE_NOWRITE.
>   */
>  #define MEM_ACCESS_SET_EMUL_READ_DATA   (1 << 8)
> + /*
> +  * If mov_to_msr events are enabled, setting this flag in the vm_event
> +  * response denies the MSR write that triggered the event.
> +  */
> +#define MEM_ACCESS_SKIP_MSR_WRITE       (1 << 9)

>From an interface point of view - does this need to be MSR-
specific? I.e. can't this just be a flag to deny whatever the
operation was (not necessarily supported/valid for all events,
but possibly for more than just MSR writes)?

Jan

Re: [PATCH 4/5] xen/vm_event: Deny MSR writes if refused by vm_event reply

[Razvan Cojocaru]

On 05/08/2015 07:23 PM, Jan Beulich wrote:
>>>> On 06.05.15 at 19:12, <rcojocaru@bitdefender.com> wrote:
>> --- a/xen/include/asm-x86/domain.h
>> +++ b/xen/include/asm-x86/domain.h
>> @@ -518,6 +518,11 @@ struct arch_vcpu
>>          struct vm_event_emul_read_data emul_read_data;
>>      } vm_event;
>>  
>> +    struct {
>> +        bool_t do_write;
>> +        uint64_t msr;
>> +        uint64_t value;
>> +    } msr_write;
>>  };
> 
> Again a growth of struct vcpu by 24 bytes for everyone even though
> quite likely only very few VMs would actually need this. To be honest
> I'd even be hesitant to accept a pointer addition here. Perhaps this
> should be a suitably sized, dynamically allocated array hanging off of
> struct domain?

Sorry, I don't follow the dynamically allocated _array_ part. Could you
please give a small example of what you mean?

>> --- a/xen/include/public/vm_event.h
>> +++ b/xen/include/public/vm_event.h
>> @@ -158,6 +158,11 @@ struct vm_event_regs_x86 {
>>   * MEM_ACCESS_EMULATE_NOWRITE.
>>   */
>>  #define MEM_ACCESS_SET_EMUL_READ_DATA   (1 << 8)
>> + /*
>> +  * If mov_to_msr events are enabled, setting this flag in the vm_event
>> +  * response denies the MSR write that triggered the event.
>> +  */
>> +#define MEM_ACCESS_SKIP_MSR_WRITE       (1 << 9)
> 
>>From an interface point of view - does this need to be MSR-
> specific? I.e. can't this just be a flag to deny whatever the
> operation was (not necessarily supported/valid for all events,
> but possibly for more than just MSR writes)?

Yes, that's a good idea - it could just be a DENY flag, and the actual
action to be rejected can be inferred from the response type.


Thanks,
Razvan

Re: [PATCH 4/5] xen/vm_event: Deny MSR writes if refused by vm_event reply

[Jan Beulich]

>>> On 08.05.15 at 19:05, <rcojocaru@bitdefender.com> wrote:
> On 05/08/2015 07:23 PM, Jan Beulich wrote:
>>>>> On 06.05.15 at 19:12, <rcojocaru@bitdefender.com> wrote:
>>> --- a/xen/include/asm-x86/domain.h
>>> +++ b/xen/include/asm-x86/domain.h
>>> @@ -518,6 +518,11 @@ struct arch_vcpu
>>>          struct vm_event_emul_read_data emul_read_data;
>>>      } vm_event;
>>>  
>>> +    struct {
>>> +        bool_t do_write;
>>> +        uint64_t msr;
>>> +        uint64_t value;
>>> +    } msr_write;
>>>  };
>> 
>> Again a growth of struct vcpu by 24 bytes for everyone even though
>> quite likely only very few VMs would actually need this. To be honest
>> I'd even be hesitant to accept a pointer addition here. Perhaps this
>> should be a suitably sized, dynamically allocated array hanging off of
>> struct domain?
> 
> Sorry, I don't follow the dynamically allocated _array_ part. Could you
> please give a small example of what you mean?

Have a pointer in struct arch_domain which gets populated only
when needed, and if so by an array allocation with the array
dimension being the maximum number of vCPU-s the domain may
have. That way the memory overhead for non-monitored domains
is a pointer per domain (as opposed to quite a bit more unused
space per vCPU).

Jan

Re: [PATCH 4/5] xen/vm_event: Deny MSR writes if refused by vm_event reply

[Razvan Cojocaru]

On 05/11/2015 10:03 AM, Jan Beulich wrote:
>>>> On 08.05.15 at 19:05, <rcojocaru@bitdefender.com> wrote:
>> On 05/08/2015 07:23 PM, Jan Beulich wrote:
>>>>>> On 06.05.15 at 19:12, <rcojocaru@bitdefender.com> wrote:
>>>> --- a/xen/include/asm-x86/domain.h
>>>> +++ b/xen/include/asm-x86/domain.h
>>>> @@ -518,6 +518,11 @@ struct arch_vcpu
>>>>          struct vm_event_emul_read_data emul_read_data;
>>>>      } vm_event;
>>>>  
>>>> +    struct {
>>>> +        bool_t do_write;
>>>> +        uint64_t msr;
>>>> +        uint64_t value;
>>>> +    } msr_write;
>>>>  };
>>>
>>> Again a growth of struct vcpu by 24 bytes for everyone even though
>>> quite likely only very few VMs would actually need this. To be honest
>>> I'd even be hesitant to accept a pointer addition here. Perhaps this
>>> should be a suitably sized, dynamically allocated array hanging off of
>>> struct domain?
>>
>> Sorry, I don't follow the dynamically allocated _array_ part. Could you
>> please give a small example of what you mean?
> 
> Have a pointer in struct arch_domain which gets populated only
> when needed, and if so by an array allocation with the array
> dimension being the maximum number of vCPU-s the domain may
> have. That way the memory overhead for non-monitored domains
> is a pointer per domain (as opposed to quite a bit more unused
> space per vCPU).

All clear now.


Thanks,
Razvan

[PATCH 5/5] xen: Write CR0, CR3 and CR4 in arch_set_info_guest()

[Razvan Cojocaru]

Arch_set_info_guest() doesn't set CR0, CR3 or CR4. Added code
that does that.

Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
---
 xen/arch/x86/domain.c             |    3 +++
 xen/arch/x86/hvm/emulate.c        |    6 +++---
 xen/arch/x86/hvm/hvm.c            |   30 ++++++++++++++++--------------
 xen/arch/x86/hvm/svm/nestedsvm.c  |   14 +++++++-------
 xen/arch/x86/hvm/vmx/vmx.c        |    2 +-
 xen/arch/x86/hvm/vmx/vvmx.c       |   12 ++++++------
 xen/include/asm-x86/hvm/support.h |    6 +++---
 7 files changed, 39 insertions(+), 34 deletions(-)

diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 1f1550e..fc2a64d 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -814,6 +814,9 @@ int arch_set_info_guest(
         for ( i = 0; i < ARRAY_SIZE(v->arch.debugreg); ++i )
             v->arch.debugreg[i] = c(debugreg[i]);
 
+        hvm_set_cr0(v, c(ctrlreg[0]), 0);
+        hvm_set_cr4(v, c(ctrlreg[4]), 0);
+        hvm_set_cr3(v, c(ctrlreg[3]), 0);
         hvm_set_info_guest(v);
 
         if ( is_hvm_vcpu(v) || v->is_initialised )
diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
index 566eee7..e48c34d 100644
--- a/xen/arch/x86/hvm/emulate.c
+++ b/xen/arch/x86/hvm/emulate.c
@@ -1265,14 +1265,14 @@ static int hvmemul_write_cr(
     switch ( reg )
     {
     case 0:
-        return hvm_set_cr0(val);
+        return hvm_set_cr0(current, val, 1);
     case 2:
         current->arch.hvm_vcpu.guest_cr[2] = val;
         return X86EMUL_OKAY;
     case 3:
-        return hvm_set_cr3(val);
+        return hvm_set_cr3(current, val, 1);
     case 4:
-        return hvm_set_cr4(val);
+        return hvm_set_cr4(current, val, 1);
     default:
         break;
     }
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index ed0ec9a..d236771 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -3074,13 +3074,13 @@ int hvm_mov_to_cr(unsigned int cr, unsigned int gpr)
     switch ( cr )
     {
     case 0:
-        return hvm_set_cr0(val);
+        return hvm_set_cr0(curr, val, 1);
 
     case 3:
-        return hvm_set_cr3(val);
+        return hvm_set_cr3(curr, val, 1);
 
     case 4:
-        return hvm_set_cr4(val);
+        return hvm_set_cr4(curr, val, 1);
 
     case 8:
         vlapic_set_reg(vcpu_vlapic(curr), APIC_TASKPRI, ((val & 0x0f) << 4));
@@ -3177,9 +3177,8 @@ static void hvm_update_cr(struct vcpu *v, unsigned int cr, unsigned long value)
     hvm_update_guest_cr(v, cr);
 }
 
-int hvm_set_cr0(unsigned long value)
+int hvm_set_cr0(struct vcpu *v, unsigned long value, bool_t with_vm_event)
 {
-    struct vcpu *v = current;
     struct domain *d = v->domain;
     unsigned long gfn, old_value = v->arch.hvm_vcpu.guest_cr[0];
     struct page_info *page;
@@ -3278,7 +3277,9 @@ int hvm_set_cr0(unsigned long value)
         hvm_funcs.handle_cd(v, value);
 
     hvm_update_cr(v, 0, value);
-    hvm_event_cr0(value, old_value);
+
+    if ( with_vm_event )
+        hvm_event_cr0(value, old_value);
 
     if ( (value ^ old_value) & X86_CR0_PG ) {
         if ( !nestedhvm_vmswitch_in_progress(v) && nestedhvm_vcpu_in_guestmode(v) )
@@ -3294,9 +3295,8 @@ int hvm_set_cr0(unsigned long value)
     return X86EMUL_EXCEPTION;
 }
 
-int hvm_set_cr3(unsigned long value)
+int hvm_set_cr3(struct vcpu *v, unsigned long value, bool_t with_vm_event)
 {
-    struct vcpu *v = current;
     struct page_info *page;
     unsigned long old;
 
@@ -3319,7 +3319,9 @@ int hvm_set_cr3(unsigned long value)
     old=v->arch.hvm_vcpu.guest_cr[3];
     v->arch.hvm_vcpu.guest_cr[3] = value;
     paging_update_cr3(v);
-    hvm_event_cr3(value, old);
+
+    if ( with_vm_event )
+        hvm_event_cr3(value, old);
     return X86EMUL_OKAY;
 
  bad_cr3:
@@ -3328,12 +3330,11 @@ int hvm_set_cr3(unsigned long value)
     return X86EMUL_UNHANDLEABLE;
 }
 
-int hvm_set_cr4(unsigned long value)
+int hvm_set_cr4(struct vcpu *v, unsigned long value, bool_t with_vm_event)
 {
-    struct vcpu *v = current;
     unsigned long old_cr;
 
-    if ( value & hvm_cr4_guest_reserved_bits(v, 0) )
+    if ( value & hvm_cr4_guest_reserved_bits(v, with_vm_event ? 0 : 1) )
     {
         HVM_DBG_LOG(DBG_LEVEL_1,
                     "Guest attempts to set reserved bit in CR4: %lx",
@@ -3359,7 +3360,8 @@ int hvm_set_cr4(unsigned long value)
         goto gpf;
     }
 
-    hvm_update_cr(v, 4, value);
+    if ( with_vm_event )
+        hvm_update_cr(v, 4, value);
     hvm_event_cr4(value, old_cr);
 
     /*
@@ -3824,7 +3826,7 @@ void hvm_task_switch(
         goto out;
 
 
-    if ( hvm_set_cr3(tss.cr3) )
+    if ( hvm_set_cr3(current, tss.cr3, 1) )
         goto out;
 
     regs->eip    = tss.eip;
diff --git a/xen/arch/x86/hvm/svm/nestedsvm.c b/xen/arch/x86/hvm/svm/nestedsvm.c
index be5797a..6f322f4 100644
--- a/xen/arch/x86/hvm/svm/nestedsvm.c
+++ b/xen/arch/x86/hvm/svm/nestedsvm.c
@@ -274,7 +274,7 @@ int nsvm_vcpu_hostrestore(struct vcpu *v, struct cpu_user_regs *regs)
 
     /* CR4 */
     v->arch.hvm_vcpu.guest_cr[4] = n1vmcb->_cr4;
-    rc = hvm_set_cr4(n1vmcb->_cr4);
+    rc = hvm_set_cr4(current, n1vmcb->_cr4, 1);
     if (rc != X86EMUL_OKAY)
         gdprintk(XENLOG_ERR, "hvm_set_cr4 failed, rc: %u\n", rc);
 
@@ -283,7 +283,7 @@ int nsvm_vcpu_hostrestore(struct vcpu *v, struct cpu_user_regs *regs)
         svm->ns_cr0, v->arch.hvm_vcpu.guest_cr[0]);
     v->arch.hvm_vcpu.guest_cr[0] = n1vmcb->_cr0 | X86_CR0_PE;
     n1vmcb->rflags &= ~X86_EFLAGS_VM;
-    rc = hvm_set_cr0(n1vmcb->_cr0 | X86_CR0_PE);
+    rc = hvm_set_cr0(current, n1vmcb->_cr0 | X86_CR0_PE, 1);
     if (rc != X86EMUL_OKAY)
         gdprintk(XENLOG_ERR, "hvm_set_cr0 failed, rc: %u\n", rc);
     svm->ns_cr0 = v->arch.hvm_vcpu.guest_cr[0];
@@ -309,7 +309,7 @@ int nsvm_vcpu_hostrestore(struct vcpu *v, struct cpu_user_regs *regs)
         v->arch.guest_table = pagetable_null();
         /* hvm_set_cr3() below sets v->arch.hvm_vcpu.guest_cr[3] for us. */
     }
-    rc = hvm_set_cr3(n1vmcb->_cr3);
+    rc = hvm_set_cr3(current, n1vmcb->_cr3, 1);
     if (rc != X86EMUL_OKAY)
         gdprintk(XENLOG_ERR, "hvm_set_cr3 failed, rc: %u\n", rc);
 
@@ -534,7 +534,7 @@ static int nsvm_vmcb_prepare4vmrun(struct vcpu *v, struct cpu_user_regs *regs)
 
     /* CR4 */
     v->arch.hvm_vcpu.guest_cr[4] = ns_vmcb->_cr4;
-    rc = hvm_set_cr4(ns_vmcb->_cr4);
+    rc = hvm_set_cr4(current, ns_vmcb->_cr4, 1);
     if (rc != X86EMUL_OKAY)
         gdprintk(XENLOG_ERR, "hvm_set_cr4 failed, rc: %u\n", rc);
 
@@ -542,7 +542,7 @@ static int nsvm_vmcb_prepare4vmrun(struct vcpu *v, struct cpu_user_regs *regs)
     svm->ns_cr0 = v->arch.hvm_vcpu.guest_cr[0];
     cr0 = nestedsvm_fpu_vmentry(svm->ns_cr0, ns_vmcb, n1vmcb, n2vmcb);
     v->arch.hvm_vcpu.guest_cr[0] = ns_vmcb->_cr0;
-    rc = hvm_set_cr0(cr0);
+    rc = hvm_set_cr0(current, cr0, 1);
     if (rc != X86EMUL_OKAY)
         gdprintk(XENLOG_ERR, "hvm_set_cr0 failed, rc: %u\n", rc);
 
@@ -558,7 +558,7 @@ static int nsvm_vmcb_prepare4vmrun(struct vcpu *v, struct cpu_user_regs *regs)
         nestedsvm_vmcb_set_nestedp2m(v, ns_vmcb, n2vmcb);
 
         /* hvm_set_cr3() below sets v->arch.hvm_vcpu.guest_cr[3] for us. */
-        rc = hvm_set_cr3(ns_vmcb->_cr3);
+        rc = hvm_set_cr3(current, ns_vmcb->_cr3, 1);
         if (rc != X86EMUL_OKAY)
             gdprintk(XENLOG_ERR, "hvm_set_cr3 failed, rc: %u\n", rc);
     } else if (paging_mode_hap(v->domain)) {
@@ -570,7 +570,7 @@ static int nsvm_vmcb_prepare4vmrun(struct vcpu *v, struct cpu_user_regs *regs)
          * we assume it intercepts page faults.
          */
         /* hvm_set_cr3() below sets v->arch.hvm_vcpu.guest_cr[3] for us. */
-        rc = hvm_set_cr3(ns_vmcb->_cr3);
+        rc = hvm_set_cr3(current, ns_vmcb->_cr3, 1);
         if (rc != X86EMUL_OKAY)
             gdprintk(XENLOG_ERR, "hvm_set_cr3 failed, rc: %u\n", rc);
     } else {
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 562507a..7309c71 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -2022,7 +2022,7 @@ static int vmx_cr_access(unsigned long exit_qualification)
                 (VMX_CONTROL_REG_ACCESS_DATA(exit_qualification) &
                  (X86_CR0_PE|X86_CR0_MP|X86_CR0_EM|X86_CR0_TS));
         HVMTRACE_LONG_1D(LMSW, value);
-        return hvm_set_cr0(value);
+        return hvm_set_cr0(current, value, 1);
     }
     default:
         BUG();
diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
index 01726d5..fb16fc6 100644
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -1048,9 +1048,9 @@ static void load_shadow_guest_state(struct vcpu *v)
 
     nvcpu->guest_cr[0] = __get_vvmcs(vvmcs, CR0_READ_SHADOW);
     nvcpu->guest_cr[4] = __get_vvmcs(vvmcs, CR4_READ_SHADOW);
-    hvm_set_cr0(__get_vvmcs(vvmcs, GUEST_CR0));
-    hvm_set_cr4(__get_vvmcs(vvmcs, GUEST_CR4));
-    hvm_set_cr3(__get_vvmcs(vvmcs, GUEST_CR3));
+    hvm_set_cr0(current, __get_vvmcs(vvmcs, GUEST_CR0), 1);
+    hvm_set_cr4(current, __get_vvmcs(vvmcs, GUEST_CR4), 1);
+    hvm_set_cr3(current, __get_vvmcs(vvmcs, GUEST_CR3), 1);
 
     control = __get_vvmcs(vvmcs, VM_ENTRY_CONTROLS);
     if ( control & VM_ENTRY_LOAD_GUEST_PAT )
@@ -1250,9 +1250,9 @@ static void load_vvmcs_host_state(struct vcpu *v)
         __vmwrite(vmcs_h2g_field[i].guest_field, r);
     }
 
-    hvm_set_cr0(__get_vvmcs(vvmcs, HOST_CR0));
-    hvm_set_cr4(__get_vvmcs(vvmcs, HOST_CR4));
-    hvm_set_cr3(__get_vvmcs(vvmcs, HOST_CR3));
+    hvm_set_cr0(current, __get_vvmcs(vvmcs, HOST_CR0), 1);
+    hvm_set_cr4(current, __get_vvmcs(vvmcs, HOST_CR4), 1);
+    hvm_set_cr3(current, __get_vvmcs(vvmcs, HOST_CR3), 1);
 
     control = __get_vvmcs(vvmcs, VM_EXIT_CONTROLS);
     if ( control & VM_EXIT_LOAD_HOST_PAT )
diff --git a/xen/include/asm-x86/hvm/support.h b/xen/include/asm-x86/hvm/support.h
index f55373e..625ad66 100644
--- a/xen/include/asm-x86/hvm/support.h
+++ b/xen/include/asm-x86/hvm/support.h
@@ -124,9 +124,9 @@ void hvm_shadow_handle_cd(struct vcpu *v, unsigned long value);
 
 /* These functions all return X86EMUL return codes. */
 int hvm_set_efer(uint64_t value);
-int hvm_set_cr0(unsigned long value);
-int hvm_set_cr3(unsigned long value);
-int hvm_set_cr4(unsigned long value);
+int hvm_set_cr0(struct vcpu *v, unsigned long value, bool_t with_vm_event);
+int hvm_set_cr3(struct vcpu *v, unsigned long value, bool_t with_vm_event);
+int hvm_set_cr4(struct vcpu *v, unsigned long value, bool_t with_vm_event);
 int hvm_msr_read_intercept(unsigned int msr, uint64_t *msr_content);
 int hvm_msr_write_intercept(
     unsigned int msr, uint64_t msr_content, bool_t event_only);
-- 
1.7.9.5

Re: [PATCH 5/5] xen: Write CR0, CR3 and CR4 in arch_set_info_guest()

[Boris Ostrovsky]

On 05/06/2015 01:12 PM, Razvan Cojocaru wrote:
> Arch_set_info_guest() doesn't set CR0, CR3 or CR4. Added code
> that does that.
>
> Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
> ---
>   xen/arch/x86/domain.c             |    3 +++
>   xen/arch/x86/hvm/emulate.c        |    6 +++---
>   xen/arch/x86/hvm/hvm.c            |   30 ++++++++++++++++--------------
>   xen/arch/x86/hvm/svm/nestedsvm.c  |   14 +++++++-------
>   xen/arch/x86/hvm/vmx/vmx.c        |    2 +-
>   xen/arch/x86/hvm/vmx/vvmx.c       |   12 ++++++------
>   xen/include/asm-x86/hvm/support.h |    6 +++---
>   7 files changed, 39 insertions(+), 34 deletions(-)

For SVM bits:

Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>

>
> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> index 1f1550e..fc2a64d 100644
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -814,6 +814,9 @@ int arch_set_info_guest(
>           for ( i = 0; i < ARRAY_SIZE(v->arch.debugreg); ++i )
>               v->arch.debugreg[i] = c(debugreg[i]);
>   
> +        hvm_set_cr0(v, c(ctrlreg[0]), 0);
> +        hvm_set_cr4(v, c(ctrlreg[4]), 0);
> +        hvm_set_cr3(v, c(ctrlreg[3]), 0);
>           hvm_set_info_guest(v);
>   
>           if ( is_hvm_vcpu(v) || v->is_initialised )
> diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
> index 566eee7..e48c34d 100644
> --- a/xen/arch/x86/hvm/emulate.c
> +++ b/xen/arch/x86/hvm/emulate.c
> @@ -1265,14 +1265,14 @@ static int hvmemul_write_cr(
>       switch ( reg )
>       {
>       case 0:
> -        return hvm_set_cr0(val);
> +        return hvm_set_cr0(current, val, 1);
>       case 2:
>           current->arch.hvm_vcpu.guest_cr[2] = val;
>           return X86EMUL_OKAY;
>       case 3:
> -        return hvm_set_cr3(val);
> +        return hvm_set_cr3(current, val, 1);
>       case 4:
> -        return hvm_set_cr4(val);
> +        return hvm_set_cr4(current, val, 1);
>       default:
>           break;
>       }
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index ed0ec9a..d236771 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -3074,13 +3074,13 @@ int hvm_mov_to_cr(unsigned int cr, unsigned int gpr)
>       switch ( cr )
>       {
>       case 0:
> -        return hvm_set_cr0(val);
> +        return hvm_set_cr0(curr, val, 1);
>   
>       case 3:
> -        return hvm_set_cr3(val);
> +        return hvm_set_cr3(curr, val, 1);
>   
>       case 4:
> -        return hvm_set_cr4(val);
> +        return hvm_set_cr4(curr, val, 1);
>   
>       case 8:
>           vlapic_set_reg(vcpu_vlapic(curr), APIC_TASKPRI, ((val & 0x0f) << 4));
> @@ -3177,9 +3177,8 @@ static void hvm_update_cr(struct vcpu *v, unsigned int cr, unsigned long value)
>       hvm_update_guest_cr(v, cr);
>   }
>   
> -int hvm_set_cr0(unsigned long value)
> +int hvm_set_cr0(struct vcpu *v, unsigned long value, bool_t with_vm_event)
>   {
> -    struct vcpu *v = current;
>       struct domain *d = v->domain;
>       unsigned long gfn, old_value = v->arch.hvm_vcpu.guest_cr[0];
>       struct page_info *page;
> @@ -3278,7 +3277,9 @@ int hvm_set_cr0(unsigned long value)
>           hvm_funcs.handle_cd(v, value);
>   
>       hvm_update_cr(v, 0, value);
> -    hvm_event_cr0(value, old_value);
> +
> +    if ( with_vm_event )
> +        hvm_event_cr0(value, old_value);
>   
>       if ( (value ^ old_value) & X86_CR0_PG ) {
>           if ( !nestedhvm_vmswitch_in_progress(v) && nestedhvm_vcpu_in_guestmode(v) )
> @@ -3294,9 +3295,8 @@ int hvm_set_cr0(unsigned long value)
>       return X86EMUL_EXCEPTION;
>   }
>   
> -int hvm_set_cr3(unsigned long value)
> +int hvm_set_cr3(struct vcpu *v, unsigned long value, bool_t with_vm_event)
>   {
> -    struct vcpu *v = current;
>       struct page_info *page;
>       unsigned long old;
>   
> @@ -3319,7 +3319,9 @@ int hvm_set_cr3(unsigned long value)
>       old=v->arch.hvm_vcpu.guest_cr[3];
>       v->arch.hvm_vcpu.guest_cr[3] = value;
>       paging_update_cr3(v);
> -    hvm_event_cr3(value, old);
> +
> +    if ( with_vm_event )
> +        hvm_event_cr3(value, old);
>       return X86EMUL_OKAY;
>   
>    bad_cr3:
> @@ -3328,12 +3330,11 @@ int hvm_set_cr3(unsigned long value)
>       return X86EMUL_UNHANDLEABLE;
>   }
>   
> -int hvm_set_cr4(unsigned long value)
> +int hvm_set_cr4(struct vcpu *v, unsigned long value, bool_t with_vm_event)
>   {
> -    struct vcpu *v = current;
>       unsigned long old_cr;
>   
> -    if ( value & hvm_cr4_guest_reserved_bits(v, 0) )
> +    if ( value & hvm_cr4_guest_reserved_bits(v, with_vm_event ? 0 : 1) )
>       {
>           HVM_DBG_LOG(DBG_LEVEL_1,
>                       "Guest attempts to set reserved bit in CR4: %lx",
> @@ -3359,7 +3360,8 @@ int hvm_set_cr4(unsigned long value)
>           goto gpf;
>       }
>   
> -    hvm_update_cr(v, 4, value);
> +    if ( with_vm_event )
> +        hvm_update_cr(v, 4, value);
>       hvm_event_cr4(value, old_cr);
>   
>       /*
> @@ -3824,7 +3826,7 @@ void hvm_task_switch(
>           goto out;
>   
>   
> -    if ( hvm_set_cr3(tss.cr3) )
> +    if ( hvm_set_cr3(current, tss.cr3, 1) )
>           goto out;
>   
>       regs->eip    = tss.eip;
> diff --git a/xen/arch/x86/hvm/svm/nestedsvm.c b/xen/arch/x86/hvm/svm/nestedsvm.c
> index be5797a..6f322f4 100644
> --- a/xen/arch/x86/hvm/svm/nestedsvm.c
> +++ b/xen/arch/x86/hvm/svm/nestedsvm.c
> @@ -274,7 +274,7 @@ int nsvm_vcpu_hostrestore(struct vcpu *v, struct cpu_user_regs *regs)
>   
>       /* CR4 */
>       v->arch.hvm_vcpu.guest_cr[4] = n1vmcb->_cr4;
> -    rc = hvm_set_cr4(n1vmcb->_cr4);
> +    rc = hvm_set_cr4(current, n1vmcb->_cr4, 1);
>       if (rc != X86EMUL_OKAY)
>           gdprintk(XENLOG_ERR, "hvm_set_cr4 failed, rc: %u\n", rc);
>   
> @@ -283,7 +283,7 @@ int nsvm_vcpu_hostrestore(struct vcpu *v, struct cpu_user_regs *regs)
>           svm->ns_cr0, v->arch.hvm_vcpu.guest_cr[0]);
>       v->arch.hvm_vcpu.guest_cr[0] = n1vmcb->_cr0 | X86_CR0_PE;
>       n1vmcb->rflags &= ~X86_EFLAGS_VM;
> -    rc = hvm_set_cr0(n1vmcb->_cr0 | X86_CR0_PE);
> +    rc = hvm_set_cr0(current, n1vmcb->_cr0 | X86_CR0_PE, 1);
>       if (rc != X86EMUL_OKAY)
>           gdprintk(XENLOG_ERR, "hvm_set_cr0 failed, rc: %u\n", rc);
>       svm->ns_cr0 = v->arch.hvm_vcpu.guest_cr[0];
> @@ -309,7 +309,7 @@ int nsvm_vcpu_hostrestore(struct vcpu *v, struct cpu_user_regs *regs)
>           v->arch.guest_table = pagetable_null();
>           /* hvm_set_cr3() below sets v->arch.hvm_vcpu.guest_cr[3] for us. */
>       }
> -    rc = hvm_set_cr3(n1vmcb->_cr3);
> +    rc = hvm_set_cr3(current, n1vmcb->_cr3, 1);
>       if (rc != X86EMUL_OKAY)
>           gdprintk(XENLOG_ERR, "hvm_set_cr3 failed, rc: %u\n", rc);
>   
> @@ -534,7 +534,7 @@ static int nsvm_vmcb_prepare4vmrun(struct vcpu *v, struct cpu_user_regs *regs)
>   
>       /* CR4 */
>       v->arch.hvm_vcpu.guest_cr[4] = ns_vmcb->_cr4;
> -    rc = hvm_set_cr4(ns_vmcb->_cr4);
> +    rc = hvm_set_cr4(current, ns_vmcb->_cr4, 1);
>       if (rc != X86EMUL_OKAY)
>           gdprintk(XENLOG_ERR, "hvm_set_cr4 failed, rc: %u\n", rc);
>   
> @@ -542,7 +542,7 @@ static int nsvm_vmcb_prepare4vmrun(struct vcpu *v, struct cpu_user_regs *regs)
>       svm->ns_cr0 = v->arch.hvm_vcpu.guest_cr[0];
>       cr0 = nestedsvm_fpu_vmentry(svm->ns_cr0, ns_vmcb, n1vmcb, n2vmcb);
>       v->arch.hvm_vcpu.guest_cr[0] = ns_vmcb->_cr0;
> -    rc = hvm_set_cr0(cr0);
> +    rc = hvm_set_cr0(current, cr0, 1);
>       if (rc != X86EMUL_OKAY)
>           gdprintk(XENLOG_ERR, "hvm_set_cr0 failed, rc: %u\n", rc);
>   
> @@ -558,7 +558,7 @@ static int nsvm_vmcb_prepare4vmrun(struct vcpu *v, struct cpu_user_regs *regs)
>           nestedsvm_vmcb_set_nestedp2m(v, ns_vmcb, n2vmcb);
>   
>           /* hvm_set_cr3() below sets v->arch.hvm_vcpu.guest_cr[3] for us. */
> -        rc = hvm_set_cr3(ns_vmcb->_cr3);
> +        rc = hvm_set_cr3(current, ns_vmcb->_cr3, 1);
>           if (rc != X86EMUL_OKAY)
>               gdprintk(XENLOG_ERR, "hvm_set_cr3 failed, rc: %u\n", rc);
>       } else if (paging_mode_hap(v->domain)) {
> @@ -570,7 +570,7 @@ static int nsvm_vmcb_prepare4vmrun(struct vcpu *v, struct cpu_user_regs *regs)
>            * we assume it intercepts page faults.
>            */
>           /* hvm_set_cr3() below sets v->arch.hvm_vcpu.guest_cr[3] for us. */
> -        rc = hvm_set_cr3(ns_vmcb->_cr3);
> +        rc = hvm_set_cr3(current, ns_vmcb->_cr3, 1);
>           if (rc != X86EMUL_OKAY)
>               gdprintk(XENLOG_ERR, "hvm_set_cr3 failed, rc: %u\n", rc);
>       } else {
> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> index 562507a..7309c71 100644
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -2022,7 +2022,7 @@ static int vmx_cr_access(unsigned long exit_qualification)
>                   (VMX_CONTROL_REG_ACCESS_DATA(exit_qualification) &
>                    (X86_CR0_PE|X86_CR0_MP|X86_CR0_EM|X86_CR0_TS));
>           HVMTRACE_LONG_1D(LMSW, value);
> -        return hvm_set_cr0(value);
> +        return hvm_set_cr0(current, value, 1);
>       }
>       default:
>           BUG();
> diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
> index 01726d5..fb16fc6 100644
> --- a/xen/arch/x86/hvm/vmx/vvmx.c
> +++ b/xen/arch/x86/hvm/vmx/vvmx.c
> @@ -1048,9 +1048,9 @@ static void load_shadow_guest_state(struct vcpu *v)
>   
>       nvcpu->guest_cr[0] = __get_vvmcs(vvmcs, CR0_READ_SHADOW);
>       nvcpu->guest_cr[4] = __get_vvmcs(vvmcs, CR4_READ_SHADOW);
> -    hvm_set_cr0(__get_vvmcs(vvmcs, GUEST_CR0));
> -    hvm_set_cr4(__get_vvmcs(vvmcs, GUEST_CR4));
> -    hvm_set_cr3(__get_vvmcs(vvmcs, GUEST_CR3));
> +    hvm_set_cr0(current, __get_vvmcs(vvmcs, GUEST_CR0), 1);
> +    hvm_set_cr4(current, __get_vvmcs(vvmcs, GUEST_CR4), 1);
> +    hvm_set_cr3(current, __get_vvmcs(vvmcs, GUEST_CR3), 1);
>   
>       control = __get_vvmcs(vvmcs, VM_ENTRY_CONTROLS);
>       if ( control & VM_ENTRY_LOAD_GUEST_PAT )
> @@ -1250,9 +1250,9 @@ static void load_vvmcs_host_state(struct vcpu *v)
>           __vmwrite(vmcs_h2g_field[i].guest_field, r);
>       }
>   
> -    hvm_set_cr0(__get_vvmcs(vvmcs, HOST_CR0));
> -    hvm_set_cr4(__get_vvmcs(vvmcs, HOST_CR4));
> -    hvm_set_cr3(__get_vvmcs(vvmcs, HOST_CR3));
> +    hvm_set_cr0(current, __get_vvmcs(vvmcs, HOST_CR0), 1);
> +    hvm_set_cr4(current, __get_vvmcs(vvmcs, HOST_CR4), 1);
> +    hvm_set_cr3(current, __get_vvmcs(vvmcs, HOST_CR3), 1);
>   
>       control = __get_vvmcs(vvmcs, VM_EXIT_CONTROLS);
>       if ( control & VM_EXIT_LOAD_HOST_PAT )
> diff --git a/xen/include/asm-x86/hvm/support.h b/xen/include/asm-x86/hvm/support.h
> index f55373e..625ad66 100644
> --- a/xen/include/asm-x86/hvm/support.h
> +++ b/xen/include/asm-x86/hvm/support.h
> @@ -124,9 +124,9 @@ void hvm_shadow_handle_cd(struct vcpu *v, unsigned long value);
>   
>   /* These functions all return X86EMUL return codes. */
>   int hvm_set_efer(uint64_t value);
> -int hvm_set_cr0(unsigned long value);
> -int hvm_set_cr3(unsigned long value);
> -int hvm_set_cr4(unsigned long value);
> +int hvm_set_cr0(struct vcpu *v, unsigned long value, bool_t with_vm_event);
> +int hvm_set_cr3(struct vcpu *v, unsigned long value, bool_t with_vm_event);
> +int hvm_set_cr4(struct vcpu *v, unsigned long value, bool_t with_vm_event);
>   int hvm_msr_read_intercept(unsigned int msr, uint64_t *msr_content);
>   int hvm_msr_write_intercept(
>       unsigned int msr, uint64_t msr_content, bool_t event_only);

Re: [PATCH 5/5] xen: Write CR0, CR3 and CR4 in arch_set_info_guest()

[Jan Beulich]

>>> On 06.05.15 at 19:12, <rcojocaru@bitdefender.com> wrote:
> Arch_set_info_guest() doesn't set CR0, CR3 or CR4. Added code
> that does that.

But you should also say a word on why this is needed, since things
worked fine so far without, and enabling the functions to run
outside of their own vCPU context is not immediately obviously
correct.

> -int hvm_set_cr0(unsigned long value)
> +int hvm_set_cr0(struct vcpu *v, unsigned long value, bool_t with_vm_event)
>  {
> -    struct vcpu *v = current;

This change is covered by neither the title nor the description, but
considering it's you who sends this likely is the meat of the change.
However, considering that the three calls you add to
arch_set_info_guest() pass this in as zero, I even more wonder why
what the title says is needed in the first place.

I further wonder whether you wouldn't want an event if and only
if v == current (in which case the flag parameter could be dropped).

> @@ -3328,12 +3330,11 @@ int hvm_set_cr3(unsigned long value)
>      return X86EMUL_UNHANDLEABLE;
>  }
>  
> -int hvm_set_cr4(unsigned long value)
> +int hvm_set_cr4(struct vcpu *v, unsigned long value, bool_t with_vm_event)
>  {
> -    struct vcpu *v = current;
>      unsigned long old_cr;
>  
> -    if ( value & hvm_cr4_guest_reserved_bits(v, 0) )
> +    if ( value & hvm_cr4_guest_reserved_bits(v, with_vm_event ? 0 : 1) )

Why does this depend on with_vm_event? And if indeed correct,
please simplify to just !with_vm_event.

> @@ -3359,7 +3360,8 @@ int hvm_set_cr4(unsigned long value)
>          goto gpf;
>      }
>  
> -    hvm_update_cr(v, 4, value);
> +    if ( with_vm_event )
> +        hvm_update_cr(v, 4, value);

Again, why? Such non-obvious conditionals need comments, or
people wanting to make future changes in these areas won't know
how to correctly change such code.

Jan

Re: [PATCH 5/5] xen: Write CR0, CR3 and CR4 in arch_set_info_guest()

[Razvan Cojocaru]

On 05/15/2015 06:57 PM, Jan Beulich wrote:
>>>> On 06.05.15 at 19:12, <rcojocaru@bitdefender.com> wrote:
>> Arch_set_info_guest() doesn't set CR0, CR3 or CR4. Added code
>> that does that.
> 
> But you should also say a word on why this is needed, since things
> worked fine so far without, and enabling the functions to run
> outside of their own vCPU context is not immediately obviously
> correct.

This is a way to undo malicious CR writes. This is achieved for MSR
writes with the deny vm_event response flag patch in this series, but
the CR events are being send after the actual write. In such cases,
while the VCPU is paused before I put a vm_response in the ring, I can
simply write the old value back.

I've brought up the issue in the past, and the consensus, IIRC, was that
I should not alter existing behaviour (post-write events) - so the
alternatives were either to add a new pre-write CR event (which seemed
messy), or this (which seemed less intrusive).

Of course, if it has now become acceptable to reconsider having the CR
vm_events consistently pre-write, the deny patch could be extended to them.

>> -int hvm_set_cr0(unsigned long value)
>> +int hvm_set_cr0(struct vcpu *v, unsigned long value, bool_t with_vm_event)
>>  {
>> -    struct vcpu *v = current;
> 
> This change is covered by neither the title nor the description, but
> considering it's you who sends this likely is the meat of the change.
> However, considering that the three calls you add to
> arch_set_info_guest() pass this in as zero, I even more wonder why
> what the title says is needed in the first place.
> 
> I further wonder whether you wouldn't want an event if and only
> if v == current (in which case the flag parameter could be dropped).

It just seemed useless to send out a vm_event in the case you mention,
since presumably the application setting them is very likely the same
one receiving the events (though, granted, it doesn't need to be). So in
that case, it would be pointless to notify itself that it has done what
it knows it's done.

>> @@ -3328,12 +3330,11 @@ int hvm_set_cr3(unsigned long value)
>>      return X86EMUL_UNHANDLEABLE;
>>  }
>>  
>> -int hvm_set_cr4(unsigned long value)
>> +int hvm_set_cr4(struct vcpu *v, unsigned long value, bool_t with_vm_event)
>>  {
>> -    struct vcpu *v = current;
>>      unsigned long old_cr;
>>  
>> -    if ( value & hvm_cr4_guest_reserved_bits(v, 0) )
>> +    if ( value & hvm_cr4_guest_reserved_bits(v, with_vm_event ? 0 : 1) )
> 
> Why does this depend on with_vm_event? And if indeed correct,
> please simplify to just !with_vm_event.

hvm_cr4_guest_reserved_bits(v, 1) has an ASSERT(v != current) that
crashes the hypervisor in debug mode (surely for a very good reason). If
vm_event is true, then v != current, so there I've tried to avert the crash.


Thanks for the review,
Razvan

Re: [PATCH 5/5] xen: Write CR0, CR3 and CR4 in arch_set_info_guest()

[Andrew Cooper]

On 15/05/2015 21:45, Razvan Cojocaru wrote:
> On 05/15/2015 06:57 PM, Jan Beulich wrote:
>>>>> On 06.05.15 at 19:12, <rcojocaru@bitdefender.com> wrote:
>>> Arch_set_info_guest() doesn't set CR0, CR3 or CR4. Added code
>>> that does that.
>> But you should also say a word on why this is needed, since things
>> worked fine so far without, and enabling the functions to run
>> outside of their own vCPU context is not immediately obviously
>> correct.
> This is a way to undo malicious CR writes. This is achieved for MSR
> writes with the deny vm_event response flag patch in this series, but
> the CR events are being send after the actual write. In such cases,
> while the VCPU is paused before I put a vm_response in the ring, I can
> simply write the old value back.
>
> I've brought up the issue in the past, and the consensus, IIRC, was that
> I should not alter existing behaviour (post-write events) - so the
> alternatives were either to add a new pre-write CR event (which seemed
> messy), or this (which seemed less intrusive).

I always found it curious that some events where post and some pre.

I though I suggested (or at least considered suggesting) at one point
that it would be sensible to select pre/post for event notification. 
Pre comes with an ability to modify the outcome, whereas post is simply
a notification that something happened.

Once again, if you deem this sensible then now is very definitely the
time to do something about it :)

> Of course, if it has now become acceptable to reconsider having the CR
> vm_events consistently pre-write, the deny patch could be extended to them.
>
>>> -int hvm_set_cr0(unsigned long value)
>>> +int hvm_set_cr0(struct vcpu *v, unsigned long value, bool_t with_vm_event)
>>>  {
>>> -    struct vcpu *v = current;
>> This change is covered by neither the title nor the description, but
>> considering it's you who sends this likely is the meat of the change.
>> However, considering that the three calls you add to
>> arch_set_info_guest() pass this in as zero, I even more wonder why
>> what the title says is needed in the first place.
>>
>> I further wonder whether you wouldn't want an event if and only
>> if v == current (in which case the flag parameter could be dropped).
> It just seemed useless to send out a vm_event in the case you mention,
> since presumably the application setting them is very likely the same
> one receiving the events (though, granted, it doesn't need to be). So in
> that case, it would be pointless to notify itself that it has done what
> it knows it's done.
>
>>> @@ -3328,12 +3330,11 @@ int hvm_set_cr3(unsigned long value)
>>>      return X86EMUL_UNHANDLEABLE;
>>>  }
>>>  
>>> -int hvm_set_cr4(unsigned long value)
>>> +int hvm_set_cr4(struct vcpu *v, unsigned long value, bool_t with_vm_event)
>>>  {
>>> -    struct vcpu *v = current;
>>>      unsigned long old_cr;
>>>  
>>> -    if ( value & hvm_cr4_guest_reserved_bits(v, 0) )
>>> +    if ( value & hvm_cr4_guest_reserved_bits(v, with_vm_event ? 0 : 1) )
>> Why does this depend on with_vm_event? And if indeed correct,
>> please simplify to just !with_vm_event.
> hvm_cr4_guest_reserved_bits(v, 1) has an ASSERT(v != current) that
> crashes the hypervisor in debug mode (surely for a very good reason). If
> vm_event is true, then v != current, so there I've tried to avert the crash.

(As a side note, attempting to work around an ASSERT is never a valid
option. There will be a reason why it is there in the first place, and
that reason will most likely invalidate whatever train of logic
attempted to work around it in the first place.  The other option is
that the ASSERT is wrong, in which case it should be removed.)

This ASSERT is muddled up with the cpuid handling.  The second parameter
flips between querying the domain cpuid policy, and the host cpuid
policy, as a stopgap solution for some validity checking on domain restore.

I am in the middle (well - more like just started, too many high
priority interrupts) of rewriting cpuid handling from scratch in an
effort to make heterogeneous feature levelling work in a moderately sane
fashion.

As it stands, altering this 0 to some function of with_vm_event is not
valid, as it will change the calculation of which CR4 bits are permitted
to be modified.  Unfortunately, my best suggestion here is to wire up as
much logic as you can, and leave it short circuited with a /*TODO fixme
when domain cpuid handling works properly */ comment.

~Andrew

Re: [PATCH 5/5] xen: Write CR0, CR3 and CR4 in arch_set_info_guest()

[Razvan Cojocaru]

On 05/16/2015 02:13 AM, Andrew Cooper wrote:
> On 15/05/2015 21:45, Razvan Cojocaru wrote:
>> On 05/15/2015 06:57 PM, Jan Beulich wrote:
>>>>>> On 06.05.15 at 19:12, <rcojocaru@bitdefender.com> wrote:
>>>> Arch_set_info_guest() doesn't set CR0, CR3 or CR4. Added code
>>>> that does that.
>>> But you should also say a word on why this is needed, since things
>>> worked fine so far without, and enabling the functions to run
>>> outside of their own vCPU context is not immediately obviously
>>> correct.
>> This is a way to undo malicious CR writes. This is achieved for MSR
>> writes with the deny vm_event response flag patch in this series, but
>> the CR events are being send after the actual write. In such cases,
>> while the VCPU is paused before I put a vm_response in the ring, I can
>> simply write the old value back.
>>
>> I've brought up the issue in the past, and the consensus, IIRC, was that
>> I should not alter existing behaviour (post-write events) - so the
>> alternatives were either to add a new pre-write CR event (which seemed
>> messy), or this (which seemed less intrusive).
> 
> I always found it curious that some events where post and some pre.
> 
> I though I suggested (or at least considered suggesting) at one point
> that it would be sensible to select pre/post for event notification. 
> Pre comes with an ability to modify the outcome, whereas post is simply
> a notification that something happened.
> 
> Once again, if you deem this sensible then now is very definitely the
> time to do something about it :)

You did suggest it here:

http://lists.xen.org/archives/html/xen-devel/2014-10/msg00434.html

In fact, that thread spawned the whole vm_event redesign discussion:

http://lists.xen.org/archives/html/xen-devel/2014-10/threads.html#00438

I took the suggestion to mean at the time that we should have something
like EVENT_CR3_PRE and EVENT_CR3_POST, where basically all we needed was
for all events for which this applicable to be pre-write events. IMHO
that's simpler and sufficient: just send out an event when you know
that, unless you deny it, simply responding to it / unblocking the VCPU
will perform the write. So you know that the write is about to happen,
and by no denying it, that it will happen (or at least, that it's
extremely likely to happen - since some HV check can still fail somewhere).

So, again, just IMHO, the simpler modification would just be to turn all
events where this is applicable into pre-write events.

>> Of course, if it has now become acceptable to reconsider having the CR
>> vm_events consistently pre-write, the deny patch could be extended to them.
>>
>>>> -int hvm_set_cr0(unsigned long value)
>>>> +int hvm_set_cr0(struct vcpu *v, unsigned long value, bool_t with_vm_event)
>>>>  {
>>>> -    struct vcpu *v = current;
>>> This change is covered by neither the title nor the description, but
>>> considering it's you who sends this likely is the meat of the change.
>>> However, considering that the three calls you add to
>>> arch_set_info_guest() pass this in as zero, I even more wonder why
>>> what the title says is needed in the first place.
>>>
>>> I further wonder whether you wouldn't want an event if and only
>>> if v == current (in which case the flag parameter could be dropped).
>> It just seemed useless to send out a vm_event in the case you mention,
>> since presumably the application setting them is very likely the same
>> one receiving the events (though, granted, it doesn't need to be). So in
>> that case, it would be pointless to notify itself that it has done what
>> it knows it's done.
>>
>>>> @@ -3328,12 +3330,11 @@ int hvm_set_cr3(unsigned long value)
>>>>      return X86EMUL_UNHANDLEABLE;
>>>>  }
>>>>  
>>>> -int hvm_set_cr4(unsigned long value)
>>>> +int hvm_set_cr4(struct vcpu *v, unsigned long value, bool_t with_vm_event)
>>>>  {
>>>> -    struct vcpu *v = current;
>>>>      unsigned long old_cr;
>>>>  
>>>> -    if ( value & hvm_cr4_guest_reserved_bits(v, 0) )
>>>> +    if ( value & hvm_cr4_guest_reserved_bits(v, with_vm_event ? 0 : 1) )
>>> Why does this depend on with_vm_event? And if indeed correct,
>>> please simplify to just !with_vm_event.
>> hvm_cr4_guest_reserved_bits(v, 1) has an ASSERT(v != current) that
>> crashes the hypervisor in debug mode (surely for a very good reason). If
>> vm_event is true, then v != current, so there I've tried to avert the crash.
> 
> (As a side note, attempting to work around an ASSERT is never a valid
> option. There will be a reason why it is there in the first place, and
> that reason will most likely invalidate whatever train of logic
> attempted to work around it in the first place.  The other option is
> that the ASSERT is wrong, in which case it should be removed.)
> 
> This ASSERT is muddled up with the cpuid handling.  The second parameter
> flips between querying the domain cpuid policy, and the host cpuid
> policy, as a stopgap solution for some validity checking on domain restore.
> 
> I am in the middle (well - more like just started, too many high
> priority interrupts) of rewriting cpuid handling from scratch in an
> effort to make heterogeneous feature levelling work in a moderately sane
> fashion.
> 
> As it stands, altering this 0 to some function of with_vm_event is not
> valid, as it will change the calculation of which CR4 bits are permitted
> to be modified.  Unfortunately, my best suggestion here is to wire up as
> much logic as you can, and leave it short circuited with a /*TODO fixme
> when domain cpuid handling works properly */ comment.

Thank you, I was hoping that the ASSERT code will get discussed during
the review. I certainly didn't think that the ASSERT was wrong, but my
code did throw a new path to setting CR4 into the mix, so a discussion
was certainly needed here.

Do you mean wire up as much logic as possible inside
hvm_cr4_guest_reserved_bits(), or simply changing the condition to
if ( v == current && value & hvm_cr4_guest_reserved_bits(v, 0) ) ?


Thanks,
Razvan

Re: [PATCH 5/5] xen: Write CR0, CR3 and CR4 in arch_set_info_guest()

[Tamas K Lengyel]

>
> I took the suggestion to mean at the time that we should have something
> like EVENT_CR3_PRE and EVENT_CR3_POST, where basically all we needed was
> for all events for which this applicable to be pre-write events. IMHO
> that's simpler and sufficient: just send out an event when you know
> that, unless you deny it, simply responding to it / unblocking the VCPU
> will perform the write. So you know that the write is about to happen,
> and by no denying it, that it will happen (or at least, that it's
> extremely likely to happen - since some HV check can still fail somewhere).
>
> So, again, just IMHO, the simpler modification would just be to turn all
> events where this is applicable into pre-write events.

Isn't the event from the guest's perspective guaranteed to be
pre-write already? IMHO there is not much point in having two distinct
event types (PRE/POST). I'm not particularly happy with the "deny
change" flag idea either. Why not let the user specify what value he
wants to set the register to in such a case? It could the one that was
already there (old_value) in "deny change" style, but it might as well
be something else. I'm thinking we could keep the existing vm_event
hooks in place and simply let the vm_event response specify the value
the register should be set to (in the new_value field).

Tamas

Re: [PATCH 5/5] xen: Write CR0, CR3 and CR4 in arch_set_info_guest()

[Razvan Cojocaru]

On 05/17/2015 09:32 PM, Tamas K Lengyel wrote:
>>
>> I took the suggestion to mean at the time that we should have something
>> like EVENT_CR3_PRE and EVENT_CR3_POST, where basically all we needed was
>> for all events for which this applicable to be pre-write events. IMHO
>> that's simpler and sufficient: just send out an event when you know
>> that, unless you deny it, simply responding to it / unblocking the VCPU
>> will perform the write. So you know that the write is about to happen,
>> and by no denying it, that it will happen (or at least, that it's
>> extremely likely to happen - since some HV check can still fail somewhere).
>>
>> So, again, just IMHO, the simpler modification would just be to turn all
>> events where this is applicable into pre-write events.
> 
> Isn't the event from the guest's perspective guaranteed to be
> pre-write already? IMHO there is not much point in having two distinct

The CR events are not pre-write:

3289 int hvm_set_cr3(unsigned long value)
3290 {
3291     struct vcpu *v = current;
3292     struct page_info *page;
3293     unsigned long old;
3294
3295     if ( hvm_paging_enabled(v) && !paging_mode_hap(v->domain) &&
3296          (value != v->arch.hvm_vcpu.guest_cr[3]) )
3297     {
3298         /* Shadow-mode CR3 change. Check PDBR and update refcounts. */
3299         HVM_DBG_LOG(DBG_LEVEL_VMMU, "CR3 value = %lx", value);
3300         page = get_page_from_gfn(v->domain, value >> PAGE_SHIFT,
3301                                  NULL, P2M_ALLOC);
3302         if ( !page )
3303             goto bad_cr3;
3304
3305         put_page(pagetable_get_page(v->arch.guest_table));
3306         v->arch.guest_table = pagetable_from_page(page);
3307
3308         HVM_DBG_LOG(DBG_LEVEL_VMMU, "Update CR3 value = %lx", value);
3309     }
3310
3311     old=v->arch.hvm_vcpu.guest_cr[3];
3312     v->arch.hvm_vcpu.guest_cr[3] = value;
3313     paging_update_cr3(v);
3314     hvm_event_cr(VM_EVENT_X86_CR3, value, old);
3315     return X86EMUL_OKAY;
3316
3317  bad_cr3:
3318     gdprintk(XENLOG_ERR, "Invalid CR3\n");
3319     domain_crash(v->domain);
3320     return X86EMUL_UNHANDLEABLE;
3321 }

The line numbers assume my patch has been applied, but the only relevant
change here is that hvm_event_cr(VM_EVENT_X86_CR3, value, old); replaced
the old hvm_event_cr3(value, old); at line 3314.

As you can see, first CR3 is being updated, and the events is being sent
afterwards. This applies to CR4, etc. also.

> event types (PRE/POST). I'm not particularly happy with the "deny
> change" flag idea either. Why not let the user specify what value he
> wants to set the register to in such a case? It could the one that was
> already there (old_value) in "deny change" style, but it might as well
> be something else. I'm thinking we could keep the existing vm_event
> hooks in place and simply let the vm_event response specify the value
> the register should be set to (in the new_value field).

You mean not have a distinct DENY vm_event response flag, but if rsp's
new_value != req's new value set that one? Sure, that'll work, but it's
less explicit, and thus, IMHO, more error-prone (it's easy for a
vm_event consumer to just create the response on the stack, forget (or
not know) that this might happen, and have the guest just write garbage
to some register).


Thanks,
Razvan

Re: [PATCH 5/5] xen: Write CR0, CR3 and CR4 in arch_set_info_guest()

[Tamas K Lengyel]

>> Isn't the event from the guest's perspective guaranteed to be
>> pre-write already? IMHO there is not much point in having two distinct
>
> The CR events are not pre-write:
>
> 3289 int hvm_set_cr3(unsigned long value)
> 3290 {
> 3291     struct vcpu *v = current;
> 3292     struct page_info *page;
> 3293     unsigned long old;
> 3294
> 3295     if ( hvm_paging_enabled(v) && !paging_mode_hap(v->domain) &&
> 3296          (value != v->arch.hvm_vcpu.guest_cr[3]) )
> 3297     {
> 3298         /* Shadow-mode CR3 change. Check PDBR and update refcounts. */
> 3299         HVM_DBG_LOG(DBG_LEVEL_VMMU, "CR3 value = %lx", value);
> 3300         page = get_page_from_gfn(v->domain, value >> PAGE_SHIFT,
> 3301                                  NULL, P2M_ALLOC);
> 3302         if ( !page )
> 3303             goto bad_cr3;
> 3304
> 3305         put_page(pagetable_get_page(v->arch.guest_table));
> 3306         v->arch.guest_table = pagetable_from_page(page);
> 3307
> 3308         HVM_DBG_LOG(DBG_LEVEL_VMMU, "Update CR3 value = %lx", value);
> 3309     }
> 3310
> 3311     old=v->arch.hvm_vcpu.guest_cr[3];
> 3312     v->arch.hvm_vcpu.guest_cr[3] = value;
> 3313     paging_update_cr3(v);
> 3314     hvm_event_cr(VM_EVENT_X86_CR3, value, old);
> 3315     return X86EMUL_OKAY;
> 3316
> 3317  bad_cr3:
> 3318     gdprintk(XENLOG_ERR, "Invalid CR3\n");
> 3319     domain_crash(v->domain);
> 3320     return X86EMUL_UNHANDLEABLE;
> 3321 }
>
> The line numbers assume my patch has been applied, but the only relevant
> change here is that hvm_event_cr(VM_EVENT_X86_CR3, value, old); replaced
> the old hvm_event_cr3(value, old); at line 3314.
>
> As you can see, first CR3 is being updated, and the events is being sent
> afterwards. This applies to CR4, etc. also.

Well, from the guest's perspective it is still pre-write as the guest
hasn't executed yet with the new CR value set. It's post-write only
from the hypervisor's perspective right now. So where we actually send
the event is somewhat arbitrary IMHO as the response processing would
have to call hvm_set_cr3 again if the deny/change_value flag is set.

>> event types (PRE/POST). I'm not particularly happy with the "deny
>> change" flag idea either. Why not let the user specify what value he
>> wants to set the register to in such a case? It could the one that was
>> already there (old_value) in "deny change" style, but it might as well
>> be something else. I'm thinking we could keep the existing vm_event
>> hooks in place and simply let the vm_event response specify the value
>> the register should be set to (in the new_value field).
>
> You mean not have a distinct DENY vm_event response flag, but if rsp's
> new_value != req's new value set that one? Sure, that'll work, but it's
> less explicit, and thus, IMHO, more error-prone (it's easy for a
> vm_event consumer to just create the response on the stack, forget (or
> not know) that this might happen, and have the guest just write garbage
> to some register).

You can have a response flag for it to tell Xen to look at the
new_value. What I meant is why restrict the feature to be DENY only.
You might as well let the user choose the value he wants to see in the
register. The flag would still be necessary to be defined for the
response as to avoid accidentally changing new_value without realizing
the consequence of it.

Cheers,
Tamas

>
>
> Thanks,
> Razvan

Re: [PATCH 5/5] xen: Write CR0, CR3 and CR4 in arch_set_info_guest()

[Jan Beulich]

>>> On 19.05.15 at 12:14, <tamas.lengyel@zentific.com> wrote:
> You can have a response flag for it to tell Xen to look at the
> new_value. What I meant is why restrict the feature to be DENY only.
> You might as well let the user choose the value he wants to see in the
> register.

Hmm, I don't think allowing the use to chose arbitrary values here
is going to be the right direction.

Jan

Re: [PATCH 5/5] xen: Write CR0, CR3 and CR4 in arch_set_info_guest()

[Tamas K Lengyel]

On Tue, May 19, 2015 at 12:31 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 19.05.15 at 12:14, <tamas.lengyel@zentific.com> wrote:
>> You can have a response flag for it to tell Xen to look at the
>> new_value. What I meant is why restrict the feature to be DENY only.
>> You might as well let the user choose the value he wants to see in the
>> register.
>
> Hmm, I don't think allowing the use to chose arbitrary values here
> is going to be the right direction.
>
> Jan

(This time adding xen-devel)

Care to elaborate why it would be a problem? The user would still have
to have knowledge about what value he sets the register as an
"arbitrary" value will crash the system most probably.

Tamas

Re: [PATCH 5/5] xen: Write CR0, CR3 and CR4 in arch_set_info_guest()

[Jan Beulich]

>>> On 19.05.15 at 12:45, <tamas.lengyel@zentific.com> wrote:
> On Tue, May 19, 2015 at 12:31 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>> On 19.05.15 at 12:14, <tamas.lengyel@zentific.com> wrote:
>>> You can have a response flag for it to tell Xen to look at the
>>> new_value. What I meant is why restrict the feature to be DENY only.
>>> You might as well let the user choose the value he wants to see in the
>>> register.
>>
>> Hmm, I don't think allowing the use to chose arbitrary values here
>> is going to be the right direction.
> 
> Care to elaborate why it would be a problem? The user would still have
> to have knowledge about what value he sets the register as an
> "arbitrary" value will crash the system most probably.

Understood, but even that already seems too much of an intrusion
into the guest. And then I'm worried about this introducing subtle
security issues (perhaps due to bypassing some consistency checks),
but this of course can be got under control if such overrides were to
be injected strictly only at places where guest values are being used
as inputs anyway.

Jan

Re: [PATCH 5/5] xen: Write CR0, CR3 and CR4 in arch_set_info_guest()

[Tamas K Lengyel]

On Tue, May 19, 2015 at 3:45 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 19.05.15 at 12:45, <tamas.lengyel@zentific.com> wrote:
>> On Tue, May 19, 2015 at 12:31 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>> On 19.05.15 at 12:14, <tamas.lengyel@zentific.com> wrote:
>>>> You can have a response flag for it to tell Xen to look at the
>>>> new_value. What I meant is why restrict the feature to be DENY only.
>>>> You might as well let the user choose the value he wants to see in the
>>>> register.
>>>
>>> Hmm, I don't think allowing the use to chose arbitrary values here
>>> is going to be the right direction.
>>
>> Care to elaborate why it would be a problem? The user would still have
>> to have knowledge about what value he sets the register as an
>> "arbitrary" value will crash the system most probably.
>
> Understood, but even that already seems too much of an intrusion
> into the guest. And then I'm worried about this introducing subtle
> security issues (perhaps due to bypassing some consistency checks),
> but this of course can be got under control if such overrides were to
> be injected strictly only at places where guest values are being used
> as inputs anyway.
>
> Jan

I guess for now it's fine to have only the "deny" method as that would
make the most sense for CR0/CR4 in case something tries to disable
protective features like SMEP/SMAP. This extra feature for setting
event-listener provided value for the register in response to a write
event would probably make sense only for CR3 (if for any). If it's too
much of a change to how things are wired up right now we can skip it
and revisit it in the future when the need for it arises.

Cheers,
Tamas

Re: [PATCH 5/5] xen: Write CR0, CR3 and CR4 in arch_set_info_guest()

[Razvan Cojocaru]

On 05/19/2015 01:14 PM, Tamas K Lengyel wrote:
>>> Isn't the event from the guest's perspective guaranteed to be
>>> pre-write already? IMHO there is not much point in having two distinct
>>
>> The CR events are not pre-write:
>>
>> 3289 int hvm_set_cr3(unsigned long value)
>> 3290 {
>> 3291     struct vcpu *v = current;
>> 3292     struct page_info *page;
>> 3293     unsigned long old;
>> 3294
>> 3295     if ( hvm_paging_enabled(v) && !paging_mode_hap(v->domain) &&
>> 3296          (value != v->arch.hvm_vcpu.guest_cr[3]) )
>> 3297     {
>> 3298         /* Shadow-mode CR3 change. Check PDBR and update refcounts. */
>> 3299         HVM_DBG_LOG(DBG_LEVEL_VMMU, "CR3 value = %lx", value);
>> 3300         page = get_page_from_gfn(v->domain, value >> PAGE_SHIFT,
>> 3301                                  NULL, P2M_ALLOC);
>> 3302         if ( !page )
>> 3303             goto bad_cr3;
>> 3304
>> 3305         put_page(pagetable_get_page(v->arch.guest_table));
>> 3306         v->arch.guest_table = pagetable_from_page(page);
>> 3307
>> 3308         HVM_DBG_LOG(DBG_LEVEL_VMMU, "Update CR3 value = %lx", value);
>> 3309     }
>> 3310
>> 3311     old=v->arch.hvm_vcpu.guest_cr[3];
>> 3312     v->arch.hvm_vcpu.guest_cr[3] = value;
>> 3313     paging_update_cr3(v);
>> 3314     hvm_event_cr(VM_EVENT_X86_CR3, value, old);
>> 3315     return X86EMUL_OKAY;
>> 3316
>> 3317  bad_cr3:
>> 3318     gdprintk(XENLOG_ERR, "Invalid CR3\n");
>> 3319     domain_crash(v->domain);
>> 3320     return X86EMUL_UNHANDLEABLE;
>> 3321 }
>>
>> The line numbers assume my patch has been applied, but the only relevant
>> change here is that hvm_event_cr(VM_EVENT_X86_CR3, value, old); replaced
>> the old hvm_event_cr3(value, old); at line 3314.
>>
>> As you can see, first CR3 is being updated, and the events is being sent
>> afterwards. This applies to CR4, etc. also.
> 
> Well, from the guest's perspective it is still pre-write as the guest
> hasn't executed yet with the new CR value set. It's post-write only
> from the hypervisor's perspective right now. So where we actually send
> the event is somewhat arbitrary IMHO as the response processing would
> have to call hvm_set_cr3 again if the deny/change_value flag is set.

I beg to differ, the time when that control register is set in the HV
makes a world of difference.

For one, you can see in the code above that changes to the guest _are_
being made before sending out the event (see also the function that sets
CR4). Any changes to the guest during hvm_set_crX() will be seen by the
vm_event handler application, which can (and does, in our case) query
the guest state. So, whatever consequences a CR write might trigger (not
least of which is the actual new register value) will be immediately
visible to the monitoring application.

Secondly, if the vm_event client decides that the write is not wanted,
it has to issue a call to the HV to change the value back to the old
one. This implies an extra context switch. When you're dealing with
_many_ events, an extra context switch can make or break your client
application. (This is why we've submitted the initial series patch that
fills the registers and puts them in the vm_event request - that could
have been done with an extra libxc call from the application, but it
made a world of difference to not need to do that.)

As for the DENY thing, the idea with the MSR write deny patch is that
the work needed to set the MSR is done only once, in hvm_do_resume().
For the vm_event-enabled case, hvm_msr_write_intercept() becomes
basically a no-op, so things are _not_ being set twice.

>>> event types (PRE/POST). I'm not particularly happy with the "deny
>>> change" flag idea either. Why not let the user specify what value he
>>> wants to set the register to in such a case? It could the one that was
>>> already there (old_value) in "deny change" style, but it might as well
>>> be something else. I'm thinking we could keep the existing vm_event
>>> hooks in place and simply let the vm_event response specify the value
>>> the register should be set to (in the new_value field).
>>
>> You mean not have a distinct DENY vm_event response flag, but if rsp's
>> new_value != req's new value set that one? Sure, that'll work, but it's
>> less explicit, and thus, IMHO, more error-prone (it's easy for a
>> vm_event consumer to just create the response on the stack, forget (or
>> not know) that this might happen, and have the guest just write garbage
>> to some register).
> 
> You can have a response flag for it to tell Xen to look at the
> new_value. What I meant is why restrict the feature to be DENY only.
> You might as well let the user choose the value he wants to see in the
> register. The flag would still be necessary to be defined for the
> response as to avoid accidentally changing new_value without realizing
> the consequence of it.

Would you find that useful? All the use cases we have only involve not
allowing a malicious write to go through, as previously discussed here:

http://lists.xen.org/archives/html/xen-devel/2014-10/msg00254.html

It could, of course, be done, but we haven't needed anything like that
so far. Maybe there are valid use cases for that though, I'm certainly
open to discussion.


Thanks,
Razvan

Re: [PATCH 5/5] xen: Write CR0, CR3 and CR4 in arch_set_info_guest()

[Jan Beulich]

>>> On 15.05.15 at 22:45, <rcojocaru@bitdefender.com> wrote:
> On 05/15/2015 06:57 PM, Jan Beulich wrote:
>>>>> On 06.05.15 at 19:12, <rcojocaru@bitdefender.com> wrote:
>>> Arch_set_info_guest() doesn't set CR0, CR3 or CR4. Added code
>>> that does that.
>> 
>> But you should also say a word on why this is needed, since things
>> worked fine so far without, and enabling the functions to run
>> outside of their own vCPU context is not immediately obviously
>> correct.
> 
> This is a way to undo malicious CR writes. This is achieved for MSR
> writes with the deny vm_event response flag patch in this series, but
> the CR events are being send after the actual write. In such cases,
> while the VCPU is paused before I put a vm_response in the ring, I can
> simply write the old value back.
> 
> I've brought up the issue in the past, and the consensus, IIRC, was that
> I should not alter existing behaviour (post-write events) - so the
> alternatives were either to add a new pre-write CR event (which seemed
> messy), or this (which seemed less intrusive).
> 
> Of course, if it has now become acceptable to reconsider having the CR
> vm_events consistently pre-write, the deny patch could be extended to them.

Considering that in the reply to Andrew's response you already
pointed at where a suggestion towards consistent pre events was
made, it would have helped if you identified where this was advised
against before. It certainly seems sensible to treat MSR and CR
writes in a consistent fashion.

>>> -int hvm_set_cr0(unsigned long value)
>>> +int hvm_set_cr0(struct vcpu *v, unsigned long value, bool_t with_vm_event)
>>>  {
>>> -    struct vcpu *v = current;
>> 
>> This change is covered by neither the title nor the description, but
>> considering it's you who sends this likely is the meat of the change.
>> However, considering that the three calls you add to
>> arch_set_info_guest() pass this in as zero, I even more wonder why
>> what the title says is needed in the first place.
>> 
>> I further wonder whether you wouldn't want an event if and only
>> if v == current (in which case the flag parameter could be dropped).
> 
> It just seemed useless to send out a vm_event in the case you mention,
> since presumably the application setting them is very likely the same
> one receiving the events (though, granted, it doesn't need to be). So in
> that case, it would be pointless to notify itself that it has done what
> it knows it's done.

If the setting is being done by a monitor VM, I would suppose
v != current, and hence - along the lines above - no need for an
event. Whereas when v == current, the setting would be done
by the VM itself, and hence an event should always be delivered.

Jan

Re: [PATCH 5/5] xen: Write CR0, CR3 and CR4 in arch_set_info_guest()

[Razvan Cojocaru]

On 05/18/2015 10:27 AM, Jan Beulich wrote:
>>>> On 15.05.15 at 22:45, <rcojocaru@bitdefender.com> wrote:
>> On 05/15/2015 06:57 PM, Jan Beulich wrote:
>>>>>> On 06.05.15 at 19:12, <rcojocaru@bitdefender.com> wrote:
>>>> Arch_set_info_guest() doesn't set CR0, CR3 or CR4. Added code
>>>> that does that.
>>>
>>> But you should also say a word on why this is needed, since things
>>> worked fine so far without, and enabling the functions to run
>>> outside of their own vCPU context is not immediately obviously
>>> correct.
>>
>> This is a way to undo malicious CR writes. This is achieved for MSR
>> writes with the deny vm_event response flag patch in this series, but
>> the CR events are being send after the actual write. In such cases,
>> while the VCPU is paused before I put a vm_response in the ring, I can
>> simply write the old value back.
>>
>> I've brought up the issue in the past, and the consensus, IIRC, was that
>> I should not alter existing behaviour (post-write events) - so the
>> alternatives were either to add a new pre-write CR event (which seemed
>> messy), or this (which seemed less intrusive).
>>
>> Of course, if it has now become acceptable to reconsider having the CR
>> vm_events consistently pre-write, the deny patch could be extended to them.
> 
> Considering that in the reply to Andrew's response you already
> pointed at where a suggestion towards consistent pre events was
> made, it would have helped if you identified where this was advised
> against before. It certainly seems sensible to treat MSR and CR
> writes in a consistent fashion.

Sorry for the omission. This is the original reply:

http://lists.xen.org/archives/html/xen-devel/2014-10/msg00240.html

where you've pointed out that we should not break existing behaviour.

After that, Tamas suggested that I could simply write the previous value
back in the vm_event userspace handler, which is how this patch was
born, and Andrew suggested pre-write hooks.

My suggestions at this point are to either:

1. Modify this patch as suggested and resubmit it in the next series
iteration (after we finish with the CR events cleanup).

2. Change the control register events to pre-write and prevent
post-write events in the future.

3. Add new event types for pre-write control register events (a new
index, CR3_PRE_WRITE or something similar).

4. Keep the existing types of control register events but add a new
per-register flag, similar to how the sync flag works, that specifies
whether the event should be pre or post-write. So the libxc
monitor_write_ctrlreg() functions would take and additional bool
parameter (bool pre_write, for example).

In cases 2-4 this patch could probably be dropped, and control register
write preemption could be added to the vm_event reply DENY flag patch.

>>>> -int hvm_set_cr0(unsigned long value)
>>>> +int hvm_set_cr0(struct vcpu *v, unsigned long value, bool_t with_vm_event)
>>>>  {
>>>> -    struct vcpu *v = current;
>>>
>>> This change is covered by neither the title nor the description, but
>>> considering it's you who sends this likely is the meat of the change.
>>> However, considering that the three calls you add to
>>> arch_set_info_guest() pass this in as zero, I even more wonder why
>>> what the title says is needed in the first place.
>>>
>>> I further wonder whether you wouldn't want an event if and only
>>> if v == current (in which case the flag parameter could be dropped).
>>
>> It just seemed useless to send out a vm_event in the case you mention,
>> since presumably the application setting them is very likely the same
>> one receiving the events (though, granted, it doesn't need to be). So in
>> that case, it would be pointless to notify itself that it has done what
>> it knows it's done.
> 
> If the setting is being done by a monitor VM, I would suppose
> v != current, and hence - along the lines above - no need for an
> event. Whereas when v == current, the setting would be done
> by the VM itself, and hence an event should always be delivered.

Ack.


Thanks,
Razvan

Re: [PATCH 5/5] xen: Write CR0, CR3 and CR4 in arch_set_info_guest()

[Jan Beulich]

>>> On 18.05.15 at 09:58, <rcojocaru@bitdefender.com> wrote:
> On 05/18/2015 10:27 AM, Jan Beulich wrote:
>>>>> On 15.05.15 at 22:45, <rcojocaru@bitdefender.com> wrote:
>>> On 05/15/2015 06:57 PM, Jan Beulich wrote:
>>>>>>> On 06.05.15 at 19:12, <rcojocaru@bitdefender.com> wrote:
>>>>> Arch_set_info_guest() doesn't set CR0, CR3 or CR4. Added code
>>>>> that does that.
>>>>
>>>> But you should also say a word on why this is needed, since things
>>>> worked fine so far without, and enabling the functions to run
>>>> outside of their own vCPU context is not immediately obviously
>>>> correct.
>>>
>>> This is a way to undo malicious CR writes. This is achieved for MSR
>>> writes with the deny vm_event response flag patch in this series, but
>>> the CR events are being send after the actual write. In such cases,
>>> while the VCPU is paused before I put a vm_response in the ring, I can
>>> simply write the old value back.
>>>
>>> I've brought up the issue in the past, and the consensus, IIRC, was that
>>> I should not alter existing behaviour (post-write events) - so the
>>> alternatives were either to add a new pre-write CR event (which seemed
>>> messy), or this (which seemed less intrusive).
>>>
>>> Of course, if it has now become acceptable to reconsider having the CR
>>> vm_events consistently pre-write, the deny patch could be extended to them.
>> 
>> Considering that in the reply to Andrew's response you already
>> pointed at where a suggestion towards consistent pre events was
>> made, it would have helped if you identified where this was advised
>> against before. It certainly seems sensible to treat MSR and CR
>> writes in a consistent fashion.
> 
> Sorry for the omission. This is the original reply:
> 
> http://lists.xen.org/archives/html/xen-devel/2014-10/msg00240.html 
> 
> where you've pointed out that we should not break existing behaviour.

As said (mainly by Andrew) numerous times recently, breaking of
existing behavior is - with all the incompatible changes already done
to the interface after 4.5 - not currently an issue.

> 2. Change the control register events to pre-write and prevent
> post-write events in the future.

This one, as long as amongst the ones that care for the events
agreement can be reached that this is the way to go.

Jan

Re: [PATCH 5/5] xen: Write CR0, CR3 and CR4 in arch_set_info_guest()

[Razvan Cojocaru]

On 05/18/2015 11:05 AM, Jan Beulich wrote:
>>>> On 18.05.15 at 09:58, <rcojocaru@bitdefender.com> wrote:
>> On 05/18/2015 10:27 AM, Jan Beulich wrote:
>>>>>> On 15.05.15 at 22:45, <rcojocaru@bitdefender.com> wrote:
>>>> On 05/15/2015 06:57 PM, Jan Beulich wrote:
>>>>>>>> On 06.05.15 at 19:12, <rcojocaru@bitdefender.com> wrote:
>>>>>> Arch_set_info_guest() doesn't set CR0, CR3 or CR4. Added code
>>>>>> that does that.
>>>>>
>>>>> But you should also say a word on why this is needed, since things
>>>>> worked fine so far without, and enabling the functions to run
>>>>> outside of their own vCPU context is not immediately obviously
>>>>> correct.
>>>>
>>>> This is a way to undo malicious CR writes. This is achieved for MSR
>>>> writes with the deny vm_event response flag patch in this series, but
>>>> the CR events are being send after the actual write. In such cases,
>>>> while the VCPU is paused before I put a vm_response in the ring, I can
>>>> simply write the old value back.
>>>>
>>>> I've brought up the issue in the past, and the consensus, IIRC, was that
>>>> I should not alter existing behaviour (post-write events) - so the
>>>> alternatives were either to add a new pre-write CR event (which seemed
>>>> messy), or this (which seemed less intrusive).
>>>>
>>>> Of course, if it has now become acceptable to reconsider having the CR
>>>> vm_events consistently pre-write, the deny patch could be extended to them.
>>>
>>> Considering that in the reply to Andrew's response you already
>>> pointed at where a suggestion towards consistent pre events was
>>> made, it would have helped if you identified where this was advised
>>> against before. It certainly seems sensible to treat MSR and CR
>>> writes in a consistent fashion.
>>
>> Sorry for the omission. This is the original reply:
>>
>> http://lists.xen.org/archives/html/xen-devel/2014-10/msg00240.html 
>>
>> where you've pointed out that we should not break existing behaviour.
> 
> As said (mainly by Andrew) numerous times recently, breaking of
> existing behavior is - with all the incompatible changes already done
> to the interface after 4.5 - not currently an issue.
> 
>> 2. Change the control register events to pre-write and prevent
>> post-write events in the future.
> 
> This one, as long as amongst the ones that care for the events
> agreement can be reached that this is the way to go.

Ack, unless otherwise requested I'll add this modification to the next
iteration of "xen/vm_event: Clean up control-register-write vm_events".


Thanks,
Razvan