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

[chenbaodong <chenbaodong@xxxxxxxxxx>]

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@xxxxxxxxxx>
> > ---
> >   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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel