Re: [PATCH v3 2/3] vfio/pci: Support 8-byte PCI loads and stores

Ramesh Thomas
2024-05-22T23:57:58Z

On 5/21/2024 9:40 AM, Gerd Bayer wrote:
> On Fri, 2024-05-17 at 03:29 -0700, Ramesh Thomas wrote:
>> On 4/25/2024 9:56 AM, Gerd Bayer wrote:
>>> From: Ben Segal <bpsegal@us.ibm.com>
>>>
>>> Many PCI adapters can benefit or even require full 64bit read
>>> and write access to their registers. In order to enable work on
>>> user-space drivers for these devices add two new variations
>>> vfio_pci_core_io{read|write}64 of the existing access methods
>>> when the architecture supports 64-bit ioreads and iowrites.
>>
>> This is indeed necessary as back to back 32 bit may not be optimal in
>> some devices.
>>
>>>
>>> Signed-off-by: Ben Segal <bpsegal@us.ibm.com>
>>> Co-developed-by: Gerd Bayer <gbayer@linux.ibm.com>
>>> Signed-off-by: Gerd Bayer <gbayer@linux.ibm.com>
>>> ---
>>>   drivers/vfio/pci/vfio_pci_rdwr.c | 16 ++++++++++++++++
>>>   include/linux/vfio_pci_core.h    |  3 +++
>>>   2 files changed, 19 insertions(+)
>>>
>>> diff --git a/drivers/vfio/pci/vfio_pci_rdwr.c
>>> b/drivers/vfio/pci/vfio_pci_rdwr.c
>>> index 3335f1b868b1..8ed06edaee23 100644
>>> --- a/drivers/vfio/pci/vfio_pci_rdwr.c
>>> +++ b/drivers/vfio/pci/vfio_pci_rdwr.c
>>> @@ -89,6 +89,9 @@ EXPORT_SYMBOL_GPL(vfio_pci_core_ioread##size);
>>>   VFIO_IOREAD(8)
>>>   VFIO_IOREAD(16)
>>>   VFIO_IOREAD(32)
>>> +#ifdef ioread64
>>> +VFIO_IOREAD(64)
>>> +#endif
>>>
>>>   #define
>>> VFIO_IORDWR(size) \
>>>   static int vfio_pci_core_iordwr##size(struct vfio_pci_core_device
>>> *vdev,\
>>> @@ -124,6 +127,10 @@ static int vfio_pci_core_iordwr##size(struct
>>> vfio_pci_core_device *vdev,\
>>>   VFIO_IORDWR(8)
>>>   VFIO_IORDWR(16)
>>>   VFIO_IORDWR(32)
>>> +#if defined(ioread64) && defined(iowrite64)
>>> +VFIO_IORDWR(64)
>>> +#endif
>>> +
>>>   /*
>>>    * Read or write from an __iomem region (MMIO or I/O port) with
>>> an excluded
>>>    * range which is inaccessible.  The excluded range drops writes
>>> and fills
>>> @@ -148,6 +155,15 @@ ssize_t vfio_pci_core_do_io_rw(struct
>>> vfio_pci_core_device *vdev, bool test_mem,
>>>    else
>>>    fillable = 0;
>>>
>>> +#if defined(ioread64) && defined(iowrite64)
>>
>> Can we check for #ifdef CONFIG_64BIT instead? In x86, ioread64 and
>> iowrite64 get declared as extern functions if CONFIG_GENERIC_IOMAP is
>> defined and this check always fails. In include/asm-generic/io.h,
>> asm-generic/iomap.h gets included which declares them as extern
>> functions.
>
> I thinks that should be possible - since ioread64/iowrite64 depend on
> CONFIG_64BIT themselves.
>
>> One more thing to consider io-64-nonatomic-hi-lo.h and
>> io-64-nonatomic-lo-hi.h, if included would define it as a macro that
>> calls a function that rw 32 bits back to back.
>
> Even today, vfio_pci_core_do_io_rw() makes no guarantees to its users
> that register accesses will be done in the granularity they've thought
> to use. The vfs layer may coalesce the accesses and this code will then
> read/write the largest naturally aligned chunks. I've witnessed in my
> testing that one device driver was doing 8-byte writes through the 8-
> byte capable vfio layer all of a sudden when run in a KVM guest.
>
> So higher-level code needs to consider how to split register accesses
> appropriately to get the intended side-effects. Thus, I'd rather stay
> away from splitting 64bit accesses into two 32bit accesses - and decide
> if high or low order values should be written first.

Sorry, I was not clear. The main reason to include
io-64-nonatomic-hi-lo.h or io-64-nonatomic-lo-hi.h is to get the
iowrite64 and ioread64 macros defined mapping to functions that
implement them. They define and map them to generic functions in
lib/iomap.c The 64 bit rw functions (readq/writeq) get called if
present. If any architecture has not implemented them, then it maps them
to functions that do 32 bit back to back.

>
>
>>> + if (fillable >= 8 && !(off % 8)) {
>>> + ret = vfio_pci_core_iordwr64(vdev,
>>> iswrite, test_mem,
>>> +      io, buf, off,
>>> &filled);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + } else
>>> +#endif /* defined(ioread64) && defined(iowrite64) */
>>>    if (fillable >= 4 && !(off % 4)) {
>>>    ret = vfio_pci_core_iordwr32(vdev,
>>> iswrite, test_mem,
>>>         io, buf, off,
>>> &filled);
>>> diff --git a/include/linux/vfio_pci_core.h
>>> b/include/linux/vfio_pci_core.h
>>> index a2c8b8bba711..f4cf5fd2350c 100644
>>> --- a/include/linux/vfio_pci_core.h
>>> +++ b/include/linux/vfio_pci_core.h
>>> @@ -157,5 +157,8 @@ int vfio_pci_core_ioread##size(struct
>>> vfio_pci_core_device *vdev, \
>>>   VFIO_IOREAD_DECLATION(8)
>>>   VFIO_IOREAD_DECLATION(16)
>>>   VFIO_IOREAD_DECLATION(32)
>>> +#ifdef ioread64
>>> +VFIO_IOREAD_DECLATION(64)
>> nit: This macro is referenced only in this file. Can the typo be
>> corrected (_DECLARATION)?
>
> Sure thanks for pointing this out!
> I'll single this editorial change out into a separate patch of the
> series, though.
>
>>
>>> +#endif
>>>
>>>   #endif /* VFIO_PCI_CORE_H */
>>
>>
>
> Thanks, Gerd