[Qemu-devel] [PATCH 0/6] Clean up ISA dependencies so we make ISA optional to build

[Qemu-devel] [PATCH 0/6] Clean up ISA dependencies so we make ISA optional to build

[David Gibson]

At present, ISA bus support is always included in the build for all
targets.  However these days there are a number of targets that have
never had ISA, and even more where many of the individual machines
don't have ISA.

Unfortunately there are some awkward dependencies in the core code on
ISA, although b19c1c0 "isa: remove isa_mem_base variable" did already
remove one.

This series engages in some yak shaving to make the necessary
dependency cleanups, then make inclusion of ISA support optional.

Given the date, this is obviously aimed at qemu 2.4, not 2.3.

David Gibson (6):
  Split serial-isa into its own config option
  Remove monitor.c dependency on CONFIG_I8259
  pc: Use MachineClass callbacks for "irq" and "pic" hmp commands
  target-ppc: Convert PReP to machine class
  prep: Use MachineClass callbacks for "irq" and "pic" hmp commands
  Allow ISA bus to be configured out

 default-configs/alpha-softmmu.mak     |  1 +
 default-configs/arm-softmmu.mak       |  1 +
 default-configs/i386-softmmu.mak      |  1 +
 default-configs/mips-softmmu.mak      |  1 +
 default-configs/mips64-softmmu.mak    |  1 +
 default-configs/mips64el-softmmu.mak  |  1 +
 default-configs/mipsel-softmmu.mak    |  1 +
 default-configs/moxie-softmmu.mak     |  2 ++
 default-configs/pci.mak               |  1 +
 default-configs/ppc-softmmu.mak       |  1 +
 default-configs/ppc64-softmmu.mak     |  1 +
 default-configs/ppcemb-softmmu.mak    |  1 +
 default-configs/sh4-softmmu.mak       |  1 +
 default-configs/sh4eb-softmmu.mak     |  1 +
 default-configs/sparc-softmmu.mak     |  1 +
 default-configs/sparc64-softmmu.mak   |  1 +
 default-configs/unicore32-softmmu.mak |  1 +
 default-configs/x86_64-softmmu.mak    |  1 +
 hw/char/Makefile.objs                 |  3 +-
 hw/i386/pc.c                          |  2 ++
 hw/intc/i8259.c                       |  4 +--
 hw/isa/Makefile.objs                  |  2 +-
 hw/ppc/prep.c                         | 32 ++++++++++++++------
 include/hw/boards.h                   |  2 ++
 include/hw/i386/pc.h                  |  4 +--
 monitor.c                             | 57 ++++++++++++++++++++++++++---------
 26 files changed, 95 insertions(+), 30 deletions(-)

-- 
2.1.0

[Qemu-devel] [PATCH 1/6] Split serial-isa into its own config option

[David Gibson]

At present, the core device model code for 8250-like serial ports
(serial.c) and the code for serial ports attached to ISA-style legacy IO
(serial-isa.c) are both controlled by the CONFIG_ISA variable.

There are lots and lots of embedded platforms that have 8250-like serial
ports but have never had anything resembling ISA legacy IO.  Therefore,
split serial-isa into its own CONFIG_SERIAL_ISA option so it can be
disabled for platforms where it's not appropriate.

For now, I enabled CONFIG_SERIAL_ISA in every default-config where
CONFIG_SERIAL is enabled, excepting microblaze and xtensa, where it's
pretty clear there isn't legacy IO stuff.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 default-configs/alpha-softmmu.mak    | 1 +
 default-configs/arm-softmmu.mak      | 1 +
 default-configs/i386-softmmu.mak     | 1 +
 default-configs/mips-softmmu.mak     | 1 +
 default-configs/mips64-softmmu.mak   | 1 +
 default-configs/mips64el-softmmu.mak | 1 +
 default-configs/mipsel-softmmu.mak   | 1 +
 default-configs/moxie-softmmu.mak    | 1 +
 default-configs/ppc-softmmu.mak      | 1 +
 default-configs/ppc64-softmmu.mak    | 1 +
 default-configs/ppcemb-softmmu.mak   | 1 +
 default-configs/sh4-softmmu.mak      | 1 +
 default-configs/sh4eb-softmmu.mak    | 1 +
 default-configs/sparc64-softmmu.mak  | 1 +
 default-configs/x86_64-softmmu.mak   | 1 +
 hw/char/Makefile.objs                | 3 ++-
 16 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/default-configs/alpha-softmmu.mak b/default-configs/alpha-softmmu.mak
index 7f6161e..e0d75e3 100644
--- a/default-configs/alpha-softmmu.mak
+++ b/default-configs/alpha-softmmu.mak
@@ -3,6 +3,7 @@
 include pci.mak
 include usb.mak
 CONFIG_SERIAL=y
+CONFIG_SERIAL_ISA=y
 CONFIG_I8254=y
 CONFIG_PCKBD=y
 CONFIG_VGA_CIRRUS=y
diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak
index 149ae1b..d862268 100644
--- a/default-configs/arm-softmmu.mak
+++ b/default-configs/arm-softmmu.mak
@@ -7,6 +7,7 @@ CONFIG_ISA_MMIO=y
 CONFIG_NAND=y
 CONFIG_ECC=y
 CONFIG_SERIAL=y
+CONFIG_SERIAL_ISA=y
 CONFIG_PTIMER=y
 CONFIG_SD=y
 CONFIG_MAX7310=y
diff --git a/default-configs/i386-softmmu.mak b/default-configs/i386-softmmu.mak
index 0b8ce4b..6e9c6c1 100644
--- a/default-configs/i386-softmmu.mak
+++ b/default-configs/i386-softmmu.mak
@@ -9,6 +9,7 @@ CONFIG_VGA_CIRRUS=y
 CONFIG_VMWARE_VGA=y
 CONFIG_VMMOUSE=y
 CONFIG_SERIAL=y
+CONFIG_SERIAL_ISA=y
 CONFIG_PARALLEL=y
 CONFIG_I8254=y
 CONFIG_PCSPK=y
