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

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

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.

>
>> 
>> 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! :)

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.