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

[Jason Gunthorpe <jgg@ziepe.ca>]

On Tue, Apr 23, 2024 at 06:11:57PM +0200, Gerd Bayer wrote:
> On Mon, 2024-04-22 at 16:33 -0600, Alex Williamson wrote:
> > On Mon, 22 Apr 2024 14:43:05 -0300
> > Jason Gunthorpe <jgg@ziepe.ca> wrote:
> > 
> > > On Mon, Apr 22, 2024 at 05:35:08PM +0200, Gerd Bayer wrote:
> > > > From: Ben Segal <bpsegal@us.ibm.com>
> > > > 
> > > > Many PCI adapters can benefit or even require full 64bit read
> > > > and write access to their registers. In order to enable work on
> > > > user-space drivers for these devices add two new variations
> > > > vfio_pci_core_io{read|write}64 of the existing access methods
> > > > when the architecture supports 64-bit ioreads and iowrites.
> > > > 
> > > > Since these access methods are instantiated on 64bit
> > > > architectures,
> > > > only, their use in vfio_pci_core_do_io_rw() is restricted by
> > > > conditional
> > > > compiles to these architectures.
> > > > 
> > > > Signed-off-by: Ben Segal <bpsegal@us.ibm.com>
> > > > Co-developed-by: Gerd Bayer <gbayer@linux.ibm.com>
> > > > Signed-off-by: Gerd Bayer <gbayer@linux.ibm.com>
> > > > ---
> > > > Hi all,
> > > > 
> > > > we've successfully used this patch with a user-mode driver for a
> > > > PCI
> > > > device that requires 64bit register read/writes on s390. A quick
> > > > grep
> > > > showed that there are several other drivers for PCI devices in
> > > > the kernel
> > > > that use readq/writeq and eventually could use this, too.
> > > > So we decided to propose this for general inclusion.
> > > > 
> > > > Thank you,
> > > > Gerd Bayer
> > > > 
> > > > Changes v1 -> v2:
> > > > - On non 64bit architecture use at most 32bit accesses in
> > > >   vfio_pci_core_do_io_rw and describe that in the commit message.
> > > > - Drop the run-time error on 32bit architectures.
> > > > - The #endif splitting the "else if" is not really fortunate, but
> > > > I'm
> > > >   open to suggestions.  
> > > 
> > > Provide a iowrite64() that does back to back writes for 32 bit?
> > 
> > I was curious what this looked like.  If we want to repeat the 4 byte
> > access then I think we need to refactor all the read/write accesses
> > into macros to avoid duplicating code, which results in something
> > like [1] below.  But also once we refactor to macros, the #ifdef
> > within the function as originally proposed gets a lot more bearable
> > too [2].
> > 
> > I'd probably just go with something like [2] unless you want to
> > further macro-ize these branches out of existence in the main
> > function. Feel free to grab any of this you like, the VFIO_IORDWR
> > macro should probably be it's own patch.  Thanks,
> > 
> > Alex
> 
> Hi Alex,
> 
> thanks for your suggestions, I like your VFIO_IORDWR macro in [1].
> As I just explained to Jason, I don't think that the back-to-back 32bit
> accesses are a safe emulation of 64bit accesses in general, though. So
> I'd rather leave that out.

It is though, it is exactly what the code does now.

This function is not 'do exactly XX byte store' it is actually 'memcpy
to io' with some occasional support for making contiguous TLPs in
common cases..

Jason