diff --git a/default-configs/mips-softmmu.mak b/default-configs/mips-softmmu.mak
index cce2c81..28dee61 100644
--- a/default-configs/mips-softmmu.mak
+++ b/default-configs/mips-softmmu.mak
@@ -9,6 +9,7 @@ CONFIG_VGA_ISA_MM=y
 CONFIG_VGA_CIRRUS=y
 CONFIG_VMWARE_VGA=y
 CONFIG_SERIAL=y
+CONFIG_SERIAL_ISA=y
 CONFIG_PARALLEL=y
 CONFIG_I8254=y
 CONFIG_PCSPK=y
diff --git a/default-configs/mips64-softmmu.mak b/default-configs/mips64-softmmu.mak
index 7a88a08..464e8f1 100644
--- a/default-configs/mips64-softmmu.mak
+++ b/default-configs/mips64-softmmu.mak
@@ -9,6 +9,7 @@ CONFIG_VGA_ISA_MM=y
 CONFIG_VGA_CIRRUS=y
 CONFIG_VMWARE_VGA=y
 CONFIG_SERIAL=y
+CONFIG_SERIAL_ISA=y
 CONFIG_PARALLEL=y
 CONFIG_I8254=y
 CONFIG_PCSPK=y
diff --git a/default-configs/mips64el-softmmu.mak b/default-configs/mips64el-softmmu.mak
index 095de43..1b5d3f6 100644
--- a/default-configs/mips64el-softmmu.mak
+++ b/default-configs/mips64el-softmmu.mak
@@ -9,6 +9,7 @@ CONFIG_VGA_ISA_MM=y
 CONFIG_VGA_CIRRUS=y
 CONFIG_VMWARE_VGA=y
 CONFIG_SERIAL=y
+CONFIG_SERIAL_ISA=y
 CONFIG_PARALLEL=y
 CONFIG_I8254=y
 CONFIG_PCSPK=y
diff --git a/default-configs/mipsel-softmmu.mak b/default-configs/mipsel-softmmu.mak
index 0e25108..ff0e2c6 100644
--- a/default-configs/mipsel-softmmu.mak
+++ b/default-configs/mipsel-softmmu.mak
@@ -9,6 +9,7 @@ CONFIG_VGA_ISA_MM=y
 CONFIG_VGA_CIRRUS=y
 CONFIG_VMWARE_VGA=y
 CONFIG_SERIAL=y
+CONFIG_SERIAL_ISA=y
 CONFIG_PARALLEL=y
 CONFIG_I8254=y
 CONFIG_PCSPK=y
diff --git a/default-configs/moxie-softmmu.mak b/default-configs/moxie-softmmu.mak
index 1a95476..7e22863 100644
--- a/default-configs/moxie-softmmu.mak
+++ b/default-configs/moxie-softmmu.mak
@@ -2,4 +2,5 @@
 
 CONFIG_MC146818RTC=y
 CONFIG_SERIAL=y
+CONFIG_SERIAL_ISA=y
 CONFIG_VGA=y
diff --git a/default-configs/ppc-softmmu.mak b/default-configs/ppc-softmmu.mak
index 4b60e69..c969b5b 100644
--- a/default-configs/ppc-softmmu.mak
+++ b/default-configs/ppc-softmmu.mak
@@ -47,5 +47,6 @@ CONFIG_PLATFORM_BUS=y
 CONFIG_ETSEC=y
 CONFIG_LIBDECNUMBER=y
 # For PReP
+CONFIG_SERIAL_ISA=y
 CONFIG_MC146818RTC=y
 CONFIG_ISA_TESTDEV=y
diff --git a/default-configs/ppc64-softmmu.mak b/default-configs/ppc64-softmmu.mak
index de71e41..3a1e26a 100644
--- a/default-configs/ppc64-softmmu.mak
+++ b/default-configs/ppc64-softmmu.mak
@@ -51,6 +51,7 @@ CONFIG_LIBDECNUMBER=y
 CONFIG_XICS=$(CONFIG_PSERIES)
 CONFIG_XICS_KVM=$(and $(CONFIG_PSERIES),$(CONFIG_KVM))
 # For PReP
+CONFIG_SERIAL_ISA=y
 CONFIG_I82378=y
 CONFIG_I8259=y
 CONFIG_I8254=y
diff --git a/default-configs/ppcemb-softmmu.mak b/default-configs/ppcemb-softmmu.mak
index a1b3d5f..3a2af57 100644
--- a/default-configs/ppcemb-softmmu.mak
+++ b/default-configs/ppcemb-softmmu.mak
@@ -5,6 +5,7 @@ include sound.mak
 include usb.mak
 CONFIG_M48T59=y
 CONFIG_SERIAL=y
+CONFIG_SERIAL_ISA=y
 CONFIG_I8257=y
 CONFIG_OPENPIC=y
 CONFIG_PFLASH_CFI01=y
diff --git a/default-configs/sh4-softmmu.mak b/default-configs/sh4-softmmu.mak
index 8e00390..546d855 100644
--- a/default-configs/sh4-softmmu.mak
+++ b/default-configs/sh4-softmmu.mak
@@ -3,6 +3,7 @@
 include pci.mak
 include usb.mak
 CONFIG_SERIAL=y
+CONFIG_SERIAL_ISA=y
 CONFIG_PTIMER=y
 CONFIG_PFLASH_CFI02=y
 CONFIG_SH4=y
diff --git a/default-configs/sh4eb-softmmu.mak b/default-configs/sh4eb-softmmu.mak
index efdd058..2d3fd49 100644
--- a/default-configs/sh4eb-softmmu.mak
+++ b/default-configs/sh4eb-softmmu.mak
@@ -3,6 +3,7 @@
 include pci.mak
 include usb.mak
 CONFIG_SERIAL=y
+CONFIG_SERIAL_ISA=y
 CONFIG_PTIMER=y
 CONFIG_PFLASH_CFI02=y
 CONFIG_SH4=y
diff --git a/default-configs/sparc64-softmmu.mak b/default-configs/sparc64-softmmu.mak
index 123bb99..b79272c 100644
--- a/default-configs/sparc64-softmmu.mak
+++ b/default-configs/sparc64-softmmu.mak
@@ -6,6 +6,7 @@ CONFIG_ISA_MMIO=y
 CONFIG_M48T59=y
 CONFIG_PTIMER=y
 CONFIG_SERIAL=y
