64-bit data integrity field support

The block integrity subsystem knows how to construct protection information buffers with metadata beyond the protection information fields. Remove the driver restriction.

Note, this can only work if the PI field appears first in the metadata, as the integrity subsystem doesn't calculate guard tags on preceding metadata.

Reviewed-by Hannes Reinecke <[email protected]>

Reviewed-by Martin K. Petersen <[email protected]>

Signed-off-by Keith Busch <[email protected]>

The NVMe protocol extended the data integrity fields with unaligned 48-bit reference tags. Provide some helper accessors in preparation for these.

Reviewed-by Hannes Reinecke <[email protected]>

Reviewed-by Martin K. Petersen <[email protected]>

Acked-by Arnd Bergmann <[email protected]>

Signed-off-by Keith Busch <[email protected]>

---

The NVMe protocol extended the data integrity fields with unaligned 48-bit reference tags. Provide some helper accessors in preparation for these.

Reviewed-by Hannes Reinecke <[email protected]>

Reviewed-by Martin K. Petersen <[email protected]>

Acked-by Arnd Bergmann <[email protected]>

Signed-off-by Keith Busch <[email protected]>

Quoted 4 lines · Keith Busch

The NVMe protocol extended the data integrity fields with unaligned 48-bit reference tags. Provide some helper accessors in preparation for these.

Quoted 4 lines

Reviewed-by Hannes Reinecke <[email protected]>

Reviewed-by Martin K. Petersen <[email protected]>

Acked-by Arnd Bergmann <[email protected]>

Signed-off-by Keith Busch <[email protected]>

Looks good,

Reviewed-by Christoph Hellwig <[email protected]>

Quoted 10 lines · Christoph Hellwig

Quoted 8 lines · Keith Busch

The NVMe protocol extended the data integrity fields with unaligned 48-bit reference tags. Provide some helper accessors in preparation for these.

Reviewed-by Hannes Reinecke <[email protected]>

Reviewed-by Martin K. Petersen <[email protected]>

Acked-by Arnd Bergmann <[email protected]>

Signed-off-by Keith Busch <[email protected]>

Quoted 3 lines

Looks good,

Reviewed-by Christoph Hellwig <[email protected]>

Perhaps for completeness this should also add the le48 variants like the 24 bit accessors above this.

Recent data integrity field enhancements allow 48-bit reference tags. Introduce a helper macro since this will be a repeated operation.

Quoted 2 lines · Keith Busch

Recent data integrity field enhancements allow 48-bit reference tags. Introduce a helper macro since this will be a repeated operation.

[]

include/linux/kernel.h

Quoted 1 line

include/linux/kernel.h +0 -0

[]

Quoted 4 lines

include/linux/kernel.h +0 -0

@@ -63,6 +63,12 @@
 }					\
 )
 

include/linux/kernel.h line 70

Quoted 5 lines

include/linux/kernel.h +5 -0

