crypto: Add support for the Geode AES engine

Hey all -

I've had a poor excuse for a crypto driver for the Geode LX AES hardware sitting in my Geode GIT tree (and -mm) for some time now. I sent out the original version some time ago (June) and I got some very smart recommendations for changes, but other events transpired, and I didn't do a very good job of following up.

So now, in the interest of getting this out of everybody's hair and on the path to mainstreaming, I'm sending out a newly updated version of the driver for comments. This should be fully updated for all the new API changes in the crypto GIT tree. Once this is acceptable, I would like to get this pulled into the crypto tree.

Thanks,
Jordan

Signature · 4 lines

Jordan Crouse
Senior Linux Engineer
Advanced Micro Devices, Inc.
<www.amd.com/embeddedprocessors>

As far as I can see, register access is not protected, how can your driver handle the case when dm-crypt and IPsec simultaneously try to encrypt/decrypt some data, it can happen even around preemt_disable/enable calls and actually crypto processing can happen in interrupt context too (although it is not the best thing to do).

You added timeout for the broken hardware condition, I think it is better to return some error from _crypt() in that case, and, btw, that name is not very good choice.

Quoted 5 lines

As far as I can see, register access is not protected, how can your driver handle the case when dm-crypt and IPsec simultaneously try to encrypt/decrypt some data, it can happen even around preemt_disable/enable calls and actually crypto processing can happen in interrupt context too (although it is not the best thing to do).

I was sitting there trying to architect some grand scheme, and then it occured to me that we should just turn off the interrupts all together in the critical area. Its not optimal for performance, but it avoids lots of crazy if statements.

Quoted 3 lines

You added timeout for the broken hardware condition, I think it is better to return some error from _crypt() in that case, and, btw, that name is not very good choice.

Fixed the name and I return the error. I don't propagate it through the whole chain, because quite frankly, if the crypto engine fails, then its a good bet the processor is on fire. :)

...

drivers/crypto/Kconfig +0 -0

index adb5541..e816535 100644

drivers/crypto/Kconfig +10 -0

+	select CRYPTO_ALGAPI
+	select CRYPTO_BLKCIPHER
+	default m
+	help
+	  Say 'Y' here to use the AMD Geode LX processor on-board AES
+	  engine for the CryptoAPI AES alogrithm.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called geode-aes.
+
 endmenu
 

drivers/crypto/Kconfig +49 -0

+/* Static structures */
+
+static void __iomem * _iobase;
+
+/* Write a 128 bit field (either a writable key or IV) */
+static inline void
+_writefield(u32 offset, void *value)
+{
+	int i;
+	for(i = 0; i < 4; i++)
+		AWRITE(((u32 *) value)[i], offset + (i * 4));
+}
 
+/* Read a 128 bit field (either a writable key or IV) */
+static inline void
+_readfield(u32 offset, void *value)
+{
+	int i;
+	for(i = 0; i < 4; i++)
+		((u32 *) value)[i] = AREAD(offset + (i * 4));
+}
+
...
 
+unsigned int
+geode_aes_crypt(struct geode_aes_op *op)
+{
+	u32 flags = 0;
+	int iflags;
+
+	if (op->len == 0 || op->src == op->dst)
+		return 0;
+
+	if (op->flags & AES_FLAGS_COHERENT)
+		flags |= (AES_CTRL_DCA | AES_CTRL_SCA);
+
+	if (op->dir == AES_DIR_ENCRYPT)
+		flags |= AES_CTRL_ENCRYPT;
+
+	/* Start the critical section */
+
+	spin_lock_irqsave(&lock, iflags);
+
+	if (op->mode == AES_MODE_CBC) {
+		flags |= AES_CTRL_CBC;
+		SET_IV(op->iv);
+	}
+
+	if (op->flags & AES_FLAGS_USRKEY) {
+		flags |= AES_CTRL_WRKEY;
+		SET_KEY(op->key);
+	}

drivers/crypto/Kconfig +14 -0

+static int
+geode_setkey(struct crypto_tfm *tfm, const u8 *key, unsigned int len)
+{
+	struct geode_aes_op *op = ctx(tfm);
+
+	if (len != AES_KEY_LENGTH) {
+		tfm->crt_flags |= CRYPTO_TFM_RES_BAD_KEY_LEN;
+		return -EINVAL;
+	}
+
+	memcpy(op->key, key, len);
+	return 0;
+}
+

drivers/crypto/Kconfig +9 -0

+	op->mode = AES_MODE_ECB;
+	op->flags = 0;
+	op->len = AES_MIN_BLOCK_SIZE;
+	op->dir = AES_DIR_ENCRYPT;
+
+	geode_aes_crypt(op);
+}
+
+

drivers/crypto/Kconfig +5 -0

+static int
+geode_cbc_decrypt(struct blkcipher_desc *desc,
+		  struct scatterlist *dst, struct scatterlist *src,
+		  unsigned int nbytes)
+{

drivers/crypto/Kconfig +12 -0

+		op->mode = AES_MODE_CBC;
+		op->len = nbytes - (nbytes % AES_MIN_BLOCK_SIZE);
+		op->dir = AES_DIR_DECRYPT;
+
+		memcpy(op->iv, walk.iv, AES_IV_LENGTH);
+
+		ret = geode_aes_crypt(op);
+
+		memcpy(walk.iv, op->iv, AES_IV_LENGTH);
+		nbytes -= ret;
+
+		err = blkcipher_walk_done(desc, &walk, nbytes);

Does it depend on GEODE in some fashion too?

Quoted 16 lines

(quoted diff) +16 -0

+	spin_lock_irqsave(&lock, iflags);
+
+	if (op->mode == AES_MODE_CBC) {
+		flags |= AES_CTRL_CBC;
+		SET_IV(op->iv);
+	}
+
+	if (op->flags & AES_FLAGS_USRKEY) {
+		flags |= AES_CTRL_WRKEY;
+		SET_KEY(op->key);
+	}
+
+	do_crypt(op->src, op->dst, op->len, flags);
+
+	if (op->mode == AES_MODE_CBC)
+		GET_IV(op->iv);

Review

Quoted 5 lines

(quoted diff) +5 -0

+
+	spin_lock_irqrestore(&lock, iflags);
+
+	return op->len;
+}

Running do_crypt() under spin_lock_irqsave() seems a bit sad from a latency POV.

Add a driver to support the AES hardware on the Geode LX processor.

Quoted 1 line · On Thu, 28 Sep 2006 17:17:39 -0600 "Jordan Crouse" <[email protected]>…

Add a driver to support the AES hardware on the Geode LX processor.

So... what are you planning on doing with this? Ask Linus to pull that git tree, or what?

Changes made and new version follows. Thanks for the comments.

As far as I can see, register access is not protected, how can your driver handle the case when dm-crypt and IPsec simultaneously try to encrypt/decrypt some data, it can happen even around preemt_disable/enable calls and actually crypto processing can happen in interrupt context too (although it is not the best thing to do).

You added timeout for the broken hardware condition, I think it is better to return some error from _crypt() in that case, and, btw, that name is not very good choice.

Quoted 5 lines

As far as I can see, register access is not protected, how can your driver handle the case when dm-crypt and IPsec simultaneously try to encrypt/decrypt some data, it can happen even around preemt_disable/enable calls and actually crypto processing can happen in interrupt context too (although it is not the best thing to do).

I had the mutex in there, but I took it out based on our previous conversations, which probably was a little rash. If CRYPTO_TFM_REQ_MAY_SLEEP is still a valid flag to check, I could use that along with a spin lock of
some sort. I'll think about this a bit more.