[7/7] arch.monitor: move bits to common (arch_domain to domain)

Commit Message

Corneliu ZUZU Feb. 8, 2016, 4:58 p.m. UTC

This patch moves bitfield members for single-step, software-breakpoint and
guest-request monitor vm-events from the arch-side (struct arch_domain) to
the common-side (struct domain). Ctrl-reg bits (i.e. write_ctrlreg_* members)
are left on the arch-side, because control-registers number can vary across
architectures.

Signed-off-by: Corneliu ZUZU <[e-mail]>
---
 xen/arch/x86/hvm/vmx/vmcs.c  |  4 ++--
 xen/common/hvm/event.c       | 12 ++++++------
 xen/common/monitor.c         | 17 +++++++----------
 xen/include/asm-x86/domain.h | 16 ++++++----------
 xen/include/xen/sched.h      | 16 ++++++++++++++++
 5 files changed, 37 insertions(+), 28 deletions(-)

Comments

Tamas K Lengyel Feb. 8, 2016, 6:29 p.m. UTC | #1

On Mon, Feb 8, 2016 at 9:58 AM, Corneliu ZUZU <[e-mail]> wrote:

> This patch moves bitfield members for single-step, software-breakpoint and
> guest-request monitor vm-events from the arch-side (struct arch_domain) to
> the common-side (struct domain). Ctrl-reg bits (i.e. write_ctrlreg_*
> members)
> are left on the arch-side, because control-registers number can vary across
> architectures.
>
> Signed-off-by: Corneliu ZUZU <[e-mail]>
>

Technically this looks fine, but I do wonder if and what plans you have to
actually implement these events for ARM. I haven't spent too much time
looking into it, but I'm not aware of equivalent features on ARM to Intel
MTF (singlestepping) or to software-breakpoint trapping. The only
instruction I know that functionally comes close to software-breakpoint
trapping (INT3) is the SMC instruction which can be trapped into the VMM,
but I would not call that a "breakpoint" in the traditional sense.

Tamas

Corneliu ZUZU Feb. 9, 2016, 7:14 a.m. UTC | #2

On 2/8/2016 8:29 PM, Tamas K Lengyel wrote:
>
>
> On Mon, Feb 8, 2016 at 9:58 AM, Corneliu ZUZU <[e-mail] 
> <mailto:[e-mail]>> wrote:
>
>     This patch moves bitfield members for single-step,
>     software-breakpoint and
>     guest-request monitor vm-events from the arch-side (struct
>     arch_domain) to
>     the common-side (struct domain). Ctrl-reg bits (i.e.
>     write_ctrlreg_* members)
>     are left on the arch-side, because control-registers number can
>     vary across
>     architectures.
>
>     Signed-off-by: Corneliu ZUZU <[e-mail]
>     <mailto:[e-mail]>>
>
>
> Technically this looks fine, but I do wonder if and what plans you 
> have to actually implement these events for ARM.

Currently I've only planned implementations for control-register write 
events & guest-requests.
The other two also seem feasible though, I might give adding those a 
shot sometime after sending the other patches.

> I haven't spent too much time looking into it, but I'm not aware of 
> equivalent features on ARM to Intel MTF (singlestepping) or to 
> software-breakpoint trapping. The only instruction I know that 
> functionally comes close to software-breakpoint trapping (INT3) is the 
> SMC instruction which can be trapped into the VMM, but I would not 
> call that a "breakpoint" in the traditional sense.
>
> Tamas
>

There's the debugging architecture, hypervisor control of that is 
possible on both 32-bit & 64-bit ARM.
It isn't as easy as for X86 though, where MTF is a hypervisor-internal 
feature and INT3 can be
trapped specifically, whereas on ARM granularity of trap-setting is less 
of a concern apparently.
For this reason, the only issue I see here is the performance penalty 
these traps would cause for
arbitrary software breakpoints (for obvious reasons that doesn't matter 
in the case of single-stepping).

For INT3, the ARM equivalent is be the BKPT/BRK (set HDCR.TDE on 
AArch32/MDCR_EL2.TDE AArch64) instruction.
Trapping on this instruction implies trapping on
- AArch32: some other debug exceptions (looking @ B1.8.9, ARMv7 DDI 0406C.b)
- AArch64: *all software debug exceptions* + *all debug register 
accesses* (this might cause some headaches)
For MTF-like functionality, the debug architecture also provides ways 
for single-stepping.
That would similarly generate software breakpoint exceptions which can 
be routed to the hypervisor.

Corneliu.

Patch

diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
index 5bc3c74..07acbf2 100644
--- a/xen/arch/x86/hvm/vmx/vmcs.c
+++ b/xen/arch/x86/hvm/vmx/vmcs.c
@@ -1706,8 +1706,8 @@  void vmx_do_resume(struct vcpu *v)
     }
 
     debug_state = v->domain->debugger_attached
