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

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Wed, Jun 21, 2017 at 09:20:20AM +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. It is also
> not clear what other causes of indeterminism there might be.

Now you've got everything you need, you can check the IP against known
binary to gradually remove the indeterminism.

I've only skim-read your patch. It looks to be in line with what we
discussed before.

The only blocker to get this merge (not this exact patch, but the
approach in general), as I see it, is the build system.  But then I'm
not really comfortable to ask you to work on that. So if you want to
move on to the next step, that would be fine by me.

> 
> Signed-off-by: Felix Schmoll <eggi.innovations@xxxxxxxxx>
> ---
>  tools/libxc/include/xenctrl.h |  2 ++
>  tools/libxc/xc_private.c      | 22 +++++++++++++
>  tools/libxc/xc_private.h      |  8 +++++
>  xen/Kconfig                   |  4 +++
>  xen/Rules.mk                  |  4 +++
>  xen/arch/arm/traps.c          |  1 +
>  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/edge_trace.c       | 77 
> +++++++++++++++++++++++++++++++++++++++++++
>  xen/common/edge_tracer.c      | 25 ++++++++++++++
>  xen/include/public/xen.h      |  1 +
>  xen/include/xen/edge_trace.h  | 19 +++++++++++
>  xen/include/xen/hypercall.h   |  7 ++++
>  xen/include/xen/sched.h       |  6 ++++
>  16 files changed, 192 insertions(+)
>  create mode 100644 xen/common/edge_trace.c
>  create mode 100644 xen/common/edge_tracer.c
>  create mode 100644 xen/include/xen/edge_trace.h
> 
> diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
> index 8c26cb4141..75e03337f9 100644
> --- a/tools/libxc/include/xenctrl.h
> +++ b/tools/libxc/include/xenctrl.h
> @@ -1572,6 +1572,8 @@ int xc_domctl(xc_interface *xch, struct xen_domctl 
> *domctl);
>  int xc_sysctl(xc_interface *xch, struct xen_sysctl *sysctl);
>  
>  int xc_version(xc_interface *xch, int cmd, void *arg);
> +int xc_edge_trace(xc_interface *xch, domid_t dom_id, int mode,
> +    unsigned int size, uint64_t *buf);

Coding style. Please align the second line with "xc_interface ...".

The same comment applies to all other similar places in this patch.

>  
>  int xc_flask_op(xc_interface *xch, xen_flask_op_t *op);
>  
> diff --git a/tools/libxc/xc_private.c b/tools/libxc/xc_private.c
> index f395594a8f..97663f219b 100644
> --- a/tools/libxc/xc_private.c
> +++ b/tools/libxc/xc_private.c
> @@ -530,6 +530,28 @@ int xc_version(xc_interface *xch, int cmd, void *arg)
>      return rc;
>  }
>  
> +int xc_edge_trace(xc_interface *xch,
> +    domid_t dom_id, int mode, unsigned int size, uint64_t* arg)

uint64_t *arg (note the position of '*')

> +{
> +    int rc;
> +
> +    DECLARE_HYPERCALL_BOUNCE(arg, size * sizeof(uint64_t),
> +        XC_HYPERCALL_BUFFER_BOUNCE_OUT);
> +
> +    if ( xc_hypercall_bounce_pre(xch, arg) )
> +    {
> +        PERROR("Could not bounce buffer for edge_trace hypercall");
> +        return -ENOMEM;
> +    }
> +
> +    rc = do_edge_trace(xch, dom_id, mode, size, HYPERCALL_BUFFER(arg));
> +
> +    xc_hypercall_bounce_post(xch, arg);
> +
> +    return rc;
> +}
> +
> +
>  unsigned long xc_make_page_below_4G(
>      xc_interface *xch, uint32_t domid, unsigned long mfn)
>  {
> diff --git a/tools/libxc/xc_private.h b/tools/libxc/xc_private.h
> index 1c27b0fded..60b0d8ebe3 100644
> --- a/tools/libxc/xc_private.h
> +++ b/tools/libxc/xc_private.h
> @@ -229,6 +229,14 @@ static inline int do_xen_version(xc_interface *xch, int 
> cmd, xc_hypercall_buffer
>                      cmd, HYPERCALL_BUFFER_AS_ARG(dest));
>  }
>  
> +static inline int do_edge_trace(xc_interface *xch, domid_t dom_id, int mode,
> +                                unsigned int size, xc_hypercall_buffer_t 
> *buf)
> +{
> +    DECLARE_HYPERCALL_BUFFER_ARGUMENT(buf);
> +    return xencall4(xch->xcall, __HYPERVISOR_edge_trace, dom_id, mode,
> +        size, HYPERCALL_BUFFER_AS_ARG(buf));
> +}
> +

This function can be folded into xc_edge_trace.

[...]
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index c07999b518..a4d36517f9 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(edge_trace, 4),
>  };
>  
>  #ifndef NDEBUG
> diff --git a/xen/arch/x86/hvm/hypercall.c b/xen/arch/x86/hvm/hypercall.c
> index e7238ce293..fed8363d8a 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(edge_trace),
>      HYPERCALL(arch_1)
>  };
>  
> diff --git a/xen/arch/x86/hypercall.c b/xen/arch/x86/hypercall.c
> index e30181817a..caa9376d29 100644
> --- a/xen/arch/x86/hypercall.c
> +++ b/xen/arch/x86/hypercall.c
> @@ -67,6 +67,7 @@ const hypercall_args_t hypercall_args_table[NR_hypercalls] =
>      ARGS(tmem_op, 1),
>      ARGS(xenpmu_op, 2),
>      ARGS(dm_op, 3),
> +    ARGS(edge_trace, 4),

You need to be careful with the order. You need to place your hypercall
*after* the mca one.

>      ARGS(mca, 1),
>      ARGS(arch_1, 1),
>  };
> diff --git a/xen/arch/x86/pv/hypercall.c b/xen/arch/x86/pv/hypercall.c
> index 7c5e5a629d..24f931a895 100644
> --- a/xen/arch/x86/pv/hypercall.c
> +++ b/xen/arch/x86/pv/hypercall.c
> @@ -79,6 +79,7 @@ static const hypercall_table_t pv_hypercall_table[] = {
>  #endif
>      HYPERCALL(xenpmu_op),
>      COMPAT_CALL(dm_op),
> +    HYPERCALL(edge_trace),
>      HYPERCALL(mca),

Same here.

>      HYPERCALL(arch_1),
>  };
> diff --git a/xen/common/Makefile b/xen/common/Makefile
> index 26c5a64337..274baf084b 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-$(CONFIG_TRACE_PC) += edge_trace.o
> +obj-$(CONFIG_TRACE_PC) += edge_tracer.o

Just call the new files trace_pc.o and trace_pc_stub.o to match the
KCONFIG option?

>  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 smp.o sort.o stop_machine.o string.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

This is not very ideal, but I won't ask you to work on the build system.

> diff --git a/xen/common/edge_trace.c b/xen/common/edge_trace.c
> new file mode 100644
> index 0000000000..7b325f4d17
> --- /dev/null
> +++ b/xen/common/edge_trace.c
> @@ -0,0 +1,77 @@
> +/******************************************************************************
> + * edge_trace.c
> + *
> + * Copyright (c) 2017 Felix Schmoll

Add you email address.

You also need to add license here and in all other new files.  See
CONTRIBUTING.

> + */
> +
> +#include <xen/edge_trace.h>
> +#include <xen/xmalloc.h>
> +#include <xen/guest_access.h>
> +#include <xen/sched.h>
> +
> +long do_edge_trace(domid_t dom, int mode, unsigned int size,
> +    XEN_GUEST_HANDLE_PARAM(uint64_t) buf)
> +{
> +    int ret = 0;
> +    struct domain *d;
> +
> +    if( dom == DOMID_SELF )

Coding style is wrong. Space after `if' please.

> +        d = current->domain;
> +    else
> +        d = get_domain_by_id(dom);
> +
> +    if( !d )
> +        return -EINVAL; /* invalid domain */
> +
> +    switch ( mode )
> +    {
> +        case TRACE_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);
> +

You will leak the buffer if the target domain is destroyed before you
get to call TRACE_STOP.

You need to make sure it is free in the domain destruction path.

> +            if( !d->tracing_buffer )
> +                ret = -ENOMEM;
> +            break;
> +        }
> +
> +        case TRACE_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;
> +}
> +
> +/*
> + * 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/edge_tracer.c b/xen/common/edge_tracer.c
> new file mode 100644
> index 0000000000..eb7ab96d8f
> --- /dev/null
> +++ b/xen/common/edge_tracer.c
> @@ -0,0 +1,25 @@
> +/******************************************************************************
> + * edge_tracer.c
> + *
> + * Copyright (c) 2017 Felix Schmoll
> + */
> +
> +#include <xen/edge_trace.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/xen.h b/xen/include/public/xen.h
> index 2ac6b1e24d..0b113847c9 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_edge_trace           42
>  
>  /* Architecture-specific hypercall definitions. */
>  #define __HYPERVISOR_arch_0               48
> diff --git a/xen/include/xen/edge_trace.h b/xen/include/xen/edge_trace.h
> new file mode 100644
> index 0000000000..aa34c3ef96
> --- /dev/null
> +++ b/xen/include/xen/edge_trace.h
> @@ -0,0 +1,19 @@
> +/******************************************************************************
> + * edge_trace.h
> + *
> + * Information structure that lives at the bottom of the per-cpu Xen stack.
> + */
> +
> +#ifndef __TRACE_H__
> +#define __TRACE_H__
> +
> +#include <xen/types.h>
> +#include <xen/sched.h>

Please sort these two alphabetically.

> +#include <asm/current.h>
> +
> +#define TRACE_START 0
> +#define TRACE_STOP 1
> +

These two defines need to be the in a public header, so that userspace
code can use them.  Also properly prefix them with XEN_.

The public header should be tools only. The defines will be enclosed by
__XEN_TOOLS__. See existing headers in xen/include/public.

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