On Mon, 2025-11-17 at 17:33 -0500, Joel Fernandes 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.
>
> Some more doc comments and documentation explaining the header structure
> would be great.
The header structure is the same, it's just that on pre-GA102 platforms, some
header fields are
used, and on GA102+ platforms other header fields are used. I can't explain
why -- that's just how
the images were built.
I'm not sure what kind of documentation I could add. The headers are already
all documented, and it
just seems somewhat arbitrary how the fields are used.
> > +
> > Ok(Self {
> > - imem_sec_load_target: FalconLoadTarget {
> > - src_start: app0.offset,
> > - dst_start: 0,
> > - len: app0.len,
> > + imem_sec_load_target: if chipset > Chipset::GA100 {
> > + FalconLoadTarget {
> > + src_start: app0.offset,
> > + dst_start: 0,
> > + len: app0.len,
> > + }
> > + } else {
> > + FalconLoadTarget {
> > + src_start: load_hdr.os_code_size,
> > + dst_start: app0.offset,
> > + len: app0.len,
> > + }
>
> Can write more succinctly:
>
> imem_sec_load_target: FalconLoadTarget {
> src_start: match chipset > Chipset::GA100 {
> true => app0.offset,
> false => load_hdr.os_code_size,
> },
> dst_start: match chipset > Chipset::GA100 {
> true => 0,
> false => app0.offset,
>
> len: app0.len,
>
> },
Do we really want to use "match" instead of "if", just because we don't need
"else"?
>
> > + },
> > + imem_ns_load_target: if chipset > Chipset::GA100 {
> > + None
> > + } else {
> > + Some(FalconLoadTarget {
> > + src_start: 0,
> > + dst_start: load_hdr.os_code_offset,
> > + len: load_hdr.os_code_size,
> > + })
> > },
> > - // Exists only in the booter image for Turing and GA100
> > - imem_ns_load_target: None,
> > dmem_load_target: FalconLoadTarget {
> > src_start: load_hdr.os_data_offset,
> > dst_start: 0,
> > @@ -393,7 +413,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
> > + ns_target.dst_start
> > + } else {
> > + // GA102+ (Ampere) - use secure load target
>
> s/Ampere/Ampere and later/ ? Also missing period at end of comment, here and
> elsewhere.
Sure, I'll clean those up. I'll just remove the (Ampere) though.
Please keep in mind that I've been working on these patches on-and-off over
several weeks and
through multiple rebases. There will be a lot of nits.
