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

[Anoob Joseph <anoobj@marvell.com>]

Hi Fiona,

Any more comments on this?

@ Akhil, Pablo

Can you review this change and share your thoughts?

Thanks,
Anoob

> -----Original Message-----
> From: Shally Verma
> Sent: 18 January 2019 12:29
> To: Anoob Joseph <anoobj@marvell.com>; Trahe, Fiona
> <fiona.trahe@intel.com>; Akhil Goyal <akhil.goyal@nxp.com>; De Lara Guarch,
> Pablo <pablo.de.lara.guarch@intel.com>
> Cc: Jerin Jacob Kollanukkaran <jerinj@marvell.com>; Narayana Prasad Raju
> Athreya <pathreya@marvell.com>; dev@dpdk.org
> Subject: RE: [PATCH] doc: announce ABI change for cryptodev config
> 
> HI Fiona, Anoob
> 
> >-----Original Message-----
> >From: Anoob Joseph
> >Sent: 17 January 2019 19:17
> >To: Trahe, Fiona <fiona.trahe@intel.com>; Akhil Goyal
> ><akhil.goyal@nxp.com>; De Lara Guarch, Pablo
> ><pablo.de.lara.guarch@intel.com>
> >Cc: Jerin Jacob Kollanukkaran <jerinj@marvell.com>; Narayana Prasad
> >Raju Athreya <pathreya@marvell.com>; Shally Verma
> ><shallyv@marvell.com>; dev@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@intel.com>
> >> Sent: 17 January 2019 17:07
> >> To: Anoob Joseph <anoobj@marvell.com>; Akhil Goyal
> >> <akhil.goyal@nxp.com>; De Lara Guarch, Pablo
> >> <pablo.de.lara.guarch@intel.com>
> >> Cc: Jerin Jacob Kollanukkaran <jerinj@marvell.com>; Narayana Prasad
> >> Raju Athreya <pathreya@marvell.com>; Shally Verma
> >> <shallyv@marvell.com>; dev@dpdk.org; Trahe, Fiona
> >> <fiona.trahe@intel.com>
> >> Subject: RE: [PATCH] doc: announce ABI change for cryptodev config
> >>
> >> Hi Joseph,
> >>
> >> > -----Original Message-----
> >> > From: Anoob Joseph [mailto:anoobj@marvell.com]
> >> > Sent: Thursday, January 17, 2019 9:40 AM
> >> > To: Akhil Goyal <akhil.goyal@nxp.com>; De Lara Guarch, Pablo
> >> > <pablo.de.lara.guarch@intel.com>; Trahe, Fiona
> >> > <fiona.trahe@intel.com>
> >> > Cc: Anoob Joseph <anoobj@marvell.com>; Jerin Jacob Kollanukkaran
> >> > <jerinj@marvell.com>; Narayana Prasad Raju Athreya
> >> > <pathreya@marvell.com>; Shally Verma <shallyv@marvell.com>;
> >> > dev@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.