Re: [PATCH v2 08/11] gpu: nova-core: vbios: compute FWSEC-relative Falcon data offset

[Joel Fernandes <joelagnelf@nvidia.com>]

On 4/16/2026 10:41 PM, Eliot Courtney wrote:
> On Fri Apr 17, 2026 at 1:13 AM JST, Joel Fernandes wrote:
>> On 4/14/2026 7:54 AM, Eliot Courtney wrote:
>>> Push the computation of the falcon data offset into a helper function.
>>> The subtraction to create the offset should be checked, and by doing
>>> this the check can be folded into the existing check in
>>> `falcon_data_ptr`.
>>>
>>> Signed-off-by: Eliot Courtney <ecourtney@nvidia.com>
>>> ---
>>>  drivers/gpu/nova-core/vbios.rs | 48 +++++++++++++++++-------------------------
>>>  1 file changed, 19 insertions(+), 29 deletions(-)
>>>
>>> diff --git a/drivers/gpu/nova-core/vbios.rs b/drivers/gpu/nova-core/vbios.rs
>>> index 01f65d50cbb3..0c0e0402e715 100644
>>> --- a/drivers/gpu/nova-core/vbios.rs
>>> +++ b/drivers/gpu/nova-core/vbios.rs
>>> @@ -765,33 +765,29 @@ fn get_bit_token(&self, token_id: u8) -> Result<BitToken> {
>>>          BitToken::from_id(self, token_id)
>>>      }
>>>  
>>> -    /// Find the Falcon data pointer structure in the [`PciAtBiosImage`].
>>> +    /// Find the Falcon data offset from the start of the FWSEC region.
>>
>> The comment change is incorrect, this subtraction is just for normalizing.
>> It basically normalizes the pointer wrt the PciAt image.
>>
>> It is only after the following in the caller that we get the true offset
>> within the FWSEC.
>>             offset -= first_fwsec.base.data.len();
>>
>> I suggest, let us rename falcon_data_offset() to
>> falcon_normalize_fwsec_offset() and update the comment above.
> 
> Thanks for your reviews! W.r.t. this, my understanding is that the
> layout is something like:
> 
> PCI-AT | Efi? | FWSEC | FWSEC
> 
> And that the falcon data pointer that we get out of PCI-AT starts off
> like this (indicated by ^):
> 
> ^ PCI-AT | Efi? | FWSEC | FWSEC
> 
> But the actual "address space" it's in is:
> 
> ^ PCI-AT | FWSEC | FWSEC
> 
> Because it doesn't count whatever images are between PCI-AT and the
> first FWSEC as part of that space. So by subtracting the PCI-AT size, we
> convert it to this logical space:
> 
> ^ FWSEC | FWSEC
> 
> Based on the above understanding doesn't it make sense to say that
> `falcon_data_offset` transforms the pointer to be relative from the
> start of the FWSEC region? Once we subtract off the first fwsec image
> length, it's then relative to the second FWSEC image. Please LMK if I've
> missed something. We could also emphasise in the doc that the "FWSEC
> region" means the contiguous region defined by the first two FWSEC
> images. WDYT?

You did not miss anything, your explanation above is spot-on.

You could also keep your function name and add a comment on top of
`falcon_data_offset()`. Something like "offset from the end of PCI-AT
(i.e., the start of the combined FWSEC region)". Sounds good?

thanks,

--
Joel Fernandes