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

From: Jason Gunthorpe <jgg@nvidia.com>
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
Date: Fri, 16 Jul 2021 13:29:04 -0300	[thread overview]
Message-ID: <20210716162904.GL543781@nvidia.com> (raw)
In-Reply-To: <6a07ad6e-5167-9d71-e22b-94efb9b64401@gmail.com>

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