qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3 00/11] hw: Strengthen SysBus & QBus API
@ 2024-02-08 18:12 Philippe Mathieu-Daudé
  2024-02-08 18:12 ` [PATCH v3 01/11] hw/ide/ich9: Use AHCIPCIState typedef Philippe Mathieu-Daudé
                   ` (11 more replies)
  0 siblings, 12 replies; 45+ messages in thread
From: Philippe Mathieu-Daudé @ 2024-02-08 18:12 UTC (permalink / raw)
  To: qemu-devel
  Cc: Mark Cave-Ayland, Zhao Liu, Paolo Bonzini, qemu-block, John Snow,
	qemu-ppc, Eduardo Habkost, Richard Henderson, Markus Armbruster,
	Philippe Mathieu-Daudé

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é (11):
  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/pc_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: 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      |  2 +-
 hw/ide/ich.c          |  6 +++---
 hw/misc/macio/macio.c |  8 +++++---
 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      | 11 ++++++-----
 hw/sparc/sun4m.c      |  7 +++++--
 hw/sparc64/sparc64.c  |  4 +++-
 11 files changed, 35 insertions(+), 27 deletions(-)

-- 
2.41.0



^ permalink raw reply	[flat|nested] 45+ messages in thread

* [PATCH v3 01/11] hw/ide/ich9: Use AHCIPCIState typedef
  2024-02-08 18:12 [PATCH v3 00/11] hw: Strengthen SysBus & QBus API Philippe Mathieu-Daudé
@ 2024-02-08 18:12 ` Philippe Mathieu-Daudé
  2024-02-09 11:28   ` Peter Maydell
  2024-02-08 18:12 ` [PATCH v3 02/11] hw/rx/rx62n: Reduce inclusion of 'qemu/units.h' Philippe Mathieu-Daudé
                   ` (10 subsequent siblings)
  11 siblings, 1 reply; 45+ messages in thread
From: Philippe Mathieu-Daudé @ 2024-02-08 18:12 UTC (permalink / raw)
  To: qemu-devel
  Cc: Mark Cave-Ayland, Zhao Liu, Paolo Bonzini, qemu-block, John Snow,
	qemu-ppc, Eduardo Habkost, Richard Henderson, Markus Armbruster,
	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>
---
 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



^ permalink raw reply related	[flat|nested] 45+ messages in thread

* [PATCH v3 02/11] hw/rx/rx62n: Reduce inclusion of 'qemu/units.h'
  2024-02-08 18:12 [PATCH v3 00/11] hw: Strengthen SysBus & QBus API Philippe Mathieu-Daudé
  2024-02-08 18:12 ` [PATCH v3 01/11] hw/ide/ich9: Use AHCIPCIState typedef Philippe Mathieu-Daudé
@ 2024-02-08 18:12 ` Philippe Mathieu-Daudé
  2024-02-09 11:28   ` Peter Maydell
  2024-02-12 11:24   ` Yoshinori Sato
  2024-02-08 18:12 ` [PATCH v3 03/11] hw/rx/rx62n: Only call qdev_get_gpio_in() when necessary Philippe Mathieu-Daudé
                   ` (9 subsequent siblings)
  11 siblings, 2 replies; 45+ messages in thread
From: Philippe Mathieu-Daudé @ 2024-02-08 18:12 UTC (permalink / raw)
  To: qemu-devel
  Cc: Mark Cave-Ayland, Zhao Liu, Paolo Bonzini, qemu-block, John Snow,
	qemu-ppc, Eduardo Habkost, Richard Henderson, Markus Armbruster,
	Philippe Mathieu-Daudé, Yoshinori Sato

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



^ permalink raw reply related	[flat|nested] 45+ messages in thread

* [PATCH v3 03/11] hw/rx/rx62n: Only call qdev_get_gpio_in() when necessary
  2024-02-08 18:12 [PATCH v3 00/11] hw: Strengthen SysBus & QBus API Philippe Mathieu-Daudé
  2024-02-08 18:12 ` [PATCH v3 01/11] hw/ide/ich9: Use AHCIPCIState typedef Philippe Mathieu-Daudé
  2024-02-08 18:12 ` [PATCH v3 02/11] hw/rx/rx62n: Reduce inclusion of 'qemu/units.h' Philippe Mathieu-Daudé
@ 2024-02-08 18:12 ` Philippe Mathieu-Daudé
  2024-02-09 11:29   ` Peter Maydell
  2024-02-12 11:25   ` Yoshinori Sato
  2024-02-08 18:12 ` [PATCH v3 04/11] hw/i386/pc_q35: Realize LPC PCI function before accessing it Philippe Mathieu-Daudé
                   ` (8 subsequent siblings)
  11 siblings, 2 replies; 45+ messages in thread
From: Philippe Mathieu-Daudé @ 2024-02-08 18:12 UTC (permalink / raw)
  To: qemu-devel
  Cc: Mark Cave-Ayland, Zhao Liu, Paolo Bonzini, qemu-block, John Snow,
	qemu-ppc, Eduardo Habkost, Richard Henderson, Markus Armbruster,
	Philippe Mathieu-Daudé, Yoshinori Sato

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



^ permalink raw reply related	[flat|nested] 45+ messages in thread

* [PATCH v3 04/11] hw/i386/pc_q35: Realize LPC PCI function before accessing it
  2024-02-08 18:12 [PATCH v3 00/11] hw: Strengthen SysBus & QBus API Philippe Mathieu-Daudé
                   ` (2 preceding siblings ...)
  2024-02-08 18:12 ` [PATCH v3 03/11] hw/rx/rx62n: Only call qdev_get_gpio_in() when necessary Philippe Mathieu-Daudé
@ 2024-02-08 18:12 ` Philippe Mathieu-Daudé
  2024-02-08 18:29   ` BALATON Zoltan
  2024-02-08 18:12 ` [PATCH v3 05/11] hw/ppc/prep: Realize ISA bridge " Philippe Mathieu-Daudé
                   ` (7 subsequent siblings)
  11 siblings, 1 reply; 45+ messages in thread
From: Philippe Mathieu-Daudé @ 2024-02-08 18:12 UTC (permalink / raw)
  To: qemu-devel
  Cc: Mark Cave-Ayland, Zhao Liu, Paolo Bonzini, qemu-block, John Snow,
	qemu-ppc, Eduardo Habkost, Richard Henderson, Markus Armbruster,
	Philippe Mathieu-Daudé, Michael S. Tsirkin, Marcel Apfelbaum

We should not wire IRQs on unrealized device.

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

diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 7ca3f465e0..f67e5a55df 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -250,11 +250,11 @@ static void pc_q35_init(MachineState *machine)
                                 TYPE_ICH9_LPC_DEVICE);
     qdev_prop_set_bit(DEVICE(lpc), "smm-enabled",
                       x86_machine_is_smm_enabled(x86ms));
+    pci_realize_and_unref(lpc, host_bus, &error_fatal);
     lpc_dev = DEVICE(lpc);
     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



^ permalink raw reply related	[flat|nested] 45+ messages in thread

* [PATCH v3 05/11] hw/ppc/prep: Realize ISA bridge before accessing it
  2024-02-08 18:12 [PATCH v3 00/11] hw: Strengthen SysBus & QBus API Philippe Mathieu-Daudé
                   ` (3 preceding siblings ...)
  2024-02-08 18:12 ` [PATCH v3 04/11] hw/i386/pc_q35: Realize LPC PCI function before accessing it Philippe Mathieu-Daudé
@ 2024-02-08 18:12 ` Philippe Mathieu-Daudé
  2024-02-09 11:29   ` Peter Maydell
  2024-02-09 21:29   ` Mark Cave-Ayland
  2024-02-08 18:12 ` [PATCH v3 06/11] hw/misc/macio: Realize IDE controller " Philippe Mathieu-Daudé
                   ` (6 subsequent siblings)
  11 siblings, 2 replies; 45+ messages in thread
From: Philippe Mathieu-Daudé @ 2024-02-08 18:12 UTC (permalink / raw)
  To: qemu-devel
  Cc: Mark Cave-Ayland, Zhao Liu, Paolo Bonzini, qemu-block, John Snow,
	qemu-ppc, Eduardo Habkost, Richard Henderson, Markus Armbruster,
	Philippe Mathieu-Daudé, Hervé Poussineau

We should not wire IRQs on unrealized device.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 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



^ permalink raw reply related	[flat|nested] 45+ messages in thread

* [PATCH v3 06/11] hw/misc/macio: Realize IDE controller before accessing it
  2024-02-08 18:12 [PATCH v3 00/11] hw: Strengthen SysBus & QBus API Philippe Mathieu-Daudé
                   ` (4 preceding siblings ...)
  2024-02-08 18:12 ` [PATCH v3 05/11] hw/ppc/prep: Realize ISA bridge " Philippe Mathieu-Daudé
@ 2024-02-08 18:12 ` Philippe Mathieu-Daudé
  2024-02-08 18:33   ` BALATON Zoltan
  2024-02-09 21:32   ` Mark Cave-Ayland
  2024-02-08 18:12 ` [PATCH v3 07/11] hw/sh4/r2d: " Philippe Mathieu-Daudé
                   ` (5 subsequent siblings)
  11 siblings, 2 replies; 45+ messages in thread
From: Philippe Mathieu-Daudé @ 2024-02-08 18:12 UTC (permalink / raw)
  To: qemu-devel
  Cc: Mark Cave-Ayland, Zhao Liu, Paolo Bonzini, qemu-block, John Snow,
	qemu-ppc, Eduardo Habkost, Richard Henderson, Markus Armbruster,
	Philippe Mathieu-Daudé

We should not wire IRQs on unrealized device.

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

diff --git a/hw/misc/macio/macio.c b/hw/misc/macio/macio.c
index c9f22f8515..db662a2065 100644
--- a/hw/misc/macio/macio.c
+++ b/hw/misc/macio/macio.c
@@ -122,15 +122,17 @@ static bool macio_realize_ide(MacIOState *s, MACIOIDEState *ide,
                               Error **errp)
 {
     SysBusDevice *sbd = SYS_BUS_DEVICE(ide);
+    bool success;
 
-    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);
+    success = qdev_realize(DEVICE(ide), BUS(&s->macio_bus), errp);
+    sysbus_connect_irq(sbd, 0, irq0);
+    sysbus_connect_irq(sbd, 1, irq1);
 
-    return qdev_realize(DEVICE(ide), BUS(&s->macio_bus), errp);
+    return success;
 }
 
 static void macio_oldworld_realize(PCIDevice *d, Error **errp)
-- 
2.41.0



^ permalink raw reply related	[flat|nested] 45+ messages in thread

* [PATCH v3 07/11] hw/sh4/r2d: Realize IDE controller before accessing it
  2024-02-08 18:12 [PATCH v3 00/11] hw: Strengthen SysBus & QBus API Philippe Mathieu-Daudé
                   ` (5 preceding siblings ...)
  2024-02-08 18:12 ` [PATCH v3 06/11] hw/misc/macio: Realize IDE controller " Philippe Mathieu-Daudé
@ 2024-02-08 18:12 ` Philippe Mathieu-Daudé
  2024-02-09 11:30   ` Peter Maydell
                     ` (2 more replies)
  2024-02-08 18:12 ` [PATCH v3 08/11] hw/sparc/sun4m: Realize DMA " Philippe Mathieu-Daudé
                   ` (4 subsequent siblings)
  11 siblings, 3 replies; 45+ messages in thread
