Re: [dpdk-dev] [PATCH v5] net/iavf: fix hash configuration on i40e VF

["Xing, Beilei" <beilei.xing@intel.com>]

> -----Original Message-----
> From: Zhang, AlvinX <alvinx.zhang@intel.com>
> Sent: Thursday, April 22, 2021 1:08 PM
> To: Wu, Jingjing <jingjing.wu@intel.com>; Xing, Beilei <beilei.xing@intel.com>
> Cc: dev@dpdk.org; Zhang, AlvinX <alvinx.zhang@intel.com>; stable@dpdk.org
> Subject: [PATCH v5] net/iavf: fix hash configuration on i40e VF
> 
> In i40evf PMD, the VF directly accesses the hash enable registers to enable or
> disable hashing on ingress packets. When binding i40e VF to iavf, because the
> PF doesn't support VIRTCHNL_VF_OFFLOAD_ADV_RSS_PF capability.
> Therefore, the VF hashing cannot be enabled.

Better to descript what's the issue first and then give the root cause.

> 
> This patch adds support of hash configuration for i40e VF by sending
> VIRTCHNL_OP_SET_RSS_HENA message to the PF after checking that the PF
> does not support VIRTCHNL_VF_OFFLOAD_ADV_RSS_PF capability.
> 
> Fixes: c678299594a8 ("net/iavf: fix default RSS configuration")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Alvin Zhang <alvinx.zhang@intel.com>
> ---
> 
> v2: Update commit log.
> v3: Increase compatibility with old drivers.
> v4: Fix compile issues.
> v5: Rebase to 1b593b9c832e9b284cc59665fe662242a3fc1daf.
> ---
>  drivers/net/iavf/iavf.h        |   1 +
>  drivers/net/iavf/iavf_ethdev.c | 111
> ++++++++++++++++++++++++++++++++++++++++-
>  drivers/net/iavf/iavf_vchnl.c  |  45 ++++++++++++++---
>  3 files changed, 150 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/iavf/iavf.h b/drivers/net/iavf/iavf.h index
> d1ae5a3..1e73f01 100644
> --- a/drivers/net/iavf/iavf.h
> +++ b/drivers/net/iavf/iavf.h
> @@ -336,6 +336,7 @@ int iavf_fdir_check(struct iavf_adapter *adapter,
>  		struct iavf_fdir_conf *filter);
>  int iavf_add_del_rss_cfg(struct iavf_adapter *adapter,
>  			 struct virtchnl_rss_cfg *rss_cfg, bool add);
> +int iavf_get_hena_caps(struct iavf_adapter *adapter, uint64_t *caps);
>  int iavf_set_hena(struct iavf_adapter *adapter, uint64_t hena);  int
> iavf_rss_hash_set(struct iavf_adapter *ad, uint64_t rss_hf, bool add);  int
> iavf_add_del_mc_addr_list(struct iavf_adapter *adapter, diff --git
> a/drivers/net/iavf/iavf_ethdev.c b/drivers/net/iavf/iavf_ethdev.c index
> d523a06..97a2dc7 100644
> --- a/drivers/net/iavf/iavf_ethdev.c
> +++ b/drivers/net/iavf/iavf_ethdev.c
> @@ -246,6 +246,107 @@ struct rte_iavf_xstats_name_off {  }
> 
>  static int
> +iavf_config_rss_hf(struct iavf_adapter *adapter, uint64_t rss_hf) {
> +	static const uint64_t map_hena_rss[] = {
> +		/* IPv4 */
> +		[IAVF_FILTER_PCTYPE_NONF_UNICAST_IPV4_UDP] =
> +				ETH_RSS_NONFRAG_IPV4_UDP,
> +		[IAVF_FILTER_PCTYPE_NONF_MULTICAST_IPV4_UDP] =
> +				ETH_RSS_NONFRAG_IPV4_UDP,
> +		[IAVF_FILTER_PCTYPE_NONF_IPV4_UDP] =
> +				ETH_RSS_NONFRAG_IPV4_UDP,
> +		[IAVF_FILTER_PCTYPE_NONF_IPV4_TCP_SYN_NO_ACK] =
> +				ETH_RSS_NONFRAG_IPV4_TCP,
> +		[IAVF_FILTER_PCTYPE_NONF_IPV4_TCP] =
> +				ETH_RSS_NONFRAG_IPV4_TCP,
> +		[IAVF_FILTER_PCTYPE_NONF_IPV4_SCTP] =
> +				ETH_RSS_NONFRAG_IPV4_SCTP,
> +		[IAVF_FILTER_PCTYPE_NONF_IPV4_OTHER] =
> +				ETH_RSS_NONFRAG_IPV4_OTHER,
> +		[IAVF_FILTER_PCTYPE_FRAG_IPV4] = ETH_RSS_FRAG_IPV4,
> +
> +		/* IPv6 */
> +		[IAVF_FILTER_PCTYPE_NONF_UNICAST_IPV6_UDP] =
> +				ETH_RSS_NONFRAG_IPV6_UDP,
> +		[IAVF_FILTER_PCTYPE_NONF_MULTICAST_IPV6_UDP] =
> +				ETH_RSS_NONFRAG_IPV6_UDP,
> +		[IAVF_FILTER_PCTYPE_NONF_IPV6_UDP] =
> +				ETH_RSS_NONFRAG_IPV6_UDP,
> +		[IAVF_FILTER_PCTYPE_NONF_IPV6_TCP_SYN_NO_ACK] =
> +				ETH_RSS_NONFRAG_IPV6_TCP,
> +		[IAVF_FILTER_PCTYPE_NONF_IPV6_TCP] =
> +				ETH_RSS_NONFRAG_IPV6_TCP,
> +		[IAVF_FILTER_PCTYPE_NONF_IPV6_SCTP] =
> +				ETH_RSS_NONFRAG_IPV6_SCTP,
> +		[IAVF_FILTER_PCTYPE_NONF_IPV6_OTHER] =
> +				ETH_RSS_NONFRAG_IPV6_OTHER,
> +		[IAVF_FILTER_PCTYPE_FRAG_IPV6] = ETH_RSS_FRAG_IPV6,
> +
> +		/* L2 Payload */
> +		[IAVF_FILTER_PCTYPE_L2_PAYLOAD] = ETH_RSS_L2_PAYLOAD
> +	};
> +
> +	const uint64_t ipv4_rss = ETH_RSS_NONFRAG_IPV4_UDP |
> +				  ETH_RSS_NONFRAG_IPV4_TCP |
> +				  ETH_RSS_NONFRAG_IPV4_SCTP |
> +				  ETH_RSS_NONFRAG_IPV4_OTHER |
> +				  ETH_RSS_FRAG_IPV4;
> +
> +	const uint64_t ipv6_rss = ETH_RSS_NONFRAG_IPV6_UDP |
> +				  ETH_RSS_NONFRAG_IPV6_TCP |
> +				  ETH_RSS_NONFRAG_IPV6_SCTP |
> +				  ETH_RSS_NONFRAG_IPV6_OTHER |
> +				  ETH_RSS_FRAG_IPV6;
> +
> +	struct iavf_info *vf =  IAVF_DEV_PRIVATE_TO_VF(adapter);
> +	uint64_t caps = 0, hena = 0, valid_rss_hf = 0;
> +	uint32_t i;
> +	int ret;
> +
> +	ret = iavf_get_hena_caps(adapter, &caps);
> +	if (ret)
> +		return ret;
> +	/**
> +	 * ETH_RSS_IPV4 and ETH_RSS_IPV6 can be considered as 2
> +	 * generalizations of all other IPv4 and IPv6 RSS types.
> +	 */
> +	if (rss_hf & ETH_RSS_IPV4)
> +		rss_hf |= ipv4_rss;
> +
> +	if (rss_hf & ETH_RSS_IPV6)
> +		rss_hf |= ipv6_rss;
> +
> +	RTE_BUILD_BUG_ON(RTE_DIM(map_hena_rss) > sizeof(uint64_t) *
> CHAR_BIT);
> +
> +	for (i = 0; i < RTE_DIM(map_hena_rss); i++) {
> +		uint64_t bit = BIT_ULL(i);
> +
> +		if ((caps & bit) && (map_hena_rss[i] & rss_hf)) {
> +			valid_rss_hf |= map_hena_rss[i];
> +			hena |= bit;
> +		}
> +	}
> +
> +	ret = iavf_set_hena(adapter, hena);
> +	if (ret)
> +		return ret;
> +
> +	if (valid_rss_hf & ipv4_rss)
> +		valid_rss_hf |= rss_hf & ETH_RSS_IPV4;
> +
> +	if (valid_rss_hf & ipv6_rss)
> +		valid_rss_hf |= rss_hf & ETH_RSS_IPV6;
> +
> +	if (rss_hf & ~valid_rss_hf)
> +		PMD_DRV_LOG(WARNING, "Unsupported rss_hf 0x%" PRIx64,
> +			    rss_hf & ~valid_rss_hf);
> +
> +	vf->rss_hf = valid_rss_hf;
> +	return 0;
> +}
> +
> +static int
>  iavf_init_rss(struct iavf_adapter *adapter)  {
>  	struct iavf_info *vf =  IAVF_DEV_PRIVATE_TO_VF(adapter); @@ -301,6
> +402,10 @@ struct rte_iavf_xstats_name_off {
>  			PMD_DRV_LOG(ERR, "fail to set default RSS");
>  			return ret;
>  		}
> +	} else {
> +		ret = iavf_config_rss_hf(adapter, rss_conf->rss_hf);
> +		if (ret != -ENOTSUP)
> +			return ret;
>  	}
> 
>  	return 0;
> @@ -1282,6 +1387,10 @@ static int iavf_config_rx_queues_irqs(struct
> rte_eth_dev *dev,
>  			PMD_DRV_LOG(ERR, "fail to set new RSS");
>  			return ret;
>  		}
> +	} else {
> +		ret = iavf_config_rss_hf(adapter, rss_conf->rss_hf);
> +		if (ret != -ENOTSUP)
> +			return ret;
>  	}
> 
>  	return 0;
> @@ -2096,7 +2205,7 @@ static int iavf_parse_devargs(struct rte_eth_dev
> *dev)
>  	struct iavf_info *vf = IAVF_DEV_PRIVATE_TO_VF(adapter);
>  	int ret = 0;
> 
> -	if (vf->vf_res->vf_cap_flags & VIRTCHNL_VF_OFFLOAD_ADV_RSS_PF) {
> +	if (vf->vf_res->vf_cap_flags & VIRTCHNL_VF_OFFLOAD_RSS_PF) {
>  		/* Set hena = 0 to ask PF to cleanup all existing RSS. */
>  		ret = iavf_set_hena(adapter, 0);
>  		if (ret)
> diff --git a/drivers/net/iavf/iavf_vchnl.c b/drivers/net/iavf/iavf_vchnl.c index
> 45096cb..8f2df0a 100644
> --- a/drivers/net/iavf/iavf_vchnl.c
> +++ b/drivers/net/iavf/iavf_vchnl.c
> @@ -9,6 +9,7 @@
>  #include <unistd.h>
>  #include <stdarg.h>
>  #include <inttypes.h>
> +#include <assert.h>
>  #include <rte_byteorder.h>
>  #include <rte_common.h>
> 
> @@ -228,13 +229,19 @@
>  			rte_delay_ms(ASQ_DELAY_MS);
>  			/* If don't read msg or read sys event, continue */
>  		} while (i++ < MAX_TRY_TIMES);
> -		/* If there's no response is received, clear command */
> -		if (i >= MAX_TRY_TIMES  ||
> -		    vf->cmd_retval != VIRTCHNL_STATUS_SUCCESS) {
> -			err = -1;
> -			PMD_DRV_LOG(ERR, "No response or return failure
> (%d)"
> -				    " for cmd %d", vf->cmd_retval, args->ops);
> +
> +		if (i >= MAX_TRY_TIMES) {
> +			PMD_DRV_LOG(ERR, "No response for cmd %d", args-
> >ops);
>  			_clear_cmd(vf);
> +			err = -EIO;
> +		} else if (vf->cmd_retval ==
> +			   (uint32_t)VIRTCHNL_STATUS_ERR_NOT_SUPPORTED)
> {
> +			PMD_DRV_LOG(ERR, "Cmd %d not supported", args-
> >ops);
> +			err = -ENOTSUP;
> +		} else if (vf->cmd_retval != VIRTCHNL_STATUS_SUCCESS) {
> +			PMD_DRV_LOG(ERR, "Return failure %d for cmd %d",
> +				    vf->cmd_retval, args->ops);
> +			err = -EINVAL;

Seems it's not related to the issue, better to split them.

>  		}
>  		break;
>  	}
> @@ -1496,6 +1503,32 @@
>  }
> 
>  int
> +iavf_get_hena_caps(struct iavf_adapter *adapter, uint64_t *caps) {
> +	struct iavf_info *vf = IAVF_DEV_PRIVATE_TO_VF(adapter);
> +	struct iavf_cmd_info args;
> +	int err;
> +
> +	assert(caps);

Why need assert here? Can we remove it?

> +
> +	args.ops = VIRTCHNL_OP_GET_RSS_HENA_CAPS;
> +	args.in_args = NULL;
> +	args.in_args_size = 0;
> +	args.out_buffer = vf->aq_resp;
> +	args.out_size = IAVF_AQ_BUF_SZ;
> +
> +	err = iavf_execute_vf_cmd(adapter, &args);
> +	if (err) {
> +		PMD_DRV_LOG(ERR,
> +			    "Failed to execute command of
> OP_GET_RSS_HENA_CAPS");
> +		return err;
> +	}
> +
> +	*caps = ((struct virtchnl_rss_hena *)args.out_buffer)->hena;
> +	return 0;
> +}
> +
> +int
>  iavf_set_hena(struct iavf_adapter *adapter, uint64_t hena)  {
>  	struct iavf_info *vf = IAVF_DEV_PRIVATE_TO_VF(adapter);
> --
> 1.8.3.1