[dpdk-dev,RFC,v4,1/1] lib/compressdev: Adding hash support

  Added hash support in lib compressdev. It's an incremental patch to
compression lib RFC v3 https://dpdk.org/dev/patchwork/patch/32331/

Changes from RFC v3:
- Added hash algo enumeration and associated capability stucture
  and params in xform and rte_comp_op
- Rearranged rte_compresdev_capability structure to have
  separate rte_comp_algo_capability and moved
  algo specific capabilities: window_size, dictionary support,
  and hash as part of it
- Added RTE_COMP_UNSPECIFIED=0 in enum rte_comp_algorithm
- Redefined RTE_COMP_END_OF_CAPABILITIES_LIST to use RTE_COMP_UNSPECIFIED
  to resolve missing-field-initializer compiler warning
- Updated compress/decompress xform to input hash algorithm
  during session init
- Updated struct rte_comp_op to input hash buffer
- Fixed checkpatch reported errors on RFCv3

Every compression algorithm can indicate its capability to
perform alongside hash in its associate rte_comp_algo_capa structure.
If none is supported, can terminate array with
hash_algo = RTE_COMP_HASH_ALGO_UNSPECIFIED.
if supported, application can initialize session with desired
algorithm enumeration in xform structure and pass valid hash buffer
pointer during enqueue_burst().

Signed-off-by: Shally Verma <shally.verma@caviumnetworks.com>
---
 lib/librte_compressdev/rte_comp.h                  | 83 +++++++++++++++++-----
 lib/librte_compressdev/rte_compressdev.c           | 19 +++++
 lib/librte_compressdev/rte_compressdev.h           | 59 ++++++++++++---
 lib/librte_compressdev/rte_compressdev_version.map |  1 +
 4 files changed, 137 insertions(+), 25 deletions(-)
  

Hi Shally,

Sorry about the delay in responding. Comments below.
If you want to push next update to github we can continue dev there.
Regards,
Fiona


