On Wed, 2025-12-17 at 15:23 +0900, Alexandre Courbot wrote:
> > + // There are two versions of Booter, one for Turing/GA100, and
> > another for
> > + // GA102+. The extraction of the IMEM sections differs between
> > the two
> > + // versions. Unfortunately, the file names are the same, and the
> > headers
> > + // don't indicate the versions. The only way to differentiate is
> > by the Chipset.
> > +
>
> This comment begs for some code following it, and I notice that below
> the same `if chipset > Chipset::GA100` test is repeated twice.
>
> How about doing the following:
>
> let (imem_dst_start, imem_ns_load_target) = if chipset <= Chipset::GA100 {
> (app0.offset, Some(FalconLoadTarget {
> ...
> }))
> } else {
> (0, None)
> }
>
> ... and using the local variables to initialize the result?
Oh yeah, that's a great idea. Thanks.
> > @@ -393,7 +406,13 @@ fn brom_params(&self) -> FalconBromParams {
> > }
> >
> > fn boot_addr(&self) -> u32 {
> > - self.imem_sec_load_target.src_start
> > + if let Some(ns_target) = &self.imem_ns_load_target {
> > + // Turing and GA100 - use non-secure load target
>
> This comment is confusing - the logic is clear and doesn't mention the
> chipset, so let's remove it. Ideally we can limit these chipset-specific
> things to a single place and a single comment.
>
> > + ns_target.dst_start
> > + } else {
> > + // GA102 and later - use secure load target
>
> Same here, the code is explicit enough and doesn't mention the chipset.
Ok.
