Re: [PATCH v2 2/9] gpu: nova-core: gsp: introduce and use proper RpcMessageHeader type

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

On Sun Sep 27, 2026 at 10:46 PM JST, Alexandre Courbot wrote:
> So far, the GSP command queue transport and message layer code were
> intertwined, a design issue that goes as deep as the types themselves:
> the generated bindings for `GspMsgElement` even include the RPC header
> at its end.
>
> This makes it difficult to introduce the new GMC message type; thus this
> patch works around these limitations to make the RPC message header more
> explicit and allow it to be eventually handled by a different layer.
>
> The `RpcMessageHeader` wrapping type is introduced following the same
> model as `GspMsgElement`, and can be obtained from the latter. The
> methods of `GspMsgElement` that actually query the RPC header are moved
> to `RpcMessageHeader`.
>
> Regarding initialization, `GspMsgElement` leaves the RPC header zeroed,
> and the command queue code is now responsible for initializing it in a
> separate call.
>
> The only functional change is that the RPC debug messages now display
> the size of the RPC payload instead of the whole message including its
> headers, as they are technically part of the message layer. This metric
> is arguably more useful as the headers have successfully been parsed by
> the time we can print these messages.
>
> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
> ---
>  drivers/gpu/nova-core/gsp/cmdq.rs | 25 +++++++----
>  drivers/gpu/nova-core/gsp/fw.rs   | 95 +++++++++++++++++++++++++--------------
>  2 files changed, 78 insertions(+), 42 deletions(-)
>
> diff --git a/drivers/gpu/nova-core/gsp/cmdq.rs b/drivers/gpu/nova-core/gsp/cmdq.rs
> index d293d28b0967..3a8548a51259 100644
> --- a/drivers/gpu/nova-core/gsp/cmdq.rs
> +++ b/drivers/gpu/nova-core/gsp/cmdq.rs
> @@ -50,6 +50,7 @@
>              MsgFunction,
>              MsgqRxHeader,
>              MsgqTxHeader,
> +            RpcMessageHeader,
>              GSP_MSG_QUEUE_ELEMENT_SIZE_MAX, //
>          },
>          PteArray,
> @@ -664,11 +665,16 @@ fn send_single_command<M>(&mut self, command: M) -> Result
>          let (cmd, payload_1) = M::Command::from_bytes_mut_prefix(dst.contents.0).ok_or(EIO)?;
>  
>          // Fill the header and command in-place.
> -        let msg_element = GspMsgElement::init(self.seq, size_in_bytes, M::FUNCTION);
> +        let msg_element_init = GspMsgElement::init(self.seq, size_in_bytes);
> +        let rpc_header_init = RpcMessageHeader::init(size_in_bytes, M::FUNCTION);
>          // SAFETY: `msg_header` and `cmd` are valid references, and not touched if the initializer
>          // fails.
>          unsafe {
> -            pin_init::raw_try_init(core::ptr::from_mut(dst.header), msg_element)?;
> +            pin_init::raw_try_init(core::ptr::from_mut(dst.header), msg_element_init)?;
> +            pin_init::raw_try_init(
> +                core::ptr::from_mut(dst.header.rpc_header_mut()),
> +                rpc_header_init,
> +            )?;
>              pin_init::raw_try_init(core::ptr::from_mut(cmd), command.init())?;
>          }

nit: I think the style is to have separate unsafe blocks for each call
with separate justifications.

