Message-Id: <DDJJ4Y320YEC.O6QIUDDLFVOH@nvidia.com>
Date: Thu, 16 Oct 2025 15:23:57 +0900
From: "Alexandre Courbot" <acourbot@...dia.com>
To: "Alistair Popple" <apopple@...dia.com>,
 <rust-for-linux@...r.kernel.org>, <dri-devel@...ts.freedesktop.org>,
 <dakr@...nel.org>, <acourbot@...dia.com>
Cc: "Miguel Ojeda" <ojeda@...nel.org>, "Alex Gaynor"
 <alex.gaynor@...il.com>, "Boqun Feng" <boqun.feng@...il.com>, "Gary Guo"
 <gary@...yguo.net>, Björn Roy Baron
 <bjorn3_gh@...tonmail.com>, "Benno Lossin" <lossin@...nel.org>, "Andreas
 Hindborg" <a.hindborg@...nel.org>, "Alice Ryhl" <aliceryhl@...gle.com>,
 "Trevor Gross" <tmgross@...ch.edu>, "David Airlie" <airlied@...il.com>,
 "Simona Vetter" <simona@...ll.ch>, "Maarten Lankhorst"
 <maarten.lankhorst@...ux.intel.com>, "Maxime Ripard" <mripard@...nel.org>,
 "Thomas Zimmermann" <tzimmermann@...e.de>, "John Hubbard"
 <jhubbard@...dia.com>, "Joel Fernandes" <joelagnelf@...dia.com>, "Timur
 Tabi" <ttabi@...dia.com>, <linux-kernel@...r.kernel.org>,
 <nouveau@...ts.freedesktop.org>
Subject: Re: [PATCH v5 06/14] gpu: nova-core: Add GSP command queue bindings

On Mon Oct 13, 2025 at 3:20 PM JST, Alistair Popple wrote:
> Add bindings and accessors used for the GSP command queue.
>
> Signed-off-by: Alistair Popple <apopple@...dia.com>

There are lots of unused warnings when building on this commit. I know
we don't like big patches, but since this is semantically related to the
next one and the two touch different files anyway, how about merging
them? IMHO this is preferable to adding lots of `#[expect(unused)]` that
we are going to remove right after.

If you have been told to split in a previous version, let's just add a
the `#[expect(unused)]`  where needed.

>
> ---
>
> Changes for v5:
>  - Derive the Zeroable trait for structs and enums
>
> Changes for v4:
>  - Don't panic the kernel if trying to initialise a large (> 4GB)
>    message header.
>  - Use `init!` to provide safe and complete initialisers.
>  - Take an enum type instead of a u32 for the function.
>
> Changes for v3:
>  - New for v3
> ---
>  drivers/gpu/nova-core/gsp/fw.rs               | 275 +++++++++
>  .../gpu/nova-core/gsp/fw/r570_144/bindings.rs | 541 ++++++++++++++++++
>  2 files changed, 816 insertions(+)
>
> diff --git a/drivers/gpu/nova-core/gsp/fw.rs b/drivers/gpu/nova-core/gsp/fw.rs
> index 1cc992ca492c..a2ce570ddfaf 100644
> --- a/drivers/gpu/nova-core/gsp/fw.rs
> +++ b/drivers/gpu/nova-core/gsp/fw.rs
> @@ -5,6 +5,7 @@
>  // Alias to avoid repeating the version number with every use.
>  use r570_144 as bindings;
>  
> +use core::fmt;
>  use core::ops::Range;
>  
>  use kernel::dma::CoherentAllocation;
> @@ -16,6 +17,7 @@
>  use crate::firmware::gsp::GspFirmware;
>  use crate::gpu::Chipset;
>  use crate::gsp::FbLayout;
> +use crate::gsp::GSP_PAGE_SIZE;
>  
>  /// Dummy type to group methods related to heap parameters for running the GSP firmware.
>  pub(crate) struct GspFwHeapParams(());
> @@ -158,6 +160,120 @@ pub(crate) fn new(gsp_firmware: &GspFirmware, fb_layout: &FbLayout) -> Self {
>      }
>  }
>  
> +#[derive(PartialEq)]
> +pub(crate) enum MsgFunction {
> +    // Common function codes
> +    Nop = bindings::NV_VGPU_MSG_FUNCTION_NOP as isize,
> +    SetGuestSystemInfo = bindings::NV_VGPU_MSG_FUNCTION_SET_GUEST_SYSTEM_INFO as isize,
> +    AllocRoot = bindings::NV_VGPU_MSG_FUNCTION_ALLOC_ROOT as isize,
> +    AllocDevice = bindings::NV_VGPU_MSG_FUNCTION_ALLOC_DEVICE as isize,
> +    AllocMemory = bindings::NV_VGPU_MSG_FUNCTION_ALLOC_MEMORY as isize,
> +    AllocCtxDma = bindings::NV_VGPU_MSG_FUNCTION_ALLOC_CTX_DMA as isize,
> +    AllocChannelDma = bindings::NV_VGPU_MSG_FUNCTION_ALLOC_CHANNEL_DMA as isize,
> +    MapMemory = bindings::NV_VGPU_MSG_FUNCTION_MAP_MEMORY as isize,
> +    BindCtxDma = bindings::NV_VGPU_MSG_FUNCTION_BIND_CTX_DMA as isize,
> +    AllocObject = bindings::NV_VGPU_MSG_FUNCTION_ALLOC_OBJECT as isize,
> +    Free = bindings::NV_VGPU_MSG_FUNCTION_FREE as isize,
> +    Log = bindings::NV_VGPU_MSG_FUNCTION_LOG as isize,
> +    GetGspStaticInfo = bindings::NV_VGPU_MSG_FUNCTION_GET_GSP_STATIC_INFO as isize,
> +    SetRegistry = bindings::NV_VGPU_MSG_FUNCTION_SET_REGISTRY as isize,
> +    GspSetSystemInfo = bindings::NV_VGPU_MSG_FUNCTION_GSP_SET_SYSTEM_INFO as isize,
> +    GspInitPostObjGpu = bindings::NV_VGPU_MSG_FUNCTION_GSP_INIT_POST_OBJGPU as isize,
> +    GspRmControl = bindings::NV_VGPU_MSG_FUNCTION_GSP_RM_CONTROL as isize,
> +    GetStaticInfo = bindings::NV_VGPU_MSG_FUNCTION_GET_STATIC_INFO as isize,
> +
> +    // Event codes
> +    GspInitDone = bindings::NV_VGPU_MSG_EVENT_GSP_INIT_DONE as isize,
> +    GspRunCpuSequencer = bindings::NV_VGPU_MSG_EVENT_GSP_RUN_CPU_SEQUENCER as isize,
> +    PostEvent = bindings::NV_VGPU_MSG_EVENT_POST_EVENT as isize,
> +    RcTriggered = bindings::NV_VGPU_MSG_EVENT_RC_TRIGGERED as isize,
> +    MmuFaultQueued = bindings::NV_VGPU_MSG_EVENT_MMU_FAULT_QUEUED as isize,
> +    OsErrorLog = bindings::NV_VGPU_MSG_EVENT_OS_ERROR_LOG as isize,
> +    GspPostNoCat = bindings::NV_VGPU_MSG_EVENT_GSP_POST_NOCAT_RECORD as isize,
> +    GspLockdownNotice = bindings::NV_VGPU_MSG_EVENT_GSP_LOCKDOWN_NOTICE as isize,
> +    UcodeLibOsPrint = bindings::NV_VGPU_MSG_EVENT_UCODE_LIBOS_PRINT as isize,
> +}

Can we split functions and events into distinct types? Their bindings
even have different prefixes.

Also, rather than casting these all as `isize`, I'd suggest making the
type `#[repr(u32)]`. We don't need negative values here.

> +
> +impl fmt::Display for MsgFunction {
> +    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
> +        match self {
> +            // Common function codes
> +            MsgFunction::Nop => write!(f, "NOP"),
> +            MsgFunction::SetGuestSystemInfo => write!(f, "SET_GUEST_SYSTEM_INFO"),
> +            MsgFunction::AllocRoot => write!(f, "ALLOC_ROOT"),
> +            MsgFunction::AllocDevice => write!(f, "ALLOC_DEVICE"),
> +            MsgFunction::AllocMemory => write!(f, "ALLOC_MEMORY"),
> +            MsgFunction::AllocCtxDma => write!(f, "ALLOC_CTX_DMA"),
> +            MsgFunction::AllocChannelDma => write!(f, "ALLOC_CHANNEL_DMA"),
> +            MsgFunction::MapMemory => write!(f, "MAP_MEMORY"),
> +            MsgFunction::BindCtxDma => write!(f, "BIND_CTX_DMA"),
> +            MsgFunction::AllocObject => write!(f, "ALLOC_OBJECT"),
> +            MsgFunction::Free => write!(f, "FREE"),
> +            MsgFunction::Log => write!(f, "LOG"),
> +            MsgFunction::GetGspStaticInfo => write!(f, "GET_GSP_STATIC_INFO"),
> +            MsgFunction::SetRegistry => write!(f, "SET_REGISTRY"),
> +            MsgFunction::GspSetSystemInfo => write!(f, "GSP_SET_SYSTEM_INFO"),
> +            MsgFunction::GspInitPostObjGpu => write!(f, "GSP_INIT_POST_OBJGPU"),
> +            MsgFunction::GspRmControl => write!(f, "GSP_RM_CONTROL"),
> +            MsgFunction::GetStaticInfo => write!(f, "GET_STATIC_INFO"),
> +
> +            // Event codes
> +            MsgFunction::GspInitDone => write!(f, "INIT_DONE"),
> +            MsgFunction::GspRunCpuSequencer => write!(f, "RUN_CPU_SEQUENCER"),
> +            MsgFunction::PostEvent => write!(f, "POST_EVENT"),
> +            MsgFunction::RcTriggered => write!(f, "RC_TRIGGERED"),
> +            MsgFunction::MmuFaultQueued => write!(f, "MMU_FAULT_QUEUED"),
> +            MsgFunction::OsErrorLog => write!(f, "OS_ERROR_LOG"),
> +            MsgFunction::GspPostNoCat => write!(f, "NOCAT"),
> +            MsgFunction::GspLockdownNotice => write!(f, "LOCKDOWN_NOTICE"),
> +            MsgFunction::UcodeLibOsPrint => write!(f, "LIBOS_PRINT"),
> +        }
> +    }
> +}
> +
> +impl TryFrom<u32> for MsgFunction {
> +    type Error = kernel::error::Error;
> +
> +    fn try_from(value: u32) -> Result<MsgFunction> {
> +        match value {
> +            bindings::NV_VGPU_MSG_FUNCTION_NOP => Ok(MsgFunction::Nop),
> +            bindings::NV_VGPU_MSG_FUNCTION_SET_GUEST_SYSTEM_INFO => {
> +                Ok(MsgFunction::SetGuestSystemInfo)
> +            }
> +            bindings::NV_VGPU_MSG_FUNCTION_ALLOC_ROOT => Ok(MsgFunction::AllocRoot),
> +            bindings::NV_VGPU_MSG_FUNCTION_ALLOC_DEVICE => Ok(MsgFunction::AllocDevice),
> +            bindings::NV_VGPU_MSG_FUNCTION_ALLOC_MEMORY => Ok(MsgFunction::AllocMemory),
> +            bindings::NV_VGPU_MSG_FUNCTION_ALLOC_CTX_DMA => Ok(MsgFunction::AllocCtxDma),
> +            bindings::NV_VGPU_MSG_FUNCTION_ALLOC_CHANNEL_DMA => Ok(MsgFunction::AllocChannelDma),
> +            bindings::NV_VGPU_MSG_FUNCTION_MAP_MEMORY => Ok(MsgFunction::MapMemory),
> +            bindings::NV_VGPU_MSG_FUNCTION_BIND_CTX_DMA => Ok(MsgFunction::BindCtxDma),
> +            bindings::NV_VGPU_MSG_FUNCTION_ALLOC_OBJECT => Ok(MsgFunction::AllocObject),
> +            bindings::NV_VGPU_MSG_FUNCTION_FREE => Ok(MsgFunction::Free),
> +            bindings::NV_VGPU_MSG_FUNCTION_LOG => Ok(MsgFunction::Log),
> +            bindings::NV_VGPU_MSG_FUNCTION_GET_GSP_STATIC_INFO => Ok(MsgFunction::GetGspStaticInfo),
> +            bindings::NV_VGPU_MSG_FUNCTION_SET_REGISTRY => Ok(MsgFunction::SetRegistry),
> +            bindings::NV_VGPU_MSG_FUNCTION_GSP_SET_SYSTEM_INFO => Ok(MsgFunction::GspSetSystemInfo),
> +            bindings::NV_VGPU_MSG_FUNCTION_GSP_INIT_POST_OBJGPU => {
> +                Ok(MsgFunction::GspInitPostObjGpu)
> +            }
> +            bindings::NV_VGPU_MSG_FUNCTION_GSP_RM_CONTROL => Ok(MsgFunction::GspRmControl),
> +            bindings::NV_VGPU_MSG_FUNCTION_GET_STATIC_INFO => Ok(MsgFunction::GetStaticInfo),
> +            bindings::NV_VGPU_MSG_EVENT_GSP_INIT_DONE => Ok(MsgFunction::GspInitDone),
> +            bindings::NV_VGPU_MSG_EVENT_GSP_RUN_CPU_SEQUENCER => {
> +                Ok(MsgFunction::GspRunCpuSequencer)
> +            }
> +            bindings::NV_VGPU_MSG_EVENT_POST_EVENT => Ok(MsgFunction::PostEvent),
> +            bindings::NV_VGPU_MSG_EVENT_RC_TRIGGERED => Ok(MsgFunction::RcTriggered),
> +            bindings::NV_VGPU_MSG_EVENT_MMU_FAULT_QUEUED => Ok(MsgFunction::MmuFaultQueued),
> +            bindings::NV_VGPU_MSG_EVENT_OS_ERROR_LOG => Ok(MsgFunction::OsErrorLog),
> +            bindings::NV_VGPU_MSG_EVENT_GSP_POST_NOCAT_RECORD => Ok(MsgFunction::GspPostNoCat),
> +            bindings::NV_VGPU_MSG_EVENT_GSP_LOCKDOWN_NOTICE => Ok(MsgFunction::GspLockdownNotice),
> +            bindings::NV_VGPU_MSG_EVENT_UCODE_LIBOS_PRINT => Ok(MsgFunction::UcodeLibOsPrint),
> +            _ => Err(EINVAL),
> +        }
> +    }
> +}

