Change the pcidevs_lock from rw_lock to spin_lock
authorKeir Fraser <keir.fraser@citrix.com>
Fri, 19 Dec 2008 14:52:32 +0000 (14:52 +0000)
committerKeir Fraser <keir.fraser@citrix.com>
Fri, 19 Dec 2008 14:52:32 +0000 (14:52 +0000)
As pcidevs_lock is changed from protecting only the alldevs_list to
more than that, it doesn't benifit too much from the rw_lock. Also the
previous patch 18906:2941b1a97c60 is wrong to use read_lock to protect some
sensitive data (thanks Espen pointed out that).

Also two minor fix in this patch:
a) deassign_device will deadlock when try to get the pcidevs_lock if
called by pci_release_devices, remove the lock to the caller.
b) The iommu_domain_teardown should not ASSERT for the pcidevs_lock
because it just update the domain's vt-d mapping.

Signed-off-by: Yunhong Jiang <yunhong.jiang@intel.com>
xen/arch/x86/domctl.c
xen/arch/x86/irq.c
xen/arch/x86/msi.c
xen/arch/x86/physdev.c
xen/drivers/passthrough/amd/pci_amd_iommu.c
xen/drivers/passthrough/iommu.c
xen/drivers/passthrough/pci.c
xen/drivers/passthrough/vtd/iommu.c
xen/include/xen/pci.h

index 8b4c40fea04f2ae65388a3949b09b649170f535b..d0b60ca5e94bfa4452e37fcb25c5e516c08989b8 100644 (file)
@@ -708,7 +708,9 @@ long arch_do_domctl(
             break;
         }
         ret = 0;
+        spin_lock(&pcidevs_lock);
         ret = deassign_device(d, bus, devfn);
+        spin_unlock(&pcidevs_lock);
         gdprintk(XENLOG_INFO, "XEN_DOMCTL_deassign_device: bdf = %x:%x:%x\n",
             bus, PCI_SLOT(devfn), PCI_FUNC(devfn));
 
