[PATCH v3 0/2] PSCI system off and reset for KVM ARM/ARM64

[Rob Herring]

On Tue, Jan 7, 2014 at 5:50 AM, Mark Rutland <mark.rutland at arm.com> wrote:
> On Wed, Dec 18, 2013 at 06:10:27PM +0000, Marc Zyngier wrote:
>> Hi Rob,
>>
>> On Wed, Dec 18 2013 at 03:42:09 PM, Rob Herring <robherring2 at gmail.com> wrote:
>> > Adding Mark Rutland.
>> >
>> > On 12/18/2013 08:38 AM, Marc Zyngier wrote:
>> >> Christoffer Dall <christoffer.dall at linaro.org> writes:
>> >>
>> >>> On Tue, Dec 17, 2013 at 05:05:34PM +0530, Anup Patel wrote:
>> >>>> The Power State and Coordination Interface (PSCI) specification defines
>> >>>> SYSTEM_OFF and SYSTEM_RESET functions for system poweroff and reboot.
>> >>>>
>> >>>> This patchset adds emulation of PSCI SYSTEM_OFF and SYSTEM_RESET functions
>> >>>> in KVM ARM/ARM64 by forwarding them to user space (QEMU or KVMTOOL) using
>> >>>> KVM_EXIT_SYSTEM_EVENT exit reason.
>> >>>>
>> >>>> To try this patch from guest kernel, we will need PSCI-based restart and
>> >>>> poweroff support in the guest kenel for both ARM and ARM64.
>> >>>>
>> >>>> Rob Herring has already submitted patches for PSCI-based restart and
>> >>>> poweroff in ARM kernel but these are not merged yet due unstable device
>> >>>> tree bindings of kernel PSCI support. We will be having similar patches
>> >>>> for PSCI-based restart and poweroff in ARM64 kernel.
>> >>>> (Refer http://www.spinics.net/lists/arm-kernel/msg262217.html)
>> >>>> (Refer http://www.spinics.net/lists/devicetree/msg05348.html)
>> >>>
>> >>> Reviewed-by: Christoffer Dall <christoffer.dall at linaro.org>
>> >>>
>> >>> I can merge this series if Marc acks it as well.
>> >>
>> >> The patches themselves are mostly fine. One issue though: They implement
>> >> part of the v0.2 spec, but keep on using the range of function IDs that
>> >> we made up for v0.1.
>> >>
>> >> I just had a chat with the person responsible for the spec, and realized
>> >> that the Function IDs mentionned in the v0.2 spec are not optional, and
>> >> not using them would be in direct violation of the spec (the new numbers
>> >> now come directly from the SMC calling convention).
>> >
>> > News to me. That is exactly the opposite of what Mark Rutland told me.
>> > This would certainly simplify things since the SMC calling convention
>> > IDs encode the size and there would be no reason to put the IDs into
>> > DT.
>
> Hi Rob,
>
> Sorry for the confusion here. My position changed over the course of the
> discussion, so I'm probably arguing against my original side now.
>
> I think the key point was that I wanted a PSCI 0.2 system to function
> with an existing kernel if possible, as all the existing calls still
> exist. That implied placing the numbers (redundantly) into the DT. This
> is important for extending KVM with PSCI 0.2 support without breaking
> support for existing guests and having to have a load of messy flags
> depending on the guest you want to boot.
>
> I agree that we should use the specified function IDs, and therefore we
> do not need to specify the IDs. A new system that old kernels can't run
> on anyway can just have:
>
> psci {
>         compatible = "arm,psci-0.2";
>         methoc = "smc";
> };
>
> However, it would be nice to have a set of function IDs in the DT for
> existing clients as a fallback. Preferably in the same node (we have the
> compatible list, we may as well use it and make it clear we expect
> clients to use one or the other):
>
> psci {
>         compatible = "arm,psci-0.2", "arm,psci";
>         method = "smc";
>
>         /*
>          * The mandated PSCI 0.2 IDs are used by the client if it
>          * understands PSCI 0.2. These IDs are for older clients that
>          * only support the kernel's old PSCI version.
>          *
>          * These IDs might not match any of the PSCI 0.2 IDs, and are
>          * purely here for compatibility with existing software.
>          */
>         cpu_off = <...>;
>         cpu_on = <...>;
>         cpu_suspend = <...>;
> };
>
> I believe KVM currently uses the same ID numbers regardless of guest
> register-width, which is unfortunately different from what PSCI 0.2 says
> it should. As KVM can figure out the register width of the caller it
> might be able to do the right thing (unless that's in conflict with PSCI
> 0.2). At worst we should be able to allocate a third set of
> width-agnostic IDs for older clients that imply the current behaviour.
>
> Does that sound reasonable? Sorry for leading you in circles.

Yes. Certainly the simplest solution is the best.

>>
>> Mark (assuming you're reading this): what were your objections about
>> following the ID mentioned in the spec?
>
> Originally I was worried about more spectacularly bad implementations
> with wrong IDs or a subset of IDs supported. Now I agree that we can
> handle those later if and when they arise, and using the correct IDs is
> a far better option.
>
> The other issue was KVM forwards-compatibility, but I think the above
> covers that.
>
>>
>> >> So I rekon we need to create a separate range for those. Also, I'd like
>> >> to progress the DT and kernel side of things as well (otherwise this is
>> >> all a bit pointless).
>> >>
>> >> Rob: what are your plans regarding your PSCI v0.2 patches?
>> >
>> > My plan was to simply add 2 optional properties for reset/off and be
>> > done with it like is done here. I'll leave it to ARM to sort out all of
>> > v0.2 ID and 32-bit vs. 64-bit issues.
>
> Does the above sound OK for adding that support?

Yes, but this doesn't actually work for highbank/midway without a
firmware update because they are using IDs from a pre-release of the
v0.2 spec. Fortunately, they do not claim v0.2 compatibility, so we
should be okay. It is only a question of removing the highbank
reset/poweroff code from the kernel and I guess that will just not
happen now.

Rob