[dpdk-dev,v5,07/17] net/i40e: add flow validate function
Checks
Commit Message
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>
---
drivers/net/i40e/Makefile | 1 +
drivers/net/i40e/i40e_ethdev.c | 7 +
drivers/net/i40e/i40e_ethdev.h | 18 ++
drivers/net/i40e/i40e_flow.c | 447 +++++++++++++++++++++++++++++++++++++++++
4 files changed, 473 insertions(+)
create mode 100644 drivers/net/i40e/i40e_flow.c
Comments
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(ð_mask->src) ||
> + (!is_zero_ether_addr(ð_mask->dst) &&
> + !is_broadcast_ether_addr(ð_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(ð_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.
<...>
Hi Ferruh,
> -----Original Message-----
> From: Yigit, Ferruh
> Sent: Thursday, January 5, 2017 2:57 AM
> To: Xing, Beilei <beilei.xing@intel.com>; Wu, Jingjing
> <jingjing.wu@intel.com>; Zhang, Helin <helin.zhang@intel.com>
> 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:
> > 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.
>
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.
> <...>
>
> > +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.
>
> <...>
>
> > +
> > +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.
>
> <...>
>
> > +
> > +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.
>
> > + 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.
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.
>
> > +{
> > + 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.
> 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.
>
> > + 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(ð_mask->src) ||
> > + (!is_zero_ether_addr(ð_mask->dst) &&
> > + !is_broadcast_ether_addr(ð_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?
No. as I said above, we support specific dst with specific eth_type, or only specific eth_type for ethertype_filter.
>
> > + return -rte_errno;
> > + }
> > +
> > + /* If mask bits of destination MAC address
> > + * are full of 1, set RTE_ETHTYPE_FLAGS_MAC.
> > + */
> > + if (is_broadcast_ether_addr(ð_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 ?
No, we don't support drop MAC_addr + eth_type_IP for ethertype filter.
>
> > + 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?
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?
>
> > +
> > + 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.
>
> <...>
>
> > +
> > +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.
Yes. Thanks for reminder.
Best Regards,
Beilei
>
> <...>
>
On 1/5/2017 6:08 AM, Xing, Beilei wrote:
> Hi Ferruh,
>
>> -----Original Message-----
>> From: Yigit, Ferruh
>> Sent: Thursday, January 5, 2017 2:57 AM
>> To: Xing, Beilei <beilei.xing@intel.com>; Wu, Jingjing
>> <jingjing.wu@intel.com>; Zhang, Helin <helin.zhang@intel.com>
>> 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:
>>> 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.
>>
>
> 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.
>
>> <...>
>>
>>> +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.
>
>>
>> <...>
>>
>>> +
>>> +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.
>
>>
>> <...>
>>
>>> +
>>> +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.
>
>>
>>> + 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.
>
> 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.
>
>>
>>> +{
>>> + 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.
>
>> 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.
>
>>
>>> + 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(ð_mask->src) ||
>>> + (!is_zero_ether_addr(ð_mask->dst) &&
>>> + !is_broadcast_ether_addr(ð_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?
>
> No. as I said above, we support specific dst with specific eth_type, or only specific eth_type for ethertype_filter.
>
>>
>>> + return -rte_errno;
>>> + }
>>> +
>>> + /* If mask bits of destination MAC address
>>> + * are full of 1, set RTE_ETHTYPE_FLAGS_MAC.
>>> + */
>>> + if (is_broadcast_ether_addr(ð_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 ?
>
> No, we don't support drop MAC_addr + eth_type_IP for ethertype filter.
>
>>
>>> + 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?
>
> 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.
>
>>
>>> +
>>> + 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.
>
>>
>> <...>
>>
>>> +
>>> +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.
>
> Yes. Thanks for reminder.
>
> Best Regards,
> Beilei
>
>>
>> <...>
>>
>
> -----Original Message-----
> From: Yigit, Ferruh
> Sent: Thursday, January 5, 2017 7:16 PM
> To: Xing, Beilei <beilei.xing@intel.com>; Wu, Jingjing
> <jingjing.wu@intel.com>; Zhang, Helin <helin.zhang@intel.com>
> Cc: dev@dpdk.org; Zhao1, Wei <wei.zhao1@intel.com>
> Subject: Re: [dpdk-dev] [PATCH v5 07/17] net/i40e: add flow validate
> function
>
> On 1/5/2017 6:08 AM, Xing, Beilei wrote:
> > Hi Ferruh,
> >
> >> -----Original Message-----
> >> From: Yigit, Ferruh
> >> Sent: Thursday, January 5, 2017 2:57 AM
> >> To: Xing, Beilei <beilei.xing@intel.com>; Wu, Jingjing
> >> <jingjing.wu@intel.com>; Zhang, Helin <helin.zhang@intel.com>
> >> 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:
> >>> 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.
> >>
> >
> > 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.
> >
> >> <...>
> >>
> >>> +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.
> >
> >>
> >> <...>
> >>
> >>> +
> >>> +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.
>
> >
> >>
> >> <...>
> >>
> >>> +
> >>> +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.
> >
> >>
> >>> + 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.
> >
> > 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.
OK, I see, will add the comments in the function.
>
> >
> >>
> >>> +{
> >>> + 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.
> >
> >> 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.
>
> >
> >>
> >>> + 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(ð_mask->src) ||
> >>> + (!is_zero_ether_addr(ð_mask->dst) &&
> >>> + !is_broadcast_ether_addr(ð_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?
> >
> > No. as I said above, we support specific dst with specific eth_type, or only
> specific eth_type for ethertype_filter.
> >
> >>
> >>> + return -rte_errno;
> >>> + }
> >>> +
> >>> + /* If mask bits of destination MAC address
> >>> + * are full of 1, set RTE_ETHTYPE_FLAGS_MAC.
> >>> + */
> >>> + if (is_broadcast_ether_addr(ð_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 ?
> >
> > No, we don't support drop MAC_addr + eth_type_IP for ethertype filter.
> >
> >>
> >>> + 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?
> >
> > 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.
I have tuned QinQ, and it will not work when ether_type is equal to the outer vlan. So I will update.
>
> >
> >>
> >>> +
> >>> + 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.
> >
> >>
> >> <...>
> >>
> >>> +
> >>> +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.
> >
> > Yes. Thanks for reminder.
> >
> > Best Regards,
> > Beilei
> >
> >>
> >> <...>
> >>
> >
@@ -105,6 +105,7 @@ endif
SRCS-$(CONFIG_RTE_LIBRTE_I40E_PMD) += i40e_ethdev_vf.c
SRCS-$(CONFIG_RTE_LIBRTE_I40E_PMD) += i40e_pf.c
SRCS-$(CONFIG_RTE_LIBRTE_I40E_PMD) += i40e_fdir.c
+SRCS-$(CONFIG_RTE_LIBRTE_I40E_PMD) += i40e_flow.c
# vector PMD driver needs SSE4.1 support
ifeq ($(findstring RTE_MACHINE_CPUFLAG_SSE4_1,$(CFLAGS)),)
@@ -8426,6 +8426,8 @@ i40e_ethertype_filter_handle(struct rte_eth_dev *dev,
return ret;
}
+const struct rte_flow_ops i40e_flow_ops;
+
static int
i40e_dev_filter_ctrl(struct rte_eth_dev *dev,
enum rte_filter_type filter_type,
@@ -8457,6 +8459,11 @@ i40e_dev_filter_ctrl(struct rte_eth_dev *dev,
case RTE_ETH_FILTER_FDIR:
ret = i40e_fdir_ctrl_func(dev, filter_op, arg);
break;
+ case RTE_ETH_FILTER_GENERIC:
+ if (filter_op != RTE_ETH_FILTER_GET)
+ return -EINVAL;
+ *(const void **)arg = &i40e_flow_ops;
+ break;
default:
PMD_DRV_LOG(WARNING, "Filter type (%d) not supported",
filter_type);
@@ -38,6 +38,7 @@
#include <rte_time.h>
#include <rte_kvargs.h>
#include <rte_hash.h>
+#include <rte_flow_driver.h>
#define I40E_VLAN_TAG_SIZE 4
@@ -629,6 +630,23 @@ struct i40e_adapter {
struct rte_timecounter tx_tstamp_tc;
};
+union i40e_filter_t {
+ struct rte_eth_ethertype_filter ethertype_filter;
+ struct rte_eth_fdir_filter fdir_filter;
+ struct rte_eth_tunnel_filter_conf tunnel_filter;
+};
+
+typedef int (*parse_filter_t)(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 i40e_valid_pattern {
+ enum rte_flow_item_type *items;
+ parse_filter_t parse_filter;
+};
+
int i40e_dev_switch_queues(struct i40e_pf *pf, bool on);
int i40e_vsi_release(struct i40e_vsi *vsi);
struct i40e_vsi *i40e_vsi_setup(struct i40e_pf *pf,
new file mode 100644
@@ -0,0 +1,447 @@
+/*-
+ * BSD LICENSE
+ *
+ * Copyright (c) 2016 Intel Corporation. All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ *
+ * * Redistributions of source code must retain the above copyright
+ * notice, this list of conditions and the following disclaimer.
+ * * Redistributions in binary form must reproduce the above copyright
+ * notice, this list of conditions and the following disclaimer in
+ * the documentation and/or other materials provided with the
+ * distribution.
+ * * Neither the name of Intel Corporation nor the names of its
+ * contributors may be used to endorse or promote products derived
+ * from this software without specific prior written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+ * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+ * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+ * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+ * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+ * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+ * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+ * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+
+#include <sys/queue.h>
+#include <stdio.h>
+#include <errno.h>
+#include <stdint.h>
+#include <string.h>
+#include <unistd.h>
+#include <stdarg.h>
+
+#include <rte_ether.h>
+#include <rte_ethdev.h>
+#include <rte_log.h>
+#include <rte_memzone.h>
+#include <rte_malloc.h>
+#include <rte_eth_ctrl.h>
+#include <rte_tailq.h>
+#include <rte_flow_driver.h>
+
+#include "i40e_logs.h"
+#include "base/i40e_type.h"
+#include "i40e_ethdev.h"
+
+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);
+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);
+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);
+static int i40e_parse_attr(const struct rte_flow_attr *attr,
+ struct rte_flow_error *error);
+
+const struct rte_flow_ops i40e_flow_ops = {
+ .validate = i40e_flow_validate,
+};
+
+union i40e_filter_t cons_filter;
+
+/* Pattern matched ethertype filter */
+static enum rte_flow_item_type pattern_ethertype[] = {
+ RTE_FLOW_ITEM_TYPE_ETH,
+ RTE_FLOW_ITEM_TYPE_END,
+};
+
+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);
+ if (ret)
+ return ret;
+
+ return ret;
+}
+
+static struct i40e_valid_pattern i40e_supported_patterns[] = {
+ /* Ethertype */
+ { pattern_ethertype, i40e_parse_ethertype_filter },
+};
+
+#define NEXT_ITEM_OF_ACTION(act, actions, index) \
+ do { \
+ act = actions + index; \
+ while (act->type == RTE_FLOW_ACTION_TYPE_VOID) { \
+ index++; \
+ act = actions + index; \
+ } \
+ } while (0)
+
+/* Find the first VOID or non-VOID item pointer */
+static const struct rte_flow_item *
+i40e_find_first_item(const struct rte_flow_item *item, bool is_void)
+{
+ bool is_find;
+
+ while (item->type != RTE_FLOW_ITEM_TYPE_END) {
+ if (is_void)
+ is_find = item->type == RTE_FLOW_ITEM_TYPE_VOID;
+ else
+ is_find = item->type != RTE_FLOW_ITEM_TYPE_VOID;
+ if (is_find)
+ break;
+ item++;
+ }
+ return item;
+}
+
+/* Skip all VOID items of the pattern */
+static void
+i40e_pattern_skip_void_item(struct rte_flow_item *items,
+ const struct rte_flow_item *pattern)
+{
+ uint32_t cpy_count = 0;
+ const struct rte_flow_item *pb = pattern, *pe = pattern;
+
+ for (;;) {
+ /* Find a non-void item first */
+ pb = i40e_find_first_item(pb, false);
+ if (pb->type == RTE_FLOW_ITEM_TYPE_END) {
+ pe = pb;
+ break;
+ }
+
+ /* Find a void item */
+ pe = i40e_find_first_item(pb + 1, true);
+
+ cpy_count = pe - pb;
+ rte_memcpy(items, pb, sizeof(struct rte_flow_item) * cpy_count);
+
+ items += cpy_count;
+
+ if (pe->type == RTE_FLOW_ITEM_TYPE_END) {
+ pb = pe;
+ break;
+ }
+
+ pb = pe + 1;
+ }
+ /* Copy the END item. */
+ rte_memcpy(items, pe, sizeof(struct rte_flow_item));
+}
+
+/* Check if the pattern matches a supported item type array */
+static bool
+i40e_match_pattern(enum rte_flow_item_type *item_array,
+ struct rte_flow_item *pattern)
+{
+ struct rte_flow_item *item = pattern;
+
+ while ((*item_array == item->type) &&
+ (*item_array != RTE_FLOW_ITEM_TYPE_END)) {
+ item_array++;
+ item++;
+ }
+
+ return (*item_array == RTE_FLOW_ITEM_TYPE_END &&
+ item->type == RTE_FLOW_ITEM_TYPE_END);
+}
+
+/* Find if there's parse filter function matched */
+static parse_filter_t
+i40e_find_parse_filter_func(struct rte_flow_item *pattern)
+{
+ parse_filter_t parse_filter = NULL;
+ uint8_t i = 0;
+
+ for (; i < RTE_DIM(i40e_supported_patterns); i++) {
+ if (i40e_match_pattern(i40e_supported_patterns[i].items,
+ pattern)) {
+ parse_filter = i40e_supported_patterns[i].parse_filter;
+ break;
+ }
+ }
+
+ return parse_filter;
+}
+
+/* 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,
+ 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
+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)
+{
+ 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) {
+ 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(ð_mask->src) ||
+ (!is_zero_ether_addr(ð_mask->dst) &&
+ !is_broadcast_ether_addr(ð_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");
+ return -rte_errno;
+ }
+
+ /* If mask bits of destination MAC address
+ * are full of 1, set RTE_ETHTYPE_FLAGS_MAC.
+ */
+ if (is_broadcast_ether_addr(ð_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.");
+ return -rte_errno;
+ }
+ if (filter->ether_type == ETHER_TYPE_VLAN)
+ PMD_DRV_LOG(WARNING, "filter vlan ether_type in"
+ " first tag is not supported.");
+
+ 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)
+{
+ struct i40e_pf *pf = I40E_DEV_PRIVATE_TO_PF(dev->data->dev_private);
+ const struct rte_flow_action *act;
+ 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,
+ 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;
+ if (filter->queue >= pf->dev_data->nb_rx_queues) {
+ rte_flow_error_set(error, EINVAL,
+ RTE_FLOW_ERROR_TYPE_ACTION,
+ act, "Invalid queue ID for"
+ " ethertype_filter.");
+ return -rte_errno;
+ }
+ } else {
+ filter->flags |= RTE_ETHTYPE_FLAGS_DROP;
+ }
+
+ /* 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, "Not supported action.");
+ return -rte_errno;
+ }
+
+ return 0;
+}
+
+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;
+ }
+
+ memset(&cons_filter, 0, sizeof(cons_filter));
+
+ /* Get the non-void item number of pattern */
+ while ((pattern + i)->type != RTE_FLOW_ITEM_TYPE_END) {
+ if ((pattern + i)->type != RTE_FLOW_ITEM_TYPE_VOID)
+ item_num++;
+ i++;
+ }
+ item_num++;
+
+ items = rte_zmalloc("i40e_pattern",
+ item_num * sizeof(struct rte_flow_item), 0);
+ if (!items) {
+ rte_flow_error_set(error, ENOMEM, RTE_FLOW_ERROR_TYPE_ITEM_NUM,
+ NULL, "No memory for PMD internal items.");
+ return -ENOMEM;
+ }
+
+ i40e_pattern_skip_void_item(items, pattern);
+
+ /* Find if there's matched parse filter function */
+ parse_filter = i40e_find_parse_filter_func(items);
+ if (!parse_filter) {
+ rte_flow_error_set(error, EINVAL,
+ RTE_FLOW_ERROR_TYPE_ITEM,
+ pattern, "Unsupported pattern");
+ return -rte_errno;
+ }
+
+ ret = parse_filter(dev, attr, items, actions, error, &cons_filter);
+
+ rte_free(items);
+
+ return ret;
+}