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

[Peter Maydell <peter.maydell@linaro.org>]

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