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 *******************************************
