| Message ID | 20230127204500.51930-3-lvivier@redhat.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 9f62d221a4b0aa6a8d2a18053a0ca349c025297c |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | virtio_net: vdpa: update MAC address when it is generated by virtio-net | expand |
On Fri, 2023-01-27 at 21:45 +0100, Laurent Vivier wrote: > In virtnet_probe(), if the device doesn't provide a MAC address the > driver assigns a random one. > As we modify the MAC address we need to notify the device to allow it > to update all the related information. > > The problem can be seen with vDPA and mlx5_vdpa driver as it doesn't > assign a MAC address by default. The virtio_net device uses a random > MAC address (we can see it with "ip link"), but we can't ping a net > namespace from another one using the virtio-vdpa device because the > new MAC address has not been provided to the hardware: > RX packets are dropped since they don't go through the receive filters, > TX packets go through unaffected. > > Signed-off-by: Laurent Vivier <lvivier@redhat.com> > --- > drivers/net/virtio_net.c | 20 ++++++++++++++++++++ > 1 file changed, 20 insertions(+) > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > index 7d700f8e545a..704a05f1c279 100644 > --- a/drivers/net/virtio_net.c > +++ b/drivers/net/virtio_net.c > @@ -3806,6 +3806,8 @@ static int virtnet_probe(struct virtio_device *vdev) > eth_hw_addr_set(dev, addr); > } else { > eth_hw_addr_random(dev); > + dev_info(&vdev->dev, "Assigned random MAC address %pM\n", > + dev->dev_addr); > } > > /* Set up our device-specific information */ > @@ -3933,6 +3935,24 @@ static int virtnet_probe(struct virtio_device *vdev) > > virtio_device_ready(vdev); > > + /* a random MAC address has been assigned, notify the device. > + * We don't fail probe if VIRTIO_NET_F_CTRL_MAC_ADDR is not there > + * because many devices work fine without getting MAC explicitly > + */ > + if (!virtio_has_feature(vdev, VIRTIO_NET_F_MAC) && > + virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_MAC_ADDR)) { > + struct scatterlist sg; > + > + sg_init_one(&sg, dev->dev_addr, dev->addr_len); > + if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_MAC, > + VIRTIO_NET_CTRL_MAC_ADDR_SET, &sg)) { > + pr_debug("virtio_net: setting MAC address failed\n"); > + rtnl_unlock(); > + err = -EINVAL; > + goto free_unregister_netdev; Since the above is still dealing with device initialization, would it make sense moving such init step before registering the netdevice? Cheers, Paolo
On 1/31/23 10:01, Paolo Abeni wrote: > On Fri, 2023-01-27 at 21:45 +0100, Laurent Vivier wrote: >> In virtnet_probe(), if the device doesn't provide a MAC address the >> driver assigns a random one. >> As we modify the MAC address we need to notify the device to allow it >> to update all the related information. >> >> The problem can be seen with vDPA and mlx5_vdpa driver as it doesn't >> assign a MAC address by default. The virtio_net device uses a random >> MAC address (we can see it with "ip link"), but we can't ping a net >> namespace from another one using the virtio-vdpa device because the >> new MAC address has not been provided to the hardware: >> RX packets are dropped since they don't go through the receive filters, >> TX packets go through unaffected. >> >> Signed-off-by: Laurent Vivier <lvivier@redhat.com> >> --- >> drivers/net/virtio_net.c | 20 ++++++++++++++++++++ >> 1 file changed, 20 insertions(+) >> >> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c >> index 7d700f8e545a..704a05f1c279 100644 >> --- a/drivers/net/virtio_net.c >> +++ b/drivers/net/virtio_net.c >> @@ -3806,6 +3806,8 @@ static int virtnet_probe(struct virtio_device *vdev) >> eth_hw_addr_set(dev, addr); >> } else { >> eth_hw_addr_random(dev); >> + dev_info(&vdev->dev, "Assigned random MAC address %pM\n", >> + dev->dev_addr); >> } >> >> /* Set up our device-specific information */ >> @@ -3933,6 +3935,24 @@ static int virtnet_probe(struct virtio_device *vdev) >> >> virtio_device_ready(vdev); >> >> + /* a random MAC address has been assigned, notify the device. >> + * We don't fail probe if VIRTIO_NET_F_CTRL_MAC_ADDR is not there >> + * because many devices work fine without getting MAC explicitly >> + */ >> + if (!virtio_has_feature(vdev, VIRTIO_NET_F_MAC) && >> + virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_MAC_ADDR)) { >> + struct scatterlist sg; >> + >> + sg_init_one(&sg, dev->dev_addr, dev->addr_len); >> + if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_MAC, >> + VIRTIO_NET_CTRL_MAC_ADDR_SET, &sg)) { >> + pr_debug("virtio_net: setting MAC address failed\n"); >> + rtnl_unlock(); >> + err = -EINVAL; >> + goto free_unregister_netdev; > > Since the above is still dealing with device initialization, would it > make sense moving such init step before registering the netdevice? It depends if we can send the command using the control command queue or not. I don't think we can use a vq before virtio_device_ready(). Thanks, Laurent
On Tue, 2023-01-31 at 10:32 +0100, Laurent Vivier wrote: > On 1/31/23 10:01, Paolo Abeni wrote: > > On Fri, 2023-01-27 at 21:45 +0100, Laurent Vivier wrote: > > > In virtnet_probe(), if the device doesn't provide a MAC address the > > > driver assigns a random one. > > > As we modify the MAC address we need to notify the device to allow it > > > to update all the related information. > > > > > > The problem can be seen with vDPA and mlx5_vdpa driver as it doesn't > > > assign a MAC address by default. The virtio_net device uses a random > > > MAC address (we can see it with "ip link"), but we can't ping a net > > > namespace from another one using the virtio-vdpa device because the > > > new MAC address has not been provided to the hardware: > > > RX packets are dropped since they don't go through the receive filters, > > > TX packets go through unaffected. > > > > > > Signed-off-by: Laurent Vivier <lvivier@redhat.com> > > > --- > > > drivers/net/virtio_net.c | 20 ++++++++++++++++++++ > > > 1 file changed, 20 insertions(+) > > > > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > > > index 7d700f8e545a..704a05f1c279 100644 > > > --- a/drivers/net/virtio_net.c > > > +++ b/drivers/net/virtio_net.c > > > @@ -3806,6 +3806,8 @@ static int virtnet_probe(struct virtio_device *vdev) > > > eth_hw_addr_set(dev, addr); > > > } else { > > > eth_hw_addr_random(dev); > > > + dev_info(&vdev->dev, "Assigned random MAC address %pM\n", > > > + dev->dev_addr); > > > } > > > > > > /* Set up our device-specific information */ > > > @@ -3933,6 +3935,24 @@ static int virtnet_probe(struct virtio_device *vdev) > > > > > > virtio_device_ready(vdev); > > > > > > + /* a random MAC address has been assigned, notify the device. > > > + * We don't fail probe if VIRTIO_NET_F_CTRL_MAC_ADDR is not there > > > + * because many devices work fine without getting MAC explicitly > > > + */ > > > + if (!virtio_has_feature(vdev, VIRTIO_NET_F_MAC) && > > > + virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_MAC_ADDR)) { > > > + struct scatterlist sg; > > > + > > > + sg_init_one(&sg, dev->dev_addr, dev->addr_len); > > > + if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_MAC, > > > + VIRTIO_NET_CTRL_MAC_ADDR_SET, &sg)) { > > > + pr_debug("virtio_net: setting MAC address failed\n"); > > > + rtnl_unlock(); > > > + err = -EINVAL; > > > + goto free_unregister_netdev; > > > > Since the above is still dealing with device initialization, would it > > make sense moving such init step before registering the netdevice? > > It depends if we can send the command using the control command queue or not. > I don't think we can use a vq before virtio_device_ready(). Sounds reasonable. @Michael: do you have any additional comments? Thanks! Paolo
On Tue, Jan 31, 2023 at 10:01:53AM +0100, Paolo Abeni wrote: > On Fri, 2023-01-27 at 21:45 +0100, Laurent Vivier wrote: > > In virtnet_probe(), if the device doesn't provide a MAC address the > > driver assigns a random one. > > As we modify the MAC address we need to notify the device to allow it > > to update all the related information. > > > > The problem can be seen with vDPA and mlx5_vdpa driver as it doesn't > > assign a MAC address by default. The virtio_net device uses a random > > MAC address (we can see it with "ip link"), but we can't ping a net > > namespace from another one using the virtio-vdpa device because the > > new MAC address has not been provided to the hardware: > > RX packets are dropped since they don't go through the receive filters, > > TX packets go through unaffected. > > > > Signed-off-by: Laurent Vivier <lvivier@redhat.com> > > --- > > drivers/net/virtio_net.c | 20 ++++++++++++++++++++ > > 1 file changed, 20 insertions(+) > > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > > index 7d700f8e545a..704a05f1c279 100644 > > --- a/drivers/net/virtio_net.c > > +++ b/drivers/net/virtio_net.c > > @@ -3806,6 +3806,8 @@ static int virtnet_probe(struct virtio_device *vdev) > > eth_hw_addr_set(dev, addr); > > } else { > > eth_hw_addr_random(dev); > > + dev_info(&vdev->dev, "Assigned random MAC address %pM\n", > > + dev->dev_addr); > > } > > > > /* Set up our device-specific information */ > > @@ -3933,6 +3935,24 @@ static int virtnet_probe(struct virtio_device *vdev) > > > > virtio_device_ready(vdev); > > > > + /* a random MAC address has been assigned, notify the device. > > + * We don't fail probe if VIRTIO_NET_F_CTRL_MAC_ADDR is not there > > + * because many devices work fine without getting MAC explicitly > > + */ > > + if (!virtio_has_feature(vdev, VIRTIO_NET_F_MAC) && > > + virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_MAC_ADDR)) { > > + struct scatterlist sg; > > + > > + sg_init_one(&sg, dev->dev_addr, dev->addr_len); > > + if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_MAC, > > + VIRTIO_NET_CTRL_MAC_ADDR_SET, &sg)) { > > + pr_debug("virtio_net: setting MAC address failed\n"); > > + rtnl_unlock(); > > + err = -EINVAL; > > + goto free_unregister_netdev; > > Since the above is still dealing with device initialization, would it > make sense moving such init step before registering the netdevice? > > Cheers, > > Paolo We can't really, device has to be ready otherwise we can't send commands to it.
On Tue, 2023-01-31 at 08:43 -0500, Michael S. Tsirkin wrote: > On Tue, Jan 31, 2023 at 10:01:53AM +0100, Paolo Abeni wrote: > > On Fri, 2023-01-27 at 21:45 +0100, Laurent Vivier wrote: > > > In virtnet_probe(), if the device doesn't provide a MAC address the > > > driver assigns a random one. > > > As we modify the MAC address we need to notify the device to allow it > > > to update all the related information. > > > > > > The problem can be seen with vDPA and mlx5_vdpa driver as it doesn't > > > assign a MAC address by default. The virtio_net device uses a random > > > MAC address (we can see it with "ip link"), but we can't ping a net > > > namespace from another one using the virtio-vdpa device because the > > > new MAC address has not been provided to the hardware: > > > RX packets are dropped since they don't go through the receive filters, > > > TX packets go through unaffected. > > > > > > Signed-off-by: Laurent Vivier <lvivier@redhat.com> > > > --- > > > drivers/net/virtio_net.c | 20 ++++++++++++++++++++ > > > 1 file changed, 20 insertions(+) > > > > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > > > index 7d700f8e545a..704a05f1c279 100644 > > > --- a/drivers/net/virtio_net.c > > > +++ b/drivers/net/virtio_net.c > > > @@ -3806,6 +3806,8 @@ static int virtnet_probe(struct virtio_device *vdev) > > > eth_hw_addr_set(dev, addr); > > > } else { > > > eth_hw_addr_random(dev); > > > + dev_info(&vdev->dev, "Assigned random MAC address %pM\n", > > > + dev->dev_addr); > > > } > > > > > > /* Set up our device-specific information */ > > > @@ -3933,6 +3935,24 @@ static int virtnet_probe(struct virtio_device *vdev) > > > > > > virtio_device_ready(vdev); > > > > > > + /* a random MAC address has been assigned, notify the device. > > > + * We don't fail probe if VIRTIO_NET_F_CTRL_MAC_ADDR is not there > > > + * because many devices work fine without getting MAC explicitly > > > + */ > > > + if (!virtio_has_feature(vdev, VIRTIO_NET_F_MAC) && > > > + virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_MAC_ADDR)) { > > > + struct scatterlist sg; > > > + > > > + sg_init_one(&sg, dev->dev_addr, dev->addr_len); > > > + if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_MAC, > > > + VIRTIO_NET_CTRL_MAC_ADDR_SET, &sg)) { > > > + pr_debug("virtio_net: setting MAC address failed\n"); > > > + rtnl_unlock(); > > > + err = -EINVAL; > > > + goto free_unregister_netdev; > > > > Since the above is still dealing with device initialization, would it > > make sense moving such init step before registering the netdevice? > > > > Cheers, > > > > Paolo > > We can't really, device has to be ready otherwise > we can't send commands to it. Thanks, I see. Also, Laurent already clarified the above. Should I read your comment as you are ok with the patches in the current form? Cheers, Paolo
On Fri, Jan 27, 2023 at 09:45:00PM +0100, Laurent Vivier wrote: > In virtnet_probe(), if the device doesn't provide a MAC address the > driver assigns a random one. > As we modify the MAC address we need to notify the device to allow it > to update all the related information. > > The problem can be seen with vDPA and mlx5_vdpa driver as it doesn't > assign a MAC address by default. The virtio_net device uses a random > MAC address (we can see it with "ip link"), but we can't ping a net > namespace from another one using the virtio-vdpa device because the > new MAC address has not been provided to the hardware: > RX packets are dropped since they don't go through the receive filters, > TX packets go through unaffected. > > Signed-off-by: Laurent Vivier <lvivier@redhat.com> Acked-by: Michael S. Tsirkin <mst@redhat.com> > --- > drivers/net/virtio_net.c | 20 ++++++++++++++++++++ > 1 file changed, 20 insertions(+) > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > index 7d700f8e545a..704a05f1c279 100644 > --- a/drivers/net/virtio_net.c > +++ b/drivers/net/virtio_net.c > @@ -3806,6 +3806,8 @@ static int virtnet_probe(struct virtio_device *vdev) > eth_hw_addr_set(dev, addr); > } else { > eth_hw_addr_random(dev); > + dev_info(&vdev->dev, "Assigned random MAC address %pM\n", > + dev->dev_addr); > } > > /* Set up our device-specific information */ > @@ -3933,6 +3935,24 @@ static int virtnet_probe(struct virtio_device *vdev) > > virtio_device_ready(vdev); > > + /* a random MAC address has been assigned, notify the device. > + * We don't fail probe if VIRTIO_NET_F_CTRL_MAC_ADDR is not there > + * because many devices work fine without getting MAC explicitly > + */ > + if (!virtio_has_feature(vdev, VIRTIO_NET_F_MAC) && > + virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_MAC_ADDR)) { > + struct scatterlist sg; > + > + sg_init_one(&sg, dev->dev_addr, dev->addr_len); > + if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_MAC, > + VIRTIO_NET_CTRL_MAC_ADDR_SET, &sg)) { > + pr_debug("virtio_net: setting MAC address failed\n"); > + rtnl_unlock(); > + err = -EINVAL; > + goto free_unregister_netdev; > + } > + } > + > rtnl_unlock(); > > err = virtnet_cpu_notif_add(vi); > -- > 2.39.1
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c index 7d700f8e545a..704a05f1c279 100644 --- a/drivers/net/virtio_net.c +++ b/drivers/net/virtio_net.c @@ -3806,6 +3806,8 @@ static int virtnet_probe(struct virtio_device *vdev) eth_hw_addr_set(dev, addr); } else { eth_hw_addr_random(dev); + dev_info(&vdev->dev, "Assigned random MAC address %pM\n", + dev->dev_addr); } /* Set up our device-specific information */ @@ -3933,6 +3935,24 @@ static int virtnet_probe(struct virtio_device *vdev) virtio_device_ready(vdev); + /* a random MAC address has been assigned, notify the device. + * We don't fail probe if VIRTIO_NET_F_CTRL_MAC_ADDR is not there + * because many devices work fine without getting MAC explicitly + */ + if (!virtio_has_feature(vdev, VIRTIO_NET_F_MAC) && + virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_MAC_ADDR)) { + struct scatterlist sg; + + sg_init_one(&sg, dev->dev_addr, dev->addr_len); + if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_MAC, + VIRTIO_NET_CTRL_MAC_ADDR_SET, &sg)) { + pr_debug("virtio_net: setting MAC address failed\n"); + rtnl_unlock(); + err = -EINVAL; + goto free_unregister_netdev; + } + } + rtnl_unlock(); err = virtnet_cpu_notif_add(vi);
In virtnet_probe(), if the device doesn't provide a MAC address the driver assigns a random one. As we modify the MAC address we need to notify the device to allow it to update all the related information. The problem can be seen with vDPA and mlx5_vdpa driver as it doesn't assign a MAC address by default. The virtio_net device uses a random MAC address (we can see it with "ip link"), but we can't ping a net namespace from another one using the virtio-vdpa device because the new MAC address has not been provided to the hardware: RX packets are dropped since they don't go through the receive filters, TX packets go through unaffected. Signed-off-by: Laurent Vivier <lvivier@redhat.com> --- drivers/net/virtio_net.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)