gpu: nova-core: expose the logging buffers via debugfs

Add a name() method to the Device type, which returns a CStr that contains the device name.

Signed-off-by Timur Tabi <[email protected]>

Reviewed-by Alice Ryhl <[email protected]>

Reviewed-by Alexandre Courbot <[email protected]>

---
 rust/helpers/device.c |  5 +++++
 rust/kernel/device.rs | 11 +++++++++++
 2 files changed, 16 insertions(+)

Add UserSliceWriter::write_dma() to copy data from a CoherentAllocation<u8> to userspace. This provides a safe interface for copying DMA buffer contents to userspace without requiring callers to work with raw pointers.

Because write_dma() and write_slice() have common code, factor that code out into a helper function, write_raw().

The method handles bounds checking and offset calculation internally, wrapping the unsafe copy_to_user() call.

Signed-off-by Timur Tabi <[email protected]>

Reviewed-by Alexandre Courbot <[email protected]>

rust/kernel/uaccess.rs +1 -1

+    unsafe fn write_raw(&mut self, ptr: *const u8, len: usize) -> Result {
         if len > self.length {
             return Err(EFAULT);
         }
-        // SAFETY: `data_ptr` points into an immutable slice of length `len`, so we may read

Quoted 12 lines · Timur Tabi

Add UserSliceWriter::write_dma() to copy data from a CoherentAllocation<u8> to userspace. This provides a safe interface for copying DMA buffer contents to userspace without requiring callers to work with raw pointers.

Because write_dma() and write_slice() have common code, factor that code out into a helper function, write_raw().

The method handles bounds checking and offset calculation internally, wrapping the unsafe copy_to_user() call.

Signed-off-by Timur Tabi <[email protected]>

Reviewed-by Alexandre Courbot <[email protected]>

Quoted 4 lines

+    /// # Safety
+    ///
+    /// The caller must ensure that ptr points to a valid slice of len bytes (i.e., it is +    /// valid for reads of len bytes).

I don't know how I feel about this 'valid slice' thing. The memory might be dma memory after all ... just say it's valid for reads of len bytes.

After all, in below SAFETY: comments, you also argue that it's valid for reads, not that it points at a valid slice.

Quoted 9 lines

+    unsafe fn write_raw(&mut self, ptr: *const u8, len: usize) -> Result {
         if len > self.length {
             return Err(EFAULT);
         }
-        // SAFETY: data_ptr points into an immutable slice of length len, so we may read -        // that many bytes from it.
-        let res = unsafe { bindings::copy_to_user(self.ptr.as_mut_ptr(), data_ptr, len) }; +        // SAFETY:
+        // - self.ptr is a userspace pointer, and len <= self.length is checked above to

Quoted 8 lines

+        //   ensure we don't exceed the caller-specified bounds.
+        // - ptr is valid for reading len bytes as required by this function's safety contract.
+        // - copy_to_user validates the userspace address at runtime and returns non-zero on +        //   failure (e.g., bad address or unmapped memory).
+        let res =
+            unsafe { bindings::copy_to_user(self.ptr.as_mut_ptr(), ptr.cast::<c_void>(), len) };

Points 1 and 3 in this bulleted list do not seem to address any actual safety requirements of copy_to_user. Yes, the function validates userspace addresses at runtime, and this is part of why its implementation is safe, but it's not a precondition on the caller. There's no way to call copy_to_user where it would not validate the userspace address at runtime.

Quoted 7 lines

+    /// Writes raw data to this user pointer from a DMA coherent allocation. +    ///
+    /// # Arguments
+    ///
+    /// * data - The DMA coherent allocation to copy from.
+    /// * offset - The byte offset into data to start copying from. +    /// * count - The number of bytes to copy.

This is not the usual Rust style for documentation. We would generally just write it in words:

Copies count bytes from alloc starting from offset into this userspace slice.

Yes, argument lists are *occassionally* used, but it's rare.

Quoted 7 lines

+    /// # Errors
+    ///
+    /// Returns [EOVERFLOW] if offset + count overflows.
+    /// Returns [ERANGE] if offset + count exceeds the size of data, or count exceeds +    ///     the size of the user-space buffer.
+    /// Returns [EFAULT] if the write happens on a bad address, or if the write goes out of +    ///     bounds of this [UserSliceWriter].

Using a list to format the possible errors seems sensible to me. But I don't think you intended for this to be rendered like this in the final docs:

Returns EOVERFLOW if offset + count overflows. Returns ERANGE if offset + count exceeds the size of data, or count exceeds the size of the user-space buffer. Returns EFAULT if the write happens on a bad address, or if the write goes out of bounds of this UserSliceWriter.

Please use a markdown list to format this, and check the generated html docs.

Quoted 19 lines · Alice Ryhl

Quoted 17 lines · Timur Tabi

Add UserSliceWriter::write_dma() to copy data from a CoherentAllocation<u8> to userspace. This provides a safe interface for copying DMA buffer contents to userspace without requiring callers to work with raw pointers.

Because write_dma() and write_slice() have common code, factor that code out into a helper function, write_raw().

The method handles bounds checking and offset calculation internally, wrapping the unsafe copy_to_user() call.

Signed-off-by Timur Tabi <[email protected]>

Reviewed-by Alexandre Courbot <[email protected]>

+    /// # Safety
+    ///
+    /// The caller must ensure that ptr points to a valid slice of len bytes (i.e., it is +    /// valid for reads of len bytes).

Quoted 3 lines

I don't know how I feel about this 'valid slice' thing. The memory might be dma memory after all ... just say it's valid for reads of len bytes.

Shouldn't the safety comment also say that len is valid for writes to self.ptr, not just reads for ptr? I know there is a "if len > self.length" check. Does that check obviate the need for a safety comment that references self.ptr?

Maybe I should rename ptr, to avoid confusion with self.ptr. Would be okay to rename it to from, since it's copying from that kernel address?

Quoted 21 lines

After all, in below SAFETY: comments, you also argue that it's valid for reads, not that it points at a valid slice.

Quoted 9 lines

+    unsafe fn write_raw(&mut self, ptr: *const u8, len: usize) -> Result {
         if len > self.length {
             return Err(EFAULT);
         }
-        // SAFETY: data_ptr points into an immutable slice of length len, so we may read -        // that many bytes from it.
-        let res = unsafe { bindings::copy_to_user(self.ptr.as_mut_ptr(), data_ptr, len) }; +        // SAFETY:
+        // - self.ptr is a userspace pointer, and len <= self.length is checked above to

Quoted 8 lines

+        //   ensure we don't exceed the caller-specified bounds.
+        // - ptr is valid for reading len bytes as required by this function's safety contract.
+        // - copy_to_user validates the userspace address at runtime and returns non-zero on +        //   failure (e.g., bad address or unmapped memory).
+        let res =
+            unsafe { bindings::copy_to_user(self.ptr.as_mut_ptr(), ptr.cast::<c_void>(), len) };

Quoted 6 lines

Points 1 and 3 in this bulleted list do not seem to address any actual safety requirements of copy_to_user. Yes, the function validates userspace addresses at runtime, and this is part of why its implementation is safe, but it's not a precondition on the caller. There's no way to call copy_to_user where it would not validate the userspace address at runtime.

Fair enough, but I'm struggling to come up with a safety comment that doesn't just repeat what's in the function comment:

    /// The caller must ensure that `ptr` is valid for reads of `len` bytes.

(and maybe also valid for writes to self.ptr).

It seems to me that we're just extending the safety conditions copy_to_user() directly to write_raw(). That is, the reason write_raw() is unsafe is because copy_to_user() is unsafe. Therefore, shouldn't the safety comment for copy_to_user just be:

// SAFETY: Caller guarantees ptr is valid for len bytes (see this function's safety contract).

Add UserSliceWriter::write_dma() to copy data from a CoherentAllocation<u8> to userspace. This provides a safe interface for copying DMA buffer contents to userspace without requiring callers to work with raw pointers.

Because write_dma() and write_slice() have common code, factor that code out into a helper function, write_raw().

The method handles bounds checking and offset calculation internally, wrapping the unsafe copy_to_user() call.

Signed-off-by Timur Tabi <[email protected]>

Reviewed-by Alexandre Courbot <[email protected]>

By the way, there is a typo in both the arguments and errors -- there is no data parameter (which I guess came from the other methods).

Since I imagine you will send a new version, please consider adding an example, even if it doesn't run (like the others). If it did run, then I would suggest exercising the error paths.

Implement the BinaryWriter trait for CoherentAllocation<u8>, enabling DMA coherent allocations to be exposed as readable binary files. The implementation handles offset tracking and bounds checking, copying data from the coherent allocation to userspace via write_dma().

Signed-off-by Timur Tabi <[email protected]>

+ let count = self.count().saturating_sub(offset_val).min(writer.len());

Quoted 6 lines · Timur Tabi

Implement the BinaryWriter trait for CoherentAllocation<u8>, enabling DMA coherent allocations to be exposed as readable binary files. The implementation handles offset tracking and bounds checking, copying data from the coherent allocation to userspace via write_dma().

Signed-off-by Timur Tabi <[email protected]>

Reviewed-by Alice Ryhl <[email protected]>

Quoted 1 line

+ let count = self.count().saturating_sub(offset_val).min(writer.len());

It's kind of weird to call the length method self.count() rather than self.len(). All other methods called count() in the stdlib are cases where you get a length by literally counting with a for loop.

Anyway, not a problem of this patch. LGTM.

Alice

Implement the BinaryWriter trait for CoherentAllocation<u8>, enabling DMA coherent allocations to be exposed as readable binary files. The implementation handles offset tracking and bounds checking, copying data from the coherent allocation to userspace via write_dma().

Signed-off-by Timur Tabi <[email protected]>

Replace the module_pci_driver! macro with an explicit module initialization using the standard module! macro and InPlaceModule trait implementation. No functional change intended, with the exception that the driver now prints a message when loaded.

This change is necessary so that we can create a top-level "nova_core" debugfs entry when the driver is loaded.

Signed-off-by Timur Tabi <[email protected]>

---
 drivers/gpu/nova-core/nova_core.rs | 25 +++++++++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)

Create the 'nova_core' root debugfs entry when the driver loads.

Normally, non-const global variables need to be protected by a mutex. Instead, we use unsafe code, as we know the entry is never modified after the driver is loaded. This solves the lifetime issue of the mutex guard, which would otherwise have required the use of pin_init_scope.

Signed-off-by Timur Tabi <[email protected]>

---
 drivers/gpu/nova-core/nova_core.rs | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

Create read-only debugfs entries for LOGINIT, LOGRM, and LOGINTR, which are the three primary printf logging buffers from GSP-RM. LOGPMU will be added at a later date, as it requires it support for its RPC message first.

This patch uses the pin_init_scope feature to create the entries. pin_init_scope solves the lifetime issue over the DEBUGFS_ROOT reference by delaying its acquisition until the time the entry is actually initialized.

Co-developed-by Alexandre Courbot <[email protected]>

Signed-off-by Alexandre Courbot <[email protected]>

Signed-off-by Timur Tabi <[email protected]>

---
 drivers/gpu/nova-core/gsp.rs | 49 ++++++++++++++++++++++++++++++------
 1 file changed, 41 insertions(+), 8 deletions(-)

drivers/gpu/nova-core/gsp.rs +27 -8

index a6f3918c20b1..484e6aa48bf3 100644
@@ -3,6 +3,8 @@
 mod boot;
 
 use kernel::{
+    c_str,
+    debugfs,
     device,
     dma::{
         CoherentAllocation,

@@ -100,17 +102,24 @@ fn new(dev: &device::Device<device::Bound>) -> Result<Self> {
     }
 }
 
-/// GSP runtime data.
-#[pin_data]
-pub(crate) struct Gsp {
-    /// Libos arguments.
-    pub(crate) libos: CoherentAllocation<LibosMemoryRegionInitArgument>,
+/// Log buffers used by GSP-RM for debug logging.
+struct LogBuffers {
     /// Init log buffer.
     loginit: LogBuffer,
     /// Interrupts log buffer.
     logintr: LogBuffer,
     /// RM log buffer.
     logrm: LogBuffer,
+}
+
+/// GSP runtime data.
+#[pin_data]
+pub(crate) struct Gsp {
+    /// Libos arguments.
+    pub(crate) libos: CoherentAllocation<LibosMemoryRegionInitArgument>,
+    /// Log buffers, optionally exposed via debugfs.
+    #[pin]
+    logs: debugfs::Scope<LogBuffers>,
     /// Command queue.
     #[pin]
     pub(crate) cmdq: Cmdq,

@@ -124,15 +133,17 @@ pub(crate) fn new(pdev: &pci::Device<device::Bound>) -> impl PinInit<Self, Error
         pin_init::pin_init_scope(move || {
             let dev = pdev.as_ref();
 
+            // Create log buffers before try_pin_init! so they're accessible throughout
+            let loginit = LogBuffer::new(dev)?;
+            let logintr = LogBuffer::new(dev)?;
+            let logrm = LogBuffer::new(dev)?;
+
             Ok(try_pin_init!(Self {
                 libos: CoherentAllocation::<LibosMemoryRegionInitArgument>::alloc_coherent(
                     dev,
                     GSP_PAGE_SIZE / size_of::<LibosMemoryRegionInitArgument>(),
                     GFP_KERNEL | __GFP_ZERO,
                 )?,
-                loginit: LogBuffer::new(dev)?,
-                logintr: LogBuffer::new(dev)?,
-                logrm: LogBuffer::new(dev)?,
                 cmdq <- Cmdq::new(dev),
                 rmargs: CoherentAllocation::<GspArgumentsPadded>::alloc_coherent(
                     dev,

@@ -153,6 +164,28 @@ pub(crate) fn new(pdev: &pci::Device<device::Bound>) -> impl PinInit<Self, Error
                     dma_write!(rmargs[0].inner = fw::GspArgumentsCached::new(&cmdq))?;
                     dma_write!(libos[3] = LibosMemoryRegionInitArgument::new("RMARGS", rmargs))?;
                 },
+                logs <- {
+                    let log_buffers = LogBuffers {
+                        loginit,
+                        logintr,
+                        logrm,
+                    };
+
+                    #[allow(static_mut_refs)]

drivers/gpu/nova-core/gsp.rs +4 -0

+                        dir.read_binary_file(c_str!("logintr"), &logs.logintr.0);
+                        dir.read_binary_file(c_str!("logrm"), &logs.logrm.0);
+                    })
+                },
             }))
         })
     }

GSP-RM writes its printf message to "logging buffers", which are blocks memory allocated by the driver. The messages are encoded, so exposing the buffers as debugfs entries allows the buffers to be extracted and decoded by a special application.

When the driver loads, a /sys/kernel/debug/nova_core root entry is created. To do this, the normal module_pci_driver! macro call is replaced with an explicit initialization function, as this allows that debugfs entry to be created once for all GPUs.

Then in each GPU's initialization, a subdirectory based on the PCI BDF name is created, and the logging buffer entries are created under that.

This series depends on Eliot Courtney's Cmdq patch set [1].

Note: the debugfs entry has a file size of 0, because debugfs defaults a 0 size and the Rust abstractions do not adjust it for the same of the object. Nouveau makes this adjustment manually in the driver.

Signed-off-by Timur Tabi <[email protected]>

Tested-by John Hubbard <[email protected]>

GSP-RM writes its printf message to "logging buffers", which are blocks memory allocated by the driver. The messages are encoded, so exposing the buffers as debugfs entries allows the buffers to be extracted and decoded by a special application.

When the driver loads, a /sys/kernel/debug/nova_core root entry is created. To do this, the normal module_pci_driver! macro call is replaced with an explicit initialization function, as this allows that debugfs entry to be created once for all GPUs.

Then in each GPU's initialization, a subdirectory based on the PCI BDF name is created, and the logging buffer entries are created under that.

This series depends on Eliot Courtney's Cmdq patch set [1].

Note: the debugfs entry has a file size of 0, because debugfs defaults a 0 size and the Rust abstractions do not adjust it for the same of the object. Nouveau makes this adjustment manually in the driver.

Signed-off-by Timur Tabi <[email protected]>

Tested-by John Hubbard <[email protected]>

[1] https://lore.kernel.org/all/[email protected]/

Changes since v8:
1. Added #inline to Device::name()
2. Fixed rustfmt issues again
3. Updated write_to_slice()
4. Dropped "use pin projection in method boot()"
5. Now depends on Eliot's Cmdq patch set
6. Misc nits fixed

Timur Tabi (6):
rust: device: add device name method
rust: uaccess: add write_dma() for copying from DMA buffers to
userspace
rust: dma: implement BinaryWriter for CoherentAllocation<u8>
gpu: nova-core: Replace module_pci_driver! with explicit module init
gpu: nova-core: create debugfs root in module init
gpu: nova-core: create GSP-RM logging buffers debugfs entries

 drivers/gpu/nova-core/gsp.rs       | 49 ++++++++++++++---
 drivers/gpu/nova-core/nova_core.rs | 50 +++++++++++++++++-
 rust/helpers/device.c              |  5 ++
 rust/kernel/device.rs              | 11 ++++
 rust/kernel/dma.rs                 | 35 ++++++++++++-
 rust/kernel/uaccess.rs             | 84 ++++++++++++++++++++++++++----
 6 files changed, 213 insertions(+), 21 deletions(-)

base-commit: a544873ce0575b2fd8285a1364d3e09929d9a3ba
prerequisite-patch-id: fefd403caf8af386276351dd12397dda8ae8553f
prerequisite-patch-id: 3e02192944c4dde97e6895a28371479aa49ddc96
prerequisite-patch-id: de85fb30b65233210512b8295d7fe811f485f03f
prerequisite-patch-id: 4232928e15093c74aa6d2a6519a4d7ac5e109607
prerequisite-patch-id: d4f34f7eda8f5dcad2464f1bf3789e78556eb51c