[PATCH] x86/vm_event: toggle singlestep from vm_event response

[PATCH] x86/vm_event: toggle singlestep from vm_event response

[Tamas K Lengyel]

Add an option to the vm_event response to toggle singlestepping on the vCPU.

Singed-off-by: Tamas K Lengyel <tlengyel@novetta.com>
---
 MAINTAINERS                    |  1 +
 xen/arch/x86/Makefile          |  1 +
 xen/arch/x86/hvm/hvm.c         |  8 ++++++++
 xen/arch/x86/vm_event.c        | 41 +++++++++++++++++++++++++++++++++++++++++
 xen/common/vm_event.c          |  8 +++++++-
 xen/include/asm-arm/vm_event.h | 29 +++++++++++++++++++++++++++++
 xen/include/asm-x86/hvm/hvm.h  |  3 +++
 xen/include/asm-x86/vm_event.h | 25 +++++++++++++++++++++++++
 xen/include/public/vm_event.h  | 11 ++++++++---
 9 files changed, 123 insertions(+), 4 deletions(-)
 create mode 100644 xen/arch/x86/vm_event.c
 create mode 100644 xen/include/asm-arm/vm_event.h
 create mode 100644 xen/include/asm-x86/vm_event.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 6b1068e..59c0822 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -383,6 +383,7 @@ F:	xen/common/vm_event.c
 F:	xen/common/mem_access.c
 F:	xen/arch/x86/hvm/event.c
 F:	xen/arch/x86/monitor.c
+F:	xen/arch/x86/vm_event.c
 
 XENTRACE
 M:	George Dunlap <george.dunlap@eu.citrix.com>
diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
index 37e547c..5f24951 100644
--- a/xen/arch/x86/Makefile
+++ b/xen/arch/x86/Makefile
@@ -60,6 +60,7 @@ obj-y += machine_kexec.o
 obj-y += crash.o
 obj-y += tboot.o
 obj-y += hpet.o
+obj-y += vm_event.o
 obj-y += xstate.o
 
 obj-$(crash_debug) += gdbstub.o
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 535d622..2bfd1b0 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -6431,6 +6431,14 @@ int hvm_debug_op(struct vcpu *v, int32_t op)
     return rc;
 }
 
+void hvm_toggle_singlestep(struct vcpu *v)
+{
+    if ( !cpu_has_monitor_trap_flag )
+        return;
+
+    v->arch.hvm_vcpu.single_step = !v->arch.hvm_vcpu.single_step;
+}
+
 int nhvm_vcpu_hostrestore(struct vcpu *v, struct cpu_user_regs *regs)
 {
     if (hvm_funcs.nhvm_vcpu_hostrestore)
diff --git a/xen/arch/x86/vm_event.c b/xen/arch/x86/vm_event.c
new file mode 100644
index 0000000..95b30ad
--- /dev/null
+++ b/xen/arch/x86/vm_event.c
@@ -0,0 +1,41 @@
+/*
+ * arch/x86/vm_event.c
+ *
+ * Architecture-specific vm_event handling routines
+ *
+ * Copyright (c) 2015 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, write to the
+ * Free Software Foundation, Inc., 59 Temple Place - Suite 330,
+ * Boston, MA 021110-1307, USA.
+ */
+
+#include <xen/sched.h>
+#include <asm/hvm/hvm.h>
+
+void vm_event_toggle_singlestep(struct domain *d, struct vcpu *v)
+{
+    if ( (v == current) || !is_hvm_domain(d) )
+        return;
+
+    hvm_toggle_singlestep(v);
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/common/vm_event.c b/xen/common/vm_event.c
index 120a78a..2ee27e2 100644
--- a/xen/common/vm_event.c
+++ b/xen/common/vm_event.c
@@ -27,6 +27,7 @@
 #include <xen/vm_event.h>
 #include <xen/mem_access.h>
 #include <asm/p2m.h>
+#include <asm/vm_event.h>
 #include <xsm/xsm.h>
 
 /* for public/io/ring.h macros */
@@ -399,9 +400,14 @@ void vm_event_resume(struct domain *d, struct vm_event_domain *ved)
 
         };
 
-        /* Unpause domain. */
         if ( rsp.flags & VM_EVENT_FLAG_VCPU_PAUSED )
+        {
+            if ( rsp.flags & VM_EVENT_FLAG_TOGGLE_SINGLESTEP )
+                vm_event_toggle_singlestep(d, v);
+
+            /* Unpause domain. */
             vm_event_vcpu_unpause(v);
+        }
     }
 }
 
diff --git a/xen/include/asm-arm/vm_event.h b/xen/include/asm-arm/vm_event.h
new file mode 100644
index 0000000..a4cf4c6
--- /dev/null
+++ b/xen/include/asm-arm/vm_event.h
@@ -0,0 +1,29 @@
+/*
+ * vm_event.h: architecture specific vm_event handling routines
+ *
+ * Copyright (c) 2015 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,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope 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, write to the Free Software Foundation, Inc., 59 Temple
+ * Place - Suite 330, Boston, MA 02111-1307 USA.
+ */
+
+#ifndef __ASM_ARM_VM_EVENT_H__
+#define __ASM_ARM_VM_EVENT_H__
+
+static inline
+void vm_event_toggle_singlestep(struct domain *d, struct vcpu *v)
+{
+    /* Not supported on ARM. */
+}
+
+#endif /* __ASM_ARM_VM_EVENT_H__ */
diff --git a/xen/include/asm-x86/hvm/hvm.h b/xen/include/asm-x86/hvm/hvm.h
index 57f9605..073b758 100644
--- a/xen/include/asm-x86/hvm/hvm.h
+++ b/xen/include/asm-x86/hvm/hvm.h
@@ -448,6 +448,9 @@ static inline void hvm_set_info_guest(struct vcpu *v)
 
 int hvm_debug_op(struct vcpu *v, int32_t op);
 
