Re: [PATCH 0/6] KVM: arm64: Implement PSCI SYSTEM_SUSPEND support

From: Marc Zyngier <maz@kernel.org>
To: Oliver Upton <oupton@google.com>
Cc: kvm@vger.kernel.org, kvmarm@lists.cs.columbia.edu,
	Peter Shier <pshier@google.com>,
	Ricardo Koller <ricarkol@google.com>,
	Jing Zhang <jingzhangos@google.com>,
	Raghavendra Rao Anata <rananta@google.com>,
	James Morse <james.morse@arm.com>,
	Alexandru Elisei <Alexandru.Elisei@arm.com>,
	Suzuki K Poulose <suzuki.poulose@arm.com>,
	Andrew Jones <drjones@redhat.com>
Subject: Re: [PATCH 0/6] KVM: arm64: Implement PSCI SYSTEM_SUSPEND support
Date: Tue, 21 Sep 2021 10:45:22 +0100	[thread overview]
Message-ID: <87k0jauurx.wl-maz@kernel.org> (raw)
In-Reply-To: <CAOQ_QshZe8ay03XqCo4DkM6zUaOuEoS5bRbrOy+FsuXaJ=YyKA@mail.gmail.com>

Hi Oliver,

On Tue, 07 Sep 2021 19:14:00 +0100,
Oliver Upton <oupton@google.com> wrote:
> 
> On Tue, Sep 7, 2021 at 12:43 PM Marc Zyngier <maz@kernel.org> 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?

These are fine. However, what of the GIC, for example? Can any GIC
interrupt wake-up the guest? I'm happy to say "yes" to this, but I
suspect others will have a different idea, and the thought of
introducing an IMPDEF wake-up interrupt controller doesn't fill me
with joy.

> > > > 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.

Ah! Good! :D

> 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.

This looks sensible. One question though: I think there is an implicit
requirement that the guest should be "migratable" in that state. How
does the above handles it? If the "suspend state" is solely held in
the kernel, we need to be able to snapshot it, and I don't like the
sound of that...

We could instead keep the "suspend state" in the VMM:

On PSCI_SUSPEND, the guest exits to userspace. If the VMM wants to
honour the supend request, it reenters the guest with RUN+SUSPEND,
which results in a WFI. On each wake-up, the guest exits to userspace,
and it is the VMM responsibility to either perform the wake-up (RUN)
or stay in suspend (RUN+SUSPEND).

This ensures that the guest never transitions out of suspend without
the VMM knowing, and the VMM can always force a resume by kicking the
thread back to userspace.

Thoughts?

> > > > 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.

I'm not sure that's the case. CPU_SUSPEND may not use the resume
entry-point if the suspend results is a shallower state than expected
(i.e. the call just returns instead of behaving like a CPU boot).

However, a successful SYSTEM_SUSPEND always results in the deepest
possible state. The guest should know that. There is also the fact
that performing a full clean to the PoC is going to be pretty
expensive, and I'd like to avoid that.

I'll try and reach out to some of the ARM folks for clarification on
the matter.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.