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

[Ramesh Thomas <ramesh.thomas@intel.com>]

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