This is another place where the `TryFrom` macro will shine. :)

> +
>  /// Struct containing the arguments required to pass a memory buffer to the GSP
>  /// for use during initialisation.
>  ///
> @@ -208,3 +324,162 @@ fn id8(name: &str) -> u64 {
>          }))
>      }
>  }
> +
> +#[repr(transparent)]
> +pub(crate) struct MsgqTxHeader(bindings::msgqTxHeader);

Short documentation please (`MsgqRxHeader` provides a template).

> +
> +impl MsgqTxHeader {
> +    pub(crate) fn new(msgq_size: u32, rx_hdr_offset: u32, msg_count: u32) -> Self {
> +        Self(bindings::msgqTxHeader {
> +            version: 0,
> +            size: msgq_size,
> +            msgSize: GSP_PAGE_SIZE as u32,
> +            msgCount: msg_count,
> +            writePtr: 0,
> +            flags: 1,
> +            rxHdrOff: rx_hdr_offset,
> +            entryOff: GSP_PAGE_SIZE as u32,
> +        })
> +    }
> +
> +    pub(crate) fn write_ptr(&self) -> u32 {
> +        let ptr = (&self.0.writePtr) as *const u32;
> +
> +        // SAFETY: This is part of a CoherentAllocation and implements the
> +        // equivalent as what the dma_read! macro would and is therefore safe
> +        // for the same reasons.
> +        unsafe { ptr.read_volatile() }
> +    }
> +
> +    pub(crate) fn set_write_ptr(&mut self, val: u32) {
> +        let ptr = (&mut self.0.writePtr) as *mut u32;
> +
> +        // SAFETY: This is part of a CoherentAllocation and implements the
> +        // equivalent as what the dma_write! macro would and is therefore safe
> +        // for the same reasons.
> +        unsafe { ptr.write_volatile(val) }
> +    }
> +}

These methods should also be succintly documented imho.

> +
> +// SAFETY: Padding is explicit and will not contain uninitialized data.
> +unsafe impl AsBytes for MsgqTxHeader {}
> +
> +/// RX header for setting up a message queue with the GSP.
> +#[repr(transparent)]
> +pub(crate) struct MsgqRxHeader(bindings::msgqRxHeader);
> +
> +impl MsgqRxHeader {
> +    pub(crate) fn new() -> Self {
> +        Self(Default::default())
> +    }
> +
> +    pub(crate) fn read_ptr(&self) -> u32 {
> +        let ptr = (&self.0.readPtr) as *const u32;
> +
> +        // SAFETY: This is part of a CoherentAllocation and implements the
> +        // equivalent as what the dma_read! macro would and is therefore safe
> +        // for the same reasons.
> +        unsafe { ptr.read_volatile() }
> +    }
> +
> +    pub(crate) fn set_read_ptr(&mut self, val: u32) {
> +        let ptr = (&mut self.0.readPtr) as *mut u32;
> +
> +        // SAFETY: This is part of a CoherentAllocation and implements the
> +        // equivalent as what the dma_write! macro would and is therefore safe
> +        // for the same reasons.
> +        unsafe { ptr.write_volatile(val) }
> +    }
> +}

Documentation here as well.