From: Philippe Mathieu-Daudé @ 2024-02-08 18:12 UTC (permalink / raw)
  To: qemu-devel
  Cc: Mark Cave-Ayland, Zhao Liu, Paolo Bonzini, qemu-block, John Snow,
	qemu-ppc, Eduardo Habkost, Richard Henderson, Markus Armbruster,
	Philippe Mathieu-Daudé, Yoshinori Sato, Magnus Damm

We should not wire IRQs on unrealized device.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 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



^ permalink raw reply related	[flat|nested] 45+ messages in thread

* [PATCH v3 08/11] hw/sparc/sun4m: Realize DMA controller before accessing it
  2024-02-08 18:12 [PATCH v3 00/11] hw: Strengthen SysBus & QBus API Philippe Mathieu-Daudé
                   ` (6 preceding siblings ...)
  2024-02-08 18:12 ` [PATCH v3 07/11] hw/sh4/r2d: " Philippe Mathieu-Daudé
@ 2024-02-08 18:12 ` Philippe Mathieu-Daudé
  2024-02-09 11:37   ` Peter Maydell
  2024-02-09 21:38   ` Mark Cave-Ayland
  2024-02-08 18:12 ` [PATCH v3 09/11] hw/sparc/leon3: Realize GRLIB IRQ " Philippe Mathieu-Daudé
                   ` (3 subsequent siblings)
  11 siblings, 2 replies; 45+ messages in thread
From: Philippe Mathieu-Daudé @ 2024-02-08 18:12 UTC (permalink / raw)
  To: qemu-devel
  Cc: Mark Cave-Ayland, Zhao Liu, Paolo Bonzini, qemu-block, John Snow,
	qemu-ppc, Eduardo Habkost, Richard Henderson, Markus Armbruster,
	Philippe Mathieu-Daudé, Artyom Tarasenko

We should not wire IRQs on unrealized device.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 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



^ permalink raw reply related	[flat|nested] 45+ messages in thread

* [PATCH v3 09/11] hw/sparc/leon3: Realize GRLIB IRQ controller before accessing it
  2024-02-08 18:12 [PATCH v3 00/11] hw: Strengthen SysBus & QBus API Philippe Mathieu-Daudé
                   ` (7 preceding siblings ...)
  2024-02-08 18:12 ` [PATCH v3 08/11] hw/sparc/sun4m: Realize DMA " Philippe Mathieu-Daudé
@ 2024-02-08 18:12 ` Philippe Mathieu-Daudé
  2024-02-09 11:38   ` Peter Maydell
  2024-02-09 21:39   ` Mark Cave-Ayland
  2024-02-08 18:12 ` [PATCH v3 10/11] hw/sparc/leon3: Initialize GPIO before realizing CPU devices Philippe Mathieu-Daudé
                   ` (2 subsequent siblings)
  11 siblings, 2 replies; 45+ messages in thread
From: Philippe Mathieu-Daudé @ 2024-02-08 18:12 UTC (permalink / raw)
  To: qemu-devel
  Cc: Mark Cave-Ayland, Zhao Liu, Paolo Bonzini, qemu-block, John Snow,
	qemu-ppc, Eduardo Habkost, Richard Henderson, Markus Armbruster,
	Philippe Mathieu-Daudé, Fabien Chouteau, Frederic Konrad,
	Artyom Tarasenko

We should not wire IRQs on unrealized device.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 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



^ permalink raw reply related	[flat|nested] 45+ messages in thread

* [PATCH v3 10/11] hw/sparc/leon3: Initialize GPIO before realizing CPU devices
  2024-02-08 18:12 [PATCH v3 00/11] hw: Strengthen SysBus & QBus API Philippe Mathieu-Daudé
                   ` (8 preceding siblings ...)
  2024-02-08 18:12 ` [PATCH v3 09/11] hw/sparc/leon3: Realize GRLIB IRQ " Philippe Mathieu-Daudé
@ 2024-02-08 18:12 ` Philippe Mathieu-Daudé
  2024-02-09 11:35   ` Peter Maydell
  2024-02-09 21:48   ` Mark Cave-Ayland
  2024-02-08 18:12 ` [PATCH v3 11/11] hw/sparc64/cpu: " Philippe Mathieu-Daudé
  2024-10-11  8:56 ` [PATCH v3 00/11] hw: Strengthen SysBus & QBus API Paolo Bonzini
  11 siblings, 2 replies; 45+ messages in thread
From: Philippe Mathieu-Daudé @ 2024-02-08 18:12 UTC (permalink / raw)
  To: qemu-devel
  Cc: Mark Cave-Ayland, Zhao Liu, Paolo Bonzini, qemu-block, John Snow,
	qemu-ppc, Eduardo Habkost, Richard Henderson, Markus Armbruster,
	Philippe Mathieu-Daudé, Fabien Chouteau, Frederic Konrad,
	Artyom Tarasenko

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>
---
 hw/sparc/leon3.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/hw/sparc/leon3.c b/hw/sparc/leon3.c
index 0df5fc949d..0e1d749306 100644
--- a/hw/sparc/leon3.c
+++ b/hw/sparc/leon3.c
@@ -234,8 +234,11 @@ 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));
     env = &cpu->env;
+    qdev_init_gpio_in_named_with_opaque(DEVICE(cpu), leon3_set_pil_in,
+                                        env, "pil", 1);
+    qdev_realize(DEVICE(cpu), NULL, &error_fatal);
 
     cpu_sparc_set_id(env, 0);
 
@@ -261,8 +264,6 @@ 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);
     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



^ permalink raw reply related	[flat|nested] 45+ messages in thread

* [PATCH v3 11/11] hw/sparc64/cpu: Initialize GPIO before realizing CPU devices
  2024-02-08 18:12 [PATCH v3 00/11] hw: Strengthen SysBus & QBus API Philippe Mathieu-Daudé
                   ` (9 preceding siblings ...)
  2024-02-08 18:12 ` [PATCH v3 10/11] hw/sparc/leon3: Initialize GPIO before realizing CPU devices Philippe Mathieu-Daudé
@ 2024-02-08 18:12 ` Philippe Mathieu-Daudé
  2024-02-09 11:34   ` Peter Maydell
  2024-02-09 21:49   ` Mark Cave-Ayland
  2024-10-11  8:56 ` [PATCH v3 00/11] hw: Strengthen SysBus & QBus API Paolo Bonzini
  11 siblings, 2 replies; 45+ messages in thread
From: Philippe Mathieu-Daudé @ 2024-02-08 18:12 UTC (permalink / raw)
  To: qemu-devel
  Cc: Mark Cave-Ayland, Zhao Liu, Paolo Bonzini, qemu-block, John Snow,
	qemu-ppc, Eduardo Habkost, Richard Henderson, Markus Armbruster,
	Philippe Mathieu-Daudé, Artyom Tarasenko

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



^ permalink raw reply related	[flat|nested] 45+ messages in thread

* Re: [PATCH v3 04/11] hw/i386/pc_q35: Realize LPC PCI function before accessing it
  2024-02-08 18:12 ` [PATCH v3 04/11] hw/i386/pc_q35: Realize LPC PCI function before accessing it Philippe Mathieu-Daudé
@ 2024-02-08 18:29   ` BALATON Zoltan
  0 siblings, 0 replies; 45+ messages in thread
From: BALATON Zoltan @ 2024-02-08 18:29 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé
  Cc: qemu-devel, Mark Cave-Ayland, Zhao Liu, Paolo Bonzini, qemu-block,
	John Snow, qemu-ppc, Eduardo Habkost, Richard Henderson,
	Markus Armbruster, Michael S. Tsirkin, Marcel Apfelbaum

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

On Thu, 8 Feb 2024, 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 | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index 7ca3f465e0..f67e5a55df 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -250,11 +250,11 @@ static void pc_q35_init(MachineState *machine)
>                                 TYPE_ICH9_LPC_DEVICE);
>     qdev_prop_set_bit(DEVICE(lpc), "smm-enabled",
>                       x86_machine_is_smm_enabled(x86ms));
> +    pci_realize_and_unref(lpc, host_bus, &error_fatal);
>     lpc_dev = DEVICE(lpc);

You could also move setting lpc_dev before the qdev_prop_set_bit line and 
use it for the first parameter while you're at it.

Regards,
BALATON Zoltam

>     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"));
>
>

^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [PATCH v3 06/11] hw/misc/macio: Realize IDE controller before accessing it
  2024-02-08 18:12 ` [PATCH v3 06/11] hw/misc/macio: Realize IDE controller " Philippe Mathieu-Daudé
