[PATCH v4 0/3] vfio/pci: Support 8-byte PCI loads and stores

[PATCH v4 0/3] vfio/pci: Support 8-byte PCI loads and stores

[Gerd Bayer]

Hi all,

this all started with a single patch by Ben to enable writing a user-mode
driver for a PCI device that requires 64bit register read/writes on s390.
A quick grep showed that there are several other drivers for PCI devices
in the kernel that use readq/writeq and eventually could use this, too.
So we decided to propose this for general inclusion.

A couple of suggestions for refactorizations by Jason Gunthorpe and Alex
Williamson later [1], I arrived at this little series that avoids some
code duplication in vfio_pci_core_do_io_rw().
Also, I've added a small patch to correct the spelling in one of the
declaration macros that was suggested by Ramesh Thomas [2].

This version was tested with a pass-through PCI device in a KVM guest
and with explicit test reads of size 8, 16, 32, and 64 bit on
s390. For 32bit architectures this has only been compile tested for the
32bit ARM architecture.

Thank you,
Gerd Bayer

[1] https://lore.kernel.org/all/20240422153508.2355844-1-gbayer@linux.ibm.com/
[2] https://lore.kernel.org/kvm/20240425165604.899447-1-gbayer@linux.ibm.com/T/#m1b51fe155c60d04313695fbee11a2ccea856a98c

Changes v3 -> v4:
- Make 64-bit accessors depend on CONFIG_64BIT (for x86, too).
- Drop conversion of if-else if chain to switch-case.
- Add patch to fix spelling of declaration macro.

Changes v2 -> v3:
- Introduce macro to generate body of different-size accesses in
  vfio_pci_core_do_io_rw (courtesy Alex Williamson).
- Convert if-else if chain to a switch-case construct to better
  accommodate conditional compiles.

Changes v1 -> v2:
- On non 64bit architecture use at most 32bit accesses in
  vfio_pci_core_do_io_rw and describe that in the commit message.
- Drop the run-time error on 32bit architectures.
- The #endif splitting the "else if" is not really fortunate, but I'm
  open to suggestions.


Ben Segal (1):
  vfio/pci: Support 8-byte PCI loads and stores

Gerd Bayer (2):
  vfio/pci: Extract duplicated code into macro
  vfio/pci: Fix typo in macro to declare accessors

 drivers/vfio/pci/vfio_pci_rdwr.c | 124 ++++++++++++++++---------------
 include/linux/vfio_pci_core.h    |  27 ++++---
 2 files changed, 78 insertions(+), 73 deletions(-)

-- 
2.45.0

[PATCH v4 1/3] vfio/pci: Extract duplicated code into macro

[Gerd Bayer]

vfio_pci_core_do_io_rw() repeats the same code for multiple access
widths. Factor this out into a macro

Suggested-by: Alex Williamson <alex.williamson@redhat.com>
Signed-off-by: Gerd Bayer <gbayer@linux.ibm.com>
---
 drivers/vfio/pci/vfio_pci_rdwr.c | 106 ++++++++++++++-----------------
 1 file changed, 46 insertions(+), 60 deletions(-)

diff --git a/drivers/vfio/pci/vfio_pci_rdwr.c b/drivers/vfio/pci/vfio_pci_rdwr.c
index 03b8f7ada1ac..d07bfb0ab892 100644
--- a/drivers/vfio/pci/vfio_pci_rdwr.c
+++ b/drivers/vfio/pci/vfio_pci_rdwr.c
@@ -90,6 +90,40 @@ VFIO_IOREAD(8)
 VFIO_IOREAD(16)
 VFIO_IOREAD(32)
 
