[PATCH v3 00/11] hw: Strengthen SysBus & QBus API

[PATCH v3 00/11] hw: Strengthen SysBus & QBus API

[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

[PATCH v3 01/11] hw/ide/ich9: Use AHCIPCIState typedef

[Philippe Mathieu-Daudé]

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

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

Re: [PATCH v3 01/11] hw/ide/ich9: Use AHCIPCIState typedef

[Peter Maydell]

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

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

[Philippe Mathieu-Daudé]

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

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

Re: [PATCH v3 02/11] hw/rx/rx62n: Reduce inclusion of 'qemu/units.h'

[Peter Maydell]

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

Re: [PATCH v3 02/11] hw/rx/rx62n: Reduce inclusion of 'qemu/units.h'

[Yoshinori Sato]

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

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

[Philippe Mathieu-Daudé]

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

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

Re: [PATCH v3 03/11] hw/rx/rx62n: Only call qdev_get_gpio_in() when necessary

[Peter Maydell]

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

Re: [PATCH v3 03/11] hw/rx/rx62n: Only call qdev_get_gpio_in() when necessary

[Yoshinori Sato]

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

[PATCH v3 04/11] hw/i386/pc_q35: Realize LPC PCI function before accessing it

[Philippe Mathieu-Daudé]

We should not wire IRQs on unrealized device.

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

Re: [PATCH v3 04/11] hw/i386/pc_q35: Realize LPC PCI function before accessing it

[BALATON Zoltan]

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

[PATCH v3 05/11] hw/ppc/prep: Realize ISA bridge before accessing it

[Philippe Mathieu-Daudé]

We should not wire IRQs on unrealized device.

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

Re: [PATCH v3 05/11] hw/ppc/prep: Realize ISA bridge before accessing it

[Peter Maydell]

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

Re: [PATCH v3 05/11] hw/ppc/prep: Realize ISA bridge before accessing it

[Mark Cave-Ayland]

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.

[PATCH v3 06/11] hw/misc/macio: Realize IDE controller before accessing it

[Philippe Mathieu-Daudé]

We should not wire IRQs on unrealized device.

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

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

[BALATON Zoltan]

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

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

[Philippe Mathieu-Daudé]

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

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

[Mark Cave-Ayland]

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.

[PATCH v3 07/11] hw/sh4/r2d: Realize IDE controller before accessing it

[Philippe Mathieu-Daudé]

We should not wire IRQs on unrealized device.

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

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

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

[Peter Maydell]

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

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

[Yoshinori Sato]

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

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

[Guenter Roeck]

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

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

[Thomas Huth]

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

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

[Philippe Mathieu-Daudé]

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.

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

[Thomas Huth]

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

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

[Bernhard Beschow]

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
>

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

[Guenter Roeck]

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

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

[Thomas Huth]

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

[PATCH v3 08/11] hw/sparc/sun4m: Realize DMA controller before accessing it

[Philippe Mathieu-Daudé]

We should not wire IRQs on unrealized device.

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

Re: [PATCH v3 08/11] hw/sparc/sun4m: Realize DMA controller before accessing it

[Peter Maydell]

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

Re: [PATCH v3 08/11] hw/sparc/sun4m: Realize DMA controller before accessing it

[Mark Cave-Ayland]

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.

Re: [PATCH v3 08/11] hw/sparc/sun4m: Realize DMA controller before accessing it

[Mark Cave-Ayland]

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.

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

[Philippe Mathieu-Daudé]

We should not wire IRQs on unrealized device.

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

Re: [PATCH v3 09/11] hw/sparc/leon3: Realize GRLIB IRQ controller before accessing it

[Peter Maydell]

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

Re: [PATCH v3 09/11] hw/sparc/leon3: Realize GRLIB IRQ controller before accessing it

[Mark Cave-Ayland]

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.

[PATCH v3 10/11] hw/sparc/leon3: Initialize GPIO before realizing CPU devices

[Philippe Mathieu-Daudé]

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

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

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

[Peter Maydell]

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

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

[Mark Cave-Ayland]

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.

[PATCH v3 11/11] hw/sparc64/cpu: Initialize GPIO before realizing CPU devices

[Philippe Mathieu-Daudé]

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

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

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

Re: [PATCH v3 11/11] hw/sparc64/cpu: Initialize GPIO before realizing CPU devices

[Peter Maydell]

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

Re: [PATCH v3 11/11] hw/sparc64/cpu: Initialize GPIO before realizing CPU devices

[Mark Cave-Ayland]

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.

Re: [PATCH v3 11/11] hw/sparc64/cpu: Initialize GPIO before realizing CPU devices

[Philippe Mathieu-Daudé]

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.

Re: [PATCH v3 11/11] hw/sparc64/cpu: Initialize GPIO before realizing CPU devices

[Mark Cave-Ayland]

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.

Re: [PATCH v3 00/11] hw: Strengthen SysBus & QBus API

[Paolo Bonzini]

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

Re: [PATCH v3 00/11] hw: Strengthen SysBus & QBus API

[Philippe Mathieu-Daudé]

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