Re: [dpdk-dev] [RFC 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: Friday, March 19, 2021 6:35 AM
> 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: [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.

> 
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> ---
>  .../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)
>  	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. 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
feature exists.

> +
> +		/* 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-
> >path);

Besides Adrian's comments, if we decide to return no error on this, it may also
be good to add something like 'using random MAC' to tell users that the driver will
use random mac. Adding here or in the function that generates mac is both ok.

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

Thanks,
Chenbo

>  	} 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\n",
> +					dev->path);
> +		else
> +			dev->mac_specified = 1;
>  	}
>  }
> 
> @@ -508,8 +564,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;
> @@ -537,6 +591,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 8a62f7ea79..03bcf95970 100644
> --- a/drivers/net/virtio/virtio_user/virtio_user_dev.h
> +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.h
> @@ -78,6 +78,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_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 bb36316186..34c8e7a9b5 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.30.2