@ 2024-02-08 18:33   ` BALATON Zoltan
  2024-02-09  6:40     ` Philippe Mathieu-Daudé
  2024-02-09 21:32   ` Mark Cave-Ayland
  1 sibling, 1 reply; 45+ messages in thread
From: BALATON Zoltan @ 2024-02-08 18:33 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé
  Cc: qemu-devel, Mark Cave-Ayland, Zhao Liu, Paolo Bonzini, qemu-block,
	John Snow, qemu-ppc, Eduardo Habkost, Richard Henderson,
	Markus Armbruster

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

On Thu, 8 Feb 2024, Philippe Mathieu-Daudé wrote:
> We should not wire IRQs on unrealized device.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
> hw/misc/macio/macio.c | 8 +++++---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/hw/misc/macio/macio.c b/hw/misc/macio/macio.c
> index c9f22f8515..db662a2065 100644
> --- a/hw/misc/macio/macio.c
> +++ b/hw/misc/macio/macio.c
> @@ -122,15 +122,17 @@ static bool macio_realize_ide(MacIOState *s, MACIOIDEState *ide,
>                               Error **errp)
> {
>     SysBusDevice *sbd = SYS_BUS_DEVICE(ide);
> +    bool success;
>
> -    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);
> +    success = qdev_realize(DEVICE(ide), BUS(&s->macio_bus), errp);

If realize is unsuccessful can you connect irqs if device may be 
unrealized? So maybe either the next two lines should be in an if block or 
drop the success flag and return false here if (!qdev_realize) and true at 
end of func?

Regards,
BALATON Zoltan

> +    sysbus_connect_irq(sbd, 0, irq0);
> +    sysbus_connect_irq(sbd, 1, irq1);
>
> -    return qdev_realize(DEVICE(ide), BUS(&s->macio_bus), errp);
> +    return success;
> }
>
> static void macio_oldworld_realize(PCIDevice *d, Error **errp)
>

^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [PATCH v3 06/11] hw/misc/macio: Realize IDE controller before accessing it
  2024-02-08 18:33   ` BALATON Zoltan
@ 2024-02-09  6:40     ` Philippe Mathieu-Daudé
  0 siblings, 0 replies; 45+ messages in thread
From: Philippe Mathieu-Daudé @ 2024-02-09  6:40 UTC (permalink / raw)
  To: BALATON Zoltan
  Cc: qemu-devel, Mark Cave-Ayland, Zhao Liu, Paolo Bonzini, qemu-block,
	John Snow, qemu-ppc, Eduardo Habkost, Richard Henderson,
	Markus Armbruster

On 8/2/24 19:33, BALATON Zoltan wrote:
> On Thu, 8 Feb 2024, Philippe Mathieu-Daudé wrote:
>> We should not wire IRQs on unrealized device.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>> hw/misc/macio/macio.c | 8 +++++---
>> 1 file changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/misc/macio/macio.c b/hw/misc/macio/macio.c
>> index c9f22f8515..db662a2065 100644
>> --- a/hw/misc/macio/macio.c
>> +++ b/hw/misc/macio/macio.c
>> @@ -122,15 +122,17 @@ static bool macio_realize_ide(MacIOState *s, 
>> MACIOIDEState *ide,
>>                               Error **errp)
>> {
>>     SysBusDevice *sbd = SYS_BUS_DEVICE(ide);
>> +    bool success;
>>
>> -    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);
>> +    success = qdev_realize(DEVICE(ide), BUS(&s->macio_bus), errp);
> 
> If realize is unsuccessful can you connect irqs if device may be 
> unrealized? So maybe either the next two lines should be in an if block 
> or drop the success flag and return false here if (!qdev_realize) and 
> true at end of func?

Doh you are right, thanks!

>> +    sysbus_connect_irq(sbd, 0, irq0);
>> +    sysbus_connect_irq(sbd, 1, irq1);
>>
>> -    return qdev_realize(DEVICE(ide), BUS(&s->macio_bus), errp);
>> +    return success;
>> }
>>
>> static void macio_oldworld_realize(PCIDevice *d, Error **errp)
>>



^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [PATCH v3 01/11] hw/ide/ich9: Use AHCIPCIState typedef
  2024-02-08 18:12 ` [PATCH v3 01/11] hw/ide/ich9: Use AHCIPCIState typedef Philippe Mathieu-Daudé
@ 2024-02-09 11:28   ` Peter Maydell
  0 siblings, 0 replies; 45+ messages in thread
From: Peter Maydell @ 2024-02-09 11:28 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé
  Cc: qemu-devel, Mark Cave-Ayland, Zhao Liu, Paolo Bonzini, qemu-block,
	John Snow, qemu-ppc, Eduardo Habkost, Richard Henderson,
	Markus Armbruster

On Thu, 8 Feb 2024 at 18:13, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> 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>

thanks
-- PMM


^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [PATCH v3 02/11] hw/rx/rx62n: Reduce inclusion of 'qemu/units.h'
  2024-02-08 18:12 ` [PATCH v3 02/11] hw/rx/rx62n: Reduce inclusion of 'qemu/units.h' Philippe Mathieu-Daudé
@ 2024-02-09 11:28   ` Peter Maydell
  2024-02-12 11:24   ` Yoshinori Sato
  1 sibling, 0 replies; 45+ messages in thread
From: Peter Maydell @ 2024-02-09 11:28 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé
  Cc: qemu-devel, Mark Cave-Ayland, Zhao Liu, Paolo Bonzini, qemu-block,
	John Snow, qemu-ppc, Eduardo Habkost, Richard Henderson,
	Markus Armbruster, Yoshinori Sato

On Thu, 8 Feb 2024 at 18:13, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> "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>

thanks
-- PMM


^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [PATCH v3 03/11] hw/rx/rx62n: Only call qdev_get_gpio_in() when necessary
  2024-02-08 18:12 ` [PATCH v3 03/11] hw/rx/rx62n: Only call qdev_get_gpio_in() when necessary Philippe Mathieu-Daudé
@ 2024-02-09 11:29   ` Peter Maydell
  2024-02-12 11:25   ` Yoshinori Sato
  1 sibling, 0 replies; 45+ messages in thread
From: Peter Maydell @ 2024-02-09 11:29 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé
  Cc: qemu-devel, Mark Cave-Ayland, Zhao Liu, Paolo Bonzini, qemu-block,
	John Snow, qemu-ppc, Eduardo Habkost, Richard Henderson,
	Markus Armbruster, Yoshinori Sato

On Thu, 8 Feb 2024 at 18:14, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> 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>

thanks
-- PMM


^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [PATCH v3 05/11] hw/ppc/prep: Realize ISA bridge before accessing it
  2024-02-08 18:12 ` [PATCH v3 05/11] hw/ppc/prep: Realize ISA bridge " Philippe Mathieu-Daudé
@ 2024-02-09 11:29   ` Peter Maydell
  2024-02-09 21:29   ` Mark Cave-Ayland
  1 sibling, 0 replies; 45+ messages in thread
From: Peter Maydell @ 2024-02-09 11:29 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé
  Cc: qemu-devel, Mark Cave-Ayland, Zhao Liu, Paolo Bonzini, qemu-block,
	John Snow, qemu-ppc, Eduardo Habkost, Richard Henderson,
	Markus Armbruster, Hervé Poussineau

On Thu, 8 Feb 2024 at 18:14, Philippe Mathieu-Daudé <philmd@linaro.org> 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>

thanks
-- PMM


^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [PATCH v3 07/11] hw/sh4/r2d: Realize IDE controller before accessing it
  2024-02-08 18:12 ` [PATCH v3 07/11] hw/sh4/r2d: " Philippe Mathieu-Daudé
@ 2024-02-09 11:30   ` Peter Maydell
  2024-02-12 12:48   ` Yoshinori Sato
  2024-05-03 21:34   ` Guenter Roeck
  2 siblings, 0 replies; 45+ messages in thread
From: Peter Maydell @ 2024-02-09 11:30 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé
  Cc: qemu-devel, Mark Cave-Ayland, Zhao Liu, Paolo Bonzini, qemu-block,
	John Snow, qemu-ppc, Eduardo Habkost, Richard Henderson,
	Markus Armbruster, Yoshinori Sato, Magnus Damm

On Thu, 8 Feb 2024 at 18:14, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> We should not wire IRQs on unrealized device.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  hw/sh4/r2d.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM


^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [PATCH v3 11/11] hw/sparc64/cpu: Initialize GPIO before realizing CPU devices
  2024-02-08 18:12 ` [PATCH v3 11/11] hw/sparc64/cpu: " Philippe Mathieu-Daudé
@ 2024-02-09 11:34   ` Peter Maydell
  2024-02-09 22:01     ` Mark Cave-Ayland
  2024-02-09 21:49   ` Mark Cave-Ayland
  1 sibling, 1 reply; 45+ messages in thread
From: Peter Maydell @ 2024-02-09 11:34 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé
  Cc: qemu-devel, Mark Cave-Ayland, Zhao Liu, Paolo Bonzini, qemu-block,
	John Snow, qemu-ppc, Eduardo Habkost, Richard Henderson,
	Markus Armbruster, Artyom Tarasenko

On Thu, 8 Feb 2024 at 18:14, Philippe Mathieu-Daudé <philmd@linaro.org> 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>
> ---
>  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

For the purposes of letting us enforce the "init GPIOs
before realize, not after" rule,
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

but it looks like this code is adding a GPIO to a
device from code that's not actually part of the
implementation of the device. Ideally most of the code in
this file should be rolled into the CPU itself in target/sparc.

thanks
-- PMM


^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [PATCH v3 10/11] hw/sparc/leon3: Initialize GPIO before realizing CPU devices
  2024-02-08 18:12 ` [PATCH v3 10/11] hw/sparc/leon3: Initialize GPIO before realizing CPU devices Philippe Mathieu-Daudé
@ 2024-02-09 11:35   ` Peter Maydell
  2024-02-09 21:48   ` Mark Cave-Ayland
  1 sibling, 0 replies; 45+ messages in thread
From: Peter Maydell @ 2024-02-09 11:35 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé
  Cc: qemu-devel, Mark Cave-Ayland, Zhao Liu, Paolo Bonzini, qemu-block,
	John Snow, qemu-ppc, Eduardo Habkost, Richard Henderson,
	Markus Armbruster, Fabien Chouteau, Frederic Konrad,
	Artyom Tarasenko

On Thu, 8 Feb 2024 at 18:14, Philippe Mathieu-Daudé <philmd@linaro.org> 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>
> ---
>  hw/sparc/leon3.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/hw/sparc/leon3.c b/hw/sparc/leon3.c
> index 0df5fc949d..0e1d749306 100644
> --- a/hw/sparc/leon3.c
> +++ b/hw/sparc/leon3.c
> @@ -234,8 +234,11 @@ 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));
>      env = &cpu->env;
> +    qdev_init_gpio_in_named_with_opaque(DEVICE(cpu), leon3_set_pil_in,
> +                                        env, "pil", 1);
> +    qdev_realize(DEVICE(cpu), NULL, &error_fatal);

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
with a similar caveat as with the sparc64.c patch.

thanks
-- PMM


^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [PATCH v3 08/11] hw/sparc/sun4m: Realize DMA controller before accessing it
  2024-02-08 18:12 ` [PATCH v3 08/11] hw/sparc/sun4m: Realize DMA " Philippe Mathieu-Daudé
