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

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

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.

However, I'm working on an idea - extending on the VFIO_IORDWR macro -
to convert the if - else if - chain into a switch/case construct, where
I can more easily #ifdef out the 64bit case if not available.

Now I "just" need to test this ;)

Thanks,
Gerd