[PATCH v3 0/2] gpu: nova: fix incorrect GSP barrier usage

[PATCH v3 0/2] gpu: nova: fix incorrect GSP barrier usage

[Gary Guo]

Currently Nova GSP code uses Rust barriers, which equivalent to LKMM SMP
barriers. This does not provide sufficient ordering for DMA operations on
AArch64. Fix this to be using the newly introduced `dma_mb(Ordering)` API.

Also, there is an incorrect placement of barrier, which is also fixed.

This should wait until v7.3-rc1 backmerge to drm-rust-next occurs.

---
Changes in v3:
- Drop abstraction changes which are already picked.
- Improve ordering comment messages.
- Replace control dependency reliance with explicit barrier.
- Link to v2: https://patch.msgid.link/20260609-rust-barrier-v2-0-30fcc48e1cd0@garyguo.net

Changes in v2:
- Dropped `Acquire` and `Release` aliases of `Full` (Joel)
- Use macros to implement most `MemoryBarrier`
- Split Nova change to GSP->CPU commit and CPU->GSP commit (Joel)
- Link to v1: https://patch.msgid.link/20260402152443.1059634-2-gary@kernel.org

To: Danilo Krummrich <dakr@kernel.org>
To: Alice Ryhl <aliceryhl@google.com>
To: Alexandre Courbot <acourbot@nvidia.com>
To: David Airlie <airlied@gmail.com>
To: Simona Vetter <simona@ffwll.ch>
Cc: nova-gpu@lists.linux.dev
Cc: dri-devel@lists.freedesktop.org
Cc: linux-kernel@vger.kernel.org

---
Gary Guo (2):
      gpu: nova-core: fix barrier usage in CPU->GSP messaging path
      gpu: nova-core: fix barrier usage in GSP->CPU messaging path

 drivers/gpu/nova-core/gsp/cmdq.rs | 34 ++++++++++++++++++++--------------
 1 file changed, 20 insertions(+), 14 deletions(-)
---
base-commit: e6664f2b33db9b6811eb4cec109f06cb2b4f458d
change-id: 20260609-rust-barrier-63078ea76216

Best regards,
--  
Gary Guo <gary@garyguo.net>

[PATCH v3 1/2] gpu: nova-core: fix barrier usage in CPU->GSP messaging path

[Gary Guo]

In the CPU->GSP messaging path, the code reads the read pointer from GSP,
writes the command, advances the write pointer, and then notifies the GSP.

A LOAD->STORE ordering is needed after reading the read pointer from GSP
and writing the command. Control dependency exists here which provide the
needed ordering, but it's best to avoid depending on it.

A STORE->STORE ordering is needed after the command write and before the
write pointer advance. This is currently incorrectly done after the write
pointer advance (and before GSP notification), but this can cause issue if
GSP is still processing ring buffer, as it may observe the write pointer
advance before command write. Thus move this barrier to be before the write
pointer advance. Note that barriers are not needed between write pointer
advance and GSP notification, as MMIO accessors already carries the
required barrier.

Signed-off-by: Gary Guo <gary@garyguo.net>
---
 drivers/gpu/nova-core/gsp/cmdq.rs | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/nova-core/gsp/cmdq.rs b/drivers/gpu/nova-core/gsp/cmdq.rs
index 6da728201281..70674d2d0f77 100644
--- a/drivers/gpu/nova-core/gsp/cmdq.rs
+++ b/drivers/gpu/nova-core/gsp/cmdq.rs
@@ -27,6 +27,11 @@
     ptr,
     sync::{
         aref::ARef,
+        barrier::{
+            dma_mb,
+            Full,
+            Write, //
+        },
         Mutex, //
     },
     time::Delta,
@@ -272,6 +277,10 @@ fn new(dev: &device::Device<device::Bound>) -> Result<Self> {
             (rx - 1, 0)
         };
 
+        // ORDERING: LOAD->STORE ordering needed to order `gsp_read_ptr` read before data write.
+        // Control dependency can serve the same purpose here, but we don't want to rely on it.
+        dma_mb(Full);
+
         // SAFETY:
         // - `data` was created from a valid pointer, and `rx` and `tx` are in the
         //   `0..MSGQ_NUM_PAGES` range per the invariants of `cpu_write_ptr` and `gsp_read_ptr`,
@@ -450,9 +459,6 @@ fn advance_cpu_write_ptr(&mut self, elem_count: u32) {
         let tx = io_project!(self.0, .cpuq.tx);
         let wptr = MsgqTxHeader::write_ptr(tx).wrapping_add(elem_count) % MSGQ_NUM_PAGES;
         MsgqTxHeader::set_write_ptr(tx, wptr);
-
-        // Ensure all command data is visible before triggering the GSP read.
-        fence(Ordering::SeqCst);
     }
 }
 
@@ -683,6 +689,9 @@ fn send_single_command<M>(&mut self, bar: Bar0<'_>, command: M) -> Result
             dst.header.length(),
         );
 
+        // ORDERING: STORE->STORE ordering needed to order `cpu_write_ptr` write after data write.
+        dma_mb(Write);
+
         // All set - update the write pointer and inform the GSP of the new command.
         let elem_count = dst.header.element_count();
         self.seq += 1;

-- 
2.54.0

Re: [PATCH v3 1/2] gpu: nova-core: fix barrier usage in CPU->GSP messaging path

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Missing LOAD->LOAD DMA memory barrier in the GSP->CPU receive path, risking stale data reads.
- [High] Use of CPU-centric `fence(Ordering::SeqCst)` instead of a DMA barrier in `advance_cpu_read_ptr`.
--

--- Patch [1]: [PATCH v3 1/2] gpu: nova-core: fix barrier usage in CPU->GSP messaging path ---
commit 4ff1d5624b6cb01f484147f0e0780dbdb7b0b09e
Author: Gary Guo <gary@garyguo.net>

gpu: nova-core: fix barrier usage in CPU->GSP messaging path

In the CPU->GSP messaging path, the code reads the read pointer from GSP,
writes the command, advances the write pointer, and then notifies the GSP.
This commit adds a load-to-store ordering, and moves a store-to-store
ordering to occur before the write pointer advance.

> diff --git a/drivers/gpu/nova-core/gsp/cmdq.rs b/drivers/gpu/nova-core/gsp/cmdq.rs
> index 6da728201281b..70674d2d0f77e 100644
> --- a/drivers/gpu/nova-core/gsp/cmdq.rs
> +++ b/drivers/gpu/nova-core/gsp/cmdq.rs
[ ... ]
> @@ -450,9 +459,6 @@ fn advance_cpu_write_ptr(&mut self, elem_count: u32) {
>          let tx = io_project!(self.0, .cpuq.tx);
>          let wptr = MsgqTxHeader::write_ptr(tx).wrapping_add(elem_count) % MSGQ_NUM_PAGES;
>          MsgqTxHeader::set_write_ptr(tx, wptr);
> -
> -        // Ensure all command data is visible before triggering the GSP read.
> -        fence(Ordering::SeqCst);
>      }
>  }

[Severity: High]
This is a pre-existing issue, but I noticed there might be a missing
load-to-load DMA memory barrier in the GSP to CPU receive path.

When the driver receives a message via wait_for_msg(), it polls the GSP
write pointer by calling driver_read_area(), which reads the pointer using
io_read!. After the pointer indicates new data is available, the CPU directly
reads the message payload without any intervening memory barrier:

drivers/gpu/nova-core/gsp/cmdq.rs:driver_read_area() {
    ...
        let tx = self.gsp_write_ptr();
        let rx = self.cpu_read_ptr();

        // Pointer to the first entry of the GSP message queue.
        let data = ptr::project!(self.0.as_ptr(), .gspq.msgq.data[build: 0]);

        let (tail_end, wrap_end) = if rx <= tx {
    ...
}

drivers/gpu/nova-core/gsp/cmdq.rs:wait_for_msg() {
    ...
        )
        .map(|(slice_1, slice_2)| (slice_1.as_flattened(), slice_2.as_flattened()))?;

        // Extract the GspMsgElement.
        let (header, slice_1) = GspMsgElement::from_bytes_prefix(slice_1).ok_or(EIO)?;
    ...
}

On weakly-ordered architectures, can the CPU speculatively execute the payload
data reads from the coherent DMA buffer before the read of the updated write
pointer has completed? Does this require a read-to-read DMA barrier (e.g.,
dma_mb(Read)) after reading the write pointer and before accessing the
payload?

[Severity: High]
This is also a pre-existing issue, but since the patch removes a CPU-centric
fence above, I noticed that advance_cpu_read_ptr() still uses one where a DMA
barrier might be required:

drivers/gpu/nova-core/gsp/cmdq.rs:advance_cpu_read_ptr() {
    ...
        let rptr = MsgqRxHeader::read_ptr(rx).wrapping_add(elem_count) % MSGQ_NUM_PAGES;

        // Ensure read pointer is properly ordered.
        fence(Ordering::SeqCst);

        MsgqRxHeader::set_read_ptr(rx, rptr)
    ...
}

In Rust, fence(Ordering::SeqCst) only provides an SMP-domain barrier. Since
the queue is in coherent DMA memory shared with a device, could the GSP
observe the advanced read pointer and overwrite the message payload while the
CPU is still reading it?

Should this use an Outer Shareable DMA barrier (like dma_mb(Full)) instead?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260819-rust-barrier-v3-0-d5b7bd7e6624@garyguo.net?part=1

Re: [PATCH v3 1/2] gpu: nova-core: fix barrier usage in CPU->GSP messaging path

[Eliot Courtney]

