RE: [PATCH v2 4/9] RDMA/rxe: Move ICRC generation to a subroutine

From: "Pearson, Robert B" <robert.pearson2@hpe.com>
To: Jason Gunthorpe <jgg@nvidia.com>, Bob Pearson <rpearsonhpe@gmail.com>
Cc: "zyjzyj2000@gmail.com" <zyjzyj2000@gmail.com>,
	"linux-rdma@vger.kernel.org" <linux-rdma@vger.kernel.org>
Subject: RE: [PATCH v2 4/9] RDMA/rxe: Move ICRC generation to a subroutine
Date: Fri, 16 Jul 2021 16:38:28 +0000	[thread overview]
Message-ID: <CS1PR8401MB1096ECD020D2D7667F463D51BC119@CS1PR8401MB1096.NAMPRD84.PROD.OUTLOOK.COM> (raw)
In-Reply-To: <20210716162904.GL543781@nvidia.com>

I know. If you look at <Linux>/lib/crc32.c, I'm the current author, but it is now replaced by the crypto engines.
It was a nightmare if I recall. -- Bob

-----Original Message-----
From: Jason Gunthorpe <jgg@nvidia.com> 
Sent: Friday, July 16, 2021 11:29 AM
To: Bob Pearson <rpearsonhpe@gmail.com>
Cc: zyjzyj2000@gmail.com; linux-rdma@vger.kernel.org
Subject: Re: [PATCH v2 4/9] RDMA/rxe: Move ICRC generation to a subroutine

On Fri, Jul 16, 2021 at 11:08:42AM -0500, Bob Pearson wrote:
> On 7/16/21 10:57 AM, Jason Gunthorpe wrote:
> > On Tue, Jul 06, 2021 at 11:00:36PM -0500, Bob Pearson wrote:
> > 
> >> +/* rxe_icrc_generate- compute ICRC for a packet. */ void 
> >> +rxe_icrc_generate(struct sk_buff *skb, struct rxe_pkt_info *pkt) {
> >> +	__be32 *icrcp;
> >> +	u32 icrc;
> >> +
> >> +	icrcp = (__be32 *)(pkt->hdr + pkt->paylen - RXE_ICRC_SIZE);
> >> +	icrc = rxe_icrc_hdr(pkt, skb);
> >> +	icrc = rxe_crc32(pkt->rxe, icrc, (u8 *)payload_addr(pkt),
> >> +				payload_size(pkt) + bth_pad(pkt));
> >> +	*icrcp = (__force __be32)~icrc;
> >> +}
> > 
> > Same comment here, the u32 icrc should be a  __be32 because that is 
> > what rxe_crc32 returns, no force
> > 
> > Jason
> > 
> 
> I agree. The last patch in the series tries to make sense of the byte order.
> Here I was trying to take baby steps and just move the code without changing anything.
> It makes the thing easier for Zhu to review because no logic changed just where the code is.
> However as you point out it doesn't really make sense on the face of 
> it. There isn't any really good resolution because both the hardware 
> and software versions of the crc32 calculation are clearly labeled __le but they are stuffed into the ICRC which is clearly identified as __be.
> The problem is that it works i.e. interoperates with ConnectX. I would 
> love a conversation with one of the IBA architects to resolve this.

CRC's are complicated. There are 2 ways to feed the bits into the LSFR (left or right) and at least 4 ways to represent the output.

Depending on how you design the LSFR and the algorithm you inherently get one of the combinations.

Since rxe is using crc_le, and works, it is somehow setup that the input bits are in the right order but the output is reversed. So

  cpu_to_be32(byteswap(crc_le()))

Looks like the right equation.

On LE two byteswaps cancel and you an get away with naked casting. On BE it looks like a swap will be missing?

SHASH adds an additional cpu_to_le32() hidden inside the crypto code. That would make the expected sequence this:

  cpu_to_be32(byteswap(le32_to_cpu(cpu_to_le32(crc_le32())))

Now things look better. It is confusing because the bytes output by the SHASH are called "LE", and for some versions of the crc32 they may be, however for IBTA this memory is in what we'd call BE layout. So just casting the memory image above to BE is always fine.

The above will generate 0 swaps on LE and 1 swap on BE, vs no swaps on BE for the naked crc32_le() call.

Most likely this confusion is a defect in the design of the CRC that is being papered over by swaps.

You'd have to get out a qemu running a be PPC/ARM to check it out properly, but looks to me like the shash is working, the naked crc32_le is missing a swap, and loading the initial non-zero/non-FF constants is missing a swap.

Jason