RE: [EXT] [PATCH v3 2/2] crypto/qat: add cipher-crc offload support

["O'Sullivan, Kevin" <kevin.osullivan@intel.com>]

Hi Akhil

Comments below 

Best regards
Kevin

> -----Original Message-----
> From: Akhil Goyal <gakhil@marvell.com>
> Sent: Thursday 16 March 2023 19:15
> To: O'Sullivan, Kevin <kevin.osullivan@intel.com>; dev@dpdk.org
> Cc: Ji, Kai <kai.ji@intel.com>; Coyle, David <david.coyle@intel.com>
> Subject: RE: [EXT] [PATCH v3 2/2] crypto/qat: add cipher-crc offload support
> 
> > Subject: [EXT] [PATCH v3 2/2] crypto/qat: add cipher-crc offload
> > support
> >
> Update title as
> crypto/qat: support cipher-crc offload
> 
> > This patch adds support to the QAT symmetric crypto PMD for combined
> > cipher-crc offload feature, primarily for DOCSIS, on gen2/gen3/gen4
> > QAT devices.
> >
> > A new parameter called qat_sym_cipher_crc_enable has been added to
> the
> > PMD, which can be set on process start as follows:
> 
> A new devarg called ....

<kos> Sure, I will make this change.  


> 
> >
> > -a <qat pci bdf>,qat_sym_cipher_crc_enable=1
> >
> > When enabled, a capability check for the combined cipher-crc offload
> > feature is triggered to the QAT firmware during queue pair
> > initialization. If supported by the firmware, any subsequent runtime
> > DOCSIS cipher-crc requests handled by the QAT PMD are offloaded to the
> > QAT device by setting up the content descriptor and request
> > accordingly.
> >
> > If the combined DOCSIS cipher-crc feature is not supported by the
> > firmware, the CRC continues to be calculated within the PMD, with just
> > the cipher portion of the request being offloaded to the QAT device.
> >
> > Signed-off-by: Kevin O'Sullivan <kevin.osullivan@intel.com>
> > Signed-off-by: David Coyle <david.coyle@intel.com>
> > ---
> > v3: updated the file qat.rst with details of new configuration
> > ---
> >  doc/guides/cryptodevs/qat.rst                |  23 +++
> >  drivers/common/qat/qat_device.c              |  12 +-
> >  drivers/common/qat/qat_device.h              |   3 +-
> >  drivers/common/qat/qat_qp.c                  | 157 +++++++++++++++
> >  drivers/common/qat/qat_qp.h                  |   5 +
> >  drivers/crypto/qat/dev/qat_crypto_pmd_gen2.c |   2 +-
> >  drivers/crypto/qat/dev/qat_crypto_pmd_gens.h |  24 ++-
> >  drivers/crypto/qat/dev/qat_sym_pmd_gen1.c    |   4 +
> >  drivers/crypto/qat/qat_crypto.c              |  22 ++-
> >  drivers/crypto/qat/qat_crypto.h              |   1 +
> >  drivers/crypto/qat/qat_sym.c                 |   4 +
> >  drivers/crypto/qat/qat_sym.h                 |   7 +-
> >  drivers/crypto/qat/qat_sym_session.c         | 196 ++++++++++++++++++-
> >  drivers/crypto/qat/qat_sym_session.h         |  21 +-
> >  14 files changed, 465 insertions(+), 16 deletions(-)
> >
> > diff --git a/doc/guides/cryptodevs/qat.rst
> > b/doc/guides/cryptodevs/qat.rst index ef754106a8..32e0d8a562 100644
> > --- a/doc/guides/cryptodevs/qat.rst
> > +++ b/doc/guides/cryptodevs/qat.rst
> > @@ -294,6 +294,29 @@ by comma. When the same parameter is used
> more
> > than once first occurrence of the  is used.
> >  Maximum threshold that can be set is 32.
> >
> > +
> > +Running QAT PMD with Cipher-CRC offload feature
> > +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > +
> > +Support has been added to the QAT symmetric crypto PMD for combined
> > Cipher-CRC offload,
> > +primarily for the Crypto-CRC DOCSIS security protocol, on
> > +GEN2/GEN3/GEN4
> > QAT devices.
> > +
> > +The following parameter enables a Cipher-CRC offload capability check
> > +to
> > determine
> > +if the feature is supported on the QAT device.
> > +
> > +- qat_sym_cipher_crc_enable
> 
> Use the word devarg to make it uniform across DPDK.

<kos>  Sure, I will make this change.

> 
> 
> 
> > +
> > +When enabled, a capability check for the combined Cipher-CRC offload
> > +feature
> > is triggered
> > +to the QAT firmware during queue pair initialization. If supported by
> > +the
> > firmware,
> > +any subsequent runtime Crypto-CRC DOCSIS security protocol requests
> > +handled
> > by the QAT PMD
> > +are offloaded to the QAT device by setting up the content descriptor
> > +and
> > request accordingly.
> > +If not supported, the CRC is calculated by the QAT PMD using the NET CRC
> API.
> > +
> > +To use this feature the user must set the parameter on process start
> > +as a device
> > additional parameter::
> > +
> > + -a 03:01.1,qat_sym_cipher_crc_enable=1
> > +
> > +
> >  Running QAT PMD with Intel IPSEC MB library for symmetric precomputes
> > function
> >
> >
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> ~~~~~~~
> > ~~~~~~~~~~~~~
> >
> > diff --git a/drivers/common/qat/qat_device.c
> > b/drivers/common/qat/qat_device.c index 8bce2ac073..308c59c39f 100644
> > --- a/drivers/common/qat/qat_device.c
> > +++ b/drivers/common/qat/qat_device.c
> > @@ -149,7 +149,16 @@ qat_dev_parse_cmd(const char *str, struct
> > qat_dev_cmd_param
> >  			} else {
> >  				memcpy(value_str, arg2, iter);
> >  				value = strtol(value_str, NULL, 10);
> > -				if (value > MAX_QP_THRESHOLD_SIZE) {
> > +				if (strcmp(param,
> > +					 SYM_CIPHER_CRC_ENABLE_NAME)
> ==
> > 0) {
> > +					if (value < 0 || value > 1) {
> > +						QAT_LOG(DEBUG, "The value
> > for"
> > +						"
> qat_sym_cipher_crc_enable"
> > +						" should be set to 0 or 1,"
> > +						" setting to 0");
> 
> Do not split printable strings across multiple lines even if it cross max limit.
> Fix this across the patch.
> Moreover max limit is also increased from 80 -> 100


<kos>  Ok, thanks for the info,  I will move all the parts of string to same line .


> 
> 
> > +						value = 0;
> > +					}
> > +				} else if (value > MAX_QP_THRESHOLD_SIZE)
> {
> >  					QAT_LOG(DEBUG, "Exceeded max
> size of"
> >  						" threshold, setting to %d",
> >  						MAX_QP_THRESHOLD_SIZE);
> > @@ -369,6 +378,7 @@ static int qat_pci_probe(struct rte_pci_driver
> > *pci_drv __rte_unused,
> >  			{ SYM_ENQ_THRESHOLD_NAME, 0 },
> >  			{ ASYM_ENQ_THRESHOLD_NAME, 0 },
> >  			{ COMP_ENQ_THRESHOLD_NAME, 0 },
> > +			{ SYM_CIPHER_CRC_ENABLE_NAME, 0 },
> >  			[QAT_CMD_SLICE_MAP_POS] = {
> > QAT_CMD_SLICE_MAP, 0},
> >  			{ NULL, 0 },
> >  	};
> > diff --git a/drivers/common/qat/qat_device.h
> > b/drivers/common/qat/qat_device.h index bc3da04238..4188474dde
> 100644
> > --- a/drivers/common/qat/qat_device.h
> > +++ b/drivers/common/qat/qat_device.h
> > @@ -21,8 +21,9 @@
> >  #define SYM_ENQ_THRESHOLD_NAME "qat_sym_enq_threshold"
> >  #define ASYM_ENQ_THRESHOLD_NAME "qat_asym_enq_threshold"
> >  #define COMP_ENQ_THRESHOLD_NAME "qat_comp_enq_threshold"
> > +#define SYM_CIPHER_CRC_ENABLE_NAME
> "qat_sym_cipher_crc_enable"
> >  #define QAT_CMD_SLICE_MAP "qat_cmd_slice_disable"
> > -#define QAT_CMD_SLICE_MAP_POS	4
> > +#define QAT_CMD_SLICE_MAP_POS	5
> >  #define MAX_QP_THRESHOLD_SIZE	32
> >
> >  /**
> > diff --git a/drivers/common/qat/qat_qp.c b/drivers/common/qat/qat_qp.c
> > index 9cbd19a481..1ce89c265f 100644
> > --- a/drivers/common/qat/qat_qp.c
> > +++ b/drivers/common/qat/qat_qp.c
> > @@ -11,6 +11,9 @@
> >  #include <bus_pci_driver.h>
> >  #include <rte_atomic.h>
> >  #include <rte_prefetch.h>
> > +#ifdef RTE_LIB_SECURITY
> > +#include <rte_ether.h>
> > +#endif
> >
> >  #include "qat_logs.h"
> >  #include "qat_device.h"
> > @@ -957,6 +960,160 @@ qat_cq_get_fw_version(struct qat_qp *qp)
> >  	return -EINVAL;
> >  }
> >
> > +#ifdef BUILD_QAT_SYM
> 
> Where is this defined? Even no documentation about when to
> enable/disable it.


<kos> This is an existing cflag set in the meson.build to compile the QAT code for symmetric sessions. I have used
 this existing ifdef around my code also as it is only applicable for symmetric sessions. Extract from meson.build below

if qat_crypto
    foreach f: ['qat_sym.c', 'qat_sym_session.c',
            'qat_asym.c', 'qat_crypto.c',
            'dev/qat_sym_pmd_gen1.c',
            'dev/qat_asym_pmd_gen1.c',
            'dev/qat_crypto_pmd_gen2.c',
            'dev/qat_crypto_pmd_gen3.c',
            'dev/qat_crypto_pmd_gen4.c',
        ]
        sources += files(join_paths(qat_crypto_relpath, f))
    endforeach
    deps += ['security']
    ext_deps += libcrypto
    cflags += ['-DBUILD_QAT_SYM', '-DBUILD_QAT_ASYM'] endif


> 
> 
> > +/* Sends an LA bulk req message to determine if a QAT device supports
> > +Cipher-
> > CRC
> > + * offload. This assumes that there are no inflight messages, i.e.
> > +assumes
> > + * there's space  on the qp, one message is sent and only one
> > +response
> > + * collected. The status bit of the response and returned data are
> checked.
> > + * Returns:
> > + *     1 if status bit indicates success and returned data matches expected
> > + *     data (i.e. Cipher-CRC supported)
> > + *     0 if status bit indicates error or returned data does not match
> expected
> > + *     data (i.e. Cipher-CRC not supported)
> > + *     Negative error code in case of error
> > + */
> > +int
> > +qat_cq_get_fw_cipher_crc_cap(struct qat_qp *qp) {
> > +	struct qat_queue *queue = &(qp->tx_q);
> > +	uint8_t *base_addr = (uint8_t *)queue->base_addr;
> > +	struct icp_qat_fw_la_bulk_req cipher_crc_cap_msg = {{0}};
> > +	struct icp_qat_fw_comn_resp response = {{0}};
> > +	struct icp_qat_fw_la_cipher_req_params *cipher_param;
> > +	struct icp_qat_fw_la_auth_req_params *auth_param;
> > +	struct qat_sym_session *session;
> > +	phys_addr_t phy_src_addr;
> > +	uint64_t *src_data_addr;
> > +	int ret;
> > +	uint8_t cipher_offset = 18;
> > +	uint8_t crc_offset = 6;
> > +	uint8_t ciphertext[34] = {
> > +		/* Outer protocol header */
> > +		0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> > +		/* Ethernet frame */
> > +		0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x06, 0x05,
> > +		0x04, 0x03, 0x02, 0x01, 0xD6, 0xE2, 0x70, 0x5C,
> > +		0xE6, 0x4D, 0xCC, 0x8C, 0x47, 0xB7, 0x09, 0xD6,
> > +		/* CRC */
> > +		0x54, 0x85, 0xF8, 0x32
> > +	};
> > +	uint8_t plaintext[34] = {
> > +		/* Outer protocol header */
> > +		0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> > +		/* Ethernet frame */
> > +		0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x06, 0x05,
> > +		0x04, 0x03, 0x02, 0x01, 0x08, 0x00, 0xAA, 0xAA,
> > +		0xAA, 0xAA, 0xAA, 0xAA, 0xAA, 0xAA, 0xAA, 0xAA,
> > +		/* CRC */
> > +		0xFF, 0xFF, 0xFF, 0xFF
> > +	};
> > +	uint8_t key[16] = {
> > +		0x00, 0x00, 0x00, 0x00, 0xAA, 0xBB, 0xCC, 0xDD,
> > +		0xEE, 0xFF, 0x00, 0x11, 0x22, 0x33, 0x44, 0x55
> > +	};
> > +	uint8_t iv[16] = {
> > +		0x11, 0x11, 0x11, 0x11, 0x11, 0x11, 0x11, 0x11,
> > +		0x11, 0x11, 0x11, 0x11, 0x11, 0x11, 0x11, 0x11
> > +	};
> 
> Is it not better to define them as macros?

<kos> The arrays are only used locally by this function so that is why I added them here 
           instead of using macros. Do you see merit in adding them as macros?


> 
> > +
> > +	session = rte_zmalloc(NULL, sizeof(struct qat_sym_session), 0);
> > +	if (session == NULL)
> > +		return -EINVAL;
> > +
> > +	/* Verify the session physical address is known */
> > +	rte_iova_t session_paddr = rte_mem_virt2iova(session);
> > +	if (session_paddr == 0 || session_paddr == RTE_BAD_IOVA) {
> > +		QAT_LOG(ERR, "Session physical address unknown.");
> > +		return -EINVAL;
> > +	}
> > +
> > +	/* Prepare the LA bulk request */
> > +	ret = qat_cipher_crc_cap_msg_sess_prepare(session,
> > +						  session_paddr,
> > +						  key,
> > +						  sizeof(key),
> > +						  qp->qat_dev_gen);
> > +	if (ret < 0) {
> > +		rte_free(session);
> > +		/* Returning 0 here to allow qp setup to continue, but
> > +		 * indicate that Cipher-CRC offload is not supported on the
> > +		 * device
> > +		 */
> > +		return 0;
> > +	}
> > +
> > +	cipher_crc_cap_msg = session->fw_req;
> > +
> > +	src_data_addr = rte_zmalloc(NULL, sizeof(plaintext), 0);
> > +	if (src_data_addr == NULL) {
> > +		rte_free(session);
> > +		return -EINVAL;
> > +	}
> > +
> > +	rte_memcpy(src_data_addr, plaintext, sizeof(plaintext));
> > +
> > +	phy_src_addr = rte_mem_virt2iova(src_data_addr);
> > +	if (phy_src_addr == 0 || phy_src_addr == RTE_BAD_IOVA) {
> > +		QAT_LOG(ERR, "Source physical address unknown.");
> > +		return -EINVAL;
> > +	}
> > +
> > +	cipher_crc_cap_msg.comn_mid.src_data_addr = phy_src_addr;
> > +	cipher_crc_cap_msg.comn_mid.src_length = sizeof(plaintext);
> > +	cipher_crc_cap_msg.comn_mid.dest_data_addr = phy_src_addr;
> > +	cipher_crc_cap_msg.comn_mid.dst_length = sizeof(plaintext);
> > +
> > +	cipher_param = (void *)&cipher_crc_cap_msg.serv_specif_rqpars;
> > +	auth_param = (void *)((uint8_t *)cipher_param +
> > +
> 	ICP_QAT_FW_HASH_REQUEST_PARAMETERS_OFFSET);
> > +
> > +	rte_memcpy(cipher_param->u.cipher_IV_array, iv, sizeof(iv));
> > +
> > +	cipher_param->cipher_offset = cipher_offset;
> > +	cipher_param->cipher_length = sizeof(plaintext) - cipher_offset;
> > +	auth_param->auth_off = crc_offset;
> > +	auth_param->auth_len = sizeof(plaintext) -
> > +				crc_offset -
> > +				RTE_ETHER_CRC_LEN;
> > +
> > +	ICP_QAT_FW_LA_DIGEST_IN_BUFFER_SET(
> > +			cipher_crc_cap_msg.comn_hdr.serv_specif_flags,
> > +			ICP_QAT_FW_LA_DIGEST_IN_BUFFER);
> > +
> > +#if RTE_LOG_DP_LEVEL >= RTE_LOG_DEBUG
> > +	QAT_DP_HEXDUMP_LOG(DEBUG, "LA Bulk request",
> > &cipher_crc_cap_msg,
> > +			sizeof(cipher_crc_cap_msg));
> > +#endif
> > +
> > +	/* Send the cipher_crc_cap_msg request */
> > +	memcpy(base_addr + queue->tail,
> > +	       &cipher_crc_cap_msg,
> > +	       sizeof(cipher_crc_cap_msg));
> > +	queue->tail = adf_modulo(queue->tail + queue->msg_size,
> > +			queue->modulo_mask);
> > +	txq_write_tail(qp->qat_dev_gen, qp, queue);
> > +
> > +	/* Check for response and verify data is same as ciphertext */
> > +	if (qat_cq_dequeue_response(qp, &response)) { #if
> RTE_LOG_DP_LEVEL
> > +>= RTE_LOG_DEBUG
> > +		QAT_DP_HEXDUMP_LOG(DEBUG, "LA response:",
> &response,
> > +				sizeof(response));
> > +#endif
> > +
> > +		if (memcmp(src_data_addr, ciphertext, sizeof(ciphertext)) !=
> 0)
> > +			ret = 0; /* Cipher-CRC offload not supported */
> > +		else
> > +			ret = 1;
> > +	} else {
> > +		ret = -EINVAL;
> > +	}
> > +
> > +	rte_free(src_data_addr);
> > +	rte_free(session);
> > +	return ret;
> > +}
> > +#endif
> > +
> >  __rte_weak int
> >  qat_comp_process_response(void **op __rte_unused, uint8_t *resp
> > __rte_unused,

--------------------------------------------------------------
Intel Research and Development Ireland Limited
Registered in Ireland
Registered Office: Collinstown Industrial Park, Leixlip, County Kildare
Registered Number: 308263


This e-mail and any attachments may contain confidential material for the sole
use of the intended recipient(s). Any review or distribution by others is
strictly prohibited. If you are not the intended recipient, please contact the
sender and delete all copies.