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