Re: Internal MAC addresses list (mac_table) usage

[Jason Wang <jasowang@redhat.com>]

On Tue, Jul 12, 2022 at 4:43 PM Ovchinnikov, Vitalii
<vitalii.ovchinnikov@auriga.com> wrote:
>
> Hi folks,
>
> While developing an Ethernet NIC model I noticed that QEMU maintains the following internal array which marks used/free MAC addresses in net/net.c:
>
> static int mac_table[256] = {0};
>
> with three private (static) functions accessing it: qemu_macaddr_set_used, qemu_macaddr_set_free, qemu_macaddr_get_free.
> Public (non-static) interface to this array includes two functions: qemu_macaddr_default_if_unset and qemu_del_nic.
>
> The vast majority of existing NIC models calls qemu_macaddr_default_if_unset in their *_realize functions replacing zeroed-out MAC address with the free one returned by QEMU, for instance (lan9118_realize functions from hw/net/lan9118.c):
>
>    ...
>     qemu_macaddr_default_if_unset(&s->conf.macaddr);
>
>     s->nic = qemu_new_nic(&net_lan9118_info, &s->conf,
>                           object_get_typename(OBJECT(dev)), dev->id, s);
>     qemu_format_nic_info_str(qemu_get_queue(s->nic), s->conf.macaddr.a);
>    ...
>
> qemu_del_nic is being called from net_cleanup function right before QEMU finishes execution.
>
> What appears to be a possible SW architecture gap is that NIC models have no means to inform QEMU about changing their MAC addresses during execution (again from hw/net/lan9118.c, do_mac_write function):
>
>     case MAC_ADDRH:
>         s->conf.macaddr.a[4] = val & 0xff;
>         s->conf.macaddr.a[5] = (val >> 8) & 0xff;
>         lan9118_mac_changed(s);
>         break;
>     case MAC_ADDRL:
>         s->conf.macaddr.a[0] = val & 0xff;
>         s->conf.macaddr.a[1] = (val >> 8) & 0xff;
>         s->conf.macaddr.a[2] = (val >> 16) & 0xff;
>         s->conf.macaddr.a[3] = (val >> 24) & 0xff;
>         lan9118_mac_changed(s);
>         break;
>
> lan9118_mac_changed function here simply changes NIC info string using qemu_format_nic_info_str, hence stale MAC address stays marked as used in the mac_table whereas it's not actually in use any more.
>
> Am I right in thinking of it as a SW architecture gap/bug that needs to be addressed?

I think so. Note that the code can not deal with the case when
"52:54:00:12:34:XX" was passed from cli.

Thanks

>
> BR,
> Vitalii
>