From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail.droids-corp.org (zoll.droids-corp.org [94.23.50.67]) by dpdk.org (Postfix) with ESMTP id 225AA9FE for ; Wed, 20 Sep 2017 11:44:12 +0200 (CEST) Received: from lfbn-lil-1-182-75.w90-45.abo.wanadoo.fr ([90.45.31.75] helo=droids-corp.org) by mail.droids-corp.org with esmtpsa (TLS1.0:RSA_AES_256_CBC_SHA1:256) (Exim 4.84_2) (envelope-from ) id 1dubdR-0006e2-1j; Wed, 20 Sep 2017 11:49:47 +0200 Received: by droids-corp.org (sSMTP sendmail emulation); Wed, 20 Sep 2017 11:43:49 +0200 Date: Wed, 20 Sep 2017 11:43:49 +0200 From: Olivier MATZ To: Boris Pismenny Cc: "declan.doherty@intel.com" , "pablo.de.lara.guarch@intel.com" , "hemant.agrawal@nxp.com" , "radu.nicolau@intel.com" , Aviad Yehezkel , Thomas Monjalon , "sandeep.malik@nxp.com" , "jerin.jacob@caviumnetworks.com" , Akhil Goyal , "dev@dpdk.org" , =?iso-8859-1?Q?N=E9lio?= Laranjeiro Message-ID: <20170920094346.ubyqmzcmxatkfkht@platinum> References: <20170914082651.26232-1-akhil.goyal@nxp.com> <20170914082651.26232-6-akhil.goyal@nxp.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: User-Agent: NeoMutt/20170113 (1.7.2) Subject: Re: [dpdk-dev] [PATCH 05/11] lib/librte_mbuf: add security crypto flags and mbuf fields X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 20 Sep 2017 09:44:12 -0000 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 > > > > 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 > > Signed-off-by: Boris Pismenny > > Signed-off-by: Radu Nicolau > > --- > > 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? > What do you think about this change to mbuf? > > It doesn't increase the mbuf size and it replaces some fields that have no meaning > in IPsec encapsulations (inner L2 and L3) with a meaningful field of the correct > size (inner_esp_next_proto - 8 bytes). > > We later use this for IPsec offload on both Tx and Rx to indicate the packet format. > Strangely, the abi-checker script finds an abi change. To me, it looks like a false positive of abi-checker. The html output says: Field inner_l2_type has been removed from this type. Applications will access incorrect memory when attempting to access this field. Field inner_l3_type has been removed from this type. Applications will access incorrect memory when attempting to access this field. [−] affected symbols: 3 (15.8%) __rte_pktmbuf_read ( struct rte_mbuf const* m, uint32_t off, uint32_t len, void* buf ) @@ DPDK_16.11 Field 'm.unnamed1.unnamed0' in 1st parameter 'm' (pointer) has type 'anon-struct-rte_mbuf.h-454'. rte_mbuf_sanity_check ( struct rte_mbuf const* m, int is_header ) @@ DPDK_2.0 Field 'm.unnamed1.unnamed0' in 1st parameter 'm' (pointer) has type 'anon-struct-rte_mbuf.h-454'. rte_pktmbuf_dump ( FILE* f, struct rte_mbuf const* m, unsigned int dump_len ) @@ DPDK_2.0 Field 'm.unnamed1.unnamed0' in 2nd parameter 'm' (pointer) has type 'anon-struct-rte_mbuf.h-454'. If someone has a better explanation :) You can reproduce it with the following patch: http://dpdk.org/dev/patchwork/patch/28985/ However, with pahole, we can check that the sizes/offsets are correct and also, the following test program behaves as expected: #include #include #include #include #include struct mbuf1 { union { uint32_t packet_type; struct { uint32_t l2_type:4; uint32_t l3_type:4; uint32_t l4_type:4; uint32_t tun_type:4; uint32_t inner_l2_type:4; uint32_t inner_l3_type:4; uint32_t inner_l4_type:4; }; }; }; struct mbuf2 { union { uint32_t packet_type; struct { uint32_t l2_type:4; uint32_t l3_type:4; uint32_t l4_type:4; uint32_t tun_type:4; union { uint8_t inner_esp_next_proto; struct { uint8_t inner_l2_type:4; uint8_t inner_l3_type:4; }; }; uint32_t inner_l4_type:4; }; }; }; int main(void) { struct mbuf1 m1; struct mbuf2 m2; m1.l2_type = 0x1; m1.l3_type = 0x2; m1.l4_type = 0x3; m1.tun_type = 0x4; m1.inner_l2_type = 0x5; m1.inner_l3_type = 0x6; m1.inner_l4_type = 0x7; printf("m1.l2_type=%x\n", m1.l2_type); printf("m1.l3_type=%x\n", m1.l3_type); printf("m1.l4_type=%x\n", m1.l4_type); printf("m1.tun_type=%x\n", m1.tun_type); printf("m1.inner_l2_type=%x\n", m1.inner_l2_type); printf("m1.inner_l3_type=%x\n", m1.inner_l3_type); printf("m1.inner_l4_type=%x\n", m1.inner_l4_type); memcpy(&m2, &m1, sizeof(m2)); printf("m2.l2_type=%x\n", m2.l2_type); printf("m2.l3_type=%x\n", m2.l3_type); printf("m2.l4_type=%x\n", m2.l4_type); printf("m2.tun_type=%x\n", m2.tun_type); printf("m2.inner_l2_type=%x\n", m2.inner_l2_type); printf("m2.inner_l3_type=%x\n", m2.inner_l3_type); printf("m2.inner_l4_type=%x\n", m2.inner_l4_type); return 0; } Olivier