Re: [Xen-devel] [PATCH v3] xen: Implement hypercall for tracing of program counters

2017-09-01 Thread Jan Beulich 
>>> On 11.08.17 at 17:25,  wrote:
> This commit makes the changes to the hypervisor, the build system as
> well as libxc necessary in order to facilitate tracing of program counters.

There are no libxc changes here afaics.

> 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.

Criteria for how files were chose to (not) be traced should be
stated here, or else no-one can tell whether the ones you picked
make sense. For example, I'm surprised you use a white listing
approach instead of a black listing one.

> --- 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 $@

Please find a better place for this than in the middle of rules.

> --- a/xen/common/Makefile
> +++ b/xen/common/Makefile
> @@ -55,6 +55,8 @@ obj-y += tasklet.o
>  obj-y += time.o
>  obj-y += timer.o
>  obj-y += trace.o
> +obj-y += trace_pc.o
> +obj-$(CONFIG_TRACE_PC) += trace_pc_stub.o

Looking at the sources for these, why do you need two files?
trace_pc.c has a CONFIG_TRACE_PC conditional anyway, so I
see nothing wrong with it gaining a second one.

> +long do_trace_pc(domid_t dom, int mode, unsigned int size,

No plain int-s please unless you really mean a signed quantity.

> + 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);

Any reason not to use the common rcu_lock_domain_by_any_id()?

> +if ( !d )
> +return -ESRCH; /* 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);

What about two simultaneous requests for the same domain?

> --- /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 
> + *
> + * 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 
> .
> + */
> +
> +#ifndef __TRACE_PC_H__
> +#define __TRACE_PC_H__
> +
> +#include 
> +#include 
> +
> +#include 
> +
> +void __sanitizer_cov_trace_pc(void);

Given this single declaration, what do you need the three includes
for?

Jan


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

Re: [Xen-devel] [PATCH v3] xen: Implement hypercall for tracing of program counters

2017-08-16 Thread Wei Liu 
On Fri, Aug 11, 2017 at 05:25:34PM +0200, Felix Schmoll wrote:
> 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 

There are some styling issues in code. I have queued this patch up to
one of my branches and will fix those up.

It will be properly upstreamed once I or someone else gets around to
make the build system up to our task.

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

[Xen-devel] [PATCH v3] xen: Implement hypercall for tracing of program counters

2017-08-11 Thread 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 

---
Changed since v2:
 * Fix bug that hypercall wouldn't return -EFAULT
 * Adjust error return codes of hypercall
 * Add description to Kconfig
 * Move compile-option from Kconfig to Kconfig.debug
 * Formatting changes

---
CC: Andrew Cooper 
CC: George Dunlap 
CC: Ian Jackson 
CC: Jan Beulich 
CC: Konrad Rzeszutek Wilk 
CC: Stefano Stabellini 
CC: Tim Deegan 
CC: Wei Liu 
CC: Julien Grall 
CC: xen-devel@lists.xen.org
---
 xen/Kconfig.debug |  7 +++-
 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/Makefile  |  2 +
 xen/arch/x86/pv/hypercall.c   |  1 +
 xen/common/Makefile   | 13 ++
 xen/common/domain.c   |  4 ++
 xen/common/trace_pc.c | 96 +++
 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 ++
 17 files changed, 253 insertions(+), 1 deletion(-)
 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.debug b/xen/Kconfig.debug
index 689f2974c0..d87dcd78f4 100644
--- a/xen/Kconfig.debug
+++ b/xen/Kconfig.debug
@@ -98,7 +98,6 @@ config PERF_ARRAYS
---help---
  Enables software performance counter array histograms.
 
-
 config VERBOSE_DEBUG
bool "Verbose debug messages"
default DEBUG
@@ -114,6 +113,12 @@ config DEVICE_TREE_DEBUG
  logged in the Xen ring buffer.
  If unsure, say N here.
 
+config TRACE_PC
+bool "Enable pc-tracing"
+default false
+---help---
+ Adds tracing support to the hypervisor (needed for the trace_pc 
hypercall).
+
 endif # DEBUG || EXPERT
 
 endmenu
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/Makefile b/xen/arch/x86/pv/Makefile
index 4e15484471..8c3eccdfd7 100644
--- a/xen/arch/x86/pv/Makefile
+++ b/xen/arch/x86/pv/Makefile
@@ -11,3 +11,5 @@ obj-y += traps.o
 
 obj-bin-y += dom0_build.init.o
 obj-bin-y