From: "Trahe, Fiona" <fiona.trahe@intel.com>
To: "Doherty, Declan" <declan.doherty@intel.com>,
"dev@dpdk.org" <dev@dpdk.org>
Subject: Re: [dpdk-dev] [PATCH] cryptodev: add capabilities discovery mechanism
Date: Wed, 10 Feb 2016 16:37:34 +0000 [thread overview]
Message-ID: <348A99DA5F5B7549AA880327E580B43588F66996@IRSMSX101.ger.corp.intel.com> (raw)
In-Reply-To: <1454159457-6857-1-git-send-email-declan.doherty@intel.com>
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 mechanism
>
> 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 operations
> and algorithms it supports.
>
> The parameter feature_flags added to rte_cryptodev struct is used to capture
> features such as operations supported (symmetric crypto, operation chaining
> etc) as well parameter such as whether the device is hardware accelerated or
> uses SIMD instructions.
>
> The capabilities parameter allows a PMD to define an array of supported
> operations with any limitation which that implementation may have.
>
> Finally the rte_cryptodev_info struct has been extended to allow retrieval of
> these parameter using the existing rte_cryptodev_info_get() API.
>
> Signed-off-by: Declan Doherty <declan.doherty@intel.com>
There's a lot of good data in the Capabilities struct, but is there a use-case for it? Is it likely an application will call this API and validate everything?
Or is it more likely it would just check for algorithm support and if it then uses an unsupported block_size, key_size, etc it will get an invalid param from the PMD and handle this?
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 have a more complex structure?
e.g. chaining of AES and SHA-HMAC could be supported, but not chaining of snow3g UEA2/UIA2?
But this leads to my overall comment - do we really need this level of detail 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. */
>
> + 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 bitmask and op capabilities struct), I think it's better to roll all into a single struct.
i.e. feature-flags are just another type of capability, but a device capability rather than an op capability.
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_type 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;
/**< same elements as the feature bitmask, but an enum, i.e. hw_acceleration, cpu_aesni, cpu_avx, sym_sy, asym_cy */
const char rte_crypto_device_feature feature_name[MAX_FEATURE_NAME_LEN];
};
//snip//
next prev parent reply other threads:[~2016-02-10 16:37 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-01-30 13:10 Declan Doherty
2016-02-02 17:38 ` De Lara Guarch, Pablo
2016-02-10 16:37 ` Trahe, Fiona [this message]
2016-03-10 17:07 ` [dpdk-dev] [PATCH v2] " Pablo de Lara
2016-03-10 19:54 ` [dpdk-dev] [PATCH v3] " Pablo de Lara
2016-03-10 21:27 ` Trahe, Fiona
2016-03-11 1:20 ` Thomas Monjalon
2016-03-11 1:31 ` De Lara Guarch, Pablo
2016-03-11 1:36 ` [dpdk-dev] [PATCH v4] " Pablo de Lara
2016-03-11 9:35 ` Thomas Monjalon
2016-03-14 8:13 ` Cao, Min
2016-03-14 8:25 ` [dpdk-dev] [PATCH v3] " Cao, Min
2016-03-14 8:26 ` [dpdk-dev] [PATCH v2] " Cao, Min
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=348A99DA5F5B7549AA880327E580B43588F66996@IRSMSX101.ger.corp.intel.com \
--to=fiona.trahe@intel.com \
--cc=declan.doherty@intel.com \
--cc=dev@dpdk.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).