Re: [dpdk-dev] [RFC v2 2/6] net/i40e: define the mirror filter parser

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

On 11/3/2020 8:28 AM, Steve Yang wrote:
> Define the sample filter parser for mirror, it will divide to two phases,
> the one is sample attributions pattern parsing, and the mirror config
> will be filled in according to pattern type VF/PF/VLAN when sample ratio
> is 1.
> The another is sample action parsing that the port id of mirror config
> will be filled in according to action type VF/PF.
> 
> Signed-off-by: Steve Yang <stevex.yang@intel.com>

<...>

> +#define GET_VLAN_ID_FROM_TCI(vlan_item, default_vid) \
> +	((vlan_item) ? ntohs(vlan_item->tci) & 0x0fff : (default_vid))
> +

'ntohs' usage is breaking the windows build (i40e is now build for windows) 
because of missing header, can you please prefer:
  I40E_NTOHS()

<...>

> +	if (attr->group) {
> +		rte_flow_error_set(error, EINVAL,
> +				   RTE_FLOW_ERROR_TYPE_ATTR_GROUP,
> +				   attr, "Not support group.");
> +		return -rte_errno;
> +	}
> +	if (attr->transfer) {
> +		rte_flow_error_set(error, EINVAL,
> +				   RTE_FLOW_ERROR_TYPE_ATTR_TRANSFER,
> +				   attr, "Not support group.");

copy/paste error in the error msg.

<...>

> +static int
> +i40e_flow_parse_sample_action(struct rte_eth_dev *dev,
> +			      const struct rte_flow_action *actions,
> +			      struct rte_flow_error *error,
> +			      union i40e_filter_t *filter)
> +{
> +	struct i40e_pf *pf = I40E_DEV_PRIVATE_TO_PF(dev->data->dev_private);
> +	const struct rte_flow_action *act;
> +	struct i40e_mirror_rule_conf *mirror_config =	&filter->mirror_conf;
> +	uint16_t *dst_vsi_seid = &mirror_config->dst_vsi_seid;
> +	uint32_t index = 0;
> +
> +	NEXT_ITEM_OF_ACTION(act, actions, index);
> +	if (act->type != RTE_FLOW_ACTION_TYPE_SAMPLE) {
> +		rte_flow_error_set(error, EINVAL,
> +			RTE_FLOW_ERROR_TYPE_ACTION, act,
> +			"Not supported action.");
> +		return -rte_errno;
> +	}
> +

Is above check required, isn't this function called only if the action type is 
'sample'

> +	if (((const struct rte_flow_action_sample *)act->conf)->ratio != 1) {
> +		rte_flow_error_set(error, EINVAL,
> +				RTE_FLOW_ERROR_TYPE_ACTION_CONF, act,
> +				"Invalid ratio for mirror action");

So, only "ratio == 1" is supported, better to highlight this in the error message.

> +			return -rte_errno;
> +	}
> +
> +	index++;
> +	NEXT_ITEM_OF_ACTION(act, actions, index);
> +
> +	if (act->type == RTE_FLOW_ACTION_TYPE_PORT_ID) {
> +		const struct rte_flow_action_port_id *conf =
> +			(const struct rte_flow_action_port_id *)act->conf;
> +
> +		if (!conf->id) {
> +			*dst_vsi_seid = pf->main_vsi_seid;
> +		} else if (conf->id <= pf->vf_num) {
> +			*dst_vsi_seid = pf->vfs[conf->id - 1].vsi->seid;
> +		} else {
> +			rte_flow_error_set(error, EINVAL,
> +				RTE_FLOW_ERROR_TYPE_ACTION_CONF, act,
> +				"Invalid port id mirror action");
> +			return -rte_errno;
> +		}
> +	}
> +
> +	/* Check if the next non-void item is END */
> +	index++;
> +	NEXT_ITEM_OF_ACTION(act, actions, index);
> +	if (act->type != RTE_FLOW_ACTION_TYPE_END) {
> +		rte_flow_error_set(error, EINVAL,
> +			RTE_FLOW_ERROR_TYPE_ACTION, act,
> +			"Only support pf or vf parameter item.");

So, only 'PORT_ID' is supported.

Can you please add comment on both "i40e_flow_parse_sample_attr_pattern" & 
"i40e_flow_parse_sample_action" to document these restrictions, or expectation? 
It both helps to understand what to expect without diving deep into the code and 
documents intention which can help finding any possible coding error.