[PATCH v2 0/2] Drop Opaque from IOCTL args

[PATCH v2 0/2] Drop Opaque from IOCTL args

[Beata Michalska]

Hi All,

This patch series removes the use of `Opaque<T>` for ioctl
arguments in the DRM Rust interface and updates the Nova
code accordingly.

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

Beata Michalska (1):
  rust: drm: Drop the use of Opaque for ioctl arguments

Danilo Krummrich (1):
  drm: nova-drm: Update ioctl handlers to drop Opaque usage

 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

-- 
2.25.1

[PATCH v2 1/2] 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 constraits. By using Opaque<T>,
ioctl handlers are forced to use unsafe code where non is acually
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>
---
 rust/kernel/drm/ioctl.rs | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/rust/kernel/drm/ioctl.rs b/rust/kernel/drm/ioctl.rs
index 445639404fb7..3425a835f9cd 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 as *mut $crate::uapi::$struct) };
                             // SAFETY: This is just the DRM file structure
                             let file = unsafe { $crate::drm::File::as_ref(raw_file) };
 
-- 
2.25.1

[PATCH v2 2/2] drm: nova-drm: Update ioctl handlers to drop Opaque usage

[Beata Michalska]

From: Danilo Krummrich <dakr@kernel.org>

Following the removal of `Opaque<T>` for ioctl arguments in the DRM
framework, this patch updates the affected driver code to use typed
references directly.

Signed-off-by: Danilo Krummrich <dakr@kernel.org>
Signed-off-by: Beata Michalska <beata.michalska@arm.com>
---
 drivers/gpu/drm/nova/file.rs | 23 ++++++--------
 drivers/gpu/drm/nova/nova.rs |  1 -
 drivers/gpu/drm/nova/uapi.rs | 61 ------------------------------------
 3 files changed, 9 insertions(+), 76 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 };
-    }
-}
-- 
2.25.1

Re: [PATCH v2 2/2] drm: nova-drm: Update ioctl handlers to drop Opaque usage

[Danilo Krummrich]

On 6/24/25 11:32 AM, Beata Michalska wrote:
> From: Danilo Krummrich <dakr@kernel.org>
> 
> Following the removal of `Opaque<T>` for ioctl arguments in the DRM
> framework, this patch updates the affected driver code to use typed
> references directly.
> 
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> Signed-off-by: Beata Michalska <beata.michalska@arm.com>

It's very kind you want to attribute the shared diff with a separate patch, but
you have to include this change into patch 1 ("rust: drm: Drop the use of Opaque
for ioctl arguments"), otherwise it still breaks the build intermediately.

Please feel free to just include the change in your patch -- no need for any
attribution as far as I'm concerned. :)

Re: [PATCH v2 2/2] drm: nova-drm: Update ioctl handlers to drop Opaque usage

[Beata Michalska]

On Tue, Jun 24, 2025 at 11:59:25AM +0200, Danilo Krummrich wrote:
> On 6/24/25 11:32 AM, Beata Michalska wrote:
> > From: Danilo Krummrich <dakr@kernel.org>
> > 
> > Following the removal of `Opaque<T>` for ioctl arguments in the DRM
> > framework, this patch updates the affected driver code to use typed
> > references directly.
> > 
> > Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> > Signed-off-by: Beata Michalska <beata.michalska@arm.com>
> 
> It's very kind you want to attribute the shared diff with a separate patch, but
> you have to include this change into patch 1 ("rust: drm: Drop the use of Opaque
> for ioctl arguments"), otherwise it still breaks the build intermediately.
>
It is not so uncommon to send such changes in separate patches, to clearly
distinguish the changes, as long as whole series preserves the build.
That said, I can still send those as a single patch if that is the requirement.

---
BR
Beata
> Please feel free to just include the change in your patch -- no need for any
> attribution as far as I'm concerned. :)

Re: [PATCH v2 2/2] drm: nova-drm: Update ioctl handlers to drop Opaque usage

[Danilo Krummrich]

On Tue, Jun 24, 2025 at 01:09:12PM +0200, Beata Michalska wrote:
> On Tue, Jun 24, 2025 at 11:59:25AM +0200, Danilo Krummrich wrote:
> > On 6/24/25 11:32 AM, Beata Michalska wrote:
> > > From: Danilo Krummrich <dakr@kernel.org>
> > > 
> > > Following the removal of `Opaque<T>` for ioctl arguments in the DRM
> > > framework, this patch updates the affected driver code to use typed
> > > references directly.
> > > 
> > > Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> > > Signed-off-by: Beata Michalska <beata.michalska@arm.com>
> > 
> > It's very kind you want to attribute the shared diff with a separate patch, but
> > you have to include this change into patch 1 ("rust: drm: Drop the use of Opaque
> > for ioctl arguments"), otherwise it still breaks the build intermediately.
> >
> It is not so uncommon to send such changes in separate patches, to clearly
> distinguish the changes, as long as whole series preserves the build.
> That said, I can still send those as a single patch if that is the requirement.

I think it is very uncommon; the general rule is that no patch should ever break
the build.

From [1]:

"When dividing your change into a series of patches, take special care to ensure
that the kernel builds and runs properly after each patch in the series.
Developers using `git bisect` to track down a problem can end up splitting your
patch series at any point; [...]"

[1] https://docs.kernel.org/process/submitting-patches.html#separate-your-changes

> > Please feel free to just include the change in your patch -- no need for any
> > attribution as far as I'm concerned. :)

Re: [PATCH v2 2/2] drm: nova-drm: Update ioctl handlers to drop Opaque usage

[Beata Michalska]

On Tue, Jun 24, 2025 at 01:20:51PM +0200, Danilo Krummrich wrote:
> On Tue, Jun 24, 2025 at 01:09:12PM +0200, Beata Michalska wrote:
> > On Tue, Jun 24, 2025 at 11:59:25AM +0200, Danilo Krummrich wrote:
> > > On 6/24/25 11:32 AM, Beata Michalska wrote:
> > > > From: Danilo Krummrich <dakr@kernel.org>
> > > > 
> > > > Following the removal of `Opaque<T>` for ioctl arguments in the DRM
> > > > framework, this patch updates the affected driver code to use typed
> > > > references directly.
> > > > 
> > > > Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> > > > Signed-off-by: Beata Michalska <beata.michalska@arm.com>
> > > 
> > > It's very kind you want to attribute the shared diff with a separate patch, but
> > > you have to include this change into patch 1 ("rust: drm: Drop the use of Opaque
> > > for ioctl arguments"), otherwise it still breaks the build intermediately.
> > >
> > It is not so uncommon to send such changes in separate patches, to clearly
> > distinguish the changes, as long as whole series preserves the build.
> > That said, I can still send those as a single patch if that is the requirement.
> 
> I think it is very uncommon; the general rule is that no patch should ever break
> the build.
>
Noted.
> From [1]:
> 
> "When dividing your change into a series of patches, take special care to ensure
> that the kernel builds and runs properly after each patch in the series.
> Developers using `git bisect` to track down a problem can end up splitting your
> patch series at any point; [...]"
> 
> [1] https://docs.kernel.org/process/submitting-patches.html#separate-your-changes
> 
> > > Please feel free to just include the change in your patch -- no need for any
> > > attribution as far as I'm concerned. :)