index d45ab36e0d9a57a7d8cc213a81c81ed1ed2400b9..0ea4393116c9a2857c204056bc1389e86695b3f2 100644 (file)
@@ -850,7 +850,7 @@ int map_domain_pirq(
     struct msi_desc *msi_desc;
     struct pci_dev *pdev = NULL;
 
-    ASSERT(rw_is_locked(&pcidevs_lock));
+    ASSERT(spin_is_locked(&pcidevs_lock));
     ASSERT(spin_is_locked(&d->event_lock));
 
     if ( !IS_PRIV(current->domain) )
@@ -930,7 +930,7 @@ int unmap_domain_pirq(struct domain *d, int pirq)
     if ( !IS_PRIV(current->domain) )
         return -EINVAL;
 
-    ASSERT(rw_is_locked(&pcidevs_lock));
+    ASSERT(spin_is_locked(&pcidevs_lock));
     ASSERT(spin_is_locked(&d->event_lock));
 
     vector = d->arch.pirq_vector[pirq];
@@ -993,7 +993,7 @@ void free_domain_pirqs(struct domain *d)
 {
     int i;
 
-    read_lock(&pcidevs_lock);
+    spin_lock(&pcidevs_lock);
     spin_lock(&d->event_lock);
 
     for ( i = 0; i < NR_IRQS; i++ )
@@ -1001,7 +1001,7 @@ void free_domain_pirqs(struct domain *d)
             unmap_domain_pirq(d, i);
 
     spin_unlock(&d->event_lock);
-    read_unlock(&pcidevs_lock);
+    spin_unlock(&pcidevs_lock);
 }
 
 extern void dump_ioapic_irq_info(void);
index 0726e53aa9312422c766b6f1d2ecea2e0d73f2ec..30bf9052c4331c1777f22d040051f44d7a46e5fe 100644 (file)
@@ -440,7 +440,7 @@ static int msi_capability_init(struct pci_dev *dev,
     u8 slot = PCI_SLOT(dev->devfn);
     u8 func = PCI_FUNC(dev->devfn);
 
-    ASSERT(rw_is_locked(&pcidevs_lock));
+    ASSERT(spin_is_locked(&pcidevs_lock));
     pos = pci_find_cap_offset(bus, slot, func, PCI_CAP_ID_MSI);
     control = pci_conf_read16(bus, slot, func, msi_control_reg(pos));
     /* MSI Entry Initialization */
@@ -509,7 +509,7 @@ static int msix_capability_init(struct pci_dev *dev,
     u8 slot = PCI_SLOT(dev->devfn);
     u8 func = PCI_FUNC(dev->devfn);
 
-    ASSERT(rw_is_locked(&pcidevs_lock));
+    ASSERT(spin_is_locked(&pcidevs_lock));
     ASSERT(desc);
 
     pos = pci_find_cap_offset(bus, slot, func, PCI_CAP_ID_MSIX);
@@ -574,7 +574,7 @@ static int __pci_enable_msi(struct msi_info *msi, struct msi_desc **desc)
     int status;
     struct pci_dev *pdev;
 
-    ASSERT(rw_is_locked(&pcidevs_lock));
+    ASSERT(spin_is_locked(&pcidevs_lock));
     pdev = pci_get_pdev(msi->bus, msi->devfn);
     if ( !pdev )
         return -ENODEV;
@@ -634,7 +634,7 @@ static int __pci_enable_msix(struct msi_info *msi, struct msi_desc **desc)
     u8 slot = PCI_SLOT(msi->devfn);
     u8 func = PCI_FUNC(msi->devfn);
 
-    ASSERT(rw_is_locked(&pcidevs_lock));
+    ASSERT(spin_is_locked(&pcidevs_lock));
     pdev = pci_get_pdev(msi->bus, msi->devfn);
     if ( !pdev )
         return -ENODEV;
@@ -688,7 +688,7 @@ static void __pci_disable_msix(struct msi_desc *entry)
  */
 int pci_enable_msi(struct msi_info *msi, struct msi_desc **desc)
 {
-    ASSERT(rw_is_locked(&pcidevs_lock));
+    ASSERT(spin_is_locked(&pcidevs_lock));
 
     return  msi->table_base ? __pci_enable_msix(msi, desc) :
         __pci_enable_msi(msi, desc);
index f7ec6d7c816bdf7c1ab781ae04d71ad20f00c51c..fafe74f5462d57ed5f1e7d578ef444761462ed21 100644 (file)
@@ -100,7 +100,7 @@ static int physdev_map_pirq(struct physdev_map_pirq *map)
             goto free_domain;
     }
 
-    read_lock(&pcidevs_lock);
+    spin_lock(&pcidevs_lock);
     /* Verify or get pirq. */
     spin_lock(&d->event_lock);
     if ( map->pirq < 0 )
@@ -148,7 +148,7 @@ static int physdev_map_pirq(struct physdev_map_pirq *map)
 
 done:
     spin_unlock(&d->event_lock);
-    read_unlock(&pcidevs_lock);
+    spin_unlock(&pcidevs_lock);
     if ( (ret != 0) && (map->type == MAP_PIRQ_TYPE_MSI) && (map->index == -1) )
         free_irq_vector(vector);
 free_domain:
@@ -172,11 +172,11 @@ static int physdev_unmap_pirq(struct physdev_unmap_pirq *unmap)
     if ( d == NULL )
         return -ESRCH;
 
-    read_lock(&pcidevs_lock);
+    spin_lock(&pcidevs_lock);
     spin_lock(&d->event_lock);
     ret = unmap_domain_pirq(d, unmap->pirq);
     spin_unlock(&d->event_lock);
-    read_unlock(&pcidevs_lock);
+    spin_unlock(&pcidevs_lock);
 
     rcu_unlock_domain(d);
 
@@ -345,12 +345,12 @@ ret_t do_physdev_op(int cmd, XEN_GUEST_HANDLE(void) arg)
 
         irq_op.vector = assign_irq_vector(irq);
 
-        read_lock(&pcidevs_lock);
+        spin_lock(&pcidevs_lock);
         spin_lock(&dom0->event_lock);
         ret = map_domain_pirq(dom0, irq_op.irq, irq_op.vector,
                               MAP_PIRQ_TYPE_GSI, NULL);
         spin_unlock(&dom0->event_lock);
-        read_unlock(&pcidevs_lock);
+        spin_unlock(&pcidevs_lock);
 
         if ( copy_to_guest(arg, &irq_op, 1) != 0 )
             ret = -EFAULT;
index 88ab37154d53f9466e1f28c20aedc1c3bf22faa1..1957bdc4e7beff5b7e068672506f5f7eab9363e0 100644 (file)
@@ -126,7 +126,7 @@ static void amd_iommu_setup_dom0_devices(struct domain *d)
     u32 l;
     int bdf;
 
-    write_lock(&pcidevs_lock);
+    spin_lock(&pcidevs_lock);
     for ( bus = 0; bus < 256; bus++ )
     {
         for ( dev = 0; dev < 32; dev++ )
@@ -153,7 +153,7 @@ static void amd_iommu_setup_dom0_devices(struct domain *d)
             }
         }
     }
-    write_unlock(&pcidevs_lock);
+    spin_unlock(&pcidevs_lock);
 }
 
 int amd_iov_detect(void)
