Xen devel - Jan 2013 - [RFC PATCH] Adding support for coverage informations

From: Frediano Ziglio <frediano.ziglio@citrix.com>

This patch introduce coverage support to Xen.
Currently it allows to compile Xen with coverage support but there is no way
to extract them.

The declarations came from Linux source files (as you can see from file
headers).

It also add a new hypercall (can somebody reserve a number for this stuff?).

The idea is to have some operations mainly
- get filename
- get count informations for a file
- reset counter for a specific file
- reset all counters

The op parameter in the hypercall will be the operation.
The idx parameter is the index to the file.
The arg1 is the argument (a buffer pointer for filename or counters).
The arg2 probably will be the size of previous buffer.

Do you think I should pack these parameters and pass a single pointer?

Linux use a file system to export these information. I would use the same
format as Linux use so to make it easy to reuse tools from Linux (like lcov).
That the reason why I split get filename from get counter informations.

These informations cannot be put in a specific section (allowing a safe mapping)
as gcc use .rodata, .data, .text and .ctors sections.

I added code to handle constructors used in this case to initialize a linked
list of files.

Do anybody know why I had to exclude %.init.o files to make it compile?
Are these files used before Xen start?

Signed-off-by: Frediano Ziglio <frediano.ziglio@citrix.com>

diff -r 5af4f2ab06f3 Config.mk
--- a/Config.mk Tue Jan 22 09:33:10 2013 +0100
+++ b/Config.mk Mon Jan 28 17:36:14 2013 +0000
@@ -228,3 +228,7 @@ QEMU_TAG ?= 2a1354d655d816feaad7dbdb8364
 # doing and are prepared for some pain.
 
 CONFIG_TESTS       ?= y
+
+# Test coverage support
+TEST_COVERAGE ?= y
+
diff -r 5af4f2ab06f3 xen/Rules.mk
--- a/xen/Rules.mk      Tue Jan 22 09:33:10 2013 +0100
+++ b/xen/Rules.mk      Mon Jan 28 17:36:14 2013 +0000
@@ -103,6 +103,11 @@ subdir-all := $(subdir-y) $(subdir-n)
 
 $(filter %.init.o,$(obj-y) $(obj-bin-y)): CFLAGS += -DINIT_SECTIONS_ONLY
 
+
+ifeq ($(TEST_COVERAGE),y)
+$(filter-out %.init.o,$(obj-y) $(obj-bin-y)): CFLAGS += -fprofile-arcs
-ftest-coverage -DTEST_COVERAGE
+endif
+
 ifeq ($(lto),y)
 # Would like to handle all object files as bitcode, but objects made from
 # pure asm are in a different format and have to be collected separately.
diff -r 5af4f2ab06f3 xen/arch/x86/setup.c
--- a/xen/arch/x86/setup.c      Tue Jan 22 09:33:10 2013 +0100
+++ b/xen/arch/x86/setup.c      Mon Jan 28 17:36:14 2013 +0000
@@ -46,6 +46,7 @@
 #include <asm/setup.h>
 #include <xen/cpu.h>
 #include <asm/nmi.h>
+#include <xen/gcov.h>
 
 /* opt_nosmp: If true, secondary processors are ignored. */
 static bool_t __initdata opt_nosmp;
@@ -1313,6 +1314,8 @@ void __init __start_xen(unsigned long mb
 
     init_trace_bufs();
 
+    init_coverage();
+
     console_endboot();
 
     /* Hide UART from DOM0 if we''re using it */
diff -r 5af4f2ab06f3 xen/arch/x86/x86_64/compat/entry.S
--- a/xen/arch/x86/x86_64/compat/entry.S        Tue Jan 22 09:33:10 2013 +0100
+++ b/xen/arch/x86/x86_64/compat/entry.S        Mon Jan 28 17:36:14 2013 +0000
@@ -413,6 +413,8 @@ ENTRY(compat_hypercall_table)
         .quad do_domctl
         .quad compat_kexec_op
         .quad do_tmem_op
+        .quad compat_ni_hypercall
+        .quad compat_coverage_op
         .rept __HYPERVISOR_arch_0-((.-compat_hypercall_table)/8)
         .quad compat_ni_hypercall
         .endr
@@ -461,6 +463,8 @@ ENTRY(compat_hypercall_args_table)
         .byte 1 /* do_domctl                */
         .byte 2 /* compat_kexec_op          */
         .byte 1 /* do_tmem_op               */
+        .byte 0 /* compat_ni_hypercall      */
+        .byte 4 /* compat_coverage_op       */
         .rept __HYPERVISOR_arch_0-(.-compat_hypercall_args_table)
         .byte 0 /* compat_ni_hypercall      */
         .endr
diff -r 5af4f2ab06f3 xen/arch/x86/x86_64/entry.S
--- a/xen/arch/x86/x86_64/entry.S       Tue Jan 22 09:33:10 2013 +0100
+++ b/xen/arch/x86/x86_64/entry.S       Mon Jan 28 17:36:14 2013 +0000
@@ -738,6 +738,8 @@ ENTRY(hypercall_table)
         .quad do_domctl
         .quad do_kexec_op
         .quad do_tmem_op
+        .quad do_ni_hypercall
+        .quad do_coverage_op
         .rept __HYPERVISOR_arch_0-((.-hypercall_table)/8)
         .quad do_ni_hypercall
         .endr
@@ -786,6 +788,8 @@ ENTRY(hypercall_args_table)
         .byte 1 /* do_domctl            */
         .byte 2 /* do_kexec             */
         .byte 1 /* do_tmem_op           */
+        .byte 0 /* do_ni_hypercall      */
+        .byte 4 /* do_coverage_op       */
         .rept __HYPERVISOR_arch_0-(.-hypercall_args_table)
         .byte 0 /* do_ni_hypercall      */
         .endr
diff -r 5af4f2ab06f3 xen/arch/x86/xen.lds.S
--- a/xen/arch/x86/xen.lds.S    Tue Jan 22 09:33:10 2013 +0100
+++ b/xen/arch/x86/xen.lds.S    Mon Jan 28 17:36:14 2013 +0000
@@ -78,7 +78,23 @@ SECTIONS
        *(.data)
        *(.data.rel)
        *(.data.rel.*)
-       CONSTRUCTORS
+#ifdef __x86_64__
+        . = ALIGN(8);
+        __CTOR_LIST__ = .;
+        QUAD((__CTOR_END__ - __CTOR_LIST__) / 8 - 2)
+        *(.ctors)
+        CONSTRUCTORS
+        QUAD(0)
+        __CTOR_END__ = .;
+#else
+        . = ALIGN(4);
+        __CTOR_LIST__ = .;
+        LONG((__CTOR_END__ - __CTOR_LIST__) / 4 - 2)
+        *(.ctors)
+        CONSTRUCTORS
+        LONG(0)
+        __CTOR_END__ = .;
+#endif
   } :text
 
 #ifdef LOCK_PROFILE
diff -r 5af4f2ab06f3 xen/common/Makefile
--- a/xen/common/Makefile       Tue Jan 22 09:33:10 2013 +0100
+++ b/xen/common/Makefile       Mon Jan 28 17:36:14 2013 +0000
@@ -59,5 +59,7 @@ subdir-$(CONFIG_COMPAT) += compat
 
 subdir-$(x86_64) += hvm
 
+subdir-$(TEST_COVERAGE) += gcov
+
 subdir-y += libelf
 subdir-$(HAS_DEVICE_TREE) += libfdt