-                  || v->domain->arch.monitor.software_breakpoint_enabled
-                  || v->domain->arch.monitor.singlestep_enabled;
+                  || v->domain->monitor.software_breakpoint_enabled
+                  || v->domain->monitor.singlestep_enabled;
 
     if ( unlikely(v->arch.hvm_vcpu.debug_state_latch != debug_state) )
     {
diff --git a/xen/common/hvm/event.c b/xen/common/hvm/event.c
index d4ce97a..2660093 100644
--- a/xen/common/hvm/event.c
+++ b/xen/common/hvm/event.c
@@ -110,16 +110,16 @@  bool_t hvm_event_cr(unsigned int index,
 void hvm_event_guest_request(void)
 {
     struct vcpu *curr = current;
-    struct arch_domain *ad = &curr->domain->arch;
+    struct domain *d = curr->domain;
 
-    if ( ad->monitor.guest_request_enabled )
+    if ( d->monitor.guest_request_enabled )
     {
         vm_event_request_t req = {
             .reason = VM_EVENT_REASON_GUEST_REQUEST,
             .vcpu_id = curr->vcpu_id,
         };
 
-        hvm_event_traps(curr, ad->monitor.guest_request_sync, &req);
+        hvm_event_traps(curr, d->monitor.guest_request_sync, &req);
     }
 }
 
@@ -131,9 +131,9 @@  int hvm_event_software_breakpoint(unsigned long ip, bool_t single_step)
 {
     int rc = 0;
     struct vcpu *curr = current;
-    struct arch_domain *ad = &curr->domain->arch;
-    bool_t enabled = ( single_step ? ad->monitor.singlestep_enabled
-                                   : ad->monitor.software_breakpoint_enabled );
+    struct domain *d = curr->domain;
+    bool_t enabled = ( single_step ? d->monitor.singlestep_enabled
+                                   : d->monitor.software_breakpoint_enabled );
 
     if ( enabled )
     {
diff --git a/xen/common/monitor.c b/xen/common/monitor.c
index 1165aeb..b745aa4 100644
--- a/xen/common/monitor.c
+++ b/xen/common/monitor.c
@@ -121,14 +121,13 @@  int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *mop)
 
     case XEN_DOMCTL_MONITOR_EVENT_SINGLESTEP:
     {
-        struct arch_domain *ad = &d->arch;
-        bool_t old_status = ad->monitor.singlestep_enabled;
+        bool_t old_status = d->monitor.singlestep_enabled;
 
         if ( unlikely(old_status == requested_status) )
             return -EEXIST;
 
         domain_pause(d);
-        ad->monitor.singlestep_enabled = !old_status;
+        d->monitor.singlestep_enabled = !old_status;
         domain_unpause(d);
         break;
     }
@@ -139,14 +138,13 @@  int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *mop)
 
     case XEN_DOMCTL_MONITOR_EVENT_SOFTWARE_BREAKPOINT:
     {
-        struct arch_domain *ad = &d->arch;
-        bool_t old_status = ad->monitor.software_breakpoint_enabled;
+        bool_t old_status = d->monitor.software_breakpoint_enabled;
 
         if ( unlikely(old_status == requested_status) )
             return -EEXIST;
 
         domain_pause(d);
-        ad->monitor.software_breakpoint_enabled = !old_status;
+        d->monitor.software_breakpoint_enabled = !old_status;
         domain_unpause(d);
         break;
     }
@@ -157,15 +155,14 @@  int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *mop)
 
     case XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST:
     {
-        struct arch_domain *ad = &d->arch;
-        bool_t old_status = ad->monitor.guest_request_enabled;
+        bool_t old_status = d->monitor.guest_request_enabled;
 
         if ( unlikely(old_status == requested_status) )
             return -EEXIST;
 
         domain_pause(d);
-        ad->monitor.guest_request_sync = mop->u.guest_request.sync;
-        ad->monitor.guest_request_enabled = !old_status;
+        d->monitor.guest_request_sync = mop->u.guest_request.sync;
+        d->monitor.guest_request_enabled = !old_status;
         domain_unpause(d);
         break;
     }
diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
index 4072e27..6254060 100644
--- a/xen/include/asm-x86/domain.h
+++ b/xen/include/asm-x86/domain.h
@@ -376,17 +376,13 @@  struct arch_domain
     unsigned long *pirq_eoi_map;
     unsigned long pirq_eoi_map_mfn;
 
-    /* Monitor options */
+    /* Arch-specific monitor vm-event options */
     struct {
-        unsigned int write_ctrlreg_enabled       : 4;
-        unsigned int write_ctrlreg_sync          : 4;
-        unsigned int write_ctrlreg_onchangeonly  : 4;
-        unsigned int mov_to_msr_enabled          : 1;
-        unsigned int mov_to_msr_extended         : 1;
-        unsigned int singlestep_enabled          : 1;
-        unsigned int software_breakpoint_enabled : 1;
-        unsigned int guest_request_enabled       : 1;
-        unsigned int guest_request_sync          : 1;
+        uint16_t write_ctrlreg_enabled       : 4;
+        uint16_t write_ctrlreg_sync          : 4;
+        uint16_t write_ctrlreg_onchangeonly  : 4;
+        uint16_t mov_to_msr_enabled          : 1;
+        uint16_t mov_to_msr_extended         : 1;
     } monitor;
 
     /* Mem_access emulation control */
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index b47a3fe..5b01a7f 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -464,6 +464,22 @@  struct domain
     /* vNUMA topology accesses are protected by rwlock. */
     rwlock_t vnuma_rwlock;
     struct vnuma_info *vnuma;
+
+    /* Common monitor vm-events options */
+    struct {
+#if CONFIG_HAS_VM_EVENT_SINGLESTEP
+        uint8_t singlestep_enabled          : 1;
+#endif // HAS_VM_EVENT_SINGLESTEP
+
+#if CONFIG_HAS_VM_EVENT_SOFTWARE_BREAKPOINT
+        uint8_t software_breakpoint_enabled : 1;
+#endif // HAS_VM_EVENT_SOFTWARE_BREAKPOINT
+
+#if CONFIG_HAS_VM_EVENT_GUEST_REQUEST
+        uint8_t guest_request_enabled       : 1;
+        uint8_t guest_request_sync          : 1;
+#endif // HAS_VM_EVENT_GUEST_REQUEST
+    } monitor;
 };
 
 /* Protect updates/reads (resp.) of domain_list and domain_hash. */