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 <ano...@marvell.com>; Trahe, Fiona > <fiona.tr...@intel.com>; Akhil Goyal <akhil.go...@nxp.com>; De Lara Guarch, > Pablo <pablo.de.lara.gua...@intel.com> > Cc: Jerin Jacob Kollanukkaran <jer...@marvell.com>; Narayana Prasad Raju > Athreya <pathr...@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.tr...@intel.com>; Akhil Goyal > ><akhil.go...@nxp.com>; De Lara Guarch, Pablo > ><pablo.de.lara.gua...@intel.com> > >Cc: Jerin Jacob Kollanukkaran <jer...@marvell.com>; Narayana Prasad > >Raju Athreya <pathr...@marvell.com>; Shally Verma > ><shal...@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.tr...@intel.com> > >> Sent: 17 January 2019 17:07 > >> To: Anoob Joseph <ano...@marvell.com>; Akhil Goyal > >> <akhil.go...@nxp.com>; De Lara Guarch, Pablo > >> <pablo.de.lara.gua...@intel.com> > >> Cc: Jerin Jacob Kollanukkaran <jer...@marvell.com>; Narayana Prasad > >> Raju Athreya <pathr...@marvell.com>; Shally Verma > >> <shal...@marvell.com>; dev@dpdk.org; Trahe, Fiona > >> <fiona.tr...@intel.com> > >> Subject: RE: [PATCH] doc: announce ABI change for cryptodev config > >> > >> Hi Joseph, > >> > >> > -----Original Message----- > >> > From: Anoob Joseph [mailto:ano...@marvell.com] > >> > Sent: Thursday, January 17, 2019 9:40 AM > >> > To: Akhil Goyal <akhil.go...@nxp.com>; De Lara Guarch, Pablo > >> > <pablo.de.lara.gua...@intel.com>; Trahe, Fiona > >> > <fiona.tr...@intel.com> > >> > Cc: Anoob Joseph <ano...@marvell.com>; Jerin Jacob Kollanukkaran > >> > <jer...@marvell.com>; Narayana Prasad Raju Athreya > >> > <pathr...@marvell.com>; Shally Verma <shal...@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.
