[RFC 0/2] rust: introduce abstractions for fwctl

[RFC 0/2] rust: introduce abstractions for fwctl

[Zhi Wang]

In the NVIDIA vGPU RFC [1], the vGPU type blobs must be provided to the GSP
before userspace can enumerate available vGPU types and create vGPU
instances. The original design relied on the firmware loading interface,
but fwctl is a more natural fit for this use case, as it is designed for
uploading configuration or firmware data required before the device becomes
operational.

This patch introduces a Rust abstraction over the fwctl subsystem,
providing safe and idiomatic bindings.

The new `fwctl` module allows Rust drivers to integrate with the existing
C-side fwctl core through a typed trait interface. It provides:

  - `FwCtlOps` trait — defines driver-specific operations such as
    `open_uctx()`, `close_uctx()`, `info()`, and `fw_rpc()`.  
    Each Rust driver implements this trait to describe its own per-FD
    user-context behavior and RPC handling.

  - `FwCtlUCtx<T>` — a generic wrapper around `struct fwctl_uctx`
    embedding driver-specific context data, providing safe conversion
    from raw C pointers and access to the parent device.

  - `Registration<T>` — safe registration and automatic unregistration
    of `struct fwctl_device` objects using the kernel’s device model.

  - `FwCtlVTable<T>` — a static vtable bridging C callbacks and Rust
    trait methods, ensuring type safety across the FFI boundary.

`rust/kernel/lib.rs` is updated to conditionally include this module
under `CONFIG_FWCTL`.

[1] https://lore.kernel.org/all/20250903221111.3866249-1-zhiw@nvidia.com/

Zhi Wang (2):
  rust: introduce abstractions for fwctl
  samples: rust: fwctl: add sample code for FwCtl

 include/uapi/fwctl/fwctl.h        |   1 +
 rust/bindings/bindings_helper.h   |   1 +
 rust/kernel/fwctl.rs              | 254 ++++++++++++++++++++++++++++++
 rust/kernel/lib.rs                |   2 +
 samples/rust/Kconfig              |  11 ++
 samples/rust/Makefile             |   1 +
 samples/rust/rust_driver_fwctl.rs | 123 +++++++++++++++
 7 files changed, 393 insertions(+)
 create mode 100644 rust/kernel/fwctl.rs
 create mode 100644 samples/rust/rust_driver_fwctl.rs

-- 
2.47.3

[RFC 1/2] rust: introduce abstractions for fwctl

[Zhi Wang]

Introduce safe wrappers around `struct fwctl_device` and
`struct fwctl_uctx`, allowing rust drivers to register fwctl devices and
implement their control and RPC logic in safe rust.

The core of the abstraction is the `FwCtlOps` trait, which defines the
driver callbacks (`open_uctx`, `close_uctx`, `info`, and `fw_rpc`).
`FwCtlVTable` bridges these trait methods to the C fwctl core through a
static vtable.

`rust/kernel/lib.rs` is updated to conditionally build this module when
`CONFIG_FWCTL` is enabled.

Signed-off-by: Zhi Wang <zhiw@nvidia.com>
---
 rust/bindings/bindings_helper.h |   1 +
 rust/kernel/fwctl.rs            | 254 ++++++++++++++++++++++++++++++++
 rust/kernel/lib.rs              |   2 +
 3 files changed, 257 insertions(+)
 create mode 100644 rust/kernel/fwctl.rs

diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index 2e43c66635a2..5c374965f0f1 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -56,6 +56,7 @@
 #include <linux/fdtable.h>
 #include <linux/file.h>
 #include <linux/firmware.h>
+#include <linux/fwctl.h>
 #include <linux/interrupt.h>
 #include <linux/fs.h>
 #include <linux/ioport.h>
