Skip to contentSkip to conversation

arm64: marvell: add cryptographic engine support for 7k/8k

PATCH Superseded Antoine Tenart 5 participants 19 messages Mar 29, 2017 – Apr 18, 2017

Subsystems (4)

v1 of 6 Superseded Latest is v6 →

lore
Share & tools
lore
#0 lore raw

cover arm64: marvell: add cryptographic engine support for 7k/8k

Hi all,

This series adds support for the Inside Secure SafeXcel EIP197 cryptographic engine which can be found on Marvell Armada 7k and 8k boards. A new cryptographic engine driver is added, as well as the relevant device tree definition for the Armada 7040 DB and 8040 DB boards.

This driver needs two firmwares to work correctly. These firmware are usually used for more advanced operations than the ones supported (as of now), but we still need them to pass the data to the internal cryptographic engine.

This series was tested in various ways on both the Armada 7040 DB and the Armada 8040 DB: using the crypto framework self tests, using tcrypt and while performing various transfers with iperf on top of IPsec.

This series is based on top of v4.11-rc1, and is available on a branch (which also contains the PPv2 support for 7k/8k, to ease the process of testing IPsec): https://github.com/atenart/linux v4.11-rc1/7k8k-crypto I can rebase if needed.

Thanks,
Antoine

Antoine Tenart (7):
Documentation/bindings: Document the SafeXel cryptographic engine
driver
crypto: inside-secure: add SafeXcel EIP197 crypto engine driver
MAINTAINERS: add a maintainer for the Inside Secure crypto driver
arm64: marvell: dts: add crypto engine description for 7k/8k
arm64: marvell: dts: enable the crypto engine on the Armada 7040 DB
arm64: marvell: dts: enable the crypto engine on the Armada 8040 DB
arm64: defconfig: enable the Safexcel crypto engine as a module

 .../bindings/crypto/inside-secure-safexcel.txt     |   27 +
 MAINTAINERS                                        |    6 +
 arch/arm64/boot/dts/marvell/armada-7040-db.dts     |    4 +
 arch/arm64/boot/dts/marvell/armada-8040-db.dts     |    4 +
 .../boot/dts/marvell/armada-cp110-master.dtsi      |   15 +
 .../arm64/boot/dts/marvell/armada-cp110-slave.dtsi |   15 +
 arch/arm64/configs/defconfig                       |    1 +
 drivers/crypto/Kconfig                             |   17 +
 drivers/crypto/Makefile                            |    1 +
 drivers/crypto/inside-secure/Makefile              |    2 +
 drivers/crypto/inside-secure/safexcel.c            |  939 ++++++++++++++++++
 drivers/crypto/inside-secure/safexcel.h            |  581 +++++++++++
 drivers/crypto/inside-secure/safexcel_cipher.c     |  544 ++++++++++
 drivers/crypto/inside-secure/safexcel_hash.c       | 1045 ++++++++++++++++++++
 drivers/crypto/inside-secure/safexcel_ring.c       |  173 ++++
 15 files changed, 3374 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/crypto/inside-secure-safexcel.txt
 create mode 100644 drivers/crypto/inside-secure/Makefile
 create mode 100644 drivers/crypto/inside-secure/safexcel.c
 create mode 100644 drivers/crypto/inside-secure/safexcel.h
 create mode 100644 drivers/crypto/inside-secure/safexcel_cipher.c
 create mode 100644 drivers/crypto/inside-secure/safexcel_hash.c
 create mode 100644 drivers/crypto/inside-secure/safexcel_ring.c
Signature · 1 line
2.11.0
#1 lore raw

PATCH 1/7 Documentation/bindings: Document the SafeXel cryptographic engine driver

The Inside Secure Safexcel cryptographic engine is found on some Marvell SoCs (7k/8k). Document the bindings used by its driver.

---
 .../bindings/crypto/inside-secure-safexcel.txt     | 27 ++++++++++++++++++++++
 1 file changed, 27 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/crypto/inside-secure-safexcel.txt
Documentation/devicetree/bindings/crypto/inside-secure-safexcel.txt new file +27 -0
new file mode 100644
index 000000000000..ff56b9384fcc
@@ -0,0 +1,27 @@
+Inside Secure SafeXcel cryptographic engine
+
+Required properties:
+- compatible: Should be "inside-secure,safexcel-eip197".
+- reg: Base physical address of the engine and length of memory mapped region.
+- interrupts: Interrupt numbers for the rings and engine.
+- interrupt-names: Should be "ring0", "ring1", "ring2", "ring3", "eip", "mem".
+
+Optional properties:
+- clocks: Reference to the crypto engine clock.
+
+Example:
+
+	crypto: crypto at 800000 {
+		compatible = "inside-secure,safexcel-eip197";
+		reg = <0x800000 0x200000>;
+		interrupts = <GIC_SPI 34 (IRQ_TYPE_EDGE_RISING | IRQ_TYPE_LEVEL_HIGH)>,
+			     <GIC_SPI 54 IRQ_TYPE_LEVEL_HIGH>,
+			     <GIC_SPI 55 IRQ_TYPE_LEVEL_HIGH>,
+			     <GIC_SPI 56 IRQ_TYPE_LEVEL_HIGH>,
+			     <GIC_SPI 57 IRQ_TYPE_LEVEL_HIGH>,
+			     <GIC_SPI 58 IRQ_TYPE_LEVEL_HIGH>;
+		interrupt-names = "mem", "ring0", "ring1", "ring2", "ring3",
+				  "eip";
+		clocks = <&cpm_syscon0 1 26>;
+		status = "disabled";
+	};
Signature · 1 line
2.11.0
#2 lore raw

PATCH 2/7 crypto: inside-secure: add SafeXcel EIP197 crypto engine driver

In reply to Antoine Tenart

Add support for Inside Secure SafeXcel EIP197 cryptographic engine, which can be found on Marvell Armada 7k and 8k boards. This driver currently implements: ecb(aes), cbc(aes), sha1, sha224, sha256 and hmac(sah1) algorithms.

Two firmwares are needed for this engine to work. Their are mostly used for more advanced operations than the ones supported (as of now), but we still need them to pass the data to the internal cryptographic engine.

---
 drivers/crypto/Kconfig                         |   17 +
 drivers/crypto/Makefile                        |    1 +
 drivers/crypto/inside-secure/Makefile          |    2 +
 drivers/crypto/inside-secure/safexcel.c        |  939 +++++++++++++++++++++
 drivers/crypto/inside-secure/safexcel.h        |  581 +++++++++++++
 drivers/crypto/inside-secure/safexcel_cipher.c |  544 ++++++++++++
 drivers/crypto/inside-secure/safexcel_hash.c   | 1045 ++++++++++++++++++++++++
 drivers/crypto/inside-secure/safexcel_ring.c   |  173 ++++
 8 files changed, 3302 insertions(+)
 create mode 100644 drivers/crypto/inside-secure/Makefile
 create mode 100644 drivers/crypto/inside-secure/safexcel.c
 create mode 100644 drivers/crypto/inside-secure/safexcel.h
 create mode 100644 drivers/crypto/inside-secure/safexcel_cipher.c
 create mode 100644 drivers/crypto/inside-secure/safexcel_hash.c
 create mode 100644 drivers/crypto/inside-secure/safexcel_ring.c
drivers/crypto/Kconfig +17 -0
index 473d31288ad8..4ff39d3132dc 100644
@@ -619,4 +619,21 @@ config CRYPTO_DEV_BCM_SPU
 	  Secure Processing Unit (SPU). The SPU driver registers ablkcipher,
 	  ahash, and aead algorithms with the kernel cryptographic API.
 
+config CRYPTO_DEV_SAFEXCEL
+	tristate "Inside Secure's SafeXcel cryptographic engine driver"
+	depends on HAS_DMA && OF
+	depends on (ARM64 && ARCH_MVEBU) || COMPILE_TEST
+	select CRYPTO_AES
+	select CRYPTO_BLKCIPHER
+	select CRYPTO_HASH
+	select CRYPTO_HMAC
+	select CRYPTO_SHA1
+	select CRYPTO_SHA256
+	select CRYPTO_SHA512
+	help
+	  This driver interfaces with the SafeXcel EIP-197 cryptographic engine
+	  designed by Inside Secure. Select this if you want to use CBC/ECB
+	  chain mode, AES cipher mode and SHA1/SHA224/SHA256/SHA512 hash
+	  algorithms.
+
 endif # CRYPTO_HW
