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: firmware: fix and explain v2 header offsets computations Date: Mon, 09 Mar 2026 09:15:15 +1000 Message-ID: In-Reply-To: <20260306-turing_prep-v11-9-8f0042c5d026@nvidia.com> References: <20260306-turing_prep-v11-0-8f0042c5d026@nvidia.com> <20260306-turing_prep-v11-9-8f0042c5d026@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 an important correctness fix. The old code set both secure and non-= secure IMEM `src_start` to `0`, which is incorrect =E2=80=94 they can't bot= h start at offset 0 in the firmware blob. The fix correctly computes the secure IMEM offset: ```rust let imem_sec_start =3D self.imem_sec_base.saturating_sub(self.imem_virt_bas= e); ``` **`saturating_sub` concern**: Using `saturating_sub` here means if `imem_se= c_base < imem_virt_base`, the result silently becomes 0 instead of signalin= g an error. While this shouldn't happen with valid firmware, `checked_sub` = + `ok_or(EINVAL)?` would be more defensive. The same concern applies to the= NS len computation: ```rust len: self.imem_load_size.saturating_sub(self.imem_sec_size), ``` Previously this used `checked_sub` which would return `None` and make `imem= _ns_load_params` return `None`. Now with `saturating_sub`, if `imem_sec_siz= e > imem_load_size` (corrupt firmware), the len would be 0 and an empty loa= d would be attempted rather than skipping NS entirely. This behavioral chan= ge is worth verifying is intentional. The `dst_start` calculation also uses `saturating_add`: ```rust dst_start: self.imem_phys_base.saturating_add(imem_sec_start), ``` Same concern =E2=80=94 overflow would produce a wrong value silently. --- Generated by Claude Code Patch Reviewer