Re: [dpdk-dev] [RFC 3/3] net/virtio: add MAC device config getter and setter

On 6/8/21 7:29 AM, Xia, Chenbo wrote:

Hi Maxime,

quoted

-----Original Message-----
From: Maxime Coquelin <redacted>
Sent: Thursday, June 3, 2021 10:29 PM
To: Xia, Chenbo <redacted>; Maxime Coquelin
[off-list ref]; dev@dpdk.org; amorenoz@redhat.com;
david.marchand@redhat.com
Subject: Re: [RFC 3/3] net/virtio: add MAC device config getter and setter

Hi Chenbo,

On 4/19/21 8:24 AM, Xia, Chenbo wrote:

quoted

Hi Maxime,

quoted

-----Original Message-----
From: Maxime Coquelin <redacted>
Sent: Friday, March 19, 2021 6:35 AM
To: dev@dpdk.org; Xia, Chenbo <redacted>; amorenoz@redhat.com;
david.marchand@redhat.com
Cc: Maxime Coquelin <redacted>
Subject: [RFC 3/3] net/virtio: add MAC device config getter and setter

This patch uses the new device config ops to get and set
the MAC address if supported.

If a valid MAC address is passed as devarg of the
Virtio-user PMD, the driver will try to store it in the
device config space. Otherwise the one provided in
the device config space will be used, if available.

I agree with the MAC selection strategy you proposed.

quoted

Signed-off-by: Maxime Coquelin <redacted>
---
 .../net/virtio/virtio_user/virtio_user_dev.c  | 78 ++++++++++++++++---
 .../net/virtio/virtio_user/virtio_user_dev.h  |  2 +
 drivers/net/virtio/virtio_user_ethdev.c       |  7 +-
 3 files changed, 74 insertions(+), 13 deletions(-)

diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c

b/drivers/net/virtio/virtio_user/virtio_user_dev.c
index 8757a23f6e..61517692b3 100644

--- a/drivers/net/virtio/virtio_user/virtio_user_dev.c
+++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c

@@ -259,20 +259,76 @@ int virtio_user_stop_device(struct virtio_user_dev

*dev)

quoted

quoted

 	return -1;
 }

-static inline void
-parse_mac(struct virtio_user_dev *dev, const char *mac)
+int
+virtio_user_dev_set_mac(struct virtio_user_dev *dev)
 {
-	struct rte_ether_addr tmp;
+	int ret = 0;

-	if (!mac)
-		return;
+	if (!(dev->device_features & (1ULL << VIRTIO_NET_F_MAC)))
+		return -ENOTSUP;
+
+	if (!dev->ops->set_config)
+		return -ENOTSUP;
+
+	ret = dev->ops->set_config(dev, dev->mac_addr,
+			offsetof(struct virtio_net_config, mac),
+			RTE_ETHER_ADDR_LEN);
+	if (ret)
+		PMD_DRV_LOG(ERR, "(%s) Failed to set MAC address in device\n",
dev->path);
+
+	return ret;
+}
+
+int
+virtio_user_dev_get_mac(struct virtio_user_dev *dev)
+{
+	int ret = 0;
+
+	if (!(dev->device_features & (1ULL << VIRTIO_NET_F_MAC)))
+		return -ENOTSUP;
+
+	if (!dev->ops->get_config)
+		return -ENOTSUP;
+
+	ret = dev->ops->get_config(dev, dev->mac_addr,
+			offsetof(struct virtio_net_config, mac),
+			RTE_ETHER_ADDR_LEN);
+	if (ret)
+		PMD_DRV_LOG(ERR, "(%s) Failed to get MAC address from device\n",
dev->path);

-	if (rte_ether_unformat_addr(mac, &tmp) == 0) {
-		memcpy(dev->mac_addr, &tmp, RTE_ETHER_ADDR_LEN);
+	return ret;
+}
+
+static void
+virtio_user_dev_init_mac(struct virtio_user_dev *dev, const char *mac)
+{
+	struct rte_ether_addr cmdline_mac;
+	int ret;
+
+	if (mac && rte_ether_unformat_addr(mac, &cmdline_mac) == 0) {
+		/*
+		 * MAC address was passed from command-line, try to store
+		 * it in the device if it supports it. Otherwise try to use
+		 * the device one.
+		 */
+		memcpy(dev->mac_addr, &cmdline_mac, RTE_ETHER_ADDR_LEN);
 		dev->mac_specified = 1;

How do we define mac_specified? If I understand correctly, it means the mac
we see is from device (we set it or we just use device's). Then 'dev-
mac_specified = 1'
should be after get_mac succeeds.

You are correct, mac_specified=1 means either user or device specified
MAC address. If get_mac fails below then we use the user specified MAC
address, so mac_specified = 1 is still valid in this case.

quoted

Note that during virtio_user_dev_init, we also use
this val to set VIRTIO_NET_F_MAC. But here the val is set without making

sure the

quoted

feature exists.

I am not sure to get youre point, but it sets VIRTIO_NET_F_MAC in the
frontend features there, that does not mean the feature is negotiated in
the end.

I think you are correct, I may misunderstood something when I review this first
time. And I want to make sure we are on the same page: since 'mac_specified=1'
will set VIRTIO_NET_F_MAC in frontend_features, so only when user don't set mac
and we don't get mac in device will lead to this feature unsupported, right?

Yes, correct. The idea was to keep the old behaviour, i.e. support it
when user specifies the MAC in the devargs, and extend it to support the
feature when the device can provide the MAC address.

Regards,
Maxime

quoted

quoted

quoted

+
+		/* Setting MAC may fail, continue to get the device one in this
case */
+		virtio_user_dev_set_mac(dev);
+		ret = virtio_user_dev_get_mac(dev);
+		if (ret == -ENOTSUP)
+			return;
+
+		if (memcmp(&cmdline_mac, dev->mac_addr, RTE_ETHER_ADDR_LEN))
+			PMD_DRV_LOG(INFO, "(%s) Device MAC update failed\n", dev-

quoted

path);

Besides Adrian's comments, if we decide to return no error on this, it may

also

quoted

be good to add something like 'using random MAC' to tell users that the

driver will

quoted

use random mac. Adding here or in the function that generates mac is both ok.

If it fails here, it won't be using a random MAC, but the MAC provided
by the user. The log could be improved with something like:
"Device MAC update failed, using MAC xx:xx:xx:xx:xx"

Yeah! That's good.

Thanks,
Chenbo

quoted

What do you think?

quoted

The patchset overall looks good to me. I'm looking forward to v1 😊

Thanks,
Chenbo

Thanks,
Maxime