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 <bor...@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 <avia...@mellanox.com>
> > > Signed-off-by: Boris Pismenny <bor...@mellanox.com>
> > > Signed-off-by: Radu Nicolau <radu.nico...@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.
