[RFC,01/18] rust: drm: ioctl: Add DRM ioctl abstraction

  DRM drivers need to be able to declare which driver-specific ioctls they
support. This abstraction adds the required types and a helper macro to
generate the ioctl definition inside the DRM driver.

Note that this macro is not usable until further bits of the
abstraction are in place (but it will not fail to compile on its own, if
not called).

Signed-off-by: Asahi Lina <lina@asahilina.net>
---
 drivers/gpu/drm/Kconfig         |   7 ++
 rust/bindings/bindings_helper.h |   2 +
 rust/kernel/drm/ioctl.rs        | 147 ++++++++++++++++++++++++++++++++++++++++
 rust/kernel/drm/mod.rs          |   5 ++
 rust/kernel/lib.rs              |   2 +
 5 files changed, 163 insertions(+)
  

On Tue, Mar 7, 2023 at 3:27 PM Asahi Lina <lina@asahilina.net> wrote:
>
> DRM drivers need to be able to declare which driver-specific ioctls they
> support. This abstraction adds the required types and a helper macro to
> generate the ioctl definition inside the DRM driver.
>
> Note that this macro is not usable until further bits of the
> abstraction are in place (but it will not fail to compile on its own, if
> not called).
>
> Signed-off-by: Asahi Lina <lina@asahilina.net>
> ---
>  drivers/gpu/drm/Kconfig         |   7 ++
>  rust/bindings/bindings_helper.h |   2 +
>  rust/kernel/drm/ioctl.rs        | 147 ++++++++++++++++++++++++++++++++++++++++
>  rust/kernel/drm/mod.rs          |   5 ++
>  rust/kernel/lib.rs              |   2 +
>  5 files changed, 163 insertions(+)
>
> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> index dc0f94f02a82..dab8f0f9aa96 100644
> --- a/drivers/gpu/drm/Kconfig
> +++ b/drivers/gpu/drm/Kconfig
> @@ -27,6 +27,13 @@ menuconfig DRM
>           details.  You should also select and configure AGP
>           (/dev/agpgart) support if it is available for your platform.
>
> +# Rust abstractions cannot be built as modules currently, so force them as
> +# bool by using these intermediate symbols. In the future these could be
> +# tristate once abstractions themselves can be built as modules.
> +config RUST_DRM
> +       bool "Rust support for the DRM subsystem"
> +       depends on DRM=y
> +
>  config DRM_MIPI_DBI
>         tristate
>         depends on DRM
> diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> index 91bb7906ca5a..2687bef1676f 100644
> --- a/rust/bindings/bindings_helper.h
> +++ b/rust/bindings/bindings_helper.h
> @@ -6,6 +6,7 @@
>   * Sorted alphabetically.
>   */
>
> +#include <drm/drm_ioctl.h>
>  #include <linux/delay.h>
>  #include <linux/device.h>
>  #include <linux/dma-mapping.h>
> @@ -23,6 +24,7 @@
>  #include <linux/sysctl.h>
>  #include <linux/timekeeping.h>
>  #include <linux/xarray.h>
> +#include <uapi/drm/drm.h>
>

might make more sense to add this chunk to the patch actually needing it

>  /* `bindgen` gets confused at certain things. */
>  const gfp_t BINDINGS_GFP_KERNEL = GFP_KERNEL;
> diff --git a/rust/kernel/drm/ioctl.rs b/rust/kernel/drm/ioctl.rs
> new file mode 100644
> index 000000000000..10304efbd5f1
> --- /dev/null
> +++ b/rust/kernel/drm/ioctl.rs
> @@ -0,0 +1,147 @@
> +// SPDX-License-Identifier: GPL-2.0 OR MIT
> +#![allow(non_snake_case)]
> +
> +//! DRM IOCTL definitions.
> +//!
> +//! C header: [`include/linux/drm/drm_ioctl.h`](../../../../include/linux/drm/drm_ioctl.h)
> +
> +use crate::ioctl;
> +
> +const BASE: u32 = bindings::DRM_IOCTL_BASE as u32;
> +
> +/// Construct a DRM ioctl number with no argument.
> +pub const fn IO(nr: u32) -> u32 {
> +    ioctl::_IO(BASE, nr)
> +}
> +
> +/// Construct a DRM ioctl number with a read-only argument.
> +pub const fn IOR<T>(nr: u32) -> u32 {
> +    ioctl::_IOR::<T>(BASE, nr)
> +}
> +
> +/// Construct a DRM ioctl number with a write-only argument.
> +pub const fn IOW<T>(nr: u32) -> u32 {
> +    ioctl::_IOW::<T>(BASE, nr)
> +}
> +
> +/// Construct a DRM ioctl number with a read-write argument.
> +pub const fn IOWR<T>(nr: u32) -> u32 {
> +    ioctl::_IOWR::<T>(BASE, nr)
> +}
> +
> +/// Descriptor type for DRM ioctls. Use the `declare_drm_ioctls!{}` macro to construct them.
> +pub type DrmIoctlDescriptor = bindings::drm_ioctl_desc;
> +
> +/// This is for ioctl which are used for rendering, and require that the file descriptor is either
> +/// for a render node, or if it’s a legacy/primary node, then it must be authenticated.
> +pub const AUTH: u32 = bindings::drm_ioctl_flags_DRM_AUTH;
> +
> +/// This must be set for any ioctl which can change the modeset or display state. Userspace must
> +/// call the ioctl through a primary node, while it is the active master.
> +///
> +/// Note that read-only modeset ioctl can also be called by unauthenticated clients, or when a
> +/// master is not the currently active one.
> +pub const MASTER: u32 = bindings::drm_ioctl_flags_DRM_MASTER;
> +
> +/// Anything that could potentially wreak a master file descriptor needs to have this flag set.
> +///
> +/// Current that’s only for the SETMASTER and DROPMASTER ioctl, which e.g. logind can call to force
> +/// a non-behaving master (display compositor) into compliance.
> +///
> +/// This is equivalent to callers with the SYSADMIN capability.
> +pub const ROOT_ONLY: u32 = bindings::drm_ioctl_flags_DRM_ROOT_ONLY;
> +
> +/// Whether drm_ioctl_desc.func should be called with the DRM BKL held or not. Enforced as the
> +/// default for all modern drivers, hence there should never be a need to set this flag.
> +///
> +/// Do not use anywhere else than for the VBLANK_WAIT IOCTL, which is the only legacy IOCTL which
> +/// needs this.
> +pub const UNLOCKED: u32 = bindings::drm_ioctl_flags_DRM_UNLOCKED;
> +
> +/// This is used for all ioctl needed for rendering only, for drivers which support render nodes.
> +/// This should be all new render drivers, and hence it should be always set for any ioctl with
> +/// `AUTH` set. Note though that read-only query ioctl might have this set, but have not set
> +/// DRM_AUTH because they do not require authentication.
> +pub const RENDER_ALLOW: u32 = bindings::drm_ioctl_flags_DRM_RENDER_ALLOW;
> +
> +/// Declare the DRM ioctls for a driver.
> +///
> +/// Each entry in the list should have the form:
> +///
> +/// `(ioctl_number, argument_type, flags, user_callback),`
> +///
> +/// `argument_type` is the type name within the `bindings` crate.
> +/// `user_callback` should have the following prototype:
> +///
> +/// ```
> +/// fn foo(device: &kernel::drm::device::Device<Self>,
> +///        data: &mut bindings::argument_type,
> +///        file: &kernel::drm::file::File<Self::File>,
> +/// )
> +/// ```
> +/// where `Self` is the drm::drv::Driver implementation these ioctls are being declared within.
> +///
> +/// # Examples
> +///
> +/// ```
> +/// kernel::declare_drm_ioctls! {
> +///     (FOO_GET_PARAM, drm_foo_get_param, ioctl::RENDER_ALLOW, my_get_param_handler),
> +/// }

I am wondering.. couldn't we make it a proc_macro and just tag all the
functions instead? Though I also see the point of having a central
list of all ioctls... Maybe we should have some higher level
discussions around on _how_ we want things to look like.

