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

["Gary Guo" <gary@garyguo.net>]

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!`.

best,
Gary