Re: [PATCH v6 18/20] rust: dma: drop `dma_read!` and `dma_write!` API

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The memory barrier intended to order command data writes before the doorbell ring is placed after the doorbell is updated, failing to prevent the hardware from reading stale command data.
- [High] Missing read memory barrier after reading the GSP write pointer doorbell, which can cause the CPU to read stale message queue data due to speculative execution or memory reordering.
--

commit 4f52030a2d0bed4fa2789f818006faedda8d1221
Author: Gary Guo <gary@garyguo.net>

rust: dma: drop dma_read! and dma_write! API

This patch removes the dma_read and dma_write APIs. It states that primitive
read and write use cases are now covered by the io_read and io_write macros,
and non-primitive cases should use CoherentBox or as_ref/as_mut to ensure
lack of concurrent access.

[ ... ]

> --- a/drivers/gpu/nova-core/gsp/cmdq.rs
> +++ b/drivers/gpu/nova-core/gsp/cmdq.rs
> @@ -317,7 +317,7 @@
>      fn driver_read_area(&self) -> (&[[u8; GSP_PAGE_SIZE]], &[[u8; GSP_PAGE_SIZE]]) {
>          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 {

[Severity: High]
This isn't a bug introduced by this patch, but is a read memory barrier needed
after reading the GSP write pointer doorbell?

When polling the GSP write pointer via self.gsp_write_ptr(), there is no
intervening Acquire memory barrier (such as fence(Ordering::Acquire) or
dma_rmb()) before the buffer is accessed via slice creation.

On weakly-ordered memory models like ARM64, could the CPU speculatively prefetch
the message data before the doorbell read completes or is validated, causing the
driver to read stale message queue data?

>              // Read area is non-wrapping and stops right before tx.
>              (tx, 0)

[ ... ]

> @@ -448,7 +448,7 @@
>      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 is the memory barrier placed correctly here?

The fence is placed after the volatile write to the doorbell
(MsgqTxHeader::set_write_ptr). Without a barrier before the doorbell write,
couldn't the CPU or compiler reorder the doorbell volatile write to become
globally visible before the preceding command data writes?

If so, could the GSP observe the updated doorbell and process the command before
the payload is actually flushed to memory, leading to the hardware reading stale
command data?

>      }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260706-io_projection-v6-0-72cd5d055d54@garyguo.net?part=18