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

[Shally Verma]

HI Fiona, Anoob

>-----Original Message-----
>From: Anoob Joseph
>Sent: 17 January 2019 19:17
>To: Trahe, Fiona <fiona.trahe at intel.com>; Akhil Goyal <akhil.goyal at nxp.com>; De Lara Guarch, Pablo
><pablo.de.lara.guarch at intel.com>
>Cc: Jerin Jacob Kollanukkaran <jerinj at marvell.com>; Narayana Prasad Raju Athreya <pathreya at marvell.com>; Shally Verma
><shallyv at marvell.com>; dev at dpdk.org
>Subject: RE: [PATCH] doc: announce ABI change for cryptodev config
>
>Hi Fiona,
>
>Please see inline.
>
>Thanks,
>Anoob
>
>> -----Original Message-----
>> From: Trahe, Fiona <fiona.trahe at intel.com>
>> Sent: 17 January 2019 17:07
>> To: Anoob Joseph <anoobj at marvell.com>; Akhil Goyal <akhil.goyal at nxp.com>;
>> De Lara Guarch, Pablo <pablo.de.lara.guarch at intel.com>
>> Cc: Jerin Jacob Kollanukkaran <jerinj at marvell.com>; Narayana Prasad Raju
>> Athreya <pathreya at marvell.com>; Shally Verma <shallyv at marvell.com>;
>> dev at dpdk.org; Trahe, Fiona <fiona.trahe at intel.com>
>> Subject: RE: [PATCH] doc: announce ABI change for cryptodev config
>>
>> Hi Joseph,
>>
>> > -----Original Message-----
>> > From: Anoob Joseph [mailto:anoobj at marvell.com]
>> > Sent: Thursday, January 17, 2019 9:40 AM
>> > To: Akhil Goyal <akhil.goyal at nxp.com>; De Lara Guarch, Pablo
>> > <pablo.de.lara.guarch at intel.com>; Trahe, Fiona <fiona.trahe at intel.com>
>> > Cc: Anoob Joseph <anoobj at marvell.com>; Jerin Jacob Kollanukkaran
>> > <jerinj at marvell.com>; Narayana Prasad Raju Athreya
>> > <pathreya at marvell.com>; Shally Verma <shallyv at marvell.com>;
>> > dev at dpdk.org
>> > Subject: [PATCH] doc: announce ABI change for cryptodev config
>> >
>> > 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.
>> [Fiona] I'm unfamiliar with eth config so can you explain a bit more how this
>> works?
>
>[Anoob] For eth devices, application would first do rte_eth_dev_info_get() and gets the capabilities. The device would expose it's
>capabilities using dev_info.rx_offload_capa & dev_info.tx_offload_capa. The application can enable/disable these features while
>configuring the eth ports.
>
>From ipsec-secgw:
>https://git.dpdk.org/dpdk/tree/examples/ipsec-secgw/ipsec-secgw.c#n1866
>
>if (frame_size) {
>		local_port_conf.rxmode.max_rx_pkt_len = frame_size;
>		local_port_conf.rxmode.offloads |= DEV_RX_OFFLOAD_JUMBO_FRAME;
>	}
>
><snip>
>
>ret = rte_eth_dev_configure(portid, nb_rx_queue, nb_tx_queue,
>			&local_port_conf);
>
><snip>
>
>This way application can choose to disable unwanted offloads.
>
>Port init in ipsec-secgw: https://git.dpdk.org/dpdk/tree/examples/ipsec-secgw/ipsec-secgw.c#n1826
>
>> e.g. if a crypto device's info says it supports
>> RTE_CRYPTODEV_FF_SYMMETRIC_CRYPTO
>> RTE_CRYPTODEV_FF_SYM_OPERATION_CHAINING
>> RTE_CRYPTODEV_FF_CPU_AESNI
>> RTE_CRYPTODEV_FF_SECURITY
>> RTE_CRYPTODEV_FF_OOP_LB_IN_LB_OUT
>> these things are all already enabled.
>> Is the param a way to disable some of them?
>
>[Anoob] Yes. There are few other flags in addition to the above. Like RTE_CRYPTODEV_FF_ASYMMETRIC_CRYPTO.
>
>> If so, it would be better to call it ff_disable.
>
>[Anoob] Calling it ff_enable is to make it similar to how it's done with eth devices. Either way should be fine.
[Shally]  keeping it as "ff_enable"  will require application to set these flags to configure PMD. If we define it other way around, then it would be mean to mask out unwanted features which can be quite many. 
Though purpose can be achieved both ways but keeping it as "enable" looks more easy to use, readable and inline with how ethdev handle multi feature support.
So I would prefer to keep it as "ff_enable"

Thanks
Shally

>
>> And to limit it to the subset of features that it makes sense to disable.
>> i.e. why would an application disable chaining or any of the SGL flags? It would
>> just add extra cycles in the PMD to error check  on these cases, instead the appl
>> can just not send such commands.
>> And it doesn't make sense to disable AESNI or
>> RTE_CRYPTODEV_FF_HW_ACCELERATED.
>> Actually I can't see what an application would want to achieve by disabling any
>> flag?
>
>[Anoob] Features like ASYMMETRIC or SECURITY is not required for every application. SECURITY is added for eth devices also. But
>since the feature can be disabled while configuring the port, applications are given a choice to disable it. That way apps like l2fwd
>doesn't enable SECURITY. With crypto this option is not available.
>
>If the unused offloads can be communicated to the PMD before initialization, the PMD will be allowed to optimize hardware usage.
>Otherwise, supporting more features would affect performance, even if application is not making use of those features.
>
>Ex: test-crypto-perf doesn't use ASYM/SECURITY. Now adding these features would affect the performance of this app.