HIFN 795x driver.

PATCH In review Evgeniy Polyakov 3 participants 14 messages Oct 2, 2007 – Oct 8, 2007 Crypto API

lore

Share & tools

lore

Evgeniy Polyakov

author

2007-10-02 15:01 UTC #0 lore raw

PATCH 1/1 HIFN 795x driver.

Hi.

This is a driver for HIFN 795x crypto accelerator chips.

A short changelog for this release:

* added des/3des support back
* added software queue
* changed watchdog initialization
* fixed CBC processing bug
* fixed debug and indent issues (upto 83 symbols per line)

DES currently does not pass tcrypt's multipage test, since HIFN adapter can not work with non-blocksize aligned chunks, although crypto alignmask is set to 3, tcrypt provides a block of size 2 in its multipage test, which fails.

Consider for inclusion.

Thanks.

Signed-off-by Evgeniy Polyakov <[email protected]>

drivers/crypto/Kconfig +9 -0

index c0fc4ae..7cfc26d 100644
@@ -88,4 +88,13 @@ config ZCRYPT_MONOLITHIC
 	  that contains all parts of the crypto device driver (ap bus,
 	  request router and all the card drivers).
 
+config CRYPTO_DEV_HIFN_795X
+	tristate "Driver HIFN 795x crypto accelerator chips"
+	select CRYPTO_ALGAPI
+	select CRYPTO_ABLKCIPHER
+	help
+	  This option allows you to have support for HIFN 795x crypto adapters.
+
+
+
 endif # CRYPTO_HW

drivers/crypto/Makefile +1 -0

index d070030..c0327f0 100644
@@ -1,3 +1,4 @@
 obj-$(CONFIG_CRYPTO_DEV_PADLOCK_AES) += padlock-aes.o
 obj-$(CONFIG_CRYPTO_DEV_PADLOCK_SHA) += padlock-sha.o
 obj-$(CONFIG_CRYPTO_DEV_GEODE) += geode-aes.o
+obj-$(CONFIG_CRYPTO_DEV_HIFN_795X) += hifn_795x.o

drivers/crypto/hifn_795x.c new file +2341 -0

2,341 lines not on this page · raw message

Signature · 1 line

	Evgeniy Polyakov

Sebastian Siewior

2007-10-02 19:05 UTC #1 lore raw

Quoted 4 lines · * Evgeniy Polyakov | 2007-10-02 19:00:13 [+0400]:

DES currently does not pass tcrypt's multipage test, since HIFN adapter can not work with non-blocksize aligned chunks, although crypto alignmask is set to 3, tcrypt provides a block of size 2 in its multipage test, which fails.

I recalled now that I forgot to point this out. You set the align mask to the block size of the algorithm but it is almost unused / useless. Let me explain:
- it is ensured that the key you get (in the setkey function) is properly aligned. This doesn't matter in your case because you memcpy() the key away to your private struct.
- your private struct is pointer aligned (if I remember correctly). You memcpy() your key away so it looks like you don't need the ctx aligned according to your mask (but it would be possible :))
- the proper alignment of src, dst and the IV that you get is _not_ ensured. This would be the case if you would develop a blkcipher and use blkcipher_walk_init(), blkcipher_walk_virt(), ....

Now. If you pass your AES-tcrypt-test (where you need 16 bytes alignment) than this is pure luck. If you really need aligned src,dst or the IV than you have to do it by yourself :(

The alignment for blkcipher (crypto/blkcipher.c) is enforced via
- blkcipher_walk_first() by calling blkcipher_copy_iv() for the IV
- blkcipher_walk_next() (and later) for the src/dst addr

Herbert: maybe we should remove the alignment in tcrypt.h (and make it unaligned on purpose) so we can test if the crypto API is enforcing the required alignment properly as well as the HW drivers.

Sebastian

Evgeniy Polyakov

author

2007-10-03 08:42 UTC #2 lore raw

Quoted 17 lines · Sebastian Siewior

Quoted 4 lines
DES currently does not pass tcrypt's multipage test, since HIFN adapter can not work with non-blocksize aligned chunks, although crypto alignmask is set to 3, tcrypt provides a block of size 2 in its multipage test, which fails.
I recalled now that I forgot to point this out. You set the align mask to the block size of the algorithm but it is almost unused / useless. Let me explain:
- it is ensured that the key you get (in the setkey function) is properly aligned. This doesn't matter in your case because you memcpy() the key away to your private struct.
- your private struct is pointer aligned (if I remember correctly). You memcpy() your key away so it looks like you don't need the ctx aligned according to your mask (but it would be possible :))
- the proper alignment of src, dst and the IV that you get is _not_ ensured. This would be the case if you would develop a blkcipher and use blkcipher_walk_init(), blkcipher_walk_virt(), ....

Quoted 3 lines

Now. If you pass your AES-tcrypt-test (where you need 16 bytes alignment) than this is pure luck. If you really need aligned src,dst or the IV than you have to do it by yourself :(

It has to be multiple of blocksize. I.e. it is impossible to crypt one byte - hardware will stall, DES test provides two bytes as input - this will not work. If that is going to be handled in driver, then it will relocate. I'm not sure it is the right decision.

Signature · 1 line

	Evgeniy Polyakov

Herbert Xu

maintainer

2007-10-03 11:54 UTC #3 lore raw

Hi Evgeniy:

Quoted 5 lines · Evgeniy Polyakov

It has to be multiple of blocksize. I.e. it is impossible to crypt one byte - hardware will stall, DES test provides two bytes as input - this will not work. If that is going to be handled in driver, then it will relocate. I'm not sure it is the right decision.

Right. However you can't force the user to give you data that's always layed out in blocks. For example, an IP fragment might give you an sg element that's only half an AES block.

You should either use the blkcipher walking helpers to avoid this or do your own copying when you detect that you have a partial block in an sg element.

Also, the MAY_BACKLOG option must be implemented or dm-crypt won't work properly.

Thanks,

Signature · 4 lines

Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Evgeniy Polyakov

author

2007-10-03 12:05 UTC #4 lore raw

Hi Herbert.

Quoted 5 lines · Herbert Xu

Quoted 4 lines
It has to be multiple of blocksize. I.e. it is impossible to crypt one byte - hardware will stall, DES test provides two bytes as input - this will not work. If that is going to be handled in driver, then it will relocate. I'm not sure it is the right decision.

Quoted 7 lines

Right. However you can't force the user to give you data that's always layed out in blocks. For example, an IP fragment might give you an sg element that's only half an AES block.
You should either use the blkcipher walking helpers to avoid this or do your own copying when you detect that you have a partial block in an sg element.

Ok, I will think more about this issues and ways to resolve the problem.

Quoted 2 lines

Also, the MAY_BACKLOG option must be implemented or dm-crypt won't work properly.

What is may backlog option? I did not find it in cryptd.c, which I used for reference. I found a backlog variables in the driver, but without any signs for the outside world - queue is initialized and backlog is being set to that queue - since there is a dequeueing code, what is a purpose for backlog in that case?

Signature · 1 line

	Evgeniy Polyakov

Herbert Xu

maintainer

2007-10-04 05:49 UTC #5 lore raw

Quoted 6 lines · Evgeniy Polyakov

What is may backlog option? I did not find it in cryptd.c, which I used for reference. I found a backlog variables in the driver, but without any signs for the outside world - queue is initialized and backlog is being set to that queue - since there is a dequeueing code, what is a purpose for backlog in that case?

It's not directly in cryptd because it uses the helpers from algapi.c. In particular ablkcipher_enqueue_request has the necessary logic to handle this correctly.

You could either use that helper yourself once your hardware queue fills up, or implement your own backlog logic. The key is each tfm object must be guaranteed to be able to queue at least one request.

Cheers,

Signature · 4 lines

Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Evgeniy Polyakov

author

2007-10-04 10:10 UTC #6 lore raw

Quoted 6 lines · Herbert Xu

Quoted 5 lines
What is may backlog option? I did not find it in cryptd.c, which I used for reference. I found a backlog variables in the driver, but without any signs for the outside world - queue is initialized and backlog is being set to that queue - since there is a dequeueing code, what is a purpose for backlog in that case?

Quoted 8 lines

It's not directly in cryptd because it uses the helpers from algapi.c. In particular ablkcipher_enqueue_request has the necessary logic to handle this correctly.
You could either use that helper yourself once your hardware queue fills up, or implement your own backlog logic. The key is each tfm object must be guaranteed to be able to queue at least one request.

I use ablkcipher_enqueue_request() when hardware can not handle new request, so this should be ok.

Signature · 1 line

	Evgeniy Polyakov

Herbert Xu

maintainer

2007-10-04 10:04 UTC #7 lore raw

Quoted 3 lines · Evgeniy Polyakov

I use ablkcipher_enqueue_request() when hardware can not handle new request, so this should be ok.

Good point. I missed that. So there is no problem with the backlog case.

But we still need to fix the partial block case.

Thanks,

Signature · 4 lines

Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Evgeniy Polyakov

author

2007-10-04 15:05 UTC #8 lore raw

Hi.

Quoted 1 line · Herbert Xu

But we still need to fix the partial block case.

Just a note: blkcipher_walk_* was not designed for async hardware, since there is no way to know in advance how many pages will be required for single request. For example DES second multipage test says it has three pages, but actually it will have 4 requests and thus 4 DMA descriptors in HIFN, but when new interrupt says about completed descriptor, it can not know if it is the last descriptor or not, so that callback for request should or should not be called. I've fixed that with additional atomic ops...

Given that, new driver passes all except one DES tcrypt tests, it fails on cbc multipage encryption (decryption works ok), which provides 13 bytes of misaligned data.

Here is a code snippet for current driver (error handling was ommitted):

	blkcipher_walk_init(&walk, req->dst, req->src, nbytes);
	err = blkcipher_walk_phys(&desc, &walk);

	while ((nbytes = walk.nbytes)) {
		spage = walk.src.phys.page;
		soff = walk.src.phys.offset;

		dpage = walk.dst.phys.page;
		doff = walk.dst.phys.offset;
		
		ctx->iv = walk.iv;

		++i;

		spin_lock_irqsave(&dev->lock, flags);
		err = hifn_setup_dma(dev, spage, soff, dpage, doff, nbytes,
				req, ctx);
		spin_unlock_irqrestore(&dev->lock, flags);

		nbytes &= blocksize - 1;
		err = blkcipher_walk_done(&desc, &walk, nbytes);
	}

And debug output:
Testing cbc(des) encryption across pages (chunking)
test 1 (64 bit key):
hifn0: spage: ffff81003fe8f1e8, soffset: 0, dpage: ffff81003fe8f1e8, doffset: 0, nbytes: 13, priv: ffff81003dc74138, ctx: ffff81003a9d7210.
hifn0: spage: ffff81003fe8f1e8, soffset: 8, dpage: ffff81003fe8f1e8, doffset: 0, nbytes: 8, priv: ffff81003dc74138, ctx: ffff81003a9d7210.
hifn0: spage: ffff81003fe8f1e8, soffset: 0, dpage: ffff81003fe8f1e8, doffset: 0, nbytes: 8, priv: ffff81003dc74138, ctx: ffff81003a9d7210. page 0
ccd173ffab2039f47f81654121
fail
page 1
dbd4cf266da007deeadb88
fail

And actually there is a huge problem with blkcipher_walk* interface - it always provides the same destination buffer, so it is possible to overwrite it if second crypto block was encrypted before the first one.

It just can not be used in asynchronous hardware, so I will create soemthing new for HIFN driver specially.

Signature · 1 line

	Evgeniy Polyakov

Herbert Xu

maintainer

2007-10-05 08:19 UTC #9 lore raw

Quoted 3 lines · Evgeniy Polyakov

It just can not be used in asynchronous hardware, so I will create soemthing new for HIFN driver specially.

That sounds good. We can put it into ablkcipher.c and then get rid of the phys stuff from blkcipher.c.

Cheers,

Signature · 4 lines

Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Sebastian Siewior

2007-10-06 21:59 UTC #10 lore raw

In reply to Evgeniy Polyakov

drivers/crypto/hifn_795x.c line 2341

Quoted 3 lines · * Evgeniy Polyakov | 2007-10-02 19:00:13 [+0400]:

drivers/crypto/hifn_795x.c new file +3 -0

+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Evgeniy Polyakov <[email protected]>");
+MODULE_DESCRIPTION("Driver for HIFN 795x crypto accelerator chip.");

Please also add MODULE_ALIAS for algos (that was aes, des and 3des as far as I remember).

Sebastian

Herbert Xu

maintainer

2007-10-08 03:15 UTC #11 lore raw

Quoted 8 lines · Sebastian Siewior

drivers/crypto/hifn_795x.c line 2341
Quoted 3 lines · * Evgeniy Polyakov | 2007-10-02 19:00:13 [+0400]:
drivers/crypto/hifn_795x.c new file +3 -0
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Evgeniy Polyakov <[email protected]>");
+MODULE_DESCRIPTION("Driver for HIFN 795x crypto accelerator chip.");
Please also add MODULE_ALIAS for algos (that was aes, des and 3des as far as I remember).

Actually this one should register aliases of "cbc(aes)", etc. as it doesn't implement plain old AES.

Cheers,

Signature · 4 lines

Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Sebastian Siewior

2007-10-08 15:49 UTC #12 lore raw

Quoted 10 lines · * Herbert Xu | 2007-10-08 11:15:10 [+0800]:

Quoted 8 lines · Sebastian Siewior
drivers/crypto/hifn_795x.c line 2341
Quoted 3 lines · * Evgeniy Polyakov | 2007-10-02 19:00:13 [+0400]:
drivers/crypto/hifn_795x.c new file +3 -0
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Evgeniy Polyakov <[email protected]>");
+MODULE_DESCRIPTION("Driver for HIFN 795x crypto accelerator chip.");
Please also add MODULE_ALIAS for algos (that was aes, des and 3des as far as I remember).

Quoted 2 lines

Actually this one should register aliases of "cbc(aes)", etc. as it doesn't implement plain old AES.

After thinking I realized that he does not need to provide an alias because his driver should be auto loaded due to the PCI-IDs right?

But while you mention it: Should the alias cbc(aes), etc. also be provided by the s390 and padlock driver which don't have PCI-IDs? Currently they have the aes alias (what they also provide) so it might be enough.

Quoted 1 line

Cheers,

Sebastian

Herbert Xu

maintainer

2007-10-08 16:07 UTC #13 lore raw

Quoted 3 lines · Sebastian Siewior

After thinking I realized that he does not need to provide an alias because his driver should be auto loaded due to the PCI-IDs right?

Good point.

Quoted 4 lines

But while you mention it: Should the alias cbc(aes), etc. also be provided by the s390 and padlock driver which don't have PCI-IDs? Currently they have the aes alias (what they also provide) so it might be enough.

Because they do actually provide the aes algorithm it should be sufficient.

Cheers,

Signature · 4 lines

Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt