* [PATCH 00/13] Introduce NVIDIA vGPU manager and VFIO variant driver
@ 2026-09-05 8:11 Zhi Wang
2026-09-05 8:11 ` [PATCH 12/13] vfio/nvidia-vgpu: add the NVIDIA vGPU " Zhi Wang
0 siblings, 1 reply; 24+ messages in thread
From: Zhi Wang @ 2026-09-05 8:11 UTC (permalink / raw)
To: dakr, acourbot
Cc: alex, jgg, yishaih, skolothumtho, kevin.tian, airlied, simona,
ojeda, alex.gaynor, boqun.feng, gary, bjorn3_gh, lossin,
a.hindborg, aliceryhl, tmgross, jhubbard, ecourtney, cjia, smitra,
kjaju, alkumar, ankita, aniketa, kwankhede, targupta, nova-gpu,
linux-kernel, zhiwang, Zhi Wang, kvm
Add the nova-core support needed to create and manage NVIDIA vGPU
instances, together with the NVIDIA vGPU VFIO variant driver that consumes
the lifecycle interface.
The control path introduced by this series is:
+-----------------------+ +-----------------------+
| Linux guest | | Windows guest |
| NVIDIA guest driver | | NVIDIA guest driver |
+-----------+-----------+ +-----------+-----------+
| |
+-----------v-----------+ +-----------v-----------+
| QEMU (VFIO user) | | QEMU (VFIO user) |
+-----------+-----------+ +-----------+-----------+
+----------------+----------------+
|
+-------v-------+
| VFIO core |
+-------+-------+
|
+---------------------v---------------------+
| NVIDIA vGPU VFIO variant driver |
+---------------------+---------------------+
|
+---------------+----------------------+
| binds PCI VF | PF lifecycle API
| | open / close / reset
v v
+--------+--------+ +-------------+-------------+
| PCI VF(s) | | nova-core (PF driver) |
| (virtual GPU) | | vGPU Manager |
+--------+--------+ | VRAM / channels ... |
| +-------------+-------------+
| |
+-------------v--------------------------------------v----------------+
| NVIDIA physical GPU |
| VF resources PF / GSP / VRAM |
+---------------------------------------------------------------------+
On the nova-core side, the PF driver owns the GPU firmware-facing part
of vGPU management. It already detects vGPU mode before GSP boot and
supplies the SR-IOV topology to the GPU System Processor firmware
(GSP-RM). This series extends it to:
- Retain the firmware-reported VMMU segment size and FIFO engine
ordering after GSP_INIT, together with nova-core's channel capacity.
- Add VramBlock and VramRegion ownership plus Bar1Map for bounded CPU
mappings of VRAM-backed control structures.
- Query GSP-RM through its command queue for the vGPU type already
assigned to a VF and its properties.
- Build a layout-validated, profile-wide pool of paired framebuffer and
management-heap slots, reserve contiguous channel ranges, and keep
live allocations in a per-PF instance registry.
- Use the typed r000 firmware bindings for control and response layouts,
message IDs, and firmware-defined region sizes.
- Handle GMC transactions over the GSP queues, match GMC replies by
command and sequence, dispatch interleaved RM RPCs from the shared
GSP-to-CPU message queue, and consume stale GMC messages.
- Implement the GMCAPI bootload, shutdown, and cleanup operations with
typed channel and resource maps for each plugin instance.
- Establish the BAR1-backed PluginRpc channel, negotiate its protocol,
and send the VM configuration and plugin BME-state update.
- Use per-instance CeUtils channels to scrub guest framebuffer memory
on allocation, reset, and shutdown. If ownership or completion is
uncertain, keep the affected VRAM and channels out of the allocators.
- Expose the three per-instance plugin log buffers through debugfs with
the header needed by nvlog_decoder.
- Keep resource locking, teardown, and rollback inside nova-core.
- Select the firmware-defined 48-VM WPR2 heap when a device advertises
more than 32 VFs.
On the VFIO side, the NVIDIA vGPU VFIO variant driver:
- Binds only to an NVIDIA VF explicitly selected through
driver_override.
- Derives the guest function ID (GFID) from the SR-IOV VF index and
registers a vfio-pci-core device.
- Presents the vGPU PCI device and subsystem IDs returned by nova-core,
and limits the reported framebuffer BAR size.
- Delegates the remaining PCI and VFIO operations to vfio-pci-core and
uses the standard VFIO physical-device helpers for IOMMUFD.
The ownership boundary between the two drivers is visible at these
lifecycle points:
- Open: after vfio_pci_core_enable(), the variant driver passes the PF
returned by pci_physfn(vf), the GFID, the VF's PCI
domain/bus/device/function (DBDF), and the calling process TGID to
nvidia_vgpu_open(). nova-core validates the PF and GFID, creates and
activates the instance, and returns the guest-visible PCI IDs and
BAR1 size. On failure, the variant driver disables the vfio-pci-core
device; on success, it calls
vfio_pci_core_finish_enable().
- Reset: the variant driver first calls nvidia_vgpu_reset(). nova-core
sends the plugin reset RPC and scrubs the guest framebuffer. Only
after that succeeds is VFIO_DEVICE_RESET passed to vfio-pci-core for
the PCI function reset.
- Close: the variant driver invokes the void nvidia_vgpu_close()
interface to request plugin shutdown, framebuffer scrubbing, and
resource release from nova-core, then closes the vfio-pci-core
device.
The cross-module interface is therefore limited to three GPL-only
symbols in NOVA_CORE_VGPU: nvidia_vgpu_open(), nvidia_vgpu_close(), and
nvidia_vgpu_reset().
This is a ground-up rework of the earlier vGPU RFC [1]. In particular,
the vGPU manager has been moved from the VFIO driver into the nova-core
implementation, and the VFIO side no longer carries copies of RM firmware
headers. The VFIO driver attaches to the VF and uses its PF's nova-core
state for management, as discussed in that thread.
The patches are organized as follows:
1-4 Initialize VgpuManager and add the memory and firmware building
blocks.
5-8 Allocate instances and implement GMC, plugin boot, and RPC.
9-10 Scrub guest framebuffer memory and expose plugin diagnostics.
11-12 Export the lifecycle API and add its VFIO consumer.
13 Select the larger WPR2 heap for 48-VF devices.
The series is based on the nova-core r000 GSP, memory-management,
SR-IOV, Rust bitmap/id-pool, and PCI abstraction work. The exact base
commit is recorded below, and the complete prerequisite stack is
available in [2].
[1] https://lore.kernel.org/kvm/20250903221111.3866249-1-zhiw@nvidia.com/
[2] https://github.com/zhiwang-nvidia/nova-core/tree/zhi/nova-vgpu-wip-nova-gsp-20260902
Alok Kumar (1):
gpu: nova-core: vgpu: add VRAM slot allocator
Zhi Wang (12):
gpu: nova-core: vgpu: add post-GSP-boot vGPU initialization
gpu: nova-core: mm: add VramBlock and Bar1Map
gpu: nova-core: vgpu: add r000 plugin bindings
gpu: nova-core: vgpu: add instance create/destroy
gpu: nova-core: gsp: add GMC transaction helpers
gpu: nova-core: vgpu: add vGPU bootload
gpu: nova-core: vgpu: implement PluginRpc channel and config params
gpu: nova-core: vgpu: scrub guest framebuffer memory with CeUtils
gpu: nova-core: vgpu: export plugin log buffers via debugfs
gpu: nova-core: vgpu: export lifecycle operations to VFIO
vfio/nvidia-vgpu: add the NVIDIA vGPU VFIO variant driver
gpu: nova-core: reserve the 48-VM WPR2 heap
drivers/gpu/nova-core/driver.rs | 42 +-
drivers/gpu/nova-core/fb.rs | 2 +-
drivers/gpu/nova-core/gpu.rs | 184 +++--
drivers/gpu/nova-core/gpu/channel.rs | 3 +
drivers/gpu/nova-core/gsp.rs | 29 +-
drivers/gpu/nova-core/gsp/boot.rs | 13 +-
drivers/gpu/nova-core/gsp/cmdq.rs | 356 +++++++--
drivers/gpu/nova-core/gsp/commands.rs | 78 +-
drivers/gpu/nova-core/gsp/fw.rs | 63 +-
drivers/gpu/nova-core/gsp/fw/commands.rs | 185 ++++-
.../gpu/nova-core/gsp/fw/r000_00/bindings.rs | 175 +++++
drivers/gpu/nova-core/gsp/hal/gh100.rs | 2 +-
drivers/gpu/nova-core/gsp/hal/tu102.rs | 2 +-
drivers/gpu/nova-core/mm.rs | 6 +-
drivers/gpu/nova-core/mm/bar_user.rs | 183 ++++-
drivers/gpu/nova-core/mm/vram.rs | 187 +++++
drivers/gpu/nova-core/nova_core_exports.c | 5 +
drivers/gpu/nova-core/vgpu.rs | 91 ---
drivers/gpu/nova-core/vgpu/bootload.rs | 162 ++++
drivers/gpu/nova-core/vgpu/consts.rs | 33 +
drivers/gpu/nova-core/vgpu/fw.rs | 490 ++++++++++++
drivers/gpu/nova-core/vgpu/fw/commands.rs | 28 +
drivers/gpu/nova-core/vgpu/instance.rs | 705 ++++++++++++++++++
drivers/gpu/nova-core/vgpu/log.rs | 167 +++++
drivers/gpu/nova-core/vgpu/mod.rs | 170 +++++
drivers/gpu/nova-core/vgpu/plugin_rpc.rs | 274 +++++++
drivers/gpu/nova-core/vgpu/scrubber.rs | 474 ++++++++++++
drivers/gpu/nova-core/vgpu/vfio.rs | 282 +++++++
drivers/gpu/nova-core/vgpu/vram.rs | 140 ++++
drivers/vfio/pci/Kconfig | 2 +
drivers/vfio/pci/Makefile | 2 +
drivers/vfio/pci/nvidia-vgpu/Kconfig | 16 +
drivers/vfio/pci/nvidia-vgpu/Makefile | 2 +
drivers/vfio/pci/nvidia-vgpu/main.c | 253 +++++++
include/drm/nvidia_vgpu.h | 28 +
rust/bindings/bindings_helper.h | 1 +
36 files changed, 4536 insertions(+), 299 deletions(-)
create mode 100644 drivers/gpu/nova-core/mm/vram.rs
delete mode 100644 drivers/gpu/nova-core/vgpu.rs
create mode 100644 drivers/gpu/nova-core/vgpu/bootload.rs
create mode 100644 drivers/gpu/nova-core/vgpu/consts.rs
create mode 100644 drivers/gpu/nova-core/vgpu/fw.rs
create mode 100644 drivers/gpu/nova-core/vgpu/fw/commands.rs
create mode 100644 drivers/gpu/nova-core/vgpu/instance.rs
create mode 100644 drivers/gpu/nova-core/vgpu/log.rs
create mode 100644 drivers/gpu/nova-core/vgpu/mod.rs
create mode 100644 drivers/gpu/nova-core/vgpu/plugin_rpc.rs
create mode 100644 drivers/gpu/nova-core/vgpu/scrubber.rs
create mode 100644 drivers/gpu/nova-core/vgpu/vfio.rs
create mode 100644 drivers/gpu/nova-core/vgpu/vram.rs
create mode 100644 drivers/vfio/pci/nvidia-vgpu/Kconfig
create mode 100644 drivers/vfio/pci/nvidia-vgpu/Makefile
create mode 100644 drivers/vfio/pci/nvidia-vgpu/main.c
create mode 100644 include/drm/nvidia_vgpu.h
base-commit: a1dc9fdea9ab15e5df1bd6af1f6de2e40079f02d
--
2.53.0
^ permalink raw reply [flat|nested] 24+ messages in thread* [PATCH 12/13] vfio/nvidia-vgpu: add the NVIDIA vGPU VFIO variant driver 2026-09-05 8:11 [PATCH 00/13] Introduce NVIDIA vGPU manager and VFIO variant driver Zhi Wang @ 2026-09-05 8:11 ` Zhi Wang 2026-09-09 3:00 ` Alex Williamson 2026-09-11 20:39 ` Danilo Krummrich 0 siblings, 2 replies; 24+ messages in thread From: Zhi Wang @ 2026-09-05 8:11 UTC (permalink / raw) To: dakr, acourbot Cc: alex, jgg, yishaih, skolothumtho, kevin.tian, airlied, simona, ojeda, alex.gaynor, boqun.feng, gary, bjorn3_gh, lossin, a.hindborg, aliceryhl, tmgross, jhubbard, ecourtney, cjia, smitra, kjaju, alkumar, ankita, aniketa, kwankhede, targupta, nova-gpu, linux-kernel, zhiwang, Zhi Wang, kvm NVIDIA vGPU VFs require their open, reset, and close lifecycle to be coordinated with the PF-side nova-core driver. Add a VFIO PCI variant driver that binds NVIDIA devices only through driver_override and rejects non-VFs. Delegate instance lifecycle operations to nova-core, present the firmware-selected device and subsystem IDs in configuration-space reads, and adjust the reported BAR1 aperture to the assigned profile. Use vfio-pci-core for the remaining VFIO operations. Signed-off-by: Zhi Wang <zhiw@nvidia.com> --- drivers/vfio/pci/Kconfig | 2 + drivers/vfio/pci/Makefile | 2 + drivers/vfio/pci/nvidia-vgpu/Kconfig | 16 ++ drivers/vfio/pci/nvidia-vgpu/Makefile | 2 + drivers/vfio/pci/nvidia-vgpu/main.c | 253 ++++++++++++++++++++++++++ 5 files changed, 275 insertions(+) create mode 100644 drivers/vfio/pci/nvidia-vgpu/Kconfig create mode 100644 drivers/vfio/pci/nvidia-vgpu/Makefile create mode 100644 drivers/vfio/pci/nvidia-vgpu/main.c diff --git a/drivers/vfio/pci/Kconfig b/drivers/vfio/pci/Kconfig index 296bf01e185e..b48d8d1af42a 100644 --- a/drivers/vfio/pci/Kconfig +++ b/drivers/vfio/pci/Kconfig @@ -74,4 +74,6 @@ source "drivers/vfio/pci/qat/Kconfig" source "drivers/vfio/pci/xe/Kconfig" +source "drivers/vfio/pci/nvidia-vgpu/Kconfig" + endmenu diff --git a/drivers/vfio/pci/Makefile b/drivers/vfio/pci/Makefile index 6138f1bf241d..f3498e541555 100644 --- a/drivers/vfio/pci/Makefile +++ b/drivers/vfio/pci/Makefile @@ -24,3 +24,5 @@ obj-$(CONFIG_NVGRACE_GPU_VFIO_PCI) += nvgrace-gpu/ obj-$(CONFIG_QAT_VFIO_PCI) += qat/ obj-$(CONFIG_XE_VFIO_PCI) += xe/ + +obj-$(CONFIG_NVIDIA_VGPU_VFIO_PCI) += nvidia-vgpu/ diff --git a/drivers/vfio/pci/nvidia-vgpu/Kconfig b/drivers/vfio/pci/nvidia-vgpu/Kconfig new file mode 100644 index 000000000000..098822d32380 --- /dev/null +++ b/drivers/vfio/pci/nvidia-vgpu/Kconfig @@ -0,0 +1,16 @@ +# SPDX-License-Identifier: GPL-2.0-only +config NVIDIA_VGPU_VFIO_PCI + tristate "VFIO support for the NVIDIA vGPU" + depends on NOVA_CORE && PCI_IOV + select VFIO_PCI_CORE + help + This option enables VFIO (Virtual Function I/O) support for + NVIDIA virtual GPUs (vGPU). It allows the assignment of a virtual + GPU instance to userspace applications via VFIO, typically used + with hypervisors such as KVM and device emulators like QEMU. + + The NVIDIA vGPU allows a physical GPU to be partitioned into + multiple virtual GPUs, each of which can be passed to a virtual + machine as a PCI device using the standard VFIO infrastructure. + + If you don't know what to do here, say N. diff --git a/drivers/vfio/pci/nvidia-vgpu/Makefile b/drivers/vfio/pci/nvidia-vgpu/Makefile new file mode 100644 index 000000000000..193cc801a081 --- /dev/null +++ b/drivers/vfio/pci/nvidia-vgpu/Makefile @@ -0,0 +1,2 @@ +obj-$(CONFIG_NVIDIA_VGPU_VFIO_PCI) += nvidia-vgpu-vfio-pci.o +nvidia-vgpu-vfio-pci-y := main.o diff --git a/drivers/vfio/pci/nvidia-vgpu/main.c b/drivers/vfio/pci/nvidia-vgpu/main.c new file mode 100644 index 000000000000..d8626644f952 --- /dev/null +++ b/drivers/vfio/pci/nvidia-vgpu/main.c @@ -0,0 +1,253 @@ +// SPDX-License-Identifier: GPL-2.0-only +#include <linux/module.h> +#include <linux/overflow.h> +#include <linux/pci.h> +#include <linux/pid.h> +#include <linux/vfio_pci_core.h> +#include <drm/nvidia_vgpu.h> + +static int nvidia_vgpu_fb_bar_index(struct pci_dev *pdev) +{ + if (pci_resource_flags(pdev, 0) & IORESOURCE_MEM_64) + return 2; + return 1; +} + +struct nvidia_vgpu_pci_core_device { + struct vfio_pci_core_device core_device; + struct nvidia_vgpu_type_info type_info; + unsigned int gfid; +}; + +static inline unsigned int nvidia_vgpu_vf_dbdf(struct pci_dev *vf) +{ + return ((u32)pci_domain_nr(vf->bus) << 16) | pci_dev_id(vf); +} + +static int nvidia_vgpu_open_device(struct vfio_device *core_vdev) +{ + struct nvidia_vgpu_pci_core_device *nvdev = container_of( + core_vdev, struct nvidia_vgpu_pci_core_device, core_device.vdev); + struct pci_dev *vf = to_pci_dev(core_vdev->dev); + struct nvidia_vgpu_type_info type_info; + int ret; + + if (!vf->is_virtfn) + return -ENODEV; + + ret = vfio_pci_core_enable(&nvdev->core_device); + if (ret) + return ret; + + ret = nvidia_vgpu_open(pci_physfn(vf), nvdev->gfid, + nvidia_vgpu_vf_dbdf(vf), task_tgid_nr(current), + &type_info); + if (ret) { + vfio_pci_core_disable(&nvdev->core_device); + return ret; + } + + nvdev->type_info = type_info; + pci_dbg(vf, "vgpu open: dev_id=0x%x subsys_id=0x%x bar1_length=0x%llx\n", + type_info.pci_dev_id, type_info.pci_subsys_id, + type_info.bar1_length); + vfio_pci_core_finish_enable(&nvdev->core_device); + return 0; +} + +static void nvidia_vgpu_close_device(struct vfio_device *core_vdev) +{ + struct nvidia_vgpu_pci_core_device *nvdev = container_of( + core_vdev, struct nvidia_vgpu_pci_core_device, core_device.vdev); + struct pci_dev *vf = to_pci_dev(core_vdev->dev); + + nvidia_vgpu_close(pci_physfn(vf), nvdev->gfid); + vfio_pci_core_close_device(core_vdev); +} + +static ssize_t nvidia_vgpu_pci_read_config(struct vfio_device *core_vdev, + char __user *buf, size_t count, + loff_t *ppos) +{ + struct nvidia_vgpu_pci_core_device *nvdev = container_of( + core_vdev, struct nvidia_vgpu_pci_core_device, core_device.vdev); + struct nvidia_vgpu_type_info *ti = &nvdev->type_info; + loff_t pos = *ppos & VFIO_PCI_OFFSET_MASK; + size_t register_offset; + loff_t copy_offset; + size_t copy_count; + __le16 val16; + int ret; + + ret = vfio_pci_core_read(core_vdev, buf, count, ppos); + if (ret < 0) + return ret; + + if (vfio_pci_core_range_intersect_range(pos, count, PCI_DEVICE_ID, + sizeof(val16), ©_offset, + ©_count, ®ister_offset)) { + val16 = cpu_to_le16(ti->pci_dev_id); + if (copy_to_user(buf + copy_offset, + (void *)&val16 + register_offset, copy_count)) + return -EFAULT; + } + + if (vfio_pci_core_range_intersect_range(pos, count, PCI_SUBSYSTEM_ID, + sizeof(val16), ©_offset, + ©_count, ®ister_offset)) { + val16 = cpu_to_le16(ti->pci_subsys_id); + if (copy_to_user(buf + copy_offset, + (void *)&val16 + register_offset, copy_count)) + return -EFAULT; + } + + return count; +} + +static ssize_t nvidia_vgpu_pci_read(struct vfio_device *core_vdev, + char __user *buf, size_t count, + loff_t *ppos) +{ + unsigned int index = VFIO_PCI_OFFSET_TO_INDEX(*ppos); + + if (index == VFIO_PCI_CONFIG_REGION_INDEX) + return nvidia_vgpu_pci_read_config(core_vdev, buf, count, ppos); + + return vfio_pci_core_read(core_vdev, buf, count, ppos); +} + +static int nvidia_vgpu_bar1_size(struct nvidia_vgpu_pci_core_device *nvdev, + u64 *size) +{ + if (check_shl_overflow(nvdev->type_info.bar1_length, 20, size)) + return -EOVERFLOW; + + return 0; +} + +static int nvidia_vgpu_get_region_info(struct vfio_device *core_vdev, + struct vfio_region_info *info, + struct vfio_info_cap *caps) +{ + int ret; + + ret = vfio_pci_ioctl_get_region_info(core_vdev, info, caps); + if (ret) + return ret; + + if (info->index == nvidia_vgpu_fb_bar_index( + to_pci_dev(core_vdev->dev)) && info->size) { + struct nvidia_vgpu_pci_core_device *nvdev = container_of( + core_vdev, struct nvidia_vgpu_pci_core_device, + core_device.vdev); + u64 vgpu_bar1; + + ret = nvidia_vgpu_bar1_size(nvdev, &vgpu_bar1); + if (ret) + return ret; + + if (vgpu_bar1 && vgpu_bar1 < info->size) + info->size = vgpu_bar1; + } + + return 0; +} + +static long nvidia_vgpu_pci_ioctl(struct vfio_device *core_vdev, + unsigned int cmd, unsigned long arg) +{ + if (cmd == VFIO_DEVICE_RESET) { + struct nvidia_vgpu_pci_core_device *nvdev = container_of( + core_vdev, struct nvidia_vgpu_pci_core_device, + core_device.vdev); + struct pci_dev *vf = to_pci_dev(core_vdev->dev); + int ret; + + ret = nvidia_vgpu_reset(pci_physfn(vf), nvdev->gfid); + if (ret) + return ret; + } + + return vfio_pci_core_ioctl(core_vdev, cmd, arg); +} + +static const struct vfio_device_ops nvidia_vgpu_pci_ops = { + .name = "nvidia-vgpu-vfio-pci", + .init = vfio_pci_core_init_dev, + .release = vfio_pci_core_release_dev, + .open_device = nvidia_vgpu_open_device, + .close_device = nvidia_vgpu_close_device, + .ioctl = nvidia_vgpu_pci_ioctl, + .get_region_info_caps = nvidia_vgpu_get_region_info, + .device_feature = vfio_pci_core_ioctl_feature, + .read = nvidia_vgpu_pci_read, + .write = vfio_pci_core_write, + .mmap = vfio_pci_core_mmap, + .request = vfio_pci_core_request, + .match = vfio_pci_core_match, + .match_token_uuid = vfio_pci_core_match_token_uuid, + .bind_iommufd = vfio_iommufd_physical_bind, + .unbind_iommufd = vfio_iommufd_physical_unbind, + .attach_ioas = vfio_iommufd_physical_attach_ioas, + .detach_ioas = vfio_iommufd_physical_detach_ioas, +}; + +static int nvidia_vgpu_pci_probe(struct pci_dev *pdev, + const struct pci_device_id *id) +{ + struct nvidia_vgpu_pci_core_device *nvdev; + int vf_id; + int ret; + + if (!pdev->is_virtfn) + return -ENODEV; + + vf_id = pci_iov_vf_id(pdev); + if (vf_id < 0) + return vf_id; + + nvdev = vfio_alloc_device(nvidia_vgpu_pci_core_device, core_device.vdev, + &pdev->dev, &nvidia_vgpu_pci_ops); + if (IS_ERR(nvdev)) + return PTR_ERR(nvdev); + + nvdev->gfid = vf_id + 1; + dev_set_drvdata(&pdev->dev, &nvdev->core_device); + ret = vfio_pci_core_register_device(&nvdev->core_device); + if (ret) + goto out_put_vdev; + + return 0; + +out_put_vdev: + vfio_put_device(&nvdev->core_device.vdev); + return ret; +} + +static void nvidia_vgpu_pci_remove(struct pci_dev *pdev) +{ + struct vfio_pci_core_device *core_device = dev_get_drvdata(&pdev->dev); + + vfio_pci_core_unregister_device(core_device); + vfio_put_device(&core_device->vdev); +} + +static const struct pci_device_id nvidia_vgpu_pci_table[] = { + /* Placeholder: match all NVIDIA VFs (vendor 0x10de) */ + { PCI_DRIVER_OVERRIDE_DEVICE_VFIO(PCI_VENDOR_ID_NVIDIA, PCI_ANY_ID) }, + {} +}; +MODULE_DEVICE_TABLE(pci, nvidia_vgpu_pci_table); + +static struct pci_driver nvidia_vgpu_pci_driver = { + .name = "nvidia-vgpu-vfio-pci", + .id_table = nvidia_vgpu_pci_table, + .probe = nvidia_vgpu_pci_probe, + .remove = nvidia_vgpu_pci_remove, + .driver_managed_dma = true, +}; +module_pci_driver(nvidia_vgpu_pci_driver); + +MODULE_DESCRIPTION("NVIDIA vGPU vfio-pci driver"); +MODULE_LICENSE("GPL"); +MODULE_IMPORT_NS("NOVA_CORE_VGPU"); -- 2.53.0 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH 12/13] vfio/nvidia-vgpu: add the NVIDIA vGPU VFIO variant driver 2026-09-05 8:11 ` [PATCH 12/13] vfio/nvidia-vgpu: add the NVIDIA vGPU " Zhi Wang @ 2026-09-09 3:00 ` Alex Williamson 2026-09-11 20:39 ` Danilo Krummrich 1 sibling, 0 replies; 24+ messages in thread From: Alex Williamson @ 2026-09-09 3:00 UTC (permalink / raw) To: Zhi Wang Cc: dakr, acourbot, jgg, yishaih, skolothumtho, kevin.tian, airlied, simona, ojeda, alex.gaynor, boqun.feng, gary, bjorn3_gh, lossin, a.hindborg, aliceryhl, tmgross, jhubbard, ecourtney, cjia, smitra, kjaju, alkumar, ankita, aniketa, kwankhede, targupta, nova-gpu, linux-kernel, zhiwang, kvm, alex On Sat, 5 Sep 2026 11:11:15 +0300 Zhi Wang <zhiw@nvidia.com> wrote: > NVIDIA vGPU VFs require their open, reset, and close lifecycle to be > coordinated with the PF-side nova-core driver. > > Add a VFIO PCI variant driver that binds NVIDIA devices only through > driver_override and rejects non-VFs. Delegate instance lifecycle > operations to nova-core, present the firmware-selected device and > subsystem IDs in configuration-space reads, and adjust the reported BAR1 > aperture to the assigned profile. Use vfio-pci-core for the remaining > VFIO operations. > > Signed-off-by: Zhi Wang <zhiw@nvidia.com> > --- > drivers/vfio/pci/Kconfig | 2 + > drivers/vfio/pci/Makefile | 2 + > drivers/vfio/pci/nvidia-vgpu/Kconfig | 16 ++ > drivers/vfio/pci/nvidia-vgpu/Makefile | 2 + > drivers/vfio/pci/nvidia-vgpu/main.c | 253 ++++++++++++++++++++++++++ > 5 files changed, 275 insertions(+) > create mode 100644 drivers/vfio/pci/nvidia-vgpu/Kconfig > create mode 100644 drivers/vfio/pci/nvidia-vgpu/Makefile > create mode 100644 drivers/vfio/pci/nvidia-vgpu/main.c > > diff --git a/drivers/vfio/pci/Kconfig b/drivers/vfio/pci/Kconfig > index 296bf01e185e..b48d8d1af42a 100644 > --- a/drivers/vfio/pci/Kconfig > +++ b/drivers/vfio/pci/Kconfig > @@ -74,4 +74,6 @@ source "drivers/vfio/pci/qat/Kconfig" > > source "drivers/vfio/pci/xe/Kconfig" > > +source "drivers/vfio/pci/nvidia-vgpu/Kconfig" > + > endmenu > diff --git a/drivers/vfio/pci/Makefile b/drivers/vfio/pci/Makefile > index 6138f1bf241d..f3498e541555 100644 > --- a/drivers/vfio/pci/Makefile > +++ b/drivers/vfio/pci/Makefile > @@ -24,3 +24,5 @@ obj-$(CONFIG_NVGRACE_GPU_VFIO_PCI) += nvgrace-gpu/ > obj-$(CONFIG_QAT_VFIO_PCI) += qat/ > > obj-$(CONFIG_XE_VFIO_PCI) += xe/ > + > +obj-$(CONFIG_NVIDIA_VGPU_VFIO_PCI) += nvidia-vgpu/ > diff --git a/drivers/vfio/pci/nvidia-vgpu/Kconfig b/drivers/vfio/pci/nvidia-vgpu/Kconfig > new file mode 100644 > index 000000000000..098822d32380 > --- /dev/null > +++ b/drivers/vfio/pci/nvidia-vgpu/Kconfig > @@ -0,0 +1,16 @@ > +# SPDX-License-Identifier: GPL-2.0-only > +config NVIDIA_VGPU_VFIO_PCI > + tristate "VFIO support for the NVIDIA vGPU" > + depends on NOVA_CORE && PCI_IOV > + select VFIO_PCI_CORE > + help > + This option enables VFIO (Virtual Function I/O) support for > + NVIDIA virtual GPUs (vGPU). It allows the assignment of a virtual > + GPU instance to userspace applications via VFIO, typically used > + with hypervisors such as KVM and device emulators like QEMU. > + > + The NVIDIA vGPU allows a physical GPU to be partitioned into > + multiple virtual GPUs, each of which can be passed to a virtual > + machine as a PCI device using the standard VFIO infrastructure. > + > + If you don't know what to do here, say N. > diff --git a/drivers/vfio/pci/nvidia-vgpu/Makefile b/drivers/vfio/pci/nvidia-vgpu/Makefile > new file mode 100644 > index 000000000000..193cc801a081 > --- /dev/null > +++ b/drivers/vfio/pci/nvidia-vgpu/Makefile > @@ -0,0 +1,2 @@ > +obj-$(CONFIG_NVIDIA_VGPU_VFIO_PCI) += nvidia-vgpu-vfio-pci.o > +nvidia-vgpu-vfio-pci-y := main.o > diff --git a/drivers/vfio/pci/nvidia-vgpu/main.c b/drivers/vfio/pci/nvidia-vgpu/main.c > new file mode 100644 > index 000000000000..d8626644f952 > --- /dev/null > +++ b/drivers/vfio/pci/nvidia-vgpu/main.c > @@ -0,0 +1,253 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +#include <linux/module.h> > +#include <linux/overflow.h> > +#include <linux/pci.h> > +#include <linux/pid.h> > +#include <linux/vfio_pci_core.h> > +#include <drm/nvidia_vgpu.h> > + > +static int nvidia_vgpu_fb_bar_index(struct pci_dev *pdev) > +{ > + if (pci_resource_flags(pdev, 0) & IORESOURCE_MEM_64) > + return 2; > + return 1; > +} > + > +struct nvidia_vgpu_pci_core_device { > + struct vfio_pci_core_device core_device; > + struct nvidia_vgpu_type_info type_info; > + unsigned int gfid; > +}; > + This is more commonly called an "sbdf". Also, consider some comments. > +static inline unsigned int nvidia_vgpu_vf_dbdf(struct pci_dev *vf) > +{ > + return ((u32)pci_domain_nr(vf->bus) << 16) | pci_dev_id(vf); > +} > + > +static int nvidia_vgpu_open_device(struct vfio_device *core_vdev) > +{ > + struct nvidia_vgpu_pci_core_device *nvdev = container_of( > + core_vdev, struct nvidia_vgpu_pci_core_device, core_device.vdev); > + struct pci_dev *vf = to_pci_dev(core_vdev->dev); > + struct nvidia_vgpu_type_info type_info; > + int ret; > + > + if (!vf->is_virtfn) > + return -ENODEV; This is redundant to the probe check. > + > + ret = vfio_pci_core_enable(&nvdev->core_device); > + if (ret) > + return ret; > + > + ret = nvidia_vgpu_open(pci_physfn(vf), nvdev->gfid, > + nvidia_vgpu_vf_dbdf(vf), task_tgid_nr(current), > + &type_info); > + if (ret) { > + vfio_pci_core_disable(&nvdev->core_device); > + return ret; > + } > + > + nvdev->type_info = type_info; > + pci_dbg(vf, "vgpu open: dev_id=0x%x subsys_id=0x%x bar1_length=0x%llx\n", > + type_info.pci_dev_id, type_info.pci_subsys_id, > + type_info.bar1_length); > + vfio_pci_core_finish_enable(&nvdev->core_device); > + return 0; > +} > + > +static void nvidia_vgpu_close_device(struct vfio_device *core_vdev) > +{ > + struct nvidia_vgpu_pci_core_device *nvdev = container_of( > + core_vdev, struct nvidia_vgpu_pci_core_device, core_device.vdev); > + struct pci_dev *vf = to_pci_dev(core_vdev->dev); > + > + nvidia_vgpu_close(pci_physfn(vf), nvdev->gfid); > + vfio_pci_core_close_device(core_vdev); > +} > + > +static ssize_t nvidia_vgpu_pci_read_config(struct vfio_device *core_vdev, > + char __user *buf, size_t count, > + loff_t *ppos) > +{ > + struct nvidia_vgpu_pci_core_device *nvdev = container_of( > + core_vdev, struct nvidia_vgpu_pci_core_device, core_device.vdev); > + struct nvidia_vgpu_type_info *ti = &nvdev->type_info; > + loff_t pos = *ppos & VFIO_PCI_OFFSET_MASK; > + size_t register_offset; > + loff_t copy_offset; > + size_t copy_count; > + __le16 val16; > + int ret; > + > + ret = vfio_pci_core_read(core_vdev, buf, count, ppos); > + if (ret < 0) > + return ret; > + > + if (vfio_pci_core_range_intersect_range(pos, count, PCI_DEVICE_ID, > + sizeof(val16), ©_offset, > + ©_count, ®ister_offset)) { > + val16 = cpu_to_le16(ti->pci_dev_id); > + if (copy_to_user(buf + copy_offset, > + (void *)&val16 + register_offset, copy_count)) > + return -EFAULT; > + } Just stuff the device ID into vconfig, it's already read from there. > + > + if (vfio_pci_core_range_intersect_range(pos, count, PCI_SUBSYSTEM_ID, > + sizeof(val16), ©_offset, > + ©_count, ®ister_offset)) { > + val16 = cpu_to_le16(ti->pci_subsys_id); > + if (copy_to_user(buf + copy_offset, > + (void *)&val16 + register_offset, copy_count)) > + return -EFAULT; > + } There's possibly an argument to be made that subsystem ID could be read from vconfig by default too so it could be pre-filled after vfio_config_init(), ie. after vfio_pci_core_enable(). The only reason it might change would be if firmware was updated, but a firmware update through vfio that changes the subsystem ID would be pretty sketchy already. > + > + return count; > +} > + > +static ssize_t nvidia_vgpu_pci_read(struct vfio_device *core_vdev, > + char __user *buf, size_t count, > + loff_t *ppos) > +{ > + unsigned int index = VFIO_PCI_OFFSET_TO_INDEX(*ppos); > + > + if (index == VFIO_PCI_CONFIG_REGION_INDEX) > + return nvidia_vgpu_pci_read_config(core_vdev, buf, count, ppos); > + > + return vfio_pci_core_read(core_vdev, buf, count, ppos); > +} > + > +static int nvidia_vgpu_bar1_size(struct nvidia_vgpu_pci_core_device *nvdev, > + u64 *size) > +{ > + if (check_shl_overflow(nvdev->type_info.bar1_length, 20, size)) > + return -EOVERFLOW; > + > + return 0; > +} > + > +static int nvidia_vgpu_get_region_info(struct vfio_device *core_vdev, > + struct vfio_region_info *info, > + struct vfio_info_cap *caps) > +{ > + int ret; > + > + ret = vfio_pci_ioctl_get_region_info(core_vdev, info, caps); > + if (ret) > + return ret; > + > + if (info->index == nvidia_vgpu_fb_bar_index( > + to_pci_dev(core_vdev->dev)) && info->size) { > + struct nvidia_vgpu_pci_core_device *nvdev = container_of( > + core_vdev, struct nvidia_vgpu_pci_core_device, > + core_device.vdev); > + u64 vgpu_bar1; > + > + ret = nvidia_vgpu_bar1_size(nvdev, &vgpu_bar1); > + if (ret) > + return ret; > + > + if (vgpu_bar1 && vgpu_bar1 < info->size) > + info->size = vgpu_bar1; > + } So we're changing the reported BAR1 size, but just trusting that userspace honors that size for read/write/mmap? Again, consider some comments. > + > + return 0; > +} > + > +static long nvidia_vgpu_pci_ioctl(struct vfio_device *core_vdev, > + unsigned int cmd, unsigned long arg) > +{ > + if (cmd == VFIO_DEVICE_RESET) { > + struct nvidia_vgpu_pci_core_device *nvdev = container_of( > + core_vdev, struct nvidia_vgpu_pci_core_device, > + core_device.vdev); > + struct pci_dev *vf = to_pci_dev(core_vdev->dev); > + int ret; > + > + ret = nvidia_vgpu_reset(pci_physfn(vf), nvdev->gfid); > + if (ret) > + return ret; > + } > + > + return vfio_pci_core_ioctl(core_vdev, cmd, arg); What about reset invoked through FLR? Would this be better served through .reset_prepare and .reset_done? > +} > + > +static const struct vfio_device_ops nvidia_vgpu_pci_ops = { > + .name = "nvidia-vgpu-vfio-pci", > + .init = vfio_pci_core_init_dev, > + .release = vfio_pci_core_release_dev, > + .open_device = nvidia_vgpu_open_device, > + .close_device = nvidia_vgpu_close_device, > + .ioctl = nvidia_vgpu_pci_ioctl, > + .get_region_info_caps = nvidia_vgpu_get_region_info, > + .device_feature = vfio_pci_core_ioctl_feature, > + .read = nvidia_vgpu_pci_read, > + .write = vfio_pci_core_write, > + .mmap = vfio_pci_core_mmap, > + .request = vfio_pci_core_request, > + .match = vfio_pci_core_match, > + .match_token_uuid = vfio_pci_core_match_token_uuid, > + .bind_iommufd = vfio_iommufd_physical_bind, > + .unbind_iommufd = vfio_iommufd_physical_unbind, > + .attach_ioas = vfio_iommufd_physical_attach_ioas, > + .detach_ioas = vfio_iommufd_physical_detach_ioas, > +}; > + > +static int nvidia_vgpu_pci_probe(struct pci_dev *pdev, > + const struct pci_device_id *id) > +{ > + struct nvidia_vgpu_pci_core_device *nvdev; > + int vf_id; > + int ret; > + > + if (!pdev->is_virtfn) > + return -ENODEV; This driver needs to bind to what it matches in the id table, we don't have a policy for userspace to pick a 2nd best variant driver. hisi_acc handles a similar situation where only the VFs are supported for the migration feature of the variant driver. The PF needs to be supported here and bind to a vfio-pci-core passthrough ops structure. We should probably define a PCI_DRIVER_OVERRIDE_DEVICE_VFIO variant that allows a class code to be specified so we aren't using this for all 10de: devices. I'm hoping that class code is for a 3D accelerator or the like rather than VGA class (a VF can't technically support a legacy endpoint anyway), but we need to consider what existing devices that currently use vfio-pci would now be bound to this driver and what module option features they might use. Thanks, Alex > + > + vf_id = pci_iov_vf_id(pdev); > + if (vf_id < 0) > + return vf_id; > + > + nvdev = vfio_alloc_device(nvidia_vgpu_pci_core_device, core_device.vdev, > + &pdev->dev, &nvidia_vgpu_pci_ops); > + if (IS_ERR(nvdev)) > + return PTR_ERR(nvdev); > + > + nvdev->gfid = vf_id + 1; > + dev_set_drvdata(&pdev->dev, &nvdev->core_device); > + ret = vfio_pci_core_register_device(&nvdev->core_device); > + if (ret) > + goto out_put_vdev; > + > + return 0; > + > +out_put_vdev: > + vfio_put_device(&nvdev->core_device.vdev); > + return ret; > +} > + > +static void nvidia_vgpu_pci_remove(struct pci_dev *pdev) > +{ > + struct vfio_pci_core_device *core_device = dev_get_drvdata(&pdev->dev); > + > + vfio_pci_core_unregister_device(core_device); > + vfio_put_device(&core_device->vdev); > +} > + > +static const struct pci_device_id nvidia_vgpu_pci_table[] = { > + /* Placeholder: match all NVIDIA VFs (vendor 0x10de) */ > + { PCI_DRIVER_OVERRIDE_DEVICE_VFIO(PCI_VENDOR_ID_NVIDIA, PCI_ANY_ID) }, > + {} > +}; > +MODULE_DEVICE_TABLE(pci, nvidia_vgpu_pci_table); > + > +static struct pci_driver nvidia_vgpu_pci_driver = { > + .name = "nvidia-vgpu-vfio-pci", > + .id_table = nvidia_vgpu_pci_table, > + .probe = nvidia_vgpu_pci_probe, > + .remove = nvidia_vgpu_pci_remove, > + .driver_managed_dma = true, > +}; > +module_pci_driver(nvidia_vgpu_pci_driver); > + > +MODULE_DESCRIPTION("NVIDIA vGPU vfio-pci driver"); > +MODULE_LICENSE("GPL"); > +MODULE_IMPORT_NS("NOVA_CORE_VGPU"); ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 12/13] vfio/nvidia-vgpu: add the NVIDIA vGPU VFIO variant driver 2026-09-05 8:11 ` [PATCH 12/13] vfio/nvidia-vgpu: add the NVIDIA vGPU " Zhi Wang 2026-09-09 3:00 ` Alex Williamson @ 2026-09-11 20:39 ` Danilo Krummrich 2026-09-14 18:12 ` Alex Williamson 1 sibling, 1 reply; 24+ messages in thread From: Danilo Krummrich @ 2026-09-11 20:39 UTC (permalink / raw) To: Alex Williamson, Jason Gunthorpe, Zhi Wang Cc: acourbot, yishaih, skolothumtho, kevin.tian, airlied, simona, ojeda, alex.gaynor, boqun.feng, gary, bjorn3_gh, lossin, a.hindborg, aliceryhl, tmgross, jhubbard, ecourtney, cjia, smitra, kjaju, alkumar, ankita, aniketa, kwankhede, targupta, nova-gpu, linux-kernel, zhiwang, kvm 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. I did sketch up the necessary code for this in [1], which contains the VFIO/PCI Rust infrastructure [2], a Rust implementation of the NVIDIA vGPU driver [3] and the required PCI SR-IOV infrastructure [4]. This is PoC code; I focused on the general design, and I've tested the VFIO bits to a point that I can poke the character device from userspace. But it still needs a bit of cleanup and probably a few additional new type abstractions over primitive types, etc. As conferences are approaching and I have a bunch of stuff to prepare I could only finish it up after LPC, but I hope that Zhi is also interested in picking it up. :) I'm aware that you may have some concerns about Rust code in VFIO and one of them might be maintainance. I already have a lot on my plate, but I'm happy to offer to take responsibility. It would also be great if Zhi were interested in helping maintain them. In this context, I'd like to walk through the design and a few code examples below. We can also follow up at LPC, perhaps as part of the Nova workshop. (Zhi, would you be interested in preparing a brief session on this with me?) In general, the VFIO/PCI Rust code is not much different from the FWCTL and DRM code that we have upstream already. In the end they are all the same design in terms of the Rust class device lifetime model. Below is a PCI driver skeleton that implements a VFIO/PCI class device (stripped down version of the NVIDIA vGPU driver), with a bunch of comments. #[pin_data] struct NvidiaVgpuData<'bound> { _reg: vfio::pci::Registration<'bound, NvidiaVgpuOps>, } This is the driver's bus device private data. The driver core uses a RAII approach for this, it creates the driver's bus device private data from the initializer returned from probe() and calls the destructor of the driver's bus device private data on remove(), which is the equivalent of a remove() callback in C. In this case it just contains the vfio::pci::Registration, which essentially is a RAII type for vfio_pci_core_register_device() and vfio_pci_core_unregister_device(). (I'm aware that that vfio_pci_core_register_device() currently requires the driver's bus device private data to be set to a struct vfio_pci_core_device pointer as a trick to support PCI bus callbacks, such as with vfio_pci_core_aer_err_detected(). I fixed this up in [5].) #[pin_data] pub struct NvidiaVgpuRegData<'a> { pdev: &'a pci::Device<Bound>, api: NovaCoreVfApiHandle<'a>, } This is the private data attached to the vfio::pci::Registration, which is accessible from the callbacks in struct vfio_device_ops. You may wonder why the private data is on the vfio::pci::Registration rather than the vfio::pci::Device. The reason is that the callbacks in struct vfio_device_ops are lifetime wise associated with the vfio::pci::Registration and not the vfio::pci::Device, so the destructor of this data should be called when the destructor of vfio::pci::Registration runs, i.e. directly after vfio_pci_core_unregister_device(). Furthermore, it allows us to store device resources, such as a DMA coherent allocation, within this data in the first place, as the Rust compiler will ensure that a vfio::pci::Registration can't outlive driver unbind, since its lifetime is bound to the bus driver's bus device private data. (This is also why NvidiaVgpuRegData<'a> can store e.g. &'a pci::Device<Bound>; the lifetime 'a is guaranteed to be shorter lived than the 'bound lifetime that describes the lifetime of the driver being bound to a device.) The vfio::pci::Device on the other hand is reference counted and has an unbounded lifetime. Note that in the end this is only a logical distinction for when the destructor is called. The actual memory this data goes into does not matter too much; it can be a new allocation, it can be the allocation of the struct vfio_pci_core_device (as it is in C), since the struct vfio_pci_core_device strictly outlives the vfio::pci::Registration, or it can also be the driver's bus device private data allocation itself. kernel::pci_device_table!( PCI_TABLE, <NvidiaVgpuDriver as pci::Driver>::IdInfo, [ ( pci::DeviceId::from_class_and_vendor_vfio_override( Class::DISPLAY_VGA, ClassMask::ClassSubclass, Vendor::NVIDIA ), () ), ( pci::DeviceId::from_class_and_vendor_vfio_override( Class::DISPLAY_3D, ClassMask::ClassSubclass, Vendor::NVIDIA ), () ), ] ); The device ID table. It has some nice compile time guarantees as well, but it is otherwise not very interesting in this context. Let's look at the pci::Driver trait, which provides the callbacks (such as probe()) and associated constants and types instead. impl pci::Driver for NvidiaVgpuDriver { type IdInfo = (); type Data<'bound> = NvidiaVgpuData<'bound>; const ID_TABLE: pci::IdTable<Self::IdInfo> = &PCI_TABLE; const DRIVER_MANAGED_DMA: bool = true; fn probe<'bound>( pdev: &'bound pci::Device<Core<'_>>, _info: Option<&'bound Self::IdInfo>, ) -> impl PinInit<Self::Data<'bound>, Error> + 'bound { Note the 'bound lifetime in the signature of probe(); it represents the lifetime of the driver's bus device private data and hence the lifetime of the driver being bound to a device. let vdev = vfio::pci::Device::<NvidiaVgpuOps>::new(pdev)?; This represents a struct vfio_pci_core_device and is typed over an implementation of vfio::pci::Operations (i.e. a struct vfio_device_ops), but is otherwise not very interesting. try_pin_init!(Self::Data { // SAFETY: The registration is dropped when the PCI driver is unbound. _reg <- unsafe { vfio::pci::Registration::new( pdev, &vdev, try_pin_init!(NvidiaVgpuRegData { pdev, api: NovaCoreVfApi::handle(pdev)?, }), )}, Here we create the vfio::pci::Registration (i.e. call vfio_pci_core_register_device()). It takes three arguments, a &vfio::pci::Device, the private data and a &'bound pci::Device<Bound>. The vfio::pci::Registration captures the lifetime ('bound) of the &'bound pci::Device<Bound>, such that it can't outlive driver unbind. In practice this is ensured as the compiler won't allow the vfio::pci::Registration to be stored anywhere else as in a place that is either the bus device private data itself or something else that is strictly shorter lived. The call to NovaCoreVfApi::handle(pdev)? calls into nova-core, which will provide a handle to the nova-core API representation. This handle ties back to nova-core private data that by itself is shorter lived than nova-core's 'bound lifetime, but longer lived than vGPU's 'bound lifetime. IOW, the data is guaranteed to be valid for the full lifecycle of vfio::pci::Registration and hence can be stored within its private data. I will come back to how this guarantee is upheld below. For now, let's have a look at how vfio::pci::Operations (i.e. struct vfio_device_ops) is represented. }) } } #[pin_data] struct NvidiaVgpuOpenData<'a> { instance: VgpuInstance<'a>, } This type (yes, Rust loves new types :) represents data that lives from open_device() until close_device(). Note that the implementation below does not have close_device() at all, as the destructor of the OpenData already represents close_device() as a RAII type. This is also what makes the API with nova-core much better, since... impl vfio::pci::Operations for NvidiaVgpuOps { const NAME: &'static CStr = c"nvidia-vgpu-vfio-pci"; type RegistrationData<'a> = NvidiaVgpuRegData<'a>; type OpenData<'a> = NvidiaVgpuOpenData<'a>; fn open_device<'a>( _dev: &'a vfio::pci::Device<Self>, rd: &'a Self::RegistrationData<'a>, ) -> impl PinInit<Self::OpenData<'a>, Error> + 'a { try_pin_init!(NvidiaVgpuOpenData { instance: rd.api.open(), }) ...here we can just call into nova-core via the API handle and obtain a VgpuInstance<'a> struct from nova-core. Where nova-core can just store all the objects that should be destructed on close_device() in the VgpuInstance struct. This way we avoid an API contract where we have to translate a RAII based design into procedural cleanup and vice versa. Also note how we can represent that OpenData is strictly shorter lived as RegistrationData and the driver's bus device private data, in a way that the Rust compiler can ensure this. } fn ioctl<'a>( dev: &vfio::pci::Device<Self, Ioctl>, _rd: &Self::RegistrationData<'a>, open_data: Pin<&Self::OpenData<'a>>, cmd: u32, arg: usize, ) -> Result<isize> { // Handle driver ioctl. dev.core_ioctl(cmd, arg) } fn read<'a>( dev: &vfio::pci::Device<Self, Read>, _rd: &Self::RegistrationData<'a>, open_data: Pin<&Self::OpenData<'a>>, buf: &mut vfio::UserBuf, ppos: &mut vfio::pci::Position<'_>, ) -> Result<isize> { Ok(0) } fn get_region_info<'a>( dev: &vfio::pci::Device<Self, GetRegionInfo>, rd: &Self::RegistrationData<'a>, open_data: Pin<&Self::OpenData<'a>>, info: &mut bindings::vfio_region_info, caps: &mut vfio::InfoCap<'_>, ) -> Result { Ok(()) } } The rest of the callbacks is not too interesting. The main thing to note is that the type state on the &vfio::pci::Device in e.g. ioctl() allows us to ensure that dev.core_ioctl() can only be called in ioctl() as it is only implemented for &vfio::pci::Device<_, Ioctl> and we only ever give out a &vfio::pci::Device<_, Ioctl> in ioctl(). In the VFIO/PCI code I only implemented the callbacks the NVIDIA vGPU driver needs; all other callbacks can just remain the default trampolines for now. Now, I promised to come back to how the following call works. NovaCoreVfApi::handle(vf_pdev)? As mentioned it provides a handle to the nova-core API representation, which is shorter lived than nova-core's 'bound lifetime, but longer lived than vGPU's 'bound lifetime (and therefore always valid). This is ensured by how I think we should implement the handling of the PF and VF relationship on the PCI bus. Let's have a look at nova-core's probe() for this: impl pci::Driver for NovaCoreDriver { type IdInfo = (); type Data<'bound> = NovaCore<'bound>; const ID_TABLE: pci::IdTable<Self::IdInfo> = &PCI_TABLE; fn probe<'bound>( pdev: &'bound pci::Device<Core<'_>>, _info: Option<&'bound Self::IdInfo>, ) -> impl PinInit<Self::Data<'bound>, Error> + 'bound { try_pin_init!(NovaCore { _enable: { let enable = pdev.enable_device()?; pdev.set_master(); enable }, gpu <- Gpu::new(pdev, pdev.iomap_region_sized::<BAR0_SIZE>(0, c"nova-core/bar0")?), // SAFETY: `NovaCore` is dropped when the device is unbound. _reg: unsafe { auxiliary::Registration::new_with_lt( pdev.as_ref(), c"nova-drm", AUXILIARY_ID_COUNTER.fetch_add(1, Relaxed), crate::MODULE_NAME, NovaCoreApi { gpu: gpu.get_ref(), pdev }, )? }, // SAFETY: `NovaCore` is dropped when the device is unbound. _vf_reg <- unsafe { let total_vfs = pdev.sriov_get_totalvfs().map_or(0, |v| v.get()); pci::VfRegistration::new( pdev, total_vfs, total_vfs > 0, NovaCoreVfApi { _gpu: gpu.get_ref(), pdev }, ) }, }) } } Similar to vfio::pci::Registration and auxiliary::Registration, we have a pci::VfRegistration, which can only be constructed once by a PF; for VFs it fails to construct. This pci::VfRegistration returns an initializer and lives within the driver's bus device private data allocation. The constructor of pci::VfRegistration takes the private data type that should be shared with VFs. IOW, we do not share the whole driver's bus device private data with the VFs, but just a defined container within the driver's bus device private data. This is also what we do for all other kinds of registrations, such as irq::Registration, which has the advantage that it fundamentally prevents ordering issues. For instance, when constructing an irq::Registration the IRQ private data container is guaranteed to be fully initialized before the first IRQ is received, whereas the rest of the driver's bus device private data may not be initialized yet. For the pci::VfRegistration this isn't a concern, as it would be valid to expose the entire driver's bus device private data, but it is still cleaner if a PF does not need to expose its whole bus device private data to the VFs, but just the intended API type. The required lifetime guarantee comes from the fact the pci::VfRegistration lives in the driver's bus device private data, and serves as a guard that calls pci_disable_sriov() in its destructor. This way we also do not need the patch in [6]. I still think it would be reasonable to have this, but the pci::VfRegistration approach is cleaner. In any case, it's not an either-or, we can have both. Coming back to nova-core's probe() above, we can see how this perfectly aligns with how the API between nova-core and nova-drm works via the auxiliary bus. Implementation wise the API on the nova-core side looks like this: pub struct NovaCoreVfApi<'a> { pub(crate) pdev: &'a pci::Device<device::Bound>, pub(crate) _gpu: &'a Gpu<'a>, } /// Closure-based handle to the nova-core VF API. pub struct NovaCoreVfApiHandle<'a> { vf: &'a pci::Device<device::Bound>, } /// An active vGPU instance, closed on drop while the VF binding is still valid. pub struct VgpuInstance<'a> { api: &'a NovaCoreVfApiHandle<'a>, } impl NovaCoreVfApi<'_> { /// Obtain a [`NovaCoreVfApiHandle`] from a VF registered by nova-core. pub fn handle(vf: &pci::Device<device::Bound>) -> Result<NovaCoreVfApiHandle<'_>> { NovaCoreVfApiHandle::of(vf) } } impl<'a> NovaCoreVfApiHandle<'a> { fn of(vf: &'a pci::Device<device::Bound>) -> Result<Self> { vf.vf_registration_data_with::<ForLt!(NovaCoreVfApi<'_>), ()>(|_| ())?; Ok(Self { vf }) } /// Activate a vGPU instance, which is closed on drop. pub fn open(&self) -> Result<VgpuInstance<'a>> { VgpuInstance::new(self) } /// Access the [`NovaCoreVfApi`] through a closure. pub fn with<R>(&self, f: impl for<'b> FnOnce(Pin<&NovaCoreVfApi<'b>>) -> R) -> R { self.vf .vf_registration_data_with::<ForLt!(NovaCoreVfApi<'_>), R>(f) .expect("TypeId was validated in NovaCoreVfApiHandle::of()") } } impl<'a> VgpuInstance<'a> { fn new(api: NovaCoreVfApiHandle<'a>) -> Result<Self> { // TODO: Create the vGPU instance object via `api.gpu`. Ok(Self { api }) } /// Reset this vGPU instance. pub fn reset(&self) -> Result { Ok(()) } } (Don't worry too much about the NovaCoreVfApiHandle::of() and NovaCoreVfApiHandle::with() stuff. Those are helpers we also have in the nova-drm API to deal with invariant or non-covariant types respectively.) 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. Thanks, Danilo [1] https://git.kernel.org/pub/scm/linux/kernel/git/dakr/linux.git/log/?h=poc/vgpu [2] https://git.kernel.org/pub/scm/linux/kernel/git/dakr/linux.git/commit/?id=484316d855e3d61873122c295feb9a7457eeca21 [3] https://git.kernel.org/pub/scm/linux/kernel/git/dakr/linux.git/commit/?id=6292c1de7f0758dd31496a454b4034507f38bd40 [4] https://git.kernel.org/pub/scm/linux/kernel/git/dakr/linux.git/commit/?id=efac2cec97eab36edd01a2fe79aea67a04bd8842 [5] https://git.kernel.org/pub/scm/linux/kernel/git/dakr/linux.git/commit/?id=76b3bfd6386a01f338780e1a37b5bad3f5a48d31 [6] https://lore.kernel.org/lkml/20260303-rust-pci-sriov-v3-1-4443c35f0c88@redhat.com/ ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 12/13] vfio/nvidia-vgpu: add the NVIDIA vGPU VFIO variant driver 2026-09-11 20:39 ` Danilo Krummrich @ 2026-09-14 18:12 ` Alex Williamson 2026-09-14 21:36 ` Danilo Krummrich 0 siblings, 1 reply; 24+ messages in thread From: Alex Williamson @ 2026-09-14 18:12 UTC (permalink / raw) To: Danilo Krummrich Cc: Jason Gunthorpe, Zhi Wang, acourbot, yishaih, skolothumtho, kevin.tian, airlied, simona, ojeda, alex.gaynor, boqun.feng, gary, bjorn3_gh, lossin, a.hindborg, aliceryhl, tmgross, jhubbard, ecourtney, cjia, smitra, kjaju, alkumar, ankita, aniketa, kwankhede, targupta, nova-gpu, linux-kernel, zhiwang, kvm, alex 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. Maybe Dave Airlie can share some experience here with a Rust driver growing up within a subsystem for a non-Rust-literate maintainer. 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. 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. 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. 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, then this variant driver must immediately provide feature parity of vfio-pci with pass-through to vfio-pci-core for matched devices. Libvirt selects a best matching variant driver by modalias with our override scheme. I'd also like to hear from other core vfio contributors. Thanks, Alex > [1] https://git.kernel.org/pub/scm/linux/kernel/git/dakr/linux.git/log/?h=poc/vgpu > [2] https://git.kernel.org/pub/scm/linux/kernel/git/dakr/linux.git/commit/?id=484316d855e3d61873122c295feb9a7457eeca21 > [3] https://git.kernel.org/pub/scm/linux/kernel/git/dakr/linux.git/commit/?id=6292c1de7f0758dd31496a454b4034507f38bd40 > [4] https://git.kernel.org/pub/scm/linux/kernel/git/dakr/linux.git/commit/?id=efac2cec97eab36edd01a2fe79aea67a04bd8842 > [5] https://git.kernel.org/pub/scm/linux/kernel/git/dakr/linux.git/commit/?id=76b3bfd6386a01f338780e1a37b5bad3f5a48d31 > [6] https://lore.kernel.org/lkml/20260303-rust-pci-sriov-v3-1-4443c35f0c88@redhat.com/ ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 12/13] vfio/nvidia-vgpu: add the NVIDIA vGPU VFIO variant driver 2026-09-14 18:12 ` Alex Williamson @ 2026-09-14 21:36 ` Danilo Krummrich 2026-09-15 18:01 ` Alex Williamson 0 siblings, 1 reply; 24+ messages in thread From: Danilo Krummrich @ 2026-09-14 21:36 UTC (permalink / raw) To: Alex Williamson Cc: Jason Gunthorpe, Zhi Wang, acourbot, yishaih, skolothumtho, kevin.tian, airlied, simona, ojeda, alex.gaynor, boqun.feng, gary, bjorn3_gh, lossin, a.hindborg, aliceryhl, tmgross, jhubbard, ecourtney, cjia, smitra, kjaju, alkumar, ankita, aniketa, kwankhede, targupta, nova-gpu, linux-kernel, zhiwang, kvm 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 *. > 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? ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 12/13] vfio/nvidia-vgpu: add the NVIDIA vGPU VFIO variant driver 2026-09-14 21:36 ` Danilo Krummrich @ 2026-09-15 18:01 ` Alex Williamson 2026-09-15 21:19 ` Danilo Krummrich ` (2 more replies) 0 siblings, 3 replies; 24+ messages in thread From: Alex Williamson @ 2026-09-15 18:01 UTC (permalink / raw) To: Danilo Krummrich Cc: Jason Gunthorpe, Zhi Wang, acourbot, yishaih, skolothumtho, kevin.tian, airlied, simona, ojeda, alex.gaynor, boqun.feng, gary, bjorn3_gh, lossin, a.hindborg, aliceryhl, tmgross, jhubbard, ecourtney, cjia, smitra, kjaju, alkumar, ankita, aniketa, kwankhede, targupta, nova-gpu, linux-kernel, zhiwang, kvm, alex 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. 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. > > 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. Rust may well be the better approach relative to the interface to Nova-core, but it's not clear it's the right approach for a subsystem that can't maintain or evaluate it and only has a sketch of what it looks like. > > 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/ > 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. > > 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. > 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. > 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. > > 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, Alex ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 12/13] vfio/nvidia-vgpu: add the NVIDIA vGPU VFIO variant driver 2026-09-15 18:01 ` Alex Williamson @ 2026-09-15 21:19 ` Danilo Krummrich 2026-09-15 22:55 ` Zhi Wang 2026-09-16 14:17 ` Jason Gunthorpe 2026-09-15 23:44 ` Dave Airlie 2026-09-17 7:12 ` Gary Guo 2 siblings, 2 replies; 24+ messages in thread From: Danilo Krummrich @ 2026-09-15 21:19 UTC (permalink / raw) To: Alex Williamson Cc: Jason Gunthorpe, Zhi Wang, acourbot, yishaih, skolothumtho, kevin.tian, airlied, simona, ojeda, alex.gaynor, boqun.feng, gary, bjorn3_gh, lossin, a.hindborg, aliceryhl, tmgross, jhubbard, ecourtney, cjia, smitra, kjaju, alkumar, ankita, aniketa, kwankhede, targupta, nova-gpu, linux-kernel, zhiwang, kvm 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? ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 12/13] vfio/nvidia-vgpu: add the NVIDIA vGPU VFIO variant driver 2026-09-15 21:19 ` Danilo Krummrich @ 2026-09-15 22:55 ` Zhi Wang 2026-09-16 14:17 ` Jason Gunthorpe 1 sibling, 0 replies; 24+ messages in thread From: Zhi Wang @ 2026-09-15 22:55 UTC (permalink / raw) To: Danilo Krummrich Cc: Alex Williamson, Jason Gunthorpe, acourbot, yishaih, skolothumtho, kevin.tian, airlied, simona, ojeda, alex.gaynor, boqun.feng, gary, bjorn3_gh, lossin, a.hindborg, aliceryhl, tmgross, jhubbard, ecourtney, cjia, smitra, kjaju, alkumar, ankita, aniketa, kwankhede, targupta, nova-gpu, linux-kernel, zhiwang, kvm On Tue, 15 Sep 2026 23:19:01 +0200 "Danilo Krummrich" <dakr@kernel.org> wrote: Hi folks: Since my name has been mentioned several times in this loop, It would be better I chime in besides re-spin my patches. Danilo, thanks for putting the PoC together. I am interested in working it thorough and some parts of idea has already been in my rust SR-IOV rework patches [1]. I am already contributing to nova-core and working on the vGPU manager and VFIO driver, so I can help with nova-core integration and testing. Alex, I take your concerns about review and ongoing maintenance seriously. I'm happy to help evaluate the PoC while those questions are being discussed and commit on the parts of what you think are maintainable and reasonable. For now, I've updated the existing C series to use the new SR-IOV support and incorporate your review feedback [2]. I'll keep following up on the remaining comments. [1] https://lore.kernel.org/all/20260915205659.76841-1-zhiw@nvidia.com/ [2] https://lore.kernel.org/all/20260915211811.84790-1-zhiw@nvidia.com/ > 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? ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 12/13] vfio/nvidia-vgpu: add the NVIDIA vGPU VFIO variant driver 2026-09-15 21:19 ` Danilo Krummrich 2026-09-15 22:55 ` Zhi Wang @ 2026-09-16 14:17 ` Jason Gunthorpe 2026-09-16 15:38 ` Danilo Krummrich 1 sibling, 1 reply; 24+ messages in thread From: Jason Gunthorpe @ 2026-09-16 14:17 UTC (permalink / raw) To: Danilo Krummrich Cc: Alex Williamson, Zhi Wang, acourbot, yishaih, skolothumtho, kevin.tian, airlied, simona, ojeda, alex.gaynor, boqun.feng, gary, bjorn3_gh, lossin, a.hindborg, aliceryhl, tmgross, jhubbard, ecourtney, cjia, smitra, kjaju, alkumar, ankita, aniketa, kwankhede, targupta, nova-gpu, linux-kernel, zhiwang, kvm On Tue, Sep 15, 2026 at 11:19:01PM +0200, Danilo Krummrich wrote: > 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. There are places in the kernel where the drvdata of the bound device ends up owned by the subsystem, not the end driver. Yes, an ideal driver + subsystem should never even need drvdatab beyond remove. Yet, things are not perfect.. The fundamental issue is some kernel API surfaces that the subystem needs to work with only provide a struct device in their callbacks and the subystem has no option but to use the drvdata for its own purpose to recover the subsystem specific data. For example VFIO hooks into this nasty API: ret = vga_client_register(pdev, vfio_pci_set_decode); if (ret) return ret; Which doesn't provide a void * token to pass the core code's struct. Another is all the PCI callbacks which assume the op behind them uses drvdata to get its data. It is not necessarily easy to fix. Rrouting all those PCI callbacks through trampolines in every single driver is really not an appealing design. There are alot of VFIO drivers. Maybe it needs a dev->drvdata and dev->subsystem_data, maybe it needs some PCI thing where the pm ops can get a void *, IDK. This is not some philosophical thing about busses or classes, it is just an accommodation for the way the kernel is now. Fix the above and you can get rid of it. > > 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. :) There are lots of other vfio patches that need attention too, and it seems we are short of that more than anything. Now you need to do a bunch of C refactoring patches as well just to get things ready to show a bunch of rust code. It is a lot of work. I don't really understand in a nutshell why we should do this for nova the mails were so long... Can we not just ignore the lifetime imperfection for this? > 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? nvidia gpu is a more unique scenario, if you are building a general bindings it has to support the flows like this. I've wanted to rework the way the common ops are shimmed in for a while, you'd probably want to do that before rust bindings. Jason ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 12/13] vfio/nvidia-vgpu: add the NVIDIA vGPU VFIO variant driver 2026-09-16 14:17 ` Jason Gunthorpe @ 2026-09-16 15:38 ` Danilo Krummrich 2026-09-16 16:28 ` Jason Gunthorpe 0 siblings, 1 reply; 24+ messages in thread From: Danilo Krummrich @ 2026-09-16 15:38 UTC (permalink / raw) To: Jason Gunthorpe Cc: Alex Williamson, Zhi Wang, acourbot, yishaih, skolothumtho, kevin.tian, airlied, simona, ojeda, alex.gaynor, boqun.feng, gary, bjorn3_gh, lossin, a.hindborg, aliceryhl, tmgross, jhubbard, ecourtney, cjia, smitra, kjaju, alkumar, ankita, aniketa, kwankhede, targupta, nova-gpu, linux-kernel, zhiwang, kvm On Wed Sep 16, 2026 at 4:17 PM CEST, Jason Gunthorpe wrote: > On Tue, Sep 15, 2026 at 11:19:01PM +0200, Danilo Krummrich wrote: > >> 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. > > There are places in the kernel where the drvdata of the bound device > ends up owned by the subsystem, not the end driver. Yes, an ideal > driver + subsystem should never even need drvdatab beyond remove. Yet, > things are not perfect.. > > The fundamental issue is some kernel API surfaces that the subystem > needs to work with only provide a struct device in their callbacks and > the subystem has no option but to use the drvdata for its own purpose > to recover the subsystem specific data. > > For example VFIO hooks into this nasty API: > > ret = vga_client_register(pdev, vfio_pci_set_decode); > if (ret) > return ret; > > Which doesn't provide a void * token to pass the core code's > struct. > > Another is all the PCI callbacks which assume the op behind them uses > drvdata to get its data. This is where the class (i.e. vfio-pci) should instead take a driver callback, as only the driver really knowns about the layering details. Usually, class device implementations can't make assumptions of the underlying bus, because they have to work for any bus. I.e. there's no other way than providing helpers and letting drivers do the glue code between the bus and the class device. > It is not necessarily easy to fix. Rrouting all those PCI callbacks > through trampolines in every single driver is really not an appealing > design. There are alot of VFIO drivers. The reason this seems undesirable from a vfio-pci perspective is that it is special in the sense that it is a class device that is specifically built to sit on top of a spcific bus device (i.e. struct pci_dev). Since this is a rare (maybe even unique?) edge case, the driver core has indeed no infrastructure in place to represent this. > Maybe it needs a dev->drvdata and dev->subsystem_data, maybe it needs > some PCI thing where the pm ops can get a void *, IDK. It still makes me think that there should be some closer integration of vfio-pci with the PCI core, as it is specifically built for this bus. In fact, what you say above is the generalization of my "hack" [1], which, the more I think about this, seems actually less of a hack. :) I would object a bit to the generalization with dev->subsystem_data as it screams for abuse, but the thing in [1] seems more and more reasonable to me. (Another option would be to really split it up and have a virtual vfio-pci bus on top of PCI, which would also allow for custom match logic, but that also seems pretty overkill.) >> As mentoined above, there are people volunteering now. Without starting it, it >> can't scale further than that. :) > > There are lots of other vfio patches that need attention too, and it > seems we are short of that more than anything. Now you need to do a > bunch of C refactoring patches as well just to get things ready to > show a bunch of rust code. It is a lot of work. It's only the drvdata thing, what else is missing? > I don't really understand in a nutshell why we should do this for nova > the mails were so long... Can we not just ignore the lifetime > imperfection for this? I mentioned some points in the first two paragraphs of [2]. Besides that, I don't see a reason why we should spend time and effort for working out the inferior solution, where the better alternative is even less effort, contributes to better quality and stability of the whole driver project and also offers a chance for the vfio subsystem to gain new contributors and gather experience with the language that has proven itself in many areas already. I mean, there'd still be the option to make it an experiment and say let's add the abstractions and the nvidia-vgpu driver and see how it works out for a while before allowing more Rust drivers. And if it really turns out to be bad, it should also be easy to rip it out, replace nvidia-vgpu with a C driver and throw in a crappy FFI layer. :) >> 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? > > nvidia gpu is a more unique scenario, if you are building a general > bindings it has to support the flows like this. Sure, as mentioned, the code I sketched up should already be able to do this. > I've wanted to rework the way the common ops are shimmed in for a > while, you'd probably want to do that before rust bindings. TBH, I don't think it makes a difference; having Rust abstractions is pretty much the same as having another driver. I.e. it would be equivalent to saying "before we accept another pci-vfio driver we need to do some rework". Actually, I even think it can be an advantage, as you could think of the Rust abstractions like a driver with built-in correctness checks, so it can help to validate the changes. Of course, this requires the responsibles of the Rust code to help with that and I think we have this commitment. [1] https://git.kernel.org/pub/scm/linux/kernel/git/dakr/linux.git/commit/?id=76b3bfd6386a01f338780e1a37b5bad3f5a48d31 [2] https://lore.kernel.org/nova-gpu/DLCRZLO06SIO.LS7TWQXIPZSQ@kernel.org/ ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 12/13] vfio/nvidia-vgpu: add the NVIDIA vGPU VFIO variant driver 2026-09-16 15:38 ` Danilo Krummrich @ 2026-09-16 16:28 ` Jason Gunthorpe 2026-09-16 18:02 ` Danilo Krummrich 2026-09-17 1:49 ` Dave Airlie 0 siblings, 2 replies; 24+ messages in thread From: Jason Gunthorpe @ 2026-09-16 16:28 UTC (permalink / raw) To: Danilo Krummrich Cc: Alex Williamson, Zhi Wang, acourbot, yishaih, skolothumtho, kevin.tian, airlied, simona, ojeda, alex.gaynor, boqun.feng, gary, bjorn3_gh, lossin, a.hindborg, aliceryhl, tmgross, jhubbard, ecourtney, cjia, smitra, kjaju, alkumar, ankita, aniketa, kwankhede, targupta, nova-gpu, linux-kernel, zhiwang, kvm On Wed, Sep 16, 2026 at 05:38:21PM +0200, Danilo Krummrich wrote: > Usually, class device implementations can't make assumptions of the underlying > bus, because they have to work for any bus. I.e. there's no other way than > providing helpers and letting drivers do the glue code between the bus and the > class device. You should think of VFIO as a series of helper libraries. One of those libraries is "here are all the PCI PM ops you need". Drivers rarely need to customize these ops, they just need to wire it up to the support library to avoid a bunch of code duplication. Stated another way - the very point of vfio-pci is to remove duplicated code from the drivers. So if we wanted to push hard on removing drvdata, and don't want to touch the struct device I would probably say to #define up a way for the driver to build its unique trampolines. It wastes a bunch of .text but at least it doesn't duplicate code. > The reason this seems undesirable from a vfio-pci perspective is that it is > special in the sense that it is a class device that is specifically built to sit > on top of a spcific bus device (i.e. struct pci_dev). It is a library, all these ideas to do things with the driver core to implement a library make no architectural sense. > It still makes me think that there should be some closer integration of vfio-pci > with the PCI core, as it is specifically built for this bus. It has such a basic need I don't see this as a reason to pollute pci core with any vfio specific things. Like I would nak your [1], that's completely wrong layering. > > I don't really understand in a nutshell why we should do this for nova > > the mails were so long... Can we not just ignore the lifetime > > imperfection for this? > > I mentioned some points in the first two paragraphs of [2]. Besides that, I > don't see a reason why we should spend time and effort for working out the > inferior solution, where the better alternative is even less effort, contributes > to better quality and stability of the whole driver project and also offers a > chance for the vfio subsystem to gain new contributors and gather experience > with the language that has proven itself in many areas already. It seems to be quite a leap that it is less effort. IDK.. > TBH, I don't think it makes a difference; having Rust abstractions is pretty > much the same as having another driver. I.e. it would be equivalent to saying > "before we accept another pci-vfio driver we need to do some rework". Well, it is, but thats the point when judging effort.. Jason ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 12/13] vfio/nvidia-vgpu: add the NVIDIA vGPU VFIO variant driver 2026-09-16 16:28 ` Jason Gunthorpe @ 2026-09-16 18:02 ` Danilo Krummrich 2026-09-17 13:28 ` Jason Gunthorpe 2026-09-17 1:49 ` Dave Airlie 1 sibling, 1 reply; 24+ messages in thread From: Danilo Krummrich @ 2026-09-16 18:02 UTC (permalink / raw) To: Jason Gunthorpe Cc: Alex Williamson, Zhi Wang, acourbot, yishaih, skolothumtho, kevin.tian, airlied, simona, ojeda, alex.gaynor, boqun.feng, gary, bjorn3_gh, lossin, a.hindborg, aliceryhl, tmgross, jhubbard, ecourtney, cjia, smitra, kjaju, alkumar, ankita, aniketa, kwankhede, targupta, nova-gpu, linux-kernel, zhiwang, kvm On Wed Sep 16, 2026 at 6:28 PM CEST, Jason Gunthorpe wrote: > On Wed, Sep 16, 2026 at 05:38:21PM +0200, Danilo Krummrich wrote: >> Usually, class device implementations can't make assumptions of the underlying >> bus, because they have to work for any bus. I.e. there's no other way than >> providing helpers and letting drivers do the glue code between the bus and the >> class device. > > You should think of VFIO as a series of helper libraries. One of those > libraries is "here are all the PCI PM ops you need". > > Drivers rarely need to customize these ops, they just need to wire it > up to the support library to avoid a bunch of code duplication. > > Stated another way - the very point of vfio-pci is to remove > duplicated code from the drivers. > > So if we wanted to push hard on removing drvdata, and don't want to > touch the struct device I would probably say to #define up a way for > the driver to build its unique trampolines. It wastes a bunch of .text > but at least it doesn't duplicate code. > >> The reason this seems undesirable from a vfio-pci perspective is that it is >> special in the sense that it is a class device that is specifically built to sit >> on top of a spcific bus device (i.e. struct pci_dev). > > It is a library, all these ideas to do things with the driver core to > implement a library make no architectural sense. Both is true, it is a library, and it is also a class device built on top of another class device (struct vfio_device) that accomodates to a specific underlying bus (PCI). Which means that this library has the need to make a generic connection between the class (VFIO) and the bus (PCI). And this is something that the kernel has no generic solution for (and isn't represented by the driver core in any way). Your dev->subsystem_data idea would accomodate this connection. However, I object to this, as it'd be for a very special case and I'd be worried it is abused by other subsystems in odd ways. Besides that, I still think that the correct thing to do is to have the glue in the driver and just provide the helpers in the best possible form. In fact, this is what a library should do - provide the helpers, but do not directly interfere with with other layers. >> It still makes me think that there should be some closer integration of vfio-pci >> with the PCI core, as it is specifically built for this bus. > > It has such a basic need I don't see this as a reason to pollute pci > core with any vfio specific things. Like I would nak your [1], that's > completely wrong layering. As I said in the beginning, it is just a "hack"; and in fact it is the exact same layering violation as abusing dev->driver_data or adding dev->subsystem_data. The reasons I called it "more reasonable" and "less of a hack" in my previous reply are that it doesn't break an existing (driver core) API contract and it only affects the the exact two layers this is about in the first place. >> > I don't really understand in a nutshell why we should do this for nova >> > the mails were so long... Can we not just ignore the lifetime >> > imperfection for this? >> >> I mentioned some points in the first two paragraphs of [2]. Besides that, I >> don't see a reason why we should spend time and effort for working out the >> inferior solution, where the better alternative is even less effort, contributes >> to better quality and stability of the whole driver project and also offers a >> chance for the vfio subsystem to gain new contributors and gather experience >> with the language that has proven itself in many areas already. > > It seems to be quite a leap that it is less effort. IDK.. > >> TBH, I don't think it makes a difference; having Rust abstractions is pretty >> much the same as having another driver. I.e. it would be equivalent to saying >> "before we accept another pci-vfio driver we need to do some rework". > > Well, it is, but thats the point when judging effort.. So it seems that we agree that there isn't really a difference between adding a new C or Rust driver in this regard. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 12/13] vfio/nvidia-vgpu: add the NVIDIA vGPU VFIO variant driver 2026-09-16 18:02 ` Danilo Krummrich @ 2026-09-17 13:28 ` Jason Gunthorpe 2026-09-17 14:50 ` Danilo Krummrich 0 siblings, 1 reply; 24+ messages in thread From: Jason Gunthorpe @ 2026-09-17 13:28 UTC (permalink / raw) To: Danilo Krummrich Cc: Alex Williamson, Zhi Wang, acourbot, yishaih, skolothumtho, kevin.tian, airlied, simona, ojeda, alex.gaynor, boqun.feng, gary, bjorn3_gh, lossin, a.hindborg, aliceryhl, tmgross, jhubbard, ecourtney, cjia, smitra, kjaju, alkumar, ankita, aniketa, kwankhede, targupta, nova-gpu, linux-kernel, zhiwang, kvm On Wed, Sep 16, 2026 at 08:02:17PM +0200, Danilo Krummrich wrote: > >> TBH, I don't think it makes a difference; having Rust abstractions is pretty > >> much the same as having another driver. I.e. it would be equivalent to saying > >> "before we accept another pci-vfio driver we need to do some rework". > > > > Well, it is, but thats the point when judging effort.. > > So it seems that we agree that there isn't really a difference between adding a > new C or Rust driver in this regard. To be clear I was trying to say I think you are are underestimating how much work this is and how long it will take. Including all the preconditon C reworks has to be included when considering how much work is involved. Why can't we just go forward with what Zhi already drafted? Do we really need another side quest? I cannot forsee rust bindings for vfio until sometime in 2027. Jason ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 12/13] vfio/nvidia-vgpu: add the NVIDIA vGPU VFIO variant driver 2026-09-17 13:28 ` Jason Gunthorpe @ 2026-09-17 14:50 ` Danilo Krummrich 2026-09-17 17:10 ` Jason Gunthorpe 0 siblings, 1 reply; 24+ messages in thread From: Danilo Krummrich @ 2026-09-17 14:50 UTC (permalink / raw) To: Jason Gunthorpe Cc: Alex Williamson, Zhi Wang, acourbot, yishaih, skolothumtho, kevin.tian, airlied, simona, ojeda, alex.gaynor, boqun.feng, gary, bjorn3_gh, lossin, a.hindborg, aliceryhl, tmgross, jhubbard, ecourtney, cjia, smitra, kjaju, alkumar, ankita, aniketa, kwankhede, targupta, nova-gpu, linux-kernel, zhiwang, kvm On Thu Sep 17, 2026 at 3:28 PM CEST, Jason Gunthorpe wrote: > On Wed, Sep 16, 2026 at 08:02:17PM +0200, Danilo Krummrich wrote: >> >> TBH, I don't think it makes a difference; having Rust abstractions is pretty >> >> much the same as having another driver. I.e. it would be equivalent to saying >> >> "before we accept another pci-vfio driver we need to do some rework". >> > >> > Well, it is, but thats the point when judging effort.. >> >> So it seems that we agree that there isn't really a difference between adding a >> new C or Rust driver in this regard. > > To be clear I was trying to say I think you are are underestimating > how much work this is and how long it will take. Including all the > preconditon C reworks has to be included when considering how much > work is involved. I'm not sure I follow what you mean by "preconditon C reworks". Do you refer to the drvdata thing? That seems rather trivial to address? Or do you refer to other C core reworks people are working on? If so, they are only relevant if they affect the drivers. And if they affect the drivers, there's not really a difference between adding a new C driver and adding a Rust abstraction. > Why can't we just go forward with what Zhi already drafted? Do we > really need another side quest? Because I don't think it is a side quest. As mentioned, the FFI layer is already more complicated than the Rust abstraction we need, and I also don't see that this work is progressed further than the Rust abstraction I drafted. So, why not just go for the proper solution right away? I still have to finish some talks, but I could probably finish that work until LPC and then we can walk through it? > I cannot forsee rust bindings for vfio until sometime in 2027. What is different in 2027 than is now? ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 12/13] vfio/nvidia-vgpu: add the NVIDIA vGPU VFIO variant driver 2026-09-17 14:50 ` Danilo Krummrich @ 2026-09-17 17:10 ` Jason Gunthorpe 2026-09-17 20:09 ` Danilo Krummrich 0 siblings, 1 reply; 24+ messages in thread From: Jason Gunthorpe @ 2026-09-17 17:10 UTC (permalink / raw) To: Danilo Krummrich Cc: Alex Williamson, Zhi Wang, acourbot, yishaih, skolothumtho, kevin.tian, airlied, simona, ojeda, alex.gaynor, boqun.feng, gary, bjorn3_gh, lossin, a.hindborg, aliceryhl, tmgross, jhubbard, ecourtney, cjia, smitra, kjaju, alkumar, ankita, aniketa, kwankhede, targupta, nova-gpu, linux-kernel, zhiwang, kvm On Thu, Sep 17, 2026 at 04:50:07PM +0200, Danilo Krummrich wrote: > On Thu Sep 17, 2026 at 3:28 PM CEST, Jason Gunthorpe wrote: > > On Wed, Sep 16, 2026 at 08:02:17PM +0200, Danilo Krummrich wrote: > >> >> TBH, I don't think it makes a difference; having Rust abstractions is pretty > >> >> much the same as having another driver. I.e. it would be equivalent to saying > >> >> "before we accept another pci-vfio driver we need to do some rework". > >> > > >> > Well, it is, but thats the point when judging effort.. > >> > >> So it seems that we agree that there isn't really a difference between adding a > >> new C or Rust driver in this regard. > > > > To be clear I was trying to say I think you are are underestimating > > how much work this is and how long it will take. Including all the > > preconditon C reworks has to be included when considering how much > > work is involved. > > I'm not sure I follow what you mean by "preconditon C reworks". Rust bindings often seem to need C code rework for Rust, drvdata here is an example. > Or do you refer to other C core reworks people are working on? If so, they are > only relevant if they affect the drivers. And if they affect the drivers, > there's not really a difference between adding a new C driver and adding a Rust > abstraction. A new C driver should not need reworks? > > I cannot forsee rust bindings for vfio until sometime in 2027. > > What is different in 2027 than is now? I mean I think that is how long it will take to merge something like that, given how many other vfio series there are, the lack of expertise, and some general expectations. There are already lots of series for vfio that are in front of something like this in the queue. And then we end up with what will be one of the most complex drivers in a form nobody familiar with vfio can review. It just seems like a much bigger thing to me than whatever the ffi problem is.. Jason ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 12/13] vfio/nvidia-vgpu: add the NVIDIA vGPU VFIO variant driver 2026-09-17 17:10 ` Jason Gunthorpe @ 2026-09-17 20:09 ` Danilo Krummrich 2026-09-23 20:05 ` Alex Williamson 0 siblings, 1 reply; 24+ messages in thread From: Danilo Krummrich @ 2026-09-17 20:09 UTC (permalink / raw) To: Jason Gunthorpe Cc: Alex Williamson, Zhi Wang, acourbot, yishaih, skolothumtho, kevin.tian, airlied, simona, ojeda, alex.gaynor, boqun.feng, gary, bjorn3_gh, lossin, a.hindborg, aliceryhl, tmgross, jhubbard, ecourtney, cjia, smitra, kjaju, alkumar, ankita, aniketa, kwankhede, targupta, nova-gpu, linux-kernel, zhiwang, kvm On Thu Sep 17, 2026 at 7:10 PM CEST, Jason Gunthorpe wrote: > On Thu, Sep 17, 2026 at 04:50:07PM +0200, Danilo Krummrich wrote: >> On Thu Sep 17, 2026 at 3:28 PM CEST, Jason Gunthorpe wrote: >> > On Wed, Sep 16, 2026 at 08:02:17PM +0200, Danilo Krummrich wrote: >> >> >> TBH, I don't think it makes a difference; having Rust abstractions is pretty >> >> >> much the same as having another driver. I.e. it would be equivalent to saying >> >> >> "before we accept another pci-vfio driver we need to do some rework". >> >> > >> >> > Well, it is, but thats the point when judging effort.. >> >> >> >> So it seems that we agree that there isn't really a difference between adding a >> >> new C or Rust driver in this regard. >> > >> > To be clear I was trying to say I think you are are underestimating >> > how much work this is and how long it will take. Including all the >> > preconditon C reworks has to be included when considering how much >> > work is involved. >> >> I'm not sure I follow what you mean by "preconditon C reworks". > > Rust bindings often seem to need C code rework for Rust, drvdata here > is an example. I can assure you we can set this concern aside. The driver core code, auxiliary, platform, PCI, DMA, scatterlist, DRM, firmware loader, IRQ, I/O and fwctl, to name just a few, did not need any changes. And from the top of my head I can't think of any case where changes were needed. For VFIO I already wrote the code, and no changes were required either, except for the drvdata things of course. But, as discussed, this is an existing layering violation. The Rust driver code code simply relies on its own subsystem's API contract. Please let's not frame this as a Rust issue. Plus, it is trivial to address, so it luckily is not a huge problem anyway. :) >> Or do you refer to other C core reworks people are working on? If so, they are >> only relevant if they affect the drivers. And if they affect the drivers, >> there's not really a difference between adding a new C driver and adding a Rust >> abstraction. > > A new C driver should not need reworks? If a new C driver is not affected the Rust abstraction won't be affected either. They are just users of the same APIs the C drivers consume. So, that doesn't seem to be a concern then. >> > I cannot forsee rust bindings for vfio until sometime in 2027. >> >> What is different in 2027 than is now? > > I mean I think that is how long it will take to merge something like > that, given how many other vfio series there are, the lack of > expertise, and some general expectations. There are already lots of > series for vfio that are in front of something like this in the queue. The same would then be true for an nvidia-vgpu driver written in C, no? Again, the Rust abstractions are nothing else than a driver in the end. They only consume the driver facing APIs and translate them to a safe Rust API. > And then we end up with what will be one of the most complex drivers > in a form nobody familiar with vfio can review. I think this isn't the case; the people you refer to can still review semantics and vfio design specifics. It's not like everything is different. For instance, there's still a get_region_info() callback on vfio::pci::Operations where people can review whether this semantically makes sense. And for the language specifics we have a lot of Rust familiar people around who will review the code and help to improve it. Why not see this as a chance to scale on both ends? More people on both ends will get familiar with VFIO and Rust at the same time. Thanks, Danilo ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 12/13] vfio/nvidia-vgpu: add the NVIDIA vGPU VFIO variant driver 2026-09-17 20:09 ` Danilo Krummrich @ 2026-09-23 20:05 ` Alex Williamson 0 siblings, 0 replies; 24+ messages in thread From: Alex Williamson @ 2026-09-23 20:05 UTC (permalink / raw) To: Danilo Krummrich Cc: Jason Gunthorpe, Zhi Wang, acourbot, yishaih, skolothumtho, kevin.tian, airlied, simona, ojeda, alex.gaynor, boqun.feng, gary, bjorn3_gh, lossin, a.hindborg, aliceryhl, tmgross, jhubbard, ecourtney, cjia, smitra, kjaju, alkumar, ankita, aniketa, kwankhede, targupta, nova-gpu, linux-kernel, zhiwang, kvm, alex, Simon Song (SW-GPU) On Thu, 17 Sep 2026 22:09:58 +0200 "Danilo Krummrich" <dakr@kernel.org> wrote: > On Thu Sep 17, 2026 at 7:10 PM CEST, Jason Gunthorpe wrote: > > On Thu, Sep 17, 2026 at 04:50:07PM +0200, Danilo Krummrich wrote: > >> On Thu Sep 17, 2026 at 3:28 PM CEST, Jason Gunthorpe wrote: > >> > On Wed, Sep 16, 2026 at 08:02:17PM +0200, Danilo Krummrich wrote: > >> >> >> TBH, I don't think it makes a difference; having Rust abstractions is pretty > >> >> >> much the same as having another driver. I.e. it would be equivalent to saying > >> >> >> "before we accept another pci-vfio driver we need to do some rework". > >> >> > > >> >> > Well, it is, but thats the point when judging effort.. > >> >> > >> >> So it seems that we agree that there isn't really a difference between adding a > >> >> new C or Rust driver in this regard. > >> > > >> > To be clear I was trying to say I think you are are underestimating > >> > how much work this is and how long it will take. Including all the > >> > preconditon C reworks has to be included when considering how much > >> > work is involved. > >> > >> I'm not sure I follow what you mean by "preconditon C reworks". > > > > Rust bindings often seem to need C code rework for Rust, drvdata here > > is an example. > > I can assure you we can set this concern aside. The driver core code, auxiliary, > platform, PCI, DMA, scatterlist, DRM, firmware loader, IRQ, I/O and fwctl, to > name just a few, did not need any changes. And from the top of my head I can't > think of any case where changes were needed. > > For VFIO I already wrote the code, and no changes were required either, except > for the drvdata things of course. > > But, as discussed, this is an existing layering violation. The Rust driver code > code simply relies on its own subsystem's API contract. Please let's not frame > this as a Rust issue. > > Plus, it is trivial to address, so it luckily is not a huge problem anyway. :) > > >> Or do you refer to other C core reworks people are working on? If so, they are > >> only relevant if they affect the drivers. And if they affect the drivers, > >> there's not really a difference between adding a new C driver and adding a Rust > >> abstraction. > > > > A new C driver should not need reworks? > > If a new C driver is not affected the Rust abstraction won't be affected either. > They are just users of the same APIs the C drivers consume. So, that doesn't > seem to be a concern then. > > >> > I cannot forsee rust bindings for vfio until sometime in 2027. > >> > >> What is different in 2027 than is now? > > > > I mean I think that is how long it will take to merge something like > > that, given how many other vfio series there are, the lack of > > expertise, and some general expectations. There are already lots of > > series for vfio that are in front of something like this in the queue. > > The same would then be true for an nvidia-vgpu driver written in C, no? Again, > the Rust abstractions are nothing else than a driver in the end. They only > consume the driver facing APIs and translate them to a safe Rust API. > > > And then we end up with what will be one of the most complex drivers > > in a form nobody familiar with vfio can review. > > I think this isn't the case; the people you refer to can still review semantics > and vfio design specifics. It's not like everything is different. > > For instance, there's still a get_region_info() callback on > vfio::pci::Operations where people can review whether this semantically makes > sense. > > And for the language specifics we have a lot of Rust familiar people around who > will review the code and help to improve it. > > Why not see this as a chance to scale on both ends? More people on both ends > will get familiar with VFIO and Rust at the same time. This thread has gone cold for a few days and nothing really seems to be dissuading from the direction of implementing the Nova interfacing vfio-pci variant vGPU driver in Rust. Zhi has been investigating the gaps and bugs in the PoC, and afaik, nothing stands out as a blocker. I'll do my best to use LLM assisted reviews on my part, but the expectation should be that I won't be merging any Rust code without formal reviews on list. Over time I hope to be able to contribute from that perspective, but the Rust community is going to need to hold their end of the bargain as well. If meaningful reviews are not provided, the driver can be orphaned and removed. The Rust community also needs to be proactive in supporting the vfio community such that abstractions and interfaces between C and Rust don't become a barrier for C-based development. Keeping the Rust code working when the C code changes is the Rust community's job, we shouldn't expect more than "best effort" from the existing C-focused community. I also want to be clear, I'm not endorsing Rust as a pattern anywhere beyond this individual vfio-pci variant driver. Any other proposals would need to bring a comparably sized community to sponsor them. Dave's approach to make vfio-pci-core a stateless helper with respect to drvdata, with a dash of Jason's macro idea to minimize the driver boilerplate for the common cases, seems like the right direction to remove that convention. I've handed off my own LLM generated version that does this and it should be refined and posted soon. So, unless there's an aspect we haven't considered, it seems the Rust variant driver should be the direction here. Thanks, Alex ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 12/13] vfio/nvidia-vgpu: add the NVIDIA vGPU VFIO variant driver 2026-09-16 16:28 ` Jason Gunthorpe 2026-09-16 18:02 ` Danilo Krummrich @ 2026-09-17 1:49 ` Dave Airlie 2026-09-17 7:01 ` Gary Guo 2026-09-17 13:20 ` Jason Gunthorpe 1 sibling, 2 replies; 24+ messages in thread From: Dave Airlie @ 2026-09-17 1:49 UTC (permalink / raw) To: Jason Gunthorpe Cc: Danilo Krummrich, Alex Williamson, Zhi Wang, acourbot, yishaih, skolothumtho, kevin.tian, simona, ojeda, alex.gaynor, boqun.feng, gary, bjorn3_gh, lossin, a.hindborg, aliceryhl, tmgross, jhubbard, ecourtney, cjia, smitra, kjaju, alkumar, ankita, aniketa, kwankhede, targupta, nova-gpu, linux-kernel, zhiwang, kvm On Thu, 17 Sept 2026 at 02:28, Jason Gunthorpe <jgg@nvidia.com> wrote: > > On Wed, Sep 16, 2026 at 05:38:21PM +0200, Danilo Krummrich wrote: > > Usually, class device implementations can't make assumptions of the underlying > > bus, because they have to work for any bus. I.e. there's no other way than > > providing helpers and letting drivers do the glue code between the bus and the > > class device. > > You should think of VFIO as a series of helper libraries. One of those > libraries is "here are all the PCI PM ops you need". > > Drivers rarely need to customize these ops, they just need to wire it > up to the support library to avoid a bunch of code duplication. It's currently a library being used as a midlayer. demidlayering it so drivers control drvdata removes that. I sent the diffstat that covers all the current vfio pci drivers and that didn't seem excessive. > > Stated another way - the very point of vfio-pci is to remove > duplicated code from the drivers. > > So if we wanted to push hard on removing drvdata, and don't want to > touch the struct device I would probably say to #define up a way for > the driver to build its unique trampolines. It wastes a bunch of .text > but at least it doesn't duplicate code. +static pci_ers_result_t hisi_acc_vfio_pci_aer_err_detected( + struct pci_dev *pdev, pci_channel_state_t state) +{ + struct hisi_acc_vf_core_device *hisi_acc_vdev = hisi_acc_drvdata(pdev); + + return vfio_pci_core_aer_err_detected(&hisi_acc_vdev->core_device, state); +} + +static int hisi_acc_vfio_pci_runtime_suspend(struct device *dev) +{ + struct hisi_acc_vf_core_device *hisi_acc_vdev = dev_get_drvdata(dev); + + return vfio_pci_core_runtime_suspend(&hisi_acc_vdev->core_device); +} + +static int hisi_acc_vfio_pci_runtime_resume(struct device *dev) +{ + struct hisi_acc_vf_core_device *hisi_acc_vdev = dev_get_drvdata(dev); + + return vfio_pci_core_runtime_resume(&hisi_acc_vdev->core_device); +} + +static const struct dev_pm_ops hisi_acc_vfio_pci_pm_ops = { + SET_RUNTIME_PM_OPS(hisi_acc_vfio_pci_runtime_suspend, + hisi_acc_vfio_pci_runtime_resume, NULL) +}; is how much code it adds to current drivers, this doesn't seem excessive for 10 drivers, like you could obfuscate it a bit with some macros, but I really don't see a lot of value in hiding what is effectively just standard driver boilerplate. Dave. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 12/13] vfio/nvidia-vgpu: add the NVIDIA vGPU VFIO variant driver 2026-09-17 1:49 ` Dave Airlie @ 2026-09-17 7:01 ` Gary Guo 2026-09-17 13:20 ` Jason Gunthorpe 1 sibling, 0 replies; 24+ messages in thread From: Gary Guo @ 2026-09-17 7:01 UTC (permalink / raw) To: Dave Airlie, Jason Gunthorpe Cc: Danilo Krummrich, Alex Williamson, Zhi Wang, acourbot, yishaih, skolothumtho, kevin.tian, simona, ojeda, alex.gaynor, boqun.feng, gary, bjorn3_gh, lossin, a.hindborg, aliceryhl, tmgross, jhubbard, ecourtney, cjia, smitra, kjaju, alkumar, ankita, aniketa, kwankhede, targupta, nova-gpu, linux-kernel, zhiwang, kvm On Thu Sep 17, 2026 at 2:49 AM BST, Dave Airlie wrote: > On Thu, 17 Sept 2026 at 02:28, Jason Gunthorpe <jgg@nvidia.com> wrote: >> >> Stated another way - the very point of vfio-pci is to remove >> duplicated code from the drivers. >> >> So if we wanted to push hard on removing drvdata, and don't want to >> touch the struct device I would probably say to #define up a way for >> the driver to build its unique trampolines. It wastes a bunch of .text >> but at least it doesn't duplicate code. > > +static pci_ers_result_t hisi_acc_vfio_pci_aer_err_detected( > + struct pci_dev *pdev, pci_channel_state_t state) > +{ > + struct hisi_acc_vf_core_device *hisi_acc_vdev = hisi_acc_drvdata(pdev); > + > + return vfio_pci_core_aer_err_detected(&hisi_acc_vdev->core_device, > state); > +} > + > +static int hisi_acc_vfio_pci_runtime_suspend(struct device *dev) > +{ > + struct hisi_acc_vf_core_device *hisi_acc_vdev = dev_get_drvdata(dev); > + > + return vfio_pci_core_runtime_suspend(&hisi_acc_vdev->core_device); > +} > + > +static int hisi_acc_vfio_pci_runtime_resume(struct device *dev) > +{ > + struct hisi_acc_vf_core_device *hisi_acc_vdev = dev_get_drvdata(dev); > + > + return vfio_pci_core_runtime_resume(&hisi_acc_vdev->core_device); > +} > + > +static const struct dev_pm_ops hisi_acc_vfio_pci_pm_ops = { > + SET_RUNTIME_PM_OPS(hisi_acc_vfio_pci_runtime_suspend, > + hisi_acc_vfio_pci_runtime_resume, NULL) > +}; > > is how much code it adds to current drivers, this doesn't seem > excessive for 10 drivers, > like you could obfuscate it a bit with some macros, but I really don't > see a lot of value in hiding > what is effectively just standard driver boilerplate. If it's just these callbacks, you could even have vfio-pci directly proxying these callbacks by having a `struct device` -> `struct vfio_pci_core_device` hashmap. (Not that I'm saying it's a good idea). Best, Gary ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 12/13] vfio/nvidia-vgpu: add the NVIDIA vGPU VFIO variant driver 2026-09-17 1:49 ` Dave Airlie 2026-09-17 7:01 ` Gary Guo @ 2026-09-17 13:20 ` Jason Gunthorpe 1 sibling, 0 replies; 24+ messages in thread From: Jason Gunthorpe @ 2026-09-17 13:20 UTC (permalink / raw) To: Dave Airlie Cc: Danilo Krummrich, Alex Williamson, Zhi Wang, acourbot, yishaih, skolothumtho, kevin.tian, simona, ojeda, alex.gaynor, boqun.feng, gary, bjorn3_gh, lossin, a.hindborg, aliceryhl, tmgross, jhubbard, ecourtney, cjia, smitra, kjaju, alkumar, ankita, aniketa, kwankhede, targupta, nova-gpu, linux-kernel, zhiwang, kvm On Thu, Sep 17, 2026 at 11:49:29AM +1000, Dave Airlie wrote: > +static pci_ers_result_t hisi_acc_vfio_pci_aer_err_detected( > + struct pci_dev *pdev, pci_channel_state_t state) > +{ > + struct hisi_acc_vf_core_device *hisi_acc_vdev = hisi_acc_drvdata(pdev); > + > + return vfio_pci_core_aer_err_detected(&hisi_acc_vdev->core_device, > state); > +} > + > +static int hisi_acc_vfio_pci_runtime_suspend(struct device *dev) > +{ > + struct hisi_acc_vf_core_device *hisi_acc_vdev = dev_get_drvdata(dev); > + > + return vfio_pci_core_runtime_suspend(&hisi_acc_vdev->core_device); > +} > + > +static int hisi_acc_vfio_pci_runtime_resume(struct device *dev) > +{ > + struct hisi_acc_vf_core_device *hisi_acc_vdev = dev_get_drvdata(dev); > + > + return vfio_pci_core_runtime_resume(&hisi_acc_vdev->core_device); > +} > + > +static const struct dev_pm_ops hisi_acc_vfio_pci_pm_ops = { > + SET_RUNTIME_PM_OPS(hisi_acc_vfio_pci_runtime_suspend, > + hisi_acc_vfio_pci_runtime_resume, NULL) > +}; > > is how much code it adds to current drivers, this doesn't seem > excessive for 10 drivers, Well, I think it is alot, and there it sucks if new ones are added. > like you could obfuscate it a bit with some macros, but I really don't > see a lot of value in hiding > what is effectively just standard driver boilerplate. This is exactly the kind of boilerplate people have been removing with macros. I would macro it.. Jason ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 12/13] vfio/nvidia-vgpu: add the NVIDIA vGPU VFIO variant driver 2026-09-15 18:01 ` Alex Williamson 2026-09-15 21:19 ` Danilo Krummrich @ 2026-09-15 23:44 ` Dave Airlie 2026-09-16 14:25 ` Jason Gunthorpe 2026-09-17 7:12 ` Gary Guo 2 siblings, 1 reply; 24+ messages in thread From: Dave Airlie @ 2026-09-15 23:44 UTC (permalink / raw) To: Alex Williamson Cc: Danilo Krummrich, Jason Gunthorpe, Zhi Wang, acourbot, yishaih, skolothumtho, kevin.tian, simona, ojeda, alex.gaynor, boqun.feng, gary, bjorn3_gh, lossin, a.hindborg, aliceryhl, tmgross, jhubbard, ecourtney, cjia, smitra, kjaju, alkumar, ankita, aniketa, kwankhede, targupta, nova-gpu, linux-kernel, zhiwang, kvm On Wed, 16 Sept 2026 at 04:01, Alex Williamson <alex@shazbot.org> 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. Not to be critical of an evolved design, vfio-pci-core should probably be structured as a series of helpers that drivers can use, rather than a midlayer. Midlayers plagued us in drm, let drivers drive and provide the VGA/PM interfaces using standard vfio-pci-core methods rather than forcing vfio-pci-core to own the pci device data. Just because C let's us take shortcuts with struct layouts and pointers, it doesn't mean it's a clean pattern we want to continue with indefinitely. You can look at the rust use of the subsystem as a chance to clean up some of the corners where layers aren't clean. I just threw opencode at the refactor and for $12 of my massive budget it produced the prompt was: "in linux/drivers/vfio/pci the vfio pci core decides that drivers must install a vfio_pci_core_device struct to drvdata, this feels like a layering violation, I'd like the vfio pci core to be a set of helpers the drivers use instead of midlayering itself, can you plan that out and then execute" drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c | 54 ++++++++++++++++++++++++++++++++++++++---------------- drivers/vfio/pci/ism/main.c | 45 ++++++++++++++++++++++++++++++++++++--------- drivers/vfio/pci/mlx5/main.c | 39 ++++++++++++++++++++++++++++++++------- drivers/vfio/pci/nvgrace-gpu/main.c | 45 +++++++++++++++++++++++++++++++++++---------- drivers/vfio/pci/pds/pci_drv.c | 34 +++++++++++++++++++++++++++++++--- drivers/vfio/pci/pds/vfio_dev.c | 5 +---- drivers/vfio/pci/qat/main.c | 38 ++++++++++++++++++++++++++++++++------ drivers/vfio/pci/vfio_pci.c | 34 ++++++++++++++++++++++++++++++++-- drivers/vfio/pci/vfio_pci_core.c | 66 +++++++++++++++++++++++------------------------------------------- drivers/vfio/pci/virtio/main.c | 34 +++++++++++++++++++++++++++++++--- drivers/vfio/pci/xe/main.c | 34 +++++++++++++++++++++++++++++++--- 11 files changed, 322 insertions(+), 106 deletions(-) It seems to be correct code, and makes things look more like drivers owning the outer structs. I did look for this pattern elsewhere in the kernel and it's not common at all, in fact my quick search couldn't spot any other instances which again suggests it's not a pattern we should be leaning into. As for the rust stuff, I think for vfio your drivers are just not that much of a horror show or refactoring often enough that rust will be anything but a blip on your radar, having Zhi/Danilo and access to any sort of LLM will generally make most refactors trivial, lots of maintainers keep bringing up the same concerns about refactors and interfaces and breakage, but I'm not seeing the problems, and most of the refactors that are rust driven are make the C code clearer. Dave. > > > > 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. > > > 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. > > > 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. > > > > 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, > > Alex ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 12/13] vfio/nvidia-vgpu: add the NVIDIA vGPU VFIO variant driver 2026-09-15 23:44 ` Dave Airlie @ 2026-09-16 14:25 ` Jason Gunthorpe 0 siblings, 0 replies; 24+ messages in thread From: Jason Gunthorpe @ 2026-09-16 14:25 UTC (permalink / raw) To: Dave Airlie Cc: Alex Williamson, Danilo Krummrich, Zhi Wang, acourbot, yishaih, skolothumtho, kevin.tian, simona, ojeda, alex.gaynor, boqun.feng, gary, bjorn3_gh, lossin, a.hindborg, aliceryhl, tmgross, jhubbard, ecourtney, cjia, smitra, kjaju, alkumar, ankita, aniketa, kwankhede, targupta, nova-gpu, linux-kernel, zhiwang, kvm On Wed, Sep 16, 2026 at 09:44:35AM +1000, Dave Airlie wrote: > As for the rust stuff, I think for vfio your drivers are just not that > much of a horror show or refactoring often enough that rust will be > anything but a blip on your radar, Uh? I've been sending a couple whole subsystem refactorings touching every driver a couple times a year for what, 5 years now? There are still more that are needed, like trying to redo the drvdata apparently. As well as stuff on the ops side, and more and more. Granted you are looking at it after all this work, but there are still problematic areas. drvdata, some of the ops, some of the locking, I think there will be alot of C work to fix things up enough rust bindings can happen sanely. It is not so easy, your LLM looks like it put PM trampolines in every driver, which I would not like to see.. Not sure how it solved vga without touching drm.. Jason ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 12/13] vfio/nvidia-vgpu: add the NVIDIA vGPU VFIO variant driver 2026-09-15 18:01 ` Alex Williamson 2026-09-15 21:19 ` Danilo Krummrich 2026-09-15 23:44 ` Dave Airlie @ 2026-09-17 7:12 ` Gary Guo 2 siblings, 0 replies; 24+ messages in thread From: Gary Guo @ 2026-09-17 7:12 UTC (permalink / raw) To: Alex Williamson, Danilo Krummrich Cc: Jason Gunthorpe, Zhi Wang, acourbot, yishaih, skolothumtho, kevin.tian, airlied, simona, ojeda, alex.gaynor, boqun.feng, gary, bjorn3_gh, lossin, a.hindborg, aliceryhl, tmgross, jhubbard, ecourtney, cjia, smitra, kjaju, alkumar, ankita, aniketa, kwankhede, targupta, nova-gpu, linux-kernel, zhiwang, kvm 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
end of thread, other threads:[~2026-09-23 20:05 UTC | newest] Thread overview: 24+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2026-09-05 8:11 [PATCH 00/13] Introduce NVIDIA vGPU manager and VFIO variant driver Zhi Wang 2026-09-05 8:11 ` [PATCH 12/13] vfio/nvidia-vgpu: add the NVIDIA vGPU " Zhi Wang 2026-09-09 3:00 ` Alex Williamson 2026-09-11 20:39 ` Danilo Krummrich 2026-09-14 18:12 ` Alex Williamson 2026-09-14 21:36 ` Danilo Krummrich 2026-09-15 18:01 ` Alex Williamson 2026-09-15 21:19 ` Danilo Krummrich 2026-09-15 22:55 ` Zhi Wang 2026-09-16 14:17 ` Jason Gunthorpe 2026-09-16 15:38 ` Danilo Krummrich 2026-09-16 16:28 ` Jason Gunthorpe 2026-09-16 18:02 ` Danilo Krummrich 2026-09-17 13:28 ` Jason Gunthorpe 2026-09-17 14:50 ` Danilo Krummrich 2026-09-17 17:10 ` Jason Gunthorpe 2026-09-17 20:09 ` Danilo Krummrich 2026-09-23 20:05 ` Alex Williamson 2026-09-17 1:49 ` Dave Airlie 2026-09-17 7:01 ` Gary Guo 2026-09-17 13:20 ` Jason Gunthorpe 2026-09-15 23:44 ` Dave Airlie 2026-09-16 14:25 ` Jason Gunthorpe 2026-09-17 7:12 ` Gary Guo
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox