Messages in this thread Patch in this message | | | From | Xiaoming Zhou <> | Subject | [PATCHv3 06/10]crypto: add rocksoft 64b crc framework | Date | Sun, 8 May 2022 00:01:21 +0000 |
| |
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 *******************************************
| |