Re: [PATCH] gpu: nova-core: gsp: fix UB in DmaGspMem pointer accessors

[Danilo Krummrich]

On Wed Mar 11, 2026 at 1:59 PM CET, Gary Guo wrote:
> On Tue Mar 10, 2026 at 10:58 AM GMT, Danilo Krummrich wrote:
>> On Tue Mar 10, 2026 at 3:01 AM CET, Gary Guo wrote:
>>>> +// TODO: Revert to private once `IoView` projections replace the `gsp_mem` module.
>>>> +pub(in crate::gsp) struct Msgq {
>>>
>>> These could all be `(in super)`?
>>
>> Yes, or just pub(super). However, that's not the case for the functions in the
>> gsp_mem module, they could be pub(in super::super) though. But I think I prefer
>> pub(in crate::gsp) for those.
>>
>>>> +    pub(in crate::gsp) fn gsp_write_ptr(qs: &CoherentAllocation<GspMem>) -> u32 {
>>>> +        // PANIC: A `dma::CoherentAllocation` always contains at least one element.
>>>> +        || -> Result<u32> { Ok(dma_read!(qs, [0]?.gspq.tx.0.writePtr) % MSGQ_NUM_PAGES) }().unwrap()
>>>
>>> I wonder if I should add a panicking variant of index projection for this case.
>>> Perhaps of syntax `[index]!`.
>>>
>>> We could also make the existing `[index]` becoming a panicking one instead of
>>> `build_error!` one. It is more consistent with Rust index operator that way.
>>
>> I thought the same, as something like this `[n]?.ptes[i]` looks a bit odd.
>>
>> However, I think we ideally want both variants (I like your `[i]!` proposal
>> above), since generally users should have the choice (as they also have with a
>> slice through get()). For instance, the index could come from userspace. Sure,
>> you can always validate the index in advance, but having a fallible variant is a
>> bit nicer.
>
> I'm not proposing removal of the fallible variant, just that we can make the
> infallible one use panicking instead of `build_error!`.

SGTM.