+/* Caller should pause vcpu before calling this function */
+void hvm_toggle_singlestep(struct vcpu *v);
+
 static inline void hvm_invalidate_regs_fields(struct cpu_user_regs *regs)
 {
 #ifndef NDEBUG
diff --git a/xen/include/asm-x86/vm_event.h b/xen/include/asm-x86/vm_event.h
new file mode 100644
index 0000000..d36dd50
--- /dev/null
+++ b/xen/include/asm-x86/vm_event.h
@@ -0,0 +1,25 @@
+/*
+ * vm_event.h: architecture specific vm_event handling routines
+ *
+ * Copyright (c) 2015 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,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope 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, write to the Free Software Foundation, Inc., 59 Temple
+ * Place - Suite 330, Boston, MA 02111-1307 USA.
+ */
+
+#ifndef __ASM_X86_VM_EVENT_H__
+#define __ASM_X86_VM_EVENT_H__
+
+void vm_event_toggle_singlestep(struct domain *d, struct vcpu *v);
+
+#endif /* __ASM_X86_VM_EVENT_H__ */
diff --git a/xen/include/public/vm_event.h b/xen/include/public/vm_event.h
index 577e971..6156d9e 100644
--- a/xen/include/public/vm_event.h
+++ b/xen/include/public/vm_event.h
@@ -44,9 +44,14 @@
  *  paused
  * VCPU_PAUSED in a response signals to unpause the vCPU
  */
-#define VM_EVENT_FLAG_VCPU_PAUSED     (1 << 0)
-/* Flags to aid debugging mem_event */
-#define VM_EVENT_FLAG_FOREIGN         (1 << 1)
+#define VM_EVENT_FLAG_VCPU_PAUSED       (1 << 0)
+/* Flag to aid debugging mem_event */
+#define VM_EVENT_FLAG_FOREIGN           (1 << 1)
+/*
+ * Toggle singlestepping on vm_event response.
+ * Requires the vCPU to be paused already (synchronous events only).
+ */
+#define VM_EVENT_FLAG_TOGGLE_SINGLESTEP (1 << 2)
 
 /*
  * Reasons for the vm event request
-- 
2.1.4

Re: [PATCH] x86/vm_event: toggle singlestep from vm_event response

[Razvan Cojocaru]

On 06/29/2015 03:58 PM, Tamas K Lengyel wrote:
> Add an option to the vm_event response to toggle singlestepping on the vCPU.
> 
> Singed-off-by: Tamas K Lengyel <tlengyel@novetta.com>
> ---
>  MAINTAINERS                    |  1 +
>  xen/arch/x86/Makefile          |  1 +
>  xen/arch/x86/hvm/hvm.c         |  8 ++++++++
>  xen/arch/x86/vm_event.c        | 41 +++++++++++++++++++++++++++++++++++++++++
>  xen/common/vm_event.c          |  8 +++++++-
>  xen/include/asm-arm/vm_event.h | 29 +++++++++++++++++++++++++++++
>  xen/include/asm-x86/hvm/hvm.h  |  3 +++
>  xen/include/asm-x86/vm_event.h | 25 +++++++++++++++++++++++++
>  xen/include/public/vm_event.h  | 11 ++++++++---
>  9 files changed, 123 insertions(+), 4 deletions(-)
>  create mode 100644 xen/arch/x86/vm_event.c
>  create mode 100644 xen/include/asm-arm/vm_event.h
>  create mode 100644 xen/include/asm-x86/vm_event.h

Looks good, and it definitely sounds useful.

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


Cheers,
Razvan

Re: [PATCH] x86/vm_event: toggle singlestep from vm_event response

[Andrew Cooper]

On 29/06/15 13:58, Tamas K Lengyel wrote:
> Add an option to the vm_event response to toggle singlestepping on the vCPU.
>
> Singed-off-by: Tamas K Lengyel <tlengyel@novetta.com>
> ---
>  MAINTAINERS                    |  1 +
>  xen/arch/x86/Makefile          |  1 +
>  xen/arch/x86/hvm/hvm.c         |  8 ++++++++
>  xen/arch/x86/vm_event.c        | 41 +++++++++++++++++++++++++++++++++++++++++
>  xen/common/vm_event.c          |  8 +++++++-
>  xen/include/asm-arm/vm_event.h | 29 +++++++++++++++++++++++++++++
>  xen/include/asm-x86/hvm/hvm.h  |  3 +++
>  xen/include/asm-x86/vm_event.h | 25 +++++++++++++++++++++++++
>  xen/include/public/vm_event.h  | 11 ++++++++---
>  9 files changed, 123 insertions(+), 4 deletions(-)
>  create mode 100644 xen/arch/x86/vm_event.c
>  create mode 100644 xen/include/asm-arm/vm_event.h
>  create mode 100644 xen/include/asm-x86/vm_event.h
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 6b1068e..59c0822 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -383,6 +383,7 @@ F:	xen/common/vm_event.c
>  F:	xen/common/mem_access.c
>  F:	xen/arch/x86/hvm/event.c
>  F:	xen/arch/x86/monitor.c
> +F:	xen/arch/x86/vm_event.c
>  
>  XENTRACE
>  M:	George Dunlap <george.dunlap@eu.citrix.com>
> diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
> index 37e547c..5f24951 100644
> --- a/xen/arch/x86/Makefile
> +++ b/xen/arch/x86/Makefile
> @@ -60,6 +60,7 @@ obj-y += machine_kexec.o
>  obj-y += crash.o
>  obj-y += tboot.o
>  obj-y += hpet.o
> +obj-y += vm_event.o
>  obj-y += xstate.o
>  
>  obj-$(crash_debug) += gdbstub.o
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 535d622..2bfd1b0 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -6431,6 +6431,14 @@ int hvm_debug_op(struct vcpu *v, int32_t op)
>      return rc;
>  }
>  
> +void hvm_toggle_singlestep(struct vcpu *v)
> +{
> +    if ( !cpu_has_monitor_trap_flag )

monitor_trap_flag is a VMX feature.  This will never be true on AMD
systems.  (its use in hvm_debug_op() is also dubious)

> +        return;
> +
> +    v->arch.hvm_vcpu.single_step = !v->arch.hvm_vcpu.single_step;
> +}
> +
>  int nhvm_vcpu_hostrestore(struct vcpu *v, struct cpu_user_regs *regs)
>  {
>      if (hvm_funcs.nhvm_vcpu_hostrestore)
> diff --git a/xen/arch/x86/vm_event.c b/xen/arch/x86/vm_event.c
> new file mode 100644
> index 0000000..95b30ad
> --- /dev/null
> +++ b/xen/arch/x86/vm_event.c
> @@ -0,0 +1,41 @@
> +/*
> + * arch/x86/vm_event.c
> + *
> + * Architecture-specific vm_event handling routines
> + *
> + * Copyright (c) 2015 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, write to the
> + * Free Software Foundation, Inc., 59 Temple Place - Suite 330,
> + * Boston, MA 021110-1307, USA.
> + */
> +
> +#include <xen/sched.h>
> +#include <asm/hvm/hvm.h>
> +
> +void vm_event_toggle_singlestep(struct domain *d, struct vcpu *v)
> +{
> +    if ( (v == current) || !is_hvm_domain(d) )

Why is 'current' excluded?

> +        return;
> +
> +    hvm_toggle_singlestep(v);
> +}
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/xen/common/vm_event.c b/xen/common/vm_event.c
> index 120a78a..2ee27e2 100644
> --- a/xen/common/vm_event.c
> +++ b/xen/common/vm_event.c
> @@ -27,6 +27,7 @@
>  #include <xen/vm_event.h>
>  #include <xen/mem_access.h>
>  #include <asm/p2m.h>
> +#include <asm/vm_event.h>
>  #include <xsm/xsm.h>
>  
>  /* for public/io/ring.h macros */
> @@ -399,9 +400,14 @@ void vm_event_resume(struct domain *d, struct vm_event_domain *ved)
>  
>          };
>  
> -        /* Unpause domain. */
>          if ( rsp.flags & VM_EVENT_FLAG_VCPU_PAUSED )
> +        {
> +            if ( rsp.flags & VM_EVENT_FLAG_TOGGLE_SINGLESTEP )
> +                vm_event_toggle_singlestep(d, v);
> +
> +            /* Unpause domain. */

I don't think this comment is useful to keep.

>              vm_event_vcpu_unpause(v);
> +        }
>      }
>  }
>  
> diff --git a/xen/include/asm-arm/vm_event.h b/xen/include/asm-arm/vm_event.h
> new file mode 100644
> index 0000000..a4cf4c6
> --- /dev/null
> +++ b/xen/include/asm-arm/vm_event.h
> @@ -0,0 +1,29 @@
> +/*
> + * vm_event.h: architecture specific vm_event handling routines
> + *
> + * Copyright (c) 2015 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,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope 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, write to the Free Software Foundation, Inc., 59 Temple
> + * Place - Suite 330, Boston, MA 02111-1307 USA.
> + */
> +
> +#ifndef __ASM_ARM_VM_EVENT_H__
> +#define __ASM_ARM_VM_EVENT_H__
> +
> +static inline
> +void vm_event_toggle_singlestep(struct domain *d, struct vcpu *v)
> +{
> +    /* Not supported on ARM. */
> +}
> +
> +#endif /* __ASM_ARM_VM_EVENT_H__ */
> diff --git a/xen/include/asm-x86/hvm/hvm.h b/xen/include/asm-x86/hvm/hvm.h
> index 57f9605..073b758 100644
> --- a/xen/include/asm-x86/hvm/hvm.h
> +++ b/xen/include/asm-x86/hvm/hvm.h
> @@ -448,6 +448,9 @@ static inline void hvm_set_info_guest(struct vcpu *v)
>  
>  int hvm_debug_op(struct vcpu *v, int32_t op);
>  
> +/* Caller should pause vcpu before calling this function */
> +void hvm_toggle_singlestep(struct vcpu *v);
> +
>  static inline void hvm_invalidate_regs_fields(struct cpu_user_regs *regs)
>  {
>  #ifndef NDEBUG
> diff --git a/xen/include/asm-x86/vm_event.h b/xen/include/asm-x86/vm_event.h
> new file mode 100644
> index 0000000..d36dd50
> --- /dev/null
> +++ b/xen/include/asm-x86/vm_event.h
> @@ -0,0 +1,25 @@
> +/*
> + * vm_event.h: architecture specific vm_event handling routines
> + *
> + * Copyright (c) 2015 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,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope 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, write to the Free Software Foundation, Inc., 59 Temple
> + * Place - Suite 330, Boston, MA 02111-1307 USA.
> + */
> +
> +#ifndef __ASM_X86_VM_EVENT_H__
> +#define __ASM_X86_VM_EVENT_H__
> +

This should either include sched.h or pre-declare struct domain and vcpu.

Otherwise, including <asm/vm_event.h> first in a list of includes will
cause a compile error.

~Andrew

> +void vm_event_toggle_singlestep(struct domain *d, struct vcpu *v);
> +
> +#endif /* __ASM_X86_VM_EVENT_H__ */
> diff --git a/xen/include/public/vm_event.h b/xen/include/public/vm_event.h
> index 577e971..6156d9e 100644
> --- a/xen/include/public/vm_event.h
> +++ b/xen/include/public/vm_event.h
> @@ -44,9 +44,14 @@
>   *  paused
>   * VCPU_PAUSED in a response signals to unpause the vCPU
>   */
> -#define VM_EVENT_FLAG_VCPU_PAUSED     (1 << 0)
> -/* Flags to aid debugging mem_event */
> -#define VM_EVENT_FLAG_FOREIGN         (1 << 1)
> +#define VM_EVENT_FLAG_VCPU_PAUSED       (1 << 0)
> +/* Flag to aid debugging mem_event */
> +#define VM_EVENT_FLAG_FOREIGN           (1 << 1)
> +/*
> + * Toggle singlestepping on vm_event response.
> + * Requires the vCPU to be paused already (synchronous events only).
> + */
> +#define VM_EVENT_FLAG_TOGGLE_SINGLESTEP (1 << 2)
>  
>  /*
>   * Reasons for the vm event request

Re: [PATCH] x86/vm_event: toggle singlestep from vm_event response

[Lengyel, Tamas]

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

On Mon, Jun 29, 2015 at 9:35 AM, Andrew Cooper <andrew.cooper3@citrix.com>
wrote:

> On 29/06/15 13:58, Tamas K Lengyel wrote:
> > Add an option to the vm_event response to toggle singlestepping on the
> vCPU.
> >
> > Singed-off-by: Tamas K Lengyel <tlengyel@novetta.com>
> > ---
> >  MAINTAINERS                    |  1 +
> >  xen/arch/x86/Makefile          |  1 +
> >  xen/arch/x86/hvm/hvm.c         |  8 ++++++++
> >  xen/arch/x86/vm_event.c        | 41
> +++++++++++++++++++++++++++++++++++++++++
> >  xen/common/vm_event.c          |  8 +++++++-
> >  xen/include/asm-arm/vm_event.h | 29 +++++++++++++++++++++++++++++
> >  xen/include/asm-x86/hvm/hvm.h  |  3 +++
> >  xen/include/asm-x86/vm_event.h | 25 +++++++++++++++++++++++++
> >  xen/include/public/vm_event.h  | 11 ++++++++---
> >  9 files changed, 123 insertions(+), 4 deletions(-)
> >  create mode 100644 xen/arch/x86/vm_event.c
> >  create mode 100644 xen/include/asm-arm/vm_event.h
> >  create mode 100644 xen/include/asm-x86/vm_event.h
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 6b1068e..59c0822 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -383,6 +383,7 @@ F:        xen/common/vm_event.c
> >  F:   xen/common/mem_access.c
> >  F:   xen/arch/x86/hvm/event.c
> >  F:   xen/arch/x86/monitor.c
> > +F:   xen/arch/x86/vm_event.c
> >
> >  XENTRACE
> >  M:   George Dunlap <george.dunlap@eu.citrix.com>
> > diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
> > index 37e547c..5f24951 100644
> > --- a/xen/arch/x86/Makefile
> > +++ b/xen/arch/x86/Makefile
> > @@ -60,6 +60,7 @@ obj-y += machine_kexec.o
> >  obj-y += crash.o
> >  obj-y += tboot.o
> >  obj-y += hpet.o
> > +obj-y += vm_event.o
> >  obj-y += xstate.o
> >
> >  obj-$(crash_debug) += gdbstub.o
> > diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> > index 535d622..2bfd1b0 100644
> > --- a/xen/arch/x86/hvm/hvm.c
> > +++ b/xen/arch/x86/hvm/hvm.c
> > @@ -6431,6 +6431,14 @@ int hvm_debug_op(struct vcpu *v, int32_t op)
> >      return rc;
> >  }
> >
> > +void hvm_toggle_singlestep(struct vcpu *v)
> > +{
> > +    if ( !cpu_has_monitor_trap_flag )
>
> monitor_trap_flag is a VMX feature.  This will never be true on AMD
> systems.  (its use in hvm_debug_op() is also dubious)
>

Yes, this feature is only for Intel cpus. Reworking the use of the flag
though is a bit out-of-scope for this patch itself.


>
> > +        return;
> > +
> > +    v->arch.hvm_vcpu.single_step = !v->arch.hvm_vcpu.single_step;
> > +}
> > +
> >  int nhvm_vcpu_hostrestore(struct vcpu *v, struct cpu_user_regs *regs)
> >  {
> >      if (hvm_funcs.nhvm_vcpu_hostrestore)
> > diff --git a/xen/arch/x86/vm_event.c b/xen/arch/x86/vm_event.c
> > new file mode 100644
> > index 0000000..95b30ad
> > --- /dev/null
> > +++ b/xen/arch/x86/vm_event.c
> > @@ -0,0 +1,41 @@
> > +/*
> > + * arch/x86/vm_event.c
> > + *
> > + * Architecture-specific vm_event handling routines
> > + *
> > + * Copyright (c) 2015 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, write to the
> > + * Free Software Foundation, Inc., 59 Temple Place - Suite 330,
> > + * Boston, MA 021110-1307, USA.
> > + */
> > +
> > +#include <xen/sched.h>
> > +#include <asm/hvm/hvm.h>
> > +
> > +void vm_event_toggle_singlestep(struct domain *d, struct vcpu *v)
> > +{
> > +    if ( (v == current) || !is_hvm_domain(d) )
>
> Why is 'current' excluded?
>

Only to be consistent with the sanity check applied for XEN_DOMCTL_debug_op.


>
> > +        return;
> > +
> > +    hvm_toggle_singlestep(v);
> > +}
> > +
> > +/*
> > + * Local variables:
> > + * mode: C
> > + * c-file-style: "BSD"
> > + * c-basic-offset: 4
> > + * indent-tabs-mode: nil
> > + * End:
> > + */
> > diff --git a/xen/common/vm_event.c b/xen/common/vm_event.c
> > index 120a78a..2ee27e2 100644
> > --- a/xen/common/vm_event.c
> > +++ b/xen/common/vm_event.c
> > @@ -27,6 +27,7 @@
> >  #include <xen/vm_event.h>
> >  #include <xen/mem_access.h>
> >  #include <asm/p2m.h>
> > +#include <asm/vm_event.h>
> >  #include <xsm/xsm.h>
> >
> >  /* for public/io/ring.h macros */
> > @@ -399,9 +400,14 @@ void vm_event_resume(struct domain *d, struct
> vm_event_domain *ved)
> >
> >          };
> >
> > -        /* Unpause domain. */
> >          if ( rsp.flags & VM_EVENT_FLAG_VCPU_PAUSED )
> > +        {
> > +            if ( rsp.flags & VM_EVENT_FLAG_TOGGLE_SINGLESTEP )
> > +                vm_event_toggle_singlestep(d, v);
> > +
> > +            /* Unpause domain. */
>
> I don't think this comment is useful to keep.
>

Yea, agree.


> >              vm_event_vcpu_unpause(v);
> > +        }
> >      }
> >  }
> >
> > diff --git a/xen/include/asm-arm/vm_event.h
> b/xen/include/asm-arm/vm_event.h
> > new file mode 100644
> > index 0000000..a4cf4c6
> > --- /dev/null
> > +++ b/xen/include/asm-arm/vm_event.h
> > @@ -0,0 +1,29 @@
> > +/*
> > + * vm_event.h: architecture specific vm_event handling routines
> > + *
> > + * Copyright (c) 2015 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,
> > + * version 2, as published by the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope 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, write to the Free Software Foundation, Inc.,
> 59 Temple
> > + * Place - Suite 330, Boston, MA 02111-1307 USA.
> > + */
> > +
> > +#ifndef __ASM_ARM_VM_EVENT_H__
> > +#define __ASM_ARM_VM_EVENT_H__
> > +
> > +static inline
> > +void vm_event_toggle_singlestep(struct domain *d, struct vcpu *v)
> > +{
> > +    /* Not supported on ARM. */
> > +}
> > +
> > +#endif /* __ASM_ARM_VM_EVENT_H__ */
> > diff --git a/xen/include/asm-x86/hvm/hvm.h
> b/xen/include/asm-x86/hvm/hvm.h
> > index 57f9605..073b758 100644
> > --- a/xen/include/asm-x86/hvm/hvm.h
> > +++ b/xen/include/asm-x86/hvm/hvm.h
> > @@ -448,6 +448,9 @@ static inline void hvm_set_info_guest(struct vcpu *v)
> >
> >  int hvm_debug_op(struct vcpu *v, int32_t op);
> >
> > +/* Caller should pause vcpu before calling this function */
> > +void hvm_toggle_singlestep(struct vcpu *v);
> > +
> >  static inline void hvm_invalidate_regs_fields(struct cpu_user_regs
> *regs)
> >  {
> >  #ifndef NDEBUG
> > diff --git a/xen/include/asm-x86/vm_event.h
> b/xen/include/asm-x86/vm_event.h
> > new file mode 100644
> > index 0000000..d36dd50
> > --- /dev/null
> > +++ b/xen/include/asm-x86/vm_event.h
> > @@ -0,0 +1,25 @@
> > +/*
> > + * vm_event.h: architecture specific vm_event handling routines
> > + *
> > + * Copyright (c) 2015 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,
> > + * version 2, as published by the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope 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, write to the Free Software Foundation, Inc.,
> 59 Temple
> > + * Place - Suite 330, Boston, MA 02111-1307 USA.
> > + */
> > +
> > +#ifndef __ASM_X86_VM_EVENT_H__
> > +#define __ASM_X86_VM_EVENT_H__
> > +
>
> This should either include sched.h or pre-declare struct domain and vcpu.
>
> Otherwise, including <asm/vm_event.h> first in a list of includes will
> cause a compile error.
>

Ack.


> ~Andrew
>

Thanks!
Tamas

[-- Attachment #1.2: Type: text/html, Size: 11338 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] x86/vm_event: toggle singlestep from vm_event response

[Andrew Cooper]

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

On 29/06/15 14:42, Lengyel, Tamas wrote:
>
>
>     > --- a/xen/arch/x86/hvm/hvm.c
>     > +++ b/xen/arch/x86/hvm/hvm.c
>     > @@ -6431,6 +6431,14 @@ int hvm_debug_op(struct vcpu *v, int32_t op)
>     >      return rc;
>     >  }
>     >
>     > +void hvm_toggle_singlestep(struct vcpu *v)
>     > +{
>     > +    if ( !cpu_has_monitor_trap_flag )
>
>     monitor_trap_flag is a VMX feature.  This will never be true on AMD
>     systems.  (its use in hvm_debug_op() is also dubious)
>
>
> Yes, this feature is only for Intel cpus. Reworking the use of the
> flag though is a bit out-of-scope for this patch itself.

In which case you must make it very clear in the commit message that
this is for Intel only and needs fixing up for AMD.  Best also to have a
comment to the same effect in this function.

>  
>
>
>     > +        return;
>     > +
>     > +    v->arch.hvm_vcpu.single_step = !v->arch.hvm_vcpu.single_step;
>     > +}
>     > +
>     >  int nhvm_vcpu_hostrestore(struct vcpu *v, struct cpu_user_regs
>     *regs)
>     >  {
>     >      if (hvm_funcs.nhvm_vcpu_hostrestore)
>     > diff --git a/xen/arch/x86/vm_event.c b/xen/arch/x86/vm_event.c
>     > new file mode 100644
>     > index 0000000..95b30ad
>     > --- /dev/null
>     > +++ b/xen/arch/x86/vm_event.c
>     > @@ -0,0 +1,41 @@
>     > +/*
>     > + * arch/x86/vm_event.c
>     > + *
>     > + * Architecture-specific vm_event handling routines
>     > + *
>     > + * Copyright (c) 2015 Tamas K Lengyel (tamas@tklengyel.com
>     <mailto: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, write to the
>     > + * Free Software Foundation, Inc., 59 Temple Place - Suite 330,
>     > + * Boston, MA 021110-1307, USA.
>     > + */
>     > +
>     > +#include <xen/sched.h>
>     > +#include <asm/hvm/hvm.h>
>     > +
>     > +void vm_event_toggle_singlestep(struct domain *d, struct vcpu *v)
>     > +{
>     > +    if ( (v == current) || !is_hvm_domain(d) )
>
>     Why is 'current' excluded?
>
>
> Only to be consistent with the sanity check applied for
> XEN_DOMCTL_debug_op.

XEN_DOMCTL_debug_op needs the check because of vcpu_pause().  It is
always run in the context of the hypercaller domain, and must never end
up calling singlestep on itself.

However in this case, we can only legitimately use this on an
already-paused vcpu, which guarentees that Xen is not currently in the
context of the target vcpu.

It would probably be better to have a check against the vcpu pause count
here (with early return), and hvm_toggle_singlestep() assert so.

~Andrew

[-- Attachment #1.2: Type: text/html, Size: 6425 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] x86/vm_event: toggle singlestep from vm_event response

[Lengyel, Tamas]

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

On Mon, Jun 29, 2015 at 9:55 AM, Andrew Cooper <andrew.cooper3@citrix.com>
wrote:

>  On 29/06/15 14:42, Lengyel, Tamas wrote:
>
>
>
>   > --- a/xen/arch/x86/hvm/hvm.c
>> > +++ b/xen/arch/x86/hvm/hvm.c
>> > @@ -6431,6 +6431,14 @@ int hvm_debug_op(struct vcpu *v, int32_t op)
>> >      return rc;
>> >  }
>> >
>> > +void hvm_toggle_singlestep(struct vcpu *v)
>> > +{
>> > +    if ( !cpu_has_monitor_trap_flag )
>>
>>  monitor_trap_flag is a VMX feature.  This will never be true on AMD
>> systems.  (its use in hvm_debug_op() is also dubious)
>>
>
>  Yes, this feature is only for Intel cpus. Reworking the use of the flag
> though is a bit out-of-scope for this patch itself.
>
>
> In which case you must make it very clear in the commit message that this
> is for Intel only and needs fixing up for AMD.  Best also to have a comment
> to the same effect in this function.
>

Sure.


>
>
>
>
>>
>> > +        return;
>> > +
>> > +    v->arch.hvm_vcpu.single_step = !v->arch.hvm_vcpu.single_step;
>> > +}
>> > +
>> >  int nhvm_vcpu_hostrestore(struct vcpu *v, struct cpu_user_regs *regs)
>> >  {
>> >      if (hvm_funcs.nhvm_vcpu_hostrestore)
>> > diff --git a/xen/arch/x86/vm_event.c b/xen/arch/x86/vm_event.c
>> > new file mode 100644
>> > index 0000000..95b30ad
>> > --- /dev/null
>> > +++ b/xen/arch/x86/vm_event.c
>> > @@ -0,0 +1,41 @@
>> > +/*
>> > + * arch/x86/vm_event.c
>> > + *
>> > + * Architecture-specific vm_event handling routines
>> > + *
>> > + * Copyright (c) 2015 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, write to the
>> > + * Free Software Foundation, Inc., 59 Temple Place - Suite 330,
>> > + * Boston, MA 021110-1307, USA.
>> > + */
>> > +
>> > +#include <xen/sched.h>
>> > +#include <asm/hvm/hvm.h>
>> > +
>> > +void vm_event_toggle_singlestep(struct domain *d, struct vcpu *v)
>> > +{
>> > +    if ( (v == current) || !is_hvm_domain(d) )
>>
>>  Why is 'current' excluded?
>>
>
>  Only to be consistent with the sanity check applied for
> XEN_DOMCTL_debug_op.
>
>
> XEN_DOMCTL_debug_op needs the check because of vcpu_pause().  It is always
> run in the context of the hypercaller domain, and must never end up calling
> singlestep on itself.
>
> However in this case, we can only legitimately use this on an
> already-paused vcpu, which guarentees that Xen is not currently in the
> context of the target vcpu.
>
> It would probably be better to have a check against the vcpu pause count
> here (with early return), and hvm_toggle_singlestep() assert so.
>

I guess that's a fair sanity check to add in case the vm_event listener is
buggy.


>
> ~Andrew
>

Thanks,
Tamas

[-- Attachment #1.2: Type: text/html, Size: 7061 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] x86/vm_event: toggle singlestep from vm_event response

[Lengyel, Tamas]

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

On Mon, Jun 29, 2015 at 10:09 AM, Lengyel, Tamas <tlengyel@novetta.com>
wrote:

>
>
> On Mon, Jun 29, 2015 at 9:55 AM, Andrew Cooper <andrew.cooper3@citrix.com>
> wrote:
>
>>  On 29/06/15 14:42, Lengyel, Tamas wrote:
>>
>>
>>
>>   > --- a/xen/arch/x86/hvm/hvm.c
>>> > +++ b/xen/arch/x86/hvm/hvm.c
>>> > @@ -6431,6 +6431,14 @@ int hvm_debug_op(struct vcpu *v, int32_t op)
>>> >      return rc;
>>> >  }
>>> >
>>> > +void hvm_toggle_singlestep(struct vcpu *v)
>>> > +{
>>> > +    if ( !cpu_has_monitor_trap_flag )
>>>
>>>  monitor_trap_flag is a VMX feature.  This will never be true on AMD
>>> systems.  (its use in hvm_debug_op() is also dubious)
>>>
>>
>>  Yes, this feature is only for Intel cpus. Reworking the use of the flag
>> though is a bit out-of-scope for this patch itself.
>>
>>
>> In which case you must make it very clear in the commit message that this
>> is for Intel only and needs fixing up for AMD.  Best also to have a comment
>> to the same effect in this function.
>>
>
> Sure.
>
>
>>
>>
>>
>>
>>>
>>> > +        return;
>>> > +
>>> > +    v->arch.hvm_vcpu.single_step = !v->arch.hvm_vcpu.single_step;
>>> > +}
>>> > +
>>> >  int nhvm_vcpu_hostrestore(struct vcpu *v, struct cpu_user_regs *regs)
>>> >  {
>>> >      if (hvm_funcs.nhvm_vcpu_hostrestore)
>>> > diff --git a/xen/arch/x86/vm_event.c b/xen/arch/x86/vm_event.c
>>> > new file mode 100644
>>> > index 0000000..95b30ad
>>> > --- /dev/null
>>> > +++ b/xen/arch/x86/vm_event.c
>>> > @@ -0,0 +1,41 @@
>>> > +/*
>>> > + * arch/x86/vm_event.c
>>> > + *
>>> > + * Architecture-specific vm_event handling routines
>>> > + *
>>> > + * Copyright (c) 2015 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, write to the
>>> > + * Free Software Foundation, Inc., 59 Temple Place - Suite 330,
>>> > + * Boston, MA 021110-1307, USA.
>>> > + */
>>> > +
>>> > +#include <xen/sched.h>
>>> > +#include <asm/hvm/hvm.h>
>>> > +
>>> > +void vm_event_toggle_singlestep(struct domain *d, struct vcpu *v)
>>> > +{
>>> > +    if ( (v == current) || !is_hvm_domain(d) )
>>>
>>>  Why is 'current' excluded?
>>>
>>
>>  Only to be consistent with the sanity check applied for
>> XEN_DOMCTL_debug_op.
>>
>>
>> XEN_DOMCTL_debug_op needs the check because of vcpu_pause().  It is
>> always run in the context of the hypercaller domain, and must never end up
>> calling singlestep on itself.
>>
>> However in this case, we can only legitimately use this on an
>> already-paused vcpu, which guarentees that Xen is not currently in the
>> context of the target vcpu.
>>
>> It would probably be better to have a check against the vcpu pause count
>> here (with early return), and hvm_toggle_singlestep() assert so.
>>
>
> I guess that's a fair sanity check to add in case the vm_event listener is
> buggy.
>

I was thinking ASSERT(v->vm_event_pause_count.counter); but that locks the
function to only be usable through the vm_event response method. What do
you think?

Tamas

[-- Attachment #1.2: Type: text/html, Size: 7421 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] x86/vm_event: toggle singlestep from vm_event response

[Andrew Cooper]

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

On 29/06/15 15:17, Lengyel, Tamas wrote:
>
>
> On Mon, Jun 29, 2015 at 10:09 AM, Lengyel, Tamas <tlengyel@novetta.com
> <mailto:tlengyel@novetta.com>> wrote:
>
>
>
>     On Mon, Jun 29, 2015 at 9:55 AM, Andrew Cooper
>     <andrew.cooper3@citrix.com <mailto:andrew.cooper3@citrix.com>> wrote:
>
>         On 29/06/15 14:42, Lengyel, Tamas wrote:
>>
>>
>>             > --- a/xen/arch/x86/hvm/hvm.c
>>             > +++ b/xen/arch/x86/hvm/hvm.c
>>             > @@ -6431,6 +6431,14 @@ int hvm_debug_op(struct vcpu *v,
>>             int32_t op)
>>             >      return rc;
>>             >  }
>>             >
>>             > +void hvm_toggle_singlestep(struct vcpu *v)
>>             > +{
>>             > +    if ( !cpu_has_monitor_trap_flag )
>>
>>             monitor_trap_flag is a VMX feature.  This will never be
>>             true on AMD
>>             systems.  (its use in hvm_debug_op() is also dubious)
>>
>>
>>         Yes, this feature is only for Intel cpus. Reworking the use
>>         of the flag though is a bit out-of-scope for this patch itself.
>
>         In which case you must make it very clear in the commit
>         message that this is for Intel only and needs fixing up for
>         AMD.  Best also to have a comment to the same effect in this
>         function.
>
>
>     Sure.
>      
>
>
>
>>          
>>
>>
>>             > +        return;
>>             > +
>>             > +    v->arch.hvm_vcpu.single_step =
>>             !v->arch.hvm_vcpu.single_step;
>>             > +}
>>             > +
>>             >  int nhvm_vcpu_hostrestore(struct vcpu *v, struct
>>             cpu_user_regs *regs)
>>             >  {
>>             >      if (hvm_funcs.nhvm_vcpu_hostrestore)
>>             > diff --git a/xen/arch/x86/vm_event.c
>>             b/xen/arch/x86/vm_event.c
>>             > new file mode 100644
>>             > index 0000000..95b30ad
>>             > --- /dev/null
>>             > +++ b/xen/arch/x86/vm_event.c
>>             > @@ -0,0 +1,41 @@
>>             > +/*
>>             > + * arch/x86/vm_event.c
>>             > + *
>>             > + * Architecture-specific vm_event handling routines
>>             > + *
>>             > + * Copyright (c) 2015 Tamas K Lengyel
>>             (tamas@tklengyel.com <mailto: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, write to the
>>             > + * Free Software Foundation, Inc., 59 Temple Place -
>>             Suite 330,
>>             > + * Boston, MA 021110-1307, USA.
>>             > + */
>>             > +
>>             > +#include <xen/sched.h>
>>             > +#include <asm/hvm/hvm.h>
>>             > +
>>             > +void vm_event_toggle_singlestep(struct domain *d,
>>             struct vcpu *v)
>>             > +{
>>             > +    if ( (v == current) || !is_hvm_domain(d) )
>>
>>             Why is 'current' excluded?
>>
>>
>>         Only to be consistent with the sanity check applied for
>>         XEN_DOMCTL_debug_op.
>
>         XEN_DOMCTL_debug_op needs the check because of vcpu_pause(). 
>         It is always run in the context of the hypercaller domain, and
>         must never end up calling singlestep on itself.
>
>         However in this case, we can only legitimately use this on an
>         already-paused vcpu, which guarentees that Xen is not
>         currently in the context of the target vcpu.
>
>         It would probably be better to have a check against the vcpu
>         pause count here (with early return), and
>         hvm_toggle_singlestep() assert so.
>
>
>     I guess that's a fair sanity check to add in case the vm_event
>     listener is buggy.
>
>
> I was thinking ASSERT(v->vm_event_pause_count.counter); but that locks
> the function to only be usable through the vm_event response method.
> What do you think?

You want to use atomic_read() instead of peeking into .counter.

vm_event_toggle_singlestep() can legitimately gate on
v->vm_event_pause_count, whereas hvm_toggle_singlestep() can get away
with looking at v->pause_count.

See vmx_domain_emable_pml() as a similar example.

~Andrew

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

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

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