Add `upcall` for delegation by sangho2 · Pull Request #556 · microsoft/litebox

sangho2 · 2025-12-13T00:36:47Z

This PR adds the upcall trait to let platforms delegate handling of unknown
messages/requests (e.g., OP-TEE messages from the normal-world/VTL0
kernel) to other layers of LiteBox (i.e., runner or shim).

litebox/src/upcall.rs

github-actions · 2025-12-18T06:50:07Z

🤖 SemverChecks 🤖 No breaking API changes detected

Note: this does not mean API is unchanged, or even that there are no breaking changes; simply, none of the detections triggered.

praveen-pk · 2026-01-07T20:26:02Z

litebox_platform_lvbs/src/mshv/vsm_optee_smc.rs

+        // Placeholder for now
+        let upcall = crate::UPCALL.get().expect("OP-TEE upcall not registered");
+        let mut ctx = litebox_common_linux::PtRegs::default();
+        let _ = upcall.execute(&mut ctx);


Did you intend to ignore the return the value here?

Seems like this should be properly handled/propagated.

As wrote in the comment, this is just a placeholder. we should handle the return value.

Minor nit: keeping a TODO:/XXX:/... or such signifier makes it easier to grep in the future quickly

Didn't put TODO here because the actual implementation already exists in another PR.
https://github.com/microsoft/litebox/pull/564/changes#diff-3eeb171b29f4643a455abd0bf670819afc33087f009eb4f6300fff0364f19d6bR127

praveen-pk · 2026-01-07T20:48:31Z

litebox_runner_lvbs/src/lib.rs

+    type Parameter = litebox_common_linux::PtRegs;
+    type Return = litebox_common_linux::PtRegs;


Curious to know why you need the registers here?

Do you intend to run a thread during upcall.execute?

This is just for defining parameter and return types. We don't specify any particular data types here yet. The reason I use PtRegs here is because it is the most common data type used in many places in the LiteBox code base.

wdcui · 2026-01-17T00:39:25Z

@jaybosamiya-ms It would be good to have you take a look at this PR since it changes litebox.

jaybosamiya-ms

Minor comments throughout, but I have a more general comment about the design/idea itself: this is roughly equivalent to the functionality that already exists in litebox::shim::... right? This feels like adding a parallel way of handling roughly similar things, which seems suboptimal. Until we can clearly articulate when that should be used and when this should be used, I think this PR should not be merged, which is why I am marking this as "Request changes".

I am happy to discuss this IRL more if it would help tweak the design of the existing litebox::shim::... to support the use case (or maybe it already can support the use case?). Currently, this PR is only introducing the new functionality without the use case, which makes it a little hard to understand why the required use case cannot be handled by existing litebox::shim::EnterShim and related bits.

CC: @jstarks I expect you'll have some useful insights on this PR

jaybosamiya-ms · 2026-01-17T01:02:50Z

litebox/src/upcall.rs

+//! Examples of such messages or requests include HVCI/Heki requests from
+//! VTL0 and OP-TEE SMC calls from the normal world.


Would signals from the outside world be a similar situation (i.e., should they be handled through this mechanism)?

Unlike the Linux shim case that all signals will be handled by the shim, certain exceptions/interrupts will be handled by the LVBS platform whereas some other ones are handled by the OP-TEE shim.

That's fine right? The platform can just decide not to forward along things to the shim. There is nothing in the design that says that the platform must forward everything over, right? Maybe I am misunderstanding why the signals world and the upcall world are drastically different. Might be easier to chat in-person?

Yes, I think the current explanation is misleading. It is more about there are something the platform doesn't know how to handle and delegates them to the others if applicable.

jaybosamiya-ms · 2026-01-17T01:05:11Z

litebox/src/upcall.rs

+//! calls within LiteBox. However, care must be taken to ensure that the upcall's
+//! parameters and return values are properly validated and sanitized to prevent
+//! potential security vulnerabilities. This is because the parameters might be


"care must be taken" -> by whom?

the upcall handler. will clarify this.

jaybosamiya-ms · 2026-01-17T01:07:14Z

litebox/src/upcall.rs

+//! does not have semantics to validate them). We can specify a function for early
+//! validation at the platform side if needed but its advantages are not clear at
+//! this moment (since there is no costly context switch within LiteBox).


Nit: this sentence is surprising to see as part of the doc string. Might be better just as a comment, rather than being in the doc string.

jaybosamiya-ms · 2026-01-17T01:10:49Z

litebox/src/upcall.rs

+    /// Initialize the upcall handler. Must be called by the platform exactly once.
+    /// Per-thread initialization is possible but all threads must share the same
+    /// upcall handler.


This comment is confusing. What happens if the platform does not invoke this? (I might have missed something but I didn't see the platform invoke this here). I also don't understand the "per-thread initialization is possible but ..." comment here. Should each thread invoke this? Or should different threads within the platform coordinate to make sure they have not invoked this more than once?

A slightly easier-to-use interface is likely something like "get singleton", so that the platform doesn't need to store/manage this, and the runner side can choose to make it more/less performant as it needs. Alternatively, one can just eliminate initialization and set up the execute to initialize on first use?

Basically, this is a callback registration. I agree that the comment is confusing. I'll fix this.

jaybosamiya-ms · 2026-01-17T01:12:29Z

litebox/src/upcall.rs

+    fn init(
+        &self,
+    ) -> alloc::boxed::Box<
+        dyn crate::upcall::Upcall<Parameter = Self::Parameter, Return = Self::Return>,
+    >;


You need self to get essentially a Box<dyn itself>? This is a confusing signature. Where do you get teh original Upcall to even invoke this initialization? Why is it returning a boxed version of itself?

the runner has a function and registers it to the platform through this (Platform::register_upcall).

jaybosamiya-ms · 2026-01-17T01:16:22Z

litebox/src/upcall.rs

+    /// platform validates the parameters, the implementation of `execute` must validate
+    /// parameters to avoid potential security vulnerabilities. Also, it must sanitize
+    /// the return values before returning them to the platform.
+    fn execute(&self, ctx: &mut Self::Parameter) -> Result<Self::Return, UpcallError>;


I'm mildly surprised by seeing the &mut here for the parameter. Due to lifetime elision and such, Self::Return's lifetime cannot depend on that mutable reference anyways. A slightly cleaner design might be to just pass the parameter as a (consumed/owned) value? Alternatively, the Return should have a generic lifetime that gets bound to the lifetime of the parameter (or even more generally, it would get bound to a new lifetime that is itself bounded by the lifetimes of &self and ctx).

Yes. Technically, we don't need to struggle with lifetime. let me use values.

jaybosamiya-ms · 2026-01-17T01:18:03Z

litebox/src/upcall.rs

+    #[error("Upcall failed")]
+    Failure,


Failed due to? It becomes impossible in the current design for the upcall to give more useful information to the place below. Would be good to make this generic in some way (can do it generically, or can do a Failure(Box<dyn Error>) might be enough).

Let me do that.

jaybosamiya-ms · 2026-01-17T01:19:28Z

litebox/src/upcall.rs

+    #[error("Upcall needs to be retried")]
+    Retry,


If the upcall needs to be retried, it needs to mention when it needs to be retried, right? Otherwise, if "retry immediately" is allowed, might as well do the retrying internally. The failure case is that "it cannot be handled right now, please try later", but yeah, something else feels necessary here in the output so that a platform can know when to retry.

Yes. Technically, there is no easy way to know when the platform can try it again. Let me think.

jaybosamiya-ms · 2026-01-17T01:20:31Z

litebox_platform_lvbs/src/mshv/vsm_optee_smc.rs

+        // Placeholder for now
+        let upcall = crate::UPCALL.get().expect("OP-TEE upcall not registered");
+        let mut ctx = litebox_common_linux::PtRegs::default();
+        let _ = upcall.execute(&mut ctx);


Minor nit: keeping a TODO:/XXX:/... or such signifier makes it easier to grep in the future quickly

jaybosamiya-ms · 2026-01-17T01:21:57Z

litebox_platform_lvbs/src/lib.rs

+        upcall: &'static (
+                     dyn litebox::upcall::Upcall<
+            Parameter = litebox_common_linux::PtRegs,
+            Return = litebox_common_linux::PtRegs,
+        > + Send
+                         + Sync
+                 ),


oof, this looks like rustfmt did a horrifyingly bad job here. Consider putting the type into a type alias (type Upcall = ...) to make this be nicer maintained?

sangho2 · 2026-01-17T02:30:17Z

