On Tue, 19-05-14, 11:00, Adrien Mazarguil wrote:
> On Tue, May 14, 2019 at 10:34:22AM +0300, Andrew Rybchenko wrote:
> > On 5/14/19 10:18 AM, Xiaoyu Min wrote:
> > > Add GRE's checksum, key, and sequence field to the
> > > struct rte_flow_item_gre in order to match.
> > >
> > > Signed-off-by: Xiaoyu Min <jack...@mellanox.com>
> > > ---
> > > lib/librte_ethdev/rte_flow.h | 4 ++++
> > > 1 file changed, 4 insertions(+)
> > >
> > > diff --git a/lib/librte_ethdev/rte_flow.h b/lib/librte_ethdev/rte_flow.h
> > > index 63f84fca65..fb04af3268 100644
> > > --- a/lib/librte_ethdev/rte_flow.h
> > > +++ b/lib/librte_ethdev/rte_flow.h
> > > @@ -847,6 +847,10 @@ struct rte_flow_item_gre {
> > > */
> > > rte_be16_t c_rsvd0_ver;
> > > rte_be16_t protocol; /**< Protocol type. */
> > > + rte_be16_t checksum; /**< chksum for the header and payload, optional.*/
> > > + rte_be16_t rsvd1; /**< present when C bit is set, optional. */
> > > + rte_be32_t key; /**< application specific key value, optional. */
> > > + rte_be32_t sequence; /**< sequence num for the GRE packet, optional. */
> > > };
> > > /** Default mask for RTE_FLOW_ITEM_TYPE_GRE. */
> >
> > What is the purpose to match checksum, reserved and sequence number?
>
> I think it's not really an issue, this structure only describes a packet
> header as found on the wire like other pattern items; rte_flow users only
> have to provide a mask to select the fields to be matched.
>
> However you can't just modify an existing public structure without going
> through the lengthy API/ABI deprecation/versioning process.
>
> The reason these fields were not initially part of rte_flow_item_gre is that
> each of them is optional, meaning the GRE header has variable length.
> They should be handled through separate objects like IPv6 options (struct
> rte_flow_item_ipv6_ext), ARP (struct rte_flow_item_arp_eth_ipv4) or ICMPv6
> neighbor discovery (struct rte_flow_item_icmp6_nd_opt), either all together
> e.g.:
>
> RTE_FLOW_ITEM_TYPE_GRE_OPTS
>
> struct rte_flow_item_gre_opts {
> rte_be16_t checksum; /**< Checksum for GRE header and payload (C bit). */
> rte_be16_t rsvd1; /**< Reserved bits (C bit). */
> rte_be32_t key; /**< Application specific key value (K bit). */
> rte_be32_t sequence; /**< Sequence number for GRE packet (S bit). */
> };
>
> Or separately, since I guess only key matters no need to define the others:
>
> RTE_FLOW_ITEM_TYPE_GRE_KEY
>
> struct rte_flow_item_gre_key {
> rte_be32_t key; /**< Application specific key value (K bit). */
> };
I will go to this approach.
>
> In both cases, the default mask for this object should cover "key". Make
> sure to update documentation and testpmd in the same patch.
>
Sure, I will do.
Thank you, adrien, for your detail explaination.
I'll send v2 RFC soon.
> --
> Adrien Mazarguil
> 6WIND
