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

[Alexandre Courbot]

On Mon Sep 28, 2026 at 1:43 PM JST, Eliot Courtney wrote:
> 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@xxxxxxxxxx>
>> ---
>>  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.

Ah yeah, now that I've removed patch 1 that is indeed the case.

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

As long as there is only one message type this does work yes. r000 has a
proper size field in its transport header (and no checksum at all) so
this will be removed eventually, so I'll move that comment inside the
method as suggested.