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

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

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.

Thanks, Roger.