Thanks, Alex!
Am incorporating...
On Tuesday 29 June 2010 11:14:12 pm Alex Williamson wrote:
> On Tue, 2010-06-08 at 14:21 -0700, Tom Lyon wrote:
> > The VFIO "driver" is used to allow privileged AND non-privileged processes to
> > implement user-level device drivers for any well-behaved PCI, PCI-X, and PCIe
> > devices.
>
> Hi Tom,
>
> I found a few bugs. Patch below. The first chunk clears the
> pci_config_map on close, otherwise we end up passing virtualized state
> from one user to the next. The second is an off by one in the basic
> perms. Finally, vfio_bar_fixup() needs an overhaul. It wasn't setting
> the lower bits right and is allowing virtual writes of bits that aren't
> aligned to the size. This section probably needs another pass or two of
> refinement. Thanks,
>
> Alex
>
> Signed-off-by: Alex Williamson <alex.williamson@xxxxxxxxxx>
> ---
>
> diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c
> index 96639e5..a0e8227 100644
> --- a/drivers/vfio/vfio_main.c
> +++ b/drivers/vfio/vfio_main.c
> @@ -129,6 +129,10 @@ static int vfio_release(struct inode *inode, struct file *filep)
> eventfd_ctx_put(vdev->ev_msi);
> vdev->ev_irq = NULL;
> }
> + if (vdev->pci_config_map) {
> + kfree(vdev->pci_config_map);
> + vdev->pci_config_map = NULL;
> + }
> vfio_domain_unset(vdev);
> /* reset to known state if we can */
> (void) pci_reset_function(vdev->pdev);
> diff --git a/drivers/vfio/vfio_pci_config.c b/drivers/vfio/vfio_pci_config.c
> index c821b5d..f6e26b1 100644
> --- a/drivers/vfio/vfio_pci_config.c
> +++ b/drivers/vfio/vfio_pci_config.c
> @@ -79,18 +79,18 @@ struct perm_bits {
> static struct perm_bits pci_cap_basic_perm[] = {
> { 0xFFFFFFFF, 0, }, /* 0x00 vendor & device id - RO */
> { 0, 0xFFFFFFFC, }, /* 0x04 cmd & status except mem/io */
> - { 0, 0xFF00FFFF, }, /* 0x08 bist, htype, lat, cache */
> - { 0xFFFFFFFF, 0xFFFFFFFF, }, /* 0x0c bar */
> + { 0, 0, }, /* 0x08 class code & revision id */
> + { 0, 0xFF00FFFF, }, /* 0x0c bist, htype, lat, cache */
> { 0xFFFFFFFF, 0xFFFFFFFF, }, /* 0x10 bar */
> { 0xFFFFFFFF, 0xFFFFFFFF, }, /* 0x14 bar */
> { 0xFFFFFFFF, 0xFFFFFFFF, }, /* 0x18 bar */
> { 0xFFFFFFFF, 0xFFFFFFFF, }, /* 0x1c bar */
> { 0xFFFFFFFF, 0xFFFFFFFF, }, /* 0x20 bar */
> - { 0, 0, }, /* 0x24 cardbus - not yet */
> - { 0, 0, }, /* 0x28 subsys vendor & dev */
> - { 0xFFFFFFFF, 0xFFFFFFFF, }, /* 0x2c rom bar */
> - { 0, 0, }, /* 0x30 capability ptr & resv */
> - { 0, 0, }, /* 0x34 resv */
> + { 0xFFFFFFFF, 0xFFFFFFFF, }, /* 0x24 bar */
> + { 0, 0, }, /* 0x28 cardbus - not yet */
> + { 0, 0, }, /* 0x2c subsys vendor & dev */
> + { 0xFFFFFFFF, 0xFFFFFFFF, }, /* 0x30 rom bar */
> + { 0, 0, }, /* 0x34 capability ptr & resv */
> { 0, 0, }, /* 0x38 resv */
> { 0x000000FF, 0x000000FF, }, /* 0x3c max_lat ... irq */
> };
> @@ -318,30 +318,55 @@ static void vfio_virt_init(struct vfio_dev *vdev)
> static void vfio_bar_fixup(struct vfio_dev *vdev)
> {
> struct pci_dev *pdev = vdev->pdev;
> - int bar;
> - u32 *lp;
> - u32 len;
> + int bar, mem64 = 0;
> + u32 *lp = NULL;
> + u64 len = 0;
>
> for (bar = 0; bar <= 5; bar++) {
> - len = pci_resource_len(pdev, bar);
> - lp = (u32 *)&vdev->vinfo.bar[bar * 4];
> - if (len == 0) {
> - *lp = 0;
> - } else if (pci_resource_flags(pdev, bar) & IORESOURCE_MEM) {
> - *lp &= ~0x1;
> - *lp = (*lp & ~(len-1)) |
> - (*lp & ~PCI_BASE_ADDRESS_MEM_MASK);
> - if (*lp & PCI_BASE_ADDRESS_MEM_TYPE_64)
> - bar++;
> - } else if (pci_resource_flags(pdev, bar) & IORESOURCE_IO) {
> + if (!mem64) {
> + len = pci_resource_len(pdev, bar);
> + lp = (u32 *)&vdev->vinfo.bar[bar * 4];
> + if (len == 0) {
> + *lp = 0;
> + continue;
> + }
> +
> + len = ~(len - 1);
> + } else
> + len >>= 32;
> +
> + if (*lp == ~0U)
> + *lp = (u32)len;
> + else
> + *lp &= (u32)len;
> +
> + if (mem64) {
> + mem64 = 0;
> + continue;
> + }
> +
> + if (pci_resource_flags(pdev, bar) & IORESOURCE_IO)
> *lp |= PCI_BASE_ADDRESS_SPACE_IO;
> - *lp = (*lp & ~(len-1)) |
> - (*lp & ~PCI_BASE_ADDRESS_IO_MASK);
> + else {
> + *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;
> + mem64 = 1;
> + }
> }
> }
> +
> lp = (u32 *)vdev->vinfo.rombar;
> len = pci_resource_len(pdev, PCI_ROM_RESOURCE);
> - *lp = *lp & PCI_ROM_ADDRESS_MASK & ~(len-1);
> + len = ~(len - 1);
> +
> + if (*lp == ~PCI_ROM_ADDRESS_ENABLE)
> + *lp = (u32)len;
> + else
> + *lp = *lp & ((u32)len | PCI_ROM_ADDRESS_ENABLE);
> +
> vdev->vinfo.bardirty = 0;
> }
>
>
>
>
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
