[PATCH v2] xen: Implement hypercall for tracing of program counters

[PATCH v2] xen: Implement hypercall for tracing of program counters

[Felix Schmoll]

This commit makes the changes to the hypervisor, the build system as
well as libxc necessary in order to facilitate tracing of program counters.

A discussion of the design can be found in the mailing list:
https://lists.xen.org/archives/html/xen-devel/2017-05/threads.html#02210

The list of files to be included for tracing might still be too extensive,
resulting in indeterministic tracing output for some use cases.

Signed-off-by: Felix Schmoll <eggi.innovations@gmail.com>

---
Changed since v1:
 * Fixed bug that prevented xen from compiling when CONFIG_TRACE_PC was disabled
 * Adapted formatting to coding style
 * Renamed files and variables to conform to trace_pc-idiom
 * Changed hypercall variables to be publicly accessible
 * Added appropriate licenses
 * Fix position of hypercall in hypercall-table
 * Increase tracing coverage
 * Delete buffer if on domain destruction path
---
 xen/Kconfig                   |  4 ++
 xen/Rules.mk                  |  4 ++
 xen/arch/arm/traps.c          |  1 +
 xen/arch/x86/Makefile         |  2 +
 xen/arch/x86/hvm/hypercall.c  |  1 +
 xen/arch/x86/hypercall.c      |  1 +
 xen/arch/x86/pv/hypercall.c   |  1 +
 xen/common/Makefile           | 13 ++++++
 xen/common/domain.c           |  4 ++
 xen/common/trace_pc.c         | 95 +++++++++++++++++++++++++++++++++++++++++++
 xen/common/trace_pc_stub.c    | 39 ++++++++++++++++++
 xen/include/public/trace_pc.h | 38 +++++++++++++++++
 xen/include/public/xen.h      |  1 +
 xen/include/xen/hypercall.h   |  7 ++++
 xen/include/xen/sched.h       |  6 +++
 xen/include/xen/trace_pc.h    | 31 ++++++++++++++
 16 files changed, 248 insertions(+)
 create mode 100644 xen/common/trace_pc.c
 create mode 100644 xen/common/trace_pc_stub.c
 create mode 100644 xen/include/public/trace_pc.h
 create mode 100644 xen/include/xen/trace_pc.h

diff --git a/xen/Kconfig b/xen/Kconfig
index 65d491d776..5ed2c9c390 100644
--- a/xen/Kconfig
+++ b/xen/Kconfig
@@ -38,4 +38,8 @@ config LTO
 
 	  If unsure, say N.
 
+config TRACE_PC
+    bool "Enable tracing e.g. for fuzzing"
+    default false
+
 source "Kconfig.debug"
diff --git a/xen/Rules.mk b/xen/Rules.mk
index 77bcd44922..dde14e3228 100644
--- a/xen/Rules.mk
+++ b/xen/Rules.mk
@@ -170,6 +170,10 @@ clean:: $(addprefix _clean_, $(subdir-all))
 _clean_%/: FORCE
 	$(MAKE) -f $(BASEDIR)/Rules.mk -C $* clean
 
+ifeq ($(CONFIG_TRACE_PC),y)
+$(objs-need-tracing): CFLAGS += -fsanitize-coverage=trace-pc
+endif
+
 %.o: %.c Makefile
 	$(CC) $(CFLAGS) -c $< -o $@
 
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index c07999b518..247a68c964 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1419,6 +1419,7 @@ static arm_hypercall_t arm_hypercall_table[] = {
     HYPERCALL(platform_op, 1),
     HYPERCALL_ARM(vcpu_op, 3),
     HYPERCALL(vm_assist, 2),
+    HYPERCALL(trace_pc, 4),
 };
 
 #ifndef NDEBUG
diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
index 93ead6e5dd..b283c3e22c 100644
--- a/xen/arch/x86/Makefile
+++ b/xen/arch/x86/Makefile
@@ -74,6 +74,8 @@ efi-y := $(shell if [ ! -r $(BASEDIR)/include/xen/compile.h -o \
                       -O $(BASEDIR)/include/xen/compile.h ]; then \
                          echo '$(TARGET).efi'; fi)
 
+objs-need-tracing := cpuid.o hypercall.o
+
 ifneq ($(build_id_linker),)
 notes_phdrs = --notes
 else
diff --git a/xen/arch/x86/hvm/hypercall.c b/xen/arch/x86/hvm/hypercall.c
index e7238ce293..b59d7d481e 100644
--- a/xen/arch/x86/hvm/hypercall.c
+++ b/xen/arch/x86/hvm/hypercall.c
@@ -132,6 +132,7 @@ static const hypercall_table_t hvm_hypercall_table[] = {
     COMPAT_CALL(mmuext_op),
     HYPERCALL(xenpmu_op),
     COMPAT_CALL(dm_op),
+    HYPERCALL(trace_pc),
     HYPERCALL(arch_1)
 };
 
diff --git a/xen/arch/x86/hypercall.c b/xen/arch/x86/hypercall.c
index e30181817a..672ffe7ef5 100644
--- a/xen/arch/x86/hypercall.c
+++ b/xen/arch/x86/hypercall.c
@@ -68,6 +68,7 @@ const hypercall_args_t hypercall_args_table[NR_hypercalls] =
     ARGS(xenpmu_op, 2),
     ARGS(dm_op, 3),
     ARGS(mca, 1),
+    ARGS(trace_pc, 4),
     ARGS(arch_1, 1),
 };
 
diff --git a/xen/arch/x86/pv/hypercall.c b/xen/arch/x86/pv/hypercall.c
index f79f7eef62..13eb2e86a2 100644
--- a/xen/arch/x86/pv/hypercall.c
+++ b/xen/arch/x86/pv/hypercall.c
@@ -80,6 +80,7 @@ static const hypercall_table_t pv_hypercall_table[] = {
     HYPERCALL(xenpmu_op),
     COMPAT_CALL(dm_op),
     HYPERCALL(mca),
+    HYPERCALL(trace_pc),
     HYPERCALL(arch_1),
 };
 
diff --git a/xen/common/Makefile b/xen/common/Makefile
index 26c5a64337..7f7345cb90 100644
--- a/xen/common/Makefile
+++ b/xen/common/Makefile
@@ -6,6 +6,8 @@ obj-y += cpupool.o
 obj-$(CONFIG_HAS_DEVICE_TREE) += device_tree.o
 obj-y += domctl.o
 obj-y += domain.o
+obj-y += trace_pc.o
+obj-$(CONFIG_TRACE_PC) += trace_pc_stub.o
 obj-y += event_2l.o
 obj-y += event_channel.o
 obj-y += event_fifo.o
@@ -80,3 +82,14 @@ subdir-$(CONFIG_GCOV) += gcov
 
 subdir-y += libelf
 subdir-$(CONFIG_HAS_DEVICE_TREE) += libfdt
+
+objs-need-tracing := bsearch.o \
+    decompress.o device_tree.o domain.o domctl.o earlycpio.o grant_table.o \
+    guestcopy.o gunzip.o inflate.o kernel.o kexec.o keyhandler.o kimage.o \
+    lib.o livepatch.o lzo.o mem_access.o memory.o multicall.o notifier.o \
+    page_alloc.o pdx.o perfc.o radix_tree.o rangeset.o \
+    rbtree.o shutdown.o sort.o stop_machine.o \
+    symbols.o symbols-dummy.o sysctl.o time.o tmem.o \
+    tmem_control.o tmem_xen.o trace.o unlz4.o unlzo.o unxz.o version.o \
+    virtual_region.o vmap.o vm_event.o warning.o xenoprof.o \
+    xmalloc_tlsf.o
diff --git a/xen/common/domain.c b/xen/common/domain.c
index b22aacc57e..c98a0a94ec 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -874,6 +874,10 @@ void domain_destroy(struct domain *d)
     rcu_assign_pointer(*pd, d->next_in_hashbucket);
     spin_unlock(&domlist_update_lock);
 
+#ifdef CONFIG_TRACE_PC
+    xfree(d->tracing_buffer);
+#endif
+
     /* Schedule RCU asynchronous completion of domain destroy. */
     call_rcu(&d->rcu, complete_domain_destroy);
 }