@@ -282,11 +282,11 @@ static int reassign_device( struct domain *source, struct domain *target,
     struct amd_iommu *iommu;
     int bdf;
 
-    read_lock(&pcidevs_lock);
+    spin_lock(&pcidevs_lock);
     pdev = pci_get_pdev_by_domain(source, bus, devfn);
     if ( !pdev )
     {
-        read_unlock(&pcidevs_lock);
+        spin_unlock(&pcidevs_lock);
         return -ENODEV;
     }
 
@@ -297,7 +297,7 @@ static int reassign_device( struct domain *source, struct domain *target,
 
     if ( !iommu )
     {
-        read_unlock(&pcidevs_lock);
+        spin_unlock(&pcidevs_lock);
         amd_iov_error("Fail to find iommu."
                      " %x:%x.%x cannot be assigned to domain %d\n", 
                      bus, PCI_SLOT(devfn), PCI_FUNC(devfn), target->domain_id);
@@ -314,7 +314,7 @@ static int reassign_device( struct domain *source, struct domain *target,
                  bus, PCI_SLOT(devfn), PCI_FUNC(devfn),
                  source->domain_id, target->domain_id);
 
-    read_unlock(&pcidevs_lock);
+    spin_unlock(&pcidevs_lock);
     return 0;
 }
 
index 13d275fbd3f37807d27a976b9c94064acf0a6528..22e2379c2714611327531f4bcc64193a0a03a2a9 100644 (file)
@@ -87,7 +87,7 @@ int iommu_add_device(struct pci_dev *pdev)
     if ( !pdev->domain )
         return -EINVAL;
 
-    ASSERT(rw_is_locked(&pcidevs_lock));
+    ASSERT(spin_is_locked(&pcidevs_lock));
 
     hd = domain_hvm_iommu(pdev->domain);
     if ( !iommu_enabled || !hd->platform_ops )
@@ -117,7 +117,7 @@ int assign_device(struct domain *d, u8 bus, u8 devfn)
     if ( !iommu_enabled || !hd->platform_ops )
         return 0;
 
-    read_lock(&pcidevs_lock);
+    spin_lock(&pcidevs_lock);
     if ( (rc = hd->platform_ops->assign_device(d, bus, devfn)) )
         goto done;
 
@@ -128,7 +128,7 @@ int assign_device(struct domain *d, u8 bus, u8 devfn)
         goto done;
     }
 done:    
-    read_unlock(&pcidevs_lock);
+    spin_unlock(&pcidevs_lock);
     return rc;
 }
 
@@ -211,7 +211,8 @@ int iommu_unmap_page(struct domain *d, unsigned long gfn)
     return hd->platform_ops->unmap_page(d, gfn);
 }
 