diff -r 5af4f2ab06f3 xen/common/gcov/Makefile
--- /dev/null   Thu Jan 01 00:00:00 1970 +0000
+++ b/xen/common/gcov/Makefile  Mon Jan 28 17:36:14 2013 +0000
@@ -0,0 +1,1 @@
+obj-y += gcov.o
diff -r 5af4f2ab06f3 xen/common/gcov/gcov.c
--- /dev/null   Thu Jan 01 00:00:00 1970 +0000
+++ b/xen/common/gcov/gcov.c    Mon Jan 28 17:36:14 2013 +0000
@@ -0,0 +1,97 @@
+/*
+ *  This code maintains a list of active profiling data structures.
+ *
+ *    Copyright IBM Corp. 2009
+ *    Author(s): Peter Oberparleiter <oberpar@linux.vnet.ibm.com>
+ *
+ *    Uses gcc-internal data definitions.
+ *    Based on the gcov-kernel patch by:
+ *       Hubertus Franke <frankeh@us.ibm.com>
+ *       Nigel Hinds <nhinds@us.ibm.com>
+ *       Rajan Ravindran <rajancr@us.ibm.com>
+ *       Peter Oberparleiter <oberpar@linux.vnet.ibm.com>
+ *       Paul Larson
+ */
+
+#include <xen/config.h>
+#include <xen/init.h>
+#include <xen/lib.h>
+#include <xen/gcov.h>
+#include <xen/errno.h>
+#include <public/xen.h>
+
+static struct gcov_info *info_list;
+static unsigned num_info = 0;
+
+/*
+ * __gcov_init is called by gcc-generated constructor code for each object
+ * file compiled with -fprofile-arcs.
+ * 
+ * Although this function is called only during initialization is called from
+ * a .text section which is still present after initialization so not declare
+ * as __init.
+ */
+void __gcov_init(struct gcov_info *info)
+{
+    /* add new profiling data structure to list */
+    info->next = info_list;
+    info_list = info;
+    ++num_info;
+}
+
+/*
+ * These functions may be referenced by gcc-generated profiling code but serve
+ * no function for Xen.
+ */
+void __gcov_flush(void)
+{
+    /* Unused. */
+}
+
+void __gcov_merge_add(gcov_type *counters, unsigned int n_counters)
+{
+    /* Unused. */
+}
+
+void __gcov_merge_single(gcov_type *counters, unsigned int n_counters)
+{
+    /* Unused. */
+}
+
+void __gcov_merge_delta(gcov_type *counters, unsigned int n_counters)
+{
+    /* Unused. */
+}
+
+typedef void (*ctor_func_t)(void);
+extern struct {
+    unsigned long count;
+    ctor_func_t funcs[1];
+} __CTOR_LIST__;
+
+void __init init_coverage(void)
+{
+    unsigned long n;
+    for (n = 0; n < __CTOR_LIST__.count; ++n)
+        __CTOR_LIST__.funcs[n]();
+
+#ifndef NDEBUG
+    printk(XENLOG_INFO "Initialized %u coverage strucures\n",
num_info);
+    if (info_list)
+        printk(XENLOG_INFO "First coverage file %s\n",
info_list->filename);
+#endif
+}
+
+long do_coverage_op(int op, int idx, XEN_GUEST_HANDLE_PARAM(void) uarg, int
arg2)
+{
+    return -EINVAL;
+}
+
+#ifdef CONFIG_COMPAT
+int compat_coverage_op(int op, int idx, XEN_GUEST_HANDLE_PARAM(void) uarg, int
arg2)
+{
+    return -EINVAL;
+}
+#endif
+
+
diff -r 5af4f2ab06f3 xen/include/public/xen.h
--- a/xen/include/public/xen.h  Tue Jan 22 09:33:10 2013 +0100
+++ b/xen/include/public/xen.h  Mon Jan 28 17:36:14 2013 +0000
@@ -101,6 +101,7 @@ DEFINE_XEN_GUEST_HANDLE(xen_ulong_t);
 #define __HYPERVISOR_kexec_op             37
 #define __HYPERVISOR_tmem_op              38
 #define __HYPERVISOR_xc_reserved_op       39 /* reserved for XenClient */
+#define __HYPERVISOR_coverage             40 /* reserved to get coverage
information */
 
 /* Architecture-specific hypercall definitions. */
 #define __HYPERVISOR_arch_0               48