> -----Original Message-----
> From: Shally Verma [mailto:shally.verma@caviumnetworks.com]
> Sent: Thursday, February 1, 2018 11:07 AM
> To: Trahe, Fiona <fiona.trahe@intel.com>
> Cc: pathreya@caviumnetworks.com; mchalla@caviumnetworks.com; agupta@caviumnetworks.com;
> dev@dpdk.org; ahmed.mansour@nxp.com
> Subject: [RFC v4 1/1] lib/compressdev: Adding hash support
> 
> Added hash support in lib compressdev. It's an incremental patch to
> compression lib RFC v3 https://dpdk.org/dev/patchwork/patch/32331/
> 
> Changes from RFC v3:
> - Added hash algo enumeration and associated capability stucture
>   and params in xform and rte_comp_op
> - Rearranged rte_compresdev_capability structure to have
>   separate rte_comp_algo_capability and moved
>   algo specific capabilities: window_size, dictionary support,
>   and hash as part of it
> - Added RTE_COMP_UNSPECIFIED=0 in enum rte_comp_algorithm
> - Redefined RTE_COMP_END_OF_CAPABILITIES_LIST to use RTE_COMP_UNSPECIFIED
>   to resolve missing-field-initializer compiler warning
> - Updated compress/decompress xform to input hash algorithm
>   during session init
> - Updated struct rte_comp_op to input hash buffer
> - Fixed checkpatch reported errors on RFCv3
> 
> Every compression algorithm can indicate its capability to
> perform alongside hash in its associate rte_comp_algo_capa structure.
> If none is supported, can terminate array with
> hash_algo = RTE_COMP_HASH_ALGO_UNSPECIFIED.
> if supported, application can initialize session with desired
> algorithm enumeration in xform structure and pass valid hash buffer
> pointer during enqueue_burst().
> 
> Signed-off-by: Shally Verma <shally.verma@caviumnetworks.com>
> ---
>  lib/librte_compressdev/rte_comp.h                  | 83 +++++++++++++++++-----
>  lib/librte_compressdev/rte_compressdev.c           | 19 +++++
>  lib/librte_compressdev/rte_compressdev.h           | 59 ++++++++++++---
>  lib/librte_compressdev/rte_compressdev_version.map |  1 +
>  4 files changed, 137 insertions(+), 25 deletions(-)
> 
> diff --git a/lib/librte_compressdev/rte_comp.h b/lib/librte_compressdev/rte_comp.h
> index ca8cbb4..341f59f 100644
> --- a/lib/librte_compressdev/rte_comp.h
> +++ b/lib/librte_compressdev/rte_comp.h
> @@ -75,10 +75,11 @@ enum rte_comp_op_status {
>  	 */
>  };
> 
> -
>  /** Compression Algorithms */
>  enum rte_comp_algorithm {
> -	RTE_COMP_NULL = 0,
> +	RTE_COMP_UNSPECIFIED = 0,
> +	/** No Compression algo */
> +	RTE_COMP_NULL,
>  	/**< No compression.
>  	 * Pass-through, data is copied unchanged from source buffer to
>  	 * destination buffer.
> @@ -94,6 +95,18 @@ enum rte_comp_algorithm {
>  	RTE_COMP_ALGO_LIST_END
>  };
> 
> +
> +/** Compression hash algorithms */
> +enum rte_comp_hash_algorithm {
> +	RTE_COMP_HASH_ALGO_UNSPECIFIED = 0,
> +	/**< No hash */
> +	RTE_COMP_HASH_ALGO_SHA1,
> +	/**< SHA1 hash algorithm */
> +	RTE_COMP_HASH_ALGO_SHA256,
> +	/**< SHA256 hash algorithm */
> +	RTE_COMP_HASH_ALGO_LIST_END,
> +};
> +
>  /**< Compression Level.
>   * The number is interpreted by each PMD differently. However, lower numbers
>   * give fastest compression, at the expense of compression ratio while
> @@ -154,21 +167,24 @@ enum rte_comp_flush_flag {
>  	RTE_COMP_FLUSH_SYNC,
>  	/**< All data should be flushed to output buffer. Output data can be
>  	 * decompressed. However state and history is not cleared, so future
> -	 * ops may use history from this op */
> +	 * ops may use history from this op
> +	 */
>  	RTE_COMP_FLUSH_FULL,
>  	/**< All data should be flushed to output buffer. Output data can be
>  	 * decompressed. State and history data is cleared, so future
>  	 * ops will be independent of ops processed before this.
>  	 */
>  	RTE_COMP_FLUSH_FINAL
> -	/**< Same as RTE_COMP_FLUSH_FULL but also bfinal bit is set in last block
> +	/**< Same as RTE_COMP_FLUSH_FULL but also bfinal bit is set in
> +	 * last block
>  	 */
>  	 /* TODO:
>  	  * describe flag meanings for decompression.
>  	  * describe behavous in OUT_OF_SPACE case.
>  	  * At least the last flag is specific to deflate algo. Should this be
>  	  * called rte_comp_deflate_flush_flag? And should there be
> -	  * comp_op_deflate_params in the op? */
> +	  * comp_op_deflate_params in the op?
> +	  */
>  };
> 
>  /** Compression transform types */
> @@ -180,17 +196,17 @@ enum rte_comp_xform_type {
>  };
> 
>  enum rte_comp_op_type {
> -    RTE_COMP_OP_STATELESS,
> -    /**< All data to be processed is submitted in the op, no state or history
> -     * from previous ops is used and none will be stored for future ops.
> -     * flush must be set to either FLUSH_FULL or FLUSH_FINAL
> -     */
> -    RTE_COMP_OP_STATEFUL
> -    /**< There may be more data to be processed after this op, it's part of a
> -     * stream of data. State and history from previous ops can be used
> -     * and resulting state and history can be stored for future ops,
> -     * depending on flush_flag.
> -     */
> +	RTE_COMP_OP_STATELESS,
> +	/**< All data to be processed is submitted in the op, no state or
> +	 * history from previous ops is used and none will be stored for
> +	 * future ops.flush must be set to either FLUSH_FULL or FLUSH_FINAL
> +	 */
> +	RTE_COMP_OP_STATEFUL
> +	/**< There may be more data to be processed after this op, it's
> +	 * part of a stream of data. State and history from previous ops
> +	 * can be usedand resulting state and history can be stored for
> +	 * future ops, depending on flush_flag.
> +	 */
>  };
> 
> 
> @@ -207,6 +223,13 @@ struct rte_comp_deflate_params {
>  struct rte_comp_compress_common_params {
>  	enum rte_comp_algorithm algo;
>  	/**< Algorithm to use for compress operation */
> +	enum rte_comp_hash_algorithm hash_algo;
> +	/**< Hash algorithm to be used with compress operation. Hash is always
> +	 * done on plaintext. application can query
> +	 * rte_compressdev_capability_get() and parse hash_capa array per each
> +	 * algorithm to know supported hash algos and associated params until
> +	 * it find an entry with RTE_COMP_HASH_ALGO_UNSPECIFIED
> +	 */
>  	union {
>  		struct rte_comp_deflate_params deflate;
>  		/**< Parameters specific to the deflate algorithm */
> @@ -240,6 +263,13 @@ struct rte_comp_compress_xform {
>  struct rte_comp_decompress_common_params {
>  	enum rte_comp_algorithm algo;
>  	/**< Algorithm to use for decompression */
> +	enum rte_comp_hash_algorithm hash_algo;
> +	/**< Hash algorithm to be used with decompress operation. Hash is always
> +	 * done on plaintext. application can query
> +	 * rte_compressdev_capability_get() and parse hash_capa array per each
> +	 * algorithm to know supported hash algos and associated params until
> +	 * it find an entry with RTE_COMP_HASH_ALGO_UNSPECIFIED
> +	 */
>  	enum rte_comp_checksum_type chksum;
>  	/**< Type of checksum to generate on the decompressed data. */
>  	uint16_t window_size;
> @@ -301,7 +331,7 @@ struct rte_comp_xform {
>  struct rte_comp_op {
> 
>  	enum rte_comp_op_type op_type;
> -	void * stream_private;
> +	void *stream_private;
>  	/* location where PMD maintains stream state
>  	 * only required if op_type is STATEFUL, else should be NULL
>  	 */
> @@ -344,6 +374,25 @@ struct rte_comp_op {
>  		 * decompress direction.
>  		 */
>  	} dst;
> +	struct {
> +		uint8_t *hash_buf;
> +		/**< Pointer to output hash calculated on plaintext if session
> +		 * is initialized with Hash algorithm RTE_COMP_HASH_ALGO_SHA1 /
> +		 * RTE_COMP_HASH_ALGO_SHA256. Buffer would contain valid value
> +		 * only after an op with
> +		 * flush flag = RTE_COMP_FLUSH_FULL/FLUSH_FINAL is processed
> +		 * successfully.
> +		 *
> +		 * Length of buffer should be large enough to accommodate digest
> +		 * produced by specific hash algo. Application can know
> +		 * digest_size via parsing hash capability array in struct
> +		 * rte_compressdev_capabilities
> +		 */
> +		uint32_t offset;
> +		/**< Starting offset for writing hash bytes, specified as
> +		 * number of bytes from start of hash buffer pointer.
> +		 */
[Fiona] Is offset really necessary?
If it is then need to update description of length of hash buffer above to be digest_size + offset.
Also I expect an iova address is necessary for a hw accelerator to produce this hash.

> +	} hash;
>  	enum rte_comp_flush_flag flush_flag;
>  	/**< defines flush characteristics for the output data.
>  	 * Only applicable in compress direction
> diff --git a/lib/librte_compressdev/rte_compressdev.c b/lib/librte_compressdev/rte_compressdev.c
> index 6186ce5..b62b1ce 100644
> --- a/lib/librte_compressdev/rte_compressdev.c
> +++ b/lib/librte_compressdev/rte_compressdev.c
> @@ -113,6 +113,25 @@ struct rte_compressdev_callback {
> 
>  };
> 
> +const struct rte_compressdev_capabilities *
> +rte_compressdev_capability_get(uint8_t dev_id,
> +		const struct rte_compressdev_capability_idx *idx)
> +{
> +	const struct rte_compressdev_capabilities *capability;
> +	struct rte_compressdev_info dev_info;
> +	int i = 0;
> +
> +	memset(&dev_info, 0, sizeof(struct rte_compressdev_info));
> +	rte_compressdev_info_get(dev_id, &dev_info);
> +
> +	while ((capability = &dev_info.capabilities[i++])->algo !=
> +			RTE_COMP_UNSPECIFIED){
> +		if (capability->algo == idx->algo)
> +			return capability;
> +	}
> +
> +	return NULL;
> +}
> 
>  #define param_range_check(x, y) \
>  	(((x < y.min) || (x > y.max)) || \
> diff --git a/lib/librte_compressdev/rte_compressdev.h b/lib/librte_compressdev/rte_compressdev.h
> index 9a86603..7d1b9b1 100644
> --- a/lib/librte_compressdev/rte_compressdev.h
> +++ b/lib/librte_compressdev/rte_compressdev.h
> @@ -125,22 +125,65 @@ struct rte_comp_param_range {
>  	 */
>  };
> 
> +/**
> + * Compression hash algo Capability
> + */
> +struct rte_comp_hash_algo_capability {
> +	enum rte_comp_hash_algorithm hash_alg;
> +	/**< hash algo enumeration */
> +	uint32_t digest_size;
> +	/**< length of digest produced by hash */
[Fiona] op doesn't give an option to specify digest_length.
So if it's a fixed size per hash algo then no need for 
length in capabilities. And so can maybe use a bitmask for hash capabilities.
If a range is needed then this should be a range and add digest_length to op or xform

> +};
> +
> +/**
> + * Compression algo Capability
> + */
> +struct rte_comp_algo_capability {
> +	struct rte_comp_param_range window_size;
> +	/**< window size range in bytes */
> +	int support_dict;
> +	/** Indicate algo support for dictionary load */
[Fiona] I need more info on this - What else is needed on the API to support it?

> +	struct rte_comp_hash_algo_capability *hash_capa;
> +	/** pointer to an array of hash capability struct */
[Fiona] Does the hash capability depend on the compression algorithm?
I'd have thought it's completely independent as it's computed 
on the uncompressed data?
I suppose the same can be said of checksum.
This all seems a bit awkward.
Would something like this be better:

enum rte_comp_capability_type {
  RTE_COMP_CAPA_TYPE_ALGO,
  RTE_COMP_CAPA_TYPE_CHKSUM,
  RTE_COMP_CAPA_TYPE_HASH
}

struct rte_comp_algo_capability {
               enum rte_comp_algorithm algo;
	/** Compression Algorithm */
	uint64_t comp_feature_flags;
	/**< bitmap of flags for compression service features supported with this algo */
	struct rte_comp_param_range window_size;
	/**< window size range in bytes supported with this algo */
               /* int support_dict; */
              /** Indicate algo support for dictionary load */
};

struct rte_compressdev_capabilities {
{
	rte_comp_capability_type capa_type;
	union {
	    struct rte_comp_algo_capability algo_capa,
	    int checksum_capa, /* bitmask of supported checksums  */
	    int hash_capa, /* bitmask of supported hashes - this could be a struct if more data needed */
	}
};

> 
>  /** Structure used to capture a capability of a comp device */
>  struct rte_compressdev_capabilities {
> -	/* TODO */
>  	enum rte_comp_algorithm algo;
> +	/** Compression Algorithm */
> +	struct rte_comp_algo_capability alg_capa;
> +	/**< Compression algo capabilities */
>  	uint64_t comp_feature_flags;
> -	/**< bitmap of flags for compression service features*/
> -	struct rte_comp_param_range window_size;
> -	/**< window size range in bytes */
> +	/**< bitmap of flags for compression service features */
>  };
> 
> +/**  Structure used to index compression device capability array */
> +struct rte_compressdev_capability_idx {
> +	enum rte_comp_algorithm algo;
> +};
> +
> +/**
> + *  Provide device capability for requested algorithm
> + *
> + * @param       dev_id          The identifier of the device.
> + * @param       idx             Index to capability array
> + *
> + * @return
> + *   - Return pointer to capability structure, if exist.
> + *   - Return NULL, if does not exist.
> + */
> +const struct rte_compressdev_capabilities *
> +rte_compressdev_capability_get(uint8_t dev_id,
> +		const struct rte_compressdev_capability_idx *idx);
> +
> 
>  /** Macro used at end of comp PMD list */
>  #define RTE_COMP_END_OF_CAPABILITIES_LIST() \
> -	{ RTE_COMP_ALGO_LIST_END }
> +	{ RTE_COMP_UNSPECIFIED }
> 
> +/** Macro used at end of comp hash capability list */
> +#define RTE_COMP_HASH_END_OF_CAPABILITIES_LIST() \
> +	{ RTE_COMP_HASH_ALG_UNSPECIFIED }
> 
>  /**
>   * compression device supported feature flags
> @@ -833,7 +876,7 @@ struct rte_comp_session *
>  rte_compressdev_queue_pair_detach_session(uint8_t dev_id, uint16_t qp_id,
>  		struct rte_comp_session *session);
>  /**
> - * This should alloc a stream from the device's mempool and initialise it.
> + * This should alloc a stream from the device's mempool and initialise it.
>   * This handle will be passed to the PMD with every op in the stream.
>   *
>   * @param	dev_id		The identifier of the device.
> @@ -850,7 +893,7 @@ struct rte_comp_session *
>   */
>  int
>  rte_comp_stream_create(uint8_t dev_id, struct rte_comp_session *sess,
> -					void ** stream);
> +					void **stream);
> 
>  /**
>   * This should clear the stream and return it to the device's mempool.
> @@ -863,7 +906,7 @@ struct rte_comp_session *
>   * @return
>   */
>  int
> -rte_comp_stream_free(uint8_t dev_id, void * stream);
> +rte_comp_stream_free(uint8_t dev_id, void *stream);
> 
>  /**
>   * Provide driver identifier.
> diff --git a/lib/librte_compressdev/rte_compressdev_version.map
> b/lib/librte_compressdev/rte_compressdev_version.map
> index 665f0bf..3114949 100644
> --- a/lib/librte_compressdev/rte_compressdev_version.map
> +++ b/lib/librte_compressdev/rte_compressdev_version.map
> @@ -5,6 +5,7 @@ EXPERIMENTAL {
>  	rte_compressdev_allocate_driver;
>  	rte_compressdev_callback_register;
>  	rte_compressdev_callback_unregister;
> +	rte_compressdev_capability_get;
>  	rte_compressdev_close;
>  	rte_compressdev_configure;
>  	rte_compressdev_count;
> --
> 1.9.1
  

Hi Fiona 

Thanks for feedback. 
Comments inline. 


>-----Original Message-----
>From: Trahe, Fiona [mailto:fiona.trahe@intel.com]
>Sent: 14 March 2018 18:12
>To: Verma, Shally <Shally.Verma@cavium.com>
>Cc: Athreya, Narayana Prasad <NarayanaPrasad.Athreya@cavium.com>; Challa, Mahipal <Mahipal.Challa@cavium.com>; Gupta,
>Ashish <Ashish.Gupta@cavium.com>; dev@dpdk.org; ahmed.mansour@nxp.com; Trahe, Fiona <fiona.trahe@intel.com>; De Lara
>Guarch, Pablo <pablo.de.lara.guarch@intel.com>; Jain, Deepak K <deepak.k.jain@intel.com>
>Subject: RE: [RFC v4 1/1] lib/compressdev: Adding hash support
>
>Hi Shally,
>
>Sorry about the delay in responding. Comments below.
>If you want to push next update to github we can continue dev there.
>Regards,
>Fiona
>
>
>> -----Original Message-----
>> From: Shally Verma [mailto:shally.verma@caviumnetworks.com]
>> Sent: Thursday, February 1, 2018 11:07 AM
>> To: Trahe, Fiona <fiona.trahe@intel.com>
>> Cc: pathreya@caviumnetworks.com; mchalla@caviumnetworks.com; agupta@caviumnetworks.com;
>> dev@dpdk.org; ahmed.mansour@nxp.com
>> Subject: [RFC v4 1/1] lib/compressdev: Adding hash support
>>
>> Added hash support in lib compressdev. It's an incremental patch to
>> compression lib RFC v3 https://dpdk.org/dev/patchwork/patch/32331/
>>
>> Changes from RFC v3:
>> - Added hash algo enumeration and associated capability stucture
>>   and params in xform and rte_comp_op
>> - Rearranged rte_compresdev_capability structure to have
>>   separate rte_comp_algo_capability and moved
>>   algo specific capabilities: window_size, dictionary support,
>>   and hash as part of it
>> - Added RTE_COMP_UNSPECIFIED=0 in enum rte_comp_algorithm
>> - Redefined RTE_COMP_END_OF_CAPABILITIES_LIST to use RTE_COMP_UNSPECIFIED
>>   to resolve missing-field-initializer compiler warning
>> - Updated compress/decompress xform to input hash algorithm
>>   during session init
>> - Updated struct rte_comp_op to input hash buffer
>> - Fixed checkpatch reported errors on RFCv3
>>
>> Every compression algorithm can indicate its capability to
>> perform alongside hash in its associate rte_comp_algo_capa structure.
>> If none is supported, can terminate array with
>> hash_algo = RTE_COMP_HASH_ALGO_UNSPECIFIED.
>> if supported, application can initialize session with desired
>> algorithm enumeration in xform structure and pass valid hash buffer
>> pointer during enqueue_burst().
>>
>> Signed-off-by: Shally Verma <shally.verma@caviumnetworks.com>
>> ---
>>  lib/librte_compressdev/rte_comp.h                  | 83 +++++++++++++++++-----
>>  lib/librte_compressdev/rte_compressdev.c           | 19 +++++
>>  lib/librte_compressdev/rte_compressdev.h           | 59 ++++++++++++---
>>  lib/librte_compressdev/rte_compressdev_version.map |  1 +
>>  4 files changed, 137 insertions(+), 25 deletions(-)
>>
>> diff --git a/lib/librte_compressdev/rte_comp.h b/lib/librte_compressdev/rte_comp.h
>> index ca8cbb4..341f59f 100644
>> --- a/lib/librte_compressdev/rte_comp.h
>> +++ b/lib/librte_compressdev/rte_comp.h
>> @@ -75,10 +75,11 @@ enum rte_comp_op_status {
>>  	 */
>>  };
>>
>> -
>>  /** Compression Algorithms */
>>  enum rte_comp_algorithm {
>> -	RTE_COMP_NULL = 0,
>> +	RTE_COMP_UNSPECIFIED = 0,
>> +	/** No Compression algo */
>> +	RTE_COMP_NULL,
>>  	/**< No compression.
>>  	 * Pass-through, data is copied unchanged from source buffer to
>>  	 * destination buffer.
>> @@ -94,6 +95,18 @@ enum rte_comp_algorithm {
>>  	RTE_COMP_ALGO_LIST_END
>>  };
>>
>> +
>> +/** Compression hash algorithms */
>> +enum rte_comp_hash_algorithm {
>> +	RTE_COMP_HASH_ALGO_UNSPECIFIED = 0,
>> +	/**< No hash */
>> +	RTE_COMP_HASH_ALGO_SHA1,
>> +	/**< SHA1 hash algorithm */
>> +	RTE_COMP_HASH_ALGO_SHA256,
>> +	/**< SHA256 hash algorithm */
>> +	RTE_COMP_HASH_ALGO_LIST_END,
>> +};
>> +
>>  /**< Compression Level.
>>   * The number is interpreted by each PMD differently. However, lower numbers
>>   * give fastest compression, at the expense of compression ratio while
>> @@ -154,21 +167,24 @@ enum rte_comp_flush_flag {
>>  	RTE_COMP_FLUSH_SYNC,
>>  	/**< All data should be flushed to output buffer. Output data can be
>>  	 * decompressed. However state and history is not cleared, so future
>> -	 * ops may use history from this op */
>> +	 * ops may use history from this op
>> +	 */
>>  	RTE_COMP_FLUSH_FULL,
>>  	/**< All data should be flushed to output buffer. Output data can be
>>  	 * decompressed. State and history data is cleared, so future
>>  	 * ops will be independent of ops processed before this.
>>  	 */
>>  	RTE_COMP_FLUSH_FINAL
>> -	/**< Same as RTE_COMP_FLUSH_FULL but also bfinal bit is set in last block
>> +	/**< Same as RTE_COMP_FLUSH_FULL but also bfinal bit is set in
>> +	 * last block
>>  	 */
>>  	 /* TODO:
>>  	  * describe flag meanings for decompression.
>>  	  * describe behavous in OUT_OF_SPACE case.
>>  	  * At least the last flag is specific to deflate algo. Should this be
>>  	  * called rte_comp_deflate_flush_flag? And should there be
>> -	  * comp_op_deflate_params in the op? */
>> +	  * comp_op_deflate_params in the op?
>> +	  */
>>  };
>>
>>  /** Compression transform types */
>> @@ -180,17 +196,17 @@ enum rte_comp_xform_type {
>>  };
>>
>>  enum rte_comp_op_type {
>> -    RTE_COMP_OP_STATELESS,
>> -    /**< All data to be processed is submitted in the op, no state or history
>> -     * from previous ops is used and none will be stored for future ops.
>> -     * flush must be set to either FLUSH_FULL or FLUSH_FINAL
>> -     */
>> -    RTE_COMP_OP_STATEFUL
>> -    /**< There may be more data to be processed after this op, it's part of a
>> -     * stream of data. State and history from previous ops can be used
>> -     * and resulting state and history can be stored for future ops,
>> -     * depending on flush_flag.
>> -     */
>> +	RTE_COMP_OP_STATELESS,
>> +	/**< All data to be processed is submitted in the op, no state or
>> +	 * history from previous ops is used and none will be stored for
>> +	 * future ops.flush must be set to either FLUSH_FULL or FLUSH_FINAL
>> +	 */
>> +	RTE_COMP_OP_STATEFUL
>> +	/**< There may be more data to be processed after this op, it's
>> +	 * part of a stream of data. State and history from previous ops
>> +	 * can be usedand resulting state and history can be stored for
>> +	 * future ops, depending on flush_flag.
>> +	 */
>>  };
>>
>>
>> @@ -207,6 +223,13 @@ struct rte_comp_deflate_params {
>>  struct rte_comp_compress_common_params {
>>  	enum rte_comp_algorithm algo;
>>  	/**< Algorithm to use for compress operation */
>> +	enum rte_comp_hash_algorithm hash_algo;
>> +	/**< Hash algorithm to be used with compress operation. Hash is always
>> +	 * done on plaintext. application can query
>> +	 * rte_compressdev_capability_get() and parse hash_capa array per each
>> +	 * algorithm to know supported hash algos and associated params until
>> +	 * it find an entry with RTE_COMP_HASH_ALGO_UNSPECIFIED
>> +	 */
>>  	union {
>>  		struct rte_comp_deflate_params deflate;
>>  		/**< Parameters specific to the deflate algorithm */
>> @@ -240,6 +263,13 @@ struct rte_comp_compress_xform {
>>  struct rte_comp_decompress_common_params {
>>  	enum rte_comp_algorithm algo;
>>  	/**< Algorithm to use for decompression */
>> +	enum rte_comp_hash_algorithm hash_algo;
>> +	/**< Hash algorithm to be used with decompress operation. Hash is always
>> +	 * done on plaintext. application can query
>> +	 * rte_compressdev_capability_get() and parse hash_capa array per each
>> +	 * algorithm to know supported hash algos and associated params until
>> +	 * it find an entry with RTE_COMP_HASH_ALGO_UNSPECIFIED
>> +	 */
>>  	enum rte_comp_checksum_type chksum;
>>  	/**< Type of checksum to generate on the decompressed data. */
>>  	uint16_t window_size;
>> @@ -301,7 +331,7 @@ struct rte_comp_xform {
>>  struct rte_comp_op {
>>
>>  	enum rte_comp_op_type op_type;
>> -	void * stream_private;
>> +	void *stream_private;
>>  	/* location where PMD maintains stream state
>>  	 * only required if op_type is STATEFUL, else should be NULL
>>  	 */
>> @@ -344,6 +374,25 @@ struct rte_comp_op {
>>  		 * decompress direction.
>>  		 */
>>  	} dst;
>> +	struct {
>> +		uint8_t *hash_buf;
>> +		/**< Pointer to output hash calculated on plaintext if session
>> +		 * is initialized with Hash algorithm RTE_COMP_HASH_ALGO_SHA1 /
>> +		 * RTE_COMP_HASH_ALGO_SHA256. Buffer would contain valid value
>> +		 * only after an op with
>> +		 * flush flag = RTE_COMP_FLUSH_FULL/FLUSH_FINAL is processed
>> +		 * successfully.
>> +		 *
>> +		 * Length of buffer should be large enough to accommodate digest
>> +		 * produced by specific hash algo. Application can know
>> +		 * digest_size via parsing hash capability array in struct
>> +		 * rte_compressdev_capabilities
>> +		 */
>> +		uint32_t offset;
>> +		/**< Starting offset for writing hash bytes, specified as
>> +		 * number of bytes from start of hash buffer pointer.
>> +		 */
>[Fiona] Is offset really necessary?
>If it is then need to update description of length of hash buffer above to be digest_size + offset.
>Also I expect an iova address is necessary for a hw accelerator to produce this hash.
[Shally] offset isn't necessary so I will make It simpler to just have pointer and corresponding iova.
Only requirement is buffer should be contiguous.

>
>> +	} hash;
>>  	enum rte_comp_flush_flag flush_flag;
>>  	/**< defines flush characteristics for the output data.
>>  	 * Only applicable in compress direction
>> diff --git a/lib/librte_compressdev/rte_compressdev.c b/lib/librte_compressdev/rte_compressdev.c
>> index 6186ce5..b62b1ce 100644
>> --- a/lib/librte_compressdev/rte_compressdev.c
>> +++ b/lib/librte_compressdev/rte_compressdev.c
>> @@ -113,6 +113,25 @@ struct rte_compressdev_callback {
>>
>>  };
>>
>> +const struct rte_compressdev_capabilities *
>> +rte_compressdev_capability_get(uint8_t dev_id,
>> +		const struct rte_compressdev_capability_idx *idx)
>> +{
>> +	const struct rte_compressdev_capabilities *capability;
>> +	struct rte_compressdev_info dev_info;
>> +	int i = 0;
>> +
>> +	memset(&dev_info, 0, sizeof(struct rte_compressdev_info));
>> +	rte_compressdev_info_get(dev_id, &dev_info);
>> +
>> +	while ((capability = &dev_info.capabilities[i++])->algo !=
>> +			RTE_COMP_UNSPECIFIED){
>> +		if (capability->algo == idx->algo)
>> +			return capability;
>> +	}
>> +
>> +	return NULL;
>> +}
>>
>>  #define param_range_check(x, y) \
>>  	(((x < y.min) || (x > y.max)) || \
>> diff --git a/lib/librte_compressdev/rte_compressdev.h b/lib/librte_compressdev/rte_compressdev.h
>> index 9a86603..7d1b9b1 100644
>> --- a/lib/librte_compressdev/rte_compressdev.h
>> +++ b/lib/librte_compressdev/rte_compressdev.h
>> @@ -125,22 +125,65 @@ struct rte_comp_param_range {
>>  	 */
>>  };
>>
>> +/**
>> + * Compression hash algo Capability
>> + */
>> +struct rte_comp_hash_algo_capability {
>> +	enum rte_comp_hash_algorithm hash_alg;
>> +	/**< hash algo enumeration */
>> +	uint32_t digest_size;
>> +	/**< length of digest produced by hash */
>[Fiona] op doesn't give an option to specify digest_length.
>So if it's a fixed size per hash algo then no need for
>length in capabilities. And so can maybe use a bitmask for hash capabilities.
>If a range is needed then this should be a range and add digest_length to op or xform
>
[Shally] Range is not needed here as current hash algos support fixed size digest lengths. So, for I will remove digest_size from capability for now and make it a bitmask.
Apps then would be expected to know digest size of algo it want to use.
If, in future, there are more SHA variations or such added to list, then capability structure can be revisited.

>> +};
>> +
>> +/**
>> + * Compression algo Capability
>> + */
>> +struct rte_comp_algo_capability {
>> +	struct rte_comp_param_range window_size;
>> +	/**< window size range in bytes */
>> +	int support_dict;
>> +	/** Indicate algo support for dictionary load */
>[Fiona] I need more info on this - What else is needed on the API to support it?
[Shally] We will need to add an API, say, rte_comp_set_dictionary() to load pre-set dictionary.
If a particular algorithm implementation supports dictionary load, then app can call this API before starting compression.

>
>> +	struct rte_comp_hash_algo_capability *hash_capa;
>> +	/** pointer to an array of hash capability struct */
>[Fiona] Does the hash capability depend on the compression algorithm?
>I'd have thought it's completely independent as it's computed
>on the uncompressed data?
>I suppose the same can be said of checksum.

[Shally] Ok. I miss to set background here. 
Though practically hash can be performed standalone on data but we are proposing it as a feature that can be performed along with compression and so
is the assumption for crc/adler. Thus , we proposed it as part of compression algo capability. Ex. Implementation may support Deflate+sha1 but not lzs+sha1.
Do you want to propose hash/checksum as standalone capability on compression API? shouldn't standalone hash a part of  crypto spec? similar is question 
for checksum because app can use processor optimized crc/adler library func, if required. So, I see standalone hash/checksums as duplicated feature on compression.

I will issue next patch on github, once we this requirement is resolved.

>This all seems a bit awkward.
>Would something like this be better:
>
>enum rte_comp_capability_type {
>  RTE_COMP_CAPA_TYPE_ALGO,
>  RTE_COMP_CAPA_TYPE_CHKSUM,
>  RTE_COMP_CAPA_TYPE_HASH
>}
>
>struct rte_comp_algo_capability {
>               enum rte_comp_algorithm algo;
>	/** Compression Algorithm */
>	uint64_t comp_feature_flags;
>	/**< bitmap of flags for compression service features supported with this algo */
>	struct rte_comp_param_range window_size;
>	/**< window size range in bytes supported with this algo */
>               /* int support_dict; */
>              /** Indicate algo support for dictionary load */
>};
>
>struct rte_compressdev_capabilities {
>{
>	rte_comp_capability_type capa_type;
>	union {
>	    struct rte_comp_algo_capability algo_capa,
>	    int checksum_capa, /* bitmask of supported checksums  */
>	    int hash_capa, /* bitmask of supported hashes - this could be a struct if more data needed */
>	}
>};
>
>>
>>  /** Structure used to capture a capability of a comp device */
>>  struct rte_compressdev_capabilities {
>> -	/* TODO */
>>  	enum rte_comp_algorithm algo;
>> +	/** Compression Algorithm */
>> +	struct rte_comp_algo_capability alg_capa;
>> +	/**< Compression algo capabilities */
>>  	uint64_t comp_feature_flags;
>> -	/**< bitmap of flags for compression service features*/
>> -	struct rte_comp_param_range window_size;
>> -	/**< window size range in bytes */
>> +	/**< bitmap of flags for compression service features */
>>  };
>>
>> +/**  Structure used to index compression device capability array */
>> +struct rte_compressdev_capability_idx {
>> +	enum rte_comp_algorithm algo;
>> +};
>> +
>> +/**
>> + *  Provide device capability for requested algorithm
>> + *
>> + * @param       dev_id          The identifier of the device.
>> + * @param       idx             Index to capability array
>> + *
>> + * @return
>> + *   - Return pointer to capability structure, if exist.
>> + *   - Return NULL, if does not exist.
>> + */
>> +const struct rte_compressdev_capabilities *
>> +rte_compressdev_capability_get(uint8_t dev_id,
>> +		const struct rte_compressdev_capability_idx *idx);
>> +
>>
>>  /** Macro used at end of comp PMD list */
>>  #define RTE_COMP_END_OF_CAPABILITIES_LIST() \
>> -	{ RTE_COMP_ALGO_LIST_END }
>> +	{ RTE_COMP_UNSPECIFIED }
>>
>> +/** Macro used at end of comp hash capability list */
>> +#define RTE_COMP_HASH_END_OF_CAPABILITIES_LIST() \
>> +	{ RTE_COMP_HASH_ALG_UNSPECIFIED }
>>
>>  /**
>>   * compression device supported feature flags
>> @@ -833,7 +876,7 @@ struct rte_comp_session *
>>  rte_compressdev_queue_pair_detach_session(uint8_t dev_id, uint16_t qp_id,
>>  		struct rte_comp_session *session);
>>  /**
>> - * This should alloc a stream from the device's mempool and initialise it.
>> + * This should alloc a stream from the device's mempool and initialise it.
>>   * This handle will be passed to the PMD with every op in the stream.
>>   *
>>   * @param	dev_id		The identifier of the device.
>> @@ -850,7 +893,7 @@ struct rte_comp_session *
>>   */
>>  int
>>  rte_comp_stream_create(uint8_t dev_id, struct rte_comp_session *sess,
>> -					void ** stream);
>> +					void **stream);
>>
>>  /**
>>   * This should clear the stream and return it to the device's mempool.
>> @@ -863,7 +906,7 @@ struct rte_comp_session *
>>   * @return
>>   */
>>  int
>> -rte_comp_stream_free(uint8_t dev_id, void * stream);
>> +rte_comp_stream_free(uint8_t dev_id, void *stream);
>>
>>  /**
>>   * Provide driver identifier.
>> diff --git a/lib/librte_compressdev/rte_compressdev_version.map
>> b/lib/librte_compressdev/rte_compressdev_version.map
>> index 665f0bf..3114949 100644
>> --- a/lib/librte_compressdev/rte_compressdev_version.map
>> +++ b/lib/librte_compressdev/rte_compressdev_version.map
>> @@ -5,6 +5,7 @@ EXPERIMENTAL {
>>  	rte_compressdev_allocate_driver;
>>  	rte_compressdev_callback_register;
>>  	rte_compressdev_callback_unregister;
>> +	rte_compressdev_capability_get;
>>  	rte_compressdev_close;
>>  	rte_compressdev_configure;
>>  	rte_compressdev_count;
>> --
>> 1.9.1
  

Hi Shally,

//snip//;
> >> +	/**< window size range in bytes */
> >> +	int support_dict;
> >> +	/** Indicate algo support for dictionary load */
> >[Fiona] I need more info on this - What else is needed on the API to support it?
> [Shally] We will need to add an API, say, rte_comp_set_dictionary() to load pre-set dictionary.
> If a particular algorithm implementation supports dictionary load, then app can call this API before starting compression.
[Fiona] ok. so maybe submit in a patch separate to hash on github.
With any needed APIs.
 
> 
> >
> >> +	struct rte_comp_hash_algo_capability *hash_capa;
> >> +	/** pointer to an array of hash capability struct */
> >[Fiona] Does the hash capability depend on the compression algorithm?
> >I'd have thought it's completely independent as it's computed
> >on the uncompressed data?
> >I suppose the same can be said of checksum.
> 
> [Shally] Ok. I miss to set background here.
> Though practically hash can be performed standalone on data but we are proposing it as a feature that can
> be performed along with compression and so
> is the assumption for crc/adler. Thus , we proposed it as part of compression algo capability. Ex.
> Implementation may support Deflate+sha1 but not lzs+sha1.
> Do you want to propose hash/checksum as standalone capability on compression API? shouldn't
> standalone hash a part of  crypto spec? similar is question
> for checksum because app can use processor optimized crc/adler library func, if required. So, I see
> standalone hash/checksums as duplicated feature on compression.
> 
> I will issue next patch on github, once we this requirement is resolved.
[Fiona] No, I wasn't suggesting as standalone op - just that I thought it a device had the capability
to do hash it could probably do it regardless of which algo is used. 
But if you think there's a case where a device has deflate+hash capability and not lzs+sha1
then the hash capability can be per-algo. Based on above can it just be added to the 
existing feature-flags bitmask? Probably no need for the separate hash and algo capability
structs you were proposing since all capabilities are per-algo.

> 
> >This all seems a bit awkward.
> >Would something like this be better:
> >
> >enum rte_comp_capability_type {
> >  RTE_COMP_CAPA_TYPE_ALGO,
> >  RTE_COMP_CAPA_TYPE_CHKSUM,
> >  RTE_COMP_CAPA_TYPE_HASH
> >}
> >
> >struct rte_comp_algo_capability {
> >               enum rte_comp_algorithm algo;
> >	/** Compression Algorithm */
> >	uint64_t comp_feature_flags;
> >	/**< bitmap of flags for compression service features supported with this algo */
> >	struct rte_comp_param_range window_size;
> >	/**< window size range in bytes supported with this algo */
> >               /* int support_dict; */
> >              /** Indicate algo support for dictionary load */
> >};
> >
> >struct rte_compressdev_capabilities {
> >{
> >	rte_comp_capability_type capa_type;
> >	union {
> >	    struct rte_comp_algo_capability algo_capa,
> >	    int checksum_capa, /* bitmask of supported checksums  */
> >	    int hash_capa, /* bitmask of supported hashes - this could be a struct if more data needed */
> >	}
> >};
> >
> >>
> >>  /** Structure used to capture a capability of a comp device */
> >>  struct rte_compressdev_capabilities {
> >> -	/* TODO */
> >>  	enum rte_comp_algorithm algo;
> >> +	/** Compression Algorithm */
> >> +	struct rte_comp_algo_capability alg_capa;
> >> +	/**< Compression algo capabilities */
> >>  	uint64_t comp_feature_flags;
> >> -	/**< bitmap of flags for compression service features*/
> >> -	struct rte_comp_param_range window_size;
> >> -	/**< window size range in bytes */
> >> +	/**< bitmap of flags for compression service features */
> >>  };
> >> +
//snip