> +/// ```
> +///
> +#[macro_export]
> +macro_rules! declare_drm_ioctls {
> +    ( $(($cmd:ident, $struct:ident, $flags:expr, $func:expr)),* $(,)? ) => {
> +        const IOCTLS: &'static [$crate::drm::ioctl::DrmIoctlDescriptor] = {
> +            const _:() = {
> +                let i: u32 = $crate::bindings::DRM_COMMAND_BASE;
> +                // Assert that all the IOCTLs are in the right order and there are no gaps,
> +                // and that the sizeof of the specified type is correct.
> +                $(
> +                    let cmd: u32 = $crate::macros::concat_idents!($crate::bindings::DRM_IOCTL_, $cmd);
> +                    ::core::assert!(i == $crate::ioctl::_IOC_NR(cmd));
> +                    ::core::assert!(core::mem::size_of::<$crate::bindings::$struct>() == $crate::ioctl::_IOC_SIZE(cmd));

::core::mem::size_of

> +                    let i: u32 = i + 1;
> +                )*
> +            };
> +
> +            let ioctls = &[$(
> +                $crate::bindings::drm_ioctl_desc {
> +                    cmd: $crate::macros::concat_idents!($crate::bindings::DRM_IOCTL_, $cmd) as u32,
> +                    func: {
> +                        #[allow(non_snake_case)]
> +                        unsafe extern "C" fn $cmd(
> +                                raw_dev: *mut $crate::bindings::drm_device,
> +                                raw_data: *mut ::core::ffi::c_void,
> +                                raw_file_priv: *mut $crate::bindings::drm_file,
> +                        ) -> core::ffi::c_int {

::core

> +                            // SAFETY: We never drop this, and the DRM core ensures the device lives
> +                            // while callbacks are being called.
> +                            //
> +                            // FIXME: Currently there is nothing enforcing that the types of the
> +                            // dev/file match the current driver these ioctls are being declared
> +                            // for, and it's not clear how to enforce this within the type system.
> +                            let dev = ::core::mem::ManuallyDrop::new(unsafe {
> +                                $crate::drm::device::Device::from_raw(raw_dev)
> +                            });
> +                            // SAFETY: This is just the ioctl argument, which hopefully has the right type
> +                            // (we've done our best checking the size).
> +                            let data = unsafe { &mut *(raw_data as *mut $crate::bindings::$struct) };
> +                            // SAFETY: This is just the DRM file structure
> +                            let file = unsafe { $crate::drm::file::File::from_raw(raw_file_priv) };
> +
> +                            match $func(&*dev, data, &file) {
> +                                Err(e) => e.to_kernel_errno(),
> +                                Ok(i) => i.try_into().unwrap_or(ERANGE.to_kernel_errno()),

need to specify the namespace on ERANGE, no?

> +                            }
> +                        }
> +                        Some($cmd)
> +                    },
> +                    flags: $flags,
> +                    name: $crate::c_str!(::core::stringify!($cmd)).as_char_ptr(),
> +                }
> +            ),*];
> +            ioctls
> +        };
> +    };
> +}
> diff --git a/rust/kernel/drm/mod.rs b/rust/kernel/drm/mod.rs
> new file mode 100644
> index 000000000000..9ec6d7cbcaf3
> --- /dev/null
> +++ b/rust/kernel/drm/mod.rs
> @@ -0,0 +1,5 @@
> +// SPDX-License-Identifier: GPL-2.0 OR MIT
> +
> +//! DRM subsystem abstractions.
> +
> +pub mod ioctl;
> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index 7903490816bf..cb23d24c6718 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -37,6 +37,8 @@ mod build_assert;
>  pub mod delay;
>  pub mod device;
>  pub mod driver;
> +#[cfg(CONFIG_RUST_DRM)]
> +pub mod drm;
>  pub mod error;
>  pub mod io_buffer;
>  pub mod io_mem;
>
> --
> 2.35.1
>
  

On Tue, Mar 7, 2023 at 3:48 PM Karol Herbst <kherbst@redhat.com> wrote:
>
> On Tue, Mar 7, 2023 at 3:27 PM Asahi Lina <lina@asahilina.net> wrote:
> >
> > DRM drivers need to be able to declare which driver-specific ioctls they
> > support. This abstraction adds the required types and a helper macro to
> > generate the ioctl definition inside the DRM driver.
> >
> > Note that this macro is not usable until further bits of the
> > abstraction are in place (but it will not fail to compile on its own, if
> > not called).
> >
> > Signed-off-by: Asahi Lina <lina@asahilina.net>
> > ---
> >  drivers/gpu/drm/Kconfig         |   7 ++
> >  rust/bindings/bindings_helper.h |   2 +
> >  rust/kernel/drm/ioctl.rs        | 147 ++++++++++++++++++++++++++++++++++++++++
> >  rust/kernel/drm/mod.rs          |   5 ++
> >  rust/kernel/lib.rs              |   2 +
> >  5 files changed, 163 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> > index dc0f94f02a82..dab8f0f9aa96 100644
> > --- a/drivers/gpu/drm/Kconfig
> > +++ b/drivers/gpu/drm/Kconfig
> > @@ -27,6 +27,13 @@ menuconfig DRM
> >           details.  You should also select and configure AGP
> >           (/dev/agpgart) support if it is available for your platform.
> >
> > +# Rust abstractions cannot be built as modules currently, so force them as
> > +# bool by using these intermediate symbols. In the future these could be
> > +# tristate once abstractions themselves can be built as modules.
> > +config RUST_DRM
> > +       bool "Rust support for the DRM subsystem"
> > +       depends on DRM=y
> > +
> >  config DRM_MIPI_DBI
> >         tristate
> >         depends on DRM
> > diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> > index 91bb7906ca5a..2687bef1676f 100644
> > --- a/rust/bindings/bindings_helper.h
> > +++ b/rust/bindings/bindings_helper.h
> > @@ -6,6 +6,7 @@
> >   * Sorted alphabetically.
> >   */
> >
> > +#include <drm/drm_ioctl.h>
> >  #include <linux/delay.h>
> >  #include <linux/device.h>
> >  #include <linux/dma-mapping.h>
> > @@ -23,6 +24,7 @@
> >  #include <linux/sysctl.h>
> >  #include <linux/timekeeping.h>
> >  #include <linux/xarray.h>
> > +#include <uapi/drm/drm.h>
> >
>
> might make more sense to add this chunk to the patch actually needing it
>

ehh, ignore this comment please :)

