Virtualization - Jan 2023 - [PATCH v2 1/1] virtio_net: notify MAC address change on device initialization

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.

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)) {
+		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");
+		}
+	}
+
 	return 0;
 
 free_unregister_netdev:
-- 
2.39.0

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.
> 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.
> +		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 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?


This will also handle corner cases such as VIRTIO_NET_F_STANDBY
which are not currently addressed.

>  free_unregister_netdev:
> -- 
> 2.39.0