[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

[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:

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

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

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.

<<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



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.