Re: [PATCH 3/3] libxc: add xc_tbuf_trace() to insert trace records

[George Dunlap <george.dunlap@eu.citrix.com>]

On 25/07/13 14:23, David Vrabel wrote:
> From: David Vrabel <david.vrabel@citrix.com>
>
> Add xc_tbuf_trace() to allow trace records to be added to the trace
> buffer.  The subclass and event number and up to 7 uin32_t arguments
> may be specified.
>
> The hypercall sub-op used is HVMOP_xentrace which (despite the name)
> may be used by PV guests.

Would it make sense to make this interface more like the hypervisor's 
trace_var() -- that is, taking a sizeof() and single pointer, so that 
callers can easily pass in packed structs?

I suppose there's no reason we couldn't add a function with that 
interface at some point in the future if people want it.  Any thoughts 
from the toolstack maintainers?

  -George

>
> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
> ---
>   tools/libxc/xc_tbuf.c  |   39 +++++++++++++++++++++++++++++++++++++++
>   tools/libxc/xenctrl.h  |   11 +++++++++++
>   xen/arch/x86/hvm/hvm.c |    2 +-
>   3 files changed, 51 insertions(+), 1 deletions(-)
>
> diff --git a/tools/libxc/xc_tbuf.c b/tools/libxc/xc_tbuf.c
> index 4fb7bb1..0644952 100644
> --- a/tools/libxc/xc_tbuf.c
> +++ b/tools/libxc/xc_tbuf.c
> @@ -156,3 +156,42 @@ int xc_tbuf_set_evt_mask(xc_interface *xch, uint32_t mask)
>       return do_sysctl(xch, &sysctl);
>   }
>   
> +int xc_tbuf_trace(xc_interface *xch, uint16_t event, unsigned nr_args, ...)
> +{
> +    DECLARE_HYPERCALL;
> +    DECLARE_HYPERCALL_BUFFER(xen_hvm_xentrace_t, trace);
> +    unsigned i;
> +    va_list args;
> +    int ret = -1;
> +
> +    if ( nr_args > 7 ) {
> +        errno = -EINVAL;
> +        goto out;
> +    }
> +
> +    trace = xc_hypercall_buffer_alloc(xch, trace, sizeof(*trace));
> +    if ( trace == NULL )
> +    {
> +        PERROR("Count not alloc bounce buffer for trace hypercall");
> +        goto out;
> +    }
> +
> +    trace->event = event;
> +    trace->extra_bytes = nr_args * sizeof(uint32_t);
> +
> +    va_start(args, nr_args);
> +    for (i = 0; i < nr_args; i++)
> +        ((uint32_t *)trace->extra)[i] = va_arg(args, uint32_t);
> +    va_end(args);
> +
> +    hypercall.op = __HYPERVISOR_hvm_op;
> +    hypercall.arg[0] = HVMOP_xentrace;
> +    hypercall.arg[1] = HYPERCALL_BUFFER_AS_ARG(trace);
> +
> +    ret = do_xen_hypercall(xch, &hypercall);
> +
> +out:
> +    xc_hypercall_buffer_free(xch, trace);
> +
> +    return ret;
> +}
> diff --git a/tools/libxc/xenctrl.h b/tools/libxc/xenctrl.h
> index 388a9c3..927b198 100644
> --- a/tools/libxc/xenctrl.h
> +++ b/tools/libxc/xenctrl.h
> @@ -1387,6 +1387,17 @@ int xc_tbuf_set_cpu_mask(xc_interface *xch, uint32_t mask);
>   
>   int xc_tbuf_set_evt_mask(xc_interface *xch, uint32_t mask);
>   
> +/**
> + * Insert a trace record into the trace buffer.
> + *
> + * The trace records use the TRC_GUEST class and 'event' sets the
> + * sub-class and the event number.  There are no predefined values for
> + * these.
> + *
> + * Up to 7 uint32_t arguments may be included in the trace record.
> + */
> +int xc_tbuf_trace(xc_interface *xch, uint16_t event, unsigned nr_args, ...);
> +
>   int xc_domctl(xc_interface *xch, struct xen_domctl *domctl);
>   int xc_sysctl(xc_interface *xch, struct xen_sysctl *sysctl);
>   
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index b0d8094..b24a37d 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -4388,7 +4388,7 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
>               return -EFAULT;
>   
>           if ( tr.extra_bytes > sizeof(tr.extra)
> -             || (tr.event & ~((1u<<TRC_CLS_SHIFT)-1)) )
> +             || (tr.event & ~((1u<<TRC_SUBCLS_SHIFT)-1)) )
>               return -EINVAL;
>   
>           trace_var(TRC_GUEST_EVENT(tr.event), 1 /*cycles*/,