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 <apop...@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-los...@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,
+ })
}
}
