[PATCH for-5.1 0/2] Unbreak make check SPEED=slow

[PATCH for-5.1 0/2] Unbreak make check SPEED=slow

[Markus Armbruster]

On the one hand, getting bitten by this bug in production seems
somewhat unlikely.  On the other hand, shipping with broken "make
check" would be embarrassing.  So let's consider the fix for 5.1.

Markus Armbruster (2):
  msf2: Unbreak device-list-properties for "msf-soc"
  hw: Mark nd_table[] misuse in realize methods FIXME

 hw/arm/allwinner-h3.c | 1 +
 hw/arm/msf2-soc.c     | 9 +++++----
 hw/arm/xlnx-versal.c  | 1 +
 hw/arm/xlnx-zynqmp.c  | 1 +
 hw/dma/sparc32_dma.c  | 1 +
 hw/riscv/sifive_u.c   | 1 +
 6 files changed, 10 insertions(+), 4 deletions(-)

-- 
2.26.2

[PATCH for-5.1 1/2] msf2: Unbreak device-list-properties for "msf-soc"

[Markus Armbruster]

Watch this:

    $ qemu-system-aarch64 -M ast2600-evb -S -display none -qmp stdio
    {"QMP": {"version": {"qemu": {"micro": 50, "minor": 0, "major": 5}, "package": "v5.0.0-2464-g3a9163af4e"}, "capabilities": ["oob"]}}
    {"execute": "qmp_capabilities"}
    {"return": {}}
    {"execute": "device-list-properties", "arguments": {"typename": "msf2-soc"}}
    Unsupported NIC model: ftgmac100
    armbru@dusky:~/work/images$ echo $?
    1

This is what breaks "make check SPEED=slow".

Root cause is m2sxxx_soc_initfn()'s messing with nd_table[] via
qemu_check_nic_model().  That's wrong.

We fixed the exact same bug for device "allwinner-a10" in commit
8aabc5437b "hw/arm/allwinner-a10: Do not use nd_table in instance_init
function".  Fix this instance the same way: move the offending code to
m2sxxx_soc_realize(), where it's less wrong, and add a FIXME comment.

Fixes: 05b7374a58cd18aa3516e33513808896d0ac9b7b
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/arm/msf2-soc.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/hw/arm/msf2-soc.c b/hw/arm/msf2-soc.c
index 16bb7c9916..33ea7df342 100644
--- a/hw/arm/msf2-soc.c
+++ b/hw/arm/msf2-soc.c
@@ -82,10 +82,6 @@ static void m2sxxx_soc_initfn(Object *obj)
     }
 
     object_initialize_child(obj, "emac", &s->emac, TYPE_MSS_EMAC);
-    if (nd_table[0].used) {
-        qemu_check_nic_model(&nd_table[0], TYPE_MSS_EMAC);
-        qdev_set_nic_properties(DEVICE(&s->emac), &nd_table[0]);
-    }
 }
 
 static void m2sxxx_soc_realize(DeviceState *dev_soc, Error **errp)
@@ -187,6 +183,11 @@ static void m2sxxx_soc_realize(DeviceState *dev_soc, Error **errp)
         g_free(bus_name);
     }
 
+    /* FIXME use qdev NIC properties instead of nd_table[] */
+    if (nd_table[0].used) {
+        qemu_check_nic_model(&nd_table[0], TYPE_MSS_EMAC);
+        qdev_set_nic_properties(DEVICE(&s->emac), &nd_table[0]);
+    }
     dev = DEVICE(&s->emac);
     object_property_set_link(OBJECT(&s->emac), "ahb-bus",
                              OBJECT(get_system_memory()), &error_abort);
-- 
2.26.2

[PATCH for-5.1 2/2] hw: Mark nd_table[] misuse in realize methods FIXME

[Markus Armbruster]

nd_table[] contains NIC configuration for boards to pick up.  Device
code has no business looking there.  Several devices do it anyway.
Two of them already have a suitable FIXME comment: "allwinner-a10" and
"msf2-soc".  Copy it to the others: "allwinner-h3", "xlnx-versal",
"xlnx,zynqmp", "sparc32-ledma", "riscv.sifive.u.soc".

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/arm/allwinner-h3.c | 1 +
 hw/arm/xlnx-versal.c  | 1 +
 hw/arm/xlnx-zynqmp.c  | 1 +
 hw/dma/sparc32_dma.c  | 1 +
 hw/riscv/sifive_u.c   | 1 +
 5 files changed, 5 insertions(+)

diff --git a/hw/arm/allwinner-h3.c b/hw/arm/allwinner-h3.c
index 8e09468e86..ff92ded82c 100644
--- a/hw/arm/allwinner-h3.c
+++ b/hw/arm/allwinner-h3.c
@@ -358,6 +358,7 @@ static void allwinner_h3_realize(DeviceState *dev, Error **errp)
                               "sd-bus");
 
     /* EMAC */
