Re: [PATCH v3 08/13] gpu: nova-core: Add bindings and accessors for GspSystemInfo

["Alexandre Courbot" <acourbot@nvidia.com>]

Hi Alistair, (+Benno as this concerns the `init!` macros)

On Tue Sep 30, 2025 at 10:16 PM JST, Alistair Popple wrote:
> Adds bindings and an in-place initialiser for the GspSystemInfo struct.
>
> Signed-off-by: Alistair Popple <apopple@nvidia.com>
>
> ---
>
> It would be good to move to using the `init!` macros at some point, but
> I couldn't figure out how to make that work to initialise an enum rather
> than a struct as is required for the transparent representation.

Indeed we have to jump through a few (minor) hoops.

First the `init!` macros do not seem to support tuple structs. They
match a `{` after the type name, which is not present in
`GspSystemInfo`. By turning it into a regular struct with a single
field, we can overcome this, and it doesn't affect the layout the
`#[repr(transparent)]` can still be used.

Then, due to a limitation with declarative macros, `init!` interprets
`::` as a separator for generic arguments, so `bindings::GspSystemInfo`
also doesn't parse. Here the trick is to use a local type alias.

After overcoming these two, I have been able to make
`GspSystemInfo::init` return an in-place initializer. It is then a
matter of changing `send_gsp_command` to accept it - please apply the
following patch on top of your series for an illustration of how it can
be done.

Note that I have added a new generic argument for the error returned by
the `Init` - this is to let us also use infallible initializers
transparently. The cool thing is that callers don't need to specify any
generic argument since they can be inferred automatically.

diff --git a/drivers/gpu/nova-core/gsp/cmdq.rs b/drivers/gpu/nova-core/gsp/cmdq.rs
index 5580eaf52f7b..0709153f9dc9 100644
--- a/drivers/gpu/nova-core/gsp/cmdq.rs
+++ b/drivers/gpu/nova-core/gsp/cmdq.rs
@@ -247,12 +247,20 @@ fn notify_gsp(bar: &Bar0) {
         NV_PGSP_QUEUE_HEAD::default().set_address(0).write(bar);
     }

