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

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

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.  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
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)