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

[Jan Beulich <jbeulich@xxxxxxxx>]

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().

Jan