On Tue, Sep 19, 2023 at 05:55:42PM +0200, Jan Beulich wrote: > On 19.09.2023 17:39, Roger Pau Monné wrote: > > On Tue, Aug 29, 2023 at 11:19:42PM +0000, Volodymyr Babchuk wrote: > >> @@ -2908,7 +2908,13 @@ int allocate_and_map_msi_pirq(struct domain *d, int > >> index, int *pirq_p, > >> > >> msi->irq = irq; > >> > >> - pcidevs_lock(); > >> + /* > >> + * If we are called via vPCI->vMSI path, we already are holding > >> + * d->pci_lock so there is no need to take pcidevs_lock, as it > >> + * will cause lock inversion. > >> + */ > >> + if ( !rw_is_locked(&d->pci_lock) ) > >> + pcidevs_lock(); > > > > This is not a safe expression to use, rw_is_locked() just returns > > whether the lock is taken, but not if it's taken by the current CPU. > > This is fine to use in assertions and debug code, but not in order to > > take lock ordering decisions I'm afraid. > > > > You will likely need to move the locking to the callers of the > > function. > > Along the lines of a later comment, I think it would by rw_is_write_locked() > here anyway. Noting that xen/rwlock.h already has an internal > _is_write_locked_by_me(), it would in principle be possible to construct > something along the lines of what the comment says. But it would certainly > be better if that could be avoided.
I personally don't like construct like the above, they are fragile and should be avoided. It might be better to introduce some wrappers around allocate_and_map_msi_pirq() for the different locking contexts of callers if possible. Regards, Roger.
