[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