[PATCH] Revert "gpu: nova-core: gsp: fix undefined behavior in command queue code"

[PATCH] Revert "gpu: nova-core: gsp: fix undefined behavior in command queue code"

[John Hubbard]

This reverts commit e3dcfad7a3c485456146587f80498348efd4f3de.

The commit uses ptr::project!() infallible range indexing on runtime
values read from DMA-coherent memory. When the compiler cannot prove
the indices are in-bounds, the build_error!() fallback survives
optimization and the rust_build_error symbol remains in the object.
For module builds this causes a modpost failure:

  ERROR: modpost: "rust_build_error" [drivers/gpu/nova-core/nova_core.ko] undefined!

The preceding commit 8815e8427a82 ("gpu: nova-core: gsp: inline methods
providing queue range invariants") adds #[inline(always)] to the pointer
accessors so the optimizer can see the range invariants and eliminate
the build_error!() path. This works on LLVM 21 (rustc 1.93.0) but not on
LLVM 18 (rustc 1.78.0), nor on LLVM 19 (rustc 1.85.0).

The kernel minimum is rustc 1.78.0.

Cc: Alexandre Courbot <acourbot@nvidia.com>
Signed-off-by: John Hubbard <jhubbard@nvidia.com>
---
 drivers/gpu/nova-core/gsp/cmdq.rs | 114 +++++++++++++-----------------
 1 file changed, 49 insertions(+), 65 deletions(-)

diff --git a/drivers/gpu/nova-core/gsp/cmdq.rs b/drivers/gpu/nova-core/gsp/cmdq.rs
index 485d0c0f2a4b..72e9b79619eb 100644
--- a/drivers/gpu/nova-core/gsp/cmdq.rs
+++ b/drivers/gpu/nova-core/gsp/cmdq.rs
@@ -17,7 +17,6 @@
     },
     new_mutex,
     prelude::*,