> >  /* `bindgen` gets confused at certain things. */
> >  const gfp_t BINDINGS_GFP_KERNEL = GFP_KERNEL;
> > diff --git a/rust/kernel/drm/ioctl.rs b/rust/kernel/drm/ioctl.rs
> > new file mode 100644
> > index 000000000000..10304efbd5f1
> > --- /dev/null
> > +++ b/rust/kernel/drm/ioctl.rs
> > @@ -0,0 +1,147 @@
> > +// SPDX-License-Identifier: GPL-2.0 OR MIT
> > +#![allow(non_snake_case)]
> > +
> > +//! DRM IOCTL definitions.
> > +//!
> > +//! C header: [`include/linux/drm/drm_ioctl.h`](../../../../include/linux/drm/drm_ioctl.h)
> > +
> > +use crate::ioctl;
> > +
> > +const BASE: u32 = bindings::DRM_IOCTL_BASE as u32;
> > +
> > +/// Construct a DRM ioctl number with no argument.
> > +pub const fn IO(nr: u32) -> u32 {
> > +    ioctl::_IO(BASE, nr)
> > +}
> > +
> > +/// Construct a DRM ioctl number with a read-only argument.
> > +pub const fn IOR<T>(nr: u32) -> u32 {
> > +    ioctl::_IOR::<T>(BASE, nr)
> > +}
> > +
> > +/// Construct a DRM ioctl number with a write-only argument.
> > +pub const fn IOW<T>(nr: u32) -> u32 {
> > +    ioctl::_IOW::<T>(BASE, nr)
> > +}
> > +
> > +/// Construct a DRM ioctl number with a read-write argument.
> > +pub const fn IOWR<T>(nr: u32) -> u32 {
> > +    ioctl::_IOWR::<T>(BASE, nr)
> > +}
> > +
> > +/// Descriptor type for DRM ioctls. Use the `declare_drm_ioctls!{}` macro to construct them.
> > +pub type DrmIoctlDescriptor = bindings::drm_ioctl_desc;
> > +
> > +/// This is for ioctl which are used for rendering, and require that the file descriptor is either
> > +/// for a render node, or if it’s a legacy/primary node, then it must be authenticated.
> > +pub const AUTH: u32 = bindings::drm_ioctl_flags_DRM_AUTH;
> > +
> > +/// This must be set for any ioctl which can change the modeset or display state. Userspace must
> > +/// call the ioctl through a primary node, while it is the active master.
> > +///
> > +/// Note that read-only modeset ioctl can also be called by unauthenticated clients, or when a
> > +/// master is not the currently active one.
> > +pub const MASTER: u32 = bindings::drm_ioctl_flags_DRM_MASTER;
> > +
> > +/// Anything that could potentially wreak a master file descriptor needs to have this flag set.
> > +///
> > +/// Current that’s only for the SETMASTER and DROPMASTER ioctl, which e.g. logind can call to force
> > +/// a non-behaving master (display compositor) into compliance.
> > +///
> > +/// This is equivalent to callers with the SYSADMIN capability.
> > +pub const ROOT_ONLY: u32 = bindings::drm_ioctl_flags_DRM_ROOT_ONLY;
> > +
> > +/// Whether drm_ioctl_desc.func should be called with the DRM BKL held or not. Enforced as the
> > +/// default for all modern drivers, hence there should never be a need to set this flag.
> > +///
> > +/// Do not use anywhere else than for the VBLANK_WAIT IOCTL, which is the only legacy IOCTL which
> > +/// needs this.
> > +pub const UNLOCKED: u32 = bindings::drm_ioctl_flags_DRM_UNLOCKED;
> > +
> > +/// This is used for all ioctl needed for rendering only, for drivers which support render nodes.
> > +/// This should be all new render drivers, and hence it should be always set for any ioctl with
> > +/// `AUTH` set. Note though that read-only query ioctl might have this set, but have not set
> > +/// DRM_AUTH because they do not require authentication.
> > +pub const RENDER_ALLOW: u32 = bindings::drm_ioctl_flags_DRM_RENDER_ALLOW;
> > +
> > +/// Declare the DRM ioctls for a driver.
> > +///
> > +/// Each entry in the list should have the form:
> > +///
> > +/// `(ioctl_number, argument_type, flags, user_callback),`
> > +///
> > +/// `argument_type` is the type name within the `bindings` crate.
> > +/// `user_callback` should have the following prototype:
> > +///
> > +/// ```
> > +/// fn foo(device: &kernel::drm::device::Device<Self>,
> > +///        data: &mut bindings::argument_type,
> > +///        file: &kernel::drm::file::File<Self::File>,
> > +/// )
> > +/// ```
> > +/// where `Self` is the drm::drv::Driver implementation these ioctls are being declared within.
> > +///
> > +/// # Examples
> > +///
> > +/// ```
> > +/// kernel::declare_drm_ioctls! {
> > +///     (FOO_GET_PARAM, drm_foo_get_param, ioctl::RENDER_ALLOW, my_get_param_handler),
> > +/// }
>
> I am wondering.. couldn't we make it a proc_macro and just tag all the
> functions instead? Though I also see the point of having a central
> list of all ioctls... Maybe we should have some higher level
> discussions around on _how_ we want things to look like.
>
> > +/// ```
> > +///
> > +#[macro_export]
> > +macro_rules! declare_drm_ioctls {
> > +    ( $(($cmd:ident, $struct:ident, $flags:expr, $func:expr)),* $(,)? ) => {
> > +        const IOCTLS: &'static [$crate::drm::ioctl::DrmIoctlDescriptor] = {
> > +            const _:() = {
> > +                let i: u32 = $crate::bindings::DRM_COMMAND_BASE;
> > +                // Assert that all the IOCTLs are in the right order and there are no gaps,
> > +                // and that the sizeof of the specified type is correct.
> > +                $(
> > +                    let cmd: u32 = $crate::macros::concat_idents!($crate::bindings::DRM_IOCTL_, $cmd);
> > +                    ::core::assert!(i == $crate::ioctl::_IOC_NR(cmd));
> > +                    ::core::assert!(core::mem::size_of::<$crate::bindings::$struct>() == $crate::ioctl::_IOC_SIZE(cmd));
>
> ::core::mem::size_of
>
> > +                    let i: u32 = i + 1;
> > +                )*
> > +            };
> > +
> > +            let ioctls = &[$(
> > +                $crate::bindings::drm_ioctl_desc {
> > +                    cmd: $crate::macros::concat_idents!($crate::bindings::DRM_IOCTL_, $cmd) as u32,
> > +                    func: {
> > +                        #[allow(non_snake_case)]
> > +                        unsafe extern "C" fn $cmd(
> > +                                raw_dev: *mut $crate::bindings::drm_device,
> > +                                raw_data: *mut ::core::ffi::c_void,
> > +                                raw_file_priv: *mut $crate::bindings::drm_file,
> > +                        ) -> core::ffi::c_int {
>
> ::core
>
> > +                            // SAFETY: We never drop this, and the DRM core ensures the device lives
> > +                            // while callbacks are being called.
> > +                            //
> > +                            // FIXME: Currently there is nothing enforcing that the types of the
> > +                            // dev/file match the current driver these ioctls are being declared
> > +                            // for, and it's not clear how to enforce this within the type system.
> > +                            let dev = ::core::mem::ManuallyDrop::new(unsafe {
> > +                                $crate::drm::device::Device::from_raw(raw_dev)
> > +                            });
> > +                            // SAFETY: This is just the ioctl argument, which hopefully has the right type
> > +                            // (we've done our best checking the size).
> > +                            let data = unsafe { &mut *(raw_data as *mut $crate::bindings::$struct) };
> > +                            // SAFETY: This is just the DRM file structure
> > +                            let file = unsafe { $crate::drm::file::File::from_raw(raw_file_priv) };
> > +
> > +                            match $func(&*dev, data, &file) {
> > +                                Err(e) => e.to_kernel_errno(),
> > +                                Ok(i) => i.try_into().unwrap_or(ERANGE.to_kernel_errno()),
>
> need to specify the namespace on ERANGE, no?
>
> > +                            }
> > +                        }
> > +                        Some($cmd)
> > +                    },
> > +                    flags: $flags,
> > +                    name: $crate::c_str!(::core::stringify!($cmd)).as_char_ptr(),
> > +                }
> > +            ),*];
> > +            ioctls
> > +        };
> > +    };
> > +}
> > diff --git a/rust/kernel/drm/mod.rs b/rust/kernel/drm/mod.rs
> > new file mode 100644
> > index 000000000000..9ec6d7cbcaf3
> > --- /dev/null
> > +++ b/rust/kernel/drm/mod.rs
> > @@ -0,0 +1,5 @@
> > +// SPDX-License-Identifier: GPL-2.0 OR MIT
> > +
> > +//! DRM subsystem abstractions.
> > +
> > +pub mod ioctl;
> > diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> > index 7903490816bf..cb23d24c6718 100644
> > --- a/rust/kernel/lib.rs
> > +++ b/rust/kernel/lib.rs
> > @@ -37,6 +37,8 @@ mod build_assert;
> >  pub mod delay;
> >  pub mod device;
> >  pub mod driver;
> > +#[cfg(CONFIG_RUST_DRM)]
> > +pub mod drm;
> >  pub mod error;
> >  pub mod io_buffer;
> >  pub mod io_mem;
> >
> > --
> > 2.35.1
> >
  

On 3/7/23 11:25, Asahi Lina wrote:
> DRM drivers need to be able to declare which driver-specific ioctls they
> support. This abstraction adds the required types and a helper macro to
> generate the ioctl definition inside the DRM driver.
> 
> Note that this macro is not usable until further bits of the
> abstraction are in place (but it will not fail to compile on its own, if
> not called).
> 
> Signed-off-by: Asahi Lina <lina@asahilina.net>
> ---
>   drivers/gpu/drm/Kconfig         |   7 ++
>   rust/bindings/bindings_helper.h |   2 +
>   rust/kernel/drm/ioctl.rs        | 147 ++++++++++++++++++++++++++++++++++++++++
>   rust/kernel/drm/mod.rs          |   5 ++
>   rust/kernel/lib.rs              |   2 +
>   5 files changed, 163 insertions(+)
> 
> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> index dc0f94f02a82..dab8f0f9aa96 100644
> --- a/drivers/gpu/drm/Kconfig
> +++ b/drivers/gpu/drm/Kconfig
> @@ -27,6 +27,13 @@ menuconfig DRM
>   	  details.  You should also select and configure AGP
>   	  (/dev/agpgart) support if it is available for your platform.
>   

[...]

> +
> +/// Declare the DRM ioctls for a driver.
> +///
> +/// Each entry in the list should have the form:
> +///
> +/// `(ioctl_number, argument_type, flags, user_callback),`
> +///
> +/// `argument_type` is the type name within the `bindings` crate.
> +/// `user_callback` should have the following prototype:
> +///
> +/// ```
> +/// fn foo(device: &kernel::drm::device::Device<Self>,
> +///        data: &mut bindings::argument_type,
> +///        file: &kernel::drm::file::File<Self::File>,
> +/// )
> +/// ```
> +/// where `Self` is the drm::drv::Driver implementation these ioctls are being declared within.
> +///
> +/// # Examples
> +///
> +/// ```
> +/// kernel::declare_drm_ioctls! {
> +///     (FOO_GET_PARAM, drm_foo_get_param, ioctl::RENDER_ALLOW, my_get_param_handler),
> +/// }
> +/// ```
> +///
> +#[macro_export]
> +macro_rules! declare_drm_ioctls {
> +    ( $(($cmd:ident, $struct:ident, $flags:expr, $func:expr)),* $(,)? ) => {
> +        const IOCTLS: &'static [$crate::drm::ioctl::DrmIoctlDescriptor] = {
> +            const _:() = {
> +                let i: u32 = $crate::bindings::DRM_COMMAND_BASE;
> +                // Assert that all the IOCTLs are in the right order and there are no gaps,
> +                // and that the sizeof of the specified type is correct.

I believe that not necessarily the IOCTLs need to be in the right order and
with no gaps. For example, armada_drm.h has a gap in between 0x00 and
0x02 and exynos_drm.h also have gaps. Moreover, some drivers, like vgem and
virtgpu, start their IOCTLs with 0x01.

Best Regards,
- Maíra Canal

> +                $(
> +                    let cmd: u32 = $crate::macros::concat_idents!($crate::bindings::DRM_IOCTL_, $cmd);
> +                    ::core::assert!(i == $crate::ioctl::_IOC_NR(cmd));
> +                    ::core::assert!(core::mem::size_of::<$crate::bindings::$struct>() == $crate::ioctl::_IOC_SIZE(cmd));
> +                    let i: u32 = i + 1;
> +                )*
> +            };
> +
> +            let ioctls = &[$(
> +                $crate::bindings::drm_ioctl_desc {
> +                    cmd: $crate::macros::concat_idents!($crate::bindings::DRM_IOCTL_, $cmd) as u32,
> +                    func: {
> +                        #[allow(non_snake_case)]
> +                        unsafe extern "C" fn $cmd(
> +                                raw_dev: *mut $crate::bindings::drm_device,
> +                                raw_data: *mut ::core::ffi::c_void,
> +                                raw_file_priv: *mut $crate::bindings::drm_file,
> +                        ) -> core::ffi::c_int {
> +                            // SAFETY: We never drop this, and the DRM core ensures the device lives
> +                            // while callbacks are being called.
> +                            //
> +                            // FIXME: Currently there is nothing enforcing that the types of the
> +                            // dev/file match the current driver these ioctls are being declared
> +                            // for, and it's not clear how to enforce this within the type system.
> +                            let dev = ::core::mem::ManuallyDrop::new(unsafe {
> +                                $crate::drm::device::Device::from_raw(raw_dev)
> +                            });
> +                            // SAFETY: This is just the ioctl argument, which hopefully has the right type
> +                            // (we've done our best checking the size).
> +                            let data = unsafe { &mut *(raw_data as *mut $crate::bindings::$struct) };
> +                            // SAFETY: This is just the DRM file structure
> +                            let file = unsafe { $crate::drm::file::File::from_raw(raw_file_priv) };
> +
> +                            match $func(&*dev, data, &file) {
> +                                Err(e) => e.to_kernel_errno(),
> +                                Ok(i) => i.try_into().unwrap_or(ERANGE.to_kernel_errno()),
> +                            }
> +                        }
> +                        Some($cmd)
> +                    },
> +                    flags: $flags,
> +                    name: $crate::c_str!(::core::stringify!($cmd)).as_char_ptr(),
> +                }
> +            ),*];
> +            ioctls
> +        };
> +    };
> +}
> diff --git a/rust/kernel/drm/mod.rs b/rust/kernel/drm/mod.rs
> new file mode 100644
> index 000000000000..9ec6d7cbcaf3
> --- /dev/null
> +++ b/rust/kernel/drm/mod.rs
> @@ -0,0 +1,5 @@
> +// SPDX-License-Identifier: GPL-2.0 OR MIT
> +
> +//! DRM subsystem abstractions.
> +
> +pub mod ioctl;
> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index 7903490816bf..cb23d24c6718 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -37,6 +37,8 @@ mod build_assert;
>   pub mod delay;
>   pub mod device;
>   pub mod driver;
> +#[cfg(CONFIG_RUST_DRM)]
> +pub mod drm;
>   pub mod error;
>   pub mod io_buffer;
>   pub mod io_mem;
>
  

