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

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

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