| Message ID | 20200917155851.20636-1-lulu@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3] virtio-net: Set mac address to hardware if the peer is vdpa | expand |
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; >
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; > > >
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; >>> >
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; > >>> > > >
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;
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(-)