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.
Joel Fernandes
2025-Nov-19 20:34 UTC
[PATCH 02/11] gpu: nova-core: add ImemNs section infrastructure
On 11/19/2025 2:54 PM, Timur Tabi wrote:> 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.A struct could be another option? You have 2 entities here, the location of the memory (instruction memory or data memory) and the secure aspect. struct FalconMem { type: FalconMemType, // enum which can be instruction or data security: FalconMemSecurity, // enum can be secure or insecure. } That documents everything. But it is just an option I am putting out to consider if it helps. Thanks.
Alexandre Courbot
2025-Nov-20 01:45 UTC
[PATCH 02/11] gpu: nova-core: add ImemNs section infrastructure
On Thu Nov 20, 2025 at 4:54 AM JST, Timur Tabi wrote:> 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.It lets you capture both `Imem` variants with a single match arm instead of having to remember to use `|`, a pattern that is common in this series.