Re: [Xen-devel] [PATCH] xen/keyhandler: Rework keyhandler infrastructure
On 22/09/15 11:35, Jan Beulich wrote:
--- 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?
I can certainly see about correcting some of the text. I suppose this
is a good time to make it all consistent.
+
+key_table[key] = k;
I don't really see the value of the intermediate variable k here.
Nor me - I thing it might have been a way of how the patch developed.
@@ -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?
I will audit all the text in v2.
~Andrew
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH] xen/keyhandler: Rework keyhandler infrastructure
>>> On 14.09.15 at 15:49, 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 de
[Xen-devel] [PATCH] xen/keyhandler: Rework keyhandler infrastructure
struct keyhandler does not contain much information, and requires a lot
of boilerplate to use. It is far more convenient to have
register_keyhandler() take each piece of information a parameter,
especially when introducing temporary debugging keyhandlers.
This in turn allows struct keyhandler itself to become private to
keyhandler.c and for the key_table to become more efficient.
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). The overall effect of this is the
key_table grows in size by 50%, but there are no longer 24-byte
keyhandler structures all over the data section.
All of the key_table entries in keyhandler.c can be initialised at
compile time rather than runtime.
Signed-off-by: Andrew Cooper
CC: Jan Beulich
CC: Keir Fraser
---
Note: I have avoided CC'ing all maintainers as this is largely a mechanical
change across the whole codebase. Most non-mechanical changes are in common
code, but there is a trivial externing change
xen/drivers/passthrough/vtd/extern.h as previously the struct keyhandler was
in a different translation unit to the function it referenced.
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.
---
xen/arch/x86/acpi/cpu_idle.c |8 +-
xen/arch/x86/hvm/irq.c |8 +-
xen/arch/x86/hvm/svm/vmcb.c |8 +-
xen/arch/x86/hvm/vmx/vmcs.c |8 +-
xen/arch/x86/io_apic.c |7 +-
xen/arch/x86/irq.c |8 +-
xen/arch/x86/mm/p2m-ept.c|8 +-
xen/arch/x86/mm/shadow/common.c | 14 +--
xen/arch/x86/msi.c |8 +-
xen/arch/x86/nmi.c | 15 +--
xen/arch/x86/numa.c |8 +-
xen/arch/x86/time.c |8 +-
xen/common/event_channel.c |8 +-
xen/common/grant_table.c |9 +-
xen/common/kexec.c |7 +-
xen/common/keyhandler.c | 199 --
xen/common/page_alloc.c | 16 +--
xen/common/timer.c |8 +-
xen/drivers/char/console.c | 16 +--
xen/drivers/passthrough/amd/iommu_intr.c |9 +-
xen/drivers/passthrough/iommu.c |8 +-
xen/drivers/passthrough/pci.c|8 +-
xen/drivers/passthrough/vtd/extern.h |2 +-
xen/drivers/passthrough/vtd/iommu.c |2 +-
xen/drivers/passthrough/vtd/utils.c |8 +-
xen/include/xen/keyhandler.h | 49 +++-
26 files changed, 124 insertions(+), 333 deletions(-)
diff --git a/xen/arch/x86/acpi/cpu_idle.c b/xen/arch/x86/acpi/cpu_idle.c
index 15fe2e9..d1f99a7 100644
--- a/xen/arch/x86/acpi/cpu_idle.c
+++ b/xen/arch/x86/acpi/cpu_idle.c
@@ -334,15 +334,9 @@ static void dump_cx(unsigned char key)
print_acpi_power(cpu, processor_powers[cpu]);
}
-static struct keyhandler dump_cx_keyhandler = {
-.diagnostic = 1,
-.u.fn = dump_cx,
-.desc = "dump ACPI Cx structures"
-};
-
static int __init cpu_idle_key_init(void)
{
-register_keyhandler('c', &dump_cx_keyhandler);
+register_keyhandler('c', dump_cx, "dump ACPI Cx structures", 1);
return 0;
}
__initcall(cpu_idle_key_init);
diff --git a/xen/arch/x86/hvm/irq.c b/xen/arch/x86/hvm/irq.c
index 50fcf73..990a2ca 100644
--- a/xen/arch/x86/hvm/irq.c
+++ b/xen/arch/x86/hvm/irq.c
@@ -527,15 +527,9 @@ static void dump_irq_info(unsigned char key)
rcu_read_unlock(&domlist_read_lock);
}
-static struct keyhandler dump_irq_info_keyhandler = {
-.diagnostic = 1,
-.u.fn = dump_irq_info,
-.desc = "dump HVM irq info"
-};
-
static int __init dump_irq_info_key_init(void)
{
-register_keyhandler('I', &dump_irq_info_keyhandler);
+register_keyhandler('I', dump_irq_info, "dump HVM irq info", 1);
return 0;
}
__initcall(dump_irq_info_key_init);
diff --git a/xen/arch/x86/hvm/svm/vmcb.c b/xen/arch/x86/hvm/svm/vmcb.c
index b5d7165..9ea014f 100644
--- a/xen/arch/x86/hvm/svm/vmcb.c
+++ b/xen/arch/x86/hvm/svm/vmcb.c
@@ -303,15 +303,9 @@ static void vmcb_dump(unsigned char ch)
printk("**\n");
}
-static struct keyhandler vmcb_dump_keyhandler = {
-.diagnostic = 1,
-.u.fn = vmcb_dump,
-.desc = "dump AMD-V VMCBs"
-};
-
void __init setup_vmcb_dump(void)
{
-register_keyhandler('v', &vmcb_dump_keyhandler);
+register_keyhandler('v', vmcb_dump, "dump AMD-V VMCBs", 1);
}
/*
diff --git a/xen/arch/x86/hv
