All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCHv3 06/10]crypto: add rocksoft 64b crc framework
@ 2022-05-08  0:01 Xiaoming Zhou
  2022-05-09 20:37 ` Eric Biggers
  0 siblings, 1 reply; 9+ messages in thread
From: Xiaoming Zhou @ 2022-05-08  0:01 UTC (permalink / raw)
  To: kbusch
  Cc: linux-nvme, linux-block, linux-crypto, x86, linux-kernel, axboe,
	hch, martin.petersen, colyli

Hi Keith,
For the polynomial you used in this path is 0x9A6C9329AC4BC9B5ULL,  why it is different than the 0xAD93D23594C93659ULL defined in NVMe command set spec 5.2.1.3.4 ? Though the crc66 implemented in this patch can pass with test cases defined in Figure 121: 64b CRC Test Cases for 4KiB Logical Block with no Metadata.  Could you explain the discrepancy between the spec and the patch? 

Thanks,
Xiaoming

-----Original Message-----
From: Linux-nvme <linux-nvme-bounces@lists.infradead.org> On Behalf Of linux-nvme-request@lists.infradead.org
Sent: Tuesday, February 22, 2022 12:00 PM
To: linux-nvme@lists.infradead.org
Subject: [EXT] Linux-nvme Digest, Vol 95, Issue 129

External Email

----------------------------------------------------------------------
Send Linux-nvme mailing list submissions to
	linux-nvme@lists.infradead.org

To subscribe or unsubscribe via the World Wide Web, visit
	https://urldefense.proofpoint.com/v2/url?u=http-3A__lists.infradead.org_mailman_listinfo_linux-2Dnvme&d=DwICAg&c=nKjWec2b6R0mOyPaz7xtfQ&r=i2QEdWf3G3LCjxQ2FY9geCmDX5uXEL6k-yI1XpluRMU&m=aO_Z5Vo-ZK5JNnkryiz1SPDNY97daT4vk4ofnRS3deDGItjh6hmpsmYhvngO8oj7&s=flu55Dn1W8d8Kpb07ZIOf9EiWvJEacADVUk_k10Fj8w&e=
or, via email, send a message with subject or body 'help' to
	linux-nvme-request@lists.infradead.org

You can reach the person managing the list at
	linux-nvme-owner@lists.infradead.org

When replying, please edit your Subject line so it is more specific than "Re: Contents of Linux-nvme digest..."


Today's Topics:

   1. Re: [GIT PULL] nvme fixes for Linux 5.17 (Christoph Hellwig)
   2. Re: [PATCHv3 04/10] linux/kernel: introduce lower_48_bits
      macro (Joe Perches)
   3. Re: [PATCHv3 06/10] crypto: add rocksoft 64b crc framework
      (Eric Biggers)
   4. Re: [PATCHv3 06/10] crypto: add rocksoft 64b crc framework
      (Eric Biggers)
   5. Re: [PATCHv3 06/10] crypto: add rocksoft 64b crc framework
      (Eric Biggers)


----------------------------------------------------------------------

Message: 1
Date: Tue, 22 Feb 2022 09:29:34 -0800
From: Christoph Hellwig <hch@infradead.org>
To: Jens Axboe <axboe@kernel.dk>
Cc: Christoph Hellwig <hch@infradead.org>, Keith Busch
	<kbusch@kernel.org>, linux-block@vger.kernel.org, Sagi Grimberg
	<sagi@grimberg.me>, linux-nvme@lists.infradead.org
Subject: Re: [GIT PULL] nvme fixes for Linux 5.17
Message-ID: <YhUdfqXy0wCDQywm@infradead.org>
Content-Type: text/plain; charset=us-ascii

On Tue, Feb 22, 2022 at 10:26:16AM -0700, Jens Axboe wrote:
> Hmm?
> 
> https://urldefense.proofpoint.com/v2/url?u=https-3A__git.kernel.dk_cgi
> t_linux-2Dblock_commit_-3Fh-3Dblock-2D5.17-26id-3D93e2c52d71a6067d08ee
> 927e2682e9781cb911ef&d=DwICAg&c=nKjWec2b6R0mOyPaz7xtfQ&r=i2QEdWf3G3LCj
> xQ2FY9geCmDX5uXEL6k-yI1XpluRMU&m=aO_Z5Vo-ZK5JNnkryiz1SPDNY97daT4vk4ofn
> RS3deDGItjh6hmpsmYhvngO8oj7&s=vE-IaGHIXqznhuUOWrva610L8_iwmV2_3jo301Ps
> eEY&e=

Indeed.  I somehow had a stale block-5.17 branch locally.



------------------------------

Message: 2
Date: Tue, 22 Feb 2022 10:43:21 -0800
From: Joe Perches <joe@perches.com>
To: Keith Busch <kbusch@kernel.org>, Christoph Hellwig <hch@lst.de>
Cc: linux-nvme@lists.infradead.org, linux-block@vger.kernel.org,
	linux-crypto@vger.kernel.org, x86@kernel.org,
	linux-kernel@vger.kernel.org, axboe@kernel.dk,
	martin.petersen@oracle.com, colyli@suse.de, Bart Van Assche
	<bvanassche@acm.org>
Subject: Re: [PATCHv3 04/10] linux/kernel: introduce lower_48_bits
	macro
Message-ID:
	<603f9243bb9e1c4c50aaec83a527266b48ab9e20.camel@perches.com>
Content-Type: text/plain; charset="ISO-8859-1"

On Tue, 2022-02-22 at 08:56 -0800, Keith Busch wrote:
> On Tue, Feb 22, 2022 at 05:50:45PM +0100, Christoph Hellwig wrote:
> > On Tue, Feb 22, 2022 at 08:45:53AM -0800, Joe Perches wrote:
> > > On Tue, 2022-02-22 at 08:31 -0800, Keith Busch wrote:
> > > > +/ *
> > > > + * lower_48_bits - return bits 0-47 of a number
> > > > + * @n: the number we're accessing  */ #define lower_48_bits(n) 
> > > > +((u64)((n) & 0xffffffffffffull))
> > > 
> > > why not make this a static inline function?
> > 
> > Agreed.
> 
> Sure, that sounds good to me. I only did it this way to match the 
> existing local convention, but I personally prefer the inline function 
> too.

The existing convention is used there to allow the compiler to avoid warnings and unnecessary conversions of a u32 to a u64 when shifting by 32 or more bits.

If it's possible to be used with an architecture dependent typedef like dma_addr_t, then perhaps it's reasonable to do something like:

#define lower_48_bits(val)					\
({								\
	typeof(val) high = lower_16_bits(upper_32_bits(val));	\
	typeof(val) low = lower_32_bits(val);			\
								\
	(high << 16 << 16) | low;				\
})

and have the compiler have the return value be an appropriate type.





------------------------------

Message: 3
Date: Tue, 22 Feb 2022 11:50:42 -0800
From: Eric Biggers <ebiggers@kernel.org>
To: Keith Busch <kbusch@kernel.org>
Cc: linux-nvme@lists.infradead.org, linux-block@vger.kernel.org,
	linux-crypto@vger.kernel.org, x86@kernel.org,
	linux-kernel@vger.kernel.org, axboe@kernel.dk, hch@lst.de,
	martin.petersen@oracle.com, colyli@suse.de
Subject: Re: [PATCHv3 06/10] crypto: add rocksoft 64b crc framework
Message-ID: <YhU+kuMhueXVQvxe@sol.localdomain>
Content-Type: text/plain; charset=us-ascii

On Tue, Feb 22, 2022 at 08:31:40AM -0800, Keith Busch wrote:
> +config CRYPTO_CRC64_ROCKSOFT
> +	tristate "Rocksoft Model CRC64 algorithm"
> +	depends on CRC64
> +	select CRYPTO_HASH
> +	help
> +	  Rocksoft Model CRC64 computation is being cast as a crypto
> +	  transform. This allows for faster crc64 transforms to be used
> +	  if they are available.

The first sentence of this help text doesn't make sense.

> diff --git a/crypto/crc64_rocksoft_generic.c 
> b/crypto/crc64_rocksoft_generic.c new file mode 100644 index 
> 000000000000..55bad1939614
> --- /dev/null
> +++ b/crypto/crc64_rocksoft_generic.c
> @@ -0,0 +1,104 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Cryptographic API.

The "Cryptographic API" line doesn't provide any helpful information.

> +static int chksum_final(struct shash_desc *desc, u8 *out) {
> +	struct chksum_desc_ctx *ctx = shash_desc_ctx(desc);
> +
> +	*(u64 *)out = ctx->crc;
> +	return 0;
> +}
> +
> +static int __chksum_finup(u64 crc, const u8 *data, unsigned int len, 
> +u8 *out) {
> +	*(u64 *)out = crc64_rocksoft_generic(crc, data, len);
> +	return 0;
> +}

These 64-bit writes violate alignment rules and will give the wrong result on big endian CPUs.  They need to use put_unaligned_le64().

> +static int __init crc64_rocksoft_x86_mod_init(void) {
> +	return crypto_register_shash(&alg);
> +}
> +
> +static void __exit crc64_rocksoft_x86_mod_fini(void) {
> +	crypto_unregister_shash(&alg);
> +}

This has nothing to do with x86.

> +config CRC64_ROCKSOFT
> +	tristate "CRC calculation for the Rocksoft^TM model CRC64"

I'm sure what the rules for trademarks are, but kernel source code usually doesn't have the trademark symbol/abbreviation scattered everywhere.

> +	select CRYPTO
> +	select CRYPTO_CRC64_ROCKSOFT
> +	help
> +	  This option is only needed if a module that's not in the
> +	  kernel tree needs to calculate CRC checks for use with the
> +	  rocksoft model parameters.

Out-of-tree modules can't be the reason to have a kconfig option.  What is the real reason?

> +u64 crc64_rocksoft(const unsigned char *buffer, size_t len) {
> +	return crc64_rocksoft_update(~0ULL, buffer, len); } 
> +EXPORT_SYMBOL(crc64_rocksoft);

Isn't this missing the bitwise inversion at the end?

> +MODULE_AUTHOR("Keith Busch <kbusch@kernel.org>"); 
> +MODULE_DESCRIPTION("Rocksoft model CRC64 calculation (library API)"); 
> +MODULE_LICENSE("GPL");
> +MODULE_SOFTDEP("pre: crc64");

Shouldn't the MODULE_SOFTDEP be on crc64-rocksoft?

- Eric



------------------------------

Message: 4
Date: Tue, 22 Feb 2022 11:54:31 -0800
From: Eric Biggers <ebiggers@kernel.org>
To: Keith Busch <kbusch@kernel.org>
Cc: linux-nvme@lists.infradead.org, linux-block@vger.kernel.org,
	linux-crypto@vger.kernel.org, x86@kernel.org,
	linux-kernel@vger.kernel.org, axboe@kernel.dk, hch@lst.de,
	martin.petersen@oracle.com, colyli@suse.de
Subject: Re: [PATCHv3 06/10] crypto: add rocksoft 64b crc framework
Message-ID: <YhU/d6wn55/GWPxm@sol.localdomain>
Content-Type: text/plain; charset=us-ascii

On Tue, Feb 22, 2022 at 11:50:44AM -0800, Eric Biggers wrote:
> > +config CRC64_ROCKSOFT
> > +	tristate "CRC calculation for the Rocksoft^TM model CRC64"
> 
> I'm sure what the rules for trademarks are, but kernel source code 
> usually doesn't have the trademark symbol/abbreviation scattered everywhere.
> 
> > +	select CRYPTO
> > +	select CRYPTO_CRC64_ROCKSOFT
> > +	help
> > +	  This option is only needed if a module that's not in the
> > +	  kernel tree needs to calculate CRC checks for use with the
> > +	  rocksoft model parameters.
> 
> Out-of-tree modules can't be the reason to have a kconfig option.  
> What is the real reason?

Also this option can be enabled without the CONFIG_CRC64 it depends on, which is broken.

- Eric



------------------------------

Message: 5
Date: Tue, 22 Feb 2022 11:56:59 -0800
From: Eric Biggers <ebiggers@kernel.org>
To: Keith Busch <kbusch@kernel.org>
Cc: linux-nvme@lists.infradead.org, linux-block@vger.kernel.org,
	linux-crypto@vger.kernel.org, x86@kernel.org,
	linux-kernel@vger.kernel.org, axboe@kernel.dk, hch@lst.de,
	martin.petersen@oracle.com, colyli@suse.de
Subject: Re: [PATCHv3 06/10] crypto: add rocksoft 64b crc framework
Message-ID: <YhVACzTEylUg5LJx@sol.localdomain>
Content-Type: text/plain; charset=us-ascii

On Tue, Feb 22, 2022 at 08:31:40AM -0800, Keith Busch wrote:
> Hardware specific features may be able to calculate a crc64, so 
> provide a framework for drivers to register their implementation. If 
> nothing is registered, fallback to the generic table lookup 
> implementation. The implementation is modeled after the crct10dif equivalent.
> 
> Signed-off-by: Keith Busch <kbusch@kernel.org>
> ---
>  crypto/Kconfig                  |   9 +++
>  crypto/Makefile                 |   1 +
>  crypto/crc64_rocksoft_generic.c | 104 +++++++++++++++++++++++++
>  include/linux/crc64.h           |   5 ++
>  lib/Kconfig                     |   9 +++
>  lib/Makefile                    |   1 +
>  lib/crc64-rocksoft.c            | 129 ++++++++++++++++++++++++++++++++
>  7 files changed, 258 insertions(+)
>  create mode 100644 crypto/crc64_rocksoft_generic.c  create mode 
> 100644 lib/crc64-rocksoft.c

I tried testing this, but I can't because it is missing a self-test:

[    0.736340] alg: No test for crc64-rocksoft (crc64-rocksoft-generic)
[    5.440398] alg: No test for crc64-rocksoft (crc64-rocksoft-pclmul)

All algorithms registered with the crypto API need to have a self-test (in crypto/testmgr.c).

- Eric



------------------------------

Subject: Digest Footer

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
https://urldefense.proofpoint.com/v2/url?u=http-3A__lists.infradead.org_mailman_listinfo_linux-2Dnvme&d=DwICAg&c=nKjWec2b6R0mOyPaz7xtfQ&r=i2QEdWf3G3LCjxQ2FY9geCmDX5uXEL6k-yI1XpluRMU&m=aO_Z5Vo-ZK5JNnkryiz1SPDNY97daT4vk4ofnRS3deDGItjh6hmpsmYhvngO8oj7&s=flu55Dn1W8d8Kpb07ZIOf9EiWvJEacADVUk_k10Fj8w&e= 


------------------------------

End of Linux-nvme Digest, Vol 95, Issue 129
*******************************************

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCHv3 06/10]crypto: add rocksoft 64b crc framework
  2022-05-08  0:01 [PATCHv3 06/10]crypto: add rocksoft 64b crc framework Xiaoming Zhou
@ 2022-05-09 20:37 ` Eric Biggers
  2022-05-09 21:29   ` [EXT] " Xiaoming Zhou
  0 siblings, 1 reply; 9+ messages in thread
From: Eric Biggers @ 2022-05-09 20:37 UTC (permalink / raw)
  To: Xiaoming Zhou
  Cc: kbusch, linux-nvme, linux-block, linux-crypto, x86, linux-kernel,
	axboe, hch, martin.petersen, colyli

On Sun, May 08, 2022 at 12:01:21AM +0000, Xiaoming Zhou wrote:
> Hi Keith,
> For the polynomial you used in this path is 0x9A6C9329AC4BC9B5ULL,  why it is
> different than the 0xAD93D23594C93659ULL defined in NVMe command set spec
> 5.2.1.3.4 ? Though the crc66 implemented in this patch can pass with test
> cases defined in Figure 121: 64b CRC Test Cases for 4KiB Logical Block with no
> Metadata.  Could you explain the discrepancy between the spec and the patch? 
> 

0x9A6C9329AC4BC9B5 is 0xAD93D23594C93659 with its bits reversed.

0xAD93D23594C93659 maps the polynomial coefficients to bits in the natural way.
However, writing the polynomial in this way isn't useful for this CRC variant,
as it is a bit-reversed CRC.

- Eric

^ permalink raw reply	[flat|nested] 9+ messages in thread

* RE: [EXT] Re: [PATCHv3 06/10]crypto: add rocksoft 64b crc framework
  2022-05-09 20:37 ` Eric Biggers
