[Qemu-devel] [PATCH 0/4] Allow ISA bus support to be configured out

[Qemu-devel] [PATCH 0/4] Allow ISA bus support to be configured out

[David Gibson]

At the moment isa-bus.c is compiled unconditionally for all targets.
However, some targets have never used legacy ISA devices.  Many more
targets have at least some machine types without ISA.

These patches allow ISA bus to be disabled in the configuration, thus
allowing cut down configurations for targets and machine types that
don't have ISA.

In addition to creating the CONFIG_ISA_BUS config option itself, this
involves breaking some non-obvious dependencies on the ISA code.

David Gibson (4):
  Split serial-isa into its own config option
  Allow ISA bus to be configured out
  Split ISA and sysbus versions of m48t59 device
  Disable "info irq" and "info pic" for target-ppc

 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/isa/Makefile.objs                  |   2 +-
 hw/timer/Makefile.objs                |   3 +
 hw/timer/m48t59-internal.h            |  82 ++++++++++++
 hw/timer/m48t59-isa.c                 | 180 +++++++++++++++++++++++++++
 hw/timer/m48t59.c                     | 228 +++-------------------------------
 monitor.c                             |   2 +-
 25 files changed, 306 insertions(+), 213 deletions(-)
 create mode 100644 hw/timer/m48t59-internal.h
 create mode 100644 hw/timer/m48t59-isa.c

-- 
2.4.3

[Qemu-devel] [PATCH 1/4] 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_SERIAL 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, xtensa and or32.  Those
platforms have technically lost functionality, but since they have no
other PCI or ISA devices, it's fairly clear they never actually used
leagacy 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 74f1db3..abeec21 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 91d602c..bc2fc72 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 44467c3..cccf41b 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 66ed5f9..6fab828 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 bfca2b2..2facfc6 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 0162ef0..8331100 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 4befde3..b5bb29b 100644
--- a/default-configs/ppc-softmmu.mak
+++ b/default-configs/ppc-softmmu.mak
@@ -46,5 +46,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 ab62cc7..2b05329 100644
--- a/default-configs/ppc64-softmmu.mak
+++ b/default-configs/ppc64-softmmu.mak
@@ -50,5 +50,6 @@ CONFIG_LIBDECNUMBER=y
 CONFIG_XICS=$(CONFIG_PSERIES)
 CONFIG_XICS_KVM=$(and $(CONFIG_PSERIES),$(CONFIG_KVM))
 # For PReP
+CONFIG_SERIAL_ISA=y
 CONFIG_MC146818RTC=y
 CONFIG_ISA_TESTDEV=y
diff --git a/default-configs/ppcemb-softmmu.mak b/default-configs/ppcemb-softmmu.mak
index 54acc4d..7f56004 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 62575eb..a9a7238 100644
--- a/default-configs/x86_64-softmmu.mak
+++ b/default-configs/x86_64-softmmu.mak
@@ -10,6 +10,7 @@ CONFIG_VMWARE_VGA=y
 CONFIG_VIRTIO_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 5931cc8..be42d2f 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.4.3

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

[Andreas Färber]

Am 29.06.2015 um 07:36 schrieb 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_SERIAL 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, xtensa and or32.  Those
> platforms have technically lost functionality, but since they have no
> other PCI or ISA devices, it's fairly clear they never actually used
> leagacy IO stuff.

I think we can add moxie to that list, I'd be very surprised if it uses
ISA. Applies also to 2/4. CC'ing the maintainer.

Which arm machines use ISA?

Regards,
Andreas

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

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

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

[David Gibson]

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

On Mon, Jun 29, 2015 at 12:12:01PM +0200, Andreas Färber wrote:
> Am 29.06.2015 um 07:36 schrieb 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_SERIAL 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, xtensa and or32.  Those
> > platforms have technically lost functionality, but since they have no
> > other PCI or ISA devices, it's fairly clear they never actually used
> > leagacy IO stuff.
> 
> I think we can add moxie to that list, I'd be very surprised if it uses
> ISA. Applies also to 2/4. CC'ing the maintainer.

Right.  I'm being deliberately conservative in what I removed here.
With this patch in place it's easy for boards to remove ISA serial
support in future, but that seems like a change best made by the
people who actually know the boards.

> Which arm machines use ISA?

I don't know, but the point was I couldn't quickly convince myself
that none did.

-- 
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 1/4] Split serial-isa into its own config option

[Peter Maydell]

On 29 June 2015 at 06:36, David Gibson <david@gibson.dropbear.id.au> wrote:
> 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_SERIAL 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, xtensa and or32.  Those
> platforms have technically lost functionality, but since they have no
> other PCI or ISA devices, it's fairly clear they never actually used
> leagacy IO stuff.

s/leagacy/legacy/

Is it worth having a default-configs/isa.mak along the
lines of our current pci.mak and usb.mak, for putting
CONFIG_ defines for all ISA devices?

A quick grep suggests you can knock arm, moxie, sh4
and sh4eb off your list of things with CONFIG_SERIAL_ISA
(no hits for ISABus).

I notice in passing that we still have a handful of config
files which set CONFIG_ISA_MMIO, despite that having been
deleted in commit 61fcb628627ea4 two years ago.

thanks
-- PMM

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

[David Gibson]

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

On Mon, Jun 29, 2015 at 11:35:23AM +0100, Peter Maydell wrote:
> On 29 June 2015 at 06:36, David Gibson <david@gibson.dropbear.id.au> wrote:
> > 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_SERIAL 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, xtensa and or32.  Those
> > platforms have technically lost functionality, but since they have no
> > other PCI or ISA devices, it's fairly clear they never actually used
> > leagacy IO stuff.
> 
> s/leagacy/legacy/
> 
> Is it worth having a default-configs/isa.mak along the
> lines of our current pci.mak and usb.mak, for putting
> CONFIG_ defines for all ISA devices?

I considered it, but there were enough variations in exactly what ISA
options were selected in the existing configs for different targets
that it seemed like a substantial distraction from what I'm trying to
accomplish.

> A quick grep suggests you can knock arm, moxie, sh4
> and sh4eb off your list of things with CONFIG_SERIAL_ISA
> (no hits for ISABus).
> 
> I notice in passing that we still have a handful of config
> files which set CONFIG_ISA_MMIO, despite that having been
> deleted in commit 61fcb628627ea4 two years ago.
> 
> thanks
> -- PMM
> 

-- 
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 1/4] Split serial-isa into its own config option

[David Gibson]

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

On Mon, Jun 29, 2015 at 11:35:23AM +0100, Peter Maydell wrote:
> On 29 June 2015 at 06:36, David Gibson <david@gibson.dropbear.id.au> wrote:
> > 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_SERIAL 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, xtensa and or32.  Those
> > platforms have technically lost functionality, but since they have no
> > other PCI or ISA devices, it's fairly clear they never actually used
> > leagacy IO stuff.
> 
> s/leagacy/legacy/
> 
> Is it worth having a default-configs/isa.mak along the
> lines of our current pci.mak and usb.mak, for putting
> CONFIG_ defines for all ISA devices?
> 
> A quick grep suggests you can knock arm, moxie, sh4
> and sh4eb off your list of things with CONFIG_SERIAL_ISA
> (no hits for ISABus).

I'll remove moxie, sh4 and sh4eb in my next spin.

I'm not so comfortable about ARM.  I get the impression that there are
so many x86 people now working in the ARM space, that it wouldn't
surprise me at all if some vendors go and stick legacy stuff on
there.  So I'd prefer to leave it to the ARM people themselves to turn
it off if I'm wrong.

> I notice in passing that we still have a handful of config
> files which set CONFIG_ISA_MMIO, despite that having been
> deleted in commit 61fcb628627ea4 two years ago.

Hm, I'll add a patch to clean that up, why not.

-- 
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 1/4] Split serial-isa into its own config option

[Peter Maydell]

On 1 July 2015 at 05:20, David Gibson <david@gibson.dropbear.id.au> wrote:
> On Mon, Jun 29, 2015 at 11:35:23AM +0100, Peter Maydell wrote:
>> A quick grep suggests you can knock arm, moxie, sh4
>> and sh4eb off your list of things with CONFIG_SERIAL_ISA
>> (no hits for ISABus).
>
> I'll remove moxie, sh4 and sh4eb in my next spin.
>
> I'm not so comfortable about ARM.  I get the impression that there are
> so many x86 people now working in the ARM space, that it wouldn't
> surprise me at all if some vendors go and stick legacy stuff on
> there.  So I'd prefer to leave it to the ARM people themselves to turn
> it off if I'm wrong.

I'm an ARM person :-)

I'm happy to turn ISA off for ARM, except for the case of
generic PCI-to-ISA devices which IIRC you're handling by
having pci.mak set CONFIG_ISA ?

(In practice that means ARM will still compile in the ISA
code, but hey.)

-- PMM

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

[David Gibson]

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

On Wed, Jul 01, 2015 at 12:15:09PM +0100, Peter Maydell wrote:
> On 1 July 2015 at 05:20, David Gibson <david@gibson.dropbear.id.au> wrote:
> > On Mon, Jun 29, 2015 at 11:35:23AM +0100, Peter Maydell wrote:
> >> A quick grep suggests you can knock arm, moxie, sh4
> >> and sh4eb off your list of things with CONFIG_SERIAL_ISA
> >> (no hits for ISABus).
> >
> > I'll remove moxie, sh4 and sh4eb in my next spin.
> >
> > I'm not so comfortable about ARM.  I get the impression that there are
> > so many x86 people now working in the ARM space, that it wouldn't
> > surprise me at all if some vendors go and stick legacy stuff on
> > there.  So I'd prefer to leave it to the ARM people themselves to turn
> > it off if I'm wrong.
> 
> I'm an ARM person :-)
> 
> I'm happy to turn ISA off for ARM, except for the case of
> generic PCI-to-ISA devices which IIRC you're handling by
> having pci.mak set CONFIG_ISA ?
> 
> (In practice that means ARM will still compile in the ISA
> code, but hey.)

