[Xen-devel] [PATCH 1/5] xen: add tainted state and show warning is gcov is enabled
Signed-off-by: Wei Liu --- The location of warning_add() is a bit arbitrary because gcov doesn't have an initialisation routine. Cc: Andrew Cooper Cc: George Dunlap Cc: Ian Jackson Cc: Jan Beulich Cc: Konrad Rzeszutek Wilk Cc: Stefano Stabellini Cc: Tim Deegan Cc: Wei Liu --- xen/common/gcov/gcov.c | 5 + xen/common/kernel.c| 6 -- xen/drivers/char/console.c | 9 + xen/include/xen/lib.h | 1 + 4 files changed, 19 insertions(+), 2 deletions(-) diff --git a/xen/common/gcov/gcov.c b/xen/common/gcov/gcov.c index b5717b9..6d18729 100644 --- a/xen/common/gcov/gcov.c +++ b/xen/common/gcov/gcov.c @@ -23,6 +23,11 @@ #include #include +const char __initconst warning_gcov[] = +"WARNING: GCOV SUPPORT IS ENABLED\n" +"This option is *ONLY* intended to aid testing of Xen.\n" +"Please *DO NOT* use this in production.\n"; + static struct gcov_info *info_list; /* diff --git a/xen/common/kernel.c b/xen/common/kernel.c index 2d3db64..324cc24 100644 --- a/xen/common/kernel.c +++ b/xen/common/kernel.c @@ -173,6 +173,7 @@ unsigned int tainted; * * 'C' - Console output is synchronous. * 'E' - An error (e.g. a machine check exceptions) has been injected. + * 'G' - GCov support is enabled. * 'H' - HVM forced emulation prefix is permitted. * 'M' - Machine had a machine check experience. * @@ -182,11 +183,12 @@ char *print_tainted(char *str) { if ( tainted ) { -snprintf(str, TAINT_STRING_MAX_LEN, "Tainted: %c%c%c%c", +snprintf(str, TAINT_STRING_MAX_LEN, "Tainted: %c%c%c%c%c", tainted & TAINT_MACHINE_CHECK ? 'M' : ' ', tainted & TAINT_SYNC_CONSOLE ? 'C' : ' ', tainted & TAINT_ERROR_INJECT ? 'E' : ' ', - tainted & TAINT_HVM_FEP ? 'H' : ' '); + tainted & TAINT_HVM_FEP ? 'H' : ' ', + tainted & TAINT_GCOV ? 'G' : ' '); } else { diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c index 650035d..77604f9 100644 --- a/xen/drivers/char/console.c +++ b/xen/drivers/char/console.c @@ -792,6 +792,10 @@ void __init console_init_postirq(void) console_init_ring(); } +#ifdef CONFIG_GCOV +extern char warning_gcov[]; +#endif + void __init console_endboot(void) { printk("Std. Loglevel: %s", loglvl_str(xenlog_lower_thresh)); @@ -802,6 +806,11 @@ void __init console_endboot(void) printk(" (Rate-limited: %s)", loglvl_str(xenlog_guest_upper_thresh)); printk("\n"); +#ifdef CONFIG_GCOV +warning_add(warning_gcov); +add_taint(TAINT_GCOV); +#endif + warning_print(); video_endboot(); diff --git a/xen/include/xen/lib.h b/xen/include/xen/lib.h index e518adc..7bcc910 100644 --- a/xen/include/xen/lib.h +++ b/xen/include/xen/lib.h @@ -143,6 +143,7 @@ uint64_t muldiv64(uint64_t a, uint32_t b, uint32_t c); #define TAINT_MACHINE_CHECK (1u << 1) #define TAINT_ERROR_INJECT (1u << 2) #define TAINT_HVM_FEP (1u << 3) +#define TAINT_GCOV (1u << 4) extern unsigned int tainted; #define TAINT_STRING_MAX_LEN20 extern char *print_tainted(char *str); -- 2.1.4 ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 1/5] xen: add tainted state and show warning is gcov is enabled
>>> On 02.09.16 at 13:47, wrote: Since this is a config option - why bother issuing a warning and tainting the hypervisor? > --- a/xen/common/gcov/gcov.c > +++ b/xen/common/gcov/gcov.c > @@ -23,6 +23,11 @@ > #include > #include > > +const char __initconst warning_gcov[] = > +"WARNING: GCOV SUPPORT IS ENABLED\n" > +"This option is *ONLY* intended to aid testing of Xen.\n" > +"Please *DO NOT* use this in production.\n"; Note the type (const) difference between this and ... > --- a/xen/drivers/char/console.c > +++ b/xen/drivers/char/console.c > @@ -792,6 +792,10 @@ void __init console_init_postirq(void) > console_init_ring(); > } > > +#ifdef CONFIG_GCOV > +extern char warning_gcov[]; > +#endif ... this. That's one of the reasons declarations of stuff defined in C sources should be put in a header, which then gets included by both producer and consumer(s). But ... > @@ -802,6 +806,11 @@ void __init console_endboot(void) > printk(" (Rate-limited: %s)", loglvl_str(xenlog_guest_upper_thresh)); > printk("\n"); > > +#ifdef CONFIG_GCOV > +warning_add(warning_gcov); > +add_taint(TAINT_GCOV); > +#endif ... (if we want this in the first place) how about #ifdef CONFIG_GCOV { static const char __initconst warning_gcov[] = "..."; warning_add(warning_gcov); add_taint(TAINT_GCOV); } #endif Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 1/5] xen: add tainted state and show warning is gcov is enabled
On Fri, Sep 02, 2016 at 05:56:49AM -0600, Jan Beulich wrote: > >>> On 02.09.16 at 13:47, wrote: > > Since this is a config option - why bother issuing a warning and > tainting the hypervisor? > Because there isn't a clear indicator if gcov is enabled. I think it would be valuable to just tell from the backtrace or console log that gcov is enabled, then we can legitimately refuse to provide (security) support for such builds. > > --- a/xen/common/gcov/gcov.c > > +++ b/xen/common/gcov/gcov.c > > @@ -23,6 +23,11 @@ > > #include > > #include > > > > +const char __initconst warning_gcov[] = > > +"WARNING: GCOV SUPPORT IS ENABLED\n" > > +"This option is *ONLY* intended to aid testing of Xen.\n" > > +"Please *DO NOT* use this in production.\n"; > > Note the type (const) difference between this and ... > > > --- a/xen/drivers/char/console.c > > +++ b/xen/drivers/char/console.c > > @@ -792,6 +792,10 @@ void __init console_init_postirq(void) > > console_init_ring(); > > } > > > > +#ifdef CONFIG_GCOV > > +extern char warning_gcov[]; > > +#endif > > ... this. That's one of the reasons declarations of stuff defined in > C sources should be put in a header, which then gets included by > both producer and consumer(s). But ... > > > @@ -802,6 +806,11 @@ void __init console_endboot(void) > > printk(" (Rate-limited: %s)", > > loglvl_str(xenlog_guest_upper_thresh)); > > printk("\n"); > > > > +#ifdef CONFIG_GCOV > > +warning_add(warning_gcov); > > +add_taint(TAINT_GCOV); > > +#endif > > ... (if we want this in the first place) how about > > #ifdef CONFIG_GCOV > { > static const char __initconst warning_gcov[] = "..."; > > warning_add(warning_gcov); > add_taint(TAINT_GCOV); > } > #endif > Fine with me. Wei. > Jan > ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 1/5] xen: add tainted state and show warning is gcov is enabled
>>> On 02.09.16 at 14:01, wrote: > On Fri, Sep 02, 2016 at 05:56:49AM -0600, Jan Beulich wrote: >> >>> On 02.09.16 at 13:47, wrote: >> >> Since this is a config option - why bother issuing a warning and >> tainting the hypervisor? >> > > Because there isn't a clear indicator if gcov is enabled. > > I think it would be valuable to just tell from the backtrace or console > log that gcov is enabled, then we can legitimately refuse to provide > (security) support for such builds. Then perhaps making it match the "debug=" would be the better approach for a feature not controlled on the command line? Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 1/5] xen: add tainted state and show warning is gcov is enabled
On 02/09/16 13:06, Jan Beulich wrote: On 02.09.16 at 14:01, wrote: >> On Fri, Sep 02, 2016 at 05:56:49AM -0600, Jan Beulich wrote: >> On 02.09.16 at 13:47, wrote: >>> Since this is a config option - why bother issuing a warning and >>> tainting the hypervisor? >>> >> Because there isn't a clear indicator if gcov is enabled. >> >> I think it would be valuable to just tell from the backtrace or console >> log that gcov is enabled, then we can legitimately refuse to provide >> (security) support for such builds. > Then perhaps making it match the "debug=" would be the better > approach for a feature not controlled on the command line? I would prefer not to make it depend on debug= Coverage on a release hypervisor is equally important, and will be different from a debug hypervisor. I am on the fence as to whether a taint is right to use, but I do think that a "with GCOV" is needed somewhere obvious on the banner line. ~Andrew ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 1/5] xen: add tainted state and show warning is gcov is enabled
>>> On 02.09.16 at 14:13, wrote: > On 02/09/16 13:06, Jan Beulich wrote: > On 02.09.16 at 14:01, wrote: >>> On Fri, Sep 02, 2016 at 05:56:49AM -0600, Jan Beulich wrote: >>> On 02.09.16 at 13:47, wrote: Since this is a config option - why bother issuing a warning and tainting the hypervisor? >>> Because there isn't a clear indicator if gcov is enabled. >>> >>> I think it would be valuable to just tell from the backtrace or console >>> log that gcov is enabled, then we can legitimately refuse to provide >>> (security) support for such builds. >> Then perhaps making it match the "debug=" would be the better >> approach for a feature not controlled on the command line? > > I would prefer not to make it depend on debug= > > Coverage on a release hypervisor is equally important, and will be > different from a debug hypervisor. I didn't say "depend on", but "match" (which I mean just logging wise). > I am on the fence as to whether a taint is right to use, but I do think > that a "with GCOV" is needed somewhere obvious on the banner line. Right, hence the matching goal with "debug=". Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 1/5] xen: add tainted state and show warning is gcov is enabled
On 02/09/16 13:26, Jan Beulich wrote: On 02.09.16 at 14:13, wrote: >> On 02/09/16 13:06, Jan Beulich wrote: >> On 02.09.16 at 14:01, wrote: On Fri, Sep 02, 2016 at 05:56:49AM -0600, Jan Beulich wrote: On 02.09.16 at 13:47, wrote: > Since this is a config option - why bother issuing a warning and > tainting the hypervisor? > Because there isn't a clear indicator if gcov is enabled. I think it would be valuable to just tell from the backtrace or console log that gcov is enabled, then we can legitimately refuse to provide (security) support for such builds. >>> Then perhaps making it match the "debug=" would be the better >>> approach for a feature not controlled on the command line? >> I would prefer not to make it depend on debug= >> >> Coverage on a release hypervisor is equally important, and will be >> different from a debug hypervisor. > I didn't say "depend on", but "match" (which I mean just logging wise). > >> I am on the fence as to whether a taint is right to use, but I do think >> that a "with GCOV" is needed somewhere obvious on the banner line. > Right, hence the matching goal with "debug=". Ah - I see what you mean. Yes - that would be fine by me. ~Andrew ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 1/5] xen: add tainted state and show warning is gcov is enabled
Hi Wei, On 02/09/16 13:01, Wei Liu wrote: On Fri, Sep 02, 2016 at 05:56:49AM -0600, Jan Beulich wrote: On 02.09.16 at 13:47, wrote: Since this is a config option - why bother issuing a warning and tainting the hypervisor? Because there isn't a clear indicator if gcov is enabled. I think this is an issue to all .config option. If I am not mistaken, currently you are not able to know whether option FOO has been enabled for your kernel. Maybe we should integrate the .config in the binary? Regards, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 1/5] xen: add tainted state and show warning is gcov is enabled
On Fri, Sep 02, 2016 at 01:30:40PM +0100, Andrew Cooper wrote: > On 02/09/16 13:26, Jan Beulich wrote: > On 02.09.16 at 14:13, wrote: > >> On 02/09/16 13:06, Jan Beulich wrote: > >> On 02.09.16 at 14:01, wrote: > On Fri, Sep 02, 2016 at 05:56:49AM -0600, Jan Beulich wrote: > On 02.09.16 at 13:47, wrote: > > Since this is a config option - why bother issuing a warning and > > tainting the hypervisor? > > > Because there isn't a clear indicator if gcov is enabled. > > I think it would be valuable to just tell from the backtrace or console > log that gcov is enabled, then we can legitimately refuse to provide > (security) support for such builds. > >>> Then perhaps making it match the "debug=" would be the better > >>> approach for a feature not controlled on the command line? > >> I would prefer not to make it depend on debug= > >> > >> Coverage on a release hypervisor is equally important, and will be > >> different from a debug hypervisor. > > I didn't say "depend on", but "match" (which I mean just logging wise). > > > >> I am on the fence as to whether a taint is right to use, but I do think > >> that a "with GCOV" is needed somewhere obvious on the banner line. > > Right, hence the matching goal with "debug=". > > Ah - I see what you mean. Yes - that would be fine by me. > Fine by me, too. I will replace this patch with a new one. Wei. > ~Andrew ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 1/5] xen: add tainted state and show warning is gcov is enabled
On Fri, Sep 02, 2016 at 02:21:28PM +0100, Julien Grall wrote: > Hi Wei, > > On 02/09/16 13:01, Wei Liu wrote: > >On Fri, Sep 02, 2016 at 05:56:49AM -0600, Jan Beulich wrote: > >On 02.09.16 at 13:47, wrote: > >> > >>Since this is a config option - why bother issuing a warning and > >>tainting the hypervisor? > >> > > > >Because there isn't a clear indicator if gcov is enabled. > > I think this is an issue to all .config option. If I am not mistaken, > currently you are not able to know whether option FOO has been enabled for > your kernel. > > Maybe we should integrate the .config in the binary? > I think that would be a good idea. It is orthogonal to what I'm trying to do here though. Wei. > Regards, > > -- > Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel