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

["Tian, Kevin" <kevin.tian@xxxxxxxxx>]

> From: Quan Xu
> Sent: Wednesday, March 09, 2016 11:08 AM

Hi, Quan, sorry that I still didn't quite get below description.

> 
> When iommu_setup() is called in __start_xen(), interrupts have
> already been enabled, and nothing disables (without reenabling)
> them again in the path that leads to calling
> set_iommu_interrupt_handler(). There's therefore no risk of them
> being disabled and needing remaining so, and hence no need for

no risk of them being 'enabled' since no one disables them again?

> saving and restoring the flags. This means that, even if interrupt
> would need to be disabled when taking pcidevs_lock, spin_lock_irq()
> and spin_unlock_irq() could be used.

I didn't see how this explanation relates to below change. You are
changing from spin_lock_irqsave to spin_lock. But here you explains
the reason to spin_lock_irq...

> 
> 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). I think it should be clear 
enough to state that pci_get_pdev doesn't require holding lock with
interrupt disabled (so we should use spin_lock in this AMD case),
and add the fact that when this function is invoked the interrupt
is indeed enabled.


> 
> In order to fix this, we can use just plain spin_lock() and spin_unlock(),
> instead of spin_lock_irqsave() and spin_unlock_irqrestore().

What about revise like below?
--

pcidevs_lock should be held with interrupt enabled. 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.
--

> 
> Signed-off-by: Quan Xu <quan.xu@xxxxxxxxx>
> Reviewed-by: Dario Faggioli <dario.faggioli@xxxxxxxxxx>
> CC: Suravee Suthikulpanit <suravee.suthikulpanit@xxxxxxx>
> CC: Dario Faggioli <dario.faggioli@xxxxxxxxxx>
> CC: Jan Beulich <jbeulich@xxxxxxxx>
> ---
>  xen/drivers/passthrough/amd/iommu_init.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> 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",
> --
> 1.9.1
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxx
> http://lists.xen.org/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel