From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: gpu: nova-core: use the Generic Bootloader to boot FWSEC on Turing Date: Tue, 03 Mar 2026 13:55:13 +1000 Message-ID: In-Reply-To: <20260301-turing_prep-v10-8-dde5ee437c60@nvidia.com> References: <20260301-turing_prep-v10-0-dde5ee437c60@nvidia.com> <20260301-turing_prep-v10-8-dde5ee437c60@nvidia.com> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review This is the main feature patch. It introduces `FwsecFirmwareWithBl` which w= raps `FwsecFirmware` with the "gen_bootloader" firmware that performs the a= ctual DMA-based load of FWSEC into falcon memory. **Architecture flow:** The bootloader code is loaded via PIO into non-secur= e IMEM. A `BootloaderDmemDescV2` descriptor pointing to the FWSEC firmware = (in a DMA object) is loaded into DMEM. The falcon boots the bootloader, whi= ch then DMA-loads FWSEC and runs it. **Key observations:** 1. **`_firmware_dma` field:** The DMA object is stored with a leading under= score to indicate it's "unused" directly, but it must stay alive because th= e bootloader reads from it via DMA: ```rust _firmware_dma: DmaObject, ``` This is correct =E2=80=94 the DMA object must outlive the bootloader's exec= ution. The field keeps it alive. 2. **`BootloaderDmemDescV2` is `repr(C, packed)`:** This is necessary becau= se the struct has `u64` fields (`code_dma_base`, `data_dma_base`) at offset= s that are not 8-byte aligned (due to preceding `u32` fields). The `packed`= attribute ensures no padding is inserted. The `unsafe impl AsBytes` is cor= rect here since all fields are primitive types with no padding. 3. **`BootloaderDesc` has `unsafe impl FromBytes`:** Correct =E2=80=94 all = fields are `u32` and any byte pattern is valid. 4. **IMEM placement:** ```rust const BOOTLOADER_LOAD_CEILING: usize =3D sizes::SZ_64K; let imem_dst_start =3D BOOTLOADER_LOAD_CEILING .checked_sub(ucode.len()) .ok_or(EOVERFLOW)?; ``` The bootloader is placed right below the 64K boundary of IMEM. The code is = aligned to `MEM_BLOCK_ALIGNMENT` (256 bytes) via `align_up`, which ensures = the `u16::try_from(imem_dst_start)` conversion won't produce a misaligned v= alue. 5. **`firmware.ucode.0` access:** In `FwsecFirmwareWithBl::new`: ```rust let firmware_dma =3D DmaObject::from_data(dev, &firmware.ucode.0)?; ``` This accesses the private inner field of `FirmwareObject` directly. Since `= bootloader.rs` is a submodule of `fwsec.rs` (via `pub(crate) mod bootloader= `), and `FirmwareObject` is module-local, this field access should work wit= hin the same module hierarchy. However, `FirmwareObject` is defined in `fir= mware.rs` (parent module), so this access may depend on Rust visibility rul= es for sibling/child modules. If `FirmwareObject`'s tuple field `.0` is not= `pub(crate)`, this would be a compile error. Since this is v10 and presuma= bly compiles, this is fine. 6. **`try_update` usage:** The patch uses `regs::NV_PFALCON_FBIF_TRANSCFG::= try_update()` instead of `update()` when the index comes from runtime data = (`self.dmem_desc.ctx_dma`). This is the correct approach since the index is= n't known at compile time =E2=80=94 `try_update` validates the array bounds= at runtime. No blocking issues. --- Generated by Claude Code Patch Reviewer