1. Patchew
  2. QEMU
  3. [PATCH v4 00/47] Rework matching of network devices to -nic options
  4. View patch

[PATCH v4 29/47] hw/arm/stellaris: use qemu_find_nic_info()

David Woodhouse posted 47 patches 10 months ago
Maintainers: Richard Henderson <richard.henderson@linaro.org>, Beniamino Galvani <b.galvani@gmail.com>, Peter Maydell <peter.maydell@linaro.org>, Strahinja Jankovic <strahinja.p.jankovic@gmail.com>, Niek Linnenbank <nieklinnenbank@gmail.com>, "Cédric Le Goater" <clg@kaod.org>, Andrew Jeffery <andrew@codeconstruct.com.au>, Joel Stanley <joel@jms.id.au>, Igor Mitsyanko <i.mitsyanko@gmail.com>, Jean-Christophe Dubois <jcd@tribudubois.net>, Andrey Smirnov <andrew.smirnov@gmail.com>, "Philippe Mathieu-Daudé" <philmd@linaro.org>, Rob Herring <robh@kernel.org>, Subbaraya Sundeep <sundeep.lkml@gmail.com>, Jan Kiszka <jan.kiszka@web.de>, Tyrone Ting <kfting@nuvoton.com>, Hao Wu <wuhaotsh@google.com>, Radoslaw Biernacki <rad@semihalf.com>, Leif Lindholm <quic_llindhol@quicinc.com>, Marcin Juszkiewicz <marcin.juszkiewicz@linaro.org>, "Edgar E. Iglesias" <edgar.iglesias@gmail.com>, Alistair Francis <alistair@alistair23.me>, Helge Deller <deller@gmx.de>, "Michael S. Tsirkin" <mst@redhat.com>, Marcel Apfelbaum <marcel.apfelbaum@gmail.com>, Paolo Bonzini <pbonzini@redhat.com>, Eduardo Habkost <eduardo@habkost.net>, Song Gao <gaosong@loongson.cn>, Thomas Huth <huth@tuxfamily.org>, Laurent Vivier <laurent@vivier.eu>, Huacai Chen <chenhuacai@kernel.org>, Jiaxun Yang <jiaxun.yang@flygoat.com>, "Hervé Poussineau" <hpoussin@reactos.org>, Aleksandar Rikalo <aleksandar.rikalo@syrmia.com>, Aurelien Jarno <aurelien@aurel32.net>, Jason Wang <jasowang@redhat.com>, Jia Liu <proljc@gmail.com>, Stafford Horne <shorne@gmail.com>, Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>, Nicholas Piggin <npiggin@gmail.com>, Daniel Henrique Barboza <danielhb413@gmail.com>, David Gibson <david@gibson.dropbear.id.au>, Harsh Prateek Bora <harshpb@linux.ibm.com>, Bin Meng <bin.meng@windriver.com>, Palmer Dabbelt <palmer@dabbelt.com>, Weiwei Li <liwei1518@gmail.com>, Liu Zhiwei <zhiwei_liu@linux.alibaba.com>, David Hildenbrand <david@redhat.com>, Ilya Leoshkevich <iii@linux.ibm.com>, Halil Pasic <pasic@linux.ibm.com>, Christian Borntraeger <borntraeger@linux.ibm.com>, Eric Farman <farman@linux.ibm.com>, Yoshinori Sato <ysato@users.sourceforge.jp>, Magnus Damm <magnus.damm@gmail.com>, Artyom Tarasenko <atar4qemu@gmail.com>, Stefano Stabellini <sstabellini@kernel.org>, Anthony Perard <anthony.perard@citrix.com>, Paul Durrant <paul@xen.org>, Max Filippov <jcmvbkbc@gmail.com>
[PATCH v4 29/47] hw/arm/stellaris: use qemu_find_nic_info()
Posted by David Woodhouse 10 months ago 
From: David Woodhouse <dwmw@amazon.co.uk>

Rather than just using qemu_configure_nic_device(), populate the MAC
address in the system-registers device by peeking at the NICInfo before
it's assigned to the device.

