[PATCH v5] rust: drm: Drop the use of Opaque for ioctl arguments

[PATCH v5] rust: drm: Drop the use of Opaque for ioctl arguments

[Beata Michalska]

With the Opaque<T>, the expectations are that Rust should not
make any assumptions on the layout or invariants of the wrapped
C types. That runs rather counter to ioctl arguments, which must
adhere to certain data-layout constraints. By using Opaque<T>,
ioctl handlers are forced to use unsafe code where none is actually
needed. This adds needless complexity and maintenance overhead,
brining no safety benefits.
Drop the use of Opaque for ioctl arguments as that is not the best
fit here.

Signed-off-by: Beata Michalska <beata.michalska@arm.com>
Acked-by: Danilo Krummrich <dakr@kernel.org>
Reviewed-by: Boqun Feng <boqun.feng@gmail.com>
---

Changes in v5:
- Use `.cast::<T>()` for ptr casting

Changes in v4:
- Fixed typo in commit message

Changes in v3:
- Squashed the changes into single commit

Changes in v2:
- Missing Nova changes now included
- Fixed formatting and added comment on safety guarantees


 drivers/gpu/drm/nova/file.rs | 23 ++++++--------
 drivers/gpu/drm/nova/nova.rs |  1 -
 drivers/gpu/drm/nova/uapi.rs | 61 ------------------------------------
 rust/kernel/drm/ioctl.rs     | 11 ++++---
 4 files changed, 16 insertions(+), 80 deletions(-)
 delete mode 100644 drivers/gpu/drm/nova/uapi.rs

diff --git a/drivers/gpu/drm/nova/file.rs b/drivers/gpu/drm/nova/file.rs
index 7e59a34b830d..7e7d4e2de2fb 100644
--- a/drivers/gpu/drm/nova/file.rs
+++ b/drivers/gpu/drm/nova/file.rs
@@ -2,13 +2,11 @@
 
 use crate::driver::{NovaDevice, NovaDriver};
 use crate::gem::NovaObject;
-use crate::uapi::{GemCreate, GemInfo, Getparam};
 use kernel::{
     alloc::flags::*,
     drm::{self, gem::BaseObject},
     pci,
     prelude::*,
-    types::Opaque,
     uapi,
 };
 
