Re: [PATCH 4/6] arm64/io: Provide a WC friendly __iowriteXX_copy()

On Fri, 2024-02-23 at 08:58 -0400, Jason Gunthorpe wrote:

On Fri, Feb 23, 2024 at 12:38:18PM +0100, Niklas Schnelle wrote:

quoted

quoted

Although I doubt that generating long TLP from byte writes is
really necessary.

I might have gotten confused but I think these are not byte writes.
Remember that the count is in terms of the number of bits sized
quantities to copy so "count == 1" is 4/8 bytes here.

Right.

There seem to be two callers of this API in the kernel, one is calling
with a constant size and wants a large TLP

Another seems to want memcpy_to_io with a guarenteed 32/64 bit store.

I don't really understand how that works together with the order not
being guaranteed. Do they use normal ioremap() and then require 32/64
bit TLPs and don't care about the order? But then the generic and ARM
variants do things in order so who knows if they actually rely on that.

quoted

quoted

IIRC you were merging at most 4 writes.
So better to do a single 32bit write instead.
(Unless you have misaligned source data - unlikely.)

While write-combining to generate long TLP is probably mostly
safe for PCIe targets, there are some that will only handle
TLP for single 32bit data items.
Which might be why the code is explicitly requesting 4 byte copies.
So it may be entirely wrong to write-combine anything except
the generic memcpy_toio().

On anything other than s390x this should only do write-combine if the
memory mapping allows it, no? Meaning a driver that can't handle larger
TLPs really shouldn't use ioremap_wc() then.

Right.

quoted

On s390x one could argue that our version of __iowriteXX_copy() is
strictly speaking not correct in that zpci_memcpy_toio() doesn't really
use XX bit writes which is why for us memcpy_toio() was actually a
better fit indeed. On the other hand doing 32 bit PCI stores (an s390x
thing) can't combine multiple stores into a single TLP which these
functions are used for and which has much more use cases than forcing a
copy loop with 32/64 bit sized writes which would also be a lot slower
on s390x than an aligned zpci_memcpy_toio().

mlx5 will definitely not work right if __iowrite64_copy() results in
anything smaller than 32/64 bit PCIe TLPs.

Jason

Yes and we do actually have mlx5 on s390x so this is my priority.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel