On Mon, Mar 30, 2015 at 10:37:45AM +0200, Markus Armbruster wrote:
> Markus Armbruster <arm...@redhat.com> writes:
>
> > David Gibson <da...@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 <da...@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 | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
pgpAscxhOaPdw.pgp
Description: PGP signature
