Re: [PATCH v2 1/2] IOMMU/spinlock: Fix a bug found in AMD IOMMU initialization.

From: "Xu, Quan" <quan.xu@intel.com>
To: Jan Beulich <jbeulich@suse.com>
Cc: "dario.faggioli@citrix.com" <dario.faggioli@citrix.com>,
	"Tian, Kevin" <kevin.tian@intel.com>,
	"suravee.suthikulpanit@amd.com" <suravee.suthikulpanit@amd.com>,
	"xen-devel@lists.xen.org" <xen-devel@lists.xen.org>
Subject: Re: [PATCH v2 1/2] IOMMU/spinlock: Fix a bug found in AMD IOMMU initialization.
Date: Wed, 9 Mar 2016 12:18:28 +0000	[thread overview]
Message-ID: <945CA011AD5F084CBEA3E851C0AB28894B85E1AC@SHSMSX101.ccr.corp.intel.com> (raw)

On March 09, 2016 6:10pm, <jbeulich@suse.com> wrote:
> >>> "Xu, Quan" <quan.xu@intel.com> 03/09/16 8:32 AM >>>
> >On March 09, 2016 1:19pm, <Tian, Kevin> wrote:
> >> > Inside set_iommu_interrupt_handler(), pcidevs_lock serializes calls
> >> > to pci_get_pdev(), which does not require interrupts to be disabled
> >> > during its execution. In fact, pcidevs_lock is always (except for
> >> > this
> >> > case) taken without disabling interrupts, and disabling them in
> >> > this case would make the BUG_ON() in check_lock() trigger, if it
> >> > wasn't for the fact that spinlock debugging checks are still
> >> > disabled, during initialization, when iommu_setup() (which then end
> >> > up calling
> >> > set_iommu_interrupt_handler()) is called.
> >>
> >> The key problem is that we need consistent lock usage in all places
> >> (either all in IRQ-safe or all in IRQ-unsafe), regardless of whether
> >> check_lock is activated or not (which is just a debug method to help identify
> such inconsistency problem).
> >
> >IMO, this is not the key problem,  __Wait for Dario's / Jan's opinions__.
> 
> The inconsistency is one way of look at the problem, so with that it is kind of
> "key".
> 

Thanks for correcting me. 
Before this email, I really think inconsistency is one of the problem, and
The key problem is the case would make the BUG_ON() in check_lock() trigger. :(

> >> However there remains an
> >> exception in AMD IOMMU code, where the lock is acquired with
> >> interrupt disabled. This inconsistency can 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.
> >
> >I really appreciate you send out a revised one, but I think it is not only for
> consistency.
> >__Wait for Dario's / Jan's opinions__.
> >
> >To be honest, to me, __my_changlog_ is complicated.
> 
> I think Kevin's text above is okay. Perhaps weaken the "can lead to a deadlock"
> slightly, because that's just a theoretical concern, not one that's possible in
> practice on that code path.

Thanks.
I would use Kevin's text above and change 'can lead to a deadlock' to 'might lead to a deadlock' as changelog.

Quan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel