[PATCH 1/3] virtio-net: Set mac address to hardware if the peer is vdpa

[PATCH 1/3] virtio-net: Set mac address to hardware if the peer is vdpa

[Cindy Lu]

If the peer's type is vdpa,set the mac address to NIC in virtio_net_device_realize,
Also sometime vdpa get an all 0 macaddress from the hardware, this will cause the traffic down
So we add the check for this part.
if we get an 0 mac address we will use the default mac address instead

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

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index cb0d27084c..7db9da1482 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,7 +152,11 @@ 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) {
-            memcpy(config, &netcfg, n->config_size);
+            if (memcmp(&netcfg.mac, &zero, sizeof(zero)) != 0) {
+                memcpy(config, &netcfg, n->config_size);
+        } else {
+                error_report("Get an all zero mac address from hardware");
+            }
         }
     }
 }
@@ -3399,6 +3404,11 @@ static void virtio_net_device_realize(DeviceState *dev, Error **errp)
     nc = qemu_get_queue(n->nic);
     nc->rxfilter_notify_enabled = 1;
 
+   if (nc->peer && nc->peer->info->type == NET_CLIENT_DRIVER_VHOST_VDPA) {
+        struct virtio_net_config netcfg = {};
+        memcpy(&netcfg.mac, &n->nic_conf.macaddr, ETH_ALEN);
+        virtio_net_set_config(vdev, (uint8_t *)&netcfg);
+    }
     QTAILQ_INIT(&n->rsc_chains);
     n->qdev = dev;
 
-- 
2.21.3

[PATCH 2/3] vhost-vdpa: Add qemu_close in vhost_vdpa_cleanup

[Cindy Lu]

fix the bug that fd will still open after the cleanup

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

diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
index bc0e0d2d35..0480b92102 100644
--- a/net/vhost-vdpa.c
+++ b/net/vhost-vdpa.c
@@ -144,6 +144,10 @@ static void vhost_vdpa_cleanup(NetClientState *nc)
         g_free(s->vhost_net);
         s->vhost_net = NULL;
     }
+     if (s->vhost_vdpa.device_fd >= 0) {
+        qemu_close(s->vhost_vdpa.device_fd);
+        s->vhost_vdpa.device_fd = -1;
+    }
 }
 
 static bool vhost_vdpa_has_vnet_hdr(NetClientState *nc)
-- 
2.21.3

[PATCH 3/3] net: Add vhost-vdpa in show_netdevs()

[Cindy Lu]

Fix the bug that while Check qemu supported netdev,
there is no vhost-vdpa

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

diff --git a/net/net.c b/net/net.c
index 7a2a0fb5ac..794c652282 100644
--- a/net/net.c
+++ b/net/net.c
@@ -1049,6 +1049,9 @@ static void show_netdevs(void)
 #endif
 #ifdef CONFIG_POSIX
         "vhost-user",
+#endif
+#ifdef CONFIG_VHOST_VDPA
+        "vhost-vdpa",
 #endif
     };
 
-- 
2.21.3

Re: [PATCH 1/3] virtio-net: Set mac address to hardware if the peer is vdpa

[Jason Wang]

On 2020/9/17 下午11:58, Cindy Lu wrote:
> If the peer's type is vdpa,set the mac address to NIC in virtio_net_device_realize,
> Also sometime vdpa get an all 0 macaddress from the hardware, this will cause the traffic down
> So we add the check for this part.
> if we get an 0 mac address we will use the default mac address instead
>
> Signed-off-by: Cindy Lu <lulu@redhat.com>
> ---
>   hw/net/virtio-net.c | 12 +++++++++++-
>   1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index cb0d27084c..7db9da1482 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,7 +152,11 @@ 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) {
> -            memcpy(config, &netcfg, n->config_size);
> +            if (memcmp(&netcfg.mac, &zero, sizeof(zero)) != 0) {
> +                memcpy(config, &netcfg, n->config_size);
> +        } else {
> +                error_report("Get an all zero mac address from hardware");


This is probably a hint that MAC is not properly provisioned.

So I guess we can leave this as is, or simply warn until the management 
interface is finalized.


> +            }
>           }
>       }
>   }
> @@ -3399,6 +3404,11 @@ static void virtio_net_device_realize(DeviceState *dev, Error **errp)
>       nc = qemu_get_queue(n->nic);
>       nc->rxfilter_notify_enabled = 1;
>   
> +   if (nc->peer && nc->peer->info->type == NET_CLIENT_DRIVER_VHOST_VDPA) {
> +        struct virtio_net_config netcfg = {};
> +        memcpy(&netcfg.mac, &n->nic_conf.macaddr, ETH_ALEN);
> +        virtio_net_set_config(vdev, (uint8_t *)&netcfg);


Won't this overwrite all other fields in the netcfg? I think we should 
only touch mac part.

Thanks


> +    }
>       QTAILQ_INIT(&n->rsc_chains);
>       n->qdev = dev;
>   

Re: [PATCH 2/3] vhost-vdpa: Add qemu_close in vhost_vdpa_cleanup

[Jason Wang]

On 2020/9/17 下午11:58, Cindy Lu wrote:
> fix the bug that fd will still open after the cleanup
>
> Signed-off-by: Cindy Lu <lulu@redhat.com>


Acked-by: Jason Wang <jasowang@redhat.com>


> ---
>   net/vhost-vdpa.c | 4 ++++
>   1 file changed, 4 insertions(+)
>
> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> index bc0e0d2d35..0480b92102 100644
> --- a/net/vhost-vdpa.c
> +++ b/net/vhost-vdpa.c
> @@ -144,6 +144,10 @@ static void vhost_vdpa_cleanup(NetClientState *nc)
>           g_free(s->vhost_net);
>           s->vhost_net = NULL;
>       }
> +     if (s->vhost_vdpa.device_fd >= 0) {
> +        qemu_close(s->vhost_vdpa.device_fd);
> +        s->vhost_vdpa.device_fd = -1;
> +    }
>   }
>   
>   static bool vhost_vdpa_has_vnet_hdr(NetClientState *nc)

Re: [PATCH 3/3] net: Add vhost-vdpa in show_netdevs()

[Jason Wang]

On 2020/9/17 下午11:58, Cindy Lu wrote:
> Fix the bug that while Check qemu supported netdev,
> there is no vhost-vdpa
>
> Signed-off-by: Cindy Lu <lulu@redhat.com>


Acked-by: Jason Wang <jasowang@redhat.com>


> ---
>   net/net.c | 3 +++
>   1 file changed, 3 insertions(+)
>
> diff --git a/net/net.c b/net/net.c
> index 7a2a0fb5ac..794c652282 100644
> --- a/net/net.c
> +++ b/net/net.c
> @@ -1049,6 +1049,9 @@ static void show_netdevs(void)
>   #endif
>   #ifdef CONFIG_POSIX
>           "vhost-user",
> +#endif
> +#ifdef CONFIG_VHOST_VDPA
> +        "vhost-vdpa",
>   #endif
>       };
>   

Re: [PATCH 1/3] virtio-net: Set mac address to hardware if the peer is vdpa

[Cindy Lu]