-int  deassign_device(struct domain *d, u8 bus, u8 devfn)
+/* caller should hold the pcidevs_lock */
+int deassign_device(struct domain *d, u8 bus, u8 devfn)
 {
     struct hvm_iommu *hd = domain_hvm_iommu(d);
     struct pci_dev *pdev = NULL;
@@ -219,20 +220,16 @@ int  deassign_device(struct domain *d, u8 bus, u8 devfn)
     if ( !iommu_enabled || !hd->platform_ops )
         return -EINVAL;
 
-    read_lock(&pcidevs_lock);
+    ASSERT(spin_is_locked(&pcidevs_lock));
     pdev = pci_get_pdev(bus, devfn);
     if (!pdev)
-    {
-        read_unlock(&pcidevs_lock);
         return -ENODEV;
-    }
 
     if (pdev->domain != d)
     {
-        read_unlock(&pcidevs_lock);
         gdprintk(XENLOG_ERR VTDPREFIX,
                 "IOMMU: deassign a device not owned\n");
-       return -EINVAL;
+        return -EINVAL;
     }
 
     hd->platform_ops->reassign_device(d, dom0, bus, devfn);
@@ -243,8 +240,6 @@ int  deassign_device(struct domain *d, u8 bus, u8 devfn)
         hd->platform_ops->teardown(d);
     }
 
-    read_unlock(&pcidevs_lock);
-
     return 0;
 }
 
@@ -288,7 +283,7 @@ int iommu_get_device_group(struct domain *d, u8 bus, u8 devfn,
 
     group_id = ops->get_device_group_id(bus, devfn);
 
-    read_lock(&pcidevs_lock);
+    spin_lock(&pcidevs_lock);
     for_each_pdev( d, pdev )
     {
         if ( (pdev->bus == bus) && (pdev->devfn == devfn) )
@@ -302,13 +297,13 @@ int iommu_get_device_group(struct domain *d, u8 bus, u8 devfn,
             bdf |= (pdev->devfn & 0xff) << 8;
             if ( unlikely(copy_to_guest_offset(buf, i, &bdf, 1)) )
             {
-                read_unlock(&pcidevs_lock);
+                spin_unlock(&pcidevs_lock);
                 return -1;
             }
             i++;
         }
     }
-    read_unlock(&pcidevs_lock);
+    spin_unlock(&pcidevs_lock);
 
     return i;
 }
index e6bd8da6ce5330a8e772dce2d3523f38a25f3908..ec49b3d9772ea5d075f8bf5518739ac2713b3130 100644 (file)
@@ -28,7 +28,7 @@
 
 
 LIST_HEAD(alldevs_list);
