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 <[email protected]>
>>> ---
>>> 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.