So that makes sense for the patch in this series which allows
deconfiguration of the ISA bus itself.  But this patch is just about
ISA serial ports - given that ARM does have ISA for now, and will
still be able to with a bridge it seems odd to disable ISA serial
specifically.

Come to think of it, the same applies to SH4, since it has PCI. So
I've put SERIAL_ISA back in for SH4, and updated the commit message to
explain that for now I'm only taking out SERIAL_ISA support for things
that clearly have neither ISA nor PCI support.

-- 
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/4] 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 7e10903..9f2b98c 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.4.3

Re: [Qemu-devel] [PATCH 2/4] Allow ISA bus to be configured out

[Andreas Färber]

Am 29.06.2015 um 07:36 schrieb 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.

PCI also allows to have a PCI-ISA bridge such as the i82378, which means
that all PCI platforms can potentially obtain an actual ISA bus. So at
least the commit message could use a makeover to avoid someone touching
IDE to blindly disable the dependency.

You'd also need to think about the qtests then:

$ git grep i82378 -- tests/
tests/endianness-test.c:    { "ppc", "g3beige", 0xfe000000, .bswap =
true, .superio = "i82378" },
tests/endianness-test.c:    { "ppc", "bamboo", 0xe8000000, .bswap =
true, .superio = "i82378" },
tests/endianness-test.c:    { "ppc64", "mac99", 0xf2000000, .bswap =
true, .superio = "i82378" },
tests/endianness-test.c:      .bswap = true, .superio = "i82378" },
tests/endianness-test.c:    { "sh4", "r2d", 0xfe240000, .superio =
"i82378" },
tests/endianness-test.c:    { "sh4eb", "r2d", 0xfe240000, .bswap = true,
.superio = "i82378" },

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

I guess we can drop it for moxie ...

> diff --git a/default-configs/pci.mak b/default-configs/pci.mak
> index 7e10903..9f2b98c 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

... and for unicore32?

Regards,
Andreas

> 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

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

Re: [Qemu-devel] [PATCH 2/4] Allow ISA bus to be configured out

[David Gibson]

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

On Mon, Jun 29, 2015 at 12:26:50PM +0200, Andreas Färber wrote:
> Am 29.06.2015 um 07:36 schrieb 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.
> 
> PCI also allows to have a PCI-ISA bridge such as the i82378, which means
> that all PCI platforms can potentially obtain an actual ISA bus. So at
> least the commit message could use a makeover to avoid someone touching
> IDE to blindly disable the dependency.

Sorry, I'm not quite following what you're getting at here.

> You'd also need to think about the qtests then:
> 
> $ git grep i82378 -- tests/
> tests/endianness-test.c:    { "ppc", "g3beige", 0xfe000000, .bswap =
> true, .superio = "i82378" },
> tests/endianness-test.c:    { "ppc", "bamboo", 0xe8000000, .bswap =
> true, .superio = "i82378" },
> tests/endianness-test.c:    { "ppc64", "mac99", 0xf2000000, .bswap =
> true, .superio = "i82378" },
> tests/endianness-test.c:      .bswap = true, .superio = "i82378" },
> tests/endianness-test.c:    { "sh4", "r2d", 0xfe240000, .superio =
> "i82378" },
> tests/endianness-test.c:    { "sh4eb", "r2d", 0xfe240000, .bswap = true,
> .superio = "i82378" },

Ah, good point.  I'll need to rework for that,

-- 
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/4] Allow ISA bus to be configured out

[David Gibson]

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

On Tue, Jun 30, 2015 at 01:44:10PM +1000, David Gibson wrote:
> On Mon, Jun 29, 2015 at 12:26:50PM +0200, Andreas Färber wrote:
> > Am 29.06.2015 um 07:36 schrieb 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.
> > 
> > PCI also allows to have a PCI-ISA bridge such as the i82378, which means
> > that all PCI platforms can potentially obtain an actual ISA bus. So at
> > least the commit message could use a makeover to avoid someone touching
> > IDE to blindly disable the dependency.
> 
> Sorry, I'm not quite following what you're getting at here.
> 
> > You'd also need to think about the qtests then:
> > 
> > $ git grep i82378 -- tests/
> > tests/endianness-test.c:    { "ppc", "g3beige", 0xfe000000, .bswap =
> > true, .superio = "i82378" },
> > tests/endianness-test.c:    { "ppc", "bamboo", 0xe8000000, .bswap =
> > true, .superio = "i82378" },
> > tests/endianness-test.c:    { "ppc64", "mac99", 0xf2000000, .bswap =
> > true, .superio = "i82378" },
> > tests/endianness-test.c:      .bswap = true, .superio = "i82378" },
> > tests/endianness-test.c:    { "sh4", "r2d", 0xfe240000, .superio =
> > "i82378" },
> > tests/endianness-test.c:    { "sh4eb", "r2d", 0xfe240000, .bswap = true,
> > .superio = "i82378" },
> 
> Ah, good point.  I'll need to rework for that,

Actually.. on second thoughts..

CONFIG_I82378 already exists, and will break those tests of disabled.

So while making the tests more robust against config changes would be
a good thing in general, I don't think it's in the scope of what I'm
trying to do here - making ISA configurable won't make these tests any
more broken with nonstandard configs than they already are.

-- 
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/4] Allow ISA bus to be configured out

[Andreas Färber]

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

Am 30.06.2015 um 06:48 schrieb David Gibson:
> On Tue, Jun 30, 2015 at 01:44:10PM +1000, David Gibson wrote:
>> On Mon, Jun 29, 2015 at 12:26:50PM +0200, Andreas Färber wrote:
>>> Am 29.06.2015 um 07:36 schrieb 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.
>>>
>>> PCI also allows to have a PCI-ISA bridge such as the i82378, which means
>>> that all PCI platforms can potentially obtain an actual ISA bus. So at
>>> least the commit message could use a makeover to avoid someone touching
>>> IDE to blindly disable the dependency.
>>
>> Sorry, I'm not quite following what you're getting at here.
>>
>>> You'd also need to think about the qtests then:
>>>
>>> $ git grep i82378 -- tests/
>>> tests/endianness-test.c:    { "ppc", "g3beige", 0xfe000000, .bswap =
>>> true, .superio = "i82378" },
>>> tests/endianness-test.c:    { "ppc", "bamboo", 0xe8000000, .bswap =
>>> true, .superio = "i82378" },
>>> tests/endianness-test.c:    { "ppc64", "mac99", 0xf2000000, .bswap =
>>> true, .superio = "i82378" },
>>> tests/endianness-test.c:      .bswap = true, .superio = "i82378" },
>>> tests/endianness-test.c:    { "sh4", "r2d", 0xfe240000, .superio =
>>> "i82378" },
>>> tests/endianness-test.c:    { "sh4eb", "r2d", 0xfe240000, .bswap = true,
>>> .superio = "i82378" },
>>
>> Ah, good point.  I'll need to rework for that,
> 
> Actually.. on second thoughts..
> 
> CONFIG_I82378 already exists, and will break those tests of disabled.
> 
> So while making the tests more robust against config changes would be
> a good thing in general, I don't think it's in the scope of what I'm
> trying to do here - making ISA configurable won't make these tests any
> more broken with nonstandard configs than they already are.

I am mainly saying that your commit message is misleading.

CONFIG_IDE_CORE is not the only reason that pci.mak needs
CONFIG_ISA_BUS=y, as you make it sound. The other problem is that we
don't have Kconfig yet, so we have no way of modeling that I82378
depends on ISA_BUS, therefore either pci.mak (as done here) or
ppc-softmmu.mak and ppc64-softmmu.mak need CONFIG_ISA_BUS=y. And due to
the qtest either sh4/sh4eb need that same dependency too, or the test
needs to be changed.

So please either revise the commit message when you resend, or you'll
need to repeat CONFIG_ISA_BUS=y for ppc, ppc64, sh4, sh4eb.

Regards,
Andreas

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


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

[Qemu-devel] [PATCH 3/4] Split ISA and sysbus versions of m48t59 device

[David Gibson]

The m48t59 device supports both ISA and direct sysbus attached versions of
the device in the one .c file.  This can be awkward for some embedded
machine types which need the sysbus M48T59, but don't want to pull in the
ISA bus code and its other dependencies.

Therefore, this patch splits out the code for the ISA attached M48T59 into
its own C file.  It will be built when both CONFIG_M48T59 and
CONFIG_ISA_BUS are enabled.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/timer/Makefile.objs     |   3 +
 hw/timer/m48t59-internal.h |  82 ++++++++++++++++
 hw/timer/m48t59-isa.c      | 180 +++++++++++++++++++++++++++++++++++
 hw/timer/m48t59.c          | 228 ++++-----------------------------------------
 4 files changed, 283 insertions(+), 210 deletions(-)
 create mode 100644 hw/timer/m48t59-internal.h
 create mode 100644 hw/timer/m48t59-isa.c

diff --git a/hw/timer/Makefile.objs b/hw/timer/Makefile.objs
index 133bd0d..803fde7 100644
--- a/hw/timer/Makefile.objs
+++ b/hw/timer/Makefile.objs
@@ -6,6 +6,9 @@ common-obj-$(CONFIG_DS1338) += ds1338.o
 common-obj-$(CONFIG_HPET) += hpet.o
 common-obj-$(CONFIG_I8254) += i8254_common.o i8254.o
 common-obj-$(CONFIG_M48T59) += m48t59.o
