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

[PATCH v5 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]. However,
after some discussions about making 8-byte accesses available for x86,
Ramesh and I decided to do this in a separate patch [3].

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
[3] https://lore.kernel.org/all/20240522232125.548643-1-ramesh.thomas@intel.com/

Changes v4 -> v5:
- Make 8-byte accessors depend on the definitions of ioread64 and
  iowrite64, again. Ramesh agreed to sort these out for x86 separately.

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 | 122 ++++++++++++++++---------------
 include/linux/vfio_pci_core.h    |  25 ++++---
 2 files changed, 76 insertions(+), 71 deletions(-)

-- 
2.45.1

[PATCH v5 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.1

[PATCH v5 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 d07bfb0ab892..66b72c289284 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_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 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_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..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.45.1

[PATCH v5 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 f4cf5fd2350c..fa59d40573f1 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 iowrite64
-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 ioread64
-VFIO_IOREAD_DECLATION(64)
+VFIO_IOREAD_DECLARATION(64);
 #endif
 
 #endif /* VFIO_PCI_CORE_H */
-- 
2.45.1

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

[Alex Williamson]

On Wed,  5 Jun 2024 18:01:12 +0200
Gerd Bayer <gbayer@linux.ibm.com> wrote:

> Correct spelling of DECLA[RA]TION

But why did we also transfer the semicolon from the body of the macro
to the call site?  This doesn't match how we handle macros for
VFIO_IOWRITE, VFIO_IOREAD, or the new VFIO_IORDWR added in this series.
Thanks,

Alex

> 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 f4cf5fd2350c..fa59d40573f1 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 iowrite64
> -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 ioread64
> -VFIO_IOREAD_DECLATION(64)
> +VFIO_IOREAD_DECLARATION(64);
>  #endif
>  
>  #endif /* VFIO_PCI_CORE_H */

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

[Gerd Bayer]

On Tue, 2024-06-18 at 11:20 -0600, Alex Williamson wrote:
> On Wed,  5 Jun 2024 18:01:12 +0200
> Gerd Bayer <gbayer@linux.ibm.com> wrote:
> 
> > Correct spelling of DECLA[RA]TION
> 
> But why did we also transfer the semicolon from the body of the macro
> to the call site?  This doesn't match how we handle macros for
> VFIO_IOWRITE, VFIO_IOREAD, or the new VFIO_IORDWR added in this
> series.
> Thanks,
> 
> Alex

Hi Alex,

I wanted to make it visible, already in the contracted form, that
VFIO_IO{READ|WRITE}_DECLARATION is in fact expanding to a function
prototype declaration, while the marco defines in
drivers/vfio/pci/vfio_pci_core.c expand to function implementations.

My quick searching for in-tree precedence was pretty inconclusive
though. So, I can revert that if you want.

Thank you,
Gerd


> > 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 f4cf5fd2350c..fa59d40573f1 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 iowrite64
> > -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 ioread64
> > -VFIO_IOREAD_DECLATION(64)
> > +VFIO_IOREAD_DECLARATION(64);
> >  #endif
> >  
> >  #endif /* VFIO_PCI_CORE_H */
> 
> 

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

[Alex Williamson]

On Tue, 18 Jun 2024 20:04:26 +0200
Gerd Bayer <gbayer@linux.ibm.com> wrote:

> On Tue, 2024-06-18 at 11:20 -0600, Alex Williamson wrote:
> > On Wed,  5 Jun 2024 18:01:12 +0200
> > Gerd Bayer <gbayer@linux.ibm.com> wrote:
> >   
> > > Correct spelling of DECLA[RA]TION  
> > 
> > But why did we also transfer the semicolon from the body of the macro
> > to the call site?  This doesn't match how we handle macros for
> > VFIO_IOWRITE, VFIO_IOREAD, or the new VFIO_IORDWR added in this
> > series.
> > Thanks,
> > 
> > Alex  
> 
> Hi Alex,
> 
> I wanted to make it visible, already in the contracted form, that
> VFIO_IO{READ|WRITE}_DECLARATION is in fact expanding to a function
> prototype declaration, while the marco defines in
> drivers/vfio/pci/vfio_pci_core.c expand to function implementations.
> 
> My quick searching for in-tree precedence was pretty inconclusive
> though. So, I can revert that if you want.

Hi Gerd,

I'd tend to keep them as is since both are declaring something, a
prototype or a function, rather than a macro intended to be used
inline.  Ideally one macro could handle both declarations now that we
sort of have symmetry but we'd currently still need a #ifdef in the
macro which doesn't trivially work.  If we were to do something like
that though, relocating the semicolon doesn't make sense.

In any case, this proposal is stated as just a typo fix, but it's more.
Thanks,

Alex

> > > 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 f4cf5fd2350c..fa59d40573f1 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 iowrite64
> > > -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 ioread64
> > > -VFIO_IOREAD_DECLATION(64)
> > > +VFIO_IOREAD_DECLARATION(64);
> > >  #endif
> > >  
> > >  #endif /* VFIO_PCI_CORE_H */  
> > 
> >   
> 

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

[Gerd Bayer]

On Tue, 2024-06-18 at 13:01 -0600, Alex Williamson wrote:
> On Tue, 18 Jun 2024 20:04:26 +0200
> Gerd Bayer <gbayer@linux.ibm.com> wrote:
> 
> > On Tue, 2024-06-18 at 11:20 -0600, Alex Williamson wrote:
> > > On Wed,  5 Jun 2024 18:01:12 +0200
> > > Gerd Bayer <gbayer@linux.ibm.com> wrote:
> > >   
> > > > Correct spelling of DECLA[RA]TION  
> > > 
> > > But why did we also transfer the semicolon from the body of the
> > > macro
> > > to the call site?  This doesn't match how we handle macros for
> > > VFIO_IOWRITE, VFIO_IOREAD, or the new VFIO_IORDWR added in this
> > > series.
> > > Thanks,
> > > 
> > > Alex  
> > 
> > Hi Alex,
> > 
> > I wanted to make it visible, already in the contracted form, that
> > VFIO_IO{READ|WRITE}_DECLARATION is in fact expanding to a function
> > prototype declaration, while the marco defines in
> > drivers/vfio/pci/vfio_pci_core.c expand to function
> > implementations.
> > 
> > My quick searching for in-tree precedence was pretty inconclusive
> > though. So, I can revert that if you want.
> 
> Hi Gerd,

Hi Alex,

> I'd tend to keep them as is since both are declaring something, a
> prototype or a function, rather than a macro intended to be used
> inline.  Ideally one macro could handle both declarations now that we
> sort of have symmetry but we'd currently still need a #ifdef in the
> macro which doesn't trivially work.  If we were to do something like
> that though, relocating the semicolon doesn't make sense.
> 
> In any case, this proposal is stated as just a typo fix, but it's
> more.

I have no hard feelings about the place of the semicolon - I'll be
sending out a v6 with just the typo fix in patch 3/3.

> Thanks,
> 
> Alex

Thanks,
Gerd

> 
> > > > 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 f4cf5fd2350c..fa59d40573f1 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)
> > > >