Messages in this thread | | | Date | Wed, 25 Jan 2023 09:13:49 +0200 | Subject | Re: [PATCH v2 1/1] virtio_net: notify MAC address change on device initialization | From | Eli Cohen <> |
| |
On 24/01/2023 13:04, 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?
If the MAC address is changed without letting mlx5_vdpa know, RX packets will be dropped since they won't go through the receive filters.
TX packets will go through unaffected.
> >> >>> 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. > >> 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. > >> >> >>> free_unregister_netdev: >>> -- >>> 2.39.0 >> > > Thanks, > Laurent >
| |