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

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

On Mon, Mar 07, 2022 at 05:11:34PM +0100, Jan Beulich wrote:
> On 07.03.2022 17:06, Roger Pau Monné wrote:
> > 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.
> 
> Yeah, that looks to be the assumption. It has become my understanding
> that ACCESS_ONCE() is generally favored over {read,write}_atomic().
> Personally I prefer the latter when the goal is to have single insns.

Oh, OK, so I should use 'ACCESS_ONCE(msix->pba) = pba;' rather than
write_atomic then.

Thanks, Roger.