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

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

On Mon, Mar 07, 2022 at 05:17:59PM +0100, Jan Beulich wrote:
> On 07.03.2022 17:15, Roger Pau Monné wrote:
> > 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.
> 
> To please others, perhaps. As said, I'd be fine with you using
> {read,write}_atomic().

Well, given that you are the only one that has provided review I'm
fine with using {read,write}_atomic if that's also your preference.

Thanks, Roger.