From mboxrd@z Thu Jan  1 00:00:00 1970
From: Alex Williamson <alex.williamson@redhat.com>
Subject: Re: [PATCH 3/3] VFIO V4: VFIO driver: Non-privileged user level
 PCI drivers
Date: Mon, 27 Sep 2010 14:41:13 -0600
Message-ID: <1285620073.4951.44.camel@x201>
References: <4c9a72a0.u2cnjUB7QZ91tLeo%pugs@cisco.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
Cc: linux-pci@vger.kernel.org, jbarnes@virtuousgeek.org,
	linux-kernel@vger.kernel.org, kvm@vger.kernel.org,
	randy.dunlap@oracle.com, arnd@arndb.de, joro@8bytes.org,
	hjk@linutronix.de, avi@redhat.com, gregkh@suse.de,
	chrisw@sous-sol.org, mst@redhat.com
To: Tom Lyon <pugs@cisco.com>
Return-path: <linux-kernel-owner@vger.kernel.org>
In-Reply-To: <4c9a72a0.u2cnjUB7QZ91tLeo%pugs@cisco.com>
Sender: linux-kernel-owner@vger.kernel.org
List-Id: kvm.vger.kernel.org

On Wed, 2010-09-22 at 14:18 -0700, Tom Lyon wrote:
> +/*
> + * Pretend we're hardware and tweak the values
> + * of the *virtual* pci BARs to reflect the hardware
> + * capabilities
> + */
> +static void vfio_bar_fixup(struct vfio_dev *vdev)
> +{
> +	struct pci_dev *pdev = vdev->pdev;
> +	int bar;
> +	u32 *lp;
> +	u64 mask;
> +
> +	for (bar = 0; bar <= 5; bar++) {
> +		if (pci_resource_start(pdev, bar))
> +			mask = ~(pci_resource_len(pdev, bar) - 1);
> +		else
> +			mask = 0;
> +		lp = (u32 *)vdev->vconfig + PCI_BASE_ADDRESS_0 + 4*bar;
> +		*lp &= (u32)mask;
> +
> +		if (pci_resource_flags(pdev, bar) & IORESOURCE_IO)
> +			*lp |= PCI_BASE_ADDRESS_SPACE_IO;
> +		else if (pci_resource_flags(pdev, bar) & IORESOURCE_MEM) {
> +			*lp |= PCI_BASE_ADDRESS_SPACE_MEMORY;
> +			if (pci_resource_flags(pdev, bar) & IORESOURCE_PREFETCH)
> +				*lp |= PCI_BASE_ADDRESS_MEM_PREFETCH;
> +			if (pci_resource_flags(pdev, bar) & IORESOURCE_MEM_64) {
> +				*lp |= PCI_BASE_ADDRESS_MEM_TYPE_64;
> +				lp++;
> +				*lp &= (u32)(mask >> 32);
> +				bar++;
> +			}
> +		}
> +	}
> +
> +	if (pci_resource_start(pdev, PCI_ROM_RESOURCE))
> +		mask = ~(pci_resource_len(pdev, PCI_ROM_RESOURCE) - 1);
> +	else
> +		mask = 0;
> +	lp = (u32 *)vdev->vconfig + PCI_ROM_ADDRESS;
> +	*lp &= (u32)mask;
> +
> +	vdev->bardirty = 0;
> +}

Hey Tom,

A couple bugs have snuck into the above.  The (u32 *) cast needs to be
done after the vconfig index, and for some reason we're no longer
preserving the ROM enable bit.  I think we need something like this:


diff --git a/drivers/vfio/vfio_pci_config.c b/drivers/vfio/vfio_pci_config.c
index b7de0bf..b1ee352 100644
--- a/drivers/vfio/vfio_pci_config.c
+++ b/drivers/vfio/vfio_pci_config.c
@@ -402,7 +402,7 @@ static void vfio_bar_fixup(struct vfio_dev *vdev)
 			mask = ~(pci_resource_len(pdev, bar) - 1);
 		else
 			mask = 0;
-		lp = (u32 *)vdev->vconfig + PCI_BASE_ADDRESS_0 + 4*bar;
+		lp = (u32 *)(vdev->vconfig + PCI_BASE_ADDRESS_0 + 4*bar);
 		*lp &= (u32)mask;
 
 		if (pci_resource_flags(pdev, bar) & IORESOURCE_IO)
@@ -420,11 +420,12 @@ static void vfio_bar_fixup(struct vfio_dev *vdev)
 		}
 	}
 
-	if (pci_resource_start(pdev, PCI_ROM_RESOURCE))
+	if (pci_resource_start(pdev, PCI_ROM_RESOURCE)) {
 		mask = ~(pci_resource_len(pdev, PCI_ROM_RESOURCE) - 1);
-	else
+		mask |= PCI_ROM_ADDRESS_ENABLE;
+	} else
 		mask = 0;
-	lp = (u32 *)vdev->vconfig + PCI_ROM_ADDRESS;
+	lp = (u32 *)(vdev->vconfig + PCI_ROM_ADDRESS);
 	*lp &= (u32)mask;
 
 	vdev->bardirty = 0;