On Thu Aug 20, 2026 at 2:28 AM JST, Gary Guo wrote:
> In the CPU->GSP messaging path, the code reads the read pointer from GSP,
> writes the command, advances the write pointer, and then notifies the GSP.
>
> A LOAD->STORE ordering is needed after reading the read pointer from GSP
> and writing the command. Control dependency exists here which provide the
> needed ordering, but it's best to avoid depending on it.
>
> A STORE->STORE ordering is needed after the command write and before the
> write pointer advance. This is currently incorrectly done after the write
> pointer advance (and before GSP notification), but this can cause issue if
> GSP is still processing ring buffer, as it may observe the write pointer
> advance before command write. Thus move this barrier to be before the write
> pointer advance. Note that barriers are not needed between write pointer
> advance and GSP notification, as MMIO accessors already carries the
> required barrier.
>
> Signed-off-by: Gary Guo <gary@garyguo.net>
> ---
>  drivers/gpu/nova-core/gsp/cmdq.rs | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/nova-core/gsp/cmdq.rs b/drivers/gpu/nova-core/gsp/cmdq.rs
> index 6da728201281..70674d2d0f77 100644
> --- a/drivers/gpu/nova-core/gsp/cmdq.rs
> +++ b/drivers/gpu/nova-core/gsp/cmdq.rs
> @@ -27,6 +27,11 @@
>      ptr,
>      sync::{
>          aref::ARef,
> +        barrier::{
> +            dma_mb,
> +            Full,
> +            Write, //
> +        },
>          Mutex, //
>      },
>      time::Delta,
> @@ -272,6 +277,10 @@ fn new(dev: &device::Device<device::Bound>) -> Result<Self> {
>              (rx - 1, 0)
>          };
>  
> +        // ORDERING: LOAD->STORE ordering needed to order `gsp_read_ptr` read before data write.
> +        // Control dependency can serve the same purpose here, but we don't want to rely on it.
> +        dma_mb(Full);
> +
>          // SAFETY:
>          // - `data` was created from a valid pointer, and `rx` and `tx` are in the
>          //   `0..MSGQ_NUM_PAGES` range per the invariants of `cpu_write_ptr` and `gsp_read_ptr`,
> @@ -450,9 +459,6 @@ fn advance_cpu_write_ptr(&mut self, elem_count: u32) {
>          let tx = io_project!(self.0, .cpuq.tx);
>          let wptr = MsgqTxHeader::write_ptr(tx).wrapping_add(elem_count) % MSGQ_NUM_PAGES;
>          MsgqTxHeader::set_write_ptr(tx, wptr);
> -
> -        // Ensure all command data is visible before triggering the GSP read.
> -        fence(Ordering::SeqCst);
>      }
>  }
>  
> @@ -683,6 +689,9 @@ fn send_single_command<M>(&mut self, bar: Bar0<'_>, command: M) -> Result
>              dst.header.length(),
>          );
>  
> +        // ORDERING: STORE->STORE ordering needed to order `cpu_write_ptr` write after data write.
> +        dma_mb(Write);
> +

Is there a reason this can't go into `advance_cpu_write_ptr`?

>          // All set - update the write pointer and inform the GSP of the new command.
>          let elem_count = dst.header.element_count();
>          self.seq += 1;

Re: [PATCH v3 1/2] gpu: nova-core: fix barrier usage in CPU->GSP messaging path

[Gary Guo]

On Mon Aug 24, 2026 at 1:50 PM BST, Eliot Courtney wrote:
> On Thu Aug 20, 2026 at 2:28 AM JST, Gary Guo wrote:
>> In the CPU->GSP messaging path, the code reads the read pointer from GSP,
>> writes the command, advances the write pointer, and then notifies the GSP.
>>
>> A LOAD->STORE ordering is needed after reading the read pointer from GSP
>> and writing the command. Control dependency exists here which provide the
>> needed ordering, but it's best to avoid depending on it.
>>
>> A STORE->STORE ordering is needed after the command write and before the
>> write pointer advance. This is currently incorrectly done after the write
>> pointer advance (and before GSP notification), but this can cause issue if
>> GSP is still processing ring buffer, as it may observe the write pointer
>> advance before command write. Thus move this barrier to be before the write
>> pointer advance. Note that barriers are not needed between write pointer
>> advance and GSP notification, as MMIO accessors already carries the
>> required barrier.
>>
>> Signed-off-by: Gary Guo <gary@garyguo.net>
>> ---
>>  drivers/gpu/nova-core/gsp/cmdq.rs | 15 ++++++++++++---
>>  1 file changed, 12 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/nova-core/gsp/cmdq.rs b/drivers/gpu/nova-core/gsp/cmdq.rs
>> index 6da728201281..70674d2d0f77 100644
>> --- a/drivers/gpu/nova-core/gsp/cmdq.rs
>> +++ b/drivers/gpu/nova-core/gsp/cmdq.rs
>> @@ -27,6 +27,11 @@
>>      ptr,
>>      sync::{
>>          aref::ARef,
>> +        barrier::{
>> +            dma_mb,
>> +            Full,
>> +            Write, //
>> +        },
>>          Mutex, //
>>      },
>>      time::Delta,
>> @@ -272,6 +277,10 @@ fn new(dev: &device::Device<device::Bound>) -> Result<Self> {
>>              (rx - 1, 0)
>>          };
>>  
>> +        // ORDERING: LOAD->STORE ordering needed to order `gsp_read_ptr` read before data write.
>> +        // Control dependency can serve the same purpose here, but we don't want to rely on it.
>> +        dma_mb(Full);
>> +
>>          // SAFETY:
>>          // - `data` was created from a valid pointer, and `rx` and `tx` are in the
>>          //   `0..MSGQ_NUM_PAGES` range per the invariants of `cpu_write_ptr` and `gsp_read_ptr`,
>> @@ -450,9 +459,6 @@ fn advance_cpu_write_ptr(&mut self, elem_count: u32) {
>>          let tx = io_project!(self.0, .cpuq.tx);
>>          let wptr = MsgqTxHeader::write_ptr(tx).wrapping_add(elem_count) % MSGQ_NUM_PAGES;
>>          MsgqTxHeader::set_write_ptr(tx, wptr);
>> -
>> -        // Ensure all command data is visible before triggering the GSP read.
>> -        fence(Ordering::SeqCst);
>>      }
>>  }
>>  
>> @@ -683,6 +689,9 @@ fn send_single_command<M>(&mut self, bar: Bar0<'_>, command: M) -> Result
>>              dst.header.length(),
>>          );
>>  
>> +        // ORDERING: STORE->STORE ordering needed to order `cpu_write_ptr` write after data write.
>> +        dma_mb(Write);
>> +
>
> Is there a reason this can't go into `advance_cpu_write_ptr`?

I think it's more clear to consider `advance_cpu_write_ptr` to just be the
pointer increment, and the ordering should be visible in code that performs both
memory ops.

Best,
Gary

>
>>          // All set - update the write pointer and inform the GSP of the new command.
>>          let elem_count = dst.header.element_count();
>>          self.seq += 1;

Re: [PATCH v3 1/2] gpu: nova-core: fix barrier usage in CPU->GSP messaging path

[Eliot Courtney]

On Mon Aug 24, 2026 at 9:56 PM JST, Gary Guo wrote:
> On Mon Aug 24, 2026 at 1:50 PM BST, Eliot Courtney wrote:
>> On Thu Aug 20, 2026 at 2:28 AM JST, Gary Guo wrote:
>>> In the CPU->GSP messaging path, the code reads the read pointer from GSP,
>>> writes the command, advances the write pointer, and then notifies the GSP.
>>>
>>> A LOAD->STORE ordering is needed after reading the read pointer from GSP
>>> and writing the command. Control dependency exists here which provide the
>>> needed ordering, but it's best to avoid depending on it.
>>>
>>> A STORE->STORE ordering is needed after the command write and before the
>>> write pointer advance. This is currently incorrectly done after the write
>>> pointer advance (and before GSP notification), but this can cause issue if
>>> GSP is still processing ring buffer, as it may observe the write pointer
>>> advance before command write. Thus move this barrier to be before the write
>>> pointer advance. Note that barriers are not needed between write pointer
>>> advance and GSP notification, as MMIO accessors already carries the
>>> required barrier.
>>>
>>> Signed-off-by: Gary Guo <gary@garyguo.net>
>>> ---
>>>  drivers/gpu/nova-core/gsp/cmdq.rs | 15 ++++++++++++---
>>>  1 file changed, 12 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/gpu/nova-core/gsp/cmdq.rs b/drivers/gpu/nova-core/gsp/cmdq.rs
>>> index 6da728201281..70674d2d0f77 100644
>>> --- a/drivers/gpu/nova-core/gsp/cmdq.rs
>>> +++ b/drivers/gpu/nova-core/gsp/cmdq.rs
>>> @@ -27,6 +27,11 @@
>>>      ptr,
>>>      sync::{
>>>          aref::ARef,
>>> +        barrier::{
>>> +            dma_mb,
>>> +            Full,
>>> +            Write, //
>>> +        },
>>>          Mutex, //
>>>      },
>>>      time::Delta,
>>> @@ -272,6 +277,10 @@ fn new(dev: &device::Device<device::Bound>) -> Result<Self> {
>>>              (rx - 1, 0)
>>>          };
>>>  
>>> +        // ORDERING: LOAD->STORE ordering needed to order `gsp_read_ptr` read before data write.
>>> +        // Control dependency can serve the same purpose here, but we don't want to rely on it.
>>> +        dma_mb(Full);
>>> +
>>>          // SAFETY:
>>>          // - `data` was created from a valid pointer, and `rx` and `tx` are in the
>>>          //   `0..MSGQ_NUM_PAGES` range per the invariants of `cpu_write_ptr` and `gsp_read_ptr`,
>>> @@ -450,9 +459,6 @@ fn advance_cpu_write_ptr(&mut self, elem_count: u32) {
>>>          let tx = io_project!(self.0, .cpuq.tx);
>>>          let wptr = MsgqTxHeader::write_ptr(tx).wrapping_add(elem_count) % MSGQ_NUM_PAGES;
>>>          MsgqTxHeader::set_write_ptr(tx, wptr);
>>> -
>>> -        // Ensure all command data is visible before triggering the GSP read.
>>> -        fence(Ordering::SeqCst);
>>>      }
>>>  }
>>>  
>>> @@ -683,6 +689,9 @@ fn send_single_command<M>(&mut self, bar: Bar0<'_>, command: M) -> Result
>>>              dst.header.length(),
>>>          );
>>>  
>>> +        // ORDERING: STORE->STORE ordering needed to order `cpu_write_ptr` write after data write.
>>> +        dma_mb(Write);
>>> +
>>
>> Is there a reason this can't go into `advance_cpu_write_ptr`?
>
> I think it's more clear to consider `advance_cpu_write_ptr` to just be the
> pointer increment, and the ordering should be visible in code that performs both
> memory ops.

