Re: [PATCH v7 01/12] vpci: introduce per-domain lock to protect vpci structure

[Jan Beulich <jbeulich@xxxxxxxx>]

On 04.07.2023 23:03, Volodymyr Babchuk wrote:
> I am currently implementing your proposal (along with Jan's
> suggestions), but I am facing ABBA deadlock with IOMMU's
> reassign_device() call, which has this piece of code:
> 
>         list_move(&pdev->domain_list, &target->pdev_list);
> 
> My immediate change was:
> 
>         write_lock(&pdev->domain->pci_lock);
>         list_del(&pdev->domain_list);
>         write_unlock(&pdev->domain->pci_lock);
> 
>         write_lock(&target->pci_lock);
>         list_add(&pdev->domain_list, &target->pdev_list);
>         write_unlock(&target->pci_lock);
> 
> But this will not work because reassign_device is called from
> pci_release_devices() which iterates over d->pdev_list, so we need to
> take a d->pci_lock early.
> 
> Any suggestions on how to fix this? My idea is to remove a device from a
> list one at time:
> 
> int pci_release_devices(struct domain *d)
> {
>     struct pci_dev *pdev;
>     u8 bus, devfn;
>     int ret;
> 
>     pcidevs_lock();
>     write_lock(&d->pci_lock);
>     ret = arch_pci_clean_pirqs(d);
>     if ( ret )
>     {
>         pcidevs_unlock();
>         write_unlock(&d->pci_lock);
>         return ret;
>     }
> 
>     while ( !list_empty(&d->pdev_list) )
>     {
>         pdev = list_entry(&d->pdev_list, struct pci_dev, domain_list);
>         bus = pdev->bus;
>         devfn = pdev->devfn;
>         list_del(&pdev->domain_list);
>         write_unlock(&d->pci_lock);
>         ret = deassign_device(d, pdev->seg, bus, devfn) ?: ret;
>         write_lock(&d->pci_lock);

I think it needs doing almost like this, but with two more tweaks and
no list_del() right here (first and foremost to avoid needing to
figure whether removing early isn't going to subtly break anything;
see below for an error case that would end up with changed behavior):

    while ( !list_empty(&d->pdev_list) )
    {
        const struct pci_dev *pdev = list_first_entry(&d->pdev_list, struct 
pci_dev, domain_list);
        uint16_t seg = pdev->seg;
        uint8_t bus = pdev->bus;
        uint8_t devfn = pdev->devfn;

        write_unlock(&d->pci_lock);
        ret = deassign_device(d, seg, bus, devfn) ?: ret;
        write_lock(&d->pci_lock);
    }

One caveat though: The original list_for_each_entry_safe() guarantees
the loop to complete; your use of list_del() would guarantee that too,
but then the device wouldn't be on the list anymore if deassign_device()
failed. Therefore I guess you will need another local list where you
(temporarily) put all the devices which deassign_device() left on the
list, and which you would then move back to d->pdev_list after the loop
has finished. (Whether it is sufficient to inspect the list head to
determine whether the pdev is still on the list needs careful checking.)

Jan