Re: [PATCH 12/13] vfio/nvidia-vgpu: add the NVIDIA vGPU VFIO variant driver

[Danilo Krummrich]

On Mon Sep 14, 2026 at 8:12 PM CEST, Alex Williamson wrote:
> Hi Danilo,
>
> On Fri, 11 Sep 2026 22:39:16 +0200
> "Danilo Krummrich" <dakr@xxxxxxxxxx> wrote:
>
>> Hi Alex, Jason, Zhi,
>> 
>> On Sat Sep 5, 2026 at 10:11 AM CEST, Zhi Wang wrote:
>> > NVIDIA vGPU VFs require their open, reset, and close lifecycle to be
>> > coordinated with the PF-side nova-core driver.  
>> 
>> [...]
>> 
>> >  drivers/vfio/pci/nvidia-vgpu/main.c   | 253 ++++++++++++++++++++++++++  
>> 
>> This is going to be a longer response; sorry about this in advance.
>> 
>> Looking at the FFI boundary introduced in the previous patch, I'm concerned that
>> it translates the driver model relationships we've expressed through Rust's
>> ownership and lifetime model back into raw pointers and lifetime assumptions
>> that callers must uphold. It also introduces manual lifecycle management across
>> the boundary, rather than preserving nova-core's RAII-based ownership model.
>> 
>> I think implementing the NVIDIA vGPU driver in Rust would let us preserve those
>> relationships across the interface, make lifecycle management less error-prone,
>> and fit naturally alongside nova-core and nova-drm.
> [snip]
>> 
>> If you've made it this far, thanks for reading through this long write-up. I
>> hope you find it useful. Please let me know if you have any questions or
>> thoughts.
>
> I can't really say I made it this far with comprehension, but thanks
> for the effort ;)
>
> The one piece here that I can actually review is [5], where
> dev_get_drvdata() is replaced with a vfio-pci-core struct pointer
> embedded in the struct pci_dev, which is a non-starter as far as having
> a common PCI-core shared by various drivers.

Well, that was just a quick hack to get it out of the way. :)

I think there are a couple of options.

  (1) Make the PM helpers take a struct vfio_pci_core_device * in the first
      place and let the driver forward to the helpers in its own PM callbacks.

  (2) Provide an (optional?) driver callback that translates a struct pci_dev to
      struct vfio_pci_core_device.

  (3) Provide a macro for drivers to define PM ops, letting drivers provide the
      function that translates struct pci_dev to struct vfio_pci_core_device.

  (4) Give struct vfio_pci_core_device its own PM domain (which is probably a
      bit overkill :).

I understand that the idea is to hide the PM handling in the vfio-pci framwork,
but I think the existing implementation is a bit of a layering violation, since
class device implementations shouldn't impose requirements on the bus device
private data layout.

I also think that the approach to fully hide it in the framework is only really
worth if it doesn't otherwise impose subtle requirements on the driver (such as
the layout requirement of the bus device private data).

Thus, I'd personally just go with (1) as it is the most honest approach in terms
of driver layering. But I think (2) is a good alternative that is not more
invasive than asking drivers to set the bus device private data to
struct vfio_pci_core_device *.

> My concerns are of course who is going to review the Rust vfio-pci
> variant drivers from a vfio perspective, not just a drm driver
> viewpoint.

I did put some considerations about this; I think Zhi would be a great candidate
for this. :) And as mentioned I can also take some of the responsibility, but I
also want to be honest about the fact that I have a lot on my plate already.

> Who is going to be responsive when the interfaces break and
> monitor vfio proactively to prevent such breakages, and how do we avoid
> derailing feature development in the core code base.

In practice we shouldn't see any breakages with the Rust code that wouldn't also
break the C VFIO drivers. If this would be the case it would mean that the Rust
code relies on guarantees that none of the C drivers rely on, which would likely
mean that is was never a guarantee that was actually promised by the VFIO core
code.

There may be cases where Rust code breaks, and C code won't, but those should be
mechanical things, such as a type mismatch where Rust is more careful, e.g. if
we'd change a CPP define to an enum, etc.

> Can a Rust vfio-pci variant driver be self-contained, or to what extent does
> it impose on the framework, such as the drvdata idiom.

As mentioned above, I think this one is more of a layering violation in the
vfio-pci-core; class devices shouldn't impose layout requirements on bus device
private data.

The reason C drivers can get away with it more easily is e.g. that C relies on
procedural cleanup and that all responsibility for managing lifetimes sits on
the drivers themselves, so it is easier for them to adjust. But in general, it
wouldn't work out if all class device registrations or other core primitives
would have the same expectation.

For Rust specifically it is that the driver core controls the lifetime of the
bus device private data, which is a fundamental requirement to e.g. represent
registrations as RAII types. E.g. the vfio::pci::Registration has to be stored
in the bus device private data, such that it is guaranteed to be correctly
destroyed on driver unbind.

To get back to your question, a Rust vfio-pci variant driver should be
self-contained. The interface sits in the abstraction that translates the C
driver API to a Rust driver API. It sometimes can help quite significantly (e.g.
in terms of how complex the Rust code needs to get in order to actually be safe)
if the C code does a minor adjustment, but it shouldn't be necessary.

For instance, the only addition to the driver core we have is an additional
callback in struct device_driver, and in the future an additional pointer in
struct device_private; none of those couldn't be worked around in some way
though.

On the other hand there are examples where the Rust introduction motivated
design improvements on the C side, or bug fixes for issues that were caught
while writing a safe abstraction. For instance, we recently had some fixes
around dyn IDs in the PCI and USB core, which both were motivated by Rust code.

> FWIW, AI can only go so far to support reviews.  Having the code
> insight to ask the right questions is essential.  A human in the loop
> is a requirement.
>
> Additionally, if we can't narrow the device matching to only the
> Nova-core supported VFs,

Agreed, and I think that should be possible. Is there a particular case you
think of where this wouldn't hold?