Re: [PATCH v3 09/33] gpu: nova-core: add GMC API message types

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

On Fri Sep 18, 2026 at 10:06 AM JST, John Hubbard wrote:
<...>
> diff --git a/drivers/gpu/nova-core/gsp/fw.rs b/drivers/gpu/nova-core/gsp/fw.rs
> index 285c23cea771..1aca5ca82764 100644
> --- a/drivers/gpu/nova-core/gsp/fw.rs
> +++ b/drivers/gpu/nova-core/gsp/fw.rs
> @@ -50,6 +50,11 @@
>          cmdq::Cmdq, //
>          GSP_PAGE_SIZE,
>      },
> +    mctp::{
> +        MctpHeader,
> +        NvdmHeader,
> +        NvdmType, //
> +    },
>      num::{
>          self,
>          FromSafeCast, //
> @@ -889,6 +894,221 @@ unsafe impl AsBytes for GspMsgElement {}
>  // are valid.
>  unsafe impl FromBytes for GspMsgElement {}
>  
> +/// First word of every queue element: `"MCTP"` in ASCII.
> +const MCTP_MAGIC: u32 = 0x4D43_5450;
> +
> +/// The queue element header that opens every queue element, whatever kind of message follows.
> +///
> +/// It holds an MCTP (Management Component Transport Protocol) header and an NVDM (NVIDIA
> +/// vendor-defined message) header. The NVDM type selects the message header that follows: the RPC
> +/// header or the GMC (GPU Management Controller) API header.
> +///
> +/// ```text
> +///     +------------------------------------+
> +///     | queue element header               |  QueueElementHeader: magic, element length, MCTP
> +///     |                                    |  header, NVDM header, message length
> +///     +------------------------------------+
> +///     | message header                     |  the RPC header or the GMC API header. The NVDM
> +///     +------------------------------------+  type selects between the two.
> +///     | payload                            |  command-specific data
> +///     +------------------------------------+
> +/// ```
> +#[repr(C)]
> +pub(crate) struct QueueElementHeader {
> +    magic: u32,

Do we want to mention that the expected value is `MCTP_MAGIC`?

> +    /// Length of the whole element: the queue element header, the message header and the
> +    /// payload. Open RM calls it `mctpPayloadSize`.

We probably don't care what OpenRM (without a space :)) calls it. But
why not use the same name, so the relationship becomes clear?

> +    element_len: u32,
> +    mctp: MctpHeader,
> +    nvdm: NvdmHeader,
> +    /// Length of the message header and the payload, the queue element header excluded. Open RM
> +    /// calls it `nvdmPayloadSize`.
> +    message_len: u32,

Same here. Btw, reading at the comment, it sounds like `message_len` is
always equal to `element_len - size_of::<QueueElementHeader>`? Or am I
misreading?

> +    reserved: u32,
> +}
> +
> +static_assert!(
> +    core::mem::offset_of!(QueueElementHeader, magic)
> +        == core::mem::offset_of!(r000_00::GSP_MSG_QUEUE_ELEMENT, mctpMagic)
> +);
> +static_assert!(
> +    core::mem::offset_of!(QueueElementHeader, element_len)
> +        == core::mem::offset_of!(r000_00::GSP_MSG_QUEUE_ELEMENT, mctpPayloadSize)
> +);
> +static_assert!(
> +    core::mem::offset_of!(QueueElementHeader, mctp)
> +        == core::mem::offset_of!(r000_00::GSP_MSG_QUEUE_ELEMENT, mctpHeader)
> +);
> +static_assert!(
> +    core::mem::offset_of!(QueueElementHeader, nvdm)
> +        == core::mem::offset_of!(r000_00::GSP_MSG_QUEUE_ELEMENT, nvdmHeader)
> +);

Mmm, that's not great. Basically we are redefining a new type that is
mirroring a binding-generated type, and relying on these asserts to make
sure the two types match. All that because `GSP_MSG_QUEUE_ELEMENT` has
too long a size due to the encryption support part.

This is something where I would like to ask OpenRM to revise their
definition so we can leverage the generated bindings properly, and end
up with just

  pub(crate) struct QueueElementHeader(r000_00::GSP_MSG_QUEUE_ELEMENT);

But in the meantime, I guess defining our own type like this patch does
is the only viable route. Let's add a (short) note explaining the reason
for this though.

If we confirm the layout at build-time though, we should also assert the
offsets of the members in the union - which IIUC from your reply to
Timur you already did.

> +
> +#[expect(dead_code)]
> +impl QueueElementHeader {
> +    /// Builds the queue element header of an element whose message header and payload together
> +    /// take `message_len` bytes.
> +    ///
> +    /// # Errors
> +    ///
> +    /// - `EOVERFLOW` if a length does not fit its 32-bit field.
> +    fn new(nvdm: NvdmType, message_len: usize) -> Result<Self> {
> +        Ok(Self {
> +            magic: MCTP_MAGIC,
> +            element_len: size_of::<Self>()
> +                .checked_add(message_len)
> +                .ok_or(EOVERFLOW)?
> +                .try_into()
> +                .map_err(|_| EOVERFLOW)?,
> +            mctp: MctpHeader::single_packet(),
> +            nvdm: NvdmHeader::new(nvdm),
> +            message_len: message_len.try_into().map_err(|_| EOVERFLOW)?,
> +            reserved: 0,
> +        })
> +    }
> +
> +    /// Returns the length of the whole element, the queue element header included.
> +    fn element_len(&self) -> usize {
> +        num::u32_as_usize(self.element_len)
> +    }
> +
> +    /// Returns the length of the payload that follows a message header of `message_header_len`
> +    /// bytes.
> +    fn payload_len(&self, message_header_len: usize) -> usize {
> +        num::u32_as_usize(self.message_len).saturating_sub(message_header_len)
> +    }

Passing the header length looks awkward and error-prone, and as
discussed with Timur you then need to manage errors (which was done in
the "add GMC transport receive path" patch rather than now).

There are only two callers of this, one per header type, let's make the
caller responsible for subtracting their own header after they have
matched against it instead of unconditionally subtracting an arbitrary
amount here.

> +
> +    /// Returns the number of queue slots that this element occupies.
> +    fn element_count(&self) -> u32 {
> +        self.element_len
> +            .div_ceil(num::usize_into_u32::<GSP_PAGE_SIZE>())
> +    }
> +}
> +
> +// SAFETY: All fields are integer types or transparent wrappers over one, with no padding.
> +unsafe impl AsBytes for QueueElementHeader {}
> +
> +// SAFETY: All fields are integer types for which all bit patterns are valid.
> +unsafe impl FromBytes for QueueElementHeader {}
> +
> +/// Header of a GMC API message.
> +#[repr(C)]
> +#[derive(Zeroable)]
> +pub(crate) struct GmcApiHeader {
> +    /// Command id in the low three bytes, flags in the high byte.

... then this should be a `bitfield` type. :) Especially since I see we
are doing some bit-masking in later patches. Commands can then be a
nicely-defined enum used for the relevant field.

> +    pub(crate) command: u32,
> +    /// Payload size in bytes.
> +    pub(crate) size: u32,
> +    /// Sequence number that GSP-RM copies from a request into its response.
> +    pub(crate) sequence: u64,
> +    /// In a request, the largest response that the sender accepts. In a response, the `NV_STATUS`.
> +    pub(crate) max_resp_or_status: u32,
> +    reserved: [u32; 5],
> +}
> +
> +static_assert!(size_of::<GmcApiHeader>() == size_of::<r000_00::GMCAPI_HEADER>());
> +static_assert!(
> +    core::mem::offset_of!(GmcApiHeader, command)
> +        == core::mem::offset_of!(r000_00::GMCAPI_HEADER, command)
> +);
> +static_assert!(
> +    core::mem::offset_of!(GmcApiHeader, size)
> +        == core::mem::offset_of!(r000_00::GMCAPI_HEADER, size)
> +);
> +static_assert!(
> +    core::mem::offset_of!(GmcApiHeader, sequence)
> +        == core::mem::offset_of!(r000_00::GMCAPI_HEADER, sequence)
> +);
> +static_assert!(
> +    core::mem::offset_of!(GmcApiHeader, max_resp_or_status)
> +        == core::mem::offset_of!(r000_00::GMCAPI_HEADER, __bindgen_anon_1)
> +);
> +static_assert!(
> +    core::mem::offset_of!(GmcApiHeader, reserved)
> +        == core::mem::offset_of!(r000_00::GMCAPI_HEADER, reserved)
> +);

So for this one the story is different than `QueueElementHeader`:
`GMCAPI_HEADER` has the right size and a layout that we can wrap, so
here we have no excuse for not defining a newtype instead of doing this
tedious checking.

I understand there is an union and this requires an unsafe block to
read, but we would need exactly one, in the `status` method. Which imho
is better than adding ~60 LoCs redefining what we already have and
confirming what we already know.

> +
> +impl GmcApiHeader {
> +    /// Returns the `NV_STATUS` that a response carries.
> +    ///
> +    /// The value is meaningful only on a response, which GSP-RM marks with a flag in the command
> +    /// word. In a request, the same word holds the largest response that the sender accepts.
> +    #[expect(dead_code)]
> +    pub(crate) fn status(&self) -> u32 {
> +        self.max_resp_or_status
> +    }
> +}
> +
> +// SAFETY: All fields are integer types with no uninitialized padding bytes.
> +unsafe impl AsBytes for GmcApiHeader {}
> +
> +// SAFETY: All fields are integer types for which all bit patterns are valid.
> +unsafe impl FromBytes for GmcApiHeader {}
> +
> +/// The headers that open a GMC API queue element: the queue element header and the GMC API
> +/// header.
> +#[repr(C)]
> +pub(crate) struct GspGmcMsgElement {
> +    element_header: QueueElementHeader,
> +    pub(crate) gmc: GmcApiHeader,
> +}
> +
> +// `AsBytes` below requires that no padding separates the two headers.
> +static_assert!(
> +    size_of::<GspGmcMsgElement>() == size_of::<QueueElementHeader>() + size_of::<GmcApiHeader>()
> +);
> +
> +#[expect(dead_code)]
> +impl GspGmcMsgElement {
> +    /// Creates the queue element header and the GMC API header of a request that carries
> +    /// `payload_size` bytes of payload.
> +    ///
> +    /// `max_response_size` is the largest response that the sender accepts, and zero for a request
> +    /// that GSP-RM does not answer.
> +    ///
> +    /// # Errors
> +    ///
> +    /// - `EOVERFLOW` if a length does not fit its 32-bit field.
> +    pub(crate) fn init(
> +        command_id: u32,
> +        sequence: u64,
> +        payload_size: usize,
> +        max_response_size: u32,
> +    ) -> impl Init<Self, Error> {
> +        try_init!(GspGmcMsgElement {
> +            element_header: QueueElementHeader::new(
> +                NvdmType::GmcApi,
> +                size_of::<GmcApiHeader>()
> +                    .checked_add(payload_size)
> +                    .ok_or(EOVERFLOW)?,
> +            )?,
> +            gmc: GmcApiHeader {
> +                command: command_id,
> +                size: payload_size.try_into().map_err(|_| EOVERFLOW)?,
> +                sequence,
> +                max_resp_or_status: max_response_size,
> +                reserved: [0; 5],
> +            },
> +        })
> +    }
> +
> +    /// Returns the length of the whole element, both headers included.
> +    pub(crate) fn length(&self) -> usize {
> +        self.element_header.element_len()
> +    }
> +
> +    /// Returns the number of queue slots that this element occupies.
> +    pub(crate) fn element_count(&self) -> u32 {
> +        self.element_header.element_count()
> +    }

Let's provide an `element_header` returning the header which the caller
can then query instead of one proxy method per method of the header we
want to access.