Adding support for coverage informations

Quoted 1 line

Quoted 1 line · Frediano Ziglio

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?

Quoted 8 lines

Quoted 1 line

Quoted 1 line · Frediano Ziglio

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).

Quoted 7 lines

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...

Quoted 6 lines

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.

Quoted 5 lines

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.

Quoted 3 lines

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?

Quoted 4 lines

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.

Quoted 1 line

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).

Quoted 11 lines

xen/arch/x86/xen.lds.S Mon Jan 28 17:36:14 2013 +0000 renamed from xen/arch/x86/xen.lds.S Tue Jan 22 09:33:10 2013 +0100 +4 -1

@@ -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)

xen/arch/x86/xen.lds.S Mon Jan 28 17:36:14 2013 +0000 SECTIONS · line 86

Quoted 2 lines

xen/arch/x86/xen.lds.S Mon Jan 28 17:36:14 2013 +0000 renamed from xen/arch/x86/xen.lds.S Tue Jan 22 09:33:10 2013 +0100 +2 -0

+        *(.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.

Quoted 5 lines

xen/arch/x86/xen.lds.S Mon Jan 28 17:36:14 2013 +0000 renamed from xen/arch/x86/xen.lds.S Tue Jan 22 09:33:10 2013 +0100 +5 -0

+        QUAD(0)
+        __CTOR_END__ = .;
+#else
+        . = ALIGN(4);
+        __CTOR_LIST__ = .;

xen/arch/x86/xen.lds.S Mon Jan 28 17:36:14 2013 +0000 SECTIONS · line 97

Quoted 6 lines

xen/arch/x86/xen.lds.S Mon Jan 28 17:36:14 2013 +0000 renamed from xen/arch/x86/xen.lds.S Tue Jan 22 09:33:10 2013 +0100 +6 -0

+        LONG((__CTOR_END__ - __CTOR_LIST__) / 4 - 2)
+        *(.ctors)
+        CONSTRUCTORS
+        LONG(0)
+        __CTOR_END__ = .;
+#endif

What case is this #else portion trying to address?

xen/common/gcov/gcov.c Mon Jan 28 17:36:14 2013 +0000

Quoted 2 lines

xen/common/gcov/gcov.c Mon Jan 28 17:36:14 2013 +0000 renamed from dev/null Thu Jan 01 00:00:00 1970 +0000 +0 -0

Please decide for a consistent coding style in the newly added files - either all Linux or all Xen, but not a mixture of both.

xen/common/gcov/gcov.c Mon Jan 28 17:36:14 2013 +0000 line 95

Quoted 6 lines

xen/common/gcov/gcov.c Mon Jan 28 17:36:14 2013 +0000 renamed from dev/null Thu Jan 01 00:00:00 1970 +0000 +6 -0

+#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.

xen/common/gcov/gcov.c Mon Jan 28 17:36:14 2013 +0000

Quoted 2 lines

xen/common/gcov/gcov.c Mon Jan 28 17:36:14 2013 +0000 renamed from dev/null Thu Jan 01 00:00:00 1970 +0000 +2 -0

+#ifdef TEST_COVERAGE
+void __init init_coverage(void);

No __init annotations in declarations please (they only belong on the actual definition).

Jan

Quoted 1 line

Quoted 1 line · Frediano Ziglio

Quoted 4 lines · Jan Beulich

Quoted 2 lines

Quoted 1 line

Quoted 1 line · Frediano Ziglio

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).

Quoted 10 lines

xen/common/gcov/gcov.c Mon Jan 28 17:36:14 2013 +0000 line 95

Quoted 6 lines

xen/common/gcov/gcov.c Mon Jan 28 17:36:14 2013 +0000 renamed from dev/null Thu Jan 01 00:00:00 1970 +0000 +6 -0

+#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?

From: Frediano Ziglio <[email protected]>

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.

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

xen/arch/x86/x86_64/compat/entry.S Mon Jan 28 17:36:14 2013 +0000 renamed from xen/arch/x86/x86_64/compat/entry.S Tue Jan 22 09:33:10 2013 +0100 +8 -0

