public inbox for linux-api@vger.kernel.org
 help / color / mirror / Atom feed
* [RFC PATCH v3 0/6] Rust goldfish_address_space driver (ioctl-only subset)
       [not found] <cover.1775456181.git.wenzhaoliao@ruc.edu.cn>
@ 2026-04-06 16:51 ` Wenzhao Liao
  2026-04-06 16:51   ` [RFC PATCH v3 1/6] uapi: add goldfish_address_space userspace ABI header Wenzhao Liao
                     ` (5 more replies)
  0 siblings, 6 replies; 8+ messages in thread
From: Wenzhao Liao @ 2026-04-06 16:51 UTC (permalink / raw)
  To: rust-for-linux, linux-pci
  Cc: ojeda, dakr, bhelgaas, kwilczynski, arnd, gregkh, linux-kernel,
	linux-api

This respin narrows the Rust goldfish_address_space RFC to the
open/release/ioctl ABI subset. Userspace mmap and PING_WITH_DATA are
not part of this series.

I would like to send this as a small first upstream step for the Rust
driver, instead of asking reviewers to take the mmap/VMA lifecycle work
in the same round.

The goal of the respin is to keep only the pieces that are still
required by the current driver:
- the goldfish UAPI header and Rust bindings exposure,
- minimal page helpers for the ping page,
- a small SharedMemoryBar abstraction for shared BAR reservation,
  memremap() lifetime, and physical base discovery,
- hardened miscdevice registration/open boundaries,
- and the Rust goldfish_address_space driver itself.

Compared to the previous round, this drops the Rust VMA/BAR-to-VMA
mapping work from the series and rewrites the driver and miscdevice
pieces around the current teardown and publication model. The driver
remains #![forbid(unsafe_code)].

Feedback would be especially helpful on:
- whether the ioctl-only ABI subset is a reasonable first upstream step
  for goldfish_address_space;
- whether SharedMemoryBar is the right minimal Rust abstraction for
  shared-memory BAR reservation plus memremap() lifetime;
- whether the miscdevice hardening direction makes sense, especially the
  publication-safe open context and the THIS_MODULE-owned safe
  file_operations path.

Changes since v2:
- dropped the userspace mmap portion of the RFC and removed the unused
  Rust VMA/BAR-to-VMA mapping patch from the series;
- narrowed the goldfish Kconfig help text and driver description to the
  open/release/ioctl ABI subset;
- reworked miscdevice so safe open() only sees publication-safe state
  and safe drivers no longer have a raw file_operations escape hatch;
- reworked goldfish teardown around deregister() -> shutdown() ->
  disable_device(), with live-file revocation before PCI disable and
  explicit enable_device_mem() probe unwind;
- kept the in-tree Rust VMA helpers still used by binder out of this
  series, so the respin only carries code with a current caller.

Behavior exercised for the RFC-limited ABI subset:
- open / release
- allocate_block / deallocate_block
- ping
- claim_shared / unclaim_shared
- unknown ioctl
- reopen

No claim is made beyond that subset in this respin.

Build-tested:
- make LLVM=1 rust/kernel.o
- make LLVM=1 drivers/platform/goldfish/goldfish_address_space.o
- make LLVM=1 samples/rust/rust_misc_device.o

Wenzhao Liao (6):
  uapi: add goldfish_address_space userspace ABI header
  rust: bindings: expose goldfish address-space headers
  rust: page: add helpers for page-backed ping state
  rust: pci: add shared BAR memremap support
  rust: miscdevice: harden registration and safe file_operations
    invariants
  platform/goldfish: add Rust goldfish_address_space driver

 MAINTAINERS                                   |  10 +
 drivers/platform/goldfish/Kconfig             |  11 +
 drivers/platform/goldfish/Makefile            |   1 +
 .../goldfish/goldfish_address_space.rs        | 917 ++++++++++++++++++
 include/uapi/linux/goldfish_address_space.h   |  54 ++
 rust/bindings/bindings_helper.h               |   1 +
 rust/helpers/page.c                           |   5 +
 rust/kernel/miscdevice.rs                     | 409 +++++---
 rust/kernel/page.rs                           |  52 +-
 rust/kernel/pci.rs                            |   8 +
 rust/kernel/pci/id.rs                         |   2 +-
 rust/kernel/pci/io.rs                         | 112 ++-
 rust/uapi/uapi_helper.h                       |   1 +
 samples/rust/rust_misc_device.rs              |   9 +-
 14 files changed, 1453 insertions(+), 139 deletions(-)
 create mode 100644 drivers/platform/goldfish/goldfish_address_space.rs
 create mode 100644 include/uapi/linux/goldfish_address_space.h

-- 
2.34.1

^ permalink raw reply	[flat|nested] 8+ messages in thread
* [RFC PATCH v3 1/6] uapi: add goldfish_address_space userspace ABI header
  2026-04-06 16:51 ` [RFC PATCH v3 0/6] Rust goldfish_address_space driver (ioctl-only subset) Wenzhao Liao
@ 2026-04-06 16:51   ` Wenzhao Liao
  2026-04-13 16:28     ` Arnd Bergmann
  2026-04-06 16:51   ` [RFC PATCH v3 2/6] rust: bindings: expose goldfish address-space headers Wenzhao Liao
                     ` (4 subsequent siblings)
  5 siblings, 1 reply; 8+ messages in thread
From: Wenzhao Liao @ 2026-04-06 16:51 UTC (permalink / raw)
  To: rust-for-linux, linux-pci
  Cc: ojeda, dakr, bhelgaas, kwilczynski, arnd, gregkh, linux-kernel,
	linux-api

The external goldfish address-space driver exposes its userspace
contract through a dedicated header. Land the ioctl definitions in
include/uapi/linux so the Rust driver can depend on an in-tree UAPI
surface instead of carrying an external private header.

This RFC intentionally narrows the first upstream step to the
open/release/ioctl ABI subset. Userspace mmap and PING_WITH_DATA stay
out of this series until they have their own review and validation
story.

Signed-off-by: Wenzhao Liao <wenzhaoliao@ruc.edu.cn>
---
 MAINTAINERS                                 |  8 +++
 include/uapi/linux/goldfish_address_space.h | 54 +++++++++++++++++++++
 2 files changed, 62 insertions(+)
 create mode 100644 include/uapi/linux/goldfish_address_space.h

diff --git a/MAINTAINERS b/MAINTAINERS
index a62f6af55c3a..800b2fe0e648 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1882,6 +1882,14 @@ S:	Supported
 F:	Documentation/devicetree/bindings/interrupt-controller/google,goldfish-pic.yaml
 F:	drivers/irqchip/irq-goldfish-pic.c
 
+ANDROID GOLDFISH ADDRESS SPACE DRIVER
+M:	Wenzhao Liao <wenzhaoliao@ruc.edu.cn>
+L:	linux-kernel@vger.kernel.org
+L:	linux-pci@vger.kernel.org
+L:	rust-for-linux@vger.kernel.org
+S:	Maintained
+F:	include/uapi/linux/goldfish_address_space.h
+
 ANDROID GOLDFISH RTC DRIVER
 M:	Jiaxun Yang <jiaxun.yang@flygoat.com>
 S:	Supported
diff --git a/include/uapi/linux/goldfish_address_space.h b/include/uapi/linux/goldfish_address_space.h
new file mode 100644
index 000000000000..b782d82f53df
--- /dev/null
+++ b/include/uapi/linux/goldfish_address_space.h
@@ -0,0 +1,54 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _UAPI_LINUX_GOLDFISH_ADDRESS_SPACE_H
+#define _UAPI_LINUX_GOLDFISH_ADDRESS_SPACE_H
+
+#include <linux/ioctl.h>
+#include <linux/types.h>
+
+#define GOLDFISH_ADDRESS_SPACE_DEVICE_NAME "goldfish_address_space"
+
+struct goldfish_address_space_allocate_block {
+	__u64 size;
+	__u64 offset;
+	__u64 phys_addr;
+};
+
+struct goldfish_address_space_ping {
+	__u64 offset;
+	__u64 size;
+	__u64 metadata;
+	__u32 version;
+	__u32 wait_fd;
+	__u32 wait_flags;
+	__u32 direction;
+};
+
+struct goldfish_address_space_claim_shared {
+	__u64 offset;
+	__u64 size;
+};
+
+#define GOLDFISH_ADDRESS_SPACE_IOCTL_MAGIC 'G'
+
+#define GOLDFISH_ADDRESS_SPACE_IOCTL_OP(OP, T) \
+	_IOWR(GOLDFISH_ADDRESS_SPACE_IOCTL_MAGIC, OP, T)
+
+#define GOLDFISH_ADDRESS_SPACE_IOCTL_ALLOCATE_BLOCK \
+	GOLDFISH_ADDRESS_SPACE_IOCTL_OP(10, \
+		struct goldfish_address_space_allocate_block)
+
+#define GOLDFISH_ADDRESS_SPACE_IOCTL_DEALLOCATE_BLOCK \
+	GOLDFISH_ADDRESS_SPACE_IOCTL_OP(11, __u64)
+
+#define GOLDFISH_ADDRESS_SPACE_IOCTL_PING \
+	GOLDFISH_ADDRESS_SPACE_IOCTL_OP(12, \
+		struct goldfish_address_space_ping)
+
+#define GOLDFISH_ADDRESS_SPACE_IOCTL_CLAIM_SHARED \
+	GOLDFISH_ADDRESS_SPACE_IOCTL_OP(13, \
+		struct goldfish_address_space_claim_shared)
+
+#define GOLDFISH_ADDRESS_SPACE_IOCTL_UNCLAIM_SHARED \
+	GOLDFISH_ADDRESS_SPACE_IOCTL_OP(14, __u64)
+
+#endif /* _UAPI_LINUX_GOLDFISH_ADDRESS_SPACE_H */
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 8+ messages in thread
* [RFC PATCH v3 2/6] rust: bindings: expose goldfish address-space headers
  2026-04-06 16:51 ` [RFC PATCH v3 0/6] Rust goldfish_address_space driver (ioctl-only subset) Wenzhao Liao
  2026-04-06 16:51   ` [RFC PATCH v3 1/6] uapi: add goldfish_address_space userspace ABI header Wenzhao Liao
@ 2026-04-06 16:51   ` Wenzhao Liao
  2026-04-06 16:51   ` [RFC PATCH v3 3/6] rust: page: add helpers for page-backed ping state Wenzhao Liao
                     ` (3 subsequent siblings)
  5 siblings, 0 replies; 8+ messages in thread
From: Wenzhao Liao @ 2026-04-06 16:51 UTC (permalink / raw)
  To: rust-for-linux, linux-pci
  Cc: ojeda, dakr, bhelgaas, kwilczynski, arnd, gregkh, linux-kernel,
	linux-api

Expose the UAPI header and the Linux I/O declarations needed by the Rust goldfish address-space driver.

This keeps the driver-side code on typed Rust interfaces while still allowing the binding and helper layers to see the header and memremap support required by the abstraction patches that follow.

Signed-off-by: Wenzhao Liao <wenzhaoliao@ruc.edu.cn>
---
 rust/bindings/bindings_helper.h | 1 +
 rust/uapi/uapi_helper.h         | 1 +
 2 files changed, 2 insertions(+)

diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index 083cc44aa952..b0baff4c6349 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -59,6 +59,7 @@
 #include <linux/fs.h>
 #include <linux/i2c.h>
 #include <linux/interrupt.h>
+#include <linux/io.h>
 #include <linux/io-pgtable.h>
 #include <linux/ioport.h>
 #include <linux/jiffies.h>
diff --git a/rust/uapi/uapi_helper.h b/rust/uapi/uapi_helper.h
index 06d7d1a2e8da..ff19edab81da 100644
--- a/rust/uapi/uapi_helper.h
+++ b/rust/uapi/uapi_helper.h
@@ -11,6 +11,7 @@
 #include <uapi/drm/nova_drm.h>
 #include <uapi/drm/panthor_drm.h>
 #include <uapi/linux/android/binder.h>
+#include <uapi/linux/goldfish_address_space.h>
 #include <uapi/linux/mdio.h>
 #include <uapi/linux/mii.h>
 #include <uapi/linux/ethtool.h>
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 8+ messages in thread
* [RFC PATCH v3 3/6] rust: page: add helpers for page-backed ping state
  2026-04-06 16:51 ` [RFC PATCH v3 0/6] Rust goldfish_address_space driver (ioctl-only subset) Wenzhao Liao
  2026-04-06 16:51   ` [RFC PATCH v3 1/6] uapi: add goldfish_address_space userspace ABI header Wenzhao Liao
  2026-04-06 16:51   ` [RFC PATCH v3 2/6] rust: bindings: expose goldfish address-space headers Wenzhao Liao
