Alexandre Courbot
2025-Nov-19 06:55 UTC
[PATCH 02/11] gpu: nova-core: add ImemNs section infrastructure
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 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. :)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.
Timur Tabi
2025-Nov-19 19:54 UTC
[PATCH 02/11] gpu: nova-core: add ImemNs section infrastructure
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 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. :) > > 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.> > > > > > > > > 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! :)But it is secure Imem. That's why I called it ImemSec.> > 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.Is it the goal of Rust to chose the most convoluted way to do something? I don't see how any of these is better than what I had initially. There are three types of memory regions, Dmem, Non- Secure Imem, and Secure Imem. Just expand the enum to include all three, and everything else just fits.