| Message ID | 1425615506-1829-3-git-send-email-david@gibson.dropbear.id.au |
|---|---|
| State | New |
| Headers | show |
David Gibson <david@gibson.dropbear.id.au> writes: > The hmp commands "irq" and "pic" are a bit of a mess. They're implemented > on a number of targets, but not all. On sparc32 and LM32 they do target > specific things, but on the remainder (i386, ppc and mips) they call into > the i8259 PIC code. Where "info pic" does absolutely nothing unless we're using "isa-i8259". In particular, it does nothing when we're using "kvm-i8259" instead. Fun! > But really, what these commands do shouldn't be dependent on the target > arch, but on the specific machine that's in use. On ppc, for example, > the "prep" machine usually does have an ISA bridge with an i8259, but > most of the other machine types have never had an i8259 at all. Similarly > the sparc specific target would stop working if we ever had a sparc32 > machine that wasn't sun4m. > > This patch cleans things up by implementing these hmp commands on all > targets via a MachineClass callback. If the callback is NULL, for now > we fallback to target specific defaults that match the existing behaviour. > The hope is we can remove those later with target specific cleanups. > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > --- > hw/intc/i8259.c | 4 ++-- > include/hw/boards.h | 2 ++ > include/hw/i386/pc.h | 4 ++-- > monitor.c | 57 ++++++++++++++++++++++++++++++++++++++-------------- > 4 files changed, 48 insertions(+), 19 deletions(-) > > diff --git a/hw/intc/i8259.c b/hw/intc/i8259.c > index 0f5c025..43e90b9 100644 > --- a/hw/intc/i8259.c > +++ b/hw/intc/i8259.c > @@ -429,7 +429,7 @@ static void pic_realize(DeviceState *dev, Error **errp) > pc->parent_realize(dev, errp); > } > > -void hmp_info_pic(Monitor *mon, const QDict *qdict) > +void i8259_hmp_info_pic(Monitor *mon, const QDict *qdict) > { > int i; > PICCommonState *s; > @@ -447,7 +447,7 @@ void hmp_info_pic(Monitor *mon, const QDict *qdict) > } > } > > -void hmp_info_irq(Monitor *mon, const QDict *qdict) > +void i8259_hmp_info_irq(Monitor *mon, const QDict *qdict) > { > #ifndef DEBUG_IRQ_COUNT > monitor_printf(mon, "irq statistic code not compiled.\n"); > diff --git a/include/hw/boards.h b/include/hw/boards.h > index 3ddc449..214a778 100644 > --- a/include/hw/boards.h > +++ b/include/hw/boards.h > @@ -111,6 +111,8 @@ struct MachineClass { > > HotplugHandler *(*get_hotplug_handler)(MachineState *machine, > DeviceState *dev); > + void (*hmp_info_irq)(Monitor *mon, const QDict *qdict); > + void (*hmp_info_pic)(Monitor *mon, const QDict *qdict); > }; > > /** Here you're defining the MachineClass callback. The callback is designed for HMP. This will make code implementing it depend on the monitor. You could do a QMP-style callback instead, and get a dependency on QAPI or QObject instead. But I'm fine with it as is. > diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h > index 08ab67d..0f376c6 100644 > --- a/include/hw/i386/pc.h > +++ b/include/hw/i386/pc.h > @@ -121,8 +121,8 @@ qemu_irq *i8259_init(ISABus *bus, qemu_irq parent_irq); > qemu_irq *kvm_i8259_init(ISABus *bus); > int pic_read_irq(DeviceState *d); > int pic_get_output(DeviceState *d); > -void hmp_info_pic(Monitor *mon, const QDict *qdict); > -void hmp_info_irq(Monitor *mon, const QDict *qdict); > +void i8259_hmp_info_pic(Monitor *mon, const QDict *qdict); > +void i8259_hmp_info_irq(Monitor *mon, const QDict *qdict); > > /* Global System Interrupts */ > > diff --git a/monitor.c b/monitor.c > index c86a89e..ca226a9 100644 > --- a/monitor.c > +++ b/monitor.c > @@ -1064,6 +1064,48 @@ static void hmp_info_history(Monitor *mon, const QDict *qdict) > } > } > > +static void hmp_info_pic(Monitor *mon, const QDict *qdict) > +{ > + MachineClass *mc = MACHINE_GET_CLASS(current_machine); > + > + if (mc->hmp_info_pic) { > + (mc->hmp_info_pic)(mon, qdict); Here you're using the MachineClass callback. However, you're not setting it anywhere, so the callback is dead code. Interfacing to the machine-specific parts with a MachineClass callback sounds sensible enough, but not without at least one user. Please either add one, or drop the dead code for now. > + } else { > + /* FIXME: Backwards compat fallbacks. These can go away once > + * we've finished converting to natively using MachineClass, > + * rather thatn QEMUMachine */ > +#if defined(TARGET_SPARC) && !defined(TARGET_SPARC64) > + sun4m_hmp_info_pic(mon, qdict); > +#elif defined(TARGET_LM32) > + lm32_hmp_info_pic(mon, qdict); > +#elif defined(TARGET_i386) || defined(TARGET_PPC) || defined(TARGET_MIPS) > + i8259_hmp_info_pic(mon, qdict); > +#endif > + } > +} > + > +static void hmp_info_irq(Monitor *mon, const QDict *qdict) > +{ > + /* FIXME: The ifdefs can go away once the sun4m and LM32 machines > + * are converted to use machine classes natively */ > + MachineClass *mc = MACHINE_GET_CLASS(current_machine); > + > + if (mc->hmp_info_irq) { > + (mc->hmp_info_irq)(mon, qdict); > + } else { > + /* FIXME: Backwards compat fallbacks. These can go away once > + * we've finished converting to natively using MachineClass, > + * rather thatn QEMUMachine */ > +#if defined(TARGET_SPARC) && !defined(TARGET_SPARC64) > + sun4m_hmp_info_irq(mon, qdict); > +#elif defined(TARGET_LM32) > + lm32_hmp_info_irq(mon, qdict); > +#elif defined(TARGET_i386) || defined(TARGET_PPC) || defined(TARGET_MIPS) > + i8259_hmp_info_irq(mon, qdict); > +#endif > + } > +} > + > static void hmp_info_cpustats(Monitor *mon, const QDict *qdict) > { > CPUState *cpu; > @@ -2661,35 +2703,20 @@ static mon_cmd_t info_cmds[] = { > .help = "show the command line history", > .mhandler.cmd = hmp_info_history, > }, > -#if defined(TARGET_I386) || defined(TARGET_PPC) || defined(TARGET_MIPS) || \ > - defined(TARGET_LM32) || (defined(TARGET_SPARC) && !defined(TARGET_SPARC64)) > { > .name = "irq", > .args_type = "", > .params = "", > .help = "show the interrupts statistics (if available)", > -#ifdef TARGET_SPARC > - .mhandler.cmd = sun4m_hmp_info_irq, > -#elif defined(TARGET_LM32) > - .mhandler.cmd = lm32_hmp_info_irq, > -#else > .mhandler.cmd = hmp_info_irq, > -#endif > }, > { > .name = "pic", > .args_type = "", > .params = "", > .help = "show i8259 (PIC) state", > -#ifdef TARGET_SPARC > - .mhandler.cmd = sun4m_hmp_info_pic, > -#elif defined(TARGET_LM32) > - .mhandler.cmd = lm32_hmp_info_pic, > -#else > .mhandler.cmd = hmp_info_pic, > -#endif > }, > -#endif > { > .name = "pci", > .args_type = "",
Markus Armbruster <armbru@redhat.com> writes: > David Gibson <david@gibson.dropbear.id.au> writes: > >> The hmp commands "irq" and "pic" are a bit of a mess. They're implemented >> on a number of targets, but not all. On sparc32 and LM32 they do target >> specific things, but on the remainder (i386, ppc and mips) they call into >> the i8259 PIC code. > > Where "info pic" does absolutely nothing unless we're using "isa-i8259". > In particular, it does nothing when we're using "kvm-i8259" instead. > Fun! > >> But really, what these commands do shouldn't be dependent on the target >> arch, but on the specific machine that's in use. On ppc, for example, >> the "prep" machine usually does have an ISA bridge with an i8259, but >> most of the other machine types have never had an i8259 at all. Similarly >> the sparc specific target would stop working if we ever had a sparc32 >> machine that wasn't sun4m. >> >> This patch cleans things up by implementing these hmp commands on all >> targets via a MachineClass callback. If the callback is NULL, for now >> we fallback to target specific defaults that match the existing behaviour. >> The hope is we can remove those later with target specific cleanups. >> >> Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > >> --- >> hw/intc/i8259.c | 4 ++-- >> include/hw/boards.h | 2 ++ >> include/hw/i386/pc.h | 4 ++-- >> monitor.c | 57 ++++++++++++++++++++++++++++++++++++++-------------- >> 4 files changed, 48 insertions(+), 19 deletions(-) >> >> diff --git a/hw/intc/i8259.c b/hw/intc/i8259.c >> index 0f5c025..43e90b9 100644 >> --- a/hw/intc/i8259.c >> +++ b/hw/intc/i8259.c >> @@ -429,7 +429,7 @@ static void pic_realize(DeviceState *dev, Error **errp) >> pc->parent_realize(dev, errp); >> } >> >> -void hmp_info_pic(Monitor *mon, const QDict *qdict) >> +void i8259_hmp_info_pic(Monitor *mon, const QDict *qdict) >> { >> int i; >> PICCommonState *s; >> @@ -447,7 +447,7 @@ void hmp_info_pic(Monitor *mon, const QDict *qdict) >> } >> } >> >> -void hmp_info_irq(Monitor *mon, const QDict *qdict) >> +void i8259_hmp_info_irq(Monitor *mon, const QDict *qdict) >> { >> #ifndef DEBUG_IRQ_COUNT >> monitor_printf(mon, "irq statistic code not compiled.\n"); >> diff --git a/include/hw/boards.h b/include/hw/boards.h >> index 3ddc449..214a778 100644 >> --- a/include/hw/boards.h >> +++ b/include/hw/boards.h >> @@ -111,6 +111,8 @@ struct MachineClass { >> >> HotplugHandler *(*get_hotplug_handler)(MachineState *machine, >> DeviceState *dev); >> + void (*hmp_info_irq)(Monitor *mon, const QDict *qdict); >> + void (*hmp_info_pic)(Monitor *mon, const QDict *qdict); >> }; >> >> /** > > Here you're defining the MachineClass callback. > > The callback is designed for HMP. This will make code implementing it > depend on the monitor. You could do a QMP-style callback instead, and > get a dependency on QAPI or QObject instead. > > But I'm fine with it as is. > >> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h >> index 08ab67d..0f376c6 100644 >> --- a/include/hw/i386/pc.h >> +++ b/include/hw/i386/pc.h >> @@ -121,8 +121,8 @@ qemu_irq *i8259_init(ISABus *bus, qemu_irq parent_irq); >> qemu_irq *kvm_i8259_init(ISABus *bus); >> int pic_read_irq(DeviceState *d); >> int pic_get_output(DeviceState *d); >> -void hmp_info_pic(Monitor *mon, const QDict *qdict); >> -void hmp_info_irq(Monitor *mon, const QDict *qdict); >> +void i8259_hmp_info_pic(Monitor *mon, const QDict *qdict); >> +void i8259_hmp_info_irq(Monitor *mon, const QDict *qdict); >> >> /* Global System Interrupts */ >> >> diff --git a/monitor.c b/monitor.c >> index c86a89e..ca226a9 100644 >> --- a/monitor.c >> +++ b/monitor.c >> @@ -1064,6 +1064,48 @@ static void hmp_info_history(Monitor *mon, const QDict *qdict) >> } >> } >> >> +static void hmp_info_pic(Monitor *mon, const QDict *qdict) >> +{ >> + MachineClass *mc = MACHINE_GET_CLASS(current_machine); >> + >> + if (mc->hmp_info_pic) { >> + (mc->hmp_info_pic)(mon, qdict); > > Here you're using the MachineClass callback. > > However, you're not setting it anywhere, so the callback is dead code. > > Interfacing to the machine-specific parts with a MachineClass callback > sounds sensible enough, but not without at least one user. Please > either add one, or drop the dead code for now. Scratch that, you're adding one in the next patch. Suggest to point to it in the commit message. Remember, reviewers effectively squint at your patches through a toilet roll, and can really use help with the non-local stuff, especially connections between patches. >> + } else { >> + /* FIXME: Backwards compat fallbacks. These can go away once >> + * we've finished converting to natively using MachineClass, >> + * rather thatn QEMUMachine */ >> +#if defined(TARGET_SPARC) && !defined(TARGET_SPARC64) >> + sun4m_hmp_info_pic(mon, qdict); >> +#elif defined(TARGET_LM32) >> + lm32_hmp_info_pic(mon, qdict); >> +#elif defined(TARGET_i386) || defined(TARGET_PPC) || defined(TARGET_MIPS) >> + i8259_hmp_info_pic(mon, qdict); >> +#endif >> + } >> +} >> + >> +static void hmp_info_irq(Monitor *mon, const QDict *qdict) >> +{ >> + /* FIXME: The ifdefs can go away once the sun4m and LM32 machines >> + * are converted to use machine classes natively */ >> + MachineClass *mc = MACHINE_GET_CLASS(current_machine); >> + >> + if (mc->hmp_info_irq) { >> + (mc->hmp_info_irq)(mon, qdict); >> + } else { >> + /* FIXME: Backwards compat fallbacks. These can go away once >> + * we've finished converting to natively using MachineClass, >> + * rather thatn QEMUMachine */ >> +#if defined(TARGET_SPARC) && !defined(TARGET_SPARC64) >> + sun4m_hmp_info_irq(mon, qdict); >> +#elif defined(TARGET_LM32) >> + lm32_hmp_info_irq(mon, qdict); >> +#elif defined(TARGET_i386) || defined(TARGET_PPC) || defined(TARGET_MIPS) >> + i8259_hmp_info_irq(mon, qdict); >> +#endif >> + } >> +} >> + >> static void hmp_info_cpustats(Monitor *mon, const QDict *qdict) >> { >> CPUState *cpu; >> @@ -2661,35 +2703,20 @@ static mon_cmd_t info_cmds[] = { >> .help = "show the command line history", >> .mhandler.cmd = hmp_info_history, >> }, >> -#if defined(TARGET_I386) || defined(TARGET_PPC) || defined(TARGET_MIPS) || \ >> - defined(TARGET_LM32) || (defined(TARGET_SPARC) && !defined(TARGET_SPARC64)) This adds "info irq" and "info pic" to the targets that didn't have them before, implemented by i8259.c's hmp_info_pic() and hmp_info_irq(). They do nothing unless the machine has an "isa-i8259" device. Cases: 1. If the machine has one, and it's the only interrupt controller, the commands work as advertized. 2. If the machine doesn't have one, the commands are empty promises. 3. If the machine has one, but it's not the only interrupt controller, the commands confidently claim the i8259 is all there is. Misinformation. Cases 2 and 3 are common, case 1 is rare. We can: A. Fix the commands to cover all interrupt controllers. B. Fix them to warn the user about missing interrupt controllers. We can approximate this by warning always, because it's almost never the only interrupt controller anyway :) C. Rip 'em both out and be done with it. D. Do nothing. E. Provide them as is on all targets. Spread the badness fairly. I vote for C or B. A seems not worthwhile. What does your series do? Remember, I'm squinting through a toilet roll... >> { >> .name = "irq", >> .args_type = "", >> .params = "", >> .help = "show the interrupts statistics (if available)", >> -#ifdef TARGET_SPARC >> - .mhandler.cmd = sun4m_hmp_info_irq, >> -#elif defined(TARGET_LM32) >> - .mhandler.cmd = lm32_hmp_info_irq, >> -#else >> .mhandler.cmd = hmp_info_irq, >> -#endif >> }, >> { >> .name = "pic", >> .args_type = "", >> .params = "", >> .help = "show i8259 (PIC) state", >> -#ifdef TARGET_SPARC >> - .mhandler.cmd = sun4m_hmp_info_pic, >> -#elif defined(TARGET_LM32) >> - .mhandler.cmd = lm32_hmp_info_pic, >> -#else >> .mhandler.cmd = hmp_info_pic, >> -#endif >> }, >> -#endif >> { >> .name = "pci", >> .args_type = "",
Am 06.03.2015 um 05:18 schrieb David Gibson: > The hmp commands "irq" and "pic" are a bit of a mess. They're implemented > on a number of targets, but not all. On sparc32 and LM32 they do target > specific things, but on the remainder (i386, ppc and mips) they call into > the i8259 PIC code. > > But really, what these commands do shouldn't be dependent on the target > arch, but on the specific machine that's in use. On ppc, for example, > the "prep" machine usually does have an ISA bridge with an i8259, but > most of the other machine types have never had an i8259 at all. Similarly > the sparc specific target would stop working if we ever had a sparc32 > machine that wasn't sun4m. > > This patch cleans things up by implementing these hmp commands on all > targets via a MachineClass callback. If the callback is NULL, for now > we fallback to target specific defaults that match the existing behaviour. > The hope is we can remove those later with target specific cleanups. > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > --- > hw/intc/i8259.c | 4 ++-- > include/hw/boards.h | 2 ++ > include/hw/i386/pc.h | 4 ++-- > monitor.c | 57 ++++++++++++++++++++++++++++++++++++++-------------- > 4 files changed, 48 insertions(+), 19 deletions(-) This commit message is terribly misleading: Nothing is done wrt CONFIG_I8259 AFAICT. What about "monitor: Implement info irq and info pit for all targets"? The implementation looks fine and once fixed can get a Reviewed-by: Andreas Färber <afaerber@suse.de> Regards, Andreas
On Mon, Mar 30, 2015 at 10:37:45AM +0200, Markus Armbruster wrote: > Markus Armbruster <armbru@redhat.com> writes: > > > David Gibson <david@gibson.dropbear.id.au> writes: > > > >> The hmp commands "irq" and "pic" are a bit of a mess. They're implemented > >> on a number of targets, but not all. On sparc32 and LM32 they do target > >> specific things, but on the remainder (i386, ppc and mips) they call into > >> the i8259 PIC code. > > > > Where "info pic" does absolutely nothing unless we're using "isa-i8259". > > In particular, it does nothing when we're using "kvm-i8259" instead. > > Fun! > > > >> But really, what these commands do shouldn't be dependent on the target > >> arch, but on the specific machine that's in use. On ppc, for example, > >> the "prep" machine usually does have an ISA bridge with an i8259, but > >> most of the other machine types have never had an i8259 at all. Similarly > >> the sparc specific target would stop working if we ever had a sparc32 > >> machine that wasn't sun4m. > >> > >> This patch cleans things up by implementing these hmp commands on all > >> targets via a MachineClass callback. If the callback is NULL, for now > >> we fallback to target specific defaults that match the existing behaviour. > >> The hope is we can remove those later with target specific cleanups. > >> > >> Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > > > >> --- > >> hw/intc/i8259.c | 4 ++-- > >> include/hw/boards.h | 2 ++ > >> include/hw/i386/pc.h | 4 ++-- > >> monitor.c | 57 ++++++++++++++++++++++++++++++++++++++-------------- > >> 4 files changed, 48 insertions(+), 19 deletions(-) > >> > >> diff --git a/hw/intc/i8259.c b/hw/intc/i8259.c > >> index 0f5c025..43e90b9 100644 > >> --- a/hw/intc/i8259.c > >> +++ b/hw/intc/i8259.c > >> @@ -429,7 +429,7 @@ static void pic_realize(DeviceState *dev, Error **errp) > >> pc->parent_realize(dev, errp); > >> } > >> > >> -void hmp_info_pic(Monitor *mon, const QDict *qdict) > >> +void i8259_hmp_info_pic(Monitor *mon, const QDict *qdict) > >> { > >> int i; > >> PICCommonState *s; > >> @@ -447,7 +447,7 @@ void hmp_info_pic(Monitor *mon, const QDict *qdict) > >> } > >> } > >> > >> -void hmp_info_irq(Monitor *mon, const QDict *qdict) > >> +void i8259_hmp_info_irq(Monitor *mon, const QDict *qdict) > >> { > >> #ifndef DEBUG_IRQ_COUNT > >> monitor_printf(mon, "irq statistic code not compiled.\n"); > >> diff --git a/include/hw/boards.h b/include/hw/boards.h > >> index 3ddc449..214a778 100644 > >> --- a/include/hw/boards.h > >> +++ b/include/hw/boards.h > >> @@ -111,6 +111,8 @@ struct MachineClass { > >> > >> HotplugHandler *(*get_hotplug_handler)(MachineState *machine, > >> DeviceState *dev); > >> + void (*hmp_info_irq)(Monitor *mon, const QDict *qdict); > >> + void (*hmp_info_pic)(Monitor *mon, const QDict *qdict); > >> }; > >> > >> /** > > > > Here you're defining the MachineClass callback. > > > > The callback is designed for HMP. This will make code implementing it > > depend on the monitor. You could do a QMP-style callback instead, and > > get a dependency on QAPI or QObject instead. > > > > But I'm fine with it as is. > > > >> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h > >> index 08ab67d..0f376c6 100644 > >> --- a/include/hw/i386/pc.h > >> +++ b/include/hw/i386/pc.h > >> @@ -121,8 +121,8 @@ qemu_irq *i8259_init(ISABus *bus, qemu_irq parent_irq); > >> qemu_irq *kvm_i8259_init(ISABus *bus); > >> int pic_read_irq(DeviceState *d); > >> int pic_get_output(DeviceState *d); > >> -void hmp_info_pic(Monitor *mon, const QDict *qdict); > >> -void hmp_info_irq(Monitor *mon, const QDict *qdict); > >> +void i8259_hmp_info_pic(Monitor *mon, const QDict *qdict); > >> +void i8259_hmp_info_irq(Monitor *mon, const QDict *qdict); > >> > >> /* Global System Interrupts */ > >> > >> diff --git a/monitor.c b/monitor.c > >> index c86a89e..ca226a9 100644 > >> --- a/monitor.c > >> +++ b/monitor.c > >> @@ -1064,6 +1064,48 @@ static void hmp_info_history(Monitor *mon, const QDict *qdict) > >> } > >> } > >> > >> +static void hmp_info_pic(Monitor *mon, const QDict *qdict) > >> +{ > >> + MachineClass *mc = MACHINE_GET_CLASS(current_machine); > >> + > >> + if (mc->hmp_info_pic) { > >> + (mc->hmp_info_pic)(mon, qdict); > > > > Here you're using the MachineClass callback. > > > > However, you're not setting it anywhere, so the callback is dead code. > > > > Interfacing to the machine-specific parts with a MachineClass callback > > sounds sensible enough, but not without at least one user. Please > > either add one, or drop the dead code for now. > > Scratch that, you're adding one in the next patch. > > Suggest to point to it in the commit message. Remember, reviewers > effectively squint at your patches through a toilet roll, and can really > use help with the non-local stuff, especially connections between > patches. > > >> + } else { > >> + /* FIXME: Backwards compat fallbacks. These can go away once > >> + * we've finished converting to natively using MachineClass, > >> + * rather thatn QEMUMachine */ > >> +#if defined(TARGET_SPARC) && !defined(TARGET_SPARC64) > >> + sun4m_hmp_info_pic(mon, qdict); > >> +#elif defined(TARGET_LM32) > >> + lm32_hmp_info_pic(mon, qdict); > >> +#elif defined(TARGET_i386) || defined(TARGET_PPC) || defined(TARGET_MIPS) > >> + i8259_hmp_info_pic(mon, qdict); > >> +#endif > >> + } > >> +} > >> + > >> +static void hmp_info_irq(Monitor *mon, const QDict *qdict) > >> +{ > >> + /* FIXME: The ifdefs can go away once the sun4m and LM32 machines > >> + * are converted to use machine classes natively */ > >> + MachineClass *mc = MACHINE_GET_CLASS(current_machine); > >> + > >> + if (mc->hmp_info_irq) { > >> + (mc->hmp_info_irq)(mon, qdict); > >> + } else { > >> + /* FIXME: Backwards compat fallbacks. These can go away once > >> + * we've finished converting to natively using MachineClass, > >> + * rather thatn QEMUMachine */ > >> +#if defined(TARGET_SPARC) && !defined(TARGET_SPARC64) > >> + sun4m_hmp_info_irq(mon, qdict); > >> +#elif defined(TARGET_LM32) > >> + lm32_hmp_info_irq(mon, qdict); > >> +#elif defined(TARGET_i386) || defined(TARGET_PPC) || defined(TARGET_MIPS) > >> + i8259_hmp_info_irq(mon, qdict); > >> +#endif > >> + } > >> +} > >> + > >> static void hmp_info_cpustats(Monitor *mon, const QDict *qdict) > >> { > >> CPUState *cpu; > >> @@ -2661,35 +2703,20 @@ static mon_cmd_t info_cmds[] = { > >> .help = "show the command line history", > >> .mhandler.cmd = hmp_info_history, > >> }, > >> -#if defined(TARGET_I386) || defined(TARGET_PPC) || defined(TARGET_MIPS) || \ > >> - defined(TARGET_LM32) || (defined(TARGET_SPARC) && !defined(TARGET_SPARC64)) > > This adds "info irq" and "info pic" to the targets that didn't have them > before, implemented by i8259.c's hmp_info_pic() and hmp_info_irq(). > They do nothing unless the machine has an "isa-i8259" device. > > Cases: > > 1. If the machine has one, and it's the only interrupt controller, the > commands work as advertized. > > 2. If the machine doesn't have one, the commands are empty promises. > > 3. If the machine has one, but it's not the only interrupt controller, > the commands confidently claim the i8259 is all there is. > Misinformation. > > Cases 2 and 3 are common, case 1 is rare. > > We can: > > A. Fix the commands to cover all interrupt controllers. > > B. Fix them to warn the user about missing interrupt controllers. > > We can approximate this by warning always, because it's almost never > the only interrupt controller anyway :) > > C. Rip 'em both out and be done with it. > > D. Do nothing. > > E. Provide them as is on all targets. > > Spread the badness fairly. > > I vote for C or B. A seems not worthwhile. I'd love to do C, if we can get confirmation that no-one's really using the existing HMP commands. That would make a bunch of things simpler. > What does your series do? Remember, I'm squinting through a toilet > roll... The current draft aims to do 2 things: * Keep identical behaviour on machines that currently have a non-trivial implementation of "info pic" and "info irq" * For all other machines, implement the commands as no-op. I guess that's closest to your option (E).
David Gibson <david@gibson.dropbear.id.au> writes: > On Mon, Mar 30, 2015 at 10:37:45AM +0200, Markus Armbruster wrote: [...] >> This adds "info irq" and "info pic" to the targets that didn't have them >> before, implemented by i8259.c's hmp_info_pic() and hmp_info_irq(). >> They do nothing unless the machine has an "isa-i8259" device. >> >> Cases: >> >> 1. If the machine has one, and it's the only interrupt controller, the >> commands work as advertized. >> >> 2. If the machine doesn't have one, the commands are empty promises. >> >> 3. If the machine has one, but it's not the only interrupt controller, >> the commands confidently claim the i8259 is all there is. >> Misinformation. >> >> Cases 2 and 3 are common, case 1 is rare. >> >> We can: >> >> A. Fix the commands to cover all interrupt controllers. >> >> B. Fix them to warn the user about missing interrupt controllers. >> >> We can approximate this by warning always, because it's almost never >> the only interrupt controller anyway :) >> >> C. Rip 'em both out and be done with it. >> >> D. Do nothing. >> >> E. Provide them as is on all targets. >> >> Spread the badness fairly. >> >> I vote for C or B. A seems not worthwhile. > > I'd love to do C, if we can get confirmation that no-one's really > using the existing HMP commands. That would make a bunch of things > simpler. I gave it a shot, let's see how people react. [...]
On 6 March 2015 at 04:18, David Gibson <david@gibson.dropbear.id.au> wrote: > The hmp commands "irq" and "pic" are a bit of a mess. They're implemented > on a number of targets, but not all. On sparc32 and LM32 they do target > specific things, but on the remainder (i386, ppc and mips) they call into > the i8259 PIC code. > > But really, what these commands do shouldn't be dependent on the target > arch, but on the specific machine that's in use. On ppc, for example, > the "prep" machine usually does have an ISA bridge with an i8259, but > most of the other machine types have never had an i8259 at all. Similarly > the sparc specific target would stop working if we ever had a sparc32 > machine that wasn't sun4m. > > This patch cleans things up by implementing these hmp commands on all > targets via a MachineClass callback. If the callback is NULL, for now > we fallback to target specific defaults that match the existing behaviour. > The hope is we can remove those later with target specific cleanups. I would rather we provided a function for "register a monitor command" that devices could call. Then we could (for instance) move the CPU architecture specific monitor commands out to target-* where they belong, and generally chop down the amount of ifdeffery in monitor.c. A per-machine hook seems insufficiently flexible. -- PMM
On Tue, Mar 31, 2015 at 11:57:28AM +0200, Markus Armbruster wrote: > David Gibson <david@gibson.dropbear.id.au> writes: > > > On Mon, Mar 30, 2015 at 10:37:45AM +0200, Markus Armbruster wrote: > [...] > >> This adds "info irq" and "info pic" to the targets that didn't have them > >> before, implemented by i8259.c's hmp_info_pic() and hmp_info_irq(). > >> They do nothing unless the machine has an "isa-i8259" device. > >> > >> Cases: > >> > >> 1. If the machine has one, and it's the only interrupt controller, the > >> commands work as advertized. > >> > >> 2. If the machine doesn't have one, the commands are empty promises. > >> > >> 3. If the machine has one, but it's not the only interrupt controller, > >> the commands confidently claim the i8259 is all there is. > >> Misinformation. > >> > >> Cases 2 and 3 are common, case 1 is rare. > >> > >> We can: > >> > >> A. Fix the commands to cover all interrupt controllers. > >> > >> B. Fix them to warn the user about missing interrupt controllers. > >> > >> We can approximate this by warning always, because it's almost never > >> the only interrupt controller anyway :) > >> > >> C. Rip 'em both out and be done with it. > >> > >> D. Do nothing. > >> > >> E. Provide them as is on all targets. > >> > >> Spread the badness fairly. > >> > >> I vote for C or B. A seems not worthwhile. > > > > I'd love to do C, if we can get confirmation that no-one's really > > using the existing HMP commands. That would make a bunch of things > > simpler. > > I gave it a shot, let's see how people react. Heh, you beat me to it. Thanks for the detailed analysis.
diff --git a/hw/intc/i8259.c b/hw/intc/i8259.c index 0f5c025..43e90b9 100644 --- a/hw/intc/i8259.c +++ b/hw/intc/i8259.c @@ -429,7 +429,7 @@ static void pic_realize(DeviceState *dev, Error **errp) pc->parent_realize(dev, errp); } -void hmp_info_pic(Monitor *mon, const QDict *qdict) +void i8259_hmp_info_pic(Monitor *mon, const QDict *qdict) { int i; PICCommonState *s; @@ -447,7 +447,7 @@ void hmp_info_pic(Monitor *mon, const QDict *qdict) } } -void hmp_info_irq(Monitor *mon, const QDict *qdict) +void i8259_hmp_info_irq(Monitor *mon, const QDict *qdict) { #ifndef DEBUG_IRQ_COUNT monitor_printf(mon, "irq statistic code not compiled.\n"); diff --git a/include/hw/boards.h b/include/hw/boards.h index 3ddc449..214a778 100644 --- a/include/hw/boards.h +++ b/include/hw/boards.h @@ -111,6 +111,8 @@ struct MachineClass { HotplugHandler *(*get_hotplug_handler)(MachineState *machine, DeviceState *dev); + void (*hmp_info_irq)(Monitor *mon, const QDict *qdict); + void (*hmp_info_pic)(Monitor *mon, const QDict *qdict); }; /** diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h index 08ab67d..0f376c6 100644 --- a/include/hw/i386/pc.h +++ b/include/hw/i386/pc.h @@ -121,8 +121,8 @@ qemu_irq *i8259_init(ISABus *bus, qemu_irq parent_irq); qemu_irq *kvm_i8259_init(ISABus *bus); int pic_read_irq(DeviceState *d); int pic_get_output(DeviceState *d); -void hmp_info_pic(Monitor *mon, const QDict *qdict); -void hmp_info_irq(Monitor *mon, const QDict *qdict); +void i8259_hmp_info_pic(Monitor *mon, const QDict *qdict); +void i8259_hmp_info_irq(Monitor *mon, const QDict *qdict); /* Global System Interrupts */ diff --git a/monitor.c b/monitor.c index c86a89e..ca226a9 100644 --- a/monitor.c +++ b/monitor.c @@ -1064,6 +1064,48 @@ static void hmp_info_history(Monitor *mon, const QDict *qdict) } } +static void hmp_info_pic(Monitor *mon, const QDict *qdict) +{ + MachineClass *mc = MACHINE_GET_CLASS(current_machine); + + if (mc->hmp_info_pic) { + (mc->hmp_info_pic)(mon, qdict); + } else { + /* FIXME: Backwards compat fallbacks. These can go away once + * we've finished converting to natively using MachineClass, + * rather thatn QEMUMachine */ +#if defined(TARGET_SPARC) && !defined(TARGET_SPARC64) + sun4m_hmp_info_pic(mon, qdict); +#elif defined(TARGET_LM32) + lm32_hmp_info_pic(mon, qdict); +#elif defined(TARGET_i386) || defined(TARGET_PPC) || defined(TARGET_MIPS) + i8259_hmp_info_pic(mon, qdict); +#endif + } +} + +static void hmp_info_irq(Monitor *mon, const QDict *qdict) +{ + /* FIXME: The ifdefs can go away once the sun4m and LM32 machines + * are converted to use machine classes natively */ + MachineClass *mc = MACHINE_GET_CLASS(current_machine); + + if (mc->hmp_info_irq) { + (mc->hmp_info_irq)(mon, qdict); + } else { + /* FIXME: Backwards compat fallbacks. These can go away once + * we've finished converting to natively using MachineClass, + * rather thatn QEMUMachine */ +#if defined(TARGET_SPARC) && !defined(TARGET_SPARC64) + sun4m_hmp_info_irq(mon, qdict); +#elif defined(TARGET_LM32) + lm32_hmp_info_irq(mon, qdict); +#elif defined(TARGET_i386) || defined(TARGET_PPC) || defined(TARGET_MIPS) + i8259_hmp_info_irq(mon, qdict); +#endif + } +} + static void hmp_info_cpustats(Monitor *mon, const QDict *qdict) { CPUState *cpu; @@ -2661,35 +2703,20 @@ static mon_cmd_t info_cmds[] = { .help = "show the command line history", .mhandler.cmd = hmp_info_history, }, -#if defined(TARGET_I386) || defined(TARGET_PPC) || defined(TARGET_MIPS) || \ - defined(TARGET_LM32) || (defined(TARGET_SPARC) && !defined(TARGET_SPARC64)) { .name = "irq", .args_type = "", .params = "", .help = "show the interrupts statistics (if available)", -#ifdef TARGET_SPARC - .mhandler.cmd = sun4m_hmp_info_irq, -#elif defined(TARGET_LM32) - .mhandler.cmd = lm32_hmp_info_irq, -#else .mhandler.cmd = hmp_info_irq, -#endif }, { .name = "pic", .args_type = "", .params = "", .help = "show i8259 (PIC) state", -#ifdef TARGET_SPARC - .mhandler.cmd = sun4m_hmp_info_pic, -#elif defined(TARGET_LM32) - .mhandler.cmd = lm32_hmp_info_pic, -#else .mhandler.cmd = hmp_info_pic, -#endif }, -#endif { .name = "pci", .args_type = "",
The hmp commands "irq" and "pic" are a bit of a mess. They're implemented on a number of targets, but not all. On sparc32 and LM32 they do target specific things, but on the remainder (i386, ppc and mips) they call into the i8259 PIC code. But really, what these commands do shouldn't be dependent on the target arch, but on the specific machine that's in use. On ppc, for example, the "prep" machine usually does have an ISA bridge with an i8259, but most of the other machine types have never had an i8259 at all. Similarly the sparc specific target would stop working if we ever had a sparc32 machine that wasn't sun4m. This patch cleans things up by implementing these hmp commands on all targets via a MachineClass callback. If the callback is NULL, for now we fallback to target specific defaults that match the existing behaviour. The hope is we can remove those later with target specific cleanups. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> --- hw/intc/i8259.c | 4 ++-- include/hw/boards.h | 2 ++ include/hw/i386/pc.h | 4 ++-- monitor.c | 57 ++++++++++++++++++++++++++++++++++++++-------------- 4 files changed, 48 insertions(+), 19 deletions(-)