diff mbox

[v4,1/2] IOMMU/spinlock: Fix a bug found in AMD IOMMU initialization

Message ID 1457619007-41460-2-git-send-email-quan.xu@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Quan Xu March 10, 2016, 2:10 p.m. UTC 
pcidevs_lock doesn't require interrupts to be disabled while being acquired.
However there remains an exception in AMD IOMMU code, where the lock is
acquired with interrupt disabled. This inconsistency might lead to deadlock.

The fix is straightforward to use spin_lock instead. Also interrupt has been
enabled when this function is invoked, so we're sure consistency around
pcidevs_lock can be guaranteed after this fix.

Signed-off-by: Quan Xu <quan.xu@intel.com>
Reviewed-by: Dario Faggioli <dario.faggioli@citrix.com>
CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
CC: Dario Faggioli <dario.faggioli@citrix.com>
CC: Jan Beulich <jbeulich@suse.com>
CC: Kevin Tian <kevin.tian@intel.com>
---
 xen/drivers/passthrough/amd/iommu_init.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

Comments

Tian, Kevin March 11, 2016, 12:24 a.m. UTC | #1 
> From: Xu, Quan
> Sent: Thursday, March 10, 2016 10:10 PM
> 
> pcidevs_lock doesn't require interrupts to be disabled while being acquired.
> However there remains an exception in AMD IOMMU code, where the lock is
> acquired with interrupt disabled. This inconsistency might lead to deadlock.
> 
> The fix is straightforward to use spin_lock instead. Also interrupt has been
> enabled when this function is invoked, so we're sure consistency around
> pcidevs_lock can be guaranteed after this fix.
> 
> Signed-off-by: Quan Xu <quan.xu@intel.com>
> Reviewed-by: Dario Faggioli <dario.faggioli@citrix.com>
> CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> CC: Dario Faggioli <dario.faggioli@citrix.com>
> CC: Jan Beulich <jbeulich@suse.com>
> CC: Kevin Tian <kevin.tian@intel.com>

Reviewed-by: Kevin Tian <kevin.tian@intel.com>
Quan Xu March 11, 2016, 1:40 a.m. UTC | #2 
On March 11, 2016 8:24am, <Tian, Kevin> wrote:
> > From: Xu, Quan
> > Sent: Thursday, March 10, 2016 10:10 PM
> >
> > pcidevs_lock doesn't require interrupts to be disabled while being acquired.
> > However there remains an exception in AMD IOMMU code, where the lock
> > is acquired with interrupt disabled. This inconsistency might lead to deadlock.
> >
> > The fix is straightforward to use spin_lock instead. Also interrupt
> > has been enabled when this function is invoked, so we're sure
> > consistency around pcidevs_lock can be guaranteed after this fix.
> >
> > Signed-off-by: Quan Xu <quan.xu@intel.com>
> > Reviewed-by: Dario Faggioli <dario.faggioli@citrix.com>
> > CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> > CC: Dario Faggioli <dario.faggioli@citrix.com>
> > CC: Jan Beulich <jbeulich@suse.com>
> > CC: Kevin Tian <kevin.tian@intel.com>
> 
> Reviewed-by: Kevin Tian <kevin.tian@intel.com>

Kevin, thanks!!
Quan
diff mbox

Patch

diff --git a/xen/drivers/passthrough/amd/iommu_init.c b/xen/drivers/passthrough/amd/iommu_init.c
index d90a2d2..a400497 100644
--- a/xen/drivers/passthrough/amd/iommu_init.c
+++ b/xen/drivers/passthrough/amd/iommu_init.c
@@ -778,7 +778,6 @@  static bool_t __init set_iommu_interrupt_handler(struct amd_iommu *iommu)
 {
     int irq, ret;
     hw_irq_controller *handler;
-    unsigned long flags;
     u16 control;
 
     irq = create_irq(NUMA_NO_NODE);
@@ -788,10 +787,10 @@  static bool_t __init set_iommu_interrupt_handler(struct amd_iommu *iommu)
         return 0;
     }
 
-    spin_lock_irqsave(&pcidevs_lock, flags);
+    spin_lock(&pcidevs_lock);
     iommu->msi.dev = pci_get_pdev(iommu->seg, PCI_BUS(iommu->bdf),
                                   PCI_DEVFN2(iommu->bdf));
-    spin_unlock_irqrestore(&pcidevs_lock, flags);
+    spin_unlock(&pcidevs_lock);
     if ( !iommu->msi.dev )
     {
         AMD_IOMMU_DEBUG("IOMMU: no pdev for %04x:%02x:%02x.%u\n",