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

[Eliot Courtney]

On Tue Apr 21, 2026 at 6:46 AM JST, Joel Fernandes wrote:
>
>
> 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@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?
>
> 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?

Yerp SG. I realised I sent the next version without applying this
feedback, sorry! But I do agree. I think we can strengthen it by saying
the offset from the /combined/ FWSEC region like you suggest, and maybe
mention that there may be an EFI after the end of PCI-AT.

thanks