[dpdk-dev] [PATCH] hash: fix CRC32c computation

[dpdk-dev] [PATCH] hash: fix CRC32c computation

[Didier Pallard]

As demonstrated by the following code, CRC32c computation is not valid
when buffer length is not a multiple of 4 bytes:
(Output obtained by code below)

CRC of 1 NULL bytes expected: 0x527d5351
    soft: 527d5351
    rte accelerated: 48674bc7
    rte soft: 48674bc7
CRC of 2 NULL bytes expected: 0xf16177d2
    soft: f16177d2
    rte accelerated: 48674bc7
    rte soft: 48674bc7
CRC of 2x1 NULL bytes expected: 0xf16177d2
    soft: f16177d2
    rte accelerated: 8c28b28a
    rte soft: 8c28b28a
CRC of 3 NULL bytes expected: 0x6064a37a
    soft: 6064a37a
    rte accelerated: 48674bc7
    rte soft: 48674bc7
CRC of 4 NULL bytes expected: 0x48674bc7
    soft: 48674bc7
    rte accelerated: 48674bc7
    rte soft: 48674bc7

Values returned by rte_hash_crc functions does not match the one
computed by a trivial crc32c implementation.

ARM code is a guess, it is not tested, neither compiled.

code showing the problem:

uint8_t null_test[32] = {0};

static uint32_t crc32c_trivial(uint8_t *buffer, uint32_t length, uint32_t crc)
{
    uint32_t i, j;
    for (i = 0; i < length; ++i)
    {
        crc = crc ^ buffer[i];
        for (j = 0; j < 8; j++)
            crc = (crc >> 1) ^ 0x80000000 ^ ((~crc & 1) * 0x82f63b78);
    }
    return crc;
}

void hash_test(void);
void hash_test(void)
{
	printf("CRC of 1 nul byte expected: 0x527d5351\n");
	printf("    soft: %08x\n", crc32c_trivial(null_test, 1, 0));
	rte_hash_crc_init_alg();
	printf("    rte accelerated: %08x\n", ~rte_hash_crc(null_test, 1, 0xFFFFFFFF));
	rte_hash_crc_set_alg(CRC32_SW);
	printf("    rte soft: %08x\n", ~rte_hash_crc(null_test, 1, 0xFFFFFFFF));

	printf("CRC of 2 nul bytes expected: 0xf16177d2\n");
	printf("    soft: %08x\n", crc32c_trivial(null_test, 2, 0));
	rte_hash_crc_init_alg();
	printf("    rte accelerated: %08x\n", ~rte_hash_crc(null_test, 2, 0xFFFFFFFF));
	rte_hash_crc_set_alg(CRC32_SW);
	printf("    rte soft: %08x\n", ~rte_hash_crc(null_test, 2, 0xFFFFFFFF));

	printf("CRC of 2x1 nul bytes expected: 0xf16177d2\n");
	printf("    soft: %08x\n", crc32c_trivial(null_test, 1, crc32c_trivial(null_test, 1, 0)));
	rte_hash_crc_init_alg();
	printf("    rte accelerated: %08x\n", ~rte_hash_crc(null_test, 1, rte_hash_crc(null_test, 1, 0xFFFFFFFF)));
	rte_hash_crc_set_alg(CRC32_SW);
	printf("    rte soft: %08x\n", ~rte_hash_crc(null_test, 1, rte_hash_crc(null_test, 1, 0xFFFFFFFF)));

	printf("CRC of 3 nul bytes expected: 0x6064a37a\n");
	printf("    soft: %08x\n", crc32c_trivial(null_test, 3, 0));
	rte_hash_crc_init_alg();
	printf("    rte accelerated: %08x\n", ~rte_hash_crc(null_test, 3, 0xFFFFFFFF));
	rte_hash_crc_set_alg(CRC32_SW);
	printf("    rte soft: %08x\n", ~rte_hash_crc(null_test, 3, 0xFFFFFFFF));

	printf("CRC of 4 nul bytes expected: 0x48674bc7\n");
	printf("    soft: %08x\n", crc32c_trivial(null_test, 4, 0));
	rte_hash_crc_init_alg();
	printf("    rte accelerated: %08x\n", ~rte_hash_crc(null_test, 4, 0xFFFFFFFF));
	rte_hash_crc_set_alg(CRC32_SW);
	printf("    rte soft: %08x\n", ~rte_hash_crc(null_test, 4, 0xFFFFFFFF));
}

Signed-off-by: Didier Pallard <didier.pallard@6wind.com>
Acked-by: David Marchand <david.marchand@6wind.com>
---
 lib/librte_hash/rte_crc_arm64.h |  64 ++++++++++++++++++++
 lib/librte_hash/rte_hash_crc.h  | 125 +++++++++++++++++++++++++++++++---------
 2 files changed, 162 insertions(+), 27 deletions(-)

diff --git a/lib/librte_hash/rte_crc_arm64.h b/lib/librte_hash/rte_crc_arm64.h
index 02e26bc..44ef460 100644
--- a/lib/librte_hash/rte_crc_arm64.h
+++ b/lib/librte_hash/rte_crc_arm64.h
@@ -50,6 +50,28 @@ extern "C" {
 #include <rte_common.h>
 
 static inline uint32_t
+crc32c_arm64_u8(uint8_t data, uint32_t init_val)
+{
+	asm(".arch armv8-a+crc");
+	__asm__ volatile(
+			"crc32cb %w[crc], %w[crc], %b[value]"
+			: [crc] "+r" (init_val)
+			: [value] "r" (data));
+	return init_val;
+}
+
+static inline uint32_t
+crc32c_arm64_u16(uint16_t data, uint32_t init_val)
+{
+	asm(".arch armv8-a+crc");
+	__asm__ volatile(
+			"crc32ch %w[crc], %w[crc], %h[value]"
+			: [crc] "+r" (init_val)
+			: [value] "r" (data));
+	return init_val;
+}
+
+static inline uint32_t
 crc32c_arm64_u32(uint32_t data, uint32_t init_val)
 {
 	asm(".arch armv8-a+crc");
@@ -103,6 +125,48 @@ rte_hash_crc_init_alg(void)
 }
 
 /**
+ * Use single crc32 instruction to perform a hash on a 1 byte value.
+ * Fall back to software crc32 implementation in case arm64 crc intrinsics is
+ * not supported
+ *
+ * @param data
+ *   Data to perform hash on.
+ * @param init_val
+ *   Value to initialise hash generator.
+ * @return
+ *   32bit calculated hash value.
+ */
+static inline uint32_t
+rte_hash_crc_1byte(uint8_t data, uint32_t init_val)
+{
+	if (likely(crc32_alg & CRC32_ARM64))
+		return crc32c_arm64_u8(data, init_val);
+
+	return crc32c_1byte(data, init_val);
+}
+
+/**
+ * Use single crc32 instruction to perform a hash on a 2 bytes value.
+ * Fall back to software crc32 implementation in case arm64 crc intrinsics is
+ * not supported
+ *
+ * @param data
+ *   Data to perform hash on.
+ * @param init_val
+ *   Value to initialise hash generator.
+ * @return
+ *   32bit calculated hash value.
+ */
+static inline uint32_t
+rte_hash_crc_2byte(uint16_t data, uint32_t init_val)
+{
+	if (likely(crc32_alg & CRC32_ARM64))
+		return crc32c_arm64_u16(data, init_val);
+
+	return crc32c_2bytes(data, init_val);
+}
+
+/**
  * Use single crc32 instruction to perform a hash on a 4 byte value.
  * Fall back to software crc32 implementation in case arm64 crc intrinsics is
  * not supported
diff --git a/lib/librte_hash/rte_hash_crc.h b/lib/librte_hash/rte_hash_crc.h
index 78a34b7..485f8a2 100644
--- a/lib/librte_hash/rte_hash_crc.h
+++ b/lib/librte_hash/rte_hash_crc.h
@@ -328,6 +328,28 @@ static const uint32_t crc32c_tables[8][256] = {{
 	 crc32c_tables[(n)-1][((crc) >> 8) & 0xFF])
 
 static inline uint32_t
+crc32c_1byte(uint8_t data, uint32_t init_val)
+{
+	uint32_t crc;
+	crc = init_val;
+	crc ^= data;
+
+	return crc32c_tables[0][crc & 0xff] ^ (crc >> 8);
+}
+
+static inline uint32_t
+crc32c_2bytes(uint16_t data, uint32_t init_val)
+{
+	uint32_t crc;
+	crc = init_val;
+	crc ^= data;
+
+	crc = CRC32_UPD(crc, 1) ^ (crc >> 16);
+
+	return crc;
+}
+
+static inline uint32_t
 crc32c_1word(uint32_t data, uint32_t init_val)
 {
 	uint32_t crc, term1, term2;
@@ -367,6 +389,26 @@ crc32c_2words(uint64_t data, uint32_t init_val)
 
 #if defined(RTE_ARCH_I686) || defined(RTE_ARCH_X86_64)
 static inline uint32_t
+crc32c_sse42_u8(uint8_t data, uint32_t init_val)
+{
+	__asm__ volatile(
+			"crc32b %[data], %[init_val];"
+			: [init_val] "+r" (init_val)
+			: [data] "rm" (data));
+	return init_val;
+}
+
+static inline uint32_t
+crc32c_sse42_u16(uint16_t data, uint32_t init_val)
+{
+	__asm__ volatile(
+			"crc32w %[data], %[init_val];"
+			: [init_val] "+r" (init_val)
+			: [data] "rm" (data));
+	return init_val;
+}
+
+static inline uint32_t
 crc32c_sse42_u32(uint32_t data, uint32_t init_val)
 {
 	__asm__ volatile(
@@ -453,6 +495,52 @@ rte_hash_crc_init_alg(void)
 }
 
 /**
+ * Use single crc32 instruction to perform a hash on a byte value.
+ * Fall back to software crc32 implementation in case SSE4.2 is
+ * not supported
+ *
+ * @param data
+ *   Data to perform hash on.
+ * @param init_val
+ *   Value to initialise hash generator.
+ * @return
+ *   32bit calculated hash value.
+ */
+static inline uint32_t
+rte_hash_crc_1byte(uint8_t data, uint32_t init_val)
+{
+#if defined RTE_ARCH_I686 || defined RTE_ARCH_X86_64
+	if (likely(crc32_alg & CRC32_SSE42))
+		return crc32c_sse42_u8(data, init_val);
+#endif
+
+	return crc32c_1byte(data, init_val);
+}
+
+/**
+ * Use single crc32 instruction to perform a hash on a byte value.
+ * Fall back to software crc32 implementation in case SSE4.2 is
+ * not supported
+ *
+ * @param data
+ *   Data to perform hash on.
+ * @param init_val
+ *   Value to initialise hash generator.
+ * @return
+ *   32bit calculated hash value.
+ */
+static inline uint32_t
+rte_hash_crc_2byte(uint16_t data, uint32_t init_val)
+{
+#if defined RTE_ARCH_I686 || defined RTE_ARCH_X86_64
+	if (likely(crc32_alg & CRC32_SSE42))
+		return crc32c_sse42_u16(data, init_val);
+#endif
+
+	return crc32c_2bytes(data, init_val);
+}
+
+/**
  * Use single crc32 instruction to perform a hash on a 4 byte value.
  * Fall back to software crc32 implementation in case SSE4.2 is
  * not supported
@@ -521,7 +609,6 @@ static inline uint32_t
 rte_hash_crc(const void *data, uint32_t data_len, uint32_t init_val)
 {
 	unsigned i;
-	uint64_t temp = 0;
 	uintptr_t pd = (uintptr_t) data;
 
 	for (i = 0; i < data_len / 8; i++) {
@@ -529,35 +616,19 @@ rte_hash_crc(const void *data, uint32_t data_len, uint32_t init_val)
 		pd += 8;
 	}
 
-	switch (7 - (data_len & 0x07)) {
-	case 0:
-		temp |= (uint64_t) *((const uint8_t *)pd + 6) << 48;
-		/* Fallthrough */
-	case 1:
-		temp |= (uint64_t) *((const uint8_t *)pd + 5) << 40;
-		/* Fallthrough */
-	case 2:
-		temp |= (uint64_t) *((const uint8_t *)pd + 4) << 32;
-		temp |= *(const uint32_t *)pd;
-		init_val = rte_hash_crc_8byte(temp, init_val);
-		break;
-	case 3:
+	if (data_len & 0x4) {
 		init_val = rte_hash_crc_4byte(*(const uint32_t *)pd, init_val);
-		break;
-	case 4:
-		temp |= *((const uint8_t *)pd + 2) << 16;
-		/* Fallthrough */
-	case 5:
-		temp |= *((const uint8_t *)pd + 1) << 8;
-		/* Fallthrough */
-	case 6:
-		temp |= *(const uint8_t *)pd;
-		init_val = rte_hash_crc_4byte(temp, init_val);
-		/* Fallthrough */
-	default:
-		break;
+		pd += 4;
+	}
+
+	if (data_len & 0x2) {
+		init_val = rte_hash_crc_2byte(*(const uint16_t *)pd, init_val);
+		pd += 2;
 	}
 
+	if (data_len & 0x1)
+		init_val = rte_hash_crc_1byte(*(const uint8_t *)pd, init_val);
+
 	return init_val;
 }
 
-- 
2.1.4

Re: [dpdk-dev] [PATCH] hash: fix CRC32c computation

[Qiu, Michael]

Is it suitable to put so many code in commit log?

Thanks,
Michael
On 12/22/2015 5:36 PM, Didier Pallard wrote:
> As demonstrated by the following code, CRC32c computation is not valid
> when buffer length is not a multiple of 4 bytes:
> (Output obtained by code below)
>
> CRC of 1 NULL bytes expected: 0x527d5351
>     soft: 527d5351
>     rte accelerated: 48674bc7
>     rte soft: 48674bc7
> CRC of 2 NULL bytes expected: 0xf16177d2
>     soft: f16177d2
>     rte accelerated: 48674bc7
>     rte soft: 48674bc7
> CRC of 2x1 NULL bytes expected: 0xf16177d2
>     soft: f16177d2
>     rte accelerated: 8c28b28a
>     rte soft: 8c28b28a
> CRC of 3 NULL bytes expected: 0x6064a37a
>     soft: 6064a37a
>     rte accelerated: 48674bc7
>     rte soft: 48674bc7
> CRC of 4 NULL bytes expected: 0x48674bc7
>     soft: 48674bc7
>     rte accelerated: 48674bc7
>     rte soft: 48674bc7
>
> Values returned by rte_hash_crc functions does not match the one
> computed by a trivial crc32c implementation.
>
> ARM code is a guess, it is not tested, neither compiled.
>
> code showing the problem:
>
> uint8_t null_test[32] = {0};
>
> static uint32_t crc32c_trivial(uint8_t *buffer, uint32_t length, uint32_t crc)
> {
>     uint32_t i, j;
>     for (i = 0; i < length; ++i)
>     {
>         crc = crc ^ buffer[i];
>         for (j = 0; j < 8; j++)
>             crc = (crc >> 1) ^ 0x80000000 ^ ((~crc & 1) * 0x82f63b78);
>     }
>     return crc;
> }
>
> void hash_test(void);
> void hash_test(void)
> {
> 	printf("CRC of 1 nul byte expected: 0x527d5351\n");
> 	printf("    soft: %08x\n", crc32c_trivial(null_test, 1, 0));
> 	rte_hash_crc_init_alg();
> 	printf("    rte accelerated: %08x\n", ~rte_hash_crc(null_test, 1, 0xFFFFFFFF));
> 	rte_hash_crc_set_alg(CRC32_SW);
> 	printf("    rte soft: %08x\n", ~rte_hash_crc(null_test, 1, 0xFFFFFFFF));
>
> 	printf("CRC of 2 nul bytes expected: 0xf16177d2\n");
> 	printf("    soft: %08x\n", crc32c_trivial(null_test, 2, 0));
> 	rte_hash_crc_init_alg();
> 	printf("    rte accelerated: %08x\n", ~rte_hash_crc(null_test, 2, 0xFFFFFFFF));
> 	rte_hash_crc_set_alg(CRC32_SW);
> 	printf("    rte soft: %08x\n", ~rte_hash_crc(null_test, 2, 0xFFFFFFFF));
>
> 	printf("CRC of 2x1 nul bytes expected: 0xf16177d2\n");
> 	printf("    soft: %08x\n", crc32c_trivial(null_test, 1, crc32c_trivial(null_test, 1, 0)));
> 	rte_hash_crc_init_alg();
> 	printf("    rte accelerated: %08x\n", ~rte_hash_crc(null_test, 1, rte_hash_crc(null_test, 1, 0xFFFFFFFF)));
> 	rte_hash_crc_set_alg(CRC32_SW);
> 	printf("    rte soft: %08x\n", ~rte_hash_crc(null_test, 1, rte_hash_crc(null_test, 1, 0xFFFFFFFF)));
>
> 	printf("CRC of 3 nul bytes expected: 0x6064a37a\n");
> 	printf("    soft: %08x\n", crc32c_trivial(null_test, 3, 0));
> 	rte_hash_crc_init_alg();
> 	printf("    rte accelerated: %08x\n", ~rte_hash_crc(null_test, 3, 0xFFFFFFFF));
> 	rte_hash_crc_set_alg(CRC32_SW);
> 	printf("    rte soft: %08x\n", ~rte_hash_crc(null_test, 3, 0xFFFFFFFF));
>
> 	printf("CRC of 4 nul bytes expected: 0x48674bc7\n");
> 	printf("    soft: %08x\n", crc32c_trivial(null_test, 4, 0));
> 	rte_hash_crc_init_alg();
> 	printf("    rte accelerated: %08x\n", ~rte_hash_crc(null_test, 4, 0xFFFFFFFF));
> 	rte_hash_crc_set_alg(CRC32_SW);
> 	printf("    rte soft: %08x\n", ~rte_hash_crc(null_test, 4, 0xFFFFFFFF));
> }
>
> Signed-off-by: Didier Pallard <didier.pallard@6wind.com>
> Acked-by: David Marchand <david.marchand@6wind.com>
> ---
>  lib/librte_hash/rte_crc_arm64.h |  64 ++++++++++++++++++++
>  lib/librte_hash/rte_hash_crc.h  | 125 +++++++++++++++++++++++++++++++---------
>  2 files changed, 162 insertions(+), 27 deletions(-)
>
> diff --git a/lib/librte_hash/rte_crc_arm64.h b/lib/librte_hash/rte_crc_arm64.h
> index 02e26bc..44ef460 100644
> --- a/lib/librte_hash/rte_crc_arm64.h
> +++ b/lib/librte_hash/rte_crc_arm64.h
> @@ -50,6 +50,28 @@ extern "C" {
>  #include <rte_common.h>
>  
>  static inline uint32_t
> +crc32c_arm64_u8(uint8_t data, uint32_t init_val)
> +{
> +	asm(".arch armv8-a+crc");
> +	__asm__ volatile(
> +			"crc32cb %w[crc], %w[crc], %b[value]"
> +			: [crc] "+r" (init_val)
> +			: [value] "r" (data));
> +	return init_val;
> +}
> +
> +static inline uint32_t
> +crc32c_arm64_u16(uint16_t data, uint32_t init_val)
> +{
> +	asm(".arch armv8-a+crc");
> +	__asm__ volatile(
> +			"crc32ch %w[crc], %w[crc], %h[value]"
> +			: [crc] "+r" (init_val)
> +			: [value] "r" (data));
> +	return init_val;
> +}
> +
> +static inline uint32_t
>  crc32c_arm64_u32(uint32_t data, uint32_t init_val)
>  {
>  	asm(".arch armv8-a+crc");
> @@ -103,6 +125,48 @@ rte_hash_crc_init_alg(void)
>  }
>  
>  /**
> + * Use single crc32 instruction to perform a hash on a 1 byte value.
> + * Fall back to software crc32 implementation in case arm64 crc intrinsics is
> + * not supported
> + *
> + * @param data
> + *   Data to perform hash on.
> + * @param init_val
> + *   Value to initialise hash generator.
> + * @return
> + *   32bit calculated hash value.
> + */
> +static inline uint32_t
> +rte_hash_crc_1byte(uint8_t data, uint32_t init_val)
> +{
> +	if (likely(crc32_alg & CRC32_ARM64))
> +		return crc32c_arm64_u8(data, init_val);
> +
> +	return crc32c_1byte(data, init_val);
> +}
> +
> +/**
> + * Use single crc32 instruction to perform a hash on a 2 bytes value.
> + * Fall back to software crc32 implementation in case arm64 crc intrinsics is
> + * not supported
> + *
> + * @param data
> + *   Data to perform hash on.
> + * @param init_val
> + *   Value to initialise hash generator.
> + * @return
> + *   32bit calculated hash value.
> + */
> +static inline uint32_t
> +rte_hash_crc_2byte(uint16_t data, uint32_t init_val)
> +{
> +	if (likely(crc32_alg & CRC32_ARM64))
> +		return crc32c_arm64_u16(data, init_val);
> +
> +	return crc32c_2bytes(data, init_val);
> +}
> +
> +/**
>   * Use single crc32 instruction to perform a hash on a 4 byte value.
>   * Fall back to software crc32 implementation in case arm64 crc intrinsics is
>   * not supported
> diff --git a/lib/librte_hash/rte_hash_crc.h b/lib/librte_hash/rte_hash_crc.h
> index 78a34b7..485f8a2 100644
> --- a/lib/librte_hash/rte_hash_crc.h
> +++ b/lib/librte_hash/rte_hash_crc.h
> @@ -328,6 +328,28 @@ static const uint32_t crc32c_tables[8][256] = {{
>  	 crc32c_tables[(n)-1][((crc) >> 8) & 0xFF])
>  
>  static inline uint32_t
> +crc32c_1byte(uint8_t data, uint32_t init_val)
> +{
> +	uint32_t crc;
> +	crc = init_val;
> +	crc ^= data;
> +
> +	return crc32c_tables[0][crc & 0xff] ^ (crc >> 8);
> +}
> +
> +static inline uint32_t
> +crc32c_2bytes(uint16_t data, uint32_t init_val)
> +{
> +	uint32_t crc;
> +	crc = init_val;
> +	crc ^= data;
> +
> +	crc = CRC32_UPD(crc, 1) ^ (crc >> 16);
> +
> +	return crc;
> +}
> +
> +static inline uint32_t
>  crc32c_1word(uint32_t data, uint32_t init_val)
>  {
>  	uint32_t crc, term1, term2;
> @@ -367,6 +389,26 @@ crc32c_2words(uint64_t data, uint32_t init_val)
>  
>  #if defined(RTE_ARCH_I686) || defined(RTE_ARCH_X86_64)
>  static inline uint32_t
> +crc32c_sse42_u8(uint8_t data, uint32_t init_val)
> +{
> +	__asm__ volatile(
> +			"crc32b %[data], %[init_val];"
> +			: [init_val] "+r" (init_val)
> +			: [data] "rm" (data));
> +	return init_val;
> +}
> +
> +static inline uint32_t
> +crc32c_sse42_u16(uint16_t data, uint32_t init_val)
> +{
> +	__asm__ volatile(
> +			"crc32w %[data], %[init_val];"
> +			: [init_val] "+r" (init_val)
> +			: [data] "rm" (data));
> +	return init_val;
> +}
> +
> +static inline uint32_t
>  crc32c_sse42_u32(uint32_t data, uint32_t init_val)
>  {
>  	__asm__ volatile(
> @@ -453,6 +495,52 @@ rte_hash_crc_init_alg(void)
>  }
>  
>  /**
> + * Use single crc32 instruction to perform a hash on a byte value.
> + * Fall back to software crc32 implementation in case SSE4.2 is
> + * not supported
> + *
> + * @param data
> + *   Data to perform hash on.
> + * @param init_val
> + *   Value to initialise hash generator.
> + * @return
> + *   32bit calculated hash value.
> + */
> +static inline uint32_t
> +rte_hash_crc_1byte(uint8_t data, uint32_t init_val)
> +{
> +#if defined RTE_ARCH_I686 || defined RTE_ARCH_X86_64
> +	if (likely(crc32_alg & CRC32_SSE42))
> +		return crc32c_sse42_u8(data, init_val);
> +#endif
> +
> +	return crc32c_1byte(data, init_val);
> +}
> +
> +/**
> + * Use single crc32 instruction to perform a hash on a byte value.
> + * Fall back to software crc32 implementation in case SSE4.2 is
> + * not supported
> + *
> + * @param data
> + *   Data to perform hash on.
> + * @param init_val
> + *   Value to initialise hash generator.
> + * @return
> + *   32bit calculated hash value.
> + */
> +static inline uint32_t
> +rte_hash_crc_2byte(uint16_t data, uint32_t init_val)
> +{
> +#if defined RTE_ARCH_I686 || defined RTE_ARCH_X86_64
> +	if (likely(crc32_alg & CRC32_SSE42))
> +		return crc32c_sse42_u16(data, init_val);
> +#endif
> +
> +	return crc32c_2bytes(data, init_val);
> +}
> +
> +/**
>   * Use single crc32 instruction to perform a hash on a 4 byte value.
>   * Fall back to software crc32 implementation in case SSE4.2 is
>   * not supported
> @@ -521,7 +609,6 @@ static inline uint32_t
>  rte_hash_crc(const void *data, uint32_t data_len, uint32_t init_val)
>  {
>  	unsigned i;
> -	uint64_t temp = 0;
>  	uintptr_t pd = (uintptr_t) data;
>  
>  	for (i = 0; i < data_len / 8; i++) {
> @@ -529,35 +616,19 @@ rte_hash_crc(const void *data, uint32_t data_len, uint32_t init_val)
>  		pd += 8;
>  	}
>  
> -	switch (7 - (data_len & 0x07)) {
> -	case 0:
> -		temp |= (uint64_t) *((const uint8_t *)pd + 6) << 48;
> -		/* Fallthrough */
> -	case 1:
> -		temp |= (uint64_t) *((const uint8_t *)pd + 5) << 40;
> -		/* Fallthrough */
> -	case 2:
> -		temp |= (uint64_t) *((const uint8_t *)pd + 4) << 32;
> -		temp |= *(const uint32_t *)pd;
> -		init_val = rte_hash_crc_8byte(temp, init_val);
> -		break;
> -	case 3:
> +	if (data_len & 0x4) {
>  		init_val = rte_hash_crc_4byte(*(const uint32_t *)pd, init_val);
> -		break;
> -	case 4:
> -		temp |= *((const uint8_t *)pd + 2) << 16;
> -		/* Fallthrough */
> -	case 5:
> -		temp |= *((const uint8_t *)pd + 1) << 8;
> -		/* Fallthrough */
> -	case 6:
> -		temp |= *(const uint8_t *)pd;
> -		init_val = rte_hash_crc_4byte(temp, init_val);
> -		/* Fallthrough */
> -	default:
> -		break;
> +		pd += 4;
> +	}
> +
> +	if (data_len & 0x2) {
> +		init_val = rte_hash_crc_2byte(*(const uint16_t *)pd, init_val);
> +		pd += 2;
>  	}
>  
> +	if (data_len & 0x1)
> +		init_val = rte_hash_crc_1byte(*(const uint8_t *)pd, init_val);
> +
>  	return init_val;
>  }
>  

Re: [dpdk-dev] [PATCH] hash: fix CRC32c computation

[Vincent JARDIN]

Le 23 déc. 2015 10:12, "Qiu, Michael" <michael.qiu@intel.com> a écrit :
>
> Is it suitable to put so many code in commit log?

It is more explicit than a text/comment. I do not think it should be
maintained code.

>
> Thanks,
> Michael
> On 12/22/2015 5:36 PM, Didier Pallard wrote:
> > As demonstrated by the following code, CRC32c computation is not valid
> > when buffer length is not a multiple of 4 bytes:
> > (Output obtained by code below)
> >
> > CRC of 1 NULL bytes expected: 0x527d5351
> >     soft: 527d5351
> >     rte accelerated: 48674bc7
> >     rte soft: 48674bc7
> > CRC of 2 NULL bytes expected: 0xf16177d2
> >     soft: f16177d2
> >     rte accelerated: 48674bc7
> >     rte soft: 48674bc7
> > CRC of 2x1 NULL bytes expected: 0xf16177d2
> >     soft: f16177d2
> >     rte accelerated: 8c28b28a
> >     rte soft: 8c28b28a
> > CRC of 3 NULL bytes expected: 0x6064a37a
> >     soft: 6064a37a
> >     rte accelerated: 48674bc7
> >     rte soft: 48674bc7
> > CRC of 4 NULL bytes expected: 0x48674bc7
> >     soft: 48674bc7
> >     rte accelerated: 48674bc7
> >     rte soft: 48674bc7
> >
> > Values returned by rte_hash_crc functions does not match the one
> > computed by a trivial crc32c implementation.
> >
> > ARM code is a guess, it is not tested, neither compiled.
> >
> > code showing the problem:
> >
> > uint8_t null_test[32] = {0};
> >
> > static uint32_t crc32c_trivial(uint8_t *buffer, uint32_t length,
uint32_t crc)
> > {
> >     uint32_t i, j;
> >     for (i = 0; i < length; ++i)
> >     {
> >         crc = crc ^ buffer[i];
> >         for (j = 0; j < 8; j++)
> >             crc = (crc >> 1) ^ 0x80000000 ^ ((~crc & 1) * 0x82f63b78);
> >     }
> >     return crc;
> > }
> >
> > void hash_test(void);
> > void hash_test(void)
> > {
> >       printf("CRC of 1 nul byte expected: 0x527d5351\n");
> >       printf("    soft: %08x\n", crc32c_trivial(null_test, 1, 0));
> >       rte_hash_crc_init_alg();
> >       printf("    rte accelerated: %08x\n", ~rte_hash_crc(null_test, 1,
0xFFFFFFFF));
> >       rte_hash_crc_set_alg(CRC32_SW);
> >       printf("    rte soft: %08x\n", ~rte_hash_crc(null_test, 1,
0xFFFFFFFF));
> >
> >       printf("CRC of 2 nul bytes expected: 0xf16177d2\n");
> >       printf("    soft: %08x\n", crc32c_trivial(null_test, 2, 0));
> >       rte_hash_crc_init_alg();
> >       printf("    rte accelerated: %08x\n", ~rte_hash_crc(null_test, 2,
0xFFFFFFFF));
> >       rte_hash_crc_set_alg(CRC32_SW);
> >       printf("    rte soft: %08x\n", ~rte_hash_crc(null_test, 2,
0xFFFFFFFF));
> >
> >       printf("CRC of 2x1 nul bytes expected: 0xf16177d2\n");
> >       printf("    soft: %08x\n", crc32c_trivial(null_test, 1,
crc32c_trivial(null_test, 1, 0)));
> >       rte_hash_crc_init_alg();
> >       printf("    rte accelerated: %08x\n", ~rte_hash_crc(null_test, 1,
rte_hash_crc(null_test, 1, 0xFFFFFFFF)));
> >       rte_hash_crc_set_alg(CRC32_SW);
> >       printf("    rte soft: %08x\n", ~rte_hash_crc(null_test, 1,
rte_hash_crc(null_test, 1, 0xFFFFFFFF)));
> >
> >       printf("CRC of 3 nul bytes expected: 0x6064a37a\n");
> >       printf("    soft: %08x\n", crc32c_trivial(null_test, 3, 0));
> >       rte_hash_crc_init_alg();
> >       printf("    rte accelerated: %08x\n", ~rte_hash_crc(null_test, 3,
0xFFFFFFFF));
> >       rte_hash_crc_set_alg(CRC32_SW);
> >       printf("    rte soft: %08x\n", ~rte_hash_crc(null_test, 3,
0xFFFFFFFF));
> >
> >       printf("CRC of 4 nul bytes expected: 0x48674bc7\n");
> >       printf("    soft: %08x\n", crc32c_trivial(null_test, 4, 0));
> >       rte_hash_crc_init_alg();
> >       printf("    rte accelerated: %08x\n", ~rte_hash_crc(null_test, 4,
0xFFFFFFFF));
> >       rte_hash_crc_set_alg(CRC32_SW);
> >       printf("    rte soft: %08x\n", ~rte_hash_crc(null_test, 4,
0xFFFFFFFF));
> > }
> >
> > Signed-off-by: Didier Pallard <didier.pallard@6wind.com>
> > Acked-by: David Marchand <david.marchand@6wind.com>
> > ---
> >  lib/librte_hash/rte_crc_arm64.h |  64 ++++++++++++++++++++
> >  lib/librte_hash/rte_hash_crc.h  | 125
+++++++++++++++++++++++++++++++---------
> >  2 files changed, 162 insertions(+), 27 deletions(-)
> >
> > diff --git a/lib/librte_hash/rte_crc_arm64.h
b/lib/librte_hash/rte_crc_arm64.h
> > index 02e26bc..44ef460 100644
> > --- a/lib/librte_hash/rte_crc_arm64.h
> > +++ b/lib/librte_hash/rte_crc_arm64.h
> > @@ -50,6 +50,28 @@ extern "C" {
> >  #include <rte_common.h>
> >
> >  static inline uint32_t
> > +crc32c_arm64_u8(uint8_t data, uint32_t init_val)
> > +{
> > +     asm(".arch armv8-a+crc");
> > +     __asm__ volatile(
> > +                     "crc32cb %w[crc], %w[crc], %b[value]"
> > +                     : [crc] "+r" (init_val)
> > +                     : [value] "r" (data));
> > +     return init_val;
> > +}
> > +
> > +static inline uint32_t
> > +crc32c_arm64_u16(uint16_t data, uint32_t init_val)
> > +{
> > +     asm(".arch armv8-a+crc");
> > +     __asm__ volatile(
> > +                     "crc32ch %w[crc], %w[crc], %h[value]"
> > +                     : [crc] "+r" (init_val)
> > +                     : [value] "r" (data));
> > +     return init_val;
> > +}
> > +
> > +static inline uint32_t
> >  crc32c_arm64_u32(uint32_t data, uint32_t init_val)
> >  {
> >       asm(".arch armv8-a+crc");
> > @@ -103,6 +125,48 @@ rte_hash_crc_init_alg(void)
> >  }
> >
> >  /**
> > + * Use single crc32 instruction to perform a hash on a 1 byte value.
> > + * Fall back to software crc32 implementation in case arm64 crc
intrinsics is
> > + * not supported
> > + *
> > + * @param data
> > + *   Data to perform hash on.
> > + * @param init_val
> > + *   Value to initialise hash generator.
> > + * @return
> > + *   32bit calculated hash value.
> > + */
> > +static inline uint32_t
> > +rte_hash_crc_1byte(uint8_t data, uint32_t init_val)
> > +{
> > +     if (likely(crc32_alg & CRC32_ARM64))
> > +             return crc32c_arm64_u8(data, init_val);
> > +
> > +     return crc32c_1byte(data, init_val);
> > +}
> > +
> > +/**
> > + * Use single crc32 instruction to perform a hash on a 2 bytes value.
> > + * Fall back to software crc32 implementation in case arm64 crc
intrinsics is
> > + * not supported
> > + *
> > + * @param data
> > + *   Data to perform hash on.
> > + * @param init_val
> > + *   Value to initialise hash generator.
> > + * @return
> > + *   32bit calculated hash value.
> > + */
> > +static inline uint32_t
> > +rte_hash_crc_2byte(uint16_t data, uint32_t init_val)
> > +{
> > +     if (likely(crc32_alg & CRC32_ARM64))
> > +             return crc32c_arm64_u16(data, init_val);
> > +
> > +     return crc32c_2bytes(data, init_val);
> > +}
> > +
> > +/**
> >   * Use single crc32 instruction to perform a hash on a 4 byte value.
> >   * Fall back to software crc32 implementation in case arm64 crc
intrinsics is
> >   * not supported
> > diff --git a/lib/librte_hash/rte_hash_crc.h
b/lib/librte_hash/rte_hash_crc.h
> > index 78a34b7..485f8a2 100644
> > --- a/lib/librte_hash/rte_hash_crc.h
> > +++ b/lib/librte_hash/rte_hash_crc.h
> > @@ -328,6 +328,28 @@ static const uint32_t crc32c_tables[8][256] = {{
> >        crc32c_tables[(n)-1][((crc) >> 8) & 0xFF])
> >
> >  static inline uint32_t
> > +crc32c_1byte(uint8_t data, uint32_t init_val)
> > +{
> > +     uint32_t crc;
> > +     crc = init_val;
> > +     crc ^= data;
> > +
> > +     return crc32c_tables[0][crc & 0xff] ^ (crc >> 8);
> > +}
> > +
> > +static inline uint32_t
> > +crc32c_2bytes(uint16_t data, uint32_t init_val)
> > +{
> > +     uint32_t crc;
> > +     crc = init_val;
> > +     crc ^= data;
> > +
> > +     crc = CRC32_UPD(crc, 1) ^ (crc >> 16);
> > +
> > +     return crc;
> > +}
> > +
> > +static inline uint32_t
> >  crc32c_1word(uint32_t data, uint32_t init_val)
> >  {
> >       uint32_t crc, term1, term2;
> > @@ -367,6 +389,26 @@ crc32c_2words(uint64_t data, uint32_t init_val)
> >
> >  #if defined(RTE_ARCH_I686) || defined(RTE_ARCH_X86_64)
> >  static inline uint32_t
> > +crc32c_sse42_u8(uint8_t data, uint32_t init_val)
> > +{
> > +     __asm__ volatile(
> > +                     "crc32b %[data], %[init_val];"
> > +                     : [init_val] "+r" (init_val)
> > +                     : [data] "rm" (data));
> > +     return init_val;
> > +}
> > +
> > +static inline uint32_t
> > +crc32c_sse42_u16(uint16_t data, uint32_t init_val)
> > +{
> > +     __asm__ volatile(
> > +                     "crc32w %[data], %[init_val];"
> > +                     : [init_val] "+r" (init_val)
> > +                     : [data] "rm" (data));
> > +     return init_val;
> > +}
> > +
> > +static inline uint32_t
> >  crc32c_sse42_u32(uint32_t data, uint32_t init_val)
> >  {
> >       __asm__ volatile(
> > @@ -453,6 +495,52 @@ rte_hash_crc_init_alg(void)
> >  }
> >
> >  /**
> > + * Use single crc32 instruction to perform a hash on a byte value.
> > + * Fall back to software crc32 implementation in case SSE4.2 is
> > + * not supported
> > + *
> > + * @param data
> > + *   Data to perform hash on.
> > + * @param init_val
> > + *   Value to initialise hash generator.
> > + * @return
> > + *   32bit calculated hash value.
> > + */
> > +static inline uint32_t
> > +rte_hash_crc_1byte(uint8_t data, uint32_t init_val)
> > +{
> > +#if defined RTE_ARCH_I686 || defined RTE_ARCH_X86_64
> > +     if (likely(crc32_alg & CRC32_SSE42))
> > +             return crc32c_sse42_u8(data, init_val);
> > +#endif
> > +
> > +     return crc32c_1byte(data, init_val);
> > +}
> > +
> > +/**
> > + * Use single crc32 instruction to perform a hash on a byte value.
> > + * Fall back to software crc32 implementation in case SSE4.2 is
> > + * not supported
> > + *
> > + * @param data
> > + *   Data to perform hash on.
> > + * @param init_val
> > + *   Value to initialise hash generator.
> > + * @return
> > + *   32bit calculated hash value.
> > + */
> > +static inline uint32_t
> > +rte_hash_crc_2byte(uint16_t data, uint32_t init_val)
> > +{
> > +#if defined RTE_ARCH_I686 || defined RTE_ARCH_X86_64
> > +     if (likely(crc32_alg & CRC32_SSE42))
> > +             return crc32c_sse42_u16(data, init_val);
> > +#endif
> > +
> > +     return crc32c_2bytes(data, init_val);
> > +}
> > +
> > +/**
> >   * Use single crc32 instruction to perform a hash on a 4 byte value.
> >   * Fall back to software crc32 implementation in case SSE4.2 is
> >   * not supported
> > @@ -521,7 +609,6 @@ static inline uint32_t
> >  rte_hash_crc(const void *data, uint32_t data_len, uint32_t init_val)
> >  {
> >       unsigned i;
> > -     uint64_t temp = 0;
> >       uintptr_t pd = (uintptr_t) data;
> >
> >       for (i = 0; i < data_len / 8; i++) {
> > @@ -529,35 +616,19 @@ rte_hash_crc(const void *data, uint32_t data_len,
uint32_t init_val)
> >               pd += 8;
> >       }
> >
> > -     switch (7 - (data_len & 0x07)) {
> > -     case 0:
> > -             temp |= (uint64_t) *((const uint8_t *)pd + 6) << 48;
> > -             /* Fallthrough */
> > -     case 1:
> > -             temp |= (uint64_t) *((const uint8_t *)pd + 5) << 40;
> > -             /* Fallthrough */
> > -     case 2:
> > -             temp |= (uint64_t) *((const uint8_t *)pd + 4) << 32;
> > -             temp |= *(const uint32_t *)pd;
> > -             init_val = rte_hash_crc_8byte(temp, init_val);
> > -             break;
> > -     case 3:
> > +     if (data_len & 0x4) {
> >               init_val = rte_hash_crc_4byte(*(const uint32_t *)pd,
init_val);
> > -             break;
> > -     case 4:
> > -             temp |= *((const uint8_t *)pd + 2) << 16;
> > -             /* Fallthrough */
> > -     case 5:
> > -             temp |= *((const uint8_t *)pd + 1) << 8;
> > -             /* Fallthrough */
> > -     case 6:
> > -             temp |= *(const uint8_t *)pd;
> > -             init_val = rte_hash_crc_4byte(temp, init_val);
> > -             /* Fallthrough */
> > -     default:
> > -             break;
> > +             pd += 4;
> > +     }
> > +
> > +     if (data_len & 0x2) {
> > +             init_val = rte_hash_crc_2byte(*(const uint16_t *)pd,
init_val);
> > +             pd += 2;
> >       }
> >
> > +     if (data_len & 0x1)
> > +             init_val = rte_hash_crc_1byte(*(const uint8_t *)pd,
init_val);
> > +
> >       return init_val;
> >  }
> >
>

Re: [dpdk-dev] [PATCH] hash: fix CRC32c computation

[De Lara Guarch, Pablo]

> -----Original Message-----
> From: De Lara Guarch, Pablo
> Sent: Monday, February 08, 2016 2:40 PM
> To: De Lara Guarch, Pablo
> Subject: FW: [dpdk-dev] [PATCH] hash: fix CRC32c computation
> 
> 
> 
> From: Vincent JARDIN [mailto:vincent.jardin@6wind.com]
> Sent: Wednesday, December 23, 2015 11:38 AM
> To: Qiu, Michael
> Cc: didier. pallard; De Lara Guarch, Pablo; dev@dpdk.org; Richardson, Bruce
> Subject: Re: [dpdk-dev] [PATCH] hash: fix CRC32c computation
> 
> 
> Le 23 déc. 2015 10:12, "Qiu, Michael" <michael.qiu@intel.com> a écrit :
> >
> > Is it suitable to put so many code in commit log?
> It is more explicit than a text/comment. I do not think it should be
> maintained code.
> >
> > Thanks,
> > Michael
> > On 12/22/2015 5:36 PM, Didier Pallard wrote:
> > > As demonstrated by the following code, CRC32c computation is not valid
> > > when buffer length is not a multiple of 4 bytes:
> > > (Output obtained by code below)
> > >
> > > CRC of 1 NULL bytes expected: 0x527d5351
> > >     soft: 527d5351
> > >     rte accelerated: 48674bc7
> > >     rte soft: 48674bc7
> > > CRC of 2 NULL bytes expected: 0xf16177d2
> > >     soft: f16177d2
> > >     rte accelerated: 48674bc7
> > >     rte soft: 48674bc7
> > > CRC of 2x1 NULL bytes expected: 0xf16177d2
> > >     soft: f16177d2
> > >     rte accelerated: 8c28b28a
> > >     rte soft: 8c28b28a
> > > CRC of 3 NULL bytes expected: 0x6064a37a
> > >     soft: 6064a37a
> > >     rte accelerated: 48674bc7
> > >     rte soft: 48674bc7
> > > CRC of 4 NULL bytes expected: 0x48674bc7
> > >     soft: 48674bc7
> > >     rte accelerated: 48674bc7
> > >     rte soft: 48674bc7
> > >
> > > Values returned by rte_hash_crc functions does not match the one
> > > computed by a trivial crc32c implementation.
> > >
> > > ARM code is a guess, it is not tested, neither compiled.

