Re: [PATCH v12.2 01/15] vpci: use per-domain PCI lock to protect vpci structure

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Wed, Jan 24, 2024 at 12:07:28AM -0500, Stewart Hildebrand wrote:
> On 1/23/24 09:29, Jan Beulich wrote:
> > On 15.01.2024 20:43, Stewart Hildebrand wrote:
> >> @@ -1043,11 +1043,11 @@ static int __pci_enable_msix(struct pci_dev *pdev, 
> >> struct msi_info *msi,
> >>  {
> >>      struct msi_desc *old_desc;
> >>  
> >> -    ASSERT(pcidevs_locked());
> >> -
> >>      if ( !pdev || !pdev->msix )
> >>          return -ENODEV;
> >>  
> >> +    ASSERT(pcidevs_locked() || rw_is_locked(&pdev->domain->pci_lock));
> >> +
> >>      if ( msi->entry_nr >= pdev->msix->nr_entries )
> >>          return -EINVAL;
> > 
> > Further looking at this - is dereferencing pdev actually safe without 
> > holding
> > the global lock?

It is safe because either the global pcidevs lock or the per-domain
pci_lock will be held, which should prevent the device from being
removed.

> Are you referring to the new placement of the ASSERT, which opens up the 
> possibility that pdev could be dereferenced and the function return before 
> the ASSERT? If that is what you mean, I see your point. The ASSERT was placed 
> there simply because we wanted to check that pdev != NULL first. See prior 
> discussion at [1]. Hmm.. How about splitting the pdev-checking condition? 
> E.g.:
> 
>     if ( !pdev )
>         return -ENODEV;
> 
>     ASSERT(pcidevs_locked() || rw_is_locked(&pdev->domain->pci_lock));
> 
>     if ( !pdev->msix )
>         return -ENODEV;

I'm not specially worried about the position of the assert, those are
just debug messages at the end.

One worry I have after further looking at the code, when called from
ns16550_init_postirq(), does the device have pdev->domain set?

That case would satisfy the first condition of the assert, so won't
attempt to dereference pdev->domain, but still would be good to ensure
consistency here wrt the state of pdev->domain.

Regards, Roger.