Add and use memcpy_toio_64()

From: Jason Gunthorpe <[email protected]>

The kernel supports write combining IO memory which is commonly used to generate 64 byte TLPs in a PCIe environment. On many CPUs this mechanism is pretty tolerant and a simple C loop will suffice to generate a 64 byte TLP.

However modern ARM64 CPUs are quite sensitive and a compiler generated loop is not enough to reliably generate a 64 byte TLP. Especially given the ARM64 issue that writel() does not codegen anything other than "[xN]" as the address calculation.

These newer CPUs require an orderly consecutive block of stores to work reliably. This is best done with four STP integer instructions (perhaps ST64B in future), or a single ST4 vector instruction.

Provide a new generic function memcpy_toio_64() which should reliably generate the needed instructions for the architecture, assuming address alignment. As the usual need for this operation is performance sensitive a fast inline implementation is preferred.

Implement an optimized version on ARM that is a block of 4 STP instructions.

The generic implementation is just a simple loop. x86-64 (clang 16) compiles this into an unrolled loop of 16 movq pairs.

Cc Arnd Bergmann <[email protected]>

Cc Catalin Marinas <[email protected]>

Cc Will Deacon <[email protected]>

Cc [email protected]

Cc [email protected]

Signed-off-by Jason Gunthorpe <[email protected]>

Signed-off-by Leon Romanovsky <[email protected]>

---
 arch/arm64/include/asm/io.h | 20 ++++++++++++++++++++
 include/asm-generic/io.h    | 30 ++++++++++++++++++++++++++++++
 2 files changed, 50 insertions(+)

arch/arm64/include/asm/io.h +20 -0

index 3b694511b98f..73ab91913790 100644
@@ -135,6 +135,26 @@ extern void __memset_io(volatile void __iomem *, int, size_t);
 #define memcpy_fromio(a,c,l)	__memcpy_fromio((a),(c),(l))
 #define memcpy_toio(c,a,l)	__memcpy_toio((c),(a),(l))
 
+static inline void __memcpy_toio_64(volatile void __iomem *to, const void *from)
+{
+	const u64 *from64 = from;
+
+	/*
+	 * Newer ARM core have sensitive write combining buffers, it is
+	 * important that the stores be contiguous blocks of store instructions.
+	 * Normal memcpy does not work reliably.
+	 */
+	asm volatile("stp %x0, %x1, [%8, #16 * 0]\n"
+		     "stp %x2, %x3, [%8, #16 * 1]\n"
+		     "stp %x4, %x5, [%8, #16 * 2]\n"
+		     "stp %x6, %x7, [%8, #16 * 3]\n"
+		     :
+		     : "rZ"(from64[0]), "rZ"(from64[1]), "rZ"(from64[2]),
+		       "rZ"(from64[3]), "rZ"(from64[4]), "rZ"(from64[5]),
+		       "rZ"(from64[6]), "rZ"(from64[7]), "r"(to));
+}
+#define memcpy_toio_64(to, from) __memcpy_toio_64(to, from)
+
 /*
  * I/O memory mapping functions.
  */

include/asm-generic/io.h +30 -0

index bac63e874c7b..2d6d60ed2128 100644
@@ -1202,6 +1202,36 @@ static inline void memcpy_toio(volatile void __iomem *addr, const void *buffer,
 }
 #endif
 
+#ifndef memcpy_toio_64
+#define memcpy_toio_64 memcpy_toio_64
+/**
+ * memcpy_toio_64	Copy 64 bytes of data into I/O memory
+ * @dst:		The (I/O memory) destination for the copy
+ * @src:		The (RAM) source for the data
+ * @count:		The number of bytes to copy
+ *
+ * dst and src must be aligned to 8 bytes. This operation copies exactly 64
+ * bytes. It is intended to be used for write combining IO memory. The
+ * architecture should provide an implementation that has a high chance of
+ * generating a single combined transaction.
+ */
+static inline void memcpy_toio_64(volatile void __iomem *addr,
+				  const void *buffer)
+{
+	unsigned int i = 0;
+
+#if BITS_PER_LONG == 64
+	for (; i != 8; i++)
+		__raw_writeq(((const u64 *)buffer)[i],
+			     ((u64 __iomem *)addr) + i);
+#else
+	for (; i != 16; i++)
+		__raw_writel(((const u32 *)buffer)[i],
+			     ((u32 __iomem *)addr) + i);
+#endif
+}
+#endif
+
 extern int devmem_is_allowed(unsigned long pfn);
 
 #endif /* __KERNEL__ */

Signature · 1 line

2.42.0

Quoted 12 lines · Leon Romanovsky

From: Jason Gunthorpe <[email protected]>

The kernel supports write combining IO memory which is commonly used to generate 64 byte TLPs in a PCIe environment. On many CPUs this mechanism is pretty tolerant and a simple C loop will suffice to generate a 64 byte TLP.

However modern ARM64 CPUs are quite sensitive and a compiler generated loop is not enough to reliably generate a 64 byte TLP. Especially given the ARM64 issue that writel() does not codegen anything other than "[xN]" as the address calculation.

Quoted 8 lines

These newer CPUs require an orderly consecutive block of stores to work reliably. This is best done with four STP integer instructions (perhaps ST64B in future), or a single ST4 vector instruction.

Provide a new generic function memcpy_toio_64() which should reliably generate the needed instructions for the architecture, assuming address alignment. As the usual need for this operation is performance sensitive a fast inline implementation is preferred.

There is *no* architectural sequence that is guaranteed to reliably generate a 64-byte TLP, and this sequence won't guarnatee that (e.g. even if the CPU *always* merged adjacent stores, we can take an interrupt mid-sequence that would prevent that).

What's the actual requirement here? Is this just for performance?

From: Jason Gunthorpe <[email protected]>

The kernel supports write combining IO memory which is commonly used to generate 64 byte TLPs in a PCIe environment. On many CPUs this mechanism is pretty tolerant and a simple C loop will suffice to generate a 64 byte TLP.

However modern ARM64 CPUs are quite sensitive and a compiler generated loop is not enough to reliably generate a 64 byte TLP. Especially given the ARM64 issue that writel() does not codegen anything other than "[xN]" as the address calculation.

These newer CPUs require an orderly consecutive block of stores to work reliably. This is best done with four STP integer instructions (perhaps ST64B in future), or a single ST4 vector instruction.

Provide a new generic function memcpy_toio_64() which should reliably generate the needed instructions for the architecture, assuming address alignment. As the usual need for this operation is performance sensitive a fast inline implementation is preferred.

Implement an optimized version on ARM that is a block of 4 STP instructions.

The generic implementation is just a simple loop. x86-64 (clang 16) compiles this into an unrolled loop of 16 movq pairs.

Cc Arnd Bergmann <[email protected]>

Cc Catalin Marinas <[email protected]>

Cc Will Deacon <[email protected]>

Cc [email protected]

Cc [email protected]

Signed-off-by Jason Gunthorpe <[email protected]>

Signed-off-by Leon Romanovsky <[email protected]>

---
  arch/arm64/include/asm/io.h | 20 ++++++++++++++++++++
  include/asm-generic/io.h    | 30 ++++++++++++++++++++++++++++++
  2 files changed, 50 insertions(+)

arch/arm64/include/asm/io.h +7 -0

index 3b694511b98f..73ab91913790 100644
@@ -135,6 +135,26 @@ extern void __memset_io(volatile void __iomem *, int, size_t);
  #define memcpy_fromio(a,c,l)	__memcpy_fromio((a),(c),(l))
  #define memcpy_toio(c,a,l)	__memcpy_toio((c),(a),(l))
 