Minor comments throughout, but I have a more general comment about the design/idea itself: this is roughly equivalent to the functionality that already exists in litebox::shim::... right? This feels like adding a parallel way of handling roughly similar things, which seems suboptimal. Until we can clearly articulate when that should be used and when this should be used, I think this PR should not be merged, which is why I am marking this as "Request changes".

I am happy to discuss this IRL more if it would help tweak the design of the existing litebox::shim::... to support the use case (or maybe it already can support the use case?). Currently, this PR is only introducing the new functionality without the use case, which makes it a little hard to understand why the required use case cannot be handled by existing litebox::shim::EnterShim and related bits.

CC: @jstarks I expect you'll have some useful insights on this PR

We can definitely discuss this later. This design is based on some discussion between John, Weidong, and me. Long story short, we need a runner or something below the shim to support this stuff. For example, two of this upcall's main usages are loading ELF and invoking run_thread that shim cannot do, like https://github.com/microsoft/litebox/pull/564/changes#diff-3eeb171b29f4643a455abd0bf670819afc33087f009eb4f6300fff0364f19d6bR127

Anyhow, what we need is there is a function in the runner which should be invoked by the platform through a simple interface.

sangho2 · 2026-01-21T00:02:48Z

Discussion result: Instead of defining a new interface, we will add one function to EnterShim to support this feature.

sangho2 · 2026-01-23T21:30:21Z

Turns that using EnterShim is not straightforward or may be not possible. The reason is that our current implementation passes shim to the platform via run_thread (we no longer explicitly register a shim to the platform). Perhaps we need to use litebox_common_optee for now (sounds like litebox_common_* are becoming a collection of second-class providers).

sangho2 · 2026-01-28T19:10:53Z

We decided to move vtl_switch_loop and its sub functions to litebox_runner_lvbs. To this end, we no longer need upcall.

upcall trait and impl placeholder

556e1a1

sangho2 changed the title ~~Add an upcall trait to handle OP-TEE messages~~ Add an upcall trait to delegate handling of OP-TEE messages Dec 13, 2025

sangho2 changed the title ~~Add an upcall trait to delegate handling of OP-TEE messages~~ Add an upcall trait for delegation Dec 13, 2025

sangho2 marked this pull request as ready for review December 13, 2025 00:50

wdcui requested a review from jstarks December 13, 2025 01:20

wdcui reviewed Dec 13, 2025

View reviewed changes

litebox/src/upcall.rs Outdated Show resolved Hide resolved

sangho2 changed the title ~~Add an upcall trait for delegation~~ Add upcall for delegation Dec 13, 2025

Sangho Lee added 2 commits December 14, 2025 02:38

fix comment. separate param and ret types

eabf6a6

revise comments

57f874b

sangho2 force-pushed the sanghle/upcall branch from e20000a to 57f874b Compare December 18, 2025 06:40

sangho2 mentioned this pull request Jan 7, 2026

Add run_thread to LVBS platform and revise its syscall handling #566

Merged

praveen-pk reviewed Jan 7, 2026

View reviewed changes

praveen-pk approved these changes Jan 7, 2026

View reviewed changes

wdcui requested a review from jaybosamiya-ms January 17, 2026 00:38

jaybosamiya-ms requested changes Jan 17, 2026

View reviewed changes

sangho2 marked this pull request as draft January 21, 2026 00:01

sangho2 closed this Jan 28, 2026

sangho2 deleted the sanghle/upcall branch February 6, 2026 04:42

		type Parameter = litebox_common_linux::PtRegs;
		type Return = litebox_common_linux::PtRegs;

		//! Examples of such messages or requests include HVCI/Heki requests from
		//! VTL0 and OP-TEE SMC calls from the normal world.

Conversation

sangho2 commented Dec 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Dec 18, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sangho2 Jan 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wdcui commented Jan 17, 2026

Uh oh!

jaybosamiya-ms left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sangho2 Jan 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sangho2 Jan 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sangho2 commented Jan 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sangho2 commented Jan 21, 2026

Uh oh!

sangho2 commented Jan 23, 2026

Uh oh!

sangho2 commented Jan 28, 2026

Uh oh!

Reviewers

Assignees

sangho2 commented Dec 13, 2025 •

edited

Loading

sangho2 Jan 7, 2026 •

edited

Loading

jaybosamiya-ms left a comment •

edited

Loading

sangho2 Jan 17, 2026 •

edited

Loading

sangho2 Jan 17, 2026 •

edited

Loading

sangho2 commented Jan 17, 2026 •

edited

Loading