On Thu, Nov 26, 2020 at 03:54:18PM +0000, David Brazdil wrote: > Add a handler of CPU_SUSPEND host PSCI SMCs. The SMC can either enter > a sleep state indistinguishable from a WFI or a deeper sleep state that > behaves like a CPU_OFF+CPU_ON except that the core is still considered > online when asleep. > > The handler saves r0,pc of the host and makes the same call to EL3 with > the hyp CPU entry point. It either returns back to the handler and then > back to the host, or wakes up into the entry point and initializes EL2 > state before dropping back to EL1. For those CPU_SUSPEND calls which lose context, is there no EL2 state that you need to save/restore, or is that all saved elsewhere already? The usual suspects are PMU, debug, and timers, so maybe not. It'd be nice to have a statement in the commit message if we're certain there's no state that we need to save. > A core can only suspend itself but other cores can concurrently invoke > CPU_ON with this core as target. To avoid racing them for the same > boot args struct, CPU_SUSPEND uses a different struct instance and entry > point. Each entry point selects the corresponding struct to restore host > boot args from. This avoids the need for locking in CPU_SUSPEND. I found this a bit confusing since the first sentence can be read to mean that CPU_ON is expected to compose with CPU_SUSPEND, whereas what this is actually saying is the implementation ensures they don't interact. How about: | CPU_ON and CPU_SUSPEND are both implemented using struct cpu_boot_args | to store the state upon powerup, with each CPU having separate structs | for CPU_ON and CPU_SUSPEND so that CPU_SUSPEND can operate locklessly | and so that a CPU_ON xall targetting a CPU cannot interfere with a | concurrent CPU_SUSPEND call on that CPU. The patch itself looks fine to me. Thanks, Mark.
