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

["Xu, Quan" <quan.xu@xxxxxxxxx>]

On March 04, 2016 7:59am, <dario.faggioli@xxxxxxxxxx> wrote:
> 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.
> 

Dario, Welcome~  it's never too late.:)

> > @@ -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->bd
> f));
> > -    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, 

Yes, I fully agree your analysis and conclusion.
I tried to implement _irqsave() and _irqrestore() variants of recursive 
spinlocks, but I found that it is no need to add them.

Also I'd highlight the below modification:
-    if ( !spin_trylock(&pcidevs_lock) )
-        return -ERESTART;
-
+    pcidevs_lock();

IMO, it is right too.


> but I'd add a mention of this in the changelog.

In git log?


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