On 2025-10-02 at 23:49 +1000, Alexandre Courbot <acour...@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 <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.
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-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,
> + })
> }
> }