+#define VFIO_IORDWR(size)						\
+static int vfio_pci_iordwr##size(struct vfio_pci_core_device *vdev,\
+				bool iswrite, bool test_mem,		\
+				void __iomem *io, char __user *buf,	\
+				loff_t off, size_t *filled)		\
+{									\
+	u##size val;							\
+	int ret;							\
+									\
+	if (iswrite) {							\
+		if (copy_from_user(&val, buf, sizeof(val)))		\
+			return -EFAULT;					\
+									\
+		ret = vfio_pci_core_iowrite##size(vdev, test_mem,	\
+						  val, io + off);	\
+		if (ret)						\
+			return ret;					\
+	} else {							\
+		ret = vfio_pci_core_ioread##size(vdev, test_mem,	\
+						 &val, io + off);	\
+		if (ret)						\
+			return ret;					\
+									\
+		if (copy_to_user(buf, &val, sizeof(val)))		\
+			return -EFAULT;					\
+	}								\
+									\
+	*filled = sizeof(val);						\
+	return 0;							\
+}									\
+
+VFIO_IORDWR(8)
+VFIO_IORDWR(16)
+VFIO_IORDWR(32)
 /*
  * Read or write from an __iomem region (MMIO or I/O port) with an excluded
  * range which is inaccessible.  The excluded range drops writes and fills
@@ -115,71 +149,23 @@ ssize_t vfio_pci_core_do_io_rw(struct vfio_pci_core_device *vdev, bool test_mem,
 			fillable = 0;
 
 		if (fillable >= 4 && !(off % 4)) {
-			u32 val;
-
-			if (iswrite) {
-				if (copy_from_user(&val, buf, 4))
-					return -EFAULT;
-
-				ret = vfio_pci_core_iowrite32(vdev, test_mem,
-							      val, io + off);
-				if (ret)
-					return ret;
-			} else {
-				ret = vfio_pci_core_ioread32(vdev, test_mem,
-							     &val, io + off);
-				if (ret)
-					return ret;
-
-				if (copy_to_user(buf, &val, 4))
-					return -EFAULT;
-			}
+			ret = vfio_pci_iordwr32(vdev, iswrite, test_mem,
+						io, buf, off, &filled);
+			if (ret)
+				return ret;
 
-			filled = 4;
 		} else if (fillable >= 2 && !(off % 2)) {
-			u16 val;
-
-			if (iswrite) {
-				if (copy_from_user(&val, buf, 2))
-					return -EFAULT;
-
-				ret = vfio_pci_core_iowrite16(vdev, test_mem,
-							      val, io + off);
-				if (ret)
-					return ret;
-			} else {
-				ret = vfio_pci_core_ioread16(vdev, test_mem,
-							     &val, io + off);
-				if (ret)
-					return ret;
-
-				if (copy_to_user(buf, &val, 2))
-					return -EFAULT;
-			}
+			ret = vfio_pci_iordwr16(vdev, iswrite, test_mem,
+						io, buf, off, &filled);
+			if (ret)
+				return ret;
 
-			filled = 2;
 		} else if (fillable) {
-			u8 val;
-
-			if (iswrite) {
-				if (copy_from_user(&val, buf, 1))
-					return -EFAULT;
-
-				ret = vfio_pci_core_iowrite8(vdev, test_mem,
-							     val, io + off);
-				if (ret)
-					return ret;
-			} else {
-				ret = vfio_pci_core_ioread8(vdev, test_mem,
-							    &val, io + off);
-				if (ret)
-					return ret;
-
-				if (copy_to_user(buf, &val, 1))
-					return -EFAULT;
-			}
+			ret = vfio_pci_iordwr8(vdev, iswrite, test_mem,
+					       io, buf, off, &filled);
+			if (ret)
+				return ret;
 
-			filled = 1;
 		} else {
 			/* Fill reads with -1, drop writes */
 			filled = min(count, (size_t)(x_end - off));
-- 
2.45.0

[PATCH v4 2/3] vfio/pci: Support 8-byte PCI loads and stores

[Gerd Bayer]

From: Ben Segal <bpsegal@us.ibm.com>

Many PCI adapters can benefit or even require full 64bit read
and write access to their registers. In order to enable work on
user-space drivers for these devices add two new variations
vfio_pci_core_io{read|write}64 of the existing access methods
when the architecture supports 64-bit ioreads and iowrites.

Signed-off-by: Ben Segal <bpsegal@us.ibm.com>
Co-developed-by: Gerd Bayer <gbayer@linux.ibm.com>
Signed-off-by: Gerd Bayer <gbayer@linux.ibm.com>
---
 drivers/vfio/pci/vfio_pci_rdwr.c | 18 +++++++++++++++++-
 include/linux/vfio_pci_core.h    |  5 ++++-
 2 files changed, 21 insertions(+), 2 deletions(-)

diff --git a/drivers/vfio/pci/vfio_pci_rdwr.c b/drivers/vfio/pci/vfio_pci_rdwr.c
index d07bfb0ab892..07351ea76604 100644
--- a/drivers/vfio/pci/vfio_pci_rdwr.c
+++ b/drivers/vfio/pci/vfio_pci_rdwr.c
@@ -61,7 +61,7 @@ EXPORT_SYMBOL_GPL(vfio_pci_core_iowrite##size);
 VFIO_IOWRITE(8)
 VFIO_IOWRITE(16)
 VFIO_IOWRITE(32)
-#ifdef iowrite64
+#ifdef CONFIG_64BIT
 VFIO_IOWRITE(64)
 #endif
 
@@ -89,6 +89,9 @@ EXPORT_SYMBOL_GPL(vfio_pci_core_ioread##size);
 VFIO_IOREAD(8)
 VFIO_IOREAD(16)
 VFIO_IOREAD(32)
+#ifdef CONFIG_64BIT
+VFIO_IOREAD(64)
+#endif
 
 #define VFIO_IORDWR(size)						\
 static int vfio_pci_iordwr##size(struct vfio_pci_core_device *vdev,\
@@ -124,6 +127,10 @@ static int vfio_pci_iordwr##size(struct vfio_pci_core_device *vdev,\
 VFIO_IORDWR(8)
 VFIO_IORDWR(16)
 VFIO_IORDWR(32)
+#if CONFIG_64BIT
+VFIO_IORDWR(64)
+#endif
+
 /*
  * Read or write from an __iomem region (MMIO or I/O port) with an excluded
  * range which is inaccessible.  The excluded range drops writes and fills
@@ -148,6 +155,15 @@ ssize_t vfio_pci_core_do_io_rw(struct vfio_pci_core_device *vdev, bool test_mem,
 		else
 			fillable = 0;
 
+#if CONFIG_64BIT
+		if (fillable >= 8 && !(off % 8)) {
+			ret = vfio_pci_iordwr64(vdev, iswrite, test_mem,
+						io, buf, off, &filled);
+			if (ret)
+				return ret;
+
+		} else
+#endif
 		if (fillable >= 4 && !(off % 4)) {
 			ret = vfio_pci_iordwr32(vdev, iswrite, test_mem,
 						io, buf, off, &filled);
diff --git a/include/linux/vfio_pci_core.h b/include/linux/vfio_pci_core.h
index a2c8b8bba711..5f9b02d4a3e9 100644
--- a/include/linux/vfio_pci_core.h
+++ b/include/linux/vfio_pci_core.h
@@ -146,7 +146,7 @@ int vfio_pci_core_iowrite##size(struct vfio_pci_core_device *vdev,	\
 VFIO_IOWRITE_DECLATION(8)
 VFIO_IOWRITE_DECLATION(16)
 VFIO_IOWRITE_DECLATION(32)
-#ifdef iowrite64
+#ifdef CONFIG_64BIT
 VFIO_IOWRITE_DECLATION(64)
 #endif
 
@@ -157,5 +157,8 @@ int vfio_pci_core_ioread##size(struct vfio_pci_core_device *vdev,	\
 VFIO_IOREAD_DECLATION(8)
 VFIO_IOREAD_DECLATION(16)
 VFIO_IOREAD_DECLATION(32)
+#ifdef CONFIG_64BIT
+VFIO_IOREAD_DECLATION(64)
+#endif
 
 #endif /* VFIO_PCI_CORE_H */
-- 
2.45.0

Re: [PATCH v4 2/3] vfio/pci: Support 8-byte PCI loads and stores

[Ramesh Thomas]

The removal of the check for iowrite64 and ioread64 causes build error 
because those macros don't get defined anywhere if CONFIG_GENERIC_IOMAP 
is not defined. However, I do think the removal of the checks is correct.

It is better to include linux/io-64-nonatomic-lo-hi.h which define those 
macros mapping to generic implementations in lib/iomap.c. If the 
architecture does not implement 64 bit rw functions (readq/writeq), then 
it does 32 bit back to back. I have sent a patch with the change that 
includes the above header file. Please review and include in this patch 
series if ok.

Thanks,
Ramesh

On 5/22/2024 8:06 AM, Gerd Bayer wrote:
> From: Ben Segal <bpsegal@us.ibm.com>
> 
> Many PCI adapters can benefit or even require full 64bit read
> and write access to their registers. In order to enable work on
> user-space drivers for these devices add two new variations
> vfio_pci_core_io{read|write}64 of the existing access methods
> when the architecture supports 64-bit ioreads and iowrites.
> 
> Signed-off-by: Ben Segal <bpsegal@us.ibm.com>
> Co-developed-by: Gerd Bayer <gbayer@linux.ibm.com>
> Signed-off-by: Gerd Bayer <gbayer@linux.ibm.com>
> ---
>   drivers/vfio/pci/vfio_pci_rdwr.c | 18 +++++++++++++++++-
>   include/linux/vfio_pci_core.h    |  5 ++++-
>   2 files changed, 21 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/vfio/pci/vfio_pci_rdwr.c b/drivers/vfio/pci/vfio_pci_rdwr.c
> index d07bfb0ab892..07351ea76604 100644
> --- a/drivers/vfio/pci/vfio_pci_rdwr.c
> +++ b/drivers/vfio/pci/vfio_pci_rdwr.c
> @@ -61,7 +61,7 @@ EXPORT_SYMBOL_GPL(vfio_pci_core_iowrite##size);
>   VFIO_IOWRITE(8)
>   VFIO_IOWRITE(16)
>   VFIO_IOWRITE(32)
> -#ifdef iowrite64
> +#ifdef CONFIG_64BIT
>   VFIO_IOWRITE(64)
>   #endif
>   
> @@ -89,6 +89,9 @@ EXPORT_SYMBOL_GPL(vfio_pci_core_ioread##size);
>   VFIO_IOREAD(8)
>   VFIO_IOREAD(16)
>   VFIO_IOREAD(32)
> +#ifdef CONFIG_64BIT
> +VFIO_IOREAD(64)
> +#endif
>   
>   #define VFIO_IORDWR(size)						\
>   static int vfio_pci_iordwr##size(struct vfio_pci_core_device *vdev,\
> @@ -124,6 +127,10 @@ static int vfio_pci_iordwr##size(struct vfio_pci_core_device *vdev,\
>   VFIO_IORDWR(8)
>   VFIO_IORDWR(16)
>   VFIO_IORDWR(32)
> +#if CONFIG_64BIT
> +VFIO_IORDWR(64)
> +#endif
> +
>   /*
>    * Read or write from an __iomem region (MMIO or I/O port) with an excluded
>    * range which is inaccessible.  The excluded range drops writes and fills
> @@ -148,6 +155,15 @@ ssize_t vfio_pci_core_do_io_rw(struct vfio_pci_core_device *vdev, bool test_mem,
>   		else
>   			fillable = 0;
>   
> +#if CONFIG_64BIT
> +		if (fillable >= 8 && !(off % 8)) {
> +			ret = vfio_pci_iordwr64(vdev, iswrite, test_mem,
> +						io, buf, off, &filled);
> +			if (ret)
> +				return ret;
> +
> +		} else
> +#endif
>   		if (fillable >= 4 && !(off % 4)) {
>   			ret = vfio_pci_iordwr32(vdev, iswrite, test_mem,
>   						io, buf, off, &filled);
> diff --git a/include/linux/vfio_pci_core.h b/include/linux/vfio_pci_core.h
> index a2c8b8bba711..5f9b02d4a3e9 100644
> --- a/include/linux/vfio_pci_core.h
> +++ b/include/linux/vfio_pci_core.h
> @@ -146,7 +146,7 @@ int vfio_pci_core_iowrite##size(struct vfio_pci_core_device *vdev,	\
>   VFIO_IOWRITE_DECLATION(8)
>   VFIO_IOWRITE_DECLATION(16)
>   VFIO_IOWRITE_DECLATION(32)
> -#ifdef iowrite64
> +#ifdef CONFIG_64BIT
>   VFIO_IOWRITE_DECLATION(64)
>   #endif
>   
> @@ -157,5 +157,8 @@ int vfio_pci_core_ioread##size(struct vfio_pci_core_device *vdev,	\
>   VFIO_IOREAD_DECLATION(8)
>   VFIO_IOREAD_DECLATION(16)
>   VFIO_IOREAD_DECLATION(32)
> +#ifdef CONFIG_64BIT
> +VFIO_IOREAD_DECLATION(64)
> +#endif
>   
>   #endif /* VFIO_PCI_CORE_H */

Re: [PATCH v4 2/3] vfio/pci: Support 8-byte PCI loads and stores

[Gerd Bayer]

Hi Ramesh,

On Wed, 2024-05-22 at 16:38 -0700, Ramesh Thomas wrote:
> The removal of the check for iowrite64 and ioread64 causes build
> error because those macros don't get defined anywhere if
> CONFIG_GENERIC_IOMAP is not defined. However, I do think the removal
> of the checks is correct.

Wait, I believe it is the other way around. If your config *is*
specifying CONFIG_GENERIC_IOMAP, lib/iomap.c will provide
implementations for back-to-back 32bit operations to emulate 64bit
accesses - and you have to "select" which of the two types of emulation
(hi/lo or lo/hi order) get mapped onto ioread64(be) or iowrite64(be) by
including linux/io-64-nonatomic-lo-hi.h (or -hi-lo.h).

> It is better to include linux/io-64-nonatomic-lo-hi.h which define
> those macros mapping to generic implementations in lib/iomap.c. If
> the architecture does not implement 64 bit rw functions
> (readq/writeq), then  it does 32 bit back to back. I have sent a
> patch with the change that includes the above header file. Please
> review and include in this patch series if ok.

I did find your patch, thank you. I had a very hard time to find a
kernel config that actually showed the unresolved symbols situation:
Some 64bit MIPS config, that relied on GENERIC_IOMAP. And with your
patch applied, I could compile successfully.
Do you have an easier way steer a kernel config into this dead-end?

> Thanks,
> Ramesh

Frankly, I'd rather not make any assumptions in this rather generic
vfio/pci layer about whether hi-lo or lo-hi is the right order to
emulate a 64bit access when the base architecture does not support
64bit accesses naturally. So, if CONFIG_64BIT is no guarantee that
there's a definitive implementation of ioread64/iowrite64, I'd rather
revert to make the conditional compiles depend on those definitions.

But maybe Alex has an opinion on this, too?

Thanks,
Gerd

Re: [PATCH v4 2/3] vfio/pci: Support 8-byte PCI loads and stores

[Ramesh Thomas]

On 5/23/2024 8:01 AM, Gerd Bayer wrote:
> Hi Ramesh,
> 
> On Wed, 2024-05-22 at 16:38 -0700, Ramesh Thomas wrote:
>> The removal of the check for iowrite64 and ioread64 causes build
>> error because those macros don't get defined anywhere if
>> CONFIG_GENERIC_IOMAP is not defined. However, I do think the removal
>> of the checks is correct.
> 
> Wait, I believe it is the other way around. If your config *is*
> specifying CONFIG_GENERIC_IOMAP, lib/iomap.c will provide
> implementations for back-to-back 32bit operations to emulate 64bit
> accesses - and you have to "select" which of the two types of emulation
> (hi/lo or lo/hi order) get mapped onto ioread64(be) or iowrite64(be) by
> including linux/io-64-nonatomic-lo-hi.h (or -hi-lo.h).

Sorry, yes I meant to write they don't get defined anywhere in your code 
path if CONFIG_GENERIC_IOMAP *is defined*. The only place in your code 
path where iowrit64 and ioread64 get defined is in asm/io.h. Those 
definitions are surrounded by #ifndef CONFIG_GENERIC_IOMAP. 
CONFIG_GENERIC_IOMAP gets defined for x86.

> 
>> It is better to include linux/io-64-nonatomic-lo-hi.h which define
>> those macros mapping to generic implementations in lib/iomap.c. If
>> the architecture does not implement 64 bit rw functions
>> (readq/writeq), then  it does 32 bit back to back. I have sent a
>> patch with the change that includes the above header file. Please
>> review and include in this patch series if ok.
> 
> I did find your patch, thank you. I had a very hard time to find a
> kernel config that actually showed the unresolved symbols situation:
> Some 64bit MIPS config, that relied on GENERIC_IOMAP. And with your
> patch applied, I could compile successfully.
> Do you have an easier way steer a kernel config into this dead-end?

The generic implementation takes care of all conditions. I guess some 
build bot would report error on build failures. But checks like #ifdef 
iowrite64 would hide the missing definitions error.

> 
>> Thanks,
>> Ramesh
> 
> Frankly, I'd rather not make any assumptions in this rather generic
> vfio/pci layer about whether hi-lo or lo-hi is the right order to > emulate a 64bit access when the base architecture does not support
> 64bit accesses naturally. So, if CONFIG_64BIT is no guarantee that
> there's a definitive implementation of ioread64/iowrite64, I'd rather

There is already an assumption of the order in the current 
implementation regardless e.g. vfio_pci_core_do_io_rw(). If there is no 
iowrite64 found, the code does back to back 34 bit writes without 
checking for any particular order requirements.

io-64-nonatomic-lo-hi.h and io-64-nonatomic-hi-lo.h would define 
ioread64/iowrite64 only if they are not already defined in asm/io.h.

Also since there is a check for CONFIG_64BIT, most likely a 64 bit 
readq/writeq will get used in the lib/iomap.c implementations. I think 
we can pick either lo-hi or hi-lo for the unlikely 32 bit fall through 
when CONFIG_64BIT is defined.


> revert to make the conditional compiles depend on those definitions.
> 
> But maybe Alex has an opinion on this, too?
> 
> Thanks,
> Gerd
> 
> 

Re: [PATCH v4 2/3] vfio/pci: Support 8-byte PCI loads and stores

[Gerd Bayer]

On Thu, 2024-05-23 at 14:47 -0700, Ramesh Thomas wrote:
> On 5/23/2024 8:01 AM, Gerd Bayer wrote:
> > Hi Ramesh,
> > 
> > On Wed, 2024-05-22 at 16:38 -0700, Ramesh Thomas wrote:
> > > The removal of the check for iowrite64 and ioread64 causes build
> > > error because those macros don't get defined anywhere if
> > > CONFIG_GENERIC_IOMAP is not defined. However, I do think the
> > > removal of the checks is correct.
> > 
> > Wait, I believe it is the other way around. If your config *is*
> > specifying CONFIG_GENERIC_IOMAP, lib/iomap.c will provide
> > implementations for back-to-back 32bit operations to emulate 64bit
> > accesses - and you have to "select" which of the two types of
> > emulation (hi/lo or lo/hi order) get mapped onto ioread64(be) or
> > iowrite64(be) by including linux/io-64-nonatomic-lo-hi.h (or -hi-
> > lo.h).
> 
> Sorry, yes I meant to write they don't get defined anywhere in your
> code path if CONFIG_GENERIC_IOMAP *is defined*. The only place in
> your code path where iowrit64 and ioread64 get defined is in
> asm/io.h. Those definitions are surrounded by #ifndef
> CONFIG_GENERIC_IOMAP. CONFIG_GENERIC_IOMAP gets defined for x86.

Now I got it - I think. And I see that plain x86 is aleady affected by
this issue.

> > > It is better to include linux/io-64-nonatomic-lo-hi.h which
> > > define those macros mapping to generic implementations in
> > > lib/iomap.c.
> > > If the architecture does not implement 64 bit rw functions
> > > (readq/writeq), then  it does 32 bit back to back. I have sent a
> > > patch with the change that includes the above header file. Please
> > > review and include in this patch series if ok.
> > 
> > I did find your patch, thank you. I had a very hard time to find a
> > kernel config that actually showed the unresolved symbols
> > situation:
> > Some 64bit MIPS config, that relied on GENERIC_IOMAP. And with your
> > patch applied, I could compile successfully.
> > Do you have an easier way steer a kernel config into this dead-end?
> 
> The generic implementation takes care of all conditions. I guess some
> build bot would report error on build failures. But checks like
> #ifdef iowrite64 would hide the missing definitions error.

Yes definitely, we need to avoid this.

> > 
> > > Thanks,
> > > Ramesh
> > 
> > Frankly, I'd rather not make any assumptions in this rather generic
> > vfio/pci layer about whether hi-lo or lo-hi is the right order to >
> > emulate a 64bit access when the base architecture does not support
> > 64bit accesses naturally. So, if CONFIG_64BIT is no guarantee that
> > there's a definitive implementation of ioread64/iowrite64, I'd
> > rather
> 
> There is already an assumption of the order in the current 
> implementation regardless e.g. vfio_pci_core_do_io_rw(). If there is
> no iowrite64 found, the code does back to back 34 bit writes without 
> checking for any particular order requirements.
> 
> io-64-nonatomic-lo-hi.h and io-64-nonatomic-hi-lo.h would define 
> ioread64/iowrite64 only if they are not already defined in asm/io.h.
> 
> Also since there is a check for CONFIG_64BIT, most likely a 64 bit 
> readq/writeq will get used in the lib/iomap.c implementations. I
> think we can pick either lo-hi or hi-lo for the unlikely 32 bit fall
> through when CONFIG_64BIT is defined.

I dug into lib/iomap.c some more today and I see your point, that it is
desireable to make the 64bit accessors useable through vfio/pci when
they're implemented in lib/iomap.c. And I follow your argument that in
most cases these will map onto readq/writeq - only programmed IO (PIO)
has to emulate this with 2 32bit back-to-back accesses.

If only the code in lib/iomap.c was structured differently - and made
readq/writeq available under ioread64/iowrite64 proper and only fell
back to the nonatomic hi-lo or lo-hi emulation with 32bit accesses if
PIO is used.

As much as I'd like to have it differently, it seems like it was a
lengthy process to have that change accepted at the time:
https://lore.kernel.org/all/20181106205234.25792-1-logang@deltatee.com/

I'm not sure if we can clean that up, easily. Plus there are appear to
be plenty of users of io-64-nonatomic-{lo-hi|-hi-lo}.h in tree already
- 103 and 18, resp.

> > revert to make the conditional compiles depend on those
> > definitions. But maybe Alex has an opinion on this, too?
> > 
> > Thanks,
> > Gerd

So I'd like to hear from Alex and Tian (who was not a big fan) if we
should support 64bit accessors in vfio/pci (primarily) on x86 with this
series, or not at all, or split that work off, maybe?

Thanks,
Gerd

Re: [PATCH v4 2/3] vfio/pci: Support 8-byte PCI loads and stores

[Ramesh Thomas]

On 5/24/2024 6:42 AM, Gerd Bayer wrote:
> On Thu, 2024-05-23 at 14:47 -0700, Ramesh Thomas wrote:
>> On 5/23/2024 8:01 AM, Gerd Bayer wrote:
>>> Hi Ramesh,
>>>
>>> On Wed, 2024-05-22 at 16:38 -0700, Ramesh Thomas wrote:
>>>> The removal of the check for iowrite64 and ioread64 causes build
>>>> error because those macros don't get defined anywhere if
>>>> CONFIG_GENERIC_IOMAP is not defined. However, I do think the
>>>> removal of the checks is correct.
>>>
>>> Wait, I believe it is the other way around. If your config *is*
>>> specifying CONFIG_GENERIC_IOMAP, lib/iomap.c will provide
>>> implementations for back-to-back 32bit operations to emulate 64bit
>>> accesses - and you have to "select" which of the two types of
>>> emulation (hi/lo or lo/hi order) get mapped onto ioread64(be) or
>>> iowrite64(be) by including linux/io-64-nonatomic-lo-hi.h (or -hi-
>>> lo.h).
>>
>> Sorry, yes I meant to write they don't get defined anywhere in your
>> code path if CONFIG_GENERIC_IOMAP *is defined*. The only place in
>> your code path where iowrit64 and ioread64 get defined is in
>> asm/io.h. Those definitions are surrounded by #ifndef
>> CONFIG_GENERIC_IOMAP. CONFIG_GENERIC_IOMAP gets defined for x86.
> 
> Now I got it - I think. And I see that plain x86 is aleady affected by
> this issue.
> 
>>>> It is better to include linux/io-64-nonatomic-lo-hi.h which
>>>> define those macros mapping to generic implementations in
>>>> lib/iomap.c.
>>>> If the architecture does not implement 64 bit rw functions
>>>> (readq/writeq), then  it does 32 bit back to back. I have sent a
>>>> patch with the change that includes the above header file. Please
>>>> review and include in this patch series if ok.
>>>
>>> I did find your patch, thank you. I had a very hard time to find a
>>> kernel config that actually showed the unresolved symbols
>>> situation:
>>> Some 64bit MIPS config, that relied on GENERIC_IOMAP. And with your
>>> patch applied, I could compile successfully.
>>> Do you have an easier way steer a kernel config into this dead-end?
>>
>> The generic implementation takes care of all conditions. I guess some
>> build bot would report error on build failures. But checks like
>> #ifdef iowrite64 would hide the missing definitions error.
> 
> Yes definitely, we need to avoid this.
> 
>>>
>>>> Thanks,
>>>> Ramesh
>>>
>>> Frankly, I'd rather not make any assumptions in this rather generic
>>> vfio/pci layer about whether hi-lo or lo-hi is the right order to >
>>> emulate a 64bit access when the base architecture does not support
>>> 64bit accesses naturally. So, if CONFIG_64BIT is no guarantee that
>>> there's a definitive implementation of ioread64/iowrite64, I'd
>>> rather
>>
>> There is already an assumption of the order in the current
>> implementation regardless e.g. vfio_pci_core_do_io_rw(). If there is
>> no iowrite64 found, the code does back to back 34 bit writes without
>> checking for any particular order requirements.
>>
>> io-64-nonatomic-lo-hi.h and io-64-nonatomic-hi-lo.h would define
>> ioread64/iowrite64 only if they are not already defined in asm/io.h.
>>
>> Also since there is a check for CONFIG_64BIT, most likely a 64 bit
>> readq/writeq will get used in the lib/iomap.c implementations. I
>> think we can pick either lo-hi or hi-lo for the unlikely 32 bit fall
>> through when CONFIG_64BIT is defined.
> 
> I dug into lib/iomap.c some more today and I see your point, that it is
> desireable to make the 64bit accessors useable through vfio/pci when
> they're implemented in lib/iomap.c. And I follow your argument that in
> most cases these will map onto readq/writeq - only programmed IO (PIO)
> has to emulate this with 2 32bit back-to-back accesses.
> 
> If only the code in lib/iomap.c was structured differently - and made
> readq/writeq available under ioread64/iowrite64 proper and only fell
> back to the nonatomic hi-lo or lo-hi emulation with 32bit accesses if
> PIO is used.

It determines if it is PIO or MMIO based on the address. If the address 
is >= PIO_RESERVED then it calls readq/writeq. PIO_RESERVED is arch 
dependent.

Looks like if asm/io.h didn't define ioread64/iowrite64 already, then 
the intention is to use the generic implementation, especially when it 
defines CONFIG_GENERIC_IOMAP. lib/iomap.c and 
io-64-nonatomic-{lo-hi|-hi-lo}.h include linux/io.h which includes asm/io.h.

> 
> As much as I'd like to have it differently, it seems like it was a
> lengthy process to have that change accepted at the time:
> https://lore.kernel.org/all/20181106205234.25792-1-logang@deltatee.com/
> 
> I'm not sure if we can clean that up, easily. Plus there are appear to
> be plenty of users of io-64-nonatomic-{lo-hi|-hi-lo}.h in tree already
> - 103 and 18, resp.
> 
>>> revert to make the conditional compiles depend on those
>>> definitions. But maybe Alex has an opinion on this, too?
>>>
>>> Thanks,
>>> Gerd
> 
> So I'd like to hear from Alex and Tian (who was not a big fan) if we
> should support 64bit accessors in vfio/pci (primarily) on x86 with this
> series, or not at all, or split that work off, maybe?
> 
> Thanks,
> Gerd
> 

Re: [PATCH v4 2/3] vfio/pci: Support 8-byte PCI loads and stores

[Gerd Bayer]

On Wed, 2024-05-22 at 17:06 +0200, Gerd Bayer wrote:
> diff --git a/drivers/vfio/pci/vfio_pci_rdwr.c
> b/drivers/vfio/pci/vfio_pci_rdwr.c
> index d07bfb0ab892..07351ea76604 100644
> --- a/drivers/vfio/pci/vfio_pci_rdwr.c
> +++ b/drivers/vfio/pci/vfio_pci_rdwr.c
> @@ -124,6 +127,10 @@ static int vfio_pci_iordwr##size(struct
> vfio_pci_core_device *vdev,\
>  VFIO_IORDWR(8)
>  VFIO_IORDWR(16)
>  VFIO_IORDWR(32)
> +#if CONFIG_64BIT

During my experimenations to reproduce Ramesh's complaint about
unresolved symbols, I found that this should have been #ifdef

> +VFIO_IORDWR(64)
> +#endif
> +
>  /*
>   * Read or write from an __iomem region (MMIO or I/O port) with an
> excluded
>   * range which is inaccessible.  The excluded range drops writes and
> fills
> @@ -148,6 +155,15 @@ ssize_t vfio_pci_core_do_io_rw(struct
> vfio_pci_core_device *vdev, bool test_mem,
>  		else
>  			fillable = 0;
>  
> +#if CONFIG_64BIT

... as should this have been.

> +		if (fillable >= 8 && !(off % 8)) {
> +			ret = vfio_pci_iordwr64(vdev, iswrite,
> test_mem,
> +						io, buf, off,
> &filled);
> +			if (ret)
> +				return ret;
> +
> +		} else
> +#endif
>  		if (fillable >= 4 && !(off % 4)) {
>  			ret = vfio_pci_iordwr32(vdev, iswrite,
> test_mem,
>  						io, buf, off,
> &filled);
> diff --git a/include/linux/vfio_pci_core.h
> b/include/linux/vfio_pci_core.h
> index a2c8b8bba711..5f9b02d4a3e9 100644
> --- a/include/linux/vfio_pci_core.h
> +++ b/include/linux/vfio_pci_core.h
> @@ -146,7 +146,7 @@ int vfio_pci_core_iowrite##size(struct
> vfio_pci_core_device *vdev,	\
>  VFIO_IOWRITE_DECLATION(8)
>  VFIO_IOWRITE_DECLATION(16)
>  VFIO_IOWRITE_DECLATION(32)
> -#ifdef iowrite64
> +#ifdef CONFIG_64BIT
>  VFIO_IOWRITE_DECLATION(64)
>  #endif
>  
> @@ -157,5 +157,8 @@ int vfio_pci_core_ioread##size(struct
> vfio_pci_core_device *vdev,	\
>  VFIO_IOREAD_DECLATION(8)
>  VFIO_IOREAD_DECLATION(16)
>  VFIO_IOREAD_DECLATION(32)
> +#ifdef CONFIG_64BIT
> +VFIO_IOREAD_DECLATION(64)
> +#endif
>  
>  #endif /* VFIO_PCI_CORE_H */

So there will be a v5.

Thanks,
Gerd

[PATCH v4 3/3] vfio/pci: Fix typo in macro to declare accessors

[Gerd Bayer]

Correct spelling of DECLA[RA]TION

Suggested-by: Ramesh Thomas <ramesh.thomas@intel.com>
Signed-off-by: Gerd Bayer <gbayer@linux.ibm.com>
---
 include/linux/vfio_pci_core.h | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/include/linux/vfio_pci_core.h b/include/linux/vfio_pci_core.h
index 5f9b02d4a3e9..15d6d6da6e78 100644
--- a/include/linux/vfio_pci_core.h
+++ b/include/linux/vfio_pci_core.h
@@ -139,26 +139,26 @@ bool vfio_pci_core_range_intersect_range(loff_t buf_start, size_t buf_cnt,
 					 loff_t *buf_offset,
 					 size_t *intersect_count,
 					 size_t *register_offset);
-#define VFIO_IOWRITE_DECLATION(size) \
+#define VFIO_IOWRITE_DECLARATION(size) \
 int vfio_pci_core_iowrite##size(struct vfio_pci_core_device *vdev,	\
-			bool test_mem, u##size val, void __iomem *io);
+			bool test_mem, u##size val, void __iomem *io)
 
-VFIO_IOWRITE_DECLATION(8)
-VFIO_IOWRITE_DECLATION(16)
-VFIO_IOWRITE_DECLATION(32)
+VFIO_IOWRITE_DECLARATION(8);
+VFIO_IOWRITE_DECLARATION(16);
+VFIO_IOWRITE_DECLARATION(32);
 #ifdef CONFIG_64BIT
-VFIO_IOWRITE_DECLATION(64)
+VFIO_IOWRITE_DECLARATION(64);
 #endif
 
-#define VFIO_IOREAD_DECLATION(size) \
+#define VFIO_IOREAD_DECLARATION(size) \
 int vfio_pci_core_ioread##size(struct vfio_pci_core_device *vdev,	\
-			bool test_mem, u##size *val, void __iomem *io);
+			bool test_mem, u##size *val, void __iomem *io)
 
-VFIO_IOREAD_DECLATION(8)
-VFIO_IOREAD_DECLATION(16)
-VFIO_IOREAD_DECLATION(32)
+VFIO_IOREAD_DECLARATION(8);
+VFIO_IOREAD_DECLARATION(16);
+VFIO_IOREAD_DECLARATION(32);
 #ifdef CONFIG_64BIT
-VFIO_IOREAD_DECLATION(64)
+VFIO_IOREAD_DECLARATION(64);
 #endif
 
 #endif /* VFIO_PCI_CORE_H */
-- 
2.45.0