------- Original Message -------
On Tuesday, March 7th, 2023 at 15:25, Asahi Lina <lina@asahilina.net> wrote:

> DRM drivers need to be able to declare which driver-specific ioctls they
> support. This abstraction adds the required types and a helper macro to
> generate the ioctl definition inside the DRM driver.
> 
> Note that this macro is not usable until further bits of the
> abstraction are in place (but it will not fail to compile on its own, if
> not called).
> 
> Signed-off-by: Asahi Lina lina@asahilina.net
> 
> ---
>  drivers/gpu/drm/Kconfig         |   7 ++
>  rust/bindings/bindings_helper.h |   2 +
>  rust/kernel/drm/ioctl.rs        | 147 ++++++++++++++++++++++++++++++++++++++++
>  rust/kernel/drm/mod.rs          |   5 ++
>  rust/kernel/lib.rs              |   2 +
>  5 files changed, 163 insertions(+)
> 
> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> index dc0f94f02a82..dab8f0f9aa96 100644
> --- a/drivers/gpu/drm/Kconfig
> +++ b/drivers/gpu/drm/Kconfig
> @@ -27,6 +27,13 @@ menuconfig DRM
>  	  details.  You should also select and configure AGP
>  	  (/dev/agpgart) support if it is available for your platform.
> 
> +# Rust abstractions cannot be built as modules currently, so force them as
> +# bool by using these intermediate symbols. In the future these could be
> +# tristate once abstractions themselves can be built as modules.
> +config RUST_DRM
> +	bool "Rust support for the DRM subsystem"
> +	depends on DRM=y
> +
>  config DRM_MIPI_DBI
>  	tristate
>  	depends on DRM
> diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> index 91bb7906ca5a..2687bef1676f 100644
> --- a/rust/bindings/bindings_helper.h
> +++ b/rust/bindings/bindings_helper.h
> @@ -6,6 +6,7 @@
>   * Sorted alphabetically.
>   */
> 
> +#include <drm/drm_ioctl.h>
>  #include <linux/delay.h>
>  #include <linux/device.h>
>  #include <linux/dma-mapping.h>
> @@ -23,6 +24,7 @@
>  #include <linux/sysctl.h>
>  #include <linux/timekeeping.h>
>  #include <linux/xarray.h>
> +#include <uapi/drm/drm.h>
> 
>  /* `bindgen` gets confused at certain things. */
>  const gfp_t BINDINGS_GFP_KERNEL = GFP_KERNEL;
> diff --git a/rust/kernel/drm/ioctl.rs b/rust/kernel/drm/ioctl.rs
> new file mode 100644
> index 000000000000..10304efbd5f1
> --- /dev/null
> +++ b/rust/kernel/drm/ioctl.rs
> @@ -0,0 +1,147 @@
> +// SPDX-License-Identifier: GPL-2.0 OR MIT
> +#![allow(non_snake_case)]
> +
> +//! DRM IOCTL definitions.
> +//!
> +//! C header: [`include/linux/drm/drm_ioctl.h`](../../../../include/linux/drm/drm_ioctl.h)
> +
> +use crate::ioctl;
> +
> +const BASE: u32 = bindings::DRM_IOCTL_BASE as u32;
> +
> +/// Construct a DRM ioctl number with no argument.
> +pub const fn IO(nr: u32) -> u32 {
> +    ioctl::_IO(BASE, nr)
> +}
> +
> +/// Construct a DRM ioctl number with a read-only argument.
> +pub const fn IOR<T>(nr: u32) -> u32 {
> +    ioctl::_IOR::<T>(BASE, nr)
> +}
> +
> +/// Construct a DRM ioctl number with a write-only argument.
> +pub const fn IOW<T>(nr: u32) -> u32 {
> +    ioctl::_IOW::<T>(BASE, nr)
> +}
> +
> +/// Construct a DRM ioctl number with a read-write argument.
> +pub const fn IOWR<T>(nr: u32) -> u32 {
> +    ioctl::_IOWR::<T>(BASE, nr)
> +}
> +
> +/// Descriptor type for DRM ioctls. Use the `declare_drm_ioctls!{}` macro to construct them.
> +pub type DrmIoctlDescriptor = bindings::drm_ioctl_desc;
> +
> +/// This is for ioctl which are used for rendering, and require that the file descriptor is either
> +/// for a render node, or if it’s a legacy/primary node, then it must be authenticated.
> +pub const AUTH: u32 = bindings::drm_ioctl_flags_DRM_AUTH;
> +
> +/// This must be set for any ioctl which can change the modeset or display state. Userspace must
> +/// call the ioctl through a primary node, while it is the active master.
> +///
> +/// Note that read-only modeset ioctl can also be called by unauthenticated clients, or when a
> +/// master is not the currently active one.
> +pub const MASTER: u32 = bindings::drm_ioctl_flags_DRM_MASTER;
> +
> +/// Anything that could potentially wreak a master file descriptor needs to have this flag set.
> +///
> +/// Current that’s only for the SETMASTER and DROPMASTER ioctl, which e.g. logind can call to force
> +/// a non-behaving master (display compositor) into compliance.
> +///
> +/// This is equivalent to callers with the SYSADMIN capability.
> +pub const ROOT_ONLY: u32 = bindings::drm_ioctl_flags_DRM_ROOT_ONLY;
> +
> +/// Whether drm_ioctl_desc.func should be called with the DRM BKL held or not. Enforced as the
> +/// default for all modern drivers, hence there should never be a need to set this flag.
> +///
> +/// Do not use anywhere else than for the VBLANK_WAIT IOCTL, which is the only legacy IOCTL which
> +/// needs this.
> +pub const UNLOCKED: u32 = bindings::drm_ioctl_flags_DRM_UNLOCKED;
> +
> +/// This is used for all ioctl needed for rendering only, for drivers which support render nodes.
> +/// This should be all new render drivers, and hence it should be always set for any ioctl with
> +/// `AUTH` set. Note though that read-only query ioctl might have this set, but have not set
> +/// DRM_AUTH because they do not require authentication.
> +pub const RENDER_ALLOW: u32 = bindings::drm_ioctl_flags_DRM_RENDER_ALLOW;
> +
> +/// Declare the DRM ioctls for a driver.
> +///
> +/// Each entry in the list should have the form:
> +///
> +/// `(ioctl_number, argument_type, flags, user_callback),`
> +///
> +/// `argument_type` is the type name within the `bindings` crate.
> +/// `user_callback` should have the following prototype:
> +///
> +/// ```
> +/// fn foo(device: &kernel::drm::device::Device<Self>,
> +///        data: &mut bindings::argument_type,
> +///        file: &kernel::drm::file::File<Self::File>,
> +/// )
> +/// ```
> +/// where `Self` is the drm::drv::Driver implementation these ioctls are being declared within.
> +///
> +/// # Examples
> +///
> +/// ```
> +/// kernel::declare_drm_ioctls! {
> +///     (FOO_GET_PARAM, drm_foo_get_param, ioctl::RENDER_ALLOW, my_get_param_handler),
> +/// }
> +/// ```
> +///
> +#[macro_export]
> +macro_rules! declare_drm_ioctls {
> +    ( $(($cmd:ident, $struct:ident, $flags:expr, $func:expr)),* $(,)? ) => {
> +        const IOCTLS: &'static [$crate::drm::ioctl::DrmIoctlDescriptor] = {
> +            const _:() = {
> +                let i: u32 = $crate::bindings::DRM_COMMAND_BASE;
> +                // Assert that all the IOCTLs are in the right order and there are no gaps,
> +                // and that the sizeof of the specified type is correct.
> +                $(
> +                    let cmd: u32 = $crate::macros::concat_idents!($crate::bindings::DRM_IOCTL_, $cmd);
> +                    ::core::assert!(i == $crate::ioctl::_IOC_NR(cmd));
> +                    ::core::assert!(core::mem::size_of::<$crate::bindings::$struct>() == $crate::ioctl::_IOC_SIZE(cmd));
> +                    let i: u32 = i + 1;
> +                )*
> +            };
> +
> +            let ioctls = &[$(
> +                $crate::bindings::drm_ioctl_desc {
> +                    cmd: $crate::macros::concat_idents!($crate::bindings::DRM_IOCTL_, $cmd) as u32,
> +                    func: {
> +                        #[allow(non_snake_case)]
> +                        unsafe extern "C" fn $cmd(
> +                                raw_dev: *mut $crate::bindings::drm_device,
> +                                raw_data: *mut ::core::ffi::c_void,
> +                                raw_file_priv: *mut $crate::bindings::drm_file,
> +                        ) -> core::ffi::c_int {
> +                            // SAFETY: We never drop this, and the DRM core ensures the device lives
> +                            // while callbacks are being called.
> +                            //
> +                            // FIXME: Currently there is nothing enforcing that the types of the
> +                            // dev/file match the current driver these ioctls are being declared
> +                            // for, and it's not clear how to enforce this within the type system.
> +                            let dev = ::core::mem::ManuallyDrop::new(unsafe {
> +                                $crate::drm::device::Device::from_raw(raw_dev)
> +                            });
> +                            // SAFETY: This is just the ioctl argument, which hopefully has the right type
> +                            // (we've done our best checking the size).

In the rust tree there is the ReadableFromBytes [1] trait which indicates that it is safe to read arbitrary bytes into the type. Maybe you could add it as bound on the argument type when it lands in rust-next? This way you can't end up with for example a struct containing a bool with the byte value 2, which is UB.

https://rust-for-linux.github.io/docs/kernel/io_buffer/trait.ReadableFromBytes.html [1]

> +                            let data = unsafe { &mut *(raw_data as *mut $crate::bindings::$struct) };
> +                            // SAFETY: This is just the DRM file structure
> +                            let file = unsafe { $crate::drm::file::File::from_raw(raw_file_priv) };
> +
> +                            match $func(&*dev, data, &file) {
> +                                Err(e) => e.to_kernel_errno(),
> +                                Ok(i) => i.try_into().unwrap_or(ERANGE.to_kernel_errno()),
> +                            }
> +                        }
> +                        Some($cmd)
> +                    },
> +                    flags: $flags,
> +                    name: $crate::c_str!(::core::stringify!($cmd)).as_char_ptr(),
> +                }
> +            ),*];
> +            ioctls
> +        };
> +    };
> +}
> diff --git a/rust/kernel/drm/mod.rs b/rust/kernel/drm/mod.rs
> new file mode 100644
> index 000000000000..9ec6d7cbcaf3
> --- /dev/null
> +++ b/rust/kernel/drm/mod.rs
> @@ -0,0 +1,5 @@
> +// SPDX-License-Identifier: GPL-2.0 OR MIT
> +
> +//! DRM subsystem abstractions.
> +
> +pub mod ioctl;
> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index 7903490816bf..cb23d24c6718 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -37,6 +37,8 @@ mod build_assert;
>  pub mod delay;
>  pub mod device;
>  pub mod driver;
> +#[cfg(CONFIG_RUST_DRM)]
> +pub mod drm;
>  pub mod error;
>  pub mod io_buffer;
>  pub mod io_mem;
> 
> --
> 2.35.1

Cheers,
Bjorn
  

On 08/03/2023 00.32, Maíra Canal wrote:
> On 3/7/23 11:25, Asahi Lina wrote:
>> DRM drivers need to be able to declare which driver-specific ioctls they
>> support. This abstraction adds the required types and a helper macro to
>> generate the ioctl definition inside the DRM driver.
>>
>> Note that this macro is not usable until further bits of the
>> abstraction are in place (but it will not fail to compile on its own, if
>> not called).
>>
>> Signed-off-by: Asahi Lina <lina@asahilina.net>
>> ---
>>   drivers/gpu/drm/Kconfig         |   7 ++
>>   rust/bindings/bindings_helper.h |   2 +
>>   rust/kernel/drm/ioctl.rs        | 147 ++++++++++++++++++++++++++++++++++++++++
>>   rust/kernel/drm/mod.rs          |   5 ++
>>   rust/kernel/lib.rs              |   2 +
>>   5 files changed, 163 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
>> index dc0f94f02a82..dab8f0f9aa96 100644
>> --- a/drivers/gpu/drm/Kconfig
>> +++ b/drivers/gpu/drm/Kconfig
>> @@ -27,6 +27,13 @@ menuconfig DRM
>>   	  details.  You should also select and configure AGP
>>   	  (/dev/agpgart) support if it is available for your platform.
>>   
> 
> [...]
> 
>> +
>> +/// Declare the DRM ioctls for a driver.
>> +///
>> +/// Each entry in the list should have the form:
>> +///
>> +/// `(ioctl_number, argument_type, flags, user_callback),`
>> +///
>> +/// `argument_type` is the type name within the `bindings` crate.
>> +/// `user_callback` should have the following prototype:
>> +///
>> +/// ```
>> +/// fn foo(device: &kernel::drm::device::Device<Self>,
>> +///        data: &mut bindings::argument_type,
>> +///        file: &kernel::drm::file::File<Self::File>,
>> +/// )
>> +/// ```
>> +/// where `Self` is the drm::drv::Driver implementation these ioctls are being declared within.
>> +///
>> +/// # Examples
>> +///
>> +/// ```
>> +/// kernel::declare_drm_ioctls! {
>> +///     (FOO_GET_PARAM, drm_foo_get_param, ioctl::RENDER_ALLOW, my_get_param_handler),
>> +/// }
>> +/// ```
>> +///
>> +#[macro_export]
>> +macro_rules! declare_drm_ioctls {
>> +    ( $(($cmd:ident, $struct:ident, $flags:expr, $func:expr)),* $(,)? ) => {
>> +        const IOCTLS: &'static [$crate::drm::ioctl::DrmIoctlDescriptor] = {
>> +            const _:() = {
>> +                let i: u32 = $crate::bindings::DRM_COMMAND_BASE;
>> +                // Assert that all the IOCTLs are in the right order and there are no gaps,
>> +                // and that the sizeof of the specified type is correct.
> 
> I believe that not necessarily the IOCTLs need to be in the right order and
> with no gaps. For example, armada_drm.h has a gap in between 0x00 and
> 0x02 and exynos_drm.h also have gaps. Moreover, some drivers, like vgem and
> virtgpu, start their IOCTLs with 0x01.

Yeah, we talked about this a bit... do you have any ideas about how to
design this? I think it should be possible with a const function
initializing an array entry by entry, we just need a two-pass macro
(once to determine the max ioctl number, then again to actually output
the implementation).

I'm not sure why drivers would have gaps in the ioctl numbers though...
my idea was that new drivers shouldn't need that as far as I can tell
(you can't remove APIs after the fact due to UAPI stability guarantees,
so as long as you don't have gaps to begin with...). But I guess if
we're reimplementing existing drivers in Rust we'll need this... though
maybe it makes sense to just say it's not supported and require
reimplementations that have holes to just explicitly add dummy ioctls
that return EINVAL? We could even provide such a dummy generic ioctl
handler on the abstraction side, so drivers just have to add it to the
list, or make the macro take a special token that is used for
placeholder ioctls that don't exist (which then creates the NULL
function pointer that the drm core interprets as invalid)...

Basically I'm not sure if it makes sense to fully support noncontiguous
ioctl numbers automagically, or just say drivers need to explicitly list
gaps. I'd love to hear the opinion of other DRM folks about this!

~~ Lina
  

On 08/03/2023 02.34, Björn Roy Baron wrote:
>> +                            // SAFETY: This is just the ioctl argument, which hopefully has the right type
>> +                            // (we've done our best checking the size).
> 
> In the rust tree there is the ReadableFromBytes [1] trait which indicates that it is safe to read arbitrary bytes into the type. Maybe you could add it as bound on the argument type when it lands in rust-next? This way you can't end up with for example a struct containing a bool with the byte value 2, which is UB.

There's actually a much bigger story here, because that trait isn't
really very useful without a way to auto-derive it. I need the same kind
of guarantee for all the GPU firmware structs...

There's one using only declarative macros [1] and one using proc macros
[2]. And then, since ioctl arguments are declared in C UAPI header
files, we need a way to be able to derive those traits for them... which
I guess means bindgen changes?

For now though, I don't think this is something we need to worry about
too much for this particular use case because the macro forces all
struct types to be part of `bindings`, and any driver UAPI should
already follow these constraints if it is well-formed (and UAPIs are
going to already attract a lot of scrutiny anyway). Technically you
could try taking a random kernel struct containing a `bool` in an ioctl
list, but that would stand out as nonsense just as much as trying to
unsafe impl ReadableFromBytes for it so... it's kind of an academic
problem ^^

Actually, I think we talked of moving UAPI types to a separate crate (so
drivers can get access to those types and only those types, not the main
bindings crate). Then maybe we could just say that if the macro forces
the type to be from that crate, it's inherently safe since all UAPIs
should already be castable to/from bytes if properly designed.

Aside: I'm not sure the ReadableFromBytes/WritableToBytes distinction is
very useful. I know it exists (padding bytes, uninit fields, and
technically bool should be WritableToBytes but not ReadableFromBytes),
but I can't think of a good use case for it... I think I'd rather start
with a single trait and just always enforce the union of the rules,
because pretty much any time you're casting to/from bytes you want
well-defined "bag of bytes" struct layouts anyway. ioctls can be R/W/RW
so having separate traits depending on ioctl type complicates the code...

[1]
https://github.com/QubesOS/qubes-gui-rust/blob/940754bfefb7325548eece658c307a0c41c9bc7c/qubes-castable/src/lib.rs
[2] https://docs.rs/pkbuffer/latest/pkbuffer/derive.Castable.html

> 
> https://rust-for-linux.github.io/docs/kernel/io_buffer/trait.ReadableFromBytes.html [1]
> 

~~ Lina
  

On Thu, 9 Mar 2023 at 15:32, Asahi Lina <lina@asahilina.net> wrote:
>
> On 08/03/2023 00.32, Maíra Canal wrote:
> > On 3/7/23 11:25, Asahi Lina wrote:
> >> DRM drivers need to be able to declare which driver-specific ioctls they
> >> support. This abstraction adds the required types and a helper macro to
> >> generate the ioctl definition inside the DRM driver.
> >>
> >> Note that this macro is not usable until further bits of the
> >> abstraction are in place (but it will not fail to compile on its own, if
> >> not called).
> >>
> >> Signed-off-by: Asahi Lina <lina@asahilina.net>
> >> ---
> >>   drivers/gpu/drm/Kconfig         |   7 ++
> >>   rust/bindings/bindings_helper.h |   2 +
> >>   rust/kernel/drm/ioctl.rs        | 147 ++++++++++++++++++++++++++++++++++++++++
> >>   rust/kernel/drm/mod.rs          |   5 ++
> >>   rust/kernel/lib.rs              |   2 +
> >>   5 files changed, 163 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> >> index dc0f94f02a82..dab8f0f9aa96 100644
> >> --- a/drivers/gpu/drm/Kconfig
> >> +++ b/drivers/gpu/drm/Kconfig
> >> @@ -27,6 +27,13 @@ menuconfig DRM
> >>        details.  You should also select and configure AGP
> >>        (/dev/agpgart) support if it is available for your platform.
> >>
> >
> > [...]
> >
> >> +
> >> +/// Declare the DRM ioctls for a driver.
> >> +///
> >> +/// Each entry in the list should have the form:
> >> +///
> >> +/// `(ioctl_number, argument_type, flags, user_callback),`
> >> +///
> >> +/// `argument_type` is the type name within the `bindings` crate.
> >> +/// `user_callback` should have the following prototype:
> >> +///
> >> +/// ```
> >> +/// fn foo(device: &kernel::drm::device::Device<Self>,
> >> +///        data: &mut bindings::argument_type,
> >> +///        file: &kernel::drm::file::File<Self::File>,
> >> +/// )
> >> +/// ```
> >> +/// where `Self` is the drm::drv::Driver implementation these ioctls are being declared within.
> >> +///
> >> +/// # Examples
> >> +///
> >> +/// ```
> >> +/// kernel::declare_drm_ioctls! {
> >> +///     (FOO_GET_PARAM, drm_foo_get_param, ioctl::RENDER_ALLOW, my_get_param_handler),
> >> +/// }
> >> +/// ```
> >> +///
> >> +#[macro_export]
> >> +macro_rules! declare_drm_ioctls {
> >> +    ( $(($cmd:ident, $struct:ident, $flags:expr, $func:expr)),* $(,)? ) => {
> >> +        const IOCTLS: &'static [$crate::drm::ioctl::DrmIoctlDescriptor] = {
> >> +            const _:() = {
> >> +                let i: u32 = $crate::bindings::DRM_COMMAND_BASE;
> >> +                // Assert that all the IOCTLs are in the right order and there are no gaps,
> >> +                // and that the sizeof of the specified type is correct.
> >
> > I believe that not necessarily the IOCTLs need to be in the right order and
> > with no gaps. For example, armada_drm.h has a gap in between 0x00 and
> > 0x02 and exynos_drm.h also have gaps. Moreover, some drivers, like vgem and
> > virtgpu, start their IOCTLs with 0x01.
>
> Yeah, we talked about this a bit... do you have any ideas about how to
> design this? I think it should be possible with a const function
> initializing an array entry by entry, we just need a two-pass macro
> (once to determine the max ioctl number, then again to actually output
> the implementation).
>
> I'm not sure why drivers would have gaps in the ioctl numbers though...
> my idea was that new drivers shouldn't need that as far as I can tell
> (you can't remove APIs after the fact due to UAPI stability guarantees,
> so as long as you don't have gaps to begin with...). But I guess if
> we're reimplementing existing drivers in Rust we'll need this... though
> maybe it makes sense to just say it's not supported and require
> reimplementations that have holes to just explicitly add dummy ioctls
> that return EINVAL? We could even provide such a dummy generic ioctl
> handler on the abstraction side, so drivers just have to add it to the
> list, or make the macro take a special token that is used for
> placeholder ioctls that don't exist (which then creates the NULL
> function pointer that the drm core interprets as invalid)...

I can think of two reason for gaps having appeared:

a) developers wanted to group new uapis at a nice base number.
This is never essential it's just makes things easier to read, and
allows slotting other ioctls into the gaps later.

b) parallel feature development ends up conflicting then one thread never lands.
I've got two-three devs each adding a uAPI, we assign them 0x10, 0x11,
0x12 while they work, then 0x11 never lands because it was a bad idea.

However I think you should be fine enforcing a non-sparse space here
unless we want to handle replacing current drivers, as long as it's
hard to screw up so you know early.

Dave.
  

On 3/9/23 03:15, Dave Airlie wrote:
> On Thu, 9 Mar 2023 at 15:32, Asahi Lina <lina@asahilina.net> wrote:
>>
>> On 08/03/2023 00.32, Maíra Canal wrote:
>>> On 3/7/23 11:25, Asahi Lina wrote:
>>>> DRM drivers need to be able to declare which driver-specific ioctls they
>>>> support. This abstraction adds the required types and a helper macro to
>>>> generate the ioctl definition inside the DRM driver.
>>>>
>>>> Note that this macro is not usable until further bits of the
>>>> abstraction are in place (but it will not fail to compile on its own, if
>>>> not called).
>>>>
>>>> Signed-off-by: Asahi Lina <lina@asahilina.net>
>>>> ---
>>>>    drivers/gpu/drm/Kconfig         |   7 ++
>>>>    rust/bindings/bindings_helper.h |   2 +
>>>>    rust/kernel/drm/ioctl.rs        | 147 ++++++++++++++++++++++++++++++++++++++++
>>>>    rust/kernel/drm/mod.rs          |   5 ++
>>>>    rust/kernel/lib.rs              |   2 +
>>>>    5 files changed, 163 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
>>>> index dc0f94f02a82..dab8f0f9aa96 100644
>>>> --- a/drivers/gpu/drm/Kconfig
>>>> +++ b/drivers/gpu/drm/Kconfig
>>>> @@ -27,6 +27,13 @@ menuconfig DRM
>>>>         details.  You should also select and configure AGP
>>>>         (/dev/agpgart) support if it is available for your platform.
>>>>
>>>
>>> [...]
>>>
>>>> +
>>>> +/// Declare the DRM ioctls for a driver.
>>>> +///
>>>> +/// Each entry in the list should have the form:
>>>> +///
>>>> +/// `(ioctl_number, argument_type, flags, user_callback),`
>>>> +///
>>>> +/// `argument_type` is the type name within the `bindings` crate.
>>>> +/// `user_callback` should have the following prototype:
>>>> +///
>>>> +/// ```
>>>> +/// fn foo(device: &kernel::drm::device::Device<Self>,
>>>> +///        data: &mut bindings::argument_type,
>>>> +///        file: &kernel::drm::file::File<Self::File>,
>>>> +/// )
>>>> +/// ```
>>>> +/// where `Self` is the drm::drv::Driver implementation these ioctls are being declared within.
>>>> +///
>>>> +/// # Examples
>>>> +///
>>>> +/// ```
>>>> +/// kernel::declare_drm_ioctls! {
>>>> +///     (FOO_GET_PARAM, drm_foo_get_param, ioctl::RENDER_ALLOW, my_get_param_handler),
>>>> +/// }
>>>> +/// ```
>>>> +///
>>>> +#[macro_export]
>>>> +macro_rules! declare_drm_ioctls {
>>>> +    ( $(($cmd:ident, $struct:ident, $flags:expr, $func:expr)),* $(,)? ) => {
>>>> +        const IOCTLS: &'static [$crate::drm::ioctl::DrmIoctlDescriptor] = {
>>>> +            const _:() = {
>>>> +                let i: u32 = $crate::bindings::DRM_COMMAND_BASE;
>>>> +                // Assert that all the IOCTLs are in the right order and there are no gaps,
>>>> +                // and that the sizeof of the specified type is correct.
>>>
>>> I believe that not necessarily the IOCTLs need to be in the right order and
>>> with no gaps. For example, armada_drm.h has a gap in between 0x00 and
>>> 0x02 and exynos_drm.h also have gaps. Moreover, some drivers, like vgem and
>>> virtgpu, start their IOCTLs with 0x01.
>>
>> Yeah, we talked about this a bit... do you have any ideas about how to
>> design this? I think it should be possible with a const function
>> initializing an array entry by entry, we just need a two-pass macro
>> (once to determine the max ioctl number, then again to actually output
>> the implementation).
>>
>> I'm not sure why drivers would have gaps in the ioctl numbers though...
>> my idea was that new drivers shouldn't need that as far as I can tell
>> (you can't remove APIs after the fact due to UAPI stability guarantees,
>> so as long as you don't have gaps to begin with...). But I guess if
>> we're reimplementing existing drivers in Rust we'll need this... though
>> maybe it makes sense to just say it's not supported and require
>> reimplementations that have holes to just explicitly add dummy ioctls
>> that return EINVAL? We could even provide such a dummy generic ioctl
>> handler on the abstraction side, so drivers just have to add it to the
>> list, or make the macro take a special token that is used for
>> placeholder ioctls that don't exist (which then creates the NULL
>> function pointer that the drm core interprets as invalid)...
> 
> I can think of two reason for gaps having appeared:
> 
> a) developers wanted to group new uapis at a nice base number.
> This is never essential it's just makes things easier to read, and
> allows slotting other ioctls into the gaps later.
> 
> b) parallel feature development ends up conflicting then one thread never lands.
> I've got two-three devs each adding a uAPI, we assign them 0x10, 0x11,
> 0x12 while they work, then 0x11 never lands because it was a bad idea.
> 
> However I think you should be fine enforcing a non-sparse space here
> unless we want to handle replacing current drivers, as long as it's
> hard to screw up so you know early.

I guess it would be nice to support old UAPIs for cases of reimplementations.
Currently, I'm working on a reimplementation of vgem and I ended up having to
create a dummy IOCTL to deal with the sparse number space. Although creating
dummy IOCTLs works, I don't believe it is a nice practice.

Moreover, I believe that if we keep developing new drivers with Rust, cases
(a) and (b) will end up happening, and maybe the Rust abstractions should
work like DRM and allow it to happen.

Best Regards,
- Maíra Canal

> 
> Dave.
  

On Thu, 2023-03-09 at 15:04 +0900, Asahi Lina wrote:
> On 08/03/2023 02.34, Björn Roy Baron wrote:
> > > +                            // SAFETY: This is just the ioctl
> > > argument, which hopefully has the right type
> > > +                            // (we've done our best checking the
> > > size).
> > 
> > In the rust tree there is the ReadableFromBytes [1] trait which
> > indicates that it is safe to read arbitrary bytes into the type.
> > Maybe you could add it as bound on the argument type when it lands
> > in rust-next? This way you can't end up with for example a struct
> > containing a bool with the byte value 2, which is UB.
> 
> There's actually a much bigger story here, because that trait isn't
> really very useful without a way to auto-derive it. I need the same
> kind
> of guarantee for all the GPU firmware structs...
> 
> There's one using only declarative macros [1] and one using proc
> macros
> [2]. And then, since ioctl arguments are declared in C UAPI header
> files, we need a way to be able to derive those traits for them...
> which
> I guess means bindgen changes?

It'd be cool to be able to auto-verify that uAPI structs are all
tightly packed and use the right subset of types.  Maybe not possible
this iteration but it'd be cool to see in future.  I'd like to see it
for C as well, ideally.

~Faith
  

On Thu, Mar 9, 2023 at 9:24 PM Faith Ekstrand
<faith.ekstrand@collabora.com> wrote:
>
> On Thu, 2023-03-09 at 15:04 +0900, Asahi Lina wrote:
> > On 08/03/2023 02.34, Björn Roy Baron wrote:
> > > > +                            // SAFETY: This is just the ioctl
> > > > argument, which hopefully has the right type
> > > > +                            // (we've done our best checking the
> > > > size).
> > >
> > > In the rust tree there is the ReadableFromBytes [1] trait which
> > > indicates that it is safe to read arbitrary bytes into the type.
> > > Maybe you could add it as bound on the argument type when it lands
> > > in rust-next? This way you can't end up with for example a struct
> > > containing a bool with the byte value 2, which is UB.
> >
> > There's actually a much bigger story here, because that trait isn't
> > really very useful without a way to auto-derive it. I need the same
> > kind
> > of guarantee for all the GPU firmware structs...
> >
> > There's one using only declarative macros [1] and one using proc
> > macros
> > [2]. And then, since ioctl arguments are declared in C UAPI header
> > files, we need a way to be able to derive those traits for them...
> > which
> > I guess means bindgen changes?
>
> It'd be cool to be able to auto-verify that uAPI structs are all
> tightly packed and use the right subset of types.  Maybe not possible
> this iteration but it'd be cool to see in future.  I'd like to see it
> for C as well, ideally.
>
> ~Faith
>

I'm sure that with a macro you could verify that a struct definition
doesn't contain any gaps, just not sure on how one would enforce that.
Could add a trait which can only be implemented through a proc_macro?
Maybe we can have a proc_macro ensuring no gaps? Would be cool tech to
have indeed.
  

On 10/03/2023 05.39, Karol Herbst wrote:
> On Thu, Mar 9, 2023 at 9:24 PM Faith Ekstrand
> <faith.ekstrand@collabora.com> wrote:
>>
>> On Thu, 2023-03-09 at 15:04 +0900, Asahi Lina wrote:
>>> On 08/03/2023 02.34, Björn Roy Baron wrote:
>>>>> +                            // SAFETY: This is just the ioctl
>>>>> argument, which hopefully has the right type
>>>>> +                            // (we've done our best checking the
>>>>> size).
>>>>
>>>> In the rust tree there is the ReadableFromBytes [1] trait which
>>>> indicates that it is safe to read arbitrary bytes into the type.
>>>> Maybe you could add it as bound on the argument type when it lands
>>>> in rust-next? This way you can't end up with for example a struct
>>>> containing a bool with the byte value 2, which is UB.
>>>
>>> There's actually a much bigger story here, because that trait isn't
>>> really very useful without a way to auto-derive it. I need the same
>>> kind
>>> of guarantee for all the GPU firmware structs...
>>>
>>> There's one using only declarative macros [1] and one using proc
>>> macros
>>> [2]. And then, since ioctl arguments are declared in C UAPI header
>>> files, we need a way to be able to derive those traits for them...
>>> which
>>> I guess means bindgen changes?
>>
>> It'd be cool to be able to auto-verify that uAPI structs are all
>> tightly packed and use the right subset of types.  Maybe not possible
>> this iteration but it'd be cool to see in future.  I'd like to see it
>> for C as well, ideally.
>>
>> ~Faith
>>
> 
> I'm sure that with a macro you could verify that a struct definition
> doesn't contain any gaps, just not sure on how one would enforce that.
> Could add a trait which can only be implemented through a proc_macro?
> Maybe we can have a proc_macro ensuring no gaps? Would be cool tech to
> have indeed.

You just make the trait unsafe, as usual, then implement it via that
macro. It's how the things I linked work ^^

The tricky thing with C UAPI definitions is just that we need to get
bindgen to emit those macro instantiations around struct definitions
somehow. Or maybe it could be done with a brute force text-based
postprocessing pass? If we put all UAPI defs into their own crate, you
could probably just do it with sed or a python script or something on
the bindgen output to add it for all struct types...

@Rust folks: Should I try creating a uapi crate for this? I think we can
just mirror the bindings crate logic, and we don't need helpers or
anything like that here, so it shouldn't be very difficult. Then I could
(eventually) eliminate all usage of the full bindings crate in the
driver, and also try experimenting with stuff like this to validate all
UAPI types and implement special traits for them...

~~ Lina
  

On Tue, Mar 07, 2023 at 11:25:26PM +0900, Asahi Lina wrote:
> DRM drivers need to be able to declare which driver-specific ioctls they
> support. This abstraction adds the required types and a helper macro to
> generate the ioctl definition inside the DRM driver.
> 
> Note that this macro is not usable until further bits of the
> abstraction are in place (but it will not fail to compile on its own, if
> not called).
> 
> Signed-off-by: Asahi Lina <lina@asahilina.net>

A bunch of thoughts/questions:

- You have the pub functions to create ioctl numbers, but it looks like
  most drivers just do this in the C uapi headers instead and then use the
  direct number from the bindings? I wonder whether we shouldn't just use
  that as standard way, since in the end we do need the C headers for
  userspace to use the ioctls/structs. Or could we generate the headers
  from rust?

- More type safety would be nice. You have the one for device, but not yet
  for DrmFile. At least if I read the examples in asahi/vgem right. Also
  the FIXME for how to make sure you generate the table for the right kind
  of driver would be nice to fix.

- Type safety against the size of the struct an ioctl number is great!

- I wonder whether we could adjust the type according to _IOR/W/RW, i.e.
  if you have W then your ioctl function is Result<Struct>, if not then
  Result<()> since it's just errno, and you get the paramater only when
  you have R set. We had in the past confusions where people got this
  wrong and wondered why their parameters don't make it to userspace.

- There's also the question of drm_ioctl() zero-extending the ioctl
  parameter struct both ways (i.e. kernel kernel or newer userspace). I
  think trying to encode that with Some() is overkill, but maybe worth a
  thought.

- It would be _really_ great if rust ioctl abstractions enforce
  https://dri.freedesktop.org/docs/drm/process/botching-up-ioctls.html at
  the type level, i.e. everything naturally aligned, no gaps, all that
  stuff. This would also hold for get/put_user and all these things (I
  didn't look into that stuff yet in the drivers when you pull in entire
  arrays).

Cheers, Daniel
> ---
>  drivers/gpu/drm/Kconfig         |   7 ++
>  rust/bindings/bindings_helper.h |   2 +
>  rust/kernel/drm/ioctl.rs        | 147 ++++++++++++++++++++++++++++++++++++++++
>  rust/kernel/drm/mod.rs          |   5 ++
>  rust/kernel/lib.rs              |   2 +
>  5 files changed, 163 insertions(+)
> 
> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> index dc0f94f02a82..dab8f0f9aa96 100644
> --- a/drivers/gpu/drm/Kconfig
> +++ b/drivers/gpu/drm/Kconfig
> @@ -27,6 +27,13 @@ menuconfig DRM
>  	  details.  You should also select and configure AGP
>  	  (/dev/agpgart) support if it is available for your platform.
>  
> +# Rust abstractions cannot be built as modules currently, so force them as
> +# bool by using these intermediate symbols. In the future these could be
> +# tristate once abstractions themselves can be built as modules.
> +config RUST_DRM
> +	bool "Rust support for the DRM subsystem"
> +	depends on DRM=y
> +
>  config DRM_MIPI_DBI
>  	tristate
>  	depends on DRM
> diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> index 91bb7906ca5a..2687bef1676f 100644
> --- a/rust/bindings/bindings_helper.h
> +++ b/rust/bindings/bindings_helper.h
> @@ -6,6 +6,7 @@
>   * Sorted alphabetically.
>   */
>  
> +#include <drm/drm_ioctl.h>
>  #include <linux/delay.h>
>  #include <linux/device.h>
>  #include <linux/dma-mapping.h>
> @@ -23,6 +24,7 @@
>  #include <linux/sysctl.h>
>  #include <linux/timekeeping.h>
>  #include <linux/xarray.h>
> +#include <uapi/drm/drm.h>
>  
>  /* `bindgen` gets confused at certain things. */
>  const gfp_t BINDINGS_GFP_KERNEL = GFP_KERNEL;
> diff --git a/rust/kernel/drm/ioctl.rs b/rust/kernel/drm/ioctl.rs
> new file mode 100644
> index 000000000000..10304efbd5f1
> --- /dev/null
> +++ b/rust/kernel/drm/ioctl.rs
> @@ -0,0 +1,147 @@
> +// SPDX-License-Identifier: GPL-2.0 OR MIT
> +#![allow(non_snake_case)]
> +
> +//! DRM IOCTL definitions.
> +//!
> +//! C header: [`include/linux/drm/drm_ioctl.h`](../../../../include/linux/drm/drm_ioctl.h)
> +
> +use crate::ioctl;
> +
> +const BASE: u32 = bindings::DRM_IOCTL_BASE as u32;
> +
> +/// Construct a DRM ioctl number with no argument.
> +pub const fn IO(nr: u32) -> u32 {
> +    ioctl::_IO(BASE, nr)
> +}
> +
> +/// Construct a DRM ioctl number with a read-only argument.
> +pub const fn IOR<T>(nr: u32) -> u32 {
> +    ioctl::_IOR::<T>(BASE, nr)
> +}
> +
> +/// Construct a DRM ioctl number with a write-only argument.
> +pub const fn IOW<T>(nr: u32) -> u32 {
> +    ioctl::_IOW::<T>(BASE, nr)
> +}
> +
> +/// Construct a DRM ioctl number with a read-write argument.
> +pub const fn IOWR<T>(nr: u32) -> u32 {
> +    ioctl::_IOWR::<T>(BASE, nr)
> +}
> +
> +/// Descriptor type for DRM ioctls. Use the `declare_drm_ioctls!{}` macro to construct them.
> +pub type DrmIoctlDescriptor = bindings::drm_ioctl_desc;
> +
> +/// This is for ioctl which are used for rendering, and require that the file descriptor is either
> +/// for a render node, or if it’s a legacy/primary node, then it must be authenticated.
> +pub const AUTH: u32 = bindings::drm_ioctl_flags_DRM_AUTH;
> +
> +/// This must be set for any ioctl which can change the modeset or display state. Userspace must
> +/// call the ioctl through a primary node, while it is the active master.
> +///
> +/// Note that read-only modeset ioctl can also be called by unauthenticated clients, or when a
> +/// master is not the currently active one.
> +pub const MASTER: u32 = bindings::drm_ioctl_flags_DRM_MASTER;
> +
> +/// Anything that could potentially wreak a master file descriptor needs to have this flag set.
> +///
> +/// Current that’s only for the SETMASTER and DROPMASTER ioctl, which e.g. logind can call to force
> +/// a non-behaving master (display compositor) into compliance.
> +///
> +/// This is equivalent to callers with the SYSADMIN capability.
> +pub const ROOT_ONLY: u32 = bindings::drm_ioctl_flags_DRM_ROOT_ONLY;
> +
> +/// Whether drm_ioctl_desc.func should be called with the DRM BKL held or not. Enforced as the
> +/// default for all modern drivers, hence there should never be a need to set this flag.
> +///
> +/// Do not use anywhere else than for the VBLANK_WAIT IOCTL, which is the only legacy IOCTL which
> +/// needs this.
> +pub const UNLOCKED: u32 = bindings::drm_ioctl_flags_DRM_UNLOCKED;
> +
> +/// This is used for all ioctl needed for rendering only, for drivers which support render nodes.
> +/// This should be all new render drivers, and hence it should be always set for any ioctl with
> +/// `AUTH` set. Note though that read-only query ioctl might have this set, but have not set
> +/// DRM_AUTH because they do not require authentication.
> +pub const RENDER_ALLOW: u32 = bindings::drm_ioctl_flags_DRM_RENDER_ALLOW;
> +
> +/// Declare the DRM ioctls for a driver.
> +///
> +/// Each entry in the list should have the form:
> +///
> +/// `(ioctl_number, argument_type, flags, user_callback),`
> +///
> +/// `argument_type` is the type name within the `bindings` crate.
> +/// `user_callback` should have the following prototype:
> +///
> +/// ```
> +/// fn foo(device: &kernel::drm::device::Device<Self>,
> +///        data: &mut bindings::argument_type,
> +///        file: &kernel::drm::file::File<Self::File>,
> +/// )
> +/// ```
> +/// where `Self` is the drm::drv::Driver implementation these ioctls are being declared within.
> +///
> +/// # Examples
> +///
> +/// ```
> +/// kernel::declare_drm_ioctls! {
> +///     (FOO_GET_PARAM, drm_foo_get_param, ioctl::RENDER_ALLOW, my_get_param_handler),
> +/// }
> +/// ```
> +///
> +#[macro_export]
> +macro_rules! declare_drm_ioctls {
> +    ( $(($cmd:ident, $struct:ident, $flags:expr, $func:expr)),* $(,)? ) => {
> +        const IOCTLS: &'static [$crate::drm::ioctl::DrmIoctlDescriptor] = {
> +            const _:() = {
> +                let i: u32 = $crate::bindings::DRM_COMMAND_BASE;
> +                // Assert that all the IOCTLs are in the right order and there are no gaps,
> +                // and that the sizeof of the specified type is correct.
> +                $(
> +                    let cmd: u32 = $crate::macros::concat_idents!($crate::bindings::DRM_IOCTL_, $cmd);
> +                    ::core::assert!(i == $crate::ioctl::_IOC_NR(cmd));
> +                    ::core::assert!(core::mem::size_of::<$crate::bindings::$struct>() == $crate::ioctl::_IOC_SIZE(cmd));
> +                    let i: u32 = i + 1;
> +                )*
> +            };
> +
> +            let ioctls = &[$(
> +                $crate::bindings::drm_ioctl_desc {
> +                    cmd: $crate::macros::concat_idents!($crate::bindings::DRM_IOCTL_, $cmd) as u32,
> +                    func: {
> +                        #[allow(non_snake_case)]
> +                        unsafe extern "C" fn $cmd(
> +                                raw_dev: *mut $crate::bindings::drm_device,
> +                                raw_data: *mut ::core::ffi::c_void,
> +                                raw_file_priv: *mut $crate::bindings::drm_file,
> +                        ) -> core::ffi::c_int {
> +                            // SAFETY: We never drop this, and the DRM core ensures the device lives
> +                            // while callbacks are being called.
> +                            //
> +                            // FIXME: Currently there is nothing enforcing that the types of the
> +                            // dev/file match the current driver these ioctls are being declared
> +                            // for, and it's not clear how to enforce this within the type system.
> +                            let dev = ::core::mem::ManuallyDrop::new(unsafe {
> +                                $crate::drm::device::Device::from_raw(raw_dev)
> +                            });
> +                            // SAFETY: This is just the ioctl argument, which hopefully has the right type
> +                            // (we've done our best checking the size).
> +                            let data = unsafe { &mut *(raw_data as *mut $crate::bindings::$struct) };
> +                            // SAFETY: This is just the DRM file structure
> +                            let file = unsafe { $crate::drm::file::File::from_raw(raw_file_priv) };
> +
> +                            match $func(&*dev, data, &file) {
> +                                Err(e) => e.to_kernel_errno(),
> +                                Ok(i) => i.try_into().unwrap_or(ERANGE.to_kernel_errno()),
> +                            }
> +                        }
> +                        Some($cmd)
> +                    },
> +                    flags: $flags,
> +                    name: $crate::c_str!(::core::stringify!($cmd)).as_char_ptr(),
> +                }
> +            ),*];
> +            ioctls
> +        };
> +    };
> +}
> diff --git a/rust/kernel/drm/mod.rs b/rust/kernel/drm/mod.rs
> new file mode 100644
> index 000000000000..9ec6d7cbcaf3
> --- /dev/null
> +++ b/rust/kernel/drm/mod.rs
> @@ -0,0 +1,5 @@
> +// SPDX-License-Identifier: GPL-2.0 OR MIT
> +
> +//! DRM subsystem abstractions.
> +
> +pub mod ioctl;
> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index 7903490816bf..cb23d24c6718 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -37,6 +37,8 @@ mod build_assert;
>  pub mod delay;
>  pub mod device;
>  pub mod driver;
> +#[cfg(CONFIG_RUST_DRM)]
> +pub mod drm;
>  pub mod error;
>  pub mod io_buffer;
>  pub mod io_mem;
> 
> -- 
> 2.35.1
>