diff mbox series

virtio-net: Add check for mac address while peer is vdpa

Message ID 20210224073333.16035-1-lulu@redhat.com (mailing list archive)
State New, archived
Headers show
Series virtio-net: Add check for mac address while peer is vdpa | expand

Commit Message

Cindy Lu Feb. 24, 2021, 7:33 a.m. UTC
Sometime vdpa get an all zero mac address from the hardware, this will cause the
traffic down, So we add the check for in part.if we get an zero mac address we will
use the default/cmdline mac address instead

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

Comments

Michael S. Tsirkin Feb. 24, 2021, 7:59 a.m. UTC | #1
> [PATCH] virtio-net: Add check for mac address while peer is vdpa
please do keep numbering patch versions.


On Wed, Feb 24, 2021 at 03:33:33PM +0800, Cindy Lu wrote:
> Sometime vdpa get an all zero mac address from the hardware, this will cause the
> traffic down, So we add the check for in part.if we get an zero mac address we will
> use the default/cmdline mac address instead

How does this differ from v4 of same patch?

Lots of typos above. I guess I can just rewrite the whole log ...

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

Below needs a huge code comment explaining what is going on.

E.g.  if we get a valid mac from peer, use it.

If we get 0 instead
then we don't know the peer mac but since the
mac is 0 and that is not a legal value,
try to proceed assume something else (management, hardware) sets up the mac
correctly and consistently with the qemu configuration.


> +            if (memcmp(&netcfg.mac, &zero, sizeof(zero)) != 0) {

!= 0 is not necessary

> +                memcpy(config, &netcfg, n->config_size);

> +        } else {
> +                error_report("Get an all zero mac address from hardware");

So why are you skipping the copying of the whole config space? Why not
just skip the mac? Seems quite risky e.g. looks like it will break
things like reporting the link status, right?

> +            }
>          }
>      }
>  }
> -- 
> 2.21.3
Cindy Lu Feb. 25, 2021, 4:43 p.m. UTC | #2
sure, Thanks michael, I will address, these comment and send a new version

On Wed, Feb 24, 2021 at 3:59 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> > [PATCH] virtio-net: Add check for mac address while peer is vdpa
> please do keep numbering patch versions.
>
>
> On Wed, Feb 24, 2021 at 03:33:33PM +0800, Cindy Lu wrote:
> > Sometime vdpa get an all zero mac address from the hardware, this will cause the
> > traffic down, So we add the check for in part.if we get an zero mac address we will
> > use the default/cmdline mac address instead
>
> How does this differ from v4 of same patch?
>
> Lots of typos above. I guess I can just rewrite the whole log ...
>
> >
> > 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);
>
> Below needs a huge code comment explaining what is going on.
>
> E.g.  if we get a valid mac from peer, use it.
>
> If we get 0 instead
> then we don't know the peer mac but since the
> mac is 0 and that is not a legal value,
> try to proceed assume something else (management, hardware) sets up the mac
> correctly and consistently with the qemu configuration.
>
>
> > +            if (memcmp(&netcfg.mac, &zero, sizeof(zero)) != 0) {
>
> != 0 is not necessary
>
> > +                memcpy(config, &netcfg, n->config_size);
>
> > +        } else {
> > +                error_report("Get an all zero mac address from hardware");
>
> So why are you skipping the copying of the whole config space? Why not
> just skip the mac? Seems quite risky e.g. looks like it will break
> things like reporting the link status, right?
>
> > +            }
> >          }
> >      }
> >  }
> > --
> > 2.21.3
>
diff mbox series

Patch

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");
+            }
         }
     }
 }