On Tue, Jan 7, 2014 at 5:50 AM, Mark Rutland <mark.rutland@xxxxxxx> 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@xxxxxxxxx> wrote: >> > Adding Mark Rutland. >> > >> > On 12/18/2013 08:38 AM, Marc Zyngier wrote: >> >> Christoffer Dall <christoffer.dall@xxxxxxxxxx> 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@xxxxxxxxxx> >> >>> >> >>> 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 _______________________________________________ kvmarm mailing list kvmarm@xxxxxxxxxxxxxxxxxxxxx https://lists.cs.columbia.edu/cucslists/listinfo/kvmarm