+CONFIG_SERIAL_ISA=y
 CONFIG_PARALLEL=y
 CONFIG_PCKBD=y
 CONFIG_FDC=y
diff --git a/default-configs/x86_64-softmmu.mak b/default-configs/x86_64-softmmu.mak
index 6add04a..07977e3 100644
--- a/default-configs/x86_64-softmmu.mak
+++ b/default-configs/x86_64-softmmu.mak
@@ -9,6 +9,7 @@ CONFIG_VGA_CIRRUS=y
 CONFIG_VMWARE_VGA=y
 CONFIG_VMMOUSE=y
 CONFIG_SERIAL=y
+CONFIG_SERIAL_ISA=y
 CONFIG_PARALLEL=y
 CONFIG_I8254=y
 CONFIG_PCSPK=y
diff --git a/hw/char/Makefile.objs b/hw/char/Makefile.objs
index 317385d..dd63815 100644
--- a/hw/char/Makefile.objs
+++ b/hw/char/Makefile.objs
@@ -2,7 +2,8 @@ common-obj-$(CONFIG_IPACK) += ipoctal232.o
 common-obj-$(CONFIG_ESCC) += escc.o
 common-obj-$(CONFIG_PARALLEL) += parallel.o
 common-obj-$(CONFIG_PL011) += pl011.o
-common-obj-$(CONFIG_SERIAL) += serial.o serial-isa.o
+common-obj-$(CONFIG_SERIAL) += serial.o
+common-obj-$(CONFIG_SERIAL_ISA) += serial-isa.o
 common-obj-$(CONFIG_SERIAL_PCI) += serial-pci.o
 common-obj-$(CONFIG_VIRTIO) += virtio-console.o
 common-obj-$(CONFIG_XILINX) += xilinx_uartlite.o
-- 
2.1.0

Re: [Qemu-devel] [PATCH 1/6] Split serial-isa into its own config option

[Markus Armbruster]

David Gibson <david@gibson.dropbear.id.au> writes:

> At present, the core device model code for 8250-like serial ports
> (serial.c) and the code for serial ports attached to ISA-style legacy IO
> (serial-isa.c) are both controlled by the CONFIG_ISA variable.
>
> There are lots and lots of embedded platforms that have 8250-like serial
> ports but have never had anything resembling ISA legacy IO.  Therefore,
> split serial-isa into its own CONFIG_SERIAL_ISA option so it can be
> disabled for platforms where it's not appropriate.
>
> For now, I enabled CONFIG_SERIAL_ISA in every default-config where
> CONFIG_SERIAL is enabled, excepting microblaze and xtensa, where it's
> pretty clear there isn't legacy IO stuff.

Related: in PATCH 6, you configure ISA support away for a bunch of
machines.  This includes device isabus-bridge.  You keep it for machines
sporting PCI.

> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  default-configs/alpha-softmmu.mak    | 1 +
>  default-configs/arm-softmmu.mak      | 1 +
>  default-configs/i386-softmmu.mak     | 1 +
>  default-configs/mips-softmmu.mak     | 1 +
>  default-configs/mips64-softmmu.mak   | 1 +
>  default-configs/mips64el-softmmu.mak | 1 +
>  default-configs/mipsel-softmmu.mak   | 1 +
>  default-configs/moxie-softmmu.mak    | 1 +
>  default-configs/ppc-softmmu.mak      | 1 +
>  default-configs/ppc64-softmmu.mak    | 1 +
>  default-configs/ppcemb-softmmu.mak   | 1 +
>  default-configs/sh4-softmmu.mak      | 1 +
>  default-configs/sh4eb-softmmu.mak    | 1 +
>  default-configs/sparc64-softmmu.mak  | 1 +
>  default-configs/x86_64-softmmu.mak   | 1 +
>  hw/char/Makefile.objs                | 3 ++-
>  16 files changed, 17 insertions(+), 1 deletion(-)

In addition for not adding CONFIG_SERIAL_ISA to microblaze and xtensa,
you don't seem to add it to or32.  If that's correct, please adjust your
commit message.

Quick check for machines sporting no ISA device other than isa-serial:

    $ for i in *-softmmu/qemu-system*; do echo -e 'info qdm\nq' | $i -S -M none -monitor stdio -display none | grep -v '^name "isa-serial"' | grep -q 'bus ISA' || echo $i; done
    aarch64-softmmu/qemu-system-aarch64
    arm-softmmu/qemu-system-arm
    cris-softmmu/qemu-system-cris
    lm32-softmmu/qemu-system-lm32
    m68k-softmmu/qemu-system-m68k
    microblaze-softmmu/qemu-system-microblaze
    microblazeel-softmmu/qemu-system-microblazeel
    or32-softmmu/qemu-system-or32
    s390x-softmmu/qemu-system-s390x
    tricore-softmmu/qemu-system-tricore
    xtensa-softmmu/qemu-system-xtensa
    xtensaeb-softmmu/qemu-system-xtensaeb

Same check for PCI devices:

    cris-softmmu/qemu-system-cris
    lm32-softmmu/qemu-system-lm32
    microblaze-softmmu/qemu-system-microblaze
    microblazeel-softmmu/qemu-system-microblazeel
    moxie-softmmu/qemu-system-moxie
    or32-softmmu/qemu-system-or32
    sparc-softmmu/qemu-system-sparc
    tricore-softmmu/qemu-system-tricore
    unicore32-softmmu/qemu-system-unicore32
    xtensa-softmmu/qemu-system-xtensa
    xtensaeb-softmmu/qemu-system-xtensaeb

Machines with neither kind of device:

    cris-softmmu/qemu-system-cris
    lm32-softmmu/qemu-system-lm32
    microblaze-softmmu/qemu-system-microblaze
    microblazeel-softmmu/qemu-system-microblazeel
    or32-softmmu/qemu-system-or32
    tricore-softmmu/qemu-system-tricore
    xtensa-softmmu/qemu-system-xtensa
    xtensaeb-softmmu/qemu-system-xtensaeb

I figure none of them has a use for isa-serial after PATCH 6.  Shouldn't
we drop CONFIG_SERIAL_ISA for all of them, not just microblaze, xtensa
and or32?

