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

[Anoob Joseph <anoobj@marvell.com>]

Hi Akhil,

I'll send a v2 incorporating Fiona's comments. 

Thanks,
Anoob

> -----Original Message-----
> From: Akhil Goyal <akhil.goyal@nxp.com>
> Sent: Thursday, February 28, 2019 3:32 PM
> To: Trahe, Fiona <fiona.trahe@intel.com>; Thomas Monjalon
> <thomas@monjalon.net>; dev@dpdk.org
> Cc: Anoob Joseph <anoobj@marvell.com>; De Lara Guarch, Pablo
> <pablo.de.lara.guarch@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
> 
> Hi Anoob,
> 
> On 2/1/2019 5:19 PM, Trahe, Fiona wrote:
> > 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.
> 
> could you send a new version of this patch as per the comments from Fiona.
> 
> Thanks,
> Akhil