| Message ID | 1559211432-30973-1-git-send-email-chenbaodong@mxnavi.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen: make tracebuffer configurable | expand |
>>> On 30.05.19 at 12:17, <chenbaodong@mxnavi.com> wrote: > Default: enabled. > Can be disabled for smaller code footprint. But you're aware that we're, for now at least, trying to limit the number of independently selectable config options? Ones depending on EXPERT are sort of an exception in certain cases. > --- a/xen/common/Kconfig > +++ b/xen/common/Kconfig > @@ -368,4 +368,10 @@ config DOM0_MEM > > Leave empty if you are not sure what to specify. > > +config HAS_TRACEBUFFER > + bool "Enable/Disable tracebuffer" > + default y > + ---help--- > + Enable or disable tracebuffer function. HAS_* options should not come with a prompt, and should only be "select"-ed by (normally) per-architecture Kconfig files. If we are to permit having this option, then I also think the help text needs expanding. > --- a/xen/include/xen/trace.h > +++ b/xen/include/xen/trace.h > @@ -21,12 +21,14 @@ > #ifndef __XEN_TRACE_H__ > #define __XEN_TRACE_H__ > > -extern int tb_init_done; > > #include <public/sysctl.h> > #include <public/trace.h> > #include <asm/trace.h> > > +#ifdef CONFIG_HAS_TRACEBUFFER > + > +extern int tb_init_done; > /* Used to initialise trace buffer functionality */ > void init_trace_bufs(void); I wonder if there hadn't been a reason for the declaration to live up where it was. Also please separate independent blocks of code by a blank line. > @@ -47,6 +49,20 @@ static inline void trace_var(u32 event, int cycles, int > extra, > void __trace_hypercall(uint32_t event, unsigned long op, > const xen_ulong_t *args); > > +#else > +#define tb_init_done (0) Perhaps better "false", and no real need for the parentheses afaict. > +static inline void init_trace_bufs(void) {} > +static inline int tb_control(struct xen_sysctl_tbuf_op *tbc) { return -ENOSYS; } > + > +static inline int trace_will_trace_event(u32 event) { return 0; } > +static inline void trace_var(u32 event, int cycles, int extra, > + const void *extra_data) {} > +static inline void __trace_var(u32 event, bool_t cycles, unsigned int extra, > + const void *extra_data) {} > +static inline void __trace_hypercall(uint32_t event, unsigned long op, > + const xen_ulong_t *args) {} > +#endif We try to do away with u32 and friends, so please don't introduce new uses - even less so when in one case here you already use uint32_t. Similarly please use "bool" in favor of "bool_t". Jan
> On May 31, 2019, at 12:10 PM, Jan Beulich <JBeulich@suse.com> wrote: > >>>> On 30.05.19 at 12:17, <chenbaodong@mxnavi.com> wrote: >> Default: enabled. >> Can be disabled for smaller code footprint. > > But you're aware that we're, for now at least, trying to limit the > number of independently selectable config options? Ones depending > on EXPERT are sort of an exception in certain cases. I’m trying to remember exactly what we have or haven’t decided. I take it you think we should avoid having a load of independently-selectable configurations to support? Baodong, what was your main purpose in adding a patch like this? Just to make things a bit tidier, or was it to try to go through and generate a far smaller hypervisor codebase (for instance, perhaps to make safety certification more tractable)? I think we’ve talked about this before, but our basic options, as far as support, would be: 1. Have a single large config option which disabled large swathes of unused functionality 2. Have individual bits configurable, but have only a handful of “security supported” configurations. The idea with #2 is that we’d have a “certification” config that we tested and security supported, with all of these individual bits off, as well as “cloud” and “client” configs with all of these “optional” bits on (or some subset on, depending on what each community thought made the most sense for their use cafe). If people wanted to enable or disable individual config options outside fo those, they’d be taking a risk wrt breakage (not tested) or security issues (no XSA issued unless it affected one of the supported configs). Rich / Daniel, am I on the right track here? Any thoughts? -George
>>> On 31.05.19 at 14:58, <George.Dunlap@citrix.com> wrote: >> On May 31, 2019, at 12:10 PM, Jan Beulich <JBeulich@suse.com> wrote: >>>>> On 30.05.19 at 12:17, <chenbaodong@mxnavi.com> wrote: >>> Default: enabled. >>> Can be disabled for smaller code footprint. >> >> But you're aware that we're, for now at least, trying to limit the >> number of independently selectable config options? Ones depending >> on EXPERT are sort of an exception in certain cases. > > I’m trying to remember exactly what we have or haven’t decided. I take it > you think we should avoid having a load of independently-selectable > configurations to support? Yes, that's the main (and perhaps only) reason to limit options with user visible prompts. > Baodong, what was your main purpose in adding a patch like this? Just to > make things a bit tidier, or was it to try to go through and generate a far > smaller hypervisor codebase (for instance, perhaps to make safety > certification more tractable)? > > I think we’ve talked about this before, but our basic options, as far as > support, would be: > > 1. Have a single large config option which disabled large swathes of unused > functionality Perhaps this is a worthwhile thing to have anyway. > 2. Have individual bits configurable, but have only a handful of “security > supported” configurations. > > The idea with #2 is that we’d have a “certification” config that we tested > and security supported, with all of these individual bits off, as well as > “cloud” and “client” configs with all of these “optional” bits on (or some > subset on, depending on what each community thought made the most sense for > their use cafe). If people wanted to enable or disable individual config > options outside fo those, they’d be taking a risk wrt breakage (not tested) > or security issues (no XSA issued unless it affected one of the supported > configs). The one issue with this that I see (besides the implied testing needs, as generally all or at least most of the possible combinations will need testing) is that once we have chosen such a "handful", their volume will likely grow. Plus it may be difficult to come to an agreement what should or should not be part of this "handful". But sure, we can give it a try ... Jan
On 5/31/19 19:10, Jan Beulich wrote: >>>> On 30.05.19 at 12:17, <chenbaodong@mxnavi.com> wrote: >> Default: enabled. >> Can be disabled for smaller code footprint. > But you're aware that we're, for now at least, trying to limit the > number of independently selectable config options? Ones depending > on EXPERT are sort of an exception in certain cases. Limit the number of independently selectable config sounds good to me. Does the following looks good? +config HAS_TRACEBUFFER + bool "Enable/Disable tracebuffer" if EXPERT = "y" + ---help--- + Enable or disable tracebuffer function. + Xen internal running status(trace event) will be saved to trace memory + when enabled. + > >> --- a/xen/common/Kconfig >> +++ b/xen/common/Kconfig >> @@ -368,4 +368,10 @@ config DOM0_MEM >> >> Leave empty if you are not sure what to specify. >> >> +config HAS_TRACEBUFFER >> + bool "Enable/Disable tracebuffer" >> + default y >> + ---help--- >> + Enable or disable tracebuffer function. > HAS_* options should not come with a prompt, and should > only be "select"-ed by (normally) per-architecture Kconfig > files. If we are to permit having this option, then I also think > the help text needs expanding. Tanks for your clarification. >> --- a/xen/include/xen/trace.h >> +++ b/xen/include/xen/trace.h >> @@ -21,12 +21,14 @@ >> #ifndef __XEN_TRACE_H__ >> #define __XEN_TRACE_H__ >> >> -extern int tb_init_done; >> >> #include <public/sysctl.h> >> #include <public/trace.h> >> #include <asm/trace.h> >> >> +#ifdef CONFIG_HAS_TRACEBUFFER >> + >> +extern int tb_init_done; >> /* Used to initialise trace buffer functionality */ >> void init_trace_bufs(void); > I wonder if there hadn't been a reason for the declaration to live > up where it was. Also please separate independent blocks of code > by a blank line. Roger that. > >> @@ -47,6 +49,20 @@ static inline void trace_var(u32 event, int cycles, int >> extra, >> void __trace_hypercall(uint32_t event, unsigned long op, >> const xen_ulong_t *args); >> >> +#else >> +#define tb_init_done (0) > Perhaps better "false", and no real need for the parentheses afaict. > >> +static inline void init_trace_bufs(void) {} >> +static inline int tb_control(struct xen_sysctl_tbuf_op *tbc) { return -ENOSYS; } >> + >> +static inline int trace_will_trace_event(u32 event) { return 0; } >> +static inline void trace_var(u32 event, int cycles, int extra, >> + const void *extra_data) {} >> +static inline void __trace_var(u32 event, bool_t cycles, unsigned int extra, >> + const void *extra_data) {} >> +static inline void __trace_hypercall(uint32_t event, unsigned long op, >> + const xen_ulong_t *args) {} >> +#endif > We try to do away with u32 and friends, so please don't introduce > new uses - even less so when in one case here you already use > uint32_t. Similarly please use "bool" in favor of "bool_t". Roger that. > Jan > > > . >
On 5/31/19 20:58, George Dunlap wrote: > >> On May 31, 2019, at 12:10 PM, Jan Beulich <JBeulich@suse.com> wrote: >> >>>>> On 30.05.19 at 12:17, <chenbaodong@mxnavi.com> wrote: >>> Default: enabled. >>> Can be disabled for smaller code footprint. >> But you're aware that we're, for now at least, trying to limit the >> number of independently selectable config options? Ones depending >> on EXPERT are sort of an exception in certain cases. > I’m trying to remember exactly what we have or haven’t decided. I take it you think we should avoid having a load of independently-selectable configurations to support? > > Baodong, what was your main purpose in adding a patch like this? Just to make things a bit tidier, or was it to try to go through and generate a far smaller hypervisor codebase (for instance, perhaps to make safety certification more tractable)? Hello George, yes yes, smaller code base for safety certification is under my thought. plan to run xen for automotive use case on arm (perhaps i.MX8 ) devices. > > I think we’ve talked about this before, but our basic options, as far as support, would be: > > 1. Have a single large config option which disabled large swathes of unused functionality > 2. Have individual bits configurable, but have only a handful of “security supported” configurations. > > The idea with #2 is that we’d have a “certification” config that we tested and security supported, with all of these individual bits off, as well as “cloud” and “client” configs with all of these “optional” bits on (or some subset on, depending on what each community thought made the most sense for their use cafe). If people wanted to enable or disable individual config options outside fo those, they’d be taking a risk wrt breakage (not tested) or security issues (no XSA issued unless it affected one of the supported configs). I like the idea about 'certification' config. > Rich / Daniel, am I on the right track here? Any thoughts? > > -George >
>>> On 03.06.19 at 05:07, <chenbaodong@mxnavi.com> wrote: > On 5/31/19 19:10, Jan Beulich wrote: >>>>> On 30.05.19 at 12:17, <chenbaodong@mxnavi.com> wrote: >>> Default: enabled. >>> Can be disabled for smaller code footprint. >> But you're aware that we're, for now at least, trying to limit the >> number of independently selectable config options? Ones depending >> on EXPERT are sort of an exception in certain cases. > > Limit the number of independently selectable config sounds good to me. > > Does the following looks good? > > +config HAS_TRACEBUFFER > + bool "Enable/Disable tracebuffer" if EXPERT = "y" > + ---help--- > + Enable or disable tracebuffer function. > + Xen internal running status(trace event) will be saved to > trace memory > + when enabled. > + The EXPERT addition make introducing this fine by me. But its name is still wrong, and the help text also needs further improvement imo. Jan
On 6/3/19 16:31, Jan Beulich wrote: >>>> On 03.06.19 at 05:07, <chenbaodong@mxnavi.com> wrote: >> On 5/31/19 19:10, Jan Beulich wrote: >>>>>> On 30.05.19 at 12:17, <chenbaodong@mxnavi.com> wrote: >>>> Default: enabled. >>>> Can be disabled for smaller code footprint. >>> But you're aware that we're, for now at least, trying to limit the >>> number of independently selectable config options? Ones depending >>> on EXPERT are sort of an exception in certain cases. >> Limit the number of independently selectable config sounds good to me. >> >> Does the following looks good? >> >> +config HAS_TRACEBUFFER >> + bool "Enable/Disable tracebuffer" if EXPERT = "y" >> + ---help--- >> + Enable or disable tracebuffer function. >> + Xen internal running status(trace event) will be saved to >> trace memory >> + when enabled. >> + > The EXPERT addition make introducing this fine by me. But its name > is still wrong, and the help text also needs further improvement imo. Hi Jan, thanks for your kindly review and feedback. For this, would you please give your suggestions about the name and help text? > > Jan > > > . >
>>> On 03.06.19 at 12:41, <chenbaodong@mxnavi.com> wrote: > On 6/3/19 16:31, Jan Beulich wrote: >>>>> On 03.06.19 at 05:07, <chenbaodong@mxnavi.com> wrote: >>> On 5/31/19 19:10, Jan Beulich wrote: >>>>>>> On 30.05.19 at 12:17, <chenbaodong@mxnavi.com> wrote: >>>>> Default: enabled. >>>>> Can be disabled for smaller code footprint. >>>> But you're aware that we're, for now at least, trying to limit the >>>> number of independently selectable config options? Ones depending >>>> on EXPERT are sort of an exception in certain cases. >>> Limit the number of independently selectable config sounds good to me. >>> >>> Does the following looks good? >>> >>> +config HAS_TRACEBUFFER >>> + bool "Enable/Disable tracebuffer" if EXPERT = "y" >>> + ---help--- >>> + Enable or disable tracebuffer function. >>> + Xen internal running status(trace event) will be saved to >>> trace memory >>> + when enabled. >>> + >> The EXPERT addition make introducing this fine by me. But its name >> is still wrong, and the help text also needs further improvement imo. > > Hi Jan, thanks for your kindly review and feedback. > > For this, would you please give your suggestions about the name and help > text? As far as the name is concerned, the HAS_ should be dropped. I'm afraid I don't have a particular suggestion for the help text. Jan
> On Jun 3, 2019, at 11:54 AM, Jan Beulich <JBeulich@suse.com> wrote: > >>>> On 03.06.19 at 12:41, <chenbaodong@mxnavi.com> wrote: > >> On 6/3/19 16:31, Jan Beulich wrote: >>>>>> On 03.06.19 at 05:07, <chenbaodong@mxnavi.com> wrote: >>>> On 5/31/19 19:10, Jan Beulich wrote: >>>>>>>> On 30.05.19 at 12:17, <chenbaodong@mxnavi.com> wrote: >>>>>> Default: enabled. >>>>>> Can be disabled for smaller code footprint. >>>>> But you're aware that we're, for now at least, trying to limit the >>>>> number of independently selectable config options? Ones depending >>>>> on EXPERT are sort of an exception in certain cases. >>>> Limit the number of independently selectable config sounds good to me. >>>> >>>> Does the following looks good? >>>> >>>> +config HAS_TRACEBUFFER >>>> + bool "Enable/Disable tracebuffer" if EXPERT = "y" >>>> + ---help--- >>>> + Enable or disable tracebuffer function. >>>> + Xen internal running status(trace event) will be saved to >>>> trace memory >>>> + when enabled. >>>> + >>> The EXPERT addition make introducing this fine by me. But its name >>> is still wrong, and the help text also needs further improvement imo. >> >> Hi Jan, thanks for your kindly review and feedback. >> >> For this, would you please give your suggestions about the name and help >> text? > > As far as the name is concerned, the HAS_ should be dropped. > I'm afraid I don't have a particular suggestion for the help text. You could at least give an idea what you think the help text should include, or some kind of guidance as to what would satisfy you. Obviously you shouldn’t be required to write everybody’s help text for them; but by the same token, everybody shouldn’t be required to read your mind. Is, “A description of the feature, along with the costs of enabling it” the sort of thing you had in mind? -George
>>> On 03.06.19 at 16:08, <George.Dunlap@citrix.com> wrote: > >> On Jun 3, 2019, at 11:54 AM, Jan Beulich <JBeulich@suse.com> wrote: >> >>>>> On 03.06.19 at 12:41, <chenbaodong@mxnavi.com> wrote: >> >>> On 6/3/19 16:31, Jan Beulich wrote: >>>>>>> On 03.06.19 at 05:07, <chenbaodong@mxnavi.com> wrote: >>>>> On 5/31/19 19:10, Jan Beulich wrote: >>>>>>>>> On 30.05.19 at 12:17, <chenbaodong@mxnavi.com> wrote: >>>>>>> Default: enabled. >>>>>>> Can be disabled for smaller code footprint. >>>>>> But you're aware that we're, for now at least, trying to limit the >>>>>> number of independently selectable config options? Ones depending >>>>>> on EXPERT are sort of an exception in certain cases. >>>>> Limit the number of independently selectable config sounds good to me. >>>>> >>>>> Does the following looks good? >>>>> >>>>> +config HAS_TRACEBUFFER >>>>> + bool "Enable/Disable tracebuffer" if EXPERT = "y" >>>>> + ---help--- >>>>> + Enable or disable tracebuffer function. >>>>> + Xen internal running status(trace event) will be saved to >>>>> trace memory >>>>> + when enabled. >>>>> + >>>> The EXPERT addition make introducing this fine by me. But its name >>>> is still wrong, and the help text also needs further improvement imo. >>> >>> Hi Jan, thanks for your kindly review and feedback. >>> >>> For this, would you please give your suggestions about the name and help >>> text? >> >> As far as the name is concerned, the HAS_ should be dropped. >> I'm afraid I don't have a particular suggestion for the help text. > > You could at least give an idea what you think the help text should include, > or some kind of guidance as to what would satisfy you. Obviously you > shouldn’t be required to write everybody’s help text for them; but by the > same token, everybody shouldn’t be required to read your mind. > > Is, “A description of the feature, along with the costs of enabling it” the > sort of thing you had in mind? I had nothing in particular in mind. There ought to be other Kconfig options with at least half way reasonable help text, which I think could be used as guidance. Beyond that I think help text largely only re-stating what the prompt already says isn't helpful, and hence could as well be omitted. Jan
On 6/3/19 22:31, Jan Beulich wrote: >>>> On 03.06.19 at 16:08, <George.Dunlap@citrix.com> wrote: >>> On Jun 3, 2019, at 11:54 AM, Jan Beulich <JBeulich@suse.com> wrote: >>> >>>>>> On 03.06.19 at 12:41, <chenbaodong@mxnavi.com> wrote: >>>> On 6/3/19 16:31, Jan Beulich wrote: >>>>>>>> On 03.06.19 at 05:07, <chenbaodong@mxnavi.com> wrote: >>>>>> On 5/31/19 19:10, Jan Beulich wrote: >>>>>>>>>> On 30.05.19 at 12:17, <chenbaodong@mxnavi.com> wrote: >>>>>>>> Default: enabled. >>>>>>>> Can be disabled for smaller code footprint. >>>>>>> But you're aware that we're, for now at least, trying to limit the >>>>>>> number of independently selectable config options? Ones depending >>>>>>> on EXPERT are sort of an exception in certain cases. >>>>>> Limit the number of independently selectable config sounds good to me. >>>>>> >>>>>> Does the following looks good? >>>>>> >>>>>> +config HAS_TRACEBUFFER >>>>>> + bool "Enable/Disable tracebuffer" if EXPERT = "y" >>>>>> + ---help--- >>>>>> + Enable or disable tracebuffer function. >>>>>> + Xen internal running status(trace event) will be saved to >>>>>> trace memory >>>>>> + when enabled. >>>>>> + >>>>> The EXPERT addition make introducing this fine by me. But its name >>>>> is still wrong, and the help text also needs further improvement imo. >>>> Hi Jan, thanks for your kindly review and feedback. >>>> >>>> For this, would you please give your suggestions about the name and help >>>> text? >>> As far as the name is concerned, the HAS_ should be dropped. >>> I'm afraid I don't have a particular suggestion for the help text. >> You could at least give an idea what you think the help text should include, >> or some kind of guidance as to what would satisfy you. Obviously you >> shouldn’t be required to write everybody’s help text for them; but by the >> same token, everybody shouldn’t be required to read your mind. >> >> Is, “A description of the feature, along with the costs of enabling it” the >> sort of thing you had in mind? > I had nothing in particular in mind. There ought to be other Kconfig > options with at least half way reasonable help text, which I think > could be used as guidance. Beyond that I think help text largely only > re-stating what the prompt already says isn't helpful, and hence > could as well be omitted. Update the help text and the HAS_ has been dropped in v1. > > Jan > > . >
diff --git a/xen/common/Kconfig b/xen/common/Kconfig index c838506..3a6eec8 100644 --- a/xen/common/Kconfig +++ b/xen/common/Kconfig @@ -368,4 +368,10 @@ config DOM0_MEM Leave empty if you are not sure what to specify. +config HAS_TRACEBUFFER + bool "Enable/Disable tracebuffer" + default y + ---help--- + Enable or disable tracebuffer function. + endmenu diff --git a/xen/common/Makefile b/xen/common/Makefile index bca48e6..86c5bf9 100644 --- a/xen/common/Makefile +++ b/xen/common/Makefile @@ -56,7 +56,7 @@ obj-y += sysctl.o obj-y += tasklet.o obj-y += time.o obj-y += timer.o -obj-y += trace.o +obj-$(CONFIG_HAS_TRACEBUFFER) += trace.o obj-y += version.o obj-y += virtual_region.o obj-y += vm_event.o diff --git a/xen/include/xen/trace.h b/xen/include/xen/trace.h index 12966ea..fb1a2bc 100644 --- a/xen/include/xen/trace.h +++ b/xen/include/xen/trace.h @@ -21,12 +21,14 @@ #ifndef __XEN_TRACE_H__ #define __XEN_TRACE_H__ -extern int tb_init_done; #include <public/sysctl.h> #include <public/trace.h> #include <asm/trace.h> +#ifdef CONFIG_HAS_TRACEBUFFER + +extern int tb_init_done; /* Used to initialise trace buffer functionality */ void init_trace_bufs(void); @@ -47,6 +49,20 @@ static inline void trace_var(u32 event, int cycles, int extra, void __trace_hypercall(uint32_t event, unsigned long op, const xen_ulong_t *args); +#else +#define tb_init_done (0) +static inline void init_trace_bufs(void) {} +static inline int tb_control(struct xen_sysctl_tbuf_op *tbc) { return -ENOSYS; } + +static inline int trace_will_trace_event(u32 event) { return 0; } +static inline void trace_var(u32 event, int cycles, int extra, + const void *extra_data) {} +static inline void __trace_var(u32 event, bool_t cycles, unsigned int extra, + const void *extra_data) {} +static inline void __trace_hypercall(uint32_t event, unsigned long op, + const xen_ulong_t *args) {} +#endif + /* Convenience macros for calling the trace function. */ #define TRACE_0D(_e) \ do { \
Default: enabled. Can be disabled for smaller code footprint. Signed-off-by: Baodong Chen <chenbaodong@mxnavi.com> --- xen/common/Kconfig | 6 ++++++ xen/common/Makefile | 2 +- xen/include/xen/trace.h | 18 +++++++++++++++++- 3 files changed, 24 insertions(+), 2 deletions(-)