-    pub(crate) fn send_gsp_command<M: CommandToGsp>(
+    pub(crate) fn send_gsp_command<M, E>(
         &mut self,
         bar: &Bar0,
         payload_size: usize,
-        init: impl FnOnce(&mut M, SBuffer<core::array::IntoIter<&mut [u8], 2>>) -> Result,
-    ) -> Result {
+        init: impl Init<M, E>,
+        init_sbuffer: impl FnOnce(SBuffer<core::array::IntoIter<&mut [u8], 2>>) -> Result,
+    ) -> Result
+    where
+        M: CommandToGsp,
+        // This allows all error types, including `Infallible`, to be used with `init`. Without
+        // this we cannot use regular stack objects as `init` since their `Init` implementation
+        // does not return any error.
+        Error: From<E>,
+    {
         // TODO: a method that extracts the regions for a given command?
         // ... and another that reduces the region to a given number of bytes!
         let driver_area = self.gsp_mem.driver_write_area();
@@ -264,7 +272,7 @@ pub(crate) fn send_gsp_command<M: CommandToGsp>(
             return Err(EAGAIN);
         }

-        let (msg_header, cmd, payload_1, payload_2) = {
+        let (msg_header, cmd_ptr, payload_1, payload_2) = {
             #[allow(clippy::incompatible_msrv)]
             let (msg_header_slice, slice_1) = driver_area
                 .0
@@ -272,7 +280,6 @@ pub(crate) fn send_gsp_command<M: CommandToGsp>(
                 .split_at_mut(size_of::<GspMsgElement>());
             let msg_header = GspMsgElement::from_bytes_mut(msg_header_slice).ok_or(EINVAL)?;
             let (cmd_slice, payload_1) = slice_1.split_at_mut(size_of::<M>());
-            let cmd = M::from_bytes_mut(cmd_slice).ok_or(EINVAL)?;
             #[allow(clippy::incompatible_msrv)]
             let payload_2 = driver_area.1.as_flattened_mut();
             // TODO: Replace this workaround to cut the payload size.
@@ -283,11 +290,22 @@ pub(crate) fn send_gsp_command<M: CommandToGsp>(
                 None => (&mut payload_1[..payload_size], payload_2),
             };

-            (msg_header, cmd, payload_1, payload_2)
+            (
+                msg_header,
+                cmd_slice.as_mut_ptr().cast(),
+                payload_1,
+                payload_2,
+            )
+        };
+
+        let cmd = unsafe {
+            init.__init(cmd_ptr)?;
+            // Convert the pointer backto a reference for checksum.
+            &mut *cmd_ptr
         };

         let sbuffer = SBuffer::new_writer([&mut payload_1[..], &mut payload_2[..]]);
-        init(cmd, sbuffer)?;
+        init_sbuffer(sbuffer)?;

         *msg_header =
             GspMsgElement::new(self.seq, size_of::<M>() + payload_size, M::FUNCTION as u32);
diff --git a/drivers/gpu/nova-core/gsp/commands.rs b/drivers/gpu/nova-core/gsp/commands.rs
index 69df8d4be353..6f1be9078853 100644
--- a/drivers/gpu/nova-core/gsp/commands.rs
+++ b/drivers/gpu/nova-core/gsp/commands.rs
@@ -79,10 +79,12 @@ pub(crate) fn build_registry(cmdq: &mut Cmdq, bar: &Bar0) -> Result {
         ],
     };

-    cmdq.send_gsp_command::<PackedRegistryTable>(bar, registry.size(), |table, sbuffer| {
-        *table = PackedRegistryTable::new(GSP_REGISTRY_NUM_ENTRIES as u32, registry.size() as u32);
-        registry.write_payload(sbuffer)
-    })
+    cmdq.send_gsp_command(
+        bar,
+        registry.size(),
+        PackedRegistryTable::new(GSP_REGISTRY_NUM_ENTRIES as u32, registry.size() as u32),
+        |sbuffer| registry.write_payload(sbuffer),
+    )
 }

 impl CommandToGsp for GspSystemInfo {
@@ -95,7 +97,7 @@ pub(crate) fn set_system_info(
     bar: &Bar0,
 ) -> Result {
     build_assert!(size_of::<GspSystemInfo>() < GSP_PAGE_SIZE);
-    cmdq.send_gsp_command::<GspSystemInfo>(bar, 0, |info, _| GspSystemInfo::init(info, dev))?;
+    cmdq.send_gsp_command(bar, 0, GspSystemInfo::init(dev), |_| Ok(()))?;

     Ok(())
 }
diff --git a/drivers/gpu/nova-core/gsp/fw/commands.rs b/drivers/gpu/nova-core/gsp/fw/commands.rs
index 83c2b017c4cb..e69be2f422f2 100644
--- a/drivers/gpu/nova-core/gsp/fw/commands.rs
+++ b/drivers/gpu/nova-core/gsp/fw/commands.rs
@@ -4,31 +4,50 @@
 use kernel::transmute::{AsBytes, FromBytes};
 use kernel::{device, pci};

+// Ideally we would derive this for all our bindings, using the same technique as
+// https://lore.kernel.org/rust-for-linux/20250814093046.2071971-3-lossin@kernel.org/
+unsafe impl Zeroable for bindings::GspSystemInfo {}
+
 #[repr(transparent)]
-pub(crate) struct GspSystemInfo(bindings::GspSystemInfo);
+pub(crate) struct GspSystemInfo {
+    // `try_init!` doesn't seem to work with tuple structs. Work around this by declaring a regular
+    // field, which comes down to exactly the same.
+    inner: bindings::GspSystemInfo,
+}

 impl GspSystemInfo {
-    pub(crate) fn init(&mut self, dev: &pci::Device<device::Bound>) -> Result {
-        self.0.gpuPhysAddr = dev.resource_start(0)?;
-        self.0.gpuPhysFbAddr = dev.resource_start(1)?;
-        self.0.gpuPhysInstAddr = dev.resource_start(3)?;
-        self.0.nvDomainBusDeviceFunc = u64::from(dev.dev_id());
+    #[allow(non_snake_case)]
+    pub(crate) fn init<'a>(dev: &'a pci::Device<device::Bound>) -> impl Init<Self, Error> + 'a {
+        // `try_init!` interprets `::` as a separator for generics, use a type alias to remove
+        // them.
+        type InnerGspSystemInfo = bindings::GspSystemInfo;

-        // Using TASK_SIZE in r535_gsp_rpc_set_system_info() seems wrong because
-        // TASK_SIZE is per-task. That's probably a design issue in GSP-RM though.
-        self.0.maxUserVa = (1 << 47) - 4096;
-        self.0.pciConfigMirrorBase = 0x088000;
-        self.0.pciConfigMirrorSize = 0x001000;
+        // Initializer for the bindings type.
+        let init_inner = try_init!(InnerGspSystemInfo {
+            gpuPhysAddr: dev.resource_start(0)?,
+            gpuPhysFbAddr: dev.resource_start(1)?,
+            gpuPhysInstAddr: dev.resource_start(3)?,
+            nvDomainBusDeviceFunc: u64::from(dev.dev_id()),

-        self.0.PCIDeviceID =
-            (u32::from(dev.device_id()) << 16) | u32::from(dev.vendor_id().as_raw());
-        self.0.PCISubDeviceID =
-            (u32::from(dev.subsystem_device_id()) << 16) | u32::from(dev.subsystem_vendor_id());
-        self.0.PCIRevisionID = u32::from(dev.revision_id());
-        self.0.bIsPrimary = 0;
-        self.0.bPreserveVideoMemoryAllocations = 0;
+            // Using TASK_SIZE in r535_gsp_rpc_set_system_info() seems wrong because
+            // TASK_SIZE is per-task. That's probably a design issue in GSP-RM though.
+            maxUserVa: (1 << 47) - 4096,
+            pciConfigMirrorBase: 0x088000,
+            pciConfigMirrorSize: 0x001000,

-        Ok(())
+            PCIDeviceID: (u32::from(dev.device_id()) << 16) | u32::from(dev.vendor_id().as_raw()),
+            PCISubDeviceID: (u32::from(dev.subsystem_device_id()) << 16)
+                | u32::from(dev.subsystem_vendor_id()),
+            PCIRevisionID: u32::from(dev.revision_id()),
+            bIsPrimary: 0,
+            bPreserveVideoMemoryAllocations: 0,
+            ..Zeroable::init_zeroed()
+        });
+
+        // Final initializer for our type.
+        try_init!(GspSystemInfo {
+            inner <- init_inner,
+        })
     }
 }