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

[Suanming Mou <suanmingm@nvidia.com>]

> -----Original Message-----
> From: Akhil Goyal <gakhil@marvell.com>
> Sent: Thursday, June 20, 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
> 
> > > 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.
> > >
> > > Even if it is an optional thing.
> > > After adding the devarg, the user is expected to use the buffers the
> > > way your PMD is expecting. So, this is a restriction. Right?
> >
> > The devarg is not enabled by default, if user adds the devarg, that
> > means user know what he is doing, and the input is suitable for that
> optimization.
> > PMD doesn't restrict user must use that hint to handle IPsec case,
> > user will still be able to handle IPsec operation without that devarg.
> 
> Devarg is optional and not enabled by default. That is clear to me at the first
> place.
> 
> The point is PMD devarg can dictate the behavior of PMD and NOT the user.
> The standard APIs are the ones which user must adhere to.
> 
> You cannot expect a user to change its code when it wants to use the devarg
> optimization.

Sorry, it is my bad missing the background introduction here. In fact, the motivation to implement that feature is not to request user to build the memory in that order.
Opsitely, after the first UMR version, we noticed that user may have IPsec as input and that input can be optimized by that hint.
Since the API does not reject user's IPsec input, so finally we decided to add that hint to optimize the IPsec input case.
I agree we can implement based on other API and force user to use other API, but if the current API does not reject user's IPsec input, I assume it is worth to optimize that. What do you think?

> 
> > If user has mixed cases, just leave the devarg away, does that make sense?
> >
> > >
> > > What would be the behavior if devarg is set but the buffers are
> > > configured the same way as before?
> > >
> > > > 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.
> > >
> > > I do not understand how it is bringing better compatibility.
> > >
> > > The devarg that is added for ipsec_opt seems redundant.
> > > We should use standard APIs when they are available.
> > > Devargs are added to pass on additional run time configuration which
> > > is not part of standard API set and is specific to a particular PMD.
> > > But in this case, we do have rte_security and rte_ipsec APIs to
> > > configure IPsec specific requirements.
> >
> > OK, I assume you agreed before that the standard API does not define
> > the memory layout, right?
> Yes it does not for crypto APIs.
> 
> But if we use rte_security and rte_ipsec APIs, format of packets should be as
> per IPsec requirement.
> That is implicit for the application to improve performance.

I always agree with rte_security and rte_ipsec APIs, and this is what we want to expand next. but as replied above, the hint does not conflicts with these API, we can have both supported finally.
User can choose anyway he wants since current API does not rejects the IPsec inputs. What do you think?

> 
> > AAD, IV and payload are all defined separately. The API is not
> > affected. Let's align the patch does not break the API.
> > And another reason to have it is due to AES_GCM can also be use as
> > IPsec mechanism(that is what has been merged). What do you think?
> 
> That is supported and tested using ipsec-secgw as well as dpdk-test
> application.
> But adding a PMD devarg for explicitly supporting IPsec is not required.
> User cannot be dictated by the PMD devarg to align its buffers.

Just to be more accurate, in fact our current PMD has supported IPsec already via UMR's way. This series is an optimization, not newly supporting IPsec, but to optimize the IPsec input case.
And like I replied above, it is not the patches invites the layout and require user to have such buffers, but the truth is we want to better serve the IPsec input come with that layout case in the real world as API does not reject that IPsec input.

> 
> > And again, I still agree with you other API may also be able to
> > achieve that. But that's another topic, for now, we expect mlx5 PMD
> > AES_GCM can serve IPsec with and without the hint(after that patch).
> >
> > Thanks,
> > Suanming