On Mon, Oct 26, 2020 at 11:20:03PM +0100, Thomas Monjalon wrote:
> The device-specific metadata was stored in the deprecated field udata64.
> It is moved to a dynamic mbuf field in order to allow removal of udata64.
>
> Signed-off-by: Thomas Monjalon <tho...@monjalon.net>
> ---
> doc/guides/prog_guide/rte_security.rst | 9 +++---
> drivers/crypto/octeontx2/otx2_cryptodev_sec.c | 5 ++-
> drivers/net/ixgbe/ixgbe_ipsec.c | 5 ++-
> drivers/net/ixgbe/ixgbe_rxtx.c | 6 ++--
> drivers/net/octeontx2/otx2_ethdev.h | 1 +
> drivers/net/octeontx2/otx2_ethdev_sec.c | 5 ++-
> drivers/net/octeontx2/otx2_ethdev_sec_tx.h | 2 +-
> drivers/net/octeontx2/otx2_rx.h | 2 +-
> examples/ipsec-secgw/ipsec-secgw.c | 9 +++---
> examples/ipsec-secgw/ipsec_worker.c | 12 ++++---
> lib/librte_security/rte_security.c | 22 +++++++++++++
> lib/librte_security/rte_security.h | 32 +++++++++++++++++++
> lib/librte_security/rte_security_driver.h | 3 ++
> lib/librte_security/version.map | 3 ++
> 14 files changed, 96 insertions(+), 20 deletions(-)
>
<...>
> --- a/examples/ipsec-secgw/ipsec_worker.c
> +++ b/examples/ipsec-secgw/ipsec_worker.c
> @@ -208,7 +208,7 @@ process_ipsec_ev_inbound(struct ipsec_ctx *ctx, struct
> route_table *rt,
> "Inbound security offload failed\n");
> goto drop_pkt_and_exit;
> }
> - sa = pkt->userdata;
> + sa = *(struct ipsec_sa **)rte_security_dynfield(pkt);
> }
>
> /* Check if we have a match */
> @@ -226,7 +226,7 @@ process_ipsec_ev_inbound(struct ipsec_ctx *ctx, struct
> route_table *rt,
> "Inbound security offload failed\n");
> goto drop_pkt_and_exit;
> }
> - sa = pkt->userdata;
> + sa = *(struct ipsec_sa **)rte_security_dynfield(pkt);
> }
>
> /* Check if we have a match */
> @@ -357,7 +357,8 @@ process_ipsec_ev_outbound(struct ipsec_ctx *ctx, struct
> route_table *rt,
> }
>
> if (sess->security.ol_flags & RTE_SECURITY_TX_OLOAD_NEED_MDATA)
> - pkt->userdata = sess->security.ses;
> + *(struct rte_security_session **)rte_security_dynfield(pkt) =
> + sess->security.ses;
>
> /* Mark the packet for Tx security offload */
> pkt->ol_flags |= PKT_TX_SEC_OFFLOAD;
> @@ -465,7 +466,10 @@ ipsec_wrkr_non_burst_int_port_drv_mode(struct
> eh_event_link_info *links,
> }
>
> /* Save security session */
> - pkt->userdata = sess_tbl[port_id];
> + if (rte_security_dynfield_is_registered())
> + *(struct rte_security_session **)
> + rte_security_dynfield(pkt) =
> + sess_tbl[port_id];
>
Maybe the last 2 lines can be on the same line (a bit more than 80,
but less than 100 chars).
This is not clear to me why you need to call
rte_security_dynfield_is_registered() here, and not in other places.
Would it make sense instead to always register the dynfield at some
place in rte_security, so that we are sure that as soon as we use
rte_security, the dynfield is registered. A good place would be an init
function, but I don't see one.
> /* Mark the packet for Tx security offload */
> pkt->ol_flags |= PKT_TX_SEC_OFFLOAD;
> diff --git a/lib/librte_security/rte_security.c
> b/lib/librte_security/rte_security.c
> index ee4666026a..4fb0b797e9 100644
> --- a/lib/librte_security/rte_security.c
> +++ b/lib/librte_security/rte_security.c
> @@ -23,6 +23,28 @@
> RTE_PTR_OR_ERR_RET(p1->p2->p3, last_retval); \
> } while (0)
>
> +#define RTE_SECURITY_DYNFIELD_NAME "rte_security_dynfield_metadata"
> +int rte_security_dynfield_offset = -1;
> +
> +int
> +rte_security_dynfield_register(void)
> +{
> + static const struct rte_mbuf_dynfield dynfield_desc = {
> + .name = RTE_SECURITY_DYNFIELD_NAME,
> + .size = sizeof(RTE_SECURITY_DYNFIELD_TYPE),
> + .align = __alignof__(RTE_SECURITY_DYNFIELD_TYPE),
> + };
> + rte_security_dynfield_offset =
> + rte_mbuf_dynfield_register(&dynfield_desc);
> + return rte_security_dynfield_offset;
> +}
> +
> +bool
> +rte_security_dynfield_is_registered(void)
> +{
> + return rte_security_dynfield_offset >= 0;
> +}
> +
Wouldn't it be better to have it in a static inline function?
The point is just to check that offset is >= 0, and it is used
in data path.
> struct rte_security_session *
> rte_security_session_create(struct rte_security_ctx *instance,
> struct rte_security_session_conf *conf,
> diff --git a/lib/librte_security/rte_security.h
> b/lib/librte_security/rte_security.h
> index 271531af12..7fbdee99cc 100644
> --- a/lib/librte_security/rte_security.h
> +++ b/lib/librte_security/rte_security.h
> @@ -27,6 +27,7 @@ extern "C" {
> #include <rte_common.h>
> #include <rte_crypto.h>
> #include <rte_mbuf.h>
> +#include <rte_mbuf_dyn.h>
> #include <rte_memory.h>
> #include <rte_mempool.h>
>
> @@ -451,6 +452,37 @@ int
> rte_security_session_destroy(struct rte_security_ctx *instance,
> struct rte_security_session *sess);
>
> +/** Device-specific metadata field type */
> +#define RTE_SECURITY_DYNFIELD_TYPE uint64_t
What about using a typedef instead of a #define?
It could be in lowercase: rte_security_dynfield_t
