| Message ID | 1559267880-27863-1-git-send-email-chenbaodong@mxnavi.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen: make keyhanler configurable | expand |
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
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
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
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
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 > . >
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 > . >
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);
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(-)