On Tue, Sep 22, 2020 at 9:55 AM Jason Wang <jasowang@redhat.com> wrote:
>
>
> On 2020/9/17 下午11:58, Cindy Lu wrote:
> > If the peer's type is vdpa,set the mac address to NIC in virtio_net_device_realize,
> > Also sometime vdpa get an all 0 macaddress from the hardware, this will cause the traffic down
> > So we add the check for this part.
> > if we get an 0 mac address we will use the default mac address instead
> >
> > Signed-off-by: Cindy Lu <lulu@redhat.com>
> > ---
> >   hw/net/virtio-net.c | 12 +++++++++++-
> >   1 file changed, 11 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> > index cb0d27084c..7db9da1482 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,7 +152,11 @@ 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) {
> > -            memcpy(config, &netcfg, n->config_size);
> > +            if (memcmp(&netcfg.mac, &zero, sizeof(zero)) != 0) {
> > +                memcpy(config, &netcfg, n->config_size);
> > +        } else {
> > +                error_report("Get an all zero mac address from hardware");
>
>
> This is probably a hint that MAC is not properly provisioned.
>
> So I guess we can leave this as is, or simply warn until the management
> interface is finalized.
>
Hi Jason, For sure this is NIC card's problem, They cannot provide an
correct MAC address,
But if we continue use this 0 mac address will cause this traffic
down, maybe will cost a lot of effort in debugging
So I think maybe Just an warn is not enough, We can use the default
mac address  and let the traffic working
>
> > +            }
> >           }
> >       }
> >   }
> > @@ -3399,6 +3404,11 @@ static void virtio_net_device_realize(DeviceState *dev, Error **errp)
> >       nc = qemu_get_queue(n->nic);
> >       nc->rxfilter_notify_enabled = 1;
> >
> > +   if (nc->peer && nc->peer->info->type == NET_CLIENT_DRIVER_VHOST_VDPA) {
> > +        struct virtio_net_config netcfg = {};
> > +        memcpy(&netcfg.mac, &n->nic_conf.macaddr, ETH_ALEN);
> > +        virtio_net_set_config(vdev, (uint8_t *)&netcfg);
>
>
> Won't this overwrite all other fields in the netcfg? I think we should
> only touch mac part.
>
> Thanks
>
>
Sure, will fix this
> > +    }
> >       QTAILQ_INIT(&n->rsc_chains);
> >       n->qdev = dev;
> >
>

Re: [PATCH 1/3] virtio-net: Set mac address to hardware if the peer is vdpa

[Jason Wang]

On 2020/9/22 上午11:01, Cindy Lu wrote:
> On Tue, Sep 22, 2020 at 9:55 AM Jason Wang <jasowang@redhat.com> wrote:
>>
>> On 2020/9/17 下午11:58, Cindy Lu wrote:
>>> If the peer's type is vdpa,set the mac address to NIC in virtio_net_device_realize,
>>> Also sometime vdpa get an all 0 macaddress from the hardware, this will cause the traffic down
>>> So we add the check for this part.
>>> if we get an 0 mac address we will use the default mac address instead
>>>
>>> Signed-off-by: Cindy Lu <lulu@redhat.com>
>>> ---
>>>    hw/net/virtio-net.c | 12 +++++++++++-
>>>    1 file changed, 11 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
>>> index cb0d27084c..7db9da1482 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,7 +152,11 @@ 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) {
>>> -            memcpy(config, &netcfg, n->config_size);
>>> +            if (memcmp(&netcfg.mac, &zero, sizeof(zero)) != 0) {
>>> +                memcpy(config, &netcfg, n->config_size);
>>> +        } else {
>>> +                error_report("Get an all zero mac address from hardware");
>>
>> This is probably a hint that MAC is not properly provisioned.
>>
>> So I guess we can leave this as is, or simply warn until the management
>> interface is finalized.
>>
> Hi Jason, For sure this is NIC card's problem, They cannot provide an
> correct MAC address,
> But if we continue use this 0 mac address will cause this traffic
> down, maybe will cost a lot of effort in debugging
> So I think maybe Just an warn is not enough, We can use the default
> mac address  and let the traffic working


Yes, and it's done by the following code.

But the question is there's no much value for the error here consider 
you've already had a solution.

Thanks


>>> +            }
>>>            }
>>>        }
>>>    }
>>> @@ -3399,6 +3404,11 @@ static void virtio_net_device_realize(DeviceState *dev, Error **errp)
>>>        nc = qemu_get_queue(n->nic);
>>>        nc->rxfilter_notify_enabled = 1;
>>>
>>> +   if (nc->peer && nc->peer->info->type == NET_CLIENT_DRIVER_VHOST_VDPA) {
>>> +        struct virtio_net_config netcfg = {};
>>> +        memcpy(&netcfg.mac, &n->nic_conf.macaddr, ETH_ALEN);
>>> +        virtio_net_set_config(vdev, (uint8_t *)&netcfg);
>>
>> Won't this overwrite all other fields in the netcfg? I think we should
>> only touch mac part.
>>
>> Thanks
>>
>>
> Sure, will fix this
>>> +    }
>>>        QTAILQ_INIT(&n->rsc_chains);
>>>        n->qdev = dev;
>>>
>

Re: [PATCH 1/3] virtio-net: Set mac address to hardware if the peer is vdpa

[Cindy Lu]

On Thu, Sep 24, 2020 at 3:18 PM Jason Wang <jasowang@redhat.com> wrote:
>
>
> On 2020/9/22 上午11:01, Cindy Lu wrote:
> > On Tue, Sep 22, 2020 at 9:55 AM Jason Wang <jasowang@redhat.com> wrote:
> >>
> >> On 2020/9/17 下午11:58, Cindy Lu wrote:
> >>> If the peer's type is vdpa,set the mac address to NIC in virtio_net_device_realize,
> >>> Also sometime vdpa get an all 0 macaddress from the hardware, this will cause the traffic down
> >>> So we add the check for this part.
> >>> if we get an 0 mac address we will use the default mac address instead
> >>>
> >>> Signed-off-by: Cindy Lu <lulu@redhat.com>
> >>> ---
> >>>    hw/net/virtio-net.c | 12 +++++++++++-
> >>>    1 file changed, 11 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> >>> index cb0d27084c..7db9da1482 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,7 +152,11 @@ 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) {
> >>> -            memcpy(config, &netcfg, n->config_size);
> >>> +            if (memcmp(&netcfg.mac, &zero, sizeof(zero)) != 0) {
> >>> +                memcpy(config, &netcfg, n->config_size);
> >>> +        } else {
> >>> +                error_report("Get an all zero mac address from hardware");
> >>
> >> This is probably a hint that MAC is not properly provisioned.
> >>
> >> So I guess we can leave this as is, or simply warn until the management
> >> interface is finalized.
> >>
> > Hi Jason, For sure this is NIC card's problem, They cannot provide an
> > correct MAC address,
> > But if we continue use this 0 mac address will cause this traffic
> > down, maybe will cost a lot of effort in debugging
> > So I think maybe Just an warn is not enough, We can use the default
> > mac address  and let the traffic working
>
>
> Yes, and it's done by the following code.
>
> But the question is there's no much value for the error here consider
> you've already had a solution.
>
> Thanks
>
Sure, Thanks Jason I will remove this
>
> >>> +            }
> >>>            }
> >>>        }
> >>>    }
> >>> @@ -3399,6 +3404,11 @@ static void virtio_net_device_realize(DeviceState *dev, Error **errp)
> >>>        nc = qemu_get_queue(n->nic);
> >>>        nc->rxfilter_notify_enabled = 1;
> >>>
> >>> +   if (nc->peer && nc->peer->info->type == NET_CLIENT_DRIVER_VHOST_VDPA) {
> >>> +        struct virtio_net_config netcfg = {};
> >>> +        memcpy(&netcfg.mac, &n->nic_conf.macaddr, ETH_ALEN);
> >>> +        virtio_net_set_config(vdev, (uint8_t *)&netcfg);
> >>
> >> Won't this overwrite all other fields in the netcfg? I think we should
> >> only touch mac part.
> >>
> >> Thanks
> >>
> >>
> > Sure, will fix this
> >>> +    }
> >>>        QTAILQ_INIT(&n->rsc_chains);
> >>>        n->qdev = dev;
> >>>
> >
>