[PATCH v5 0/1] virtio-net: Add check for mac address while peer is vdpa

[PATCH v5 0/1] virtio-net: Add check for mac address while peer is vdpa

[Cindy Lu]

Add check for mac address while peer is vdpa
Change from v4->v5
1. Add comment for this work around
2. while copy the netconfig, only overwrite the mac address


Cindy Lu (1):
  virtio-net: Add check for mac address while peer is vdpa

 hw/net/virtio-net.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

-- 
2.21.3

[PATCH v5 1/1] virtio-net: Add check for mac address while peer is vdpa

[Cindy Lu]

While peer is vdpa, sometime qemu get an all zero mac address from the hardware,
This is not a legal value. Add the check for this.if we get an zero mac address.
qemu will use the default mac address or the mac address from qemu cmdline

Signed-off-by: Cindy Lu <lulu@redhat.com>
---
 hw/net/virtio-net.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 9179013ac4..8f36ca5066 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -126,6 +126,7 @@ static void virtio_net_get_config(VirtIODevice *vdev, uint8_t *config)
     VirtIONet *n = VIRTIO_NET(vdev);
     struct virtio_net_config netcfg;
     NetClientState *nc = qemu_get_queue(n->nic);
+    static const MACAddr zero = { .a = { 0, 0, 0, 0, 0, 0 } };
 
     int ret = 0;
     memset(&netcfg, 0 , sizeof(struct virtio_net_config));
@@ -151,6 +152,15 @@ static void virtio_net_get_config(VirtIODevice *vdev, uint8_t *config)
         ret = vhost_net_get_config(get_vhost_net(nc->peer), (uint8_t *)&netcfg,
                                    n->config_size);
         if (ret != -1) {
+            /*
+             * Here is a work around, the 0 mac address is not a legal value.
+             * if we got this from hardware, qemu will use the mac address
+             * saved in VirtIONet->mac.
+             */
+            if (memcmp(&netcfg.mac, &zero, sizeof(zero)) == 0) {
+                info_report("Get an all zero mac address from hardware");
+                memcpy(netcfg.mac, n->mac, ETH_ALEN);
+            }
             memcpy(config, &netcfg, n->config_size);
         }
     }
-- 
2.21.3

Re: [PATCH v5 1/1] virtio-net: Add check for mac address while peer is vdpa

[Michael S. Tsirkin]

On Fri, Feb 26, 2021 at 12:55:06AM +0800, Cindy Lu wrote:
> While peer is vdpa, sometime qemu get an all zero mac address from the hardware,
> This is not a legal value. Add the check for this.if we get an zero mac address.
> qemu will use the default mac address or the mac address from qemu cmdline
> 
> Signed-off-by: Cindy Lu <lulu@redhat.com>

I guess I will have to rewrite the comments and commit log :(

It is all saying what does the patch do. We want it to rather
give motivation.

Sean could you please comment on whether this patch fixes your
config?

> ---
>  hw/net/virtio-net.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index 9179013ac4..8f36ca5066 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -126,6 +126,7 @@ static void virtio_net_get_config(VirtIODevice *vdev, uint8_t *config)
>      VirtIONet *n = VIRTIO_NET(vdev);
>      struct virtio_net_config netcfg;
>      NetClientState *nc = qemu_get_queue(n->nic);
> +    static const MACAddr zero = { .a = { 0, 0, 0, 0, 0, 0 } };
>  
>      int ret = 0;
>      memset(&netcfg, 0 , sizeof(struct virtio_net_config));
> @@ -151,6 +152,15 @@ static void virtio_net_get_config(VirtIODevice *vdev, uint8_t *config)
>          ret = vhost_net_get_config(get_vhost_net(nc->peer), (uint8_t *)&netcfg,
>                                     n->config_size);
>          if (ret != -1) {
> +            /*
> +             * Here is a work around, the 0 mac address is not a legal value.
> +             * if we got this from hardware, qemu will use the mac address
> +             * saved in VirtIONet->mac.
> +             */
> +            if (memcmp(&netcfg.mac, &zero, sizeof(zero)) == 0) {
> +                info_report("Get an all zero mac address from hardware");
> +                memcpy(netcfg.mac, n->mac, ETH_ALEN);
> +            }
>              memcpy(config, &netcfg, n->config_size);
>          }
>      }
> -- 
> 2.21.3

Re: [PATCH v5 1/1] virtio-net: Add check for mac address while peer is vdpa

[Michael S. Tsirkin]

