Re: [PATCH 0/1] pcie: Allow atomic completion on PCIE root port

[Alex Williamson <alex.williamson@redhat.com>]

On Thu, 18 May 2023 16:03:07 -0400
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Fri, Apr 21, 2023 at 06:06:49PM +0200, Robin Voetter wrote:
> > 
> > 
> > On 4/21/23 10:22, Michael S. Tsirkin wrote:  
> > > On Thu, Apr 20, 2023 at 05:38:39PM +0200, robin@streamhpc.com wrote:  
> > >> From: Robin Voetter <robin@streamhpc.com>
> > >>
> > >> The ROCm driver for Linux uses PCIe atomics to schedule work and
> > >> generally communicate between the host and the device.  This does not
> > >> currently work in QEMU with regular vfio-pci passthrough, because the
> > >> pcie-root-port does not advertise the PCIe atomic completer
> > >> capabilities.  When initializing the GPU from the Linux driver, it
> > >> queries whether the PCIe connection from the CPU to GPU supports the
> > >> required capabilities[1] in the pci_enable_atomic_ops_to_root
> > >> function[2].  Currently the only part where this fails is checking the
> > >> atomic completer capabilities (32 and 64 bits) on the root port[3].  In
> > >> this case, the driver determines that PCIe atomics are not supported at
> > >> all, and this causes ROCm programs to misbehave.  (While AMD advertises
> > >> that there is some support for ROCm without PCIe atomics, I have never
> > >> actually gotten that working...)
> > >>
> > >> This patch allows ROCm to properly function by introducing an
> > >> additional experimental property to the pcie-root-port,
> > >> x-atomic-completion.  
> > > 
> > > so what exactly makes it experimental? from this description
> > > it looks like it actually has to be enabled for things to work?  
> > 
> > I was not sure which would be appropriate, but I'm fine with making it a
> > non-experimental option.  
> 
> So I guess the real thing to do is to query this from vfio right?
> Unfortunately we don't have access to vfio when we
> are creating the root port, but I think the thing to do would
> be to check at the time when vfio is attached, and if
> atomic is set but not supported, fail attaching vfio.
> 
> Right?

We don't currently provide a way to query this in vfio, but I imagine
we could call pci_enable_atomic_ops_to_root() in the host kernel
ourselves with various sizes and expose which are supported via a
capability on the vfio-device.  I'm not sure what we do for VFs though
since that function is invalid for them (maybe worry about them later).
I also see that one of the in-kernel drivers (mlx5) tries to enable
128-bit support, so I wonder if we want separate options for 32/64-bit
and 128-bit.

QEMU device options are clearly the most straightforward path to enable
this, but would it actually make sense, perhaps in addition, to
implement the above in the kernel and then have the QEMU vfio-pci
driver enable the available completer bits in the root port during
realize?  We could probably get away with it on hotplug, but if
necessary it could be something we only do for cold-plug devices (we
also have a no-hotplug vfio-pci variant if we're concerned what happens
after the device is removed in the VM - again, be could probably get
away with clearing the bits on unplug).

I'm not entirely sure where we stand in QEMU on whether options that
can cause poor behavior should always be experimental or we allow users
to shoot themselves in the foot as they please.  Obviously it makes it
more difficult for libvirt to support such configurations, but maybe
they'd rely on the above automatic enabling rather than try to guess
themselves.  Thanks,

Alex