+static inline void __memcpy_toio_64(volatile void __iomem *to, const void *from)
+{
+	const u64 *from64 = from;
+
+	/*
+	 * Newer ARM core have sensitive write combining buffers, it is
+	 * important that the stores be contiguous blocks of store instructions.

arch/arm64/include/asm/io.h +3 -0

+}
+#define memcpy_toio_64(to, from) __memcpy_toio_64(to, from)
+
  /*
   * I/O memory mapping functions.
   */

include/asm-generic/io.h +30 -0

index bac63e874c7b..2d6d60ed2128 100644
@@ -1202,6 +1202,36 @@ static inline void memcpy_toio(volatile void __iomem *addr, const void *buffer,
  }
  #endif
 
+#ifndef memcpy_toio_64
+#define memcpy_toio_64 memcpy_toio_64
+/**
+ * memcpy_toio_64	Copy 64 bytes of data into I/O memory
+ * @dst:		The (I/O memory) destination for the copy
+ * @src:		The (RAM) source for the data
+ * @count:		The number of bytes to copy
+ *
+ * dst and src must be aligned to 8 bytes. This operation copies exactly 64
+ * bytes. It is intended to be used for write combining IO memory. The
+ * architecture should provide an implementation that has a high chance of
+ * generating a single combined transaction.
+ */
+static inline void memcpy_toio_64(volatile void __iomem *addr,
+				  const void *buffer)
+{
+	unsigned int i = 0;
+
+#if BITS_PER_LONG == 64
+	for (; i != 8; i++)
+		__raw_writeq(((const u64 *)buffer)[i],
+			     ((u64 __iomem *)addr) + i);
+#else
+	for (; i != 16; i++)
+		__raw_writel(((const u32 *)buffer)[i],
+			     ((u32 __iomem *)addr) + i);
+#endif
+}
+#endif
+
  extern int devmem_is_allowed(unsigned long pfn);
 
  #endif /* __KERNEL__ */

Quoted 18 lines

arch/arm64/include/asm/io.h +11 -0

index 3b694511b98f..73ab91913790 100644
@@ -135,6 +135,26 @@ extern void __memset_io(volatile void __iomem *, int, size_t);
  #define memcpy_fromio(a,c,l)	__memcpy_fromio((a),(c),(l))
  #define memcpy_toio(c,a,l)	__memcpy_toio((c),(a),(l))
+static inline void __memcpy_toio_64(volatile void __iomem *to, const void *from)
+{
+	const u64 *from64 = from;
+
+	/*
+	 * Newer ARM core have sensitive write combining buffers, it is
+	 * important that the stores be contiguous blocks of store instructions.
+	 * Normal memcpy does not work reliably.
+	 */
+	asm volatile("stp %x0, %x1, [%8, #16 * 0]\n"
+		     "stp %x2, %x3, [%8, #16 * 1]\n"

arch/arm64/include/asm/io.h extern void __memset_io(volatile void __iomem *, int, size_t); · line 154

Quoted 6 lines

arch/arm64/include/asm/io.h +6 -0

+		     "stp %x4, %x5, [%8, #16 * 2]\n"
+		     "stp %x6, %x7, [%8, #16 * 3]\n"
+		     :
+		     : "rZ"(from64[0]), "rZ"(from64[1]), "rZ"(from64[2]),
+		       "rZ"(from64[3]), "rZ"(from64[4]), "rZ"(from64[5]),
+		       "rZ"(from64[6]), "rZ"(from64[7]), "r"(to));

Is this correct for big-endian? LDP/STP are kinda tricksy in that regard.

From: Jason Gunthorpe <[email protected]>

The kernel supports write combining IO memory which is commonly used to generate 64 byte TLPs in a PCIe environment. On many CPUs this mechanism is pretty tolerant and a simple C loop will suffice to generate a 64 byte TLP.

However modern ARM64 CPUs are quite sensitive and a compiler generated loop is not enough to reliably generate a 64 byte TLP. Especially given the ARM64 issue that writel() does not codegen anything other than "[xN]" as the address calculation.

These newer CPUs require an orderly consecutive block of stores to work reliably. This is best done with four STP integer instructions (perhaps ST64B in future), or a single ST4 vector instruction.

Provide a new generic function memcpy_toio_64() which should reliably generate the needed instructions for the architecture, assuming address alignment. As the usual need for this operation is performance sensitive a fast inline implementation is preferred.

Implement an optimized version on ARM that is a block of 4 STP instructions.

The generic implementation is just a simple loop. x86-64 (clang 16) compiles this into an unrolled loop of 16 movq pairs.

Cc Arnd Bergmann <[email protected]>

Cc Catalin Marinas <[email protected]>

Cc Will Deacon <[email protected]>

Cc [email protected]

Cc [email protected]

Signed-off-by Jason Gunthorpe <[email protected]>

Signed-off-by Leon Romanovsky <[email protected]>

---

include/asm-generic/io.h +17 -0

+#ifndef memcpy_toio_64
+#define memcpy_toio_64 memcpy_toio_64
+/**
+ * memcpy_toio_64	Copy 64 bytes of data into I/O memory
+ * @dst:		The (I/O memory) destination for the copy
+ * @src:		The (RAM) source for the data
+ * @count:		The number of bytes to copy
+ *
+ * dst and src must be aligned to 8 bytes. This operation copies exactly 64
+ * bytes. It is intended to be used for write combining IO memory. The
+ * architecture should provide an implementation that has a high chance of
+ * generating a single combined transaction.
+ */
+static inline void memcpy_toio_64(volatile void __iomem *addr,
+				  const void *buffer)
+{
+	unsigned int i = 0;

include/asm-generic/io.h +3 -0

+}
+#endif
+
 extern int devmem_is_allowed(unsigned long pfn);
 
 #endif /* __KERNEL__ */

What's the reasoning behind not using the existing memcpy_toio() here?

For s390 the above generic variant would do 8 of our special PCI store instructions while memcpy_toio() is defined to zpci_memcpy_toio() which can do the same as a single PCI store block instruction. Now of course we could provide our own memcpy_toio_64() but that would end up the same as just doing memcpy_toio(addr, buffer, 64) here.

Quoted 2 lines · Niklas Schnelle

What's the reasoning behind not using the existing memcpy_toio() here?

Going forward CPUs are implementing an instruction to do a 64 byte aligned store, this is a wrapper for exactly that operation.

memcpy_toio() is much more general, it allows unaligned buffers and non-multiples of 64. Adapting the general version to generate the optimized version in the cases it can is complex and has a codegen penalty..

Quoted 2 lines · Jason Gunthorpe

On Fri, Nov 24, 2023 at 03:10:29PM +0100, Niklas Schnelle wrote:

Also seeing the second patch of course that would no longer really test for write combining for us which we can also do but I think that's okay and you're probably going to use memcpy_toio_64() in more places and there we really want the PCI store block.

Quoted 8 lines · Jason Gunthorpe

Quoted 2 lines · Niklas Schnelle

What's the reasoning behind not using the existing memcpy_toio() here?

Going forward CPUs are implementing an instruction to do a 64 byte aligned store, this is a wrapper for exactly that operation.

Quoted 4 lines

memcpy_toio() is much more general, it allows unaligned buffers and non-multiples of 64. Adapting the general version to generate the optimized version in the cases it can is complex and has a codegen penalty..

I think you misunderstood me. I understand why you want a separate memcpy_toio_64(). I just wonder if its generic implementation shouldn't just be a define or inline wrapper for memcpy_toio(addr, buffer, 64).

Also seeing the second patch of course that would no longer really test for write combining for us which we can also do but I think that's okay and you're probably going to use memcpy_toio_64() in more places and there we really want the PCI store block.

Quoted 18 lines · Robin Murphy

Quoted 18 lines

arch/arm64/include/asm/io.h +11 -0

index 3b694511b98f..73ab91913790 100644
@@ -135,6 +135,26 @@ extern void __memset_io(volatile void __iomem *, int, size_t);
   #define memcpy_fromio(a,c,l)	__memcpy_fromio((a),(c),(l))
   #define memcpy_toio(c,a,l)	__memcpy_toio((c),(a),(l))
+static inline void __memcpy_toio_64(volatile void __iomem *to, const void *from)
+{
+	const u64 *from64 = from;
+
+	/*
+	 * Newer ARM core have sensitive write combining buffers, it is
+	 * important that the stores be contiguous blocks of store instructions.
+	 * Normal memcpy does not work reliably.
+	 */
+	asm volatile("stp %x0, %x1, [%8, #16 * 0]\n"
+		     "stp %x2, %x3, [%8, #16 * 1]\n"

Quoted 8 lines

arch/arm64/include/asm/io.h extern void __memset_io(volatile void __iomem *, int, size_t); · line 154

Quoted 6 lines

arch/arm64/include/asm/io.h +6 -0

+		     "stp %x4, %x5, [%8, #16 * 2]\n"
+		     "stp %x6, %x7, [%8, #16 * 3]\n"
+		     :
+		     : "rZ"(from64[0]), "rZ"(from64[1]), "rZ"(from64[2]),
+		       "rZ"(from64[3]), "rZ"(from64[4]), "rZ"(from64[5]),
+		       "rZ"(from64[6]), "rZ"(from64[7]), "r"(to));

Is this correct for big-endian? LDP/STP are kinda tricksy in that regard.

Uh.. I didn't think about it at all..

By no means do I have any skill reading the ARM documents, but I think it is OK, it says:

Mem[address, dbytes, AccType_NORMAL] = data1; Mem[address+dbytes, dbytes, AccType_NORMAL] = data2;

So I understand that as

Mem[%8, #16 * 0, 8, AccType_NORMAL] = from64[0] Mem[%8, #16 * 0 + 1 , 8, AccType_NORMAL] = from64[1] Mem[%8, #16 * 1, 8, AccType_NORMAL] = from64[2] Mem[%8, #16 * 1 + 1, 8, AccType_NORMAL] = from64[3] ..

Which is the same on BE/LE?

But I don't know the pitfall to watch for here. This is memcpy so we don't have to swap, the order of the bits in the register doesn't matter.

Quoted 9 lines · Niklas Schnelle

Quoted 8 lines · Jason Gunthorpe

Quoted 2 lines · Niklas Schnelle

What's the reasoning behind not using the existing memcpy_toio() here?

Going forward CPUs are implementing an instruction to do a 64 byte aligned store, this is a wrapper for exactly that operation.

Quoted 8 lines

Quoted 4 lines

memcpy_toio() is much more general, it allows unaligned buffers and non-multiples of 64. Adapting the general version to generate the optimized version in the cases it can is complex and has a codegen penalty..

I think you misunderstood me. I understand why you want a separate memcpy_toio_64(). I just wonder if its generic implementation shouldn't just be a define or inline wrapper for memcpy_toio(addr, buffer, 64).

Oh, yes, I totally did.

I'm worried that x86 will less reliably generate write combining with it's memcpy_toio implemention. It codegens byte copies for that function :(

Quoted 5 lines

Also seeing the second patch of course that would no longer really test for write combining for us which we can also do but I think that's okay and you're probably going to use memcpy_toio_64() in more places and there we really want the PCI store block.

Right now we don't have in-kernel performance use cases for write combining for mlx5.

Userspace uses the WC and we already have the special 390 instructions for batching in rdma-core already, IIRC.

So it would be appropriate for s390 to use a consistent path.

Jason

This should be as easy as adding

#define memcpy_toio_64(to, from) zpci_memcpy_toio(to, from, 64)

to arch/s390/include/asm/io.h. I'm wondering if we should do that as part of this series. It's not as good as a special case but probably better than the existing loop.

I don't think we have any existing in-kernel users of memcpy_toio() on s390 so far though so I'd like to give this some extra testing. Could you share instructions on how to exercise the code path of patch 2 on a ConnectX-5 or 6? Is this exercised e.g. when using NVMe-oF RDMA?

Quoted 2 lines · Mark Rutland

Quoted 1 line · Leon Romanovsky

From: Jason Gunthorpe <[email protected]>

Quoted 9 lines

Quoted 4 lines

Provide a new generic function memcpy_toio_64() which should reliably generate the needed instructions for the architecture, assuming address alignment. As the usual need for this operation is performance sensitive a fast inline implementation is preferred.

There is *no* architectural sequence that is guaranteed to reliably generate a 64-byte TLP, and this sequence won't guarnatee that (e.g. even if the CPU *always* merged adjacent stores, we can take an interrupt mid-sequence that would prevent that).

WC is not guaranteed on any arch, that is well known.

The HW has means to handle fragmented TLPs, it just hurts performance when it happens. "reliable" here means we'd like to see something like a > 90% chance of the large TLP instead of the < 1% chance with the C loop.

Future ARM CPUs have the ST64B instruction which does provide the architectural guarantee, and x86 has a similar guaranteed instruction now too.

Quoted 1 line

What's the actual requirement here? Is this just for performance?

Yes, just performance.

Quoted 3 lines

Quoted 1 line

What's the actual requirement here? Is this just for performance?

Yes, just performance.

Do you have any rough numbers (percentage)? It's highly microarchitecture-dependent until we get the ST64B instruction.

More of a bike-shedding, I wonder whether the __iowrite*_copy() semantics are better suited for what you need in terms of ordering (not that mempcy_toio() to Normal NC memory gives us any ordering).

Quoted 7 lines · Niklas Schnelle

This should be as easy as adding

#define memcpy_toio_64(to, from) zpci_memcpy_toio(to, from, 64)

to arch/s390/include/asm/io.h. I'm wondering if we should do that as part of this series. It's not as good as a special case but probably better than the existing loop.

Makes sense

Quoted 5 lines

I don't think we have any existing in-kernel users of memcpy_toio() on s390 so far though so I'd like to give this some extra testing. Could you share instructions on how to exercise the code path of patch 2 on a ConnectX-5 or 6? Is this exercised e.g. when using NVMe-oF RDMA?

Simply boot and look at pr_debug from mlx5 to see if writecombining is on or off - you want to see on.

Thanks,
Jason

Also it turns out the writeq() loop we had so far does not produce the needed 64 byte TLP on s390 either so this actually makes us newly pass this test.

Quoted 3 lines · Niklas Schnelle

Also it turns out the writeq() loop we had so far does not produce the needed 64 byte TLP on s390 either so this actually makes us newly pass this test.

Ooh, that is a significant problem - the userspace code won't be used unless this test passes. So we need this on S390 to fix a bug as well :\

Thanks,
Jason

Quoted 6 lines · Catalin Marinas

Quoted 3 lines

Quoted 1 line

What's the actual requirement here? Is this just for performance?

Yes, just performance.

Do you have any rough numbers (percentage)? It's highly microarchitecture-dependent until we get the ST64B instruction.

The current C code is an open coded store loop. The kernel does 250 tries and measures if any one of them succeeds to combine.

On x86, and older ARM cores we see that 100% of the time at least 1 in 250 tries succeeds.

With the new CPU cores we see more like 9 out of 10 time there are 0 in 250 tries that succeed. Ie we can go thousands of times without seeing any successful WC combine.

The STP block brings it back to 100% of the time 1 in 250 succeed.

However, in userspace we have long been using ST4 to create a single-instruction 64 byte store on ARM64. As far as I know this is highly reliable. I don't have direct data on the STP configuration.

Quoted 3 lines

More of a bike-shedding, I wonder whether the __iowrite*_copy() semantics are better suited for what you need in terms of ordering (not that mempcy_toio() to Normal NC memory gives us any ordering).

I have the same remark I gave to Niklas, this does not require alignment or an exact 64 byte size. It was clearly made to support WC stores since Pathscale did it, but I don't see this mapping nicely to the future 64 byte store instructions are we getting.

We could name it __iowrite512_copy() if that makes more sense?

Quoted 18 lines · Jason Gunthorpe

Quoted 6 lines · Catalin Marinas

Quoted 3 lines

Quoted 1 line

What's the actual requirement here? Is this just for performance?

Yes, just performance.

Do you have any rough numbers (percentage)? It's highly microarchitecture-dependent until we get the ST64B instruction.

The current C code is an open coded store loop. The kernel does 250 tries and measures if any one of them succeeds to combine.

On x86, and older ARM cores we see that 100% of the time at least 1 in 250 tries succeeds.

With the new CPU cores we see more like 9 out of 10 time there are 0 in 250 tries that succeed. Ie we can go thousands of times without seeing any successful WC combine.

Quoted 1 line

The STP block brings it back to 100% of the time 1 in 250 succeed.

That's a bit confusing to me: 1 in 250 succeeding is still pretty rare. But I guess what your benchmark says is that at least 1 succeeded to write-combine and it might as well be all 250 tries. It's more interesting to see if there's actual performance gain in real-world traffic, not just some artificial benchmark (I may have misunderstood your numbers above).

Quoted 3 lines

However, in userspace we have long been using ST4 to create a single-instruction 64 byte store on ARM64. As far as I know this is highly reliable. I don't have direct data on the STP configuration.

Personally I'd optimise the mempcy_toio() arm64 implementation to do STPs if the alignment is right (like we do for classic memcpy()). There's a slight overhead for alignment checking but I suspect it would be lost as long as you can get the write-combining. Not sure whether the interspersed reads in memcpy_toio() would somehow prevent the write-combining.

A memcpy_toio_64() can use the new ST64B instruction if available or fall back to memcpy_toio() on arm64. It should also have the DGH instruction (io_stop_wc()) but only if falling back to classic memcpy_toio(). We don't need DGH with ST64B.

Quoted 6 lines · Catalin Marinas

Personally I'd optimise the mempcy_toio() arm64 implementation to do STPs if the alignment is right (like we do for classic memcpy()). There's a slight overhead for alignment checking but I suspect it would be lost as long as you can get the write-combining. Not sure whether the interspersed reads in memcpy_toio() would somehow prevent the write-combining.

I understand on these new CPUs anything other than a block of contiguous STPs is risky to break the WC. I was told we should not have any loads between them.

So we can't just update memcpy_toio to optimize a 128 bit store variant like memcpy might. We actually need a special case just for 64 byte.

IMHO it does not look good as the chance any existing callers can use this optmized 64B path is probably small, but everyone has to pay the costs to check for it.

I also would not do this on x86 - Pathscale apparently decided the needed special __iowrite*_copy() things to actually make this work on xome x86 systems - I'm very leary to change x86 stuff away from the 64 bit copy loopw we know works already on x86.

IMHO encoding the alignment expectation in the API is best, especially since this is typically a performance path.

Quoted 4 lines

A memcpy_toio_64() can use the new ST64B instruction if available or fall back to memcpy_toio() on arm64. It should also have the DGH instruction (io_stop_wc()) but only if falling back to classic memcpy_toio(). We don't need DGH with ST64B.

I'm told it is problematic, something about ST64B not working with NORMAL_NC.

Also in a future ST64B world we are going to see HW start relying on large TLPs, not just being an optional performance win. To my mind it makes more sense that there is an API that guarantees a large TLP or oops. We really don't want an automatic fallback to memcpy.

Quoted 8 lines · Jason Gunthorpe

Quoted 6 lines · Catalin Marinas

Personally I'd optimise the mempcy_toio() arm64 implementation to do STPs if the alignment is right (like we do for classic memcpy()). There's a slight overhead for alignment checking but I suspect it would be lost as long as you can get the write-combining. Not sure whether the interspersed reads in memcpy_toio() would somehow prevent the write-combining.

Quoted 3 lines

I understand on these new CPUs anything other than a block of contiguous STPs is risky to break the WC. I was told we should not have any loads between them.

Classic memcpy does similar tricks with four LDPs in a row before starting to issue the STPs (though there are new LDPs for the next data in-between). But that was tuned for cacheable memory, not sure if something similar would behave well on Normal-NC memory.

Quoted 2 lines

I'm told it is problematic, something about ST64B not working with NORMAL_NC.

Last time I checked it was meant to work on Normal-NC (not cacheable though). That's on page 285 of the Arm ARM J.a.

Quoted 9 lines · Catalin Marinas

Quoted 8 lines · Jason Gunthorpe

Quoted 6 lines · Catalin Marinas

Personally I'd optimise the mempcy_toio() arm64 implementation to do STPs if the alignment is right (like we do for classic memcpy()). There's a slight overhead for alignment checking but I suspect it would be lost as long as you can get the write-combining. Not sure whether the interspersed reads in memcpy_toio() would somehow prevent the write-combining.

Quoted 8 lines

Quoted 3 lines

I understand on these new CPUs anything other than a block of contiguous STPs is risky to break the WC. I was told we should not have any loads between them.

Classic memcpy does similar tricks with four LDPs in a row before starting to issue the STPs (though there are new LDPs for the next data in-between). But that was tuned for cacheable memory, not sure if something similar would behave well on Normal-NC memory.

Can we conclude a direction here?

1) I don't want to mess with x86 so we keep a dedicated API Are we agreed to call it __iowrite512_copy() and note its special alignment limitation?

2) You want to #define __iowrite512_copy() to memcpy_toio() on ARM and implement some quad STP optimization for this case?

3) A future ST64B and the x86 version would be put under __iowrite512_copy()?

4) A future ST64B would come with some kind of 'must do 64b copy or oops' to support the future HW that must have this instruction? eg we already see on Intel that HW must use ENQCMD and nothing else.

Quoted 2 lines

2) You want to #define __iowrite512_copy() to memcpy_toio() on ARM and implement some quad STP optimization for this case?

