* [PATCH] xen: make keyhanler configurable
@ 2019-05-31 1:58 Baodong Chen
2019-05-31 1:58 ` [Xen-devel] " Baodong Chen
` (3 more replies)
0 siblings, 4 replies; 14+ messages in thread
From: Baodong Chen @ 2019-05-31 1:58 UTC (permalink / raw)
To: xen-devel
Cc: Juergen Gross, Stefano Stabellini, Wei Liu,
Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson,
Tim Deegan, Julien Grall, Jan Beulich, Baodong Chen,
Dario Faggioli, Roger Pau Monné
keyhandler mainly used for debug usage by developers which can dump
xen module(eg. timer, scheduler) status at runtime by input
character from console.
Signed-off-by: Baodong Chen <chenbaodong@mxnavi.com>
---
xen/arch/arm/gic.c | 2 ++
xen/arch/x86/apic.c | 2 ++
xen/common/Kconfig | 9 +++++++++
xen/common/Makefile | 2 +-
xen/common/cpupool.c | 2 ++
xen/common/schedule.c | 2 ++
xen/include/xen/keyhandler.h | 14 ++++++++++++++
xen/include/xen/lib.h | 2 ++
xen/include/xen/sched.h | 2 ++
9 files changed, 36 insertions(+), 1 deletion(-)
diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 6cc7dec..fff88c5 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -361,7 +361,9 @@ static void do_sgi(struct cpu_user_regs *regs, enum gic_sgi sgi)
/* Nothing to do, will check for events on return path */
break;
case GIC_SGI_DUMP_STATE:
+#ifdef CONFIG_HAS_KEYHANDLER
dump_execstate(regs);
+#endif
break;
case GIC_SGI_CALL_FUNCTION:
smp_call_function_interrupt();
diff --git a/xen/arch/x86/apic.c b/xen/arch/x86/apic.c
index fafc0bd..e5f004a 100644
--- a/xen/arch/x86/apic.c
+++ b/xen/arch/x86/apic.c
@@ -1410,7 +1410,9 @@ void spurious_interrupt(struct cpu_user_regs *regs)
ack_APIC_irq();
if (this_cpu(state_dump_pending)) {
this_cpu(state_dump_pending) = false;
+#ifdef CONFIG_HAS_KEYHANDLER
dump_execstate(regs);
+#endif
return;
}
}
diff --git a/xen/common/Kconfig b/xen/common/Kconfig
index c838506..450541c 100644
--- a/xen/common/Kconfig
+++ b/xen/common/Kconfig
@@ -368,4 +368,13 @@ config DOM0_MEM
Leave empty if you are not sure what to specify.
+config HAS_KEYHANDLER
+ bool "Enable/Disable keyhandler"
+ default y
+ ---help---
+ Enable or disable keyhandler function.
+ keyhandler mainly used for debug usage by developers which can dump
+ xen module(eg. timer, scheduler) status at runtime by input character
+ from console.
+
endmenu
diff --git a/xen/common/Makefile b/xen/common/Makefile
index bca48e6..c7bcd26 100644
--- a/xen/common/Makefile
+++ b/xen/common/Makefile
@@ -16,7 +16,7 @@ obj-y += guestcopy.o
obj-bin-y += gunzip.init.o
obj-y += irq.o
obj-y += kernel.o
-obj-y += keyhandler.o
+obj-$(CONFIG_HAS_KEYHANDLER) += keyhandler.o
obj-$(CONFIG_KEXEC) += kexec.o
obj-$(CONFIG_KEXEC) += kimage.o
obj-y += lib.o
diff --git a/xen/common/cpupool.c b/xen/common/cpupool.c
index 31ac323..721a729 100644
--- a/xen/common/cpupool.c
+++ b/xen/common/cpupool.c
@@ -699,6 +699,7 @@ int cpupool_do_sysctl(struct xen_sysctl_cpupool_op *op)
return ret;
}
+#ifdef CONFIG_HAS_KEYHANDLER
void dump_runq(unsigned char key)
{
unsigned long flags;
@@ -730,6 +731,7 @@ void dump_runq(unsigned char key)
local_irq_restore(flags);
spin_unlock(&cpupool_lock);
}
+#endif
static int cpu_callback(
struct notifier_block *nfb, unsigned long action, void *hcpu)
diff --git a/xen/common/schedule.c b/xen/common/schedule.c
index 66f1e26..617c444 100644
--- a/xen/common/schedule.c
+++ b/xen/common/schedule.c
@@ -1913,6 +1913,7 @@ void scheduler_free(struct scheduler *sched)
xfree(sched);
}
+#ifdef CONFIG_HAS_KEYHANDLER
void schedule_dump(struct cpupool *c)
{
unsigned int i;
@@ -1941,6 +1942,7 @@ void schedule_dump(struct cpupool *c)
SCHED_OP(sched, dump_cpu_state, i);
}
}
+#endif
void sched_tick_suspend(void)
{
diff --git a/xen/include/xen/keyhandler.h b/xen/include/xen/keyhandler.h
index 5131e86..1050b80 100644
--- a/xen/include/xen/keyhandler.h
+++ b/xen/include/xen/keyhandler.h
@@ -28,6 +28,7 @@ struct cpu_user_regs;
typedef void (irq_keyhandler_fn_t)(unsigned char key,
struct cpu_user_regs *regs);
+#ifdef CONFIG_HAS_KEYHANDLER
/* Initialize keytable with default handlers. */
void initialize_keytable(void);
@@ -48,4 +49,17 @@ void register_irq_keyhandler(unsigned char key,
/* Inject a keypress into the key-handling subsystem. */
extern void handle_keypress(unsigned char key, struct cpu_user_regs *regs);
+#else
+static inline void initialize_keytable(void) {}
+static inline void register_keyhandler(unsigned char key, keyhandler_fn_t *fn,
+ const char *desc, bool_t diagnostic) {}
+static inline void register_irq_keyhandler(unsigned char key,
+ irq_keyhandler_fn_t *fn,
+ const char *desc,
+ bool_t diagnostic) {}
+
+static inline void handle_keypress(unsigned char key,
+ struct cpu_user_regs *regs) {}
+#endif
+
#endif /* __XEN_KEYHANDLER_H__ */
diff --git a/xen/include/xen/lib.h b/xen/include/xen/lib.h
index e0b7bcb..8710305 100644
--- a/xen/include/xen/lib.h
+++ b/xen/include/xen/lib.h
@@ -171,8 +171,10 @@ extern unsigned int tainted;
extern char *print_tainted(char *str);
extern void add_taint(unsigned int taint);
+#ifdef CONFIG_HAS_KEYHANDLER
struct cpu_user_regs;
void dump_execstate(struct cpu_user_regs *);
+#endif
void init_constructors(void);
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index 748bb0f..b82cdee 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -994,8 +994,10 @@ int cpupool_add_domain(struct domain *d, int poolid);
void cpupool_rm_domain(struct domain *d);
int cpupool_move_domain(struct domain *d, struct cpupool *c);
int cpupool_do_sysctl(struct xen_sysctl_cpupool_op *op);
+#ifdef CONFIG_HAS_KEYHANDLER
void schedule_dump(struct cpupool *c);
extern void dump_runq(unsigned char key);
+#endif
void arch_do_physinfo(struct xen_sysctl_physinfo *pi);
--
2.7.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [Xen-devel] [PATCH] xen: make keyhanler configurable
2019-05-31 1:58 [PATCH] xen: make keyhanler configurable Baodong Chen
@ 2019-05-31 1:58 ` Baodong Chen
2019-05-31 10:39 ` Dario Faggioli
` (2 subsequent siblings)
3 siblings, 0 replies; 14+ messages in thread
From: Baodong Chen @ 2019-05-31 1:58 UTC (permalink / raw)
To: xen-devel
Cc: Juergen Gross, Stefano Stabellini, Wei Liu,
Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson,
Tim Deegan, Julien Grall, Jan Beulich, Baodong Chen,
Dario Faggioli, Roger Pau Monné
keyhandler mainly used for debug usage by developers which can dump
xen module(eg. timer, scheduler) status at runtime by input
character from console.
Signed-off-by: Baodong Chen <chenbaodong@mxnavi.com>
---
xen/arch/arm/gic.c | 2 ++
xen/arch/x86/apic.c | 2 ++
xen/common/Kconfig | 9 +++++++++
xen/common/Makefile | 2 +-
xen/common/cpupool.c | 2 ++
xen/common/schedule.c | 2 ++
xen/include/xen/keyhandler.h | 14 ++++++++++++++
xen/include/xen/lib.h | 2 ++
xen/include/xen/sched.h | 2 ++
9 files changed, 36 insertions(+), 1 deletion(-)
diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 6cc7dec..fff88c5 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -361,7 +361,9 @@ static void do_sgi(struct cpu_user_regs *regs, enum gic_sgi sgi)
/* Nothing to do, will check for events on return path */
break;
case GIC_SGI_DUMP_STATE:
+#ifdef CONFIG_HAS_KEYHANDLER
dump_execstate(regs);
+#endif
break;
case GIC_SGI_CALL_FUNCTION:
smp_call_function_interrupt();
diff --git a/xen/arch/x86/apic.c b/xen/arch/x86/apic.c
index fafc0bd..e5f004a 100644
--- a/xen/arch/x86/apic.c
+++ b/xen/arch/x86/apic.c
@@ -1410,7 +1410,9 @@ void spurious_interrupt(struct cpu_user_regs *regs)
ack_APIC_irq();
if (this_cpu(state_dump_pending)) {
this_cpu(state_dump_pending) = false;
+#ifdef CONFIG_HAS_KEYHANDLER
dump_execstate(regs);
+#endif
return;
}
}
diff --git a/xen/common/Kconfig b/xen/common/Kconfig
index c838506..450541c 100644
--- a/xen/common/Kconfig
+++ b/xen/common/Kconfig
@@ -368,4 +368,13 @@ config DOM0_MEM
Leave empty if you are not sure what to specify.
+config HAS_KEYHANDLER
+ bool "Enable/Disable keyhandler"
+ default y
+ ---help---
+ Enable or disable keyhandler function.
+ keyhandler mainly used for debug usage by developers which can dump
+ xen module(eg. timer, scheduler) status at runtime by input character
+ from console.
+
endmenu
diff --git a/xen/common/Makefile b/xen/common/Makefile
index bca48e6..c7bcd26 100644
--- a/xen/common/Makefile
+++ b/xen/common/Makefile
@@ -16,7 +16,7 @@ obj-y += guestcopy.o
obj-bin-y += gunzip.init.o
obj-y += irq.o
obj-y += kernel.o
-obj-y += keyhandler.o
+obj-$(CONFIG_HAS_KEYHANDLER) += keyhandler.o
obj-$(CONFIG_KEXEC) += kexec.o
obj-$(CONFIG_KEXEC) += kimage.o
obj-y += lib.o
diff --git a/xen/common/cpupool.c b/xen/common/cpupool.c
index 31ac323..721a729 100644
--- a/xen/common/cpupool.c
+++ b/xen/common/cpupool.c
@@ -699,6 +699,7 @@ int cpupool_do_sysctl(struct xen_sysctl_cpupool_op *op)
return ret;
}
+#ifdef CONFIG_HAS_KEYHANDLER
void dump_runq(unsigned char key)
{
unsigned long flags;
@@ -730,6 +731,7 @@ void dump_runq(unsigned char key)
local_irq_restore(flags);
spin_unlock(&cpupool_lock);
}
+#endif
static int cpu_callback(
struct notifier_block *nfb, unsigned long action, void *hcpu)
diff --git a/xen/common/schedule.c b/xen/common/schedule.c
index 66f1e26..617c444 100644
--- a/xen/common/schedule.c
+++ b/xen/common/schedule.c
@@ -1913,6 +1913,7 @@ void scheduler_free(struct scheduler *sched)
xfree(sched);
}
+#ifdef CONFIG_HAS_KEYHANDLER
void schedule_dump(struct cpupool *c)
{
unsigned int i;
@@ -1941,6 +1942,7 @@ void schedule_dump(struct cpupool *c)
SCHED_OP(sched, dump_cpu_state, i);
}
}
+#endif
void sched_tick_suspend(void)
{
diff --git a/xen/include/xen/keyhandler.h b/xen/include/xen/keyhandler.h
index 5131e86..1050b80 100644
--- a/xen/include/xen/keyhandler.h
+++ b/xen/include/xen/keyhandler.h
@@ -28,6 +28,7 @@ struct cpu_user_regs;
typedef void (irq_keyhandler_fn_t)(unsigned char key,
struct cpu_user_regs *regs);
+#ifdef CONFIG_HAS_KEYHANDLER
/* Initialize keytable with default handlers. */
void initialize_keytable(void);
@@ -48,4 +49,17 @@ void register_irq_keyhandler(unsigned char key,
/* Inject a keypress into the key-handling subsystem. */
extern void handle_keypress(unsigned char key, struct cpu_user_regs *regs);
+#else
+static inline void initialize_keytable(void) {}
+static inline void register_keyhandler(unsigned char key, keyhandler_fn_t *fn,
+ const char *desc, bool_t diagnostic) {}
+static inline void register_irq_keyhandler(unsigned char key,
+ irq_keyhandler_fn_t *fn,
+ const char *desc,
+ bool_t diagnostic) {}
+
+static inline void handle_keypress(unsigned char key,
+ struct cpu_user_regs *regs) {}
+#endif
+
#endif /* __XEN_KEYHANDLER_H__ */
diff --git a/xen/include/xen/lib.h b/xen/include/xen/lib.h
index e0b7bcb..8710305 100644
--- a/xen/include/xen/lib.h
+++ b/xen/include/xen/lib.h
@@ -171,8 +171,10 @@ extern unsigned int tainted;
extern char *print_tainted(char *str);
extern void add_taint(unsigned int taint);
+#ifdef CONFIG_HAS_KEYHANDLER
struct cpu_user_regs;
void dump_execstate(struct cpu_user_regs *);
+#endif
void init_constructors(void);
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index 748bb0f..b82cdee 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -994,8 +994,10 @@ int cpupool_add_domain(struct domain *d, int poolid);
void cpupool_rm_domain(struct domain *d);
int cpupool_move_domain(struct domain *d, struct cpupool *c);
int cpupool_do_sysctl(struct xen_sysctl_cpupool_op *op);
+#ifdef CONFIG_HAS_KEYHANDLER
void schedule_dump(struct cpupool *c);
extern void dump_runq(unsigned char key);
+#endif
void arch_do_physinfo(struct xen_sysctl_physinfo *pi);
--
2.7.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] xen: make keyhanler configurable
2019-05-31 1:58 [PATCH] xen: make keyhanler configurable Baodong Chen
2019-05-31 1:58 ` [Xen-devel] " Baodong Chen
@ 2019-05-31 10:39 ` Dario Faggioli
2019-05-31 10:39 ` [Xen-devel] " Dario Faggioli
2019-06-03 5:02 ` chenbaodong
2019-05-31 10:53 ` Juergen Gross
2019-05-31 22:30 ` Andrew Cooper
3 siblings, 2 replies; 14+ messages in thread
From: Dario Faggioli @ 2019-05-31 10:39 UTC (permalink / raw)
To: Baodong Chen, xen-devel
Cc: Juergen Gross, Stefano Stabellini, Wei Liu,
Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson,
Tim Deegan, Julien Grall, Jan Beulich, Roger Pau Monné
[-- Attachment #1.1: Type: text/plain, Size: 5044 bytes --]
On Fri, 2019-05-31 at 09:58 +0800, Baodong Chen wrote:
> keyhandler mainly used for debug usage by developers which can dump
> xen module(eg. timer, scheduler) status at runtime by input
> character from console.
>
> Signed-off-by: Baodong Chen <chenbaodong@mxnavi.com>
> ---
> --- a/xen/common/Kconfig
> +++ b/xen/common/Kconfig
> @@ -368,4 +368,13 @@ config DOM0_MEM
>
> Leave empty if you are not sure what to specify.
>
> +config HAS_KEYHANDLER
> + bool "Enable/Disable keyhandler"
> + default y
> + ---help---
> + Enable or disable keyhandler function.
> + keyhandler mainly used for debug usage by developers which
> can dump
> + xen module(eg. timer, scheduler) status at runtime by input
> character
> + from console.
> +
> endmenu
>
I personally like the idea.
I've probably would have called the option CONFIG_KEYHANDLERS, even if
I can see that we have quite a few CONFIG_HAS_*.
But it's not for me to ask/decide, and I don't have a too strong
opinion on this anyway, so let's hear what others think.
I'd at least add the 'S', though (as in CONFIG_HAS_KEYHANDLERS).
> --- a/xen/common/cpupool.c
> +++ b/xen/common/cpupool.c
> @@ -699,6 +699,7 @@ int cpupool_do_sysctl(struct
> xen_sysctl_cpupool_op *op)
> return ret;
> }
>
> +#ifdef CONFIG_HAS_KEYHANDLER
> void dump_runq(unsigned char key)
> {
> unsigned long flags;
> @@ -730,6 +731,7 @@ void dump_runq(unsigned char key)
> local_irq_restore(flags);
> spin_unlock(&cpupool_lock);
> }
> +#endif
>
> static int cpu_callback(
> struct notifier_block *nfb, unsigned long action, void *hcpu)
> --- a/xen/common/schedule.c
> +++ b/xen/common/schedule.c
> @@ -1913,6 +1913,7 @@ void scheduler_free(struct scheduler *sched)
> xfree(sched);
> }
>
> +#ifdef CONFIG_HAS_KEYHANDLER
> void schedule_dump(struct cpupool *c)
> {
> unsigned int i;
> @@ -1941,6 +1942,7 @@ void schedule_dump(struct cpupool *c)
> SCHED_OP(sched, dump_cpu_state, i);
> }
> }
> +#endif
>
> void sched_tick_suspend(void)
> {
Mmm... a lot of #ifdef-fery spread around quite a bit.. I have to admit
I don't especially like that.
> --- a/xen/include/xen/keyhandler.h
> +++ b/xen/include/xen/keyhandler.h
> @@ -48,4 +49,17 @@ void register_irq_keyhandler(unsigned char key,
> /* Inject a keypress into the key-handling subsystem. */
> extern void handle_keypress(unsigned char key, struct cpu_user_regs
> *regs);
>
> +#else
> +static inline void initialize_keytable(void) {}
> +static inline void register_keyhandler(unsigned char key,
> keyhandler_fn_t *fn,
> + const char *desc, bool_t
> diagnostic) {}
> +static inline void register_irq_keyhandler(unsigned char key,
> + irq_keyhandler_fn_t *fn,
> + const char *desc,
> + bool_t diagnostic) {}
> +
> +static inline void handle_keypress(unsigned char key,
> + struct cpu_user_regs *regs) {}
>
So, with this last change, we have:
static DECLARE_TASKLET(keypress_tasklet, keypress_action, 0);
But since all keypress_action() does is calling handle_keypress(),
which is becoming a nop... can't we kill the tasklet alltogether?
> --- a/xen/include/xen/lib.h
> +++ b/xen/include/xen/lib.h
> @@ -171,8 +171,10 @@ extern unsigned int tainted;
> extern char *print_tainted(char *str);
> extern void add_taint(unsigned int taint);
>
> +#ifdef CONFIG_HAS_KEYHANDLER
> struct cpu_user_regs;
> void dump_execstate(struct cpu_user_regs *);
> +#endif
>
Yes. Or, you provide an empty stub of dump_execstate(), if
CONFIG_HAS_KEYHANDLER is not defined, which means we don't have to mess
with #ifdef-s at the caller site(s).
Of course, I'm not maintainer of this specific piece of code, but I'd
prefer this stub-based approach to be used in general.... ... ...
> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -994,8 +994,10 @@ int cpupool_add_domain(struct domain *d, int
> poolid);
> void cpupool_rm_domain(struct domain *d);
> int cpupool_move_domain(struct domain *d, struct cpupool *c);
> int cpupool_do_sysctl(struct xen_sysctl_cpupool_op *op);
> +#ifdef CONFIG_HAS_KEYHANDLER
> void schedule_dump(struct cpupool *c);
> extern void dump_runq(unsigned char key);
> +#endif
>
> void arch_do_physinfo(struct xen_sysctl_physinfo *pi);
>
... ... ... Like, for instance, in here.
But again, sine these changes are spread around many files, let's see
what others prefer, and use the same strategy everywhere.
Regards
--
Dario Faggioli, Ph.D
http://about.me/dario.faggioli
Virtualization Software Engineer
SUSE Labs, SUSE https://www.suse.com/
-------------------------------------------------------------------
<<This happens because _I_ choose it to happen!>> (Raistlin Majere)
[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
[-- Attachment #2: Type: text/plain, Size: 157 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Xen-devel] [PATCH] xen: make keyhanler configurable
2019-05-31 10:39 ` Dario Faggioli
@ 2019-05-31 10:39 ` Dario Faggioli
2019-06-03 5:02 ` chenbaodong
1 sibling, 0 replies; 14+ messages in thread
From: Dario Faggioli @ 2019-05-31 10:39 UTC (permalink / raw)
To: Baodong Chen, xen-devel
Cc: Juergen Gross, Stefano Stabellini, Wei Liu,
Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson,
Tim Deegan, Julien Grall, Jan Beulich, Roger Pau Monné
[-- Attachment #1.1: Type: text/plain, Size: 5044 bytes --]
On Fri, 2019-05-31 at 09:58 +0800, Baodong Chen wrote:
> keyhandler mainly used for debug usage by developers which can dump
> xen module(eg. timer, scheduler) status at runtime by input
> character from console.
>
> Signed-off-by: Baodong Chen <chenbaodong@mxnavi.com>
> ---
> --- a/xen/common/Kconfig
> +++ b/xen/common/Kconfig
> @@ -368,4 +368,13 @@ config DOM0_MEM
>
> Leave empty if you are not sure what to specify.
>
> +config HAS_KEYHANDLER
> + bool "Enable/Disable keyhandler"
> + default y
> + ---help---
> + Enable or disable keyhandler function.
> + keyhandler mainly used for debug usage by developers which
> can dump
> + xen module(eg. timer, scheduler) status at runtime by input
> character
> + from console.
> +
> endmenu
>
I personally like the idea.
I've probably would have called the option CONFIG_KEYHANDLERS, even if
I can see that we have quite a few CONFIG_HAS_*.
But it's not for me to ask/decide, and I don't have a too strong
opinion on this anyway, so let's hear what others think.
I'd at least add the 'S', though (as in CONFIG_HAS_KEYHANDLERS).
> --- a/xen/common/cpupool.c
> +++ b/xen/common/cpupool.c
> @@ -699,6 +699,7 @@ int cpupool_do_sysctl(struct
> xen_sysctl_cpupool_op *op)
> return ret;
> }
>
> +#ifdef CONFIG_HAS_KEYHANDLER
> void dump_runq(unsigned char key)
> {
> unsigned long flags;
> @@ -730,6 +731,7 @@ void dump_runq(unsigned char key)
> local_irq_restore(flags);
> spin_unlock(&cpupool_lock);
> }
> +#endif
>
> static int cpu_callback(
> struct notifier_block *nfb, unsigned long action, void *hcpu)
> --- a/xen/common/schedule.c
> +++ b/xen/common/schedule.c
> @@ -1913,6 +1913,7 @@ void scheduler_free(struct scheduler *sched)
> xfree(sched);
> }
>
> +#ifdef CONFIG_HAS_KEYHANDLER
> void schedule_dump(struct cpupool *c)
> {
> unsigned int i;
> @@ -1941,6 +1942,7 @@ void schedule_dump(struct cpupool *c)
> SCHED_OP(sched, dump_cpu_state, i);
> }
> }
> +#endif
>
> void sched_tick_suspend(void)
> {
Mmm... a lot of #ifdef-fery spread around quite a bit.. I have to admit
I don't especially like that.
> --- a/xen/include/xen/keyhandler.h
> +++ b/xen/include/xen/keyhandler.h
> @@ -48,4 +49,17 @@ void register_irq_keyhandler(unsigned char key,
> /* Inject a keypress into the key-handling subsystem. */
> extern void handle_keypress(unsigned char key, struct cpu_user_regs
> *regs);
>
> +#else
> +static inline void initialize_keytable(void) {}
> +static inline void register_keyhandler(unsigned char key,
> keyhandler_fn_t *fn,
> + const char *desc, bool_t
> diagnostic) {}
> +static inline void register_irq_keyhandler(unsigned char key,
> + irq_keyhandler_fn_t *fn,
> + const char *desc,
> + bool_t diagnostic) {}
> +
> +static inline void handle_keypress(unsigned char key,
> + struct cpu_user_regs *regs) {}
>
So, with this last change, we have:
static DECLARE_TASKLET(keypress_tasklet, keypress_action, 0);
But since all keypress_action() does is calling handle_keypress(),
which is becoming a nop... can't we kill the tasklet alltogether?
> --- a/xen/include/xen/lib.h
> +++ b/xen/include/xen/lib.h
> @@ -171,8 +171,10 @@ extern unsigned int tainted;
> extern char *print_tainted(char *str);
> extern void add_taint(unsigned int taint);
>
> +#ifdef CONFIG_HAS_KEYHANDLER
> struct cpu_user_regs;
> void dump_execstate(struct cpu_user_regs *);
> +#endif
>
Yes. Or, you provide an empty stub of dump_execstate(), if
CONFIG_HAS_KEYHANDLER is not defined, which means we don't have to mess
with #ifdef-s at the caller site(s).
Of course, I'm not maintainer of this specific piece of code, but I'd
prefer this stub-based approach to be used in general.... ... ...
> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -994,8 +994,10 @@ int cpupool_add_domain(struct domain *d, int
> poolid);
> void cpupool_rm_domain(struct domain *d);
> int cpupool_move_domain(struct domain *d, struct cpupool *c);
> int cpupool_do_sysctl(struct xen_sysctl_cpupool_op *op);
> +#ifdef CONFIG_HAS_KEYHANDLER
> void schedule_dump(struct cpupool *c);
> extern void dump_runq(unsigned char key);
> +#endif
>
> void arch_do_physinfo(struct xen_sysctl_physinfo *pi);
>
... ... ... Like, for instance, in here.
But again, sine these changes are spread around many files, let's see
what others prefer, and use the same strategy everywhere.
Regards
--
Dario Faggioli, Ph.D
http://about.me/dario.faggioli
Virtualization Software Engineer
SUSE Labs, SUSE https://www.suse.com/
-------------------------------------------------------------------
<<This happens because _I_ choose it to happen!>> (Raistlin Majere)
[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
[-- Attachment #2: Type: text/plain, Size: 157 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] xen: make keyhanler configurable
2019-05-31 1:58 [PATCH] xen: make keyhanler configurable Baodong Chen
2019-05-31 1:58 ` [Xen-devel] " Baodong Chen
2019-05-31 10:39 ` Dario Faggioli
@ 2019-05-31 10:53 ` Juergen Gross
2019-05-31 10:53 ` [Xen-devel] " Juergen Gross
2019-06-03 5:10 ` chenbaodong
2019-05-31 22:30 ` Andrew Cooper
3 siblings, 2 replies; 14+ messages in thread
From: Juergen Gross @ 2019-05-31 10:53 UTC (permalink / raw)
To: Baodong Chen, xen-devel
Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan,
Julien Grall, Jan Beulich, Dario Faggioli, Roger Pau Monné
On 31/05/2019 03:58, Baodong Chen wrote:
> keyhandler mainly used for debug usage by developers which can dump
> xen module(eg. timer, scheduler) status at runtime by input
> character from console.
>
> Signed-off-by: Baodong Chen <chenbaodong@mxnavi.com>
> ---
> xen/arch/arm/gic.c | 2 ++
> xen/arch/x86/apic.c | 2 ++
> xen/common/Kconfig | 9 +++++++++
> xen/common/Makefile | 2 +-
> xen/common/cpupool.c | 2 ++
> xen/common/schedule.c | 2 ++
> xen/include/xen/keyhandler.h | 14 ++++++++++++++
> xen/include/xen/lib.h | 2 ++
> xen/include/xen/sched.h | 2 ++
> 9 files changed, 36 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index 6cc7dec..fff88c5 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -361,7 +361,9 @@ static void do_sgi(struct cpu_user_regs *regs, enum gic_sgi sgi)
> /* Nothing to do, will check for events on return path */
> break;
> case GIC_SGI_DUMP_STATE:
> +#ifdef CONFIG_HAS_KEYHANDLER
> dump_execstate(regs);
> +#endif
> break;
> case GIC_SGI_CALL_FUNCTION:
> smp_call_function_interrupt();
> diff --git a/xen/arch/x86/apic.c b/xen/arch/x86/apic.c
> index fafc0bd..e5f004a 100644
> --- a/xen/arch/x86/apic.c
> +++ b/xen/arch/x86/apic.c
> @@ -1410,7 +1410,9 @@ void spurious_interrupt(struct cpu_user_regs *regs)
> ack_APIC_irq();
> if (this_cpu(state_dump_pending)) {
> this_cpu(state_dump_pending) = false;
> +#ifdef CONFIG_HAS_KEYHANDLER
> dump_execstate(regs);
> +#endif
> return;
> }
> }
> diff --git a/xen/common/Kconfig b/xen/common/Kconfig
> index c838506..450541c 100644
> --- a/xen/common/Kconfig
> +++ b/xen/common/Kconfig
> @@ -368,4 +368,13 @@ config DOM0_MEM
>
> Leave empty if you are not sure what to specify.
>
> +config HAS_KEYHANDLER
AFAIK the HAS_* config options are meant to be selected by other options
and not be user selectable.
So I think you should drop the "HAS_" and maybe use the plural as Dario
already suggested ("KEYHANDLERS").
> + bool "Enable/Disable keyhandler"
> + default y
> + ---help---
> + Enable or disable keyhandler function.
> + keyhandler mainly used for debug usage by developers which can dump
> + xen module(eg. timer, scheduler) status at runtime by input character
> + from console.
I'd drop the "by developers". In case of customer problems with Xen
hosts the output of keyhandlers is requested on a rather regular base.
> +
> endmenu
> diff --git a/xen/common/Makefile b/xen/common/Makefile
> index bca48e6..c7bcd26 100644
> --- a/xen/common/Makefile
> +++ b/xen/common/Makefile
> @@ -16,7 +16,7 @@ obj-y += guestcopy.o
> obj-bin-y += gunzip.init.o
> obj-y += irq.o
> obj-y += kernel.o
> -obj-y += keyhandler.o
> +obj-$(CONFIG_HAS_KEYHANDLER) += keyhandler.o
> obj-$(CONFIG_KEXEC) += kexec.o
> obj-$(CONFIG_KEXEC) += kimage.o
> obj-y += lib.o
> diff --git a/xen/common/cpupool.c b/xen/common/cpupool.c
> index 31ac323..721a729 100644
> --- a/xen/common/cpupool.c
> +++ b/xen/common/cpupool.c
> @@ -699,6 +699,7 @@ int cpupool_do_sysctl(struct xen_sysctl_cpupool_op *op)
> return ret;
> }
>
> +#ifdef CONFIG_HAS_KEYHANDLER
> void dump_runq(unsigned char key)
> {
> unsigned long flags;
> @@ -730,6 +731,7 @@ void dump_runq(unsigned char key)
> local_irq_restore(flags);
> spin_unlock(&cpupool_lock);
> }
> +#endif
>
> static int cpu_callback(
> struct notifier_block *nfb, unsigned long action, void *hcpu)
> diff --git a/xen/common/schedule.c b/xen/common/schedule.c
> index 66f1e26..617c444 100644
> --- a/xen/common/schedule.c
> +++ b/xen/common/schedule.c
> @@ -1913,6 +1913,7 @@ void scheduler_free(struct scheduler *sched)
> xfree(sched);
> }
>
> +#ifdef CONFIG_HAS_KEYHANDLER
> void schedule_dump(struct cpupool *c)
> {
> unsigned int i;
> @@ -1941,6 +1942,7 @@ void schedule_dump(struct cpupool *c)
> SCHED_OP(sched, dump_cpu_state, i);
> }
> }
> +#endif
>
> void sched_tick_suspend(void)
> {
> diff --git a/xen/include/xen/keyhandler.h b/xen/include/xen/keyhandler.h
> index 5131e86..1050b80 100644
> --- a/xen/include/xen/keyhandler.h
> +++ b/xen/include/xen/keyhandler.h
> @@ -28,6 +28,7 @@ struct cpu_user_regs;
> typedef void (irq_keyhandler_fn_t)(unsigned char key,
> struct cpu_user_regs *regs);
>
> +#ifdef CONFIG_HAS_KEYHANDLER
> /* Initialize keytable with default handlers. */
> void initialize_keytable(void);
>
> @@ -48,4 +49,17 @@ void register_irq_keyhandler(unsigned char key,
> /* Inject a keypress into the key-handling subsystem. */
> extern void handle_keypress(unsigned char key, struct cpu_user_regs *regs);
>
> +#else
> +static inline void initialize_keytable(void) {}
> +static inline void register_keyhandler(unsigned char key, keyhandler_fn_t *fn,
> + const char *desc, bool_t diagnostic) {}
> +static inline void register_irq_keyhandler(unsigned char key,
> + irq_keyhandler_fn_t *fn,
> + const char *desc,
> + bool_t diagnostic) {}
> +
> +static inline void handle_keypress(unsigned char key,
> + struct cpu_user_regs *regs) {}
> +#endif
> +
> #endif /* __XEN_KEYHANDLER_H__ */
> diff --git a/xen/include/xen/lib.h b/xen/include/xen/lib.h
> index e0b7bcb..8710305 100644
> --- a/xen/include/xen/lib.h
> +++ b/xen/include/xen/lib.h
> @@ -171,8 +171,10 @@ extern unsigned int tainted;
> extern char *print_tainted(char *str);
> extern void add_taint(unsigned int taint);
>
> +#ifdef CONFIG_HAS_KEYHANDLER
> struct cpu_user_regs;
> void dump_execstate(struct cpu_user_regs *);
> +#endif
>
> void init_constructors(void);
>
> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
> index 748bb0f..b82cdee 100644
> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -994,8 +994,10 @@ int cpupool_add_domain(struct domain *d, int poolid);
> void cpupool_rm_domain(struct domain *d);
> int cpupool_move_domain(struct domain *d, struct cpupool *c);
> int cpupool_do_sysctl(struct xen_sysctl_cpupool_op *op);
> +#ifdef CONFIG_HAS_KEYHANDLER
> void schedule_dump(struct cpupool *c);
> extern void dump_runq(unsigned char key);
> +#endif
>
> void arch_do_physinfo(struct xen_sysctl_physinfo *pi);
Why stopping halfway here? There are lots of other keyhandlers which can
be removed for the hypervisor in case there is no code calling them.
Juergen
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Xen-devel] [PATCH] xen: make keyhanler configurable
2019-05-31 10:53 ` Juergen Gross
@ 2019-05-31 10:53 ` Juergen Gross
2019-06-03 5:10 ` chenbaodong
1 sibling, 0 replies; 14+ messages in thread
From: Juergen Gross @ 2019-05-31 10:53 UTC (permalink / raw)
To: Baodong Chen, xen-devel
Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan,
Julien Grall, Jan Beulich, Dario Faggioli, Roger Pau Monné
On 31/05/2019 03:58, Baodong Chen wrote:
> keyhandler mainly used for debug usage by developers which can dump
> xen module(eg. timer, scheduler) status at runtime by input
> character from console.
>
> Signed-off-by: Baodong Chen <chenbaodong@mxnavi.com>
> ---
> xen/arch/arm/gic.c | 2 ++
> xen/arch/x86/apic.c | 2 ++
> xen/common/Kconfig | 9 +++++++++
> xen/common/Makefile | 2 +-
> xen/common/cpupool.c | 2 ++
> xen/common/schedule.c | 2 ++
> xen/include/xen/keyhandler.h | 14 ++++++++++++++
> xen/include/xen/lib.h | 2 ++
> xen/include/xen/sched.h | 2 ++
> 9 files changed, 36 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index 6cc7dec..fff88c5 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -361,7 +361,9 @@ static void do_sgi(struct cpu_user_regs *regs, enum gic_sgi sgi)
> /* Nothing to do, will check for events on return path */
> break;
> case GIC_SGI_DUMP_STATE:
> +#ifdef CONFIG_HAS_KEYHANDLER
> dump_execstate(regs);
> +#endif
> break;
> case GIC_SGI_CALL_FUNCTION:
> smp_call_function_interrupt();
> diff --git a/xen/arch/x86/apic.c b/xen/arch/x86/apic.c
> index fafc0bd..e5f004a 100644
> --- a/xen/arch/x86/apic.c
> +++ b/xen/arch/x86/apic.c
> @@ -1410,7 +1410,9 @@ void spurious_interrupt(struct cpu_user_regs *regs)
> ack_APIC_irq();
> if (this_cpu(state_dump_pending)) {
> this_cpu(state_dump_pending) = false;
> +#ifdef CONFIG_HAS_KEYHANDLER
> dump_execstate(regs);
> +#endif
> return;
> }
> }
> diff --git a/xen/common/Kconfig b/xen/common/Kconfig
> index c838506..450541c 100644
> --- a/xen/common/Kconfig
> +++ b/xen/common/Kconfig
> @@ -368,4 +368,13 @@ config DOM0_MEM
>
> Leave empty if you are not sure what to specify.
>
> +config HAS_KEYHANDLER
AFAIK the HAS_* config options are meant to be selected by other options
and not be user selectable.
So I think you should drop the "HAS_" and maybe use the plural as Dario
already suggested ("KEYHANDLERS").
> + bool "Enable/Disable keyhandler"
> + default y
> + ---help---
> + Enable or disable keyhandler function.
> + keyhandler mainly used for debug usage by developers which can dump
> + xen module(eg. timer, scheduler) status at runtime by input character
> + from console.
I'd drop the "by developers". In case of customer problems with Xen
hosts the output of keyhandlers is requested on a rather regular base.
> +
> endmenu
> diff --git a/xen/common/Makefile b/xen/common/Makefile
> index bca48e6..c7bcd26 100644
> --- a/xen/common/Makefile
> +++ b/xen/common/Makefile
> @@ -16,7 +16,7 @@ obj-y += guestcopy.o
> obj-bin-y += gunzip.init.o
> obj-y += irq.o
> obj-y += kernel.o
> -obj-y += keyhandler.o
> +obj-$(CONFIG_HAS_KEYHANDLER) += keyhandler.o
> obj-$(CONFIG_KEXEC) += kexec.o
> obj-$(CONFIG_KEXEC) += kimage.o
> obj-y += lib.o
> diff --git a/xen/common/cpupool.c b/xen/common/cpupool.c
> index 31ac323..721a729 100644
> --- a/xen/common/cpupool.c
> +++ b/xen/common/cpupool.c
> @@ -699,6 +699,7 @@ int cpupool_do_sysctl(struct xen_sysctl_cpupool_op *op)
> return ret;
> }
>
> +#ifdef CONFIG_HAS_KEYHANDLER
> void dump_runq(unsigned char key)
> {
> unsigned long flags;
> @@ -730,6 +731,7 @@ void dump_runq(unsigned char key)
> local_irq_restore(flags);
> spin_unlock(&cpupool_lock);
> }
> +#endif
>
> static int cpu_callback(
> struct notifier_block *nfb, unsigned long action, void *hcpu)
> diff --git a/xen/common/schedule.c b/xen/common/schedule.c
> index 66f1e26..617c444 100644
> --- a/xen/common/schedule.c
> +++ b/xen/common/schedule.c
> @@ -1913,6 +1913,7 @@ void scheduler_free(struct scheduler *sched)
> xfree(sched);
> }
>
> +#ifdef CONFIG_HAS_KEYHANDLER
> void schedule_dump(struct cpupool *c)
> {
> unsigned int i;
> @@ -1941,6 +1942,7 @@ void schedule_dump(struct cpupool *c)
> SCHED_OP(sched, dump_cpu_state, i);
> }
> }
> +#endif
>
> void sched_tick_suspend(void)
> {
> diff --git a/xen/include/xen/keyhandler.h b/xen/include/xen/keyhandler.h
> index 5131e86..1050b80 100644
> --- a/xen/include/xen/keyhandler.h
> +++ b/xen/include/xen/keyhandler.h
> @@ -28,6 +28,7 @@ struct cpu_user_regs;
> typedef void (irq_keyhandler_fn_t)(unsigned char key,
> struct cpu_user_regs *regs);
>
> +#ifdef CONFIG_HAS_KEYHANDLER
> /* Initialize keytable with default handlers. */
> void initialize_keytable(void);
>
> @@ -48,4 +49,17 @@ void register_irq_keyhandler(unsigned char key,
> /* Inject a keypress into the key-handling subsystem. */
> extern void handle_keypress(unsigned char key, struct cpu_user_regs *regs);
>
> +#else
> +static inline void initialize_keytable(void) {}
> +static inline void register_keyhandler(unsigned char key, keyhandler_fn_t *fn,
> + const char *desc, bool_t diagnostic) {}
> +static inline void register_irq_keyhandler(unsigned char key,
> + irq_keyhandler_fn_t *fn,
> + const char *desc,
> + bool_t diagnostic) {}
> +
> +static inline void handle_keypress(unsigned char key,
> + struct cpu_user_regs *regs) {}
> +#endif
> +
> #endif /* __XEN_KEYHANDLER_H__ */
> diff --git a/xen/include/xen/lib.h b/xen/include/xen/lib.h
> index e0b7bcb..8710305 100644
> --- a/xen/include/xen/lib.h
> +++ b/xen/include/xen/lib.h
> @@ -171,8 +171,10 @@ extern unsigned int tainted;
> extern char *print_tainted(char *str);
> extern void add_taint(unsigned int taint);
>
> +#ifdef CONFIG_HAS_KEYHANDLER
> struct cpu_user_regs;
> void dump_execstate(struct cpu_user_regs *);
> +#endif
>
> void init_constructors(void);
>
> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
> index 748bb0f..b82cdee 100644
> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -994,8 +994,10 @@ int cpupool_add_domain(struct domain *d, int poolid);
> void cpupool_rm_domain(struct domain *d);
> int cpupool_move_domain(struct domain *d, struct cpupool *c);
> int cpupool_do_sysctl(struct xen_sysctl_cpupool_op *op);
> +#ifdef CONFIG_HAS_KEYHANDLER
> void schedule_dump(struct cpupool *c);
> extern void dump_runq(unsigned char key);
> +#endif
>
> void arch_do_physinfo(struct xen_sysctl_physinfo *pi);
Why stopping halfway here? There are lots of other keyhandlers which can
be removed for the hypervisor in case there is no code calling them.
Juergen
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] xen: make keyhanler configurable
2019-05-31 1:58 [PATCH] xen: make keyhanler configurable Baodong Chen
` (2 preceding siblings ...)
2019-05-31 10:53 ` Juergen Gross
@ 2019-05-31 22:30 ` Andrew Cooper
2019-05-31 22:30 ` [Xen-devel] " Andrew Cooper
2019-06-03 5:35 ` chenbaodong
3 siblings, 2 replies; 14+ messages in thread
From: Andrew Cooper @ 2019-05-31 22:30 UTC (permalink / raw)
To: Baodong Chen, xen-devel
Cc: Juergen Gross, Stefano Stabellini, Wei Liu,
Konrad Rzeszutek Wilk, George Dunlap, Tim Deegan, Ian Jackson,
Dario Faggioli, Julien Grall, Jan Beulich, Roger Pau Monné
On 30/05/2019 18:58, Baodong Chen wrote:
> keyhandler mainly used for debug usage by developers which can dump
> xen module(eg. timer, scheduler) status at runtime by input
> character from console.
>
> Signed-off-by: Baodong Chen <chenbaodong@mxnavi.com>
What is the motivation here? I don't have a specific objection to
making this configurable (so long as it excises the entire keyhandler
infrastructure, which is rather more than this patch does), but I also
don't see why we would want to do so in the first place.
In particular, it doesn't require a serial console to function correctly
in the first place. This functionality can be used with `xl debug-keys
$char; xl dmesg`
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Xen-devel] [PATCH] xen: make keyhanler configurable
2019-05-31 22:30 ` Andrew Cooper
@ 2019-05-31 22:30 ` Andrew Cooper
2019-06-03 5:35 ` chenbaodong
1 sibling, 0 replies; 14+ messages in thread
From: Andrew Cooper @ 2019-05-31 22:30 UTC (permalink / raw)
To: Baodong Chen, xen-devel
Cc: Juergen Gross, Stefano Stabellini, Wei Liu,
Konrad Rzeszutek Wilk, George Dunlap, Tim Deegan, Ian Jackson,
Dario Faggioli, Julien Grall, Jan Beulich, Roger Pau Monné
On 30/05/2019 18:58, Baodong Chen wrote:
> keyhandler mainly used for debug usage by developers which can dump
> xen module(eg. timer, scheduler) status at runtime by input
> character from console.
>
> Signed-off-by: Baodong Chen <chenbaodong@mxnavi.com>
What is the motivation here? I don't have a specific objection to
making this configurable (so long as it excises the entire keyhandler
infrastructure, which is rather more than this patch does), but I also
don't see why we would want to do so in the first place.
In particular, it doesn't require a serial console to function correctly
in the first place. This functionality can be used with `xl debug-keys
$char; xl dmesg`
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] xen: make keyhanler configurable
2019-05-31 10:39 ` Dario Faggioli
2019-05-31 10:39 ` [Xen-devel] " Dario Faggioli
@ 2019-06-03 5:02 ` chenbaodong
2019-06-03 5:02 ` [Xen-devel] " chenbaodong
1 sibling, 1 reply; 14+ messages in thread
From: chenbaodong @ 2019-06-03 5:02 UTC (permalink / raw)
To: Dario Faggioli, xen-devel
Cc: Juergen Gross, Stefano Stabellini, Wei Liu,
Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson,
Tim Deegan, Julien Grall, Jan Beulich, Roger Pau Monné
On 5/31/19 18:39, Dario Faggioli wrote:
> On Fri, 2019-05-31 at 09:58 +0800, Baodong Chen wrote:
>> keyhandler mainly used for debug usage by developers which can dump
>> xen module(eg. timer, scheduler) status at runtime by input
>> character from console.
>>
>> Signed-off-by: Baodong Chen <chenbaodong@mxnavi.com>
>> ---
>> --- a/xen/common/Kconfig
>> +++ b/xen/common/Kconfig
>> @@ -368,4 +368,13 @@ config DOM0_MEM
>>
>> Leave empty if you are not sure what to specify.
>>
>> +config HAS_KEYHANDLER
>> + bool "Enable/Disable keyhandler"
>> + default y
>> + ---help---
>> + Enable or disable keyhandler function.
>> + keyhandler mainly used for debug usage by developers which
>> can dump
>> + xen module(eg. timer, scheduler) status at runtime by input
>> character
>> + from console.
>> +
>> endmenu
>>
> I personally like the idea.
>
> I've probably would have called the option CONFIG_KEYHANDLERS, even if
> I can see that we have quite a few CONFIG_HAS_*.
>
> But it's not for me to ask/decide, and I don't have a too strong
> opinion on this anyway, so let's hear what others think.
>
> I'd at least add the 'S', though (as in CONFIG_HAS_KEYHANDLERS).
Yes, can be fixed.
>
>> --- a/xen/common/cpupool.c
>> +++ b/xen/common/cpupool.c
>> @@ -699,6 +699,7 @@ int cpupool_do_sysctl(struct
>> xen_sysctl_cpupool_op *op)
>> return ret;
>> }
>>
>> +#ifdef CONFIG_HAS_KEYHANDLER
>> void dump_runq(unsigned char key)
>> {
>> unsigned long flags;
>> @@ -730,6 +731,7 @@ void dump_runq(unsigned char key)
>> local_irq_restore(flags);
>> spin_unlock(&cpupool_lock);
>> }
>> +#endif
>>
>> static int cpu_callback(
>> struct notifier_block *nfb, unsigned long action, void *hcpu)
>> --- a/xen/common/schedule.c
>> +++ b/xen/common/schedule.c
>> @@ -1913,6 +1913,7 @@ void scheduler_free(struct scheduler *sched)
>> xfree(sched);
>> }
>>
>> +#ifdef CONFIG_HAS_KEYHANDLER
>> void schedule_dump(struct cpupool *c)
>> {
>> unsigned int i;
>> @@ -1941,6 +1942,7 @@ void schedule_dump(struct cpupool *c)
>> SCHED_OP(sched, dump_cpu_state, i);
>> }
>> }
>> +#endif
>>
>> void sched_tick_suspend(void)
>> {
> Mmm... a lot of #ifdef-fery spread around quite a bit.. I have to admit
> I don't especially like that.
Me too, can leave it as what is was.
but since schedule_dump prototype have external linkage.
so even no one will call it, it maybe still in output executable file,
right?
>> --- a/xen/include/xen/keyhandler.h
>> +++ b/xen/include/xen/keyhandler.h
>> @@ -48,4 +49,17 @@ void register_irq_keyhandler(unsigned char key,
>> /* Inject a keypress into the key-handling subsystem. */
>> extern void handle_keypress(unsigned char key, struct cpu_user_regs
>> *regs);
>>
>> +#else
>> +static inline void initialize_keytable(void) {}
>> +static inline void register_keyhandler(unsigned char key,
>> keyhandler_fn_t *fn,
>> + const char *desc, bool_t
>> diagnostic) {}
>> +static inline void register_irq_keyhandler(unsigned char key,
>> + irq_keyhandler_fn_t *fn,
>> + const char *desc,
>> + bool_t diagnostic) {}
>> +
>> +static inline void handle_keypress(unsigned char key,
>> + struct cpu_user_regs *regs) {}
>>
> So, with this last change, we have:
>
> static DECLARE_TASKLET(keypress_tasklet, keypress_action, 0);
>
> But since all keypress_action() does is calling handle_keypress(),
> which is becoming a nop... can't we kill the tasklet alltogether?
the whole keyhandler.c will not compiled when config disabled.
am i misunderstood something?
>
>> --- a/xen/include/xen/lib.h
>> +++ b/xen/include/xen/lib.h
>> @@ -171,8 +171,10 @@ extern unsigned int tainted;
>> extern char *print_tainted(char *str);
>> extern void add_taint(unsigned int taint);
>>
>> +#ifdef CONFIG_HAS_KEYHANDLER
>> struct cpu_user_regs;
>> void dump_execstate(struct cpu_user_regs *);
>> +#endif
>>
> Yes. Or, you provide an empty stub of dump_execstate(), if
> CONFIG_HAS_KEYHANDLER is not defined, which means we don't have to mess
> with #ifdef-s at the caller site(s).
>
> Of course, I'm not maintainer of this specific piece of code, but I'd
> prefer this stub-based approach to be used in general.... ... ...
Agree, can be fixed.
>
>> --- a/xen/include/xen/sched.h
>> +++ b/xen/include/xen/sched.h
>> @@ -994,8 +994,10 @@ int cpupool_add_domain(struct domain *d, int
>> poolid);
>> void cpupool_rm_domain(struct domain *d);
>> int cpupool_move_domain(struct domain *d, struct cpupool *c);
>> int cpupool_do_sysctl(struct xen_sysctl_cpupool_op *op);
>> +#ifdef CONFIG_HAS_KEYHANDLER
>> void schedule_dump(struct cpupool *c);
>> extern void dump_runq(unsigned char key);
>> +#endif
>>
>> void arch_do_physinfo(struct xen_sysctl_physinfo *pi);
>>
> ... ... ... Like, for instance, in here.
>
> But again, sine these changes are spread around many files, let's see
> what others prefer, and use the same strategy everywhere.
>
> Regards
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Xen-devel] [PATCH] xen: make keyhanler configurable
2019-06-03 5:02 ` chenbaodong
@ 2019-06-03 5:02 ` chenbaodong
0 siblings, 0 replies; 14+ messages in thread
From: chenbaodong @ 2019-06-03 5:02 UTC (permalink / raw)
To: Dario Faggioli, xen-devel
Cc: Juergen Gross, Stefano Stabellini, Wei Liu,
Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson,
Tim Deegan, Julien Grall, Jan Beulich, Roger Pau Monné
On 5/31/19 18:39, Dario Faggioli wrote:
> On Fri, 2019-05-31 at 09:58 +0800, Baodong Chen wrote:
>> keyhandler mainly used for debug usage by developers which can dump
>> xen module(eg. timer, scheduler) status at runtime by input
>> character from console.
>>
>> Signed-off-by: Baodong Chen <chenbaodong@mxnavi.com>
>> ---
>> --- a/xen/common/Kconfig
>> +++ b/xen/common/Kconfig
>> @@ -368,4 +368,13 @@ config DOM0_MEM
>>
>> Leave empty if you are not sure what to specify.
>>
>> +config HAS_KEYHANDLER
>> + bool "Enable/Disable keyhandler"
>> + default y
>> + ---help---
>> + Enable or disable keyhandler function.
>> + keyhandler mainly used for debug usage by developers which
>> can dump
>> + xen module(eg. timer, scheduler) status at runtime by input
>> character
>> + from console.
>> +
>> endmenu
>>
> I personally like the idea.
>
> I've probably would have called the option CONFIG_KEYHANDLERS, even if
> I can see that we have quite a few CONFIG_HAS_*.
>
> But it's not for me to ask/decide, and I don't have a too strong
> opinion on this anyway, so let's hear what others think.
>
> I'd at least add the 'S', though (as in CONFIG_HAS_KEYHANDLERS).
Yes, can be fixed.
>
>> --- a/xen/common/cpupool.c
>> +++ b/xen/common/cpupool.c
>> @@ -699,6 +699,7 @@ int cpupool_do_sysctl(struct
>> xen_sysctl_cpupool_op *op)
>> return ret;
>> }
>>
>> +#ifdef CONFIG_HAS_KEYHANDLER
>> void dump_runq(unsigned char key)
>> {
>> unsigned long flags;
>> @@ -730,6 +731,7 @@ void dump_runq(unsigned char key)
>> local_irq_restore(flags);
>> spin_unlock(&cpupool_lock);
>> }
>> +#endif
>>
>> static int cpu_callback(
>> struct notifier_block *nfb, unsigned long action, void *hcpu)
>> --- a/xen/common/schedule.c
>> +++ b/xen/common/schedule.c
>> @@ -1913,6 +1913,7 @@ void scheduler_free(struct scheduler *sched)
>> xfree(sched);
>> }
>>
>> +#ifdef CONFIG_HAS_KEYHANDLER
>> void schedule_dump(struct cpupool *c)
>> {
>> unsigned int i;
>> @@ -1941,6 +1942,7 @@ void schedule_dump(struct cpupool *c)
>> SCHED_OP(sched, dump_cpu_state, i);
>> }
>> }
>> +#endif
>>
>> void sched_tick_suspend(void)
>> {
> Mmm... a lot of #ifdef-fery spread around quite a bit.. I have to admit
> I don't especially like that.
Me too, can leave it as what is was.
but since schedule_dump prototype have external linkage.
so even no one will call it, it maybe still in output executable file,
right?
>> --- a/xen/include/xen/keyhandler.h
>> +++ b/xen/include/xen/keyhandler.h
>> @@ -48,4 +49,17 @@ void register_irq_keyhandler(unsigned char key,
>> /* Inject a keypress into the key-handling subsystem. */
>> extern void handle_keypress(unsigned char key, struct cpu_user_regs
>> *regs);
>>
>> +#else
>> +static inline void initialize_keytable(void) {}
>> +static inline void register_keyhandler(unsigned char key,
>> keyhandler_fn_t *fn,
>> + const char *desc, bool_t
>> diagnostic) {}
>> +static inline void register_irq_keyhandler(unsigned char key,
>> + irq_keyhandler_fn_t *fn,
>> + const char *desc,
>> + bool_t diagnostic) {}
>> +
>> +static inline void handle_keypress(unsigned char key,
>> + struct cpu_user_regs *regs) {}
>>
> So, with this last change, we have:
>
> static DECLARE_TASKLET(keypress_tasklet, keypress_action, 0);
>
> But since all keypress_action() does is calling handle_keypress(),
> which is becoming a nop... can't we kill the tasklet alltogether?
the whole keyhandler.c will not compiled when config disabled.
am i misunderstood something?
>
>> --- a/xen/include/xen/lib.h
>> +++ b/xen/include/xen/lib.h
>> @@ -171,8 +171,10 @@ extern unsigned int tainted;
>> extern char *print_tainted(char *str);
>> extern void add_taint(unsigned int taint);
>>
>> +#ifdef CONFIG_HAS_KEYHANDLER
>> struct cpu_user_regs;
>> void dump_execstate(struct cpu_user_regs *);
>> +#endif
>>
> Yes. Or, you provide an empty stub of dump_execstate(), if
> CONFIG_HAS_KEYHANDLER is not defined, which means we don't have to mess
> with #ifdef-s at the caller site(s).
>
> Of course, I'm not maintainer of this specific piece of code, but I'd
> prefer this stub-based approach to be used in general.... ... ...
Agree, can be fixed.
>
>> --- a/xen/include/xen/sched.h
>> +++ b/xen/include/xen/sched.h
>> @@ -994,8 +994,10 @@ int cpupool_add_domain(struct domain *d, int
>> poolid);
>> void cpupool_rm_domain(struct domain *d);
>> int cpupool_move_domain(struct domain *d, struct cpupool *c);
>> int cpupool_do_sysctl(struct xen_sysctl_cpupool_op *op);
>> +#ifdef CONFIG_HAS_KEYHANDLER
>> void schedule_dump(struct cpupool *c);
>> extern void dump_runq(unsigned char key);
>> +#endif
>>
>> void arch_do_physinfo(struct xen_sysctl_physinfo *pi);
>>
> ... ... ... Like, for instance, in here.
>
> But again, sine these changes are spread around many files, let's see
> what others prefer, and use the same strategy everywhere.
>
> Regards
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] xen: make keyhanler configurable
2019-05-31 10:53 ` Juergen Gross
2019-05-31 10:53 ` [Xen-devel] " Juergen Gross
@ 2019-06-03 5:10 ` chenbaodong
2019-06-03 5:10 ` [Xen-devel] " chenbaodong
1 sibling, 1 reply; 14+ messages in thread
From: chenbaodong @ 2019-06-03 5:10 UTC (permalink / raw)
To: Juergen Gross, xen-devel
Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan,
Julien Grall, Jan Beulich, Dario Faggioli, Roger Pau Monné
On 5/31/19 18:53, Juergen Gross wrote:
> On 31/05/2019 03:58, Baodong Chen wrote:
>> keyhandler mainly used for debug usage by developers which can dump
>> xen module(eg. timer, scheduler) status at runtime by input
>> character from console.
>>
>> Signed-off-by: Baodong Chen <chenbaodong@mxnavi.com>
>> ---
>> xen/arch/arm/gic.c | 2 ++
>> xen/arch/x86/apic.c | 2 ++
>> xen/common/Kconfig | 9 +++++++++
>> xen/common/Makefile | 2 +-
>> xen/common/cpupool.c | 2 ++
>> xen/common/schedule.c | 2 ++
>> xen/include/xen/keyhandler.h | 14 ++++++++++++++
>> xen/include/xen/lib.h | 2 ++
>> xen/include/xen/sched.h | 2 ++
>> 9 files changed, 36 insertions(+), 1 deletion(-)
>>
>> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
>> index 6cc7dec..fff88c5 100644
>> --- a/xen/arch/arm/gic.c
>> +++ b/xen/arch/arm/gic.c
>> @@ -361,7 +361,9 @@ static void do_sgi(struct cpu_user_regs *regs, enum gic_sgi sgi)
>> /* Nothing to do, will check for events on return path */
>> break;
>> case GIC_SGI_DUMP_STATE:
>> +#ifdef CONFIG_HAS_KEYHANDLER
>> dump_execstate(regs);
>> +#endif
>> break;
>> case GIC_SGI_CALL_FUNCTION:
>> smp_call_function_interrupt();
>> diff --git a/xen/arch/x86/apic.c b/xen/arch/x86/apic.c
>> index fafc0bd..e5f004a 100644
>> --- a/xen/arch/x86/apic.c
>> +++ b/xen/arch/x86/apic.c
>> @@ -1410,7 +1410,9 @@ void spurious_interrupt(struct cpu_user_regs *regs)
>> ack_APIC_irq();
>> if (this_cpu(state_dump_pending)) {
>> this_cpu(state_dump_pending) = false;
>> +#ifdef CONFIG_HAS_KEYHANDLER
>> dump_execstate(regs);
>> +#endif
>> return;
>> }
>> }
>> diff --git a/xen/common/Kconfig b/xen/common/Kconfig
>> index c838506..450541c 100644
>> --- a/xen/common/Kconfig
>> +++ b/xen/common/Kconfig
>> @@ -368,4 +368,13 @@ config DOM0_MEM
>>
>> Leave empty if you are not sure what to specify.
>>
>> +config HAS_KEYHANDLER
> AFAIK the HAS_* config options are meant to be selected by other options
> and not be user selectable.
>
> So I think you should drop the "HAS_" and maybe use the plural as Dario
> already suggested ("KEYHANDLERS").
Yes.
>
>> + bool "Enable/Disable keyhandler"
>> + default y
>> + ---help---
>> + Enable or disable keyhandler function.
>> + keyhandler mainly used for debug usage by developers which can dump
>> + xen module(eg. timer, scheduler) status at runtime by input character
>> + from console.
> I'd drop the "by developers". In case of customer problems with Xen
> hosts the output of keyhandlers is requested on a rather regular base.
Agree, can be fixed.
>
>> +
>> endmenu
>> diff --git a/xen/common/Makefile b/xen/common/Makefile
>> index bca48e6..c7bcd26 100644
>> --- a/xen/common/Makefile
>> +++ b/xen/common/Makefile
>> @@ -16,7 +16,7 @@ obj-y += guestcopy.o
>> obj-bin-y += gunzip.init.o
>> obj-y += irq.o
>> obj-y += kernel.o
>> -obj-y += keyhandler.o
>> +obj-$(CONFIG_HAS_KEYHANDLER) += keyhandler.o
>> obj-$(CONFIG_KEXEC) += kexec.o
>> obj-$(CONFIG_KEXEC) += kimage.o
>> obj-y += lib.o
>> diff --git a/xen/common/cpupool.c b/xen/common/cpupool.c
>> index 31ac323..721a729 100644
>> --- a/xen/common/cpupool.c
>> +++ b/xen/common/cpupool.c
>> @@ -699,6 +699,7 @@ int cpupool_do_sysctl(struct xen_sysctl_cpupool_op *op)
>> return ret;
>> }
>>
>> +#ifdef CONFIG_HAS_KEYHANDLER
>> void dump_runq(unsigned char key)
>> {
>> unsigned long flags;
>> @@ -730,6 +731,7 @@ void dump_runq(unsigned char key)
>> local_irq_restore(flags);
>> spin_unlock(&cpupool_lock);
>> }
>> +#endif
>>
>> static int cpu_callback(
>> struct notifier_block *nfb, unsigned long action, void *hcpu)
>> diff --git a/xen/common/schedule.c b/xen/common/schedule.c
>> index 66f1e26..617c444 100644
>> --- a/xen/common/schedule.c
>> +++ b/xen/common/schedule.c
>> @@ -1913,6 +1913,7 @@ void scheduler_free(struct scheduler *sched)
>> xfree(sched);
>> }
>>
>> +#ifdef CONFIG_HAS_KEYHANDLER
>> void schedule_dump(struct cpupool *c)
>> {
>> unsigned int i;
>> @@ -1941,6 +1942,7 @@ void schedule_dump(struct cpupool *c)
>> SCHED_OP(sched, dump_cpu_state, i);
>> }
>> }
>> +#endif
>>
>> void sched_tick_suspend(void)
>> {
>> diff --git a/xen/include/xen/keyhandler.h b/xen/include/xen/keyhandler.h
>> index 5131e86..1050b80 100644
>> --- a/xen/include/xen/keyhandler.h
>> +++ b/xen/include/xen/keyhandler.h
>> @@ -28,6 +28,7 @@ struct cpu_user_regs;
>> typedef void (irq_keyhandler_fn_t)(unsigned char key,
>> struct cpu_user_regs *regs);
>>
>> +#ifdef CONFIG_HAS_KEYHANDLER
>> /* Initialize keytable with default handlers. */
>> void initialize_keytable(void);
>>
>> @@ -48,4 +49,17 @@ void register_irq_keyhandler(unsigned char key,
>> /* Inject a keypress into the key-handling subsystem. */
>> extern void handle_keypress(unsigned char key, struct cpu_user_regs *regs);
>>
>> +#else
>> +static inline void initialize_keytable(void) {}
>> +static inline void register_keyhandler(unsigned char key, keyhandler_fn_t *fn,
>> + const char *desc, bool_t diagnostic) {}
>> +static inline void register_irq_keyhandler(unsigned char key,
>> + irq_keyhandler_fn_t *fn,
>> + const char *desc,
>> + bool_t diagnostic) {}
>> +
>> +static inline void handle_keypress(unsigned char key,
>> + struct cpu_user_regs *regs) {}
>> +#endif
>> +
>> #endif /* __XEN_KEYHANDLER_H__ */
>> diff --git a/xen/include/xen/lib.h b/xen/include/xen/lib.h
>> index e0b7bcb..8710305 100644
>> --- a/xen/include/xen/lib.h
>> +++ b/xen/include/xen/lib.h
>> @@ -171,8 +171,10 @@ extern unsigned int tainted;
>> extern char *print_tainted(char *str);
>> extern void add_taint(unsigned int taint);
>>
>> +#ifdef CONFIG_HAS_KEYHANDLER
>> struct cpu_user_regs;
>> void dump_execstate(struct cpu_user_regs *);
>> +#endif
>>
>> void init_constructors(void);
>>
>> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
>> index 748bb0f..b82cdee 100644
>> --- a/xen/include/xen/sched.h
>> +++ b/xen/include/xen/sched.h
>> @@ -994,8 +994,10 @@ int cpupool_add_domain(struct domain *d, int poolid);
>> void cpupool_rm_domain(struct domain *d);
>> int cpupool_move_domain(struct domain *d, struct cpupool *c);
>> int cpupool_do_sysctl(struct xen_sysctl_cpupool_op *op);
>> +#ifdef CONFIG_HAS_KEYHANDLER
>> void schedule_dump(struct cpupool *c);
>> extern void dump_runq(unsigned char key);
>> +#endif
>>
>> void arch_do_physinfo(struct xen_sysctl_physinfo *pi);
> Why stopping halfway here? There are lots of other keyhandlers which can
> be removed for the hypervisor in case there is no code calling them.
Not sure about 'halfway' this.
Most of the callers for key hander will register a handler function with
**static** prefix. so when config disabled, the static handler function
will not be in final executable file.
so there is no need to spread '#ifdef CONFIG_KEYHANDLERS' to many files.
right?
>
> Juergen
> .
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Xen-devel] [PATCH] xen: make keyhanler configurable
2019-06-03 5:10 ` chenbaodong
@ 2019-06-03 5:10 ` chenbaodong
0 siblings, 0 replies; 14+ messages in thread
From: chenbaodong @ 2019-06-03 5:10 UTC (permalink / raw)
To: Juergen Gross, xen-devel
Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan,
Julien Grall, Jan Beulich, Dario Faggioli, Roger Pau Monné
On 5/31/19 18:53, Juergen Gross wrote:
> On 31/05/2019 03:58, Baodong Chen wrote:
>> keyhandler mainly used for debug usage by developers which can dump
>> xen module(eg. timer, scheduler) status at runtime by input
>> character from console.
>>
>> Signed-off-by: Baodong Chen <chenbaodong@mxnavi.com>
>> ---
>> xen/arch/arm/gic.c | 2 ++
>> xen/arch/x86/apic.c | 2 ++
>> xen/common/Kconfig | 9 +++++++++
>> xen/common/Makefile | 2 +-
>> xen/common/cpupool.c | 2 ++
>> xen/common/schedule.c | 2 ++
>> xen/include/xen/keyhandler.h | 14 ++++++++++++++
>> xen/include/xen/lib.h | 2 ++
>> xen/include/xen/sched.h | 2 ++
>> 9 files changed, 36 insertions(+), 1 deletion(-)
>>
>> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
>> index 6cc7dec..fff88c5 100644
>> --- a/xen/arch/arm/gic.c
>> +++ b/xen/arch/arm/gic.c
>> @@ -361,7 +361,9 @@ static void do_sgi(struct cpu_user_regs *regs, enum gic_sgi sgi)
>> /* Nothing to do, will check for events on return path */
>> break;
>> case GIC_SGI_DUMP_STATE:
>> +#ifdef CONFIG_HAS_KEYHANDLER
>> dump_execstate(regs);
>> +#endif
>> break;
>> case GIC_SGI_CALL_FUNCTION:
>> smp_call_function_interrupt();
>> diff --git a/xen/arch/x86/apic.c b/xen/arch/x86/apic.c
>> index fafc0bd..e5f004a 100644
>> --- a/xen/arch/x86/apic.c
>> +++ b/xen/arch/x86/apic.c
>> @@ -1410,7 +1410,9 @@ void spurious_interrupt(struct cpu_user_regs *regs)
>> ack_APIC_irq();
>> if (this_cpu(state_dump_pending)) {
>> this_cpu(state_dump_pending) = false;
>> +#ifdef CONFIG_HAS_KEYHANDLER
>> dump_execstate(regs);
>> +#endif
>> return;
>> }
>> }
>> diff --git a/xen/common/Kconfig b/xen/common/Kconfig
>> index c838506..450541c 100644
>> --- a/xen/common/Kconfig
>> +++ b/xen/common/Kconfig
>> @@ -368,4 +368,13 @@ config DOM0_MEM
>>
>> Leave empty if you are not sure what to specify.
>>
>> +config HAS_KEYHANDLER
> AFAIK the HAS_* config options are meant to be selected by other options
> and not be user selectable.
>
> So I think you should drop the "HAS_" and maybe use the plural as Dario
> already suggested ("KEYHANDLERS").
Yes.
>
>> + bool "Enable/Disable keyhandler"
>> + default y
>> + ---help---
>> + Enable or disable keyhandler function.
>> + keyhandler mainly used for debug usage by developers which can dump
>> + xen module(eg. timer, scheduler) status at runtime by input character
>> + from console.
> I'd drop the "by developers". In case of customer problems with Xen
> hosts the output of keyhandlers is requested on a rather regular base.
Agree, can be fixed.
>
>> +
>> endmenu
>> diff --git a/xen/common/Makefile b/xen/common/Makefile
>> index bca48e6..c7bcd26 100644
>> --- a/xen/common/Makefile
>> +++ b/xen/common/Makefile
>> @@ -16,7 +16,7 @@ obj-y += guestcopy.o
>> obj-bin-y += gunzip.init.o
>> obj-y += irq.o
>> obj-y += kernel.o
>> -obj-y += keyhandler.o
>> +obj-$(CONFIG_HAS_KEYHANDLER) += keyhandler.o
>> obj-$(CONFIG_KEXEC) += kexec.o
>> obj-$(CONFIG_KEXEC) += kimage.o
>> obj-y += lib.o
>> diff --git a/xen/common/cpupool.c b/xen/common/cpupool.c
>> index 31ac323..721a729 100644
>> --- a/xen/common/cpupool.c
>> +++ b/xen/common/cpupool.c
>> @@ -699,6 +699,7 @@ int cpupool_do_sysctl(struct xen_sysctl_cpupool_op *op)
>> return ret;
>> }
>>
>> +#ifdef CONFIG_HAS_KEYHANDLER
>> void dump_runq(unsigned char key)
>> {
>> unsigned long flags;
>> @@ -730,6 +731,7 @@ void dump_runq(unsigned char key)
>> local_irq_restore(flags);
>> spin_unlock(&cpupool_lock);
>> }
>> +#endif
>>
>> static int cpu_callback(
>> struct notifier_block *nfb, unsigned long action, void *hcpu)
>> diff --git a/xen/common/schedule.c b/xen/common/schedule.c
>> index 66f1e26..617c444 100644
>> --- a/xen/common/schedule.c
>> +++ b/xen/common/schedule.c
>> @@ -1913,6 +1913,7 @@ void scheduler_free(struct scheduler *sched)
>> xfree(sched);
>> }
>>
>> +#ifdef CONFIG_HAS_KEYHANDLER
>> void schedule_dump(struct cpupool *c)
>> {
>> unsigned int i;
>> @@ -1941,6 +1942,7 @@ void schedule_dump(struct cpupool *c)
>> SCHED_OP(sched, dump_cpu_state, i);
>> }
>> }
>> +#endif
>>
>> void sched_tick_suspend(void)
>> {
>> diff --git a/xen/include/xen/keyhandler.h b/xen/include/xen/keyhandler.h
>> index 5131e86..1050b80 100644
>> --- a/xen/include/xen/keyhandler.h
>> +++ b/xen/include/xen/keyhandler.h
>> @@ -28,6 +28,7 @@ struct cpu_user_regs;
>> typedef void (irq_keyhandler_fn_t)(unsigned char key,
>> struct cpu_user_regs *regs);
>>
>> +#ifdef CONFIG_HAS_KEYHANDLER
>> /* Initialize keytable with default handlers. */
>> void initialize_keytable(void);
>>
>> @@ -48,4 +49,17 @@ void register_irq_keyhandler(unsigned char key,
>> /* Inject a keypress into the key-handling subsystem. */
>> extern void handle_keypress(unsigned char key, struct cpu_user_regs *regs);
>>
>> +#else
>> +static inline void initialize_keytable(void) {}
>> +static inline void register_keyhandler(unsigned char key, keyhandler_fn_t *fn,
>> + const char *desc, bool_t diagnostic) {}
>> +static inline void register_irq_keyhandler(unsigned char key,
>> + irq_keyhandler_fn_t *fn,
>> + const char *desc,
>> + bool_t diagnostic) {}
>> +
>> +static inline void handle_keypress(unsigned char key,
>> + struct cpu_user_regs *regs) {}
>> +#endif
>> +
>> #endif /* __XEN_KEYHANDLER_H__ */
>> diff --git a/xen/include/xen/lib.h b/xen/include/xen/lib.h
>> index e0b7bcb..8710305 100644
>> --- a/xen/include/xen/lib.h
>> +++ b/xen/include/xen/lib.h
>> @@ -171,8 +171,10 @@ extern unsigned int tainted;
>> extern char *print_tainted(char *str);
>> extern void add_taint(unsigned int taint);
>>
>> +#ifdef CONFIG_HAS_KEYHANDLER
>> struct cpu_user_regs;
>> void dump_execstate(struct cpu_user_regs *);
>> +#endif
>>
>> void init_constructors(void);
>>
>> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
>> index 748bb0f..b82cdee 100644
>> --- a/xen/include/xen/sched.h
>> +++ b/xen/include/xen/sched.h
>> @@ -994,8 +994,10 @@ int cpupool_add_domain(struct domain *d, int poolid);
>> void cpupool_rm_domain(struct domain *d);
>> int cpupool_move_domain(struct domain *d, struct cpupool *c);
>> int cpupool_do_sysctl(struct xen_sysctl_cpupool_op *op);
>> +#ifdef CONFIG_HAS_KEYHANDLER
>> void schedule_dump(struct cpupool *c);
>> extern void dump_runq(unsigned char key);
>> +#endif
>>
>> void arch_do_physinfo(struct xen_sysctl_physinfo *pi);
> Why stopping halfway here? There are lots of other keyhandlers which can
> be removed for the hypervisor in case there is no code calling them.
Not sure about 'halfway' this.
Most of the callers for key hander will register a handler function with
**static** prefix. so when config disabled, the static handler function
will not be in final executable file.
so there is no need to spread '#ifdef CONFIG_KEYHANDLERS' to many files.
right?
>
> Juergen
> .
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] xen: make keyhanler configurable
2019-05-31 22:30 ` Andrew Cooper
2019-05-31 22:30 ` [Xen-devel] " Andrew Cooper
@ 2019-06-03 5:35 ` chenbaodong
2019-06-03 5:35 ` [Xen-devel] " chenbaodong
1 sibling, 1 reply; 14+ messages in thread
From: chenbaodong @ 2019-06-03 5:35 UTC (permalink / raw)
To: Andrew Cooper, xen-devel
Cc: Juergen Gross, Stefano Stabellini, Wei Liu,
Konrad Rzeszutek Wilk, George Dunlap, Tim Deegan, Ian Jackson,
Dario Faggioli, Julien Grall, Jan Beulich, Roger Pau Monné
On 6/1/19 06:30, Andrew Cooper wrote:
> On 30/05/2019 18:58, Baodong Chen wrote:
>> keyhandler mainly used for debug usage by developers which can dump
>> xen module(eg. timer, scheduler) status at runtime by input
>> character from console.
>>
>> Signed-off-by: Baodong Chen <chenbaodong@mxnavi.com>
> What is the motivation here? I don't have a specific objection to
> making this configurable (so long as it excises the entire keyhandler
> infrastructure, which is rather more than this patch does), but I also
> don't see why we would want to do so in the first place.
>
> In particular, it doesn't require a serial console to function correctly
> in the first place. This functionality can be used with `xl debug-keys
> $char; xl dmesg`
IIUC the config cut nearly the entire keyhandler infrastructure.
I'm interested in arm64 automotive use case, safety certification
is nice to have.
so the smaller code base is preferred.
BTW, i heard the works "dom0less", is it possible run vms over xen
without xl?
> ~Andrew
> .
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Xen-devel] [PATCH] xen: make keyhanler configurable
2019-06-03 5:35 ` chenbaodong
@ 2019-06-03 5:35 ` chenbaodong
0 siblings, 0 replies; 14+ messages in thread
From: chenbaodong @ 2019-06-03 5:35 UTC (permalink / raw)
To: Andrew Cooper, xen-devel
Cc: Juergen Gross, Stefano Stabellini, Wei Liu,
Konrad Rzeszutek Wilk, George Dunlap, Tim Deegan, Ian Jackson,
Dario Faggioli, Julien Grall, Jan Beulich, Roger Pau Monné
On 6/1/19 06:30, Andrew Cooper wrote:
> On 30/05/2019 18:58, Baodong Chen wrote:
>> keyhandler mainly used for debug usage by developers which can dump
>> xen module(eg. timer, scheduler) status at runtime by input
>> character from console.
>>
>> Signed-off-by: Baodong Chen <chenbaodong@mxnavi.com>
> What is the motivation here? I don't have a specific objection to
> making this configurable (so long as it excises the entire keyhandler
> infrastructure, which is rather more than this patch does), but I also
> don't see why we would want to do so in the first place.
>
> In particular, it doesn't require a serial console to function correctly
> in the first place. This functionality can be used with `xl debug-keys
> $char; xl dmesg`
IIUC the config cut nearly the entire keyhandler infrastructure.
I'm interested in arm64 automotive use case, safety certification
is nice to have.
so the smaller code base is preferred.
BTW, i heard the works "dom0less", is it possible run vms over xen
without xl?
> ~Andrew
> .
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2019-06-03 5:36 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-31 1:58 [PATCH] xen: make keyhanler configurable Baodong Chen
2019-05-31 1:58 ` [Xen-devel] " Baodong Chen
2019-05-31 10:39 ` Dario Faggioli
2019-05-31 10:39 ` [Xen-devel] " Dario Faggioli
2019-06-03 5:02 ` chenbaodong
2019-06-03 5:02 ` [Xen-devel] " chenbaodong
2019-05-31 10:53 ` Juergen Gross
2019-05-31 10:53 ` [Xen-devel] " Juergen Gross
2019-06-03 5:10 ` chenbaodong
2019-06-03 5:10 ` [Xen-devel] " chenbaodong
2019-05-31 22:30 ` Andrew Cooper
2019-05-31 22:30 ` [Xen-devel] " Andrew Cooper
2019-06-03 5:35 ` chenbaodong
2019-06-03 5:35 ` [Xen-devel] " chenbaodong
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).