+    /* FIXME use qdev NIC properties instead of nd_table[] */
     if (nd_table[0].used) {
         qemu_check_nic_model(&nd_table[0], TYPE_AW_SUN8I_EMAC);
         qdev_set_nic_properties(DEVICE(&s->emac), &nd_table[0]);
diff --git a/hw/arm/xlnx-versal.c b/hw/arm/xlnx-versal.c
index ead038b971..e3aa4bd1e5 100644
--- a/hw/arm/xlnx-versal.c
+++ b/hw/arm/xlnx-versal.c
@@ -160,6 +160,7 @@ static void versal_create_gems(Versal *s, qemu_irq *pic)
         object_initialize_child(OBJECT(s), name, &s->lpd.iou.gem[i],
                                 TYPE_CADENCE_GEM);
         dev = DEVICE(&s->lpd.iou.gem[i]);
+        /* FIXME use qdev NIC properties instead of nd_table[] */
         if (nd->used) {
             qemu_check_nic_model(nd, "cadence_gem");
             qdev_set_nic_properties(dev, nd);
diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c
index 772cfa3771..5855e5d5bf 100644
--- a/hw/arm/xlnx-zynqmp.c
+++ b/hw/arm/xlnx-zynqmp.c
@@ -455,6 +455,7 @@ static void xlnx_zynqmp_realize(DeviceState *dev, Error **errp)
     for (i = 0; i < XLNX_ZYNQMP_NUM_GEMS; i++) {
         NICInfo *nd = &nd_table[i];
 
+        /* FIXME use qdev NIC properties instead of nd_table[] */
         if (nd->used) {
             qemu_check_nic_model(nd, TYPE_CADENCE_GEM);
             qdev_set_nic_properties(DEVICE(&s->gem[i]), nd);
diff --git a/hw/dma/sparc32_dma.c b/hw/dma/sparc32_dma.c
index 9459178866..bcd1626fbd 100644
--- a/hw/dma/sparc32_dma.c
+++ b/hw/dma/sparc32_dma.c
@@ -341,6 +341,7 @@ static void sparc32_ledma_device_realize(DeviceState *dev, Error **errp)
     DeviceState *d;
     NICInfo *nd = &nd_table[0];
 
+    /* FIXME use qdev NIC properties instead of nd_table[] */
     qemu_check_nic_model(nd, TYPE_LANCE);
 
     d = qdev_new(TYPE_LANCE);
diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
index 19a976c9a6..e5682c38a9 100644
--- a/hw/riscv/sifive_u.c
+++ b/hw/riscv/sifive_u.c
@@ -714,6 +714,7 @@ static void sifive_u_soc_realize(DeviceState *dev, Error **errp)
     }
     sysbus_mmio_map(SYS_BUS_DEVICE(&s->otp), 0, memmap[SIFIVE_U_OTP].base);
 
+    /* FIXME use qdev NIC properties instead of nd_table[] */
     if (nd->used) {
         qemu_check_nic_model(nd, TYPE_CADENCE_GEM);
         qdev_set_nic_properties(DEVICE(&s->gem), nd);
-- 
2.26.2

Re: [PATCH for-5.1 1/2] msf2: Unbreak device-list-properties for "msf-soc"

[Philippe Mathieu-Daudé]

On 7/15/20 4:04 PM, Markus Armbruster wrote:
> Watch this:
> 
>     $ qemu-system-aarch64 -M ast2600-evb -S -display none -qmp stdio
>     {"QMP": {"version": {"qemu": {"micro": 50, "minor": 0, "major": 5}, "package": "v5.0.0-2464-g3a9163af4e"}, "capabilities": ["oob"]}}
>     {"execute": "qmp_capabilities"}
>     {"return": {}}
>     {"execute": "device-list-properties", "arguments": {"typename": "msf2-soc"}}
>     Unsupported NIC model: ftgmac100
>     armbru@dusky:~/work/images$ echo $?
>     1
> 
> This is what breaks "make check SPEED=slow".
> 
> Root cause is m2sxxx_soc_initfn()'s messing with nd_table[] via
> qemu_check_nic_model().  That's wrong.
> 
> We fixed the exact same bug for device "allwinner-a10" in commit
> 8aabc5437b "hw/arm/allwinner-a10: Do not use nd_table in instance_init
> function".  Fix this instance the same way: move the offending code to
> m2sxxx_soc_realize(), where it's less wrong, and add a FIXME comment.

That addresses this other thread, right?
https://www.mail-archive.com/qemu-devel@nongnu.org/msg720658.html

> 
> Fixes: 05b7374a58cd18aa3516e33513808896d0ac9b7b
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  hw/arm/msf2-soc.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/arm/msf2-soc.c b/hw/arm/msf2-soc.c
> index 16bb7c9916..33ea7df342 100644
> --- a/hw/arm/msf2-soc.c
> +++ b/hw/arm/msf2-soc.c
> @@ -82,10 +82,6 @@ static void m2sxxx_soc_initfn(Object *obj)
>      }
>  
>      object_initialize_child(obj, "emac", &s->emac, TYPE_MSS_EMAC);
> -    if (nd_table[0].used) {
> -        qemu_check_nic_model(&nd_table[0], TYPE_MSS_EMAC);
> -        qdev_set_nic_properties(DEVICE(&s->emac), &nd_table[0]);
> -    }
>  }
>  
>  static void m2sxxx_soc_realize(DeviceState *dev_soc, Error **errp)
> @@ -187,6 +183,11 @@ static void m2sxxx_soc_realize(DeviceState *dev_soc, Error **errp)
>          g_free(bus_name);
>      }
>  
> +    /* FIXME use qdev NIC properties instead of nd_table[] */
> +    if (nd_table[0].used) {
> +        qemu_check_nic_model(&nd_table[0], TYPE_MSS_EMAC);
> +        qdev_set_nic_properties(DEVICE(&s->emac), &nd_table[0]);
> +    }
>      dev = DEVICE(&s->emac);
>      object_property_set_link(OBJECT(&s->emac), "ahb-bus",
>                               OBJECT(get_system_memory()), &error_abort);
> 

Re: [PATCH for-5.1 1/2] msf2: Unbreak device-list-properties for "msf-soc"

[Thomas Huth]

On 15/07/2020 16.04, Markus Armbruster wrote:
> Watch this:
> 
>     $ qemu-system-aarch64 -M ast2600-evb -S -display none -qmp stdio
>     {"QMP": {"version": {"qemu": {"micro": 50, "minor": 0, "major": 5}, "package": "v5.0.0-2464-g3a9163af4e"}, "capabilities": ["oob"]}}
>     {"execute": "qmp_capabilities"}
>     {"return": {}}
>     {"execute": "device-list-properties", "arguments": {"typename": "msf2-soc"}}
>     Unsupported NIC model: ftgmac100
>     armbru@dusky:~/work/images$ echo $?
>     1
> 
> This is what breaks "make check SPEED=slow".
> 
> Root cause is m2sxxx_soc_initfn()'s messing with nd_table[] via
> qemu_check_nic_model().  That's wrong.
> 
> We fixed the exact same bug for device "allwinner-a10" in commit
> 8aabc5437b "hw/arm/allwinner-a10: Do not use nd_table in instance_init
> function".  Fix this instance the same way: move the offending code to
> m2sxxx_soc_realize(), where it's less wrong, and add a FIXME comment.
> 
> Fixes: 05b7374a58cd18aa3516e33513808896d0ac9b7b
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  hw/arm/msf2-soc.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/arm/msf2-soc.c b/hw/arm/msf2-soc.c
> index 16bb7c9916..33ea7df342 100644
> --- a/hw/arm/msf2-soc.c
> +++ b/hw/arm/msf2-soc.c
> @@ -82,10 +82,6 @@ static void m2sxxx_soc_initfn(Object *obj)
>      }
>  
>      object_initialize_child(obj, "emac", &s->emac, TYPE_MSS_EMAC);
> -    if (nd_table[0].used) {
> -        qemu_check_nic_model(&nd_table[0], TYPE_MSS_EMAC);
> -        qdev_set_nic_properties(DEVICE(&s->emac), &nd_table[0]);
> -    }
>  }
>  
>  static void m2sxxx_soc_realize(DeviceState *dev_soc, Error **errp)
> @@ -187,6 +183,11 @@ static void m2sxxx_soc_realize(DeviceState *dev_soc, Error **errp)
>          g_free(bus_name);
>      }
>  
> +    /* FIXME use qdev NIC properties instead of nd_table[] */
> +    if (nd_table[0].used) {
> +        qemu_check_nic_model(&nd_table[0], TYPE_MSS_EMAC);
> +        qdev_set_nic_properties(DEVICE(&s->emac), &nd_table[0]);
> +    }
>      dev = DEVICE(&s->emac);
>      object_property_set_link(OBJECT(&s->emac), "ahb-bus",
>                               OBJECT(get_system_memory()), &error_abort);

As long as nobody comes up with a proper clean up within the next days
that moves the nd_table[] access to msf2-som.c, I think this is an
acceptable work-around for 5.1.

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH for-5.1 2/2] hw: Mark nd_table[] misuse in realize methods FIXME

[Thomas Huth]

On 15/07/2020 16.04, Markus Armbruster wrote:
> nd_table[] contains NIC configuration for boards to pick up.  Device
> code has no business looking there.  Several devices do it anyway.
> Two of them already have a suitable FIXME comment: "allwinner-a10" and
> "msf2-soc".  Copy it to the others: "allwinner-h3", "xlnx-versal",
> "xlnx,zynqmp", "sparc32-ledma", "riscv.sifive.u.soc".
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  hw/arm/allwinner-h3.c | 1 +
>  hw/arm/xlnx-versal.c  | 1 +
>  hw/arm/xlnx-zynqmp.c  | 1 +
>  hw/dma/sparc32_dma.c  | 1 +
>  hw/riscv/sifive_u.c   | 1 +
>  5 files changed, 5 insertions(+)

Maybe we should add an entry to
https://wiki.qemu.org/Contribute/BiteSizedTasks for this?

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH for-5.1 0/2] Unbreak make check SPEED=slow

[no-reply]

Patchew URL: https://patchew.org/QEMU/20200715140440.3540942-1-armbru@redhat.com/



Hi,

This series failed the docker-quick@centos7 build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
make docker-image-centos7 V=1 NETWORK=1
time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1
=== TEST SCRIPT END ===

qemu-system-aarch64: falling back to tcg
  TEST    check-unit: tests/test-char
