Re: [Xen-devel] [PATCH v6 3/5] IOMMU: Make the pcidevs_lock a recursive one

[Dario Faggioli <dario.faggioli@xxxxxxxxxx>]

[I've removed some of the people that shouldn't be involved in this
discussion any longer from the Cc-list]

On Wed, 2016-03-02 at 22:31 +0800, Quan Xu wrote:
> Signed-off-by: Quan Xu <quan.xu@xxxxxxxxx>
> 
So, this patch looks ok to me.

I spotted something, though, that I think needs some attention.

Since I'm jumping on this series only now, if this has been discussed
before and I missed it, sorry for the noise.

> @@ -788,10 +787,10 @@ static bool_t __init
> set_iommu_interrupt_handler(struct amd_iommu *iommu)
>          return 0;
>      }
>  
> -    spin_lock_irqsave(&pcidevs_lock, flags);
> +    pcidevs_lock();
>
So, spin_lock_irqsave() does:
  local_irq_save()
    local_save_flags()
    local_irq_disable()
  _spin_lock()

i.e., it saves the flags and disable interrupts.

pcidevs_lock() does:
  spin_lock_recursive()
    ... //handle recursion
    _spin_lock()

i.e., it does not disable interrupts.

And therefore it is possible that we are actually skipping disabling
interrupts (if they're not disabled already), isn't it?

And, of course, the same reasoning --mutatis mutandis-- applies to the
unlock side of things.

>      iommu->msi.dev = pci_get_pdev(iommu->seg, PCI_BUS(iommu->bdf),
>                                    PCI_DEVFN2(iommu->bdf));
> -    spin_unlock_irqrestore(&pcidevs_lock, flags);
> +    pcidevs_unlock();
>
i.e., spin_unlock_irqrestore() restore the flags, including the
interrupt enabled/disabled status, which means it can re-enable the
interrupts or not, depending on whether they were enabled at the time
of the previous spin_lock_irqsave(); pcidevs_unlock() just don't affect
interrupt enabling/disabling at all.

So, if the original code is correct in using
spin_lock_irqsave()/spin_unlock_irqrestore(), I think that we need
_irqsave() and _irqrestore() variants of recursive spinlocks, in order
to deal with this case.

However, from a quick inspection, it looks to me that:
 - this can only be called (during initialization), with interrupt
   enabled, so least saving & restoring flags shouldn't be necessary
   (unless I missed where they can be disabled in the call chain
   from iommu_setup() toward set_iommu_interrupt_handler()).
 - This protects pci_get_dev(); looking at other places where 
   pci_get_dev() is called, I don't think it is really necessary to
   disable interrupts.

If I'm right, it means that the original code could well have been
using just plain spin_lock() and spin_unlock(), and it would then be
fine to turn them into pcidevs_lock() and pcidevs_unlock(), and so no
need to add more spin_[un]lock_recursive() variants.

That would also mean that the patch is indeed ok, but I'd add a mention
of this in the changelog.

Regards,
Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)

Attachment: signature.asc
Description: This is a digitally signed message part

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