Messages in this thread | | | Date | Fri, 27 Jan 2023 13:28:01 +0100 | Subject | Re: [PATCH v2 1/1] virtio_net: notify MAC address change on device initialization | From | Laurent Vivier <> |
| |
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@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 >
| |