| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-Id: <DD7VU4239GS2.2MKVFPBFEY1R4@nvidia.com>
Date: Thu, 02 Oct 2025 22:49:03 +0900
From: "Alexandre Courbot" <acourbot@...dia.com>
To: "Alistair Popple" <apopple@...dia.com>, "Benno Lossin"
<lossin@...nel.org>, <rust-for-linux@...r.kernel.org>,
<dri-devel@...ts.freedesktop.org>, <dakr@...nel.org>, <acourbot@...dia.com>
Cc: "Miguel Ojeda" <ojeda@...nel.org>, "Alex Gaynor"
<alex.gaynor@...il.com>, "Boqun Feng" <boqun.feng@...il.com>, "Gary Guo"
<gary@...yguo.net>, Björn Roy Baron
<bjorn3_gh@...tonmail.com>, "Andreas Hindborg" <a.hindborg@...nel.org>,
"Alice Ryhl" <aliceryhl@...gle.com>, "Trevor Gross" <tmgross@...ch.edu>,
"David Airlie" <airlied@...il.com>, "Simona Vetter" <simona@...ll.ch>,
"Maarten Lankhorst" <maarten.lankhorst@...ux.intel.com>, "Maxime Ripard"
<mripard@...nel.org>, "Thomas Zimmermann" <tzimmermann@...e.de>, "John
Hubbard" <jhubbard@...dia.com>, "Joel Fernandes" <joelagnelf@...dia.com>,
"Timur Tabi" <ttabi@...dia.com>, <linux-kernel@...r.kernel.org>,
<nouveau@...ts.freedesktop.org>
Subject: Re: [PATCH v3 08/13] gpu: nova-core: Add bindings and accessors for
GspSystemInfo
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@...dia.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,
+ })
}
}
Powered by blists - more mailing lists