+typedef void (*ctor_func_t)(void);
+extern struct {
+    unsigned long count;
+    ctor_func_t funcs[1];
+} __CTOR_LIST__;
+
+void __init init_coverage(void)
+{

diff -r 5af4f2ab06f3 xen/include/xen/gcov.h

xen/include/xen/gcov.h Mon Jan 28 17:36:14 2013 +0000 renamed from dev/null Thu Jan 01 00:00:00 1970 +0000 +1 -0

@@ -0,0 +1,141 @@
+/*

Quoted 1 line

Quoted 1 line · Ian Campbell

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?

Quoted 4 lines

Quoted 1 line

Quoted 1 line · Ian Campbell

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

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

Quoted 1 line

Quoted 1 line · Andrew Cooper

Quoted 5 lines · Jan Beulich

Quoted 4 lines

Quoted 1 line

Quoted 1 line · Ian Campbell

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?

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.

Quoted 10 lines · Frediano Ziglio

From: Frediano Ziglio <[email protected]>

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).

Quoted 2 lines

The idea is to have some operations mainly
- get filename

This is get a list of filenames (i.e. input for the next two)?

Quoted 2 lines

- 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.

Quoted 6 lines

- 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?

Quoted 5 lines

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?

Quoted 2 lines

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?

Config.mk Mon Jan 28 17:36:14 2013 +0000 QEMU_TAG ?= 2a1354d655d816feaad7dbdb8364 · line 233

Quoted 12 lines

Signed-off-by Frediano Ziglio <[email protected]>

diff -r 5af4f2ab06f3 Config.mk

Config.mk Mon Jan 28 17:36:14 2013 +0000 renamed from Config.mk Tue Jan 22 09:33:10 2013 +0100 +3 -0

@@ -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.

xen/arch/x86/x86_64/compat/entry.S Mon Jan 28 17:36:14 2013 +0000 ENTRY(compat_hypercall_table) · line 417

Quoted 9 lines

diff -r 5af4f2ab06f3 xen/arch/x86/x86_64/compat/entry.S

xen/arch/x86/x86_64/compat/entry.S Mon Jan 28 17:36:14 2013 +0000 renamed from xen/arch/x86/x86_64/compat/entry.S Tue Jan 22 09:33:10 2013 +0100 +2 -0

@@ -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().

Quoted 8 lines

xen/arch/x86/x86_64/compat/entry.S Mon Jan 28 17:36:14 2013 +0000 renamed from xen/arch/x86/x86_64/compat/entry.S Tue Jan 22 09:33:10 2013 +0100 +8 -0

+typedef void (*ctor_func_t)(void);
+extern struct {
+    unsigned long count;
+    ctor_func_t funcs[1];
+} __CTOR_LIST__;
+
+void __init init_coverage(void)
+{

xen/arch/x86/x86_64/compat/entry.S Mon Jan 28 17:36:14 2013 +0000 ENTRY(compat_hypercall_table)

Quoted 10 lines

xen/arch/x86/x86_64/compat/entry.S Mon Jan 28 17:36:14 2013 +0000 renamed from xen/arch/x86/x86_64/compat/entry.S Tue Jan 22 09:33:10 2013 +0100 +10 -0

+    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?

Quoted 9 lines

diff -r 5af4f2ab06f3 xen/include/xen/gcov.h

xen/include/xen/gcov.h Mon Jan 28 17:36:14 2013 +0000 renamed from dev/null Thu Jan 01 00:00:00 1970 +0000 +5 -0

@@ -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

xen/include/xen/gcov.h Mon Jan 28 17:36:14 2013 +0000 line 11

Quoted 6 lines

xen/include/xen/gcov.h Mon Jan 28 17:36:14 2013 +0000 renamed from dev/null Thu Jan 01 00:00:00 1970 +0000 +6 -0

+ *  understanding, refer to gcc source: gcc/gcov-io.h.
+ *
+ *    Copyright IBM Corp. 2009
+ *    Author(s): Peter Oberparleiter <[email protected]>
+ *
+ *    Uses gcc-internal data definitions.

Any idea what the license on these is?

Are these interfaces subject to change with different gcc versions?

Ian.

Quoted 12 lines · Ian Campbell

Quoted 10 lines · Frediano Ziglio

From: Frediano Ziglio <[email protected]>

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?).

Quoted 9 lines

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).

Quoted 6 lines

Quoted 3 lines

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*.

Quoted 12 lines

Quoted 2 lines

- 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.

Quoted 10 lines

Quoted 6 lines

- 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.

Quoted 4 lines

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.

Quoted 9 lines

Quoted 5 lines

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.

Quoted 2 lines

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.

Quoted 8 lines

Quoted 2 lines

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.

Quoted 4 lines

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?) ?

Quoted 12 lines · Ian Campbell

Quoted 10 lines · Frediano Ziglio

From: Frediano Ziglio <[email protected]>

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?).

Quoted 9 lines

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).

Quoted 5 lines

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?

Quoted 6 lines · Konrad Rzeszutek Wilk

Quoted 5 lines

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).

Quoted 2 lines

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.

Quoted 10 lines · Ian Campbell

Quoted 8 lines · Konrad Rzeszutek Wilk

Quoted 5 lines

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?

Quoted 6 lines

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.

Quoted 15 lines · Frediano Ziglio

Quoted 14 lines · Ian Campbell

Quoted 8 lines · Konrad Rzeszutek Wilk

Quoted 5 lines

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.

Quoted 4 lines

Quoted 2 lines

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?

Quoted 16 lines · Konrad Rzeszutek Wilk

Quoted 15 lines · Frediano Ziglio

Quoted 14 lines · Ian Campbell

Quoted 8 lines · Konrad Rzeszutek Wilk

Quoted 5 lines

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.

Quoted 8 lines

Quoted 4 lines

Quoted 2 lines

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;
}

Quoted 28 lines · Frediano Ziglio

Quoted 12 lines · Ian Campbell

Quoted 10 lines · Frediano Ziglio

From: Frediano Ziglio <[email protected]>

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?).

Quoted 9 lines

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.

Quoted 5 lines

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?