Re: [PATCH v2 06/10] gpu: nova-core: correct FRTS vidmem offset calculation

[Alexandre Courbot]

On Fri Jul 3, 2026 at 7:22 PM JST, Eliot Courtney wrote:
> Currently, the frts vidmem offset is calculated based on the non-wpr
> heap size and pmu reservation size, but this is not right. The layout
> actually looks like this:
>
> | non-wpr heap | WPR2 .. FRTS | PMU reserved | ... | VGA workspace |
>
> It's just by coincidence + generous alignment that the values happened
> to match. Instead, define a per-architecture reserved size at the end of
> the framebuffer and use this plus the PMU reserved size to calculate the
> frts vidmem offset.
>
> Fixes: d317e4585fa3 ("gpu: nova-core: Hopper/Blackwell: add FSP Chain of Trust boot")
> Signed-off-by: Eliot Courtney <ecourtney@xxxxxxxxxx>
> ---
>  drivers/gpu/nova-core/fb.rs           |  4 ++++
>  drivers/gpu/nova-core/fb/hal.rs       |  3 +++
>  drivers/gpu/nova-core/fb/hal/ga100.rs |  4 ++++
>  drivers/gpu/nova-core/fb/hal/ga102.rs |  4 ++++
>  drivers/gpu/nova-core/fb/hal/gb100.rs |  5 +++++
>  drivers/gpu/nova-core/fb/hal/gb202.rs |  5 +++++
>  drivers/gpu/nova-core/fb/hal/gh100.rs |  4 ++++
>  drivers/gpu/nova-core/fb/hal/tu102.rs |  8 ++++++++
>  drivers/gpu/nova-core/fsp.rs          | 25 ++++++++++++++++++-------
>  9 files changed, 55 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/nova-core/fb.rs b/drivers/gpu/nova-core/fb.rs
> index fd60f93258a9..5ffe66af282f 100644
> --- a/drivers/gpu/nova-core/fb.rs
> +++ b/drivers/gpu/nova-core/fb.rs
> @@ -305,6 +305,9 @@ pub(crate) struct FbSizes {
>      pub(crate) heap_size: u64,
>      /// PMU reserved memory size, in bytes.
>      pub(crate) pmu_reserved_size: u32,
> +    /// Size reserved at the end of the framebuffer. This is architecture dependent and used to
> +    /// compute the FRTS offset for the FSP CoT message.
> +    pub(crate) fb_end_reserved_size: u32,
>      /// Number of VF partitions.
>      pub(crate) vf_partition_count: u8,
>  }
> @@ -322,6 +325,7 @@ fn new(chipset: Chipset, bar: Bar0<'_>) -> Result<Self> {
>                  .wpr_heap_size(chipset, fb_size)?,
>              heap_size: u64::from(hal.non_wpr_heap_size()),
>              pmu_reserved_size: hal.pmu_reserved_size(),
> +            fb_end_reserved_size: hal.fb_end_reserved_size(),
>              vf_partition_count: 0,
>          })
>      }
> diff --git a/drivers/gpu/nova-core/fb/hal.rs b/drivers/gpu/nova-core/fb/hal.rs
> index 714f0b51cd8f..aa50534550eb 100644
> --- a/drivers/gpu/nova-core/fb/hal.rs
> +++ b/drivers/gpu/nova-core/fb/hal.rs
> @@ -41,6 +41,9 @@ pub(crate) trait FbHal {
>  
>      /// Returns the FRTS size, in bytes.
>      fn frts_size(&self) -> u64;
> +
> +    /// Returns the size reserved at the end of the framebuffer, in bytes.
> +    fn fb_end_reserved_size(&self) -> u32;

This connects to my comments on the previous patch, but this HAL method
is only ever used on the FSP path, yet we have to provide values (that
will remain unused), for all chipsets. This really strenghen the case
for making FbLayout/FbRanges/FbSizes local to the boot method they
belong to.

Since `fb_end_reserved_size` is only ever used by `fsp.rs`, it would
make sense (from an architectural point of view at least) to have it
defined as a HAL method - even though its name screams "fb". Since they
are firmware-dependent, I'm even tempted to place their definition as
constants into `firmware/fw.rs` to make that fact unmistakable (and keep
all firmware-dependent data in the same place, as missing these upon
update is a source for headaches). It's not perfectly clean but there is
precedent for that: `fb.rs` uses `LibosParams` for instance, and
`GspFwWprMeta` also comes from there.

<snip>
> diff --git a/drivers/gpu/nova-core/fsp.rs b/drivers/gpu/nova-core/fsp.rs
> index 533fb95573ab..a38ba66626d8 100644
> --- a/drivers/gpu/nova-core/fsp.rs
> +++ b/drivers/gpu/nova-core/fsp.rs
> @@ -134,20 +134,31 @@ struct FspCotMessage {
>  }
>  
>  impl FspCotMessage {
> +    /// Computes the FRTS vidmem offset for the Chain-of-Trust message. It is measured from the end
> +    /// of the framebuffer.
> +    fn frts_vidmem_offset(fb_info: &FbSizes) -> Result<u64> {
> +        let mut offset = u64::from(fb_info.fb_end_reserved_size);
> +
> +        if fb_info.pmu_reserved_size != 0 {
> +            offset = offset
> +                .checked_add(u64::from(fb_info.pmu_reserved_size))
> +                .ok_or(EINVAL)?
> +                // The 2 MiB alignment is r570-specific.
> +                .align_up(Alignment::new::<SZ_2M>())
> +                .ok_or(EINVAL)?;

Note that we are adding two `u32`s and aligning to 2M - so there is no
risk of overflow. I think we can just document this, use unfallible
operators, and return a `u64` directly. Although `align_up` will still
return `Result`, so maybe we need to keep it after all - but the initial
addition can be unchecked.