1. Patchew
  2. QEMU
  3. View series

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

Cindy Lu posted 1 patch 5 years ago
Test checkpatch passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20201023091559.4858-1-lulu@redhat.com
Maintainers: "Michael S. Tsirkin" <mst@redhat.com>, Jason Wang <jasowang@redhat.com>
There is a newer version of this series
hw/net/virtio-net.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
[PATCH v2] virtio-net: Add check for mac address while peer is vdpa
Posted by Cindy Lu 5 years ago 
Sometime vdpa get an all 0 mac address 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 | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 9179013ac4..f1648fc47d 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");
+            }
         }
     }
 }
-- 
2.21.3
Re: [PATCH v2] virtio-net: Add check for mac address while peer is vdpa
Posted by Jason Wang 5 years ago 
On 2020/10/23 下午5:15, Cindy Lu wrote:
> Sometime vdpa get an all 0 mac address 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 | 7 ++++++-
>   1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index 9179013ac4..f1648fc47d 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");
> +            }
>           }
>       }
>   }


Applied.

Thanks
Re: [PATCH v2] virtio-net: Add check for mac address while peer is vdpa
Posted by Jason Wang 5 years ago 
On 2020/10/26 上午10:43, Jason Wang wrote:
>
> On 2020/10/23 下午5:15, Cindy Lu wrote:
>> Sometime vdpa get an all 0 mac address 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 | 7 ++++++-
>>   1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
>> index 9179013ac4..f1648fc47d 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");
>> +            }
>>           }
>>       }
>>   }
>
>
> Applied.
>
> Thanks
>
>

Speak too fast. Some questions:

1) Any reason that you do such check or get_config() instead of doing it 
once in device initalization
2) the indentation looks wrong
3) There's no need for an error here since we can workaround it

Thanks
Re: [PATCH v2] virtio-net: Add check for mac address while peer is vdpa
Posted by Cindy Lu 5 years ago 
Hi jason


On Mon, Oct 26, 2020 at 11:20 AM Jason Wang <jasowang@redhat.com> wrote:

>
> On 2020/10/26 上午10:43, Jason Wang wrote:
> >
> > On 2020/10/23 下午5:15, Cindy Lu wrote:
> >> Sometime vdpa get an all 0 mac address 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 | 7 ++++++-
> >>   1 file changed, 6 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> >> index 9179013ac4..f1648fc47d 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");
> >> +            }
> >>           }
> >>       }
> >>   }
> >
> >
> > Applied.
> >
> > Thanks
> >
> >
>
> Speak too fast. Some questions:
>
> 1) Any reason that you do such check or get_config() instead of doing it
> once in device initalization
>
get_config()  was called before the device realized.  If we check in the
device initalization, the mac address is already overwritten to 0 and
we lost the default mac address
Also for my understanding. the mac address read from hardware should never
been 0, so we need to check it every time we got it, Just in case it will
change to 0 and overwrite the mac address while the qemu running or some
other case

> 2) the indentation looks wrong
>
sure will fix this

> 3) There's no need for an error here since we can workaround it

sure, I will change it to log

>
>


> Thanks
>
>
Re: [PATCH v2] virtio-net: Add check for mac address while peer is vdpa
Posted by Jason Wang 5 years ago 
----- Original Message -----
> Hi jason
> 
> 
> On Mon, Oct 26, 2020 at 11:20 AM Jason Wang <jasowang@redhat.com> wrote:
> 
> >
> > On 2020/10/26 上午10:43, Jason Wang wrote:
> > >
> > > On 2020/10/23 下午5:15, Cindy Lu wrote:
> > >> Sometime vdpa get an all 0 mac address 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 | 7 ++++++-
> > >>   1 file changed, 6 insertions(+), 1 deletion(-)
> > >>
> > >> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> > >> index 9179013ac4..f1648fc47d 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");
> > >> +            }
> > >>           }
> > >>       }
> > >>   }
> > >
> > >
> > > Applied.
> > >
> > > Thanks
> > >
> > >
> >
> > Speak too fast. Some questions:
> >
> > 1) Any reason that you do such check or get_config() instead of doing it
> > once in device initalization
> >
> get_config()  was called before the device realized.

Intersting, any reason that we need call get_config() even before
device is realized?

> If we check in the
> device initalization, the mac address is already overwritten to 0 and
> we lost the default mac address
> Also for my understanding. the mac address read from hardware should never
> been 0, so we need to check it every time we got it,

This sounds self-contradict.

> Just in case it will
> change to 0 and overwrite the mac address while the qemu running or some
> other case

This looks like a bug in the device or driver.

Even if this is true, you can only detect it via get_config(). And I
don't think we need to workaround a buggy device.

Thanks

> 
> > 2) the indentation looks wrong
> >
> sure will fix this
> 
> > 3) There's no need for an error here since we can workaround it
> 
> sure, I will change it to log
> 
> >
> >
> 
> 
> > Thanks
> >
> >
>

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.