Internal MAC addresses list (mac_table) usage

Internal MAC addresses list (mac_table) usage

[Ovchinnikov, Vitalii]

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?

BR,
Vitalii

Re: Internal MAC addresses list (mac_table) usage

[Jason Wang]

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
>

Re: Internal MAC addresses list (mac_table) usage

[Ovchinnikov, Vitalii]

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 <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
>

    

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
> >
>
>
>