Patch looks good otherwise.

Re: [Qemu-devel] [PATCH 1/6] Split serial-isa into its own config option

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 3225 bytes --]

On Mon, Mar 30, 2015 at 09:28:39AM +0200, Markus Armbruster wrote:
> David Gibson <david@gibson.dropbear.id.au> writes:
> 
> > At present, the core device model code for 8250-like serial ports
> > (serial.c) and the code for serial ports attached to ISA-style legacy IO
> > (serial-isa.c) are both controlled by the CONFIG_ISA variable.
> >
> > There are lots and lots of embedded platforms that have 8250-like serial
> > ports but have never had anything resembling ISA legacy IO.  Therefore,
> > split serial-isa into its own CONFIG_SERIAL_ISA option so it can be
> > disabled for platforms where it's not appropriate.
> >
> > For now, I enabled CONFIG_SERIAL_ISA in every default-config where
> > CONFIG_SERIAL is enabled, excepting microblaze and xtensa, where it's
> > pretty clear there isn't legacy IO stuff.
> 
> Related: in PATCH 6, you configure ISA support away for a bunch of
> machines.  This includes device isabus-bridge.  You keep it for machines
> sporting PCI.
> 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  default-configs/alpha-softmmu.mak    | 1 +
> >  default-configs/arm-softmmu.mak      | 1 +
> >  default-configs/i386-softmmu.mak     | 1 +
> >  default-configs/mips-softmmu.mak     | 1 +
> >  default-configs/mips64-softmmu.mak   | 1 +
> >  default-configs/mips64el-softmmu.mak | 1 +
> >  default-configs/mipsel-softmmu.mak   | 1 +
> >  default-configs/moxie-softmmu.mak    | 1 +
> >  default-configs/ppc-softmmu.mak      | 1 +
> >  default-configs/ppc64-softmmu.mak    | 1 +
> >  default-configs/ppcemb-softmmu.mak   | 1 +
> >  default-configs/sh4-softmmu.mak      | 1 +
> >  default-configs/sh4eb-softmmu.mak    | 1 +
> >  default-configs/sparc64-softmmu.mak  | 1 +
> >  default-configs/x86_64-softmmu.mak   | 1 +
> >  hw/char/Makefile.objs                | 3 ++-
> >  16 files changed, 17 insertions(+), 1 deletion(-)
> 
> In addition for not adding CONFIG_SERIAL_ISA to microblaze and xtensa,
> you don't seem to add it to or32.  If that's correct, please adjust your
> commit message.

Oops.  I think I first wrote this patch on a downstream qemu, and
forgot to update the commit message when the or32 case was added when
I ported upstream.

Will fix in the next spin.

> Quick check for machines sporting no ISA device other than
> isa-serial:
[snip]
> Machines with neither kind of device:
> 
>     cris-softmmu/qemu-system-cris
>     lm32-softmmu/qemu-system-lm32
>     microblaze-softmmu/qemu-system-microblaze
>     microblazeel-softmmu/qemu-system-microblazeel
>     or32-softmmu/qemu-system-or32
>     tricore-softmmu/qemu-system-tricore
>     xtensa-softmmu/qemu-system-xtensa
>     xtensaeb-softmmu/qemu-system-xtensaeb
> 
> I figure none of them has a use for isa-serial after PATCH 6.  Shouldn't
> we drop CONFIG_SERIAL_ISA for all of them, not just microblaze, xtensa
> and or32?

cris, lm32 and tricore don't have CONFIG_SERIAL at all, so that only
leaves microblaze, xtensa and or32.

-- 
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

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

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

[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(-)

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

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

[Markus Armbruster]

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

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

[Markus Armbruster]

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

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

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 8937 bytes --]

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			| 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

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

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

[Markus Armbruster]

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.

[...]

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

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1886 bytes --]

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.

-- 
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

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

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

[Andreas Färber]

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

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Jennifer Guild, Dilip Upmanyu,
Graham Norton; HRB 21284 (AG Nürnberg)

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

[Peter Maydell]

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

[Qemu-devel] [PATCH 3/6] pc: Use MachineClass callbacks for "irq" and "pic" hmp commands

[David Gibson]

Currently PC machine types rely on fallback code in the monitor
implementation to correctly implement these hmp commands.  Now that we have
MachineClass callbacks to control this properly, instantiate them in
pc_generic_machine_class_init().

Since this sets the MachineClass callbacks correctly for all x86 machine
types, we can now remove the TARGET_I386 fallback case from the monitor
code.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/i386/pc.c | 2 ++
 monitor.c    | 4 ++--
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index b229856..cb48165 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1522,6 +1522,8 @@ static void pc_generic_machine_class_init(ObjectClass *oc, void *data)
     mc->default_display = qm->default_display;
     mc->compat_props = qm->compat_props;
     mc->hw_version = qm->hw_version;
+    mc->hmp_info_irq = i8259_hmp_info_irq;
+    mc->hmp_info_pic = i8259_hmp_info_pic;
 }
 
 void qemu_register_pc_machine(QEMUMachine *m)
diff --git a/monitor.c b/monitor.c
index ca226a9..30da438 100644
--- a/monitor.c
+++ b/monitor.c
@@ -1078,7 +1078,7 @@ static void hmp_info_pic(Monitor *mon, const QDict *qdict)
         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)
+#elif defined(TARGET_PPC) || defined(TARGET_MIPS)
         i8259_hmp_info_pic(mon, qdict);
 #endif
     }
@@ -1100,7 +1100,7 @@ static void hmp_info_irq(Monitor *mon, const QDict *qdict)
         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)
+#elif defined(TARGET_PPC) || defined(TARGET_MIPS)
         i8259_hmp_info_irq(mon, qdict);
 #endif
     }
-- 
2.1.0

Re: [Qemu-devel] [PATCH 3/6] pc: Use MachineClass callbacks for "irq" and "pic" hmp commands

[Andreas Färber]

