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

From: Xiaoming Zhou
Date: Sat May 07 2022 - 20:02:00 EST

Next message: Finn Thain: "Re: [RFC v2 01/39] Kconfig: introduce HAS_IOPORT option and select it as necessary"
Previous message: Finn Thain: "Re: [RFC v2 01/39] Kconfig: introduce HAS_IOPORT option and select it as necessary"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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

Next message: Finn Thain: "Re: [RFC v2 01/39] Kconfig: introduce HAS_IOPORT option and select it as necessary"
Previous message: Finn Thain: "Re: [RFC v2 01/39] Kconfig: introduce HAS_IOPORT option and select it as necessary"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]