Hi Jason, Thanks for pointing out that corner case with "52:54:00:12:34:XX".
In the NIC model I'm developing qemu_macaddr_default_if_unset is called every time MAC is updated in the NIC registers. This way a just assigned "52:54:00:12:34:XX" MAC is at least marked as used in the mac_table. However it doesn't cover the case when "52:54:00:12:34:XX" MAC being assigned through NIC registers has already been assigned to another NIC by QEMU. So one more improvement the code might need is a way to check whether MAC is free or used from within NIC model. Returning bool from qemu_macaddr_default_if_unset may well do the trick. Moreover it might also help to spot an error when -1 is returned from qemu_macaddr_get_free (for the time being it's silently interpreted as 0xFF MAC LSB). BR, Vitalii From: Jason Wang <jasow...@redhat.com> Sent: Thursday, July 14, 2022 9:44 To: Ovchinnikov, Vitalii Cc: qemu-devel@nongnu.org Subject: Re: Internal MAC addresses list (mac_table) usage On Tue, Jul 12, 2022 at 4:43 PM Ovchinnikov, Vitalii <vitalii.ovchinni...@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 >