Re: [Qemu-devel] [PATCH 2/6] Remove monitor.c dependency on CONFIG_I8259

[David Gibson]

On Mon, Mar 30, 2015 at 10:37:45AM +0200, Markus Armbruster wrote:
> Markus Armbruster <address@hidden> writes:
> 
> > David Gibson <address@hidden> 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 <address@hidden>
> >
> >> ---
> >>  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

pgpoezv_ehEW9.pgp
Description: PGP signature