Re: [PATCH V5 vfio 8/9] vfio/pci: Expose vfio_pci_core_iowrite/read##size()

[Alex Williamson <alex.williamson@redhat.com>]

On Tue, 5 Dec 2023 19:06:22 +0200
Yishai Hadas <yishaih@nvidia.com> wrote:

> Expose vfio_pci_core_iowrite/read##size() to let it be used by drivers.
> 
> This functionality is needed to enable direct access to some physical
> BAR of the device with the proper locks/checks in place.
> 
> The next patches from this series will use this functionality on a data
> path flow when a direct access to the BAR is needed.
> 
> Signed-off-by: Yishai Hadas <yishaih@nvidia.com>
> ---
>  drivers/vfio/pci/vfio_pci_rdwr.c | 46 +++++++++++++++++---------------
>  include/linux/vfio_pci_core.h    | 19 +++++++++++++
>  2 files changed, 43 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/vfio/pci/vfio_pci_rdwr.c b/drivers/vfio/pci/vfio_pci_rdwr.c
> index a9887fd6de46..448ee90a3bb1 100644
> --- a/drivers/vfio/pci/vfio_pci_rdwr.c
> +++ b/drivers/vfio/pci/vfio_pci_rdwr.c
> @@ -38,7 +38,7 @@
>  #define vfio_iowrite8	iowrite8
>  
>  #define VFIO_IOWRITE(size) \
> -static int vfio_pci_iowrite##size(struct vfio_pci_core_device *vdev,		\
> +int vfio_pci_core_iowrite##size(struct vfio_pci_core_device *vdev,	\
>  			bool test_mem, u##size val, void __iomem *io)	\
>  {									\
>  	if (test_mem) {							\
> @@ -55,7 +55,8 @@ static int vfio_pci_iowrite##size(struct vfio_pci_core_device *vdev,		\
>  		up_read(&vdev->memory_lock);				\
>  									\
>  	return 0;							\
> -}
> +}									\
> +EXPORT_SYMBOL_GPL(vfio_pci_core_iowrite##size);
>  
>  VFIO_IOWRITE(8)
>  VFIO_IOWRITE(16)
> @@ -65,7 +66,7 @@ VFIO_IOWRITE(64)
>  #endif
>  
>  #define VFIO_IOREAD(size) \
> -static int vfio_pci_ioread##size(struct vfio_pci_core_device *vdev,		\
> +int vfio_pci_core_ioread##size(struct vfio_pci_core_device *vdev,	\
>  			bool test_mem, u##size *val, void __iomem *io)	\
>  {									\
>  	if (test_mem) {							\
> @@ -82,7 +83,8 @@ static int vfio_pci_ioread##size(struct vfio_pci_core_device *vdev,		\
>  		up_read(&vdev->memory_lock);				\
>  									\
>  	return 0;							\
> -}
> +}									\
> +EXPORT_SYMBOL_GPL(vfio_pci_core_ioread##size);
>  
>  VFIO_IOREAD(8)
>  VFIO_IOREAD(16)
> @@ -119,13 +121,13 @@ static ssize_t do_io_rw(struct vfio_pci_core_device *vdev, bool test_mem,
>  				if (copy_from_user(&val, buf, 4))
>  					return -EFAULT;
>  
> -				ret = vfio_pci_iowrite32(vdev, test_mem,
> -							 val, io + off);
> +				ret = vfio_pci_core_iowrite32(vdev, test_mem,
> +							      val, io + off);
>  				if (ret)
>  					return ret;
>  			} else {
> -				ret = vfio_pci_ioread32(vdev, test_mem,
> -							&val, io + off);
> +				ret = vfio_pci_core_ioread32(vdev, test_mem,
> +							     &val, io + off);
>  				if (ret)
>  					return ret;
>  
> @@ -141,13 +143,13 @@ static ssize_t do_io_rw(struct vfio_pci_core_device *vdev, bool test_mem,
>  				if (copy_from_user(&val, buf, 2))
>  					return -EFAULT;
>  
> -				ret = vfio_pci_iowrite16(vdev, test_mem,
> -							 val, io + off);
> +				ret = vfio_pci_core_iowrite16(vdev, test_mem,
> +							      val, io + off);
>  				if (ret)
>  					return ret;
>  			} else {
> -				ret = vfio_pci_ioread16(vdev, test_mem,
> -							&val, io + off);
> +				ret = vfio_pci_core_ioread16(vdev, test_mem,
> +							     &val, io + off);
>  				if (ret)
>  					return ret;
>  
> @@ -163,13 +165,13 @@ static ssize_t do_io_rw(struct vfio_pci_core_device *vdev, bool test_mem,
>  				if (copy_from_user(&val, buf, 1))
>  					return -EFAULT;
>  
> -				ret = vfio_pci_iowrite8(vdev, test_mem,
> -							val, io + off);
> +				ret = vfio_pci_core_iowrite8(vdev, test_mem,
> +							     val, io + off);
>  				if (ret)
>  					return ret;
>  			} else {
> -				ret = vfio_pci_ioread8(vdev, test_mem,
> -						       &val, io + off);
> +				ret = vfio_pci_core_ioread8(vdev, test_mem,
> +							    &val, io + off);
>  				if (ret)
>  					return ret;
>  
> @@ -364,16 +366,16 @@ static void vfio_pci_ioeventfd_do_write(struct vfio_pci_ioeventfd *ioeventfd,
>  {
>  	switch (ioeventfd->count) {
>  	case 1:
> -		vfio_pci_iowrite8(ioeventfd->vdev, test_mem,
> -				  ioeventfd->data, ioeventfd->addr);
> +		vfio_pci_core_iowrite8(ioeventfd->vdev, test_mem,
> +				       ioeventfd->data, ioeventfd->addr);
>  		break;
>  	case 2:
> -		vfio_pci_iowrite16(ioeventfd->vdev, test_mem,
> -				   ioeventfd->data, ioeventfd->addr);
> +		vfio_pci_core_iowrite16(ioeventfd->vdev, test_mem,
> +					ioeventfd->data, ioeventfd->addr);
>  		break;
>  	case 4:
> -		vfio_pci_iowrite32(ioeventfd->vdev, test_mem,
> -				   ioeventfd->data, ioeventfd->addr);
> +		vfio_pci_core_iowrite32(ioeventfd->vdev, test_mem,
> +					ioeventfd->data, ioeventfd->addr);
>  		break;
>  #ifdef iowrite64
>  	case 8:

There's a vfio_pci_iowrite64() call just below here that was missed.
Otherwise the vfio parts of the series looks ok to me.  We still need
to recruit another reviewer though.

My preferred merge approach would be that virtio maintainers take
patches 1-6 and provide a branch or tag I can merge to bring 7-9 in
through the vfio tree.  Thanks,

Alex

> diff --git a/include/linux/vfio_pci_core.h b/include/linux/vfio_pci_core.h
> index 67ac58e20e1d..85e84b92751b 100644
> --- a/include/linux/vfio_pci_core.h
> +++ b/include/linux/vfio_pci_core.h
> @@ -131,4 +131,23 @@ int vfio_pci_core_setup_barmap(struct vfio_pci_core_device *vdev, int bar);
>  pci_ers_result_t vfio_pci_core_aer_err_detected(struct pci_dev *pdev,
>  						pci_channel_state_t state);
>  
> +#define VFIO_IOWRITE_DECLATION(size) \
> +int vfio_pci_core_iowrite##size(struct vfio_pci_core_device *vdev,	\
> +			bool test_mem, u##size val, void __iomem *io);
> +
> +VFIO_IOWRITE_DECLATION(8)
> +VFIO_IOWRITE_DECLATION(16)
> +VFIO_IOWRITE_DECLATION(32)
> +#ifdef iowrite64
> +VFIO_IOWRITE_DECLATION(64)
> +#endif
> +
> +#define VFIO_IOREAD_DECLATION(size) \
> +int vfio_pci_core_ioread##size(struct vfio_pci_core_device *vdev,	\
> +			bool test_mem, u##size *val, void __iomem *io);
> +
> +VFIO_IOREAD_DECLATION(8)
> +VFIO_IOREAD_DECLATION(16)
> +VFIO_IOREAD_DECLATION(32)
> +
>  #endif /* VFIO_PCI_CORE_H */