AMCC Crypto4xx Device Driver v4]

Hi,

This patch add canyonlands support.

Few performance optimizations:
Redesigned the crypto4xx_build_pd(), which now calculate number of scatter and gather descriptors need before taking them. Instead take these descriptors one by one, now we take them together. Doing this way the time needed to be locked is much reduced. This function now supports aad which needed by future release.

Introduced functions to setup the commands registers. This is to reduce code space and increase the likelihood for cache hit.

Eliminate the need for dynamically alloc memory for request context, by cache up per packet 'sa' in pd_uinfo.

Response to previous review:
Removed not needed includes.
Avoid using if else flow as Kim suggested in few places. lineup multiline parameters of functions. removed change log.

We still include crypto/internal/hash.h because we support 'ahash' which need that header file.

We still have the wrapper function for alg_init(), because we will have multiple algorithm files and external define for the struct ...alg[] does not work.

Thanks
James

Signed-off-by James Hsiao <[email protected]>

---

drivers/crypto/amcc/crypto4xx_alg.c new file +0 -0

new file mode 100644
index 0000000..7a693e4

drivers/crypto/amcc/crypto4xx_core.c +25 -0

+		nbytes -= sg->length;
+		sg = sg_next(sg);
+	}
+
+	return sg_nents;
+}
 
+u32 crypto4xx_build_pd(struct crypto_async_request *req,
+			struct crypto4xx_ctx *ctx,
+			struct scatterlist *src,
+			struct scatterlist *dst,
+			unsigned int datalen,
+			struct scatterlist *assoc,
+			u32 aad_len, void *iv, u32 iv_len)
+{
+	struct crypto4xx_device *dev = ctx->dev;
+	dma_addr_t addr, pd_dma, sd_dma, gd_dma;
+	struct dynamic_sa_ctl *sa;
+	struct scatterlist *sg;
+	struct scatterlist *aad;
+	struct ce_gd *gd;
+	struct ce_pd *pd;
+	u32 num_gd, num_sd;
+	u32 fst_gd = 0xffffffff;
+	u32 fst_sd = 0xffffffff;
+	u32 pd_entry;

drivers/crypto/amcc/crypto4xx_core.c +4 -0

+		while (nbytes) {
+			sd_idx = get_next_sd(sd_idx);
+			sd = crypto4xx_get_sdp(dev, &sd_dma, sd_idx);
+			/* setup scatter descriptor */

drivers/crypto/amcc/crypto4xx_core.c +10 -0

+/**
+ * Algorithm Registration Functions
+ */
+static int crypto4xx_alg_init(struct crypto_tfm *tfm)
+{
+	struct crypto_alg    *alg = tfm->__crt_alg;
+	struct crypto4xx_alg *amcc_alg = crypto_alg_to_crypto4xx_alg(alg);
+	struct crypto4xx_ctx *ctx = crypto_tfm_ctx(tfm);
+
+	ctx->dev = amcc_alg->dev;

drivers/crypto/amcc/crypto4xx_core.c +1 -0

+	crypto4xx_free_sa(ctx);

+err_build_gdr:

+extern struct crypto_alg crypto4xx_basic_alg[];

Quoted 8 lines

(quoted diff) +0 -0

b/arch/powerpc/boot/dts/canyonlands.dts index 79fe412..b0f0096 100644

arch/powerpc/boot/dts/canyonlands.dts +0 -0

@@ -116,6 +116,13 @@
 			dcr-reg = <0x010 0x002>;
 		};
 

arch/powerpc/boot/dts/canyonlands.dts line 123

Quoted 5 lines

arch/powerpc/boot/dts/canyonlands.dts +5 -0

+		CRYPTO: crypto@180000 {
+			compatible = "amcc,ppc460ex-crypto", "amcc,ppc4xx-crypto";
+			reg = <4 0x00180000 0x80400>;
+			interrupt-parent = <&UIC0>;
+			interrupts = <0x1d 0x4>;

that's odd, according to the current canyonlands.dts, irq 0x1d is already assigned to UART2 (and the request_irq this driver makes doesn't specify a shared flag).

Quoted 8 lines · Kim Phillips

Quoted 8 lines

(quoted diff) +0 -0

b/arch/powerpc/boot/dts/canyonlands.dts index 79fe412..b0f0096 100644

arch/powerpc/boot/dts/canyonlands.dts +0 -0

@@ -116,6 +116,13 @@
 			dcr-reg = <0x010 0x002>;
 		};
 

Quoted 9 lines

arch/powerpc/boot/dts/canyonlands.dts line 123

Quoted 5 lines

arch/powerpc/boot/dts/canyonlands.dts +5 -0

+		CRYPTO: crypto@180000 {
+			compatible = "amcc,ppc460ex-crypto", "amcc,ppc4xx-crypto";
+			reg = <4 0x00180000 0x80400>;
+			interrupt-parent = <&UIC0>;
+			interrupts = <0x1d 0x4>;

that's odd, according to the current canyonlands.dts, irq 0x1d is already assigned to UART2 (and the request_irq this driver makes doesn't specify a shared flag).

UART2 is using irq 0x1d from UIC1. Using 0x1d/UIC0 is correct for Security EIP-94 according to the latest users manual. No sharing needed.

Without the check, nbytes could become negative. The aead test case with .np will crash(ie. gcm tests), those test have sg->length > nbytes.

About aad_len, we didn't release code that use aad yet. We did test this function with aad_len none zero(the gcm tests).

About the irq_disable or spin_lock.
The driver could be used by a kernel thread and esp4 at same time. As I know process is preemptable. When the driver is used by a process it is possible to be preempted. The hardware require scatter/gather descriptor to be consecutive. So, if the process get a gather descriptor and then it is preempted by another process or esp4 which get a gather descriptor and return to the original process, the origianl process could get a non consecutive gather descriptor.
So, if spin_lock is recommended then I have to use spin_lock_irq_save, which use irq_disable too. Do you think that is acceptable?

drivers/crypto/amcc/crypto4xx_core.c line 825

Quoted 4 lines

drivers/crypto/amcc/crypto4xx_core.c new file +4 -0

+     /* figure how many gd is needed */
+     if (aad_len) {
+             num_gd = get_sg_count(assoc, aad_len) +
+                     get_sg_count(src, datalen);

this is dead code - aad_len is never non-zero - is there some code missing from crypto4xx_alg.c? Also, IIRC, assoc is a superset of src, so I believe something like num_gd = get_sg_count(assoc, aad_len + datalen) would work better - this should also permit removal of the nbytes reached check in [1] in get_sg_count.

Quoted 10 lines

drivers/crypto/amcc/crypto4xx_core.c line 825

Quoted 4 lines

drivers/crypto/amcc/crypto4xx_core.c new file +4 -0

+     /* figure how many gd is needed */
+     if (aad_len) {
+             num_gd = get_sg_count(assoc, aad_len) +
+                     get_sg_count(src, datalen);

this is dead code - aad_len is never non-zero - is there some code missing from crypto4xx_alg.c? Also, IIRC, assoc is a superset of src, so I believe something like num_gd = get_sg_count(assoc, aad_len + datalen) would work better - this should also permit removal of the nbytes reached check in [1] in get_sg_count.

For the get num_gd, assoc and src are two different scatter_lists. One for the packet and one for the aad, so, it makes more sense to me to count them seperately. Yes?

The way to driver pass aad and aad_len is from crypto4xx_build_pd. This aad support will be used in the future. Do you want me to remover from the current release? The reason I have it in, is to avoid too much code change for the future release.