Re: [dpdk-dev] [PATCH 03/10] crypto/caam_jr: add HW config for job rings

[Gagandeep Singh <G.Singh@nxp.com>]

Hi Akhil,

> -----Original Message-----
> From: Akhil Goyal
> Sent: Tuesday, September 18, 2018 7:07 PM
> To: Gagandeep Singh <G.Singh@nxp.com>; dev@dpdk.org
> Cc: Hemant Agrawal <hemant.agrawal@nxp.com>
> Subject: Re: [dpdk-dev] [PATCH 03/10] crypto/caam_jr: add HW config for job
> rings
> 
> Hi Gagan,
> 
> On 9/13/2018 11:38 AM, Gagandeep Singh wrote:
> > From: Hemant Agrawal <hemant.agrawal@nxp.com>
> >
> > Signed-off-by: Gagandeep Singh <g.singh@nxp.com>
> > Signed-off-by: Hemant Agrawal <hemant.agrawal@nxp.com>
> > ---
> >   drivers/crypto/caam_jr/Makefile              |   6 +
> >   drivers/crypto/caam_jr/caam_jr.c             | 329 +++++++++++-
> >   drivers/crypto/caam_jr/caam_jr_config.h      | 207 ++++++++
> >   drivers/crypto/caam_jr/caam_jr_hw.c          | 365 ++++++++++++++
> >   drivers/crypto/caam_jr/caam_jr_hw_specific.h | 503 +++++++++++++++++++
> >   drivers/crypto/caam_jr/caam_jr_pvt.h         | 285 +++++++++++
> >   drivers/crypto/caam_jr/caam_jr_uio.c         | 491 ++++++++++++++++++
> >   drivers/crypto/caam_jr/meson.build           |   5 +-
> >   8 files changed, 2188 insertions(+), 3 deletions(-)
> >   create mode 100644 drivers/crypto/caam_jr/caam_jr_config.h
> >   create mode 100644 drivers/crypto/caam_jr/caam_jr_hw.c
> >   create mode 100644 drivers/crypto/caam_jr/caam_jr_hw_specific.h
> >   create mode 100644 drivers/crypto/caam_jr/caam_jr_pvt.h
> >   create mode 100644 drivers/crypto/caam_jr/caam_jr_uio.c
> >
> > diff --git a/drivers/crypto/caam_jr/Makefile
> > b/drivers/crypto/caam_jr/Makefile index 46d752af7..8b863b4af 100644
> > --- a/drivers/crypto/caam_jr/Makefile
> > +++ b/drivers/crypto/caam_jr/Makefile
> > @@ -19,7 +19,10 @@ CFLAGS += -O3
> >   CFLAGS += $(WERROR_FLAGS)
> >   endif
> >
> > +CFLAGS += -I$(RTE_SDK)/drivers/bus/dpaa/include
> >   CFLAGS += -I$(RTE_SDK)/drivers/crypto/caam_jr
> > +#sharing the hw flib headers from dpaa2_sec pmd CFLAGS +=
> > +-I$(RTE_SDK)/drivers/crypto/dpaa2_sec/
> >   CFLAGS += -I$(RTE_SDK)/lib/librte_eal/common/include
> >   CFLAGS += -I$(RTE_SDK)/lib/librte_eal/linuxapp/eal
> >
> > @@ -30,11 +33,14 @@ EXPORT_MAP := rte_pmd_caam_jr_version.map
> >   LIBABIVER := 1
> >
> >   # library source files
> > +SRCS-$(CONFIG_RTE_LIBRTE_PMD_CAAM_JR) += caam_jr_hw.c
> > +SRCS-$(CONFIG_RTE_LIBRTE_PMD_CAAM_JR) += caam_jr_uio.c
> >   SRCS-$(CONFIG_RTE_LIBRTE_PMD_CAAM_JR) += caam_jr.c
> >   # library dependencies
> >
> >   LDLIBS += -lrte_eal -lrte_mbuf -lrte_mempool -lrte_ring
> >   LDLIBS += -lrte_cryptodev
> > +LDLIBS += -lrte_bus_dpaa
> >   LDLIBS += -lrte_bus_vdev
> >
> >   include $(RTE_SDK)/mk/rte.lib.mk
> > diff --git a/drivers/crypto/caam_jr/caam_jr.c
> > b/drivers/crypto/caam_jr/caam_jr.c
> > index 68779cba5..9d5f5b79b 100644
> > --- a/drivers/crypto/caam_jr/caam_jr.c
> > +++ b/drivers/crypto/caam_jr/caam_jr.c
> > @@ -16,13 +16,146 @@
> >   #include <rte_malloc.h>
> >   #include <rte_security_driver.h>
> >   #include <rte_hexdump.h>
> > +#include <caam_jr_config.h>
> Please give one line break between the rte_ includes and driver specific includes
ok
> >
> > +/* RTA header files */
> > +#include <hw/desc/common.h>
> > +#include <of.h>
> > +#include <caam_jr_hw_specific.h>
> > +#include <caam_jr_pvt.h>
> >   #include <caam_jr_log.h>
> >
> >   #define CRYPTODEV_NAME_CAAM_JR_PMD	crypto_caam_jr
> >   static uint8_t cryptodev_driver_id;
> >   int caam_jr_logtype;
> >
> > +enum rta_sec_era rta_sec_era;
> > +
> > +/* Lists the states possible for the SEC user space driver. */ enum
> > +sec_driver_state_e {
> > +	SEC_DRIVER_STATE_IDLE,		/* Driver not initialized */
> > +	SEC_DRIVER_STATE_STARTED,	/* Driver initialized and can be used*/
> > +	SEC_DRIVER_STATE_RELEASE,	/* Driver release is in progress */
> > +};
> > +
> > +/* Job rings used for communication with SEC HW */ static struct
> > +sec_job_ring_t g_job_rings[MAX_SEC_JOB_RINGS];
> > +
> > +/* The current state of SEC user space driver */ static enum
> > +sec_driver_state_e g_driver_state = SEC_DRIVER_STATE_IDLE;
> > +
> > +/* The number of job rings used by SEC user space driver */ static
> > +int g_job_rings_no; static int g_job_rings_max;
> > +
> > +/* @brief Poll the HW for already processed jobs in the JR
> > + * and silently discard the available jobs or notify them to UA
> > + * with indicated error code.
> > + *
> > + * @param [in,out]  job_ring        The job ring to poll.
> > + * @param [in]  do_notify           Can be #TRUE or #FALSE. Indicates if
> > + *				    descriptors are to be discarded
> > + *                                  or notified to UA with given error_code.
> > + * @param [out] notified_descs    Number of notified descriptors. Can be
> NULL
> > + *					if do_notify is #FALSE
> > + */
> > +static void hw_flush_job_ring(struct sec_job_ring_t *job_ring,
> > +			      uint32_t do_notify,
> > +			      uint32_t *notified_descs)
> static void should be in a separate line.. Please check in rest of the code as well.
I will handled all of them in next version.
> > +{
> > +	int32_t jobs_no_to_discard = 0;
> > +	int32_t discarded_descs_no = 0;
> > +
> > +	CAAM_JR_DEBUG("Jr[%p] pi[%d] ci[%d].Flushing jr notify desc=[%d]",
> > +		job_ring, job_ring->pidx, job_ring->cidx, do_notify);
> > +
> > +	jobs_no_to_discard = hw_get_no_finished_jobs(job_ring);
> > +
> > +	/* Discard all jobs */
> > +	CAAM_JR_DEBUG("Jr[%p] pi[%d] ci[%d].Discarding %d descs",
> > +		  job_ring, job_ring->pidx, job_ring->cidx,
> > +		  jobs_no_to_discard);
> > +
> > +	while (jobs_no_to_discard > discarded_descs_no) {
> > +		/* Get completed descriptor */
> > +#if 0
> > +		/*TODO if we want to do something with desc*/
> > +		/* Since the memory is contigous, then P2V translation is a
> > +		 * mere addition to the base descriptor physical address
> > +		 */
> > +		current_desc = job_ring->output_ring[job_ring->cidx].desc;
> > +#endif
> Please remove dead code.
ok
> > +
> > +		discarded_descs_no++;
> > +		/* Now increment the consumer index for the current job ring,
> > +		 * AFTER saving job in temporary location!
> > +		 * Increment the consumer index for the current job ring
> > +		 */
> > +		job_ring->cidx = SEC_CIRCULAR_COUNTER(job_ring->cidx,
> > +					 SEC_JOB_RING_SIZE);
> > +
> > +		hw_remove_entries(job_ring, 1);
> > +	}
> > +
> > +	if (do_notify == true) {
> > +		ASSERT(notified_descs != NULL);
> > +		*notified_descs = discarded_descs_no;
> > +	}
> > +}
> > +
> > +
> > +/* @brief Flush job rings of any processed descs.
> > + * The processed descs are silently dropped,
> > + * WITHOUT being notified to UA.
> > + */
> > +static void close_job_ring(struct sec_job_ring_t *job_ring) {
> > +	if (job_ring->irq_fd) {
> > +		/* Producer index is frozen. If consumer index is not equal
> > +		 * with producer index, then we have descs to flush.
> > +		 */
> > +		while (job_ring->pidx != job_ring->cidx)
> > +			hw_flush_job_ring(job_ring, false, NULL);
> > +
> > +		/* free the uio job ring */
> > +		free_job_ring(job_ring->irq_fd);
> > +		job_ring->irq_fd = 0;
> > +		caam_jr_dma_free(job_ring->input_ring);
> > +		caam_jr_dma_free(job_ring->output_ring);
> > +
> > +		g_job_rings_no--;
> > +
> > +	}
> > +}
> > +
> > +/** @brief Release the software and hardware resources tied to a job ring.
> > + * @param [in] job_ring The job ring
> > + *
> > + * @retval  0 for success
> > + * @retval  -1 for error
> > + */
> > +static int shutdown_job_ring(struct sec_job_ring_t *job_ring) {
> > +	int ret = 0;
> > +
> > +	ASSERT(job_ring != NULL);
> > +	ret = hw_shutdown_job_ring(job_ring);
> > +	SEC_ASSERT(ret == 0, ret,
> > +		"Failed to shutdown hardware job ring %p",
> > +		job_ring);
> > +
> > +	if (job_ring->coalescing_en)
> > +		hw_job_ring_disable_coalescing(job_ring);
> > +
> > +	if (job_ring->jr_mode != SEC_NOTIFICATION_TYPE_POLL) {
> > +		ret = caam_jr_disable_irqs(job_ring->irq_fd);
> > +		SEC_ASSERT(ret == 0, ret,
> > +		"Failed to disable irqs for job ring %p",
> > +		job_ring);
> > +	}
> > +
> > +	return ret;
> > +}
> >
> >   /*
> >    * @brief Release the resources used by the SEC user space driver.
> > @@ -42,31 +175,195 @@ int caam_jr_logtype;
> >   static int
> >   caam_jr_dev_uninit(struct rte_cryptodev *dev)
> >   {
> > +	struct sec_job_ring_t *internals;
> >
> >   	if (dev == NULL)
> >   		return -ENODEV;
> >
> > +	internals = dev->data->dev_private;
> > +	rte_free(dev->security_ctx);
> > +
> > +
> > +	/* If any descriptors in flight , poll and wait
> > +	 * until all descriptors are received and silently discarded.
> > +	 */
> > +	if (internals) {
> > +		shutdown_job_ring(internals);
> > +		close_job_ring(internals);
> > +		rte_mempool_free(internals->ctx_pool);
> > +	}
> >
> >   	CAAM_JR_INFO("Closing DPAA_SEC device %s", dev->data->name);
> DPAA_SEC??? check rest of the code as well
Message will be updated in next version.
> >
> > +	/* last caam jr instance) */
> > +	if (g_job_rings_no == 0)
> > +		g_driver_state = SEC_DRIVER_STATE_IDLE;
> >
> > -	return 0;
> > +	return SEC_SUCCESS;
> >   }
> >
> > +/* @brief Initialize the software and hardware resources tied to a job ring.
> .. [snip]
> > diff --git a/drivers/crypto/caam_jr/caam_jr_hw_specific.h
> > b/drivers/crypto/caam_jr/caam_jr_hw_specific.h
> > new file mode 100644
> > index 000000000..7c8909d2b
> > --- /dev/null
> > +++ b/drivers/crypto/caam_jr/caam_jr_hw_specific.h
> > @@ -0,0 +1,503 @@
> > +/* SPDX-License-Identifier: BSD-3-Clause
> > + * Copyright 2017 NXP
> > + */
> > +
> > +#ifndef CAAM_JR_HW_SPECIFIC_H
> > +#define CAAM_JR_HW_SPECIFIC_H
> > +
> > +#include <caam_jr_config.h>
> > +
> > +/*
> > + * Offset to the registers of a job ring.
> > + * Is different for each job ring.
> > + */
> > +#define CHAN_BASE(jr)   ((size_t)(jr)->register_base_addr)
> > +
> > +#ifndef unlikely
> > +#define unlikely(x)     __builtin_expect(!!(x), 0)
> > +#endif
> likely/unlikely are already defined in DPDK
Ok, will be removed.
> > +
> .. [snip]
> > +
> > +static inline rte_iova_t
> > +caam_jr_mem_vtop(void *vaddr)
> > +{
> > +	const struct rte_memseg *ms;
> > +
> > +	ms = rte_mem_virt2memseg(vaddr, NULL);
> > +	if (ms)
> > +		return ms->iova + RTE_PTR_DIFF(vaddr, ms->addr);
> > +	return (size_t)NULL;
> > +}
> > +
> > +static inline void *
> > +caam_jr_dma_ptov(rte_iova_t paddr)
> > +{
> > +	return rte_mem_iova2virt(paddr);
> > +}
> > +
> > +/* Virtual to physical address conversion */ static inline rte_iova_t
> > +caam_jr_dma_vtop(void *ptr) {
> > +	//return rte_malloc_virt2iova(ptr);
> remove this comment.
ok
> > +	return caam_jr_mem_vtop(ptr);
> > +}
> > +
> >