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

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

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