Am Tue, 28 Feb 2017 11:01:54 +0100 schrieb Jan Kiszka <[email protected]>:
> Subject: -> "introduce" > > On 2017-02-27 16:40, Henning Schild wrote: > > This introduces pretty minimal GCOV support. The hypervisor does not > > need to understand much of that. It just needs to call the init > > functions to gather the addresses of the gcov related data > > structures. These are just linked together so something outside the > > hypervisor can look at them later. > > > > Signed-off-by: Henning Schild <[email protected]> > > > > diff --git a/driver/main.c b/driver/main.c > > --- a/driver/main.c > > +++ b/driver/main.c > > @@ -419,6 +419,7 @@ > > > > header = (struct jailhouse_header *)hypervisor_mem; > > header->max_cpus = max_cpus; > > + header->jailhouse_base = (void *)JAILHOUSE_BASE; > > Can be done during hypervisor compile time. Then you also do not need > patch 7. > > > > > hypervisor_size = hypervisor->size; > > err = jailhouse_sysfs_firmware_init(jailhouse_dev); > > diff --git a/hypervisor/Makefile b/hypervisor/Makefile > > --- a/hypervisor/Makefile > > +++ b/hypervisor/Makefile > > @@ -34,6 +34,14 @@ > > > > CORE_OBJECTS = setup.o printk.o paging.o control.o lib.o mmio.o > > pci.o ivshmem.o > > +## for now "make CONFIG_JAILHOUSE_GCOV=y" > > +## using config.(h|mk) does not work on this level > > +## probably move config.h->mk up one dir > > +ifdef CONFIG_JAILHOUSE_GCOV > > +CORE_OBJECTS += gcov.o > > +endif > > +ccflags-$(CONFIG_JAILHOUSE_GCOV) += --coverage > > -DCONFIG_JAILHOUSE_GCOV=1 + > > define filechk_config_mk > > ( > > \ echo "\$$(foreach config,\$$(filter CONFIG_%, \ > > diff --git a/hypervisor/arch/arm-common/Kbuild > > b/hypervisor/arch/arm-common/Kbuild --- > > a/hypervisor/arch/arm-common/Kbuild +++ > > b/hypervisor/arch/arm-common/Kbuild @@ -13,6 +13,7 @@ > > include $(CONFIG_MK) > > > > GCOV_PROFILE := n > > +ccflags-$(CONFIG_JAILHOUSE_GCOV) += --coverage > > -DCONFIG_JAILHOUSE_GCOV=1 > > OBJS-y += dbg-write.o lib.o psci.o control.o paging.o mmu_cell.o > > OBJS-y += irqchip.o pci.o ivshmem.o uart-pl011.o uart-8250.o > > uart-xuartps.o diff --git a/hypervisor/arch/x86/Kbuild > > b/hypervisor/arch/x86/Kbuild --- a/hypervisor/arch/x86/Kbuild > > +++ b/hypervisor/arch/x86/Kbuild > > @@ -13,6 +13,7 @@ > > # > > > > GCOV_PROFILE := n > > +ccflags-$(CONFIG_JAILHOUSE_GCOV) += --coverage > > -DCONFIG_JAILHOUSE_GCOV=1 > > BUILT_IN_OBJECTS := built-in-amd.o built-in-intel.o > > COMMON_OBJECTS := apic.o dbg-write.o entry.o setup.o control.o > > mmio.o iommu.o \ diff --git a/hypervisor/gcov.c b/hypervisor/gcov.c > > new file mode 100644 > > --- /dev/null > > +++ b/hypervisor/gcov.c > > @@ -0,0 +1,35 @@ > > +/* > > + * 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 <jailhouse/entry.h> > > + > > +/* the actual data structure is bigger but we just need to know > > the version > > + * independant beginning to link the element to a list */ > > independent > > "element" or "elements"? > > > +struct gcov_min_info { > > + unsigned int version; > > + struct gcov_min_info *next; > > +}; > > Blank line. > > > +void __gcov_init(struct gcov_min_info *info); > > +void __gcov_merge_add(void *counters, unsigned int n_counters); > > + > > + > > +/* just link them all together and leave the head in the header > > + * where a processing tool can find it */ > > +void __gcov_init(struct gcov_min_info *info) > > +{ > > + info->next = (struct > > gcov_min_info*)hypervisor_header.gcov_info_head; > > (struct gcov_min_info *) > > > + hypervisor_header.gcov_info_head = info; > > +} > > + > > +/* Satisfy the linker, never called */ > > +void __gcov_merge_add(void *counters, unsigned int n_counters) > > +{ > > +} > > diff --git a/hypervisor/hypervisor.lds.S > > b/hypervisor/hypervisor.lds.S --- a/hypervisor/hypervisor.lds.S > > +++ b/hypervisor/hypervisor.lds.S > > @@ -30,6 +30,11 @@ > > . = ALIGN(16); > > .data : { *(.data) } > > > > + . = ALIGN(8); > > + __init_array_start = .; > > + .init_array : { *(SORT(.init_array.*)) > > *(.init_array) } > > + __init_array_end = .; > > + > > ARCH_SECTIONS > > > > . = ALIGN(16); > > diff --git a/hypervisor/include/jailhouse/header.h > > b/hypervisor/include/jailhouse/header.h --- > > a/hypervisor/include/jailhouse/header.h +++ > > b/hypervisor/include/jailhouse/header.h @@ -56,7 +56,19 @@ > > /** Offset of the console page inside the hypervisor memory > > * @note Filled at build time. */ > > unsigned long console_page; > > + // TODO we could also printk those two values or put them > > in sysfs > > + // but putting them here the tools will find everything it > > needs > > + // in one place > > I don't get the comment: How can we put that in sysfs without telling > the driver first what the value is? Then we do need this header field, > no? Please clarify, then remove the comment. It just means i am not sure the use-case justifies modifying the header. I will take JAILHOUSE_BASE at compile time, which leaves us with the pointer to that first element. Sysfs really does not make any sense. But if we expected the user to find the pointer from the logs and provide it to the tool as parameter, the header could stay untouched. > > + /** Pointer to the first struct gcov_info > > + * @note Filled at build time */ > > + void *gcov_info_head; > > > > + // TODO would it be cleaner to do fill this in from the > > hypervisor > > + // would make arm64 export patch redundant > > Exactly. Please fix. > > > + /** JAILHOUSE_BASE, keep it here so debugging tools can > > know > > + * the firmware image was mapped > > + * @note Filled by Linux loader driver before entry */ > > + void *jailhouse_base; > > /** Configured maximum logical CPU ID + 1. > > * @note Filled by Linux loader driver before entry. */ > > unsigned int max_cpus; > > diff --git a/hypervisor/setup.c b/hypervisor/setup.c > > --- a/hypervisor/setup.c > > +++ b/hypervisor/setup.c > > @@ -23,7 +23,9 @@ > > extern u8 __text_start[], __page_pool[]; > > The __init_arrays should go here. > > > > > static const __attribute__((aligned(PAGE_SIZE))) u8 > > empty_page[PAGE_SIZE]; - > > +#ifdef CONFIG_JAILHOUSE_GCOV > > +extern unsigned long __init_array_start[], __init_array_end[]; > > +#endif > > Unneeded #ifdef. > > > static DEFINE_SPINLOCK(init_lock); > > static unsigned int master_cpu_id = -1; > > static volatile unsigned int initialized_cpus; > > @@ -216,7 +218,19 @@ > > arch_cpu_restore(cpu_data, error); > > return error; > > } > > - > > +// TODO is there a better time to do that > > Yes, early in init_early, maybe after the first printk but before the > first return on error. > > > +#ifdef CONFIG_JAILHOUSE_GCOV > > + if (master) { > > + unsigned long *iarray = __init_array_start; > > + unsigned long *iarray_end = __init_array_end; > > + void (*__init_func)(void); > > Blank line. > > > + while (iarray < iarray_end) { > > + __init_func = (void(*)(void))(iarray[0]); > > The braces around iarray[0] are unneeded. > > > + iarray++; > > Could be folded into the statement above, but I also don't mind > keeping it separate. > > > + (*__init_func)(); > > __init_func(); > > > + } > > + } > > +#endif > > if (master) > > printk("Activating hypervisor\n"); > > > > > > 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.