-rwlock_t pcidevs_lock = RW_LOCK_UNLOCKED;
+spinlock_t pcidevs_lock = SPIN_LOCK_UNLOCKED;
 
 struct pci_dev *alloc_pdev(u8 bus, u8 devfn)
 {
@@ -62,7 +62,7 @@ struct pci_dev *pci_get_pdev(int bus, int devfn)
 {
     struct pci_dev *pdev = NULL;
 
-    ASSERT(rw_is_locked(&pcidevs_lock));
+    ASSERT(spin_is_locked(&pcidevs_lock));
 
     list_for_each_entry ( pdev, &alldevs_list, alldevs_list )
         if ( (pdev->bus == bus || bus == -1) &&
@@ -78,7 +78,7 @@ struct pci_dev *pci_get_pdev_by_domain(struct domain *d, int bus, int devfn)
 {
     struct pci_dev *pdev = NULL;
 
-    ASSERT(rw_is_locked(&pcidevs_lock));
+    ASSERT(spin_is_locked(&pcidevs_lock));
 
     list_for_each_entry ( pdev, &alldevs_list, alldevs_list )
          if ( (pdev->bus == bus || bus == -1) &&
@@ -96,7 +96,7 @@ int pci_add_device(u8 bus, u8 devfn)
     struct pci_dev *pdev;
     int ret = -ENOMEM;
 
-    write_lock(&pcidevs_lock);
+    spin_lock(&pcidevs_lock);
     pdev = alloc_pdev(bus, devfn);
     if ( !pdev )
         goto out;
@@ -113,7 +113,7 @@ int pci_add_device(u8 bus, u8 devfn)
     }
 
 out:
-    write_unlock(&pcidevs_lock);
+    spin_unlock(&pcidevs_lock);
     printk(XENLOG_DEBUG "PCI add device %02x:%02x.%x\n", bus,
            PCI_SLOT(devfn), PCI_FUNC(devfn));
     return ret;
@@ -124,7 +124,7 @@ int pci_remove_device(u8 bus, u8 devfn)
     struct pci_dev *pdev;
     int ret = -ENODEV;;
 
-    write_lock(&pcidevs_lock);
+    spin_lock(&pcidevs_lock);
     list_for_each_entry ( pdev, &alldevs_list, alldevs_list )
         if ( pdev->bus == bus && pdev->devfn == devfn )
         {
@@ -138,7 +138,7 @@ int pci_remove_device(u8 bus, u8 devfn)
             break;
         }
 
-    write_unlock(&pcidevs_lock);
+    spin_unlock(&pcidevs_lock);
     return ret;
 }
 
@@ -187,7 +187,7 @@ void pci_release_devices(struct domain *d)
     struct pci_dev *pdev;
     u8 bus, devfn;
 
-    read_lock(&pcidevs_lock);
+    spin_lock(&pcidevs_lock);
     pci_clean_dpci_irqs(d);
     while ( (pdev = pci_get_pdev_by_domain(d, -1, -1)) )
     {
@@ -195,7 +195,7 @@ void pci_release_devices(struct domain *d)
         bus = pdev->bus; devfn = pdev->devfn;
         deassign_device(d, bus, devfn);
     }
-    read_unlock(&pcidevs_lock);
+    spin_unlock(&pcidevs_lock);
 }
 
 #ifdef SUPPORT_MSI_REMAPPING
@@ -205,7 +205,7 @@ static void dump_pci_devices(unsigned char ch)
     struct msi_desc *msi;
 
     printk("==== PCI devices ====\n");
-    read_lock(&pcidevs_lock);
+    spin_lock(&pcidevs_lock);
 
     list_for_each_entry ( pdev, &alldevs_list, alldevs_list )
     {
@@ -217,7 +217,7 @@ static void dump_pci_devices(unsigned char ch)
         printk(">\n");
     }
 
-    read_unlock(&pcidevs_lock);
+    spin_unlock(&pcidevs_lock);
 }
 
 static int __init setup_dump_pcidevs(void)
index 414b4c9f47fea8c12a8f43698518129d7bb83639..d2f0522dda2c79d79cf2b38bcdc7651349d3cc24 100644 (file)
@@ -1037,7 +1037,7 @@ static int domain_context_mapping_one(
     struct pci_dev *pdev = NULL;
     int agaw;
 
-    ASSERT(rw_is_locked(&pcidevs_lock));
+    ASSERT(spin_is_locked(&pcidevs_lock));
     spin_lock(&iommu->lock);
     maddr = bus_to_context_maddr(iommu, bus);
     context_entries = (struct context_entry *)map_vtd_domain_page(maddr);
@@ -1215,7 +1215,7 @@ static int domain_context_mapping(struct domain *domain, u8 bus, u8 devfn)
     if ( !drhd )
         return -ENODEV;
 
-    ASSERT(rw_is_locked(&pcidevs_lock));
+    ASSERT(spin_is_locked(&pcidevs_lock));
 
     type = pdev_type(bus, devfn);
     switch ( type )
@@ -1297,7 +1297,7 @@ static int domain_context_unmap_one(
     struct context_entry *context, *context_entries;
     u64 maddr;
 
-    ASSERT(rw_is_locked(&pcidevs_lock));
+    ASSERT(spin_is_locked(&pcidevs_lock));
     spin_lock(&iommu->lock);
 
     maddr = bus_to_context_maddr(iommu, bus);
@@ -1399,7 +1399,7 @@ static int reassign_device_ownership(
     struct iommu *pdev_iommu;
     int ret, found = 0;
 
-    ASSERT(rw_is_locked(&pcidevs_lock));
+    ASSERT(spin_is_locked(&pcidevs_lock));
     pdev = pci_get_pdev_by_domain(source, bus, devfn);
 
     if (!pdev)
@@ -1439,7 +1439,6 @@ void iommu_domain_teardown(struct domain *d)
     if ( list_empty(&acpi_drhd_units) )
         return;
 
-    ASSERT(rw_is_locked(&pcidevs_lock));
     spin_lock(&hd->mapping_lock);
     iommu_free_pagetable(hd->pgd_maddr, agaw_to_level(hd->agaw));
     hd->pgd_maddr = 0;
@@ -1529,7 +1528,7 @@ static int iommu_prepare_rmrr_dev(struct domain *d,
     u64 base, end;
     unsigned long base_pfn, end_pfn;
 
-    ASSERT(rw_is_locked(&pcidevs_lock));
+    ASSERT(spin_is_locked(&pcidevs_lock));
     ASSERT(rmrr->base_address < rmrr->end_address);
     
     base = rmrr->base_address & PAGE_MASK_4K;
@@ -1554,7 +1553,7 @@ static int intel_iommu_add_device(struct pci_dev *pdev)
     u16 bdf;
     int ret, i;
 
-    ASSERT(rw_is_locked(&pcidevs_lock));
+    ASSERT(spin_is_locked(&pcidevs_lock));
 
     if ( !pdev->domain )
         return -EINVAL;
@@ -1617,7 +1616,7 @@ static void setup_dom0_devices(struct domain *d)
 
     hd = domain_hvm_iommu(d);
 
-    write_lock(&pcidevs_lock);
+    spin_lock(&pcidevs_lock);
     for ( bus = 0; bus < 256; bus++ )
     {
         for ( dev = 0; dev < 32; dev++ )
@@ -1637,7 +1636,7 @@ static void setup_dom0_devices(struct domain *d)
             }
         }
     }
-    write_unlock(&pcidevs_lock);
+    spin_unlock(&pcidevs_lock);
 }
 
 void clear_fault_bits(struct iommu *iommu)
@@ -1710,7 +1709,7 @@ static void setup_dom0_rmrr(struct domain *d)
     u16 bdf;
     int ret, i;
 
-    read_lock(&pcidevs_lock);
+    spin_lock(&pcidevs_lock);
     for_each_rmrr_device ( rmrr, bdf, i )
     {
         ret = iommu_prepare_rmrr_dev(d, rmrr, PCI_BUS(bdf), PCI_DEVFN2(bdf));
@@ -1718,7 +1717,7 @@ static void setup_dom0_rmrr(struct domain *d)
             gdprintk(XENLOG_ERR VTDPREFIX,
                      "IOMMU: mapping reserved region failed\n");
     }
-    read_unlock(&pcidevs_lock);
+    spin_unlock(&pcidevs_lock);
 }
 
 int intel_vtd_setup(void)
@@ -1771,15 +1770,15 @@ int device_assigned(u8 bus, u8 devfn)
 {
     struct pci_dev *pdev;
 
-    read_lock(&pcidevs_lock);
+    spin_lock(&pcidevs_lock);
     pdev = pci_get_pdev_by_domain(dom0, bus, devfn);
     if (!pdev)
     {
-        read_unlock(&pcidevs_lock);
+        spin_unlock(&pcidevs_lock);
         return -1;
     }
 
-    read_unlock(&pcidevs_lock);
+    spin_unlock(&pcidevs_lock);
     return 0;
 }
 
@@ -1793,7 +1792,7 @@ int intel_iommu_assign_device(struct domain *d, u8 bus, u8 devfn)
     if ( list_empty(&acpi_drhd_units) )
         return -ENODEV;
 
-    ASSERT(rw_is_locked(&pcidevs_lock));
+    ASSERT(spin_is_locked(&pcidevs_lock));
     pdev = pci_get_pdev(bus, devfn);
     if (!pdev)
         return -ENODEV;
index 8951d481c443d0a2c1b5d0b737649c4ad6b1ff0b..8ca77b31653d23e36cbac465abc1b975ce085d65 100644 (file)
@@ -42,13 +42,12 @@ struct pci_dev {
     list_for_each_entry(pdev, &(domain->arch.pdev_list), domain_list)
 
 /*
- * The pcidevs_lock write-lock must be held when doing alloc_pdev() or
- * free_pdev().  Never de-reference pdev without holding pdev->lock or
- * pcidevs_lock.  Always aquire pcidevs_lock before pdev->lock when
- * doing free_pdev().
+ * The pcidevs_lock protect alldevs_list, and the assignment for the 
+ * devices, it also sync the access to the msi capability that is not
+ * interrupt handling related (the mask bit register).
  */
 
-extern rwlock_t pcidevs_lock;
+extern spinlock_t pcidevs_lock;
 
 struct pci_dev *alloc_pdev(u8 bus, u8 devfn);
 void free_pdev(struct pci_dev *pdev);