[PATCH v3 30/33] gpu: nova-core: match GSP RPC replies by sequence, not just function

[John Hubbard <jhubbard@nvidia.com>]

GSP-RM copies a command's RPC sequence number into its reply, alongside
the function code. The r570 firmware leaves that field zero, and the
r000 firmware fills it in.

Nova-core matched a reply on the function code alone and never set the
sequence in a command, so a reply to a command that had already timed
out could satisfy a later command with the same function code.

Write the sequence number into every RPC command, and require both the
function code and the sequence to match before accepting a reply. An
unsolicited event answers no command, so a caller that waits for an
event still matches on the function code alone. A message with the
expected function code and a stale sequence is a late reply rather than
an event, so it is logged and dropped.

Assisted-by: LLM
Signed-off-by: John Hubbard <jhubbard@nvidia.com>
---
 Documentation/gpu/nova/core/interrupts.rst |  9 ++-
 drivers/gpu/nova-core/gsp/cmdq.rs          | 94 ++++++++++++++--------
 drivers/gpu/nova-core/gsp/fw.rs            | 13 ++-
 3 files changed, 77 insertions(+), 39 deletions(-)

diff --git a/Documentation/gpu/nova/core/interrupts.rst b/Documentation/gpu/nova/core/interrupts.rst
index fdcd789cf4f9..8519c73e9989 100644
--- a/Documentation/gpu/nova/core/interrupts.rst
+++ b/Documentation/gpu/nova/core/interrupts.rst
@@ -574,9 +574,12 @@ function code says which it is.
   receive trace at debug level already records every message's arrival with
   its sequence number, function code, and length.
 
-The sequence number takes no part in the match, because the GSP does not echo
-the command's sequence number on every reply. On r570 the reply to
-``UnloadingGuestDriver`` carries sequence 0.
+A command's reply must carry the RPC sequence number that nova-core wrote into
+the command, as well as its function code. A message with the awaited function
+code and a different sequence number is a stale reply to a command that already
+timed out, so it is logged at warning level and dropped rather than classified
+as an event. An unsolicited event answers no command, so a caller awaiting one
+matches on the function code alone.
 
 The read pointer advances past every message, whether it matched, was an event,
 or matched but failed to decode, so a message is never left at the queue head
diff --git a/drivers/gpu/nova-core/gsp/cmdq.rs b/drivers/gpu/nova-core/gsp/cmdq.rs
index 93c31b49903a..fc26c7d8aac0 100644
--- a/drivers/gpu/nova-core/gsp/cmdq.rs
+++ b/drivers/gpu/nova-core/gsp/cmdq.rs
@@ -555,7 +555,7 @@ pub(crate) fn new(
                 inner <- new_mutex!(CmdqInner {
                     dev,
                     gsp_mem,
-                    seq: 0,
+                    rpc_seq: 0,
                     poisoned: Cell::new(false),
                 }),
             }))
@@ -587,9 +587,9 @@ pub(crate) fn send_command<M>(&self, command: M) -> Result<M::Reply>
         Error: From<<M::Reply as MessageFromGsp>::InitError>,
     {
         let mut inner = self.inner.lock();
-        inner.send_command(command)?;
+        let expected_seq = inner.send_command(command)?;
 
-        inner.await_msg()
+        inner.await_msg(Some(expected_seq))
     }
 
     /// Sends `command` to the GSP without waiting for a reply.
@@ -607,7 +607,7 @@ pub(crate) fn send_command_no_wait<M>(&self, command: M) -> Result
         M: CommandToGsp<Reply = NoReply>,
         Error: From<M::InitError>,
     {
-        self.inner.lock().send_command(command)
+        self.inner.lock().send_command(command).map(|_| ())
     }
 
     /// Waits for the response to the GMC request with command id `command_id`, and passes every
@@ -647,6 +647,8 @@ pub(crate) fn send_gmc_no_wait(
     /// Waits for an unsolicited GSP event of type `M`. Events that arrive before it are logged and
     /// consumed.
     ///
+    /// The event answers no command, so it is matched on its function code alone.
+    ///
     /// The queue mutex is held for the whole wait, up to [`Self::RECEIVE_TIMEOUT`], so no other
     /// caller can send a command or consume an event meanwhile.
     ///
@@ -663,7 +665,7 @@ pub(crate) fn await_msg<M: MessageFromGsp>(&self) -> Result<M>
         // This allows all error types, including `Infallible`, to be used for `M::InitError`.
         Error: From<M::InitError>,
     {
-        self.inner.lock().await_msg()
+        self.inner.lock().await_msg(None)
     }
 
     /// Logs and consumes every message the GSP has already posted, and returns without waiting for
@@ -685,8 +687,9 @@ pub(crate) fn drain(&self) -> Result {
 struct CmdqInner<'a> {
     /// Device this command queue belongs to.
     dev: &'a device::Device,
-    /// Current command sequence number.
-    seq: u32,
+    /// Next RPC sequence number, advanced once per command, however many messages the command is
+    /// split into.
+    rpc_seq: u32,
     /// Set once a message fails framing validation. Every later receive fails, since
     /// the bad message cannot be skipped. See "Draining the GSP-to-CPU queue" in
     /// `Documentation/gpu/nova/core/interrupts.rst`.
@@ -711,7 +714,7 @@ impl CmdqInner<'_> {
     ///   written to by its [`CommandToGsp::init_variable_payload`] method.
     ///
     /// Error codes returned by the command initializers are propagated as-is.
-    fn send_single_command<M>(&mut self, command: M) -> Result
+    fn send_single_command<M>(&mut self, command: M, rpc_seq: u32) -> Result
     where
         M: CommandToGsp,
         // This allows all error types, including `Infallible`, to be used for `M::InitError`.
@@ -728,7 +731,7 @@ 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(size_in_bytes, M::FUNCTION);
+        let msg_element = GspMsgElement::init(rpc_seq, size_in_bytes, M::FUNCTION);
         // SAFETY: `msg_header` and `cmd` are valid references, and not touched if the initializer
         // fails.
         unsafe {
@@ -748,22 +751,22 @@ fn send_single_command<M>(&mut self, command: M) -> Result
         dev_dbg!(
             &self.dev,
             "GSP RPC: send: seq# {}, function={:?}, length=0x{:x}\n",
-            self.seq,
+            rpc_seq,
             M::FUNCTION,
             dst.header.length(),
         );
 
         // All set - update the write pointer and inform the GSP of the new command.
         let elem_count = dst.header.element_count();
-        self.seq += 1;
         self.gsp_mem.advance_cpu_write_ptr(elem_count);
 
         Ok(())
     }
 
-    /// Sends `command` to the GSP.
+    /// Sends `command` to the GSP and returns the RPC sequence number assigned to it.
     ///
-    /// The command may be split into multiple messages if it is large.
+    /// The command may be split into multiple messages if it is large. GSP-RM copies the
+    /// sequence number into its reply.
     ///
     /// # Errors
     ///
@@ -772,24 +775,27 @@ fn send_single_command<M>(&mut self, command: M) -> Result
     ///   written to by its [`CommandToGsp::init_variable_payload`] method.
     ///
     /// Error codes returned by the command initializers are propagated as-is.
-    fn send_command<M>(&mut self, command: M) -> Result
+    fn send_command<M>(&mut self, command: M) -> Result<u32>
     where
         M: CommandToGsp,
         Error: From<M::InitError>,
     {
+        let rpc_seq = self.rpc_seq;
+        self.rpc_seq = self.rpc_seq.wrapping_add(1);
+
         match SplitState::new(command)? {
-            SplitState::Single(command) => self.send_single_command(command),
+            SplitState::Single(command) => self.send_single_command(command, rpc_seq)?,
             SplitState::Split(command, mut continuations) => {
-                self.send_single_command(command)?;
+                self.send_single_command(command, rpc_seq)?;
 
                 while let Some(continuation) = continuations.next() {
                     // Turbofish needed because the compiler cannot infer M here.
-                    self.send_single_command::<ContinuationRecord<'_>>(continuation)?;
+                    self.send_single_command::<ContinuationRecord<'_>>(continuation, rpc_seq)?;
                 }
-
-                Ok(())
             }
         }
+
+        Ok(rpc_seq)
     }
 
     /// Logs `reason`, poisons the queue, and returns `EIO` for the caller to propagate.
@@ -803,22 +809,26 @@ fn poison(&self, reason: fmt::Arguments<'_>) -> Error {
     /// Sends a GMC API request to the GSP.
     ///
     /// `payload` follows the GMC API header in the element, and `max_response_size` is the largest
-    /// response that the caller accepts. The request carries the next sequence number, which GSP-RM
-    /// copies into its response. The number is consumed even if the send fails.
+    /// response that the caller accepts. The request carries the next RPC sequence number, which
+    /// GSP-RM copies into its response. The number is consumed even if the send fails.
     ///
     /// # Errors
     ///
     /// Errors from [`DmaGspMem::allocate_command`] are propagated as-is.
     fn send_gmc(&mut self, command_id: u32, payload: &[u8], max_response_size: u32) -> Result {
-        let seq = self.seq;
-        self.seq = self.seq.wrapping_add(1);
+        let rpc_seq = self.rpc_seq;
+        self.rpc_seq = self.rpc_seq.wrapping_add(1);
 
         let dst = self
             .gsp_mem
             .allocate_command::<GspGmcMsgElement>(payload.len(), Self::ALLOCATE_TIMEOUT)?;
 
-        let msg_element =
-            GspGmcMsgElement::init(command_id, u64::from(seq), payload.len(), max_response_size);
+        let msg_element = GspGmcMsgElement::init(
+            command_id,
+            u64::from(rpc_seq),
+            payload.len(),
+            max_response_size,
+        );
         // SAFETY: `dst.header` is a valid reference, and not written if the initializer fails.
         unsafe {
             pin_init::raw_try_init(core::ptr::from_mut(dst.header), msg_element)?;
@@ -830,7 +840,7 @@ fn send_gmc(&mut self, command_id: u32, payload: &[u8], max_response_size: u32)
         dev_dbg!(
             &self.dev,
             "GSP GMC: send: seq# {}, command_id=0x{:x}, length=0x{:x}\n",
-            seq,
+            rpc_seq,
             command_id,
             dst.header.length(),
         );
@@ -908,6 +918,10 @@ fn wait_for_msg(&self, timeout: Delta) -> Result<GspMessage<'_>> {
     /// A message whose function code is `M::FUNCTION` is decoded and returned. Any other message
     /// is logged as an event.
     ///
+    /// With `expected_seq` set, the message must carry that RPC sequence number too. A message
+    /// with the expected function code and a different sequence is a stale reply to a command
+    /// that already timed out, so it is logged and dropped rather than classified as an event.
+    ///
     /// The read pointer advances past the message in every case, including a decode failure.
     ///
     /// # Errors
@@ -918,7 +932,11 @@ fn wait_for_msg(&self, timeout: Delta) -> Result<GspMessage<'_>> {
     /// - `ENOMSG` if the message was not the awaited reply.
     ///
     /// Error codes returned by [`MessageFromGsp::read`] are propagated as-is.
-    fn receive_msg<M: MessageFromGsp>(&mut self, timeout: Delta) -> Result<M>
+    fn receive_msg<M: MessageFromGsp>(
+        &mut self,
+        timeout: Delta,
+        expected_seq: Option<u32>,
+    ) -> Result<M>
     where
         // This allows all error types, including `Infallible`, to be used for `M::InitError`.
         Error: From<M::InitError>,
@@ -926,9 +944,11 @@ fn receive_msg<M: MessageFromGsp>(&mut self, timeout: Delta) -> Result<M>
         let message = self.wait_for_msg(timeout)?;
         let function = message.header.function();
         let seq = message.header.sequence();
+        let func_matches = matches!(function, Ok(f) if f == M::FUNCTION);
+        let matched = func_matches && expected_seq.is_none_or(|expected| seq == expected);
 
         // An early return here would leave the read pointer on this message.
-        let result = if matches!(function, Ok(f) if f == M::FUNCTION) {
+        let result = if matched {
             match M::Message::from_bytes_prefix(message.contents.0) {
                 Some((cmd, contents_1)) => {
                     let mut sbuffer = SBufferIter::new_reader([contents_1, message.contents.1]);
@@ -951,7 +971,17 @@ fn receive_msg<M: MessageFromGsp>(&mut self, timeout: Delta) -> Result<M>
                 }
             }
         } else {
-            self.log_event(function, seq);
+            if func_matches {
+                dev_warn!(
+                    &self.dev,
+                    "GSP RPC: dropping stale {:?} reply (seq {}, awaiting {:?})\n",
+                    M::FUNCTION,
+                    seq,
+                    expected_seq,
+                );
+            } else {
+                self.log_event(function, seq);
+            }
 
             Err(ENOMSG)
         };
@@ -967,7 +997,7 @@ fn receive_msg<M: MessageFromGsp>(&mut self, timeout: Delta) -> Result<M>
     /// Receives a message of type `M`, waiting up to [`Cmdq::RECEIVE_TIMEOUT`] from the call.
     ///
     /// Any other message that arrives first is logged as an event and does not extend the
-    /// deadline.
+    /// deadline. `expected_seq` narrows the match as [`Self::receive_msg`] describes.
     ///
     /// # Errors
     ///
@@ -977,7 +1007,7 @@ fn receive_msg<M: MessageFromGsp>(&mut self, timeout: Delta) -> Result<M>
     ///   [`Self::wait_for_msg`]).
     ///
     /// Error codes returned by [`MessageFromGsp::read`] are propagated as-is.
-    fn await_msg<M: MessageFromGsp>(&mut self) -> Result<M>
+    fn await_msg<M: MessageFromGsp>(&mut self, expected_seq: Option<u32>) -> Result<M>
     where
         // This allows all error types, including `Infallible`, to be used for `M::InitError`.
         Error: From<M::InitError>,
@@ -988,7 +1018,7 @@ fn await_msg<M: MessageFromGsp>(&mut self) -> Result<M>
             if remaining.is_negative() {
                 break Err(ETIMEDOUT);
             }
-            match self.receive_msg::<M>(remaining) {
+            match self.receive_msg::<M>(remaining, expected_seq) {
                 Ok(msg) => break Ok(msg),
                 Err(ENOMSG) => continue,
                 Err(e) => break Err(e),
diff --git a/drivers/gpu/nova-core/gsp/fw.rs b/drivers/gpu/nova-core/gsp/fw.rs
index 14271fbe0c25..ced7da14c0b2 100644
--- a/drivers/gpu/nova-core/gsp/fw.rs
+++ b/drivers/gpu/nova-core/gsp/fw.rs
@@ -470,13 +470,14 @@ fn new() -> Self {
 }
 
 impl bindings::rpc_message_header_v {
-    fn init(cmd_size: usize, function: MsgFunction) -> impl Init<Self, Error> {
+    fn init(sequence: u32, cmd_size: usize, function: MsgFunction) -> impl Init<Self, Error> {
         type RpcMessageHeader = bindings::rpc_message_header_v;
 
         try_init!(RpcMessageHeader {
             header_version: MsgHeaderVersion::new().into(),
             signature: bindings::NV_VGPU_MSG_SIGNATURE_VALID,
             function: function.into(),
+            sequence,
             length: size_of::<Self>()
                 .checked_add(cmd_size)
                 .ok_or(EOVERFLOW)
@@ -503,8 +504,12 @@ pub(crate) struct GspMsgElement {
 
 impl GspMsgElement {
     /// Creates the queue element header and the RPC header of a command with a `cmd_size`-byte
-    /// payload.
-    pub(crate) fn init(cmd_size: usize, function: MsgFunction) -> impl Init<Self, Error> {
+    /// payload and the RPC sequence number `rpc_seq`.
+    pub(crate) fn init(
+        rpc_seq: u32,
+        cmd_size: usize,
+        function: MsgFunction,
+    ) -> impl Init<Self, Error> {
         type RpcMessageHeader = bindings::rpc_message_header_v;
 
         try_init!(GspMsgElement {
@@ -514,7 +519,7 @@ pub(crate) fn init(cmd_size: usize, function: MsgFunction) -> impl Init<Self, Er
                     .checked_add(cmd_size)
                     .ok_or(EOVERFLOW)?,
             )?,
-            rpc <- RpcMessageHeader::init(cmd_size, function),
+            rpc <- RpcMessageHeader::init(rpc_seq, cmd_size, function),
         })
     }
 
-- 
2.55.0