[PATCH v4 00/12] hw: Strengthen SysBus & QBus API

[PATCH v4 00/12] hw: Strengthen SysBus & QBus API

[Philippe Mathieu-Daudé]

Missing review: 4, 12

Since v1:
- Addressed Zoltan review comments
- Addressed Mark suggestion
- Added R-b tags

Hi,

This series ensure following is called *before* a
device is realized:
- qbus_new()
- sysbus_init_mmio()
- qdev_init_gpio_in_named_with_opaque()

and these are called *after* it is:
- sysbus_mmio_map()
- sysbus_connect_irq(),
- qdev_connect_gpio_out()
- qdev_connect_gpio_out_named()

Patches from v2 enforcing these checks will be posted
in a separate series.

Philippe Mathieu-Daudé (12):
  hw/ide/ich9: Use AHCIPCIState typedef
  hw/rx/rx62n: Reduce inclusion of 'qemu/units.h'
  hw/rx/rx62n: Only call qdev_get_gpio_in() when necessary
  hw/i386/q35: Realize LPC PCI function before accessing it
  hw/ppc/prep: Realize ISA bridge before accessing it
  hw/misc/macio: Realize IDE controller before accessing it
  hw/sh4/r2d: Realize IDE controller before accessing it
  hw/sparc/sun4m: Realize DMA controller before accessing it
  hw/sparc/leon3: Realize GRLIB IRQ controller before accessing it
  hw/sparc/leon3: Pass DeviceState opaque argument to leon3_set_pil_in()
  hw/sparc/leon3: Initialize GPIO before realizing CPU devices
  hw/sparc64/cpu: Initialize GPIO before realizing CPU devices

 include/hw/rx/rx62n.h |  2 --
 hw/i386/pc_q35.c      |  6 +++---
 hw/ide/ich.c          |  6 +++---
 hw/misc/macio/macio.c |  9 ++++++---
 hw/ppc/prep.c         |  2 +-
 hw/rx/rx-gdbsim.c     |  1 +
 hw/rx/rx62n.c         | 17 +++++++++--------
 hw/sh4/r2d.c          |  2 +-
 hw/sparc/leon3.c      | 17 ++++++++---------
 hw/sparc/sun4m.c      |  7 +++++--
 hw/sparc64/sparc64.c  |  4 +++-
 11 files changed, 40 insertions(+), 33 deletions(-)

-- 
2.41.0

[PATCH v4 01/12] hw/ide/ich9: Use AHCIPCIState typedef

[Philippe Mathieu-Daudé]