On Thu, Feb 25, 2021 at 02:14:39PM -0500, Michael S. Tsirkin wrote:
> On Fri, Feb 26, 2021 at 12:55:06AM +0800, Cindy Lu wrote:
> > While peer is vdpa, sometime qemu get an all zero mac address from the hardware,
> > This is not a legal value. Add the check for this.if we get an zero mac address.
> > qemu will use the default mac address or the mac address from qemu cmdline
> > 
> > Signed-off-by: Cindy Lu <lulu@redhat.com>
> 
> I guess I will have to rewrite the comments and commit log :(
> 
> It is all saying what does the patch do. We want it to rather
> give motivation.
> 
> Sean could you please comment on whether this patch fixes your
> config?

ping. if I'm to try and merge this work around it's critical
that someone with access to hardware confirm it actually works.


> > ---
> >  hw/net/virtio-net.c | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> > 
> > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> > index 9179013ac4..8f36ca5066 100644
> > --- a/hw/net/virtio-net.c
> > +++ b/hw/net/virtio-net.c
> > @@ -126,6 +126,7 @@ static void virtio_net_get_config(VirtIODevice *vdev, uint8_t *config)
> >      VirtIONet *n = VIRTIO_NET(vdev);
> >      struct virtio_net_config netcfg;
> >      NetClientState *nc = qemu_get_queue(n->nic);
> > +    static const MACAddr zero = { .a = { 0, 0, 0, 0, 0, 0 } };
> >  
> >      int ret = 0;
> >      memset(&netcfg, 0 , sizeof(struct virtio_net_config));
> > @@ -151,6 +152,15 @@ static void virtio_net_get_config(VirtIODevice *vdev, uint8_t *config)
> >          ret = vhost_net_get_config(get_vhost_net(nc->peer), (uint8_t *)&netcfg,
> >                                     n->config_size);
> >          if (ret != -1) {
> > +            /*
> > +             * Here is a work around, the 0 mac address is not a legal value.
> > +             * if we got this from hardware, qemu will use the mac address
> > +             * saved in VirtIONet->mac.
> > +             */
> > +            if (memcmp(&netcfg.mac, &zero, sizeof(zero)) == 0) {
> > +                info_report("Get an all zero mac address from hardware");
> > +                memcpy(netcfg.mac, n->mac, ETH_ALEN);
> > +            }
> >              memcpy(config, &netcfg, n->config_size);
> >          }
> >      }
> > -- 
> > 2.21.3

Re: [PATCH v5 1/1] virtio-net: Add check for mac address while peer is vdpa

[Cindy Lu]

On Mon, Mar 1, 2021 at 4:40 AM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Thu, Feb 25, 2021 at 02:14:39PM -0500, Michael S. Tsirkin wrote:
> > On Fri, Feb 26, 2021 at 12:55:06AM +0800, Cindy Lu wrote:
> > > While peer is vdpa, sometime qemu get an all zero mac address from the hardware,
> > > This is not a legal value. Add the check for this.if we get an zero mac address.
> > > qemu will use the default mac address or the mac address from qemu cmdline
> > >
> > > Signed-off-by: Cindy Lu <lulu@redhat.com>
> >
> > I guess I will have to rewrite the comments and commit log :(
> >
> > It is all saying what does the patch do. We want it to rather
> > give motivation.
> >
> > Sean could you please comment on whether this patch fixes your
> > config?
>
> ping. if I'm to try and merge this work around it's critical
> that someone with access to hardware confirm it actually works.
>
Hi Michael, I have tested this patch in qemu+vdpa+mlx environment.
it's working fine.
>
> > > ---
> > >  hw/net/virtio-net.c | 10 ++++++++++
> > >  1 file changed, 10 insertions(+)
> > >
> > > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> > > index 9179013ac4..8f36ca5066 100644
> > > --- a/hw/net/virtio-net.c
> > > +++ b/hw/net/virtio-net.c
> > > @@ -126,6 +126,7 @@ static void virtio_net_get_config(VirtIODevice *vdev, uint8_t *config)
> > >      VirtIONet *n = VIRTIO_NET(vdev);
> > >      struct virtio_net_config netcfg;
> > >      NetClientState *nc = qemu_get_queue(n->nic);
> > > +    static const MACAddr zero = { .a = { 0, 0, 0, 0, 0, 0 } };
> > >
> > >      int ret = 0;
> > >      memset(&netcfg, 0 , sizeof(struct virtio_net_config));
> > > @@ -151,6 +152,15 @@ static void virtio_net_get_config(VirtIODevice *vdev, uint8_t *config)
> > >          ret = vhost_net_get_config(get_vhost_net(nc->peer), (uint8_t *)&netcfg,
> > >                                     n->config_size);
> > >          if (ret != -1) {
> > > +            /*
> > > +             * Here is a work around, the 0 mac address is not a legal value.
> > > +             * if we got this from hardware, qemu will use the mac address
> > > +             * saved in VirtIONet->mac.
> > > +             */
> > > +            if (memcmp(&netcfg.mac, &zero, sizeof(zero)) == 0) {
> > > +                info_report("Get an all zero mac address from hardware");
> > > +                memcpy(netcfg.mac, n->mac, ETH_ALEN);
> > > +            }
> > >              memcpy(config, &netcfg, n->config_size);
> > >          }
> > >      }
> > > --
> > > 2.21.3
>

Re: [PATCH v5 1/1] virtio-net: Add check for mac address while peer is vdpa

[Adrian Moreno]

On 3/1/21 2:36 AM, Cindy Lu wrote:
> On Mon, Mar 1, 2021 at 4:40 AM Michael S. Tsirkin <mst@redhat.com> wrote:
>>
>> On Thu, Feb 25, 2021 at 02:14:39PM -0500, Michael S. Tsirkin wrote:
>>> On Fri, Feb 26, 2021 at 12:55:06AM +0800, Cindy Lu wrote:
>>>> While peer is vdpa, sometime qemu get an all zero mac address from the hardware,
>>>> This is not a legal value. Add the check for this.if we get an zero mac address.
>>>> qemu will use the default mac address or the mac address from qemu cmdline
>>>>
>>>> Signed-off-by: Cindy Lu <lulu@redhat.com>
>>>
>>> I guess I will have to rewrite the comments and commit log :(
>>>
>>> It is all saying what does the patch do. We want it to rather
>>> give motivation.
>>>
>>> Sean could you please comment on whether this patch fixes your
>>> config?
>>
>> ping. if I'm to try and merge this work around it's critical
>> that someone with access to hardware confirm it actually works.
>>
> Hi Michael, I have tested this patch in qemu+vdpa+mlx environment.
> it's working fine.

I have also verified it. For the record, I'll expand:

Libvirt definition contains:

    <interface type='vdpa'>
      <mac address='52:54:00:8e:a4:12'/>
      <source dev='/dev/vhost-vdpa-0'/>
      <model type='virtio'/>
      <alias name='net1'/>
      <address type='pci' domain='0x0000' bus='0x06' slot='0x00' function='0x0'/>
    </interface>
	
Qemu command line contains:

-add-fd set=3,fd=42,opaque=/dev/vhost-vdpa-0 \
-netdev vhost-vdpa,vhostdev=/dev/fdset/3,id=hostnet1 \
-device
virtio-net-pci,netdev=hostnet1,id=net1,mac=52:54:00:8e:a4:12,bus=pci.4,addr=0x0 \

The vdpa device is created by mlx_vdpa (Connect-X6 DX) on switchdev mode.

The original problem was:
The mac address in struct virtio_net_config in the vdpa device is zero because
it has not been configured. This does not have implications in the traffic
steering since the card will rely on the eswitch flow configuration to perform
smac/dmac filtering (in my case, this is done through ovs + tcflower hw offload).

Since we anyhow have to rely on the mac address having been configured
externally on the eswitch, we can also trust that whoever applied such flow
configuration also gave us the correct mac address in the cmd line. Therefore,
exposing such address to the guest seems like a sane way to tell it what mac
address to use.

Until the vdpa framework / iproute2 tool support configuring the
virtio_net_config struct and such mechanism is used externally to align the
virtio device's mac with the one configured in the eswitch, this patch allows
the guest to have connectivity.


>>
>>>> ---
>>>>  hw/net/virtio-net.c | 10 ++++++++++
>>>>  1 file changed, 10 insertions(+)
>>>>
>>>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
>>>> index 9179013ac4..8f36ca5066 100644
>>>> --- a/hw/net/virtio-net.c
>>>> +++ b/hw/net/virtio-net.c
>>>> @@ -126,6 +126,7 @@ static void virtio_net_get_config(VirtIODevice *vdev, uint8_t *config)
>>>>      VirtIONet *n = VIRTIO_NET(vdev);
>>>>      struct virtio_net_config netcfg;
>>>>      NetClientState *nc = qemu_get_queue(n->nic);
>>>> +    static const MACAddr zero = { .a = { 0, 0, 0, 0, 0, 0 } };
>>>>
>>>>      int ret = 0;
>>>>      memset(&netcfg, 0 , sizeof(struct virtio_net_config));
>>>> @@ -151,6 +152,15 @@ static void virtio_net_get_config(VirtIODevice *vdev, uint8_t *config)
>>>>          ret = vhost_net_get_config(get_vhost_net(nc->peer), (uint8_t *)&netcfg,
>>>>                                     n->config_size);
>>>>          if (ret != -1) {
>>>> +            /*
>>>> +             * Here is a work around, the 0 mac address is not a legal value.
>>>> +             * if we got this from hardware, qemu will use the mac address
>>>> +             * saved in VirtIONet->mac.
>>>> +             */
>>>> +            if (memcmp(&netcfg.mac, &zero, sizeof(zero)) == 0) {
>>>> +                info_report("Get an all zero mac address from hardware");

s/Get/Got/?

>>>> +                memcpy(netcfg.mac, n->mac, ETH_ALEN);
>>>> +            }
>>>>              memcpy(config, &netcfg, n->config_size);
>>>>          }
>>>>      }
>>>> --
>>>> 2.21.3
>>
> 

FWIW:
Tested-by: Adrián Moreno <amorenoz@redhat.com>

-- 
Adrián Moreno

Re: [PATCH v5 1/1] virtio-net: Add check for mac address while peer is vdpa

[Sean Mooney]

On Thu, 2021-02-25 at 14:14 -0500, Michael S. Tsirkin wrote:
> On Fri, Feb 26, 2021 at 12:55:06AM +0800, Cindy Lu wrote:
> > While peer is vdpa, sometime qemu get an all zero mac address from the hardware,
> > This is not a legal value. Add the check for this.if we get an zero mac address.
> > qemu will use the default mac address or the mac address from qemu cmdline
> > 
> > Signed-off-by: Cindy Lu <lulu@redhat.com>
> 
> I guess I will have to rewrite the comments and commit log :(
> 
> It is all saying what does the patch do. We want it to rather
> give motivation.
> 
> Sean could you please comment on whether this patch fixes your
> config?
sorry this came in just as i was finsihing up for the day and firday was a company day.

i just applied the patch on my dev env.
when i do the mac is set to the value i set on the qemu command line.
i am able to bring up the interface in the guest and i can successfully ping the host so setting 
only the mac seams to be sufficent to allow the vdpa interface to be used in a guest.

i tested using an unmodifed alpine linux image for the guest and appied the patch
to qemu commit e90ef02389dc8b57eaea22b290244609d720a8bf which was the had of master when i cloned qemu a few days ago.
regards
sean

> 
> > ---
> >  hw/net/virtio-net.c | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> > 
> > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> > index 9179013ac4..8f36ca5066 100644
> > --- a/hw/net/virtio-net.c
> > +++ b/hw/net/virtio-net.c
> > @@ -126,6 +126,7 @@ static void virtio_net_get_config(VirtIODevice *vdev, uint8_t *config)
> >      VirtIONet *n = VIRTIO_NET(vdev);
> >      struct virtio_net_config netcfg;
> >      NetClientState *nc = qemu_get_queue(n->nic);
> > +    static const MACAddr zero = { .a = { 0, 0, 0, 0, 0, 0 } };
> >  
> > 
> > 
> > 
> >      int ret = 0;
> >      memset(&netcfg, 0 , sizeof(struct virtio_net_config));
> > @@ -151,6 +152,15 @@ static void virtio_net_get_config(VirtIODevice *vdev, uint8_t *config)
> >          ret = vhost_net_get_config(get_vhost_net(nc->peer), (uint8_t *)&netcfg,
> >                                     n->config_size);
> >          if (ret != -1) {
> > +            /*
> > +             * Here is a work around, the 0 mac address is not a legal value.
> > +             * if we got this from hardware, qemu will use the mac address
> > +             * saved in VirtIONet->mac.
> > +             */
> > +            if (memcmp(&netcfg.mac, &zero, sizeof(zero)) == 0) {
> > +                info_report("Get an all zero mac address from hardware");
> > +                memcpy(netcfg.mac, n->mac, ETH_ALEN);
> > +            }
> >              memcpy(config, &netcfg, n->config_size);
> >          }
> >      }
> > -- 
> > 2.21.3
>