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

[Dario Faggioli <dfaggioli@xxxxxxxx>]

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

> --- 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: signature.asc
Description: This is a digitally signed message part

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