[PATCH v4 1/8] monitor: Rename vm_event_monitor_get_capabilities

[PATCH v4 1/8] monitor: Rename vm_event_monitor_get_capabilities

[Tamas K Lengyel]

The monitor_get_capabilities check actually belongs to the monitor subsystem so
relocating and renaming it to sanitize the code's name and location. Mechanical
patch, no code changes introduced.

Signed-off-by: Tamas K Lengyel <tamas@tklengyel.com>
Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Julien Grall <julien.grall@arm.com>
---
 xen/arch/x86/monitor.c         |  2 +-
 xen/common/monitor.c           |  5 ++---
 xen/include/asm-arm/monitor.h  | 11 ++++++++++-
 xen/include/asm-arm/vm_event.h |  9 ---------
 xen/include/asm-x86/monitor.h  | 23 +++++++++++++++++++++++
 xen/include/asm-x86/vm_event.h | 23 -----------------------
 6 files changed, 36 insertions(+), 37 deletions(-)

diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c
index 1fec412..621f91a 100644
--- a/xen/arch/x86/monitor.c
+++ b/xen/arch/x86/monitor.c
@@ -126,7 +126,7 @@ int arch_monitor_domctl_event(struct domain *d,
 
     default:
         /*
-         * Should not be reached unless vm_event_monitor_get_capabilities() is
+         * Should not be reached unless arch_monitor_get_capabilities() is
          * not properly implemented.
          */
         ASSERT_UNREACHABLE();
diff --git a/xen/common/monitor.c b/xen/common/monitor.c
index d950a7c..7c3d1c8 100644
--- a/xen/common/monitor.c
+++ b/xen/common/monitor.c
@@ -24,7 +24,6 @@
 #include <xsm/xsm.h>
 #include <public/domctl.h>
 #include <asm/monitor.h>
-#include <asm/vm_event.h>
 
 int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *mop)
 {
@@ -48,12 +47,12 @@ int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *mop)
         if ( unlikely(mop->event > 31) )
             return -EINVAL;
         /* Check if event type is available. */
-        if ( unlikely(!(vm_event_monitor_get_capabilities(d) & (1U << mop->event))) )
+        if ( unlikely(!(arch_monitor_get_capabilities(d) & (1U << mop->event))) )
             return -EOPNOTSUPP;
         break;
 
     case XEN_DOMCTL_MONITOR_OP_GET_CAPABILITIES:
-        mop->event = vm_event_monitor_get_capabilities(d);
+        mop->event = arch_monitor_get_capabilities(d);
         return 0;
 
     default:
diff --git a/xen/include/asm-arm/monitor.h b/xen/include/asm-arm/monitor.h
index 6e36e99..3fd3c9d 100644
--- a/xen/include/asm-arm/monitor.h
+++ b/xen/include/asm-arm/monitor.h
@@ -39,11 +39,20 @@ int arch_monitor_domctl_event(struct domain *d,
     /*
      * No arch-specific monitor vm-events on ARM.
      *
-     * Should not be reached unless vm_event_monitor_get_capabilities() is not
+     * Should not be reached unless arch_monitor_get_capabilities() is not
      * properly implemented.
      */
     ASSERT_UNREACHABLE();
     return -EOPNOTSUPP;
 }
 
+static inline uint32_t arch_monitor_get_capabilities(struct domain *d)
+{
+    uint32_t capabilities = 0;
+
+    capabilities = (1U << XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST);
+
+    return capabilities;
+}
+
 #endif /* __ASM_ARM_MONITOR_H__ */
diff --git a/xen/include/asm-arm/vm_event.h b/xen/include/asm-arm/vm_event.h
index 014d9ba..a3fc4ce 100644
--- a/xen/include/asm-arm/vm_event.h
+++ b/xen/include/asm-arm/vm_event.h
@@ -59,13 +59,4 @@ static inline void vm_event_fill_regs(vm_event_request_t *req)
     /* Not supported on ARM. */
 }
 
-static inline uint32_t vm_event_monitor_get_capabilities(struct domain *d)
-{
-    uint32_t capabilities = 0;
-
-    capabilities = (1U << XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST);
-
-    return capabilities;
-}
-
 #endif /* __ASM_ARM_VM_EVENT_H__ */
diff --git a/xen/include/asm-x86/monitor.h b/xen/include/asm-x86/monitor.h
index d367099..0fee750 100644
--- a/xen/include/asm-x86/monitor.h
+++ b/xen/include/asm-x86/monitor.h
@@ -60,4 +60,27 @@ int arch_monitor_domctl_op(struct domain *d, struct xen_domctl_monitor_op *mop)
 int arch_monitor_domctl_event(struct domain *d,
                               struct xen_domctl_monitor_op *mop);
 
+static inline uint32_t arch_monitor_get_capabilities(struct domain *d)
+{
+    uint32_t capabilities = 0;
+
+    /*
+     * At the moment only Intel HVM domains are supported. However, event
+     * delivery could be extended to AMD and PV domains.
+     */
+    if ( !is_hvm_domain(d) || !cpu_has_vmx )
+        return capabilities;
+
+    capabilities = (1U << XEN_DOMCTL_MONITOR_EVENT_WRITE_CTRLREG) |
+                   (1U << XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR) |
+                   (1U << XEN_DOMCTL_MONITOR_EVENT_SOFTWARE_BREAKPOINT) |
+                   (1U << XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST);
+
+    /* Since we know this is on VMX, we can just call the hvm func */
+    if ( hvm_is_singlestep_supported() )
+        capabilities |= (1U << XEN_DOMCTL_MONITOR_EVENT_SINGLESTEP);
+
+    return capabilities;
+}
+
 #endif /* __ASM_X86_MONITOR_H__ */
diff --git a/xen/include/asm-x86/vm_event.h b/xen/include/asm-x86/vm_event.h
index 0470240..026f42e 100644
--- a/xen/include/asm-x86/vm_event.h
+++ b/xen/include/asm-x86/vm_event.h
@@ -44,27 +44,4 @@ void vm_event_set_registers(struct vcpu *v, vm_event_response_t *rsp);
 
 void vm_event_fill_regs(vm_event_request_t *req);
 
-static inline uint32_t vm_event_monitor_get_capabilities(struct domain *d)
-{
-    uint32_t capabilities = 0;
-
-    /*
-     * At the moment only Intel HVM domains are supported. However, event
-     * delivery could be extended to AMD and PV domains.
-     */
-    if ( !is_hvm_domain(d) || !cpu_has_vmx )
-        return capabilities;
-
-    capabilities = (1U << XEN_DOMCTL_MONITOR_EVENT_WRITE_CTRLREG) |
-                   (1U << XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR) |
-                   (1U << XEN_DOMCTL_MONITOR_EVENT_SOFTWARE_BREAKPOINT) |
-                   (1U << XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST);
-
-    /* Since we know this is on VMX, we can just call the hvm func */
-    if ( hvm_is_singlestep_supported() )
-        capabilities |= (1U << XEN_DOMCTL_MONITOR_EVENT_SINGLESTEP);
-
-    return capabilities;
-}
-
 #endif /* __ASM_X86_VM_EVENT_H__ */
-- 
2.8.1


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

[PATCH v4 2/8] monitor: Rename vm_event_monitor_guest_request

[Tamas K Lengyel]

Mechanical renaming and relocation to the monitor subsystem.

Signed-off-by: Tamas K Lengyel <tamas@tklengyel.com>
---
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Julien Grall <julien.grall@arm.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Razvan Cojocaru <rcojocaru@bitdefender.com>
---
 xen/arch/arm/hvm.c         |  4 ++--
 xen/arch/x86/hvm/hvm.c     |  3 ++-
 xen/common/monitor.c       | 17 +++++++++++++++++
 xen/common/vm_event.c      | 16 ----------------
 xen/include/xen/monitor.h  |  1 +
 xen/include/xen/vm_event.h |  2 --
 6 files changed, 22 insertions(+), 21 deletions(-)

diff --git a/xen/arch/arm/hvm.c b/xen/arch/arm/hvm.c
index c01123a..d999bde 100644
--- a/xen/arch/arm/hvm.c
+++ b/xen/arch/arm/hvm.c
@@ -22,7 +22,7 @@
 #include <xen/errno.h>
 #include <xen/guest_access.h>
 #include <xen/sched.h>
-#include <xen/vm_event.h>
+#include <xen/monitor.h>
 
 #include <xsm/xsm.h>
 
@@ -75,7 +75,7 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
 
     case HVMOP_guest_request_vm_event:
         if ( guest_handle_is_null(arg) )
-            vm_event_monitor_guest_request();
+            monitor_guest_request();
         else
             rc = -EINVAL;
         break;
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 5040a5c..7bf6a36 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -37,6 +37,7 @@
 #include <xen/mem_access.h>
 #include <xen/rangeset.h>
 #include <xen/vm_event.h>
+#include <xen/monitor.h>
 #include <asm/shadow.h>
 #include <asm/hap.h>
 #include <asm/current.h>
@@ -5704,7 +5705,7 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
 
     case HVMOP_guest_request_vm_event:
         if ( guest_handle_is_null(arg) )
-            vm_event_monitor_guest_request();
+            monitor_guest_request();
         else
             rc = -EINVAL;
         break;
diff --git a/xen/common/monitor.c b/xen/common/monitor.c
index 7c3d1c8..436214a 100644
--- a/xen/common/monitor.c
+++ b/xen/common/monitor.c
@@ -21,6 +21,7 @@
 
 #include <xen/monitor.h>
 #include <xen/sched.h>
+#include <xen/vm_event.h>
 #include <xsm/xsm.h>
 #include <public/domctl.h>
 #include <asm/monitor.h>
@@ -84,6 +85,22 @@ int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *mop)
     return 0;
 }
 
+void monitor_guest_request(void)
+{
+    struct vcpu *curr = current;
+    struct domain *d = curr->domain;
+
+    if ( d->monitor.guest_request_enabled )
+    {
+        vm_event_request_t req = {
+            .reason = VM_EVENT_REASON_GUEST_REQUEST,
+            .vcpu_id = curr->vcpu_id,
+        };
+
+        vm_event_monitor_traps(curr, d->monitor.guest_request_sync, &req);
+    }
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/common/vm_event.c b/xen/common/vm_event.c
index 2906407..a489c04 100644
--- a/xen/common/vm_event.c
+++ b/xen/common/vm_event.c
@@ -824,22 +824,6 @@ int vm_event_monitor_traps(struct vcpu *v, uint8_t sync,
     return 1;
 }
 
-void vm_event_monitor_guest_request(void)
-{
-    struct vcpu *curr = current;
-    struct domain *d = curr->domain;
-
-    if ( d->monitor.guest_request_enabled )
-    {
-        vm_event_request_t req = {
-            .reason = VM_EVENT_REASON_GUEST_REQUEST,
-            .vcpu_id = curr->vcpu_id,
-        };
-
-        vm_event_monitor_traps(curr, d->monitor.guest_request_sync, &req);
-    }
-}
-
 /*
  * Local variables:
  * mode: C
diff --git a/xen/include/xen/monitor.h b/xen/include/xen/monitor.h
index 7015e6d..204d5cc 100644
--- a/xen/include/xen/monitor.h
+++ b/xen/include/xen/monitor.h
@@ -26,5 +26,6 @@ struct domain;
 struct xen_domctl_monitor_op;
 
 int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *op);
+void monitor_guest_request(void);
 
 #endif /* __XEN_MONITOR_H__ */
diff --git a/xen/include/xen/vm_event.h b/xen/include/xen/vm_event.h
index beda9fe..89e6243 100644
--- a/xen/include/xen/vm_event.h
+++ b/xen/include/xen/vm_event.h
@@ -81,8 +81,6 @@ void vm_event_vcpu_unpause(struct vcpu *v);
 int vm_event_monitor_traps(struct vcpu *v, uint8_t sync,
                            vm_event_request_t *req);
 
-void vm_event_monitor_guest_request(void);
-
 #endif /* __VM_EVENT_H__ */
 
 
-- 
2.8.1


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

Re: [PATCH v4 2/8] monitor: Rename vm_event_monitor_guest_request

[Razvan Cojocaru]

On 05/30/2016 01:37 AM, Tamas K Lengyel wrote:
> Mechanical renaming and relocation to the monitor subsystem.
> 
> Signed-off-by: Tamas K Lengyel <tamas@tklengyel.com>
> ---
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Julien Grall <julien.grall@arm.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: Razvan Cojocaru <rcojocaru@bitdefender.com>
> ---
>  xen/arch/arm/hvm.c         |  4 ++--
>  xen/arch/x86/hvm/hvm.c     |  3 ++-
>  xen/common/monitor.c       | 17 +++++++++++++++++
>  xen/common/vm_event.c      | 16 ----------------
>  xen/include/xen/monitor.h  |  1 +
>  xen/include/xen/vm_event.h |  2 --
>  6 files changed, 22 insertions(+), 21 deletions(-)

Acked-by: Razvan Cojocaru <rcojocaru@bitdefender.com>

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

Re: [PATCH v4 2/8] monitor: Rename vm_event_monitor_guest_request

[Jan Beulich]

>>> On 30.05.16 at 00:37, <tamas@tklengyel.com> wrote:
> Mechanical renaming and relocation to the monitor subsystem.
> 
> Signed-off-by: Tamas K Lengyel <tamas@tklengyel.com>

Non-VM-event parts
Acked-by: Jan Beulich <jbeulich@suse.com>


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

[PATCH v4 3/8] monitor: Rename hvm/event to hvm/monitor

[Tamas K Lengyel]

Mechanical renaming to better describe that the code in hvm/event is part of
the monitor subsystem.

Signed-off-by: Tamas K Lengyel <tamas@tklengyel.com>
Acked-by: Kevin Tian <kevin.tian@intel.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Razvan Cojocaru <rcojocaru@bitdefender.com>
Cc: Jun Nakajima <jun.nakajima@intel.com>
---
 xen/arch/x86/hvm/Makefile                      |  2 +-
 xen/arch/x86/hvm/hvm.c                         | 12 +++++------
 xen/arch/x86/hvm/{event.c => monitor.c}        | 17 ++++++++-------
 xen/arch/x86/hvm/vmx/vmx.c                     | 13 +++++------
 xen/include/asm-x86/hvm/{event.h => monitor.h} | 30 +++++++++++++-------------
 5 files changed, 38 insertions(+), 36 deletions(-)
 rename xen/arch/x86/hvm/{event.c => monitor.c} (88%)
 rename xen/include/asm-x86/hvm/{event.h => monitor.h} (59%)

diff --git a/xen/arch/x86/hvm/Makefile b/xen/arch/x86/hvm/Makefile
index 8bc55a9..f750d13 100644
--- a/xen/arch/x86/hvm/Makefile
+++ b/xen/arch/x86/hvm/Makefile
@@ -3,7 +3,6 @@ subdir-y += vmx
 
 obj-y += asid.o
 obj-y += emulate.o
-obj-y += event.o
 obj-y += hpet.o
 obj-y += hvm.o
 obj-y += i8254.o
@@ -11,6 +10,7 @@ obj-y += intercept.o
 obj-y += io.o
 obj-y += ioreq.o
 obj-y += irq.o
+obj-y += monitor.o
 obj-y += mtrr.o
 obj-y += nestedhvm.o
 obj-y += pmtimer.o
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 7bf6a36..c4089be 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -60,7 +60,7 @@
 #include <asm/hvm/cacheattr.h>
 #include <asm/hvm/trace.h>
 #include <asm/hvm/nestedhvm.h>
-#include <asm/hvm/event.h>
+#include <asm/hvm/monitor.h>
 #include <asm/hvm/ioreq.h>
 #include <asm/hvm/vmx/vmx.h>
 #include <asm/altp2m.h>
@@ -1961,7 +1961,7 @@ int hvm_handle_xsetbv(u32 index, u64 new_bv)
 {
     int rc;
 
-    hvm_event_crX(XCR0, new_bv, current->arch.xcr0);
+    hvm_monitor_crX(XCR0, new_bv, current->arch.xcr0);
 
     rc = handle_xsetbv(index, new_bv);
     if ( rc )
@@ -2197,7 +2197,7 @@ int hvm_set_cr0(unsigned long value, bool_t may_defer)
     {
         ASSERT(v->arch.vm_event);
 
-        if ( hvm_event_crX(CR0, value, old_value) )
+        if ( hvm_monitor_crX(CR0, value, old_value) )
         {
             /* The actual write will occur in hvm_do_resume(), if permitted. */
             v->arch.vm_event->write_data.do_write.cr0 = 1;
@@ -2299,7 +2299,7 @@ int hvm_set_cr3(unsigned long value, bool_t may_defer)
     {
         ASSERT(v->arch.vm_event);
 
-        if ( hvm_event_crX(CR3, value, old) )
+        if ( hvm_monitor_crX(CR3, value, old) )
         {
             /* The actual write will occur in hvm_do_resume(), if permitted. */
             v->arch.vm_event->write_data.do_write.cr3 = 1;
@@ -2379,7 +2379,7 @@ int hvm_set_cr4(unsigned long value, bool_t may_defer)
     {
         ASSERT(v->arch.vm_event);
 
-        if ( hvm_event_crX(CR4, value, old_cr) )
+        if ( hvm_monitor_crX(CR4, value, old_cr) )
         {
             /* The actual write will occur in hvm_do_resume(), if permitted. */
             v->arch.vm_event->write_data.do_write.cr4 = 1;
@@ -3722,7 +3722,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_event_msr(msr, msr_content);
+        hvm_monitor_msr(msr, msr_content);
         return X86EMUL_OKAY;
     }
 
diff --git a/xen/arch/x86/hvm/event.c b/xen/arch/x86/hvm/monitor.c
similarity index 88%
rename from xen/arch/x86/hvm/event.c
rename to xen/arch/x86/hvm/monitor.c
index 56c5514..8b41b56 100644
--- a/xen/arch/x86/hvm/event.c
+++ b/xen/arch/x86/hvm/monitor.c
@@ -1,5 +1,5 @@
 /*
- * arch/x86/hvm/event.c
+ * arch/x86/hvm/monitor.c
  *
  * Arch-specific hardware virtual machine event abstractions.
  *
@@ -7,6 +7,7 @@
  * Copyright (c) 2005, International Business Machines Corporation.
  * Copyright (c) 2008, Citrix Systems, Inc.
  * Copyright (c) 2016, Bitdefender S.R.L.
+ * Copyright (c) 2016, Tamas K Lengyel (tamas@tklengyel.com)
  *
  * This program is free software; you can redistribute it and/or modify it
  * under the terms and conditions of the GNU General Public License,
@@ -22,12 +23,12 @@
  */
 
 #include <xen/vm_event.h>
-#include <asm/hvm/event.h>
+#include <asm/hvm/monitor.h>
 #include <asm/monitor.h>
 #include <asm/vm_event.h>
 #include <public/vm_event.h>
 
-bool_t hvm_event_cr(unsigned int index, unsigned long value, unsigned long old)
+bool_t hvm_monitor_cr(unsigned int index, unsigned long value, unsigned long old)
 {
     struct vcpu *curr = current;
     struct arch_domain *ad = &curr->domain->arch;
@@ -54,7 +55,7 @@ bool_t hvm_event_cr(unsigned int index, unsigned long value, unsigned long old)
     return 0;
 }
 
-void hvm_event_msr(unsigned int msr, uint64_t value)
+void hvm_monitor_msr(unsigned int msr, uint64_t value)
 {
     struct vcpu *curr = current;
     struct arch_domain *ad = &curr->domain->arch;
@@ -87,8 +88,8 @@ static inline unsigned long gfn_of_rip(unsigned long rip)
     return paging_gva_to_gfn(curr, sreg.base + rip, &pfec);
 }
 
-int hvm_event_breakpoint(unsigned long rip,
-                         enum hvm_event_breakpoint_type type)
+int hvm_monitor_breakpoint(unsigned long rip,
+                           enum hvm_monitor_breakpoint_type type)
 {
     struct vcpu *curr = current;
     struct arch_domain *ad = &curr->domain->arch;
@@ -96,14 +97,14 @@ int hvm_event_breakpoint(unsigned long rip,
 
     switch ( type )
     {
-    case HVM_EVENT_SOFTWARE_BREAKPOINT:
+    case HVM_MONITOR_SOFTWARE_BREAKPOINT:
         if ( !ad->monitor.software_breakpoint_enabled )
             return 0;
         req.reason = VM_EVENT_REASON_SOFTWARE_BREAKPOINT;
         req.u.software_breakpoint.gfn = gfn_of_rip(rip);
         break;
 
-    case HVM_EVENT_SINGLESTEP_BREAKPOINT:
+    case HVM_MONITOR_SINGLESTEP_BREAKPOINT:
         if ( !ad->monitor.singlestep_enabled )
             return 0;
         req.reason = VM_EVENT_REASON_SINGLESTEP;
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 3acf1ab..081aef1 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -50,7 +50,7 @@
 #include <asm/hvm/vpt.h>
 #include <public/hvm/save.h>
 #include <asm/hvm/trace.h>
-#include <asm/hvm/event.h>
+#include <asm/hvm/monitor.h>
 #include <asm/xenoprof.h>
 #include <asm/debugger.h>
 #include <asm/apic.h>
@@ -2477,10 +2477,10 @@ static int vmx_cr_access(unsigned long exit_qualification)
 
         /*
          * Special case unlikely to be interesting to a
-         * VM_EVENT_FLAG_DENY-capable application, so the hvm_event_crX()
+         * VM_EVENT_FLAG_DENY-capable application, so the hvm_monitor_crX()
          * return value is ignored for now.
          */
-        hvm_event_crX(CR0, value, old);
+        hvm_monitor_crX(CR0, value, old);
         curr->arch.hvm_vcpu.guest_cr[0] = value;
         vmx_update_guest_cr(curr, 0);
         HVMTRACE_0D(CLTS);
@@ -3393,8 +3393,8 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
             }
             else {
                 int handled =
-                        hvm_event_breakpoint(regs->eip,
-                                             HVM_EVENT_SOFTWARE_BREAKPOINT);
+                    hvm_monitor_breakpoint(regs->eip,
+                                           HVM_MONITOR_SOFTWARE_BREAKPOINT);
 
                 if ( handled < 0 ) 
                 {
@@ -3721,7 +3721,8 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
         vmx_update_cpu_exec_control(v);
         if ( v->arch.hvm_vcpu.single_step )
         {
-            hvm_event_breakpoint(regs->eip, HVM_EVENT_SINGLESTEP_BREAKPOINT);
+            hvm_monitor_breakpoint(regs->eip,
+                                   HVM_MONITOR_SINGLESTEP_BREAKPOINT);
             if ( v->domain->debugger_attached )
                 domain_pause_for_debugger();
         }
diff --git a/xen/include/asm-x86/hvm/event.h b/xen/include/asm-x86/hvm/monitor.h
similarity index 59%
rename from xen/include/asm-x86/hvm/event.h
rename to xen/include/asm-x86/hvm/monitor.h
index 03f7fee..55d435e 100644
--- a/xen/include/asm-x86/hvm/event.h
+++ b/xen/include/asm-x86/hvm/monitor.h
@@ -1,7 +1,7 @@
 /*
- * include/asm-x86/hvm/event.h
+ * include/asm-x86/hvm/monitor.h
  *
- * Arch-specific hardware virtual machine event abstractions.
+ * Arch-specific hardware virtual machine monitor abstractions.
  *
  * This program is free software; you can redistribute it and/or modify it
  * under the terms and conditions of the GNU General Public License,
@@ -16,17 +16,17 @@
  * this program; If not, see <http://www.gnu.org/licenses/>.
  */
 
-#ifndef __ASM_X86_HVM_EVENT_H__
-#define __ASM_X86_HVM_EVENT_H__
+#ifndef __ASM_X86_HVM_MONITOR_H__
+#define __ASM_X86_HVM_MONITOR_H__
 
 #include <xen/sched.h>
 #include <xen/paging.h>
 #include <public/vm_event.h>
 
-enum hvm_event_breakpoint_type
+enum hvm_monitor_breakpoint_type
 {
-    HVM_EVENT_SOFTWARE_BREAKPOINT,
-    HVM_EVENT_SINGLESTEP_BREAKPOINT,
+    HVM_MONITOR_SOFTWARE_BREAKPOINT,
+    HVM_MONITOR_SINGLESTEP_BREAKPOINT,
 };
 
 /*
@@ -34,15 +34,15 @@ enum hvm_event_breakpoint_type
  * The event might not fire if the client has subscribed to it in onchangeonly
  * mode, hence the bool_t return type for control register write events.
  */
-bool_t hvm_event_cr(unsigned int index, unsigned long value,
-                    unsigned long old);
-#define hvm_event_crX(cr, new, old) \
-    hvm_event_cr(VM_EVENT_X86_##cr, new, old)
-void hvm_event_msr(unsigned int msr, uint64_t value);
-int hvm_event_breakpoint(unsigned long rip,
-                         enum hvm_event_breakpoint_type type);
+bool_t 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);
+int hvm_monitor_breakpoint(unsigned long rip,
+                           enum hvm_monitor_breakpoint_type type);
 
-#endif /* __ASM_X86_HVM_EVENT_H__ */
+#endif /* __ASM_X86_HVM_MONITOR_H__ */
 
 /*
  * Local variables:
-- 
2.8.1


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

Re: [PATCH v4 3/8] monitor: Rename hvm/event to hvm/monitor

[Razvan Cojocaru]

On 05/30/2016 01:37 AM, Tamas K Lengyel wrote:
> Mechanical renaming to better describe that the code in hvm/event is part of
> the monitor subsystem.
> 
> Signed-off-by: Tamas K Lengyel <tamas@tklengyel.com>
> Acked-by: Kevin Tian <kevin.tian@intel.com>
> ---
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: Razvan Cojocaru <rcojocaru@bitdefender.com>
> Cc: Jun Nakajima <jun.nakajima@intel.com>
> ---
>  xen/arch/x86/hvm/Makefile                      |  2 +-
>  xen/arch/x86/hvm/hvm.c                         | 12 +++++------
>  xen/arch/x86/hvm/{event.c => monitor.c}        | 17 ++++++++-------
>  xen/arch/x86/hvm/vmx/vmx.c                     | 13 +++++------
>  xen/include/asm-x86/hvm/{event.h => monitor.h} | 30 +++++++++++++-------------
>  5 files changed, 38 insertions(+), 36 deletions(-)
>  rename xen/arch/x86/hvm/{event.c => monitor.c} (88%)
>  rename xen/include/asm-x86/hvm/{event.h => monitor.h} (59%)

Acked-by: Razvan Cojocaru <rcojocaru@bitdefender.com>

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

Re: [PATCH v4 3/8] monitor: Rename hvm/event to hvm/monitor

[Jan Beulich]

>>> On 30.05.16 at 00:37, <tamas@tklengyel.com> wrote:
> Mechanical renaming to better describe that the code in hvm/event is part of
> the monitor subsystem.
> 
> Signed-off-by: Tamas K Lengyel <tamas@tklengyel.com>
> Acked-by: Kevin Tian <kevin.tian@intel.com>

General x86/hvm parts
Acked-by: Jan Beulich <jbeulich@suse.com>


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

[PATCH v4 4/8] monitor: ARM SMC events

[Tamas K Lengyel]

Add support for monitoring ARM SMC events. This patch only adds the required
bits to enable/disable monitoring and forwarding the event through vm_event.

Signed-off-by: Tamas K Lengyel <tamas@tklengyel.com>
Acked-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
---
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Julien Grall <julien.grall@arm.com>

v4: Style fixes
v3: Split parts off as separate patches
    Union for arm32/64 register structs in vm_event
    Cosmetic fixes
---
 xen/arch/arm/Makefile         |  1 +
 xen/arch/arm/monitor.c        | 81 +++++++++++++++++++++++++++++++++++++++++++
 xen/arch/arm/traps.c          | 13 +++++--
 xen/include/asm-arm/domain.h  |  5 +++
 xen/include/asm-arm/monitor.h | 24 ++++---------
 xen/include/public/domctl.h   |  1 +
 xen/include/public/vm_event.h |  2 ++
 7 files changed, 107 insertions(+), 20 deletions(-)
 create mode 100644 xen/arch/arm/monitor.c

diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
index ead0cc0..344d3ad 100644
--- a/xen/arch/arm/Makefile
+++ b/xen/arch/arm/Makefile
@@ -41,6 +41,7 @@ obj-y += decode.o
 obj-y += processor.o
 obj-y += smc.o
 obj-$(CONFIG_XSPLICE) += xsplice.o
+obj-y += monitor.o
 
 #obj-bin-y += ....o
 
diff --git a/xen/arch/arm/monitor.c b/xen/arch/arm/monitor.c
new file mode 100644
index 0000000..788b7e8
--- /dev/null
+++ b/xen/arch/arm/monitor.c
@@ -0,0 +1,81 @@
+/*
+ * arch/arm/monitor.c
+ *
+ * Arch-specific monitor_op domctl handler.
+ *
+ * Copyright (c) 2016 Tamas K Lengyel (tamas@tklengyel.com)
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public
+ * License v2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public
+ * License along with this program; If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <asm/vm_event.h>
+#include <public/vm_event.h>
+
+int arch_monitor_domctl_event(struct domain *d,
+                              struct xen_domctl_monitor_op *mop)
+{
+    struct arch_domain *ad = &d->arch;
+    bool_t requested_status = (XEN_DOMCTL_MONITOR_OP_ENABLE == mop->op);
+
+    switch ( mop->event )
+    {
+    case XEN_DOMCTL_MONITOR_EVENT_PRIVILEGED_CALL:
+    {
+        bool_t old_status = ad->monitor.privileged_call_enabled;
+
+        if ( unlikely(old_status == requested_status) )
+            return -EEXIST;
+
+        domain_pause(d);
+        ad->monitor.privileged_call_enabled = requested_status;
+        domain_unpause(d);
+        break;
+    }
+
+    default:
+        /*
+         * Should not be reached unless arch_monitor_get_capabilities() is
+         * not properly implemented.
+         */
+        ASSERT_UNREACHABLE();
+        return -EOPNOTSUPP;
+    }
+
+    return 0;
+}
+
+bool_t monitor_smc(const struct cpu_user_regs *regs)
+{
+    struct vcpu *curr = current;
+    vm_event_request_t req = { 0 };
+
+    if ( !curr->domain->arch.monitor.privileged_call_enabled )
+        return 0;
+
+    req.reason = VM_EVENT_REASON_PRIVILEGED_CALL;
+    req.vcpu_id = curr->vcpu_id;
+
+    if ( vm_event_monitor_traps(curr, 1, &req) <= 0 )
+        return 0;
+    else
+        return 1;
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index aa3e3c2..4167b06 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -42,6 +42,7 @@
 #include <asm/mmio.h>
 #include <asm/cpufeature.h>
 #include <asm/flushtlb.h>
+#include <asm/monitor.h>
 
 #include "decode.h"
 #include "vtimer.h"
@@ -2506,6 +2507,14 @@ bad_data_abort:
     inject_dabt_exception(regs, info.gva, hsr.len);
 }
 
+static void do_trap_smc(struct cpu_user_regs *regs, const union hsr hsr)
+{
+    bool_t handled = monitor_smc(regs);
+
+    if ( !handled )
+        inject_undef_exception(regs, hsr);
+}
+
 static void enter_hypervisor_head(struct cpu_user_regs *regs)
 {
     if ( guest_mode(regs) )
@@ -2581,7 +2590,7 @@ asmlinkage void do_trap_hypervisor(struct cpu_user_regs *regs)
          */
         GUEST_BUG_ON(!psr_mode_is_32bit(regs->cpsr));
         perfc_incr(trap_smc32);
-        inject_undef32_exception(regs);
+        do_trap_smc(regs, hsr);
         break;
     case HSR_EC_HVC32:
         GUEST_BUG_ON(!psr_mode_is_32bit(regs->cpsr));
@@ -2614,7 +2623,7 @@ asmlinkage void do_trap_hypervisor(struct cpu_user_regs *regs)
          */
         GUEST_BUG_ON(psr_mode_is_32bit(regs->cpsr));
         perfc_incr(trap_smc64);
-        inject_undef64_exception(regs, hsr.len);
+        do_trap_smc(regs, hsr);
         break;
     case HSR_EC_SYSREG:
         GUEST_BUG_ON(psr_mode_is_32bit(regs->cpsr));
diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
index 370cdeb..ed56fc9 100644
--- a/xen/include/asm-arm/domain.h
+++ b/xen/include/asm-arm/domain.h
@@ -127,6 +127,11 @@ struct arch_domain
     paddr_t efi_acpi_gpa;
     paddr_t efi_acpi_len;
 #endif
+
+    /* Monitor options */
+    struct {
+        uint8_t privileged_call_enabled : 1;
+    } monitor;
 }  __cacheline_aligned;
 
 struct arch_vcpu
diff --git a/xen/include/asm-arm/monitor.h b/xen/include/asm-arm/monitor.h
index 3fd3c9d..9a10d1f 100644
--- a/xen/include/asm-arm/monitor.h
+++ b/xen/include/asm-arm/monitor.h
@@ -3,7 +3,7 @@
  *
  * Arch-specific monitor_op domctl handler.
  *
- * Copyright (c) 2015 Tamas K Lengyel (tamas@tklengyel.com)
+ * Copyright (c) 2015-2016 Tamas K Lengyel (tamas@tklengyel.com)
  * Copyright (c) 2016, Bitdefender S.R.L.
  *
  * This program is free software; you can redistribute it and/or
@@ -32,27 +32,15 @@ int arch_monitor_domctl_op(struct domain *d, struct xen_domctl_monitor_op *mop)
     return -EOPNOTSUPP;
 }
 
-static inline
 int arch_monitor_domctl_event(struct domain *d,
-                              struct xen_domctl_monitor_op *mop)
-{
-    /*
-     * No arch-specific monitor vm-events on ARM.
-     *
-     * Should not be reached unless arch_monitor_get_capabilities() is not
-     * properly implemented.
-     */
-    ASSERT_UNREACHABLE();
-    return -EOPNOTSUPP;
-}
+                              struct xen_domctl_monitor_op *mop);
 
 static inline uint32_t arch_monitor_get_capabilities(struct domain *d)
 {
-    uint32_t capabilities = 0;
-
-    capabilities = (1U << XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST);
-
-    return capabilities;
+    return (1U << XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST) |
+           (1U << XEN_DOMCTL_MONITOR_EVENT_PRIVILEGED_CALL);
 }
 
+bool_t monitor_smc(const struct cpu_user_regs *regs);
+
 #endif /* __ASM_ARM_MONITOR_H__ */
diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index 2457698..35adce2 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -1080,6 +1080,7 @@ DEFINE_XEN_GUEST_HANDLE(xen_domctl_psr_cmt_op_t);
 #define XEN_DOMCTL_MONITOR_EVENT_SINGLESTEP            2
 #define XEN_DOMCTL_MONITOR_EVENT_SOFTWARE_BREAKPOINT   3
 #define XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST         4
+#define XEN_DOMCTL_MONITOR_EVENT_PRIVILEGED_CALL       5
 
 struct xen_domctl_monitor_op {
     uint32_t op; /* XEN_DOMCTL_MONITOR_OP_* */
diff --git a/xen/include/public/vm_event.h b/xen/include/public/vm_event.h
index 9270d52..3acf217 100644
--- a/xen/include/public/vm_event.h
+++ b/xen/include/public/vm_event.h
@@ -119,6 +119,8 @@
 #define VM_EVENT_REASON_SINGLESTEP              7
 /* An event has been requested via HVMOP_guest_request_vm_event. */
 #define VM_EVENT_REASON_GUEST_REQUEST           8
+/* Privileged call executed (e.g. SMC) */
+#define VM_EVENT_REASON_PRIVILEGED_CALL         9
 
 /* Supported values for the vm_event_write_ctrlreg index. */
 #define VM_EVENT_X86_CR0    0
-- 
2.8.1


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

Re: [PATCH v4 4/8] monitor: ARM SMC events

[Julien Grall]

Hi Tamas,

On 29/05/16 23:37, Tamas K Lengyel wrote:
> Add support for monitoring ARM SMC events. This patch only adds the required
> bits to enable/disable monitoring and forwarding the event through vm_event.

Couple of questions:

- How the vm-event app will differentiate a SMC64 vs a SMC32 call?
- How the vm-event app will know the SMC number (i.e the

In an AArch64 state, those informations are available from the syndrome 
register (HSR_EL2.ISS).

Regards,

-- 
Julien Grall

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

Re: [PATCH v4 4/8] monitor: ARM SMC events

[Tamas K Lengyel]

[-- Attachment #1.1: Type: text/plain, Size: 882 bytes --]

On Jun 1, 2016 05:37, "Julien Grall" <julien.grall@arm.com> wrote:
>
> Hi Tamas,
>
>
> On 29/05/16 23:37, Tamas K Lengyel wrote:
>>
>> Add support for monitoring ARM SMC events. This patch only adds the
required
>> bits to enable/disable monitoring and forwarding the event through
vm_event.
>
>
> Couple of questions:
>
> - How the vm-event app will differentiate a SMC64 vs a SMC32 call?

Wouldn't cpsr record the guest state when the smc was executed, telling us
whether it was 32bit or 64bit?

> - How the vm-event app will know the SMC number (i.e the
>
> In an AArch64 state, those informations are available from the syndrome
register (HSR_EL2.ISS).

Good question, it should probably be sent alongside the other registers. At
the moment my usecase doesn't care about it as I'm just using SMC for
inducing traps to the vmm but other applications may indeed need this.

Tamas

[-- Attachment #1.2: Type: text/html, Size: 1160 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH v4 4/8] monitor: ARM SMC events

[Julien Grall]

Hello Tamas,

On 01/06/16 16:41, Tamas K Lengyel wrote:
>
> On Jun 1, 2016 05:37, "Julien Grall" <julien.grall@arm.com
> <mailto:julien.grall@arm.com>> wrote:
>  > On 29/05/16 23:37, Tamas K Lengyel wrote:
>  >>
>  >> Add support for monitoring ARM SMC events. This patch only adds the
> required
>  >> bits to enable/disable monitoring and forwarding the event through
> vm_event.
>  >
>  >
>  > Couple of questions:
>  >
>  > - How the vm-event app will differentiate a SMC64 vs a SMC32 call?
>
> Wouldn't cpsr record the guest state when the smc was executed, telling
> us whether it was 32bit or 64bit?

You are right.

>  > - How the vm-event app will know the SMC number (i.e the
>  >
>  > In an AArch64 state, those informations are available from the
> syndrome register (HSR_EL2.ISS).
>
> Good question, it should probably be sent alongside the other registers.
> At the moment my usecase doesn't care about it as I'm just using SMC for
> inducing traps to the vmm but other applications may indeed need this.

In this case, we should either provide HSR_EL2, or decode it provide the 
value.

Note that HSR_EL2.ISS is unknown for ARMv7 (see B3-1431 in ARM DDI 0406C.c).

On ARMv8, ESR_EL2.ISS has a different encoding depending on the state 
(i.e AArch64 or AArch32). See D7-1861 in ARM DDI 0487A.i. Furthermore, 
in AArch32 state, the ARMv8 specs permits to trap conditional SMC 
instructions that fail their condition check (see D7-1897 in ARM DDI 
0406C.c).

So I guess, this need to be abstract in Xen to avoid burden to the 
introspection app.

Regards,

-- 
Julien Grall

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

Re: [PATCH v4 4/8] monitor: ARM SMC events

[Tamas K Lengyel]

On Thu, Jun 2, 2016 at 8:23 AM, Julien Grall <julien.grall@arm.com> wrote:
>
> Hello Tamas,
>
> On 01/06/16 16:41, Tamas K Lengyel wrote:
>>
>>
>> On Jun 1, 2016 05:37, "Julien Grall" <julien.grall@arm.com
>> <mailto:julien.grall@arm.com>> wrote:
>>  > On 29/05/16 23:37, Tamas K Lengyel wrote:
>>  >>
>>  >> Add support for monitoring ARM SMC events. This patch only adds the
>> required
>>  >> bits to enable/disable monitoring and forwarding the event through
>> vm_event.
>>  >
>>  >
>>  > Couple of questions:
>>  >
>>  > - How the vm-event app will differentiate a SMC64 vs a SMC32 call?
>>
>> Wouldn't cpsr record the guest state when the smc was executed, telling
>> us whether it was 32bit or 64bit?
>
>
> You are right.
>
>>  > - How the vm-event app will know the SMC number (i.e the
>>  >
>>  > In an AArch64 state, those informations are available from the
>> syndrome register (HSR_EL2.ISS).
>>
>> Good question, it should probably be sent alongside the other registers.
>> At the moment my usecase doesn't care about it as I'm just using SMC for
>> inducing traps to the vmm but other applications may indeed need this.
>
>
> In this case, we should either provide HSR_EL2, or decode it provide the
> value.
>
> Note that HSR_EL2.ISS is unknown for ARMv7 (see B3-1431 in ARM DDI 0406C.c).
>
> On ARMv8, ESR_EL2.ISS has a different encoding depending on the state (i.e
> AArch64 or AArch32). See D7-1861 in ARM DDI 0487A.i. Furthermore, in AArch32
> state, the ARMv8 specs permits to trap conditional SMC instructions that
> fail their condition check (see D7-1897 in ARM DDI 0406C.c).
>
> So I guess, this need to be abstract in Xen to avoid burden to the
> introspection app.

So during the trap ESR_EL2 is already read into const union hsr hsr,
so my thinking was to just submit hsr.iss to the introspection app and
let it deal with decoding it if it needs to.

Tamas

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

Re: [PATCH v4 4/8] monitor: ARM SMC events

[Tamas K Lengyel]

> On ARMv8, ESR_EL2.ISS has a different encoding depending on the state (i.e
> AArch64 or AArch32). See D7-1861 in ARM DDI 0487A.i. Furthermore, in AArch32
> state, the ARMv8 specs permits to trap conditional SMC instructions that
> fail their condition check (see D7-1897 in ARM DDI 0406C.c).

Hi Julien,
I'm unable to find the reference you mention about conditional SMCs
generating Hyp traps even if they fail their condition checks on
certain implementations. The only references I find in ARMv8 manual
states explicitly that "The trap that HCR_EL2.TSC enables traps the
attempted execution of a conditional SMC instruction only if the
instruction passes its condition code check" (D1.15) and "The
architecture requires that a Hyp trap on a conditional SMC instruction
generates an exception only if the instruction passes its condition
code check, see Trapping use of the SMC instruction on page G1-3510"
(G1.16).

Tamas

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

Re: [PATCH v4 4/8] monitor: ARM SMC events

[Julien Grall]

On 04/07/2016 20:13, Tamas K Lengyel wrote:
>> On ARMv8, ESR_EL2.ISS has a different encoding depending on the state (i.e
>> AArch64 or AArch32). See D7-1861 in ARM DDI 0487A.i. Furthermore, in AArch32
>> state, the ARMv8 specs permits to trap conditional SMC instructions that
>> fail their condition check (see D7-1897 in ARM DDI 0406C.c).
>
> Hi Julien,

Hello Tamas,

> I'm unable to find the reference you mention about conditional SMCs
> generating Hyp traps even if they fail their condition checks on
> certain implementations. The only references I find in ARMv8 manual
> states explicitly that "The trap that HCR_EL2.TSC enables traps the
> attempted execution of a conditional SMC instruction only if the
> instruction passes its condition code check" (D1.15) and "The
> architecture requires that a Hyp trap on a conditional SMC instruction
> generates an exception only if the instruction passes its condition
> code check, see Trapping use of the SMC instruction on page G1-3510"
> (G1.16).

If you look at description of HCR_EL2.TSC at D7-1973 in DDI 0487A.j;
"In AArch32 state, the ARMv8-A architecture permits, but does not 
require, this trap to apply to conditional SMC instructions that fail 
their condition code check, in the same way as with traps on
other conditional instructions."

AFAICT the change was already present in the ARM ARM beginning of 2015. 
So I suspect you are using an old version of the spec. In general, I 
would recommend you to use the most recent spec for your development.

For future question related to the spec, please mention the version of 
the spec you quote. It makes easier to find out what is going on.

Regards,

-- 
Julien Grall

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

Re: [PATCH v4 4/8] monitor: ARM SMC events

[Tamas K Lengyel]

On Mon, Jul 4, 2016 at 2:02 PM, Julien Grall <julien.grall@arm.com> wrote:
>
>
> On 04/07/2016 20:13, Tamas K Lengyel wrote:
>>>
>>> On ARMv8, ESR_EL2.ISS has a different encoding depending on the state
>>> (i.e
>>> AArch64 or AArch32). See D7-1861 in ARM DDI 0487A.i. Furthermore, in
>>> AArch32
>>> state, the ARMv8 specs permits to trap conditional SMC instructions that
>>> fail their condition check (see D7-1897 in ARM DDI 0406C.c).
>>
>>
>> Hi Julien,
>
>
> Hello Tamas,
>
>> I'm unable to find the reference you mention about conditional SMCs
>> generating Hyp traps even if they fail their condition checks on
>> certain implementations. The only references I find in ARMv8 manual
>> states explicitly that "The trap that HCR_EL2.TSC enables traps the
>> attempted execution of a conditional SMC instruction only if the
>> instruction passes its condition code check" (D1.15) and "The
>> architecture requires that a Hyp trap on a conditional SMC instruction
>> generates an exception only if the instruction passes its condition
>> code check, see Trapping use of the SMC instruction on page G1-3510"
>> (G1.16).
>
>
> If you look at description of HCR_EL2.TSC at D7-1973 in DDI 0487A.j;
> "In AArch32 state, the ARMv8-A architecture permits, but does not require,
> this trap to apply to conditional SMC instructions that fail their condition
> code check, in the same way as with traps on
> other conditional instructions."
>
> AFAICT the change was already present in the ARM ARM beginning of 2015. So I
> suspect you are using an old version of the spec. In general, I would
> recommend you to use the most recent spec for your development.
>
> For future question related to the spec, please mention the version of the
> spec you quote. It makes easier to find out what is going on.

Ah yes, indeed, I was looking at an older version. From the manual
though it is not clear to me how we can differentiate SMCs that had
their condition check fail from the others once its trapped to the
VMM.. Are you still planning to implement that filter?

Thanks,
Tamas

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

Re: [PATCH v4 4/8] monitor: ARM SMC events

[Julien Grall]

Hello Tamas,

On 04/07/16 22:05, Tamas K Lengyel wrote:
> On Mon, Jul 4, 2016 at 2:02 PM, Julien Grall <julien.grall@arm.com> wrote:
>>
>>
>> On 04/07/2016 20:13, Tamas K Lengyel wrote:
>>>>
>>>> On ARMv8, ESR_EL2.ISS has a different encoding depending on the state
>>>> (i.e
>>>> AArch64 or AArch32). See D7-1861 in ARM DDI 0487A.i. Furthermore, in
>>>> AArch32
>>>> state, the ARMv8 specs permits to trap conditional SMC instructions that
>>>> fail their condition check (see D7-1897 in ARM DDI 0406C.c).

[...]

>>> I'm unable to find the reference you mention about conditional SMCs
>>> generating Hyp traps even if they fail their condition checks on
>>> certain implementations. The only references I find in ARMv8 manual
>>> states explicitly that "The trap that HCR_EL2.TSC enables traps the
>>> attempted execution of a conditional SMC instruction only if the
>>> instruction passes its condition code check" (D1.15) and "The
>>> architecture requires that a Hyp trap on a conditional SMC instruction
>>> generates an exception only if the instruction passes its condition
>>> code check, see Trapping use of the SMC instruction on page G1-3510"
>>> (G1.16).
>>
>>
>> If you look at description of HCR_EL2.TSC at D7-1973 in DDI 0487A.j;
>> "In AArch32 state, the ARMv8-A architecture permits, but does not require,
>> this trap to apply to conditional SMC instructions that fail their condition
>> code check, in the same way as with traps on
>> other conditional instructions."
>>
>> AFAICT the change was already present in the ARM ARM beginning of 2015. So I
>> suspect you are using an old version of the spec. In general, I would
>> recommend you to use the most recent spec for your development.
>>
>> For future question related to the spec, please mention the version of the
>> spec you quote. It makes easier to find out what is going on.
>
> Ah yes, indeed, I was looking at an older version. From the manual
> though it is not clear to me how we can differentiate SMCs that had
> their condition check fail from the others once its trapped to the
> VMM..

It would be very similar to how Xen is checking if a conditional 
instruction fails its condition check (see check_conditional_instr).

> Are you still planning to implement that filter?

It is in my todo list, however at the moment it towards the end. Feel 
free to send  patch yourself.

Regards,

-- 
Julien Grall

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

[PATCH v4 5/8] arm/vm_event: get/set registers

[Tamas K Lengyel]

Add support for getting/setting registers through vm_event on ARM.

Signed-off-by: Tamas K Lengyel <tamas@tklengyel.com>
---
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Julien Grall <julien.grall@arm.com>
Cc: Razvan Cojocaru <rcojocaru@bitdefender.com>

v4: Use psr mode to determine whether to full 32-bit or 64-bit structs
---
 xen/arch/arm/Makefile          |   1 +
 xen/arch/arm/vm_event.c        | 139 +++++++++++++++++++++++++++++++++++++++++
 xen/include/asm-arm/vm_event.h |  11 ----
 xen/include/asm-x86/vm_event.h |   4 --
 xen/include/public/vm_event.h  |  58 ++++++++++++++++-
 xen/include/xen/vm_event.h     |   3 +
 6 files changed, 199 insertions(+), 17 deletions(-)
 create mode 100644 xen/arch/arm/vm_event.c

diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
index 344d3ad..7d2641c 100644
--- a/xen/arch/arm/Makefile
+++ b/xen/arch/arm/Makefile
@@ -42,6 +42,7 @@ obj-y += processor.o
 obj-y += smc.o
 obj-$(CONFIG_XSPLICE) += xsplice.o
 obj-y += monitor.o
+obj-y += vm_event.o
 
 #obj-bin-y += ....o
 
diff --git a/xen/arch/arm/vm_event.c b/xen/arch/arm/vm_event.c
new file mode 100644
index 0000000..dcf9f1c
--- /dev/null
+++ b/xen/arch/arm/vm_event.c
@@ -0,0 +1,139 @@
+/*
+ * arch/arm/vm_event.c
+ *
+ * Architecture-specific vm_event handling routines
+ *
+ * Copyright (c) 2016 Tamas K Lengyel (tamas@tklengyel.com)
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public
+ * License v2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public
+ * License along with this program; If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <xen/sched.h>
+#include <asm/vm_event.h>
+
+void vm_event_fill_regs(vm_event_request_t *req)
+{
+    const struct cpu_user_regs *regs = guest_cpu_user_regs();
+
+    req->data.regs.arm.cpsr = regs->cpsr;
+    req->data.regs.arm.ttbr0 = READ_SYSREG64(TTBR0_EL1);
+    req->data.regs.arm.ttbr1 = READ_SYSREG64(TTBR1_EL1);
+
+    if ( psr_mode_is_32bit(regs->cpsr) )
+    {
+        req->data.regs.arm.arch.arm32.r0_usr = regs->r0;
+        req->data.regs.arm.arch.arm32.r1_usr = regs->r1;
+        req->data.regs.arm.arch.arm32.r2_usr = regs->r2;
+        req->data.regs.arm.arch.arm32.r3_usr = regs->r3;
+        req->data.regs.arm.arch.arm32.r4_usr = regs->r4;
+        req->data.regs.arm.arch.arm32.r5_usr = regs->r5;
+        req->data.regs.arm.arch.arm32.r6_usr = regs->r6;
+        req->data.regs.arm.arch.arm32.r7_usr = regs->r7;
+        req->data.regs.arm.arch.arm32.r8_usr = regs->r8;
+        req->data.regs.arm.arch.arm32.r9_usr = regs->r9;
+        req->data.regs.arm.arch.arm32.r10_usr = regs->r10;
+        req->data.regs.arm.arch.arm32.r12_usr = regs->r12;
+        req->data.regs.arm.arch.arm32.lr_usr = regs->lr_usr;
+        req->data.regs.arm.arch.arm32.pc = regs->pc32;
+        req->data.regs.arm.arch.arm32.fp = regs->fp;
+        req->data.regs.arm.arch.arm32.sp_usr = regs->sp_usr;
+        req->data.regs.arm.arch.arm32.sp_svc = regs->sp_svc;
+        req->data.regs.arm.arch.arm32.spsr_svc = regs->spsr_svc;
+    }
+#ifdef CONFIG_ARM_64
+    else
+    {
+        req->data.regs.arm.arch.arm64.x0 = regs->x0;
+        req->data.regs.arm.arch.arm64.x1 = regs->x1;
+        req->data.regs.arm.arch.arm64.x2 = regs->x2;
+        req->data.regs.arm.arch.arm64.x3 = regs->x3;
+        req->data.regs.arm.arch.arm64.x4 = regs->x4;
+        req->data.regs.arm.arch.arm64.x5 = regs->x5;
+        req->data.regs.arm.arch.arm64.x6 = regs->x6;
+        req->data.regs.arm.arch.arm64.x7 = regs->x7;
+        req->data.regs.arm.arch.arm64.x8 = regs->x8;
+        req->data.regs.arm.arch.arm64.x9 = regs->x9;
+        req->data.regs.arm.arch.arm64.x10 = regs->x10;
+        req->data.regs.arm.arch.arm64.x16 = regs->x16;
+        req->data.regs.arm.arch.arm64.pc = regs->pc;
+        req->data.regs.arm.arch.arm64.sp_el0 = regs->sp_el0;
+        req->data.regs.arm.arch.arm64.sp_el1 = regs->sp_el1;
+        req->data.regs.arm.arch.arm64.fp = regs->fp;
+        req->data.regs.arm.arch.arm64.lr = regs->lr;
+        req->data.regs.arm.arch.arm64.spsr_el1 = regs->spsr_svc;
+    }
+#endif
+}
+
+void vm_event_set_registers(struct vcpu *v, vm_event_response_t *rsp)
+{
+    struct cpu_user_regs *regs = &v->arch.cpu_info->guest_cpu_user_regs;
+
+    regs->cpsr = rsp->data.regs.arm.cpsr;
+    v->arch.ttbr0 = rsp->data.regs.arm.ttbr0;
+    v->arch.ttbr1 = rsp->data.regs.arm.ttbr1;
+
+    if ( psr_mode_is_32bit(regs->cpsr) )
+    {
+        regs->r0 = rsp->data.regs.arm.arch.arm32.r0_usr;
+        regs->r1 = rsp->data.regs.arm.arch.arm32.r1_usr;
+        regs->r2 = rsp->data.regs.arm.arch.arm32.r2_usr;
+        regs->r3 = rsp->data.regs.arm.arch.arm32.r3_usr;
+        regs->r4 = rsp->data.regs.arm.arch.arm32.r4_usr;
+        regs->r5 = rsp->data.regs.arm.arch.arm32.r5_usr;
+        regs->r6 = rsp->data.regs.arm.arch.arm32.r6_usr;
+        regs->r7 = rsp->data.regs.arm.arch.arm32.r7_usr;
+        regs->r8 = rsp->data.regs.arm.arch.arm32.r8_usr;
+        regs->r9 = rsp->data.regs.arm.arch.arm32.r9_usr;
+        regs->r10 = rsp->data.regs.arm.arch.arm32.r10_usr;
+        regs->r12 = rsp->data.regs.arm.arch.arm32.r12_usr;
+        regs->pc32 = rsp->data.regs.arm.arch.arm32.pc;
+        regs->fp = rsp->data.regs.arm.arch.arm32.fp;
+        regs->lr_usr = rsp->data.regs.arm.arch.arm32.lr_usr;
+        regs->sp_usr = rsp->data.regs.arm.arch.arm32.sp_usr;
+        regs->sp_svc = rsp->data.regs.arm.arch.arm32.sp_svc;
+        regs->spsr_svc = rsp->data.regs.arm.arch.arm32.spsr_svc;
+    }
+#ifdef CONFIG_ARM_64
+    else
+    {
+        regs->x0 = rsp->data.regs.arm.arch.arm64.x0;
+        regs->x1 = rsp->data.regs.arm.arch.arm64.x1;
+        regs->x2 = rsp->data.regs.arm.arch.arm64.x2;
+        regs->x3 = rsp->data.regs.arm.arch.arm64.x3;
+        regs->x4 = rsp->data.regs.arm.arch.arm64.x4;
+        regs->x5 = rsp->data.regs.arm.arch.arm64.x5;
+        regs->x6 = rsp->data.regs.arm.arch.arm64.x6;
+        regs->x7 = rsp->data.regs.arm.arch.arm64.x7;
+        regs->x8 = rsp->data.regs.arm.arch.arm64.x8;
+        regs->x9 = rsp->data.regs.arm.arch.arm64.x9;
+        regs->x10 = rsp->data.regs.arm.arch.arm64.x10;
+        regs->x16 = rsp->data.regs.arm.arch.arm64.x16;
+        regs->pc = rsp->data.regs.arm.arch.arm64.pc;
+        regs->sp_el0 = rsp->data.regs.arm.arch.arm64.sp_el0;
+        regs->sp_el1 = rsp->data.regs.arm.arch.arm64.sp_el1;
+        regs->fp = rsp->data.regs.arm.arch.arm64.fp;
+        regs->lr = rsp->data.regs.arm.arch.arm64.lr;
+        regs->spsr_svc = rsp->data.regs.arm.arch.arm64.spsr_el1;
+    }
+#endif
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/include/asm-arm/vm_event.h b/xen/include/asm-arm/vm_event.h
index a3fc4ce..a4922b3 100644
--- a/xen/include/asm-arm/vm_event.h
+++ b/xen/include/asm-arm/vm_event.h
@@ -48,15 +48,4 @@ void vm_event_register_write_resume(struct vcpu *v, vm_event_response_t *rsp)
     /* Not supported on ARM. */
 }
 
-static inline
-void vm_event_set_registers(struct vcpu *v, vm_event_response_t *rsp)
-{
-    /* Not supported on ARM. */
-}
-
-static inline void vm_event_fill_regs(vm_event_request_t *req)
-{
-    /* Not supported on ARM. */
-}
-
 #endif /* __ASM_ARM_VM_EVENT_H__ */
diff --git a/xen/include/asm-x86/vm_event.h b/xen/include/asm-x86/vm_event.h
index 026f42e..cf2077c 100644
--- a/xen/include/asm-x86/vm_event.h
+++ b/xen/include/asm-x86/vm_event.h
@@ -40,8 +40,4 @@ void vm_event_toggle_singlestep(struct domain *d, struct vcpu *v);
 
 void vm_event_register_write_resume(struct vcpu *v, vm_event_response_t *rsp);
 
-void vm_event_set_registers(struct vcpu *v, vm_event_response_t *rsp);
-
-void vm_event_fill_regs(vm_event_request_t *req);
-
 #endif /* __ASM_X86_VM_EVENT_H__ */
diff --git a/xen/include/public/vm_event.h b/xen/include/public/vm_event.h
index 3acf217..6ff7cc0 100644
--- a/xen/include/public/vm_event.h
+++ b/xen/include/public/vm_event.h
@@ -129,8 +129,8 @@
 #define VM_EVENT_X86_XCR0   3
 
 /*
- * Using a custom struct (not hvm_hw_cpu) so as to not fill
- * the vm_event ring buffer too quickly.
+ * Using custom vCPU structs (i.e. not hvm_hw_cpu) for both x86 and ARM
+ * so as to not fill the vm_event ring buffer too quickly.
  */
 struct vm_event_regs_x86 {
     uint64_t rax;
@@ -168,6 +168,59 @@ struct vm_event_regs_x86 {
     uint32_t _pad;
 };
 
+struct vm_event_regs_arm32 {
+    uint32_t r0_usr;
+    uint32_t r1_usr;
+    uint32_t r2_usr;
+    uint32_t r3_usr;
+    uint32_t r4_usr;
+    uint32_t r5_usr;
+    uint32_t r6_usr;
+    uint32_t r7_usr;
+    uint32_t r8_usr;
+    uint32_t r9_usr;
+    uint32_t r10_usr;
+    uint32_t r12_usr;
+    uint32_t lr_usr;
+    uint32_t fp;
+    uint32_t pc;
+    uint32_t sp_usr;
+    uint32_t sp_svc;
+    uint32_t spsr_svc;
+};
+
+struct vm_event_regs_arm64 {
+    uint64_t x0;
+    uint64_t x1;
+    uint64_t x2;
+    uint64_t x3;
+    uint64_t x4;
+    uint64_t x5;
+    uint64_t x6;
+    uint64_t x7;
+    uint64_t x8;
+    uint64_t x9;
+    uint64_t x10;
+    uint64_t x16;
+    uint64_t lr;
+    uint64_t fp;
+    uint64_t pc;
+    uint64_t sp_el0;
+    uint64_t sp_el1;
+    uint32_t spsr_el1;
+    uint32_t _pad;
+};
+
+struct vm_event_regs_arm {
+    uint32_t cpsr; /* PSR_MODE_BIT is set iff arm32 is used below */
+    uint64_t ttbr0;
+    uint64_t ttbr1;
+    union {
+        struct vm_event_regs_arm32 arm32;
+        struct vm_event_regs_arm64 arm64;
+    } arch;
+};
+
 /*
  * mem_access flag definitions
  *
@@ -256,6 +309,7 @@ typedef struct vm_event_st {
     union {
         union {
             struct vm_event_regs_x86 x86;
+            struct vm_event_regs_arm arm;
         } regs;
 
         struct vm_event_emul_read_data emul_read_data;
diff --git a/xen/include/xen/vm_event.h b/xen/include/xen/vm_event.h
index 89e6243..a5767ab 100644
--- a/xen/include/xen/vm_event.h
+++ b/xen/include/xen/vm_event.h
@@ -75,6 +75,9 @@ int vm_event_domctl(struct domain *d, xen_domctl_vm_event_op_t *vec,
 void vm_event_vcpu_pause(struct vcpu *v);
 void vm_event_vcpu_unpause(struct vcpu *v);
 
+void vm_event_fill_regs(vm_event_request_t *req);
+void vm_event_set_registers(struct vcpu *v, vm_event_response_t *rsp);
+
 /*
  * Monitor vm-events
  */
-- 
2.8.1


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

Re: [PATCH v4 5/8] arm/vm_event: get/set registers

[Razvan Cojocaru]

On 05/30/2016 01:37 AM, Tamas K Lengyel wrote:
> Add support for getting/setting registers through vm_event on ARM.
> 
> Signed-off-by: Tamas K Lengyel <tamas@tklengyel.com>
> ---
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Julien Grall <julien.grall@arm.com>
> Cc: Razvan Cojocaru <rcojocaru@bitdefender.com>
> 
> v4: Use psr mode to determine whether to full 32-bit or 64-bit structs
> ---
>  xen/arch/arm/Makefile          |   1 +
>  xen/arch/arm/vm_event.c        | 139 +++++++++++++++++++++++++++++++++++++++++
>  xen/include/asm-arm/vm_event.h |  11 ----
>  xen/include/asm-x86/vm_event.h |   4 --
>  xen/include/public/vm_event.h  |  58 ++++++++++++++++-
>  xen/include/xen/vm_event.h     |   3 +
>  6 files changed, 199 insertions(+), 17 deletions(-)
>  create mode 100644 xen/arch/arm/vm_event.c

Acked-by: Razvan Cojocaru <rcojocaru@bitdefender.com>

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

Re: [PATCH v4 5/8] arm/vm_event: get/set registers

[Jan Beulich]

>>> On 30.05.16 at 00:37, <tamas@tklengyel.com> wrote:
> +struct vm_event_regs_arm32 {
> +    uint32_t r0_usr;
> +    uint32_t r1_usr;
> +    uint32_t r2_usr;
> +    uint32_t r3_usr;
> +    uint32_t r4_usr;
> +    uint32_t r5_usr;
> +    uint32_t r6_usr;
> +    uint32_t r7_usr;
> +    uint32_t r8_usr;
> +    uint32_t r9_usr;
> +    uint32_t r10_usr;
> +    uint32_t r12_usr;
> +    uint32_t lr_usr;
> +    uint32_t fp;
> +    uint32_t pc;
> +    uint32_t sp_usr;
> +    uint32_t sp_svc;
> +    uint32_t spsr_svc;
> +};

It would seem more natural for the "ordinary" registers to be
arranged in the numerical sequence, i.e. fp, r12, sp, lr, pc.

> +struct vm_event_regs_arm64 {
> +    uint64_t x0;
> +    uint64_t x1;
> +    uint64_t x2;
> +    uint64_t x3;
> +    uint64_t x4;
> +    uint64_t x5;
> +    uint64_t x6;
> +    uint64_t x7;
> +    uint64_t x8;
> +    uint64_t x9;
> +    uint64_t x10;
> +    uint64_t x16;
> +    uint64_t lr;
> +    uint64_t fp;
> +    uint64_t pc;
> +    uint64_t sp_el0;
> +    uint64_t sp_el1;
> +    uint32_t spsr_el1;
> +    uint32_t _pad;
> +};

My ARM knowledge is certainly quite limited, but the incomplete set
of GPRs here is quite obvious: Is there a reason to not make all of
them available here? And if there is, could the criteria of which
registers got put here please be documented in a comment (so that
the next person noticing the incomplete set won't ask again)?

I'm also puzzled by fp and lr - I'm not aware of registers of those
names (and gas also doesn't accept them afaict).

> +struct vm_event_regs_arm {
> +    uint32_t cpsr; /* PSR_MODE_BIT is set iff arm32 is used below */

Explicit padding missing after this one.

Jan


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

Re: [PATCH v4 5/8] arm/vm_event: get/set registers

[Tamas K Lengyel]

On Mon, May 30, 2016 at 5:50 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 30.05.16 at 00:37, <tamas@tklengyel.com> wrote:
>> +struct vm_event_regs_arm32 {
>> +    uint32_t r0_usr;
>> +    uint32_t r1_usr;
>> +    uint32_t r2_usr;
>> +    uint32_t r3_usr;
>> +    uint32_t r4_usr;
>> +    uint32_t r5_usr;
>> +    uint32_t r6_usr;
>> +    uint32_t r7_usr;
>> +    uint32_t r8_usr;
>> +    uint32_t r9_usr;
>> +    uint32_t r10_usr;
>> +    uint32_t r12_usr;
>> +    uint32_t lr_usr;
>> +    uint32_t fp;
>> +    uint32_t pc;
>> +    uint32_t sp_usr;
>> +    uint32_t sp_svc;
>> +    uint32_t spsr_svc;
>> +};
>
> It would seem more natural for the "ordinary" registers to be
> arranged in the numerical sequence, i.e. fp, r12, sp, lr, pc.

Not sure I follow.

>
>> +struct vm_event_regs_arm64 {
>> +    uint64_t x0;
>> +    uint64_t x1;
>> +    uint64_t x2;
>> +    uint64_t x3;
>> +    uint64_t x4;
>> +    uint64_t x5;
>> +    uint64_t x6;
>> +    uint64_t x7;
>> +    uint64_t x8;
>> +    uint64_t x9;
>> +    uint64_t x10;
>> +    uint64_t x16;
>> +    uint64_t lr;
>> +    uint64_t fp;
>> +    uint64_t pc;
>> +    uint64_t sp_el0;
>> +    uint64_t sp_el1;
>> +    uint32_t spsr_el1;
>> +    uint32_t _pad;
>> +};
>
> My ARM knowledge is certainly quite limited, but the incomplete set
> of GPRs here is quite obvious: Is there a reason to not make all of
> them available here? And if there is, could the criteria of which
> registers got put here please be documented in a comment (so that
> the next person noticing the incomplete set won't ask again)?

There already is a comment in place that explains why we are not
sending the full set of registers here.

>
> I'm also puzzled by fp and lr - I'm not aware of registers of those
> names (and gas also doesn't accept them afaict).

Not sure why but Xen names x29 fp and x30 lr. See
include/asm-arm/arm64/processor.h.

>
>> +struct vm_event_regs_arm {
>> +    uint32_t cpsr; /* PSR_MODE_BIT is set iff arm32 is used below */
>
> Explicit padding missing after this one.

Ack.

Thanks,
Tamas

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

Re: [PATCH v4 5/8] arm/vm_event: get/set registers

[Julien Grall]

Hi Tamas,

On 30/05/2016 20:47, Tamas K Lengyel wrote:
> On Mon, May 30, 2016 at 5:50 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>> +struct vm_event_regs_arm64 {
>>> +    uint64_t x0;
>>> +    uint64_t x1;
>>> +    uint64_t x2;
>>> +    uint64_t x3;
>>> +    uint64_t x4;
>>> +    uint64_t x5;
>>> +    uint64_t x6;
>>> +    uint64_t x7;
>>> +    uint64_t x8;
>>> +    uint64_t x9;
>>> +    uint64_t x10;
>>> +    uint64_t x16;
>>> +    uint64_t lr;
>>> +    uint64_t fp;
>>> +    uint64_t pc;
>>> +    uint64_t sp_el0;
>>> +    uint64_t sp_el1;
>>> +    uint32_t spsr_el1;
>>> +    uint32_t _pad;
>>> +};
>>
>> My ARM knowledge is certainly quite limited, but the incomplete set
>> of GPRs here is quite obvious: Is there a reason to not make all of
>> them available here? And if there is, could the criteria of which
>> registers got put here please be documented in a comment (so that
>> the next person noticing the incomplete set won't ask again)?
>
> There already is a comment in place that explains why we are not
> sending the full set of registers here.

Your comment only says:
"Using custom vCPU structs (i.e. not hvm_hw_cpu) for both x86 and ARM
so as to not fill the vm_event ring buffer too quickly." it does not 
explain the criteria of which registers got put here.

I already mentioned it on a previous version, and it seems to have been 
dropped from the commit message.

Anyway, we should avoid to be stingy with comments. They are helpful for 
anyone interested by the vm_event subsystem and even you in few years if 
you decide to modify this code.

>>
>> I'm also puzzled by fp and lr - I'm not aware of registers of those
>> names (and gas also doesn't accept them afaict).
>
> Not sure why but Xen names x29 fp and x30 lr. See
> include/asm-arm/arm64/processor.h.

They are not officially called "lr" and "fp" in the ARM ARM. However the 
AAPCS64 [1] (5.1.1) defines x29 and x30 as special registers holding 
resp. "lr" and "fp".

The convention is used internally in Xen for convenience. However, the 
public header uses x29 and x30 (see include/public/arch-arm.h).

I would prefer if you use the official name in the public headers.

Regards,

[1] 
http://infocenter.arm.com/help/topic/com.arm.doc.ihi0055b/IHI0055B_aapcs64.pdf


-- 
Julien Grall

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

Re: [PATCH v4 5/8] arm/vm_event: get/set registers

[Tamas K Lengyel]

On Mon, May 30, 2016 at 2:20 PM, Julien Grall <julien.grall@arm.com> wrote:
> Hi Tamas,
>
> On 30/05/2016 20:47, Tamas K Lengyel wrote:
>>
>> On Mon, May 30, 2016 at 5:50 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>
>>>> +struct vm_event_regs_arm64 {
>>>> +    uint64_t x0;
>>>> +    uint64_t x1;
>>>> +    uint64_t x2;
>>>> +    uint64_t x3;
>>>> +    uint64_t x4;
>>>> +    uint64_t x5;
>>>> +    uint64_t x6;
>>>> +    uint64_t x7;
>>>> +    uint64_t x8;
>>>> +    uint64_t x9;
>>>> +    uint64_t x10;
>>>> +    uint64_t x16;
>>>> +    uint64_t lr;
>>>> +    uint64_t fp;
>>>> +    uint64_t pc;
>>>> +    uint64_t sp_el0;
>>>> +    uint64_t sp_el1;
>>>> +    uint32_t spsr_el1;
>>>> +    uint32_t _pad;
>>>> +};
>>>
>>>
>>> My ARM knowledge is certainly quite limited, but the incomplete set
>>> of GPRs here is quite obvious: Is there a reason to not make all of
>>> them available here? And if there is, could the criteria of which
>>> registers got put here please be documented in a comment (so that
>>> the next person noticing the incomplete set won't ask again)?
>>
>>
>> There already is a comment in place that explains why we are not
>> sending the full set of registers here.
>
>
> Your comment only says:
> "Using custom vCPU structs (i.e. not hvm_hw_cpu) for both x86 and ARM
> so as to not fill the vm_event ring buffer too quickly." it does not explain
> the criteria of which registers got put here.

Well, as we discussed it in the previous revision, there is no
hard-set rule of what can and cannot be transmitted here. The only
thing to keep in mind is to not grow this struct to be too large. The
registers sent right now represent a "best guess" of what may be
useful for performance-sensitive vm_event applications on ARM. It can
be adjusted in the future if applications require other registers.
Right now there are no applications at all in this space so we don't
have any specifications to rely on for making this selection. I'm
happy to make adjustments to the selection if others have a better
idea what to send, the only registers I certainly find very useful are
TTBR0/1, cpsr and pc at this time.

>
> I already mentioned it on a previous version, and it seems to have been
> dropped from the commit message.
>
> Anyway, we should avoid to be stingy with comments. They are helpful for
> anyone interested by the vm_event subsystem and even you in few years if you
> decide to modify this code.

Certainly, I would be happy to put the above description into a comment here.

>
>>>
>>> I'm also puzzled by fp and lr - I'm not aware of registers of those
>>> names (and gas also doesn't accept them afaict).
>>
>>
>> Not sure why but Xen names x29 fp and x30 lr. See
>> include/asm-arm/arm64/processor.h.
>
>
> They are not officially called "lr" and "fp" in the ARM ARM. However the
> AAPCS64 [1] (5.1.1) defines x29 and x30 as special registers holding resp.
> "lr" and "fp".
>
> The convention is used internally in Xen for convenience. However, the
> public header uses x29 and x30 (see include/public/arch-arm.h).
>
> I would prefer if you use the official name in the public headers.

Sounds good to me.

Thanks,
Tamas

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

Re: [PATCH v4 5/8] arm/vm_event: get/set registers

[Razvan Cojocaru]

On 05/30/16 23:37, Tamas K Lengyel wrote:
> Well, as we discussed it in the previous revision, there is no
> hard-set rule of what can and cannot be transmitted here. The only
> thing to keep in mind is to not grow this struct to be too large. The
> registers sent right now represent a "best guess" of what may be
> useful for performance-sensitive vm_event applications on ARM. It can
> be adjusted in the future if applications require other registers.

Obviously I agree with Tamas here, my only comment would be that in my
humble opinion multipage vm_event ring buffers are probably the way to
go in the long run. I'm not sure if other parts of Xen already employ
such devices (or if indeed there's support for this at all at this
time), but this might be something worth looking into.

Again, this does not imply that there's anything wrong with this patch
or that this is an issue this series should address.


Thanks,
Razvan

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

Re: [PATCH v4 5/8] arm/vm_event: get/set registers

[Tamas K Lengyel]

On Mon, May 30, 2016 at 2:46 PM, Razvan Cojocaru
<rcojocaru@bitdefender.com> wrote:
> On 05/30/16 23:37, Tamas K Lengyel wrote:
>> Well, as we discussed it in the previous revision, there is no
>> hard-set rule of what can and cannot be transmitted here. The only
>> thing to keep in mind is to not grow this struct to be too large. The
>> registers sent right now represent a "best guess" of what may be
>> useful for performance-sensitive vm_event applications on ARM. It can
>> be adjusted in the future if applications require other registers.
>
> Obviously I agree with Tamas here, my only comment would be that in my
> humble opinion multipage vm_event ring buffers are probably the way to
> go in the long run. I'm not sure if other parts of Xen already employ
> such devices (or if indeed there's support for this at all at this
> time), but this might be something worth looking into.
>
> Again, this does not imply that there's anything wrong with this patch
> or that this is an issue this series should address.

Yeap, something ackin to libvchan would be very handy to have for
guest-hypervisor communication as well. I recall it popping up on the
mailinglist before, not sure if there was any consensus on it in the
end.

Tamas

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

Re: [PATCH v4 5/8] arm/vm_event: get/set registers

[Julien Grall]

On 30/05/2016 21:37, Tamas K Lengyel wrote:
> On Mon, May 30, 2016 at 2:20 PM, Julien Grall <julien.grall@arm.com> wrote:
>> Hi Tamas,
>>
>> On 30/05/2016 20:47, Tamas K Lengyel wrote:
>>>
>>> On Mon, May 30, 2016 at 5:50 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>
>>>>> +struct vm_event_regs_arm64 {
>>>>> +    uint64_t x0;
>>>>> +    uint64_t x1;
>>>>> +    uint64_t x2;
>>>>> +    uint64_t x3;
>>>>> +    uint64_t x4;
>>>>> +    uint64_t x5;
>>>>> +    uint64_t x6;
>>>>> +    uint64_t x7;
>>>>> +    uint64_t x8;
>>>>> +    uint64_t x9;
>>>>> +    uint64_t x10;
>>>>> +    uint64_t x16;
>>>>> +    uint64_t lr;
>>>>> +    uint64_t fp;
>>>>> +    uint64_t pc;
>>>>> +    uint64_t sp_el0;
>>>>> +    uint64_t sp_el1;
>>>>> +    uint32_t spsr_el1;
>>>>> +    uint32_t _pad;
>>>>> +};
>>>>
>>>>
>>>> My ARM knowledge is certainly quite limited, but the incomplete set
>>>> of GPRs here is quite obvious: Is there a reason to not make all of
>>>> them available here? And if there is, could the criteria of which
>>>> registers got put here please be documented in a comment (so that
>>>> the next person noticing the incomplete set won't ask again)?
>>>
>>>
>>> There already is a comment in place that explains why we are not
>>> sending the full set of registers here.
>>
>>
>> Your comment only says:
>> "Using custom vCPU structs (i.e. not hvm_hw_cpu) for both x86 and ARM
>> so as to not fill the vm_event ring buffer too quickly." it does not explain
>> the criteria of which registers got put here.
>
> Well, as we discussed it in the previous revision, there is no
> hard-set rule of what can and cannot be transmitted here. The only
> thing to keep in mind is to not grow this struct to be too large. The
> registers sent right now represent a "best guess" of what may be
> useful for performance-sensitive vm_event applications on ARM. It can
> be adjusted in the future if applications require other registers.
> Right now there are no applications at all in this space so we don't
> have any specifications to rely on for making this selection. I'm
> happy to make adjustments to the selection if others have a better
> idea what to send, the only registers I certainly find very useful are
> TTBR0/1, cpsr and pc at this time.

Please log it in the commit message and the code. If someone emitted 
multiple time the same concern on previous version, it likely means that 
your commit message was not clear enough and should be updated.

The number of patch to review on Xen-devel is very consequence, so we 
cannot really afford to spend a lot of time digging into previous 
threads. As a maintainer of a subsystem, you should be aware of that.

We are trying, at least on ARM, to get as much details as possible in 
the commit message and document any possible unclear code to help 
developer understanding why it has been done like that. It also helps us 
(the reviewers and maintainers) to give useful advice later on.

Regards,

-- 
Julien Grall

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

Re: [PATCH v4 5/8] arm/vm_event: get/set registers

[Tamas K Lengyel]

On Mon, May 30, 2016 at 3:35 PM, Julien Grall <julien.grall@arm.com> wrote:
>
>
> On 30/05/2016 21:37, Tamas K Lengyel wrote:
>>
>> On Mon, May 30, 2016 at 2:20 PM, Julien Grall <julien.grall@arm.com>
>> wrote:
>>>
>>> Hi Tamas,
>>>
>>> On 30/05/2016 20:47, Tamas K Lengyel wrote:
>>>>
>>>>
>>>> On Mon, May 30, 2016 at 5:50 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>>
>>>>>>
>>>>>> +struct vm_event_regs_arm64 {
>>>>>> +    uint64_t x0;
>>>>>> +    uint64_t x1;
>>>>>> +    uint64_t x2;
>>>>>> +    uint64_t x3;
>>>>>> +    uint64_t x4;
>>>>>> +    uint64_t x5;
>>>>>> +    uint64_t x6;
>>>>>> +    uint64_t x7;
>>>>>> +    uint64_t x8;
>>>>>> +    uint64_t x9;
>>>>>> +    uint64_t x10;
>>>>>> +    uint64_t x16;
>>>>>> +    uint64_t lr;
>>>>>> +    uint64_t fp;
>>>>>> +    uint64_t pc;
>>>>>> +    uint64_t sp_el0;
>>>>>> +    uint64_t sp_el1;
>>>>>> +    uint32_t spsr_el1;
>>>>>> +    uint32_t _pad;
>>>>>> +};
>>>>>
>>>>>
>>>>>
>>>>> My ARM knowledge is certainly quite limited, but the incomplete set
>>>>> of GPRs here is quite obvious: Is there a reason to not make all of
>>>>> them available here? And if there is, could the criteria of which
>>>>> registers got put here please be documented in a comment (so that
>>>>> the next person noticing the incomplete set won't ask again)?
>>>>
>>>>
>>>>
>>>> There already is a comment in place that explains why we are not
>>>> sending the full set of registers here.
>>>
>>>
>>>
>>> Your comment only says:
>>> "Using custom vCPU structs (i.e. not hvm_hw_cpu) for both x86 and ARM
>>> so as to not fill the vm_event ring buffer too quickly." it does not
>>> explain
>>> the criteria of which registers got put here.
>>
>>
>> Well, as we discussed it in the previous revision, there is no
>> hard-set rule of what can and cannot be transmitted here. The only
>> thing to keep in mind is to not grow this struct to be too large. The
>> registers sent right now represent a "best guess" of what may be
>> useful for performance-sensitive vm_event applications on ARM. It can
>> be adjusted in the future if applications require other registers.
>> Right now there are no applications at all in this space so we don't
>> have any specifications to rely on for making this selection. I'm
>> happy to make adjustments to the selection if others have a better
>> idea what to send, the only registers I certainly find very useful are
>> TTBR0/1, cpsr and pc at this time.
>
>
> Please log it in the commit message and the code. If someone emitted
> multiple time the same concern on previous version, it likely means that
> your commit message was not clear enough and should be updated.
>
> The number of patch to review on Xen-devel is very consequence, so we cannot
> really afford to spend a lot of time digging into previous threads. As a
> maintainer of a subsystem, you should be aware of that.
>
> We are trying, at least on ARM, to get as much details as possible in the
> commit message and document any possible unclear code to help developer
> understanding why it has been done like that. It also helps us (the
> reviewers and maintainers) to give useful advice later on.
>

Of course, thanks.

Tamas

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

Re: [PATCH v4 5/8] arm/vm_event: get/set registers

[Jan Beulich]

>>> On 30.05.16 at 22:37, <tamas@tklengyel.com> wrote:
> On Mon, May 30, 2016 at 2:20 PM, Julien Grall <julien.grall@arm.com> wrote:
>> On 30/05/2016 20:47, Tamas K Lengyel wrote:
>>> On Mon, May 30, 2016 at 5:50 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>> +struct vm_event_regs_arm64 {
>>>>> +    uint64_t x0;
>>>>> +    uint64_t x1;
>>>>> +    uint64_t x2;
>>>>> +    uint64_t x3;
>>>>> +    uint64_t x4;
>>>>> +    uint64_t x5;
>>>>> +    uint64_t x6;
>>>>> +    uint64_t x7;
>>>>> +    uint64_t x8;
>>>>> +    uint64_t x9;
>>>>> +    uint64_t x10;
>>>>> +    uint64_t x16;
>>>>> +    uint64_t lr;
>>>>> +    uint64_t fp;
>>>>> +    uint64_t pc;
>>>>> +    uint64_t sp_el0;
>>>>> +    uint64_t sp_el1;
>>>>> +    uint32_t spsr_el1;
>>>>> +    uint32_t _pad;
>>>>> +};
>>>>
>>>>
>>>> My ARM knowledge is certainly quite limited, but the incomplete set
>>>> of GPRs here is quite obvious: Is there a reason to not make all of
>>>> them available here? And if there is, could the criteria of which
>>>> registers got put here please be documented in a comment (so that
>>>> the next person noticing the incomplete set won't ask again)?
>>>
>>>
>>> There already is a comment in place that explains why we are not
>>> sending the full set of registers here.
>>
>>
>> Your comment only says:
>> "Using custom vCPU structs (i.e. not hvm_hw_cpu) for both x86 and ARM
>> so as to not fill the vm_event ring buffer too quickly." it does not explain
>> the criteria of which registers got put here.
> 
> Well, as we discussed it in the previous revision, there is no
> hard-set rule of what can and cannot be transmitted here. The only
> thing to keep in mind is to not grow this struct to be too large. The
> registers sent right now represent a "best guess" of what may be
> useful for performance-sensitive vm_event applications on ARM. It can
> be adjusted in the future if applications require other registers.
> Right now there are no applications at all in this space so we don't
> have any specifications to rely on for making this selection. I'm
> happy to make adjustments to the selection if others have a better
> idea what to send, the only registers I certainly find very useful are
> TTBR0/1, cpsr and pc at this time.

Not being an ARM maintainer I'd say "Then include just those and no
(other) GPRs at all, or include all GPRs." Such a "best guess" approach
can really only end up being wrong from some future consumer. And
in that consideration, please also include the aspects that lead to all
x86 GPRs to get included here (not to speak of even various MSR
values). IOW the same criteria should be applied to all architectures.

Jan


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

Re: [PATCH v4 5/8] arm/vm_event: get/set registers

[Razvan Cojocaru]

On 05/31/2016 10:54 AM, Jan Beulich wrote:
>>>> On 30.05.16 at 22:37, <tamas@tklengyel.com> wrote:
>> On Mon, May 30, 2016 at 2:20 PM, Julien Grall <julien.grall@arm.com> wrote:
>>> On 30/05/2016 20:47, Tamas K Lengyel wrote:
>>>> On Mon, May 30, 2016 at 5:50 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>> +struct vm_event_regs_arm64 {
>>>>>> +    uint64_t x0;
>>>>>> +    uint64_t x1;
>>>>>> +    uint64_t x2;
>>>>>> +    uint64_t x3;
>>>>>> +    uint64_t x4;
>>>>>> +    uint64_t x5;
>>>>>> +    uint64_t x6;
>>>>>> +    uint64_t x7;
>>>>>> +    uint64_t x8;
>>>>>> +    uint64_t x9;
>>>>>> +    uint64_t x10;
>>>>>> +    uint64_t x16;
>>>>>> +    uint64_t lr;
>>>>>> +    uint64_t fp;
>>>>>> +    uint64_t pc;
>>>>>> +    uint64_t sp_el0;
>>>>>> +    uint64_t sp_el1;
>>>>>> +    uint32_t spsr_el1;
>>>>>> +    uint32_t _pad;
>>>>>> +};
>>>>>
>>>>>
>>>>> My ARM knowledge is certainly quite limited, but the incomplete set
>>>>> of GPRs here is quite obvious: Is there a reason to not make all of
>>>>> them available here? And if there is, could the criteria of which
>>>>> registers got put here please be documented in a comment (so that
>>>>> the next person noticing the incomplete set won't ask again)?
>>>>
>>>>
>>>> There already is a comment in place that explains why we are not
>>>> sending the full set of registers here.
>>>
>>>
>>> Your comment only says:
>>> "Using custom vCPU structs (i.e. not hvm_hw_cpu) for both x86 and ARM
>>> so as to not fill the vm_event ring buffer too quickly." it does not explain
>>> the criteria of which registers got put here.
>>
>> Well, as we discussed it in the previous revision, there is no
>> hard-set rule of what can and cannot be transmitted here. The only
>> thing to keep in mind is to not grow this struct to be too large. The
>> registers sent right now represent a "best guess" of what may be
>> useful for performance-sensitive vm_event applications on ARM. It can
>> be adjusted in the future if applications require other registers.
>> Right now there are no applications at all in this space so we don't
>> have any specifications to rely on for making this selection. I'm
>> happy to make adjustments to the selection if others have a better
>> idea what to send, the only registers I certainly find very useful are
>> TTBR0/1, cpsr and pc at this time.
> 
> Not being an ARM maintainer I'd say "Then include just those and no
> (other) GPRs at all, or include all GPRs." Such a "best guess" approach
> can really only end up being wrong from some future consumer. And
> in that consideration, please also include the aspects that lead to all
> x86 GPRs to get included here (not to speak of even various MSR
> values). IOW the same criteria should be applied to all architectures.

The x86 GPRS are already all included in the vm_event request:

133 struct vm_event_regs_x86 {
134     uint64_t rax;
135     uint64_t rcx;
136     uint64_t rdx;
137     uint64_t rbx;
138     uint64_t rsp;
139     uint64_t rbp;
140     uint64_t rsi;
141     uint64_t rdi;


Thanks,
Razvan

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

Re: [PATCH v4 5/8] arm/vm_event: get/set registers

[Jan Beulich]

>>> On 31.05.16 at 10:06, <rcojocaru@bitdefender.com> wrote:
> On 05/31/2016 10:54 AM, Jan Beulich wrote:
>>>>> On 30.05.16 at 22:37, <tamas@tklengyel.com> wrote:
>>> On Mon, May 30, 2016 at 2:20 PM, Julien Grall <julien.grall@arm.com> wrote:
>>>> On 30/05/2016 20:47, Tamas K Lengyel wrote:
>>>>> On Mon, May 30, 2016 at 5:50 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>>> +struct vm_event_regs_arm64 {
>>>>>>> +    uint64_t x0;
>>>>>>> +    uint64_t x1;
>>>>>>> +    uint64_t x2;
>>>>>>> +    uint64_t x3;
>>>>>>> +    uint64_t x4;
>>>>>>> +    uint64_t x5;
>>>>>>> +    uint64_t x6;
>>>>>>> +    uint64_t x7;
>>>>>>> +    uint64_t x8;
>>>>>>> +    uint64_t x9;
>>>>>>> +    uint64_t x10;
>>>>>>> +    uint64_t x16;
>>>>>>> +    uint64_t lr;
>>>>>>> +    uint64_t fp;
>>>>>>> +    uint64_t pc;
>>>>>>> +    uint64_t sp_el0;
>>>>>>> +    uint64_t sp_el1;
>>>>>>> +    uint32_t spsr_el1;
>>>>>>> +    uint32_t _pad;
>>>>>>> +};
>>>>>>
>>>>>>
>>>>>> My ARM knowledge is certainly quite limited, but the incomplete set
>>>>>> of GPRs here is quite obvious: Is there a reason to not make all of
>>>>>> them available here? And if there is, could the criteria of which
>>>>>> registers got put here please be documented in a comment (so that
>>>>>> the next person noticing the incomplete set won't ask again)?
>>>>>
>>>>>
>>>>> There already is a comment in place that explains why we are not
>>>>> sending the full set of registers here.
>>>>
>>>>
>>>> Your comment only says:
>>>> "Using custom vCPU structs (i.e. not hvm_hw_cpu) for both x86 and ARM
>>>> so as to not fill the vm_event ring buffer too quickly." it does not explain
>>>> the criteria of which registers got put here.
>>>
>>> Well, as we discussed it in the previous revision, there is no
>>> hard-set rule of what can and cannot be transmitted here. The only
>>> thing to keep in mind is to not grow this struct to be too large. The
>>> registers sent right now represent a "best guess" of what may be
>>> useful for performance-sensitive vm_event applications on ARM. It can
>>> be adjusted in the future if applications require other registers.
>>> Right now there are no applications at all in this space so we don't
>>> have any specifications to rely on for making this selection. I'm
>>> happy to make adjustments to the selection if others have a better
>>> idea what to send, the only registers I certainly find very useful are
>>> TTBR0/1, cpsr and pc at this time.
>> 
>> Not being an ARM maintainer I'd say "Then include just those and no
>> (other) GPRs at all, or include all GPRs." Such a "best guess" approach
>> can really only end up being wrong from some future consumer. And
>> in that consideration, please also include the aspects that lead to all
>> x86 GPRs to get included here (not to speak of even various MSR
>> values). IOW the same criteria should be applied to all architectures.
> 
> The x86 GPRS are already all included in the vm_event request:

Well, that's what I've been saying, or at least I had tried to: I
realize I've typoed the past tense of "lead" - should have been
"led". Sorry for the confusion.

Jan


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

Re: [PATCH v4 5/8] arm/vm_event: get/set registers

[Tamas K Lengyel]

[-- Attachment #1.1: Type: text/plain, Size: 3425 bytes --]

On May 31, 2016 01:54, "Jan Beulich" <JBeulich@suse.com> wrote:
>
> >>> On 30.05.16 at 22:37, <tamas@tklengyel.com> wrote:
> > On Mon, May 30, 2016 at 2:20 PM, Julien Grall <julien.grall@arm.com>
wrote:
> >> On 30/05/2016 20:47, Tamas K Lengyel wrote:
> >>> On Mon, May 30, 2016 at 5:50 AM, Jan Beulich <JBeulich@suse.com>
wrote:
> >>>>> +struct vm_event_regs_arm64 {
> >>>>> +    uint64_t x0;
> >>>>> +    uint64_t x1;
> >>>>> +    uint64_t x2;
> >>>>> +    uint64_t x3;
> >>>>> +    uint64_t x4;
> >>>>> +    uint64_t x5;
> >>>>> +    uint64_t x6;
> >>>>> +    uint64_t x7;
> >>>>> +    uint64_t x8;
> >>>>> +    uint64_t x9;
> >>>>> +    uint64_t x10;
> >>>>> +    uint64_t x16;
> >>>>> +    uint64_t lr;
> >>>>> +    uint64_t fp;
> >>>>> +    uint64_t pc;
> >>>>> +    uint64_t sp_el0;
> >>>>> +    uint64_t sp_el1;
> >>>>> +    uint32_t spsr_el1;
> >>>>> +    uint32_t _pad;
> >>>>> +};
> >>>>
> >>>>
> >>>> My ARM knowledge is certainly quite limited, but the incomplete set
> >>>> of GPRs here is quite obvious: Is there a reason to not make all of
> >>>> them available here? And if there is, could the criteria of which
> >>>> registers got put here please be documented in a comment (so that
> >>>> the next person noticing the incomplete set won't ask again)?
> >>>
> >>>
> >>> There already is a comment in place that explains why we are not
> >>> sending the full set of registers here.
> >>
> >>
> >> Your comment only says:
> >> "Using custom vCPU structs (i.e. not hvm_hw_cpu) for both x86 and ARM
> >> so as to not fill the vm_event ring buffer too quickly." it does not
explain
> >> the criteria of which registers got put here.
> >
> > Well, as we discussed it in the previous revision, there is no
> > hard-set rule of what can and cannot be transmitted here. The only
> > thing to keep in mind is to not grow this struct to be too large. The
> > registers sent right now represent a "best guess" of what may be
> > useful for performance-sensitive vm_event applications on ARM. It can
> > be adjusted in the future if applications require other registers.
> > Right now there are no applications at all in this space so we don't
> > have any specifications to rely on for making this selection. I'm
> > happy to make adjustments to the selection if others have a better
> > idea what to send, the only registers I certainly find very useful are
> > TTBR0/1, cpsr and pc at this time.
>
> Not being an ARM maintainer I'd say "Then include just those and no
> (other) GPRs at all, or include all GPRs." Such a "best guess" approach
> can really only end up being wrong from some future consumer. And
> in that consideration, please also include the aspects that lead to all
> x86 GPRs to get included here (not to speak of even various MSR
> values). IOW the same criteria should be applied to all architectures.
>

I don't think there is such a thing as being wrong here. The user always
has access to the full set of registers plus there is ample room here to
add other registers in the future while we are smaller then the x86 struct.
For an initial set I think it's perfectly fine to do a subset and add more
as we go forward and learn about the usecases. So since there is no
technical reason that doing a subset would be incorrect I don't see an
issue here. As I said, I'm happy to explain it in the commit message and a
comment in the code that the register set is expandable and adjustable.

Tamas

[-- Attachment #1.2: Type: text/html, Size: 4832 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH v4 5/8] arm/vm_event: get/set registers

[Jan Beulich]

>>> On 30.05.16 at 21:47, <tamas@tklengyel.com> wrote:
> On Mon, May 30, 2016 at 5:50 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>> On 30.05.16 at 00:37, <tamas@tklengyel.com> wrote:
>>> +struct vm_event_regs_arm32 {
>>> +    uint32_t r0_usr;
>>> +    uint32_t r1_usr;
>>> +    uint32_t r2_usr;
>>> +    uint32_t r3_usr;
>>> +    uint32_t r4_usr;
>>> +    uint32_t r5_usr;
>>> +    uint32_t r6_usr;
>>> +    uint32_t r7_usr;
>>> +    uint32_t r8_usr;
>>> +    uint32_t r9_usr;
>>> +    uint32_t r10_usr;
>>> +    uint32_t r12_usr;
>>> +    uint32_t lr_usr;
>>> +    uint32_t fp;
>>> +    uint32_t pc;
>>> +    uint32_t sp_usr;
>>> +    uint32_t sp_svc;
>>> +    uint32_t spsr_svc;
>>> +};
>>
>> It would seem more natural for the "ordinary" registers to be
>> arranged in the numerical sequence, i.e. fp, r12, sp, lr, pc.
> 
> Not sure I follow.

For one it is quite natural for someone looking at a sequence of
register values to assume / expect them to be in natural order.
And then, having them in natural (numeric) order allows e.g.
extracting the register identifying bits from an instruction to use
them as an array index into (part of) this structure.

(For some background: I've been bitten a number of times by
people sorting x86 registers alphabetically instead or naturally,
i.e. EAX, EBX, ECX, EDX instead of EAX, ECX, EDX, EBX).

>>> +struct vm_event_regs_arm64 {
>>> +    uint64_t x0;
>>> +    uint64_t x1;
>>> +    uint64_t x2;
>>> +    uint64_t x3;
>>> +    uint64_t x4;
>>> +    uint64_t x5;
>>> +    uint64_t x6;
>>> +    uint64_t x7;
>>> +    uint64_t x8;
>>> +    uint64_t x9;
>>> +    uint64_t x10;
>>> +    uint64_t x16;
>>> +    uint64_t lr;
>>> +    uint64_t fp;
>>> +    uint64_t pc;
>>> +    uint64_t sp_el0;
>>> +    uint64_t sp_el1;
>>> +    uint32_t spsr_el1;
>>> +    uint32_t _pad;
>>> +};
>>
>> My ARM knowledge is certainly quite limited, but the incomplete set
>> of GPRs here is quite obvious: Is there a reason to not make all of
>> them available here? And if there is, could the criteria of which
>> registers got put here please be documented in a comment (so that
>> the next person noticing the incomplete set won't ask again)?
> 
> There already is a comment in place that explains why we are not
> sending the full set of registers here.

I've just gone through the entire patch again, and I didn't find any.
Are you perhaps referring to "Using custom vCPU structs (i.e. not
hvm_hw_cpu) for both x86 and ARM so as to not fill the vm_event
ring buffer too quickly"? If so, that's irrelevant here: It explains why
e.g. floating point registers don't get sent, but it doesn't explain in
any way why some GPRs are more important than others.

>> I'm also puzzled by fp and lr - I'm not aware of registers of those
>> names (and gas also doesn't accept them afaict).
> 
> Not sure why but Xen names x29 fp and x30 lr. See
> include/asm-arm/arm64/processor.h.

See Julien's reply.

Jan


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

Re: [PATCH v4 5/8] arm/vm_event: get/set registers

[Tamas K Lengyel]

[-- Attachment #1.1: Type: text/plain, Size: 3598 bytes --]

On May 31, 2016 01:48, "Jan Beulich" <JBeulich@suse.com> wrote:
>
> >>> On 30.05.16 at 21:47, <tamas@tklengyel.com> wrote:
> > On Mon, May 30, 2016 at 5:50 AM, Jan Beulich <JBeulich@suse.com> wrote:
> >>>>> On 30.05.16 at 00:37, <tamas@tklengyel.com> wrote:
> >>> +struct vm_event_regs_arm32 {
> >>> +    uint32_t r0_usr;
> >>> +    uint32_t r1_usr;
> >>> +    uint32_t r2_usr;
> >>> +    uint32_t r3_usr;
> >>> +    uint32_t r4_usr;
> >>> +    uint32_t r5_usr;
> >>> +    uint32_t r6_usr;
> >>> +    uint32_t r7_usr;
> >>> +    uint32_t r8_usr;
> >>> +    uint32_t r9_usr;
> >>> +    uint32_t r10_usr;
> >>> +    uint32_t r12_usr;
> >>> +    uint32_t lr_usr;
> >>> +    uint32_t fp;
> >>> +    uint32_t pc;
> >>> +    uint32_t sp_usr;
> >>> +    uint32_t sp_svc;
> >>> +    uint32_t spsr_svc;
> >>> +};
> >>
> >> It would seem more natural for the "ordinary" registers to be
> >> arranged in the numerical sequence, i.e. fp, r12, sp, lr, pc.
> >
> > Not sure I follow.
>
> For one it is quite natural for someone looking at a sequence of
> register values to assume / expect them to be in natural order.
> And then, having them in natural (numeric) order allows e.g.
> extracting the register identifying bits from an instruction to use
> them as an array index into (part of) this structure.
>
> (For some background: I've been bitten a number of times by
> people sorting x86 registers alphabetically instead or naturally,
> i.e. EAX, EBX, ECX, EDX instead of EAX, ECX, EDX, EBX).

I see, however I believe that would be a very careless use of this struct
from the user as the register sizes are not even necessarily match the
architecture. For example we only define the 64bit x86 registers, so trying
to access it as an array of 32bit registers wouldn't work at all. Plus we
are not doing a full set of registers, and I rather not imply that every
element in the "natural sequence" is present. It may be, it may be not.

>
> >>> +struct vm_event_regs_arm64 {
> >>> +    uint64_t x0;
> >>> +    uint64_t x1;
> >>> +    uint64_t x2;
> >>> +    uint64_t x3;
> >>> +    uint64_t x4;
> >>> +    uint64_t x5;
> >>> +    uint64_t x6;
> >>> +    uint64_t x7;
> >>> +    uint64_t x8;
> >>> +    uint64_t x9;
> >>> +    uint64_t x10;
> >>> +    uint64_t x16;
> >>> +    uint64_t lr;
> >>> +    uint64_t fp;
> >>> +    uint64_t pc;
> >>> +    uint64_t sp_el0;
> >>> +    uint64_t sp_el1;
> >>> +    uint32_t spsr_el1;
> >>> +    uint32_t _pad;
> >>> +};
> >>
> >> My ARM knowledge is certainly quite limited, but the incomplete set
> >> of GPRs here is quite obvious: Is there a reason to not make all of
> >> them available here? And if there is, could the criteria of which
> >> registers got put here please be documented in a comment (so that
> >> the next person noticing the incomplete set won't ask again)?
> >
> > There already is a comment in place that explains why we are not
> > sending the full set of registers here.
>
> I've just gone through the entire patch again, and I didn't find any.
> Are you perhaps referring to "Using custom vCPU structs (i.e. not
> hvm_hw_cpu) for both x86 and ARM so as to not fill the vm_event
> ring buffer too quickly"? If so, that's irrelevant here: It explains why
> e.g. floating point registers don't get sent, but it doesn't explain in
> any way why some GPRs are more important than others.
>
> >> I'm also puzzled by fp and lr - I'm not aware of registers of those
> >> names (and gas also doesn't accept them afaict).
> >
> > Not sure why but Xen names x29 fp and x30 lr. See
> > include/asm-arm/arm64/processor.h.
>
> See Julien's reply.
>
> Jan
>

[-- Attachment #1.2: Type: text/html, Size: 5193 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH v4 5/8] arm/vm_event: get/set registers

[Jan Beulich]

>>> On 31.05.16 at 18:28, <tamas@tklengyel.com> wrote:
> On May 31, 2016 01:48, "Jan Beulich" <JBeulich@suse.com> wrote:
>>
>> >>> On 30.05.16 at 21:47, <tamas@tklengyel.com> wrote:
>> > On Mon, May 30, 2016 at 5:50 AM, Jan Beulich <JBeulich@suse.com> wrote:
>> >>>>> On 30.05.16 at 00:37, <tamas@tklengyel.com> wrote:
>> >>> +struct vm_event_regs_arm32 {
>> >>> +    uint32_t r0_usr;
>> >>> +    uint32_t r1_usr;
>> >>> +    uint32_t r2_usr;
>> >>> +    uint32_t r3_usr;
>> >>> +    uint32_t r4_usr;
>> >>> +    uint32_t r5_usr;
>> >>> +    uint32_t r6_usr;
>> >>> +    uint32_t r7_usr;
>> >>> +    uint32_t r8_usr;
>> >>> +    uint32_t r9_usr;
>> >>> +    uint32_t r10_usr;
>> >>> +    uint32_t r12_usr;
>> >>> +    uint32_t lr_usr;
>> >>> +    uint32_t fp;
>> >>> +    uint32_t pc;
>> >>> +    uint32_t sp_usr;
>> >>> +    uint32_t sp_svc;
>> >>> +    uint32_t spsr_svc;
>> >>> +};
>> >>
>> >> It would seem more natural for the "ordinary" registers to be
>> >> arranged in the numerical sequence, i.e. fp, r12, sp, lr, pc.
>> >
>> > Not sure I follow.
>>
>> For one it is quite natural for someone looking at a sequence of
>> register values to assume / expect them to be in natural order.
>> And then, having them in natural (numeric) order allows e.g.
>> extracting the register identifying bits from an instruction to use
>> them as an array index into (part of) this structure.
>>
>> (For some background: I've been bitten a number of times by
>> people sorting x86 registers alphabetically instead or naturally,
>> i.e. EAX, EBX, ECX, EDX instead of EAX, ECX, EDX, EBX).
> 
> I see, however I believe that would be a very careless use of this struct
> from the user as the register sizes are not even necessarily match the
> architecture. For example we only define the 64bit x86 registers, so trying
> to access it as an array of 32bit registers wouldn't work at all. Plus we
> are not doing a full set of registers, and I rather not imply that every
> element in the "natural sequence" is present. It may be, it may be not.

Once an ABI is set in stone, and if that ABI allows for optimizations
(by consumers) like the one mentioned, I don't think this would be
careless use. The resulting code is very clearly much more efficient
than e.g. a switch() statement with a case label for each and every
register. Well, yes, I already hear the "memory is cheap and hence
code size doesn't matter" argument, but as said elsewhere quite
recently I don't buy this.

Jan


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

Re: [PATCH v4 5/8] arm/vm_event: get/set registers

[Julien Grall]

Hi,

On 01/06/16 09:41, Jan Beulich wrote:
>>>> On 31.05.16 at 18:28, <tamas@tklengyel.com> wrote:
>> On May 31, 2016 01:48, "Jan Beulich" <JBeulich@suse.com> wrote:
>>>
>>>>>> On 30.05.16 at 21:47, <tamas@tklengyel.com> wrote:
>>>> On Mon, May 30, 2016 at 5:50 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>>>> On 30.05.16 at 00:37, <tamas@tklengyel.com> wrote:
>>>>>> +struct vm_event_regs_arm32 {
>>>>>> +    uint32_t r0_usr;
>>>>>> +    uint32_t r1_usr;
>>>>>> +    uint32_t r2_usr;
>>>>>> +    uint32_t r3_usr;
>>>>>> +    uint32_t r4_usr;
>>>>>> +    uint32_t r5_usr;
>>>>>> +    uint32_t r6_usr;
>>>>>> +    uint32_t r7_usr;
>>>>>> +    uint32_t r8_usr;
>>>>>> +    uint32_t r9_usr;
>>>>>> +    uint32_t r10_usr;
>>>>>> +    uint32_t r12_usr;
>>>>>> +    uint32_t lr_usr;
>>>>>> +    uint32_t fp;
>>>>>> +    uint32_t pc;
>>>>>> +    uint32_t sp_usr;
>>>>>> +    uint32_t sp_svc;
>>>>>> +    uint32_t spsr_svc;
>>>>>> +};
>>>>>
>>>>> It would seem more natural for the "ordinary" registers to be
>>>>> arranged in the numerical sequence, i.e. fp, r12, sp, lr, pc.
>>>>
>>>> Not sure I follow.
>>>
>>> For one it is quite natural for someone looking at a sequence of
>>> register values to assume / expect them to be in natural order.
>>> And then, having them in natural (numeric) order allows e.g.
>>> extracting the register identifying bits from an instruction to use
>>> them as an array index into (part of) this structure.
>>>
>>> (For some background: I've been bitten a number of times by
>>> people sorting x86 registers alphabetically instead or naturally,
>>> i.e. EAX, EBX, ECX, EDX instead of EAX, ECX, EDX, EBX).
>>
>> I see, however I believe that would be a very careless use of this struct
>> from the user as the register sizes are not even necessarily match the
>> architecture. For example we only define the 64bit x86 registers, so trying
>> to access it as an array of 32bit registers wouldn't work at all. Plus we
>> are not doing a full set of registers, and I rather not imply that every
>> element in the "natural sequence" is present. It may be, it may be not.
>
> Once an ABI is set in stone, and if that ABI allows for optimizations
> (by consumers) like the one mentioned, I don't think this would be
> careless use. The resulting code is very clearly much more efficient
> than e.g. a switch() statement with a case label for each and every
> register. Well, yes, I already hear the "memory is cheap and hence
> code size doesn't matter" argument, but as said elsewhere quite
> recently I don't buy this.

I agree with Jan here.

ARM64 has 31 general purposes registers (x0-x30). The switch to find a 
register based on the index will be quite big.

If you order the register and provide all the general purposes registers 
(x0 - x30), you will be able to replace by a single line (for instance 
see select_user_reg in arch/arm/traps.c).

This will also likely speed up your introspection application.

Regards,

-- 
Julien Grall

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

Re: [PATCH v4 5/8] arm/vm_event: get/set registers

[Tamas K Lengyel]

On Wed, Jun 1, 2016 at 5:24 AM, Julien Grall <julien.grall@arm.com> wrote:
> Hi,
>
>
> On 01/06/16 09:41, Jan Beulich wrote:
>>>>>
>>>>> On 31.05.16 at 18:28, <tamas@tklengyel.com> wrote:
>>>
>>> On May 31, 2016 01:48, "Jan Beulich" <JBeulich@suse.com> wrote:
>>>>
>>>>
>>>>>>> On 30.05.16 at 21:47, <tamas@tklengyel.com> wrote:
>>>>>
>>>>> On Mon, May 30, 2016 at 5:50 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>>>>>
>>>>>>>>> On 30.05.16 at 00:37, <tamas@tklengyel.com> wrote:
>>>>>>>
>>>>>>> +struct vm_event_regs_arm32 {
>>>>>>> +    uint32_t r0_usr;
>>>>>>> +    uint32_t r1_usr;
>>>>>>> +    uint32_t r2_usr;
>>>>>>> +    uint32_t r3_usr;
>>>>>>> +    uint32_t r4_usr;
>>>>>>> +    uint32_t r5_usr;
>>>>>>> +    uint32_t r6_usr;
>>>>>>> +    uint32_t r7_usr;
>>>>>>> +    uint32_t r8_usr;
>>>>>>> +    uint32_t r9_usr;
>>>>>>> +    uint32_t r10_usr;
>>>>>>> +    uint32_t r12_usr;
>>>>>>> +    uint32_t lr_usr;
>>>>>>> +    uint32_t fp;
>>>>>>> +    uint32_t pc;
>>>>>>> +    uint32_t sp_usr;
>>>>>>> +    uint32_t sp_svc;
>>>>>>> +    uint32_t spsr_svc;
>>>>>>> +};
>>>>>>
>>>>>>
>>>>>> It would seem more natural for the "ordinary" registers to be
>>>>>> arranged in the numerical sequence, i.e. fp, r12, sp, lr, pc.
>>>>>
>>>>>
>>>>> Not sure I follow.
>>>>
>>>>
>>>> For one it is quite natural for someone looking at a sequence of
>>>> register values to assume / expect them to be in natural order.
>>>> And then, having them in natural (numeric) order allows e.g.
>>>> extracting the register identifying bits from an instruction to use
>>>> them as an array index into (part of) this structure.
>>>>
>>>> (For some background: I've been bitten a number of times by
>>>> people sorting x86 registers alphabetically instead or naturally,
>>>> i.e. EAX, EBX, ECX, EDX instead of EAX, ECX, EDX, EBX).
>>>
>>>
>>> I see, however I believe that would be a very careless use of this struct
>>> from the user as the register sizes are not even necessarily match the
>>> architecture. For example we only define the 64bit x86 registers, so
>>> trying
>>> to access it as an array of 32bit registers wouldn't work at all. Plus we
>>> are not doing a full set of registers, and I rather not imply that every
>>> element in the "natural sequence" is present. It may be, it may be not.
>>
>>
>> Once an ABI is set in stone, and if that ABI allows for optimizations
>> (by consumers) like the one mentioned, I don't think this would be
>> careless use. The resulting code is very clearly much more efficient
>> than e.g. a switch() statement with a case label for each and every
>> register. Well, yes, I already hear the "memory is cheap and hence
>> code size doesn't matter" argument, but as said elsewhere quite
>> recently I don't buy this.
>
>
> I agree with Jan here.
>
> ARM64 has 31 general purposes registers (x0-x30). The switch to find a
> register based on the index will be quite big.
>
> If you order the register and provide all the general purposes registers (x0
> - x30), you will be able to replace by a single line (for instance see
> select_user_reg in arch/arm/traps.c).

The issue is that the x86 vm_event struct right now has 32*uint64_t
size. So if we would want to transmit all ARM64 GPRs + cpsr, PC and
TTBR0/1 we would end up growing this structure beyond what it is
currently. I want to avoid that as it affects both ARM32 and x86
introspection applications as well as now we can hold fewer events on
the ring. I would say it is better to avoid that then potentially
saving some on a switch in ARM64 introspection applications.

>
> This will also likely speed up your introspection application.

Win some, lose some. I would prefer if these changes had no
cross-architectural effect unless absolutely required. Razvan, what do
you think?

Tamas

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

Re: [PATCH v4 5/8] arm/vm_event: get/set registers

[Razvan Cojocaru]

On 06/01/16 21:21, Tamas K Lengyel wrote:
> On Wed, Jun 1, 2016 at 5:24 AM, Julien Grall <julien.grall@arm.com> wrote:
>> Hi,
>>
>>
>> On 01/06/16 09:41, Jan Beulich wrote:
>>>>>>
>>>>>> On 31.05.16 at 18:28, <tamas@tklengyel.com> wrote:
>>>>
>>>> On May 31, 2016 01:48, "Jan Beulich" <JBeulich@suse.com> wrote:
>>>>>
>>>>>
>>>>>>>> On 30.05.16 at 21:47, <tamas@tklengyel.com> wrote:
>>>>>>
>>>>>> On Mon, May 30, 2016 at 5:50 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>>>>>>
>>>>>>>>>> On 30.05.16 at 00:37, <tamas@tklengyel.com> wrote:
>>>>>>>>
>>>>>>>> +struct vm_event_regs_arm32 {
>>>>>>>> +    uint32_t r0_usr;
>>>>>>>> +    uint32_t r1_usr;
>>>>>>>> +    uint32_t r2_usr;
>>>>>>>> +    uint32_t r3_usr;
>>>>>>>> +    uint32_t r4_usr;
>>>>>>>> +    uint32_t r5_usr;
>>>>>>>> +    uint32_t r6_usr;
>>>>>>>> +    uint32_t r7_usr;
>>>>>>>> +    uint32_t r8_usr;
>>>>>>>> +    uint32_t r9_usr;
>>>>>>>> +    uint32_t r10_usr;
>>>>>>>> +    uint32_t r12_usr;
>>>>>>>> +    uint32_t lr_usr;
>>>>>>>> +    uint32_t fp;
>>>>>>>> +    uint32_t pc;
>>>>>>>> +    uint32_t sp_usr;
>>>>>>>> +    uint32_t sp_svc;
>>>>>>>> +    uint32_t spsr_svc;
>>>>>>>> +};
>>>>>>>
>>>>>>>
>>>>>>> It would seem more natural for the "ordinary" registers to be
>>>>>>> arranged in the numerical sequence, i.e. fp, r12, sp, lr, pc.
>>>>>>
>>>>>>
>>>>>> Not sure I follow.
>>>>>
>>>>>
>>>>> For one it is quite natural for someone looking at a sequence of
>>>>> register values to assume / expect them to be in natural order.
>>>>> And then, having them in natural (numeric) order allows e.g.
>>>>> extracting the register identifying bits from an instruction to use
>>>>> them as an array index into (part of) this structure.
>>>>>
>>>>> (For some background: I've been bitten a number of times by
>>>>> people sorting x86 registers alphabetically instead or naturally,
>>>>> i.e. EAX, EBX, ECX, EDX instead of EAX, ECX, EDX, EBX).
>>>>
>>>>
>>>> I see, however I believe that would be a very careless use of this struct
>>>> from the user as the register sizes are not even necessarily match the
>>>> architecture. For example we only define the 64bit x86 registers, so
>>>> trying
>>>> to access it as an array of 32bit registers wouldn't work at all. Plus we
>>>> are not doing a full set of registers, and I rather not imply that every
>>>> element in the "natural sequence" is present. It may be, it may be not.
>>>
>>>
>>> Once an ABI is set in stone, and if that ABI allows for optimizations
>>> (by consumers) like the one mentioned, I don't think this would be
>>> careless use. The resulting code is very clearly much more efficient
>>> than e.g. a switch() statement with a case label for each and every
>>> register. Well, yes, I already hear the "memory is cheap and hence
>>> code size doesn't matter" argument, but as said elsewhere quite
>>> recently I don't buy this.
>>
>>
>> I agree with Jan here.
>>
>> ARM64 has 31 general purposes registers (x0-x30). The switch to find a
>> register based on the index will be quite big.
>>
>> If you order the register and provide all the general purposes registers (x0
>> - x30), you will be able to replace by a single line (for instance see
>> select_user_reg in arch/arm/traps.c).
> 
> The issue is that the x86 vm_event struct right now has 32*uint64_t
> size. So if we would want to transmit all ARM64 GPRs + cpsr, PC and
> TTBR0/1 we would end up growing this structure beyond what it is
> currently. I want to avoid that as it affects both ARM32 and x86
> introspection applications as well as now we can hold fewer events on
> the ring. I would say it is better to avoid that then potentially
> saving some on a switch in ARM64 introspection applications.
> 
>>
>> This will also likely speed up your introspection application.
> 
> Win some, lose some. I would prefer if these changes had no
> cross-architectural effect unless absolutely required. Razvan, what do
> you think?

I think that if we keep a single page sized event ring buffer it would
be best to only send what consumers need right now.

The only purpose of having that information in the request is to quickly
get things that are immediately necessary - otherwise the full context
can be obtained with xc_domain_hvm_getcontext_partial().

As long as there's a comment present that says that this is all the
information needed at this time, and the struct can grow if needs
change, it might be best not to add unnecessary data just in case
somebody will need it later.

Having said that, growing the struct by about 16 bytes or so shouldn't
pose huge problems (I'm not sure I've calculated the correct size based
on your previous message). Our application only uses sync-style events,
so the ring buffer can only be full when all VCPUs are blocked, so even
a one page ring buffer should be enough for most sensible applications
right now.

But, the way to go for the future, as I've said before, is IMHO to grow
the ring buffer and possibly add all the data that
xc_domain_hvm_getcontext_partial() now retrieves to the events.

To summarize, FWIW I think we should hold back on including all
available data to requests as long as we keep using a one page ring
buffer, but growing the structure by not much is acceptable.


Thanks,
Razvan

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

Re: [PATCH v4 5/8] arm/vm_event: get/set registers

[Julien Grall]

Hi Razvan,

On 01/06/16 20:34, Razvan Cojocaru wrote:
> On 06/01/16 21:21, Tamas K Lengyel wrote:
>> On Wed, Jun 1, 2016 at 5:24 AM, Julien Grall <julien.grall@arm.com> wrote:
> The only purpose of having that information in the request is to quickly
> get things that are immediately necessary - otherwise the full context
> can be obtained with xc_domain_hvm_getcontext_partial().

xc_domain_hvm_getcontext_partial is not yet implemented on ARM. So 
currently the only solution to get the other registers will be 
XEN_DOMCTL_getvcpucontext.

Regards,

-- 
Julien Grall

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

Re: [PATCH v4 5/8] arm/vm_event: get/set registers

[Jan Beulich]

>>> On 01.06.16 at 21:34, <rcojocaru@bitdefender.com> wrote:
> On 06/01/16 21:21, Tamas K Lengyel wrote:
>> On Wed, Jun 1, 2016 at 5:24 AM, Julien Grall <julien.grall@arm.com> wrote:
>>> On 01/06/16 09:41, Jan Beulich wrote:
>>>> Once an ABI is set in stone, and if that ABI allows for optimizations
>>>> (by consumers) like the one mentioned, I don't think this would be
>>>> careless use. The resulting code is very clearly much more efficient
>>>> than e.g. a switch() statement with a case label for each and every
>>>> register. Well, yes, I already hear the "memory is cheap and hence
>>>> code size doesn't matter" argument, but as said elsewhere quite
>>>> recently I don't buy this.
>>>
>>>
>>> I agree with Jan here.
>>>
>>> ARM64 has 31 general purposes registers (x0-x30). The switch to find a
>>> register based on the index will be quite big.
>>>
>>> If you order the register and provide all the general purposes registers (x0
>>> - x30), you will be able to replace by a single line (for instance see
>>> select_user_reg in arch/arm/traps.c).
>> 
>> The issue is that the x86 vm_event struct right now has 32*uint64_t
>> size. So if we would want to transmit all ARM64 GPRs + cpsr, PC and
>> TTBR0/1 we would end up growing this structure beyond what it is
>> currently. I want to avoid that as it affects both ARM32 and x86
>> introspection applications as well as now we can hold fewer events on
>> the ring. I would say it is better to avoid that then potentially
>> saving some on a switch in ARM64 introspection applications.
>> 
>>>
>>> This will also likely speed up your introspection application.
>> 
>> Win some, lose some. I would prefer if these changes had no
>> cross-architectural effect unless absolutely required. Razvan, what do
>> you think?

Perhaps that cross architectural effect then hints at some design
shortcoming? I.e. things should have been arranged for changes
to one arch'es register set to _never_ affect others.

> I think that if we keep a single page sized event ring buffer it would
> be best to only send what consumers need right now.
> 
> The only purpose of having that information in the request is to quickly
> get things that are immediately necessary - otherwise the full context
> can be obtained with xc_domain_hvm_getcontext_partial().
> 
> As long as there's a comment present that says that this is all the
> information needed at this time, and the struct can grow if needs
> change, it might be best not to add unnecessary data just in case
> somebody will need it later.

Well, that's not quite enough I would say (and this is as a general
remark, as I've peeked ahead and hence did see that Tamas agreed
to include all GPRs): The criteria for inclusion or exclusion should
follow a predictable model. I.e. if someone comes along and says
"I need register Y", then there should be rules that (s)he can apply
up front to determine what (at least in the vast majority of cases)
the response is going to be. You saying "I need only this arbitrary
subset of registers" is completely in-transparent, as it leaves open
why this is so, and why this would also be so for others. I.e. you'd
at least need to answer the question why (as an example) you
need x5 included, but not x25, despite both registers being equal
from an architecture pov. An answer like "My application gets away
with it" is not acceptable here.

Jan


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

Re: [PATCH v4 5/8] arm/vm_event: get/set registers

[Razvan Cojocaru]

On 06/02/2016 10:35 AM, Jan Beulich wrote:
> The criteria for inclusion or exclusion should
> follow a predictable model. I.e. if someone comes along and says
> "I need register Y", then there should be rules that (s)he can apply
> up front to determine what (at least in the vast majority of cases)
> the response is going to be. You saying "I need only this arbitrary
> subset of registers" is completely in-transparent, as it leaves open
> why this is so, and why this would also be so for others. I.e. you'd
> at least need to answer the question why (as an example) you
> need x5 included, but not x25, despite both registers being equal
> from an architecture pov. An answer like "My application gets away
> with it" is not acceptable here.

There will be trade-offs. Again, my preference would be to have a
multi-page ring buffer with all the VCPU context included in the
request, always. This would work best as far as both completeness and
performance goes.

But that is obviously not a trivial change and I can see how Tamas would
prefer to get things done sooner along the lines of the pre-existing
design (whatever its merits and demerits are).

As long as we're using a smaller ring buffer, the answer (at least as
I've meant it) is not "_my_ application gets away with it" - which is
indeed not the most reasonable statement, but "_current_ introspection
applications - of which my application is one of - don't need more than
this at this time". So in the spirit of "the rest of the context is a
hypercall away" + we don't have that much space available in the ring
buffer, the whole context is not included. The assumption here being
that once another application (or even our application, at a later time)
needs to enlarge the set of registers sent with a request, it will be
requested here and we can discuss what that entails.


Thanks,
Razvan

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

Re: [PATCH v4 5/8] arm/vm_event: get/set registers

[Jan Beulich]

>>> On 02.06.16 at 10:26, <rcojocaru@bitdefender.com> wrote:
> On 06/02/2016 10:35 AM, Jan Beulich wrote:
>> The criteria for inclusion or exclusion should
>> follow a predictable model. I.e. if someone comes along and says
>> "I need register Y", then there should be rules that (s)he can apply
>> up front to determine what (at least in the vast majority of cases)
>> the response is going to be. You saying "I need only this arbitrary
>> subset of registers" is completely in-transparent, as it leaves open
>> why this is so, and why this would also be so for others. I.e. you'd
>> at least need to answer the question why (as an example) you
>> need x5 included, but not x25, despite both registers being equal
>> from an architecture pov. An answer like "My application gets away
>> with it" is not acceptable here.
> 
> There will be trade-offs. Again, my preference would be to have a
> multi-page ring buffer with all the VCPU context included in the
> request, always. This would work best as far as both completeness and
> performance goes.
> 
> But that is obviously not a trivial change and I can see how Tamas would
> prefer to get things done sooner along the lines of the pre-existing
> design (whatever its merits and demerits are).
> 
> As long as we're using a smaller ring buffer, the answer (at least as
> I've meant it) is not "_my_ application gets away with it" - which is
> indeed not the most reasonable statement, but "_current_ introspection
> applications - of which my application is one of - don't need more than
> this at this time".

But then could one of you finally say _why_ that is? I.e. why some
GPRs are "better" to your applications than others? If you need
access to _any_ GPRs that don't have a special purpose (like PC,
SP, LR, and maybe FP; I realize this is more applicable to ARM32
than ARM64), I would suspect you need them to e.g. obtain
instruction operands. Yet then I can't see why some of them are
needed while others aren't. I could mayve see you wanting access
to function argument and return value registers, but afaict the
proposed set isn't matching that set either, plus that's a guest ABI
thing anyway, i.e. would again be arbitrary due to you limiting
things to work with just certain guests.

Jan


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

Re: [PATCH v4 5/8] arm/vm_event: get/set registers

[Razvan Cojocaru]

On 06/02/2016 12:38 PM, Jan Beulich wrote:
>>>> On 02.06.16 at 10:26, <rcojocaru@bitdefender.com> wrote:
>> On 06/02/2016 10:35 AM, Jan Beulich wrote:
>>> The criteria for inclusion or exclusion should
>>> follow a predictable model. I.e. if someone comes along and says
>>> "I need register Y", then there should be rules that (s)he can apply
>>> up front to determine what (at least in the vast majority of cases)
>>> the response is going to be. You saying "I need only this arbitrary
>>> subset of registers" is completely in-transparent, as it leaves open
>>> why this is so, and why this would also be so for others. I.e. you'd
>>> at least need to answer the question why (as an example) you
>>> need x5 included, but not x25, despite both registers being equal
>>> from an architecture pov. An answer like "My application gets away
>>> with it" is not acceptable here.
>>
>> There will be trade-offs. Again, my preference would be to have a
>> multi-page ring buffer with all the VCPU context included in the
>> request, always. This would work best as far as both completeness and
>> performance goes.
>>
>> But that is obviously not a trivial change and I can see how Tamas would
>> prefer to get things done sooner along the lines of the pre-existing
>> design (whatever its merits and demerits are).
>>
>> As long as we're using a smaller ring buffer, the answer (at least as
>> I've meant it) is not "_my_ application gets away with it" - which is
>> indeed not the most reasonable statement, but "_current_ introspection
>> applications - of which my application is one of - don't need more than
>> this at this time".
> 
> But then could one of you finally say _why_ that is? I.e. why some
> GPRs are "better" to your applications than others? If you need
> access to _any_ GPRs that don't have a special purpose (like PC,
> SP, LR, and maybe FP; I realize this is more applicable to ARM32
> than ARM64), I would suspect you need them to e.g. obtain
> instruction operands. Yet then I can't see why some of them are
> needed while others aren't. I could mayve see you wanting access
> to function argument and return value registers, but afaict the
> proposed set isn't matching that set either, plus that's a guest ABI
> thing anyway, i.e. would again be arbitrary due to you limiting
> things to work with just certain guests.

Fair point. Unfortunately I can't speak for Tamas' introspection needs
on ARM so he'll have to answer there - for my part, I do include all x86
GPRs in the request.


Thanks,
Razvan

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

Re: [PATCH v4 5/8] arm/vm_event: get/set registers

[Julien Grall]

Hi Tamas,

On 01/06/16 19:21, Tamas K Lengyel wrote:
> On Wed, Jun 1, 2016 at 5:24 AM, Julien Grall <julien.grall@arm.com> wrote:
>> Hi,
>>
>>
>> On 01/06/16 09:41, Jan Beulich wrote:
>>>>>>
>>>>>> On 31.05.16 at 18:28, <tamas@tklengyel.com> wrote:
>>>>
>>>> On May 31, 2016 01:48, "Jan Beulich" <JBeulich@suse.com> wrote:
>>>>>
>>>>>
>>>>>>>> On 30.05.16 at 21:47, <tamas@tklengyel.com> wrote:
>>>>>>
>>>>>> On Mon, May 30, 2016 at 5:50 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>>>>>>
>>>>>>>>>> On 30.05.16 at 00:37, <tamas@tklengyel.com> wrote:
>>>>>>>>
>>>>>>>> +struct vm_event_regs_arm32 {
>>>>>>>> +    uint32_t r0_usr;
>>>>>>>> +    uint32_t r1_usr;
>>>>>>>> +    uint32_t r2_usr;
>>>>>>>> +    uint32_t r3_usr;
>>>>>>>> +    uint32_t r4_usr;
>>>>>>>> +    uint32_t r5_usr;
>>>>>>>> +    uint32_t r6_usr;
>>>>>>>> +    uint32_t r7_usr;
>>>>>>>> +    uint32_t r8_usr;
>>>>>>>> +    uint32_t r9_usr;
>>>>>>>> +    uint32_t r10_usr;
>>>>>>>> +    uint32_t r12_usr;
>>>>>>>> +    uint32_t lr_usr;
>>>>>>>> +    uint32_t fp;
>>>>>>>> +    uint32_t pc;
>>>>>>>> +    uint32_t sp_usr;
>>>>>>>> +    uint32_t sp_svc;
>>>>>>>> +    uint32_t spsr_svc;
>>>>>>>> +};
>>>>>>>
>>>>>>>
>>>>>>> It would seem more natural for the "ordinary" registers to be
>>>>>>> arranged in the numerical sequence, i.e. fp, r12, sp, lr, pc.
>>>>>>
>>>>>>
>>>>>> Not sure I follow.
>>>>>
>>>>>
>>>>> For one it is quite natural for someone looking at a sequence of
>>>>> register values to assume / expect them to be in natural order.
>>>>> And then, having them in natural (numeric) order allows e.g.
>>>>> extracting the register identifying bits from an instruction to use
>>>>> them as an array index into (part of) this structure.
>>>>>
>>>>> (For some background: I've been bitten a number of times by
>>>>> people sorting x86 registers alphabetically instead or naturally,
>>>>> i.e. EAX, EBX, ECX, EDX instead of EAX, ECX, EDX, EBX).
>>>>
>>>>
>>>> I see, however I believe that would be a very careless use of this struct
>>>> from the user as the register sizes are not even necessarily match the
>>>> architecture. For example we only define the 64bit x86 registers, so
>>>> trying
>>>> to access it as an array of 32bit registers wouldn't work at all. Plus we
>>>> are not doing a full set of registers, and I rather not imply that every
>>>> element in the "natural sequence" is present. It may be, it may be not.
>>>
>>>
>>> Once an ABI is set in stone, and if that ABI allows for optimizations
>>> (by consumers) like the one mentioned, I don't think this would be
>>> careless use. The resulting code is very clearly much more efficient
>>> than e.g. a switch() statement with a case label for each and every
>>> register. Well, yes, I already hear the "memory is cheap and hence
>>> code size doesn't matter" argument, but as said elsewhere quite
>>> recently I don't buy this.
>>
>>
>> I agree with Jan here.
>>
>> ARM64 has 31 general purposes registers (x0-x30). The switch to find a
>> register based on the index will be quite big.
>>
>> If you order the register and provide all the general purposes registers (x0
>> - x30), you will be able to replace by a single line (for instance see
>> select_user_reg in arch/arm/traps.c).
>
> The issue is that the x86 vm_event struct right now has 32*uint64_t
> size. So if we would want to transmit all ARM64 GPRs + cpsr, PC and
> TTBR0/1 we would end up growing this structure beyond what it is
> currently. I want to avoid that as it affects both ARM32 and x86
> introspection applications as well as now we can hold fewer events on
> the ring. I would say it is better to avoid that then potentially
> saving some on a switch in ARM64 introspection applications.

The design choice made for x86 should not impact the ARM design (and 
vice-versa). There are key structures in the public interface which 
differ between x86 and ARM (see arch_vcpu_info and arch_shared_info). 
And this is fine because Xen is not meant to run an x86 guest on an ARM 
hypervisor.

As far as I can tell, we currently support VM_EVENT_REASON_MEM_ACCESS 
and VM_EVENT_REASON_GUEST_REQUEST. So technically the structure is set 
in stone. However, we have an interface version that could be bumped, we 
can still decide what is the sensible choice.

With your suggestions only a part of the general-purpose registers will 
be present in the vm_event. I understand that the ring has a limited 
size. If I counted correctly, the current size of the vm_event structure 
is 304 bytes. So 15 vm_event slots are available.

If we grow the structure for ARM64, i.e 3 64-bits registers (PC, TTBR0, 
TTBR1) and 1 32-bit register (CPSR). Which mean 311 bytes, i.e 13 
vm_event slots.

If the vm event subsystem is under pressure, I admit that 2 slots could 
be a lot. However, as not all the GPRs will be available in the 
structure you may have to fetch them, through XEN_DOMCTL_getvcpucontext 
I guess (?). The impact of the introspection to the guest will be 
significant.

I cannot tell how often this will be the case, but I can tell you a 
compiler can do anything with the register allocation. I.e it could 
decide to privilege allocation in registers x20-x30 (because big number 
are nicer).

If you are still concern about the pressure on the ring page, Razvan 
suggested to support multi-ring page.

Regards,

-- 
Julien Grall

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

Re: [PATCH v4 5/8] arm/vm_event: get/set registers

[Julien Grall]

On 01/06/16 20:38, Julien Grall wrote:
> Hi Tamas,
>
> On 01/06/16 19:21, Tamas K Lengyel wrote:
>> On Wed, Jun 1, 2016 at 5:24 AM, Julien Grall <julien.grall@arm.com>
>> wrote:
>>> Hi,
>>>
>>>
>>> On 01/06/16 09:41, Jan Beulich wrote:
>>>>>>>
>>>>>>> On 31.05.16 at 18:28, <tamas@tklengyel.com> wrote:
>>>>>
>>>>> On May 31, 2016 01:48, "Jan Beulich" <JBeulich@suse.com> wrote:
>>>>>>
>>>>>>
>>>>>>>>> On 30.05.16 at 21:47, <tamas@tklengyel.com> wrote:
>>>>>>>
>>>>>>> On Mon, May 30, 2016 at 5:50 AM, Jan Beulich <JBeulich@suse.com>
>>>>>>> wrote:
>>>>>>>>>>>
>>>>>>>>>>> On 30.05.16 at 00:37, <tamas@tklengyel.com> wrote:
>>>>>>>>>
>>>>>>>>> +struct vm_event_regs_arm32 {
>>>>>>>>> +    uint32_t r0_usr;
>>>>>>>>> +    uint32_t r1_usr;
>>>>>>>>> +    uint32_t r2_usr;
>>>>>>>>> +    uint32_t r3_usr;
>>>>>>>>> +    uint32_t r4_usr;
>>>>>>>>> +    uint32_t r5_usr;
>>>>>>>>> +    uint32_t r6_usr;
>>>>>>>>> +    uint32_t r7_usr;
>>>>>>>>> +    uint32_t r8_usr;
>>>>>>>>> +    uint32_t r9_usr;
>>>>>>>>> +    uint32_t r10_usr;
>>>>>>>>> +    uint32_t r12_usr;
>>>>>>>>> +    uint32_t lr_usr;
>>>>>>>>> +    uint32_t fp;
>>>>>>>>> +    uint32_t pc;
>>>>>>>>> +    uint32_t sp_usr;
>>>>>>>>> +    uint32_t sp_svc;
>>>>>>>>> +    uint32_t spsr_svc;
>>>>>>>>> +};
>>>>>>>>
>>>>>>>>
>>>>>>>> It would seem more natural for the "ordinary" registers to be
>>>>>>>> arranged in the numerical sequence, i.e. fp, r12, sp, lr, pc.
>>>>>>>
>>>>>>>
>>>>>>> Not sure I follow.
>>>>>>
>>>>>>
>>>>>> For one it is quite natural for someone looking at a sequence of
>>>>>> register values to assume / expect them to be in natural order.
>>>>>> And then, having them in natural (numeric) order allows e.g.
>>>>>> extracting the register identifying bits from an instruction to use
>>>>>> them as an array index into (part of) this structure.
>>>>>>
>>>>>> (For some background: I've been bitten a number of times by
>>>>>> people sorting x86 registers alphabetically instead or naturally,
>>>>>> i.e. EAX, EBX, ECX, EDX instead of EAX, ECX, EDX, EBX).
>>>>>
>>>>>
>>>>> I see, however I believe that would be a very careless use of this
>>>>> struct
>>>>> from the user as the register sizes are not even necessarily match the
>>>>> architecture. For example we only define the 64bit x86 registers, so
>>>>> trying
>>>>> to access it as an array of 32bit registers wouldn't work at all.
>>>>> Plus we
>>>>> are not doing a full set of registers, and I rather not imply that
>>>>> every
>>>>> element in the "natural sequence" is present. It may be, it may be
>>>>> not.
>>>>
>>>>
>>>> Once an ABI is set in stone, and if that ABI allows for optimizations
>>>> (by consumers) like the one mentioned, I don't think this would be
>>>> careless use. The resulting code is very clearly much more efficient
>>>> than e.g. a switch() statement with a case label for each and every
>>>> register. Well, yes, I already hear the "memory is cheap and hence
>>>> code size doesn't matter" argument, but as said elsewhere quite
>>>> recently I don't buy this.
>>>
>>>
>>> I agree with Jan here.
>>>
>>> ARM64 has 31 general purposes registers (x0-x30). The switch to find a
>>> register based on the index will be quite big.
>>>
>>> If you order the register and provide all the general purposes
>>> registers (x0
>>> - x30), you will be able to replace by a single line (for instance see
>>> select_user_reg in arch/arm/traps.c).
>>
>> The issue is that the x86 vm_event struct right now has 32*uint64_t
>> size. So if we would want to transmit all ARM64 GPRs + cpsr, PC and
>> TTBR0/1 we would end up growing this structure beyond what it is
>> currently. I want to avoid that as it affects both ARM32 and x86
>> introspection applications as well as now we can hold fewer events on
>> the ring. I would say it is better to avoid that then potentially
>> saving some on a switch in ARM64 introspection applications.
>
> The design choice made for x86 should not impact the ARM design (and
> vice-versa). There are key structures in the public interface which
> differ between x86 and ARM (see arch_vcpu_info and arch_shared_info).
> And this is fine because Xen is not meant to run an x86 guest on an ARM
> hypervisor.
>
> As far as I can tell, we currently support VM_EVENT_REASON_MEM_ACCESS
> and VM_EVENT_REASON_GUEST_REQUEST. So technically the structure is set
> in stone. However, we have an interface version that could be bumped, we
> can still decide what is the sensible choice.
>
> With your suggestions only a part of the general-purpose registers will
> be present in the vm_event. I understand that the ring has a limited
> size. If I counted correctly, the current size of the vm_event structure
> is 304 bytes. So 15 vm_event slots are available.
>
> If we grow the structure for ARM64, i.e 3 64-bits registers (PC, TTBR0,
> TTBR1) and 1 32-bit register (CPSR). Which mean 311 bytes, i.e 13

Sorry I miscalculated. So it should be:
   - 332 bytes
   - 12 slots

> vm_event slots.
>
> If the vm event subsystem is under pressure, I admit that 2 slots could
> be a lot. However, as not all the GPRs will be available in the
> structure you may have to fetch them, through XEN_DOMCTL_getvcpucontext
> I guess (?). The impact of the introspection to the guest will be
> significant.
>
> I cannot tell how often this will be the case, but I can tell you a
> compiler can do anything with the register allocation. I.e it could
> decide to privilege allocation in registers x20-x30 (because big number
> are nicer).
>
> If you are still concern about the pressure on the ring page, Razvan
> suggested to support multi-ring page.
>
> Regards,
>

-- 
Julien Grall

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

Re: [PATCH v4 5/8] arm/vm_event: get/set registers

[Tamas K Lengyel]

On Wed, Jun 1, 2016 at 1:38 PM, Julien Grall <julien.grall@arm.com> wrote:
> Hi Tamas,
>
>
> On 01/06/16 19:21, Tamas K Lengyel wrote:
>>
>> On Wed, Jun 1, 2016 at 5:24 AM, Julien Grall <julien.grall@arm.com> wrote:
>>>
>>> Hi,
>>>
>>>
>>> On 01/06/16 09:41, Jan Beulich wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 31.05.16 at 18:28, <tamas@tklengyel.com> wrote:
>>>>>
>>>>>
>>>>> On May 31, 2016 01:48, "Jan Beulich" <JBeulich@suse.com> wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>>>>> On 30.05.16 at 21:47, <tamas@tklengyel.com> wrote:
>>>>>>>
>>>>>>>
>>>>>>> On Mon, May 30, 2016 at 5:50 AM, Jan Beulich <JBeulich@suse.com>
>>>>>>> wrote:
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> On 30.05.16 at 00:37, <tamas@tklengyel.com> wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> +struct vm_event_regs_arm32 {
>>>>>>>>> +    uint32_t r0_usr;
>>>>>>>>> +    uint32_t r1_usr;
>>>>>>>>> +    uint32_t r2_usr;
>>>>>>>>> +    uint32_t r3_usr;
>>>>>>>>> +    uint32_t r4_usr;
>>>>>>>>> +    uint32_t r5_usr;
>>>>>>>>> +    uint32_t r6_usr;
>>>>>>>>> +    uint32_t r7_usr;
>>>>>>>>> +    uint32_t r8_usr;
>>>>>>>>> +    uint32_t r9_usr;
>>>>>>>>> +    uint32_t r10_usr;
>>>>>>>>> +    uint32_t r12_usr;
>>>>>>>>> +    uint32_t lr_usr;
>>>>>>>>> +    uint32_t fp;
>>>>>>>>> +    uint32_t pc;
>>>>>>>>> +    uint32_t sp_usr;
>>>>>>>>> +    uint32_t sp_svc;
>>>>>>>>> +    uint32_t spsr_svc;
>>>>>>>>> +};
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> It would seem more natural for the "ordinary" registers to be
>>>>>>>> arranged in the numerical sequence, i.e. fp, r12, sp, lr, pc.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Not sure I follow.
>>>>>>
>>>>>>
>>>>>>
>>>>>> For one it is quite natural for someone looking at a sequence of
>>>>>> register values to assume / expect them to be in natural order.
>>>>>> And then, having them in natural (numeric) order allows e.g.
>>>>>> extracting the register identifying bits from an instruction to use
>>>>>> them as an array index into (part of) this structure.
>>>>>>
>>>>>> (For some background: I've been bitten a number of times by
>>>>>> people sorting x86 registers alphabetically instead or naturally,
>>>>>> i.e. EAX, EBX, ECX, EDX instead of EAX, ECX, EDX, EBX).
>>>>>
>>>>>
>>>>>
>>>>> I see, however I believe that would be a very careless use of this
>>>>> struct
>>>>> from the user as the register sizes are not even necessarily match the
>>>>> architecture. For example we only define the 64bit x86 registers, so
>>>>> trying
>>>>> to access it as an array of 32bit registers wouldn't work at all. Plus
>>>>> we
>>>>> are not doing a full set of registers, and I rather not imply that
>>>>> every
>>>>> element in the "natural sequence" is present. It may be, it may be not.
>>>>
>>>>
>>>>
>>>> Once an ABI is set in stone, and if that ABI allows for optimizations
>>>> (by consumers) like the one mentioned, I don't think this would be
>>>> careless use. The resulting code is very clearly much more efficient
>>>> than e.g. a switch() statement with a case label for each and every
>>>> register. Well, yes, I already hear the "memory is cheap and hence
>>>> code size doesn't matter" argument, but as said elsewhere quite
>>>> recently I don't buy this.
>>>
>>>
>>>
>>> I agree with Jan here.
>>>
>>> ARM64 has 31 general purposes registers (x0-x30). The switch to find a
>>> register based on the index will be quite big.
>>>
>>> If you order the register and provide all the general purposes registers
>>> (x0
>>> - x30), you will be able to replace by a single line (for instance see
>>> select_user_reg in arch/arm/traps.c).
>>
>>
>> The issue is that the x86 vm_event struct right now has 32*uint64_t
>> size. So if we would want to transmit all ARM64 GPRs + cpsr, PC and
>> TTBR0/1 we would end up growing this structure beyond what it is
>> currently. I want to avoid that as it affects both ARM32 and x86
>> introspection applications as well as now we can hold fewer events on
>> the ring. I would say it is better to avoid that then potentially
>> saving some on a switch in ARM64 introspection applications.
>
>
> The design choice made for x86 should not impact the ARM design (and
> vice-versa). There are key structures in the public interface which differ
> between x86 and ARM (see arch_vcpu_info and arch_shared_info). And this is
> fine because Xen is not meant to run an x86 guest on an ARM hypervisor.
>
> As far as I can tell, we currently support VM_EVENT_REASON_MEM_ACCESS and
> VM_EVENT_REASON_GUEST_REQUEST. So technically the structure is set in stone.
> However, we have an interface version that could be bumped, we can still
> decide what is the sensible choice.
>
> With your suggestions only a part of the general-purpose registers will be
> present in the vm_event. I understand that the ring has a limited size. If I
> counted correctly, the current size of the vm_event structure is 304 bytes.
> So 15 vm_event slots are available.
>
> If we grow the structure for ARM64, i.e 3 64-bits registers (PC, TTBR0,
> TTBR1) and 1 32-bit register (CPSR). Which mean 311 bytes, i.e 13 vm_event
> slots.
>
> If the vm event subsystem is under pressure, I admit that 2 slots could be a
> lot. However, as not all the GPRs will be available in the structure you may
> have to fetch them, through XEN_DOMCTL_getvcpucontext I guess (?). The
> impact of the introspection to the guest will be significant.
>
> I cannot tell how often this will be the case, but I can tell you a compiler
> can do anything with the register allocation. I.e it could decide to
> privilege allocation in registers x20-x30 (because big number are nicer).

Fair point.

>
> If you are still concern about the pressure on the ring page, Razvan
> suggested to support multi-ring page.

Going the multi-page ring route is a bit beyond of what I would like
to get reasonable done right now. As per Razvan's comment growing the
struct size is not that big of a concern so I'll include all ARM64
GPRs in the next revision.

Thanks,
Tamas

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

[PATCH v4 6/8] tools/libxc: add xc_monitor_privileged_call

[Tamas K Lengyel]

These are the user-space components for the new ARM SMC events.

Signed-off-by: Tamas K Lengyel <tamas@tklengyel.com>
Acked-by: Wei Liu <wei.liu2@citrix.com>
---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
---
 tools/libxc/include/xenctrl.h |  2 ++
 tools/libxc/xc_monitor.c      | 24 ++++++++++++++++++++++++
 2 files changed, 26 insertions(+)

diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index dc54612..3085130 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -2160,6 +2160,8 @@ int xc_monitor_software_breakpoint(xc_interface *xch, domid_t domain_id,
                                    bool enable);
 int xc_monitor_guest_request(xc_interface *xch, domid_t domain_id,
                              bool enable, bool sync);
+int xc_monitor_privileged_call(xc_interface *xch, domid_t domain_id,
+                               bool enable);
 
 /**
  * This function enables / disables emulation for each REP for a
diff --git a/tools/libxc/xc_monitor.c b/tools/libxc/xc_monitor.c
index b1705dd..1e4a9d2 100644
--- a/tools/libxc/xc_monitor.c
+++ b/tools/libxc/xc_monitor.c
@@ -156,3 +156,27 @@ int xc_monitor_emulate_each_rep(xc_interface *xch, domid_t domain_id,
 
     return do_domctl(xch, &domctl);
 }
+
+int xc_monitor_privileged_call(xc_interface *xch, domid_t domain_id,
+                               bool enable)
+{
+    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_PRIVILEGED_CALL;
+
+    return do_domctl(xch, &domctl);
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
-- 
2.8.1


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

[PATCH v4 7/8] tools/xen-access: add test-case for ARM SMC

[Tamas K Lengyel]

Signed-off-by: Tamas K Lengyel <tamas@tklengyel.com>
Acked-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
---
 tools/tests/xen-access/xen-access.c | 48 ++++++++++++++++++++++++++++++++++---
 1 file changed, 45 insertions(+), 3 deletions(-)

diff --git a/tools/tests/xen-access/xen-access.c b/tools/tests/xen-access/xen-access.c
index f26e723..1b2b3fd 100644
--- a/tools/tests/xen-access/xen-access.c
+++ b/tools/tests/xen-access/xen-access.c
@@ -1,5 +1,4 @@
-/*
- * xen-access.c
+/* xen-access.c
  *
  * Exercises the basic per-page access mechanisms
  *
@@ -334,6 +333,8 @@ void usage(char* progname)
     fprintf(stderr, "Usage: %s [-m] <domain_id> write|exec", progname);
 #if defined(__i386__) || defined(__x86_64__)
             fprintf(stderr, "|breakpoint|altp2m_write|altp2m_exec");
+#elif defined(__arm__) || defined(__aarch64__)
+            fprintf(stderr, "|privcall");
 #endif
             fprintf(stderr,
             "\n"
@@ -357,6 +358,7 @@ int main(int argc, char *argv[])
     int required = 0;
     int breakpoint = 0;
     int shutting_down = 0;
+    int privcall = 0;
     int altp2m = 0;
     uint16_t altp2m_view_id = 0;
 
@@ -412,6 +414,11 @@ int main(int argc, char *argv[])
         default_access = XENMEM_access_rw;
         altp2m = 1;
     }
+#elif defined(__arm__) || defined(__aarch64__)
+    else if ( !strcmp(argv[0], "privcall") )
+    {
+        privcall = 1;
+    }
 #endif
     else
     {
@@ -524,6 +531,16 @@ int main(int argc, char *argv[])
         }
     }
 
+    if ( privcall )
+    {
+        rc = xc_monitor_privileged_call(xch, domain_id, 1);
+        if ( rc < 0 )
+        {
+            ERROR("Error %d setting privileged call trapping with vm_event\n", rc);
+            goto exit;
+        }
+    }
+
     /* Wait for access */
     for (;;)
     {
@@ -535,6 +552,9 @@ int main(int argc, char *argv[])
             if ( breakpoint )
                 rc = xc_monitor_software_breakpoint(xch, domain_id, 0);
 
+            if ( privcall )
+                rc = xc_monitor_privileged_call(xch, domain_id, 0);
+
             if ( altp2m )
             {
                 rc = xc_altp2m_switch_to_view( xch, domain_id, 0 );
@@ -635,7 +655,7 @@ int main(int argc, char *argv[])
                 rsp.u.mem_access = req.u.mem_access;
                 break;
             case VM_EVENT_REASON_SOFTWARE_BREAKPOINT:
-                printf("Breakpoint: rip=%016"PRIx64", gfn=%"PRIx64" (vcpu %d)\n",
+                printf("Breakpoint: rip=%016"PRIx64" gfn=%"PRIx64" (vcpu %d)\n",
                        req.data.regs.x86.rip,
                        req.u.software_breakpoint.gfn,
                        req.vcpu_id);
@@ -650,7 +670,29 @@ int main(int argc, char *argv[])
                     interrupted = -1;
                     continue;
                 }
+                break;
+            case VM_EVENT_REASON_PRIVILEGED_CALL:
+                {
+                    const struct vm_event_regs_arm *in_regs = &req.data.regs.arm;
+                    struct vm_event_regs_arm *out_regs = &rsp.data.regs.arm;
+                    bool is32bit = !!(in_regs->cpsr & PSR_MODE_BIT);
+                    uint64_t pc;
+
+                    *out_regs = *in_regs;
+
+                    if ( is32bit ) {
+                        pc = in_regs->arch.arm32.pc;
+                        out_regs->arch.arm32.pc += 4;
+                    } else {
+                        pc = in_regs->arch.arm64.pc;
+                        out_regs->arch.arm64.pc += 8;
+                    }
 
+                    printf("Privileged call: pc=%016"PRIx64" (vcpu %d)\n",
+                           pc, req.vcpu_id);
+
+                    rsp.flags |= VM_EVENT_FLAG_SET_REGISTERS;
+                }
                 break;
             case VM_EVENT_REASON_SINGLESTEP:
                 printf("Singlestep: rip=%016"PRIx64", vcpu %d, altp2m %u\n",
-- 
2.8.1


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

Re: [PATCH v4 7/8] tools/xen-access: add test-case for ARM SMC

[Wei Liu]

On Sun, May 29, 2016 at 04:37:09PM -0600, Tamas K Lengyel wrote:
> Signed-off-by: Tamas K Lengyel <tamas@tklengyel.com>
> Acked-by: Razvan Cojocaru <rcojocaru@bitdefender.com>

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

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

[PATCH v4 8/8] x86/vm_event: Add HVM debug exception vm_events

[Tamas K Lengyel]

Since in-guest debug exceptions are now unconditionally trapped to Xen, adding
a hook for vm_event subscribers to tap into this new always-on guest event. We
rename along the way hvm_event_breakpoint_type to hvm_event_type to better
match the various events that can be passed with it. We also introduce the
necessary monitor_op domctl's to enable subscribing to the events.

Signed-off-by: Tamas K Lengyel <tamas@tklengyel.com>
---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: Razvan Cojocaru <rcojocaru@bitdefender.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Jun Nakajima <jun.nakajima@intel.com>
Cc: Kevin Tian <kevin.tian@intel.com>

v3: Increment vm_event interface version
v2: Rename hvm_monitor_event to hvm_monitor_debug
---
 tools/libxc/include/xenctrl.h       |  3 +-
 tools/libxc/xc_monitor.c            | 15 +++++++++
 tools/tests/xen-access/xen-access.c | 61 ++++++++++++++++++++++++++++++++-----
 xen/arch/x86/hvm/monitor.c          | 26 ++++++++++++++--
 xen/arch/x86/hvm/vmx/vmx.c          | 26 +++++++++++++---
 xen/arch/x86/monitor.c              | 16 ++++++++++
 xen/include/asm-x86/domain.h        |  2 ++
 xen/include/asm-x86/hvm/monitor.h   |  7 +++--
 xen/include/asm-x86/monitor.h       |  3 +-
 xen/include/public/domctl.h         |  6 ++++
 xen/include/public/vm_event.h       | 14 +++++++--
 11 files changed, 157 insertions(+), 22 deletions(-)

diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index 3085130..29d983e 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -2162,7 +2162,8 @@ int xc_monitor_guest_request(xc_interface *xch, domid_t domain_id,
                              bool enable, bool sync);
 int xc_monitor_privileged_call(xc_interface *xch, domid_t domain_id,
                                bool enable);
-
+int xc_monitor_debug_exceptions(xc_interface *xch, domid_t domain_id,
+                                bool enable, bool sync);
 /**
  * This function enables / disables emulation for each REP for a
  * REP-compatible instruction.
diff --git a/tools/libxc/xc_monitor.c b/tools/libxc/xc_monitor.c
index 1e4a9d2..b0bf708 100644
--- a/tools/libxc/xc_monitor.c
+++ b/tools/libxc/xc_monitor.c
@@ -171,6 +171,21 @@ int xc_monitor_privileged_call(xc_interface *xch, domid_t domain_id,
     return do_domctl(xch, &domctl);
 }
 
+int xc_monitor_debug_exceptions(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_DEBUG_EXCEPTION;
+    domctl.u.monitor_op.u.debug_exception.sync = sync;
+
+    return do_domctl(xch, &domctl);
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/tools/tests/xen-access/xen-access.c b/tools/tests/xen-access/xen-access.c
index 1b2b3fd..8678ccd 100644
--- a/tools/tests/xen-access/xen-access.c
+++ b/tools/tests/xen-access/xen-access.c
@@ -52,6 +52,10 @@
 #define ERROR(a, b...) fprintf(stderr, a "\n", ## b)
 #define PERROR(a, b...) fprintf(stderr, a ": %s\n", ## b, strerror(errno))
 
+/* From xen/include/asm-x86/processor.h */
+#define X86_TRAP_DEBUG  1
+#define X86_TRAP_INT3   3
+
 typedef struct vm_event {
     domid_t domain_id;
     xenevtchn_handle *xce_handle;
@@ -332,7 +336,7 @@ void usage(char* progname)
 {
     fprintf(stderr, "Usage: %s [-m] <domain_id> write|exec", progname);
 #if defined(__i386__) || defined(__x86_64__)
-            fprintf(stderr, "|breakpoint|altp2m_write|altp2m_exec");
+            fprintf(stderr, "|breakpoint|altp2m_write|altp2m_exec|debug");
 #elif defined(__arm__) || defined(__aarch64__)
             fprintf(stderr, "|privcall");
 #endif
@@ -355,11 +359,13 @@ int main(int argc, char *argv[])
     xc_interface *xch;
     xenmem_access_t default_access = XENMEM_access_rwx;
     xenmem_access_t after_first_access = XENMEM_access_rwx;
+    int memaccess = 0;
     int required = 0;
     int breakpoint = 0;
     int shutting_down = 0;
     int privcall = 0;
     int altp2m = 0;
+    int debug = 0;
     uint16_t altp2m_view_id = 0;
 
     char* progname = argv[0];
@@ -393,11 +399,13 @@ int main(int argc, char *argv[])
     {
         default_access = XENMEM_access_rx;
         after_first_access = XENMEM_access_rwx;
+        memaccess = 1;
     }
     else if ( !strcmp(argv[0], "exec") )
     {
         default_access = XENMEM_access_rw;
         after_first_access = XENMEM_access_rwx;
+        memaccess = 1;
     }
 #if defined(__i386__) || defined(__x86_64__)
     else if ( !strcmp(argv[0], "breakpoint") )
@@ -408,11 +416,17 @@ int main(int argc, char *argv[])
     {
         default_access = XENMEM_access_rx;
         altp2m = 1;
+        memaccess = 1;
     }
     else if ( !strcmp(argv[0], "altp2m_exec") )
     {
         default_access = XENMEM_access_rw;
         altp2m = 1;
+        memaccess = 1;
+    }
+    else if ( !strcmp(argv[0], "debug") )
+    {
+        debug = 1;
     }
 #elif defined(__arm__) || defined(__aarch64__)
     else if ( !strcmp(argv[0], "privcall") )
@@ -453,7 +467,7 @@ int main(int argc, char *argv[])
     }
 
     /* With altp2m we just create a new, restricted view of the memory */
-    if ( altp2m )
+    if ( memaccess && altp2m )
     {
         xen_pfn_t gfn = 0;
         unsigned long perm_set = 0;
@@ -500,7 +514,7 @@ int main(int argc, char *argv[])
         }
     }
 
-    if ( !altp2m )
+    if ( memaccess && !altp2m )
     {
         /* Set the default access type and convert all pages to it */
         rc = xc_set_mem_access(xch, domain_id, default_access, ~0ull, 0);
@@ -541,6 +555,16 @@ int main(int argc, char *argv[])
         }
     }
 
+    if ( debug )
+    {
+        rc = xc_monitor_debug_exceptions(xch, domain_id, 1, 1);
+        if ( rc < 0 )
+        {
+            ERROR("Error %d setting debug exception listener with vm_event\n", rc);
+            goto exit;
+        }
+    }
+
     /* Wait for access */
     for (;;)
     {
@@ -551,9 +575,10 @@ int main(int argc, char *argv[])
 
             if ( breakpoint )
                 rc = xc_monitor_software_breakpoint(xch, domain_id, 0);
-
             if ( privcall )
                 rc = xc_monitor_privileged_call(xch, domain_id, 0);
+            if ( debug )
+                rc = xc_monitor_debug_exceptions(xch, domain_id, 0, 0);
 
             if ( altp2m )
             {
@@ -661,9 +686,10 @@ int main(int argc, char *argv[])
                        req.vcpu_id);
 
                 /* Reinject */
-                rc = xc_hvm_inject_trap(
-                    xch, domain_id, req.vcpu_id, 3,
-                    HVMOP_TRAP_sw_exc, -1, 0, 0);
+                rc = xc_hvm_inject_trap(xch, domain_id, req.vcpu_id,
+                                        X86_TRAP_INT3,
+                                        req.u.software_breakpoint.type, -1,
+                                        req.u.software_breakpoint.insn_length, 0);
                 if (rc < 0)
                 {
                     ERROR("Error %d injecting breakpoint\n", rc);
@@ -711,6 +737,27 @@ int main(int argc, char *argv[])
                 rsp.flags |= VM_EVENT_FLAG_TOGGLE_SINGLESTEP;
 
                 break;
+            case VM_EVENT_REASON_DEBUG_EXCEPTION:
+                printf("Debug exception: rip=%016"PRIx64", vcpu %d. Type: %u. Length: %u\n",
+                       req.data.regs.x86.rip,
+                       req.vcpu_id,
+                       req.u.debug_exception.type,
+                       req.u.debug_exception.insn_length);
+
+                /* Reinject */
+                rc = xc_hvm_inject_trap(xch, domain_id, req.vcpu_id,
+                                        X86_TRAP_DEBUG,
+                                        req.u.debug_exception.type, -1,
+                                        req.u.debug_exception.insn_length,
+                                        req.data.regs.x86.cr2);
+                if (rc < 0)
+                {
+                    ERROR("Error %d injecting breakpoint\n", rc);
+                    interrupted = -1;
+                    continue;
+                }
+
+                break;
             default:
                 fprintf(stderr, "UNKNOWN REASON CODE %d\n", req.reason);
             }
diff --git a/xen/arch/x86/hvm/monitor.c b/xen/arch/x86/hvm/monitor.c
index 8b41b56..5e040ed 100644
--- a/xen/arch/x86/hvm/monitor.c
+++ b/xen/arch/x86/hvm/monitor.c
@@ -88,12 +88,14 @@ static inline unsigned long gfn_of_rip(unsigned long rip)
     return paging_gva_to_gfn(curr, sreg.base + rip, &pfec);
 }
 
-int hvm_monitor_breakpoint(unsigned long rip,
-                           enum hvm_monitor_breakpoint_type type)
+int hvm_monitor_debug(unsigned long rip, enum hvm_monitor_debug_type type,
+                      unsigned long trap_type, unsigned long insn_length)
 {
     struct vcpu *curr = current;
     struct arch_domain *ad = &curr->domain->arch;
     vm_event_request_t req = {};
+    bool_t sync;
+    int rc;
 
     switch ( type )
     {
@@ -102,6 +104,9 @@ int hvm_monitor_breakpoint(unsigned long rip,
             return 0;
         req.reason = VM_EVENT_REASON_SOFTWARE_BREAKPOINT;
         req.u.software_breakpoint.gfn = gfn_of_rip(rip);
+        req.u.software_breakpoint.type = trap_type;
+        req.u.software_breakpoint.insn_length = insn_length;
+        sync = 1;
         break;
 
     case HVM_MONITOR_SINGLESTEP_BREAKPOINT:
@@ -109,6 +114,17 @@ int hvm_monitor_breakpoint(unsigned long rip,
             return 0;
         req.reason = VM_EVENT_REASON_SINGLESTEP;
         req.u.singlestep.gfn = gfn_of_rip(rip);
+        sync = 1;
+        break;
+
+    case HVM_MONITOR_DEBUG_EXCEPTION:
+        if ( !ad->monitor.debug_exception_enabled )
+            return 0;
+        req.reason = VM_EVENT_REASON_DEBUG_EXCEPTION;
+        req.u.debug_exception.gfn = gfn_of_rip(rip);
+        req.u.debug_exception.type = trap_type;
+        req.u.debug_exception.insn_length = insn_length;
+        sync = !!ad->monitor.debug_exception_sync;
         break;
 
     default:
@@ -117,7 +133,11 @@ int hvm_monitor_breakpoint(unsigned long rip,
 
     req.vcpu_id = curr->vcpu_id;
 
-    return vm_event_monitor_traps(curr, 1, &req);
+    rc = vm_event_monitor_traps(curr, sync, &req);
+    if ( type == HVM_MONITOR_DEBUG_EXCEPTION && rc == 1 && !sync )
+        rc = 0;
+
+    return rc;
 }
 
 /*
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 081aef1..cea0761 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -3377,9 +3377,25 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
             HVMTRACE_1D(TRAP_DEBUG, exit_qualification);
             write_debugreg(6, exit_qualification | DR_STATUS_RESERVED_ONE);
             if ( !v->domain->debugger_attached )
-                vmx_propagate_intr(intr_info);
+            {
+                unsigned long insn_length = 0;
+                int handled;
+                unsigned long trap_type = MASK_EXTR(intr_info,
+                                                    INTR_INFO_INTR_TYPE_MASK);
+
+                if( trap_type >= X86_EVENTTYPE_SW_INTERRUPT )
+                    __vmread(VM_EXIT_INSTRUCTION_LEN, &insn_length);
+
+                handled = hvm_monitor_debug(regs->eip,
+                                            HVM_MONITOR_DEBUG_EXCEPTION,
+                                            trap_type, insn_length);
+                if ( handled <= 0 )
+                    vmx_propagate_intr(intr_info);
+
+            }
             else
                 domain_pause_for_debugger();
+
             break;
         case TRAP_int3: 
         {
@@ -3393,8 +3409,9 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
             }
             else {
                 int handled =
-                    hvm_monitor_breakpoint(regs->eip,
-                                           HVM_MONITOR_SOFTWARE_BREAKPOINT);
+                        hvm_monitor_debug(regs->eip,
+                                          HVM_MONITOR_SOFTWARE_BREAKPOINT,
+                                          X86_EVENTTYPE_SW_EXCEPTION, 1);
 
                 if ( handled < 0 ) 
                 {
@@ -3721,8 +3738,7 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
         vmx_update_cpu_exec_control(v);
         if ( v->arch.hvm_vcpu.single_step )
         {
-            hvm_monitor_breakpoint(regs->eip,
-                                   HVM_MONITOR_SINGLESTEP_BREAKPOINT);
+            hvm_monitor_debug(regs->eip, HVM_MONITOR_SINGLESTEP_BREAKPOINT, 0, 0);
             if ( v->domain->debugger_attached )
                 domain_pause_for_debugger();
         }
diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c
index 621f91a..e8b79f6 100644
--- a/xen/arch/x86/monitor.c
+++ b/xen/arch/x86/monitor.c
@@ -124,6 +124,22 @@ int arch_monitor_domctl_event(struct domain *d,
         break;
     }
 
+    case XEN_DOMCTL_MONITOR_EVENT_DEBUG_EXCEPTION:
+    {
+        bool_t old_status = ad->monitor.debug_exception_enabled;
+
+        if ( unlikely(old_status == requested_status) )
+            return -EEXIST;
+
+        domain_pause(d);
+        ad->monitor.debug_exception_enabled = requested_status;
+        ad->monitor.debug_exception_sync = requested_status ?
+                                            mop->u.debug_exception.sync :
+                                            0;
+        domain_unpause(d);
+        break;
+    }
+
     default:
         /*
          * Should not be reached unless arch_monitor_get_capabilities() is
diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
index 165e533..1cf97c3 100644
--- a/xen/include/asm-x86/domain.h
+++ b/xen/include/asm-x86/domain.h
@@ -405,6 +405,8 @@ struct arch_domain
         unsigned int mov_to_msr_extended         : 1;
         unsigned int singlestep_enabled          : 1;
         unsigned int software_breakpoint_enabled : 1;
+        unsigned int debug_exception_enabled     : 1;
+        unsigned int debug_exception_sync        : 1;
     } monitor;
 
     /* Mem_access emulation control */
diff --git a/xen/include/asm-x86/hvm/monitor.h b/xen/include/asm-x86/hvm/monitor.h
index 55d435e..8b0f119 100644
--- a/xen/include/asm-x86/hvm/monitor.h
+++ b/xen/include/asm-x86/hvm/monitor.h
@@ -23,10 +23,11 @@
 #include <xen/paging.h>
 #include <public/vm_event.h>
 
-enum hvm_monitor_breakpoint_type
+enum hvm_monitor_debug_type
 {
     HVM_MONITOR_SOFTWARE_BREAKPOINT,
     HVM_MONITOR_SINGLESTEP_BREAKPOINT,
+    HVM_MONITOR_DEBUG_EXCEPTION,
 };
 
 /*
@@ -39,8 +40,8 @@ bool_t hvm_monitor_cr(unsigned int index, unsigned long value,
 #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);
-int hvm_monitor_breakpoint(unsigned long rip,
-                           enum hvm_monitor_breakpoint_type type);
+int hvm_monitor_debug(unsigned long rip, enum hvm_monitor_debug_type type,
+                      unsigned long trap_type, unsigned long insn_length);
 
 #endif /* __ASM_X86_HVM_MONITOR_H__ */
 
diff --git a/xen/include/asm-x86/monitor.h b/xen/include/asm-x86/monitor.h
index 0fee750..74a08f0f 100644
--- a/xen/include/asm-x86/monitor.h
+++ b/xen/include/asm-x86/monitor.h
@@ -74,7 +74,8 @@ static inline uint32_t arch_monitor_get_capabilities(struct domain *d)
     capabilities = (1U << XEN_DOMCTL_MONITOR_EVENT_WRITE_CTRLREG) |
                    (1U << XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR) |
                    (1U << XEN_DOMCTL_MONITOR_EVENT_SOFTWARE_BREAKPOINT) |
-                   (1U << XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST);
+                   (1U << XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST) |
+                   (1U << XEN_DOMCTL_MONITOR_EVENT_DEBUG_EXCEPTION);
 
     /* Since we know this is on VMX, we can just call the hvm func */
     if ( hvm_is_singlestep_supported() )
diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index 35adce2..b69b1bb 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -1081,6 +1081,7 @@ DEFINE_XEN_GUEST_HANDLE(xen_domctl_psr_cmt_op_t);
 #define XEN_DOMCTL_MONITOR_EVENT_SOFTWARE_BREAKPOINT   3
 #define XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST         4
 #define XEN_DOMCTL_MONITOR_EVENT_PRIVILEGED_CALL       5
+#define XEN_DOMCTL_MONITOR_EVENT_DEBUG_EXCEPTION       6
 
 struct xen_domctl_monitor_op {
     uint32_t op; /* XEN_DOMCTL_MONITOR_OP_* */
@@ -1116,6 +1117,11 @@ struct xen_domctl_monitor_op {
             /* Pause vCPU until response */
             uint8_t sync;
         } guest_request;
+
+        struct {
+            /* Pause vCPU until response */
+            uint8_t sync;
+        } debug_exception;
     } u;
 };
 typedef struct xen_domctl_monitor_op xen_domctl_monitor_op_t;
diff --git a/xen/include/public/vm_event.h b/xen/include/public/vm_event.h
index 6ff7cc0..13855b5 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 0x00000001
+#define VM_EVENT_INTERFACE_VERSION 0x00000002
 
 #if defined(__XEN__) || defined(__XEN_TOOLS__)
 
@@ -121,6 +121,8 @@
 #define VM_EVENT_REASON_GUEST_REQUEST           8
 /* Privileged call executed (e.g. SMC) */
 #define VM_EVENT_REASON_PRIVILEGED_CALL         9
+/* A debug exception was caught */
+#define VM_EVENT_REASON_DEBUG_EXCEPTION         10
 
 /* Supported values for the vm_event_write_ctrlreg index. */
 #define VM_EVENT_X86_CR0    0
@@ -258,8 +260,15 @@ struct vm_event_write_ctrlreg {
     uint64_t old_value;
 };
 
+struct vm_event_singlestep {
+    uint64_t gfn;
+};
+
 struct vm_event_debug {
     uint64_t gfn;
+    uint8_t type;        /* HVMOP_TRAP_* */
+    uint8_t insn_length;
+    uint8_t _pad[6];
 };
 
 struct vm_event_mov_to_msr {
@@ -303,7 +312,8 @@ typedef struct vm_event_st {
         struct vm_event_write_ctrlreg         write_ctrlreg;
         struct vm_event_mov_to_msr            mov_to_msr;
         struct vm_event_debug                 software_breakpoint;
-        struct vm_event_debug                 singlestep;
+        struct vm_event_singlestep            singlestep;
+        struct vm_event_debug                 debug_exception;
     } u;
 
     union {
-- 
2.8.1


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

Re: [PATCH v4 8/8] x86/vm_event: Add HVM debug exception vm_events

[Razvan Cojocaru]

On 05/30/2016 01:37 AM, Tamas K Lengyel wrote:
> Since in-guest debug exceptions are now unconditionally trapped to Xen, adding
> a hook for vm_event subscribers to tap into this new always-on guest event. We
> rename along the way hvm_event_breakpoint_type to hvm_event_type to better
> match the various events that can be passed with it. We also introduce the
> necessary monitor_op domctl's to enable subscribing to the events.
> 
> Signed-off-by: Tamas K Lengyel <tamas@tklengyel.com>
> ---
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
> Cc: Razvan Cojocaru <rcojocaru@bitdefender.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: Jun Nakajima <jun.nakajima@intel.com>
> Cc: Kevin Tian <kevin.tian@intel.com>
> 
> v3: Increment vm_event interface version
> v2: Rename hvm_monitor_event to hvm_monitor_debug
> ---
>  tools/libxc/include/xenctrl.h       |  3 +-
>  tools/libxc/xc_monitor.c            | 15 +++++++++
>  tools/tests/xen-access/xen-access.c | 61 ++++++++++++++++++++++++++++++++-----
>  xen/arch/x86/hvm/monitor.c          | 26 ++++++++++++++--
>  xen/arch/x86/hvm/vmx/vmx.c          | 26 +++++++++++++---
>  xen/arch/x86/monitor.c              | 16 ++++++++++
>  xen/include/asm-x86/domain.h        |  2 ++
>  xen/include/asm-x86/hvm/monitor.h   |  7 +++--
>  xen/include/asm-x86/monitor.h       |  3 +-
>  xen/include/public/domctl.h         |  6 ++++
>  xen/include/public/vm_event.h       | 14 +++++++--
>  11 files changed, 157 insertions(+), 22 deletions(-)

Acked-by: Razvan Cojocaru <rcojocaru@bitdefender.com>

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

Re: [PATCH v4 8/8] x86/vm_event: Add HVM debug exception vm_events

[Jan Beulich]

>>> On 30.05.16 at 00:37, <tamas@tklengyel.com> wrote:
> @@ -117,7 +133,11 @@ int hvm_monitor_breakpoint(unsigned long rip,
>  
>      req.vcpu_id = curr->vcpu_id;
>  
> -    return vm_event_monitor_traps(curr, 1, &req);
> +    rc = vm_event_monitor_traps(curr, sync, &req);
> +    if ( type == HVM_MONITOR_DEBUG_EXCEPTION && rc == 1 && !sync )
> +        rc = 0;
> +
> +    return rc;
>  }

To someone like me, not intimately familiar with the code, this added
logic looks pretty arbitrary. Please add a comment indicating why
under these special circumstances rc needs to be altered here, which
then will hopefully also clarify why that can't be done right in
vm_event_monitor_traps().

> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -3377,9 +3377,25 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
>              HVMTRACE_1D(TRAP_DEBUG, exit_qualification);
>              write_debugreg(6, exit_qualification | DR_STATUS_RESERVED_ONE);
>              if ( !v->domain->debugger_attached )
> -                vmx_propagate_intr(intr_info);
> +            {
> +                unsigned long insn_length = 0;
> +                int handled;

The variable name suggests it wants to be bool_t, but ...

> +                unsigned long trap_type = MASK_EXTR(intr_info,
> +                                                    INTR_INFO_INTR_TYPE_MASK);
> +
> +                if( trap_type >= X86_EVENTTYPE_SW_INTERRUPT )
> +                    __vmread(VM_EXIT_INSTRUCTION_LEN, &insn_length);
> +
> +                handled = hvm_monitor_debug(regs->eip,
> +                                            HVM_MONITOR_DEBUG_EXCEPTION,
> +                                            trap_type, insn_length);
> +                if ( handled <= 0 )

... it clearly can't. Please use a better name (could by just "rc" or
"ret"). (Otoh I see that code you modify further down uses that
same naming for a similar purpose variable. Let's see what the VMX
maintainers say.)

> +                    vmx_propagate_intr(intr_info);
> +
> +            }
>              else
>                  domain_pause_for_debugger();
> +
>              break;
>          case TRAP_int3: 

If anywhere, this added blank line wants to go after the break.

> @@ -3393,8 +3409,9 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
>              }
>              else {
>                  int handled =
> -                    hvm_monitor_breakpoint(regs->eip,
> -                                           HVM_MONITOR_SOFTWARE_BREAKPOINT);
> +                        hvm_monitor_debug(regs->eip,
> +                                          HVM_MONITOR_SOFTWARE_BREAKPOINT,
> +                                          X86_EVENTTYPE_SW_EXCEPTION, 1);

Please let's not add further mistakes like this, assuming INT3 can't
have any prefixes. It can, even if they're useless.

> @@ -3721,8 +3738,7 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
>          vmx_update_cpu_exec_control(v);
>          if ( v->arch.hvm_vcpu.single_step )
>          {
> -            hvm_monitor_breakpoint(regs->eip,
> -                                   HVM_MONITOR_SINGLESTEP_BREAKPOINT);
> +            hvm_monitor_debug(regs->eip, HVM_MONITOR_SINGLESTEP_BREAKPOINT, 0, 0);

How come the 3rd argument is literal zero here?

Also you're creating a long line here.

Jan


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

Re: [PATCH v4 8/8] x86/vm_event: Add HVM debug exception vm_events

[Tamas K Lengyel]

On Mon, May 30, 2016 at 8:16 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 30.05.16 at 00:37, <tamas@tklengyel.com> wrote:
>> @@ -117,7 +133,11 @@ int hvm_monitor_breakpoint(unsigned long rip,
>>
>>      req.vcpu_id = curr->vcpu_id;
>>
>> -    return vm_event_monitor_traps(curr, 1, &req);
>> +    rc = vm_event_monitor_traps(curr, sync, &req);
>> +    if ( type == HVM_MONITOR_DEBUG_EXCEPTION && rc == 1 && !sync )
>> +        rc = 0;
>> +
>> +    return rc;
>>  }
>
> To someone like me, not intimately familiar with the code, this added
> logic looks pretty arbitrary. Please add a comment indicating why
> under these special circumstances rc needs to be altered here, which
> then will hopefully also clarify why that can't be done right in
> vm_event_monitor_traps().

Yea, vm_event_monitor_traps is not the most straight forward function
in regards of what the return value means. I'll see if I can clean it
up a bit in another patch.

>
>> --- a/xen/arch/x86/hvm/vmx/vmx.c
>> +++ b/xen/arch/x86/hvm/vmx/vmx.c
>> @@ -3377,9 +3377,25 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
>>              HVMTRACE_1D(TRAP_DEBUG, exit_qualification);
>>              write_debugreg(6, exit_qualification | DR_STATUS_RESERVED_ONE);
>>              if ( !v->domain->debugger_attached )
>> -                vmx_propagate_intr(intr_info);
>> +            {
>> +                unsigned long insn_length = 0;
>> +                int handled;
>
> The variable name suggests it wants to be bool_t, but ...
>
>> +                unsigned long trap_type = MASK_EXTR(intr_info,
>> +                                                    INTR_INFO_INTR_TYPE_MASK);
>> +
>> +                if( trap_type >= X86_EVENTTYPE_SW_INTERRUPT )
>> +                    __vmread(VM_EXIT_INSTRUCTION_LEN, &insn_length);
>> +
>> +                handled = hvm_monitor_debug(regs->eip,
>> +                                            HVM_MONITOR_DEBUG_EXCEPTION,
>> +                                            trap_type, insn_length);
>> +                if ( handled <= 0 )
>
> ... it clearly can't. Please use a better name (could by just "rc" or
> "ret"). (Otoh I see that code you modify further down uses that
> same naming for a similar purpose variable. Let's see what the VMX
> maintainers say.)
>
>> +                    vmx_propagate_intr(intr_info);
>> +
>> +            }
>>              else
>>                  domain_pause_for_debugger();
>> +
>>              break;
>>          case TRAP_int3:
>
> If anywhere, this added blank line wants to go after the break.
>
>> @@ -3393,8 +3409,9 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
>>              }
>>              else {
>>                  int handled =
>> -                    hvm_monitor_breakpoint(regs->eip,
>> -                                           HVM_MONITOR_SOFTWARE_BREAKPOINT);
>> +                        hvm_monitor_debug(regs->eip,
>> +                                          HVM_MONITOR_SOFTWARE_BREAKPOINT,
>> +                                          X86_EVENTTYPE_SW_EXCEPTION, 1);
>
> Please let's not add further mistakes like this, assuming INT3 can't
> have any prefixes. It can, even if they're useless.

You mean the instruction length is not necessarily 1? Ultimately it
doesn't seem to matter because reinjecting it with xc_hvm_inject_trap
ignores this field. Instruction length is only required to be properly
set AFAICT for a subset of debug exceptions during reinjection.

>
>> @@ -3721,8 +3738,7 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
>>          vmx_update_cpu_exec_control(v);
>>          if ( v->arch.hvm_vcpu.single_step )
>>          {
>> -            hvm_monitor_breakpoint(regs->eip,
>> -                                   HVM_MONITOR_SINGLESTEP_BREAKPOINT);
>> +            hvm_monitor_debug(regs->eip, HVM_MONITOR_SINGLESTEP_BREAKPOINT, 0, 0);
>
> How come the 3rd argument is literal zero here?

Instruction length is only meaningful for a subset of debug exceptions
that could be reinjected to the guest using xc_hvm_inject_trap.
Monitor trap flag events are external to the guest so there is nothing
to inject. The instruction length field won't even exist for this type
of singlestep events (we distinguish vm_event_singlestep and
vm_event_debug structs for this reason), so 0 here is arbitrary. We
could set it to ~0 to make it more obvious that it's just a
placeholder in this context.

> Also you're creating a long line here.

Ack.

Thanks,
Tamas

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

Re: [PATCH v4 8/8] x86/vm_event: Add HVM debug exception vm_events

[Andrew Cooper]

>>> @@ -3393,8 +3409,9 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
>>>              }
>>>              else {
>>>                  int handled =
>>> -                    hvm_monitor_breakpoint(regs->eip,
>>> -                                           HVM_MONITOR_SOFTWARE_BREAKPOINT);
>>> +                        hvm_monitor_debug(regs->eip,
>>> +                                          HVM_MONITOR_SOFTWARE_BREAKPOINT,
>>> +                                          X86_EVENTTYPE_SW_EXCEPTION, 1);
>> Please let's not add further mistakes like this, assuming INT3 can't
>> have any prefixes. It can, even if they're useless.
> You mean the instruction length is not necessarily 1? Ultimately it
> doesn't seem to matter because reinjecting it with xc_hvm_inject_trap
> ignores this field. Instruction length is only required to be properly
> set AFAICT for a subset of debug exceptions during reinjection.

Almost all x86 instructions can have redundant prefixes which change
their length without altering their functionality.

This specific area was the subject of XSA-106, and is astoundingly fragile.

Luckily, I have an available functional test which confirms correct
behaviour from the point of view of the guest.

http://xenbits.xen.org/people/andrewcoop/xen-test-framework/test-swint-emulation.html

Please confirm that this test returns success even when being monitored
with this new functionality.

~Andrew

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

Re: [PATCH v4 8/8] x86/vm_event: Add HVM debug exception vm_events

[Jan Beulich]

>>> On 30.05.16 at 22:13, <tamas@tklengyel.com> wrote:
> On Mon, May 30, 2016 at 8:16 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>> On 30.05.16 at 00:37, <tamas@tklengyel.com> wrote:
>>> @@ -3393,8 +3409,9 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
>>>              }
>>>              else {
>>>                  int handled =
>>> -                    hvm_monitor_breakpoint(regs->eip,
>>> -                                           HVM_MONITOR_SOFTWARE_BREAKPOINT);
>>> +                        hvm_monitor_debug(regs->eip,
>>> +                                          HVM_MONITOR_SOFTWARE_BREAKPOINT,
>>> +                                          X86_EVENTTYPE_SW_EXCEPTION, 1);
>>
>> Please let's not add further mistakes like this, assuming INT3 can't
>> have any prefixes. It can, even if they're useless.
> 
> You mean the instruction length is not necessarily 1? Ultimately it
> doesn't seem to matter because reinjecting it with xc_hvm_inject_trap
> ignores this field. Instruction length is only required to be properly
> set AFAICT for a subset of debug exceptions during reinjection.

As you suggest later in your reply, if the insn length really doesn't
matter, this should be made recognizable here. Either by a suitably
named manifest constant (which could then even evaluate to zero),
or by a comment (personally I'd prefer the former, but I'm not
maintainer of this code).

Jan


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

Re: [PATCH v4 8/8] x86/vm_event: Add HVM debug exception vm_events

[Tamas K Lengyel]

On Tue, May 31, 2016 at 1:59 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 30.05.16 at 22:13, <tamas@tklengyel.com> wrote:
>> On Mon, May 30, 2016 at 8:16 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>> On 30.05.16 at 00:37, <tamas@tklengyel.com> wrote:
>>>> @@ -3393,8 +3409,9 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
>>>>              }
>>>>              else {
>>>>                  int handled =
>>>> -                    hvm_monitor_breakpoint(regs->eip,
>>>> -                                           HVM_MONITOR_SOFTWARE_BREAKPOINT);
>>>> +                        hvm_monitor_debug(regs->eip,
>>>> +                                          HVM_MONITOR_SOFTWARE_BREAKPOINT,
>>>> +                                          X86_EVENTTYPE_SW_EXCEPTION, 1);
>>>
>>> Please let's not add further mistakes like this, assuming INT3 can't
>>> have any prefixes. It can, even if they're useless.
>>
>> You mean the instruction length is not necessarily 1? Ultimately it
>> doesn't seem to matter because reinjecting it with xc_hvm_inject_trap
>> ignores this field. Instruction length is only required to be properly
>> set AFAICT for a subset of debug exceptions during reinjection.
>
> As you suggest later in your reply, if the insn length really doesn't
> matter, this should be made recognizable here. Either by a suitably
> named manifest constant (which could then even evaluate to zero),
> or by a comment (personally I'd prefer the former, but I'm not
> maintainer of this code).
>
> Jan


Running Andrew's framework with xen-access monitoring breakpoints results in

xen-access:
Got event from Xen
Breakpoint: rip=00000000001032d1, gfn=103 (vcpu 0)

xl dmesg:
(d28) --- Xen Test Framework ---
(d28) Environment: HVM 64bit (Long mode 4 levels)
(d28) Trap emulation
(d28) Warning: FEP support not detected - some tests will be skipped
(d28) Test cpl0: all perms ok
(d28)   Testing int3
(XEN) d28v0 VMRESUME error: 0x7
(XEN) domain_crash_sync called from vmcs.c:1599
(XEN) Domain 28 (vcpu#0) crashed on cpu#7:
(XEN) ----[ Xen-4.6.1  x86_64  debug=n  Not tainted ]----
(XEN) CPU:    7
(XEN) RIP:    0008:[<00000000001032d1>]
(XEN) RFLAGS: 0000000000000046   CONTEXT: hvm guest (d28v0)
(XEN) rax: 00000000001032d2   rbx: 00000000001102b0   rcx: 0000000000000000
(XEN) rdx: 0000000000104af0   rsi: 0000000000000000   rdi: 0000000000000000
(XEN) rbp: 0000000000000001   rsp: 0000000000114f98   r8:  000000000000000f
(XEN) r9:  00000000000000ad   r10: 000000000000000f   r11: 0000000000000004
(XEN) r12: 0000000000000003   r13: 0000000000000000   r14: 0000000000000000
(XEN) r15: 0000000000000000   cr0: 0000000080000011   cr4: 0000000000000020
(XEN) cr3: 000000000010b000   cr2: 0000000000000000
(XEN) ds: 0033   es: 0033   fs: 0033   gs: 0033   ss: 0000   cs: 0008

This is likely because xen-access sets the instruction length to 0
during reinjection. If I change that to 1 the tests still fail but
without crashing the domain, output:

xen-access:
Got event from Xen
Breakpoint: rip=00000000001032d1, gfn=103 (vcpu 0)
Got event from Xen
Breakpoint: rip=00000000001032e1, gfn=103 (vcpu 0)
Got event from Xen
Breakpoint: rip=00000000001032e2, gfn=103 (vcpu 0)
Got event from Xen
Breakpoint: rip=00000000001032d1, gfn=103 (vcpu 0)
Got event from Xen
Breakpoint: rip=00000000001032e1, gfn=103 (vcpu 0)
Got event from Xen
Breakpoint: rip=00000000001032d1, gfn=103 (vcpu 0)
Got event from Xen
Breakpoint: rip=00000000001032e1, gfn=103 (vcpu 0)
Got event from Xen
Breakpoint: rip=00000000001032e2, gfn=103 (vcpu 0)
Got event from Xen
Breakpoint: rip=00000000001032d1, gfn=103 (vcpu 0)
Got event from Xen
Breakpoint: rip=00000000001032e1, gfn=103 (vcpu 0)
Got event from Xen
Breakpoint: rip=00000000001032d1, gfn=103 (vcpu 0)
Got event from Xen
Breakpoint: rip=00000000001032e1, gfn=103 (vcpu 0)

xl dmesg:
(d30) Environment: HVM 64bit (Long mode 4 levels)
(d30) Trap emulation
(d30) Warning: FEP support not detected - some tests will be skipped
(d30) Test cpl0: all perms ok
(d30)   Testing int3
(d30) Fail redundant: Expected 1 exception (vec 3 at 00000000001032e3), got 2
(d30)  exlog[00] 0008:00000000001032e2 vec 3[0000]
(d30)  exlog[01] 0008:00000000001032e3 vec 3[0000]
(d30)   Testing int $3
(d30)   Testing icebp
(d30)   Testing int $1
(d30)   Testing into
(d30) Test cpl0: p=0
(d30)   Testing int3
(d30)   Testing int $3
(d30)   Testing icebp
(d30)   Testing int $1
(d30)   Testing into
(d30) Test cpl3: all perms ok
(d30)   Testing int3
(d30) Fail redundant: Expected 1 exception (vec 3 at 00000000001032e3), got 2
(d30)  exlog[00] 0023:00000000001032e2 vec 3[0000]
(d30)  exlog[01] 0023:00000000001032e3 vec 3[0000]
(d30)   Testing int $3
(d30)   Testing icebp
(d30)   Testing int $1
(d30)   Testing into
(d30) Test cpl3: p=0
(d30)   Testing int3
(d30)   Testing int $3
(d30)   Testing icebp
(d30)   Testing int $1
(d30)   Testing into
(d30) Test cpl3: dpl=0
(d30)   Testing int3
(d30)   Testing int $3
(d30)   Testing icebp
(d30)   Testing int $1
(d30)   Testing into
(d30) Test result: FAILURE

So we _should be_ sending the instruction length information along for
this type of vm_events and it is in fact buggy right now.

Tamas

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

Re: [PATCH v4 8/8] x86/vm_event: Add HVM debug exception vm_events

[Andrew Cooper]

On 01/06/2016 22:46, Tamas K Lengyel wrote:
> On Tue, May 31, 2016 at 1:59 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>> On 30.05.16 at 22:13, <tamas@tklengyel.com> wrote:
>>> On Mon, May 30, 2016 at 8:16 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>>> On 30.05.16 at 00:37, <tamas@tklengyel.com> wrote:
>>>>> @@ -3393,8 +3409,9 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
>>>>>              }
>>>>>              else {
>>>>>                  int handled =
>>>>> -                    hvm_monitor_breakpoint(regs->eip,
>>>>> -                                           HVM_MONITOR_SOFTWARE_BREAKPOINT);
>>>>> +                        hvm_monitor_debug(regs->eip,
>>>>> +                                          HVM_MONITOR_SOFTWARE_BREAKPOINT,
>>>>> +                                          X86_EVENTTYPE_SW_EXCEPTION, 1);
>>>> Please let's not add further mistakes like this, assuming INT3 can't
>>>> have any prefixes. It can, even if they're useless.
>>> You mean the instruction length is not necessarily 1? Ultimately it
>>> doesn't seem to matter because reinjecting it with xc_hvm_inject_trap
>>> ignores this field. Instruction length is only required to be properly
>>> set AFAICT for a subset of debug exceptions during reinjection.
>> As you suggest later in your reply, if the insn length really doesn't
>> matter, this should be made recognizable here. Either by a suitably
>> named manifest constant (which could then even evaluate to zero),
>> or by a comment (personally I'd prefer the former, but I'm not
>> maintainer of this code).
>>
>> Jan
>
> Running Andrew's framework with xen-access monitoring breakpoints results in
>
> xen-access:
> Got event from Xen
> Breakpoint: rip=00000000001032d1, gfn=103 (vcpu 0)
>
> xl dmesg:
> (d28) --- Xen Test Framework ---
> (d28) Environment: HVM 64bit (Long mode 4 levels)
> (d28) Trap emulation
> (d28) Warning: FEP support not detected - some tests will be skipped
> (d28) Test cpl0: all perms ok
> (d28)   Testing int3
> (XEN) d28v0 VMRESUME error: 0x7
> (XEN) domain_crash_sync called from vmcs.c:1599
> (XEN) Domain 28 (vcpu#0) crashed on cpu#7:
> (XEN) ----[ Xen-4.6.1  x86_64  debug=n  Not tainted ]----
> (XEN) CPU:    7
> (XEN) RIP:    0008:[<00000000001032d1>]
> (XEN) RFLAGS: 0000000000000046   CONTEXT: hvm guest (d28v0)
> (XEN) rax: 00000000001032d2   rbx: 00000000001102b0   rcx: 0000000000000000
> (XEN) rdx: 0000000000104af0   rsi: 0000000000000000   rdi: 0000000000000000
> (XEN) rbp: 0000000000000001   rsp: 0000000000114f98   r8:  000000000000000f
> (XEN) r9:  00000000000000ad   r10: 000000000000000f   r11: 0000000000000004
> (XEN) r12: 0000000000000003   r13: 0000000000000000   r14: 0000000000000000
> (XEN) r15: 0000000000000000   cr0: 0000000080000011   cr4: 0000000000000020
> (XEN) cr3: 000000000010b000   cr2: 0000000000000000
> (XEN) ds: 0033   es: 0033   fs: 0033   gs: 0033   ss: 0000   cs: 0008
>
> This is likely because xen-access sets the instruction length to 0
> during reinjection. If I change that to 1 the tests still fail but
> without crashing the domain, output:
>
> xen-access:
> Got event from Xen
> Breakpoint: rip=00000000001032d1, gfn=103 (vcpu 0)
> Got event from Xen
> Breakpoint: rip=00000000001032e1, gfn=103 (vcpu 0)
> Got event from Xen
> Breakpoint: rip=00000000001032e2, gfn=103 (vcpu 0)
> Got event from Xen
> Breakpoint: rip=00000000001032d1, gfn=103 (vcpu 0)
> Got event from Xen
> Breakpoint: rip=00000000001032e1, gfn=103 (vcpu 0)
> Got event from Xen
> Breakpoint: rip=00000000001032d1, gfn=103 (vcpu 0)
> Got event from Xen
> Breakpoint: rip=00000000001032e1, gfn=103 (vcpu 0)
> Got event from Xen
> Breakpoint: rip=00000000001032e2, gfn=103 (vcpu 0)
> Got event from Xen
> Breakpoint: rip=00000000001032d1, gfn=103 (vcpu 0)
> Got event from Xen
> Breakpoint: rip=00000000001032e1, gfn=103 (vcpu 0)
> Got event from Xen
> Breakpoint: rip=00000000001032d1, gfn=103 (vcpu 0)
> Got event from Xen
> Breakpoint: rip=00000000001032e1, gfn=103 (vcpu 0)
>
> xl dmesg:
> (d30) Environment: HVM 64bit (Long mode 4 levels)
> (d30) Trap emulation
> (d30) Warning: FEP support not detected - some tests will be skipped
> (d30) Test cpl0: all perms ok
> (d30)   Testing int3
> (d30) Fail redundant: Expected 1 exception (vec 3 at 00000000001032e3), got 2
> (d30)  exlog[00] 0008:00000000001032e2 vec 3[0000]
> (d30)  exlog[01] 0008:00000000001032e3 vec 3[0000]
> (d30)   Testing int $3
> (d30)   Testing icebp
> (d30)   Testing int $1
> (d30)   Testing into
> (d30) Test cpl0: p=0
> (d30)   Testing int3
> (d30)   Testing int $3
> (d30)   Testing icebp
> (d30)   Testing int $1
> (d30)   Testing into
> (d30) Test cpl3: all perms ok
> (d30)   Testing int3
> (d30) Fail redundant: Expected 1 exception (vec 3 at 00000000001032e3), got 2
> (d30)  exlog[00] 0023:00000000001032e2 vec 3[0000]
> (d30)  exlog[01] 0023:00000000001032e3 vec 3[0000]
> (d30)   Testing int $3
> (d30)   Testing icebp
> (d30)   Testing int $1
> (d30)   Testing into
> (d30) Test cpl3: p=0
> (d30)   Testing int3
> (d30)   Testing int $3
> (d30)   Testing icebp
> (d30)   Testing int $1
> (d30)   Testing into
> (d30) Test cpl3: dpl=0
> (d30)   Testing int3
> (d30)   Testing int $3
> (d30)   Testing icebp
> (d30)   Testing int $1
> (d30)   Testing into
> (d30) Test result: FAILURE
>
> So we _should be_ sending the instruction length information along for
> this type of vm_events and it is in fact buggy right now.

On a related note, do emulated instruction get appropriately sent for
introspection?

You can check this by booting a debug hypervisor with "hvm_fep" on the
command line, which will double the number of tests run by this suite.

If they are sent for emulation, you would expect to see some "Fail force
redundant:" issues as well.  I can't think of any codepath offhand which
links the emulation results up to the current introspection paths.

~Andrew

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

Re: [PATCH v4 8/8] x86/vm_event: Add HVM debug exception vm_events

[Tamas K Lengyel]

On Wed, Jun 1, 2016 at 4:17 PM, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> On 01/06/2016 22:46, Tamas K Lengyel wrote:
>> On Tue, May 31, 2016 at 1:59 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>> On 30.05.16 at 22:13, <tamas@tklengyel.com> wrote:
>>>> On Mon, May 30, 2016 at 8:16 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>>>> On 30.05.16 at 00:37, <tamas@tklengyel.com> wrote:
>>>>>> @@ -3393,8 +3409,9 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
>>>>>>              }
>>>>>>              else {
>>>>>>                  int handled =
>>>>>> -                    hvm_monitor_breakpoint(regs->eip,
>>>>>> -                                           HVM_MONITOR_SOFTWARE_BREAKPOINT);
>>>>>> +                        hvm_monitor_debug(regs->eip,
>>>>>> +                                          HVM_MONITOR_SOFTWARE_BREAKPOINT,
>>>>>> +                                          X86_EVENTTYPE_SW_EXCEPTION, 1);
>>>>> Please let's not add further mistakes like this, assuming INT3 can't
>>>>> have any prefixes. It can, even if they're useless.
>>>> You mean the instruction length is not necessarily 1? Ultimately it
>>>> doesn't seem to matter because reinjecting it with xc_hvm_inject_trap
>>>> ignores this field. Instruction length is only required to be properly
>>>> set AFAICT for a subset of debug exceptions during reinjection.
>>> As you suggest later in your reply, if the insn length really doesn't
>>> matter, this should be made recognizable here. Either by a suitably
>>> named manifest constant (which could then even evaluate to zero),
>>> or by a comment (personally I'd prefer the former, but I'm not
>>> maintainer of this code).
>>>
>>> Jan
>>
>> Running Andrew's framework with xen-access monitoring breakpoints results in
>>
>> xen-access:
>> Got event from Xen
>> Breakpoint: rip=00000000001032d1, gfn=103 (vcpu 0)
>>
>> xl dmesg:
>> (d28) --- Xen Test Framework ---
>> (d28) Environment: HVM 64bit (Long mode 4 levels)
>> (d28) Trap emulation
>> (d28) Warning: FEP support not detected - some tests will be skipped
>> (d28) Test cpl0: all perms ok
>> (d28)   Testing int3
>> (XEN) d28v0 VMRESUME error: 0x7
>> (XEN) domain_crash_sync called from vmcs.c:1599
>> (XEN) Domain 28 (vcpu#0) crashed on cpu#7:
>> (XEN) ----[ Xen-4.6.1  x86_64  debug=n  Not tainted ]----
>> (XEN) CPU:    7
>> (XEN) RIP:    0008:[<00000000001032d1>]
>> (XEN) RFLAGS: 0000000000000046   CONTEXT: hvm guest (d28v0)
>> (XEN) rax: 00000000001032d2   rbx: 00000000001102b0   rcx: 0000000000000000
>> (XEN) rdx: 0000000000104af0   rsi: 0000000000000000   rdi: 0000000000000000
>> (XEN) rbp: 0000000000000001   rsp: 0000000000114f98   r8:  000000000000000f
>> (XEN) r9:  00000000000000ad   r10: 000000000000000f   r11: 0000000000000004
>> (XEN) r12: 0000000000000003   r13: 0000000000000000   r14: 0000000000000000
>> (XEN) r15: 0000000000000000   cr0: 0000000080000011   cr4: 0000000000000020
>> (XEN) cr3: 000000000010b000   cr2: 0000000000000000
>> (XEN) ds: 0033   es: 0033   fs: 0033   gs: 0033   ss: 0000   cs: 0008
>>
>> This is likely because xen-access sets the instruction length to 0
>> during reinjection. If I change that to 1 the tests still fail but
>> without crashing the domain, output:
>>
>> xen-access:
>> Got event from Xen
>> Breakpoint: rip=00000000001032d1, gfn=103 (vcpu 0)
>> Got event from Xen
>> Breakpoint: rip=00000000001032e1, gfn=103 (vcpu 0)
>> Got event from Xen
>> Breakpoint: rip=00000000001032e2, gfn=103 (vcpu 0)
>> Got event from Xen
>> Breakpoint: rip=00000000001032d1, gfn=103 (vcpu 0)
>> Got event from Xen
>> Breakpoint: rip=00000000001032e1, gfn=103 (vcpu 0)
>> Got event from Xen
>> Breakpoint: rip=00000000001032d1, gfn=103 (vcpu 0)
>> Got event from Xen
>> Breakpoint: rip=00000000001032e1, gfn=103 (vcpu 0)
>> Got event from Xen
>> Breakpoint: rip=00000000001032e2, gfn=103 (vcpu 0)
>> Got event from Xen
>> Breakpoint: rip=00000000001032d1, gfn=103 (vcpu 0)
>> Got event from Xen
>> Breakpoint: rip=00000000001032e1, gfn=103 (vcpu 0)
>> Got event from Xen
>> Breakpoint: rip=00000000001032d1, gfn=103 (vcpu 0)
>> Got event from Xen
>> Breakpoint: rip=00000000001032e1, gfn=103 (vcpu 0)
>>
>> xl dmesg:
>> (d30) Environment: HVM 64bit (Long mode 4 levels)
>> (d30) Trap emulation
>> (d30) Warning: FEP support not detected - some tests will be skipped
>> (d30) Test cpl0: all perms ok
>> (d30)   Testing int3
>> (d30) Fail redundant: Expected 1 exception (vec 3 at 00000000001032e3), got 2
>> (d30)  exlog[00] 0008:00000000001032e2 vec 3[0000]
>> (d30)  exlog[01] 0008:00000000001032e3 vec 3[0000]
>> (d30)   Testing int $3
>> (d30)   Testing icebp
>> (d30)   Testing int $1
>> (d30)   Testing into
>> (d30) Test cpl0: p=0
>> (d30)   Testing int3
>> (d30)   Testing int $3
>> (d30)   Testing icebp
>> (d30)   Testing int $1
>> (d30)   Testing into
>> (d30) Test cpl3: all perms ok
>> (d30)   Testing int3
>> (d30) Fail redundant: Expected 1 exception (vec 3 at 00000000001032e3), got 2
>> (d30)  exlog[00] 0023:00000000001032e2 vec 3[0000]
>> (d30)  exlog[01] 0023:00000000001032e3 vec 3[0000]
>> (d30)   Testing int $3
>> (d30)   Testing icebp
>> (d30)   Testing int $1
>> (d30)   Testing into
>> (d30) Test cpl3: p=0
>> (d30)   Testing int3
>> (d30)   Testing int $3
>> (d30)   Testing icebp
>> (d30)   Testing int $1
>> (d30)   Testing into
>> (d30) Test cpl3: dpl=0
>> (d30)   Testing int3
>> (d30)   Testing int $3
>> (d30)   Testing icebp
>> (d30)   Testing int $1
>> (d30)   Testing into
>> (d30) Test result: FAILURE
>>
>> So we _should be_ sending the instruction length information along for
>> this type of vm_events and it is in fact buggy right now.
>
> On a related note, do emulated instruction get appropriately sent for
> introspection?

I'm fairly certain emulated int3 is not forwarded through vm_event as
the only hook we have right now for int3 comes from vmx.c.

>
> You can check this by booting a debug hypervisor with "hvm_fep" on the
> command line, which will double the number of tests run by this suite.
>
> If they are sent for emulation, you would expect to see some "Fail force
> redundant:" issues as well.  I can't think of any codepath offhand which
> links the emulation results up to the current introspection paths.

I'll try it out, thanks!

Tamas

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