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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

External Email

----------------------------------------------------------------------
Send Linux-nvme mailing list submissions to
	linux-nvme@xxxxxxxxxxxxxxxxxxx

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@xxxxxxxxxxxxxxxxxxx

You can reach the person managing the list at
	linux-nvme-owner@xxxxxxxxxxxxxxxxxxx

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@xxxxxxxxxxxxx>
To: Jens Axboe <axboe@xxxxxxxxx>
Cc: Christoph Hellwig <hch@xxxxxxxxxxxxx>, Keith Busch
	<kbusch@xxxxxxxxxx>, linux-block@xxxxxxxxxxxxxxx, Sagi Grimberg
	<sagi@xxxxxxxxxxx>, linux-nvme@xxxxxxxxxxxxxxxxxxx
Subject: Re: [GIT PULL] nvme fixes for Linux 5.17
Message-ID: <YhUdfqXy0wCDQywm@xxxxxxxxxxxxx>
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@xxxxxxxxxxx>
To: Keith Busch <kbusch@xxxxxxxxxx>, Christoph Hellwig <hch@xxxxxx>
Cc: linux-nvme@xxxxxxxxxxxxxxxxxxx, linux-block@xxxxxxxxxxxxxxx,
	linux-crypto@xxxxxxxxxxxxxxx, x86@xxxxxxxxxx,
	linux-kernel@xxxxxxxxxxxxxxx, axboe@xxxxxxxxx,
	martin.petersen@xxxxxxxxxx, colyli@xxxxxxx, Bart Van Assche
	<bvanassche@xxxxxxx>
Subject: Re: [PATCHv3 04/10] linux/kernel: introduce lower_48_bits
	macro
Message-ID:
	<603f9243bb9e1c4c50aaec83a527266b48ab9e20.camel@xxxxxxxxxxx>
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@xxxxxxxxxx>
To: Keith Busch <kbusch@xxxxxxxxxx>
Cc: linux-nvme@xxxxxxxxxxxxxxxxxxx, linux-block@xxxxxxxxxxxxxxx,
	linux-crypto@xxxxxxxxxxxxxxx, x86@xxxxxxxxxx,
	linux-kernel@xxxxxxxxxxxxxxx, axboe@xxxxxxxxx, hch@xxxxxx,
	martin.petersen@xxxxxxxxxx, colyli@xxxxxxx
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@xxxxxxxxxx>"); 
> +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@xxxxxxxxxx>
To: Keith Busch <kbusch@xxxxxxxxxx>
Cc: linux-nvme@xxxxxxxxxxxxxxxxxxx, linux-block@xxxxxxxxxxxxxxx,
	linux-crypto@xxxxxxxxxxxxxxx, x86@xxxxxxxxxx,
	linux-kernel@xxxxxxxxxxxxxxx, axboe@xxxxxxxxx, hch@xxxxxx,
	martin.petersen@xxxxxxxxxx, colyli@xxxxxxx
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@xxxxxxxxxx>
To: Keith Busch <kbusch@xxxxxxxxxx>
Cc: linux-nvme@xxxxxxxxxxxxxxxxxxx, linux-block@xxxxxxxxxxxxxxx,
	linux-crypto@xxxxxxxxxxxxxxx, x86@xxxxxxxxxx,
	linux-kernel@xxxxxxxxxxxxxxx, axboe@xxxxxxxxx, hch@xxxxxx,
	martin.petersen@xxxxxxxxxx, colyli@xxxxxxx
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@xxxxxxxxxx>
> ---
>  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@xxxxxxxxxxxxxxxxxxx
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
*******************************************




[Index of Archives]     [Linux RAID]     [Linux SCSI]     [Linux ATA RAID]     [IDE]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Device Mapper]

  Powered by Linux