-
Notifications
You must be signed in to change notification settings - Fork 113
Add AddressSpaceProvider trait for multi-process address space management #758
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: wdcui/pr3f-raw-message
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,137 @@ | ||
| // Copyright (c) Microsoft Corporation. | ||
| // Licensed under the MIT license. | ||
|
|
||
| //! Address-space management types and traits for multi-process support. | ||
| //! | ||
| //! The [`AddressSpaceProvider`] trait is an **optional** South interface that | ||
| //! platforms implement to manage per-process address spaces. Platforms may use | ||
| //! separate page tables, VA-range partitioning, or other techniques to isolate | ||
| //! address spaces. | ||
|
|
||
| use core::ops::Range; | ||
| use thiserror::Error; | ||
|
|
||
| /// The result of forking an address space. | ||
| /// | ||
| /// The variant tells the caller what kind of copy was created so it can adjust | ||
| /// its behavior (e.g., whether to copy page contents or share them). | ||
| #[derive(Debug, Clone, Copy, PartialEq, Eq)] | ||
| pub enum ForkedAddressSpace<Id> { | ||
| /// Independent copy-on-write copy with the full address range. The child | ||
| /// has its own backing structures; CoW faults are resolved by the | ||
| /// platform. | ||
| Independent(Id), | ||
| /// A new VA-range partition is assigned to the child. Parent memory is | ||
| /// shared; the shim is responsible for copying pages as needed. | ||
| SharedWithParent(Id), | ||
| } | ||
|
|
||
| /// Errors that can occur during address-space operations. | ||
| #[derive(Error, Debug)] | ||
| #[non_exhaustive] | ||
| pub enum AddressSpaceError { | ||
| /// No free address-space slots or VA ranges available. | ||
| #[error("no address space slots available")] | ||
| NoSpace, | ||
| /// The given address-space ID is not valid (already destroyed, never | ||
| /// created, etc.). | ||
| #[error("invalid address space id")] | ||
| InvalidId, | ||
| /// The platform does not support this operation. | ||
| #[error("operation not supported by this platform")] | ||
| NotSupported, | ||
| } | ||
|
|
||
| /// A provider for managing per-process address spaces. | ||
| /// | ||
| /// This is an **optional** trait — platforms that do not yet support | ||
| /// multi-process may leave all methods at the default (which returns | ||
| /// [`AddressSpaceError::NotSupported`]). | ||
|
Comment on lines
+47
to
+49
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't like the design of a full trait filled with methods with |
||
| /// | ||
| /// # Associated Type | ||
| /// | ||
| /// `AddressSpaceId` is an opaque, lightweight handle that identifies one | ||
| /// address space. It must be `Copy + Eq + Send + Sync` so it can be stored | ||
| /// inside process contexts and passed across threads. | ||
|
Comment on lines
+51
to
+55
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Doc strings for associated types should be at the associated type, not at the top level of the trait |
||
| pub trait AddressSpaceProvider { | ||
| /// Opaque identifier for an address space. | ||
| type AddressSpaceId: Copy + Eq + Send + Sync + core::fmt::Debug; | ||
|
Comment on lines
+57
to
+58
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is in conflict with #759 which needs a concrete I am also not convinced that |
||
|
|
||
| /// Create a new, empty address space. | ||
| /// | ||
| /// The platform allocates whatever backing structures are needed for the | ||
| /// new address space. | ||
| fn create_address_space(&self) -> Result<Self::AddressSpaceId, AddressSpaceError> { | ||
| Err(AddressSpaceError::NotSupported) | ||
| } | ||
|
|
||
| /// Destroy an address space, releasing all associated resources. | ||
| /// | ||
| /// After this call, `id` is invalid and must not be reused. | ||
| fn destroy_address_space(&self, id: Self::AddressSpaceId) -> Result<(), AddressSpaceError> { | ||
| let _ = id; | ||
| Err(AddressSpaceError::NotSupported) | ||
| } | ||
|
Comment on lines
+69
to
+74
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If |
||
|
|
||
| /// Fork an address space from `parent`. | ||
| /// | ||
| /// Returns a [`ForkedAddressSpace`] indicating what kind of fork was | ||
| /// performed: | ||
| /// | ||
| /// * [`Independent`](ForkedAddressSpace::Independent) — full CoW copy. | ||
| /// * [`SharedWithParent`](ForkedAddressSpace::SharedWithParent) — new VA | ||
| /// partition, parent pages shared. | ||
|
Comment on lines
+78
to
+83
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No point re-documenting what is documented at the enum. This leads to long-term drift if things change. |
||
| fn fork_address_space( | ||
| &self, | ||
| parent: Self::AddressSpaceId, | ||
| ) -> Result<ForkedAddressSpace<Self::AddressSpaceId>, AddressSpaceError> { | ||
| let _ = parent; | ||
| Err(AddressSpaceError::NotSupported) | ||
| } | ||
|
|
||
| /// Make `id` the active address space for the current CPU / thread. | ||
| fn activate_address_space(&self, id: Self::AddressSpaceId) -> Result<(), AddressSpaceError> { | ||
|
Comment on lines
+86
to
+93
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As part of the "linear" move, functions like this would take a ref to the addressspaceid instead of the object |
||
| let _ = id; | ||
| Err(AddressSpaceError::NotSupported) | ||
| } | ||
|
|
||
| /// Execute `f` with the given address space active, then restore the | ||
| /// previously active address space. | ||
| /// | ||
| /// Implementations **must** restore the prior address space even if `f` | ||
| /// panics (use a guard / RAII pattern). | ||
| /// | ||
| /// The default returns [`AddressSpaceError::NotSupported`]. Platforms that | ||
| /// implement [`activate_address_space`](Self::activate_address_space) should | ||
| /// also override this method with a proper save/restore sequence. | ||
| fn with_address_space<R>( | ||
| &self, | ||
| id: Self::AddressSpaceId, | ||
| f: impl FnOnce() -> R, | ||
| ) -> Result<R, AddressSpaceError> { | ||
| let _ = (id, f); | ||
| Err(AddressSpaceError::NotSupported) | ||
| } | ||
|
Comment on lines
+98
to
+114
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As I'd spent time reviewing on the non-split #743 (comment):
|
||
|
|
||
| /// Whether the platform requires eager copy-on-write snapshots during | ||
| /// fork instead of lazy page-fault-driven CoW. | ||
| /// | ||
| /// When `true`, the shim eagerly copies all writable guest pages before | ||
| /// spawning the forked child and restores them after the child execs or | ||
| /// exits. When `false` (the default), the shim marks writable pages | ||
| /// read-only and lazily snapshots individual pages on first write fault. | ||
| /// | ||
| /// Platforms where the exception/fault handler shares the guest address | ||
| /// space must set this to `true` because a CoW fault inside the handler | ||
| /// itself would be fatal. | ||
| const EAGER_COW_ON_FORK: bool = false; | ||
|
Comment on lines
+116
to
+127
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure this should be a shim-level task, right? Also, generally speaking, bool-based config flags are often a smell of bad abstraction. In this case, as far as I can tell, the issue is wrt exception/fault handler: why can't the platform just do an eager copy of the exception/fault handler page, and then everything can just work through normal CoW faults, right? That would also lead to better performance than forcing literally everything to become eager just to handle one page? The platform already should know at least a little bit about the exception handling behavior, because of our userpointer design (which we have not finished finalizing the fully nice version, but nonetheless, should already iirc have the necessary functionality exposable directly there). |
||
|
|
||
| /// Return the VA range available to the given address space. | ||
| fn address_space_range( | ||
| &self, | ||
| id: Self::AddressSpaceId, | ||
| ) -> Result<Range<usize>, AddressSpaceError> { | ||
| let _ = id; | ||
| Err(AddressSpaceError::NotSupported) | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -61,6 +61,12 @@ impl Provider for MockPlatform {} | |
|
|
||
| impl RawMessageProvider for MockPlatform {} | ||
|
|
||
| impl AddressSpaceProvider for MockPlatform { | ||
| // All methods default to `Err(NotSupported)`, which is correct for the | ||
| // mock platform (single-process only). | ||
| type AddressSpaceId = u32; | ||
| } | ||
|
Comment on lines
+64
to
+68
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If it is all unsupported, using |
||
|
|
||
| pub(crate) struct MockRawMutex { | ||
| inner: AtomicU32, | ||
| internal_state: std::sync::RwLock<MockRawMutexInternalState>, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,6 +7,7 @@ | |
| //! trait is merely a collection of subtraits that could be composed independently from various | ||
| //! other crates that implement them upon various types. | ||
|
|
||
| pub mod address_space; | ||
| pub mod common_providers; | ||
| pub mod page_mgmt; | ||
| pub mod trivial_providers; | ||
|
|
@@ -18,6 +19,7 @@ use either::Either; | |
| use thiserror::Error; | ||
| use zerocopy::{FromBytes, IntoBytes}; | ||
|
|
||
| pub use address_space::*; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Prefer not using wildcard imports |
||
| pub use page_mgmt::PageManagementProvider; | ||
|
|
||
| #[macro_export] | ||
|
|
@@ -48,6 +50,7 @@ pub trait Provider: | |
| + PunchthroughProvider | ||
| + DebugLogProvider | ||
| + RawPointerProvider | ||
| + AddressSpaceProvider | ||
| { | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -447,6 +447,10 @@ impl litebox::platform::Provider for LinuxUserland {} | |
|
|
||
| impl litebox::platform::RawMessageProvider for LinuxUserland {} | ||
|
|
||
| impl litebox::platform::AddressSpaceProvider for LinuxUserland { | ||
| type AddressSpaceId = u32; | ||
| } | ||
|
Comment on lines
+450
to
+452
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we not plan to support multi process on Linux userland? Why is there a comment on LVBS but not here, for example? Similarly for other platforms. |
||
|
|
||
| impl litebox::platform::SignalProvider for LinuxUserland { | ||
| type Signal = litebox_common_linux::signal::Signal; | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1353,6 +1353,12 @@ impl<Host: HostInterface> litebox::platform::SystemInfoProvider for LinuxKernel< | |
|
|
||
| impl<Host: HostInterface> litebox::platform::RawMessageProvider for LinuxKernel<Host> {} | ||
|
|
||
| impl<Host: HostInterface> litebox::platform::AddressSpaceProvider for LinuxKernel<Host> { | ||
| // All methods default to `Err(NotSupported)` — real implementation comes | ||
| // when LVBS multi-process (separate page tables) is added. | ||
| type AddressSpaceId = u32; | ||
| } | ||
|
Comment on lines
+1356
to
+1360
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If the real implementation comes later, then those bits should still be marked as |
||
|
|
||
| #[cfg(feature = "optee_syscall")] | ||
| /// Checks whether the given physical addresses are contiguous with respect to ALIGN. | ||
| fn is_contiguous<const ALIGN: usize>(addrs: &[PhysPageAddr<ALIGN>]) -> bool { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When are pages expected to be copied by the shim? Why not have the platform itself copy the pages over if so? Alternatively, even for "SharedWithParent", it should be possible to often set up some copy-on-first-access behavior by the platform, right?