On Wed, Jul 26, 2023 at 01:17:58AM +0000, Volodymyr Babchuk wrote:
>
> Hi Roger,
>
> Roger Pau Monné <roger....@citrix.com> writes:
>
> > On Thu, Jul 20, 2023 at 12:32:31AM +0000, Volodymyr Babchuk wrote:
> >> From: Oleksandr Andrushchenko <oleksandr_andrushche...@epam.com>
> >> @@ -498,6 +537,7 @@ void vpci_write(pci_sbdf_t sbdf, unsigned int reg,
> >> unsigned int size,
> >> ASSERT(data_offset < size);
> >> }
> >> spin_unlock(&pdev->vpci->lock);
> >> + unlock_locks(d);
> >
> > There's one issue here, some handlers will cal pcidevs_lock(), which
> > will result in a lock over inversion, as in the previous patch we
> > agreed that the locking order was pcidevs_lock first, d->pci_lock
> > after.
> >
> > For example the MSI control_write() handler will call
> > vpci_msi_arch_enable() which takes the pcidevs lock. I think I will
> > have to look into using a dedicated lock for MSI related handling, as
> > that's the only place where I think we have this pattern of taking the
> > pcidevs_lock after the d->pci_lock.
>
> I'll mention this in the commit message. Is there something else that I
> should do right now?
Well, I don't think we want to commit this as-is with a known lock
inversion.
The functions that require the pcidevs lock are:
pt_irq_{create,destroy}_bind()
unmap_domain_pirq()
AFAICT those functions require the lock in order to assert that the
underlying device doesn't go away, as they do also use d->event_lock
in order to get exclusive access to the data fields. Please double
check that I'm not mistaken.
If that's accurate you will have to check the call tree that spawns
from those functions in order to modify the asserts to check for
either the pcidevs or the per-domain pci_list lock being taken.
Thanks, Roger.
