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

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

On Thu, Sep 21, 2023 at 09:42:08AM +0200, Jan Beulich wrote:
> On 20.09.2023 15:56, Stewart Hildebrand wrote:
> > On 9/20/23 04:09, Roger Pau Monné wrote:
> >> On Tue, Sep 19, 2023 at 12:20:39PM -0400, Stewart Hildebrand wrote:
> >>> On 9/19/23 11:39, Roger Pau Monné wrote:
> >>>> On Tue, Aug 29, 2023 at 11:19:42PM +0000, Volodymyr Babchuk wrote:
> >>>>> diff --git a/xen/drivers/vpci/msi.c b/xen/drivers/vpci/msi.c
> >>>>> index 8f2b59e61a..a0733bb2cb 100644
> >>>>> --- a/xen/drivers/vpci/msi.c
> >>>>> +++ b/xen/drivers/vpci/msi.c
> >>>>> @@ -318,15 +321,28 @@ void vpci_dump_msi(void)
> >>>>>                       * holding the lock.
> >>>>>                       */
> >>>>>                      printk("unable to print all MSI-X entries: %d\n", 
> >>>>> rc);
> >>>>> -                    process_pending_softirqs();
> >>>>> -                    continue;
> >>>>> +                    goto pdev_done;
> >>>>>                  }
> >>>>>              }
> >>>>>
> >>>>>              spin_unlock(&pdev->vpci->lock);
> >>>>> + pdev_done:
> >>>>> +            /*
> >>>>> +             * Unlock lock to process pending softirqs. This is
> >>>>> +             * potentially unsafe, as d->pdev_list can be changed in
> >>>>> +             * meantime.
> >>>>> +             */
> >>>>> +            read_unlock(&d->pci_lock);
> >>>>>              process_pending_softirqs();
> >>>>> +            if ( !read_trylock(&d->pci_lock) )
> >>>>> +            {
> >>>>> +                printk("unable to access other devices for the 
> >>>>> domain\n");
> >>>>> +                goto domain_done;
> >>>>
> >>>> Shouldn't the domain_done label be after the read_unlock(), so that we
> >>>> can proceed to try to dump the devices for the next domain?  With the
> >>>> proposed code a failure to acquire one of the domains pci_lock
> >>>> terminates the dump.
> >>>>
> >>>>> +            }
> >>>>>          }
> >>>>> +        read_unlock(&d->pci_lock);
> >>>>>      }
> >>>>> + domain_done:
> >>>>>      rcu_read_unlock(&domlist_read_lock);
> >>>>>  }
> >>>>>
> >>>
> >>> With the label moved, a no-op expression after the label is needed to 
> >>> make the compiler happy:
> >>>
> >>>             }
> >>>         }
> >>>         read_unlock(&d->pci_lock);
> >>>  domain_done:
> >>>         (void)0;
> >>>     }
> >>>     rcu_read_unlock(&domlist_read_lock);
> >>> }
> >>>
> >>>
> >>> If the no-op is omitted, the compiler may complain (gcc 9.4.0):
> >>>
> >>> drivers/vpci/msi.c: In function ‘vpci_dump_msi’:
> >>> drivers/vpci/msi.c:351:2: error: label at end of compound statement
> >>>   351 |  domain_done:
> >>>       |  ^~~~~~~~~~~
> >>
> >>
> >> Might be better to place the label at the start of the loop, and
> >> likely rename to next_domain.
> > 
> > That would bypass the loop condition and increment statements.
> 
> Right, such a label would be bogus even without that; instead of "goto"
> the use site then simply should use "continue".

IIRC continue is not suitable because the code would reach the
read_unlock() without having the lock held.

Anyway, I would leave to the submitter to find a suitable way to
continue the domain iteration.

Thanks, Roger.