[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
