Re: [dpdk-dev] [PATCH v2 1/4] net/i40e: implement dynamic mapping of sw flow types to hw pctypes

[Ferruh Yigit <ferruh.yigit@intel.com>]

On 9/1/2017 4:02 PM, Kirill Rybalchenko wrote:
> Implement dynamic mapping of software flow types to hardware pctypes.
> This allows to add new flow types and pctypes for DDP without changing
> API of the driver. The mapping table is located in private
> data area for particular network adapter and can be individually
> modified with set of appropriate functions.
> 
> Signed-off-by: Kirill Rybalchenko <kirill.rybalchenko@intel.com>
> ---
> v2
> Re-arrange patchset to avoid compillation errors.
> Remove usage of statically defined flow types and pctypes.
<...>

> +	for (i = 0; i < I40E_FLOW_TYPE_MAX; i++) {
> +		if (flags & (1ULL << i))
> +			hena |= adapter->pcypes_tbl[i];

s/pcypes_tbl/pctypes_tbl/ ?

<...>

> @@ -6668,16 +6601,9 @@ static void
>  i40e_pf_disable_rss(struct i40e_pf *pf)
>  {
>  	struct i40e_hw *hw = I40E_PF_TO_HW(pf);
> -	uint64_t hena;
>  
> -	hena = (uint64_t)i40e_read_rx_ctl(hw, I40E_PFQF_HENA(0));
> -	hena |= ((uint64_t)i40e_read_rx_ctl(hw, I40E_PFQF_HENA(1))) << 32;
> -	if (hw->mac.type == I40E_MAC_X722)
> -		hena &= ~I40E_RSS_HENA_ALL_X722;
> -	else
> -		hena &= ~I40E_RSS_HENA_ALL;
> -	i40e_write_rx_ctl(hw, I40E_PFQF_HENA(0), (uint32_t)hena);
> -	i40e_write_rx_ctl(hw, I40E_PFQF_HENA(1), (uint32_t)(hena >> 32));

Above logic keeps intact bits of I40E_PFQF_HENA register that are not
supported RSS has filter. But below new code just sets all to zero.

I don't know why above prefer this logic, but would you mind separating
this decision into own its patch? I mean in this patch keep this as it
is, and make new patch to switch new register values.

> +	i40e_write_rx_ctl(hw, I40E_PFQF_HENA(0), 0);
> +	i40e_write_rx_ctl(hw, I40E_PFQF_HENA(1), 0);
>  	I40E_WRITE_FLUSH(hw);
>  }
>  
> @@ -6749,23 +6675,14 @@ static int
>  i40e_hw_rss_hash_set(struct i40e_pf *pf, struct rte_eth_rss_conf *rss_conf)
>  {
>  	struct i40e_hw *hw = I40E_PF_TO_HW(pf);
> -	uint64_t rss_hf;
>  	uint64_t hena;
>  	int ret;
>  
> -	ret = i40e_set_rss_key(pf->main_vsi, rss_conf->rss_key,
> -			       rss_conf->rss_key_len);
> +	ret = i40e_set_rss_key(pf->main_vsi, rss_conf->rss_key, rss_conf->rss_key_len);

I think there is no change here, and original code is correct as syntax,
please keep it unchanged.

>  	if (ret)
>  		return ret;
>  
> -	rss_hf = rss_conf->rss_hf;
> -	hena = (uint64_t)i40e_read_rx_ctl(hw, I40E_PFQF_HENA(0));
> -	hena |= ((uint64_t)i40e_read_rx_ctl(hw, I40E_PFQF_HENA(1))) << 32;
> -	if (hw->mac.type == I40E_MAC_X722)
> -		hena &= ~I40E_RSS_HENA_ALL_X722;
> -	else
> -		hena &= ~I40E_RSS_HENA_ALL;
> -	hena |= i40e_config_hena(rss_hf, hw->mac.type);

Same register comment as above one.

> +	hena = i40e_config_hena(rss_conf->rss_hf, pf->adapter);
>  	i40e_write_rx_ctl(hw, I40E_PFQF_HENA(0), (uint32_t)hena);
>  	i40e_write_rx_ctl(hw, I40E_PFQF_HENA(1), (uint32_t)(hena >> 32));
>  	I40E_WRITE_FLUSH(hw);

<...>

> @@ -7820,29 +7736,31 @@ i40e_get_hash_filter_global_config(struct i40e_hw *hw,
>  	PMD_DRV_LOG(DEBUG, "Hash function is %s",
>  		(reg & I40E_GLQF_CTL_HTOEP_MASK) ? "Toeplitz" : "Simple XOR");
>  
> -	for (i = 0; mask && i < RTE_ETH_FLOW_MAX; i++) {
> -		if (!(mask & (1UL << i)))
> -			continue;
> -		mask &= ~(1UL << i);
> -		/* Bit set indicats the coresponding flow type is supported */
> -		g_cfg->valid_bit_mask[0] |= (1UL << i);
> -		/* if flowtype is invalid, continue */
> -		if (!I40E_VALID_FLOW(i))
> -			continue;
> -		pctype = i40e_flowtype_to_pctype(i);
> -		reg = i40e_read_rx_ctl(hw, I40E_GLQF_HSYM(pctype));
> -		if (reg & I40E_GLQF_HSYM_SYMH_ENA_MASK)
> -			g_cfg->sym_hash_enable_mask[0] |= (1UL << i);
> +	g_cfg->valid_bit_mask[0] = (uint32_t)adapter->flow_types_msk;

Loosing data here by casting to 32bit, intentional?

> +
> +	for (i = 0; i < UINT32_BIT; i++) {

Why use UINT32_BIT? not I40E_FLOW_TYPE_MAX? Is it because
sym_hash_enable_mask is 32bits? Why not increase its stroge if we need
64 flow type / pctype ?

> +		if (adapter->pcypes_tbl[i]) {
> +			for (j = 0; j < I40E_PCTYPE_MAX; j++) {
> +				if (adapter->pcypes_tbl[i] & (1ULL << j)) {
> +					reg = i40e_read_rx_ctl(hw, I40E_GLQF_HSYM(j));
> +					if (reg & I40E_GLQF_HSYM_SYMH_ENA_MASK) {
> +						g_cfg->sym_hash_enable_mask[0] |=
> +									(1UL << i);
> +					}
> +				}
> +			}
> +		}
>  	}
>  
>  	return 0;
>  }

<...>

> @@ -7885,64 +7803,26 @@ static int
>  i40e_set_hash_filter_global_config(struct i40e_hw *hw,
>  				   struct rte_eth_hash_global_conf *g_cfg)
>  {
> +	struct i40e_adapter *adapter = (struct i40e_adapter *)hw->back;
>  	int ret;
> -	uint16_t i;
> +	uint16_t i, j;
>  	uint32_t reg;
> -	uint32_t mask0 = g_cfg->valid_bit_mask[0];
> -	enum i40e_filter_pctype pctype;
> +	uint32_t mask0 = g_cfg->valid_bit_mask[0] & (uint32_t)adapter->flow_types_msk;

Loosing data here by casting to 32bit, intentional?


<...>

> @@ -8636,7 +8513,8 @@ i40e_hash_filter_inset_select(struct i40e_hw *hw,
>  		return -EINVAL;
>  	}
>  
> -	if (!I40E_VALID_FLOW(conf->flow_type)) {
> +	pctype = i40e_flowtype_to_pctype(conf->flow_type, pf->adapter);
> +	if (!pctype) {

pctype 0 is not defined in enum, which technically possible to be
defined in the future and break this logic, it could be good to define
it now to something UNKNOWN, RESERVED etc.. to be sure it won't be used.

>  		PMD_DRV_LOG(ERR, "invalid flow_type input.");
>  		return -EINVAL;
>  	}

<...>

> @@ -8655,11 +8531,7 @@ i40e_hash_filter_inset_select(struct i40e_hw *hw,
>  		PMD_DRV_LOG(ERR, "Failed to parse input set");
>  		return -EINVAL;
>  	}
> -	if (i40e_validate_input_set(pctype, RTE_ETH_FILTER_HASH,
> -				    input_set) != 0) {
> -		PMD_DRV_LOG(ERR, "Invalid input set");
> -		return -EINVAL;
> -	}

Why removing this validation?

> +
>  	if (conf->op == RTE_ETH_INPUT_SET_ADD) {
>  		/* get inset value in register */
>  		inset_reg = i40e_read_rx_ctl(hw, I40E_GLQF_HASH_INSET(1, pctype));
> @@ -8713,24 +8585,19 @@ i40e_fdir_filter_inset_select(struct i40e_pf *pf,
>  		return -EINVAL;
>  	}
>  
> -	if (!I40E_VALID_FLOW(conf->flow_type)) {
> +	pctype = i40e_flowtype_to_pctype(conf->flow_type, pf->adapter);
> +
> +	if (!pctype) {
>  		PMD_DRV_LOG(ERR, "invalid flow_type input.");
>  		return -EINVAL;
>  	}
>  
> -	pctype = i40e_flowtype_to_pctype(conf->flow_type);
> -
>  	ret = i40e_parse_input_set(&input_set, pctype, conf->field,
>  				   conf->inset_size);
>  	if (ret) {
>  		PMD_DRV_LOG(ERR, "Failed to parse input set");
>  		return -EINVAL;
>  	}
> -	if (i40e_validate_input_set(pctype, RTE_ETH_FILTER_FDIR,
> -				    input_set) != 0) {
> -		PMD_DRV_LOG(ERR, "Invalid input set");
> -		return -EINVAL;
> -	}

And why removing this part?

>  
>  	/* get inset value in register */
>  	inset_reg = i40e_read_rx_ctl(hw, I40E_PRTQF_FD_INSET(pctype, 1));
> @@ -9169,72 +9036,34 @@ i40e_hw_init(struct rte_eth_dev *dev)
>  	i40e_set_symmetric_hash_enable_per_port(hw, 0);
>  }
>  
> -enum i40e_filter_pctype
> -i40e_flowtype_to_pctype(uint16_t flow_type)
<...>
> +uint16_t

Why changed return type from "enum i40e_filter_pctype" to "uint16_t",
this function should be returning pctype?

> +i40e_flowtype_to_pctype(uint16_t flow_type, struct i40e_adapter *adapter)
> +{
> +	int i;
> +	uint64_t pctype_mask;
>  
> -	return pctype_table[flow_type];
> +	if (flow_type < I40E_FLOW_TYPE_MAX) {
> +		pctype_mask = adapter->pcypes_tbl[flow_type];
> +		for (i = I40E_PCTYPE_MAX - 1; i >= 0; i--) {
> +			if (pctype_mask & (1ULL << i))

pctype_mask can be having multiple pctype, why returning at first one,
this may be breaking some I40E_MAC_X722 code.

> +				return (uint16_t)i;

if you intend to cast "i" to uint16_t why not define it uint16_t at
first place.

If return type is changed to uint16_t to OR pctype values, that logic is
missing here and caller of the function should take this into account,
and function should be commented to clarify this. If this functions
intented to return single pctype as original code, it should return
"enum i40e_filter_pctype" I think.

> +		}
> +	}
> +	return 0;
>  }
>  

<...>

> +	uint16_t flowtype = RTE_ETH_FLOW_UNKNOWN;
> +	uint64_t pctype_mask = 1ULL << pctype;
> +
> +	for (; flowtype < I40E_FLOW_TYPE_MAX; flowtype++) {
> +		if (adapter->pcypes_tbl[flowtype] & pctype_mask)
> +			return flowtype;
> +	}
>  
> -	return flowtype_table[pctype];
> +	return flowtype;

If pctype is not in the table, this will return I40E_FLOW_TYPE_MAX,
which is wrong.


<...>

> @@ -881,6 +883,10 @@ struct i40e_adapter {
>  
>  	/* ptype mapping table */
>  	uint32_t ptype_tbl[I40E_MAX_PKT_TYPE] __rte_cache_min_aligned;
> +	/* flow type to pctype mapping table */

It can be good to note, not sure where, this is 1:N mapping, one flow
table can has multiple pctypes, bitmasked into pctype variable in the table.

> +	uint64_t pcypes_tbl[I40E_FLOW_TYPE_MAX] __rte_cache_min_aligned;

I guess there is a typo in variable name, is intention "pctypes_tbl" ?

> +	uint64_t flow_types_msk> +	uint64_t pctypes_msk;

abbrevation is only gaining one letter, "a", I would convert this to
"flow_types_mask" and "pctypes_mask".

>  };
>  
>  extern const struct rte_flow_ops i40e_flow_ops;
> @@ -925,8 +931,8 @@ int i40e_vsi_vlan_pvid_set(struct i40e_vsi *vsi,
>  			   struct i40e_vsi_vlan_pvid_info *info);
>  int i40e_vsi_config_vlan_stripping(struct i40e_vsi *vsi, bool on);
>  int i40e_vsi_config_vlan_filter(struct i40e_vsi *vsi, bool on);
> -uint64_t i40e_config_hena(uint64_t flags, enum i40e_mac_type type);
> -uint64_t i40e_parse_hena(uint64_t flags);
> +uint64_t i40e_config_hena(uint64_t flags, struct i40e_adapter *adapter);
> +uint64_t i40e_parse_hena(uint64_t flags, struct i40e_adapter *adapter);
>  enum i40e_status_code i40e_fdir_setup_tx_resources(struct i40e_pf *pf);
>  enum i40e_status_code i40e_fdir_setup_rx_resources(struct i40e_pf *pf);
>  int i40e_fdir_setup(struct i40e_pf *pf);
> @@ -935,8 +941,8 @@ const struct rte_memzone *i40e_memzone_reserve(const char *name,
>  					int socket_id);
>  int i40e_fdir_configure(struct rte_eth_dev *dev);
>  void i40e_fdir_teardown(struct i40e_pf *pf);
> -enum i40e_filter_pctype i40e_flowtype_to_pctype(uint16_t flow_type);
> -uint16_t i40e_pctype_to_flowtype(enum i40e_filter_pctype pctype);
> +uint16_t i40e_flowtype_to_pctype(uint16_t flow_type, struct i40e_adapter *adapter);
> +uint16_t i40e_pctype_to_flowtype(uint16_t pctype, struct i40e_adapter *adapter);
>  int i40e_fdir_ctrl_func(struct rte_eth_dev *dev,
>  			  enum rte_filter_op filter_op,
>  			  void *arg);
> diff --git a/drivers/net/i40e/i40e_ethdev_vf.c b/drivers/net/i40e/i40e_ethdev_vf.c

Same above comments for VF code.

<...>