[PATCH] xen: make tracebuffer configurable

[PATCH] xen: make tracebuffer configurable

[Baodong Chen]

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(-)

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 {                                        \
-- 
2.7.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH] xen: make tracebuffer configurable

[Baodong Chen]

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(-)

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 {                                        \
-- 
2.7.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] xen: make tracebuffer configurable

[Jan Beulich]

>>> 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



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] xen: make tracebuffer configurable

[Jan Beulich]

>>> 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



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] xen: make tracebuffer configurable

[George Dunlap]

> 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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] xen: make tracebuffer configurable

[George Dunlap]

> 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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] xen: make tracebuffer configurable

[Jan Beulich]

>>> 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


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] xen: make tracebuffer configurable

[Jan Beulich]

>>> 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


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] xen: make tracebuffer configurable

[chenbaodong]

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
>
>
> .
>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] xen: make tracebuffer configurable

[chenbaodong]

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
>
>
> .
>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] xen: make tracebuffer configurable

[chenbaodong]

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
>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] xen: make tracebuffer configurable

[chenbaodong]

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
>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] xen: make tracebuffer configurable

[Jan Beulich]

>>> 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



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] xen: make tracebuffer configurable

[Jan Beulich]

>>> 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



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] xen: make tracebuffer configurable

[chenbaodong]

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
>
>
> .
>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] xen: make tracebuffer configurable

[chenbaodong]

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
>
>
> .
>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] xen: make tracebuffer configurable

[Jan Beulich]

>>> 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



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] xen: make tracebuffer configurable

[Jan Beulich]

>>> 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



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] xen: make tracebuffer configurable

[George Dunlap]

> 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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] xen: make tracebuffer configurable

[George Dunlap]

> 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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] xen: make tracebuffer configurable

[Jan Beulich]

>>> 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


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] xen: make tracebuffer configurable

[Jan Beulich]

>>> 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


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] xen: make tracebuffer configurable

[chenbaodong]

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
>
> .
>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] xen: make tracebuffer configurable

[chenbaodong]

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
>
> .
>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel