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 <[email protected]> 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/[email protected]?part=18
