The GSP requires some pieces of metadata to boot. These are passed in a
struct which the GSP transfers via DMA. Create this struct and get a
handle to it for future use when booting the GSP.
Signed-off-by: Alistair Popple <apopple@nvidia.com>
---
drivers/gpu/nova-core/fb.rs | 1 -
drivers/gpu/nova-core/firmware.rs | 2 +-
drivers/gpu/nova-core/firmware/gsp.rs | 1 -
drivers/gpu/nova-core/firmware/riscv.rs | 6 +-
drivers/gpu/nova-core/gpu.rs | 4 +-
drivers/gpu/nova-core/gsp.rs | 77 ++++++++++++++++++-
drivers/gpu/nova-core/nvfw.rs | 7 ++
.../gpu/nova-core/nvfw/r570_144_bindings.rs | 2 +
8 files changed, 89 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/nova-core/fb.rs b/drivers/gpu/nova-core/fb.rs
index a3eb063f86b3a..b1131a94389c6 100644
--- a/drivers/gpu/nova-core/fb.rs
+++ b/drivers/gpu/nova-core/fb.rs
@@ -130,7 +130,6 @@ pub(crate) fn wpr_heap_size(&self, fb_size: u64) -> Result<u64> {
///
/// Contains ranges of GPU memory reserved for a given purpose during the GSP boot process.
#[derive(Debug)]
-#[expect(dead_code)]
pub(crate) struct FbLayout {
/// Range of the framebuffer. Starts at `0`.
pub(crate) fb: Range<u64>,
diff --git a/drivers/gpu/nova-core/firmware.rs b/drivers/gpu/nova-core/firmware.rs
index 05e57730a3c6f..6c210e668d541 100644
--- a/drivers/gpu/nova-core/firmware.rs
+++ b/drivers/gpu/nova-core/firmware.rs
@@ -126,7 +126,7 @@ pub(crate) struct Firmware {
/// GSP firmware.
pub gsp: Pin<KBox<GspFirmware>>,
/// GSP signatures, to be passed as parameter to the bootloader for validation.
- gsp_sigs: DmaObject,
+ pub gsp_sigs: DmaObject,
}
impl Firmware {
diff --git a/drivers/gpu/nova-core/firmware/gsp.rs b/drivers/gpu/nova-core/firmware/gsp.rs
index f37bd619bfb71..69322fa7c1466 100644
--- a/drivers/gpu/nova-core/firmware/gsp.rs
+++ b/drivers/gpu/nova-core/firmware/gsp.rs
@@ -94,7 +94,6 @@ pub(crate) fn new<'a>(
})
}
- #[expect(unused)]
/// Returns the DMA handle of the level 0 page table.
pub(crate) fn lvl0_dma_handle(&self) -> DmaAddress {
self.lvl0.dma_handle()
diff --git a/drivers/gpu/nova-core/firmware/riscv.rs b/drivers/gpu/nova-core/firmware/riscv.rs
index b2f646c1f02c6..81bb348055031 100644
--- a/drivers/gpu/nova-core/firmware/riscv.rs
+++ b/drivers/gpu/nova-core/firmware/riscv.rs
@@ -53,11 +53,11 @@ fn new(bin_fw: &BinFirmware<'_>) -> Result<Self> {
#[expect(unused)]
pub(crate) struct RiscvFirmware {
/// Offset at which the code starts in the firmware image.
- code_offset: u32,
+ pub code_offset: u32,
/// Offset at which the data starts in the firmware image.
- data_offset: u32,
+ pub data_offset: u32,
/// Offset at which the manifest starts in the firmware image.
- manifest_offset: u32,
+ pub manifest_offset: u32,
/// Application version.
app_version: u32,
/// Device-mapped firmware image.
diff --git a/drivers/gpu/nova-core/gpu.rs b/drivers/gpu/nova-core/gpu.rs
index 6190199e055c2..bf762353f1d91 100644
--- a/drivers/gpu/nova-core/gpu.rs
+++ b/drivers/gpu/nova-core/gpu.rs
@@ -13,6 +13,7 @@
use crate::regs;
use crate::util;
use crate::vbios::Vbios;
+
use core::fmt;
macro_rules! define_chipset {
@@ -311,8 +312,9 @@ pub(crate) fn new(
Self::run_fwsec_frts(pdev.as_ref(), &gsp_falcon, bar, &bios, &fb_layout)?;
- let libos = gsp::GspMemObjects::new(pdev)?;
+ let libos = gsp::GspMemObjects::new(pdev, &fw, &fb_layout)?;
let _libos_handle = libos.libos_dma_handle();
+ let _wpr_handle = libos.wpr_meta.dma_handle();
Ok(pin_init!(Self {
spec,
diff --git a/drivers/gpu/nova-core/gsp.rs b/drivers/gpu/nova-core/gsp.rs
index 161c057350622..1f51e354b9569 100644
--- a/drivers/gpu/nova-core/gsp.rs
+++ b/drivers/gpu/nova-core/gsp.rs
@@ -6,12 +6,17 @@
use kernel::dma_write;
use kernel::pci;
use kernel::prelude::*;
-use kernel::ptr::Alignment;
+use kernel::ptr::{Alignable, Alignment};
+use kernel::sizes::SZ_128K;
use kernel::transmute::{AsBytes, FromBytes};
+use crate::fb::FbLayout;
+use crate::firmware::Firmware;
use crate::nvfw::{
- LibosMemoryRegionInitArgument, LibosMemoryRegionKind_LIBOS_MEMORY_REGION_CONTIGUOUS,
- LibosMemoryRegionLoc_LIBOS_MEMORY_REGION_LOC_SYSMEM,
+ GspFwWprMeta, GspFwWprMetaBootInfo, GspFwWprMetaBootResumeInfo, LibosMemoryRegionInitArgument,
+ LibosMemoryRegionKind_LIBOS_MEMORY_REGION_CONTIGUOUS,
+ LibosMemoryRegionLoc_LIBOS_MEMORY_REGION_LOC_SYSMEM, GSP_FW_WPR_META_MAGIC,
+ GSP_FW_WPR_META_REVISION,
};
pub(crate) const GSP_PAGE_SHIFT: usize = 12;
@@ -25,12 +30,69 @@ unsafe impl AsBytes for LibosMemoryRegionInitArgument {}
// are valid.
unsafe impl FromBytes for LibosMemoryRegionInitArgument {}
+// SAFETY: Padding is explicit and will not contain uninitialized data.
+unsafe impl AsBytes for GspFwWprMeta {}
+
+// SAFETY: This struct only contains integer types for which all bit patterns
+// are valid.
+unsafe impl FromBytes for GspFwWprMeta {}
+
#[allow(unused)]
pub(crate) struct GspMemObjects {
libos: CoherentAllocation<LibosMemoryRegionInitArgument>,
pub loginit: CoherentAllocation<u8>,
pub logintr: CoherentAllocation<u8>,
pub logrm: CoherentAllocation<u8>,
+ pub wpr_meta: CoherentAllocation<GspFwWprMeta>,
+}
+
+pub(crate) fn build_wpr_meta(
+ dev: &device::Device<device::Bound>,
+ fw: &Firmware,
+ fb_layout: &FbLayout,
+) -> Result<CoherentAllocation<GspFwWprMeta>> {
+ let wpr_meta =
+ CoherentAllocation::<GspFwWprMeta>::alloc_coherent(dev, 1, GFP_KERNEL | __GFP_ZERO)?;
+ dma_write!(
+ wpr_meta[0] = GspFwWprMeta {
+ magic: GSP_FW_WPR_META_MAGIC as u64,
+ revision: u64::from(GSP_FW_WPR_META_REVISION),
+ sysmemAddrOfRadix3Elf: fw.gsp.lvl0_dma_handle(),
+ sizeOfRadix3Elf: fw.gsp.size as u64,
+ sysmemAddrOfBootloader: fw.gsp_bootloader.ucode.dma_handle(),
+ sizeOfBootloader: fw.gsp_bootloader.ucode.size() as u64,
+ bootloaderCodeOffset: u64::from(fw.gsp_bootloader.code_offset),
+ bootloaderDataOffset: u64::from(fw.gsp_bootloader.data_offset),
+ bootloaderManifestOffset: u64::from(fw.gsp_bootloader.manifest_offset),
+ __bindgen_anon_1: GspFwWprMetaBootResumeInfo {
+ __bindgen_anon_1: GspFwWprMetaBootInfo {
+ sysmemAddrOfSignature: fw.gsp_sigs.dma_handle(),
+ sizeOfSignature: fw.gsp_sigs.size() as u64,
+ }
+ },
+ gspFwRsvdStart: fb_layout.heap.start,
+ nonWprHeapOffset: fb_layout.heap.start,
+ nonWprHeapSize: fb_layout.heap.end - fb_layout.heap.start,
+ gspFwWprStart: fb_layout.wpr2.start,
+ gspFwHeapOffset: fb_layout.wpr2_heap.start,
+ gspFwHeapSize: fb_layout.wpr2_heap.end - fb_layout.wpr2_heap.start,
+ gspFwOffset: fb_layout.elf.start,
+ bootBinOffset: fb_layout.boot.start,
+ frtsOffset: fb_layout.frts.start,
+ frtsSize: fb_layout.frts.end - fb_layout.frts.start,
+ gspFwWprEnd: fb_layout
+ .vga_workspace
+ .start
+ .align_down(Alignment::new(SZ_128K)),
+ gspFwHeapVfPartitionCount: fb_layout.vf_partition_count,
+ fbSize: fb_layout.fb.end - fb_layout.fb.start,
+ vgaWorkspaceOffset: fb_layout.vga_workspace.start,
+ vgaWorkspaceSize: fb_layout.vga_workspace.end - fb_layout.vga_workspace.start,
+ ..Default::default()
+ }
+ )?;
+
+ Ok(wpr_meta)
}
/// Generates the `ID8` identifier required for some GSP objects.
@@ -92,7 +154,11 @@ fn create_coherent_dma_object<A: AsBytes + FromBytes>(
}
impl GspMemObjects {
- pub(crate) fn new(pdev: &pci::Device<device::Bound>) -> Result<Self> {
+ pub(crate) fn new(
+ pdev: &pci::Device<device::Bound>,
+ fw: &Firmware,
+ fb_layout: &FbLayout,
+ ) -> Result<Self> {
let dev = pdev.as_ref();
let mut libos = CoherentAllocation::<LibosMemoryRegionInitArgument>::alloc_coherent(
dev,
@@ -106,11 +172,14 @@ pub(crate) fn new(pdev: &pci::Device<device::Bound>) -> Result<Self> {
let mut logrm = create_coherent_dma_object::<u8>(dev, "LOGRM", 0x10000, &mut libos, 2)?;
create_pte_array(&mut logrm);
+ let wpr_meta = build_wpr_meta(dev, fw, fb_layout)?;
+
Ok(GspMemObjects {
libos,
loginit,
logintr,
logrm,
+ wpr_meta,
})
}
diff --git a/drivers/gpu/nova-core/nvfw.rs b/drivers/gpu/nova-core/nvfw.rs
index 9a2f0c84ab103..c04b8e218758b 100644
--- a/drivers/gpu/nova-core/nvfw.rs
+++ b/drivers/gpu/nova-core/nvfw.rs
@@ -46,4 +46,11 @@ pub(crate) struct LibosParams {
LibosMemoryRegionInitArgument,
LibosMemoryRegionKind_LIBOS_MEMORY_REGION_CONTIGUOUS,
LibosMemoryRegionLoc_LIBOS_MEMORY_REGION_LOC_SYSMEM,
+
+ // GSP firmware constants
+ GSP_FW_WPR_META_MAGIC,
+ GSP_FW_WPR_META_REVISION,
};
+
+pub(crate) type GspFwWprMetaBootResumeInfo = r570_144::GspFwWprMeta__bindgen_ty_1;
+pub(crate) type GspFwWprMetaBootInfo = r570_144::GspFwWprMeta__bindgen_ty_1__bindgen_ty_1;
diff --git a/drivers/gpu/nova-core/nvfw/r570_144_bindings.rs b/drivers/gpu/nova-core/nvfw/r570_144_bindings.rs
index 6a14cc3243918..392b25dc6991a 100644
--- a/drivers/gpu/nova-core/nvfw/r570_144_bindings.rs
+++ b/drivers/gpu/nova-core/nvfw/r570_144_bindings.rs
@@ -9,6 +9,8 @@
pub const GSP_FW_HEAP_SIZE_OVERRIDE_LIBOS2_MAX_MB: u32 = 256;
pub const GSP_FW_HEAP_SIZE_OVERRIDE_LIBOS3_BAREMETAL_MIN_MB: u32 = 88;
pub const GSP_FW_HEAP_SIZE_OVERRIDE_LIBOS3_BAREMETAL_MAX_MB: u32 = 280;
+pub const GSP_FW_WPR_META_REVISION: u32 = 1;
+pub const GSP_FW_WPR_META_MAGIC: i64 = -2577556379034558285;
pub type __u8 = ffi::c_uchar;
pub type __u16 = ffi::c_ushort;
pub type __u32 = ffi::c_uint;
--
2.47.2Hi Alistair,
On Wed Aug 27, 2025 at 5:20 PM JST, Alistair Popple wrote:
<snip>
> index 161c057350622..1f51e354b9569 100644
> --- a/drivers/gpu/nova-core/gsp.rs
> +++ b/drivers/gpu/nova-core/gsp.rs
> @@ -6,12 +6,17 @@
> use kernel::dma_write;
> use kernel::pci;
> use kernel::prelude::*;
> -use kernel::ptr::Alignment;
> +use kernel::ptr::{Alignable, Alignment};
> +use kernel::sizes::SZ_128K;
> use kernel::transmute::{AsBytes, FromBytes};
>
> +use crate::fb::FbLayout;
> +use crate::firmware::Firmware;
> use crate::nvfw::{
> - LibosMemoryRegionInitArgument, LibosMemoryRegionKind_LIBOS_MEMORY_REGION_CONTIGUOUS,
> - LibosMemoryRegionLoc_LIBOS_MEMORY_REGION_LOC_SYSMEM,
> + GspFwWprMeta, GspFwWprMetaBootInfo, GspFwWprMetaBootResumeInfo, LibosMemoryRegionInitArgument,
> + LibosMemoryRegionKind_LIBOS_MEMORY_REGION_CONTIGUOUS,
> + LibosMemoryRegionLoc_LIBOS_MEMORY_REGION_LOC_SYSMEM, GSP_FW_WPR_META_MAGIC,
> + GSP_FW_WPR_META_REVISION,
> };
>
> pub(crate) const GSP_PAGE_SHIFT: usize = 12;
> @@ -25,12 +30,69 @@ unsafe impl AsBytes for LibosMemoryRegionInitArgument {}
> // are valid.
> unsafe impl FromBytes for LibosMemoryRegionInitArgument {}
>
> +// SAFETY: Padding is explicit and will not contain uninitialized data.
> +unsafe impl AsBytes for GspFwWprMeta {}
> +
> +// SAFETY: This struct only contains integer types for which all bit patterns
> +// are valid.
> +unsafe impl FromBytes for GspFwWprMeta {}
> +
> #[allow(unused)]
> pub(crate) struct GspMemObjects {
> libos: CoherentAllocation<LibosMemoryRegionInitArgument>,
> pub loginit: CoherentAllocation<u8>,
> pub logintr: CoherentAllocation<u8>,
> pub logrm: CoherentAllocation<u8>,
> + pub wpr_meta: CoherentAllocation<GspFwWprMeta>,
> +}
I think `wpr_meta` is a bit out-of-place in this structure. There are
several reason for this:
- All the other members of this structure (including `cmdq` which is
added later) are referenced by `libos` and constitute the GSP runtime:
they are used as long as the GSP is active. `wpr_meta`, OTOH, does not
reference any of the other objects, nor is it referenced by them.
- `wpr_meta` is never used by the GSP, but needed as a parameter of
Booter on SEC2 to load the GSP firmware. It can actually be discarded
once this step is completed. This is very different from the rest of
this structure, which is used by the GSP.
So I think it doesn't really belong here, and would probably fit better
in `Firmware`. Now the fault lies in my own series, which doesn't let
you build `wpr_meta` easily from there. I'll try to fix that in the v3.
And with the removal of `wpr_meta`, this structure ends up strictly
containing the GSP runtime, including the command queue... Maybe it can
simply be named `Gsp` then? It is even already in the right module! :)
Loosely related, but looking at this series made me realize there is a
very logical split of our firmware into two "bundles":
- The GSP bundle includes the GSP runtime data, which is this
`GspMemObjects` structure minus `wpr_meta`. We pass it as an input
parameter to the GSP firmware using the GSP's falcon mbox registers.
It must live as long as the GSP is running.
- The SEC2 bundle includes Booter, `wpr_meta`, the GSP firmware binary,
bootloader and its signatures (which are all referenced by
`wpr_meta`). All this data is consumed by SEC2, and crucially can be
dropped once the GSP is booted.
This separation is important as currently we are stuffing anything
firmware-related into the `Firmware` struct and keep it there forever,
consuming dozens of megabytes of host memory that we could free. Booting
the GSP is typically a one-time operation in the life of the GPU device,
and even if we ever need to do it again, we can very well build the SEC2
bundle from scratch again.
I will try to reflect the separation better in the v3 of my patchset -
then we can just build `wpr_meta` as a local variable of the method that
runs `Booter`, and drop it (alongside the rest of the SEC2 bundle) upon
return.
> +
> +pub(crate) fn build_wpr_meta(
> + dev: &device::Device<device::Bound>,
> + fw: &Firmware,
> + fb_layout: &FbLayout,
> +) -> Result<CoherentAllocation<GspFwWprMeta>> {
> + let wpr_meta =
> + CoherentAllocation::<GspFwWprMeta>::alloc_coherent(dev, 1, GFP_KERNEL | __GFP_ZERO)?;
> + dma_write!(
> + wpr_meta[0] = GspFwWprMeta {
> + magic: GSP_FW_WPR_META_MAGIC as u64,
> + revision: u64::from(GSP_FW_WPR_META_REVISION),
> + sysmemAddrOfRadix3Elf: fw.gsp.lvl0_dma_handle(),
> + sizeOfRadix3Elf: fw.gsp.size as u64,
> + sysmemAddrOfBootloader: fw.gsp_bootloader.ucode.dma_handle(),
> + sizeOfBootloader: fw.gsp_bootloader.ucode.size() as u64,
> + bootloaderCodeOffset: u64::from(fw.gsp_bootloader.code_offset),
> + bootloaderDataOffset: u64::from(fw.gsp_bootloader.data_offset),
> + bootloaderManifestOffset: u64::from(fw.gsp_bootloader.manifest_offset),
> + __bindgen_anon_1: GspFwWprMetaBootResumeInfo {
> + __bindgen_anon_1: GspFwWprMetaBootInfo {
> + sysmemAddrOfSignature: fw.gsp_sigs.dma_handle(),
> + sizeOfSignature: fw.gsp_sigs.size() as u64,
> + }
> + },
> + gspFwRsvdStart: fb_layout.heap.start,
> + nonWprHeapOffset: fb_layout.heap.start,
> + nonWprHeapSize: fb_layout.heap.end - fb_layout.heap.start,
> + gspFwWprStart: fb_layout.wpr2.start,
> + gspFwHeapOffset: fb_layout.wpr2_heap.start,
> + gspFwHeapSize: fb_layout.wpr2_heap.end - fb_layout.wpr2_heap.start,
> + gspFwOffset: fb_layout.elf.start,
> + bootBinOffset: fb_layout.boot.start,
> + frtsOffset: fb_layout.frts.start,
> + frtsSize: fb_layout.frts.end - fb_layout.frts.start,
> + gspFwWprEnd: fb_layout
> + .vga_workspace
> + .start
> + .align_down(Alignment::new(SZ_128K)),
> + gspFwHeapVfPartitionCount: fb_layout.vf_partition_count,
> + fbSize: fb_layout.fb.end - fb_layout.fb.start,
> + vgaWorkspaceOffset: fb_layout.vga_workspace.start,
> + vgaWorkspaceSize: fb_layout.vga_workspace.end - fb_layout.vga_workspace.start,
> + ..Default::default()
> + }
> + )?;
> +
> + Ok(wpr_meta)
I've discussed the bindings abstractions with Danilo last week. We
agreed that no layout information should ever escape the `nvfw` module.
I.e. the fields of `GspFwWprMeta` should not even be visible here.
Instead, `GspFwWprMeta` should be wrapped privately into another
structure inside `nvfw`:
/// Structure passed to the GSP bootloader, containing the framebuffer layout as well as the DMA
/// addresses of the GSP bootloader and firmware.
#[repr(transparent)]
pub(crate) struct GspFwWprMeta(r570_144::GspFwWprMeta);
All its implementations should also be there:
// SAFETY: Padding is explicit and will not contain uninitialized data.
unsafe impl AsBytes for GspFwWprMeta {}
// SAFETY: This struct only contains integer types for which all bit patterns
// are valid.
unsafe impl FromBytes for GspFwWprMeta {}
And lastly, this `new` method can also be moved into `nvfw`, as an impl
block for the wrapping `GspFwWprMeta` type. That way no layout detail
escapes that module, and it will be easier to adapt the code to
potential layout chances with new firmware versions.
(note that my series is the one carelessly re-exporting `GspFwWprMeta`
as-is - I'll fix that too in v3)
The same applies to `LibosMemoryRegionInitArgument` of the previous
patch, and other types introduced in subsequent patches. Usually there
is little more work to do than moving the implentations into `nvfw` as
everything is already abstracted correctly - just not where we
eventually want it.
On 2025-09-01 at 17:46 +1000, Alexandre Courbot <acourbot@nvidia.com> wrote...
> Hi Alistair,
>
> On Wed Aug 27, 2025 at 5:20 PM JST, Alistair Popple wrote:
> <snip>
> > index 161c057350622..1f51e354b9569 100644
> > --- a/drivers/gpu/nova-core/gsp.rs
> > +++ b/drivers/gpu/nova-core/gsp.rs
> > @@ -6,12 +6,17 @@
> > use kernel::dma_write;
> > use kernel::pci;
> > use kernel::prelude::*;
> > -use kernel::ptr::Alignment;
> > +use kernel::ptr::{Alignable, Alignment};
> > +use kernel::sizes::SZ_128K;
> > use kernel::transmute::{AsBytes, FromBytes};
> >
> > +use crate::fb::FbLayout;
> > +use crate::firmware::Firmware;
> > use crate::nvfw::{
> > - LibosMemoryRegionInitArgument, LibosMemoryRegionKind_LIBOS_MEMORY_REGION_CONTIGUOUS,
> > - LibosMemoryRegionLoc_LIBOS_MEMORY_REGION_LOC_SYSMEM,
> > + GspFwWprMeta, GspFwWprMetaBootInfo, GspFwWprMetaBootResumeInfo, LibosMemoryRegionInitArgument,
> > + LibosMemoryRegionKind_LIBOS_MEMORY_REGION_CONTIGUOUS,
> > + LibosMemoryRegionLoc_LIBOS_MEMORY_REGION_LOC_SYSMEM, GSP_FW_WPR_META_MAGIC,
> > + GSP_FW_WPR_META_REVISION,
> > };
> >
> > pub(crate) const GSP_PAGE_SHIFT: usize = 12;
> > @@ -25,12 +30,69 @@ unsafe impl AsBytes for LibosMemoryRegionInitArgument {}
> > // are valid.
> > unsafe impl FromBytes for LibosMemoryRegionInitArgument {}
> >
> > +// SAFETY: Padding is explicit and will not contain uninitialized data.
> > +unsafe impl AsBytes for GspFwWprMeta {}
> > +
> > +// SAFETY: This struct only contains integer types for which all bit patterns
> > +// are valid.
> > +unsafe impl FromBytes for GspFwWprMeta {}
> > +
> > #[allow(unused)]
> > pub(crate) struct GspMemObjects {
> > libos: CoherentAllocation<LibosMemoryRegionInitArgument>,
> > pub loginit: CoherentAllocation<u8>,
> > pub logintr: CoherentAllocation<u8>,
> > pub logrm: CoherentAllocation<u8>,
> > + pub wpr_meta: CoherentAllocation<GspFwWprMeta>,
> > +}
>
> I think `wpr_meta` is a bit out-of-place in this structure. There are
> several reason for this:
>
> - All the other members of this structure (including `cmdq` which is
> added later) are referenced by `libos` and constitute the GSP runtime:
> they are used as long as the GSP is active. `wpr_meta`, OTOH, does not
> reference any of the other objects, nor is it referenced by them.
> - `wpr_meta` is never used by the GSP, but needed as a parameter of
> Booter on SEC2 to load the GSP firmware. It can actually be discarded
> once this step is completed. This is very different from the rest of
> this structure, which is used by the GSP.
Yes, I had noticed that too and had tried to remove it previously. But as you
mention below that was a little bit tricky but if you fix it for v3 I think this
all makes perfect sense.
> So I think it doesn't really belong here, and would probably fit better
> in `Firmware`. Now the fault lies in my own series, which doesn't let
> you build `wpr_meta` easily from there. I'll try to fix that in the v3.
>
> And with the removal of `wpr_meta`, this structure ends up strictly
> containing the GSP runtime, including the command queue... Maybe it can
> simply be named `Gsp` then? It is even already in the right module! :)
Agreed - I noticed this right after I renamed this struct last time so wanted
to let things settle down a bit before doing another rename. But I think `Gsp`
makes a whole lot more sense, especially if we remove the wpr_meta data.
> Loosely related, but looking at this series made me realize there is a
> very logical split of our firmware into two "bundles":
>
> - The GSP bundle includes the GSP runtime data, which is this
> `GspMemObjects` structure minus `wpr_meta`. We pass it as an input
> parameter to the GSP firmware using the GSP's falcon mbox registers.
> It must live as long as the GSP is running.
> - The SEC2 bundle includes Booter, `wpr_meta`, the GSP firmware binary,
> bootloader and its signatures (which are all referenced by
> `wpr_meta`). All this data is consumed by SEC2, and crucially can be
> dropped once the GSP is booted.
>
> This separation is important as currently we are stuffing anything
> firmware-related into the `Firmware` struct and keep it there forever,
> consuming dozens of megabytes of host memory that we could free. Booting
> the GSP is typically a one-time operation in the life of the GPU device,
> and even if we ever need to do it again, we can very well build the SEC2
> bundle from scratch again.
>
> I will try to reflect the separation better in the v3 of my patchset -
> then we can just build `wpr_meta` as a local variable of the method that
> runs `Booter`, and drop it (alongside the rest of the SEC2 bundle) upon
> return.
>
> > +
> > +pub(crate) fn build_wpr_meta(
> > + dev: &device::Device<device::Bound>,
> > + fw: &Firmware,
> > + fb_layout: &FbLayout,
> > +) -> Result<CoherentAllocation<GspFwWprMeta>> {
> > + let wpr_meta =
> > + CoherentAllocation::<GspFwWprMeta>::alloc_coherent(dev, 1, GFP_KERNEL | __GFP_ZERO)?;
> > + dma_write!(
> > + wpr_meta[0] = GspFwWprMeta {
> > + magic: GSP_FW_WPR_META_MAGIC as u64,
> > + revision: u64::from(GSP_FW_WPR_META_REVISION),
> > + sysmemAddrOfRadix3Elf: fw.gsp.lvl0_dma_handle(),
> > + sizeOfRadix3Elf: fw.gsp.size as u64,
> > + sysmemAddrOfBootloader: fw.gsp_bootloader.ucode.dma_handle(),
> > + sizeOfBootloader: fw.gsp_bootloader.ucode.size() as u64,
> > + bootloaderCodeOffset: u64::from(fw.gsp_bootloader.code_offset),
> > + bootloaderDataOffset: u64::from(fw.gsp_bootloader.data_offset),
> > + bootloaderManifestOffset: u64::from(fw.gsp_bootloader.manifest_offset),
> > + __bindgen_anon_1: GspFwWprMetaBootResumeInfo {
> > + __bindgen_anon_1: GspFwWprMetaBootInfo {
> > + sysmemAddrOfSignature: fw.gsp_sigs.dma_handle(),
> > + sizeOfSignature: fw.gsp_sigs.size() as u64,
> > + }
> > + },
> > + gspFwRsvdStart: fb_layout.heap.start,
> > + nonWprHeapOffset: fb_layout.heap.start,
> > + nonWprHeapSize: fb_layout.heap.end - fb_layout.heap.start,
> > + gspFwWprStart: fb_layout.wpr2.start,
> > + gspFwHeapOffset: fb_layout.wpr2_heap.start,
> > + gspFwHeapSize: fb_layout.wpr2_heap.end - fb_layout.wpr2_heap.start,
> > + gspFwOffset: fb_layout.elf.start,
> > + bootBinOffset: fb_layout.boot.start,
> > + frtsOffset: fb_layout.frts.start,
> > + frtsSize: fb_layout.frts.end - fb_layout.frts.start,
> > + gspFwWprEnd: fb_layout
> > + .vga_workspace
> > + .start
> > + .align_down(Alignment::new(SZ_128K)),
> > + gspFwHeapVfPartitionCount: fb_layout.vf_partition_count,
> > + fbSize: fb_layout.fb.end - fb_layout.fb.start,
> > + vgaWorkspaceOffset: fb_layout.vga_workspace.start,
> > + vgaWorkspaceSize: fb_layout.vga_workspace.end - fb_layout.vga_workspace.start,
> > + ..Default::default()
> > + }
> > + )?;
> > +
> > + Ok(wpr_meta)
>
> I've discussed the bindings abstractions with Danilo last week. We
> agreed that no layout information should ever escape the `nvfw` module.
> I.e. the fields of `GspFwWprMeta` should not even be visible here.
>
> Instead, `GspFwWprMeta` should be wrapped privately into another
> structure inside `nvfw`:
>
> /// Structure passed to the GSP bootloader, containing the framebuffer layout as well as the DMA
> /// addresses of the GSP bootloader and firmware.
> #[repr(transparent)]
> pub(crate) struct GspFwWprMeta(r570_144::GspFwWprMeta);
I'm a little bit unsure what the advantage of this is? Admittedly I'm not sure
I've seen the discussion from last week so I may have missed something but it's
not obvious how creating another layer of abstraction is better. How would it
help contain any layout changes to nvfw? Supporting any new struct fields for
example would almost certainly still require code changes outside nvfw.
My thinking here was that the bindings (at least for GSP) probably want to live
in the Gsp crate/module, and the rest of the driver would be isolated from Gsp
changes by the public API provided by the Gsp crate/module rather than trying to
do that at the binding level. For example the get_gsp_info() command implemented
in [1] provides a separate public struct representing what the rest of the
driver needs, thus ensuring the implementation specific details of Gsp (such as
struct layout) do not leak into the wider nova-core driver.
> All its implementations should also be there:
>
> // SAFETY: Padding is explicit and will not contain uninitialized data.
> unsafe impl AsBytes for GspFwWprMeta {}
>
> // SAFETY: This struct only contains integer types for which all bit patterns
> // are valid.
> unsafe impl FromBytes for GspFwWprMeta {}
Makes sense.
> And lastly, this `new` method can also be moved into `nvfw`, as an impl
> block for the wrapping `GspFwWprMeta` type. That way no layout detail
> escapes that module, and it will be easier to adapt the code to
> potential layout chances with new firmware versions.
>
> (note that my series is the one carelessly re-exporting `GspFwWprMeta`
> as-is - I'll fix that too in v3)
>
> The same applies to `LibosMemoryRegionInitArgument` of the previous
> patch, and other types introduced in subsequent patches. Usually there
> is little more work to do than moving the implentations into `nvfw` as
> everything is already abstracted correctly - just not where we
> eventually want it.
This is where I get a little bit uncomfortable - this doesn't feel right to me.
It seems to me moving all these implementations to the bindings would just end
up with a significant amount of Gsp code in nvfw.rs rather than in the places
that actually use it, making nvfw.rs large and unwieldy and the code more
distributed and harder to follow.
And it's all tightly coupled anyway - for example the Gsp boot arguments require some
command queue offsets which are all pretty specific to the Gsp implementation.
Ie. we can't define some nice public API in the Gsp crate for "getting arguments
required for booting Gsp" without that just being "here is a struct containing
all the fields that must be packed into the Gsp arguments for this version",
which at that point may as well just be the actual struct itself right?
- Alistair
[1] - https://lore.kernel.org/rust-for-linux/20250829173254.2068763-18-joelagnelf@nvidia.com/
On Wed Sep 3, 2025 at 5:57 PM JST, Alistair Popple wrote:
<snip>
>> I've discussed the bindings abstractions with Danilo last week. We
>> agreed that no layout information should ever escape the `nvfw` module.
>> I.e. the fields of `GspFwWprMeta` should not even be visible here.
>>
>> Instead, `GspFwWprMeta` should be wrapped privately into another
>> structure inside `nvfw`:
>>
>> /// Structure passed to the GSP bootloader, containing the framebuffer layout as well as the DMA
>> /// addresses of the GSP bootloader and firmware.
>> #[repr(transparent)]
>> pub(crate) struct GspFwWprMeta(r570_144::GspFwWprMeta);
>
> I'm a little bit unsure what the advantage of this is? Admittedly I'm not sure
> I've seen the discussion from last week so I may have missed something but it's
> not obvious how creating another layer of abstraction is better. How would it
> help contain any layout changes to nvfw? Supporting any new struct fields for
> example would almost certainly still require code changes outside nvfw.
It is not as much creating a new abstraction layer as it is controlling
where it resides - nicely contained in `nvfw` or all over the place.
This is particularly relevant if we consider that binding abstractions
are more likely to require `unsafe` code, that we will then be able to
confine to the `nvfw` module. As I got reminded in my own series, we
don't want `unsafe` code in regular driver modules.
Even if a new field is added to `GspFwWprMeta`, there is a good chance
that the parameters of its current constructor will cover what we need
to initialize it, so the calling code outside of `nvfw` won't need to
change. Of course we cannot guarantee this will be true all the time,
but it still covers us better than the alternative.
And then there is the question of if/when we need to support several
firmware versions. If we start having code in `gsp` that is specific to
a given firmware version, this is already a non-starter. Whereas having
all the abstractions in a single module leaves us in a better position
to use trait objects and virtual calls, or apply proc-macro magic.
>
> My thinking here was that the bindings (at least for GSP) probably want to live
> in the Gsp crate/module, and the rest of the driver would be isolated from Gsp
> changes by the public API provided by the Gsp crate/module rather than trying to
> do that at the binding level. For example the get_gsp_info() command implemented
> in [1] provides a separate public struct representing what the rest of the
> driver needs, thus ensuring the implementation specific details of Gsp (such as
> struct layout) do not leak into the wider nova-core driver.
>
>> All its implementations should also be there:
>>
>> // SAFETY: Padding is explicit and will not contain uninitialized data.
>> unsafe impl AsBytes for GspFwWprMeta {}
>>
>> // SAFETY: This struct only contains integer types for which all bit patterns
>> // are valid.
>> unsafe impl FromBytes for GspFwWprMeta {}
>
> Makes sense.
>
>> And lastly, this `new` method can also be moved into `nvfw`, as an impl
>> block for the wrapping `GspFwWprMeta` type. That way no layout detail
>> escapes that module, and it will be easier to adapt the code to
>> potential layout chances with new firmware versions.
>>
>> (note that my series is the one carelessly re-exporting `GspFwWprMeta`
>> as-is - I'll fix that too in v3)
>>
>> The same applies to `LibosMemoryRegionInitArgument` of the previous
>> patch, and other types introduced in subsequent patches. Usually there
>> is little more work to do than moving the implentations into `nvfw` as
>> everything is already abstracted correctly - just not where we
>> eventually want it.
>
> This is where I get a little bit uncomfortable - this doesn't feel right to me.
> It seems to me moving all these implementations to the bindings would just end
> up with a significant amount of Gsp code in nvfw.rs rather than in the places
> that actually use it, making nvfw.rs large and unwieldy and the code more
> distributed and harder to follow.
If we want to split things more logically, I think it's perfectly fine
to have e.g. a `nvfw/gsp` module that contains all the GSP abstractions,
another one for the sequencer, etc. As long as all the version-specific
bits are contained below `nvfw`.
>
> And it's all tightly coupled anyway - for example the Gsp boot arguments require some
> command queue offsets which are all pretty specific to the Gsp implementation.
> Ie. we can't define some nice public API in the Gsp crate for "getting arguments
> required for booting Gsp" without that just being "here is a struct containing
> all the fields that must be packed into the Gsp arguments for this version",
> which at that point may as well just be the actual struct itself right?
Which particular structure are you refering to?
On Wed Sep 3, 2025 at 9:51 PM JST, Alexandre Courbot wrote:
>> And it's all tightly coupled anyway - for example the Gsp boot arguments require some
>> command queue offsets which are all pretty specific to the Gsp implementation.
>> Ie. we can't define some nice public API in the Gsp crate for "getting arguments
>> required for booting Gsp" without that just being "here is a struct containing
>> all the fields that must be packed into the Gsp arguments for this version",
>> which at that point may as well just be the actual struct itself right?
>
> Which particular structure are you refering to?
Ah, I guess that was about `GspArgumentsCached` and the message queue's
`get_cmdq_offsets` method. For this I guess we can just have a
fn new(cmdq: &GspCmdq) -> Self
constructor for `GspArgumentsCached` which grabs the information it
needs from the queue and initializes itself. You would need to have this
code anyway, it's just a matter of where we put it - inside a function
of `gsp.rs`, or as a reusable (and easily replacable) constructor.
© 2016 - 2025 Red Hat, Inc.