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

[Markus Armbruster]

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.

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  = "",