Re: [PATCH 2/2] gpu: nova-core: gsp: cmdq: increase MSGQ_NUM_PAGES to 64

["Alexandre Courbot" <acourbot@nvidia.com>]

On Thu Nov 12, 2026 at 10:43 AM JST, Eliot Courtney wrote:
> On Wed Nov 11, 2026 at 11:49 PM JST, Alexandre Courbot wrote:
>> On Wed Nov 11, 2026 at 10:37 AM JST, Eliot Courtney wrote:
>>> In the msgq v2 protocol, which the r000 firmware uses, the queue
>>> pointers are kept in u32 registers and increase monotonically until they
>>> wrap at 2^32.
>>>
>>> `MSGQ_NUM_PAGES` is currently 63 and does not divide 2^32. This means
>>> that on msgq v2, the slot can jump backwards when the counters wrap.
>>> For example:
>>>
>>>     (2^32 - 2) % 63 == 2
>>>     (2^32 - 1) % 63 == 3
>>>              0 % 63 == 0    (slot 0 instead of slot 4)
>>>
>>> The writer can then overwrite slots before the reader has read them.
>>>
>>> Increase `MSGQ_NUM_PAGES` to 64 and check at compile time that
>>> `MSGQ_NUM_PAGES` divides 2^32.
>>
>> Have we tested that this was also working on msgq v0?
>
> Yerp I tested boot on turing, ampere, ada, blackwell.
>
>>
>>>
>>> Signed-off-by: Eliot Courtney <ecourtney@nvidia.com>
>>> ---
>>>  drivers/gpu/nova-core/gsp/cmdq.rs | 10 +++++++++-
>>>  1 file changed, 9 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/nova-core/gsp/cmdq.rs b/drivers/gpu/nova-core/gsp/cmdq.rs
>>> index 33074bb53e8c..240ab42db557 100644
>>> --- a/drivers/gpu/nova-core/gsp/cmdq.rs
>>> +++ b/drivers/gpu/nova-core/gsp/cmdq.rs
>>> @@ -84,7 +84,15 @@ trait CommandElement {
>>>  }
>>>  
>>>  /// Number of GSP pages making the [`Msgq`].
>>> -pub(crate) const MSGQ_NUM_PAGES: u32 = 0x3f;
>>> +pub(crate) const MSGQ_NUM_PAGES: u32 = 64;
>>> +
>>> +// From version 2 of the msgq protocol, the queue pointers increase
>>> +// monotonically in `u32` counters until they wrap. If `MSGQ_NUM_PAGES` does not
>>> +// divide `1 << 32`, then the slot, `counter % MSGQ_NUM_PAGES`, can jump
>>> +// backwards when wrapping, which could overwrite slots that still need to be
>>> +// read. So, require that the last counter value, `u32::MAX`, maps to the last
>>> +// slot.
>>
>> That's a pretty convoluted way to say the number of slots must be a
>> power of 2 representable as a u32. :)
>>
>>> +static_assert!(u32::MAX % MSGQ_NUM_PAGES == MSGQ_NUM_PAGES - 1);
>>
>> Same here, can't we use `MSGQ_NUM_PAGES.is_power_of_two()`?
>
> I actually had this originally but I think that this form expresses the
> condition more directly. The condition/issue is what happens when you
> wrap the counter - it should go to the next slot (which necessarily
> needs to be zero since a wrapped counter is 0). So you want
> MSGQ_NUM_PAGES to divide 2^32. So if you write
> MSGQ_NUM_PAGES.is_power_of_two() you are implicitly relying on all
> divisors being a power of two, which is an extra logical step.
>
> But in the end I don't really mind, so lmk wdyt.

The current condition also involves an extra logical step. What you
ideally would have tested is

  (1 << 32).is_multiple_of(MSGQ_NUM_PAGES)

... but as `u32::MAX + 1` is not representable using a u32, the one you
chose is the closest one. It does takes some extra labor to understand
what it intends to convey though.

Trying to put myself into the shoes of someone looking at this code for
the first time, this comment and the condition look a bit hard to chew.
How about using a u64 to test the condition we actually want:

  // The queue pointers are wrapping `u32` counters, and the slot is
  // `counter % MSGQ_NUM_PAGES`. For slots to remain contiguous when a counter
  // wraps, `MSGQ_NUM_PAGES` must divide `1 << 32`.
  static_assert!((1u64 << u32::BITS).is_multiple_of(cv!(MSGQ_NUM_PAGES)));

(I have removed the mention the the msgq v2 detail - it is useful in the
commit log, but becomes unneeded information in the code as it
eventually will only support v2 anyway)

WDYT?