Re: [PATCH 2/2] gpu: nova-core: use FromSafeCast wherever possible

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Missing DMA write memory barrier before updating the GSP command queue write pointer.
- [High] Missing DMA read memory barrier after reading the GSP message queue write pointer.
--

commit 899e3ba6af7f4cc7e2c28b0f4f3f73db18de1f16
Author: Alexandre Courbot <acourbot@nvidia.com>

gpu: nova-core: use FromSafeCast wherever possible

Replaces various *_as_* casting functions with FromSafeCast in
non-const contexts to simplify conversions.

> diff --git a/drivers/gpu/nova-core/gsp/cmdq.rs b/drivers/gpu/nova-core/gsp/cmdq.rs
> index f85fde09aa6e3..658d0a9b2cfb2 100644
> --- a/drivers/gpu/nova-core/gsp/cmdq.rs
> +++ b/drivers/gpu/nova-core/gsp/cmdq.rs

[ ... ]

> @@ -343,10 +346,10 @@ fn driver_write_area_size(&self) -> usize {
>          unsafe {
>              (
>                  core::slice::from_raw_parts(
> -                    data.add(casts::u32_as_usize(rx)),
> -                    casts::u32_as_usize(tail_end - rx),
> +                    data.add(usize::from_safe_cast(rx)),
> +                    usize::from_safe_cast(tail_end - rx),
>                  ),
> -                core::slice::from_raw_parts(data, casts::u32_as_usize(wrap_end)),
> +                core::slice::from_raw_parts(data, usize::from_safe_cast(wrap_end)),
>              )
>          }
>      }

[Severity: High]
This isn't a bug introduced by this patch, but while reviewing this
file I noticed a potential memory barrier issue in advance_cpu_write_ptr():

>     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);
>     }

Because there is no memory barrier between the payload writes (done prior
to calling this function) and the write pointer volatile write in
MsgqTxHeader::set_write_ptr(), the CPU can reorder them.

If the GSP observes the updated write pointer before the payload writes
are globally visible, could it read stale command data?

[Severity: High]
This is also a pre-existing issue, but is there a missing read memory
barrier after reading the message queue write pointer in wait_for_msg()?

>     fn wait_for_msg(&self, timeout: Delta) -> Result<GspMessage<'_>> {
>         // Wait for a message to arrive from the GSP.
>         let (slice_1, slice_2) = read_poll_timeout(
>             || Ok(self.gsp_mem.driver_read_area()),
>             |driver_area| !driver_area.0.is_empty(),
>             Delta::from_millis(1),
>             timeout,
>         )
>         .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)?;

When polling for a new message, read_poll_timeout repeatedly evaluates
driver_read_area(), which performs a volatile read of the GSP write
pointer.

Once a new message is indicated, the payload memory slices are immediately
accessed.

Since there is no read memory barrier between the volatile read of the
write pointer and the payload access, could modern CPUs speculatively
execute the data loads before the write pointer read completes, causing
the driver to read stale payload data?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260828-nova_num-v1-0-e21f17ba4127@nvidia.com?part=2