drivers/crypto/Makefile +1 -0
index 739609471169..7ed3e7940f76 100644
@@ -36,3 +36,4 @@ obj-$(CONFIG_CRYPTO_DEV_UX500) += ux500/
 obj-$(CONFIG_CRYPTO_DEV_VIRTIO) += virtio/
 obj-$(CONFIG_CRYPTO_DEV_VMX) += vmx/
 obj-$(CONFIG_CRYPTO_DEV_BCM_SPU) += bcm/
+obj-$(CONFIG_CRYPTO_DEV_SAFEXCEL) += inside-secure/
drivers/crypto/inside-secure/Makefile new file +2 -0
new file mode 100644
index 000000000000..302f07dde98c
@@ -0,0 +1,2 @@
+obj-$(CONFIG_CRYPTO_DEV_SAFEXCEL) += crypto_safexcel.o
+crypto_safexcel-objs := safexcel.o safexcel_ring.o safexcel_cipher.o safexcel_hash.o
drivers/crypto/inside-secure/safexcel.c new file +939 -0

939 lines not on this page · raw message

drivers/crypto/inside-secure/safexcel.h new file +581 -0

581 lines not on this page · raw message

drivers/crypto/inside-secure/safexcel_cipher.c new file +544 -0

544 lines not on this page · raw message

drivers/crypto/inside-secure/safexcel_hash.c new file +1045 -0

909 lines not on this page · raw message

drivers/crypto/inside-secure/safexcel_ring.c new file +173 -0

173 lines not on this page · raw message

The rest of this diff is not shown. View the raw message.

Signature · 1 line
2.11.0

Part of the diff is not shown here. View the raw message.

#3 lore raw

PATCH 2/7 crypto: inside-secure: add SafeXcel EIP197 crypto engine driver

Hi Antoine,

Bit of a drive-by, but since I have it in my head that crypto drivers are a hotspot for dodgy DMA usage (in part due to the hardware often being a bit esoteric with embedded RAMs and such), this caught my eye and I thought I'd give this a quick once-over to check for anything smelly. Unfortunately, I was not disappointed... ;)

On 29/03/17 13:44, Antoine Tenart wrote:
[...]

Quoted 5 lines
drivers/crypto/inside-secure/safexcel.c new file +0 -0
new file mode 100644
index 000000000000..00f3f2c85d05

[...]

drivers/crypto/inside-secure/safexcel.c line 563

