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

["Alexandre Courbot" <acourbot@nvidia.com>]

On Thu Nov 5, 2026 at 6:23 PM JST, Eliot Courtney wrote:
<...>
>> 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.

Oh yes, that simplifies things quite a bit, thanks!

<...>
>> +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)),
>     }
> }
> ```

Although the subtraction from the `None` branch can never underflow, it
would still need a justifying comment and I was wondering whether we
could remove it entirely. I came with this alternative version which I
like slightly better, let me know if you see a problem with it:

    // See if `slice_1` covers the whole message, take it entirely if it doesn't.
    let slice_1 = slice_1
        .split_at_checked(length)
        .map(|split| split.0)
        .unwrap_or(slice_1);
    // `slice_2` is the remainder of the message not covered by `slice_1`.
    let slice_2 = slice_2
        .get(..length.saturating_sub(slice_1.len()))
        .ok_or(EIO)?;

>
>> +
>> +        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?

Done.

>
>> +
>> +    /// 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?

Fixed, thanks!

<...>
>> +    /// 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.

Yeah, that's indeed a blind spot in the command queue design. Your
nested Result suggestion makes sense, so I'll introduce the idea as a
separate patch in the next revision.

>
> [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.

Done. TBH I am still not super happy with the way we are handling events
actually, but would like to have a bit more visibility about how we will
use GMCAPI to change things more fundamentally. The current way is not
perfect for gets the job done for now.

>
>> +                // 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.

Indeed. This behavior will be gone in the next revision.

>
>> +                    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.

I am not quite sure what you want to me do here (probably because I lack
the context about NvStatus). Can you be more specific?