Remove platform singletons from the LVBS platform and OP-TEE shim - #1127
Remove platform singletons from the LVBS platform and OP-TEE shim#1127Sangho Lee (sangho2) wants to merge 2 commits into
Conversation
c3d9103 to
e7364ba
Compare
|
A sequel of #1056 |
e7364ba to
00bf3ef
Compare
00bf3ef to
4273ff1
Compare
|
This is rather a large PR. would be nice if this can be broken down to smaller commits. Overall, the PR LGTM! |
|
Jay Bosamiya (Microsoft) (@jaybosamiya-ms) , can you help review this PR? Thanks! |
|
Sangho Lee (@sangho2) , the agent found the following issue: Medium — OpteeShim unintentionally loses Clone — litebox_shim_optee/src/lib.rs:292 . #[derive(Clone)] adds Platform: Clone , but neither platform implements it. Replace the derive with a manual Clone impl that clones the inner Arc . |
4273ff1 to
4543b4b
Compare
|
🤖 SemverChecks 🤖 Click for details |
Jay Bosamiya (Microsoft) (jaybosamiya-ms)
left a comment
There was a problem hiding this comment.
Thanks Sangho for getting rid of the final bits of the multiplex! The PR generally looks good to me. Most of it is fairly mechanical, so I haven't reviewed every line, but have done a look through for any bits that stood out to me, and have pointed out a few very minor nits. If there are any parts that you think I should specifically take a closer look at or sanity check, let me know.
| unsafe { crate::platform_low().vmap_privileged(pages, perms) } | ||
| unsafe { self.0.vmap_privileged(pages, perms) } |
There was a problem hiding this comment.
Nit: do we want to name it as self.platform_low for consistency/readability? Or just self.platform? It is possible .0 good enough. No response needed here, feel free to mark as resolved either way.
There was a problem hiding this comment.
yes, self.0 sometimes looks bad. self.platform might work well. Let me figure out better option.
| // NOTE: the session manager singleton lives in the composition root (each | ||
| // runner), not here. A `static` cannot name a generic parameter, and a shim | ||
| // instance is built per session, so the shim has nowhere to put it. The runner | ||
| // knows its concrete platform, so it can hold the `static` and hand out | ||
| // `&'static SessionManager<ConcretePlatform>`. |
There was a problem hiding this comment.
Nit: this feels like a comment left in by an agent removing code leaving breadcrumbs; not sure it needs to stay?
There was a problem hiding this comment.
Good catch. Let me go over all comments again.
This PR removes platform singletons from the LVBS platform and OP-TEE shim crates, allowing us to drop
litebox_platform_multiplexcompletely. Refactor both crates at once because they relied on the sameGlobalVmapManagerwhich is now removed as well.