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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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







[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux