[PATCH v2 0/2] i.MX 8M Plus EVK Fixes

[PATCH v2 0/2] i.MX 8M Plus EVK Fixes

[Bernhard Beschow]

As discussed in [1], this series modifies the SoC class be derived from
TYPE_SYS_BUS_DEVICE to fix the reset mechanism and to prevent it from being
user-creatable. It also removes an unused define.

v2:
* Do not set user_creatable = false; (Zoltan, Peter)

[1] https://lore.kernel.org/qemu-devel/1cdb6643-8fcc-4bd8-93fc-fcc93589c9a3@redhat.com/

Bernhard Beschow (2):
  hw/arm/fsl-imx8mp: Derive struct FslImx8mpState from
    TYPE_SYS_BUS_DEVICE
  hw/arm/fsl-imx8mp: Remove unused define

 include/hw/arm/fsl-imx8mp.h | 4 ++--
 hw/arm/fsl-imx8mp.c         | 2 +-
 hw/arm/imx8mp-evk.c         | 2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)

-- 
2.48.1

[PATCH v2 1/2] hw/arm/fsl-imx8mp: Derive struct FslImx8mpState from TYPE_SYS_BUS_DEVICE

[Bernhard Beschow]

Deriving from TYPE_SYS_BUS_DEVICE fixes the SoC object to be reset upon machine
reset. It also makes the SoC implementation not user-creatable which can trigger
the following crash:

  $ ./qemu-system-aarch64  -M virt -device fsl-imx8mp
  **
  ERROR:../../devel/qemu/tcg/tcg.c:1006:tcg_register_thread: assertion failed:
  (n < tcg_max_ctxs)
  Bail out! ERROR:../../devel/qemu/tcg/tcg.c:1006:tcg_register_thread:
  assertion failed: (n < tcg_max_ctxs)
  Aborted (core dumped)

Fixes: a4eefc69b237 "hw/arm: Add i.MX 8M Plus EVK board"
Reported-by: Thomas Huth <thuth@redhat.com>
Suggested-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 include/hw/arm/fsl-imx8mp.h | 3 ++-
 hw/arm/fsl-imx8mp.c         | 2 +-
 hw/arm/imx8mp-evk.c         | 2 +-
 3 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/include/hw/arm/fsl-imx8mp.h b/include/hw/arm/fsl-imx8mp.h
index bc97fc416e..22fdc0d67c 100644
--- a/include/hw/arm/fsl-imx8mp.h
+++ b/include/hw/arm/fsl-imx8mp.h
@@ -26,6 +26,7 @@
 #include "hw/timer/imx_gpt.h"
 #include "hw/usb/hcd-dwc3.h"
 #include "hw/watchdog/wdt_imx2.h"
+#include "hw/sysbus.h"
 #include "qom/object.h"
 #include "qemu/units.h"
 
@@ -49,7 +50,7 @@ enum FslImx8mpConfiguration {
 };
 
 struct FslImx8mpState {
-    DeviceState    parent_obj;
+    SysBusDevice   parent_obj;
 
     ARMCPU             cpu[FSL_IMX8MP_NUM_CPUS];
     GICv3State         gic;
diff --git a/hw/arm/fsl-imx8mp.c b/hw/arm/fsl-imx8mp.c
index c3f6da6322..82edf61082 100644
--- a/hw/arm/fsl-imx8mp.c
+++ b/hw/arm/fsl-imx8mp.c
@@ -702,7 +702,7 @@ static void fsl_imx8mp_class_init(ObjectClass *oc, void *data)
 static const TypeInfo fsl_imx8mp_types[] = {
     {
         .name = TYPE_FSL_IMX8MP,
-        .parent = TYPE_DEVICE,
+        .parent = TYPE_SYS_BUS_DEVICE,
         .instance_size = sizeof(FslImx8mpState),
         .instance_init = fsl_imx8mp_init,
         .class_init = fsl_imx8mp_class_init,
diff --git a/hw/arm/imx8mp-evk.c b/hw/arm/imx8mp-evk.c
index e1a7892fd7..f17d5db466 100644
--- a/hw/arm/imx8mp-evk.c
+++ b/hw/arm/imx8mp-evk.c
@@ -37,7 +37,7 @@ static void imx8mp_evk_init(MachineState *machine)
     s = FSL_IMX8MP(object_new(TYPE_FSL_IMX8MP));
     object_property_add_child(OBJECT(machine), "soc", OBJECT(s));
     object_property_set_uint(OBJECT(s), "fec1-phy-num", 1, &error_fatal);
-    qdev_realize(DEVICE(s), NULL, &error_fatal);
+    sysbus_realize_and_unref(SYS_BUS_DEVICE(s), &error_fatal);
 
     memory_region_add_subregion(get_system_memory(), FSL_IMX8MP_RAM_START,
                                 machine->ram);
-- 
2.48.1

[PATCH v2 2/2] hw/arm/fsl-imx8mp: Remove unused define

[Bernhard Beschow]

The SoC has three SPI controllers, not four. Remove the extra define of an SPI
IRQ.

Fixes: 06908a84f036 "hw/arm/fsl-imx8mp: Add SPI controllers"
Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 include/hw/arm/fsl-imx8mp.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/include/hw/arm/fsl-imx8mp.h b/include/hw/arm/fsl-imx8mp.h
index 22fdc0d67c..d016f7d337 100644
--- a/include/hw/arm/fsl-imx8mp.h
+++ b/include/hw/arm/fsl-imx8mp.h
@@ -238,7 +238,6 @@ enum FslImx8mpIrqs {
     FSL_IMX8MP_ECSPI1_IRQ   = 31,
     FSL_IMX8MP_ECSPI2_IRQ   = 32,
     FSL_IMX8MP_ECSPI3_IRQ   = 33,
-    FSL_IMX8MP_ECSPI4_IRQ   = 34,
 
     FSL_IMX8MP_I2C1_IRQ     = 35,
     FSL_IMX8MP_I2C2_IRQ     = 36,
-- 
2.48.1

Re: [PATCH v2 2/2] hw/arm/fsl-imx8mp: Remove unused define

[Peter Maydell]

On Fri, 14 Mar 2025 at 18:32, Bernhard Beschow <shentey@gmail.com> wrote:
>
> The SoC has three SPI controllers, not four. Remove the extra define of an SPI
> IRQ.
>
> Fixes: 06908a84f036 "hw/arm/fsl-imx8mp: Add SPI controllers"
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>

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

thanks
-- PMM

Re: [PATCH v2 1/2] hw/arm/fsl-imx8mp: Derive struct FslImx8mpState from TYPE_SYS_BUS_DEVICE

[Peter Maydell]

On Fri, 14 Mar 2025 at 18:32, Bernhard Beschow <shentey@gmail.com> wrote:
>
> Deriving from TYPE_SYS_BUS_DEVICE fixes the SoC object to be reset upon machine
> reset. It also makes the SoC implementation not user-creatable which can trigger
> the following crash:
>
>   $ ./qemu-system-aarch64  -M virt -device fsl-imx8mp
>   **
>   ERROR:../../devel/qemu/tcg/tcg.c:1006:tcg_register_thread: assertion failed:
>   (n < tcg_max_ctxs)
>   Bail out! ERROR:../../devel/qemu/tcg/tcg.c:1006:tcg_register_thread:
>   assertion failed: (n < tcg_max_ctxs)
>   Aborted (core dumped)

> diff --git a/hw/arm/fsl-imx8mp.c b/hw/arm/fsl-imx8mp.c
> index c3f6da6322..82edf61082 100644
> --- a/hw/arm/fsl-imx8mp.c
> +++ b/hw/arm/fsl-imx8mp.c
> @@ -702,7 +702,7 @@ static void fsl_imx8mp_class_init(ObjectClass *oc, void *data)
>  static const TypeInfo fsl_imx8mp_types[] = {
>      {
>          .name = TYPE_FSL_IMX8MP,
> -        .parent = TYPE_DEVICE,
> +        .parent = TYPE_SYS_BUS_DEVICE,
>          .instance_size = sizeof(FslImx8mpState),
>          .instance_init = fsl_imx8mp_init,
>          .class_init = fsl_imx8mp_class_init,
> diff --git a/hw/arm/imx8mp-evk.c b/hw/arm/imx8mp-evk.c
> index e1a7892fd7..f17d5db466 100644
> --- a/hw/arm/imx8mp-evk.c
> +++ b/hw/arm/imx8mp-evk.c
> @@ -37,7 +37,7 @@ static void imx8mp_evk_init(MachineState *machine)
>      s = FSL_IMX8MP(object_new(TYPE_FSL_IMX8MP));
>      object_property_add_child(OBJECT(machine), "soc", OBJECT(s));
>      object_property_set_uint(OBJECT(s), "fec1-phy-num", 1, &error_fatal);
> -    qdev_realize(DEVICE(s), NULL, &error_fatal);
> +    sysbus_realize_and_unref(SYS_BUS_DEVICE(s), &error_fatal);

You want sysbus_realize() here, not the _and_unref() variant,
because the device was created with object_initialize_child().

The pairing is:
 * object_initialize_child() + sysbus_realize() / qdev_realize()
 * qdev_new() + sysbus_realize_and_unref() / qdev_realize_and_unref()

(See the doc comment in include/hw/qdev-core.h for
qdev_realize_and_unref() for more detail.)

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

thanks
-- PMM

Re: [PATCH v2 1/2] hw/arm/fsl-imx8mp: Derive struct FslImx8mpState from TYPE_SYS_BUS_DEVICE

[Peter Maydell]

On Tue, 18 Mar 2025 at 15:27, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Fri, 14 Mar 2025 at 18:32, Bernhard Beschow <shentey@gmail.com> wrote:
> >
> > Deriving from TYPE_SYS_BUS_DEVICE fixes the SoC object to be reset upon machine
> > reset. It also makes the SoC implementation not user-creatable which can trigger
> > the following crash:
> >
> >   $ ./qemu-system-aarch64  -M virt -device fsl-imx8mp
> >   **
> >   ERROR:../../devel/qemu/tcg/tcg.c:1006:tcg_register_thread: assertion failed:
> >   (n < tcg_max_ctxs)
> >   Bail out! ERROR:../../devel/qemu/tcg/tcg.c:1006:tcg_register_thread:
> >   assertion failed: (n < tcg_max_ctxs)
> >   Aborted (core dumped)
>
> > diff --git a/hw/arm/fsl-imx8mp.c b/hw/arm/fsl-imx8mp.c
> > index c3f6da6322..82edf61082 100644
> > --- a/hw/arm/fsl-imx8mp.c
> > +++ b/hw/arm/fsl-imx8mp.c
> > @@ -702,7 +702,7 @@ static void fsl_imx8mp_class_init(ObjectClass *oc, void *data)
> >  static const TypeInfo fsl_imx8mp_types[] = {
> >      {
> >          .name = TYPE_FSL_IMX8MP,
> > -        .parent = TYPE_DEVICE,
> > +        .parent = TYPE_SYS_BUS_DEVICE,
> >          .instance_size = sizeof(FslImx8mpState),
> >          .instance_init = fsl_imx8mp_init,
> >          .class_init = fsl_imx8mp_class_init,
> > diff --git a/hw/arm/imx8mp-evk.c b/hw/arm/imx8mp-evk.c
> > index e1a7892fd7..f17d5db466 100644
> > --- a/hw/arm/imx8mp-evk.c
> > +++ b/hw/arm/imx8mp-evk.c
> > @@ -37,7 +37,7 @@ static void imx8mp_evk_init(MachineState *machine)
> >      s = FSL_IMX8MP(object_new(TYPE_FSL_IMX8MP));
> >      object_property_add_child(OBJECT(machine), "soc", OBJECT(s));
> >      object_property_set_uint(OBJECT(s), "fec1-phy-num", 1, &error_fatal);
> > -    qdev_realize(DEVICE(s), NULL, &error_fatal);
> > +    sysbus_realize_and_unref(SYS_BUS_DEVICE(s), &error_fatal);
>
> You want sysbus_realize() here, not the _and_unref() variant,
> because the device was created with object_initialize_child().

No, that's wrong, we create it with object_new(). So
the _and_unref() *is* correct, but this is a separate bug fix
from the "should be sysbus, not qdev" bug this patch says
it is fixing. Can it be in a separate patch, please?

thanks
-- PMM

Re: [PATCH v2 1/2] hw/arm/fsl-imx8mp: Derive struct FslImx8mpState from TYPE_SYS_BUS_DEVICE

[Bernhard Beschow]

Am 18. März 2025 15:29:17 UTC schrieb Peter Maydell <peter.maydell@linaro.org>:
>On Tue, 18 Mar 2025 at 15:27, Peter Maydell <peter.maydell@linaro.org> wrote:
>>
>> On Fri, 14 Mar 2025 at 18:32, Bernhard Beschow <shentey@gmail.com> wrote:
>> >
>> > Deriving from TYPE_SYS_BUS_DEVICE fixes the SoC object to be reset upon machine
>> > reset. It also makes the SoC implementation not user-creatable which can trigger
>> > the following crash:
>> >
>> >   $ ./qemu-system-aarch64  -M virt -device fsl-imx8mp
>> >   **
>> >   ERROR:../../devel/qemu/tcg/tcg.c:1006:tcg_register_thread: assertion failed:
>> >   (n < tcg_max_ctxs)
>> >   Bail out! ERROR:../../devel/qemu/tcg/tcg.c:1006:tcg_register_thread:
>> >   assertion failed: (n < tcg_max_ctxs)
>> >   Aborted (core dumped)
>>
>> > diff --git a/hw/arm/fsl-imx8mp.c b/hw/arm/fsl-imx8mp.c
>> > index c3f6da6322..82edf61082 100644
>> > --- a/hw/arm/fsl-imx8mp.c
>> > +++ b/hw/arm/fsl-imx8mp.c
>> > @@ -702,7 +702,7 @@ static void fsl_imx8mp_class_init(ObjectClass *oc, void *data)
>> >  static const TypeInfo fsl_imx8mp_types[] = {
>> >      {
>> >          .name = TYPE_FSL_IMX8MP,
>> > -        .parent = TYPE_DEVICE,
>> > +        .parent = TYPE_SYS_BUS_DEVICE,
>> >          .instance_size = sizeof(FslImx8mpState),
>> >          .instance_init = fsl_imx8mp_init,
>> >          .class_init = fsl_imx8mp_class_init,
>> > diff --git a/hw/arm/imx8mp-evk.c b/hw/arm/imx8mp-evk.c
>> > index e1a7892fd7..f17d5db466 100644
>> > --- a/hw/arm/imx8mp-evk.c
>> > +++ b/hw/arm/imx8mp-evk.c
>> > @@ -37,7 +37,7 @@ static void imx8mp_evk_init(MachineState *machine)
>> >      s = FSL_IMX8MP(object_new(TYPE_FSL_IMX8MP));
>> >      object_property_add_child(OBJECT(machine), "soc", OBJECT(s));
>> >      object_property_set_uint(OBJECT(s), "fec1-phy-num", 1, &error_fatal);
>> > -    qdev_realize(DEVICE(s), NULL, &error_fatal);
>> > +    sysbus_realize_and_unref(SYS_BUS_DEVICE(s), &error_fatal);
>>
>> You want sysbus_realize() here, not the _and_unref() variant,
>> because the device was created with object_initialize_child().
>
>No, that's wrong, we create it with object_new(). So
>the _and_unref() *is* correct, but this is a separate bug fix
>from the "should be sysbus, not qdev" bug this patch says
>it is fixing. Can it be in a separate patch, please?

Sure, fixed in v3.

Best regards,
Bernhard

>
>thanks
>-- PMM