drivers/crypto/nx: Add CRC and validation support for nx842
diff mbox

Message ID 1442707362.6178.11.camel@hbabu-laptop
State Changes Requested
Delegated to: Herbert Xu
Headers show

Commit Message

Haren Myneni Sept. 20, 2015, 12:02 a.m. UTC
Hi, 

This patch allows nx842 coprocessor to add CRC for compression and
check the computed CRC value for uncompression. Please let me know 
if you have any comments.

Thanks
Haren

commit d0b34d2e3ed41e7ec2afdbd654f0dd7716e4d4c0
Author: Haren Myneni <haren@us.ibm.com>
Date:   Sat Sep 12 01:20:51 2015 -0700

    crypto/nx842: Add CRC and validation support
    
    This patch adds CRC generation and validation support for nx-842. 
    Add CRC flag so that nx842 coprocessor includes CRC during 
    compression and validates during uncompression. 
    
    Signed-off-by: Haren Myneni <haren@us.ibm.com>

 		dev_dbg(dev, "%s: Bad data for decompression (code:%d)\n",
@@ -324,7 +327,7 @@ static int nx842_pseries_compress(const unsigned
char *in, unsigned int inlen,
 	slout.entries = (struct nx842_slentry *)workmem->slout;
 
 	/* Init operation */
-	op.flags = NX842_OP_COMPRESS;
+	op.flags = NX842_OP_COMPRESS_CRC;
 	csbcpb = &workmem->csbcpb;
 	memset(csbcpb, 0, sizeof(*csbcpb));
 	op.csbcpb = nx842_get_pa(csbcpb);
@@ -457,7 +460,7 @@ static int nx842_pseries_decompress(const unsigned
char *in, unsigned int inlen,
 	slout.entries = (struct nx842_slentry *)workmem->slout;
 
 	/* Init operation */
-	op.flags = NX842_OP_DECOMPRESS;
+	op.flags = NX842_OP_DECOMPRESS_CRC;
 	csbcpb = &workmem->csbcpb;
 	memset(csbcpb, 0, sizeof(*csbcpb));
 	op.csbcpb = nx842_get_pa(csbcpb);


--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Herbert Xu Sept. 21, 2015, 2:26 p.m. UTC | #1
On Sat, Sep 19, 2015 at 05:02:42PM -0700, Haren Myneni wrote:
> Hi, 
> 
> This patch allows nx842 coprocessor to add CRC for compression and
> check the computed CRC value for uncompression. Please let me know 
> if you have any comments.
> 
> Thanks
> Haren
> 
> commit d0b34d2e3ed41e7ec2afdbd654f0dd7716e4d4c0
> Author: Haren Myneni <haren@us.ibm.com>
> Date:   Sat Sep 12 01:20:51 2015 -0700
> 
>     crypto/nx842: Add CRC and validation support
>     
>     This patch adds CRC generation and validation support for nx-842. 
>     Add CRC flag so that nx842 coprocessor includes CRC during 
>     compression and validates during uncompression. 
>     
>     Signed-off-by: Haren Myneni <haren@us.ibm.com>

In future please post the patch without all the metadata.  You
can use the helper git format-patch to help you prepare the patch
for submission.

As to the CRC itself what is the purpose of this and wouldn't it
fail our test vectors if we had any? Remember that we also have a
software implementation now and the hardware should produce exactly
the same output as the software version.

Thanks,
Dan Streetman Sept. 21, 2015, 3:21 p.m. UTC | #2
On Mon, Sep 21, 2015 at 10:26 AM, Herbert Xu
<herbert@gondor.apana.org.au> wrote:
> On Sat, Sep 19, 2015 at 05:02:42PM -0700, Haren Myneni wrote:
>> Hi,
>>
>> This patch allows nx842 coprocessor to add CRC for compression and
>> check the computed CRC value for uncompression. Please let me know
>> if you have any comments.
>>
>> Thanks
>> Haren
>>
>> commit d0b34d2e3ed41e7ec2afdbd654f0dd7716e4d4c0
>> Author: Haren Myneni <haren@us.ibm.com>
>> Date:   Sat Sep 12 01:20:51 2015 -0700
>>
>>     crypto/nx842: Add CRC and validation support
>>
>>     This patch adds CRC generation and validation support for nx-842.
>>     Add CRC flag so that nx842 coprocessor includes CRC during
>>     compression and validates during uncompression.
>>
>>     Signed-off-by: Haren Myneni <haren@us.ibm.com>
>
> In future please post the patch without all the metadata.  You
> can use the helper git format-patch to help you prepare the patch
> for submission.
>
> As to the CRC itself what is the purpose of this and wouldn't it
> fail our test vectors if we had any? Remember that we also have a
> software implementation now and the hardware should produce exactly
> the same output as the software version.

As far as the hw and sw drivers producing the exact same output, I
don't think that's possible with the current hw and sw drivers,
because the hw driver may have to add a header to the actual byte
stream that the hw creates, depending on buffer alignment and size
(the hw has specific restrictions).  Currently, the sw driver doesn't
understand that header that the 842 hw driver creates, although that
could be added to the sw driver.  And, the hw driver skips adding the
header if the buffers are correctly aligned/sized, which would result
in a test vector failure if it doesn't align the buffer the same way
each time.

However, you're right that if the hw driver is doing crc checking, the
sw driver needs to be updated to make sure to add the crc to anything
it encodes, otherwise the hw driver will fail decompressing it; Haren,
you'll need to update the lib/842 code to also include crc
creation/checking.

Also, it might be a good time to add what we talked about a while ago,
to push the alignment/size restrictions into the crypto compression
layer, by adding cra_alignmask and cra_blocksize support to
crypto/compress.c.  Since the 842 hw has requirements not only for
specific alignment and min/max sizes, but also a requirement for
specific length multiple (i.e. must be !(len % 8)) it might be
worthwhile to also add a cra_sizemodulo or something like that.
However, if the common crypto alignment/size handling code allows any
alignment/size buffers (instead of just returning error for mis-sized
buffers), I think a common crypto header would need to be added in
cases of mis-sizing, which may not be appropriate for common code.
Alternately, the common crypto code could just return error for
mis-sized buffers; users of the crypto comp api would just have to
check crypto_tfm_alg_blocksize() before calling.

In case I haven't said it before, I really hate how the 842 hw
requires specific alignment and sizing.  How hard is it to add support
for any alignment/size in the hw?!?


>
> Thanks,
> --
> Email: Herbert Xu <herbert@gondor.apana.org.au>
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
> --
> To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Herbert Xu Sept. 22, 2015, 12:21 p.m. UTC | #3
On Mon, Sep 21, 2015 at 11:21:14AM -0400, Dan Streetman wrote:
> 
> As far as the hw and sw drivers producing the exact same output, I
> don't think that's possible with the current hw and sw drivers,
> because the hw driver may have to add a header to the actual byte
> stream that the hw creates, depending on buffer alignment and size
> (the hw has specific restrictions).  Currently, the sw driver doesn't
> understand that header that the 842 hw driver creates, although that
> could be added to the sw driver.  And, the hw driver skips adding the
> header if the buffers are correctly aligned/sized, which would result
> in a test vector failure if it doesn't align the buffer the same way
> each time.

I guess they don't have to be exactly the same.  As long as each
can take the output of the other and compress/decompress them it
should be fine.

> Also, it might be a good time to add what we talked about a while ago,
> to push the alignment/size restrictions into the crypto compression
> layer, by adding cra_alignmask and cra_blocksize support to
> crypto/compress.c.  Since the 842 hw has requirements not only for
> specific alignment and min/max sizes, but also a requirement for
> specific length multiple (i.e. must be !(len % 8)) it might be
> worthwhile to also add a cra_sizemodulo or something like that.
> However, if the common crypto alignment/size handling code allows any
> alignment/size buffers (instead of just returning error for mis-sized
> buffers), I think a common crypto header would need to be added in
> cases of mis-sizing, which may not be appropriate for common code.
> Alternately, the common crypto code could just return error for
> mis-sized buffers; users of the crypto comp api would just have to
> check crypto_tfm_alg_blocksize() before calling.

I'd like to see another hardware implementation before we start
moving this into the API.

> In case I haven't said it before, I really hate how the 842 hw
> requires specific alignment and sizing.  How hard is it to add support
> for any alignment/size in the hw?!?

Another option is to use a software fallback for the cases that
the hardware can't handle.

Cheers,
Dan Streetman Sept. 22, 2015, 2:39 p.m. UTC | #4
On Tue, Sep 22, 2015 at 8:21 AM, Herbert Xu <herbert@gondor.apana.org.au> wrote:
> On Mon, Sep 21, 2015 at 11:21:14AM -0400, Dan Streetman wrote:
>>
>> As far as the hw and sw drivers producing the exact same output, I
>> don't think that's possible with the current hw and sw drivers,
>> because the hw driver may have to add a header to the actual byte
>> stream that the hw creates, depending on buffer alignment and size
>> (the hw has specific restrictions).  Currently, the sw driver doesn't
>> understand that header that the 842 hw driver creates, although that
>> could be added to the sw driver.  And, the hw driver skips adding the
>> header if the buffers are correctly aligned/sized, which would result
>> in a test vector failure if it doesn't align the buffer the same way
>> each time.
>
> I guess they don't have to be exactly the same.  As long as each
> can take the output of the other and compress/decompress them it
> should be fine.

The hw driver code that handles the re-aligning and re-sizing, and
creating the header, is (mostly) isolated from the code that actually
talks to the NX 842 coprocessor, it's in drivers/crypto/nx/nx-842.c.

Haren, you could change the 842 sw driver to use that code to parse
compressed 842 buffers.  You'll have to modify the nx-842.c code some
though, as it expects to not only parse the header but also re-align
and re-size input and output buffers, which the sw driver doesn't need
to do, it can process buffers with any alignment and size.  Maybe you
could split out the header processing code into somewhere common, and
leave the code in 842-nx.c that actually re-aligns and re-sizes the
buffers.


>
>> Also, it might be a good time to add what we talked about a while ago,
>> to push the alignment/size restrictions into the crypto compression
>> layer, by adding cra_alignmask and cra_blocksize support to
>> crypto/compress.c.  Since the 842 hw has requirements not only for
>> specific alignment and min/max sizes, but also a requirement for
>> specific length multiple (i.e. must be !(len % 8)) it might be
>> worthwhile to also add a cra_sizemodulo or something like that.
>> However, if the common crypto alignment/size handling code allows any
>> alignment/size buffers (instead of just returning error for mis-sized
>> buffers), I think a common crypto header would need to be added in
>> cases of mis-sizing, which may not be appropriate for common code.
>> Alternately, the common crypto code could just return error for
>> mis-sized buffers; users of the crypto comp api would just have to
>> check crypto_tfm_alg_blocksize() before calling.
>
> I'd like to see another hardware implementation before we start
> moving this into the API.

ok.

>
>> In case I haven't said it before, I really hate how the 842 hw
>> requires specific alignment and sizing.  How hard is it to add support
>> for any alignment/size in the hw?!?
>
> Another option is to use a software fallback for the cases that
> the hardware can't handle.

hmm, that's true, and that would simplify the code a lot!  No need for
the header anymore.  But, since the sw driver is sooo much slower, it
would be important to be able to communicate the alignment and size
limits to the caller of the crypto compression api, via cra_alignmask
and cra_blocksize (for minimum size), as well as new fields for max
size (maybe cra_maxsize) and buffer length multiple - i.e. the buffer
length must be a multiple of N (maybe cra_blockmult or cra_blockmodulo
or something).  Otherwise callers would get unexpectedly slow
performance if they use the wrong alignment or size.

thanks!

>
> Cheers,
> --
> Email: Herbert Xu <herbert@gondor.apana.org.au>
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Herbert Xu Sept. 22, 2015, 2:42 p.m. UTC | #5
On Tue, Sep 22, 2015 at 10:39:53AM -0400, Dan Streetman wrote:
>
> hmm, that's true, and that would simplify the code a lot!  No need for
> the header anymore.  But, since the sw driver is sooo much slower, it
> would be important to be able to communicate the alignment and size
> limits to the caller of the crypto compression api, via cra_alignmask
> and cra_blocksize (for minimum size), as well as new fields for max
> size (maybe cra_maxsize) and buffer length multiple - i.e. the buffer
> length must be a multiple of N (maybe cra_blockmult or cra_blockmodulo
> or something).  Otherwise callers would get unexpectedly slow
> performance if they use the wrong alignment or size.

Right.  So who actually uses this?

Cheers,
Dan Streetman Sept. 22, 2015, 3:08 p.m. UTC | #6
On Tue, Sep 22, 2015 at 10:42 AM, Herbert Xu
<herbert@gondor.apana.org.au> wrote:
> On Tue, Sep 22, 2015 at 10:39:53AM -0400, Dan Streetman wrote:
>>
>> hmm, that's true, and that would simplify the code a lot!  No need for
>> the header anymore.  But, since the sw driver is sooo much slower, it
>> would be important to be able to communicate the alignment and size
>> limits to the caller of the crypto compression api, via cra_alignmask
>> and cra_blocksize (for minimum size), as well as new fields for max
>> size (maybe cra_maxsize) and buffer length multiple - i.e. the buffer
>> length must be a multiple of N (maybe cra_blockmult or cra_blockmodulo
>> or something).  Otherwise callers would get unexpectedly slow
>> performance if they use the wrong alignment or size.
>
> Right.  So who actually uses this?

currently, zswap.  Probably nobody else.  And zswap will always
provide an input buffer that is exactly one page, and output buffer
that is exactly 2 pages.  So, it will always meet the 842 hw
requirements.

Maybe I got a little too excited about adding flexibility to the 842
hw driver? ;-)

As long as zswap is the only user of the 842 hw driver, it could be
changed to simply return error for any mis-aligned or mis-sized
buffer.  A quick git grep of crypto_comp shows the only users of it
outside of crypto itself are ubifs and ipcomp.  ubifs uses only lzo
and deflate. ipcomp appears to use only deflate (it also lists lzs and
lzjh but i don't see those implemented anywhere).

you think we should just strip out the 842-nx alignment/sizing code
and change it to fallback to the sw driver?


>
> Cheers,
> --
> Email: Herbert Xu <herbert@gondor.apana.org.au>
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Herbert Xu Sept. 23, 2015, 9:54 a.m. UTC | #7
On Tue, Sep 22, 2015 at 11:08:22AM -0400, Dan Streetman wrote:
>
> you think we should just strip out the 842-nx alignment/sizing code
> and change it to fallback to the sw driver?

Right, if the only intended user can provide aligned input then
by all means make the unaligned case use the software fallback as
there is no point in investing any effort in making it fast.

Cheers,
Dan Streetman Sept. 24, 2015, 12:32 p.m. UTC | #8
On Wed, Sep 23, 2015 at 5:54 AM, Herbert Xu <herbert@gondor.apana.org.au> wrote:
> On Tue, Sep 22, 2015 at 11:08:22AM -0400, Dan Streetman wrote:
>>
>> you think we should just strip out the 842-nx alignment/sizing code
>> and change it to fallback to the sw driver?
>
> Right, if the only intended user can provide aligned input then
> by all means make the unaligned case use the software fallback as
> there is no point in investing any effort in making it fast.

ok sounds good.  thanks!

>
> Cheers,
> --
> Email: Herbert Xu <herbert@gondor.apana.org.au>
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch
diff mbox

diff --git a/drivers/crypto/nx/nx-842-powernv.c
b/drivers/crypto/nx/nx-842-powernv.c
index 3750e13..9ef51fa 100644
--- a/drivers/crypto/nx/nx-842-powernv.c
+++ b/drivers/crypto/nx/nx-842-powernv.c
@@ -491,7 +491,7 @@  static int nx842_powernv_compress(const unsigned
char *in, unsigned int inlen,
 				  void *wmem)
 {
 	return nx842_powernv_function(in, inlen, out, outlenp,
-				      wmem, CCW_FC_842_COMP_NOCRC);
+				      wmem, CCW_FC_842_COMP_CRC);
 }
 
 /**
@@ -519,7 +519,7 @@  static int nx842_powernv_decompress(const unsigned
char *in, unsigned int inlen,
 				    void *wmem)
 {
 	return nx842_powernv_function(in, inlen, out, outlenp,
-				      wmem, CCW_FC_842_DECOMP_NOCRC);
+				      wmem, CCW_FC_842_DECOMP_CRC);
 }
 
 static int __init nx842_powernv_probe(struct device_node *dn)
diff --git a/drivers/crypto/nx/nx-842-pseries.c
b/drivers/crypto/nx/nx-842-pseries.c
index f4cbde0..5532dab 100644
--- a/drivers/crypto/nx/nx-842-pseries.c
+++ b/drivers/crypto/nx/nx-842-pseries.c
@@ -234,6 +234,9 @@  static int nx842_validate_result(struct device *dev,
 		dev_dbg(dev, "%s: Out of space in output buffer\n",
 					__func__);
 		return -ENOSPC;
+	case 65: /* Calculated CRC doesn't match the passed value */
+		dev_dbg(dev, "%s: CRC mismatch for decompression\n", __func__);
+		return -EINVAL;
 	case 66: /* Input data contains an illegal template field */
 	case 67: /* Template indicates data past the end of the input stream
*/