RE: [EXTERNAL] [PATCH v2 1/2] crypto/mlx5: optimize AES-GCM IPsec operation

[Suanming Mou <suanmingm@nvidia.com>]

Hi Akhil,

Any other suggestions regarding the series?

BR,
Suanming

> -----Original Message-----
> From: Suanming Mou <suanmingm@nvidia.com>
> Sent: Friday, June 14, 2024 5:32 PM
> To: Akhil Goyal <gakhil@marvell.com>; Matan Azrad <matan@nvidia.com>
> Cc: dev@dpdk.org
> Subject: RE: [EXTERNAL] [PATCH v2 1/2] crypto/mlx5: optimize AES-GCM
> IPsec operation
> 
> Hi,
> 
> > -----Original Message-----
> > From: Akhil Goyal <gakhil@marvell.com>
> > Sent: Friday, June 14, 2024 5:07 PM
> > To: Suanming Mou <suanmingm@nvidia.com>; Matan Azrad
> > <matan@nvidia.com>
> > Cc: dev@dpdk.org
> > Subject: RE: [EXTERNAL] [PATCH v2 1/2] crypto/mlx5: optimize AES-GCM
> > IPsec operation
> >
> > > Hi Akhil,
> > >
> > > > -----Original Message-----
> > > > From: Akhil Goyal <gakhil@marvell.com>
> > > > Sent: Friday, June 14, 2024 2:49 PM
> > > > To: Suanming Mou <suanmingm@nvidia.com>; Matan Azrad
> > > > <matan@nvidia.com>
> > > > Cc: dev@dpdk.org
> > > > Subject: RE: [EXTERNAL] [PATCH v2 1/2] crypto/mlx5: optimize
> > > > AES-GCM IPsec operation
> > > >
> > > > > To optimize AES-GCM IPsec operation within crypto/mlx5, the DPDK
> > > > > API typically supplies AES_GCM AAD/Payload/Digest in separate
> > > > > locations, potentially disrupting their contiguous layout. In
> > > > > cases where the memory layout fails to meet hardware (HW)
> > > > > requirements, an UMR WQE is initiated ahead of the GCM's GGA WQE
> > > > > to establish a continuous AAD/Payload/Digest virtual memory
> > > > > space for the
> > HW MMU.
> > > > >
> > > > > For IPsec scenarios, where the memory layout consistently
> > > > > adheres to the fixed order of AAD/IV/Payload/Digest, directly
> > > > > shrinking memory for AAD proves more efficient than preparing a
> > > > > UMR WQE. To address this, a new devarg "crypto_mode" with mode
> > > > > "ipsec_opt" is introduced in the commit, offering an
> > > > > optimization hint specifically for IPsec cases. When enabled,
> > > > > the PMD copies AAD directly before Payload in the enqueue_burst
> > > > > function instead of employing the UMR WQE. Subsequently, in the
> > > > > dequeue_burst function, the overridden IV before Payload is
> > > > > restored from the GGA WQE. It's crucial for users to avoid
> > > > > utilizing the input mbuf data during
> > processing.
> > > >
> > > > This seems very specific to mlx5 and is not as per the
> > > > expectations of cryptodev APIs.
> > > >
> > > > It seems you are asking to alter the user application to
> > > > accommodate this to support IPsec.
> > > >
> > > > Cryptodev APIs are for generic crypto processing of data as
> > > > defined in rte_crypto_op.
> > > > With your proposed changes, it seems the behavior of the crypto
> > > > APIs will be different in case of mlx5 which I believe is not correct.
> > > >
> > > > Is it not possible for you to use rte_security IPsec path?
> > > >
> > >
> > > Sorry I don't understand why that conflicts the API, IIUC crypto API
> > > only just defines the AAD/Payload/Digest in struct
> > > rte_crypto_sym_op, but not restrict the sequence, and
> > > AAD/Payload/Digest may come from
> > difference memory space.
> > > Am I missing something here?
> >
> > Yes you are correct that there is no restriction there.
> >
> > > The input requirements from mlx5 HW is AAD/Payload/Digest sequence,
> > > if the input memory of AAD/Payload/Digest does not meet the
> > > requirements, PMD will try to combine the memory address space with
> > > UMR WQE as that commit does by software shrink.
> >
> > And here, you are adding a restriction for IPsec case.
> > I believe you need a way to identify IPsec case with non-ipsec case in data
> path.
> > For that, instead of using a devarg(which is a specific case for
> > mlx5), you can use generic rte_security session with action type
> > RTE_SECURITY_ACTION_TYPE_NONE.
> 
> Just to emphasize, this is not a restriction, we don't restrict user must use that
> devarg for IPSEC case.
> The way to identify or apply that optimization is user's devarg of "ipsec_opt".
> Without that hint from devarg, pmd will work in UMR mode to combine the
> memory addresses.
> I agree move to other API will also make the hint work. But if providing one
> hint devarg here does not conflict the API and bring better compatibility, it
> does not hurt.
> 
> >
> > You may also benefit from rte_ipsec library APIs and test framework,
> > for processing of protocol specific things which are specifically
> > written for RTE_SECURITY_ACTION_TYPE_NONE case.
> And again, thanks for the suggestion, I assume we will also consider
> supporting that next for rte_security as well if possible, to provide more
> choice for user.
> 
> >
> > > And the most important thing is that "ipsec_opt" is not mandatory,
> > > only if user have such case of layout and allows that optimization
> > > happens. Otherwise, the existing UMR WQE will still be the default
> > > behavior
> > here.
> > >
> > With this new devarg which application would you be using for testing?
> > I do not see a patch for application changes for the layout that you
> > are mentioning.
> IIRC we used test-crypto-perf with headroom proper configured mbuf to
> verify.
> If you think another new arg is worth to be added to test-crypto-perf to build
> everything, I can also send another application patch to support that later.(but
> sorry due to effort limitation maybe not happened soon in that series).
> 
> Thanks,
> Suanming