Re: [RFC PATCH v3 1/9] vfio/pci: export vfio_pci_device public and add vfio_pci_device_private

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

On Tue, 11 Feb 2020 05:10:38 -0500
Yan Zhao <yan.y.zhao@intel.com> wrote:

> (1) make vfio_pci_device public, so it is accessible from external code.
> (2) add a private struct vfio_pci_device_private, which is only accessible
> from internal code. It extends struct vfio_pci_device.
> 
> Signed-off-by: Yan Zhao <yan.y.zhao@intel.com>
> ---
>  drivers/vfio/pci/vfio_pci.c         | 256 +++++++++++++++-------------
>  drivers/vfio/pci/vfio_pci_config.c  | 186 ++++++++++++--------
>  drivers/vfio/pci/vfio_pci_igd.c     |  19 ++-
>  drivers/vfio/pci/vfio_pci_intrs.c   | 186 +++++++++++---------
>  drivers/vfio/pci/vfio_pci_nvlink2.c |  22 +--
>  drivers/vfio/pci/vfio_pci_private.h |   7 +-
>  drivers/vfio/pci/vfio_pci_rdwr.c    |  40 +++--
>  include/linux/vfio.h                |   5 +
>  8 files changed, 408 insertions(+), 313 deletions(-)

[SNIP!]

> diff --git a/include/linux/vfio.h b/include/linux/vfio.h
> index e42a711a2800..70a2b8fb6179 100644
> --- a/include/linux/vfio.h
> +++ b/include/linux/vfio.h
> @@ -195,4 +195,9 @@ extern int vfio_virqfd_enable(void *opaque,
>  			      void *data, struct virqfd **pvirqfd, int fd);
>  extern void vfio_virqfd_disable(struct virqfd **pvirqfd);
>  
> +struct vfio_pci_device {
> +	struct pci_dev			*pdev;
> +	int				num_regions;
> +	int				irq_type;
> +};
>  #endif /* VFIO_H */

Hi Yan,

Sorry for the delay.  I'm still not very happy with this result, I was
hoping the changes could be done less intrusively.  Maybe here's
another suggestion, why can't the vendor driver use a struct
vfio_pci_device* as an opaque pointer?  If you only want these three
things initially, I think this whole massive patch can be reduced to:

struct pci_dev *vfio_pci_pdev(struct vfio_pci_device *vdev)
{
	return vdev->pdev;
}
EXPORT_SYMBOL_GPL(vfio_pci_dev);

int vfio_pci_num_regions(struct vfio_pci_device *vdev)
{
	return vdev->num_regions;
}
EXPORT_SYMBOL_GPL(vfio_pci_num_region);

int vfio_pci_irq_type(struct vfio_pci_device *vdev)
{
	return vdev->irq_type;
}
EXPORT_SYMBOL_GPL(vfio_pci_irq_type);

This is how vfio-pci works with vfio, we don't know a struct
vfio_device as anything other than an opaque pointer and we have access
function where we need to see some property of that object.

Patch 5/9 would become a vfio_pci_set_vendor_regions() interface.

Thanks,
Alex