Quoted 12 lines
drivers/crypto/inside-secure/safexcel.c new file +12 -0
+int safexcel_invalidate_cache(struct crypto_async_request *async,
+			      struct safexcel_context *ctx,
+			      struct safexcel_crypto_priv *priv,
+			      dma_addr_t ctxr_dma, int ring,
+			      struct safexcel_request *request)
+{
+	struct safexcel_command_desc *cdesc;
+	struct safexcel_result_desc *rdesc;
+	phys_addr_t ctxr_phys;
+	int ret = 0;
+
+	ctxr_phys = dma_to_phys(priv->dev, ctxr_dma);

No no no. This is a SWIOTLB-specific (or otherwise arch-private, depending on implementation) helper, not a DMA API function, and should not be called directly by drivers. There is no guarantee it will give the result you expect, or even compile, in all cases (if the kbuild robot hasn't already revealed via the COMPILE_TEST dependency).

That said, AFAICS ctxr_phys ends up in a descriptor which is given to the device, so trying to use a physical address is wrong and it should still be the DMA address anyway.

Quoted 32 lines
drivers/crypto/inside-secure/safexcel.c new file +32 -0
+	spin_lock_bh(&priv->ring[ring].egress_lock);
+
+	/* prepare command descriptor */
+	cdesc = safexcel_add_cdesc(priv, ring, true, true, 0, 0, 0, ctxr_phys);
+	if (IS_ERR(cdesc)) {
+		ret = PTR_ERR(cdesc);
+		goto unlock;
+	}
+
+	cdesc->control_data.type = EIP197_TYPE_EXTENDED;
+	cdesc->control_data.options = 0;
+	cdesc->control_data.refresh = 0;
+	cdesc->control_data.control0 = CONTEXT_CONTROL_INV_TR;
+
+	/* prepare result descriptor */
+	rdesc = safexcel_add_rdesc(priv, ring, true, true, 0, 0);
+
+	if (IS_ERR(rdesc)) {
+		ret = PTR_ERR(rdesc);
+		goto cdesc_rollback;
+	}
+
+	request->req = async;
+	goto unlock;
+
+cdesc_rollback:
+	safexcel_ring_rollback_wptr(priv, &priv->ring[ring].cdr);
+
+unlock:
+	spin_unlock_bh(&priv->ring[ring].egress_lock);
+	return ret;
+}

[...]

Quoted 23 lines
drivers/crypto/inside-secure/safexcel.c new file +23 -0
+static int safexcel_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct resource *res;
+	struct safexcel_crypto_priv *priv;
+	int i, ret;
+
+	priv = devm_kzalloc(dev, sizeof(struct safexcel_crypto_priv),
+			    GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->dev = dev;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	priv->base = devm_ioremap_resource(dev, res);
+	if (IS_ERR(priv->base)) {
+		dev_err(dev, "failed to get resource\n");
+		return PTR_ERR(priv->base);
+	}
+
+	priv->clk = of_clk_get(dev->of_node, 0);
+	if (!IS_ERR(priv->clk)) {

drivers/crypto/inside-secure/safexcel.c line 818

Quoted 10 lines
drivers/crypto/inside-secure/safexcel.c new file +10 -0
+		ret = clk_prepare_enable(priv->clk);
+		if (ret) {
+			dev_err(dev, "unable to enable clk (%d)\n", ret);
+			return ret;
+		}
+	} else {
+		/* The clock isn't mandatory */
+		if (PTR_ERR(priv->clk) == -EPROBE_DEFER)
+			return -EPROBE_DEFER;
+	}

You should call dma_set_mask_and_coherent() before any DMA API calls, both to confirm DMA really is going to work all, and also (since this IP apparently supports >32-bit addresses) to describe the full inherent addressing capability, not least to avoid wasting time/space with unnecessary bounce buffering otherwise.

Quoted 67 lines
drivers/crypto/inside-secure/safexcel.c new file +67 -0
+	priv->context_pool = dmam_pool_create("safexcel-context", dev,
+					      sizeof(struct safexcel_context_record),
+					      1, 0);
+	if (!priv->context_pool) {
+		ret = -ENOMEM;
+		goto err_clk;
+	}
+
+	safexcel_configure(priv);
+
+	for (i = 0; i < priv->config.rings; i++) {
+		char irq_name[6] = {0}; /* "ringX\0" */
+		char wq_name[9] = {0}; /* "wq_ringX\0" */
+		int irq;
+		struct safexcel_ring_irq_data *ring_irq;
+
+		ret = safexcel_init_ring_descriptors(priv,
+						     &priv->ring[i].cdr,
+						     &priv->ring[i].rdr);
+		if (ret)
+			goto err_pool;
+
+		ring_irq = devm_kzalloc(dev, sizeof(struct safexcel_ring_irq_data),
+					GFP_KERNEL);
+		if (!ring_irq) {
+			ret = -ENOMEM;
+			goto err_pool;
+		}
+
+		ring_irq->priv = priv;
+		ring_irq->ring = i;
+
+		snprintf(irq_name, 6, "ring%d", i);
+		irq = safexcel_request_ring_irq(pdev, irq_name, safexcel_irq_ring,
+						ring_irq);
+
+		if (irq < 0)
+			goto err_pool;
+
+		priv->ring[i].work_data.priv = priv;
+		priv->ring[i].work_data.ring = i;
+		INIT_WORK(&priv->ring[i].work_data.work, safexcel_handle_result_work);
+
+		snprintf(wq_name, 9, "wq_ring%d", i);
+		priv->ring[i].workqueue = create_singlethread_workqueue(wq_name);
+		if (!priv->ring[i].workqueue) {
+			ret = -ENOMEM;
+			goto err_pool;
+		}
+
+		INIT_LIST_HEAD(&priv->ring[i].list);
+		spin_lock_init(&priv->ring[i].lock);
+		spin_lock_init(&priv->ring[i].egress_lock);
+	}
+
+	platform_set_drvdata(pdev, priv);
+	atomic_set(&priv->ring_used, 0);
+
+	spin_lock_init(&priv->lock);
+	crypto_init_queue(&priv->queue, EIP197_DEFAULT_RING_SIZE);
+
+	ret = safexcel_hw_init(priv);
+	if (ret) {
+		dev_err(dev, "EIP h/w init failed (%d)\n", ret);
+		goto err_pool;
+	}
+

drivers/crypto/inside-secure/safexcel.c line 896

Quoted 10 lines
drivers/crypto/inside-secure/safexcel.c new file +10 -0
+	ret = safexcel_register_algorithms(priv);
+	if (ret) {
+		dev_err(dev, "Failed to register algorithms (%d)\n", ret);
+		goto err_pool;
+	}
+
+	return 0;
+
+err_pool:
+	dma_pool_destroy(priv->context_pool);

You used dmam_pool_create(), so not only is an explicit destroy unnecessary, but doing so with the non-managed version is actively harmful, since devres would end up double-freeing the pool after you return.

Quoted 4 lines
drivers/crypto/inside-secure/safexcel.c new file +4 -0
+err_clk:
+	clk_disable_unprepare(priv->clk);
+	return ret;
+}

[...]

Quoted 5 lines
drivers/crypto/inside-secure/safexcel.h new file +0 -0
new file mode 100644
index 000000000000..ffe39aaabe1c

[...]

Quoted 36 lines
drivers/crypto/inside-secure/safexcel.h new file +36 -0
+/* Result data */
+struct result_data_desc {
+	u32 packet_length:17;
+	u32 error_code:15;
+
+	u8 bypass_length:4;
+	u8 e15:1;
+	u16 rsvd0;
+	u8 hash_bytes:1;
+	u8 hash_length:6;
+	u8 generic_bytes:1;
+	u8 checksum:1;
+	u8 next_header:1;
+	u8 length:1;
+
+	u16 application_id;
+	u16 rsvd1;
+
+	u32 rsvd2;
+} __packed;
+
+
+/* Basic Result Descriptor format */
+struct safexcel_result_desc {
+	u32 particle_size:17;
+	u8 rsvd0:3;
+	u8 descriptor_overflow:1;
+	u8 buffer_overflow:1;
+	u8 last_seg:1;
+	u8 first_seg:1;
+	u16 result_size:8;
+
+	u32 rsvd1;
+
+	u32 data_lo;
+	u32 data_hi;

drivers/crypto/inside-secure/safexcel.h line 357

Quoted 10 lines
drivers/crypto/inside-secure/safexcel.h new file +10 -0
+
+	struct result_data_desc result_data;
+} __packed;
+
+struct safexcel_token {
+	u32 packet_length:17;
+	u8 stat:2;
+	u16 instructions:9;
+	u8 opcode:4;
+} __packed;

Using bitfields in hardware descriptors seems pretty risky, since you've no real guarantee two compilers will lay them out the same way (or that either would be correct WRT what the hardware expects). I'd be more inclined to construct all these fields with explicit masking and shifting.

[...]

Quoted 5 lines
drivers/crypto/inside-secure/safexcel_cipher.c new file +0 -0
new file mode 100644
index 000000000000..ff42e45ae43e

[...]

Quoted 11 lines
drivers/crypto/inside-secure/safexcel_cipher.c new file +11 -0
+static int safexcel_aes_send(struct crypto_async_request *async,
+			     int ring, struct safexcel_request *request,
+			     int *commands, int *results)
+{
+	struct ablkcipher_request *req = ablkcipher_request_cast(async);
+	struct safexcel_cipher_ctx *ctx = crypto_tfm_ctx(req->base.tfm);
+	struct safexcel_crypto_priv *priv = ctx->priv;
+	struct safexcel_command_desc *cdesc;
+	struct safexcel_result_desc *rdesc;
+	struct scatterlist *sg;
+	phys_addr_t ctxr_phys;

drivers/crypto/inside-secure/safexcel_cipher.c line 195

Quoted 10 lines
drivers/crypto/inside-secure/safexcel_cipher.c new file +10 -0
+	int nr_src, nr_dst, n_cdesc = 0, n_rdesc = 0, queued = req->nbytes;
+	int i, ret = 0;
+
+	request->req = &req->base;
+
+	if (req->src == req->dst) {
+		nr_src = sg_nents_for_len(req->src, req->nbytes);
+		nr_dst = nr_src;
+
+		if (dma_map_sg(priv->dev, req->src, nr_src, DMA_BIDIRECTIONAL) <= 0)

Nit: you only need to check for zero/nonzero to determine failure (similarly below) - dma_map_sg() cannot return negative values.

Bigger complaint: you should not ignore the successful return value and rely on it being equal to nr_src - please see the description of dma_map_sg() in Documenatation/DMA-API.txt

Quoted 6 lines
drivers/crypto/inside-secure/safexcel_cipher.c new file +6 -0
+			return -EINVAL;
+	} else {
+		nr_src = sg_nents_for_len(req->src, req->nbytes);
+		nr_dst = sg_nents_for_len(req->dst, req->nbytes);
+
+		if (dma_map_sg(priv->dev, req->src, nr_src, DMA_TO_DEVICE) <= 0)

drivers/crypto/inside-secure/safexcel_cipher.c line 211

Quoted 10 lines
drivers/crypto/inside-secure/safexcel_cipher.c new file +10 -0
+			return -EINVAL;
+
+		if (dma_map_sg(priv->dev, req->dst, nr_dst, DMA_FROM_DEVICE) <= 0) {
+			dma_unmap_sg(priv->dev, req->src, nr_src,
+				     DMA_TO_DEVICE);
+			return -EINVAL;
+		}
+	}
+
+	ctxr_phys = dma_to_phys(priv->dev, ctx->base.ctxr_dma);

Once again, wrong and probably unnecessary.

drivers/crypto/inside-secure/safexcel_cipher.c line 220

Quoted 8 lines
drivers/crypto/inside-secure/safexcel_cipher.c new file +8 -0
+	memcpy(ctx->base.ctxr->data, ctx->key, ctx->key_len);
+
+	spin_lock_bh(&priv->ring[ring].egress_lock);
+
+	/* command descriptors */
+	for_each_sg(req->src, sg, nr_src, i) {
+		phys_addr_t sg_phys = dma_to_phys(priv->dev, sg_dma_address(sg));
+		int len = sg_dma_len(sg);

If dma_map_sg() coalesced any entries into the same mapping such that count < nents, these could well give bogus values toward the end of the list once i >= count(if you're lucky, at least len might be 0).

Quoted 17 lines
drivers/crypto/inside-secure/safexcel_cipher.c new file +17 -0
+		/* Do not overflow the request */
+		if (queued - len < 0)
+			len = queued;
+
+		cdesc = safexcel_add_cdesc(priv, ring, !n_cdesc, !(queued - len),
+					   sg_phys, len, req->nbytes, ctxr_phys);
+		if (IS_ERR(cdesc)) {
+			/* No space left in the command descriptor ring */
+			ret = PTR_ERR(cdesc);
+			goto cdesc_rollback;
+		}
+		n_cdesc++;
+
+		if (n_cdesc == 1) {
+			safexcel_context_control(ctx, cdesc);
+			safexcel_cipher_token(ctx, async, cdesc, req->nbytes);
+		}

drivers/crypto/inside-secure/safexcel_cipher.c line 248

Quoted 10 lines
drivers/crypto/inside-secure/safexcel_cipher.c new file +10 -0
+
+		queued -= len;
+		if (!queued)
+			break;
+	}
+
+	/* result descriptors */
+	for_each_sg(req->dst, sg, nr_dst, i) {
+		bool first = !i, last = (i == nr_dst - 1);
+		phys_addr_t sg_phys = dma_to_phys(priv->dev, sg_dma_address(sg));

And again, no. Also note that sg_phys() already exists as a function, so I imagine sparse could get quite cross here.

drivers/crypto/inside-secure/safexcel_cipher.c line 249

Quoted 1 line
drivers/crypto/inside-secure/safexcel_cipher.c new file +1 -0
+		u32 len = sg_dma_len(sg);

Plus the same comment again about potentially having iterated past the point where these are valid.

Quoted 22 lines
drivers/crypto/inside-secure/safexcel_cipher.c new file +22 -0
+
+		rdesc = safexcel_add_rdesc(priv, ring, first, last, sg_phys, len);
+		if (IS_ERR(rdesc)) {
+			/* No space left in the result descriptor ring */
+			ret = PTR_ERR(rdesc);
+			goto rdesc_rollback;
+		}
+		n_rdesc++;
+	}
+
+	ctx->base.handle_result = safexcel_handle_result;
+
+	spin_unlock_bh(&priv->ring[ring].egress_lock);
+
+	*commands = n_cdesc;
+	*results = nr_dst;
+	return 0;
+
+rdesc_rollback:
+	for (i = 0; i < n_rdesc; i++)
+		safexcel_ring_rollback_wptr(priv, &priv->ring[ring].rdr);
+cdesc_rollback:

drivers/crypto/inside-secure/safexcel_cipher.c line 281

Quoted 10 lines
drivers/crypto/inside-secure/safexcel_cipher.c new file +10 -0
+	for (i = 0; i < n_cdesc; i++)
+		safexcel_ring_rollback_wptr(priv, &priv->ring[ring].cdr);
+
+	spin_unlock_bh(&priv->ring[ring].egress_lock);
+
+	if (req->src == req->dst) {
+		dma_unmap_sg(priv->dev, req->src, nr_src, DMA_BIDIRECTIONAL);
+	} else {
+		dma_unmap_sg(priv->dev, req->src, nr_src, DMA_TO_DEVICE);
+		dma_unmap_sg(priv->dev, req->dst, nr_dst, DMA_FROM_DEVICE);

I will note that the unmap_sg calls *should* have the same nents value as originally passed to map_sg, so these remain correct.

Quoted 4 lines
drivers/crypto/inside-secure/safexcel_cipher.c new file +4 -0
+	}
+
+	return ret;
+}

[...]

Quoted 5 lines
drivers/crypto/inside-secure/safexcel_hash.c new file +0 -0
new file mode 100644
index 000000000000..1f44e0a2ddf1

[...]

Quoted 27 lines
drivers/crypto/inside-secure/safexcel_hash.c new file +27 -0
+static int safexcel_ahash_send(struct crypto_async_request *async, int ring,
+			       struct safexcel_request *request, int *commands,
+			       int *results)
+{
+	struct ahash_request *areq = ahash_request_cast(async);
+	struct crypto_ahash *ahash = crypto_ahash_reqtfm(areq);
+	struct safexcel_ahash_req *req = ahash_request_ctx(areq);
+	struct safexcel_ahash_ctx *ctx = crypto_ahash_ctx(crypto_ahash_reqtfm(areq));
+	struct safexcel_crypto_priv *priv = ctx->priv;
+	struct safexcel_command_desc *cdesc, *first_cdesc = NULL;
+	struct safexcel_result_desc *rdesc;
+	struct scatterlist *sg;
+	int i, nents, queued, len, cache_len, extra, n_cdesc = 0, ret = 0;
+	phys_addr_t ctxr_phys = 0;
+
+	queued = len = req->len - req->processed;
+	if (queued < crypto_ahash_blocksize(ahash))
+		cache_len = queued;
+	else
+		cache_len = queued - areq->nbytes;
+
+	/*
+	 * If this is not the last request and the queued data does not fit
+	 * into full blocks, cache it for the next send() call.
+	 */
+	extra = queued & (crypto_ahash_blocksize(ahash) - 1);
+	if (!req->last_req && extra) {

drivers/crypto/inside-secure/safexcel_hash.c line 218

Quoted 10 lines
drivers/crypto/inside-secure/safexcel_hash.c new file +10 -0
+		sg_pcopy_to_buffer(areq->src, sg_nents(areq->src),
+				   req->cache_next, extra, areq->nbytes - extra);
+
+		queued -= extra;
+		len -= extra;
+	}
+
+	request->req = &areq->base;
+	ctx->base.handle_result = safexcel_handle_result;
+	ctxr_phys = dma_to_phys(priv->dev, ctx->base.ctxr_dma);

No.

drivers/crypto/inside-secure/safexcel_hash.c line 224

Quoted 5 lines
drivers/crypto/inside-secure/safexcel_hash.c new file +5 -0
+	spin_lock_bh(&priv->ring[ring].egress_lock);
+
+	/* Add a command descriptor for the cached data, if any */
+	if (cache_len) {
+		ctx->base.cache_dma = dma_map_single(priv->dev, req->cache,

This is pretty dodgy, since req->cache is inside a live data structure, adjoining parts of which are updated whilst still mapped (the update of req->processed below). You just about get away without data corruption since we *probably* don't invalidate anything when unmapping DMA_TO_DEVICE, and the coherency hardware *probably* doesn't do anything crazy, but you've no real guarantee of that - any DMA buffer should really be separately kmalloced. "That's a nice dirty cache line you've got there, it'd be a shame if anything were to happen to it..."

drivers/crypto/inside-secure/safexcel_hash.c line 234

Quoted 10 lines
drivers/crypto/inside-secure/safexcel_hash.c new file +10 -0
+						     cache_len, DMA_TO_DEVICE);
+		if (dma_mapping_error(priv->dev, ctx->base.cache_dma)) {
+			ret = -EINVAL;
+			goto unlock;
+		}
+
+		ctx->base.cache_sz = cache_len;
+		first_cdesc = safexcel_add_cdesc(priv, ring, 1,
+						 (cache_len == len),
+						 dma_to_phys(priv->dev, ctx->base.cache_dma),

No.

Quoted 5 lines
drivers/crypto/inside-secure/safexcel_hash.c new file +5 -0
+						 cache_len, len, ctxr_phys);
+		if (IS_ERR(first_cdesc)) {
+			ret = PTR_ERR(first_cdesc);
+			goto free_cache;
+		}

drivers/crypto/inside-secure/safexcel_hash.c line 249

Quoted 10 lines
drivers/crypto/inside-secure/safexcel_hash.c new file +10 -0
+		n_cdesc++;
+
+		queued -= cache_len;
+		if (!queued)
+			goto send_command;
+	}
+
+	/* Now handle the current ahash request buffer(s) */
+	nents = sg_nents_for_len(areq->src, areq->nbytes);
+	if (dma_map_sg(priv->dev, areq->src, nents, DMA_TO_DEVICE) <= 0) {

Same comments about the return value.

drivers/crypto/inside-secure/safexcel_hash.c line 255

Quoted 6 lines
drivers/crypto/inside-secure/safexcel_hash.c new file +6 -0
+		ret = -ENOMEM;
+		goto cdesc_rollback;
+	}
+
+	for_each_sg(areq->src, sg, nents, i) {
+		phys_addr_t sg_phys = dma_to_phys(priv->dev, sg_dma_address(sg));

No.

Quoted 66 lines
drivers/crypto/inside-secure/safexcel_hash.c new file +66 -0
+		int sglen = sg_dma_len(sg);
+
+		/* Do not overflow the request */
+		if (queued - sglen < 0)
+			sglen = queued;
+
+		cdesc = safexcel_add_cdesc(priv, ring, !n_cdesc,
+					   !(queued - sglen), sg_phys, sglen,
+					   len, ctxr_phys);
+		if (IS_ERR(cdesc)) {
+			ret = PTR_ERR(cdesc);
+			goto cdesc_rollback;
+		}
+		n_cdesc++;
+
+		if (n_cdesc == 1)
+			first_cdesc = cdesc;
+
+		queued -= sglen;
+		if (!queued)
+			break;
+	}
+
+send_command:
+	/* Setup the context options */
+	safexcel_context_control(ctx, req, first_cdesc, req->state_sz,
+				 crypto_ahash_blocksize(ahash));
+
+	/* Add the token */
+	safexcel_hash_token(first_cdesc, len, req->state_sz);
+
+	ctx->base.result_dma = dma_map_single(priv->dev, areq->result,
+					      req->state_sz, DMA_FROM_DEVICE);
+	if (dma_mapping_error(priv->dev, ctx->base.result_dma)) {
+		ret = -EINVAL;
+		goto cdesc_rollback;
+	}
+
+	/* Add a result descriptor */
+	rdesc = safexcel_add_rdesc(priv, ring, 1, 1,
+				   dma_to_phys(priv->dev, ctx->base.result_dma),
+				   req->state_sz);
+	if (IS_ERR(rdesc)) {
+		ret = PTR_ERR(rdesc);
+		goto cdesc_rollback;
+	}
+
+	req->processed += len;
+	spin_unlock_bh(&priv->ring[ring].egress_lock);
+
+	*commands = n_cdesc;
+	*results = 1;
+	return 0;
+
+cdesc_rollback:
+	for (i = 0; i < n_cdesc; i++)
+		safexcel_ring_rollback_wptr(priv, &priv->ring[ring].cdr);
+free_cache:
+	if (ctx->base.cache_dma)
+		dma_unmap_single(priv->dev, ctx->base.cache_dma,
+				 ctx->base.cache_sz, DMA_TO_DEVICE);
+
+unlock:
+	spin_unlock_bh(&priv->ring[ring].egress_lock);
+	return ret;
+}

[...]

Quoted 5 lines
drivers/crypto/inside-secure/safexcel_ring.c new file +0 -0
new file mode 100644
index 000000000000..5b4ba7a8cc0a

[...]

Quoted 12 lines
drivers/crypto/inside-secure/safexcel_ring.c new file +12 -0
+int safexcel_init_ring_descriptors(struct safexcel_crypto_priv *priv,
+				   struct safexcel_ring *cdr,
+				   struct safexcel_ring *rdr)
+{
+	cdr->offset = sizeof(u32) * priv->config.cd_offset;
+	cdr->base = dmam_alloc_coherent(priv->dev,
+					cdr->offset * EIP197_DEFAULT_RING_SIZE,
+					&cdr->base_dma, GFP_KERNEL);
+	if (!cdr->base)
+		return -ENOMEM;
+	cdr->write = cdr->base;
+	cdr->base_end = cdr->base + cdr->offset * EIP197_DEFAULT_RING_SIZE;

drivers/crypto/inside-secure/safexcel_ring.c line 37

Quoted 10 lines
drivers/crypto/inside-secure/safexcel_ring.c new file +10 -0
+	cdr->read = cdr->base;
+
+	rdr->offset = sizeof(u32) * priv->config.rd_offset;
+	rdr->base = dmam_alloc_coherent(priv->dev,
+					rdr->offset * EIP197_DEFAULT_RING_SIZE,
+					&rdr->base_dma, GFP_KERNEL);
+	if (!rdr->base) {
+		dmam_free_coherent(priv->dev,
+				   cdr->offset * EIP197_DEFAULT_RING_SIZE,
+				   cdr->base, cdr->base_dma);

Returning an error here will abort your probe routine, so devres will clean these up there and then - there's no need to do free anything explicitly. That's rather the point of using devm_*() to begin with.

Quoted 10 lines
drivers/crypto/inside-secure/safexcel_ring.c new file +10 -0
+		return -ENOMEM;
+	}
+	rdr->write = rdr->base;
+	rdr->base_end = rdr->base + rdr->offset * EIP197_DEFAULT_RING_SIZE;
+	rdr->read = rdr->base;
+
+	return 0;
+}
+
+void safexcel_free_ring_descriptors(struct safexcel_crypto_priv *priv,

drivers/crypto/inside-secure/safexcel_ring.c line 57

Quoted 10 lines
drivers/crypto/inside-secure/safexcel_ring.c new file +10 -0
+				    struct safexcel_ring *cdr,
+				    struct safexcel_ring *rdr)
+{
+	dmam_free_coherent(priv->dev,
+			   cdr->offset * EIP197_DEFAULT_RING_SIZE,
+			   cdr->base, cdr->base_dma);
+	dmam_free_coherent(priv->dev,
+			   rdr->offset * EIP197_DEFAULT_RING_SIZE,
+			   rdr->base, rdr->base_dma);
+}

Again, this is only called at device teardown, so the whole function is redundant.

Robin.

#4 lore raw

PATCH 2/7 crypto: inside-secure: add SafeXcel EIP197 crypto engine driver

Hi Robin,

Quoted 6 lines · Robin Murphy

Bit of a drive-by, but since I have it in my head that crypto drivers are a hotspot for dodgy DMA usage (in part due to the hardware often being a bit esoteric with embedded RAMs and such), this caught my eye and I thought I'd give this a quick once-over to check for anything smelly. Unfortunately, I was not disappointed... ;)

:-)

Quoted 27 lines · Antoine Tenart

[...]

Quoted 5 lines
drivers/crypto/inside-secure/safexcel.c new file +0 -0
new file mode 100644
index 000000000000..00f3f2c85d05

[...]

drivers/crypto/inside-secure/safexcel.c line 563

Quoted 12 lines
drivers/crypto/inside-secure/safexcel.c new file +12 -0
+int safexcel_invalidate_cache(struct crypto_async_request *async,
+			      struct safexcel_context *ctx,
+			      struct safexcel_crypto_priv *priv,
+			      dma_addr_t ctxr_dma, int ring,
+			      struct safexcel_request *request)
+{
+	struct safexcel_command_desc *cdesc;
+	struct safexcel_result_desc *rdesc;
+	phys_addr_t ctxr_phys;
+	int ret = 0;
+
+	ctxr_phys = dma_to_phys(priv->dev, ctxr_dma);

No no no. This is a SWIOTLB-specific (or otherwise arch-private, depending on implementation) helper, not a DMA API function, and should not be called directly by drivers. There is no guarantee it will give the result you expect, or even compile, in all cases (if the kbuild robot hasn't already revealed via the COMPILE_TEST dependency).

Quoted 3 lines

That said, AFAICS ctxr_phys ends up in a descriptor which is given to the device, so trying to use a physical address is wrong and it should still be the DMA address anyway.

I see. The cryptographic engine should always use dma addresses. I'll update the descriptors structure members from "phys" to "dma" as well.

Quoted 32 lines

[...]

Quoted 31 lines
drivers/crypto/inside-secure/safexcel.c new file +31 -0
+static int safexcel_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct resource *res;
+	struct safexcel_crypto_priv *priv;
+	int i, ret;
+
+	priv = devm_kzalloc(dev, sizeof(struct safexcel_crypto_priv),
+			    GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->dev = dev;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	priv->base = devm_ioremap_resource(dev, res);
+	if (IS_ERR(priv->base)) {
+		dev_err(dev, "failed to get resource\n");
+		return PTR_ERR(priv->base);
+	}
+
+	priv->clk = of_clk_get(dev->of_node, 0);
+	if (!IS_ERR(priv->clk)) {
+		ret = clk_prepare_enable(priv->clk);
+		if (ret) {
+			dev_err(dev, "unable to enable clk (%d)\n", ret);
+			return ret;
+		}
+	} else {
+		/* The clock isn't mandatory */
+		if (PTR_ERR(priv->clk) == -EPROBE_DEFER)
Quoted 8 lines

drivers/crypto/inside-secure/safexcel.c line 818

Quoted 2 lines
drivers/crypto/inside-secure/safexcel.c new file +2 -0
+			return -EPROBE_DEFER;
+	}

You should call dma_set_mask_and_coherent() before any DMA API calls, both to confirm DMA really is going to work all, and also (since this IP apparently supports >32-bit addresses) to describe the full inherent addressing capability, not least to avoid wasting time/space with unnecessary bounce buffering otherwise.

OK, I'll add a call to this helper before using DMA API calls.

Quoted 7 lines

drivers/crypto/inside-secure/safexcel.c line 896

Quoted 3 lines
drivers/crypto/inside-secure/safexcel.c new file +3 -0
+
+err_pool:
+	dma_pool_destroy(priv->context_pool);

You used dmam_pool_create(), so not only is an explicit destroy unnecessary, but doing so with the non-managed version is actively harmful, since devres would end up double-freeing the pool after you return.

I saw this and fixed it in my local version.

Quoted 11 lines

drivers/crypto/inside-secure/safexcel.c

Quoted 6 lines
drivers/crypto/inside-secure/safexcel.c new file +6 -0
+struct safexcel_token {
+	u32 packet_length:17;
+	u8 stat:2;
+	u16 instructions:9;
+	u8 opcode:4;
+} __packed;

Using bitfields in hardware descriptors seems pretty risky, since you've no real guarantee two compilers will lay them out the same way (or that either would be correct WRT what the hardware expects). I'd be more inclined to construct all these fields with explicit masking and shifting.

Hmm, I'm not sure to follow you here. If I use bitfields in a __packed structure, we should be safe. Why would the compiler have a different behaviour?

Quoted 16 lines
Quoted 16 lines
drivers/crypto/inside-secure/safexcel.c new file +16 -0
+static int safexcel_aes_send(struct crypto_async_request *async,
+			     int ring, struct safexcel_request *request,
+			     int *commands, int *results)
+{
+	struct ablkcipher_request *req = ablkcipher_request_cast(async);
+	struct safexcel_cipher_ctx *ctx = crypto_tfm_ctx(req->base.tfm);
+	struct safexcel_crypto_priv *priv = ctx->priv;
+	struct safexcel_command_desc *cdesc;
+	struct safexcel_result_desc *rdesc;
+	struct scatterlist *sg;
+	phys_addr_t ctxr_phys;
+	int nr_src, nr_dst, n_cdesc = 0, n_rdesc = 0, queued = req->nbytes;
+	int i, ret = 0;
+
+	request->req = &req->base;
+
Quoted 8 lines

drivers/crypto/inside-secure/safexcel.c

Quoted 5 lines
drivers/crypto/inside-secure/safexcel.c new file +5 -0
+	if (req->src == req->dst) {
+		nr_src = sg_nents_for_len(req->src, req->nbytes);
+		nr_dst = nr_src;
+
+		if (dma_map_sg(priv->dev, req->src, nr_src, DMA_BIDIRECTIONAL) <= 0)

Nit: you only need to check for zero/nonzero to determine failure (similarly below) - dma_map_sg() cannot return negative values.

OK.

Quoted 3 lines

Bigger complaint: you should not ignore the successful return value and rely on it being equal to nr_src - please see the description of dma_map_sg() in Documenatation/DMA-API.txt

OK, I'll have a look at it and fix this.

Quoted 8 lines

drivers/crypto/inside-secure/safexcel.c

Quoted 4 lines
drivers/crypto/inside-secure/safexcel.c new file +4 -0
+	/* command descriptors */
+	for_each_sg(req->src, sg, nr_src, i) {
+		phys_addr_t sg_phys = dma_to_phys(priv->dev, sg_dma_address(sg));
+		int len = sg_dma_len(sg);

If dma_map_sg() coalesced any entries into the same mapping such that count < nents, these could well give bogus values toward the end of the list once i >= count(if you're lucky, at least len might be 0).

OK, I'll fix this.

Quoted 4 lines

drivers/crypto/inside-secure/safexcel.c

Quoted 3 lines
drivers/crypto/inside-secure/safexcel.c new file +3 -0
+	/* Add a command descriptor for the cached data, if any */
+	if (cache_len) {
+		ctx->base.cache_dma = dma_map_single(priv->dev, req->cache,
Quoted 8 lines

This is pretty dodgy, since req->cache is inside a live data structure, adjoining parts of which are updated whilst still mapped (the update of req->processed below). You just about get away without data corruption since we *probably* don't invalidate anything when unmapping DMA_TO_DEVICE, and the coherency hardware *probably* doesn't do anything crazy, but you've no real guarantee of that - any DMA buffer should really be separately kmalloced. "That's a nice dirty cache line you've got there, it'd be a shame if anything were to happen to it..."

OK, good to know.

Quoted 11 lines

drivers/crypto/inside-secure/safexcel.c

Quoted 7 lines
drivers/crypto/inside-secure/safexcel.c new file +7 -0
+	rdr->base = dmam_alloc_coherent(priv->dev,
+					rdr->offset * EIP197_DEFAULT_RING_SIZE,
+					&rdr->base_dma, GFP_KERNEL);
+	if (!rdr->base) {
+		dmam_free_coherent(priv->dev,
+				   cdr->offset * EIP197_DEFAULT_RING_SIZE,
+				   cdr->base, cdr->base_dma);

Returning an error here will abort your probe routine, so devres will clean these up there and then - there's no need to do free anything explicitly. That's rather the point of using devm_*() to begin with.

Sure.

Quoted 10 lines

drivers/crypto/inside-secure/safexcel.c

Quoted 7 lines
drivers/crypto/inside-secure/safexcel.c new file +7 -0
+	dmam_free_coherent(priv->dev,
+			   cdr->offset * EIP197_DEFAULT_RING_SIZE,
+			   cdr->base, cdr->base_dma);
+	dmam_free_coherent(priv->dev,
+			   rdr->offset * EIP197_DEFAULT_RING_SIZE,
+			   rdr->base, rdr->base_dma);
+}

Again, this is only called at device teardown, so the whole function is redundant.

OK, I'll remove these.

Thanks for the review!
Antoine

Signature · 10 lines
Antoine T?nart, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20170418/2a1b30cf/attachment-0001.sig>
#5 lore raw

PATCH 3/7 MAINTAINERS: add a maintainer for the Inside Secure crypto driver

In reply to Antoine Tenart

A new cryptographic engine driver was added in drivers/crypto/inside-secure. Add myself as a maintainer for this driver.

---
 MAINTAINERS | 6 ++++++
 1 file changed, 6 insertions(+)
MAINTAINERS +6 -0
index c265a5fe4848..7240b9bca638 100644
@@ -6487,6 +6487,12 @@ F:	Documentation/input/multi-touch-protocol.txt
 F:	drivers/input/input-mt.c
 K:	\b(ABS|SYN)_MT_
 
+INSIDE SECURE CRYPTO DRIVER
+M:	Antoine Tenart <[email protected]>
+F:	drivers/crypto/inside-secure/
+S:	Maintained
+L:	linux-crypto at vger.kernel.org
+
 INTEL ASoC BDW/HSW DRIVERS
 M:	Jie Yang <[email protected]>
 L:	alsa-devel at alsa-project.org (moderated for non-subscribers)
Signature · 1 line
2.11.0
#6 lore raw

PATCH 4/7 arm64: marvell: dts: add crypto engine description for 7k/8k

In reply to Antoine Tenart

Add the description of the crypto engine hardware block for the Marvell Armada 7k and Armada 8k processors; for both the CP110 slave and master.

---
 arch/arm64/boot/dts/marvell/armada-cp110-master.dtsi | 15 +++++++++++++++
 arch/arm64/boot/dts/marvell/armada-cp110-slave.dtsi  | 15 +++++++++++++++
 2 files changed, 30 insertions(+)
arch/arm64/boot/dts/marvell/armada-cp110-master.dtsi +15 -0
index 9a2ce2ae49cd..7530a1b541c5 100644
@@ -210,6 +210,21 @@
 				clocks = <&cpm_syscon0 1 25>;
 				status = "okay";
 			};
+
+			cpm_crypto: crypto at 800000 {
+				compatible = "inside-secure,safexcel-eip197";
+				reg = <0x800000 0x200000>;
+				interrupts = <GIC_SPI 34 (IRQ_TYPE_EDGE_RISING | IRQ_TYPE_LEVEL_HIGH)>,
+					     <GIC_SPI 54 IRQ_TYPE_LEVEL_HIGH>,
+					     <GIC_SPI 55 IRQ_TYPE_LEVEL_HIGH>,
+					     <GIC_SPI 56 IRQ_TYPE_LEVEL_HIGH>,
+					     <GIC_SPI 57 IRQ_TYPE_LEVEL_HIGH>,
+					     <GIC_SPI 58 IRQ_TYPE_LEVEL_HIGH>;
+				interrupt-names = "mem", "ring0", "ring1",
+						  "ring2", "ring3", "eip";
+				clocks = <&cpm_syscon0 1 26>;
+				status = "disabled";
+			};
 		};
 
 		cpm_pcie0: pcie at f2600000 {
arch/arm64/boot/dts/marvell/armada-cp110-slave.dtsi +15 -0
index c9dfa244ebb6..6110d39087a6 100644
@@ -210,6 +210,21 @@
 				clocks = <&cps_syscon0 1 25>;
 				status = "okay";
 			};
+
+			cps_crypto: crypto at 800000 {
+				compatible = "inside-secure,safexcel-eip197";
+				reg = <0x800000 0x200000>;
+				interrupts = <GIC_SPI 34 (IRQ_TYPE_EDGE_RISING | IRQ_TYPE_LEVEL_HIGH)>,
+					     <GIC_SPI 278 IRQ_TYPE_LEVEL_HIGH>,
+					     <GIC_SPI 279 IRQ_TYPE_LEVEL_HIGH>,
+					     <GIC_SPI 280 IRQ_TYPE_LEVEL_HIGH>,
+					     <GIC_SPI 281 IRQ_TYPE_LEVEL_HIGH>,
+					     <GIC_SPI 282 IRQ_TYPE_LEVEL_HIGH>;
+				interrupt-names = "mem", "ring0", "ring1",
+						  "ring2", "ring3", "eip";
+				clocks = <&cps_syscon0 1 26>;
+				status = "disabled";
+			};
 		};
 
 		cps_pcie0: pcie at f4600000 {
Signature · 1 line
2.11.0
#7 lore raw

PATCH 4/7 arm64: marvell: dts: add crypto engine description for 7k/8k

Hi Antoine,

Quoted 4 lines · Antoine Tenart

Add the description of the crypto engine hardware block for the Marvell Armada 7k and Armada 8k processors; for both the CP110 slave and master.

Applied on mvebu/dt64, I fixed a merge conflict with the current mvebu/dt64 and I took this opportunity to fix the lines over 80 characters for the interrupts.

Thanks,

Gregory

Quoted 60 lines
---
 arch/arm64/boot/dts/marvell/armada-cp110-master.dtsi | 15 +++++++++++++++
 arch/arm64/boot/dts/marvell/armada-cp110-slave.dtsi  | 15 +++++++++++++++
 2 files changed, 30 insertions(+)
arch/arm64/boot/dts/marvell/armada-cp110-master.dtsi +15 -0
index 9a2ce2ae49cd..7530a1b541c5 100644
@@ -210,6 +210,21 @@
 				clocks = <&cpm_syscon0 1 25>;
 				status = "okay";
 			};
+
+			cpm_crypto: crypto at 800000 {
+				compatible = "inside-secure,safexcel-eip197";
+				reg = <0x800000 0x200000>;
+				interrupts = <GIC_SPI 34 (IRQ_TYPE_EDGE_RISING | IRQ_TYPE_LEVEL_HIGH)>,
+					     <GIC_SPI 54 IRQ_TYPE_LEVEL_HIGH>,
+					     <GIC_SPI 55 IRQ_TYPE_LEVEL_HIGH>,
+					     <GIC_SPI 56 IRQ_TYPE_LEVEL_HIGH>,
+					     <GIC_SPI 57 IRQ_TYPE_LEVEL_HIGH>,
+					     <GIC_SPI 58 IRQ_TYPE_LEVEL_HIGH>;
+				interrupt-names = "mem", "ring0", "ring1",
+						  "ring2", "ring3", "eip";
+				clocks = <&cpm_syscon0 1 26>;
+				status = "disabled";
+			};
 		};
 
 		cpm_pcie0: pcie at f2600000 {
arch/arm64/boot/dts/marvell/armada-cp110-slave.dtsi +15 -0
index c9dfa244ebb6..6110d39087a6 100644
@@ -210,6 +210,21 @@
 				clocks = <&cps_syscon0 1 25>;
 				status = "okay";
 			};
+
+			cps_crypto: crypto at 800000 {
+				compatible = "inside-secure,safexcel-eip197";
+				reg = <0x800000 0x200000>;
+				interrupts = <GIC_SPI 34 (IRQ_TYPE_EDGE_RISING | IRQ_TYPE_LEVEL_HIGH)>,
+					     <GIC_SPI 278 IRQ_TYPE_LEVEL_HIGH>,
+					     <GIC_SPI 279 IRQ_TYPE_LEVEL_HIGH>,
+					     <GIC_SPI 280 IRQ_TYPE_LEVEL_HIGH>,
+					     <GIC_SPI 281 IRQ_TYPE_LEVEL_HIGH>,
+					     <GIC_SPI 282 IRQ_TYPE_LEVEL_HIGH>;
+				interrupt-names = "mem", "ring0", "ring1",
+						  "ring2", "ring3", "eip";
+				clocks = <&cps_syscon0 1 26>;
+				status = "disabled";
+			};
 		};
 
 		cps_pcie0: pcie at f4600000 {
Signature · 1 line
2.11.0
Signature · 4 lines
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
#8 lore raw

PATCH 4/7 arm64: marvell: dts: add crypto engine description for 7k/8k

In reply to Antoine Tenart

Hello,

arch/arm64/boot/dts/marvell/armada-cp110-master.dtsi line 217

Quoted 4 lines · Antoine Tenart
arch/arm64/boot/dts/marvell/armada-cp110-master.dtsi +4 -0
+			cpm_crypto: crypto at 800000 {
+				compatible = "inside-secure,safexcel-eip197";
+				reg = <0x800000 0x200000>;
+				interrupts = <GIC_SPI 34 (IRQ_TYPE_EDGE_RISING | IRQ_TYPE_LEVEL_HIGH)>,

Now that I look into this, does it makes sense for an interrupt to be both an edge interrupt and a level interrupt at the same time? This looks odd.

Best regards,

Thomas

Signature · 3 lines
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
#9 lore raw

PATCH 4/7 arm64: marvell: dts: add crypto engine description for 7k/8k

Hi Thomas,

Quoted 10 lines · Thomas Petazzoni

arch/arm64/boot/dts/marvell/armada-cp110-master.dtsi line 217

Quoted 4 lines · Antoine Tenart
arch/arm64/boot/dts/marvell/armada-cp110-master.dtsi +4 -0
+			cpm_crypto: crypto at 800000 {
+				compatible = "inside-secure,safexcel-eip197";
+				reg = <0x800000 0x200000>;
+				interrupts = <GIC_SPI 34 (IRQ_TYPE_EDGE_RISING | IRQ_TYPE_LEVEL_HIGH)>,

Now that I look into this, does it makes sense for an interrupt to be both an edge interrupt and a level interrupt at the same time? This looks odd.

I agree this looks odd. I took it from Russel's ICU mapping:
http://lists.infradead.org/pipermail/linux-arm-kernel/2017-February/489040.html

Also note the driver does not use it (yet?).

Antoine

Signature · 10 lines
Antoine T?nart, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20170418/b8e12a4e/attachment.sig>
#10 lore raw

PATCH 5/7 arm64: marvell: dts: enable the crypto engine on the Armada 7040 DB

In reply to Antoine Tenart

Enable the cryptographic engine available in the CP110 master on the Armada 7040 DB.

---
 arch/arm64/boot/dts/marvell/armada-7040-db.dts | 4 ++++
 1 file changed, 4 insertions(+)
arch/arm64/boot/dts/marvell/armada-7040-db.dts +4 -0
index bb0e1e8e7138..7e9a8a4e89a4 100644
@@ -171,3 +171,7 @@
 	phy = <&phy1>;
 	phy-mode = "rgmii-id";
 };
+
+&cpm_crypto {
+	status = "okay";
+};
Signature · 1 line
2.11.0
#11 lore raw

PATCH 5/7 arm64: marvell: dts: enable the crypto engine on the Armada 7040 DB

Hi Antoine,

Quoted 4 lines · Antoine Tenart

Enable the cryptographic engine available in the CP110 master on the Armada 7040 DB.

Applied on mvebu/dt64

Thanks,

Gregory

Quoted 19 lines
---
 arch/arm64/boot/dts/marvell/armada-7040-db.dts | 4 ++++
 1 file changed, 4 insertions(+)
arch/arm64/boot/dts/marvell/armada-7040-db.dts +4 -0
index bb0e1e8e7138..7e9a8a4e89a4 100644
@@ -171,3 +171,7 @@
 	phy = <&phy1>;
 	phy-mode = "rgmii-id";
 };
+
+&cpm_crypto {
+	status = "okay";
+};
Signature · 1 line
2.11.0
Signature · 4 lines
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
#12 lore raw

PATCH 6/7 arm64: marvell: dts: enable the crypto engine on the Armada 8040 DB

In reply to Antoine Tenart

Enable the cryptographic engine available in the CP110 master on the Armada 8040 DB. Do not enable the one in the CP110 salve for now, as we do not support multiple cryptographic engines yet.

---
 arch/arm64/boot/dts/marvell/armada-8040-db.dts | 4 ++++
 1 file changed, 4 insertions(+)
arch/arm64/boot/dts/marvell/armada-8040-db.dts +4 -0
index 80e685e4235d..f6d808bced51 100644
@@ -140,6 +140,10 @@
 	phy-mode = "rgmii-id";
 };
 
+&cpm_crypto {
+	status = "okay";
+};
+
 /* CON5 on CP1 expansion */
 &cps_pcie2 {
 	status = "okay";
Signature · 1 line
2.11.0
#13 lore raw

PATCH 6/7 arm64: marvell: dts: enable the crypto engine on the Armada 8040 DB

Hi Antoine,

Quoted 5 lines · Antoine Tenart

Enable the cryptographic engine available in the CP110 master on the Armada 8040 DB. Do not enable the one in the CP110 salve for now, as we do not support multiple cryptographic engines yet.

Applied on mvebu/dt64

Thanks,

Gregory

Quoted 22 lines
---
 arch/arm64/boot/dts/marvell/armada-8040-db.dts | 4 ++++
 1 file changed, 4 insertions(+)
arch/arm64/boot/dts/marvell/armada-8040-db.dts +4 -0
index 80e685e4235d..f6d808bced51 100644
@@ -140,6 +140,10 @@
 	phy-mode = "rgmii-id";
 };
 
+&cpm_crypto {
+	status = "okay";
+};
+
 /* CON5 on CP1 expansion */
 &cps_pcie2 {
 	status = "okay";
Signature · 1 line
2.11.0
Signature · 4 lines
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
#14 lore raw

PATCH 7/7 arm64: defconfig: enable the Safexcel crypto engine as a module

In reply to Antoine Tenart

The Safexcel EIP197 cryptographic engine is used on some Marvell SoCs, such as Armada 7040 and Armada 8040. Enable this driver as a module in the ARM64 defconfig.

---
 arch/arm64/configs/defconfig | 1 +
 1 file changed, 1 insertion(+)
arch/arm64/configs/defconfig +1 -0
index fece4d3c2095..76308fa2fa7a 100644
@@ -519,6 +519,7 @@ CONFIG_MEMTEST=y
 CONFIG_SECURITY=y
 CONFIG_CRYPTO_ECHAINIV=y
 CONFIG_CRYPTO_ANSI_CPRNG=y
+CONFIG_CRYPTO_DEV_SAFEXCEL=m
 CONFIG_ARM64_CRYPTO=y
 CONFIG_CRYPTO_SHA1_ARM64_CE=y
 CONFIG_CRYPTO_SHA2_ARM64_CE=y
Signature · 1 line
2.11.0
#15 lore raw

PATCH 7/7 arm64: defconfig: enable the Safexcel crypto engine as a module

Hi Antoine,

Quoted 5 lines · Antoine Tenart

The Safexcel EIP197 cryptographic engine is used on some Marvell SoCs, such as Armada 7040 and Armada 8040. Enable this driver as a module in the ARM64 defconfig.

Applied on mvebu/defconfig64

Thanks,

Gregory

Quoted 19 lines
---
 arch/arm64/configs/defconfig | 1 +
 1 file changed, 1 insertion(+)
arch/arm64/configs/defconfig +1 -0
index fece4d3c2095..76308fa2fa7a 100644
@@ -519,6 +519,7 @@ CONFIG_MEMTEST=y
 CONFIG_SECURITY=y
 CONFIG_CRYPTO_ECHAINIV=y
 CONFIG_CRYPTO_ANSI_CPRNG=y
+CONFIG_CRYPTO_DEV_SAFEXCEL=m
 CONFIG_ARM64_CRYPTO=y
 CONFIG_CRYPTO_SHA1_ARM64_CE=y
 CONFIG_CRYPTO_SHA2_ARM64_CE=y
Signature · 1 line
2.11.0
Signature · 4 lines
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
maintainer
#16 lore raw

cover arm64: marvell: add cryptographic engine support for 7k/8k

In reply to Antoine Tenart
Quoted 26 lines · Antoine Tenart

Hi all,

This series adds support for the Inside Secure SafeXcel EIP197 cryptographic engine which can be found on Marvell Armada 7k and 8k boards. A new cryptographic engine driver is added, as well as the relevant device tree definition for the Armada 7040 DB and 8040 DB boards.

This driver needs two firmwares to work correctly. These firmware are usually used for more advanced operations than the ones supported (as of now), but we still need them to pass the data to the internal cryptographic engine.

This series was tested in various ways on both the Armada 7040 DB and the Armada 8040 DB: using the crypto framework self tests, using tcrypt and while performing various transfers with iperf on top of IPsec.

This series is based on top of v4.11-rc1, and is available on a branch (which also contains the PPv2 support for 7k/8k, to ease the process of testing IPsec): https://github.com/atenart/linux v4.11-rc1/7k8k-crypto I can rebase if needed.

Thanks,
Antoine

Quoted 9 lines

Antoine Tenart (7):
Documentation/bindings: Document the SafeXel cryptographic engine
driver
crypto: inside-secure: add SafeXcel EIP197 crypto engine driver
MAINTAINERS: add a maintainer for the Inside Secure crypto driver
arm64: marvell: dts: add crypto engine description for 7k/8k
arm64: marvell: dts: enable the crypto engine on the Armada 7040 DB
arm64: marvell: dts: enable the crypto engine on the Armada 8040 DB
arm64: defconfig: enable the Safexcel crypto engine as a module

I have no problems with the crypto bits.

Thanks,

Signature · 3 lines
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
#17 lore raw

cover arm64: marvell: add cryptographic engine support for 7k/8k

Hi Herbert,

Quoted 37 lines · Herbert Xu
Quoted 26 lines · Antoine Tenart

Hi all,

This series adds support for the Inside Secure SafeXcel EIP197 cryptographic engine which can be found on Marvell Armada 7k and 8k boards. A new cryptographic engine driver is added, as well as the relevant device tree definition for the Armada 7040 DB and 8040 DB boards.

This driver needs two firmwares to work correctly. These firmware are usually used for more advanced operations than the ones supported (as of now), but we still need them to pass the data to the internal cryptographic engine.

This series was tested in various ways on both the Armada 7040 DB and the Armada 8040 DB: using the crypto framework self tests, using tcrypt and while performing various transfers with iperf on top of IPsec.

This series is based on top of v4.11-rc1, and is available on a branch (which also contains the PPv2 support for 7k/8k, to ease the process of testing IPsec): https://github.com/atenart/linux v4.11-rc1/7k8k-crypto I can rebase if needed.

Thanks,
Antoine

Quoted 9 lines

Antoine Tenart (7):
Documentation/bindings: Document the SafeXel cryptographic engine
driver
crypto: inside-secure: add SafeXcel EIP197 crypto engine driver
MAINTAINERS: add a maintainer for the Inside Secure crypto driver
arm64: marvell: dts: add crypto engine description for 7k/8k
arm64: marvell: dts: enable the crypto engine on the Armada 7040 DB
arm64: marvell: dts: enable the crypto engine on the Armada 8040 DB
arm64: defconfig: enable the Safexcel crypto engine as a module

Quoted 1 line

I have no problems with the crypto bits.

Does it means that you agree that I apply the arm-soc related patches (from 4 to 7) to my mvebu trees?

Thanks,

Gregory

Quoted 6 lines

Thanks,

Signature · 3 lines
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Signature · 4 lines
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
maintainer
#18 lore raw

cover arm64: marvell: add cryptographic engine support for 7k/8k

Quoted 5 lines · Gregory CLEMENT
Quoted 2 lines

I have no problems with the crypto bits.

Does it means that you agree that I apply the arm-soc related patches (from 4 to 7) to my mvebu trees?

Yes sure.

Thanks,

Signature · 3 lines
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt