Message ID | 20230123120022.2364889-2-lvivier@redhat.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:adf:eb09:0:0:0:0:0 with SMTP id s9csp1568991wrn; Mon, 23 Jan 2023 04:06:36 -0800 (PST) X-Google-Smtp-Source: AMrXdXtaRTG7AeX3gLwkiFmWVLDYGQVwKFZTjjNS5z8282RPrurTS/GuPn8alOr4AifHcYfei+Iy X-Received: by 2002:a17:902:be09:b0:194:92a9:ced9 with SMTP id r9-20020a170902be0900b0019492a9ced9mr24984451pls.47.1674475595880; Mon, 23 Jan 2023 04:06:35 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1674475595; cv=none; d=google.com; s=arc-20160816; b=hGfyd6gkQs5k9JLcPAbEpow0yBNHc/r9Y6wf0gB6uVWFLfyt558nCbu19IeYEow2ZH /cWpa+YhFbXWxEG7q3IleV1EpzxffzaJsUzcnj9GnmSJB6slhXhuYqj1y/PK6fYWkXUS a8AQO/7/jgivtvnrI9Ewxycbwi3xk+9mpO68q7fbxOThvzaX4XElMTb/PEc20oGP6JWl o9WtD7Ldjf3+R0HdZxzue25QNkFjxXzEUXdJGpV/sI622XZGUiXVRpCcrlksZw632l/D FDn9z2eDhWUz0MSUex4iIMXZy/QEI/BtedvfAp7uBqUU0zRwwv2dwXgGm3ZZp9lWFm+C pQoQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:ui-outboundreport:content-transfer-encoding :mime-version:references:in-reply-to:message-id:date:subject:cc:to :from; bh=Ayo3Z/JHP7285EUCek7YM07ner8r6YwTfFuj7Vg1MuI=; b=M6CIVky1TH37hH9fTZHF6aBUURR9b2vW79XvQa1cvXvD4Ja3KEg7RkwroVak3SQkMt 3KsaBXy8HKlI+vbUtkX8aqsWtiNowmVHS/bZbJhfT9fA8iQ//WYjRqapRV5mlDQXcvHL eRShqHwL4p5ywhAXdcJFVkr80+hw4Fvnl33kj5EsMwo87Ti3LItKxBGymAUvf/A48mBQ QyPN0lAap4PODyFiyKgihiBy0oRemv+/wwt/tpLS3kBWoUburaJycIiEsn6ekMFJAYfr YD8jWROPKMR1RYDV4VrG4injSq5sZ6pq3xl3QnMpCZjTxINqJNpT0iXEQb8oC4ZJi0j5 slMA== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=redhat.com Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id f7-20020a170902ce8700b00194691a9e6csi34509847plg.360.2023.01.23.04.06.23; Mon, 23 Jan 2023 04:06:35 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=redhat.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231783AbjAWMB0 (ORCPT <rfc822;forouhar.linux@gmail.com> + 99 others); Mon, 23 Jan 2023 07:01:26 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:46656 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231671AbjAWMBZ (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Mon, 23 Jan 2023 07:01:25 -0500 Received: from mout.kundenserver.de (mout.kundenserver.de [212.227.126.131]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 2DEEF1167F; Mon, 23 Jan 2023 04:00:40 -0800 (PST) Received: from lenovo-t14s.redhat.com ([82.142.8.70]) by mrelayeu.kundenserver.de (mreue009 [212.227.15.167]) with ESMTPSA (Nemesis) id 1M8QNs-1pOJD73y9A-004VEn; Mon, 23 Jan 2023 13:00:26 +0100 From: Laurent Vivier <lvivier@redhat.com> To: linux-kernel@vger.kernel.org Cc: Gautam Dawar <gautam.dawar@xilinx.com>, =?utf-8?q?Eugenio_P=C3=A9rez?= <eperezma@redhat.com>, netdev@vger.kernel.org, virtualization@lists.linux-foundation.org, Eli Cohen <elic@nvidia.com>, Cindy Lu <lulu@redhat.com>, "Michael S. Tsirkin" <mst@redhat.com>, Jason Wang <jasowang@redhat.com>, "David S. Miller" <davem@davemloft.net>, Parav Pandit <parav@nvidia.com> Subject: [PATCH v2 1/1] virtio_net: notify MAC address change on device initialization Date: Mon, 23 Jan 2023 13:00:22 +0100 Message-Id: <20230123120022.2364889-2-lvivier@redhat.com> X-Mailer: git-send-email 2.39.0 In-Reply-To: <20230123120022.2364889-1-lvivier@redhat.com> References: <20230123120022.2364889-1-lvivier@redhat.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Provags-ID: V03:K1:U0OU6mhChXwxhUWbxTYRcj2Ph4Oul09dobjDl8aQ0FSqEgeTUXd jdosvrGb4AtJQz0Df17BbboemEU7njqjKZhWLwC+AKhJ+Fs88CSjBVU7WomS3OXXc4zLCaI nmCXrP/uY6BLKx2zrFGAG32xUfOoIJmbWzhQQ1uqMey/0SHyWlWdPe+TxbBil97AylNbX23 14GYG08545QkHihjYhKQg== UI-OutboundReport: notjunk:1;M01:P0:qEBdQkGMqCk=;GMabZS3iibKazuhA5ggR/ksGTDv lDJPpsnMhCK26MmckF6wd0wv4Wh6CCI2msPvKYVXOOWeG+i2fCQl11Gpiy9PYf6o19EsqvmSz fp09Eu2Z0NhlCMyA+9z/jgl8DY9MHi/FdQSSbjj6bOiUe+YeaNvLNVNMyOYFRqMcp14SoklZD YdIF1qCJBbD0cyguM6FM9N5tORMLZWqZ8xhEOHpAhcyoiANFONWHzJWy3SgfqwilWwEFNY3/C hYMUMxxY7sDBbheynrUF5YJ3CqwsBG9wnyOceg1vnn8TeBpW9/5RH7eajpth3Yq9MaHaPq1po xQaIDEhOXvQuC82zL7M4oNH3OaibP1sIr1sOUqn3fqkSX6WGs+8Lh6pDN48E2mA8fyettEMRO 7iWlcAwbBl68EaKSanOaSp9V4XhClGheziTw0+qHZlbEU0kR77fw13/ldAe4Ray6qua/MmSKy B42VUCnvywjSOJTXLkD3QxH7yOHvHsFAr1t28D5rd0+uMskwLT4+4LfAKCvaZDLXrga9PqhSl f1TVFYarEwJa5NVyUDbyl9DnQztSXfRSp+3kwivp2JosjxHIY9kfW7jMOz0OgX6hzHqotK+xY l8nwWIS9qymjaP/fV1O4rzxEwgzLnrLQeJDEL59rUk3ZZxgtLIUfpgNgerwh5w0phZC32h9hM m2UITY6L2AW4PwZex9hfT/giI82eTxQoOnooxK1rew== X-Spam-Status: No, score=-1.9 required=5.0 tests=BAYES_00,RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1755814922557193412?= X-GMAIL-MSGID: =?utf-8?q?1755814922557193412?= |
Series |
virtio_net: vdpa: update MAC address when it is generated by virtio-net
|
|
Commit Message
Laurent Vivier
Jan. 23, 2023, noon UTC
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@redhat.com>
---
drivers/net/virtio_net.c | 14 ++++++++++++++
1 file changed, 14 insertions(+)
Comments
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@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
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. > 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 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 >
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. > > 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
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 >
On Fri, Jan 27, 2023 at 01:28:01PM +0100, Laurent Vivier wrote: > 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 I would rather add these checks in virtnet_validate. And I think it's cleaner to just do __virtio_clear_bit on VIRTIO_NET_F_STANDBY rather than failing simply because we previously did not prohibit it so there could be devices like these out there. A spec patch saying VIRTIO_NET_F_STANDBY should also have VIRTIO_NET_F_MAC is also welcome. > > > > > > > > > > > > > > > free_unregister_netdev: > > > > > -- > > > > > 2.39.0 > > > > > > > > > > Thanks, > > > Laurent > >
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: