Am Wed, 5 Apr 2017 08:59:18 +0200 schrieb Jan Kiszka <[email protected]>:
> On 2017-04-03 14:00, Henning Schild wrote: > > With the new feature to download the used firmware out of sysfs and > > a way to get GCOV data into that. It is now time to extract this > > information and generate .gcda files for higher level tools to work > > with. > > Instead of dealing with the details of that just use what the > > compiler gives us when we link to its gcov lib. > > > > Signed-off-by: Henning Schild <[email protected]> > > > > diff --git a/.gitignore b/.gitignore > > --- a/.gitignore > > +++ b/.gitignore > > @@ -4,6 +4,7 @@ > > *.cmd > > *.bin > > *.gcno > > +*.gcda > > .tmp_versions > > *.dtb > > *.dtb.S > > @@ -14,6 +15,7 @@ driver/jailhouse.ko > > hypervisor/include/jailhouse/config.h > > hypervisor/hypervisor.lds > > tools/jailhouse > > +tools/jailhouse-gcov-extract > > tools/jailhouse-config-collect > > configs/*.cell > > Documentation/generated > > diff --git a/Kbuild b/Kbuild > > --- a/Kbuild > > +++ b/Kbuild > > @@ -45,6 +45,6 @@ subdir-ccflags-y := -Werror > > > > # directory dependencies on generated files > > $(obj)/driver $(obj)/hypervisor: $(VERSION_H) > > -$(obj)/hypervisor $(obj)/inmates $(obj)/driver: $(CONFIG_MK) > > +$(obj)/hypervisor $(obj)/inmates $(obj)/driver $(obj)/tools: > > $(CONFIG_MK) > > clean-dirs := Documentation/generated hypervisor/include/generated > > diff --git a/Makefile b/Makefile > > --- a/Makefile > > +++ b/Makefile > > @@ -49,7 +49,7 @@ tool_inmates_install: $(DESTDIR)$(libexe > > $(INSTALL_DATA) inmates/tools/$(ARCH)/*.bin $< > > > > install: modules_install firmware_install tool_inmates_install > > - $(Q)$(MAKE) -C tools $@ src=. > > + $(Q)$(MAKE) -C tools $@ src=. obj=. HACK! but jailhouse does not support out-of-its-own-tree anyways ... more on that later. Introducing the "src=." was a hack already. > > .PHONY: modules_install install clean firmware_install modules > > tools docs \ docs_clean > > diff --git a/tools/Makefile b/tools/Makefile > > --- a/tools/Makefile > > +++ b/tools/Makefile > > @@ -12,6 +12,7 @@ > > > > # includes installation-related variables and definitions > > include $(src)/../scripts/include.mk > > +include $(obj)/../hypervisor/include/generated/config.mk Include we need the HACK for. > > LD = $(CC) > > NOSTDINC_FLAGS := > > @@ -23,6 +24,7 @@ LDFLAGS := > > GCOV_PROFILE := n > > > > BINARIES := jailhouse > > + > > HELPERS := \ > > jailhouse-cell-linux \ > > jailhouse-cell-stats \ > > @@ -58,6 +60,20 @@ targets += jailhouse.o > > $(obj)/jailhouse: $(obj)/jailhouse.o > > $(call if_changed,ld) > > > > +ifdef CONFIG_JAILHOUSE_GCOV > > Did I ask this before? What speaks against building this tool > unconditionally? Yes we can just always build and install that. In that case the include config.mk would go away and the HACK would not be required. > > +CFLAGS_jailhouse-gcov-extract.o := > > -I$(src)/../hypervisor/include \ > > + -I$(src)/../hypervisor/arch/$(SRCARCH)/include > > +# add that just to LDFLAGS, otherwise we would profile the tool as > > well +LDFLAGS_jailhouse-gcov-extract := --coverage > > Would that be a problem? Do we want that for the main jailhouse tool? I do not see a problem. The tool should just do nothing. I will look into that. Maybe linking to the gcov lib limits the choice of toolchains. For the integration into "tools/jailhouse" i would put the code into that tool directly. No seperate binary and execve. > > + > > +targets += jailhouse-gcov-extract.o > > +BINARIES += jailhouse-gcov-extract > > +always += jailhouse-gcov-extract > > + > > +$(obj)/jailhouse-gcov-extract: $(obj)/jailhouse-gcov-extract.o > > + $(call if_changed,ld) > > +endif > > + > > $(obj)/jailhouse-config-collect: $(src)/jailhouse-config-create > > $(src)/jailhouse-config-collect.tmpl $(call if_changed,gen_collect) > > > > diff --git a/tools/jailhouse-gcov-extract.c > > b/tools/jailhouse-gcov-extract.c new file mode 100644 > > --- /dev/null > > +++ b/tools/jailhouse-gcov-extract.c > > @@ -0,0 +1,191 @@ > > +/* > > + * Jailhouse, a Linux-based partitioning hypervisor > > + * > > + * Copyright (c) Siemens AG, 2017 > > + * > > + * Authors: > > + * Henning Schild <[email protected]> > > + * > > + * This work is licensed under the terms of the GNU GPL, version > > 2. See > > + * the COPYING file in the top-level directory. > > + */ > > + > > +#include <stdio.h> > > +#include <sys/types.h> > > +#include <sys/stat.h> > > +#include <sys/mman.h> > > +#include <fcntl.h> > > +#include <errno.h> > > +#include <error.h> > > +#include <unistd.h> > > +#include <assert.h> > > +#include <stdlib.h> > > +#include <string.h> > > +#include <jailhouse/header.h> > > +#include <asm/jailhouse_header.h> > > + > > +#if (__GNUC__ < 4) || (__GNUC__ == 4 && __GNUC_MINOR__ < 7) > > +// does anyone still care about that ? > > At least we care about bailing out when building with an unsupported > toolchain - just remove the comment. The #error is there to stay. The comment is an open question whether support for the older format needs to be added. > > +#error "Gcov format of gcc < 4.7 is not supported!" > > +#endif > > + > > +/* > > + * the following bits are heavily inspired by > > linux/kernel/gcov/gcc_4.7.c > > + * with some slight modification > > + */ > > +#if BITS_PER_LONG >= 64 > > +typedef long gcov_type; > > +#else > > +typedef long long gcov_type; > > +#endif > > + > > +#if (__GNUC__ > 5) || (__GNUC__ == 5 && __GNUC_MINOR__ >= 1) > > +#define GCOV_COUNTERS 10 > > +#elif __GNUC__ == 4 && __GNUC_MINOR__ >= 9 > > +#define GCOV_COUNTERS 9 > > +#else > > +#define GCOV_COUNTERS 8 > > +#endif > > + > > +struct gcov_ctr_info { > > + unsigned int num; > > + gcov_type *values; > > +}; > > + > > +struct gcov_fn_info { > > + struct gcov_info *key; > > + unsigned int ident; > > + unsigned int lineno_checksum; > > + unsigned int cfg_checksum; > > + struct gcov_ctr_info ctrs[0]; > > +}; > > + > > +struct gcov_info { > > + unsigned int version; > > + struct gcov_info *next; > > + unsigned int stamp; > > + char *filename; > > + void (*merge[GCOV_COUNTERS])(gcov_type *, unsigned int); > > + unsigned int n_functions; > > + struct gcov_fn_info **functions; > > +}; > > +/* > > + * end of linux/kernel/gcov/gcc_4.7.c > > + */ > > + > > +static void *hypervisor; > > +static size_t hypervisor_size; > > +// TODO > > +// do we care about the other possible merge functions from > > libgcc? > > Can you clarify this? Ideally before merging. __gcov_merge_add will add current counter values to existing .gcda-files. If the files do not exist you just get new ones with the absolute values. If you have multiple runs you get multiple sets of counter values and can choose how to merge them together. __gcov_merge_add is the default but there are others https://github.com/gcc-mirror/gcc/blob/master/libgcc/libgcov-merge.c I guess we are ok with __gcov_merge_add. If one needs to merge multiple runs together i.e. gcov-tool should be used on multiple sets of gcda-files. http://man7.org/linux/man-pages/man1/gcov-tool.1.html But i am not sure whether gcov-tool supports all the fancy merge functions that libgcc does, or which ones we truly care about. For the first version i guess just supporting __gcov_merge_add is fine, the comment should go away. > > +extern void __gcov_merge_add(gcov_type *counters, unsigned int > > n_counters); +extern void __gcov_init(struct gcov_info *); > > +extern void __gcov_dump(); > > + > > + > > +static void *hypervisor2current(void *hvp) > > +{ > > + unsigned long hvaddr = (unsigned long)hvp; > > + void *ret; > > + > > + if(hvp == NULL) > > Coding style: if (hyp... > > > + return NULL; > > + assert(hvaddr >= JAILHOUSE_BASE && > > + hvaddr < JAILHOUSE_BASE + hypervisor_size); > > + ret = (void *)(hvaddr - JAILHOUSE_BASE + (unsigned > > long)hypervisor); > > + > > + return ret; > > +} > > + > > +/* > > + * translate one gcov-"tree" from the hypervisor address space to > > the current > > + * addresses > > + */ > > +static void translate_all_pointers(struct gcov_info *info) > > +{ > > + struct gcov_fn_info *fn_info; > > + struct gcov_ctr_info *ctr_info; > > + unsigned int i, j, active; > > + > > + info->next = hypervisor2current(info->next); > > + info->filename = hypervisor2current(info->filename); > > + active = 0; > > + for (i = 0; i < GCOV_COUNTERS; i++) { > > + if (info->merge[i]) { > > + active++; > > + info->merge[i] = &__gcov_merge_add; > > + } else > > + break; > > if (!info->merge[i]) > break; > ... > > > + } > > + info->functions = hypervisor2current(info->functions); > > + for (i = 0; i < info->n_functions; i++) { > > + info->functions[i] = > > hypervisor2current(info->functions[i]); > > + fn_info = info->functions[i]; > > + if (fn_info) { > > + fn_info->key = > > hypervisor2current(fn_info->key); > > + assert(fn_info->key == info); > > + for (j = 0; j < active; j++) { > > + ctr_info = fn_info->ctrs + j; > > + ctr_info->values = > > + > > hypervisor2current(ctr_info->values); > > + } > > + } > > + } > > +} > > + > > +int main(int argc, char **argv) > > +{ > > + struct gcov_info *gcov_info_head, *info, *next; > > + struct jailhouse_header *header; > > + struct stat sbuf; > > + char *filename; > > + int fd, ret; > > + > > + if (argc != 2) { > > + errno = EINVAL; > > + goto out; > > + } > > + filename = argv[1]; > > + fd = open(filename, O_RDONLY); > > + if (fd < 1) > > + goto out; > > + > > + ret = fstat(fd, &sbuf); > > + if (ret) > > + goto out; > > + hypervisor_size = sbuf.st_size; > > + hypervisor = mmap(NULL, hypervisor_size, PROT_READ | > > PROT_WRITE, > > + MAP_PRIVATE, fd, 0); > > If mmap is not supported on sysfs, how about reading the file into a > buffer here? Would remove the need to copy it around. We are talking > about less than a megabyte. Well i could implement mmap for sysfs but wanted to keep the driver simple. Good idea, i will fetch a copy here and save the argument and the cp. > > + if (hypervisor == MAP_FAILED) > > + goto out_fd; > > + > > + header = (struct jailhouse_header *)hypervisor; > > + if (strcmp(header->signature, JAILHOUSE_SIGNATURE)) { > > + errno = EINVAL; > > + error(0, 0, "%s does not seem to be a hypervisor > > image", > > + filename); > > + goto out_m; > > + } > > + > > + gcov_info_head = > > hypervisor2current(header->gcov_info_head); > > + info = gcov_info_head; > > + for (info = gcov_info_head; info; info = info->next) > > + translate_all_pointers(info); > > + > > + for (info = gcov_info_head; info;) > > + { > > Coding style. > > > + // remember next because __gcov_init changes it > > Is that a TODO? If not, please avoid //-style comments. No TODO, will fix that. > > + next = info->next; > > + __gcov_init(info); > > + info = next; > > + } > > + __gcov_dump(); > > + > > +out_m: > > + munmap(hypervisor, hypervisor_size); > > +out_fd: > > + close(fd); > > +out: > > + if (errno) > > + error(errno, errno, " "); > > + return 0; > > +} > > > > Jan > -- You received this message because you are subscribed to the Google Groups "Jailhouse" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. For more options, visit https://groups.google.com/d/optout.