Am 06.03.2015 um 05:18 schrieb David Gibson:
> Currently PC machine types rely on fallback code in the monitor
> implementation to correctly implement these hmp commands.  Now that we have
> MachineClass callbacks to control this properly, instantiate them in
> pc_generic_machine_class_init().
> 
> Since this sets the MachineClass callbacks correctly for all x86 machine
> types, we can now remove the TARGET_I386 fallback case from the monitor
> code.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

Reviewed-by: Andreas Färber <afaerber@suse.de>

Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Jennifer Guild, Dilip Upmanyu,
Graham Norton; HRB 21284 (AG Nürnberg)

[Qemu-devel] [PATCH 4/6] target-ppc: Convert PReP to machine class

[David Gibson]

The more commonly used ppc machine types: spapr, and newworld Mac have
already been converted to the newer MachineClass representation, but some
others still use QEMUMachine.

This patch cleans things up slightly, by converting the "prep" machine
type to the new style.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/prep.c | 30 +++++++++++++++++++++---------
 1 file changed, 21 insertions(+), 9 deletions(-)

diff --git a/hw/ppc/prep.c b/hw/ppc/prep.c
index 15df7f3..dfc8689 100644
--- a/hw/ppc/prep.c
+++ b/hw/ppc/prep.c
@@ -44,6 +44,8 @@
 #include "exec/address-spaces.h"
 #include "elf.h"
 
+#define TYPE_PREP_MACHINE "PReP-machine"
+
 //#define HARD_DEBUG_PPC_IO
 //#define DEBUG_PPC_IO
 
@@ -561,17 +563,27 @@ static void ppc_prep_init(MachineState *machine)
                          graphic_width, graphic_height, graphic_depth);
 }
 
-static QEMUMachine prep_machine = {
-    .name = "prep",
-    .desc = "PowerPC PREP platform",
-    .init = ppc_prep_init,
-    .max_cpus = MAX_CPUS,
-    .default_boot_order = "cad",
+static void prep_machine_class_init(ObjectClass *oc, void *data)
+{
+    MachineClass *mc = MACHINE_CLASS(oc);
+
+    mc->name = "prep";
+    mc->desc = "PowerPC PREP platform";
+    mc->init = ppc_prep_init;
+    mc->max_cpus = MAX_CPUS;
+    mc->default_boot_order = "cad";
+}
+
+static const TypeInfo prep_machine_info = {
+    .name          = TYPE_PREP_MACHINE,
+    .parent        = TYPE_MACHINE,
+    .instance_size = sizeof(MachineState),
+    .class_init    = prep_machine_class_init,
 };
 
-static void prep_machine_init(void)
+static void prep_machine_register_types(void)
 {
-    qemu_register_machine(&prep_machine);
+    type_register_static(&prep_machine_info);
 }
 
-machine_init(prep_machine_init);
+type_init(prep_machine_register_types)
-- 
2.1.0

Re: [Qemu-devel] [PATCH 4/6] target-ppc: Convert PReP to machine class

[Andreas Färber]

Am 06.03.2015 um 05:18 schrieb David Gibson:
> The more commonly used ppc machine types: spapr, and newworld Mac have
> already been converted to the newer MachineClass representation, but some
> others still use QEMUMachine.
> 
> This patch cleans things up slightly, by converting the "prep" machine
> type to the new style.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  hw/ppc/prep.c | 30 +++++++++++++++++++++---------
>  1 file changed, 21 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/ppc/prep.c b/hw/ppc/prep.c
> index 15df7f3..dfc8689 100644
> --- a/hw/ppc/prep.c
> +++ b/hw/ppc/prep.c
> @@ -44,6 +44,8 @@
>  #include "exec/address-spaces.h"
>  #include "elf.h"
>  
> +#define TYPE_PREP_MACHINE "PReP-machine"
[snip]

qemu_register_machine() generated the type name by concatenating
QEMUMachine::name with "-machine", so this should be "prep-machine".

Otherwise looks great.

Thanks,
Andreas

Re: [Qemu-devel] [PATCH 4/6] target-ppc: Convert PReP to machine class

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1358 bytes --]

On Mon, Mar 30, 2015 at 11:33:43PM +0200, Andreas Färber wrote:
> Am 06.03.2015 um 05:18 schrieb David Gibson:
> > The more commonly used ppc machine types: spapr, and newworld Mac have
> > already been converted to the newer MachineClass representation, but some
> > others still use QEMUMachine.
> > 
> > This patch cleans things up slightly, by converting the "prep" machine
> > type to the new style.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  hw/ppc/prep.c | 30 +++++++++++++++++++++---------
> >  1 file changed, 21 insertions(+), 9 deletions(-)
> > 
> > diff --git a/hw/ppc/prep.c b/hw/ppc/prep.c
> > index 15df7f3..dfc8689 100644
> > --- a/hw/ppc/prep.c
> > +++ b/hw/ppc/prep.c
> > @@ -44,6 +44,8 @@
> >  #include "exec/address-spaces.h"
> >  #include "elf.h"
> >  
> > +#define TYPE_PREP_MACHINE "PReP-machine"
> [snip]
> 
> qemu_register_machine() generated the type name by concatenating
> QEMUMachine::name with "-machine", so this should be "prep-machine".

Ah, I thought the type names were internal only and therefore
changeable.  I've corrected this.

> Otherwise looks great.

-- 
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

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

[Qemu-devel] [PATCH 5/6] prep: Use MachineClass callbacks for "irq" and "pic" hmp commands

[David Gibson]

Currently all ppc targets rely on fallback code in monitor.c to implement
the "irq" and "pic" hmp commands, by calling into the i8259 code.  For the
PReP machine type, which does usually have an ISA bridge and legacy IO,
including an i8259, this patch correctly sets the MachineClass callbacks
to implement those commands properly without the fallback.

In fact PReP is the only ppc machine for which the i8259 implementation
of those hmp commands makes sense.  The other machine types won't typically
have an i8259 at all.  So we can remove the fallback case from the monitor
meaning that other ppc targets will correctly implement those commands
as no-ops.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/prep.c | 2 ++
 monitor.c     | 4 ++--
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/hw/ppc/prep.c b/hw/ppc/prep.c
index dfc8689..b99e87d 100644
--- a/hw/ppc/prep.c
+++ b/hw/ppc/prep.c
@@ -572,6 +572,8 @@ static void prep_machine_class_init(ObjectClass *oc, void *data)
     mc->init = ppc_prep_init;
     mc->max_cpus = MAX_CPUS;
     mc->default_boot_order = "cad";
