| Message ID | 20240425165604.899447-2-gbayer@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vfio/pci: Support 8-byte PCI loads and stores | expand |
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));
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
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 >
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
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 >> > > > . >
On 4/25/2024 9:56 AM, 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))) \ Another way to get the size is (size)/8 "if (copy_from_user(&val, buf, (size)/8))" > + 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));
On Mon, 2024-04-29 at 10:31 -0600, Alex Williamson wrote: > 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 > > Sure that's easy enough. Thanks, Gerd
On Mon, 2024-04-29 at 19:33 -0300, Jason Gunthorpe wrote: > 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.. I appears like duplications, I agree - but the vfio_pci_core_ioreadX and vfio_pci_core_iowriteX accessors are exported as such, or might be reused by way of vfio_pci_iordwrX in vfio_pci_core_do_io_rw for arbitrarily sized read/writes, too. > > 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.. At first, I was overwhelmed by the macro definitions, too. But after a while I started to like the strict typing once the value came out of memcpy or until it is memcpy'd. > > Jason >
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));
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(-)