* [PATCH v6 1/9] gpu: nova-core: gsp: sort `MsgFunction` variants alphabetically
2026-03-06 7:21 [PATCH v6 0/9] gpu: nova-core: gsp: add continuation record support Eliot Courtney
@ 2026-03-06 7:21 ` Eliot Courtney
2026-03-08 23:11 ` Claude review: " Claude Code Review Bot
2026-03-06 7:21 ` [PATCH v6 2/9] gpu: nova-core: gsp: add mechanism to wait for space on command queue Eliot Courtney
` (8 subsequent siblings)
9 siblings, 1 reply; 24+ messages in thread
From: Eliot Courtney @ 2026-03-06 7:21 UTC (permalink / raw)
To: Danilo Krummrich, Alice Ryhl, Alexandre Courbot, David Airlie,
Simona Vetter, Miguel Ojeda, Boqun Feng, Gary Guo,
Björn Roy Baron, Benno Lossin, Andreas Hindborg,
Trevor Gross
Cc: Zhi Wang, John Hubbard, Alistair Popple, Joel Fernandes,
Timur Tabi, nouveau, dri-devel, linux-kernel, rust-for-linux,
Eliot Courtney
There is no particular order required here and keeping them alphabetical
will help preventing future mistakes.
Tested-by: Zhi Wang <zhiw@nvidia.com>
Signed-off-by: Eliot Courtney <ecourtney@nvidia.com>
---
drivers/gpu/nova-core/gsp/fw.rs | 67 +++++++++++++++++++++--------------------
1 file changed, 35 insertions(+), 32 deletions(-)
diff --git a/drivers/gpu/nova-core/gsp/fw.rs b/drivers/gpu/nova-core/gsp/fw.rs
index f1797e1f0d9d..4b998485360b 100644
--- a/drivers/gpu/nova-core/gsp/fw.rs
+++ b/drivers/gpu/nova-core/gsp/fw.rs
@@ -191,34 +191,34 @@ pub(crate) fn new(gsp_firmware: &GspFirmware, fb_layout: &FbLayout) -> Self {
#[repr(u32)]
pub(crate) enum MsgFunction {
// Common function codes
- Nop = bindings::NV_VGPU_MSG_FUNCTION_NOP,
- SetGuestSystemInfo = bindings::NV_VGPU_MSG_FUNCTION_SET_GUEST_SYSTEM_INFO,
- AllocRoot = bindings::NV_VGPU_MSG_FUNCTION_ALLOC_ROOT,
+ AllocChannelDma = bindings::NV_VGPU_MSG_FUNCTION_ALLOC_CHANNEL_DMA,
+ AllocCtxDma = bindings::NV_VGPU_MSG_FUNCTION_ALLOC_CTX_DMA,
AllocDevice = bindings::NV_VGPU_MSG_FUNCTION_ALLOC_DEVICE,
AllocMemory = bindings::NV_VGPU_MSG_FUNCTION_ALLOC_MEMORY,
- AllocCtxDma = bindings::NV_VGPU_MSG_FUNCTION_ALLOC_CTX_DMA,
- AllocChannelDma = bindings::NV_VGPU_MSG_FUNCTION_ALLOC_CHANNEL_DMA,
- MapMemory = bindings::NV_VGPU_MSG_FUNCTION_MAP_MEMORY,
- BindCtxDma = bindings::NV_VGPU_MSG_FUNCTION_BIND_CTX_DMA,
AllocObject = bindings::NV_VGPU_MSG_FUNCTION_ALLOC_OBJECT,
+ AllocRoot = bindings::NV_VGPU_MSG_FUNCTION_ALLOC_ROOT,
+ BindCtxDma = bindings::NV_VGPU_MSG_FUNCTION_BIND_CTX_DMA,
Free = bindings::NV_VGPU_MSG_FUNCTION_FREE,
- Log = bindings::NV_VGPU_MSG_FUNCTION_LOG,
GetGspStaticInfo = bindings::NV_VGPU_MSG_FUNCTION_GET_GSP_STATIC_INFO,
- SetRegistry = bindings::NV_VGPU_MSG_FUNCTION_SET_REGISTRY,
- GspSetSystemInfo = bindings::NV_VGPU_MSG_FUNCTION_GSP_SET_SYSTEM_INFO,
+ GetStaticInfo = bindings::NV_VGPU_MSG_FUNCTION_GET_STATIC_INFO,
GspInitPostObjGpu = bindings::NV_VGPU_MSG_FUNCTION_GSP_INIT_POST_OBJGPU,
GspRmControl = bindings::NV_VGPU_MSG_FUNCTION_GSP_RM_CONTROL,
- GetStaticInfo = bindings::NV_VGPU_MSG_FUNCTION_GET_STATIC_INFO,
+ GspSetSystemInfo = bindings::NV_VGPU_MSG_FUNCTION_GSP_SET_SYSTEM_INFO,
+ Log = bindings::NV_VGPU_MSG_FUNCTION_LOG,
+ MapMemory = bindings::NV_VGPU_MSG_FUNCTION_MAP_MEMORY,
+ Nop = bindings::NV_VGPU_MSG_FUNCTION_NOP,
+ SetGuestSystemInfo = bindings::NV_VGPU_MSG_FUNCTION_SET_GUEST_SYSTEM_INFO,
+ SetRegistry = bindings::NV_VGPU_MSG_FUNCTION_SET_REGISTRY,
// Event codes
GspInitDone = bindings::NV_VGPU_MSG_EVENT_GSP_INIT_DONE,
+ GspLockdownNotice = bindings::NV_VGPU_MSG_EVENT_GSP_LOCKDOWN_NOTICE,
+ GspPostNoCat = bindings::NV_VGPU_MSG_EVENT_GSP_POST_NOCAT_RECORD,
GspRunCpuSequencer = bindings::NV_VGPU_MSG_EVENT_GSP_RUN_CPU_SEQUENCER,
- PostEvent = bindings::NV_VGPU_MSG_EVENT_POST_EVENT,
- RcTriggered = bindings::NV_VGPU_MSG_EVENT_RC_TRIGGERED,
MmuFaultQueued = bindings::NV_VGPU_MSG_EVENT_MMU_FAULT_QUEUED,
OsErrorLog = bindings::NV_VGPU_MSG_EVENT_OS_ERROR_LOG,
- GspPostNoCat = bindings::NV_VGPU_MSG_EVENT_GSP_POST_NOCAT_RECORD,
- GspLockdownNotice = bindings::NV_VGPU_MSG_EVENT_GSP_LOCKDOWN_NOTICE,
+ PostEvent = bindings::NV_VGPU_MSG_EVENT_POST_EVENT,
+ RcTriggered = bindings::NV_VGPU_MSG_EVENT_RC_TRIGGERED,
UcodeLibOsPrint = bindings::NV_VGPU_MSG_EVENT_UCODE_LIBOS_PRINT,
}
@@ -227,38 +227,41 @@ impl TryFrom<u32> for MsgFunction {
fn try_from(value: u32) -> Result<MsgFunction> {
match value {
- bindings::NV_VGPU_MSG_FUNCTION_NOP => Ok(MsgFunction::Nop),
- bindings::NV_VGPU_MSG_FUNCTION_SET_GUEST_SYSTEM_INFO => {
- Ok(MsgFunction::SetGuestSystemInfo)
- }
- bindings::NV_VGPU_MSG_FUNCTION_ALLOC_ROOT => Ok(MsgFunction::AllocRoot),
+ // Common function codes
+ bindings::NV_VGPU_MSG_FUNCTION_ALLOC_CHANNEL_DMA => Ok(MsgFunction::AllocChannelDma),
+ bindings::NV_VGPU_MSG_FUNCTION_ALLOC_CTX_DMA => Ok(MsgFunction::AllocCtxDma),
bindings::NV_VGPU_MSG_FUNCTION_ALLOC_DEVICE => Ok(MsgFunction::AllocDevice),
bindings::NV_VGPU_MSG_FUNCTION_ALLOC_MEMORY => Ok(MsgFunction::AllocMemory),
- bindings::NV_VGPU_MSG_FUNCTION_ALLOC_CTX_DMA => Ok(MsgFunction::AllocCtxDma),
- bindings::NV_VGPU_MSG_FUNCTION_ALLOC_CHANNEL_DMA => Ok(MsgFunction::AllocChannelDma),
- bindings::NV_VGPU_MSG_FUNCTION_MAP_MEMORY => Ok(MsgFunction::MapMemory),
- bindings::NV_VGPU_MSG_FUNCTION_BIND_CTX_DMA => Ok(MsgFunction::BindCtxDma),
bindings::NV_VGPU_MSG_FUNCTION_ALLOC_OBJECT => Ok(MsgFunction::AllocObject),
+ bindings::NV_VGPU_MSG_FUNCTION_ALLOC_ROOT => Ok(MsgFunction::AllocRoot),
+ bindings::NV_VGPU_MSG_FUNCTION_BIND_CTX_DMA => Ok(MsgFunction::BindCtxDma),
bindings::NV_VGPU_MSG_FUNCTION_FREE => Ok(MsgFunction::Free),
- bindings::NV_VGPU_MSG_FUNCTION_LOG => Ok(MsgFunction::Log),
bindings::NV_VGPU_MSG_FUNCTION_GET_GSP_STATIC_INFO => Ok(MsgFunction::GetGspStaticInfo),
- bindings::NV_VGPU_MSG_FUNCTION_SET_REGISTRY => Ok(MsgFunction::SetRegistry),
- bindings::NV_VGPU_MSG_FUNCTION_GSP_SET_SYSTEM_INFO => Ok(MsgFunction::GspSetSystemInfo),
+ bindings::NV_VGPU_MSG_FUNCTION_GET_STATIC_INFO => Ok(MsgFunction::GetStaticInfo),
bindings::NV_VGPU_MSG_FUNCTION_GSP_INIT_POST_OBJGPU => {
Ok(MsgFunction::GspInitPostObjGpu)
}
bindings::NV_VGPU_MSG_FUNCTION_GSP_RM_CONTROL => Ok(MsgFunction::GspRmControl),
- bindings::NV_VGPU_MSG_FUNCTION_GET_STATIC_INFO => Ok(MsgFunction::GetStaticInfo),
+ bindings::NV_VGPU_MSG_FUNCTION_GSP_SET_SYSTEM_INFO => Ok(MsgFunction::GspSetSystemInfo),
+ bindings::NV_VGPU_MSG_FUNCTION_LOG => Ok(MsgFunction::Log),
+ bindings::NV_VGPU_MSG_FUNCTION_MAP_MEMORY => Ok(MsgFunction::MapMemory),
+ bindings::NV_VGPU_MSG_FUNCTION_NOP => Ok(MsgFunction::Nop),
+ bindings::NV_VGPU_MSG_FUNCTION_SET_GUEST_SYSTEM_INFO => {
+ Ok(MsgFunction::SetGuestSystemInfo)
+ }
+ bindings::NV_VGPU_MSG_FUNCTION_SET_REGISTRY => Ok(MsgFunction::SetRegistry),
+
+ // Event codes
bindings::NV_VGPU_MSG_EVENT_GSP_INIT_DONE => Ok(MsgFunction::GspInitDone),
+ bindings::NV_VGPU_MSG_EVENT_GSP_LOCKDOWN_NOTICE => Ok(MsgFunction::GspLockdownNotice),
+ bindings::NV_VGPU_MSG_EVENT_GSP_POST_NOCAT_RECORD => Ok(MsgFunction::GspPostNoCat),
bindings::NV_VGPU_MSG_EVENT_GSP_RUN_CPU_SEQUENCER => {
Ok(MsgFunction::GspRunCpuSequencer)
}
- bindings::NV_VGPU_MSG_EVENT_POST_EVENT => Ok(MsgFunction::PostEvent),
- bindings::NV_VGPU_MSG_EVENT_RC_TRIGGERED => Ok(MsgFunction::RcTriggered),
bindings::NV_VGPU_MSG_EVENT_MMU_FAULT_QUEUED => Ok(MsgFunction::MmuFaultQueued),
bindings::NV_VGPU_MSG_EVENT_OS_ERROR_LOG => Ok(MsgFunction::OsErrorLog),
- bindings::NV_VGPU_MSG_EVENT_GSP_POST_NOCAT_RECORD => Ok(MsgFunction::GspPostNoCat),
- bindings::NV_VGPU_MSG_EVENT_GSP_LOCKDOWN_NOTICE => Ok(MsgFunction::GspLockdownNotice),
+ bindings::NV_VGPU_MSG_EVENT_POST_EVENT => Ok(MsgFunction::PostEvent),
+ bindings::NV_VGPU_MSG_EVENT_RC_TRIGGERED => Ok(MsgFunction::RcTriggered),
bindings::NV_VGPU_MSG_EVENT_UCODE_LIBOS_PRINT => Ok(MsgFunction::UcodeLibOsPrint),
_ => Err(EINVAL),
}
--
2.53.0
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH v6 2/9] gpu: nova-core: gsp: add mechanism to wait for space on command queue
2026-03-06 7:21 [PATCH v6 0/9] gpu: nova-core: gsp: add continuation record support Eliot Courtney
2026-03-06 7:21 ` [PATCH v6 1/9] gpu: nova-core: gsp: sort `MsgFunction` variants alphabetically Eliot Courtney
@ 2026-03-06 7:21 ` Eliot Courtney
2026-03-08 23:11 ` Claude review: " Claude Code Review Bot
2026-03-06 7:22 ` [PATCH v6 3/9] rust: add EMSGSIZE error code Eliot Courtney
` (7 subsequent siblings)
9 siblings, 1 reply; 24+ messages in thread
From: Eliot Courtney @ 2026-03-06 7:21 UTC (permalink / raw)
To: Danilo Krummrich, Alice Ryhl, Alexandre Courbot, David Airlie,
Simona Vetter, Miguel Ojeda, Boqun Feng, Gary Guo,
Björn Roy Baron, Benno Lossin, Andreas Hindborg,
Trevor Gross
Cc: Zhi Wang, John Hubbard, Alistair Popple, Joel Fernandes,
Timur Tabi, nouveau, dri-devel, linux-kernel, rust-for-linux,
Eliot Courtney
Add a timeout to `allocate_command` which waits for space on the GSP
command queue. It uses a similar timeout to nouveau.
This lets `send_command` wait for space to free up in the command queue.
This is required to support continuation records which can fill up the
queue.
Tested-by: Zhi Wang <zhiw@nvidia.com>
Signed-off-by: Eliot Courtney <ecourtney@nvidia.com>
---
drivers/gpu/nova-core/gsp/cmdq.rs | 42 ++++++++++++++++++++++++++++-----------
1 file changed, 30 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/nova-core/gsp/cmdq.rs b/drivers/gpu/nova-core/gsp/cmdq.rs
index 87dbbd6d1be9..12849bc057f2 100644
--- a/drivers/gpu/nova-core/gsp/cmdq.rs
+++ b/drivers/gpu/nova-core/gsp/cmdq.rs
@@ -250,6 +250,19 @@ fn new(dev: &device::Device<device::Bound>) -> Result<Self> {
}
}
+ /// Returns the size of the region of the CPU message queue that the driver is currently allowed
+ /// to write to, in bytes.
+ fn driver_write_area_size(&self) -> usize {
+ let tx = self.cpu_write_ptr();
+ let rx = self.gsp_read_ptr();
+
+ // `rx` and `tx` are both in `0..MSGQ_NUM_PAGES` per the invariants of `gsp_read_ptr` and
+ // `cpu_write_ptr`. The minimum value case is where `rx == 0` and `tx == MSGQ_NUM_PAGES -
+ // 1`, which gives `0 + MSGQ_NUM_PAGES - (MSGQ_NUM_PAGES - 1) - 1 == 0`.
+ let slots = (rx + MSGQ_NUM_PAGES - tx - 1) % MSGQ_NUM_PAGES;
+ num::u32_as_usize(slots) * GSP_PAGE_SIZE
+ }
+
/// Returns the region of the GSP message queue that the driver is currently allowed to read
/// from.
///
@@ -281,15 +294,22 @@ fn new(dev: &device::Device<device::Bound>) -> Result<Self> {
}
/// Allocates a region on the command queue that is large enough to send a command of `size`
- /// bytes.
+ /// bytes, waiting for space to become available based on the provided timeout.
///
/// This returns a [`GspCommand`] ready to be written to by the caller.
///
/// # Errors
///
- /// - `EAGAIN` if the driver area is too small to hold the requested command.
+ /// - `ETIMEDOUT` if space does not become available within the timeout.
/// - `EIO` if the command header is not properly aligned.
- fn allocate_command(&mut self, size: usize) -> Result<GspCommand<'_>> {
+ fn allocate_command(&mut self, size: usize, timeout: Delta) -> Result<GspCommand<'_>> {
+ read_poll_timeout(
+ || Ok(self.driver_write_area_size()),
+ |available_bytes| *available_bytes >= size_of::<GspMsgElement>() + size,
+ Delta::from_micros(1),
+ timeout,
+ )?;
+
// Get the current writable area as an array of bytes.
let (slice_1, slice_2) = {
let (slice_1, slice_2) = self.driver_write_area();
@@ -298,13 +318,6 @@ fn allocate_command(&mut self, size: usize) -> Result<GspCommand<'_>> {
(slice_1.as_flattened_mut(), slice_2.as_flattened_mut())
};
- // If the GSP is still processing previous messages the shared region
- // may be full in which case we will have to retry once the GSP has
- // processed the existing commands.
- if size_of::<GspMsgElement>() + size > slice_1.len() + slice_2.len() {
- return Err(EAGAIN);
- }
-
// Extract area for the `GspMsgElement`.
let (header, slice_1) = GspMsgElement::from_bytes_mut_prefix(slice_1).ok_or(EIO)?;
@@ -462,6 +475,9 @@ impl Cmdq {
/// Number of page table entries for the GSP shared region.
pub(crate) const NUM_PTES: usize = size_of::<GspMem>() >> GSP_PAGE_SHIFT;
+ /// Timeout for waiting for space on the command queue.
+ const ALLOCATE_TIMEOUT: Delta = Delta::from_secs(1);
+
/// Creates a new command queue for `dev`.
pub(crate) fn new(dev: &device::Device<device::Bound>) -> Result<Cmdq> {
let gsp_mem = DmaGspMem::new(dev)?;
@@ -497,7 +513,7 @@ fn notify_gsp(bar: &Bar0) {
///
/// # Errors
///
- /// - `EAGAIN` if there was not enough space in the command queue to send the command.
+ /// - `ETIMEDOUT` if space does not become available within the timeout.
/// - `EIO` if the variable payload requested by the command has not been entirely
/// written to by its [`CommandToGsp::init_variable_payload`] method.
///
@@ -509,7 +525,9 @@ pub(crate) fn send_command<M>(&mut self, bar: &Bar0, command: M) -> Result
Error: From<M::InitError>,
{
let command_size = size_of::<M::Command>() + command.variable_payload_len();
- let dst = self.gsp_mem.allocate_command(command_size)?;
+ let dst = self
+ .gsp_mem
+ .allocate_command(command_size, Self::ALLOCATE_TIMEOUT)?;
// Extract area for the command itself.
let (cmd, payload_1) = M::Command::from_bytes_mut_prefix(dst.contents.0).ok_or(EIO)?;
--
2.53.0
^ permalink raw reply related [flat|nested] 24+ messages in thread* Claude review: gpu: nova-core: gsp: add mechanism to wait for space on command queue
2026-03-06 7:21 ` [PATCH v6 2/9] gpu: nova-core: gsp: add mechanism to wait for space on command queue Eliot Courtney
@ 2026-03-08 23:11 ` Claude Code Review Bot
0 siblings, 0 replies; 24+ messages in thread
From: Claude Code Review Bot @ 2026-03-08 23:11 UTC (permalink / raw)
To: dri-devel-reviews
Patch Review
Replaces the immediate `EAGAIN` failure in `allocate_command` with a `read_poll_timeout` loop, adding a 1-second timeout. This is required because continuation records can fill up the queue and need the GSP to drain it.
The new `driver_write_area_size()` method computes the writable area size without materializing the slices, which is the right approach for a polling check.
The poll delay of `Delta::from_micros(1)` (changed from zero in v5 to 1us in v6) is reasonable — it avoids a busy spin while still being responsive.
The `ALLOCATE_TIMEOUT` of 1 second matches nouveau's behavior per the commit message.
One minor note: the existing `driver_write_area` method has essentially the same slot calculation logic. There's some duplication but it's justified since `driver_write_area` returns actual slices while `driver_write_area_size` just returns the size, and combining them would be awkward.
No issues.
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v6 3/9] rust: add EMSGSIZE error code
2026-03-06 7:21 [PATCH v6 0/9] gpu: nova-core: gsp: add continuation record support Eliot Courtney
2026-03-06 7:21 ` [PATCH v6 1/9] gpu: nova-core: gsp: sort `MsgFunction` variants alphabetically Eliot Courtney
2026-03-06 7:21 ` [PATCH v6 2/9] gpu: nova-core: gsp: add mechanism to wait for space on command queue Eliot Courtney
@ 2026-03-06 7:22 ` Eliot Courtney
2026-03-08 23:11 ` Claude review: " Claude Code Review Bot
2026-03-06 7:22 ` [PATCH v6 4/9] gpu: nova-core: gsp: add checking oversized commands Eliot Courtney
` (6 subsequent siblings)
9 siblings, 1 reply; 24+ messages in thread
From: Eliot Courtney @ 2026-03-06 7:22 UTC (permalink / raw)
To: Danilo Krummrich, Alice Ryhl, Alexandre Courbot, David Airlie,
Simona Vetter, Miguel Ojeda, Boqun Feng, Gary Guo,
Björn Roy Baron, Benno Lossin, Andreas Hindborg,
Trevor Gross
Cc: Zhi Wang, John Hubbard, Alistair Popple, Joel Fernandes,
Timur Tabi, nouveau, dri-devel, linux-kernel, rust-for-linux,
Eliot Courtney
Add the EMSGSIZE error code, which indicates that a message is too
long.
Tested-by: Zhi Wang <zhiw@nvidia.com>
Acked-by: Miguel Ojeda <ojeda@kernel.org>
Reviewed-by: Gary Guo <gary@garyguo.net>
Signed-off-by: Eliot Courtney <ecourtney@nvidia.com>
---
rust/kernel/error.rs | 1 +
1 file changed, 1 insertion(+)
diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs
index 258b12afdcba..10fcf1f0404d 100644
--- a/rust/kernel/error.rs
+++ b/rust/kernel/error.rs
@@ -67,6 +67,7 @@ macro_rules! declare_err {
declare_err!(EDOM, "Math argument out of domain of func.");
declare_err!(ERANGE, "Math result not representable.");
declare_err!(EOVERFLOW, "Value too large for defined data type.");
+ declare_err!(EMSGSIZE, "Message too long.");
declare_err!(ETIMEDOUT, "Connection timed out.");
declare_err!(ERESTARTSYS, "Restart the system call.");
declare_err!(ERESTARTNOINTR, "System call was interrupted by a signal and will be restarted.");
--
2.53.0
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH v6 4/9] gpu: nova-core: gsp: add checking oversized commands
2026-03-06 7:21 [PATCH v6 0/9] gpu: nova-core: gsp: add continuation record support Eliot Courtney
` (2 preceding siblings ...)
2026-03-06 7:22 ` [PATCH v6 3/9] rust: add EMSGSIZE error code Eliot Courtney
@ 2026-03-06 7:22 ` Eliot Courtney
2026-03-08 23:11 ` Claude review: " Claude Code Review Bot
2026-03-06 7:22 ` [PATCH v6 5/9] gpu: nova-core: gsp: clarify invariant on command queue Eliot Courtney
` (5 subsequent siblings)
9 siblings, 1 reply; 24+ messages in thread
From: Eliot Courtney @ 2026-03-06 7:22 UTC (permalink / raw)
To: Danilo Krummrich, Alice Ryhl, Alexandre Courbot, David Airlie,
Simona Vetter, Miguel Ojeda, Boqun Feng, Gary Guo,
Björn Roy Baron, Benno Lossin, Andreas Hindborg,
Trevor Gross
Cc: Zhi Wang, John Hubbard, Alistair Popple, Joel Fernandes,
Timur Tabi, nouveau, dri-devel, linux-kernel, rust-for-linux,
Eliot Courtney
The limit is 16 pages for a single command sent to the GSP. Return an
error if `allocate_command` is called with a too large size.
Tested-by: Zhi Wang <zhiw@nvidia.com>
Signed-off-by: Eliot Courtney <ecourtney@nvidia.com>
---
drivers/gpu/nova-core/gsp/cmdq.rs | 7 ++++++-
drivers/gpu/nova-core/gsp/fw.rs | 4 ++++
drivers/gpu/nova-core/gsp/fw/r570_144/bindings.rs | 1 +
3 files changed, 11 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/nova-core/gsp/cmdq.rs b/drivers/gpu/nova-core/gsp/cmdq.rs
index 12849bc057f2..8b970523d789 100644
--- a/drivers/gpu/nova-core/gsp/cmdq.rs
+++ b/drivers/gpu/nova-core/gsp/cmdq.rs
@@ -32,7 +32,8 @@
GspMsgElement,
MsgFunction,
MsgqRxHeader,
- MsgqTxHeader, //
+ MsgqTxHeader,
+ GSP_MSG_QUEUE_ELEMENT_SIZE_MAX, //
},
PteArray,
GSP_PAGE_SHIFT,
@@ -300,9 +301,13 @@ fn driver_write_area_size(&self) -> usize {
///
/// # Errors
///
+ /// - `EMSGSIZE` if the command is larger than [`GSP_MSG_QUEUE_ELEMENT_SIZE_MAX`].
/// - `ETIMEDOUT` if space does not become available within the timeout.
/// - `EIO` if the command header is not properly aligned.
fn allocate_command(&mut self, size: usize, timeout: Delta) -> Result<GspCommand<'_>> {
+ if size_of::<GspMsgElement>() + size > GSP_MSG_QUEUE_ELEMENT_SIZE_MAX {
+ return Err(EMSGSIZE);
+ }
read_poll_timeout(
|| Ok(self.driver_write_area_size()),
|available_bytes| *available_bytes >= size_of::<GspMsgElement>() + size,
diff --git a/drivers/gpu/nova-core/gsp/fw.rs b/drivers/gpu/nova-core/gsp/fw.rs
index 4b998485360b..6005362450cb 100644
--- a/drivers/gpu/nova-core/gsp/fw.rs
+++ b/drivers/gpu/nova-core/gsp/fw.rs
@@ -39,6 +39,10 @@
},
};
+/// Maximum size of a single GSP message queue element in bytes.
+pub(crate) const GSP_MSG_QUEUE_ELEMENT_SIZE_MAX: usize =
+ num::u32_as_usize(bindings::GSP_MSG_QUEUE_ELEMENT_SIZE_MAX);
+
/// Empty type to group methods related to heap parameters for running the GSP firmware.
enum GspFwHeapParams {}
diff --git a/drivers/gpu/nova-core/gsp/fw/r570_144/bindings.rs b/drivers/gpu/nova-core/gsp/fw/r570_144/bindings.rs
index 6d25fe0bffa9..334e8be5fde8 100644
--- a/drivers/gpu/nova-core/gsp/fw/r570_144/bindings.rs
+++ b/drivers/gpu/nova-core/gsp/fw/r570_144/bindings.rs
@@ -43,6 +43,7 @@ fn fmt(&self, fmt: &mut ::core::fmt::Formatter<'_>) -> ::core::fmt::Result {
pub const GSP_FW_WPR_META_REVISION: u32 = 1;
pub const GSP_FW_WPR_META_MAGIC: i64 = -2577556379034558285;
pub const REGISTRY_TABLE_ENTRY_TYPE_DWORD: u32 = 1;
+pub const GSP_MSG_QUEUE_ELEMENT_SIZE_MAX: u32 = 65536;
pub type __u8 = ffi::c_uchar;
pub type __u16 = ffi::c_ushort;
pub type __u32 = ffi::c_uint;
--
2.53.0
^ permalink raw reply related [flat|nested] 24+ messages in thread* Claude review: gpu: nova-core: gsp: add checking oversized commands
2026-03-06 7:22 ` [PATCH v6 4/9] gpu: nova-core: gsp: add checking oversized commands Eliot Courtney
@ 2026-03-08 23:11 ` Claude Code Review Bot
0 siblings, 0 replies; 24+ messages in thread
From: Claude Code Review Bot @ 2026-03-08 23:11 UTC (permalink / raw)
To: dri-devel-reviews
Patch Review
Adds a size check at the top of `allocate_command` to reject commands larger than `GSP_MSG_QUEUE_ELEMENT_SIZE_MAX` (65536 bytes) with `EMSGSIZE`. Also adds the `GSP_MSG_QUEUE_ELEMENT_SIZE_MAX` constant from bindings.
This is a defensive check — after patch 8, `send_command` will split large commands so they never hit this check, but it protects `send_single_command` (and documents the constraint) clearly.
No issues.
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v6 5/9] gpu: nova-core: gsp: clarify invariant on command queue
2026-03-06 7:21 [PATCH v6 0/9] gpu: nova-core: gsp: add continuation record support Eliot Courtney
` (3 preceding siblings ...)
2026-03-06 7:22 ` [PATCH v6 4/9] gpu: nova-core: gsp: add checking oversized commands Eliot Courtney
@ 2026-03-06 7:22 ` Eliot Courtney
2026-03-08 23:11 ` Claude review: " Claude Code Review Bot
2026-03-06 7:22 ` [PATCH v6 6/9] gpu: nova-core: gsp: unconditionally call variable payload handling Eliot Courtney
` (4 subsequent siblings)
9 siblings, 1 reply; 24+ messages in thread
From: Eliot Courtney @ 2026-03-06 7:22 UTC (permalink / raw)
To: Danilo Krummrich, Alice Ryhl, Alexandre Courbot, David Airlie,
Simona Vetter, Miguel Ojeda, Boqun Feng, Gary Guo,
Björn Roy Baron, Benno Lossin, Andreas Hindborg,
Trevor Gross
Cc: Zhi Wang, John Hubbard, Alistair Popple, Joel Fernandes,
Timur Tabi, nouveau, dri-devel, linux-kernel, rust-for-linux,
Eliot Courtney
Clarify why using only the first returned slice from allocate_command
for the message headers is okay.
Tested-by: Zhi Wang <zhiw@nvidia.com>
Signed-off-by: Eliot Courtney <ecourtney@nvidia.com>
---
drivers/gpu/nova-core/gsp/cmdq.rs | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/nova-core/gsp/cmdq.rs b/drivers/gpu/nova-core/gsp/cmdq.rs
index 8b970523d789..806b1e02715e 100644
--- a/drivers/gpu/nova-core/gsp/cmdq.rs
+++ b/drivers/gpu/nova-core/gsp/cmdq.rs
@@ -534,7 +534,9 @@ pub(crate) fn send_command<M>(&mut self, bar: &Bar0, command: M) -> Result
.gsp_mem
.allocate_command(command_size, Self::ALLOCATE_TIMEOUT)?;
- // Extract area for the command itself.
+ // Extract area for the command itself. The GSP message header and the command header
+ // together are guaranteed to fit entirely into a single page, so it's ok to only look
+ // at `dst.contents.0` here.
let (cmd, payload_1) = M::Command::from_bytes_mut_prefix(dst.contents.0).ok_or(EIO)?;
// Fill the header and command in-place.
--
2.53.0
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH v6 6/9] gpu: nova-core: gsp: unconditionally call variable payload handling
2026-03-06 7:21 [PATCH v6 0/9] gpu: nova-core: gsp: add continuation record support Eliot Courtney
` (4 preceding siblings ...)
2026-03-06 7:22 ` [PATCH v6 5/9] gpu: nova-core: gsp: clarify invariant on command queue Eliot Courtney
@ 2026-03-06 7:22 ` Eliot Courtney
2026-03-08 23:11 ` Claude review: " Claude Code Review Bot
2026-03-06 7:22 ` [PATCH v6 7/9] gpu: nova-core: gsp: add `size` helper to `CommandToGsp` Eliot Courtney
` (3 subsequent siblings)
9 siblings, 1 reply; 24+ messages in thread
From: Eliot Courtney @ 2026-03-06 7:22 UTC (permalink / raw)
To: Danilo Krummrich, Alice Ryhl, Alexandre Courbot, David Airlie,
Simona Vetter, Miguel Ojeda, Boqun Feng, Gary Guo,
Björn Roy Baron, Benno Lossin, Andreas Hindborg,
Trevor Gross
Cc: Zhi Wang, John Hubbard, Alistair Popple, Joel Fernandes,
Timur Tabi, nouveau, dri-devel, linux-kernel, rust-for-linux,
Eliot Courtney
Unconditionally call the variable length payload code, which is a no-op
if there is no such payload but could defensively catch some coding
errors by e.g. checking that the allocated size is completely filled.
Tested-by: Zhi Wang <zhiw@nvidia.com>
Signed-off-by: Eliot Courtney <ecourtney@nvidia.com>
---
drivers/gpu/nova-core/gsp/cmdq.rs | 14 ++++++--------
1 file changed, 6 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/nova-core/gsp/cmdq.rs b/drivers/gpu/nova-core/gsp/cmdq.rs
index 806b1e02715e..b41a866e24da 100644
--- a/drivers/gpu/nova-core/gsp/cmdq.rs
+++ b/drivers/gpu/nova-core/gsp/cmdq.rs
@@ -548,16 +548,14 @@ pub(crate) fn send_command<M>(&mut self, bar: &Bar0, command: M) -> Result
command.init().__init(core::ptr::from_mut(cmd))?;
}
- // Fill the variable-length payload.
- if command_size > size_of::<M::Command>() {
- let mut sbuffer =
- SBufferIter::new_writer([&mut payload_1[..], &mut dst.contents.1[..]]);
- command.init_variable_payload(&mut sbuffer)?;
+ // Fill the variable-length payload, which may be empty.
+ let mut sbuffer = SBufferIter::new_writer([&mut payload_1[..], &mut dst.contents.1[..]]);
+ command.init_variable_payload(&mut sbuffer)?;
- if !sbuffer.is_empty() {
- return Err(EIO);
- }
+ if !sbuffer.is_empty() {
+ return Err(EIO);
}
+ drop(sbuffer);
// Compute checksum now that the whole message is ready.
dst.header
--
2.53.0
^ permalink raw reply related [flat|nested] 24+ messages in thread* Claude review: gpu: nova-core: gsp: unconditionally call variable payload handling
2026-03-06 7:22 ` [PATCH v6 6/9] gpu: nova-core: gsp: unconditionally call variable payload handling Eliot Courtney
@ 2026-03-08 23:11 ` Claude Code Review Bot
0 siblings, 0 replies; 24+ messages in thread
From: Claude Code Review Bot @ 2026-03-08 23:11 UTC (permalink / raw)
To: dri-devel-reviews
Patch Review
Moves the `SBufferIter::new_writer` and `sbuffer.is_empty()` check outside the `if command_size > size_of::<M::Command>()` conditional. This means the variable payload path runs even when `variable_payload_len() == 0`, making the code defensively catch bugs where a command incorrectly reports its payload size.
The addition of `drop(sbuffer)` explicitly ends the mutable borrow before computing the checksum, which is necessary since the `if` block previously scoped the borrow.
No issues.
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v6 7/9] gpu: nova-core: gsp: add `size` helper to `CommandToGsp`
2026-03-06 7:21 [PATCH v6 0/9] gpu: nova-core: gsp: add continuation record support Eliot Courtney
` (5 preceding siblings ...)
2026-03-06 7:22 ` [PATCH v6 6/9] gpu: nova-core: gsp: unconditionally call variable payload handling Eliot Courtney
@ 2026-03-06 7:22 ` Eliot Courtney
2026-03-08 23:11 ` Claude review: " Claude Code Review Bot
2026-03-06 7:22 ` [PATCH v6 8/9] gpu: nova-core: gsp: support large RPCs via continuation record Eliot Courtney
` (2 subsequent siblings)
9 siblings, 1 reply; 24+ messages in thread
From: Eliot Courtney @ 2026-03-06 7:22 UTC (permalink / raw)
To: Danilo Krummrich, Alice Ryhl, Alexandre Courbot, David Airlie,
Simona Vetter, Miguel Ojeda, Boqun Feng, Gary Guo,
Björn Roy Baron, Benno Lossin, Andreas Hindborg,
Trevor Gross
Cc: Zhi Wang, John Hubbard, Alistair Popple, Joel Fernandes,
Timur Tabi, nouveau, dri-devel, linux-kernel, rust-for-linux,
Eliot Courtney
Add a default method to `CommandToGsp` which computes the size of a
command.
Tested-by: Zhi Wang <zhiw@nvidia.com>
Signed-off-by: Eliot Courtney <ecourtney@nvidia.com>
---
drivers/gpu/nova-core/gsp/cmdq.rs | 12 +++++++++---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/nova-core/gsp/cmdq.rs b/drivers/gpu/nova-core/gsp/cmdq.rs
index b41a866e24da..861f5666fe7f 100644
--- a/drivers/gpu/nova-core/gsp/cmdq.rs
+++ b/drivers/gpu/nova-core/gsp/cmdq.rs
@@ -94,6 +94,12 @@ fn init_variable_payload(
) -> Result {
Ok(())
}
+
+ /// Total size of the command (including its variable-length payload) without the
+ /// [`GspMsgElement`] header.
+ fn size(&self) -> usize {
+ size_of::<Self::Command>() + self.variable_payload_len()
+ }
}
/// Trait representing messages received from the GSP.
@@ -529,10 +535,10 @@ pub(crate) fn send_command<M>(&mut self, bar: &Bar0, command: M) -> Result
// This allows all error types, including `Infallible`, to be used for `M::InitError`.
Error: From<M::InitError>,
{
- let command_size = size_of::<M::Command>() + command.variable_payload_len();
+ let size_in_bytes = command.size();
let dst = self
.gsp_mem
- .allocate_command(command_size, Self::ALLOCATE_TIMEOUT)?;
+ .allocate_command(size_in_bytes, Self::ALLOCATE_TIMEOUT)?;
// Extract area for the command itself. The GSP message header and the command header
// together are guaranteed to fit entirely into a single page, so it's ok to only look
@@ -540,7 +546,7 @@ pub(crate) fn send_command<M>(&mut self, bar: &Bar0, command: M) -> Result
let (cmd, payload_1) = M::Command::from_bytes_mut_prefix(dst.contents.0).ok_or(EIO)?;
// Fill the header and command in-place.
- let msg_element = GspMsgElement::init(self.seq, command_size, M::FUNCTION);
+ let msg_element = GspMsgElement::init(self.seq, size_in_bytes, M::FUNCTION);
// SAFETY: `msg_header` and `cmd` are valid references, and not touched if the initializer
// fails.
unsafe {
--
2.53.0
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH v6 8/9] gpu: nova-core: gsp: support large RPCs via continuation record
2026-03-06 7:21 [PATCH v6 0/9] gpu: nova-core: gsp: add continuation record support Eliot Courtney
` (6 preceding siblings ...)
2026-03-06 7:22 ` [PATCH v6 7/9] gpu: nova-core: gsp: add `size` helper to `CommandToGsp` Eliot Courtney
@ 2026-03-06 7:22 ` Eliot Courtney
2026-03-08 23:11 ` Claude review: " Claude Code Review Bot
2026-03-06 7:22 ` [PATCH v6 9/9] gpu: nova-core: gsp: add tests for continuation records Eliot Courtney
2026-03-08 23:11 ` Claude review: gpu: nova-core: gsp: add continuation record support Claude Code Review Bot
9 siblings, 1 reply; 24+ messages in thread
From: Eliot Courtney @ 2026-03-06 7:22 UTC (permalink / raw)
To: Danilo Krummrich, Alice Ryhl, Alexandre Courbot, David Airlie,
Simona Vetter, Miguel Ojeda, Boqun Feng, Gary Guo,
Björn Roy Baron, Benno Lossin, Andreas Hindborg,
Trevor Gross
Cc: Zhi Wang, John Hubbard, Alistair Popple, Joel Fernandes,
Timur Tabi, nouveau, dri-devel, linux-kernel, rust-for-linux,
Eliot Courtney
Splits large RPCs if necessary and sends the remaining parts using
continuation records. RPCs that do not need continuation records
continue to write directly into the command buffer. Ones that do write
into a staging buffer first, so there is one copy.
Continuation record for receive is not necessary to support at the
moment because those replies do not need to be read and are currently
drained by retrying `receive_msg` on `ERANGE`.
Signed-off-by: Eliot Courtney <ecourtney@nvidia.com>
---
drivers/gpu/nova-core/gsp/cmdq.rs | 42 ++++++-
drivers/gpu/nova-core/gsp/cmdq/continuation.rs | 163 +++++++++++++++++++++++++
drivers/gpu/nova-core/gsp/fw.rs | 4 +
3 files changed, 207 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/nova-core/gsp/cmdq.rs b/drivers/gpu/nova-core/gsp/cmdq.rs
index 861f5666fe7f..e0b096546d23 100644
--- a/drivers/gpu/nova-core/gsp/cmdq.rs
+++ b/drivers/gpu/nova-core/gsp/cmdq.rs
@@ -1,5 +1,7 @@
// SPDX-License-Identifier: GPL-2.0
+mod continuation;
+
use core::{
mem,
sync::atomic::{
@@ -25,6 +27,11 @@
},
};
+use continuation::{
+ ContinuationRecord,
+ SplitState, //
+};
+
use crate::{
driver::Bar0,
gsp::{
@@ -520,7 +527,7 @@ fn notify_gsp(bar: &Bar0) {
.write(bar);
}
- /// Sends `command` to the GSP.
+ /// Sends `command` to the GSP, without splitting it.
///
/// # Errors
///
@@ -529,7 +536,7 @@ fn notify_gsp(bar: &Bar0) {
/// written to by its [`CommandToGsp::init_variable_payload`] method.
///
/// Error codes returned by the command initializers are propagated as-is.
- pub(crate) fn send_command<M>(&mut self, bar: &Bar0, command: M) -> Result
+ fn send_single_command<M>(&mut self, bar: &Bar0, command: M) -> Result
where
M: CommandToGsp,
// This allows all error types, including `Infallible`, to be used for `M::InitError`.
@@ -588,6 +595,37 @@ pub(crate) fn send_command<M>(&mut self, bar: &Bar0, command: M) -> Result
Ok(())
}
+ /// Sends `command` to the GSP.
+ ///
+ /// The command may be split into multiple messages if it is large.
+ ///
+ /// # Errors
+ ///
+ /// - `ETIMEDOUT` if space does not become available within the timeout.
+ /// - `EIO` if the variable payload requested by the command has not been entirely
+ /// written to by its [`CommandToGsp::init_variable_payload`] method.
+ ///
+ /// Error codes returned by the command initializers are propagated as-is.
+ pub(crate) fn send_command<M>(&mut self, bar: &Bar0, command: M) -> Result
+ where
+ M: CommandToGsp,
+ Error: From<M::InitError>,
+ {
+ match SplitState::new(command)? {
+ SplitState::Single(command) => self.send_single_command(bar, command),
+ SplitState::Split(command, mut continuations) => {
+ self.send_single_command(bar, command)?;
+
+ while let Some(continuation) = continuations.next() {
+ // Turbofish needed because the compiler cannot infer M here.
+ self.send_single_command::<ContinuationRecord<'_>>(bar, continuation)?;
+ }
+
+ Ok(())
+ }
+ }
+ }
+
/// Wait for a message to become available on the message queue.
///
/// This works purely at the transport layer and does not interpret or validate the message
diff --git a/drivers/gpu/nova-core/gsp/cmdq/continuation.rs b/drivers/gpu/nova-core/gsp/cmdq/continuation.rs
new file mode 100644
index 000000000000..67b3e03fd8ea
--- /dev/null
+++ b/drivers/gpu/nova-core/gsp/cmdq/continuation.rs
@@ -0,0 +1,163 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Support for splitting large GSP commands across continuation records.
+
+use core::convert::Infallible;
+
+use kernel::prelude::*;
+
+use super::CommandToGsp;
+
+use crate::{
+ gsp::fw::{
+ GspMsgElement,
+ MsgFunction,
+ GSP_MSG_QUEUE_ELEMENT_SIZE_MAX, //
+ },
+ sbuffer::SBufferIter,
+};
+
+/// Maximum command size that fits in a single queue element.
+const MAX_CMD_SIZE: usize = GSP_MSG_QUEUE_ELEMENT_SIZE_MAX - size_of::<GspMsgElement>();
+
+/// Acts as an iterator over the continuation records for a split command.
+pub(super) struct ContinuationRecords {
+ payload: KVVec<u8>,
+ offset: usize,
+}
+
+impl ContinuationRecords {
+ /// Creates a new iterator over continuation records for the given payload.
+ fn new(payload: KVVec<u8>) -> Self {
+ Self { payload, offset: 0 }
+ }
+
+ /// Returns the next continuation record, or [`None`] if there are no more.
+ pub(super) fn next(&mut self) -> Option<ContinuationRecord<'_>> {
+ let remaining = self.payload.len() - self.offset;
+
+ if remaining > 0 {
+ let chunk_size = remaining.min(MAX_CMD_SIZE);
+ let record =
+ ContinuationRecord::new(&self.payload[self.offset..(self.offset + chunk_size)]);
+ self.offset += chunk_size;
+ Some(record)
+ } else {
+ None
+ }
+ }
+}
+
+/// The [`ContinuationRecord`] command.
+pub(super) struct ContinuationRecord<'a> {
+ data: &'a [u8],
+}
+
+impl<'a> ContinuationRecord<'a> {
+ /// Creates a new [`ContinuationRecord`] command with the given data.
+ fn new(data: &'a [u8]) -> Self {
+ Self { data }
+ }
+}
+
+impl<'a> CommandToGsp for ContinuationRecord<'a> {
+ const FUNCTION: MsgFunction = MsgFunction::ContinuationRecord;
+ type Command = ();
+ type InitError = Infallible;
+
+ fn init(&self) -> impl Init<Self::Command, Self::InitError> {
+ <()>::init_zeroed()
+ }
+
+ fn variable_payload_len(&self) -> usize {
+ self.data.len()
+ }
+
+ fn init_variable_payload(
+ &self,
+ dst: &mut SBufferIter<core::array::IntoIter<&mut [u8], 2>>,
+ ) -> Result {
+ dst.write_all(self.data)
+ }
+}
+
+/// Whether a command needs to be split across continuation records or not.
+pub(super) enum SplitState<C: CommandToGsp> {
+ /// A command that fits in a single queue element.
+ Single(C),
+ /// A command split across continuation records.
+ Split(SplitCommand<C>, ContinuationRecords),
+}
+
+impl<C: CommandToGsp> SplitState<C> {
+ /// Maximum variable payload size that fits in the first command alongside the command header.
+ const MAX_FIRST_PAYLOAD: usize = MAX_CMD_SIZE - size_of::<C::Command>();
+
+ /// Creates a new [`SplitState`] for the given command.
+ ///
+ /// If the command is too large, it will be split into a main command and some number of
+ /// continuation records.
+ pub(super) fn new(command: C) -> Result<Self> {
+ let payload_len = command.variable_payload_len();
+
+ if command.size() > MAX_CMD_SIZE {
+ let mut command_payload =
+ KVVec::<u8>::from_elem(0u8, payload_len.min(Self::MAX_FIRST_PAYLOAD), GFP_KERNEL)?;
+ let mut continuation_payload =
+ KVVec::<u8>::from_elem(0u8, payload_len - command_payload.len(), GFP_KERNEL)?;
+ let mut sbuffer = SBufferIter::new_writer([
+ command_payload.as_mut_slice(),
+ continuation_payload.as_mut_slice(),
+ ]);
+
+ command.init_variable_payload(&mut sbuffer)?;
+ if !sbuffer.is_empty() {
+ return Err(EIO);
+ }
+ drop(sbuffer);
+
+ Ok(Self::Split(
+ SplitCommand::new(command, command_payload),
+ ContinuationRecords::new(continuation_payload),
+ ))
+ } else {
+ Ok(Self::Single(command))
+ }
+ }
+}
+
+/// A command that has been truncated to maximum accepted length of the command queue.
+///
+/// The remainder of its payload is expected to be sent using [`ContinuationRecords`].
+pub(super) struct SplitCommand<C: CommandToGsp> {
+ command: C,
+ payload: KVVec<u8>,
+}
+
+impl<C: CommandToGsp> SplitCommand<C> {
+ /// Creates a new [`SplitCommand`] wrapping `command` with the given truncated payload.
+ fn new(command: C, payload: KVVec<u8>) -> Self {
+ Self { command, payload }
+ }
+}
+
+impl<C: CommandToGsp> CommandToGsp for SplitCommand<C> {
+ const FUNCTION: MsgFunction = C::FUNCTION;
+ type Command = C::Command;
+ type InitError = C::InitError;
+
+ fn init(&self) -> impl Init<Self::Command, Self::InitError> {
+ self.command.init()
+ }
+
+ fn variable_payload_len(&self) -> usize {
+ self.payload.len()
+ }
+
+ fn init_variable_payload(
+ &self,
+ dst: &mut SBufferIter<core::array::IntoIter<&mut [u8], 2>>,
+ ) -> Result {
+ dst.write_all(&self.payload)
+ }
+}
diff --git a/drivers/gpu/nova-core/gsp/fw.rs b/drivers/gpu/nova-core/gsp/fw.rs
index 6005362450cb..25fca1f6db2c 100644
--- a/drivers/gpu/nova-core/gsp/fw.rs
+++ b/drivers/gpu/nova-core/gsp/fw.rs
@@ -202,6 +202,7 @@ pub(crate) enum MsgFunction {
AllocObject = bindings::NV_VGPU_MSG_FUNCTION_ALLOC_OBJECT,
AllocRoot = bindings::NV_VGPU_MSG_FUNCTION_ALLOC_ROOT,
BindCtxDma = bindings::NV_VGPU_MSG_FUNCTION_BIND_CTX_DMA,
+ ContinuationRecord = bindings::NV_VGPU_MSG_FUNCTION_CONTINUATION_RECORD,
Free = bindings::NV_VGPU_MSG_FUNCTION_FREE,
GetGspStaticInfo = bindings::NV_VGPU_MSG_FUNCTION_GET_GSP_STATIC_INFO,
GetStaticInfo = bindings::NV_VGPU_MSG_FUNCTION_GET_STATIC_INFO,
@@ -239,6 +240,9 @@ fn try_from(value: u32) -> Result<MsgFunction> {
bindings::NV_VGPU_MSG_FUNCTION_ALLOC_OBJECT => Ok(MsgFunction::AllocObject),
bindings::NV_VGPU_MSG_FUNCTION_ALLOC_ROOT => Ok(MsgFunction::AllocRoot),
bindings::NV_VGPU_MSG_FUNCTION_BIND_CTX_DMA => Ok(MsgFunction::BindCtxDma),
+ bindings::NV_VGPU_MSG_FUNCTION_CONTINUATION_RECORD => {
+ Ok(MsgFunction::ContinuationRecord)
+ }
bindings::NV_VGPU_MSG_FUNCTION_FREE => Ok(MsgFunction::Free),
bindings::NV_VGPU_MSG_FUNCTION_GET_GSP_STATIC_INFO => Ok(MsgFunction::GetGspStaticInfo),
bindings::NV_VGPU_MSG_FUNCTION_GET_STATIC_INFO => Ok(MsgFunction::GetStaticInfo),
--
2.53.0
^ permalink raw reply related [flat|nested] 24+ messages in thread* Claude review: gpu: nova-core: gsp: support large RPCs via continuation record
2026-03-06 7:22 ` [PATCH v6 8/9] gpu: nova-core: gsp: support large RPCs via continuation record Eliot Courtney
@ 2026-03-08 23:11 ` Claude Code Review Bot
0 siblings, 0 replies; 24+ messages in thread
From: Claude Code Review Bot @ 2026-03-08 23:11 UTC (permalink / raw)
To: dri-devel-reviews
Patch Review
This is the core patch. Key design:
- `send_command` now wraps `send_single_command` via `SplitState::new(command)`.
- `SplitState::Single(command)` passes through to `send_single_command` directly — no extra allocation or copy for normal-sized commands.
- `SplitState::Split(command, continuations)` stages the variable payload into a `KVVec<u8>`, splits it at `MAX_FIRST_PAYLOAD`, and sends the remainder via `ContinuationRecord` commands.
- `ContinuationRecord` uses `type Command = ()` (zero-sized), so the entire queue element is payload.
**Observations:**
1. The `ContinuationRecords` struct has a `next()` method but doesn't implement the `Iterator` trait. The comment at line 1337 says `while let Some(continuation) = continuations.next()` with a note about turbofish being needed. Not implementing `Iterator` is likely intentional to avoid the trait's `Item` type requiring a lifetime, but a brief comment on why would help.
2. In `SplitState::new`:
```rust
let mut command_payload =
KVVec::<u8>::from_elem(0u8, payload_len.min(Self::MAX_FIRST_PAYLOAD), GFP_KERNEL)?;
let mut continuation_payload =
KVVec::<u8>::from_elem(0u8, payload_len - command_payload.len(), GFP_KERNEL)?;
```
This allocates `command_payload` of size `min(payload_len, MAX_FIRST_PAYLOAD)` and `continuation_payload` for the rest. The buffers are initialized to zero then overwritten by `init_variable_payload` — the double-write is acceptable as this is the oversized path.
3. `MsgFunction::ContinuationRecord` is added to the enum and `TryFrom`, maintaining alphabetical order established in patch 1. The binding `NV_VGPU_MSG_FUNCTION_CONTINUATION_RECORD` already exists in the tree (confirmed value 71).
4. The cover letter mentions "Continuation record for receive is not necessary to support at the moment" — this is documented inline as well, which is good.
No blocking issues. This looks correct.
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v6 9/9] gpu: nova-core: gsp: add tests for continuation records
2026-03-06 7:21 [PATCH v6 0/9] gpu: nova-core: gsp: add continuation record support Eliot Courtney
` (7 preceding siblings ...)
2026-03-06 7:22 ` [PATCH v6 8/9] gpu: nova-core: gsp: support large RPCs via continuation record Eliot Courtney
@ 2026-03-06 7:22 ` Eliot Courtney
2026-03-08 23:11 ` Claude review: " Claude Code Review Bot
2026-03-08 23:11 ` Claude review: gpu: nova-core: gsp: add continuation record support Claude Code Review Bot
9 siblings, 1 reply; 24+ messages in thread
From: Eliot Courtney @ 2026-03-06 7:22 UTC (permalink / raw)
To: Danilo Krummrich, Alice Ryhl, Alexandre Courbot, David Airlie,
Simona Vetter, Miguel Ojeda, Boqun Feng, Gary Guo,
Björn Roy Baron, Benno Lossin, Andreas Hindborg,
Trevor Gross
Cc: Zhi Wang, John Hubbard, Alistair Popple, Joel Fernandes,
Timur Tabi, nouveau, dri-devel, linux-kernel, rust-for-linux,
Eliot Courtney
Add tests for continuation record splitting. They cover boundary
conditions at the split points to make sure the right number of
continuation records are made. They also check that the data
concatenated is correct.
Tested-by: Zhi Wang <zhiw@nvidia.com>
Signed-off-by: Eliot Courtney <ecourtney@nvidia.com>
---
drivers/gpu/nova-core/gsp/cmdq/continuation.rs | 138 +++++++++++++++++++++++++
1 file changed, 138 insertions(+)
diff --git a/drivers/gpu/nova-core/gsp/cmdq/continuation.rs b/drivers/gpu/nova-core/gsp/cmdq/continuation.rs
index 67b3e03fd8ea..2aa17caac2e0 100644
--- a/drivers/gpu/nova-core/gsp/cmdq/continuation.rs
+++ b/drivers/gpu/nova-core/gsp/cmdq/continuation.rs
@@ -161,3 +161,141 @@ fn init_variable_payload(
dst.write_all(&self.payload)
}
}
+
+#[kunit_tests(nova_core_gsp_continuation)]
+mod tests {
+ use super::*;
+
+ use kernel::transmute::{
+ AsBytes,
+ FromBytes, //
+ };
+
+ /// Non-zero-sized command header for testing.
+ #[repr(C)]
+ #[derive(Clone, Copy, Zeroable)]
+ struct TestHeader([u8; 64]);
+
+ // SAFETY: `TestHeader` is a plain array of bytes for which all bit patterns are valid.
+ unsafe impl FromBytes for TestHeader {}
+
+ // SAFETY: `TestHeader` is a plain array of bytes for which all bit patterns are valid.
+ unsafe impl AsBytes for TestHeader {}
+
+ struct TestPayload {
+ data: KVVec<u8>,
+ }
+
+ impl TestPayload {
+ fn generate_pattern(len: usize) -> Result<KVVec<u8>> {
+ let mut data = KVVec::with_capacity(len, GFP_KERNEL)?;
+ for i in 0..len {
+ // Mix in higher bits so the pattern does not repeat every 256 bytes.
+ data.push((i ^ (i >> 8)) as u8, GFP_KERNEL)?;
+ }
+ Ok(data)
+ }
+
+ fn new(len: usize) -> Result<Self> {
+ Ok(Self {
+ data: Self::generate_pattern(len)?,
+ })
+ }
+ }
+
+ impl CommandToGsp for TestPayload {
+ const FUNCTION: MsgFunction = MsgFunction::Nop;
+ type Command = TestHeader;
+ type InitError = Infallible;
+
+ fn init(&self) -> impl Init<Self::Command, Self::InitError> {
+ TestHeader::init_zeroed()
+ }
+
+ fn variable_payload_len(&self) -> usize {
+ self.data.len()
+ }
+
+ fn init_variable_payload(
+ &self,
+ dst: &mut SBufferIter<core::array::IntoIter<&mut [u8], 2>>,
+ ) -> Result {
+ dst.write_all(self.data.as_slice())
+ }
+ }
+
+ /// Maximum variable payload size that fits in the first command alongside the header.
+ const MAX_FIRST_PAYLOAD: usize = SplitState::<TestPayload>::MAX_FIRST_PAYLOAD;
+
+ fn read_payload(cmd: impl CommandToGsp) -> Result<KVVec<u8>> {
+ let len = cmd.variable_payload_len();
+ let mut buf = KVVec::from_elem(0u8, len, GFP_KERNEL)?;
+ let mut sbuf = SBufferIter::new_writer([buf.as_mut_slice(), &mut []]);
+ cmd.init_variable_payload(&mut sbuf)?;
+ drop(sbuf);
+ Ok(buf)
+ }
+
+ struct SplitTest {
+ payload_size: usize,
+ num_continuations: usize,
+ }
+
+ fn check_split(t: SplitTest) -> Result {
+ let payload = TestPayload::new(t.payload_size)?;
+ let mut num_continuations = 0;
+
+ let buf = match SplitState::new(payload)? {
+ SplitState::Single(cmd) => read_payload(cmd)?,
+ SplitState::Split(cmd, mut continuations) => {
+ let mut buf = read_payload(cmd)?;
+ assert!(size_of::<TestHeader>() + buf.len() <= MAX_CMD_SIZE);
+
+ while let Some(cont) = continuations.next() {
+ let payload = read_payload(cont)?;
+ assert!(payload.len() <= MAX_CMD_SIZE);
+ buf.extend_from_slice(&payload, GFP_KERNEL)?;
+ num_continuations += 1;
+ }
+
+ buf
+ }
+ };
+
+ assert_eq!(num_continuations, t.num_continuations);
+ assert_eq!(
+ buf.as_slice(),
+ TestPayload::generate_pattern(t.payload_size)?.as_slice()
+ );
+ Ok(())
+ }
+
+ #[test]
+ fn split_command() -> Result {
+ check_split(SplitTest {
+ payload_size: 0,
+ num_continuations: 0,
+ })?;
+ check_split(SplitTest {
+ payload_size: MAX_FIRST_PAYLOAD,
+ num_continuations: 0,
+ })?;
+ check_split(SplitTest {
+ payload_size: MAX_FIRST_PAYLOAD + 1,
+ num_continuations: 1,
+ })?;
+ check_split(SplitTest {
+ payload_size: MAX_FIRST_PAYLOAD + MAX_CMD_SIZE,
+ num_continuations: 1,
+ })?;
+ check_split(SplitTest {
+ payload_size: MAX_FIRST_PAYLOAD + MAX_CMD_SIZE + 1,
+ num_continuations: 2,
+ })?;
+ check_split(SplitTest {
+ payload_size: MAX_FIRST_PAYLOAD + MAX_CMD_SIZE * 3 + MAX_CMD_SIZE / 2,
+ num_continuations: 4,
+ })?;
+ Ok(())
+ }
+}
--
2.53.0
^ permalink raw reply related [flat|nested] 24+ messages in thread* Claude review: gpu: nova-core: gsp: add tests for continuation records
2026-03-06 7:22 ` [PATCH v6 9/9] gpu: nova-core: gsp: add tests for continuation records Eliot Courtney
@ 2026-03-08 23:11 ` Claude Code Review Bot
0 siblings, 0 replies; 24+ messages in thread
From: Claude Code Review Bot @ 2026-03-08 23:11 UTC (permalink / raw)
To: dri-devel-reviews
Patch Review
Comprehensive kunit tests covering:
- Zero payload (no split)
- Payload exactly at `MAX_FIRST_PAYLOAD` (fits in one command)
- Payload at `MAX_FIRST_PAYLOAD + 1` (triggers 1 continuation)
- Payload at `MAX_FIRST_PAYLOAD + MAX_CMD_SIZE` (fills exactly 1 continuation)
- Payload at `MAX_FIRST_PAYLOAD + MAX_CMD_SIZE + 1` (triggers 2 continuations)
- Larger payload with fractional continuation fill
The test verifies both the number of continuation records and that the concatenated data matches the original pattern. The `TestHeader` uses a 64-byte struct to exercise the non-trivial case where the first command has less payload space due to the header.
The `TestPayload::generate_pattern` uses `(i ^ (i >> 8)) as u8` to avoid a repeating 256-byte pattern, which is a nice touch for catching off-by-one errors in splitting.
One very minor nit: `read_payload` creates an `SBufferIter` with `[buf.as_mut_slice(), &mut []]` — the empty second slice is fine but could have a brief comment. Not a blocking issue.
No issues. Good test coverage.
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 24+ messages in thread
* Claude review: gpu: nova-core: gsp: add continuation record support
2026-03-06 7:21 [PATCH v6 0/9] gpu: nova-core: gsp: add continuation record support Eliot Courtney
` (8 preceding siblings ...)
2026-03-06 7:22 ` [PATCH v6 9/9] gpu: nova-core: gsp: add tests for continuation records Eliot Courtney
@ 2026-03-08 23:11 ` Claude Code Review Bot
9 siblings, 0 replies; 24+ messages in thread
From: Claude Code Review Bot @ 2026-03-08 23:11 UTC (permalink / raw)
To: dri-devel-reviews
Overall Series Review
Subject: gpu: nova-core: gsp: add continuation record support
Author: Eliot Courtney <ecourtney@nvidia.com>
Patches: 10
Reviewed: 2026-03-09T09:11:25.577837
---
This is a well-structured 9-patch series adding continuation record support for the nova-core GSP command queue. Large RPCs (>16 pages / 64KB) need to be split across multiple queue elements, and this series implements that cleanly. The patches are logically decomposed: preparatory cleanups (sorting, timeout mechanism, size checking), then the core continuation record implementation, followed by tests.
The code quality is high. The `SplitState` design avoids unnecessary copies for commands that fit in a single element, and the `ContinuationRecords` iterator pattern keeps the splitting logic contained. The tests cover important boundary conditions. This is at v6 with prior reviews incorporated.
**Minor observations** are noted below per-patch, but overall this series looks good to merge.
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v5 8/9] gpu: nova-core: gsp: support large RPCs via continuation record
2026-03-04 1:42 [PATCH v5 0/9] " Eliot Courtney
@ 2026-03-04 1:42 ` Eliot Courtney
2026-03-05 3:55 ` Claude review: " Claude Code Review Bot
0 siblings, 1 reply; 24+ messages in thread
From: Eliot Courtney @ 2026-03-04 1:42 UTC (permalink / raw)
To: Danilo Krummrich, Alice Ryhl, Alexandre Courbot, David Airlie,
Simona Vetter, Miguel Ojeda, Boqun Feng, Gary Guo,
Björn Roy Baron, Benno Lossin, Andreas Hindborg,
Trevor Gross
Cc: Zhi Wang, John Hubbard, Alistair Popple, Joel Fernandes,
Timur Tabi, nouveau, dri-devel, linux-kernel, rust-for-linux,
Eliot Courtney
Splits large RPCs if necessary and sends the remaining parts using
continuation records. RPCs that do not need continuation records
continue to write directly into the command buffer. Ones that do write
into a staging buffer first, so there is one copy.
Continuation record for receive is not necessary to support at the
moment because those replies do not need to be read and are currently
drained by retrying `receive_msg` on `ERANGE`.
Signed-off-by: Eliot Courtney <ecourtney@nvidia.com>
---
drivers/gpu/nova-core/gsp/cmdq.rs | 42 ++++++-
drivers/gpu/nova-core/gsp/cmdq/continuation.rs | 163 +++++++++++++++++++++++++
drivers/gpu/nova-core/gsp/fw.rs | 4 +
3 files changed, 207 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/nova-core/gsp/cmdq.rs b/drivers/gpu/nova-core/gsp/cmdq.rs
index 3424be4e15f8..492e9489e808 100644
--- a/drivers/gpu/nova-core/gsp/cmdq.rs
+++ b/drivers/gpu/nova-core/gsp/cmdq.rs
@@ -1,5 +1,7 @@
// SPDX-License-Identifier: GPL-2.0
+mod continuation;
+
use core::{
mem,
sync::atomic::{
@@ -25,6 +27,11 @@
},
};
+use continuation::{
+ ContinuationRecord,
+ SplitState, //
+};
+
use crate::{
driver::Bar0,
gsp::{
@@ -520,7 +527,7 @@ fn notify_gsp(bar: &Bar0) {
.write(bar);
}
- /// Sends `command` to the GSP.
+ /// Sends `command` to the GSP, without splitting it.
///
/// # Errors
///
@@ -529,7 +536,7 @@ fn notify_gsp(bar: &Bar0) {
/// written to by its [`CommandToGsp::init_variable_payload`] method.
///
/// Error codes returned by the command initializers are propagated as-is.
- pub(crate) fn send_command<M>(&mut self, bar: &Bar0, command: M) -> Result
+ fn send_single_command<M>(&mut self, bar: &Bar0, command: M) -> Result
where
M: CommandToGsp,
// This allows all error types, including `Infallible`, to be used for `M::InitError`.
@@ -588,6 +595,37 @@ pub(crate) fn send_command<M>(&mut self, bar: &Bar0, command: M) -> Result
Ok(())
}
+ /// Sends `command` to the GSP.
+ ///
+ /// The command may be split into multiple messages if it is large.
+ ///
+ /// # Errors
+ ///
+ /// - `ETIMEDOUT` if space does not become available within the timeout.
+ /// - `EIO` if the variable payload requested by the command has not been entirely
+ /// written to by its [`CommandToGsp::init_variable_payload`] method.
+ ///
+ /// Error codes returned by the command initializers are propagated as-is.
+ pub(crate) fn send_command<M>(&mut self, bar: &Bar0, command: M) -> Result
+ where
+ M: CommandToGsp,
+ Error: From<M::InitError>,
+ {
+ match SplitState::new(command)? {
+ SplitState::Single(command) => self.send_single_command(bar, command),
+ SplitState::Split(command, mut continuations) => {
+ self.send_single_command(bar, command)?;
+
+ while let Some(continuation) = continuations.next() {
+ // Turbofish needed because the compiler cannot infer M here.
+ self.send_single_command::<ContinuationRecord<'_>>(bar, continuation)?;
+ }
+
+ Ok(())
+ }
+ }
+ }
+
/// Wait for a message to become available on the message queue.
///
/// This works purely at the transport layer and does not interpret or validate the message
diff --git a/drivers/gpu/nova-core/gsp/cmdq/continuation.rs b/drivers/gpu/nova-core/gsp/cmdq/continuation.rs
new file mode 100644
index 000000000000..3b83cdcbb39e
--- /dev/null
+++ b/drivers/gpu/nova-core/gsp/cmdq/continuation.rs
@@ -0,0 +1,163 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Support for splitting large GSP commands across continuation records.
+
+use core::convert::Infallible;
+
+use kernel::prelude::*;
+
+use super::CommandToGsp;
+
+use crate::{
+ gsp::fw::{
+ GspMsgElement,
+ MsgFunction,
+ GSP_MSG_QUEUE_ELEMENT_SIZE_MAX, //
+ },
+ sbuffer::SBufferIter,
+};
+
+/// Maximum command size that fits in a single queue element.
+const MAX_CMD_SIZE: usize = GSP_MSG_QUEUE_ELEMENT_SIZE_MAX - size_of::<GspMsgElement>();
+
+/// Acts as an iterator over the continuation records for a split command.
+pub(super) struct ContinuationRecords {
+ payload: KVVec<u8>,
+ offset: usize,
+}
+
+impl ContinuationRecords {
+ /// Creates a new iterator over continuation records for the given payload.
+ fn new(payload: KVVec<u8>) -> Self {
+ Self { payload, offset: 0 }
+ }
+
+ /// Returns the next continuation record, or [`None`] if there are no more.
+ pub(super) fn next(&mut self) -> Option<ContinuationRecord<'_>> {
+ let remaining = self.payload.len() - self.offset;
+
+ if remaining > 0 {
+ let chunk_size = remaining.min(MAX_CMD_SIZE);
+ let record =
+ ContinuationRecord::new(&self.payload[self.offset..(self.offset + chunk_size)]);
+ self.offset += chunk_size;
+ Some(record)
+ } else {
+ None
+ }
+ }
+}
+
+/// The [`ContinuationRecord`] command.
+pub(super) struct ContinuationRecord<'a> {
+ data: &'a [u8],
+}
+
+impl<'a> ContinuationRecord<'a> {
+ /// Creates a new [`ContinuationRecord`] command with the given data.
+ fn new(data: &'a [u8]) -> Self {
+ Self { data }
+ }
+}
+
+impl<'a> CommandToGsp for ContinuationRecord<'a> {
+ const FUNCTION: MsgFunction = MsgFunction::ContinuationRecord;
+ type Command = ();
+ type InitError = Infallible;
+
+ fn init(&self) -> impl Init<Self::Command, Self::InitError> {
+ <()>::init_zeroed()
+ }
+
+ fn variable_payload_len(&self) -> usize {
+ self.data.len()
+ }
+
+ fn init_variable_payload(
+ &self,
+ dst: &mut SBufferIter<core::array::IntoIter<&mut [u8], 2>>,
+ ) -> Result {
+ dst.write_all(self.data)
+ }
+}
+
+/// Whether a command needs to be split across continuation records or not.
+pub(super) enum SplitState<C: CommandToGsp> {
+ /// A command that fits in a single queue element.
+ Single(C),
+ /// A command split across continuation records.
+ Split(SplitCommand<C>, ContinuationRecords),
+}
+
+impl<C: CommandToGsp> SplitState<C> {
+ /// Maximum variable payload size that fits in the first command alongside the command header.
+ const MAX_FIRST_PAYLOAD: usize = MAX_CMD_SIZE - size_of::<C::Command>();
+
+ /// Creates a new [`SplitState`] for the given command.
+ ///
+ /// If the command is too large, it will be split into a main command and some number of
+ /// continuation records.
+ pub(super) fn new(command: C) -> Result<Self> {
+ let payload_len = command.variable_payload_len();
+
+ if command.size_in_bytes() > MAX_CMD_SIZE {
+ let mut command_payload =
+ KVVec::<u8>::from_elem(0u8, payload_len.min(Self::MAX_FIRST_PAYLOAD), GFP_KERNEL)?;
+ let mut continuation_payload =
+ KVVec::<u8>::from_elem(0u8, payload_len - command_payload.len(), GFP_KERNEL)?;
+ let mut sbuffer = SBufferIter::new_writer([
+ command_payload.as_mut_slice(),
+ continuation_payload.as_mut_slice(),
+ ]);
+
+ command.init_variable_payload(&mut sbuffer)?;
+ if !sbuffer.is_empty() {
+ return Err(EIO);
+ }
+ drop(sbuffer);
+
+ Ok(Self::Split(
+ SplitCommand::new(command, command_payload),
+ ContinuationRecords::new(continuation_payload),
+ ))
+ } else {
+ Ok(Self::Single(command))
+ }
+ }
+}
+
+/// A command that has been truncated to maximum accepted length of the command queue.
+///
+/// The remainder of its payload is expected to be sent using [`ContinuationRecords`].
+pub(super) struct SplitCommand<C: CommandToGsp> {
+ command: C,
+ payload: KVVec<u8>,
+}
+
+impl<C: CommandToGsp> SplitCommand<C> {
+ /// Creates a new [`SplitCommand`] wrapping `command` with the given truncated payload.
+ fn new(command: C, payload: KVVec<u8>) -> Self {
+ Self { command, payload }
+ }
+}
+
+impl<C: CommandToGsp> CommandToGsp for SplitCommand<C> {
+ const FUNCTION: MsgFunction = C::FUNCTION;
+ type Command = C::Command;
+ type InitError = C::InitError;
+
+ fn init(&self) -> impl Init<Self::Command, Self::InitError> {
+ self.command.init()
+ }
+
+ fn variable_payload_len(&self) -> usize {
+ self.payload.len()
+ }
+
+ fn init_variable_payload(
+ &self,
+ dst: &mut SBufferIter<core::array::IntoIter<&mut [u8], 2>>,
+ ) -> Result {
+ dst.write_all(&self.payload)
+ }
+}
diff --git a/drivers/gpu/nova-core/gsp/fw.rs b/drivers/gpu/nova-core/gsp/fw.rs
index 6005362450cb..25fca1f6db2c 100644
--- a/drivers/gpu/nova-core/gsp/fw.rs
+++ b/drivers/gpu/nova-core/gsp/fw.rs
@@ -202,6 +202,7 @@ pub(crate) enum MsgFunction {
AllocObject = bindings::NV_VGPU_MSG_FUNCTION_ALLOC_OBJECT,
AllocRoot = bindings::NV_VGPU_MSG_FUNCTION_ALLOC_ROOT,
BindCtxDma = bindings::NV_VGPU_MSG_FUNCTION_BIND_CTX_DMA,
+ ContinuationRecord = bindings::NV_VGPU_MSG_FUNCTION_CONTINUATION_RECORD,
Free = bindings::NV_VGPU_MSG_FUNCTION_FREE,
GetGspStaticInfo = bindings::NV_VGPU_MSG_FUNCTION_GET_GSP_STATIC_INFO,
GetStaticInfo = bindings::NV_VGPU_MSG_FUNCTION_GET_STATIC_INFO,
@@ -239,6 +240,9 @@ fn try_from(value: u32) -> Result<MsgFunction> {
bindings::NV_VGPU_MSG_FUNCTION_ALLOC_OBJECT => Ok(MsgFunction::AllocObject),
bindings::NV_VGPU_MSG_FUNCTION_ALLOC_ROOT => Ok(MsgFunction::AllocRoot),
bindings::NV_VGPU_MSG_FUNCTION_BIND_CTX_DMA => Ok(MsgFunction::BindCtxDma),
+ bindings::NV_VGPU_MSG_FUNCTION_CONTINUATION_RECORD => {
+ Ok(MsgFunction::ContinuationRecord)
+ }
bindings::NV_VGPU_MSG_FUNCTION_FREE => Ok(MsgFunction::Free),
bindings::NV_VGPU_MSG_FUNCTION_GET_GSP_STATIC_INFO => Ok(MsgFunction::GetGspStaticInfo),
bindings::NV_VGPU_MSG_FUNCTION_GET_STATIC_INFO => Ok(MsgFunction::GetStaticInfo),
--
2.53.0
^ permalink raw reply related [flat|nested] 24+ messages in thread* Claude review: gpu: nova-core: gsp: support large RPCs via continuation record
2026-03-04 1:42 ` [PATCH v5 8/9] gpu: nova-core: gsp: support large RPCs via continuation record Eliot Courtney
@ 2026-03-05 3:55 ` Claude Code Review Bot
0 siblings, 0 replies; 24+ messages in thread
From: Claude Code Review Bot @ 2026-03-05 3:55 UTC (permalink / raw)
To: dri-devel-reviews
Patch Review
This is the main patch. The design is solid:
1. `SplitState::new()` checks if splitting is needed and stages the payload into kernel-allocated buffers
2. `SplitCommand` wraps the original command with truncated payload for the first message
3. `ContinuationRecords` iterates over remaining payload chunks
A few observations:
**`ContinuationRecords` doesn't implement `Iterator`**: The `next()` method signature matches `Iterator` but it doesn't implement the trait. This is likely intentional since `Iterator` would require `&mut self` lifetime complications with the returned `ContinuationRecord<'_>` borrowing from `self`. The usage in `send_command` with `while let Some(...)` is fine. Consider a brief comment explaining this design choice.
**Missing `Tested-by` tag**: This is the only non-trivial patch without a `Tested-by: Zhi Wang` tag (patches 1-7 and 9 all have it). This may be intentional since the patch changed substantively in v4/v5.
**The `SplitState::MAX_FIRST_PAYLOAD` associated constant**:
```rust
const MAX_FIRST_PAYLOAD: usize = MAX_CMD_SIZE - size_of::<C::Command>();
```
This is defined as an associated constant on the generic `SplitState<C>`, which is elegant since it depends on `C::Command`. It's also exposed to tests in patch 9 via `SplitState::<TestPayload>::MAX_FIRST_PAYLOAD`, which is a nice pattern.
**Allocation strategy**: The staging buffer uses `KVVec::from_elem(0u8, ...)` which zero-initializes. The data is immediately overwritten by `init_variable_payload`, so `KVVec::with_capacity` + `resize` would save the zero-fill, but this is a minor optimization concern for a path that's only taken for large (>64KB) RPCs.
**`ContinuationRecord` uses `Command = ()`**: This means the GspMsgElement header is written but the command body is entirely variable-length payload. This matches the continuation record protocol where only the raw data matters.
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v4 8/9] gpu: nova-core: gsp: support large RPCs via continuation record
2026-03-02 11:42 [PATCH v4 0/9] gpu: nova-core: gsp: add continuation record support Eliot Courtney
@ 2026-03-02 11:42 ` Eliot Courtney
2026-03-03 3:34 ` Claude review: " Claude Code Review Bot
0 siblings, 1 reply; 24+ messages in thread
From: Eliot Courtney @ 2026-03-02 11:42 UTC (permalink / raw)
To: Danilo Krummrich, Alice Ryhl, Alexandre Courbot, David Airlie,
Simona Vetter, Miguel Ojeda, Boqun Feng, Gary Guo,
Björn Roy Baron, Benno Lossin, Andreas Hindborg,
Trevor Gross
Cc: Zhi Wang, John Hubbard, Alistair Popple, Joel Fernandes,
Timur Tabi, nouveau, dri-devel, linux-kernel, rust-for-linux,
Eliot Courtney
Splits large RPCs if necessary and sends the remaining parts using
continuation records. RPCs that do not need continuation records
continue to write directly into the command buffer. Ones that do write
into a staging buffer first, so there is one copy.
Continuation record for receive is not necessary to support at the
moment because those replies do not need to be read and are currently
drained by retrying `receive_msg` on ERANGE.
Signed-off-by: Eliot Courtney <ecourtney@nvidia.com>
---
drivers/gpu/nova-core/gsp.rs | 1 +
drivers/gpu/nova-core/gsp/cmdq.rs | 41 +++++++-
drivers/gpu/nova-core/gsp/continuation.rs | 167 ++++++++++++++++++++++++++++++
drivers/gpu/nova-core/gsp/fw.rs | 4 +
4 files changed, 210 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/nova-core/gsp.rs b/drivers/gpu/nova-core/gsp.rs
index 174feaca0a6b..ccf56f1ad246 100644
--- a/drivers/gpu/nova-core/gsp.rs
+++ b/drivers/gpu/nova-core/gsp.rs
@@ -16,6 +16,7 @@
pub(crate) mod cmdq;
pub(crate) mod commands;
+mod continuation;
mod fw;
mod sequencer;
diff --git a/drivers/gpu/nova-core/gsp/cmdq.rs b/drivers/gpu/nova-core/gsp/cmdq.rs
index 4a663a5b3437..4caf5917de64 100644
--- a/drivers/gpu/nova-core/gsp/cmdq.rs
+++ b/drivers/gpu/nova-core/gsp/cmdq.rs
@@ -28,6 +28,10 @@
use crate::{
driver::Bar0,
gsp::{
+ continuation::{
+ ContinuationRecord,
+ SplitState, //
+ },
fw::{
GspMsgElement,
MsgFunction,
@@ -452,7 +456,7 @@ struct GspMessage<'a> {
/// Computes the total size of the command (including its variable-length payload) without the
/// [`GspMsgElement`] header.
-fn command_size<M>(command: &M) -> usize
+pub(crate) fn command_size<M>(command: &M) -> usize
where
M: CommandToGsp,
{
@@ -520,7 +524,7 @@ fn notify_gsp(bar: &Bar0) {
.write(bar);
}
- /// Sends `command` to the GSP.
+ /// Sends `command` to the GSP, without splitting it.
///
/// # Errors
///
@@ -529,7 +533,7 @@ fn notify_gsp(bar: &Bar0) {
/// written to by its [`CommandToGsp::init_variable_payload`] method.
///
/// Error codes returned by the command initializers are propagated as-is.
- pub(crate) fn send_command<M>(&mut self, bar: &Bar0, command: M) -> Result
+ fn send_single_command<M>(&mut self, bar: &Bar0, command: M) -> Result
where
M: CommandToGsp,
// This allows all error types, including `Infallible`, to be used for `M::InitError`.
@@ -588,6 +592,37 @@ pub(crate) fn send_command<M>(&mut self, bar: &Bar0, command: M) -> Result
Ok(())
}
+ /// Sends `command` to the GSP.
+ ///
+ /// The command may be split into multiple messages if it is large.
+ ///
+ /// # Errors
+ ///
+ /// - `ETIMEDOUT` if space does not become available within the timeout.
+ /// - `EIO` if the variable payload requested by the command has not been entirely
+ /// written to by its [`CommandToGsp::init_variable_payload`] method.
+ ///
+ /// Error codes returned by the command initializers are propagated as-is.
+ pub(crate) fn send_command<M>(&mut self, bar: &Bar0, command: M) -> Result
+ where
+ M: CommandToGsp,
+ Error: From<M::InitError>,
+ {
+ match SplitState::new(command)? {
+ SplitState::Single(command) => self.send_single_command(bar, command),
+ SplitState::Split(command, mut continuations) => {
+ self.send_single_command(bar, command)?;
+
+ while let Some(continuation) = continuations.next() {
+ // Turbofish needed because the compiler cannot infer M here.
+ self.send_single_command::<ContinuationRecord<'_>>(bar, continuation)?;
+ }
+
+ Ok(())
+ }
+ }
+ }
+
/// Wait for a message to become available on the message queue.
///
/// This works purely at the transport layer and does not interpret or validate the message
diff --git a/drivers/gpu/nova-core/gsp/continuation.rs b/drivers/gpu/nova-core/gsp/continuation.rs
new file mode 100644
index 000000000000..cf6d950a0e3f
--- /dev/null
+++ b/drivers/gpu/nova-core/gsp/continuation.rs
@@ -0,0 +1,167 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Support for splitting large GSP commands across continuation records.
+
+use core::convert::Infallible;
+
+use kernel::prelude::*;
+
+use crate::{
+ gsp::{
+ cmdq::{
+ command_size,
+ CommandToGsp, //
+ },
+ fw::{
+ GspMsgElement,
+ MsgFunction,
+ GSP_MSG_QUEUE_ELEMENT_SIZE_MAX, //
+ },
+ },
+ sbuffer::SBufferIter,
+};
+
+/// Maximum command size that fits in a single queue element.
+const MAX_CMD_SIZE: usize = GSP_MSG_QUEUE_ELEMENT_SIZE_MAX - size_of::<GspMsgElement>();
+
+/// Acts as an iterator over the continuation records for a split command.
+pub(super) struct ContinuationRecords {
+ payload: KVVec<u8>,
+ offset: usize,
+}
+
+impl ContinuationRecords {
+ /// Creates a new iterator over continuation records for the given payload.
+ fn new(payload: KVVec<u8>) -> Self {
+ Self { payload, offset: 0 }
+ }
+
+ /// Returns the next continuation record, or [`None`] if there are no more.
+ pub(super) fn next(&mut self) -> Option<ContinuationRecord<'_>> {
+ let remaining = self.payload.len() - self.offset;
+
+ if remaining > 0 {
+ let chunk_size = remaining.min(MAX_CMD_SIZE);
+ let record =
+ ContinuationRecord::new(&self.payload[self.offset..(self.offset + chunk_size)]);
+ self.offset += chunk_size;
+ Some(record)
+ } else {
+ None
+ }
+ }
+}
+
+/// The [`ContinuationRecord`] command.
+pub(super) struct ContinuationRecord<'a> {
+ data: &'a [u8],
+}
+
+impl<'a> ContinuationRecord<'a> {
+ /// Creates a new [`ContinuationRecord`] command with the given data.
+ fn new(data: &'a [u8]) -> Self {
+ Self { data }
+ }
+}
+
+impl<'a> CommandToGsp for ContinuationRecord<'a> {
+ const FUNCTION: MsgFunction = MsgFunction::ContinuationRecord;
+ type Command = ();
+ type InitError = Infallible;
+
+ fn init(&self) -> impl Init<Self::Command, Self::InitError> {
+ <()>::init_zeroed()
+ }
+
+ fn variable_payload_len(&self) -> usize {
+ self.data.len()
+ }
+
+ fn init_variable_payload(
+ &self,
+ dst: &mut SBufferIter<core::array::IntoIter<&mut [u8], 2>>,
+ ) -> Result {
+ dst.write_all(self.data)
+ }
+}
+
+/// Whether a command needs to be split across continuation records or not.
+pub(super) enum SplitState<C: CommandToGsp> {
+ /// A command that fits in a single queue element.
+ Single(C),
+ /// A command split across continuation records.
+ Split(SplitCommand<C>, ContinuationRecords),
+}
+
+impl<C: CommandToGsp> SplitState<C> {
+ /// Maximum variable payload size that fits in the first command alongside the command header.
+ const MAX_FIRST_PAYLOAD: usize = MAX_CMD_SIZE - size_of::<C::Command>();
+
+ /// Creates a new [`SplitState`] for the given command.
+ ///
+ /// If the command is too large, it will be split into a main command and some number of
+ /// continuation records.
+ pub(super) fn new(command: C) -> Result<Self> {
+ let payload_len = command.variable_payload_len();
+
+ if command_size(&command) > MAX_CMD_SIZE {
+ let mut command_payload =
+ KVVec::<u8>::from_elem(0u8, payload_len.min(Self::MAX_FIRST_PAYLOAD), GFP_KERNEL)?;
+ let mut continuation_payload =
+ KVVec::<u8>::from_elem(0u8, payload_len - command_payload.len(), GFP_KERNEL)?;
+ let mut sbuffer = SBufferIter::new_writer([
+ command_payload.as_mut_slice(),
+ continuation_payload.as_mut_slice(),
+ ]);
+
+ command.init_variable_payload(&mut sbuffer)?;
+ if !sbuffer.is_empty() {
+ return Err(EIO);
+ }
+ drop(sbuffer);
+
+ Ok(Self::Split(
+ SplitCommand::new(command, command_payload),
+ ContinuationRecords::new(continuation_payload),
+ ))
+ } else {
+ Ok(Self::Single(command))
+ }
+ }
+}
+
+/// A command that has been truncated to maximum accepted length of the command queue.
+///
+/// The remainder of its payload is expected to be sent using [`ContinuationRecords`].
+pub(super) struct SplitCommand<C: CommandToGsp> {
+ command: C,
+ payload: KVVec<u8>,
+}
+
+impl<C: CommandToGsp> SplitCommand<C> {
+ /// Creates a new [`SplitCommand`] wrapping `command` with the given truncated payload.
+ fn new(command: C, payload: KVVec<u8>) -> Self {
+ Self { command, payload }
+ }
+}
+
+impl<C: CommandToGsp> CommandToGsp for SplitCommand<C> {
+ const FUNCTION: MsgFunction = C::FUNCTION;
+ type Command = C::Command;
+ type InitError = C::InitError;
+
+ fn init(&self) -> impl Init<Self::Command, Self::InitError> {
+ self.command.init()
+ }
+
+ fn variable_payload_len(&self) -> usize {
+ self.payload.len()
+ }
+
+ fn init_variable_payload(
+ &self,
+ dst: &mut SBufferIter<core::array::IntoIter<&mut [u8], 2>>,
+ ) -> Result {
+ dst.write_all(&self.payload)
+ }
+}
diff --git a/drivers/gpu/nova-core/gsp/fw.rs b/drivers/gpu/nova-core/gsp/fw.rs
index 6005362450cb..25fca1f6db2c 100644
--- a/drivers/gpu/nova-core/gsp/fw.rs
+++ b/drivers/gpu/nova-core/gsp/fw.rs
@@ -202,6 +202,7 @@ pub(crate) enum MsgFunction {
AllocObject = bindings::NV_VGPU_MSG_FUNCTION_ALLOC_OBJECT,
AllocRoot = bindings::NV_VGPU_MSG_FUNCTION_ALLOC_ROOT,
BindCtxDma = bindings::NV_VGPU_MSG_FUNCTION_BIND_CTX_DMA,
+ ContinuationRecord = bindings::NV_VGPU_MSG_FUNCTION_CONTINUATION_RECORD,
Free = bindings::NV_VGPU_MSG_FUNCTION_FREE,
GetGspStaticInfo = bindings::NV_VGPU_MSG_FUNCTION_GET_GSP_STATIC_INFO,
GetStaticInfo = bindings::NV_VGPU_MSG_FUNCTION_GET_STATIC_INFO,
@@ -239,6 +240,9 @@ fn try_from(value: u32) -> Result<MsgFunction> {
bindings::NV_VGPU_MSG_FUNCTION_ALLOC_OBJECT => Ok(MsgFunction::AllocObject),
bindings::NV_VGPU_MSG_FUNCTION_ALLOC_ROOT => Ok(MsgFunction::AllocRoot),
bindings::NV_VGPU_MSG_FUNCTION_BIND_CTX_DMA => Ok(MsgFunction::BindCtxDma),
+ bindings::NV_VGPU_MSG_FUNCTION_CONTINUATION_RECORD => {
+ Ok(MsgFunction::ContinuationRecord)
+ }
bindings::NV_VGPU_MSG_FUNCTION_FREE => Ok(MsgFunction::Free),
bindings::NV_VGPU_MSG_FUNCTION_GET_GSP_STATIC_INFO => Ok(MsgFunction::GetGspStaticInfo),
bindings::NV_VGPU_MSG_FUNCTION_GET_STATIC_INFO => Ok(MsgFunction::GetStaticInfo),
--
2.53.0
^ permalink raw reply related [flat|nested] 24+ messages in thread* Claude review: gpu: nova-core: gsp: support large RPCs via continuation record
2026-03-02 11:42 ` [PATCH v4 8/9] gpu: nova-core: gsp: support large RPCs via continuation record Eliot Courtney
@ 2026-03-03 3:34 ` Claude Code Review Bot
0 siblings, 0 replies; 24+ messages in thread
From: Claude Code Review Bot @ 2026-03-03 3:34 UTC (permalink / raw)
To: dri-devel-reviews
Patch Review
This is the core patch. The design is solid:
- `ContinuationRecord<'a>` wraps a `&[u8]` slice, uses `MsgFunction::ContinuationRecord`, and has `type Command = ()` (zero-sized header). This is correct since continuation records are pure payload.
- `ContinuationRecords` provides a lending-iterator pattern via a custom `next()` method rather than implementing `Iterator`, which is the right approach since `ContinuationRecord` borrows from the parent.
- `SplitState<C>` cleanly separates the single vs. split cases. The associated constant `MAX_FIRST_PAYLOAD` properly accounts for the command header:
```rust
const MAX_FIRST_PAYLOAD: usize = MAX_CMD_SIZE - size_of::<C::Command>();
```
- `SplitCommand<C>` delegates `init()` to the original command but replaces the variable payload with the pre-filled staging buffer.
**Observation on the send_command flow:** For split commands, `SplitState::new()` calls `command.init_variable_payload()` to fill the staging buffer. Then `send_single_command` calls `SplitCommand.init_variable_payload()` which copies from the staging buffer into the command queue. This means the payload is initialized once and copied once (for the first chunk) — which is correct and matches the cover letter's description.
**Observation on ContinuationRecord being sent through send_single_command:** The `allocate_command` size check uses `size_of::<GspMsgElement>() + size`. For ContinuationRecord, the size is `0 (Command=()) + data.len()`, and `data.len() <= MAX_CMD_SIZE`, so `size_of::<GspMsgElement>() + data.len() <= GSP_MSG_QUEUE_ELEMENT_SIZE_MAX`. The check passes correctly.
**Minor style observation:** The trailing `//` in imports is a rustfmt formatting workaround:
```rust
ContinuationRecord,
SplitState, //
```
This is a known pattern but could be confusing to newcomers. Not a blocking issue.
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v3 8/9] gpu: nova-core: gsp: support large RPCs via continuation record
2026-02-26 11:45 [PATCH v3 0/9] gpu: nova-core: gsp: add continuation record support Eliot Courtney
@ 2026-02-26 11:45 ` Eliot Courtney
2026-02-27 2:21 ` Claude review: " Claude Code Review Bot
0 siblings, 1 reply; 24+ messages in thread
From: Eliot Courtney @ 2026-02-26 11:45 UTC (permalink / raw)
To: Danilo Krummrich, Alice Ryhl, Alexandre Courbot, David Airlie,
Simona Vetter, Miguel Ojeda, Boqun Feng, Gary Guo,
Björn Roy Baron, Benno Lossin, Andreas Hindborg,
Trevor Gross
Cc: nouveau, dri-devel, linux-kernel, rust-for-linux, Eliot Courtney,
Zhi Wang
Splits large RPCs if necessary and sends the remaining parts using
continuation records. RPCs that do not need continuation records
continue to write directly into the command buffer. Ones that do write
into a staging buffer first, so there is one copy.
Continuation record for receive is not necessary to support at the
moment because those replies do not need to be read and are currently
drained by retrying `receive_msg` on ERANGE.
Tested-by: Zhi Wang <zhiw@nvidia.com>
Signed-off-by: Eliot Courtney <ecourtney@nvidia.com>
---
drivers/gpu/nova-core/gsp/cmdq.rs | 43 +++++++++-
drivers/gpu/nova-core/gsp/commands.rs | 144 +++++++++++++++++++++++++++++++++-
drivers/gpu/nova-core/gsp/fw.rs | 4 +
3 files changed, 187 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/nova-core/gsp/cmdq.rs b/drivers/gpu/nova-core/gsp/cmdq.rs
index 4a663a5b3437..d68b93ddf7cc 100644
--- a/drivers/gpu/nova-core/gsp/cmdq.rs
+++ b/drivers/gpu/nova-core/gsp/cmdq.rs
@@ -28,6 +28,10 @@
use crate::{
driver::Bar0,
gsp::{
+ commands::{
+ ContinuationRecord,
+ SplitState, //
+ },
fw::{
GspMsgElement,
MsgFunction,
@@ -452,7 +456,7 @@ struct GspMessage<'a> {
/// Computes the total size of the command (including its variable-length payload) without the
/// [`GspMsgElement`] header.
-fn command_size<M>(command: &M) -> usize
+pub(crate) fn command_size<M>(command: &M) -> usize
where
M: CommandToGsp,
{
@@ -520,7 +524,7 @@ fn notify_gsp(bar: &Bar0) {
.write(bar);
}
- /// Sends `command` to the GSP.
+ /// Sends `command` to the GSP, without splitting it.
///
/// # Errors
///
@@ -529,7 +533,7 @@ fn notify_gsp(bar: &Bar0) {
/// written to by its [`CommandToGsp::init_variable_payload`] method.
///
/// Error codes returned by the command initializers are propagated as-is.
- pub(crate) fn send_command<M>(&mut self, bar: &Bar0, command: M) -> Result
+ fn send_single_command<M>(&mut self, bar: &Bar0, command: M) -> Result
where
M: CommandToGsp,
// This allows all error types, including `Infallible`, to be used for `M::InitError`.
@@ -588,6 +592,39 @@ pub(crate) fn send_command<M>(&mut self, bar: &Bar0, command: M) -> Result
Ok(())
}
+ /// Sends `command` to the GSP.
+ ///
+ /// The command may be split into multiple messages if it is large.
+ ///
+ /// # Errors
+ ///
+ /// - `ETIMEDOUT` if space does not become available within the timeout.
+ /// - `EIO` if the variable payload requested by the command has not been entirely
+ /// written to by its [`CommandToGsp::init_variable_payload`] method.
+ ///
+ /// Error codes returned by the command initializers are propagated as-is.
+ pub(crate) fn send_command<M>(&mut self, bar: &Bar0, command: M) -> Result
+ where
+ M: CommandToGsp,
+ Error: From<M::InitError>,
+ {
+ let mut state = SplitState::new(&command)?;
+
+ self.send_single_command(bar, state.command(command))?;
+
+ while let Some(continuation) = state.next_continuation_record() {
+ dev_dbg!(
+ &self.dev,
+ "GSP RPC: send continuation: size=0x{:x}\n",
+ command_size(&continuation),
+ );
+ // Turbofish needed because the compiler cannot infer M here.
+ self.send_single_command::<ContinuationRecord<'_>>(bar, continuation)?;
+ }
+
+ Ok(())
+ }
+
/// Wait for a message to become available on the message queue.
///
/// This works purely at the transport layer and does not interpret or validate the message
diff --git a/drivers/gpu/nova-core/gsp/commands.rs b/drivers/gpu/nova-core/gsp/commands.rs
index 8f270eca33be..6ffd0b9cbf05 100644
--- a/drivers/gpu/nova-core/gsp/commands.rs
+++ b/drivers/gpu/nova-core/gsp/commands.rs
@@ -4,6 +4,7 @@
array,
convert::Infallible,
ffi::FromBytesUntilNulError,
+ marker::PhantomData,
str::Utf8Error, //
};
@@ -22,13 +23,16 @@
driver::Bar0,
gsp::{
cmdq::{
+ command_size,
Cmdq,
CommandToGsp,
MessageFromGsp, //
},
fw::{
commands::*,
- MsgFunction, //
+ GspMsgElement,
+ MsgFunction,
+ GSP_MSG_QUEUE_ELEMENT_SIZE_MAX, //
},
},
sbuffer::SBufferIter,
@@ -242,3 +246,141 @@ pub(crate) fn get_gsp_info(cmdq: &mut Cmdq, bar: &Bar0) -> Result<GetGspStaticIn
}
}
}
+
+/// The `ContinuationRecord` command.
+pub(crate) struct ContinuationRecord<'a> {
+ data: &'a [u8],
+}
+
+impl<'a> ContinuationRecord<'a> {
+ /// Creates a new `ContinuationRecord` command with the given data.
+ pub(crate) fn new(data: &'a [u8]) -> Self {
+ Self { data }
+ }
+}
+
+impl<'a> CommandToGsp for ContinuationRecord<'a> {
+ const FUNCTION: MsgFunction = MsgFunction::ContinuationRecord;
+ type Command = ();
+ type InitError = Infallible;
+
+ fn init(&self) -> impl Init<Self::Command, Self::InitError> {
+ <()>::init_zeroed()
+ }
+
+ fn variable_payload_len(&self) -> usize {
+ self.data.len()
+ }
+
+ fn init_variable_payload(
+ &self,
+ dst: &mut SBufferIter<core::array::IntoIter<&mut [u8], 2>>,
+ ) -> Result {
+ dst.write_all(self.data)
+ }
+}
+
+/// Wrapper that splits a command across continuation records if needed.
+pub(crate) struct SplitState<C: CommandToGsp> {
+ state: Option<(KVVec<u8>, usize)>,
+ _phantom: PhantomData<C>,
+}
+
+impl<C: CommandToGsp> SplitState<C> {
+ /// Maximum command size that fits in a single queue element.
+ const MAX_CMD_SIZE: usize = GSP_MSG_QUEUE_ELEMENT_SIZE_MAX - size_of::<GspMsgElement>();
+
+ /// Maximum size of the variable payload that can be sent in the main command.
+ const MAX_FIRST_PAYLOAD_SIZE: usize = Self::MAX_CMD_SIZE - size_of::<C::Command>();
+
+ /// Creates a new `SplitState` for the given command.
+ ///
+ /// If the command is too large, it will be split into a main command and some number of
+ /// continuation records.
+ pub(crate) fn new(inner: &C) -> Result<Self> {
+ if command_size(inner) > Self::MAX_CMD_SIZE {
+ let mut staging =
+ KVVec::<u8>::from_elem(0u8, inner.variable_payload_len(), GFP_KERNEL)?;
+ let mut sbuffer = SBufferIter::new_writer([staging.as_mut_slice(), &mut []]);
+ inner.init_variable_payload(&mut sbuffer)?;
+ if !sbuffer.is_empty() {
+ return Err(EIO);
+ }
+ drop(sbuffer);
+
+ Ok(Self {
+ state: Some((staging, Self::MAX_FIRST_PAYLOAD_SIZE)),
+ _phantom: PhantomData,
+ })
+ } else {
+ Ok(Self {
+ state: None,
+ _phantom: PhantomData,
+ })
+ }
+ }
+
+ /// Returns the main command.
+ pub(crate) fn command(&self, inner: C) -> SplitCommand<'_, C> {
+ if let Some((staging, _)) = &self.state {
+ SplitCommand::Split(inner, staging)
+ } else {
+ SplitCommand::Single(inner)
+ }
+ }
+
+ /// Returns the next continuation record, or `None` if there are no more.
+ pub(crate) fn next_continuation_record(&mut self) -> Option<ContinuationRecord<'_>> {
+ let (staging, offset) = self.state.as_mut()?;
+
+ let remaining = staging.len() - *offset;
+ if remaining > 0 {
+ let chunk_size = remaining.min(Self::MAX_CMD_SIZE);
+ let record = ContinuationRecord::new(&staging[*offset..(*offset + chunk_size)]);
+ *offset += chunk_size;
+ Some(record)
+ } else {
+ None
+ }
+ }
+}
+
+/// Wrapper enum that represents either a single command or a split command with its staging buffer.
+pub(crate) enum SplitCommand<'a, C: CommandToGsp> {
+ /// A command that fits in a single queue element.
+ Single(C),
+ /// A command split across continuation records, with its full payload in a staging buffer.
+ Split(C, &'a [u8]),
+}
+
+impl<'a, C: CommandToGsp> CommandToGsp for SplitCommand<'a, C> {
+ const FUNCTION: MsgFunction = C::FUNCTION;
+ type Command = C::Command;
+ type InitError = C::InitError;
+
+ fn init(&self) -> impl Init<Self::Command, Self::InitError> {
+ match self {
+ SplitCommand::Single(cmd) => cmd.init(),
+ SplitCommand::Split(cmd, _) => cmd.init(),
+ }
+ }
+
+ fn variable_payload_len(&self) -> usize {
+ match self {
+ SplitCommand::Single(cmd) => cmd.variable_payload_len(),
+ SplitCommand::Split(_, _) => SplitState::<C>::MAX_FIRST_PAYLOAD_SIZE,
+ }
+ }
+
+ fn init_variable_payload(
+ &self,
+ dst: &mut SBufferIter<core::array::IntoIter<&mut [u8], 2>>,
+ ) -> Result {
+ match self {
+ SplitCommand::Single(cmd) => cmd.init_variable_payload(dst),
+ SplitCommand::Split(_, staging) => {
+ dst.write_all(&staging[..self.variable_payload_len()])
+ }
+ }
+ }
+}
diff --git a/drivers/gpu/nova-core/gsp/fw.rs b/drivers/gpu/nova-core/gsp/fw.rs
index 6005362450cb..25fca1f6db2c 100644
--- a/drivers/gpu/nova-core/gsp/fw.rs
+++ b/drivers/gpu/nova-core/gsp/fw.rs
@@ -202,6 +202,7 @@ pub(crate) enum MsgFunction {
AllocObject = bindings::NV_VGPU_MSG_FUNCTION_ALLOC_OBJECT,
AllocRoot = bindings::NV_VGPU_MSG_FUNCTION_ALLOC_ROOT,
BindCtxDma = bindings::NV_VGPU_MSG_FUNCTION_BIND_CTX_DMA,
+ ContinuationRecord = bindings::NV_VGPU_MSG_FUNCTION_CONTINUATION_RECORD,
Free = bindings::NV_VGPU_MSG_FUNCTION_FREE,
GetGspStaticInfo = bindings::NV_VGPU_MSG_FUNCTION_GET_GSP_STATIC_INFO,
GetStaticInfo = bindings::NV_VGPU_MSG_FUNCTION_GET_STATIC_INFO,
@@ -239,6 +240,9 @@ fn try_from(value: u32) -> Result<MsgFunction> {
bindings::NV_VGPU_MSG_FUNCTION_ALLOC_OBJECT => Ok(MsgFunction::AllocObject),
bindings::NV_VGPU_MSG_FUNCTION_ALLOC_ROOT => Ok(MsgFunction::AllocRoot),
bindings::NV_VGPU_MSG_FUNCTION_BIND_CTX_DMA => Ok(MsgFunction::BindCtxDma),
+ bindings::NV_VGPU_MSG_FUNCTION_CONTINUATION_RECORD => {
+ Ok(MsgFunction::ContinuationRecord)
+ }
bindings::NV_VGPU_MSG_FUNCTION_FREE => Ok(MsgFunction::Free),
bindings::NV_VGPU_MSG_FUNCTION_GET_GSP_STATIC_INFO => Ok(MsgFunction::GetGspStaticInfo),
bindings::NV_VGPU_MSG_FUNCTION_GET_STATIC_INFO => Ok(MsgFunction::GetStaticInfo),
--
2.53.0
^ permalink raw reply related [flat|nested] 24+ messages in thread* Claude review: gpu: nova-core: gsp: support large RPCs via continuation record
2026-02-26 11:45 ` [PATCH v3 8/9] gpu: nova-core: gsp: support large RPCs via continuation record Eliot Courtney
@ 2026-02-27 2:21 ` Claude Code Review Bot
0 siblings, 0 replies; 24+ messages in thread
From: Claude Code Review Bot @ 2026-02-27 2:21 UTC (permalink / raw)
To: dri-devel-reviews
Patch Review
This is the core patch. Three new types are introduced:
**`ContinuationRecord<'a>`**: A simple command that writes its payload as-is, using `type Command = ()` (zero-size header). This correctly means the entire element payload is variable data.
**`SplitState<C>`**: Determines whether splitting is needed and manages the staging buffer. Key constants:
```rust
const MAX_CMD_SIZE: usize = GSP_MSG_QUEUE_ELEMENT_SIZE_MAX - size_of::<GspMsgElement>();
const MAX_FIRST_PAYLOAD_SIZE: usize = Self::MAX_CMD_SIZE - size_of::<C::Command>();
```
The split decision in `new()` is correct — only when `command_size(inner) > MAX_CMD_SIZE` does staging occur. The staging buffer is populated eagerly by calling `inner.init_variable_payload()` once, which avoids calling the user's init function multiple times.
**`SplitCommand<'a, C>`**: An enum wrapping either the original command (`Single`) or the truncated first-chunk version (`Split`). The `CommandToGsp` impl for `Split` truncates `variable_payload_len()` to `MAX_FIRST_PAYLOAD_SIZE` and copies from the staging buffer.
The updated `send_command` flow is clean:
```rust
let mut state = SplitState::new(&command)?;
self.send_single_command(bar, state.command(command))?;
while let Some(continuation) = state.next_continuation_record() {
self.send_single_command::<ContinuationRecord<'_>>(bar, continuation)?;
}
```
**One observation**: `command_size` is changed from `fn` to `pub(crate) fn` to allow access from `commands.rs`. This is a minor visibility escalation that's fine for a crate-internal function.
**Observation on `next_continuation_record` continuation chunk sizing**: Each continuation record gets at most `MAX_CMD_SIZE` bytes of payload. Since `ContinuationRecord` has `type Command = ()` (zero-size), the total element size is `size_of::<GspMsgElement>() + chunk_size`, which maxes out at exactly `GSP_MSG_QUEUE_ELEMENT_SIZE_MAX`. This correctly stays within the per-element limit and won't trigger the `EMSGSIZE` check added in patch 4.
**Minor style nit**: The `_phantom: PhantomData` field in `SplitState` exists because the struct needs to carry the `C` type parameter for the associated constants (`MAX_CMD_SIZE`, `MAX_FIRST_PAYLOAD_SIZE`). This is a standard Rust pattern and is fine.
**Question for the author**: The cover letter mentions "continuation record for receive is not necessary to support at the moment because those replies do not need to be read and are currently drained by retrying `receive_msg` on ERANGE." Is there a concern that GSP could *respond* with continuation records to these large RPCs? If so, silently draining them seems fragile if future RPCs need to read large responses. A `dev_warn!` or tracking note might be useful there, though this is outside the scope of this series.
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v2 8/9] gpu: nova-core: gsp: support large RPCs via continuation record
2026-02-19 7:30 [PATCH v2 0/9] gpu: nova-core: gsp: add continuation record support Eliot Courtney
@ 2026-02-19 7:30 ` Eliot Courtney
2026-02-19 8:43 ` Claude review: " Claude Code Review Bot
0 siblings, 1 reply; 24+ messages in thread
From: Eliot Courtney @ 2026-02-19 7:30 UTC (permalink / raw)
To: Danilo Krummrich, Alice Ryhl, Alexandre Courbot, David Airlie,
Simona Vetter, Miguel Ojeda, Boqun Feng, Gary Guo,
Björn Roy Baron, Benno Lossin, Andreas Hindborg,
Trevor Gross
Cc: nouveau, dri-devel, linux-kernel, rust-for-linux, Eliot Courtney
Splits large RPCs if necessary and sends the remaining parts using
continuation records. RPCs that do not need continuation records
continue to write directly into the command buffer. Ones that do write
into a staging buffer first, so there is one copy.
Continuation record for receive is not necessary to support at the
moment because those replies do not need to be read and are currently
drained by retrying `receive_msg` on ERANGE.
Signed-off-by: Eliot Courtney <ecourtney@nvidia.com>
---
drivers/gpu/nova-core/gsp/cmdq.rs | 43 +++++++++-
drivers/gpu/nova-core/gsp/commands.rs | 143 +++++++++++++++++++++++++++++++++-
drivers/gpu/nova-core/gsp/fw.rs | 5 ++
3 files changed, 187 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/nova-core/gsp/cmdq.rs b/drivers/gpu/nova-core/gsp/cmdq.rs
index f832252ae45c..402b0b9418ab 100644
--- a/drivers/gpu/nova-core/gsp/cmdq.rs
+++ b/drivers/gpu/nova-core/gsp/cmdq.rs
@@ -29,6 +29,10 @@
use crate::{
driver::Bar0,
gsp::{
+ commands::{
+ ContinuationRecord,
+ SplitState, //
+ },
fw::{
GspMsgElement,
MsgFunction,
@@ -439,7 +443,7 @@ struct GspMessage<'a> {
/// Computes the total size of the command (not including the [`GspMsgElement`] header), including
/// its variable-length payload.
-fn command_size<M>(command: &M) -> usize
+pub(crate) fn command_size<M>(command: &M) -> usize
where
M: CommandToGsp,
{
@@ -507,7 +511,7 @@ fn notify_gsp(bar: &Bar0) {
.write(bar);
}
- /// Sends `command` to the GSP.
+ /// Sends `command` to the GSP, without splitting it.
///
/// # Errors
///
@@ -516,7 +520,7 @@ fn notify_gsp(bar: &Bar0) {
/// written to by its [`CommandToGsp::init_variable_payload`] method.
///
/// Error codes returned by the command initializers are propagated as-is.
- pub(crate) fn send_command<M>(&mut self, bar: &Bar0, command: M) -> Result
+ fn send_single_command<M>(&mut self, bar: &Bar0, command: M) -> Result
where
M: CommandToGsp,
// This allows all error types, including `Infallible`, to be used for `M::InitError`.
@@ -575,6 +579,39 @@ pub(crate) fn send_command<M>(&mut self, bar: &Bar0, command: M) -> Result
Ok(())
}
+ /// Sends `command` to the GSP.
+ ///
+ /// The command may be split into multiple messages if it is large.
+ ///
+ /// # Errors
+ ///
+ /// - `ETIMEDOUT` if space does not become available within the timeout.
+ /// - `EIO` if the variable payload requested by the command has not been entirely
+ /// written to by its [`CommandToGsp::init_variable_payload`] method.
+ ///
+ /// Error codes returned by the command initializers are propagated as-is.
+ pub(crate) fn send_command<M>(&mut self, bar: &Bar0, command: M) -> Result
+ where
+ M: CommandToGsp,
+ Error: From<M::InitError>,
+ {
+ let mut state = SplitState::new(&command)?;
+
+ self.send_single_command(bar, state.command(command))?;
+
+ while let Some(continuation) = state.next_continuation_record() {
+ dev_dbg!(
+ &self.dev,
+ "GSP RPC: send continuation: size=0x{:x}\n",
+ command_size(&continuation),
+ );
+ // Turbofish needed because the compiler cannot infer M here.
+ self.send_single_command::<ContinuationRecord<'_>>(bar, continuation)?;
+ }
+
+ Ok(())
+ }
+
/// Wait for a message to become available on the message queue.
///
/// This works purely at the transport layer and does not interpret or validate the message
diff --git a/drivers/gpu/nova-core/gsp/commands.rs b/drivers/gpu/nova-core/gsp/commands.rs
index 8f270eca33be..e722c6091ec8 100644
--- a/drivers/gpu/nova-core/gsp/commands.rs
+++ b/drivers/gpu/nova-core/gsp/commands.rs
@@ -4,6 +4,7 @@
array,
convert::Infallible,
ffi::FromBytesUntilNulError,
+ marker::PhantomData,
str::Utf8Error, //
};
@@ -22,13 +23,16 @@
driver::Bar0,
gsp::{
cmdq::{
+ command_size,
Cmdq,
CommandToGsp,
MessageFromGsp, //
},
fw::{
commands::*,
- MsgFunction, //
+ GspMsgElement,
+ MsgFunction,
+ GSP_MSG_QUEUE_ELEMENT_SIZE_MAX, //
},
},
sbuffer::SBufferIter,
@@ -242,3 +246,140 @@ pub(crate) fn get_gsp_info(cmdq: &mut Cmdq, bar: &Bar0) -> Result<GetGspStaticIn
}
}
}
+
+/// The `ContinuationRecord` command.
+pub(crate) struct ContinuationRecord<'a> {
+ data: &'a [u8],
+}
+
+impl<'a> ContinuationRecord<'a> {
+ /// Creates a new `ContinuationRecord` command with the given data.
+ pub(crate) fn new(data: &'a [u8]) -> Self {
+ Self { data }
+ }
+}
+
+impl<'a> CommandToGsp for ContinuationRecord<'a> {
+ const FUNCTION: MsgFunction = MsgFunction::ContinuationRecord;
+ type Command = ();
+ type InitError = Infallible;
+
+ fn init(&self) -> impl Init<Self::Command, Self::InitError> {
+ <()>::init_zeroed()
+ }
+
+ fn variable_payload_len(&self) -> usize {
+ self.data.len()
+ }
+
+ fn init_variable_payload(
+ &self,
+ dst: &mut SBufferIter<core::array::IntoIter<&mut [u8], 2>>,
+ ) -> Result {
+ dst.write_all(self.data)
+ }
+}
+
+/// Wrapper that splits a command across continuation records if needed.
+pub(crate) struct SplitState<C: CommandToGsp> {
+ state: Option<(KVVec<u8>, usize)>,
+ _phantom: PhantomData<C>,
+}
+
+impl<C: CommandToGsp> SplitState<C> {
+ /// Maximum command size that fits in a single queue element.
+ const MAX_CMD_SIZE: usize = GSP_MSG_QUEUE_ELEMENT_SIZE_MAX - size_of::<GspMsgElement>();
+
+ /// Maximum size of the variable payload that can be sent in the main command.
+ const MAX_FIRST_PAYLOAD_SIZE: usize = Self::MAX_CMD_SIZE - size_of::<C::Command>();
+
+ /// Creates a new `SplitState` for the given command.
+ ///
+ /// If the command is too large, it will be split into a main command and some number of
+ /// continuation records.
+ pub(crate) fn new(inner: &C) -> Result<Self> {
+ if command_size(inner) > Self::MAX_CMD_SIZE {
+ let mut staging =
+ KVVec::<u8>::from_elem(0u8, inner.variable_payload_len(), GFP_KERNEL)?;
+ let mut sbuffer = SBufferIter::new_writer([staging.as_mut_slice(), &mut []]);
+ inner.init_variable_payload(&mut sbuffer)?;
+ if !sbuffer.is_empty() {
+ return Err(EIO);
+ }
+ drop(sbuffer);
+
+ Ok(Self {
+ state: Some((staging, Self::MAX_FIRST_PAYLOAD_SIZE)),
+ _phantom: PhantomData,
+ })
+ } else {
+ Ok(Self {
+ state: None,
+ _phantom: PhantomData,
+ })
+ }
+ }
+
+ /// Returns the main command.
+ pub(crate) fn command(&self, inner: C) -> SplitCommand<'_, C> {
+ if let Some((staging, _)) = &self.state {
+ SplitCommand::Split(inner, staging)
+ } else {
+ SplitCommand::Single(inner)
+ }
+ }
+
+ /// Returns the next continuation record, or `None` if there are no more.
+ pub(crate) fn next_continuation_record(&mut self) -> Option<ContinuationRecord<'_>> {
+ let (staging, offset) = self.state.as_mut()?;
+
+ let remaining = staging.len() - *offset;
+ if remaining > 0 {
+ let chunk_size = remaining.min(Self::MAX_CMD_SIZE);
+ let record = ContinuationRecord::new(&staging[*offset..(*offset + chunk_size)]);
+ *offset += chunk_size;
+ Some(record)
+ } else {
+ None
+ }
+ }
+}
+
+/// Wrapper enum that represents either a single command or a split command with its staging buffer.
+pub(crate) enum SplitCommand<'a, C: CommandToGsp> {
+ Single(C),
+ Split(C, &'a [u8]),
+}
+
+impl<'a, C: CommandToGsp> CommandToGsp for SplitCommand<'a, C> {
+ const FUNCTION: MsgFunction = C::FUNCTION;
+ type Command = C::Command;
+ type InitError = C::InitError;
+
+ fn init(&self) -> impl Init<Self::Command, Self::InitError> {
+ match self {
+ SplitCommand::Single(cmd) => cmd.init(),
+ SplitCommand::Split(cmd, _) => cmd.init(),
+ }
+ }
+
+ fn variable_payload_len(&self) -> usize {
+ match self {
+ SplitCommand::Single(cmd) => cmd.variable_payload_len(),
+ SplitCommand::Split(_, _) => SplitState::<C>::MAX_FIRST_PAYLOAD_SIZE,
+ }
+ }
+
+ fn init_variable_payload(
+ &self,
+ dst: &mut SBufferIter<core::array::IntoIter<&mut [u8], 2>>,
+ ) -> Result {
+ match self {
+ SplitCommand::Single(cmd) => cmd.init_variable_payload(dst),
+ SplitCommand::Split(_, staging) => {
+ dst.write_all(&staging[..self.variable_payload_len()])
+ }
+ }
+ }
+}
+
diff --git a/drivers/gpu/nova-core/gsp/fw.rs b/drivers/gpu/nova-core/gsp/fw.rs
index c604f423fff3..1a317dfb0bc9 100644
--- a/drivers/gpu/nova-core/gsp/fw.rs
+++ b/drivers/gpu/nova-core/gsp/fw.rs
@@ -203,6 +203,7 @@ pub(crate) enum MsgFunction {
AllocObject = bindings::NV_VGPU_MSG_FUNCTION_ALLOC_OBJECT,
AllocRoot = bindings::NV_VGPU_MSG_FUNCTION_ALLOC_ROOT,
BindCtxDma = bindings::NV_VGPU_MSG_FUNCTION_BIND_CTX_DMA,
+ ContinuationRecord = bindings::NV_VGPU_MSG_FUNCTION_CONTINUATION_RECORD,
Free = bindings::NV_VGPU_MSG_FUNCTION_FREE,
GetGspStaticInfo = bindings::NV_VGPU_MSG_FUNCTION_GET_GSP_STATIC_INFO,
GetStaticInfo = bindings::NV_VGPU_MSG_FUNCTION_GET_STATIC_INFO,
@@ -238,6 +239,7 @@ fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
MsgFunction::AllocObject => write!(f, "ALLOC_OBJECT"),
MsgFunction::AllocRoot => write!(f, "ALLOC_ROOT"),
MsgFunction::BindCtxDma => write!(f, "BIND_CTX_DMA"),
+ MsgFunction::ContinuationRecord => write!(f, "CONTINUATION_RECORD"),
MsgFunction::Free => write!(f, "FREE"),
MsgFunction::GetGspStaticInfo => write!(f, "GET_GSP_STATIC_INFO"),
MsgFunction::GetStaticInfo => write!(f, "GET_STATIC_INFO"),
@@ -277,6 +279,9 @@ fn try_from(value: u32) -> Result<MsgFunction> {
bindings::NV_VGPU_MSG_FUNCTION_ALLOC_OBJECT => Ok(MsgFunction::AllocObject),
bindings::NV_VGPU_MSG_FUNCTION_ALLOC_ROOT => Ok(MsgFunction::AllocRoot),
bindings::NV_VGPU_MSG_FUNCTION_BIND_CTX_DMA => Ok(MsgFunction::BindCtxDma),
+ bindings::NV_VGPU_MSG_FUNCTION_CONTINUATION_RECORD => {
+ Ok(MsgFunction::ContinuationRecord)
+ }
bindings::NV_VGPU_MSG_FUNCTION_FREE => Ok(MsgFunction::Free),
bindings::NV_VGPU_MSG_FUNCTION_GET_GSP_STATIC_INFO => Ok(MsgFunction::GetGspStaticInfo),
bindings::NV_VGPU_MSG_FUNCTION_GET_STATIC_INFO => Ok(MsgFunction::GetStaticInfo),
--
2.53.0
^ permalink raw reply related [flat|nested] 24+ messages in thread* Claude review: gpu: nova-core: gsp: support large RPCs via continuation record
2026-02-19 7:30 ` [PATCH v2 8/9] gpu: nova-core: gsp: support large RPCs via continuation record Eliot Courtney
@ 2026-02-19 8:43 ` Claude Code Review Bot
0 siblings, 0 replies; 24+ messages in thread
From: Claude Code Review Bot @ 2026-02-19 8:43 UTC (permalink / raw)
To: dri-devel-reviews
Patch Review
This is the main patch. A few observations:
> +impl<C: CommandToGsp> SplitState<C> {
> + const MAX_CMD_SIZE: usize = GSP_MSG_QUEUE_ELEMENT_SIZE_MAX - size_of::<GspMsgElement>();
> + const MAX_FIRST_PAYLOAD_SIZE: usize = Self::MAX_CMD_SIZE - size_of::<C::Command>();
`MAX_FIRST_PAYLOAD_SIZE` could underflow if `size_of::<C::Command>() > MAX_CMD_SIZE`. In practice this would mean a command header larger than ~64KB which seems impossible for any real GSP command, and `SplitState` is only instantiated with concrete types the driver controls, so this is not a real concern.
> + pub(crate) fn new(inner: &C) -> Result<Self> {
> + if command_size(inner) > Self::MAX_CMD_SIZE {
> + let mut staging =
> + KVVec::<u8>::from_elem(0u8, inner.variable_payload_len(), GFP_KERNEL)?;
> + let mut sbuffer = SBufferIter::new_writer([staging.as_mut_slice(), &mut []]);
> + inner.init_variable_payload(&mut sbuffer)?;
> + if !sbuffer.is_empty() {
> + return Err(EIO);
> + }
> + drop(sbuffer);
> +
> + Ok(Self {
> + state: Some((staging, Self::MAX_FIRST_PAYLOAD_SIZE)),
> + _phantom: PhantomData,
> + })
When a split is needed, the entire variable payload is serialized into a staging buffer. This means `init_variable_payload` is called once during `SplitState::new`, and then the staging buffer is read from when writing the main command and continuation records. This avoids calling `init_variable_payload` multiple times, which is correct.
> + pub(crate) fn command(&self, inner: C) -> SplitCommand<'_, C> {
> + if let Some((staging, _)) = &self.state {
> + SplitCommand::Split(inner, staging)
> + } else {
> + SplitCommand::Single(inner)
> + }
> + }
`command()` takes `&self` but consumes `inner`. This means after calling `command()`, the caller no longer has the original command, but `SplitState` still holds the staging buffer for continuation records. The borrow relationships look correct.
> + pub(crate) fn next_continuation_record(&mut self) -> Option<ContinuationRecord<'_>> {
> + let (staging, offset) = self.state.as_mut()?;
> +
> + let remaining = staging.len() - *offset;
> + if remaining > 0 {
> + let chunk_size = remaining.min(Self::MAX_CMD_SIZE);
> + let record = ContinuationRecord::new(&staging[*offset..(*offset + chunk_size)]);
> + *offset += chunk_size;
> + Some(record)
> + } else {
> + None
> + }
> + }
The staging buffer length is `inner.variable_payload_len()` and the initial offset is `MAX_FIRST_PAYLOAD_SIZE`. Could `MAX_FIRST_PAYLOAD_SIZE > staging.len()` (i.e. `variable_payload_len()`)? That would cause `remaining` to underflow (panic in debug, wrap in release). Let's check: we only enter the split path when `command_size(inner) > MAX_CMD_SIZE`, which means `size_of::<C::Command>() + variable_payload_len() > MAX_CMD_SIZE`, so `variable_payload_len() > MAX_CMD_SIZE - size_of::<C::Command>() = MAX_FIRST_PAYLOAD_SIZE`. So the staging buffer is always strictly larger than `MAX_FIRST_PAYLOAD_SIZE`, meaning `remaining` is always at least 1 on the first call. No underflow.
> + fn variable_payload_len(&self) -> usize {
> + match self {
> + SplitCommand::Single(cmd) => cmd.variable_payload_len(),
> + SplitCommand::Split(_, _) => SplitState::<C>::MAX_FIRST_PAYLOAD_SIZE,
> + }
> + }
For the `Split` variant, the variable payload is exactly `MAX_FIRST_PAYLOAD_SIZE` bytes, which is the truncated first chunk of the full payload. This means `send_single_command` will allocate `size_of::<C::Command>() + MAX_FIRST_PAYLOAD_SIZE = MAX_CMD_SIZE`, which fits within one element. Correct.
> + fn init_variable_payload(
> + &self,
> + dst: &mut SBufferIter<core::array::IntoIter<&mut [u8], 2>>,
> + ) -> Result {
> + match self {
> + SplitCommand::Single(cmd) => cmd.init_variable_payload(dst),
> + SplitCommand::Split(_, staging) => {
> + dst.write_all(&staging[..self.variable_payload_len()])
> + }
> + }
> + }
For `Split`, this writes the first `MAX_FIRST_PAYLOAD_SIZE` bytes of the staging buffer. `self.variable_payload_len()` calls back into the match and returns `MAX_FIRST_PAYLOAD_SIZE`. The `SBufferIter` was constructed with capacity `MAX_FIRST_PAYLOAD_SIZE`, so after `write_all` it should be exactly empty, passing the `is_empty` check in `send_single_command`. Correct.
In `send_command`:
> + let mut state = SplitState::new(&command)?;
> +
> + self.send_single_command(bar, state.command(command))?;
`SplitState::new` takes `&command` and `state.command` takes `command` by value. If the command is not split, the original `command` is passed through to `send_single_command` without staging. If it is split, the staging buffer was already populated in `new`, and `command` is moved into `SplitCommand::Split` where its `init()` method is still called but its `init_variable_payload` is not (the staging buffer is used instead). This is correct.
One minor observation: the `SplitState` is declared `mut` but is only mutated during `next_continuation_record`. For the non-split path, `state` is not mutated at all. This is fine; the compiler would optimize it.
No bugs found in this patch.
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 24+ messages in thread