-
Notifications
You must be signed in to change notification settings - Fork 9
Add warm_boot_entry for suspend support #40
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: main
Are you sure you want to change the base?
Changes from 2 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 |
|---|---|---|
|
|
@@ -88,3 +88,64 @@ pub unsafe extern "C" fn secondary_entry(stack_end: *mut u64) -> ! { | |
| set_exception_vector = sym crate::set_exception_vector, | ||
| ) | ||
| } | ||
|
|
||
| /// An assembly entry point for warm boot (e.g. resume from suspend). | ||
| /// | ||
| /// It will enable the MMU, disable trapping of floating point instructions, | ||
| /// set up the exception vector, set the stack pointer to `stack_ptr`, | ||
| /// and then jump to `entry_point(arg)`. | ||
| /// | ||
| /// The function expects to be passed a pointer to a `SuspendContext` instance | ||
| /// that will be valid after returning from suspend. It should therefore be | ||
| /// a static, or allocated on the heap, to avoid being deallocated before resuming. | ||
| /// | ||
| /// This is a low-level function that should be used as the entry point when | ||
| /// manually calling the `CPU_SUSPEND` PSCI call. It deliberately doesn't store any | ||
| /// data itself so that the caller has maximum flexibility over things such as | ||
| /// where the `SuspendContext` is stored. If you need to restore any state (such | ||
| /// as the registers), or you want to emulate returning from a function after | ||
| /// suspending the CPU, you need to implement this functionality yourself. | ||
| /// | ||
| /// # Safety | ||
| /// | ||
| /// The caller guarantees that the `SuspendContext` instance passed to the function | ||
| /// will be valid and safe to read when the CPU resumes (especially important when | ||
| /// the data is wrapped in a mutex, for instance). | ||
|
||
| /// | ||
| /// The `context` parameter has the following restrictions on its fields: | ||
| /// * `stack_ptr` must be a valid stack pointer. | ||
| /// * `entry` must be a valid function pointer taking `SuspendContext<T>`. | ||
| #[unsafe(naked)] | ||
| pub unsafe extern "C" fn warm_boot_entry<T>(context: *const SuspendContext<T>) -> ! { | ||
| naked_asm!( | ||
| "mov x19, x0", | ||
| "bl enable_mmu", | ||
| // Disable trapping floating point access in EL1. | ||
| "mrs x30, cpacr_el1", | ||
| "orr x30, x30, #(0x3 << 20)", | ||
| "msr cpacr_el1, x30", | ||
| "isb", | ||
| // Load data from SuspendContext | ||
| "ldr x0, [x19, #{stack_ptr_offset}]", | ||
| "ldr x1, [x19, #{entry_offset}]", | ||
qwandor marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| // Set the exception vector. | ||
| "bl {set_exception_vector}", | ||
|
Collaborator
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. The stack pointer needs to be set before calling |
||
| // Set the stack pointer (x0). | ||
| "mov sp, x0", | ||
| // Jump to entry point (x1) with arg (x2). | ||
| "mov x0, x19", | ||
| "br x1", | ||
| set_exception_vector = sym crate::set_exception_vector, | ||
| stack_ptr_offset = const offset_of!(SuspendContext<T>, stack_ptr), | ||
| entry_offset = const offset_of!(SuspendContext<T>, entry), | ||
| ) | ||
| } | ||
|
|
||
| /// Data used by [`warm_boot_entry`] to restore the CPU state after resuming. | ||
| #[derive(Debug, Copy, Clone)] | ||
| #[repr(C)] | ||
| pub struct SuspendContext<T> { | ||
| pub stack_ptr: u64, | ||
| pub entry: extern "C" fn(&Self) -> !, | ||
|
||
| pub data: T, | ||
| } | ||
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.
It would be nice if we could put the
SuspendContexton the stack of the core being resumed. What about if we made thedatafield ofSuspendContexta pointer rather than a value? Then theSuspendContextitself could go on the stack, as it isn't needed once its contents is loaded into registers, and only the value behind thedatapointer would need to be allocated statically.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.
Alternatively, looking at how this is used in RITM, you could remove the
datafield fromSuspendContextentirely, and theentryfunction could just find its own data based on the MPIDR. (In the case of RITM,SUSPEND_CONTEXTScould store pairs ofSuspendContextandSuspendCoreData.)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.
Either way, I think it should work to always put the
entrypointer anddatapointer (if we keep it) on the stack of the core that is being resumed. That only requires that there is enough spare stack space to fit, and if the stack is so small or full that there isn't, then resuming isn't going to go very well anyway. The stack pointer itself doesn't need to go on the stack, as it can be passed directly as the context of the PSCI CPU_SUSPEND call. This would work with a wrapper function in this crate for CPU_SUSPEND to put them on the stack.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.
I've pushed a couple of commits implementing my suggestions here, see what you think.
I've changed
datato a u64 rather than an arbitrary type. This means thatSuspendContextcan always go on the stack of the core being resumed, so there's no need to worry about deallocating it. I realised that we do still need to keep the stack pointer there too, as depending on how you want to handle resuming it may be the bottom of the stack or somewhere near the top, and won't necessarily be a constant offset from theSuspendContext. This also maintains the ability to put theSuspendContextsomewhere else if you really want to.If you want to pass more data than a
u64then you can cast a pointer to it, in which case you can handle the deallocation yourself (if you need to). But I don't think you need this for RITM anyway.I've also added a
suspend_corefunction which constructs aSuspendContexton the stack and passes it to the PSCIcpu_suspend. I think this should work for RITM.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.
Thanks a lot for helping me with this! Yes, I think the current version is probably best of both worlds - still low level enough to be a good fit for RITM and easy to make it memory safe.