We can have the generic __iowrite512_copy() do memcpy_toio() and have the arm64 implement an optimised version.

What I'm not entirely sure of is the DGH (whatever the io_* barrier name is). I'd put it in the same __iowrite512_copy() function and remove it from the driver code. Otherwise when ST64B is added, we have an unnecessary DGH in the driver. If this does not match the other __iowrite*_copy() semantics, we can come up with another name. But start with this for now and document the function.

Quoted 2 lines

3) A future ST64B and the x86 version would be put under __iowrite512_copy()?

Yes, arch-specific override.

Quoted 3 lines

4) A future ST64B would come with some kind of 'must do 64b copy or oops' to support the future HW that must have this instruction? eg we already see on Intel that HW must use ENQCMD and nothing else.

I don't agree with the oops part. We can't guarantee it on arm64, ST64B I think is optional in the architecture. If you do need such guarantees, we'd need the driver to probe for the feature (e.g. arch_has_...()) and invoke a new macro.

Quoted 6 lines · Catalin Marinas

Quoted 2 lines

2) You want to #define __iowrite512_copy() to memcpy_toio() on ARM and implement some quad STP optimization for this case?

We can have the generic __iowrite512_copy() do memcpy_toio() and have the arm64 implement an optimised version.

Quoted 6 lines

