RE: [PATCH v3 2/6] common/idpf: add RSS set/get ops

["Wu, Jingjing" <jingjing.wu@intel.com>]

> +static int idpf_config_rss_hf(struct idpf_vport *vport, uint64_t rss_hf)
> +{
> +	uint64_t hena = 0, valid_rss_hf = 0;
According to the coding style, only the last variable on a line should be initialized.

> +	int ret = 0;
> +	uint16_t i;
> +
> +	/**
> +	 * RTE_ETH_RSS_IPV4 and RTE_ETH_RSS_IPV6 can be considered as 2
> +	 * generalizations of all other IPv4 and IPv6 RSS types.
> +	 */
> +	if (rss_hf & RTE_ETH_RSS_IPV4)
> +		rss_hf |= idpf_ipv4_rss;
> +
> +	if (rss_hf & RTE_ETH_RSS_IPV6)
> +		rss_hf |= idpf_ipv6_rss;
> +
> +	for (i = 0; i < RTE_DIM(idpf_map_hena_rss); i++) {
> +		uint64_t bit = BIT_ULL(i);
> +
> +		if (idpf_map_hena_rss[i] & rss_hf) {
> +			valid_rss_hf |= idpf_map_hena_rss[i];
> +			hena |= bit;
> +		}
> +	}
> +
> +	vport->rss_hf = hena;
> +
> +	ret = idpf_vc_set_rss_hash(vport);
> +	if (ret != 0) {
> +		PMD_DRV_LOG(WARNING,
> +			    "fail to set RSS offload types, ret: %d", ret);
> +		return ret;
> +	}
> +
> +	if (valid_rss_hf & idpf_ipv4_rss)
> +		valid_rss_hf |= rss_hf & RTE_ETH_RSS_IPV4;
> +
> +	if (valid_rss_hf & idpf_ipv6_rss)
> +		valid_rss_hf |= rss_hf & RTE_ETH_RSS_IPV6;
> +
> +	if (rss_hf & ~valid_rss_hf)
> +		PMD_DRV_LOG(WARNING, "Unsupported rss_hf 0x%" PRIx64,
> +			    rss_hf & ~valid_rss_hf);
It makes me a bit confused, valid_rss_hf is would be the sub of rss_hf according above assignment. Would it be possible to go here?
And if it is possible, why not set valid_rss_hf before calling vc command?

> +	vport->last_general_rss_hf = valid_rss_hf;
> +
> +	return ret;
> +}
> +
>  static int
>  idpf_init_rss(struct idpf_vport *vport)
>  {
> @@ -256,6 +357,204 @@ idpf_init_rss(struct idpf_vport *vport)
>  	return ret;
>  }
> 
> +static int
> +idpf_rss_reta_update(struct rte_eth_dev *dev,
> +		     struct rte_eth_rss_reta_entry64 *reta_conf,
> +		     uint16_t reta_size)
> +{
> +	struct idpf_vport *vport = dev->data->dev_private;
> +	struct idpf_adapter *adapter = vport->adapter;
> +	uint16_t idx, shift;
> +	uint32_t *lut;
> +	int ret = 0;
> +	uint16_t i;
> +
> +	if (adapter->caps.rss_caps == 0 || dev->data->nb_rx_queues == 0) {
> +		PMD_DRV_LOG(DEBUG, "RSS is not supported");
> +		return -ENOTSUP;
> +	}
> +
> +	if (reta_size != vport->rss_lut_size) {
> +		PMD_DRV_LOG(ERR, "The size of hash lookup table configured "
> +				 "(%d) doesn't match the number of hardware can "
> +				 "support (%d)",
> +			    reta_size, vport->rss_lut_size);
> +		return -EINVAL;
> +	}
> +
> +	/* It MUST use the current LUT size to get the RSS lookup table,
> +	 * otherwise if will fail with -100 error code.
> +	 */
> +	lut = rte_zmalloc(NULL, reta_size * sizeof(uint32_t), 0);
> +	if (!lut) {
> +		PMD_DRV_LOG(ERR, "No memory can be allocated");
> +		return -ENOMEM;
> +	}
> +	/* store the old lut table temporarily */
> +	rte_memcpy(lut, vport->rss_lut, reta_size * sizeof(uint32_t));
Stored the vport->rss_lut to lut? But you overwrite the lut below?

> +
> +	for (i = 0; i < reta_size; i++) {
> +		idx = i / RTE_ETH_RETA_GROUP_SIZE;
> +		shift = i % RTE_ETH_RETA_GROUP_SIZE;
> +		if (reta_conf[idx].mask & (1ULL << shift))
> +			lut[i] = reta_conf[idx].reta[shift];
> +	}
> +
> +	rte_memcpy(vport->rss_lut, lut, reta_size * sizeof(uint32_t));
> +	/* send virtchnl ops to configure RSS */
> +	ret = idpf_vc_set_rss_lut(vport);
> +	if (ret) {
> +		PMD_INIT_LOG(ERR, "Failed to configure RSS lut");
> +		goto out;
> +	}
> +out:
> +	rte_free(lut);
> +
> +	return ret;
> +}