| Message ID | 20241120-vma-v8-7-eb31425da66b@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Rust support for mm_struct, vm_area_struct, and mmap | expand |
On Wed, Nov 20, 2024 at 02:50:01PM +0000, Alice Ryhl wrote: > Using the vma support introduced by the previous commit, introduce mmap > support for miscdevices. The mmap call is given a vma that is undergoing > initial setup, so the VmAreaNew type is used. Again, I'd be explicit about the VMA being passed to a struct file_operations->mmap() hook on mmap. Otherwise this seems super vague to me! > > Signed-off-by: Alice Ryhl <aliceryhl@google.com> > --- > rust/kernel/miscdevice.rs | 28 ++++++++++++++++++++++++++++ > 1 file changed, 28 insertions(+) > > diff --git a/rust/kernel/miscdevice.rs b/rust/kernel/miscdevice.rs > index 7e2a79b3ae26..4e4b9476e092 100644 > --- a/rust/kernel/miscdevice.rs > +++ b/rust/kernel/miscdevice.rs > @@ -11,6 +11,7 @@ > use crate::{ > bindings, > error::{to_result, Error, Result, VTABLE_DEFAULT_ERROR}, > + mm::virt::VmAreaNew, > prelude::*, > str::CStr, > types::{ForeignOwnable, Opaque}, > @@ -110,6 +111,11 @@ fn release(device: Self::Ptr) { > drop(device); > } > > + /// Handle for mmap. > + fn mmap(_device: <Self::Ptr as ForeignOwnable>::Borrowed<'_>, _vma: &VmAreaNew) -> Result { You will have to help me with this :) ForeignOwnable, Borrowed<'_>, _vma I'm a bit lost here! > + kernel::build_error(VTABLE_DEFAULT_ERROR) What is this? Is this not yet implemented or something, and this is a placeholder or something? > + } > + > /// Handler for ioctls. > /// > /// The `cmd` argument is usually manipulated using the utilties in [`kernel::ioctl`]. > @@ -156,6 +162,7 @@ impl<T: MiscDevice> VtableHelper<T> { > const VTABLE: bindings::file_operations = bindings::file_operations { > open: Some(fops_open::<T>), > release: Some(fops_release::<T>), > + mmap: maybe_fn(T::HAS_MMAP, fops_mmap::<T>), > unlocked_ioctl: maybe_fn(T::HAS_IOCTL, fops_ioctl::<T>), > #[cfg(CONFIG_COMPAT)] > compat_ioctl: if T::HAS_COMPAT_IOCTL { > @@ -216,6 +223,27 @@ impl<T: MiscDevice> VtableHelper<T> { > 0 > } > > +/// # Safety > +/// > +/// `file` must be a valid file that is associated with a `MiscDeviceRegistration<T>`. > +/// `vma` must be a vma that is currently being mmap'ed with this file. > +unsafe extern "C" fn fops_mmap<T: MiscDevice>( > + file: *mut bindings::file, > + vma: *mut bindings::vm_area_struct, > +) -> c_int { > + // SAFETY: The mmap call of a file can access the private data. > + let private = unsafe { (*file).private_data }; > + // SAFETY: Mmap calls can borrow the private data of the file. > + let device = unsafe { <T::Ptr as ForeignOwnable>::borrow(private) }; > + // SAFETY: The caller provides a vma that is undergoing initial VMA setup. > + let area = unsafe { VmAreaNew::from_raw(vma) }; > + > + match T::mmap(device, area) { > + Ok(()) => 0, > + Err(err) => err.to_errno() as c_int, > + } > +} > + > /// # Safety > /// > /// `file` must be a valid file that is associated with a `MiscDeviceRegistration<T>`. > > -- > 2.47.0.371.ga323438b13-goog >
On Wed, Nov 20, 2024 at 9:07 PM Lorenzo Stoakes <lorenzo.stoakes@oracle.com> wrote: > > On Wed, Nov 20, 2024 at 02:50:01PM +0000, Alice Ryhl wrote: > > Using the vma support introduced by the previous commit, introduce mmap > > support for miscdevices. The mmap call is given a vma that is undergoing > > initial setup, so the VmAreaNew type is used. > > Again, I'd be explicit about the VMA being passed to a struct > file_operations->mmap() hook on mmap. Otherwise this seems super vague to > me! I wasn't sure if vmas could be constructed in any context other than mmap. Will reword. > > Signed-off-by: Alice Ryhl <aliceryhl@google.com> > > --- > > rust/kernel/miscdevice.rs | 28 ++++++++++++++++++++++++++++ > > 1 file changed, 28 insertions(+) > > > > diff --git a/rust/kernel/miscdevice.rs b/rust/kernel/miscdevice.rs > > index 7e2a79b3ae26..4e4b9476e092 100644 > > --- a/rust/kernel/miscdevice.rs > > +++ b/rust/kernel/miscdevice.rs > > @@ -11,6 +11,7 @@ > > use crate::{ > > bindings, > > error::{to_result, Error, Result, VTABLE_DEFAULT_ERROR}, > > + mm::virt::VmAreaNew, > > prelude::*, > > str::CStr, > > types::{ForeignOwnable, Opaque}, > > @@ -110,6 +111,11 @@ fn release(device: Self::Ptr) { > > drop(device); > > } > > > > + /// Handle for mmap. > > + fn mmap(_device: <Self::Ptr as ForeignOwnable>::Borrowed<'_>, _vma: &VmAreaNew) -> Result { > > You will have to help me with this :) ForeignOwnable, Borrowed<'_>, _vma I'm a > bit lost here! When you implement the Miscdevice for your own type, you write a block like this one: impl Miscdevice for MyType { type Ptr = Arc<MyType>; ... } Here Arc is a pointer type (very similar to ARef) that represents ownership over a refcount to a refcounted value. In this case: * Self refers to MyType. * Self::Ptr refers to Arc<MyType> * <Self::Ptr as ForeignOwnable>::Borrowed<'_> refers to ArcBorrow<MyType> The last step is resolved to ArcBorrow<MyType> because of this impl block in rust/kernel/sync/arc.rs: impl<T: 'static> ForeignOwnable for Arc<T> { type Borrowed<'a> = ArcBorrow<'a, T>; ... } Note that Self::Ptr is short-hand for <Self as Miscdevice>::Ptr. > > + kernel::build_error(VTABLE_DEFAULT_ERROR) > > What is this? Is this not yet implemented or something, and this is a > placeholder or something? It's a build-time assertion that this function is dead-code eliminated. There are two cases: * Either the driver does not override mmap. In this case, we store a null pointer in the fops table. * Or the driver overrides mmap with their own implementation. In this case, we store a function pointer to whichever implementation they provided. In neither case is the above function present anywhere in the final executable. Alice
On Thu, Nov 21, 2024 at 11:08:05AM +0100, Alice Ryhl wrote: > On Wed, Nov 20, 2024 at 9:07 PM Lorenzo Stoakes > <lorenzo.stoakes@oracle.com> wrote: > > > > On Wed, Nov 20, 2024 at 02:50:01PM +0000, Alice Ryhl wrote: > > > Using the vma support introduced by the previous commit, introduce mmap > > > support for miscdevices. The mmap call is given a vma that is undergoing > > > initial setup, so the VmAreaNew type is used. > > > > Again, I'd be explicit about the VMA being passed to a struct > > file_operations->mmap() hook on mmap. Otherwise this seems super vague to > > me! > > I wasn't sure if vmas could be constructed in any context other than > mmap. Will reword. Thanks. > > > > Signed-off-by: Alice Ryhl <aliceryhl@google.com> _Definitely_ need the commit msg update above, but other than this from mm side looks reasonable, so: Acked-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com> (for mm bits) > > > --- > > > rust/kernel/miscdevice.rs | 28 ++++++++++++++++++++++++++++ > > > 1 file changed, 28 insertions(+) > > > > > > diff --git a/rust/kernel/miscdevice.rs b/rust/kernel/miscdevice.rs > > > index 7e2a79b3ae26..4e4b9476e092 100644 > > > --- a/rust/kernel/miscdevice.rs > > > +++ b/rust/kernel/miscdevice.rs > > > @@ -11,6 +11,7 @@ > > > use crate::{ > > > bindings, > > > error::{to_result, Error, Result, VTABLE_DEFAULT_ERROR}, > > > + mm::virt::VmAreaNew, > > > prelude::*, > > > str::CStr, > > > types::{ForeignOwnable, Opaque}, > > > @@ -110,6 +111,11 @@ fn release(device: Self::Ptr) { > > > drop(device); > > > } > > > > > > + /// Handle for mmap. > > > + fn mmap(_device: <Self::Ptr as ForeignOwnable>::Borrowed<'_>, _vma: &VmAreaNew) -> Result { > > > > You will have to help me with this :) ForeignOwnable, Borrowed<'_>, _vma I'm a > > bit lost here! > > When you implement the Miscdevice for your own type, you write a block > like this one: > > impl Miscdevice for MyType { > type Ptr = Arc<MyType>; > ... > } > > Here Arc is a pointer type (very similar to ARef) that represents > ownership over a refcount to a refcounted value. > > In this case: > * Self refers to MyType. > * Self::Ptr refers to Arc<MyType> > * <Self::Ptr as ForeignOwnable>::Borrowed<'_> refers to ArcBorrow<MyType> > > The last step is resolved to ArcBorrow<MyType> because of this impl > block in rust/kernel/sync/arc.rs: > > impl<T: 'static> ForeignOwnable for Arc<T> { > type Borrowed<'a> = ArcBorrow<'a, T>; > ... > } > > Note that Self::Ptr is short-hand for <Self as Miscdevice>::Ptr. Thanks! > > > > + kernel::build_error(VTABLE_DEFAULT_ERROR) > > > > What is this? Is this not yet implemented or something, and this is a > > placeholder or something? > > It's a build-time assertion that this function is dead-code eliminated. > > There are two cases: > * Either the driver does not override mmap. In this case, we store a > null pointer in the fops table. > * Or the driver overrides mmap with their own implementation. In this > case, we store a function pointer to whichever implementation they > provided. > In neither case is the above function present anywhere in the final executable. Ah nice, zero-cost stuff. > > Alice
diff --git a/rust/kernel/miscdevice.rs b/rust/kernel/miscdevice.rs index 7e2a79b3ae26..4e4b9476e092 100644 --- a/rust/kernel/miscdevice.rs +++ b/rust/kernel/miscdevice.rs @@ -11,6 +11,7 @@ use crate::{ bindings, error::{to_result, Error, Result, VTABLE_DEFAULT_ERROR}, + mm::virt::VmAreaNew, prelude::*, str::CStr, types::{ForeignOwnable, Opaque}, @@ -110,6 +111,11 @@ fn release(device: Self::Ptr) { drop(device); } + /// Handle for mmap. + fn mmap(_device: <Self::Ptr as ForeignOwnable>::Borrowed<'_>, _vma: &VmAreaNew) -> Result { + kernel::build_error(VTABLE_DEFAULT_ERROR) + } + /// Handler for ioctls. /// /// The `cmd` argument is usually manipulated using the utilties in [`kernel::ioctl`]. @@ -156,6 +162,7 @@ impl<T: MiscDevice> VtableHelper<T> { const VTABLE: bindings::file_operations = bindings::file_operations { open: Some(fops_open::<T>), release: Some(fops_release::<T>), + mmap: maybe_fn(T::HAS_MMAP, fops_mmap::<T>), unlocked_ioctl: maybe_fn(T::HAS_IOCTL, fops_ioctl::<T>), #[cfg(CONFIG_COMPAT)] compat_ioctl: if T::HAS_COMPAT_IOCTL { @@ -216,6 +223,27 @@ impl<T: MiscDevice> VtableHelper<T> { 0 } +/// # Safety +/// +/// `file` must be a valid file that is associated with a `MiscDeviceRegistration<T>`. +/// `vma` must be a vma that is currently being mmap'ed with this file. +unsafe extern "C" fn fops_mmap<T: MiscDevice>( + file: *mut bindings::file, + vma: *mut bindings::vm_area_struct, +) -> c_int { + // SAFETY: The mmap call of a file can access the private data. + let private = unsafe { (*file).private_data }; + // SAFETY: Mmap calls can borrow the private data of the file. + let device = unsafe { <T::Ptr as ForeignOwnable>::borrow(private) }; + // SAFETY: The caller provides a vma that is undergoing initial VMA setup. + let area = unsafe { VmAreaNew::from_raw(vma) }; + + match T::mmap(device, area) { + Ok(()) => 0, + Err(err) => err.to_errno() as c_int, + } +} + /// # Safety /// /// `file` must be a valid file that is associated with a `MiscDeviceRegistration<T>`.
Using the vma support introduced by the previous commit, introduce mmap support for miscdevices. The mmap call is given a vma that is undergoing initial setup, so the VmAreaNew type is used. Signed-off-by: Alice Ryhl <aliceryhl@google.com> --- rust/kernel/miscdevice.rs | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+)