Message-Id: <DD08VDPA45X8.2VXMD1RJBAJER@nvidia.com>
Date: Tue, 23 Sep 2025 23:20:37 +0900
From: "Alexandre Courbot" <acourbot@...dia.com>
To: "Alistair Popple" <apopple@...dia.com>,
 <rust-for-linux@...r.kernel.org>, <dri-devel@...ts.freedesktop.org>,
 <dakr@...nel.org>, <acourbot@...dia.com>
Cc: "Miguel Ojeda" <ojeda@...nel.org>, "Alex Gaynor"
 <alex.gaynor@...il.com>, "Boqun Feng" <boqun.feng@...il.com>, "Gary Guo"
 <gary@...yguo.net>, Björn Roy Baron
 <bjorn3_gh@...tonmail.com>, "Benno Lossin" <lossin@...nel.org>, "Andreas
 Hindborg" <a.hindborg@...nel.org>, "Alice Ryhl" <aliceryhl@...gle.com>,
 "Trevor Gross" <tmgross@...ch.edu>, "David Airlie" <airlied@...il.com>,
 "Simona Vetter" <simona@...ll.ch>, "Maarten Lankhorst"
 <maarten.lankhorst@...ux.intel.com>, "Maxime Ripard" <mripard@...nel.org>,
 "Thomas Zimmermann" <tzimmermann@...e.de>, "John Hubbard"
 <jhubbard@...dia.com>, "Joel Fernandes" <joelagnelf@...dia.com>, "Timur
 Tabi" <ttabi@...dia.com>, <linux-kernel@...r.kernel.org>,
 <nouveau@...ts.freedesktop.org>
Subject: Re: [PATCH v2 02/10] gpu: nova-core: Create initial Gsp

Hi Alistair,

On Mon Sep 22, 2025 at 8:30 PM JST, Alistair Popple wrote:
> The GSP requires several areas of memory to operate. Each of these have
> their own simple embedded page tables. Set these up and map them for DMA
> to/from GSP using CoherentAllocation's. Return the DMA handle describing
> where each of these regions are for future use when booting GSP.
>
> Signed-off-by: Alistair Popple <apopple@...dia.com>
>
> ---
>
> Changes for v2:
>
>  - Renamed GspMemOjbects to Gsp as that is what they are
>  - Rebased on Alex's latest series
> ---
>  drivers/gpu/nova-core/gpu.rs                  |  2 +-
>  drivers/gpu/nova-core/gsp.rs                  | 80 +++++++++++++++++--
>  drivers/gpu/nova-core/gsp/fw.rs               | 39 +++++++++
>  .../gpu/nova-core/gsp/fw/r570_144/bindings.rs | 19 +++++
>  4 files changed, 131 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/nova-core/gpu.rs b/drivers/gpu/nova-core/gpu.rs
> index 5da9ad726483..c939b3868271 100644
> --- a/drivers/gpu/nova-core/gpu.rs
> +++ b/drivers/gpu/nova-core/gpu.rs
> @@ -221,7 +221,7 @@ pub(crate) fn new<'a>(
>  
>              sec2_falcon: Falcon::new(pdev.as_ref(), spec.chipset, bar, true)?,
>  
> -            gsp <- Gsp::new(),
> +            gsp <- Gsp::new(pdev)?,
>  
>              _: { gsp.boot(pdev, bar, spec.chipset, gsp_falcon, sec2_falcon)? },
>  
> diff --git a/drivers/gpu/nova-core/gsp.rs b/drivers/gpu/nova-core/gsp.rs
> index 503ce8ee0420..0185f66971ff 100644
> --- a/drivers/gpu/nova-core/gsp.rs
> +++ b/drivers/gpu/nova-core/gsp.rs
> @@ -1,27 +1,91 @@
>  // SPDX-License-Identifier: GPL-2.0
>  
>  mod boot;
> -
> -use kernel::prelude::*;
> -

Oops, not sure why I put that here but thanks for fixing this.