@ 2022-05-09 21:29   ` Xiaoming Zhou
  0 siblings, 0 replies; 9+ messages in thread
From: Xiaoming Zhou @ 2022-05-09 21:29 UTC (permalink / raw)
  To: Eric Biggers
  Cc: kbusch, linux-nvme, linux-block, linux-crypto, x86, linux-kernel,
	axboe, hch, martin.petersen, colyli

Agree.  The "11199E50_6128D175h" of 64b CRC Check in the Spec also is the AE8B1486_0A799888h with bits reversed.

Regards,
Xiaoming

-----Original Message-----
From: Eric Biggers <ebiggers@kernel.org> 
Sent: Monday, May 9, 2022 1:37 PM
To: Xiaoming Zhou <xzhou@marvell.com>
Cc: kbusch@kernel.org; linux-nvme@lists.infradead.org; linux-block@vger.kernel.org; linux-crypto@vger.kernel.org; x86@kernel.org; linux-kernel@vger.kernel.org; axboe@kernel.dk; hch@lst.de; martin.petersen@oracle.com; colyli@suse.de
Subject: [EXT] Re: [PATCHv3 06/10]crypto: add rocksoft 64b crc framework

External Email

----------------------------------------------------------------------
On Sun, May 08, 2022 at 12:01:21AM +0000, Xiaoming Zhou wrote:
> Hi Keith,
> For the polynomial you used in this path is 0x9A6C9329AC4BC9B5ULL,  
> why it is different than the 0xAD93D23594C93659ULL defined in NVMe 
> command set spec
> 5.2.1.3.4 ? Though the crc66 implemented in this patch can pass with 
> test cases defined in Figure 121: 64b CRC Test Cases for 4KiB Logical 
> Block with no Metadata.  Could you explain the discrepancy between the spec and the patch?
> 

0x9A6C9329AC4BC9B5 is 0xAD93D23594C93659 with its bits reversed.

0xAD93D23594C93659 maps the polynomial coefficients to bits in the natural way.
However, writing the polynomial in this way isn't useful for this CRC variant, as it is a bit-reversed CRC.

- Eric

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCHv3 06/10] crypto: add rocksoft 64b crc framework
  2022-02-22 20:09     ` Keith Busch
@ 2022-02-25 16:11       ` Christoph Hellwig
  0 siblings, 0 replies; 9+ messages in thread
From: Christoph Hellwig @ 2022-02-25 16:11 UTC (permalink / raw)
  To: Keith Busch
  Cc: Eric Biggers, linux-nvme, linux-block, linux-crypto, x86,
	linux-kernel, axboe, hch, martin.petersen, colyli

On Tue, Feb 22, 2022 at 12:09:07PM -0800, Keith Busch wrote:
> On Tue, Feb 22, 2022 at 11:50:42AM -0800, Eric Biggers wrote:
> > On Tue, Feb 22, 2022 at 08:31:40AM -0800, Keith Busch wrote:
> > > +config CRYPTO_CRC64_ROCKSOFT
> > > +	tristate "Rocksoft Model CRC64 algorithm"
> > > +	depends on CRC64
> > > +	select CRYPTO_HASH
> > > +	help
> > > +	  Rocksoft Model CRC64 computation is being cast as a crypto
> > > +	  transform. This allows for faster crc64 transforms to be used
> > > +	  if they are available.
> > 
> > The first sentence of this help text doesn't make sense.
> 
> Much of the this patch is a copy from the equivalent T10DIF option,
> which says the same as above. I meant to revist these help texts because
> they don't make sense to me either. I'll be sure to do that for the next
> version.

It might make sense to make CRC_T10DIF an invisible option as well
and drop the help text entirely.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCHv3 06/10] crypto: add rocksoft 64b crc framework
  2022-02-22 19:50   ` Eric Biggers
  2022-02-22 19:54     ` Eric Biggers
@ 2022-02-22 20:09     ` Keith Busch
  2022-02-25 16:11       ` Christoph Hellwig
  1 sibling, 1 reply; 9+ messages in thread
From: Keith Busch @ 2022-02-22 20:09 UTC (permalink / raw)
  To: Eric Biggers
  Cc: linux-nvme, linux-block, linux-crypto, x86, linux-kernel, axboe,
	hch, martin.petersen, colyli

On Tue, Feb 22, 2022 at 11:50:42AM -0800, Eric Biggers wrote:
> On Tue, Feb 22, 2022 at 08:31:40AM -0800, Keith Busch wrote:
> > +config CRYPTO_CRC64_ROCKSOFT
> > +	tristate "Rocksoft Model CRC64 algorithm"
> > +	depends on CRC64
> > +	select CRYPTO_HASH
> > +	help
> > +	  Rocksoft Model CRC64 computation is being cast as a crypto
> > +	  transform. This allows for faster crc64 transforms to be used
> > +	  if they are available.
> 
> The first sentence of this help text doesn't make sense.

Much of the this patch is a copy from the equivalent T10DIF option,
which says the same as above. I meant to revist these help texts because
they don't make sense to me either. I'll be sure to do that for the next
version.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCHv3 06/10] crypto: add rocksoft 64b crc framework
  2022-02-22 16:31 ` [PATCHv3 06/10] crypto: add rocksoft 64b crc framework Keith Busch
  2022-02-22 19:50   ` Eric Biggers
@ 2022-02-22 19:56   ` Eric Biggers
  1 sibling, 0 replies; 9+ messages in thread
