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

[Akhil Goyal <gakhil@marvell.com>]

> > > > > > > > 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.

Ok got it.

But if we set the devarg for a mlx5 device, it will be a hint to the PMD for all the flows that it will process. Right?

And there may be an application use case where it has 2 separate flows simultaneously - IPsec and non-IPsec.

Then how would PMD handle that? 
Will it assume contiguous memory for non-IPsec case as well?
How will PMD identify between IPsec and non-IPsec case?


> 
> >
> > > 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