diff --git a/xen/common/trace_pc.c b/xen/common/trace_pc.c
new file mode 100644
index 0000000000..780bbc0660
--- /dev/null
+++ b/xen/common/trace_pc.c
@@ -0,0 +1,95 @@
+/******************************************************************************
+ * trace_pc.c
+ *
+ * Implementation of the program counter tracing hypercall.
+ *
+ * Copyright (c) 2017 Felix Schmoll <eggi.innovations@gmail.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 that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public
+ * License along with this program; If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <xen/xmalloc.h>
+#include <xen/guest_access.h>
+#include <xen/sched.h>
+#include <public/trace_pc.h>
+
+long do_trace_pc(domid_t dom, int mode, unsigned int size,
+                 XEN_GUEST_HANDLE_PARAM(uint64_t) buf)
+{
+#ifdef CONFIG_TRACE_PC
+    int ret = 0;
+    struct domain *d;
+
+    if ( dom == DOMID_SELF )
+        d = current->domain;
+    else
+        d = get_domain_by_id(dom);
+
+    if ( !d )
+        return -EINVAL; /* invalid domain */
+
+    switch ( mode )
+    {
+    case XEN_TRACE_PC_START:
+    {
+        if ( d->tracing_buffer )
+        {
+            ret = -EBUSY; /* domain already being traced */
+            break;
+        }
+
+        d->tracing_buffer_pos = 0;
+        d->tracing_buffer_size = size;
+        d->tracing_buffer = xmalloc_array(uint64_t, size);
+
+        if ( !d->tracing_buffer )
+            ret = -ENOMEM;
+        break;
+    }
+
+    case XEN_TRACE_PC_STOP:
+    {
+        uint64_t* temp = d->tracing_buffer;
+        d->tracing_buffer = NULL;
+
+        if ( copy_to_guest(buf, temp, d->tracing_buffer_pos) )
+            ret = -EFAULT;
+
+        xfree(temp);
+
+        ret = d->tracing_buffer_pos;
+        break;
+    }
+
+    default:
+        ret = -ENOSYS;
+    }
+
+    if ( dom != DOMID_SELF )
+        put_domain(d);
+
+    return ret;
+#else
+    return 0;
+#endif
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/common/trace_pc_stub.c b/xen/common/trace_pc_stub.c
new file mode 100644
index 0000000000..4aba7dba9f
--- /dev/null
+++ b/xen/common/trace_pc_stub.c
@@ -0,0 +1,39 @@
+/******************************************************************************
+ * trace_pc_stub.c
+ *
+ * Edge function/stub for the program counter tracing hypercall.
+ *
+ * Copyright (c) 2017 Felix Schmoll <eggi.innovations@gmail.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 that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public
+ * License along with this program; If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <xen/trace_pc.h>
+#include <xen/kernel.h>
+
+void __sanitizer_cov_trace_pc(void)
+{
+    struct domain *d;
+
+    if ( system_state < SYS_STATE_active )
+        return;
+
+    d = current->domain;
+
+    if ( d->tracing_buffer &&
+        (d->tracing_buffer_pos < d->tracing_buffer_size) )
+    {
+        d->tracing_buffer[d->tracing_buffer_pos++] =
+            (uint64_t) __builtin_return_address(0);
+    }
+}
diff --git a/xen/include/public/trace_pc.h b/xen/include/public/trace_pc.h
new file mode 100644
index 0000000000..54e430a561
--- /dev/null
+++ b/xen/include/public/trace_pc.h
@@ -0,0 +1,38 @@
+/******************************************************************************
+ * trace_pc.h
+ *
+ * Macros for program counter tracing hypercall.
+ *
+ * Copyright (C) 2017 Felix Schmoll <eggi.innovations@gmail.com>
+ *
+ * Permission is hereby granted, free of charge, to any person
+ * obtaining a copy of this software and associated documentation
+ * files (the "Software"), to deal in the Software without restriction,
+ * including without limitation the rights to use, copy, modify, merge,
+ * publish, distribute, sublicense, and/or sell copies of the Software,
+ * and to permit persons to whom the Software is furnished to do so,
+ * subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be
+ * included in all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
+ * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+ * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.
+ * IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY
+ * CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT,
+ * TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE
+ * SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
+ */
+
+#ifndef __XEN_PUBLIC_TRACE_PC_H__
+#define __XEN_PUBLIC_TRACE_PC_H__
+
+#if defined(__XEN__) || defined(__XEN_TOOLS__)
+
+#define XEN_TRACE_PC_START 0
+#define XEN_TRACE_PC_STOP 1
+
+#endif /* defined(__XEN__) || defined(__XEN_TOOLS__) */
+
+#endif /* __XEN_PUBLIC_TRACE_PC_H__ */
diff --git a/xen/include/public/xen.h b/xen/include/public/xen.h
index 2ac6b1e24d..95d83c21ce 100644
--- a/xen/include/public/xen.h
+++ b/xen/include/public/xen.h
@@ -121,6 +121,7 @@ DEFINE_XEN_GUEST_HANDLE(xen_ulong_t);
 #define __HYPERVISOR_xc_reserved_op       39 /* reserved for XenClient */
 #define __HYPERVISOR_xenpmu_op            40
 #define __HYPERVISOR_dm_op                41
+#define __HYPERVISOR_trace_pc             42
 
 /* Architecture-specific hypercall definitions. */
 #define __HYPERVISOR_arch_0               48
diff --git a/xen/include/xen/hypercall.h b/xen/include/xen/hypercall.h
index cc99aea57d..aa6269e7b7 100644
--- a/xen/include/xen/hypercall.h
+++ b/xen/include/xen/hypercall.h
@@ -83,6 +83,13 @@ do_xen_version(
     XEN_GUEST_HANDLE_PARAM(void) arg);
 
 extern long
+do_trace_pc(
+    domid_t dom_id,
+    int mode,
+    unsigned int size,
+    XEN_GUEST_HANDLE_PARAM(uint64_t) buf);
+
+extern long
 do_console_io(
     int cmd,
     int count,
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index 6673b27d88..4bd3fe2417 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -483,6 +483,12 @@ struct domain
         unsigned int guest_request_enabled       : 1;
         unsigned int guest_request_sync          : 1;
     } monitor;
+
+#ifdef CONFIG_TRACE_PC
+    uint64_t* tracing_buffer;
+    unsigned int tracing_buffer_pos;
+    unsigned int tracing_buffer_size;
+#endif
 };
 
 /* Protect updates/reads (resp.) of domain_list and domain_hash. */
diff --git a/xen/include/xen/trace_pc.h b/xen/include/xen/trace_pc.h
new file mode 100644
index 0000000000..631815de30
--- /dev/null
+++ b/xen/include/xen/trace_pc.h
@@ -0,0 +1,31 @@
+/******************************************************************************
+ * trace_pc.h
+ *
+ * Declarations for the program counter tracing hypercall
+ *
+ * Copyright (C) 2017 Felix Schmoll <eggi.innovations@gmail.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 that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public
+ * License along with this program; If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef __TRACE_PC_H__
+#define __TRACE_PC_H__
+
+#include <xen/sched.h>
+#include <xen/types.h>
+
+#include <asm/current.h>
+
+void __sanitizer_cov_trace_pc(void);
+
+#endif /* __TRACE_PC_H__ */
-- 
2.11.0


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

Re: [PATCH v2] xen: Implement hypercall for tracing of program counters

[Wei Liu]

On Wed, Jul 26, 2017 at 12:43:45PM +0200, Felix Schmoll wrote:
> 
> diff --git a/xen/Kconfig b/xen/Kconfig
> index 65d491d776..5ed2c9c390 100644
> --- a/xen/Kconfig
> +++ b/xen/Kconfig
> @@ -38,4 +38,8 @@ config LTO
>  
>  	  If unsure, say N.
>  
> +config TRACE_PC
> +    bool "Enable tracing e.g. for fuzzing"
> +    default false

Could use a paragraph to describe what it does. And it probably belongs
to Kconfig.debug.

> +
>  source "Kconfig.debug"
> diff --git a/xen/Rules.mk b/xen/Rules.mk
> index 77bcd44922..dde14e3228 100644
> --- a/xen/Rules.mk
> +++ b/xen/Rules.mk
> @@ -170,6 +170,10 @@ clean:: $(addprefix _clean_, $(subdir-all))
>  _clean_%/: FORCE
>  	$(MAKE) -f $(BASEDIR)/Rules.mk -C $* clean
>  
[...]
>  
> diff --git a/xen/arch/x86/pv/hypercall.c b/xen/arch/x86/pv/hypercall.c
> index f79f7eef62..13eb2e86a2 100644
> --- a/xen/arch/x86/pv/hypercall.c
> +++ b/xen/arch/x86/pv/hypercall.c
> @@ -80,6 +80,7 @@ static const hypercall_table_t pv_hypercall_table[] = {
>      HYPERCALL(xenpmu_op),
>      COMPAT_CALL(dm_op),
>      HYPERCALL(mca),
> +    HYPERCALL(trace_pc),
>      HYPERCALL(arch_1),
>  };
>  
> diff --git a/xen/common/Makefile b/xen/common/Makefile
> index 26c5a64337..7f7345cb90 100644
> --- a/xen/common/Makefile
> +++ b/xen/common/Makefile
> @@ -6,6 +6,8 @@ obj-y += cpupool.o
>  obj-$(CONFIG_HAS_DEVICE_TREE) += device_tree.o
>  obj-y += domctl.o
>  obj-y += domain.o
> +obj-y += trace_pc.o
> +obj-$(CONFIG_TRACE_PC) += trace_pc_stub.o

The list of objects is sorted alphabetically. You need to move the new
objects to their right location.

>  obj-y += event_2l.o
>  obj-y += event_channel.o
>  obj-y += event_fifo.o
> @@ -80,3 +82,14 @@ subdir-$(CONFIG_GCOV) += gcov
>  
>  subdir-y += libelf
>  subdir-$(CONFIG_HAS_DEVICE_TREE) += libfdt
[...]
> +#include <xen/xmalloc.h>
> +#include <xen/guest_access.h>
> +#include <xen/sched.h>

Sort these alphabetically.

> +#include <public/trace_pc.h>
> +
> +long do_trace_pc(domid_t dom, int mode, unsigned int size,
> +                 XEN_GUEST_HANDLE_PARAM(uint64_t) buf)
> +{
> +#ifdef CONFIG_TRACE_PC
> +    int ret = 0;
> +    struct domain *d;
> +
> +    if ( dom == DOMID_SELF )
> +        d = current->domain;
> +    else
> +        d = get_domain_by_id(dom);
> +
> +    if ( !d )
> +        return -EINVAL; /* invalid domain */
> +
> +    switch ( mode )
> +    {
> +    case XEN_TRACE_PC_START:
> +    {
> +        if ( d->tracing_buffer )
> +        {
> +            ret = -EBUSY; /* domain already being traced */
> +            break;
> +        }
> +
> +        d->tracing_buffer_pos = 0;
> +        d->tracing_buffer_size = size;
> +        d->tracing_buffer = xmalloc_array(uint64_t, size);
> +
> +        if ( !d->tracing_buffer )
> +            ret = -ENOMEM;
> +        break;
> +    }
> +
> +    case XEN_TRACE_PC_STOP:
> +    {
> +        uint64_t* temp = d->tracing_buffer;

uint64_t *temp;

> +        d->tracing_buffer = NULL;
> +
> +        if ( copy_to_guest(buf, temp, d->tracing_buffer_pos) )
> +            ret = -EFAULT;
> +
> +        xfree(temp);
> +
> +        ret = d->tracing_buffer_pos;
> +        break;
> +    }
> +
> +    default:
> +        ret = -ENOSYS;

EINVAL

> +    }
> +
> +    if ( dom != DOMID_SELF )
> +        put_domain(d);
> +
> +    return ret;
> +#else
> +    return 0;

return -EOPNOTSUPP;

> +#endif
> +}
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * tab-width: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/xen/common/trace_pc_stub.c b/xen/common/trace_pc_stub.c
> new file mode 100644
> index 0000000000..4aba7dba9f
> --- /dev/null
> +++ b/xen/common/trace_pc_stub.c
> @@ -0,0 +1,39 @@
> +/******************************************************************************
> + * trace_pc_stub.c
> + *
> + * Edge function/stub for the program counter tracing hypercall.
> + *
> + * Copyright (c) 2017 Felix Schmoll <eggi.innovations@gmail.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 that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public
> + * License along with this program; If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <xen/trace_pc.h>
> +#include <xen/kernel.h>
> +
> +void __sanitizer_cov_trace_pc(void)
> +{
> +    struct domain *d;
> +
> +    if ( system_state < SYS_STATE_active )
> +        return;
> +
> +    d = current->domain;
> +
> +    if ( d->tracing_buffer &&
> +        (d->tracing_buffer_pos < d->tracing_buffer_size) )
> +    {
> +        d->tracing_buffer[d->tracing_buffer_pos++] =
> +            (uint64_t) __builtin_return_address(0);
> +    }
> +}
> diff --git a/xen/include/public/trace_pc.h b/xen/include/public/trace_pc.h
> new file mode 100644
> index 0000000000..54e430a561
> --- /dev/null
> +++ b/xen/include/public/trace_pc.h
> @@ -0,0 +1,38 @@
> +/******************************************************************************
> + * trace_pc.h
> + *
> + * Macros for program counter tracing hypercall.
> + *
> + * Copyright (C) 2017 Felix Schmoll <eggi.innovations@gmail.com>
> + *
> + * Permission is hereby granted, free of charge, to any person
> + * obtaining a copy of this software and associated documentation
> + * files (the "Software"), to deal in the Software without restriction,
> + * including without limitation the rights to use, copy, modify, merge,
> + * publish, distribute, sublicense, and/or sell copies of the Software,
> + * and to permit persons to whom the Software is furnished to do so,
> + * subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be
> + * included in all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> + * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> + * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.
> + * IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY
> + * CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT,
> + * TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE
> + * SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
> + */
> +
> +#ifndef __XEN_PUBLIC_TRACE_PC_H__
> +#define __XEN_PUBLIC_TRACE_PC_H__
> +
> +#if defined(__XEN__) || defined(__XEN_TOOLS__)
> +
> +#define XEN_TRACE_PC_START 0
> +#define XEN_TRACE_PC_STOP 1
> +
> +#endif /* defined(__XEN__) || defined(__XEN_TOOLS__) */
> +
> +#endif /* __XEN_PUBLIC_TRACE_PC_H__ */
> diff --git a/xen/include/public/xen.h b/xen/include/public/xen.h
> index 2ac6b1e24d..95d83c21ce 100644
> --- a/xen/include/public/xen.h
> +++ b/xen/include/public/xen.h
> @@ -121,6 +121,7 @@ DEFINE_XEN_GUEST_HANDLE(xen_ulong_t);
>  #define __HYPERVISOR_xc_reserved_op       39 /* reserved for XenClient */
>  #define __HYPERVISOR_xenpmu_op            40
>  #define __HYPERVISOR_dm_op                41
> +#define __HYPERVISOR_trace_pc             42
>  
>  /* Architecture-specific hypercall definitions. */
>  #define __HYPERVISOR_arch_0               48
> diff --git a/xen/include/xen/hypercall.h b/xen/include/xen/hypercall.h
> index cc99aea57d..aa6269e7b7 100644
> --- a/xen/include/xen/hypercall.h
> +++ b/xen/include/xen/hypercall.h
> @@ -83,6 +83,13 @@ do_xen_version(
>      XEN_GUEST_HANDLE_PARAM(void) arg);
>  
>  extern long
> +do_trace_pc(
> +    domid_t dom_id,
> +    int mode,
> +    unsigned int size,
> +    XEN_GUEST_HANDLE_PARAM(uint64_t) buf);
> +
> +extern long
>  do_console_io(
>      int cmd,
>      int count,
> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
> index 6673b27d88..4bd3fe2417 100644
> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -483,6 +483,12 @@ struct domain
>          unsigned int guest_request_enabled       : 1;
>          unsigned int guest_request_sync          : 1;
>      } monitor;
> +
> +#ifdef CONFIG_TRACE_PC
> +    uint64_t* tracing_buffer;
> +    unsigned int tracing_buffer_pos;
> +    unsigned int tracing_buffer_size;
> +#endif
>  };
>  
>  /* Protect updates/reads (resp.) of domain_list and domain_hash. */
> diff --git a/xen/include/xen/trace_pc.h b/xen/include/xen/trace_pc.h
> new file mode 100644
> index 0000000000..631815de30
> --- /dev/null
> +++ b/xen/include/xen/trace_pc.h
> @@ -0,0 +1,31 @@
> +/******************************************************************************
> + * trace_pc.h
> + *
> + * Declarations for the program counter tracing hypercall
> + *
> + * Copyright (C) 2017 Felix Schmoll <eggi.innovations@gmail.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 that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public
> + * License along with this program; If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#ifndef __TRACE_PC_H__
> +#define __TRACE_PC_H__
> +
> +#include <xen/sched.h>
> +#include <xen/types.h>
> +
> +#include <asm/current.h>
> +
> +void __sanitizer_cov_trace_pc(void);
> +
> +#endif /* __TRACE_PC_H__ */
> -- 
> 2.11.0
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> https://lists.xen.org/xen-devel

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

Re: [PATCH v2] xen: Implement hypercall for tracing of program counters

[Felix Schmoll]

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

Thanks. Will do.

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

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

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

Re: [PATCH v2] xen: Implement hypercall for tracing of program counters

[Julien Grall]

Hi,

On 07/28/2017 04:37 PM, Wei Liu wrote:
> On Wed, Jul 26, 2017 at 12:43:45PM +0200, Felix Schmoll wrote:
>> +        d->tracing_buffer = NULL;
>> +
>> +        if ( copy_to_guest(buf, temp, d->tracing_buffer_pos) )
>> +            ret = -EFAULT;
>> +
>> +        xfree(temp);
>> +
>> +        ret = d->tracing_buffer_pos;
>> +        break;
>> +    }
>> +
>> +    default:
>> +        ret = -ENOSYS;
> 
> EINVAL

Should not it be -EOPNOTSUPP to match return error when CONFIG_TRACE_PC 
is not?

Cheers,

-- 
Julien Grall

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

Re: [PATCH v2] xen: Implement hypercall for tracing of program counters

[Wei Liu]

On Mon, Jul 31, 2017 at 09:22:35AM +0100, Julien Grall wrote:
> Hi,
> 
> On 07/28/2017 04:37 PM, Wei Liu wrote:
> > On Wed, Jul 26, 2017 at 12:43:45PM +0200, Felix Schmoll wrote:
> > > +        d->tracing_buffer = NULL;
> > > +
> > > +        if ( copy_to_guest(buf, temp, d->tracing_buffer_pos) )
> > > +            ret = -EFAULT;
> > > +
> > > +        xfree(temp);
> > > +
> > > +        ret = d->tracing_buffer_pos;
> > > +        break;
> > > +    }
> > > +
> > > +    default:
> > > +        ret = -ENOSYS;
> > 
> > EINVAL
> 
> Should not it be -EOPNOTSUPP to match return error when CONFIG_TRACE_PC is
> not?

AIUI EOPNOTSUPP means "This is a valid operation but I am not configured
to support it" while EINVAL means "This is an invalid value
(operation)".

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

Re: [PATCH v2] xen: Implement hypercall for tracing of program counters

[Julien Grall]

On 31/07/17 10:56, Wei Liu wrote:
> On Mon, Jul 31, 2017 at 09:22:35AM +0100, Julien Grall wrote:
>> Hi,
>>
>> On 07/28/2017 04:37 PM, Wei Liu wrote:
>>> On Wed, Jul 26, 2017 at 12:43:45PM +0200, Felix Schmoll wrote:
>>>> +        d->tracing_buffer = NULL;
>>>> +
>>>> +        if ( copy_to_guest(buf, temp, d->tracing_buffer_pos) )
>>>> +            ret = -EFAULT;
>>>> +
>>>> +        xfree(temp);
>>>> +
>>>> +        ret = d->tracing_buffer_pos;
>>>> +        break;
>>>> +    }
>>>> +
>>>> +    default:
>>>> +        ret = -ENOSYS;
>>>
>>> EINVAL
>>
>> Should not it be -EOPNOTSUPP to match return error when CONFIG_TRACE_PC is
>> not?
>
> AIUI EOPNOTSUPP means "This is a valid operation but I am not configured
> to support it" while EINVAL means "This is an invalid value
> (operation)".

Fair enough. However, you impose the caller to check -EINVAL and 
-EOPNOTSUPP in order to know if an operation can be done.

I first thought all the other hypercalls use -EOPNOTSUPP, but in face 
they use -ENOSYS. It would be to be consistent with the rest rather than 
reinventing our own.

-- 
Julien Grall

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

Re: [PATCH v2] xen: Implement hypercall for tracing of program counters

[Wei Liu]

On Mon, Jul 31, 2017 at 11:13:33AM +0100, Julien Grall wrote:
> 
> 
> On 31/07/17 10:56, Wei Liu wrote:
> > On Mon, Jul 31, 2017 at 09:22:35AM +0100, Julien Grall wrote:
> > > Hi,
> > > 
> > > On 07/28/2017 04:37 PM, Wei Liu wrote:
> > > > On Wed, Jul 26, 2017 at 12:43:45PM +0200, Felix Schmoll wrote:
> > > > > +        d->tracing_buffer = NULL;
> > > > > +
> > > > > +        if ( copy_to_guest(buf, temp, d->tracing_buffer_pos) )
> > > > > +            ret = -EFAULT;
> > > > > +
> > > > > +        xfree(temp);
> > > > > +
> > > > > +        ret = d->tracing_buffer_pos;
> > > > > +        break;
> > > > > +    }
> > > > > +
> > > > > +    default:
> > > > > +        ret = -ENOSYS;
> > > > 
> > > > EINVAL
> > > 
> > > Should not it be -EOPNOTSUPP to match return error when CONFIG_TRACE_PC is
> > > not?
> > 
> > AIUI EOPNOTSUPP means "This is a valid operation but I am not configured
> > to support it" while EINVAL means "This is an invalid value
> > (operation)".
> 
> Fair enough. However, you impose the caller to check -EINVAL and -EOPNOTSUPP
> in order to know if an operation can be done.
> 
> I first thought all the other hypercalls use -EOPNOTSUPP, but in face they
> use -ENOSYS. It would be to be consistent with the rest rather than
> reinventing our own.
> 