> +
> +// SAFETY: Padding is explicit and will not contain uninitialized data.
> +unsafe impl AsBytes for MsgqRxHeader {}
> +
> +impl bindings::rpc_message_header_v {
> +    pub(crate) fn init(cmd_size: u32, function: MsgFunction) -> impl Init<Self, Error> {

This method can be kept private.

> +        type RpcMessageHeader = bindings::rpc_message_header_v;
> +        try_init!(RpcMessageHeader {
> +            // TODO: magic number
> +            header_version: 0x03000000,

Let's resolve this TODO by declaring a constant or even better,
adding the relevant bindings symbol if there is one.

> +            signature: bindings::NV_VGPU_MSG_SIGNATURE_VALID,
> +            function: function as u32,

This `as` is ok once we set `MsgFunction` to have a `u32`
representation, but...

> +            length: (size_of::<Self>() as u32)

... at the risk of sounding too pedantic, we probably want to do the
arithmetic on the `usize`, and then do a `try_into` u32 to remove this
use of `as` here:

    length: size_of::<Self>()
        .checked_add(cmd_size)
        .ok_or(EOVERFLOW)
        .and_then(|v| v.try_into().map_err(|_| EINVAL))?,

This also requires changing the `cmd_size` to `usize`, which is actually
better as that's also the type used by the caller anyway, so this
removes another conversion.

> +                .checked_add(cmd_size)
> +                .ok_or(EOVERFLOW)?,
> +            rpc_result: 0xffffffff,
> +            rpc_result_private: 0xffffffff,
> +            ..Zeroable::init_zeroed()
> +        })
> +    }
> +}
> +
> +// SAFETY: We can't derive the Zeroable trait for this binding because the
> +// procedural macro doesn't support the syntax used by bindgen to create the
> +// __IncompleteArrayField types. So instead we implement it here, which is safe
> +// because these are explicitly padded structures only containing types for
> +// which any bit pattern, including all zeros, is valid.
> +unsafe impl Zeroable for bindings::rpc_message_header_v {}
> +
> +#[repr(transparent)]
> +pub(crate) struct GspMsgElement {
> +    inner: bindings::GSP_MSG_QUEUE_ELEMENT,
> +}

Small documentation needed.

> +
> +impl GspMsgElement {
> +    #[allow(non_snake_case)]
> +    pub(crate) fn init(
> +        sequence: u32,
> +        cmd_size: usize,
> +        function: MsgFunction,
> +    ) -> impl Init<Self, Error> {
> +        type RpcMessageHeader = bindings::rpc_message_header_v;
> +        type InnerGspMsgElement = bindings::GSP_MSG_QUEUE_ELEMENT;
> +        let init_inner = try_init!(InnerGspMsgElement {
> +            seqNum: sequence,
> +            elemCount: size_of::<Self>()
> +                .checked_add(cmd_size)
> +                .ok_or(EOVERFLOW)?
> +                .div_ceil(GSP_PAGE_SIZE) as u32,

Let's chain a call to `try_into` here as well.

> +            rpc <- RpcMessageHeader::init(cmd_size as u32, function),

... and thanks to the change suggested 3 blocks above, this `as` is no
longer needed.

> +            ..Zeroable::init_zeroed()
> +        });
> +
> +        try_init!(GspMsgElement {
> +            inner <- init_inner,
> +        })
> +    }
> +
> +    pub(crate) fn set_checksum(&mut self, checksum: u32) {
> +        self.inner.checkSum = checksum;
> +    }
> +
> +    // Return the total length of the message, noting that rpc.length includes
> +    // the length of the GspRpcHeader but not the message header.
> +    pub(crate) fn length(&self) -> u32 {
> +        size_of::<Self>() as u32 - size_of::<bindings::rpc_message_header_v>() as u32
> +            + self.inner.rpc.length
> +    }

We can save ourselves a bunch of `as` conversions in the following patch
if we make this method just return `usize`. The only one needed will be
for `self.inner.rpc.length` to `usize`, we really ought to have
non-fallible converters for things like this that obviously cannot fail.

> +
> +    pub(crate) fn sequence(&self) -> u32 {
> +        self.inner.rpc.sequence
> +    }
> +
> +    pub(crate) fn function_number(&self) -> u32 {
> +        self.inner.rpc.function
> +    }
> +
> +    pub(crate) fn function(&self) -> Result<MsgFunction> {
> +        self.inner.rpc.function.try_into()
> +    }
> +
> +    pub(crate) fn element_count(&self) -> u32 {
> +        self.inner.elemCount
> +    }
> +}

Documentation needed for these too.

Powered by blists - more mailing lists