Re: [PATCH v7 1/1] vfio/nvgpu: Add vfio pci variant module for grace hopper

Re: [PATCH v7 1/1] vfio/nvgpu: Add vfio pci variant module for grace hopper

[Jason Gunthorpe]

On Wed, Aug 30, 2023 at 06:50:32AM -0700, Christoph Hellwig wrote:
> I know I'm chiming in a bit late, but what ultimate user space is going
> to use this?  We should not add anything to the kernel that can't
> be used without fully open user space.

qemu will get the matching VFIO userspace patches, I think they were
posted someplace already.

> vfio has traditionally been a bit special as it "just" passes devices
> through, so any user space could just be a user space driver for a
> random device on $FOO bus, including an actual Linux driver in a VM,
> but this driver has very specific semantics for a very specific piece
> of hardware, so it really needs to be treated like a generic GPU driver
> or accelerator driver.

This is basically a pre-CXL driver. It takes a PCI device and some
non-standard CXL-ish metadata and adapts it to VFIO.

In a post-CXL world this same functionality of managing the 'cache
coherent BAR' for VFIO would be done generically by some generic
vfio-cxl driver.

Jason

Re: [PATCH v7 1/1] vfio/nvgpu: Add vfio pci variant module for grace hopper

[Ankit Agrawal]

Hi Christoph,

>Whats the actual consumer running in a qemu VM here?
The primary use case in the VM is to run the open source Nvidia
driver (https://github.com/NVIDIA/open-gpu-kernel-modules)
and workloads.

Re: [PATCH v7 1/1] vfio/nvgpu: Add vfio pci variant module for grace hopper

[Alex Williamson]

On Thu, 31 Aug 2023 07:04:10 -0700
Christoph Hellwig <hch@infradead.org> wrote:

> On Thu, Aug 31, 2023 at 01:51:11PM +0000, Ankit Agrawal wrote:
> > Hi Christoph,
> >   
> > >Whats the actual consumer running in a qemu VM here?  
> > The primary use case in the VM is to run the open source Nvidia
> > driver (https://github.com/NVIDIA/open-gpu-kernel-modules)
> > and workloads.  
> 
> So this infrastructure to run things in a VM that we don't even support
> in mainline?  I think we need nouveau support for this hardware in the
> drm driver first, before adding magic vfio support.

There's really never a guarantee that the thing we're exposing via the
vfio uAPI has mainline drivers, for example we don't consult the
nouveau device table before we expose an NVIDIA GPU to a Windows guest
running proprietary device drivers.

We've also never previously made a requirement that any new code in
vfio must directly contribute to supporting a mainline driver, in fact
I think you'll find examples where we do have such code.

This driver is proposing to expose a coherent memory region associated
with the device, composed as a PCI BAR, largely to bring it into the
vfio device model.  Access to that memory region is still pass-through.
This is essentially behavior that we also enable though mdev drivers
like kvmgt (modulo the coherent aspect).

I assume the above driver understands how to access and make use of
this coherent memory whether running bare-metal or virtualized, so
potentially we have some understanding of how it's used by the driver,
which can't be said for all devices used with vfio.  I'm therefore not
sure how we can suddenly decide to impose a mainline driver requirement
for exposing a device to userspace.  Thanks,

Alex

Re: [PATCH v7 1/1] vfio/nvgpu: Add vfio pci variant module for grace hopper

[Jason Gunthorpe]

On Thu, Aug 31, 2023 at 12:21:50PM -0600, Alex Williamson wrote:

> I assume the above driver understands how to access and make use of
> this coherent memory whether running bare-metal or virtualized, so
> potentially we have some understanding of how it's used by the driver,
> which can't be said for all devices used with vfio.  I'm therefore not
> sure how we can suddenly decide to impose a mainline driver requirement
> for exposing a device to userspace.  Thanks,

Yeah, I was comfortable with removing the old powernv VFIO stuff based
on the combined logic that the platform was dead, powernv has weird
arch entanglements and there was no open source driver anyhow so
maintaining the mess past the vendor lifetime was looking bad. This
has none of those issues.

I think the threshold here should be the maintainability of the kernel
and its associated open ecosystem. An open source qemu, and a open
source VM kernel driver is a pretty good situation to sustain this
driver. In particular if Alex&co think the qemu side should not
advance then this should not be merged.

For comparison, I'm much more unhappy about VFIO_UPDATE_VADDR from a
maintainability perspective than I am about this. There was a half
hearted effort to get the userspace side in qemu and now we are now
stuck with an ugly kernel side mess with no open source userspace so
we can't even test. :\ Considering the vigorous objections when I
tried to remove it I assume a cloud operator is running it with a
proprietary userspace.

Certainly I would strongly support removing kernel side parts if the
qemu side doesn't get merged within a year or something like that.

Jason