On 1/5/2017 6:08 AM, Xing, Beilei wrote:

Hi Ferruh,

quoted

-----Original Message-----
From: Yigit, Ferruh
Sent: Thursday, January 5, 2017 2:57 AM
To: Xing, Beilei <redacted>; Wu, Jingjing
[off-list ref]; Zhang, Helin [off-list ref]
Cc: dev@dpdk.org
Subject: Re: [dpdk-dev] [PATCH v5 07/17] net/i40e: add flow validate
function

On 1/4/2017 3:22 AM, Beilei Xing wrote:

quoted

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 <redacted>
---

<...>

quoted

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,

quoted

 	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.

Actually it's the global variable definition in i40e_flow.c.  I thought gcc would add extern automatically during compiling, as I checked the address of the variable is the same in different files.
To avoid confusion, I will add extern in next version.

quoted

<...>

quoted

+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.

I will change the API name in next version. Thanks.

quoted

<...>

quoted

+
+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?

You are right, cons_filter will get filter info during validation, and it's for flow_create function.
User needn't to call the flow_validate function, as validate function will be called in i40e_flow_create.

Ok then.

quoted

<...>

quoted

+
+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.

Make sense. I'll update the function names.

quoted

quoted

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

<...>

quoted

+
+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.

In fact, the array of i40e_supported_patterns has listed all supported patterns for each filter type.
i40e_supported_patterns is also defined in this patch.

i40e_supported_patterns only shows item->type values, I think it is good
to documents expected/valid mask (.dst, .src, .type) and last values for
this type.

quoted

quoted

+{
+	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?

Yes, since eth_type mask in eth_mask  should be UINT16_MAX. 

quoted

Can't driver support drop/queue packets from specific src to specific dst with
specific eth_type?

No,  we support specific dst with specific eth_type, or only specific eth_type. Perfect match.

Thanks for clarification.

quoted

quoted

+				rte_flow_error_set(error, EINVAL,
+

RTE_FLOW_ERROR_TYPE_ITEM,

quoted

+						   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,

quoted

+						   item,
+						   "Invalid MAC_addr mask");
+				return -rte_errno;
+			}
+
+			if ((eth_mask->type & UINT16_MAX) !=

UINT16_MAX) {

quoted

+				rte_flow_error_set(error, EINVAL,
+

RTE_FLOW_ERROR_TYPE_ITEM,

quoted

+						   item,
+						   "Invalid ethertype mask");

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

No. as I said above, we support specific dst with specific eth_type, or only specific eth_type for ethertype_filter.

quoted

quoted

+				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,

quoted

+						   item,
+						   "Unsupported ether_type

in"

quoted

+						   " control packet filter.");

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

No, we don't support drop MAC_addr + eth_type_IP for ethertype filter.

quoted

quoted

+				return -rte_errno;
+			}
+			if (filter->ether_type == ETHER_TYPE_VLAN)
+				PMD_DRV_LOG(WARNING, "filter vlan

ether_type in"

quoted

+					    " 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?

Actually I add this warning according to the original processing in i40e_dev_eythertype_filter_set. 
After checing datasheet, "The ethertype programmed by this command should not be one of the L2 tags ethertype (VLAN, E-tag, S-tag, etc.) and should not be IP or IPv6" is descripted.
But if QinQ is disabled, and inner vlan is ETHER_TYPE_VLAN, the filter works. So the message is "vlan ether_type in outer tag is not supported".
I want to simplify it in next version, don't support the situation above, and return error if (filter->ether_type == ETHER_TYPE_VLAN), because HW only recognizes ETH when QinQ is diabled. What do you think?

I think it is better.
And this can be fine tuned in the future to check QinQ and return
accordingly.

quoted

quoted

+
+			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.

Yes, will update in next version.

quoted

<...>

quoted

+
+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,

quoted

+				   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.

Yes. Thanks for reminder.

Best Regards,
Beilei

quoted

<...>