Oh, if that is already an established convention, I think using ENOSYS
is fine. Felix, please keep it as ENOSYS.

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

Re: [PATCH v2] xen: Implement hypercall for tracing of program counters

[Ian Jackson]

Wei Liu writes ("Re: [Xen-devel] [PATCH v2] xen: Implement hypercall for tracing of program counters"):
> On Mon, Jul 31, 2017 at 09:22:35AM +0100, Julien Grall wrote:
..
> > Should not it be -EOPNOTSUPP to match return error when CONFIG_TRACE_PC is
> > not?
> 
> AIUI EOPNOTSUPP means "This is a valid operation but I am not configured
> to support it" while EINVAL means "This is an invalid value
> (operation)".

EOPNOTSUPP means "someone somewhere might think this is valid, but I
don't understand it".  It can be used, for example, for unknown
operation numbers.

"ENOSYS" is used in exactly the same situation, but where the enum
whose value is not understood is precisely the syscall number.  In the
context of the hypervisor I think ENOSYS is used for "unknown
hypercall number".  I haven't checked whether it is used for "unknown
operation number" but I suspect that the hypervisor users EOPNOTSUPP
for that.

It would be sensible if hypervisor maintainers were to write this
stuff down somewhere.

EINVAL means "I definitely know that this is invalid".  It should
rarely be used for an unknown value of an enum, since enums can gain
new values in future implementations.  It can be used for "value out
of range" or "I understand this combination of parameters, but it is
not meaningful".  It should not be used for "I understand this
combination of parameters, and I do not implement it, even though in
principle the combination might somehow be implemented in the future".

In this particular case I suspect that EOPNOTSUPP is right.  EINVAL is
clearly wrong.

While looking at the original patch, I saw this:

> +    if ( !d )
> +        return -EINVAL; /* invalid domain */

Is this conventional ?  EINVAL is a remarkably unhelpful error code
for this case.  IMO the hypervisor ought to have a dedicated error
code for "referenced domain does not exist".  But maybe it doesn't.

Ian.

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

Re: [PATCH v2] xen: Implement hypercall for tracing of program counters

[Paul Durrant]

