Re: [PATCH 6/9] gpu: nova-core: generalize `flush_into_kvec` to `flush_into_vec`

[Alexandre Courbot]

On Tue Mar 17, 2026 at 7:49 PM JST, Danilo Krummrich wrote:
> On Tue Mar 17, 2026 at 2:55 AM CET, Alexandre Courbot wrote:
>> We shouldn't be doing that - I think we are limited by the current
>> CoherentAllocation API though. But IIUC this is something that I/O
>> projections will allow us to handle properly?
>
> Why do we need projections to avoid UB here? driver_read_area() already even
> peeks into the firmware abstraction layer, which is where MsgqData technically
> belongs into (despite being trivial).
>
> 	let gsp_mem = &unsafe { self.0.as_slice(0, 1) }.unwrap()[0];
> 	let data = &gsp_mem.gspq.msgq.data;
>
> Why do we need I/O projections to do raw pointer arithmetic where creating a
> reference is UB?
>
> (Eventually, we want to use IoView of course, as this is a textbook example of
> what I proposed IoSlice for.)

Limiting the amount of `unsafe`s, but I guess we can live with that as
this is going to be short-term anyway.

>
> Another option in the meantime would be / have been to use dma_read!() and
> extract (copy) the data right away in driver_read_area(), which I'd probably
> prefer over raw pointer arithmetic.

I'd personally like to keep the current "no-copy" approach as it
implements the right reference discipline (i.e. you need a mutable
reference to update the read pointer, which cannot be done if the buffer
is read by the driver) and moving to copy semantics would open a window
of opportunity to mess with that balance further (on top of requiring
bigger code changes that will be temporary).

>
> But in any case, this can (and should) be fixed even without IoView.
>
> Besides that, nothing prevents us doing the same thing I did for gsp_write_ptr()
> in the meantime to not break out of the firmware abstraction layer.
>
>> This is guaranteed by the inability to update the CPU read pointer for
>> as long as the slices exists.
>
> Fair enough.
>
>> Unless we decide to not trust the GSP, but that would be opening a whole
>> new can of worms.
>
> I thought about this as well, and I think it's fine. The safety comment within
> the function has to justify why the device won't access the memory. If the
> device does so regardless, it's simply a bug.
>
>>> I don't want to merge any code that builds on top of this before we have sorted
>>> this out.
>>
>> If what I have written above is correct, then the fix should simply be
>> to use I/O projections to create properly-bounded references.
>
> I still don't think we need I/O projections for a reasonable fix and I also
> don't agree that we should keep UB until new features land.

I have the following (modulo missing safety comments) to fix
`driver_read_area` - does it look acceptable to you? If so I'll go
ahead and fix `driver_write_area` as well.

diff --git a/drivers/gpu/nova-core/gsp/cmdq.rs b/drivers/gpu/nova-core/gsp/cmdq.rs
index efa1aab1568f..3bddb5a2923f 100644
--- a/drivers/gpu/nova-core/gsp/cmdq.rs
+++ b/drivers/gpu/nova-core/gsp/cmdq.rs
@@ -296,24 +296,53 @@ fn driver_write_area_size(&self) -> usize {
         let tx = self.gsp_write_ptr() as usize;
         let rx = self.cpu_read_ptr() as usize;

+        // Pointer to the start of the GSP message queue.
+        //
         // SAFETY:
-        // - The `CoherentAllocation` contains exactly one object.
-        // - 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_slice(0, 1) }.unwrap()[0];
-        let data = &gsp_mem.gspq.msgq.data;
+        // - `self.0` contains exactly one element.
+        // - `gspq.msgq.data[0]` is within the bounds of that element.
+        let data = unsafe { &raw const (*self.0.start_ptr()).gspq.msgq.data[0] };
+
+        // Safety/Panic comments to be referenced by the code below.
+        //
+        // SAFETY[1]:
+        // - `data` contains `MSGQ_NUM_PAGES` elements.
+        // - 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.
+        //
+        // PANIC[1]:
+        // - Per the invariant of `cpu_read_ptr`, `rx < MSGQ_NUM_PAGES`.
+        // - Per the invariant of `gsp_write_ptr`, `tx < MSGQ_NUM_PAGES`.

-        // 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], &[])
+            (
+                // SAFETY: See SAFETY[1].
+                //
+                // PANIC:
+                // - See PANIC[1].
+                // - Per the branch test, `rx <= tx`.
+                unsafe { core::slice::from_raw_parts(data.add(rx), tx - rx) },
+                &[],
+            )
         } else {
             // The area is discontiguous.
-            (&data[rx..], &data[..tx])
+            (
+                // SAFETY: See SAFETY[1].
+                //
+                // PANIC: See PANIC[1].
+                unsafe {
+                    core::slice::from_raw_parts(
+                        data.add(rx),
+                        num::u32_as_usize(MSGQ_NUM_PAGES) - rx,
+                    )
+                },
+                // SAFETY: See SAFETY[1].
+                //
+                // PANIC: See PANIC[1].
+                unsafe { core::slice::from_raw_parts(data, tx) },
+            )
         }
     }