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

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

On March 04, 2016 5:29pm, <JBeulich@xxxxxxxx> wrote:
> >>> On 04.03.16 at 03:45, <quan.xu@xxxxxxxxx> wrote:
> > On March 04, 2016 7:59am, <dario.faggioli@xxxxxxxxxx> wrote:
> >> On Wed, 2016-03-02 at 22:31 +0800, Quan Xu wrote:
> >> > @@ -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,
> >
> > 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.
> 
> Well, I'll have to see where exactly this is (pulling such out of context is 
> pretty
> unhelpful), but I suspect it can't be replaced like this.
> 

Jan, I am looking forward to your review.
btw, It is in the assign_device(), in the xen/drivers/passthrough/pci.c file.

> >> but I'd add a mention of this in the changelog.
> >
> > In git log?
> 
> To be honest, changes like this would better not be buried in a big rework 
> like
> the one here. Make it a prereq patch, where you then will kind of 
> automatically
> describe why it's correct. (I agree current code is bogus, and we're not 
> hitting
> the respective
> BUG_ON() in check_lock() only because spin_debug gets set to true only after
> most __init code has run.)

Agreed, I would make a prereq patch in v7.

Quan

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