In the second patch, it looks like you're adding the memory barrier
directly in `advance_cpu_read_ptr`. So we'd have one barrier directly in
the code advancing the pointer and one not, which seems asymmetric. I
think it's less error prone to put the barrier in the function so it
can't be misused (and we already have evidence the barriers are easy to
get wrong, since this code was already broken).

>
> Best,
> Gary
>
>>
>>>          // All set - update the write pointer and inform the GSP of the new command.
>>>          let elem_count = dst.header.element_count();
>>>          self.seq += 1;

Re: [PATCH v3 1/2] gpu: nova-core: fix barrier usage in CPU->GSP messaging path

[Gary Guo]

On Mon Aug 24, 2026 at 2:03 PM BST, Eliot Courtney wrote:
> On Mon Aug 24, 2026 at 9:56 PM JST, Gary Guo wrote:
>> On Mon Aug 24, 2026 at 1:50 PM BST, Eliot Courtney wrote:
>>> On Thu Aug 20, 2026 at 2:28 AM JST, Gary Guo wrote:
>>>> In the CPU->GSP messaging path, the code reads the read pointer from GSP,
>>>> writes the command, advances the write pointer, and then notifies the GSP.
>>>>
>>>> A LOAD->STORE ordering is needed after reading the read pointer from GSP
>>>> and writing the command. Control dependency exists here which provide the
>>>> needed ordering, but it's best to avoid depending on it.
>>>>
>>>> A STORE->STORE ordering is needed after the command write and before the
>>>> write pointer advance. This is currently incorrectly done after the write
>>>> pointer advance (and before GSP notification), but this can cause issue if
>>>> GSP is still processing ring buffer, as it may observe the write pointer
>>>> advance before command write. Thus move this barrier to be before the write
>>>> pointer advance. Note that barriers are not needed between write pointer
>>>> advance and GSP notification, as MMIO accessors already carries the
>>>> required barrier.
>>>>
>>>> Signed-off-by: Gary Guo <gary@garyguo.net>
>>>> ---
>>>>  drivers/gpu/nova-core/gsp/cmdq.rs | 15 ++++++++++++---
>>>>  1 file changed, 12 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/nova-core/gsp/cmdq.rs b/drivers/gpu/nova-core/gsp/cmdq.rs
>>>> index 6da728201281..70674d2d0f77 100644
>>>> --- a/drivers/gpu/nova-core/gsp/cmdq.rs
>>>> +++ b/drivers/gpu/nova-core/gsp/cmdq.rs
>>>> @@ -27,6 +27,11 @@
>>>>      ptr,
>>>>      sync::{
>>>>          aref::ARef,
>>>> +        barrier::{
>>>> +            dma_mb,
>>>> +            Full,
>>>> +            Write, //
>>>> +        },
>>>>          Mutex, //
>>>>      },
>>>>      time::Delta,
>>>> @@ -272,6 +277,10 @@ fn new(dev: &device::Device<device::Bound>) -> Result<Self> {
>>>>              (rx - 1, 0)
>>>>          };
>>>>  
>>>> +        // ORDERING: LOAD->STORE ordering needed to order `gsp_read_ptr` read before data write.
>>>> +        // Control dependency can serve the same purpose here, but we don't want to rely on it.
>>>> +        dma_mb(Full);
>>>> +
>>>>          // SAFETY:
>>>>          // - `data` was created from a valid pointer, and `rx` and `tx` are in the
>>>>          //   `0..MSGQ_NUM_PAGES` range per the invariants of `cpu_write_ptr` and `gsp_read_ptr`,
>>>> @@ -450,9 +459,6 @@ fn advance_cpu_write_ptr(&mut self, elem_count: u32) {
>>>>          let tx = io_project!(self.0, .cpuq.tx);
>>>>          let wptr = MsgqTxHeader::write_ptr(tx).wrapping_add(elem_count) % MSGQ_NUM_PAGES;
>>>>          MsgqTxHeader::set_write_ptr(tx, wptr);
>>>> -
>>>> -        // Ensure all command data is visible before triggering the GSP read.
>>>> -        fence(Ordering::SeqCst);
>>>>      }
>>>>  }
>>>>  
>>>> @@ -683,6 +689,9 @@ fn send_single_command<M>(&mut self, bar: Bar0<'_>, command: M) -> Result
>>>>              dst.header.length(),
>>>>          );
>>>>  
>>>> +        // ORDERING: STORE->STORE ordering needed to order `cpu_write_ptr` write after data write.
>>>> +        dma_mb(Write);
>>>> +
>>>
>>> Is there a reason this can't go into `advance_cpu_write_ptr`?
>>
>> I think it's more clear to consider `advance_cpu_write_ptr` to just be the
>> pointer increment, and the ordering should be visible in code that performs both
>> memory ops.
>
> In the second patch, it looks like you're adding the memory barrier
> directly in `advance_cpu_read_ptr`. So we'd have one barrier directly in
> the code advancing the pointer and one not, which seems asymmetric. I
> think it's less error prone to put the barrier in the function so it
> can't be misused (and we already have evidence the barriers are easy to
> get wrong, since this code was already broken).

