Re: [PATCH] NTB: ntb_perf: fix cast to restricted __le32

[Serge Semin]

On Tue, Jan 23, 2018 at 11:18:06PM -0500, Jon Mason <jdmason@xxxxxxxx> wrote:
> On Fri, Jan 19, 2018 at 10:26:37PM +0100, Arnd Bergmann wrote:
> > On Fri, Jan 19, 2018 at 10:03 PM, Serge Semin <fancer.lancer@xxxxxxxxx> wrote:
> > >
> > > Actually the provided patch is the best solution I could come up with.
> > > The thing is, that the methods can't be changed. Those functions are
> > > the part of the NTB API methods used by many drivers. So basically they
> > > are like pci_{read,write}_config_{byte,word,dword}() methods. We can't
> > > change their prototypes only because it's suit some driver. The methods
> > > give an access to the NTB device dummy u32-sized registers, nothing
> > > else. So endianness is the transmitted data settings in this case.
> > >
> > > NTB is the technology to interconnect some two systems with possibly
> > > different endianness (unlike PCI, which interconnect CPU with LE devices).
> > > In this case I'd need to set some agreement up between two systems about
> > > the endianness of the exchanged data like host and network types in
> > > Linux networking. I've chosen the network data to be little-endian,
> > > that's why I needed first to convert them from CPU to le32, then on
> > > remote side convert them back from le32 to CPU.
> > >
> > > If you have any better suggestion how the warning can be fixed, I'd
> > > be glad to stick to it.
> > 
> > I don't think your description matches what you actually do: The
> > underlying ntb hardware drivers (amd, idt, intel, mscc) all treat the
> > incoming data as CPU-endian and convert it to little-endian on
> > the register side, so the framework already assumes that whatever
> > you do here uses a little-endian wire-level protocol.
> > 
> > On a little-endian kernel/CPU, nothing is ever swapped here, neither
> > in the ntb_perf front-end nor in the back-ends. On a big-endian
> > kernel/CPU, they both swap, so you end up with CPU-endian
> > data on the wire, so it should be impossible for a big-endian
> > system to talk to a little-endian one. Have you actually tried that
> > combination with the current code?
> 
> I do not believe anyone has every tried NTB on a big endien system,
> let alone tried it with one side LE and the other BE.  To my
> knowledge, this has only ever been used on x86 to x86.
> 
> > If my interpretation is correct, then the best solution would be to
> > completely remove the cpu_to_le32/le32_to_cpu conversions
> > from ntb_perf, and just define that it works like any other PCI
> > device, exchanging little-endian data.
> 
> Yes, this would be the best solution.  Thank you for the insight.
> 
> Serge, when you get a chance, please make this change and resumbit.
> 

Ok. I'll do it in an hour.

Regards,
-Sergey

> Thanks,
> Jon
> 
> 
> > 
> > There are two interesting cases to consider though:
> > 
> > - if someone wants to implement an NTB based protocol
> >   using big-endian data on the wire, you probably want to add
> >   a ntb_peer_spad_read_be()/ntb_peer_msg_write_be()
> >   set of interfaces, to go along with ioread32_be()/iowrite32_be()
> >   the same way that ntb_peer_spad_read()/ntb_peer_msg_write()
> >   ends up doing ioread32()/iowrite32() with the implied little-endian
> >   behavior.
> > 
> > - memcpy_toio()/memcpy_fromio() and ioread32_rep()/iowrite32_rep
> >   importantly do not do any byteswap, they are meant to
> >   transfer byte streams.
> > 
> >         Arnd