Re: [Xen-devel] [PATCH] xen/keyhandler: Rework keyhandler infrastructure

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 14.09.15 at 15:49, <andrew.cooper3@xxxxxxxxxx> wrote:
> key_table doesn't need to contain 256 entries; all keys are ASCII
> which limits them to 7 bits of index, rather than 8.  It can also
> become a straight array, rather than an array of pointers.  struct
> keyhandler itself can become smaller simply via reordering (losing 6
> bytes of implicit padding).

I don't see you losing 6 bytes of padding, I only see you moving them
to the end of the structure.

> Further reduction in the size of the key_table could be achieved by
> dropping the first 0x20 entries corresponding to ASCII control
> characters, and by hiding the two boolean fields in the low-order bits
> of the function pointer, but both of these feel like too much effort
> for too little gain.

Besides the ugliness on the use site(s), using the low bits of the
function pointer won't work reliably on x86 due to there no being
and alignment guarantees. If anything you could use some of the
high bits, but I agree the benefit would not outweigh the hassle.

> --- a/xen/arch/x86/hvm/vmx/vmcs.c
> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
> @@ -1858,15 +1858,9 @@ static void vmcs_dump(unsigned char ch)
>      printk("**************************************\n");
>  }
>  
> -static struct keyhandler vmcs_dump_keyhandler = {
> -    .diagnostic = 1,
> -    .u.fn = vmcs_dump,
> -    .desc = "dump Intel's VMCS"
> -};
> -
>  void __init setup_vmcs_dump(void)
>  {
> -    register_keyhandler('v', &vmcs_dump_keyhandler);
> +    register_keyhandler('v', vmcs_dump, "dump Intel's VMCS", 1);
>  }

Mind switching this from "Intel" to VMX as you go, considering that
it's not just Intel using that structure?

> +static struct keyhandler {
> +    union {
> +        keyhandler_fn_t fn;
> +        irq_keyhandler_fn_t irq_fn;
> +    };
> +
> +    const char *desc;    /* Description for help message.                 */
> +    bool_t irq_callback, /* Call in irq context? if not, tasklet context. */
> +        diagnostic;      /* Include in 'dump all' handler.                */
> +} key_table[128] __read_mostly =
> +{
> +#define KEYHANDLER(_k, _fn, _desc, _diag)                   \
> +    [(_k)] = { .fn = (_fn), .desc = (_desc),                \
> +               .irq_callback = 0, .diagnostic = (_diag) }
> +#define IRQ_KEYHANDLER(_k, _fn, _desc, _diag)               \
> +    [(_k)] = { .irq_fn = (_fn), .desc = (_desc),            \
> +               .irq_callback = 1, .diagnostic = (_diag) }

May I ask to avoid redundant (even if of different kind) brackets?
I.e. [(_k)] can be just [_k] without causing any issues. And I'd also
like to see us move away from underscore prefixed macro parameter
names - underscore prefixed names have a different purpose in the
C standard.

> @@ -54,27 +106,32 @@ void handle_keypress(unsigned char key, struct 
> cpu_user_regs *regs)
>      }
>  }
>  
> -void register_keyhandler(unsigned char key, struct keyhandler *handler)
> +void register_keyhandler(unsigned char key, keyhandler_fn_t fn,
> +                         const char *desc, bool_t diagnostic)
>  {
> -    ASSERT(key_table[key] == NULL);
> -    key_table[key] = handler;
> +    struct keyhandler k = {
> +        .fn = fn,
> +        .desc = desc,
> +        .irq_callback = 0,
> +        .diagnostic = diagnostic,
> +    };
> +
> +    BUG_ON(key >= ARRAY_SIZE(key_table)); /* Key in range? */
> +    BUG_ON(key_table[key].fn != NULL);    /* Clobbering something else? */

Please keep this an ASSERT() (the first of the two being a BUG_ON()
is fine).

> +
> +    key_table[key] = k;

I don't really see the value of the intermediate variable k here.

> @@ -492,11 +488,11 @@ static void run_all_keyhandlers(unsigned char key, 
> struct cpu_user_regs *regs)
>      /* Fire all the IRQ-context diangostic keyhandlers now */
>      for ( k = 0; k < ARRAY_SIZE(key_table); k++ )
>      {
> -        h = key_table[k];
> -        if ( (h == NULL) || !h->diagnostic || !h->irq_callback )
> +        h = &key_table[k];
> +        if ( !h->fn || !h->diagnostic || !h->irq_callback )

Since you intend to use h->irq_fn below, please probe that one
instead of h->fn here.

> @@ -1901,15 +1895,9 @@ static void dump_heap(unsigned char key)
>      }
>  }
>  
> -static struct keyhandler dump_heap_keyhandler = {
> -    .diagnostic = 1,
> -    .u.fn = dump_heap,
> -    .desc = "dump heap info"
> -};
> -
>  static __init int register_heap_trigger(void)
>  {
> -    register_keyhandler('H', &dump_heap_keyhandler);
> +    register_keyhandler('H', dump_heap, "dump heap info", 1);

Considering the other one in this file is "memory info", just
"heap info" perhaps?

> --- a/xen/include/xen/keyhandler.h
> +++ b/xen/include/xen/keyhandler.h
> @@ -1,53 +1,32 @@
>  
> /******************************************************************************
>   * keyhandler.h
> - * 
> + *
>   * We keep an array of 'handlers' for each key code between 0 and 255;
> - * this is intended to allow very simple debugging routines (toggle 
> + * this is intended to allow very simple debugging routines (toggle
>   * debug flag, dump registers, reboot, etc) to be hooked in in a slightly
> - * nicer fashion than just editing the serial/keyboard drivers. 
> + * nicer fashion than just editing the serial/keyboard drivers.
>   */
>  
>  #ifndef __XEN_KEYHANDLER_H__
>  #define __XEN_KEYHANDLER_H__
>  
> -typedef void keyhandler_fn_t(
> -    unsigned char key);
> -typedef void irq_keyhandler_fn_t(
> -    unsigned char key, struct cpu_user_regs *regs);
> -
> -struct keyhandler {
> -    /*
> -     * If TRUE then u.irq_fn is called in hardirq context with interrupts
> -     * disabled. The @regs callback parameter points at the interrupted
> -     * register context. 
> -     * If FALSE then u.fn is called in softirq context with no locks held and
> -     * interrupts enabled.
> -     */
> -    bool_t irq_callback;
> -
> -    /*
> -     * If TRUE then the keyhandler will be included in the "dump everything"
> -     * keyhandler, so must not have any side-effects.
> -     */
> -    bool_t diagnostic;
> -
> -    union {
> -        keyhandler_fn_t *fn;
> -        irq_keyhandler_fn_t *irq_fn;
> -    } u;
> -
> -    /* The string is not copied by register_keyhandler(), so must persist. */
> -    char *desc;
> -};
> +#include <xen/types.h>
> +
> +typedef void (*keyhandler_fn_t)(unsigned char key);
> +typedef void (*irq_keyhandler_fn_t)(unsigned char key,
> +                                    struct cpu_user_regs *regs);

This type can now also be private to keyhandler.c.

I think I would also like it better if you retained the original
model of these being function types, not pointer-to-function
ones, so that consumers caring could use them to prototype
functions (and you could actually do so yourself for all the
static function declarations at the top of keyhandler.c and in
VT-d code).

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel