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 <[email protected]> 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.
