Re: [PATCH] gpu: nova-core: write initial RPCs before booting GSP-RM

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

Pre-existing issues:
- [High] Missing release memory barrier before updating the CPU write pointer, allowing the GSP to observe uninitialized or incomplete command data.
- [High] Missing acquire memory barrier after observing a new message in the queue allows the CPU to read stale payload data.
--

--- Patch [1]: [PATCH] gpu: nova-core: write initial RPCs before booting GSP-RM ---
commit 3ab88c6e4aa22a470f9df92e225ac238c75c0482
Author: Timur Tabi <[email protected]>
Subject: gpu: nova-core: write initial RPCs before booting GSP-RM

This patch writes the SetSystemInfo and SetRegistry RPCs to the command
queue before booting GSP-RM. This ensures the messages are present when
GSP-RM processes the queue early in boot, fixing a failure to initialize.

> diff --git a/drivers/gpu/nova-core/gsp/boot.rs b/drivers/gpu/nova-core/gsp/boot.rs
> index e03700ee7beaf..874f6e9499f01 100644
> --- a/drivers/gpu/nova-core/gsp/boot.rs
> +++ b/drivers/gpu/nova-core/gsp/boot.rs
> @@ -44,6 +44,11 @@ pub(crate) fn boot(
>  
>          let gsp_fw = KBox::pin_init(GspFirmware::new(dev, chipset), GFP_KERNEL)?;
>  
> +        self.cmdq
> +            .send_command_no_wait(bar, commands::SetSystemInfo::new(pdev, chipset))?;
> +        self.cmdq
> +            .send_command_no_wait(bar, commands::SetRegistry::new(ctx.vgpu.state())?)?;

While reviewing this code, I noticed a couple of pre-existing issues in the
cmdq implementation being used here.

In drivers/gpu/nova-core/gsp/cmdq.rs:advance_cpu_write_ptr():

        MsgqTxHeader::set_write_ptr(tx, wptr);

        // Ensure all command data is visible before triggering the GSP read.
        fence(Ordering::SeqCst);

[Severity: High]
This isn't a bug introduced by this patch, but is the memory barrier placed
incorrectly here?

It looks like the fence is placed after the set_write_ptr() update. Should
it be placed before the pointer update to ensure preceding payload writes are
globally visible to the GSP when it observes the new pointer? If the GSP
polls the queue, could it observe the new write pointer before the payload
writes are visible, leading it to read uninitialized or corrupted commands?

Also in drivers/gpu/nova-core/gsp/cmdq.rs:wait_for_msg():

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

[Severity: High]
This is a pre-existing issue, but is there a missing acquire memory barrier
here?

The read_poll_timeout() observes the updated GSP write pointer via
driver_read_area(), but there doesn't seem to be an acquire barrier before
GspMsgElement::from_bytes_prefix() reads the payload. On weakly-ordered
architectures, could the CPU speculatively read the payload data from memory
before the GSP's write pointer update is strictly observed, resulting in
stale memory reads?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1