On Mon, 22 Apr 2024 14:43:05 -0300
Jason Gunthorpe <jgg@xxxxxxxx> wrote:
> On Mon, Apr 22, 2024 at 05:35:08PM +0200, Gerd Bayer wrote:
> > From: Ben Segal <bpsegal@xxxxxxxxxx>
> >
> > 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.
> >
> > Since these access methods are instantiated on 64bit architectures,
> > only, their use in vfio_pci_core_do_io_rw() is restricted by conditional
> > compiles to these architectures.
> >
> > Signed-off-by: Ben Segal <bpsegal@xxxxxxxxxx>
> > Co-developed-by: Gerd Bayer <gbayer@xxxxxxxxxxxxx>
> > Signed-off-by: Gerd Bayer <gbayer@xxxxxxxxxxxxx>
> > ---
> > Hi all,
> >
> > we've successfully used this patch with 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.
> >
> > Thank you,
> > Gerd Bayer
> >
> > 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.
>
> Provide a iowrite64() that does back to back writes for 32 bit?
I was curious what this looked like. If we want to repeat the 4 byte
access then I think we need to refactor all the read/write accesses
into macros to avoid duplicating code, which results in something like
[1] below. But also once we refactor to macros, the #ifdef within the
function as originally proposed gets a lot more bearable too [2].
I'd probably just go with something like [2] unless you want to further
macro-ize these branches out of existence in the main function. Feel
free to grab any of this you like, the VFIO_IORDWR macro should
probably be it's own patch. Thanks,
Alex
[1]
drivers/vfio/pci/vfio_pci_rdwr.c | 145 +++++++++++++++++--------------
include/linux/vfio_pci_core.h | 3
2 files changed, 87 insertions(+), 61 deletions(-)
diff --git a/drivers/vfio/pci/vfio_pci_rdwr.c b/drivers/vfio/pci/vfio_pci_rdwr.c
index 03b8f7ada1ac..84bd1d670086 100644
--- a/drivers/vfio/pci/vfio_pci_rdwr.c
+++ b/drivers/vfio/pci/vfio_pci_rdwr.c
@@ -89,6 +89,71 @@ 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,\
+ 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)
+#if defined(ioread64) && defined(iowrite64)
+VFIO_IORDWR(64)
+#else
+static int vfio_pci_core_iordwr64(struct vfio_pci_core_device *vdev,
+ bool iswrite, bool test_mem,
+ void __iomem *io, char __user *buf,
+ loff_t off, size_t *filled)
+{
+ int ret;
+ size_t cnt;
+
+ ret = vfio_pci_core_iordwr32(vdev, iswrite, test_mem,
+ io, buf, off, &cnt);
+ if (ret)
+ return ret;
+
+ ret = vfio_pci_core_iordwr32(vdev, iswrite, test_mem,
+ io, buf + cnt, off + cnt,
+ filled);
+ if (ret)
+ return ret;
+
+ *filled += cnt;
+
+ return 0;
+}
+#endif
/*
* Read or write from an __iomem region (MMIO or I/O port) with an excluded
@@ -114,72 +179,30 @@ ssize_t vfio_pci_core_do_io_rw(struct vfio_pci_core_device *vdev, bool test_mem,
else
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;
- }
+ if (fillable >= 8 && !(off % 8)) {
+ ret = vfio_pci_core_iordwr64(vdev, iswrite, test_mem,
+ io, buf, off, &filled);
+ if (ret)
+ return ret;
+
+ } else if (fillable >= 4 && !(off % 4)) {
+ 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));
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]
drivers/vfio/pci/vfio_pci_rdwr.c | 122 +++++++++++++++----------------
include/linux/vfio_pci_core.h | 3
2 files changed, 65 insertions(+), 60 deletions(-)
commit 05aa3342b4f9e249f3bb370e2fdf66e95de556b8
Author: Alex Williamson <alex.williamson@xxxxxxxxxx>
Date: Mon Apr 22 15:58:48 2024 -0600
test
diff --git a/drivers/vfio/pci/vfio_pci_rdwr.c b/drivers/vfio/pci/vfio_pci_rdwr.c
index 03b8f7ada1ac..22e889374e47 100644
--- a/drivers/vfio/pci/vfio_pci_rdwr.c
+++ b/drivers/vfio/pci/vfio_pci_rdwr.c
@@ -89,6 +89,47 @@ 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,\
+ 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)
+#if defined(ioread64) && defined(iowrite64)
+VFIO_IORDWR(64)
+#endif
/*
* Read or write from an __iomem region (MMIO or I/O port) with an excluded
@@ -114,72 +155,33 @@ 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
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));
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 */
