[PATCH 0/2] Fix issues with npcm7xx and recent NIC cleanup

[PATCH 0/2] Fix issues with npcm7xx and recent NIC cleanup

[Peter Maydell]

The changes to the NPCM7xx to add the GMAC devices crossed in the mail
with the NIC cleanup patchset, which had a couple of unfortunate results:
 * no qemu_configure_nic_device() call for the GMAC modules
 * the npcm7xx_emc-test produces some spurious warnings

This patchset fixes both of those. Arguably patch 2 shouldn't
be necessary, as the "nic foo has no peer" warning is a bit
overzealous for NICs that always exist on the board, but I preferred
the simple approach of silencing it rather than spending ages trying
to figure out what the "right" condition for the warning should be.

thanks
-- PMM

Peter Maydell (2):
  hw/arm/npcm7xx: Call qemu_configure_nic_device() for GMAC modules
  tests/qtest/npcm7xx_emc-test: Connect all NICs to a backend

 hw/arm/npcm7xx.c               | 1 +
 tests/qtest/npcm7xx_emc-test.c | 5 ++++-
 2 files changed, 5 insertions(+), 1 deletion(-)

-- 
2.34.1

[PATCH 1/2] hw/arm/npcm7xx: Call qemu_configure_nic_device() for GMAC modules

[Peter Maydell]

The patchset adding the GMAC ethernet to this SoC crossed in the
mail with the patchset cleaning up the NIC handling. When we
create the GMAC modules we must call qemu_configure_nic_device()
so that the user has the opportunity to use the -nic commandline
option to create a network backend and connect it to the GMACs.

Add the missing call.

Fixes: 21e5326a7c ("hw/arm: Add GMAC devices to NPCM7XX SoC")
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm/npcm7xx.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/arm/npcm7xx.c b/hw/arm/npcm7xx.c
index ff3ecde9043..cc68b5d8f12 100644
--- a/hw/arm/npcm7xx.c
+++ b/hw/arm/npcm7xx.c
@@ -710,6 +710,7 @@ static void npcm7xx_realize(DeviceState *dev, Error **errp)
     for (i = 0; i < ARRAY_SIZE(s->gmac); i++) {
         SysBusDevice *sbd = SYS_BUS_DEVICE(&s->gmac[i]);
 
+        qemu_configure_nic_device(DEVICE(sbd), false, NULL);
         /*
          * The device exists regardless of whether it's connected to a QEMU
          * netdev backend. So always instantiate it even if there is no
-- 
2.34.1

Re: [PATCH 1/2] hw/arm/npcm7xx: Call qemu_configure_nic_device() for GMAC modules

[David Woodhouse]

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

On Tue, 2024-02-06 at 17:12 +0000, Peter Maydell wrote:
> The patchset adding the GMAC ethernet to this SoC crossed in the
> mail with the patchset cleaning up the NIC handling. When we
> create the GMAC modules we must call qemu_configure_nic_device()
> so that the user has the opportunity to use the -nic commandline
> option to create a network backend and connect it to the GMACs.
> 
> Add the missing call.
> 
> Fixes: 21e5326a7c ("hw/arm: Add GMAC devices to NPCM7XX SoC")
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: David Woodhouse <dwmw@amazon.co.uk>

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5965 bytes --]

[PATCH 2/2] tests/qtest/npcm7xx_emc-test: Connect all NICs to a backend

[Peter Maydell]

Currently QEMU will warn if there is a NIC on the board that
is not connected to a backend. By default the '-nic user' will
get used for all NICs, but if you manually connect a specific
NIC to a specific backend, then the other NICs on the board
have no backend and will be warned about:

qemu-system-arm: warning: nic npcm7xx-emc.1 has no peer
qemu-system-arm: warning: nic npcm-gmac.0 has no peer
qemu-system-arm: warning: nic npcm-gmac.1 has no peer

So suppress those warnings by manually connecting every NIC
on the board to some backend.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 tests/qtest/npcm7xx_emc-test.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/tests/qtest/npcm7xx_emc-test.c b/tests/qtest/npcm7xx_emc-test.c
index f7646fae2c9..63f6cadb5cc 100644
--- a/tests/qtest/npcm7xx_emc-test.c
+++ b/tests/qtest/npcm7xx_emc-test.c
@@ -228,7 +228,10 @@ static int *packet_test_init(int module_num, GString *cmd_line)
      * KISS and use -nic. The driver accepts 'emc0' and 'emc1' as aliases
      * in the 'model' field to specify the device to match.
      */
-    g_string_append_printf(cmd_line, " -nic socket,fd=%d,model=emc%d ",
+    g_string_append_printf(cmd_line, " -nic socket,fd=%d,model=emc%d "
+                           "-nic user,model=npcm7xx-emc "
+                           "-nic user,model=npcm-gmac "
+                           "-nic user,model=npcm-gmac",
                            test_sockets[1], module_num);
 
     g_test_queue_destroy(packet_test_clear, test_sockets);
-- 
2.34.1

Re: [PATCH 2/2] tests/qtest/npcm7xx_emc-test: Connect all NICs to a backend

[Peter Maydell]

On Tue, 6 Feb 2024 at 17:12, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> Currently QEMU will warn if there is a NIC on the board that
> is not connected to a backend. By default the '-nic user' will
> get used for all NICs, but if you manually connect a specific
> NIC to a specific backend, then the other NICs on the board
> have no backend and will be warned about:
>
> qemu-system-arm: warning: nic npcm7xx-emc.1 has no peer
> qemu-system-arm: warning: nic npcm-gmac.0 has no peer
> qemu-system-arm: warning: nic npcm-gmac.1 has no peer
>
> So suppress those warnings by manually connecting every NIC
> on the board to some backend.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  tests/qtest/npcm7xx_emc-test.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/tests/qtest/npcm7xx_emc-test.c b/tests/qtest/npcm7xx_emc-test.c
> index f7646fae2c9..63f6cadb5cc 100644
> --- a/tests/qtest/npcm7xx_emc-test.c
> +++ b/tests/qtest/npcm7xx_emc-test.c
> @@ -228,7 +228,10 @@ static int *packet_test_init(int module_num, GString *cmd_line)
>       * KISS and use -nic. The driver accepts 'emc0' and 'emc1' as aliases
>       * in the 'model' field to specify the device to match.
>       */

Whoops, made the classic "create patch without having saved in
the editor" mistake. I meant to also include this change to
the comment:

     /*
      * KISS and use -nic. The driver accepts 'emc0' and 'emc1' as aliases
      * in the 'model' field to specify the device to match.
+     * We wire up the other NICs on the board to the 'user' backend
+     * purely to suppress the "warning: nic npcm7xx-emc.1 has no peer"
+     * etc warnings that otherwise are currently produced.
      */


> -    g_string_append_printf(cmd_line, " -nic socket,fd=%d,model=emc%d ",
> +    g_string_append_printf(cmd_line, " -nic socket,fd=%d,model=emc%d "
> +                           "-nic user,model=npcm7xx-emc "
> +                           "-nic user,model=npcm-gmac "
> +                           "-nic user,model=npcm-gmac",
>                             test_sockets[1], module_num);
>
>      g_test_queue_destroy(packet_test_clear, test_sockets);
> --
> 2.34.1
>

thanks
-- PMM

Re: [PATCH 2/2] tests/qtest/npcm7xx_emc-test: Connect all NICs to a backend

[David Woodhouse]

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

On Tue, 2024-02-06 at 17:12 +0000, Peter Maydell wrote:
> Currently QEMU will warn if there is a NIC on the board that
> is not connected to a backend. By default the '-nic user' will
> get used for all NICs, but if you manually connect a specific
> NIC to a specific backend, then the other NICs on the board
> have no backend and will be warned about:
> 
> qemu-system-arm: warning: nic npcm7xx-emc.1 has no peer
> qemu-system-arm: warning: nic npcm-gmac.0 has no peer
> qemu-system-arm: warning: nic npcm-gmac.1 has no peer
> 
> So suppress those warnings by manually connecting every NIC
> on the board to some backend.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: David Woodhouse <dwmw@amazon.co.uk>


... although do we want to expand this test to cover the GMACs? 

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5965 bytes --]

Re: [PATCH 2/2] tests/qtest/npcm7xx_emc-test: Connect all NICs to a backend

[Peter Maydell]

On Tue, 6 Feb 2024 at 17:36, David Woodhouse <dwmw2@infradead.org> wrote:
>
> On Tue, 2024-02-06 at 17:12 +0000, Peter Maydell wrote:
> > Currently QEMU will warn if there is a NIC on the board that
> > is not connected to a backend. By default the '-nic user' will
> > get used for all NICs, but if you manually connect a specific
> > NIC to a specific backend, then the other NICs on the board
> > have no backend and will be warned about:
> >
> > qemu-system-arm: warning: nic npcm7xx-emc.1 has no peer
> > qemu-system-arm: warning: nic npcm-gmac.0 has no peer
> > qemu-system-arm: warning: nic npcm-gmac.1 has no peer
> >
> > So suppress those warnings by manually connecting every NIC
> > on the board to some backend.
> >
> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>
> Reviewed-by: David Woodhouse <dwmw@amazon.co.uk>
>
>
> ... although do we want to expand this test to cover the GMACs?

It's very specific to details of how to program the particular
ethernet controller. The GMAC is supposed to be covered by
npcm_gmac-test.c (but that has issues of its own, see
https://lore.kernel.org/qemu-devel/CAFEAcA_gkQz7q+PhiqrVd+YrVJvLt1H=Ypp4av9qn+6mYC6jdA@mail.gmail.com/
and in any case doesn't yet try to actually exercise the device
beyond a rather minimal "check the register reset values"
test that doesn't need any particular backend connected).

thanks
-- PMM

Re: [PATCH 2/2] tests/qtest/npcm7xx_emc-test: Connect all NICs to a backend

[Thomas Huth]

On 06/02/2024 18.12, Peter Maydell wrote:
> Currently QEMU will warn if there is a NIC on the board that
> is not connected to a backend. By default the '-nic user' will
> get used for all NICs, but if you manually connect a specific
> NIC to a specific backend, then the other NICs on the board
> have no backend and will be warned about:
> 
> qemu-system-arm: warning: nic npcm7xx-emc.1 has no peer
> qemu-system-arm: warning: nic npcm-gmac.0 has no peer
> qemu-system-arm: warning: nic npcm-gmac.1 has no peer
> 
> So suppress those warnings by manually connecting every NIC
> on the board to some backend.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   tests/qtest/npcm7xx_emc-test.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/qtest/npcm7xx_emc-test.c b/tests/qtest/npcm7xx_emc-test.c
> index f7646fae2c9..63f6cadb5cc 100644
> --- a/tests/qtest/npcm7xx_emc-test.c
> +++ b/tests/qtest/npcm7xx_emc-test.c
> @@ -228,7 +228,10 @@ static int *packet_test_init(int module_num, GString *cmd_line)
>        * KISS and use -nic. The driver accepts 'emc0' and 'emc1' as aliases
>        * in the 'model' field to specify the device to match.
>        */
> -    g_string_append_printf(cmd_line, " -nic socket,fd=%d,model=emc%d ",
> +    g_string_append_printf(cmd_line, " -nic socket,fd=%d,model=emc%d "
> +                           "-nic user,model=npcm7xx-emc "
> +                           "-nic user,model=npcm-gmac "
> +                           "-nic user,model=npcm-gmac",

Alternatively, use -nic hubport,hubid=0 in case we even want to run this 
test without slirp support, too (but currently there is already a check for 
this in the meson.build file, so -nic user should be fine, too). Anyway,

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