>  
> @@ -694,7 +700,7 @@ fn send_single_command<M>(&mut self, command: M) -> Result
>              "GSP RPC: send: seq# {}, function={:?}, length=0x{:x}\n",
>              self.seq,
>              M::FUNCTION,
> -            dst.header.length(),
> +            size_in_bytes,
>          );
>  
>          // All set - update the write pointer and inform the GSP of the new command.
> @@ -777,16 +783,17 @@ fn wait_for_msg(&self, timeout: Delta) -> Result<GspMessage<'_>> {
>              return Err(EIO);
>          }
>  
> +        let rpc_header = header.rpc_header();
> +        let payload_length = rpc_header.length();
> +
>          dev_dbg!(
>              &self.dev,
>              "GSP RPC: receive: seq# {}, function={:?}, length=0x{:x}\n",
> -            header.sequence(),
> -            header.function(),
> -            header.length(),
> +            rpc_header.sequence(),
> +            rpc_header.function(),
> +            payload_length,
>          );
>  
> -        let payload_length = header.payload_length();
> -
>          // Check that the driver read area is large enough for the message.
>          if slice_1.len() + slice_2.len() < payload_length {
>              return Err(EIO);
> @@ -833,7 +840,7 @@ fn receive_msg<M: MessageFromGsp>(&mut self, timeout: Delta) -> Result<M>
>          Error: From<M::InitError>,
>      {
>          let message = self.wait_for_msg(timeout)?;
> -        let function = message.header.function().map_err(|_| EINVAL)?;
> +        let function = message.header.rpc_header().function().map_err(|_| EINVAL)?;
>  
>          // Extract the message. Store the result as we want to advance the read pointer even in
>          // case of failure.
> diff --git a/drivers/gpu/nova-core/gsp/fw.rs b/drivers/gpu/nova-core/gsp/fw.rs
> index 918a7ae809eb..b12034db7857 100644
> --- a/drivers/gpu/nova-core/gsp/fw.rs
> +++ b/drivers/gpu/nova-core/gsp/fw.rs
> @@ -781,11 +781,25 @@ fn new() -> Self {
>      }
>  }
>  
> -impl bindings::rpc_message_header_v {
> -    fn init(cmd_size: usize, function: MsgFunction) -> impl Init<Self, Error> {
> -        type RpcMessageHeader = bindings::rpc_message_header_v;
> +#[repr(transparent)]
> +pub(crate) struct RpcMessageHeader {
> +    inner: bindings::rpc_message_header_v,
> +}
>  
> -        try_init!(RpcMessageHeader {
> +// SAFETY: Padding is explicit and does not contain uninitialized data.
> +unsafe impl AsBytes for RpcMessageHeader {}
> +
> +// SAFETY: This struct only contains integer types for which all bit patterns
> +// are valid.
> +unsafe impl FromBytes for RpcMessageHeader {}

nit: these impls are not used

> +
> +impl RpcMessageHeader {
> +    /// Creates a new RPC header.
> +    ///
> +    /// `cmd_size` is the size in bytes of the payload. `function` is the RPC function of the
> +    /// message.
> +    pub(crate) fn init(cmd_size: usize, function: MsgFunction) -> impl Init<Self, Error> {
> +        let init_inner = try_init!(bindings::rpc_message_header_v {
>              header_version: MsgHeaderVersion::new().into(),
>              signature: bindings::NV_VGPU_MSG_SIGNATURE_VALID,
>              function: function.into(),
> @@ -796,8 +810,32 @@ fn init(cmd_size: usize, function: MsgFunction) -> impl Init<Self, Error> {
>              rpc_result: 0xffffffff,
>              rpc_result_private: 0xffffffff,
>              ..Zeroable::init_zeroed()
> +        });
> +
> +        try_init!(RpcMessageHeader {
> +            inner <- init_inner,
>          })
>      }
> +
> +    /// Returns the length of the RPC's payload, not including the header.
> +    pub(crate) fn length(&self) -> usize {
> +        // `length` includes the length of the RPC message header.
> +        num::u32_as_usize(self.inner.length).saturating_sub(size_of::<Self>())
> +    }

Suggest calling this `payload_length` because now we have to `lengths`,
one which is the length of the entire thing (On GspMsgElement), and
this, which is just the payload. optional nit: rename
GspMsgElement::length to frame_length.

> +
> +    /// Returns the sequence number of the message.
> +    pub(crate) fn sequence(&self) -> u32 {
> +        self.inner.sequence
> +    }
> +
> +    /// Returns the function of the message, if it is valid, or the invalid function number as an
> +    /// error.
> +    pub(crate) fn function(&self) -> Result<MsgFunction, u32> {
> +        self.inner
> +            .function
> +            .try_into()
> +            .map_err(|_| self.inner.function)
> +    }
>  }
>  
>  /// GSP Message Element.
> @@ -811,17 +849,15 @@ pub(crate) struct GspMsgElement {
>  impl GspMsgElement {
>      /// Creates a new message element.
>      ///
> +    /// The RPC header is left initialized to zero and must be initialized separately using e.g.
> +    /// [`Self::rpc_header_mut`].
> +    ///
>      /// # Arguments
>      ///
>      /// * `sequence` - Sequence number of the message.
>      /// * `cmd_size` - Size of the command (not including the message element), in bytes.
>      /// * `function` - Function of the message.

nit: Update or remove argument list?

> -    pub(crate) fn init(
> -        sequence: u32,
> -        cmd_size: usize,
> -        function: MsgFunction,
> -    ) -> impl Init<Self, Error> {
> -        type RpcMessageHeader = bindings::rpc_message_header_v;
> +    pub(crate) fn init(sequence: u32, cmd_size: usize) -> impl Init<Self, Error> {
>          type InnerGspMsgElement = bindings::GSP_MSG_QUEUE_ELEMENT;
>          let init_inner = try_init!(InnerGspMsgElement {
>              seqNum: sequence,
> @@ -831,7 +867,6 @@ pub(crate) fn init(
>                  .div_ceil(GSP_PAGE_SIZE)
>                  .try_into()
>                  .map_err(|_| EOVERFLOW)?,
> -            rpc <- RpcMessageHeader::init(cmd_size, function),
>              ..Zeroable::init_zeroed()
>          });
>  
> @@ -848,34 +883,28 @@ pub(crate) fn set_checksum(&mut self, checksum: u32) {
>          self.inner.checkSum = checksum;
>      }
>  
> -    /// Returns the length of the message's payload.
> -    pub(crate) fn payload_length(&self) -> usize {
> -        // `rpc.length` includes the length of the RPC message header.
> -        num::u32_as_usize(self.inner.rpc.length)
> -            .saturating_sub(size_of::<bindings::rpc_message_header_v>())
> +    /// Returns a reference to the RPC header within the message element.
> +    pub(crate) fn rpc_header(&self) -> &RpcMessageHeader {
> +        // SAFETY: transparent type.
> +        unsafe { core::mem::transmute(&self.inner.rpc) }
> +    }
> +
> +    /// Returns a mutable reference to the RPC header within the message element.
> +    pub(crate) fn rpc_header_mut(&mut self) -> &mut RpcMessageHeader {
> +        // SAFETY: `RpcMessageHeader` is a transparent wrapper for the type of `inner.rpc`.
> +        unsafe { core::mem::transmute(&mut self.inner.rpc) }
>      }
>  
>      /// Returns the total length of the message, message and RPC headers included.
> +    ///
> +    /// Note: this method is technically a layering violation as it is transport-layer code
> +    /// accessing message-layer data. It only exists because it is necessary to accurately compute
> +    /// the checksum upon receiving a message from the GSP.

This is also used in `receive_msg` for `advance_cpu_read_ptr`, not just
for checksum. Also, I wouldn't necessarily describe this as a layering
violation. The transport needs to know the size of each frame being
sent, it just so happens that the data that contains that is at a weird
offset in the transport layer message (i.e. in the RPC header). It's a
nit but I would move the comment from on the function to inside saying
that > it's weird but the transport message size needs to look in
message layer data to know the length; Because it's very natural for a
transport layer to need to know the size of its frames.


With comments fixed or refuted:

Reviewed-by: Eliot Courtney <ecourtney@nvidia.com>