On Tue, Sep 7, 2021 at 12:43 PM Marc Zyngier <maz@xxxxxxxxxx> wrote: > > > Although the definition of SYSTEM_SUSPEND is very simple from a PSCI > > > perspective, I don't think it is that simple at the system level, > > > because PSCI is only concerned with the CPU. > > > > > > For example, what is a wake-up event? My first approach would be to > > > consider interrupts to be such events. However, this approach suffers > > > from at least two issues: > > > > > > - How do you define which interrupts are actual wake-up events? > > > Nothing in the GIC architecture defines what a wake-up is (let alone > > > a wake-up event). > > > > Good point. > > > > One possible implementation of suspend could just be a `WFI` in a > > higher EL. In this case, KVM could emulate WFI wake up events > > according to D1.16.2 in DDI 0487G.a. But I agree, it isn't entirely > > clear what constitutes a wakeup from powered down state. > > It isn't, and it is actually IMPDEF (there isn't much in the ARM ARM > in terms of what constitutes a low power state). And even if you > wanted to emulate a WFI in userspace, the problem of interrupts that > have their source in the kernel remains. How to you tell userspace > that such an event has occurred if the vcpu thread isn't in the > kernel? Well, are there any objections to saying for the KVM implementation we observe the WFI wake-up events per the cited section of the ARM ARM? > > > It looks to me that your implementation can only work with userspace > > > provided events, which is pretty limited. > > > > Right. I implemented this from the mindset that userspace may do > > something heavyweight when a guest suspends, like save it to a > > persistent store to resume later on. No matter what we do in KVM, I > > think it's probably best to give userspace the right of first refusal > > to handle the suspension. > > Maybe. But if you want to handle wake-up from interrupts to actually > work, you must return to the kernel for the wake-up to occurs. > > The problem is that you piggyback on an existing feature (suspend) to > implement something else (opportunistic save/restore?). Oddly enough > the stars don't exactly align! ;-) > > I have the feeling that a solution to this problem would be to exit to > userspace with something indicating an *intent* to suspend. At this > stage, userspace can do two things: > > - resume the guest: the guest may have been moved to some other > machine, cold storage, whatever... The important thing is that the > guest is directly runnable without any extra event > > - confirm the suspension by returning to the kernel, which will > execute a blocking WFI on behalf of the guest > > With this, you end-up with something that is works from an interrupt > perspective (even for directly injected interrupts), and you can save > your guest on suspend. This is exactly what I was trying to get at with my last mail, although not quite as eloquently stated. So I completely agree. Just to check understanding for v2: We agree that an exit to userspace is fine so it has the opportunity to do something crazy when the guest attempts a suspend. If a VMM does nothing and immediately re-enters the kernel, we emulate the suspend there by waiting for some event to fire, which for our purposes we will say is an interrupt originating from userspace or the kernel (WFI). In all, the SUSPEND exit type does not indicate that emulation terminates with the VMM. It only indicates we are about to block in the kernel. If there is some IMPDEF event specific to the VMM, it should signal the vCPU thread to kick it out of the kernel, make it runnable, and re-enter. No need to do anything special from the kernel perspective for this. This is only for the case where we decide to block in the kernel. > > > > > > Other items worth considering: ongoing DMA, state of the caches at > > > suspend time, device state in general All of this really needs to be > > > defined before we can move forward with this feature. > > > > I believe it is largely up to the caller to get devices in a quiesced > > state appropriate for a system suspend, but PSCI is delightfully vague > > on this topic. > > Indeed, it only deals with the CPU. Oh look, another opportunity to > write a new spec! :) > > > On the contrary, it is up to KVM's implementation to > > guarantee caches are clean when servicing the guest request. > > This last point is pretty unclear to me. If the guest doesn't clean to > the PoC (or even to one of the PoPs) when it calls into suspend, > that's a clear indication that it doesn't care about its data. Why > should KVM be more conservative here? It shouldn't be in the business > of working around guest bugs. PSCI is vague on this, sadly. DEN0022D.b, 5.4.8 "Implementation responsibilities: Cache and coherency management states" that for CPU_SUSPEND, the PSCI implementation must perform a cache clean operation before entering the powerdown state. I don't see any reason why SYSTEM_SUSPEND should be excluded from this requirement. -- Thanks, Oliver
