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

Ramesh Thomas
2024-05-23T21:47:26Z

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
>
>