From: Eric Biggers @ 2022-02-22 19:56 UTC (permalink / raw)
  To: Keith Busch
  Cc: linux-nvme, linux-block, linux-crypto, x86, linux-kernel, axboe,
	hch, martin.petersen, colyli

On Tue, Feb 22, 2022 at 08:31:40AM -0800, Keith Busch wrote:
> Hardware specific features may be able to calculate a crc64, so provide
> a framework for drivers to register their implementation. If nothing is
> registered, fallback to the generic table lookup implementation. The
> implementation is modeled after the crct10dif equivalent.
> 
> Signed-off-by: Keith Busch <kbusch@kernel.org>
> ---
>  crypto/Kconfig                  |   9 +++
>  crypto/Makefile                 |   1 +
>  crypto/crc64_rocksoft_generic.c | 104 +++++++++++++++++++++++++
>  include/linux/crc64.h           |   5 ++
>  lib/Kconfig                     |   9 +++
>  lib/Makefile                    |   1 +
>  lib/crc64-rocksoft.c            | 129 ++++++++++++++++++++++++++++++++
>  7 files changed, 258 insertions(+)
>  create mode 100644 crypto/crc64_rocksoft_generic.c
>  create mode 100644 lib/crc64-rocksoft.c

I tried testing this, but I can't because it is missing a self-test:

[    0.736340] alg: No test for crc64-rocksoft (crc64-rocksoft-generic)
[    5.440398] alg: No test for crc64-rocksoft (crc64-rocksoft-pclmul)

All algorithms registered with the crypto API need to have a self-test
(in crypto/testmgr.c).

- Eric

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCHv3 06/10] crypto: add rocksoft 64b crc framework
  2022-02-22 19:50   ` Eric Biggers
@ 2022-02-22 19:54     ` Eric Biggers
  2022-02-22 20:09     ` Keith Busch
  1 sibling, 0 replies; 9+ messages in thread
From: Eric Biggers @ 2022-02-22 19:54 UTC (permalink / raw)
  To: Keith Busch
  Cc: linux-nvme, linux-block, linux-crypto, x86, linux-kernel, axboe,
	hch, martin.petersen, colyli

On Tue, Feb 22, 2022 at 11:50:44AM -0800, Eric Biggers wrote:
> > +config CRC64_ROCKSOFT
> > +	tristate "CRC calculation for the Rocksoft^TM model CRC64"
> 
> I'm sure what the rules for trademarks are, but kernel source code usually
> doesn't have the trademark symbol/abbreviation scattered everywhere.
> 
> > +	select CRYPTO
> > +	select CRYPTO_CRC64_ROCKSOFT
> > +	help
> > +	  This option is only needed if a module that's not in the
> > +	  kernel tree needs to calculate CRC checks for use with the
> > +	  rocksoft model parameters.
> 
> Out-of-tree modules can't be the reason to have a kconfig option.  What is the
> real reason?

Also this option can be enabled without the CONFIG_CRC64 it depends on, which is
broken.

- Eric

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCHv3 06/10] crypto: add rocksoft 64b crc framework
  2022-02-22 16:31 ` [PATCHv3 06/10] crypto: add rocksoft 64b crc framework Keith Busch
@ 2022-02-22 19:50   ` Eric Biggers
  2022-02-22 19:54     ` Eric Biggers
  2022-02-22 20:09     ` Keith Busch
  2022-02-22 19:56   ` Eric Biggers
  1 sibling, 2 replies; 9+ messages in thread
From: Eric Biggers @ 2022-02-22 19:50 UTC (permalink / raw)
  To: Keith Busch
  Cc: linux-nvme, linux-block, linux-crypto, x86, linux-kernel, axboe,
	hch, martin.petersen, colyli

On Tue, Feb 22, 2022 at 08:31:40AM -0800, Keith Busch wrote:
> +config CRYPTO_CRC64_ROCKSOFT
> +	tristate "Rocksoft Model CRC64 algorithm"
> +	depends on CRC64
> +	select CRYPTO_HASH
> +	help
> +	  Rocksoft Model CRC64 computation is being cast as a crypto
> +	  transform. This allows for faster crc64 transforms to be used
> +	  if they are available.

The first sentence of this help text doesn't make sense.

> diff --git a/crypto/crc64_rocksoft_generic.c b/crypto/crc64_rocksoft_generic.c
> new file mode 100644
> index 000000000000..55bad1939614
> --- /dev/null
> +++ b/crypto/crc64_rocksoft_generic.c
> @@ -0,0 +1,104 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Cryptographic API.

The "Cryptographic API" line doesn't provide any helpful information.

> +static int chksum_final(struct shash_desc *desc, u8 *out)
> +{
> +	struct chksum_desc_ctx *ctx = shash_desc_ctx(desc);
> +
> +	*(u64 *)out = ctx->crc;
> +	return 0;
> +}
> +
> +static int __chksum_finup(u64 crc, const u8 *data, unsigned int len, u8 *out)
> +{
> +	*(u64 *)out = crc64_rocksoft_generic(crc, data, len);
> +	return 0;
> +}

These 64-bit writes violate alignment rules and will give the wrong result on
big endian CPUs.  They need to use put_unaligned_le64().

> +static int __init crc64_rocksoft_x86_mod_init(void)
> +{
> +	return crypto_register_shash(&alg);
> +}
> +
> +static void __exit crc64_rocksoft_x86_mod_fini(void)
> +{
> +	crypto_unregister_shash(&alg);
> +}

This has nothing to do with x86.