Generate the MAC address early, if there is no matching -nic option.
Otherwise the MAC address wouldn't be generated until net_client_init1()
runs.

Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
---
 hw/arm/stellaris.c | 30 ++++++++++++++++++++++--------
 1 file changed, 22 insertions(+), 8 deletions(-)

diff --git a/hw/arm/stellaris.c b/hw/arm/stellaris.c
index d18b1144af..34c5a86ac2 100644
--- a/hw/arm/stellaris.c
+++ b/hw/arm/stellaris.c
@@ -1028,7 +1028,8 @@ static void stellaris_init(MachineState *ms, stellaris_board_info *board)
     DeviceState *ssys_dev;
     int i;
     int j;
-    const uint8_t *macaddr;
+    NICInfo *nd;
+    MACAddr mac;
 
     MemoryRegion *sram = g_new(MemoryRegion, 1);
     MemoryRegion *flash = g_new(MemoryRegion, 1);
@@ -1051,12 +1052,22 @@ static void stellaris_init(MachineState *ms, stellaris_board_info *board)
      * need its sysclk output.
      */
     ssys_dev = qdev_new(TYPE_STELLARIS_SYS);
-    /* Most devices come preprogrammed with a MAC address in the user data. */
-    macaddr = nd_table[0].macaddr.a;
+
+    /*
+     * Most devices come preprogrammed with a MAC address in the user data.
+     * Generate a MAC address now, if there isn't a matching -nic for it.
+     */
+    nd = qemu_find_nic_info("stellaris_enet", true, "stellaris");
+    if (nd) {
+        memcpy(mac.a, nd->macaddr.a, sizeof(mac.a));
+    } else {
+        qemu_macaddr_default_if_unset(&mac);
+    }
+
     qdev_prop_set_uint32(ssys_dev, "user0",
-                         macaddr[0] | (macaddr[1] << 8) | (macaddr[2] << 16));
+                         mac.a[0] | (mac.a[1] << 8) | (mac.a[2] << 16));
     qdev_prop_set_uint32(ssys_dev, "user1",
-                         macaddr[3] | (macaddr[4] << 8) | (macaddr[5] << 16));
+                         mac.a[3] | (mac.a[4] << 8) | (mac.a[5] << 16));
     qdev_prop_set_uint32(ssys_dev, "did0", board->did0);
     qdev_prop_set_uint32(ssys_dev, "did1", board->did1);
     qdev_prop_set_uint32(ssys_dev, "dc0", board->dc0);
@@ -1269,10 +1280,13 @@ static void stellaris_init(MachineState *ms, stellaris_board_info *board)
     if (board->dc4 & (1 << 28)) {
         DeviceState *enet;
 
-        qemu_check_nic_model(&nd_table[0], "stellaris");
-
         enet = qdev_new("stellaris_enet");
-        qdev_set_nic_properties(enet, &nd_table[0]);
+        if (nd) {
+            qdev_set_nic_properties(enet, nd);
+        } else {
+            qdev_prop_set_macaddr(enet, "mac", mac.a);
+        }
+
         sysbus_realize_and_unref(SYS_BUS_DEVICE(enet), &error_fatal);
         sysbus_mmio_map(SYS_BUS_DEVICE(enet), 0, 0x40048000);
         sysbus_connect_irq(SYS_BUS_DEVICE(enet), 0, qdev_get_gpio_in(nvic, 42));
-- 
2.43.0
Re: [PATCH v4 29/47] hw/arm/stellaris: use qemu_find_nic_info()
Posted by Thomas Huth 10 months ago 
On 26/01/2024 18.25, David Woodhouse wrote:
> From: David Woodhouse <dwmw@amazon.co.uk>
> 
> Rather than just using qemu_configure_nic_device(), populate the MAC
> address in the system-registers device by peeking at the NICInfo before
> it's assigned to the device.
> 
> Generate the MAC address early, if there is no matching -nic option.
> Otherwise the MAC address wouldn't be generated until net_client_init1()
> runs.
> 
> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> ---
>   hw/arm/stellaris.c | 30 ++++++++++++++++++++++--------
>   1 file changed, 22 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/arm/stellaris.c b/hw/arm/stellaris.c
> index d18b1144af..34c5a86ac2 100644
> --- a/hw/arm/stellaris.c
> +++ b/hw/arm/stellaris.c
> @@ -1028,7 +1028,8 @@ static void stellaris_init(MachineState *ms, stellaris_board_info *board)
>       DeviceState *ssys_dev;
>       int i;
>       int j;
> -    const uint8_t *macaddr;
> +    NICInfo *nd;
> +    MACAddr mac;
>   
>       MemoryRegion *sram = g_new(MemoryRegion, 1);
>       MemoryRegion *flash = g_new(MemoryRegion, 1);
> @@ -1051,12 +1052,22 @@ static void stellaris_init(MachineState *ms, stellaris_board_info *board)
>        * need its sysclk output.
>        */
>       ssys_dev = qdev_new(TYPE_STELLARIS_SYS);
> -    /* Most devices come preprogrammed with a MAC address in the user data. */
> -    macaddr = nd_table[0].macaddr.a;
> +
> +    /*
> +     * Most devices come preprogrammed with a MAC address in the user data.
> +     * Generate a MAC address now, if there isn't a matching -nic for it.
> +     */
> +    nd = qemu_find_nic_info("stellaris_enet", true, "stellaris");
> +    if (nd) {
> +        memcpy(mac.a, nd->macaddr.a, sizeof(mac.a));
> +    } else {
> +        qemu_macaddr_default_if_unset(&mac);
> +    }
> +
>       qdev_prop_set_uint32(ssys_dev, "user0",
> -                         macaddr[0] | (macaddr[1] << 8) | (macaddr[2] << 16));
> +                         mac.a[0] | (mac.a[1] << 8) | (mac.a[2] << 16));
>       qdev_prop_set_uint32(ssys_dev, "user1",
> -                         macaddr[3] | (macaddr[4] << 8) | (macaddr[5] << 16));
> +                         mac.a[3] | (mac.a[4] << 8) | (mac.a[5] << 16));