In the second one `message.header.length()` is read, so if I move the barrier to
before the advance it'll be incorrect.

Best,
Gary

Re: [PATCH v3 1/2] gpu: nova-core: fix barrier usage in CPU->GSP messaging path

[Eliot Courtney]

On Mon Aug 24, 2026 at 10:07 PM JST, Gary Guo wrote:
> On Mon Aug 24, 2026 at 2:03 PM BST, Eliot Courtney wrote:
>> On Mon Aug 24, 2026 at 9:56 PM JST, Gary Guo wrote:
>>> On Mon Aug 24, 2026 at 1:50 PM BST, Eliot Courtney wrote:
>>>> On Thu Aug 20, 2026 at 2:28 AM JST, Gary Guo wrote:
>>>>> In the CPU->GSP messaging path, the code reads the read pointer from GSP,
>>>>> writes the command, advances the write pointer, and then notifies the GSP.
>>>>>
>>>>> A LOAD->STORE ordering is needed after reading the read pointer from GSP
>>>>> and writing the command. Control dependency exists here which provide the
>>>>> needed ordering, but it's best to avoid depending on it.
>>>>>
>>>>> A STORE->STORE ordering is needed after the command write and before the
>>>>> write pointer advance. This is currently incorrectly done after the write
>>>>> pointer advance (and before GSP notification), but this can cause issue if
>>>>> GSP is still processing ring buffer, as it may observe the write pointer
>>>>> advance before command write. Thus move this barrier to be before the write
>>>>> pointer advance. Note that barriers are not needed between write pointer
>>>>> advance and GSP notification, as MMIO accessors already carries the
>>>>> required barrier.
>>>>>
>>>>> Signed-off-by: Gary Guo <gary@garyguo.net>
>>>>> ---
>>>>>  drivers/gpu/nova-core/gsp/cmdq.rs | 15 ++++++++++++---
>>>>>  1 file changed, 12 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/nova-core/gsp/cmdq.rs b/drivers/gpu/nova-core/gsp/cmdq.rs
>>>>> index 6da728201281..70674d2d0f77 100644
>>>>> --- a/drivers/gpu/nova-core/gsp/cmdq.rs
>>>>> +++ b/drivers/gpu/nova-core/gsp/cmdq.rs
>>>>> @@ -27,6 +27,11 @@
>>>>>      ptr,
>>>>>      sync::{
>>>>>          aref::ARef,
>>>>> +        barrier::{
>>>>> +            dma_mb,
>>>>> +            Full,
>>>>> +            Write, //
>>>>> +        },
>>>>>          Mutex, //
>>>>>      },
>>>>>      time::Delta,
>>>>> @@ -272,6 +277,10 @@ fn new(dev: &device::Device<device::Bound>) -> Result<Self> {
>>>>>              (rx - 1, 0)
>>>>>          };
>>>>>  
>>>>> +        // ORDERING: LOAD->STORE ordering needed to order `gsp_read_ptr` read before data write.
>>>>> +        // Control dependency can serve the same purpose here, but we don't want to rely on it.
>>>>> +        dma_mb(Full);
>>>>> +
>>>>>          // SAFETY:
>>>>>          // - `data` was created from a valid pointer, and `rx` and `tx` are in the
>>>>>          //   `0..MSGQ_NUM_PAGES` range per the invariants of `cpu_write_ptr` and `gsp_read_ptr`,
>>>>> @@ -450,9 +459,6 @@ fn advance_cpu_write_ptr(&mut self, elem_count: u32) {
>>>>>          let tx = io_project!(self.0, .cpuq.tx);
>>>>>          let wptr = MsgqTxHeader::write_ptr(tx).wrapping_add(elem_count) % MSGQ_NUM_PAGES;
>>>>>          MsgqTxHeader::set_write_ptr(tx, wptr);
>>>>> -
>>>>> -        // Ensure all command data is visible before triggering the GSP read.
>>>>> -        fence(Ordering::SeqCst);
>>>>>      }
>>>>>  }
>>>>>  
>>>>> @@ -683,6 +689,9 @@ fn send_single_command<M>(&mut self, bar: Bar0<'_>, command: M) -> Result
>>>>>              dst.header.length(),
>>>>>          );
>>>>>  
>>>>> +        // ORDERING: STORE->STORE ordering needed to order `cpu_write_ptr` write after data write.
>>>>> +        dma_mb(Write);
>>>>> +
>>>>
>>>> Is there a reason this can't go into `advance_cpu_write_ptr`?
>>>
>>> I think it's more clear to consider `advance_cpu_write_ptr` to just be the
>>> pointer increment, and the ordering should be visible in code that performs both
>>> memory ops.
>>
>> In the second patch, it looks like you're adding the memory barrier
>> directly in `advance_cpu_read_ptr`. So we'd have one barrier directly in
>> the code advancing the pointer and one not, which seems asymmetric. I
>> think it's less error prone to put the barrier in the function so it
>> can't be misused (and we already have evidence the barriers are easy to
>> get wrong, since this code was already broken).
>
> In the second one `message.header.length()` is read, so if I move the barrier to
> before the advance it'll be incorrect.
>
> Best,
> Gary

Yerp I mean move the barrier into `advance_cpu_write_ptr` not move the
barrier out of `advance_cpu_read_ptr` - I agree that'd be incorrect. On
clearness, it feels very odd to me to have these two functions
(advance_cpu_read_ptr, advance_cpu_write_ptr) where one controls the
memory barrier and one doesn't, purely based off the structure of the
callers. And I still think it's less error prone (for future changes) to
do it this way too.

[PATCH v3 2/2] gpu: nova-core: fix barrier usage in GSP->CPU messaging path

[Gary Guo]

In the GSP->CPU messaging path, the code reads the write pointer from GSP,
reads the response and advances the read pointer.

A LOAD->LOAD ordering is required after the write pointer read and the data
read. Add it as this is currently missing.

A LOAD->STORE ordering is required after the data read and the advance of
read pointer. Currently a Rust `SeqCst` barrier is used, which roughly maps
to `smp_mb(Full)`; this however does not order DMA operations (notably on
ARM, the generate barrier orders inner shareable and not outer shareable,
which is ordered by `dma_mb`). This ordering does not need to be in between
read pointer read and write, because it's for ordering between the ring
buffer data and the pointer; the RMW operation does not internally need a
barrier (nor it has to be atomic, as CPU pointers are updated by CPU only),
so move it to before the RMW sequence for clarity.

Signed-off-by: Gary Guo <gary@garyguo.net>
---
 drivers/gpu/nova-core/gsp/cmdq.rs | 19 ++++++++-----------
 1 file changed, 8 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/nova-core/gsp/cmdq.rs b/drivers/gpu/nova-core/gsp/cmdq.rs
index 70674d2d0f77..9fe393da6b10 100644
--- a/drivers/gpu/nova-core/gsp/cmdq.rs
+++ b/drivers/gpu/nova-core/gsp/cmdq.rs
@@ -2,13 +2,7 @@
 
 mod continuation;
 
