Re: [PATCH v6 03/13] vpci: move lock outside of struct vpci

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>]

On 07.02.22 14:46, Roger Pau Monné wrote:
> On Mon, Feb 07, 2022 at 11:08:39AM +0000, Oleksandr Andrushchenko wrote:
>> Hello,
>>
>> On 04.02.22 16:57, Roger Pau Monné wrote:
>>> On Fri, Feb 04, 2022 at 02:43:07PM +0000, Oleksandr Andrushchenko wrote:
>>>> On 04.02.22 15:06, Roger Pau Monné wrote:
>>>>> On Fri, Feb 04, 2022 at 12:53:20PM +0000, Oleksandr Andrushchenko wrote:
>>>>>> On 04.02.22 14:47, Jan Beulich wrote:
>>>>>>> On 04.02.2022 13:37, Oleksandr Andrushchenko wrote:
>>>>>>>> On 04.02.22 13:37, Jan Beulich wrote:
>>>>>>>>> On 04.02.2022 12:13, Roger Pau Monné wrote:
>>>>>>>>>> On Fri, Feb 04, 2022 at 11:49:18AM +0100, Jan Beulich wrote:
>>>>>>>>>>> On 04.02.2022 11:12, Oleksandr Andrushchenko wrote:
>>>>>>>>>>>> On 04.02.22 11:15, Jan Beulich wrote:
>>>>>>>>>>>>> On 04.02.2022 09:58, Oleksandr Andrushchenko wrote:
>>>>>>>>>>>>>> On 04.02.22 09:52, Jan Beulich wrote:
>>>>>>>>>>>>>>> On 04.02.2022 07:34, Oleksandr Andrushchenko wrote:
>>>>>>>>>>>>>>>> @@ -285,6 +286,12 @@ static int modify_bars(const struct 
>>>>>>>>>>>>>>>> pci_dev *pdev, uint16_t cmd, bool rom_only)
>>>>>>>>>>>>>>>>                         continue;
>>>>>>>>>>>>>>>>                 }
>>>>>>>>>>>>>>>>         
>>>>>>>>>>>>>>>> +        spin_lock(&tmp->vpci_lock);
>>>>>>>>>>>>>>>> +        if ( !tmp->vpci )
>>>>>>>>>>>>>>>> +        {
>>>>>>>>>>>>>>>> +            spin_unlock(&tmp->vpci_lock);
>>>>>>>>>>>>>>>> +            continue;
>>>>>>>>>>>>>>>> +        }
>>>>>>>>>>>>>>>>                 for ( i = 0; i < 
>>>>>>>>>>>>>>>> ARRAY_SIZE(tmp->vpci->header.bars); i++ )
>>>>>>>>>>>>>>>>                 {
>>>>>>>>>>>>>>>>                     const struct vpci_bar *bar = 
>>>>>>>>>>>>>>>> &tmp->vpci->header.bars[i];
>>>>>>>>>>>>>>>> @@ -303,12 +310,14 @@ static int modify_bars(const struct 
>>>>>>>>>>>>>>>> pci_dev *pdev, uint16_t cmd, bool rom_only)
>>>>>>>>>>>>>>>>                     rc = rangeset_remove_range(mem, start, 
>>>>>>>>>>>>>>>> end);
>>>>>>>>>>>>>>>>                     if ( rc )
>>>>>>>>>>>>>>>>                     {
>>>>>>>>>>>>>>>> +                spin_unlock(&tmp->vpci_lock);
>>>>>>>>>>>>>>>>                         printk(XENLOG_G_WARNING "Failed to 
>>>>>>>>>>>>>>>> remove [%lx, %lx]: %d\n",
>>>>>>>>>>>>>>>>                                start, end, rc);
>>>>>>>>>>>>>>>>                         rangeset_destroy(mem);
>>>>>>>>>>>>>>>>                         return rc;
>>>>>>>>>>>>>>>>                     }
>>>>>>>>>>>>>>>>                 }
>>>>>>>>>>>>>>>> +        spin_unlock(&tmp->vpci_lock);
>>>>>>>>>>>>>>>>             }
>>>>>>>>>>>>>>> At the first glance this simply looks like another unjustified 
>>>>>>>>>>>>>>> (in the
>>>>>>>>>>>>>>> description) change, as you're not converting anything here but 
>>>>>>>>>>>>>>> you
>>>>>>>>>>>>>>> actually add locking (and I realize this was there before, so 
>>>>>>>>>>>>>>> I'm sorry
>>>>>>>>>>>>>>> for not pointing this out earlier).
>>>>>>>>>>>>>> Well, I thought that the description already has "...the lock 
>>>>>>>>>>>>>> can be
>>>>>>>>>>>>>> used (and in a few cases is used right away) to check whether 
>>>>>>>>>>>>>> vpci
>>>>>>>>>>>>>> is present" and this is enough for such uses as here.
>>>>>>>>>>>>>>>         But then I wonder whether you
>>>>>>>>>>>>>>> actually tested this, since I can't help getting the impression 
>>>>>>>>>>>>>>> that
>>>>>>>>>>>>>>> you're introducing a live-lock: The function is called from 
>>>>>>>>>>>>>>> cmd_write()
>>>>>>>>>>>>>>> and rom_write(), which in turn are called out of vpci_write(). 
>>>>>>>>>>>>>>> Yet that
>>>>>>>>>>>>>>> function already holds the lock, and the lock is not (currently)
>>>>>>>>>>>>>>> recursive. (For the 3rd caller of the function - init_bars() - 
>>>>>>>>>>>>>>> otoh
>>>>>>>>>>>>>>> the locking looks to be entirely unnecessary.)
>>>>>>>>>>>>>> Well, you are correct: if tmp != pdev then it is correct to 
>>>>>>>>>>>>>> acquire
>>>>>>>>>>>>>> the lock. But if tmp == pdev and rom_only == true
>>>>>>>>>>>>>> then we'll deadlock.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> It seems we need to have the locking conditional, e.g. only lock
>>>>>>>>>>>>>> if tmp != pdev
>>>>>>>>>>>>> Which will address the live-lock, but introduce ABBA deadlock 
>>>>>>>>>>>>> potential
>>>>>>>>>>>>> between the two locks.
>>>>>>>>>>>> I am not sure I can suggest a better solution here
>>>>>>>>>>>> @Roger, @Jan, could you please help here?
>>>>>>>>>>> Well, first of all I'd like to mention that while it may have been 
>>>>>>>>>>> okay to
>>>>>>>>>>> not hold pcidevs_lock here for Dom0, it surely needs acquiring when 
>>>>>>>>>>> dealing
>>>>>>>>>>> with DomU-s' lists of PCI devices. The requirement really applies 
>>>>>>>>>>> to the
>>>>>>>>>>> other use of for_each_pdev() as well (in vpci_dump_msi()), except 
>>>>>>>>>>> that
>>>>>>>>>>> there it probably wants to be a try-lock.
>>>>>>>>>>>
>>>>>>>>>>> Next I'd like to point out that here we have the still pending 
>>>>>>>>>>> issue of
>>>>>>>>>>> how to deal with hidden devices, which Dom0 can access. See my RFC 
>>>>>>>>>>> patch
>>>>>>>>>>> "vPCI: account for hidden devices in modify_bars()". Whatever the 
>>>>>>>>>>> solution
>>>>>>>>>>> here, I think it wants to at least account for the extra need there.
>>>>>>>>>> Yes, sorry, I should take care of that.
>>>>>>>>>>
>>>>>>>>>>> Now it is quite clear that pcidevs_lock isn't going to help with 
>>>>>>>>>>> avoiding
>>>>>>>>>>> the deadlock, as it's imo not an option at all to acquire that lock
>>>>>>>>>>> everywhere else you access ->vpci (or else the vpci lock itself 
>>>>>>>>>>> would be
>>>>>>>>>>> pointless). But a per-domain auxiliary r/w lock may help: Other 
>>>>>>>>>>> paths
>>>>>>>>>>> would acquire it in read mode, and here you'd acquire it in write 
>>>>>>>>>>> mode (in
>>>>>>>>>>> the former case around the vpci lock, while in the latter case 
>>>>>>>>>>> there may
>>>>>>>>>>> then not be any need to acquire the individual vpci locks at all). 
>>>>>>>>>>> FTAOD:
>>>>>>>>>>> I haven't fully thought through all implications (and hence whether 
>>>>>>>>>>> this is
>>>>>>>>>>> viable in the first place); I expect you will, documenting what 
>>>>>>>>>>> you've
>>>>>>>>>>> found in the resulting patch description. Of course the double lock
>>>>>>>>>>> acquire/release would then likely want hiding in helper functions.
>>>>>>>>>> I've been also thinking about this, and whether it's really worth to
>>>>>>>>>> have a per-device lock rather than a per-domain one that protects all
>>>>>>>>>> vpci regions of the devices assigned to the domain.
>>>>>>>>>>
>>>>>>>>>> The OS is likely to serialize accesses to the PCI config space 
>>>>>>>>>> anyway,
>>>>>>>>>> and the only place I could see a benefit of having per-device locks 
>>>>>>>>>> is
>>>>>>>>>> in the handling of MSI-X tables, as the handling of the mask bit is
>>>>>>>>>> likely very performance sensitive, so adding a per-domain lock there
>>>>>>>>>> could be a bottleneck.
>>>>>>>>> Hmm, with method 1 accesses serializing globally is basically
>>>>>>>>> unavoidable, but with MMCFG I see no reason why OSes may not (move
>>>>>>>>> to) permit(ting) parallel accesses, with serialization perhaps done
>>>>>>>>> only at device level. See our own pci_config_lock, which applies to
>>>>>>>>> only method 1 accesses; we don't look to be serializing MMCFG
>>>>>>>>> accesses at all.
>>>>>>>>>
>>>>>>>>>> We could alternatively do a per-domain rwlock for vpci and special 
>>>>>>>>>> case
>>>>>>>>>> the MSI-X area to also have a per-device specific lock. At which 
>>>>>>>>>> point
>>>>>>>>>> it becomes fairly similar to what you propose.
>>>>>>>> @Jan, @Roger
>>>>>>>>
>>>>>>>> 1. d->vpci_lock - rwlock <- this protects vpci
>>>>>>>> 2. pdev->vpci->msix_tbl_lock - rwlock <- this protects MSI-X tables
>>>>>>>> or should it better be pdev->msix_tbl_lock as MSI-X tables don't
>>>>>>>> really depend on vPCI?
>>>>>>> If so, perhaps indeed better the latter. But as said in reply to Roger,
>>>>>>> I'm not convinced (yet) that doing away with the per-device lock is a
>>>>>>> good move. As said there - we're ourselves doing fully parallel MMCFG
>>>>>>> accesses, so OSes ought to be fine to do so, too.
>>>>>> But with pdev->vpci_lock we face ABBA...
>>>>> I think it would be easier to start with a per-domain rwlock that
>>>>> guarantees pdev->vpci cannot be removed under our feet. This would be
>>>>> taken in read mode in vpci_{read,write} and in write mode when
>>>>> removing a device from a domain.
>>>>>
>>>>> Then there are also other issues regarding vPCI locking that need to
>>>>> be fixed, but that lock would likely be a start.
>>>> Or let's see the problem at a different angle: this is the only place
>>>> which breaks the use of pdev->vpci_lock. Because all other places
>>>> do not try to acquire the lock of any two devices at a time.
>>>> So, what if we re-work the offending piece of code instead?
>>>> That way we do not break parallel access and have the lock per-device
>>>> which might also be a plus.
>>>>
>>>> By re-work I mean, that instead of reading already mapped regions
>>>> from tmp we can employ a d->pci_mapped_regions range set which
>>>> will hold all the already mapped ranges. And when it is needed to access
>>>> that range set we use pcidevs_lock which seems to be rare.
>>>> So, modify_bars will rely on pdev->vpci_lock + pcidevs_lock and
>>>> ABBA won't be possible at all.
>>> Sadly that won't replace the usage of the loop in modify_bars. This is
>>> not (exclusively) done in order to prevent mapping the same region
>>> multiple times, but rather to prevent unmapping of regions as long as
>>> there's an enabled BAR that's using it.
>>>
>>> If you wanted to use something like d->pci_mapped_regions it would
>>> have to keep reference counts to regions, in order to know when a
>>> mapping is no longer required by any BAR on the system with memory
>>> decoding enabled.
>> I missed this path, thank you
>>
>> I tried to analyze the locking in pci/vpci.
>>
>> First of all some context to refresh the target we want:
>> the rationale behind moving pdev->vpci->lock outside
>> is to be able dynamically create and destroy pdev->vpci.
>> So, for that reason lock needs to be moved outside of the pdev->vpci.
>>
>> Some of the callers of the vPCI code and locking used:
>>
>> ======================================
>> vpci_mmio_read/vpci_mmcfg_read
>> ======================================
>>     - vpci_ecam_read
>>     - vpci_read
>>      !!!!!!!! pdev is acquired, then pdev->vpci_lock is used !!!!!!!!
>>      - msix:
>>       - control_read
>>      - header:
>>       - guest_bar_read
>>      - msi:
>>       - control_read
>>       - address_read/address_hi_read
>>       - data_read
>>       - mask_read
>>
>> ======================================
>> vpci_mmio_write/vpci_mmcfg_write
>> ======================================
>>     - vpci_ecam_write
>>     - vpci_write
>>      !!!!!!!! pdev is acquired, then pdev->vpci_lock is used !!!!!!!!
>>      - msix:
>>       - control_write
>>      - header:
>>       - bar_write/guest_bar_write
>>       - cmd_write/guest_cmd_write
>>       - rom_write
>>        - all write handlers may call modify_bars
>>         modify_bars
>>      - msi:
>>       - control_write
>>       - address_write/address_hi_write
>>       - data_write
>>       - mask_write
>>
>> ======================================
>> pci_add_device: locked with pcidevs_lock
>> ======================================
>>     - vpci_add_handlers
>>      ++++++++ pdev->vpci_lock is used ++++++++
>>
>> ======================================
>> pci_remove_device: locked with pcidevs_lock
>> ======================================
>> - vpci_remove_device
>>     ++++++++ pdev->vpci_lock is used ++++++++
>> - pci_cleanup_msi
>> - free_pdev
>>
>> ======================================
>> XEN_DOMCTL_assign_device: locked with pcidevs_lock
>> ======================================
>> - assign_device
>>    - vpci_deassign_device
>>    - pdev_msix_assign
>>    - vpci_assign_device
>>     - vpci_add_handlers
>>       ++++++++ pdev->vpci_lock is used ++++++++
>>
>> ======================================
>> XEN_DOMCTL_deassign_device: locked with pcidevs_lock
>> ======================================
>> - deassign_device
>>    - vpci_deassign_device
>>      ++++++++ pdev->vpci_lock is used ++++++++
>>     - vpci_remove_device
>>
>>
>> ======================================
>> modify_bars is a special case: this is the only function which tries to lock
>> two pci_dev devices: it is done to check for overlaps with other BARs which 
>> may have been
>> already mapped or unmapped.
>>
>> So, this is the only case which may deadlock because of pci_dev->vpci_lock.
>> ======================================
>>
>> Bottom line:
>> ======================================
>>
>> 1. vpci_{read|write} are not protected with pcidevs_lock and can run in
>> parallel with pci_remove_device which can remove pdev after vpci_{read|write}
>> acquired the pdev pointer. This may lead to a fail due to pdev dereference.
>>
>> So, to protect pdev dereference vpci_{read|write} must also use pdevs_lock.
> We would like to take the pcidevs_lock only while fetching the device
> (ie: pci_get_pdev_by_domain), afterwards it should be fine to lock the
> device using a vpci specific lock so calls to vpci_{read,write} can be
> partially concurrent across multiple domains.
This means this can't be done a pre-req patch, but as a part of the
patch which changes locking.
>
> In fact I think Jan had already pointed out that the pci lock would
> need taking while searching for the device in vpci_{read,write}.
I was referring to the time after we found pdev and it is currently
possible to free pdev while using it after the search
>
> It seems to me that if you implement option 3 below taking the
> per-domain rwlock in read mode in vpci_{read|write} will already
> protect you from the device being removed if the same per-domain lock
> is taken in write mode in vpci_remove_device.
Yes, it should. Again this can't be done as a pre-req patch because
this relies on pdev->vpci_lock
>
>> 2. The only offending place which is in the way of pci_dev->vpci_lock is
>> modify_bars. If it can be re-worked to track already mapped and unmapped
>> regions then we can avoid having a possible deadlock and can use
>> pci_dev->vpci_lock (rangesets won't help here as we also need refcounting be
>> implemented).
> I think a refcounting based solution will be very complex to
> implement. I'm however happy to be proven wrong.
I can't estimate, but I have a feeling that all these plays around locking
is just because of this single piece of code. No other place suffer from
pdev->vpci_lock and no d->lock
>
>> If pcidevs_lock is used for vpci_{read|write} then no deadlock is possible,
>> but modify_bars code must be re-worked not to lock itself (pdev->vpci_lock 
>> and
>> tmp->vpci_lock when pdev == tmp, this is minor).
> Taking the pcidevs lock (a global lock) is out of the picture IMO, as
> it's going to serialize all calls of vpci_{read|write}, and would
> create too much contention on the pcidevs lock.
I understand that. But if we would like to fix the existing code I see
no other alternative.
>
>> 3. We may think about a per-domain rwlock and pdev->vpci_lock, so this solves
>> modify_bars's two pdevs access. But this doesn't solve possible pdev
>> de-reference in vpci_{read|write} vs pci_remove_device.
> pci_remove device will call vpci_remove_device, so as long as
> vpci_remove_device taken the per-domain lock in write (exclusive) mode
> it should be fine.
I think I need to see if there are any other places which similarly
require the write lock
>
>> @Roger, @Jan, I would like to hear what do you think about the above analysis
>> and how can we proceed with locking re-work?
> I think the per-domain rwlock seems like a good option. I would do
> that as a pre-patch.
It is. But it seems it won't solve the thing we started this adventure for:

With per-domain read lock and still ABBA in modify_bars (hope the below
is correctly seen with a monospace font):

cpu0: vpci_write-> d->RLock -> pdev1->lock ->                                   
               rom_write -> modify_bars: tmp (pdev2) ->lock
cpu1:        vpci_write-> d->RLock pdev2->lock -> cmd_write -> modify_bars: tmp 
(pdev1) ->lock

There is no API to upgrade read lock to write lock in modify_bars which could 
help,
so in both cases vpci_write should take write lock.

Am I missing something here?
>
> Thanks, Roger.
Thank you,
Oleksandr