[PATCH 0/4] gpu: nova-core: Fixups for GSP message queue and bindings

[PATCH 0/4] gpu: nova-core: Fixups for GSP message queue and bindings

[Alexandre Courbot]

This series contains a few fixups for the recently merged GSP
command-queue code, by order of importance:

- Some explicit padding required to safely implement `AsBytes` was
  missing in the bindings,
- A bug in the received message length calculation results in the
  message handler being given more data than it should, 
- `MaybeZeroable` is now derived by the kernel's bindgen, but the Nova
  bindings have not been updated for that,
- Some items in the bindings were referred to using the version module
  directly, instead of the alias we defined to limit the diff when we
  upgrade firmware versions.

All of them address "bugs" (with the first two fixing actual correctness
issues), but since Nova does not do much anyway, they are also not
absolutely critical and can wait -rc1 if we prefer to do so.

Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
Alexandre Courbot (4):
      gpu: nova-core: bindings: Add missing explicit padding
      gpu: nova-core: gsp: Fix length of received messages
      gpu: nova-core: bindings: Derive `MaybeZeroable`
      gpu: nova-core: gsp: Replace firmware version with "bindings" alias

 drivers/gpu/nova-core/gsp/cmdq.rs                 |  11 ++-
 drivers/gpu/nova-core/gsp/fw.rs                   |  67 +++++++-------
 drivers/gpu/nova-core/gsp/fw/r570_144.rs          |  11 ++-
 drivers/gpu/nova-core/gsp/fw/r570_144/bindings.rs | 105 ++++++++++++----------
 4 files changed, 103 insertions(+), 91 deletions(-)
---
base-commit: 57dc2ea0b7bdb828c5d966d9135c28fe854933a4
change-id: 20251121-nova-fixes-dc9b4f17b90e

Best regards,
-- 
Alexandre Courbot <acourbot@nvidia.com>

[PATCH 1/4] gpu: nova-core: bindings: Add missing explicit padding

[Alexandre Courbot]

Explicit padding is needed in order to avoid uninitialized bytes and
safely implement `AsBytes`. The `--explicit-padding` of bindgen was
omitted by mistake when these bindings were generated.

Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
 drivers/gpu/nova-core/gsp/fw/r570_144/bindings.rs | 12 ++++++++++++
 1 file changed, 12 insertions(+)

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 5bcfbcd1ad22..5f0569dcc4a0 100644
--- a/drivers/gpu/nova-core/gsp/fw/r570_144/bindings.rs
+++ b/drivers/gpu/nova-core/gsp/fw/r570_144/bindings.rs
@@ -325,6 +325,7 @@ pub struct NV0080_CTRL_GPU_GET_SRIOV_CAPS_PARAMS {
     pub totalVFs: u32_,
     pub firstVfOffset: u32_,
     pub vfFeatureMask: u32_,
+    pub __bindgen_padding_0: [u8; 4usize],
     pub FirstVFBar0Address: u64_,
     pub FirstVFBar1Address: u64_,
     pub FirstVFBar2Address: u64_,
@@ -340,6 +341,7 @@ pub struct NV0080_CTRL_GPU_GET_SRIOV_CAPS_PARAMS {
     pub bClientRmAllocatedCtxBuffer: u8_,
     pub bNonPowerOf2ChannelCountSupported: u8_,
     pub bVfResizableBAR1Supported: u8_,
+    pub __bindgen_padding_1: [u8; 7usize],
 }
 #[repr(C)]
 #[derive(Debug, Default, Copy, Clone)]
@@ -347,11 +349,13 @@ pub struct NV2080_CTRL_BIOS_GET_SKU_INFO_PARAMS {
     pub BoardID: u32_,
     pub chipSKU: [ffi::c_char; 9usize],
     pub chipSKUMod: [ffi::c_char; 5usize],
+    pub __bindgen_padding_0: [u8; 2usize],
     pub skuConfigVersion: u32_,
     pub project: [ffi::c_char; 5usize],
     pub projectSKU: [ffi::c_char; 5usize],
     pub CDP: [ffi::c_char; 6usize],
     pub projectSKUMod: [ffi::c_char; 2usize],
+    pub __bindgen_padding_1: [u8; 2usize],
     pub businessCycle: u32_,
 }
 pub type NV2080_CTRL_CMD_FB_GET_FB_REGION_SURFACE_MEM_TYPE_FLAG = [u8_; 17usize];
@@ -371,6 +375,7 @@ pub struct NV2080_CTRL_CMD_FB_GET_FB_REGION_FB_REGION_INFO {
 #[derive(Debug, Default, Copy, Clone)]
 pub struct NV2080_CTRL_CMD_FB_GET_FB_REGION_INFO_PARAMS {
     pub numFBRegions: u32_,
+    pub __bindgen_padding_0: [u8; 4usize],
     pub fbRegion: [NV2080_CTRL_CMD_FB_GET_FB_REGION_FB_REGION_INFO; 16usize],
 }
 #[repr(C)]
@@ -495,13 +500,16 @@ pub struct FW_WPR_LAYOUT_OFFSET {
 #[derive(Debug, Copy, Clone)]
 pub struct GspStaticConfigInfo_t {
     pub grCapsBits: [u8_; 23usize],
+    pub __bindgen_padding_0: u8,
     pub gidInfo: NV2080_CTRL_GPU_GET_GID_INFO_PARAMS,
     pub SKUInfo: NV2080_CTRL_BIOS_GET_SKU_INFO_PARAMS,
+    pub __bindgen_padding_1: [u8; 4usize],
     pub fbRegionInfoParams: NV2080_CTRL_CMD_FB_GET_FB_REGION_INFO_PARAMS,
     pub sriovCaps: NV0080_CTRL_GPU_GET_SRIOV_CAPS_PARAMS,
     pub sriovMaxGfid: u32_,
     pub engineCaps: [u32_; 3usize],
     pub poisonFuseEnabled: u8_,
+    pub __bindgen_padding_2: [u8; 7usize],
     pub fb_length: u64_,
     pub fbio_mask: u64_,
     pub fb_bus_width: u32_,
@@ -527,16 +535,20 @@ pub struct GspStaticConfigInfo_t {
     pub bIsMigSupported: u8_,
     pub RTD3GC6TotalBoardPower: u16_,
     pub RTD3GC6PerstDelay: u16_,
+    pub __bindgen_padding_3: [u8; 2usize],
     pub bar1PdeBase: u64_,
     pub bar2PdeBase: u64_,
     pub bVbiosValid: u8_,
+    pub __bindgen_padding_4: [u8; 3usize],
     pub vbiosSubVendor: u32_,
     pub vbiosSubDevice: u32_,
     pub bPageRetirementSupported: u8_,
     pub bSplitVasBetweenServerClientRm: u8_,
     pub bClRootportNeedsNosnoopWAR: u8_,
+    pub __bindgen_padding_5: u8,
     pub displaylessMaxHeads: VIRTUAL_DISPLAY_GET_NUM_HEADS_PARAMS,
     pub displaylessMaxResolution: VIRTUAL_DISPLAY_GET_MAX_RESOLUTION_PARAMS,
+    pub __bindgen_padding_6: [u8; 4usize],
     pub displaylessMaxPixels: u64_,
     pub hInternalClient: u32_,
     pub hInternalDevice: u32_,

-- 
2.51.2

[PATCH 2/4] gpu: nova-core: gsp: Fix length of received messages

[Alexandre Courbot]

The size of messages' payload is miscalculated, leading to extra data
passed to the message handler. While this is not a problem with our
current set of commands, others with a variable-length payload may
misbehave. Fix this.

Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
 drivers/gpu/nova-core/gsp/cmdq.rs | 11 +++++++----
 drivers/gpu/nova-core/gsp/fw.rs   |  2 +-
 2 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/nova-core/gsp/cmdq.rs b/drivers/gpu/nova-core/gsp/cmdq.rs
index 6f946d14868a..dab73377c526 100644
--- a/drivers/gpu/nova-core/gsp/cmdq.rs
+++ b/drivers/gpu/nova-core/gsp/cmdq.rs
@@ -588,21 +588,24 @@ fn wait_for_msg(&self, timeout: Delta) -> Result<GspMessage<'_>> {
             header.length(),
         );
 
+        // The length of the message that follows the header.
+        let msg_length = header.length() - size_of::<GspMsgElement>();
+
         // Check that the driver read area is large enough for the message.
-        if slice_1.len() + slice_2.len() < header.length() {
+        if slice_1.len() + slice_2.len() < msg_length {
             return Err(EIO);
         }
 
         // Cut the message slices down to the actual length of the message.
-        let (slice_1, slice_2) = if slice_1.len() > header.length() {
+        let (slice_1, slice_2) = if slice_1.len() > msg_length {
             // PANIC: we checked above that `slice_1` is at least as long as `msg_header.length()`.
-            (slice_1.split_at(header.length()).0, &slice_2[0..0])
+            (slice_1.split_at(msg_length).0, &slice_2[0..0])
         } else {
             (
                 slice_1,
                 // PANIC: we checked above that `slice_1.len() + slice_2.len()` is at least as
                 // large as `msg_header.length()`.
-                slice_2.split_at(header.length() - slice_1.len()).0,
+                slice_2.split_at(msg_length - slice_1.len()).0,
             )
         };
 
diff --git a/drivers/gpu/nova-core/gsp/fw.rs b/drivers/gpu/nova-core/gsp/fw.rs
index abffd6beec65..7fcba5afb0a3 100644
--- a/drivers/gpu/nova-core/gsp/fw.rs
+++ b/drivers/gpu/nova-core/gsp/fw.rs
@@ -853,7 +853,7 @@ pub(crate) fn set_checksum(&mut self, checksum: u32) {
         self.inner.checkSum = checksum;
     }
 
-    /// Returns the total length of the message.
+    /// Returns the total length of the message, message and RPC headers included.
     pub(crate) fn length(&self) -> usize {
         // `rpc.length` includes the length of the GspRpcHeader but not the message header.
         size_of::<Self>() - size_of::<bindings::rpc_message_header_v>()

-- 
2.51.2

[PATCH 3/4] gpu: nova-core: bindings: Derive `MaybeZeroable`

[Alexandre Courbot]

Commit 4846300ba8f9 ("rust: derive `Zeroable` for all structs & unions
generated by bindgen where possible") automatically derives
`MaybeZeroable` for all bindings. This is better than selectively
deriving `Zeroable` as it ensures all types that can implement
`Zeroable` do.

Regenerate the nova-core bindings so they benefit from this, and remove
a now unneeded implementation of `Zeroable`.

Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
 drivers/gpu/nova-core/gsp/fw.rs                   |  7 --
 drivers/gpu/nova-core/gsp/fw/r570_144.rs          | 11 ++-
 drivers/gpu/nova-core/gsp/fw/r570_144/bindings.rs | 93 ++++++++++++-----------
 3 files changed, 54 insertions(+), 57 deletions(-)

diff --git a/drivers/gpu/nova-core/gsp/fw.rs b/drivers/gpu/nova-core/gsp/fw.rs
index 7fcba5afb0a3..252755dbb73c 100644
--- a/drivers/gpu/nova-core/gsp/fw.rs
+++ b/drivers/gpu/nova-core/gsp/fw.rs
@@ -797,13 +797,6 @@ fn init(cmd_size: usize, function: MsgFunction) -> impl Init<Self, Error> {
     }
 }
 
-// SAFETY: We can't derive the Zeroable trait for this binding because the
-// procedural macro doesn't support the syntax used by bindgen to create the
-// __IncompleteArrayField types. So instead we implement it here, which is safe
-// because these are explicitly padded structures only containing types for
-// which any bit pattern, including all zeros, is valid.
-unsafe impl Zeroable for bindings::rpc_message_header_v {}
-
 /// GSP Message Element.
 ///
 /// This is essentially a message header expected to be followed by the message data.
diff --git a/drivers/gpu/nova-core/gsp/fw/r570_144.rs b/drivers/gpu/nova-core/gsp/fw/r570_144.rs
index 048234d1a9d1..e99d315ae74c 100644
--- a/drivers/gpu/nova-core/gsp/fw/r570_144.rs
+++ b/drivers/gpu/nova-core/gsp/fw/r570_144.rs
@@ -24,8 +24,11 @@
     unreachable_pub,
     unsafe_op_in_unsafe_fn
 )]
-use kernel::{
-    ffi,
-    prelude::Zeroable, //
-};
+use kernel::ffi;
+use pin_init::MaybeZeroable;
+
 include!("r570_144/bindings.rs");
+
+// SAFETY: This type has a size of zero, so its inclusion into another type should not affect their
+// ability to implement `Zeroable`.
+unsafe impl<T> kernel::prelude::Zeroable for __IncompleteArrayField<T> {}
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 5f0569dcc4a0..6d25fe0bffa9 100644
--- a/drivers/gpu/nova-core/gsp/fw/r570_144/bindings.rs
+++ b/drivers/gpu/nova-core/gsp/fw/r570_144/bindings.rs
@@ -320,7 +320,7 @@ fn fmt(&self, fmt: &mut ::core::fmt::Formatter<'_>) -> ::core::fmt::Result {
 pub const NV_VGPU_MSG_EVENT_NUM_EVENTS: _bindgen_ty_3 = 4131;
 pub type _bindgen_ty_3 = ffi::c_uint;
 #[repr(C)]
-#[derive(Debug, Default, Copy, Clone)]
+#[derive(Debug, Default, Copy, Clone, MaybeZeroable)]
 pub struct NV0080_CTRL_GPU_GET_SRIOV_CAPS_PARAMS {
     pub totalVFs: u32_,
     pub firstVfOffset: u32_,
@@ -344,7 +344,7 @@ pub struct NV0080_CTRL_GPU_GET_SRIOV_CAPS_PARAMS {
     pub __bindgen_padding_1: [u8; 7usize],
 }
 #[repr(C)]
-#[derive(Debug, Default, Copy, Clone)]
+#[derive(Debug, Default, Copy, Clone, MaybeZeroable)]
 pub struct NV2080_CTRL_BIOS_GET_SKU_INFO_PARAMS {
     pub BoardID: u32_,
     pub chipSKU: [ffi::c_char; 9usize],
@@ -360,7 +360,7 @@ pub struct NV2080_CTRL_BIOS_GET_SKU_INFO_PARAMS {
 }
 pub type NV2080_CTRL_CMD_FB_GET_FB_REGION_SURFACE_MEM_TYPE_FLAG = [u8_; 17usize];
 #[repr(C)]
-#[derive(Debug, Default, Copy, Clone)]
+#[derive(Debug, Default, Copy, Clone, MaybeZeroable)]
 pub struct NV2080_CTRL_CMD_FB_GET_FB_REGION_FB_REGION_INFO {
     pub base: u64_,
     pub limit: u64_,
@@ -372,14 +372,14 @@ pub struct NV2080_CTRL_CMD_FB_GET_FB_REGION_FB_REGION_INFO {
     pub blackList: NV2080_CTRL_CMD_FB_GET_FB_REGION_SURFACE_MEM_TYPE_FLAG,
 }
 #[repr(C)]
-#[derive(Debug, Default, Copy, Clone)]
+#[derive(Debug, Default, Copy, Clone, MaybeZeroable)]
 pub struct NV2080_CTRL_CMD_FB_GET_FB_REGION_INFO_PARAMS {
     pub numFBRegions: u32_,
     pub __bindgen_padding_0: [u8; 4usize],
     pub fbRegion: [NV2080_CTRL_CMD_FB_GET_FB_REGION_FB_REGION_INFO; 16usize],
 }
 #[repr(C)]
-#[derive(Debug, Copy, Clone)]
+#[derive(Debug, Copy, Clone, MaybeZeroable)]
 pub struct NV2080_CTRL_GPU_GET_GID_INFO_PARAMS {
     pub index: u32_,
     pub flags: u32_,
@@ -396,14 +396,14 @@ fn default() -> Self {
     }
 }
 #[repr(C)]
-#[derive(Debug, Default, Copy, Clone, Zeroable)]
+#[derive(Debug, Default, Copy, Clone, MaybeZeroable)]
 pub struct DOD_METHOD_DATA {
     pub status: u32_,
     pub acpiIdListLen: u32_,
     pub acpiIdList: [u32_; 16usize],
 }
 #[repr(C)]
-#[derive(Debug, Default, Copy, Clone, Zeroable)]
+#[derive(Debug, Default, Copy, Clone, MaybeZeroable)]
 pub struct JT_METHOD_DATA {
     pub status: u32_,
     pub jtCaps: u32_,
@@ -412,14 +412,14 @@ pub struct JT_METHOD_DATA {
     pub __bindgen_padding_0: u8,
 }
 #[repr(C)]
-#[derive(Debug, Default, Copy, Clone, Zeroable)]
+#[derive(Debug, Default, Copy, Clone, MaybeZeroable)]
 pub struct MUX_METHOD_DATA_ELEMENT {
     pub acpiId: u32_,
     pub mode: u32_,
     pub status: u32_,
 }
 #[repr(C)]
-#[derive(Debug, Default, Copy, Clone, Zeroable)]
+#[derive(Debug, Default, Copy, Clone, MaybeZeroable)]
 pub struct MUX_METHOD_DATA {
     pub tableLen: u32_,
     pub acpiIdMuxModeTable: [MUX_METHOD_DATA_ELEMENT; 16usize],
@@ -427,13 +427,13 @@ pub struct MUX_METHOD_DATA {
     pub acpiIdMuxStateTable: [MUX_METHOD_DATA_ELEMENT; 16usize],
 }
 #[repr(C)]
-#[derive(Debug, Default, Copy, Clone, Zeroable)]
+#[derive(Debug, Default, Copy, Clone, MaybeZeroable)]
 pub struct CAPS_METHOD_DATA {
     pub status: u32_,
     pub optimusCaps: u32_,
 }
 #[repr(C)]
-#[derive(Debug, Default, Copy, Clone, Zeroable)]
+#[derive(Debug, Default, Copy, Clone, MaybeZeroable)]
 pub struct ACPI_METHOD_DATA {
     pub bValid: u8_,
     pub __bindgen_padding_0: [u8; 3usize],
@@ -443,20 +443,20 @@ pub struct ACPI_METHOD_DATA {
     pub capsMethodData: CAPS_METHOD_DATA,
 }
 #[repr(C)]
-#[derive(Debug, Default, Copy, Clone)]
+#[derive(Debug, Default, Copy, Clone, MaybeZeroable)]
 pub struct VIRTUAL_DISPLAY_GET_MAX_RESOLUTION_PARAMS {
     pub headIndex: u32_,
     pub maxHResolution: u32_,
     pub maxVResolution: u32_,
 }
 #[repr(C)]
-#[derive(Debug, Default, Copy, Clone)]
+#[derive(Debug, Default, Copy, Clone, MaybeZeroable)]
 pub struct VIRTUAL_DISPLAY_GET_NUM_HEADS_PARAMS {
     pub numHeads: u32_,
     pub maxNumHeads: u32_,
 }
 #[repr(C)]
-#[derive(Debug, Default, Copy, Clone, Zeroable)]
+#[derive(Debug, Default, Copy, Clone, MaybeZeroable)]
 pub struct BUSINFO {
     pub deviceID: u16_,
     pub vendorID: u16_,
@@ -466,7 +466,7 @@ pub struct BUSINFO {
     pub __bindgen_padding_0: u8,
 }
 #[repr(C)]
-#[derive(Debug, Default, Copy, Clone, Zeroable)]
+#[derive(Debug, Default, Copy, Clone, MaybeZeroable)]
 pub struct GSP_VF_INFO {
     pub totalVFs: u32_,
     pub firstVFOffset: u32_,
@@ -479,25 +479,25 @@ pub struct GSP_VF_INFO {
     pub __bindgen_padding_0: [u8; 5usize],
 }
 #[repr(C)]
-#[derive(Debug, Default, Copy, Clone, Zeroable)]
+#[derive(Debug, Default, Copy, Clone, MaybeZeroable)]
 pub struct GSP_PCIE_CONFIG_REG {
     pub linkCap: u32_,
 }
 #[repr(C)]
-#[derive(Debug, Default, Copy, Clone)]
+#[derive(Debug, Default, Copy, Clone, MaybeZeroable)]
 pub struct EcidManufacturingInfo {
     pub ecidLow: u32_,
     pub ecidHigh: u32_,
     pub ecidExtended: u32_,
 }
 #[repr(C)]
-#[derive(Debug, Default, Copy, Clone)]
+#[derive(Debug, Default, Copy, Clone, MaybeZeroable)]
 pub struct FW_WPR_LAYOUT_OFFSET {
     pub nonWprHeapOffset: u64_,
     pub frtsOffset: u64_,
 }
 #[repr(C)]
-#[derive(Debug, Copy, Clone)]
+#[derive(Debug, Copy, Clone, MaybeZeroable)]
 pub struct GspStaticConfigInfo_t {
     pub grCapsBits: [u8_; 23usize],
     pub __bindgen_padding_0: u8,
@@ -570,7 +570,7 @@ fn default() -> Self {
     }
 }
 #[repr(C)]
-#[derive(Debug, Default, Copy, Clone, Zeroable)]
+#[derive(Debug, Default, Copy, Clone, MaybeZeroable)]
 pub struct GspSystemInfo {
     pub gpuPhysAddr: u64_,
     pub gpuPhysFbAddr: u64_,
@@ -627,7 +627,7 @@ pub struct GspSystemInfo {
     pub hostPageSize: u64_,
 }
 #[repr(C)]
-#[derive(Debug, Default, Copy, Clone, Zeroable)]
+#[derive(Debug, Default, Copy, Clone, MaybeZeroable)]
 pub struct MESSAGE_QUEUE_INIT_ARGUMENTS {
     pub sharedMemPhysAddr: u64_,
     pub pageTableEntryCount: u32_,
@@ -636,7 +636,7 @@ pub struct MESSAGE_QUEUE_INIT_ARGUMENTS {
     pub statQueueOffset: u64_,
 }
 #[repr(C)]
-#[derive(Debug, Default, Copy, Clone, Zeroable)]
+#[derive(Debug, Default, Copy, Clone, MaybeZeroable)]
 pub struct GSP_SR_INIT_ARGUMENTS {
     pub oldLevel: u32_,
     pub flags: u32_,
@@ -644,7 +644,7 @@ pub struct GSP_SR_INIT_ARGUMENTS {
     pub __bindgen_padding_0: [u8; 3usize],
 }
 #[repr(C)]
-#[derive(Debug, Default, Copy, Clone, Zeroable)]
+#[derive(Debug, Default, Copy, Clone, MaybeZeroable)]
 pub struct GSP_ARGUMENTS_CACHED {
     pub messageQueueInitArguments: MESSAGE_QUEUE_INIT_ARGUMENTS,
     pub srInitArguments: GSP_SR_INIT_ARGUMENTS,
@@ -654,13 +654,13 @@ pub struct GSP_ARGUMENTS_CACHED {
     pub profilerArgs: GSP_ARGUMENTS_CACHED__bindgen_ty_1,
 }
 #[repr(C)]
-#[derive(Debug, Default, Copy, Clone, Zeroable)]
+#[derive(Debug, Default, Copy, Clone, MaybeZeroable)]
 pub struct GSP_ARGUMENTS_CACHED__bindgen_ty_1 {
     pub pa: u64_,
     pub size: u64_,
 }
 #[repr(C)]
-#[derive(Copy, Clone, Zeroable)]
+#[derive(Copy, Clone, MaybeZeroable)]
 pub union rpc_message_rpc_union_field_v03_00 {
     pub spare: u32_,
     pub cpuRmGfid: u32_,
@@ -676,6 +676,7 @@ fn default() -> Self {
 }
 pub type rpc_message_rpc_union_field_v = rpc_message_rpc_union_field_v03_00;
 #[repr(C)]
+#[derive(MaybeZeroable)]
 pub struct rpc_message_header_v03_00 {
     pub header_version: u32_,
     pub signature: u32_,
@@ -698,7 +699,7 @@ fn default() -> Self {
 }
 pub type rpc_message_header_v = rpc_message_header_v03_00;
 #[repr(C)]
-#[derive(Copy, Clone, Zeroable)]
+#[derive(Copy, Clone, MaybeZeroable)]
 pub struct GspFwWprMeta {
     pub magic: u64_,
     pub revision: u64_,
@@ -733,19 +734,19 @@ pub struct GspFwWprMeta {
     pub verified: u64_,
 }
 #[repr(C)]
-#[derive(Copy, Clone, Zeroable)]
+#[derive(Copy, Clone, MaybeZeroable)]
 pub union GspFwWprMeta__bindgen_ty_1 {
     pub __bindgen_anon_1: GspFwWprMeta__bindgen_ty_1__bindgen_ty_1,
     pub __bindgen_anon_2: GspFwWprMeta__bindgen_ty_1__bindgen_ty_2,
 }
 #[repr(C)]
-#[derive(Debug, Default, Copy, Clone, Zeroable)]
+#[derive(Debug, Default, Copy, Clone, MaybeZeroable)]
 pub struct GspFwWprMeta__bindgen_ty_1__bindgen_ty_1 {
     pub sysmemAddrOfSignature: u64_,
     pub sizeOfSignature: u64_,
 }
 #[repr(C)]
-#[derive(Debug, Default, Copy, Clone, Zeroable)]
+#[derive(Debug, Default, Copy, Clone, MaybeZeroable)]
 pub struct GspFwWprMeta__bindgen_ty_1__bindgen_ty_2 {
     pub gspFwHeapFreeListWprOffset: u32_,
     pub unused0: u32_,
@@ -761,13 +762,13 @@ fn default() -> Self {
     }
 }
 #[repr(C)]
-#[derive(Copy, Clone, Zeroable)]
+#[derive(Copy, Clone, MaybeZeroable)]
 pub union GspFwWprMeta__bindgen_ty_2 {
     pub __bindgen_anon_1: GspFwWprMeta__bindgen_ty_2__bindgen_ty_1,
     pub __bindgen_anon_2: GspFwWprMeta__bindgen_ty_2__bindgen_ty_2,
 }
 #[repr(C)]
-#[derive(Debug, Default, Copy, Clone, Zeroable)]
+#[derive(Debug, Default, Copy, Clone, MaybeZeroable)]
 pub struct GspFwWprMeta__bindgen_ty_2__bindgen_ty_1 {
     pub partitionRpcAddr: u64_,
     pub partitionRpcRequestOffset: u16_,
@@ -779,7 +780,7 @@ pub struct GspFwWprMeta__bindgen_ty_2__bindgen_ty_1 {
     pub lsUcodeVersion: u32_,
 }
 #[repr(C)]
-#[derive(Debug, Default, Copy, Clone, Zeroable)]
+#[derive(Debug, Default, Copy, Clone, MaybeZeroable)]
 pub struct GspFwWprMeta__bindgen_ty_2__bindgen_ty_2 {
     pub partitionRpcPadding: [u32_; 4usize],
     pub sysmemAddrOfCrashReportQueue: u64_,
@@ -814,7 +815,7 @@ fn default() -> Self {
 pub const LibosMemoryRegionLoc_LIBOS_MEMORY_REGION_LOC_FB: LibosMemoryRegionLoc = 2;
 pub type LibosMemoryRegionLoc = ffi::c_uint;
 #[repr(C)]
-#[derive(Debug, Default, Copy, Clone, Zeroable)]
+#[derive(Debug, Default, Copy, Clone, MaybeZeroable)]
 pub struct LibosMemoryRegionInitArgument {
     pub id8: LibosAddress,
     pub pa: LibosAddress,
@@ -824,7 +825,7 @@ pub struct LibosMemoryRegionInitArgument {
     pub __bindgen_padding_0: [u8; 6usize],
 }
 #[repr(C)]
-#[derive(Debug, Default, Copy, Clone)]
+#[derive(Debug, Default, Copy, Clone, MaybeZeroable)]
 pub struct PACKED_REGISTRY_ENTRY {
     pub nameOffset: u32_,
     pub type_: u8_,
@@ -833,14 +834,14 @@ pub struct PACKED_REGISTRY_ENTRY {
     pub length: u32_,
 }
 #[repr(C)]
-#[derive(Debug, Default)]
+#[derive(Debug, Default, MaybeZeroable)]
 pub struct PACKED_REGISTRY_TABLE {
     pub size: u32_,
     pub numEntries: u32_,
     pub entries: __IncompleteArrayField<PACKED_REGISTRY_ENTRY>,
 }
 #[repr(C)]
-#[derive(Debug, Default, Copy, Clone, Zeroable)]
+#[derive(Debug, Default, Copy, Clone, MaybeZeroable)]
 pub struct msgqTxHeader {
     pub version: u32_,
     pub size: u32_,
@@ -852,13 +853,13 @@ pub struct msgqTxHeader {
     pub entryOff: u32_,
 }
 #[repr(C)]
-#[derive(Debug, Default, Copy, Clone, Zeroable)]
+#[derive(Debug, Default, Copy, Clone, MaybeZeroable)]
 pub struct msgqRxHeader {
     pub readPtr: u32_,
 }
 #[repr(C)]
 #[repr(align(8))]
-#[derive(Zeroable)]
+#[derive(MaybeZeroable)]
 pub struct GSP_MSG_QUEUE_ELEMENT {
     pub authTagBuffer: [u8_; 16usize],
     pub aadBuffer: [u8_; 16usize],
@@ -878,7 +879,7 @@ fn default() -> Self {
     }
 }
 #[repr(C)]
-#[derive(Debug, Default)]
+#[derive(Debug, Default, MaybeZeroable)]
 pub struct rpc_run_cpu_sequencer_v17_00 {
     pub bufferSizeDWord: u32_,
     pub cmdIndex: u32_,
@@ -896,20 +897,20 @@ pub struct rpc_run_cpu_sequencer_v17_00 {
 pub const GSP_SEQ_BUF_OPCODE_GSP_SEQ_BUF_OPCODE_CORE_RESUME: GSP_SEQ_BUF_OPCODE = 8;
 pub type GSP_SEQ_BUF_OPCODE = ffi::c_uint;
 #[repr(C)]
-#[derive(Debug, Default, Copy, Clone)]
+#[derive(Debug, Default, Copy, Clone, MaybeZeroable)]
 pub struct GSP_SEQ_BUF_PAYLOAD_REG_WRITE {
     pub addr: u32_,
     pub val: u32_,
 }
 #[repr(C)]
-#[derive(Debug, Default, Copy, Clone)]
+#[derive(Debug, Default, Copy, Clone, MaybeZeroable)]
 pub struct GSP_SEQ_BUF_PAYLOAD_REG_MODIFY {
     pub addr: u32_,
     pub mask: u32_,
     pub val: u32_,
 }
 #[repr(C)]
-#[derive(Debug, Default, Copy, Clone)]
+#[derive(Debug, Default, Copy, Clone, MaybeZeroable)]
 pub struct GSP_SEQ_BUF_PAYLOAD_REG_POLL {
     pub addr: u32_,
     pub mask: u32_,
@@ -918,24 +919,24 @@ pub struct GSP_SEQ_BUF_PAYLOAD_REG_POLL {
     pub error: u32_,
 }
 #[repr(C)]
-#[derive(Debug, Default, Copy, Clone)]
+#[derive(Debug, Default, Copy, Clone, MaybeZeroable)]
 pub struct GSP_SEQ_BUF_PAYLOAD_DELAY_US {
     pub val: u32_,
 }
 #[repr(C)]
-#[derive(Debug, Default, Copy, Clone)]
+#[derive(Debug, Default, Copy, Clone, MaybeZeroable)]
 pub struct GSP_SEQ_BUF_PAYLOAD_REG_STORE {
     pub addr: u32_,
     pub index: u32_,
 }
 #[repr(C)]
-#[derive(Copy, Clone)]
+#[derive(Copy, Clone, MaybeZeroable)]
 pub struct GSP_SEQUENCER_BUFFER_CMD {
     pub opCode: GSP_SEQ_BUF_OPCODE,
     pub payload: GSP_SEQUENCER_BUFFER_CMD__bindgen_ty_1,
 }
 #[repr(C)]
-#[derive(Copy, Clone)]
+#[derive(Copy, Clone, MaybeZeroable)]
 pub union GSP_SEQUENCER_BUFFER_CMD__bindgen_ty_1 {
     pub regWrite: GSP_SEQ_BUF_PAYLOAD_REG_WRITE,
     pub regModify: GSP_SEQ_BUF_PAYLOAD_REG_MODIFY,

-- 
2.51.2

[PATCH 4/4] gpu: nova-core: gsp: Replace firmware version with "bindings" alias

[Alexandre Courbot]

We have an "bindings" alias to avoid having to mention the firmware
version again and again, and limit the diff when upgrading the firmware.
Use it where we neglected to.

Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
 drivers/gpu/nova-core/gsp/fw.rs | 58 ++++++++++++++++++++---------------------
 1 file changed, 29 insertions(+), 29 deletions(-)

diff --git a/drivers/gpu/nova-core/gsp/fw.rs b/drivers/gpu/nova-core/gsp/fw.rs
index 252755dbb73c..3baa5455cc32 100644
--- a/drivers/gpu/nova-core/gsp/fw.rs
+++ b/drivers/gpu/nova-core/gsp/fw.rs
@@ -141,8 +141,8 @@ unsafe impl AsBytes for GspFwWprMeta {}
 // are valid.
 unsafe impl FromBytes for GspFwWprMeta {}
 
-type GspFwWprMetaBootResumeInfo = r570_144::GspFwWprMeta__bindgen_ty_1;
-type GspFwWprMetaBootInfo = r570_144::GspFwWprMeta__bindgen_ty_1__bindgen_ty_1;
+type GspFwWprMetaBootResumeInfo = bindings::GspFwWprMeta__bindgen_ty_1;
+type GspFwWprMetaBootInfo = bindings::GspFwWprMeta__bindgen_ty_1__bindgen_ty_1;
 
 impl GspFwWprMeta {
     /// Fill in and return a `GspFwWprMeta` suitable for booting `gsp_firmware` using the
@@ -150,8 +150,8 @@ impl GspFwWprMeta {
     pub(crate) fn new(gsp_firmware: &GspFirmware, fb_layout: &FbLayout) -> Self {
         Self(bindings::GspFwWprMeta {
             // CAST: we want to store the bits of `GSP_FW_WPR_META_MAGIC` unmodified.
-            magic: r570_144::GSP_FW_WPR_META_MAGIC as u64,
-            revision: u64::from(r570_144::GSP_FW_WPR_META_REVISION),
+            magic: bindings::GSP_FW_WPR_META_MAGIC as u64,
+            revision: u64::from(bindings::GSP_FW_WPR_META_REVISION),
             sysmemAddrOfRadix3Elf: gsp_firmware.radix3_dma_handle(),
             sizeOfRadix3Elf: u64::from_safe_cast(gsp_firmware.size),
             sysmemAddrOfBootloader: gsp_firmware.bootloader.ucode.dma_handle(),
@@ -315,19 +315,19 @@ fn from(value: MsgFunction) -> Self {
 #[repr(u32)]
 pub(crate) enum SeqBufOpcode {
     // Core operation opcodes
-    CoreReset = r570_144::GSP_SEQ_BUF_OPCODE_GSP_SEQ_BUF_OPCODE_CORE_RESET,
-    CoreResume = r570_144::GSP_SEQ_BUF_OPCODE_GSP_SEQ_BUF_OPCODE_CORE_RESUME,
-    CoreStart = r570_144::GSP_SEQ_BUF_OPCODE_GSP_SEQ_BUF_OPCODE_CORE_START,
-    CoreWaitForHalt = r570_144::GSP_SEQ_BUF_OPCODE_GSP_SEQ_BUF_OPCODE_CORE_WAIT_FOR_HALT,
+    CoreReset = bindings::GSP_SEQ_BUF_OPCODE_GSP_SEQ_BUF_OPCODE_CORE_RESET,
+    CoreResume = bindings::GSP_SEQ_BUF_OPCODE_GSP_SEQ_BUF_OPCODE_CORE_RESUME,
+    CoreStart = bindings::GSP_SEQ_BUF_OPCODE_GSP_SEQ_BUF_OPCODE_CORE_START,
+    CoreWaitForHalt = bindings::GSP_SEQ_BUF_OPCODE_GSP_SEQ_BUF_OPCODE_CORE_WAIT_FOR_HALT,
 
     // Delay opcode
-    DelayUs = r570_144::GSP_SEQ_BUF_OPCODE_GSP_SEQ_BUF_OPCODE_DELAY_US,
+    DelayUs = bindings::GSP_SEQ_BUF_OPCODE_GSP_SEQ_BUF_OPCODE_DELAY_US,
 
     // Register operation opcodes
-    RegModify = r570_144::GSP_SEQ_BUF_OPCODE_GSP_SEQ_BUF_OPCODE_REG_MODIFY,
-    RegPoll = r570_144::GSP_SEQ_BUF_OPCODE_GSP_SEQ_BUF_OPCODE_REG_POLL,
-    RegStore = r570_144::GSP_SEQ_BUF_OPCODE_GSP_SEQ_BUF_OPCODE_REG_STORE,
-    RegWrite = r570_144::GSP_SEQ_BUF_OPCODE_GSP_SEQ_BUF_OPCODE_REG_WRITE,
+    RegModify = bindings::GSP_SEQ_BUF_OPCODE_GSP_SEQ_BUF_OPCODE_REG_MODIFY,
+    RegPoll = bindings::GSP_SEQ_BUF_OPCODE_GSP_SEQ_BUF_OPCODE_REG_POLL,
+    RegStore = bindings::GSP_SEQ_BUF_OPCODE_GSP_SEQ_BUF_OPCODE_REG_STORE,
+    RegWrite = bindings::GSP_SEQ_BUF_OPCODE_GSP_SEQ_BUF_OPCODE_REG_WRITE,
 }
 
 impl fmt::Display for SeqBufOpcode {
@@ -351,25 +351,25 @@ impl TryFrom<u32> for SeqBufOpcode {
 
     fn try_from(value: u32) -> Result<SeqBufOpcode> {
         match value {
-            r570_144::GSP_SEQ_BUF_OPCODE_GSP_SEQ_BUF_OPCODE_CORE_RESET => {
+            bindings::GSP_SEQ_BUF_OPCODE_GSP_SEQ_BUF_OPCODE_CORE_RESET => {
                 Ok(SeqBufOpcode::CoreReset)
             }
-            r570_144::GSP_SEQ_BUF_OPCODE_GSP_SEQ_BUF_OPCODE_CORE_RESUME => {
+            bindings::GSP_SEQ_BUF_OPCODE_GSP_SEQ_BUF_OPCODE_CORE_RESUME => {
                 Ok(SeqBufOpcode::CoreResume)
             }
-            r570_144::GSP_SEQ_BUF_OPCODE_GSP_SEQ_BUF_OPCODE_CORE_START => {
+            bindings::GSP_SEQ_BUF_OPCODE_GSP_SEQ_BUF_OPCODE_CORE_START => {
                 Ok(SeqBufOpcode::CoreStart)
             }
-            r570_144::GSP_SEQ_BUF_OPCODE_GSP_SEQ_BUF_OPCODE_CORE_WAIT_FOR_HALT => {
+            bindings::GSP_SEQ_BUF_OPCODE_GSP_SEQ_BUF_OPCODE_CORE_WAIT_FOR_HALT => {
                 Ok(SeqBufOpcode::CoreWaitForHalt)
             }
-            r570_144::GSP_SEQ_BUF_OPCODE_GSP_SEQ_BUF_OPCODE_DELAY_US => Ok(SeqBufOpcode::DelayUs),
-            r570_144::GSP_SEQ_BUF_OPCODE_GSP_SEQ_BUF_OPCODE_REG_MODIFY => {
+            bindings::GSP_SEQ_BUF_OPCODE_GSP_SEQ_BUF_OPCODE_DELAY_US => Ok(SeqBufOpcode::DelayUs),
+            bindings::GSP_SEQ_BUF_OPCODE_GSP_SEQ_BUF_OPCODE_REG_MODIFY => {
                 Ok(SeqBufOpcode::RegModify)
             }
-            r570_144::GSP_SEQ_BUF_OPCODE_GSP_SEQ_BUF_OPCODE_REG_POLL => Ok(SeqBufOpcode::RegPoll),
-            r570_144::GSP_SEQ_BUF_OPCODE_GSP_SEQ_BUF_OPCODE_REG_STORE => Ok(SeqBufOpcode::RegStore),
-            r570_144::GSP_SEQ_BUF_OPCODE_GSP_SEQ_BUF_OPCODE_REG_WRITE => Ok(SeqBufOpcode::RegWrite),
+            bindings::GSP_SEQ_BUF_OPCODE_GSP_SEQ_BUF_OPCODE_REG_POLL => Ok(SeqBufOpcode::RegPoll),
+            bindings::GSP_SEQ_BUF_OPCODE_GSP_SEQ_BUF_OPCODE_REG_STORE => Ok(SeqBufOpcode::RegStore),
+            bindings::GSP_SEQ_BUF_OPCODE_GSP_SEQ_BUF_OPCODE_REG_WRITE => Ok(SeqBufOpcode::RegWrite),
             _ => Err(EINVAL),
         }
     }
@@ -385,7 +385,7 @@ fn from(value: SeqBufOpcode) -> Self {
 /// Wrapper for GSP sequencer register write payload.
 #[repr(transparent)]
 #[derive(Copy, Clone)]
-pub(crate) struct RegWritePayload(r570_144::GSP_SEQ_BUF_PAYLOAD_REG_WRITE);
+pub(crate) struct RegWritePayload(bindings::GSP_SEQ_BUF_PAYLOAD_REG_WRITE);
 
 impl RegWritePayload {
     /// Returns the register address.
@@ -408,7 +408,7 @@ unsafe impl AsBytes for RegWritePayload {}
 /// Wrapper for GSP sequencer register modify payload.
 #[repr(transparent)]
 #[derive(Copy, Clone)]
-pub(crate) struct RegModifyPayload(r570_144::GSP_SEQ_BUF_PAYLOAD_REG_MODIFY);
+pub(crate) struct RegModifyPayload(bindings::GSP_SEQ_BUF_PAYLOAD_REG_MODIFY);
 
 impl RegModifyPayload {
     /// Returns the register address.
@@ -436,7 +436,7 @@ unsafe impl AsBytes for RegModifyPayload {}
 /// Wrapper for GSP sequencer register poll payload.
 #[repr(transparent)]
 #[derive(Copy, Clone)]
-pub(crate) struct RegPollPayload(r570_144::GSP_SEQ_BUF_PAYLOAD_REG_POLL);
+pub(crate) struct RegPollPayload(bindings::GSP_SEQ_BUF_PAYLOAD_REG_POLL);
 
 impl RegPollPayload {
     /// Returns the register address.
@@ -469,7 +469,7 @@ unsafe impl AsBytes for RegPollPayload {}
 /// Wrapper for GSP sequencer delay payload.
 #[repr(transparent)]
 #[derive(Copy, Clone)]
-pub(crate) struct DelayUsPayload(r570_144::GSP_SEQ_BUF_PAYLOAD_DELAY_US);
+pub(crate) struct DelayUsPayload(bindings::GSP_SEQ_BUF_PAYLOAD_DELAY_US);
 
 impl DelayUsPayload {
     /// Returns the delay value in microseconds.
@@ -487,7 +487,7 @@ unsafe impl AsBytes for DelayUsPayload {}
 /// Wrapper for GSP sequencer register store payload.
 #[repr(transparent)]
 #[derive(Copy, Clone)]
-pub(crate) struct RegStorePayload(r570_144::GSP_SEQ_BUF_PAYLOAD_REG_STORE);
+pub(crate) struct RegStorePayload(bindings::GSP_SEQ_BUF_PAYLOAD_REG_STORE);
 
 impl RegStorePayload {
     /// Returns the register address.
@@ -510,7 +510,7 @@ unsafe impl AsBytes for RegStorePayload {}
 
 /// Wrapper for GSP sequencer buffer command.
 #[repr(transparent)]
-pub(crate) struct SequencerBufferCmd(r570_144::GSP_SEQUENCER_BUFFER_CMD);
+pub(crate) struct SequencerBufferCmd(bindings::GSP_SEQUENCER_BUFFER_CMD);
 
 impl SequencerBufferCmd {
     /// Returns the opcode as a `SeqBufOpcode` enum, or error if invalid.
@@ -612,7 +612,7 @@ unsafe impl AsBytes for SequencerBufferCmd {}
 
 /// Wrapper for GSP run CPU sequencer RPC.
 #[repr(transparent)]
-pub(crate) struct RunCpuSequencer(r570_144::rpc_run_cpu_sequencer_v17_00);
+pub(crate) struct RunCpuSequencer(bindings::rpc_run_cpu_sequencer_v17_00);
 
 impl RunCpuSequencer {
     /// Returns the command index.

-- 
2.51.2

Re: [PATCH 0/4] gpu: nova-core: Fixups for GSP message queue and bindings

[Alexandre Courbot]

On Sat Nov 22, 2025 at 12:00 AM JST, Alexandre Courbot wrote:
> This series contains a few fixups for the recently merged GSP
> command-queue code, by order of importance:
>
> - Some explicit padding required to safely implement `AsBytes` was
>   missing in the bindings,
> - A bug in the received message length calculation results in the
>   message handler being given more data than it should, 
> - `MaybeZeroable` is now derived by the kernel's bindgen, but the Nova
>   bindings have not been updated for that,
> - Some items in the bindings were referred to using the version module
>   directly, instead of the alias we defined to limit the diff when we
>   upgrade firmware versions.
>
> All of them address "bugs" (with the first two fixing actual correctness
> issues), but since Nova does not do much anyway, they are also not
> absolutely critical and can wait -rc1 if we prefer to do so.

These patches are missing "Fixes:" tags - will send a v2 with them.

Re: [PATCH 2/4] gpu: nova-core: gsp: Fix length of received messages

[Joel Fernandes]

Hi Alex,

> On Nov 22, 2025, at 12:00 AM, Alexandre Courbot <acourbot@nvidia.com> wrote:
> 
> The size of messages' payload is miscalculated, leading to extra data
> passed to the message handler. While this is not a problem with our
> current set of commands, others with a variable-length payload may
> misbehave. Fix this.
> 
> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
> ---
> drivers/gpu/nova-core/gsp/cmdq.rs | 11 +++++++----
> drivers/gpu/nova-core/gsp/fw.rs   |  2 +-
> 2 files changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/nova-core/gsp/cmdq.rs b/drivers/gpu/nova-core/gsp/cmdq.rs
> index 6f946d14868a..dab73377c526 100644
> --- a/drivers/gpu/nova-core/gsp/cmdq.rs
> +++ b/drivers/gpu/nova-core/gsp/cmdq.rs
> @@ -588,21 +588,24 @@ fn wait_for_msg(&self, timeout: Delta) -> Result<GspMessage<'_>> {
>             header.length(),
>         );
> 
> +        // The length of the message that follows the header.
> +        let msg_length = header.length() - size_of::<GspMsgElement>();

Is this immune to under flow without one of the checked subtraction wrappers? Either way, we should not tolerate the underflow I think. Which means it can panic when the rust overflow checks are enabled. Since the header length comes from firmware, this cannot be guaranteed to not underflow in the event of a malformed message.

thanks,

 - Joel



> +
>         // Check that the driver read area is large enough for the message.
> -        if slice_1.len() + slice_2.len() < header.length() {
> +        if slice_1.len() + slice_2.len() < msg_length {
>             return Err(EIO);
>         }
> 
>         // Cut the message slices down to the actual length of the message.
> -        let (slice_1, slice_2) = if slice_1.len() > header.length() {
> +        let (slice_1, slice_2) = if slice_1.len() > msg_length {
>             // PANIC: we checked above that `slice_1` is at least as long as `msg_header.length()`.
> -            (slice_1.split_at(header.length()).0, &slice_2[0..0])
> +            (slice_1.split_at(msg_length).0, &slice_2[0..0])
>         } else {
>             (
>                 slice_1,
>                 // PANIC: we checked above that `slice_1.len() + slice_2.len()` is at least as
>                 // large as `msg_header.length()`.
> -                slice_2.split_at(header.length() - slice_1.len()).0,
> +                slice_2.split_at(msg_length - slice_1.len()).0,
>             )
>         };
> 
> diff --git a/drivers/gpu/nova-core/gsp/fw.rs b/drivers/gpu/nova-core/gsp/fw.rs
> index abffd6beec65..7fcba5afb0a3 100644
> --- a/drivers/gpu/nova-core/gsp/fw.rs
> +++ b/drivers/gpu/nova-core/gsp/fw.rs
> @@ -853,7 +853,7 @@ pub(crate) fn set_checksum(&mut self, checksum: u32) {
>         self.inner.checkSum = checksum;
>     }
> 
> -    /// Returns the total length of the message.
> +    /// Returns the total length of the message, message and RPC headers included.
>     pub(crate) fn length(&self) -> usize {
>         // `rpc.length` includes the length of the GspRpcHeader but not the message header.
>         size_of::<Self>() - size_of::<bindings::rpc_message_header_v>()
> 
> --
> 2.51.2
> 

Re: [PATCH 2/4] gpu: nova-core: gsp: Fix length of received messages

[Dirk Behme]

On 12.12.25 08:59, Joel Fernandes wrote:
> Hi Alex,
> 
>> On Nov 22, 2025, at 12:00 AM, Alexandre Courbot <acourbot@nvidia.com> wrote:
>>
>> The size of messages' payload is miscalculated, leading to extra data
>> passed to the message handler. While this is not a problem with our
>> current set of commands, others with a variable-length payload may
>> misbehave. Fix this.
>>
>> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
>> ---
>> drivers/gpu/nova-core/gsp/cmdq.rs | 11 +++++++----
>> drivers/gpu/nova-core/gsp/fw.rs   |  2 +-
>> 2 files changed, 8 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/nova-core/gsp/cmdq.rs b/drivers/gpu/nova-core/gsp/cmdq.rs
>> index 6f946d14868a..dab73377c526 100644
>> --- a/drivers/gpu/nova-core/gsp/cmdq.rs
>> +++ b/drivers/gpu/nova-core/gsp/cmdq.rs
>> @@ -588,21 +588,24 @@ fn wait_for_msg(&self, timeout: Delta) -> Result<GspMessage<'_>> {
>>             header.length(),
>>         );
>>
>> +        // The length of the message that follows the header.
>> +        let msg_length = header.length() - size_of::<GspMsgElement>();
> 
> Is this immune to under flow without one of the checked subtraction wrappers? Either way, we should not tolerate the underflow I think. Which means it can panic when the rust overflow checks are enabled. Since the header length comes from firmware, this cannot be guaranteed to not underflow in the event of a malformed message.

Would this be a possible use case for the untrusted data proposal

https://lwn.net/Articles/1034603/

?

Cheers

Dirk

Re: [PATCH 2/4] gpu: nova-core: gsp: Fix length of received messages

[Alistair Popple]

On 2025-12-12 at 18:59 +1100, Joel Fernandes <joelagnelf@nvidia.com> wrote...
> Hi Alex,
> 
> > On Nov 22, 2025, at 12:00 AM, Alexandre Courbot <acourbot@nvidia.com> wrote:
> > 
> > The size of messages' payload is miscalculated, leading to extra data
> > passed to the message handler. While this is not a problem with our
> > current set of commands, others with a variable-length payload may
> > misbehave. Fix this.
> > 
> > Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
> > ---
> > drivers/gpu/nova-core/gsp/cmdq.rs | 11 +++++++----
> > drivers/gpu/nova-core/gsp/fw.rs   |  2 +-
> > 2 files changed, 8 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/gpu/nova-core/gsp/cmdq.rs b/drivers/gpu/nova-core/gsp/cmdq.rs
> > index 6f946d14868a..dab73377c526 100644
> > --- a/drivers/gpu/nova-core/gsp/cmdq.rs
> > +++ b/drivers/gpu/nova-core/gsp/cmdq.rs
> > @@ -588,21 +588,24 @@ fn wait_for_msg(&self, timeout: Delta) -> Result<GspMessage<'_>> {
> >             header.length(),
> >         );
> > 
> > +        // The length of the message that follows the header.
> > +        let msg_length = header.length() - size_of::<GspMsgElement>();

Wouldn't it be better to add a new method to GspMsgElement to get the size of
the associated message rather than open coding it here?

 - Alistair

> Is this immune to under flow without one of the checked subtraction wrappers? Either way, we should not tolerate the underflow I think. Which means it can panic when the rust overflow checks are enabled. Since the header length comes from firmware, this cannot be guaranteed to not underflow in the event of a malformed message.
> 
> thanks,
> 
>  - Joel
> 
> 
> 
> > +
> >         // Check that the driver read area is large enough for the message.
> > -        if slice_1.len() + slice_2.len() < header.length() {
> > +        if slice_1.len() + slice_2.len() < msg_length {
> >             return Err(EIO);
> >         }
> > 
> >         // Cut the message slices down to the actual length of the message.
> > -        let (slice_1, slice_2) = if slice_1.len() > header.length() {
> > +        let (slice_1, slice_2) = if slice_1.len() > msg_length {
> >             // PANIC: we checked above that `slice_1` is at least as long as `msg_header.length()`.
> > -            (slice_1.split_at(header.length()).0, &slice_2[0..0])
> > +            (slice_1.split_at(msg_length).0, &slice_2[0..0])
> >         } else {
> >             (
> >                 slice_1,
> >                 // PANIC: we checked above that `slice_1.len() + slice_2.len()` is at least as
> >                 // large as `msg_header.length()`.
> > -                slice_2.split_at(header.length() - slice_1.len()).0,
> > +                slice_2.split_at(msg_length - slice_1.len()).0,
> >             )
> >         };
> > 
> > diff --git a/drivers/gpu/nova-core/gsp/fw.rs b/drivers/gpu/nova-core/gsp/fw.rs
> > index abffd6beec65..7fcba5afb0a3 100644
> > --- a/drivers/gpu/nova-core/gsp/fw.rs
> > +++ b/drivers/gpu/nova-core/gsp/fw.rs
> > @@ -853,7 +853,7 @@ pub(crate) fn set_checksum(&mut self, checksum: u32) {
> >         self.inner.checkSum = checksum;
> >     }
> > 
> > -    /// Returns the total length of the message.
> > +    /// Returns the total length of the message, message and RPC headers included.
> >     pub(crate) fn length(&self) -> usize {
> >         // `rpc.length` includes the length of the GspRpcHeader but not the message header.
> >         size_of::<Self>() - size_of::<bindings::rpc_message_header_v>()
> > 
> > --
> > 2.51.2
> > 

Re: [PATCH 2/4] gpu: nova-core: gsp: Fix length of received messages

[Alistair Popple]

On 2025-12-12 at 19:10 +1100, Dirk Behme <dirk.behme@gmail.com> wrote...
> On 12.12.25 08:59, Joel Fernandes wrote:
> > Hi Alex,
> > 
> >> On Nov 22, 2025, at 12:00 AM, Alexandre Courbot <acourbot@nvidia.com> wrote:
> >>
> >> The size of messages' payload is miscalculated, leading to extra data
> >> passed to the message handler. While this is not a problem with our
> >> current set of commands, others with a variable-length payload may
> >> misbehave. Fix this.
> >>
> >> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
> >> ---
> >> drivers/gpu/nova-core/gsp/cmdq.rs | 11 +++++++----
> >> drivers/gpu/nova-core/gsp/fw.rs   |  2 +-
> >> 2 files changed, 8 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/drivers/gpu/nova-core/gsp/cmdq.rs b/drivers/gpu/nova-core/gsp/cmdq.rs
> >> index 6f946d14868a..dab73377c526 100644
> >> --- a/drivers/gpu/nova-core/gsp/cmdq.rs
> >> +++ b/drivers/gpu/nova-core/gsp/cmdq.rs
> >> @@ -588,21 +588,24 @@ fn wait_for_msg(&self, timeout: Delta) -> Result<GspMessage<'_>> {
> >>             header.length(),
> >>         );
> >>
> >> +        // The length of the message that follows the header.
> >> +        let msg_length = header.length() - size_of::<GspMsgElement>();
> > 
> > Is this immune to under flow without one of the checked subtraction wrappers? Either way, we should not tolerate the underflow I think. Which means it can panic when the rust overflow checks are enabled. Since the header length comes from firmware, this cannot be guaranteed to not underflow in the event of a malformed message.

I think we're guaranteed not to underflow here - check out the implementation for header.length():
 
    /// Returns the total length of the message.
    pub(crate) fn length(&self) -> usize {
        // `rpc.length` includes the length of the GspRpcHeader but not the message header.
        size_of::<Self>() - size_of::<bindings::rpc_message_header_v>()
            + num::u32_as_usize(self.inner.rpc.length)
    }

So the above calculation expands to:

msg_length = size_of::<Self>() - size_of::<bindings::rpc_message_header_v>()
            + num::u32_as_usize(self.inner.rpc.length) - size_of::<GspMsgElement>();

Where self.inner.rpc.length is guaranteed to be >= size_of::<rpc_message_header_v>() by the construction of the type.

> Would this be a possible use case for the untrusted data proposal
> 
> https://lwn.net/Articles/1034603/
> 
> ?

Responding here because Joel appears to have sent a HTML only response ;-)

I agree with Joel's points - this does sound useful but as a separate project.
I'd imagine we'd want to it one layer lower though - ie. in the construction of
the GspMsgElement.

> Cheers
> 
> Dirk

Re: [PATCH 2/4] gpu: nova-core: gsp: Fix length of received messages

[Joel Fernandes]

Hi Alistair,

> On Dec 15, 2025, at 8:43 AM, Alistair Popple <apopple@nvidia.com> wrote:
> 
> On 2025-12-12 at 19:10 +1100, Dirk Behme <dirk.behme@gmail.com> wrote...
>>> On 12.12.25 08:59, Joel Fernandes wrote:
>>> Hi Alex,
>>> 
>>>> On Nov 22, 2025, at 12:00 AM, Alexandre Courbot <acourbot@nvidia.com> wrote:
>>>> 
>>>> The size of messages' payload is miscalculated, leading to extra data
>>>> passed to the message handler. While this is not a problem with our
>>>> current set of commands, others with a variable-length payload may
>>>> misbehave. Fix this.
>>>> 
>>>> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
>>>> ---
>>>> drivers/gpu/nova-core/gsp/cmdq.rs | 11 +++++++----
>>>> drivers/gpu/nova-core/gsp/fw.rs   |  2 +-
>>>> 2 files changed, 8 insertions(+), 5 deletions(-)
>>>> 
>>>> diff --git a/drivers/gpu/nova-core/gsp/cmdq.rs b/drivers/gpu/nova-core/gsp/cmdq.rs
>>>> index 6f946d14868a..dab73377c526 100644
>>>> --- a/drivers/gpu/nova-core/gsp/cmdq.rs
>>>> +++ b/drivers/gpu/nova-core/gsp/cmdq.rs
>>>> @@ -588,21 +588,24 @@ fn wait_for_msg(&self, timeout: Delta) -> Result<GspMessage<'_>> {
>>>>            header.length(),
>>>>        );
>>>> 
>>>> +        // The length of the message that follows the header.
>>>> +        let msg_length = header.length() - size_of::<GspMsgElement>();
>>> 
>>> Is this immune to under flow without one of the checked subtraction wrappers? Either way, we should not tolerate the underflow I think. Which means it can panic when the rust overflow checks are enabled. Since the header length comes from firmware, this cannot be guaranteed to not underflow in the event of a malformed message.
> 
> I think we're guaranteed not to underflow here - check out the implementation for header.length():
> 
>    /// Returns the total length of the message.
>    pub(crate) fn length(&self) -> usize {
>        // `rpc.length` includes the length of the GspRpcHeader but not the message header.
>        size_of::<Self>() - size_of::<bindings::rpc_message_header_v>()
>            + num::u32_as_usize(self.inner.rpc.length)
>    }
> 
> So the above calculation expands to:
> 
> msg_length = size_of::<Self>() - size_of::<bindings::rpc_message_header_v>()
>            + num::u32_as_usize(self.inner.rpc.length) - size_of::<GspMsgElement>();
> 
> Where self.inner.rpc.length is guaranteed to be >= size_of::<rpc_message_header_v>() by the construction of the type.

But this length field is coming from the firmware, correct? The guarantee is provided by firmware, not by Rust code calculating the length.

Maybe Rust validating that the length matches, or is greater than or equal to, the message header would be one way to avoid doing the checked subtraction. I would still be comfortable doing the checked subtraction in case the firmware payload in the message buffer is corrupted and the length field is corrupted.

I think Rust cannot trust fields coming from the firmware and needs to check them to prevent undefined behavior. Or maybe the policy is to include safety comments, like we do when expecting C code to behave in a certain way. I do not know. But we should identify the policy for this and stick to it for future such issues.

I think one way to verify that there is a Rust guarantee about the length field is to do the unchecked subtraction, compile the code, and then see if the generated code has any panics in it (With the overflow checking config enabled.)

If it does, then dead code elimination could not determine at compile time that the subtraction would not overflow. Right?

> 
>> Would this be a possible use case for the untrusted data proposal
>> 
>> https://lwn.net/Articles/1034603/
>> 
>> ?
> 
> Responding here because Joel appears to have sent a HTML only response ;-)

Sorry. :)

> 
> I agree with Joel's points - this does sound useful but as a separate project.
> I'd imagine we'd want to it one layer lower though - ie. in the construction of
> the GspMsgElement.

Agreed, thanks.

 - Joel 



> 
>> Cheers
>> 
>> Dirk

Re: [PATCH 2/4] gpu: nova-core: gsp: Fix length of received messages

[John Hubbard]

On 12/15/25 10:22 AM, Joel Fernandes wrote:
>> On Dec 15, 2025, at 8:43 AM, Alistair Popple <apopple@nvidia.com> wrote:...
>> So the above calculation expands to:
>>
>> msg_length = size_of::<Self>() - size_of::<bindings::rpc_message_header_v>()
>>             + num::u32_as_usize(self.inner.rpc.length) - size_of::<GspMsgElement>();
>>
>> Where self.inner.rpc.length is guaranteed to be >= size_of::<rpc_message_header_v>() by the construction of the type.
> 
> But this length field is coming from the firmware, correct? The guarantee is provided by firmware, not by Rust code calculating the length.
> 
> Maybe Rust validating that the length matches, or is greater than or equal to, the message header would be one way to avoid doing the checked subtraction. I would still be comfortable doing the checked subtraction in case the firmware payload in the message buffer is corrupted and the length field is corrupted.
> 
> I think Rust cannot trust fields coming from the firmware and needs to check them to prevent undefined behavior.

Right. The firmware is a separate code base, running on a separate
processor, and it is not part of the Rust driver. So it cannot
participate in any of the various Rust guarantees.

We should treat data that comes from the firmware as not yet
validated, external data.

  Or maybe the policy is to include safety comments, like we do when 
expecting C code to behave in a certain way. I
do not know. But we should identify the policy for this and stick to it 
for future such issues.es

Yes. I've written above what I believe we should use for a policy.


thanks,
-- 
John Hubbard

Re: [PATCH 2/4] gpu: nova-core: gsp: Fix length of received messages

[Timur Tabi]

On Mon, 2025-12-15 at 11:46 +0900, John Hubbard wrote:
> We should treat data that comes from the firmware as not yet
> validated, external data.

Maybe we should make it a policy that all data read from firmware / disk needs to be instantiate
via a constructor that validates all the fields.

Re: [PATCH 2/4] gpu: nova-core: gsp: Fix length of received messages

[John Hubbard]

On 12/15/25 11:47 AM, Timur Tabi wrote:
> On Mon, 2025-12-15 at 11:46 +0900, John Hubbard wrote:
>> We should treat data that comes from the firmware as not yet
>> validated, external data.
> 
> Maybe we should make it a policy that all data read from firmware / disk needs to be instantiate
> via a constructor that validates all the fields.

That's one approach, and maybe it will end up being the best and
most desirable approach.

But I'd stop short of making it a full-blown policy, because there
are other ways of validating the data that comes from the firmware,
and there's not (yet?) any real need to require one particular
solution, I think.



thanks,
-- 
John Hubbard

Re: [PATCH 2/4] gpu: nova-core: gsp: Fix length of received messages

[Alexandre Courbot]

On Mon Dec 15, 2025 at 8:43 AM JST, Alistair Popple wrote:
> On 2025-12-12 at 19:10 +1100, Dirk Behme <dirk.behme@gmail.com> wrote...
>> On 12.12.25 08:59, Joel Fernandes wrote:
>> > Hi Alex,
>> > 
>> >> On Nov 22, 2025, at 12:00 AM, Alexandre Courbot <acourbot@nvidia.com> wrote:
>> >>
>> >> The size of messages' payload is miscalculated, leading to extra data
>> >> passed to the message handler. While this is not a problem with our
>> >> current set of commands, others with a variable-length payload may
>> >> misbehave. Fix this.
>> >>
>> >> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
>> >> ---
>> >> drivers/gpu/nova-core/gsp/cmdq.rs | 11 +++++++----
>> >> drivers/gpu/nova-core/gsp/fw.rs   |  2 +-
>> >> 2 files changed, 8 insertions(+), 5 deletions(-)
>> >>
>> >> diff --git a/drivers/gpu/nova-core/gsp/cmdq.rs b/drivers/gpu/nova-core/gsp/cmdq.rs
>> >> index 6f946d14868a..dab73377c526 100644
>> >> --- a/drivers/gpu/nova-core/gsp/cmdq.rs
>> >> +++ b/drivers/gpu/nova-core/gsp/cmdq.rs
>> >> @@ -588,21 +588,24 @@ fn wait_for_msg(&self, timeout: Delta) -> Result<GspMessage<'_>> {
>> >>             header.length(),
>> >>         );
>> >>
>> >> +        // The length of the message that follows the header.
>> >> +        let msg_length = header.length() - size_of::<GspMsgElement>();
>> > 
>> > Is this immune to under flow without one of the checked subtraction wrappers? Either way, we should not tolerate the underflow I think. Which means it can panic when the rust overflow checks are enabled. Since the header length comes from firmware, this cannot be guaranteed to not underflow in the event of a malformed message.
>
> I think we're guaranteed not to underflow here - check out the implementation for header.length():
>  
>     /// Returns the total length of the message.
>     pub(crate) fn length(&self) -> usize {
>         // `rpc.length` includes the length of the GspRpcHeader but not the message header.
>         size_of::<Self>() - size_of::<bindings::rpc_message_header_v>()
>             + num::u32_as_usize(self.inner.rpc.length)
>     }
>
> So the above calculation expands to:
>
> msg_length = size_of::<Self>() - size_of::<bindings::rpc_message_header_v>()
>             + num::u32_as_usize(self.inner.rpc.length) - size_of::<GspMsgElement>();
>
> Where self.inner.rpc.length is guaranteed to be >= size_of::<rpc_message_header_v>() by the construction of the type.

That's correct, although it defers the possibility of underflow to
`length`, albeit using two constants. Still, doing this as a const would
catch any potential issue at build-time:

    pub(crate) fn length(&self) -> usize {
        // `rpc.length` includes the length of the GspRpcHeader but not the message header.
        const RPC_LENGTH: usize = size_of::<Self>() - size_of::<bindings::rpc_message_header_v>();

        RPC_LENGTH + num::u32_as_usize(self.inner.rpc.length)
    }

This length computation has been the subject of debate between me and
Alistair back when we were writing the code, so this part can be subject
to change if doing so limits the amount of potentially panicking
operations.

>
>> Would this be a possible use case for the untrusted data proposal
>> 
>> https://lwn.net/Articles/1034603/
>> 
>> ?
>
> Responding here because Joel appears to have sent a HTML only response ;-)
>
> I agree with Joel's points - this does sound useful but as a separate project.
> I'd imagine we'd want to it one layer lower though - ie. in the construction of
> the GspMsgElement.

How would that be beneficial? We would need to use `unsafe` to access
the data, but then unless we can encode the guarantees that we verified
in the type itself (or its invariants, but that would quickly become
cumbersome to manage) we would still have the same ops ongoing.

IMHO the simple and safe way is to use checked operations with data that
comes from the GSP.

Re: [PATCH 2/4] gpu: nova-core: gsp: Fix length of received messages

[Alexandre Courbot]

On Mon Dec 15, 2025 at 8:29 AM JST, Alistair Popple wrote:
> On 2025-12-12 at 18:59 +1100, Joel Fernandes <joelagnelf@nvidia.com> wrote...
>> Hi Alex,
>> 
>> > On Nov 22, 2025, at 12:00 AM, Alexandre Courbot <acourbot@nvidia.com> wrote:
>> > 
>> > The size of messages' payload is miscalculated, leading to extra data
>> > passed to the message handler. While this is not a problem with our
>> > current set of commands, others with a variable-length payload may
>> > misbehave. Fix this.
>> > 
>> > Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
>> > ---
>> > drivers/gpu/nova-core/gsp/cmdq.rs | 11 +++++++----
>> > drivers/gpu/nova-core/gsp/fw.rs   |  2 +-
>> > 2 files changed, 8 insertions(+), 5 deletions(-)
>> > 
>> > diff --git a/drivers/gpu/nova-core/gsp/cmdq.rs b/drivers/gpu/nova-core/gsp/cmdq.rs
>> > index 6f946d14868a..dab73377c526 100644
>> > --- a/drivers/gpu/nova-core/gsp/cmdq.rs
>> > +++ b/drivers/gpu/nova-core/gsp/cmdq.rs
>> > @@ -588,21 +588,24 @@ fn wait_for_msg(&self, timeout: Delta) -> Result<GspMessage<'_>> {
>> >             header.length(),
>> >         );
>> > 
>> > +        // The length of the message that follows the header.
>> > +        let msg_length = header.length() - size_of::<GspMsgElement>();
>
> Wouldn't it be better to add a new method to GspMsgElement to get the size of
> the associated message rather than open coding it here?

Agreed, that seems to make sense and should provide us with a safe
operation.

Re: [PATCH 2/4] gpu: nova-core: gsp: Fix length of received messages

[Alistair Popple]

On 2025-12-15 at 12:22 +1100, Joel Fernandes <joelagnelf@nvidia.com> wrote...
> Hi Alistair,
> 
> > On Dec 15, 2025, at 8:43 AM, Alistair Popple <apopple@nvidia.com> wrote:
> > 
> > On 2025-12-12 at 19:10 +1100, Dirk Behme <dirk.behme@gmail.com> wrote...
> >>> On 12.12.25 08:59, Joel Fernandes wrote:
> >>> Hi Alex,
> >>> 
> >>>> On Nov 22, 2025, at 12:00 AM, Alexandre Courbot <acourbot@nvidia.com> wrote:
> >>>> 
> >>>> The size of messages' payload is miscalculated, leading to extra data
> >>>> passed to the message handler. While this is not a problem with our
> >>>> current set of commands, others with a variable-length payload may
> >>>> misbehave. Fix this.
> >>>> 
> >>>> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
> >>>> ---
> >>>> drivers/gpu/nova-core/gsp/cmdq.rs | 11 +++++++----
> >>>> drivers/gpu/nova-core/gsp/fw.rs   |  2 +-
> >>>> 2 files changed, 8 insertions(+), 5 deletions(-)
> >>>> 
> >>>> diff --git a/drivers/gpu/nova-core/gsp/cmdq.rs b/drivers/gpu/nova-core/gsp/cmdq.rs
> >>>> index 6f946d14868a..dab73377c526 100644
> >>>> --- a/drivers/gpu/nova-core/gsp/cmdq.rs
> >>>> +++ b/drivers/gpu/nova-core/gsp/cmdq.rs
> >>>> @@ -588,21 +588,24 @@ fn wait_for_msg(&self, timeout: Delta) -> Result<GspMessage<'_>> {
> >>>>            header.length(),
> >>>>        );
> >>>> 
> >>>> +        // The length of the message that follows the header.
> >>>> +        let msg_length = header.length() - size_of::<GspMsgElement>();
> >>> 
> >>> Is this immune to under flow without one of the checked subtraction wrappers? Either way, we should not tolerate the underflow I think. Which means it can panic when the rust overflow checks are enabled. Since the header length comes from firmware, this cannot be guaranteed to not underflow in the event of a malformed message.
> > 
> > I think we're guaranteed not to underflow here - check out the implementation for header.length():
> > 
> >    /// Returns the total length of the message.
> >    pub(crate) fn length(&self) -> usize {
> >        // `rpc.length` includes the length of the GspRpcHeader but not the message header.
> >        size_of::<Self>() - size_of::<bindings::rpc_message_header_v>()
> >            + num::u32_as_usize(self.inner.rpc.length)
> >    }
> > 
> > So the above calculation expands to:
> > 
> > msg_length = size_of::<Self>() - size_of::<bindings::rpc_message_header_v>()
> >            + num::u32_as_usize(self.inner.rpc.length) - size_of::<GspMsgElement>();
> > 
> > Where self.inner.rpc.length is guaranteed to be >= size_of::<rpc_message_header_v>() by the construction of the type.
> 
> But this length field is coming from the firmware, correct? The guarantee is provided by firmware, not by Rust code calculating the length.

Oh you're right - I had this around the wrong way, I forgot our constructors were only used for creating messages not receiving them. Obviously killing time reading mailing lists in airports is not such a good idea :-)

> Maybe Rust validating that the length matches, or is greater than or equal to, the message header would be one way to avoid doing the checked subtraction. I would still be comfortable doing the checked subtraction in case the firmware payload in the message buffer is corrupted and the length field is corrupted.
> 
> I think Rust cannot trust fields coming from the firmware and needs to check them to prevent undefined behavior. Or maybe the policy is to include safety comments, like we do when expecting C code to behave in a certain way. I do not know. But we should identify the policy for this and stick to it for future such issues.
> 
> I think one way to verify that there is a Rust guarantee about the length field is to do the unchecked subtraction, compile the code, and then see if the generated code has any panics in it (With the overflow checking config enabled.)
> 
> If it does, then dead code elimination could not determine at compile time that the subtraction would not overflow. Right?

Agreed. Whilst we can't avoid needing to trust the firmware at some level crashing the kernel in response to a bad message would be bad and make debugging it a pain.

> > 
> >> Would this be a possible use case for the untrusted data proposal
> >> 
> >> https://lwn.net/Articles/1034603/
> >> 
> >> ?
> > 
> > Responding here because Joel appears to have sent a HTML only response ;-)
> 
> Sorry. :)
> 
> > 
> > I agree with Joel's points - this does sound useful but as a separate project.
> > I'd imagine we'd want to it one layer lower though - ie. in the construction of
> > the GspMsgElement.

So I guess we'd need our own implementation of a from_bytes trait or some such that would also validate the fields when deserialising the struct.

> Agreed, thanks.
> 
>  - Joel 
> 
> 
> 
> > 
> >> Cheers
> >> 
> >> Dirk