+ifeq ($(CONFIG_ISA_BUS),y)
+common-obj-$(CONFIG_M48T59) += m48t59-isa.o
+endif
 common-obj-$(CONFIG_PL031) += pl031.o
 common-obj-$(CONFIG_PUV3) += puv3_ost.o
 common-obj-$(CONFIG_TWL92230) += twl92230.o
diff --git a/hw/timer/m48t59-internal.h b/hw/timer/m48t59-internal.h
new file mode 100644
index 0000000..c50f134
--- /dev/null
+++ b/hw/timer/m48t59-internal.h
@@ -0,0 +1,82 @@
+/*
+ * QEMU M48T59 and M48T08 NVRAM emulation (common header)
+ *
+ * Copyright (c) 2003-2005, 2007 Jocelyn Mayer
+ * Copyright (c) 2013 Hervé Poussineau
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+#ifndef HW_M48T59_H
+#define HW_M48T59_H 1
+
+//#define DEBUG_NVRAM
+
+#if defined(DEBUG_NVRAM)
+#define NVRAM_PRINTF(fmt, ...) do { printf(fmt , ## __VA_ARGS__); } while (0)
+#else
+#define NVRAM_PRINTF(fmt, ...) do { } while (0)
+#endif
+
+/*
+ * The M48T02, M48T08 and M48T59 chips are very similar. The newer '59 has
+ * alarm and a watchdog timer and related control registers. In the
+ * PPC platform there is also a nvram lock function.
+ */
+
+typedef struct M48txxInfo {
+    const char *bus_name;
+    uint32_t model; /* 2 = m48t02, 8 = m48t08, 59 = m48t59 */
+    uint32_t size;
+} M48txxInfo;
+
+typedef struct M48t59State {
+    /* Hardware parameters */
+    qemu_irq IRQ;
+    MemoryRegion iomem;
+    uint32_t size;
+    int32_t base_year;
+    /* RTC management */
+    time_t   time_offset;
+    time_t   stop_time;
+    /* Alarm & watchdog */
+    struct tm alarm;
+    QEMUTimer *alrm_timer;
+    QEMUTimer *wd_timer;
+    /* NVRAM storage */
+    uint8_t *buffer;
+    /* Model parameters */
+    uint32_t model; /* 2 = m48t02, 8 = m48t08, 59 = m48t59 */
+    /* NVRAM storage */
+    uint16_t addr;
+    uint8_t  lock;
+} M48t59State;
+
+uint32_t m48t59_read(M48t59State *NVRAM, uint32_t addr);
+void m48t59_write(M48t59State *NVRAM, uint32_t addr, uint32_t val);
+void m48t59_reset_common(M48t59State *NVRAM);
+void m48t59_realize_common(M48t59State *s, Error **errp);
+
+static inline void m48t59_toggle_lock(M48t59State *NVRAM, int lock)
+{
+    NVRAM->lock ^= 1 << lock;
+}
+
+extern const MemoryRegionOps m48t59_io_ops;
+
+#endif /* HW_M48T59_H */
diff --git a/hw/timer/m48t59-isa.c b/hw/timer/m48t59-isa.c
new file mode 100644
index 0000000..3a521dc
--- /dev/null
+++ b/hw/timer/m48t59-isa.c
@@ -0,0 +1,180 @@
+/*
+ * QEMU M48T59 and M48T08 NVRAM emulation (ISA bus interface
+ *
+ * Copyright (c) 2003-2005, 2007 Jocelyn Mayer
+ * Copyright (c) 2013 Hervé Poussineau
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+#include "hw/isa/isa.h"
+#include "hw/timer/m48t59.h"
+#include "m48t59-internal.h"
+
+#define TYPE_M48TXX_ISA "isa-m48txx"
+#define M48TXX_ISA_GET_CLASS(obj) \
+    OBJECT_GET_CLASS(M48txxISADeviceClass, (obj), TYPE_M48TXX_ISA)
+#define M48TXX_ISA_CLASS(klass) \
+    OBJECT_CLASS_CHECK(M48txxISADeviceClass, (klass), TYPE_M48TXX_ISA)
+#define M48TXX_ISA(obj) \
+    OBJECT_CHECK(M48txxISAState, (obj), TYPE_M48TXX_ISA)
+
+typedef struct M48txxISAState {
+    ISADevice parent_obj;
+    M48t59State state;
+    uint32_t io_base;
+    MemoryRegion io;
+} M48txxISAState;
+
+typedef struct M48txxISADeviceClass {
+    ISADeviceClass parent_class;
+    M48txxInfo info;
+} M48txxISADeviceClass;
+
+static M48txxInfo m48txx_isa_info[] = {
+    {
+        .bus_name = "isa-m48t59",
+        .model = 59,
+        .size = 0x2000,
+    }
+};
+
+Nvram *m48t59_init_isa(ISABus *bus, uint32_t io_base, uint16_t size,
+                       int base_year, int model)
+{
+    DeviceState *dev;
+    int i;
+
+    for (i = 0; i < ARRAY_SIZE(m48txx_isa_info); i++) {
+        if (m48txx_isa_info[i].size != size ||
+            m48txx_isa_info[i].model != model) {
+            continue;
+        }
+
+        dev = DEVICE(isa_create(bus, m48txx_isa_info[i].bus_name));
+        qdev_prop_set_uint32(dev, "iobase", io_base);
+        qdev_prop_set_int32(dev, "base-year", base_year);
+        qdev_init_nofail(dev);
+        return NVRAM(dev);
+    }
+
+    assert(false);
+    return NULL;
+}
+
+static uint32_t m48txx_isa_read(Nvram *obj, uint32_t addr)
+{
+    M48txxISAState *d = M48TXX_ISA(obj);
+    return m48t59_read(&d->state, addr);
+}
+
+static void m48txx_isa_write(Nvram *obj, uint32_t addr, uint32_t val)
+{
+    M48txxISAState *d = M48TXX_ISA(obj);
+    m48t59_write(&d->state, addr, val);
+}
+
+static void m48txx_isa_toggle_lock(Nvram *obj, int lock)
+{
+    M48txxISAState *d = M48TXX_ISA(obj);
+    m48t59_toggle_lock(&d->state, lock);
+}
+
+static Property m48t59_isa_properties[] = {
+    DEFINE_PROP_INT32("base-year", M48txxISAState, state.base_year, 0),
+    DEFINE_PROP_UINT32("iobase", M48txxISAState, io_base, 0x74),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static void m48t59_reset_isa(DeviceState *d)
+{
+    M48txxISAState *isa = M48TXX_ISA(d);
+    M48t59State *NVRAM = &isa->state;
+
+    m48t59_reset_common(NVRAM);
+}
+
+static void m48t59_isa_realize(DeviceState *dev, Error **errp)
+{
+    M48txxISADeviceClass *u = M48TXX_ISA_GET_CLASS(dev);
+    ISADevice *isadev = ISA_DEVICE(dev);
+    M48txxISAState *d = M48TXX_ISA(dev);
+    M48t59State *s = &d->state;
+
+    s->model = u->info.model;
+    s->size = u->info.size;
+    isa_init_irq(isadev, &s->IRQ, 8);
+    m48t59_realize_common(s, errp);
+    memory_region_init_io(&d->io, OBJECT(dev), &m48t59_io_ops, s, "m48t59", 4);
+    if (d->io_base != 0) {
+        isa_register_ioport(isadev, &d->io, d->io_base);
+    }
+}
+
+static void m48txx_isa_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    NvramClass *nc = NVRAM_CLASS(klass);
+
+    dc->realize = m48t59_isa_realize;
+    dc->reset = m48t59_reset_isa;
+    dc->props = m48t59_isa_properties;
+    nc->read = m48txx_isa_read;
+    nc->write = m48txx_isa_write;
+    nc->toggle_lock = m48txx_isa_toggle_lock;
+}
+
+static void m48txx_isa_concrete_class_init(ObjectClass *klass, void *data)
+{
+    M48txxISADeviceClass *u = M48TXX_ISA_CLASS(klass);
+    M48txxInfo *info = data;
+
+    u->info = *info;
+}
+
+static const TypeInfo m48txx_isa_type_info = {
+    .name = TYPE_M48TXX_ISA,
+    .parent = TYPE_ISA_DEVICE,
+    .instance_size = sizeof(M48txxISAState),
+    .abstract = true,
+    .class_init = m48txx_isa_class_init,
+    .interfaces = (InterfaceInfo[]) {
+        { TYPE_NVRAM },
+        { }
+    }
+};
+
+static void m48t59_isa_register_types(void)
+{
+    TypeInfo isa_type_info = {
+        .parent = TYPE_M48TXX_ISA,
+        .class_size = sizeof(M48txxISADeviceClass),
+        .class_init = m48txx_isa_concrete_class_init,
+    };
+    int i;
+
+    type_register_static(&m48txx_isa_type_info);
+
+    for (i = 0; i < ARRAY_SIZE(m48txx_isa_info); i++) {
+        isa_type_info.name = m48txx_isa_info[i].bus_name;
+        isa_type_info.class_data = &m48txx_isa_info[i];
+        type_register(&isa_type_info);
+    }
+}
+
+type_init(m48t59_isa_register_types)
diff --git a/hw/timer/m48t59.c b/hw/timer/m48t59.c
index 8ab683d..ac90734 100644
--- a/hw/timer/m48t59.c
+++ b/hw/timer/m48t59.c
@@ -27,16 +27,9 @@
 #include "qemu/timer.h"
 #include "sysemu/sysemu.h"
 #include "hw/sysbus.h"
-#include "hw/isa/isa.h"
 #include "exec/address-spaces.h"
 
-//#define DEBUG_NVRAM
-
-#if defined(DEBUG_NVRAM)
-#define NVRAM_PRINTF(fmt, ...) do { printf(fmt , ## __VA_ARGS__); } while (0)
-#else
-#define NVRAM_PRINTF(fmt, ...) do { } while (0)
-#endif
+#include "m48t59-internal.h"
 
 #define TYPE_M48TXX_SYS_BUS "sysbus-m48txx"
 #define M48TXX_SYS_BUS_GET_CLASS(obj) \
@@ -46,27 +39,6 @@
 #define M48TXX_SYS_BUS(obj) \
     OBJECT_CHECK(M48txxSysBusState, (obj), TYPE_M48TXX_SYS_BUS)
 
-#define TYPE_M48TXX_ISA "isa-m48txx"
-#define M48TXX_ISA_GET_CLASS(obj) \
-    OBJECT_GET_CLASS(M48txxISADeviceClass, (obj), TYPE_M48TXX_ISA)
-#define M48TXX_ISA_CLASS(klass) \
-    OBJECT_CLASS_CHECK(M48txxISADeviceClass, (klass), TYPE_M48TXX_ISA)
-#define M48TXX_ISA(obj) \
-    OBJECT_CHECK(M48txxISAState, (obj), TYPE_M48TXX_ISA)
-
-/*
- * The M48T02, M48T08 and M48T59 chips are very similar. The newer '59 has
- * alarm and a watchdog timer and related control registers. In the
- * PPC platform there is also a nvram lock function.
- */
-
-typedef struct M48txxInfo {
-    const char *isa_name;
-    const char *sysbus_name;
-    uint32_t model; /* 2 = m48t02, 8 = m48t08, 59 = m48t59 */
-    uint32_t size;
-} M48txxInfo;
-
 /*
  * Chipset docs:
  * http://www.st.com/stonline/products/literature/ds/2410/m48t02.pdf
@@ -74,40 +46,6 @@ typedef struct M48txxInfo {
  * http://www.st.com/stonline/products/literature/od/7001/m48t59y.pdf
  */
 
-typedef struct M48t59State {
-    /* Hardware parameters */
-    qemu_irq IRQ;
-    MemoryRegion iomem;
-    uint32_t size;
-    int32_t base_year;
-    /* RTC management */
-    time_t   time_offset;
-    time_t   stop_time;
-    /* Alarm & watchdog */
-    struct tm alarm;
-    QEMUTimer *alrm_timer;
-    QEMUTimer *wd_timer;
-    /* NVRAM storage */
-    uint8_t *buffer;
-    /* Model parameters */
-    uint32_t model; /* 2 = m48t02, 8 = m48t08, 59 = m48t59 */
-    /* NVRAM storage */
-    uint16_t addr;
-    uint8_t  lock;
-} M48t59State;
-
-typedef struct M48txxISAState {
-    ISADevice parent_obj;
-    M48t59State state;
-    uint32_t io_base;
-    MemoryRegion io;
-} M48txxISAState;
-
-typedef struct M48txxISADeviceClass {
-    ISADeviceClass parent_class;
-    M48txxInfo info;
-} M48txxISADeviceClass;
-
 typedef struct M48txxSysBusState {
     SysBusDevice parent_obj;
     M48t59State state;
@@ -119,21 +57,17 @@ typedef struct M48txxSysBusDeviceClass {
     M48txxInfo info;
 } M48txxSysBusDeviceClass;
 
-static M48txxInfo m48txx_info[] = {
+static M48txxInfo m48txx_sysbus_info[] = {
     {
-        .sysbus_name = "sysbus-m48t02",
+        .bus_name = "sysbus-m48t02",
         .model = 2,
         .size = 0x800,
     },{
-        .sysbus_name = "sysbus-m48t08",
+        .bus_name = "sysbus-m48t08",
         .model = 8,
         .size = 0x2000,
     },{
-        .sysbus_name = "sysbus-m48t59",
-        .model = 59,
-        .size = 0x2000,
-    },{
-        .isa_name = "isa-m48t59",
+        .bus_name = "sysbus-m48t59",
         .model = 59,
         .size = 0x2000,
     }
@@ -245,7 +179,7 @@ static void set_up_watchdog(M48t59State *NVRAM, uint8_t value)
 }
 
 /* Direct access to NVRAM */
-static void m48t59_write(M48t59State *NVRAM, uint32_t addr, uint32_t val)
+void m48t59_write(M48t59State *NVRAM, uint32_t addr, uint32_t val)
 {
     struct tm tm;
     int tmp;
@@ -410,7 +344,7 @@ static void m48t59_write(M48t59State *NVRAM, uint32_t addr, uint32_t val)
     }
 }
 
-static uint32_t m48t59_read(M48t59State *NVRAM, uint32_t addr)
+uint32_t m48t59_read(M48t59State *NVRAM, uint32_t addr)
 {
     struct tm tm;
     uint32_t retval = 0xFF;
@@ -514,11 +448,6 @@ static uint32_t m48t59_read(M48t59State *NVRAM, uint32_t addr)
     return retval;
 }
 
-static void m48t59_toggle_lock(M48t59State *NVRAM, int lock)
-{
-    NVRAM->lock ^= 1 << lock;
-}
-
 /* IO access to NVRAM */
 static void NVRAM_writeb(void *opaque, hwaddr addr, uint64_t val,
                          unsigned size)
@@ -638,7 +567,7 @@ static const VMStateDescription vmstate_m48t59 = {
     }
 };
 
-static void m48t59_reset_common(M48t59State *NVRAM)
+void m48t59_reset_common(M48t59State *NVRAM)
 {
     NVRAM->addr = 0;
     NVRAM->lock = 0;
@@ -649,14 +578,6 @@ static void m48t59_reset_common(M48t59State *NVRAM)
         timer_del(NVRAM->wd_timer);
 }
 
-static void m48t59_reset_isa(DeviceState *d)
-{
-    M48txxISAState *isa = M48TXX_ISA(d);
-    M48t59State *NVRAM = &isa->state;
-
-    m48t59_reset_common(NVRAM);
-}
-
 static void m48t59_reset_sysbus(DeviceState *d)
 {
     M48txxSysBusState *sys = M48TXX_SYS_BUS(d);
@@ -665,7 +586,7 @@ static void m48t59_reset_sysbus(DeviceState *d)
     m48t59_reset_common(NVRAM);
 }
 
-static const MemoryRegionOps m48t59_io_ops = {
+const MemoryRegionOps m48t59_io_ops = {
     .read = NVRAM_readb,
     .write = NVRAM_writeb,
     .impl = {
@@ -684,14 +605,13 @@ Nvram *m48t59_init(qemu_irq IRQ, hwaddr mem_base,
     SysBusDevice *s;
     int i;
 
-    for (i = 0; i < ARRAY_SIZE(m48txx_info); i++) {
-        if (!m48txx_info[i].sysbus_name ||
-            m48txx_info[i].size != size ||
-            m48txx_info[i].model != model) {
+    for (i = 0; i < ARRAY_SIZE(m48txx_sysbus_info); i++) {
+        if (m48txx_sysbus_info[i].size != size ||
+            m48txx_sysbus_info[i].model != model) {
             continue;
         }
 
-        dev = qdev_create(NULL, m48txx_info[i].sysbus_name);
+        dev = qdev_create(NULL, m48txx_sysbus_info[i].bus_name);
         qdev_prop_set_int32(dev, "base-year", base_year);
         qdev_init_nofail(dev);
         s = SYS_BUS_DEVICE(dev);
@@ -711,31 +631,7 @@ Nvram *m48t59_init(qemu_irq IRQ, hwaddr mem_base,
     return NULL;
 }
 
-Nvram *m48t59_init_isa(ISABus *bus, uint32_t io_base, uint16_t size,
-                       int base_year, int model)
-{
-    DeviceState *dev;
-    int i;
-
-    for (i = 0; i < ARRAY_SIZE(m48txx_info); i++) {
-        if (!m48txx_info[i].isa_name ||
-            m48txx_info[i].size != size ||
-            m48txx_info[i].model != model) {
-            continue;
-        }
-
-        dev = DEVICE(isa_create(bus, m48txx_info[i].isa_name));
-        qdev_prop_set_uint32(dev, "iobase", io_base);
-        qdev_prop_set_int32(dev, "base-year", base_year);
-        qdev_init_nofail(dev);
-        return NVRAM(dev);
-    }
-
-    assert(false);
-    return NULL;
-}
-
-static void m48t59_realize_common(M48t59State *s, Error **errp)
+void m48t59_realize_common(M48t59State *s, Error **errp)
 {
     s->buffer = g_malloc0(s->size);
     if (s->model == 59) {
@@ -747,23 +643,6 @@ static void m48t59_realize_common(M48t59State *s, Error **errp)
     vmstate_register(NULL, -1, &vmstate_m48t59, s);
 }
 
-static void m48t59_isa_realize(DeviceState *dev, Error **errp)
-{
-    M48txxISADeviceClass *u = M48TXX_ISA_GET_CLASS(dev);
-    ISADevice *isadev = ISA_DEVICE(dev);
-    M48txxISAState *d = M48TXX_ISA(dev);
-    M48t59State *s = &d->state;
-
-    s->model = u->info.model;
-    s->size = u->info.size;
-    isa_init_irq(isadev, &s->IRQ, 8);
-    m48t59_realize_common(s, errp);
-    memory_region_init_io(&d->io, OBJECT(dev), &m48t59_io_ops, s, "m48t59", 4);
-    if (d->io_base != 0) {
-        isa_register_ioport(isadev, &d->io, d->io_base);
-    }
-}
-
 static int m48t59_init1(SysBusDevice *dev)
 {
     M48txxSysBusDeviceClass *u = M48TXX_SYS_BUS_GET_CLASS(dev);
@@ -790,51 +669,6 @@ static int m48t59_init1(SysBusDevice *dev)
     return 0;
 }
 
-static uint32_t m48txx_isa_read(Nvram *obj, uint32_t addr)
-{
-    M48txxISAState *d = M48TXX_ISA(obj);
-    return m48t59_read(&d->state, addr);
-}
-
-static void m48txx_isa_write(Nvram *obj, uint32_t addr, uint32_t val)
-{
-    M48txxISAState *d = M48TXX_ISA(obj);
-    m48t59_write(&d->state, addr, val);
-}
-
-static void m48txx_isa_toggle_lock(Nvram *obj, int lock)
-{
-    M48txxISAState *d = M48TXX_ISA(obj);
-    m48t59_toggle_lock(&d->state, lock);
-}
-
-static Property m48t59_isa_properties[] = {
-    DEFINE_PROP_INT32("base-year", M48txxISAState, state.base_year, 0),
-    DEFINE_PROP_UINT32("iobase", M48txxISAState, io_base, 0x74),
-    DEFINE_PROP_END_OF_LIST(),
-};
-
-static void m48txx_isa_class_init(ObjectClass *klass, void *data)
-{
-    DeviceClass *dc = DEVICE_CLASS(klass);
-    NvramClass *nc = NVRAM_CLASS(klass);
-
-    dc->realize = m48t59_isa_realize;
-    dc->reset = m48t59_reset_isa;
-    dc->props = m48t59_isa_properties;
-    nc->read = m48txx_isa_read;
-    nc->write = m48txx_isa_write;
-    nc->toggle_lock = m48txx_isa_toggle_lock;
-}
-
-static void m48txx_isa_concrete_class_init(ObjectClass *klass, void *data)
-{
-    M48txxISADeviceClass *u = M48TXX_ISA_CLASS(klass);
-    M48txxInfo *info = data;
-
-    u->info = *info;
-}
-
 static uint32_t m48txx_sysbus_read(Nvram *obj, uint32_t addr)
 {
     M48txxSysBusState *d = M48TXX_SYS_BUS(obj);
@@ -898,18 +732,6 @@ static const TypeInfo m48txx_sysbus_type_info = {
     }
 };
 
-static const TypeInfo m48txx_isa_type_info = {
-    .name = TYPE_M48TXX_ISA,
-    .parent = TYPE_ISA_DEVICE,
-    .instance_size = sizeof(M48txxISAState),
-    .abstract = true,
-    .class_init = m48txx_isa_class_init,
-    .interfaces = (InterfaceInfo[]) {
-        { TYPE_NVRAM },
-        { }
-    }
-};
-
 static void m48t59_register_types(void)
 {
     TypeInfo sysbus_type_info = {
@@ -917,29 +739,15 @@ static void m48t59_register_types(void)
         .class_size = sizeof(M48txxSysBusDeviceClass),
         .class_init = m48txx_sysbus_concrete_class_init,
     };
-    TypeInfo isa_type_info = {
-        .parent = TYPE_M48TXX_ISA,
-        .class_size = sizeof(M48txxISADeviceClass),
-        .class_init = m48txx_isa_concrete_class_init,
-    };
     int i;
 
     type_register_static(&nvram_info);
     type_register_static(&m48txx_sysbus_type_info);
-    type_register_static(&m48txx_isa_type_info);
 
-    for (i = 0; i < ARRAY_SIZE(m48txx_info); i++) {
-        if (m48txx_info[i].sysbus_name) {
-            sysbus_type_info.name = m48txx_info[i].sysbus_name;
-            sysbus_type_info.class_data = &m48txx_info[i];
-            type_register(&sysbus_type_info);
-        }
-
-        if (m48txx_info[i].isa_name) {
-            isa_type_info.name = m48txx_info[i].isa_name;
-            isa_type_info.class_data = &m48txx_info[i];
-            type_register(&isa_type_info);
-        }
+    for (i = 0; i < ARRAY_SIZE(m48txx_sysbus_info); i++) {
+        sysbus_type_info.name = m48txx_sysbus_info[i].bus_name;
+        sysbus_type_info.class_data = &m48txx_sysbus_info[i];
+        type_register(&sysbus_type_info);
     }
 }
 
-- 
2.4.3

[Qemu-devel] [PATCH 4/4] Disable "info irq" and "info pic" for target-ppc

[David Gibson]

The "info irq" and "info pic" HMP commands are available on some, but not
all targets, and what they do isn't terribly consistent.  For SPARC and
LM32 they do something platform specific, but for x86, powerpc, and MIPS
they print some information from the i8259 (and only the i8259) interrupt
controller.

It's debatable whether these commands are any use at all, and we should
probably make better, qdev aware ways of getting information from a
machines PICs.  However, those don't exist yet, so on x86 it's at least
potentially useful to have these HMP commands.  I can't speak for MIPS.

For ppc, though, the i8259, if it exists at all, is usually just a
secondary controller for legacy ISA.  The only case where i8259 is the
main system PIC on ppc is for the ancient and little-used PReP platform.

So, even without QOM-ish replacement, the info pic and info irq HMP
commands have no value on ppc.

This patch, therefore, disables these commands for ppc targets.  This will
allow ppc builds which don't include PReP to not include ISA bus support
either.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 monitor.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/monitor.c b/monitor.c
index aeea2b5..8c56bfa 100644
--- a/monitor.c
+++ b/monitor.c
@@ -2573,7 +2573,7 @@ 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) || \
+#if defined(TARGET_I386) || defined(TARGET_MIPS) || \
     defined(TARGET_LM32) || (defined(TARGET_SPARC) && !defined(TARGET_SPARC64))
     {
         .name       = "irq",
-- 
2.4.3

Re: [Qemu-devel] [PATCH 4/4] Disable "info irq" and "info pic" for target-ppc

[Laurent Vivier]

On 29/06/2015 07:36, David Gibson wrote:
> The "info irq" and "info pic" HMP commands are available on some, but not
> all targets, and what they do isn't terribly consistent.  For SPARC and
> LM32 they do something platform specific, but for x86, powerpc, and MIPS
> they print some information from the i8259 (and only the i8259) interrupt
> controller.
> 
> It's debatable whether these commands are any use at all, and we should
> probably make better, qdev aware ways of getting information from a
> machines PICs.  However, those don't exist yet, so on x86 it's at least
> potentially useful to have these HMP commands.  I can't speak for MIPS.
> 
> For ppc, though, the i8259, if it exists at all, is usually just a
> secondary controller for legacy ISA.  The only case where i8259 is the
> main system PIC on ppc is for the ancient and little-used PReP platform.
> 
> So, even without QOM-ish replacement, the info pic and info irq HMP
> commands have no value on ppc.
> 
> This patch, therefore, disables these commands for ppc targets.  This will
> allow ppc builds which don't include PReP to not include ISA bus support
> either.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  monitor.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/monitor.c b/monitor.c
> index aeea2b5..8c56bfa 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -2573,7 +2573,7 @@ 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) || \
> +#if defined(TARGET_I386) || defined(TARGET_MIPS) || \
>      defined(TARGET_LM32) || (defined(TARGET_SPARC) && !defined(TARGET_SPARC64))
>      {
>          .name       = "irq",
> 

Perhaps we can a use a "#if defined(CONFIG_I8259) ||
defined(CONFIG_LM32) || (defined(TARGE_SPARC) &&
!defined(TARGET_SPARC64))" instead, so we keep the command for PReP ?

Laurent

Re: [Qemu-devel] [PATCH 4/4] Disable "info irq" and "info pic" for target-ppc

[Thomas Huth]

On Mon, 29 Jun 2015 09:52:56 +0200
Laurent Vivier <lvivier@redhat.com> wrote:

> 
> 
> On 29/06/2015 07:36, David Gibson wrote:
> > The "info irq" and "info pic" HMP commands are available on some, but not
> > all targets, and what they do isn't terribly consistent.  For SPARC and
> > LM32 they do something platform specific, but for x86, powerpc, and MIPS
> > they print some information from the i8259 (and only the i8259) interrupt
> > controller.
> > 
> > It's debatable whether these commands are any use at all, and we should
> > probably make better, qdev aware ways of getting information from a
> > machines PICs.  However, those don't exist yet, so on x86 it's at least
> > potentially useful to have these HMP commands.  I can't speak for MIPS.
> > 
> > For ppc, though, the i8259, if it exists at all, is usually just a
> > secondary controller for legacy ISA.  The only case where i8259 is the
> > main system PIC on ppc is for the ancient and little-used PReP platform.
> > 
> > So, even without QOM-ish replacement, the info pic and info irq HMP
> > commands have no value on ppc.
> > 
> > This patch, therefore, disables these commands for ppc targets.  This will
> > allow ppc builds which don't include PReP to not include ISA bus support
> > either.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  monitor.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/monitor.c b/monitor.c
> > index aeea2b5..8c56bfa 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -2573,7 +2573,7 @@ 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) || \
> > +#if defined(TARGET_I386) || defined(TARGET_MIPS) || \
> >      defined(TARGET_LM32) || (defined(TARGET_SPARC) && !defined(TARGET_SPARC64))
> >      {
> >          .name       = "irq",
> > 
> 
> Perhaps we can a use a "#if defined(CONFIG_I8259) ||
> defined(CONFIG_LM32) || (defined(TARGE_SPARC) &&
> !defined(TARGET_SPARC64))" instead, so we keep the command for PReP ?

AFAIK this currently won't work since CONFIG_I8259 is only defined for
the Makefiles, but not for the C pre-processor :-(

So unless somebody fixes that first, I think David's approach is the
only practicable solution right now.

 Thomas

Re: [Qemu-devel] [PATCH 4/4] Disable "info irq" and "info pic" for target-ppc

[Laurent Vivier]

On 29/06/2015 11:30, Thomas Huth wrote:
> On Mon, 29 Jun 2015 09:52:56 +0200
> Laurent Vivier <lvivier@redhat.com> wrote:
> 
>>
>>
>> On 29/06/2015 07:36, David Gibson wrote:
>>> The "info irq" and "info pic" HMP commands are available on some, but not
>>> all targets, and what they do isn't terribly consistent.  For SPARC and
>>> LM32 they do something platform specific, but for x86, powerpc, and MIPS
>>> they print some information from the i8259 (and only the i8259) interrupt
>>> controller.
>>>
>>> It's debatable whether these commands are any use at all, and we should
>>> probably make better, qdev aware ways of getting information from a
>>> machines PICs.  However, those don't exist yet, so on x86 it's at least
>>> potentially useful to have these HMP commands.  I can't speak for MIPS.
>>>
>>> For ppc, though, the i8259, if it exists at all, is usually just a
>>> secondary controller for legacy ISA.  The only case where i8259 is the
>>> main system PIC on ppc is for the ancient and little-used PReP platform.
>>>
>>> So, even without QOM-ish replacement, the info pic and info irq HMP
>>> commands have no value on ppc.
>>>
>>> This patch, therefore, disables these commands for ppc targets.  This will
>>> allow ppc builds which don't include PReP to not include ISA bus support
>>> either.
>>>
>>> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
>>> ---
>>>  monitor.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/monitor.c b/monitor.c
>>> index aeea2b5..8c56bfa 100644
>>> --- a/monitor.c
>>> +++ b/monitor.c
>>> @@ -2573,7 +2573,7 @@ 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) || \
>>> +#if defined(TARGET_I386) || defined(TARGET_MIPS) || \
>>>      defined(TARGET_LM32) || (defined(TARGET_SPARC) && !defined(TARGET_SPARC64))
>>>      {
>>>          .name       = "irq",
>>>
>>
>> Perhaps we can a use a "#if defined(CONFIG_I8259) ||
>> defined(CONFIG_LM32) || (defined(TARGE_SPARC) &&
>> !defined(TARGET_SPARC64))" instead, so we keep the command for PReP ?
> 
> AFAIK this currently won't work since CONFIG_I8259 is only defined for
> the Makefiles, but not for the C pre-processor :-(

Yes, I see that afterward, but ...

> So unless somebody fixes that first, I think David's approach is the
> only practicable solution right now.

if you add "config-devices.h" in GENERATED_HEADERS in Makefile.target,
and include "config-devices.h" in monitor.c, it works (all PREP
dependencies in default-configs/ppc64-softmmu.mak must be removed too)

But does this change acceptable for a tiny improvement ?

Laurent

Re: [Qemu-devel] [PATCH 4/4] Disable "info irq" and "info pic" for target-ppc

[Laurent Vivier]

On 29/06/2015 11:51, Laurent Vivier wrote:
> 
> 
> On 29/06/2015 11:30, Thomas Huth wrote:
>> On Mon, 29 Jun 2015 09:52:56 +0200
>> Laurent Vivier <lvivier@redhat.com> wrote:
>>
>>>
>>>
>>> On 29/06/2015 07:36, David Gibson wrote:
>>>> The "info irq" and "info pic" HMP commands are available on some, but not
>>>> all targets, and what they do isn't terribly consistent.  For SPARC and
>>>> LM32 they do something platform specific, but for x86, powerpc, and MIPS
>>>> they print some information from the i8259 (and only the i8259) interrupt
>>>> controller.
>>>>
>>>> It's debatable whether these commands are any use at all, and we should
>>>> probably make better, qdev aware ways of getting information from a
>>>> machines PICs.  However, those don't exist yet, so on x86 it's at least
>>>> potentially useful to have these HMP commands.  I can't speak for MIPS.
>>>>
>>>> For ppc, though, the i8259, if it exists at all, is usually just a
>>>> secondary controller for legacy ISA.  The only case where i8259 is the
>>>> main system PIC on ppc is for the ancient and little-used PReP platform.
>>>>
>>>> So, even without QOM-ish replacement, the info pic and info irq HMP
>>>> commands have no value on ppc.
>>>>
>>>> This patch, therefore, disables these commands for ppc targets.  This will
>>>> allow ppc builds which don't include PReP to not include ISA bus support
>>>> either.
>>>>
>>>> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
>>>> ---
>>>>  monitor.c | 2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/monitor.c b/monitor.c
>>>> index aeea2b5..8c56bfa 100644
>>>> --- a/monitor.c
>>>> +++ b/monitor.c
>>>> @@ -2573,7 +2573,7 @@ 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) || \
>>>> +#if defined(TARGET_I386) || defined(TARGET_MIPS) || \
>>>>      defined(TARGET_LM32) || (defined(TARGET_SPARC) && !defined(TARGET_SPARC64))
>>>>      {
>>>>          .name       = "irq",
>>>>
>>>
>>> Perhaps we can a use a "#if defined(CONFIG_I8259) ||
>>> defined(CONFIG_LM32) || (defined(TARGE_SPARC) &&
>>> !defined(TARGET_SPARC64))" instead, so we keep the command for PReP ?
>>
>> AFAIK this currently won't work since CONFIG_I8259 is only defined for
>> the Makefiles, but not for the C pre-processor :-(
> 
> Yes, I see that afterward, but ...
> 
>> So unless somebody fixes that first, I think David's approach is the
>> only practicable solution right now.
> 
> if you add "config-devices.h" in GENERATED_HEADERS in Makefile.target,
> and include "config-devices.h" in monitor.c, it works (all PREP
> dependencies in default-configs/ppc64-softmmu.mak must be removed too)
> 
> But does this change acceptable for a tiny improvement ?

In fine, I think we can also do like for sparc:

defined(TARGET_PPC) && !defined(TARGET_PPC64)

Laurent

Re: [Qemu-devel] [PATCH 4/4] Disable "info irq" and "info pic" for target-ppc

[Andreas Färber]

Am 29.06.2015 um 11:55 schrieb Laurent Vivier:
> On 29/06/2015 11:51, Laurent Vivier wrote:
>> On 29/06/2015 11:30, Thomas Huth wrote:
>>> On Mon, 29 Jun 2015 09:52:56 +0200
>>> Laurent Vivier <lvivier@redhat.com> wrote:
>>>> On 29/06/2015 07:36, David Gibson wrote:
>>>>> diff --git a/monitor.c b/monitor.c
>>>>> index aeea2b5..8c56bfa 100644
>>>>> --- a/monitor.c
>>>>> +++ b/monitor.c
>>>>> @@ -2573,7 +2573,7 @@ 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) || \
>>>>> +#if defined(TARGET_I386) || defined(TARGET_MIPS) || \
>>>>>      defined(TARGET_LM32) || (defined(TARGET_SPARC) && !defined(TARGET_SPARC64))
>>>>>      {
>>>>>          .name       = "irq",
>>>>>
>>>>
>>>> Perhaps we can a use a "#if defined(CONFIG_I8259) ||
>>>> defined(CONFIG_LM32) || (defined(TARGE_SPARC) &&
>>>> !defined(TARGET_SPARC64))" instead, so we keep the command for PReP ?
>>>
>>> AFAIK this currently won't work since CONFIG_I8259 is only defined for
>>> the Makefiles, but not for the C pre-processor :-(
>>
>> Yes, I see that afterward, but ...
>>
>>> So unless somebody fixes that first, I think David's approach is the
>>> only practicable solution right now.
>>
>> if you add "config-devices.h" in GENERATED_HEADERS in Makefile.target,
>> and include "config-devices.h" in monitor.c, it works (all PREP
>> dependencies in default-configs/ppc64-softmmu.mak must be removed too)
>>
>> But does this change acceptable for a tiny improvement ?
> 
> In fine, I think we can also do like for sparc:
> 
> defined(TARGET_PPC) && !defined(TARGET_PPC64)

Alex specifically requested PReP to be made available in ppc64, too.

Regards,
Andreas

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

Re: [Qemu-devel] [PATCH 4/4] Disable "info irq" and "info pic" for target-ppc

[Laurent Vivier]

On 29/06/2015 12:06, Andreas Färber wrote:
> Am 29.06.2015 um 11:55 schrieb Laurent Vivier:
>> On 29/06/2015 11:51, Laurent Vivier wrote:
>>> On 29/06/2015 11:30, Thomas Huth wrote:
>>>> On Mon, 29 Jun 2015 09:52:56 +0200
>>>> Laurent Vivier <lvivier@redhat.com> wrote:
>>>>> On 29/06/2015 07:36, David Gibson wrote:
>>>>>> diff --git a/monitor.c b/monitor.c
>>>>>> index aeea2b5..8c56bfa 100644
>>>>>> --- a/monitor.c
>>>>>> +++ b/monitor.c
>>>>>> @@ -2573,7 +2573,7 @@ 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) || \
>>>>>> +#if defined(TARGET_I386) || defined(TARGET_MIPS) || \
>>>>>>      defined(TARGET_LM32) || (defined(TARGET_SPARC) && !defined(TARGET_SPARC64))
>>>>>>      {
>>>>>>          .name       = "irq",
>>>>>>
>>>>>
>>>>> Perhaps we can a use a "#if defined(CONFIG_I8259) ||
>>>>> defined(CONFIG_LM32) || (defined(TARGE_SPARC) &&
>>>>> !defined(TARGET_SPARC64))" instead, so we keep the command for PReP ?
>>>>
>>>> AFAIK this currently won't work since CONFIG_I8259 is only defined for
>>>> the Makefiles, but not for the C pre-processor :-(
>>>
>>> Yes, I see that afterward, but ...
>>>
>>>> So unless somebody fixes that first, I think David's approach is the
>>>> only practicable solution right now.
>>>
>>> if you add "config-devices.h" in GENERATED_HEADERS in Makefile.target,
>>> and include "config-devices.h" in monitor.c, it works (all PREP
>>> dependencies in default-configs/ppc64-softmmu.mak must be removed too)
>>>
>>> But does this change acceptable for a tiny improvement ?
>>
>> In fine, I think we can also do like for sparc:
>>
>> defined(TARGET_PPC) && !defined(TARGET_PPC64)
> 
> Alex specifically requested PReP to be made available in ppc64, too.

Thank you Andreas.

But why ? (I didn't find the answer with google, a link can be helpful).

Is there any 64bit PReP ?

BTW using CONFIG_I8259 cannot enable it only for PReP and not for other
PPC64. So the solution from David is the best.

Laurent

Re: [Qemu-devel] [PATCH 4/4] Disable "info irq" and "info pic" for target-ppc

[Andreas Färber]

Am 29.06.2015 um 12:22 schrieb Laurent Vivier:
> On 29/06/2015 12:06, Andreas Färber wrote:
>> Am 29.06.2015 um 11:55 schrieb Laurent Vivier:
>>> On 29/06/2015 11:51, Laurent Vivier wrote:
>>>> On 29/06/2015 11:30, Thomas Huth wrote:
>>>>> On Mon, 29 Jun 2015 09:52:56 +0200
>>>>> Laurent Vivier <lvivier@redhat.com> wrote:
>>>>>> On 29/06/2015 07:36, David Gibson wrote:
>>>>>>> diff --git a/monitor.c b/monitor.c
>>>>>>> index aeea2b5..8c56bfa 100644
>>>>>>> --- a/monitor.c
>>>>>>> +++ b/monitor.c
>>>>>>> @@ -2573,7 +2573,7 @@ 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) || \
>>>>>>> +#if defined(TARGET_I386) || defined(TARGET_MIPS) || \
>>>>>>>      defined(TARGET_LM32) || (defined(TARGET_SPARC) && !defined(TARGET_SPARC64))
>>>>>>>      {
>>>>>>>          .name       = "irq",
>>>>>>>
>>>>>>
>>>>>> Perhaps we can a use a "#if defined(CONFIG_I8259) ||
>>>>>> defined(CONFIG_LM32) || (defined(TARGE_SPARC) &&
>>>>>> !defined(TARGET_SPARC64))" instead, so we keep the command for PReP ?
>>>>>
>>>>> AFAIK this currently won't work since CONFIG_I8259 is only defined for
>>>>> the Makefiles, but not for the C pre-processor :-(
>>>>
>>>> Yes, I see that afterward, but ...
>>>>
>>>>> So unless somebody fixes that first, I think David's approach is the
>>>>> only practicable solution right now.
>>>>
>>>> if you add "config-devices.h" in GENERATED_HEADERS in Makefile.target,
>>>> and include "config-devices.h" in monitor.c, it works (all PREP
>>>> dependencies in default-configs/ppc64-softmmu.mak must be removed too)
>>>>
>>>> But does this change acceptable for a tiny improvement ?
>>>
>>> In fine, I think we can also do like for sparc:
>>>
>>> defined(TARGET_PPC) && !defined(TARGET_PPC64)
>>
>> Alex specifically requested PReP to be made available in ppc64, too.
> 
> Thank you Andreas.
> 
> But why ? (I didn't find the answer with google, a link can be helpful).
> 
> Is there any 64bit PReP ?
> 
> BTW using CONFIG_I8259 cannot enable it only for PReP and not for other
> PPC64. So the solution from David is the best.

For all newer targets (excluding sparc64 etc.) the 64-bit target is
expected to deliver all 32-bit targets as well, for use with KVM in that
bitness or for convenience of installing just one executable.

Alex is on CC and if you ping him hard enough he might reply himself. ;)

So, generating a header file, like you proposed above, seems like the
most elegant solution to me here.
That won't help the qtest issue I raised on the other patch though,
which would need a QMP lookup as discussed in the ivshmem context. I
rebased that patch on Friday and can look into factoring that out into
libqtest.[ch].

Regards,
Andreas

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

Re: [Qemu-devel] [PATCH 4/4] Disable "info irq" and "info pic" for target-ppc

[Andreas Färber]

Am 29.06.2015 um 12:36 schrieb Andreas Färber:
> Am 29.06.2015 um 12:22 schrieb Laurent Vivier:
>> On 29/06/2015 12:06, Andreas Färber wrote:
>>> Am 29.06.2015 um 11:55 schrieb Laurent Vivier:
>>>> On 29/06/2015 11:51, Laurent Vivier wrote:
>>>>> On 29/06/2015 11:30, Thomas Huth wrote:
>>>>>> On Mon, 29 Jun 2015 09:52:56 +0200
>>>>>> Laurent Vivier <lvivier@redhat.com> wrote:
>>>>>>> On 29/06/2015 07:36, David Gibson wrote:
>>>>>>>> diff --git a/monitor.c b/monitor.c
>>>>>>>> index aeea2b5..8c56bfa 100644
>>>>>>>> --- a/monitor.c
>>>>>>>> +++ b/monitor.c
>>>>>>>> @@ -2573,7 +2573,7 @@ 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) || \
>>>>>>>> +#if defined(TARGET_I386) || defined(TARGET_MIPS) || \
>>>>>>>>      defined(TARGET_LM32) || (defined(TARGET_SPARC) && !defined(TARGET_SPARC64))
>>>>>>>>      {
>>>>>>>>          .name       = "irq",
>>>>>>>>
>>>>>>>
>>>>>>> Perhaps we can a use a "#if defined(CONFIG_I8259) ||
>>>>>>> defined(CONFIG_LM32) || (defined(TARGE_SPARC) &&
>>>>>>> !defined(TARGET_SPARC64))" instead, so we keep the command for PReP ?
>>>>>>
>>>>>> AFAIK this currently won't work since CONFIG_I8259 is only defined for
>>>>>> the Makefiles, but not for the C pre-processor :-(
>>>>>
>>>>> Yes, I see that afterward, but ...
>>>>>
>>>>>> So unless somebody fixes that first, I think David's approach is the
>>>>>> only practicable solution right now.
>>>>>
>>>>> if you add "config-devices.h" in GENERATED_HEADERS in Makefile.target,
>>>>> and include "config-devices.h" in monitor.c, it works (all PREP
>>>>> dependencies in default-configs/ppc64-softmmu.mak must be removed too)
>>>>>
>>>>> But does this change acceptable for a tiny improvement ?
>>>>
>>>> In fine, I think we can also do like for sparc:
>>>>
>>>> defined(TARGET_PPC) && !defined(TARGET_PPC64)
>>>
>>> Alex specifically requested PReP to be made available in ppc64, too.
>>
>> Thank you Andreas.
>>
>> But why ? (I didn't find the answer with google, a link can be helpful).

http://git.qemu-project.org/?p=qemu.git;a=commit;h=acbb090b2400f627a801074c4e3e006c7501bb26

(found by looking at the ppc64-softmmu.mak Git history)

Judging by Markus as the reporter, I assume it was a tree-wide analysis
that came up with this inconsistency, which I was then asked to fix this
way.

Andreas

>>
>> Is there any 64bit PReP ?
>>
>> BTW using CONFIG_I8259 cannot enable it only for PReP and not for other
>> PPC64. So the solution from David is the best.
> 
> For all newer targets (excluding sparc64 etc.) the 64-bit target is
> expected to deliver all 32-bit targets as well, for use with KVM in that
> bitness or for convenience of installing just one executable.
> 
> Alex is on CC and if you ping him hard enough he might reply himself. ;)
> 
> So, generating a header file, like you proposed above, seems like the
> most elegant solution to me here.
> That won't help the qtest issue I raised on the other patch though,
> which would need a QMP lookup as discussed in the ivshmem context. I
> rebased that patch on Friday and can look into factoring that out into
> libqtest.[ch].
> 
> Regards,
> Andreas
> 


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

Re: [Qemu-devel] [PATCH 4/4] Disable "info irq" and "info pic" for target-ppc

[Alexander Graf]

On 06/29/15 12:43, Andreas Färber wrote:
> Am 29.06.2015 um 12:36 schrieb Andreas Färber:
>> Am 29.06.2015 um 12:22 schrieb Laurent Vivier:
>>> On 29/06/2015 12:06, Andreas Färber wrote:
>>>> Am 29.06.2015 um 11:55 schrieb Laurent Vivier:
>>>>> On 29/06/2015 11:51, Laurent Vivier wrote:
>>>>>> On 29/06/2015 11:30, Thomas Huth wrote:
>>>>>>> On Mon, 29 Jun 2015 09:52:56 +0200
>>>>>>> Laurent Vivier <lvivier@redhat.com> wrote:
>>>>>>>> On 29/06/2015 07:36, David Gibson wrote:
>>>>>>>>> diff --git a/monitor.c b/monitor.c
>>>>>>>>> index aeea2b5..8c56bfa 100644
>>>>>>>>> --- a/monitor.c
>>>>>>>>> +++ b/monitor.c
>>>>>>>>> @@ -2573,7 +2573,7 @@ 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) || \
>>>>>>>>> +#if defined(TARGET_I386) || defined(TARGET_MIPS) || \
>>>>>>>>>       defined(TARGET_LM32) || (defined(TARGET_SPARC) && !defined(TARGET_SPARC64))
>>>>>>>>>       {
>>>>>>>>>           .name       = "irq",
>>>>>>>>>
>>>>>>>> Perhaps we can a use a "#if defined(CONFIG_I8259) ||
>>>>>>>> defined(CONFIG_LM32) || (defined(TARGE_SPARC) &&
>>>>>>>> !defined(TARGET_SPARC64))" instead, so we keep the command for PReP ?
>>>>>>> AFAIK this currently won't work since CONFIG_I8259 is only defined for
>>>>>>> the Makefiles, but not for the C pre-processor :-(
>>>>>> Yes, I see that afterward, but ...
>>>>>>
>>>>>>> So unless somebody fixes that first, I think David's approach is the
>>>>>>> only practicable solution right now.
>>>>>> if you add "config-devices.h" in GENERATED_HEADERS in Makefile.target,
>>>>>> and include "config-devices.h" in monitor.c, it works (all PREP
>>>>>> dependencies in default-configs/ppc64-softmmu.mak must be removed too)
>>>>>>
>>>>>> But does this change acceptable for a tiny improvement ?
>>>>> In fine, I think we can also do like for sparc:
>>>>>
>>>>> defined(TARGET_PPC) && !defined(TARGET_PPC64)
>>>> Alex specifically requested PReP to be made available in ppc64, too.
>>> Thank you Andreas.
>>>
>>> But why ? (I didn't find the answer with google, a link can be helpful).
> http://git.qemu-project.org/?p=qemu.git;a=commit;h=acbb090b2400f627a801074c4e3e006c7501bb26
>
> (found by looking at the ppc64-softmmu.mak Git history)
>
> Judging by Markus as the reporter, I assume it was a tree-wide analysis
> that came up with this inconsistency, which I was then asked to fix this
> way.

Yes, it's consistency. All 64bit targets allow to run their 32bit 
machine types as well. Whether this is a sensible thing to do or not is 
a different discussion that (if we want to) we need to do outside of the 
scope of this mail thread.

As far as this patch goes, you get a clear nack from me, as it's a 
regression for the prep target. Please just QOM'ify the interrupt 
controller, add an interfact that allows you to query the irq stats and 
then loop through all devices searching for that interface in the object 
tree. That way we should be able to get rid of all #ifdefs in that 
particular code and enable new irq controllers to expose their stats easily.


Alex

Re: [Qemu-devel] [PATCH 4/4] Disable "info irq" and "info pic" for target-ppc

[David Gibson]

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

On Mon, Jun 29, 2015 at 01:02:50PM +0200, Alexander Graf wrote:
> On 06/29/15 12:43, Andreas Färber wrote:
> >Am 29.06.2015 um 12:36 schrieb Andreas Färber:
> >>Am 29.06.2015 um 12:22 schrieb Laurent Vivier:
> >>>On 29/06/2015 12:06, Andreas Färber wrote:
> >>>>Am 29.06.2015 um 11:55 schrieb Laurent Vivier:
> >>>>>On 29/06/2015 11:51, Laurent Vivier wrote:
> >>>>>>On 29/06/2015 11:30, Thomas Huth wrote:
> >>>>>>>On Mon, 29 Jun 2015 09:52:56 +0200
> >>>>>>>Laurent Vivier <lvivier@redhat.com> wrote:
> >>>>>>>>On 29/06/2015 07:36, David Gibson wrote:
> >>>>>>>>>diff --git a/monitor.c b/monitor.c
> >>>>>>>>>index aeea2b5..8c56bfa 100644
> >>>>>>>>>--- a/monitor.c
> >>>>>>>>>+++ b/monitor.c
> >>>>>>>>>@@ -2573,7 +2573,7 @@ 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) || \
> >>>>>>>>>+#if defined(TARGET_I386) || defined(TARGET_MIPS) || \
> >>>>>>>>>      defined(TARGET_LM32) || (defined(TARGET_SPARC) && !defined(TARGET_SPARC64))
> >>>>>>>>>      {
> >>>>>>>>>          .name       = "irq",
> >>>>>>>>>
> >>>>>>>>Perhaps we can a use a "#if defined(CONFIG_I8259) ||
> >>>>>>>>defined(CONFIG_LM32) || (defined(TARGE_SPARC) &&
> >>>>>>>>!defined(TARGET_SPARC64))" instead, so we keep the command for PReP ?
> >>>>>>>AFAIK this currently won't work since CONFIG_I8259 is only defined for
> >>>>>>>the Makefiles, but not for the C pre-processor :-(
> >>>>>>Yes, I see that afterward, but ...
> >>>>>>
> >>>>>>>So unless somebody fixes that first, I think David's approach is the
> >>>>>>>only practicable solution right now.
> >>>>>>if you add "config-devices.h" in GENERATED_HEADERS in Makefile.target,
> >>>>>>and include "config-devices.h" in monitor.c, it works (all PREP
> >>>>>>dependencies in default-configs/ppc64-softmmu.mak must be removed too)
> >>>>>>
> >>>>>>But does this change acceptable for a tiny improvement ?
> >>>>>In fine, I think we can also do like for sparc:
> >>>>>
> >>>>>defined(TARGET_PPC) && !defined(TARGET_PPC64)
> >>>>Alex specifically requested PReP to be made available in ppc64, too.
> >>>Thank you Andreas.
> >>>
> >>>But why ? (I didn't find the answer with google, a link can be helpful).
> >http://git.qemu-project.org/?p=qemu.git;a=commit;h=acbb090b2400f627a801074c4e3e006c7501bb26
> >
> >(found by looking at the ppc64-softmmu.mak Git history)
> >
> >Judging by Markus as the reporter, I assume it was a tree-wide analysis
> >that came up with this inconsistency, which I was then asked to fix this
> >way.
> 
> Yes, it's consistency. All 64bit targets allow to run their 32bit machine
> types as well. Whether this is a sensible thing to do or not is a different
> discussion that (if we want to) we need to do outside of the scope of this
> mail thread.
> 
> As far as this patch goes, you get a clear nack from me, as it's a
> regression for the prep target. Please just QOM'ify the interrupt

"just"!?!

> controller, add an interfact that allows you to query the irq stats and then
> loop through all devices searching for that interface in the object tree.
> That way we should be able to get rid of all #ifdefs in that particular code
> and enable new irq controllers to expose their stats easily.

-- 
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 4/4] Disable "info irq" and "info pic" for target-ppc

[Thomas Huth]

On Mon, 29 Jun 2015 11:55:08 +0200
Laurent Vivier <lvivier@redhat.com> wrote:

> 
> 
> On 29/06/2015 11:51, Laurent Vivier wrote:
> > 
> > 
> > On 29/06/2015 11:30, Thomas Huth wrote:
> >> On Mon, 29 Jun 2015 09:52:56 +0200
> >> Laurent Vivier <lvivier@redhat.com> wrote:
> >>
> >>>
> >>>
> >>> On 29/06/2015 07:36, David Gibson wrote:
> >>>> The "info irq" and "info pic" HMP commands are available on some, but not
> >>>> all targets, and what they do isn't terribly consistent.  For SPARC and
> >>>> LM32 they do something platform specific, but for x86, powerpc, and MIPS
> >>>> they print some information from the i8259 (and only the i8259) interrupt
> >>>> controller.
> >>>>
> >>>> It's debatable whether these commands are any use at all, and we should
> >>>> probably make better, qdev aware ways of getting information from a
> >>>> machines PICs.  However, those don't exist yet, so on x86 it's at least
> >>>> potentially useful to have these HMP commands.  I can't speak for MIPS.
> >>>>
> >>>> For ppc, though, the i8259, if it exists at all, is usually just a
> >>>> secondary controller for legacy ISA.  The only case where i8259 is the
> >>>> main system PIC on ppc is for the ancient and little-used PReP platform.
> >>>>
> >>>> So, even without QOM-ish replacement, the info pic and info irq HMP
> >>>> commands have no value on ppc.
> >>>>
> >>>> This patch, therefore, disables these commands for ppc targets.  This will
> >>>> allow ppc builds which don't include PReP to not include ISA bus support
> >>>> either.
> >>>>
> >>>> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> >>>> ---
> >>>>  monitor.c | 2 +-
> >>>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/monitor.c b/monitor.c
> >>>> index aeea2b5..8c56bfa 100644
> >>>> --- a/monitor.c
> >>>> +++ b/monitor.c
> >>>> @@ -2573,7 +2573,7 @@ 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) || \
> >>>> +#if defined(TARGET_I386) || defined(TARGET_MIPS) || \
> >>>>      defined(TARGET_LM32) || (defined(TARGET_SPARC) && !defined(TARGET_SPARC64))
> >>>>      {
> >>>>          .name       = "irq",
> >>>>
> >>>
> >>> Perhaps we can a use a "#if defined(CONFIG_I8259) ||
> >>> defined(CONFIG_LM32) || (defined(TARGE_SPARC) &&
> >>> !defined(TARGET_SPARC64))" instead, so we keep the command for PReP ?
> >>
> >> AFAIK this currently won't work since CONFIG_I8259 is only defined for
> >> the Makefiles, but not for the C pre-processor :-(
> > 
> > Yes, I see that afterward, but ...
> > 
> >> So unless somebody fixes that first, I think David's approach is the
> >> only practicable solution right now.
> > 
> > if you add "config-devices.h" in GENERATED_HEADERS in Makefile.target,
> > and include "config-devices.h" in monitor.c, it works (all PREP
> > dependencies in default-configs/ppc64-softmmu.mak must be removed too)
> > 
> > But does this change acceptable for a tiny improvement ?
> 
> In fine, I think we can also do like for sparc:
> 
> defined(TARGET_PPC) && !defined(TARGET_PPC64)

+1

I like that idea, sounds like a good compromise.

 Thomas