QEMU coding style recommend using structure typedefs:
https://www.qemu.org/docs/master/devel/style.html#typedefs

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/ide/ich.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/ide/ich.c b/hw/ide/ich.c
index 49f8eb8a7d..048ea7e509 100644
--- a/hw/ide/ich.c
+++ b/hw/ide/ich.c
@@ -99,14 +99,14 @@ static void pci_ich9_reset(DeviceState *dev)
 
 static void pci_ich9_ahci_init(Object *obj)
 {
-    struct AHCIPCIState *d = ICH9_AHCI(obj);
+    AHCIPCIState *d = ICH9_AHCI(obj);
 
     ahci_init(&d->ahci, DEVICE(obj));
 }
 
 static void pci_ich9_ahci_realize(PCIDevice *dev, Error **errp)
 {
-    struct AHCIPCIState *d;
+    AHCIPCIState *d;
     int sata_cap_offset;
     uint8_t *sata_cap;
     d = ICH9_AHCI(dev);
@@ -154,7 +154,7 @@ static void pci_ich9_ahci_realize(PCIDevice *dev, Error **errp)
 
 static void pci_ich9_uninit(PCIDevice *dev)
 {
-    struct AHCIPCIState *d;
+    AHCIPCIState *d;
     d = ICH9_AHCI(dev);
 
     msi_uninit(dev);
-- 
2.41.0

[PATCH v4 02/12] hw/rx/rx62n: Reduce inclusion of 'qemu/units.h'

[Philippe Mathieu-Daudé]

"qemu/units.h" is not used in the "hw/rx/rx62n.h"
header, include it in the source where it is.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Yoshinori Sato <ysato@users.sourceforge.jp>
---
 include/hw/rx/rx62n.h | 1 -
 hw/rx/rx-gdbsim.c     | 1 +
 hw/rx/rx62n.c         | 1 +
 3 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/hw/rx/rx62n.h b/include/hw/rx/rx62n.h
index 73ceeb58e5..bcda583ab3 100644
--- a/include/hw/rx/rx62n.h
+++ b/include/hw/rx/rx62n.h
@@ -29,7 +29,6 @@
 #include "hw/timer/renesas_tmr.h"
 #include "hw/timer/renesas_cmt.h"
 #include "hw/char/renesas_sci.h"
-#include "qemu/units.h"
 #include "qom/object.h"
 
 #define TYPE_RX62N_MCU "rx62n-mcu"
diff --git a/hw/rx/rx-gdbsim.c b/hw/rx/rx-gdbsim.c
index 47c17026c7..bb4746c556 100644
--- a/hw/rx/rx-gdbsim.c
+++ b/hw/rx/rx-gdbsim.c
@@ -20,6 +20,7 @@
 #include "qemu/cutils.h"
 #include "qemu/error-report.h"
 #include "qemu/guest-random.h"
+#include "qemu/units.h"
 #include "qapi/error.h"
 #include "hw/loader.h"
 #include "hw/rx/rx62n.h"
diff --git a/hw/rx/rx62n.c b/hw/rx/rx62n.c
index 4dc44afd9d..d3f61a6837 100644
--- a/hw/rx/rx62n.c
+++ b/hw/rx/rx62n.c
@@ -23,6 +23,7 @@
 #include "qemu/osdep.h"
 #include "qapi/error.h"
 #include "qemu/error-report.h"
+#include "qemu/units.h"
 #include "hw/rx/rx62n.h"
 #include "hw/loader.h"
 #include "hw/sysbus.h"
-- 
2.41.0

[PATCH v4 03/12] hw/rx/rx62n: Only call qdev_get_gpio_in() when necessary

[Philippe Mathieu-Daudé]

Instead of filling an array of all the possible IRQs, only call
qdev_get_gpio_in() when an IRQ is used. Remove the array from
RX62NState. Doing so we avoid calling qdev_get_gpio_in() on an
unrealized device.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Yoshinori Sato <ysato@users.sourceforge.jp>
---
 include/hw/rx/rx62n.h |  1 -
 hw/rx/rx62n.c         | 16 ++++++++--------
 2 files changed, 8 insertions(+), 9 deletions(-)

diff --git a/include/hw/rx/rx62n.h b/include/hw/rx/rx62n.h
index bcda583ab3..766fe0e435 100644
--- a/include/hw/rx/rx62n.h
+++ b/include/hw/rx/rx62n.h
@@ -67,7 +67,6 @@ struct RX62NState {
     MemoryRegion iomem2;
     MemoryRegion iomem3;
     MemoryRegion c_flash;
-    qemu_irq irq[NR_IRQS];
 
     /* Input Clock (XTAL) frequency */
     uint32_t xtal_freq_hz;
diff --git a/hw/rx/rx62n.c b/hw/rx/rx62n.c
index d3f61a6837..560f53a58a 100644
--- a/hw/rx/rx62n.c
+++ b/hw/rx/rx62n.c
@@ -148,14 +148,11 @@ static void register_icu(RX62NState *s)
         qlist_append_int(trigger_level, levelirq[i]);
     }
     qdev_prop_set_array(DEVICE(icu), "trigger-level", trigger_level);
-
-    for (i = 0; i < NR_IRQS; i++) {
-        s->irq[i] = qdev_get_gpio_in(DEVICE(icu), i);
-    }
     sysbus_realize(icu, &error_abort);
+
     sysbus_connect_irq(icu, 0, qdev_get_gpio_in(DEVICE(&s->cpu), RX_CPU_IRQ));
     sysbus_connect_irq(icu, 1, qdev_get_gpio_in(DEVICE(&s->cpu), RX_CPU_FIR));
-    sysbus_connect_irq(icu, 2, s->irq[SWI]);
+    sysbus_connect_irq(icu, 2, qdev_get_gpio_in(DEVICE(&s->icu), SWI));
     sysbus_mmio_map(icu, 0, RX62N_ICU_BASE);
 }
 
@@ -172,7 +169,8 @@ static void register_tmr(RX62NState *s, int unit)
 
     irqbase = RX62N_TMR_IRQ + TMR_NR_IRQ * unit;
     for (i = 0; i < TMR_NR_IRQ; i++) {
-        sysbus_connect_irq(tmr, i, s->irq[irqbase + i]);
+        sysbus_connect_irq(tmr, i,
+                           qdev_get_gpio_in(DEVICE(&s->icu), irqbase + i));
     }
     sysbus_mmio_map(tmr, 0, RX62N_TMR_BASE + unit * 0x10);
 }
@@ -190,7 +188,8 @@ static void register_cmt(RX62NState *s, int unit)
 
     irqbase = RX62N_CMT_IRQ + CMT_NR_IRQ * unit;
     for (i = 0; i < CMT_NR_IRQ; i++) {
-        sysbus_connect_irq(cmt, i, s->irq[irqbase + i]);
+        sysbus_connect_irq(cmt, i,
+                           qdev_get_gpio_in(DEVICE(&s->icu), irqbase + i));
     }
     sysbus_mmio_map(cmt, 0, RX62N_CMT_BASE + unit * 0x10);
 }
@@ -209,7 +208,8 @@ static void register_sci(RX62NState *s, int unit)
 
     irqbase = RX62N_SCI_IRQ + SCI_NR_IRQ * unit;
     for (i = 0; i < SCI_NR_IRQ; i++) {
-        sysbus_connect_irq(sci, i, s->irq[irqbase + i]);
+        sysbus_connect_irq(sci, i,
+                           qdev_get_gpio_in(DEVICE(&s->icu), irqbase + i));
     }
     sysbus_mmio_map(sci, 0, RX62N_SCI_BASE + unit * 0x08);
 }
-- 
2.41.0

[PATCH v4 04/12] hw/i386/q35: Realize LPC PCI function before accessing it

[Philippe Mathieu-Daudé]

We should not wire IRQs on unrealized device.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/i386/pc_q35.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 7ca3f465e0..b7c69d55d6 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -248,13 +248,13 @@ static void pc_q35_init(MachineState *machine)
     /* create ISA bus */
     lpc = pci_new_multifunction(PCI_DEVFN(ICH9_LPC_DEV, ICH9_LPC_FUNC),
                                 TYPE_ICH9_LPC_DEVICE);
-    qdev_prop_set_bit(DEVICE(lpc), "smm-enabled",
-                      x86_machine_is_smm_enabled(x86ms));
     lpc_dev = DEVICE(lpc);
+    qdev_prop_set_bit(lpc_dev, "smm-enabled",
+                      x86_machine_is_smm_enabled(x86ms));
+    pci_realize_and_unref(lpc, host_bus, &error_fatal);
     for (i = 0; i < IOAPIC_NUM_PINS; i++) {
         qdev_connect_gpio_out_named(lpc_dev, ICH9_GPIO_GSI, i, x86ms->gsi[i]);
     }
-    pci_realize_and_unref(lpc, host_bus, &error_fatal);
 
     rtc_state = ISA_DEVICE(object_resolve_path_component(OBJECT(lpc), "rtc"));
 
-- 
2.41.0

Re: [PATCH v4 04/12] hw/i386/q35: Realize LPC PCI function before accessing it

[Damien Hedde]

On 2/13/24 14:03, Philippe Mathieu-Daudé wrote:
> We should not wire IRQs on unrealized device.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> > ---
>   hw/i386/pc_q35.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index 7ca3f465e0..b7c69d55d6 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -248,13 +248,13 @@ static void pc_q35_init(MachineState *machine)
>       /* create ISA bus */
>       lpc = pci_new_multifunction(PCI_DEVFN(ICH9_LPC_DEV, ICH9_LPC_FUNC),
>                                   TYPE_ICH9_LPC_DEVICE);
> -    qdev_prop_set_bit(DEVICE(lpc), "smm-enabled",
> -                      x86_machine_is_smm_enabled(x86ms));
>       lpc_dev = DEVICE(lpc);
> +    qdev_prop_set_bit(lpc_dev, "smm-enabled",
> +                      x86_machine_is_smm_enabled(x86ms));
> +    pci_realize_and_unref(lpc, host_bus, &error_fatal);
>       for (i = 0; i < IOAPIC_NUM_PINS; i++) {
>           qdev_connect_gpio_out_named(lpc_dev, ICH9_GPIO_GSI, i, x86ms->gsi[i]);
>       }
> -    pci_realize_and_unref(lpc, host_bus, &error_fatal);
>   
>       rtc_state = ISA_DEVICE(object_resolve_path_component(OBJECT(lpc), "rtc"));
>   

Reviewed-by: Damien Hedde <dhedde@kalrayinc.com>

Re: [PATCH v4 04/12] hw/i386/q35: Realize LPC PCI function before accessing it

[BALATON Zoltan]

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

On Tue, 13 Feb 2024, Philippe Mathieu-Daudé wrote:
> We should not wire IRQs on unrealized device.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Reviewed-by: BALATON Zoltan <balaton@eik.bme.hu>

> ---
> hw/i386/pc_q35.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index 7ca3f465e0..b7c69d55d6 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -248,13 +248,13 @@ static void pc_q35_init(MachineState *machine)
>     /* create ISA bus */
>     lpc = pci_new_multifunction(PCI_DEVFN(ICH9_LPC_DEV, ICH9_LPC_FUNC),
>                                 TYPE_ICH9_LPC_DEVICE);
> -    qdev_prop_set_bit(DEVICE(lpc), "smm-enabled",
> -                      x86_machine_is_smm_enabled(x86ms));
>     lpc_dev = DEVICE(lpc);
> +    qdev_prop_set_bit(lpc_dev, "smm-enabled",
> +                      x86_machine_is_smm_enabled(x86ms));
> +    pci_realize_and_unref(lpc, host_bus, &error_fatal);
>     for (i = 0; i < IOAPIC_NUM_PINS; i++) {
>         qdev_connect_gpio_out_named(lpc_dev, ICH9_GPIO_GSI, i, x86ms->gsi[i]);
>     }
> -    pci_realize_and_unref(lpc, host_bus, &error_fatal);
>
>     rtc_state = ISA_DEVICE(object_resolve_path_component(OBJECT(lpc), "rtc"));
>
>

[PATCH v4 05/12] hw/ppc/prep: Realize ISA bridge before accessing it

[Philippe Mathieu-Daudé]

We should not wire IRQs on unrealized device.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/ppc/prep.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/ppc/prep.c b/hw/ppc/prep.c
index 1a6cd05c61..4eb5477069 100644
--- a/hw/ppc/prep.c
+++ b/hw/ppc/prep.c
@@ -278,9 +278,9 @@ static void ibm_40p_init(MachineState *machine)
 
     /* PCI -> ISA bridge */
     i82378_dev = DEVICE(pci_new(PCI_DEVFN(11, 0), "i82378"));
+    qdev_realize_and_unref(i82378_dev, BUS(pci_bus), &error_fatal);
     qdev_connect_gpio_out(i82378_dev, 0,
                           qdev_get_gpio_in(DEVICE(cpu), PPC6xx_INPUT_INT));
-    qdev_realize_and_unref(i82378_dev, BUS(pci_bus), &error_fatal);
 
     sysbus_connect_irq(pcihost, 0, qdev_get_gpio_in(i82378_dev, 15));
     isa_bus = ISA_BUS(qdev_get_child_bus(i82378_dev, "isa.0"));
-- 
2.41.0

[PATCH v4 06/12] hw/misc/macio: Realize IDE controller before accessing it

[Philippe Mathieu-Daudé]

We should not wire IRQs on unrealized device.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/misc/macio/macio.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/hw/misc/macio/macio.c b/hw/misc/macio/macio.c
index c9f22f8515..3f449f91c0 100644
--- a/hw/misc/macio/macio.c
+++ b/hw/misc/macio/macio.c
@@ -123,14 +123,17 @@ static bool macio_realize_ide(MacIOState *s, MACIOIDEState *ide,
 {
     SysBusDevice *sbd = SYS_BUS_DEVICE(ide);
 
-    sysbus_connect_irq(sbd, 0, irq0);
-    sysbus_connect_irq(sbd, 1, irq1);
     qdev_prop_set_uint32(DEVICE(ide), "channel", dmaid);
     object_property_set_link(OBJECT(ide), "dbdma", OBJECT(&s->dbdma),
                              &error_abort);
     macio_ide_register_dma(ide);
+    if (!qdev_realize(DEVICE(ide), BUS(&s->macio_bus), errp)) {
+        return false;
+    }
+    sysbus_connect_irq(sbd, 0, irq0);
+    sysbus_connect_irq(sbd, 1, irq1);
 
-    return qdev_realize(DEVICE(ide), BUS(&s->macio_bus), errp);
+    return true;
 }
 
 static void macio_oldworld_realize(PCIDevice *d, Error **errp)
-- 
2.41.0

Re: [PATCH v4 06/12] hw/misc/macio: Realize IDE controller before accessing it

[BALATON Zoltan]

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

On Tue, 13 Feb 2024, Philippe Mathieu-Daudé wrote:
> We should not wire IRQs on unrealized device.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

Reviewed-by: BALATON Zoltan <balaton@eik.bme.hu>

> ---
> hw/misc/macio/macio.c | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/hw/misc/macio/macio.c b/hw/misc/macio/macio.c
> index c9f22f8515..3f449f91c0 100644
> --- a/hw/misc/macio/macio.c
> +++ b/hw/misc/macio/macio.c
> @@ -123,14 +123,17 @@ static bool macio_realize_ide(MacIOState *s, MACIOIDEState *ide,
> {
>     SysBusDevice *sbd = SYS_BUS_DEVICE(ide);
>
> -    sysbus_connect_irq(sbd, 0, irq0);
> -    sysbus_connect_irq(sbd, 1, irq1);
>     qdev_prop_set_uint32(DEVICE(ide), "channel", dmaid);
>     object_property_set_link(OBJECT(ide), "dbdma", OBJECT(&s->dbdma),
>                              &error_abort);
>     macio_ide_register_dma(ide);
> +    if (!qdev_realize(DEVICE(ide), BUS(&s->macio_bus), errp)) {
> +        return false;
> +    }
> +    sysbus_connect_irq(sbd, 0, irq0);
> +    sysbus_connect_irq(sbd, 1, irq1);
>
> -    return qdev_realize(DEVICE(ide), BUS(&s->macio_bus), errp);
> +    return true;
> }
>
> static void macio_oldworld_realize(PCIDevice *d, Error **errp)
>

[PATCH v4 07/12] hw/sh4/r2d: Realize IDE controller before accessing it

[Philippe Mathieu-Daudé]

We should not wire IRQs on unrealized device.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Yoshinori Sato <ysato@users.sourceforge.jp>
---
 hw/sh4/r2d.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/sh4/r2d.c b/hw/sh4/r2d.c
index e9f316a6ce..c73e8f49b8 100644
--- a/hw/sh4/r2d.c
+++ b/hw/sh4/r2d.c
@@ -285,9 +285,9 @@ static void r2d_init(MachineState *machine)
     dinfo = drive_get(IF_IDE, 0, 0);
     dev = qdev_new("mmio-ide");
     busdev = SYS_BUS_DEVICE(dev);
-    sysbus_connect_irq(busdev, 0, irq[CF_IDE]);
     qdev_prop_set_uint32(dev, "shift", 1);
     sysbus_realize_and_unref(busdev, &error_fatal);
+    sysbus_connect_irq(busdev, 0, irq[CF_IDE]);
     sysbus_mmio_map(busdev, 0, 0x14001000);
     sysbus_mmio_map(busdev, 1, 0x1400080c);
     mmio_ide_init_drives(dev, dinfo, NULL);
-- 
2.41.0

Re: [PATCH v4 07/12] hw/sh4/r2d: Realize IDE controller before accessing it

[Michael Tokarev]

On 13.02.2024 16:03, Philippe Mathieu-Daudé wrote:
> We should not wire IRQs on unrealized device.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> Reviewed-by: Yoshinori Sato <ysato@users.sourceforge.jp>
> ---
>   hw/sh4/r2d.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/sh4/r2d.c b/hw/sh4/r2d.c
> index e9f316a6ce..c73e8f49b8 100644
> --- a/hw/sh4/r2d.c
> +++ b/hw/sh4/r2d.c
> @@ -285,9 +285,9 @@ static void r2d_init(MachineState *machine)
>       dinfo = drive_get(IF_IDE, 0, 0);
>       dev = qdev_new("mmio-ide");
>       busdev = SYS_BUS_DEVICE(dev);
> -    sysbus_connect_irq(busdev, 0, irq[CF_IDE]);
>       qdev_prop_set_uint32(dev, "shift", 1);
>       sysbus_realize_and_unref(busdev, &error_fatal);
> +    sysbus_connect_irq(busdev, 0, irq[CF_IDE]);
>       sysbus_mmio_map(busdev, 0, 0x14001000);
>       sysbus_mmio_map(busdev, 1, 0x1400080c);
>       mmio_ide_init_drives(dev, dinfo, NULL);

Should we pick this up for stable-9.0 & -9.1 series?

Thanks,

/mjt

Re: [PATCH v4 07/12] hw/sh4/r2d: Realize IDE controller before accessing it

[Bernhard Beschow]

Am 22. Oktober 2024 14:53:31 UTC schrieb Michael Tokarev <mjt@tls.msk.ru>:
>On 13.02.2024 16:03, Philippe Mathieu-Daudé wrote:
>> We should not wire IRQs on unrealized device.
>> 
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
>> Reviewed-by: Yoshinori Sato <ysato@users.sourceforge.jp>
>> ---
>>   hw/sh4/r2d.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/hw/sh4/r2d.c b/hw/sh4/r2d.c
>> index e9f316a6ce..c73e8f49b8 100644
>> --- a/hw/sh4/r2d.c
>> +++ b/hw/sh4/r2d.c
>> @@ -285,9 +285,9 @@ static void r2d_init(MachineState *machine)
>>       dinfo = drive_get(IF_IDE, 0, 0);
>>       dev = qdev_new("mmio-ide");
>>       busdev = SYS_BUS_DEVICE(dev);
>> -    sysbus_connect_irq(busdev, 0, irq[CF_IDE]);
>>       qdev_prop_set_uint32(dev, "shift", 1);
>>       sysbus_realize_and_unref(busdev, &error_fatal);
>> +    sysbus_connect_irq(busdev, 0, irq[CF_IDE]);
>>       sysbus_mmio_map(busdev, 0, 0x14001000);
>>       sysbus_mmio_map(busdev, 1, 0x1400080c);
>>       mmio_ide_init_drives(dev, dinfo, NULL);
>
>Should we pick this up for stable-9.0 & -9.1 series?

IMO yes, because it fixes a regression.

Best regards,
Bernhard

>
>Thanks,
>
>/mjt

[PATCH v4 08/12] hw/sparc/sun4m: Realize DMA controller before accessing it

[Philippe Mathieu-Daudé]

We should not wire IRQs on unrealized device.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/sparc/sun4m.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/hw/sparc/sun4m.c b/hw/sparc/sun4m.c
index e782c8ec7a..d52e6a7213 100644
--- a/hw/sparc/sun4m.c
+++ b/hw/sparc/sun4m.c
@@ -312,13 +312,11 @@ static void *sparc32_dma_init(hwaddr dma_base,
     dma = qdev_new(TYPE_SPARC32_DMA);
     espdma = SPARC32_ESPDMA_DEVICE(object_resolve_path_component(
                                    OBJECT(dma), "espdma"));
-    sysbus_connect_irq(SYS_BUS_DEVICE(espdma), 0, espdma_irq);
 
     esp = SYSBUS_ESP(object_resolve_path_component(OBJECT(espdma), "esp"));
 
     ledma = SPARC32_LEDMA_DEVICE(object_resolve_path_component(
                                  OBJECT(dma), "ledma"));
-    sysbus_connect_irq(SYS_BUS_DEVICE(ledma), 0, ledma_irq);
 
     lance = SYSBUS_PCNET(object_resolve_path_component(
                          OBJECT(ledma), "lance"));
@@ -332,6 +330,11 @@ static void *sparc32_dma_init(hwaddr dma_base,
     }
 
     sysbus_realize_and_unref(SYS_BUS_DEVICE(dma), &error_fatal);
+
+    sysbus_connect_irq(SYS_BUS_DEVICE(espdma), 0, espdma_irq);
+
+    sysbus_connect_irq(SYS_BUS_DEVICE(ledma), 0, ledma_irq);
+
     sysbus_mmio_map(SYS_BUS_DEVICE(dma), 0, dma_base);
 
     sysbus_mmio_map(SYS_BUS_DEVICE(esp), 0, esp_base);
-- 
2.41.0

[PATCH v4 09/12] hw/sparc/leon3: Realize GRLIB IRQ controller before accessing it

[Philippe Mathieu-Daudé]

We should not wire IRQs on unrealized device.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/sparc/leon3.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/sparc/leon3.c b/hw/sparc/leon3.c
index 2dfb742566..0df5fc949d 100644
--- a/hw/sparc/leon3.c
+++ b/hw/sparc/leon3.c
@@ -263,10 +263,10 @@ static void leon3_generic_hw_init(MachineState *machine)
     irqmpdev = qdev_new(TYPE_GRLIB_IRQMP);
     qdev_init_gpio_in_named_with_opaque(DEVICE(cpu), leon3_set_pil_in,
                                         env, "pil", 1);
-    qdev_connect_gpio_out_named(irqmpdev, "grlib-irq", 0,
-                                qdev_get_gpio_in_named(DEVICE(cpu), "pil", 0));
     sysbus_realize_and_unref(SYS_BUS_DEVICE(irqmpdev), &error_fatal);
     sysbus_mmio_map(SYS_BUS_DEVICE(irqmpdev), 0, LEON3_IRQMP_OFFSET);
+    qdev_connect_gpio_out_named(irqmpdev, "grlib-irq", 0,
+                                qdev_get_gpio_in_named(DEVICE(cpu), "pil", 0));
     env->irq_manager = irqmpdev;
     env->qemu_irq_ack = leon3_irq_manager;
     grlib_apb_pnp_add_entry(apb_pnp, LEON3_IRQMP_OFFSET, 0xFFF,
-- 
2.41.0

[PATCH v4 10/12] hw/sparc/leon3: Pass DeviceState opaque argument to leon3_set_pil_in()

[Philippe Mathieu-Daudé]

By passing a DeviceState context to a QDev IRQ handler,
we can simplify and use qdev_init_gpio_in_named() instead
of qdev_init_gpio_in_named_with_opaque().

Suggested-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/sparc/leon3.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/hw/sparc/leon3.c b/hw/sparc/leon3.c
index 0df5fc949d..d2be900988 100644
--- a/hw/sparc/leon3.c
+++ b/hw/sparc/leon3.c
@@ -175,9 +175,10 @@ static void leon3_irq_ack(CPUSPARCState *env, int intno)
  */
 static void leon3_set_pil_in(void *opaque, int n, int level)
 {
-    CPUSPARCState *env = opaque;
+    DeviceState *cpu = opaque;
+    CPUState *cs = CPU(cpu);
+    CPUSPARCState *env = cpu_env(cs);
     uint32_t pil_in = level;
-    CPUState *cs;
 
     assert(env != NULL);
 
@@ -193,7 +194,6 @@ static void leon3_set_pil_in(void *opaque, int n, int level)
 
                 env->interrupt_index = TT_EXTINT | i;
                 if (old_interrupt != env->interrupt_index) {
-                    cs = env_cpu(env);
                     trace_leon3_set_irq(i);
                     cpu_interrupt(cs, CPU_INTERRUPT_HARD);
                 }
@@ -201,7 +201,6 @@ static void leon3_set_pil_in(void *opaque, int n, int level)
             }
         }
     } else if (!env->pil_in && (env->interrupt_index & ~15) == TT_EXTINT) {
-        cs = env_cpu(env);
         trace_leon3_reset_irq(env->interrupt_index & 15);
         env->interrupt_index = 0;
         cpu_reset_interrupt(cs, CPU_INTERRUPT_HARD);
@@ -261,8 +260,7 @@ static void leon3_generic_hw_init(MachineState *machine)
 
     /* Allocate IRQ manager */
     irqmpdev = qdev_new(TYPE_GRLIB_IRQMP);
-    qdev_init_gpio_in_named_with_opaque(DEVICE(cpu), leon3_set_pil_in,
-                                        env, "pil", 1);
+    qdev_init_gpio_in_named(DEVICE(cpu), leon3_set_pil_in, "pil", 1);
     sysbus_realize_and_unref(SYS_BUS_DEVICE(irqmpdev), &error_fatal);
     sysbus_mmio_map(SYS_BUS_DEVICE(irqmpdev), 0, LEON3_IRQMP_OFFSET);
     qdev_connect_gpio_out_named(irqmpdev, "grlib-irq", 0,
-- 
2.41.0

Re: [PATCH v4 10/12] hw/sparc/leon3: Pass DeviceState opaque argument to leon3_set_pil_in()

[Mark Cave-Ayland]

On 13/02/2024 13:03, Philippe Mathieu-Daudé wrote:

> By passing a DeviceState context to a QDev IRQ handler,
> we can simplify and use qdev_init_gpio_in_named() instead
> of qdev_init_gpio_in_named_with_opaque().
> 
> Suggested-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   hw/sparc/leon3.c | 10 ++++------
>   1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/sparc/leon3.c b/hw/sparc/leon3.c
> index 0df5fc949d..d2be900988 100644
> --- a/hw/sparc/leon3.c
> +++ b/hw/sparc/leon3.c
> @@ -175,9 +175,10 @@ static void leon3_irq_ack(CPUSPARCState *env, int intno)
>    */
>   static void leon3_set_pil_in(void *opaque, int n, int level)
>   {
> -    CPUSPARCState *env = opaque;
> +    DeviceState *cpu = opaque;
> +    CPUState *cs = CPU(cpu);
> +    CPUSPARCState *env = cpu_env(cs);
>       uint32_t pil_in = level;
> -    CPUState *cs;
>   
>       assert(env != NULL);
>   
> @@ -193,7 +194,6 @@ static void leon3_set_pil_in(void *opaque, int n, int level)
>   
>                   env->interrupt_index = TT_EXTINT | i;
>                   if (old_interrupt != env->interrupt_index) {
> -                    cs = env_cpu(env);
>                       trace_leon3_set_irq(i);
>                       cpu_interrupt(cs, CPU_INTERRUPT_HARD);
>                   }
> @@ -201,7 +201,6 @@ static void leon3_set_pil_in(void *opaque, int n, int level)
>               }
>           }
>       } else if (!env->pil_in && (env->interrupt_index & ~15) == TT_EXTINT) {
> -        cs = env_cpu(env);
>           trace_leon3_reset_irq(env->interrupt_index & 15);
>           env->interrupt_index = 0;
>           cpu_reset_interrupt(cs, CPU_INTERRUPT_HARD);
> @@ -261,8 +260,7 @@ static void leon3_generic_hw_init(MachineState *machine)
>   
>       /* Allocate IRQ manager */
>       irqmpdev = qdev_new(TYPE_GRLIB_IRQMP);
> -    qdev_init_gpio_in_named_with_opaque(DEVICE(cpu), leon3_set_pil_in,
> -                                        env, "pil", 1);
> +    qdev_init_gpio_in_named(DEVICE(cpu), leon3_set_pil_in, "pil", 1);
>       sysbus_realize_and_unref(SYS_BUS_DEVICE(irqmpdev), &error_fatal);
>       sysbus_mmio_map(SYS_BUS_DEVICE(irqmpdev), 0, LEON3_IRQMP_OFFSET);
>       qdev_connect_gpio_out_named(irqmpdev, "grlib-irq", 0,

Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>


ATB,

Mark.

[PATCH v4 11/12] hw/sparc/leon3: Initialize GPIO before realizing CPU devices

[Philippe Mathieu-Daudé]

Inline cpu_create() in order to call
qdev_init_gpio_in_named_with_opaque()
before the CPU is realized.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/sparc/leon3.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/sparc/leon3.c b/hw/sparc/leon3.c
index d2be900988..d0ff4a2892 100644
--- a/hw/sparc/leon3.c
+++ b/hw/sparc/leon3.c
@@ -233,7 +233,9 @@ static void leon3_generic_hw_init(MachineState *machine)
     APBPnp *apb_pnp;
 
     /* Init CPU */
-    cpu = SPARC_CPU(cpu_create(machine->cpu_type));
+    cpu = SPARC_CPU(object_new(machine->cpu_type));
+    qdev_init_gpio_in_named(DEVICE(cpu), leon3_set_pil_in, "pil", 1);
+    qdev_realize(DEVICE(cpu), NULL, &error_fatal);
     env = &cpu->env;
 
     cpu_sparc_set_id(env, 0);
@@ -260,7 +262,6 @@ static void leon3_generic_hw_init(MachineState *machine)
 
     /* Allocate IRQ manager */
     irqmpdev = qdev_new(TYPE_GRLIB_IRQMP);
-    qdev_init_gpio_in_named(DEVICE(cpu), leon3_set_pil_in, "pil", 1);
     sysbus_realize_and_unref(SYS_BUS_DEVICE(irqmpdev), &error_fatal);
     sysbus_mmio_map(SYS_BUS_DEVICE(irqmpdev), 0, LEON3_IRQMP_OFFSET);
     qdev_connect_gpio_out_named(irqmpdev, "grlib-irq", 0,
-- 
2.41.0

Re: [PATCH v4 11/12] hw/sparc/leon3: Initialize GPIO before realizing CPU devices

[Mark Cave-Ayland]

On 13/02/2024 13:03, Philippe Mathieu-Daudé wrote:

> Inline cpu_create() in order to call
> qdev_init_gpio_in_named_with_opaque()
> before the CPU is realized.

Note that with the previous new patch included in the series, 
qdev_init_gpio_in_named_with_opaque() should be replaced by
qdev_init_gpio_in_named() in the commit message above instead.

> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>   hw/sparc/leon3.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/sparc/leon3.c b/hw/sparc/leon3.c
> index d2be900988..d0ff4a2892 100644
> --- a/hw/sparc/leon3.c
> +++ b/hw/sparc/leon3.c
> @@ -233,7 +233,9 @@ static void leon3_generic_hw_init(MachineState *machine)
>       APBPnp *apb_pnp;
>   
>       /* Init CPU */
> -    cpu = SPARC_CPU(cpu_create(machine->cpu_type));
> +    cpu = SPARC_CPU(object_new(machine->cpu_type));
> +    qdev_init_gpio_in_named(DEVICE(cpu), leon3_set_pil_in, "pil", 1);
> +    qdev_realize(DEVICE(cpu), NULL, &error_fatal);
>       env = &cpu->env;
>   
>       cpu_sparc_set_id(env, 0);
> @@ -260,7 +262,6 @@ static void leon3_generic_hw_init(MachineState *machine)
>   
>       /* Allocate IRQ manager */
>       irqmpdev = qdev_new(TYPE_GRLIB_IRQMP);
> -    qdev_init_gpio_in_named(DEVICE(cpu), leon3_set_pil_in, "pil", 1);
>       sysbus_realize_and_unref(SYS_BUS_DEVICE(irqmpdev), &error_fatal);
>       sysbus_mmio_map(SYS_BUS_DEVICE(irqmpdev), 0, LEON3_IRQMP_OFFSET);
>       qdev_connect_gpio_out_named(irqmpdev, "grlib-irq", 0,


ATB,

Mark.

[PATCH v4 12/12] hw/sparc64/cpu: Initialize GPIO before realizing CPU devices

[Philippe Mathieu-Daudé]

Inline cpu_create() in order to call
qdev_init_gpio_in_named_with_opaque()
before the CPU is realized.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/sparc64/sparc64.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/sparc64/sparc64.c b/hw/sparc64/sparc64.c
index 72f0849f50..3091cde586 100644
--- a/hw/sparc64/sparc64.c
+++ b/hw/sparc64/sparc64.c
@@ -24,6 +24,7 @@
 
 
 #include "qemu/osdep.h"
+#include "qapi/error.h"
 #include "cpu.h"
 #include "hw/boards.h"
 #include "hw/sparc/sparc64.h"
@@ -271,9 +272,10 @@ SPARCCPU *sparc64_cpu_devinit(const char *cpu_type, uint64_t prom_addr)
     uint32_t  stick_frequency = 100 * 1000000;
     uint32_t hstick_frequency = 100 * 1000000;
 
-    cpu = SPARC_CPU(cpu_create(cpu_type));
+    cpu = SPARC_CPU(object_new(cpu_type));
     qdev_init_gpio_in_named(DEVICE(cpu), sparc64_cpu_set_ivec_irq,
                             "ivec-irq", IVEC_MAX);
+    qdev_realize(DEVICE(cpu), NULL, &error_fatal);
     env = &cpu->env;
 
     env->tick = cpu_timer_create("tick", cpu, tick_irq,
-- 
2.41.0

Re: [PATCH v4 12/12] hw/sparc64/cpu: Initialize GPIO before realizing CPU devices

[Damien Hedde]

On 2/13/24 14:03, Philippe Mathieu-Daudé wrote:
> Inline cpu_create() in order to call
> qdev_init_gpio_in_named_with_opaque()
> before the CPU is realized.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>   hw/sparc64/sparc64.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/sparc64/sparc64.c b/hw/sparc64/sparc64.c
> index 72f0849f50..3091cde586 100644
> --- a/hw/sparc64/sparc64.c
> +++ b/hw/sparc64/sparc64.c
> @@ -24,6 +24,7 @@
>   
>   
>   #include "qemu/osdep.h"
> +#include "qapi/error.h"
>   #include "cpu.h"
>   #include "hw/boards.h"
>   #include "hw/sparc/sparc64.h"
> @@ -271,9 +272,10 @@ SPARCCPU *sparc64_cpu_devinit(const char *cpu_type, uint64_t prom_addr)
>       uint32_t  stick_frequency = 100 * 1000000;
>       uint32_t hstick_frequency = 100 * 1000000;
>   
> -    cpu = SPARC_CPU(cpu_create(cpu_type));
> +    cpu = SPARC_CPU(object_new(cpu_type));
>       qdev_init_gpio_in_named(DEVICE(cpu), sparc64_cpu_set_ivec_irq,
>                               "ivec-irq", IVEC_MAX);
> +    qdev_realize(DEVICE(cpu), NULL, &error_fatal);
>       env = &cpu->env;
>   
>       env->tick = cpu_timer_create("tick", cpu, tick_irq,

Reviewed-by: Damien Hedde <dhedde@kalrayinc.com>

Re: [PATCH v4 00/12] hw: Strengthen SysBus & QBus API

[Richard Henderson]

On 2/13/24 03:03, Philippe Mathieu-Daudé wrote:
> Philippe Mathieu-Daudé (12):
>    hw/ide/ich9: Use AHCIPCIState typedef
>    hw/rx/rx62n: Reduce inclusion of 'qemu/units.h'
>    hw/rx/rx62n: Only call qdev_get_gpio_in() when necessary
>    hw/i386/q35: Realize LPC PCI function before accessing it
>    hw/ppc/prep: Realize ISA bridge before accessing it
>    hw/misc/macio: Realize IDE controller before accessing it
>    hw/sh4/r2d: Realize IDE controller before accessing it
>    hw/sparc/sun4m: Realize DMA controller before accessing it
>    hw/sparc/leon3: Realize GRLIB IRQ controller before accessing it
>    hw/sparc/leon3: Pass DeviceState opaque argument to leon3_set_pil_in()
>    hw/sparc/leon3: Initialize GPIO before realizing CPU devices
>    hw/sparc64/cpu: Initialize GPIO before realizing CPU devices

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH v4 00/12] hw: Strengthen SysBus & QBus API

[Philippe Mathieu-Daudé]

On 13/2/24 14:03, Philippe Mathieu-Daudé wrote:

> Philippe Mathieu-Daudé (12):
>    hw/ide/ich9: Use AHCIPCIState typedef
>    hw/rx/rx62n: Reduce inclusion of 'qemu/units.h'
>    hw/rx/rx62n: Only call qdev_get_gpio_in() when necessary
>    hw/i386/q35: Realize LPC PCI function before accessing it
>    hw/ppc/prep: Realize ISA bridge before accessing it
>    hw/misc/macio: Realize IDE controller before accessing it
>    hw/sh4/r2d: Realize IDE controller before accessing it
>    hw/sparc/sun4m: Realize DMA controller before accessing it
>    hw/sparc/leon3: Realize GRLIB IRQ controller before accessing it
>    hw/sparc/leon3: Pass DeviceState opaque argument to leon3_set_pil_in()
>    hw/sparc/leon3: Initialize GPIO before realizing CPU devices
>    hw/sparc64/cpu: Initialize GPIO before realizing CPU devices

Patches 2-8,12 queued.