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(-)
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-develOn 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)
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
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
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
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
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
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
© 2016 - 2024 Red Hat, Inc.