> +config CRC64_ROCKSOFT
> +	tristate "CRC calculation for the Rocksoft^TM model CRC64"

I'm sure what the rules for trademarks are, but kernel source code usually
doesn't have the trademark symbol/abbreviation scattered everywhere.

> +	select CRYPTO
> +	select CRYPTO_CRC64_ROCKSOFT
> +	help
> +	  This option is only needed if a module that's not in the
> +	  kernel tree needs to calculate CRC checks for use with the
> +	  rocksoft model parameters.

Out-of-tree modules can't be the reason to have a kconfig option.  What is the
real reason?

> +u64 crc64_rocksoft(const unsigned char *buffer, size_t len)
> +{
> +	return crc64_rocksoft_update(~0ULL, buffer, len);
> +}
> +EXPORT_SYMBOL(crc64_rocksoft);

Isn't this missing the bitwise inversion at the end?

> +MODULE_AUTHOR("Keith Busch <kbusch@kernel.org>");
> +MODULE_DESCRIPTION("Rocksoft model CRC64 calculation (library API)");
> +MODULE_LICENSE("GPL");
> +MODULE_SOFTDEP("pre: crc64");

Shouldn't the MODULE_SOFTDEP be on crc64-rocksoft?

- Eric

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCHv3 06/10] crypto: add rocksoft 64b crc framework
  2022-02-22 16:31 [PATCHv3 00/10] 64-bit data integrity field support Keith Busch
@ 2022-02-22 16:31 ` Keith Busch
  2022-02-22 19:50   ` Eric Biggers
  2022-02-22 19:56   ` Eric Biggers
  0 siblings, 2 replies; 9+ messages in thread
From: Keith Busch @ 2022-02-22 16:31 UTC (permalink / raw)
  To: linux-nvme, linux-block, linux-crypto, x86, linux-kernel
  Cc: axboe, hch, martin.petersen, colyli, Keith Busch

Hardware specific features may be able to calculate a crc64, so provide
a framework for drivers to register their implementation. If nothing is
registered, fallback to the generic table lookup implementation. The
implementation is modeled after the crct10dif equivalent.

Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 crypto/Kconfig                  |   9 +++
 crypto/Makefile                 |   1 +
 crypto/crc64_rocksoft_generic.c | 104 +++++++++++++++++++++++++
 include/linux/crc64.h           |   5 ++
 lib/Kconfig                     |   9 +++
 lib/Makefile                    |   1 +
 lib/crc64-rocksoft.c            | 129 ++++++++++++++++++++++++++++++++
 7 files changed, 258 insertions(+)
 create mode 100644 crypto/crc64_rocksoft_generic.c
 create mode 100644 lib/crc64-rocksoft.c

diff --git a/crypto/Kconfig b/crypto/Kconfig
index 442765219c37..e343147b9f8f 100644
--- a/crypto/Kconfig
+++ b/crypto/Kconfig
@@ -735,6 +735,15 @@ config CRYPTO_CRCT10DIF_VPMSUM
 	  multiply-sum (vpmsum) instructions, introduced in POWER8. Enable on
 	  POWER8 and newer processors for improved performance.
 
+config CRYPTO_CRC64_ROCKSOFT
+	tristate "Rocksoft Model CRC64 algorithm"
+	depends on CRC64
+	select CRYPTO_HASH
+	help
+	  Rocksoft Model CRC64 computation is being cast as a crypto
+	  transform. This allows for faster crc64 transforms to be used
+	  if they are available.
+
 config CRYPTO_VPMSUM_TESTER
 	tristate "Powerpc64 vpmsum hardware acceleration tester"
 	depends on CRYPTO_CRCT10DIF_VPMSUM && CRYPTO_CRC32C_VPMSUM
diff --git a/crypto/Makefile b/crypto/Makefile
index d76bff8d0ffd..f754c4d17d6b 100644
--- a/crypto/Makefile
+++ b/crypto/Makefile
@@ -152,6 +152,7 @@ obj-$(CONFIG_CRYPTO_MICHAEL_MIC) += michael_mic.o
 obj-$(CONFIG_CRYPTO_CRC32C) += crc32c_generic.o
 obj-$(CONFIG_CRYPTO_CRC32) += crc32_generic.o
 obj-$(CONFIG_CRYPTO_CRCT10DIF) += crct10dif_common.o crct10dif_generic.o
+obj-$(CONFIG_CRYPTO_CRC64_ROCKSOFT) += crc64_rocksoft_generic.o
 obj-$(CONFIG_CRYPTO_AUTHENC) += authenc.o authencesn.o
 obj-$(CONFIG_CRYPTO_LZO) += lzo.o lzo-rle.o
 obj-$(CONFIG_CRYPTO_LZ4) += lz4.o
diff --git a/crypto/crc64_rocksoft_generic.c b/crypto/crc64_rocksoft_generic.c
new file mode 100644
index 000000000000..55bad1939614
--- /dev/null
+++ b/crypto/crc64_rocksoft_generic.c
@@ -0,0 +1,104 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Cryptographic API.
+ *
+ * Rocksoft^TM model CRC64 Crypto Transform
+ */
+
+#include <linux/types.h>
+#include <linux/module.h>
+#include <linux/crc64.h>
+#include <crypto/internal/hash.h>
+#include <crypto/internal/simd.h>
+#include <linux/init.h>
+#include <linux/string.h>
+#include <linux/kernel.h>
+#include <asm/cpufeatures.h>
+#include <asm/cpu_device_id.h>
+#include <asm/simd.h>
+
+struct chksum_desc_ctx {
+	u64 crc;
+};
+
+static int chksum_init(struct shash_desc *desc)
+{
+	struct chksum_desc_ctx *ctx = shash_desc_ctx(desc);
+
+	ctx->crc = 0;
+
+	return 0;
+}
+
+static int chksum_update(struct shash_desc *desc, const u8 *data,
+			 unsigned int length)
+{
+	struct chksum_desc_ctx *ctx = shash_desc_ctx(desc);
+
+	ctx->crc = crc64_rocksoft_generic(ctx->crc, data, length);
+	return 0;
+}
+
+static int chksum_final(struct shash_desc *desc, u8 *out)
+{
+	struct chksum_desc_ctx *ctx = shash_desc_ctx(desc);
+
+	*(u64 *)out = ctx->crc;
+	return 0;
+}
+
+static int __chksum_finup(u64 crc, const u8 *data, unsigned int len, u8 *out)
+{
+	*(u64 *)out = crc64_rocksoft_generic(crc, data, len);
+	return 0;
+}
+
+static int chksum_finup(struct shash_desc *desc, const u8 *data,
+			unsigned int len, u8 *out)
+{
+	struct chksum_desc_ctx *ctx = shash_desc_ctx(desc);
+
+	return __chksum_finup(ctx->crc, data, len, out);
+}
+
+static int chksum_digest(struct shash_desc *desc, const u8 *data,
+			 unsigned int length, u8 *out)
+{
+	return __chksum_finup(0, data, length, out);
+}
+
+static struct shash_alg alg = {
+	.digestsize	= 	8,
+	.init		=	chksum_init,
+	.update		=	chksum_update,
+	.final		=	chksum_final,
+	.finup		=	chksum_finup,
+	.digest		=	chksum_digest,
+	.descsize	=	sizeof(struct chksum_desc_ctx),
+	.base		=	{
+		.cra_name		=	CRC64_ROCKSOFT_STRING,
+		.cra_driver_name	=	"crc64-rocksoft-generic",
+		.cra_priority		=	200,
+		.cra_blocksize		=	1,
+		.cra_module		=	THIS_MODULE,
+	}
+};
+
+static int __init crc64_rocksoft_x86_mod_init(void)
+{
+	return crypto_register_shash(&alg);
+}
+
+static void __exit crc64_rocksoft_x86_mod_fini(void)
+{
+	crypto_unregister_shash(&alg);
+}
+
+module_init(crc64_rocksoft_x86_mod_init);
+module_exit(crc64_rocksoft_x86_mod_fini);
+
+MODULE_AUTHOR("Keith Busch <kbusch@kernel.org>");
+MODULE_DESCRIPTION("Rocksoft model CRC64 calculation.");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS_CRYPTO("crc64-rocksoft");
+MODULE_ALIAS_CRYPTO("crc64-rocksoft-generic");
diff --git a/include/linux/crc64.h b/include/linux/crc64.h
index 9480f38cc7cf..e044c60d1e61 100644
--- a/include/linux/crc64.h
+++ b/include/linux/crc64.h
@@ -7,7 +7,12 @@
 
 #include <linux/types.h>
 
+#define CRC64_ROCKSOFT_STRING "crc64-rocksoft"
+
 u64 __pure crc64_be(u64 crc, const void *p, size_t len);
 u64 __pure crc64_rocksoft_generic(u64 crc, const void *p, size_t len);
 
+u64 crc64_rocksoft(const unsigned char *buffer, size_t len);
+u64 crc64_rocksoft_update(u64 crc, const unsigned char *buffer, size_t len);
+
 #endif /* _LINUX_CRC64_H */
diff --git a/lib/Kconfig b/lib/Kconfig
index c80fde816a7e..d4a46d635cb1 100644
--- a/lib/Kconfig
+++ b/lib/Kconfig
@@ -146,6 +146,15 @@ config CRC_T10DIF
 	  kernel tree needs to calculate CRC checks for use with the
 	  SCSI data integrity subsystem.
 
+config CRC64_ROCKSOFT
+	tristate "CRC calculation for the Rocksoft^TM model CRC64"
+	select CRYPTO
+	select CRYPTO_CRC64_ROCKSOFT
+	help
+	  This option is only needed if a module that's not in the
+	  kernel tree needs to calculate CRC checks for use with the
+	  rocksoft model parameters.
+
 config CRC_ITU_T
 	tristate "CRC ITU-T V.41 functions"
 	help
diff --git a/lib/Makefile b/lib/Makefile
index 300f569c626b..130bed83cdf2 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -177,6 +177,7 @@ obj-$(CONFIG_LIBCRC32C)	+= libcrc32c.o
 obj-$(CONFIG_CRC8)	+= crc8.o
 obj-$(CONFIG_XXHASH)	+= xxhash.o
 obj-$(CONFIG_GENERIC_ALLOCATOR) += genalloc.o
+obj-$(CONFIG_CRC64_ROCKSOFT) += crc64-rocksoft.o
 
 obj-$(CONFIG_842_COMPRESS) += 842/
 obj-$(CONFIG_842_DECOMPRESS) += 842/
diff --git a/lib/crc64-rocksoft.c b/lib/crc64-rocksoft.c
new file mode 100644
index 000000000000..49d7418ea827
--- /dev/null
+++ b/lib/crc64-rocksoft.c
@@ -0,0 +1,129 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Rocksoft^TM model CRC64 calculation
+ */
+
+#include <linux/types.h>
+#include <linux/module.h>
+#include <linux/crc64.h>
+#include <linux/err.h>
+#include <linux/init.h>
+#include <crypto/hash.h>
+#include <crypto/algapi.h>
+#include <linux/static_key.h>
+#include <linux/notifier.h>
+
+static struct crypto_shash __rcu *crc64_rocksoft_tfm;
+static DEFINE_STATIC_KEY_TRUE(crc64_rocksoft_fallback);
+static DEFINE_MUTEX(crc64_rocksoft_mutex);
+static struct work_struct crc64_rocksoft_rehash_work;
+
+static int crc64_rocksoft_notify(struct notifier_block *self, unsigned long val, void *data)
+{
+	struct crypto_alg *alg = data;
+
+	if (val != CRYPTO_MSG_ALG_LOADED ||
+	    strcmp(alg->cra_name, CRC64_ROCKSOFT_STRING))
+		return NOTIFY_DONE;
+
+	schedule_work(&crc64_rocksoft_rehash_work);
+	return NOTIFY_OK;
+}
+
+static void crc64_rocksoft_rehash(struct work_struct *work)
+{
+	struct crypto_shash *new, *old;
+
+	mutex_lock(&crc64_rocksoft_mutex);
+	old = rcu_dereference_protected(crc64_rocksoft_tfm,
+					lockdep_is_held(&crc64_rocksoft_mutex));
+	new = crypto_alloc_shash(CRC64_ROCKSOFT_STRING, 0, 0);
+	if (IS_ERR(new)) {
+		mutex_unlock(&crc64_rocksoft_mutex);
+		return;
+	}
+	rcu_assign_pointer(crc64_rocksoft_tfm, new);
+	mutex_unlock(&crc64_rocksoft_mutex);
+
+	if (old) {
+		synchronize_rcu();
+		crypto_free_shash(old);
+	} else {
+		static_branch_disable(&crc64_rocksoft_fallback);
+	}
+}
+
+static struct notifier_block crc64_rocksoft_nb = {
+	.notifier_call = crc64_rocksoft_notify,
+};
+
+u64 crc64_rocksoft_update(u64 crc, const unsigned char *buffer, size_t len)
+{
+	struct {
+		struct shash_desc shash;
+		u64 crc;
+	} desc;
+	int err;
+
+	if (static_branch_unlikely(&crc64_rocksoft_fallback))
+		return crc64_rocksoft_generic(crc, buffer, len);
+
+	rcu_read_lock();
+	desc.shash.tfm = rcu_dereference(crc64_rocksoft_tfm);
+	desc.crc = crc;
+	err = crypto_shash_update(&desc.shash, buffer, len);
+	rcu_read_unlock();
+
+	BUG_ON(err);
+
+	return desc.crc;
+}
+EXPORT_SYMBOL(crc64_rocksoft_update);
+
+u64 crc64_rocksoft(const unsigned char *buffer, size_t len)
+{
+	return crc64_rocksoft_update(~0ULL, buffer, len);
+}
+EXPORT_SYMBOL(crc64_rocksoft);
+
+static int __init crc64_rocksoft_mod_init(void)
+{
+	INIT_WORK(&crc64_rocksoft_rehash_work, crc64_rocksoft_rehash);
+	crypto_register_notifier(&crc64_rocksoft_nb);
+	crc64_rocksoft_rehash(&crc64_rocksoft_rehash_work);
+	return 0;
+}
+
+static void __exit crc64_rocksoft_mod_fini(void)
+{
+	crypto_unregister_notifier(&crc64_rocksoft_nb);
+	cancel_work_sync(&crc64_rocksoft_rehash_work);
+	crypto_free_shash(rcu_dereference_protected(crc64_rocksoft_tfm, 1));
+}
+
+module_init(crc64_rocksoft_mod_init);
+module_exit(crc64_rocksoft_mod_fini);
+
+static int crc64_rocksoft_transform_show(char *buffer, const struct kernel_param *kp)
+{
+	struct crypto_shash *tfm;
+	int len;
+
+	if (static_branch_unlikely(&crc64_rocksoft_fallback))
+		return sprintf(buffer, "fallback\n");
+
+	rcu_read_lock();
+	tfm = rcu_dereference(crc64_rocksoft_tfm);
+	len = snprintf(buffer, PAGE_SIZE, "%s\n",
+		       crypto_shash_driver_name(tfm));
+	rcu_read_unlock();
+
+	return len;
+}
+
+module_param_call(transform, NULL, crc64_rocksoft_transform_show, NULL, 0444);
+
+MODULE_AUTHOR("Keith Busch <kbusch@kernel.org>");
+MODULE_DESCRIPTION("Rocksoft model CRC64 calculation (library API)");
+MODULE_LICENSE("GPL");
+MODULE_SOFTDEP("pre: crc64");
-- 
2.25.4


^ permalink raw reply related	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2022-05-09 21:30 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-05-08  0:01 [PATCHv3 06/10]crypto: add rocksoft 64b crc framework Xiaoming Zhou
2022-05-09 20:37 ` Eric Biggers
2022-05-09 21:29   ` [EXT] " Xiaoming Zhou
  -- strict thread matches above, loose matches on Subject: below --
2022-02-22 16:31 [PATCHv3 00/10] 64-bit data integrity field support Keith Busch
2022-02-22 16:31 ` [PATCHv3 06/10] crypto: add rocksoft 64b crc framework Keith Busch
2022-02-22 19:50   ` Eric Biggers
2022-02-22 19:54     ` Eric Biggers
2022-02-22 20:09     ` Keith Busch
2022-02-25 16:11       ` Christoph Hellwig
2022-02-22 19:56   ` Eric Biggers

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.