diff --git a/rust/kernel/fwctl.rs b/rust/kernel/fwctl.rs
new file mode 100644
index 000000000000..21f8f7d11d6f
--- /dev/null
+++ b/rust/kernel/fwctl.rs
@@ -0,0 +1,254 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+//! Abstractions for the fwctl.
+//!
+//! This module provides bindings for working with fwctl devices in kernel modules.
+//!
+//! C header: [`include/linux/fwctl.h`]
+
+use crate::device::Device;
+use crate::types::ARef;
+use crate::{bindings, container_of, device, error::code::*, prelude::*};
+
+use core::marker::PhantomData;
+use core::ptr::NonNull;
+use core::slice;
+
+/// The registration of a fwctl device.
+///
+/// This type represents the registration of a [`struct fwctl_device`]. When an instance of this
+/// type is dropped, its respective fwctl device will be unregistered and freed.
+///
+/// [`struct fwctl_device`]: srctree/include/linux/device/fwctl.h
+pub struct Registration<T: FwCtlOps> {
+    fwctl_dev: NonNull<bindings::fwctl_device>,
+    _marker: PhantomData<T>,
+}
+
+impl<T: FwCtlOps> Registration<T> {
+    /// Allocate and register a new fwctl device under the given parent device.
+    pub fn new(parent: &device::Device) -> Result<Self> {
+        let ops = &FwCtlVTable::<T>::VTABLE as *const _ as *mut _;
+
+        // SAFETY: `_fwctl_alloc_device()` allocates a new `fwctl_device`
+        // and initializes its embedded `struct device`.
+        let dev = unsafe {
+            bindings::_fwctl_alloc_device(
+                parent.as_raw(),
+                ops,
+                core::mem::size_of::<bindings::fwctl_device>(),
+            )
+        };
+
+        let dev = NonNull::new(dev).ok_or(ENOMEM)?;
+
+        // SAFETY: `fwctl_register()` expects a valid device from `_fwctl_alloc_device()`.
+        let ret = unsafe { bindings::fwctl_register(dev.as_ptr()) };
+        if ret != 0 {
+            // SAFETY: If registration fails, release the allocated fwctl_device().
+            unsafe {
+                bindings::put_device(core::ptr::addr_of_mut!((*dev.as_ptr()).dev));
+            }
+            return Err(Error::from_errno(ret));
+        }
+
+        Ok(Self {
+            fwctl_dev: dev,
+            _marker: PhantomData,
+        })
+    }
+
+    fn as_raw(&self) -> *mut bindings::fwctl_device {
+        self.fwctl_dev.as_ptr()
+    }
+}
+
+impl<T: FwCtlOps> Drop for Registration<T> {
+    fn drop(&mut self) {
+        // SAFETY: `fwctl_unregister()` expects a valid device from `_fwctl_alloc_device()`.
+        unsafe {
+            bindings::fwctl_unregister(self.as_raw());
+            bindings::put_device(core::ptr::addr_of_mut!((*self.as_raw()).dev));
+        }
+    }
+}
+
+// SAFETY: The only action allowed in a `Registration` instance is dropping it, which is safe to do
+// from any thread because `fwctl_unregister()/put_device()` can be called from any sleepible
+// context.
+unsafe impl<T: FwCtlOps> Send for Registration<T> {}
+
+/// Trait implemented by each Rust driver that integrates with the fwctl subsystem.
+///
+/// Each implementation corresponds to a specific device type and provides
+/// the vtable used by the core `fwctl` layer to manage per-FD user contexts
+/// and handle RPC requests.
+pub trait FwCtlOps: Sized {
+    /// Driver UCtx type.
+    type UCtx;
+
+    /// fwctl device type, matching the C enum `fwctl_device_type`.
+    const DEVICE_TYPE: u32;
+
+    /// Called when a new user context is opened by userspace.
+    fn open_uctx(uctx: &mut FwCtlUCtx<Self::UCtx>) -> Result<(), Error>;
+
+    /// Called when the user context is being closed.
+    fn close_uctx(uctx: &mut FwCtlUCtx<Self::UCtx>);
+
+    /// Return device or context information to userspace.
+    fn info(uctx: &mut FwCtlUCtx<Self::UCtx>) -> Result<KVec<u8>, Error>;
+
+    /// Called when a userspace RPC request is received.
+    fn fw_rpc(
+        uctx: &mut FwCtlUCtx<Self::UCtx>,
+        scope: u32,
+        rpc_in: &mut [u8],
+        out_len: *mut usize,
+    ) -> Result<Option<KVec<u8>>, Error>;
+}
+
+/// Represents a per-FD user context (`struct fwctl_uctx`).
+///
+/// Each driver embeds `struct fwctl_uctx` as the first field of its own
+/// context type and uses this wrapper to access driver-specific data.
+#[repr(C)]
+#[pin_data]
+pub struct FwCtlUCtx<T> {
+    /// The core fwctl user context shared with the C implementation.
+    #[pin]
+    pub fwctl_uctx: bindings::fwctl_uctx,
+    /// Driver-specific data associated with this user context.
+    pub uctx: T,
+}
+
+impl<T> FwCtlUCtx<T> {
+    /// Converts a raw C pointer to `struct fwctl_uctx` into a reference to the
+    /// enclosing `FwCtlUCtx<T>`.
+    ///
+    /// # Safety
+    /// * `ptr` must be a valid pointer to a `fwctl_uctx` that is embedded
+    ///   inside an existing `FwCtlUCtx<T>` instance.
+    /// * The caller must ensure that the lifetime of the returned reference
+    ///   does not outlive the underlying object managed on the C side.
+    pub unsafe fn from_raw<'a>(ptr: *mut bindings::fwctl_uctx) -> &'a mut Self {
+        // SAFETY: `ptr` was originally created from a valid `FwCtlUCtx<T>`.
+        unsafe { &mut *container_of!(ptr, FwCtlUCtx<T>, fwctl_uctx) }
+    }
+
+    /// Returns the parent device of this user context.
+    ///
+    /// # Safety
+    /// The `fwctl_device` pointer inside `fwctl_uctx` must be valid.
+    pub fn get_parent_device(&self) -> ARef<Device> {
+        // SAFETY: `self.fwctl_uctx.fwctl` is initialized by the fwctl subsystem and guaranteed
+        // to remain valid for the lifetime of this `FwCtlUCtx`.
+        let raw_dev =
+            unsafe { (*(self.fwctl_uctx.fwctl)).dev.parent as *mut kernel::bindings::device };
+        // SAFETY: `raw_dev` points to a live device object.
+        unsafe { Device::get_device(raw_dev) }
+    }
+
+    /// Returns a mutable reference to the driver-specific context.
+    pub fn to_driver_uctx_mut(&mut self) -> &mut T {
+        &mut self.uctx
+    }
+}
+
+/// Static vtable mapping Rust trait methods to C callbacks.
+pub struct FwCtlVTable<T: FwCtlOps>(PhantomData<T>);
+
+impl<T: FwCtlOps> FwCtlVTable<T> {
+    /// Static instance of `fwctl_ops` used by the C core to call into Rust.
+    pub const VTABLE: bindings::fwctl_ops = bindings::fwctl_ops {
+        device_type: T::DEVICE_TYPE,
+        uctx_size: core::mem::size_of::<FwCtlUCtx<T::UCtx>>(),
+        open_uctx: Some(Self::open_uctx_callback),
+        close_uctx: Some(Self::close_uctx_callback),
+        info: Some(Self::info_callback),
+        fw_rpc: Some(Self::fw_rpc_callback),
+    };
+
+    /// Called when a new user context is opened by userspace.
+    unsafe extern "C" fn open_uctx_callback(uctx: *mut bindings::fwctl_uctx) -> ffi::c_int {
+        // SAFETY: `uctx` is guaranteed by the fwctl subsystem to be a valid pointer.
+        let ctx = unsafe { FwCtlUCtx::<T::UCtx>::from_raw(uctx) };
+        match T::open_uctx(ctx) {
+            Ok(()) => 0,
+            Err(e) => e.to_errno(),
+        }
+    }
+
+    /// Called when the user context is being closed.
+    unsafe extern "C" fn close_uctx_callback(uctx: *mut bindings::fwctl_uctx) {
+        // SAFETY: `uctx` is guaranteed by the fwctl subsystem to be a valid pointer.
+        let ctx = unsafe { FwCtlUCtx::<T::UCtx>::from_raw(uctx) };
+        T::close_uctx(ctx);
+    }
+
+    /// Returns device or context information.
+    unsafe extern "C" fn info_callback(
+        uctx: *mut bindings::fwctl_uctx,
+        length: *mut usize,
+    ) -> *mut ffi::c_void {
+        // SAFETY: `uctx` is guaranteed by the fwctl subsystem to be a valid pointer.
+        let ctx = unsafe { FwCtlUCtx::<T::UCtx>::from_raw(uctx) };
+
+        match T::info(ctx) {
+            Ok(kvec) => {
+                // The ownership of the buffer is now transferred to the foreign
+                // caller. It must eventually be released by fwctl framework.
+                let (ptr, len, _cap) = kvec.into_raw_parts();
+
+                // SAFETY: `length` is a valid out-parameter provided by the C
+                // caller. Write the number of bytes in the returned buffer.
+                unsafe {
+                    *length = len;
+                }
+
+                ptr.cast::<ffi::c_void>()
+            }
+
+            Err(e) => Error::to_ptr(e),
+        }
+    }
+
+    /// Called when a user-space RPC request is received.
+    unsafe extern "C" fn fw_rpc_callback(
+        uctx: *mut bindings::fwctl_uctx,
+        scope: u32,
+        rpc_in: *mut ffi::c_void,
+        in_len: usize,
+        out_len: *mut usize,
+    ) -> *mut ffi::c_void {
+        // SAFETY: `uctx` is guaranteed by the fwctl framework to be a valid pointer.
+        let ctx = unsafe { FwCtlUCtx::<T::UCtx>::from_raw(uctx) };
+
+        // SAFETY: `rpc_in` points to a valid input buffer of size `in_len`
+        // provided by fwctl subsystem.
+        let rpc_in_slice: &mut [u8] =
+            unsafe { slice::from_raw_parts_mut(rpc_in as *mut u8, in_len) };
+
+        match T::fw_rpc(ctx, scope, rpc_in_slice, out_len) {
+            // Driver allocates a new output buffer.
+            Ok(Some(kvec)) => {
+                // The ownership of the buffer is now transferred to the foreign
+                // caller. It must eventually be released by fwctl subsystem.
+                let (ptr, len, _cap) = kvec.into_raw_parts();
+
+                // SAFETY: `out_len` is a valid writable pointer provided by the C caller.
+                unsafe {
+                    *out_len = len;
+                }
+
+                ptr.cast::<ffi::c_void>()
+            }
+
+            // Driver re-uses the existing input buffer and writes the out_len.
+            Ok(None) => rpc_in,
+
+            // Return an ERR_PTR-style encoded error pointer.
+            Err(e) => Error::to_ptr(e),
+        }
+    }
+}
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index 3dd7bebe7888..8ddad8ae211a 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -94,6 +94,8 @@
 pub mod firmware;
 pub mod fmt;
 pub mod fs;
+#[cfg(CONFIG_FWCTL)]
+pub mod fwctl;
 pub mod id_pool;
 pub mod init;
 pub mod io;
-- 
2.47.3

[RFC 2/2] samples: rust: fwctl: add sample code for FwCtl

[Zhi Wang]

Add sample code for creating a FwCtl device, getting device info and
issuing an RPC.

Signed-off-by: Zhi Wang <zhiw@nvidia.com>
---
 include/uapi/fwctl/fwctl.h        |   1 +
 samples/rust/Kconfig              |  11 +++
 samples/rust/Makefile             |   1 +
 samples/rust/rust_driver_fwctl.rs | 123 ++++++++++++++++++++++++++++++
 4 files changed, 136 insertions(+)
 create mode 100644 samples/rust/rust_driver_fwctl.rs

