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

On Fri, 2024-02-23 at 09:07 +0000, David Laight wrote:

From: Jason Gunthorpe

quoted

Sent: 22 February 2024 22:36
To: David Laight <redacted>

On Thu, Feb 22, 2024 at 10:05:04PM +0000, David Laight wrote:

quoted

From: Jason Gunthorpe

quoted

Sent: 21 February 2024 01:17

The kernel provides driver support for using write combining IO memory
through the __iowriteXX_copy() API which is commonly used as an optional
optimization to generate 16/32/64 byte MemWr TLPs in a PCIe environment.

...

quoted

Implement __iowrite32/64_copy() specifically for ARM64 and use inline
assembly to build consecutive blocks of STR instructions. Provide direct
support for 64/32/16 large TLP generation in this manner. Optimize for
common constant lengths so that the compiler can directly inline the store
blocks.

...

quoted

+/*
+ * This generates a memcpy that works on a from/to address which is aligned to
+ * bits. Count is in terms of the number of bits sized quantities to copy. It
+ * optimizes to use the STR groupings when possible so that it is WC friendly.
+ */
+#define memcpy_toio_aligned(to, from, count, bits)                        \
+	({                                                                \
+		volatile u##bits __iomem *_to = to;                       \
+		const u##bits *_from = from;                              \
+		size_t _count = count;                                    \
+		const u##bits *_end_from = _from + ALIGN_DOWN(_count, 8); \
+                                                                          \
+		for (; _from < _end_from; _from += 8, _to += 8)           \
+			__const_memcpy_toio_aligned##bits(_to, _from, 8); \
+		if ((_count % 8) >= 4) {

If (_count & 4) {

That would be obfuscating, IMHO. The compiler doesn't need such things
to generate optimal code.

Try it: https://godbolt.org/z/EvvGrTxv3 
And it isn't that obfuscated - no more so than your version.

quoted

quoted

quoted

+			__const_memcpy_toio_aligned##bits(_to, _from, 1); \
+	})

But that looks bit a bit large to be inlined.

You trimmed alot, this #define is in a C file and it is a template to
generate the 32 and 64 bit out of line functions. Things are done like
this because the 32/64 version are exactly the same logic except just
with different types and sizes.

I missed that in a quick read at 11pm :-(

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.

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().

	David

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.

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().

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