Re: Internal MAC addresses list (mac_table) usage

[Jason Wang]

On Thu, Jul 14, 2022 at 7:54 PM Ovchinnikov, Vitalii
<vitalii.ovchinnikov@auriga.com> wrote:
>
> 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.

This should be fine, and it needs to be addressed in a separate patch.

Thanks

> 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 <jasowang@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.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
> >
>
>
>