From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Xing, Beilei" Subject: Re: [PATCH v4 3/4] net/i40e: add MPLS parsing function Date: Thu, 30 Mar 2017 06:35:38 +0000 Message-ID: <94479800C636CB44BD422CB454846E01315C50BE@SHSMSX101.ccr.corp.intel.com> References: <1490683208-38096-1-git-send-email-beilei.xing@intel.com> <1490846875-30484-1-git-send-email-beilei.xing@intel.com> <1490846875-30484-4-git-send-email-beilei.xing@intel.com> <9BB6961774997848B5B42BEC655768F810D1B0F1@SHSMSX103.ccr.corp.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Cc: "Zhang, Helin" , "Iremonger, Bernard" , "dev@dpdk.org" To: "Wu, Jingjing" Return-path: Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by dpdk.org (Postfix) with ESMTP id F369610BAC for ; Thu, 30 Mar 2017 08:35:49 +0200 (CEST) In-Reply-To: <9BB6961774997848B5B42BEC655768F810D1B0F1@SHSMSX103.ccr.corp.intel.com> Content-Language: en-US List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" > -----Original Message----- > From: Wu, Jingjing > Sent: Thursday, March 30, 2017 2:04 PM > To: Xing, Beilei > Cc: Zhang, Helin ; Iremonger, Bernard > ; dev@dpdk.org > Subject: RE: [PATCH v4 3/4] net/i40e: add MPLS parsing function >=20 >=20 >=20 > > -----Original Message----- > > From: Xing, Beilei > > Sent: Thursday, March 30, 2017 12:08 PM > > To: Wu, Jingjing > > Cc: Zhang, Helin ; Iremonger, Bernard > > ; dev@dpdk.org > > Subject: [PATCH v4 3/4] net/i40e: add MPLS parsing function > > > > This patch add MPLS parsing function to support MPLS filtering. > > > > Signed-off-by: Beilei Xing > > --- > > drivers/net/i40e/i40e_ethdev.h | 2 + > > drivers/net/i40e/i40e_flow.c | 234 > > ++++++++++++++++++++++++++++++++++++++++- > > 2 files changed, 235 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/net/i40e/i40e_ethdev.h > > b/drivers/net/i40e/i40e_ethdev.h index 9aacff7..dec1bd6 100644 > > --- a/drivers/net/i40e/i40e_ethdev.h > > +++ b/drivers/net/i40e/i40e_ethdev.h > > @@ -543,6 +543,8 @@ enum i40e_tunnel_type { > > I40E_TUNNEL_TYPE_NVGRE, > > I40E_TUNNEL_TYPE_IP_IN_GRE, > > I40E_L2_TUNNEL_TYPE_E_TAG, > > + I40E_TUNNEL_TYPE_MPLSoUDP, > > + I40E_TUNNEL_TYPE_MPLSoGRE, > > I40E_TUNNEL_TYPE_MAX, > > }; > > > > diff --git a/drivers/net/i40e/i40e_flow.c > > b/drivers/net/i40e/i40e_flow.c index 148e519..c9b9b3b 100644 > > --- a/drivers/net/i40e/i40e_flow.c > > +++ b/drivers/net/i40e/i40e_flow.c > > @@ -57,6 +57,7 @@ > > #define I40E_IPV6_FRAG_HEADER 44 > > #define I40E_TENANT_ARRAY_NUM 3 > > #define I40E_TCI_MASK 0xFFFF > > +#define I40E_MPLS_LABEL_MASK 0xFFFFF > > > > static int i40e_flow_validate(struct rte_eth_dev *dev, > > const struct rte_flow_attr *attr, @@ -114,6 > > +115,12 @@ static int i40e_flow_parse_vxlan_filter(struct rte_eth_dev > > +*dev, > > const struct rte_flow_action actions[], > > struct rte_flow_error *error, > > union i40e_filter_t *filter); > > +static int i40e_flow_parse_mpls_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); > > static int i40e_flow_destroy_ethertype_filter(struct i40e_pf *pf, > > struct i40e_ethertype_filter *filter); static > int > > i40e_flow_destroy_tunnel_filter(struct i40e_pf *pf, @@ -278,6 +285,39 > > @@ static enum rte_flow_item_type pattern_vxlan_4[] =3D { > > RTE_FLOW_ITEM_TYPE_END, > > }; > > > > +/* Pattern matched MPLS */ > > +static enum rte_flow_item_type pattern_mpls_1[] =3D { > > + RTE_FLOW_ITEM_TYPE_ETH, > > + RTE_FLOW_ITEM_TYPE_IPV4, > > + RTE_FLOW_ITEM_TYPE_UDP, > > + RTE_FLOW_ITEM_TYPE_MPLS, > > + RTE_FLOW_ITEM_TYPE_END, > > +}; > > + > > +static enum rte_flow_item_type pattern_mpls_2[] =3D { > > + RTE_FLOW_ITEM_TYPE_ETH, > > + RTE_FLOW_ITEM_TYPE_IPV6, > > + RTE_FLOW_ITEM_TYPE_UDP, > > + RTE_FLOW_ITEM_TYPE_MPLS, > > + RTE_FLOW_ITEM_TYPE_END, > > +}; > > + > > +static enum rte_flow_item_type pattern_mpls_3[] =3D { > > + RTE_FLOW_ITEM_TYPE_ETH, > > + RTE_FLOW_ITEM_TYPE_IPV4, > > + RTE_FLOW_ITEM_TYPE_GRE, > > + RTE_FLOW_ITEM_TYPE_MPLS, > > + RTE_FLOW_ITEM_TYPE_END, > > +}; > > + > > +static enum rte_flow_item_type pattern_mpls_4[] =3D { > > + RTE_FLOW_ITEM_TYPE_ETH, > > + RTE_FLOW_ITEM_TYPE_IPV6, > > + RTE_FLOW_ITEM_TYPE_GRE, > > + RTE_FLOW_ITEM_TYPE_MPLS, > > + RTE_FLOW_ITEM_TYPE_END, > > +}; > > + > > static struct i40e_valid_pattern i40e_supported_patterns[] =3D { > > /* Ethertype */ > > { pattern_ethertype, i40e_flow_parse_ethertype_filter }, @@ -303,6 > > +343,11 @@ static struct i40e_valid_pattern i40e_supported_patterns[] > > +=3D { > > { pattern_vxlan_2, i40e_flow_parse_vxlan_filter }, > > { pattern_vxlan_3, i40e_flow_parse_vxlan_filter }, > > { pattern_vxlan_4, i40e_flow_parse_vxlan_filter }, > > + /* MPLSoUDP & MPLSoGRE */ > > + { pattern_mpls_1, i40e_flow_parse_mpls_filter }, > > + { pattern_mpls_2, i40e_flow_parse_mpls_filter }, > > + { pattern_mpls_3, i40e_flow_parse_mpls_filter }, > > + { pattern_mpls_4, i40e_flow_parse_mpls_filter }, > > }; > > > > #define NEXT_ITEM_OF_ACTION(act, actions, index) = \ > > @@ -1516,6 +1561,193 @@ i40e_flow_parse_vxlan_filter(struct > > rte_eth_dev *dev, } > > > > static int > > +i40e_check_label_mask(const uint8_t *mask) { > > + uint32_t j; > > + int is_masked =3D 0; > > + > > + for (j =3D 0; j < I40E_TENANT_ARRAY_NUM; j++) { > > + if (*(mask + j) =3D=3D 0) { > > + if (j > 0 && (*(mask + j) !=3D *(mask + j - 1))) > > + return -EINVAL; > > + is_masked =3D 1; > > + } else if (*(mask + j) =3D=3D UINT8_MAX) { > > + if (j =3D=3D 2) > > + return -EINVAL; > > + } else if (*(mask + j) =3D=3D 0xF0) { > > + if (j !=3D 2) > > + return -EINVAL; > > + is_masked =3D 0; > > + } else > > + return -EINVAL; > > + } > Do you want to check if the mask is {00, 00, 00} or {FF, FF, F0}? > Why not just compare the mask with two constant array? > This function is difficult to understand. Yes, make sense, and actually we only care about {0xFF, 0xFF, 0xF0}. I will= remove the function and judge it in parse function. > > + return is_masked; > > +} > > + > > +/* 1. Last in item should be NULL as range is not supported. > > + * 2. Supported filter types: MPLS label. > > + * 3. Mask of fields which need to be matched should be > > + * filled with 1. > > + * 4. Mask of fields which needn't to be matched should be > > + * filled with 0. > > + */ > > +static int > > +i40e_flow_parse_mpls_pattern(__rte_unused struct rte_eth_dev *dev, > > + const struct rte_flow_item *pattern, > > + struct rte_flow_error *error, > > + struct i40e_tunnel_filter_conf *filter) { > > + const struct rte_flow_item *item =3D pattern; > > + const struct rte_flow_item_mpls *mpls_spec; > > + const struct rte_flow_item_mpls *mpls_mask; > > + enum rte_flow_item_type item_type; > > + bool is_mplsoudp =3D 0; /* 1 - MPLSoUDP, 0 - MPLSoGRE */ > > + int is_label_masked =3D 0; > > + uint32_t label_be =3D 0; > > + > > + for (; item->type !=3D 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 =3D item->type; > > + switch (item_type) { > > + case RTE_FLOW_ITEM_TYPE_ETH: > > + if (item->spec || item->mask) { > > + rte_flow_error_set(error, EINVAL, > > + > > RTE_FLOW_ERROR_TYPE_ITEM, > > + item, > > + "Invalid ETH item"); > > + return -rte_errno; > > + } > > + break; > > + case RTE_FLOW_ITEM_TYPE_IPV4: > > + filter->ip_type =3D I40E_TUNNEL_IPTYPE_IPV4; > > + /* IPv4 is used to describe protocol, > > + * spec amd mask should be NULL. > Typo: amd->and > > + */ > > + if (item->spec || item->mask) { > > + rte_flow_error_set(error, EINVAL, > > + > > RTE_FLOW_ERROR_TYPE_ITEM, > > + item, > > + "Invalid IPv4 item"); > > + return -rte_errno; > > + } > > + break; > > + case RTE_FLOW_ITEM_TYPE_IPV6: > > + filter->ip_type =3D I40E_TUNNEL_IPTYPE_IPV6; > > + /* IPv6 is used to describe protocol, > > + * spec amd mask should be NULL. > > + */ > Typo: amd->and > > + if (item->spec || item->mask) { > > + rte_flow_error_set(error, EINVAL, > > + > > RTE_FLOW_ERROR_TYPE_ITEM, > > + item, > > + "Invalid IPv6 item"); > > + return -rte_errno; > > + } > > + break; > > + case RTE_FLOW_ITEM_TYPE_UDP: > > + /* UDP is used to describe protocol, > > + * spec amd mask should be NULL. > > + */ > > + if (item->spec || item->mask) { > > + rte_flow_error_set(error, EINVAL, > > + > > RTE_FLOW_ERROR_TYPE_ITEM, > > + item, > > + "Invalid UDP item"); > > + return -rte_errno; > > + } > > + is_mplsoudp =3D 1; > > + break; > > + case RTE_FLOW_ITEM_TYPE_GRE: > > + /* GRE is used to describe protocol, > > + * spec amd mask should be NULL. > > + */ > Typo: amd->and > > + if (item->spec || item->mask) { > > + rte_flow_error_set(error, EINVAL, > > + > > RTE_FLOW_ERROR_TYPE_ITEM, > > + item, > > + "Invalid GRE item"); > > + return -rte_errno; > > + } > > + break; >=20 > You are checking (item->spec || item->mask) in all above cases. > Can we merge them to: > case RTE_FLOW_ITEM_TYPE_ETH: > case RTE_FLOW_ITEM_TYPE_IPV4: > case RTE_FLOW_ITEM_TYPE_IPV6: > case RTE_FLOW_ITEM_TYPE_UDP: > case RTE_FLOW_ITEM_TYPE_GRE: The error info returned to user is different, and different flag need to be= set. > if (item->spec || item->mask) { > ...... > } > break; > > + case RTE_FLOW_ITEM_TYPE_MPLS: > > + mpls_spec =3D > > + (const struct rte_flow_item_mpls *)item- > >spec; > > + mpls_mask =3D > > + (const struct rte_flow_item_mpls *)item- > > >mask; > > + > > + if (!mpls_spec || !mpls_mask) { > > + rte_flow_error_set(error, EINVAL, > > + > > RTE_FLOW_ERROR_TYPE_ITEM, > > + item, > > + "Invalid MPLS item"); > > + return -rte_errno; > > + } > > + > > + is_label_masked =3D > > + i40e_check_label_mask(mpls_mask- > > >label_tc_s); > > + > > + if (is_label_masked !=3D 0) { > > + rte_flow_error_set(error, EINVAL, > > + > > RTE_FLOW_ERROR_TYPE_ITEM, > > + item, > > + "Invalid MPLS label mask"); > > + return -rte_errno; > > + } > > + rte_memcpy(((uint8_t *)&label_be + 1), > > + mpls_spec->label_tc_s, 3); > > + filter->tenant_id =3D rte_be_to_cpu_32(label_be) >> 4; > > + break; > > + default: > > + break; > > + } > > + } > > + > > + if (is_mplsoudp) > > + filter->tunnel_type =3D I40E_TUNNEL_TYPE_MPLSoUDP; > > + else > > + filter->tunnel_type =3D I40E_TUNNEL_TYPE_MPLSoGRE; > > + > > + return 0; > > +} > > + > > +static int > > +i40e_flow_parse_mpls_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 i40e_tunnel_filter_conf *tunnel_filter =3D > > + &filter->consistent_tunnel_filter; > > + int ret; > > + > > + ret =3D i40e_flow_parse_mpls_pattern(dev, pattern, > > + error, tunnel_filter); > > + if (ret) > > + return ret; > > + > > + ret =3D i40e_flow_parse_tunnel_action(dev, actions, error, > tunnel_filter); > > + if (ret) > > + return ret; > > + > > + ret =3D i40e_flow_parse_attr(attr, error); > > + if (ret) > > + return ret; > > + > > + cons_filter_type =3D RTE_ETH_FILTER_TUNNEL; > > + > > + return ret; > > +} > > + > > +static int > > i40e_flow_validate(struct rte_eth_dev *dev, > > const struct rte_flow_attr *attr, > > const struct rte_flow_item pattern[], @@ -1750,7 +1982,7 > @@ > > i40e_flow_destroy_tunnel_filter(struct i40e_pf *pf, > > ret =3D i40e_aq_remove_cloud_filters(hw, vsi->seid, > > &cld_filter.element, 1); > > if (ret < 0) > > - return ret; > > + return -ENOTSUP; > > > > node =3D i40e_sw_tunnel_filter_lookup(tunnel_rule, &filter->input); > > if (!node) > > -- > > 2.5.5