[dpdk-dev,v2,12/18] net/ixgbe: parse ethertype filter

Message ID 1483084390-53159-13-git-send-email-wei.zhao1@intel.com (mailing list archive)
State Superseded, archived
Delegated to: Ferruh Yigit
Headers

Checks

Context Check Description
ci/checkpatch warning coding style issues
ci/Intel compilation fail Compilation issues

Commit Message

Zhao1, Wei Dec. 30, 2016, 7:53 a.m. UTC
  check if the rule is a ethertype rule, and get the ethertype info.
Signed-off-by: Wei Zhao <wei.zhao1@intel.com>
Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>

---

v2:add new error set function
---
 drivers/net/ixgbe/ixgbe_ethdev.c | 251 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 251 insertions(+)
  

Comments

Ferruh Yigit Jan. 6, 2017, 5:11 p.m. UTC | #1
On 12/30/2016 7:53 AM, Wei Zhao wrote:
> check if the rule is a ethertype rule, and get the ethertype info.
> Signed-off-by: Wei Zhao <wei.zhao1@intel.com>
> Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
> 
> ---
> 
> v2:add new error set function
> ---

<...>

> +static int
> +ixgbe_parse_ethertype_filter(const struct rte_flow_attr *attr,
> +			     const struct rte_flow_item pattern[],
> +			     const struct rte_flow_action actions[],
> +			     struct rte_eth_ethertype_filter *filter,
> +			     struct rte_flow_error *error)
> +{
> +	int ret;
> +
> +	ret = cons_parse_ethertype_filter(attr, pattern,
> +					actions, filter, error);
> +
> +	if (ret)
> +		return ret;
> +
> +	/* Ixgbe doesn't support MAC address. */
> +	if (filter->flags & RTE_ETHTYPE_FLAGS_MAC) {
> +		memset(filter, 0, sizeof(struct rte_eth_ethertype_filter));

Is memset required for error cases, if so is other error cases also
require it?

> +		rte_flow_error_set(error, EINVAL,
> +				RTE_FLOW_ERROR_TYPE_ITEM,
> +				NULL, "Not supported by ethertype filter");
> +		return -rte_errno;
> +	}
> +
> +	if (filter->queue >= IXGBE_MAX_RX_QUEUE_NUM)
> +		return -rte_errno;
> +
> +	if (filter->ether_type == ETHER_TYPE_IPv4 ||
> +		filter->ether_type == ETHER_TYPE_IPv6) {
> +		PMD_DRV_LOG(ERR, "unsupported ether_type(0x%04x) in"
> +			" ethertype filter.", filter->ether_type);

Not sure about this log here, specially it is in ERR level.
This function is returning error, and public API will return an error,
if we want to notify user with a log, should app do this as library
(here) should do this? More comment welcome?

btw, should rte_flow_error_set() used here (and below) ?

> +		return -rte_errno;
> +	}
> +
> +	if (filter->flags & RTE_ETHTYPE_FLAGS_MAC) {

Isn't this duplicate with above check?

> +		PMD_DRV_LOG(ERR, "mac compare is unsupported.");
> +		return -rte_errno;
> +	}
> +
> +	if (filter->flags & RTE_ETHTYPE_FLAGS_DROP) {

Just to double check, isn't drop action by ether filters?

> +		PMD_DRV_LOG(ERR, "drop option is unsupported.");
> +		return -rte_errno;
> +	}
> +
> +	return 0;
> +}
> +
> +/**
<...>
  
Zhao1, Wei Jan. 11, 2017, 8:54 a.m. UTC | #2
Hi, Yigit

> -----Original Message-----
> From: Yigit, Ferruh
> Sent: Saturday, January 7, 2017 1:11 AM
> To: Zhao1, Wei <wei.zhao1@intel.com>; dev@dpdk.org
> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>
> Subject: Re: [dpdk-dev] [PATCH v2 12/18] net/ixgbe: parse ethertype filter
> 
> On 12/30/2016 7:53 AM, Wei Zhao wrote:
> > check if the rule is a ethertype rule, and get the ethertype info.
> > Signed-off-by: Wei Zhao <wei.zhao1@intel.com>
> > Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
> >
> > ---
> >
> > v2:add new error set function
> > ---
> 
> <...>
> 
> > +static int
> > +ixgbe_parse_ethertype_filter(const struct rte_flow_attr *attr,
> > +			     const struct rte_flow_item pattern[],
> > +			     const struct rte_flow_action actions[],
> > +			     struct rte_eth_ethertype_filter *filter,
> > +			     struct rte_flow_error *error) {
> > +	int ret;
> > +
> > +	ret = cons_parse_ethertype_filter(attr, pattern,
> > +					actions, filter, error);
> > +
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* Ixgbe doesn't support MAC address. */
> > +	if (filter->flags & RTE_ETHTYPE_FLAGS_MAC) {
> > +		memset(filter, 0, sizeof(struct rte_eth_ethertype_filter));
> 
> Is memset required for error cases, if so is other error cases also require it?


Yes, Ok , I will do as your suggestion.

> 
> > +		rte_flow_error_set(error, EINVAL,
> > +				RTE_FLOW_ERROR_TYPE_ITEM,
> > +				NULL, "Not supported by ethertype filter");
> > +		return -rte_errno;
> > +	}
> > +
> > +	if (filter->queue >= IXGBE_MAX_RX_QUEUE_NUM)
> > +		return -rte_errno;
> > +
> > +	if (filter->ether_type == ETHER_TYPE_IPv4 ||
> > +		filter->ether_type == ETHER_TYPE_IPv6) {
> > +		PMD_DRV_LOG(ERR, "unsupported ether_type(0x%04x) in"
> > +			" ethertype filter.", filter->ether_type);
> 
> Not sure about this log here, specially it is in ERR level.
> This function is returning error, and public API will return an error, if we want
> to notify user with a log, should app do this as library
> (here) should do this? More comment welcome?
> 
> btw, should rte_flow_error_set() used here (and below) ?

Yes, I will change to rte_flow_error_set() 

> 
> > +		return -rte_errno;
> > +	}
> > +
> > +	if (filter->flags & RTE_ETHTYPE_FLAGS_MAC) {
> 
> Isn't this duplicate with above check?
> 
> > +		PMD_DRV_LOG(ERR, "mac compare is unsupported.");
> > +		return -rte_errno;
> > +	}
> > +
> > +	if (filter->flags & RTE_ETHTYPE_FLAGS_DROP) {
> 
> Just to double check, isn't drop action by ether filters?

Yse , ixgbe do not, but i40e is.
> 
> > +		PMD_DRV_LOG(ERR, "drop option is unsupported.");
> > +		return -rte_errno;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +/**
> <...>
  

Patch

diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
index 198cc4b..f2f6cc7 100644
--- a/drivers/net/ixgbe/ixgbe_ethdev.c
+++ b/drivers/net/ixgbe/ixgbe_ethdev.c
@@ -401,6 +401,18 @@  ixgbe_parse_ntuple_filter(const struct rte_flow_attr *attr,
 					struct rte_eth_ntuple_filter *filter,
 					struct rte_flow_error *error);
 static int
+cons_parse_ethertype_filter(const struct rte_flow_attr *attr,
+			    const struct rte_flow_item *pattern,
+			    const struct rte_flow_action *actions,
+			    struct rte_eth_ethertype_filter *filter,
+			    struct rte_flow_error *error);
+static int
+ixgbe_parse_ethertype_filter(const struct rte_flow_attr *attr,
+				const struct rte_flow_item pattern[],
+				const struct rte_flow_action actions[],
+				struct rte_eth_ethertype_filter *filter,
+				struct rte_flow_error *error);
+static int
 ixgbe_flow_validate(__rte_unused struct rte_eth_dev *dev,
 		const struct rte_flow_attr *attr,
 		const struct rte_flow_item pattern[],
@@ -8451,6 +8463,238 @@  ixgbe_parse_ntuple_filter(const struct rte_flow_attr *attr,
 }
 
 /**
+ * Parse the rule to see if it is a ethertype rule.
+ * And get the ethertype filter info BTW.
+ */
+static int
+cons_parse_ethertype_filter(const struct rte_flow_attr *attr,
+			    const struct rte_flow_item *pattern,
+			    const struct rte_flow_action *actions,
+			    struct rte_eth_ethertype_filter *filter,
+			    struct rte_flow_error *error)
+{
+	const struct rte_flow_item *item;
+	const struct rte_flow_action *act;
+	const struct rte_flow_item_eth *eth_spec;
+	const struct rte_flow_item_eth *eth_mask;
+	const struct rte_flow_action_queue *act_q;
+	uint32_t index;
+
+	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;
+	}
+
+	/* Parse pattern */
+	index = 0;
+
+	/* The first non-void item should be MAC. */
+	item = pattern + index;
+	while (item->type == RTE_FLOW_ITEM_TYPE_VOID) {
+		index++;
+		item = pattern + index;
+	}
+	if (item->type != RTE_FLOW_ITEM_TYPE_ETH) {
+		rte_flow_error_set(error, EINVAL,
+			RTE_FLOW_ERROR_TYPE_ITEM,
+			item, "Not supported by ethertype filter");
+		return -rte_errno;
+	}
+
+	/*Not supported last point for range*/
+	if (item->last) {
+		rte_flow_error_set(error, EINVAL,
+			RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
+			item, "Not supported last point for range");
+		return -rte_errno;
+	}
+
+	/* Get the MAC info. */
+	if (!item->spec || !item->mask) {
+		rte_flow_error_set(error, EINVAL,
+				RTE_FLOW_ERROR_TYPE_ITEM,
+				item, "Not supported by ethertype filter");
+		return -rte_errno;
+	}
+
+	eth_spec = (const struct rte_flow_item_eth *)item->spec;
+	eth_mask = (const struct rte_flow_item_eth *)item->mask;
+
+	/* 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 ether address 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");
+		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);
+
+	/* Check if the next non-void item is END. */
+	index++;
+	item = pattern + index;
+	while (item->type == RTE_FLOW_ITEM_TYPE_VOID) {
+		index++;
+		item = pattern + index;
+	}
+	if (item->type != RTE_FLOW_ITEM_TYPE_END) {
+		rte_flow_error_set(error, EINVAL,
+				RTE_FLOW_ERROR_TYPE_ITEM,
+				item, "Not supported by ethertype filter.");
+		return -rte_errno;
+	}
+
+	/* Parse action */
+
+	index = 0;
+	/* Check if the first non-void action is QUEUE or DROP. */
+	act = actions + index;
+	while (act->type == RTE_FLOW_ACTION_TYPE_VOID) {
+		index++;
+		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,
+				act, "Not supported action.");
+		return -rte_errno;
+	}
+
+	if (act->type == RTE_FLOW_ACTION_TYPE_QUEUE) {
+		act_q = (const struct rte_flow_action_queue *)act->conf;
+		filter->queue = act_q->index;
+	} else {
+		filter->flags |= RTE_ETHTYPE_FLAGS_DROP;
+	}
+
+	/* Check if the next non-void item is END */
+	index++;
+	act = actions + index;
+	while (act->type == RTE_FLOW_ACTION_TYPE_VOID) {
+		index++;
+		act = actions + index;
+	}
+	if (act->type != RTE_FLOW_ACTION_TYPE_END) {
+		rte_flow_error_set(error, EINVAL,
+				RTE_FLOW_ERROR_TYPE_ACTION,
+				act, "Not supported action.");
+		return -rte_errno;
+	}
+
+	/* Parse attr */
+	/* Must be input direction */
+	if (!attr->ingress) {
+		rte_flow_error_set(error, EINVAL,
+				RTE_FLOW_ERROR_TYPE_ATTR_INGRESS,
+				attr, "Only support ingress.");
+		return -rte_errno;
+	}
+
+	/* Not supported */
+	if (attr->egress) {
+		rte_flow_error_set(error, EINVAL,
+				RTE_FLOW_ERROR_TYPE_ATTR_EGRESS,
+				attr, "Not support egress.");
+		return -rte_errno;
+	}
+
+	/* Not supported */
+	if (attr->priority) {
+		rte_flow_error_set(error, EINVAL,
+				RTE_FLOW_ERROR_TYPE_ATTR_PRIORITY,
+				attr, "Not support priority.");
+		return -rte_errno;
+	}
+
+	/* Not supported */
+	if (attr->group) {
+		rte_flow_error_set(error, EINVAL,
+				RTE_FLOW_ERROR_TYPE_ATTR_GROUP,
+				attr, "Not support group.");
+		return -rte_errno;
+	}
+
+	return 0;
+}
+
+static int
+ixgbe_parse_ethertype_filter(const struct rte_flow_attr *attr,
+			     const struct rte_flow_item pattern[],
+			     const struct rte_flow_action actions[],
+			     struct rte_eth_ethertype_filter *filter,
+			     struct rte_flow_error *error)
+{
+	int ret;
+
+	ret = cons_parse_ethertype_filter(attr, pattern,
+					actions, filter, error);
+
+	if (ret)
+		return ret;
+
+	/* Ixgbe doesn't support MAC address. */
+	if (filter->flags & RTE_ETHTYPE_FLAGS_MAC) {
+		memset(filter, 0, sizeof(struct rte_eth_ethertype_filter));
+		rte_flow_error_set(error, EINVAL,
+				RTE_FLOW_ERROR_TYPE_ITEM,
+				NULL, "Not supported by ethertype filter");
+		return -rte_errno;
+	}
+
+	if (filter->queue >= IXGBE_MAX_RX_QUEUE_NUM)
+		return -rte_errno;
+
+	if (filter->ether_type == ETHER_TYPE_IPv4 ||
+		filter->ether_type == ETHER_TYPE_IPv6) {
+		PMD_DRV_LOG(ERR, "unsupported ether_type(0x%04x) in"
+			" ethertype filter.", filter->ether_type);
+		return -rte_errno;
+	}
+
+	if (filter->flags & RTE_ETHTYPE_FLAGS_MAC) {
+		PMD_DRV_LOG(ERR, "mac compare is unsupported.");
+		return -rte_errno;
+	}
+
+	if (filter->flags & RTE_ETHTYPE_FLAGS_DROP) {
+		PMD_DRV_LOG(ERR, "drop option is unsupported.");
+		return -rte_errno;
+	}
+
+	return 0;
+}
+
+/**
  * Check if the flow rule is supported by ixgbe.
  * It only checkes the format. Don't guarantee the rule can be programmed into
  * the HW. Because there can be no enough room for the rule.
@@ -8463,6 +8707,7 @@  ixgbe_flow_validate(__rte_unused struct rte_eth_dev *dev,
 		struct rte_flow_error *error)
 {
 	struct rte_eth_ntuple_filter ntuple_filter;
+	struct rte_eth_ethertype_filter ethertype_filter;
 	int ret;
 
 	memset(&ntuple_filter, 0, sizeof(struct rte_eth_ntuple_filter));
@@ -8471,6 +8716,12 @@  ixgbe_flow_validate(__rte_unused struct rte_eth_dev *dev,
 	if (!ret)
 		return 0;
 
+	memset(&ethertype_filter, 0, sizeof(struct rte_eth_ethertype_filter));
+	ret = ixgbe_parse_ethertype_filter(attr, pattern,
+				actions, &ethertype_filter, error);
+	if (!ret)
+		return 0;
+
 	return ret;
 }