On 10/26/20 8:20 AM, 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>
[snip]
> diff --git a/drivers/net/ixgbe/ixgbe_ipsec.c b/drivers/net/ixgbe/ixgbe_ipsec.c
> index 48f5082d49..0232db20ed 100644
> --- a/drivers/net/ixgbe/ixgbe_ipsec.c
> +++ b/drivers/net/ixgbe/ixgbe_ipsec.c
> @@ -484,7 +484,8 @@ ixgbe_crypto_update_mb(void *device __rte_unused,
> get_sec_session_private_data(session);
> if (ic_session->op == IXGBE_OP_AUTHENTICATED_ENCRYPTION) {
> union ixgbe_crypto_tx_desc_md *mdata =
> - (union ixgbe_crypto_tx_desc_md *)&m->udata64;
> + (union ixgbe_crypto_tx_desc_md *)
> + rte_security_dynfield(m);
IMHO alignment looks a bit confusing here, may be add one more
TAB?
> mdata->enc = 1;
> mdata->sa_idx = ic_session->sa_index;
> mdata->pad_len = ixgbe_crypto_compute_pad_len(m);
> @@ -751,5 +752,7 @@ ixgbe_ipsec_ctx_create(struct rte_eth_dev *dev)
> return -ENOMEM;
> }
> }
> + if (rte_security_dynfield_register() < 0)
> + return -rte_errno;
> return 0;
> }
[snip]
> diff --git a/examples/ipsec-secgw/ipsec_worker.c
> b/examples/ipsec-secgw/ipsec_worker.c
> index b6c851f257..72f698893d 100644
> --- a/examples/ipsec-secgw/ipsec_worker.c
> +++ b/examples/ipsec-secgw/ipsec_worker.c
> @@ -208,7 +208,8 @@ 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 = RTE_MBUF_DYNFIELD(pkt, security_dynfield_offset,
> + struct ipsec_sa *);
I think it should be de-reference above, i.e.
sa = (struct ipsec_sa *)*RTE_MBUF_DYNFIELD(pkt,
security_dynfield_offset, RTE_SECURITY_DYNFIELD_TYPE *);
or just
sa = *RTE_MBUF_DYNFIELD(pkt, security_dynfield_offset, struct ipsec_sa **);
and why not rte_security_dynfield()?
It all looks very fragile. May be at least add
RTE_BUILD_BUG_ON(sizeof(RTE_SECURITY_DYNFIELD_TYPE) ==
sizeof(ipsec_sa *));
and similar checks when an application or a library does
lookup for a dynamic field.
In general since lookup should not happen on data path,
may be lookup should return size of the field which must
be checked by the caller for consistency.
> }
>
> /* Check if we have a match */
> @@ -226,7 +227,8 @@ 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 = RTE_MBUF_DYNFIELD(pkt, security_dynfield_offset,
> + struct ipsec_sa *);
same
> }
>
> /* Check if we have a match */
> @@ -357,7 +359,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;
> + *RTE_MBUF_DYNFIELD(pkt, security_dynfield_offset,
> + struct rte_security_session **) = sess->security.ses;
rte_security_dynfield() ?
Is it really different types of value in one example
application? It looks like it should be different
dynamic fields. Otherwise, I don't understand how
to work with it.
In fact may be above is out of scope of the patch series...
>
> /* Mark the packet for Tx security offload */
> pkt->ol_flags |= PKT_TX_SEC_OFFLOAD;
> @@ -465,7 +468,9 @@ ipsec_wrkr_non_burst_int_port_drv_mode(struct
> eh_event_link_info *links,
> }
>
> /* Save security session */
> - pkt->userdata = sess_tbl[port_id];
> + *RTE_MBUF_DYNFIELD(pkt, security_dynfield_offset,
> + struct rte_security_session **) =
> + sess_tbl[port_id];
rte_security_dynfield() ?
>
> /* Mark the packet for Tx security offload */
> pkt->ol_flags |= PKT_TX_SEC_OFFLOAD;
[snip]
