Re: [PATCH 2/2] gpu: nova-core: add fwctl driver for firmware control interface

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

On Thu Mar 5, 2026 at 8:09 PM CET, Zhi Wang wrote:
> +use kernel::{
> +    fwctl::{
> +        self,
> +        DeviceType,
> +        FwRpcResponse,
> +        Operations,
> +        RpcScope, //
> +    },
> +    prelude::*,
> +    transmute::{AsBytes, FromBytes},

NIT: Formatting.

> +    uapi, //
> +};
> +
> +use crate::{
> +    driver::NovaCore,
> +    gsp::{
> +        RmControlMsgFunction,
> +        rm::commands::send_rm_control, //
> +    },
> +};
> +
> +/// Byte-serializable wrapper for [`uapi::fwctl_rpc_nova_core_request_hdr`].
> +#[repr(transparent)]
> +struct FwctlNovaCoreReqHdr(uapi::fwctl_rpc_nova_core_request_hdr);
> +
> +// SAFETY: All fields are plain `__u32` with no padding.
> +unsafe impl FromBytes for FwctlNovaCoreReqHdr {}
> +
> +/// Byte-serializable wrapper for [`uapi::fwctl_rpc_nova_core_resp_hdr`].
> +#[repr(transparent)]
> +struct FwctlNovaCoreRespHdr(uapi::fwctl_rpc_nova_core_resp_hdr);
> +
> +// SAFETY: All fields are plain `__u32` with no padding.
> +unsafe impl AsBytes for FwctlNovaCoreRespHdr {}
> +
> +/// Per-FD fwctl user context and operations for nova-core.
> +pub(crate) struct NovaCoreFwCtl;
> +
> +impl Operations for NovaCoreFwCtl {
> +    type DeviceData = ();
> +
> +    const DEVICE_TYPE: DeviceType = DeviceType::NovaCore;
> +
> +    fn open(_device: &fwctl::Device<Self>) -> Result<impl PinInit<Self, Error>, Error> {
> +        Ok(Ok(NovaCoreFwCtl))
> +    }
> +
> +    fn fw_rpc(
> +        _this: &Self,
> +        device: &fwctl::Device<Self>,
> +        scope: RpcScope,
> +        rpc_in: &mut [u8],
> +    ) -> Result<FwRpcResponse, Error> {

Result<FwRpcResponse>

> +        let hdr_size = size_of::<FwctlNovaCoreReqHdr>();
> +
> +        if rpc_in.len() < hdr_size {
> +            return Err(EINVAL);
> +        }
> +
> +        if scope != RpcScope::Configuration {
> +            return Err(EPERM);
> +        }
> +
> +        let (hdr, _) = FwctlNovaCoreReqHdr::from_bytes_prefix(rpc_in).ok_or(EINVAL)?;
> +        let cmd = hdr.0.cmd;
> +
> +        let rm_cmd = match cmd {
> +            uapi::fwctl_cmd_nova_core_FWCTL_CMD_NOVA_CORE_UPLOAD_VGPU_TYPE => {
> +                RmControlMsgFunction::VgpuMgrInternalPgpuAddVgpuType
> +            }
> +            _ => return Err(EINVAL),
> +        };
> +
> +        let parent = device.parent();
> +        let data = parent.drvdata::<NovaCore>()?;

Please don't use drvdata() for this, it is not what it is intended for and I
kinda regret a bit that I added it in the first place.

Instead, please use your fwctl::Device private data, i.e. Self.

> +        let bar = data.gpu.bar.as_ref().access(parent)?;
> +
> +        let params = &rpc_in[hdr_size..];
> +        let reply_params = send_rm_control(
> +            &data.gpu.gsp.cmdq,
> +            bar,
> +            data.gpu.gsp.h_client,
> +            data.gpu.gsp.h_subdevice,
> +            rm_cmd,
> +            params,
> +        )?;
> +
> +        let resp_hdr = FwctlNovaCoreRespHdr(uapi::fwctl_rpc_nova_core_resp_hdr {
> +            mctp_header: 0,
> +            nvdm_header: 0,
> +        });
> +        let mut out = KVec::new();
> +        out.extend_from_slice(resp_hdr.as_bytes(), GFP_KERNEL)?;
> +        out.extend_from_slice(&reply_params, GFP_KERNEL)?;
> +        Ok(FwRpcResponse::NewBuffer(out))
> +    }
> +}
> diff --git a/drivers/gpu/nova-core/gpu.rs b/drivers/gpu/nova-core/gpu.rs
> index 60c85fffaeaf..7965ce37eb08 100644
> --- a/drivers/gpu/nova-core/gpu.rs
> +++ b/drivers/gpu/nova-core/gpu.rs
> @@ -241,7 +241,7 @@ fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
>  pub(crate) struct Gpu {
>      spec: Spec,
>      /// MMIO mapping of PCI BAR 0
> -    bar: Arc<Devres<Bar0>>,
> +    pub(crate) bar: Arc<Devres<Bar0>>,
>      /// System memory page required for flushing all pending GPU-side memory writes done through
>      /// PCIE into system memory, via sysmembar (A GPU-initiated HW memory-barrier operation).
>      sysmem_flush: SysmemFlush,
> @@ -251,7 +251,7 @@ pub(crate) struct Gpu {
>      sec2_falcon: Falcon<Sec2Falcon>,
>      /// GSP runtime data. Temporarily an empty placeholder.
>      #[pin]
> -    gsp: Gsp,
> +    pub(crate) gsp: Gsp,
>  }
>  
>  impl Gpu {
> diff --git a/drivers/gpu/nova-core/gsp.rs b/drivers/gpu/nova-core/gsp.rs
> index 1a1c4e9808ac..77eb30010c2f 100644
> --- a/drivers/gpu/nova-core/gsp.rs
> +++ b/drivers/gpu/nova-core/gsp.rs
> @@ -4,11 +4,13 @@
>  
>  use kernel::{
>      device,
> +    devres::Devres,
>      dma::{
>          CoherentAllocation,
>          DmaAddress, //
>      },
>      dma_write,
> +    fwctl,
>      pci,
>      prelude::*,
>      transmute::AsBytes, //
> @@ -21,15 +23,19 @@
>  mod sequencer;
>  
>  pub(crate) use fw::{
> +    rm::RmControlMsgFunction,
>      GspFwWprMeta,
>      LibosParams, //
>  };
>  
>  use crate::{
> -    gsp::cmdq::Cmdq,
> -    gsp::fw::{
> -        GspArgumentsPadded,
> -        LibosMemoryRegionInitArgument, //
> +    fwctl::NovaCoreFwCtl,
> +    gsp::{
> +        cmdq::Cmdq,
> +        fw::{
> +            GspArgumentsPadded,
> +            LibosMemoryRegionInitArgument, //
> +        },
>      },
>      num,
>  };
> @@ -117,6 +123,12 @@ pub(crate) struct Gsp {
>      pub(crate) cmdq: Cmdq,
>      /// RM arguments.
>      rmargs: CoherentAllocation<GspArgumentsPadded>,
> +    /// Cached RM internal client handle from GSP static info.
> +    pub(crate) h_client: u32,
> +    /// Cached RM internal subdevice handle from GSP static info.
> +    pub(crate) h_subdevice: u32,
> +    /// fwctl registration for userspace RM control.
> +    fwctl: Pin<KBox<Devres<fwctl::Registration<NovaCoreFwCtl>>>>,

This does not need to be in a separate allocation.

>  }
>  
>  impl Gsp {
> @@ -125,6 +137,8 @@ pub(crate) fn new(pdev: &pci::Device<device::Bound>) -> impl PinInit<Self, Error
>          pin_init::pin_init_scope(move || {
>              let dev = pdev.as_ref();
>  
> +            let fwctl_dev = fwctl::Device::<NovaCoreFwCtl>::new(pdev.as_ref(), Ok(()))?;

You should store everything you need in the private data of your fwctl::Device
to properly tie it to its lifetime, e.g.

	struct NovaCoreFwCtl {
		gpu: Arc<Gpu>,
		bar: Arc<Devres<Bar0>>,
	}

> +
>              Ok(try_pin_init!(Self {
>                  libos: CoherentAllocation::<LibosMemoryRegionInitArgument>::alloc_coherent(
>                      dev,
> @@ -140,6 +154,9 @@ pub(crate) fn new(pdev: &pci::Device<device::Bound>) -> impl PinInit<Self, Error
>                      1,
>                      GFP_KERNEL | __GFP_ZERO,
>                  )?,
> +                h_client: 0,
> +                h_subdevice: 0,
> +                fwctl: KBox::pin_init(fwctl::Registration::new(pdev.as_ref(), &fwctl_dev), GFP_KERNEL)?,

I don't think this should live in the Gsp structure, please move this to probe()
and store the registration in struct NovaCore after the Gpu structure.

>                  _: {
>                      // Initialise the logging structures. The OpenRM equivalents are in:
>                      // _kgspInitLibosLoggingStructures (allocates memory for buffers)
> diff --git a/drivers/gpu/nova-core/gsp/boot.rs b/drivers/gpu/nova-core/gsp/boot.rs
> index bc53e667cd9e..f493546b78ff 100644
> --- a/drivers/gpu/nova-core/gsp/boot.rs
> +++ b/drivers/gpu/nova-core/gsp/boot.rs
> @@ -128,7 +128,7 @@ fn run_fwsec_frts(
>      ///
>      /// Upon return, the GSP is up and running, and its runtime object given as return value.
>      pub(crate) fn boot(
> -        self: Pin<&mut Self>,
> +        mut self: Pin<&mut Self>,
>          pdev: &pci::Device<device::Bound>,
>          bar: &Bar0,
>          chipset: Chipset,
> @@ -221,6 +221,10 @@ pub(crate) fn boot(
>  
>          // Obtain and display basic GPU information.
>          let info = commands::get_gsp_info(&self.cmdq, bar)?;
> +        // SAFETY: h_client and h_subdevice are not structurally pinned.
> +        let this = unsafe { self.as_mut().get_unchecked_mut() };
> +        this.h_client = info.h_client();
> +        this.h_subdevice = info.h_subdevice();

Ick! If this is only needed from the fwctl callbacks, just move those into your
fwctl::Device private data.

But I think this is needed in other cases as well. So, more generally I think
Gsp::boot() should just be part of the constructor, as we won't be able to
properly construct a Gsp object without having a chance to interact with the Gsp
before.

>  /// Sends an RM control command, checks the reply status, and returns the raw parameter bytes.
> -fn send_rm_control(
> +pub(crate) fn send_rm_control(
>      cmdq: &Cmdq,
>      bar: &Bar0,
>      h_client: u32,
> @@ -106,7 +106,7 @@ fn send_rm_control(
>      cmd: RmControlMsgFunction,
>      params: &[u8],
>  ) -> Result<KVVec<u8>> {
> -    let reply = cmdq.send_sync_command(bar, RmControl::new(h_client, h_object, cmd, params))?;
> +    let reply = cmdq.send_command(bar, RmControl::new(h_client, h_object, cmd, params))?;

This looks like it should be a separate patch, or fixed in one of the
dependencies of this patch.

>  
>      Result::from(reply.status)?;
>  
> diff --git a/drivers/gpu/nova-core/nova_core.rs b/drivers/gpu/nova-core/nova_core.rs
> index b5caf1044697..863dc041272c 100644
> --- a/drivers/gpu/nova-core/nova_core.rs
> +++ b/drivers/gpu/nova-core/nova_core.rs
> @@ -10,6 +10,7 @@
>  mod falcon;
>  mod fb;
>  mod firmware;
> +mod fwctl;
>  mod gfw;
>  mod gpu;
>  mod gsp;
> @@ -27,6 +28,7 @@
>      description: "Nova Core GPU driver",
>      license: "GPL v2",
>      firmware: [],
> +    imports_ns: ["FWCTL"],
>  }
>  
>  kernel::module_firmware!(firmware::ModInfoBuilder);
> diff --git a/include/uapi/fwctl/fwctl.h b/include/uapi/fwctl/fwctl.h
> index 716ac0eee42d..f6289fbf3062 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_NOVA_CORE = 5,
>  };
>  
>  /**
> diff --git a/include/uapi/fwctl/nova-core.h b/include/uapi/fwctl/nova-core.h
> new file mode 100644
> index 000000000000..3f1d94b44ec8
> --- /dev/null
> +++ b/include/uapi/fwctl/nova-core.h

Why is this not include/uapi/gpu/nova-core.h? This driver does not live under
drivers/fwctl/.

If we want to separate it, I suggest include/uapi/gpu/nova-core/fwctl.h, but it
doesn't seem necessary as we don't have any other UAPIs.