* [PATCH 1/6] RDMA/rxe: handle ICRC correctly on big endian systems
2025-01-27 22:38 [PATCH 0/6] RDMA: switch to using CRC32 library functions Eric Biggers
@ 2025-01-27 22:38 ` Eric Biggers
2025-01-29 9:44 ` Zhu Yanjun
2025-01-29 18:27 ` Zhu Yanjun
2025-01-27 22:38 ` [PATCH 2/6] RDMA/rxe: consolidate code for calculating ICRC of packets Eric Biggers
` (4 subsequent siblings)
5 siblings, 2 replies; 28+ messages in thread
From: Eric Biggers @ 2025-01-27 22:38 UTC (permalink / raw)
To: linux-rdma, Mustafa Ismail, Tatyana Nikolova, Jason Gunthorpe,
Leon Romanovsky, Zhu Yanjun, Bernard Metzler
Cc: linux-kernel
From: Eric Biggers <ebiggers@google.com>
The usual big endian convention of InfiniBand does not apply to the
ICRC field, whose transmission is specified in terms of the CRC32
polynomial coefficients. The coefficients are transmitted in
descending order from the x^31 coefficient to the x^0 one. When the
result is interpreted as a 32-bit integer using the required reverse
mapping between bits and polynomial coefficients, it's a __le32.
The code used __be32, but it did not actually do an endianness
conversion. So it actually just used the CPU's native endianness,
making it comply with the spec on little endian systems only.
Fix the code to use __le32 and do the needed le32_to_cpu() and
cpu_to_le32() so that it complies with the spec on big endian systems.
Also update the lower-level helper functions to use u32, as they are
working with CPU native values. This part does not change behavior.
Found through code review. I don't know whether anyone is actually
using this code on big endian systems. Probably not.
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
drivers/infiniband/sw/rxe/rxe_icrc.c | 41 +++++++++++++++-------------
1 file changed, 22 insertions(+), 19 deletions(-)
diff --git a/drivers/infiniband/sw/rxe/rxe_icrc.c b/drivers/infiniband/sw/rxe/rxe_icrc.c
index fdf5f08cd8f17..ee417a0bbbdca 100644
--- a/drivers/infiniband/sw/rxe/rxe_icrc.c
+++ b/drivers/infiniband/sw/rxe/rxe_icrc.c
@@ -38,26 +38,26 @@ int rxe_icrc_init(struct rxe_dev *rxe)
* @next: starting address of current segment
* @len: length of current segment
*
* Return: the cumulative crc32 checksum
*/
-static __be32 rxe_crc32(struct rxe_dev *rxe, __be32 crc, void *next, size_t len)
+static u32 rxe_crc32(struct rxe_dev *rxe, u32 crc, void *next, size_t len)
{
- __be32 icrc;
+ u32 icrc;
int err;
SHASH_DESC_ON_STACK(shash, rxe->tfm);
shash->tfm = rxe->tfm;
- *(__be32 *)shash_desc_ctx(shash) = crc;
+ *(u32 *)shash_desc_ctx(shash) = crc;
err = crypto_shash_update(shash, next, len);
if (unlikely(err)) {
rxe_dbg_dev(rxe, "failed crc calculation, err: %d\n", err);
- return (__force __be32)crc32_le((__force u32)crc, next, len);
+ return crc32_le(crc, next, len);
}
- icrc = *(__be32 *)shash_desc_ctx(shash);
+ icrc = *(u32 *)shash_desc_ctx(shash);
barrier_data(shash_desc_ctx(shash));
return icrc;
}
@@ -67,18 +67,18 @@ static __be32 rxe_crc32(struct rxe_dev *rxe, __be32 crc, void *next, size_t len)
* @skb: packet buffer
* @pkt: packet information
*
* Return: the partial ICRC
*/
-static __be32 rxe_icrc_hdr(struct sk_buff *skb, struct rxe_pkt_info *pkt)
+static u32 rxe_icrc_hdr(struct sk_buff *skb, struct rxe_pkt_info *pkt)
{
unsigned int bth_offset = 0;
struct iphdr *ip4h = NULL;
struct ipv6hdr *ip6h = NULL;
struct udphdr *udph;
struct rxe_bth *bth;
- __be32 crc;
+ u32 crc;
int length;
int hdr_size = sizeof(struct udphdr) +
(skb->protocol == htons(ETH_P_IP) ?
sizeof(struct iphdr) : sizeof(struct ipv6hdr));
/* pseudo header buffer size is calculate using ipv6 header size since
@@ -89,11 +89,11 @@ static __be32 rxe_icrc_hdr(struct sk_buff *skb, struct rxe_pkt_info *pkt)
RXE_BTH_BYTES];
/* This seed is the result of computing a CRC with a seed of
* 0xfffffff and 8 bytes of 0xff representing a masked LRH.
*/
- crc = (__force __be32)0xdebb20e3;
+ crc = 0xdebb20e3;
if (skb->protocol == htons(ETH_P_IP)) { /* IPv4 */
memcpy(pshdr, ip_hdr(skb), hdr_size);
ip4h = (struct iphdr *)pshdr;
udph = (struct udphdr *)(ip4h + 1);
@@ -137,23 +137,27 @@ static __be32 rxe_icrc_hdr(struct sk_buff *skb, struct rxe_pkt_info *pkt)
*
* Return: 0 if the values match else an error
*/
int rxe_icrc_check(struct sk_buff *skb, struct rxe_pkt_info *pkt)
{
- __be32 *icrcp;
- __be32 pkt_icrc;
- __be32 icrc;
-
- icrcp = (__be32 *)(pkt->hdr + pkt->paylen - RXE_ICRC_SIZE);
- pkt_icrc = *icrcp;
+ /*
+ * The usual big endian convention of InfiniBand does not apply to the
+ * ICRC field, whose transmission is specified in terms of the CRC32
+ * polynomial coefficients. The coefficients are transmitted in
+ * descending order from the x^31 coefficient to the x^0 one. When the
+ * result is interpreted as a 32-bit integer using the required reverse
+ * mapping between bits and polynomial coefficients, it's a __le32.
+ */
+ __le32 *icrcp = (__le32 *)(pkt->hdr + pkt->paylen - RXE_ICRC_SIZE);
+ u32 icrc;
icrc = rxe_icrc_hdr(skb, pkt);
icrc = rxe_crc32(pkt->rxe, icrc, (u8 *)payload_addr(pkt),
payload_size(pkt) + bth_pad(pkt));
icrc = ~icrc;
- if (unlikely(icrc != pkt_icrc))
+ if (unlikely(icrc != le32_to_cpu(*icrcp)))
return -EINVAL;
return 0;
}
@@ -162,14 +166,13 @@ int rxe_icrc_check(struct sk_buff *skb, struct rxe_pkt_info *pkt)
* @skb: packet buffer
* @pkt: packet information
*/
void rxe_icrc_generate(struct sk_buff *skb, struct rxe_pkt_info *pkt)
{
- __be32 *icrcp;
- __be32 icrc;
+ __le32 *icrcp = (__le32 *)(pkt->hdr + pkt->paylen - RXE_ICRC_SIZE);
+ u32 icrc;
- icrcp = (__be32 *)(pkt->hdr + pkt->paylen - RXE_ICRC_SIZE);
icrc = rxe_icrc_hdr(skb, pkt);
icrc = rxe_crc32(pkt->rxe, icrc, (u8 *)payload_addr(pkt),
payload_size(pkt) + bth_pad(pkt));
- *icrcp = ~icrc;
+ *icrcp = cpu_to_le32(~icrc);
}
--
2.48.1
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [PATCH 1/6] RDMA/rxe: handle ICRC correctly on big endian systems
2025-01-27 22:38 ` [PATCH 1/6] RDMA/rxe: handle ICRC correctly on big endian systems Eric Biggers
@ 2025-01-29 9:44 ` Zhu Yanjun
2025-01-29 18:30 ` Jason Gunthorpe
2025-01-29 18:27 ` Zhu Yanjun
1 sibling, 1 reply; 28+ messages in thread
From: Zhu Yanjun @ 2025-01-29 9:44 UTC (permalink / raw)
To: Eric Biggers, linux-rdma, Mustafa Ismail, Tatyana Nikolova,
Jason Gunthorpe, Leon Romanovsky, Zhu Yanjun, Bernard Metzler
Cc: linux-kernel@vger.kernel.org
在 2025/1/27 23:38, Eric Biggers 写道:
> From: Eric Biggers <ebiggers@google.com>
>
> The usual big endian convention of InfiniBand does not apply to the
> ICRC field, whose transmission is specified in terms of the CRC32
> polynomial coefficients. The coefficients are transmitted in
> descending order from the x^31 coefficient to the x^0 one. When the
> result is interpreted as a 32-bit integer using the required reverse
> mapping between bits and polynomial coefficients, it's a __le32.
>
> The code used __be32, but it did not actually do an endianness
> conversion. So it actually just used the CPU's native endianness,
> making it comply with the spec on little endian systems only.
>
> Fix the code to use __le32 and do the needed le32_to_cpu() and
> cpu_to_le32() so that it complies with the spec on big endian systems.
>
> Also update the lower-level helper functions to use u32, as they are
> working with CPU native values. This part does not change behavior.
>
> Found through code review. I don't know whether anyone is actually
> using this code on big endian systems. Probably not.
>
> Signed-off-by: Eric Biggers <ebiggers@google.com>
In this commit, the little endian is used instead of big endian in the
rxe source code. To x86_64 architecture, I am fine with this commit.
To other big endian architecture, e.g. ppc/ppc64, I do not have such
host, thus, I can not make tests on the big endian host.
Thanks,
Reviewed-by: Zhu Yanjun <yanjun.zhu@linux.dev>
Zhu Yanjun
> ---
> drivers/infiniband/sw/rxe/rxe_icrc.c | 41 +++++++++++++++-------------
> 1 file changed, 22 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/infiniband/sw/rxe/rxe_icrc.c b/drivers/infiniband/sw/rxe/rxe_icrc.c
> index fdf5f08cd8f17..ee417a0bbbdca 100644
> --- a/drivers/infiniband/sw/rxe/rxe_icrc.c
> +++ b/drivers/infiniband/sw/rxe/rxe_icrc.c
> @@ -38,26 +38,26 @@ int rxe_icrc_init(struct rxe_dev *rxe)
> * @next: starting address of current segment
> * @len: length of current segment
> *
> * Return: the cumulative crc32 checksum
> */
> -static __be32 rxe_crc32(struct rxe_dev *rxe, __be32 crc, void *next, size_t len)
> +static u32 rxe_crc32(struct rxe_dev *rxe, u32 crc, void *next, size_t len)
> {
> - __be32 icrc;
> + u32 icrc;
> int err;
>
> SHASH_DESC_ON_STACK(shash, rxe->tfm);
>
> shash->tfm = rxe->tfm;
> - *(__be32 *)shash_desc_ctx(shash) = crc;
> + *(u32 *)shash_desc_ctx(shash) = crc;
> err = crypto_shash_update(shash, next, len);
> if (unlikely(err)) {
> rxe_dbg_dev(rxe, "failed crc calculation, err: %d\n", err);
> - return (__force __be32)crc32_le((__force u32)crc, next, len);
> + return crc32_le(crc, next, len);
> }
>
> - icrc = *(__be32 *)shash_desc_ctx(shash);
> + icrc = *(u32 *)shash_desc_ctx(shash);
> barrier_data(shash_desc_ctx(shash));
>
> return icrc;
> }
>
> @@ -67,18 +67,18 @@ static __be32 rxe_crc32(struct rxe_dev *rxe, __be32 crc, void *next, size_t len)
> * @skb: packet buffer
> * @pkt: packet information
> *
> * Return: the partial ICRC
> */
> -static __be32 rxe_icrc_hdr(struct sk_buff *skb, struct rxe_pkt_info *pkt)
> +static u32 rxe_icrc_hdr(struct sk_buff *skb, struct rxe_pkt_info *pkt)
> {
> unsigned int bth_offset = 0;
> struct iphdr *ip4h = NULL;
> struct ipv6hdr *ip6h = NULL;
> struct udphdr *udph;
> struct rxe_bth *bth;
> - __be32 crc;
> + u32 crc;
> int length;
> int hdr_size = sizeof(struct udphdr) +
> (skb->protocol == htons(ETH_P_IP) ?
> sizeof(struct iphdr) : sizeof(struct ipv6hdr));
> /* pseudo header buffer size is calculate using ipv6 header size since
> @@ -89,11 +89,11 @@ static __be32 rxe_icrc_hdr(struct sk_buff *skb, struct rxe_pkt_info *pkt)
> RXE_BTH_BYTES];
>
> /* This seed is the result of computing a CRC with a seed of
> * 0xfffffff and 8 bytes of 0xff representing a masked LRH.
> */
> - crc = (__force __be32)0xdebb20e3;
> + crc = 0xdebb20e3;
>
> if (skb->protocol == htons(ETH_P_IP)) { /* IPv4 */
> memcpy(pshdr, ip_hdr(skb), hdr_size);
> ip4h = (struct iphdr *)pshdr;
> udph = (struct udphdr *)(ip4h + 1);
> @@ -137,23 +137,27 @@ static __be32 rxe_icrc_hdr(struct sk_buff *skb, struct rxe_pkt_info *pkt)
> *
> * Return: 0 if the values match else an error
> */
> int rxe_icrc_check(struct sk_buff *skb, struct rxe_pkt_info *pkt)
> {
> - __be32 *icrcp;
> - __be32 pkt_icrc;
> - __be32 icrc;
> -
> - icrcp = (__be32 *)(pkt->hdr + pkt->paylen - RXE_ICRC_SIZE);
> - pkt_icrc = *icrcp;
> + /*
> + * The usual big endian convention of InfiniBand does not apply to the
> + * ICRC field, whose transmission is specified in terms of the CRC32
> + * polynomial coefficients. The coefficients are transmitted in
> + * descending order from the x^31 coefficient to the x^0 one. When the
> + * result is interpreted as a 32-bit integer using the required reverse
> + * mapping between bits and polynomial coefficients, it's a __le32.
> + */
> + __le32 *icrcp = (__le32 *)(pkt->hdr + pkt->paylen - RXE_ICRC_SIZE);
> + u32 icrc;
>
> icrc = rxe_icrc_hdr(skb, pkt);
> icrc = rxe_crc32(pkt->rxe, icrc, (u8 *)payload_addr(pkt),
> payload_size(pkt) + bth_pad(pkt));
> icrc = ~icrc;
>
> - if (unlikely(icrc != pkt_icrc))
> + if (unlikely(icrc != le32_to_cpu(*icrcp)))
> return -EINVAL;
>
> return 0;
> }
>
> @@ -162,14 +166,13 @@ int rxe_icrc_check(struct sk_buff *skb, struct rxe_pkt_info *pkt)
> * @skb: packet buffer
> * @pkt: packet information
> */
> void rxe_icrc_generate(struct sk_buff *skb, struct rxe_pkt_info *pkt)
> {
> - __be32 *icrcp;
> - __be32 icrc;
> + __le32 *icrcp = (__le32 *)(pkt->hdr + pkt->paylen - RXE_ICRC_SIZE);
> + u32 icrc;
>
> - icrcp = (__be32 *)(pkt->hdr + pkt->paylen - RXE_ICRC_SIZE);
> icrc = rxe_icrc_hdr(skb, pkt);
> icrc = rxe_crc32(pkt->rxe, icrc, (u8 *)payload_addr(pkt),
> payload_size(pkt) + bth_pad(pkt));
> - *icrcp = ~icrc;
> + *icrcp = cpu_to_le32(~icrc);
> }
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH 1/6] RDMA/rxe: handle ICRC correctly on big endian systems
2025-01-29 9:44 ` Zhu Yanjun
@ 2025-01-29 18:30 ` Jason Gunthorpe
2025-01-29 18:51 ` Eric Biggers
2025-01-30 7:27 ` Zhu Yanjun
0 siblings, 2 replies; 28+ messages in thread
From: Jason Gunthorpe @ 2025-01-29 18:30 UTC (permalink / raw)
To: Zhu Yanjun
Cc: Eric Biggers, linux-rdma, Mustafa Ismail, Tatyana Nikolova,
Leon Romanovsky, Zhu Yanjun, Bernard Metzler,
linux-kernel@vger.kernel.org
On Wed, Jan 29, 2025 at 10:44:39AM +0100, Zhu Yanjun wrote:
> 在 2025/1/27 23:38, Eric Biggers 写道:
> > From: Eric Biggers <ebiggers@google.com>
> >
> > The usual big endian convention of InfiniBand does not apply to the
> > ICRC field, whose transmission is specified in terms of the CRC32
> > polynomial coefficients.
This patch is on to something but this is not a good explanation.
The CRC32 in IB is stored as big endian and computed in big endian,
the spec says so explicitly:
2) The CRC calculation is done in big endian byte order with the least
31 significant bit of the most significant byte being the first
bits in the 32 CRC calculation.
In this context saying it is not "big endian" doesn't seem to be quite
right..
The spec gives a sample data packet (in offset/value pairs):
0 0xF0 15 0xB3 30 0x7A 45 0x8B
1 0x12 16 0x00 31 0x05 46 0xC0
2 0x37 17 0x0D 32 0x00 47 0x69
3 0x5C 18 0xEC 33 0x00 48 0x0E
4 0x00 19 0x2A 34 0x00 49 0xD4
5 0x0E 20 0x01 35 0x0E 50 0x00
6 0x17 21 0x71 36 0xBB 51 0x00
7 0xD2 22 0x0A 37 0x88
8 0x0A 23 0x1C 38 0x4D
9 0x20 24 0x01 39 0x85
10 0x24 25 0x5D 40 0xFD
11 0x87 26 0x40 41 0x5C
12 0xFF 27 0x02 42 0xFB
13 0x87 28 0x38 43 0xA4
14 0xB1 29 0xF2 44 0x72
If you feed that to the CRC32 you should get 0x9625B75A in a CPU
register u32.
cpu_to_be32() will put it in the right order for on the wire.
Since rxe doesn't have any cpu_to_be32() on this path, I'm guessing
the Linux CRC32 implementations gives a u32 with the
value = 0x5AB72596 ie swapped.
Probably the issue here is that the Linux CRC32 and the IBTA CRC32 are
using a different mapping of LFSR bits. I love CRCs. So many different
ways to implement the same thing.
Thus, I guess, the code should really read:
linux_crc32 = swab32(be32_to_cpu(val));
Which is a NOP on x86 and requires a swap on BE.
Zhu, can you check it for Eric? (this is page 229 in the spec).
I assume the Linux CRC32 always gives the same CPU value regardless of
LE or BE?
Jason
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 1/6] RDMA/rxe: handle ICRC correctly on big endian systems
2025-01-29 18:30 ` Jason Gunthorpe
@ 2025-01-29 18:51 ` Eric Biggers
2025-01-29 19:43 ` Jason Gunthorpe
2025-01-30 7:27 ` Zhu Yanjun
1 sibling, 1 reply; 28+ messages in thread
From: Eric Biggers @ 2025-01-29 18:51 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Zhu Yanjun, linux-rdma, Mustafa Ismail, Tatyana Nikolova,
Leon Romanovsky, Zhu Yanjun, Bernard Metzler,
linux-kernel@vger.kernel.org
Hi Jason,
On Wed, Jan 29, 2025 at 02:30:09PM -0400, Jason Gunthorpe wrote:
> On Wed, Jan 29, 2025 at 10:44:39AM +0100, Zhu Yanjun wrote:
> > 在 2025/1/27 23:38, Eric Biggers 写道:
> > > From: Eric Biggers <ebiggers@google.com>
> > >
> > > The usual big endian convention of InfiniBand does not apply to the
> > > ICRC field, whose transmission is specified in terms of the CRC32
> > > polynomial coefficients.
>
> This patch is on to something but this is not a good explanation.
>
> The CRC32 in IB is stored as big endian and computed in big endian,
> the spec says so explicitly:
>
> 2) The CRC calculation is done in big endian byte order with the least
> significant bit of the most significant byte being the first
> bits in the CRC calculation.
(2) just specifies the order in which the bits are passed to the CRC. It says
nothing about how the CRC value is stored; that's in (4) instead.
The mention of "big endian" seems to refer to the bytes being passed from first
to last, which is the nearly universal convention. (I would not have used the
term "big endian" here, as it's confusing.) The rest clarifies that it's a
least-significant-bit (LSB)-first CRC, i.e. "little endian" or "le" in Linux
terminology. (The Linux terminology here is a mistake too -- it would be
clearer to call it lsb rather than le, given that it's about bits.)
> In this context saying it is not "big endian" doesn't seem to be quite
> right..
>
> The spec gives a sample data packet (in offset/value pairs):
>
> 0 0xF0 15 0xB3 30 0x7A 45 0x8B
> 1 0x12 16 0x00 31 0x05 46 0xC0
> 2 0x37 17 0x0D 32 0x00 47 0x69
> 3 0x5C 18 0xEC 33 0x00 48 0x0E
> 4 0x00 19 0x2A 34 0x00 49 0xD4
> 5 0x0E 20 0x01 35 0x0E 50 0x00
> 6 0x17 21 0x71 36 0xBB 51 0x00
> 7 0xD2 22 0x0A 37 0x88
> 8 0x0A 23 0x1C 38 0x4D
> 9 0x20 24 0x01 39 0x85
> 10 0x24 25 0x5D 40 0xFD
> 11 0x87 26 0x40 41 0x5C
> 12 0xFF 27 0x02 42 0xFB
> 13 0x87 28 0x38 43 0xA4
> 14 0xB1 29 0xF2 44 0x72
>
> If you feed that to the CRC32 you should get 0x9625B75A in a CPU
> register u32.
>
> cpu_to_be32() will put it in the right order for on the wire.
I think cpu_to_be32() would put it in the wrong order. Refer to the following:
4. The resulting bits are sent in order from the bit representing the
coefficient of the highest term of the remainder polynomial. The least
significant bit, most significant byte first ordering of the packet does not
apply to the ICRC field.
As per (2) it's a least-significant-bit first CRC, i.e. bit 0 represents the
coefficient of x^31 and bit 31 represents the coefficient of x^0. So the least
significant byte of the u32 has the highest 8 polynomial terms (the coefficients
of x^24 through x^31) and must be sent first. That's an __le32. Yes, the fact
that the mapping between bits and polynomial terms is backwards makes it
confusing, but that's how LSB-first CRCs work.
> I assume the Linux CRC32 always gives the same CPU value regardless of
> LE or BE?
Yes, they return a u32. (Unless you use crypto_shash_final or
crypto_shash_digest in which case you get a u8[4] a.k.a. __le32.)
- Eric
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH 1/6] RDMA/rxe: handle ICRC correctly on big endian systems
2025-01-29 18:51 ` Eric Biggers
@ 2025-01-29 19:43 ` Jason Gunthorpe
2025-01-29 20:25 ` Eric Biggers
2025-01-30 9:17 ` Zhu Yanjun
0 siblings, 2 replies; 28+ messages in thread
From: Jason Gunthorpe @ 2025-01-29 19:43 UTC (permalink / raw)
To: Eric Biggers
Cc: Zhu Yanjun, linux-rdma, Mustafa Ismail, Tatyana Nikolova,
Leon Romanovsky, Zhu Yanjun, Bernard Metzler,
linux-kernel@vger.kernel.org
On Wed, Jan 29, 2025 at 06:51:15PM +0000, Eric Biggers wrote:
> Hi Jason,
>
> On Wed, Jan 29, 2025 at 02:30:09PM -0400, Jason Gunthorpe wrote:
> > On Wed, Jan 29, 2025 at 10:44:39AM +0100, Zhu Yanjun wrote:
> > > 在 2025/1/27 23:38, Eric Biggers 写道:
> > > > From: Eric Biggers <ebiggers@google.com>
> > > >
> > > > The usual big endian convention of InfiniBand does not apply to the
> > > > ICRC field, whose transmission is specified in terms of the CRC32
> > > > polynomial coefficients.
> >
> > This patch is on to something but this is not a good explanation.
> >
> > The CRC32 in IB is stored as big endian and computed in big endian,
> > the spec says so explicitly:
> >
> > 2) The CRC calculation is done in big endian byte order with the least
> > significant bit of the most significant byte being the first
> > bits in the CRC calculation.
>
> (2) just specifies the order in which the bits are passed to the CRC. It says
> nothing about how the CRC value is stored; that's in (4) instead.
It could be. The other parts of the spec are more definitive.
> The mention of "big endian" seems to refer to the bytes being passed
> from first to last, which is the nearly universal convention.
The LFSR Figure 57 shows how the numerical representation the spec
uses (ie the 0x9625B75A) maps to the LFSR, and it could be called big
endian.
Regardless, table 29 makes this crystal clear, it shows how the
numerical representation of 0x9625B75A is placed on the wire, it is
big endian as all IBTA values are - byte 52 is 0x96, byte 55 is 0x5A.
> (I would not have used the term "big endian" here, as it's
> confusing.)
It could be read as going byte-by-byte, or it could be referring to
the layout of the numerical representation..
> > If you feed that to the CRC32 you should get 0x9625B75A in a CPU
> > register u32.
> >
> > cpu_to_be32() will put it in the right order for on the wire.
>
> I think cpu_to_be32() would put it in the wrong order. Refer to the following:
It is fully explicit in table 29, 0x9625B75A is stored in big
endian format starting at byte 52.
It is easy to answer without guessing, Zhu should just run the sample
packet through the new crc library code and determine what the u32
value is. It is either 0x9625B75A or swab(0x9625B75A) and that will
tell what the code should be.
Jason
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 1/6] RDMA/rxe: handle ICRC correctly on big endian systems
2025-01-29 19:43 ` Jason Gunthorpe
@ 2025-01-29 20:25 ` Eric Biggers
2025-01-29 21:16 ` Jason Gunthorpe
2025-01-30 9:17 ` Zhu Yanjun
1 sibling, 1 reply; 28+ messages in thread
From: Eric Biggers @ 2025-01-29 20:25 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Zhu Yanjun, linux-rdma, Mustafa Ismail, Tatyana Nikolova,
Leon Romanovsky, Zhu Yanjun, Bernard Metzler,
linux-kernel@vger.kernel.org
On Wed, Jan 29, 2025 at 03:43:44PM -0400, Jason Gunthorpe wrote:
> On Wed, Jan 29, 2025 at 06:51:15PM +0000, Eric Biggers wrote:
> > Hi Jason,
> >
> > On Wed, Jan 29, 2025 at 02:30:09PM -0400, Jason Gunthorpe wrote:
> > > On Wed, Jan 29, 2025 at 10:44:39AM +0100, Zhu Yanjun wrote:
> > > > 在 2025/1/27 23:38, Eric Biggers 写道:
> > > > > From: Eric Biggers <ebiggers@google.com>
> > > > >
> > > > > The usual big endian convention of InfiniBand does not apply to the
> > > > > ICRC field, whose transmission is specified in terms of the CRC32
> > > > > polynomial coefficients.
> > >
> > > This patch is on to something but this is not a good explanation.
> > >
> > > The CRC32 in IB is stored as big endian and computed in big endian,
> > > the spec says so explicitly:
> > >
> > > 2) The CRC calculation is done in big endian byte order with the least
> > > significant bit of the most significant byte being the first
> > > bits in the CRC calculation.
> >
> > (2) just specifies the order in which the bits are passed to the CRC. It says
> > nothing about how the CRC value is stored; that's in (4) instead.
>
> It could be. The other parts of the spec are more definitive.
>
> > The mention of "big endian" seems to refer to the bytes being passed
> > from first to last, which is the nearly universal convention.
>
> The LFSR Figure 57 shows how the numerical representation the spec
> uses (ie the 0x9625B75A) maps to the LFSR, and it could be called big
> endian.
>
> Regardless, table 29 makes this crystal clear, it shows how the
> numerical representation of 0x9625B75A is placed on the wire, it is
> big endian as all IBTA values are - byte 52 is 0x96, byte 55 is 0x5A.
>
> > (I would not have used the term "big endian" here, as it's
> > confusing.)
>
> It could be read as going byte-by-byte, or it could be referring to
> the layout of the numerical representation..
>
> > > If you feed that to the CRC32 you should get 0x9625B75A in a CPU
> > > register u32.
> > >
> > > cpu_to_be32() will put it in the right order for on the wire.
> >
> > I think cpu_to_be32() would put it in the wrong order. Refer to the following:
>
> It is fully explicit in table 29, 0x9625B75A is stored in big
> endian format starting at byte 52.
>
> It is easy to answer without guessing, Zhu should just run the sample
> packet through the new crc library code and determine what the u32
> value is. It is either 0x9625B75A or swab(0x9625B75A) and that will
> tell what the code should be.
>
> Jason
static const u8 data[52] = {
0xf0, 0x12, 0x37, 0x5c, 0x00, 0x0e, 0x17, 0xd2, 0x0a, 0x20, 0x24, 0x87, 0xff, 0x87, 0xb1,
0xb3, 0x00, 0x0d, 0xec, 0x2a, 0x01, 0x71, 0x0a, 0x1c, 0x01, 0x5d, 0x40, 0x02, 0x38, 0xf2,
0x7a, 0x05, 0x00, 0x00, 0x00, 0x0e, 0xbb, 0x88, 0x4d, 0x85, 0xfd, 0x5c, 0xfb, 0xa4, 0x72,
0x8b, 0xc0, 0x69, 0x0e, 0xd4, 0x00, 0x00
};
pr_info("crcval=0x%x\n", ~crc32_le(~0,data,sizeof(data)));
crcval=0x5ab72596
So yes, the InfiniBand spec gives swab(0x5ab72596) = 0x9625B75A.
It's a least-significant-bit first CRC32, so bits 0 through 31 of 0x5ab72596
represent the coefficients of x^31 through x^0 in that order. The byte swap to
0x9625B75A reorders the coefficients to x^7 through x^0, x^15 through x^8, x^23
through x^16, x^31 through x^24. (This is no longer a sequential order, so this
order is not usually used.) The spec then stores the swapped value in big
endian order to cancel out the extra swap, resulting in the polynomial
coefficients being in a sequential order again.
IMO, it's easier to think about this as storing the least-significant-bit first
CRC32 value using little endian byte order.
- Eric
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH 1/6] RDMA/rxe: handle ICRC correctly on big endian systems
2025-01-29 20:25 ` Eric Biggers
@ 2025-01-29 21:16 ` Jason Gunthorpe
2025-01-29 22:21 ` Eric Biggers
0 siblings, 1 reply; 28+ messages in thread
From: Jason Gunthorpe @ 2025-01-29 21:16 UTC (permalink / raw)
To: Eric Biggers
Cc: Zhu Yanjun, linux-rdma, Mustafa Ismail, Tatyana Nikolova,
Leon Romanovsky, Zhu Yanjun, Bernard Metzler,
linux-kernel@vger.kernel.org
On Wed, Jan 29, 2025 at 12:25:37PM -0800, Eric Biggers wrote:
> static const u8 data[52] = {
> 0xf0, 0x12, 0x37, 0x5c, 0x00, 0x0e, 0x17, 0xd2, 0x0a, 0x20, 0x24, 0x87, 0xff, 0x87, 0xb1,
> 0xb3, 0x00, 0x0d, 0xec, 0x2a, 0x01, 0x71, 0x0a, 0x1c, 0x01, 0x5d, 0x40, 0x02, 0x38, 0xf2,
> 0x7a, 0x05, 0x00, 0x00, 0x00, 0x0e, 0xbb, 0x88, 0x4d, 0x85, 0xfd, 0x5c, 0xfb, 0xa4, 0x72,
> 0x8b, 0xc0, 0x69, 0x0e, 0xd4, 0x00, 0x00
> };
> pr_info("crcval=0x%x\n", ~crc32_le(~0,data,sizeof(data)));
>
> crcval=0x5ab72596
>
> So yes, the InfiniBand spec gives swab(0x5ab72596) = 0x9625B75A.
>
> It's a least-significant-bit first CRC32, so bits 0 through 31 of 0x5ab72596
> represent the coefficients of x^31 through x^0 in that order. The byte swap to
> 0x9625B75A reorders the coefficients to x^7 through x^0, x^15 through x^8, x^23
> through x^16, x^31 through x^24. (This is no longer a sequential order, so this
> order is not usually used.) The spec then stores the swapped value in big
> endian order to cancel out the extra swap, resulting in the polynomial
> coefficients being in a sequential order again.
> IMO, it's easier to think about this as storing the least-significant-bit first
> CRC32 value using little endian byte order.
To be most clear this should be written as:
u32 ibta_crc = swab32(~crc32_le(..)) // Gives you the IBTA defined value
*packet = cpu_to_be32(ibta_crc); // Puts it in the packet
It follows the spec clearly and exactly.
Yes, you can get the same net effect using le:
u32 not_ibta_crc = ~crc32_le(..)
*packet = cpu_to_le32(not_ibta_crc)
It does work, but it is very hard to follow how that relates to the
specification when the u32 is not in the spec's format anymore.
What matters here, in rxe, is how to use the Linux crc32 library to
get exactly the value written down in the spec.
IMHO the le approach is an optimization to avoid the dobule swap, and
it should simply be described as such in a comment:
The crc32 library gives a byte swapped result compared to the IBTA
specification. swab32(~crc32_le(..)) will give values that match
IBTA.
To avoid double swapping we can instead write:
*icrc = cpu_to_le32(~crc32_le(..))
The value will still be big endian on the network.
No need to talk about coefficients.
Jason
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH 1/6] RDMA/rxe: handle ICRC correctly on big endian systems
2025-01-29 21:16 ` Jason Gunthorpe
@ 2025-01-29 22:21 ` Eric Biggers
2025-01-30 1:29 ` Jason Gunthorpe
0 siblings, 1 reply; 28+ messages in thread
From: Eric Biggers @ 2025-01-29 22:21 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Zhu Yanjun, linux-rdma, Mustafa Ismail, Tatyana Nikolova,
Leon Romanovsky, Zhu Yanjun, Bernard Metzler,
linux-kernel@vger.kernel.org
On Wed, Jan 29, 2025 at 05:16:51PM -0400, Jason Gunthorpe wrote:
> On Wed, Jan 29, 2025 at 12:25:37PM -0800, Eric Biggers wrote:
> > static const u8 data[52] = {
> > 0xf0, 0x12, 0x37, 0x5c, 0x00, 0x0e, 0x17, 0xd2, 0x0a, 0x20, 0x24, 0x87, 0xff, 0x87, 0xb1,
> > 0xb3, 0x00, 0x0d, 0xec, 0x2a, 0x01, 0x71, 0x0a, 0x1c, 0x01, 0x5d, 0x40, 0x02, 0x38, 0xf2,
> > 0x7a, 0x05, 0x00, 0x00, 0x00, 0x0e, 0xbb, 0x88, 0x4d, 0x85, 0xfd, 0x5c, 0xfb, 0xa4, 0x72,
> > 0x8b, 0xc0, 0x69, 0x0e, 0xd4, 0x00, 0x00
> > };
> > pr_info("crcval=0x%x\n", ~crc32_le(~0,data,sizeof(data)));
> >
> > crcval=0x5ab72596
> >
> > So yes, the InfiniBand spec gives swab(0x5ab72596) = 0x9625B75A.
> >
> > It's a least-significant-bit first CRC32, so bits 0 through 31 of 0x5ab72596
> > represent the coefficients of x^31 through x^0 in that order. The byte swap to
> > 0x9625B75A reorders the coefficients to x^7 through x^0, x^15 through x^8, x^23
> > through x^16, x^31 through x^24. (This is no longer a sequential order, so this
> > order is not usually used.) The spec then stores the swapped value in big
> > endian order to cancel out the extra swap, resulting in the polynomial
> > coefficients being in a sequential order again.
>
> > IMO, it's easier to think about this as storing the least-significant-bit first
> > CRC32 value using little endian byte order.
>
> To be most clear this should be written as:
>
> u32 ibta_crc = swab32(~crc32_le(..)) // Gives you the IBTA defined value
> *packet = cpu_to_be32(ibta_crc); // Puts it in the packet
>
> It follows the spec clearly and exactly.
>
> Yes, you can get the same net effect using le:
>
> u32 not_ibta_crc = ~crc32_le(..)
> *packet = cpu_to_le32(not_ibta_crc)
>
> It does work, but it is very hard to follow how that relates to the
> specification when the u32 is not in the spec's format anymore.
>
> What matters here, in rxe, is how to use the Linux crc32 library to
> get exactly the value written down in the spec.
>
> IMHO the le approach is an optimization to avoid the dobule swap, and
> it should simply be described as such in a comment:
>
> The crc32 library gives a byte swapped result compared to the IBTA
> specification. swab32(~crc32_le(..)) will give values that match
> IBTA.
>
> To avoid double swapping we can instead write:
> *icrc = cpu_to_le32(~crc32_le(..))
> The value will still be big endian on the network.
>
> No need to talk about coefficients.
We are looking at the same spec, right? The following is the specification for
the ICRC field:
The resulting bits are sent in order from the bit representing the
coefficient of the highest term of the remainder polynomial. The least
significant bit, most significant byte first ordering of the packet does not
apply to the ICRC field.
So it does talk about the polynomial coefficients.
It sounds like you want to add two unnecessary byteswaps to match the example in
Table 25, which misleadingly shows a byte-swapped ICRC value as a u32 without
mentioning it is byte-swapped. I don't agree that would be clearer, but we can
do it if you prefer.
- Eric
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH 1/6] RDMA/rxe: handle ICRC correctly on big endian systems
2025-01-29 22:21 ` Eric Biggers
@ 2025-01-30 1:29 ` Jason Gunthorpe
2025-01-30 2:04 ` Eric Biggers
0 siblings, 1 reply; 28+ messages in thread
From: Jason Gunthorpe @ 2025-01-30 1:29 UTC (permalink / raw)
To: Eric Biggers
Cc: Zhu Yanjun, linux-rdma, Mustafa Ismail, Tatyana Nikolova,
Leon Romanovsky, Zhu Yanjun, Bernard Metzler,
linux-kernel@vger.kernel.org
On Wed, Jan 29, 2025 at 10:21:47PM +0000, Eric Biggers wrote:
> > To be most clear this should be written as:
> >
> > u32 ibta_crc = swab32(~crc32_le(..)) // Gives you the IBTA defined value
> > *packet = cpu_to_be32(ibta_crc); // Puts it in the packet
> >
> > It follows the spec clearly and exactly.
> >
> > Yes, you can get the same net effect using le:
> >
> > u32 not_ibta_crc = ~crc32_le(..)
> > *packet = cpu_to_le32(not_ibta_crc)
> >
> > It does work, but it is very hard to follow how that relates to the
> > specification when the u32 is not in the spec's format anymore.
> >
> > What matters here, in rxe, is how to use the Linux crc32 library to
> > get exactly the value written down in the spec.
> >
> > IMHO the le approach is an optimization to avoid the dobule swap, and
> > it should simply be described as such in a comment:
> >
> > The crc32 library gives a byte swapped result compared to the IBTA
> > specification. swab32(~crc32_le(..)) will give values that match
> > IBTA.
> >
> > To avoid double swapping we can instead write:
> > *icrc = cpu_to_le32(~crc32_le(..))
> > The value will still be big endian on the network.
> >
> > No need to talk about coefficients.
>
> We are looking at the same spec, right? The following is the specification for
> the ICRC field:
>
> The resulting bits are sent in order from the bit representing the
> coefficient of the highest term of the remainder polynomial. The least
> significant bit, most significant byte first ordering of the packet does not
> apply to the ICRC field.
>
> So it does talk about the polynomial coefficients.
Of course it does, it is defining a CRC.
The above text is reflected in Figure 57 which shows the Remainder
being swapped all around to produce the ICRC.
The spec goes on to say:
CRC Field is obtained from the Remainder as shown in Figure 57. ICRC
Field is transmitted using Big Endian byte ordering like every field
of an InfiniBand packet.
From a spec perspective is *total nonsense* to describe something the
spec explicitly says is big endian as little endian.
Yes from a maths perspective coefficients are reversed and whatever,
but that doesn't matter to someone reading the code. Clearly state
how to calculate the u32 "ICRC Field" as called out in the spec using
Linux. That is swab32(~crc32_le(..)) - that detail clarifies everything.
> It sounds like you want to add two unnecessary byteswaps to match
> the example in Table 25, which misleadingly shows a byte-swapped
> ICRC value as a u32 without mentioning it is byte-swapped.
There are an obnoxious number of ways to make, label and describe
these LFSRs. IBTA choose their representation, Linux choose a
different one.
It isn't misleadingly byte-swapped, it is self consistent with the
rest of the spec, and different from Linux.
> I don't agree that would be clearer, but we can do it if you prefer.
If you want to keep the le32 optimization, then keep it, but have a
comment to explain that it is an optimization based on the logical
be32 double swap that matches the plain text of the spec.
I gave an example comment above.
Jason
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 1/6] RDMA/rxe: handle ICRC correctly on big endian systems
2025-01-30 1:29 ` Jason Gunthorpe
@ 2025-01-30 2:04 ` Eric Biggers
2025-01-30 13:52 ` Jason Gunthorpe
0 siblings, 1 reply; 28+ messages in thread
From: Eric Biggers @ 2025-01-30 2:04 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Zhu Yanjun, linux-rdma, Mustafa Ismail, Tatyana Nikolova,
Leon Romanovsky, Zhu Yanjun, Bernard Metzler,
linux-kernel@vger.kernel.org
On Wed, Jan 29, 2025 at 09:29:51PM -0400, Jason Gunthorpe wrote:
> On Wed, Jan 29, 2025 at 10:21:47PM +0000, Eric Biggers wrote:
>
> > > To be most clear this should be written as:
> > >
> > > u32 ibta_crc = swab32(~crc32_le(..)) // Gives you the IBTA defined value
> > > *packet = cpu_to_be32(ibta_crc); // Puts it in the packet
> > >
> > > It follows the spec clearly and exactly.
> > >
> > > Yes, you can get the same net effect using le:
> > >
> > > u32 not_ibta_crc = ~crc32_le(..)
> > > *packet = cpu_to_le32(not_ibta_crc)
> > >
> > > It does work, but it is very hard to follow how that relates to the
> > > specification when the u32 is not in the spec's format anymore.
> > >
> > > What matters here, in rxe, is how to use the Linux crc32 library to
> > > get exactly the value written down in the spec.
> > >
> > > IMHO the le approach is an optimization to avoid the dobule swap, and
> > > it should simply be described as such in a comment:
> > >
> > > The crc32 library gives a byte swapped result compared to the IBTA
> > > specification. swab32(~crc32_le(..)) will give values that match
> > > IBTA.
> > >
> > > To avoid double swapping we can instead write:
> > > *icrc = cpu_to_le32(~crc32_le(..))
> > > The value will still be big endian on the network.
> > >
> > > No need to talk about coefficients.
> >
> > We are looking at the same spec, right? The following is the specification for
> > the ICRC field:
> >
> > The resulting bits are sent in order from the bit representing the
> > coefficient of the highest term of the remainder polynomial. The least
> > significant bit, most significant byte first ordering of the packet does not
> > apply to the ICRC field.
> >
> > So it does talk about the polynomial coefficients.
>
> Of course it does, it is defining a CRC.
Yes.
> From a spec perspective is *total nonsense* to describe something the
> spec explicitly says is big endian as little endian.
The spec also says "The least significant bit, most significant byte first
ordering of the packet does not apply to the ICRC field." I.e. it is not big
endian. The full explanation would need to point out the two seemingly
conflicting parts of the spec and how they are resolved.
> Yes from a maths perspective coefficients are reversed and whatever,
> but that doesn't matter to someone reading the code.
It does matter. Besides the above, it's also the only way to know whether the
least-significant-bit-first CRC32 (crc32_le()) or most-significant-bit-first
CRC32 (crc32_be()) is the correct function to call.
> There are an obnoxious number of ways to make, label and describe these LFSRs.
> IBTA choose their representation, Linux choose a different one.
It's a CRC, not an LFSR. LFSR is an implementation choice.
> If you want to keep the le32 optimization, then keep it, but have a
> comment to explain that it is an optimization based on the logical
> be32 double swap that matches the plain text of the spec.
I will plan to clarify the comment to point out the two seemingly conflicting
parts of the spec and how they apply.
But feel free to send your own patches that you would approve of though, since
you seem very opinionated about this.
- Eric
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 1/6] RDMA/rxe: handle ICRC correctly on big endian systems
2025-01-30 2:04 ` Eric Biggers
@ 2025-01-30 13:52 ` Jason Gunthorpe
0 siblings, 0 replies; 28+ messages in thread
From: Jason Gunthorpe @ 2025-01-30 13:52 UTC (permalink / raw)
To: Eric Biggers
Cc: Zhu Yanjun, linux-rdma, Mustafa Ismail, Tatyana Nikolova,
Leon Romanovsky, Zhu Yanjun, Bernard Metzler,
linux-kernel@vger.kernel.org
On Wed, Jan 29, 2025 at 06:04:03PM -0800, Eric Biggers wrote:
> > From a spec perspective is *total nonsense* to describe something the
> > spec explicitly says is big endian as little endian.
>
> The spec also says "The least significant bit, most significant byte first
> ordering of the packet does not apply to the ICRC field."
I'm pretty sure this is text trying to explain the Remainder -> ICRC
transformation in Figure 57.
Jason
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 1/6] RDMA/rxe: handle ICRC correctly on big endian systems
2025-01-29 19:43 ` Jason Gunthorpe
2025-01-29 20:25 ` Eric Biggers
@ 2025-01-30 9:17 ` Zhu Yanjun
1 sibling, 0 replies; 28+ messages in thread
From: Zhu Yanjun @ 2025-01-30 9:17 UTC (permalink / raw)
To: Jason Gunthorpe, Eric Biggers
Cc: linux-rdma, Mustafa Ismail, Tatyana Nikolova, Leon Romanovsky,
Zhu Yanjun, Bernard Metzler, linux-kernel@vger.kernel.org
On 29.01.25 20:43, Jason Gunthorpe wrote:
> On Wed, Jan 29, 2025 at 06:51:15PM +0000, Eric Biggers wrote:
>> Hi Jason,
>>
>> On Wed, Jan 29, 2025 at 02:30:09PM -0400, Jason Gunthorpe wrote:
>>> On Wed, Jan 29, 2025 at 10:44:39AM +0100, Zhu Yanjun wrote:
>>>> 在 2025/1/27 23:38, Eric Biggers 写道:
>>>>> From: Eric Biggers <ebiggers@google.com>
>>>>>
>>>>> The usual big endian convention of InfiniBand does not apply to the
>>>>> ICRC field, whose transmission is specified in terms of the CRC32
>>>>> polynomial coefficients.
>>>
>>> This patch is on to something but this is not a good explanation.
>>>
>>> The CRC32 in IB is stored as big endian and computed in big endian,
>>> the spec says so explicitly:
>>>
>>> 2) The CRC calculation is done in big endian byte order with the least
>>> significant bit of the most significant byte being the first
>>> bits in the CRC calculation.
>>
>> (2) just specifies the order in which the bits are passed to the CRC. It says
>> nothing about how the CRC value is stored; that's in (4) instead.
>
> It could be. The other parts of the spec are more definitive.
>
>> The mention of "big endian" seems to refer to the bytes being passed
>> from first to last, which is the nearly universal convention.
>
> The LFSR Figure 57 shows how the numerical representation the spec
> uses (ie the 0x9625B75A) maps to the LFSR, and it could be called big
> endian.
>
> Regardless, table 29 makes this crystal clear, it shows how the
> numerical representation of 0x9625B75A is placed on the wire, it is
> big endian as all IBTA values are - byte 52 is 0x96, byte 55 is 0x5A.
>
>> (I would not have used the term "big endian" here, as it's
>> confusing.)
>
> It could be read as going byte-by-byte, or it could be referring to
> the layout of the numerical representation..
>
>>> If you feed that to the CRC32 you should get 0x9625B75A in a CPU
>>> register u32.
>>>
>>> cpu_to_be32() will put it in the right order for on the wire.
>>
>> I think cpu_to_be32() would put it in the wrong order. Refer to the following:
>
> It is fully explicit in table 29, 0x9625B75A is stored in big
> endian format starting at byte 52.
>
> It is easy to answer without guessing, Zhu should just run the sample
> packet through the new crc library code and determine what the u32
> value is. It is either 0x9625B75A or swab(0x9625B75A) and that will
> tell what the code should be.
Got it. I will run the sample packet through the new crc library code
and determine what the u32 value is.
Zhu Yanjun
>
> Jason
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 1/6] RDMA/rxe: handle ICRC correctly on big endian systems
2025-01-29 18:30 ` Jason Gunthorpe
2025-01-29 18:51 ` Eric Biggers
@ 2025-01-30 7:27 ` Zhu Yanjun
1 sibling, 0 replies; 28+ messages in thread
From: Zhu Yanjun @ 2025-01-30 7:27 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Eric Biggers, linux-rdma, Mustafa Ismail, Tatyana Nikolova,
Leon Romanovsky, Zhu Yanjun, Bernard Metzler,
linux-kernel@vger.kernel.org
在 2025/1/29 19:30, Jason Gunthorpe 写道:
> On Wed, Jan 29, 2025 at 10:44:39AM +0100, Zhu Yanjun wrote:
>> 在 2025/1/27 23:38, Eric Biggers 写道:
>>> From: Eric Biggers <ebiggers@google.com>
>>>
>>> The usual big endian convention of InfiniBand does not apply to the
>>> ICRC field, whose transmission is specified in terms of the CRC32
>>> polynomial coefficients.
>
> This patch is on to something but this is not a good explanation.
>
> The CRC32 in IB is stored as big endian and computed in big endian,
> the spec says so explicitly:
>
> 2) The CRC calculation is done in big endian byte order with the least
> 31 significant bit of the most significant byte being the first
> bits in the 32 CRC calculation.
>
> In this context saying it is not "big endian" doesn't seem to be quite
> right..
>
> The spec gives a sample data packet (in offset/value pairs):
>
> 0 0xF0 15 0xB3 30 0x7A 45 0x8B
> 1 0x12 16 0x00 31 0x05 46 0xC0
> 2 0x37 17 0x0D 32 0x00 47 0x69
> 3 0x5C 18 0xEC 33 0x00 48 0x0E
> 4 0x00 19 0x2A 34 0x00 49 0xD4
> 5 0x0E 20 0x01 35 0x0E 50 0x00
> 6 0x17 21 0x71 36 0xBB 51 0x00
> 7 0xD2 22 0x0A 37 0x88
> 8 0x0A 23 0x1C 38 0x4D
> 9 0x20 24 0x01 39 0x85
> 10 0x24 25 0x5D 40 0xFD
> 11 0x87 26 0x40 41 0x5C
> 12 0xFF 27 0x02 42 0xFB
> 13 0x87 28 0x38 43 0xA4
> 14 0xB1 29 0xF2 44 0x72
>
> If you feed that to the CRC32 you should get 0x9625B75A in a CPU
> register u32.
>
> cpu_to_be32() will put it in the right order for on the wire.
>
> Since rxe doesn't have any cpu_to_be32() on this path, I'm guessing
> the Linux CRC32 implementations gives a u32 with the
> value = 0x5AB72596 ie swapped.
>
> Probably the issue here is that the Linux CRC32 and the IBTA CRC32 are
> using a different mapping of LFSR bits. I love CRCs. So many different
> ways to implement the same thing.
>
> Thus, I guess, the code should really read:
> linux_crc32 = swab32(be32_to_cpu(val));
>
> Which is a NOP on x86 and requires a swap on BE.
>
> Zhu, can you check it for Eric? (this is page 229 in the spec).
Got it. In my IBTA spec, I can find what you mentioned in the IBTA spec.
Your explanation is in details and very nice.
Thanks a lot.
Zhu Yanjun
>
> I assume the Linux CRC32 always gives the same CPU value regardless of
> LE or BE?
>
> Jason
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 1/6] RDMA/rxe: handle ICRC correctly on big endian systems
2025-01-27 22:38 ` [PATCH 1/6] RDMA/rxe: handle ICRC correctly on big endian systems Eric Biggers
2025-01-29 9:44 ` Zhu Yanjun
@ 2025-01-29 18:27 ` Zhu Yanjun
2025-01-29 19:02 ` Eric Biggers
1 sibling, 1 reply; 28+ messages in thread
From: Zhu Yanjun @ 2025-01-29 18:27 UTC (permalink / raw)
To: Eric Biggers, linux-rdma, Mustafa Ismail, Tatyana Nikolova,
Jason Gunthorpe, Leon Romanovsky, Zhu Yanjun, Bernard Metzler
Cc: linux-kernel
在 2025/1/27 23:38, Eric Biggers 写道:
> From: Eric Biggers <ebiggers@google.com>
>
> The usual big endian convention of InfiniBand does not apply to the
> ICRC field, whose transmission is specified in terms of the CRC32
> polynomial coefficients. The coefficients are transmitted in
> descending order from the x^31 coefficient to the x^0 one. When the
> result is interpreted as a 32-bit integer using the required reverse
> mapping between bits and polynomial coefficients, it's a __le32.
In InfiniBand Architecture Specification, the following
"
The resulting bits are sent in order from the bit representing the
coefficient of the highest term of the remainder polynomial. The least
significant bit, most significant byte first ordering of the packet does
not apply to the ICRC field.
"
and
"
The 32 Flip-Flops are initialized to 1 before every ICRC generation. The
packet is fed to the reference design of Figure 57 one bit at a time in
the order specified in Section 7.8.1 on page 222. The Remainder is the
bitwise NOT of the value stored at the 32 Flip-Flops after the last bit
of the packet was clocked into the ICRC Generator. ICRC Field is
obtained from the Remainder as shown in Figure 57. ICRC Field is
transmitted using Big Endian byte ordering like every field of an
InfiniBand packet.
"
It seems that big endian byte ordering is needed in ICRC field. I am not
sure if MLX NIC can connect to RXE after this patchset is applied.
Thanks,
Zhu Yanjun
>
> The code used __be32, but it did not actually do an endianness
> conversion. So it actually just used the CPU's native endianness,
> making it comply with the spec on little endian systems only.
>
> Fix the code to use __le32 and do the needed le32_to_cpu() and
> cpu_to_le32() so that it complies with the spec on big endian systems.
>
> Also update the lower-level helper functions to use u32, as they are
> working with CPU native values. This part does not change behavior.
>
> Found through code review. I don't know whether anyone is actually
> using this code on big endian systems. Probably not.
>
> Signed-off-by: Eric Biggers <ebiggers@google.com>
> ---
> drivers/infiniband/sw/rxe/rxe_icrc.c | 41 +++++++++++++++-------------
> 1 file changed, 22 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/infiniband/sw/rxe/rxe_icrc.c b/drivers/infiniband/sw/rxe/rxe_icrc.c
> index fdf5f08cd8f17..ee417a0bbbdca 100644
> --- a/drivers/infiniband/sw/rxe/rxe_icrc.c
> +++ b/drivers/infiniband/sw/rxe/rxe_icrc.c
> @@ -38,26 +38,26 @@ int rxe_icrc_init(struct rxe_dev *rxe)
> * @next: starting address of current segment
> * @len: length of current segment
> *
> * Return: the cumulative crc32 checksum
> */
> -static __be32 rxe_crc32(struct rxe_dev *rxe, __be32 crc, void *next, size_t len)
> +static u32 rxe_crc32(struct rxe_dev *rxe, u32 crc, void *next, size_t len)
> {
> - __be32 icrc;
> + u32 icrc;
> int err;
>
> SHASH_DESC_ON_STACK(shash, rxe->tfm);
>
> shash->tfm = rxe->tfm;
> - *(__be32 *)shash_desc_ctx(shash) = crc;
> + *(u32 *)shash_desc_ctx(shash) = crc;
> err = crypto_shash_update(shash, next, len);
> if (unlikely(err)) {
> rxe_dbg_dev(rxe, "failed crc calculation, err: %d\n", err);
> - return (__force __be32)crc32_le((__force u32)crc, next, len);
> + return crc32_le(crc, next, len);
> }
>
> - icrc = *(__be32 *)shash_desc_ctx(shash);
> + icrc = *(u32 *)shash_desc_ctx(shash);
> barrier_data(shash_desc_ctx(shash));
>
> return icrc;
> }
>
> @@ -67,18 +67,18 @@ static __be32 rxe_crc32(struct rxe_dev *rxe, __be32 crc, void *next, size_t len)
> * @skb: packet buffer
> * @pkt: packet information
> *
> * Return: the partial ICRC
> */
> -static __be32 rxe_icrc_hdr(struct sk_buff *skb, struct rxe_pkt_info *pkt)
> +static u32 rxe_icrc_hdr(struct sk_buff *skb, struct rxe_pkt_info *pkt)
> {
> unsigned int bth_offset = 0;
> struct iphdr *ip4h = NULL;
> struct ipv6hdr *ip6h = NULL;
> struct udphdr *udph;
> struct rxe_bth *bth;
> - __be32 crc;
> + u32 crc;
> int length;
> int hdr_size = sizeof(struct udphdr) +
> (skb->protocol == htons(ETH_P_IP) ?
> sizeof(struct iphdr) : sizeof(struct ipv6hdr));
> /* pseudo header buffer size is calculate using ipv6 header size since
> @@ -89,11 +89,11 @@ static __be32 rxe_icrc_hdr(struct sk_buff *skb, struct rxe_pkt_info *pkt)
> RXE_BTH_BYTES];
>
> /* This seed is the result of computing a CRC with a seed of
> * 0xfffffff and 8 bytes of 0xff representing a masked LRH.
> */
> - crc = (__force __be32)0xdebb20e3;
> + crc = 0xdebb20e3;
>
> if (skb->protocol == htons(ETH_P_IP)) { /* IPv4 */
> memcpy(pshdr, ip_hdr(skb), hdr_size);
> ip4h = (struct iphdr *)pshdr;
> udph = (struct udphdr *)(ip4h + 1);
> @@ -137,23 +137,27 @@ static __be32 rxe_icrc_hdr(struct sk_buff *skb, struct rxe_pkt_info *pkt)
> *
> * Return: 0 if the values match else an error
> */
> int rxe_icrc_check(struct sk_buff *skb, struct rxe_pkt_info *pkt)
> {
> - __be32 *icrcp;
> - __be32 pkt_icrc;
> - __be32 icrc;
> -
> - icrcp = (__be32 *)(pkt->hdr + pkt->paylen - RXE_ICRC_SIZE);
> - pkt_icrc = *icrcp;
> + /*
> + * The usual big endian convention of InfiniBand does not apply to the
> + * ICRC field, whose transmission is specified in terms of the CRC32
> + * polynomial coefficients. The coefficients are transmitted in
> + * descending order from the x^31 coefficient to the x^0 one. When the
> + * result is interpreted as a 32-bit integer using the required reverse
> + * mapping between bits and polynomial coefficients, it's a __le32.
> + */
> + __le32 *icrcp = (__le32 *)(pkt->hdr + pkt->paylen - RXE_ICRC_SIZE);
> + u32 icrc;
>
> icrc = rxe_icrc_hdr(skb, pkt);
> icrc = rxe_crc32(pkt->rxe, icrc, (u8 *)payload_addr(pkt),
> payload_size(pkt) + bth_pad(pkt));
> icrc = ~icrc;
>
> - if (unlikely(icrc != pkt_icrc))
> + if (unlikely(icrc != le32_to_cpu(*icrcp)))
> return -EINVAL;
>
> return 0;
> }
>
> @@ -162,14 +166,13 @@ int rxe_icrc_check(struct sk_buff *skb, struct rxe_pkt_info *pkt)
> * @skb: packet buffer
> * @pkt: packet information
> */
> void rxe_icrc_generate(struct sk_buff *skb, struct rxe_pkt_info *pkt)
> {
> - __be32 *icrcp;
> - __be32 icrc;
> + __le32 *icrcp = (__le32 *)(pkt->hdr + pkt->paylen - RXE_ICRC_SIZE);
> + u32 icrc;
>
> - icrcp = (__be32 *)(pkt->hdr + pkt->paylen - RXE_ICRC_SIZE);
> icrc = rxe_icrc_hdr(skb, pkt);
> icrc = rxe_crc32(pkt->rxe, icrc, (u8 *)payload_addr(pkt),
> payload_size(pkt) + bth_pad(pkt));
> - *icrcp = ~icrc;
> + *icrcp = cpu_to_le32(~icrc);
> }
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH 1/6] RDMA/rxe: handle ICRC correctly on big endian systems
2025-01-29 18:27 ` Zhu Yanjun
@ 2025-01-29 19:02 ` Eric Biggers
0 siblings, 0 replies; 28+ messages in thread
From: Eric Biggers @ 2025-01-29 19:02 UTC (permalink / raw)
To: Zhu Yanjun
Cc: linux-rdma, Mustafa Ismail, Tatyana Nikolova, Jason Gunthorpe,
Leon Romanovsky, Zhu Yanjun, Bernard Metzler, linux-kernel
On Wed, Jan 29, 2025 at 07:27:20PM +0100, Zhu Yanjun wrote:
> 在 2025/1/27 23:38, Eric Biggers 写道:
> > From: Eric Biggers <ebiggers@google.com>
> >
> > The usual big endian convention of InfiniBand does not apply to the
> > ICRC field, whose transmission is specified in terms of the CRC32
> > polynomial coefficients. The coefficients are transmitted in
> > descending order from the x^31 coefficient to the x^0 one. When the
> > result is interpreted as a 32-bit integer using the required reverse
> > mapping between bits and polynomial coefficients, it's a __le32.
> In InfiniBand Architecture Specification, the following
> "
> The resulting bits are sent in order from the bit representing the
> coefficient of the highest term of the remainder polynomial. The least
> significant bit, most significant byte first ordering of the packet does not
> apply to the ICRC field.
> "
> and
> "
> The 32 Flip-Flops are initialized to 1 before every ICRC generation. The
> packet is fed to the reference design of Figure 57 one bit at a time in the
> order specified in Section 7.8.1 on page 222. The Remainder is the bitwise
> NOT of the value stored at the 32 Flip-Flops after the last bit of the
> packet was clocked into the ICRC Generator. ICRC Field is obtained from the
> Remainder as shown in Figure 57. ICRC Field is transmitted using Big Endian
> byte ordering like every field of an InfiniBand packet.
> "
>
> It seems that big endian byte ordering is needed in ICRC field. I am not
> sure if MLX NIC can connect to RXE after this patchset is applied.
As I mentioned in my response to Jason, it's a least-significant-bit first CRC,
so the mapping between bits and polynomial coefficients is reversed from the
natural order. So that needs to be kept in mind.
In "Figure 56 ICRC Generator" (looking at
https://www.afs.enea.it/asantoro/V1r1_2_1.Release_12062007.pdf) that shows the
sample hardware, it looks like what's going on is that the bytes are already
reversed in the hardware, i.e. what it calls bits 0 through 7 are the
coefficients of x^0 through x^7, which in a software implementation are actually
in bits 31 through 24. Thus in software it would be a __le32.
- Eric
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 2/6] RDMA/rxe: consolidate code for calculating ICRC of packets
2025-01-27 22:38 [PATCH 0/6] RDMA: switch to using CRC32 library functions Eric Biggers
2025-01-27 22:38 ` [PATCH 1/6] RDMA/rxe: handle ICRC correctly on big endian systems Eric Biggers
@ 2025-01-27 22:38 ` Eric Biggers
2025-01-29 18:11 ` Zhu Yanjun
2025-01-27 22:38 ` [PATCH 3/6] RDMA/rxe: switch to using the crc32 library Eric Biggers
` (3 subsequent siblings)
5 siblings, 1 reply; 28+ messages in thread
From: Eric Biggers @ 2025-01-27 22:38 UTC (permalink / raw)
To: linux-rdma, Mustafa Ismail, Tatyana Nikolova, Jason Gunthorpe,
Leon Romanovsky, Zhu Yanjun, Bernard Metzler
Cc: linux-kernel
From: Eric Biggers <ebiggers@google.com>
Since rxe_icrc_hdr() is always immediately followed by updating the CRC
with the packet's payload, just rename it to rxe_icrc() and make it
include the payload in the CRC, so it now handles the entire packet.
This is a refactor with no change in behavior.
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
drivers/infiniband/sw/rxe/rxe_icrc.c | 36 ++++++++++++----------------
1 file changed, 15 insertions(+), 21 deletions(-)
diff --git a/drivers/infiniband/sw/rxe/rxe_icrc.c b/drivers/infiniband/sw/rxe/rxe_icrc.c
index ee417a0bbbdca..c7b0b4673b959 100644
--- a/drivers/infiniband/sw/rxe/rxe_icrc.c
+++ b/drivers/infiniband/sw/rxe/rxe_icrc.c
@@ -60,26 +60,24 @@ static u32 rxe_crc32(struct rxe_dev *rxe, u32 crc, void *next, size_t len)
return icrc;
}
/**
- * rxe_icrc_hdr() - Compute the partial ICRC for the network and transport
- * headers of a packet.
+ * rxe_icrc() - Compute the ICRC of a packet
* @skb: packet buffer
* @pkt: packet information
*
- * Return: the partial ICRC
+ * Return: the ICRC
*/
-static u32 rxe_icrc_hdr(struct sk_buff *skb, struct rxe_pkt_info *pkt)
+static u32 rxe_icrc(struct sk_buff *skb, struct rxe_pkt_info *pkt)
{
unsigned int bth_offset = 0;
struct iphdr *ip4h = NULL;
struct ipv6hdr *ip6h = NULL;
struct udphdr *udph;
struct rxe_bth *bth;
u32 crc;
- int length;
int hdr_size = sizeof(struct udphdr) +
(skb->protocol == htons(ETH_P_IP) ?
sizeof(struct iphdr) : sizeof(struct ipv6hdr));
/* pseudo header buffer size is calculate using ipv6 header size since
* it is bigger than ipv4
@@ -118,17 +116,23 @@ static u32 rxe_icrc_hdr(struct sk_buff *skb, struct rxe_pkt_info *pkt)
bth = (struct rxe_bth *)&pshdr[bth_offset];
/* exclude bth.resv8a */
bth->qpn |= cpu_to_be32(~BTH_QPN_MASK);
- length = hdr_size + RXE_BTH_BYTES;
- crc = rxe_crc32(pkt->rxe, crc, pshdr, length);
+ /* Update the CRC with the first part of the headers. */
+ crc = rxe_crc32(pkt->rxe, crc, pshdr, hdr_size + RXE_BTH_BYTES);
- /* And finish to compute the CRC on the remainder of the headers. */
+ /* Update the CRC with the remainder of the headers. */
crc = rxe_crc32(pkt->rxe, crc, pkt->hdr + RXE_BTH_BYTES,
rxe_opcode[pkt->opcode].length - RXE_BTH_BYTES);
- return crc;
+
+ /* Update the CRC with the payload. */
+ crc = rxe_crc32(pkt->rxe, crc, payload_addr(pkt),
+ payload_size(pkt) + bth_pad(pkt));
+
+ /* Invert the CRC and return it. */
+ return ~crc;
}
/**
* rxe_icrc_check() - Compute ICRC for a packet and compare to the ICRC
* delivered in the packet.
@@ -146,18 +150,12 @@ int rxe_icrc_check(struct sk_buff *skb, struct rxe_pkt_info *pkt)
* descending order from the x^31 coefficient to the x^0 one. When the
* result is interpreted as a 32-bit integer using the required reverse
* mapping between bits and polynomial coefficients, it's a __le32.
*/
__le32 *icrcp = (__le32 *)(pkt->hdr + pkt->paylen - RXE_ICRC_SIZE);
- u32 icrc;
- icrc = rxe_icrc_hdr(skb, pkt);
- icrc = rxe_crc32(pkt->rxe, icrc, (u8 *)payload_addr(pkt),
- payload_size(pkt) + bth_pad(pkt));
- icrc = ~icrc;
-
- if (unlikely(icrc != le32_to_cpu(*icrcp)))
+ if (unlikely(rxe_icrc(skb, pkt) != le32_to_cpu(*icrcp)))
return -EINVAL;
return 0;
}
@@ -167,12 +165,8 @@ int rxe_icrc_check(struct sk_buff *skb, struct rxe_pkt_info *pkt)
* @pkt: packet information
*/
void rxe_icrc_generate(struct sk_buff *skb, struct rxe_pkt_info *pkt)
{
__le32 *icrcp = (__le32 *)(pkt->hdr + pkt->paylen - RXE_ICRC_SIZE);
- u32 icrc;
- icrc = rxe_icrc_hdr(skb, pkt);
- icrc = rxe_crc32(pkt->rxe, icrc, (u8 *)payload_addr(pkt),
- payload_size(pkt) + bth_pad(pkt));
- *icrcp = cpu_to_le32(~icrc);
+ *icrcp = cpu_to_le32(rxe_icrc(skb, pkt));
}
--
2.48.1
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [PATCH 2/6] RDMA/rxe: consolidate code for calculating ICRC of packets
2025-01-27 22:38 ` [PATCH 2/6] RDMA/rxe: consolidate code for calculating ICRC of packets Eric Biggers
@ 2025-01-29 18:11 ` Zhu Yanjun
2025-01-30 2:15 ` Eric Biggers
0 siblings, 1 reply; 28+ messages in thread
From: Zhu Yanjun @ 2025-01-29 18:11 UTC (permalink / raw)
To: Eric Biggers, linux-rdma, Mustafa Ismail, Tatyana Nikolova,
Jason Gunthorpe, Leon Romanovsky, Zhu Yanjun, Bernard Metzler
Cc: linux-kernel
在 2025/1/27 23:38, Eric Biggers 写道:
> From: Eric Biggers <ebiggers@google.com>
>
> Since rxe_icrc_hdr() is always immediately followed by updating the CRC
> with the packet's payload, just rename it to rxe_icrc() and make it
> include the payload in the CRC, so it now handles the entire packet.
>
> This is a refactor with no change in behavior.
In this commit, currently the entire packet are checked while the header
is checked in the original source code.
Now it can work between RXE <----> RXE.
I am not sure whether RXE <---> MLX can work or not.
If it can work well, I am fine with this patch.
Reviewed-by: Zhu Yanjun <yanjun.zhu@linux.dev>
Zhu Yanjun
>
> Signed-off-by: Eric Biggers <ebiggers@google.com>
> ---
> drivers/infiniband/sw/rxe/rxe_icrc.c | 36 ++++++++++++----------------
> 1 file changed, 15 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/infiniband/sw/rxe/rxe_icrc.c b/drivers/infiniband/sw/rxe/rxe_icrc.c
> index ee417a0bbbdca..c7b0b4673b959 100644
> --- a/drivers/infiniband/sw/rxe/rxe_icrc.c
> +++ b/drivers/infiniband/sw/rxe/rxe_icrc.c
> @@ -60,26 +60,24 @@ static u32 rxe_crc32(struct rxe_dev *rxe, u32 crc, void *next, size_t len)
>
> return icrc;
> }
>
> /**
> - * rxe_icrc_hdr() - Compute the partial ICRC for the network and transport
> - * headers of a packet.
> + * rxe_icrc() - Compute the ICRC of a packet
> * @skb: packet buffer
> * @pkt: packet information
> *
> - * Return: the partial ICRC
> + * Return: the ICRC
> */
> -static u32 rxe_icrc_hdr(struct sk_buff *skb, struct rxe_pkt_info *pkt)
> +static u32 rxe_icrc(struct sk_buff *skb, struct rxe_pkt_info *pkt)
> {
> unsigned int bth_offset = 0;
> struct iphdr *ip4h = NULL;
> struct ipv6hdr *ip6h = NULL;
> struct udphdr *udph;
> struct rxe_bth *bth;
> u32 crc;
> - int length;
> int hdr_size = sizeof(struct udphdr) +
> (skb->protocol == htons(ETH_P_IP) ?
> sizeof(struct iphdr) : sizeof(struct ipv6hdr));
> /* pseudo header buffer size is calculate using ipv6 header size since
> * it is bigger than ipv4
> @@ -118,17 +116,23 @@ static u32 rxe_icrc_hdr(struct sk_buff *skb, struct rxe_pkt_info *pkt)
> bth = (struct rxe_bth *)&pshdr[bth_offset];
>
> /* exclude bth.resv8a */
> bth->qpn |= cpu_to_be32(~BTH_QPN_MASK);
>
> - length = hdr_size + RXE_BTH_BYTES;
> - crc = rxe_crc32(pkt->rxe, crc, pshdr, length);
> + /* Update the CRC with the first part of the headers. */
> + crc = rxe_crc32(pkt->rxe, crc, pshdr, hdr_size + RXE_BTH_BYTES);
>
> - /* And finish to compute the CRC on the remainder of the headers. */
> + /* Update the CRC with the remainder of the headers. */
> crc = rxe_crc32(pkt->rxe, crc, pkt->hdr + RXE_BTH_BYTES,
> rxe_opcode[pkt->opcode].length - RXE_BTH_BYTES);
> - return crc;
> +
> + /* Update the CRC with the payload. */
> + crc = rxe_crc32(pkt->rxe, crc, payload_addr(pkt),
> + payload_size(pkt) + bth_pad(pkt));
> +
> + /* Invert the CRC and return it. */
> + return ~crc;
> }
>
> /**
> * rxe_icrc_check() - Compute ICRC for a packet and compare to the ICRC
> * delivered in the packet.
> @@ -146,18 +150,12 @@ int rxe_icrc_check(struct sk_buff *skb, struct rxe_pkt_info *pkt)
> * descending order from the x^31 coefficient to the x^0 one. When the
> * result is interpreted as a 32-bit integer using the required reverse
> * mapping between bits and polynomial coefficients, it's a __le32.
> */
> __le32 *icrcp = (__le32 *)(pkt->hdr + pkt->paylen - RXE_ICRC_SIZE);
> - u32 icrc;
>
> - icrc = rxe_icrc_hdr(skb, pkt);
> - icrc = rxe_crc32(pkt->rxe, icrc, (u8 *)payload_addr(pkt),
> - payload_size(pkt) + bth_pad(pkt));
> - icrc = ~icrc;
> -
> - if (unlikely(icrc != le32_to_cpu(*icrcp)))
> + if (unlikely(rxe_icrc(skb, pkt) != le32_to_cpu(*icrcp)))
> return -EINVAL;
>
> return 0;
> }
>
> @@ -167,12 +165,8 @@ int rxe_icrc_check(struct sk_buff *skb, struct rxe_pkt_info *pkt)
> * @pkt: packet information
> */
> void rxe_icrc_generate(struct sk_buff *skb, struct rxe_pkt_info *pkt)
> {
> __le32 *icrcp = (__le32 *)(pkt->hdr + pkt->paylen - RXE_ICRC_SIZE);
> - u32 icrc;
>
> - icrc = rxe_icrc_hdr(skb, pkt);
> - icrc = rxe_crc32(pkt->rxe, icrc, (u8 *)payload_addr(pkt),
> - payload_size(pkt) + bth_pad(pkt));
> - *icrcp = cpu_to_le32(~icrc);
> + *icrcp = cpu_to_le32(rxe_icrc(skb, pkt));
> }
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH 2/6] RDMA/rxe: consolidate code for calculating ICRC of packets
2025-01-29 18:11 ` Zhu Yanjun
@ 2025-01-30 2:15 ` Eric Biggers
2025-01-30 7:24 ` Zhu Yanjun
0 siblings, 1 reply; 28+ messages in thread
From: Eric Biggers @ 2025-01-30 2:15 UTC (permalink / raw)
To: Zhu Yanjun
Cc: linux-rdma, Mustafa Ismail, Tatyana Nikolova, Jason Gunthorpe,
Leon Romanovsky, Zhu Yanjun, Bernard Metzler, linux-kernel
On Wed, Jan 29, 2025 at 07:11:35PM +0100, Zhu Yanjun wrote:
> 在 2025/1/27 23:38, Eric Biggers 写道:
> > From: Eric Biggers <ebiggers@google.com>
> >
> > Since rxe_icrc_hdr() is always immediately followed by updating the CRC
> > with the packet's payload, just rename it to rxe_icrc() and make it
> > include the payload in the CRC, so it now handles the entire packet.
> >
> > This is a refactor with no change in behavior.
>
> In this commit, currently the entire packet are checked while the header is
> checked in the original source code.
>
> Now it can work between RXE <----> RXE.
> I am not sure whether RXE <---> MLX can work or not.
>
> If it can work well, I am fine with this patch.
>
> Reviewed-by: Zhu Yanjun <yanjun.zhu@linux.dev>
>
Both the header and payload are checksummed both before and after this patch.
Can you elaborate on why you think this patch changed any behavior?
- Eric
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 2/6] RDMA/rxe: consolidate code for calculating ICRC of packets
2025-01-30 2:15 ` Eric Biggers
@ 2025-01-30 7:24 ` Zhu Yanjun
2025-01-31 2:42 ` Eric Biggers
0 siblings, 1 reply; 28+ messages in thread
From: Zhu Yanjun @ 2025-01-30 7:24 UTC (permalink / raw)
To: Eric Biggers
Cc: linux-rdma, Mustafa Ismail, Tatyana Nikolova, Jason Gunthorpe,
Leon Romanovsky, Zhu Yanjun, Bernard Metzler, linux-kernel
在 2025/1/30 3:15, Eric Biggers 写道:
> On Wed, Jan 29, 2025 at 07:11:35PM +0100, Zhu Yanjun wrote:
>> 在 2025/1/27 23:38, Eric Biggers 写道:
>>> From: Eric Biggers <ebiggers@google.com>
>>>
>>> Since rxe_icrc_hdr() is always immediately followed by updating the CRC
>>> with the packet's payload, just rename it to rxe_icrc() and make it
>>> include the payload in the CRC, so it now handles the entire packet.
>>>
>>> This is a refactor with no change in behavior.
>>
>> In this commit, currently the entire packet are checked while the header is
>> checked in the original source code.
>>
>> Now it can work between RXE <----> RXE.
>> I am not sure whether RXE <---> MLX can work or not.
>>
>> If it can work well, I am fine with this patch.
>>
>> Reviewed-by: Zhu Yanjun <yanjun.zhu@linux.dev>
>>
>
> Both the header and payload are checksummed both before and after this patch.
> Can you elaborate on why you think this patch changed any behavior?
From the source code, it seems that only the header is checked. And RXE
can connect to MLX RDMA NIC. That is, the CRC of the header can be
verified both in RXE and MLX RDMA NIC.
Now in your commit, the header and payload are checked. Thus, the CRC
value in RDMA header may be different from the CRC of the header(that
CRC can be verified in RXE and MLX RDMA NIC). Finally the CRC of the
header and payload will not be verified in MLX RDMA NIC?
IMO, after your patchset is applied, if RXE can connect to MLX RDMA NIC,
I am fine with it.
In the function rxe_rcv as below,
"
...
err = rxe_icrc_check(skb, pkt);
if (unlikely(err))
goto drop;
...
"
rxe_icrc_check is called to check the RDMA packet. In your commit, the
icrc is changed. I am not sure whether this icrc can also be verified
correctly in MLX RDMA NIC or not.
Because RXE can connect to MLX RDMA NIC, after your patchset is applied,
hope that RXE can also connect to MLX RDMA NIC successfully.
Thanks,
Zhu Yanjun
>
> - Eric
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH 2/6] RDMA/rxe: consolidate code for calculating ICRC of packets
2025-01-30 7:24 ` Zhu Yanjun
@ 2025-01-31 2:42 ` Eric Biggers
0 siblings, 0 replies; 28+ messages in thread
From: Eric Biggers @ 2025-01-31 2:42 UTC (permalink / raw)
To: Zhu Yanjun
Cc: linux-rdma, Mustafa Ismail, Tatyana Nikolova, Jason Gunthorpe,
Leon Romanovsky, Zhu Yanjun, Bernard Metzler, linux-kernel
On Thu, Jan 30, 2025 at 08:24:59AM +0100, Zhu Yanjun wrote:
> 在 2025/1/30 3:15, Eric Biggers 写道:
> > On Wed, Jan 29, 2025 at 07:11:35PM +0100, Zhu Yanjun wrote:
> > > 在 2025/1/27 23:38, Eric Biggers 写道:
> > > > From: Eric Biggers <ebiggers@google.com>
> > > >
> > > > Since rxe_icrc_hdr() is always immediately followed by updating the CRC
> > > > with the packet's payload, just rename it to rxe_icrc() and make it
> > > > include the payload in the CRC, so it now handles the entire packet.
> > > >
> > > > This is a refactor with no change in behavior.
> > >
> > > In this commit, currently the entire packet are checked while the header is
> > > checked in the original source code.
> > >
> > > Now it can work between RXE <----> RXE.
> > > I am not sure whether RXE <---> MLX can work or not.
> > >
> > > If it can work well, I am fine with this patch.
> > >
> > > Reviewed-by: Zhu Yanjun <yanjun.zhu@linux.dev>
> > >
> >
> > Both the header and payload are checksummed both before and after this patch.
> > Can you elaborate on why you think this patch changed any behavior?
>
> From the source code, it seems that only the header is checked. And RXE can
> connect to MLX RDMA NIC. That is, the CRC of the header can be verified both
> in RXE and MLX RDMA NIC.
>
> Now in your commit, the header and payload are checked. Thus, the CRC value
> in RDMA header may be different from the CRC of the header(that CRC can be
> verified in RXE and MLX RDMA NIC). Finally the CRC of the header and payload
> will not be verified in MLX RDMA NIC?
>
> IMO, after your patchset is applied, if RXE can connect to MLX RDMA NIC, I
> am fine with it.
>
> In the function rxe_rcv as below,
> "
> ...
> err = rxe_icrc_check(skb, pkt);
> if (unlikely(err))
> goto drop;
> ...
> "
> rxe_icrc_check is called to check the RDMA packet. In your commit, the icrc
> is changed. I am not sure whether this icrc can also be verified correctly
> in MLX RDMA NIC or not.
>
> Because RXE can connect to MLX RDMA NIC, after your patchset is applied,
> hope that RXE can also connect to MLX RDMA NIC successfully.
>
> Thanks,
> Zhu Yanjun
Again, the payload was checksummed before too. Please read the whole patch and
not just part of it. In particular, note that as the commit message points out,
both of the calls to rxe_icrc_hdr() were immediately followed by updating the
CRC with the packet's payload.
Anyway, I might just reduce the scope of this patchset to just the bare minimum
changes needed to replace shash with the CRC library anyway, as it seems hard to
get anything else fixed in this subsystem.
- Eric
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 3/6] RDMA/rxe: switch to using the crc32 library
2025-01-27 22:38 [PATCH 0/6] RDMA: switch to using CRC32 library functions Eric Biggers
2025-01-27 22:38 ` [PATCH 1/6] RDMA/rxe: handle ICRC correctly on big endian systems Eric Biggers
2025-01-27 22:38 ` [PATCH 2/6] RDMA/rxe: consolidate code for calculating ICRC of packets Eric Biggers
@ 2025-01-27 22:38 ` Eric Biggers
2025-01-29 18:30 ` Zhu Yanjun
2025-01-27 22:38 ` [PATCH 4/6] RDMA/irdma: switch to using the crc32c library Eric Biggers
` (2 subsequent siblings)
5 siblings, 1 reply; 28+ messages in thread
From: Eric Biggers @ 2025-01-27 22:38 UTC (permalink / raw)
To: linux-rdma, Mustafa Ismail, Tatyana Nikolova, Jason Gunthorpe,
Leon Romanovsky, Zhu Yanjun, Bernard Metzler
Cc: linux-kernel
From: Eric Biggers <ebiggers@google.com>
Now that the crc32() library function takes advantage of
architecture-specific optimizations, it is unnecessary to go through the
crypto API. Just use crc32(). This is much simpler, and it improves
performance due to eliminating the crypto API overhead.
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
drivers/infiniband/sw/rxe/Kconfig | 3 +-
drivers/infiniband/sw/rxe/rxe.c | 3 --
drivers/infiniband/sw/rxe/rxe.h | 1 -
drivers/infiniband/sw/rxe/rxe_icrc.c | 61 ++-------------------------
drivers/infiniband/sw/rxe/rxe_loc.h | 1 -
drivers/infiniband/sw/rxe/rxe_req.c | 1 -
drivers/infiniband/sw/rxe/rxe_verbs.c | 4 --
drivers/infiniband/sw/rxe/rxe_verbs.h | 1 -
8 files changed, 5 insertions(+), 70 deletions(-)
diff --git a/drivers/infiniband/sw/rxe/Kconfig b/drivers/infiniband/sw/rxe/Kconfig
index 06b8dc5093f77..c180e7ebcfc5b 100644
--- a/drivers/infiniband/sw/rxe/Kconfig
+++ b/drivers/infiniband/sw/rxe/Kconfig
@@ -2,12 +2,11 @@
config RDMA_RXE
tristate "Software RDMA over Ethernet (RoCE) driver"
depends on INET && PCI && INFINIBAND
depends on INFINIBAND_VIRT_DMA
select NET_UDP_TUNNEL
- select CRYPTO
- select CRYPTO_CRC32
+ select CRC32
help
This driver implements the InfiniBand RDMA transport over
the Linux network stack. It enables a system with a
standard Ethernet adapter to interoperate with a RoCE
adapter or with another system running the RXE driver.
diff --git a/drivers/infiniband/sw/rxe/rxe.c b/drivers/infiniband/sw/rxe/rxe.c
index 1ba4a0c8726ae..f8ac79ef70143 100644
--- a/drivers/infiniband/sw/rxe/rxe.c
+++ b/drivers/infiniband/sw/rxe/rxe.c
@@ -29,13 +29,10 @@ void rxe_dealloc(struct ib_device *ib_dev)
rxe_pool_cleanup(&rxe->mr_pool);
rxe_pool_cleanup(&rxe->mw_pool);
WARN_ON(!RB_EMPTY_ROOT(&rxe->mcg_tree));
- if (rxe->tfm)
- crypto_free_shash(rxe->tfm);
-
mutex_destroy(&rxe->usdev_lock);
}
/* initialize rxe device parameters */
static void rxe_init_device_param(struct rxe_dev *rxe)
diff --git a/drivers/infiniband/sw/rxe/rxe.h b/drivers/infiniband/sw/rxe/rxe.h
index fe7f970667325..8db65731499d0 100644
--- a/drivers/infiniband/sw/rxe/rxe.h
+++ b/drivers/infiniband/sw/rxe/rxe.h
@@ -19,11 +19,10 @@
#include <rdma/ib_pack.h>
#include <rdma/ib_smi.h>
#include <rdma/ib_umem.h>
#include <rdma/ib_cache.h>
#include <rdma/ib_addr.h>
-#include <crypto/hash.h>
#include "rxe_net.h"
#include "rxe_opcode.h"
#include "rxe_hdr.h"
#include "rxe_param.h"
diff --git a/drivers/infiniband/sw/rxe/rxe_icrc.c b/drivers/infiniband/sw/rxe/rxe_icrc.c
index c7b0b4673b959..63d03f0f71e38 100644
--- a/drivers/infiniband/sw/rxe/rxe_icrc.c
+++ b/drivers/infiniband/sw/rxe/rxe_icrc.c
@@ -7,62 +7,10 @@
#include <linux/crc32.h>
#include "rxe.h"
#include "rxe_loc.h"
-/**
- * rxe_icrc_init() - Initialize crypto function for computing crc32
- * @rxe: rdma_rxe device object
- *
- * Return: 0 on success else an error
- */
-int rxe_icrc_init(struct rxe_dev *rxe)
-{
- struct crypto_shash *tfm;
-
- tfm = crypto_alloc_shash("crc32", 0, 0);
- if (IS_ERR(tfm)) {
- rxe_dbg_dev(rxe, "failed to init crc32 algorithm err: %ld\n",
- PTR_ERR(tfm));
- return PTR_ERR(tfm);
- }
-
- rxe->tfm = tfm;
-
- return 0;
-}
-
-/**
- * rxe_crc32() - Compute cumulative crc32 for a contiguous segment
- * @rxe: rdma_rxe device object
- * @crc: starting crc32 value from previous segments
- * @next: starting address of current segment
- * @len: length of current segment
- *
- * Return: the cumulative crc32 checksum
- */
-static u32 rxe_crc32(struct rxe_dev *rxe, u32 crc, void *next, size_t len)
-{
- u32 icrc;
- int err;
-
- SHASH_DESC_ON_STACK(shash, rxe->tfm);
-
- shash->tfm = rxe->tfm;
- *(u32 *)shash_desc_ctx(shash) = crc;
- err = crypto_shash_update(shash, next, len);
- if (unlikely(err)) {
- rxe_dbg_dev(rxe, "failed crc calculation, err: %d\n", err);
- return crc32_le(crc, next, len);
- }
-
- icrc = *(u32 *)shash_desc_ctx(shash);
- barrier_data(shash_desc_ctx(shash));
-
- return icrc;
-}
-
/**
* rxe_icrc() - Compute the ICRC of a packet
* @skb: packet buffer
* @pkt: packet information
*
@@ -117,19 +65,18 @@ static u32 rxe_icrc(struct sk_buff *skb, struct rxe_pkt_info *pkt)
/* exclude bth.resv8a */
bth->qpn |= cpu_to_be32(~BTH_QPN_MASK);
/* Update the CRC with the first part of the headers. */
- crc = rxe_crc32(pkt->rxe, crc, pshdr, hdr_size + RXE_BTH_BYTES);
+ crc = crc32(crc, pshdr, hdr_size + RXE_BTH_BYTES);
/* Update the CRC with the remainder of the headers. */
- crc = rxe_crc32(pkt->rxe, crc, pkt->hdr + RXE_BTH_BYTES,
- rxe_opcode[pkt->opcode].length - RXE_BTH_BYTES);
+ crc = crc32(crc, pkt->hdr + RXE_BTH_BYTES,
+ rxe_opcode[pkt->opcode].length - RXE_BTH_BYTES);
/* Update the CRC with the payload. */
- crc = rxe_crc32(pkt->rxe, crc, payload_addr(pkt),
- payload_size(pkt) + bth_pad(pkt));
+ crc = crc32(crc, payload_addr(pkt), payload_size(pkt) + bth_pad(pkt));
/* Invert the CRC and return it. */
return ~crc;
}
diff --git a/drivers/infiniband/sw/rxe/rxe_loc.h b/drivers/infiniband/sw/rxe/rxe_loc.h
index ded46119151bb..c57ab8975c5d1 100644
--- a/drivers/infiniband/sw/rxe/rxe_loc.h
+++ b/drivers/infiniband/sw/rxe/rxe_loc.h
@@ -166,11 +166,10 @@ int rxe_completer(struct rxe_qp *qp);
int rxe_requester(struct rxe_qp *qp);
int rxe_sender(struct rxe_qp *qp);
int rxe_receiver(struct rxe_qp *qp);
/* rxe_icrc.c */
-int rxe_icrc_init(struct rxe_dev *rxe);
int rxe_icrc_check(struct sk_buff *skb, struct rxe_pkt_info *pkt);
void rxe_icrc_generate(struct sk_buff *skb, struct rxe_pkt_info *pkt);
void rxe_resp_queue_pkt(struct rxe_qp *qp, struct sk_buff *skb);
diff --git a/drivers/infiniband/sw/rxe/rxe_req.c b/drivers/infiniband/sw/rxe/rxe_req.c
index 87a02f0deb000..9d0392df8a92f 100644
--- a/drivers/infiniband/sw/rxe/rxe_req.c
+++ b/drivers/infiniband/sw/rxe/rxe_req.c
@@ -3,11 +3,10 @@
* Copyright (c) 2016 Mellanox Technologies Ltd. All rights reserved.
* Copyright (c) 2015 System Fabric Works, Inc. All rights reserved.
*/
#include <linux/skbuff.h>
-#include <crypto/hash.h>
#include "rxe.h"
#include "rxe_loc.h"
#include "rxe_queue.h"
diff --git a/drivers/infiniband/sw/rxe/rxe_verbs.c b/drivers/infiniband/sw/rxe/rxe_verbs.c
index 6152a0fdfc8ca..c05379f8b4f57 100644
--- a/drivers/infiniband/sw/rxe/rxe_verbs.c
+++ b/drivers/infiniband/sw/rxe/rxe_verbs.c
@@ -1531,14 +1531,10 @@ int rxe_register_device(struct rxe_dev *rxe, const char *ibdev_name,
ib_set_device_ops(dev, &rxe_dev_ops);
err = ib_device_set_netdev(&rxe->ib_dev, ndev, 1);
if (err)
return err;
- err = rxe_icrc_init(rxe);
- if (err)
- return err;
-
err = ib_register_device(dev, ibdev_name, NULL);
if (err)
rxe_dbg_dev(rxe, "failed with error %d\n", err);
/*
diff --git a/drivers/infiniband/sw/rxe/rxe_verbs.h b/drivers/infiniband/sw/rxe/rxe_verbs.h
index 6573ceec0ef58..6e31134a5fa5b 100644
--- a/drivers/infiniband/sw/rxe/rxe_verbs.h
+++ b/drivers/infiniband/sw/rxe/rxe_verbs.h
@@ -400,11 +400,10 @@ struct rxe_dev {
u64 mmap_offset;
atomic64_t stats_counters[RXE_NUM_OF_COUNTERS];
struct rxe_port port;
- struct crypto_shash *tfm;
};
static inline struct net_device *rxe_ib_device_get_netdev(struct ib_device *dev)
{
return ib_device_get_netdev(dev, RXE_PORT);
--
2.48.1
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [PATCH 3/6] RDMA/rxe: switch to using the crc32 library
2025-01-27 22:38 ` [PATCH 3/6] RDMA/rxe: switch to using the crc32 library Eric Biggers
@ 2025-01-29 18:30 ` Zhu Yanjun
0 siblings, 0 replies; 28+ messages in thread
From: Zhu Yanjun @ 2025-01-29 18:30 UTC (permalink / raw)
To: Eric Biggers, linux-rdma, Mustafa Ismail, Tatyana Nikolova,
Jason Gunthorpe, Leon Romanovsky, Zhu Yanjun, Bernard Metzler
Cc: linux-kernel
在 2025/1/27 23:38, Eric Biggers 写道:
> From: Eric Biggers <ebiggers@google.com>
>
> Now that the crc32() library function takes advantage of
> architecture-specific optimizations, it is unnecessary to go through the
> crypto API. Just use crc32(). This is much simpler, and it improves
> performance due to eliminating the crypto API overhead.
>
> Signed-off-by: Eric Biggers <ebiggers@google.com>
After the patchset is applied, if the RXE can connect to MLX RDMA NIC
successfully, I am fine with it.
Reviewed-by: Zhu Yanjun <yanjun.zhu@linux.dev>
Zhu Yanjun
> ---
> drivers/infiniband/sw/rxe/Kconfig | 3 +-
> drivers/infiniband/sw/rxe/rxe.c | 3 --
> drivers/infiniband/sw/rxe/rxe.h | 1 -
> drivers/infiniband/sw/rxe/rxe_icrc.c | 61 ++-------------------------
> drivers/infiniband/sw/rxe/rxe_loc.h | 1 -
> drivers/infiniband/sw/rxe/rxe_req.c | 1 -
> drivers/infiniband/sw/rxe/rxe_verbs.c | 4 --
> drivers/infiniband/sw/rxe/rxe_verbs.h | 1 -
> 8 files changed, 5 insertions(+), 70 deletions(-)
>
> diff --git a/drivers/infiniband/sw/rxe/Kconfig b/drivers/infiniband/sw/rxe/Kconfig
> index 06b8dc5093f77..c180e7ebcfc5b 100644
> --- a/drivers/infiniband/sw/rxe/Kconfig
> +++ b/drivers/infiniband/sw/rxe/Kconfig
> @@ -2,12 +2,11 @@
> config RDMA_RXE
> tristate "Software RDMA over Ethernet (RoCE) driver"
> depends on INET && PCI && INFINIBAND
> depends on INFINIBAND_VIRT_DMA
> select NET_UDP_TUNNEL
> - select CRYPTO
> - select CRYPTO_CRC32
> + select CRC32
> help
> This driver implements the InfiniBand RDMA transport over
> the Linux network stack. It enables a system with a
> standard Ethernet adapter to interoperate with a RoCE
> adapter or with another system running the RXE driver.
> diff --git a/drivers/infiniband/sw/rxe/rxe.c b/drivers/infiniband/sw/rxe/rxe.c
> index 1ba4a0c8726ae..f8ac79ef70143 100644
> --- a/drivers/infiniband/sw/rxe/rxe.c
> +++ b/drivers/infiniband/sw/rxe/rxe.c
> @@ -29,13 +29,10 @@ void rxe_dealloc(struct ib_device *ib_dev)
> rxe_pool_cleanup(&rxe->mr_pool);
> rxe_pool_cleanup(&rxe->mw_pool);
>
> WARN_ON(!RB_EMPTY_ROOT(&rxe->mcg_tree));
>
> - if (rxe->tfm)
> - crypto_free_shash(rxe->tfm);
> -
> mutex_destroy(&rxe->usdev_lock);
> }
>
> /* initialize rxe device parameters */
> static void rxe_init_device_param(struct rxe_dev *rxe)
> diff --git a/drivers/infiniband/sw/rxe/rxe.h b/drivers/infiniband/sw/rxe/rxe.h
> index fe7f970667325..8db65731499d0 100644
> --- a/drivers/infiniband/sw/rxe/rxe.h
> +++ b/drivers/infiniband/sw/rxe/rxe.h
> @@ -19,11 +19,10 @@
> #include <rdma/ib_pack.h>
> #include <rdma/ib_smi.h>
> #include <rdma/ib_umem.h>
> #include <rdma/ib_cache.h>
> #include <rdma/ib_addr.h>
> -#include <crypto/hash.h>
>
> #include "rxe_net.h"
> #include "rxe_opcode.h"
> #include "rxe_hdr.h"
> #include "rxe_param.h"
> diff --git a/drivers/infiniband/sw/rxe/rxe_icrc.c b/drivers/infiniband/sw/rxe/rxe_icrc.c
> index c7b0b4673b959..63d03f0f71e38 100644
> --- a/drivers/infiniband/sw/rxe/rxe_icrc.c
> +++ b/drivers/infiniband/sw/rxe/rxe_icrc.c
> @@ -7,62 +7,10 @@
> #include <linux/crc32.h>
>
> #include "rxe.h"
> #include "rxe_loc.h"
>
> -/**
> - * rxe_icrc_init() - Initialize crypto function for computing crc32
> - * @rxe: rdma_rxe device object
> - *
> - * Return: 0 on success else an error
> - */
> -int rxe_icrc_init(struct rxe_dev *rxe)
> -{
> - struct crypto_shash *tfm;
> -
> - tfm = crypto_alloc_shash("crc32", 0, 0);
> - if (IS_ERR(tfm)) {
> - rxe_dbg_dev(rxe, "failed to init crc32 algorithm err: %ld\n",
> - PTR_ERR(tfm));
> - return PTR_ERR(tfm);
> - }
> -
> - rxe->tfm = tfm;
> -
> - return 0;
> -}
> -
> -/**
> - * rxe_crc32() - Compute cumulative crc32 for a contiguous segment
> - * @rxe: rdma_rxe device object
> - * @crc: starting crc32 value from previous segments
> - * @next: starting address of current segment
> - * @len: length of current segment
> - *
> - * Return: the cumulative crc32 checksum
> - */
> -static u32 rxe_crc32(struct rxe_dev *rxe, u32 crc, void *next, size_t len)
> -{
> - u32 icrc;
> - int err;
> -
> - SHASH_DESC_ON_STACK(shash, rxe->tfm);
> -
> - shash->tfm = rxe->tfm;
> - *(u32 *)shash_desc_ctx(shash) = crc;
> - err = crypto_shash_update(shash, next, len);
> - if (unlikely(err)) {
> - rxe_dbg_dev(rxe, "failed crc calculation, err: %d\n", err);
> - return crc32_le(crc, next, len);
> - }
> -
> - icrc = *(u32 *)shash_desc_ctx(shash);
> - barrier_data(shash_desc_ctx(shash));
> -
> - return icrc;
> -}
> -
> /**
> * rxe_icrc() - Compute the ICRC of a packet
> * @skb: packet buffer
> * @pkt: packet information
> *
> @@ -117,19 +65,18 @@ static u32 rxe_icrc(struct sk_buff *skb, struct rxe_pkt_info *pkt)
>
> /* exclude bth.resv8a */
> bth->qpn |= cpu_to_be32(~BTH_QPN_MASK);
>
> /* Update the CRC with the first part of the headers. */
> - crc = rxe_crc32(pkt->rxe, crc, pshdr, hdr_size + RXE_BTH_BYTES);
> + crc = crc32(crc, pshdr, hdr_size + RXE_BTH_BYTES);
>
> /* Update the CRC with the remainder of the headers. */
> - crc = rxe_crc32(pkt->rxe, crc, pkt->hdr + RXE_BTH_BYTES,
> - rxe_opcode[pkt->opcode].length - RXE_BTH_BYTES);
> + crc = crc32(crc, pkt->hdr + RXE_BTH_BYTES,
> + rxe_opcode[pkt->opcode].length - RXE_BTH_BYTES);
>
> /* Update the CRC with the payload. */
> - crc = rxe_crc32(pkt->rxe, crc, payload_addr(pkt),
> - payload_size(pkt) + bth_pad(pkt));
> + crc = crc32(crc, payload_addr(pkt), payload_size(pkt) + bth_pad(pkt));
>
> /* Invert the CRC and return it. */
> return ~crc;
> }
>
> diff --git a/drivers/infiniband/sw/rxe/rxe_loc.h b/drivers/infiniband/sw/rxe/rxe_loc.h
> index ded46119151bb..c57ab8975c5d1 100644
> --- a/drivers/infiniband/sw/rxe/rxe_loc.h
> +++ b/drivers/infiniband/sw/rxe/rxe_loc.h
> @@ -166,11 +166,10 @@ int rxe_completer(struct rxe_qp *qp);
> int rxe_requester(struct rxe_qp *qp);
> int rxe_sender(struct rxe_qp *qp);
> int rxe_receiver(struct rxe_qp *qp);
>
> /* rxe_icrc.c */
> -int rxe_icrc_init(struct rxe_dev *rxe);
> int rxe_icrc_check(struct sk_buff *skb, struct rxe_pkt_info *pkt);
> void rxe_icrc_generate(struct sk_buff *skb, struct rxe_pkt_info *pkt);
>
> void rxe_resp_queue_pkt(struct rxe_qp *qp, struct sk_buff *skb);
>
> diff --git a/drivers/infiniband/sw/rxe/rxe_req.c b/drivers/infiniband/sw/rxe/rxe_req.c
> index 87a02f0deb000..9d0392df8a92f 100644
> --- a/drivers/infiniband/sw/rxe/rxe_req.c
> +++ b/drivers/infiniband/sw/rxe/rxe_req.c
> @@ -3,11 +3,10 @@
> * Copyright (c) 2016 Mellanox Technologies Ltd. All rights reserved.
> * Copyright (c) 2015 System Fabric Works, Inc. All rights reserved.
> */
>
> #include <linux/skbuff.h>
> -#include <crypto/hash.h>
>
> #include "rxe.h"
> #include "rxe_loc.h"
> #include "rxe_queue.h"
>
> diff --git a/drivers/infiniband/sw/rxe/rxe_verbs.c b/drivers/infiniband/sw/rxe/rxe_verbs.c
> index 6152a0fdfc8ca..c05379f8b4f57 100644
> --- a/drivers/infiniband/sw/rxe/rxe_verbs.c
> +++ b/drivers/infiniband/sw/rxe/rxe_verbs.c
> @@ -1531,14 +1531,10 @@ int rxe_register_device(struct rxe_dev *rxe, const char *ibdev_name,
> ib_set_device_ops(dev, &rxe_dev_ops);
> err = ib_device_set_netdev(&rxe->ib_dev, ndev, 1);
> if (err)
> return err;
>
> - err = rxe_icrc_init(rxe);
> - if (err)
> - return err;
> -
> err = ib_register_device(dev, ibdev_name, NULL);
> if (err)
> rxe_dbg_dev(rxe, "failed with error %d\n", err);
>
> /*
> diff --git a/drivers/infiniband/sw/rxe/rxe_verbs.h b/drivers/infiniband/sw/rxe/rxe_verbs.h
> index 6573ceec0ef58..6e31134a5fa5b 100644
> --- a/drivers/infiniband/sw/rxe/rxe_verbs.h
> +++ b/drivers/infiniband/sw/rxe/rxe_verbs.h
> @@ -400,11 +400,10 @@ struct rxe_dev {
> u64 mmap_offset;
>
> atomic64_t stats_counters[RXE_NUM_OF_COUNTERS];
>
> struct rxe_port port;
> - struct crypto_shash *tfm;
> };
>
> static inline struct net_device *rxe_ib_device_get_netdev(struct ib_device *dev)
> {
> return ib_device_get_netdev(dev, RXE_PORT);
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 4/6] RDMA/irdma: switch to using the crc32c library
2025-01-27 22:38 [PATCH 0/6] RDMA: switch to using CRC32 library functions Eric Biggers
` (2 preceding siblings ...)
2025-01-27 22:38 ` [PATCH 3/6] RDMA/rxe: switch to using the crc32 library Eric Biggers
@ 2025-01-27 22:38 ` Eric Biggers
2025-01-27 22:38 ` [PATCH 5/6] RDMA/siw: fix type of CRC field Eric Biggers
2025-01-27 22:38 ` [PATCH 6/6] RDMA/siw: switch to using the crc32c library Eric Biggers
5 siblings, 0 replies; 28+ messages in thread
From: Eric Biggers @ 2025-01-27 22:38 UTC (permalink / raw)
To: linux-rdma, Mustafa Ismail, Tatyana Nikolova, Jason Gunthorpe,
Leon Romanovsky, Zhu Yanjun, Bernard Metzler
Cc: linux-kernel
From: Eric Biggers <ebiggers@google.com>
Now that the crc32c() library function directly takes advantage of
architecture-specific optimizations, it is unnecessary to go through the
crypto API. Just use crc32c(). This is much simpler, and it improves
performance due to eliminating the crypto API overhead.
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
drivers/infiniband/hw/irdma/Kconfig | 1 +
drivers/infiniband/hw/irdma/main.h | 1 -
drivers/infiniband/hw/irdma/osdep.h | 6 +---
drivers/infiniband/hw/irdma/puda.c | 19 +++++-------
drivers/infiniband/hw/irdma/puda.h | 5 +--
drivers/infiniband/hw/irdma/utils.c | 47 ++---------------------------
6 files changed, 12 insertions(+), 67 deletions(-)
diff --git a/drivers/infiniband/hw/irdma/Kconfig b/drivers/infiniband/hw/irdma/Kconfig
index b6f9c41bca51d..5f49a58590ed7 100644
--- a/drivers/infiniband/hw/irdma/Kconfig
+++ b/drivers/infiniband/hw/irdma/Kconfig
@@ -5,8 +5,9 @@ config INFINIBAND_IRDMA
depends on IPV6 || !IPV6
depends on PCI
depends on ICE && I40E
select GENERIC_ALLOCATOR
select AUXILIARY_BUS
+ select CRC32
help
This is an Intel(R) Ethernet Protocol Driver for RDMA driver
that support E810 (iWARP/RoCE) and X722 (iWARP) network devices.
diff --git a/drivers/infiniband/hw/irdma/main.h b/drivers/infiniband/hw/irdma/main.h
index 9f0ed6e844711..0705ef3d72a93 100644
--- a/drivers/infiniband/hw/irdma/main.h
+++ b/drivers/infiniband/hw/irdma/main.h
@@ -28,11 +28,10 @@
#ifndef CONFIG_64BIT
#include <linux/io-64-nonatomic-lo-hi.h>
#endif
#include <linux/auxiliary_bus.h>
#include <linux/net/intel/iidc.h>
-#include <crypto/hash.h>
#include <rdma/ib_smi.h>
#include <rdma/ib_verbs.h>
#include <rdma/ib_pack.h>
#include <rdma/rdma_cm.h>
#include <rdma/iw_cm.h>
diff --git a/drivers/infiniband/hw/irdma/osdep.h b/drivers/infiniband/hw/irdma/osdep.h
index ddf02a462efa2..4b4f78288d12e 100644
--- a/drivers/infiniband/hw/irdma/osdep.h
+++ b/drivers/infiniband/hw/irdma/osdep.h
@@ -4,11 +4,10 @@
#define IRDMA_OSDEP_H
#include <linux/pci.h>
#include <linux/bitfield.h>
#include <linux/net/intel/iidc.h>
-#include <crypto/hash.h>
#include <rdma/ib_verbs.h>
#define STATS_TIMER_DELAY 60000
struct irdma_dma_info {
@@ -41,19 +40,16 @@ struct ib_device *to_ibdev(struct irdma_sc_dev *dev);
void irdma_ieq_mpa_crc_ae(struct irdma_sc_dev *dev, struct irdma_sc_qp *qp);
enum irdma_status_code irdma_vf_wait_vchnl_resp(struct irdma_sc_dev *dev);
bool irdma_vf_clear_to_send(struct irdma_sc_dev *dev);
void irdma_add_dev_ref(struct irdma_sc_dev *dev);
void irdma_put_dev_ref(struct irdma_sc_dev *dev);
-int irdma_ieq_check_mpacrc(struct shash_desc *desc, void *addr, u32 len,
- u32 val);
+int irdma_ieq_check_mpacrc(const void *addr, u32 len, u32 val);
struct irdma_sc_qp *irdma_ieq_get_qp(struct irdma_sc_dev *dev,
struct irdma_puda_buf *buf);
void irdma_send_ieq_ack(struct irdma_sc_qp *qp);
void irdma_ieq_update_tcpip_info(struct irdma_puda_buf *buf, u16 len,
u32 seqnum);
-void irdma_free_hash_desc(struct shash_desc *hash_desc);
-int irdma_init_hash_desc(struct shash_desc **hash_desc);
int irdma_puda_get_tcpip_info(struct irdma_puda_cmpl_info *info,
struct irdma_puda_buf *buf);
int irdma_cqp_sds_cmd(struct irdma_sc_dev *dev,
struct irdma_update_sds_info *info);
int irdma_cqp_manage_hmc_fcn_cmd(struct irdma_sc_dev *dev,
diff --git a/drivers/infiniband/hw/irdma/puda.c b/drivers/infiniband/hw/irdma/puda.c
index 7e3f9bca2c235..694e5a9ed15d0 100644
--- a/drivers/infiniband/hw/irdma/puda.c
+++ b/drivers/infiniband/hw/irdma/puda.c
@@ -921,12 +921,10 @@ void irdma_puda_dele_rsrc(struct irdma_sc_vsi *vsi, enum puda_rsrc_type type,
return;
}
switch (rsrc->cmpl) {
case PUDA_HASH_CRC_COMPLETE:
- irdma_free_hash_desc(rsrc->hash_desc);
- fallthrough;
case PUDA_QP_CREATED:
irdma_qp_rem_qos(&rsrc->qp);
if (!reset)
irdma_puda_free_qp(rsrc);
@@ -1093,19 +1091,16 @@ int irdma_puda_create_rsrc(struct irdma_sc_vsi *vsi,
ret = irdma_puda_replenish_rq(rsrc, true);
if (ret)
goto error;
if (info->type == IRDMA_PUDA_RSRC_TYPE_IEQ) {
- if (!irdma_init_hash_desc(&rsrc->hash_desc)) {
- rsrc->check_crc = true;
- rsrc->cmpl = PUDA_HASH_CRC_COMPLETE;
- ret = 0;
- }
+ rsrc->check_crc = true;
+ rsrc->cmpl = PUDA_HASH_CRC_COMPLETE;
}
irdma_sc_ccq_arm(&rsrc->cq);
- return ret;
+ return 0;
error:
irdma_puda_dele_rsrc(vsi, info->type, false);
return ret;
@@ -1394,12 +1389,12 @@ static int irdma_ieq_handle_partial(struct irdma_puda_rsrc *ieq,
irdma_ieq_update_tcpip_info(txbuf, fpdu_len, seqnum);
crcptr = txbuf->data + fpdu_len - 4;
mpacrc = *(u32 *)crcptr;
if (ieq->check_crc) {
- status = irdma_ieq_check_mpacrc(ieq->hash_desc, txbuf->data,
- (fpdu_len - 4), mpacrc);
+ status = irdma_ieq_check_mpacrc(txbuf->data, fpdu_len - 4,
+ mpacrc);
if (status) {
ibdev_dbg(to_ibdev(ieq->dev), "IEQ: error bad crc\n");
goto error;
}
}
@@ -1463,12 +1458,12 @@ static int irdma_ieq_process_buf(struct irdma_puda_rsrc *ieq,
break;
}
crcptr = datap + fpdu_len - 4;
mpacrc = *(u32 *)crcptr;
if (ieq->check_crc)
- ret = irdma_ieq_check_mpacrc(ieq->hash_desc, datap,
- fpdu_len - 4, mpacrc);
+ ret = irdma_ieq_check_mpacrc(datap, fpdu_len - 4,
+ mpacrc);
if (ret) {
list_add(&buf->list, rxlist);
ibdev_dbg(to_ibdev(ieq->dev),
"ERR: IRDMA_ERR_MPA_CRC\n");
return -EINVAL;
diff --git a/drivers/infiniband/hw/irdma/puda.h b/drivers/infiniband/hw/irdma/puda.h
index bc6d9514c9c10..2fc638f2b1434 100644
--- a/drivers/infiniband/hw/irdma/puda.h
+++ b/drivers/infiniband/hw/irdma/puda.h
@@ -117,11 +117,10 @@ struct irdma_puda_rsrc {
u64 *rq_wrid_array;
u32 compl_rxwqe_idx;
u32 rx_wqe_idx;
u32 rxq_invalid_cnt;
u32 tx_wqe_avail_cnt;
- struct shash_desc *hash_desc;
struct list_head txpend;
struct list_head bufpool; /* free buffers pool list for recv and xmit */
u32 alloc_buf_count;
u32 avail_buf_count; /* snapshot of currently available buffers */
spinlock_t bufpool_lock;
@@ -161,14 +160,12 @@ int irdma_puda_poll_cmpl(struct irdma_sc_dev *dev, struct irdma_sc_cq *cq,
struct irdma_sc_qp *irdma_ieq_get_qp(struct irdma_sc_dev *dev,
struct irdma_puda_buf *buf);
int irdma_puda_get_tcpip_info(struct irdma_puda_cmpl_info *info,
struct irdma_puda_buf *buf);
-int irdma_ieq_check_mpacrc(struct shash_desc *desc, void *addr, u32 len, u32 val);
-int irdma_init_hash_desc(struct shash_desc **desc);
+int irdma_ieq_check_mpacrc(const void *addr, u32 len, u32 val);
void irdma_ieq_mpa_crc_ae(struct irdma_sc_dev *dev, struct irdma_sc_qp *qp);
-void irdma_free_hash_desc(struct shash_desc *desc);
void irdma_ieq_update_tcpip_info(struct irdma_puda_buf *buf, u16 len, u32 seqnum);
int irdma_cqp_qp_create_cmd(struct irdma_sc_dev *dev, struct irdma_sc_qp *qp);
int irdma_cqp_cq_create_cmd(struct irdma_sc_dev *dev, struct irdma_sc_cq *cq);
int irdma_cqp_qp_destroy_cmd(struct irdma_sc_dev *dev, struct irdma_sc_qp *qp);
void irdma_cqp_cq_destroy_cmd(struct irdma_sc_dev *dev, struct irdma_sc_cq *cq);
diff --git a/drivers/infiniband/hw/irdma/utils.c b/drivers/infiniband/hw/irdma/utils.c
index 0e594122baa78..57c3335be103c 100644
--- a/drivers/infiniband/hw/irdma/utils.c
+++ b/drivers/infiniband/hw/irdma/utils.c
@@ -1271,62 +1271,19 @@ void irdma_ieq_mpa_crc_ae(struct irdma_sc_dev *dev, struct irdma_sc_qp *qp)
info.ae_code = IRDMA_AE_LLP_RECEIVED_MPA_CRC_ERROR;
info.ae_src = IRDMA_AE_SOURCE_RQ;
irdma_gen_ae(rf, qp, &info, false);
}
-/**
- * irdma_init_hash_desc - initialize hash for crc calculation
- * @desc: cryption type
- */
-int irdma_init_hash_desc(struct shash_desc **desc)
-{
- struct crypto_shash *tfm;
- struct shash_desc *tdesc;
-
- tfm = crypto_alloc_shash("crc32c", 0, 0);
- if (IS_ERR(tfm))
- return -EINVAL;
-
- tdesc = kzalloc(sizeof(*tdesc) + crypto_shash_descsize(tfm),
- GFP_KERNEL);
- if (!tdesc) {
- crypto_free_shash(tfm);
- return -EINVAL;
- }
-
- tdesc->tfm = tfm;
- *desc = tdesc;
-
- return 0;
-}
-
-/**
- * irdma_free_hash_desc - free hash desc
- * @desc: to be freed
- */
-void irdma_free_hash_desc(struct shash_desc *desc)
-{
- if (desc) {
- crypto_free_shash(desc->tfm);
- kfree(desc);
- }
-}
-
/**
* irdma_ieq_check_mpacrc - check if mpa crc is OK
- * @desc: desc for hash
* @addr: address of buffer for crc
* @len: length of buffer
* @val: value to be compared
*/
-int irdma_ieq_check_mpacrc(struct shash_desc *desc, void *addr, u32 len,
- u32 val)
+int irdma_ieq_check_mpacrc(const void *addr, u32 len, u32 val)
{
- u32 crc = 0;
-
- crypto_shash_digest(desc, addr, len, (u8 *)&crc);
- if (crc != val)
+ if (~crc32c(~0, addr, len) != val)
return -EINVAL;
return 0;
}
--
2.48.1
^ permalink raw reply related [flat|nested] 28+ messages in thread* [PATCH 5/6] RDMA/siw: fix type of CRC field
2025-01-27 22:38 [PATCH 0/6] RDMA: switch to using CRC32 library functions Eric Biggers
` (3 preceding siblings ...)
2025-01-27 22:38 ` [PATCH 4/6] RDMA/irdma: switch to using the crc32c library Eric Biggers
@ 2025-01-27 22:38 ` Eric Biggers
2025-01-31 12:24 ` Bernard Metzler
2025-01-27 22:38 ` [PATCH 6/6] RDMA/siw: switch to using the crc32c library Eric Biggers
5 siblings, 1 reply; 28+ messages in thread
From: Eric Biggers @ 2025-01-27 22:38 UTC (permalink / raw)
To: linux-rdma, Mustafa Ismail, Tatyana Nikolova, Jason Gunthorpe,
Leon Romanovsky, Zhu Yanjun, Bernard Metzler
Cc: linux-kernel
From: Eric Biggers <ebiggers@google.com>
The usual big endian convention of InfiniBand does not apply to the MPA
CRC field, whose transmission is specified in terms of the CRC32
polynomial coefficients. The coefficients are transmitted in descending
order from the x^31 coefficient to the x^0 one. When the result is
interpreted as a 32-bit integer using the required reverse mapping
between bits and polynomial coefficients, it's a __le32.
Fix the code to use the correct type.
The endianness conversion to little endian was actually already done in
crypto_shash_final(). Therefore this patch does not change any
behavior, except for adding the missing le32_to_cpu() to the pr_warn()
message in siw_get_trailer() which makes the correct values be printed
on big endian systems.
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
drivers/infiniband/sw/siw/iwarp.h | 2 +-
drivers/infiniband/sw/siw/siw.h | 8 ++++----
drivers/infiniband/sw/siw/siw_qp.c | 2 +-
drivers/infiniband/sw/siw/siw_qp_rx.c | 16 +++++++++++-----
drivers/infiniband/sw/siw/siw_qp_tx.c | 2 +-
5 files changed, 18 insertions(+), 12 deletions(-)
diff --git a/drivers/infiniband/sw/siw/iwarp.h b/drivers/infiniband/sw/siw/iwarp.h
index 8cf69309827d6..afebb5d8643e3 100644
--- a/drivers/infiniband/sw/siw/iwarp.h
+++ b/drivers/infiniband/sw/siw/iwarp.h
@@ -79,11 +79,11 @@ struct mpa_marker {
/*
* maximum MPA trailer
*/
struct mpa_trailer {
__u8 pad[4];
- __be32 crc;
+ __le32 crc;
};
#define MPA_HDR_SIZE 2
#define MPA_CRC_SIZE 4
diff --git a/drivers/infiniband/sw/siw/siw.h b/drivers/infiniband/sw/siw/siw.h
index ea5eee50dc39d..50649971f6254 100644
--- a/drivers/infiniband/sw/siw/siw.h
+++ b/drivers/infiniband/sw/siw/siw.h
@@ -336,26 +336,26 @@ struct siw_rx_fpdu {
* Shorthands for short packets w/o payload
* to be transmitted more efficient.
*/
struct siw_send_pkt {
struct iwarp_send send;
- __be32 crc;
+ __le32 crc;
};
struct siw_write_pkt {
struct iwarp_rdma_write write;
- __be32 crc;
+ __le32 crc;
};
struct siw_rreq_pkt {
struct iwarp_rdma_rreq rreq;
- __be32 crc;
+ __le32 crc;
};
struct siw_rresp_pkt {
struct iwarp_rdma_rresp rresp;
- __be32 crc;
+ __le32 crc;
};
struct siw_iwarp_tx {
union {
union iwarp_hdr hdr;
diff --git a/drivers/infiniband/sw/siw/siw_qp.c b/drivers/infiniband/sw/siw/siw_qp.c
index da92cfa2073d7..ea7d2f5c8b8ee 100644
--- a/drivers/infiniband/sw/siw/siw_qp.c
+++ b/drivers/infiniband/sw/siw/siw_qp.c
@@ -394,11 +394,11 @@ void siw_send_terminate(struct siw_qp *qp)
struct iwarp_terminate *term = NULL;
union iwarp_hdr *err_hdr = NULL;
struct socket *s = qp->attrs.sk;
struct siw_rx_stream *srx = &qp->rx_stream;
union iwarp_hdr *rx_hdr = &srx->hdr;
- u32 crc = 0;
+ __le32 crc = 0;
int num_frags, len_terminate, rv;
if (!qp->term_info.valid)
return;
diff --git a/drivers/infiniband/sw/siw/siw_qp_rx.c b/drivers/infiniband/sw/siw/siw_qp_rx.c
index ed4fc39718b49..098e32fb36fb1 100644
--- a/drivers/infiniband/sw/siw/siw_qp_rx.c
+++ b/drivers/infiniband/sw/siw/siw_qp_rx.c
@@ -951,11 +951,11 @@ int siw_proc_terminate(struct siw_qp *qp)
static int siw_get_trailer(struct siw_qp *qp, struct siw_rx_stream *srx)
{
struct sk_buff *skb = srx->skb;
int avail = min(srx->skb_new, srx->fpdu_part_rem);
u8 *tbuf = (u8 *)&srx->trailer.crc - srx->pad;
- __wsum crc_in, crc_own = 0;
+ __le32 crc_in, crc_own = 0;
siw_dbg_qp(qp, "expected %d, available %d, pad %u\n",
srx->fpdu_part_rem, srx->skb_new, srx->pad);
skb_copy_bits(skb, srx->skb_offset, tbuf, avail);
@@ -969,20 +969,26 @@ static int siw_get_trailer(struct siw_qp *qp, struct siw_rx_stream *srx)
if (!srx->mpa_crc_hd)
return 0;
if (srx->pad)
crypto_shash_update(srx->mpa_crc_hd, tbuf, srx->pad);
+
/*
- * CRC32 is computed, transmitted and received directly in NBO,
- * so there's never a reason to convert byte order.
+ * The usual big endian convention of InfiniBand does not apply to the
+ * CRC field, whose transmission is specified in terms of the CRC32
+ * polynomial coefficients. The coefficients are transmitted in
+ * descending order from the x^31 coefficient to the x^0 one. When the
+ * result is interpreted as a 32-bit integer using the required reverse
+ * mapping between bits and polynomial coefficients, it's a __le32.
*/
crypto_shash_final(srx->mpa_crc_hd, (u8 *)&crc_own);
- crc_in = (__force __wsum)srx->trailer.crc;
+ crc_in = srx->trailer.crc;
if (unlikely(crc_in != crc_own)) {
pr_warn("siw: crc error. in: %08x, own %08x, op %u\n",
- crc_in, crc_own, qp->rx_stream.rdmap_op);
+ le32_to_cpu(crc_in), le32_to_cpu(crc_own),
+ qp->rx_stream.rdmap_op);
siw_init_terminate(qp, TERM_ERROR_LAYER_LLP,
LLP_ETYPE_MPA,
LLP_ECODE_RECEIVED_CRC, 0);
return -EINVAL;
diff --git a/drivers/infiniband/sw/siw/siw_qp_tx.c b/drivers/infiniband/sw/siw/siw_qp_tx.c
index a034264c56698..f9db69a82cdd5 100644
--- a/drivers/infiniband/sw/siw/siw_qp_tx.c
+++ b/drivers/infiniband/sw/siw/siw_qp_tx.c
@@ -242,11 +242,11 @@ static int siw_qp_prepare_tx(struct siw_iwarp_tx *c_tx)
else
c_tx->pkt.c_tagged.ddp_to =
cpu_to_be64(wqe->sqe.raddr);
}
- *(u32 *)crc = 0;
+ *(__le32 *)crc = 0;
/*
* Do complete CRC if enabled and short packet
*/
if (c_tx->mpa_crc_hd &&
crypto_shash_digest(c_tx->mpa_crc_hd, (u8 *)&c_tx->pkt,
--
2.48.1
^ permalink raw reply related [flat|nested] 28+ messages in thread* RE: [PATCH 5/6] RDMA/siw: fix type of CRC field
2025-01-27 22:38 ` [PATCH 5/6] RDMA/siw: fix type of CRC field Eric Biggers
@ 2025-01-31 12:24 ` Bernard Metzler
0 siblings, 0 replies; 28+ messages in thread
From: Bernard Metzler @ 2025-01-31 12:24 UTC (permalink / raw)
To: Eric Biggers, linux-rdma@vger.kernel.org, Mustafa Ismail,
Tatyana Nikolova, Jason Gunthorpe, Leon Romanovsky, Zhu Yanjun
Cc: linux-kernel@vger.kernel.org
> -----Original Message-----
> From: Eric Biggers <ebiggers@kernel.org>
> Sent: Monday, January 27, 2025 11:39 PM
> To: linux-rdma@vger.kernel.org; Mustafa Ismail <mustafa.ismail@intel.com>;
> Tatyana Nikolova <tatyana.e.nikolova@intel.com>; Jason Gunthorpe
> <jgg@ziepe.ca>; Leon Romanovsky <leon@kernel.org>; Zhu Yanjun
> <zyjzyj2000@gmail.com>; Bernard Metzler <BMT@zurich.ibm.com>
> Cc: linux-kernel@vger.kernel.org
> Subject: [EXTERNAL] [PATCH 5/6] RDMA/siw: fix type of CRC field
>
> From: Eric Biggers <ebiggers@google.com>
>
Many thanks for making the correct CRC32c handling explicit.
> The usual big endian convention of InfiniBand does not apply to the MPA
'InfiniBand' is misleading here. Transmitting protocol header fields
in big endian order is a network protocol convention in general, so
it is often called network byte order. iWarp is not an InfiniBand
protocol but an IETF protocol. So in IMO, the specifics of CRC32c
network transmission would be better highlighted when comparing to
NBO in general.
> CRC field, whose transmission is specified in terms of the CRC32
> polynomial coefficients. The coefficients are transmitted in descending
> order from the x^31 coefficient to the x^0 one. When the result is
> interpreted as a 32-bit integer using the required reverse mapping
> between bits and polynomial coefficients, it's a __le32.
>
> Fix the code to use the correct type.
>
> The endianness conversion to little endian was actually already done in
> crypto_shash_final(). Therefore this patch does not change any
> behavior, except for adding the missing le32_to_cpu() to the pr_warn()
> message in siw_get_trailer() which makes the correct values be printed
> on big endian systems.
>
> Signed-off-by: Eric Biggers <ebiggers@google.com>
> ---
> drivers/infiniband/sw/siw/iwarp.h | 2 +-
> drivers/infiniband/sw/siw/siw.h | 8 ++++----
> drivers/infiniband/sw/siw/siw_qp.c | 2 +-
> drivers/infiniband/sw/siw/siw_qp_rx.c | 16 +++++++++++-----
> drivers/infiniband/sw/siw/siw_qp_tx.c | 2 +-
> 5 files changed, 18 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/infiniband/sw/siw/iwarp.h
> b/drivers/infiniband/sw/siw/iwarp.h
> index 8cf69309827d6..afebb5d8643e3 100644
> --- a/drivers/infiniband/sw/siw/iwarp.h
> +++ b/drivers/infiniband/sw/siw/iwarp.h
> @@ -79,11 +79,11 @@ struct mpa_marker {
> /*
> * maximum MPA trailer
> */
> struct mpa_trailer {
> __u8 pad[4];
> - __be32 crc;
> + __le32 crc;
> };
>
> #define MPA_HDR_SIZE 2
> #define MPA_CRC_SIZE 4
>
> diff --git a/drivers/infiniband/sw/siw/siw.h
> b/drivers/infiniband/sw/siw/siw.h
> index ea5eee50dc39d..50649971f6254 100644
> --- a/drivers/infiniband/sw/siw/siw.h
> +++ b/drivers/infiniband/sw/siw/siw.h
> @@ -336,26 +336,26 @@ struct siw_rx_fpdu {
> * Shorthands for short packets w/o payload
> * to be transmitted more efficient.
> */
> struct siw_send_pkt {
> struct iwarp_send send;
> - __be32 crc;
> + __le32 crc;
> };
>
> struct siw_write_pkt {
> struct iwarp_rdma_write write;
> - __be32 crc;
> + __le32 crc;
> };
>
> struct siw_rreq_pkt {
> struct iwarp_rdma_rreq rreq;
> - __be32 crc;
> + __le32 crc;
> };
>
> struct siw_rresp_pkt {
> struct iwarp_rdma_rresp rresp;
> - __be32 crc;
> + __le32 crc;
> };
>
> struct siw_iwarp_tx {
> union {
> union iwarp_hdr hdr;
> diff --git a/drivers/infiniband/sw/siw/siw_qp.c
> b/drivers/infiniband/sw/siw/siw_qp.c
> index da92cfa2073d7..ea7d2f5c8b8ee 100644
> --- a/drivers/infiniband/sw/siw/siw_qp.c
> +++ b/drivers/infiniband/sw/siw/siw_qp.c
> @@ -394,11 +394,11 @@ void siw_send_terminate(struct siw_qp *qp)
> struct iwarp_terminate *term = NULL;
> union iwarp_hdr *err_hdr = NULL;
> struct socket *s = qp->attrs.sk;
> struct siw_rx_stream *srx = &qp->rx_stream;
> union iwarp_hdr *rx_hdr = &srx->hdr;
> - u32 crc = 0;
> + __le32 crc = 0;
> int num_frags, len_terminate, rv;
>
> if (!qp->term_info.valid)
> return;
>
> diff --git a/drivers/infiniband/sw/siw/siw_qp_rx.c
> b/drivers/infiniband/sw/siw/siw_qp_rx.c
> index ed4fc39718b49..098e32fb36fb1 100644
> --- a/drivers/infiniband/sw/siw/siw_qp_rx.c
> +++ b/drivers/infiniband/sw/siw/siw_qp_rx.c
> @@ -951,11 +951,11 @@ int siw_proc_terminate(struct siw_qp *qp)
> static int siw_get_trailer(struct siw_qp *qp, struct siw_rx_stream *srx)
> {
> struct sk_buff *skb = srx->skb;
> int avail = min(srx->skb_new, srx->fpdu_part_rem);
> u8 *tbuf = (u8 *)&srx->trailer.crc - srx->pad;
> - __wsum crc_in, crc_own = 0;
> + __le32 crc_in, crc_own = 0;
>
> siw_dbg_qp(qp, "expected %d, available %d, pad %u\n",
> srx->fpdu_part_rem, srx->skb_new, srx->pad);
>
> skb_copy_bits(skb, srx->skb_offset, tbuf, avail);
> @@ -969,20 +969,26 @@ static int siw_get_trailer(struct siw_qp *qp, struct
> siw_rx_stream *srx)
> if (!srx->mpa_crc_hd)
> return 0;
>
> if (srx->pad)
> crypto_shash_update(srx->mpa_crc_hd, tbuf, srx->pad);
> +
> /*
> - * CRC32 is computed, transmitted and received directly in NBO,
> - * so there's never a reason to convert byte order.
> + * The usual big endian convention of InfiniBand does not apply to
> the
Better not mention InfiniBand here, this is an IETF protocol on top
of TCP/SCTP. Just compare to NBO in general.
> + * CRC field, whose transmission is specified in terms of the CRC32
> + * polynomial coefficients. The coefficients are transmitted in
> + * descending order from the x^31 coefficient to the x^0 one. When
> the
> + * result is interpreted as a 32-bit integer using the required
> reverse
> + * mapping between bits and polynomial coefficients, it's a __le32.
> */
> crypto_shash_final(srx->mpa_crc_hd, (u8 *)&crc_own);
> - crc_in = (__force __wsum)srx->trailer.crc;
> + crc_in = srx->trailer.crc;
>
> if (unlikely(crc_in != crc_own)) {
> pr_warn("siw: crc error. in: %08x, own %08x, op %u\n",
> - crc_in, crc_own, qp->rx_stream.rdmap_op);
> + le32_to_cpu(crc_in), le32_to_cpu(crc_own),
> + qp->rx_stream.rdmap_op);
>
> siw_init_terminate(qp, TERM_ERROR_LAYER_LLP,
> LLP_ETYPE_MPA,
> LLP_ECODE_RECEIVED_CRC, 0);
> return -EINVAL;
> diff --git a/drivers/infiniband/sw/siw/siw_qp_tx.c
> b/drivers/infiniband/sw/siw/siw_qp_tx.c
> index a034264c56698..f9db69a82cdd5 100644
> --- a/drivers/infiniband/sw/siw/siw_qp_tx.c
> +++ b/drivers/infiniband/sw/siw/siw_qp_tx.c
> @@ -242,11 +242,11 @@ static int siw_qp_prepare_tx(struct siw_iwarp_tx
> *c_tx)
> else
> c_tx->pkt.c_tagged.ddp_to =
> cpu_to_be64(wqe->sqe.raddr);
> }
>
> - *(u32 *)crc = 0;
> + *(__le32 *)crc = 0;
> /*
> * Do complete CRC if enabled and short packet
> */
> if (c_tx->mpa_crc_hd &&
> crypto_shash_digest(c_tx->mpa_crc_hd, (u8 *)&c_tx->pkt,
> --
> 2.48.1
Reviewed-by: Bernard Metzler <bmt@zurich.ibm.com>
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 6/6] RDMA/siw: switch to using the crc32c library
2025-01-27 22:38 [PATCH 0/6] RDMA: switch to using CRC32 library functions Eric Biggers
` (4 preceding siblings ...)
2025-01-27 22:38 ` [PATCH 5/6] RDMA/siw: fix type of CRC field Eric Biggers
@ 2025-01-27 22:38 ` Eric Biggers
2025-01-31 14:17 ` Bernard Metzler
5 siblings, 1 reply; 28+ messages in thread
From: Eric Biggers @ 2025-01-27 22:38 UTC (permalink / raw)
To: linux-rdma, Mustafa Ismail, Tatyana Nikolova, Jason Gunthorpe,
Leon Romanovsky, Zhu Yanjun, Bernard Metzler
Cc: linux-kernel
From: Eric Biggers <ebiggers@google.com>
Now that the crc32c() library function directly takes advantage of
architecture-specific optimizations, it is unnecessary to go through the
crypto API. Just use crc32c(). This is much simpler, and it improves
performance due to eliminating the crypto API overhead.
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
drivers/infiniband/sw/siw/Kconfig | 4 +-
drivers/infiniband/sw/siw/siw.h | 38 +++++++++++++++----
drivers/infiniband/sw/siw/siw_main.c | 22 +----------
drivers/infiniband/sw/siw/siw_qp.c | 54 ++++++---------------------
drivers/infiniband/sw/siw/siw_qp_rx.c | 28 ++++++--------
drivers/infiniband/sw/siw/siw_qp_tx.c | 45 ++++++++++------------
drivers/infiniband/sw/siw/siw_verbs.c | 3 --
7 files changed, 75 insertions(+), 119 deletions(-)
diff --git a/drivers/infiniband/sw/siw/Kconfig b/drivers/infiniband/sw/siw/Kconfig
index 81b70a3eeb878..ae4a953e2a039 100644
--- a/drivers/infiniband/sw/siw/Kconfig
+++ b/drivers/infiniband/sw/siw/Kconfig
@@ -1,12 +1,10 @@
config RDMA_SIW
tristate "Software RDMA over TCP/IP (iWARP) driver"
depends on INET && INFINIBAND
depends on INFINIBAND_VIRT_DMA
- select LIBCRC32C
- select CRYPTO
- select CRYPTO_CRC32C
+ select CRC32
help
This driver implements the iWARP RDMA transport over
the Linux TCP/IP network stack. It enables a system with a
standard Ethernet adapter to interoperate with a iWARP
adapter or with another system running the SIW driver.
diff --git a/drivers/infiniband/sw/siw/siw.h b/drivers/infiniband/sw/siw/siw.h
index 50649971f6254..10c69388b4028 100644
--- a/drivers/infiniband/sw/siw/siw.h
+++ b/drivers/infiniband/sw/siw/siw.h
@@ -8,11 +8,10 @@
#include <rdma/ib_verbs.h>
#include <rdma/restrack.h>
#include <linux/socket.h>
#include <linux/skbuff.h>
-#include <crypto/hash.h>
#include <linux/crc32.h>
#include <linux/crc32c.h>
#include <rdma/siw-abi.h>
#include "iwarp.h"
@@ -287,11 +286,12 @@ struct siw_rx_stream {
enum siw_rx_state state;
union iwarp_hdr hdr;
struct mpa_trailer trailer;
- struct shash_desc *mpa_crc_hd;
+ u32 mpa_crc;
+ u8 mpa_crc_enabled : 1;
/*
* For each FPDU, main RX loop runs through 3 stages:
* Receiving protocol headers, placing DDP payload and receiving
* trailer information (CRC + possibly padding).
@@ -388,11 +388,12 @@ struct siw_iwarp_tx {
u16 ctrl_len; /* ddp+rdmap hdr */
u16 ctrl_sent;
int burst;
int bytes_unsent; /* ddp payload bytes */
- struct shash_desc *mpa_crc_hd;
+ u32 mpa_crc;
+ u8 mpa_crc_enabled : 1;
u8 do_crc : 1; /* do crc for segment */
u8 use_sendpage : 1; /* send w/o copy */
u8 tx_suspend : 1; /* stop sending DDP segs. */
u8 pad : 2; /* # pad in current fpdu */
@@ -494,11 +495,10 @@ extern const bool mpa_crc_strict;
extern const bool siw_tcp_nagle;
extern u_char mpa_version;
extern const bool peer_to_peer;
extern struct task_struct *siw_tx_thread[];
-extern struct crypto_shash *siw_crypto_shash;
extern struct iwarp_msg_info iwarp_pktinfo[RDMAP_TERMINATE + 1];
/* QP general functions */
int siw_qp_modify(struct siw_qp *qp, struct siw_qp_attrs *attr,
enum siw_qp_attr_mask mask);
@@ -666,10 +666,34 @@ static inline struct siw_sqe *irq_alloc_free(struct siw_qp *qp)
return irq_e;
}
return NULL;
}
+static inline void siw_crc_init(u32 *crc)
+{
+ *crc = ~0;
+}
+
+static inline void siw_crc_update(u32 *crc, const void *addr, unsigned int len)
+{
+ *crc = crc32c(*crc, addr, len);
+}
+
+static inline __le32 siw_crc_final(u32 *crc)
+{
+ return cpu_to_le32(~*crc);
+}
+
+static inline __le32 siw_crc_oneshot(const void *addr, unsigned int len)
+{
+ u32 crc;
+
+ siw_crc_init(&crc);
+ siw_crc_update(&crc, addr, len);
+ return siw_crc_final(&crc);
+}
+
static inline __wsum siw_csum_update(const void *buff, int len, __wsum sum)
{
return (__force __wsum)crc32c((__force __u32)sum, buff, len);
}
@@ -684,15 +708,13 @@ static inline void siw_crc_skb(struct siw_rx_stream *srx, unsigned int len)
{
const struct skb_checksum_ops siw_cs_ops = {
.update = siw_csum_update,
.combine = siw_csum_combine,
};
- __wsum crc = *(u32 *)shash_desc_ctx(srx->mpa_crc_hd);
- crc = __skb_checksum(srx->skb, srx->skb_offset, len, crc,
- &siw_cs_ops);
- *(u32 *)shash_desc_ctx(srx->mpa_crc_hd) = crc;
+ srx->mpa_crc = __skb_checksum(srx->skb, srx->skb_offset, len,
+ srx->mpa_crc, &siw_cs_ops);
}
#define siw_dbg(ibdev, fmt, ...) \
ibdev_dbg(ibdev, "%s: " fmt, __func__, ##__VA_ARGS__)
diff --git a/drivers/infiniband/sw/siw/siw_main.c b/drivers/infiniband/sw/siw/siw_main.c
index b17752bd1ecc1..5168307229a9e 100644
--- a/drivers/infiniband/sw/siw/siw_main.c
+++ b/drivers/infiniband/sw/siw/siw_main.c
@@ -57,11 +57,10 @@ u_char mpa_version = MPA_REVISION_2;
* setup, if true.
*/
const bool peer_to_peer;
struct task_struct *siw_tx_thread[NR_CPUS];
-struct crypto_shash *siw_crypto_shash;
static int siw_device_register(struct siw_device *sdev, const char *name)
{
struct ib_device *base_dev = &sdev->base_dev;
static int dev_id = 1;
@@ -465,24 +464,11 @@ static __init int siw_init_module(void)
if (!siw_create_tx_threads()) {
pr_info("siw: Could not start any TX thread\n");
rv = -ENOMEM;
goto out_error;
}
- /*
- * Locate CRC32 algorithm. If unsuccessful, fail
- * loading siw only, if CRC is required.
- */
- siw_crypto_shash = crypto_alloc_shash("crc32c", 0, 0);
- if (IS_ERR(siw_crypto_shash)) {
- pr_info("siw: Loading CRC32c failed: %ld\n",
- PTR_ERR(siw_crypto_shash));
- siw_crypto_shash = NULL;
- if (mpa_crc_required) {
- rv = -EOPNOTSUPP;
- goto out_error;
- }
- }
+
rv = register_netdevice_notifier(&siw_netdev_nb);
if (rv)
goto out_error;
rdma_link_register(&siw_link_ops);
@@ -491,13 +477,10 @@ static __init int siw_init_module(void)
return 0;
out_error:
siw_stop_tx_threads();
- if (siw_crypto_shash)
- crypto_free_shash(siw_crypto_shash);
-
pr_info("SoftIWARP attach failed. Error: %d\n", rv);
siw_cm_exit();
siw_destroy_cpulist(siw_cpu_info.num_nodes);
@@ -514,13 +497,10 @@ static void __exit siw_exit_module(void)
siw_cm_exit();
siw_destroy_cpulist(siw_cpu_info.num_nodes);
- if (siw_crypto_shash)
- crypto_free_shash(siw_crypto_shash);
-
pr_info("SoftiWARP detached\n");
}
module_init(siw_init_module);
module_exit(siw_exit_module);
diff --git a/drivers/infiniband/sw/siw/siw_qp.c b/drivers/infiniband/sw/siw/siw_qp.c
index ea7d2f5c8b8ee..75c8ca9559391 100644
--- a/drivers/infiniband/sw/siw/siw_qp.c
+++ b/drivers/infiniband/sw/siw/siw_qp.c
@@ -224,37 +224,10 @@ static int siw_qp_readq_init(struct siw_qp *qp, int irq_size, int orq_size)
qp->attrs.orq_size = orq_size;
siw_dbg_qp(qp, "ORD %d, IRD %d\n", orq_size, irq_size);
return 0;
}
-static int siw_qp_enable_crc(struct siw_qp *qp)
-{
- struct siw_rx_stream *c_rx = &qp->rx_stream;
- struct siw_iwarp_tx *c_tx = &qp->tx_ctx;
- int size;
-
- if (siw_crypto_shash == NULL)
- return -ENOENT;
-
- size = crypto_shash_descsize(siw_crypto_shash) +
- sizeof(struct shash_desc);
-
- c_tx->mpa_crc_hd = kzalloc(size, GFP_KERNEL);
- c_rx->mpa_crc_hd = kzalloc(size, GFP_KERNEL);
- if (!c_tx->mpa_crc_hd || !c_rx->mpa_crc_hd) {
- kfree(c_tx->mpa_crc_hd);
- kfree(c_rx->mpa_crc_hd);
- c_tx->mpa_crc_hd = NULL;
- c_rx->mpa_crc_hd = NULL;
- return -ENOMEM;
- }
- c_tx->mpa_crc_hd->tfm = siw_crypto_shash;
- c_rx->mpa_crc_hd->tfm = siw_crypto_shash;
-
- return 0;
-}
-
/*
* Send a non signalled READ or WRITE to peer side as negotiated
* with MPAv2 P2P setup protocol. The work request is only created
* as a current active WR and does not consume Send Queue space.
*
@@ -581,32 +554,26 @@ void siw_send_terminate(struct siw_qp *qp)
rx_hdr->ctrl.mpa_len = cpu_to_be16(real_ddp_len);
}
term->ctrl.mpa_len =
cpu_to_be16(len_terminate - (MPA_HDR_SIZE + MPA_CRC_SIZE));
- if (qp->tx_ctx.mpa_crc_hd) {
- crypto_shash_init(qp->tx_ctx.mpa_crc_hd);
- if (crypto_shash_update(qp->tx_ctx.mpa_crc_hd,
- (u8 *)iov[0].iov_base,
- iov[0].iov_len))
- goto out;
-
+ if (qp->tx_ctx.mpa_crc_enabled) {
+ siw_crc_init(&qp->tx_ctx.mpa_crc);
+ siw_crc_update(&qp->tx_ctx.mpa_crc,
+ iov[0].iov_base, iov[0].iov_len);
if (num_frags == 3) {
- if (crypto_shash_update(qp->tx_ctx.mpa_crc_hd,
- (u8 *)iov[1].iov_base,
- iov[1].iov_len))
- goto out;
+ siw_crc_update(&qp->tx_ctx.mpa_crc,
+ iov[1].iov_base, iov[1].iov_len);
}
- crypto_shash_final(qp->tx_ctx.mpa_crc_hd, (u8 *)&crc);
+ crc = siw_crc_final(&qp->tx_ctx.mpa_crc);
}
rv = kernel_sendmsg(s, &msg, iov, num_frags, len_terminate);
siw_dbg_qp(qp, "sent TERM: %s, layer %d, type %d, code %d (%d bytes)\n",
rv == len_terminate ? "success" : "failure",
__rdmap_term_layer(term), __rdmap_term_etype(term),
__rdmap_term_ecode(term), rv);
-out:
kfree(term);
kfree(err_hdr);
}
/*
@@ -641,13 +608,14 @@ static int siw_qp_nextstate_from_idle(struct siw_qp *qp,
int rv = 0;
switch (attrs->state) {
case SIW_QP_STATE_RTS:
if (attrs->flags & SIW_MPA_CRC) {
- rv = siw_qp_enable_crc(qp);
- if (rv)
- break;
+ siw_crc_init(&qp->tx_ctx.mpa_crc);
+ qp->tx_ctx.mpa_crc_enabled = 1;
+ siw_crc_init(&qp->rx_stream.mpa_crc);
+ qp->rx_stream.mpa_crc_enabled = 1;
}
if (!(mask & SIW_QP_ATTR_LLP_HANDLE)) {
siw_dbg_qp(qp, "no socket\n");
rv = -EINVAL;
break;
diff --git a/drivers/infiniband/sw/siw/siw_qp_rx.c b/drivers/infiniband/sw/siw/siw_qp_rx.c
index 098e32fb36fb1..2765bd0df2d0c 100644
--- a/drivers/infiniband/sw/siw/siw_qp_rx.c
+++ b/drivers/infiniband/sw/siw/siw_qp_rx.c
@@ -65,14 +65,14 @@ static int siw_rx_umem(struct siw_rx_stream *srx, struct siw_umem *umem,
pr_warn("siw: [QP %u]: %s, len %d, page %p, rv %d\n",
qp_id(rx_qp(srx)), __func__, len, p, rv);
return -EFAULT;
}
- if (srx->mpa_crc_hd) {
+ if (srx->mpa_crc_enabled) {
if (rdma_is_kernel_res(&rx_qp(srx)->base_qp.res)) {
- crypto_shash_update(srx->mpa_crc_hd,
- (u8 *)(dest + pg_off), bytes);
+ siw_crc_update(&srx->mpa_crc, dest + pg_off,
+ bytes);
kunmap_atomic(dest);
} else {
kunmap_atomic(dest);
/*
* Do CRC on original, not target buffer.
@@ -112,12 +112,12 @@ static int siw_rx_kva(struct siw_rx_stream *srx, void *kva, int len)
pr_warn("siw: [QP %u]: %s, len %d, kva 0x%pK, rv %d\n",
qp_id(rx_qp(srx)), __func__, len, kva, rv);
return rv;
}
- if (srx->mpa_crc_hd)
- crypto_shash_update(srx->mpa_crc_hd, (u8 *)kva, len);
+ if (srx->mpa_crc_enabled)
+ siw_crc_update(&srx->mpa_crc, kva, len);
srx->skb_offset += len;
srx->skb_copied += len;
srx->skb_new -= len;
@@ -964,25 +964,24 @@ static int siw_get_trailer(struct siw_qp *qp, struct siw_rx_stream *srx)
srx->fpdu_part_rem -= avail;
if (srx->fpdu_part_rem)
return -EAGAIN;
- if (!srx->mpa_crc_hd)
+ if (!srx->mpa_crc_enabled)
return 0;
if (srx->pad)
- crypto_shash_update(srx->mpa_crc_hd, tbuf, srx->pad);
-
+ siw_crc_update(&srx->mpa_crc, tbuf, srx->pad);
/*
* The usual big endian convention of InfiniBand does not apply to the
* CRC field, whose transmission is specified in terms of the CRC32
* polynomial coefficients. The coefficients are transmitted in
* descending order from the x^31 coefficient to the x^0 one. When the
* result is interpreted as a 32-bit integer using the required reverse
* mapping between bits and polynomial coefficients, it's a __le32.
*/
- crypto_shash_final(srx->mpa_crc_hd, (u8 *)&crc_own);
+ crc_own = siw_crc_final(&srx->mpa_crc);
crc_in = srx->trailer.crc;
if (unlikely(crc_in != crc_own)) {
pr_warn("siw: crc error. in: %08x, own %08x, op %u\n",
le32_to_cpu(crc_in), le32_to_cpu(crc_own),
@@ -1097,17 +1096,14 @@ static int siw_get_hdr(struct siw_rx_stream *srx)
* the current tagged or untagged message gets eventually completed
* w/o intersection from another message of the same type
* (tagged/untagged). E.g., a WRITE can get intersected by a SEND,
* but not by a READ RESPONSE etc.
*/
- if (srx->mpa_crc_hd) {
- /*
- * Restart CRC computation
- */
- crypto_shash_init(srx->mpa_crc_hd);
- crypto_shash_update(srx->mpa_crc_hd, (u8 *)c_hdr,
- srx->fpdu_part_rcvd);
+ if (srx->mpa_crc_enabled) {
+ /* Restart CRC computation */
+ siw_crc_init(&srx->mpa_crc);
+ siw_crc_update(&srx->mpa_crc, c_hdr, srx->fpdu_part_rcvd);
}
if (frx->more_ddp_segs) {
frx->first_ddp_seg = 0;
if (frx->prev_rdmap_op != opcode) {
pr_warn("siw: packet intersection: %u : %u\n",
diff --git a/drivers/infiniband/sw/siw/siw_qp_tx.c b/drivers/infiniband/sw/siw/siw_qp_tx.c
index f9db69a82cdd5..ff6951fa64bc3 100644
--- a/drivers/infiniband/sw/siw/siw_qp_tx.c
+++ b/drivers/infiniband/sw/siw/siw_qp_tx.c
@@ -246,14 +246,13 @@ static int siw_qp_prepare_tx(struct siw_iwarp_tx *c_tx)
*(__le32 *)crc = 0;
/*
* Do complete CRC if enabled and short packet
*/
- if (c_tx->mpa_crc_hd &&
- crypto_shash_digest(c_tx->mpa_crc_hd, (u8 *)&c_tx->pkt,
- c_tx->ctrl_len, (u8 *)crc) != 0)
- return -EINVAL;
+ if (c_tx->mpa_crc_enabled)
+ *(__le32 *)crc = siw_crc_oneshot(&c_tx->pkt,
+ c_tx->ctrl_len);
c_tx->ctrl_len += MPA_CRC_SIZE;
return PKT_COMPLETE;
}
c_tx->ctrl_len += MPA_CRC_SIZE;
@@ -480,13 +479,12 @@ static int siw_tx_hdt(struct siw_iwarp_tx *c_tx, struct socket *s)
iov[seg].iov_base =
ib_virt_dma_to_ptr(sge->laddr + sge_off);
iov[seg].iov_len = sge_len;
if (do_crc)
- crypto_shash_update(c_tx->mpa_crc_hd,
- iov[seg].iov_base,
- sge_len);
+ siw_crc_update(&c_tx->mpa_crc,
+ iov[seg].iov_base, sge_len);
sge_off += sge_len;
data_len -= sge_len;
seg++;
goto sge_done;
}
@@ -514,19 +512,18 @@ static int siw_tx_hdt(struct siw_iwarp_tx *c_tx, struct socket *s)
kmap_mask |= BIT(seg);
iov[seg].iov_base = kaddr + fp_off;
iov[seg].iov_len = plen;
if (do_crc)
- crypto_shash_update(
- c_tx->mpa_crc_hd,
+ siw_crc_update(
+ &c_tx->mpa_crc,
iov[seg].iov_base,
plen);
} else if (do_crc) {
kaddr = kmap_local_page(p);
- crypto_shash_update(c_tx->mpa_crc_hd,
- kaddr + fp_off,
- plen);
+ siw_crc_update(&c_tx->mpa_crc,
+ kaddr + fp_off, plen);
kunmap_local(kaddr);
}
} else {
/*
* Cast to an uintptr_t to preserve all 64 bits
@@ -534,14 +531,13 @@ static int siw_tx_hdt(struct siw_iwarp_tx *c_tx, struct socket *s)
*/
u64 va = sge->laddr + sge_off;
page_array[seg] = ib_virt_dma_to_page(va);
if (do_crc)
- crypto_shash_update(
- c_tx->mpa_crc_hd,
- ib_virt_dma_to_ptr(va),
- plen);
+ siw_crc_update(&c_tx->mpa_crc,
+ ib_virt_dma_to_ptr(va),
+ plen);
}
sge_len -= plen;
sge_off += plen;
data_len -= plen;
@@ -574,18 +570,18 @@ static int siw_tx_hdt(struct siw_iwarp_tx *c_tx, struct socket *s)
}
if (c_tx->pad) {
*(u32 *)c_tx->trailer.pad = 0;
if (do_crc)
- crypto_shash_update(c_tx->mpa_crc_hd,
- (u8 *)&c_tx->trailer.crc - c_tx->pad,
- c_tx->pad);
+ siw_crc_update(&c_tx->mpa_crc,
+ (u8 *)&c_tx->trailer.crc - c_tx->pad,
+ c_tx->pad);
}
- if (!c_tx->mpa_crc_hd)
+ if (!c_tx->mpa_crc_enabled)
c_tx->trailer.crc = 0;
else if (do_crc)
- crypto_shash_final(c_tx->mpa_crc_hd, (u8 *)&c_tx->trailer.crc);
+ c_tx->trailer.crc = siw_crc_final(&c_tx->mpa_crc);
data_len = c_tx->bytes_unsent;
if (c_tx->use_sendpage) {
rv = siw_0copy_tx(s, page_array, &wqe->sqe.sge[c_tx->sge_idx],
@@ -734,14 +730,13 @@ static void siw_prepare_fpdu(struct siw_qp *qp, struct siw_wqe *wqe)
htons(c_tx->ctrl_len + data_len - MPA_HDR_SIZE);
/*
* Init MPA CRC computation
*/
- if (c_tx->mpa_crc_hd) {
- crypto_shash_init(c_tx->mpa_crc_hd);
- crypto_shash_update(c_tx->mpa_crc_hd, (u8 *)&c_tx->pkt,
- c_tx->ctrl_len);
+ if (c_tx->mpa_crc_enabled) {
+ siw_crc_init(&c_tx->mpa_crc);
+ siw_crc_update(&c_tx->mpa_crc, &c_tx->pkt, c_tx->ctrl_len);
c_tx->do_crc = 1;
}
}
/*
diff --git a/drivers/infiniband/sw/siw/siw_verbs.c b/drivers/infiniband/sw/siw/siw_verbs.c
index 5ac8bd450d240..fd7b266a221b2 100644
--- a/drivers/infiniband/sw/siw/siw_verbs.c
+++ b/drivers/infiniband/sw/siw/siw_verbs.c
@@ -629,13 +629,10 @@ int siw_destroy_qp(struct ib_qp *base_qp, struct ib_udata *udata)
siw_cep_put(qp->cep);
qp->cep = NULL;
}
up_write(&qp->state_lock);
- kfree(qp->tx_ctx.mpa_crc_hd);
- kfree(qp->rx_stream.mpa_crc_hd);
-
qp->scq = qp->rcq = NULL;
siw_qp_put(qp);
wait_for_completion(&qp->qp_free);
--
2.48.1
^ permalink raw reply related [flat|nested] 28+ messages in thread* RE: [PATCH 6/6] RDMA/siw: switch to using the crc32c library
2025-01-27 22:38 ` [PATCH 6/6] RDMA/siw: switch to using the crc32c library Eric Biggers
@ 2025-01-31 14:17 ` Bernard Metzler
0 siblings, 0 replies; 28+ messages in thread
From: Bernard Metzler @ 2025-01-31 14:17 UTC (permalink / raw)
To: Eric Biggers, linux-rdma@vger.kernel.org, Mustafa Ismail,
Tatyana Nikolova, Jason Gunthorpe, Leon Romanovsky, Zhu Yanjun
Cc: linux-kernel@vger.kernel.org
> -----Original Message-----
> From: Eric Biggers <ebiggers@kernel.org>
> Sent: Monday, January 27, 2025 11:39 PM
> To: linux-rdma@vger.kernel.org; Mustafa Ismail <mustafa.ismail@intel.com>;
> Tatyana Nikolova <tatyana.e.nikolova@intel.com>; Jason Gunthorpe
> <jgg@ziepe.ca>; Leon Romanovsky <leon@kernel.org>; Zhu Yanjun
> <zyjzyj2000@gmail.com>; Bernard Metzler <BMT@zurich.ibm.com>
> Cc: linux-kernel@vger.kernel.org
> Subject: [EXTERNAL] [PATCH 6/6] RDMA/siw: switch to using the crc32c
> library
>
> From: Eric Biggers <ebiggers@google.com>
>
> Now that the crc32c() library function directly takes advantage of
> architecture-specific optimizations, it is unnecessary to go through the
> crypto API. Just use crc32c(). This is much simpler, and it improves
> performance due to eliminating the crypto API overhead.
>
Agreed, it looks cleaner and is probably also more
efficient. Thanks!
> Signed-off-by: Eric Biggers <ebiggers@google.com>
> ---
> drivers/infiniband/sw/siw/Kconfig | 4 +-
> drivers/infiniband/sw/siw/siw.h | 38 +++++++++++++++----
> drivers/infiniband/sw/siw/siw_main.c | 22 +----------
> drivers/infiniband/sw/siw/siw_qp.c | 54 ++++++---------------------
> drivers/infiniband/sw/siw/siw_qp_rx.c | 28 ++++++--------
> drivers/infiniband/sw/siw/siw_qp_tx.c | 45 ++++++++++------------
> drivers/infiniband/sw/siw/siw_verbs.c | 3 --
> 7 files changed, 75 insertions(+), 119 deletions(-)
>
> diff --git a/drivers/infiniband/sw/siw/Kconfig
> b/drivers/infiniband/sw/siw/Kconfig
> index 81b70a3eeb878..ae4a953e2a039 100644
> --- a/drivers/infiniband/sw/siw/Kconfig
> +++ b/drivers/infiniband/sw/siw/Kconfig
> @@ -1,12 +1,10 @@
> config RDMA_SIW
> tristate "Software RDMA over TCP/IP (iWARP) driver"
> depends on INET && INFINIBAND
> depends on INFINIBAND_VIRT_DMA
> - select LIBCRC32C
> - select CRYPTO
> - select CRYPTO_CRC32C
> + select CRC32
> help
> This driver implements the iWARP RDMA transport over
> the Linux TCP/IP network stack. It enables a system with a
> standard Ethernet adapter to interoperate with a iWARP
> adapter or with another system running the SIW driver.
> diff --git a/drivers/infiniband/sw/siw/siw.h
> b/drivers/infiniband/sw/siw/siw.h
> index 50649971f6254..10c69388b4028 100644
> --- a/drivers/infiniband/sw/siw/siw.h
> +++ b/drivers/infiniband/sw/siw/siw.h
> @@ -8,11 +8,10 @@
>
> #include <rdma/ib_verbs.h>
> #include <rdma/restrack.h>
> #include <linux/socket.h>
> #include <linux/skbuff.h>
> -#include <crypto/hash.h>
> #include <linux/crc32.h>
> #include <linux/crc32c.h>
>
> #include <rdma/siw-abi.h>
> #include "iwarp.h"
> @@ -287,11 +286,12 @@ struct siw_rx_stream {
>
> enum siw_rx_state state;
>
> union iwarp_hdr hdr;
> struct mpa_trailer trailer;
> - struct shash_desc *mpa_crc_hd;
> + u32 mpa_crc;
> + u8 mpa_crc_enabled : 1;
The bit field would unfortunately not save us
any memory footprint. Better make it a bool?
>
> /*
> * For each FPDU, main RX loop runs through 3 stages:
> * Receiving protocol headers, placing DDP payload and receiving
> * trailer information (CRC + possibly padding).
> @@ -388,11 +388,12 @@ struct siw_iwarp_tx {
> u16 ctrl_len; /* ddp+rdmap hdr */
> u16 ctrl_sent;
> int burst;
> int bytes_unsent; /* ddp payload bytes */
>
> - struct shash_desc *mpa_crc_hd;
> + u32 mpa_crc;
> + u8 mpa_crc_enabled : 1;
>
> u8 do_crc : 1; /* do crc for segment */
> u8 use_sendpage : 1; /* send w/o copy */
> u8 tx_suspend : 1; /* stop sending DDP segs. */
> u8 pad : 2; /* # pad in current fpdu */
> @@ -494,11 +495,10 @@ extern const bool mpa_crc_strict;
> extern const bool siw_tcp_nagle;
> extern u_char mpa_version;
> extern const bool peer_to_peer;
> extern struct task_struct *siw_tx_thread[];
>
> -extern struct crypto_shash *siw_crypto_shash;
> extern struct iwarp_msg_info iwarp_pktinfo[RDMAP_TERMINATE + 1];
>
> /* QP general functions */
> int siw_qp_modify(struct siw_qp *qp, struct siw_qp_attrs *attr,
> enum siw_qp_attr_mask mask);
> @@ -666,10 +666,34 @@ static inline struct siw_sqe *irq_alloc_free(struct
> siw_qp *qp)
> return irq_e;
> }
> return NULL;
> }
>
> +static inline void siw_crc_init(u32 *crc)
> +{
> + *crc = ~0;
> +}
> +
> +static inline void siw_crc_update(u32 *crc, const void *addr, unsigned int
> len)
> +{
> + *crc = crc32c(*crc, addr, len);
> +}
> +
> +static inline __le32 siw_crc_final(u32 *crc)
> +{
> + return cpu_to_le32(~*crc);
> +}
> +
> +static inline __le32 siw_crc_oneshot(const void *addr, unsigned int len)
> +{
> + u32 crc;
> +
> + siw_crc_init(&crc);
> + siw_crc_update(&crc, addr, len);
> + return siw_crc_final(&crc);
> +}
> +
> static inline __wsum siw_csum_update(const void *buff, int len, __wsum
> sum)
> {
> return (__force __wsum)crc32c((__force __u32)sum, buff, len);
> }
>
> @@ -684,15 +708,13 @@ static inline void siw_crc_skb(struct siw_rx_stream
> *srx, unsigned int len)
> {
> const struct skb_checksum_ops siw_cs_ops = {
> .update = siw_csum_update,
> .combine = siw_csum_combine,
> };
> - __wsum crc = *(u32 *)shash_desc_ctx(srx->mpa_crc_hd);
>
> - crc = __skb_checksum(srx->skb, srx->skb_offset, len, crc,
> - &siw_cs_ops);
> - *(u32 *)shash_desc_ctx(srx->mpa_crc_hd) = crc;
> + srx->mpa_crc = __skb_checksum(srx->skb, srx->skb_offset, len,
> + srx->mpa_crc, &siw_cs_ops);
> }
>
> #define siw_dbg(ibdev, fmt, ...)
> \
> ibdev_dbg(ibdev, "%s: " fmt, __func__, ##__VA_ARGS__)
>
> diff --git a/drivers/infiniband/sw/siw/siw_main.c
> b/drivers/infiniband/sw/siw/siw_main.c
> index b17752bd1ecc1..5168307229a9e 100644
> --- a/drivers/infiniband/sw/siw/siw_main.c
> +++ b/drivers/infiniband/sw/siw/siw_main.c
> @@ -57,11 +57,10 @@ u_char mpa_version = MPA_REVISION_2;
> * setup, if true.
> */
> const bool peer_to_peer;
>
> struct task_struct *siw_tx_thread[NR_CPUS];
> -struct crypto_shash *siw_crypto_shash;
>
> static int siw_device_register(struct siw_device *sdev, const char *name)
> {
> struct ib_device *base_dev = &sdev->base_dev;
> static int dev_id = 1;
> @@ -465,24 +464,11 @@ static __init int siw_init_module(void)
> if (!siw_create_tx_threads()) {
> pr_info("siw: Could not start any TX thread\n");
> rv = -ENOMEM;
> goto out_error;
> }
> - /*
> - * Locate CRC32 algorithm. If unsuccessful, fail
> - * loading siw only, if CRC is required.
> - */
> - siw_crypto_shash = crypto_alloc_shash("crc32c", 0, 0);
> - if (IS_ERR(siw_crypto_shash)) {
> - pr_info("siw: Loading CRC32c failed: %ld\n",
> - PTR_ERR(siw_crypto_shash));
> - siw_crypto_shash = NULL;
> - if (mpa_crc_required) {
> - rv = -EOPNOTSUPP;
> - goto out_error;
> - }
> - }
> +
> rv = register_netdevice_notifier(&siw_netdev_nb);
> if (rv)
> goto out_error;
>
> rdma_link_register(&siw_link_ops);
> @@ -491,13 +477,10 @@ static __init int siw_init_module(void)
> return 0;
>
> out_error:
> siw_stop_tx_threads();
>
> - if (siw_crypto_shash)
> - crypto_free_shash(siw_crypto_shash);
> -
> pr_info("SoftIWARP attach failed. Error: %d\n", rv);
>
> siw_cm_exit();
> siw_destroy_cpulist(siw_cpu_info.num_nodes);
>
> @@ -514,13 +497,10 @@ static void __exit siw_exit_module(void)
>
> siw_cm_exit();
>
> siw_destroy_cpulist(siw_cpu_info.num_nodes);
>
> - if (siw_crypto_shash)
> - crypto_free_shash(siw_crypto_shash);
> -
> pr_info("SoftiWARP detached\n");
> }
>
> module_init(siw_init_module);
> module_exit(siw_exit_module);
> diff --git a/drivers/infiniband/sw/siw/siw_qp.c
> b/drivers/infiniband/sw/siw/siw_qp.c
> index ea7d2f5c8b8ee..75c8ca9559391 100644
> --- a/drivers/infiniband/sw/siw/siw_qp.c
> +++ b/drivers/infiniband/sw/siw/siw_qp.c
> @@ -224,37 +224,10 @@ static int siw_qp_readq_init(struct siw_qp *qp, int
> irq_size, int orq_size)
> qp->attrs.orq_size = orq_size;
> siw_dbg_qp(qp, "ORD %d, IRD %d\n", orq_size, irq_size);
> return 0;
> }
>
> -static int siw_qp_enable_crc(struct siw_qp *qp)
> -{
> - struct siw_rx_stream *c_rx = &qp->rx_stream;
> - struct siw_iwarp_tx *c_tx = &qp->tx_ctx;
> - int size;
> -
> - if (siw_crypto_shash == NULL)
> - return -ENOENT;
> -
> - size = crypto_shash_descsize(siw_crypto_shash) +
> - sizeof(struct shash_desc);
> -
> - c_tx->mpa_crc_hd = kzalloc(size, GFP_KERNEL);
> - c_rx->mpa_crc_hd = kzalloc(size, GFP_KERNEL);
> - if (!c_tx->mpa_crc_hd || !c_rx->mpa_crc_hd) {
> - kfree(c_tx->mpa_crc_hd);
> - kfree(c_rx->mpa_crc_hd);
> - c_tx->mpa_crc_hd = NULL;
> - c_rx->mpa_crc_hd = NULL;
> - return -ENOMEM;
> - }
> - c_tx->mpa_crc_hd->tfm = siw_crypto_shash;
> - c_rx->mpa_crc_hd->tfm = siw_crypto_shash;
> -
> - return 0;
> -}
> -
> /*
> * Send a non signalled READ or WRITE to peer side as negotiated
> * with MPAv2 P2P setup protocol. The work request is only created
> * as a current active WR and does not consume Send Queue space.
> *
> @@ -581,32 +554,26 @@ void siw_send_terminate(struct siw_qp *qp)
> rx_hdr->ctrl.mpa_len = cpu_to_be16(real_ddp_len);
> }
>
> term->ctrl.mpa_len =
> cpu_to_be16(len_terminate - (MPA_HDR_SIZE + MPA_CRC_SIZE));
> - if (qp->tx_ctx.mpa_crc_hd) {
> - crypto_shash_init(qp->tx_ctx.mpa_crc_hd);
> - if (crypto_shash_update(qp->tx_ctx.mpa_crc_hd,
> - (u8 *)iov[0].iov_base,
> - iov[0].iov_len))
> - goto out;
> -
> + if (qp->tx_ctx.mpa_crc_enabled) {
> + siw_crc_init(&qp->tx_ctx.mpa_crc);
> + siw_crc_update(&qp->tx_ctx.mpa_crc,
> + iov[0].iov_base, iov[0].iov_len);
> if (num_frags == 3) {
> - if (crypto_shash_update(qp->tx_ctx.mpa_crc_hd,
> - (u8 *)iov[1].iov_base,
> - iov[1].iov_len))
> - goto out;
> + siw_crc_update(&qp->tx_ctx.mpa_crc,
> + iov[1].iov_base, iov[1].iov_len);
> }
> - crypto_shash_final(qp->tx_ctx.mpa_crc_hd, (u8 *)&crc);
> + crc = siw_crc_final(&qp->tx_ctx.mpa_crc);
> }
>
> rv = kernel_sendmsg(s, &msg, iov, num_frags, len_terminate);
> siw_dbg_qp(qp, "sent TERM: %s, layer %d, type %d, code %d (%d
> bytes)\n",
> rv == len_terminate ? "success" : "failure",
> __rdmap_term_layer(term), __rdmap_term_etype(term),
> __rdmap_term_ecode(term), rv);
> -out:
> kfree(term);
> kfree(err_hdr);
> }
>
> /*
> @@ -641,13 +608,14 @@ static int siw_qp_nextstate_from_idle(struct siw_qp
> *qp,
> int rv = 0;
>
> switch (attrs->state) {
> case SIW_QP_STATE_RTS:
> if (attrs->flags & SIW_MPA_CRC) {
> - rv = siw_qp_enable_crc(qp);
> - if (rv)
> - break;
> + siw_crc_init(&qp->tx_ctx.mpa_crc);
> + qp->tx_ctx.mpa_crc_enabled = 1;
> + siw_crc_init(&qp->rx_stream.mpa_crc);
> + qp->rx_stream.mpa_crc_enabled = 1;
> }
> if (!(mask & SIW_QP_ATTR_LLP_HANDLE)) {
> siw_dbg_qp(qp, "no socket\n");
> rv = -EINVAL;
> break;
> diff --git a/drivers/infiniband/sw/siw/siw_qp_rx.c
> b/drivers/infiniband/sw/siw/siw_qp_rx.c
> index 098e32fb36fb1..2765bd0df2d0c 100644
> --- a/drivers/infiniband/sw/siw/siw_qp_rx.c
> +++ b/drivers/infiniband/sw/siw/siw_qp_rx.c
> @@ -65,14 +65,14 @@ static int siw_rx_umem(struct siw_rx_stream *srx,
> struct siw_umem *umem,
> pr_warn("siw: [QP %u]: %s, len %d, page %p, rv %d\n",
> qp_id(rx_qp(srx)), __func__, len, p, rv);
>
> return -EFAULT;
> }
> - if (srx->mpa_crc_hd) {
> + if (srx->mpa_crc_enabled) {
> if (rdma_is_kernel_res(&rx_qp(srx)->base_qp.res)) {
> - crypto_shash_update(srx->mpa_crc_hd,
> - (u8 *)(dest + pg_off), bytes);
> + siw_crc_update(&srx->mpa_crc, dest + pg_off,
> + bytes);
> kunmap_atomic(dest);
> } else {
> kunmap_atomic(dest);
> /*
> * Do CRC on original, not target buffer.
> @@ -112,12 +112,12 @@ static int siw_rx_kva(struct siw_rx_stream *srx, void
> *kva, int len)
> pr_warn("siw: [QP %u]: %s, len %d, kva 0x%pK, rv %d\n",
> qp_id(rx_qp(srx)), __func__, len, kva, rv);
>
> return rv;
> }
> - if (srx->mpa_crc_hd)
> - crypto_shash_update(srx->mpa_crc_hd, (u8 *)kva, len);
> + if (srx->mpa_crc_enabled)
> + siw_crc_update(&srx->mpa_crc, kva, len);
>
> srx->skb_offset += len;
> srx->skb_copied += len;
> srx->skb_new -= len;
>
> @@ -964,25 +964,24 @@ static int siw_get_trailer(struct siw_qp *qp, struct
> siw_rx_stream *srx)
> srx->fpdu_part_rem -= avail;
>
> if (srx->fpdu_part_rem)
> return -EAGAIN;
>
> - if (!srx->mpa_crc_hd)
> + if (!srx->mpa_crc_enabled)
> return 0;
>
> if (srx->pad)
> - crypto_shash_update(srx->mpa_crc_hd, tbuf, srx->pad);
> -
> + siw_crc_update(&srx->mpa_crc, tbuf, srx->pad);
> /*
> * The usual big endian convention of InfiniBand does not apply to
> the
> * CRC field, whose transmission is specified in terms of the CRC32
> * polynomial coefficients. The coefficients are transmitted in
> * descending order from the x^31 coefficient to the x^0 one. When
> the
> * result is interpreted as a 32-bit integer using the required
> reverse
> * mapping between bits and polynomial coefficients, it's a __le32.
> */
> - crypto_shash_final(srx->mpa_crc_hd, (u8 *)&crc_own);
> + crc_own = siw_crc_final(&srx->mpa_crc);
> crc_in = srx->trailer.crc;
>
> if (unlikely(crc_in != crc_own)) {
> pr_warn("siw: crc error. in: %08x, own %08x, op %u\n",
> le32_to_cpu(crc_in), le32_to_cpu(crc_own),
> @@ -1097,17 +1096,14 @@ static int siw_get_hdr(struct siw_rx_stream *srx)
> * the current tagged or untagged message gets eventually completed
> * w/o intersection from another message of the same type
> * (tagged/untagged). E.g., a WRITE can get intersected by a SEND,
> * but not by a READ RESPONSE etc.
> */
> - if (srx->mpa_crc_hd) {
> - /*
> - * Restart CRC computation
> - */
> - crypto_shash_init(srx->mpa_crc_hd);
> - crypto_shash_update(srx->mpa_crc_hd, (u8 *)c_hdr,
> - srx->fpdu_part_rcvd);
> + if (srx->mpa_crc_enabled) {
> + /* Restart CRC computation */
> + siw_crc_init(&srx->mpa_crc);
> + siw_crc_update(&srx->mpa_crc, c_hdr, srx->fpdu_part_rcvd);
> }
> if (frx->more_ddp_segs) {
> frx->first_ddp_seg = 0;
> if (frx->prev_rdmap_op != opcode) {
> pr_warn("siw: packet intersection: %u : %u\n",
> diff --git a/drivers/infiniband/sw/siw/siw_qp_tx.c
> b/drivers/infiniband/sw/siw/siw_qp_tx.c
> index f9db69a82cdd5..ff6951fa64bc3 100644
> --- a/drivers/infiniband/sw/siw/siw_qp_tx.c
> +++ b/drivers/infiniband/sw/siw/siw_qp_tx.c
> @@ -246,14 +246,13 @@ static int siw_qp_prepare_tx(struct siw_iwarp_tx
> *c_tx)
>
> *(__le32 *)crc = 0;
> /*
> * Do complete CRC if enabled and short packet
> */
> - if (c_tx->mpa_crc_hd &&
> - crypto_shash_digest(c_tx->mpa_crc_hd, (u8 *)&c_tx->pkt,
> - c_tx->ctrl_len, (u8 *)crc) != 0)
> - return -EINVAL;
> + if (c_tx->mpa_crc_enabled)
> + *(__le32 *)crc = siw_crc_oneshot(&c_tx->pkt,
> + c_tx->ctrl_len);
> c_tx->ctrl_len += MPA_CRC_SIZE;
>
> return PKT_COMPLETE;
> }
> c_tx->ctrl_len += MPA_CRC_SIZE;
> @@ -480,13 +479,12 @@ static int siw_tx_hdt(struct siw_iwarp_tx *c_tx,
> struct socket *s)
> iov[seg].iov_base =
> ib_virt_dma_to_ptr(sge->laddr + sge_off);
> iov[seg].iov_len = sge_len;
>
> if (do_crc)
> - crypto_shash_update(c_tx->mpa_crc_hd,
> - iov[seg].iov_base,
> - sge_len);
> + siw_crc_update(&c_tx->mpa_crc,
> + iov[seg].iov_base, sge_len);
> sge_off += sge_len;
> data_len -= sge_len;
> seg++;
> goto sge_done;
> }
> @@ -514,19 +512,18 @@ static int siw_tx_hdt(struct siw_iwarp_tx *c_tx,
> struct socket *s)
> kmap_mask |= BIT(seg);
> iov[seg].iov_base = kaddr + fp_off;
> iov[seg].iov_len = plen;
>
> if (do_crc)
> - crypto_shash_update(
> - c_tx->mpa_crc_hd,
> + siw_crc_update(
> + &c_tx->mpa_crc,
> iov[seg].iov_base,
> plen);
> } else if (do_crc) {
> kaddr = kmap_local_page(p);
> - crypto_shash_update(c_tx->mpa_crc_hd,
> - kaddr + fp_off,
> - plen);
> + siw_crc_update(&c_tx->mpa_crc,
> + kaddr + fp_off, plen);
> kunmap_local(kaddr);
> }
> } else {
> /*
> * Cast to an uintptr_t to preserve all 64 bits
> @@ -534,14 +531,13 @@ static int siw_tx_hdt(struct siw_iwarp_tx *c_tx,
> struct socket *s)
> */
> u64 va = sge->laddr + sge_off;
>
> page_array[seg] = ib_virt_dma_to_page(va);
> if (do_crc)
> - crypto_shash_update(
> - c_tx->mpa_crc_hd,
> - ib_virt_dma_to_ptr(va),
> - plen);
> + siw_crc_update(&c_tx->mpa_crc,
> + ib_virt_dma_to_ptr(va),
> + plen);
> }
>
> sge_len -= plen;
> sge_off += plen;
> data_len -= plen;
> @@ -574,18 +570,18 @@ static int siw_tx_hdt(struct siw_iwarp_tx *c_tx,
> struct socket *s)
> }
>
> if (c_tx->pad) {
> *(u32 *)c_tx->trailer.pad = 0;
> if (do_crc)
> - crypto_shash_update(c_tx->mpa_crc_hd,
> - (u8 *)&c_tx->trailer.crc - c_tx->pad,
> - c_tx->pad);
> + siw_crc_update(&c_tx->mpa_crc,
> + (u8 *)&c_tx->trailer.crc - c_tx->pad,
> + c_tx->pad);
> }
> - if (!c_tx->mpa_crc_hd)
> + if (!c_tx->mpa_crc_enabled)
> c_tx->trailer.crc = 0;
> else if (do_crc)
> - crypto_shash_final(c_tx->mpa_crc_hd, (u8 *)&c_tx->trailer.crc);
> + c_tx->trailer.crc = siw_crc_final(&c_tx->mpa_crc);
>
> data_len = c_tx->bytes_unsent;
>
> if (c_tx->use_sendpage) {
> rv = siw_0copy_tx(s, page_array, &wqe->sqe.sge[c_tx->sge_idx],
> @@ -734,14 +730,13 @@ static void siw_prepare_fpdu(struct siw_qp *qp,
> struct siw_wqe *wqe)
> htons(c_tx->ctrl_len + data_len - MPA_HDR_SIZE);
>
> /*
> * Init MPA CRC computation
> */
> - if (c_tx->mpa_crc_hd) {
> - crypto_shash_init(c_tx->mpa_crc_hd);
> - crypto_shash_update(c_tx->mpa_crc_hd, (u8 *)&c_tx->pkt,
> - c_tx->ctrl_len);
> + if (c_tx->mpa_crc_enabled) {
> + siw_crc_init(&c_tx->mpa_crc);
> + siw_crc_update(&c_tx->mpa_crc, &c_tx->pkt, c_tx->ctrl_len);
> c_tx->do_crc = 1;
> }
> }
>
> /*
> diff --git a/drivers/infiniband/sw/siw/siw_verbs.c
> b/drivers/infiniband/sw/siw/siw_verbs.c
> index 5ac8bd450d240..fd7b266a221b2 100644
> --- a/drivers/infiniband/sw/siw/siw_verbs.c
> +++ b/drivers/infiniband/sw/siw/siw_verbs.c
> @@ -629,13 +629,10 @@ int siw_destroy_qp(struct ib_qp *base_qp, struct
> ib_udata *udata)
> siw_cep_put(qp->cep);
> qp->cep = NULL;
> }
> up_write(&qp->state_lock);
>
> - kfree(qp->tx_ctx.mpa_crc_hd);
> - kfree(qp->rx_stream.mpa_crc_hd);
> -
> qp->scq = qp->rcq = NULL;
>
> siw_qp_put(qp);
> wait_for_completion(&qp->qp_free);
>
> --
> 2.48.1
Reviewed-by: Bernard Metzler <bmt@zurich.ibm.com>
^ permalink raw reply [flat|nested] 28+ messages in thread