[PATCHv3 06/10]crypto: add rocksoft 64b crc framework

Xiaoming Zhou xzhou at marvell.com
Sat May 7 17:01:21 PDT 2022


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 at lists.infradead.org> On Behalf Of linux-nvme-request at lists.infradead.org
Sent: Tuesday, February 22, 2022 12:00 PM
To: linux-nvme at lists.infradead.org
Subject: [EXT] Linux-nvme Digest, Vol 95, Issue 129

External Email

----------------------------------------------------------------------
Send Linux-nvme mailing list submissions to
	linux-nvme at 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 at lists.infradead.org

You can reach the person managing the list at
	linux-nvme-owner at 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 at infradead.org>
To: Jens Axboe <axboe at kernel.dk>
Cc: Christoph Hellwig <hch at infradead.org>, Keith Busch
	<kbusch at kernel.org>, linux-block at vger.kernel.org, Sagi Grimberg
	<sagi at grimberg.me>, linux-nvme at lists.infradead.org
Subject: Re: [GIT PULL] nvme fixes for Linux 5.17
Message-ID: <YhUdfqXy0wCDQywm at 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 at perches.com>
To: Keith Busch <kbusch at kernel.org>, Christoph Hellwig <hch at lst.de>
Cc: linux-nvme at lists.infradead.org, linux-block at vger.kernel.org,
	linux-crypto at vger.kernel.org, x86 at kernel.org,
	linux-kernel at vger.kernel.org, axboe at kernel.dk,
	martin.petersen at oracle.com, colyli at suse.de, Bart Van Assche
	<bvanassche at acm.org>
Subject: Re: [PATCHv3 04/10] linux/kernel: introduce lower_48_bits
	macro
Message-ID:
	<603f9243bb9e1c4c50aaec83a527266b48ab9e20.camel at 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 at kernel.org>
To: Keith Busch <kbusch at kernel.org>
Cc: linux-nvme at lists.infradead.org, linux-block at vger.kernel.org,
	linux-crypto at vger.kernel.org, x86 at kernel.org,
	linux-kernel at vger.kernel.org, axboe at kernel.dk, hch at lst.de,
	martin.petersen at oracle.com, colyli at suse.de
Subject: Re: [PATCHv3 06/10] crypto: add rocksoft 64b crc framework
Message-ID: <YhU+kuMhueXVQvxe at 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 at 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 at kernel.org>
To: Keith Busch <kbusch at kernel.org>
Cc: linux-nvme at lists.infradead.org, linux-block at vger.kernel.org,
	linux-crypto at vger.kernel.org, x86 at kernel.org,
	linux-kernel at vger.kernel.org, axboe at kernel.dk, hch at lst.de,
	martin.petersen at oracle.com, colyli at suse.de
Subject: Re: [PATCHv3 06/10] crypto: add rocksoft 64b crc framework
Message-ID: <YhU/d6wn55/GWPxm at 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 at kernel.org>
To: Keith Busch <kbusch at kernel.org>
Cc: linux-nvme at lists.infradead.org, linux-block at vger.kernel.org,
	linux-crypto at vger.kernel.org, x86 at kernel.org,
	linux-kernel at vger.kernel.org, axboe at kernel.dk, hch at lst.de,
	martin.petersen at oracle.com, colyli at suse.de
Subject: Re: [PATCHv3 06/10] crypto: add rocksoft 64b crc framework
Message-ID: <YhVACzTEylUg5LJx at 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 at 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 at 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
*******************************************



More information about the Linux-nvme mailing list