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

[Gerd Bayer <gbayer@linux.ibm.com>]

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@deltatee.com/

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