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

[Eliot Courtney]

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@xxxxxxxxxx>
>> ---
>>  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?