From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <olivier.matz@6wind.com>
Received: from mail.droids-corp.org (zoll.droids-corp.org [94.23.50.67])
by dpdk.org (Postfix) with ESMTP id 225AA9FE
for <dev@dpdk.org>; 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 <olivier.matz@6wind.com>)
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 <olivier.matz@6wind.com>
To: Boris Pismenny <borisp@mellanox.com>
Cc: "declan.doherty@intel.com" <declan.doherty@intel.com>,
"pablo.de.lara.guarch@intel.com" <pablo.de.lara.guarch@intel.com>,
"hemant.agrawal@nxp.com" <hemant.agrawal@nxp.com>,
"radu.nicolau@intel.com" <radu.nicolau@intel.com>,
Aviad Yehezkel <aviadye@mellanox.com>,
Thomas Monjalon <thomas@monjalon.net>,
"sandeep.malik@nxp.com" <sandeep.malik@nxp.com>,
"jerin.jacob@caviumnetworks.com" <jerin.jacob@caviumnetworks.com>,
Akhil Goyal <akhil.goyal@nxp.com>, "dev@dpdk.org" <dev@dpdk.org>,
=?iso-8859-1?Q?N=E9lio?= Laranjeiro <nelio.laranjeiro@6wind.com>
Message-ID: <20170920094346.ubyqmzcmxatkfkht@platinum>
References: <20170914082651.26232-1-akhil.goyal@nxp.com>
<20170914082651.26232-6-akhil.goyal@nxp.com>
<DB6PR05MB31765D1FEF330CE0FF0A7E95B0630@DB6PR05MB3176.eurprd05.prod.outlook.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <DB6PR05MB31765D1FEF330CE0FF0A7E95B0630@DB6PR05MB3176.eurprd05.prod.outlook.com>
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 <dev.dpdk.org>
List-Unsubscribe: <http://dpdk.org/ml/options/dev>,
<mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://dpdk.org/ml/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <http://dpdk.org/ml/listinfo/dev>,
<mailto:dev-request@dpdk.org?subject=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 <borisp@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@mellanox.com>
> > Signed-off-by: Boris Pismenny <borisp@mellanox.com>
> > Signed-off-by: Radu Nicolau <radu.nicolau@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?
> 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 <stdio.h>
#include <stdint.h>
#include <string.h>
#include <unistd.h>
#include <sys/mman.h>
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