Re: [PATCH] rust: io: use const generics for read/write offsets

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

On Sat Sep 20, 2025 at 5:53 AM JST, Joel Fernandes wrote:
> On Fri, Sep 19, 2025 at 11:26:19AM +0200, Benno Lossin wrote:
>> On Fri Sep 19, 2025 at 9:59 AM CEST, Joel Fernandes wrote:
>> > Hello, Danilo,
>> >
>> >> On Sep 19, 2025, at 1:26 AM, Danilo Krummrich <dakr@kernel.org> wrote:
>> >> 
>> >> On Thu Sep 18, 2025 at 8:13 PM CEST, Joel Fernandes wrote:
>> >>>> On Thu, Sep 18, 2025 at 03:02:11PM +0000, Alice Ryhl wrote:
>> >>>> Using build_assert! to assert that offsets are in bounds is really
>> >>>> fragile and likely to result in spurious and hard-to-debug build
>> >>>> failures. Therefore, build_assert! should be avoided for this case.
>> >>>> Thus, update the code to perform the check in const evaluation
>> >>>> instead.
>> >>> 
>> >>> I really don't think this patch is a good idea (and nobody I spoke to
>> >>> thinks so). Not only does it mess up the user's caller syntax
>> >>> completely, it is also
>> >> 
>> >> I appreacite you raising the concern,
>> >> but I rather have other people speak up
>> >> themselves.
>> >
>> > I did not mean to speak for others, sorry it came across like that
>> > (and that is certainly not what I normally do). But I discussed the
>> > patch in person since we are at a conference and discussing it in
>> > person, and I did not get a lot of consensus on this. That is what I
>> > was trying to say. If it was a brilliant or great idea, I would have
>> > hoped for at least one person to tell me that this is exactly how we
>> > should do it.
>> 
>> I'm also not really thrilled to see lots more turbofish syntax. However,
>> if we can avoid the nasty build_assert errors then in my opinion it's
>> better. (yes we do have Gary's cool klint tool to handle them correctly,
>
> Yes, thanks. Also I tried to apply this patch and it doesn't always work
> because of array indexing usecase in nova, where we compute the offset based
> on a runtime register index  (**/nova-core/**/macros.rs). Here idx is not a
> constant:
>
>             /// Read the array register at index `idx` from its address in `io`.
>             #[inline(always)]
>             pub(crate) fn read<const SIZE: usize, T>(
>                 io: &T,
>                 idx: usize,
>             ) -> Self where
>                 T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
>
> In **/ga102.rs, we have the following usage where ucode_idx cannot be const:
>
> regs::NV_FUSE_OPT_FPF_SEC2_UCODE1_VERSION::read(bar, ucode_idx).data()
>
> So I am afraid this wont work. Also even if it did work, it means now we have
> to also put idx as a const generic (turbofish syntax).

We could always use the `try_read*` variant for these, but that would
introduce runtime checking for errors that can't happen. We have been
pretty successful in avoiding using `try_read*` in Nova so far, and I
think that's something we should try to preserve as it brings confidence
that our register accesses are correct.