-    ptr,
     sync::{
         aref::ARef,
         Mutex, //
@@ -256,52 +255,38 @@ fn new(dev: &device::Device<device::Bound>) -> Result<Self> {
     /// As the message queue is a circular buffer, the region may be discontiguous in memory. In
     /// that case the second slice will have a non-zero length.
     fn driver_write_area(&mut self) -> (&mut [[u8; GSP_PAGE_SIZE]], &mut [[u8; GSP_PAGE_SIZE]]) {
-        let tx = num::u32_as_usize(self.cpu_write_ptr());
-        let rx = num::u32_as_usize(self.gsp_read_ptr());
-
-        // Command queue data.
-        let data = ptr::project!(mut self.0.as_mut_ptr(), .cpuq.msgq.data);
-
-        let (tail_slice, wrap_slice) = if rx == 0 {
-            // The write area is non-wrapping, and stops at the second-to-last entry of the command
-            // queue (to leave the last one empty).
-            (
-                ptr::project!(mut data, [tx..num::u32_as_usize(MSGQ_NUM_PAGES) - 1]),
-                ptr::project!(mut data, [..0]),
-            )
-        } else {
-            // Leave an empty slot before `rx`.
-            let end = rx - 1;
-
-            if rx <= tx {
-                // The write area wraps and continues until `end`.
-                (
-                    ptr::project!(mut data, [tx..]),
-                    ptr::project!(mut data, [..end]),
-                )
-            } else {
-                // The write area doesn't wrap and stops at `end`.
-                (
-                    ptr::project!(mut data, [tx..end]),
-                    ptr::project!(mut data, [..0]),
-                )
-            }
-        };
+        let tx = self.cpu_write_ptr() as usize;
+        let rx = self.gsp_read_ptr() as usize;
 
         // SAFETY:
-        // - Since `data` was created from a valid pointer, both `tail_slice` and `wrap_slice` are
-        //   pointers to valid arrays.
-        // - The area starting at `tx` and ending at `rx - 2` modulo `MSGQ_NUM_PAGES`,
-        //   inclusive, belongs to the driver for writing and is not accessed concurrently by
-        //   the GSP.
-        // - The caller holds a reference to `self` for as long as the returned slices are live,
-        //   meaning the CPU write pointer cannot be advanced and thus that the returned area
-        //   remains exclusive to the CPU for the duration of the slices.
-        // - `tail_slice` and `wrap_slice` point to non-overlapping sub-ranges of `data` in all
-        //   branches (in the `rx <= tx` case, `wrap_slice` ends at `rx - 1` which is strictly less
-        //   than `tx` where `tail_slice` starts; in the other cases `wrap_slice` is empty), so
-        //   creating two `&mut` references from them does not violate aliasing rules.
-        (unsafe { &mut *tail_slice }, unsafe { &mut *wrap_slice })
+        // - We will only access the driver-owned part of the shared memory.
+        // - Per the safety statement of the function, no concurrent access will be performed.
+        let gsp_mem = unsafe { &mut *self.0.as_mut() };
+        // PANIC: per the invariant of `cpu_write_ptr`, `tx` is `< MSGQ_NUM_PAGES`.
+        let (before_tx, after_tx) = gsp_mem.cpuq.msgq.data.split_at_mut(tx);
+
+        // The area starting at `tx` and ending at `rx - 2` modulo MSGQ_NUM_PAGES, inclusive,
+        // belongs to the driver for writing.
+
+        if rx == 0 {
+            // Since `rx` is zero, leave an empty slot at end of the buffer.
+            let last = after_tx.len() - 1;
+            (&mut after_tx[..last], &mut [])
+        } else if rx <= tx {
+            // The area is discontiguous and we leave an empty slot before `rx`.
+            // PANIC:
+            // - The index `rx - 1` is non-negative because `rx != 0` in this branch.
+            // - The index does not exceed `before_tx.len()` (which equals `tx`) because
+            //   `rx <= tx` in this branch.
+            (after_tx, &mut before_tx[..(rx - 1)])
+        } else {
+            // The area is contiguous and we leave an empty slot before `rx`.
+            // PANIC:
+            // - The index `rx - tx - 1` is non-negative because `rx > tx` in this branch.
+            // - The index does not exceed `after_tx.len()` (which is `MSGQ_NUM_PAGES - tx`)
+            //   because `rx < MSGQ_NUM_PAGES` by the `gsp_read_ptr` invariant.
+            (&mut after_tx[..(rx - tx - 1)], &mut [])
+        }
     }
 
     /// Returns the size of the region of the CPU message queue that the driver is currently allowed
@@ -323,28 +308,27 @@ fn driver_write_area_size(&self) -> usize {
     /// As the message queue is a circular buffer, the region may be discontiguous in memory. In
     /// that case the second slice will have a non-zero length.
     fn driver_read_area(&self) -> (&[[u8; GSP_PAGE_SIZE]], &[[u8; GSP_PAGE_SIZE]]) {
-        let tx = num::u32_as_usize(self.gsp_write_ptr());
-        let rx = num::u32_as_usize(self.cpu_read_ptr());
-
-        // Message queue data.
-        let data = ptr::project!(self.0.as_ptr(), .gspq.msgq.data);
-
-        let (tail_slice, wrap_slice) = if rx <= tx {
-            (ptr::project!(data, [rx..tx]), ptr::project!(data, [..0]))
-        } else {
-            (ptr::project!(data, [rx..]), ptr::project!(data, [..tx]))
-        };
+        let tx = self.gsp_write_ptr() as usize;
+        let rx = self.cpu_read_ptr() as usize;
 
         // SAFETY:
-        // - Since `data` was created from a valid pointer, both `tail_slice` and `wrap_slice` are
-        //   pointers to valid arrays.
-        // - The area starting at `rx` and ending at `tx - 1` modulo `MSGQ_NUM_PAGES`,
-        //   inclusive, belongs to the driver for reading and is not accessed concurrently by
-        //   the GSP.
-        // - The caller holds a reference to `self` for as long as the returned slices are live,
-        //   meaning the CPU read pointer cannot be advanced and thus that the returned area
-        //   remains exclusive to the CPU for the duration of the slices.
-        (unsafe { &*tail_slice }, unsafe { &*wrap_slice })
+        // - We will only access the driver-owned part of the shared memory.
+        // - Per the safety statement of the function, no concurrent access will be performed.
+        let gsp_mem = unsafe { &*self.0.as_ptr() };
+        let data = &gsp_mem.gspq.msgq.data;
+
+        // The area starting at `rx` and ending at `tx - 1` modulo MSGQ_NUM_PAGES, inclusive,
+        // belongs to the driver for reading.
+        // PANIC:
+        // - per the invariant of `cpu_read_ptr`, `rx < MSGQ_NUM_PAGES`
+        // - per the invariant of `gsp_write_ptr`, `tx < MSGQ_NUM_PAGES`
+        if rx <= tx {
+            // The area is contiguous.
+            (&data[rx..tx], &[])
+        } else {
+            // The area is discontiguous.
+            (&data[rx..], &data[..tx])
+        }
     }
 
     /// Allocates a region on the command queue that is large enough to send a command of `size`

base-commit: 1db742b7ac4dd3fdb7dd62d1e1e6d0faf57f1173
-- 
2.53.0

Re: [PATCH] Revert "gpu: nova-core: gsp: fix undefined behavior in command queue code"

[Miguel Ojeda]

On Sat, Apr 4, 2026 at 2:03 AM John Hubbard <jhubbard@nvidia.com> wrote:
>
> This works on LLVM 21 (rustc 1.93.0) but not on
> LLVM 18 (rustc 1.78.0), nor on LLVM 19 (rustc 1.85.0).
>
> The kernel minimum is rustc 1.78.0.

It will be 1.85.0 this cycle, but a related note in case it helps:
`rustc` supports typically an older LLVM major version even if they
provide a newer one.

For instance, Rust 1.85.0 has a minimum LLVM major of 18, but the
upstream binaries come with LLVM 19.1.7.

Cheers,
Miguel

Re: [PATCH] Revert "gpu: nova-core: gsp: fix undefined behavior in command queue code"

[John Hubbard]

On 4/3/26 5:26 PM, Miguel Ojeda wrote:
> On Sat, Apr 4, 2026 at 2:03 AM John Hubbard <jhubbard@nvidia.com> wrote:
>>
>> This works on LLVM 21 (rustc 1.93.0) but not on
>> LLVM 18 (rustc 1.78.0), nor on LLVM 19 (rustc 1.85.0).
>>
>> The kernel minimum is rustc 1.78.0.
> 
> It will be 1.85.0 this cycle, but a related note in case it helps:

I was really confused about which cycle 1.85.0 would land in, and
still want to confirm: "this cycle" means Linux 7.1?

> `rustc` supports typically an older LLVM major version even if they
> provide a newer one.
> 
> For instance, Rust 1.85.0 has a minimum LLVM major of 18, but the
> upstream binaries come with LLVM 19.1.7.
> 

That is also good to know, thanks.

thanks,
-- 
John Hubbard

Re: [PATCH] Revert "gpu: nova-core: gsp: fix undefined behavior in command queue code"

[Miguel Ojeda]

On Sat, Apr 4, 2026 at 2:31 AM John Hubbard <jhubbard@nvidia.com> wrote:
>
> I was really confused about which cycle 1.85.0 would land in, and
> still want to confirm: "this cycle" means Linux 7.1?

Yeah.

> That is also good to know, thanks.

You're welcome!

Cheers,
Miguel