@ 2024-02-09 11:37   ` Peter Maydell
  2024-02-09 22:37     ` Mark Cave-Ayland
  2024-02-09 21:38   ` Mark Cave-Ayland
  1 sibling, 1 reply; 45+ messages in thread
From: Peter Maydell @ 2024-02-09 11:37 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé
  Cc: qemu-devel, Mark Cave-Ayland, Zhao Liu, Paolo Bonzini, qemu-block,
	John Snow, qemu-ppc, Eduardo Habkost, Richard Henderson,
	Markus Armbruster, Artyom Tarasenko

On Thu, 8 Feb 2024 at 18:14, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> We should not wire IRQs on unrealized device.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  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);
> +

(This confused me briefly until I realised that this function
is reaching into the dma device to find child objects to connect
the IRQs to. Perhaps it would be nicer if the wrapping 'dma' object
passed through those IRQs to avoid that.)

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM


^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [PATCH v3 09/11] hw/sparc/leon3: Realize GRLIB IRQ controller before accessing it
  2024-02-08 18:12 ` [PATCH v3 09/11] hw/sparc/leon3: Realize GRLIB IRQ " Philippe Mathieu-Daudé
@ 2024-02-09 11:38   ` Peter Maydell
  2024-02-09 21:39   ` Mark Cave-Ayland
  1 sibling, 0 replies; 45+ messages in thread
From: Peter Maydell @ 2024-02-09 11:38 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé
  Cc: qemu-devel, Mark Cave-Ayland, Zhao Liu, Paolo Bonzini, qemu-block,
	John Snow, qemu-ppc, Eduardo Habkost, Richard Henderson,
	Markus Armbruster, Fabien Chouteau, Frederic Konrad,
	Artyom Tarasenko

On Thu, 8 Feb 2024 at 18:15, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> We should not wire IRQs on unrealized device.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  hw/sparc/leon3.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM


^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [PATCH v3 05/11] hw/ppc/prep: Realize ISA bridge before accessing it
  2024-02-08 18:12 ` [PATCH v3 05/11] hw/ppc/prep: Realize ISA bridge " Philippe Mathieu-Daudé
  2024-02-09 11:29   ` Peter Maydell
@ 2024-02-09 21:29   ` Mark Cave-Ayland
  1 sibling, 0 replies; 45+ messages in thread
From: Mark Cave-Ayland @ 2024-02-09 21:29 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé, qemu-devel
  Cc: Zhao Liu, Paolo Bonzini, qemu-block, John Snow, qemu-ppc,
	Eduardo Habkost, Richard Henderson, Markus Armbruster,
	Hervé Poussineau

On 08/02/2024 18:12, Philippe Mathieu-Daudé wrote:

> We should not wire IRQs on unrealized device.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   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"));

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


ATB,

Mark.



^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [PATCH v3 06/11] hw/misc/macio: Realize IDE controller before accessing it
  2024-02-08 18:12 ` [PATCH v3 06/11] hw/misc/macio: Realize IDE controller " Philippe Mathieu-Daudé
  2024-02-08 18:33   ` BALATON Zoltan
@ 2024-02-09 21:32   ` Mark Cave-Ayland
  1 sibling, 0 replies; 45+ messages in thread
From: Mark Cave-Ayland @ 2024-02-09 21:32 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé, qemu-devel
  Cc: Zhao Liu, Paolo Bonzini, qemu-block, John Snow, qemu-ppc,
	Eduardo Habkost, Richard Henderson, Markus Armbruster

On 08/02/2024 18:12, Philippe Mathieu-Daudé wrote:

> We should not wire IRQs on unrealized device.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   hw/misc/macio/macio.c | 8 +++++---
>   1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/misc/macio/macio.c b/hw/misc/macio/macio.c
> index c9f22f8515..db662a2065 100644
> --- a/hw/misc/macio/macio.c
> +++ b/hw/misc/macio/macio.c
> @@ -122,15 +122,17 @@ static bool macio_realize_ide(MacIOState *s, MACIOIDEState *ide,
>                                 Error **errp)
>   {
>       SysBusDevice *sbd = SYS_BUS_DEVICE(ide);
> +    bool success;
>   
> -    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);
> +    success = qdev_realize(DEVICE(ide), BUS(&s->macio_bus), errp);
> +    sysbus_connect_irq(sbd, 0, irq0);
> +    sysbus_connect_irq(sbd, 1, irq1);
>   
> -    return qdev_realize(DEVICE(ide), BUS(&s->macio_bus), errp);
> +    return success;
>   }
>   
>   static void macio_oldworld_realize(PCIDevice *d, Error **errp)

I see that Zoltan has already commented about checking the success of qdev_realise() 
before wiring the sysbus IRQs, so with that fixed:

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


ATB,

Mark.



^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [PATCH v3 08/11] hw/sparc/sun4m: Realize DMA controller before accessing it
  2024-02-08 18:12 ` [PATCH v3 08/11] hw/sparc/sun4m: Realize DMA " Philippe Mathieu-Daudé
  2024-02-09 11:37   ` Peter Maydell
@ 2024-02-09 21:38   ` Mark Cave-Ayland
  1 sibling, 0 replies; 45+ messages in thread
From: Mark Cave-Ayland @ 2024-02-09 21:38 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé, qemu-devel
  Cc: Zhao Liu, Paolo Bonzini, qemu-block, John Snow, qemu-ppc,
	Eduardo Habkost, Richard Henderson, Markus Armbruster,
	Artyom Tarasenko

On 08/02/2024 18:12, Philippe Mathieu-Daudé wrote:

> We should not wire IRQs on unrealized device.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   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);

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


ATB,

Mark.



^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [PATCH v3 09/11] hw/sparc/leon3: Realize GRLIB IRQ controller before accessing it
  2024-02-08 18:12 ` [PATCH v3 09/11] hw/sparc/leon3: Realize GRLIB IRQ " Philippe Mathieu-Daudé
  2024-02-09 11:38   ` Peter Maydell
@ 2024-02-09 21:39   ` Mark Cave-Ayland
  1 sibling, 0 replies; 45+ messages in thread
From: Mark Cave-Ayland @ 2024-02-09 21:39 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé, qemu-devel
  Cc: Zhao Liu, Paolo Bonzini, qemu-block, John Snow, qemu-ppc,
	Eduardo Habkost, Richard Henderson, Markus Armbruster,
	Fabien Chouteau, Frederic Konrad, Artyom Tarasenko

On 08/02/2024 18:12, Philippe Mathieu-Daudé wrote:

> We should not wire IRQs on unrealized device.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   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,

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


ATB,

Mark.



^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [PATCH v3 10/11] hw/sparc/leon3: Initialize GPIO before realizing CPU devices
  2024-02-08 18:12 ` [PATCH v3 10/11] hw/sparc/leon3: Initialize GPIO before realizing CPU devices Philippe Mathieu-Daudé
  2024-02-09 11:35   ` Peter Maydell
@ 2024-02-09 21:48   ` Mark Cave-Ayland
  1 sibling, 0 replies; 45+ messages in thread
From: Mark Cave-Ayland @ 2024-02-09 21:48 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé, qemu-devel
  Cc: Zhao Liu, Paolo Bonzini, qemu-block, John Snow, qemu-ppc,
	Eduardo Habkost, Richard Henderson, Markus Armbruster,
	Fabien Chouteau, Frederic Konrad, Artyom Tarasenko

On 08/02/2024 18:12, 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>
> ---
>   hw/sparc/leon3.c | 7 ++++---
>   1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/sparc/leon3.c b/hw/sparc/leon3.c
> index 0df5fc949d..0e1d749306 100644
> --- a/hw/sparc/leon3.c
> +++ b/hw/sparc/leon3.c
> @@ -234,8 +234,11 @@ 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));
>       env = &cpu->env;
> +    qdev_init_gpio_in_named_with_opaque(DEVICE(cpu), leon3_set_pil_in,
> +                                        env, "pil", 1);
> +    qdev_realize(DEVICE(cpu), NULL, &error_fatal);

I know it's not part of this patch, but I think that 
qdev_init_gpio_in_named_with_opaque() can be replaced with just 
qdev_init_gpio_in_named(), and leon3_set_pil_in() updated to take CPUState.

>       cpu_sparc_set_id(env, 0);
>   
> @@ -261,8 +264,6 @@ 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);
>       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.



^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [PATCH v3 11/11] hw/sparc64/cpu: Initialize GPIO before realizing CPU devices
  2024-02-08 18:12 ` [PATCH v3 11/11] hw/sparc64/cpu: " Philippe Mathieu-Daudé
  2024-02-09 11:34   ` Peter Maydell
@ 2024-02-09 21:49   ` Mark Cave-Ayland
  1 sibling, 0 replies; 45+ messages in thread
From: Mark Cave-Ayland @ 2024-02-09 21:49 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé, qemu-devel
  Cc: Zhao Liu, Paolo Bonzini, qemu-block, John Snow, qemu-ppc,
	Eduardo Habkost, Richard Henderson, Markus Armbruster,
	Artyom Tarasenko

On 08/02/2024 18:12, 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>
> ---
>   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: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>


ATB,

Mark.



^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [PATCH v3 11/11] hw/sparc64/cpu: Initialize GPIO before realizing CPU devices
  2024-02-09 11:34   ` Peter Maydell
@ 2024-02-09 22:01     ` Mark Cave-Ayland
  2024-02-13 13:00       ` Philippe Mathieu-Daudé
  0 siblings, 1 reply; 45+ messages in thread
From: Mark Cave-Ayland @ 2024-02-09 22:01 UTC (permalink / raw)
  To: Peter Maydell, Philippe Mathieu-Daudé
  Cc: qemu-devel, Zhao Liu, Paolo Bonzini, qemu-block, John Snow,
	qemu-ppc, Eduardo Habkost, Richard Henderson, Markus Armbruster,
	Artyom Tarasenko

On 09/02/2024 11:34, Peter Maydell wrote:

> On Thu, 8 Feb 2024 at 18:14, Philippe Mathieu-Daudé <philmd@linaro.org> 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>
>> ---
>>   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
> 
> For the purposes of letting us enforce the "init GPIOs
> before realize, not after" rule,
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> 
> but it looks like this code is adding a GPIO to a
> device from code that's not actually part of the
> implementation of the device. Ideally most of the code in
> this file should be rolled into the CPU itself in target/sparc.

I suspect the reason the code is arranged like this is because IVECs aren't part of 
the core SPARC 64-bit architecture specification, although they happen to be 
implemented by the CPUs used by QEMU. Perhaps this would be better be handled on a 
CPU model basis, but I agree this shouldn't be a blocker for this patch.


ATB,

Mark.



^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [PATCH v3 08/11] hw/sparc/sun4m: Realize DMA controller before accessing it
  2024-02-09 11:37   ` Peter Maydell
@ 2024-02-09 22:37     ` Mark Cave-Ayland
  0 siblings, 0 replies; 45+ messages in thread
From: Mark Cave-Ayland @ 2024-02-09 22:37 UTC (permalink / raw)
  To: Peter Maydell, Philippe Mathieu-Daudé
  Cc: qemu-devel, Zhao Liu, Paolo Bonzini, qemu-block, John Snow,
	qemu-ppc, Eduardo Habkost, Richard Henderson, Markus Armbruster,
	Artyom Tarasenko

On 09/02/2024 11:37, Peter Maydell wrote:

> On Thu, 8 Feb 2024 at 18:14, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>
>> We should not wire IRQs on unrealized device.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   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);
>> +
> 
> (This confused me briefly until I realised that this function
> is reaching into the dma device to find child objects to connect
> the IRQs to. Perhaps it would be nicer if the wrapping 'dma' object
> passed through those IRQs to avoid that.)

FWIW I can't say I'm a fan of passing through IRQs for container devices that contain 
one or more child devices, since you end up with an extra layer of wiring complexity 
that can change depending upon the child device IRQ wiring. I find it much easier to 
access the child devices directly, since then the wiring is always consistent across 
all users.


ATB,

Mark.



^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [PATCH v3 02/11] hw/rx/rx62n: Reduce inclusion of 'qemu/units.h'
  2024-02-08 18:12 ` [PATCH v3 02/11] hw/rx/rx62n: Reduce inclusion of 'qemu/units.h' Philippe Mathieu-Daudé
  2024-02-09 11:28   ` Peter Maydell
@ 2024-02-12 11:24   ` Yoshinori Sato
  1 sibling, 0 replies; 45+ messages in thread
From: Yoshinori Sato @ 2024-02-12 11:24 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé
  Cc: qemu-devel, Mark Cave-Ayland, Zhao Liu, Paolo Bonzini, qemu-block,
	John Snow, qemu-ppc, Eduardo Habkost, Richard Henderson,
	Markus Armbruster

On Fri, 09 Feb 2024 03:12:35 +0900,
Philippe Mathieu-Daudé wrote:
> 
> "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>
> ---
>  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
> 

Reviewed-by: Yoshinori Sato <ysato@users.sourceforge.jp>

-- 
Yosinori Sato


^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [PATCH v3 03/11] hw/rx/rx62n: Only call qdev_get_gpio_in() when necessary
  2024-02-08 18:12 ` [PATCH v3 03/11] hw/rx/rx62n: Only call qdev_get_gpio_in() when necessary Philippe Mathieu-Daudé
  2024-02-09 11:29   ` Peter Maydell
@ 2024-02-12 11:25   ` Yoshinori Sato
  1 sibling, 0 replies; 45+ messages in thread
From: Yoshinori Sato @ 2024-02-12 11:25 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé
  Cc: qemu-devel, Mark Cave-Ayland, Zhao Liu, Paolo Bonzini, qemu-block,
	John Snow, qemu-ppc, Eduardo Habkost, Richard Henderson,
	Markus Armbruster

On Fri, 09 Feb 2024 03:12:36 +0900,
Philippe Mathieu-Daudé wrote:
> 
> 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>
> ---
>  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
> 

Reviewed-by: Yoshinori Sato <ysato@users.sourceforge.jp>

-- 
Yosinori Sato


^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [PATCH v3 07/11] hw/sh4/r2d: Realize IDE controller before accessing it
  2024-02-08 18:12 ` [PATCH v3 07/11] hw/sh4/r2d: " Philippe Mathieu-Daudé
  2024-02-09 11:30   ` Peter Maydell
@ 2024-02-12 12:48   ` Yoshinori Sato
  2024-05-03 21:34   ` Guenter Roeck
  2 siblings, 0 replies; 45+ messages in thread
From: Yoshinori Sato @ 2024-02-12 12:48 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé
  Cc: qemu-devel, Mark Cave-Ayland, Zhao Liu, Paolo Bonzini, qemu-block,
	John Snow, qemu-ppc, Eduardo Habkost, Richard Henderson,
	Markus Armbruster, Magnus Damm

On Fri, 09 Feb 2024 03:12:40 +0900,
Philippe Mathieu-Daudé wrote:
> 
> We should not wire IRQs on unrealized device.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  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
> 

Reviewed-by: Yoshinori Sato <ysato@users.sourceforge.jp>

-- 
Yosinori Sato


^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [PATCH v3 11/11] hw/sparc64/cpu: Initialize GPIO before realizing CPU devices
  2024-02-09 22:01     ` Mark Cave-Ayland
@ 2024-02-13 13:00       ` Philippe Mathieu-Daudé
  0 siblings, 0 replies; 45+ messages in thread
From: Philippe Mathieu-Daudé @ 2024-02-13 13:00 UTC (permalink / raw)
  To: Mark Cave-Ayland, Peter Maydell
  Cc: qemu-devel, Zhao Liu, Paolo Bonzini, qemu-block, John Snow,
	qemu-ppc, Eduardo Habkost, Richard Henderson, Markus Armbruster,
	Artyom Tarasenko

On 9/2/24 23:01, Mark Cave-Ayland wrote:
> On 09/02/2024 11:34, Peter Maydell wrote:
> 
>> On Thu, 8 Feb 2024 at 18:14, Philippe Mathieu-Daudé 
>> <philmd@linaro.org> 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>
>>> ---
>>>   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
>>
>> For the purposes of letting us enforce the "init GPIOs
>> before realize, not after" rule,
>> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
>>
>> but it looks like this code is adding a GPIO to a
>> device from code that's not actually part of the
>> implementation of the device. Ideally most of the code in
>> this file should be rolled into the CPU itself in target/sparc.
> 
> I suspect the reason the code is arranged like this is because IVECs 
> aren't part of the core SPARC 64-bit architecture specification, 
> although they happen to be implemented by the CPUs used by QEMU. Perhaps 
> this would be better be handled on a CPU model basis, but I agree this 
> shouldn't be a blocker for this patch.

Suggestion recorded as https://gitlab.com/qemu-project/qemu/-/issues/2163.

Thanks both!

Phil.


^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [PATCH v3 07/11] hw/sh4/r2d: Realize IDE controller before accessing it
  2024-02-08 18:12 ` [PATCH v3 07/11] hw/sh4/r2d: " Philippe Mathieu-Daudé
  2024-02-09 11:30   ` Peter Maydell
  2024-02-12 12:48   ` Yoshinori Sato
@ 2024-05-03 21:34   ` Guenter Roeck
  2024-10-11  8:23     ` Thomas Huth
  2 siblings, 1 reply; 45+ messages in thread
From: Guenter Roeck @ 2024-05-03 21:34 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé
  Cc: qemu-devel, Mark Cave-Ayland, Zhao Liu, Paolo Bonzini, qemu-block,
	John Snow, qemu-ppc, Eduardo Habkost, Richard Henderson,
	Markus Armbruster, Yoshinori Sato, Magnus Damm

Hi,

On Thu, Feb 08, 2024 at 07:12:40PM +0100, 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>

qemu 9.0 fails to boot Linux from ide/ata drives with the sh4
and sh4eb emulations. Error log is as follows.

ata1.00: ATA-7: QEMU HARDDISK, 2.5+, max UDMA/100
ata1.00: 16384 sectors, multi 16: LBA48
ata1.00: configured for PIO
scsi 0:0:0:0: Direct-Access     ATA      QEMU HARDDISK    2.5+ PQ: 0 ANSI: 5
sd 0:0:0:0: [sda] 16384 512-byte logical blocks: (8.39 MB/8.00 MiB)
sd 0:0:0:0: [sda] Write Protect is off
sd 0:0:0:0: [sda] Write cache: enabled, read cache: enabled, doesn't support DPO or FUA
ata1: lost interrupt (Status 0x58)

[ and more similar errors ]

qemu command line:

qemu-system-sh4eb -M r2d -kernel arch/sh/boot/zImage \
	-snapshot -drive file=rootfs.ext2,format=raw,if=ide \
	-append "root=/dev/sda console=ttySC1,115200 noiotrap" \
	-serial null -serial stdio -monitor null -nographic -no-reboot

Bisect points to this patch (see below). Reverting it fixes the problem.

Guenter

---
bisect log:

# bad: [c25df57ae8f9fe1c72eee2dab37d76d904ac382e] Update version for 9.0.0 release
# good: [1600b9f46b1bd08b00fe86c46ef6dbb48cbe10d6] Update version for v8.2.0 release
git bisect start 'v9.0.0' 'v8.2.0'
# good: [62357c047a5abc6ede992159ed7c0aaaeb50617a] Merge tag 'qemu-sparc-20240213' of https://github.com/mcayland/qemu into staging
git bisect good 62357c047a5abc6ede992159ed7c0aaaeb50617a
# bad: [d65f1ed7de1559534d0a1fabca5bdd81c594c7ca] docs/acpi/bits: add some clarity and details while also improving formating
git bisect bad d65f1ed7de1559534d0a1fabca5bdd81c594c7ca
# bad: [99e1c1137b6f339be1e4b76e243ad7b7c3d3cb8c] hw/i386/pc: Populate RTC attribute directly
git bisect bad 99e1c1137b6f339be1e4b76e243ad7b7c3d3cb8c
# bad: [760b4dcdddba4a40b9fa0eb78fdfc7eda7cb83d0] Merge tag 'for-upstream' of https://gitlab.com/bonzini/qemu into staging
git bisect bad 760b4dcdddba4a40b9fa0eb78fdfc7eda7cb83d0
# good: [f2b4a98930c122648e9dc494e49cea5dffbcc2be] target/arm: Allow access to SPSR_hyp from hyp mode
git bisect good f2b4a98930c122648e9dc494e49cea5dffbcc2be
# bad: [1a8e2f58c5dd721086284f827326b370d19ad9eb] hw/i386/q35: Use DEVICE() cast macro with PCIDevice object
git bisect bad 1a8e2f58c5dd721086284f827326b370d19ad9eb
# good: [59ae6bcddc3651b55b96c2bf05a6cd4312e46d10] hw/ppc/prep: Realize ISA bridge before accessing it
git bisect good 59ae6bcddc3651b55b96c2bf05a6cd4312e46d10
# bad: [7ed9a5f626a6c932a8c869a91e6a8b3e2029f5ef] hw/intc/grlib_irqmp: implements the multiprocessor status register
git bisect bad 7ed9a5f626a6c932a8c869a91e6a8b3e2029f5ef
# bad: [d08b7af3f7f27f6f3da8446756bf0b9352026b1d] target/sparc: Provide hint about CPUSPARCState::irq_manager member
git bisect bad d08b7af3f7f27f6f3da8446756bf0b9352026b1d
# bad: [5e37bc4997c32a1c9a6621a060462c84df9f1b8f] hw/dma: Pass parent object to i8257_dma_init()
git bisect bad 5e37bc4997c32a1c9a6621a060462c84df9f1b8f
# bad: [3c5f86a22686ef475a8259c0d8ee714f61c770c9] hw/sh4/r2d: Realize IDE controller before accessing it
git bisect bad 3c5f86a22686ef475a8259c0d8ee714f61c770c9
# good: [fc432ba0f58343c8912b80e9056315bb9bd8df92] hw/misc/macio: Realize IDE controller before accessing it
git bisect good fc432ba0f58343c8912b80e9056315bb9bd8df92
# first bad commit: [3c5f86a22686ef475a8259c0d8ee714f61c770c9] hw/sh4/r2d: Realize IDE controller before accessing it


^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [PATCH v3 07/11] hw/sh4/r2d: Realize IDE controller before accessing it
  2024-05-03 21:34   ` Guenter Roeck
@ 2024-10-11  8:23     ` Thomas Huth
  2024-10-11 22:48       ` Philippe Mathieu-Daudé
  0 siblings, 1 reply; 45+ messages in thread
From: Thomas Huth @ 2024-10-11  8:23 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé
  Cc: qemu-devel, Mark Cave-Ayland, Paolo Bonzini, qemu-block,
	John Snow, qemu-ppc, Richard Henderson, Markus Armbruster,
	Yoshinori Sato, Magnus Damm, Guenter Roeck

On 03/05/2024 23.34, Guenter Roeck wrote:
> Hi,
> 
> On Thu, Feb 08, 2024 at 07:12:40PM +0100, 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>
> 
> qemu 9.0 fails to boot Linux from ide/ata drives with the sh4
> and sh4eb emulations. Error log is as follows.
> 
> ata1.00: ATA-7: QEMU HARDDISK, 2.5+, max UDMA/100
> ata1.00: 16384 sectors, multi 16: LBA48
> ata1.00: configured for PIO
> scsi 0:0:0:0: Direct-Access     ATA      QEMU HARDDISK    2.5+ PQ: 0 ANSI: 5
> sd 0:0:0:0: [sda] 16384 512-byte logical blocks: (8.39 MB/8.00 MiB)
> sd 0:0:0:0: [sda] Write Protect is off
> sd 0:0:0:0: [sda] Write cache: enabled, read cache: enabled, doesn't support DPO or FUA
> ata1: lost interrupt (Status 0x58)
> 
> [ and more similar errors ]
> 
> qemu command line:
> 
> qemu-system-sh4eb -M r2d -kernel arch/sh/boot/zImage \
> 	-snapshot -drive file=rootfs.ext2,format=raw,if=ide \
> 	-append "root=/dev/sda console=ttySC1,115200 noiotrap" \
> 	-serial null -serial stdio -monitor null -nographic -no-reboot
> 
> Bisect points to this patch (see below). Reverting it fixes the problem.

  Hi Philippe!

Today I noticed that our sh4 test from tests/avocado/tuxrun_baselines.py is 
failing (which is not run by default, that's why nobody noticed), and 
bisection took me to the same result that Guenter already reported.

So unless you have a clue how to fix it in a better way, I think we should 
revert this patch?

  Thomas



^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [PATCH v3 00/11] hw: Strengthen SysBus & QBus API
  2024-02-08 18:12 [PATCH v3 00/11] hw: Strengthen SysBus & QBus API Philippe Mathieu-Daudé
                   ` (10 preceding siblings ...)
  2024-02-08 18:12 ` [PATCH v3 11/11] hw/sparc64/cpu: " Philippe Mathieu-Daudé
@ 2024-10-11  8:56 ` Paolo Bonzini
  2024-10-11 22:11   ` Philippe Mathieu-Daudé
  11 siblings, 1 reply; 45+ messages in thread
From: Paolo Bonzini @ 2024-10-11  8:56 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé, qemu-devel
  Cc: Mark Cave-Ayland, Zhao Liu, qemu-block, John Snow, qemu-ppc,
	Eduardo Habkost, Richard Henderson, Markus Armbruster

On 2/8/24 19:12, Philippe Mathieu-Daudé wrote:
> 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()

This series is missing a _why_.  The original vision for qdev was that 
the whole machine would be realized at once from vl.c, and therefore 
there would be no need for realizing subcomponents by hand.

This probably will never happen, but it is still worth explaining why 
it's now considered so conceptually wrong, that it is (or will be) 
enforced by the API.

Paolo

> Patches from v2 enforcing these checks will be posted
> in a separate series.
> 
> Philippe Mathieu-Daudé (11):
>    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/pc_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: 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      |  2 +-
>   hw/ide/ich.c          |  6 +++---
>   hw/misc/macio/macio.c |  8 +++++---
>   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      | 11 ++++++-----
>   hw/sparc/sun4m.c      |  7 +++++--
>   hw/sparc64/sparc64.c  |  4 +++-
>   11 files changed, 35 insertions(+), 27 deletions(-)
> 



^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [PATCH v3 00/11] hw: Strengthen SysBus & QBus API
  2024-10-11  8:56 ` [PATCH v3 00/11] hw: Strengthen SysBus & QBus API Paolo Bonzini
@ 2024-10-11 22:11   ` Philippe Mathieu-Daudé
  0 siblings, 0 replies; 45+ messages in thread
From: Philippe Mathieu-Daudé @ 2024-10-11 22:11 UTC (permalink / raw)
  To: Paolo Bonzini, qemu-devel, Markus Armbruster
  Cc: Mark Cave-Ayland, Zhao Liu, qemu-block, John Snow, qemu-ppc,
	Eduardo Habkost, Richard Henderson, Kevin Wolf

On 11/10/24 05:56, Paolo Bonzini wrote:
> On 2/8/24 19:12, Philippe Mathieu-Daudé wrote:
>> 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()
> 
> This series is missing a _why_.  The original vision for qdev was that 
> the whole machine would be realized at once from vl.c, and therefore 
> there would be no need for realizing subcomponents by hand.
> 
> This probably will never happen, but it is still worth explaining why 
> it's now considered so conceptually wrong, that it is (or will be) 
> enforced by the API.

When looking at a roadmap toward "dynamic machines" with Markus we
needed to clarify the QOM life cycle before thinking about possible
DSL to declare machines and components. See:

= Problem 4: The /machine/unattached/ orphanage =
= Problem 5: QOM lacks a clear life cycle =
= Problem 7: Design of the machine specification DSL =
in https://lore.kernel.org/qemu-devel/87o7d1i7ky.fsf@pond.sub.org/,
in particular:

     '''
     We need to manage a state transition from "object created,
     but not ready for use" to "object configured and ready for
     use".  In what order do the objects change state?
     '''

and:

     '''
     Ideally, a composite object's components go through the life
     cycle together.  First, create all the components and assign
     parents.  This also creates all the properties.  Then configure
     the object by setting property values.  Finally, complete / realize
     all components.
     '''

We need a better QOM documentation, "Device Life-cycle" in
docs/devel/qom.rst doesn't reallyr describe the life cycle, and
doesn't mention the "realization" API contract.
Since you have more experience and historical view on the APIs,
I wouldn't mind if you want to clarify that; otherwise I'll try
to do it.

Regards,

Phil.


> Paolo



^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [PATCH v3 07/11] hw/sh4/r2d: Realize IDE controller before accessing it
  2024-10-11  8:23     ` Thomas Huth
@ 2024-10-11 22:48       ` Philippe Mathieu-Daudé
  2024-10-12  9:40         ` Thomas Huth
  0 siblings, 1 reply; 45+ messages in thread
From: Philippe Mathieu-Daudé @ 2024-10-11 22:48 UTC (permalink / raw)
  To: Thomas Huth, John Snow, Markus Armbruster
  Cc: qemu-devel, Mark Cave-Ayland, Paolo Bonzini, qemu-block, qemu-ppc,
	Richard Henderson, Yoshinori Sato, Magnus Damm, Guenter Roeck

On 11/10/24 05:23, Thomas Huth wrote:
> On 03/05/2024 23.34, Guenter Roeck wrote:
>> Hi,
>>
>> On Thu, Feb 08, 2024 at 07:12:40PM +0100, 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>
>>
>> qemu 9.0 fails to boot Linux from ide/ata drives with the sh4
>> and sh4eb emulations. Error log is as follows.
>>
>> ata1.00: ATA-7: QEMU HARDDISK, 2.5+, max UDMA/100
>> ata1.00: 16384 sectors, multi 16: LBA48
>> ata1.00: configured for PIO
>> scsi 0:0:0:0: Direct-Access     ATA      QEMU HARDDISK    2.5+ PQ: 0 
>> ANSI: 5
>> sd 0:0:0:0: [sda] 16384 512-byte logical blocks: (8.39 MB/8.00 MiB)
>> sd 0:0:0:0: [sda] Write Protect is off
>> sd 0:0:0:0: [sda] Write cache: enabled, read cache: enabled, doesn't 
>> support DPO or FUA
>> ata1: lost interrupt (Status 0x58)
>>
>> [ and more similar errors ]
>>
>> qemu command line:
>>
>> qemu-system-sh4eb -M r2d -kernel arch/sh/boot/zImage \
>>     -snapshot -drive file=rootfs.ext2,format=raw,if=ide \
>>     -append "root=/dev/sda console=ttySC1,115200 noiotrap" \
>>     -serial null -serial stdio -monitor null -nographic -no-reboot
>>
>> Bisect points to this patch (see below). Reverting it fixes the problem.

Sorry Guenter for missing your email :(

> 
>   Hi Philippe!
> 
> Today I noticed that our sh4 test from tests/avocado/tuxrun_baselines.py 
> is failing (which is not run by default, that's why nobody noticed), and 
> bisection took me to the same result that Guenter already reported.

"not run by default" because flaky.

Having a quick look, the problem seems hw/ide/core.c uses non-QOM
shortcuts. In particular ide_bus_init_output_irq() expects a preset
IRQ. Not something trivial to fix.

> 
> So unless you have a clue how to fix it in a better way, I think we 
> should revert this patch?

This patch is what we want long term, and reveals a long standing issue.
I'm not objecting in reverting it short term (I doubt I'll have time to
look at it right now).

Regards,

Phil.


^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [PATCH v3 07/11] hw/sh4/r2d: Realize IDE controller before accessing it
  2024-10-11 22:48       ` Philippe Mathieu-Daudé
@ 2024-10-12  9:40         ` Thomas Huth
  2024-10-12 14:06           ` Bernhard Beschow
  0 siblings, 1 reply; 45+ messages in thread
From: Thomas Huth @ 2024-10-12  9:40 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé, John Snow, Markus Armbruster
  Cc: qemu-devel, Mark Cave-Ayland, Paolo Bonzini, qemu-block, qemu-ppc,
	Richard Henderson, Yoshinori Sato, Magnus Damm, Guenter Roeck,
	Bernhard Beschow

On 12/10/2024 00.48, Philippe Mathieu-Daudé wrote:
> On 11/10/24 05:23, Thomas Huth wrote:
>> On 03/05/2024 23.34, Guenter Roeck wrote:
>>> Hi,
>>>
>>> On Thu, Feb 08, 2024 at 07:12:40PM +0100, 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>
>>>
>>> qemu 9.0 fails to boot Linux from ide/ata drives with the sh4
>>> and sh4eb emulations. Error log is as follows.
>>>
>>> ata1.00: ATA-7: QEMU HARDDISK, 2.5+, max UDMA/100
>>> ata1.00: 16384 sectors, multi 16: LBA48
>>> ata1.00: configured for PIO
>>> scsi 0:0:0:0: Direct-Access     ATA      QEMU HARDDISK    2.5+ PQ: 0 ANSI: 5
>>> sd 0:0:0:0: [sda] 16384 512-byte logical blocks: (8.39 MB/8.00 MiB)
>>> sd 0:0:0:0: [sda] Write Protect is off
>>> sd 0:0:0:0: [sda] Write cache: enabled, read cache: enabled, doesn't 
>>> support DPO or FUA
>>> ata1: lost interrupt (Status 0x58)
>>>
>>> [ and more similar errors ]
>>>
>>> qemu command line:
>>>
>>> qemu-system-sh4eb -M r2d -kernel arch/sh/boot/zImage \
>>>     -snapshot -drive file=rootfs.ext2,format=raw,if=ide \
>>>     -append "root=/dev/sda console=ttySC1,115200 noiotrap" \
>>>     -serial null -serial stdio -monitor null -nographic -no-reboot
>>>
>>> Bisect points to this patch (see below). Reverting it fixes the problem.
> 
> Sorry Guenter for missing your email :(
> 
>>
>>   Hi Philippe!
>>
>> Today I noticed that our sh4 test from tests/avocado/tuxrun_baselines.py 
>> is failing (which is not run by default, that's why nobody noticed), and 
>> bisection took me to the same result that Guenter already reported.
> 
> "not run by default" because flaky.
> 
> Having a quick look, the problem seems hw/ide/core.c uses non-QOM
> shortcuts. In particular ide_bus_init_output_irq() expects a preset
> IRQ. Not something trivial to fix.

I wonder whether the other spots that use ide_bus_init_output_irq() or 
similar constructs are broken now, too. Bernhard apparently already fixed 
(reverted) one in commit 143f3fd3d8b51d6526c8ea80e8a2a085f6f01cdf.

But what about fc432ba0f58343c8912b80e9056315bb9bd8df92 ?

  Thomas



^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [PATCH v3 07/11] hw/sh4/r2d: Realize IDE controller before accessing it
  2024-10-12  9:40         ` Thomas Huth
@ 2024-10-12 14:06           ` Bernhard Beschow
  2024-10-13  4:53             ` Guenter Roeck
  0 siblings, 1 reply; 45+ messages in thread
From: Bernhard Beschow @ 2024-10-12 14:06 UTC (permalink / raw)
  To: Thomas Huth, Philippe Mathieu-Daudé, John Snow,
	Markus Armbruster
  Cc: qemu-devel, Mark Cave-Ayland, Paolo Bonzini, qemu-block, qemu-ppc,
	Richard Henderson, Yoshinori Sato, Magnus Damm, Guenter Roeck



Am 12. Oktober 2024 09:40:27 UTC schrieb Thomas Huth <thuth@redhat.com>:
>On 12/10/2024 00.48, Philippe Mathieu-Daudé wrote:
>> On 11/10/24 05:23, Thomas Huth wrote:
>>> On 03/05/2024 23.34, Guenter Roeck wrote:
>>>> Hi,
>>>> 
>>>> On Thu, Feb 08, 2024 at 07:12:40PM +0100, 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>
>>>> 
>>>> qemu 9.0 fails to boot Linux from ide/ata drives with the sh4
>>>> and sh4eb emulations. Error log is as follows.
>>>> 
>>>> ata1.00: ATA-7: QEMU HARDDISK, 2.5+, max UDMA/100
>>>> ata1.00: 16384 sectors, multi 16: LBA48
>>>> ata1.00: configured for PIO
>>>> scsi 0:0:0:0: Direct-Access     ATA      QEMU HARDDISK    2.5+ PQ: 0 ANSI: 5
>>>> sd 0:0:0:0: [sda] 16384 512-byte logical blocks: (8.39 MB/8.00 MiB)
>>>> sd 0:0:0:0: [sda] Write Protect is off
>>>> sd 0:0:0:0: [sda] Write cache: enabled, read cache: enabled, doesn't support DPO or FUA
>>>> ata1: lost interrupt (Status 0x58)
>>>> 
>>>> [ and more similar errors ]
>>>> 
>>>> qemu command line:
>>>> 
>>>> qemu-system-sh4eb -M r2d -kernel arch/sh/boot/zImage \
>>>>     -snapshot -drive file=rootfs.ext2,format=raw,if=ide \
>>>>     -append "root=/dev/sda console=ttySC1,115200 noiotrap" \
>>>>     -serial null -serial stdio -monitor null -nographic -no-reboot
>>>> 
>>>> Bisect points to this patch (see below). Reverting it fixes the problem.
>> 
>> Sorry Guenter for missing your email :(
>> 
>>> 
>>>   Hi Philippe!
>>> 
>>> Today I noticed that our sh4 test from tests/avocado/tuxrun_baselines.py is failing (which is not run by default, that's why nobody noticed), and bisection took me to the same result that Guenter already reported.
>> 
>> "not run by default" because flaky.
>> 
>> Having a quick look, the problem seems hw/ide/core.c uses non-QOM
>> shortcuts. In particular ide_bus_init_output_irq() expects a preset
>> IRQ. Not something trivial to fix.
>
>I wonder whether the other spots that use ide_bus_init_output_irq() or similar constructs are broken now, too. Bernhard apparently already fixed (reverted) one in commit 143f3fd3d8b51d6526c8ea80e8a2a085f6f01cdf.
>
>But what about fc432ba0f58343c8912b80e9056315bb9bd8df92 ?

There is an indirection going on in pmac_ide_irq() which triggers real_*_irq. These get wired via sysbus API after realize() while ide_bus_init_output_irq() wires to pmac_ide_irq(). So fc432ba0f58343c8912b80e9056315bb9bd8df92 seems safe to me (haven't tested it though).

Best regards,
Bernhard

>
> Thomas
>


^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [PATCH v3 07/11] hw/sh4/r2d: Realize IDE controller before accessing it
  2024-10-12 14:06           ` Bernhard Beschow
@ 2024-10-13  4:53             ` Guenter Roeck
  2024-10-13  5:18               ` Thomas Huth
  0 siblings, 1 reply; 45+ messages in thread
From: Guenter Roeck @ 2024-10-13  4:53 UTC (permalink / raw)
  To: Bernhard Beschow, Thomas Huth, Philippe Mathieu-Daudé,
	John Snow, Markus Armbruster
  Cc: qemu-devel, Mark Cave-Ayland, Paolo Bonzini, qemu-block, qemu-ppc,
	Richard Henderson, Yoshinori Sato, Magnus Damm

On 10/12/24 07:06, Bernhard Beschow wrote:
> 
> 
> Am 12. Oktober 2024 09:40:27 UTC schrieb Thomas Huth <thuth@redhat.com>:
>> On 12/10/2024 00.48, Philippe Mathieu-Daudé wrote:
>>> On 11/10/24 05:23, Thomas Huth wrote:
>>>> On 03/05/2024 23.34, Guenter Roeck wrote:
>>>>> Hi,
>>>>>
>>>>> On Thu, Feb 08, 2024 at 07:12:40PM +0100, 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>
>>>>>
>>>>> qemu 9.0 fails to boot Linux from ide/ata drives with the sh4
>>>>> and sh4eb emulations. Error log is as follows.
>>>>>
>>>>> ata1.00: ATA-7: QEMU HARDDISK, 2.5+, max UDMA/100
>>>>> ata1.00: 16384 sectors, multi 16: LBA48
>>>>> ata1.00: configured for PIO
>>>>> scsi 0:0:0:0: Direct-Access     ATA      QEMU HARDDISK    2.5+ PQ: 0 ANSI: 5
>>>>> sd 0:0:0:0: [sda] 16384 512-byte logical blocks: (8.39 MB/8.00 MiB)
>>>>> sd 0:0:0:0: [sda] Write Protect is off
>>>>> sd 0:0:0:0: [sda] Write cache: enabled, read cache: enabled, doesn't support DPO or FUA
>>>>> ata1: lost interrupt (Status 0x58)
>>>>>
>>>>> [ and more similar errors ]
>>>>>
>>>>> qemu command line:
>>>>>
>>>>> qemu-system-sh4eb -M r2d -kernel arch/sh/boot/zImage \
>>>>>      -snapshot -drive file=rootfs.ext2,format=raw,if=ide \
>>>>>      -append "root=/dev/sda console=ttySC1,115200 noiotrap" \
>>>>>      -serial null -serial stdio -monitor null -nographic -no-reboot
>>>>>
>>>>> Bisect points to this patch (see below). Reverting it fixes the problem.
>>>
>>> Sorry Guenter for missing your email :(
>>>
>>>>
>>>>    Hi Philippe!
>>>>
>>>> Today I noticed that our sh4 test from tests/avocado/tuxrun_baselines.py is failing (which is not run by default, that's why nobody noticed), and bisection took me to the same result that Guenter already reported.
>>>
>>> "not run by default" because flaky.
>>>
>>> Having a quick look, the problem seems hw/ide/core.c uses non-QOM
>>> shortcuts. In particular ide_bus_init_output_irq() expects a preset
>>> IRQ. Not something trivial to fix.
>>
>> I wonder whether the other spots that use ide_bus_init_output_irq() or similar constructs are broken now, too. Bernhard apparently already fixed (reverted) one in commit 143f3fd3d8b51d6526c8ea80e8a2a085f6f01cdf.
>>
>> But what about fc432ba0f58343c8912b80e9056315bb9bd8df92 ?
> 
> There is an indirection going on in pmac_ide_irq() which triggers real_*_irq. These get wired via sysbus API after realize() while ide_bus_init_output_irq() wires to pmac_ide_irq(). So fc432ba0f58343c8912b80e9056315bb9bd8df92 seems safe to me (haven't tested it though).
> 

Not sure if that answers the question, but booting from ide works
for both mac99 and g3beige emulations.

Guenter



^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [PATCH v3 07/11] hw/sh4/r2d: Realize IDE controller before accessing it
  2024-10-13  4:53             ` Guenter Roeck
@ 2024-10-13  5:18               ` Thomas Huth
  0 siblings, 0 replies; 45+ messages in thread
From: Thomas Huth @ 2024-10-13  5:18 UTC (permalink / raw)
  To: Guenter Roeck, Bernhard Beschow, Philippe Mathieu-Daudé,
	John Snow, Markus Armbruster
  Cc: qemu-devel, Mark Cave-Ayland, Paolo Bonzini, qemu-block, qemu-ppc,
	Richard Henderson, Yoshinori Sato, Magnus Damm

On 13/10/2024 06.53, Guenter Roeck wrote:
> On 10/12/24 07:06, Bernhard Beschow wrote:
>>
>>
>> Am 12. Oktober 2024 09:40:27 UTC schrieb Thomas Huth <thuth@redhat.com>:
>>> On 12/10/2024 00.48, Philippe Mathieu-Daudé wrote:
>>>> On 11/10/24 05:23, Thomas Huth wrote:
>>>>> On 03/05/2024 23.34, Guenter Roeck wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On Thu, Feb 08, 2024 at 07:12:40PM +0100, 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>
>>>>>>
>>>>>> qemu 9.0 fails to boot Linux from ide/ata drives with the sh4
>>>>>> and sh4eb emulations. Error log is as follows.
>>>>>>
>>>>>> ata1.00: ATA-7: QEMU HARDDISK, 2.5+, max UDMA/100
>>>>>> ata1.00: 16384 sectors, multi 16: LBA48
>>>>>> ata1.00: configured for PIO
>>>>>> scsi 0:0:0:0: Direct-Access     ATA      QEMU HARDDISK    2.5+ PQ: 0 
>>>>>> ANSI: 5
>>>>>> sd 0:0:0:0: [sda] 16384 512-byte logical blocks: (8.39 MB/8.00 MiB)
>>>>>> sd 0:0:0:0: [sda] Write Protect is off
>>>>>> sd 0:0:0:0: [sda] Write cache: enabled, read cache: enabled, doesn't 
>>>>>> support DPO or FUA
>>>>>> ata1: lost interrupt (Status 0x58)
>>>>>>
>>>>>> [ and more similar errors ]
>>>>>>
>>>>>> qemu command line:
>>>>>>
>>>>>> qemu-system-sh4eb -M r2d -kernel arch/sh/boot/zImage \
>>>>>>      -snapshot -drive file=rootfs.ext2,format=raw,if=ide \
>>>>>>      -append "root=/dev/sda console=ttySC1,115200 noiotrap" \
>>>>>>      -serial null -serial stdio -monitor null -nographic -no-reboot
>>>>>>
>>>>>> Bisect points to this patch (see below). Reverting it fixes the problem.
>>>>
>>>> Sorry Guenter for missing your email :(
>>>>
>>>>>
>>>>>    Hi Philippe!
>>>>>
>>>>> Today I noticed that our sh4 test from tests/avocado/ 
>>>>> tuxrun_baselines.py is failing (which is not run by default, that's why 
>>>>> nobody noticed), and bisection took me to the same result that Guenter 
>>>>> already reported.
>>>>
>>>> "not run by default" because flaky.
>>>>
>>>> Having a quick look, the problem seems hw/ide/core.c uses non-QOM
>>>> shortcuts. In particular ide_bus_init_output_irq() expects a preset
>>>> IRQ. Not something trivial to fix.
>>>
>>> I wonder whether the other spots that use ide_bus_init_output_irq() or 
>>> similar constructs are broken now, too. Bernhard apparently already fixed 
>>> (reverted) one in commit 143f3fd3d8b51d6526c8ea80e8a2a085f6f01cdf.
>>>
>>> But what about fc432ba0f58343c8912b80e9056315bb9bd8df92 ?
>>
>> There is an indirection going on in pmac_ide_irq() which triggers 
>> real_*_irq. These get wired via sysbus API after realize() while 
>> ide_bus_init_output_irq() wires to pmac_ide_irq(). So 
>> fc432ba0f58343c8912b80e9056315bb9bd8df92 seems safe to me (haven't tested 
>> it though).
>>
> 
> Not sure if that answers the question, but booting from ide works
> for both mac99 and g3beige emulations.

Ok, thank you both, that sounds like we should be safe with macio IDE, indeed!

  Thomas



^ permalink raw reply	[flat|nested] 45+ messages in thread

end of thread, other threads:[~2024-10-13  5:19 UTC | newest]

Thread overview: 45+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-02-08 18:12 [PATCH v3 00/11] hw: Strengthen SysBus & QBus API Philippe Mathieu-Daudé
2024-02-08 18:12 ` [PATCH v3 01/11] hw/ide/ich9: Use AHCIPCIState typedef Philippe Mathieu-Daudé
2024-02-09 11:28   ` Peter Maydell
2024-02-08 18:12 ` [PATCH v3 02/11] hw/rx/rx62n: Reduce inclusion of 'qemu/units.h' Philippe Mathieu-Daudé
2024-02-09 11:28   ` Peter Maydell
2024-02-12 11:24   ` Yoshinori Sato
2024-02-08 18:12 ` [PATCH v3 03/11] hw/rx/rx62n: Only call qdev_get_gpio_in() when necessary Philippe Mathieu-Daudé
2024-02-09 11:29   ` Peter Maydell
2024-02-12 11:25   ` Yoshinori Sato
2024-02-08 18:12 ` [PATCH v3 04/11] hw/i386/pc_q35: Realize LPC PCI function before accessing it Philippe Mathieu-Daudé
2024-02-08 18:29   ` BALATON Zoltan
2024-02-08 18:12 ` [PATCH v3 05/11] hw/ppc/prep: Realize ISA bridge " Philippe Mathieu-Daudé
2024-02-09 11:29   ` Peter Maydell
2024-02-09 21:29   ` Mark Cave-Ayland
2024-02-08 18:12 ` [PATCH v3 06/11] hw/misc/macio: Realize IDE controller " Philippe Mathieu-Daudé
2024-02-08 18:33   ` BALATON Zoltan
2024-02-09  6:40     ` Philippe Mathieu-Daudé
2024-02-09 21:32   ` Mark Cave-Ayland
2024-02-08 18:12 ` [PATCH v3 07/11] hw/sh4/r2d: " Philippe Mathieu-Daudé
2024-02-09 11:30   ` Peter Maydell
2024-02-12 12:48   ` Yoshinori Sato
2024-05-03 21:34   ` Guenter Roeck
2024-10-11  8:23     ` Thomas Huth
2024-10-11 22:48       ` Philippe Mathieu-Daudé
2024-10-12  9:40         ` Thomas Huth
2024-10-12 14:06           ` Bernhard Beschow
2024-10-13  4:53             ` Guenter Roeck
2024-10-13  5:18               ` Thomas Huth
2024-02-08 18:12 ` [PATCH v3 08/11] hw/sparc/sun4m: Realize DMA " Philippe Mathieu-Daudé
2024-02-09 11:37   ` Peter Maydell
2024-02-09 22:37     ` Mark Cave-Ayland
2024-02-09 21:38   ` Mark Cave-Ayland
2024-02-08 18:12 ` [PATCH v3 09/11] hw/sparc/leon3: Realize GRLIB IRQ " Philippe Mathieu-Daudé
2024-02-09 11:38   ` Peter Maydell
2024-02-09 21:39   ` Mark Cave-Ayland
2024-02-08 18:12 ` [PATCH v3 10/11] hw/sparc/leon3: Initialize GPIO before realizing CPU devices Philippe Mathieu-Daudé
2024-02-09 11:35   ` Peter Maydell
2024-02-09 21:48   ` Mark Cave-Ayland
2024-02-08 18:12 ` [PATCH v3 11/11] hw/sparc64/cpu: " Philippe Mathieu-Daudé
2024-02-09 11:34   ` Peter Maydell
2024-02-09 22:01     ` Mark Cave-Ayland
2024-02-13 13:00       ` Philippe Mathieu-Daudé
2024-02-09 21:49   ` Mark Cave-Ayland
2024-10-11  8:56 ` [PATCH v3 00/11] hw: Strengthen SysBus & QBus API Paolo Bonzini
2024-10-11 22:11   ` Philippe Mathieu-Daudé

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).