[dpdk-dev] [PATCH 05/11] lib/librte_mbuf: add security crypto flags and mbuf fields

[Boris Pismenny]

Hi Olivier,

Thanks for the feedback. I'll submit the fixes for our V2 of this series.
Some comments inline.

Olivier Matz wrote:
> 
> Hi Boris,
> 
> Some comments inline.
> 
> On Mon, Sep 18, 2017 at 07:54:03AM +0000, Boris Pismenny wrote:
> > Hi Olivier,
> >
> > On 9/14/2017 11:27 AM, Akhil Goyal wrote:
> > >
> > > From: Boris Pismenny <borisp at mellanox.com>
> > >
> > > add security crypto flags and update mbuf fields to support
> > > IPsec crypto offload for transmitted packets, and to indicate
> > > crypto result for received packets.
> > >
> > > Signed-off-by: Aviad Yehezkel <aviadye at mellanox.com>
> > > Signed-off-by: Boris Pismenny <borisp at mellanox.com>
> > > Signed-off-by: Radu Nicolau <radu.nicolau at intel.com>
> > > ---
> > >  lib/librte_mbuf/rte_mbuf.c |  6 ++++++
> > >  lib/librte_mbuf/rte_mbuf.h | 32 +++++++++++++++++++++++++++++---
> > >  2 files changed, 35 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c
> > > index 26a62b8..bbd42a6 100644
> > > --- a/lib/librte_mbuf/rte_mbuf.c
> > > +++ b/lib/librte_mbuf/rte_mbuf.c
> > > @@ -323,6 +323,8 @@ const char *rte_get_rx_ol_flag_name(uint64_t
> mask)
> > >  	case PKT_RX_QINQ_STRIPPED: return "PKT_RX_QINQ_STRIPPED";
> > >  	case PKT_RX_LRO: return "PKT_RX_LRO";
> > >  	case PKT_RX_TIMESTAMP: return "PKT_RX_TIMESTAMP";
> > > +	case PKT_RX_SEC_OFFLOAD: return "PKT_RX_SECURITY_OFFLOAD";
> > > +	case PKT_RX_SEC_OFFLOAD_FAILED: return
> > > "PKT_RX_SECURITY_OFFLOAD_FAILED";
> 
> I think the string should be the same than the macro.
> (SEC vs SECURITY)
> 
> > ...
> > > +/**
> > > + * Indicate that security offload processing was applied on the RX packet.
> > > + */
> > > +#define PKT_RX_SEC_OFFLOAD		(1ULL << 18)
> > > +
> > > +/**
> > > + * Indicate that security offload processing failed on the RX packet.
> > > + */
> > > +#define PKT_RX_SEC_OFFLOAD_FAILED  (1ULL << 19)
> > > +
> 
> If the presence of these flags implies that some fields are
> valid (ex: inner_esp_next_proto), it should be specified in
> the API comments.
> 
> > ...
> > > @@ -456,8 +472,18 @@ struct rte_mbuf {
> > >  			uint32_t l3_type:4; /**< (Outer) L3 type. */
> > >  			uint32_t l4_type:4; /**< (Outer) L4 type. */
> > >  			uint32_t tun_type:4; /**< Tunnel type. */
> > > -			uint32_t inner_l2_type:4; /**< Inner L2 type. */
> > > -			uint32_t inner_l3_type:4; /**< Inner L3 type. */
> > > +			RTE_STD_C11
> > > +			union {
> > > +				uint8_t inner_esp_next_proto;
> > > +
> > > +				__extension__
> > > +				struct {
> > > +					uint8_t inner_l2_type:4;
> > > +					/**< Inner L2 type. */
> > > +					uint8_t inner_l3_type:4;
> > > +					/**< Inner L3 type. */
> > > +				};
> > > +			};
> > >  			uint32_t inner_l4_type:4; /**< Inner L4 type. */
> > >  		};
> > >  	};
> 
> The (quite useless) API comment is missing. I think we should
> have it for consistency.
> 
> Can you please also detail in which conditions inner_esp_next_proto is
> valid, and when inner_l2/l3_type is valid?
> 

It is valid when the packet has an ESP header with plaintext data.
As explained below, ESP packets have no inner_l2 and possibly no inner_l3
So these meaningless fields are replaced with a meaningful field for ESP packets.
I'll add a new PTYPE_TUNNEL_ESP to indicate that the inner_esp_next_proto
is valid.

We will use this field later in the ipsec-secgw example for both Rx and Tx to
indicate the inner ESP protocol type in packets whose trailer is added/removed
by HW.

Thanks,
Boris.