**
ERROR:/tmp/qemu-test/src/tests/test-char.c:1204:char_serial_test: 'chr' should not be NULL
ERROR test-char - Bail out! ERROR:/tmp/qemu-test/src/tests/test-char.c:1204:char_serial_test: 'chr' should not be NULL
make: *** [check-unit] Error 1
make: *** Waiting for unfinished jobs....
  TEST    iotest-qcow2: 025
  TEST    iotest-qcow2: 027
---
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=aa283144540a4d70bf303fb95ee05619', '-u', '1003', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=1', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew2/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-sn6v_tr1/src/docker-src.2020-07-15-10.15.38.17277:/var/tmp/qemu:z,ro', 'qemu/centos7', '/var/tmp/qemu/run', 'test-quick']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=aa283144540a4d70bf303fb95ee05619
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-sn6v_tr1/src'
make: *** [docker-run-test-quick@centos7] Error 2

real    14m52.787s
user    0m8.650s


The full log is available at
http://patchew.org/logs/20200715140440.3540942-1-armbru@redhat.com/testing.docker-quick@centos7/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [PATCH for-5.1 1/2] msf2: Unbreak device-list-properties for "msf-soc"

[Alistair Francis]

On Wed, Jul 15, 2020 at 7:08 AM Markus Armbruster <armbru@redhat.com> wrote:
>
> Watch this:
>
>     $ qemu-system-aarch64 -M ast2600-evb -S -display none -qmp stdio
>     {"QMP": {"version": {"qemu": {"micro": 50, "minor": 0, "major": 5}, "package": "v5.0.0-2464-g3a9163af4e"}, "capabilities": ["oob"]}}
>     {"execute": "qmp_capabilities"}
>     {"return": {}}
>     {"execute": "device-list-properties", "arguments": {"typename": "msf2-soc"}}
>     Unsupported NIC model: ftgmac100
>     armbru@dusky:~/work/images$ echo $?
>     1
>
> This is what breaks "make check SPEED=slow".
>
> Root cause is m2sxxx_soc_initfn()'s messing with nd_table[] via
> qemu_check_nic_model().  That's wrong.
>
> We fixed the exact same bug for device "allwinner-a10" in commit
> 8aabc5437b "hw/arm/allwinner-a10: Do not use nd_table in instance_init
> function".  Fix this instance the same way: move the offending code to
> m2sxxx_soc_realize(), where it's less wrong, and add a FIXME comment.
>
> Fixes: 05b7374a58cd18aa3516e33513808896d0ac9b7b
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  hw/arm/msf2-soc.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/hw/arm/msf2-soc.c b/hw/arm/msf2-soc.c
> index 16bb7c9916..33ea7df342 100644
> --- a/hw/arm/msf2-soc.c
> +++ b/hw/arm/msf2-soc.c
> @@ -82,10 +82,6 @@ static void m2sxxx_soc_initfn(Object *obj)
>      }
>
>      object_initialize_child(obj, "emac", &s->emac, TYPE_MSS_EMAC);
> -    if (nd_table[0].used) {
> -        qemu_check_nic_model(&nd_table[0], TYPE_MSS_EMAC);
> -        qdev_set_nic_properties(DEVICE(&s->emac), &nd_table[0]);
> -    }
>  }
>
>  static void m2sxxx_soc_realize(DeviceState *dev_soc, Error **errp)
> @@ -187,6 +183,11 @@ static void m2sxxx_soc_realize(DeviceState *dev_soc, Error **errp)
>          g_free(bus_name);
>      }
>
> +    /* FIXME use qdev NIC properties instead of nd_table[] */
> +    if (nd_table[0].used) {
> +        qemu_check_nic_model(&nd_table[0], TYPE_MSS_EMAC);
> +        qdev_set_nic_properties(DEVICE(&s->emac), &nd_table[0]);
> +    }
>      dev = DEVICE(&s->emac);
>      object_property_set_link(OBJECT(&s->emac), "ahb-bus",
>                               OBJECT(get_system_memory()), &error_abort);
> --
> 2.26.2
>
>

Re: [PATCH for-5.1 2/2] hw: Mark nd_table[] misuse in realize methods FIXME

[Alistair Francis]

On Wed, Jul 15, 2020 at 7:06 AM Markus Armbruster <armbru@redhat.com> wrote:
>
> nd_table[] contains NIC configuration for boards to pick up.  Device
> code has no business looking there.  Several devices do it anyway.
> Two of them already have a suitable FIXME comment: "allwinner-a10" and
> "msf2-soc".  Copy it to the others: "allwinner-h3", "xlnx-versal",
> "xlnx,zynqmp", "sparc32-ledma", "riscv.sifive.u.soc".
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  hw/arm/allwinner-h3.c | 1 +
>  hw/arm/xlnx-versal.c  | 1 +
>  hw/arm/xlnx-zynqmp.c  | 1 +
>  hw/dma/sparc32_dma.c  | 1 +
>  hw/riscv/sifive_u.c   | 1 +
>  5 files changed, 5 insertions(+)
>
> diff --git a/hw/arm/allwinner-h3.c b/hw/arm/allwinner-h3.c
> index 8e09468e86..ff92ded82c 100644
> --- a/hw/arm/allwinner-h3.c
> +++ b/hw/arm/allwinner-h3.c
> @@ -358,6 +358,7 @@ static void allwinner_h3_realize(DeviceState *dev, Error **errp)
>                                "sd-bus");
>
>      /* EMAC */
> +    /* FIXME use qdev NIC properties instead of nd_table[] */
>      if (nd_table[0].used) {
>          qemu_check_nic_model(&nd_table[0], TYPE_AW_SUN8I_EMAC);
>          qdev_set_nic_properties(DEVICE(&s->emac), &nd_table[0]);
> diff --git a/hw/arm/xlnx-versal.c b/hw/arm/xlnx-versal.c
> index ead038b971..e3aa4bd1e5 100644
> --- a/hw/arm/xlnx-versal.c
> +++ b/hw/arm/xlnx-versal.c
> @@ -160,6 +160,7 @@ static void versal_create_gems(Versal *s, qemu_irq *pic)
>          object_initialize_child(OBJECT(s), name, &s->lpd.iou.gem[i],
>                                  TYPE_CADENCE_GEM);
>          dev = DEVICE(&s->lpd.iou.gem[i]);
> +        /* FIXME use qdev NIC properties instead of nd_table[] */
>          if (nd->used) {
>              qemu_check_nic_model(nd, "cadence_gem");
>              qdev_set_nic_properties(dev, nd);
> diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c
> index 772cfa3771..5855e5d5bf 100644
> --- a/hw/arm/xlnx-zynqmp.c
> +++ b/hw/arm/xlnx-zynqmp.c
> @@ -455,6 +455,7 @@ static void xlnx_zynqmp_realize(DeviceState *dev, Error **errp)
>      for (i = 0; i < XLNX_ZYNQMP_NUM_GEMS; i++) {
>          NICInfo *nd = &nd_table[i];
>
> +        /* FIXME use qdev NIC properties instead of nd_table[] */
>          if (nd->used) {
>              qemu_check_nic_model(nd, TYPE_CADENCE_GEM);
>              qdev_set_nic_properties(DEVICE(&s->gem[i]), nd);
> diff --git a/hw/dma/sparc32_dma.c b/hw/dma/sparc32_dma.c
> index 9459178866..bcd1626fbd 100644
> --- a/hw/dma/sparc32_dma.c
> +++ b/hw/dma/sparc32_dma.c
> @@ -341,6 +341,7 @@ static void sparc32_ledma_device_realize(DeviceState *dev, Error **errp)
>      DeviceState *d;
>      NICInfo *nd = &nd_table[0];
>
> +    /* FIXME use qdev NIC properties instead of nd_table[] */
>      qemu_check_nic_model(nd, TYPE_LANCE);
>
>      d = qdev_new(TYPE_LANCE);
> diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
> index 19a976c9a6..e5682c38a9 100644
> --- a/hw/riscv/sifive_u.c
> +++ b/hw/riscv/sifive_u.c
> @@ -714,6 +714,7 @@ static void sifive_u_soc_realize(DeviceState *dev, Error **errp)
>      }
>      sysbus_mmio_map(SYS_BUS_DEVICE(&s->otp), 0, memmap[SIFIVE_U_OTP].base);
>
> +    /* FIXME use qdev NIC properties instead of nd_table[] */
>      if (nd->used) {
>          qemu_check_nic_model(nd, TYPE_CADENCE_GEM);
>          qdev_set_nic_properties(DEVICE(&s->gem), nd);
> --
> 2.26.2
>
>

Re: [PATCH for-5.1 1/2] msf2: Unbreak device-list-properties for "msf-soc"

[Markus Armbruster]

Philippe Mathieu-Daudé <f4bug@amsat.org> writes:

> On 7/15/20 4:04 PM, Markus Armbruster wrote:
>> Watch this:
>> 
>>     $ qemu-system-aarch64 -M ast2600-evb -S -display none -qmp stdio
>>     {"QMP": {"version": {"qemu": {"micro": 50, "minor": 0, "major": 5}, "package": "v5.0.0-2464-g3a9163af4e"}, "capabilities": ["oob"]}}
>>     {"execute": "qmp_capabilities"}
>>     {"return": {}}
>>     {"execute": "device-list-properties", "arguments": {"typename": "msf2-soc"}}
>>     Unsupported NIC model: ftgmac100
>>     armbru@dusky:~/work/images$ echo $?
>>     1
>> 
>> This is what breaks "make check SPEED=slow".
>> 
>> Root cause is m2sxxx_soc_initfn()'s messing with nd_table[] via
>> qemu_check_nic_model().  That's wrong.
>> 
>> We fixed the exact same bug for device "allwinner-a10" in commit
>> 8aabc5437b "hw/arm/allwinner-a10: Do not use nd_table in instance_init
>> function".  Fix this instance the same way: move the offending code to
>> m2sxxx_soc_realize(), where it's less wrong, and add a FIXME comment.
>
> That addresses this other thread, right?
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg720658.html

Correct!  I wasn't aware of it, thanks for making the connection.

Re: [PATCH for-5.1 2/2] hw: Mark nd_table[] misuse in realize methods FIXME

[Markus Armbruster]

Thomas Huth <thuth@redhat.com> writes:

> On 15/07/2020 16.04, Markus Armbruster wrote:
>> nd_table[] contains NIC configuration for boards to pick up.  Device
>> code has no business looking there.  Several devices do it anyway.
>> Two of them already have a suitable FIXME comment: "allwinner-a10" and
>> "msf2-soc".  Copy it to the others: "allwinner-h3", "xlnx-versal",
>> "xlnx,zynqmp", "sparc32-ledma", "riscv.sifive.u.soc".
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  hw/arm/allwinner-h3.c | 1 +
>>  hw/arm/xlnx-versal.c  | 1 +
>>  hw/arm/xlnx-zynqmp.c  | 1 +
>>  hw/dma/sparc32_dma.c  | 1 +
>>  hw/riscv/sifive_u.c   | 1 +
>>  5 files changed, 5 insertions(+)
>
> Maybe we should add an entry to
> https://wiki.qemu.org/Contribute/BiteSizedTasks for this?

I'd prefer to leave that to someone who knows how this should be done
properly, and thus has a better idea on how big the bite actually is.
Figuring this out myself might be more work than fixing it.

> Reviewed-by: Thomas Huth <thuth@redhat.com>

Thanks!

Re: [PATCH for-5.1 2/2] hw: Mark nd_table[] misuse in realize methods FIXME

[Niek Linnenbank]

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

On Wed, Jul 15, 2020, 16:04 Markus Armbruster <armbru@redhat.com> wrote:

> nd_table[] contains NIC configuration for boards to pick up.  Device
> code has no business looking there.  Several devices do it anyway.
> Two of them already have a suitable FIXME comment: "allwinner-a10" and
> "msf2-soc".  Copy it to the others: "allwinner-h3", "xlnx-versal",
> "xlnx,zynqmp", "sparc32-ledma", "riscv.sifive.u.soc".
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>
Reviewed-by: Niek Linnenbank <nieklinnenbank@gmail.com>

---
>  hw/arm/allwinner-h3.c | 1 +
>  hw/arm/xlnx-versal.c  | 1 +
>  hw/arm/xlnx-zynqmp.c  | 1 +
>  hw/dma/sparc32_dma.c  | 1 +
>  hw/riscv/sifive_u.c   | 1 +
>  5 files changed, 5 insertions(+)
>
> diff --git a/hw/arm/allwinner-h3.c b/hw/arm/allwinner-h3.c
> index 8e09468e86..ff92ded82c 100644
> --- a/hw/arm/allwinner-h3.c
> +++ b/hw/arm/allwinner-h3.c
> @@ -358,6 +358,7 @@ static void allwinner_h3_realize(DeviceState *dev,
> Error **errp)
>                                "sd-bus");
>
>      /* EMAC */
> +    /* FIXME use qdev NIC properties instead of nd_table[] */
>      if (nd_table[0].used) {
>          qemu_check_nic_model(&nd_table[0], TYPE_AW_SUN8I_EMAC);
>          qdev_set_nic_properties(DEVICE(&s->emac), &nd_table[0]);
> diff --git a/hw/arm/xlnx-versal.c b/hw/arm/xlnx-versal.c
> index ead038b971..e3aa4bd1e5 100644
> --- a/hw/arm/xlnx-versal.c
> +++ b/hw/arm/xlnx-versal.c
> @@ -160,6 +160,7 @@ static void versal_create_gems(Versal *s, qemu_irq
> *pic)
>          object_initialize_child(OBJECT(s), name, &s->lpd.iou.gem[i],
>                                  TYPE_CADENCE_GEM);
>          dev = DEVICE(&s->lpd.iou.gem[i]);
> +        /* FIXME use qdev NIC properties instead of nd_table[] */
>          if (nd->used) {
>              qemu_check_nic_model(nd, "cadence_gem");
>              qdev_set_nic_properties(dev, nd);
> diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c
> index 772cfa3771..5855e5d5bf 100644
> --- a/hw/arm/xlnx-zynqmp.c
> +++ b/hw/arm/xlnx-zynqmp.c
> @@ -455,6 +455,7 @@ static void xlnx_zynqmp_realize(DeviceState *dev,
> Error **errp)
>      for (i = 0; i < XLNX_ZYNQMP_NUM_GEMS; i++) {
>          NICInfo *nd = &nd_table[i];
>
> +        /* FIXME use qdev NIC properties instead of nd_table[] */
>          if (nd->used) {
>              qemu_check_nic_model(nd, TYPE_CADENCE_GEM);
>              qdev_set_nic_properties(DEVICE(&s->gem[i]), nd);
> diff --git a/hw/dma/sparc32_dma.c b/hw/dma/sparc32_dma.c
> index 9459178866..bcd1626fbd 100644
> --- a/hw/dma/sparc32_dma.c
> +++ b/hw/dma/sparc32_dma.c
> @@ -341,6 +341,7 @@ static void sparc32_ledma_device_realize(DeviceState
> *dev, Error **errp)
>      DeviceState *d;
>      NICInfo *nd = &nd_table[0];
>
> +    /* FIXME use qdev NIC properties instead of nd_table[] */
>      qemu_check_nic_model(nd, TYPE_LANCE);
>
>      d = qdev_new(TYPE_LANCE);
> diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
> index 19a976c9a6..e5682c38a9 100644
> --- a/hw/riscv/sifive_u.c
> +++ b/hw/riscv/sifive_u.c
> @@ -714,6 +714,7 @@ static void sifive_u_soc_realize(DeviceState *dev,
> Error **errp)
>      }
>      sysbus_mmio_map(SYS_BUS_DEVICE(&s->otp), 0,
> memmap[SIFIVE_U_OTP].base);
>
> +    /* FIXME use qdev NIC properties instead of nd_table[] */
>      if (nd->used) {
>          qemu_check_nic_model(nd, TYPE_CADENCE_GEM);
>          qdev_set_nic_properties(DEVICE(&s->gem), nd);
> --
> 2.26.2
>
>

[-- Attachment #2: Type: text/html, Size: 4611 bytes --]

Re: [PATCH for-5.1 1/2] msf2: Unbreak device-list-properties for "msf-soc"

[sundeep subbaraya]

On Wed, Jul 15, 2020 at 8:12 PM Markus Armbruster <armbru@redhat.com> wrote:
>
> Philippe Mathieu-Daudé <f4bug@amsat.org> writes:
>
> > On 7/15/20 4:04 PM, Markus Armbruster wrote:
> >> Watch this:
> >>
> >>     $ qemu-system-aarch64 -M ast2600-evb -S -display none -qmp stdio
> >>     {"QMP": {"version": {"qemu": {"micro": 50, "minor": 0, "major": 5}, "package": "v5.0.0-2464-g3a9163af4e"}, "capabilities": ["oob"]}}
> >>     {"execute": "qmp_capabilities"}
> >>     {"return": {}}
> >>     {"execute": "device-list-properties", "arguments": {"typename": "msf2-soc"}}
> >>     Unsupported NIC model: ftgmac100
> >>     armbru@dusky:~/work/images$ echo $?
> >>     1
> >>
> >> This is what breaks "make check SPEED=slow".
> >>
> >> Root cause is m2sxxx_soc_initfn()'s messing with nd_table[] via
> >> qemu_check_nic_model().  That's wrong.
> >>
> >> We fixed the exact same bug for device "allwinner-a10" in commit
> >> 8aabc5437b "hw/arm/allwinner-a10: Do not use nd_table in instance_init
> >> function".  Fix this instance the same way: move the offending code to
> >> m2sxxx_soc_realize(), where it's less wrong, and add a FIXME comment.
> >
> > That addresses this other thread, right?
> > https://www.mail-archive.com/qemu-devel@nongnu.org/msg720658.html
>
> Correct!  I wasn't aware of it, thanks for making the connection.
>

Thanks Markus for the fix.
Sundeep

Re: [PATCH for-5.1 1/2] msf2: Unbreak device-list-properties for "msf-soc"

[Thomas Huth]

On 16/07/2020 04.59, sundeep subbaraya wrote:
> On Wed, Jul 15, 2020 at 8:12 PM Markus Armbruster <armbru@redhat.com> wrote:
>>
>> Philippe Mathieu-Daudé <f4bug@amsat.org> writes:
>>
>>> On 7/15/20 4:04 PM, Markus Armbruster wrote:
>>>> Watch this:
>>>>
>>>>     $ qemu-system-aarch64 -M ast2600-evb -S -display none -qmp stdio
>>>>     {"QMP": {"version": {"qemu": {"micro": 50, "minor": 0, "major": 5}, "package": "v5.0.0-2464-g3a9163af4e"}, "capabilities": ["oob"]}}
>>>>     {"execute": "qmp_capabilities"}
>>>>     {"return": {}}
>>>>     {"execute": "device-list-properties", "arguments": {"typename": "msf2-soc"}}
>>>>     Unsupported NIC model: ftgmac100
>>>>     armbru@dusky:~/work/images$ echo $?
>>>>     1
>>>>
>>>> This is what breaks "make check SPEED=slow".
>>>>
>>>> Root cause is m2sxxx_soc_initfn()'s messing with nd_table[] via
>>>> qemu_check_nic_model().  That's wrong.
>>>>
>>>> We fixed the exact same bug for device "allwinner-a10" in commit
>>>> 8aabc5437b "hw/arm/allwinner-a10: Do not use nd_table in instance_init
>>>> function".  Fix this instance the same way: move the offending code to
>>>> m2sxxx_soc_realize(), where it's less wrong, and add a FIXME comment.
>>>
>>> That addresses this other thread, right?
>>> https://www.mail-archive.com/qemu-devel@nongnu.org/msg720658.html
>>
>> Correct!  I wasn't aware of it, thanks for making the connection.
>>
> 
> Thanks Markus for the fix.

It's rather just a work-around that avoids the crash than a real fix. I
think we can use it for the upcoming 5.1 release, but it would be great
if you could rework this code for 5.2, so that the nd_table handling is
moved to msf2-som.c instead.

 Thanks,
  Thomas

Re: [PATCH for-5.1 1/2] msf2: Unbreak device-list-properties for "msf-soc"

[Philippe Mathieu-Daudé]

On 7/16/20 8:07 AM, Thomas Huth wrote:
> On 16/07/2020 04.59, sundeep subbaraya wrote:
>> On Wed, Jul 15, 2020 at 8:12 PM Markus Armbruster <armbru@redhat.com> wrote:
>>>
>>> Philippe Mathieu-Daudé <f4bug@amsat.org> writes:
>>>
>>>> On 7/15/20 4:04 PM, Markus Armbruster wrote:
>>>>> Watch this:
>>>>>
>>>>>     $ qemu-system-aarch64 -M ast2600-evb -S -display none -qmp stdio
>>>>>     {"QMP": {"version": {"qemu": {"micro": 50, "minor": 0, "major": 5}, "package": "v5.0.0-2464-g3a9163af4e"}, "capabilities": ["oob"]}}
>>>>>     {"execute": "qmp_capabilities"}
>>>>>     {"return": {}}
>>>>>     {"execute": "device-list-properties", "arguments": {"typename": "msf2-soc"}}
>>>>>     Unsupported NIC model: ftgmac100
>>>>>     armbru@dusky:~/work/images$ echo $?
>>>>>     1
>>>>>
>>>>> This is what breaks "make check SPEED=slow".
>>>>>
>>>>> Root cause is m2sxxx_soc_initfn()'s messing with nd_table[] via
>>>>> qemu_check_nic_model().  That's wrong.
>>>>>
>>>>> We fixed the exact same bug for device "allwinner-a10" in commit
>>>>> 8aabc5437b "hw/arm/allwinner-a10: Do not use nd_table in instance_init
>>>>> function".  Fix this instance the same way: move the offending code to
>>>>> m2sxxx_soc_realize(), where it's less wrong, and add a FIXME comment.
>>>>
>>>> That addresses this other thread, right?
>>>> https://www.mail-archive.com/qemu-devel@nongnu.org/msg720658.html
>>>
>>> Correct!  I wasn't aware of it, thanks for making the connection.
>>>
>>
>> Thanks Markus for the fix.
> 
> It's rather just a work-around that avoids the crash than a real fix. I
> think we can use it for the upcoming 5.1 release, but it would be great
> if you could rework this code for 5.2, so that the nd_table handling is
> moved to msf2-som.c instead.

Yes this is not a 'fix' but rather a kludge.
Thomas gave some tips to work on a fix here:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg720658.html

Thanks,

Phil.

Re: [PATCH for-5.1 1/2] msf2: Unbreak device-list-properties for "msf-soc"

[sundeep subbaraya]

Yep I will rework on this soon.

Thanks guys,
Sundeep

On Thu, Jul 16, 2020 at 1:06 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> On 7/16/20 8:07 AM, Thomas Huth wrote:
> > On 16/07/2020 04.59, sundeep subbaraya wrote:
> >> On Wed, Jul 15, 2020 at 8:12 PM Markus Armbruster <armbru@redhat.com> wrote:
> >>>
> >>> Philippe Mathieu-Daudé <f4bug@amsat.org> writes:
> >>>
> >>>> On 7/15/20 4:04 PM, Markus Armbruster wrote:
> >>>>> Watch this:
> >>>>>
> >>>>>     $ qemu-system-aarch64 -M ast2600-evb -S -display none -qmp stdio
> >>>>>     {"QMP": {"version": {"qemu": {"micro": 50, "minor": 0, "major": 5}, "package": "v5.0.0-2464-g3a9163af4e"}, "capabilities": ["oob"]}}
> >>>>>     {"execute": "qmp_capabilities"}
> >>>>>     {"return": {}}
> >>>>>     {"execute": "device-list-properties", "arguments": {"typename": "msf2-soc"}}
> >>>>>     Unsupported NIC model: ftgmac100
> >>>>>     armbru@dusky:~/work/images$ echo $?
> >>>>>     1
> >>>>>
> >>>>> This is what breaks "make check SPEED=slow".
> >>>>>
> >>>>> Root cause is m2sxxx_soc_initfn()'s messing with nd_table[] via
> >>>>> qemu_check_nic_model().  That's wrong.
> >>>>>
> >>>>> We fixed the exact same bug for device "allwinner-a10" in commit
> >>>>> 8aabc5437b "hw/arm/allwinner-a10: Do not use nd_table in instance_init
> >>>>> function".  Fix this instance the same way: move the offending code to
> >>>>> m2sxxx_soc_realize(), where it's less wrong, and add a FIXME comment.
> >>>>
> >>>> That addresses this other thread, right?
> >>>> https://www.mail-archive.com/qemu-devel@nongnu.org/msg720658.html
> >>>
> >>> Correct!  I wasn't aware of it, thanks for making the connection.
> >>>
> >>
> >> Thanks Markus for the fix.
> >
> > It's rather just a work-around that avoids the crash than a real fix. I
> > think we can use it for the upcoming 5.1 release, but it would be great
> > if you could rework this code for 5.2, so that the nd_table handling is
> > moved to msf2-som.c instead.
>
> Yes this is not a 'fix' but rather a kludge.
> Thomas gave some tips to work on a fix here:
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg720658.html
>
> Thanks,
>
> Phil.