What I'm not entirely sure of is the DGH (whatever the io_* barrier name is). I'd put it in the same __iowrite512_copy() function and remove it from the driver code. Otherwise when ST64B is added, we have an unnecessary DGH in the driver. If this does not match the other __iowrite*_copy() semantics, we can come up with another name. But start with this for now and document the function.

I think the iowrite is only used for WC and the DGH is functionally harmless for non-WC, so it makes sense.

In this case we should just remove the DGH macro from the generic architecture code and tell people to use iowrite - since we now understand that callers basically have to in order to use DGH on new ARM CPUs.

Quoted 17 lines · Jason Gunthorpe

Quoted 6 lines · Catalin Marinas

Quoted 2 lines

2) You want to #define __iowrite512_copy() to memcpy_toio() on ARM and implement some quad STP optimization for this case?

We can have the generic __iowrite512_copy() do memcpy_toio() and have the arm64 implement an optimised version.

Quoted 6 lines

What I'm not entirely sure of is the DGH (whatever the io_* barrier name is). I'd put it in the same __iowrite512_copy() function and remove it from the driver code. Otherwise when ST64B is added, we have an unnecessary DGH in the driver. If this does not match the other __iowrite*_copy() semantics, we can come up with another name. But start with this for now and document the function.

I think the iowrite is only used for WC and the DGH is functionally harmless for non-WC, so it makes sense.

Quoted 4 lines

In this case we should just remove the DGH macro from the generic architecture code and tell people to use iowrite - since we now understand that callers basically have to in order to use DGH on new ARM CPUs.

That works for me but what would the semantics be for __iowrite64_copy() for example? Is there a DGH at the end of the whole write or after each iteration?

I'd go with the former since e.g. hns3_tx_push_bd() does that (and doesn't seem to be a 64 byte copy).

Quoted 9 lines · Jason Gunthorpe

Quoted 3 lines · Niklas Schnelle

Also it turns out the writeq() loop we had so far does not produce the needed 64 byte TLP on s390 either so this actually makes us newly pass this test.

Ooh, that is a significant problem - the userspace code won't be used unless this test passes. So we need this on S390 to fix a bug as well :\

Yes ;-(

In the meantime I also found out that zpci_write_block(dst, src, 64) is not correct for all cases because the PCI store block requires the (pseudo-)MMIO write not to cross a 4K boundary and we need src/dst to be double word aligned. In rdma-core this is neatly handled by the get_max_write_size() but the kernel variant of that zpci_get_max_write_size() isn't just a lot harder to read and likely less efficient but also too strict thus breaking the 64 byte write up needlessly.

In total we have 5 conditions for the PCI block stores:

1. The dst+len must not cross a 4K boundary in the (pseudo-)MMIO space
2. The length must not exceed the maximum write size
3. The length must be a multiple of 8
4. The src needs to be double word aligned
5. The dst needs to be double word aligned

So I think a good solution would be to improve zpci_memcpy_toio() with an enhanced zpci_get_max_write_size() based on the code in rdma-core extended to also handle the alignment and length restrictions which in rdma-core are already assumed (see comment there). Then we can use zpci_memcpy_toio(dst, src, 64) for memcpy_toio_64() and rely on the compiler to optimize out the unnecessary checks (2, 3 and possibly 4, 5).

So yeah this is getting a bit more complicated than originally thought. Let me cook up a patch.

Hey Catalin,

I'm just revising this and I'm wondering if you know why ARM64 has this:

#define __raw_writeq __raw_writeq
static __always_inline void __raw_writeq(u64 val, volatile void __iomem *addr)

{
	asm volatile("str %x0, [%1]" : : "rZ" (val), "r" (addr));
}

Instead of

#define __raw_writeq __raw_writeq
static __always_inline void __raw_writeq(u64 val, volatile void __iomem *addr)

{
	asm volatile("str %x0, %1" : : "rZ" (val), "m" (*(volatile u64 *)addr));
}

?? Like x86 has.

The codegen for a 64 byte unrolled copy loop is way better with "m" on gcc:

"r" constraint (gcc 13.2.0):

.L3:

        ldr     x3, [x1]
        str x3, [x0]
        ldr     x3, [x1, 8]
        add     x4, x0, 8
        str x3, [x4]
        ldr     x3, [x1, 16]
        add     x4, x0, 16
        str x3, [x4]
        ldr     x3, [x1, 24]
        add     x4, x0, 24
        str x3, [x4]
        ldr     x3, [x1, 32]
        add     x4, x0, 32
        str x3, [x4]
        ldr     x3, [x1, 40]
        add     x4, x0, 40
        str x3, [x4]
        ldr     x3, [x1, 48]
        add     x4, x0, 48
        str x3, [x4]
        ldr     x3, [x1, 56]
        add     x4, x0, 56
        str x3, [x4]
        add     x1, x1, 64
        add     x0, x0, 64
        cmp     x2, x1
        bhi     .L3

"m" constraint:

.L3:

        ldp     x10, x9, [x1]
        ldp     x8, x7, [x1, 16]
        ldp     x6, x5, [x1, 32]
        ldp     x4, x3, [x1, 48]
        str x10, [x0]
        str x9, [x0, 8]
        str x8, [x0, 16]
        str x7, [x0, 24]
        str x6, [x0, 32]
        str x5, [x0, 40]
        str x4, [x0, 48]
        str x3, [x0, 56]
        add     x1, x1, 64
        add     x0, x0, 64
        cmp     x2, x1
        bhi     .L3

clang 17 doesn't do any better either way, it doesn't seem to do anything with 'm', but I guess it could..

clang 17 (either):

.LBB0_2: // =>This Inner Loop Header: Depth=1

        ldp     x9, x10, [x1]
        add     x14, x0, #8
        add     x18, x0, #40
        ldp     x11, x12, [x1, #16]
        add     x2, x0, #48
        add     x3, x0, #56
        ldp     x13, x15, [x1, #32]
        ldp     x16, x17, [x1, #48]
        str     x9, [x0]
        str     x10, [x14]
        add     x9, x0, #16
        add     x10, x0, #24
        add     x14, x0, #32
        str     x11, [x9]
        str     x12, [x10]
        str     x13, [x14]
        str     x15, [x18]
        str     x16, [x2]
        str     x17, [x3]
        add     x1, x1, #64
        add     x0, x0, #64
        cmp     x1, x8
        b.lo    .LBB0_2

It doesn't matter for this series, but it seems like something ARM64 might want to look at to improve..

Jason

Quoted 12 lines · Jason Gunthorpe

Hey Catalin,

I'm just revising this and I'm wondering if you know why ARM64 has this:

#define __raw_writeq __raw_writeq
static __always_inline void __raw_writeq(u64 val, volatile void __iomem *addr)

{
	asm volatile("str %x0, [%1]" : : "rZ" (val), "r" (addr));
}

Instead of

Quoted 7 lines

#define __raw_writeq __raw_writeq
static __always_inline void __raw_writeq(u64 val, volatile void __iomem *addr)

{
	asm volatile("str %x0, %1" : : "rZ" (val), "m" (*(volatile u64 *)addr));
}

?? Like x86 has.

I believe this is for the same reason as doing so in all of our other IO accessors.

We've deliberately ensured that our IO accessors use a single base register with no offset as this is the only form that HW can represent in ESR_ELx.ISS.SRT when reporting a stage-2 abort, which a hypervisor may use for emulating IO.

Quoted 21 lines · Mark Rutland

Quoted 12 lines · Jason Gunthorpe

Hey Catalin,

I'm just revising this and I'm wondering if you know why ARM64 has this:

#define __raw_writeq __raw_writeq
static __always_inline void __raw_writeq(u64 val, volatile void __iomem *addr)

{
	asm volatile("str %x0, [%1]" : : "rZ" (val), "r" (addr));
}

Instead of

Quoted 7 lines

#define __raw_writeq __raw_writeq
static __always_inline void __raw_writeq(u64 val, volatile void __iomem *addr)

{
	asm volatile("str %x0, %1" : : "rZ" (val), "m" (*(volatile u64 *)addr));
}

?? Like x86 has.

Quoted 7 lines

I believe this is for the same reason as doing so in all of our other IO accessors.

We've deliberately ensured that our IO accessors use a single base register with no offset as this is the only form that HW can represent in ESR_ELx.ISS.SRT when reporting a stage-2 abort, which a hypervisor may use for emulating IO.

Wow, harming bare metal performace to accommodate imperfect emulation sounds like a horrible reason :(

So what happens with this patch where IO is done with STP? Are you going to tell me I can't do it because of this?

Quoted 38 lines · Niklas Schnelle

Quoted 9 lines · Jason Gunthorpe

Quoted 3 lines · Niklas Schnelle

Also it turns out the writeq() loop we had so far does not produce the needed 64 byte TLP on s390 either so this actually makes us newly pass this test.

Ooh, that is a significant problem - the userspace code won't be used unless this test passes. So we need this on S390 to fix a bug as well :\

Yes ;-(

In the meantime I also found out that zpci_write_block(dst, src, 64) is not correct for all cases because the PCI store block requires the (pseudo-)MMIO write not to cross a 4K boundary and we need src/dst to be double word aligned. In rdma-core this is neatly handled by the get_max_write_size() but the kernel variant of that zpci_get_max_write_size() isn't just a lot harder to read and likely less efficient but also too strict thus breaking the 64 byte write up needlessly.

In total we have 5 conditions for the PCI block stores:

1. The dst+len must not cross a 4K boundary in the (pseudo-)MMIO space
2. The length must not exceed the maximum write size
3. The length must be a multiple of 8
4. The src needs to be double word aligned
5. The dst needs to be double word aligned

So I think a good solution would be to improve zpci_memcpy_toio() with an enhanced zpci_get_max_write_size() based on the code in rdma-core extended to also handle the alignment and length restrictions which in rdma-core are already assumed (see comment there). Then we can use zpci_memcpy_toio(dst, src, 64) for memcpy_toio_64() and rely on the compiler to optimize out the unnecessary checks (2, 3 and possibly 4, 5).

Quoted 2 lines

So yeah this is getting a bit more complicated than originally thought. Let me cook up a patch.

Did you come up with something?

Jason

Sorry I haven't replied. Yes, I have a fix for zpci_memcpy_toio() titled "s390/pci: fix max size calculation in zpci_memcpy_toio()" that I tested with this series plus the following define added to arch/s390/include/asm/io.h:

#define memcpy_toio_64 zpci_memcpy_toio(dst, src, 64)

It's already in the s390 tree's feature branch and linux-next.

Quoted 24 lines · Mark Rutland

Quoted 12 lines · Jason Gunthorpe

Hey Catalin,

I'm just revising this and I'm wondering if you know why ARM64 has this:

#define __raw_writeq __raw_writeq
static __always_inline void __raw_writeq(u64 val, volatile void __iomem *addr)

{
	asm volatile("str %x0, [%1]" : : "rZ" (val), "r" (addr));
}

Instead of

Quoted 7 lines

#define __raw_writeq __raw_writeq
static __always_inline void __raw_writeq(u64 val, volatile void __iomem *addr)

{
	asm volatile("str %x0, %1" : : "rZ" (val), "m" (*(volatile u64 *)addr));
}

?? Like x86 has.

I believe this is for the same reason as doing so in all of our other IO accessors.

Quoted 4 lines

We've deliberately ensured that our IO accessors use a single base register with no offset as this is the only form that HW can represent in ESR_ELx.ISS.SRT when reporting a stage-2 abort, which a hypervisor may use for emulating IO.

Wow, harming bare metal performace to accommodate imperfect emulation sounds like a horrible reason :(

So what happens with this patch where IO is done with STP? Are you going to tell me I can't do it because of this?

Quoted 18 lines · Jason Gunthorpe

Hey Catalin,

I'm just revising this and I'm wondering if you know why ARM64 has this:

#define __raw_writeq __raw_writeq
static __always_inline void __raw_writeq(u64 val, volatile void __iomem *addr)

{
	asm volatile("str %x0, [%1]" : : "rZ" (val), "r" (addr));
}

Instead of

#define __raw_writeq __raw_writeq
static __always_inline void __raw_writeq(u64 val, volatile void __iomem *addr)

{
	asm volatile("str %x0, %1" : : "rZ" (val), "m" (*(volatile u64 *)addr));
}

Quoted 1 line

?? Like x86 has.

I believe this is for the same reason as doing so in all of our other IO accessors.

We've deliberately ensured that our IO accessors use a single base register with no offset as this is the only form that HW can represent in ESR_ELx.ISS.SRT when reporting a stage-2 abort, which a hypervisor may use for emulating IO.

Quoted 6 lines

I believe this is for the same reason as doing so in all of our other IO accessors.

We've deliberately ensured that our IO accessors use a single base register with no offset as this is the only form that HW can represent in ESR_ELx.ISS.SRT when reporting a stage-2 abort, which a hypervisor may use for emulating IO.

FWIW, IIUC the immediate-offset forms *without* writeback can still be reported usefully in ESR_ELx, so I believe that we could use the "o" constraint for the __raw_write*() functions, e.g.

static __always_inline void __raw_writeq(u64 val, volatile void __iomem *addr)

{
	asm volatile("str %x0, %1" : : "rZ" (val), "o" (*(volatile u64 *)addr));
}

Quoted 7 lines · Mark Rutland

Quoted 6 lines

I believe this is for the same reason as doing so in all of our other IO accessors.

We've deliberately ensured that our IO accessors use a single base register with no offset as this is the only form that HW can represent in ESR_ELx.ISS.SRT when reporting a stage-2 abort, which a hypervisor may use for emulating IO.

Quoted 8 lines

FWIW, IIUC the immediate-offset forms *without* writeback can still be reported usefully in ESR_ELx, so I believe that we could use the "o" constraint for the __raw_write*() functions, e.g.

static __always_inline void __raw_writeq(u64 val, volatile void __iomem *addr)

{
	asm volatile("str %x0, %1" : : "rZ" (val), "o" (*(volatile u64 *)addr));
}

"o" works well in the same simple memcpy loop:

        add     x2, x1, w2, uxtw 3
        cmp     x1, x2
        bcs     .L1
.L3:
        ldp     x10, x9, [x1]
        ldp     x8, x7, [x1, 16]
        ldp     x6, x5, [x1, 32]
        ldp     x4, x3, [x1, 48]
        str x10, [x0]
        str x9, [x0, 8]
        str x8, [x0, 16]
        str x7, [x0, 24]
        str x6, [x0, 32]
        str x5, [x0, 40]
        str x4, [x0, 48]
        str x3, [x0, 56]
        add     x1, x1, 64
        add     x0, x0, 64
        cmp     x2, x1
        bhi     .L3

.L1:
ret

Seems intersting to pursue?

Quoted 8 lines · Niklas Schnelle

Sorry I haven't replied. Yes, I have a fix for zpci_memcpy_toio() titled "s390/pci: fix max size calculation in zpci_memcpy_toio()" that I tested with this series plus the following define added to arch/s390/include/asm/io.h:

#define memcpy_toio_64 zpci_memcpy_toio(dst, src, 64)

It's already in the s390 tree's feature branch and linux-next.

Great!

Quoted 11 lines · Jason Gunthorpe

Quoted 8 lines · Niklas Schnelle

Sorry I haven't replied. Yes, I have a fix for zpci_memcpy_toio() titled "s390/pci: fix max size calculation in zpci_memcpy_toio()" that I tested with this series plus the following define added to arch/s390/include/asm/io.h:

#define memcpy_toio_64 zpci_memcpy_toio(dst, src, 64)

It's already in the s390 tree's feature branch and linux-next.

Great!

Is this wrong too?

/* combine single writes by using store-block insn */
void __iowrite64_copy(void __iomem *to, const void *from, size_t count)

{
       zpci_memcpy_toio(to, from, count);
}

* __iowrite64_copy - copy data to MMIO space, in 64-bit or 32-bit units
* @to: destination, in MMIO space (must be 64-bit aligned)
* @from: source (must be 64-bit aligned)
* @count: number of 64-bit quantities to copy

                ^^^^^^^^^^^^^^^^^^^^^^^^^

Ie it should be

       zpci_memcpy_toio(to, from, count * 8);

Right?

(I'll fix it)

Jason

Yes, we need zpci_memcpy_toio(to, from, count * 8) since our count is in bytes like for memcpy_toio().

Quoted 2 lines · Niklas Schnelle

Yes, we need zpci_memcpy_toio(to, from, count * 8) since our count is in bytes like for memcpy_toio().

https://github.com/jgunthorpe/linux/commits/mlx5_wc/

Jason

Quoted 43 lines · Jason Gunthorpe

Quoted 7 lines · Mark Rutland

Quoted 6 lines

I believe this is for the same reason as doing so in all of our other IO accessors.

We've deliberately ensured that our IO accessors use a single base register with no offset as this is the only form that HW can represent in ESR_ELx.ISS.SRT when reporting a stage-2 abort, which a hypervisor may use for emulating IO.

Quoted 8 lines

FWIW, IIUC the immediate-offset forms *without* writeback can still be reported usefully in ESR_ELx, so I believe that we could use the "o" constraint for the __raw_write*() functions, e.g.

static __always_inline void __raw_writeq(u64 val, volatile void __iomem *addr)

{
	asm volatile("str %x0, %1" : : "rZ" (val), "o" (*(volatile u64 *)addr));
}

"o" works well in the same simple memcpy loop:

        add     x2, x1, w2, uxtw 3
        cmp     x1, x2
        bcs     .L1
.L3:
        ldp     x10, x9, [x1]
        ldp     x8, x7, [x1, 16]
        ldp     x6, x5, [x1, 32]
        ldp     x4, x3, [x1, 48]
        str x10, [x0]
        str x9, [x0, 8]
        str x8, [x0, 16]
        str x7, [x0, 24]
        str x6, [x0, 32]
        str x5, [x0, 40]
        str x4, [x0, 48]
        str x3, [x0, 56]
        add     x1, x1, 64
        add     x0, x0, 64
        cmp     x2, x1
        bhi     .L3

.L1:
ret

Quoted 1 line

Seems intersting to pursue?

I've seen the compiler struggle with plain "o" in the past ("Impossible constraint in asm") so we might want "Qo" if we go down this route.

Quoted 9 lines · Jason Gunthorpe

Quoted 2 lines · Niklas Schnelle

Yes, we need zpci_memcpy_toio(to, from, count * 8) since our count is in bytes like for memcpy_toio().

https://github.com/jgunthorpe/linux/commits/mlx5_wc/

Jason

Thanks, the s390 patches:

s390: Implement __iowrite32_copy()
s390: Use the correct count for __iowrite64_copy()
s390: Stop using weak symbols for __iowrite64_copy()

Look good to me. I.e. you may add my.

Acked-by Niklas Schnelle <[email protected]>

I did test your patches too and by accident confirmed again that these do need commit 80df7d6af7f6 ("s390/pci: fix max size calculation in zpci_memcpy_toio()") from the s390 feature branch to get the mlx5 driver to detect Write-Combining as supported. Note, as far as I know Alexander Gordeev is targeting that one for v6.8-rc2 since we had quite a few changes for v6.8-rc1.

Quoted 14 lines · Niklas Schnelle

Quoted 9 lines · Jason Gunthorpe

Quoted 2 lines · Niklas Schnelle

Yes, we need zpci_memcpy_toio(to, from, count * 8) since our count is in bytes like for memcpy_toio().

https://github.com/jgunthorpe/linux/commits/mlx5_wc/

Jason

Thanks, the s390 patches:

Quoted 7 lines

s390: Implement __iowrite32_copy()
s390: Use the correct count for __iowrite64_copy()
s390: Stop using weak symbols for __iowrite64_copy()

Look good to me. I.e. you may add my.

Acked-by Niklas Schnelle <[email protected]>

Great, thanks. I'll post this once rc1 comes out

Quoted 6 lines

I did test your patches too and by accident confirmed again that these do need commit 80df7d6af7f6 ("s390/pci: fix max size calculation in zpci_memcpy_toio()") from the s390 feature branch to get the mlx5 driver to detect Write-Combining as supported. Note, as far as I know Alexander Gordeev is targeting that one for v6.8-rc2 since we had quite a few changes for v6.8-rc1.

OK, but we can still run these two things in parallel?

Jason

Quoted 4 lines · Jason Gunthorpe

Quoted 3 lines · Niklas Schnelle

Quoted 1 line · Jason Gunthorpe

On Thu, Jan 18, 2024 at 02:46:40PM +0100, Niklas Schnelle wrote:

Quoted 9 lines

Quoted 8 lines

I did test your patches too and by accident confirmed again that these do need commit 80df7d6af7f6 ("s390/pci: fix max size calculation in zpci_memcpy_toio()") from the s390 feature branch to get the mlx5 driver to detect Write-Combining as supported. Note, as far as I know Alexander Gordeev is targeting that one for v6.8-rc2 since we had quite a few changes for v6.8-rc1.

Quoted 3 lines

OK, but we can still run these two things in parallel?

Jason

Sure, it's not worse without my patch than what we had before and clearly __iowrite64_copy() has been completely broken for ages without anyone noticing and is fixed by your patches even without my fix for the too strict issue in that at least it then copies what it is supposed to copy even if it does so with 8*8 byte stores.

Thanks,
Niklas

Quoted 23 lines · Jason Gunthorpe

Quoted 22 lines · Mark Rutland

Quoted 10 lines · Jason Gunthorpe

I'm just revising this and I'm wondering if you know why ARM64 has this:

#define __raw_writeq __raw_writeq
static __always_inline void __raw_writeq(u64 val, volatile void __iomem *addr)

{
	asm volatile("str %x0, [%1]" : : "rZ" (val), "r" (addr));
}

Instead of

Quoted 7 lines

#define __raw_writeq __raw_writeq
static __always_inline void __raw_writeq(u64 val, volatile void __iomem *addr)

{
	asm volatile("str %x0, %1" : : "rZ" (val), "m" (*(volatile u64 *)addr));
}

?? Like x86 has.

I believe this is for the same reason as doing so in all of our other IO accessors.

Quoted 7 lines

Quoted 4 lines

We've deliberately ensured that our IO accessors use a single base register with no offset as this is the only form that HW can represent in ESR_ELx.ISS.SRT when reporting a stage-2 abort, which a hypervisor may use for emulating IO.

Wow, harming bare metal performace to accommodate imperfect emulation sounds like a horrible reason :(

Having working functionality everywhere is a very good reason. :)

Quoted 2 lines

So what happens with this patch where IO is done with STP? Are you going to tell me I can't do it because of this?

I'm not personally going to make that judgement, but it's certainly something for Catalin and Will to consider (and I've added Marc in case he has any opinion).

(fixed Marc's email address)

Quoted 37 lines · Mark Rutland

Quoted 23 lines · Jason Gunthorpe

Quoted 22 lines · Mark Rutland

Quoted 10 lines · Jason Gunthorpe

I'm just revising this and I'm wondering if you know why ARM64 has this:

#define __raw_writeq __raw_writeq
static __always_inline void __raw_writeq(u64 val, volatile void __iomem *addr)

{
	asm volatile("str %x0, [%1]" : : "rZ" (val), "r" (addr));
}

Instead of

Quoted 7 lines

#define __raw_writeq __raw_writeq
static __always_inline void __raw_writeq(u64 val, volatile void __iomem *addr)

{
	asm volatile("str %x0, %1" : : "rZ" (val), "m" (*(volatile u64 *)addr));
}

?? Like x86 has.

I believe this is for the same reason as doing so in all of our other IO accessors.

Quoted 7 lines

Quoted 4 lines

We've deliberately ensured that our IO accessors use a single base register with no offset as this is the only form that HW can represent in ESR_ELx.ISS.SRT when reporting a stage-2 abort, which a hypervisor may use for emulating IO.

Wow, harming bare metal performace to accommodate imperfect emulation sounds like a horrible reason :(

Having working functionality everywhere is a very good reason. :)

Quoted 2 lines

So what happens with this patch where IO is done with STP? Are you going to tell me I can't do it because of this?

Quoted 3 lines

I'm not personally going to make that judgement, but it's certainly something for Catalin and Will to consider (and I've added Marc in case he has any opinion).

Good point, I missed this part. We definitely can't use STP in the I/O accessors, we'd have a big surprise when running the same code in a guest with emulated I/O.

If eight STRs without other operations interleaved give us the write-combining on most CPUs (with Normal NC), we should go with this instead of STP.

FWIW, IIUC the immediate-offset forms *without* writeback can still be reported usefully in ESR_ELx,

Quoted 40 lines · Catalin Marinas

(fixed Marc's email address)

Quoted 37 lines · Mark Rutland

Quoted 23 lines · Jason Gunthorpe

Quoted 22 lines · Mark Rutland

Quoted 10 lines · Jason Gunthorpe

I'm just revising this and I'm wondering if you know why ARM64 has this:

#define __raw_writeq __raw_writeq
static __always_inline void __raw_writeq(u64 val, volatile void __iomem *addr)

{
	asm volatile("str %x0, [%1]" : : "rZ" (val), "r" (addr));
}

Instead of

Quoted 7 lines

#define __raw_writeq __raw_writeq
static __always_inline void __raw_writeq(u64 val, volatile void __iomem *addr)

{
	asm volatile("str %x0, %1" : : "rZ" (val), "m" (*(volatile u64 *)addr));
}

?? Like x86 has.

I believe this is for the same reason as doing so in all of our other IO accessors.

Quoted 7 lines

Quoted 4 lines

We've deliberately ensured that our IO accessors use a single base register with no offset as this is the only form that HW can represent in ESR_ELx.ISS.SRT when reporting a stage-2 abort, which a hypervisor may use for emulating IO.

Wow, harming bare metal performace to accommodate imperfect emulation sounds like a horrible reason :(

Having working functionality everywhere is a very good reason. :)

Quoted 2 lines

So what happens with this patch where IO is done with STP? Are you going to tell me I can't do it because of this?

Quoted 7 lines

Quoted 3 lines

I'm not personally going to make that judgement, but it's certainly something for Catalin and Will to consider (and I've added Marc in case he has any opinion).

Good point, I missed this part. We definitely can't use STP in the I/O accessors, we'd have a big surprise when running the same code in a guest with emulated I/O.

Unfortunately there is no hard distinction in KVM/qemu for "emulated IO" and "VFIO MMIO". Even devices using VFIO can get funneled down the emulated path for legitimate reasons.

Again, userspace is already widely deployed using complex IO accessors. ST4 has been out there for years and at this moment this patch with STP is already being deployed in production environments.

Even if you refuse to take STP to mainline it *will* be running in VMs under ARM hypervisors.

What exactly do you think should be done about that?

I thought the guiding mantra here was that any time KVM does not perfectly emulate bare metal it is a bug. "We can't assume all VMs are Linux!". Indeed we recently had some long and *very* theoretical discussions about possible incompatibilties due to kvm changes in the memory attributes thread.

But here it seems to be just shrugging off something so catastrophic as performance IO accessors *that are widely deployed already* don't work reliably in VMs!?!?

"Oh well, don't use them"!?

Damn I hope it crashes the VM and doesn't corrupt the MMIO. I just debugged a x86 KVM issue with it corrupting VFIO MMIO and that was a total nightmare to find.

Quoted 3 lines

If eight STRs without other operations interleaved give us the write-combining on most CPUs (with Normal NC), we should go with this instead of STP.

__iowrite64_copy() is a performance IO accessor, we should not degrade it because buggy hypervisors might exist that have a problem with STP or other instructions. :( :(

Anyhow, I know nothing about whatever this issue is - Mark said:

Quoted 2 lines

FWIW, IIUC the immediate-offset forms *without* writeback can still be reported usefully in ESR_ELx,

Which excludes the post/pre increment forms - but does STP and ST4 also have some kind of problem because the emulation path can't know about wider than a 64 bit access?

What is the plan for ST64B? Don't get to use that either?

Quoted 44 lines · Will Deacon

Quoted 43 lines · Jason Gunthorpe

Quoted 7 lines · Mark Rutland

Quoted 6 lines

I believe this is for the same reason as doing so in all of our other IO accessors.

We've deliberately ensured that our IO accessors use a single base register with no offset as this is the only form that HW can represent in ESR_ELx.ISS.SRT when reporting a stage-2 abort, which a hypervisor may use for emulating IO.

Quoted 8 lines

FWIW, IIUC the immediate-offset forms *without* writeback can still be reported usefully in ESR_ELx, so I believe that we could use the "o" constraint for the __raw_write*() functions, e.g.

static __always_inline void __raw_writeq(u64 val, volatile void __iomem *addr)

{
	asm volatile("str %x0, %1" : : "rZ" (val), "o" (*(volatile u64 *)addr));
}

"o" works well in the same simple memcpy loop:

        add     x2, x1, w2, uxtw 3
        cmp     x1, x2
        bcs     .L1
.L3:
        ldp     x10, x9, [x1]
        ldp     x8, x7, [x1, 16]
        ldp     x6, x5, [x1, 32]
        ldp     x4, x3, [x1, 48]
        str x10, [x0]
        str x9, [x0, 8]
        str x8, [x0, 16]
        str x7, [x0, 24]
        str x6, [x0, 32]
        str x5, [x0, 40]
        str x4, [x0, 48]
        str x3, [x0, 56]
        add     x1, x1, 64
        add     x0, x0, 64
        cmp     x2, x1
        bhi     .L3

.L1:
ret

Quoted 4 lines

Quoted 1 line

Seems intersting to pursue?

I've seen the compiler struggle with plain "o" in the past ("Impossible constraint in asm") so we might want "Qo" if we go down this route.

I'll stick a patch in 0-day and lets see if there are explosions. "Qo" generates the same assembly.

So to summarize:
- We don't like "m" because something about virtualization traps breaks with post/pre indexed forms like:

     str x1, [x0, 8]!

And "m" will allow the compiler to emit that.
- o selects only base register plus offset so it is OK
- Q allows base register only (no offset) on some compilers that won't allow o for 0 offset
- read side stays at 'r' due to an alternates errata workaround requiring ldar which doesn't accept the same effective address as ldr.

(fixed Marc's email address)

Quoted 37 lines · Mark Rutland

Quoted 23 lines · Jason Gunthorpe

Quoted 22 lines · Mark Rutland

Quoted 10 lines · Jason Gunthorpe

I'm just revising this and I'm wondering if you know why ARM64 has this:

#define __raw_writeq __raw_writeq
static __always_inline void __raw_writeq(u64 val, volatile void __iomem *addr)

{
	asm volatile("str %x0, [%1]" : : "rZ" (val), "r" (addr));
}

Instead of

Quoted 7 lines

#define __raw_writeq __raw_writeq
static __always_inline void __raw_writeq(u64 val, volatile void __iomem *addr)

{
	asm volatile("str %x0, %1" : : "rZ" (val), "m" (*(volatile u64 *)addr));
}

?? Like x86 has.

I believe this is for the same reason as doing so in all of our other IO accessors.

Quoted 7 lines

Quoted 4 lines

We've deliberately ensured that our IO accessors use a single base register with no offset as this is the only form that HW can represent in ESR_ELx.ISS.SRT when reporting a stage-2 abort, which a hypervisor may use for emulating IO.

Wow, harming bare metal performace to accommodate imperfect emulation sounds like a horrible reason :(

Having working functionality everywhere is a very good reason. :)

Quoted 2 lines

So what happens with this patch where IO is done with STP? Are you going to tell me I can't do it because of this?

Quoted 3 lines

I'm not personally going to make that judgement, but it's certainly something for Catalin and Will to consider (and I've added Marc in case he has any opinion).

Good point, I missed this part. We definitely can't use STP in the I/O accessors, we'd have a big surprise when running the same code in a guest with emulated I/O.

If eight STRs without other operations interleaved give us the write-combining on most CPUs (with Normal NC), we should go with this instead of STP.

Quoted 41 lines · Catalin Marinas

Quoted 40 lines · Jason Gunthorpe

Quoted 23 lines · Mark Rutland

Quoted 22 lines · Jason Gunthorpe

Quoted 10 lines

I'm just revising this and I'm wondering if you know why ARM64 has this:

#define __raw_writeq __raw_writeq
static __always_inline void __raw_writeq(u64 val, volatile void __iomem *addr)

{
	asm volatile("str %x0, [%1]" : : "rZ" (val), "r" (addr));
}

Instead of

Quoted 7 lines

#define __raw_writeq __raw_writeq
static __always_inline void __raw_writeq(u64 val, volatile void __iomem *addr)

{
	asm volatile("str %x0, %1" : : "rZ" (val), "m" (*(volatile u64 *)addr));
}

?? Like x86 has.

I believe this is for the same reason as doing so in all of our other IO accessors.

Quoted 7 lines

Quoted 4 lines

We've deliberately ensured that our IO accessors use a single base register with no offset as this is the only form that HW can represent in ESR_ELx.ISS.SRT when reporting a stage-2 abort, which a hypervisor may use for emulating IO.

Wow, harming bare metal performace to accommodate imperfect emulation sounds like a horrible reason :(

Having working functionality everywhere is a very good reason. :)

Quoted 2 lines

So what happens with this patch where IO is done with STP? Are you going to tell me I can't do it because of this?

I'm not personally going to make that judgement, but it's certainly something for Catalin and Will to consider (and I've added Marc in case he has any opinion).

Quoted 3 lines

Good point, I missed this part. We definitely can't use STP in the I/O accessors, we'd have a big surprise when running the same code in a guest with emulated I/O.

Just to be clear, that means we should drop this patch ("arm64/io: add memcpy_toio_64") for now, right?

It would be helpful if we could explciitly say so in direct reply to that:

https://lore.kernel.org/linux-arm-kernel/c3ae87aea7660c3d266905c19d10d8de0f9fb779.1700766072.git.leon@kernel.org/

... to avoid any confusion there.

Quoted 3 lines

If eight STRs without other operations interleaved give us the write-combining on most CPUs (with Normal NC), we should go with this instead of STP.

Agreed; I've sent out a patch to allow the offset addressing at:

https://lore.kernel.org/linux-arm-kernel/[email protected]/

... and it should be possible to build atop that to use eight STRs.

Quoted 2 lines

Even if you refuse to take STP to mainline it *will* be running in VMs under ARM hypervisors.

A hypervisor can't do anything with it. If you cared to read the architecture, you'd know by now. So your VM will be either dead, or dog slow, depending on your hypervisor. In any case, I'm sure it will reflect positively on your favourite software.

Quoted 1 line

What exactly do you think should be done about that?

Well, you could use KVM_CAP_ARM_NISV_TO_USER in userspace and see everything slow down. Your call.

Or you can stop whining and try to get better performance out of what we have today.

Quoted 2 lines

Just to be clear, that means we should drop this patch ("arm64/io: add memcpy_toio_64") for now, right?

In its current form yes, but that doesn't mean that memcpy_toio_64() cannot be reworked differently.

Quoted 9 lines

Quoted 3 lines

If eight STRs without other operations interleaved give us the write-combining on most CPUs (with Normal NC), we should go with this instead of STP.

Agreed; I've sent out a patch to allow the offset addressing at:

https://lore.kernel.org/linux-arm-kernel/[email protected]/

... and it should be possible to build atop that to use eight STRs.

That's great, thanks.

Quoted 7 lines · Marc Zyngier

Quoted 2 lines

Even if you refuse to take STP to mainline it *will* be running in VMs under ARM hypervisors.

A hypervisor can't do anything with it. If you cared to read the architecture, you'd know by now. So your VM will be either dead, or dog slow, depending on your hypervisor. In any case, I'm sure it will reflect positively on your favourite software.

"Dog slow" is fine. Forcing IO emulation on paths that shouldn't have it is a VMM problem. KVM & qemu have some issues where this can happen infrequently for VFIO MMIO maps. It is just important that it be functionally correct if you get unlucky. The performance path is to not take a fault in the first place.

Quoted 4 lines

Quoted 1 line

What exactly do you think should be done about that?

Well, you could use KVM_CAP_ARM_NISV_TO_USER in userspace and see everything slow down. Your call.

The issue Mark raised here was that things like STP/etc cannot work in VMs, not that they are slow.

The places we are talking about using the STP pattern are all high performance HW drivers, that do not have any existing SW emulation to worry about. ie the VMM will be using VFIO to back the MMIO the acessors target.

So, I'm fine if the answer is that VMM's using VFIO need to use KVM_CAP_ARM_NISV_TO_USER and do instruction parsing for emulated IO in userspace if they have a design where VFIO MMIO can infrequently generate faults. That is all VMM design stuff and has nothing to do with the kernel.

My objection is this notion we should degrade a performance hot path in drivers to accomodate an ARM VMM issue that should be solved in the VMM.

Quoted 2 lines

Or you can stop whining and try to get better performance out of what we have today.

"better performance"!?!? You are telling me I have to destroy one of our important fast paths for HPC workloads to accommodate some theoretical ARM KVM problem?

Quoted 5 lines

So, I'm fine if the answer is that VMM's using VFIO need to use KVM_CAP_ARM_NISV_TO_USER and do instruction parsing for emulated IO in userspace if they have a design where VFIO MMIO can infrequently generate faults. That is all VMM design stuff and has nothing to do with the kernel.

Which will work a treat with things like CCA, I'm sure.

Quoted 6 lines

Quoted 2 lines

Or you can stop whining and try to get better performance out of what we have today.

"better performance"!?!? You are telling me I have to destroy one of our important fast paths for HPC workloads to accommodate some theoretical ARM KVM problem?

What I'm saying is that there are way to make it better without breaking your particular toy workload which, as important as it may be to *you*, doesn't cover everybody's use case.

Mark did post such an example that has the potential of having that improvement. I'd suggest that you give it a go.

But your attitude of "who cares if it breaks as long as it works for me" is not something I can adhere to.

Quoted 3 lines · Catalin Marinas

Quoted 2 lines

Just to be clear, that means we should drop this patch ("arm64/io: add memcpy_toio_64") for now, right?

Quoted 2 lines

In its current form yes, but that doesn't mean that memcpy_toio_64() cannot be reworked differently.

I gave up on touching memcpy_toio_64(), it doesn't work very well because of the weak alignment

Instead I followed your suggestion to fix __iowrite64_copy()

There are only a couple of places that use this API:

__iowrite64_copy() has a sufficient API that the compiler can inline the STP block as this patch did.

I experimented with having memcpy_toio_64() invoke __iowrite64_copy(), but it did not look very nice. Maybe there is a possible performance win there, I don't know.

Quoted 3 lines · Marc Zyngier

What I'm saying is that there are way to make it better without breaking your particular toy workload which, as important as it may be to *you*, doesn't cover everybody's use case.

Please, do we need the "toy" stuff? The industry is spending 10's of billions of dollars right now to run "my workload". Currently not widely on ARM servers, but we are all hoping ARM can succeed here, right?

I still don't know what you mean by "better". There are several issues now

1) This series, where WC doesn't trigger on new cores. Maybe 8x STR will fix it, but it is not better performance wise than 4x STP.

2) Userspace does ST4 to MMIO memory, and the VMM can't explode because of this. Replacing the ST4 with 8x STR is NOT better, that would be a big performance downside, especially for the quirky hi-silicon hardware.

Quoted 8 lines · Jason Gunthorpe

Quoted 5 lines · Catalin Marinas

Quoted 2 lines

Just to be clear, that means we should drop this patch ("arm64/io: add memcpy_toio_64") for now, right?

In its current form yes, but that doesn't mean that memcpy_toio_64() cannot be reworked differently.

Quoted 4 lines

I gave up on touching memcpy_toio_64(), it doesn't work very well because of the weak alignment

Instead I followed your suggestion to fix __iowrite64_copy()

I forgot the details. Was it to introduce an __iowrite512_copy() function or to simply use __iowrite64_copy() with a count of 8?

Just invoking __iowrite64_copy() with a count of 8 wouldn't work well even if we have the writeq generating STR with an offset (well, it also increments the pointers, so I don't think Mark's optimisation would help). The copy implies loads and these would be interleaved with stores and potentially get in the way of write combining. An __iowrite512_copy() or maybe even an optimised __iowrite64_copy() for count 8 could do the loads first followed by the stores. You can use a special path in __iowrite64_copy() with __builtin_contant_p().

void __iowrite64_copy(void __iomem *to, const void *from,
size_t count)

{
	u64 __iomem *dst = to;
	const u64 *src = from;
	const u64 *end = src + count;

	/*
	 * Try a 64-byte write, the CPUs tend to write-combine them.
	 */
	if (__builtin_contant_p(count) && count == 8) {
		__raw_writeq(*src, dst);
		__raw_writeq(*(src + 1), dst + 1);

		__raw_writeq(*(src + 2), dst + 2);
		__raw_writeq(*(src + 3), dst + 3);
		__raw_writeq(*(src + 4), dst + 4);
		__raw_writeq(*(src + 5), dst + 5);
		__raw_writeq(*(src + 6), dst + 6);
		__raw_writeq(*(src + 7), dst + 7);
		return;
	}

What we don't have is inlining of __iowrite64_copy() but if we need that we can move away from a weak symbol to a static inline.

Give this a go and see if it you get write-combining in your hardware. If the loads interleaves with stores get in the way, maybe we can resort to inline asm.

Quoted 13 lines · Jason Gunthorpe

Quoted 3 lines · Marc Zyngier

What I'm saying is that there are way to make it better without breaking your particular toy workload which, as important as it may be to *you*, doesn't cover everybody's use case.

Please, do we need the "toy" stuff? The industry is spending 10's of billions of dollars right now to run "my workload". Currently not widely on ARM servers, but we are all hoping ARM can succeed here, right?

I still don't know what you mean by "better". There are several issues now

Quoted 2 lines

1) This series, where WC doesn't trigger on new cores. Maybe 8x STR will fix it, but it is not better performance wise than 4x STP.

It would be good to know. If the performance difference is significant, we can revisit. I'm not keen on using alternatives here without backing it up by numbers (do we even have a way to detect whether Linux is running natively or not? we may have to invent something).

Quoted 4 lines

2) Userspace does ST4 to MMIO memory, and the VMM can't explode because of this. Replacing the ST4 with 8x STR is NOT better, that would be a big performance downside, especially for the quirky hi-silicon hardware.

I was hoping KVM injects an error into the guest rather than killing it but at a quick look I couldn't find it. The kvm_handle_guest_abort() -> io_mem_abort() ends up returning -ENOSYS while handle_trap_exceptions() only understands handled or not (like 1 or 0). Well, maybe I didn't look deep enough.

The suggestion that it should not have any interleaving instructions and use STP came from our CPU architecture team.

Quoted 2 lines · Jason Gunthorpe

The suggestion that it should not have any interleaving instructions and use STP came from our CPU architecture team.

I got some more details here.

They point to the ARM publication about write combining

https://community.arm.com/cfs-file/__key/telligent-evolution-components-attachments/13-150-00-00-00-00-10-12/Understanding_5F00_Write_5F00_Combining_5F00_on_5F00_Arm_5F00_V.1.0.pdf

specifically to the example code using 4x 128 bit NEON stores.

They point at the actual CPU design and say it is optimized for 128 bit stores (STP and ST4 included, it seems).

64 bit stores trigger some different behavior.

I have no way to know if it will be OK for other drivers that expect this to be a performance path in the kernel.

Are you *sure* you want to do this str version? If it works for mlx5 I will send the patch and the other companies can come later with performance data.

Quoted 6 lines · Jason Gunthorpe

Quoted 2 lines · Jason Gunthorpe

The suggestion that it should not have any interleaving instructions and use STP came from our CPU architecture team.

I got some more details here.

Quoted 5 lines

They point to the ARM publication about write combining

https://community.arm.com/cfs-file/__key/telligent-evolution-components-attachments/13-150-00-00-00-00-10-12/Understanding_5F00_Write_5F00_Combining_5F00_on_5F00_Arm_5F00_V.1.0.pdf

specifically to the example code using 4x 128 bit NEON stores.

That's an example but this document doesn't make any statements about 64-bit writes.

Quoted 11 lines · Catalin Marinas

Quoted 4 lines · Jason Gunthorpe

2) Userspace does ST4 to MMIO memory, and the VMM can't explode because of this. Replacing the ST4 with 8x STR is NOT better, that would be a big performance downside, especially for the quirky hi-silicon hardware.

I was hoping KVM injects an error into the guest rather than killing it but at a quick look I couldn't find it. The kvm_handle_guest_abort() -> io_mem_abort() ends up returning -ENOSYS while handle_trap_exceptions() only understands handled or not (like 1 or 0). Well, maybe I didn't look deep enough.

It looks to me like qemu turns on the KVM_CAP_ARM_NISV_TO_USER and then when it gets a NISV it always converts it to a data abort to the guest. See kvm_arm_handle_dabt_nisv() in qemu. So it is just a correctness issue, not a 'VM userspace can crash the VMM' security problem.

Thus, IMHO, doing IO emulation for VFIO that doesn't support all the instructions actual existing SW uses to do IO is hard to justify. We are already on a slow path that only exists for technical correctness, it should be perfect. It is perfect on x86 because x86 KVM does SW instruction decode and emulation. ARM could too, but doesn't.

Quoted 5 lines

It looks to me like qemu turns on the KVM_CAP_ARM_NISV_TO_USER and then when it gets a NISV it always converts it to a data abort to the guest. See kvm_arm_handle_dabt_nisv() in qemu. So it is just a correctness issue, not a 'VM userspace can crash the VMM' security problem.

The VMM wasn't my concern but rather a guest getting killed or not functioning correctly (user app killed).

Quoted 5 lines

Thus, IMHO, doing IO emulation for VFIO that doesn't support all the instructions actual existing SW uses to do IO is hard to justify. We are already on a slow path that only exists for technical correctness, it should be perfect. It is perfect on x86 because x86 KVM does SW instruction decode and emulation. ARM could too, but doesn't.

It could fall back to instruction decode, either in KVM or the VMM (strong preference for the latter), but I'd only do this if it's justified.

I don't think the issue here is VFIO, I doubt we'd ever see emulation for hardware like mlx5.

But we are changing generic kernel functions
like memcpy_toio/__iowrite64_copy() that end up being used in other drivers (e.g. USB, UART) for emulated devices.

Quoted 9 lines · Jason Gunthorpe

Quoted 2 lines · Niklas Schnelle

Yes, we need zpci_memcpy_toio(to, from, count * 8) since our count is in bytes like for memcpy_toio().

https://github.com/jgunthorpe/linux/commits/mlx5_wc/

Jason

Thanks, the s390 patches:

s390: Implement __iowrite32_copy()
s390: Use the correct count for __iowrite64_copy()
s390: Stop using weak symbols for __iowrite64_copy()

Look good to me. I.e. you may add my.

Acked-by Niklas Schnelle <[email protected]>

I did test your patches too and by accident confirmed again that these do need commit 80df7d6af7f6 ("s390/pci: fix max size calculation in zpci_memcpy_toio()") from the s390 feature branch to get the mlx5 driver to detect Write-Combining as supported. Note, as far as I know Alexander Gordeev is targeting that one for v6.8-rc2 since we had quite a few changes for v6.8-rc1.

Thanks,
Niklas

Quoted 20 lines · Niklas Schnelle

Quoted 9 lines · Jason Gunthorpe

Quoted 2 lines · Niklas Schnelle

Yes, we need zpci_memcpy_toio(to, from, count * 8) since our count is in bytes like for memcpy_toio().

https://github.com/jgunthorpe/linux/commits/mlx5_wc/

Jason

Thanks, the s390 patches:

s390: Implement __iowrite32_copy()
s390: Use the correct count for __iowrite64_copy()
s390: Stop using weak symbols for __iowrite64_copy()

Look good to me. I.e. you may add my.

Quoted 8 lines

Acked-by Niklas Schnelle <[email protected]>

I did test your patches too and by accident confirmed again that these do need commit 80df7d6af7f6 ("s390/pci: fix max size calculation in zpci_memcpy_toio()") from the s390 feature branch to get the mlx5 driver to detect Write-Combining as supported. Note, as far as I know Alexander Gordeev is targeting that one for v6.8-rc2 since we had quite a few changes for v6.8-rc1.

Since I haven't seen a new version of this yet I was just wondering what the current plan is. If this is somewhat stuck would you be willing to send the "s390: Use the correct count for __iowrite64_copy()" patch separately, preferably with "s390/pci:" prefix?