Hi Akhil, I'll send a v2 incorporating Fiona's comments.
Thanks, Anoob > -----Original Message----- > From: Akhil Goyal <[email protected]> > Sent: Thursday, February 28, 2019 3:32 PM > To: Trahe, Fiona <[email protected]>; Thomas Monjalon > <[email protected]>; [email protected] > Cc: Anoob Joseph <[email protected]>; De Lara Guarch, Pablo > <[email protected]>; Jerin Jacob Kollanukkaran > <[email protected]>; Narayana Prasad Raju Athreya > <[email protected]>; Shally Verma <[email protected]>; Doherty, > Declan <[email protected]> > 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:[email protected]] > >> Sent: Friday, February 1, 2019 11:14 AM > >> To: [email protected] > >> Cc: Akhil Goyal <[email protected]>; Anoob Joseph > >> <[email protected]>; De Lara Guarch, Pablo > >> <[email protected]>; Trahe, Fiona > >> <[email protected]>; Jerin Jacob Kollanukkaran > >> <[email protected]>; Narayana Prasad Raju Athreya > >> <[email protected]>; Shally Verma <[email protected]>; > Doherty, > >> Declan <[email protected]> > >> 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 <[email protected]> > >>>> > >>> Acked-by: Akhil Goyal <[email protected]> > > [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