@@ -26,20 +24,19 @@ impl File {
     /// IOCTL: get_param: Query GPU / driver metadata.
     pub(crate) fn get_param(
         dev: &NovaDevice,
-        getparam: &Opaque<uapi::drm_nova_getparam>,
+        getparam: &mut uapi::drm_nova_getparam,
         _file: &drm::File<File>,
     ) -> Result<u32> {
         let adev = &dev.adev;
         let parent = adev.parent().ok_or(ENOENT)?;
         let pdev: &pci::Device = parent.try_into()?;
-        let getparam: &Getparam = getparam.into();
 
-        let value = match getparam.param() as u32 {
+        let value = match getparam.param as u32 {
             uapi::NOVA_GETPARAM_VRAM_BAR_SIZE => pdev.resource_len(1)?,
             _ => return Err(EINVAL),
         };
 
-        getparam.set_value(value);
+        getparam.value = value;
 
         Ok(0)
     }
@@ -47,13 +44,12 @@ pub(crate) fn get_param(
     /// IOCTL: gem_create: Create a new DRM GEM object.
     pub(crate) fn gem_create(
         dev: &NovaDevice,
-        req: &Opaque<uapi::drm_nova_gem_create>,
+        req: &mut uapi::drm_nova_gem_create,
         file: &drm::File<File>,
     ) -> Result<u32> {
-        let req: &GemCreate = req.into();
-        let obj = NovaObject::new(dev, req.size().try_into()?)?;
+        let obj = NovaObject::new(dev, req.size.try_into()?)?;
 
-        req.set_handle(obj.create_handle(file)?);
+        req.handle = obj.create_handle(file)?;
 
         Ok(0)
     }
@@ -61,13 +57,12 @@ pub(crate) fn gem_create(
     /// IOCTL: gem_info: Query GEM metadata.
     pub(crate) fn gem_info(
         _dev: &NovaDevice,
-        req: &Opaque<uapi::drm_nova_gem_info>,
+        req: &mut uapi::drm_nova_gem_info,
         file: &drm::File<File>,
     ) -> Result<u32> {
-        let req: &GemInfo = req.into();
-        let bo = NovaObject::lookup_handle(file, req.handle())?;
+        let bo = NovaObject::lookup_handle(file, req.handle)?;
 
-        req.set_size(bo.size().try_into()?);
+        req.size = bo.size().try_into()?;
 
         Ok(0)
     }
diff --git a/drivers/gpu/drm/nova/nova.rs b/drivers/gpu/drm/nova/nova.rs
index 902876aa14d1..730598defe04 100644
--- a/drivers/gpu/drm/nova/nova.rs
+++ b/drivers/gpu/drm/nova/nova.rs
@@ -5,7 +5,6 @@
 mod driver;
 mod file;
 mod gem;
-mod uapi;
 
 use crate::driver::NovaDriver;
 
diff --git a/drivers/gpu/drm/nova/uapi.rs b/drivers/gpu/drm/nova/uapi.rs
deleted file mode 100644
index eb228a58d423..000000000000
--- a/drivers/gpu/drm/nova/uapi.rs
+++ /dev/null
@@ -1,61 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0
-
-use kernel::uapi;
-
-// TODO Work out some common infrastructure to avoid boilerplate code for uAPI abstractions.
-
-macro_rules! define_uapi_abstraction {
-    ($name:ident <= $inner:ty) => {
-        #[repr(transparent)]
-        pub struct $name(::kernel::types::Opaque<$inner>);
-
-        impl ::core::convert::From<&::kernel::types::Opaque<$inner>> for &$name {
-            fn from(value: &::kernel::types::Opaque<$inner>) -> Self {
-                // SAFETY: `Self` is a transparent wrapper of `$inner`.
-                unsafe { ::core::mem::transmute(value) }
-            }
-        }
-    };
-}
-
-define_uapi_abstraction!(Getparam <= uapi::drm_nova_getparam);
-
-impl Getparam {
-    pub fn param(&self) -> u64 {
-        // SAFETY: `self.get()` is a valid pointer to a `struct drm_nova_getparam`.
-        unsafe { (*self.0.get()).param }
-    }
-
-    pub fn set_value(&self, v: u64) {
-        // SAFETY: `self.get()` is a valid pointer to a `struct drm_nova_getparam`.
-        unsafe { (*self.0.get()).value = v };
-    }
-}
-
-define_uapi_abstraction!(GemCreate <= uapi::drm_nova_gem_create);
-
-impl GemCreate {
-    pub fn size(&self) -> u64 {
-        // SAFETY: `self.get()` is a valid pointer to a `struct drm_nova_gem_create`.
-        unsafe { (*self.0.get()).size }
-    }
-
-    pub fn set_handle(&self, handle: u32) {
-        // SAFETY: `self.get()` is a valid pointer to a `struct drm_nova_gem_create`.
-        unsafe { (*self.0.get()).handle = handle };
-    }
-}
-
-define_uapi_abstraction!(GemInfo <= uapi::drm_nova_gem_info);
-
-impl GemInfo {
-    pub fn handle(&self) -> u32 {
-        // SAFETY: `self.get()` is a valid pointer to a `struct drm_nova_gem_info`.
-        unsafe { (*self.0.get()).handle }
-    }
-
-    pub fn set_size(&self, size: u64) {
-        // SAFETY: `self.get()` is a valid pointer to a `struct drm_nova_gem_info`.
-        unsafe { (*self.0.get()).size = size };
-    }
-}
diff --git a/rust/kernel/drm/ioctl.rs b/rust/kernel/drm/ioctl.rs
index 445639404fb7..cb30b9ad1808 100644
--- a/rust/kernel/drm/ioctl.rs
+++ b/rust/kernel/drm/ioctl.rs
@@ -83,7 +83,7 @@ pub mod internal {
 ///
 /// ```ignore
 /// fn foo(device: &kernel::drm::Device<Self>,
-///        data: &Opaque<uapi::argument_type>,
+///        data: &mut uapi::argument_type,
 ///        file: &kernel::drm::File<Self::File>,
 /// ) -> Result<u32>
 /// ```
@@ -138,9 +138,12 @@ pub mod internal {
                             // SAFETY: The ioctl argument has size `_IOC_SIZE(cmd)`, which we
                             // asserted above matches the size of this type, and all bit patterns of
                             // UAPI structs must be valid.
-                            let data = unsafe {
-                                &*(raw_data as *const $crate::types::Opaque<$crate::uapi::$struct>)
-                            };
+                            // The `ioctl` argument is exclusively owned by the handler
+                            // and guaranteed by the C implementation (`drm_ioctl()`) to remain
+                            // valid for the entire lifetime of the reference taken here.
+                            // There is no concurrent access or aliasing; no other references
+                            // to this object exist during this call.
+                            let data = unsafe { &mut *(raw_data.cast::<$crate::uapi::$struct>()) };
                             // SAFETY: This is just the DRM file structure
                             let file = unsafe { $crate::drm::File::as_ref(raw_file) };
 
-- 
2.25.1

Re: [PATCH v5] rust: drm: Drop the use of Opaque for ioctl arguments

[Alice Ryhl]

On Thu, Jun 26, 2025 at 06:23:13PM +0200, Beata Michalska wrote:
> With the Opaque<T>, the expectations are that Rust should not
> make any assumptions on the layout or invariants of the wrapped
> C types. That runs rather counter to ioctl arguments, which must
> adhere to certain data-layout constraints. By using Opaque<T>,
> ioctl handlers are forced to use unsafe code where none is actually
> needed. This adds needless complexity and maintenance overhead,
> brining no safety benefits.
> Drop the use of Opaque for ioctl arguments as that is not the best
> fit here.
> 
> Signed-off-by: Beata Michalska <beata.michalska@arm.com>
> Acked-by: Danilo Krummrich <dakr@kernel.org>
> Reviewed-by: Boqun Feng <boqun.feng@gmail.com>

I'm guessing this should go through the DRM tree?

Reviewed-by: Alice Ryhl <aliceryhl@google.com>

Re: [PATCH v5] rust: drm: Drop the use of Opaque for ioctl arguments

[Danilo Krummrich]

On 7/21/25 4:55 PM, Alice Ryhl wrote:
> On Thu, Jun 26, 2025 at 06:23:13PM +0200, Beata Michalska wrote:
>> With the Opaque<T>, the expectations are that Rust should not
>> make any assumptions on the layout or invariants of the wrapped
>> C types. That runs rather counter to ioctl arguments, which must
>> adhere to certain data-layout constraints. By using Opaque<T>,
>> ioctl handlers are forced to use unsafe code where none is actually
>> needed. This adds needless complexity and maintenance overhead,
>> brining no safety benefits.
>> Drop the use of Opaque for ioctl arguments as that is not the best
>> fit here.
>>
>> Signed-off-by: Beata Michalska <beata.michalska@arm.com>
>> Acked-by: Danilo Krummrich <dakr@kernel.org>
>> Reviewed-by: Boqun Feng <boqun.feng@gmail.com>
> 
> I'm guessing this should go through the DRM tree?
> 
> Reviewed-by: Alice Ryhl <aliceryhl@google.com>

Yeah, I can pick it up, but it won't be considered for the upcoming merge window
anymore, but for the next. After -rc6 drm-misc is in feature freeze and I also
already send the PR for Nova.

Daniel, Beata: Is there a reason you need this earlier?

Re: [PATCH v5] rust: drm: Drop the use of Opaque for ioctl arguments

[Daniel Almeida]

Hi Danilo,

> 
> Yeah, I can pick it up, but it won't be considered for the upcoming merge window
> anymore, but for the next. After -rc6 drm-misc is in feature freeze and I also
> already send the PR for Nova.
> 
> Daniel, Beata: Is there a reason you need this earlier?

IIUC, this will be merged into drm-misc-next, but it will not be picked up in
the PR for the current merge window, is that correct?

If that's the case, then I don't think there will be any issues because Tyr
will go through drm-misc anyway. This means that if you pick it up, we will
have this patch as soon as we rebase on drm-misc-next regardless of where we
are in the cycle.

— Daniel

Re: [PATCH v5] rust: drm: Drop the use of Opaque for ioctl arguments

[Danilo Krummrich]

On Thu Jun 26, 2025 at 6:23 PM CEST, Beata Michalska wrote:
> With the Opaque<T>, the expectations are that Rust should not
> make any assumptions on the layout or invariants of the wrapped
> C types. That runs rather counter to ioctl arguments, which must
> adhere to certain data-layout constraints. By using Opaque<T>,
> ioctl handlers are forced to use unsafe code where none is actually
> needed. This adds needless complexity and maintenance overhead,
> brining no safety benefits.
> Drop the use of Opaque for ioctl arguments as that is not the best
> fit here.
>
> Signed-off-by: Beata Michalska <beata.michalska@arm.com>
> Acked-by: Danilo Krummrich <dakr@kernel.org>
> Reviewed-by: Boqun Feng <boqun.feng@gmail.com>

The patch does not apply on top of drm-misc-next and does not have a base
revision.

Can you please let me know which commit this patch applies on top of or resend?

- Danilo

Re: [PATCH v5] rust: drm: Drop the use of Opaque for ioctl arguments

[Daniel Almeida]

> On 21 Jul 2025, at 12:33, Danilo Krummrich <dakr@kernel.org> wrote:
> 
> On Thu Jun 26, 2025 at 6:23 PM CEST, Beata Michalska wrote:
>> With the Opaque<T>, the expectations are that Rust should not
>> make any assumptions on the layout or invariants of the wrapped
>> C types. That runs rather counter to ioctl arguments, which must
>> adhere to certain data-layout constraints. By using Opaque<T>,
>> ioctl handlers are forced to use unsafe code where none is actually
>> needed. This adds needless complexity and maintenance overhead,
>> brining no safety benefits.
>> Drop the use of Opaque for ioctl arguments as that is not the best
>> fit here.
>> 
>> Signed-off-by: Beata Michalska <beata.michalska@arm.com>
>> Acked-by: Danilo Krummrich <dakr@kernel.org>
>> Reviewed-by: Boqun Feng <boqun.feng@gmail.com>
> 
> The patch does not apply on top of drm-misc-next and does not have a base
> revision.
> 
> Can you please let me know which commit this patch applies on top of or resend?
> 
> - Danilo

I applied this locally on top of nova-next. Nova still builds, but I don’t
have the hardware to test.

— Daniel

Re: [PATCH v5] rust: drm: Drop the use of Opaque for ioctl arguments

[Daniel Almeida]

> On 26 Jun 2025, at 13:23, Beata Michalska <beata.michalska@arm.com> wrote:
> 
> With the Opaque<T>, the expectations are that Rust should not
> make any assumptions on the layout or invariants of the wrapped
> C types. That runs rather counter to ioctl arguments, which must
> adhere to certain data-layout constraints. By using Opaque<T>,
> ioctl handlers are forced to use unsafe code where none is actually
> needed. This adds needless complexity and maintenance overhead,
> brining no safety benefits.
> Drop the use of Opaque for ioctl arguments as that is not the best
> fit here.
> 
> Signed-off-by: Beata Michalska <beata.michalska@arm.com>
> Acked-by: Danilo Krummrich <dakr@kernel.org>
> Reviewed-by: Boqun Feng <boqun.feng@gmail.com>

Reviewed-by: Daniel Almeida <daniel.almeida@collabora.com> 

Re: [PATCH v5] rust: drm: Drop the use of Opaque for ioctl arguments

[Danilo Krummrich]

On Thu Jun 26, 2025 at 6:23 PM CEST, Beata Michalska wrote:
> With the Opaque<T>, the expectations are that Rust should not
> make any assumptions on the layout or invariants of the wrapped
> C types. That runs rather counter to ioctl arguments, which must
> adhere to certain data-layout constraints. By using Opaque<T>,
> ioctl handlers are forced to use unsafe code where none is actually
> needed. This adds needless complexity and maintenance overhead,
> brining no safety benefits.
> Drop the use of Opaque for ioctl arguments as that is not the best
> fit here.
>
> Signed-off-by: Beata Michalska <beata.michalska@arm.com>
> Acked-by: Danilo Krummrich <dakr@kernel.org>
> Reviewed-by: Boqun Feng <boqun.feng@gmail.com>

Applied to drm-misc-next, thanks!