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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

Jan