| Message ID | 1472816829-15551-2-git-send-email-wei.liu2@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 02.09.16 at 13:47, <wei.liu2@citrix.com> 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 <public/xen.h> > #include <public/gcov.h> > > +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
On Fri, Sep 02, 2016 at 05:56:49AM -0600, Jan Beulich wrote: > >>> On 02.09.16 at 13:47, <wei.liu2@citrix.com> 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 <public/xen.h> > > #include <public/gcov.h> > > > > +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 >
>>> On 02.09.16 at 14:01, <wei.liu2@citrix.com> wrote: > On Fri, Sep 02, 2016 at 05:56:49AM -0600, Jan Beulich wrote: >> >>> On 02.09.16 at 13:47, <wei.liu2@citrix.com> 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
On 02/09/16 13:06, Jan Beulich wrote: >>>> On 02.09.16 at 14:01, <wei.liu2@citrix.com> wrote: >> On Fri, Sep 02, 2016 at 05:56:49AM -0600, Jan Beulich wrote: >>>>>> On 02.09.16 at 13:47, <wei.liu2@citrix.com> 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
>>> On 02.09.16 at 14:13, <andrew.cooper3@citrix.com> wrote: > On 02/09/16 13:06, Jan Beulich wrote: >>>>> On 02.09.16 at 14:01, <wei.liu2@citrix.com> wrote: >>> On Fri, Sep 02, 2016 at 05:56:49AM -0600, Jan Beulich wrote: >>>>>>> On 02.09.16 at 13:47, <wei.liu2@citrix.com> 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
On 02/09/16 13:26, Jan Beulich wrote: >>>> On 02.09.16 at 14:13, <andrew.cooper3@citrix.com> wrote: >> On 02/09/16 13:06, Jan Beulich wrote: >>>>>> On 02.09.16 at 14:01, <wei.liu2@citrix.com> wrote: >>>> On Fri, Sep 02, 2016 at 05:56:49AM -0600, Jan Beulich wrote: >>>>>>>> On 02.09.16 at 13:47, <wei.liu2@citrix.com> 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
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, <wei.liu2@citrix.com> 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,
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, <andrew.cooper3@citrix.com> wrote: > >> On 02/09/16 13:06, Jan Beulich wrote: > >>>>>> On 02.09.16 at 14:01, <wei.liu2@citrix.com> wrote: > >>>> On Fri, Sep 02, 2016 at 05:56:49AM -0600, Jan Beulich wrote: > >>>>>>>> On 02.09.16 at 13:47, <wei.liu2@citrix.com> 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
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, <wei.liu2@citrix.com> 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
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 <public/xen.h> #include <public/gcov.h> +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_LEN 20 extern char *print_tainted(char *str);
Signed-off-by: Wei Liu <wei.liu2@citrix.com> --- The location of warning_add() is a bit arbitrary because gcov doesn't have an initialisation routine. Cc: Andrew Cooper <andrew.cooper3@citrix.com> Cc: George Dunlap <George.Dunlap@eu.citrix.com> Cc: Ian Jackson <ian.jackson@eu.citrix.com> Cc: Jan Beulich <jbeulich@suse.com> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> Cc: Stefano Stabellini <sstabellini@kernel.org> Cc: Tim Deegan <tim@xen.org> Cc: Wei Liu <wei.liu2@citrix.com> --- 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(-)