On Mon Mar 2, 2026 at 4:22 PM JST, Eliot Courtney wrote:
> On Sun Mar 1, 2026 at 11:03 PM JST, Alexandre Courbot wrote:
>> From: Timur Tabi <[email protected]>
>> +impl FwsecFirmwareWithBl {
>> + /// Loads the bootloader firmware for `dev` and `chipset`, and wrap
>> `firmware` so it can be
>> + /// loaded using it.
>> + pub(crate) fn new(
>> + firmware: FwsecFirmware,
>> + dev: &Device<device::Bound>,
>> + chipset: Chipset,
>> + ) -> Result<Self> {
>> + let fw = request_firmware(dev, chipset, "gen_bootloader",
>> FIRMWARE_VERSION)?;
>> + let hdr = fw
>> + .data()
>> + .get(0..size_of::<BinHdr>())
>> + .and_then(BinHdr::from_bytes_copy)
>> + .ok_or(EINVAL)?;
>> +
>> + let desc = {
>> + let desc_offset = usize::from_safe_cast(hdr.header_offset);
>> +
>> + fw.data()
>> + .get(desc_offset..)
>> + .and_then(BootloaderDesc::from_bytes_copy_prefix)
>> + .ok_or(EINVAL)?
>> + .0
>> + };
>> +
>> + let ucode = {
>> + let ucode_start = usize::from_safe_cast(hdr.data_offset);
>> + let code_size = usize::from_safe_cast(desc.code_size);
>> + // Align to falcon block size (256 bytes).
>> + let aligned_code_size = code_size
>> + .align_up(Alignment::new::<{ falcon::MEM_BLOCK_ALIGNMENT
>> }>())
>> + .ok_or(EINVAL)?;
>> +
>> + let mut ucode = KVec::with_capacity(aligned_code_size,
>> GFP_KERNEL)?;
>> + ucode.extend_from_slice(
>> + fw.data()
>> + .get(ucode_start..ucode_start + code_size)
>> + .ok_or(EINVAL)?,
>> + GFP_KERNEL,
>> + )?;
>> + ucode.resize(aligned_code_size, 0, GFP_KERNEL)?;
>> +
>> + ucode
>> + };
>> +
>> + let firmware_dma = DmaObject::from_data(dev, &firmware.ucode.0)?;
>> +
>> + let dmem_desc = {
>> + let imem_sec =
>> FalconDmaLoadable::imem_sec_load_params(&firmware);
>> + let imem_ns =
>> FalconDmaLoadable::imem_ns_load_params(&firmware).ok_or(EINVAL)?;
>> + let dmem = FalconDmaLoadable::dmem_load_params(&firmware);
>> +
>> + BootloaderDmemDescV2 {
>> + reserved: [0; 4],
>> + signature: [0; 4],
>> + ctx_dma: 4, // FALCON_DMAIDX_PHYS_SYS_NCOH
>> + code_dma_base: firmware_dma.dma_handle(),
>> + non_sec_code_off: imem_ns.dst_start,
>> + non_sec_code_size: imem_ns.len,
>> + sec_code_off: imem_sec.dst_start,
>
> Is it correct here to use `dst_start` for `non_sec_code_off` and
> `sec_code_off`?
> AFAICT, these are meant to be offsets from the base of the DMA memory and
> it's meant to copy into the falcon. But the documentation on `dst_start`
> says `Offset from the start of the destination memory to copy into.`
OpenRM does the following:
pUcode->imemNsPa = pDescV2->IMEMPhysBase;
...
blDmemDesc.nonSecureCodeOff = pUcode->imemNsPa;
and
pUcode->imemSecPa = pDescV2->IMEMSecBase - pDescV2->IMEMVirtBase +
pDescV2->IMEMPhysBase;
...
blDmemDesc.secureCodeOff = pUcode->imemSecPa;
So it *does* set IMEM addresses (i.e. destination) into these fields as
well. And their documentation is the same as Nova, which is all the more
intriguing.
But the reason for this became clear after I figured out that the FWSEC
firmware was a *mirror* image of what is expected in IMEM. The IMEM
destination offsets are also the offsets from the start of the source
DMA object, hence their use in `BootloaderDmemDescV2` - for the
bootloader, they are the offsets from both the source AND the
destination!
I've found a couple of differences while reviewing the code though.
Nova-core did not do the `- pDescV2->IMEMVirtBase +
pDescV2->IMEMPhysBase` part, and it did not pad the FWSEC start image
with zeroes up to `IMEMPhysBase` like OpenRM does (to have this
source/destination symmetry). This happened to work because these values
are all zero in practice, but we want to align the code to do the
correct thing otherwise we have a theoretical risk of mismatch.
>
> Also `ucode_start` is defined as `hdr.data_offset`
> but doesn't add `code_off` from `BootloaderDesc` and `code_off` appears
> unused. So does `data_off` and `dmem_load_off` for that matter.
> I find it hard to follow what's actually required but it seems odd that
> none of these are used.
>
> At least for `code_off` should it not be part of the computation of
> `ucode_start`?
> `let ucode_start = usize::from_safe_cast(hdr.data_offset);`
These two appear to be correct, at least if we follow OpenRM. For IMEM:
// Copy bootloader to top of IMEM
virtAddr = pBlUcDesc->blStartTag << 8;
NV_ASSERT_OK_OR_RETURN(
s_imemCopyTo_TU102(pGpu, pKernelFlcn,
imemDstBlk << FALCON_IMEM_BLKSIZE2,
pBlImg, // <-- start of image, no `code_off`.
blSize,
NV_FALSE,
virtAddr));
The `BootloaderDesc` is only used for `code_size` and `start_tag`. And
it's not strange considering that it just contains the code to load into
IMEM - the data being the `BootloaderDmemDescV2` that we construct from
the actual firmware. Since there is only one segment in that binary, it
makes sense that it starts at offset zero.
And here is what OpenRM does for DMEM:
// Copy dmem desc to DMEM offset 0
NV_ASSERT_OK_OR_RETURN(
s_dmemCopyTo_TU102(pGpu, pKernelFlcn,
0,
(NvU8 *) &blDmemDesc,
sizeof(RM_FLCN_BL_DMEM_DESC)));
Here too it makes sense that we load at offset 0, since there is only
one data segment (the `BootloaderDmemDescV2`) that we build ourselves.
There is no other data to place anywhere.
I suspect `BootloaderDesc` (or `RM_FLCN_BL_IMG_HEADER` in OpenRM) was
recycled from some older (pre-OpenRM), more complex firmware and
inherited its documentation only serves a limited purpose here.
The comments on `BootloaderDesc` are incorrect for us though. I'll amend
them.
>
> Overall I find the dst/srcs here confusing cos it feels hard to keep
> track of which set of memory things are in. So, sorry if these comments
> are nonsense.
The documentation clearly sends us on the wrong path, so good thing we
fixed it.
