On 5/24/2024 6:42 AM, Gerd Bayer wrote:
On Thu, 2024-05-23 at 14:47 -0700, Ramesh Thomas wrote:
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.
Now I got it - I think. And I see that plain x86 is aleady affected by
this issue.
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.
Yes definitely, we need to avoid this.
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.
I dug into lib/iomap.c some more today and I see your point, that it is
desireable to make the 64bit accessors useable through vfio/pci when
they're implemented in lib/iomap.c. And I follow your argument that in
most cases these will map onto readq/writeq - only programmed IO (PIO)
has to emulate this with 2 32bit back-to-back accesses.
If only the code in lib/iomap.c was structured differently - and made
readq/writeq available under ioread64/iowrite64 proper and only fell
back to the nonatomic hi-lo or lo-hi emulation with 32bit accesses if
PIO is used.
It determines if it is PIO or MMIO based on the address. If the address
is >= PIO_RESERVED then it calls readq/writeq. PIO_RESERVED is arch
dependent.
Looks like if asm/io.h didn't define ioread64/iowrite64 already, then
the intention is to use the generic implementation, especially when it
defines CONFIG_GENERIC_IOMAP. lib/iomap.c and
io-64-nonatomic-{lo-hi|-hi-lo}.h include linux/io.h which includes asm/io.h.
As much as I'd like to have it differently, it seems like it was a
lengthy process to have that change accepted at the time:
https://lore.kernel.org/all/20181106205234.25792-1-logang@xxxxxxxxxxxx/
I'm not sure if we can clean that up, easily. Plus there are appear to
be plenty of users of io-64-nonatomic-{lo-hi|-hi-lo}.h in tree already
- 103 and 18, resp.
revert to make the conditional compiles depend on those
definitions. But maybe Alex has an opinion on this, too?
Thanks,
Gerd
So I'd like to hear from Alex and Tian (who was not a big fan) if we
should support 64bit accessors in vfio/pci (primarily) on x86 with this
series, or not at all, or split that work off, maybe?
Thanks,
Gerd
