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

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

On Mon, 2024-04-22 at 14:43 -0300, Jason Gunthorpe 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?

Hi Jason,

unfortunately, the nomenclature in vfio_pci_rdwr.c is not very clear...
vfio_io{read|write}64 are mapped to io{read|write}64 as defined in
include/asm-generic/io.h prior to my change already. OTOH, looks like
vfio_io{read|write}64 are consumed only by the
vfio_pci_core_io{read|write}64 functions. This however is an exported
symbol - that seems to be used only as vfio_pci_core_io{read|write}16,
so far.

vfio_pci_core_io{read|write}X is also used by vfio_pci_core_do_io_rw()
which does "bulk" reads/writes using the largest suitable access size.
I think there, we can live without 64bit accesses as the while loop
there will use 32bit read/writes back-to-back as applicable.

So I think 64bit accesses on 32bit architectures through VFIO are
somewhat uncharted territory - and I'm not sure that back-to-back 32bit
accesses are the right thing to do. If the device defined 64bit
registers, you could trigger side-effects in the wrong order (or not at
all).

Somewhat overwhelmed,
Gerd