diff mbox

[v2] x86/hvm: Add MSR old value

Message ID 1507899057-4667-1-git-send-email-aisaila@bitdefender.com (mailing list archive)
State New, archived
Headers show

Commit Message

Alexandru Stefan ISAILA Oct. 13, 2017, 12:50 p.m. UTC
This patch adds the old value param and the onchangeonly option
to the VM_EVENT_REASON_MOV_TO_MSR event.

The param was added to the vm_event_mov_to_msr struct and to the
hvm_monitor_msr function. Finally I've changed the bool_t param
to a bool for the hvm_msr_write_intercept function.

Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>
Acked-by: Tamas K Lengyel <tamas@tklengyel.com>

---
Changes since V1:
	- Removed Stray blanks inside the inner parentheses
	- Added space after the if statement
	- Added * 8 to the set/clear/test_bit statements
	- Removed the blank line after monitored_msr.
---
 tools/libxc/include/xenctrl.h     |  2 +-
 tools/libxc/xc_monitor.c          |  3 ++-
 xen/arch/x86/hvm/hvm.c            | 10 ++++++++--
 xen/arch/x86/hvm/monitor.c        |  9 ++++++---
 xen/arch/x86/monitor.c            | 26 +++++++++++++++++++++++---
 xen/include/asm-x86/hvm/monitor.h |  2 +-
 xen/include/asm-x86/hvm/support.h |  2 +-
 xen/include/asm-x86/monitor.h     |  1 +
 xen/include/public/domctl.h       |  2 ++
 xen/include/public/vm_event.h     |  5 +++--
 10 files changed, 48 insertions(+), 14 deletions(-)

Comments

Jan Beulich Oct. 13, 2017, 3:50 p.m. UTC | #1
>>> On 13.10.17 at 14:50, <aisaila@bitdefender.com> wrote:
> This patch adds the old value param and the onchangeonly option
> to the VM_EVENT_REASON_MOV_TO_MSR event.
> 
> The param was added to the vm_event_mov_to_msr struct and to the
> hvm_monitor_msr function. Finally I've changed the bool_t param
> to a bool for the hvm_msr_write_intercept function.
> 
> Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>
> Acked-by: Tamas K Lengyel <tamas@tklengyel.com>

I think this should have been dropped with a bug having been fixed
(for only cosmetic changes it would be fine to keep). For the bits
where it's applicable
Acked-by: Jan Beulich <jbeulich@suse.com>

Jan
Tamas K Lengyel Oct. 13, 2017, 4:26 p.m. UTC | #2
On Fri, Oct 13, 2017 at 9:50 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 13.10.17 at 14:50, <aisaila@bitdefender.com> wrote:
>> This patch adds the old value param and the onchangeonly option
>> to the VM_EVENT_REASON_MOV_TO_MSR event.
>>
>> The param was added to the vm_event_mov_to_msr struct and to the
>> hvm_monitor_msr function. Finally I've changed the bool_t param
>> to a bool for the hvm_msr_write_intercept function.
>>
>> Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>
>> Acked-by: Tamas K Lengyel <tamas@tklengyel.com>
>
> I think this should have been dropped with a bug having been fixed
> (for only cosmetic changes it would be fine to keep). For the bits
> where it's applicable
> Acked-by: Jan Beulich <jbeulich@suse.com>

Indeed. It's fine for now, my ack still stands.

Thanks,
Tamas
Razvan Cojocaru Oct. 13, 2017, 4:35 p.m. UTC | #3
On 10/13/2017 07:26 PM, Tamas K Lengyel wrote:
> On Fri, Oct 13, 2017 at 9:50 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>> On 13.10.17 at 14:50, <aisaila@bitdefender.com> wrote:
>>> This patch adds the old value param and the onchangeonly option
>>> to the VM_EVENT_REASON_MOV_TO_MSR event.
>>>
>>> The param was added to the vm_event_mov_to_msr struct and to the
>>> hvm_monitor_msr function. Finally I've changed the bool_t param
>>> to a bool for the hvm_msr_write_intercept function.
>>>
>>> Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>
>>> Acked-by: Tamas K Lengyel <tamas@tklengyel.com>
>>
>> I think this should have been dropped with a bug having been fixed
>> (for only cosmetic changes it would be fine to keep). For the bits
>> where it's applicable
>> Acked-by: Jan Beulich <jbeulich@suse.com>
> 
> Indeed. It's fine for now, my ack still stands.

That was my mistake - I thought that the fix was so obvious that Tamas
couldn't possibly object, and advised Alexandru to keep the ack. We'll
be more careful next time.


Thanks,
Razvan
Wei Liu Oct. 16, 2017, 4:13 p.m. UTC | #4
On Fri, Oct 13, 2017 at 03:50:57PM +0300, Alexandru Isaila wrote:
> This patch adds the old value param and the onchangeonly option
> to the VM_EVENT_REASON_MOV_TO_MSR event.
> 
> The param was added to the vm_event_mov_to_msr struct and to the
> hvm_monitor_msr function. Finally I've changed the bool_t param
> to a bool for the hvm_msr_write_intercept function.
> 
> Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>
> Acked-by: Tamas K Lengyel <tamas@tklengyel.com>
> 
> ---
> Changes since V1:
> 	- Removed Stray blanks inside the inner parentheses
> 	- Added space after the if statement
> 	- Added * 8 to the set/clear/test_bit statements
> 	- Removed the blank line after monitored_msr.
> ---
>  tools/libxc/include/xenctrl.h     |  2 +-
>  tools/libxc/xc_monitor.c          |  3 ++-

Acked-by: Wei Liu <wei.liu2@citrix.com>
diff mbox

Patch

diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index 3bcab3c..b99d6eb 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -2048,7 +2048,7 @@  int xc_monitor_write_ctrlreg(xc_interface *xch, domid_t domain_id,
  * non-architectural indices.
  */
 int xc_monitor_mov_to_msr(xc_interface *xch, domid_t domain_id, uint32_t msr,
-                          bool enable);
+                          bool enable, bool onchangeonly);
 int xc_monitor_singlestep(xc_interface *xch, domid_t domain_id, bool enable);
 int xc_monitor_software_breakpoint(xc_interface *xch, domid_t domain_id,
                                    bool enable);
diff --git a/tools/libxc/xc_monitor.c b/tools/libxc/xc_monitor.c
index 6046680..09d04be 100644
--- a/tools/libxc/xc_monitor.c
+++ b/tools/libxc/xc_monitor.c
@@ -90,7 +90,7 @@  int xc_monitor_write_ctrlreg(xc_interface *xch, domid_t domain_id,
 }
 
 int xc_monitor_mov_to_msr(xc_interface *xch, domid_t domain_id, uint32_t msr,
-                          bool enable)
+                          bool enable, bool onchangeonly)
 {
     DECLARE_DOMCTL;
 
@@ -100,6 +100,7 @@  int xc_monitor_mov_to_msr(xc_interface *xch, domid_t domain_id, uint32_t msr,
                                     : XEN_DOMCTL_MONITOR_OP_DISABLE;
     domctl.u.monitor_op.event = XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR;
     domctl.u.monitor_op.u.mov_to_msr.msr = msr;
+    domctl.u.monitor_op.u.mov_to_msr.onchangeonly = onchangeonly;
 
     return do_domctl(xch, &domctl);
 }
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 205b4cb..0238787 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -3489,7 +3489,7 @@  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 may_defer)
+                            bool may_defer)
 {
     struct vcpu *v = current;
     struct domain *d = v->domain;
@@ -3500,6 +3500,12 @@  int hvm_msr_write_intercept(unsigned int msr, uint64_t msr_content,
 
     if ( may_defer && unlikely(monitored_msr(v->domain, msr)) )
     {
+        uint64_t msr_old_content;
+
+        ret = hvm_msr_read_intercept(msr, &msr_old_content);
+        if ( ret != X86EMUL_OKAY )
+            return ret;
+
         ASSERT(v->arch.vm_event);
 
         /* The actual write will occur in hvm_do_resume() (if permitted). */
@@ -3507,7 +3513,7 @@  int hvm_msr_write_intercept(unsigned int msr, uint64_t msr_content,
         v->arch.vm_event->write_data.msr = msr;
         v->arch.vm_event->write_data.value = msr_content;
 
-        hvm_monitor_msr(msr, msr_content);
+        hvm_monitor_msr(msr, msr_content, msr_old_content);
         return X86EMUL_OKAY;
     }
 
diff --git a/xen/arch/x86/hvm/monitor.c b/xen/arch/x86/hvm/monitor.c
index 4ce778c..131b852 100644
--- a/xen/arch/x86/hvm/monitor.c
+++ b/xen/arch/x86/hvm/monitor.c
@@ -74,16 +74,19 @@  bool hvm_monitor_emul_unimplemented(void)
         monitor_traps(curr, true, &req) == 1;
 }
 
-void hvm_monitor_msr(unsigned int msr, uint64_t value)
+void hvm_monitor_msr(unsigned int msr, uint64_t new_value, uint64_t old_value)
 {
     struct vcpu *curr = current;
 
-    if ( monitored_msr(curr->domain, msr) )
+    if ( monitored_msr(curr->domain, msr) &&
+         (!monitored_msr_onchangeonly(curr->domain, msr) ||
+           new_value != old_value) )
     {
         vm_event_request_t req = {
             .reason = VM_EVENT_REASON_MOV_TO_MSR,
             .u.mov_to_msr.msr = msr,
-            .u.mov_to_msr.value = value,
+            .u.mov_to_msr.new_value = new_value,
+            .u.mov_to_msr.old_value = old_value
         };
 
         monitor_traps(curr, 1, &req);
diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c
index e59f1f5..ecfa9e5 100644
--- a/xen/arch/x86/monitor.c
+++ b/xen/arch/x86/monitor.c
@@ -25,7 +25,7 @@ 
 int arch_monitor_init_domain(struct domain *d)
 {
     if ( !d->arch.monitor.msr_bitmap )
-        d->arch.monitor.msr_bitmap = xzalloc(struct monitor_msr_bitmap);
+        d->arch.monitor.msr_bitmap = xzalloc_array(struct monitor_msr_bitmap, 2);
 
     if ( !d->arch.monitor.msr_bitmap )
         return -ENOMEM;
@@ -67,7 +67,7 @@  static unsigned long *monitor_bitmap_for_msr(const struct domain *d, u32 *msr)
     }
 }
 
-static int monitor_enable_msr(struct domain *d, u32 msr)
+static int monitor_enable_msr(struct domain *d, u32 msr, bool onchangeonly)
 {
     unsigned long *bitmap;
     u32 index = msr;
@@ -84,6 +84,11 @@  static int monitor_enable_msr(struct domain *d, u32 msr)
 
     hvm_enable_msr_interception(d, msr);
 
+    if ( onchangeonly )
+        __set_bit(index + sizeof(struct monitor_msr_bitmap) * 8, bitmap);
+    else
+        __clear_bit(index + sizeof(struct monitor_msr_bitmap) * 8, bitmap);
+
     return 0;
 }
 
@@ -119,6 +124,21 @@  bool monitored_msr(const struct domain *d, u32 msr)
     return test_bit(msr, bitmap);
 }
 
+bool monitored_msr_onchangeonly(const struct domain *d, u32 msr)
+{
+    const unsigned long *bitmap;
+
+    if ( !d->arch.monitor.msr_bitmap )
+        return false;
+
+    bitmap = monitor_bitmap_for_msr(d, &msr);
+
+    if ( !bitmap )
+        return false;
+
+    return test_bit(msr + sizeof(struct monitor_msr_bitmap) * 8, bitmap);
+}
+
 int arch_monitor_domctl_event(struct domain *d,
                               struct xen_domctl_monitor_op *mop)
 {
@@ -198,7 +218,7 @@  int arch_monitor_domctl_event(struct domain *d,
         }
 
         if ( requested_status )
-            rc = monitor_enable_msr(d, msr);
+            rc = monitor_enable_msr(d, msr, mop->u.mov_to_msr.onchangeonly);
         else
             rc = monitor_disable_msr(d, msr);
 
diff --git a/xen/include/asm-x86/hvm/monitor.h b/xen/include/asm-x86/hvm/monitor.h
index 6e22091..260d7b0 100644
--- a/xen/include/asm-x86/hvm/monitor.h
+++ b/xen/include/asm-x86/hvm/monitor.h
@@ -37,7 +37,7 @@  bool hvm_monitor_cr(unsigned int index, unsigned long value,
                     unsigned long old);
 #define hvm_monitor_crX(cr, new, old) \
                         hvm_monitor_cr(VM_EVENT_X86_##cr, new, old)
-void hvm_monitor_msr(unsigned int msr, uint64_t value);
+void hvm_monitor_msr(unsigned int msr, uint64_t new_value, uint64_t old_value);
 void hvm_monitor_descriptor_access(uint64_t exit_info,
                                    uint64_t vmx_exit_qualification,
                                    uint8_t descriptor, bool is_write);
diff --git a/xen/include/asm-x86/hvm/support.h b/xen/include/asm-x86/hvm/support.h
index d784fc1..ac33eea 100644
--- a/xen/include/asm-x86/hvm/support.h
+++ b/xen/include/asm-x86/hvm/support.h
@@ -154,7 +154,7 @@  void hvm_ud_intercept(struct cpu_user_regs *);
 int __must_check hvm_msr_read_intercept(
     unsigned int msr, uint64_t *msr_content);
 int __must_check hvm_msr_write_intercept(
-    unsigned int msr, uint64_t msr_content, bool_t may_defer);
+    unsigned int msr, uint64_t msr_content, bool may_defer);
 
 #endif /* __ASM_X86_HVM_SUPPORT_H__ */
 
diff --git a/xen/include/asm-x86/monitor.h b/xen/include/asm-x86/monitor.h
index 0ada970..a0444d1 100644
--- a/xen/include/asm-x86/monitor.h
+++ b/xen/include/asm-x86/monitor.h
@@ -104,5 +104,6 @@  int arch_monitor_init_domain(struct domain *d);
 void arch_monitor_cleanup_domain(struct domain *d);
 
 bool monitored_msr(const struct domain *d, u32 msr);
+bool monitored_msr_onchangeonly(const struct domain *d, u32 msr);
 
 #endif /* __ASM_X86_MONITOR_H__ */
diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index 8853445..3dfadae 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -1046,6 +1046,8 @@  struct xen_domctl_monitor_op {
 
         struct {
             uint32_t msr;
+            /* Send event only on a change of value */
+            uint8_t onchangeonly;
         } mov_to_msr;
 
         struct {
diff --git a/xen/include/public/vm_event.h b/xen/include/public/vm_event.h
index b531f71..36e3f46 100644
--- a/xen/include/public/vm_event.h
+++ b/xen/include/public/vm_event.h
@@ -29,7 +29,7 @@ 
 
 #include "xen.h"
 
-#define VM_EVENT_INTERFACE_VERSION 0x00000002
+#define VM_EVENT_INTERFACE_VERSION 0x00000003
 
 #if defined(__XEN__) || defined(__XEN_TOOLS__)
 
@@ -260,7 +260,8 @@  struct vm_event_debug {
 
 struct vm_event_mov_to_msr {
     uint64_t msr;
-    uint64_t value;
+    uint64_t new_value;
+    uint64_t old_value;
 };
 
 #define VM_EVENT_DESC_IDTR           1