[dpdk-dev] [PATCH v2 07/17] net/i40e: add flow validate function

[Adrien Mazarguil]

Hi Beilei,

A few comments below.

On Tue, Dec 27, 2016 at 02:26:14PM +0800, Beilei Xing wrote:
> This patch adds i40e_flow_validation function to check if
> a flow is valid according to the flow pattern.
> i40e_parse_ethertype_filter is added first, it also gets
> the ethertype info.
> i40e_flow.c is added to handle all generic filter events.
> 
> Signed-off-by: Beilei Xing <beilei.xing at intel.com>
> ---
>  drivers/net/i40e/Makefile      |   1 +
>  drivers/net/i40e/i40e_ethdev.c |   5 +
>  drivers/net/i40e/i40e_ethdev.h |  20 ++
>  drivers/net/i40e/i40e_flow.c   | 431 +++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 457 insertions(+)
>  create mode 100644 drivers/net/i40e/i40e_flow.c
[...]
> diff --git a/drivers/net/i40e/i40e_flow.c b/drivers/net/i40e/i40e_flow.c
> new file mode 100644
> index 0000000..bf451ef
> --- /dev/null
> +++ b/drivers/net/i40e/i40e_flow.c
[...]
> +	if (ethertype_filter->queue >= pf->dev_data->nb_rx_queues) {
> +		rte_flow_error_set(error, EINVAL,
> +				   RTE_FLOW_ERROR_TYPE_ACTION,
> +				   NULL, "Invalid queue ID for"
> +				   " ethertype_filter.");

When setting an error type related to an existing object provided by the
application, you should set the related cause pointer to a non-NULL
value. In this particular case, retrieving the action object seems difficult
so it can remain that way, however there are many places in this series
where it can be done.

> +		return -EINVAL;

While this is perfectly valid, you could also return -rte_errno to avoid
duplicating EINVAL.

[...]
> +	}
> +	if (ethertype_filter->ether_type == ETHER_TYPE_IPv4 ||
> +	    ethertype_filter->ether_type == ETHER_TYPE_IPv6) {
> +		rte_flow_error_set(error, ENOTSUP,
> +				   RTE_FLOW_ERROR_TYPE_ITEM,
> +				   NULL, "Unsupported ether_type in"
> +				   " control packet filter.");
> +		return -ENOTSUP;
> +	}
> +	if (ethertype_filter->ether_type == ETHER_TYPE_VLAN)
> +		PMD_DRV_LOG(WARNING, "filter vlan ether_type in"
> +			    " first tag is not supported.");
> +
> +	return ret;
> +}
[...]
> +/* Parse attributes */
> +static int
> +i40e_parse_attr(const struct rte_flow_attr *attr,
> +		struct rte_flow_error *error)
> +{
> +	/* Must be input direction */
> +	if (!attr->ingress) {
> +		rte_flow_error_set(error, EINVAL,
> +				   RTE_FLOW_ERROR_TYPE_ATTR_INGRESS,
> +				   NULL, "Only support ingress.");

Regarding my previous comment, &attr could replace NULL here as well as in
subsequent calls to rte_flow_error_set().

> +		return -EINVAL;
> +	}
> +
> +	/* Not supported */
> +	if (attr->egress) {
> +		rte_flow_error_set(error, EINVAL,
> +				   RTE_FLOW_ERROR_TYPE_ATTR_EGRESS,
> +				   NULL, "Not support egress.");
> +		return -EINVAL;
> +	}
> +
> +	/* Not supported */
> +	if (attr->priority) {
> +		rte_flow_error_set(error, EINVAL,
> +				   RTE_FLOW_ERROR_TYPE_ATTR_PRIORITY,
> +				   NULL, "Not support priority.");
> +		return -EINVAL;
> +	}
> +
> +	/* Not supported */
> +	if (attr->group) {
> +		rte_flow_error_set(error, EINVAL,
> +				   RTE_FLOW_ERROR_TYPE_ATTR_GROUP,
> +				   NULL, "Not support group.");
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static int
> +i40e_parse_ethertype_pattern(const struct rte_flow_item *pattern,
> +			     struct rte_flow_error *error,
> +			     struct rte_eth_ethertype_filter *filter)
> +{
> +	const struct rte_flow_item *item = pattern;
> +	const struct rte_flow_item_eth *eth_spec;
> +	const struct rte_flow_item_eth *eth_mask;
> +	enum rte_flow_item_type item_type;
> +
> +	for (; item->type != RTE_FLOW_ITEM_TYPE_END; item++) {
> +		item_type = item->type;
> +		switch (item_type) {
> +		case RTE_FLOW_ITEM_TYPE_ETH:
> +			eth_spec = (const struct rte_flow_item_eth *)item->spec;
> +			eth_mask = (const struct rte_flow_item_eth *)item->mask;
> +			/* Get the MAC info. */
> +			if (!eth_spec || !eth_mask) {
> +				rte_flow_error_set(error, EINVAL,
> +						   RTE_FLOW_ERROR_TYPE_ITEM,
> +						   NULL,
> +						   "NULL ETH spec/mask");
> +				return -EINVAL;
> +			}

While optional, I think you should allow eth_spec and eth_mask to be NULL
here as described in [1]:

- If eth_spec is NULL, you can match anything that looks like a valid
  Ethernet header.

- If eth_mask is NULL, you should assume a default mask (for Ethernet it
  usually means matching source/destination MACs perfectly).

- You must check the "last" field as well, if non-NULL it may probably be
  supported as long as the following condition is satisfied:

   (spec & mask) == (last & mask)

[1] http://dpdk.org/doc/guides/prog_guide/rte_flow.html#pattern-item

[...]
> +	const struct rte_flow_action_queue *act_q;
> +	uint32_t index = 0;
> +
> +	/* Check if the first non-void action is QUEUE or DROP. */
> +	NEXT_ITEM_OF_ACTION(act, actions, index);
> +	if (act->type != RTE_FLOW_ACTION_TYPE_QUEUE &&
> +	    act->type != RTE_FLOW_ACTION_TYPE_DROP) {
> +		rte_flow_error_set(error, EINVAL, RTE_FLOW_ERROR_TYPE_ACTION,
> +				   NULL, "Not supported action.");

Again, you could report &act instead of NULL here (please check all
remaining calls to rte_flow_error_set()).

[...]

-- 
Adrien Mazarguil
6WIND