diff -r 5af4f2ab06f3 xen/include/xen/gcov.h
--- /dev/null   Thu Jan 01 00:00:00 1970 +0000
+++ b/xen/include/xen/gcov.h    Mon Jan 28 17:36:14 2013 +0000
@@ -0,0 +1,141 @@
+/*
+ *  Profiling infrastructure declarations.
+ *
+ *  This file is based on gcc-internal definitions. Data structures are
+ *  defined to be compatible with gcc counterparts. For a better
+ *  understanding, refer to gcc source: gcc/gcov-io.h.
+ *
+ *    Copyright IBM Corp. 2009
+ *    Author(s): Peter Oberparleiter <oberpar@linux.vnet.ibm.com>
+ *
+ *    Uses gcc-internal data definitions.
+ */
+
+#ifndef GCOV_H
+#define GCOV_H GCOV_H
+
+/*
+ * Profiling data types used for gcc 3.4 and above - these are defined by
+ * gcc and need to be kept as close to the original definition as possible to
+ * remain compatible.
+ */
+#define GCOV_COUNTERS         5
+#define GCOV_DATA_MAGIC       ((unsigned int) 0x67636461)
+#define GCOV_TAG_FUNCTION     ((unsigned int) 0x01000000)
+#define GCOV_TAG_COUNTER_BASE ((unsigned int) 0x01a10000)
+#define GCOV_TAG_FOR_COUNTER(count) \
+    (GCOV_TAG_COUNTER_BASE + ((unsigned int) (count) << 17))
+
+#if BITS_PER_LONG >= 64
+typedef long gcov_type;
+#else
+typedef long long gcov_type;
+#endif
+
+/**
+ * struct gcov_fn_info - profiling meta data per function
+ * @ident: object file-unique function identifier
+ * @checksum: function checksum
+ * @n_ctrs: number of values per counter type belonging to this function
+ *
+ * This data is generated by gcc during compilation and doesn''t change
+ * at run-time.
+ */
+struct gcov_fn_info {
+    unsigned int ident;
+    unsigned int checksum;
+    unsigned int n_ctrs[0];
+};
+
+/**
+ * struct gcov_ctr_info - profiling data per counter type
+ * @num: number of counter values for this type
+ * @values: array of counter values for this type
+ * @merge: merge function for counter values of this type (unused)
+ *
+ * This data is generated by gcc during compilation and doesn''t change
+ * at run-time with the exception of the values array.
+ */
+struct gcov_ctr_info {
+    unsigned int num;
+    gcov_type *values;
+    void (*merge)(gcov_type *, unsigned int);
+};
+
+/**
+ * struct gcov_info - profiling data per object file
+ * @version: gcov version magic indicating the gcc version used for compilation
+ * @next: list head for a singly-linked list
+ * @stamp: time stamp
+ * @filename: name of the associated gcov data file
+ * @n_functions: number of instrumented functions
+ * @functions: function data
+ * @ctr_mask: mask specifying which counter types are active
+ * @counts: counter data per counter type
+ *
+ * This data is generated by gcc during compilation and doesn''t change
+ * at run-time with the exception of the next pointer.
+ */
+struct gcov_info {
+    unsigned int              version;
+    struct gcov_info          *next;
+    unsigned int              stamp;
+    const char                *filename;
+    unsigned int              n_functions;
+    const struct gcov_fn_info *functions;
+    unsigned int              ctr_mask;
+    struct gcov_ctr_info      counts[0];
+};
+
+#if 0
+/* Base interface. */
+enum gcov_action {
+    GCOV_ADD,
+    GCOV_REMOVE,
+};
+
+void gcov_event(enum gcov_action action, struct gcov_info *info);
+void gcov_enable_events(void);
+
+/* Iterator control. */
+struct seq_file;
+struct gcov_iterator;
+
+struct gcov_iterator *gcov_iter_new(struct gcov_info *info);
+void gcov_iter_free(struct gcov_iterator *iter);
+void gcov_iter_start(struct gcov_iterator *iter);
+int gcov_iter_next(struct gcov_iterator *iter);
+int gcov_iter_write(struct gcov_iterator *iter, struct seq_file *seq);
+struct gcov_info *gcov_iter_get_info(struct gcov_iterator *iter);
+
+/* gcov_info control. */
+void gcov_info_reset(struct gcov_info *info);
+int gcov_info_is_compatible(struct gcov_info *info1, struct gcov_info *info2);
+void gcov_info_add(struct gcov_info *dest, struct gcov_info *source);
+struct gcov_info *gcov_info_dup(struct gcov_info *info);
+void gcov_info_free(struct gcov_info *info);
+
+struct gcov_link {
+    enum {
+        OBJ_TREE,
+        SRC_TREE,
+    } dir;
+    const char *ext;
+};
+extern const struct gcov_link gcov_link[];
+#endif
+
+/**
+ * Initialize coverage informations
+ * Currently create a linked list of all files compiled in with
+ * coverage informations.
+ */
+#ifdef TEST_COVERAGE
+void __init init_coverage(void);
+#else
+static inline void init_coverage(void) {
+}
+#endif
+
+
+#endif /* GCOV_H */
diff -r 5af4f2ab06f3 xen/include/xen/hypercall.h
--- a/xen/include/xen/hypercall.h       Tue Jan 22 09:33:10 2013 +0100
+++ b/xen/include/xen/hypercall.h       Mon Jan 28 17:36:14 2013 +0000
@@ -140,6 +140,12 @@ do_tmem_op(
 extern long
 do_xenoprof_op(int op, XEN_GUEST_HANDLE_PARAM(void) arg);
 
+extern long
+do_coverage_op(int op,
+    int idx,
+    XEN_GUEST_HANDLE_PARAM(void) arg1,
+    int arg2);
+
 #ifdef CONFIG_COMPAT
 
 extern int
@@ -162,6 +168,12 @@ compat_vcpu_op(
 extern int
 compat_xenoprof_op(int op, XEN_GUEST_HANDLE_PARAM(void) arg);
 
+extern long
+compat_coverage_op(int op,
+    int idx,
+    XEN_GUEST_HANDLE_PARAM(void) arg1,
+    int arg2);
+
 extern int
 compat_xen_version(
     int cmd,


Regards,
  Frediano Ziglio

>>> On 28.01.13 at 22:16, Frediano Ziglio
<frediano.ziglio@citrix.com> wrote:
> This patch introduce coverage support to Xen.
> Currently it allows to compile Xen with coverage support but there is no
way
> to extract them.
> 
> The declarations came from Linux source files (as you can see from file
> headers).
> 
> It also add a new hypercall (can somebody reserve a number for this
stuff?).
> 
> The idea is to have some operations mainly
> - get filename
> - get count informations for a file
> - reset counter for a specific file
> - reset all counters
The description you give doesn''t really tell me what all this will
really buy us. And considering that the patch enables this by
default, I think I''d prefer to know...
> The op parameter in the hypercall will be the operation.
> The idx parameter is the index to the file.
> The arg1 is the argument (a buffer pointer for filename or counters).
> The arg2 probably will be the size of previous buffer.
> 
> Do you think I should pack these parameters and pass a single pointer?
I don''t see a need.
> Linux use a file system to export these information. I would use the same
> format as Linux use so to make it easy to reuse tools from Linux (like 
> lcov).
> That the reason why I split get filename from get counter informations.
> 
> These informations cannot be put in a specific section (allowing a safe 
> mapping)
> as gcc use .rodata, .data, .text and .ctors sections.
What are you trying to tell us here?
> I added code to handle constructors used in this case to initialize a
linked
> list of files.
> 
> Do anybody know why I had to exclude %.init.o files to make it compile?
Without you telling us what problems not doing so causes, no.
> Are these files used before Xen start?
No, they''re containing code that''s being used only during boot
(when whole files are init-only, we can also move the e.g. string
literals into .init.* sections).
> --- a/xen/arch/x86/xen.lds.S    Tue Jan 22 09:33:10 2013 +0100
> +++ b/xen/arch/x86/xen.lds.S    Mon Jan 28 17:36:14 2013 +0000
> @@ -78,7 +78,23 @@ SECTIONS
>         *(.data)
>         *(.data.rel)
>         *(.data.rel.*)
> -       CONSTRUCTORS
> +#ifdef __x86_64__
> +        . = ALIGN(8);
> +        __CTOR_LIST__ = .;
> +        QUAD((__CTOR_END__ - __CTOR_LIST__) / 8 - 2)
> +        *(.ctors)
> +        CONSTRUCTORS
This doesn''t look right: Even if CONSTRUCTORS is presumably
producing nothing right now, you change its treatment by moving
it inside the __CTOR_{LIST,END}__ range. Linux has
CONSTRUCTORS and *(.ctors) separate too, and (validly) puts
.ctors inside the .init.data section.
> +        QUAD(0)
> +        __CTOR_END__ = .;
> +#else
> +        . = ALIGN(4);
> +        __CTOR_LIST__ = .;
> +        LONG((__CTOR_END__ - __CTOR_LIST__) / 4 - 2)
> +        *(.ctors)
> +        CONSTRUCTORS
> +        LONG(0)
> +        __CTOR_END__ = .;
> +#endif
What case is this #else portion trying to address?
> --- /dev/null   Thu Jan 01 00:00:00 1970 +0000
> +++ b/xen/common/gcov/gcov.c    Mon Jan 28 17:36:14 2013 +0000
Please decide for a consistent coding style in the newly added
files - either all Linux or all Xen, but not a mixture of both.
> +#ifdef CONFIG_COMPAT
> +int compat_coverage_op(int op, int idx, XEN_GUEST_HANDLE_PARAM(void) uarg,
int arg2)
> +{
> +    return -EINVAL;
> +}
> +#endif
By suitably defining the interface structures you can avoid the
need for a separate compat handler.
> +#ifdef TEST_COVERAGE
> +void __init init_coverage(void);
No __init annotations in declarations please (they only belong on
the actual definition).

Jan

On Mon, 2013-01-28 at 21:16 +0000, Frediano Ziglio
wrote:
> From: Frediano Ziglio <frediano.ziglio@citrix.com>
> 
> This patch introduce coverage support to Xen.
> Currently it allows to compile Xen with coverage support but there is no
way
> to extract them.
> 
> The declarations came from Linux source files (as you can see from file
> headers).
> 
> It also add a new hypercall (can somebody reserve a number for this
stuff?).
You can simply patch xen/include/public/xen.h to declare the new
__HYPERVISOR_foo_op. (which I now see you have done in this patch!). If
you just want to reserve the number it is also allowed to send just that
hunk to reserve a number pending the implementation.

BTW I''d suggest leaving the stub implementation out of the patch until
you''ve decided what it will look like, so it returns ENOSYS instead of
EINVAL (or change your stub to return ENOSYS).
> The idea is to have some operations mainly
> - get filename
This is get a list of filenames (i.e. input for the next two)?
> - get count informations for a file
> - reset counter for a specific file
How atomic do these need to be? Will it be necessary to obtain
consistent snapshots of the counters for multiple files? Even for a
single file I assume there will be many counters so getting a consistent
snapshot at the file level might even be tricky?

Or perhaps since you are measuring coverage you only care about one bit
per basic block (rather than a counter as such) and if you race then, oh
well, you''ll see it next time.
> - reset all counters
> 
> The op parameter in the hypercall will be the operation.
> The idx parameter is the index to the file.
> The arg1 is the argument (a buffer pointer for filename or counters).
> The arg2 probably will be the size of previous buffer.
Often the best way to deal with arg1/arg2 would be to pass a union of
the types specific to each operation.

But how does the caller determine the size of the buffer? I guess it
various from file to file? Is this what comes from "get count
informations for a file"?

Is this a dom0 only operation? Perhaps some sysctl subops would be
better than a whole new op?
> Do you think I should pack these parameters and pass a single pointer?
> 
> Linux use a file system to export these information. I would use the same
> format as Linux use so to make it easy to reuse tools from Linux (like
lcov).
> That the reason why I split get filename from get counter informations.
You could also synthesize this at the tools layer. I don''t know enough
of how this stuff works to say what makes sense at the h/v layer.

What form does this information take internally?
> Do anybody know why I had to exclude %.init.o files to make it compile?
> Are these files used before Xen start?
They are used during start of day bring up and then discarded once Xen
is up and running. I''m not sure what that would affect the link,
although it would obviously be disastrous if you were to touch the
counters associated with those files again after they were thrown away.

We might have some checks in the tree to ensure that non-init code
cannot reference init code by mistake -- maybe that''s what broke the
build for you?
> Signed-off-by: Frediano Ziglio <frediano.ziglio@citrix.com>
> 
> diff -r 5af4f2ab06f3 Config.mk
> --- a/Config.mk Tue Jan 22 09:33:10 2013 +0100
> +++ b/Config.mk Mon Jan 28 17:36:14 2013 +0000
> @@ -228,3 +228,7 @@ QEMU_TAG ?= 2a1354d655d816feaad7dbdb8364
>  # doing and are prepared for some pain.
> 
>  CONFIG_TESTS       ?= y
> +
> +# Test coverage support
> +TEST_COVERAGE ?= y
Off by default I think.
> diff -r 5af4f2ab06f3 xen/arch/x86/x86_64/compat/entry.S
> --- a/xen/arch/x86/x86_64/compat/entry.S        Tue Jan 22 09:33:10 2013
+0100
> +++ b/xen/arch/x86/x86_64/compat/entry.S        Mon Jan 28 17:36:14 2013
+0000
> @@ -413,6 +413,8 @@ ENTRY(compat_hypercall_table)
>          .quad do_domctl
>          .quad compat_kexec_op
>          .quad do_tmem_op
> +        .quad compat_ni_hypercall
> +        .quad compat_coverage_op
This is in gcov.c which is only conditionally compiled, so you need a
stub for the case where coverage is not enabled. Same goes for the
non-compat call and init_coverage().
> +typedef void (*ctor_func_t)(void);
> +extern struct {
> +    unsigned long count;
> +    ctor_func_t funcs[1];
> +} __CTOR_LIST__;
> +
> +void __init init_coverage(void)
> +{
> +    unsigned long n;
> +    for (n = 0; n < __CTOR_LIST__.count; ++n)
> +        __CTOR_LIST__.funcs[n]();
> +
> +#ifndef NDEBUG
> +    printk(XENLOG_INFO "Initialized %u coverage strucures\n",
num_info);
> +    if (info_list)
> +        printk(XENLOG_INFO "First coverage file %s\n",
info_list->filename);
> +#endif
> +}
I think this could be a more generic run_ctors type function, its use as
part of the coverage stuff is a little bit incidental.

Unless for some reason we want to discourage the use of ctors for other
purposes?
> diff -r 5af4f2ab06f3 xen/include/xen/gcov.h
> --- /dev/null   Thu Jan 01 00:00:00 1970 +0000
> +++ b/xen/include/xen/gcov.h    Mon Jan 28 17:36:14 2013 +0000
> @@ -0,0 +1,141 @@
> +/*
> + *  Profiling infrastructure declarations.
> + *
> + *  This file is based on gcc-internal definitions. Data structures are
> + *  defined to be compatible with gcc counterparts. For a better
> + *  understanding, refer to gcc source: gcc/gcov-io.h.
> + *
> + *    Copyright IBM Corp. 2009
> + *    Author(s): Peter Oberparleiter <oberpar@linux.vnet.ibm.com>
> + *
> + *    Uses gcc-internal data definitions.
Any idea what the license on these is?

Are these interfaces subject to change with different gcc versions?

Ian.

>>> On 29.01.13 at 11:56, Ian Campbell <Ian.Campbell@citrix.com>
wrote:
> We might have some checks in the tree to ensure that non-init code
> cannot reference init code by mistake -- maybe that''s what broke
the
> build for you?
No, (sadly) we don''t have such checks in place.

Jan

On 29/01/13 11:41, Jan Beulich wrote:
>>>> On 29.01.13 at 11:56, Ian Campbell
<Ian.Campbell@citrix.com> wrote:
>> We might have some checks in the tree to ensure that non-init code
>> cannot reference init code by mistake -- maybe that''s what
broke the
>> build for you?
> No, (sadly) we don''t have such checks in place.
>
> Jan
Linux has such checks.  Would it be possible to beg/steal/borrow it in a
useful fashion?

With regards to the reentrant NMI issue, I was considering introducing
__nmi and __mce tags in a similar way to __init.

At one level, simply having the tags as noops to the compiler would at
least make programmers consider the implication of what they are
inserting into the codepaths, but if there is the ability to have
compiler checks to verify a base level of sanity, that is even better.

~Andrew
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

>>> On 29.01.13 at 12:47, Andrew Cooper
<andrew.cooper3@citrix.com> wrote:
> On 29/01/13 11:41, Jan Beulich wrote:
>>>>> On 29.01.13 at 11:56, Ian Campbell
<Ian.Campbell@citrix.com> wrote:
>>> We might have some checks in the tree to ensure that non-init code
>>> cannot reference init code by mistake -- maybe that''s what
broke the
>>> build for you?
>> No, (sadly) we don''t have such checks in place.
> 
> Linux has such checks.  Would it be possible to beg/steal/borrow it in a
> useful fashion?
We could, but code refactoring would then also be needed (since
we do call __init code from non-__init is a few places iirc, suitably
guarded to make sure this really only happens on the boot path).
> With regards to the reentrant NMI issue, I was considering introducing
> __nmi and __mce tags in a similar way to __init.
> 
> At one level, simply having the tags as noops to the compiler would at
> least make programmers consider the implication of what they are
> inserting into the codepaths, but if there is the ability to have
> compiler checks to verify a base level of sanity, that is even better.
Would be nice, yes.

Jan

On Tue, 2013-01-29 at 10:56 +0000, Ian Campbell wrote:
> On Mon, 2013-01-28 at 21:16 +0000, Frediano Ziglio wrote:
> > From: Frediano Ziglio <frediano.ziglio@citrix.com>
> > 
> > This patch introduce coverage support to Xen.
> > Currently it allows to compile Xen with coverage support but there is
no way
> > to extract them.
> > 
> > The declarations came from Linux source files (as you can see from
file
> > headers).
> > 
> > It also add a new hypercall (can somebody reserve a number for this
stuff?).
> 
> You can simply patch xen/include/public/xen.h to declare the new
> __HYPERVISOR_foo_op. (which I now see you have done in this patch!). If
> you just want to reserve the number it is also allowed to send just that
> hunk to reserve a number pending the implementation.
> 
> BTW I''d suggest leaving the stub implementation out of the patch
until
> you''ve decided what it will look like, so it returns ENOSYS
instead of
> EINVAL (or change your stub to return ENOSYS).
> 
Easy to change. Actually I discovered that my patch does not even
compile if you disable TEST_COVERAGE (and it should be disabled by
default too). EINVAL say that the hypercall is present but the value
passed in invalid.

The reason why adding a new hypercall instead of a new sysctl is simply
because is easier to have a zero cost if you disable coverage
informations. The best thing would be redirect do_coverage_op to
do_ni_hypercall using linker options but even two small stub would do
(these stubs will return ENOSYS instead).
> > The idea is to have some operations mainly
> > - get filename
> 
> This is get a list of filenames (i.e. input for the next two)?
> 
The idea was just to pass the index and get a filename or a ENOENT error
(or a ENOBUF if buffer is too small). Actually you should scan the
entire list to get a proper filename so this end up in a O(N^2)
operation but as N is constant and not that big it should not be a
problem. If it''s a problem I can implement a tree or simply reuse
constructor space to store a simple array of gcov_info*.
> > - get count informations for a file
> > - reset counter for a specific file
> 
> How atomic do these need to be? Will it be necessary to obtain
> consistent snapshots of the counters for multiple files? Even for a
> single file I assume there will be many counters so getting a consistent
> snapshot at the file level might even be tricky?
> 
> Or perhaps since you are measuring coverage you only care about one bit
> per basic block (rather than a counter as such) and if you race then, oh
> well, you''ll see it next time.
> 
They cannot be so atomic anyway as the hypervisor is running, I''d not
use a lock or similar. These informations are mainly statistical one.
It''s not possible to disable incrementing any counter and also the
increment is done without locking (so it''s possible to loose some
number). Unless you pause all machines and stop any userspace program
that is doing some activities with Xen you''ll have to consider some
dirty counter. Perhaps would be better to have an operation to
query/reset more filenames at a time. But even in Linux you have to
"cat" different files to get informations.
> > - reset all counters
> > 
> > The op parameter in the hypercall will be the operation.
> > The idx parameter is the index to the file.
> > The arg1 is the argument (a buffer pointer for filename or counters).
> > The arg2 probably will be the size of previous buffer.
> 
> Often the best way to deal with arg1/arg2 would be to pass a union of
> the types specific to each operation.
> 
In this case arg1 if the buffer while arg2 would be mainly the length of
arg1 so they cannot be an union.
> But how does the caller determine the size of the buffer? I guess it
> various from file to file? Is this what comes from "get count
> informations for a file"?
> 
Well.. pass a large enough buffer and handle ENOBUF correctly.
> Is this a dom0 only operation? Perhaps some sysctl subops would be
> better than a whole new op?
> 
You can inference quite a lot of useful (and possibly sensitive)
informations from these statistics so yes, beside should be disabled in
production systems I would limit strictly to privileged domains.
> > Do you think I should pack these parameters and pass a single pointer?
> > 
> > Linux use a file system to export these information. I would use the
same
> > format as Linux use so to make it easy to reuse tools from Linux (like
lcov).
> > That the reason why I split get filename from get counter
informations.
> 
> You could also synthesize this at the tools layer. I don''t know
enough
> of how this stuff works to say what makes sense at the h/v layer.
> 
> What form does this information take internally?
> 
They are binary files. The format is defined by gcc. On userspace you
get a single .gcda file for each compile unit. This file is updated when
any program exit (multiple execution update the files). Gcc generate
also .gcno files. You need both .gcda and .gcno at the end to handle
coverage information.
> > Do anybody know why I had to exclude %.init.o files to make it
compile?
> > Are these files used before Xen start?
> 
> They are used during start of day bring up and then discarded once Xen
> is up and running. I''m not sure what that would affect the link,
> although it would obviously be disastrous if you were to touch the
> counters associated with those files again after they were thrown away.
> 
The problem would be reading (or resetting) these counter if discarded,
probably would lead to some buffer overwriting which is not good.
Actually these modules have no such informations so it''s not a problem.
> We might have some checks in the tree to ensure that non-init code
> cannot reference init code by mistake -- maybe that''s what broke
the
> build for you?
> 
Yes, I think it''s safer to disable for them (as it actually).

Are TEST_COVERAGE a good names or would be better to have something like
CONFIG_ENABLE_COVERAGE (do you use any prefix for configure stuff?) ?
> > Signed-off-by: Frediano Ziglio <frediano.ziglio@citrix.com>
> > 
> > diff -r 5af4f2ab06f3 Config.mk
> > --- a/Config.mk Tue Jan 22 09:33:10 2013 +0100
> > +++ b/Config.mk Mon Jan 28 17:36:14 2013 +0000
> > @@ -228,3 +228,7 @@ QEMU_TAG ?= 2a1354d655d816feaad7dbdb8364
> >  # doing and are prepared for some pain.
> > 
> >  CONFIG_TESTS       ?= y
> > +
> > +# Test coverage support
> > +TEST_COVERAGE ?= y
> 
> Off by default I think.
> 
> > diff -r 5af4f2ab06f3 xen/arch/x86/x86_64/compat/entry.S
> > --- a/xen/arch/x86/x86_64/compat/entry.S        Tue Jan 22 09:33:10
2013 +0100
> > +++ b/xen/arch/x86/x86_64/compat/entry.S        Mon Jan 28 17:36:14
2013 +0000
> > @@ -413,6 +413,8 @@ ENTRY(compat_hypercall_table)
> >          .quad do_domctl
> >          .quad compat_kexec_op
> >          .quad do_tmem_op
> > +        .quad compat_ni_hypercall
> > +        .quad compat_coverage_op
> 
> This is in gcov.c which is only conditionally compiled, so you need a
> stub for the case where coverage is not enabled. Same goes for the
> non-compat call and init_coverage().
> 
Yes, I added the two stubs (I''ll send the update). init_coverage is not
a problem, header define as an empty static inline function if coverage
is disabled.
> > +typedef void (*ctor_func_t)(void);
> > +extern struct {
> > +    unsigned long count;
> > +    ctor_func_t funcs[1];
> > +} __CTOR_LIST__;
> > +
> > +void __init init_coverage(void)
> > +{
> > +    unsigned long n;
> > +    for (n = 0; n < __CTOR_LIST__.count; ++n)
> > +        __CTOR_LIST__.funcs[n]();
> > +
> > +#ifndef NDEBUG
> > +    printk(XENLOG_INFO "Initialized %u coverage
strucures\n", num_info);
> > +    if (info_list)
> > +        printk(XENLOG_INFO "First coverage file %s\n",
info_list->filename);
> > +#endif
> > +}
> 
> I think this could be a more generic run_ctors type function, its use as
> part of the coverage stuff is a little bit incidental.
> 
> Unless for some reason we want to discourage the use of ctors for other
> purposes?
> 
Well... I think constructor was implemented to support dynamic
initializers in C++ and other high level languages, something like:

int num = get_num(); /* not working on C */

They are used in C to handle initialization of shared objects (using
constructor attribute in gcc) but beside this I didn''t see other uses.
The problem with C++ is that functions called in such way should not
make much assumption on other module initialization and are executed
before main(). At this point I don''t know a proper use in Xen and when
to initialize them.
> > diff -r 5af4f2ab06f3 xen/include/xen/gcov.h
> > --- /dev/null   Thu Jan 01 00:00:00 1970 +0000
> > +++ b/xen/include/xen/gcov.h    Mon Jan 28 17:36:14 2013 +0000
> > @@ -0,0 +1,141 @@
> > +/*
> > + *  Profiling infrastructure declarations.
> > + *
> > + *  This file is based on gcc-internal definitions. Data structures
are
> > + *  defined to be compatible with gcc counterparts. For a better
> > + *  understanding, refer to gcc source: gcc/gcov-io.h.
> > + *
> > + *    Copyright IBM Corp. 2009
> > + *    Author(s): Peter Oberparleiter
<oberpar@linux.vnet.ibm.com>
> > + *
> > + *    Uses gcc-internal data definitions.
> 
> Any idea what the license on these is?
> 
They are contained in Linux kernel so mainly GPL.
I think that however these header could lead to exception stated in
http://gcc.gnu.org/onlinedocs/libstdc++/manual/license.html so should
allow more uses than GPL.
> Are these interfaces subject to change with different gcc versions?
> 
The last updates was from gcc 3.4. They tend to not change but are not
set on stone. I hope that changes to these structure would lead the
program to not link correctly (for instance calling a __gcov_init2
instead of __gcov_init).
> Ian.
> 
Frediano

On Tue, 2013-01-29 at 10:41 +0000, Jan Beulich wrote:
> >>> On 28.01.13 at 22:16, Frediano Ziglio
<frediano.ziglio@citrix.com> wrote:
> > This patch introduce coverage support to Xen.
> > Currently it allows to compile Xen with coverage support but there is
no way
> > to extract them.
> > 
> > The declarations came from Linux source files (as you can see from
file
> > headers).
> > 
> > It also add a new hypercall (can somebody reserve a number for this
stuff?).
> > 
> > The idea is to have some operations mainly
> > - get filename
> > - get count informations for a file
> > - reset counter for a specific file
> > - reset all counters
> 
> The description you give doesn''t really tell me what all this will
> really buy us. And considering that the patch enables this by
> default, I think I''d prefer to know...
> 
This was a mistake, coverage should be disabled by default. Compiler add
an extra increment instruction for every basic block.

The purpose of coverage is usually for testing to detect which part of
code are not well tested. It''s used also to optimize code doing
profiling that could be fed up again to compiler to get more optimized
code.

The different with profile (oprof) is that oprof is more statistical and
does not count every line.
> > The op parameter in the hypercall will be the operation.
> > The idx parameter is the index to the file.
> > The arg1 is the argument (a buffer pointer for filename or counters).
> > The arg2 probably will be the size of previous buffer.
> > 
> > Do you think I should pack these parameters and pass a single pointer?
> 
> I don''t see a need.
> 
> > Linux use a file system to export these information. I would use the
same
> > format as Linux use so to make it easy to reuse tools from Linux (like
> > lcov).
> > That the reason why I split get filename from get counter
informations.
> > 
> > These informations cannot be put in a specific section (allowing a
safe
> > mapping)
> > as gcc use .rodata, .data, .text and .ctors sections.
> 
> What are you trying to tell us here?
> 
If gcc emitted code/data for coverage in particular section we could
pack these structures together and map them to userspace instead of
having to use hypercalls to read them.
> > I added code to handle constructors used in this case to initialize a
linked
> > list of files.
> > 
> > Do anybody know why I had to exclude %.init.o files to make it
compile?
> 
> Without you telling us what problems not doing so causes, no.
> 
Xen didn''t compile. In xen/Rules.mk there is code that change if
*.init.o do not contain some sections (text/data/bss).
> > Are these files used before Xen start?
> 
> No, they''re containing code that''s being used only during
boot
> (when whole files are init-only, we can also move the e.g. string
> literals into .init.* sections).
> 
> > --- a/xen/arch/x86/xen.lds.S    Tue Jan 22 09:33:10 2013 +0100
> > +++ b/xen/arch/x86/xen.lds.S    Mon Jan 28 17:36:14 2013 +0000
> > @@ -78,7 +78,23 @@ SECTIONS
> >         *(.data)
> >         *(.data.rel)
> >         *(.data.rel.*)
> > -       CONSTRUCTORS
> > +#ifdef __x86_64__
> > +        . = ALIGN(8);
> > +        __CTOR_LIST__ = .;
> > +        QUAD((__CTOR_END__ - __CTOR_LIST__) / 8 - 2)
> > +        *(.ctors)
> > +        CONSTRUCTORS
> 
> This doesn''t look right: Even if CONSTRUCTORS is presumably
> producing nothing right now, you change its treatment by moving
> it inside the __CTOR_{LIST,END}__ range. Linux has
> CONSTRUCTORS and *(.ctors) separate too, and (validly) puts
> .ctors inside the .init.data section.
> 
Yes, putting in .init.data is fine too. Actually these lines came from
linker manual.
> > +        QUAD(0)
> > +        __CTOR_END__ = .;
> > +#else
> > +        . = ALIGN(4);
> > +        __CTOR_LIST__ = .;
> > +        LONG((__CTOR_END__ - __CTOR_LIST__) / 4 - 2)
> > +        *(.ctors)
> > +        CONSTRUCTORS
> > +        LONG(0)
> > +        __CTOR_END__ = .;
> > +#endif
> 
> What case is this #else portion trying to address?
> 
A case (32 bit) which is no more supported by Xen, I''ll remove it.
> > --- /dev/null   Thu Jan 01 00:00:00 1970 +0000
> > +++ b/xen/common/gcov/gcov.c    Mon Jan 28 17:36:14 2013 +0000
> 
> Please decide for a consistent coding style in the newly added
> files - either all Linux or all Xen, but not a mixture of both.
> 
This file is copied verbatim from Linux cause it require few changes.
I think is better to convert everything to Xen style. Other parts will
require more changes.
> > +#ifdef CONFIG_COMPAT
> > +int compat_coverage_op(int op, int idx, XEN_GUEST_HANDLE_PARAM(void)
uarg, int arg2)
> > +{
> > +    return -EINVAL;
> > +}
> > +#endif
> 
> By suitably defining the interface structures you can avoid the
> need for a separate compat handler.
> 
How to do it?
> > +#ifdef TEST_COVERAGE
> > +void __init init_coverage(void);
> 
> No __init annotations in declarations please (they only belong on
> the actual definition).
> 
Ok
> Jan
> 
Frediano

>>> On 29.01.13 at 13:58, Frediano Ziglio
<frediano.ziglio@citrix.com> wrote:
> On Tue, 2013-01-29 at 10:41 +0000, Jan Beulich wrote:
>> >>> On 28.01.13 at 22:16, Frediano Ziglio
<frediano.ziglio@citrix.com> wrote:
>> > Do anybody know why I had to exclude %.init.o files to make it
compile?
>> 
>> Without you telling us what problems not doing so causes, no.
> 
> Xen didn''t compile. In xen/Rules.mk there is code that change if
> *.init.o do not contain some sections (text/data/bss).
Ah - you''d then have to decide whether these sections should
get converted (their names prefixed by .init.*) or whether these
files (as being init-only) would better be excluded from this
mechanism (which si sort of what you''re doing right now).
>> > +#ifdef CONFIG_COMPAT
>> > +int compat_coverage_op(int op, int idx,
XEN_GUEST_HANDLE_PARAM(void) uarg, int arg2)
>> > +{
>> > +    return -EINVAL;
>> > +}
>> > +#endif
>> 
>> By suitably defining the interface structures you can avoid the
>> need for a separate compat handler.
>> 
> 
> How to do it?
Just define all fields such that both size and alignment match
between 32- and 64-bit views on them.

Jan

On Tue, 2013-01-29 at 12:02 +0000, Frediano Ziglio
wrote:
> On Tue, 2013-01-29 at 10:56 +0000, Ian Campbell wrote:
> > On Mon, 2013-01-28 at 21:16 +0000, Frediano Ziglio wrote:
> > > From: Frediano Ziglio <frediano.ziglio@citrix.com>
> > > 
> > > This patch introduce coverage support to Xen.
> > > Currently it allows to compile Xen with coverage support but
there is no way
> > > to extract them.
> > > 
> > > The declarations came from Linux source files (as you can see
from file
> > > headers).
> > > 
> > > It also add a new hypercall (can somebody reserve a number for
this stuff?).
> > 
> > You can simply patch xen/include/public/xen.h to declare the new
> > __HYPERVISOR_foo_op. (which I now see you have done in this patch!).
If
> > you just want to reserve the number it is also allowed to send just
that
> > hunk to reserve a number pending the implementation.
> > 
> > BTW I''d suggest leaving the stub implementation out of the
patch until
> > you''ve decided what it will look like, so it returns ENOSYS
instead of
> > EINVAL (or change your stub to return ENOSYS).
> > 
> 
> Easy to change. Actually I discovered that my patch does not even
> compile if you disable TEST_COVERAGE (and it should be disabled by
> default too). EINVAL say that the hypercall is present but the value
> passed in invalid.
> 
> The reason why adding a new hypercall instead of a new sysctl is simply
> because is easier to have a zero cost if you disable coverage
> informations. The best thing would be redirect do_coverage_op to
> do_ni_hypercall using linker options but even two small stub would do
> (these stubs will return ENOSYS instead).
I don''t think you need to worry about performance of the NULL
do_coverage_op when coverage is disabled, that''s not a hot path..
> 
> > > The idea is to have some operations mainly
> > > - get filename
> > 
> > This is get a list of filenames (i.e. input for the next two)?
> > 
> 
> The idea was just to pass the index and get a filename or a ENOENT error
> (or a ENOBUF if buffer is too small). Actually you should scan the
> entire list to get a proper filename so this end up in a O(N^2)
> operation but as N is constant and not that big it should not be a
> problem. If it''s a problem I can implement a tree or simply reuse
> constructor space to store a simple array of gcov_info*.
What access pattern do you expect? Do you expect most users to just want
to get all the files (i.e. one by one, cycling through the indexes) or
would they be interested in a subset of the files?

I''d expect most users to scan the indexes at start of day to get the
list of filenames and then cache it and use it for the remainder of the
run.
> > > - get count informations for a file
> > > - reset counter for a specific file
> > 
> > How atomic do these need to be? Will it be necessary to obtain
> > consistent snapshots of the counters for multiple files? Even for a
> > single file I assume there will be many counters so getting a
consistent
> > snapshot at the file level might even be tricky?
> > 
> > Or perhaps since you are measuring coverage you only care about one
bit
> > per basic block (rather than a counter as such) and if you race then,
oh
> > well, you''ll see it next time.
> > 
> 
> They cannot be so atomic anyway as the hypervisor is running, I''d
not
> use a lock or similar. These informations are mainly statistical one.
> It''s not possible to disable incrementing any counter and also the
> increment is done without locking (so it''s possible to loose some
> number). Unless you pause all machines and stop any userspace program
Yes, that''s what I was wondering -- if it would be necessary to quiesce
the system (which probably means holding all of the pCPUS in some known
good state briefly, similar to Linux''s stop_machine() stuff) while
reading the counters. Fortunately it sounds like this won''t be
necessary.
> that is doing some activities with Xen you''ll have to consider
some
> dirty counter. Perhaps would be better to have an operation to
> query/reset more filenames at a time.
Read and reset for a single file might be sufficient? Really all this
depends on what the userspace side needs -- which is something I think
only you can answer.
>  But even in Linux you have to
> "cat" different files to get informations.
OK.
> > > - reset all counters
> > > 
> > > The op parameter in the hypercall will be the operation.
> > > The idx parameter is the index to the file.
> > > The arg1 is the argument (a buffer pointer for filename or
counters).
> > > The arg2 probably will be the size of previous buffer.
> > 
> > Often the best way to deal with arg1/arg2 would be to pass a union of
> > the types specific to each operation.
> > 
> 
> In this case arg1 if the buffer while arg2 would be mainly the length of
> arg1 so they cannot be an union.
> 
> > But how does the caller determine the size of the buffer? I guess it
> > various from file to file? Is this what comes from "get count
> > informations for a file"?
> > 
> 
> Well.. pass a large enough buffer and handle ENOBUF correctly.
I think it would be much better if you provided a way to query the
required buffer size (for each file if necessary). It would make sense
to me to do this in the same interface as the one which returns the
filename.
> > > Do you think I should pack these parameters and pass a single
pointer?
> > > 
> > > Linux use a file system to export these information. I would use
the same
> > > format as Linux use so to make it easy to reuse tools from Linux
(like lcov).
> > > That the reason why I split get filename from get counter
informations.
> > 
> > You could also synthesize this at the tools layer. I don''t
know enough
> > of how this stuff works to say what makes sense at the h/v layer.
> > 
> > What form does this information take internally?
> > 
> 
> They are binary files. The format is defined by gcc. On userspace you
> get a single .gcda file for each compile unit. This file is updated when
> any program exit (multiple execution update the files). Gcc generate
> also .gcno files. You need both .gcda and .gcno at the end to handle
> coverage information.
I meant what form does the data take internally to Xen.

My point was that since you need a Xen specific userspace tool to
extract the information from the hypervisor *it* could be the thing
which provides the same format as Linux uses, which need not necessarily
imply that the representation inside the hypervisor or at the hypercall
API looks the same. It might be easier for the hypervisor to provide a
relatively unfiltered view on the internal data and have the tools fix
it up into something which looks like the same format as Linux.
> > > Do anybody know why I had to exclude %.init.o files to make it
compile?
> > > Are these files used before Xen start?
> > 
> > They are used during start of day bring up and then discarded once Xen
> > is up and running. I''m not sure what that would affect the
link,
> > although it would obviously be disastrous if you were to touch the
> > counters associated with those files again after they were thrown
away.
> > 
> 
> The problem would be reading (or resetting) these counter if discarded,
> probably would lead to some buffer overwriting which is not good.
> Actually these modules have no such informations so it''s not a
problem.
If you cared about boot time coverage then I suppose you could save off
the counters just before the point at which Xen frees the init sections
-- after that point they certainly won''t be changing.

But I think skipping them until someone wants to profile that bit is OK
too.
> > We might have some checks in the tree to ensure that non-init code
> > cannot reference init code by mistake -- maybe that''s what
broke the
> > build for you?
> > 
> 
> Yes, I think it''s safer to disable for them (as it actually).
> 
> Are TEST_COVERAGE a good names or would be better to have something like
> CONFIG_ENABLE_COVERAGE (do you use any prefix for configure stuff?) ?
I''d model it on the perfc stuff in xen/Rules.mk or the debug stuff in
Config.mk I think, so you would do "make dist-xen coverage=y" and this
would turn into -DCOVERAGE or something. The CONFIG_* stuff tends to
come from xen/include/asm-*/config.h (TBH I''m not sure it''s
terribly
consistent, or maybe there''s a rule/pattern I don''t know about
...)

Ian.

On Tue, Jan 29, 2013 at 12:02:55PM +0000, Frediano Ziglio
wrote:
> On Tue, 2013-01-29 at 10:56 +0000, Ian Campbell wrote:
> > On Mon, 2013-01-28 at 21:16 +0000, Frediano Ziglio wrote:
> > > From: Frediano Ziglio <frediano.ziglio@citrix.com>
> > > 
> > > This patch introduce coverage support to Xen.
> > > Currently it allows to compile Xen with coverage support but
there is no way
> > > to extract them.
> > > 
> > > The declarations came from Linux source files (as you can see
from file
> > > headers).
> > > 
> > > It also add a new hypercall (can somebody reserve a number for
this stuff?).
> > 
> > You can simply patch xen/include/public/xen.h to declare the new
> > __HYPERVISOR_foo_op. (which I now see you have done in this patch!).
If
> > you just want to reserve the number it is also allowed to send just
that
> > hunk to reserve a number pending the implementation.
> > 
> > BTW I''d suggest leaving the stub implementation out of the
patch until
> > you''ve decided what it will look like, so it returns ENOSYS
instead of
> > EINVAL (or change your stub to return ENOSYS).
> > 
> 
> Easy to change. Actually I discovered that my patch does not even
> compile if you disable TEST_COVERAGE (and it should be disabled by
> default too). EINVAL say that the hypercall is present but the value
> passed in invalid.
> 
> The reason why adding a new hypercall instead of a new sysctl is simply
> because is easier to have a zero cost if you disable coverage
> informations. The best thing would be redirect do_coverage_op to
> do_ni_hypercall using linker options but even two small stub would do
> (these stubs will return ENOSYS instead).
I am not sure I follow. Is the sysctl hypercall code path "longer"
than
the hypercall path you are introducing? What is the zero cost?

On Wed, 2013-01-30 at 21:34 +0000, Konrad Rzeszutek Wilk
wrote:
> > The reason why adding a new hypercall instead of a new sysctl is
simply
> > because is easier to have a zero cost if you disable coverage
> > informations. The best thing would be redirect do_coverage_op to
> > do_ni_hypercall using linker options but even two small stub would do
> > (these stubs will return ENOSYS instead).
> 
> I am not sure I follow. Is the sysctl hypercall code path
"longer" than
> the hypercall path you are introducing? What is the zero cost?
I don''t think we care a jot about the performance of this system call
when coverage is disabled, it''s certainly not a hot path and in any
case
if it is a NOP it doesn''t really matter anyway.

Ian.

On Wed, 2013-01-30 at 16:34 -0500, Konrad Rzeszutek Wilk
wrote:
> On Tue, Jan 29, 2013 at 12:02:55PM +0000, Frediano Ziglio wrote:
> > On Tue, 2013-01-29 at 10:56 +0000, Ian Campbell wrote:
> > > On Mon, 2013-01-28 at 21:16 +0000, Frediano Ziglio wrote:
> > > > From: Frediano Ziglio <frediano.ziglio@citrix.com>
> > > > 
> > > > This patch introduce coverage support to Xen.
> > > > Currently it allows to compile Xen with coverage support but
there is no way
> > > > to extract them.
> > > > 
> > > > The declarations came from Linux source files (as you can
see from file
> > > > headers).
> > > > 
> > > > It also add a new hypercall (can somebody reserve a number
for this stuff?).
> > > 
> > > You can simply patch xen/include/public/xen.h to declare the new
> > > __HYPERVISOR_foo_op. (which I now see you have done in this
patch!). If
> > > you just want to reserve the number it is also allowed to send
just that
> > > hunk to reserve a number pending the implementation.
> > > 
> > > BTW I''d suggest leaving the stub implementation out of
the patch until
> > > you''ve decided what it will look like, so it returns
ENOSYS instead of
> > > EINVAL (or change your stub to return ENOSYS).
> > > 
> > 
> > Easy to change. Actually I discovered that my patch does not even
> > compile if you disable TEST_COVERAGE (and it should be disabled by
> > default too). EINVAL say that the hypercall is present but the value
> > passed in invalid.
> > 
> > The reason why adding a new hypercall instead of a new sysctl is
simply
> > because is easier to have a zero cost if you disable coverage
> > informations. The best thing would be redirect do_coverage_op to
> > do_ni_hypercall using linker options but even two small stub would do
> > (these stubs will return ENOSYS instead).
> 
> I am not sure I follow. Is the sysctl hypercall code path
"longer" than
> the hypercall path you are introducing? What is the zero cost?
Require more changes to code already present and more compiled in code.
Yes, perhaps I''m too paranoid, I''m talking about a bunch of
bytes never
executed.

Frediano

On Thu, 2013-01-31 at 08:51 +0000, Ian Campbell wrote:
> On Wed, 2013-01-30 at 21:34 +0000, Konrad Rzeszutek Wilk wrote:
> > > The reason why adding a new hypercall instead of a new sysctl is
simply
> > > because is easier to have a zero cost if you disable coverage
> > > informations. The best thing would be redirect do_coverage_op to
> > > do_ni_hypercall using linker options but even two small stub
would do
> > > (these stubs will return ENOSYS instead).
> > 
> > I am not sure I follow. Is the sysctl hypercall code path
"longer" than
> > the hypercall path you are introducing? What is the zero cost?
> 
> I don''t think we care a jot about the performance of this system
call
> when coverage is disabled, it''s certainly not a hot path and in
any case
> if it is a NOP it doesn''t really matter anyway.
> 
> Ian.
> 
It''s not about the speed, it''s about the bytes introduced in
Xen binary.

Frediano

On Fri, Feb 01, 2013 at 02:29:04PM +0000, Frediano Ziglio
wrote:
> On Thu, 2013-01-31 at 08:51 +0000, Ian Campbell wrote:
> > On Wed, 2013-01-30 at 21:34 +0000, Konrad Rzeszutek Wilk wrote:
> > > > The reason why adding a new hypercall instead of a new
sysctl is simply
> > > > because is easier to have a zero cost if you disable
coverage
> > > > informations. The best thing would be redirect
do_coverage_op to
> > > > do_ni_hypercall using linker options but even two small stub
would do
> > > > (these stubs will return ENOSYS instead).
> > > 
> > > I am not sure I follow. Is the sysctl hypercall code path
"longer" than
> > > the hypercall path you are introducing? What is the zero cost?
> > 
> > I don''t think we care a jot about the performance of this
system call
> > when coverage is disabled, it''s certainly not a hot path and
in any case
> > if it is a NOP it doesn''t really matter anyway.
> > 
> > Ian.
> > 
> 
> It''s not about the speed, it''s about the bytes introduced
in Xen binary.
Not sure I follow. What bytes? Just that the code size is bigger b/c it
will go through the sysctl functions? How many bytes of extra code are
talking here?

On Fri, 2013-02-01 at 09:46 -0500, Konrad Rzeszutek Wilk
wrote:
> On Fri, Feb 01, 2013 at 02:29:04PM +0000, Frediano Ziglio wrote:
> > On Thu, 2013-01-31 at 08:51 +0000, Ian Campbell wrote:
> > > On Wed, 2013-01-30 at 21:34 +0000, Konrad Rzeszutek Wilk wrote:
> > > > > The reason why adding a new hypercall instead of a new
sysctl is simply
> > > > > because is easier to have a zero cost if you disable
coverage
> > > > > informations. The best thing would be redirect
do_coverage_op to
> > > > > do_ni_hypercall using linker options but even two small
stub would do
> > > > > (these stubs will return ENOSYS instead).
> > > > 
> > > > I am not sure I follow. Is the sysctl hypercall code path
"longer" than
> > > > the hypercall path you are introducing? What is the zero
cost?
> > > 
> > > I don''t think we care a jot about the performance of
this system call
> > > when coverage is disabled, it''s certainly not a hot path
and in any case
> > > if it is a NOP it doesn''t really matter anyway.
> > > 
> > > Ian.
> > > 
> > 
> > It''s not about the speed, it''s about the bytes
introduced in Xen binary.
> 
> Not sure I follow. What bytes? Just that the code size is bigger b/c it
> will go through the sysctl functions? How many bytes of extra code are
> talking here? 
Probably less than 20...

In do_sysctl something like

    case XEN_SYSCTL_coverage_op:
        ret = coverage_op(&op->u.coverage_op);
        break;

where when disabled coverage_op should be declared as

static inline long coverage_op(struct xen_sysvtl_coverage_op *op)
{
    return -ENOSYS;
}

Frediano

On Fri, Feb 01, 2013 at 03:05:23PM +0000, Frediano Ziglio
wrote:
> On Fri, 2013-02-01 at 09:46 -0500, Konrad Rzeszutek Wilk wrote:
> > On Fri, Feb 01, 2013 at 02:29:04PM +0000, Frediano Ziglio wrote:
> > > On Thu, 2013-01-31 at 08:51 +0000, Ian Campbell wrote:
> > > > On Wed, 2013-01-30 at 21:34 +0000, Konrad Rzeszutek Wilk
wrote:
> > > > > > The reason why adding a new hypercall instead of a
new sysctl is simply
> > > > > > because is easier to have a zero cost if you
disable coverage
> > > > > > informations. The best thing would be redirect
do_coverage_op to
> > > > > > do_ni_hypercall using linker options but even two
small stub would do
> > > > > > (these stubs will return ENOSYS instead).
> > > > > 
> > > > > I am not sure I follow. Is the sysctl hypercall code
path "longer" than
> > > > > the hypercall path you are introducing? What is the
zero cost?
> > > > 
> > > > I don''t think we care a jot about the performance
of this system call
> > > > when coverage is disabled, it''s certainly not a hot
path and in any case
> > > > if it is a NOP it doesn''t really matter anyway.
> > > > 
> > > > Ian.
> > > > 
> > > 
> > > It''s not about the speed, it''s about the bytes
introduced in Xen binary.
> > 
> > Not sure I follow. What bytes? Just that the code size is bigger b/c
it
> > will go through the sysctl functions? How many bytes of extra code are
> > talking here? 
> 
> Probably less than 20...
> 
> In do_sysctl something like
> 
>     case XEN_SYSCTL_coverage_op:
>         ret = coverage_op(&op->u.coverage_op);
>         break;
> 
> where when disabled coverage_op should be declared as
> 
> static inline long coverage_op(struct xen_sysvtl_coverage_op *op)
> {
>     return -ENOSYS;
> }
Ok. Then it looks like XEN_SYSCTL_* it is the right place.