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

[PATCH v3 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 and structures the different-size accesses in
vfio_pci_core_do_io_rw() in a way that the conditional compile of
8-byte accesses no longer creates an odd split of "else-if".

The initial version was tested with a PCI device on s390. This version
has only been tested for reads of 1..8 byte sizes and only been compile
tested for a 32bit architecture (arm).

Thank you,
Gerd Bayer

[1] https://lore.kernel.org/all/20240422153508.2355844-1-gbayer@linux.ibm.com/

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: Continue to refactor vfio_pci_core_do_io_rw

 drivers/vfio/pci/vfio_pci_rdwr.c | 154 +++++++++++++++++--------------
 include/linux/vfio_pci_core.h    |   3 +
 2 files changed, 90 insertions(+), 67 deletions(-)

-- 
2.44.0

[PATCH v3 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..3335f1b868b1 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_core_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_core_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_core_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_core_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.44.0

[PATCH v3 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 | 16 ++++++++++++++++
 include/linux/vfio_pci_core.h    |  3 +++
 2 files changed, 19 insertions(+)

diff --git a/drivers/vfio/pci/vfio_pci_rdwr.c b/drivers/vfio/pci/vfio_pci_rdwr.c
index 3335f1b868b1..8ed06edaee23 100644
--- a/drivers/vfio/pci/vfio_pci_rdwr.c
+++ b/drivers/vfio/pci/vfio_pci_rdwr.c
@@ -89,6 +89,9 @@ EXPORT_SYMBOL_GPL(vfio_pci_core_ioread##size);
 VFIO_IOREAD(8)
 VFIO_IOREAD(16)
 VFIO_IOREAD(32)
+#ifdef ioread64
+VFIO_IOREAD(64)
+#endif
 
 #define VFIO_IORDWR(size)						\
 static int vfio_pci_core_iordwr##size(struct vfio_pci_core_device *vdev,\
@@ -124,6 +127,10 @@ static int vfio_pci_core_iordwr##size(struct vfio_pci_core_device *vdev,\
 VFIO_IORDWR(8)
 VFIO_IORDWR(16)
 VFIO_IORDWR(32)
+#if defined(ioread64) && defined(iowrite64)
+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 defined(ioread64) && defined(iowrite64)
+		if (fillable >= 8 && !(off % 8)) {
+			ret = vfio_pci_core_iordwr64(vdev, iswrite, test_mem,
+						     io, buf, off, &filled);
+			if (ret)
+				return ret;
+
+		} else
+#endif /* defined(ioread64) && defined(iowrite64) */
 		if (fillable >= 4 && !(off % 4)) {
 			ret = vfio_pci_core_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..f4cf5fd2350c 100644
--- a/include/linux/vfio_pci_core.h
+++ b/include/linux/vfio_pci_core.h
@@ -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 ioread64
+VFIO_IOREAD_DECLATION(64)
+#endif
 
 #endif /* VFIO_PCI_CORE_H */
-- 
2.44.0

[PATCH v3 3/3] vfio/pci: Continue to refactor vfio_pci_core_do_io_rw

[Gerd Bayer]

Convert if-elseif-chain into switch-case.
Separate out and generalize the code from the if-clauses so the result
can be used in the switch statement.

Signed-off-by: Gerd Bayer <gbayer@linux.ibm.com>
---
 drivers/vfio/pci/vfio_pci_rdwr.c | 30 ++++++++++++++++++++++++------
 1 file changed, 24 insertions(+), 6 deletions(-)

diff --git a/drivers/vfio/pci/vfio_pci_rdwr.c b/drivers/vfio/pci/vfio_pci_rdwr.c
index 8ed06edaee23..634c00b03c71 100644
--- a/drivers/vfio/pci/vfio_pci_rdwr.c
+++ b/drivers/vfio/pci/vfio_pci_rdwr.c
@@ -131,6 +131,20 @@ VFIO_IORDWR(32)
 VFIO_IORDWR(64)
 #endif
 
+static int fill_size(size_t fillable, loff_t off)
+{
+	unsigned int fill_size;
+#if defined(ioread64) && defined(iowrite64)
+	for (fill_size = 8; fill_size >= 0; fill_size /= 2) {
+#else
+	for (fill_size = 4; fill_size >= 0; fill_size /= 2) {
+#endif /* defined(ioread64) && defined(iowrite64) */
+		if (fillable >= fill_size && !(off % fill_size))
+			return fill_size;
+	}
+	return -1;
+}
+
 /*
  * 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
@@ -155,34 +169,38 @@ ssize_t vfio_pci_core_do_io_rw(struct vfio_pci_core_device *vdev, bool test_mem,
 		else
 			fillable = 0;
 
+		switch (fill_size(fillable, off)) {
 #if defined(ioread64) && defined(iowrite64)
-		if (fillable >= 8 && !(off % 8)) {
+		case 8:
 			ret = vfio_pci_core_iordwr64(vdev, iswrite, test_mem,
 						     io, buf, off, &filled);
 			if (ret)
 				return ret;
+			break;
 
-		} else
 #endif /* defined(ioread64) && defined(iowrite64) */
-		if (fillable >= 4 && !(off % 4)) {
+		case 4:
 			ret = vfio_pci_core_iordwr32(vdev, iswrite, test_mem,
 						     io, buf, off, &filled);
 			if (ret)
 				return ret;
+			break;
 
-		} else if (fillable >= 2 && !(off % 2)) {
+		case 2:
 			ret = vfio_pci_core_iordwr16(vdev, iswrite, test_mem,
 						     io, buf, off, &filled);
 			if (ret)
 				return ret;
+			break;
 
-		} else if (fillable) {
+		case 1:
 			ret = vfio_pci_core_iordwr8(vdev, iswrite, test_mem,
 						    io, buf, off, &filled);
 			if (ret)
 				return ret;
+			break;
 
-		} else {
+		default:
 			/* Fill reads with -1, drop writes */
 			filled = min(count, (size_t)(x_end - off));
 			if (!iswrite) {
-- 
2.44.0

RE: [PATCH v3 3/3] vfio/pci: Continue to refactor vfio_pci_core_do_io_rw

[Tian, Kevin]

> From: Gerd Bayer <gbayer@linux.ibm.com>
> Sent: Friday, April 26, 2024 12:56 AM
> 
> @@ -131,6 +131,20 @@ VFIO_IORDWR(32)
>  VFIO_IORDWR(64)
>  #endif
> 
> +static int fill_size(size_t fillable, loff_t off)
> +{
> +	unsigned int fill_size;
> +#if defined(ioread64) && defined(iowrite64)
> +	for (fill_size = 8; fill_size >= 0; fill_size /= 2) {
> +#else
> +	for (fill_size = 4; fill_size >= 0; fill_size /= 2) {
> +#endif /* defined(ioread64) && defined(iowrite64) */
> +		if (fillable >= fill_size && !(off % fill_size))
> +			return fill_size;
> +	}
> +	return -1;

this is unreachable with "fill_size >= 0" in the loop.

shouldn't it be:

#if defined(ioread64) && defined(iowrite64)
	for (fill_size = 8; fill_size > 0; fill_size /= 2) {
#else
	for (fill_size = 4; fill_size > 0; fill_size /= 2) {
#endif /* defined(ioread64) && defined(iowrite64) */
		if (fillable >= fill_size && !(off % fill_size))
			break;
	}
	return fill_size;


> 
> +		switch (fill_size(fillable, off)) {
>  #if defined(ioread64) && defined(iowrite64)
> -		if (fillable >= 8 && !(off % 8)) {
> +		case 8:
>  			ret = vfio_pci_core_iordwr64(vdev, iswrite, test_mem,
>  						     io, buf, off, &filled);
>  			if (ret)
>  				return ret;
> +			break;

the check on 'ret' can be moved out of the switch to be generic.

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

[Alex Williamson]

On Thu, 25 Apr 2024 18:56:02 +0200
Gerd Bayer <gbayer@linux.ibm.com> wrote:

> 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..3335f1b868b1 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_core_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)		\

I realized later after proposing this that we should drop 'core' from
the name since the resulting functions are not currently exported.  It
also helps with the wordiness.  Thanks,

Alex

> +{									\
> +	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_core_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_core_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_core_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));

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

[Alex Williamson]

On Thu, 25 Apr 2024 18:56:03 +0200
Gerd Bayer <gbayer@linux.ibm.com> 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 | 16 ++++++++++++++++
>  include/linux/vfio_pci_core.h    |  3 +++
>  2 files changed, 19 insertions(+)
> 
> diff --git a/drivers/vfio/pci/vfio_pci_rdwr.c b/drivers/vfio/pci/vfio_pci_rdwr.c
> index 3335f1b868b1..8ed06edaee23 100644
> --- a/drivers/vfio/pci/vfio_pci_rdwr.c
> +++ b/drivers/vfio/pci/vfio_pci_rdwr.c
> @@ -89,6 +89,9 @@ EXPORT_SYMBOL_GPL(vfio_pci_core_ioread##size);
>  VFIO_IOREAD(8)
>  VFIO_IOREAD(16)
>  VFIO_IOREAD(32)
> +#ifdef ioread64
> +VFIO_IOREAD(64)
> +#endif
>  
>  #define VFIO_IORDWR(size)						\
>  static int vfio_pci_core_iordwr##size(struct vfio_pci_core_device *vdev,\
> @@ -124,6 +127,10 @@ static int vfio_pci_core_iordwr##size(struct vfio_pci_core_device *vdev,\
>  VFIO_IORDWR(8)
>  VFIO_IORDWR(16)
>  VFIO_IORDWR(32)
> +#if defined(ioread64) && defined(iowrite64)
> +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 defined(ioread64) && defined(iowrite64)

Nit, #ifdef vfio_pci_core_iordwr64

> +		if (fillable >= 8 && !(off % 8)) {
> +			ret = vfio_pci_core_iordwr64(vdev, iswrite, test_mem,
> +						     io, buf, off, &filled);
> +			if (ret)
> +				return ret;
> +
> +		} else
> +#endif /* defined(ioread64) && defined(iowrite64) */

AFAIK, the comment appended to the #endif is really only suggested when
the code block is too long to reasonable fit in a terminal.  That's no
longer the case with the new helper.  Thanks,

Alex

>  		if (fillable >= 4 && !(off % 4)) {
>  			ret = vfio_pci_core_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..f4cf5fd2350c 100644
> --- a/include/linux/vfio_pci_core.h
> +++ b/include/linux/vfio_pci_core.h
> @@ -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 ioread64
> +VFIO_IOREAD_DECLATION(64)
> +#endif
>  
>  #endif /* VFIO_PCI_CORE_H */

Re: [PATCH v3 3/3] vfio/pci: Continue to refactor vfio_pci_core_do_io_rw

[Alex Williamson]

On Thu, 25 Apr 2024 18:56:04 +0200
Gerd Bayer <gbayer@linux.ibm.com> wrote:

> Convert if-elseif-chain into switch-case.
> Separate out and generalize the code from the if-clauses so the result
> can be used in the switch statement.
> 
> Signed-off-by: Gerd Bayer <gbayer@linux.ibm.com>
> ---
>  drivers/vfio/pci/vfio_pci_rdwr.c | 30 ++++++++++++++++++++++++------
>  1 file changed, 24 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/vfio/pci/vfio_pci_rdwr.c b/drivers/vfio/pci/vfio_pci_rdwr.c
> index 8ed06edaee23..634c00b03c71 100644
> --- a/drivers/vfio/pci/vfio_pci_rdwr.c
> +++ b/drivers/vfio/pci/vfio_pci_rdwr.c
> @@ -131,6 +131,20 @@ VFIO_IORDWR(32)
>  VFIO_IORDWR(64)

#define MAX_FILL_SIZE 8
#else
#define MAX_FILL_SIZE 4

>  #endif
>  
> +static int fill_size(size_t fillable, loff_t off)
> +{
> +	unsigned int fill_size;

	unsigned int fill_size = MAX_FILL_SIZE;

> +#if defined(ioread64) && defined(iowrite64)
> +	for (fill_size = 8; fill_size >= 0; fill_size /= 2) {
> +#else
> +	for (fill_size = 4; fill_size >= 0; fill_size /= 2) {
> +#endif /* defined(ioread64) && defined(iowrite64) */
> +		if (fillable >= fill_size && !(off % fill_size))
> +			return fill_size;
> +	}
> +	return -1;
> +}
> +
>  /*
>   * 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
> @@ -155,34 +169,38 @@ ssize_t vfio_pci_core_do_io_rw(struct vfio_pci_core_device *vdev, bool test_mem,
>  		else
>  			fillable = 0;
>  
> +		switch (fill_size(fillable, off)) {
>  #if defined(ioread64) && defined(iowrite64)
> -		if (fillable >= 8 && !(off % 8)) {
> +		case 8:
>  			ret = vfio_pci_core_iordwr64(vdev, iswrite, test_mem,
>  						     io, buf, off, &filled);
>  			if (ret)
>  				return ret;
> +			break;
>  
> -		} else

AFAICT, avoiding this dangling else within the #ifdef is really the
only tangible advantage of conversion to a switch statement.  Getting
rid of that alone while keeping, and actually increasing, the inline
ifdefs in the code doesn't seem worthwhile to me.  I'd probably only go
this route if we could make vfio_pci_iordwr64() stubbed as a BUG_ON
when we don't have the ioread64 and iowrite64 accessors, in which case
the switch helper would never return 8 and the function would be
unreachable.

>  #endif /* defined(ioread64) && defined(iowrite64) */
> -		if (fillable >= 4 && !(off % 4)) {
> +		case 4:
>  			ret = vfio_pci_core_iordwr32(vdev, iswrite, test_mem,
>  						     io, buf, off, &filled);
>  			if (ret)
>  				return ret;
> +			break;
>  
> -		} else if (fillable >= 2 && !(off % 2)) {
> +		case 2:
>  			ret = vfio_pci_core_iordwr16(vdev, iswrite, test_mem,
>  						     io, buf, off, &filled);
>  			if (ret)
>  				return ret;
> +			break;
>  
> -		} else if (fillable) {
> +		case 1:
>  			ret = vfio_pci_core_iordwr8(vdev, iswrite, test_mem,
>  						    io, buf, off, &filled);
>  			if (ret)
>  				return ret;
> +			break;
>  
> -		} else {
> +		default:

This condition also seems a little more obfuscated without being
preceded by the 'if (fillable)' test, which might warrant handling
separate from the switch if we continue to pursue the switch
conversion.  Thanks,

Alex

>  			/* Fill reads with -1, drop writes */
>  			filled = min(count, (size_t)(x_end - off));
>  			if (!iswrite) {

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

[Jason Gunthorpe]

On Thu, Apr 25, 2024 at 06:56:02PM +0200, Gerd Bayer wrote:
> 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..3335f1b868b1 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_core_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)

I'd suggest to try writing this without so many macros.

This isn't very performance optimal already, we take a lock on every
iteration, so there isn't much point in inlining multiple copies of
everything to save an branch.

Push the sizing switch down to the bottom, start with a function like:

static void __iowrite(const void *val, void __iomem *io, size_t len)
{
	switch (len) {
	case 8: {
#ifdef iowrite64 // NOTE this doesn't seem to work on x86?
		if (IS_ENABLED(CONFIG_CPU_BIG_ENDIAN))
			return iowrite64be(*(const u64 *)val, io);
		return iowrite64(*(const u64 *)val, io);
#else
		if (IS_ENABLED(CONFIG_CPU_BIG_ENDIAN)) {
			iowrite32be(*(const u32 *)val, io);
			iowrite32be(*(const u32 *)(val + 4), io + 4);
		} else {
			iowrite32(*(const u32 *)val, io);
			iowrite32(*(const u32 *)(val + 4), io + 4);
		}
		return;
#endif
	}

	case 4:
		if (IS_ENABLED(CONFIG_CPU_BIG_ENDIAN))
			return iowrite32be(*(const u32 *)val, io);
		return iowrite32(*(const u32 *)val, io);
	case 2:
		if (IS_ENABLED(CONFIG_CPU_BIG_ENDIAN))
			return iowrite16be(*(const u16 *)val, io);
		return iowrite16(*(const u16 *)val, io);

	case 1:
		return iowrite8(*(const u8 *)val, io);
	}
}

And then wrap it with the copy and the lock:

static int do_iordwr(struct vfio_pci_core_device *vdev, bool test_mem,
		     const void __user *buf, void __iomem *io, size_t len,
		     bool iswrite)
{
	u64 val;

	if (iswrite && copy_from_user(&val, buf, len))
		return -EFAULT;

	if (test_mem) {
		down_read(&vdev->memory_lock);
		if (!__vfio_pci_memory_enabled(vdev)) {
			up_read(&vdev->memory_lock);
			return -EIO;
		}
	}

	if (iswrite)
		__iowrite(&val, io, len);
	else
		__ioread(&val, io, len);

	if (test_mem)
		up_read(&vdev->memory_lock);

	if (!iswrite && copy_to_user(buf, &val, len))
		return -EFAULT;

	return 0;
}

And then the loop can be simple:

		if (fillable) {
			filled = num_bytes(fillable, off);
			ret = do_iordwr(vdev, test_mem, buf, io + off, filled,
					iswrite);
			if (ret)
				return ret;
		} else {
			filled = min(count, (size_t)(x_end - off));
			/* Fill reads with -1, drop writes */
			ret = fill_err(buf, filled);
			if (ret)
				return ret;
		}

Jason

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

[Alex Williamson]

On Mon, 29 Apr 2024 17:09:10 -0300
Jason Gunthorpe <jgg@ziepe.ca> wrote:

> On Thu, Apr 25, 2024 at 06:56:02PM +0200, Gerd Bayer wrote:
> > 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..3335f1b868b1 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_core_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)  
> 
> I'd suggest to try writing this without so many macros.
> 
> This isn't very performance optimal already, we take a lock on every
> iteration, so there isn't much point in inlining multiple copies of
> everything to save an branch.

These macros are to reduce duplicate code blocks and the errors that
typically come from such duplication, as well as to provide type safe
functions in the spirit of the ioread# and iowrite# helpers.  It really
has nothing to do with, nor is it remotely effective at saving a branch.
Thanks,

Alex

> Push the sizing switch down to the bottom, start with a function like:
> 
> static void __iowrite(const void *val, void __iomem *io, size_t len)
> {
> 	switch (len) {
> 	case 8: {
> #ifdef iowrite64 // NOTE this doesn't seem to work on x86?
> 		if (IS_ENABLED(CONFIG_CPU_BIG_ENDIAN))
> 			return iowrite64be(*(const u64 *)val, io);
> 		return iowrite64(*(const u64 *)val, io);
> #else
> 		if (IS_ENABLED(CONFIG_CPU_BIG_ENDIAN)) {
> 			iowrite32be(*(const u32 *)val, io);
> 			iowrite32be(*(const u32 *)(val + 4), io + 4);
> 		} else {
> 			iowrite32(*(const u32 *)val, io);
> 			iowrite32(*(const u32 *)(val + 4), io + 4);
> 		}
> 		return;
> #endif
> 	}
> 
> 	case 4:
> 		if (IS_ENABLED(CONFIG_CPU_BIG_ENDIAN))
> 			return iowrite32be(*(const u32 *)val, io);
> 		return iowrite32(*(const u32 *)val, io);
> 	case 2:
> 		if (IS_ENABLED(CONFIG_CPU_BIG_ENDIAN))
> 			return iowrite16be(*(const u16 *)val, io);
> 		return iowrite16(*(const u16 *)val, io);
> 
> 	case 1:
> 		return iowrite8(*(const u8 *)val, io);
> 	}
> }
> 
> And then wrap it with the copy and the lock:
> 
> static int do_iordwr(struct vfio_pci_core_device *vdev, bool test_mem,
> 		     const void __user *buf, void __iomem *io, size_t len,
> 		     bool iswrite)
> {
> 	u64 val;
> 
> 	if (iswrite && copy_from_user(&val, buf, len))
> 		return -EFAULT;
> 
> 	if (test_mem) {
> 		down_read(&vdev->memory_lock);
> 		if (!__vfio_pci_memory_enabled(vdev)) {
> 			up_read(&vdev->memory_lock);
> 			return -EIO;
> 		}
> 	}
> 
> 	if (iswrite)
> 		__iowrite(&val, io, len);
> 	else
> 		__ioread(&val, io, len);
> 
> 	if (test_mem)
> 		up_read(&vdev->memory_lock);
> 
> 	if (!iswrite && copy_to_user(buf, &val, len))
> 		return -EFAULT;
> 
> 	return 0;
> }
> 
> And then the loop can be simple:
> 
> 		if (fillable) {
> 			filled = num_bytes(fillable, off);
> 			ret = do_iordwr(vdev, test_mem, buf, io + off, filled,
> 					iswrite);
> 			if (ret)
> 				return ret;
> 		} else {
> 			filled = min(count, (size_t)(x_end - off));
> 			/* Fill reads with -1, drop writes */
> 			ret = fill_err(buf, filled);
> 			if (ret)
> 				return ret;
> 		}
> 
> Jason
> 

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

[Jason Gunthorpe]

On Mon, Apr 29, 2024 at 04:11:03PM -0600, Alex Williamson wrote:
> > This isn't very performance optimal already, we take a lock on every
> > iteration, so there isn't much point in inlining multiple copies of
> > everything to save an branch.
> 
> These macros are to reduce duplicate code blocks and the errors that
> typically come from such duplication, 

But there is still quite a bit of repetition here..

> as well as to provide type safe functions in the spirit of the
> ioread# and iowrite# helpers.

But it never really takes any advantage of type safety? It is making a
memcpy..

Jason

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

[liulongfang]

On 2024/4/30 6:11, Alex Williamson wrote:
> On Mon, 29 Apr 2024 17:09:10 -0300
> Jason Gunthorpe <jgg@ziepe.ca> wrote:
> 
>> On Thu, Apr 25, 2024 at 06:56:02PM +0200, Gerd Bayer wrote:
>>> 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..3335f1b868b1 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_core_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)  
>>
>> I'd suggest to try writing this without so many macros.
>>
>> This isn't very performance optimal already, we take a lock on every
>> iteration, so there isn't much point in inlining multiple copies of
>> everything to save an branch.
> 
> These macros are to reduce duplicate code blocks and the errors that

Although simple and straightforward writing will result in more lines of code.
But it's not easy to squeeze in "extra" code.
The backdoor of "XZ Utils" is implanted through code complication.

Thanks.
Longfang.

> typically come from such duplication, as well as to provide type safe
> functions in the spirit of the ioread# and iowrite# helpers.  It really
> has nothing to do with, nor is it remotely effective at saving a branch.
> Thanks,
> 
> Alex
> 
>> Push the sizing switch down to the bottom, start with a function like:
>>
>> static void __iowrite(const void *val, void __iomem *io, size_t len)
>> {
>> 	switch (len) {
>> 	case 8: {
>> #ifdef iowrite64 // NOTE this doesn't seem to work on x86?
>> 		if (IS_ENABLED(CONFIG_CPU_BIG_ENDIAN))
>> 			return iowrite64be(*(const u64 *)val, io);
>> 		return iowrite64(*(const u64 *)val, io);
>> #else
>> 		if (IS_ENABLED(CONFIG_CPU_BIG_ENDIAN)) {
>> 			iowrite32be(*(const u32 *)val, io);
>> 			iowrite32be(*(const u32 *)(val + 4), io + 4);
>> 		} else {
>> 			iowrite32(*(const u32 *)val, io);
>> 			iowrite32(*(const u32 *)(val + 4), io + 4);
>> 		}
>> 		return;
>> #endif
>> 	}
>>
>> 	case 4:
>> 		if (IS_ENABLED(CONFIG_CPU_BIG_ENDIAN))
>> 			return iowrite32be(*(const u32 *)val, io);
>> 		return iowrite32(*(const u32 *)val, io);
>> 	case 2:
>> 		if (IS_ENABLED(CONFIG_CPU_BIG_ENDIAN))
>> 			return iowrite16be(*(const u16 *)val, io);
>> 		return iowrite16(*(const u16 *)val, io);
>>
>> 	case 1:
>> 		return iowrite8(*(const u8 *)val, io);
>> 	}
>> }
>>
>> And then wrap it with the copy and the lock:
>>
>> static int do_iordwr(struct vfio_pci_core_device *vdev, bool test_mem,
>> 		     const void __user *buf, void __iomem *io, size_t len,
>> 		     bool iswrite)
>> {
>> 	u64 val;
>>
>> 	if (iswrite && copy_from_user(&val, buf, len))
>> 		return -EFAULT;
>>
>> 	if (test_mem) {
>> 		down_read(&vdev->memory_lock);
>> 		if (!__vfio_pci_memory_enabled(vdev)) {
>> 			up_read(&vdev->memory_lock);
>> 			return -EIO;
>> 		}
>> 	}
>>
>> 	if (iswrite)
>> 		__iowrite(&val, io, len);
>> 	else
>> 		__ioread(&val, io, len);
>>
>> 	if (test_mem)
>> 		up_read(&vdev->memory_lock);
>>
>> 	if (!iswrite && copy_to_user(buf, &val, len))
>> 		return -EFAULT;
>>
>> 	return 0;
>> }
>>
>> And then the loop can be simple:
>>
>> 		if (fillable) {
>> 			filled = num_bytes(fillable, off);
>> 			ret = do_iordwr(vdev, test_mem, buf, io + off, filled,
>> 					iswrite);
>> 			if (ret)
>> 				return ret;
>> 		} else {
>> 			filled = min(count, (size_t)(x_end - off));
>> 			/* Fill reads with -1, drop writes */
>> 			ret = fill_err(buf, filled);
>> 			if (ret)
>> 				return ret;
>> 		}
>>
>> Jason
>>
> 
> 
> .
>