Alexandre Courbot
2025-Nov-19 01:54 UTC
[PATCH 02/11] gpu: nova-core: add ImemNs section infrastructure
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 at 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 }, 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 } => { 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.
John Hubbard
2025-Nov-19 06:30 UTC
[PATCH 02/11] gpu: nova-core: add ImemNs section infrastructure
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 at 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