On Tue, Jul 06, 2021 at 12:42:34PM +0000, Ananyev, Konstantin wrote:
>
> > On Tue, Jul 06, 2021 at 10:56:10AM +0000, Ananyev, Konstantin wrote:
> > >
> > > >
> > > > From: Nithin Dabilpuram <ndabilpu...@marvell.com>
> > > >
> > > > For Tx inline processing, when RTE_SECURITY_TX_OLOAD_NEED_MDATA is
> > > > set, rte_security_set_pkt_metadata() needs to be called for pkts
> > > > to associate a Security session with a mbuf before submitting
> > > > to Ethdev Tx. This is apart from setting PKT_TX_SEC_OFFLOAD in
> > > > mbuf.ol_flags. rte_security_set_pkt_metadata() is also used to
> > > > set some opaque metadata in mbuf for PMD's use.
> > > > This patch updates documentation that rte_security_set_pkt_metadata()
> > > > should be called only with mbuf containing Layer 3 and above data.
> > > > This behaviour is consistent with existing PMD's such as ixgbe.
> > > >
> > > > On Tx, not all net PMD's/HW can parse packet and identify
> > > > L2 header and L3 header locations on Tx. This is inline with other
> > > > Tx offloads requirements such as L3 checksum, L4 checksum offload,
> > > > etc, where mbuf.l2_len, mbuf.l3_len etc, needs to be set for
> > > > HW to be able to generate checksum. Since Inline IPSec is also
> > > > such a Tx offload, some PMD's at least need mbuf.l2_len to be
> > > > valid to find L3 header and perform Outbound IPSec processing.
> > > > Hence, this patch updates documentation to enforce setting
> > > > mbuf.l2_len while setting PKT_TX_SEC_OFFLOAD in mbuf.ol_flags
> > > > for Inline IPSec Crypto / Protocol offload processing to
> > > > work on Tx.
> > > >
> > > > Signed-off-by: Nithin Dabilpuram <ndabilpu...@marvell.com>
> > > > Reviewed-by: Akhil Goyal <gak...@marvell.com>
> > > > ---
> > > > doc/guides/nics/features.rst | 2 ++
> > > > doc/guides/prog_guide/rte_security.rst | 6 +++++-
> > > > lib/mbuf/rte_mbuf_core.h | 2 ++
> > > > 3 files changed, 9 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/doc/guides/nics/features.rst b/doc/guides/nics/features.rst
> > > > index 403c2b03a..414baf14f 100644
> > > > --- a/doc/guides/nics/features.rst
> > > > +++ b/doc/guides/nics/features.rst
> > > > @@ -430,6 +430,7 @@ of protocol operations. See Security library and
> > > > PMD documentation for more deta
> > > >
> > > > * **[uses] rte_eth_rxconf,rte_eth_rxmode**:
> > > > ``offloads:DEV_RX_OFFLOAD_SECURITY``,
> > > > * **[uses] rte_eth_txconf,rte_eth_txmode**:
> > > > ``offloads:DEV_TX_OFFLOAD_SECURITY``.
> > > > +* **[uses] mbuf**: ``mbuf.l2_len``.
> > > > * **[implements] rte_security_ops**: ``session_create``,
> > > > ``session_update``,
> > > > ``session_stats_get``, ``session_destroy``, ``set_pkt_metadata``,
> > > > ``capabilities_get``.
> > > > * **[provides] rte_eth_dev_info**:
> > > > ``rx_offload_capa,rx_queue_offload_capa:DEV_RX_OFFLOAD_SECURITY``,
> > > > @@ -451,6 +452,7 @@ protocol operations. See security library and PMD
> > > > documentation for more details
> > > >
> > > > * **[uses] rte_eth_rxconf,rte_eth_rxmode**:
> > > > ``offloads:DEV_RX_OFFLOAD_SECURITY``,
> > > > * **[uses] rte_eth_txconf,rte_eth_txmode**:
> > > > ``offloads:DEV_TX_OFFLOAD_SECURITY``.
> > > > +* **[uses] mbuf**: ``mbuf.l2_len``.
> > > > * **[implements] rte_security_ops**: ``session_create``,
> > > > ``session_update``,
> > > > ``session_stats_get``, ``session_destroy``, ``set_pkt_metadata``,
> > > > ``get_userdata``,
> > > > ``capabilities_get``.
> > > > diff --git a/doc/guides/prog_guide/rte_security.rst
> > > > b/doc/guides/prog_guide/rte_security.rst
> > > > index f72bc8a78..7b68c698d 100644
> > > > --- a/doc/guides/prog_guide/rte_security.rst
> > > > +++ b/doc/guides/prog_guide/rte_security.rst
> > > > @@ -560,7 +560,11 @@ created by the application is attached to the
> > > > security session by the API
> > > >
> > > > For Inline Crypto and Inline protocol offload, device specific defined
> > > > metadata is
> > > > updated in the mbuf using ``rte_security_set_pkt_metadata()`` if
> > > > -``DEV_TX_OFFLOAD_SEC_NEED_MDATA`` is set.
> > > > +``RTE_SECURITY_TX_OLOAD_NEED_MDATA`` is set.
> > > > ``rte_security_set_pkt_metadata()``
> > > > +should be called on mbuf only with Layer 3 and above data present and
> > > > +``mbuf.data_off`` should be pointing to Layer 3 Header.
> > >
> > > Hmm... not sure why mbuf.data_off should point to L3 hdr.
> > > Who will add L2 hdr to the packet in that case?
> > > Or did you mean ``mbuf.data_off + mbuf.l2_len`` here?
> >
> > That is the semantics I was trying to define. I think below are the
> > sequence of
> > operations to be done for ipsec processing,
> >
> > 1. receive_pkt()
> > 2. strip_l2_hdr()
> > 3. Do policy lookup ()
> > 4. Call rte_security_set_pkt_metadata() if pkt needs to be encrypted with a
> > particular SA. Now pkt only has L3 and above data.
> > 5. Do route_lookup()
> > 6. add_l2hdr() which might be different from stripped l2hdr.
> > 7. Send packet out.
> >
> > The above sequence is what I believe the current poll mode worker thread in
> > ipsec-secgw is following.
>
> That's just a sample app, it doesn't mean it has to be the only possible way.
>
> > While in event mode, step 2 and step 6 are missing.
>
> I think this L2 hdr manipulation is totally optional.
> If your rte_security_set_pkt_metadata() implementation really needs to know
> L3 hdr offset (not sure why?),
Since rte_security_set_pkt_metadata() is PMD specific function ptr call, we are
currently doing some pre-processing
here before submitting packet to inline IPSec via rte_eth_tx_burst(). This
saves us cycles later in rte_eth_tx_burst().
If we cannot know for sure, the pkt content at the time of
rte_security_set_pkt_metadata() call, then I think
having a PMD specific callback is not much of use except for saving SA priv
data to rte_mbuf.
> then I suppose we can add a requirement that l2_len has to be set properly
> before calling rte_security_set_pkt_metadata().
This is also fine with us.
>
> >
> > This patch is trying to enforce semantics as above so that
> > rte_security_set_pkt_metadata() can predict what comes in the pkt when he is
> > called.
> >
> > I also think above sequence is what Linux kernel stack or other stacks
> > follow.
> > Does it makes sense ?
> >
> > >
> > > > Once called,
> > > > +Layer 3 and above data cannot be modified or moved around unless
> > > > +``rte_security_set_pkt_metadata()`` is called again.
> > > >
> > > > For inline protocol offloaded ingress traffic, the application can
> > > > register a
> > > > pointer, ``userdata`` , in the security session. When the packet is
> > > > received,
> > > > diff --git a/lib/mbuf/rte_mbuf_core.h b/lib/mbuf/rte_mbuf_core.h
> > > > index bb38d7f58..9d8e3ddc8 100644
> > > > --- a/lib/mbuf/rte_mbuf_core.h
> > > > +++ b/lib/mbuf/rte_mbuf_core.h
> > > > @@ -228,6 +228,8 @@ extern "C" {
> > > >
> > > > /**
> > > > * Request security offload processing on the TX packet.
> > > > + * To use Tx security offload, the user needs to fill l2_len in mbuf
> > > > + * indicating L2 header size and where L3 header starts.
> > > > */
> > > > #define PKT_TX_SEC_OFFLOAD (1ULL << 43)
> > > >
> > > > --
> > > > 2.25.1
> > >
