[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v4][PART 1 2/4] xen/arm: Implement PSCI SYSTEM_SUSPEND call for guests
Hi, @Julien Grall On Wed, Jun 4, 2025 at 2:00 AM Julien Grall <julien@xxxxxxx> wrote: > > Hi Mykola, > > On 27/05/2025 10:18, Mykola Kvach wrote: > > From: Mykola Kvach <mykola_kvach@xxxxxxxx> > > > > This patch adds support for the PSCI SYSTEM_SUSPEND function in the vPSCI > > (virtual PSCI) interface, allowing guests to request suspend via the PSCI > > v1.0 SYSTEM_SUSPEND call (both 32-bit and 64-bit variants). > > > > The implementation: > > - Adds SYSTEM_SUSPEND function IDs to PSCI definitions > > - Implements trapping and handling of SYSTEM_SUSPEND in vPSCI > > - Allows only non-hardware domains to invoke SYSTEM_SUSPEND; for the > > hardware domain, PSCI_NOT_SUPPORTED is returned to avoid halting the > > system in hwdom_shutdown() called from domain_shutdown > > - Ensures all secondary VCPUs of the calling domain are offline before > > allowing suspend due to PSCI spec > > - Treats suspend as a "standby" operation: the domain is shut down with > > SHUTDOWN_suspend, and resumes execution at the instruction following > > the call > > Looking at the specification, I am still not convinced you can implement > PSCI SUSPEND as a NOP. For instance, in the section "5.1.19 > SYSTEM_SUSPEND", the wording implies the call cannot return when it is > successul. > > I understand that 5.20.2 ("Caller reponsabilities" for SYSTEM_SUSPEND) > suggests the caller should apply all the rules from 5.4 ("Caller > responsabilties" for CPU_SUSPEND), but it is also mentioned that > SYSTEM_SUSPEND behave as the deepest power down state. > > So I don't think standby is an option. I would like an opinion from the > other maintainers. Sure, let's discuss this with the others. > > > +static int32_t do_psci_1_0_system_suspend(register_t epoint, register_t > > cid) > > +{> + struct vcpu *v; > > + struct domain *d = current->domain; > > + > > + /* Drop this check once SYSTEM_SUSPEND is supported in hardware domain > > */ > > + if ( is_hardware_domain(d) ) > > + return PSCI_NOT_SUPPORTED; > > + > > + /* Ensure that all CPUs other than the calling one are offline */ > > + for_each_vcpu ( d, v ) > > + { > > + if ( v != current && is_vcpu_online(v) ) > > I think this is racy because you can still turn on a vCPU afterwards > from a vCPU you haven't checked. > I'll think about how to protect against such cases. Thank you for pointing that out. > Did you add this check just to follow the specification, or is there any > other problem in Xen? Yes, it's just to comply with the specification — at least, I've never seen PSCI_DENIED triggered because of this check. It's a leftover from a previous patch series. > > > + return PSCI_DENIED; > > + }> + > > + /* > > + * System suspend requests are treated as performing standby > > + * as this simplifies Xen implementation. > > + * > > + * Arm Power State Coordination Interface (DEN0022F.b) > > This comment is a bit too verbose. There is no need to copy/paste the > specification. You can just write a couple of sentence with link to the > specification. Got it, I'll revise the comment accordingly. > > Cheers, > > -- > Julien Grall > Best regards, Mykola
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |