Hi Akhil, I'll send a v2 incorporating Fiona's comments.
Thanks, Anoob > -----Original Message----- > From: Akhil Goyal <akhil.go...@nxp.com> > Sent: Thursday, February 28, 2019 3:32 PM > To: Trahe, Fiona <fiona.tr...@intel.com>; Thomas Monjalon > <tho...@monjalon.net>; dev@dpdk.org > Cc: Anoob Joseph <ano...@marvell.com>; De Lara Guarch, Pablo > <pablo.de.lara.gua...@intel.com>; Jerin Jacob Kollanukkaran > <jer...@marvell.com>; Narayana Prasad Raju Athreya > <pathr...@marvell.com>; Shally Verma <shal...@marvell.com>; Doherty, > Declan <declan.dohe...@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:tho...@monjalon.net] > >> Sent: Friday, February 1, 2019 11:14 AM > >> To: dev@dpdk.org > >> Cc: Akhil Goyal <akhil.go...@nxp.com>; Anoob Joseph > >> <ano...@marvell.com>; De Lara Guarch, Pablo > >> <pablo.de.lara.gua...@intel.com>; Trahe, Fiona > >> <fiona.tr...@intel.com>; Jerin Jacob Kollanukkaran > >> <jer...@marvell.com>; Narayana Prasad Raju Athreya > >> <pathr...@marvell.com>; Shally Verma <shal...@marvell.com>; > Doherty, > >> Declan <declan.dohe...@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 <ano...@marvell.com> > >>>> > >>> Acked-by: Akhil Goyal <akhil.go...@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
