From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by dpdk.org (Postfix) with ESMTP id 214CE5963 for ; Wed, 10 Feb 2016 17:37:36 +0100 (CET) Received: from orsmga002.jf.intel.com ([10.7.209.21]) by orsmga102.jf.intel.com with ESMTP; 10 Feb 2016 08:37:36 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.22,426,1449561600"; d="scan'208";a="909368417" Received: from irsmsx110.ger.corp.intel.com ([163.33.3.25]) by orsmga002.jf.intel.com with ESMTP; 10 Feb 2016 08:37:35 -0800 Received: from irsmsx101.ger.corp.intel.com ([169.254.1.113]) by irsmsx110.ger.corp.intel.com ([169.254.15.31]) with mapi id 14.03.0248.002; Wed, 10 Feb 2016 16:37:34 +0000 From: "Trahe, Fiona" To: "Doherty, Declan" , "dev@dpdk.org" Thread-Topic: [dpdk-dev] [PATCH] cryptodev: add capabilities discovery mechanism Thread-Index: AQHRW1/qkoEImr/+m0ytkQ6qDIa5g58la7wQ Date: Wed, 10 Feb 2016 16:37:34 +0000 Message-ID: <348A99DA5F5B7549AA880327E580B43588F66996@IRSMSX101.ger.corp.intel.com> References: <1454159457-6857-1-git-send-email-declan.doherty@intel.com> In-Reply-To: <1454159457-6857-1-git-send-email-declan.doherty@intel.com> Accept-Language: en-IE, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiZjgwNzg1ZGEtMTdkYy00ODA0LThiNTgtNTBjOGI5ZjA2ZGYwIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX0lDIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE1LjkuNi42IiwiVHJ1c3RlZExhYmVsSGFzaCI6IlJyNFlRUm9sbEhkZXdDUG10K2M1M0d4cGRZQm9qRm1NUzRtMHhMQ2NFZXM9In0= x-ctpclassification: CTP_IC x-originating-ip: [163.33.239.181] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH] cryptodev: add capabilities discovery mechanism X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 10 Feb 2016 16:37:37 -0000 Hi Declan, > -----Original Message----- > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Declan Doherty > Sent: Saturday, January 30, 2016 1:11 PM > To: dev@dpdk.org > Subject: [dpdk-dev] [PATCH] cryptodev: add capabilities discovery mechani= sm >=20 > This patch add a mechanism for discovery of crypto device features and > supported crypto operations and algorithms. It also provides a method for= a > crypto PMD to publish any data range limitations it may have for the oper= ations > and algorithms it supports. >=20 > The parameter feature_flags added to rte_cryptodev struct is used to capt= ure > features such as operations supported (symmetric crypto, operation chaini= ng > etc) as well parameter such as whether the device is hardware accelerated= or > uses SIMD instructions. >=20 > The capabilities parameter allows a PMD to define an array of supported > operations with any limitation which that implementation may have. >=20 > Finally the rte_cryptodev_info struct has been extended to allow retrieva= l of > these parameter using the existing rte_cryptodev_info_get() API. >=20 > Signed-off-by: Declan Doherty There's a lot of good data in the Capabilities struct, but is there a use-c= ase for it? Is it likely an application will call this API and validate ev= erything? Or is it more likely it would just check for algorithm support and if it th= en uses an unsupported block_size, key_size, etc it will get an invalid par= am from the PMD and handle this?=20 Secondly, instead of spreading capabilities across 2 data elements (feature= bitmask and op capabilities struct), see suggestion below to roll all into= a single struct. //snip// > + > +/** > + * Symmetric Crypto Capability > + */ > +struct rte_cryptodev_symmetric_capability { > + enum rte_crypto_xform_type type; > + /**< Transform type : Authentication / Cipher */ > + It would be more helpful to call this xform_type > + union { > + struct { > + enum rte_crypto_auth_algorithm algo; > + /**< authentication algorithm */ > + uint16_t block; > + /**< algorithm block size */ > + uint16_t key; > + /**< Key size supported */ > + uint16_t digest; > + /**< Maximum digest size supported */ > + struct { > + uint16_t min; /**< minimum aad size */ > + uint16_t max; /**< maximum aad size */ > + uint16_t increment; > + /**< if a range of sizes are supported, > + * this parameter is used to indicate > + * increments in byte size that are supported > + * between the minimum and maximum */ > + } aad; > + /**< Additional authentication data size range */ > + } auth; > + /**< Symmetric Authentication transform capabilities */ > + struct { > + enum rte_crypto_cipher_algorithm algo; > + /**< cipher algorithm */ > + uint16_t block_size; > + /**< algorithm block size */ > + struct { > + uint16_t min; /**< minimum key size */ > + uint16_t max; /**< maximum key size */ > + uint16_t increment; > + /**< if a range of sizes are supported, > + * this parameter is used to indicate > + * increments in byte size that are supported > + * between the minimum and maximum */ > + } key; > + /**< cipher key size range */ > + struct { > + uint16_t min; /**< minimum iv size */ > + uint16_t max; /**< maximum iv size */ > + uint16_t increment; > + /**< if a range of sizes are supported, > + * this parameter is used to indicate > + * increments in byte size that are supported > + * between the minimum and maximum */ > + } iv; > + /**< Initialisation vector data size range */ > + } cipher; > + /**< Symmetric Cipher transform capabilities */ > + }; > +}; > + > +/** Structure used to capture a capability of a crypto device */ struct > +rte_cryptodev_capabilities { > + enum rte_crypto_op_type op; > + /**< Operation type */ > + > + union { > + struct rte_cryptodev_symmetric_capability sym; > + /**< Symmetric operation capability parameters */ > + }; > +}; > + > +/** Macro used at end of crypto PMD list */ #define > +RTE_CRYPTODEV_END_OF_CAPABILITIES_LIST() \ > + { RTE_CRYPTO_OP_TYPE_UNDEFINED } > + > + > +/** > + * Crypto device supported feature flags > + * > + * Note: > + * New features flags should be added to the end of the list > + * > + * Keep these flags synchronised with rte_cryptodev_get_feature_name() > +*/ > +#define RTE_CRYPTODEV_FF_SYMMETRIC_CRYPTO (1ULL << 0) > +/**< Symmetric crypto operations are supported */ > +#define RTE_CRYPTODEV_FF_ASYMMETRIC_CRYPTO (1ULL << 1) > +/**< Asymmetric crypto operations are supported */ > +#define RTE_CRYPTODEV_FF_OPERATION_CHAINING (1ULL << 2) > +/**< Chaining crypto operations are supported */ Should this flag be moved into the rte_cryptodev_symmetric_capability and h= ave a more complex structure? e.g. chaining of AES and SHA-HMAC could be supported, but not chaining of s= now3g UEA2/UIA2? But this leads to my overall comment - do we really need this level of deta= il in capabilities? If keeping as a feature flag, then call it SYM_OP_CHAINING? > +#define RTE_CRYPTODEV_FF_CPU_SSE (1ULL << 3) > +/**< Utilises CPU SIMD SSE instructions */ > +#define RTE_CRYPTODEV_FF_CPU_AVX (1ULL << 4) > +/**< Utilises CPU SIMD AVX instructions */ > +#define RTE_CRYPTODEV_FF_CPU_AVX2 (1ULL << 5) > +/**< Utilises CPU SIMD AVX2 instructions */ > +#define RTE_CRYPTODEV_FF_CPU_AESNI (1ULL << 6) > +/**< Utilises CPU AES-NI instructions */ > +#define RTE_CRYPTODEV_FF_HW_ACCELERATED (1ULL > << 7) > +/**< Operations are off-loaded to an external hardware accelerator */ > + > + > +/** > + * Get the name of a crypto device feature flag > + * > + * @param mask The mask describing the flag. > + * > + * @return > + * The name of this flag, or NULL if it's not a valid feature flag. > + */ > + > +extern const char * > +rte_cryptodev_get_feature_name(uint64_t flag); > + > /** Crypto device information */ > struct rte_cryptodev_info { > const char *driver_name; /**< Driver name. */ > enum rte_cryptodev_type dev_type; /**< Device type */ > struct rte_pci_device *pci_dev; /**< PCI information. */ >=20 > + uint64_t feature_flags; /**< Feature flags */ > + > + const struct rte_cryptodev_capabilities *capabilities; > + /**< Array of devices supported capabilities */ > + Instead of spreading capabilities across 2 data elements (feature_flags bit= mask and op capabilities struct), I think it's better to roll all into a si= ngle struct. i.e. feature-flags are just another type of capability, but a device capabi= lity rather than an op capability. =20 e.g. /** Structure used to capture a capability of a crypto device */ struct rte_cryptodev_capabilities { enum rte_crypto_capability_type cap_type; /* this was enum rte_crypto_op_t= ype op; */ /**< Capability_type: device_cap/sym_op_cap/asym_op_cap */ union { struct rte_cryptodev_symmetric_capability sym_op_cap; /**< Symmetric operation capability parameters */ struct rte_cryptodev_device_capability device_cap; /**< Device capability */ }; }; struct rte_cryptodev_device_capability { enum rte_crypto_device_feature feature_id;=20 /**< same elements as the feature bitmask, but an enum, i.e. hw_accelerati= on, cpu_aesni, cpu_avx, sym_sy, asym_cy */ const char rte_crypto_device_feature feature_name[MAX_FEATURE_NAME_LEN]; }; //snip//