Re: [PATCH 4/4] gpu: nova-core: gsp: cmdq: add the GMC receive path

["Eliot Courtney" <ecourtney@nvidia.com>]

On Tue Nov 3, 2026 at 9:02 AM JST, Alexandre Courbot wrote:
> From: John Hubbard <jhubbard@nvidia.com>
>
> GSP-RM answers a GMC request with a GMC message whose command word
> carries the response flag and the id of the request, and whose sequence
> number is that of the request. It also raises GMC events, named by a
> command id. During boot, the load-and-execute events arrive while the
> driver is waiting for the GSP_INIT reply, and GSP-RM only answers once
> they have been handled.
>
> Add the receive side of the gmc sub-module, mirroring the
> already-existing rpc sub-module.
>
> The transport cannot produce GMC messages yet: the r570 queue element
> header does not carry the type of the message it transports, so the Gmc
> variant of GspParsedMessage is only constructed once the switch to r000
> makes the transport dispatch on the NVDM type. The RPC receive path and
> drain() account for it by logging unclaimed GMC messages.
>
> Assisted-by: LLM
> Signed-off-by: John Hubbard <jhubbard@nvidia.com>
> Co-developed-by: Alexandre Courbot <acourbot@nvidia.com>
> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
> ---
>  drivers/gpu/nova-core/gsp/cmdq.rs     |   7 ++
>  drivers/gpu/nova-core/gsp/cmdq/gmc.rs | 230 +++++++++++++++++++++++++++++++++-
>  drivers/gpu/nova-core/gsp/cmdq/rpc.rs |   8 +-
>  drivers/gpu/nova-core/gsp/fw.rs       |   1 -
>  4 files changed, 243 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/nova-core/gsp/cmdq.rs b/drivers/gpu/nova-core/gsp/cmdq.rs
> index 629ab8d35f87..a1fa7a3b4079 100644
> --- a/drivers/gpu/nova-core/gsp/cmdq.rs
> +++ b/drivers/gpu/nova-core/gsp/cmdq.rs
> @@ -420,6 +420,12 @@ struct GspMessage<'a> {
>  enum GspParsedMessage<'a> {
>      /// A [RPC message](rpc::RpcMessage).
>      Rpc(rpc::RpcMessage<'a>),
> +    /// A [GMC message](gmc::GmcMessage).
> +    //
> +    // The transport header of the r570 firmware carries no message type, so this variant cannot
> +    // be constructed before the switch to r000.
> +    #[expect(dead_code)]
> +    Gmc(gmc::GmcMessage<'a>),
>  }
>  
>  impl GspParsedMessage<'_> {
> @@ -427,6 +433,7 @@ impl GspParsedMessage<'_> {
>      fn log(&self, dev: &device::Device) {
>          match self {
>              GspParsedMessage::Rpc(rpc) => rpc.log(dev),
> +            GspParsedMessage::Gmc(gmc) => gmc.log(dev),
>          }
>      }
>  }
> diff --git a/drivers/gpu/nova-core/gsp/cmdq/gmc.rs b/drivers/gpu/nova-core/gsp/cmdq/gmc.rs
> index b035f30479a8..9adba5cbcd5c 100644
> --- a/drivers/gpu/nova-core/gsp/cmdq/gmc.rs
> +++ b/drivers/gpu/nova-core/gsp/cmdq/gmc.rs
> @@ -29,7 +29,9 @@
>  use super::{
>      Cmdq,
>      CmdqInner,
> -    CommandElement, //
> +    CommandElement,
> +    GspMessage,
> +    GspParsedMessage, //
>  };
>  
>  /// Trait implemented by types representing a GMC command to send to the GSP.
> @@ -150,7 +152,167 @@ fn write(&self, dev: &device::Device, seq: u32, dst: &mut GspCommand<'_>) -> Res
>      }
>  }
>  
> +/// Trait representing GMC messages received from the GSP.
> +///
> +/// These can be either replies to sent commands that expect a reply, or an event not bound to any
> +/// command.
> +pub(crate) trait MessageFromGsp: Sized {
> +    /// Id of the command this is the reply to.
> +    const COMMAND: GmcCommandId;

What about removing "COMMAND" here, and have `await_gmc_reply` take the
CommandToGsp type, returning C::Reply (where C::Reply: MessageFromGsp)?
That removes needing to define a bidirectional mapping which could
become out of sync. Also, if this is going to be used for "an event not
bound to any command" then it's better not to specify `COMMAND` imo.

> +
> +    /// Error type returned by [`MessageFromGsp::read`].
> +    type InitError;
> +
> +    /// Method reading the reply from the payload of the message and returning it.
> +    fn read(
> +        payload: &mut SBufferIter<core::array::IntoIter<&[u8], 2>>,
> +    ) -> Result<Self, Self::InitError>;
> +}
> +
> +/// A GMC message ready to be processed from the message queue.
> +pub(crate) struct GmcMessage<'a> {
> +    // Reference to the GMC API header of the message.
> +    header: &'a GmcApiHeader,
> +    // Slices to the payload of the message. The second slice is empty unless the message loops
> +    // over the message queue.
> +    contents: (&'a [u8], &'a [u8]),
> +}
> +
> +impl<'a> GmcMessage<'a> {
> +    /// Validate the GMC layer of `element` and returns its GMC API header and its contents trimmed
> +    /// down to the GMC payload.
> +    ///
> +    /// # Errors
> +    ///
> +    /// - `EIO` if the element is shorter than the GMC API header, or than the payload length
> +    ///   advertised by the GMC API header.
> +    pub(super) fn parse(dev: &device::Device, element: GspMessage<'a>) -> Result<GmcMessage<'a>> {
> +        let GspMessage {
> +            contents: (slice_1, slice_2),
> +            ..
> +        } = element;
> +
> +        // The GSP message header and the GMC API header together are guaranteed to fit entirely
> +        // into a single page, so it's ok to only look at `slice_1` here.
> +        let (gmc_header, slice_1) = GmcApiHeader::from_bytes_prefix(slice_1).ok_or(EIO)?;
> +        let payload_size = gmc_header.payload_size();
> +
> +        dev_dbg!(
> +            dev,
> +            "GSP GMC: receive: seq# {}, command={:?}, length=0x{:x}\n",
> +            gmc_header.sequence(),
> +            gmc_header.command(),
> +            payload_size,
> +        );
> +
> +        // Check that the driver read area is large enough for the message.
> +        if slice_1.len() + slice_2.len() < payload_size {
> +            return Err(EIO);
> +        }
> +
> +        // Cut the message slices down to the actual length of the message.
> +        let (slice_1, slice_2) = if slice_1.len() > payload_size {
> +            // PANIC: we checked above that `slice_1` is at least as long as `payload_size`.
> +            (slice_1.split_at(payload_size).0, &slice_2[0..0])
> +        } else {
> +            (
> +                slice_1,
> +                // PANIC: we checked above that `slice_1.len() + slice_2.len()` is at least as
> +                // large as `payload_size`.
> +                slice_2.split_at(payload_size - slice_1.len()).0,
> +            )
> +        };

Consider sharing a helper for this (with the RPC code which does the same thing).
The split_at_checked also IMO simplifies the code.

```
fn message_prefix<'a>(
    (s1, s2): (&'a [u8], &'a [u8]),
    len: usize,
) -> Option<(&'a [u8], &'a [u8])> {
    match s1.split_at_checked(len) {
        Some((p, _)) => Some((p, &[])),
        None => s2.get(..len - s1.len()).map(|rest| (s1, rest)),
    }
}
```

> +
> +        Ok(GmcMessage {
> +            header: gmc_header,
> +            contents: (slice_1, slice_2),
> +        })
> +    }
> +
> +    /// Returns the GMC API header of the message.
> +    pub(crate) fn header(&self) -> &'a GmcApiHeader {
> +        self.header
> +    }
> +
> +    /// Returns the payload of the message.
> +    ///
> +    /// The second slice is empty unless the message loops over the message queue.
> +    pub(crate) fn payload(&self) -> (&'a [u8], &'a [u8]) {
> +        self.contents
> +    }
> +
> +    /// Returns the status code of this message if it is the reply to `command` sent with sequence
> +    /// number `seq`, `None` otherwise.
> +    fn is_reply_to(&self, command: GmcCommandId, seq: u32) -> Option<u32> {
> +        if matches!(self.header.command().id(), Ok(id) if id == command)
> +            && self.header.sequence() == u64::from(seq)
> +        {
> +            self.header.status()
> +        } else {
> +            None
> +        }
> +    }

What about calling this "reply_status" since it returns an optional
reply status?

> +
> +    /// Logs an event, meaning a message that no caller was waiting for.
> +    ///
> +    /// A reply that nobody claimed (e.g. one that arrived after its request timed out) and an
> +    /// unknown command id are logged at warning level. Other events are not logged.
> +    pub(super) fn log(&self, dev: &device::Device) {
> +        let command = self.header.command();
> +        let seq = self.header.sequence();
> +
> +        match command.id() {
> +            Ok(id) if command.is_response() => {
> +                dev_warn!(dev, "unclaimed GSP GMC reply to {:?} (seq {})\n", id, seq);
> +            }
> +            // Nothing to do for the remaining known command ids.
> +            Ok(_) => {}
> +            Err(_) => {
> +                dev_warn!(dev, "unknown GSP GMC command {:?} (seq {})\n", command, seq);
> +            }
> +        }
> +    }
> +}
> +
>  impl Cmdq<'_> {
> +    /// Sends `command` to the GSP and waits for the reply.
> +    ///
> +    /// GMC events that arrive before the reply are passed to `on_event`, and RPC messages are
> +    /// logged and consumed. The boot sequence relies on this to handle the load-and-execute
> +    /// events that GSP-RM raises before it answers `GSP_INIT`.
> +    ///
> +    /// The queue is locked for the entire send+receive cycle, including the calls to `on_event`,
> +    /// to ensure that no other command can be interleaved. `on_event` must thus not use the queue.
> +    ///
> +    /// # Errors
> +    ///
> +    /// - `EMSGSIZE` if the command does not fit in a single command queue element.
> +    /// - `ETIMEDOUT` if space does not become available to send the command, or if the reply does
> +    ///   not arrive within [`Self::RECEIVE_TIMEOUT`] of the send, however many events arrive
> +    ///   while waiting.
> +    /// - `EIO` if the variable payload requested by the command has not been entirely
> +    ///   written to by its [`GmcCommand::init_variable_payload`] method, or if the reply carries