-use core::{
-    mem,
-    sync::atomic::{
-        fence,
-        Ordering, //
-    },
-};
+use core::mem;
 
 use kernel::{
     device,
@@ -30,6 +24,7 @@
         barrier::{
             dma_mb,
             Full,
+            Read,
             Write, //
         },
         Mutex, //
@@ -339,6 +334,9 @@ fn driver_write_area_size(&self) -> usize {
             (MSGQ_NUM_PAGES, tx)
         };
 
+        // ORDERING: LOAD->LOAD ordering needed to order `gsp_write_ptr` read before data read.
+        dma_mb(Read);
+
         // SAFETY:
         // - `data` was created from a valid pointer, and `rx` and `tx` are in the
         //   `0..MSGQ_NUM_PAGES` range per the invariants of `gsp_write_ptr` and `cpu_read_ptr`,
@@ -436,12 +434,11 @@ fn cpu_read_ptr(&self) -> u32 {
 
     // Informs the GSP that it can send `elem_count` new pages into the message queue.
     fn advance_cpu_read_ptr(&mut self, elem_count: u32) {
+        // ORDERING: LOAD->STORE ordering needed to order `cpu_read_ptr` write after data read.
+        dma_mb(Full);
+
         let rx = io_project!(self.0, .cpuq.rx);
         let rptr = MsgqRxHeader::read_ptr(rx).wrapping_add(elem_count) % MSGQ_NUM_PAGES;
-
-        // Ensure read pointer is properly ordered.
-        fence(Ordering::SeqCst);
-
         MsgqRxHeader::set_read_ptr(rx, rptr)
     }
 

-- 
2.54.0

Re: [PATCH v3 2/2] gpu: nova-core: fix barrier usage in GSP->CPU messaging path

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Early returns in message parsing skip advancing the CPU read pointer, permanently wedging the command queue.
--

--- Patch [2]: [PATCH v3 2/2] gpu: nova-core: fix barrier usage in GSP->CPU messaging path ---
commit fab176fb9e47452c5f023d1bbf02cec260583fbf
Author: Gary Guo <gary@garyguo.net>

gpu: nova-core: fix barrier usage in GSP->CPU messaging path

In the GSP->CPU messaging path, the code reads the write pointer from GSP,
reads the response and advances the read pointer.

A LOAD->LOAD ordering is required after the write pointer read and the data
read. Add it as this is currently missing.

A LOAD->STORE ordering is required after the data read and the advance of
read pointer. Currently a Rust `SeqCst` barrier is used, which roughly maps
to `smp_mb(Full)`; this however does not order DMA operations (notably on
ARM, the generate barrier orders inner shareable and not outer shareable,
which is ordered by `dma_mb`). This ordering does not need to be in between
read pointer read and write, because it's for ordering between the ring
buffer data and the pointer; the RMW operation does not internally need a
barrier (nor it has to be atomic, as CPU pointers are updated by CPU only),
so move it to before the RMW sequence for clarity.

Signed-off-by: Gary Guo <gary@garyguo.net>

> diff --git a/drivers/gpu/nova-core/gsp/cmdq.rs b/drivers/gpu/nova-core/gsp/cmdq.rs
> index 70674d2d0f77e..9fe393da6b10c 100644
> --- a/drivers/gpu/nova-core/gsp/cmdq.rs
> +++ b/drivers/gpu/nova-core/gsp/cmdq.rs

[ ... ]

> @@ -436,12 +434,11 @@ fn cpu_read_ptr(&self) -> u32 {
>  
>      // Informs the GSP that it can send `elem_count` new pages into the message queue.
>      fn advance_cpu_read_ptr(&mut self, elem_count: u32) {
> +        // ORDERING: LOAD->STORE ordering needed to order `cpu_read_ptr` write after data read.
> +        dma_mb(Full);
> +
>          let rx = io_project!(self.0, .cpuq.rx);
>          let rptr = MsgqRxHeader::read_ptr(rx).wrapping_add(elem_count) % MSGQ_NUM_PAGES;
> -
> -        // Ensure read pointer is properly ordered.
> -        fence(Ordering::SeqCst);
> -
>          MsgqRxHeader::set_read_ptr(rx, rptr)
>      }

[Severity: High]
This isn't a bug introduced by this patch, but does CmdqInner::receive_msg()
skip advancing the CPU read pointer on parsing errors?

Looking at CmdqInner::receive_msg(), if the GSP sends a message with an
unrecognized function code or a truncated prefix, do the `?` operators
cause an early return?

drivers/gpu/nova-core/gsp/cmdq.rs:CmdqInner::receive_msg() {
        let message = self.wait_for_msg(timeout)?;
        let function = message.header.function().map_err(|_| EINVAL)?;

        // Extract the message. Store the result as we want to advance the read pointer even in
        // case of failure.
        let result = if function == M::FUNCTION {
            let (cmd, contents_1) = M::Message::from_bytes_prefix(message.contents.0).ok_or(EIO)?;
        ...

If an early return happens here, self.gsp_mem.advance_cpu_read_ptr() at the end
of the function is never called, despite the explicit comment's intent.

Will the unhandled message remain at the head of the queue, causing
subsequent calls to continually read the same bad message and fail again,
permanently wedging the message queue?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260819-rust-barrier-v3-0-d5b7bd7e6624@garyguo.net?part=2

Re: [PATCH v3 2/2] gpu: nova-core: fix barrier usage in GSP->CPU messaging path

[Eliot Courtney]

On Thu Aug 20, 2026 at 2:28 AM JST, Gary Guo wrote:
> In the GSP->CPU messaging path, the code reads the write pointer from GSP,
> reads the response and advances the read pointer.
>
> A LOAD->LOAD ordering is required after the write pointer read and the data
> read. Add it as this is currently missing.
>
> A LOAD->STORE ordering is required after the data read and the advance of
> read pointer. Currently a Rust `SeqCst` barrier is used, which roughly maps
> to `smp_mb(Full)`; this however does not order DMA operations (notably on
> ARM, the generate barrier orders inner shareable and not outer shareable,
> which is ordered by `dma_mb`). This ordering does not need to be in between
> read pointer read and write, because it's for ordering between the ring
> buffer data and the pointer; the RMW operation does not internally need a
> barrier (nor it has to be atomic, as CPU pointers are updated by CPU only),
> so move it to before the RMW sequence for clarity.
>
> Signed-off-by: Gary Guo <gary@garyguo.net>
> ---

Reviewed-by: Eliot Courtney <ecourtney@nvidia.com>