Re: [PATCH v4 10/20] gpu: nova-core: vbios: compute FWSEC-relative Falcon data offset

[John Hubbard]

On 5/18/26 7:55 PM, 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(-)
> 

Reviewed-by: John Hubbard <jhubbard@xxxxxxxxxx>

thanks,
-- 
John Hubbard

> diff --git a/drivers/gpu/nova-core/vbios.rs b/drivers/gpu/nova-core/vbios.rs
> index cadc6dcffefb..ca101b2b6095 100644
> --- a/drivers/gpu/nova-core/vbios.rs
> +++ b/drivers/gpu/nova-core/vbios.rs
> @@ -846,33 +846,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.
>      ///
> -    /// This is just a 4 byte structure that contains a pointer to the Falcon data in the FWSEC
> -    /// image.
> -    fn falcon_data_ptr(&self) -> Result<u32> {
> +    /// The BIT table contains a 4-byte pointer to the Falcon data. Testing shows this pointer
> +    /// treats the PCI-AT and FWSEC images as logically contiguous even when an EFI image sits in
> +    /// between them, so subtract the PCI-AT image size here to convert it to a FWSEC-relative
> +    /// offset.
> +    fn falcon_data_offset(&self) -> Result<usize> {
>          let token = self.get_bit_token(BIT_TOKEN_ID_FALCON_DATA)?;
> -
> -        // Make sure we don't go out of bounds
> -        if usize::from(token.data_offset) + 4 > self.base.data.len() {
> -            return Err(EINVAL);
> -        }
> -
> -        // read the 4 bytes at the offset specified in the token
>          let offset = usize::from(token.data_offset);
> -        let bytes: [u8; 4] = self.base.data[offset..offset + 4].try_into().map_err(|_| {
> -            dev_err!(self.base.dev, "Failed to convert data slice to array\n");
> -            EINVAL
> -        })?;
>  
> -        let data_ptr = u32::from_le_bytes(bytes);
> +        // Read the 4-byte falcon data pointer at the offset specified in the token.
> +        let data = &self.base.data;
> +        let (ptr, _) = data
> +            .get(offset..)
> +            .and_then(u32::from_bytes_copy_prefix)
> +            .ok_or(EINVAL)?;
>  
> -        if (usize::from_safe_cast(data_ptr)) < self.base.data.len() {
> -            dev_err!(self.base.dev, "Falcon data pointer out of bounds\n");
> -            return Err(EINVAL);
> -        }
> -
> -        Ok(data_ptr)
> +        usize::from_safe_cast(ptr)
> +            .checked_sub(data.len())
> +            .ok_or(EINVAL)
> +            .inspect_err(|_| {
> +                dev_err!(self.base.dev, "Falcon data pointer out of bounds\n");
> +            })
>      }
>  }
>  
> @@ -989,15 +985,9 @@ fn setup_falcon_data(
>          pci_at_image: &PciAtBiosImage,
>          first_fwsec: &FwSecBiosBuilder,
>      ) -> Result {
> -        let mut offset = usize::from_safe_cast(pci_at_image.falcon_data_ptr()?);
> +        let mut offset = pci_at_image.falcon_data_offset()?;
>          let mut pmu_in_first_fwsec = false;
>  
> -        // The falcon data pointer assumes that the PciAt and FWSEC images
> -        // are contiguous in memory. However, testing shows the EFI image sits in
> -        // between them. So calculate the offset from the end of the PciAt image
> -        // rather than the start of it. Compensate.
> -        offset -= pci_at_image.base.data.len();
> -
>          // The offset is now from the start of the first Fwsec image, however
>          // the offset points to a location in the second Fwsec image. Since
>          // the fwsec images are contiguous, subtract the length of the first Fwsec
>