[dpdk-dev] [PATCH 15/18] net/ixgbe: parse flow director filter

[Zhao1, Wei]

Hi,  Adrien

> -----Original Message-----
> From: Adrien Mazarguil [mailto:adrien.mazarguil at 6wind.com]
> Sent: Friday, December 23, 2016 4:13 PM
> To: Yigit, Ferruh <ferruh.yigit at intel.com>
> Cc: Zhao1, Wei <wei.zhao1 at intel.com>; dev at dpdk.org; Lu, Wenzhuo
> <wenzhuo.lu at intel.com>
> Subject: Re: [dpdk-dev] [PATCH 15/18] net/ixgbe: parse flow director filter
> 
> Hi,
> 
> On Thu, Dec 22, 2016 at 10:44:32AM +0000, Ferruh Yigit wrote:
> > On 12/22/2016 9:19 AM, Zhao1, Wei wrote:
> > > Hi, Yigit
> > >
> > >> -----Original Message-----
> > >> From: Yigit, Ferruh
> > >> Sent: Wednesday, December 21, 2016 1:01 AM
> > >> To: Zhao1, Wei <wei.zhao1 at intel.com>; dev at dpdk.org
> > >> Cc: Lu, Wenzhuo <wenzhuo.lu at intel.com>
> > >> Subject: Re: [dpdk-dev] [PATCH 15/18] net/ixgbe: parse flow
> > >> director filter
> > >>
> > >> On 12/2/2016 10:43 AM, Wei Zhao wrote:
> > >>> From: wei zhao1 <wei.zhao1 at intel.com>
> > >>>
> > >>> check if the rule is a flow director rule, and get the flow director info.
> > >>>
> > >>> Signed-off-by: wei zhao1 <wei.zhao1 at intel.com>
> > >>> Signed-off-by: Wenzhuo Lu <wenzhuo.lu at intel.com>
> > >>> ---
> > >>
> > >> <...>
> > >>
> > >>> +	PATTERN_SKIP_VOID(rule, struct ixgbe_fdir_rule,
> > >>> +			  RTE_FLOW_ERROR_TYPE_ITEM_NUM);
> > >>> +	if (item->type != RTE_FLOW_ITEM_TYPE_ETH &&
> > >>> +	    item->type != RTE_FLOW_ITEM_TYPE_IPV4 &&
> > >>> +	    item->type != RTE_FLOW_ITEM_TYPE_IPV6 &&
> > >>> +	    item->type != RTE_FLOW_ITEM_TYPE_UDP &&
> > >>> +	    item->type != RTE_FLOW_ITEM_TYPE_VXLAN &&
> > >>> +	    item->type != RTE_FLOW_ITEM_TYPE_NVGRE) {
> > >>
> > >> This gives build error [1], there are a few more same usage:
> > >>
> > >> .../drivers/net/ixgbe/ixgbe_ethdev.c:9238:17: error: comparison of
> > >> constant
> > >> 241 with expression of type 'const enum rte_flow_item_type' is
> > >> always true [-Werror,-Wtautological-constant-out-of-range-compare]
> > >>             item->type != RTE_FLOW_ITEM_TYPE_NVGRE) {
> > >>
> > >>
> > >>
> > >
> > > Ok, I will add two type definition RTE_FLOW_ITEM_TYPE_NVGRE and
> RTE_FLOW_ITEM_TYPE_E_TAG  into const enum rte_flow_item_type to
> eliminate this problem.
> > > Thank you.
> > >
> >
> > CC: Adrien Mazarguil <adrien.mazarguil at 6wind.com>
> 
> Thanks, the original message did not catch my attention.
> 
> > Yes, that is what right thing to do, since rte_flow patchset not
> > merged yet, perhaps Adrien may want to include this as next version of
> > his patchset?
> >
> > What do you think Adrien?
> 
> I think by now the rte_flow series is automatically categorized as spam by
> half the community already, and that new items such as these can be added
> subsequently on their own, ideally before the entire ixgbe/i40e series.
> 
> I have a few comments regarding these new items if we want to make them
> part of rte_flow.h (see definitions below).
> 
oK , I will add these type definition by my patch in v2

> Unfortunately, even though they are super convenient to use and expose in
> the testpmd flow command, we need to avoid C-style bit-field definitions
> such as these for protocol header matching because they are not endian-
> safe, particularly multi-byte fields. Only meta-data that should be interpreted
> with host endianness can use them (e.g. struct rte_flow_attr, struct
> rte_flow_action_vf, etc):
> 
>  struct rte_flow_item_nvgre {
>         uint32_t flags0:1; /**< 0 */
>         uint32_t rsvd1:1; /**< 1 bit not defined */
>         uint32_t flags1:2; /**< 2 bits, 1 0 */
>         uint32_t rsvd0:9; /**< Reserved0 */
>         uint32_t ver:3; /**< version */
>         uint32_t protocol:16; /**< protocol type, 0x6558 */
>         uint32_t tni:24; /**< tenant network ID or virtual subnet ID */
>         uint32_t flow_id:8; /**< flow ID or Reserved */  };
> 
> For an example how to avoid them, see struct ipv6_hdr definition in rte_ip.h,
> where field vtc_flow is 32 bit but covers three protocol fields and is
> considered big-endian (Nelio's endianness series [1] would be really handy to
> eliminate confusion here). Also see struct rte_flow_item_vxlan, which
> covers 24-bit fields using uint8_t arrays.
> 
>  struct rte_flow_item_e_tag {
>         struct ether_addr dst; /**< Destination MAC. */
>         struct ether_addr src; /**< Source MAC. */
>         uint16_t e_tag_ethertype; /**< E-tag EtherType, 0x893F. */
>         uint16_t e_pcp:3; /**<  E-PCP */
>         uint16_t dei:1; /**< DEI */
>         uint16_t in_e_cid_base:12; /**< Ingress E-CID base */
>         uint16_t rsv:2; /**< reserved */
>         uint16_t grp:2; /**< GRP */
>         uint16_t e_cid_base:12; /**< E-CID base */
>         uint16_t in_e_cid_ext:8; /**< Ingress E-CID extend */
>         uint16_t e_cid_ext:8; /**< E-CID extend */
>         uint16_t type; /**< MAC type. */
>         unsigned int tags; /**< Number of 802.1Q/ad tags defined. */
>         struct {
>                 uint16_t tpid; /**< Tag protocol identifier. */
>                 uint16_t tci; /**< Tag control information. */
>         } tag[]; /**< 802.1Q/ad tag definitions, outermost first. */  };
> 
> Besides the bit-field issue for this one, looks like it should be split, at least the
> initial part is already covered by rte_flow_item_eth.
> 
> I do not know much about E-Tag (IEEE 802.1BR right?) but it sort of looks like a
> 2.5 layer protocol reminiscent of VLAN.
> 
> tags and tag[] fields seem based on the VLAN definition of the original
> rte_flow RFC that has since been replaced with stacked rte_flow_item_vlan
> items, much easier to program for. Perhaps this can be relied on instead of
> having e_tag implement its own variant.
> 
> As a protocol-matching item and E-Tag TCI being 6 bytes according to IEEE
> 802.1BR, sizeof(struct rte_flow_item_e_tag) should ideally return 6 as well
> and would likely comprise three big-endian uint16_t fields. See how
> PCP/DEI/VID VLAN fields are interpreted in testpmd [2].
> 
> Again, these concerns only stand if you intend to include these definitions
> into rte_flow.h.
> 
> [1] http://dpdk.org/ml/archives/dev/2016-November/050060.html
> [2] http://dpdk.org/ml/archives/dev/2016-December/052976.html
> 
> --
> Adrien Mazarguil
> 6WIND