+    mc->hmp_info_irq = i8259_hmp_info_irq;
+    mc->hmp_info_pic = i8259_hmp_info_pic;
 }
 
 static const TypeInfo prep_machine_info = {
diff --git a/monitor.c b/monitor.c
index 30da438..3165539 100644
--- a/monitor.c
+++ b/monitor.c
@@ -1078,7 +1078,7 @@ static void hmp_info_pic(Monitor *mon, const QDict *qdict)
         sun4m_hmp_info_pic(mon, qdict);
 #elif defined(TARGET_LM32)
         lm32_hmp_info_pic(mon, qdict);
-#elif defined(TARGET_PPC) || defined(TARGET_MIPS)
+#elif defined(TARGET_MIPS)
         i8259_hmp_info_pic(mon, qdict);
 #endif
     }
@@ -1100,7 +1100,7 @@ static void hmp_info_irq(Monitor *mon, const QDict *qdict)
         sun4m_hmp_info_irq(mon, qdict);
 #elif defined(TARGET_LM32)
         lm32_hmp_info_irq(mon, qdict);
-#elif defined(TARGET_PPC) || defined(TARGET_MIPS)
+#elif defined(TARGET_MIPS)
         i8259_hmp_info_irq(mon, qdict);
 #endif
     }
-- 
2.1.0

Re: [Qemu-devel] [PATCH 5/6] prep: Use MachineClass callbacks for "irq" and "pic" hmp commands

[Andreas Färber]

Am 06.03.2015 um 05:18 schrieb David Gibson:
> Currently all ppc targets rely on fallback code in monitor.c to implement
> the "irq" and "pic" hmp commands, by calling into the i8259 code.  For the
> PReP machine type, which does usually have an ISA bridge and legacy IO,
> including an i8259, this patch correctly sets the MachineClass callbacks
> to implement those commands properly without the fallback.
> 
> In fact PReP is the only ppc machine for which the i8259 implementation
> of those hmp commands makes sense.  The other machine types won't typically
> have an i8259 at all.  So we can remove the fallback case from the monitor
> meaning that other ppc targets will correctly implement those commands
> as no-ops.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  hw/ppc/prep.c | 2 ++
>  monitor.c     | 4 ++--
>  2 files changed, 4 insertions(+), 2 deletions(-)

Reviewed-by: Andreas Färber <andreas.faerber@web.de>

Thanks,
Andreas

[Qemu-devel] [PATCH 6/6] Allow ISA bus to be configured out

[David Gibson]

Currently, the code to handle the legacy ISA bus is always included in
qemu.  However there are lots of platforms that don't include ISA legacy
devies, and quite a few that have never used ISA legacy devices at all.

This patch allows the ISA bus code to be disabled in the configuration for
platforms where it doesn't make sense.  For now, the default configs are
adjusted to include ISA on all platforms including PCI (since
CONFIG_IDE_CORE which is in pci.mak requires ISA support) and also several
others which include ISA devices.  We may want to pare this down in future.

This patch becomes more useful since b19c1c0 "isa: remove isa_mem_base
variable." since that removes a dependency on isa-bus.c from vga.c.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 default-configs/moxie-softmmu.mak     | 1 +
 default-configs/pci.mak               | 1 +
 default-configs/sparc-softmmu.mak     | 1 +
 default-configs/unicore32-softmmu.mak | 1 +
 hw/isa/Makefile.objs                  | 2 +-
 5 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/default-configs/moxie-softmmu.mak b/default-configs/moxie-softmmu.mak
index 7e22863..e00d099 100644
--- a/default-configs/moxie-softmmu.mak
+++ b/default-configs/moxie-softmmu.mak
@@ -1,5 +1,6 @@
 # Default configuration for moxie-softmmu
 
+CONFIG_ISA_BUS=y
 CONFIG_MC146818RTC=y
 CONFIG_SERIAL=y
 CONFIG_SERIAL_ISA=y
diff --git a/default-configs/pci.mak b/default-configs/pci.mak
index 58a2c0a..b082500 100644
--- a/default-configs/pci.mak
+++ b/default-configs/pci.mak
@@ -1,4 +1,5 @@
 CONFIG_PCI=y
+CONFIG_ISA_BUS=y
 CONFIG_VIRTIO_PCI=y
 CONFIG_VIRTIO=y
 CONFIG_USB_UHCI=y
diff --git a/default-configs/sparc-softmmu.mak b/default-configs/sparc-softmmu.mak
index ab796b3..004b0f4 100644
--- a/default-configs/sparc-softmmu.mak
+++ b/default-configs/sparc-softmmu.mak
@@ -1,5 +1,6 @@
 # Default configuration for sparc-softmmu
 
+CONFIG_ISA_BUS=y
 CONFIG_ECC=y
 CONFIG_ESP=y
 CONFIG_ESCC=y
diff --git a/default-configs/unicore32-softmmu.mak b/default-configs/unicore32-softmmu.mak
index de38577..5f6c4a8 100644
--- a/default-configs/unicore32-softmmu.mak
+++ b/default-configs/unicore32-softmmu.mak
@@ -1,4 +1,5 @@
 # Default configuration for unicore32-softmmu
+CONFIG_ISA_BUS=y
 CONFIG_PUV3=y
 CONFIG_PTIMER=y
 CONFIG_PCKBD=y
diff --git a/hw/isa/Makefile.objs b/hw/isa/Makefile.objs
index 9164556..fb37c55 100644
--- a/hw/isa/Makefile.objs
+++ b/hw/isa/Makefile.objs
@@ -1,4 +1,4 @@
-common-obj-y += isa-bus.o
+common-obj-$(CONFIG_ISA_BUS) += isa-bus.o
 common-obj-$(CONFIG_APM) += apm.o
 common-obj-$(CONFIG_I82378) += i82378.o
 common-obj-$(CONFIG_PC87312) += pc87312.o
-- 
2.1.0

Re: [Qemu-devel] [PATCH 0/6] Clean up ISA dependencies so we make ISA optional to build

