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

[Eliot Courtney]

On Wed Sep 30, 2026 at 11:18 PM JST, Alexandre Courbot wrote:
> 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.
>
> Fixed.
>
>>
>>>  
>>> @@ -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
>
> Nice catch, removed them.
>
>>
>>> +
>>> +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.
>
> I'm not completely convinced here, since the type should make it clear
> which length we are dealing with. One could argue that GspMsgElement's
> length is also the length of its payload, putting us in the same
> situation, only with a longer name. :)

IMO it is difficult to argue this because in patch 5 in the v3 these
lines get silently broken because the type of `header` changes from
`GspMsgElement` to `RpcMessageHeader`:

```
        self.gsp_mem.advance_cpu_read_ptr(u32::try_from(
            message.header.length().div_ceil(GSP_PAGE_SIZE),
        )?);
```

So this advances the ring buffer by the wrong length, but this would
have been noticed if the function name identified the kind of length it
meant. This is only a problem in patch 5 btw, it's fixed in patch 6.