Hi, Adrien
> -----Original Message-----
> From: Adrien Mazarguil [mailto:adrien.mazarg...@6wind.com]
> Sent: Friday, December 23, 2016 4:13 PM
> To: Yigit, Ferruh <ferruh.yi...@intel.com>
> Cc: Zhao1, Wei <wei.zh...@intel.com>; dev@dpdk.org; Lu, Wenzhuo
> <wenzhuo...@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.zh...@intel.com>; dev@dpdk.org
> > >> Cc: Lu, Wenzhuo <wenzhuo...@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.zh...@intel.com>
> > >>>
> > >>> check if the rule is a flow director rule, and get the flow director
> > >>> info.
> > >>>
> > >>> Signed-off-by: wei zhao1 <wei.zh...@intel.com>
> > >>> Signed-off-by: Wenzhuo Lu <wenzhuo...@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.mazarg...@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
