Re: [PATCH v6 10/11] tools/libxc: add xc_vmtrace_* functions

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Tue, Jul 07, 2020 at 09:39:49PM +0200, Michał Leszczyński wrote:
> From: Michal Leszczynski <michal.leszczynski@xxxxxxx>
> 
> Add functions in libxc that use the new XEN_DOMCTL_vmtrace interface.
> 
> Signed-off-by: Michal Leszczynski <michal.leszczynski@xxxxxxx>
> ---
>  tools/libxc/Makefile          |  1 +
>  tools/libxc/include/xenctrl.h | 40 ++++++++++++++++
>  tools/libxc/xc_vmtrace.c      | 87 +++++++++++++++++++++++++++++++++++
>  3 files changed, 128 insertions(+)
>  create mode 100644 tools/libxc/xc_vmtrace.c
> 
> diff --git a/tools/libxc/Makefile b/tools/libxc/Makefile
> index fae5969a73..605e44501d 100644
> --- a/tools/libxc/Makefile
> +++ b/tools/libxc/Makefile
> @@ -27,6 +27,7 @@ CTRL_SRCS-y       += xc_csched2.c
>  CTRL_SRCS-y       += xc_arinc653.c
>  CTRL_SRCS-y       += xc_rt.c
>  CTRL_SRCS-y       += xc_tbuf.c
> +CTRL_SRCS-y       += xc_vmtrace.c
>  CTRL_SRCS-y       += xc_pm.c
>  CTRL_SRCS-y       += xc_cpu_hotplug.c
>  CTRL_SRCS-y       += xc_resume.c
> diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
> index 4c89b7294c..491b2c3236 100644
> --- a/tools/libxc/include/xenctrl.h
> +++ b/tools/libxc/include/xenctrl.h
> @@ -1585,6 +1585,46 @@ int xc_tbuf_set_cpu_mask(xc_interface *xch, 
> xc_cpumap_t mask);
>  
>  int xc_tbuf_set_evt_mask(xc_interface *xch, uint32_t mask);
>  
> +/**
> + * Enable processor trace for given vCPU in given DomU.
> + * Allocate the trace ringbuffer with given size.
> + *
> + * @parm xch a handle to an open hypervisor interface
> + * @parm domid domain identifier
> + * @parm vcpu vcpu identifier
> + * @return 0 on success, -1 on failure
> + */
> +int xc_vmtrace_pt_enable(xc_interface *xch, uint32_t domid,
> +                         uint32_t vcpu);
> +
> +/**
> + * Disable processor trace for given vCPU in given DomU.
> + * Deallocate the trace ringbuffer.
> + *
> + * @parm xch a handle to an open hypervisor interface
> + * @parm domid domain identifier
> + * @parm vcpu vcpu identifier
> + * @return 0 on success, -1 on failure
> + */
> +int xc_vmtrace_pt_disable(xc_interface *xch, uint32_t domid,
> +                          uint32_t vcpu);
> +
> +/**
> + * Get current offset inside the trace ringbuffer.
> + * This allows to determine how much data was written into the buffer.
> + * Once buffer overflows, the offset will reset to 0 and the previous
> + * data will be overriden.
                   ^ overridden.
> + *
> + * @parm xch a handle to an open hypervisor interface
> + * @parm domid domain identifier
> + * @parm vcpu vcpu identifier
> + * @parm offset current offset inside trace buffer will be written there
> + * @parm size the total size of the trace buffer (in bytes)
> + * @return 0 on success, -1 on failure
> + */
> +int xc_vmtrace_pt_get_offset(xc_interface *xch, uint32_t domid,
> +                             uint32_t vcpu, uint64_t *offset, uint64_t 
> *size);
> +
>  int xc_domctl(xc_interface *xch, struct xen_domctl *domctl);
>  int xc_sysctl(xc_interface *xch, struct xen_sysctl *sysctl);
>  
> diff --git a/tools/libxc/xc_vmtrace.c b/tools/libxc/xc_vmtrace.c
> new file mode 100644
> index 0000000000..ee034da8d3
> --- /dev/null
> +++ b/tools/libxc/xc_vmtrace.c
> @@ -0,0 +1,87 @@
> +/******************************************************************************
> + * xc_vmtrace.c
> + *
> + * API for manipulating hardware tracing features
> + *
> + * Copyright (c) 2020, Michal Leszczynski
> + *
> + * Copyright 2020 CERT Polska. All rights reserved.
> + * Use is subject to license terms.
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation;
> + * version 2.1 of the License.
> + *
> + * This library 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
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; If not, see 
> <http://www.gnu.org/licenses/>.
> + */
> +
> +#include "xc_private.h"
> +#include <xen/trace.h>
> +
> +int xc_vmtrace_pt_enable(
> +        xc_interface *xch, uint32_t domid, uint32_t vcpu)
> +{
> +    DECLARE_DOMCTL;

You could do:

DECLARE_DOMCTL = {
    .cmd = XEN_DOMCTL_vmtrace_op,
    .domain = domid,
    ...
};

And avoid setting the fields below, the same applies to the rest
of the functions. Note that when doing this there's no need to set the
padding fields, as they will be zeroed by the initialization.

> +    int rc;
> +
> +    domctl.cmd = XEN_DOMCTL_vmtrace_op;
> +    domctl.domain = domid;
> +    domctl.u.vmtrace_op.cmd = XEN_DOMCTL_vmtrace_pt_enable;
> +    domctl.u.vmtrace_op.vcpu = vcpu;
> +    domctl.u.vmtrace_op.pad1 = 0;
> +    domctl.u.vmtrace_op.pad2 = 0;
> +
> +    rc = do_domctl(xch, &domctl);
> +    return rc;

Just do 'return do_domctl(xch, &domctl);', and you can drop the rc
variable (here and in xc_vmtrace_pt_disable).

> +}
> +
> +int xc_vmtrace_pt_get_offset(
> +        xc_interface *xch, uint32_t domid, uint32_t vcpu,
> +        uint64_t *offset, uint64_t *size)
> +{
> +    DECLARE_DOMCTL;
> +    int rc;
> +
> +    domctl.cmd = XEN_DOMCTL_vmtrace_op;
> +    domctl.domain = domid;
> +    domctl.u.vmtrace_op.cmd = XEN_DOMCTL_vmtrace_pt_get_offset;
> +    domctl.u.vmtrace_op.vcpu = vcpu;
> +    domctl.u.vmtrace_op.pad1 = 0;
> +    domctl.u.vmtrace_op.pad2 = 0;
> +
> +    rc = do_domctl(xch, &domctl);
> +    if ( !rc )
> +    {
> +        if (offset)
               ^ missing spaces.

Thanks.