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

Ramesh Thomas
2024-05-23T00:12:00Z

On 5/20/2024 2:02 AM, Tian, Kevin wrote:
>> From: Ramesh Thomas <ramesh.thomas@intel.com>
>> Sent: Friday, May 17, 2024 6:30 PM
>>
>> On 4/25/2024 9:56 AM, Gerd Bayer wrote:
>>> From: Ben Segal <bpsegal@us.ibm.com>
>>>
>>> @@ -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.
>>
>> 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.
>
> I don't see the problem here. when the defined check fails it falls
> back to back-to-back vfio_pci_core_iordwr32(). there is no need to
> do it in an indirect way via including io-64-nonatomic-hi-lo.h.

The issue is iowrite64 and iowrite64 was not getting defined when
CONFIG_GENERIC_IOMAP was not defined, even though the architecture
implemented the 64 bit rw functions readq and writeq.
io-64-nonatomic-hi-lo.h and io-64-nonatomic-lo-hi.h define them and map
them to generic implementations in lib/iomap.c. The implementation calls
the 64 bit rw functions if present, otherwise does 32 bit back to back
rw. Besides it also has sanity checks for port numbers in the iomap
path. I think it is better to rely on this existing generic method than
implementing the checks at places where iowrite64 and ioread64 get
called, at least in the IOMAP path.