Re: [PATCH 3/3] VFIO V4: VFIO driver: Non-privileged user level PCI drivers

["Michael S. Tsirkin" <mst@redhat.com>]

On Mon, Sep 27, 2010 at 02:46:32PM -0600, Alex Williamson wrote:
> On Wed, 2010-09-22 at 14:18 -0700, Tom Lyon wrote:
> > +ssize_t vfio_mem_readwrite(
> > +		int write,
> > +		struct vfio_dev *vdev,
> > +		char __user *buf,
> > +		size_t count,
> > +		loff_t *ppos)
> > +{
> > +	struct pci_dev *pdev = vdev->pdev;
> > +	resource_size_t end;
> > +	void __iomem *io;
> > +	loff_t pos;
> > +	int pci_space;
> > +
> > +	pci_space = vfio_offset_to_pci_space(*ppos);
> > +	pos = vfio_offset_to_pci_offset(*ppos);
> > +
> > +	if (!pci_resource_start(pdev, pci_space))
> > +		return -EINVAL;
> > +	end = pci_resource_len(pdev, pci_space);
> > +	if (vdev->barmap[pci_space] == NULL)
> > +		vdev->barmap[pci_space] = pci_iomap(pdev, pci_space, 0);
> > +	io = vdev->barmap[pci_space];
> > +
> 
> So we do a pci_iomap, but never do corresponding pci_iounmap.  This also
> only works for the first 6 BARs since the ROM BAR needs pci_map_rom.


An issue with ROM is that I think it can not be enabled together
with BARs. This is why pci_read_rom/pci_write_rom do what
they do.

>  I
> wonder if we should be doing all the BAR mapping at open and unmap at
> close so that we can fail if the device can't get basic resources.

I belive we should do this on ioctl so that e.g. hotunplug
can reset the device clean it up. Unused device should also not
consume resources.


> I
> believe we should also be calling pci_request_regions in here somewhere.
> Perhaps something like this:
> 
> diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c
> index a18e39a..d3886d9 100644
> --- a/drivers/vfio/vfio_main.c
> +++ b/drivers/vfio/vfio_main.c
> @@ -85,6 +85,53 @@ static inline int overlap(int a1, int b1, int a2, int b2)
>  	return !(b2 <= a1 || b1 <= a2);
>  }
>  
> +static int vfio_setup_pci(struct vfio_dev *vdev)
> +{
> +	int ret, bar;
> +
> +	ret = pci_enable_device(vdev->pdev);
> +	if (ret)
> +		return ret;
> +		
> +	ret = pci_request_regions(vdev->pdev, "VFIO");
> +	if (ret) {
> +		pci_disable_device(vdev->pdev);
> +		return ret;
> +	}
> +
> +	for (bar = PCI_STD_RESOURCES; bar <= PCI_ROM_RESOURCE; bar++) {
> +		if (!pci_resource_len(vdev->pdev, bar))
> +			continue;
> +		if (bar != PCI_ROM_RESOURCE) {
> +			if (!pci_resource_start(vdev->pdev, bar))
> +				continue;
> +			vdev->barmap[bar] = pci_iomap(vdev->pdev, bar, 0);
> +		} else {
> +			size_t size;
> +			vdev->barmap[bar] = pci_map_rom(vdev->pdev, &size);
> +		}
> +	}
> +	return ret;
> +}
> +
> +static void vfio_disable_pci(struct vfio_dev *vdev)
> +{
> +	int bar;
> +
> +	for (bar = PCI_STD_RESOURCES; bar <= PCI_ROM_RESOURCE; bar++) {
> +		if (!vdev->barmap[bar])
> +			continue;
> +		if (bar != PCI_ROM_RESOURCE)
> +			pci_iounmap(vdev->pdev, vdev->barmap[bar]);
> +		else
> +			pci_unmap_rom(vdev->pdev, vdev->barmap[bar]);
> +		vdev->barmap[bar] = NULL;
> +	}
> +
> +	pci_release_regions(vdev->pdev);
> +	pci_disable_device(vdev->pdev);
> +}
> +
>  static int vfio_open(struct inode *inode, struct file *filep)
>  {
>  	struct vfio_dev *vdev;
> @@ -110,7 +157,7 @@ static int vfio_open(struct inode *inode, struct file *filep)
>  	INIT_LIST_HEAD(&listener->dm_list);
>  	filep->private_data = listener;
>  	if (vdev->listeners == 0)
> -		ret = pci_enable_device(vdev->pdev);
> +		ret = vfio_setup_pci(vdev);
>  	if (ret == 0)
>  		vdev->listeners++;
>  	mutex_unlock(&vdev->lgate);
> @@ -151,7 +198,7 @@ static int vfio_release(struct inode *inode, struct file *filep)
>  		vdev->vconfig = NULL;
>  		kfree(vdev->pci_config_map);
>  		vdev->pci_config_map = NULL;
> -		pci_disable_device(vdev->pdev);
> +		vfio_disable_pci(vdev);
>  		vfio_domain_unset(vdev);
>  		wake_up(&vdev->dev_idle_q);
>  	}
> diff --git a/drivers/vfio/vfio_rdwr.c b/drivers/vfio/vfio_rdwr.c
> index 1fd50a6..7705b45 100644
> --- a/drivers/vfio/vfio_rdwr.c
> +++ b/drivers/vfio/vfio_rdwr.c
> @@ -64,7 +64,7 @@ ssize_t vfio_io_readwrite(
>  	if (pos + count > end)
>  		return -EINVAL;
>  	if (vdev->barmap[pci_space] == NULL)
> -		vdev->barmap[pci_space] = pci_iomap(pdev, pci_space, 0);
> +		return -EINVAL;
>  	io = vdev->barmap[pci_space];
>  
>  	while (count > 0) {
> @@ -137,7 +137,12 @@ ssize_t vfio_mem_readwrite(
>  		return -EINVAL;
>  	end = pci_resource_len(pdev, pci_space);
>  	if (vdev->barmap[pci_space] == NULL)
> -		vdev->barmap[pci_space] = pci_iomap(pdev, pci_space, 0);
> +		return -EINVAL;
> +	if (pci_space == PCI_ROM_RESOURCE) {
> +		u32 rom = *(u32 *)(vdev->vconfig + PCI_ROM_ADDRESS);
> +		if (!(rom & PCI_ROM_ADDRESS_ENABLE))
> +			return -EINVAL;
> +	}
>  	io = vdev->barmap[pci_space];
>  
>  	if (pos > end)
>