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

["Xia, Chenbo" <chenbo.xia@intel.com>]

Hi Maxime,

> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Tuesday, June 8, 2021 10:14 PM
> To: dev@dpdk.org; Xia, Chenbo <chenbo.xia@intel.com>; amorenoz@redhat.com;
> david.marchand@redhat.com
> Cc: Maxime Coquelin <maxime.coquelin@redhat.com>
> Subject: [PATCH 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.
> 
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> ---
>  .../net/virtio/virtio_user/virtio_user_dev.c  | 85 ++++++++++++++++---
>  .../net/virtio/virtio_user/virtio_user_dev.h  |  2 +
>  drivers/net/virtio/virtio_user_ethdev.c       |  7 +-
>  3 files changed, 81 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 ed55cd7524..5c9f142024 100644
> --- a/drivers/net/virtio/virtio_user/virtio_user_dev.c
> +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c
> @@ -260,21 +260,84 @@ int virtio_user_stop_device(struct virtio_user_dev *dev)
>  	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);

No need to add '\n'. And same for below 'PMD_DRV_LOG'

Thanks,
Chenbo

> +
> +	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);
> +
> +	return ret;
> +}
> +
> +static void
> +virtio_user_dev_init_mac(struct virtio_user_dev *dev, const char *mac)
> +{
> +	struct rte_ether_addr cmdline_mac;
> +	char buf[RTE_ETHER_ADDR_FMT_SIZE];
> +	int ret;
> 
> -	if (rte_ether_unformat_addr(mac, &tmp) == 0) {
> -		memcpy(dev->mac_addr, &tmp, RTE_ETHER_ADDR_LEN);
> +	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;
> +
> +		/* 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)
> +			goto out;
> +
> +		if (memcmp(&cmdline_mac, dev->mac_addr, RTE_ETHER_ADDR_LEN))
> +			PMD_DRV_LOG(INFO, "(%s) Device MAC update failed", dev-
> >path);
>  	} else {
> -		/* ignore the wrong mac, use random mac */
> -		PMD_DRV_LOG(ERR, "wrong format of mac: %s", mac);
> +		ret = virtio_user_dev_get_mac(dev);
> +		if (ret) {
> +			PMD_DRV_LOG(ERR, "(%s) No valid MAC in devargs or device,
> use random",
> +					dev->path);
> +			return;
> +		}
> +
> +		dev->mac_specified = 1;
>  	}
> +out:
> +	rte_ether_format_addr(buf, RTE_ETHER_ADDR_FMT_SIZE,
> +			(struct rte_ether_addr *)dev->mac_addr);
> +	PMD_DRV_LOG(INFO, "(%s) MAC %s specified", dev->path, buf);
>  }
> 
>  static int
> @@ -509,8 +572,6 @@ virtio_user_dev_init(struct virtio_user_dev *dev, char
> *path, int queues,
>  	dev->unsupported_features = 0;
>  	dev->backend_type = backend_type;
> 
> -	parse_mac(dev, mac);
> -
>  	if (*ifname) {
>  		dev->ifname = *ifname;
>  		*ifname = NULL;
> @@ -538,6 +599,8 @@ virtio_user_dev_init(struct virtio_user_dev *dev, char
> *path, int queues,
>  		return -1;
>  	}
> 
> +	virtio_user_dev_init_mac(dev, mac);
> +
>  	if (!mrg_rxbuf)
>  		dev->unsupported_features |= (1ull << VIRTIO_NET_F_MRG_RXBUF);
> 
> diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.h
> b/drivers/net/virtio/virtio_user/virtio_user_dev.h
> index 58ad5198b6..819f6463ba 100644
> --- a/drivers/net/virtio/virtio_user/virtio_user_dev.h
> +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.h
> @@ -77,6 +77,8 @@ uint8_t virtio_user_handle_mq(struct virtio_user_dev *dev,
> uint16_t q_pairs);
>  int virtio_user_dev_set_status(struct virtio_user_dev *dev, uint8_t status);
>  int virtio_user_dev_update_status(struct virtio_user_dev *dev);
>  int virtio_user_dev_update_link_state(struct virtio_user_dev *dev);
> +int virtio_user_dev_set_mac(struct virtio_user_dev *dev);
> +int virtio_user_dev_get_mac(struct virtio_user_dev *dev);
>  void virtio_user_dev_delayed_disconnect_handler(void *param);
>  int virtio_user_dev_server_reconnect(struct virtio_user_dev *dev);
>  extern const char * const virtio_user_backend_strings[];
> diff --git a/drivers/net/virtio/virtio_user_ethdev.c
> b/drivers/net/virtio/virtio_user_ethdev.c
> index 3ecbb4184a..90fcd6e7cc 100644
> --- a/drivers/net/virtio/virtio_user_ethdev.c
> +++ b/drivers/net/virtio/virtio_user_ethdev.c
> @@ -60,12 +60,15 @@ virtio_user_write_dev_config(struct virtio_hw *hw, size_t
> offset,
>  	struct virtio_user_dev *dev = virtio_user_get_dev(hw);
> 
>  	if ((offset == offsetof(struct virtio_net_config, mac)) &&
> -	    (length == RTE_ETHER_ADDR_LEN))
> +	    (length == RTE_ETHER_ADDR_LEN)) {
>  		for (i = 0; i < RTE_ETHER_ADDR_LEN; ++i)
>  			dev->mac_addr[i] = ((const uint8_t *)src)[i];
> -	else
> +		virtio_user_dev_set_mac(dev);
> +		virtio_user_dev_get_mac(dev);
> +	} else {
>  		PMD_DRV_LOG(ERR, "not supported offset=%zu, len=%d",
>  			    offset, length);
> +	}
>  }
> 
>  static void
> --
> 2.31.1