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

[PATCH v2] 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.
This is only supported on Intel CPUs which have Monitor Trap Flag capability.

Singed-off-by: Tamas K Lengyel <tlengyel@novetta.com>
Acked-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
---
v2: Include sched.h in headers and sanity check that vcpu is paused.
---
 MAINTAINERS                    |  1 +
 xen/arch/x86/Makefile          |  1 +
 xen/arch/x86/hvm/hvm.c         | 11 +++++++++++
 xen/arch/x86/vm_event.c        | 41 +++++++++++++++++++++++++++++++++++++++++
 xen/common/vm_event.c          |  7 ++++++-
 xen/include/asm-arm/vm_event.h | 31 +++++++++++++++++++++++++++++++
 xen/include/asm-x86/hvm/hvm.h  |  3 +++
 xen/include/asm-x86/vm_event.h | 27 +++++++++++++++++++++++++++
 xen/include/public/vm_event.h  | 12 +++++++++---
 9 files changed, 130 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..e951020 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -6431,6 +6431,17 @@ int hvm_debug_op(struct vcpu *v, int32_t op)
     return rc;
 }
 
+void hvm_toggle_singlestep(struct vcpu *v)
+{
+    ASSERT(atomic_read(&v->pause_count));
+
+    /* This feature is only available on Intel CPUs. */
+    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..c390225
--- /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 ( !is_hvm_domain(d) || !atomic_read(&v->vm_event_pause_count) )
+        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..a4b9c36 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,13 @@ 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);
+
             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..a517495
--- /dev/null
+++ b/xen/include/asm-arm/vm_event.h
@@ -0,0 +1,31 @@
+/*
+ * 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__
+
+#include <xen/sched.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..7cc3a3d
--- /dev/null
+++ b/xen/include/asm-x86/vm_event.h
@@ -0,0 +1,27 @@
+/*
+ * 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__
+
+#include <xen/sched.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..b8c3dde 100644
--- a/xen/include/public/vm_event.h
+++ b/xen/include/public/vm_event.h
@@ -44,9 +44,15 @@
  *  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).
+ * Only supported on Intel CPUs with MTF capability.
+ */
+#define VM_EVENT_FLAG_TOGGLE_SINGLESTEP (1 << 2)
 
 /*
  * Reasons for the vm event request
-- 
2.1.4

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

[Andrew Cooper]

On 30/06/15 15:11, Tamas K Lengyel wrote:
> diff --git a/xen/include/public/vm_event.h b/xen/include/public/vm_event.h
> index 577e971..b8c3dde 100644
> --- a/xen/include/public/vm_event.h
> +++ b/xen/include/public/vm_event.h
> @@ -44,9 +44,15 @@
>   *  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).
> + * Only supported on Intel CPUs with MTF capability.

This sentence shouldn't be in the public API.  It is a limitation of the
current implementation, not of the API, and could be removed with
further development.

With this removed, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

> + */
> +#define VM_EVENT_FLAG_TOGGLE_SINGLESTEP (1 << 2)
>  
>  /*
>   * Reasons for the vm event request

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

[Lengyel, Tamas]

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

On Tue, Jun 30, 2015 at 10:18 AM, Andrew Cooper <andrew.cooper3@citrix.com>
wrote:

> On 30/06/15 15:11, Tamas K Lengyel wrote:
> > diff --git a/xen/include/public/vm_event.h
> b/xen/include/public/vm_event.h
> > index 577e971..b8c3dde 100644
> > --- a/xen/include/public/vm_event.h
> > +++ b/xen/include/public/vm_event.h
> > @@ -44,9 +44,15 @@
> >   *  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).
> > + * Only supported on Intel CPUs with MTF capability.
>
> This sentence shouldn't be in the public API.  It is a limitation of the
> current implementation, not of the API, and could be removed with
> further development.
>

I disagree because there is no error condition returned if a user tries to
use it on non-Intel hw, so the only option a user would have to figure out
why it's not working is reading the Xen source. IMHO the public API should
describe the limitations as that's what potential users will read first.
When we have hardware other then Intel that supports something like this,
we can remove the comment.

Tamas


>
> With this removed, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
>
> > + */
> > +#define VM_EVENT_FLAG_TOGGLE_SINGLESTEP (1 << 2)
> >
> >  /*
> >   * Reasons for the vm event request
>
>

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

[Jan Beulich]

>>> On 30.06.15 at 16:40, <tlengyel@novetta.com> wrote:
> On Tue, Jun 30, 2015 at 10:18 AM, Andrew Cooper <andrew.cooper3@citrix.com>
> wrote:
> 
>> On 30/06/15 15:11, Tamas K Lengyel wrote:
>> > diff --git a/xen/include/public/vm_event.h
>> b/xen/include/public/vm_event.h
>> > index 577e971..b8c3dde 100644
>> > --- a/xen/include/public/vm_event.h
>> > +++ b/xen/include/public/vm_event.h
>> > @@ -44,9 +44,15 @@
>> >   *  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).
>> > + * Only supported on Intel CPUs with MTF capability.
>>
>> This sentence shouldn't be in the public API.  It is a limitation of the
>> current implementation, not of the API, and could be removed with
>> further development.
>>
> 
> I disagree because there is no error condition returned if a user tries to
> use it on non-Intel hw, so the only option a user would have to figure out
> why it's not working is reading the Xen source. IMHO the public API should
> describe the limitations as that's what potential users will read first.
> When we have hardware other then Intel that supports something like this,
> we can remove the comment.

FWIW I agree with Andrew, and if on non-Intel hardware there's
no error (or other indication) being returned, that's actually an
issue to be fixed imo.

Jan

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

[Lengyel, Tamas]

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

On Mon, Jul 6, 2015 at 11:26 AM, Jan Beulich <JBeulich@suse.com> wrote:

> >>> On 30.06.15 at 16:40, <tlengyel@novetta.com> wrote:
> > On Tue, Jun 30, 2015 at 10:18 AM, Andrew Cooper <
> andrew.cooper3@citrix.com>
> > wrote:
> >
> >> On 30/06/15 15:11, Tamas K Lengyel wrote:
> >> > diff --git a/xen/include/public/vm_event.h
> >> b/xen/include/public/vm_event.h
> >> > index 577e971..b8c3dde 100644
> >> > --- a/xen/include/public/vm_event.h
> >> > +++ b/xen/include/public/vm_event.h
> >> > @@ -44,9 +44,15 @@
> >> >   *  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).
> >> > + * Only supported on Intel CPUs with MTF capability.
> >>
> >> This sentence shouldn't be in the public API.  It is a limitation of the
> >> current implementation, not of the API, and could be removed with
> >> further development.
> >>
> >
> > I disagree because there is no error condition returned if a user tries
> to
> > use it on non-Intel hw, so the only option a user would have to figure
> out
> > why it's not working is reading the Xen source. IMHO the public API
> should
> > describe the limitations as that's what potential users will read first.
> > When we have hardware other then Intel that supports something like this,
> > we can remove the comment.
>
> FWIW I agree with Andrew, and if on non-Intel hardware there's
> no error (or other indication) being returned, that's actually an
> issue to be fixed imo.
>
> Jan
>

There is no opportunity for that, the current API does not provide a
mechanism to signal failure on things that were requested on the vm_event
response. Creating such a mechanism is beyond the scope of this patch and I
don't think it's necessary. IMHO the comment makes it clear that this will
only work on Intel hardware which suffices for now.

Tamas

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

[Jan Beulich]

>>> On 06.07.15 at 17:35, <tlengyel@novetta.com> wrote:
> On Mon, Jul 6, 2015 at 11:26 AM, Jan Beulich <JBeulich@suse.com> wrote:
> 
>> >>> On 30.06.15 at 16:40, <tlengyel@novetta.com> wrote:
>> > On Tue, Jun 30, 2015 at 10:18 AM, Andrew Cooper <
>> andrew.cooper3@citrix.com>
>> > wrote:
>> >
>> >> On 30/06/15 15:11, Tamas K Lengyel wrote:
>> >> > diff --git a/xen/include/public/vm_event.h
>> >> b/xen/include/public/vm_event.h
>> >> > index 577e971..b8c3dde 100644
>> >> > --- a/xen/include/public/vm_event.h
>> >> > +++ b/xen/include/public/vm_event.h
>> >> > @@ -44,9 +44,15 @@
>> >> >   *  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).
>> >> > + * Only supported on Intel CPUs with MTF capability.
>> >>
>> >> This sentence shouldn't be in the public API.  It is a limitation of the
>> >> current implementation, not of the API, and could be removed with
>> >> further development.
>> >>
>> >
>> > I disagree because there is no error condition returned if a user tries
>> to
>> > use it on non-Intel hw, so the only option a user would have to figure
>> out
>> > why it's not working is reading the Xen source. IMHO the public API
>> should
>> > describe the limitations as that's what potential users will read first.
>> > When we have hardware other then Intel that supports something like this,
>> > we can remove the comment.
>>
>> FWIW I agree with Andrew, and if on non-Intel hardware there's
>> no error (or other indication) being returned, that's actually an
>> issue to be fixed imo.
> 
> There is no opportunity for that, the current API does not provide a
> mechanism to signal failure on things that were requested on the vm_event
> response. Creating such a mechanism is beyond the scope of this patch and I
> don't think it's necessary. IMHO the comment makes it clear that this will
> only work on Intel hardware which suffices for now.

You're the maintainer of the code in question, so I won't (and
can't) enforce Andrew's and my view.

Jan

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

[Lengyel, Tamas]

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

On Mon, Jul 6, 2015 at 11:54 AM, Jan Beulich <JBeulich@suse.com> wrote:

> >>> On 06.07.15 at 17:35, <tlengyel@novetta.com> wrote:
> > On Mon, Jul 6, 2015 at 11:26 AM, Jan Beulich <JBeulich@suse.com> wrote:
> >
> >> >>> On 30.06.15 at 16:40, <tlengyel@novetta.com> wrote:
> >> > On Tue, Jun 30, 2015 at 10:18 AM, Andrew Cooper <
> >> andrew.cooper3@citrix.com>
> >> > wrote:
> >> >
> >> >> On 30/06/15 15:11, Tamas K Lengyel wrote:
> >> >> > diff --git a/xen/include/public/vm_event.h
> >> >> b/xen/include/public/vm_event.h
> >> >> > index 577e971..b8c3dde 100644
> >> >> > --- a/xen/include/public/vm_event.h
> >> >> > +++ b/xen/include/public/vm_event.h
> >> >> > @@ -44,9 +44,15 @@
> >> >> >   *  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).
> >> >> > + * Only supported on Intel CPUs with MTF capability.
> >> >>
> >> >> This sentence shouldn't be in the public API.  It is a limitation of
> the
> >> >> current implementation, not of the API, and could be removed with
> >> >> further development.
> >> >>
> >> >
> >> > I disagree because there is no error condition returned if a user
> tries
> >> to
> >> > use it on non-Intel hw, so the only option a user would have to figure
> >> out
> >> > why it's not working is reading the Xen source. IMHO the public API
> >> should
> >> > describe the limitations as that's what potential users will read
> first.
> >> > When we have hardware other then Intel that supports something like
> this,
> >> > we can remove the comment.
> >>
> >> FWIW I agree with Andrew, and if on non-Intel hardware there's
> >> no error (or other indication) being returned, that's actually an
> >> issue to be fixed imo.
> >
> > There is no opportunity for that, the current API does not provide a
> > mechanism to signal failure on things that were requested on the vm_event
> > response. Creating such a mechanism is beyond the scope of this patch
> and I
> > don't think it's necessary. IMHO the comment makes it clear that this
> will
> > only work on Intel hardware which suffices for now.
>
> You're the maintainer of the code in question, so I won't (and
> can't) enforce Andrew's and my view.
>
> Jan
>

Unless Razvan have a different opinion on the matter (although he already
Acked it), I think it's good as is.

Thanks,
Tamas

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

[Razvan Cojocaru]

On 07/06/2015 07:26 PM, Lengyel, Tamas wrote:
> 
> 
> On Mon, Jul 6, 2015 at 11:54 AM, Jan Beulich <JBeulich@suse.com
> <mailto:JBeulich@suse.com>> wrote:
> 
>     >>> On 06.07.15 at 17:35, <tlengyel@novetta.com
>     <mailto:tlengyel@novetta.com>> wrote:
>     > On Mon, Jul 6, 2015 at 11:26 AM, Jan Beulich <JBeulich@suse.com
>     <mailto:JBeulich@suse.com>> wrote:
>     >
>     >> >>> On 30.06.15 at 16:40, <tlengyel@novetta.com
>     <mailto:tlengyel@novetta.com>> wrote:
>     >> > On Tue, Jun 30, 2015 at 10:18 AM, Andrew Cooper <
>     >> andrew.cooper3@citrix.com <mailto:andrew.cooper3@citrix.com>>
>     >> > wrote:
>     >> >
>     >> >> On 30/06/15 15:11, Tamas K Lengyel wrote:
>     >> >> > diff --git a/xen/include/public/vm_event.h
>     >> >> b/xen/include/public/vm_event.h
>     >> >> > index 577e971..b8c3dde 100644
>     >> >> > --- a/xen/include/public/vm_event.h
>     >> >> > +++ b/xen/include/public/vm_event.h
>     >> >> > @@ -44,9 +44,15 @@
>     >> >> >   *  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).
>     >> >> > + * Only supported on Intel CPUs with MTF capability.
>     >> >>
>     >> >> This sentence shouldn't be in the public API.  It is a
>     limitation of the
>     >> >> current implementation, not of the API, and could be removed with
>     >> >> further development.
>     >> >>
>     >> >
>     >> > I disagree because there is no error condition returned if a
>     user tries
>     >> to
>     >> > use it on non-Intel hw, so the only option a user would have to
>     figure
>     >> out
>     >> > why it's not working is reading the Xen source. IMHO the public API
>     >> should
>     >> > describe the limitations as that's what potential users will
>     read first.
>     >> > When we have hardware other then Intel that supports something
>     like this,
>     >> > we can remove the comment.
>     >>
>     >> FWIW I agree with Andrew, and if on non-Intel hardware there's
>     >> no error (or other indication) being returned, that's actually an
>     >> issue to be fixed imo.
>     >
>     > There is no opportunity for that, the current API does not provide a
>     > mechanism to signal failure on things that were requested on the vm_event
>     > response. Creating such a mechanism is beyond the scope of this patch and I
>     > don't think it's necessary. IMHO the comment makes it clear that this will
>     > only work on Intel hardware which suffices for now.
> 
>     You're the maintainer of the code in question, so I won't (and
>     can't) enforce Andrew's and my view.
> 
>     Jan
> 
> 
> Unless Razvan have a different opinion on the matter (although he
> already Acked it), I think it's good as is.

I don't mind just having the comment for now, so for what it's worth I
stand by my ack.

Having said that (and with the understading that it is beyond the scope
of this patch), a way to validate things like these is a good idea. I
wonder if, in a future patch, we could not have ./configure detect these
things and simply disable the relevant VM_EVENT_FLAG constants with
#if(n)defs, for example. That way, you wouldn't be able to compile code
that wouldn't work silently on platforms where that is the case.


Regards,
Razvan

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

[Lengyel, Tamas]

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

> Having said that (and with the understading that it is beyond the scope
> of this patch), a way to validate things like these is a good idea. I
> wonder if, in a future patch, we could not have ./configure detect these
> things and simply disable the relevant VM_EVENT_FLAG constants with
> #if(n)defs, for example. That way, you wouldn't be able to compile code
> that wouldn't work silently on platforms where that is the case.
>

It would be something worth investigating, definitely.

Tamas

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

[Andrew Cooper]

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

On 06/07/15 18:08, Lengyel, Tamas wrote:
>
>     Having said that (and with the understading that it is beyond the
>     scope
>     of this patch), a way to validate things like these is a good idea. I
>     wonder if, in a future patch, we could not have ./configure detect
>     these
>     things and simply disable the relevant VM_EVENT_FLAG constants with
>     #if(n)defs, for example. That way, you wouldn't be able to compile
>     code
>     that wouldn't work silently on platforms where that is the case.
>
>
> It would be something worth investigating, definitely.

It would be mad to conditionally compile out code based on the features
or lackthereof of the build machine.

For bits like this, there must be active negotiation between userspace
and the running hypervisor to see what it can support.  Imagine if the
user disabled the monitor trap feature in the BIOS?  Userspace cannot
possibly assume that because it is running on Intel, that the feature is
present and usable.

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

[Razvan Cojocaru]

On 07/06/2015 08:17 PM, Andrew Cooper wrote:
> On 06/07/15 18:08, Lengyel, Tamas wrote:
>>
>>     Having said that (and with the understading that it is beyond the
>>     scope
>>     of this patch), a way to validate things like these is a good idea. I
>>     wonder if, in a future patch, we could not have ./configure detect
>>     these
>>     things and simply disable the relevant VM_EVENT_FLAG constants with
>>     #if(n)defs, for example. That way, you wouldn't be able to compile
>>     code
>>     that wouldn't work silently on platforms where that is the case.
>>
>>
>> It would be something worth investigating, definitely.
> 
> It would be mad to conditionally compile out code based on the features
> or lackthereof of the build machine.
> 
> For bits like this, there must be active negotiation between userspace
> and the running hypervisor to see what it can support.  Imagine if the
> user disabled the monitor trap feature in the BIOS?  Userspace cannot
> possibly assume that because it is running on Intel, that the feature is
> present and usable.

Fair enough, but it would at least compile out the code for machines
where it _definitely_ won't work, and on those where it _might_ work it
would just continue to do nothing silently (or perhaps with the
hypervisor logging an error) once the user disables the monitor trap
flag in the BIOS, so while not perfect it's still something, with the
benefit of less development overhead than a full-on system for
negotiating hypervisor capabilities (which is indeed the safest and
exhaustive course of action).


Just a thought,
Razvan

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

[Lengyel, Tamas]

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

On Mon, Jul 6, 2015 at 1:29 PM, Razvan Cojocaru <rcojocaru@bitdefender.com>
wrote:

> On 07/06/2015 08:17 PM, Andrew Cooper wrote:
> > On 06/07/15 18:08, Lengyel, Tamas wrote:
> >>
> >>     Having said that (and with the understading that it is beyond the
> >>     scope
> >>     of this patch), a way to validate things like these is a good idea.
> I
> >>     wonder if, in a future patch, we could not have ./configure detect
> >>     these
> >>     things and simply disable the relevant VM_EVENT_FLAG constants with
> >>     #if(n)defs, for example. That way, you wouldn't be able to compile
> >>     code
> >>     that wouldn't work silently on platforms where that is the case.
> >>
> >>
> >> It would be something worth investigating, definitely.
> >
> > It would be mad to conditionally compile out code based on the features
> > or lackthereof of the build machine.
> >
> > For bits like this, there must be active negotiation between userspace
> > and the running hypervisor to see what it can support.  Imagine if the
> > user disabled the monitor trap feature in the BIOS?  Userspace cannot
> > possibly assume that because it is running on Intel, that the feature is
> > present and usable.
>
> Fair enough, but it would at least compile out the code for machines
> where it _definitely_ won't work, and on those where it _might_ work it
> would just continue to do nothing silently (or perhaps with the
> hypervisor logging an error) once the user disables the monitor trap
> flag in the BIOS, so while not perfect it's still something, with the
> benefit of less development overhead than a full-on system for
> negotiating hypervisor capabilities (which is indeed the safest and
> exhaustive course of action).
>
> Just a thought,
> Razvan
>

I could see a new XEN_DOMCTL_MONITOR_OP_GET_CAPABILITIES making sense for
this so userspace would get a definite yes/no for features available on the
given platform. If the user then decides to ignore and attempt to use
something not available, that would be on him.

Tamas

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

[Andrew Cooper]

On 06/07/15 18:29, Razvan Cojocaru wrote:
> On 07/06/2015 08:17 PM, Andrew Cooper wrote:
>> On 06/07/15 18:08, Lengyel, Tamas wrote:
>>>     Having said that (and with the understading that it is beyond the
>>>     scope
>>>     of this patch), a way to validate things like these is a good idea. I
>>>     wonder if, in a future patch, we could not have ./configure detect
>>>     these
>>>     things and simply disable the relevant VM_EVENT_FLAG constants with
>>>     #if(n)defs, for example. That way, you wouldn't be able to compile
>>>     code
>>>     that wouldn't work silently on platforms where that is the case.
>>>
>>>
>>> It would be something worth investigating, definitely.
>> It would be mad to conditionally compile out code based on the features
>> or lackthereof of the build machine.
>>
>> For bits like this, there must be active negotiation between userspace
>> and the running hypervisor to see what it can support.  Imagine if the
>> user disabled the monitor trap feature in the BIOS?  Userspace cannot
>> possibly assume that because it is running on Intel, that the feature is
>> present and usable.
> Fair enough, but it would at least compile out the code for machines
> where it _definitely_ won't work, and on those where it _might_ work it
> would just continue to do nothing silently (or perhaps with the
> hypervisor logging an error) once the user disables the monitor trap
> flag in the BIOS, so while not perfect it's still something, with the
> benefit of less development overhead than a full-on system for
> negotiating hypervisor capabilities (which is indeed the safest and
> exhaustive course of action).

That is fine if you are compiling a custom binary for your own use.

It is not fine if you are a distro attempting to provide a binary for
general use by a broad set of users.  Code in tree should cater to this
latter category.

Compile-time feature detection like this is a step backwards from the
1993 days when mechanisms like `cpuid` were introduced.  A one-time
query of available features is nothing like as much overhead as having
to recompile the binary.

~Andrew

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

[Andrew Cooper]

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

On 06/07/15 18:36, Lengyel, Tamas wrote:
>
>
> On Mon, Jul 6, 2015 at 1:29 PM, Razvan Cojocaru
> <rcojocaru@bitdefender.com <mailto:rcojocaru@bitdefender.com>> wrote:
>
>     On 07/06/2015 08:17 PM, Andrew Cooper wrote:
>     > On 06/07/15 18:08, Lengyel, Tamas wrote:
>     >>
>     >>     Having said that (and with the understading that it is
>     beyond the
>     >>     scope
>     >>     of this patch), a way to validate things like these is a
>     good idea. I
>     >>     wonder if, in a future patch, we could not have ./configure
>     detect
>     >>     these
>     >>     things and simply disable the relevant VM_EVENT_FLAG
>     constants with
>     >>     #if(n)defs, for example. That way, you wouldn't be able to
>     compile
>     >>     code
>     >>     that wouldn't work silently on platforms where that is the
>     case.
>     >>
>     >>
>     >> It would be something worth investigating, definitely.
>     >
>     > It would be mad to conditionally compile out code based on the
>     features
>     > or lackthereof of the build machine.
>     >
>     > For bits like this, there must be active negotiation between
>     userspace
>     > and the running hypervisor to see what it can support.  Imagine
>     if the
>     > user disabled the monitor trap feature in the BIOS?  Userspace
>     cannot
>     > possibly assume that because it is running on Intel, that the
>     feature is
>     > present and usable.
>
>     Fair enough, but it would at least compile out the code for machines
>     where it _definitely_ won't work, and on those where it _might_
>     work it
>     would just continue to do nothing silently (or perhaps with the
>     hypervisor logging an error) once the user disables the monitor trap
>     flag in the BIOS, so while not perfect it's still something, with the
>     benefit of less development overhead than a full-on system for
>     negotiating hypervisor capabilities (which is indeed the safest and
>     exhaustive course of action).
>
>     Just a thought,
>     Razvan
>
>
> I could see a new XEN_DOMCTL_MONITOR_OP_GET_CAPABILITIES making sense
> for this so userspace would get a definite yes/no for features
> available on the given platform. If the user then decides to ignore
> and attempt to use something not available, that would be on him.

Very much +1 to this suggestion.

~Andrew

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

[Jan Beulich]

>>> On 06.07.15 at 19:03, <rcojocaru@bitdefender.com> wrote:
> I don't mind just having the comment for now, so for what it's worth I
> stand by my ack.
> 
> Having said that (and with the understading that it is beyond the scope
> of this patch), a way to validate things like these is a good idea. I
> wonder if, in a future patch, we could not have ./configure detect these
> things and simply disable the relevant VM_EVENT_FLAG constants with
> #if(n)defs, for example. That way, you wouldn't be able to compile code
> that wouldn't work silently on platforms where that is the case.

I don't follow: Are you saying this assuming that everyone would
configure and build on the target system? I.e. are you leaving
distros completely out of consideration?

Jan

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

[Razvan Cojocaru]

On 07/07/2015 09:20 AM, Jan Beulich wrote:
>>>> On 06.07.15 at 19:03, <rcojocaru@bitdefender.com> wrote:
>> I don't mind just having the comment for now, so for what it's worth I
>> stand by my ack.
>>
>> Having said that (and with the understading that it is beyond the scope
>> of this patch), a way to validate things like these is a good idea. I
>> wonder if, in a future patch, we could not have ./configure detect these
>> things and simply disable the relevant VM_EVENT_FLAG constants with
>> #if(n)defs, for example. That way, you wouldn't be able to compile code
>> that wouldn't work silently on platforms where that is the case.
> 
> I don't follow: Are you saying this assuming that everyone would
> configure and build on the target system? I.e. are you leaving
> distros completely out of consideration?

I was, as also pointed out by Andrew. Tamas has, in the meantime,
submitted a patch doing this the right way (adding the ability to query
hypervisor capabilities).


Thanks,
Razvan

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

[Jan Beulich]

>>> On 30.06.15 at 16:11, <tlengyel@novetta.com> wrote:
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -6431,6 +6431,17 @@ int hvm_debug_op(struct vcpu *v, int32_t op)
>      return rc;
>  }
>  
> +void hvm_toggle_singlestep(struct vcpu *v)
> +{
> +    ASSERT(atomic_read(&v->pause_count));
> +
> +    /* This feature is only available on Intel CPUs. */
> +    if ( !cpu_has_monitor_trap_flag )

I'd be okay with this if the get-capabilities patch then changed it
to use hvm_is_singlestep_supported(). And the two patches
conflict with one another anyway, so should be put together in
a series with this one being 2nd.

Jan

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

[Lengyel, Tamas]

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

On Tue, Jul 7, 2015 at 5:27 AM, Jan Beulich <JBeulich@suse.com> wrote:

> >>> On 30.06.15 at 16:11, <tlengyel@novetta.com> wrote:
> > --- a/xen/arch/x86/hvm/hvm.c
> > +++ b/xen/arch/x86/hvm/hvm.c
> > @@ -6431,6 +6431,17 @@ int hvm_debug_op(struct vcpu *v, int32_t op)
> >      return rc;
> >  }
> >
> > +void hvm_toggle_singlestep(struct vcpu *v)
> > +{
> > +    ASSERT(atomic_read(&v->pause_count));
> > +
> > +    /* This feature is only available on Intel CPUs. */
> > +    if ( !cpu_has_monitor_trap_flag )
>
> I'd be okay with this if the get-capabilities patch then changed it
> to use hvm_is_singlestep_supported(). And the two patches
> conflict with one another anyway, so should be put together in
> a series with this one being 2nd.
>
> Jan
>

Makes sense, will do so.

Thanks,
Tamas

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

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

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