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

Jason Gunthorpe
2024-04-29T20:09:13Z

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