diff --git a/include/uapi/fwctl/fwctl.h b/include/uapi/fwctl/fwctl.h
index 716ac0eee42d..eea1020ad180 100644
--- a/include/uapi/fwctl/fwctl.h
+++ b/include/uapi/fwctl/fwctl.h
@@ -45,6 +45,7 @@ enum fwctl_device_type {
 	FWCTL_DEVICE_TYPE_MLX5 = 1,
 	FWCTL_DEVICE_TYPE_CXL = 2,
 	FWCTL_DEVICE_TYPE_PDS = 4,
+	FWCTL_DEVICE_TYPE_RUST_FWCTL_TEST = 8,
 };
 
 /**
diff --git a/samples/rust/Kconfig b/samples/rust/Kconfig
index c376eb899b7a..54ed2b0b86e2 100644
--- a/samples/rust/Kconfig
+++ b/samples/rust/Kconfig
@@ -138,6 +138,17 @@ config SAMPLE_RUST_DRIVER_AUXILIARY
 
 	  If unsure, say N.
 
+config SAMPLE_RUST_DRIVER_FWCTL
+	tristate "Fwctl Driver"
+	depends on FWCTL
+	help
+	  This option builds the Rust Fwctl driver sample.
+
+	  To compile this as a module, choose M here:
+	  the module will be called rust_driver_fwctl.
+
+	  If unsure, say N.
+
 config SAMPLE_RUST_HOSTPROGS
 	bool "Host programs"
 	help
diff --git a/samples/rust/Makefile b/samples/rust/Makefile
index cf8422f8f219..643208c2380e 100644
--- a/samples/rust/Makefile
+++ b/samples/rust/Makefile
@@ -12,6 +12,7 @@ obj-$(CONFIG_SAMPLE_RUST_DRIVER_PLATFORM)	+= rust_driver_platform.o
 obj-$(CONFIG_SAMPLE_RUST_DRIVER_USB)		+= rust_driver_usb.o
 obj-$(CONFIG_SAMPLE_RUST_DRIVER_FAUX)		+= rust_driver_faux.o
 obj-$(CONFIG_SAMPLE_RUST_DRIVER_AUXILIARY)	+= rust_driver_auxiliary.o
+obj-$(CONFIG_SAMPLE_RUST_DRIVER_FWCTL)		+= rust_driver_fwctl.o
 obj-$(CONFIG_SAMPLE_RUST_CONFIGFS)		+= rust_configfs.o
 
 rust_print-y := rust_print_main.o rust_print_events.o
diff --git a/samples/rust/rust_driver_fwctl.rs b/samples/rust/rust_driver_fwctl.rs
new file mode 100644
index 000000000000..386299eaf82c
--- /dev/null
+++ b/samples/rust/rust_driver_fwctl.rs
@@ -0,0 +1,123 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Rust fwctl API test (based on QEMU's `pci-testdev`).
+//!
+//! To make this driver probe, QEMU must be run with `-device pci-testdev`.
+
+use kernel::{
+    bindings, device::Core, fwctl, fwctl::FwCtlOps, fwctl::FwCtlUCtx, pci, prelude::*,
+    sync::aref::ARef,
+};
+
+struct FwCtlSampleUCtx {
+    _drvdata: u32,
+}
+
+struct FwCtlSampleOps;
+
+impl FwCtlOps for FwCtlSampleOps {
+    type UCtx = FwCtlSampleUCtx;
+
+    const DEVICE_TYPE: u32 = bindings::fwctl_device_type_FWCTL_DEVICE_TYPE_RUST_FWCTL_TEST as u32;
+
+    fn open_uctx(uctx: &mut FwCtlUCtx<FwCtlSampleUCtx>) -> Result<(), Error> {
+        let dev = uctx.get_parent_device();
+
+        dev_info!(dev, "fwctl test driver: open_uctx().\n");
+        Ok(())
+    }
+
+    fn close_uctx(uctx: &mut FwCtlUCtx<FwCtlSampleUCtx>) {
+        let dev = uctx.get_parent_device();
+
+        dev_info!(dev, "fwctl test driver: close_uctx().\n");
+    }
+
+    fn info(uctx: &mut FwCtlUCtx<FwCtlSampleUCtx>) -> Result<KVec<u8>, Error> {
+        let dev = uctx.get_parent_device();
+
+        dev_info!(dev, "fwctl test driver: info().\n");
+
+        let mut infobuf = KVec::<u8>::new();
+        infobuf.push(0xef, GFP_KERNEL)?;
+        infobuf.push(0xbe, GFP_KERNEL)?;
+        infobuf.push(0xad, GFP_KERNEL)?;
+        infobuf.push(0xde, GFP_KERNEL)?;
+
+        Ok(infobuf)
+    }
+
+    fn fw_rpc(
+        uctx: &mut FwCtlUCtx<FwCtlSampleUCtx>,
+        scope: u32,
+        rpc_in: &mut [u8],
+        _out_len: *mut usize,
+    ) -> Result<Option<KVec<u8>>, Error> {
+        let dev = uctx.get_parent_device();
+
+        dev_info!(dev, "fwctl test driver: fw_rpc() scope {}.\n", scope);
+
+        if rpc_in.len() != 4 {
+            return Err(EINVAL);
+        }
+
+        dev_info!(
+            dev,
+            "fwctl test driver: inbuf len{} bytes[0-3] {:x} {:x} {:x} {:x}.\n",
+            rpc_in.len(),
+            rpc_in[0],
+            rpc_in[1],
+            rpc_in[2],
+            rpc_in[3]
+        );
+
+        let mut outbuf = KVec::<u8>::new();
+        outbuf.push(0xef, GFP_KERNEL)?;
+        outbuf.push(0xbe, GFP_KERNEL)?;
+        outbuf.push(0xad, GFP_KERNEL)?;
+        outbuf.push(0xde, GFP_KERNEL)?;
+
+        Ok(Some(outbuf))
+    }
+}
+
+#[pin_data]
+struct FwCtlSampleDriver {
+    pdev: ARef<pci::Device>,
+    #[pin]
+    fwctl: fwctl::Registration<FwCtlSampleOps>,
+}
+
+kernel::pci_device_table!(
+    PCI_TABLE,
+    MODULE_PCI_TABLE,
+    <FwCtlSampleDriver as pci::Driver>::IdInfo,
+    [(pci::DeviceId::from_id(pci::Vendor::REDHAT, 0x5), ())]
+);
+
+impl pci::Driver for FwCtlSampleDriver {
+    type IdInfo = ();
+    const ID_TABLE: pci::IdTable<Self::IdInfo> = &PCI_TABLE;
+
+    fn probe(pdev: &pci::Device<Core>, _info: &Self::IdInfo) -> Result<Pin<KBox<Self>>> {
+        dev_info!(pdev.as_ref(), "Probe fwctl test driver.\n");
+
+        let drvdata = KBox::pin_init(
+            try_pin_init!(Self {
+                pdev: pdev.into(),
+                fwctl <- fwctl::Registration::<FwCtlSampleOps>::new(pdev.as_ref())?,
+            }),
+            GFP_KERNEL,
+        )?;
+
+        Ok(drvdata)
+    }
+}
+
+kernel::module_pci_driver! {
+    type: FwCtlSampleDriver,
+    name: "rust_driver_fwctl",
+    authors: ["Zhi Wang"],
+    description: "Rust fwctl test",
+    license: "GPL v2",
+}
-- 
2.47.3

Re: [RFC 1/2] rust: introduce abstractions for fwctl

[Jason Gunthorpe]

On Thu, Oct 30, 2025 at 04:03:12PM +0000, Zhi Wang wrote:
> +impl<T: FwCtlOps> Registration<T> {
> +    /// Allocate and register a new fwctl device under the given parent device.
> +    pub fn new(parent: &device::Device) -> Result<Self> {
> +        let ops = &FwCtlVTable::<T>::VTABLE as *const _ as *mut _;
> +
> +        // SAFETY: `_fwctl_alloc_device()` allocates a new `fwctl_device`
> +        // and initializes its embedded `struct device`.
> +        let dev = unsafe {
> +            bindings::_fwctl_alloc_device(
> +                parent.as_raw(),
> +                ops,
> +                core::mem::size_of::<bindings::fwctl_device>(),
> +            )
> +        };
> +
> +        let dev = NonNull::new(dev).ok_or(ENOMEM)?;
> +
> +        // SAFETY: `fwctl_register()` expects a valid device from `_fwctl_alloc_device()`.
> +        let ret = unsafe { bindings::fwctl_register(dev.as_ptr()) };

This is a Bound device, not just any device.

> +        if ret != 0 {
> +            // SAFETY: If registration fails, release the allocated fwctl_device().
> +            unsafe {
> +                bindings::put_device(core::ptr::addr_of_mut!((*dev.as_ptr()).dev));

?? Don't open code fwctl_put() - it should be called directly?

> +            }
> +            return Err(Error::from_errno(ret));
> +        }
> +
> +        Ok(Self {
> +            fwctl_dev: dev,
> +            _marker: PhantomData,
> +        })
> +    }
> +
> +    fn as_raw(&self) -> *mut bindings::fwctl_device {
> +        self.fwctl_dev.as_ptr()
> +    }
> +}
> +
> +impl<T: FwCtlOps> Drop for Registration<T> {
> +    fn drop(&mut self) {
> +        // SAFETY: `fwctl_unregister()` expects a valid device from `_fwctl_alloc_device()`.

Incomplete safety statement, the device passed to fwctl_alloc_device must
still be bound prior to calling fwctl_unregister

> +        unsafe {
> +            bindings::fwctl_unregister(self.as_raw());
> +            bindings::put_device(core::ptr::addr_of_mut!((*self.as_raw()).dev));

There for Drop can only do fwctl_put() since otherwise there is no way
to guarantee a Bound device.

unregister has to happen before remove() completes, Danilo had some
approach to this I think he told me?

Jason

Re: [RFC 1/2] rust: introduce abstractions for fwctl

[Danilo Krummrich]

On Thu Oct 30, 2025 at 5:03 PM CET, Zhi Wang wrote:
> diff --git a/rust/kernel/fwctl.rs b/rust/kernel/fwctl.rs
> new file mode 100644
> index 000000000000..21f8f7d11d6f
> --- /dev/null
> +++ b/rust/kernel/fwctl.rs
> @@ -0,0 +1,254 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +//! Abstractions for the fwctl.
> +//!
> +//! This module provides bindings for working with fwctl devices in kernel modules.
> +//!
> +//! C header: [`include/linux/fwctl.h`]
> +
> +use crate::device::Device;
> +use crate::types::ARef;
> +use crate::{bindings, container_of, device, error::code::*, prelude::*};
> +
> +use core::marker::PhantomData;
> +use core::ptr::NonNull;
> +use core::slice;

Please use the import scheme as documented in [1].

[1] https://docs.kernel.org/rust/coding-guidelines.html#imports

> +/// The registration of a fwctl device.
> +///
> +/// This type represents the registration of a [`struct fwctl_device`]. When an instance of this
> +/// type is dropped, its respective fwctl device will be unregistered and freed.
> +///
> +/// [`struct fwctl_device`]: srctree/include/linux/device/fwctl.h
> +pub struct Registration<T: FwCtlOps> {
> +    fwctl_dev: NonNull<bindings::fwctl_device>,

Given that this structure has to keep a reference count of the fwctl_device, I'd
prefer to have an abstraction of struct fwctl_device (fwctl::Device) which
implements AlwaysRefCounted.

This way the Registration can store an ARef<fwctl::Device> rather than a raw
pointer.

However, I wonder if we really need a reference count? Does fwctl_register() not
take a reference count itself?

> +    _marker: PhantomData<T>,
> +}
> +
> +impl<T: FwCtlOps> Registration<T> {
> +    /// Allocate and register a new fwctl device under the given parent device.
> +    pub fn new(parent: &device::Device) -> Result<Self> {

AFAIK, fwctl_unregister() is synchronized against IOCTLs. Hence, if we guarantee
that a fwctl::Registration can not out-live parent device unbind, we can provide
a &Device<Bound> in the FwCtlOps callbacks, which allows us to do zero-cost
accesses of device resources with Devres::access().

In order to provide this guarantee, this function should return
impl PinInit<Devres<Self>, Error>.

> +        let ops = &FwCtlVTable::<T>::VTABLE as *const _ as *mut _;

Please use cast() and cast_mut() when possible.

> +
> +        // SAFETY: `_fwctl_alloc_device()` allocates a new `fwctl_device`
> +        // and initializes its embedded `struct device`.

This safety comment should justify how you guarantee that the arguments you pass
in are valid, instead of describing what the called function does.

> +        let dev = unsafe {
> +            bindings::_fwctl_alloc_device(
> +                parent.as_raw(),
> +                ops,
> +                core::mem::size_of::<bindings::fwctl_device>(),
> +            )
> +        };
> +
> +        let dev = NonNull::new(dev).ok_or(ENOMEM)?;
> +
> +        // SAFETY: `fwctl_register()` expects a valid device from `_fwctl_alloc_device()`.
> +        let ret = unsafe { bindings::fwctl_register(dev.as_ptr()) };
> +        if ret != 0 {
> +            // SAFETY: If registration fails, release the allocated fwctl_device().
> +            unsafe {
> +                bindings::put_device(core::ptr::addr_of_mut!((*dev.as_ptr()).dev));
> +            }
> +            return Err(Error::from_errno(ret));
> +        }
> +
> +        Ok(Self {
> +            fwctl_dev: dev,
> +            _marker: PhantomData,
> +        })
> +    }
> +
> +    fn as_raw(&self) -> *mut bindings::fwctl_device {
> +        self.fwctl_dev.as_ptr()
> +    }
> +}
> +
> +impl<T: FwCtlOps> Drop for Registration<T> {
> +    fn drop(&mut self) {
> +        // SAFETY: `fwctl_unregister()` expects a valid device from `_fwctl_alloc_device()`.
> +        unsafe {
> +            bindings::fwctl_unregister(self.as_raw());
> +            bindings::put_device(core::ptr::addr_of_mut!((*self.as_raw()).dev));
> +        }
> +    }
> +}
> +
> +// SAFETY: The only action allowed in a `Registration` instance is dropping it, which is safe to do
> +// from any thread because `fwctl_unregister()/put_device()` can be called from any sleepible
> +// context.
> +unsafe impl<T: FwCtlOps> Send for Registration<T> {}
> +
> +/// Trait implemented by each Rust driver that integrates with the fwctl subsystem.
> +///
> +/// Each implementation corresponds to a specific device type and provides
> +/// the vtable used by the core `fwctl` layer to manage per-FD user contexts
> +/// and handle RPC requests.
> +pub trait FwCtlOps: Sized {
> +    /// Driver UCtx type.
> +    type UCtx;
> +
> +    /// fwctl device type, matching the C enum `fwctl_device_type`.
> +    const DEVICE_TYPE: u32;
> +
> +    /// Called when a new user context is opened by userspace.
> +    fn open_uctx(uctx: &mut FwCtlUCtx<Self::UCtx>) -> Result<(), Error>;
> +
> +    /// Called when the user context is being closed.
> +    fn close_uctx(uctx: &mut FwCtlUCtx<Self::UCtx>);

Why not just open() and close()?

> +    /// Return device or context information to userspace.
> +    fn info(uctx: &mut FwCtlUCtx<Self::UCtx>) -> Result<KVec<u8>, Error>;
> +
> +    /// Called when a userspace RPC request is received.
> +    fn fw_rpc(
> +        uctx: &mut FwCtlUCtx<Self::UCtx>,
> +        scope: u32,
> +        rpc_in: &mut [u8],
> +        out_len: *mut usize,
> +    ) -> Result<Option<KVec<u8>>, Error>;

As mentioned above, if we ensure that a fwctl::Registration cannot out-live the
parent device being bound, we can provide a &Device<Bound> in those callbacks
for zero-cost accesses of device resources with Devres::access().

> +}
> +
> +/// Represents a per-FD user context (`struct fwctl_uctx`).
> +///
> +/// Each driver embeds `struct fwctl_uctx` as the first field of its own
> +/// context type and uses this wrapper to access driver-specific data.
> +#[repr(C)]
> +#[pin_data]
> +pub struct FwCtlUCtx<T> {
> +    /// The core fwctl user context shared with the C implementation.
> +    #[pin]
> +    pub fwctl_uctx: bindings::fwctl_uctx,

This should be Opaque<bindings::fwctl_uctx> and should not be a public field.

> +    /// Driver-specific data associated with this user context.
> +    pub uctx: T,

I'd rather provide a Deref and DerefMut implementation for this.

> +}
> +
> +impl<T> FwCtlUCtx<T> {
> +    /// Converts a raw C pointer to `struct fwctl_uctx` into a reference to the
> +    /// enclosing `FwCtlUCtx<T>`.
> +    ///
> +    /// # Safety
> +    /// * `ptr` must be a valid pointer to a `fwctl_uctx` that is embedded
> +    ///   inside an existing `FwCtlUCtx<T>` instance.
> +    /// * The caller must ensure that the lifetime of the returned reference
> +    ///   does not outlive the underlying object managed on the C side.
> +    pub unsafe fn from_raw<'a>(ptr: *mut bindings::fwctl_uctx) -> &'a mut Self {

Why does this need to be public?

> +        // SAFETY: `ptr` was originally created from a valid `FwCtlUCtx<T>`.
> +        unsafe { &mut *container_of!(ptr, FwCtlUCtx<T>, fwctl_uctx) }
> +    }
> +
> +    /// Returns the parent device of this user context.
> +    ///
> +    /// # Safety
> +    /// The `fwctl_device` pointer inside `fwctl_uctx` must be valid.
> +    pub fn get_parent_device(&self) -> ARef<Device> {

We the fwctl::Registration changes suggested above, this should return a
&Device<Bound>.

Regardless of this, it's better to return a &Device than an ARef<Device>. The
caller can always obtain a reference count, i.e. ARef<Device> from a &Device (or
a &Device<Bound>).

> +        // SAFETY: `self.fwctl_uctx.fwctl` is initialized by the fwctl subsystem and guaranteed
> +        // to remain valid for the lifetime of this `FwCtlUCtx`.
> +        let raw_dev =
> +            unsafe { (*(self.fwctl_uctx.fwctl)).dev.parent as *mut kernel::bindings::device };
> +        // SAFETY: `raw_dev` points to a live device object.
> +        unsafe { Device::get_device(raw_dev) }
> +    }
> +
> +    /// Returns a mutable reference to the driver-specific context.
> +    pub fn to_driver_uctx_mut(&mut self) -> &mut T {
> +        &mut self.uctx
> +    }

As mentioned, I think Deref and DerefMut are a better fit for this.

> +}
> +
> +/// Static vtable mapping Rust trait methods to C callbacks.
> +pub struct FwCtlVTable<T: FwCtlOps>(PhantomData<T>);
> +
> +impl<T: FwCtlOps> FwCtlVTable<T> {
> +    /// Static instance of `fwctl_ops` used by the C core to call into Rust.
> +    pub const VTABLE: bindings::fwctl_ops = bindings::fwctl_ops {
> +        device_type: T::DEVICE_TYPE,
> +        uctx_size: core::mem::size_of::<FwCtlUCtx<T::UCtx>>(),
> +        open_uctx: Some(Self::open_uctx_callback),
> +        close_uctx: Some(Self::close_uctx_callback),
> +        info: Some(Self::info_callback),
> +        fw_rpc: Some(Self::fw_rpc_callback),
> +    };
> +
> +    /// Called when a new user context is opened by userspace.
> +    unsafe extern "C" fn open_uctx_callback(uctx: *mut bindings::fwctl_uctx) -> ffi::c_int {
> +        // SAFETY: `uctx` is guaranteed by the fwctl subsystem to be a valid pointer.
> +        let ctx = unsafe { FwCtlUCtx::<T::UCtx>::from_raw(uctx) };
> +        match T::open_uctx(ctx) {
> +            Ok(()) => 0,
> +            Err(e) => e.to_errno(),
> +        }
> +    }
> +
> +    /// Called when the user context is being closed.
> +    unsafe extern "C" fn close_uctx_callback(uctx: *mut bindings::fwctl_uctx) {
> +        // SAFETY: `uctx` is guaranteed by the fwctl subsystem to be a valid pointer.
> +        let ctx = unsafe { FwCtlUCtx::<T::UCtx>::from_raw(uctx) };
> +        T::close_uctx(ctx);
> +    }
> +
> +    /// Returns device or context information.
> +    unsafe extern "C" fn info_callback(
> +        uctx: *mut bindings::fwctl_uctx,
> +        length: *mut usize,
> +    ) -> *mut ffi::c_void {
> +        // SAFETY: `uctx` is guaranteed by the fwctl subsystem to be a valid pointer.
> +        let ctx = unsafe { FwCtlUCtx::<T::UCtx>::from_raw(uctx) };
> +
> +        match T::info(ctx) {
> +            Ok(kvec) => {
> +                // The ownership of the buffer is now transferred to the foreign
> +                // caller. It must eventually be released by fwctl framework.
> +                let (ptr, len, _cap) = kvec.into_raw_parts();
> +
> +                // SAFETY: `length` is a valid out-parameter provided by the C
> +                // caller. Write the number of bytes in the returned buffer.
> +                unsafe {
> +                    *length = len;
> +                }
> +
> +                ptr.cast::<ffi::c_void>()
> +            }
> +
> +            Err(e) => Error::to_ptr(e),
> +        }
> +    }
> +
> +    /// Called when a user-space RPC request is received.
> +    unsafe extern "C" fn fw_rpc_callback(
> +        uctx: *mut bindings::fwctl_uctx,
> +        scope: u32,
> +        rpc_in: *mut ffi::c_void,
> +        in_len: usize,
> +        out_len: *mut usize,
> +    ) -> *mut ffi::c_void {
> +        // SAFETY: `uctx` is guaranteed by the fwctl framework to be a valid pointer.
> +        let ctx = unsafe { FwCtlUCtx::<T::UCtx>::from_raw(uctx) };
> +
> +        // SAFETY: `rpc_in` points to a valid input buffer of size `in_len`
> +        // provided by fwctl subsystem.

Please see the safety requirements of slice::from_raw_parts_mut() and justify
all of them.

> +        let rpc_in_slice: &mut [u8] =
> +            unsafe { slice::from_raw_parts_mut(rpc_in as *mut u8, in_len) };
> +
> +        match T::fw_rpc(ctx, scope, rpc_in_slice, out_len) {
> +            // Driver allocates a new output buffer.
> +            Ok(Some(kvec)) => {
> +                // The ownership of the buffer is now transferred to the foreign
> +                // caller. It must eventually be released by fwctl subsystem.
> +                let (ptr, len, _cap) = kvec.into_raw_parts();
> +
> +                // SAFETY: `out_len` is a valid writable pointer provided by the C caller.
> +                unsafe {
> +                    *out_len = len;
> +                }

NIT: If you move the semicolon at the end of the unsafe block, this is formatted
in a single line.

> +
> +                ptr.cast::<ffi::c_void>()
> +            }
> +
> +            // Driver re-uses the existing input buffer and writes the out_len.
> +            Ok(None) => rpc_in,
> +
> +            // Return an ERR_PTR-style encoded error pointer.
> +            Err(e) => Error::to_ptr(e),
> +        }
> +    }
> +}

Re: [RFC 1/2] rust: introduce abstractions for fwctl

[Zhi Wang]

On 30.10.2025 18.22, Jason Gunthorpe wrote:
> On Thu, Oct 30, 2025 at 04:03:12PM +0000, Zhi Wang wrote:
>> +impl<T: FwCtlOps> Registration<T> {
>> +    /// Allocate and register a new fwctl device under the given parent device.
>> +    pub fn new(parent: &device::Device) -> Result<Self> {
>> +        let ops = &FwCtlVTable::<T>::VTABLE as *const _ as *mut _;
>> +
>> +        // SAFETY: `_fwctl_alloc_device()` allocates a new `fwctl_device`
>> +        // and initializes its embedded `struct device`.
>> +        let dev = unsafe {
>> +            bindings::_fwctl_alloc_device(
>> +                parent.as_raw(),
>> +                ops,
>> +                core::mem::size_of::<bindings::fwctl_device>(),
>> +            )
>> +        };
>> +
>> +        let dev = NonNull::new(dev).ok_or(ENOMEM)?;
>> +
>> +        // SAFETY: `fwctl_register()` expects a valid device from `_fwctl_alloc_device()`.
>> +        let ret = unsafe { bindings::fwctl_register(dev.as_ptr()) };
> 
> This is a Bound device, not just any device.
> 

Will do this in the next re-spin. It needs extra abstraction around fwctl_device.

>> +        if ret != 0 {
>> +            // SAFETY: If registration fails, release the allocated fwctl_device().
>> +            unsafe {
>> +                bindings::put_device(core::ptr::addr_of_mut!((*dev.as_ptr()).dev));
> 
> ?? Don't open code fwctl_put() - it should be called directly?

fwctl_put() is a inline function and it seem opened by the compiler when bindings are
generated. 

$ grep -R fwctl_put *
$ pwd
/home/inno/drm-rust/rust/bindings
$ grep -R put_device *
bindings_generated.rs:    pub fn put_device(dev: *mut device);

Hehe. I am open to options.

Z.
> 
>> +            }
>> +            return Err(Error::from_errno(ret));
>> +        }
>> +
>> +        Ok(Self {
>> +            fwctl_dev: dev,
>> +            _marker: PhantomData,
>> +        })
>> +    }
>> +
>> +    fn as_raw(&self) -> *mut bindings::fwctl_device {
>> +        self.fwctl_dev.as_ptr()
>> +    }
>> +}
>> +
>> +impl<T: FwCtlOps> Drop for Registration<T> {
>> +    fn drop(&mut self) {
>> +        // SAFETY: `fwctl_unregister()` expects a valid device from `_fwctl_alloc_device()`.
> 
> Incomplete safety statement, the device passed to fwctl_alloc_device must
> still be bound prior to calling fwctl_unregister
> 
>> +        unsafe {
>> +            bindings::fwctl_unregister(self.as_raw());
>> +            bindings::put_device(core::ptr::addr_of_mut!((*self.as_raw()).dev));
> 
> There for Drop can only do fwctl_put() since otherwise there is no way
> to guarantee a Bound device.
> 
> unregister has to happen before remove() completes, Danilo had some
> approach to this I think he told me?
> 
> Jason

Re: [RFC 1/2] rust: introduce abstractions for fwctl

[Danilo Krummrich]

On Thu Oct 30, 2025 at 5:22 PM CET, Jason Gunthorpe wrote:
> On Thu, Oct 30, 2025 at 04:03:12PM +0000, Zhi Wang wrote:
>> +impl<T: FwCtlOps> Registration<T> {
>> +    /// Allocate and register a new fwctl device under the given parent device.
>> +    pub fn new(parent: &device::Device) -> Result<Self> {
>> +        let ops = &FwCtlVTable::<T>::VTABLE as *const _ as *mut _;
>> +
>> +        // SAFETY: `_fwctl_alloc_device()` allocates a new `fwctl_device`
>> +        // and initializes its embedded `struct device`.
>> +        let dev = unsafe {
>> +            bindings::_fwctl_alloc_device(
>> +                parent.as_raw(),
>> +                ops,
>> +                core::mem::size_of::<bindings::fwctl_device>(),
>> +            )
>> +        };
>> +
>> +        let dev = NonNull::new(dev).ok_or(ENOMEM)?;
>> +
>> +        // SAFETY: `fwctl_register()` expects a valid device from `_fwctl_alloc_device()`.
>> +        let ret = unsafe { bindings::fwctl_register(dev.as_ptr()) };
>
> This is a Bound device, not just any device.

Indeed, the safety comment should mention this. And if it would it could not
justify it with the current code, since the function takes a &Device instead of
the required &Device<Bound> argument.

>> +        if ret != 0 {
>> +            // SAFETY: If registration fails, release the allocated fwctl_device().
>> +            unsafe {
>> +                bindings::put_device(core::ptr::addr_of_mut!((*dev.as_ptr()).dev));
>
> ?? Don't open code fwctl_put() - it should be called directly?
>
>> +            }
>> +            return Err(Error::from_errno(ret));
>> +        }
>> +
>> +        Ok(Self {
>> +            fwctl_dev: dev,
>> +            _marker: PhantomData,
>> +        })
>> +    }
>> +
>> +    fn as_raw(&self) -> *mut bindings::fwctl_device {
>> +        self.fwctl_dev.as_ptr()
>> +    }
>> +}
>> +
>> +impl<T: FwCtlOps> Drop for Registration<T> {
>> +    fn drop(&mut self) {
>> +        // SAFETY: `fwctl_unregister()` expects a valid device from `_fwctl_alloc_device()`.
>
> Incomplete safety statement, the device passed to fwctl_alloc_device must
> still be bound prior to calling fwctl_unregister
>
>> +        unsafe {
>> +            bindings::fwctl_unregister(self.as_raw());
>> +            bindings::put_device(core::ptr::addr_of_mut!((*self.as_raw()).dev));
>
> There for Drop can only do fwctl_put() since otherwise there is no way
> to guarantee a Bound device.
>
> unregister has to happen before remove() completes, Danilo had some
> approach to this I think he told me?

Yeah, such Registration structures of (class) devices should be wrapped into a
Devres container (i.e. Devres<fwctl::Registration>)to be able to provide this
guarantee. See also my other reply to this patch [1].

While not a class device, the auxiliary bus with its auxiliary::Registration
[2], is a good example.

Alternatively (or additionally), it can also be implemented in a way that the
driver does not get control over a Registration object at all, but once created
it is not accessible anymore and automatically dropped on parent device unbind.
This approach is used by cpufreq [3].

It always depends on whether a driver might want to drop the Registration
manually before device unbind.

[1] https://lore.kernel.org/lkml/DDVT5YA564C6.3HN9WCMQX49PC@kernel.org/
[2] https://git.kernel.org/pub/scm/linux/kernel/git/driver-core/driver-core.git/tree/rust/kernel/auxiliary.rs?id=b0b7301b004301afe920b3d08caa6171dd3f4011#n304
[3] https://rust.docs.kernel.org/kernel/cpufreq/struct.Registration.html#method.new_foreign_owned

Re: [RFC 1/2] rust: introduce abstractions for fwctl

[Danilo Krummrich]

On Thu Oct 30, 2025 at 6:19 PM CET, Zhi Wang wrote:
> fwctl_put() is a inline function and it seem opened by the compiler when bindings are
> generated. 
>
> $ grep -R fwctl_put *
> $ pwd
> /home/inno/drm-rust/rust/bindings
> $ grep -R put_device *
> bindings_generated.rs:    pub fn put_device(dev: *mut device);
>
> Hehe. I am open to options.

You can add them to rust/helpers/.

Re: [RFC 0/2] rust: introduce abstractions for fwctl

[Zhi Wang]

On 30.10.2025 18.03, Zhi Wang wrote:
> In the NVIDIA vGPU RFC [1], the vGPU type blobs must be provided to the GSP
> before userspace can enumerate available vGPU types and create vGPU
> instances. The original design relied on the firmware loading interface,
> but fwctl is a more natural fit for this use case, as it is designed for
> uploading configuration or firmware data required before the device becomes
> operational.
> 

Hi Jason and Danilo:

Thanks for the comments. I had one more open to discuss, handling the buffer
allocation/free between rust and C world.

Two fwctl ioctls:

FWCTL_CMD_INFO: The driver allocates the info memory (kmalloc) and the fwctl
subsystem frees it.

FWCTL_RPC:

Case 1: The driver can choose to re-use the input buffer and write the *out_len
for actual length of data.

Case 2: The driver can allocate a new buffer (kmalloc) and the fwctl subsystem
frees it.

----
Now with the Rust driver:

FWCTL_CMD_INFO: The rust side returns a new KVec, the rust fwctl abstraction
consumes it, get void *buf and pass it to fwctl subsystem (C). The memory
will be freed by C side.

FWCTL_RPC:

The input buffer will be wrapped in a mutable slice.

Case 1: Re-use the input buffer. The rust side writes the mut slice and the
* mut out_len.

Case 2: Allocate the new output buffer. The same approach as FWCTL_CMD_INFO.

----

We know KVec is backed by kmalloc. If C side changes the requirements of
the driver memory allocation someday, E.g. from kfree() to kvfree() or vfree().

Drivers in C will be updated surely at that time.

Is possible that we can have some approaches to catch that change from the rust
side via rust compiler for rust drivers?

Z.

> This patch introduces a Rust abstraction over the fwctl subsystem,
> providing safe and idiomatic bindings.
> 
> The new `fwctl` module allows Rust drivers to integrate with the existing
> C-side fwctl core through a typed trait interface. It provides:
> 
>   - `FwCtlOps` trait — defines driver-specific operations such as
>     `open_uctx()`, `close_uctx()`, `info()`, and `fw_rpc()`.  
>     Each Rust driver implements this trait to describe its own per-FD
>     user-context behavior and RPC handling.
> 
>   - `FwCtlUCtx<T>` — a generic wrapper around `struct fwctl_uctx`
>     embedding driver-specific context data, providing safe conversion
>     from raw C pointers and access to the parent device.
> 
>   - `Registration<T>` — safe registration and automatic unregistration
>     of `struct fwctl_device` objects using the kernel’s device model.
> 
>   - `FwCtlVTable<T>` — a static vtable bridging C callbacks and Rust
>     trait methods, ensuring type safety across the FFI boundary.
> 
> `rust/kernel/lib.rs` is updated to conditionally include this module
> under `CONFIG_FWCTL`.
> 
> [1] https://lore.kernel.org/all/20250903221111.3866249-1-zhiw@nvidia.com/
> 
> Zhi Wang (2):
>   rust: introduce abstractions for fwctl
>   samples: rust: fwctl: add sample code for FwCtl
> 
>  include/uapi/fwctl/fwctl.h        |   1 +
>  rust/bindings/bindings_helper.h   |   1 +
>  rust/kernel/fwctl.rs              | 254 ++++++++++++++++++++++++++++++
>  rust/kernel/lib.rs                |   2 +
>  samples/rust/Kconfig              |  11 ++
>  samples/rust/Makefile             |   1 +
>  samples/rust/rust_driver_fwctl.rs | 123 +++++++++++++++
>  7 files changed, 393 insertions(+)
>  create mode 100644 rust/kernel/fwctl.rs
>  create mode 100644 samples/rust/rust_driver_fwctl.rs
> 

Re: [RFC 0/2] rust: introduce abstractions for fwctl

[Danilo Krummrich]

On Thu Oct 30, 2025 at 6:29 PM CET, Zhi Wang wrote:
> On 30.10.2025 18.03, Zhi Wang wrote:
>> In the NVIDIA vGPU RFC [1], the vGPU type blobs must be provided to the GSP
>> before userspace can enumerate available vGPU types and create vGPU
>> instances. The original design relied on the firmware loading interface,
>> but fwctl is a more natural fit for this use case, as it is designed for
>> uploading configuration or firmware data required before the device becomes
>> operational.
>> 
>
> Hi Jason and Danilo:
>
> Thanks for the comments. I had one more open to discuss, handling the buffer
> allocation/free between rust and C world.
>
> Two fwctl ioctls:
>
> FWCTL_CMD_INFO: The driver allocates the info memory (kmalloc) and the fwctl
> subsystem frees it.
>
> FWCTL_RPC:
>
> Case 1: The driver can choose to re-use the input buffer and write the *out_len
> for actual length of data.
>
> Case 2: The driver can allocate a new buffer (kmalloc) and the fwctl subsystem
> frees it.
>
> ----
> Now with the Rust driver:
>
> FWCTL_CMD_INFO: The rust side returns a new KVec, the rust fwctl abstraction
> consumes it, get void *buf and pass it to fwctl subsystem (C). The memory
> will be freed by C side.
>
> FWCTL_RPC:
>
> The input buffer will be wrapped in a mutable slice.
>
> Case 1: Re-use the input buffer. The rust side writes the mut slice and the
> * mut out_len.
>
> Case 2: Allocate the new output buffer. The same approach as FWCTL_CMD_INFO.
>
> ----
>
> We know KVec is backed by kmalloc. If C side changes the requirements of
> the driver memory allocation someday, E.g. from kfree() to kvfree() or vfree().
>
> Drivers in C will be updated surely at that time.
>
> Is possible that we can have some approaches to catch that change from the rust
> side via rust compiler for rust drivers?

I don't think we have the possibility of doing any compile time check here,
since on the C side the type is always void * for any memory allocation.

However, I think the only broken case would be if C switches to vmalloc() (and
hence vfree()), but the Rust code doesn't. That sounds unlikely to me for three
reasons.

  (1) I think if there'd be a change it would be to kvmalloc() and calling
      kvfree() on a kmalloc() buffer should be fine.

  (2) A breaking change would also affect all C drivers, so it'd not only be the
      Rust code being affected.

Re: [RFC 0/2] rust: introduce abstractions for fwctl

[Jason Gunthorpe]

On Thu, Oct 30, 2025 at 06:52:31PM +0100, Danilo Krummrich wrote:
>   (1) I think if there'd be a change it would be to kvmalloc() and calling
>       kvfree() on a kmalloc() buffer should be fine.

Yes

>   (2) A breaking change would also affect all C drivers, so it'd not only be the
>       Rust code being affected.

Yes

Jason

Re: [RFC 1/2] rust: introduce abstractions for fwctl

[Danilo Krummrich]

Hi Zhi,

Additional to my other comments, some thoughts about naming.

On Thu Oct 30, 2025 at 5:03 PM CET, Zhi Wang wrote:
> +/// Trait implemented by each Rust driver that integrates with the fwctl subsystem.
> +///
> +/// Each implementation corresponds to a specific device type and provides
> +/// the vtable used by the core `fwctl` layer to manage per-FD user contexts
> +/// and handle RPC requests.
> +pub trait FwCtlOps: Sized {

Up to Jason, but I usually recommend to take the Rust module name into account,
i.e a user of the API can refer to this as fwctl::FwCtlOps.

Hence, I suggest to name this Ops or even Operations, such that users can refer
to this as fwctl::Ops or fwctl::Operations.

It would also be more consistent with existing code, e.g. pci::Device,
platform::Device, etc., but also the fwctl::Registration from this patch. :)

> +    /// Driver UCtx type.
> +    type UCtx;

I think we can affort to be a bit more verbose here, maybe just spell it out as
UserContext.

> +    /// fwctl device type, matching the C enum `fwctl_device_type`.
> +    const DEVICE_TYPE: u32;

I think it would make sense to have a new (Rust enum) type (fwctl::DeviceType)
for this, rather than using a raw u32, see [1] for reference.

[1] https://lore.kernel.org/all/20250828133323.53311-2-dakr@kernel.org/

> +/// Represents a per-FD user context (`struct fwctl_uctx`).
> +///
> +/// Each driver embeds `struct fwctl_uctx` as the first field of its own
> +/// context type and uses this wrapper to access driver-specific data.
> +#[repr(C)]
> +#[pin_data]
> +pub struct FwCtlUCtx<T> {

What about UserContext<T>? A driver would refer to this as fwctl::UserContext.

Re: [RFC 1/2] rust: introduce abstractions for fwctl

[Danilo Krummrich]

On Thu Oct 30, 2025 at 5:03 PM CET, Zhi Wang wrote:
> +/// Static vtable mapping Rust trait methods to C callbacks.
> +pub struct FwCtlVTable<T: FwCtlOps>(PhantomData<T>);
> +
> +impl<T: FwCtlOps> FwCtlVTable<T> {
> +    /// Static instance of `fwctl_ops` used by the C core to call into Rust.
> +    pub const VTABLE: bindings::fwctl_ops = bindings::fwctl_ops {
> +        device_type: T::DEVICE_TYPE,
> +        uctx_size: core::mem::size_of::<FwCtlUCtx<T::UCtx>>(),

The fwctl code uses this size to allocate memory for both, struct fwctl_uctx and
the driver's private data at the end of the allocation.

This means that it is not enough to just consider the size of T::UCtx, you also
have to consider its required alignment, and, if required, allocate more memory.

> +        open_uctx: Some(Self::open_uctx_callback),
> +        close_uctx: Some(Self::close_uctx_callback),
> +        info: Some(Self::info_callback),
> +        fw_rpc: Some(Self::fw_rpc_callback),
> +    };
> +
> +    /// Called when a new user context is opened by userspace.
> +    unsafe extern "C" fn open_uctx_callback(uctx: *mut bindings::fwctl_uctx) -> ffi::c_int {
> +        // SAFETY: `uctx` is guaranteed by the fwctl subsystem to be a valid pointer.
> +        let ctx = unsafe { FwCtlUCtx::<T::UCtx>::from_raw(uctx) };

Considering the above, this is incorrect for two reasons.

  (1) FwCtlUCtx::uctx might not be aligned correctly.

  (2) FwCtlUCtx::uctx is not initialized, hence creating a reference might be
      undefined behavior.

I think the correct way to fix (2) is to only provide an abstraction of struct
fwctl_uctx as argument to T::open_uctx() and let T::open_uctx() return an
initializer for FwCtlUCtx::uctx, i.e. impl PinInit<T::UCtx, Error>.

All other callbacks should be correct as they are once the alignment is
considered.

> +        match T::open_uctx(ctx) {
> +            Ok(()) => 0,
> +            Err(e) => e.to_errno(),
> +        }
> +    }

Re: [RFC 1/2] rust: introduce abstractions for fwctl

[Jason Gunthorpe]

On Sun, Nov 02, 2025 at 06:26:58PM +0100, Danilo Krummrich wrote:
> Hi Zhi,
> 
> Additional to my other comments, some thoughts about naming.
> 
> On Thu Oct 30, 2025 at 5:03 PM CET, Zhi Wang wrote:
> > +/// Trait implemented by each Rust driver that integrates with the fwctl subsystem.
> > +///
> > +/// Each implementation corresponds to a specific device type and provides
> > +/// the vtable used by the core `fwctl` layer to manage per-FD user contexts
> > +/// and handle RPC requests.
> > +pub trait FwCtlOps: Sized {
> 
> Up to Jason, but I usually recommend to take the Rust module name into account,
> i.e a user of the API can refer to this as fwctl::FwCtlOps.

I don't have any knowledge to have a preference, I trust you to guide
Zhi to whatever is the most common and appropriate thing here..

Thanks,
Jason

Re: [RFC 1/2] rust: introduce abstractions for fwctl

[Jason Gunthorpe]

On Sun, Nov 02, 2025 at 07:33:10PM +0100, Danilo Krummrich wrote:
> On Thu Oct 30, 2025 at 5:03 PM CET, Zhi Wang wrote:
> > +/// Static vtable mapping Rust trait methods to C callbacks.
> > +pub struct FwCtlVTable<T: FwCtlOps>(PhantomData<T>);
> > +
> > +impl<T: FwCtlOps> FwCtlVTable<T> {
> > +    /// Static instance of `fwctl_ops` used by the C core to call into Rust.
> > +    pub const VTABLE: bindings::fwctl_ops = bindings::fwctl_ops {
> > +        device_type: T::DEVICE_TYPE,
> > +        uctx_size: core::mem::size_of::<FwCtlUCtx<T::UCtx>>(),
> 
> The fwctl code uses this size to allocate memory for both, struct fwctl_uctx and
> the driver's private data at the end of the allocation.
> 
> This means that it is not enough to just consider the size of T::UCtx, you also
> have to consider its required alignment, and, if required, allocate more memory.

Yes, the container_of() relationship must be such that the core struct
is first in the memory and any driver data is trailing. The C version
has a static_assertion to constrain this.

> All other callbacks should be correct as they are once the alignment is
> considered.

Yes, only alloc and put/free make this assumption.

Jason

Re: [RFC 1/2] rust: introduce abstractions for fwctl

[Zhi Wang]

On Sun, 02 Nov 2025 19:33:10 +0100
"Danilo Krummrich" <dakr@kernel.org> wrote:

> On Thu Oct 30, 2025 at 5:03 PM CET, Zhi Wang wrote:
> > +/// Static vtable mapping Rust trait methods to C callbacks.
> > +pub struct FwCtlVTable<T: FwCtlOps>(PhantomData<T>);
> > +
> > +impl<T: FwCtlOps> FwCtlVTable<T> {
> > +    /// Static instance of `fwctl_ops` used by the C core to call
> > into Rust.
> > +    pub const VTABLE: bindings::fwctl_ops = bindings::fwctl_ops {
> > +        device_type: T::DEVICE_TYPE,
> > +        uctx_size: core::mem::size_of::<FwCtlUCtx<T::UCtx>>(),
> 
> The fwctl code uses this size to allocate memory for both, struct
> fwctl_uctx and the driver's private data at the end of the allocation.
> 
> This means that it is not enough to just consider the size of
> T::UCtx, you also have to consider its required alignment, and, if
> required, allocate more memory.
> 

FwCtlUCtx is defined as below:

+#[repr(C)]
+#[pin_data]
+pub struct FwCtlUCtx<T> {
+    /// The core fwctl user context shared with the C implementation.
+    #[pin]
+    pub fwctl_uctx: bindings::fwctl_uctx,
+    /// Driver-specific data associated with this user context.
+    pub uctx: T,
+}

I assume it should be equal to C structure as below and with #[repr(C)],
the handling of layout and the alignment should be as the same as C
structure.

struct FwCtlUCtx {
	struct fwctl_uctx base;
	struct my_driver_data data;
};

uctx_size: core::mem::size_of::<FwCtlUCtx<T::UCtx>>() should be:

sizeof(FWCtlUCtx).

Do we need anything extra for alignment? Or some parts of the flow
doesn't respect the #[repr(C)]?

> > +        open_uctx: Some(Self::open_uctx_callback),
> > +        close_uctx: Some(Self::close_uctx_callback),
> > +        info: Some(Self::info_callback),
> > +        fw_rpc: Some(Self::fw_rpc_callback),
> > +    };
> > +
> > +    /// Called when a new user context is opened by userspace.
> > +    unsafe extern "C" fn open_uctx_callback(uctx: *mut
> > bindings::fwctl_uctx) -> ffi::c_int {
> > +        // SAFETY: `uctx` is guaranteed by the fwctl subsystem to
> > be a valid pointer.
> > +        let ctx = unsafe { FwCtlUCtx::<T::UCtx>::from_raw(uctx) };
> 
> Considering the above, this is incorrect for two reasons.
> 
>   (1) FwCtlUCtx::uctx might not be aligned correctly.
> 
>   (2) FwCtlUCtx::uctx is not initialized, hence creating a reference
> might be undefined behavior.
> 
> I think the correct way to fix (2) is to only provide an abstraction
> of struct fwctl_uctx as argument to T::open_uctx() and let
> T::open_uctx() return an initializer for FwCtlUCtx::uctx, i.e. impl
> PinInit<T::UCtx, Error>.
> 

Nice catch. With this approach, we can force the driver user context to
be explicitly intialized and start to be tracked in the safe world.

> All other callbacks should be correct as they are once the alignment
> is considered.
> 
> > +        match T::open_uctx(ctx) {
> > +            Ok(()) => 0,
> > +            Err(e) => e.to_errno(),
> > +        }
> > +    }

Re: [RFC 1/2] rust: introduce abstractions for fwctl

[Danilo Krummrich]

On Mon Nov 3, 2025 at 10:55 AM CET, Zhi Wang wrote:
> On Sun, 02 Nov 2025 19:33:10 +0100
> "Danilo Krummrich" <dakr@kernel.org> wrote:
>
>> On Thu Oct 30, 2025 at 5:03 PM CET, Zhi Wang wrote:
>> > +/// Static vtable mapping Rust trait methods to C callbacks.
>> > +pub struct FwCtlVTable<T: FwCtlOps>(PhantomData<T>);
>> > +
>> > +impl<T: FwCtlOps> FwCtlVTable<T> {
>> > +    /// Static instance of `fwctl_ops` used by the C core to call
>> > into Rust.
>> > +    pub const VTABLE: bindings::fwctl_ops = bindings::fwctl_ops {
>> > +        device_type: T::DEVICE_TYPE,
>> > +        uctx_size: core::mem::size_of::<FwCtlUCtx<T::UCtx>>(),
>> 
>> The fwctl code uses this size to allocate memory for both, struct
>> fwctl_uctx and the driver's private data at the end of the allocation.
>> 
>> This means that it is not enough to just consider the size of
>> T::UCtx, you also have to consider its required alignment, and, if
>> required, allocate more memory.
>> 
>
> FwCtlUCtx is defined as below:
>
> +#[repr(C)]
> +#[pin_data]
> +pub struct FwCtlUCtx<T> {
> +    /// The core fwctl user context shared with the C implementation.
> +    #[pin]
> +    pub fwctl_uctx: bindings::fwctl_uctx,
> +    /// Driver-specific data associated with this user context.
> +    pub uctx: T,
> +}
>
> I assume it should be equal to C structure as below and with #[repr(C)],
> the handling of layout and the alignment should be as the same as C
> structure.
>
> struct FwCtlUCtx {
> 	struct fwctl_uctx base;
> 	struct my_driver_data data;
> };
>
> uctx_size: core::mem::size_of::<FwCtlUCtx<T::UCtx>>() should be:

That's indeed correct then, I think I read uctx_size as the size of T::UCtx
only. (I've recently come across subsystems that do exacly that and hence run
into the problem in [2].)

Anyways, I suggest to not give out a FwCtlUCtx<T> to the driver at all, since in
open() we can't (for the discussed reasons) and in the other callbacks it
doesn't seem very useful either.

Instead, I think we should have the following callback arguments:

	impl fwctl::Operations for MyDriverContext {
	    fn open(
	        fdev: &fwctl::Device,
	        parent: &Device<Bound>
	    ) -> impl PinInit<Self, Error>;

	    fn close(
	        this: Pin<&mut Self>,
	        fdev: &fwctl::Device,
	        parent: &Device<Bound>
	    ) -> impl PinInit<Self, Error>;
	}

with

	#[repr(transparent)]
	struct Device(Opaque<bindings::fwctl_device);

Note this also gets us rid of the Self::UCtx type alias, which seems
unnecessary.

You could still keep a FwCtlUCtx<T> type internally since it might make the
implementation easier.

[2] https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=806c1a9a53a174ef39acff8ae5bb0e68