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

["Gary Guo" <gary@garyguo.net>]

On Tue Sep 15, 2026 at 7:01 PM BST, 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:
>> > 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.
>
> Turning vfio-pci-core into a proper class to be able to have a real
> layering violation claim seems like a much larger project.
>
>> > 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.  

FWIW, if we want vfio-pci to be a middle layer (looks like there're some
disagreements about this), we could quite simply achieve this by define

    mod vfio_pci {

        trait Driver {
            /* callbacks here */
        }

        struct Adapter<D: Driver>(D);

        impl pci::Driver for Adapter {
            ...
        }

    }

and then the way for people to be using this would to create the "Adapter" which
does the middle layering and register *that* as pci driver instead. Then
all PCI callbacks will first land in vfio-pci abstration's code before it
filters through things in the driver. This way, vfio-pci-core imlements the pci
driver then it controls its drvdata layout. 

Best,
Gary