[PATCH] hw/usb/network: Remove hardcoded 0x40 prefix

[PATCH] hw/usb/network: Remove hardcoded 0x40 prefix

[Stéphane Graber via]

USB NICs have a "40:" prefix hardcoded for all MAC addresses.

This overrides user-provided configuration and leads to an inconsistent
experience.

I couldn't find any documented reason (comment or git commits) for this
behavior. It seems like everyone is just expecting the MAC address to be
fully passed through to the guest, but it isn't.

This is also particularly problematic as the "40:" prefix isn't a
reserved prefix for MAC addresses (IEEE OUI). There are a number of
valid allocations out there which use this prefix, meaning that QEMU may
be causing MAC address conflicts.

Signed-off-by: Stéphane Graber <stgraber@stgraber.org>
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2951
---
 hw/usb/dev-network.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/usb/dev-network.c b/hw/usb/dev-network.c
index 81cc09dcac..1df2454181 100644
--- a/hw/usb/dev-network.c
+++ b/hw/usb/dev-network.c
@@ -1383,7 +1383,7 @@ static void usb_net_realize(USBDevice *dev, Error **errp)
     qemu_format_nic_info_str(qemu_get_queue(s->nic), s->conf.macaddr.a);
     snprintf(s->usbstring_mac, sizeof(s->usbstring_mac),
              "%02x%02x%02x%02x%02x%02x",
-             0x40,
+             s->conf.macaddr.a[0],
              s->conf.macaddr.a[1],
              s->conf.macaddr.a[2],
              s->conf.macaddr.a[3],
-- 
2.47.2

Re: [PATCH] hw/usb/network: Remove hardcoded 0x40 prefix

[Daniel P. Berrangé]

On Tue, May 06, 2025 at 12:45:53PM -0400, Stéphane Graber via wrote:
> USB NICs have a "40:" prefix hardcoded for all MAC addresses.
> 
> This overrides user-provided configuration and leads to an inconsistent
> experience.
> 
> I couldn't find any documented reason (comment or git commits) for this
> behavior. It seems like everyone is just expecting the MAC address to be
> fully passed through to the guest, but it isn't.
> 
> This is also particularly problematic as the "40:" prefix isn't a
> reserved prefix for MAC addresses (IEEE OUI). There are a number of
> valid allocations out there which use this prefix, meaning that QEMU may
> be causing MAC address conflicts.
> 
> Signed-off-by: Stéphane Graber <stgraber@stgraber.org>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2951
> ---
>  hw/usb/dev-network.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/usb/dev-network.c b/hw/usb/dev-network.c
> index 81cc09dcac..1df2454181 100644
> --- a/hw/usb/dev-network.c
> +++ b/hw/usb/dev-network.c
> @@ -1383,7 +1383,7 @@ static void usb_net_realize(USBDevice *dev, Error **errp)
>      qemu_format_nic_info_str(qemu_get_queue(s->nic), s->conf.macaddr.a);
>      snprintf(s->usbstring_mac, sizeof(s->usbstring_mac),
>               "%02x%02x%02x%02x%02x%02x",
> -             0x40,
> +             s->conf.macaddr.a[0],
>               s->conf.macaddr.a[1],
>               s->conf.macaddr.a[2],
>               s->conf.macaddr.a[3],

To repeat what I said in the ticket, the 0x40 byte originates from when
this was first committed to QEMU. We can see the finall accepted patch

  https://lists.nongnu.org/archive/html/qemu-devel/2008-07/msg00385.html

but tracing the history back further, this was *not* in the version of
the patches submitted by the original author 2 years earlier:

 https://lists.nongnu.org/archive/html/qemu-devel/2006-10/msg00339.html
 
There's no explanation of this difference. Could easily be a left-over
hack from some debugging attempt that no one noticed until now.

It can't have been forcing a specific vendor's prefix, since prefixes
are more than 1 byte long. To me, it smells like a debugging hack
where someone wanted to see this fixed byte appear, though even that
is a bit wierd as you could just set the desired mac as a cli arg.

Anyway I can't come up with a justification for keeping this, so

  Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

..unless someone believes we need to tie this change to a machine type
version ?

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH] hw/usb/network: Remove hardcoded 0x40 prefix

[Peter Maydell]

On Wed, 7 May 2025 at 09:46, Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> On Tue, May 06, 2025 at 12:45:53PM -0400, Stéphane Graber via wrote:
> > USB NICs have a "40:" prefix hardcoded for all MAC addresses.
> >
> > This overrides user-provided configuration and leads to an inconsistent
> > experience.
> >
> > I couldn't find any documented reason (comment or git commits) for this
> > behavior. It seems like everyone is just expecting the MAC address to be
> > fully passed through to the guest, but it isn't.
> >
> > This is also particularly problematic as the "40:" prefix isn't a
> > reserved prefix for MAC addresses (IEEE OUI). There are a number of
> > valid allocations out there which use this prefix, meaning that QEMU may
> > be causing MAC address conflicts.
> >
> > Signed-off-by: Stéphane Graber <stgraber@stgraber.org>
> > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2951
> > ---
> >  hw/usb/dev-network.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/hw/usb/dev-network.c b/hw/usb/dev-network.c
> > index 81cc09dcac..1df2454181 100644
> > --- a/hw/usb/dev-network.c
> > +++ b/hw/usb/dev-network.c
> > @@ -1383,7 +1383,7 @@ static void usb_net_realize(USBDevice *dev, Error **errp)
> >      qemu_format_nic_info_str(qemu_get_queue(s->nic), s->conf.macaddr.a);
> >      snprintf(s->usbstring_mac, sizeof(s->usbstring_mac),
> >               "%02x%02x%02x%02x%02x%02x",
> > -             0x40,
> > +             s->conf.macaddr.a[0],
> >               s->conf.macaddr.a[1],
> >               s->conf.macaddr.a[2],
> >               s->conf.macaddr.a[3],

Note in particular that this string is used *only* for
what we return to the guest if it queries the STRING_ETHADDR
USB string property. It's not used for what we return for
the OID_802_3_PERMANENT_ADDRESS or OID_802_3_CURRENT_ADDRESS OIDs
for NDIS, or for the MAC address we actually use in the QEMU networking
code to send/receive packets for this device, or in the NIC info
string we print for users. In all those other places we directly
use s->conf.macaddr.a, which is the full thing the user asks for.

> To repeat what I said in the ticket, the 0x40 byte originates from when
> this was first committed to QEMU. We can see the finall accepted patch
>
>   https://lists.nongnu.org/archive/html/qemu-devel/2008-07/msg00385.html
>
> but tracing the history back further, this was *not* in the version of
> the patches submitted by the original author 2 years earlier:
>
>  https://lists.nongnu.org/archive/html/qemu-devel/2006-10/msg00339.html
>
> There's no explanation of this difference. Could easily be a left-over
> hack from some debugging attempt that no one noticed until now.

That original version of the patches is even odder, because it
does this for the STRING_ETHADDR:

+                       case STRING_ETHADDR:
+                               ret = set_usb_string(data, "400102030405");
+                               break;

i.e. hardcodes it entirely.

I think we should note in the commit message that the hardcoded
0x40 is only used for STRING_ETHADDR and not for any of the
other ways we use the MAC address. But otherwise

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

thanks
-- PMM

Re: [PATCH] hw/usb/network: Remove hardcoded 0x40 prefix

[Peter Maydell]

On Mon, 12 May 2025 at 15:10, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Wed, 7 May 2025 at 09:46, Daniel P. Berrangé <berrange@redhat.com> wrote:
> >
> > On Tue, May 06, 2025 at 12:45:53PM -0400, Stéphane Graber via wrote:
> > > USB NICs have a "40:" prefix hardcoded for all MAC addresses.
> > >
> > > This overrides user-provided configuration and leads to an inconsistent
> > > experience.
> > >
> > > I couldn't find any documented reason (comment or git commits) for this
> > > behavior. It seems like everyone is just expecting the MAC address to be
> > > fully passed through to the guest, but it isn't.
> > >
> > > This is also particularly problematic as the "40:" prefix isn't a
> > > reserved prefix for MAC addresses (IEEE OUI). There are a number of
> > > valid allocations out there which use this prefix, meaning that QEMU may
> > > be causing MAC address conflicts.
> > >
> > > Signed-off-by: Stéphane Graber <stgraber@stgraber.org>
> > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2951
> > > ---
> > >  hw/usb/dev-network.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/hw/usb/dev-network.c b/hw/usb/dev-network.c
> > > index 81cc09dcac..1df2454181 100644
> > > --- a/hw/usb/dev-network.c
> > > +++ b/hw/usb/dev-network.c
> > > @@ -1383,7 +1383,7 @@ static void usb_net_realize(USBDevice *dev, Error **errp)
> > >      qemu_format_nic_info_str(qemu_get_queue(s->nic), s->conf.macaddr.a);
> > >      snprintf(s->usbstring_mac, sizeof(s->usbstring_mac),
> > >               "%02x%02x%02x%02x%02x%02x",
> > > -             0x40,
> > > +             s->conf.macaddr.a[0],
> > >               s->conf.macaddr.a[1],
> > >               s->conf.macaddr.a[2],
> > >               s->conf.macaddr.a[3],
>
> Note in particular that this string is used *only* for
> what we return to the guest if it queries the STRING_ETHADDR
> USB string property. It's not used for what we return for
> the OID_802_3_PERMANENT_ADDRESS or OID_802_3_CURRENT_ADDRESS OIDs
> for NDIS, or for the MAC address we actually use in the QEMU networking
> code to send/receive packets for this device, or in the NIC info
> string we print for users. In all those other places we directly
> use s->conf.macaddr.a, which is the full thing the user asks for.
>
> > To repeat what I said in the ticket, the 0x40 byte originates from when
> > this was first committed to QEMU. We can see the finall accepted patch
> >
> >   https://lists.nongnu.org/archive/html/qemu-devel/2008-07/msg00385.html
> >
> > but tracing the history back further, this was *not* in the version of
> > the patches submitted by the original author 2 years earlier:
> >
> >  https://lists.nongnu.org/archive/html/qemu-devel/2006-10/msg00339.html
> >
> > There's no explanation of this difference. Could easily be a left-over
> > hack from some debugging attempt that no one noticed until now.
>
> That original version of the patches is even odder, because it
> does this for the STRING_ETHADDR:
>
> +                       case STRING_ETHADDR:
> +                               ret = set_usb_string(data, "400102030405");
> +                               break;
>
> i.e. hardcodes it entirely.
>
> I think we should note in the commit message that the hardcoded
> 0x40 is only used for STRING_ETHADDR and not for any of the
> other ways we use the MAC address. But otherwise
>
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

I just noticed that we never picked up this patch. I've taken
it into target-arm.next (and added some text to the commit message
to reflect the comments in this thread).

thanks
-- PMM

Re: [PATCH] hw/usb/network: Remove hardcoded 0x40 prefix

[Philippe Mathieu-Daudé]

On 1/9/25 18:11, Peter Maydell wrote:
> On Mon, 12 May 2025 at 15:10, Peter Maydell <peter.maydell@linaro.org> wrote:


> I just noticed that we never picked up this patch. I've taken
> it into target-arm.next (and added some text to the commit message
> to reflect the comments in this thread).

Oops, I had this one tagged, but totally forgot about it... Thanks!