+/**
+ * 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?

And visually, it's difficult to quickly count a repeated character to 12.

Perhaps:

static inline u64 lower_48_bits(u64 val)

{
	return val & GENMASK_ULL(47, 0);
}

Quoted 8 lines · Joe Perches

include/linux/kernel.h line 70

Quoted 5 lines · Keith Busch

include/linux/kernel.h +5 -0

+/**
+ * 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.

Quoted 10 lines · Christoph Hellwig

Quoted 8 lines · Joe Perches

include/linux/kernel.h line 70

Quoted 5 lines · Keith Busch

include/linux/kernel.h +5 -0

+/ *
+ * 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?

Quoted 1 line

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.

Sent: 22 February 2022 18:43

Quoted 11 lines · Keith Busch

Quoted 10 lines · Christoph Hellwig

Quoted 8 lines · Joe Perches

include/linux/kernel.h line 70

Quoted 5 lines · Keith Busch

include/linux/kernel.h +5 -0

+/ *
+ * 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?

Quoted 5 lines

Quoted 1 line

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.

From: Joe Perches

Quoted 35 lines

Sent: 22 February 2022 18:43

Quoted 11 lines · Keith Busch

Quoted 10 lines · Christoph Hellwig

Quoted 8 lines · Joe Perches

include/linux/kernel.h line 70

Quoted 5 lines · Keith Busch

include/linux/kernel.h +5 -0

+/ *
+ * 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?

Quoted 5 lines

Quoted 1 line

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;				\
})

Quoted 1 line

and have the compiler have the return value be an appropriate type.

The compiler could make a real pigs breakfast of that.

Oh, did you look for GENMASK([^,]*,[ 0]*) ?

I'd only use something GENMASK() for bit ranges. Even then it is often easier to just write the value in hex.

I think the only time I've written anything like that recently (last 30 years) was for some hardware registers when the documentation user 'bit 1' for the most significant bit.

Quoted 2 lines

I'd only use something GENMASK() for bit ranges. Even then it is often easier to just write the value in hex.

Mostly it's the count of the repeated f that's difficult to quickly verify visually.

Quoted 5 lines · Joe Perches

Quoted 2 lines

I'd only use something GENMASK() for bit ranges. Even then it is often easier to just write the value in hex.

Mostly it's the count of the repeated f that's difficult to quickly verify visually.

I admit I made this counting mistake in earlier versions of this series.

Quoted 10 lines · Joe Perches

Quoted 2 lines · Keith Busch

Recent data integrity field enhancements allow 48-bit reference tags. Introduce a helper macro since this will be a repeated operation.

[]

include/linux/kernel.h

Quoted 1 line

include/linux/kernel.h +0 -0

[]

Quoted 4 lines

include/linux/kernel.h +0 -0

@@ -63,6 +63,12 @@
 }					\
 )
 

Quoted 7 lines

include/linux/kernel.h line 70

Quoted 5 lines

include/linux/kernel.h +5 -0

+/**
+ * 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.

Quoted 4 lines

And visually, it's difficult to quickly count a repeated character to 12.

Perhaps:

Quoted 4 lines

static inline u64 lower_48_bits(u64 val)

{
	return val & GENMASK_ULL(47, 0);
}

For anyone who has a minimum knowledge of C and hardware your version is an obsfucated clusterfuck, while the version Keith wrote is trivial to read.

Sent: 22 February 2022 16:51

Quoted 10 lines · Joe Perches

Quoted 2 lines · Keith Busch

Recent data integrity field enhancements allow 48-bit reference tags. Introduce a helper macro since this will be a repeated operation.

[]

include/linux/kernel.h

Quoted 1 line

include/linux/kernel.h +0 -0

[]

Quoted 4 lines

include/linux/kernel.h +0 -0

@@ -63,6 +63,12 @@
 }					\
 )
 

Quoted 7 lines

include/linux/kernel.h line 70

Quoted 5 lines

include/linux/kernel.h +5 -0

+/**
+ * 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.

Quoted 4 lines

And visually, it's difficult to quickly count a repeated character to 12.

Perhaps:

Quoted 4 lines

static inline u64 lower_48_bits(u64 val)

{
	return val & GENMASK_ULL(47, 0);
}

For anyone who has a minimum knowledge of C and hardware your version is an obsfucated clusterfuck, while the version Keith wrote is trivial to read.

Quoted 10 lines · Joe Perches

Quoted 2 lines · Keith Busch

Recent data integrity field enhancements allow 48-bit reference tags. Introduce a helper macro since this will be a repeated operation.

[]

include/linux/kernel.h

Quoted 1 line

include/linux/kernel.h +0 -0

[]

Quoted 4 lines

include/linux/kernel.h +0 -0

@@ -63,6 +63,12 @@
  }					\
  )
 

Quoted 7 lines

include/linux/kernel.h line 70

Quoted 5 lines

include/linux/kernel.h +5 -0

+/**
+ * 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.

Recent data integrity field enhancements allow 48-bit reference tags. Introduce a helper macro since this will be a repeated operation.

Suggested-by Bart Van Assche <[email protected]>

Signed-off-by Keith Busch <[email protected]>

---

lib/Kconfig config CRC_T10DIF · line 150

Quoted 2 lines

lib/Kconfig +2 -0

+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.

lib/Kconfig config CRC_T10DIF · line 156

Quoted 6 lines

lib/Kconfig +6 -0

+	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?

crypto/Kconfig config CRYPTO_CRCT10DIF_VPMSUM · line 745

Quoted 8 lines · Keith Busch

crypto/Kconfig +8 -0

+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.

Quoted 11 lines · Eric Biggers

crypto/Kconfig config CRYPTO_CRCT10DIF_VPMSUM · line 745

Quoted 8 lines · Keith Busch

crypto/Kconfig +8 -0

+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.

Much of the this patch is a copy from the equivalent T10DIF option, which says the same as above. I meant to revist these help texts because they don't make sense to me either. I'll be sure to do that for the next version.

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 <[email protected]>

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

Provide a module to test the rocksoft crc64 calculations with well known inputs and exepected values. Check the generic table implementation and whatever module is registered from the crypto library, and compare their speeds.

Signed-off-by Keith Busch <[email protected]>

---

Provide a module to test the rocksoft crc64 calculations with well known inputs and exepected values. Check the generic table implementation and whatever module is registered from the crypto library, and compare their speeds.

Quoted 4 lines · Keith Busch

Provide a module to test the rocksoft crc64 calculations with well known inputs and exepected values. Check the generic table implementation and whatever module is registered from the crypto library, and compare their speeds.

The code looks good, but isn't the trent to use the kunit framework for these kinds of tests these days?

I don't have experience with kunit, but I'll look into that.

I am already changing the way this gets tested. Eric recommended adding to the crypto "testmgr", and I've done that on my private tree. That test framework exercises a lot more than this this patch, and it did reveal a problem with how I've implemented the initial XOR when the buffer is split, so I have some minor updates coming soon.

block/t10-pi.c +164 -0

+{
+	unsigned int i;
+
+	for (i = 0 ; i < iter->data_size ; i += iter->interval) {
+		struct nvme_crc64_pi_tuple *pi = iter->prot_buf;
+
+		pi->guard_tag = nvme_pi_crc64(iter->data_buf, iter->interval);
+		pi->app_tag = 0;
+
+		if (type == T10_PI_TYPE1_PROTECTION)
+			put_unaligned_be48(iter->seed, pi->ref_tag);
+		else
+			put_unaligned_be48(0ULL, pi->ref_tag);
+
+		iter->data_buf += iter->interval;
+		iter->prot_buf += iter->tuple_size;
+		iter->seed++;
+	}
+
+	return BLK_STS_OK;
+}
+
+static bool nvme_crc64_ref_escape(u8 *ref_tag)
+{
+	static u8 ref_escape[6] = { 0xff, 0xff, 0xff, 0xff, 0xff, 0xff };
+
+	return memcmp(ref_tag, ref_escape, sizeof(ref_escape)) == 0;
+}
+
+static blk_status_t nvme_crc64_verify(struct blk_integrity_iter *iter,
+				      enum t10_dif_type type)
+{
+	unsigned int i;
+
+	for (i = 0; i < iter->data_size; i += iter->interval) {
+		struct nvme_crc64_pi_tuple *pi = iter->prot_buf;
+		u64 ref, seed;
+		__be64 csum;
+
+		if (type == T10_PI_TYPE1_PROTECTION) {
+			if (pi->app_tag == T10_PI_APP_ESCAPE)
+				goto next;
+
+			ref = get_unaligned_be48(pi->ref_tag);
+			seed = lower_48_bits(iter->seed);
+			if (ref != seed) {
+				pr_err("%s: ref tag error at location %llu (rcvd %llu)\n",
+					iter->disk_name, seed, ref);
+				return BLK_STS_PROTECTION;
+			}
+		} else if (type == T10_PI_TYPE3_PROTECTION) {
+			if (pi->app_tag == T10_PI_APP_ESCAPE &&
+			    nvme_crc64_ref_escape(pi->ref_tag))
+				goto next;
+		}
+
+		csum = nvme_pi_crc64(iter->data_buf, iter->interval);
+		if (pi->guard_tag != csum) {
+			pr_err("%s: guard tag error at sector %llu " \
+			       "(rcvd %016llx, want %016llx)\n",
+				iter->disk_name, (unsigned long long)iter->seed,
+				be64_to_cpu(pi->guard_tag), be64_to_cpu(csum));
+			return BLK_STS_PROTECTION;
+		}
+
+next:
+		iter->data_buf += iter->interval;
+		iter->prot_buf += iter->tuple_size;
+		iter->seed++;
+	}
+
+	return BLK_STS_OK;
+}
+
+static blk_status_t nvme_pi_type1_verify_crc(struct blk_integrity_iter *iter)
+{
+	return nvme_crc64_verify(iter, T10_PI_TYPE1_PROTECTION);
+}
+
+static blk_status_t nvme_pi_type1_generate_crc(struct blk_integrity_iter *iter)
+{
+	return nvme_crc64_generate(iter, T10_PI_TYPE1_PROTECTION);
+}
+
+static void nvme_pi_type1_prepare(struct request *rq)
+{
+	const int tuple_sz = rq->q->integrity.tuple_size;
+	u64 ref_tag = nvme_pi_extended_ref_tag(rq);
+	struct bio *bio;
+
+	__rq_for_each_bio(bio, rq) {
+		struct bio_integrity_payload *bip = bio_integrity(bio);
+		u64 virt = lower_48_bits(bip_get_seed(bip));
+		struct bio_vec iv;
+		struct bvec_iter iter;
+
+		/* Already remapped? */
+		if (bip->bip_flags & BIP_MAPPED_INTEGRITY)
+			break;
+
+		bip_for_each_vec(iv, bip, iter) {
+			unsigned int j;
+			void *p;
+
+			p = bvec_kmap_local(&iv);
+			for (j = 0; j < iv.bv_len; j += tuple_sz) {
+				struct nvme_crc64_pi_tuple *pi = p;
+				u64 ref = get_unaligned_be48(pi->ref_tag);
+
+				if (ref == virt)
+					put_unaligned_be48(ref_tag, pi->ref_tag);
+				virt++;
+				ref_tag++;
+				p += tuple_sz;
+			}
+			kunmap_local(p);
+		}
+
+		bip->bip_flags |= BIP_MAPPED_INTEGRITY;
+	}
+}
+
+static void nvme_pi_type1_complete(struct request *rq, unsigned int nr_bytes)
+{
+	unsigned intervals = nr_bytes >> rq->q->integrity.interval_exp;
+	const int tuple_sz = rq->q->integrity.tuple_size;
+	u64 ref_tag = nvme_pi_extended_ref_tag(rq);
+	struct bio *bio;
+
+	__rq_for_each_bio(bio, rq) {
+		struct bio_integrity_payload *bip = bio_integrity(bio);
+		u64 virt = lower_48_bits(bip_get_seed(bip));
+		struct bio_vec iv;
+		struct bvec_iter iter;
+
+		bip_for_each_vec(iv, bip, iter) {
+			unsigned int j;
+			void *p;
+
+			p = bvec_kmap_local(&iv);
+			for (j = 0; j < iv.bv_len && intervals; j += tuple_sz) {
+				struct nvme_crc64_pi_tuple *pi = p;
+				u64 ref = get_unaligned_be48(pi->ref_tag);
+
+				if (ref == ref_tag)
+					put_unaligned_be48(virt, pi->ref_tag);
+				virt++;
+				ref_tag++;
+				intervals--;
+				p += tuple_sz;
+			}
+			kunmap_local(p);
+		}
+	}
+}
+
+static blk_status_t nvme_pi_type3_verify_crc(struct blk_integrity_iter *iter)
+{
+	return nvme_crc64_verify(iter, T10_PI_TYPE3_PROTECTION);
+}
+
+static blk_status_t nvme_pi_type3_generate_crc(struct blk_integrity_iter *iter)
+{
+	return nvme_crc64_generate(iter, T10_PI_TYPE3_PROTECTION);

Review

Quoted 2 lines

(quoted diff) +2 -0

+static blk_status_t nvme_crc64_generate(struct blk_integrity_iter *iter,
+					enum t10_dif_type type)

Shouldn't the naming be something more like ext_pi_*? For one thing I kinda hate having the nvme prefix here in block layer code, but also nvme supports the normal 8 byte PI tuples, so this is a bit confusing.

NVM Express ratified TP 4068 defines new protection information formats. Implement support for the CRC64 guard tags.

Since the block layer doesn't support variable length reference tags, driver support for the Storage Tag space is not supported at this time.

Sent: 22 February 2022 16:32

The crc64 table lookup method is inefficient, using a significant number of CPU cycles in the block stack per IO. If available on x86, use a PCLMULQDQ implementation to accelerate the calculation.

The assembly from this patch was mostly generated by gcc from a C program using library functions provided by x86 intrinsics, and measures ~20x faster than the table lookup.

From: Keith Busch

Quoted 6 lines

Sent: 22 February 2022 16:32

The crc64 table lookup method is inefficient, using a significant number of CPU cycles in the block stack per IO. If available on x86, use a PCLMULQDQ implementation to accelerate the calculation.

Quoted 3 lines

The assembly from this patch was mostly generated by gcc from a C program using library functions provided by x86 intrinsics, and measures ~20x faster than the table lookup.

I think I'd like to see the C code and compiler options used to generate the assembler as comments in the committed source file. Either that or reasonable comments in the assembler.

Quoted 7 lines · David Laight

From: Keith Busch

Quoted 6 lines

Sent: 22 February 2022 16:32

The crc64 table lookup method is inefficient, using a significant number of CPU cycles in the block stack per IO. If available on x86, use a PCLMULQDQ implementation to accelerate the calculation.

Quoted 7 lines

Quoted 3 lines

The assembly from this patch was mostly generated by gcc from a C program using library functions provided by x86 intrinsics, and measures ~20x faster than the table lookup.

I think I'd like to see the C code and compiler options used to generate the assembler as comments in the committed source file. Either that or reasonable comments in the assembler.

The C code, compiled as "gcc -O3 -msse4 -mpclmul -S", was adapted from this found on the internet:

https://github.com/rawrunprotected/crc/blob/master/crc64.c

I just ported it to linux, changed the poly parameters and removed the unnecessary stuff.

I'm okay with dropping this patch from the series for now since I don't think I'm qualified to write it. :) I just needed something to test the crytpo module registration.

Is the license of that code compatible with the kernel's license?

In any case, adding uncommented generated assembly isn't acceptable. The most common convention for Linux kernel crypto is to use hand-written assembly that is properly commented.

There is some precedent for using compiler intrinsics instead, e.g. crypto/aegis128-neon-inner.c. (I'm not sure why they aren't used more often.)

There are also some files where a Perl script generates the assembly code. (This is a bit ugly IMO, but it's what the author of much of OpenSSL's crypto assembly code does, and it was desired to reuse that code.)

Anyway, those are the available options. Checking in some uncommented generated assembly isn't one of them.