Out of scope of your patch, but I wonder why we didn't use 
qdev_prop_set_macaddr() with an according MAC address property for this 
device...?

>       qdev_prop_set_uint32(ssys_dev, "did0", board->did0);
>       qdev_prop_set_uint32(ssys_dev, "did1", board->did1);
>       qdev_prop_set_uint32(ssys_dev, "dc0", board->dc0);
> @@ -1269,10 +1280,13 @@ static void stellaris_init(MachineState *ms, stellaris_board_info *board)
>       if (board->dc4 & (1 << 28)) {
>           DeviceState *enet;
>   
> -        qemu_check_nic_model(&nd_table[0], "stellaris");
> -
>           enet = qdev_new("stellaris_enet");
> -        qdev_set_nic_properties(enet, &nd_table[0]);
> +        if (nd) {
> +            qdev_set_nic_properties(enet, nd);
> +        } else {
> +            qdev_prop_set_macaddr(enet, "mac", mac.a);
> +        }
> +
>           sysbus_realize_and_unref(SYS_BUS_DEVICE(enet), &error_fatal);
>           sysbus_mmio_map(SYS_BUS_DEVICE(enet), 0, 0x40048000);
>           sysbus_connect_irq(SYS_BUS_DEVICE(enet), 0, qdev_get_gpio_in(nvic, 42));

Reviewed-by: Thomas Huth <thuth@redhat.com>
Re: [PATCH v4 29/47] hw/arm/stellaris: use qemu_find_nic_info()
Posted by Peter Maydell 10 months ago 
On Wed, 31 Jan 2024 at 12:14, Thomas Huth <thuth@redhat.com> wrote:
>
> On 26/01/2024 18.25, David Woodhouse wrote:
> > From: David Woodhouse <dwmw@amazon.co.uk>
> >
> > Rather than just using qemu_configure_nic_device(), populate the MAC
> > address in the system-registers device by peeking at the NICInfo before
> > it's assigned to the device.
> >
> > Generate the MAC address early, if there is no matching -nic option.
> > Otherwise the MAC address wouldn't be generated until net_client_init1()
> > runs.
> >
> > Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> > ---
> >   hw/arm/stellaris.c | 30 ++++++++++++++++++++++--------
> >   1 file changed, 22 insertions(+), 8 deletions(-)
> >
> > diff --git a/hw/arm/stellaris.c b/hw/arm/stellaris.c
> > index d18b1144af..34c5a86ac2 100644
> > --- a/hw/arm/stellaris.c
> > +++ b/hw/arm/stellaris.c
> > @@ -1028,7 +1028,8 @@ static void stellaris_init(MachineState *ms, stellaris_board_info *board)
> >       DeviceState *ssys_dev;
> >       int i;
> >       int j;
> > -    const uint8_t *macaddr;
> > +    NICInfo *nd;
> > +    MACAddr mac;
> >
> >       MemoryRegion *sram = g_new(MemoryRegion, 1);
> >       MemoryRegion *flash = g_new(MemoryRegion, 1);
> > @@ -1051,12 +1052,22 @@ static void stellaris_init(MachineState *ms, stellaris_board_info *board)
> >        * need its sysclk output.
> >        */
> >       ssys_dev = qdev_new(TYPE_STELLARIS_SYS);
> > -    /* Most devices come preprogrammed with a MAC address in the user data. */
> > -    macaddr = nd_table[0].macaddr.a;
> > +
> > +    /*
> > +     * Most devices come preprogrammed with a MAC address in the user data.
> > +     * Generate a MAC address now, if there isn't a matching -nic for it.
> > +     */
> > +    nd = qemu_find_nic_info("stellaris_enet", true, "stellaris");
> > +    if (nd) {
> > +        memcpy(mac.a, nd->macaddr.a, sizeof(mac.a));
> > +    } else {
> > +        qemu_macaddr_default_if_unset(&mac);
> > +    }
> > +
> >       qdev_prop_set_uint32(ssys_dev, "user0",
> > -                         macaddr[0] | (macaddr[1] << 8) | (macaddr[2] << 16));
> > +                         mac.a[0] | (mac.a[1] << 8) | (mac.a[2] << 16));
> >       qdev_prop_set_uint32(ssys_dev, "user1",
> > -                         macaddr[3] | (macaddr[4] << 8) | (macaddr[5] << 16));
> > +                         mac.a[3] | (mac.a[4] << 8) | (mac.a[5] << 16));
>
> Out of scope of your patch, but I wonder why we didn't use
> qdev_prop_set_macaddr() with an according MAC address property for this
> device...?

Partly because this code originates from 2007 and
qdev_prop_set_macaddr() only arrived in 2009. When I did
a basic qdev conversion in 2021 (commit 4bebb9ad4e4) I
just did a simple change from "directly set fields in the
device state struct" to "set fields in the device state
struct via some qdev properties".

But also because the device we're setting these fields on isn't
an ethernet device -- it's a "system control" device with a bunch
of registers, including two which have no effect on the hardware
behaviour but which by convention usually have the MAC address in
them. So as an interface to the system control device it does make
some sense to have it be "what are the values of these two 'user'
registers" ?

(qdev_prop_set_macaddr() and the associated mac address property
seem a bit odd -- qdev_prop_set_macaddr() is called from exactly
one place, and it takes an array of bytes, marshalls them into
an ASCII string representation, sets the property, and then the
property setter parses them back out of ASCII and into an array
of bytes again...)

thanks
-- PMM
Re: [PATCH v4 29/47] hw/arm/stellaris: use qemu_find_nic_info()
Posted by David Woodhouse 10 months ago 
On Wed, 2024-01-31 at 13:13 +0100, Thomas Huth wrote:
> 
> >        qdev_prop_set_uint32(ssys_dev, "user0",
> > -                         macaddr[0] | (macaddr[1] << 8) | (macaddr[2] << 16));
> > +                         mac.a[0] | (mac.a[1] << 8) | (mac.a[2] << 16));
> >        qdev_prop_set_uint32(ssys_dev, "user1",
> > -                         macaddr[3] | (macaddr[4] << 8) | (macaddr[5] << 16));
> > +                         mac.a[3] | (mac.a[4] << 8) | (mac.a[5] << 16));
> 
> Out of scope of your patch, but I wonder why we didn't use 
> qdev_prop_set_macaddr() with an according MAC address property for this 
> device...?

Yeah. I suppose it could have done. But strictly speaking, it *isn't* a
MAC address on the underlying PROM device; it's just two 32-bit
registers. Which each happen to contain 24 bits of the MAC address.

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.