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

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

On 5/23/2024 8:01 AM, Gerd Bayer wrote:
> Hi Ramesh,
> 
> On Wed, 2024-05-22 at 16:38 -0700, Ramesh Thomas wrote:
>> The removal of the check for iowrite64 and ioread64 causes build
>> error because those macros don't get defined anywhere if
>> CONFIG_GENERIC_IOMAP is not defined. However, I do think the removal
>> of the checks is correct.
> 
> Wait, I believe it is the other way around. If your config *is*
> specifying CONFIG_GENERIC_IOMAP, lib/iomap.c will provide
> implementations for back-to-back 32bit operations to emulate 64bit
> accesses - and you have to "select" which of the two types of emulation
> (hi/lo or lo/hi order) get mapped onto ioread64(be) or iowrite64(be) by
> including linux/io-64-nonatomic-lo-hi.h (or -hi-lo.h).

Sorry, yes I meant to write they don't get defined anywhere in your code 
path if CONFIG_GENERIC_IOMAP *is defined*. The only place in your code 
path where iowrit64 and ioread64 get defined is in asm/io.h. Those 
definitions are surrounded by #ifndef CONFIG_GENERIC_IOMAP. 
CONFIG_GENERIC_IOMAP gets defined for x86.

> 
>> It is better to include linux/io-64-nonatomic-lo-hi.h which define
>> those macros mapping to generic implementations in lib/iomap.c. If
>> the architecture does not implement 64 bit rw functions
>> (readq/writeq), then  it does 32 bit back to back. I have sent a
>> patch with the change that includes the above header file. Please
>> review and include in this patch series if ok.
> 
> I did find your patch, thank you. I had a very hard time to find a
> kernel config that actually showed the unresolved symbols situation:
> Some 64bit MIPS config, that relied on GENERIC_IOMAP. And with your
> patch applied, I could compile successfully.
> Do you have an easier way steer a kernel config into this dead-end?

The generic implementation takes care of all conditions. I guess some 
build bot would report error on build failures. But checks like #ifdef 
iowrite64 would hide the missing definitions error.

> 
>> Thanks,
>> Ramesh
> 
> Frankly, I'd rather not make any assumptions in this rather generic
> vfio/pci layer about whether hi-lo or lo-hi is the right order to > emulate a 64bit access when the base architecture does not support
> 64bit accesses naturally. So, if CONFIG_64BIT is no guarantee that
> there's a definitive implementation of ioread64/iowrite64, I'd rather

There is already an assumption of the order in the current 
implementation regardless e.g. vfio_pci_core_do_io_rw(). If there is no 
iowrite64 found, the code does back to back 34 bit writes without 
checking for any particular order requirements.

io-64-nonatomic-lo-hi.h and io-64-nonatomic-hi-lo.h would define 
ioread64/iowrite64 only if they are not already defined in asm/io.h.

Also since there is a check for CONFIG_64BIT, most likely a 64 bit 
readq/writeq will get used in the lib/iomap.c implementations. I think 
we can pick either lo-hi or hi-lo for the unlikely 32 bit fall through 
when CONFIG_64BIT is defined.


> revert to make the conditional compiles depend on those definitions.
> 
> But maybe Alex has an opinion on this, too?
> 
> Thanks,
> Gerd
> 
>