Message-ID: <d0d05601-0eee-4684-9ed0-d6da8938603b@linux.dev>
Date: Wed, 29 Jan 2025 19:27:20 +0100
From: Zhu Yanjun <yanjun.zhu@...ux.dev>
To: Eric Biggers <ebiggers@...nel.org>, linux-rdma@...r.kernel.org,
 Mustafa Ismail <mustafa.ismail@...el.com>,
 Tatyana Nikolova <tatyana.e.nikolova@...el.com>,
 Jason Gunthorpe <jgg@...pe.ca>, Leon Romanovsky <leon@...nel.org>,
 Zhu Yanjun <zyjzyj2000@...il.com>, Bernard Metzler <bmt@...ich.ibm.com>
Cc: linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/6] RDMA/rxe: handle ICRC correctly on big endian systems

在 2025/1/27 23:38, Eric Biggers 写道:
> From: Eric Biggers <ebiggers@...gle.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@...gle.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);
>   }

Powered by blists - more mailing lists