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