Michael S. Tsirkin
2023-Jan-27 11:08 UTC
[PATCH v2 1/1] virtio_net: notify MAC address change on device initialization
On Tue, Jan 24, 2023 at 12:04:24PM +0100, Laurent Vivier wrote:> On 1/24/23 11:15, Michael S. Tsirkin wrote: > > On Mon, Jan 23, 2023 at 01:00:22PM +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. > > > > And then what exactly happens? Does hardware drop the outgoing > > or the incoming packets? Pls include in the commit log. > > I don't know. There is nothing in the kernel logs. > > The ping error is: "Destination Host Unreachable" > > I found the problem with the mlx5 driver as in "it doesn't work when MAC > address is not set"... > > Perhaps Eli can explain what happens when the MAC address is not set? > > > > > > Signed-off-by: Laurent Vivier <lvivier at redhat.com> > > > --- > > > drivers/net/virtio_net.c | 14 ++++++++++++++ > > > 1 file changed, 14 insertions(+) > > > > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > > > index 7723b2a49d8e..4bdc8286678b 100644 > > > --- a/drivers/net/virtio_net.c > > > +++ b/drivers/net/virtio_net.c > > > @@ -3800,6 +3800,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 */ > > > @@ -3956,6 +3958,18 @@ static int virtnet_probe(struct virtio_device *vdev) > > > pr_debug("virtnet: registered device %s with %d RX and TX vq's\n", > > > dev->name, max_queue_pairs); > > > + /* a random MAC address has been assigned, notify the device */ > > > + if (!virtio_has_feature(vdev, VIRTIO_NET_F_MAC) && > > > + virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_MAC_ADDR)) { > > > > Maybe add a comment explaining that we don't fail probe if > > VIRTIO_NET_F_CTRL_MAC_ADDR is not there because > > many devices work fine without getting MAC explicitly. > > OK > > > > > > + 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)) { > > > + dev_warn(&vdev->dev, "Failed to update MAC address.\n"); > > > > Here, I'm not sure we want to proceed. Is it useful sometimes? > > I think reporting an error is always useful, but I can remove that if you prefer.No the question was whether we should fail probe not whether we print the warning.> > I note that we deny with virtnet_set_mac_address. > > > > > + } > > > + } > > > + > > > return 0; > > > > > > > > Also, some code duplication with virtnet_set_mac_address here. > > > > Also: > > When using the legacy interface, \field{mac} is driver-writable > > which provided a way for drivers to update the MAC without > > negotiating VIRTIO_NET_F_CTRL_MAC_ADDR. > > > > How about factoring out code in virtnet_set_mac_address > > and reusing that? > > > > In fact, we can write in the field only if we have VIRTIO_NET_F_MAC > (according to virtnet_set_mac_address(), and this code is executed only if > we do not have VIRTIO_NET_F_MAC. So I think it's better not factoring the > code as we have only the control queue case to manage. > > > This will also handle corner cases such as VIRTIO_NET_F_STANDBY > > which are not currently addressed. > > F_STANDBY is only enabled when virtio-net device MAC address is equal to the > VFIO device MAC address, I don't think it can be enabled when the MAC > address is randomly assigned (in this case it has already failed in > net_failover_create(), as it has been called using the random mac address), > it's why I didn't check for it.But the spec did not say there's a dependency :(. My point is what should we do if there's F_STANDBY but no MAC? Maybe add a separate patch clearing F_STANDBY in this case?> > > > > > > free_unregister_netdev: > > > -- > > > 2.39.0 > > > > Thanks, > Laurent
Laurent Vivier
2023-Jan-27 12:28 UTC
[PATCH v2 1/1] virtio_net: notify MAC address change on device initialization
On 1/27/23 12:08, Michael S. Tsirkin wrote:> On Tue, Jan 24, 2023 at 12:04:24PM +0100, Laurent Vivier wrote: >> On 1/24/23 11:15, Michael S. Tsirkin wrote: >>> On Mon, Jan 23, 2023 at 01:00:22PM +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. >>> >>> And then what exactly happens? Does hardware drop the outgoing >>> or the incoming packets? Pls include in the commit log. >> >> I don't know. There is nothing in the kernel logs. >> >> The ping error is: "Destination Host Unreachable" >> >> I found the problem with the mlx5 driver as in "it doesn't work when MAC >> address is not set"... >> >> Perhaps Eli can explain what happens when the MAC address is not set? >> >>> >>>> Signed-off-by: Laurent Vivier <lvivier at redhat.com> >>>> --- >>>> drivers/net/virtio_net.c | 14 ++++++++++++++ >>>> 1 file changed, 14 insertions(+) >>>> >>>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c >>>> index 7723b2a49d8e..4bdc8286678b 100644 >>>> --- a/drivers/net/virtio_net.c >>>> +++ b/drivers/net/virtio_net.c >>>> @@ -3800,6 +3800,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 */ >>>> @@ -3956,6 +3958,18 @@ static int virtnet_probe(struct virtio_device *vdev) >>>> pr_debug("virtnet: registered device %s with %d RX and TX vq's\n", >>>> dev->name, max_queue_pairs); >>>> + /* a random MAC address has been assigned, notify the device */ >>>> + if (!virtio_has_feature(vdev, VIRTIO_NET_F_MAC) && >>>> + virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_MAC_ADDR)) { >>> >>> Maybe add a comment explaining that we don't fail probe if >>> VIRTIO_NET_F_CTRL_MAC_ADDR is not there because >>> many devices work fine without getting MAC explicitly. >> >> OK >> >>> >>>> + 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)) { >>>> + dev_warn(&vdev->dev, "Failed to update MAC address.\n"); >>> >>> Here, I'm not sure we want to proceed. Is it useful sometimes? >> >> I think reporting an error is always useful, but I can remove that if you prefer. > > No the question was whether we should fail probe not > whether we print the warning.Good question. After all, as VIRTIO_NET_F_CTRL_MAC_ADDR is set, if VIRTIO_NET_CTRL_MAC_ADDR_SET fails it means there is a real problem, so yes, we should fail.> > >>> I note that we deny with virtnet_set_mac_address. >>> >>>> + } >>>> + } >>>> + >>>> return 0; >>> >>> >>> >>> Also, some code duplication with virtnet_set_mac_address here. >>> >>> Also: >>> When using the legacy interface, \field{mac} is driver-writable >>> which provided a way for drivers to update the MAC without >>> negotiating VIRTIO_NET_F_CTRL_MAC_ADDR. >>> >>> How about factoring out code in virtnet_set_mac_address >>> and reusing that? >>> >> >> In fact, we can write in the field only if we have VIRTIO_NET_F_MAC >> (according to virtnet_set_mac_address(), and this code is executed only if >> we do not have VIRTIO_NET_F_MAC. So I think it's better not factoring the >> code as we have only the control queue case to manage. >> >>> This will also handle corner cases such as VIRTIO_NET_F_STANDBY >>> which are not currently addressed. >> >> F_STANDBY is only enabled when virtio-net device MAC address is equal to the >> VFIO device MAC address, I don't think it can be enabled when the MAC >> address is randomly assigned (in this case it has already failed in >> net_failover_create(), as it has been called using the random mac address), >> it's why I didn't check for it. > > But the spec did not say there's a dependency :(. > My point is what should we do if there's F_STANDBY but no MAC? > Maybe add a separate patch clearing F_STANDBY in this case?The simplest would be to add at the beginning of the probe function: if (!virtio_has_feature(vdev, VIRTIO_NET_F_MAC) && virtio_has_feature(vdev, VIRTIO_NET_F_STANDBY)) { pr_err("virtio-net: a standby device cannot be used without a MAC address"); return -EOPNOTSUPP; } And I think it would help a lot to debug misconfiguration of the interface. Thanks, Laurent> >>> >>> >>>> free_unregister_netdev: >>>> -- >>>> 2.39.0 >>> >> >> Thanks, >> Laurent >