John Hubbard
2025-Nov-19 07:04 UTC
[PATCH 03/11] gpu: nova-core: support header parsing on Turing/GA100
On 11/18/25 9:16 PM, Timur Tabi wrote:> On Wed, 2025-11-19 at 11:51 +0900, Alexandre Courbot wrote: >> I'd prefer if we could reason in terms of functionality instead of >> specific chipset versions. > > If you can figure it out, I'd be happy to change the code. I wasn't being lazy when I made this > comment: > > // 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. > >> IIUC the relevant factor is that Turing/GA100 have some non-secure >> bootloader code as the entry point of booter, which GA102+ doesn't >> feature as it is capable of starting in secure mode directly (please >> correct me as my understanding is probably incomplete if not outright >> wrong). > > That sounds about right. There are secure and non-secure sections in the firmware image. > >> What is the HW or SW fact that requires this on Turing? > > I don't know how to answer that question. That's just how it's done on Turing/GA100. I would > need to start an internal Slack thread to get a better answer, and I don't really see what it > would gain us.Here, would it be reasonable to just create a tiny HAL-like construct (a trait, I guess) that has the function to call, and it decides which behavior to use based on the chipset-selected HAL. In other words, we know very clearly that we need to call one version of the function for earlier chips, and the other version of the function for later chips, and the dividing line is because that behavior changed at a certain chipset. I think this HAL-centric, chipset-centric approach is fine for some things, where we really have no reason to care about the internal details. For other cases, the functionality based partitioning is probably ideal. Thoughts?> >> Is it linked >> to the fact we need to use PIO for it? What I would like to achieve is >> removing or at least reducing these chipset checks into one single >> point, which in the worst case could be a method of `Chipset` telling us >> which loading method to use. But if we can find a distinguishing factor >> in the parsed by this method, that would be even better. > > Both OpenRM and Nouveau use the chipset to gate on how to parse the headers.Yes. That also carries some weight. thanks, -- John Hubbard
Joel Fernandes
2025-Nov-19 20:10 UTC
[PATCH 03/11] gpu: nova-core: support header parsing on Turing/GA100
On 11/19/2025 2:04 AM, John Hubbard wrote:> On 11/18/25 9:16 PM, Timur Tabi wrote: >> On Wed, 2025-11-19 at 11:51 +0900, Alexandre Courbot wrote: >>> I'd prefer if we could reason in terms of functionality instead of >>> specific chipset versions. >> >> If you can figure it out, I'd be happy to change the code.? I wasn't being >> lazy when I made this >> comment: >> >> ?? // 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. >> >>> IIUC the relevant factor is that Turing/GA100 have some non-secure >>> bootloader code as the entry point of booter, which GA102+ doesn't >>> feature as it is capable of starting in secure mode directly (please >>> correct me as my understanding is probably incomplete if not outright >>> wrong). >> >> That sounds about right.? There are secure and non-secure sections in the >> firmware image. >> >>> What is the HW or SW fact that requires this on Turing? >> >> I don't know how to answer that question.? That's just how it's done on >> Turing/GA100.? I would >> need to start an internal Slack thread to get a better answer, and I don't >> really see what it >> would gain us. > > Here, would it be reasonable to just create a tiny HAL-like construct > (a trait, I guess) that has the function to call, and it decides which > behavior to use based on the chipset-selected HAL. > > In other words, we know very clearly that we need to call one version of > the function for earlier chips, and the other version of the function for > later chips, and the dividing line is because that behavior changed at > a certain chipset. > > I think this HAL-centric, chipset-centric approach is fine for some > things, where we really have no reason to care about the internal details. > > For other cases, the functionality based partitioning is probably ideal. >How about a BooterFlags enum in BooterFirmware, and during its construction in new(), do the chipset check there? That way the chipset is in only one place. And the flag in BooterFirmware saying "NonSecure" or something like that for Turing, and default to NONE for other chipsets. That's similar to John's tiny-HAL idea, perhaps the call to BooterFirmware::new() can check with the HAL callback instead of checking which chipset. Something like this on top of Timur's series, only build-tested: diff --git a/drivers/gpu/nova-core/firmware/booter.rs b/drivers/gpu/nova-core/firmware/booter.rs index 6ac9593504db..b9a8dc8d08f2 100644 --- a/drivers/gpu/nova-core/firmware/booter.rs +++ b/drivers/gpu/nova-core/firmware/booter.rs @@ -249,8 +249,19 @@ fn as_ref(&self) -> &[u8] { impl<'a> FirmwareSignature<BooterFirmware> for BooterSignature<'a> {} +pub(crate) enum BooterFlags { + NonSecure, + Secure, +} + /// The `Booter` loader firmware, responsible for loading the GSP. pub(crate) struct BooterFirmware { + flags: BooterFlags, // Load parameters for Secure `IMEM` falcon memory. imem_sec_load_target: FalconLoadTarget, // Load parameters for Non-Secure `IMEM` falcon memory, @@ -361,8 +372,16 @@ pub(crate) fn new( // 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. + // Chipset check. + let flags = if chipset > Chipset::GA100 { + BooterFlags::Secure + } else { + BooterFlags::NonSecure + }; + Ok(Self { - imem_sec_load_target: if chipset > Chipset::GA100 { + flags, + imem_sec_load_target: if flags == BooterFlags::Secure { FalconLoadTarget { src_start: app0.offset, dst_start: 0, @@ -375,14 +394,14 @@ pub(crate) fn new( len: app0.len, } }, - imem_ns_load_target: if chipset > Chipset::GA100 { - None - } else { + imem_ns_load_target: if flags == BooterFlags::NonSecure { Some(FalconLoadTarget { src_start: 0, dst_start: load_hdr.os_code_offset, len: load_hdr.os_code_size, }) + } else { + None }, dmem_load_target: FalconLoadTarget { src_start: load_hdr.os_data_offset, @@ -393,6 +412,11 @@ pub(crate) fn new( ucode: ucode_signed, }) } + + pub(crate) fn flags(&self) -> BooterFlags { + self.flags + } } impl FalconLoadParams for BooterFirmware { @@ -413,12 +437,15 @@ fn brom_params(&self) -> FalconBromParams { } fn boot_addr(&self) -> u32 { - 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 - self.imem_sec_load_target.src_start + match self.flags { + BooterFlags::NonSecure => { + // Turing and GA100 - use non-secure load target. + self.imem_ns_load_target.as_ref().unwrap().dst_start + } + BooterFlags::Secure => { + // GA102+ (Ampere and later) - use secure load target. + self.imem_sec_load_target.src_start + } } } } diff --git a/drivers/gpu/nova-core/gsp/boot.rs b/drivers/gpu/nova-core/gsp/boot.rs index ff53d58c1df6..21ac1cebb6d2 100644 --- a/drivers/gpu/nova-core/gsp/boot.rs +++ b/drivers/gpu/nova-core/gsp/boot.rs @@ -21,6 +21,7 @@ firmware::{ booter::{ BooterFirmware, + BooterFlags, BooterKind, // }, fwsec::{ @@ -184,7 +185,7 @@ pub(crate) fn boot( ); sec2_falcon.reset(bar)?; - if chipset > Chipset::GA100 { + if booter_loader.flags() == BooterFlags::Secure { sec2_falcon.dma_load(bar, &booter_loader)?; } else { sec2_falcon.pio_load(bar, &booter_loader, None)?; thanks, - Joel