Re: [dpdk-dev] [PATCH] doc: announce ABI change for cryptodev config

["Trahe, Fiona" <fiona.trahe@intel.com>]

Hi Thomas, Akhil, Anoob,

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Friday, February 1, 2019 11:14 AM
> To: dev@dpdk.org
> Cc: Akhil Goyal <akhil.goyal@nxp.com>; Anoob Joseph <anoobj@marvell.com>; De Lara Guarch, Pablo
> <pablo.de.lara.guarch@intel.com>; Trahe, Fiona <fiona.trahe@intel.com>; Jerin Jacob Kollanukkaran
> <jerinj@marvell.com>; Narayana Prasad Raju Athreya <pathreya@marvell.com>; Shally Verma
> <shallyv@marvell.com>; Doherty, Declan <declan.doherty@intel.com>
> Subject: Re: [dpdk-dev] [PATCH] doc: announce ABI change for cryptodev config
> 
> There is only one ack for this change.
> A deprecation requires more agreement (3 valuable acks).
> Other opinions?
> 
> 
> 31/01/2019 10:53, Akhil Goyal:
> > On 1/17/2019 3:09 PM, Anoob Joseph wrote:
> > > Add new field ff_enable in rte_cryptodev_config. This enables
> > > applications to control the features enabled on the crypto device.
> > >
> > > Proposed new layout:
> > >
> > > /** Crypto device configuration structure */
> > > struct rte_cryptodev_config {
> > >      int socket_id;            /**< Socket to allocate resources on */
> > >      uint16_t nb_queue_pairs;
> > >      /**< Number of queue pairs to configure on device */
> > > +   uint64_t ff_enable;
> > > +   /**< Feature flags to be enabled on the device. Only the features set
> > > +    * on rte_cryptodev_info.feature_flags are allowed to be set.
> > > +    */
> > > };
> > >
> > > For eth devices, rte_eth_conf.rx_mode.offloads and
> > > rte_eth_conf.tx_mode.offloads fields are used by applications to
> > > control the offloads enabled on the eth device. This proposal adds a
> > > similar ability for the crypto device.
> > >
> > > Signed-off-by: Anoob Joseph <anoobj@marvell.com>
> > >
> > Acked-by: Akhil Goyal <akhil.goyal@nxp.com>
[Fiona] Keeping consistent with ethdev is a lower priority that adding a
param that works well for crypto. As proposed this ff_enable is problematic for crypto
as it makes no sense to enable/disable most of the flags.
For some there's no sensible action a PMD can take, e.g. RTE_CRYPTODEV_FF_HW_ACCELERATED.
For some, PMDs would need to add performance impacting forks on the data path to check for disabled features. E.g. if an applic disables the RTE_CRYPTODEV_FF_CPU_AESNI flag, what does it expect the PMD to do? Not use the aesni capability of the CPU? Fork and do a less performant implementation?
Or if this flag is set, RTE_CRYPTODEV_FF_OOP_LB_IN_LB_OUT or RTE_CRYPTODEV_FF_SYM_OPERATION_CHAINING, does the PMD need to check for operations like these and reject them?
It seems there are only 3 of the 16 flags that it's desirable to disable, based on the earlier thread
RTE_CRYPTODEV_FF_ASYMMETRIC_CRYPTO
RTE_CRYPTODEV_FF_SYMMETRIC_CRYPTO
RTE_CRYPTODEV_FF_SECURITY
So I propose this param should be called ff_disable.
It should documented - and maybe provide a mask for - the flags the application is allowed to disable - only the above 3. Then PMDs only need to implement enable/disable functionality for that subset of flags.