@ 2026-04-06 16:51   ` Wenzhao Liao
  2026-04-06 16:51   ` [RFC PATCH v3 4/6] rust: pci: add shared BAR memremap support Wenzhao Liao
                     ` (2 subsequent siblings)
  5 siblings, 0 replies; 8+ messages in thread
From: Wenzhao Liao @ 2026-04-06 16:51 UTC (permalink / raw)
  To: rust-for-linux, linux-pci
  Cc: ojeda, dakr, bhelgaas, kwilczynski, arnd, gregkh, linux-kernel,
	linux-api

Add the minimal safe page helpers needed by the goldfish ping buffer: physical address discovery plus bounded read, write, and zeroing operations.

The driver uses these helpers to manage its per-file ping page entirely from safe Rust while keeping the raw page mapping and pointer handling inside the page abstraction.

Signed-off-by: Wenzhao Liao <wenzhaoliao@ruc.edu.cn>
---
 rust/helpers/page.c |  5 +++++
 rust/kernel/page.rs | 52 +++++++++++++++++++++++----------------------
 2 files changed, 32 insertions(+), 25 deletions(-)

diff --git a/rust/helpers/page.c b/rust/helpers/page.c
index f8463fbed2a2..05824bdc4fd8 100644
--- a/rust/helpers/page.c
+++ b/rust/helpers/page.c
@@ -20,6 +20,11 @@ __rust_helper void rust_helper_kunmap_local(const void *addr)
 	kunmap_local(addr);
 }
 
+__rust_helper phys_addr_t rust_helper_page_to_phys(struct page *page)
+{
+	return page_to_phys(page);
+}
+
 #ifndef NODE_NOT_IN_PAGE_FLAGS
 __rust_helper int rust_helper_page_to_nid(const struct page *page)
 {
diff --git a/rust/kernel/page.rs b/rust/kernel/page.rs
index adecb200c654..e8336d1bcc12 100644
--- a/rust/kernel/page.rs
+++ b/rust/kernel/page.rs
@@ -7,7 +7,7 @@
     bindings,
     error::code::*,
     error::Result,
-    uaccess::UserSliceReader,
+    io::PhysAddr,
 };
 use core::{
     marker::PhantomData,
@@ -198,6 +198,13 @@ pub fn nid(&self) -> i32 {
         unsafe { bindings::page_to_nid(self.as_ptr()) }
     }
 
+    /// Returns the physical address of the start of this page.
+    #[inline]
+    pub fn phys_addr(&self) -> PhysAddr {
+        // SAFETY: `self.as_ptr()` is a live `struct page` owned by this `Page`.
+        unsafe { bindings::page_to_phys(self.as_ptr()) }
+    }
+
     /// Runs a piece of code with this page mapped to an address.
     ///
     /// The page is unmapped when this call returns.
@@ -337,30 +344,25 @@ pub unsafe fn fill_zero_raw(&self, offset: usize, len: usize) -> Result {
         })
     }
 
-    /// Copies data from userspace into this page.
-    ///
-    /// This method will perform bounds checks on the page offset. If `offset .. offset+len` goes
-    /// outside of the page, then this call returns [`EINVAL`].
-    ///
-    /// Like the other `UserSliceReader` methods, data races are allowed on the userspace address.
-    /// However, they are not allowed on the page you are copying into.
-    ///
-    /// # Safety
-    ///
-    /// Callers must ensure that this call does not race with a read or write to the same page that
-    /// overlaps with this write.
-    pub unsafe fn copy_from_user_slice_raw(
-        &self,
-        reader: &mut UserSliceReader,
-        offset: usize,
-        len: usize,
-    ) -> Result {
-        self.with_pointer_into_page(offset, len, move |dst| {
-            // SAFETY: If `with_pointer_into_page` calls into this closure, then it has performed a
-            // bounds check and guarantees that `dst` is valid for `len` bytes. Furthermore, we have
-            // exclusive access to the slice since the caller guarantees that there are no races.
-            reader.read_raw(unsafe { core::slice::from_raw_parts_mut(dst.cast(), len) })
-        })
+    /// Maps the page and reads from it into the given buffer.
+    pub fn read_slice(&self, dst: &mut [u8], offset: usize) -> Result {
+        // SAFETY: `dst` is a valid mutable slice for `dst.len()` bytes. Safe Rust also prevents
+        // callers from obtaining a mutable reference to this `Page` while this shared borrow
+        // exists, so concurrent writes through the safe API cannot overlap with this read.
+        unsafe { self.read_raw(dst.as_mut_ptr(), offset, dst.len()) }
+    }
+
+    /// Maps the page and writes the given buffer into it.
+    pub fn write_slice(&mut self, src: &[u8], offset: usize) -> Result {
+        // SAFETY: `src` is a valid immutable slice for `src.len()` bytes, and `&mut self`
+        // guarantees unique access to the page through the safe API.
+        unsafe { self.write_raw(src.as_ptr(), offset, src.len()) }
+    }
+
+    /// Zeroes a range within the page.
+    pub fn fill_zero(&mut self, offset: usize, len: usize) -> Result {
+        // SAFETY: `&mut self` guarantees unique access to the page through the safe API.
+        unsafe { self.fill_zero_raw(offset, len) }
     }
 }
 
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 8+ messages in thread
* [RFC PATCH v3 4/6] rust: pci: add shared BAR memremap support
  2026-04-06 16:51 ` [RFC PATCH v3 0/6] Rust goldfish_address_space driver (ioctl-only subset) Wenzhao Liao
                     ` (2 preceding siblings ...)
  2026-04-06 16:51   ` [RFC PATCH v3 3/6] rust: page: add helpers for page-backed ping state Wenzhao Liao
@ 2026-04-06 16:51   ` Wenzhao Liao
  2026-04-06 16:51   ` [RFC PATCH v3 5/6] rust: miscdevice: harden registration and safe file_operations invariants Wenzhao Liao
  2026-04-06 16:51   ` [RFC PATCH v3 6/6] platform/goldfish: add Rust goldfish_address_space driver Wenzhao Liao
  5 siblings, 0 replies; 8+ messages in thread
From: Wenzhao Liao @ 2026-04-06 16:51 UTC (permalink / raw)
  To: rust-for-linux, linux-pci
  Cc: ojeda, dakr, bhelgaas, kwilczynski, arnd, gregkh, linux-kernel,
	linux-api

Add a small Rust-owned abstraction for PCI BARs that back shared memory
instead of register MMIO.

The new SharedMemoryBar type owns both the BAR reservation and the
memremap() lifetime, exposes the physical BAR start needed by the
address-space ping path, and keeps the resource bookkeeping out of the
Rust driver.

The current RFC no longer exposes userspace mmap, but the driver still
needs an owned shared-BAR reservation and the BAR's physical base for
the ping path. Keeping the reservation/memremap() pairing in a Rust
abstraction avoids pushing that lifetime bookkeeping back into driver
code.

Signed-off-by: Wenzhao Liao <wenzhaoliao@ruc.edu.cn>
---
 rust/kernel/pci.rs    |   8 +++
 rust/kernel/pci/id.rs |   2 +-
 rust/kernel/pci/io.rs | 112 +++++++++++++++++++++++++++++++++++++++++-
 3 files changed, 120 insertions(+), 2 deletions(-)

diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
index af74ddff6114..4c63c931ffb2 100644
--- a/rust/kernel/pci.rs
+++ b/rust/kernel/pci.rs
@@ -47,6 +47,7 @@
     ConfigSpaceSize,
     Extended,
     Normal, //
+    SharedMemoryBar,
 };
 pub use self::irq::{
     IrqType,
@@ -458,6 +459,13 @@ pub fn set_master(&self) {
         // SAFETY: `self.as_raw` is guaranteed to be a pointer to a valid `struct pci_dev`.
         unsafe { bindings::pci_set_master(self.as_raw()) };
     }
+
+    /// Disable this PCI device.
+    #[inline]
+    pub fn disable_device(&self) {
+        // SAFETY: `self.as_raw` is guaranteed to be a pointer to a valid `struct pci_dev`.
+        unsafe { bindings::pci_disable_device(self.as_raw()) };
+    }
 }
 
 // SAFETY: `pci::Device` is a transparent wrapper of `struct pci_dev`.
diff --git a/rust/kernel/pci/id.rs b/rust/kernel/pci/id.rs
index 50005d176561..bd3cf17fd8de 100644
--- a/rust/kernel/pci/id.rs
+++ b/rust/kernel/pci/id.rs
@@ -156,7 +156,7 @@ fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
 impl Vendor {
     /// Create a Vendor from a raw 16-bit vendor ID.
     #[inline]
-    pub(super) fn from_raw(vendor_id: u16) -> Self {
+    pub const fn from_raw(vendor_id: u16) -> Self {
         Self(vendor_id)
     }
 
diff --git a/rust/kernel/pci/io.rs b/rust/kernel/pci/io.rs
index fb6edab2aea7..89bf882b9634 100644
--- a/rust/kernel/pci/io.rs
+++ b/rust/kernel/pci/io.rs
@@ -7,6 +7,7 @@
     bindings,
     device,
     devres::Devres,
+    ffi::{c_ulong, c_void},
     io::{
         io_define_read,
         io_define_write,
@@ -17,11 +18,13 @@
         MmioRaw, //
     },
     prelude::*,
-    sync::aref::ARef, //
+    sync::aref::ARef,
+    types::ScopeGuard,
 };
 use core::{
     marker::PhantomData,
     ops::Deref, //
+    ptr::NonNull,
 };
 
 /// Represents the size of a PCI configuration space.
@@ -285,6 +288,104 @@ fn deref(&self) -> &Self::Target {
     }
 }
 
+/// A cacheable shared-memory mapping of a PCI BAR created via `memremap()`.
+///
+/// This is intended for BARs that back shared memory rather than device register MMIO. The
+/// mapping owns both the underlying PCI region reservation and the `memremap()` lifetime, so
+/// driver code does not need to keep raw pointers or manually pair teardown calls.
+pub struct SharedMemoryBar {
+    pdev: ARef<Device>,
+    addr: NonNull<c_void>,
+    phys_start: bindings::resource_size_t,
+    len: usize,
+    num: i32,
+}
+
+// SAFETY: `SharedMemoryBar` owns a stable BAR reservation plus its `memremap()` mapping. Moving
+// the owner to another thread does not change the validity of the underlying PCI resource.
+unsafe impl Send for SharedMemoryBar {}
+
+// SAFETY: Shared references only expose immutable metadata queries; the mapped pointer itself is
+// not exposed for dereferencing.
+unsafe impl Sync for SharedMemoryBar {}
+
+impl SharedMemoryBar {
+    fn new(pdev: &Device, num: u32, name: &CStr) -> Result<Self> {
+        if !Bar::index_is_valid(num) {
+            return Err(EINVAL);
+        }
+
+        let len = pdev.resource_len(num)?;
+        if len == 0 {
+            return Err(ENXIO);
+        }
+
+        let len = usize::try_from(len)?;
+        let phys_start = pdev.resource_start(num)?;
+        let num = i32::try_from(num)?;
+
+        // SAFETY:
+        // - `pdev` is valid by the invariants of `Device`.
+        // - `num` is checked above.
+        // - `name` is a valid NUL-terminated string.
+        let ret = unsafe { bindings::pci_request_region(pdev.as_raw(), num, name.as_char_ptr()) };
+        if ret != 0 {
+            return Err(EBUSY);
+        }
+
+        let release_region = ScopeGuard::new(|| {
+            // SAFETY:
+            // - `pdev` is still valid for the duration of this constructor.
+            // - `num` has just been successfully reserved.
+            unsafe { bindings::pci_release_region(pdev.as_raw(), num) };
+        });
+
+        // SAFETY:
+        // - `phys_start`/`len` describe the BAR range we just reserved.
+        // - `MEMREMAP_WB` matches the external goldfish driver behaviour.
+        let addr = unsafe { bindings::memremap(phys_start, len, bindings::MEMREMAP_WB as c_ulong) };
+        let addr = NonNull::new(addr.cast()).ok_or(ENOMEM)?;
+
+        release_region.dismiss();
+
+        Ok(Self {
+            pdev: pdev.into(),
+            addr,
+            phys_start,
+            len,
+            num,
+        })
+    }
+
+    /// Returns the physical start address of the BAR.
+    #[inline]
+    pub fn phys_start(&self) -> bindings::resource_size_t {
+        self.phys_start
+    }
+
+    /// Returns the BAR size in bytes.
+    #[inline]
+    pub fn len(&self) -> usize {
+        self.len
+    }
+
+    fn release(&self) {
+        // SAFETY:
+        // - `self.addr` is a valid `memremap()` result owned by `self`.
+        // - `self.num` is the BAR region successfully reserved by `Self::new`.
+        unsafe {
+            bindings::memunmap(self.addr.as_ptr().cast());
+            bindings::pci_release_region(self.pdev.as_raw(), self.num);
+        }
+    }
+}
+
+impl Drop for SharedMemoryBar {
+    fn drop(&mut self) {
+        self.release();
+    }
+}
+
 impl Device<device::Bound> {
     /// Maps an entire PCI BAR after performing a region-request on it. I/O operation bound checks
     /// can be performed on compile time for offsets (plus the requested type size) < SIZE.
@@ -305,6 +406,15 @@ pub fn iomap_region<'a>(
         self.iomap_region_sized::<0>(bar, name)
     }
 
+    /// Reserve and `memremap()` an entire PCI BAR as cacheable shared memory.
+    pub fn memremap_bar<'a>(
+        &'a self,
+        bar: u32,
+        name: &'a CStr,
+    ) -> impl PinInit<Devres<SharedMemoryBar>, Error> + 'a {
+        Devres::new(self.as_ref(), SharedMemoryBar::new(self, bar, name))
+    }
+
     /// Returns the size of configuration space.
     pub fn cfg_size(&self) -> ConfigSpaceSize {
         // SAFETY: `self.as_raw` is a valid pointer to a `struct pci_dev`.
-- 
2.34.1

^ permalink raw reply related	[flat|nested] 8+ messages in thread
* [RFC PATCH v3 5/6] rust: miscdevice: harden registration and safe file_operations invariants
  2026-04-06 16:51 ` [RFC PATCH v3 0/6] Rust goldfish_address_space driver (ioctl-only subset) Wenzhao Liao
                     ` (3 preceding siblings ...)
  2026-04-06 16:51   ` [RFC PATCH v3 4/6] rust: pci: add shared BAR memremap support Wenzhao Liao
@ 2026-04-06 16:51   ` Wenzhao Liao
  2026-04-06 16:51   ` [RFC PATCH v3 6/6] platform/goldfish: add Rust goldfish_address_space driver Wenzhao Liao
  5 siblings, 0 replies; 8+ messages in thread
From: Wenzhao Liao @ 2026-04-06 16:51 UTC (permalink / raw)
  To: rust-for-linux, linux-pci
  Cc: ojeda, dakr, bhelgaas, kwilczynski, arnd, gregkh, linux-kernel,
	linux-api

Extend miscdevice registration with typed per-device data that open()
can read through a publication-safe context, and move raw
file_operations exposure behind an internal vtable boundary generated by
declare_misc_device_fops!().

This keeps safe open() on pre-publication state, binds
file_operations.owner to THIS_MODULE for safe drivers, and keeps the
private_data ownership protocol inside the abstraction instead of in
driver code. The goldfish driver uses the typed registration data to
pass its runtime into open() without raw casts or container traversal.

Signed-off-by: Wenzhao Liao <wenzhaoliao@ruc.edu.cn>
---
 rust/kernel/miscdevice.rs        | 409 +++++++++++++++++++++++--------
 samples/rust/rust_misc_device.rs |   9 +-
 2 files changed, 306 insertions(+), 112 deletions(-)

diff --git a/rust/kernel/miscdevice.rs b/rust/kernel/miscdevice.rs
index c3c2052c9206..c2db81cd5da2 100644
--- a/rust/kernel/miscdevice.rs
+++ b/rust/kernel/miscdevice.rs
@@ -9,7 +9,8 @@
 //! Reference: <https://www.kernel.org/doc/html/latest/driver-api/misc_devices.html>
 
 use crate::{
-    bindings,
+    alloc::KBox,
+    bindings, container_of,
     device::Device,
     error::{to_result, Error, Result, VTABLE_DEFAULT_ERROR},
     ffi::{c_int, c_long, c_uint, c_ulong},
@@ -18,9 +19,15 @@
     mm::virt::VmaNew,
     prelude::*,
     seq_file::SeqFile,
-    types::{ForeignOwnable, Opaque},
+    sync::aref::ARef,
+    types::{ForeignOwnable, Opaque, ScopeGuard},
+};
+use core::{
+    marker::{PhantomData, PhantomPinned},
+    pin::Pin,
+    ptr::drop_in_place,
+    sync::atomic::{AtomicBool, Ordering},
 };
-use core::{marker::PhantomData, pin::Pin};
 
 /// Options for creating a misc device.
 #[derive(Copy, Clone)]
@@ -31,94 +38,258 @@ pub struct MiscDeviceOptions {
 
 impl MiscDeviceOptions {
     /// Create a raw `struct miscdev` ready for registration.
-    pub const fn into_raw<T: MiscDevice>(self) -> bindings::miscdevice {
+    pub fn into_raw<T: MiscDeviceVTable + 'static>(self) -> bindings::miscdevice {
         let mut result: bindings::miscdevice = pin_init::zeroed();
         result.minor = bindings::MISC_DYNAMIC_MINOR as ffi::c_int;
         result.name = crate::str::as_char_ptr_in_const_context(self.name);
-        result.fops = MiscdeviceVTable::<T>::build();
+        result.fops = T::file_operations();
         result
     }
 }
 
+/// Generates the `MiscDeviceVTable` implementation for a concrete miscdevice type.
+///
+/// Place this macro after `impl MiscDevice for ...`.
+///
+/// The generated implementation always binds `file_operations.owner` to the current module's
+/// `THIS_MODULE`, so safe drivers cannot accidentally publish owner-less or foreign-owned misc
+/// device callbacks.
+#[macro_export]
+macro_rules! declare_misc_device_fops {
+    ($type:ty) => {
+        // SAFETY: This implements the standard Rust miscdevice vtable generated by
+        // `build_file_operations()`, which wires up owner/module pinning and the private-data
+        // protocol enforced by this abstraction.
+        unsafe impl $crate::miscdevice::MiscDeviceVTable for $type {
+            fn file_operations() -> &'static $crate::bindings::file_operations {
+                struct AssertSync<T>(T);
+                // SAFETY: This wrapper is only used for immutable `file_operations` tables stored
+                // in a `static`.
+                unsafe impl<T> Sync for AssertSync<T> {}
+
+                static FOPS: AssertSync<$crate::bindings::file_operations> = AssertSync(
+                    $crate::miscdevice::build_file_operations::<$type>(THIS_MODULE.as_ptr()),
+                );
+
+                &FOPS.0
+            }
+        }
+    };
+}
+
+#[repr(C)]
+struct RegistrationBacking<T: MiscDevice + 'static> {
+    misc: Opaque<bindings::miscdevice>,
+    data: T::RegistrationData,
+    owner: *const MiscDeviceRegistration<T>,
+    registered: AtomicBool,
+}
+
+struct OpenFile<T: MiscDevice + 'static> {
+    data: *mut ffi::c_void,
+    _t: PhantomData<T>,
+}
+
+impl<T: MiscDevice + 'static> OpenFile<T> {
+    fn empty() -> Self {
+        Self {
+            data: core::ptr::null_mut(),
+            _t: PhantomData,
+        }
+    }
+
+    fn borrow(&self) -> <T::Ptr as ForeignOwnable>::Borrowed<'_> {
+        // SAFETY: `self.data` comes from `T::Ptr::into_foreign()` and is only converted back in
+        // `release`, after all borrows from this file operation callback have ended.
+        unsafe { <T::Ptr as ForeignOwnable>::borrow(self.data) }
+    }
+}
+
 /// A registration of a miscdevice.
 ///
 /// # Invariants
 ///
-/// - `inner` contains a `struct miscdevice` that is registered using
-///   `misc_register()`.
-/// - This registration remains valid for the entire lifetime of the
-///   [`MiscDeviceRegistration`] instance.
-/// - Deregistration occurs exactly once in [`Drop`] via `misc_deregister()`.
-/// - `inner` wraps a valid, pinned `miscdevice` created using
+/// - `backing.misc` contains a valid `struct miscdevice` created using
 ///   [`MiscDeviceOptions::into_raw`].
-#[repr(transparent)]
+/// - When `backing.registered` is `true`, `backing.misc` is registered using
+///   `misc_register()`.
+/// - Before `misc_register()` publishes `backing.misc`, every field reachable through the safe
+///   open context (`backing.data` and `backing.owner`) is fully initialized.
+/// - `backing.owner` points back to this wrapper for the entire time the miscdevice is registered.
+/// - Deregistration occurs at most once, either via [`MiscDeviceRegistration::deregister`] or
+///   [`Drop`].
 #[pin_data(PinnedDrop)]
-pub struct MiscDeviceRegistration<T> {
+pub struct MiscDeviceRegistration<T: MiscDevice + 'static> {
+    backing: KBox<RegistrationBacking<T>>,
     #[pin]
-    inner: Opaque<bindings::miscdevice>,
+    _pin: PhantomPinned,
     _t: PhantomData<T>,
 }
 
 // SAFETY: It is allowed to call `misc_deregister` on a different thread from where you called
 // `misc_register`.
-unsafe impl<T> Send for MiscDeviceRegistration<T> {}
+unsafe impl<T: MiscDevice> Send for MiscDeviceRegistration<T> {}
 // SAFETY: All `&self` methods on this type are written to ensure that it is safe to call them in
 // parallel.
-unsafe impl<T> Sync for MiscDeviceRegistration<T> {}
+unsafe impl<T: MiscDevice> Sync for MiscDeviceRegistration<T> {}
 
-impl<T: MiscDevice> MiscDeviceRegistration<T> {
+impl<T: MiscDevice + 'static> MiscDeviceRegistration<T> {
     /// Register a misc device.
-    pub fn register(opts: MiscDeviceOptions) -> impl PinInit<Self, Error> {
-        try_pin_init!(Self {
-            inner <- Opaque::try_ffi_init(move |slot: *mut bindings::miscdevice| {
-                // SAFETY: The initializer can write to the provided `slot`.
-                unsafe { slot.write(opts.into_raw::<T>()) };
-
-                // SAFETY: We just wrote the misc device options to the slot. The miscdevice will
-                // get unregistered before `slot` is deallocated because the memory is pinned and
-                // the destructor of this type deallocates the memory.
-                // INVARIANT: If this returns `Ok(())`, then the `slot` will contain a registered
-                // misc device.
-                to_result(unsafe { bindings::misc_register(slot) })
-            }),
-            _t: PhantomData,
-        })
+    pub fn register(opts: MiscDeviceOptions) -> impl PinInit<Self, Error>
+    where
+        T: MiscDevice<RegistrationData = ()> + MiscDeviceVTable,
+    {
+        Self::register_with_data(opts, ())
+    }
+
+    /// Register a misc device together with driver-defined registration data.
+    pub fn register_with_data(
+        opts: MiscDeviceOptions,
+        data: T::RegistrationData,
+    ) -> impl PinInit<Self, Error>
+    where
+        T: MiscDeviceVTable,
+    {
+        let init = move |slot: *mut Self| {
+            let backing = KBox::new(
+                RegistrationBacking {
+                    misc: Opaque::new(opts.into_raw::<T>()),
+                    data,
+                    owner: slot.cast_const(),
+                    registered: AtomicBool::new(false),
+                },
+                GFP_KERNEL,
+            )?;
+
+            // SAFETY: `slot` is valid for writes for the duration of this initializer.
+            unsafe {
+                slot.write(Self {
+                    backing,
+                    _pin: PhantomPinned,
+                    _t: PhantomData,
+                })
+            };
+
+            // SAFETY: `slot` points to the fully-initialized registration wrapper we just wrote
+            // above.
+            let this = unsafe { &mut *slot };
+            // SAFETY: `this.as_raw()` points at the fully initialized `struct miscdevice`
+            // contained in the heap-backed registration backing.
+            let ret = to_result(unsafe { bindings::misc_register(this.as_raw()) });
+            if let Err(err) = ret {
+                // SAFETY: The wrapper was fully initialized above, so dropping it here correctly
+                // releases the heap-backed registration backing.
+                unsafe { drop_in_place(slot) };
+                return Err(err);
+            }
+
+            this.backing.registered.store(true, Ordering::Release);
+            Ok(())
+        };
+
+        // SAFETY:
+        // - On success, the closure writes a fully-initialized `Self` into `slot` before making
+        //   the miscdevice visible via `misc_register()`. All state observable through the safe
+        //   open context is initialized before publication.
+        // - On failure after the write, it drops the initialized value before returning.
+        unsafe { pin_init::pin_init_from_closure(init) }
+    }
+
+    /// Returns the registration wrapper for a raw `struct miscdevice` pointer.
+    ///
+    /// # Safety
+    ///
+    /// `misc` must point at the `misc` field of a live [`RegistrationBacking<T>`].
+    unsafe fn from_raw_misc<'a>(misc: *mut bindings::miscdevice) -> &'a Self {
+        // SAFETY: The caller guarantees that `misc` points at the `misc` field of a live
+        // `RegistrationBacking<T>`, whose `owner` points back to the live registration wrapper.
+        let backing =
+            unsafe { &*container_of!(Opaque::cast_from(misc), RegistrationBacking<T>, misc) };
+        // SAFETY: By the type invariant, `owner` points at the live wrapper that owns `backing`.
+        unsafe { &*backing.owner }
     }
 
     /// Returns a raw pointer to the misc device.
     pub fn as_raw(&self) -> *mut bindings::miscdevice {
-        self.inner.get()
+        self.backing.misc.get()
+    }
+
+    /// Returns the registration data that was supplied at registration time.
+    pub fn data(&self) -> &T::RegistrationData {
+        &self.backing.data
+    }
+
+    fn deregister_inner(backing: &RegistrationBacking<T>) {
+        if backing.registered.swap(false, Ordering::AcqRel) {
+            // SAFETY: `registered == true` guarantees that the miscdevice was successfully
+            // registered and has not been deregistered yet.
+            unsafe { bindings::misc_deregister(backing.misc.get()) };
+        }
     }
 
-    /// Access the `this_device` field.
-    pub fn device(&self) -> &Device {
-        // SAFETY: This can only be called after a successful register(), which always
-        // initialises `this_device` with a valid device. Furthermore, the signature of this
-        // function tells the borrow-checker that the `&Device` reference must not outlive the
-        // `&MiscDeviceRegistration<T>` used to obtain it, so the last use of the reference must be
-        // before the underlying `struct miscdevice` is destroyed.
-        unsafe { Device::from_raw((*self.as_raw()).this_device) }
+    /// Deregister this misc device if it is still registered.
+    ///
+    /// After this returns, the misc core will no longer route new opens to [`MiscDevice::open`].
+    /// Existing open files keep their own pinned `file_operations` table and private data and must
+    /// be drained by the driver before it tears down device-side resources that those file handles
+    /// still own.
+    pub fn deregister(&self) {
+        Self::deregister_inner(&self.backing);
     }
 }
 
 #[pinned_drop]
-impl<T> PinnedDrop for MiscDeviceRegistration<T> {
+impl<T: MiscDevice + 'static> PinnedDrop for MiscDeviceRegistration<T> {
     fn drop(self: Pin<&mut Self>) {
-        // SAFETY: We know that the device is registered by the type invariants.
-        unsafe { bindings::misc_deregister(self.inner.get()) };
+        let this = self.project();
+        Self::deregister_inner(this.backing);
+    }
+}
+
+/// Publication-safe context passed to [`MiscDevice::open`].
+pub struct MiscDeviceOpenContext<'a, T: MiscDevice + 'static> {
+    registration: &'a MiscDeviceRegistration<T>,
+    device: ARef<Device>,
+}
+
+impl<'a, T: MiscDevice + 'static> MiscDeviceOpenContext<'a, T> {
+    /// Returns the registration data supplied at registration time.
+    pub fn data(&self) -> &T::RegistrationData {
+        self.registration.data()
+    }
+
+    /// Returns the class device backing this miscdevice open.
+    pub fn device(&self) -> ARef<Device> {
+        self.device.clone()
     }
 }
 
+/// Internal trait that supplies the concrete `file_operations` table used for a Rust miscdevice.
+///
+/// # Safety
+///
+/// Implementations must return a stable `file_operations` table produced by this abstraction so
+/// that owner/module pinning and the private-data protocol remain intact. Drivers should use
+/// [`declare_misc_device_fops!`] instead of implementing this trait manually.
+#[doc(hidden)]
+pub unsafe trait MiscDeviceVTable: MiscDevice + 'static {
+    /// Returns the `file_operations` table for this miscdevice implementation.
+    fn file_operations() -> &'static bindings::file_operations;
+}
+
 /// Trait implemented by the private data of an open misc device.
 #[vtable]
 pub trait MiscDevice: Sized {
     /// What kind of pointer should `Self` be wrapped in.
     type Ptr: ForeignOwnable + Send + Sync;
 
+    /// Driver-defined data stored in the miscdevice registration.
+    type RegistrationData: Send + Sync + 'static;
+
     /// Called when the misc device is opened.
     ///
     /// The returned pointer will be stored as the private data for the file.
-    fn open(_file: &File, _misc: &MiscDeviceRegistration<Self>) -> Result<Self::Ptr>;
+    fn open(_file: &File, _ctx: &MiscDeviceOpenContext<'_, Self>) -> Result<Self::Ptr>;
 
     /// Called when the misc device is released.
     fn release(device: Self::Ptr, _file: &File) {
@@ -195,7 +366,45 @@ fn show_fdinfo(
 /// A vtable for the file operations of a Rust miscdevice.
 struct MiscdeviceVTable<T: MiscDevice>(PhantomData<T>);
 
-impl<T: MiscDevice> MiscdeviceVTable<T> {
+impl<T: MiscDevice + 'static> MiscdeviceVTable<T> {
+    const fn build(owner: *mut bindings::module) -> bindings::file_operations {
+        bindings::file_operations {
+            owner,
+            open: Some(Self::open),
+            release: Some(Self::release),
+            mmap: if T::HAS_MMAP { Some(Self::mmap) } else { None },
+            read_iter: if T::HAS_READ_ITER {
+                Some(Self::read_iter)
+            } else {
+                None
+            },
+            write_iter: if T::HAS_WRITE_ITER {
+                Some(Self::write_iter)
+            } else {
+                None
+            },
+            unlocked_ioctl: if T::HAS_IOCTL {
+                Some(Self::ioctl)
+            } else {
+                None
+            },
+            #[cfg(CONFIG_COMPAT)]
+            compat_ioctl: if T::HAS_COMPAT_IOCTL {
+                Some(Self::compat_ioctl)
+            } else if T::HAS_IOCTL {
+                bindings::compat_ptr_ioctl
+            } else {
+                None
+            },
+            show_fdinfo: if T::HAS_SHOW_FDINFO {
+                Some(Self::show_fdinfo)
+            } else {
+                None
+            },
+            ..pin_init::zeroed()
+        }
+    }
+
     /// # Safety
     ///
     /// `file` and `inode` must be the file and inode for a file that is undergoing initialization.
@@ -214,25 +423,38 @@ impl<T: MiscDevice> MiscdeviceVTable<T> {
         // associated `struct miscdevice` before calling into this method. Furthermore,
         // `misc_open()` ensures that the miscdevice can't be unregistered and freed during this
         // call to `fops_open`.
-        let misc = unsafe { &*misc_ptr.cast::<MiscDeviceRegistration<T>>() };
+        let misc = unsafe { MiscDeviceRegistration::<T>::from_raw_misc(misc_ptr.cast()) };
 
         // SAFETY:
         // * This underlying file is valid for (much longer than) the duration of `T::open`.
         // * There is no active fdget_pos region on the file on this thread.
         let file = unsafe { File::from_raw_file(raw_file) };
 
-        let ptr = match T::open(file, misc) {
+        // SAFETY: `misc_open()` serializes with `misc_deregister()` via `misc_mtx`, so the class
+        // device remains live for the duration of this callback. Taking an extra reference here
+        // lets the safe open context own the device independently from later teardown.
+        let ctx = MiscDeviceOpenContext {
+            registration: misc,
+            device: unsafe { Device::get_device((*misc.as_raw()).this_device) },
+        };
+
+        let ptr = match T::open(file, &ctx) {
             Ok(ptr) => ptr,
             Err(err) => return err.to_errno(),
         };
+        let ptr = ScopeGuard::new_with_data(ptr, |ptr| T::release(ptr, file));
 
-        // This overwrites the private data with the value specified by the user, changing the type
-        // of this file's private data. All future accesses to the private data is performed by
-        // other fops_* methods in this file, which all correctly cast the private data to the new
-        // type.
+        let mut open_file = match KBox::new(OpenFile::<T>::empty(), GFP_KERNEL) {
+            Ok(open_file) => open_file,
+            Err(_) => return ENOMEM.to_errno(),
+        };
+        open_file.data = ptr.dismiss().into_foreign();
+
+        // This overwrites the private data with a small Rust-owned wrapper that keeps the module
+        // pinned for the full file lifetime and owns the driver's foreign private data handle.
         //
         // SAFETY: The open call of a file can access the private data.
-        unsafe { (*raw_file).private_data = ptr.into_foreign() };
+        unsafe { (*raw_file).private_data = open_file.into_foreign() };
 
         0
     }
@@ -243,14 +465,17 @@ impl<T: MiscDevice> MiscdeviceVTable<T> {
     /// must be associated with a `MiscDeviceRegistration<T>`.
     unsafe extern "C" fn release(_inode: *mut bindings::inode, file: *mut bindings::file) -> c_int {
         // SAFETY: The release call of a file owns the private data.
-        let private = unsafe { (*file).private_data };
-        // SAFETY: The release call of a file owns the private data.
-        let ptr = unsafe { <T::Ptr as ForeignOwnable>::from_foreign(private) };
+        let open_file =
+            unsafe { <KBox<OpenFile<T>> as ForeignOwnable>::from_foreign((*file).private_data) };
+        let data = open_file.data;
 
         // SAFETY:
         // * The file is valid for the duration of this call.
         // * There is no active fdget_pos region on the file on this thread.
-        T::release(ptr, unsafe { File::from_raw_file(file) });
+        T::release(
+            unsafe { <T::Ptr as ForeignOwnable>::from_foreign(data) },
+            unsafe { File::from_raw_file(file) },
+        );
 
         0
     }
@@ -304,11 +529,9 @@ impl<T: MiscDevice> MiscdeviceVTable<T> {
         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: This is a Rust Miscdevice, so we call `into_foreign` in `open` and
-        // `from_foreign` in `release`, and `fops_mmap` is guaranteed to be called between those
-        // two operations.
-        let device = unsafe { <T::Ptr as ForeignOwnable>::borrow(private.cast()) };
+        let open_file =
+            unsafe { <KBox<OpenFile<T>> as ForeignOwnable>::borrow((*file).private_data) };
+        let device = open_file.borrow();
         // SAFETY: The caller provides a vma that is undergoing initial VMA setup.
         let area = unsafe { VmaNew::from_raw(vma) };
         // SAFETY:
@@ -327,9 +550,9 @@ impl<T: MiscDevice> MiscdeviceVTable<T> {
     /// `file` must be a valid file that is associated with a `MiscDeviceRegistration<T>`.
     unsafe extern "C" fn ioctl(file: *mut bindings::file, cmd: c_uint, arg: c_ulong) -> c_long {
         // SAFETY: The ioctl call of a file can access the private data.
-        let private = unsafe { (*file).private_data };
-        // SAFETY: Ioctl calls can borrow the private data of the file.
-        let device = unsafe { <T::Ptr as ForeignOwnable>::borrow(private) };
+        let open_file =
+            unsafe { <KBox<OpenFile<T>> as ForeignOwnable>::borrow((*file).private_data) };
+        let device = open_file.borrow();
 
         // SAFETY:
         // * The file is valid for the duration of this call.
@@ -352,9 +575,9 @@ impl<T: MiscDevice> MiscdeviceVTable<T> {
         arg: c_ulong,
     ) -> c_long {
         // SAFETY: The compat ioctl call of a file can access the private data.
-        let private = unsafe { (*file).private_data };
-        // SAFETY: Ioctl calls can borrow the private data of the file.
-        let device = unsafe { <T::Ptr as ForeignOwnable>::borrow(private) };
+        let open_file =
+            unsafe { <KBox<OpenFile<T>> as ForeignOwnable>::borrow((*file).private_data) };
+        let device = open_file.borrow();
 
         // SAFETY:
         // * The file is valid for the duration of this call.
@@ -373,9 +596,9 @@ impl<T: MiscDevice> MiscdeviceVTable<T> {
     /// - `seq_file` must be a valid `struct seq_file` that we can write to.
     unsafe extern "C" fn show_fdinfo(seq_file: *mut bindings::seq_file, file: *mut bindings::file) {
         // SAFETY: The release call of a file owns the private data.
-        let private = unsafe { (*file).private_data };
-        // SAFETY: Ioctl calls can borrow the private data of the file.
-        let device = unsafe { <T::Ptr as ForeignOwnable>::borrow(private) };
+        let open_file =
+            unsafe { <KBox<OpenFile<T>> as ForeignOwnable>::borrow((*file).private_data) };
+        let device = open_file.borrow();
         // SAFETY:
         // * The file is valid for the duration of this call.
         // * There is no active fdget_pos region on the file on this thread.
@@ -386,43 +609,11 @@ impl<T: MiscDevice> MiscdeviceVTable<T> {
 
         T::show_fdinfo(device, m, file);
     }
+}
 
-    const VTABLE: bindings::file_operations = bindings::file_operations {
-        open: Some(Self::open),
-        release: Some(Self::release),
-        mmap: if T::HAS_MMAP { Some(Self::mmap) } else { None },
-        read_iter: if T::HAS_READ_ITER {
-            Some(Self::read_iter)
-        } else {
-            None
-        },
-        write_iter: if T::HAS_WRITE_ITER {
-            Some(Self::write_iter)
-        } else {
-            None
-        },
-        unlocked_ioctl: if T::HAS_IOCTL {
-            Some(Self::ioctl)
-        } else {
-            None
-        },
-        #[cfg(CONFIG_COMPAT)]
-        compat_ioctl: if T::HAS_COMPAT_IOCTL {
-            Some(Self::compat_ioctl)
-        } else if T::HAS_IOCTL {
-            bindings::compat_ptr_ioctl
-        } else {
-            None
-        },
-        show_fdinfo: if T::HAS_SHOW_FDINFO {
-            Some(Self::show_fdinfo)
-        } else {
-            None
-        },
-        ..pin_init::zeroed()
-    };
-
-    const fn build() -> &'static bindings::file_operations {
-        &Self::VTABLE
-    }
+#[doc(hidden)]
+pub const fn build_file_operations<T: MiscDevice + 'static>(
+    owner: *mut bindings::module,
+) -> bindings::file_operations {
+    MiscdeviceVTable::<T>::build(owner)
 }
diff --git a/samples/rust/rust_misc_device.rs b/samples/rust/rust_misc_device.rs
index 87a1fe63533a..f2d7a98a5715 100644
--- a/samples/rust/rust_misc_device.rs
+++ b/samples/rust/rust_misc_device.rs
@@ -100,7 +100,7 @@
     fs::{File, Kiocb},
     ioctl::{_IO, _IOC_SIZE, _IOR, _IOW},
     iov::{IovIterDest, IovIterSource},
-    miscdevice::{MiscDevice, MiscDeviceOptions, MiscDeviceRegistration},
+    miscdevice::{MiscDevice, MiscDeviceOpenContext, MiscDeviceOptions, MiscDeviceRegistration},
     new_mutex,
     prelude::*,
     sync::{aref::ARef, Mutex},
@@ -154,9 +154,10 @@ struct RustMiscDevice {
 #[vtable]
 impl MiscDevice for RustMiscDevice {
     type Ptr = Pin<KBox<Self>>;
+    type RegistrationData = ();
 
-    fn open(_file: &File, misc: &MiscDeviceRegistration<Self>) -> Result<Pin<KBox<Self>>> {
-        let dev = ARef::from(misc.device());
+    fn open(_file: &File, ctx: &MiscDeviceOpenContext<'_, Self>) -> Result<Pin<KBox<Self>>> {
+        let dev = ctx.device();
 
         dev_info!(dev, "Opening Rust Misc Device Sample\n");
 
@@ -222,6 +223,8 @@ fn ioctl(me: Pin<&RustMiscDevice>, _file: &File, cmd: u32, arg: usize) -> Result
     }
 }
 
+kernel::declare_misc_device_fops!(RustMiscDevice);
+
 #[pinned_drop]
 impl PinnedDrop for RustMiscDevice {
     fn drop(self: Pin<&mut Self>) {
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 8+ messages in thread
* [RFC PATCH v3 6/6] platform/goldfish: add Rust goldfish_address_space driver
  2026-04-06 16:51 ` [RFC PATCH v3 0/6] Rust goldfish_address_space driver (ioctl-only subset) Wenzhao Liao
                     ` (4 preceding siblings ...)
  2026-04-06 16:51   ` [RFC PATCH v3 5/6] rust: miscdevice: harden registration and safe file_operations invariants Wenzhao Liao
@ 2026-04-06 16:51   ` Wenzhao Liao
  5 siblings, 0 replies; 8+ messages in thread
From: Wenzhao Liao @ 2026-04-06 16:51 UTC (permalink / raw)
  To: rust-for-linux, linux-pci
  Cc: ojeda, dakr, bhelgaas, kwilczynski, arnd, gregkh, linux-kernel,
	linux-api

Add a Rust implementation of the goldfish address-space driver and wire
it into drivers/platform/goldfish.

This RFC intentionally scopes the driver to the open/release/ioctl ABI
subset; userspace mmap is not part of this series. The driver keeps all
unsafe and bindings-facing work inside the Rust abstraction layers,
carries #![forbid(unsafe_code)] in the driver crate, and uses typed
miscdevice registration data plus SharedMemoryBar to stay on the safe
side of those abstractions.

On teardown, unbind first deregisters the miscdevice, then drains
already-running operations and revokes live file-owned device state
before disabling the PCI function. Probe also pairs
enable_device_mem() with a ScopeGuard so mid-probe failures cannot leak
an enabled device.

Signed-off-by: Wenzhao Liao <wenzhaoliao@ruc.edu.cn>
---
 MAINTAINERS                                   |   2 +
 drivers/platform/goldfish/Kconfig             |  11 +
 drivers/platform/goldfish/Makefile            |   1 +
 .../goldfish/goldfish_address_space.rs        | 917 ++++++++++++++++++
 4 files changed, 931 insertions(+)
 create mode 100644 drivers/platform/goldfish/goldfish_address_space.rs

diff --git a/MAINTAINERS b/MAINTAINERS
index 800b2fe0e648..0a9193854f1b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1888,7 +1888,9 @@ L:	linux-kernel@vger.kernel.org
 L:	linux-pci@vger.kernel.org
 L:	rust-for-linux@vger.kernel.org
 S:	Maintained
+F:	drivers/platform/goldfish/goldfish_address_space.rs
 F:	include/uapi/linux/goldfish_address_space.h
+K:	\bGOLDFISH_ADDRESS_SPACE\b
 
 ANDROID GOLDFISH RTC DRIVER
 M:	Jiaxun Yang <jiaxun.yang@flygoat.com>
diff --git a/drivers/platform/goldfish/Kconfig b/drivers/platform/goldfish/Kconfig
index 03ca5bf19f98..58ccf5a757bd 100644
--- a/drivers/platform/goldfish/Kconfig
+++ b/drivers/platform/goldfish/Kconfig
@@ -17,4 +17,15 @@ config GOLDFISH_PIPE
 	  This is a virtual device to drive the QEMU pipe interface used by
 	  the Goldfish Android Virtual Device.
 
+config GOLDFISH_ADDRESS_SPACE
+	tristate "Goldfish address space driver in Rust"
+	depends on PCI && RUST && MMU
+	help
+	  Adds a Rust implementation of the Goldfish address space driver
+	  used by the Android Goldfish emulator.
+
+	  This implementation uses typed Rust abstractions for PCI resource
+	  setup, miscdevice registration, page-backed ping state, and the
+	  userspace ioctl interface.
+
 endif # GOLDFISH
diff --git a/drivers/platform/goldfish/Makefile b/drivers/platform/goldfish/Makefile
index 76ba1d571896..17f67c223e95 100644
--- a/drivers/platform/goldfish/Makefile
+++ b/drivers/platform/goldfish/Makefile
@@ -3,3 +3,4 @@
 # Makefile for Goldfish platform specific drivers
 #
 obj-$(CONFIG_GOLDFISH_PIPE)	+= goldfish_pipe.o
+obj-$(CONFIG_GOLDFISH_ADDRESS_SPACE) += goldfish_address_space.o
diff --git a/drivers/platform/goldfish/goldfish_address_space.rs b/drivers/platform/goldfish/goldfish_address_space.rs
new file mode 100644
index 000000000000..7742c76ea7fc
--- /dev/null
+++ b/drivers/platform/goldfish/goldfish_address_space.rs
@@ -0,0 +1,917 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Rust Goldfish address space driver.
+
+#![forbid(unsafe_code)]
+
+use core::{mem::size_of, pin::Pin};
+use kernel::{
+    alloc::KVVec,
+    device::Core,
+    devres::Devres,
+    error::Error,
+    fs::File,
+    io::{Io, PhysAddr},
+    ioctl,
+    miscdevice::{MiscDevice, MiscDeviceOpenContext, MiscDeviceOptions, MiscDeviceRegistration},
+    new_condvar, new_mutex,
+    page::{Page, PAGE_SIZE},
+    pci,
+    prelude::*,
+    sync::{Arc, ArcBorrow, CondVar, Mutex},
+    types::ScopeGuard,
+    uaccess::{UserPtr, UserSlice},
+    uapi,
+};
+
+const GOLDFISH_AS_CONTROL_BAR: u32 = 0;
+const GOLDFISH_AS_AREA_BAR: u32 = 1;
+const GOLDFISH_AS_VENDOR_ID: u32 = 0x607d;
+const GOLDFISH_AS_DEVICE_ID: u32 = 0xf153;
+const GOLDFISH_AS_SUPPORTED_REVISION: u8 = 1;
+const GOLDFISH_AS_INVALID_HANDLE: u32 = u32::MAX;
+
+const GOLDFISH_ADDRESS_SPACE_IOCTL_MAGIC: u32 = uapi::GOLDFISH_ADDRESS_SPACE_IOCTL_MAGIC as u32;
+const GOLDFISH_ADDRESS_SPACE_IOCTL_ALLOCATE_BLOCK: u32 =
+    ioctl::_IOWR::<AllocateBlockIoctl>(GOLDFISH_ADDRESS_SPACE_IOCTL_MAGIC, 10);
+const GOLDFISH_ADDRESS_SPACE_IOCTL_DEALLOCATE_BLOCK: u32 =
+    ioctl::_IOWR::<u64>(GOLDFISH_ADDRESS_SPACE_IOCTL_MAGIC, 11);
+const GOLDFISH_ADDRESS_SPACE_IOCTL_PING: u32 =
+    ioctl::_IOWR::<PingIoctl>(GOLDFISH_ADDRESS_SPACE_IOCTL_MAGIC, 12);
+const GOLDFISH_ADDRESS_SPACE_IOCTL_CLAIM_SHARED: u32 =
+    ioctl::_IOWR::<ClaimSharedIoctl>(GOLDFISH_ADDRESS_SPACE_IOCTL_MAGIC, 13);
+const GOLDFISH_ADDRESS_SPACE_IOCTL_UNCLAIM_SHARED: u32 =
+    ioctl::_IOWR::<u64>(GOLDFISH_ADDRESS_SPACE_IOCTL_MAGIC, 14);
+
+struct Registers;
+
+impl Registers {
+    const COMMAND: usize = 0;
+    const STATUS: usize = 4;
+    const GUEST_PAGE_SIZE: usize = 8;
+    const BLOCK_SIZE_LOW: usize = 12;
+    const BLOCK_SIZE_HIGH: usize = 16;
+    const BLOCK_OFFSET_LOW: usize = 20;
+    const BLOCK_OFFSET_HIGH: usize = 24;
+    const PING: usize = 28;
+    const PING_INFO_ADDR_LOW: usize = 32;
+    const PING_INFO_ADDR_HIGH: usize = 36;
+    const HANDLE: usize = 40;
+    const PHYS_START_LOW: usize = 44;
+    const PHYS_START_HIGH: usize = 48;
+    const END: usize = 56;
+}
+
+#[repr(u32)]
+#[derive(Clone, Copy)]
+enum CommandId {
+    AllocateBlock = 1,
+    DeallocateBlock = 2,
+    GenHandle = 3,
+    DestroyHandle = 4,
+    TellPingInfoAddr = 5,
+}
+
+type ControlBar = pci::Bar<{ Registers::END }>;
+
+#[derive(Clone, Copy)]
+struct Block {
+    offset: u64,
+    size: u64,
+}
+
+struct BlockSet {
+    blocks: KVVec<Block>,
+}
+
+impl BlockSet {
+    fn new() -> Self {
+        Self {
+            blocks: KVVec::new(),
+        }
+    }
+
+    fn insert(&mut self, block: Block) -> Result {
+        self.blocks.push(block, GFP_KERNEL)?;
+        Ok(())
+    }
+
+    fn remove(&mut self, offset: u64) -> Result<Block> {
+        let index = self
+            .blocks
+            .iter()
+            .position(|block| block.offset == offset)
+            .ok_or(ENXIO)?;
+        self.blocks.remove(index).map_err(|_| EINVAL)
+    }
+
+    fn iter(&self) -> impl Iterator<Item = Block> + '_ {
+        self.blocks.iter().copied()
+    }
+
+    fn clear(&mut self) {
+        let _ = self.take_all();
+    }
+
+    fn take_all(&mut self) -> KVVec<Block> {
+        let mut blocks = KVVec::new();
+        core::mem::swap(&mut blocks, &mut self.blocks);
+        blocks
+    }
+}
+
+#[derive(Clone, Copy, Default)]
+struct PingInfoHeader {
+    offset: u64,
+    size: u64,
+    metadata: u64,
+    version: u32,
+    wait_fd: u32,
+    wait_flags: u32,
+    direction: u32,
+    data_size: u64,
+}
+
+impl PingInfoHeader {
+    const ENCODED_LEN: usize = 48;
+
+    fn encode(self) -> [u8; Self::ENCODED_LEN] {
+        let mut bytes = [0u8; Self::ENCODED_LEN];
+
+        bytes[0..8].copy_from_slice(&self.offset.to_ne_bytes());
+        bytes[8..16].copy_from_slice(&self.size.to_ne_bytes());
+        bytes[16..24].copy_from_slice(&self.metadata.to_ne_bytes());
+        bytes[24..28].copy_from_slice(&self.version.to_ne_bytes());
+        bytes[28..32].copy_from_slice(&self.wait_fd.to_ne_bytes());
+        bytes[32..36].copy_from_slice(&self.wait_flags.to_ne_bytes());
+        bytes[36..40].copy_from_slice(&self.direction.to_ne_bytes());
+        bytes[40..48].copy_from_slice(&self.data_size.to_ne_bytes());
+
+        bytes
+    }
+
+    fn decode(bytes: &[u8; Self::ENCODED_LEN]) -> Self {
+        Self {
+            offset: u64::from_ne_bytes(bytes[0..8].try_into().unwrap()),
+            size: u64::from_ne_bytes(bytes[8..16].try_into().unwrap()),
+            metadata: u64::from_ne_bytes(bytes[16..24].try_into().unwrap()),
+            version: u32::from_ne_bytes(bytes[24..28].try_into().unwrap()),
+            wait_fd: u32::from_ne_bytes(bytes[28..32].try_into().unwrap()),
+            wait_flags: u32::from_ne_bytes(bytes[32..36].try_into().unwrap()),
+            direction: u32::from_ne_bytes(bytes[36..40].try_into().unwrap()),
+            data_size: u64::from_ne_bytes(bytes[40..48].try_into().unwrap()),
+        }
+    }
+}
+
+#[repr(C)]
+#[derive(Clone, Copy, Default)]
+struct AllocateBlockIoctl {
+    size: u64,
+    offset: u64,
+    phys_addr: u64,
+}
+
+impl AllocateBlockIoctl {
+    const ENCODED_LEN: usize = 24;
+
+    fn encode(self) -> [u8; Self::ENCODED_LEN] {
+        let mut bytes = [0u8; Self::ENCODED_LEN];
+        bytes[0..8].copy_from_slice(&self.size.to_ne_bytes());
+        bytes[8..16].copy_from_slice(&self.offset.to_ne_bytes());
+        bytes[16..24].copy_from_slice(&self.phys_addr.to_ne_bytes());
+        bytes
+    }
+
+    fn decode(bytes: &[u8; Self::ENCODED_LEN]) -> Self {
+        Self {
+            size: u64::from_ne_bytes(bytes[0..8].try_into().unwrap()),
+            offset: u64::from_ne_bytes(bytes[8..16].try_into().unwrap()),
+            phys_addr: u64::from_ne_bytes(bytes[16..24].try_into().unwrap()),
+        }
+    }
+}
+
+#[repr(C)]
+#[derive(Clone, Copy, Default)]
+struct PingIoctl {
+    offset: u64,
+    size: u64,
+    metadata: u64,
+    version: u32,
+    wait_fd: u32,
+    wait_flags: u32,
+    direction: u32,
+}
+
+impl PingIoctl {
+    const ENCODED_LEN: usize = 40;
+
+    fn encode(self) -> [u8; Self::ENCODED_LEN] {
+        let mut bytes = [0u8; Self::ENCODED_LEN];
+        bytes[0..8].copy_from_slice(&self.offset.to_ne_bytes());
+        bytes[8..16].copy_from_slice(&self.size.to_ne_bytes());
+        bytes[16..24].copy_from_slice(&self.metadata.to_ne_bytes());
+        bytes[24..28].copy_from_slice(&self.version.to_ne_bytes());
+        bytes[28..32].copy_from_slice(&self.wait_fd.to_ne_bytes());
+        bytes[32..36].copy_from_slice(&self.wait_flags.to_ne_bytes());
+        bytes[36..40].copy_from_slice(&self.direction.to_ne_bytes());
+        bytes
+    }
+
+    fn decode(bytes: &[u8; Self::ENCODED_LEN]) -> Self {
+        Self {
+            offset: u64::from_ne_bytes(bytes[0..8].try_into().unwrap()),
+            size: u64::from_ne_bytes(bytes[8..16].try_into().unwrap()),
+            metadata: u64::from_ne_bytes(bytes[16..24].try_into().unwrap()),
+            version: u32::from_ne_bytes(bytes[24..28].try_into().unwrap()),
+            wait_fd: u32::from_ne_bytes(bytes[28..32].try_into().unwrap()),
+            wait_flags: u32::from_ne_bytes(bytes[32..36].try_into().unwrap()),
+            direction: u32::from_ne_bytes(bytes[36..40].try_into().unwrap()),
+        }
+    }
+}
+
+#[repr(C)]
+#[derive(Clone, Copy, Default)]
+struct ClaimSharedIoctl {
+    offset: u64,
+    size: u64,
+}
+
+impl ClaimSharedIoctl {
+    const ENCODED_LEN: usize = 16;
+
+    fn encode(self) -> [u8; Self::ENCODED_LEN] {
+        let mut bytes = [0u8; Self::ENCODED_LEN];
+        bytes[0..8].copy_from_slice(&self.offset.to_ne_bytes());
+        bytes[8..16].copy_from_slice(&self.size.to_ne_bytes());
+        bytes
+    }
+
+    fn decode(bytes: &[u8; Self::ENCODED_LEN]) -> Self {
+        Self {
+            offset: u64::from_ne_bytes(bytes[0..8].try_into().unwrap()),
+            size: u64::from_ne_bytes(bytes[8..16].try_into().unwrap()),
+        }
+    }
+}
+
+struct PingState {
+    page: Page,
+}
+
+impl PingState {
+    fn new() -> Result<Self> {
+        let mut page = Page::alloc_page(GFP_KERNEL)?;
+        page.fill_zero(0, PAGE_SIZE)?;
+        Ok(Self { page })
+    }
+
+    fn phys_addr(&self) -> PhysAddr {
+        self.page.phys_addr()
+    }
+
+    fn shared_offset(offset: u64, shared_phys_start: PhysAddr) -> Result<u64> {
+        let shared_phys_start = u64::try_from(shared_phys_start).map_err(|_| EOVERFLOW)?;
+        offset.checked_add(shared_phys_start).ok_or(EOVERFLOW)
+    }
+
+    fn prepare_ping(&mut self, request: &PingIoctl, shared_phys_start: PhysAddr) -> Result {
+        let header = PingInfoHeader {
+            offset: Self::shared_offset(request.offset, shared_phys_start)?,
+            size: request.size,
+            metadata: request.metadata,
+            version: request.version,
+            wait_fd: request.wait_fd,
+            wait_flags: request.wait_flags,
+            direction: request.direction,
+            data_size: 0,
+        };
+
+        self.page.fill_zero(0, PAGE_SIZE)?;
+        self.page.write_slice(&header.encode(), 0)
+    }
+
+    fn finish_ping(&self, request: &mut PingIoctl) -> Result {
+        let mut bytes = [0u8; PingInfoHeader::ENCODED_LEN];
+        self.page.read_slice(&mut bytes, 0)?;
+        let header = PingInfoHeader::decode(&bytes);
+        request.offset = header.offset;
+        request.size = header.size;
+        request.metadata = header.metadata;
+        request.version = header.version;
+        request.wait_fd = header.wait_fd;
+        request.wait_flags = header.wait_flags;
+        request.direction = header.direction;
+        Ok(())
+    }
+}
+
+#[pin_data]
+struct DeviceRuntime {
+    #[pin]
+    control_bar: Devres<ControlBar>,
+    #[pin]
+    shared_bar: Devres<pci::SharedMemoryBar>,
+    #[pin]
+    registers_lock: Mutex<()>,
+    #[pin]
+    lifecycle: Mutex<RuntimeLifecycleState>,
+    #[pin]
+    lifecycle_idle: CondVar,
+}
+
+struct RuntimeLifecycleState {
+    accepting_new_ops: bool,
+    active_ops: usize,
+    live_files: KVVec<Arc<FileState>>,
+}
+
+struct RuntimeOpGuard {
+    runtime: Arc<DeviceRuntime>,
+}
+
+impl Drop for RuntimeOpGuard {
+    fn drop(&mut self) {
+        let mut state = self.runtime.lifecycle.lock();
+        state.active_ops -= 1;
+        self.runtime.notify_if_idle(&state);
+    }
+}
+
+impl DeviceRuntime {
+    fn new(pdev: &pci::Device<Core>) -> Result<Arc<Self>> {
+        Arc::pin_init(
+            try_pin_init!(Self {
+                control_bar <- pdev.iomap_region_sized::<{ Registers::END }>(
+                    GOLDFISH_AS_CONTROL_BAR,
+                    c"goldfish_address_space/control",
+                ),
+                shared_bar <- pdev.memremap_bar(
+                    GOLDFISH_AS_AREA_BAR,
+                    c"goldfish_address_space/area",
+                ),
+                registers_lock <- new_mutex!(()),
+                lifecycle <- new_mutex!(RuntimeLifecycleState {
+                    accepting_new_ops: true,
+                    active_ops: 0,
+                    live_files: KVVec::new(),
+                }),
+                lifecycle_idle <- new_condvar!("goldfish_address_space/lifecycle_idle"),
+            }),
+            GFP_KERNEL,
+        )
+    }
+
+    fn notify_if_idle(&self, state: &RuntimeLifecycleState) {
+        if !state.accepting_new_ops && state.active_ops == 0 {
+            self.lifecycle_idle.notify_all();
+        }
+    }
+
+    fn begin_operation(self: &Arc<Self>) -> Result<RuntimeOpGuard> {
+        let mut state = self.lifecycle.lock();
+        if !state.accepting_new_ops {
+            return Err(ENODEV);
+        }
+
+        state.active_ops = state.active_ops.checked_add(1).ok_or(EBUSY)?;
+        drop(state);
+
+        Ok(RuntimeOpGuard {
+            runtime: self.clone(),
+        })
+    }
+
+    fn register_live_file(&self, file: Arc<FileState>) -> Result {
+        let mut state = self.lifecycle.lock();
+        if !state.accepting_new_ops {
+            return Err(ENODEV);
+        }
+
+        state.live_files.push(file, GFP_KERNEL)?;
+        Ok(())
+    }
+
+    fn unregister_live_file(&self, file: &Arc<FileState>) {
+        let mut state = self.lifecycle.lock();
+        let Some(index) = state
+            .live_files
+            .iter()
+            .position(|entry| Arc::ptr_eq(entry, file))
+        else {
+            return;
+        };
+
+        if let Ok(entry) = state.live_files.remove(index) {
+            drop(entry);
+        }
+    }
+
+    fn shutdown(&self) {
+        let mut state = self.lifecycle.lock();
+        // `unbind()` removes miscdevice reachability before calling `shutdown()`. After that we
+        // only need to wait for already-entered syscalls to finish; live files are revoked below,
+        // so remove is no longer bounded by userspace deciding to close descriptors.
+        state.accepting_new_ops = false;
+
+        while state.active_ops != 0 {
+            self.lifecycle_idle.wait(&mut state);
+        }
+
+        let mut live_files = KVVec::new();
+        core::mem::swap(&mut live_files, &mut state.live_files);
+        drop(state);
+
+        for file in &live_files {
+            file.revoke_for_shutdown();
+        }
+    }
+
+    fn control_bar(&self) -> Result<impl core::ops::Deref<Target = ControlBar> + '_> {
+        self.control_bar.try_access().ok_or(ENXIO)
+    }
+
+    fn shared_bar(&self) -> Result<impl core::ops::Deref<Target = pci::SharedMemoryBar> + '_> {
+        self.shared_bar.try_access().ok_or(ENXIO)
+    }
+
+    fn run_command_locked(control: &ControlBar, command: CommandId) -> Result {
+        control.write32(command as u32, Registers::COMMAND);
+
+        let status = i32::try_from(control.read32(Registers::STATUS)).map_err(|_| EIO)?;
+        if status == 0 {
+            Ok(())
+        } else {
+            Err(Error::from_errno(-status))
+        }
+    }
+
+    fn issue_command_locked(control: &ControlBar, command: CommandId) {
+        control.write32(command as u32, Registers::COMMAND);
+    }
+
+    fn write_u64(control: &ControlBar, low_offset: usize, high_offset: usize, value: u64) {
+        control.write32(value as u32, low_offset);
+        control.write32((value >> 32) as u32, high_offset);
+    }
+
+    fn read_u64(control: &ControlBar, low_offset: usize, high_offset: usize) -> u64 {
+        u64::from(control.read32(low_offset)) | (u64::from(control.read32(high_offset)) << 32)
+    }
+
+    fn program_host_visible_state(&self) -> Result {
+        let control = self.control_bar()?;
+        let shared = self.shared_bar()?;
+        let phys_start = u64::try_from(shared.phys_start()).map_err(|_| EOVERFLOW)?;
+
+        control.write32(PAGE_SIZE as u32, Registers::GUEST_PAGE_SIZE);
+        Self::write_u64(
+            &control,
+            Registers::PHYS_START_LOW,
+            Registers::PHYS_START_HIGH,
+            phys_start,
+        );
+
+        Ok(())
+    }
+
+    fn shared_phys_start(&self) -> Result<PhysAddr> {
+        Ok(self.shared_bar()?.phys_start())
+    }
+
+    fn generate_handle(&self) -> Result<u32> {
+        let _guard = self.registers_lock.lock();
+        let control = self.control_bar()?;
+
+        // The external C driver does not gate `GEN_HANDLE` on the status register and instead
+        // validates completion by reading back the handle.
+        Self::issue_command_locked(&control, CommandId::GenHandle);
+
+        let handle = control.read32(Registers::HANDLE);
+        if handle == GOLDFISH_AS_INVALID_HANDLE {
+            return Err(EINVAL);
+        }
+
+        Ok(handle)
+    }
+
+    fn tell_ping_info_addr(&self, handle: u32, ping_info_phys: PhysAddr) -> Result {
+        let _guard = self.registers_lock.lock();
+        let control = self.control_bar()?;
+        let ping_info_phys = u64::try_from(ping_info_phys).map_err(|_| EOVERFLOW)?;
+
+        control.write32(handle, Registers::HANDLE);
+        Self::write_u64(
+            &control,
+            Registers::PING_INFO_ADDR_LOW,
+            Registers::PING_INFO_ADDR_HIGH,
+            ping_info_phys,
+        );
+        // The external C driver validates `TELL_PING_INFO_ADDR` through the echoed physical
+        // address rather than through the status register.
+        Self::issue_command_locked(&control, CommandId::TellPingInfoAddr);
+
+        let returned = Self::read_u64(
+            &control,
+            Registers::PING_INFO_ADDR_LOW,
+            Registers::PING_INFO_ADDR_HIGH,
+        );
+        if returned != ping_info_phys {
+            return Err(EINVAL);
+        }
+
+        Ok(())
+    }
+
+    fn destroy_handle(&self, handle: u32) -> Result {
+        let _guard = self.registers_lock.lock();
+        let control = self.control_bar()?;
+        control.write32(handle, Registers::HANDLE);
+        Self::issue_command_locked(&control, CommandId::DestroyHandle);
+        Ok(())
+    }
+
+    fn allocate_block(&self, size: u64) -> Result<Block> {
+        let _guard = self.registers_lock.lock();
+        let control = self.control_bar()?;
+
+        Self::write_u64(
+            &control,
+            Registers::BLOCK_SIZE_LOW,
+            Registers::BLOCK_SIZE_HIGH,
+            size,
+        );
+        Self::run_command_locked(&control, CommandId::AllocateBlock)?;
+
+        Ok(Block {
+            offset: Self::read_u64(
+                &control,
+                Registers::BLOCK_OFFSET_LOW,
+                Registers::BLOCK_OFFSET_HIGH,
+            ),
+            size: Self::read_u64(
+                &control,
+                Registers::BLOCK_SIZE_LOW,
+                Registers::BLOCK_SIZE_HIGH,
+            ),
+        })
+    }
+
+    fn deallocate_block(&self, offset: u64) -> Result {
+        let _guard = self.registers_lock.lock();
+        let control = self.control_bar()?;
+        Self::write_u64(
+            &control,
+            Registers::BLOCK_OFFSET_LOW,
+            Registers::BLOCK_OFFSET_HIGH,
+            offset,
+        );
+        Self::run_command_locked(&control, CommandId::DeallocateBlock)
+    }
+
+    fn ping(&self, handle: u32) -> Result {
+        let _guard = self.registers_lock.lock();
+        self.control_bar()?.write32(handle, Registers::PING);
+        Ok(())
+    }
+
+    fn cleanup_file_resources<I>(&self, handle: u32, blocks: I)
+    where
+        I: IntoIterator<Item = Block>,
+    {
+        // `unbind()` revokes live files before `disable_device()`, so both the shutdown path and a
+        // concurrent `release()` may still legitimately touch the BAR here.
+        if let Err(err) = self.destroy_handle(handle) {
+            pr_warn!(
+                "goldfish_address_space: destroy handle {} failed: {}\n",
+                handle,
+                err.to_errno()
+            );
+        }
+
+        for block in blocks {
+            if let Err(err) = self.deallocate_block(block.offset) {
+                pr_warn!(
+                    "goldfish_address_space: deallocate block 0x{:x} failed: {}\n",
+                    block.offset,
+                    err.to_errno()
+                );
+            }
+        }
+    }
+}
+
+struct FileResources {
+    handle: Option<u32>,
+    allocated_blocks: BlockSet,
+    shared_blocks: BlockSet,
+}
+
+impl FileResources {
+    fn new(handle: u32) -> Self {
+        Self {
+            handle: Some(handle),
+            allocated_blocks: BlockSet::new(),
+            shared_blocks: BlockSet::new(),
+        }
+    }
+}
+
+#[pin_data]
+struct FileState {
+    runtime: Arc<DeviceRuntime>,
+    #[pin]
+    ping: Mutex<PingState>,
+    #[pin]
+    resources: Mutex<FileResources>,
+}
+
+impl FileState {
+    fn new(runtime: Arc<DeviceRuntime>, handle: u32, ping: PingState) -> Result<Arc<Self>> {
+        Arc::pin_init(
+            try_pin_init!(Self {
+                runtime: runtime,
+                ping <- new_mutex!(ping),
+                resources <- new_mutex!(FileResources::new(handle)),
+            }),
+            GFP_KERNEL,
+        )
+    }
+
+    fn shared_phys_addr(&self, offset: u64) -> Result<u64> {
+        let base = u64::try_from(self.runtime.shared_phys_start()?).map_err(|_| EOVERFLOW)?;
+        base.checked_add(offset).ok_or(EOVERFLOW)
+    }
+
+    fn allocate_block(
+        self: ArcBorrow<'_, Self>,
+        mut request: AllocateBlockIoctl,
+    ) -> Result<AllocateBlockIoctl> {
+        let block = self.runtime.allocate_block(request.size)?;
+        let mut resources = self.resources.lock();
+        if resources.handle.is_none() {
+            drop(resources);
+            let _ = self.runtime.deallocate_block(block.offset);
+            return Err(ENODEV);
+        }
+
+        if let Err(err) = resources.allocated_blocks.insert(block) {
+            drop(resources);
+            let _ = self.runtime.deallocate_block(block.offset);
+            return Err(err);
+        }
+
+        request.size = block.size;
+        request.offset = block.offset;
+        request.phys_addr = self.shared_phys_addr(block.offset)?;
+        Ok(request)
+    }
+
+    fn deallocate_block(self: ArcBorrow<'_, Self>, offset: u64) -> Result {
+        let mut resources = self.resources.lock();
+        if resources.handle.is_none() {
+            return Err(ENODEV);
+        }
+
+        if !resources
+            .allocated_blocks
+            .iter()
+            .any(|block| block.offset == offset)
+        {
+            return Err(ENXIO);
+        }
+
+        self.runtime.deallocate_block(offset)?;
+        let _ = resources.allocated_blocks.remove(offset)?;
+        Ok(())
+    }
+
+    fn claim_shared(
+        self: ArcBorrow<'_, Self>,
+        request: ClaimSharedIoctl,
+    ) -> Result<ClaimSharedIoctl> {
+        let mut resources = self.resources.lock();
+        if resources.handle.is_none() {
+            return Err(ENODEV);
+        }
+
+        resources.shared_blocks.insert(Block {
+            offset: request.offset,
+            size: request.size,
+        })?;
+        Ok(request)
+    }
+
+    fn unclaim_shared(self: ArcBorrow<'_, Self>, offset: u64) -> Result {
+        let mut resources = self.resources.lock();
+        if resources.handle.is_none() {
+            return Err(ENODEV);
+        }
+
+        resources.shared_blocks.remove(offset)?;
+        Ok(())
+    }
+
+    fn ping(self: ArcBorrow<'_, Self>, mut request: PingIoctl) -> Result<PingIoctl> {
+        let handle = self.resources.lock().handle.ok_or(ENODEV)?;
+        let mut ping = self.ping.lock();
+        ping.prepare_ping(&request, self.runtime.shared_phys_start()?)?;
+        self.runtime.ping(handle)?;
+        ping.finish_ping(&mut request)?;
+        Ok(request)
+    }
+
+    fn cleanup_resources(&self) {
+        let mut resources = self.resources.lock();
+        let Some(handle) = resources.handle.take() else {
+            return;
+        };
+
+        self.runtime
+            .cleanup_file_resources(handle, resources.allocated_blocks.iter());
+        resources.allocated_blocks.clear();
+        resources.shared_blocks.clear();
+    }
+
+    fn revoke_for_shutdown(&self) {
+        self.cleanup_resources();
+    }
+
+    fn release(self: Arc<Self>) {
+        self.cleanup_resources();
+        self.runtime.unregister_live_file(&self);
+    }
+}
+
+#[pin_data]
+struct GoldfishAddressSpaceDriver {
+    runtime: Arc<DeviceRuntime>,
+    #[pin]
+    misc: MiscDeviceRegistration<GoldfishAddressSpaceMisc>,
+}
+
+struct GoldfishAddressSpaceMisc;
+
+#[vtable]
+impl MiscDevice for GoldfishAddressSpaceMisc {
+    type Ptr = Arc<FileState>;
+    type RegistrationData = Arc<DeviceRuntime>;
+
+    fn open(_file: &File, ctx: &MiscDeviceOpenContext<'_, Self>) -> Result<Self::Ptr> {
+        let runtime = ctx.data().clone();
+        let _op = runtime.begin_operation()?;
+        let ping = PingState::new()?;
+        let handle = runtime.generate_handle()?;
+        let cleanup = ScopeGuard::new_with_data((runtime.clone(), handle), |(runtime, handle)| {
+            let _ = runtime.destroy_handle(handle);
+        });
+
+        runtime.tell_ping_info_addr(handle, ping.phys_addr())?;
+        let state = FileState::new(runtime.clone(), handle, ping)?;
+        cleanup.dismiss();
+
+        // Publish the file as a live shutdown owner before returning it to the miscdevice core.
+        if let Err(err) = runtime.register_live_file(state.clone()) {
+            state.release();
+            return Err(err);
+        }
+
+        Ok(state)
+    }
+
+    fn release(device: Self::Ptr, _file: &File) {
+        device.release();
+    }
+
+    fn ioctl(
+        device: ArcBorrow<'_, FileState>,
+        _file: &File,
+        cmd: u32,
+        arg: usize,
+    ) -> Result<isize> {
+        let _op = device.runtime.begin_operation()?;
+        match cmd {
+            GOLDFISH_ADDRESS_SPACE_IOCTL_ALLOCATE_BLOCK => {
+                let data = UserSlice::new(UserPtr::from_addr(arg), AllocateBlockIoctl::ENCODED_LEN);
+                let (mut reader, mut writer) = data.reader_writer();
+                let mut bytes = [0u8; AllocateBlockIoctl::ENCODED_LEN];
+                reader.read_slice(&mut bytes)?;
+                let request = AllocateBlockIoctl::decode(&bytes);
+                let response = device.allocate_block(request)?;
+                writer.write_slice(&response.encode())?;
+                Ok(0)
+            }
+            GOLDFISH_ADDRESS_SPACE_IOCTL_DEALLOCATE_BLOCK => {
+                let mut reader = UserSlice::new(UserPtr::from_addr(arg), size_of::<u64>()).reader();
+                device.deallocate_block(reader.read::<u64>()?)?;
+                Ok(0)
+            }
+            GOLDFISH_ADDRESS_SPACE_IOCTL_PING => {
+                let data = UserSlice::new(UserPtr::from_addr(arg), PingIoctl::ENCODED_LEN);
+                let (mut reader, mut writer) = data.reader_writer();
+                let mut bytes = [0u8; PingIoctl::ENCODED_LEN];
+                reader.read_slice(&mut bytes)?;
+                let request = PingIoctl::decode(&bytes);
+                let response = device.ping(request)?;
+                writer.write_slice(&response.encode())?;
+                Ok(0)
+            }
+            GOLDFISH_ADDRESS_SPACE_IOCTL_CLAIM_SHARED => {
+                let data = UserSlice::new(UserPtr::from_addr(arg), ClaimSharedIoctl::ENCODED_LEN);
+                let (mut reader, mut writer) = data.reader_writer();
+                let mut bytes = [0u8; ClaimSharedIoctl::ENCODED_LEN];
+                reader.read_slice(&mut bytes)?;
+                let request = ClaimSharedIoctl::decode(&bytes);
+                let response = device.claim_shared(request)?;
+                writer.write_slice(&response.encode())?;
+                Ok(0)
+            }
+            GOLDFISH_ADDRESS_SPACE_IOCTL_UNCLAIM_SHARED => {
+                let mut reader = UserSlice::new(UserPtr::from_addr(arg), size_of::<u64>()).reader();
+                device.unclaim_shared(reader.read::<u64>()?)?;
+                Ok(0)
+            }
+            _ => Err(ENOTTY),
+        }
+    }
+
+    #[cfg(CONFIG_COMPAT)]
+    fn compat_ioctl(
+        device: ArcBorrow<'_, FileState>,
+        file: &File,
+        cmd: u32,
+        arg: usize,
+    ) -> Result<isize> {
+        Self::ioctl(device, file, cmd, arg)
+    }
+}
+
+kernel::declare_misc_device_fops!(GoldfishAddressSpaceMisc);
+
+kernel::pci_device_table!(
+    PCI_TABLE,
+    MODULE_PCI_TABLE,
+    <GoldfishAddressSpaceDriver as pci::Driver>::IdInfo,
+    [(
+        pci::DeviceId::from_id(
+            pci::Vendor::from_raw(GOLDFISH_AS_VENDOR_ID as u16),
+            GOLDFISH_AS_DEVICE_ID,
+        ),
+        (),
+    )]
+);
+
+impl pci::Driver for GoldfishAddressSpaceDriver {
+    type IdInfo = ();
+
+    const ID_TABLE: pci::IdTable<Self::IdInfo> = &PCI_TABLE;
+
+    fn probe(pdev: &pci::Device<Core>, _id_info: &Self::IdInfo) -> impl PinInit<Self, Error> {
+        pin_init::pin_init_scope(move || {
+            if pdev.revision_id() != GOLDFISH_AS_SUPPORTED_REVISION {
+                return Err(ENODEV);
+            }
+
+            pdev.enable_device_mem()?;
+            let enable_guard = ScopeGuard::new(|| pdev.disable_device());
+
+            let runtime = DeviceRuntime::new(pdev)?;
+            runtime.program_host_visible_state()?;
+
+            let driver = try_pin_init!(Self {
+                runtime: runtime.clone(),
+                misc <- MiscDeviceRegistration::register_with_data(
+                    MiscDeviceOptions {
+                        name: c"goldfish_address_space",
+                    },
+                    runtime.clone(),
+                ),
+            });
+            enable_guard.dismiss();
+
+            Ok(driver)
+        })
+    }
+
+    fn unbind(pdev: &pci::Device<Core>, this: Pin<&Self>) {
+        let this = this.get_ref();
+        // 1. Stop new miscdevice opens from reaching the driver.
+        this.misc.deregister();
+        // 2. Wait for already-running syscalls, then revoke every still-live file's device-side
+        //    state before the PCI function disappears.
+        this.runtime.shutdown();
+        // 3. Only then disable the PCI function, so post-shutdown release never needs to touch a
+        //    disabled device.
+        pdev.disable_device();
+    }
+}
+
+kernel::module_pci_driver! {
+    type: GoldfishAddressSpaceDriver,
+    name: "goldfish_address_space",
+    authors: ["Wenzhao Liao"],
+    description: "Rust Goldfish address space driver",
+    license: "GPL v2",
+}
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 8+ messages in thread
* Re: [RFC PATCH v3 1/6] uapi: add goldfish_address_space userspace ABI header
  2026-04-06 16:51   ` [RFC PATCH v3 1/6] uapi: add goldfish_address_space userspace ABI header Wenzhao Liao
@ 2026-04-13 16:28     ` Arnd Bergmann
  0 siblings, 0 replies; 8+ messages in thread
From: Arnd Bergmann @ 2026-04-13 16:28 UTC (permalink / raw)
  To: Wenzhao Liao, rust-for-linux, linux-pci
  Cc: Miguel Ojeda, Danilo Krummrich, bhelgaas,
	Krzysztof Wilczyński, Greg Kroah-Hartman, linux-kernel,
	linux-api

On Mon, Apr 6, 2026, at 18:51, Wenzhao Liao wrote:

> +struct goldfish_address_space_allocate_block {
> +	__u64 size;
> +	__u64 offset;
> +	__u64 phys_addr;
> +};
> +
> +struct goldfish_address_space_ping {
> +	__u64 offset;
> +	__u64 size;
> +	__u64 metadata;
> +	__u32 version;
> +	__u32 wait_fd;
> +	__u32 wait_flags;
> +	__u32 direction;
> +};
> +
> +struct goldfish_address_space_claim_shared {
> +	__u64 offset;
> +	__u64 size;
> +};

All these ioctl structures are well-formed in the sense that they
are portable across architectures and won't leak kernel data
through implicit padding.

Two of the members are a bit worrying, but that may just
be my own understanding:

- the 'phys_addr' member sounds like it refers to a physical
  memory location in the CPU address space, which in general
  should not be visible to user space, as that tends to
  expose security problems if users with access to the
  device can use this to access data they should not.

- the 'version' field may refer to the version of the ioctl
  command, which is similarly discouraged since it is
  harder to deal with than just coming up with new ioctl
  command codes. If this refers to the version of the
  remote side, this is probably fine.

> +#define GOLDFISH_ADDRESS_SPACE_IOCTL_MAGIC 'G'
> +
> +#define GOLDFISH_ADDRESS_SPACE_IOCTL_OP(OP, T) \
> +	_IOWR(GOLDFISH_ADDRESS_SPACE_IOCTL_MAGIC, OP, T)

I think it would be better to remove this intermediate macro, since
it prevents easy scraping of ioctl command codes from looking
at the source file with regular expressions.

It is also unusual that all commands are both reading
and writing the data. Please check if you can make some
of them read-only or write-only.

     Arnd

^ permalink raw reply	[flat|nested] 8+ messages in thread
end of thread, other threads:[~2026-04-13 16:28 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
     [not found] <cover.1775456181.git.wenzhaoliao@ruc.edu.cn>
2026-04-06 16:51 ` [RFC PATCH v3 0/6] Rust goldfish_address_space driver (ioctl-only subset) Wenzhao Liao
2026-04-06 16:51   ` [RFC PATCH v3 1/6] uapi: add goldfish_address_space userspace ABI header Wenzhao Liao
2026-04-13 16:28     ` Arnd Bergmann
2026-04-06 16:51   ` [RFC PATCH v3 2/6] rust: bindings: expose goldfish address-space headers Wenzhao Liao
2026-04-06 16:51   ` [RFC PATCH v3 3/6] rust: page: add helpers for page-backed ping state Wenzhao Liao
2026-04-06 16:51   ` [RFC PATCH v3 4/6] rust: pci: add shared BAR memremap support Wenzhao Liao
2026-04-06 16:51   ` [RFC PATCH v3 5/6] rust: miscdevice: harden registration and safe file_operations invariants Wenzhao Liao
2026-04-06 16:51   ` [RFC PATCH v3 6/6] platform/goldfish: add Rust goldfish_address_space driver Wenzhao Liao

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox