Re: [PATCH 1/6] RDMA/rxe: handle ICRC correctly on big endian systems

[Jason Gunthorpe <jgg@ziepe.ca>]

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