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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

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

Jan


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