Message-ID: <20250130012951.GF2120662@ziepe.ca>
Date: Wed, 29 Jan 2025 21:29:51 -0400
From: Jason Gunthorpe <jgg@...pe.ca>
To: Eric Biggers <ebiggers@...nel.org>
Cc: Zhu Yanjun <yanjun.zhu@...ux.dev>, linux-rdma@...r.kernel.org,
	Mustafa Ismail <mustafa.ismail@...el.com>,
	Tatyana Nikolova <tatyana.e.nikolova@...el.com>,
	Leon Romanovsky <leon@...nel.org>,
	Zhu Yanjun <zyjzyj2000@...il.com>,
	Bernard Metzler <bmt@...ich.ibm.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/6] RDMA/rxe: handle ICRC correctly on big endian systems

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.

The above text is reflected in Figure 57 which shows the Remainder
being swapped all around to produce the ICRC.

The spec goes on to say:

 CRC 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.

>From a spec perspective is *total nonsense* to describe something the
spec explicitly says is big endian as little endian.

Yes from a maths perspective coefficients are reversed and whatever,
but that doesn't matter to someone reading the code. Clearly state
how to calculate the u32 "ICRC Field" as called out in the spec using
Linux. That is swab32(~crc32_le(..)) - that detail clarifies everything.

> It sounds like you want to add two unnecessary byteswaps to match
> the example in Table 25, which misleadingly shows a byte-swapped
> ICRC value as a u32 without mentioning it is byte-swapped.

There are an obnoxious number of ways to make, label and describe
these LFSRs. IBTA choose their representation, Linux choose a
different one.

It isn't misleadingly byte-swapped, it is self consistent with the
rest of the spec, and different from Linux.

> I don't agree that would be clearer, but we can do it if you prefer.

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 gave an example comment above.

Jason

Powered by blists - more mailing lists