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

[John Hubbard <jhubbard@nvidia.com>]

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

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 },

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

>          regs::NV_PFALCON_FALCON_IMEMC::default()
>              .set_secure(secure)
> 
> Which is simpler and easier to read.
> 
> This also removes the need to rename `Imem` into `ImemSec`, so the first
> two patches can be merged into one.
> 

thanks,
-- 
John Hubbard