[1/2] security: enforce semantics for Tx inline processing
Checks
Commit Message
From: Nithin Dabilpuram <ndabilpuram@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 <ndabilpuram@marvell.com>
Reviewed-by: Akhil Goyal <gakhil@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(-)
Comments
Hi Konstantin/ Olivier,
Could you please review this patch? This has also an update in documentation of mbuf.
Regards,
Akhil
> From: Nithin Dabilpuram <ndabilpuram@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 <ndabilpuram@marvell.com>
> Reviewed-by: Akhil Goyal <gakhil@marvell.com>
>
> From: Nithin Dabilpuram <ndabilpuram@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 <ndabilpuram@marvell.com>
> Reviewed-by: Akhil Goyal <gakhil@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?
> 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
On Tue, Jul 06, 2021 at 10:56:10AM +0000, Ananyev, Konstantin wrote:
>
> >
> > From: Nithin Dabilpuram <ndabilpuram@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 <ndabilpuram@marvell.com>
> > Reviewed-by: Akhil Goyal <gakhil@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. While in event mode, step 2 and step 6 are missing.
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
>
> On Tue, Jul 06, 2021 at 10:56:10AM +0000, Ananyev, Konstantin wrote:
> >
> > >
> > > From: Nithin Dabilpuram <ndabilpuram@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 <ndabilpuram@marvell.com>
> > > Reviewed-by: Akhil Goyal <gakhil@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?),
then I suppose we can add a requirement that l2_len has to be set properly before calling rte_security_set_pkt_metadata().
>
> 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
> >
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 <ndabilpuram@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 <ndabilpuram@marvell.com>
> > > > Reviewed-by: Akhil Goyal <gakhil@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
> > >
> > > > > 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 <ndabilpuram@marvell.com>
> > > > > Reviewed-by: Akhil Goyal <gakhil@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.
Ok, so to make sure we are on the same page, you propose:
1. before calling rte_security_set_pkt_metadata() mbuf.l2_len should be properly set.
2. after rte_security_set_pkt_metadata() and before rte_eth_tx_burst() packet contents
at [mbuf.l2_len, mbuf.pkt_len) can't be modified?
Is that correct understanding?
If yes, I wonder how 2) will correlate with rte_eth_tx_prepare() concept?
> >
> > >
> > > 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
> > > >
On Tue, Jul 06, 2021 at 02:07:17PM +0000, Ananyev, Konstantin wrote:
>
>
> > > > > > 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 <ndabilpuram@marvell.com>
> > > > > > Reviewed-by: Akhil Goyal <gakhil@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.
>
> Ok, so to make sure we are on the same page, you propose:
> 1. before calling rte_security_set_pkt_metadata() mbuf.l2_len should be properly set.
> 2. after rte_security_set_pkt_metadata() and before rte_eth_tx_burst() packet contents
> at [mbuf.l2_len, mbuf.pkt_len) can't be modified?
Yes.
>
> Is that correct understanding?
> If yes, I wonder how 2) will correlate with rte_eth_tx_prepare() concept?
Since our PMD doesn't have a prepare function, I missed that but, since
rte_security_set_pkt_metadata() is only used for Inline Crypto/Protocol via
a rte_eth_dev, and both rte_security_set_pkt_metadata() and rte_eth_tx_prepare()
are callbacks from same PMD, do you see any issue ?
The restriction is from user side, data is not supposed to be modified unless
rte_security_set_pkt_metadata() is called again.
If your question is can't we do the preprocessing in rte_eth_tx_prepare() for
security, my only argument was that since there is already a hit in
rte_security_set_pkt_metadata() to PMD specific callback and
struct rte_security_session is passed as an argument to it, it is more benefitial to
do security related pre-processing there.
Also rte_eth_tx_prepare() if implemented will be called for both security and
non-security pkts.
>
> > >
> > > >
> > > > 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
> > > > >
> > > > > > > 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 <ndabilpuram@marvell.com>
> > > > > > > Reviewed-by: Akhil Goyal <gakhil@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.
> >
> > Ok, so to make sure we are on the same page, you propose:
> > 1. before calling rte_security_set_pkt_metadata() mbuf.l2_len should be properly set.
> > 2. after rte_security_set_pkt_metadata() and before rte_eth_tx_burst() packet contents
> > at [mbuf.l2_len, mbuf.pkt_len) can't be modified?
> Yes.
>
> >
> > Is that correct understanding?
> > If yes, I wonder how 2) will correlate with rte_eth_tx_prepare() concept?
>
> Since our PMD doesn't have a prepare function, I missed that but, since
> rte_security_set_pkt_metadata() is only used for Inline Crypto/Protocol via
> a rte_eth_dev, and both rte_security_set_pkt_metadata() and rte_eth_tx_prepare()
> are callbacks from same PMD, do you see any issue ?
>
> The restriction is from user side, data is not supposed to be modified unless
> rte_security_set_pkt_metadata() is called again.
Yep, I do have a concern here.
Right now it is perfectly valid to do something like that:
rte_security_set_pkt_metadata(..., mb, ...);
/* can modify contents of the packet */
rte_eth_tx_prepare(..., &mb, 1);
rte_eth_tx_burst(..., &mb, 1);
With the new restrictions you are proposing it wouldn't be allowed any more.
>
> If your question is can't we do the preprocessing in rte_eth_tx_prepare() for
> security,
Yes, that was my thought.
> my only argument was that since there is already a hit in
> rte_security_set_pkt_metadata() to PMD specific callback and
> struct rte_security_session is passed as an argument to it, it is more benefitial to
> do security related pre-processing there.
Yes, it would be extra callback call that way.
Though tx_prepare() accepts burst of packets, so the overhead
of function call will be spread around the whole burst, and I presume
shouldn't be too high.
> Also rte_eth_tx_prepare() if implemented will be called for both security and
> non-security pkts.
Yes, but tx_prepare() can distinguish (by ol_flags and/or other field contents) which
modifications are required for the packet.
>
> >
> > > >
> > > > >
> > > > > 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
> > > > > >
On Wed, Jul 07, 2021 at 09:59:10AM +0000, Ananyev, Konstantin wrote:
>
> > > > > > > > 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 <ndabilpuram@marvell.com>
> > > > > > > > Reviewed-by: Akhil Goyal <gakhil@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.
> > >
> > > Ok, so to make sure we are on the same page, you propose:
> > > 1. before calling rte_security_set_pkt_metadata() mbuf.l2_len should be properly set.
> > > 2. after rte_security_set_pkt_metadata() and before rte_eth_tx_burst() packet contents
> > > at [mbuf.l2_len, mbuf.pkt_len) can't be modified?
> > Yes.
> >
> > >
> > > Is that correct understanding?
> > > If yes, I wonder how 2) will correlate with rte_eth_tx_prepare() concept?
> >
> > Since our PMD doesn't have a prepare function, I missed that but, since
> > rte_security_set_pkt_metadata() is only used for Inline Crypto/Protocol via
> > a rte_eth_dev, and both rte_security_set_pkt_metadata() and rte_eth_tx_prepare()
> > are callbacks from same PMD, do you see any issue ?
> >
> > The restriction is from user side, data is not supposed to be modified unless
> > rte_security_set_pkt_metadata() is called again.
>
> Yep, I do have a concern here.
> Right now it is perfectly valid to do something like that:
> rte_security_set_pkt_metadata(..., mb, ...);
> /* can modify contents of the packet */
> rte_eth_tx_prepare(..., &mb, 1);
> rte_eth_tx_burst(..., &mb, 1);
>
> With the new restrictions you are proposing it wouldn't be allowed any more.
You can still modify L2 header and IPSEC is only concerned about L3 and above.
I think insisting that rte_security_set_pkt_metadata() be called after all L3
and above header modifications is no a problem. I guess existing ixgbe/txgbe
PMD which are the ones only implementing the call back are already expecting the
same ?
>
> >
> > If your question is can't we do the preprocessing in rte_eth_tx_prepare() for
> > security,
>
> Yes, that was my thought.
>
> > my only argument was that since there is already a hit in
> > rte_security_set_pkt_metadata() to PMD specific callback and
> > struct rte_security_session is passed as an argument to it, it is more benefitial to
> > do security related pre-processing there.
>
> Yes, it would be extra callback call that way.
> Though tx_prepare() accepts burst of packets, so the overhead
> of function call will be spread around the whole burst, and I presume
> shouldn't be too high.
>
> > Also rte_eth_tx_prepare() if implemented will be called for both security and
> > non-security pkts.
>
> Yes, but tx_prepare() can distinguish (by ol_flags and/or other field contents) which
> modifications are required for the packet.
But the major issues I see are
1. tx_prepare() doesn't take rte_security_session as argument though ol_flags has security flag.
In our case, we need to know the security session details to do things.
2. AFAIU tx_prepare() is not mandatory as per spec and even by default disabled under compile time
macro RTE_ETHDEV_TX_PREPARE_NOOP.
3. Even if we do tx_prepare(), rte_security_set_pkt_mdata() is mandatory to associate
struct rte_security_session to a pkt as unlike ol_flags, there is no direct space to do the same.
So I think instead of enforcing yet another callback tx_prepare() for inline security
processing, it can be done via security specific set_pkt_metadata(). I'm fine to
introduce a burst call for the same(I was thinking to propose it in future) to
compensate for the overhead.
If rte_security_set_pkt_metadata() was not a PMD specific function ptr call and
rte_mbuf had space for struct rte_security_session pointer,
then then I guess it would have been better to do the way you proposed.
>
> >
> > >
> > > > >
> > > > > >
> > > > > > 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
> > > > > > >
> > > > > > > > > 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 <ndabilpuram@marvell.com>
> > > > > > > > > Reviewed-by: Akhil Goyal <gakhil@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.
> > > >
> > > > Ok, so to make sure we are on the same page, you propose:
> > > > 1. before calling rte_security_set_pkt_metadata() mbuf.l2_len should be properly set.
> > > > 2. after rte_security_set_pkt_metadata() and before rte_eth_tx_burst() packet contents
> > > > at [mbuf.l2_len, mbuf.pkt_len) can't be modified?
> > > Yes.
> > >
> > > >
> > > > Is that correct understanding?
> > > > If yes, I wonder how 2) will correlate with rte_eth_tx_prepare() concept?
> > >
> > > Since our PMD doesn't have a prepare function, I missed that but, since
> > > rte_security_set_pkt_metadata() is only used for Inline Crypto/Protocol via
> > > a rte_eth_dev, and both rte_security_set_pkt_metadata() and rte_eth_tx_prepare()
> > > are callbacks from same PMD, do you see any issue ?
> > >
> > > The restriction is from user side, data is not supposed to be modified unless
> > > rte_security_set_pkt_metadata() is called again.
> >
> > Yep, I do have a concern here.
> > Right now it is perfectly valid to do something like that:
> > rte_security_set_pkt_metadata(..., mb, ...);
> > /* can modify contents of the packet */
> > rte_eth_tx_prepare(..., &mb, 1);
> > rte_eth_tx_burst(..., &mb, 1);
> >
> > With the new restrictions you are proposing it wouldn't be allowed any more.
> You can still modify L2 header and IPSEC is only concerned about L3 and above.
>
> I think insisting that rte_security_set_pkt_metadata() be called after all L3
> and above header modifications is no a problem. I guess existing ixgbe/txgbe
> PMD which are the ones only implementing the call back are already expecting the
> same ?
AFAIK, no there are no such requirements for ixgbe or txgbe.
All that ixgbe callback does - store session related data inside mbuf.
It's only expectation to have ESP trailer at the proper place (after ICV):
union ixgbe_crypto_tx_desc_md *mdata = (union ixgbe_crypto_tx_desc_md *)
rte_security_dynfield(m);
mdata->enc = 1;
mdata->sa_idx = ic_session->sa_index;
mdata->pad_len = ixgbe_crypto_compute_pad_len(m);
Then this data will be used by tx_burst() function.
>
> >
> > >
> > > If your question is can't we do the preprocessing in rte_eth_tx_prepare() for
> > > security,
> >
> > Yes, that was my thought.
> >
> > > my only argument was that since there is already a hit in
> > > rte_security_set_pkt_metadata() to PMD specific callback and
> > > struct rte_security_session is passed as an argument to it, it is more benefitial to
> > > do security related pre-processing there.
> >
> > Yes, it would be extra callback call that way.
> > Though tx_prepare() accepts burst of packets, so the overhead
> > of function call will be spread around the whole burst, and I presume
> > shouldn't be too high.
> >
> > > Also rte_eth_tx_prepare() if implemented will be called for both security and
> > > non-security pkts.
> >
> > Yes, but tx_prepare() can distinguish (by ol_flags and/or other field contents) which
> > modifications are required for the packet.
>
> But the major issues I see are
>
> 1. tx_prepare() doesn't take rte_security_session as argument though ol_flags has security flag.
> In our case, we need to know the security session details to do things.
I suppose you can store pointer to session (or so) inside mbuf in rte_security_dynfield, no?
> 2. AFAIU tx_prepare() is not mandatory as per spec and even by default disabled under compile time
> macro RTE_ETHDEV_TX_PREPARE_NOOP.
> 3. Even if we do tx_prepare(), rte_security_set_pkt_mdata() is mandatory to associate
> struct rte_security_session to a pkt as unlike ol_flags, there is no direct space to do the same.
Didn't get you here, obviously we do have rte_security_dynfield inside mbuf,
specially for that - to store secuiryt related data inside the mbuf.
Yes your PMD has to request it at initialization time, but I suppose it is not a big deal.
> So I think instead of enforcing yet another callback tx_prepare() for inline security
> processing, it can be done via security specific set_pkt_metadata().
But what you proposing introduces new limitations and might existing functionality.
BTW, if you don't like to use tx_prepare() - why doing these calculations inside tx_burst()
itself is not an option?
> I'm fine to
> introduce a burst call for the same(I was thinking to propose it in future) to
> compensate for the overhead.
>
> If rte_security_set_pkt_metadata() was not a PMD specific function ptr call and
> rte_mbuf had space for struct rte_security_session pointer,
But it does, see above.
In fact it even more flexible - because it is driver specific, you are not limited to one 64-bit field.
If your PMD requires more data to be associated with mbuf
- you can request it via mbuf_dynfield and store there whatever is needed.
> then then I guess it would have been better to do the way you proposed.
>
> >
> > >
> > > >
> > > > > >
> > > > > > >
> > > > > > > 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
> > > > > > > >
On Sat, Jul 10, 2021 at 12:57:19PM +0000, Ananyev, Konstantin wrote:
>
> > > > > > > > > > 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 <ndabilpuram@marvell.com>
> > > > > > > > > > Reviewed-by: Akhil Goyal <gakhil@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.
> > > > >
> > > > > Ok, so to make sure we are on the same page, you propose:
> > > > > 1. before calling rte_security_set_pkt_metadata() mbuf.l2_len should be properly set.
> > > > > 2. after rte_security_set_pkt_metadata() and before rte_eth_tx_burst() packet contents
> > > > > at [mbuf.l2_len, mbuf.pkt_len) can't be modified?
> > > > Yes.
> > > >
> > > > >
> > > > > Is that correct understanding?
> > > > > If yes, I wonder how 2) will correlate with rte_eth_tx_prepare() concept?
> > > >
> > > > Since our PMD doesn't have a prepare function, I missed that but, since
> > > > rte_security_set_pkt_metadata() is only used for Inline Crypto/Protocol via
> > > > a rte_eth_dev, and both rte_security_set_pkt_metadata() and rte_eth_tx_prepare()
> > > > are callbacks from same PMD, do you see any issue ?
> > > >
> > > > The restriction is from user side, data is not supposed to be modified unless
> > > > rte_security_set_pkt_metadata() is called again.
> > >
> > > Yep, I do have a concern here.
> > > Right now it is perfectly valid to do something like that:
> > > rte_security_set_pkt_metadata(..., mb, ...);
> > > /* can modify contents of the packet */
> > > rte_eth_tx_prepare(..., &mb, 1);
> > > rte_eth_tx_burst(..., &mb, 1);
> > >
> > > With the new restrictions you are proposing it wouldn't be allowed any more.
> > You can still modify L2 header and IPSEC is only concerned about L3 and above.
> >
> > I think insisting that rte_security_set_pkt_metadata() be called after all L3
> > and above header modifications is no a problem. I guess existing ixgbe/txgbe
> > PMD which are the ones only implementing the call back are already expecting the
> > same ?
>
> AFAIK, no there are no such requirements for ixgbe or txgbe.
> All that ixgbe callback does - store session related data inside mbuf.
> It's only expectation to have ESP trailer at the proper place (after ICV):
This implies rte_security_set_pkt_metadata() cannot be called when mbuf does't
have ESP trailer updated or when mbuf->pkt_len = 0
>
> union ixgbe_crypto_tx_desc_md *mdata = (union ixgbe_crypto_tx_desc_md *)
> rte_security_dynfield(m);
> mdata->enc = 1;
> mdata->sa_idx = ic_session->sa_index;
> mdata->pad_len = ixgbe_crypto_compute_pad_len(m);
>
> Then this data will be used by tx_burst() function.
So it implies that after above rte_security_set_pkt_metadata() call, and before tx_burst(),
mbuf data / packet len cannot be modified right as if modified, then tx_burst()
will be using incorrect pad len ?
This patch is also trying to add similar restriction on when
rte_security_set_pkt_metadata() should be called and what cannot be done after
calling rte_security_set_pkt_metadata().
>
> >
> > >
> > > >
> > > > If your question is can't we do the preprocessing in rte_eth_tx_prepare() for
> > > > security,
> > >
> > > Yes, that was my thought.
> > >
> > > > my only argument was that since there is already a hit in
> > > > rte_security_set_pkt_metadata() to PMD specific callback and
> > > > struct rte_security_session is passed as an argument to it, it is more benefitial to
> > > > do security related pre-processing there.
> > >
> > > Yes, it would be extra callback call that way.
> > > Though tx_prepare() accepts burst of packets, so the overhead
> > > of function call will be spread around the whole burst, and I presume
> > > shouldn't be too high.
> > >
> > > > Also rte_eth_tx_prepare() if implemented will be called for both security and
> > > > non-security pkts.
> > >
> > > Yes, but tx_prepare() can distinguish (by ol_flags and/or other field contents) which
> > > modifications are required for the packet.
> >
> > But the major issues I see are
> >
> > 1. tx_prepare() doesn't take rte_security_session as argument though ol_flags has security flag.
> > In our case, we need to know the security session details to do things.
>
> I suppose you can store pointer to session (or so) inside mbuf in rte_security_dynfield, no?
We can do. But having to call PMD specific function call via rte_security_set_pkt_metadata()
just for storing session pointer in rte_security_dynfield consumes unnecessary
cycles per pkt.
>
> > 2. AFAIU tx_prepare() is not mandatory as per spec and even by default disabled under compile time
> > macro RTE_ETHDEV_TX_PREPARE_NOOP.
> > 3. Even if we do tx_prepare(), rte_security_set_pkt_mdata() is mandatory to associate
> > struct rte_security_session to a pkt as unlike ol_flags, there is no direct space to do the same.
>
> Didn't get you here, obviously we do have rte_security_dynfield inside mbuf,
> specially for that - to store secuiryt related data inside the mbuf.
> Yes your PMD has to request it at initialization time, but I suppose it is not a big deal.
>
> > So I think instead of enforcing yet another callback tx_prepare() for inline security
> > processing, it can be done via security specific set_pkt_metadata().
>
> But what you proposing introduces new limitations and might existing functionality.
> BTW, if you don't like to use tx_prepare() - why doing these calculations inside tx_burst()
> itself is not an option?
We can do things in tx_burst() but if we are doing it there, then we want to avoid having callback for
rte_security_set_pkt_metadata().
Are you fine if we can update the spec that "When DEV_TX_OFFLOAD_SEC_NEED_MDATA is not
set, then, user needs to update struct rte_security_session's sess_private_data in a in
rte_security_dynfield like below ?
<snip>
static inline void
inline_outb_mbuf_prepare(const struct rte_ipsec_session *ss,
struct rte_mbuf *mb[], uint16_t num)
{
uint32_t i, ol_flags;
ol_flags = ss->security.ol_flags & RTE_SECURITY_TX_OLOAD_NEED_MDATA;
for (i = 0; i != num; i++) {
mb[i]->ol_flags |= PKT_TX_SEC_OFFLOAD;
if (ol_flags != 0)
rte_security_set_pkt_metadata(ss->security.ctx,
ss->security.ses, mb[i], NULL);
else
*rte_security_dynfield(mb[i]) =
(uint64_t)ss->security.ses->sess_private_data;
If the above can be done, then in our PMD, we will not have a callback for
set_pkt_metadata() and DEV_TX_OFFLOAD_SEC_NEED_MDATA will also be not set
in capabilities.
>
> > I'm fine to
> > introduce a burst call for the same(I was thinking to propose it in future) to
> > compensate for the overhead.
> >
> > If rte_security_set_pkt_metadata() was not a PMD specific function ptr call and
> > rte_mbuf had space for struct rte_security_session pointer,
>
> But it does, see above.
> In fact it even more flexible - because it is driver specific, you are not limited to one 64-bit field.
> If your PMD requires more data to be associated with mbuf
> - you can request it via mbuf_dynfield and store there whatever is needed.
>
> > then then I guess it would have been better to do the way you proposed.
> >
> > >
> > > >
> > > > >
> > > > > > >
> > > > > > > >
> > > > > > > > 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
> > > > > > > > >
Adding more rte_security and PMD maintainers into the loop.
> > > > > > > > > > > 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 <ndabilpuram@marvell.com>
> > > > > > > > > > > Reviewed-by: Akhil Goyal <gakhil@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.
> > > > > >
> > > > > > Ok, so to make sure we are on the same page, you propose:
> > > > > > 1. before calling rte_security_set_pkt_metadata() mbuf.l2_len should be properly set.
> > > > > > 2. after rte_security_set_pkt_metadata() and before rte_eth_tx_burst() packet contents
> > > > > > at [mbuf.l2_len, mbuf.pkt_len) can't be modified?
> > > > > Yes.
> > > > >
> > > > > >
> > > > > > Is that correct understanding?
> > > > > > If yes, I wonder how 2) will correlate with rte_eth_tx_prepare() concept?
> > > > >
> > > > > Since our PMD doesn't have a prepare function, I missed that but, since
> > > > > rte_security_set_pkt_metadata() is only used for Inline Crypto/Protocol via
> > > > > a rte_eth_dev, and both rte_security_set_pkt_metadata() and rte_eth_tx_prepare()
> > > > > are callbacks from same PMD, do you see any issue ?
> > > > >
> > > > > The restriction is from user side, data is not supposed to be modified unless
> > > > > rte_security_set_pkt_metadata() is called again.
> > > >
> > > > Yep, I do have a concern here.
> > > > Right now it is perfectly valid to do something like that:
> > > > rte_security_set_pkt_metadata(..., mb, ...);
> > > > /* can modify contents of the packet */
> > > > rte_eth_tx_prepare(..., &mb, 1);
> > > > rte_eth_tx_burst(..., &mb, 1);
> > > >
> > > > With the new restrictions you are proposing it wouldn't be allowed any more.
> > > You can still modify L2 header and IPSEC is only concerned about L3 and above.
> > >
> > > I think insisting that rte_security_set_pkt_metadata() be called after all L3
> > > and above header modifications is no a problem. I guess existing ixgbe/txgbe
> > > PMD which are the ones only implementing the call back are already expecting the
> > > same ?
> >
> > AFAIK, no there are no such requirements for ixgbe or txgbe.
> > All that ixgbe callback does - store session related data inside mbuf.
> > It's only expectation to have ESP trailer at the proper place (after ICV):
>
> This implies rte_security_set_pkt_metadata() cannot be called when mbuf does't
> have ESP trailer updated or when mbuf->pkt_len = 0
>
> >
> > union ixgbe_crypto_tx_desc_md *mdata = (union ixgbe_crypto_tx_desc_md *)
> > rte_security_dynfield(m);
> > mdata->enc = 1;
> > mdata->sa_idx = ic_session->sa_index;
> > mdata->pad_len = ixgbe_crypto_compute_pad_len(m);
> >
> > Then this data will be used by tx_burst() function.
> So it implies that after above rte_security_set_pkt_metadata() call, and before tx_burst(),
> mbuf data / packet len cannot be modified right as if modified, then tx_burst()
> will be using incorrect pad len ?
No, pkt_len can be modified.
Though ESP trailer pad_len can't.
>
> This patch is also trying to add similar restriction on when
> rte_security_set_pkt_metadata() should be called and what cannot be done after
> calling rte_security_set_pkt_metadata().
No, I don't think it is really the same.
Also, IMO, inside ixgbe set_pkt_metadata() implementaion we probably shouldn't silently imply
that ESP packet is already formed and trailer contains valid data.
In fact, I think this pad_len calculation can be moved to actual TX function.
>
> >
> > >
> > > >
> > > > >
> > > > > If your question is can't we do the preprocessing in rte_eth_tx_prepare() for
> > > > > security,
> > > >
> > > > Yes, that was my thought.
> > > >
> > > > > my only argument was that since there is already a hit in
> > > > > rte_security_set_pkt_metadata() to PMD specific callback and
> > > > > struct rte_security_session is passed as an argument to it, it is more benefitial to
> > > > > do security related pre-processing there.
> > > >
> > > > Yes, it would be extra callback call that way.
> > > > Though tx_prepare() accepts burst of packets, so the overhead
> > > > of function call will be spread around the whole burst, and I presume
> > > > shouldn't be too high.
> > > >
> > > > > Also rte_eth_tx_prepare() if implemented will be called for both security and
> > > > > non-security pkts.
> > > >
> > > > Yes, but tx_prepare() can distinguish (by ol_flags and/or other field contents) which
> > > > modifications are required for the packet.
> > >
> > > But the major issues I see are
> > >
> > > 1. tx_prepare() doesn't take rte_security_session as argument though ol_flags has security flag.
> > > In our case, we need to know the security session details to do things.
> >
> > I suppose you can store pointer to session (or so) inside mbuf in rte_security_dynfield, no?
>
> We can do. But having to call PMD specific function call via rte_security_set_pkt_metadata()
> just for storing session pointer in rte_security_dynfield consumes unnecessary
> cycles per pkt.
In fact there are two function calls: one for rte_security_set_pkt_metadata(),
second for instance->ops->set_pkt_metadata() callback.
Which off-course way too expensive for such simple operation.
Actually same thought for rte_security_get_userdata().
Both of these functions belong to data-path and ideally have to be as fast as possible.
Probably 21.11 is a right timeframe for that.
> >
> > > 2. AFAIU tx_prepare() is not mandatory as per spec and even by default disabled under compile time
> > > macro RTE_ETHDEV_TX_PREPARE_NOOP.
> > > 3. Even if we do tx_prepare(), rte_security_set_pkt_mdata() is mandatory to associate
> > > struct rte_security_session to a pkt as unlike ol_flags, there is no direct space to do the same.
> >
> > Didn't get you here, obviously we do have rte_security_dynfield inside mbuf,
> > specially for that - to store secuiryt related data inside the mbuf.
> > Yes your PMD has to request it at initialization time, but I suppose it is not a big deal.
> >
> > > So I think instead of enforcing yet another callback tx_prepare() for inline security
> > > processing, it can be done via security specific set_pkt_metadata().
> >
> > But what you proposing introduces new limitations and might existing functionality.
> > BTW, if you don't like to use tx_prepare() - why doing these calculations inside tx_burst()
> > itself is not an option?
>
> We can do things in tx_burst() but if we are doing it there, then we want to avoid having callback for
> rte_security_set_pkt_metadata().
>
> Are you fine if we can update the spec that "When DEV_TX_OFFLOAD_SEC_NEED_MDATA is not
> set, then, user needs to update struct rte_security_session's sess_private_data in a in
> rte_security_dynfield like below ?
>
> <snip>
>
> static inline void
> inline_outb_mbuf_prepare(const struct rte_ipsec_session *ss,
> struct rte_mbuf *mb[], uint16_t num)
> {
> uint32_t i, ol_flags;
>
> ol_flags = ss->security.ol_flags & RTE_SECURITY_TX_OLOAD_NEED_MDATA;
> for (i = 0; i != num; i++) {
>
> mb[i]->ol_flags |= PKT_TX_SEC_OFFLOAD;
>
> if (ol_flags != 0)
> rte_security_set_pkt_metadata(ss->security.ctx,
> ss->security.ses, mb[i], NULL);
> else
> *rte_security_dynfield(mb[i]) =
> (uint64_t)ss->security.ses->sess_private_data;
>
>
> If the above can be done, then in our PMD, we will not have a callback for
> set_pkt_metadata() and DEV_TX_OFFLOAD_SEC_NEED_MDATA will also be not set
> in capabilities.
That's an interesting idea, but what you propose is the change in current rte_security API behaviour.
So all existing apps that use this API will have to be changed.
We'd better avoid such changes unless there is really good reason for that.
So, I'd suggest to tweak your idea a bit:
1) change rte_security_set_pkt_metadata():
if ops->set_pkt_metadata != NULL, then call it (existing behaviour)
otherwise just: rte_security_dynfield(m) = sess->session_private_data;
(fast-path)
2) consider to make rte_security_set_pkt_metadata() inline function.
We probably can have some special flag inside struct rte_security_ctx,
or even store inside ctx a pointer to set_pkt_metadata() itself.
As a brief code snippet:
struct rte_security_ctx {
void *device;
/**< Crypto/ethernet device attached */
const struct rte_security_ops *ops;
/**< Pointer to security ops for the device */
uint16_t sess_cnt;
/**< Number of sessions attached to this context */
+ int (*set_pkt_mdata)(void *, struct rte_security_session *, struct rte_mbuf *, void *);
};
static inline int
rte_security_set_pkt_metadata(struct rte_security_ctx *instance,
struct rte_security_session *sess,
struct rte_mbuf *m, void *params)
{
/* fast-path */
if (instance->set_pkt_mdata == NULL) {
*rte_security_dynfield(m) = (rte_security_dynfield_t)(session->sess_priv_data);
return 0;
/* slow path */
} else
return instance->set_pkt_mdata(instance->device, sess, m, params);
}
That probably would be an ABI breakage (new fileld in rte_security_ctx) and would require
some trivial changes for all existing PMDs that use RTE_SECURITY_TX_OFLOAD_NEED_MDATA
(ctx_create()), but hopefully will benefit everyone.
>
> >
> > > I'm fine to
> > > introduce a burst call for the same(I was thinking to propose it in future) to
> > > compensate for the overhead.
> > >
> > > If rte_security_set_pkt_metadata() was not a PMD specific function ptr call and
> > > rte_mbuf had space for struct rte_security_session pointer,
> >
> > But it does, see above.
> > In fact it even more flexible - because it is driver specific, you are not limited to one 64-bit field.
> > If your PMD requires more data to be associated with mbuf
> > - you can request it via mbuf_dynfield and store there whatever is needed.
> >
> > > then then I guess it would have been better to do the way you proposed.
> > >
> > > >
> > > > >
> > > > > >
> > > > > > > >
> > > > > > > > >
> > > > > > > > > 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
> > > > > > > > > >
>
> Adding more rte_security and PMD maintainers into the loop.
>
> > > > > > > > > > > > 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 <ndabilpuram@marvell.com>
> > > > > > > > > > > > Reviewed-by: Akhil Goyal <gakhil@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.
> > > > > > >
> > > > > > > Ok, so to make sure we are on the same page, you propose:
> > > > > > > 1. before calling rte_security_set_pkt_metadata() mbuf.l2_len should be properly set.
> > > > > > > 2. after rte_security_set_pkt_metadata() and before rte_eth_tx_burst() packet contents
> > > > > > > at [mbuf.l2_len, mbuf.pkt_len) can't be modified?
> > > > > > Yes.
> > > > > >
> > > > > > >
> > > > > > > Is that correct understanding?
> > > > > > > If yes, I wonder how 2) will correlate with rte_eth_tx_prepare() concept?
> > > > > >
> > > > > > Since our PMD doesn't have a prepare function, I missed that but, since
> > > > > > rte_security_set_pkt_metadata() is only used for Inline Crypto/Protocol via
> > > > > > a rte_eth_dev, and both rte_security_set_pkt_metadata() and rte_eth_tx_prepare()
> > > > > > are callbacks from same PMD, do you see any issue ?
> > > > > >
> > > > > > The restriction is from user side, data is not supposed to be modified unless
> > > > > > rte_security_set_pkt_metadata() is called again.
> > > > >
> > > > > Yep, I do have a concern here.
> > > > > Right now it is perfectly valid to do something like that:
> > > > > rte_security_set_pkt_metadata(..., mb, ...);
> > > > > /* can modify contents of the packet */
> > > > > rte_eth_tx_prepare(..., &mb, 1);
> > > > > rte_eth_tx_burst(..., &mb, 1);
> > > > >
> > > > > With the new restrictions you are proposing it wouldn't be allowed any more.
> > > > You can still modify L2 header and IPSEC is only concerned about L3 and above.
> > > >
> > > > I think insisting that rte_security_set_pkt_metadata() be called after all L3
> > > > and above header modifications is no a problem. I guess existing ixgbe/txgbe
> > > > PMD which are the ones only implementing the call back are already expecting the
> > > > same ?
> > >
> > > AFAIK, no there are no such requirements for ixgbe or txgbe.
> > > All that ixgbe callback does - store session related data inside mbuf.
> > > It's only expectation to have ESP trailer at the proper place (after ICV):
> >
> > This implies rte_security_set_pkt_metadata() cannot be called when mbuf does't
> > have ESP trailer updated or when mbuf->pkt_len = 0
> >
> > >
> > > union ixgbe_crypto_tx_desc_md *mdata = (union ixgbe_crypto_tx_desc_md *)
> > > rte_security_dynfield(m);
> > > mdata->enc = 1;
> > > mdata->sa_idx = ic_session->sa_index;
> > > mdata->pad_len = ixgbe_crypto_compute_pad_len(m);
> > >
> > > Then this data will be used by tx_burst() function.
> > So it implies that after above rte_security_set_pkt_metadata() call, and before tx_burst(),
> > mbuf data / packet len cannot be modified right as if modified, then tx_burst()
> > will be using incorrect pad len ?
>
> No, pkt_len can be modified.
> Though ESP trailer pad_len can't.
>
> >
> > This patch is also trying to add similar restriction on when
> > rte_security_set_pkt_metadata() should be called and what cannot be done after
> > calling rte_security_set_pkt_metadata().
>
> No, I don't think it is really the same.
> Also, IMO, inside ixgbe set_pkt_metadata() implementaion we probably shouldn't silently imply
> that ESP packet is already formed and trailer contains valid data.
> In fact, I think this pad_len calculation can be moved to actual TX function.
>
> >
> > >
> > > >
> > > > >
> > > > > >
> > > > > > If your question is can't we do the preprocessing in rte_eth_tx_prepare() for
> > > > > > security,
> > > > >
> > > > > Yes, that was my thought.
> > > > >
> > > > > > my only argument was that since there is already a hit in
> > > > > > rte_security_set_pkt_metadata() to PMD specific callback and
> > > > > > struct rte_security_session is passed as an argument to it, it is more benefitial to
> > > > > > do security related pre-processing there.
> > > > >
> > > > > Yes, it would be extra callback call that way.
> > > > > Though tx_prepare() accepts burst of packets, so the overhead
> > > > > of function call will be spread around the whole burst, and I presume
> > > > > shouldn't be too high.
> > > > >
> > > > > > Also rte_eth_tx_prepare() if implemented will be called for both security and
> > > > > > non-security pkts.
> > > > >
> > > > > Yes, but tx_prepare() can distinguish (by ol_flags and/or other field contents) which
> > > > > modifications are required for the packet.
> > > >
> > > > But the major issues I see are
> > > >
> > > > 1. tx_prepare() doesn't take rte_security_session as argument though ol_flags has security flag.
> > > > In our case, we need to know the security session details to do things.
> > >
> > > I suppose you can store pointer to session (or so) inside mbuf in rte_security_dynfield, no?
> >
> > We can do. But having to call PMD specific function call via rte_security_set_pkt_metadata()
> > just for storing session pointer in rte_security_dynfield consumes unnecessary
> > cycles per pkt.
>
> In fact there are two function calls: one for rte_security_set_pkt_metadata(),
> second for instance->ops->set_pkt_metadata() callback.
> Which off-course way too expensive for such simple operation.
> Actually same thought for rte_security_get_userdata().
> Both of these functions belong to data-path and ideally have to be as fast as possible.
> Probably 21.11 is a right timeframe for that.
>
> > >
> > > > 2. AFAIU tx_prepare() is not mandatory as per spec and even by default disabled under compile time
> > > > macro RTE_ETHDEV_TX_PREPARE_NOOP.
> > > > 3. Even if we do tx_prepare(), rte_security_set_pkt_mdata() is mandatory to associate
> > > > struct rte_security_session to a pkt as unlike ol_flags, there is no direct space to do the same.
> > >
> > > Didn't get you here, obviously we do have rte_security_dynfield inside mbuf,
> > > specially for that - to store secuiryt related data inside the mbuf.
> > > Yes your PMD has to request it at initialization time, but I suppose it is not a big deal.
> > >
> > > > So I think instead of enforcing yet another callback tx_prepare() for inline security
> > > > processing, it can be done via security specific set_pkt_metadata().
> > >
> > > But what you proposing introduces new limitations and might existing functionality.
> > > BTW, if you don't like to use tx_prepare() - why doing these calculations inside tx_burst()
> > > itself is not an option?
> >
> > We can do things in tx_burst() but if we are doing it there, then we want to avoid having callback for
> > rte_security_set_pkt_metadata().
> >
> > Are you fine if we can update the spec that "When DEV_TX_OFFLOAD_SEC_NEED_MDATA is not
> > set, then, user needs to update struct rte_security_session's sess_private_data in a in
> > rte_security_dynfield like below ?
> >
> > <snip>
> >
> > static inline void
> > inline_outb_mbuf_prepare(const struct rte_ipsec_session *ss,
> > struct rte_mbuf *mb[], uint16_t num)
> > {
> > uint32_t i, ol_flags;
> >
> > ol_flags = ss->security.ol_flags & RTE_SECURITY_TX_OLOAD_NEED_MDATA;
> > for (i = 0; i != num; i++) {
> >
> > mb[i]->ol_flags |= PKT_TX_SEC_OFFLOAD;
> >
> > if (ol_flags != 0)
> > rte_security_set_pkt_metadata(ss->security.ctx,
> > ss->security.ses, mb[i], NULL);
> > else
> > *rte_security_dynfield(mb[i]) =
> > (uint64_t)ss->security.ses->sess_private_data;
> >
> >
> > If the above can be done, then in our PMD, we will not have a callback for
> > set_pkt_metadata() and DEV_TX_OFFLOAD_SEC_NEED_MDATA will also be not set
> > in capabilities.
>
> That's an interesting idea, but what you propose is the change in current rte_security API behaviour.
> So all existing apps that use this API will have to be changed.
> We'd better avoid such changes unless there is really good reason for that.
> So, I'd suggest to tweak your idea a bit:
>
> 1) change rte_security_set_pkt_metadata():
> if ops->set_pkt_metadata != NULL, then call it (existing behaviour)
> otherwise just: rte_security_dynfield(m) = sess->session_private_data;
> (fast-path)
>
> 2) consider to make rte_security_set_pkt_metadata() inline function.
> We probably can have some special flag inside struct rte_security_ctx,
> or even store inside ctx a pointer to set_pkt_metadata() itself.
After another thoughts some new flags might be better.
Then later, if we'll realize that set_pkt_metadata() and get_useradata()
are not really used by PMDs, it might be easier to deprecate these callbacks.
>
> As a brief code snippet:
>
> struct rte_security_ctx {
> void *device;
> /**< Crypto/ethernet device attached */
> const struct rte_security_ops *ops;
> /**< Pointer to security ops for the device */
> uint16_t sess_cnt;
> /**< Number of sessions attached to this context */
> + int (*set_pkt_mdata)(void *, struct rte_security_session *, struct rte_mbuf *, void *);
> };
>
> static inline int
> rte_security_set_pkt_metadata(struct rte_security_ctx *instance,
> struct rte_security_session *sess,
> struct rte_mbuf *m, void *params)
> {
> /* fast-path */
> if (instance->set_pkt_mdata == NULL) {
> *rte_security_dynfield(m) = (rte_security_dynfield_t)(session->sess_priv_data);
> return 0;
> /* slow path */
> } else
> return instance->set_pkt_mdata(instance->device, sess, m, params);
> }
>
> That probably would be an ABI breakage (new fileld in rte_security_ctx) and would require
> some trivial changes for all existing PMDs that use RTE_SECURITY_TX_OFLOAD_NEED_MDATA
> (ctx_create()), but hopefully will benefit everyone.
>
> >
> > >
> > > > I'm fine to
> > > > introduce a burst call for the same(I was thinking to propose it in future) to
> > > > compensate for the overhead.
> > > >
> > > > If rte_security_set_pkt_metadata() was not a PMD specific function ptr call and
> > > > rte_mbuf had space for struct rte_security_session pointer,
> > >
> > > But it does, see above.
> > > In fact it even more flexible - because it is driver specific, you are not limited to one 64-bit field.
> > > If your PMD requires more data to be associated with mbuf
> > > - you can request it via mbuf_dynfield and store there whatever is needed.
> > >
> > > > then then I guess it would have been better to do the way you proposed.
> > > >
> > > > >
> > > > > >
> > > > > > >
> > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > 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
> > > > > > > > > > >
On Tue, Jul 13, 2021 at 02:08:18PM +0000, Ananyev, Konstantin wrote:
>
> >
> > Adding more rte_security and PMD maintainers into the loop.
> >
> > > > > > > > > > > > > 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 <ndabilpuram@marvell.com>
> > > > > > > > > > > > > Reviewed-by: Akhil Goyal <gakhil@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.
> > > > > > > >
> > > > > > > > Ok, so to make sure we are on the same page, you propose:
> > > > > > > > 1. before calling rte_security_set_pkt_metadata() mbuf.l2_len should be properly set.
> > > > > > > > 2. after rte_security_set_pkt_metadata() and before rte_eth_tx_burst() packet contents
> > > > > > > > at [mbuf.l2_len, mbuf.pkt_len) can't be modified?
> > > > > > > Yes.
> > > > > > >
> > > > > > > >
> > > > > > > > Is that correct understanding?
> > > > > > > > If yes, I wonder how 2) will correlate with rte_eth_tx_prepare() concept?
> > > > > > >
> > > > > > > Since our PMD doesn't have a prepare function, I missed that but, since
> > > > > > > rte_security_set_pkt_metadata() is only used for Inline Crypto/Protocol via
> > > > > > > a rte_eth_dev, and both rte_security_set_pkt_metadata() and rte_eth_tx_prepare()
> > > > > > > are callbacks from same PMD, do you see any issue ?
> > > > > > >
> > > > > > > The restriction is from user side, data is not supposed to be modified unless
> > > > > > > rte_security_set_pkt_metadata() is called again.
> > > > > >
> > > > > > Yep, I do have a concern here.
> > > > > > Right now it is perfectly valid to do something like that:
> > > > > > rte_security_set_pkt_metadata(..., mb, ...);
> > > > > > /* can modify contents of the packet */
> > > > > > rte_eth_tx_prepare(..., &mb, 1);
> > > > > > rte_eth_tx_burst(..., &mb, 1);
> > > > > >
> > > > > > With the new restrictions you are proposing it wouldn't be allowed any more.
> > > > > You can still modify L2 header and IPSEC is only concerned about L3 and above.
> > > > >
> > > > > I think insisting that rte_security_set_pkt_metadata() be called after all L3
> > > > > and above header modifications is no a problem. I guess existing ixgbe/txgbe
> > > > > PMD which are the ones only implementing the call back are already expecting the
> > > > > same ?
> > > >
> > > > AFAIK, no there are no such requirements for ixgbe or txgbe.
> > > > All that ixgbe callback does - store session related data inside mbuf.
> > > > It's only expectation to have ESP trailer at the proper place (after ICV):
> > >
> > > This implies rte_security_set_pkt_metadata() cannot be called when mbuf does't
> > > have ESP trailer updated or when mbuf->pkt_len = 0
> > >
> > > >
> > > > union ixgbe_crypto_tx_desc_md *mdata = (union ixgbe_crypto_tx_desc_md *)
> > > > rte_security_dynfield(m);
> > > > mdata->enc = 1;
> > > > mdata->sa_idx = ic_session->sa_index;
> > > > mdata->pad_len = ixgbe_crypto_compute_pad_len(m);
> > > >
> > > > Then this data will be used by tx_burst() function.
> > > So it implies that after above rte_security_set_pkt_metadata() call, and before tx_burst(),
> > > mbuf data / packet len cannot be modified right as if modified, then tx_burst()
> > > will be using incorrect pad len ?
> >
> > No, pkt_len can be modified.
> > Though ESP trailer pad_len can't.
> >
> > >
> > > This patch is also trying to add similar restriction on when
> > > rte_security_set_pkt_metadata() should be called and what cannot be done after
> > > calling rte_security_set_pkt_metadata().
> >
> > No, I don't think it is really the same.
> > Also, IMO, inside ixgbe set_pkt_metadata() implementaion we probably shouldn't silently imply
> > that ESP packet is already formed and trailer contains valid data.
> > In fact, I think this pad_len calculation can be moved to actual TX function.
> >
> > >
> > > >
> > > > >
> > > > > >
> > > > > > >
> > > > > > > If your question is can't we do the preprocessing in rte_eth_tx_prepare() for
> > > > > > > security,
> > > > > >
> > > > > > Yes, that was my thought.
> > > > > >
> > > > > > > my only argument was that since there is already a hit in
> > > > > > > rte_security_set_pkt_metadata() to PMD specific callback and
> > > > > > > struct rte_security_session is passed as an argument to it, it is more benefitial to
> > > > > > > do security related pre-processing there.
> > > > > >
> > > > > > Yes, it would be extra callback call that way.
> > > > > > Though tx_prepare() accepts burst of packets, so the overhead
> > > > > > of function call will be spread around the whole burst, and I presume
> > > > > > shouldn't be too high.
> > > > > >
> > > > > > > Also rte_eth_tx_prepare() if implemented will be called for both security and
> > > > > > > non-security pkts.
> > > > > >
> > > > > > Yes, but tx_prepare() can distinguish (by ol_flags and/or other field contents) which
> > > > > > modifications are required for the packet.
> > > > >
> > > > > But the major issues I see are
> > > > >
> > > > > 1. tx_prepare() doesn't take rte_security_session as argument though ol_flags has security flag.
> > > > > In our case, we need to know the security session details to do things.
> > > >
> > > > I suppose you can store pointer to session (or so) inside mbuf in rte_security_dynfield, no?
> > >
> > > We can do. But having to call PMD specific function call via rte_security_set_pkt_metadata()
> > > just for storing session pointer in rte_security_dynfield consumes unnecessary
> > > cycles per pkt.
> >
> > In fact there are two function calls: one for rte_security_set_pkt_metadata(),
> > second for instance->ops->set_pkt_metadata() callback.
> > Which off-course way too expensive for such simple operation.
> > Actually same thought for rte_security_get_userdata().
> > Both of these functions belong to data-path and ideally have to be as fast as possible.
> > Probably 21.11 is a right timeframe for that.
> >
> > > >
> > > > > 2. AFAIU tx_prepare() is not mandatory as per spec and even by default disabled under compile time
> > > > > macro RTE_ETHDEV_TX_PREPARE_NOOP.
> > > > > 3. Even if we do tx_prepare(), rte_security_set_pkt_mdata() is mandatory to associate
> > > > > struct rte_security_session to a pkt as unlike ol_flags, there is no direct space to do the same.
> > > >
> > > > Didn't get you here, obviously we do have rte_security_dynfield inside mbuf,
> > > > specially for that - to store secuiryt related data inside the mbuf.
> > > > Yes your PMD has to request it at initialization time, but I suppose it is not a big deal.
> > > >
> > > > > So I think instead of enforcing yet another callback tx_prepare() for inline security
> > > > > processing, it can be done via security specific set_pkt_metadata().
> > > >
> > > > But what you proposing introduces new limitations and might existing functionality.
> > > > BTW, if you don't like to use tx_prepare() - why doing these calculations inside tx_burst()
> > > > itself is not an option?
> > >
> > > We can do things in tx_burst() but if we are doing it there, then we want to avoid having callback for
> > > rte_security_set_pkt_metadata().
> > >
> > > Are you fine if we can update the spec that "When DEV_TX_OFFLOAD_SEC_NEED_MDATA is not
> > > set, then, user needs to update struct rte_security_session's sess_private_data in a in
> > > rte_security_dynfield like below ?
> > >
> > > <snip>
> > >
> > > static inline void
> > > inline_outb_mbuf_prepare(const struct rte_ipsec_session *ss,
> > > struct rte_mbuf *mb[], uint16_t num)
> > > {
> > > uint32_t i, ol_flags;
> > >
> > > ol_flags = ss->security.ol_flags & RTE_SECURITY_TX_OLOAD_NEED_MDATA;
> > > for (i = 0; i != num; i++) {
> > >
> > > mb[i]->ol_flags |= PKT_TX_SEC_OFFLOAD;
> > >
> > > if (ol_flags != 0)
> > > rte_security_set_pkt_metadata(ss->security.ctx,
> > > ss->security.ses, mb[i], NULL);
> > > else
> > > *rte_security_dynfield(mb[i]) =
> > > (uint64_t)ss->security.ses->sess_private_data;
> > >
> > >
> > > If the above can be done, then in our PMD, we will not have a callback for
> > > set_pkt_metadata() and DEV_TX_OFFLOAD_SEC_NEED_MDATA will also be not set
> > > in capabilities.
> >
> > That's an interesting idea, but what you propose is the change in current rte_security API behaviour.
> > So all existing apps that use this API will have to be changed.
> > We'd better avoid such changes unless there is really good reason for that.
> > So, I'd suggest to tweak your idea a bit:
> >
> > 1) change rte_security_set_pkt_metadata():
> > if ops->set_pkt_metadata != NULL, then call it (existing behaviour)
> > otherwise just: rte_security_dynfield(m) = sess->session_private_data;
> > (fast-path)
> >
> > 2) consider to make rte_security_set_pkt_metadata() inline function.
> > We probably can have some special flag inside struct rte_security_ctx,
> > or even store inside ctx a pointer to set_pkt_metadata() itself.
>
> After another thoughts some new flags might be better.
> Then later, if we'll realize that set_pkt_metadata() and get_useradata()
> are not really used by PMDs, it might be easier to deprecate these callbacks.
Thanks, I agree with your thoughts. I'll submit a V2 with above change, new flags and
set_pkt_metadata() and get_userdata() function pointers moved to rte_security_ctx for
review so that it can be targeted for 21.11.
Even with flags moving set_pkt_metadata() and get_userdata() function pointers is still needed
as we need to make rte_security_set_pkt_metadata() API inline while struct rte_security_ops is not
exposed to user. I think this is fine as it is inline with how fast path function pointers
of rte_ethdev and rte_cryptodev are currently placed.
>
> >
> > As a brief code snippet:
> >
> > struct rte_security_ctx {
> > void *device;
> > /**< Crypto/ethernet device attached */
> > const struct rte_security_ops *ops;
> > /**< Pointer to security ops for the device */
> > uint16_t sess_cnt;
> > /**< Number of sessions attached to this context */
> > + int (*set_pkt_mdata)(void *, struct rte_security_session *, struct rte_mbuf *, void *);
> > };
> >
> > static inline int
> > rte_security_set_pkt_metadata(struct rte_security_ctx *instance,
> > struct rte_security_session *sess,
> > struct rte_mbuf *m, void *params)
> > {
> > /* fast-path */
> > if (instance->set_pkt_mdata == NULL) {
> > *rte_security_dynfield(m) = (rte_security_dynfield_t)(session->sess_priv_data);
> > return 0;
> > /* slow path */
> > } else
> > return instance->set_pkt_mdata(instance->device, sess, m, params);
> > }
> >
> > That probably would be an ABI breakage (new fileld in rte_security_ctx) and would require
> > some trivial changes for all existing PMDs that use RTE_SECURITY_TX_OFLOAD_NEED_MDATA
> > (ctx_create()), but hopefully will benefit everyone.
> >
> > >
> > > >
> > > > > I'm fine to
> > > > > introduce a burst call for the same(I was thinking to propose it in future) to
> > > > > compensate for the overhead.
> > > > >
> > > > > If rte_security_set_pkt_metadata() was not a PMD specific function ptr call and
> > > > > rte_mbuf had space for struct rte_security_session pointer,
> > > >
> > > > But it does, see above.
> > > > In fact it even more flexible - because it is driver specific, you are not limited to one 64-bit field.
> > > > If your PMD requires more data to be associated with mbuf
> > > > - you can request it via mbuf_dynfield and store there whatever is needed.
> > > >
> > > > > then then I guess it would have been better to do the way you proposed.
> > > > >
> > > > > >
> > > > > > >
> > > > > > > >
> > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > 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
> > > > > > > > > > > >
> > >
> > > Adding more rte_security and PMD maintainers into the loop.
> > >
> > > > > > > > > > > > > > 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 <ndabilpuram@marvell.com>
> > > > > > > > > > > > > > Reviewed-by: Akhil Goyal <gakhil@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.
> > > > > > > > >
> > > > > > > > > Ok, so to make sure we are on the same page, you propose:
> > > > > > > > > 1. before calling rte_security_set_pkt_metadata() mbuf.l2_len should be properly set.
> > > > > > > > > 2. after rte_security_set_pkt_metadata() and before rte_eth_tx_burst() packet contents
> > > > > > > > > at [mbuf.l2_len, mbuf.pkt_len) can't be modified?
> > > > > > > > Yes.
> > > > > > > >
> > > > > > > > >
> > > > > > > > > Is that correct understanding?
> > > > > > > > > If yes, I wonder how 2) will correlate with rte_eth_tx_prepare() concept?
> > > > > > > >
> > > > > > > > Since our PMD doesn't have a prepare function, I missed that but, since
> > > > > > > > rte_security_set_pkt_metadata() is only used for Inline Crypto/Protocol via
> > > > > > > > a rte_eth_dev, and both rte_security_set_pkt_metadata() and rte_eth_tx_prepare()
> > > > > > > > are callbacks from same PMD, do you see any issue ?
> > > > > > > >
> > > > > > > > The restriction is from user side, data is not supposed to be modified unless
> > > > > > > > rte_security_set_pkt_metadata() is called again.
> > > > > > >
> > > > > > > Yep, I do have a concern here.
> > > > > > > Right now it is perfectly valid to do something like that:
> > > > > > > rte_security_set_pkt_metadata(..., mb, ...);
> > > > > > > /* can modify contents of the packet */
> > > > > > > rte_eth_tx_prepare(..., &mb, 1);
> > > > > > > rte_eth_tx_burst(..., &mb, 1);
> > > > > > >
> > > > > > > With the new restrictions you are proposing it wouldn't be allowed any more.
> > > > > > You can still modify L2 header and IPSEC is only concerned about L3 and above.
> > > > > >
> > > > > > I think insisting that rte_security_set_pkt_metadata() be called after all L3
> > > > > > and above header modifications is no a problem. I guess existing ixgbe/txgbe
> > > > > > PMD which are the ones only implementing the call back are already expecting the
> > > > > > same ?
> > > > >
> > > > > AFAIK, no there are no such requirements for ixgbe or txgbe.
> > > > > All that ixgbe callback does - store session related data inside mbuf.
> > > > > It's only expectation to have ESP trailer at the proper place (after ICV):
> > > >
> > > > This implies rte_security_set_pkt_metadata() cannot be called when mbuf does't
> > > > have ESP trailer updated or when mbuf->pkt_len = 0
> > > >
> > > > >
> > > > > union ixgbe_crypto_tx_desc_md *mdata = (union ixgbe_crypto_tx_desc_md *)
> > > > > rte_security_dynfield(m);
> > > > > mdata->enc = 1;
> > > > > mdata->sa_idx = ic_session->sa_index;
> > > > > mdata->pad_len = ixgbe_crypto_compute_pad_len(m);
> > > > >
> > > > > Then this data will be used by tx_burst() function.
> > > > So it implies that after above rte_security_set_pkt_metadata() call, and before tx_burst(),
> > > > mbuf data / packet len cannot be modified right as if modified, then tx_burst()
> > > > will be using incorrect pad len ?
> > >
> > > No, pkt_len can be modified.
> > > Though ESP trailer pad_len can't.
> > >
> > > >
> > > > This patch is also trying to add similar restriction on when
> > > > rte_security_set_pkt_metadata() should be called and what cannot be done after
> > > > calling rte_security_set_pkt_metadata().
> > >
> > > No, I don't think it is really the same.
> > > Also, IMO, inside ixgbe set_pkt_metadata() implementaion we probably shouldn't silently imply
> > > that ESP packet is already formed and trailer contains valid data.
> > > In fact, I think this pad_len calculation can be moved to actual TX function.
> > >
> > > >
> > > > >
> > > > > >
> > > > > > >
> > > > > > > >
> > > > > > > > If your question is can't we do the preprocessing in rte_eth_tx_prepare() for
> > > > > > > > security,
> > > > > > >
> > > > > > > Yes, that was my thought.
> > > > > > >
> > > > > > > > my only argument was that since there is already a hit in
> > > > > > > > rte_security_set_pkt_metadata() to PMD specific callback and
> > > > > > > > struct rte_security_session is passed as an argument to it, it is more benefitial to
> > > > > > > > do security related pre-processing there.
> > > > > > >
> > > > > > > Yes, it would be extra callback call that way.
> > > > > > > Though tx_prepare() accepts burst of packets, so the overhead
> > > > > > > of function call will be spread around the whole burst, and I presume
> > > > > > > shouldn't be too high.
> > > > > > >
> > > > > > > > Also rte_eth_tx_prepare() if implemented will be called for both security and
> > > > > > > > non-security pkts.
> > > > > > >
> > > > > > > Yes, but tx_prepare() can distinguish (by ol_flags and/or other field contents) which
> > > > > > > modifications are required for the packet.
> > > > > >
> > > > > > But the major issues I see are
> > > > > >
> > > > > > 1. tx_prepare() doesn't take rte_security_session as argument though ol_flags has security flag.
> > > > > > In our case, we need to know the security session details to do things.
> > > > >
> > > > > I suppose you can store pointer to session (or so) inside mbuf in rte_security_dynfield, no?
> > > >
> > > > We can do. But having to call PMD specific function call via rte_security_set_pkt_metadata()
> > > > just for storing session pointer in rte_security_dynfield consumes unnecessary
> > > > cycles per pkt.
> > >
> > > In fact there are two function calls: one for rte_security_set_pkt_metadata(),
> > > second for instance->ops->set_pkt_metadata() callback.
> > > Which off-course way too expensive for such simple operation.
> > > Actually same thought for rte_security_get_userdata().
> > > Both of these functions belong to data-path and ideally have to be as fast as possible.
> > > Probably 21.11 is a right timeframe for that.
> > >
> > > > >
> > > > > > 2. AFAIU tx_prepare() is not mandatory as per spec and even by default disabled under compile time
> > > > > > macro RTE_ETHDEV_TX_PREPARE_NOOP.
> > > > > > 3. Even if we do tx_prepare(), rte_security_set_pkt_mdata() is mandatory to associate
> > > > > > struct rte_security_session to a pkt as unlike ol_flags, there is no direct space to do the same.
> > > > >
> > > > > Didn't get you here, obviously we do have rte_security_dynfield inside mbuf,
> > > > > specially for that - to store secuiryt related data inside the mbuf.
> > > > > Yes your PMD has to request it at initialization time, but I suppose it is not a big deal.
> > > > >
> > > > > > So I think instead of enforcing yet another callback tx_prepare() for inline security
> > > > > > processing, it can be done via security specific set_pkt_metadata().
> > > > >
> > > > > But what you proposing introduces new limitations and might existing functionality.
> > > > > BTW, if you don't like to use tx_prepare() - why doing these calculations inside tx_burst()
> > > > > itself is not an option?
> > > >
> > > > We can do things in tx_burst() but if we are doing it there, then we want to avoid having callback for
> > > > rte_security_set_pkt_metadata().
> > > >
> > > > Are you fine if we can update the spec that "When DEV_TX_OFFLOAD_SEC_NEED_MDATA is not
> > > > set, then, user needs to update struct rte_security_session's sess_private_data in a in
> > > > rte_security_dynfield like below ?
> > > >
> > > > <snip>
> > > >
> > > > static inline void
> > > > inline_outb_mbuf_prepare(const struct rte_ipsec_session *ss,
> > > > struct rte_mbuf *mb[], uint16_t num)
> > > > {
> > > > uint32_t i, ol_flags;
> > > >
> > > > ol_flags = ss->security.ol_flags & RTE_SECURITY_TX_OLOAD_NEED_MDATA;
> > > > for (i = 0; i != num; i++) {
> > > >
> > > > mb[i]->ol_flags |= PKT_TX_SEC_OFFLOAD;
> > > >
> > > > if (ol_flags != 0)
> > > > rte_security_set_pkt_metadata(ss->security.ctx,
> > > > ss->security.ses, mb[i], NULL);
> > > > else
> > > > *rte_security_dynfield(mb[i]) =
> > > > (uint64_t)ss->security.ses->sess_private_data;
> > > >
> > > >
> > > > If the above can be done, then in our PMD, we will not have a callback for
> > > > set_pkt_metadata() and DEV_TX_OFFLOAD_SEC_NEED_MDATA will also be not set
> > > > in capabilities.
> > >
> > > That's an interesting idea, but what you propose is the change in current rte_security API behaviour.
> > > So all existing apps that use this API will have to be changed.
> > > We'd better avoid such changes unless there is really good reason for that.
> > > So, I'd suggest to tweak your idea a bit:
> > >
> > > 1) change rte_security_set_pkt_metadata():
> > > if ops->set_pkt_metadata != NULL, then call it (existing behaviour)
> > > otherwise just: rte_security_dynfield(m) = sess->session_private_data;
> > > (fast-path)
> > >
> > > 2) consider to make rte_security_set_pkt_metadata() inline function.
> > > We probably can have some special flag inside struct rte_security_ctx,
> > > or even store inside ctx a pointer to set_pkt_metadata() itself.
> >
> > After another thoughts some new flags might be better.
> > Then later, if we'll realize that set_pkt_metadata() and get_useradata()
> > are not really used by PMDs, it might be easier to deprecate these callbacks.
>
> Thanks, I agree with your thoughts. I'll submit a V2 with above change, new flags and
> set_pkt_metadata() and get_userdata() function pointers moved to rte_security_ctx for
> review so that it can be targeted for 21.11.
>
> Even with flags moving set_pkt_metadata() and get_userdata() function pointers is still needed
> as we need to make rte_security_set_pkt_metadata() API inline while struct rte_security_ops is not
> exposed to user. I think this is fine as it is inline with how fast path function pointers
> of rte_ethdev and rte_cryptodev are currently placed.
My thought was we can get away with just flags only.
Something like that:
rte_security.h:
...
enum {
RTE_SEC_CTX_F_FAST_SET_MDATA = 0x1,
RTE_SEC_CTX_F_FAST_GET_UDATA = 0x2,
};
struct rte_security_ctx {
void *device;
/**< Crypto/ethernet device attached */
const struct rte_security_ops *ops;
/**< Pointer to security ops for the device */
uint16_t sess_cnt;
/**< Number of sessions attached to this context */
uint32_t flags;
};
extern int
__rte_security_set_pkt_metadata(struct rte_security_ctx *instance,
struct rte_security_session *sess,
struct rte_mbuf *m, void *params);
static inline int
rte_security_set_pkt_metadata(struct rte_security_ctx *instance,
struct rte_security_session *sess,
struct rte_mbuf *m, void *params)
{
/* fast-path */
if (instance->flags & RTE_SEC_CTX_F_FAST_SET_MDATA) {
*rte_security_dynfield(m) = (rte_security_dynfield_t)(session->sess_priv_data);
return 0;
/* slow path */
} else
return __rte_security_set_pkt_metadata (instance->device, sess, m, params);
}
rte_security.c:
...
/* existing one, just renamed */
int
__rte_security_set_pkt_metadata(struct rte_security_ctx *instance,
struct rte_security_session *sess,
struct rte_mbuf *m, void *params)
{
#ifdef RTE_DEBUG
RTE_PTR_OR_ERR_RET(sess, -EINVAL);
RTE_PTR_OR_ERR_RET(instance, -EINVAL);
RTE_PTR_OR_ERR_RET(instance->ops, -EINVAL);
#endif
RTE_FUNC_PTR_OR_ERR_RET(*instance->ops->set_pkt_metadata, -ENOTSUP);
return instance->ops->set_pkt_metadata(instance->device,
sess, m, params);
}
I think both ways are possible (flags vs actual func pointers) and both have
some pluses and minuses.
I suppose the main choice here what do we think should be the future of
set_pkt_metadata() and rte_security_get_userdata().
If we think that they will be useful for some future PMDs and we want to keep them,
then probably storing actual func pointers inside ctx is a better approach.
If not, then flags seems like a better one, as in that case we can eventually
deprecate and remove these callbacks.
From what I see right now, custom callbacks seems excessive,
and rte_security_dynfield is enough.
But might be there are some future plans that would require them?
>
> >
> > >
> > > As a brief code snippet:
> > >
> > > struct rte_security_ctx {
> > > void *device;
> > > /**< Crypto/ethernet device attached */
> > > const struct rte_security_ops *ops;
> > > /**< Pointer to security ops for the device */
> > > uint16_t sess_cnt;
> > > /**< Number of sessions attached to this context */
> > > + int (*set_pkt_mdata)(void *, struct rte_security_session *, struct rte_mbuf *, void *);
> > > };
> > >
> > > static inline int
> > > rte_security_set_pkt_metadata(struct rte_security_ctx *instance,
> > > struct rte_security_session *sess,
> > > struct rte_mbuf *m, void *params)
> > > {
> > > /* fast-path */
> > > if (instance->set_pkt_mdata == NULL) {
> > > *rte_security_dynfield(m) = (rte_security_dynfield_t)(session->sess_priv_data);
> > > return 0;
> > > /* slow path */
> > > } else
> > > return instance->set_pkt_mdata(instance->device, sess, m, params);
> > > }
> > >
> > > That probably would be an ABI breakage (new fileld in rte_security_ctx) and would require
> > > some trivial changes for all existing PMDs that use RTE_SECURITY_TX_OFLOAD_NEED_MDATA
> > > (ctx_create()), but hopefully will benefit everyone.
> > >
> > > >
> > > > >
> > > > > > I'm fine to
> > > > > > introduce a burst call for the same(I was thinking to propose it in future) to
> > > > > > compensate for the overhead.
> > > > > >
> > > > > > If rte_security_set_pkt_metadata() was not a PMD specific function ptr call and
> > > > > > rte_mbuf had space for struct rte_security_session pointer,
> > > > >
> > > > > But it does, see above.
> > > > > In fact it even more flexible - because it is driver specific, you are not limited to one 64-bit field.
> > > > > If your PMD requires more data to be associated with mbuf
> > > > > - you can request it via mbuf_dynfield and store there whatever is needed.
> > > > >
> > > > > > then then I guess it would have been better to do the way you proposed.
> > > > > >
> > > > > > >
> > > > > > > >
> > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > > > 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
> > > > > > > > > > > > >
On Wed, Jul 14, 2021 at 11:09:08AM +0000, Ananyev, Konstantin wrote:
> > > >
> > > > Adding more rte_security and PMD maintainers into the loop.
> > > >
> > > > > > > > > > > > > > > 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 <ndabilpuram@marvell.com>
> > > > > > > > > > > > > > > Reviewed-by: Akhil Goyal <gakhil@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.
> > > > > > > > > >
> > > > > > > > > > Ok, so to make sure we are on the same page, you propose:
> > > > > > > > > > 1. before calling rte_security_set_pkt_metadata() mbuf.l2_len should be properly set.
> > > > > > > > > > 2. after rte_security_set_pkt_metadata() and before rte_eth_tx_burst() packet contents
> > > > > > > > > > at [mbuf.l2_len, mbuf.pkt_len) can't be modified?
> > > > > > > > > Yes.
> > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > Is that correct understanding?
> > > > > > > > > > If yes, I wonder how 2) will correlate with rte_eth_tx_prepare() concept?
> > > > > > > > >
> > > > > > > > > Since our PMD doesn't have a prepare function, I missed that but, since
> > > > > > > > > rte_security_set_pkt_metadata() is only used for Inline Crypto/Protocol via
> > > > > > > > > a rte_eth_dev, and both rte_security_set_pkt_metadata() and rte_eth_tx_prepare()
> > > > > > > > > are callbacks from same PMD, do you see any issue ?
> > > > > > > > >
> > > > > > > > > The restriction is from user side, data is not supposed to be modified unless
> > > > > > > > > rte_security_set_pkt_metadata() is called again.
> > > > > > > >
> > > > > > > > Yep, I do have a concern here.
> > > > > > > > Right now it is perfectly valid to do something like that:
> > > > > > > > rte_security_set_pkt_metadata(..., mb, ...);
> > > > > > > > /* can modify contents of the packet */
> > > > > > > > rte_eth_tx_prepare(..., &mb, 1);
> > > > > > > > rte_eth_tx_burst(..., &mb, 1);
> > > > > > > >
> > > > > > > > With the new restrictions you are proposing it wouldn't be allowed any more.
> > > > > > > You can still modify L2 header and IPSEC is only concerned about L3 and above.
> > > > > > >
> > > > > > > I think insisting that rte_security_set_pkt_metadata() be called after all L3
> > > > > > > and above header modifications is no a problem. I guess existing ixgbe/txgbe
> > > > > > > PMD which are the ones only implementing the call back are already expecting the
> > > > > > > same ?
> > > > > >
> > > > > > AFAIK, no there are no such requirements for ixgbe or txgbe.
> > > > > > All that ixgbe callback does - store session related data inside mbuf.
> > > > > > It's only expectation to have ESP trailer at the proper place (after ICV):
> > > > >
> > > > > This implies rte_security_set_pkt_metadata() cannot be called when mbuf does't
> > > > > have ESP trailer updated or when mbuf->pkt_len = 0
> > > > >
> > > > > >
> > > > > > union ixgbe_crypto_tx_desc_md *mdata = (union ixgbe_crypto_tx_desc_md *)
> > > > > > rte_security_dynfield(m);
> > > > > > mdata->enc = 1;
> > > > > > mdata->sa_idx = ic_session->sa_index;
> > > > > > mdata->pad_len = ixgbe_crypto_compute_pad_len(m);
> > > > > >
> > > > > > Then this data will be used by tx_burst() function.
> > > > > So it implies that after above rte_security_set_pkt_metadata() call, and before tx_burst(),
> > > > > mbuf data / packet len cannot be modified right as if modified, then tx_burst()
> > > > > will be using incorrect pad len ?
> > > >
> > > > No, pkt_len can be modified.
> > > > Though ESP trailer pad_len can't.
> > > >
> > > > >
> > > > > This patch is also trying to add similar restriction on when
> > > > > rte_security_set_pkt_metadata() should be called and what cannot be done after
> > > > > calling rte_security_set_pkt_metadata().
> > > >
> > > > No, I don't think it is really the same.
> > > > Also, IMO, inside ixgbe set_pkt_metadata() implementaion we probably shouldn't silently imply
> > > > that ESP packet is already formed and trailer contains valid data.
> > > > In fact, I think this pad_len calculation can be moved to actual TX function.
> > > >
> > > > >
> > > > > >
> > > > > > >
> > > > > > > >
> > > > > > > > >
> > > > > > > > > If your question is can't we do the preprocessing in rte_eth_tx_prepare() for
> > > > > > > > > security,
> > > > > > > >
> > > > > > > > Yes, that was my thought.
> > > > > > > >
> > > > > > > > > my only argument was that since there is already a hit in
> > > > > > > > > rte_security_set_pkt_metadata() to PMD specific callback and
> > > > > > > > > struct rte_security_session is passed as an argument to it, it is more benefitial to
> > > > > > > > > do security related pre-processing there.
> > > > > > > >
> > > > > > > > Yes, it would be extra callback call that way.
> > > > > > > > Though tx_prepare() accepts burst of packets, so the overhead
> > > > > > > > of function call will be spread around the whole burst, and I presume
> > > > > > > > shouldn't be too high.
> > > > > > > >
> > > > > > > > > Also rte_eth_tx_prepare() if implemented will be called for both security and
> > > > > > > > > non-security pkts.
> > > > > > > >
> > > > > > > > Yes, but tx_prepare() can distinguish (by ol_flags and/or other field contents) which
> > > > > > > > modifications are required for the packet.
> > > > > > >
> > > > > > > But the major issues I see are
> > > > > > >
> > > > > > > 1. tx_prepare() doesn't take rte_security_session as argument though ol_flags has security flag.
> > > > > > > In our case, we need to know the security session details to do things.
> > > > > >
> > > > > > I suppose you can store pointer to session (or so) inside mbuf in rte_security_dynfield, no?
> > > > >
> > > > > We can do. But having to call PMD specific function call via rte_security_set_pkt_metadata()
> > > > > just for storing session pointer in rte_security_dynfield consumes unnecessary
> > > > > cycles per pkt.
> > > >
> > > > In fact there are two function calls: one for rte_security_set_pkt_metadata(),
> > > > second for instance->ops->set_pkt_metadata() callback.
> > > > Which off-course way too expensive for such simple operation.
> > > > Actually same thought for rte_security_get_userdata().
> > > > Both of these functions belong to data-path and ideally have to be as fast as possible.
> > > > Probably 21.11 is a right timeframe for that.
> > > >
> > > > > >
> > > > > > > 2. AFAIU tx_prepare() is not mandatory as per spec and even by default disabled under compile time
> > > > > > > macro RTE_ETHDEV_TX_PREPARE_NOOP.
> > > > > > > 3. Even if we do tx_prepare(), rte_security_set_pkt_mdata() is mandatory to associate
> > > > > > > struct rte_security_session to a pkt as unlike ol_flags, there is no direct space to do the same.
> > > > > >
> > > > > > Didn't get you here, obviously we do have rte_security_dynfield inside mbuf,
> > > > > > specially for that - to store secuiryt related data inside the mbuf.
> > > > > > Yes your PMD has to request it at initialization time, but I suppose it is not a big deal.
> > > > > >
> > > > > > > So I think instead of enforcing yet another callback tx_prepare() for inline security
> > > > > > > processing, it can be done via security specific set_pkt_metadata().
> > > > > >
> > > > > > But what you proposing introduces new limitations and might existing functionality.
> > > > > > BTW, if you don't like to use tx_prepare() - why doing these calculations inside tx_burst()
> > > > > > itself is not an option?
> > > > >
> > > > > We can do things in tx_burst() but if we are doing it there, then we want to avoid having callback for
> > > > > rte_security_set_pkt_metadata().
> > > > >
> > > > > Are you fine if we can update the spec that "When DEV_TX_OFFLOAD_SEC_NEED_MDATA is not
> > > > > set, then, user needs to update struct rte_security_session's sess_private_data in a in
> > > > > rte_security_dynfield like below ?
> > > > >
> > > > > <snip>
> > > > >
> > > > > static inline void
> > > > > inline_outb_mbuf_prepare(const struct rte_ipsec_session *ss,
> > > > > struct rte_mbuf *mb[], uint16_t num)
> > > > > {
> > > > > uint32_t i, ol_flags;
> > > > >
> > > > > ol_flags = ss->security.ol_flags & RTE_SECURITY_TX_OLOAD_NEED_MDATA;
> > > > > for (i = 0; i != num; i++) {
> > > > >
> > > > > mb[i]->ol_flags |= PKT_TX_SEC_OFFLOAD;
> > > > >
> > > > > if (ol_flags != 0)
> > > > > rte_security_set_pkt_metadata(ss->security.ctx,
> > > > > ss->security.ses, mb[i], NULL);
> > > > > else
> > > > > *rte_security_dynfield(mb[i]) =
> > > > > (uint64_t)ss->security.ses->sess_private_data;
> > > > >
> > > > >
> > > > > If the above can be done, then in our PMD, we will not have a callback for
> > > > > set_pkt_metadata() and DEV_TX_OFFLOAD_SEC_NEED_MDATA will also be not set
> > > > > in capabilities.
> > > >
> > > > That's an interesting idea, but what you propose is the change in current rte_security API behaviour.
> > > > So all existing apps that use this API will have to be changed.
> > > > We'd better avoid such changes unless there is really good reason for that.
> > > > So, I'd suggest to tweak your idea a bit:
> > > >
> > > > 1) change rte_security_set_pkt_metadata():
> > > > if ops->set_pkt_metadata != NULL, then call it (existing behaviour)
> > > > otherwise just: rte_security_dynfield(m) = sess->session_private_data;
> > > > (fast-path)
> > > >
> > > > 2) consider to make rte_security_set_pkt_metadata() inline function.
> > > > We probably can have some special flag inside struct rte_security_ctx,
> > > > or even store inside ctx a pointer to set_pkt_metadata() itself.
> > >
> > > After another thoughts some new flags might be better.
> > > Then later, if we'll realize that set_pkt_metadata() and get_useradata()
> > > are not really used by PMDs, it might be easier to deprecate these callbacks.
> >
> > Thanks, I agree with your thoughts. I'll submit a V2 with above change, new flags and
> > set_pkt_metadata() and get_userdata() function pointers moved to rte_security_ctx for
> > review so that it can be targeted for 21.11.
> >
> > Even with flags moving set_pkt_metadata() and get_userdata() function pointers is still needed
> > as we need to make rte_security_set_pkt_metadata() API inline while struct rte_security_ops is not
> > exposed to user. I think this is fine as it is inline with how fast path function pointers
> > of rte_ethdev and rte_cryptodev are currently placed.
>
> My thought was we can get away with just flags only.
> Something like that:
> rte_security.h:
>
> ...
>
> enum {
> RTE_SEC_CTX_F_FAST_SET_MDATA = 0x1,
> RTE_SEC_CTX_F_FAST_GET_UDATA = 0x2,
> };
>
> struct rte_security_ctx {
> void *device;
> /**< Crypto/ethernet device attached */
> const struct rte_security_ops *ops;
> /**< Pointer to security ops for the device */
> uint16_t sess_cnt;
> /**< Number of sessions attached to this context */
> uint32_t flags;
> };
>
> extern int
> __rte_security_set_pkt_metadata(struct rte_security_ctx *instance,
> struct rte_security_session *sess,
> struct rte_mbuf *m, void *params);
>
> static inline int
> rte_security_set_pkt_metadata(struct rte_security_ctx *instance,
> struct rte_security_session *sess,
> struct rte_mbuf *m, void *params)
> {
> /* fast-path */
> if (instance->flags & RTE_SEC_CTX_F_FAST_SET_MDATA) {
> *rte_security_dynfield(m) = (rte_security_dynfield_t)(session->sess_priv_data);
> return 0;
> /* slow path */
> } else
> return __rte_security_set_pkt_metadata (instance->device, sess, m, params);
> }
>
> rte_security.c:
>
> ...
> /* existing one, just renamed */
> int
> __rte_security_set_pkt_metadata(struct rte_security_ctx *instance,
> struct rte_security_session *sess,
> struct rte_mbuf *m, void *params)
> {
> #ifdef RTE_DEBUG
> RTE_PTR_OR_ERR_RET(sess, -EINVAL);
> RTE_PTR_OR_ERR_RET(instance, -EINVAL);
> RTE_PTR_OR_ERR_RET(instance->ops, -EINVAL);
> #endif
> RTE_FUNC_PTR_OR_ERR_RET(*instance->ops->set_pkt_metadata, -ENOTSUP);
> return instance->ops->set_pkt_metadata(instance->device,
> sess, m, params);
> }
>
>
> I think both ways are possible (flags vs actual func pointers) and both have
> some pluses and minuses.
> I suppose the main choice here what do we think should be the future of
> set_pkt_metadata() and rte_security_get_userdata().
> If we think that they will be useful for some future PMDs and we want to keep them,
> then probably storing actual func pointers inside ctx is a better approach.
> If not, then flags seems like a better one, as in that case we can eventually
> deprecate and remove these callbacks.
> From what I see right now, custom callbacks seems excessive,
> and rte_security_dynfield is enough.
> But might be there are some future plans that would require them?
Above method is also fine. Moving fn pointers to rte_security_ctx can be
done later if other PMD's need it.
Atleast our HW PMD's doesn't plan to use set_pkt_metada()/get_user_data()
fn pointers in future if above is implemented.
>
> >
> > >
> > > >
> > > > As a brief code snippet:
> > > >
> > > > struct rte_security_ctx {
> > > > void *device;
> > > > /**< Crypto/ethernet device attached */
> > > > const struct rte_security_ops *ops;
> > > > /**< Pointer to security ops for the device */
> > > > uint16_t sess_cnt;
> > > > /**< Number of sessions attached to this context */
> > > > + int (*set_pkt_mdata)(void *, struct rte_security_session *, struct rte_mbuf *, void *);
> > > > };
> > > >
> > > > static inline int
> > > > rte_security_set_pkt_metadata(struct rte_security_ctx *instance,
> > > > struct rte_security_session *sess,
> > > > struct rte_mbuf *m, void *params)
> > > > {
> > > > /* fast-path */
> > > > if (instance->set_pkt_mdata == NULL) {
> > > > *rte_security_dynfield(m) = (rte_security_dynfield_t)(session->sess_priv_data);
> > > > return 0;
> > > > /* slow path */
> > > > } else
> > > > return instance->set_pkt_mdata(instance->device, sess, m, params);
> > > > }
> > > >
> > > > That probably would be an ABI breakage (new fileld in rte_security_ctx) and would require
> > > > some trivial changes for all existing PMDs that use RTE_SECURITY_TX_OFLOAD_NEED_MDATA
> > > > (ctx_create()), but hopefully will benefit everyone.
> > > >
> > > > >
> > > > > >
> > > > > > > I'm fine to
> > > > > > > introduce a burst call for the same(I was thinking to propose it in future) to
> > > > > > > compensate for the overhead.
> > > > > > >
> > > > > > > If rte_security_set_pkt_metadata() was not a PMD specific function ptr call and
> > > > > > > rte_mbuf had space for struct rte_security_session pointer,
> > > > > >
> > > > > > But it does, see above.
> > > > > > In fact it even more flexible - because it is driver specific, you are not limited to one 64-bit field.
> > > > > > If your PMD requires more data to be associated with mbuf
> > > > > > - you can request it via mbuf_dynfield and store there whatever is needed.
> > > > > >
> > > > > > > then then I guess it would have been better to do the way you proposed.
> > > > > > >
> > > > > > > >
> > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > > > >
> > > > > > > > > > > > > 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
> > > > > > > > > > > > > >
> -----Original Message-----
> From: Nithin Dabilpuram <nithind1988@gmail.com>
> Sent: Wednesday, July 14, 2021 2:30 PM
> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>
> Cc: Akhil Goyal <gakhil@marvell.com>; dev@dpdk.org; hemant.agrawal@nxp.com; thomas@monjalon.net; g.singh@nxp.com; Yigit, Ferruh
> <ferruh.yigit@intel.com>; Zhang, Roy Fan <roy.fan.zhang@intel.com>; olivier.matz@6wind.com; jerinj@marvell.com; Doherty, Declan
> <declan.doherty@intel.com>; Nicolau, Radu <radu.nicolau@intel.com>; jiawenwu@trustnetic.com; jianwang@trustnetic.com
> Subject: Re: [dpdk-dev] [PATCH 1/2] security: enforce semantics for Tx inline processing
>
> On Wed, Jul 14, 2021 at 11:09:08AM +0000, Ananyev, Konstantin wrote:
> > > > >
> > > > > Adding more rte_security and PMD maintainers into the loop.
> > > > >
> > > > > > > > > > > > > > > > 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 <ndabilpuram@marvell.com>
> > > > > > > > > > > > > > > > Reviewed-by: Akhil Goyal <gakhil@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.
> > > > > > > > > > >
> > > > > > > > > > > Ok, so to make sure we are on the same page, you propose:
> > > > > > > > > > > 1. before calling rte_security_set_pkt_metadata() mbuf.l2_len should be properly set.
> > > > > > > > > > > 2. after rte_security_set_pkt_metadata() and before rte_eth_tx_burst() packet contents
> > > > > > > > > > > at [mbuf.l2_len, mbuf.pkt_len) can't be modified?
> > > > > > > > > > Yes.
> > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > Is that correct understanding?
> > > > > > > > > > > If yes, I wonder how 2) will correlate with rte_eth_tx_prepare() concept?
> > > > > > > > > >
> > > > > > > > > > Since our PMD doesn't have a prepare function, I missed that but, since
> > > > > > > > > > rte_security_set_pkt_metadata() is only used for Inline Crypto/Protocol via
> > > > > > > > > > a rte_eth_dev, and both rte_security_set_pkt_metadata() and rte_eth_tx_prepare()
> > > > > > > > > > are callbacks from same PMD, do you see any issue ?
> > > > > > > > > >
> > > > > > > > > > The restriction is from user side, data is not supposed to be modified unless
> > > > > > > > > > rte_security_set_pkt_metadata() is called again.
> > > > > > > > >
> > > > > > > > > Yep, I do have a concern here.
> > > > > > > > > Right now it is perfectly valid to do something like that:
> > > > > > > > > rte_security_set_pkt_metadata(..., mb, ...);
> > > > > > > > > /* can modify contents of the packet */
> > > > > > > > > rte_eth_tx_prepare(..., &mb, 1);
> > > > > > > > > rte_eth_tx_burst(..., &mb, 1);
> > > > > > > > >
> > > > > > > > > With the new restrictions you are proposing it wouldn't be allowed any more.
> > > > > > > > You can still modify L2 header and IPSEC is only concerned about L3 and above.
> > > > > > > >
> > > > > > > > I think insisting that rte_security_set_pkt_metadata() be called after all L3
> > > > > > > > and above header modifications is no a problem. I guess existing ixgbe/txgbe
> > > > > > > > PMD which are the ones only implementing the call back are already expecting the
> > > > > > > > same ?
> > > > > > >
> > > > > > > AFAIK, no there are no such requirements for ixgbe or txgbe.
> > > > > > > All that ixgbe callback does - store session related data inside mbuf.
> > > > > > > It's only expectation to have ESP trailer at the proper place (after ICV):
> > > > > >
> > > > > > This implies rte_security_set_pkt_metadata() cannot be called when mbuf does't
> > > > > > have ESP trailer updated or when mbuf->pkt_len = 0
> > > > > >
> > > > > > >
> > > > > > > union ixgbe_crypto_tx_desc_md *mdata = (union ixgbe_crypto_tx_desc_md *)
> > > > > > > rte_security_dynfield(m);
> > > > > > > mdata->enc = 1;
> > > > > > > mdata->sa_idx = ic_session->sa_index;
> > > > > > > mdata->pad_len = ixgbe_crypto_compute_pad_len(m);
> > > > > > >
> > > > > > > Then this data will be used by tx_burst() function.
> > > > > > So it implies that after above rte_security_set_pkt_metadata() call, and before tx_burst(),
> > > > > > mbuf data / packet len cannot be modified right as if modified, then tx_burst()
> > > > > > will be using incorrect pad len ?
> > > > >
> > > > > No, pkt_len can be modified.
> > > > > Though ESP trailer pad_len can't.
> > > > >
> > > > > >
> > > > > > This patch is also trying to add similar restriction on when
> > > > > > rte_security_set_pkt_metadata() should be called and what cannot be done after
> > > > > > calling rte_security_set_pkt_metadata().
> > > > >
> > > > > No, I don't think it is really the same.
> > > > > Also, IMO, inside ixgbe set_pkt_metadata() implementaion we probably shouldn't silently imply
> > > > > that ESP packet is already formed and trailer contains valid data.
> > > > > In fact, I think this pad_len calculation can be moved to actual TX function.
> > > > >
> > > > > >
> > > > > > >
> > > > > > > >
> > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > If your question is can't we do the preprocessing in rte_eth_tx_prepare() for
> > > > > > > > > > security,
> > > > > > > > >
> > > > > > > > > Yes, that was my thought.
> > > > > > > > >
> > > > > > > > > > my only argument was that since there is already a hit in
> > > > > > > > > > rte_security_set_pkt_metadata() to PMD specific callback and
> > > > > > > > > > struct rte_security_session is passed as an argument to it, it is more benefitial to
> > > > > > > > > > do security related pre-processing there.
> > > > > > > > >
> > > > > > > > > Yes, it would be extra callback call that way.
> > > > > > > > > Though tx_prepare() accepts burst of packets, so the overhead
> > > > > > > > > of function call will be spread around the whole burst, and I presume
> > > > > > > > > shouldn't be too high.
> > > > > > > > >
> > > > > > > > > > Also rte_eth_tx_prepare() if implemented will be called for both security and
> > > > > > > > > > non-security pkts.
> > > > > > > > >
> > > > > > > > > Yes, but tx_prepare() can distinguish (by ol_flags and/or other field contents) which
> > > > > > > > > modifications are required for the packet.
> > > > > > > >
> > > > > > > > But the major issues I see are
> > > > > > > >
> > > > > > > > 1. tx_prepare() doesn't take rte_security_session as argument though ol_flags has security flag.
> > > > > > > > In our case, we need to know the security session details to do things.
> > > > > > >
> > > > > > > I suppose you can store pointer to session (or so) inside mbuf in rte_security_dynfield, no?
> > > > > >
> > > > > > We can do. But having to call PMD specific function call via rte_security_set_pkt_metadata()
> > > > > > just for storing session pointer in rte_security_dynfield consumes unnecessary
> > > > > > cycles per pkt.
> > > > >
> > > > > In fact there are two function calls: one for rte_security_set_pkt_metadata(),
> > > > > second for instance->ops->set_pkt_metadata() callback.
> > > > > Which off-course way too expensive for such simple operation.
> > > > > Actually same thought for rte_security_get_userdata().
> > > > > Both of these functions belong to data-path and ideally have to be as fast as possible.
> > > > > Probably 21.11 is a right timeframe for that.
> > > > >
> > > > > > >
> > > > > > > > 2. AFAIU tx_prepare() is not mandatory as per spec and even by default disabled under compile time
> > > > > > > > macro RTE_ETHDEV_TX_PREPARE_NOOP.
> > > > > > > > 3. Even if we do tx_prepare(), rte_security_set_pkt_mdata() is mandatory to associate
> > > > > > > > struct rte_security_session to a pkt as unlike ol_flags, there is no direct space to do the same.
> > > > > > >
> > > > > > > Didn't get you here, obviously we do have rte_security_dynfield inside mbuf,
> > > > > > > specially for that - to store secuiryt related data inside the mbuf.
> > > > > > > Yes your PMD has to request it at initialization time, but I suppose it is not a big deal.
> > > > > > >
> > > > > > > > So I think instead of enforcing yet another callback tx_prepare() for inline security
> > > > > > > > processing, it can be done via security specific set_pkt_metadata().
> > > > > > >
> > > > > > > But what you proposing introduces new limitations and might existing functionality.
> > > > > > > BTW, if you don't like to use tx_prepare() - why doing these calculations inside tx_burst()
> > > > > > > itself is not an option?
> > > > > >
> > > > > > We can do things in tx_burst() but if we are doing it there, then we want to avoid having callback for
> > > > > > rte_security_set_pkt_metadata().
> > > > > >
> > > > > > Are you fine if we can update the spec that "When DEV_TX_OFFLOAD_SEC_NEED_MDATA is not
> > > > > > set, then, user needs to update struct rte_security_session's sess_private_data in a in
> > > > > > rte_security_dynfield like below ?
> > > > > >
> > > > > > <snip>
> > > > > >
> > > > > > static inline void
> > > > > > inline_outb_mbuf_prepare(const struct rte_ipsec_session *ss,
> > > > > > struct rte_mbuf *mb[], uint16_t num)
> > > > > > {
> > > > > > uint32_t i, ol_flags;
> > > > > >
> > > > > > ol_flags = ss->security.ol_flags & RTE_SECURITY_TX_OLOAD_NEED_MDATA;
> > > > > > for (i = 0; i != num; i++) {
> > > > > >
> > > > > > mb[i]->ol_flags |= PKT_TX_SEC_OFFLOAD;
> > > > > >
> > > > > > if (ol_flags != 0)
> > > > > > rte_security_set_pkt_metadata(ss->security.ctx,
> > > > > > ss->security.ses, mb[i], NULL);
> > > > > > else
> > > > > > *rte_security_dynfield(mb[i]) =
> > > > > > (uint64_t)ss->security.ses->sess_private_data;
> > > > > >
> > > > > >
> > > > > > If the above can be done, then in our PMD, we will not have a callback for
> > > > > > set_pkt_metadata() and DEV_TX_OFFLOAD_SEC_NEED_MDATA will also be not set
> > > > > > in capabilities.
> > > > >
> > > > > That's an interesting idea, but what you propose is the change in current rte_security API behaviour.
> > > > > So all existing apps that use this API will have to be changed.
> > > > > We'd better avoid such changes unless there is really good reason for that.
> > > > > So, I'd suggest to tweak your idea a bit:
> > > > >
> > > > > 1) change rte_security_set_pkt_metadata():
> > > > > if ops->set_pkt_metadata != NULL, then call it (existing behaviour)
> > > > > otherwise just: rte_security_dynfield(m) = sess->session_private_data;
> > > > > (fast-path)
> > > > >
> > > > > 2) consider to make rte_security_set_pkt_metadata() inline function.
> > > > > We probably can have some special flag inside struct rte_security_ctx,
> > > > > or even store inside ctx a pointer to set_pkt_metadata() itself.
> > > >
> > > > After another thoughts some new flags might be better.
> > > > Then later, if we'll realize that set_pkt_metadata() and get_useradata()
> > > > are not really used by PMDs, it might be easier to deprecate these callbacks.
> > >
> > > Thanks, I agree with your thoughts. I'll submit a V2 with above change, new flags and
> > > set_pkt_metadata() and get_userdata() function pointers moved to rte_security_ctx for
> > > review so that it can be targeted for 21.11.
> > >
> > > Even with flags moving set_pkt_metadata() and get_userdata() function pointers is still needed
> > > as we need to make rte_security_set_pkt_metadata() API inline while struct rte_security_ops is not
> > > exposed to user. I think this is fine as it is inline with how fast path function pointers
> > > of rte_ethdev and rte_cryptodev are currently placed.
> >
> > My thought was we can get away with just flags only.
> > Something like that:
> > rte_security.h:
> >
> > ...
> >
> > enum {
> > RTE_SEC_CTX_F_FAST_SET_MDATA = 0x1,
> > RTE_SEC_CTX_F_FAST_GET_UDATA = 0x2,
> > };
> >
> > struct rte_security_ctx {
> > void *device;
> > /**< Crypto/ethernet device attached */
> > const struct rte_security_ops *ops;
> > /**< Pointer to security ops for the device */
> > uint16_t sess_cnt;
> > /**< Number of sessions attached to this context */
> > uint32_t flags;
> > };
> >
> > extern int
> > __rte_security_set_pkt_metadata(struct rte_security_ctx *instance,
> > struct rte_security_session *sess,
> > struct rte_mbuf *m, void *params);
> >
> > static inline int
> > rte_security_set_pkt_metadata(struct rte_security_ctx *instance,
> > struct rte_security_session *sess,
> > struct rte_mbuf *m, void *params)
> > {
> > /* fast-path */
> > if (instance->flags & RTE_SEC_CTX_F_FAST_SET_MDATA) {
> > *rte_security_dynfield(m) = (rte_security_dynfield_t)(session->sess_priv_data);
> > return 0;
> > /* slow path */
> > } else
> > return __rte_security_set_pkt_metadata (instance->device, sess, m, params);
> > }
> >
> > rte_security.c:
> >
> > ...
> > /* existing one, just renamed */
> > int
> > __rte_security_set_pkt_metadata(struct rte_security_ctx *instance,
> > struct rte_security_session *sess,
> > struct rte_mbuf *m, void *params)
> > {
> > #ifdef RTE_DEBUG
> > RTE_PTR_OR_ERR_RET(sess, -EINVAL);
> > RTE_PTR_OR_ERR_RET(instance, -EINVAL);
> > RTE_PTR_OR_ERR_RET(instance->ops, -EINVAL);
> > #endif
> > RTE_FUNC_PTR_OR_ERR_RET(*instance->ops->set_pkt_metadata, -ENOTSUP);
> > return instance->ops->set_pkt_metadata(instance->device,
> > sess, m, params);
> > }
> >
> >
> > I think both ways are possible (flags vs actual func pointers) and both have
> > some pluses and minuses.
> > I suppose the main choice here what do we think should be the future of
> > set_pkt_metadata() and rte_security_get_userdata().
> > If we think that they will be useful for some future PMDs and we want to keep them,
> > then probably storing actual func pointers inside ctx is a better approach.
> > If not, then flags seems like a better one, as in that case we can eventually
> > deprecate and remove these callbacks.
> > From what I see right now, custom callbacks seems excessive,
> > and rte_security_dynfield is enough.
> > But might be there are some future plans that would require them?
>
> Above method is also fine. Moving fn pointers to rte_security_ctx can be
> done later if other PMD's need it.
Yes, agree.
>
> Atleast our HW PMD's doesn't plan to use set_pkt_metada()/get_user_data()
> fn pointers in future if above is implemented.
>
> >
> > >
> > > >
> > > > >
> > > > > As a brief code snippet:
> > > > >
> > > > > struct rte_security_ctx {
> > > > > void *device;
> > > > > /**< Crypto/ethernet device attached */
> > > > > const struct rte_security_ops *ops;
> > > > > /**< Pointer to security ops for the device */
> > > > > uint16_t sess_cnt;
> > > > > /**< Number of sessions attached to this context */
> > > > > + int (*set_pkt_mdata)(void *, struct rte_security_session *, struct rte_mbuf *, void *);
> > > > > };
> > > > >
> > > > > static inline int
> > > > > rte_security_set_pkt_metadata(struct rte_security_ctx *instance,
> > > > > struct rte_security_session *sess,
> > > > > struct rte_mbuf *m, void *params)
> > > > > {
> > > > > /* fast-path */
> > > > > if (instance->set_pkt_mdata == NULL) {
> > > > > *rte_security_dynfield(m) = (rte_security_dynfield_t)(session->sess_priv_data);
> > > > > return 0;
> > > > > /* slow path */
> > > > > } else
> > > > > return instance->set_pkt_mdata(instance->device, sess, m, params);
> > > > > }
> > > > >
> > > > > That probably would be an ABI breakage (new fileld in rte_security_ctx) and would require
> > > > > some trivial changes for all existing PMDs that use RTE_SECURITY_TX_OFLOAD_NEED_MDATA
> > > > > (ctx_create()), but hopefully will benefit everyone.
> > > > >
> > > > > >
> > > > > > >
> > > > > > > > I'm fine to
> > > > > > > > introduce a burst call for the same(I was thinking to propose it in future) to
> > > > > > > > compensate for the overhead.
> > > > > > > >
> > > > > > > > If rte_security_set_pkt_metadata() was not a PMD specific function ptr call and
> > > > > > > > rte_mbuf had space for struct rte_security_session pointer,
> > > > > > >
> > > > > > > But it does, see above.
> > > > > > > In fact it even more flexible - because it is driver specific, you are not limited to one 64-bit field.
> > > > > > > If your PMD requires more data to be associated with mbuf
> > > > > > > - you can request it via mbuf_dynfield and store there whatever is needed.
> > > > > > >
> > > > > > > > then then I guess it would have been better to do the way you proposed.
> > > > > > > >
> > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > > >
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > 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
> > > > > > > > > > > > > > >
@@ -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``.
@@ -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. 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,
@@ -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)