Re: [PATCH v3] vpci/msix: fix PBA accesses

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

On Mon, Mar 07, 2022 at 03:12:32PM +0100, Jan Beulich wrote:
> On 07.03.2022 13:53, Roger Pau Monne wrote:
> > --- a/xen/drivers/vpci/msix.c
> > +++ b/xen/drivers/vpci/msix.c
> > @@ -182,6 +182,33 @@ static struct vpci_msix_entry *get_entry(struct 
> > vpci_msix *msix,
> >      return &msix->entries[(addr - start) / PCI_MSIX_ENTRY_SIZE];
> >  }
> >  
> > +static void __iomem *get_pba(struct vpci *vpci)
> > +{
> > +    struct vpci_msix *msix = vpci->msix;
> > +    void __iomem *pba;
> > +
> > +    /*
> > +     * PBA will only be unmapped when the device is deassigned, so access 
> > it
> > +     * without holding the vpci lock.
> > +     */
> > +    if ( likely(msix->pba) )
> > +        return msix->pba;
> > +
> > +    pba = ioremap(vmsix_table_addr(vpci, VPCI_MSIX_PBA),
> > +                  vmsix_table_size(vpci, VPCI_MSIX_PBA));
> > +    if ( !pba )
> > +        return msix->pba;
> 
> For this particular purpose may want to consider using ACCESS_ONCE() for
> all accesses to this field.

Hm, I think I've asked before, but we do assume that ACCESS_ONCE will
generate a single instruction, or else we would have to use
read_atomic.

> > +    spin_lock(&vpci->lock);
> > +    if ( !msix->pba )
> > +        msix->pba = pba;

Here we would then use write_atomic.

> > +    else
> > +        iounmap(pba);
> > +    spin_unlock(&vpci->lock);
> 
> Whenever possible I think we should try to do things, in particular ones
> involving further locks, with as few locks held as possible. IOW I'd like
> to ask that you pull out the iounmap().
> 
> > @@ -200,6 +227,10 @@ static int cf_check msix_read(
> >  
> >      if ( VMSIX_ADDR_IN_RANGE(addr, msix->pdev->vpci, VPCI_MSIX_PBA) )
> >      {
> > +        struct vpci *vpci = msix->pdev->vpci;
> > +        unsigned int idx = addr - vmsix_table_addr(vpci, VPCI_MSIX_PBA);
> > +        void __iomem *pba = get_pba(vpci);
> 
> const?

Sure.

Thanks, Roger.