Re: [dpdk-dev] [PATCH] cryptodev: extend api of asymmetric crypto by sessionless

[Akhil Goyal <akhil.goyal@nxp.com>]

Hi Fiona

> 
> Hi Akhil,
> 
> > -----Original Message-----
> > From: Akhil Goyal [mailto:akhil.goyal@nxp.com]
> > Sent: Friday, June 21, 2019 1:23 PM
> > To: Trahe, Fiona <fiona.trahe@intel.com>; Kusztal, ArkadiuszX
> <arkadiuszx.kusztal@intel.com>;
> > dev@dpdk.org; shally.verma@caviumnetworks.com
> > Subject: RE: [PATCH] cryptodev: extend api of asymmetric crypto by
> sessionless
> >
> > Hi Fiona,
> >
> > >
> > > Hi Akhil, Arek, Shally,
> > >
> > > > -----Original Message-----
> > > > From: Akhil Goyal [mailto:akhil.goyal@nxp.com]
> > > > Sent: Thursday, June 20, 2019 3:17 PM
> > > > To: Kusztal, ArkadiuszX <arkadiuszx.kusztal@intel.com>; dev@dpdk.org
> > > > Cc: Trahe, Fiona <fiona.trahe@intel.com>;
> > > shally.verma@caviumnetworks.com
> > > > Subject: RE: [PATCH] cryptodev: extend api of asymmetric crypto by
> > > sessionless
> > > > >
> > > > > Asymmetric cryptography algorithms may more likely use
> > > > > sessionless API so there is need to extend API.
> > > > >
> > > > > Signed-off-by: Arek Kusztal <arkadiuszx.kusztal@intel.com>
> > > > > ---
> > > > Acked-by: Akhil Goyal <akhil.goyal@nxp.com>
> > >
> > > [Fiona] The code is ok but I think a little more is needed.
> > > As all PMDs don't support sessionless, this needs to be handled as an
> optional
> > > capability.
> > > And in future some PMDs may only support SESSIONLESS and some only
> support
> > > WITH_SESSION.
> >
> >
> > I believe this holds true for symmetric crypto as well. But adding a feature flag
> for everything may beat
> > the purpose
> > Of adding a feature flag. Sessionless crypto operations in symmetric crypto is
> being used without any
> > issue for a long
> > And nobody feel the need of that as of today. So my question is how
> asymmetric crypto pmds are
> > different that they
> > Need feature flag?
> >
> > If the driver does not support sessionless, then it may give an error while
> creating it. I don't think that is
> > an issue. It is
> > Already being handled in the rte_crypto_op by an enum which denote that the
> 'op' need to be
> > processed with some
> > Session or with xform.
> >
> > > So I propose adding 2 feature flags to the API
> > > RTE_CRYPTODEV_FF_ASYM_WTH_SESSION
> > > RTE_CRYPTODEV_FF_ASYM_SESSIONLESS
> > > and including in this patch the PMD and UT changes to set and test the first
> flag.
> [Fiona] symmetric crypto is inherently session-based, so all PMDs support this. I
> don't know how much real use SESSIONLESS actually gets.
> For asymmetric, my understanding is that sessionless is more likely to be used.
> Sequences of ops using the same params/keys are an unlikely use-case, so
> there's no advantage to setting up a session and it's an extra API call so
> preferable to avoid.

Agreed, if Asymmetric is not likely to have sessions, it should not call that API.

> That said, I think it would be ok with one feature flag.
> If a PMD doesn't support WITH_SESSION, the session_init  API will fail with -
> ENOTSUP, so giving the app the information it needs. This can be documented
> as a PMD limitation and I'm ok with it not having a feature flag.
> However if a PMD doesn't support SESSIONLESS, then the fail will only occur on
> the op_enqueue_burst.

Yes on the first enqueue, before actually submitting to the hardware, in the driver itself
Before sending the request to hardware and return OP_INVALID_SESSION.

> Failure to enqueue the next op is a typical outcome on a busy hardware device,
> and the app will likely assume the device is busy and try again with same result.

The PMD will not be sending the request to hardware if sessionless is not supported.
And app will not enqueue the op again if the previous error is OP_INVALID_SESSION.

> The PMD could change the op.status to OP_STATUS_ERROR  or
> OP_INVALID_SESSION but it would still require the app to check the status of the
> next op which failed to enqueue. I think it better to detect this before the
> op_enqueue by providing a RTE_CRYPTODEV_FF_ASYM_SESSIONLESS feature
> flag.

On second thought, we can have another value in the enum(op->status) to say sessionless
Is not supported if OP_INVALID_SESSION looks ambiguous instead of setting a feature flag 
and making each driver set it if it support sessionless or not.

I believe that would be simple and will not bother the data path or the busy hardware.
Anyways in case of sessions also in sym, we make session on arrival of 1st packet. That same logic
Can be applied here also. I don't think that will be an issue.

> 
> 
> > > We'll follow up with SESSIONLESS QAT implementation and UTs in a separate
> > > patchset.
> > > Also documentation updates should go with this API patch, i.e.
> > >  - update section 16.7.2 in the cryptodev programmers guide - and review
> that
> > > doc in case other sections need updating.
> > Yes this needs to be updated if the implementation is complete and we have
> some PMD supporting
> > that.
> [Fiona] Are you saying we need to submit the PMD changes with this API patch?
> Or can we do
> separately as we'd planned?

I believe you should update the documentation when some PMD support sessionless.
I think we should hold back this patch, until you have a sample PMD/app ready for reference.

You may end up adding a few more updates to the lib while actually supporting the functionality.

> 
> >
> > >  - fix comment in rte_crypto.h under STATUS_INVALID_SESSION
> > >  - release note
> > >
> > >
> >
> >
> > -Akhil