Rust bindings for gem shmem + iosys_map
Subsystems (6)
v7 cover Rust bindings for gem shmem + iosys_map
This is the next version of the shmem backed GEM objects series originally from Asahi, previously posted by Daniel Almeida.
One of the major changes in this patch series is a much better interface around vmaps, which we achieve by introducing a new set of rust bindings for iosys_map.
The previous version of the patch series can be found here:
https://patchwork.freedesktop.org/series/156093/
This patch series may be applied on top of the
driver-core/driver-core-testing branch:
Changelogs are per-patch
Asahi Lina (2):
rust: helpers: Add bindings/wrappers for dma_resv_lock
rust: drm: gem: shmem: Add DRM shmem helper abstraction
Lyude Paul (5):
rust: drm: gem: Add raw_dma_resv() function
rust: gem: Introduce DriverObject::Args
rust: drm: gem: Introduce shmem::SGTable
rust: Introduce iosys_map bindings
rust: drm/gem: Add vmap functions to shmem bindings
drivers/gpu/drm/nova/gem.rs | 5 +- drivers/gpu/drm/tyr/gem.rs | 3 +- rust/bindings/bindings_helper.h | 3 + rust/helpers/dma-resv.c | 13 + rust/helpers/drm.c | 56 +++- rust/helpers/helpers.c | 2 + rust/helpers/iosys_map.c | 34 +++ rust/kernel/drm/gem/mod.rs | 28 +- rust/kernel/drm/gem/shmem.rs | 467 ++++++++++++++++++++++++++++++++ rust/kernel/iosys_map.rs | 210 ++++++++++++++ rust/kernel/lib.rs | 1 + 11 files changed, 815 insertions(+), 7 deletions(-) create mode 100644 rust/helpers/dma-resv.c create mode 100644 rust/helpers/iosys_map.c create mode 100644 rust/kernel/drm/gem/shmem.rs create mode 100644 rust/kernel/iosys_map.rs
base-commit: 21bab791346e5b7902a04709231c0642ff6d69bc
prerequisite-patch-id: c631986f96e2073263e97e82a65b96fc5ada6924
prerequisite-patch-id: ae853e8eb8d58c77881371960be4ae92755e83c6
prerequisite-patch-id: 0ab78b50648c7d8f66b83c32ed2af0ec3ede42a3
prerequisite-patch-id: 8d20a8db8cf4660c682ee91f3db04e640c144e33
prerequisite-patch-id: 299de9cd2789c19c22e2816f7c817d80d5a4f1db
prerequisite-patch-id: 661ee334905f359daa8fb8d808ed5f4a8085f5c9
prerequisite-patch-id: 05aef545e564948160354e498a3d3c5fd5bbfacb
Signature · 1 line
2.53.0
PATCH v7 1/7 rust: drm: gem: Add raw_dma_resv() function
For retrieving a pointer to the struct dma_resv for a given GEM object. We also introduce it in a new trait, BaseObjectPrivate, which we automatically implement for all gem objects and don't expose to users outside of the crate.
--- rust/kernel/drm/gem/mod.rs | 12 ++++++++++++ 1 file changed, 12 insertions(+)
rust/kernel/drm/gem/mod.rs +12 -0
@@ -197,6 +197,18 @@ fn create_mmap_offset(&self) -> Result<u64> {impl <T :IntoGEMObject >BaseObject for T {} +/// Crate-private base operations shared by all GEM object classes. +#[expect(unused)] +pub (crate )trait BaseObjectPrivate :IntoGEMObject { +/// Return a pointer to this object's dma_resv. +fn raw_dma_resv (&self) -> *mut bindings::dma_resv { +// SAFETY: `as_gem_obj()` always returns a valid pointer to the base DRM gem object +unsafe { (*self.as_raw ()).resv } + } +} + +impl <T :IntoGEMObject >BaseObjectPrivate for T {} +/// A base GEM object. /// /// # Invariants
Signature · 1 line
2.53.0
Alice Ryhl
maintainerRe: [PATCH v7 1/7] rust: drm: gem: Add raw_dma_resv() function
Quoted 1 line · Lyude Paul
Reviewed-by Janne Grunau <[email protected]>
I think this email is misspelled?
Re: [PATCH v7 1/7] rust: drm: gem: Add raw_dma_resv() function
Quoted 4 lines · Alice Ryhl
Quoted 1 line · Lyude Paul
Reviewed-by Janne Grunau <[email protected]>I think this email is misspelled?
It is, likely my fault. I noticed a few of those particular misspellings in the last few weeks.
Janne
PATCH v7 2/7 rust: helpers: Add bindings/wrappers for dma_resv_lock
In reply to Lyude PaulFrom: Asahi Lina <[email protected]>
This is just for basic usage in the DRM shmem abstractions for implied locking, not intended as a full DMA Reservation abstraction yet.
--- rust/bindings/bindings_helper.h | 1 + rust/helpers/dma-resv.c | 13 +++++++++++++ rust/helpers/helpers.c | 1 + 3 files changed, 15 insertions(+) create mode 100644 rust/helpers/dma-resv.c
rust/bindings/bindings_helper.h +1 -0
@@ -48,6 +48,7 @@#include <linux/cpumask.h> #include <linux/cred.h> #include <linux/debugfs.h> +#include <linux/dma-resv.h> #include <linux/device/faux.h> #include <linux/dma-direction.h> #include <linux/dma-mapping.h>
rust/helpers/dma-resv.c new file +13 -0
@@ -0,0 +1,13 @@ +// SPDX-License-Identifier: GPL-2.0 + +#include <linux/dma-resv.h> + +int rust_helper_dma_resv_lock (struct dma_resv *obj,struct ww_acquire_ctx *ctx) +{ +return dma_resv_lock (obj, ctx); +} + +void rust_helper_dma_resv_unlock (struct dma_resv *obj) +{ +dma_resv_unlock (obj); +}
rust/helpers/helpers.c +1 -0
@@ -25,6 +25,7 @@#include "cred.c" #include "device.c" #include "dma.c" +#include "dma-resv.c" #include "drm.c" #include "err.c" #include "irq.c"
Signature · 1 line
2.53.0
PATCH v7 3/7 rust: gem: Introduce DriverObject::Args
In reply to Lyude PaulThis is an associated type that may be used in order to specify a data-type to pass to gem objects when construction them, allowing for drivers to more easily initialize their private-data for gem objects.
---
V3:
* s/BaseDriverObject/DriverObject/
V4:
* Fix leftover reference to BaseObjectDriver in rustdoc for DriverObject::Args
V6:
* Fix build errors in Tyr
drivers/gpu/drm/nova/gem.rs | 5 +++-- drivers/gpu/drm/tyr/gem.rs | 3 ++- rust/kernel/drm/gem/mod.rs | 13 ++++++++++--- 3 files changed, 15 insertions(+), 6 deletions(-)
drivers/gpu/drm/nova/gem.rs +3 -2
@@ -18,8 +18,9 @@ pub(crate) struct NovaObject {}impl gem::DriverObject for NovaObject {type Driver =NovaDriver ; +type Args = (); -fn new (_dev: &NovaDevice , _size:usize ) ->impl PinInit <Self ,Error > { +fn new (_dev: &NovaDevice , _size:usize , _args:Self ::Args ) ->impl PinInit <Self ,Error > {try_pin_init! (NovaObject {}) } }
@@ -33,7 +34,7 @@ pub(crate) fn new(dev: &NovaDevice, size: usize) -> Result<ARef<gem::Object<Selfreturn Err (EINVAL ); } - gem::Object ::new (dev, aligned_size) + gem::Object ::new (dev, aligned_size, ()) }/// Look up a GEM object handle for a `File` and return an `ObjectRef` for it.
drivers/gpu/drm/tyr/gem.rs +2 -1
@@ -11,8 +11,9 @@ pub(crate) struct TyrObject {}impl gem::DriverObject for TyrObject {type Driver =TyrDriver ; +type Args = (); -fn new (_dev: &TyrDevice , _size:usize ) ->impl PinInit <Self ,Error > { +fn new (_dev: &TyrDevice , _size:usize , _args: ()) ->impl PinInit <Self ,Error > {try_pin_init! (TyrObject {}) } }
rust/kernel/drm/gem/mod.rs +10 -3
@@ -62,8 +62,15 @@ pub trait DriverObject: Sync + Send + Sized {/// Parent `Driver` for this object. type Driver : drm::Driver ; +/// The data type to use for passing arguments to [`DriverObject::new`]. +type Args ; +/// 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 >; +fn new ( + dev: &drm::Device <Self ::Driver >, + size:usize , + args:Self ::Args , + ) ->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 {
@@ -242,11 +249,11 @@ impl<T: DriverObject> Object<T> { };/// Create a new GEM object. -pub fn new (dev: &drm::Device <T ::Driver >, size:usize ) ->Result <ARef <Self >> { +pub fn new (dev: &drm::Device <T ::Driver >, size:usize , args:T ::Args ) ->Result <ARef <Self >> {let obj:Pin <KBox <Self >> =KBox ::pin_init (try_pin_init! (Self { obj:Opaque ::new (bindings::drm_gem_object::default ()), - data <-T ::new (dev, size), + data <-T ::new (dev, size, args), }),GFP_KERNEL , )?;
Signature · 1 line
2.53.0
PATCH v7 4/7 rust: drm: gem: shmem: Add DRM shmem helper abstraction
In reply to Lyude PaulFrom: Asahi Lina <[email protected]>
The DRM shmem helper includes common code useful for drivers which allocate GEM objects as anonymous shmem. Add a Rust abstraction for this. Drivers can choose the raw GEM implementation or the shmem layer, depending on their needs.
---
V2:
* Use the drm_gem_shmem_init() and drm_gem_shmem_release() that I extracted so we can handle memory allocation in rust, which means we no longer have to handle freeing rust members of the struct by hand and have a closer implementation to the main gem object
(this also gets rid of gem_create_object)
* Get rid of GemObjectRef and UniqueGemObjectRef, we have ARef<T> at home.
* Use Device<T::Driver> in Object<T>
* Cleanup Object::<T>::new() a bit:
* Cleanup safety comment
* Use cast_mut()
* Just import container_of!(), we use it all over anyhow
* mut_shmem() -> as_shmem(), make it safe (there's no reason for being unsafe)
* Remove any *const and *muts in structs, just use NonNull
* Get rid of the previously hand-rolled sg_table bindings in shmem, use the bindings from Abdiel's sg_table patch series
* Add a TODO at the top about DMA reservation APIs and a desire for WwMutex
* Get rid of map_wc() and replace it with a new ObjectConfig struct. While it currently only specifies the map_wc flag, the idea here is that settings like map_wc() and parent_resv_obj() shouldn't be exposed as normal functions since the only place where it's safe to set them is when we're still guaranteed unique access to the GEM object, e.g. before returning it to the caller. Using a struct instead of individual arguments here is mainly because we'll be adding at least one more argument, and there's enough other gem shmem settings that trying to add all of them as individual function arguments in the future would be a bit messy.
* Get rid of vm_numa_fields!, Lina didn't like this macro much either and I think that it's fine for us to just specify the #[cfg(…)] attributes by hand since we only need to do it twice.
* Set drm_gem_object_funcs.vm_ops directly to drm_gem_shmem_vm_ops, don't export the various shmem funcs. I'm not sure why this wasn't possible before but it seems to work fine now.
V4:
* Switch from OpaqueObject to a normal Object<T> now that we've removed it
* Remove dev from Object, it's redundant as drm_gem_object already has a device pointer we can use.
* Use &raw instead of addr_of!()
V5:
* Fix typo in shmem::Object::as_raw()
* Add type invariant around obj always being a valid drm_gem_shmem_object for the duration of the lifetime of Object.
* Use Opaque::cast_from() instead of unrestricted casts
* Use IS_ENABLED() for gem shmem C helpers.
V7:
* Fix import style
* Don't forget to make Object<T> Send/Sync
rust/bindings/bindings_helper.h | 2 + rust/helpers/drm.c | 56 ++++++- rust/kernel/drm/gem/mod.rs | 5 +- rust/kernel/drm/gem/shmem.rs | 249 ++++++++++++++++++++++++++++++++ 4 files changed, 310 insertions(+), 2 deletions(-) create mode 100644 rust/kernel/drm/gem/shmem.rs
rust/bindings/bindings_helper.h +2 -0
@@ -33,6 +33,7 @@#include <drm/drm_drv.h> #include <drm/drm_file.h> #include <drm/drm_gem.h> +#include <drm/drm_gem_shmem_helper.h> #include <drm/drm_ioctl.h> #include <kunit/test.h> #include <linux/auxiliary_bus.h>
@@ -61,6 +62,7 @@#include <linux/fs.h> #include <linux/i2c.h> #include <linux/ioport.h> +#include <linux/iosys-map.h> #include <linux/jiffies.h> #include <linux/jump_label.h> #include <linux/mdio.h>
rust/helpers/drm.c +55 -1
65 lines not on this page · raw message
rust/kernel/drm/gem/mod.rs +4 -1
@@ -3,6 +3,8 @@//! DRM GEM API //! //! C header: [`include/drm/drm_gem.h`](srctree/include/drm/drm_gem.h) +#[cfg( CONFIG_DRM_GEM_SHMEM_HELPER )] +pub mod shmem;use crate ::{ alloc::flags::*,
@@ -50,6 +52,8 @@ unsafe fn dec_ref(obj: core::ptr::NonNull<Self>) { }; } +pub (crate )use impl_aref_for_gem_obj; +/// A type alias for retrieving a [`Driver`]s [`DriverFile`] implementation from its /// [`DriverObject`] implementation. ///
@@ -205,7 +209,6 @@ fn create_mmap_offset(&self) -> Result<u64> {impl <T :IntoGEMObject >BaseObject for T {}/// Crate-private base operations shared by all GEM object classes. -#[expect(unused)] pub (crate )trait BaseObjectPrivate :IntoGEMObject {/// Return a pointer to this object's dma_resv. fn raw_dma_resv (&self) -> *mut bindings::dma_resv {
rust/kernel/drm/gem/shmem.rs new file +249 -0
249 lines not on this page · raw message
Signature · 1 line
2.53.0
Re: [PATCH v7 4/7] rust: drm: gem: shmem: Add DRM shmem helper abstraction
Hi Lyude,
Quoted 197 lines · Lyude Paul
From: Asahi Lina <[email protected]>
The DRM shmem helper includes common code useful for drivers which allocate GEM objects as anonymous shmem. Add a Rust abstraction for this. Drivers can choose the raw GEM implementation or the shmem layer, depending on their needs.
Signed-off-by Asahi Lina <[email protected]>Signed-off-by Daniel Almeida <[email protected]>Signed-off-by Lyude Paul <[email protected]>---
V2:
* Use thedrm_gem_shmem_init()anddrm_gem_shmem_release()that I extracted so we can handle memory allocation in rust, which means we no longer have to handle freeing rust members of the struct by hand and have a closer implementation to the main gem object
(this also gets rid ofgem_create_object)
* Get rid of GemObjectRef and UniqueGemObjectRef, we have ARef<T> at home.
* Use Device<T::Driver> in Object<T>
* Cleanup Object::<T>::new() a bit:
* Cleanup safety comment
* Usecast_mut()
* Just import container_of!(), we use it all over anyhow
*mut_shmem()->as_shmem(), make it safe (there's no reason for being unsafe)
* Remove any *const and *muts in structs, just use NonNull
* Get rid of the previously hand-rolledsg_tablebindings in shmem, use the bindings from Abdiel'ssg_tablepatch series
* Add a TODO at the top about DMA reservation APIs and a desire for WwMutex
* Get rid ofmap_wc()and replace it with a new ObjectConfig struct. While it currently only specifies themap_wcflag, the idea here is that settings likemap_wc()andparent_resv_obj()shouldn't be exposed as normal functions since the only place where it's safe to set them is when we're still guaranteed unique access to the GEM object, e.g. before returning it to the caller. Using a struct instead of individual arguments here is mainly because we'll be adding at least one more argument, and there's enough other gem shmem settings that trying to add all of them as individual function arguments in the future would be a bit messy.
* Get rid ofvm_numa_fields!, Lina didn't like this macro much either and I think that it's fine for us to just specify the #[cfg(…)] attributes by hand since we only need to do it twice.
* Set drm_gem_object_funcs.vm_ops directly todrm_gem_shmem_vm_ops, don't export the various shmem funcs. I'm not sure why this wasn't possible before but it seems to work fine now.
V4:
* Switch from OpaqueObject to a normal Object<T> now that we've removed it
* Removedevfrom Object, it's redundant asdrm_gem_objectalready has a device pointer we can use.
* Use &raw instead of addr_of!()
V5:
* Fix typo inshmem::Object::as_raw()
* Add type invariant aroundobjalways being a validdrm_gem_shmem_objectfor the duration of the lifetime ofObject.
* UseOpaque::cast_from()instead of unrestricted casts
* UseIS_ENABLED()for gem shmem C helpers.
V7:
* Fix import style
* Don't forget to make Object<T> Send/Syncrust/bindings/bindings_helper.h | 2 + rust/helpers/drm.c | 56 ++++++- rust/kernel/drm/gem/mod.rs | 5 +- rust/kernel/drm/gem/shmem.rs | 249 ++++++++++++++++++++++++++++++++ 4 files changed, 310 insertions(+), 2 deletions(-)create mode 100644
rust/kernel/drm/gem/shmem.rsrust/bindings/bindings_helper.h +2
-0@@ -33,6 +33,7 @@#include <drm/drm_drv.h> #include <drm/drm_file.h> #include <drm/drm_gem.h> +#include <drm/drm_gem_shmem_helper.h> #include <drm/drm_ioctl.h> #include <kunit/test.h> #include <linux/auxiliary_bus.h> @@ -61,6 +62,7 @@#include <linux/fs.h> #include <linux/i2c.h> #include <linux/ioport.h> +#include <linux/iosys-map.h> #include <linux/jiffies.h> #include <linux/jump_label.h> #include <linux/mdio.h> rust/helpers/drm.c +55
-1@@ -1,6 +1,7 @@// SPDX-License-Identifier: GPL-2.0 #include <drm/drm_gem.h> +#include <drm/drm_gem_shmem_helper.h> #include <drm/drm_vma_manager.h> #ifdef CONFIG_DRM @@ -20,4 +21,57 @@ __u64 rust_helper_drm_vma_node_offset_addr(struct drm_vma_offset_node *node)return drm_vma_node_offset_addr (node); } -#endif +#if IS_ENABLED (CONFIG_DRM_GEM_SHMEM_HELPER ) +__rust_helper void +rust_helper_drm_gem_shmem_object_free (struct drm_gem_object *obj) +{ +return drm_gem_shmem_object_free (obj); +} + +__rust_helper void +rust_helper_drm_gem_shmem_object_print_info (struct drm_printer *p,unsigned int indent, +const struct drm_gem_object *obj) +{ +drm_gem_shmem_object_print_info (p, indent, obj); +} + +__rust_helper int +rust_helper_drm_gem_shmem_object_pin (struct drm_gem_object *obj) +{ +return drm_gem_shmem_object_pin (obj); +} + +__rust_helper void +rust_helper_drm_gem_shmem_object_unpin (struct drm_gem_object *obj) +{ +drm_gem_shmem_object_unpin (obj); +} + +__rust_helper struct sg_table * +rust_helper_drm_gem_shmem_object_get_sg_table (struct drm_gem_object *obj) +{ +return drm_gem_shmem_object_get_sg_table (obj); +} + +__rust_helper int +rust_helper_drm_gem_shmem_object_vmap (struct drm_gem_object *obj, +struct iosys_map *map) +{ +return drm_gem_shmem_object_vmap (obj, map); +} + +__rust_helper void +rust_helper_drm_gem_shmem_object_vunmap (struct drm_gem_object *obj, +struct iosys_map *map) +{ +drm_gem_shmem_object_vunmap (obj, map); +} + +__rust_helper int +rust_helper_drm_gem_shmem_object_mmap (struct drm_gem_object *obj,struct vm_area_struct *vma) +{ +return drm_gem_shmem_object_mmap (obj, vma); +} + +#endif /* CONFIG_DRM_GEM_SHMEM_HELPER */ +#endif /* CONFIG_DRM */ rust/kernel/drm/gem/mod.rs +4
-1@@ -3,6 +3,8 @@//! DRM GEM API //! //! C header: [`include/drm/drm_gem.h`](srctree/include/drm/drm_gem.h) +#[cfg( CONFIG_DRM_GEM_SHMEM_HELPER )] +pub mod shmem;use crate ::{ alloc::flags::*,@@ -50,6 +52,8 @@ unsafe fn dec_ref(obj: core::ptr::NonNull<Self>) { }; } +pub (crate )use impl_aref_for_gem_obj; +/// A type alias for retrieving a [`Driver`]s [`DriverFile`] implementation from its /// [`DriverObject`] implementation. /// @@ -205,7 +209,6 @@ fn create_mmap_offset(&self) -> Result<u64> {impl <T :IntoGEMObject >BaseObject for T {}/// Crate-private base operations shared by all GEM object classes. -#[expect(unused)] pub (crate )trait BaseObjectPrivate :IntoGEMObject {/// Return a pointer to this object's dma_resv. fn raw_dma_resv (&self) -> *mut bindings::dma_resv {rust/kernel/drm/gem/shmem.rs new file +0
-0
rust/kernel/drm/gem/shmem.rs line 10
Quoted 10 lines
rust/kernel/drm/gem/shmem.rs new file +10
-0@@ -0,0 +1,249 @@ +// SPDX-License-Identifier: GPL-2.0 + +//! DRM GEM shmem helper objects +//! +//! C header: [`include/linux/drm/drm_gem_shmem_helper.h`](srctree/include/linux/drm/drm_gem_shmem_helper.h) + +// TODO: +// - There are a number of spots here that manually acquire/release the DMA reservation lock using +// dma_resv_(un)lock(). In the future we should add support for ww mutex, expose a method to +// acquire a reference to the WwMutex, and then use that directly instead of the C functions here.
Right, thanks to Onur we’re pretty close on getting ww_mutex I’d say.
Quoted 87 lines
rust/kernel/drm/gem/shmem.rs new file +87
-0+ +use crate ::{ + container_of, + drm::{ + device, + driver, + gem, + private::Sealed ,// + }, + error::{ + from_err_ptr, + to_result,// + }, + prelude::*, + scatterlist, + types::{ +ARef , +Opaque ,// + },// +}; +use core::{ + ops::{ +Deref , +DerefMut ,// + }, + ptr::NonNull , +}; +use gem::{ +BaseObjectPrivate , +DriverObject , +IntoGEMObject ,// +}; + +/// A struct for controlling the creation of shmem-backed GEM objects. +/// +/// This is used with [`Object::new()`] to control various properties that can only be set when +/// initially creating a shmem-backed GEM object. +#[derive( Default )] +pub struct ObjectConfig <'a,T :DriverObject > { +/// Whether to set the write-combine map flag. +pub map_wc:bool , + +/// Reuse the DMA reservation from another GEM object. +/// +/// The newly created [`Object`] will hold an owned refcount to `parent_resv_obj` if specified. +pub parent_resv_obj:Option <&'aObject <T >>, +} + +/// A shmem-backed GEM object. +/// +/// # Invariants +/// +/// `obj` contains a valid initialized `struct drm_gem_shmem_object` for the lifetime of this +/// object. +#[repr( C )] +#[pin_data] +pub struct Object <T :DriverObject > { +#[pin] + obj:Opaque <bindings::drm_gem_shmem_object >, +// Parent object that owns this object's DMA reservation object + parent_resv_obj:Option <ARef <Object <T >>>, +#[pin] + inner:T , +} + +super ::impl_aref_for_gem_obj!(impl <T >for Object <T >where T :DriverObject ); + +// SAFETY: All GEM objects are thread-safe. +unsafe impl <T :DriverObject >Send for Object <T > {} + +// SAFETY: All GEM objects are thread-safe. +unsafe impl <T :DriverObject >Sync for Object <T > {} + +impl <T :DriverObject >Object <T > { +/// `drm_gem_object_funcs` vtable suitable for GEM shmem objects. +const VTABLE : bindings::drm_gem_object_funcs = bindings::drm_gem_object_funcs { + free:Some (Self ::free_callback), + open:Some (super ::open_callback ::<T >), + close:Some (super ::close_callback ::<T >), + print_info:Some (bindings::drm_gem_shmem_object_print_info), + export:None , + pin:Some (bindings::drm_gem_shmem_object_pin), + unpin:Some (bindings::drm_gem_shmem_object_unpin), + get_sg_table:Some (bindings::drm_gem_shmem_object_get_sg_table), + vmap:Some (bindings::drm_gem_shmem_object_vmap), + vunmap:Some (bindings::drm_gem_shmem_object_vunmap), + mmap:Some (bindings::drm_gem_shmem_object_mmap),
rust/kernel/drm/gem/shmem.rs line 107
Quoted 10 lines
rust/kernel/drm/gem/shmem.rs new file +10
-0+ status: None, + rss: None, +// SAFETY: `drm_gem_shmem_vm_ops` is static const on the C side, so immutable references are +// safe here and such references shall be valid forever + vm_ops: unsafe { &bindings::drm_gem_shmem_vm_ops }, + evict: None, + }; + +/// Return a raw pointer to the embedded drm_gem_shmem_object. +fn as_shmem (&self) -> *mut bindings::drm_gem_shmem_object {
nit: I’d shoehorn the term “raw” here somehow because of the pointer. Like as_raw_shmem() or something.
Quoted 146 lines
rust/kernel/drm/gem/shmem.rs new file +142
-0+ self.obj.get () + } + +/// Create a new shmem-backed DRM object of the given size. +/// +/// Additional config options can be specified using `config`. +pub fn new ( + dev: &device::Device <T ::Driver >, + size:usize , + config:ObjectConfig <'_,T >, + args:T ::Args , + ) ->Result <ARef <Self >> { +let new:Pin <KBox <Self >> =KBox ::try_pin_init ( +try_pin_init! (Self { + obj <-Opaque ::init_zeroed (), + parent_resv_obj: config.parent_resv_obj.map (|p| p.into ()), + inner <-T ::new (dev, size, args), + }), +GFP_KERNEL , + )?; + +// SAFETY: `obj.as_raw()` is guaranteed to be valid by the initialization above. +unsafe { (*new.as_raw ()).funcs = &Self ::VTABLE }; + +// SAFETY: The arguments are all valid via the type invariants. +to_result (unsafe { bindings::drm_gem_shmem_init (dev.as_raw (), new.as_shmem (), size) })?; + +// SAFETY: We never move out of `self`. +let new =KBox ::into_raw (unsafe {Pin ::into_inner_unchecked (new) }); + +// SAFETY: We're taking over the owned refcount from `drm_gem_shmem_init`. +let obj =unsafe {ARef ::from_raw (NonNull ::new_unchecked (new)) }; + +// Start filling out values from `config` +if let Some (parent_resv) = config.parent_resv_obj { +// SAFETY: We have yet to expose the new gem object outside of this function, so it is +// safe to modify this field. +unsafe { (*obj.obj.get ()).base.resv = parent_resv.raw_dma_resv () }; + } + +// SAFETY: We have yet to expose this object outside of this function, so we're guaranteed +// to have exclusive access - thus making this safe to hold a mutable reference to. +let shmem =unsafe { &mut *obj.as_shmem () }; + shmem.set_map_wc (config.map_wc); + +Ok (obj) + } + +/// Returns the `Device` that owns this GEM object. +pub fn dev (&self) -> &device::Device <T ::Driver > { +// SAFETY: `dev` will have been initialized in `Self::new()` by `drm_gem_shmem_init()`. +unsafe { device::Device ::from_raw ((*self.as_raw ()).dev) } + } + +extern "C" fn free_callback (obj: *mut bindings::drm_gem_object ) { +// SAFETY: +// - DRM always passes a valid gem object here +// - We used drm_gem_shmem_create() in our create_gem_object callback, so we know that +// `obj` is contained within a drm_gem_shmem_object +let this =unsafe {container_of! (obj, bindings::drm_gem_shmem_object, base) }; + +// SAFETY: +// - We're in free_callback - so this function is safe to call. +// - We won't be using the gem resources on `this` after this call. +unsafe { bindings::drm_gem_shmem_release (this) }; + +// SAFETY: +// - We verified above that `obj` is valid, which makes `this` valid +// - This function is set in AllocOps, so we know that `this` is contained within a +// `Object<T>` +let this =unsafe {container_of! (Opaque ::cast_from (this),Self , obj) }.cast_mut (); + +// SAFETY: We're recovering the Kbox<> we created in gem_create_object() +let _ =unsafe {KBox ::from_raw (this) }; + } + +/// Creates (if necessary) and returns an immutable reference to a scatter-gather table of DMA +/// pages for this object. +/// +/// This will pin the object in memory. +#[inline] +pub fn sg_table (&self) ->Result <&scatterlist::SGTable > { +// SAFETY: +// - drm_gem_shmem_get_pages_sgt is thread-safe. +// - drm_gem_shmem_get_pages_sgt returns either a valid pointer to a scatterlist, or an +// error pointer. +let sgt =from_err_ptr (unsafe { bindings::drm_gem_shmem_get_pages_sgt (self.as_shmem ()) })?; + +// SAFETY: We checked above that `sgt` is not an error pointer, so it must be a valid +// pointer to a scatterlist +Ok (unsafe { scatterlist::SGTable ::from_raw (sgt) }) + } +} + +impl <T :DriverObject >Deref for Object <T > { +type Target =T ; + +fn deref (&self) -> &Self ::Target { + &self.inner + } +} + +impl <T :DriverObject >DerefMut for Object <T > { +fn deref_mut (&mut self) -> &mut Self ::Target { + &mut self.inner + } +} + +impl <T :DriverObject >Sealed for Object <T > {} + +impl <T :DriverObject > gem::IntoGEMObject for Object <T > { +fn as_raw (&self) -> *mut bindings::drm_gem_object { +// SAFETY: +// - Our immutable reference is proof that this is safe to dereference. +// - `obj` is always a valid drm_gem_shmem_object via our type invariants. +unsafe { &raw mut (*self.obj.get ()).base } + } + +unsafe fn from_raw <'a>(obj: *mut bindings::drm_gem_object ) -> &'aObject <T > { +// SAFETY: The safety contract of from_gem_obj() guarantees that `obj` is contained within +// `Self` +unsafe { +let obj =Opaque ::cast_from (container_of! (obj, bindings::drm_gem_shmem_object, base)); + + &*container_of! (obj,Object <T >, obj) + } + } +} + +impl <T :DriverObject > driver::AllocImpl for Object <T > { +type Driver =T ::Driver ; + +const ALLOC_OPS : driver::AllocOps = driver::AllocOps { + gem_create_object:None , + prime_handle_to_fd:None , + prime_fd_to_handle:None , + gem_prime_import:None , + gem_prime_import_sg_table:Some (bindings::drm_gem_shmem_prime_import_sg_table), + dumb_create:Some (bindings::drm_gem_shmem_dumb_create), + dumb_map_offset:None , + }; +}Signature · 1 line
2.53.0
Looks good to me.
PATCH v7 5/7 rust: drm: gem: Introduce shmem::SGTable
In reply to Lyude PaulCurrently we expose the ability to retrieve an SGTable for an shmem gem object using gem::shmem::Object::<T>::sg_table(). However, this only gives us a borrowed reference. This being said - retrieving an SGTable is a fallible operation, and as such it's reasonable that a driver may want to hold onto an SGTable for longer then a reference would allow in order to avoid having to deal with fallibility every time they want to access the SGTable. One such driver with this usecase is the Asahi driver.
So to support this, let's introduce shmem::SGTable - which both holds a pointer to the SGTable and a reference to its respective GEM object in order to keep the GEM object alive for as long as the shmem::SGTable. The type can be used identically to a normal SGTable.
---
V3:
* Rename OwnedSGTable to shmem::SGTable. Since the current version of the SGTable abstractions now has a Owned and Borrowed variant, I think renaming this to shmem::SGTable makes things less confusing. We do however, keep the name of owned_sg_table() as-is.
V4:
* Clarify safety comments for SGTable to explain why the object is thread-safe.
* Rename from SGTableRef to SGTable
rust/kernel/drm/gem/shmem.rs | 50 ++++++++++++++++++++++++++++++++++++ 1 file changed, 50 insertions(+)
rust/kernel/drm/gem/shmem.rs +50 -0
@@ -197,6 +197,25 @@ pub fn sg_table(&self) -> Result<&scatterlist::SGTable> {// pointer to a scatterlist Ok (unsafe { scatterlist::SGTable ::from_raw (sgt) }) } + +/// Creates (if necessary) and returns an owned reference to a scatter-gather table of DMA pages +/// for this object. +/// +/// This is the same as [`sg_table`](Self::sg_table), except that it instead returns an +/// [`shmem::SGTable`] which holds a reference to the associated gem object, instead of a +/// reference to an [`scatterlist::SGTable`]. +/// +/// This will pin the object in memory. +/// +/// [`shmem::SGTable`]: SGTable +pub fn owned_sg_table (&self) ->Result <SGTable <T >> { +Ok (SGTable { + sgt: self.sg_table ()?.into (), +// INVARIANT: We take an owned refcount to `self` here, ensuring that `sgt` remains +// valid for as long as this `SGTable`. + _owner: self.into (), + }) + } }impl <T :DriverObject >Deref for Object <T > {
@@ -247,3 +266,34 @@ impl<T: DriverObject> driver::AllocImpl for Object<T> { dumb_map_offset: None, }; } + +/// An owned reference to a scatter-gather table of DMA address spans for a GEM shmem object. +/// +/// This object holds an owned reference to the underlying GEM shmem object, ensuring that the +/// [`scatterlist::SGTable`] referenced by this type remains valid for the lifetime of this object. +/// +/// # Invariants +/// +/// - `sgt` is kept alive by `_owner`, ensuring it remains valid for as long as `Self`. +/// - `sgt` corresponds to the owned object in `_owner`. +/// - This object is only exposed in situations where we know the underlying `SGTable` will not be +/// modified for the lifetime of this object. Thus, it is safe to send/access this type across +/// threads. +pub struct SGTable <T :DriverObject > { + sgt:NonNull <scatterlist::SGTable >, + _owner:ARef <Object <T >>, +} + +// SAFETY: This object is thread-safe via our type invariants. +unsafe impl <T :DriverObject >Send for SGTable <T > {} +// SAFETY: This object is thread-safe via our type invariants. +unsafe impl <T :DriverObject >Sync for SGTable <T > {} + +impl <T :DriverObject >Deref for SGTable <T > { +type Target = scatterlist::SGTable ; + +fn deref (&self) -> &Self ::Target { +// SAFETY: Creating an immutable reference to this is safe via our type invariants. +unsafe { self.sgt.as_ref () } + } +}
Signature · 1 line
2.53.0
Re: [PATCH v7 5/7] rust: drm: gem: Introduce shmem::SGTable
Quoted 102 lines · Lyude Paul
Currently we expose the ability to retrieve an SGTable for an shmem gem object using gem::shmem::Object::<T>::sg_table(). However, this only gives us a borrowed reference. This being said - retrieving an SGTable is a fallible operation, and as such it's reasonable that a driver may want to hold onto an SGTable for longer then a reference would allow in order to avoid having to deal with fallibility every time they want to access the SGTable. One such driver with this usecase is the Asahi driver.
So to support this, let's introduce
shmem::SGTable- which both holds a pointer to the SGTable and a reference to its respective GEM object in order to keep the GEM object alive for as long as theshmem::SGTable. The type can be used identically to a normal SGTable.Signed-off-by Lyude Paul <[email protected]>Reviewed-by Janne Grunau <[email protected]>---
V3:
* Rename OwnedSGTable toshmem::SGTable. Since the current version of the SGTable abstractions now has aOwnedandBorrowedvariant, I think renaming this toshmem::SGTablemakes things less confusing. We do however, keep the name ofowned_sg_table()as-is.
V4:
* Clarify safety comments for SGTable to explain why the object is thread-safe.
* Rename from SGTableRef to SGTable
rust/kernel/drm/gem/shmem.rs| 50 ++++++++++++++++++++++++++++++++++++1 file changed, 50 insertions(+)rust/kernel/drm/gem/shmem.rs +50
-0@@ -197,6 +197,25 @@ pub fn sg_table(&self) -> Result<&scatterlist::SGTable> {// pointer to a scatterlist Ok (unsafe { scatterlist::SGTable ::from_raw (sgt) }) } + +/// Creates (if necessary) and returns an owned reference to a scatter-gather table of DMA pages +/// for this object. +/// +/// This is the same as [`sg_table`](Self::sg_table), except that it instead returns an +/// [`shmem::SGTable`] which holds a reference to the associated gem object, instead of a +/// reference to an [`scatterlist::SGTable`]. +/// +/// This will pin the object in memory. +/// +/// [`shmem::SGTable`]: SGTable +pub fn owned_sg_table (&self) ->Result <SGTable <T >> { +Ok (SGTable { + sgt: self.sg_table ()?.into (), +// INVARIANT: We take an owned refcount to `self` here, ensuring that `sgt` remains +// valid for as long as this `SGTable`. + _owner: self.into (), + }) + } }impl <T :DriverObject >Deref for Object <T > {@@ -247,3 +266,34 @@ impl<T: DriverObject> driver::AllocImpl for Object<T> { dumb_map_offset: None, }; } + +/// An owned reference to a scatter-gather table of DMA address spans for a GEM shmem object. +/// +/// This object holds an owned reference to the underlying GEM shmem object, ensuring that the +/// [`scatterlist::SGTable`] referenced by this type remains valid for the lifetime of this object. +/// +/// # Invariants +/// +/// - `sgt` is kept alive by `_owner`, ensuring it remains valid for as long as `Self`. +/// - `sgt` corresponds to the owned object in `_owner`. +/// - This object is only exposed in situations where we know the underlying `SGTable` will not be +/// modified for the lifetime of this object. Thus, it is safe to send/access this type across +/// threads. +pub struct SGTable <T :DriverObject > { + sgt:NonNull <scatterlist::SGTable >, + _owner:ARef <Object <T >>, +} + +// SAFETY: This object is thread-safe via our type invariants. +unsafe impl <T :DriverObject >Send for SGTable <T > {} +// SAFETY: This object is thread-safe via our type invariants. +unsafe impl <T :DriverObject >Sync for SGTable <T > {} + +impl <T :DriverObject >Deref for SGTable <T > { +type Target = scatterlist::SGTable ; + +fn deref (&self) -> &Self ::Target { +// SAFETY: Creating an immutable reference to this is safe via our type invariants. +unsafe { self.sgt.as_ref () } + } +}Signature · 1 line
2.53.0
PATCH v7 6/7 rust: Introduce iosys_map bindings
In reply to Lyude PaulThis introduces a set of bindings for working with iosys_map in rust code using the new Io traits.
---
V5:
- Fix incorrect field size being passed to iosys_map_memcpy_to()
- Add an additional unit test, basic_macro(), which can successfully catch the above issue so it doesn't happen again in the future.
V6:
- Drop as_slice/as_mut_slice (Alice Rhyl)
V7:
- Start using Alexandre Courbot's wonderful Io, IoCapable and IoKnownSize traits instead of trying to roll our own IO accessors. This also changes the following:
- We don't have a custom AsBytes/FromBytes type that we carry around any longer with maps
- We now have optional compile-time size checking
- We don't need our own unit tests anymore
- RawIoSysMap can be used for unsafe IO operations, because why not.
- IoSysMapRef::new() can fail now since it needs to ensure SIZE is valid.
- We don't implement Deref<RawIoSysMap> for IoSysMapRef any longer. The main reason for this is that we want to avoid users having to manually specify if they want the RawIoSysMap or IoSysMapRef variant functions like io_read()/io_write().
This is fine IMHO, but to make sure things remain easy for APIs that wrap around iosys map we make IoSysMapRef.raw_map pub(crate).
rust/helpers/helpers.c | 1 + rust/helpers/iosys_map.c | 34 +++++++ rust/kernel/iosys_map.rs | 211 +++++++++++++++++++++++++++++++++++++++ rust/kernel/lib.rs | 1 + 4 files changed, 247 insertions(+) create mode 100644 rust/helpers/iosys_map.c create mode 100644 rust/kernel/iosys_map.rs
rust/helpers/helpers.c +1 -0
@@ -31,6 +31,7 @@#include "irq.c" #include "fs.c" #include "io.c" +#include "iosys_map.c" #include "jump_label.c" #include "kunit.c" #include "maple_tree.c"
rust/helpers/iosys_map.c new file +34 -0
@@ -0,0 +1,34 @@ +// SPDX-License-Identifier: GPL-2.0 + +#include <linux/iosys-map.h> +#include <linux/types.h> + +#define rust_iosys_map_rd (type__) \ + __rust_helper type__ \ + rust_helper_iosys_map_rd_ ## type__(const struct iosys_map *map, size_t offset) \ + { \ + return iosys_map_rd(map, offset, type__); \ + } +#define rust_iosys_map_wr (type__) \ + __rust_helper void \ + rust_helper_iosys_map_wr_ ## type__(const struct iosys_map *map, size_t offset, \ + type__ value) \ + { \ + iosys_map_wr(map, offset, type__, value); \ + } + +rust_iosys_map_rd (u8); +rust_iosys_map_rd (u16); +rust_iosys_map_rd (u32); + +rust_iosys_map_wr (u8); +rust_iosys_map_wr (u16); +rust_iosys_map_wr (u32); + +#ifdef CONFIG_64BIT +rust_iosys_map_rd (u64); +rust_iosys_map_wr (u64); +#endif + +#undef rust_iosys_map_rd +#undef rust_iosys_map_wr
rust/kernel/iosys_map.rs new file +211 -0
211 lines not on this page · raw message
rust/kernel/lib.rs +1 -0
@@ -103,6 +103,7 @@pub mod init;pub mod io;pub mod ioctl; +pub mod iosys_map;pub mod iov;pub mod irq;pub mod jump_label;
Signature · 1 line
2.53.0
Danilo Krummrich
maintainerRe: [PATCH v7 6/7] rust: Introduce iosys_map bindings
rust/kernel/iosys_map.rs line 32
Quoted 11 lines · Lyude Paul
rust/kernel/iosys_map.rs new file +11
-0+/// Raw unsized representation of a `struct iosys_map`. +/// +/// This struct is a transparent wrapper around `struct iosys_map`. The C API does not provide the +/// size of the mapping by default, and thus this type also does not include the size of the +/// mapping. As such, it cannot be used for actually accessing the underlying data pointed to by the +/// mapping. +/// +/// With the exception of kernel crates which may provide their own wrappers around `RawIoSysMap`, +/// users will typically not interact with this type directly. +#[repr(transparent)] +pub struct RawIoSysMap <const SIZE :usize =0 >(bindings::iosys_map );
I'm still against using struct iosys_map as a common frontend for I/O memory and system memory.
Exposing this as another I/O backend instead of just having a Rust structure as frontend for a "real" abstraction around the Rust backends has various downsides.
(1) We are limited to the features of struct iosys_map. The corresponding Rust backends may provide additional functionality, which we can't access with struct iosys_map. For instance, they Mmio will provide a relaxed() method providing access to a borrowed backend providing relaxed ordering accessors.
(2) We loose out on the capability to consider compile time checks regarding the guaranteed minimum size of the mapping. (To be fair this could be implemented on IoSysMap itself as well, but it would duplicate code that we already have in the corresponding backends.)
(3) You have to duplicate the safety requirements of the backends that struct iosys_map wraps. In fact, this series ignores that if the backend is I/O memory we have to guarantee the it is revoked when the device this I/O memory originates from is unbound.
Having a look at patch 7, it should be possible to read is_iomem and vaddr / vaddr_iomem from the struct iosys_map and just construct the "real" Mmio backend from it. We also have to create a backend for normal system memory, but that should be trivial. :)
Re: [PATCH v7 6/7] rust: Introduce iosys_map bindings
In reply to Lyude PaulHi Lyude,
Quoted 194 lines · Lyude Paul
This introduces a set of bindings for working with
iosys_mapin rust code using the new Io traits.Signed-off-by Lyude Paul <[email protected]>---
V5:
- Fix incorrect field size being passed toiosys_map_memcpy_to()
- Add an additional unit test,basic_macro(), which can successfully catch the above issue so it doesn't happen again in the future.
V6:
- Drop as_slice/as_mut_slice (Alice Rhyl)
V7:
- Start using Alexandre Courbot's wonderful Io, IoCapable and IoKnownSize traits instead of trying to roll our own IO accessors. This also changes the following:
- We don't have a custom AsBytes/FromBytes type that we carry around any longer with maps
- We now have optional compile-time size checking
- We don't need our own unit tests anymore
- RawIoSysMap can be used for unsafe IO operations, because why not.
-IoSysMapRef::new()can fail now since it needs to ensure SIZE is valid.
- We don't implement Deref<RawIoSysMap> for IoSysMapRef any longer. The main reason for this is that we want to avoid users having to manually specify if they want the RawIoSysMap or IoSysMapRef variant functions like io_read()/io_write().
This is fine IMHO, but to make sure things remain easy for APIs that wrap around iosys map we make IoSysMapRef.raw_map pub(crate).rust/helpers/helpers.c | 1 + rust/helpers/iosys_map.c | 34 +++++++ rust/kernel/iosys_map.rs | 211 +++++++++++++++++++++++++++++++++++++++ rust/kernel/lib.rs | 1 + 4 files changed, 247 insertions(+) create mode 100644 rust/helpers/iosys_map.c create mode 100644 rust/kernel/iosys_map.rsrust/helpers/helpers.c +1
-0@@ -31,6 +31,7 @@#include "irq.c" #include "fs.c" #include "io.c" +#include "iosys_map.c" #include "jump_label.c" #include "kunit.c" #include "maple_tree.c" rust/helpers/iosys_map.c new file +34
-0@@ -0,0 +1,34 @@ +// SPDX-License-Identifier: GPL-2.0 + +#include <linux/iosys-map.h> +#include <linux/types.h> + +#define rust_iosys_map_rd (type__) \ + __rust_helper type__ \ + rust_helper_iosys_map_rd_ ## type__(const struct iosys_map *map, size_t offset) \ + { \ + return iosys_map_rd(map, offset, type__); \ + } +#define rust_iosys_map_wr (type__) \ + __rust_helper void \ + rust_helper_iosys_map_wr_ ## type__(const struct iosys_map *map, size_t offset, \ + type__ value) \ + { \ + iosys_map_wr(map, offset, type__, value); \ + } + +rust_iosys_map_rd (u8); +rust_iosys_map_rd (u16); +rust_iosys_map_rd (u32); + +rust_iosys_map_wr (u8); +rust_iosys_map_wr (u16); +rust_iosys_map_wr (u32); + +#ifdef CONFIG_64BIT +rust_iosys_map_rd (u64); +rust_iosys_map_wr (u64); +#endif + +#undef rust_iosys_map_rd +#undef rust_iosys_map_wrrust/kernel/iosys_map.rs new file +99
-0@@ -0,0 +1,211 @@ +// SPDX-License-Identifier: GPL-2.0 + +//! IO-agnostic memory mapping interfaces. +//! +//! This crate provides bindings for the `struct iosys_map` type, which provides a common interface +//! for memory mappings which can reside within coherent memory, or within IO memory. +//! +//! C header: [`include/linux/iosys-map.h`](srctree/include/linux/pci.h) + +use crate ::{ + error::code::*, + io::{ +Io , +IoCapable , +IoKnownSize ,// + }, + prelude::*,// +}; +use bindings; +use core::marker::PhantomData ; + +/// Raw unsized representation of a `struct iosys_map`. +/// +/// This struct is a transparent wrapper around `struct iosys_map`. The C API does not provide the +/// size of the mapping by default, and thus this type also does not include the size of the +/// mapping. As such, it cannot be used for actually accessing the underlying data pointed to by the +/// mapping. +/// +/// With the exception of kernel crates which may provide their own wrappers around `RawIoSysMap`, +/// users will typically not interact with this type directly. +#[repr(transparent)] +pub struct RawIoSysMap <const SIZE :usize =0 >(bindings::iosys_map ); + +impl <const SIZE :usize >RawIoSysMap <SIZE > { +/// Convert from a raw `bindings::iosys_map`. +#[expect(unused)] +#[inline] +pub (crate )fn from_raw (val: bindings::iosys_map ) ->Self { +Self (val) + } + +/// Returns whether the mapping is within IO memory space or not. +#[inline] +pub fn is_iomem (&self) ->bool { + self.0 .is_iomem + } + +/// Convert from a `RawIoSysMap<SIZE>` to a raw `bindings::iosys_map` ref. +#[expect(unused)] +#[inline] +pub (crate )fn as_raw (&self) -> &bindings::iosys_map { + &self.0 + } + +/// Convert from a `RawIoSysMap<SIZE>` to a raw mutable `bindings::iosys_map` ref. +#[allow(unused)] +#[inline] +pub (crate )fn as_raw_mut (&mut self) -> &mut bindings::iosys_map { + &mut self.0 + } + +/// Returns the address pointed to by this iosys map. +/// +/// Note that this address is not guaranteed to be valid, and may or may not reside in I/O +/// memory. +#[inline] +pub fn addr (&self) ->usize { + (if self.is_iomem () { +// SAFETY: We confirmed above that this iosys map is contained within iomem, so it's +// safe to read vaddr_iomem +unsafe { self.0 .__bindgen_anon_1.vaddr_iomem } + }else { +// SAFETY: We confirmed above that this iosys map is not contaned within iomem, so it's +// safe to read vaddr. +unsafe { self.0 .__bindgen_anon_1.vaddr } + })as usize + } +} + +// SAFETY: As we make no guarantees about the validity of the mapping, there's no issue with sending +// this type between threads. +unsafe impl <const SIZE :usize >Send for RawIoSysMap <SIZE > {} + +impl <const SIZE :usize >Clone for RawIoSysMap <SIZE > { +fn clone (&self) ->Self { +Self (self.0 ) + } +} + +macro_rules! impl_raw_iosys_map_io_capable { + ($ty:ty, $read_fn:ident, $write_fn:ident) => { +impl <const SIZE :usize >IoCapable <$ty>for RawIoSysMap <SIZE > { +#[inline(always)] +unsafe fn io_read (&self, address:usize ) -> $ty { +// SAFETY: By the trait invariant `address` is a valid address for iosys map +// operations. +unsafe { bindings::$read_fn(&self.0 , address) } + } +
rust/kernel/iosys_map.rs line 109
Quoted 10 lines
rust/kernel/iosys_map.rs new file +10
-0+#[inline(always)] +unsafe fn io_write (&self, value: $ty, address:usize ) { +// SAFETY: By the trait invariant `address` is a valid address for iosys map +// operations. + unsafe { bindings::$write_fn(&self.0, address, value) }; + } + } + }; +} +
I think there might be a mismatch between the absolute address being passed to these read and write functions and the bindings helpers that expect an offset argument.
This crashed Tyr when I tried to write the firmware in u8 chunks at incremental offsets. But if i just calculated the offset and passed that instead of the absolute address, this worked fine:
let offset = address - self.addr();
unsafe { bindings::$write_fn(&self.0, offset, value) };Here's some of the call trace:
[ 31.553727] tyr fb000000.gpu: supply sram not found, using dummy regulator [ 31.555096] tyr fb000000.gpu: mali-unknown id 0xa867 major 0x67 minor 0x0 status 0x5 [ 31.555778] tyr fb000000.gpu: Features: L2:0x7120306 Tiler:0x809 Mem:0x301 MMU:0x2830 AS:0xff [ 31.556521] tyr fb000000.gpu: shader_present=0x0000000000050005 l2_present=0x0000000000000001 tiler_present=0x0000000000000001 [ 31.557868] stackdepot: allocating hash table of 524288 entries via kvcalloc [ OK ] Started systemd-tmpfiles-clean.tim…y Cleanup of Temporary Directories. [ 31.562424] stackdepot: allocating space for 8192 stack pools via kvcalloc [ OK ] Reached target timers.target - Timer Units. [ 31.563676] tyr: Firmware protected mode entry not supported, ignoring [ 31.571391] Unable to handle kernel paging request at virtual address 0000000000800069 [ 31.572762] Mem abort info: [ 31.573027] ESR = 0x0000000096000021 [ 31.573402] EC = 0x25: DABT (current EL), IL = 32 bits [ 31.573878] SET = 0, FnV = 0 [ 31.574157] EA = 0, S1PTW = 0 [ 31.574442] FSC = 0x21: alignment fault [ OK ] Listening on dbus.socket - D-Bus System Message Bus Socket. [ 31.593348] Data abort info: [ 31.593628] ISV = 0, ISS = 0x00000021, ISS2 = 0x00000000 [ 31.594117] CM = 0, WnR = 0, TnD = 0, TagAccess = 0 [ 31.594567] GCS = 0, Overlay = 0, DirtyBit = 0, Xs = 0 [ 31.595042] user pgtable: 4k pages, 48-bit VAs, pgdp=0000000105e50000 [ 31.595613] [0000000000800069] pgd=0000000000000000, p4d=0000000000000000 [ 31.596434] Internal error: Oops: 0000000096000021 [#1] SMP [ 31.596936] Modules linked in: snd tyr(+) soundcore sha256 cfg80211 rfkill pci_endpoint_test fuse dm_mod ipv6 [ 31.597830] CPU: 3 UID: 0 PID: 241 Comm: (udev-worker) Not tainted 6.19.0-rc5 #282 PREEMPT [ 31.598561] Hardware name: Radxa ROCK 5B (DT) [ 31.598944] pstate: 00400009 (nzcv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--) [ 31.599554] pc : __d_lookup_rcu+0xbc/0x168 [ 31.599921] lr : __d_lookup_rcu+0x60/0x168 [ 31.600283] sp : ffff800081ebba00 [ 31.600574] x29: ffff800081ebba00 x28: 0000000000000049 x27: ffff00010d07002b [ 31.601205] x26: 0000000000000081 x25: 000000000006915e x24: ffff0002f6600000 [ 31.601848] x23: ffff00010396a040 x22: ffff000102098460 x21: 000000036915e207 [ 31.601859] x20: ffff000101e10af8 x19: ffff800081ebbcac x18: 2f64662f666c6573 [ 31.601867] x17: ffffff00666c6573 x16: 00000000fffffffc x15: 0000000000003431 [ 31.601875] x14: 000000000000000f x13: 0080205100800107 x12: 0000000000800069 [ 31.601882] x11: ffffffffffffffff x10: 0000000000000018 x9 : 0000000000000003 [ 31.601890] x8 : 00000000008000a1 x7 : ffffb9d13df173c8 x6 : 0000000000000000 [ 31.601897] x5 : 0000000000000000 x4 : 0000000000000010 x3 : ffffffffffff0a00 [ 31.601905] x2 : ffff800081ebbcac x1 : ffffb9d1406be718 x0 : 0000000000000001 [ 31.601913] Call trace: [ 31.601915] __d_lookup_rcu+0xbc/0x168 (P) [ 31.601922] lookup_fast+0x98/0x174 [ 31.601929] link_path_walk+0x280/0x528 [ 31.601935] path_lookupat+0x60/0x1f0 [ 31.601941] do_o_path+0x34/0xb4 [ 31.601947] path_openat+0xccc/0xe34 [ 31.601953] do_filp_open+0xc0/0x170 [ 31.601958] do_sys_openat2+0x88/0x10c [ 31.601963] __arm64_sys_openat+0x70/0x9c [ 31.601968] invoke_syscall+0x40/0xcc [ 31.601974] el0_svc_common+0x80/0xd8 [ 31.601979] do_el0_svc+0x1c/0x28 [ 31.601984] el0_svc+0x54/0x1d4 [ 31.601991] el0t_64_sync_handler+0x84/0x12c [ 31.601997] el0t_64_sync+0x198/0x19c [ 31.602005] Code: 14000003 f9400108 b4000428 d100e10c (88dffd8c) [ 31.602009] ---[ end trace 0000000000000000 ]--- [ OK ] Listening on sshd-unix-local.socke…temd-ssh-generator, AF_UNIX Local). [ 31.620067] mc: Linux media interface: v0.10 [ 31.623483] [drm] Initialized panthor 1.5.0 for fb000000.gpu on minor 0 [ 31.623765] Unable to handle kernel paging request at virtual address 00802a4d0080284d [ 31.624285] tyr fb000000.gpu: Tyr initialized correctly. [ 31.624752] Mem abort info: [ 31.624754] ESR = 0x0000000096000004 [ 31.625847] EC = 0x25: DABT (current EL), IL = 32 bits [ 31.626310] SET = 0, FnV = 0 [ 31.626578] EA = 0, S1PTW = 0 [ 31.626853] FSC = 0x04: level 0 translation fault [ 31.627277] Data abort info: [ 31.627529] ISV = 0, ISS = 0x00000004, ISS2 = 0x00000000 [ 31.628006] CM = 0, WnR = 0, TnD = 0, TagAccess = 0 [ 31.628447] GCS = 0, Overlay = 0, DirtyBit = 0, Xs = 0 [ 31.628910] [00802a4d0080284d] address between user and kernel address ranges [ 31.629535] Internal error: Oops: 0000000096000004 [#2] SMP [ 31.630030] Modules linked in: mc drm_client_lib snd tyr soundcore sha256 cfg80211 rfkill pci_endpoint_test fuse dm_mod ipv6 [ 31.631017] CPU: 4 UID: 0 PID: 225 Comm: systemd-udevd Tainted: G D 6.19.0-rc5 #282 PREEMPT [ 31.631877] Tainted: [D]=DIE [ 31.632129] Hardware name: Radxa ROCK 5B (DT) [ 31.632506] pstate: a0400009 (NzCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--) [ 31.633111] pc : ___d_drop+0xd8/0x144 [ 31.633433] lr : d_invalidate+0x3c/0x110 [ 31.633776] sp : ffff800081d3ba70 [ 31.634064] x29: ffff800081d3ba90 x28: 0000000000000001 x27: ffffb9d140bfa000 [ 31.634685] x26: ffff0001039b5108 x25: ffff00010396a000 x24: ffffb9d140166275 [ 31.635305] x23: ffffb9d140bfa000 x22: ffffb9d140152f3a x21: ffff000104076000 [ 31.635925] x20: ffff00010e520098 x19: ffff00010396a000 x18: 0000000000000000 [ 31.636545] x17: 0000000000000000 x16: 0000000000000000 x15: 0000000000000000 [ 31.637165] x14: 0000000000000000 x13: ffff00010792b8f0 x12: 000000000000017a [ 31.637785] x11: 00000000008000a1 x10: 00802a4d0080284d x9 : ffff0002f6604010 [ 31.638405] x8 : ffff000105bfcd40 x7 : 0000000000000000 x6 : ffffb9d13e272c60 [ 31.639026] x5 : 0000000000000000 x4 : 0000000000000001 x3 : 0000000000000000 [ 31.639646] x2 : 0000000000000000 x1 : 0000000000000000 x0 : ffff00010396a000 [ 31.640266] Call trace: [ 31.640479] ___d_drop+0xd8/0x144 (P) [ 31.640800] d_invalidate+0x3c/0x110 [ 31.641113] proc_invalidate_siblings_dcache+0x244/0x2b8 [ 31.641577] proc_flush_pid+0x1c/0x28 [ 31.641897] release_task+0x560/0x668 [ 31.642218] wait_consider_task+0x5a0/0xb44 [ 31.642582] __do_wait+0x154/0x1f0 [ 31.642879] do_wait+0x84/0x16c [ 31.643154] __arm64_sys_waitid+0xac/0x218 [ 31.643512] invoke_syscall+0x40/0xcc [ 31.643833] el0_svc_common+0x80/0xd8 [ 31.644152] do_el0_svc+0x1c/0x28 [ 31.644441] el0_svc+0x54/0x1d4 [ 31.644718] el0t_64_sync_handler+0x84/0x12c [ 31.645090] el0t_64_sync+0x198/0x19c [ 31.645412] Code: 5280002a f85f83a0 17fffff1 a944280b (f940014c) [ 31.645941] ---[ end trace 0000000000000000 ]---
thanks,
Deborah
Re: [PATCH v7 6/7] rust: Introduce iosys_map bindings
Hey! Sorry for the long delay in response
Would you mind sending me the size/offset numbers you used here? My unit tests should have caught issues like this, so if they're happening I definitely should add it to the testcases I've got to make sure there's no problems
Quoted 424 lines · Deborah Brouwer
Hi Lyude,
Quoted 248 lines · Lyude Paul
This introduces a set of bindings for working with
iosys_mapin rust code
using the new Io traits.Signed-off-by Lyude Paul <[email protected]>---
V5:
- Fix incorrect field size being passed toiosys_map_memcpy_to()
- Add an additional unit test,basic_macro(), which can successfully catch
the above issue so it doesn't happen again in the future. V6:
- Drop as_slice/as_mut_slice (Alice Rhyl)
V7:
- Start using Alexandre Courbot's wonderful Io, IoCapable and IoKnownSize
traits instead of trying to roll our own IO accessors. This also changes
the following:
- We don't have a custom AsBytes/FromBytes type that we carry around any
longer with maps
- We now have optional compile-time size checking
- We don't need our own unit tests anymore
- RawIoSysMap can be used for unsafe IO operations, because why not.
-IoSysMapRef::new()can fail now since it needs to ensure SIZE is valid.
- We don't implement Deref<RawIoSysMap> for IoSysMapRef any longer. The
main reason for this is that we want to avoid users having to manually
specify if they want the RawIoSysMap or IoSysMapRef variant functions
like io_read()/io_write().
This is fine IMHO, but to make sure things remain easy for APIs that
wrap around iosys map we make IoSysMapRef.raw_map pub(crate).rust/helpers/helpers.c | 1 +
rust/helpers/iosys_map.c | 34 +++++++
rust/kernel/iosys_map.rs | 211
+++++++++++++++++++++++++++++++++++++++
rust/kernel/lib.rs | 1 +4 files changed, 247 insertions(+)create mode 100644
rust/helpers/iosys_map.c
create mode 100644rust/kernel/iosys_map.rsrust/helpers/helpers.c +1
-0@@ -31,6 +31,7 @@ #include "irq.c" #include "fs.c" #include "io.c" +#include "iosys_map.c" #include "jump_label.c" #include "kunit.c" #include "maple_tree.c"rust/helpers/iosys_map.c new file +6
-0@@ -0,0 +1,34 @@ +// SPDX-License-Identifier: GPL-2.0 + +#include <linux/iosys-map.h> +#include <linux/types.h> + +#define
rust_iosys_map_rd(type__)
\rust/helpers/iosys_map.c new file +1
-0+ __rust_helpertype__ \
rust/helpers/iosys_map.c new file +1
-0+ rust_helper_iosys_map_rd_ ## type__(const struct iosys_map*map,
size_toffset) \rust/helpers/iosys_map.c new file +1
-0+ {\
rust/helpers/iosys_map.c new file +1
-0+ return iosys_map_rd(map, offset,type__); \
rust/helpers/iosys_map.c new file +2
-0+ } +#define
rust_iosys_map_wr(type__)
\rust/helpers/iosys_map.c new file +1
-0+ __rust_helpervoid \
rust/helpers/iosys_map.c new file +1
-0+ rust_helper_iosys_map_wr_ ## type__(const struct iosys_map*map,
size_toffset, \rust/helpers/iosys_map.c new file +1
-0+ type__value) \
rust/helpers/iosys_map.c new file +1
-0+ {\
rust/helpers/iosys_map.c new file +1
-0+ iosys_map_wr(map, offset, type__,value); \
rust/helpers/iosys_map.c new file +17
-0+ } + +rust_iosys_map_rd (u8); +rust_iosys_map_rd (u16); +rust_iosys_map_rd (u32); + +rust_iosys_map_wr (u8); +rust_iosys_map_wr (u16); +rust_iosys_map_wr (u32); + +#ifdef CONFIG_64BIT +rust_iosys_map_rd (u64); +rust_iosys_map_wr (u64); +#endif + +#undef rust_iosys_map_rd +#undef rust_iosys_map_wrrust/kernel/iosys_map.rs new file +5
-0@@ -0,0 +1,211 @@ +// SPDX-License-Identifier: GPL-2.0 + +//! IO-agnostic memory mapping interfaces. +//! +//! This crate provides bindings for the `struct iosys_map` type, which provides a common interface
+//! for memory mappings which can reside within coherent memory, or within IO memory.rust/kernel/iosys_map.rs new file +2
-0+//! +//! C header: [`include/linux/iosys- map.h`](srctree/include/linux/pci.h)
rust/kernel/iosys_map.rs new file +16
-0+ +use crate ::{ + error::code::*, + io::{ + Io, + IoCapable, + IoKnownSize, // + }, + prelude::*, // +}; +use bindings; +use core::marker::PhantomData ; + +/// Raw unsized representation of a `struct iosys_map`. +/// +/// This struct is a transparent wrapper around `struct iosys_map`. The C API does not provide the
+/// size of the mapping by default, and thus this type also does not include the size of the
+/// mapping. As such, it cannot be used for actually accessing the underlying data pointed to by therust/kernel/iosys_map.rs new file +3
-0+/// mapping. +/// +/// With the exception of kernel crates which may provide their own wrappers around
RawIoSysMap,rust/kernel/iosys_map.rs new file +3
-0+/// users will typically not interact with this type directly. +#[repr(transparent)] +pub struct RawIoSysMap<const SIZE: usize =0>(bindings::iosys_map);
rust/kernel/iosys_map.rs new file +10
-0+ +impl <const SIZE :usize >RawIoSysMap <SIZE > { + /// Convert from a raw `bindings::iosys_map`. + #[expect(unused)] + #[inline] + pub(crate) fn from_raw(val: bindings::iosys_map) -> Self { + Self(val) + } + + /// Returns whether the mapping is within IO memory space ornot.
+ #[inline]
+ pub fnis_iomem(&self)-> bool {
+ self.0.is_iomem
+ }
+
+ /// Convert from aRawIoSysMap<SIZE>to a rawbindings::iosys_mapref.
+ #[expect(unused)]
+ #[inline]
+ pub(crate) fnas_raw(&self)->&bindings::iosys_map{ + &self.0
+ }
+
+ /// Convert from aRawIoSysMap<SIZE>to a raw mutablebindings::iosys_mapref.
+ #[allow(unused)]
+ #[inline]
+ pub(crate) fnas_raw_mut(&mut self)-> &mutbindings::iosys_map{
+ &mut self.0
+ }
+
+ /// Returns the address pointed to by this iosys map. + ///
+ /// Note that this address is not guaranteed to be valid, and may or may not reside in I/O
+ /// memory.
+ #[inline]
+ pub fnaddr(&self)-> usize {
+ (ifself.is_iomem(){
+ // SAFETY: We confirmed above that this iosys map is contained within iomem, so it's
+ // safe to readvaddr_iomem
+ unsafe { self.0.__bindgen_anon_1.vaddr_iomem } + } else {
+ // SAFETY: We confirmed above that this iosys map is not contaned within iomem, so it's
+ // safe to read vaddr.
+ unsafe { self.0.__bindgen_anon_1.vaddr }
+ }) as usize
+ }rust/kernel/iosys_map.rs new file +3
-0+} + +// SAFETY: As we make no guarantees about the validity of the mapping, there's no issue with sending
rust/kernel/iosys_map.rs new file +12
-0+// this type between threads. +unsafe impl <const SIZE :usize >Send for RawIoSysMap <SIZE > {} + +impl <const SIZE :usize >Clone for RawIoSysMap <SIZE > { + fn clone(&self) -> Self { + Self(self.0) + } +} + +macro_rules! impl_raw_iosys_map_io_capable { + ($ty:ty, $read_fn:ident, $write_fn:ident) => { + impl<const SIZE: usize> IoCapable<$ty> forRawIoSysMap<SIZE> {
+ #[inline(always)]
+ unsafe fnio_read(&self, address: usize)-> $ty { + // SAFETY: By the trait invariantaddressis a valid address for iosys map
+ // operations.
+ unsafe { bindings::$read_fn(&self.0, address) } + }
+
+ #[inline(always)]
+ unsafe fnio_write(&self, value: $ty, address: usize){
+ // SAFETY: By the trait invariantaddressis a valid address for iosys map
+ // operations.
+ unsafe { bindings::$write_fn(&self.0, address, value) };
+ }
+ }
+ };rust/kernel/iosys_map.rs
Quoted 2 lines
rust/kernel/iosys_map.rs new file +2
-0+} +I think there might be a mismatch between the absolute address being passed to these read and write functions and the bindings helpers that expect an offset argument.
This crashed Tyr when I tried to write the firmware in u8 chunks at incremental offsets. But if i just calculated the offset and passed that
instead of the absolute address, this worked fine:let offset = address -
self.addr();
unsafe { bindings::$write_fn(&self.0, offset, value) };Here's some of the call trace:
[ 31.553727] tyr fb000000.gpu: supply sram not found, using dummy regulator
[ 31.555096] tyr fb000000.gpu: mali-unknown id0xa867major0x67minor0x0status0x5
[ 31.555778] tyr fb000000.gpu: Features: L2:0x7120306 Tiler:0x809 Mem:0x301 MMU:0x2830 AS:0xff
[ 31.556521] tyr fb000000.gpu: shader_present=0x0000000000050005 l2_present=0x0000000000000001 tiler_present=0x0000000000000001 [ 31.557868] stackdepot: allocating hash table of 524288 entries via kvcalloc
[ OK ] Started systemd-tmpfiles-clean.tim…y Cleanup of Temporary Directories.
[ 31.562424] stackdepot: allocating space for 8192 stack pools via kvcalloc
[ OK ] Reached target timers.target - Timer Units.
[ 31.563676] tyr: Firmware protected mode entry not supported, ignoring
[ 31.571391] Unable to handle kernel paging request at virtual address 0000000000800069
[ 31.572762] Mem abort info:
[ 31.573027] ESR =0x0000000096000021
[ 31.573402] EC =0x25: DABT (current EL), IL = 32 bits [ 31.573878] SET = 0, FnV = 0
[ 31.574157] EA = 0, S1PTW = 0
[ 31.574442] FSC =0x21: alignment fault
[ OK ] Listening on dbus.socket - D-Bus System Message Bus Socket. [ 31.593348] Data abort info:
[ 31.593628] ISV = 0, ISS =0x00000021, ISS2 =0x00000000[ 31.594117] CM = 0, WnR = 0, TnD = 0, TagAccess = 0
[ 31.594567] GCS = 0, Overlay = 0, DirtyBit = 0, Xs = 0 [ 31.595042] user pgtable: 4k pages, 48-bit VAs, pgdp=0000000105e50000
[ 31.595613] [0000000000800069] pgd=0000000000000000, p4d=0000000000000000
[ 31.596434] Internal error: Oops: 0000000096000021 [#1] SMP [ 31.596936] Modules linked in: snd tyr(+) soundcore sha256 cfg80211 rfkillpci_endpoint_testfusedm_modipv6
[ 31.597830] CPU: 3 UID: 0 PID: 241 Comm: (udev-worker) Not tainted 6.19.0-rc5 #282 PREEMPT
[ 31.598561] Hardware name: Radxa ROCK 5B (DT)
[ 31.598944] pstate: 00400009 (nzcv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[ 31.599554] pc :__d_lookup_rcu+0xbc/0x168
[ 31.599921] lr :__d_lookup_rcu+0x60/0x168
[ 31.600283] sp :ffff800081ebba00
[ 31.600574] x29:ffff800081ebba00x28: 0000000000000049 x27:ffff00010d07002b
[ 31.601205] x26: 0000000000000081 x25:000000000006915ex24:ffff0002f6600000
[ 31.601848] x23:ffff00010396a040x22:ffff000102098460x21:000000036915e207
[ 31.601859] x20:ffff000101e10af8x19:ffff800081ebbcacx18:2f64662f666c6573
[ 31.601867] x17:ffffff00666c6573x16:00000000fffffffcx15:
0000000000003431
[ 31.601875] x14:000000000000000fx13: 0080205100800107 x12:
0000000000800069
[ 31.601882] x11: ffffffffffffffff x10: 0000000000000018 x9 :
0000000000000003
[ 31.601890] x8 :00000000008000a1x7 :ffffb9d13df173c8x6 :
0000000000000000
[ 31.601897] x5 : 0000000000000000 x4 : 0000000000000010 x3 :ffffffffffff0a00
[ 31.601905] x2 :ffff800081ebbcacx1 :ffffb9d1406be718x0 :
0000000000000001
[ 31.601913] Call trace:
[ 31.601915]__d_lookup_rcu+0xbc/0x168(P)
[ 31.601922]lookup_fast+0x98/0x174
[ 31.601929]link_path_walk+0x280/0x528
[ 31.601935]path_lookupat+0x60/0x1f0
[ 31.601941]do_o_path+0x34/0xb4
[ 31.601947]path_openat+0xccc/0xe34
[ 31.601953]do_filp_open+0xc0/0x170
[ 31.601958]do_sys_openat2+0x88/0x10c
[ 31.601963]__arm64_sys_openat+0x70/0x9c
[ 31.601968]invoke_syscall+0x40/0xcc
[ 31.601974]el0_svc_common+0x80/0xd8
[ 31.601979]do_el0_svc+0x1c/0x28
[ 31.601984]el0_svc+0x54/0x1d4
[ 31.601991]el0t_64_sync_handler+0x84/0x12c
[ 31.601997]el0t_64_sync+0x198/0x19c
[ 31.602005] Code: 14000003f9400108b4000428d100e10c(88dffd8c) [ 31.602009] ---[ end trace 0000000000000000 ]---
[ OK ] Listening on sshd-unix-local.socke…temd-ssh-generator,AF_UNIXLocal).
[ 31.620067] mc: Linux media interface: v0.10
[ 31.623483] [drm] Initialized panthor 1.5.0 for fb000000.gpu on minor 0
[ 31.623765] Unable to handle kernel paging request at virtual address00802a4d0080284d
[ 31.624285] tyr fb000000.gpu: Tyr initialized correctly. [ 31.624752] Mem abort info:
[ 31.624754] ESR =0x0000000096000004
[ 31.625847] EC =0x25: DABT (current EL), IL = 32 bits [ 31.626310] SET = 0, FnV = 0
[ 31.626578] EA = 0, S1PTW = 0
[ 31.626853] FSC =0x04: level 0 translation fault
[ 31.627277] Data abort info:
[ 31.627529] ISV = 0, ISS =0x00000004, ISS2 =0x00000000[ 31.628006] CM = 0, WnR = 0, TnD = 0, TagAccess = 0
[ 31.628447] GCS = 0, Overlay = 0, DirtyBit = 0, Xs = 0 [ 31.628910] [00802a4d0080284d] address between user and kernel address ranges
[ 31.629535] Internal error: Oops: 0000000096000004 [#2] SMP [ 31.630030] Modules linked in: mcdrm_client_libsnd tyr soundcore sha256 cfg80211 rfkillpci_endpoint_testfusedm_modipv6 [ 31.631017] CPU: 4 UID: 0 PID: 225 Comm: systemd-udevd Tainted:
G D 6.19.0-rc5 #282 PREEMPT
[ 31.631877] Tainted: [D]=DIE
[ 31.632129] Hardware name: Radxa ROCK 5B (DT)
[ 31.632506] pstate:a0400009(NzCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[ 31.633111] pc :___d_drop+0xd8/0x144
[ 31.633433] lr :d_invalidate+0x3c/0x110
[ 31.633776] sp :ffff800081d3ba70
[ 31.634064] x29:ffff800081d3ba90x28: 0000000000000001 x27:ffffb9d140bfa000
[ 31.634685] x26:ffff0001039b5108x25:ffff00010396a000x24:ffffb9d140166275
[ 31.635305] x23:ffffb9d140bfa000x22:ffffb9d140152f3ax21:ffff000104076000
[ 31.635925] x20:ffff00010e520098x19:ffff00010396a000x18:
0000000000000000
[ 31.636545] x17: 0000000000000000 x16: 0000000000000000 x15:
0000000000000000
[ 31.637165] x14: 0000000000000000 x13:ffff00010792b8f0x12:000000000000017a
[ 31.637785] x11:00000000008000a1x10:00802a4d0080284dx9 :ffff0002f6604010
[ 31.638405] x8 :ffff000105bfcd40x7 : 0000000000000000 x6 :ffffb9d13e272c60
[ 31.639026] x5 : 0000000000000000 x4 : 0000000000000001 x3 :
0000000000000000
[ 31.639646] x2 : 0000000000000000 x1 : 0000000000000000 x0 :ffff00010396a000
[ 31.640266] Call trace:
[ 31.640479]___d_drop+0xd8/0x144(P)
[ 31.640800]d_invalidate+0x3c/0x110
[ 31.641113]proc_invalidate_siblings_dcache+0x244/0x2b8[ 31.641577]proc_flush_pid+0x1c/0x28
[ 31.641897]release_task+0x560/0x668
[ 31.642218]wait_consider_task+0x5a0/0xb44
[ 31.642582]__do_wait+0x154/0x1f0
[ 31.642879]do_wait+0x84/0x16c
[ 31.643154]__arm64_sys_waitid+0xac/0x218
[ 31.643512]invoke_syscall+0x40/0xcc
[ 31.643833]el0_svc_common+0x80/0xd8
[ 31.644152]do_el0_svc+0x1c/0x28
[ 31.644441]el0_svc+0x54/0x1d4
[ 31.644718]el0t_64_sync_handler+0x84/0x12c
[ 31.645090]el0t_64_sync+0x198/0x19c
[ 31.645412] Code:5280002af85f83a017fffff1a944280b(f940014c) [ 31.645941] ---[ end trace 0000000000000000 ]---thanks,
Deborah
Re: [PATCH v7 6/7] rust: Introduce iosys_map bindings
Quoted 6 lines · On Wed, Mar 04, 2026 at 05:59:18PM -0500, [email protected] wrote:
Hey! Sorry for the long delay in response
Would you mind sending me the size/offset numbers you used here? My unit tests should have caught issues like this, so if they're happening I definitely should add it to the testcases I've got to make sure there's no problems
Sure, here's some more details of the firmware sections I am loading:
user@trixie:~$ dmesg | grep -i tyr [ 34.380056] tyr fb000000.gpu: supply sram not found, using dummy regulator [ 34.392969] tyr fb000000.gpu: mali-g610 id 0xa867 major 0x0 minor 0x0 status 0x5 [ 34.393625] tyr fb000000.gpu: Features: L2:0x7120306 Tiler:0x809 Mem:0x301 MMU:0x2830 AS:0xff [ 34.394408] tyr fb000000.gpu: shader_present=0x0000000000050005 l2_present=0x0000000000000001 tiler_present=0x0000000000000001 [ 34.400801] tyr: Firmware protected mode entry not supported, ignoring [ 34.401265] tyr: fw: Section 0 [ 34.403324] tyr: fw: init_section_mem() [ 34.403331] tyr: Base address of vmap: 0xffff800081c7f000 [ 34.403936] tyr: Size of allocated GEM buffer object (mem.bo.size()): 0x1000 [ 34.405314] tyr: Firmware data length: 0x7c [ 34.406627] tyr: fw: try_write8 - offset: 0x0, byte: 0x0 [ 34.407262] tyr: fw: try_write8 - offset: 0x1, byte: 0x0 [ 34.407901] tyr: fw: try_write8 - offset: 0x2, byte: 0x3 [ 34.408996] tyr: fw: try_write8 - offset: 0x3, byte: 0x2 [ 34.410050] tyr: fw: try_write8 - offset: 0x4, byte: 0x8d [ 34.410858] tyr: fw: try_write8 - offset: 0x5, byte: 0x1f [ 34.411940] tyr: fw: try_write8 - offset: 0x6, byte: 0x80 [ 34.412857] tyr: fw: try_write8 - offset: 0x7, byte: 0x0 [ 34.413747] tyr: fw: try_write8 - offset: 0x8, byte: 0x51 [ 34.413752] tyr: fw: try_write8 - offset: 0x9, byte: 0x20 [ 34.413924] tyr: fw: Section 1 [ 34.413940] tyr: fw: init_section_mem() [ 34.413943] tyr: Base address of vmap: 0xffff800081d1f000 [ 34.413947] tyr: Size of allocated GEM buffer object (mem.bo.size()): 0x2000 [ 34.413950] tyr: Firmware data length: 0x12dc [ 34.413952] tyr: fw: try_write8 - offset: 0x0, byte: 0x0 [ 34.413955] tyr: fw: try_write8 - offset: 0x1, byte: 0x0 [ 34.413958] tyr: fw: try_write8 - offset: 0x2, byte: 0x0 [ 34.413961] tyr: fw: try_write8 - offset: 0x3, byte: 0x0 [ 34.413964] tyr: fw: try_write8 - offset: 0x4, byte: 0x0 [ 34.413967] tyr: fw: try_write8 - offset: 0x5, byte: 0x0 [ 34.413969] tyr: fw: try_write8 - offset: 0x6, byte: 0x0 [ 34.413972] tyr: fw: try_write8 - offset: 0x7, byte: 0x0 [ 34.413975] tyr: fw: try_write8 - offset: 0x8, byte: 0x0 [ 34.413977] tyr: fw: try_write8 - offset: 0x9, byte: 0x0 [ 34.414043] tyr: fw: Section 2 [ 34.414050] tyr: fw: init_section_mem() [ 34.414052] tyr: Base address of vmap: 0xffff800081d13000 [ 34.414055] tyr: Size of allocated GEM buffer object (mem.bo.size()): 0x1000 [ 34.414058] tyr: Firmware data length: 0x7c [ 34.414061] tyr: fw: try_write8 - offset: 0x0, byte: 0x0 [ 34.414064] tyr: fw: try_write8 - offset: 0x1, byte: 0x0 [ 34.414066] tyr: fw: try_write8 - offset: 0x2, byte: 0x3 [ 34.414069] tyr: fw: try_write8 - offset: 0x3, byte: 0x2 [ 34.414072] tyr: fw: try_write8 - offset: 0x4, byte: 0x8d [ 34.414074] tyr: fw: try_write8 - offset: 0x5, byte: 0x1f [ 34.414077] tyr: fw: try_write8 - offset: 0x6, byte: 0x80 [ 34.414080] tyr: fw: try_write8 - offset: 0x7, byte: 0x0 [ 34.414082] tyr: fw: try_write8 - offset: 0x8, byte: 0x51 [ 34.414085] tyr: fw: try_write8 - offset: 0x9, byte: 0x20 [ 34.414346] tyr: fw: Section 3 [ 34.414358] tyr: fw: init_section_mem() [ 34.414361] tyr: Base address of vmap: 0xffff800083330000 [ 34.414364] tyr: Size of allocated GEM buffer object (mem.bo.size()): 0x20000 [ 34.414367] tyr: Firmware data length: 0x12ff0 [ 34.414370] tyr: fw: try_write8 - offset: 0x0, byte: 0xdf [ 34.414372] tyr: fw: try_write8 - offset: 0x1, byte: 0xf8 [ 34.414375] tyr: fw: try_write8 - offset: 0x2, byte: 0xc [ 34.414378] tyr: fw: try_write8 - offset: 0x3, byte: 0xd0 [ 34.414381] tyr: fw: try_write8 - offset: 0x4, byte: 0x2 [ 34.414384] tyr: fw: try_write8 - offset: 0x5, byte: 0xf0 [ 34.414386] tyr: fw: try_write8 - offset: 0x6, byte: 0x82 [ 34.414389] tyr: fw: try_write8 - offset: 0x7, byte: 0xfb [ 34.414392] tyr: fw: try_write8 - offset: 0x8, byte: 0x0 [ 34.414395] tyr: fw: try_write8 - offset: 0x9, byte: 0x48 [ 34.415676] tyr: fw: Section 4 [ 34.446336] tyr: fw: init_section_mem() [ 34.446345] tyr: Base address of vmap: 0xffff8000833a0000 [ 34.447001] tyr: Size of allocated GEM buffer object (mem.bo.size()): 0x40000 [ 34.447907] tyr: Firmware data length: 0x2b118 [ 34.449505] tyr: fw: try_write8 - offset: 0x0, byte: 0x0 [ 34.450342] tyr: fw: try_write8 - offset: 0x1, byte: 0x0 [ 34.451308] tyr: fw: try_write8 - offset: 0x2, byte: 0x0 [ 34.452244] tyr: fw: try_write8 - offset: 0x3, byte: 0x0 [ 34.453177] tyr: fw: try_write8 - offset: 0x4, byte: 0x0 [ 34.454122] tyr: fw: try_write8 - offset: 0x5, byte: 0x30 [ 34.455382] tyr: fw: try_write8 - offset: 0x6, byte: 0x0 [ 34.457468] tyr: fw: try_write8 - offset: 0x7, byte: 0x4 [ 34.458417] tyr: fw: try_write8 - offset: 0x8, byte: 0x0 [ 34.459111] tyr: fw: try_write8 - offset: 0x9, byte: 0x70 [ 34.461295] tyr: fw: Section 5 [ 34.462043] tyr: fw: init_section_mem() [ 34.462046] tyr: Base address of vmap: 0xffff8000833e1000 [ 34.462873] tyr: Size of allocated GEM buffer object (mem.bo.size()): 0x40000 [ 34.463793] tyr: Firmware data length: 0x2b118 [ 34.465285] tyr: fw: try_write8 - offset: 0x0, byte: 0x0 [ 34.466230] tyr: fw: try_write8 - offset: 0x1, byte: 0x0 [ 34.467201] tyr: fw: try_write8 - offset: 0x2, byte: 0x0 [ 34.468119] tyr: fw: try_write8 - offset: 0x3, byte: 0x0 [ 34.469064] tyr: fw: try_write8 - offset: 0x4, byte: 0x0 [ 34.470006] tyr: fw: try_write8 - offset: 0x5, byte: 0x30 [ 34.471145] tyr: fw: try_write8 - offset: 0x6, byte: 0x0 [ 34.471901] tyr: fw: try_write8 - offset: 0x7, byte: 0x4 [ 34.472846] tyr: fw: try_write8 - offset: 0x8, byte: 0x0 [ 34.473597] tyr: fw: try_write8 - offset: 0x9, byte: 0x70 [ 34.475292] tyr: fw: Section 6 [ 34.475752] tyr: fw: init_section_mem() [ 34.475756] tyr: Base address of vmap: 0xffff800083422000 [ 34.476570] tyr: Size of allocated GEM buffer object (mem.bo.size()): 0xc000 [ 34.477042] tyr: Firmware data length: 0x2000 [ 34.478676] tyr: fw: try_write8 - offset: 0x0, byte: 0x0 [ 34.479459] tyr: fw: try_write8 - offset: 0x1, byte: 0x0 [ 34.479462] tyr: fw: try_write8 - offset: 0x2, byte: 0x0 [ 34.479465] tyr: fw: try_write8 - offset: 0x3, byte: 0x0 [ 34.479468] tyr: fw: try_write8 - offset: 0x4, byte: 0x0 [ 34.479471] tyr: fw: try_write8 - offset: 0x5, byte: 0x0 [ 34.482247] tyr: fw: try_write8 - offset: 0x6, byte: 0x0 [ 34.482820] tyr: fw: try_write8 - offset: 0x7, byte: 0x0 [ 34.483289] tyr: fw: try_write8 - offset: 0x8, byte: 0x0 [ 34.483291] tyr: fw: try_write8 - offset: 0x9, byte: 0x20 [ 34.507842] tyr fb000000.gpu: Tyr initialized correctly.
Here's a link to the commit where I added these print statements if you would like to see more context:
https://gitlab.freedesktop.org/dbrouwer/linux/-/commit/2232b848a9ceef681382159c9b1471e6df0c00d6
Let me know if I can help more or test anything.
Thanks,
Deborah
Quoted 427 lines · Deborah Brouwer
Quoted 424 lines
Hi Lyude,
Quoted 248 lines · Lyude Paul
This introduces a set of bindings for working with
iosys_mapin rust code
using the new Io traits.Signed-off-by Lyude Paul <[email protected]>---
V5:
- Fix incorrect field size being passed toiosys_map_memcpy_to()
- Add an additional unit test,basic_macro(), which can successfully catch
the above issue so it doesn't happen again in the future. V6:
- Drop as_slice/as_mut_slice (Alice Rhyl)
V7:
- Start using Alexandre Courbot's wonderful Io, IoCapable and IoKnownSize
traits instead of trying to roll our own IO accessors. This also changes
the following:
- We don't have a custom AsBytes/FromBytes type that we carry around any
longer with maps
- We now have optional compile-time size checking
- We don't need our own unit tests anymore
- RawIoSysMap can be used for unsafe IO operations, because why not.
-IoSysMapRef::new()can fail now since it needs to ensure SIZE is valid.
- We don't implement Deref<RawIoSysMap> for IoSysMapRef any longer. The
main reason for this is that we want to avoid users having to manually
specify if they want the RawIoSysMap or IoSysMapRef variant functions
like io_read()/io_write().
This is fine IMHO, but to make sure things remain easy for APIs that
wrap around iosys map we make IoSysMapRef.raw_map pub(crate).rust/helpers/helpers.c | 1 +
rust/helpers/iosys_map.c | 34 +++++++
rust/kernel/iosys_map.rs | 211
+++++++++++++++++++++++++++++++++++++++
rust/kernel/lib.rs | 1 +4 files changed, 247 insertions(+)create mode 100644
rust/helpers/iosys_map.c
create mode 100644rust/kernel/iosys_map.rsrust/helpers/helpers.c +1
-0@@ -31,6 +31,7 @@ #include "irq.c" #include "fs.c" #include "io.c" +#include "iosys_map.c" #include "jump_label.c" #include "kunit.c" #include "maple_tree.c"rust/helpers/iosys_map.c new file +6
-0@@ -0,0 +1,34 @@ +// SPDX-License-Identifier: GPL-2.0 + +#include <linux/iosys-map.h> +#include <linux/types.h> + +#define
rust_iosys_map_rd(type__)
\rust/helpers/iosys_map.c new file +1
-0+ __rust_helpertype__ \
rust/helpers/iosys_map.c new file +1
-0+ rust_helper_iosys_map_rd_ ## type__(const struct iosys_map*map,
size_toffset) \rust/helpers/iosys_map.c new file +1
-0+ {\
rust/helpers/iosys_map.c new file +1
-0+ return iosys_map_rd(map, offset,type__); \
rust/helpers/iosys_map.c new file +2
-0+ } +#define
rust_iosys_map_wr(type__)
\rust/helpers/iosys_map.c new file +1
-0+ __rust_helpervoid \
rust/helpers/iosys_map.c new file +1
-0+ rust_helper_iosys_map_wr_ ## type__(const struct iosys_map*map,
size_toffset, \rust/helpers/iosys_map.c new file +1
-0+ type__value) \
rust/helpers/iosys_map.c new file +1
-0+ {\
rust/helpers/iosys_map.c new file +1
-0+ iosys_map_wr(map, offset, type__,value); \
rust/helpers/iosys_map.c new file +17
-0+ } + +rust_iosys_map_rd (u8); +rust_iosys_map_rd (u16); +rust_iosys_map_rd (u32); + +rust_iosys_map_wr (u8); +rust_iosys_map_wr (u16); +rust_iosys_map_wr (u32); + +#ifdef CONFIG_64BIT +rust_iosys_map_rd (u64); +rust_iosys_map_wr (u64); +#endif + +#undef rust_iosys_map_rd +#undef rust_iosys_map_wrrust/kernel/iosys_map.rs new file +5
-0@@ -0,0 +1,211 @@ +// SPDX-License-Identifier: GPL-2.0 + +//! IO-agnostic memory mapping interfaces. +//! +//! This crate provides bindings for the `struct iosys_map` type, which provides a common interface
+//! for memory mappings which can reside within coherent memory, or within IO memory.rust/kernel/iosys_map.rs new file +2
-0+//! +//! C header: [`include/linux/iosys- map.h`](srctree/include/linux/pci.h)
rust/kernel/iosys_map.rs new file +16
-0+ +use crate ::{ + error::code::*, + io::{ + Io, + IoCapable, + IoKnownSize, // + }, + prelude::*, // +}; +use bindings; +use core::marker::PhantomData ; + +/// Raw unsized representation of a `struct iosys_map`. +/// +/// This struct is a transparent wrapper around `struct iosys_map`. The C API does not provide the
+/// size of the mapping by default, and thus this type also does not include the size of the
+/// mapping. As such, it cannot be used for actually accessing the underlying data pointed to by therust/kernel/iosys_map.rs new file +3
-0+/// mapping. +/// +/// With the exception of kernel crates which may provide their own wrappers around
RawIoSysMap,rust/kernel/iosys_map.rs new file +3
-0+/// users will typically not interact with this type directly. +#[repr(transparent)] +pub struct RawIoSysMap<const SIZE: usize =0>(bindings::iosys_map);
rust/kernel/iosys_map.rs new file +10
-0+ +impl <const SIZE :usize >RawIoSysMap <SIZE > { + /// Convert from a raw `bindings::iosys_map`. + #[expect(unused)] + #[inline] + pub(crate) fn from_raw(val: bindings::iosys_map) -> Self { + Self(val) + } + + /// Returns whether the mapping is within IO memory space ornot.
+ #[inline]
+ pub fnis_iomem(&self)-> bool {
+ self.0.is_iomem
+ }
+
+ /// Convert from aRawIoSysMap<SIZE>to a rawbindings::iosys_mapref.
+ #[expect(unused)]
+ #[inline]
+ pub(crate) fnas_raw(&self)->&bindings::iosys_map{ + &self.0
+ }
+
+ /// Convert from aRawIoSysMap<SIZE>to a raw mutablebindings::iosys_mapref.
+ #[allow(unused)]
+ #[inline]
+ pub(crate) fnas_raw_mut(&mut self)-> &mutbindings::iosys_map{
+ &mut self.0
+ }
+
+ /// Returns the address pointed to by this iosys map. + ///
+ /// Note that this address is not guaranteed to be valid, and may or may not reside in I/O
+ /// memory.
+ #[inline]
+ pub fnaddr(&self)-> usize {
+ (ifself.is_iomem(){
+ // SAFETY: We confirmed above that this iosys map is contained within iomem, so it's
+ // safe to readvaddr_iomem
+ unsafe { self.0.__bindgen_anon_1.vaddr_iomem } + } else {
+ // SAFETY: We confirmed above that this iosys map is not contaned within iomem, so it's
+ // safe to read vaddr.
+ unsafe { self.0.__bindgen_anon_1.vaddr }
+ }) as usize
+ }rust/kernel/iosys_map.rs new file +3
-0+} + +// SAFETY: As we make no guarantees about the validity of the mapping, there's no issue with sending
rust/kernel/iosys_map.rs new file +12
-0+// this type between threads. +unsafe impl <const SIZE :usize >Send for RawIoSysMap <SIZE > {} + +impl <const SIZE :usize >Clone for RawIoSysMap <SIZE > { + fn clone(&self) -> Self { + Self(self.0) + } +} + +macro_rules! impl_raw_iosys_map_io_capable { + ($ty:ty, $read_fn:ident, $write_fn:ident) => { + impl<const SIZE: usize> IoCapable<$ty> forRawIoSysMap<SIZE> {
+ #[inline(always)]
+ unsafe fnio_read(&self, address: usize)-> $ty { + // SAFETY: By the trait invariantaddressis a valid address for iosys map
+ // operations.
+ unsafe { bindings::$read_fn(&self.0, address) } + }
+
+ #[inline(always)]
+ unsafe fnio_write(&self, value: $ty, address: usize){
+ // SAFETY: By the trait invariantaddressis a valid address for iosys map
+ // operations.
+ unsafe { bindings::$write_fn(&self.0, address, value) };
+ }
+ }
+ };rust/kernel/iosys_map.rs
Quoted 2 lines
rust/kernel/iosys_map.rs new file +2
-0+} +I think there might be a mismatch between the absolute address being passed to these read and write functions and the bindings helpers that expect an offset argument.
This crashed Tyr when I tried to write the firmware in u8 chunks at incremental offsets. But if i just calculated the offset and passed that
instead of the absolute address, this worked fine:let offset = address -
self.addr();
unsafe { bindings::$write_fn(&self.0, offset, value) };Here's some of the call trace:
[ 31.553727] tyr fb000000.gpu: supply sram not found, using dummy regulator
[ 31.555096] tyr fb000000.gpu: mali-unknown id0xa867major0x67minor0x0status0x5
[ 31.555778] tyr fb000000.gpu: Features: L2:0x7120306 Tiler:0x809 Mem:0x301 MMU:0x2830 AS:0xff
[ 31.556521] tyr fb000000.gpu: shader_present=0x0000000000050005 l2_present=0x0000000000000001 tiler_present=0x0000000000000001 [ 31.557868] stackdepot: allocating hash table of 524288 entries via kvcalloc
[ OK ] Started systemd-tmpfiles-clean.tim…y Cleanup of Temporary Directories.
[ 31.562424] stackdepot: allocating space for 8192 stack pools via kvcalloc
[ OK ] Reached target timers.target - Timer Units.
[ 31.563676] tyr: Firmware protected mode entry not supported, ignoring
[ 31.571391] Unable to handle kernel paging request at virtual address 0000000000800069
[ 31.572762] Mem abort info:
[ 31.573027] ESR =0x0000000096000021
[ 31.573402] EC =0x25: DABT (current EL), IL = 32 bits [ 31.573878] SET = 0, FnV = 0
[ 31.574157] EA = 0, S1PTW = 0
[ 31.574442] FSC =0x21: alignment fault
[ OK ] Listening on dbus.socket - D-Bus System Message Bus Socket. [ 31.593348] Data abort info:
[ 31.593628] ISV = 0, ISS =0x00000021, ISS2 =0x00000000[ 31.594117] CM = 0, WnR = 0, TnD = 0, TagAccess = 0
[ 31.594567] GCS = 0, Overlay = 0, DirtyBit = 0, Xs = 0 [ 31.595042] user pgtable: 4k pages, 48-bit VAs, pgdp=0000000105e50000
[ 31.595613] [0000000000800069] pgd=0000000000000000, p4d=0000000000000000
[ 31.596434] Internal error: Oops: 0000000096000021 [#1] SMP [ 31.596936] Modules linked in: snd tyr(+) soundcore sha256 cfg80211 rfkillpci_endpoint_testfusedm_modipv6
[ 31.597830] CPU: 3 UID: 0 PID: 241 Comm: (udev-worker) Not tainted 6.19.0-rc5 #282 PREEMPT
[ 31.598561] Hardware name: Radxa ROCK 5B (DT)
[ 31.598944] pstate: 00400009 (nzcv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[ 31.599554] pc :__d_lookup_rcu+0xbc/0x168
[ 31.599921] lr :__d_lookup_rcu+0x60/0x168
[ 31.600283] sp :ffff800081ebba00
[ 31.600574] x29:ffff800081ebba00x28: 0000000000000049 x27:ffff00010d07002b
[ 31.601205] x26: 0000000000000081 x25:000000000006915ex24:ffff0002f6600000
[ 31.601848] x23:ffff00010396a040x22:ffff000102098460x21:000000036915e207
[ 31.601859] x20:ffff000101e10af8x19:ffff800081ebbcacx18:2f64662f666c6573
[ 31.601867] x17:ffffff00666c6573x16:00000000fffffffcx15:
0000000000003431
[ 31.601875] x14:000000000000000fx13: 0080205100800107 x12:
0000000000800069
[ 31.601882] x11: ffffffffffffffff x10: 0000000000000018 x9 :
0000000000000003
[ 31.601890] x8 :00000000008000a1x7 :ffffb9d13df173c8x6 :
0000000000000000
[ 31.601897] x5 : 0000000000000000 x4 : 0000000000000010 x3 :ffffffffffff0a00
[ 31.601905] x2 :ffff800081ebbcacx1 :ffffb9d1406be718x0 :
0000000000000001
[ 31.601913] Call trace:
[ 31.601915]__d_lookup_rcu+0xbc/0x168(P)
[ 31.601922]lookup_fast+0x98/0x174
[ 31.601929]link_path_walk+0x280/0x528
[ 31.601935]path_lookupat+0x60/0x1f0
[ 31.601941]do_o_path+0x34/0xb4
[ 31.601947]path_openat+0xccc/0xe34
[ 31.601953]do_filp_open+0xc0/0x170
[ 31.601958]do_sys_openat2+0x88/0x10c
[ 31.601963]__arm64_sys_openat+0x70/0x9c
[ 31.601968]invoke_syscall+0x40/0xcc
[ 31.601974]el0_svc_common+0x80/0xd8
[ 31.601979]do_el0_svc+0x1c/0x28
[ 31.601984]el0_svc+0x54/0x1d4
[ 31.601991]el0t_64_sync_handler+0x84/0x12c
[ 31.601997]el0t_64_sync+0x198/0x19c
[ 31.602005] Code: 14000003f9400108b4000428d100e10c(88dffd8c) [ 31.602009] ---[ end trace 0000000000000000 ]---
[ OK ] Listening on sshd-unix-local.socke…temd-ssh-generator,AF_UNIXLocal).
[ 31.620067] mc: Linux media interface: v0.10
[ 31.623483] [drm] Initialized panthor 1.5.0 for fb000000.gpu on minor 0
[ 31.623765] Unable to handle kernel paging request at virtual address00802a4d0080284d
[ 31.624285] tyr fb000000.gpu: Tyr initialized correctly. [ 31.624752] Mem abort info:
[ 31.624754] ESR =0x0000000096000004
[ 31.625847] EC =0x25: DABT (current EL), IL = 32 bits [ 31.626310] SET = 0, FnV = 0
[ 31.626578] EA = 0, S1PTW = 0
[ 31.626853] FSC =0x04: level 0 translation fault
[ 31.627277] Data abort info:
[ 31.627529] ISV = 0, ISS =0x00000004, ISS2 =0x00000000[ 31.628006] CM = 0, WnR = 0, TnD = 0, TagAccess = 0
[ 31.628447] GCS = 0, Overlay = 0, DirtyBit = 0, Xs = 0 [ 31.628910] [00802a4d0080284d] address between user and kernel address ranges
[ 31.629535] Internal error: Oops: 0000000096000004 [#2] SMP [ 31.630030] Modules linked in: mcdrm_client_libsnd tyr soundcore sha256 cfg80211 rfkillpci_endpoint_testfusedm_modipv6 [ 31.631017] CPU: 4 UID: 0 PID: 225 Comm: systemd-udevd Tainted:
G D 6.19.0-rc5 #282 PREEMPT
[ 31.631877] Tainted: [D]=DIE
[ 31.632129] Hardware name: Radxa ROCK 5B (DT)
[ 31.632506] pstate:a0400009(NzCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[ 31.633111] pc :___d_drop+0xd8/0x144
[ 31.633433] lr :d_invalidate+0x3c/0x110
[ 31.633776] sp :ffff800081d3ba70
[ 31.634064] x29:ffff800081d3ba90x28: 0000000000000001 x27:ffffb9d140bfa000
[ 31.634685] x26:ffff0001039b5108x25:ffff00010396a000x24:ffffb9d140166275
[ 31.635305] x23:ffffb9d140bfa000x22:ffffb9d140152f3ax21:ffff000104076000
[ 31.635925] x20:ffff00010e520098x19:ffff00010396a000x18:
0000000000000000
[ 31.636545] x17: 0000000000000000 x16: 0000000000000000 x15:
0000000000000000
[ 31.637165] x14: 0000000000000000 x13:ffff00010792b8f0x12:000000000000017a
[ 31.637785] x11:00000000008000a1x10:00802a4d0080284dx9 :ffff0002f6604010
[ 31.638405] x8 :ffff000105bfcd40x7 : 0000000000000000 x6 :ffffb9d13e272c60
[ 31.639026] x5 : 0000000000000000 x4 : 0000000000000001 x3 :
0000000000000000
[ 31.639646] x2 : 0000000000000000 x1 : 0000000000000000 x0 :ffff00010396a000
[ 31.640266] Call trace:
[ 31.640479]___d_drop+0xd8/0x144(P)
[ 31.640800]d_invalidate+0x3c/0x110
[ 31.641113]proc_invalidate_siblings_dcache+0x244/0x2b8[ 31.641577]proc_flush_pid+0x1c/0x28
[ 31.641897]release_task+0x560/0x668
[ 31.642218]wait_consider_task+0x5a0/0xb44
[ 31.642582]__do_wait+0x154/0x1f0
[ 31.642879]do_wait+0x84/0x16c
[ 31.643154]__arm64_sys_waitid+0xac/0x218
[ 31.643512]invoke_syscall+0x40/0xcc
[ 31.643833]el0_svc_common+0x80/0xd8
[ 31.644152]do_el0_svc+0x1c/0x28
[ 31.644441]el0_svc+0x54/0x1d4
[ 31.644718]el0t_64_sync_handler+0x84/0x12c
[ 31.645090]el0t_64_sync+0x198/0x19c
[ 31.645412] Code:5280002af85f83a017fffff1a944280b(f940014c) [ 31.645941] ---[ end trace 0000000000000000 ]---thanks,
Deborah
PATCH v7 7/7 rust: drm/gem: Add vmap functions to shmem bindings
In reply to Lyude PaulOne of the more obvious use cases for gem shmem objects is the ability to create mappings into their contents, specifically iosys mappings. Now that we've added iosys_map rust bindings to the kernel, let's hook these up in gem shmem.
Similar to how we handle SGTables, we make sure there's two different types of mappings: owned mappings (kernel::drm::gem::shmem::VMap) and borrowed mappings (kernel::drm::gem::shmem::VMapRef).
One last note: we change the #[expect(unused)] for RawIoSysMap::from_raw() to an #[allow(unused)]. Normally we would simply remove the lint assertion, however - since shmem is conditionally built, we need allow to avoid hitting warnings in certain kernel configurations.
---
V7:
* Switch over to the new iosys map bindings that use the Io trait
rust/kernel/drm/gem/shmem.rs | 170 ++++++++++++++++++++++++++++++++++- rust/kernel/iosys_map.rs | 3 +- 2 files changed, 170 insertions(+), 3 deletions(-)
rust/kernel/drm/gem/shmem.rs +169 -1
@@ -21,6 +21,7 @@ from_err_ptr, to_result, // }, + iosys_map::*, prelude::*, scatterlist, types::{
@@ -29,13 +30,18 @@ }, // };use core::{ + mem::{ + self, +MaybeUninit ,// + }, ops::{Deref ,DerefMut ,// }, - ptr::NonNull , + ptr::NonNull ,// };use gem::{ +BaseObject ,BaseObjectPrivate ,DriverObject ,IntoGEMObject ,//
@@ -216,6 +222,72 @@ pub fn owned_sg_table(&self) -> Result<SGTable<T>> { _owner: self.into(), }) } + +/// Attempt to create a [`RawIoSysMap`] from the gem object. +fn raw_vmap <const SIZE :usize >(&self) ->Result <RawIoSysMap <SIZE >> { +let mut map:MaybeUninit <bindings::iosys_map > =MaybeUninit ::uninit (); + +// SAFETY: drm_gem_shmem_vmap can be called with the DMA reservation lock held +to_result (unsafe { +// TODO: see top of file + bindings::dma_resv_lock (self.raw_dma_resv (), core::ptr::null_mut ()); +let ret = bindings::drm_gem_shmem_vmap_locked (self.as_shmem (), map.as_mut_ptr ()); + bindings::dma_resv_unlock (self.raw_dma_resv ()); + ret + })?; + +// SAFETY: if drm_gem_shmem_vmap did not fail, map is initialized now +Ok (unsafe {RawIoSysMap ::<SIZE >::from_raw (map.assume_init ()) }) + } + +/// Unmap a [`RawIoSysMap`] from the gem object. +/// +/// # Safety +/// +/// - The caller promises that `map` came from a prior call to [`Self::raw_vmap`] on this gem +/// object. +/// - The caller promises that the memory pointed to by `map` will no longer be accesed through +/// this instance. +unsafe fn raw_vunmap <const SIZE :usize >(&self, map: &mut RawIoSysMap <SIZE >) { +let resv = self.raw_dma_resv (); + +// SAFETY: +// - This function is safe to call with the DMA reservation lock held +// - Our `ARef` is proof that the underlying gem object here is initialized and thus safe to +// dereference. +unsafe { +// TODO: see top of file + bindings::dma_resv_lock (resv, core::ptr::null_mut ()); + bindings::drm_gem_shmem_vunmap_locked (self.as_shmem (), map.as_raw_mut ()); + bindings::dma_resv_unlock (resv); + } + } + +/// Creates and returns a virtual kernel memory mapping for this object. +pub fn vmap <const SIZE :usize >(&self) ->Result <VMapRef <'_,T ,SIZE >> { +let map = self.raw_vmap ()?; + +Ok (VMapRef { +// SAFETY: +// - The size of the vmap is the same as the size of the gem +// - The vmap will remain alive until this object is dropped. + map:unsafe {IoSysMapRef ::<SIZE >::new (map, self.size ()) }?, + owner: self, + }) + } + +/// Creates and returns an owned reference to a virtual kernel memory mapping for this object. +pub fn owned_vmap <const SIZE :usize >(&self) ->Result <VMap <T ,SIZE >> { +// INVARIANT: We verify here that the mapping is at least of SIZE bytes. +if self.size () <SIZE { +return Err (EINVAL ); + } + +Ok (VMap { + map: self.raw_vmap ()?, + owner: self.into (), + }) + } }impl <T :DriverObject >Deref for Object <T > {
@@ -267,6 +339,102 @@ impl<T: DriverObject> driver::AllocImpl for Object<T> { }; } +/// A borrowed reference to a virtual mapping for a shmem-based GEM object in kernel address space. +pub struct VMapRef <'a,D :DriverObject ,const SIZE :usize =0 > { + map:IoSysMapRef <'a,SIZE >, + owner: &'aObject <D >, +} + +impl <'a,D :DriverObject ,const SIZE :usize >Clone for VMapRef <'a,D ,SIZE > { +fn clone (&self) ->Self { +// SAFETY: We have a successful vmap already, so this can't fail +unsafe { self.owner.vmap ().unwrap_unchecked () } + } +} + +impl <'a,D :DriverObject ,const SIZE :usize >Deref for VMapRef <'a,D ,SIZE > { +type Target =IoSysMapRef <'a,SIZE >; + +fn deref (&self) -> &Self ::Target { + &self.map + } +} + +impl <'a,D :DriverObject ,const SIZE :usize >DerefMut for VMapRef <'a,D ,SIZE > { +fn deref_mut (&mut self) -> &mut Self ::Target { + &mut self.map + } +} + +impl <'a,D :DriverObject ,const SIZE :usize >Drop for VMapRef <'a,D ,SIZE > { +fn drop (&mut self) { +// SAFETY: Our existence is proof that this map was previously created using self.owner. +unsafe { self.owner.raw_vunmap (&mut self.map.raw_map) }; + } +} + +/// An owned reference to a virtual mapping for a shmem-based GEM object in kernel address space. +/// +/// # Invariants +/// +/// - The size of `owner` is >= SIZE. +/// - The memory pointed to by `map` is at least as large as `T`. +/// - The memory pointed to by `map` remains valid at least until this object is dropped. +pub struct VMap <D :DriverObject ,const SIZE :usize =0 > { + map:RawIoSysMap <SIZE >, + owner:ARef <Object <D >>, +} + +impl <D :DriverObject ,const SIZE :usize >Clone for VMap <D ,SIZE > { +fn clone (&self) ->Self { +// SAFETY: We have a successful vmap already, so this can't fail +unsafe { self.owner.owned_vmap ().unwrap_unchecked () } + } +} + +impl <'a,D :DriverObject ,const SIZE :usize >From <VMapRef <'a,D ,SIZE >>for VMap <D ,SIZE > { +fn from (value:VMapRef <'a,D ,SIZE >) ->Self { +let this =Self { + map: value.map.raw_map.clone (), + owner: value.owner.into (), + }; + + mem::forget (value); + this + } +} + +impl <D :DriverObject ,const SIZE :usize >VMap <D ,SIZE > { +/// Return a reference to the iosys map for this `VMap`. +#[inline] +pub fn get (&self) ->IoSysMapRef <'_,SIZE > { +// SAFETY: +// - The size of the iosys_map is equivalent to the size of the gem object. +// - `size` is >= SIZE according to our type invariants, ensuring that we can never pass an +// invalid `size` to `IoSysMapRef::new(). +unsafe { +IoSysMapRef ::new (self.map.clone (), self.owner.size ()).unwrap_unchecked () + } + } + +/// Borrows a reference to the object that owns this virtual mapping. +pub fn owner (&self) -> &Object <D > { + &self.owner + } +} + +impl <D :DriverObject ,const SIZE :usize >Drop for VMap <D ,SIZE > { +fn drop (&mut self) { +// SAFETY: Our existence is proof that this map was previously created using self.owner +unsafe { self.owner.raw_vunmap (&mut self.map) }; + } +} + +/// SAFETY: `iosys_map` objects are safe to send across threads. +unsafe impl <D :DriverObject ,const SIZE :usize >Send for VMap <D ,SIZE > {} +/// SAFETY: `iosys_map` objects are safe to send across threads. +unsafe impl <D :DriverObject ,const SIZE :usize >Sync for VMap <D ,SIZE > {} +/// An owned reference to a scatter-gather table of DMA address spans for a GEM shmem object. /// /// This object holds an owned reference to the underlying GEM shmem object, ensuring that the
rust/kernel/iosys_map.rs +1 -2
@@ -33,7 +33,7 @@impl <const SIZE :usize >RawIoSysMap <SIZE > {/// Convert from a raw `bindings::iosys_map`. -#[expect(unused)] +#[allow(unused)] #[inline] pub (crate )fn from_raw (val: bindings::iosys_map ) ->Self {Self (val)
@@ -139,7 +139,6 @@ impl<'a, const SIZE: usize> IoSysMapRef<'a, SIZE> {/// /// - The caller guarantees that the mapping is of at least `size` bytes. /// - The caller guarantees that the mapping remains valid for the lifetime of `'a`. -#[expect(unused)] pub (crate )unsafe fn new (map:RawIoSysMap <SIZE >, size:usize ) ->Result <Self > {if size <SIZE {return Err (EINVAL );
Signature · 1 line
2.53.0