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

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

On Thu Nov 12, 2026 at 2:19 PM JST, Alexandre Courbot wrote:
> 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?

Yeah I originally also tried this but I didn't find as nice a form
(w.r.t. the conversions to u64) as you did, so I will go with your
version. Thanks!