Re: [PATCH v12 15/22] gpu: nova-core: Hopper/Blackwell: add FSP message infrastructure

["Eliot Courtney" <ecourtney@nvidia.com>]

On Wed Jun 3, 2026 at 10:14 AM JST, Alexandre Courbot wrote:
> On Tue Jun 2, 2026 at 12:33 PM JST, sashiko-bot wrote:
>> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
>> - [High] Race condition due to incorrect order of writing queue pointers in `send_msg`.
>> - [High] Silent failure when sending a 4-byte message.
>> - [High] Unchecked integer addition in `poll_msgq` can cause a kernel panic on untrusted MMIO reads.
>> --
>>
>> commit 8a59b49984011320aa631b20a05550242e2baef2
>> Author: John Hubbard <jhubbard@nvidia.com>
>>
>> gpu: nova-core: Hopper/Blackwell: add FSP message infrastructure
>>
>> This commit adds the foundation security processor (FSP) message
>> infrastructure for the nova-core GPU driver. It implements queue
>> registers and low-level helpers to communicate with the FSP falcon's EMEM.
>>
>>> diff --git a/drivers/gpu/nova-core/falcon/fsp.rs b/drivers/gpu/nova-core/falcon/fsp.rs
>>> index 6b057d958115a..0ec1c55213bc8 100644
>>> --- a/drivers/gpu/nova-core/falcon/fsp.rs
>>> +++ b/drivers/gpu/nova-core/falcon/fsp.rs
>> [ ... ]
>>> @@ -145,4 +143,63 @@ fn read_emem(&mut self, bar: &Bar0, offset: u32, data: &mut [u8]) -> Result {
>>>  
>>>          Ok(())
>>>      }
>>> +
>>> +    /// Poll FSP for incoming data.
>>> +    ///
>>> +    /// Returns the size of available data in bytes, or 0 if no data is available.
>>> +    ///
>>> +    /// The FSP message queue is not circular. Pointers are reset to 0 after each
>>> +    /// message exchange, so `tail >= head` is always true when data is present.
>>> +    #[expect(dead_code)]
>>> +    pub(crate) fn poll_msgq(&self, bar: &Bar0) -> u32 {
>>> +        let head = bar.read(regs::NV_PFSP_MSGQ_HEAD).address();
>>> +        let tail = bar.read(regs::NV_PFSP_MSGQ_TAIL).address();
>>> +
>>> +        if head == tail {
>>> +            return 0;
>>> +        }
>>> +
>>> +        // TAIL points at last DWORD written, so add 4 to get total size
>>> +        tail.saturating_sub(head) + 4
>>            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>>
>> [Severity: High]
>> Can this unchecked addition panic if we encounter a PCIe link failure?
>>
>> If `head` reads normally as 0 but a surprise removal occurs before reading
>> `tail`, `tail` could read as 0xFFFFFFFF. The saturating subtraction would
>> leave 0xFFFFFFFF, and the subsequent `+ 4` will overflow, causing a panic in
>> debug builds where CONFIG_RUST_OVERFLOW_CHECKS=y is set.
>>
>> Should this use `saturating_add(4)` or another form of checked math to handle
>> untrusted hardware inputs safely?
>
> This is cheap to fix, and indeed probably a good idea.
>
>>
>>> +    }
>>> +
>>> +    /// Writes `packet` to FSP EMEM and updates the queue pointers to notify FSP.
>>> +    ///
>>> +    /// Returns `EINVAL` if `packet` is empty or its length is not 4-byte aligned.
>>> +    #[expect(dead_code)]
>>> +    pub(crate) fn send_msg(&mut self, bar: &Bar0, packet: &[u8]) -> Result {
>>> +        if packet.is_empty() {
>>> +            return Err(EINVAL);
>>> +        }
>>> +
>>> +        // Write message to EMEM at offset 0 (validates 4-byte alignment)
>>> +        self.write_emem(bar, 0, packet)?;
>>> +
>>> +        // Update queue pointers. TAIL points at the last DWORD written.
>>> +        let tail_offset = u32::try_from(packet.len() - 4).map_err(|_| EINVAL)?;
>>            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>>
>> [Severity: High]
>> Does this create a silent failure if a 4-byte message is sent?
>>
>> If `packet.len()` is 4, `tail_offset` is calculated as 0. Both NV_PFSP_QUEUE_HEAD
>> and NV_PFSP_QUEUE_TAIL will then be written as 0. Because the FSP protocol treats
>> `HEAD == TAIL` as an empty queue, the FSP will ignore the message while the
>> function still returns `Ok(())`.
>>
>> Should this enforce a minimum packet length of 8 bytes or return an error
>> for 4-byte messages?
>
> This one looks bogus (looks like the claim that `HEAD == TAIL` means
> empty queue is hallucinated?).
>
>>
>>> +        bar.write_reg(regs::NV_PFSP_QUEUE_TAIL::zeroed().with_address(tail_offset));
>>> +        bar.write_reg(regs::NV_PFSP_QUEUE_HEAD::zeroed().with_address(0));
>>            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>>
>> [Severity: High]
>> Is there a race condition with the FSP hardware due to the order of these
>> writes?
>>
>> By writing NV_PFSP_QUEUE_TAIL before NV_PFSP_QUEUE_HEAD, if QUEUE_HEAD was
>> non-zero from a prior transaction, we momentarily create a state where
>> `TAIL != HEAD`. The FSP may wake up immediately and read from the old, incorrect
>> QUEUE_HEAD offset before the driver can write 0 to QUEUE_HEAD.
>>
>> Should NV_PFSP_QUEUE_HEAD be initialized to 0 before writing the
>> NV_PFSP_QUEUE_TAIL doorbell?
>
> Here IIUC QUEUE_HEAD is always `0`, so this is a non-issue.

AFAICT FSP looks at the queue when HEAD is written to (so MMIO
triggered) which means that HEAD==0 && TAIL==0 is possible to represent
a 4 byte send, if you disallow sending a 0 byte message. This also means
that there's no race condition here as long as you write HEAD after
writing TAIL (and you must write HEAD as 0 even though it is always 0 to
actually send the message). OTOH, I don't think FSP can ever send back 4
bytes, since that is impossible distinguish from nothing on the CPU
side.