Re: [PATCH v9 02/16] vpci: use per-domain PCI lock to protect vpci structure

["Roger Pau Monné" <roger.pau@citrix.com>]

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.