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

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

On Thu Nov 20, 2025 at 4:54 AM JST, Timur Tabi wrote:
> 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.

It lets you capture both `Imem` variants with a single match arm instead
of having to remember to use `|`, a pattern that is common in this
series.