| 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: <856327A4-2D29-4340-984E-97E2002CCAE8@collabora.com>
Date: Thu, 4 Sep 2025 09:51:41 -0300
From: Daniel Almeida <daniel.almeida@...labora.com>
To: Lyude Paul <lyude@...hat.com>
Cc: dri-devel@...ts.freedesktop.org,
rust-for-linux@...r.kernel.org,
linux-kernel@...r.kernel.org,
Danilo Krummrich <dakr@...nel.org>,
Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
Maxime Ripard <mripard@...nel.org>,
Thomas Zimmermann <tzimmermann@...e.de>,
David Airlie <airlied@...il.com>,
Simona Vetter <simona@...ll.ch>,
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>,
Benno Lossin <lossin@...nel.org>,
Andreas Hindborg <a.hindborg@...nel.org>,
Alice Ryhl <aliceryhl@...gle.com>,
Trevor Gross <tmgross@...ch.edu>,
Asahi Lina <lina+kernel@...hilina.net>,
"open list:DRM DRIVER FOR NVIDIA GPUS [RUST]" <nouveau@...ts.freedesktop.org>
Subject: Re: [PATCH v3 04/14] rust: drm: gem: Support driver-private GEM
object types
> On 29 Aug 2025, at 19:35, Lyude Paul <lyude@...hat.com> wrote:
>
> One of the original intents with the gem bindings was that drivers could
> specify additional gem implementations, in order to enable for driver
> private gem objects. This wasn't really possible however, as up until now
> our GEM bindings have always assumed that the only GEM object we would run
> into was driver::Driver::Object - meaning that implementing another GEM
> object type would result in all of the BaseDriverObject callbacks assuming
> the wrong type.
>
> This is a pretty easy fix though, all we need to do is specify a
> BaseDriverObject in driver::Driver instead of an AllocImpl, and then add an
> associated type for AllocImpl in BaseDriverObject. That way each
> BaseDriverObject has its own AllocImpl allowing it to know which type to
> provide in BaseDriverObject callbacks, and driver::Driver can simply go
> through the BaseDriverObject to its AllocImpl type in order to get access
> to ALLOC_OPS.
>
> So, let's do this and update Nova for these changes.
>
> Signed-off-by: Lyude Paul <lyude@...hat.com>
>
> ---
> V4:
> * Update trait bounds. This looks gnarlier then it is:
>
> Self: AllocImpl<Driver = D>, <-- Get the driver for this GEM object
> D: drm::Driver<Object = O, File = F>, <-- Get the driver's Object, File
> impl
> F: drm::file::DriverFile,
> O: BaseDriverObject<Object = Self>, <-- Make sure we're the driver's
> main GEM object impl.
> (don't worry, the compiler can always figure out what D, F, O are)
> * Also, rename the commit. I realized I should be clearer about what this
> does so people can stop me if this isn't what was meant by private gem
> object implementations :).
>
> Signed-off-by: Lyude Paul <lyude@...hat.com>
> ---
> drivers/gpu/drm/nova/driver.rs | 8 ++++++--
> drivers/gpu/drm/nova/gem.rs | 1 +
> rust/kernel/drm/device.rs | 17 ++++++++++-------
> rust/kernel/drm/driver.rs | 2 +-
> rust/kernel/drm/gem/mod.rs | 21 +++++++++++++--------
> 5 files changed, 31 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/gpu/drm/nova/driver.rs b/drivers/gpu/drm/nova/driver.rs
> index 91b7380f83ab4..4c252426056c5 100644
> --- a/drivers/gpu/drm/nova/driver.rs
> +++ b/drivers/gpu/drm/nova/driver.rs
> @@ -1,7 +1,11 @@
> // SPDX-License-Identifier: GPL-2.0
>
> use kernel::{
> - auxiliary, c_str, device::Core, drm, drm::gem, drm::ioctl, prelude::*, sync::aref::ARef,
> + auxiliary, c_str,
> + device::Core,
> + drm::{self, gem, ioctl},
> + prelude::*,
> + types::ARef,
> };
>
> use crate::file::File;
> @@ -59,7 +63,7 @@ fn probe(adev: &auxiliary::Device<Core>, _info: &Self::IdInfo) -> Result<Pin<KBo
> impl drm::Driver for NovaDriver {
> type Data = NovaData;
> type File = File;
> - type Object = gem::Object<NovaObject>;
> + type Object = NovaObject;
>
> const INFO: drm::DriverInfo = INFO;
>
> diff --git a/drivers/gpu/drm/nova/gem.rs b/drivers/gpu/drm/nova/gem.rs
> index 2760ba4f3450b..10e3053f1a246 100644
> --- a/drivers/gpu/drm/nova/gem.rs
> +++ b/drivers/gpu/drm/nova/gem.rs
> @@ -18,6 +18,7 @@ pub(crate) struct NovaObject {}
>
> impl gem::DriverObject for NovaObject {
> type Driver = NovaDriver;
> + type Object = gem::Object<Self>;
>
> fn new(_dev: &NovaDevice, _size: usize) -> impl PinInit<Self, Error> {
> try_pin_init!(NovaObject {})
> diff --git a/rust/kernel/drm/device.rs b/rust/kernel/drm/device.rs
> index 3bb7c83966cf2..16cf6cb53d9a7 100644
> --- a/rust/kernel/drm/device.rs
> +++ b/rust/kernel/drm/device.rs
> @@ -60,6 +60,9 @@ pub struct Device<T: drm::Driver> {
> data: T::Data,
> }
>
> +/// A type alias for referring to the [`AllocImpl`] implementation for a DRM driver.
> +type DriverAllocImpl<T> = <<T as drm::Driver>::Object as drm::gem::DriverObject>::Object;
> +
> impl<T: drm::Driver> Device<T> {
> const VTABLE: bindings::drm_driver = drm_legacy_fields! {
> load: None,
> @@ -70,13 +73,13 @@ impl<T: drm::Driver> Device<T> {
> master_set: None,
> master_drop: None,
> debugfs_init: None,
> - gem_create_object: T::Object::ALLOC_OPS.gem_create_object,
> - prime_handle_to_fd: T::Object::ALLOC_OPS.prime_handle_to_fd,
> - prime_fd_to_handle: T::Object::ALLOC_OPS.prime_fd_to_handle,
> - gem_prime_import: T::Object::ALLOC_OPS.gem_prime_import,
> - gem_prime_import_sg_table: T::Object::ALLOC_OPS.gem_prime_import_sg_table,
> - dumb_create: T::Object::ALLOC_OPS.dumb_create,
> - dumb_map_offset: T::Object::ALLOC_OPS.dumb_map_offset,
> + gem_create_object: DriverAllocImpl::<T>::ALLOC_OPS.gem_create_object,
> + prime_handle_to_fd: DriverAllocImpl::<T>::ALLOC_OPS.prime_handle_to_fd,
> + prime_fd_to_handle: DriverAllocImpl::<T>::ALLOC_OPS.prime_fd_to_handle,
> + gem_prime_import: DriverAllocImpl::<T>::ALLOC_OPS.gem_prime_import,
> + gem_prime_import_sg_table: DriverAllocImpl::<T>::ALLOC_OPS.gem_prime_import_sg_table,
> + dumb_create: DriverAllocImpl::<T>::ALLOC_OPS.dumb_create,
> + dumb_map_offset: DriverAllocImpl::<T>::ALLOC_OPS.dumb_map_offset,
There are now possibly infinitely many ALLOC_OPS. Is it a problem that
we’re choosing a particular one here?
> show_fdinfo: None,
> fbdev_probe: None,
>
> diff --git a/rust/kernel/drm/driver.rs b/rust/kernel/drm/driver.rs
> index dae0f4d1bbe3c..2500a61f45a6a 100644
> --- a/rust/kernel/drm/driver.rs
> +++ b/rust/kernel/drm/driver.rs
> @@ -103,7 +103,7 @@ pub trait Driver {
> type Data: Sync + Send;
>
> /// The type used to manage memory for this driver.
> - type Object: AllocImpl;
> + type Object: drm::gem::DriverObject;
>
> /// The type used to represent a DRM File (client)
> type File: drm::file::DriverFile;
> diff --git a/rust/kernel/drm/gem/mod.rs b/rust/kernel/drm/gem/mod.rs
> index b27b9fbf28bbb..ec36cd9ea69ed 100644
> --- a/rust/kernel/drm/gem/mod.rs
> +++ b/rust/kernel/drm/gem/mod.rs
> @@ -26,16 +26,19 @@ pub trait DriverObject: Sync + Send + Sized {
> /// Parent `Driver` for this object.
> type Driver: drm::Driver;
>
> + /// The GEM object type that will be passed to various callbacks.
> + type Object: AllocImpl;
> +
> /// Create a new driver data object for a GEM object of a given size.
> fn new(dev: &drm::Device<Self::Driver>, size: usize) -> impl PinInit<Self, Error>;
>
> /// Open a new handle to an existing object, associated with a File.
> - fn open(_obj: &<Self::Driver as drm::Driver>::Object, _file: &DriverFile<Self>) -> Result {
> + fn open(_obj: &Self::Object, _file: &DriverFile<Self>) -> Result {
> Ok(())
> }
>
> /// Close a handle to an existing object, associated with a File.
> - fn close(_obj: &<Self::Driver as drm::Driver>::Object, _file: &DriverFile<Self>) {}
> + fn close(_obj: &Self::Object, _file: &DriverFile<Self>) {}
> }
>
> /// Trait that represents a GEM object subtype
> @@ -83,7 +86,7 @@ extern "C" fn open_callback<T: DriverObject>(
>
> // SAFETY: `open_callback` is specified in the AllocOps structure for `DriverObject<T>`,
> // ensuring that `raw_obj` is contained within a `DriverObject<T>`
> - let obj = unsafe { <<T::Driver as drm::Driver>::Object as IntoGEMObject>::from_raw(raw_obj) };
> + let obj = unsafe { T::Object::from_raw(raw_obj) };
>
> match T::open(obj, file) {
> Err(e) => e.to_errno(),
> @@ -100,7 +103,7 @@ extern "C" fn close_callback<T: DriverObject>(
>
> // SAFETY: `close_callback` is specified in the AllocOps structure for `Object<T>`, ensuring
> // that `raw_obj` is indeed contained within a `Object<T>`.
> - let obj = unsafe { <<T::Driver as drm::Driver>::Object as IntoGEMObject>::from_raw(raw_obj) };
> + let obj = unsafe { T::Object::from_raw(raw_obj) };
>
> T::close(obj, file);
> }
> @@ -127,11 +130,12 @@ fn size(&self) -> usize {
>
> /// Creates a new handle for the object associated with a given `File`
> /// (or returns an existing one).
> - fn create_handle<D, F>(&self, file: &drm::File<F>) -> Result<u32>
> + fn create_handle<D, F, O>(&self, file: &drm::File<F>) -> Result<u32>
> where
> Self: AllocImpl<Driver = D>,
> - D: drm::Driver<Object = Self, File = F>,
> + D: drm::Driver<Object = O, File = F>,
> F: drm::file::DriverFile,
> + O: DriverObject<Object = Self>,
> {
> let mut handle: u32 = 0;
> // SAFETY: The arguments are all valid per the type invariants.
> @@ -142,11 +146,12 @@ fn create_handle<D, F>(&self, file: &drm::File<F>) -> Result<u32>
> }
>
> /// Looks up an object by its handle for a given `File`.
> - fn lookup_handle<D, F>(file: &drm::File<F>, handle: u32) -> Result<ARef<Self>>
> + fn lookup_handle<D, F, O>(file: &drm::File<F>, handle: u32) -> Result<ARef<Self>>
> where
> Self: AllocImpl<Driver = D>,
> - D: drm::Driver<Object = Self, File = F>,
> + D: drm::Driver<Object = O, File = F>,
> F: drm::file::DriverFile,
> + O: DriverObject<Object = Self>,
> {
> // SAFETY: The arguments are all valid per the type invariants.
> let ptr = unsafe { bindings::drm_gem_object_lookup(file.as_raw().cast(), handle) };
> --
> 2.50.0
>
Powered by blists - more mailing lists