Re: [PATCH 02/11] gpu: nova-core: add ImemNs section infrastructure

[Timur Tabi <ttabi@nvidia.com>]

On Wed, 2025-11-19 at 15:55 +0900, Alexandre Courbot wrote:
> On Wed Nov 19, 2025 at 3:30 PM JST, John Hubbard wrote:
> > On 11/18/25 5:54 PM, Alexandre Courbot wrote:
> > > On Sat Nov 15, 2025 at 8:30 AM JST, Timur Tabi wrote:
> > > > The GSP booter firmware in Turing and GA100 includes a third memory
> > > > section called ImemNs, which is non-secure IMEM.  This section must
> > > > be loaded separately from DMEM and secure IMEM, but only if it
> > > > actually exists.
> > > > 
> > > > Signed-off-by: Timur Tabi <ttabi@nvidia.com>
> > > > ---
> > > >   drivers/gpu/nova-core/falcon.rs          | 18 ++++++++++++++++--
> > > >   drivers/gpu/nova-core/firmware/booter.rs |  9 +++++++++
> > > >   drivers/gpu/nova-core/firmware/fwsec.rs  |  5 +++++
> > > >   3 files changed, 30 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/nova-core/falcon.rs b/drivers/gpu/nova-core/falcon.rs
> > > > index 0e0935dbb927..ece8b92a627e 100644
> > > > --- a/drivers/gpu/nova-core/falcon.rs
> > > > +++ b/drivers/gpu/nova-core/falcon.rs
> > > > @@ -239,6 +239,8 @@ fn from(value: PeregrineCoreSelect) -> Self {
> > > >   pub(crate) enum FalconMem {
> > > >       /// Secure Instruction Memory.
> > > >       ImemSec,
> > > > +    /// Non-Secure Instruction Memory.
> > > > +    ImemNs,
> > > 
> > > So, seeing how this is taking shape I now think we should just have one
> > > Imem variant:
> > > 
> > >      Imem { secure: bool },
> > 
> > ohhh, boolean args are usually not a good idea, because they make the
> > callsite non-self-documenting.
> > 
> > That's also true even in magical languages such as Rust. :)
> 
> I fully agree; that's why I made the field named so its name needs to be
> specified every time. :) Maybe `is_secure` would have been better
> though.
> 
> > 
> > Let's prefer enum args over bools, generally, please. So for example
> > (there are other ways to structure things, and this is just the
> > enum aspect of it):
> > 
> >      enum ImemSecurity {
> >          Secure,
> >          NonSecure,
> >      }
> > 
> >     Imem { security: ImemSecurity },
> 
> That would change
> 
>     FalconMem::Imem { secure: true }
> 
> into
> 
>     FalconMem::Imem {security: ImemSecurity::Secure }
> 
> If we want to use an enum I think we can remove the name:
> 
>      Imem(ImemSecurity),
> 
> So we can specify `Imem` as
> 
>     FalconMem::Imem(ImemSecurity::Secure)
> 
> which is as explicit, and a bit shorter.

I fail to see how any of this is an improvement.

> 
> > 
> > > 
> > > This makes matching easier for the common case of "we want to do
> > > something in case of Imem, regardless of the secure flag". Something
> > > like
> > > 
> > >      FalconMem::ImemSec | FalconMem::ImemNs => {
> > > 
> > > becomes:
> > > 
> > >      FalconMem::Imem { .. } => {
> > > 
> > > And if you need to use the flag, you can change e.g.:
> > > 
> > >      FalconMem::ImemSec | FalconMem::ImemNs => {
> > >          regs::NV_PFALCON_FALCON_IMEMC::default()
> > >              .set_secure(target_mem == FalconMem::ImemSec)
> > > 
> > > into
> > > 
> > >      FalconMem::Imem { secure } => {
> > 
> > See, this is hard and misleading to read. It reads like "secure
> > Imem", until you think at it a bit. Devastating! :)

But it is secure Imem.  That's why I called it ImemSec.

> 
> Renaming into `is_secure` would alleviate that, but the `ImemSecurity`
> enum is arguably as good, so I'm fine with it as well.
> 
> And an enum can also be used as a type to method arguments, which
> carries more semantics than `is_secure: bool`. So agreed, this is
> better.

Is it the goal of Rust to chose the most convoluted way to do something?  I don't see how any of
these is better than what I had initially.  There are three types of memory regions, Dmem, Non-
Secure Imem, and Secure Imem.  Just expand the enum to include all three, and everything else just
fits.