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

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

On 1/4/2017 3:22 AM, 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@intel.com>
> ---

<...>

> diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
> index 153322a..edfd52b 100644
> --- a/drivers/net/i40e/i40e_ethdev.c
> +++ b/drivers/net/i40e/i40e_ethdev.c
> @@ -8426,6 +8426,8 @@ i40e_ethertype_filter_handle(struct rte_eth_dev *dev,
>  	return ret;
>  }
>  
> +const struct rte_flow_ops i40e_flow_ops;

Is this intentional (instead of using extern) ?
Because i40e_flow.c has a global variable definition with same name, it
looks like this is not causing a build error, but I think confusing.

<...>

> +static int i40e_parse_ethertype_act(struct rte_eth_dev *dev,
> +				    const struct rte_flow_action *actions,
> +				    struct rte_flow_error *error,
> +				    struct rte_eth_ethertype_filter *filter);

In API naming, I would prefer full "action" instead of shorten "act",
but it is your call.

<...>

> +
> +union i40e_filter_t cons_filter;

Why this cons_filter is required. I can see this is saving some state
related rule during validate function.
If the plan is to use this during rule creation, is user has to call
validate before each create?

<...>

> +
> +static int
> +i40e_parse_ethertype_filter(struct rte_eth_dev *dev,
> +			    const struct rte_flow_attr *attr,
> +			    const struct rte_flow_item pattern[],
> +			    const struct rte_flow_action actions[],
> +			    struct rte_flow_error *error,
> +			    union i40e_filter_t *filter)
> +{
> +	struct rte_eth_ethertype_filter *ethertype_filter =
> +		&filter->ethertype_filter;
> +	int ret;
> +
> +	ret = i40e_parse_ethertype_pattern(dev, pattern, error,
> +					   ethertype_filter);
> +	if (ret)
> +		return ret;
> +
> +	ret = i40e_parse_ethertype_act(dev, actions, error,
> +				       ethertype_filter);
> +	if (ret)
> +		return ret;
> +
> +	ret = i40e_parse_attr(attr, error);

It is your call, but I would suggest using a specific namespace for all
rte_flow related functions, something like "i40e_flow_".
In this context it is clear what this function is, but in whole driver
code, the function name is too generic to understand what it does.

> +	if (ret)
> +		return ret;
> +
> +	return ret;
> +}
> +

<...>

> +
> +static int
> +i40e_parse_ethertype_pattern(__rte_unused struct rte_eth_dev *dev,
> +			     const struct rte_flow_item *pattern,
> +			     struct rte_flow_error *error,
> +			     struct rte_eth_ethertype_filter *filter)

I think it is good idea to comment what pattern is recognized in to
function comment, instead of reading code every time to figure out.

> +{
> +	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++) {
> +		if (item->last) {
> +			rte_flow_error_set(error, EINVAL,
> +					   RTE_FLOW_ERROR_TYPE_ITEM,
> +					   item,
> +					   "Not support range");
> +			return -rte_errno;
> +		}
> +		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) {

Why an eth_mask is required?
Can't driver support drop/queue packets from specific src to specific
dst with specific eth_type?

> +				rte_flow_error_set(error, EINVAL,
> +						   RTE_FLOW_ERROR_TYPE_ITEM,
> +						   item,
> +						   "NULL ETH spec/mask");
> +				return -rte_errno;
> +			}
> +
> +			/* Mask bits of source MAC address must be full of 0.
> +			 * Mask bits of destination MAC address must be full
> +			 * of 1 or full of 0.
> +			 */
> +			if (!is_zero_ether_addr(&eth_mask->src) ||
> +			    (!is_zero_ether_addr(&eth_mask->dst) &&
> +			     !is_broadcast_ether_addr(&eth_mask->dst))) {
> +				rte_flow_error_set(error, EINVAL,
> +						   RTE_FLOW_ERROR_TYPE_ITEM,
> +						   item,
> +						   "Invalid MAC_addr mask");
> +				return -rte_errno;
> +			}
> +
> +			if ((eth_mask->type & UINT16_MAX) != UINT16_MAX) {
> +				rte_flow_error_set(error, EINVAL,
> +						   RTE_FLOW_ERROR_TYPE_ITEM,
> +						   item,
> +						   "Invalid ethertype mask");

Why returning error here?
Can't we say drop packets to specific MAC address, independent from the
ether_type?

> +				return -rte_errno;
> +			}
> +
> +			/* If mask bits of destination MAC address
> +			 * are full of 1, set RTE_ETHTYPE_FLAGS_MAC.
> +			 */
> +			if (is_broadcast_ether_addr(&eth_mask->dst)) {
> +				filter->mac_addr = eth_spec->dst;
> +				filter->flags |= RTE_ETHTYPE_FLAGS_MAC;
> +			} else {
> +				filter->flags &= ~RTE_ETHTYPE_FLAGS_MAC;
> +			}
> +			filter->ether_type = rte_be_to_cpu_16(eth_spec->type);
> +
> +			if (filter->ether_type == ETHER_TYPE_IPv4 ||
> +			    filter->ether_type == ETHER_TYPE_IPv6) {
> +				rte_flow_error_set(error, EINVAL,
> +						   RTE_FLOW_ERROR_TYPE_ITEM,
> +						   item,
> +						   "Unsupported ether_type in"
> +						   " control packet filter.");

Can't we create a drop rule based on dst MAC address if eth_type is ip ?

> +				return -rte_errno;
> +			}
> +			if (filter->ether_type == ETHER_TYPE_VLAN)
> +				PMD_DRV_LOG(WARNING, "filter vlan ether_type in"
> +					    " first tag is not supported.");

Who is the target of this message?
To the caller, this API is responding as this is supported.
The end user, the user of the application, can see this message, how
this message will help to end user?

> +
> +			break;
> +		default:
> +			break;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static int
> +i40e_parse_ethertype_act(struct rte_eth_dev *dev,
> +			 const struct rte_flow_action *actions,
> +			 struct rte_flow_error *error,
> +			 struct rte_eth_ethertype_filter *filter)

I think it would be good to comment this functions to say only DROP and
QUEUE actions are supported.

<...>

> +
> +static int
> +i40e_flow_validate(struct rte_eth_dev *dev,
> +		   const struct rte_flow_attr *attr,
> +		   const struct rte_flow_item pattern[],
> +		   const struct rte_flow_action actions[],
> +		   struct rte_flow_error *error)
> +{
> +	struct rte_flow_item *items; /* internal pattern w/o VOID items */
> +	parse_filter_t parse_filter;
> +	uint32_t item_num = 0; /* non-void item number of pattern*/
> +	uint32_t i = 0;
> +	int ret;
> +
> +	if (!pattern) {
> +		rte_flow_error_set(error, EINVAL, RTE_FLOW_ERROR_TYPE_ITEM_NUM,
> +				   NULL, "NULL pattern.");
> +		return -rte_errno;
> +	}
> +
> +	if (!actions) {
> +		rte_flow_error_set(error, EINVAL,
> +				   RTE_FLOW_ERROR_TYPE_ACTION_NUM,
> +				   NULL, "NULL action.");
> +		return -rte_errno;
> +	}

It may be good to validate attr too, if it is NULL or not. It is
accessed without check in later stages of the call stack.

<...>