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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 11.08.17 at 17:25, <eggi.innovations@xxxxxxxxx> 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 <eggi.innovations@xxxxxxxxx>
> + *
> + * 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);

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

Jan


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