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.
Changes for v3:
- New for v3
---
drivers/gpu/nova-core/gsp/fw.rs | 1 +
drivers/gpu/nova-core/gsp/fw/commands.rs | 40 ++++++
.../gpu/nova-core/gsp/fw/r570_144/bindings.rs | 132 ++++++++++++++++++
3 files changed, 173 insertions(+)
create mode 100644 drivers/gpu/nova-core/gsp/fw/commands.rs
diff --git a/drivers/gpu/nova-core/gsp/fw.rs b/drivers/gpu/nova-core/gsp/fw.rs
index aec0db50adea..2ef9d4acd6f9 100644
--- a/drivers/gpu/nova-core/gsp/fw.rs
+++ b/drivers/gpu/nova-core/gsp/fw.rs
@@ -1,5 +1,6 @@
// SPDX-License-Identifier: GPL-2.0
+pub(crate) mod commands;
mod r570_144;
// Alias to avoid repeating the version number with every use.
diff --git a/drivers/gpu/nova-core/gsp/fw/commands.rs b/drivers/gpu/nova-core/gsp/fw/commands.rs
new file mode 100644
index 000000000000..f28779af2620
--- /dev/null
+++ b/drivers/gpu/nova-core/gsp/fw/commands.rs
@@ -0,0 +1,40 @@
+use super::bindings;
+
+use kernel::prelude::*;
+use kernel::transmute::{AsBytes, FromBytes};
+use kernel::{device, pci};
+
+#[repr(transparent)]
+pub(crate) struct GspSystemInfo(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());
+
+ // 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;
+
+ self.0.PCIDeviceID = (u32::from(dev.device_id()) << 16) | u32::from(dev.vendor_id());
+ 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;
+
+ Ok(())
+ }
+}
+
+// SAFETY: These structs don't meet the no-padding requirements of AsBytes but
+// that is not a problem because they are not used outside the kernel.
+unsafe impl AsBytes for GspSystemInfo {}
+
+// SAFETY: These structs don't meet the no-padding requirements of FromBytes but
+// that is not a problem because they are not used outside the kernel.
+unsafe impl FromBytes for GspSystemInfo {}
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 b87c4e6cb857..427fff82f7c1 100644
--- a/drivers/gpu/nova-core/gsp/fw/r570_144/bindings.rs
+++ b/drivers/gpu/nova-core/gsp/fw/r570_144/bindings.rs
@@ -320,6 +320,138 @@ fn fmt(&self, fmt: &mut ::core::fmt::Formatter<'_>) -> ::core::fmt::Result {
pub type _bindgen_ty_3 = ffi::c_uint;
#[repr(C)]
#[derive(Debug, Default, Copy, Clone)]
+pub struct DOD_METHOD_DATA {
+ pub status: u32_,
+ pub acpiIdListLen: u32_,
+ pub acpiIdList: [u32_; 16usize],
+}
+#[repr(C)]
+#[derive(Debug, Default, Copy, Clone)]
+pub struct JT_METHOD_DATA {
+ pub status: u32_,
+ pub jtCaps: u32_,
+ pub jtRevId: u16_,
+ pub bSBIOSCaps: u8_,
+ pub __bindgen_padding_0: u8,
+}
+#[repr(C)]
+#[derive(Debug, Default, Copy, Clone)]
+pub struct MUX_METHOD_DATA_ELEMENT {
+ pub acpiId: u32_,
+ pub mode: u32_,
+ pub status: u32_,
+}
+#[repr(C)]
+#[derive(Debug, Default, Copy, Clone)]
+pub struct MUX_METHOD_DATA {
+ pub tableLen: u32_,
+ pub acpiIdMuxModeTable: [MUX_METHOD_DATA_ELEMENT; 16usize],
+ pub acpiIdMuxPartTable: [MUX_METHOD_DATA_ELEMENT; 16usize],
+ pub acpiIdMuxStateTable: [MUX_METHOD_DATA_ELEMENT; 16usize],
+}
+#[repr(C)]
+#[derive(Debug, Default, Copy, Clone)]
+pub struct CAPS_METHOD_DATA {
+ pub status: u32_,
+ pub optimusCaps: u32_,
+}
+#[repr(C)]
+#[derive(Debug, Default, Copy, Clone)]
+pub struct ACPI_METHOD_DATA {
+ pub bValid: u8_,
+ pub __bindgen_padding_0: [u8; 3usize],
+ pub dodMethodData: DOD_METHOD_DATA,
+ pub jtMethodData: JT_METHOD_DATA,
+ pub muxMethodData: MUX_METHOD_DATA,
+ pub capsMethodData: CAPS_METHOD_DATA,
+}
+#[repr(C)]
+#[derive(Debug, Default, Copy, Clone)]
+pub struct BUSINFO {
+ pub deviceID: u16_,
+ pub vendorID: u16_,
+ pub subdeviceID: u16_,
+ pub subvendorID: u16_,
+ pub revisionID: u8_,
+ pub __bindgen_padding_0: u8,
+}
+#[repr(C)]
+#[derive(Debug, Default, Copy, Clone)]
+pub struct GSP_VF_INFO {
+ pub totalVFs: u32_,
+ pub firstVFOffset: u32_,
+ pub FirstVFBar0Address: u64_,
+ pub FirstVFBar1Address: u64_,
+ pub FirstVFBar2Address: u64_,
+ pub b64bitBar0: u8_,
+ pub b64bitBar1: u8_,
+ pub b64bitBar2: u8_,
+ pub __bindgen_padding_0: [u8; 5usize],
+}
+#[repr(C)]
+#[derive(Debug, Default, Copy, Clone)]
+pub struct GSP_PCIE_CONFIG_REG {
+ pub linkCap: u32_,
+}
+#[repr(C)]
+#[derive(Debug, Default, Copy, Clone)]
+pub struct GspSystemInfo {
+ pub gpuPhysAddr: u64_,
+ pub gpuPhysFbAddr: u64_,
+ pub gpuPhysInstAddr: u64_,
+ pub gpuPhysIoAddr: u64_,
+ pub nvDomainBusDeviceFunc: u64_,
+ pub simAccessBufPhysAddr: u64_,
+ pub notifyOpSharedSurfacePhysAddr: u64_,
+ pub pcieAtomicsOpMask: u64_,
+ pub consoleMemSize: u64_,
+ pub maxUserVa: u64_,
+ pub pciConfigMirrorBase: u32_,
+ pub pciConfigMirrorSize: u32_,
+ pub PCIDeviceID: u32_,
+ pub PCISubDeviceID: u32_,
+ pub PCIRevisionID: u32_,
+ pub pcieAtomicsCplDeviceCapMask: u32_,
+ pub oorArch: u8_,
+ pub __bindgen_padding_0: [u8; 7usize],
+ pub clPdbProperties: u64_,
+ pub Chipset: u32_,
+ pub bGpuBehindBridge: u8_,
+ pub bFlrSupported: u8_,
+ pub b64bBar0Supported: u8_,
+ pub bMnocAvailable: u8_,
+ pub chipsetL1ssEnable: u32_,
+ pub bUpstreamL0sUnsupported: u8_,
+ pub bUpstreamL1Unsupported: u8_,
+ pub bUpstreamL1PorSupported: u8_,
+ pub bUpstreamL1PorMobileOnly: u8_,
+ pub bSystemHasMux: u8_,
+ pub upstreamAddressValid: u8_,
+ pub FHBBusInfo: BUSINFO,
+ pub chipsetIDInfo: BUSINFO,
+ pub __bindgen_padding_1: [u8; 2usize],
+ pub acpiMethodData: ACPI_METHOD_DATA,
+ pub hypervisorType: u32_,
+ pub bIsPassthru: u8_,
+ pub __bindgen_padding_2: [u8; 7usize],
+ pub sysTimerOffsetNs: u64_,
+ pub gspVFInfo: GSP_VF_INFO,
+ pub bIsPrimary: u8_,
+ pub isGridBuild: u8_,
+ pub __bindgen_padding_3: [u8; 2usize],
+ pub pcieConfigReg: GSP_PCIE_CONFIG_REG,
+ pub gridBuildCsp: u32_,
+ pub bPreserveVideoMemoryAllocations: u8_,
+ pub bTdrEventSupported: u8_,
+ pub bFeatureStretchVblankCapable: u8_,
+ pub bEnableDynamicGranularityPageArrays: u8_,
+ pub bClockBoostSupported: u8_,
+ pub bRouteDispIntrsToCPU: u8_,
+ pub __bindgen_padding_4: [u8; 6usize],
+ pub hostPageSize: u64_,
+}
+#[repr(C)]
+#[derive(Debug, Default, Copy, Clone)]
pub struct MESSAGE_QUEUE_INIT_ARGUMENTS {
pub sharedMemPhysAddr: u64_,
pub pageTableEntryCount: u32_,
--
2.50.1Hi 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,
+ })
}
}
On Thu Oct 2, 2025 at 3:49 PM CEST, Alexandre Courbot wrote:
> 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.
Yeah that's the correct workaround at the moment. I'm tracking support
for tuple structs in [1]. Essentially the problem is that it requires
lots of effort to parse tuple structs using declarative macros. We will
get `syn` this cycle, which will enable me to support several things,
including tuple structs.
[1]: https://github.com/Rust-for-Linux/pin-init/issues/85
> 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.
This one will also be solved when we switch to syn.
> 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)?;
This is missing a safety comment. I haven't looked at this locally, so I
don't know what is happening in the 10-20 lines that aren't shown, so I
don't know if this is correct (if you're only assuming its initialized
after this line completes then it's fine). The rest of the patch looks
normal.
Hope this helps!
---
Cheers,
Benno
> + // Convert the pointer backto a reference for checksum.
> + &mut *cmd_ptr
> };
On Fri, Oct 03, 2025 at 06:34:12PM +0200, Benno Lossin wrote:
> On Thu Oct 2, 2025 at 3:49 PM CEST, Alexandre Courbot wrote:
> > 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.
>
> Yeah that's the correct workaround at the moment. I'm tracking support
> for tuple structs in [1]. Essentially the problem is that it requires
> lots of effort to parse tuple structs using declarative macros. We will
> get `syn` this cycle, which will enable me to support several things,
> including tuple structs.
>
> [1]: https://github.com/Rust-for-Linux/pin-init/issues/85
>
> > 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.
>
> This one will also be solved when we switch to syn.
I was planning to submit
https://github.com/AsahiLinux/linux/commit/2d95fd3b6c359634a0976f27f7a3c667826256da
https://github.com/AsahiLinux/linux/commit/515638cb47cf0ebdac378686fcbbdc6a8364096a
from the asahi downstream tree after 6.18-rc1. Does that still make
sense timing wise?
Types with type paths are used extensively in the asahi driver but I can
initially work around that.
Janne
On Fri Oct 3, 2025 at 7:25 PM CEST, Janne Grunau wrote:
> On Fri, Oct 03, 2025 at 06:34:12PM +0200, Benno Lossin wrote:
>> On Thu Oct 2, 2025 at 3:49 PM CEST, Alexandre Courbot wrote:
>> > 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.
Oh by the way, enums are not supported due to a language limitation,
see:
https://github.com/Rust-for-Linux/pin-init/issues/59
>> > 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.
>>
>> Yeah that's the correct workaround at the moment. I'm tracking support
>> for tuple structs in [1]. Essentially the problem is that it requires
>> lots of effort to parse tuple structs using declarative macros. We will
>> get `syn` this cycle, which will enable me to support several things,
>> including tuple structs.
>>
>> [1]: https://github.com/Rust-for-Linux/pin-init/issues/85
>>
>> > 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.
>>
>> This one will also be solved when we switch to syn.
>
> I was planning to submit
> https://github.com/AsahiLinux/linux/commit/2d95fd3b6c359634a0976f27f7a3c667826256da
> https://github.com/AsahiLinux/linux/commit/515638cb47cf0ebdac378686fcbbdc6a8364096a
> from the asahi downstream tree after 6.18-rc1. Does that still make
> sense timing wise?
Probably not, since I'll depend on the syn patches this cycle which will
mean that pin-init supports tuples in 6.19.
> Types with type paths are used extensively in the asahi driver but I can
> initially work around that.
Yeah they should be supported simply by moving to syn, hope it doesn't
introduce too much pain in the next cycle.
---
Cheers,
Benno
On 2025-10-02 at 23:49 +1000, Alexandre Courbot <acourbot@nvidia.com> wrote...
> 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.
I was thinking we should fix the `init!` macro to support tuple structs. Is
there some fundamental reason `init!` couldn't be modified to support tuple
structs? It seems like it would be nicer to fix that limitation rather than work
around it here.
> 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,
> + })
> }
> }
On Fri Oct 3, 2025 at 8:38 AM JST, Alistair Popple wrote:
> On 2025-10-02 at 23:49 +1000, Alexandre Courbot <acourbot@nvidia.com> wrote...
>> 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.
>
> I was thinking we should fix the `init!` macro to support tuple structs. Is
> there some fundamental reason `init!` couldn't be modified to support tuple
> structs? It seems like it would be nicer to fix that limitation rather than work
> around it here.
I took a look at it and quickly got lost in the macros's internals. :)
Let's see what Benno has to say about this. In the meantime, using a
single member is just as valid a constructs as a tuple struct for us.
© 2016 - 2026 Red Hat, Inc.