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

["Danilo Krummrich" <dakr@kernel.org>]

On Tue Sep 15, 2026 at 8:01 PM CEST, Alex Williamson wrote:
> On Mon, 14 Sep 2026 23:36:24 +0200
> "Danilo Krummrich" <dakr@kernel.org> wrote:
>
>> 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@kernel.org> 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 *.
>
> I'd position this more as a library convention than a class layering
> violation.  vfio-pci was originally one driver, vfio-pci-core was
> pulled out to enable device specific support, ex. migration, in a more
> manageable way.  struct vfio_pci_core_device is not strictly a class,
> it's the object used by the library that variant drivers opt to use
> rather than re-implementing vfio-pci from the ground up.
>
> The conventions of that library mean variant drivers get things like
> VGA routing and power management for free, in adherence with how these
> features are exported by the core, and can choose to opt-in to common
> error handling.
>
> The use of drvdata is part of that convention and audited by the core
> such that failed compliance is rejected on registration.  Clearly we
> could allow variant drivers to provide ops for their own callbacks and
> export core helpers they can use, but only a Rust driver requires this
> and we need to figure out how to do this without degrading the audit in
> the core.

The driver_data pointer in struct device is defined to be a pointer where a
driver can store *arbitrary* data for the duration the driver is bound to this
device.

This is an API contract the driver core provides for the struct device structure
when used in a bus device (such as struct pci_dev), i.e. it is an API contract
between the driver core and drivers in general.

It is *not* the Rust driver core code breaking a convention here. The Rust
driver core code just relies on its own subsystem's API contract.

The fact that the vfio-pci-core forces vfio-pci drivers to only ever store a
struct vfio_pci_core_device pointer there re-defines the driver core's API
contract and therefore is a layering violation.

> Turning vfio-pci-core into a proper class to be able to have a real
> layering violation claim seems like a much larger project.

What you are describing rather sounds to me as if struct vfio_pci_core_device
wants to be two things: a bus device layered on top of a PCI device and a class
device around struct vfio_device. But in this case it should implement a real
bus on top of the PCI bus.

Currently it really is a wrapper around struct vfio_device (and therefore a
class device) which is bending the purpose of the driver's bus device private
data pointer to make a connection with the real bus device underneath.

>> > 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.
>
> And that's part of the problem, there are bandwidth issues all around and
> neither you nor Zhi are, as of yet, core vfio contributors.

That is true, but I suggest to also see it from the perspective that this effort
is a good chance to get more people to engage with the VFIO subsystem in the
first place and eventually become experts that also contribute to the C core.

For the Rust abstraction, there are two competencies needed: knowledge about
writing a VFIO driver (which Zhi clearly covers) and knowledge about the driver
model and its Rust implementation specifics as well as the Rust ecosystem in
general (which is why I offered myself to take responsibility).

Remember the VFIO abstractions are translating the C VFIO driver APIs to Rust
APIs for Rust drivers. They don't need to mess with VFIO internals.

>> > 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.
>
> s/practice/theory/

As mentioned above, the abstractions are translating the C VFIO driver APIs to
Rust APIs for Rust drivers. I.e. they don't use other API than the C drivers
use. Thus, code breakages should be limited to what I mentioned below.

>> 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.
>
> This seems like a very optimistic outlook.  I don't have experience
> with Rust to claim otherwise, but I do have general software experience
> to be suspicious of anything sounding so clean.

I maintain some subsystems that have Rust code, and I also maintain Rust code of
subsystems I otherwise do not maintain. I haven't seen anything break so far
other than for the reasons already mentioned.

Of course, that doesn't mean that mistakes can't happen. For instance, we could
have the case that the Rust code makes assumptions that the subsystem never
guaranteed in the first place. But that'd be something that could also happen
for any other driver and it would be a bug in the Rust code that the maintainers
of the Rust code have to take responsibility for.

>> > 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.
>
> I see, so driver core has its own convention for how Rust drivers must
> use drvdata.

As mentioned above, the driver_data pointer of struct device is specifically
reserved for drivers to store arbitrary data while they are bound to the device.

The only difference is that in C it is a convention and drivers call
dev_set_drvdata() themselves, whereas in Rust we do it in the driver core code.

Note that clearing this pointer is also done in the driver core in C in
device_unbind_cleanup().

>> 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.
>
> It's not clear to me to what extent these abstractions hinder our
> ability to evolve and refactor the C code.  It may not "lock in" the
> core API for variant drivers, but it seems it raises the bar that any
> significant code refactor likely needs to refactor the abstraction
> layer, potentially the Rust variant driver itself, which imposes a
> burden on the vfio community that has so far not introduced Rust into
> the code base.

In general, if the refactor does not affect C drivers, it shouldn't affect the
Rust API either.

If the refactor affects C drivers, it will also affect Rust drivers and the
change in the Rust abstraction could also be more complicated than the change
required in the C drivers.

The reason is that we design the Rust APIs at minmum in a way that any arbitrary
usage can not produce undefined behavior. Beyond that, we also always try to
design them in a way that it also at least encourages correct semantical use.

So in theory, it could happen that we need to figure out a new way to abstract
the C API in way that can't produce undefined behavior.

But if that happens it should be something minor, e.g. a new callback which
requires a new type state, an argument that requires a new type carrying some
invariants, etc.

The biggest part of the abstraction is the integration in the driver lifecycle,
which is a repeating pattern. For instance, if you look at the fwctl Rust code
it looks almsot the same.

From my experience maintaining both C and Rust code I can also tell that good
design is pretty straight forward to abstract in Rust to produce a safe API; bad
design isn't.

Where good design means APIs with defined ownership, clear lifetime
relationships, and a consistent API surface. Whereas APIs that have unclear
ownership and lifetime relationships and inconsistent API surfaces are a huge
pain in the butt to abstract.

The reason is that Rust needs to get those things straight somehow in order to
provide a safe API surface. I.e. an underlying bad API design (as defined above)
needs the Rust implementation to make quite some stretches.

>> 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.
>
> I have no doubt that integration with a more structured language would
> lead to various improvements.  However, it doesn't seem there are
> resources to support it in the short term.

As mentoined above, there are people volunteering now. Without starting it, it
can't scale further than that. :)

>> > 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?
>
> The modalias scheme only matches on vendor/device ID, subsystem IDs,
> base/sub class, and interface.  Nothing in the PCI spec requires that
> the VF device ID is different from the PF device ID.  Variant drivers
> will often present a larger match surface to avoid the ongoing
> maintenance overhead of listing explicit device IDs.  Such an approach
> here would put us in the position I noted in the previous reply where
> the Rust variant driver needs to fully support these devices via
> vfio-pci-core on day one.  Binding GPU PFs to vfio-pci is a current,
> valid use case (VFs for some vendors as well).  Thanks,

My understanding is that in order to support PFs being bound to the variant
driver the only thing necessary would be to fall back to the vfio-pci-core
callbacks. If that's the case, my PoC code can already do that.

However, I don't really see the use-case; you can't load nvidia-vgpu without
nova-core in the first place, so it would require to unbind nova-core through
sysfs force unbind, no?