> -----Original Message-----
> From: Xen-devel [mailto:xen-devel-bounces@lists.xen.org] On Behalf Of Ian
> Jackson
> Sent: 31 July 2017 12:14
> To: Wei Liu <wei.liu2@citrix.com>
> Cc: sstabellini@kernel.org; Felix Schmoll <eggi.innovations@gmail.com>;
> Andrew Cooper <Andrew.Cooper3@citrix.com>; Julien Grall
> <julien.grall@arm.com>; jbeulich@suse.com; xen-
> devel@lists.xenproject.org
> Subject: Re: [Xen-devel] [PATCH v2] xen: Implement hypercall for tracing of
> program counters
> 
> Wei Liu writes ("Re: [Xen-devel] [PATCH v2] xen: Implement hypercall for
> tracing of program counters"):
> > On Mon, Jul 31, 2017 at 09:22:35AM +0100, Julien Grall wrote:
> ..
> > > Should not it be -EOPNOTSUPP to match return error when
> CONFIG_TRACE_PC is
> > > not?
> >
> > AIUI EOPNOTSUPP means "This is a valid operation but I am not configured
> > to support it" while EINVAL means "This is an invalid value
> > (operation)".
> 
> EOPNOTSUPP means "someone somewhere might think this is valid, but I
> don't understand it".  It can be used, for example, for unknown
> operation numbers.
> 
> "ENOSYS" is used in exactly the same situation, but where the enum
> whose value is not understood is precisely the syscall number.  In the
> context of the hypervisor I think ENOSYS is used for "unknown
> hypercall number".  I haven't checked whether it is used for "unknown
> operation number" but I suspect that the hypervisor users EOPNOTSUPP
> for that.

Nope. It's ENOSYS for that case too (certainly for hvm and memory ops... which is what I checked).

  Paul

> 
> It would be sensible if hypervisor maintainers were to write this
> stuff down somewhere.
> 
> EINVAL means "I definitely know that this is invalid".  It should
> rarely be used for an unknown value of an enum, since enums can gain
> new values in future implementations.  It can be used for "value out
> of range" or "I understand this combination of parameters, but it is
> not meaningful".  It should not be used for "I understand this
> combination of parameters, and I do not implement it, even though in
> principle the combination might somehow be implemented in the future".
> 
> In this particular case I suspect that EOPNOTSUPP is right.  EINVAL is
> clearly wrong.
> 
> While looking at the original patch, I saw this:
> 
> > +    if ( !d )
> > +        return -EINVAL; /* invalid domain */
> 
> Is this conventional ?  EINVAL is a remarkably unhelpful error code
> for this case.  IMO the hypervisor ought to have a dedicated error
> code for "referenced domain does not exist".  But maybe it doesn't.
> 
> Ian.
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> https://lists.xen.org/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2] xen: Implement hypercall for tracing of program counters

[Andrew Cooper]

On 31/07/17 12:58, Paul Durrant wrote:
>> -----Original Message-----
>> From: Xen-devel [mailto:xen-devel-bounces@lists.xen.org] On Behalf Of Ian
>> Jackson
>> Sent: 31 July 2017 12:14
>> To: Wei Liu <wei.liu2@citrix.com>
>> Cc: sstabellini@kernel.org; Felix Schmoll <eggi.innovations@gmail.com>;
>> Andrew Cooper <Andrew.Cooper3@citrix.com>; Julien Grall
>> <julien.grall@arm.com>; jbeulich@suse.com; xen-
>> devel@lists.xenproject.org
>> Subject: Re: [Xen-devel] [PATCH v2] xen: Implement hypercall for tracing of
>> program counters
>>
>> Wei Liu writes ("Re: [Xen-devel] [PATCH v2] xen: Implement hypercall for
>> tracing of program counters"):
>>> On Mon, Jul 31, 2017 at 09:22:35AM +0100, Julien Grall wrote:
>> ..
>>>> Should not it be -EOPNOTSUPP to match return error when
>> CONFIG_TRACE_PC is
>>>> not?
>>> AIUI EOPNOTSUPP means "This is a valid operation but I am not configured
>>> to support it" while EINVAL means "This is an invalid value
>>> (operation)".
>> EOPNOTSUPP means "someone somewhere might think this is valid, but I
>> don't understand it".  It can be used, for example, for unknown
>> operation numbers.
>>
>> "ENOSYS" is used in exactly the same situation, but where the enum
>> whose value is not understood is precisely the syscall number.  In the
>> context of the hypervisor I think ENOSYS is used for "unknown
>> hypercall number".  I haven't checked whether it is used for "unknown
>> operation number" but I suspect that the hypervisor users EOPNOTSUPP
>> for that.
> Nope. It's ENOSYS for that case too (certainly for hvm and memory ops... which is what I checked).

History has been poor to us.  Quite a few of the ENOSYS should be
EOPNOTSUPP, but we can't change them for ABI reasons.

>
>   Paul
>
>> It would be sensible if hypervisor maintainers were to write this
>> stuff down somewhere.
>>
>> EINVAL means "I definitely know that this is invalid".  It should
>> rarely be used for an unknown value of an enum, since enums can gain
>> new values in future implementations.  It can be used for "value out
>> of range" or "I understand this combination of parameters, but it is
>> not meaningful".  It should not be used for "I understand this
>> combination of parameters, and I do not implement it, even though in
>> principle the combination might somehow be implemented in the future".
>>
>> In this particular case I suspect that EOPNOTSUPP is right.  EINVAL is
>> clearly wrong.
>>
>> While looking at the original patch, I saw this:
>>
>>> +    if ( !d )
>>> +        return -EINVAL; /* invalid domain */
>> Is this conventional ?  EINVAL is a remarkably unhelpful error code
>> for this case.  IMO the hypervisor ought to have a dedicated error
>> code for "referenced domain does not exist".  But maybe it doesn't.

ESRCH is "no such domain".  We also use ENOENT for "no such vcpu".

~Andrew

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

Re: [PATCH v2] xen: Implement hypercall for tracing of program counters

[Jan Beulich]

>>> Andrew Cooper <andrew.cooper3@citrix.com> 07/31/17 2:01 PM >>>
>On 31/07/17 12:58, Paul Durrant wrote:
>> From: Xen-devel [mailto:xen-devel-bounces@lists.xen.org] On Behalf Of Ian
>> Jackson
>> Sent: 31 July 2017 12:14
>>> Wei Liu writes ("Re: [Xen-devel] [PATCH v2] xen: Implement hypercall for
>>> tracing of program counters"):
>>>> On Mon, Jul 31, 2017 at 09:22:35AM +0100, Julien Grall wrote:
>>>>> Should not it be -EOPNOTSUPP to match return error when
>>> CONFIG_TRACE_PC is
>>>>> not?
>>>> AIUI EOPNOTSUPP means "This is a valid operation but I am not configured
>>>> to support it" while EINVAL means "This is an invalid value
>>>> (operation)".
>>> EOPNOTSUPP means "someone somewhere might think this is valid, but I
>>> don't understand it".  It can be used, for example, for unknown
>>> operation numbers.
>>>
>>> "ENOSYS" is used in exactly the same situation, but where the enum
>>> whose value is not understood is precisely the syscall number.  In the
>>> context of the hypervisor I think ENOSYS is used for "unknown
>>> hypercall number".  I haven't checked whether it is used for "unknown
>>> operation number" but I suspect that the hypervisor users EOPNOTSUPP
>>> for that.
>> Nope. It's ENOSYS for that case too (certainly for hvm and memory ops... which is what I checked).
>
>History has been poor to us.  Quite a few of the ENOSYS should be
>EOPNOTSUPP, but we can't change them for ABI reasons.

Right, but the result here ought to be that we at least don't introduce any
new bogus uses of ENOSYS or EINVAL.

Jan


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