[`GmcCommand::init_variable_payload`] =>
[`CommandToGsp::init_variable_payload`]? Can you check the doclinks?

> +    ///   a status other than `NV_OK`.
> +    ///
> +    /// Error codes returned by the command and reply initializers, and by `on_event`, are
> +    /// propagated as-is.
> +    pub(crate) fn send_gmc_command<C>(
> +        &self,
> +        command: C,
> +        on_event: impl Fn(&GmcMessage<'_>) -> Result,
> +    ) -> Result<C::Reply>
> +    where
> +        C: CommandToGsp,
> +        C::Reply: MessageFromGsp,
> +        Error: From<C::InitError>,
> +        Error: From<<C::Reply as MessageFromGsp>::InitError>,
> +    {
> +        let mut inner = self.inner.lock();
> +        let seq = inner.send_gmc_command(command)?;
> +
> +        inner.await_gmc_reply(seq, on_event)
> +    }
> +
>      /// Sends `command` to the GSP without waiting for a reply.
>      ///
>      /// # Errors
> @@ -191,4 +353,70 @@ fn send_gmc_command<C>(&mut self, command: C) -> Result<u32>
>      {
>          self.send_command_element(GmcCommandElement(command))
>      }
> +
> +    /// Receives the reply of type `R` to the command sent with sequence number `seq`, waiting up
> +    /// to [`Cmdq::RECEIVE_TIMEOUT`] from the call.
> +    ///
> +    /// Any GMC event that arrives first is passed to `on_event`, and any RPC message is logged.
> +    /// Neither extends the deadline.
> +    ///
> +    /// # Errors
> +    ///
> +    /// - `ETIMEDOUT` if the reply does not arrive before the deadline, however many other messages
> +    ///   arrive while waiting.
> +    /// - `EIO` if a message fails framing or checksum validation, or if the reply carries a status
> +    ///   other than `NV_OK`.
> +    ///
> +    /// Error codes returned by [`MessageFromGsp::read`] and by `on_event` are propagated as-is.
> +    fn await_gmc_reply<R: MessageFromGsp>(
> +        &mut self,
> +        seq: u32,
> +        on_event: impl Fn(&GmcMessage<'_>) -> Result,
> +    ) -> Result<R>
> +    where
> +        // This allows all error types, including `Infallible`, to be used for `R::InitError`.
> +        Error: From<R::InitError>,

Currently, we have this flow:
await_gmc_reply -> await_msg_with -> consume_element -> closure

`consume_element` returns Result<R>. The outer Result is a transport
level result. The inner R is a gmc/rpc level object, and it has the
contract that ENOMSG means drain this element. In `await_gmc_reply` we
provide the closure which is doing this gmc/rpc level job, and also
asking the `MessageFromGsp` to decode itself plus getting the nvstatus. 

I think that `await_gmc_reply` should return Result<Result<R>> (and
Result<Result<R, NvStatus>> in the future, or even Result<Result<R,
NvStatus>, CmdqError>). For example, if we add an NvStatus enum and have
it convertible to ernno, then later someone adds a conversion to ENOMSG,
it could trigger this. And semantically, there is a difference between a
successfully received reply that had a failed status vs a failed/borked
reply. The RPC code also has this issue where it doesn't distinguish
between a successfully received reply which conveys a failed status vs
borked reply (actually, we don't even check the status for RPC, which
isn't good).

It's also worth considering adding a custom error type enum that has
Drain or something instead of using ENOMSG - then we can encode drain in
the type system and make it impossible for decoder/encoder
implementations to affect cmdq control flow.

Another reason to do this is that the later vGPU code [1]  (see
`CeUtilsAllocError`) needs to distinguish this case - explicitly failed
command vs transport failure.

[1]: https://lore.kernel.org/all/6fbf38e3622a135c662443155513976898b1ab68.1790580105.git.zhiw@nvidia.com/


> +    {
> +        let dev = self.dev;
> +
> +        self.await_msg_with(|message| {
> +            let gmc_message = match message {
> +                GspParsedMessage::Gmc(gmc_message) => gmc_message,
> +                _ => {
> +                    message.log(dev);
> +                    return Err(ENOMSG);
> +                }
> +            };
> +
> +            match gmc_message.is_reply_to(R::COMMAND, seq) {

It's possible if we hit a timeout earlier, to get an old reply here
(`is_response` is true but one or both of the sequence number or command
id doesn't match). In that case IMO we should log and return Err(ENOMSG)
rather than calling `on_event`, because it feels odd for `on_event` to
receive things with `is_response` true.

> +                // Message is not the reply to our command, log it.

Comment says it'll log, but it only calls "on_event".

> +                None => {
> +                    on_event(&gmc_message)?;

It's probably worth a note that if `on_event` returns Err(ENOMSG) then
it'll skip (drain) that event. So Ok(()) and Err(ENOMSG) are equivalent
here, which is a bit odd.

> +                    return Err(ENOMSG);
> +                }
> +                // Reply has an error status.
> +                Some(status @ 1..) => {

Ok for now, but maybe a TODO to add the NvStatus enum? I sent a patch
adding it before [2]. It will probably be more important to delineate
the transport vs message vs content layer errors if we do this.

[2]: https://lore.kernel.org/all/20260418-rmcontrol-v4-2-fda8c76dbb95@nvidia.com/