>  mod fw;
>  
>  pub(crate) use fw::{GspFwWprMeta, LibosParams};
>  
> +use kernel::device;
> +use kernel::dma::CoherentAllocation;
> +use kernel::dma_write;
> +use kernel::pci;
> +use kernel::prelude::*;
>  use kernel::ptr::Alignment;
> +use kernel::transmute::{AsBytes, FromBytes};
> +
> +use fw::LibosMemoryRegionInitArgument;
>  
>  pub(crate) const GSP_PAGE_SHIFT: usize = 12;
>  pub(crate) const GSP_PAGE_SIZE: usize = 1 << GSP_PAGE_SHIFT;
>  pub(crate) const GSP_HEAP_ALIGNMENT: Alignment = Alignment::new::<{ 1 << 20 }>();
>  
>  /// GSP runtime data.
> -///
> -/// This is an empty pinned placeholder for now.
>  #[pin_data]
> -pub(crate) struct Gsp {}
> +pub(crate) struct Gsp {
> +    libos: CoherentAllocation<LibosMemoryRegionInitArgument>,
> +    pub loginit: CoherentAllocation<u8>,
> +    pub logintr: CoherentAllocation<u8>,
> +    pub logrm: CoherentAllocation<u8>,

These don't need to be public for now.

> +}
> +
> +/// Creates a self-mapping page table for `obj` at its beginning.
> +fn create_pte_array(obj: &mut CoherentAllocation<u8>) {
> +    let num_pages = obj.size().div_ceil(GSP_PAGE_SIZE);
> +    let handle = obj.dma_handle();
> +
> +    // SAFETY:
> +    //  - By the invariants of the CoherentAllocation ptr is non-NULL.
> +    //  - CoherentAllocation CPU addresses are always aligned to a
> +    //    page-boundary, satisfying the alignment requirements for
> +    //    from_raw_parts_mut()
> +    //  - The allocation size is at least as long as 8 * num_pages as
> +    //    GSP_PAGE_SIZE is larger than 8 bytes.
> +    let ptes = unsafe {
> +        let ptr = obj.start_ptr_mut().cast::<u64>().add(1);
> +        core::slice::from_raw_parts_mut(ptr, num_pages)
> +    };

I think we also need to provide the same guarantees as
`CoherentAllocation::as_slice_mut`: 

* Callers must ensure that the device does not read/write to/from memory
  while the returned slice is live.
* Callers must ensure that this call does not race with a read or write
  to the same region while the returned slice is live.

Unfortunately I don't think these are covered by this function alone -
it could perfectly be called on an allocation that is currently in use
by the hardware. So technically `create_pte_arrays` in its present form
should be unsafe, but that also would be overkill here since it is a
local function and we control the conditions into which it is called.

This PTE business, where we are taking any coherent allocation and
reinterpreting its bytes to create a page table, does not look very
clean to me anyway, so maybe we can solve this with a redesign. I'd
rather have that part of the object explicitly laid out, as it is for
`GspMem` later in this series (albeit with `u8` instead of `u64`):

    struct GspMem {
        ptes: [u8; GSP_PAGE_SIZE],
        ...

What would work best here IMHO would be to have a dedicated type for the
array of PTEs, which is placed at the beginning of each object
requesting one. Then we could have an init generic method for it that
takes a reference to any object and `dma_write!`s its entries, avoiding
the slice and having a guarantee that we have exclusive access to the
object since we just created it one line above. I need to think a bit
more about this but this should be a solid basis.

As it happens, loginit, logintr and logrm all have the same size, so we
should be able to declare a new type for these 3.

> +
> +    for (i, pte) in ptes.iter_mut().enumerate() {
> +        *pte = handle + ((i as u64) << GSP_PAGE_SHIFT);
> +    }
> +}
> +
> +/// Creates a new `CoherentAllocation<A>` with `name` of `size` elements, and
> +/// register it into the `libos` object at argument position `libos_arg_nr`.
> +fn create_coherent_dma_object<A: AsBytes + FromBytes>(
> +    dev: &device::Device<device::Bound>,
> +    name: &'static str,
> +    size: usize,
> +    libos: &mut CoherentAllocation<LibosMemoryRegionInitArgument>,
> +    libos_arg_nr: usize,
> +) -> Result<CoherentAllocation<A>> {
> +    let obj = CoherentAllocation::<A>::alloc_coherent(dev, size, GFP_KERNEL | __GFP_ZERO)?;
> +
> +    dma_write!(libos[libos_arg_nr] = LibosMemoryRegionInitArgument::new(name, &obj))?;

Once we have a dedicated type for DMA objects with PTEs, I suggest to
move this `dma_write` outside of this function. It doesn't bring any
value to have it here, and that way we can remove
`create_coherent_dma_object` altogether. Let me clarify below.

> +
> +    Ok(obj)
> +}
>  
>  impl Gsp {
> -    pub(crate) fn new() -> impl PinInit<Self> {
> -        pin_init!(Self {})
> +    pub(crate) fn new(pdev: &pci::Device<device::Bound>) -> Result<impl PinInit<Self, Error>> {
> +        let dev = pdev.as_ref();
> +        let mut libos = CoherentAllocation::<LibosMemoryRegionInitArgument>::alloc_coherent(
> +            dev,
> +            GSP_PAGE_SIZE / size_of::<LibosMemoryRegionInitArgument>(),
> +            GFP_KERNEL | __GFP_ZERO,
> +        )?;
> +        let mut loginit = create_coherent_dma_object::<u8>(dev, "LOGINIT", 0x10000, &mut libos, 0)?;
> +        create_pte_array(&mut loginit);
> +        let mut logintr = create_coherent_dma_object::<u8>(dev, "LOGINTR", 0x10000, &mut libos, 1)?;
> +        create_pte_array(&mut logintr);
> +        let mut logrm = create_coherent_dma_object::<u8>(dev, "LOGRM", 0x10000, &mut libos, 2)?;
> +        create_pte_array(&mut logrm);

So with the proper PTE-prefixed type, this code would become:

    let loginit = PteDmaObject::new(dev)?;
    let logintr = PteDmaObject::new(dev)?;
    let logrm = PteDmaObject::new(dev)?;

    dma_write!(libos[0] = LibosMemoryRegionInitArgument::new("LOGINIT", &loginit))?;
    dma_write!(libos[1] = LibosMemoryRegionInitArgument::new("LOGINTR", &logintr))?;
    dma_write!(libos[2] = LibosMemoryRegionInitArgument::new("LOGRM", &logrm))?;

Note how loginit and friends now don't need to be mutable anymore.

It's getting late here so let me continue the review tomorrow. :)

Powered by blists - more mailing lists