[Alexander Graf]

On 06.03.15 05:18, David Gibson wrote:
> At present, ISA bus support is always included in the build for all
> targets.  However these days there are a number of targets that have
> never had ISA, and even more where many of the individual machines
> don't have ISA.
> 
> Unfortunately there are some awkward dependencies in the core code on
> ISA, although b19c1c0 "isa: remove isa_mem_base variable" did already
> remove one.
> 
> This series engages in some yak shaving to make the necessary
> dependency cleanups, then make inclusion of ISA support optional.
> 
> Given the date, this is obviously aimed at qemu 2.4, not 2.3.

Reviewed-by: Alexander Graf <agraf@suse.de>


Alex

Re: [Qemu-devel] [PATCH 0/6] Clean up ISA dependencies so we make ISA optional to build

[Michael S. Tsirkin]

On Fri, Mar 06, 2015 at 03:18:20PM +1100, David Gibson wrote:
> At present, ISA bus support is always included in the build for all
> targets.  However these days there are a number of targets that have
> never had ISA, and even more where many of the individual machines
> don't have ISA.
> 
> Unfortunately there are some awkward dependencies in the core code on
> ISA, although b19c1c0 "isa: remove isa_mem_base variable" did already
> remove one.
> 
> This series engages in some yak shaving to make the necessary
> dependency cleanups, then make inclusion of ISA support optional.

For PC/PCI changes
Acked-by: Michael S. Tsirkin <mst@redhat.com>


> 
> Given the date, this is obviously aimed at qemu 2.4, not 2.3.

Looks like the date for 2.3 is unclear, so it might be
ok to merge.

Who's taking this? Luiz? most changes are monitor-related.

> David Gibson (6):
>   Split serial-isa into its own config option
>   Remove monitor.c dependency on CONFIG_I8259
>   pc: Use MachineClass callbacks for "irq" and "pic" hmp commands
>   target-ppc: Convert PReP to machine class
>   prep: Use MachineClass callbacks for "irq" and "pic" hmp commands
>   Allow ISA bus to be configured out
> 
>  default-configs/alpha-softmmu.mak     |  1 +
>  default-configs/arm-softmmu.mak       |  1 +
>  default-configs/i386-softmmu.mak      |  1 +
>  default-configs/mips-softmmu.mak      |  1 +
>  default-configs/mips64-softmmu.mak    |  1 +
>  default-configs/mips64el-softmmu.mak  |  1 +
>  default-configs/mipsel-softmmu.mak    |  1 +
>  default-configs/moxie-softmmu.mak     |  2 ++
>  default-configs/pci.mak               |  1 +
>  default-configs/ppc-softmmu.mak       |  1 +
>  default-configs/ppc64-softmmu.mak     |  1 +
>  default-configs/ppcemb-softmmu.mak    |  1 +
>  default-configs/sh4-softmmu.mak       |  1 +
>  default-configs/sh4eb-softmmu.mak     |  1 +
>  default-configs/sparc-softmmu.mak     |  1 +
>  default-configs/sparc64-softmmu.mak   |  1 +
>  default-configs/unicore32-softmmu.mak |  1 +
>  default-configs/x86_64-softmmu.mak    |  1 +
>  hw/char/Makefile.objs                 |  3 +-
>  hw/i386/pc.c                          |  2 ++
>  hw/intc/i8259.c                       |  4 +--
>  hw/isa/Makefile.objs                  |  2 +-
>  hw/ppc/prep.c                         | 32 ++++++++++++++------
>  include/hw/boards.h                   |  2 ++
>  include/hw/i386/pc.h                  |  4 +--
>  monitor.c                             | 57 ++++++++++++++++++++++++++---------
>  26 files changed, 95 insertions(+), 30 deletions(-)
> 
> -- 
> 2.1.0

Re: [Qemu-devel] [PATCH 0/6] Clean up ISA dependencies so we make ISA optional to build

[Luiz Capitulino]

On Tue, 10 Mar 2015 15:20:29 +0100
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Fri, Mar 06, 2015 at 03:18:20PM +1100, David Gibson wrote:
> > At present, ISA bus support is always included in the build for all
> > targets.  However these days there are a number of targets that have
> > never had ISA, and even more where many of the individual machines
> > don't have ISA.
> > 
> > Unfortunately there are some awkward dependencies in the core code on
> > ISA, although b19c1c0 "isa: remove isa_mem_base variable" did already
> > remove one.
> > 
> > This series engages in some yak shaving to make the necessary
> > dependency cleanups, then make inclusion of ISA support optional.
> 
> For PC/PCI changes
> Acked-by: Michael S. Tsirkin <mst@redhat.com>
> 
> 
> > 
> > Given the date, this is obviously aimed at qemu 2.4, not 2.3.
> 
> Looks like the date for 2.3 is unclear, so it might be
> ok to merge.
> 
> Who's taking this? Luiz? most changes are monitor-related.

I can take them, but the most important question for me is
who's reviewing them? I could do it, but not right now.

> 
> > David Gibson (6):
> >   Split serial-isa into its own config option
> >   Remove monitor.c dependency on CONFIG_I8259
> >   pc: Use MachineClass callbacks for "irq" and "pic" hmp commands
> >   target-ppc: Convert PReP to machine class
> >   prep: Use MachineClass callbacks for "irq" and "pic" hmp commands
> >   Allow ISA bus to be configured out
> > 
> >  default-configs/alpha-softmmu.mak     |  1 +
> >  default-configs/arm-softmmu.mak       |  1 +
> >  default-configs/i386-softmmu.mak      |  1 +
> >  default-configs/mips-softmmu.mak      |  1 +
> >  default-configs/mips64-softmmu.mak    |  1 +
> >  default-configs/mips64el-softmmu.mak  |  1 +
> >  default-configs/mipsel-softmmu.mak    |  1 +
> >  default-configs/moxie-softmmu.mak     |  2 ++
> >  default-configs/pci.mak               |  1 +
> >  default-configs/ppc-softmmu.mak       |  1 +
> >  default-configs/ppc64-softmmu.mak     |  1 +
> >  default-configs/ppcemb-softmmu.mak    |  1 +
> >  default-configs/sh4-softmmu.mak       |  1 +
> >  default-configs/sh4eb-softmmu.mak     |  1 +
> >  default-configs/sparc-softmmu.mak     |  1 +
> >  default-configs/sparc64-softmmu.mak   |  1 +
> >  default-configs/unicore32-softmmu.mak |  1 +
> >  default-configs/x86_64-softmmu.mak    |  1 +
> >  hw/char/Makefile.objs                 |  3 +-
> >  hw/i386/pc.c                          |  2 ++
> >  hw/intc/i8259.c                       |  4 +--
> >  hw/isa/Makefile.objs                  |  2 +-
> >  hw/ppc/prep.c                         | 32 ++++++++++++++------
> >  include/hw/boards.h                   |  2 ++
> >  include/hw/i386/pc.h                  |  4 +--
> >  monitor.c                             | 57 ++++++++++++++++++++++++++---------
> >  26 files changed, 95 insertions(+), 30 deletions(-)
> > 
> > -- 
> > 2.1.0
> 

Re: [Qemu-devel] [PATCH 0/6] Clean up ISA dependencies so we make ISA optional to build

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1561 bytes --]

On Tue, Mar 10, 2015 at 10:56:22AM -0400, Luiz Capitulino wrote:
> On Tue, 10 Mar 2015 15:20:29 +0100
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Fri, Mar 06, 2015 at 03:18:20PM +1100, David Gibson wrote:
> > > At present, ISA bus support is always included in the build for all
> > > targets.  However these days there are a number of targets that have
> > > never had ISA, and even more where many of the individual machines
> > > don't have ISA.
> > > 
> > > Unfortunately there are some awkward dependencies in the core code on
> > > ISA, although b19c1c0 "isa: remove isa_mem_base variable" did already
> > > remove one.
> > > 
> > > This series engages in some yak shaving to make the necessary
> > > dependency cleanups, then make inclusion of ISA support optional.
> > 
> > For PC/PCI changes
> > Acked-by: Michael S. Tsirkin <mst@redhat.com>
> > 
> > 
> > > 
> > > Given the date, this is obviously aimed at qemu 2.4, not 2.3.
> > 
> > Looks like the date for 2.3 is unclear, so it might be
> > ok to merge.
> > 
> > Who's taking this? Luiz? most changes are monitor-related.
> 
> I can take them, but the most important question for me is
> who's reviewing them? I could do it, but not right now.

Any further thoughts on this?  I don't know who would be suitable for
review, beyond those I've already CCed.

-- 
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

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH 0/6] Clean up ISA dependencies so we make ISA optional to build

[Markus Armbruster]

David Gibson <david@gibson.dropbear.id.au> writes:

> On Tue, Mar 10, 2015 at 10:56:22AM -0400, Luiz Capitulino wrote:
>> On Tue, 10 Mar 2015 15:20:29 +0100
>> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>> 
>> > On Fri, Mar 06, 2015 at 03:18:20PM +1100, David Gibson wrote:
>> > > At present, ISA bus support is always included in the build for all
>> > > targets.  However these days there are a number of targets that have
>> > > never had ISA, and even more where many of the individual machines
>> > > don't have ISA.
>> > > 
>> > > Unfortunately there are some awkward dependencies in the core code on
>> > > ISA, although b19c1c0 "isa: remove isa_mem_base variable" did already
>> > > remove one.
>> > > 
>> > > This series engages in some yak shaving to make the necessary
>> > > dependency cleanups, then make inclusion of ISA support optional.
>> > 
>> > For PC/PCI changes
>> > Acked-by: Michael S. Tsirkin <mst@redhat.com>
>> > 
>> > 
>> > > 
>> > > Given the date, this is obviously aimed at qemu 2.4, not 2.3.
>> > 
>> > Looks like the date for 2.3 is unclear, so it might be
>> > ok to merge.
>> > 
>> > Who's taking this? Luiz? most changes are monitor-related.
>> 
>> I can take them, but the most important question for me is
>> who's reviewing them? I could do it, but not right now.
>
> Any further thoughts on this?  I don't know who would be suitable for
> review, beyond those I've already CCed.

You got a full R-by from Alex and a PC/PCI one from Michael.  Should be
enough.

I just had a look, too, and got questions on PATCH 1's completeness, and
the wisdom of extending the "info pic" / "info irq" crap to more
targets.  Nothing seriously wrong, really.

Re: [Qemu-devel] [PATCH 0/6] Clean up ISA dependencies so we make ISA optional to build

[Michael S. Tsirkin]

On Mon, Mar 30, 2015 at 01:41:23PM +1100, David Gibson wrote:
> On Tue, Mar 10, 2015 at 10:56:22AM -0400, Luiz Capitulino wrote:
> > On Tue, 10 Mar 2015 15:20:29 +0100
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > 
> > > On Fri, Mar 06, 2015 at 03:18:20PM +1100, David Gibson wrote:
> > > > At present, ISA bus support is always included in the build for all
> > > > targets.  However these days there are a number of targets that have
> > > > never had ISA, and even more where many of the individual machines
> > > > don't have ISA.
> > > > 
> > > > Unfortunately there are some awkward dependencies in the core code on
> > > > ISA, although b19c1c0 "isa: remove isa_mem_base variable" did already
> > > > remove one.
> > > > 
> > > > This series engages in some yak shaving to make the necessary
> > > > dependency cleanups, then make inclusion of ISA support optional.
> > > 
> > > For PC/PCI changes
> > > Acked-by: Michael S. Tsirkin <mst@redhat.com>
> > > 
> > > 
> > > > 
> > > > Given the date, this is obviously aimed at qemu 2.4, not 2.3.
> > > 
> > > Looks like the date for 2.3 is unclear, so it might be
> > > ok to merge.
> > > 
> > > Who's taking this? Luiz? most changes are monitor-related.
> > 
> > I can take them, but the most important question for me is
> > who's reviewing them? I could do it, but not right now.
> 
> Any further thoughts on this?  I don't know who would be suitable for
> review, beyond those I've already CCed.
> 
> -- 
> 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



I can queue this for 2.4. Can you please repost tweaking comments
as Markus suggested?


-- 
MST