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

[Eric Biggers <ebiggers@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