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

[John Hubbard]

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 <tt...@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