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

On Thu Jul 23, 2026 at 11:12 PM JST, Alexandre Courbot wrote:
> 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 <[email protected]>
>> ---
>>  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.

There's two values used, so a standalone constant in firmware/fw.rs
isn't a good fit IMO. I think putting it in FSP HAL for now makes sense
but we should move it to TLV firmware at some point. Sent a respin of
these remaining patches and I think it addresses your points so PTAL.
Thanks!

>
> <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.