It fails on ARM. Not sure what the correct format is:

/home/pablo/dpdk-2.3-rc0/arm64-armv8a-linuxapp-gcc/include/rte_hash_crc.h: In function ‘rte_hash_crc’:
/home/pablo/dpdk-2.3-rc0/arm64-armv8a-linuxapp-gcc/include/rte_crc_arm64.h:67:2: error: invalid 'asm': incompatible floating point / vector register operand for '%h'
  __asm__ volatile(
  ^
/home/pablo/dpdk-2.3-rc0/arm64-armv8a-linuxapp-gcc/include/rte_crc_arm64.h:56:2: error: invalid 'asm': incompatible floating point / vector register operand for '%b'
  __asm__ volatile(

Thanks,
Pablo

[dpdk-dev] [PATCH v2] hash: fix CRC32c computation

[Didier Pallard]

As demonstrated by the following code, CRC32c computation is not valid
when buffer length is not a multiple of 4 bytes:
(Output obtained by code below)

CRC of 1 NULL bytes expected: 0x527d5351
    soft: 527d5351
    rte accelerated: 48674bc7
    rte soft: 48674bc7
CRC of 2 NULL bytes expected: 0xf16177d2
    soft: f16177d2
    rte accelerated: 48674bc7
    rte soft: 48674bc7
CRC of 2x1 NULL bytes expected: 0xf16177d2
    soft: f16177d2
    rte accelerated: 8c28b28a
    rte soft: 8c28b28a
CRC of 3 NULL bytes expected: 0x6064a37a
    soft: 6064a37a
    rte accelerated: 48674bc7
    rte soft: 48674bc7
CRC of 4 NULL bytes expected: 0x48674bc7
    soft: 48674bc7
    rte accelerated: 48674bc7
    rte soft: 48674bc7

Values returned by rte_hash_crc functions does not match the one
computed by a trivial crc32c implementation.

ARM code is not tested.

code showing the problem:

uint8_t null_test[32] = {0};

static uint32_t crc32c_trivial(uint8_t *buffer, uint32_t length, uint32_t crc)
{
    uint32_t i, j;
    for (i = 0; i < length; ++i)
    {
        crc = crc ^ buffer[i];
        for (j = 0; j < 8; j++)
            crc = (crc >> 1) ^ 0x80000000 ^ ((~crc & 1) * 0x82f63b78);
    }
    return crc;
}

void hash_test(void);
void hash_test(void)
{
	printf("CRC of 1 nul byte expected: 0x527d5351\n");
	printf("    soft: %08x\n", crc32c_trivial(null_test, 1, 0));
	rte_hash_crc_init_alg();
	printf("    rte accelerated: %08x\n", ~rte_hash_crc(null_test, 1, 0xFFFFFFFF));
	rte_hash_crc_set_alg(CRC32_SW);
	printf("    rte soft: %08x\n", ~rte_hash_crc(null_test, 1, 0xFFFFFFFF));

	printf("CRC of 2 nul bytes expected: 0xf16177d2\n");
	printf("    soft: %08x\n", crc32c_trivial(null_test, 2, 0));
	rte_hash_crc_init_alg();
	printf("    rte accelerated: %08x\n", ~rte_hash_crc(null_test, 2, 0xFFFFFFFF));
	rte_hash_crc_set_alg(CRC32_SW);
	printf("    rte soft: %08x\n", ~rte_hash_crc(null_test, 2, 0xFFFFFFFF));

	printf("CRC of 2x1 nul bytes expected: 0xf16177d2\n");
	printf("    soft: %08x\n", crc32c_trivial(null_test, 1, crc32c_trivial(null_test, 1, 0)));
	rte_hash_crc_init_alg();
	printf("    rte accelerated: %08x\n", ~rte_hash_crc(null_test, 1, rte_hash_crc(null_test, 1, 0xFFFFFFFF)));
	rte_hash_crc_set_alg(CRC32_SW);
	printf("    rte soft: %08x\n", ~rte_hash_crc(null_test, 1, rte_hash_crc(null_test, 1, 0xFFFFFFFF)));

	printf("CRC of 3 nul bytes expected: 0x6064a37a\n");
	printf("    soft: %08x\n", crc32c_trivial(null_test, 3, 0));
	rte_hash_crc_init_alg();
	printf("    rte accelerated: %08x\n", ~rte_hash_crc(null_test, 3, 0xFFFFFFFF));
	rte_hash_crc_set_alg(CRC32_SW);
	printf("    rte soft: %08x\n", ~rte_hash_crc(null_test, 3, 0xFFFFFFFF));

	printf("CRC of 4 nul bytes expected: 0x48674bc7\n");
	printf("    soft: %08x\n", crc32c_trivial(null_test, 4, 0));
	rte_hash_crc_init_alg();
	printf("    rte accelerated: %08x\n", ~rte_hash_crc(null_test, 4, 0xFFFFFFFF));
	rte_hash_crc_set_alg(CRC32_SW);
	printf("    rte soft: %08x\n", ~rte_hash_crc(null_test, 4, 0xFFFFFFFF));
}

Signed-off-by: Didier Pallard <didier.pallard@6wind.com>
Acked-by: David Marchand <david.marchand@6wind.com>
---

v2: Fix ARM64 compilation issue.

v1: Initial version.

 lib/librte_hash/rte_crc_arm64.h |  64 ++++++++++++++++++++
 lib/librte_hash/rte_hash_crc.h  | 125 +++++++++++++++++++++++++++++++---------
 2 files changed, 162 insertions(+), 27 deletions(-)

diff --git a/lib/librte_hash/rte_crc_arm64.h b/lib/librte_hash/rte_crc_arm64.h
index 02e26bc..7dd6334 100644
--- a/lib/librte_hash/rte_crc_arm64.h
+++ b/lib/librte_hash/rte_crc_arm64.h
@@ -50,6 +50,28 @@ extern "C" {
 #include <rte_common.h>
 
 static inline uint32_t
+crc32c_arm64_u8(uint8_t data, uint32_t init_val)
+{
+	asm(".arch armv8-a+crc");
+	__asm__ volatile(
+			"crc32cb %w[crc], %w[crc], %w[value]"
+			: [crc] "+r" (init_val)
+			: [value] "r" (data));
+	return init_val;
+}
+
+static inline uint32_t
+crc32c_arm64_u16(uint16_t data, uint32_t init_val)
+{
+	asm(".arch armv8-a+crc");
+	__asm__ volatile(
+			"crc32ch %w[crc], %w[crc], %w[value]"
+			: [crc] "+r" (init_val)
+			: [value] "r" (data));
+	return init_val;
+}
+
+static inline uint32_t
 crc32c_arm64_u32(uint32_t data, uint32_t init_val)
 {
 	asm(".arch armv8-a+crc");
@@ -103,6 +125,48 @@ rte_hash_crc_init_alg(void)
 }
 
 /**
+ * Use single crc32 instruction to perform a hash on a 1 byte value.
+ * Fall back to software crc32 implementation in case arm64 crc intrinsics is
+ * not supported
+ *
+ * @param data
+ *   Data to perform hash on.
+ * @param init_val
+ *   Value to initialise hash generator.
+ * @return
+ *   32bit calculated hash value.
+ */
+static inline uint32_t
+rte_hash_crc_1byte(uint8_t data, uint32_t init_val)
+{
+	if (likely(crc32_alg & CRC32_ARM64))
+		return crc32c_arm64_u8(data, init_val);
+
+	return crc32c_1byte(data, init_val);
+}
+
+/**
+ * Use single crc32 instruction to perform a hash on a 2 bytes value.
+ * Fall back to software crc32 implementation in case arm64 crc intrinsics is
+ * not supported
+ *
+ * @param data
+ *   Data to perform hash on.
+ * @param init_val
+ *   Value to initialise hash generator.
+ * @return
+ *   32bit calculated hash value.
+ */
+static inline uint32_t
+rte_hash_crc_2byte(uint16_t data, uint32_t init_val)
+{
+	if (likely(crc32_alg & CRC32_ARM64))
+		return crc32c_arm64_u16(data, init_val);
+
+	return crc32c_2bytes(data, init_val);
+}
+
+/**
  * Use single crc32 instruction to perform a hash on a 4 byte value.
  * Fall back to software crc32 implementation in case arm64 crc intrinsics is
  * not supported
diff --git a/lib/librte_hash/rte_hash_crc.h b/lib/librte_hash/rte_hash_crc.h
index 78a34b7..485f8a2 100644
--- a/lib/librte_hash/rte_hash_crc.h
+++ b/lib/librte_hash/rte_hash_crc.h
@@ -328,6 +328,28 @@ static const uint32_t crc32c_tables[8][256] = {{
 	 crc32c_tables[(n)-1][((crc) >> 8) & 0xFF])
 
 static inline uint32_t
+crc32c_1byte(uint8_t data, uint32_t init_val)
+{
+	uint32_t crc;
+	crc = init_val;
+	crc ^= data;
+
+	return crc32c_tables[0][crc & 0xff] ^ (crc >> 8);
+}
+
+static inline uint32_t
+crc32c_2bytes(uint16_t data, uint32_t init_val)
+{
+	uint32_t crc;
+	crc = init_val;
+	crc ^= data;
+
+	crc = CRC32_UPD(crc, 1) ^ (crc >> 16);
+
+	return crc;
+}
+
+static inline uint32_t
 crc32c_1word(uint32_t data, uint32_t init_val)
 {
 	uint32_t crc, term1, term2;
@@ -367,6 +389,26 @@ crc32c_2words(uint64_t data, uint32_t init_val)
 
 #if defined(RTE_ARCH_I686) || defined(RTE_ARCH_X86_64)
 static inline uint32_t
+crc32c_sse42_u8(uint8_t data, uint32_t init_val)
+{
+	__asm__ volatile(
+			"crc32b %[data], %[init_val];"
+			: [init_val] "+r" (init_val)
+			: [data] "rm" (data));
+	return init_val;
+}
+
+static inline uint32_t
+crc32c_sse42_u16(uint16_t data, uint32_t init_val)
+{
+	__asm__ volatile(
+			"crc32w %[data], %[init_val];"
+			: [init_val] "+r" (init_val)
+			: [data] "rm" (data));
+	return init_val;
+}
+
+static inline uint32_t
 crc32c_sse42_u32(uint32_t data, uint32_t init_val)
 {
 	__asm__ volatile(
@@ -453,6 +495,52 @@ rte_hash_crc_init_alg(void)
 }
 
 /**
+ * Use single crc32 instruction to perform a hash on a byte value.
+ * Fall back to software crc32 implementation in case SSE4.2 is
+ * not supported
+ *
+ * @param data
+ *   Data to perform hash on.
+ * @param init_val
+ *   Value to initialise hash generator.
+ * @return
+ *   32bit calculated hash value.
+ */
+static inline uint32_t
+rte_hash_crc_1byte(uint8_t data, uint32_t init_val)
+{
+#if defined RTE_ARCH_I686 || defined RTE_ARCH_X86_64
+	if (likely(crc32_alg & CRC32_SSE42))
+		return crc32c_sse42_u8(data, init_val);
+#endif
+
+	return crc32c_1byte(data, init_val);
+}
+
+/**
+ * Use single crc32 instruction to perform a hash on a byte value.
+ * Fall back to software crc32 implementation in case SSE4.2 is
+ * not supported
+ *
+ * @param data
+ *   Data to perform hash on.
+ * @param init_val
+ *   Value to initialise hash generator.
+ * @return
+ *   32bit calculated hash value.
+ */
+static inline uint32_t
+rte_hash_crc_2byte(uint16_t data, uint32_t init_val)
+{
+#if defined RTE_ARCH_I686 || defined RTE_ARCH_X86_64
+	if (likely(crc32_alg & CRC32_SSE42))
+		return crc32c_sse42_u16(data, init_val);
+#endif
+
+	return crc32c_2bytes(data, init_val);
+}
+
+/**
  * Use single crc32 instruction to perform a hash on a 4 byte value.
  * Fall back to software crc32 implementation in case SSE4.2 is
  * not supported
@@ -521,7 +609,6 @@ static inline uint32_t
 rte_hash_crc(const void *data, uint32_t data_len, uint32_t init_val)
 {
 	unsigned i;
-	uint64_t temp = 0;
 	uintptr_t pd = (uintptr_t) data;
 
 	for (i = 0; i < data_len / 8; i++) {
@@ -529,35 +616,19 @@ rte_hash_crc(const void *data, uint32_t data_len, uint32_t init_val)
 		pd += 8;
 	}
 
-	switch (7 - (data_len & 0x07)) {
-	case 0:
-		temp |= (uint64_t) *((const uint8_t *)pd + 6) << 48;
-		/* Fallthrough */
-	case 1:
-		temp |= (uint64_t) *((const uint8_t *)pd + 5) << 40;
-		/* Fallthrough */
-	case 2:
-		temp |= (uint64_t) *((const uint8_t *)pd + 4) << 32;
-		temp |= *(const uint32_t *)pd;
-		init_val = rte_hash_crc_8byte(temp, init_val);
-		break;
-	case 3:
+	if (data_len & 0x4) {
 		init_val = rte_hash_crc_4byte(*(const uint32_t *)pd, init_val);
-		break;
-	case 4:
-		temp |= *((const uint8_t *)pd + 2) << 16;
-		/* Fallthrough */
-	case 5:
-		temp |= *((const uint8_t *)pd + 1) << 8;
-		/* Fallthrough */
-	case 6:
-		temp |= *(const uint8_t *)pd;
-		init_val = rte_hash_crc_4byte(temp, init_val);
-		/* Fallthrough */
-	default:
-		break;
+		pd += 4;
+	}
+
+	if (data_len & 0x2) {
+		init_val = rte_hash_crc_2byte(*(const uint16_t *)pd, init_val);
+		pd += 2;
 	}
 
+	if (data_len & 0x1)
+		init_val = rte_hash_crc_1byte(*(const uint8_t *)pd, init_val);
+
 	return init_val;
 }
 
-- 
2.1.4

Re: [dpdk-dev] [PATCH v2] hash: fix CRC32c computation

[De Lara Guarch, Pablo]

Hi Didier,

> -----Original Message-----
> From: Didier Pallard [mailto:didier.pallard@6wind.com]
> Sent: Tuesday, February 09, 2016 9:34 AM
> To: dev@dpdk.org; Richardson, Bruce; De Lara Guarch, Pablo
> Cc: jean-mickael.guerin@6wind.com; thomas.monjalon@6wind.com
> Subject: [PATCH v2] hash: fix CRC32c computation
> 
> As demonstrated by the following code, CRC32c computation is not valid
> when buffer length is not a multiple of 4 bytes:
> (Output obtained by code below)
> 
> CRC of 1 NULL bytes expected: 0x527d5351
>     soft: 527d5351
>     rte accelerated: 48674bc7
>     rte soft: 48674bc7
> CRC of 2 NULL bytes expected: 0xf16177d2
>     soft: f16177d2
>     rte accelerated: 48674bc7
>     rte soft: 48674bc7
> CRC of 2x1 NULL bytes expected: 0xf16177d2
>     soft: f16177d2
>     rte accelerated: 8c28b28a
>     rte soft: 8c28b28a
> CRC of 3 NULL bytes expected: 0x6064a37a
>     soft: 6064a37a
>     rte accelerated: 48674bc7
>     rte soft: 48674bc7
> CRC of 4 NULL bytes expected: 0x48674bc7
>     soft: 48674bc7
>     rte accelerated: 48674bc7
>     rte soft: 48674bc7
> 
> Values returned by rte_hash_crc functions does not match the one
> computed by a trivial crc32c implementation.
> 
> ARM code is not tested.
> 
> code showing the problem:
> 
> uint8_t null_test[32] = {0};
> 
> static uint32_t crc32c_trivial(uint8_t *buffer, uint32_t length, uint32_t crc)
> {
>     uint32_t i, j;
>     for (i = 0; i < length; ++i)
>     {
>         crc = crc ^ buffer[i];
>         for (j = 0; j < 8; j++)
>             crc = (crc >> 1) ^ 0x80000000 ^ ((~crc & 1) * 0x82f63b78);
>     }
>     return crc;
> }
> 
> void hash_test(void);
> void hash_test(void)
> {
> 	printf("CRC of 1 nul byte expected: 0x527d5351\n");
> 	printf("    soft: %08x\n", crc32c_trivial(null_test, 1, 0));
> 	rte_hash_crc_init_alg();
> 	printf("    rte accelerated: %08x\n", ~rte_hash_crc(null_test, 1,
> 0xFFFFFFFF));
> 	rte_hash_crc_set_alg(CRC32_SW);
> 	printf("    rte soft: %08x\n", ~rte_hash_crc(null_test, 1, 0xFFFFFFFF));
> 
> 	printf("CRC of 2 nul bytes expected: 0xf16177d2\n");
> 	printf("    soft: %08x\n", crc32c_trivial(null_test, 2, 0));
> 	rte_hash_crc_init_alg();
> 	printf("    rte accelerated: %08x\n", ~rte_hash_crc(null_test, 2,
> 0xFFFFFFFF));
> 	rte_hash_crc_set_alg(CRC32_SW);
> 	printf("    rte soft: %08x\n", ~rte_hash_crc(null_test, 2, 0xFFFFFFFF));
> 
> 	printf("CRC of 2x1 nul bytes expected: 0xf16177d2\n");
> 	printf("    soft: %08x\n", crc32c_trivial(null_test, 1,
> crc32c_trivial(null_test, 1, 0)));
> 	rte_hash_crc_init_alg();
> 	printf("    rte accelerated: %08x\n", ~rte_hash_crc(null_test, 1,
> rte_hash_crc(null_test, 1, 0xFFFFFFFF)));
> 	rte_hash_crc_set_alg(CRC32_SW);
> 	printf("    rte soft: %08x\n", ~rte_hash_crc(null_test, 1,
> rte_hash_crc(null_test, 1, 0xFFFFFFFF)));
> 
> 	printf("CRC of 3 nul bytes expected: 0x6064a37a\n");
> 	printf("    soft: %08x\n", crc32c_trivial(null_test, 3, 0));
> 	rte_hash_crc_init_alg();
> 	printf("    rte accelerated: %08x\n", ~rte_hash_crc(null_test, 3,
> 0xFFFFFFFF));
> 	rte_hash_crc_set_alg(CRC32_SW);
> 	printf("    rte soft: %08x\n", ~rte_hash_crc(null_test, 3, 0xFFFFFFFF));
> 
> 	printf("CRC of 4 nul bytes expected: 0x48674bc7\n");
> 	printf("    soft: %08x\n", crc32c_trivial(null_test, 4, 0));
> 	rte_hash_crc_init_alg();
> 	printf("    rte accelerated: %08x\n", ~rte_hash_crc(null_test, 4,
> 0xFFFFFFFF));
> 	rte_hash_crc_set_alg(CRC32_SW);
> 	printf("    rte soft: %08x\n", ~rte_hash_crc(null_test, 4, 0xFFFFFFFF));
> }
> 
> Signed-off-by: Didier Pallard <didier.pallard@6wind.com>
> Acked-by: David Marchand <david.marchand@6wind.com>

It compiles fine now, thanks!
Could you add also tests for not multiple of 4 bytes keys in test_hash_functions.c,
so we make sure from now on that it works (and you can demonstrate that your fix works)?
You could send a patchset with those new tests first and then the fix.

Also, a note in release notes would be welcome :)

Thanks!
Pablo

Re: [dpdk-dev] [PATCH] hash: fix CRC32c computation

[Pattan, Reshma]

Hi,

Small typo.

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Didier Pallard
> Sent: Tuesday, December 22, 2015 9:35 AM
> To: dev@dpdk.org; Richardson, Bruce <bruce.richardson@intel.com>; De Lara
> Guarch, Pablo <pablo.de.lara.guarch@intel.com>
> Subject: [dpdk-dev] [PATCH] hash: fix CRC32c computation
> diff --git a/lib/librte_hash/rte_hash_crc.h b/lib/librte_hash/rte_hash_crc.h index
> 78a34b7..485f8a2 100644
> --- a/lib/librte_hash/rte_hash_crc.h
> +++ b/lib/librte_hash/rte_hash_crc.h
> + * Use single crc32 instruction to perform a hash on a byte value.

"on a byte value" should be ===> "on 2 bytes".

> + * Fall back to software crc32 implementation in case SSE4.2 is
> + * not supported
> + *
> + * @param data
> + *   Data to perform hash on.
> + * @param init_val
> + *   Value to initialise hash generator.
> + * @return
> + *   32bit calculated hash value.
> + */
> +static inline uint32_t
> +rte_hash_crc_2byte(uint16_t data, uint32_t init_val) { #if defined
> +RTE_ARCH_I686 || defined RTE_ARCH_X86_64
> +	if (likely(crc32_alg & CRC32_SSE42))
> +		return crc32c_sse42_u16(data, init_val); #endif
> +
> +	return crc32c_2bytes(data, init_val);
> +}
> +

Thanks,
Reshma

Re: [dpdk-dev] [PATCH] hash: fix CRC32c computation

[Didier Pallard]

Ok, i will fix the typo and propose a second serie of patch including
unit tests.

thanks

On 02/10/2016 03:35 PM, Pattan, Reshma wrote:
> Hi,
>
> Small typo.
>
>> -----Original Message-----
>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Didier Pallard
>> Sent: Tuesday, December 22, 2015 9:35 AM
>> To: dev@dpdk.org; Richardson, Bruce <bruce.richardson@intel.com>; De Lara
>> Guarch, Pablo <pablo.de.lara.guarch@intel.com>
>> Subject: [dpdk-dev] [PATCH] hash: fix CRC32c computation
>> diff --git a/lib/librte_hash/rte_hash_crc.h b/lib/librte_hash/rte_hash_crc.h index
>> 78a34b7..485f8a2 100644
>> --- a/lib/librte_hash/rte_hash_crc.h
>> +++ b/lib/librte_hash/rte_hash_crc.h
>> + * Use single crc32 instruction to perform a hash on a byte value.
> "on a byte value" should be ===> "on 2 bytes".
>
>> + * Fall back to software crc32 implementation in case SSE4.2 is
>> + * not supported
>> + *
>> + * @param data
>> + *   Data to perform hash on.
>> + * @param init_val
>> + *   Value to initialise hash generator.
>> + * @return
>> + *   32bit calculated hash value.
>> + */
>> +static inline uint32_t
>> +rte_hash_crc_2byte(uint16_t data, uint32_t init_val) { #if defined
>> +RTE_ARCH_I686 || defined RTE_ARCH_X86_64
>> +	if (likely(crc32_alg & CRC32_SSE42))
>> +		return crc32c_sse42_u16(data, init_val); #endif
>> +
>> +	return crc32c_2bytes(data, init_val);
>> +}
>> +
> Thanks,
> Reshma


-- 
Didier PALLARD
6WIND
Software Engineer

Tel: +33 1 39 30 92 46
Mob: +33 6 49 11 40 14
Fax: +33 1 39 30 92 11
didier.pallard@6wind.com
www.6wind.com

This e-mail message, including any attachments, is for the sole use of the intended recipient(s) and contains information that is confidential and proprietary to 6WIND. All unauthorized review, use, disclosure or distribution is prohibited. If you are not the intended recipient, please contact the sender by reply e-mail and destroy all copies of the original message.

Ce courriel ainsi que toutes les pièces jointes, est uniquement destiné à son ou ses destinataires. Il contient des informations confidentielles qui sont la propriété de 6WIND. Toute révélation, distribution ou copie des informations qu'il contient est strictement interdite. Si vous avez reçu ce message par erreur, veuillez immédiatement le signaler à l'émetteur et détruire toutes les données reçues

[dpdk-dev] [PATCH v3 0/2] Fix CRC32c computation

[Didier Pallard]

CRC32c computation is not valid when buffer length is not a multiple of 4 bytes.
Values returned by rte_hash_crc functions does not match the one
computed by a trivial crc32c implementation.

First patch fixes crc hash function autotests, to outline the problem.
Second patch fixes CRC32c computation.

Didier Pallard (2):
  test: fix CRC hash function autotest
  hash: fix CRC32c computation

 app/test/test_hash_functions.c         |  17 +++--
 doc/guides/rel_notes/release_16_04.rst |   5 ++
 lib/librte_hash/rte_crc_arm64.h        |  64 +++++++++++++++++
 lib/librte_hash/rte_hash_crc.h         | 125 ++++++++++++++++++++++++++-------
 4 files changed, 178 insertions(+), 33 deletions(-)

-- 
2.1.4

[dpdk-dev] [PATCH v3 1/2] test: fix CRC hash function autotest

[Didier Pallard]

Add some small key lengthes (below 4 bytes), and fix odd key lengthes
expected returned values for CRC computation to match real CRC values.

Signed-off-by: Didier Pallard <didier.pallard@6wind.com>
---
 app/test/test_hash_functions.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/app/test/test_hash_functions.c b/app/test/test_hash_functions.c
index 3ad6d80..f767a48 100644
--- a/app/test/test_hash_functions.c
+++ b/app/test/test_hash_functions.c
@@ -54,26 +54,30 @@
  * e.g.: key size = 4, key = 0x03020100
  *       key size = 8, key = 0x0706050403020100
  */
-static uint32_t hash_values_jhash[2][10] = {{
+static uint32_t hash_values_jhash[2][12] = {{
+	0x8ba9414b, 0xdf0d39c9,
 	0xe4cf1d42, 0xd4ccb93c, 0x5e84eafc, 0x21362cfe,
 	0x2f4775ab, 0x9ff036cc, 0xeca51474, 0xbc9d6816,
 	0x12926a31, 0x1c9fa888
 },
 {
+	0x5c62c303, 0x1b8cf784,
 	0x8270ac65, 0x05fa6668, 0x762df861, 0xda088f2f,
 	0x59614cd4, 0x7a94f690, 0xdc1e4993, 0x30825494,
 	0x91d0e462, 0x768087fc
 }
 };
-static uint32_t hash_values_crc[2][10] = {{
+static uint32_t hash_values_crc[2][12] = {{
+	0x00000000, 0xf26b8303,
 	0x91545164, 0x06040eb1, 0x9bb99201, 0xcc4c4fe4,
-	0x14a90993, 0xf8a5dd8c, 0xc62beb31, 0x32bf340e,
-	0x72f9d22b, 0x4a11475e
+	0x14a90993, 0xf8a5dd8c, 0xcaa1ad0b, 0x7ac1e03e,
+	0x43f44466, 0x4a11475e
 },
 {
+	0xbdfd3980, 0x70204542,
 	0x98cd4c70, 0xd52c702f, 0x41fc0e1c, 0x3905f65c,
-	0x94bff47f, 0x1bab102d, 0xd2911ed7, 0xe8faa813,
-	0x6bea184b, 0x53028d3e
+	0x94bff47f, 0x1bab102d, 0xf4a2c645, 0xbf441539,
+	0x789c104f, 0x53028d3e
 }
 };
 
@@ -89,6 +93,7 @@ static uint32_t hash_values_crc[2][10] = {{
 static rte_hash_function hashtest_funcs[] = {rte_jhash, rte_hash_crc};
 static uint32_t hashtest_initvals[] = {0, 0xdeadbeef};
 static uint32_t hashtest_key_lens[] = {
+	1, 2,                 /* Unusual key sizes */
 	4, 8, 16, 32, 48, 64, /* standard key sizes */
 	9,                    /* IPv4 SRC + DST + protocol, unpadded */
 	13,                   /* IPv4 5-tuple, unpadded */
-- 
2.1.4

[dpdk-dev] [PATCH v3 2/2] hash: fix CRC32c computation

[Didier Pallard]

Fix crc32c hash functions to return a valid crc32c value for
data lengthes not multiple of 4 bytes.
ARM code is not tested.

Signed-off-by: Didier Pallard <didier.pallard@6wind.com>
Acked-by: David Marchand <david.marchand@6wind.com>
---
 doc/guides/rel_notes/release_16_04.rst |   5 ++
 lib/librte_hash/rte_crc_arm64.h        |  64 +++++++++++++++++
 lib/librte_hash/rte_hash_crc.h         | 125 ++++++++++++++++++++++++++-------
 3 files changed, 167 insertions(+), 27 deletions(-)

diff --git a/doc/guides/rel_notes/release_16_04.rst b/doc/guides/rel_notes/release_16_04.rst
index eb1b3b2..45b35dd 100644
--- a/doc/guides/rel_notes/release_16_04.rst
+++ b/doc/guides/rel_notes/release_16_04.rst
@@ -68,6 +68,11 @@ Drivers
 Libraries
 ~~~~~~~~~
 
+* **hash: Fixed CRC32c hash computation for non multiple of 4 bytes sizes.**
+
+  Fix crc32c hash functions to return a valid crc32c value for data lengthes
+  not multiple of 4 bytes.
+
 
 Examples
 ~~~~~~~~
diff --git a/lib/librte_hash/rte_crc_arm64.h b/lib/librte_hash/rte_crc_arm64.h
index 02e26bc..7dd6334 100644
--- a/lib/librte_hash/rte_crc_arm64.h
+++ b/lib/librte_hash/rte_crc_arm64.h
@@ -50,6 +50,28 @@ extern "C" {
 #include <rte_common.h>
 
 static inline uint32_t
+crc32c_arm64_u8(uint8_t data, uint32_t init_val)
+{
+	asm(".arch armv8-a+crc");
+	__asm__ volatile(
+			"crc32cb %w[crc], %w[crc], %w[value]"
+			: [crc] "+r" (init_val)
+			: [value] "r" (data));
+	return init_val;
+}
+
+static inline uint32_t
+crc32c_arm64_u16(uint16_t data, uint32_t init_val)
+{
+	asm(".arch armv8-a+crc");
+	__asm__ volatile(
+			"crc32ch %w[crc], %w[crc], %w[value]"
+			: [crc] "+r" (init_val)
+			: [value] "r" (data));
+	return init_val;
+}
+
+static inline uint32_t
 crc32c_arm64_u32(uint32_t data, uint32_t init_val)
 {
 	asm(".arch armv8-a+crc");
@@ -103,6 +125,48 @@ rte_hash_crc_init_alg(void)
 }
 
 /**
+ * Use single crc32 instruction to perform a hash on a 1 byte value.
+ * Fall back to software crc32 implementation in case arm64 crc intrinsics is
+ * not supported
+ *
+ * @param data
+ *   Data to perform hash on.
+ * @param init_val
+ *   Value to initialise hash generator.
+ * @return
+ *   32bit calculated hash value.
+ */
+static inline uint32_t
+rte_hash_crc_1byte(uint8_t data, uint32_t init_val)
+{
+	if (likely(crc32_alg & CRC32_ARM64))
+		return crc32c_arm64_u8(data, init_val);
+
+	return crc32c_1byte(data, init_val);
+}
+
+/**
+ * Use single crc32 instruction to perform a hash on a 2 bytes value.
+ * Fall back to software crc32 implementation in case arm64 crc intrinsics is
+ * not supported
+ *
+ * @param data
+ *   Data to perform hash on.
+ * @param init_val
+ *   Value to initialise hash generator.
+ * @return
+ *   32bit calculated hash value.
+ */
+static inline uint32_t
+rte_hash_crc_2byte(uint16_t data, uint32_t init_val)
+{
+	if (likely(crc32_alg & CRC32_ARM64))
+		return crc32c_arm64_u16(data, init_val);
+
+	return crc32c_2bytes(data, init_val);
+}
+
+/**
  * Use single crc32 instruction to perform a hash on a 4 byte value.
  * Fall back to software crc32 implementation in case arm64 crc intrinsics is
  * not supported
diff --git a/lib/librte_hash/rte_hash_crc.h b/lib/librte_hash/rte_hash_crc.h
index 78a34b7..63e74aa 100644
--- a/lib/librte_hash/rte_hash_crc.h
+++ b/lib/librte_hash/rte_hash_crc.h
@@ -328,6 +328,28 @@ static const uint32_t crc32c_tables[8][256] = {{
 	 crc32c_tables[(n)-1][((crc) >> 8) & 0xFF])
 
 static inline uint32_t
+crc32c_1byte(uint8_t data, uint32_t init_val)
+{
+	uint32_t crc;
+	crc = init_val;
+	crc ^= data;
+
+	return crc32c_tables[0][crc & 0xff] ^ (crc >> 8);
+}
+
+static inline uint32_t
+crc32c_2bytes(uint16_t data, uint32_t init_val)
+{
+	uint32_t crc;
+	crc = init_val;
+	crc ^= data;
+
+	crc = CRC32_UPD(crc, 1) ^ (crc >> 16);
+
+	return crc;
+}
+
+static inline uint32_t
 crc32c_1word(uint32_t data, uint32_t init_val)
 {
 	uint32_t crc, term1, term2;
@@ -367,6 +389,26 @@ crc32c_2words(uint64_t data, uint32_t init_val)
 
 #if defined(RTE_ARCH_I686) || defined(RTE_ARCH_X86_64)
 static inline uint32_t
+crc32c_sse42_u8(uint8_t data, uint32_t init_val)
+{
+	__asm__ volatile(
+			"crc32b %[data], %[init_val];"
+			: [init_val] "+r" (init_val)
+			: [data] "rm" (data));
+	return init_val;
+}
+
+static inline uint32_t
+crc32c_sse42_u16(uint16_t data, uint32_t init_val)
+{
+	__asm__ volatile(
+			"crc32w %[data], %[init_val];"
+			: [init_val] "+r" (init_val)
+			: [data] "rm" (data));
+	return init_val;
+}
+
+static inline uint32_t
 crc32c_sse42_u32(uint32_t data, uint32_t init_val)
 {
 	__asm__ volatile(
@@ -453,6 +495,52 @@ rte_hash_crc_init_alg(void)
 }
 
 /**
+ * Use single crc32 instruction to perform a hash on a byte value.
+ * Fall back to software crc32 implementation in case SSE4.2 is
+ * not supported
+ *
+ * @param data
+ *   Data to perform hash on.
+ * @param init_val
+ *   Value to initialise hash generator.
+ * @return
+ *   32bit calculated hash value.
+ */
+static inline uint32_t
+rte_hash_crc_1byte(uint8_t data, uint32_t init_val)
+{
+#if defined RTE_ARCH_I686 || defined RTE_ARCH_X86_64
+	if (likely(crc32_alg & CRC32_SSE42))
+		return crc32c_sse42_u8(data, init_val);
+#endif
+
+	return crc32c_1byte(data, init_val);
+}
+
+/**
+ * Use single crc32 instruction to perform a hash on a 2 bytes value.
+ * Fall back to software crc32 implementation in case SSE4.2 is
+ * not supported
+ *
+ * @param data
+ *   Data to perform hash on.
+ * @param init_val
+ *   Value to initialise hash generator.
+ * @return
+ *   32bit calculated hash value.
+ */
+static inline uint32_t
+rte_hash_crc_2byte(uint16_t data, uint32_t init_val)
+{
+#if defined RTE_ARCH_I686 || defined RTE_ARCH_X86_64
+	if (likely(crc32_alg & CRC32_SSE42))
+		return crc32c_sse42_u16(data, init_val);
+#endif
+
+	return crc32c_2bytes(data, init_val);
+}
+
+/**
  * Use single crc32 instruction to perform a hash on a 4 byte value.
  * Fall back to software crc32 implementation in case SSE4.2 is
  * not supported
@@ -521,7 +609,6 @@ static inline uint32_t
 rte_hash_crc(const void *data, uint32_t data_len, uint32_t init_val)
 {
 	unsigned i;
-	uint64_t temp = 0;
 	uintptr_t pd = (uintptr_t) data;
 
 	for (i = 0; i < data_len / 8; i++) {
@@ -529,35 +616,19 @@ rte_hash_crc(const void *data, uint32_t data_len, uint32_t init_val)
 		pd += 8;
 	}
 
-	switch (7 - (data_len & 0x07)) {
-	case 0:
-		temp |= (uint64_t) *((const uint8_t *)pd + 6) << 48;
-		/* Fallthrough */
-	case 1:
-		temp |= (uint64_t) *((const uint8_t *)pd + 5) << 40;
-		/* Fallthrough */
-	case 2:
-		temp |= (uint64_t) *((const uint8_t *)pd + 4) << 32;
-		temp |= *(const uint32_t *)pd;
-		init_val = rte_hash_crc_8byte(temp, init_val);
-		break;
-	case 3:
+	if (data_len & 0x4) {
 		init_val = rte_hash_crc_4byte(*(const uint32_t *)pd, init_val);
-		break;
-	case 4:
-		temp |= *((const uint8_t *)pd + 2) << 16;
-		/* Fallthrough */
-	case 5:
-		temp |= *((const uint8_t *)pd + 1) << 8;
-		/* Fallthrough */
-	case 6:
-		temp |= *(const uint8_t *)pd;
-		init_val = rte_hash_crc_4byte(temp, init_val);
-		/* Fallthrough */
-	default:
-		break;
+		pd += 4;
+	}
+
+	if (data_len & 0x2) {
+		init_val = rte_hash_crc_2byte(*(const uint16_t *)pd, init_val);
+		pd += 2;
 	}
 
+	if (data_len & 0x1)
+		init_val = rte_hash_crc_1byte(*(const uint8_t *)pd, init_val);
+
 	return init_val;
 }
 
-- 
2.1.4

Re: [dpdk-dev] [PATCH v3 0/2] Fix CRC32c computation

[De Lara Guarch, Pablo]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Didier Pallard
> Sent: Friday, February 19, 2016 11:00 AM
> To: dev@dpdk.org
> Subject: [dpdk-dev] [PATCH v3 0/2] Fix CRC32c computation
> 
> CRC32c computation is not valid when buffer length is not a multiple of 4
> bytes.
> Values returned by rte_hash_crc functions does not match the one
> computed by a trivial crc32c implementation.
> 
> First patch fixes crc hash function autotests, to outline the problem.
> Second patch fixes CRC32c computation.
> 
> Didier Pallard (2):
>   test: fix CRC hash function autotest
>   hash: fix CRC32c computation
> 
>  app/test/test_hash_functions.c         |  17 +++--
>  doc/guides/rel_notes/release_16_04.rst |   5 ++
>  lib/librte_hash/rte_crc_arm64.h        |  64 +++++++++++++++++
>  lib/librte_hash/rte_hash_crc.h         | 125 ++++++++++++++++++++++++++-----
> --
>  4 files changed, 178 insertions(+), 33 deletions(-)
> 
> --
> 2.1.4

Series-acked-by: Pablo de Lara <pablo.de.lara.guarch@intel.com>

Not sure if you need to include a "Fixes" line in the commit messages.
In the first commit, probably you should, the commit that you are fixing is
6298d2c55ae8 ("app/test: add new functional tests for hash functions").
In the second commit, it is a bit more difficult, as we don't know that the commit is,
that code was integrated a while ago, before 1.2.3, which is the first public release in dpdk.org.

Also, there is a typo "lengthes", in both commit messages.

You can leave the ack in both patches. Thanks!!

Re: [dpdk-dev] [PATCH v3 0/2] Fix CRC32c computation

[Thomas Monjalon]

> > CRC32c computation is not valid when buffer length is not a multiple of 4
> > bytes.
> > Values returned by rte_hash_crc functions does not match the one
> > computed by a trivial crc32c implementation.
> > 
> > First patch fixes crc hash function autotests, to outline the problem.
> > Second patch fixes CRC32c computation.
> > 
> > Didier Pallard (2):
> >   test: fix CRC hash function autotest
> >   hash: fix CRC32c computation
> 
> Series-acked-by: Pablo de Lara <pablo.de.lara.guarch@intel.com>
> 
> Not sure if you need to include a "Fixes" line in the commit messages.
> In the first commit, probably you should, the commit that you are fixing is
> 6298d2c55ae8 ("app/test: add new functional tests for hash functions").

Thanks

> In the second commit, it is a bit more difficult, as we don't know that the commit is,
> that code was integrated a while ago, before 1.2.3, which is the first public release in dpdk.org.

Yes it helps to know the bug was there since the beginning.

> Also, there is a typo "lengthes", in both commit messages.
> 
> You can leave the ack in both patches. Thanks!!

Applied, thanks