| Message ID | 1401041942-23471-1-git-send-email-christoffer.dall@linaro.org |
|---|---|
| State | New |
| Headers | show |
On Sun, May 25, 2014 at 08:18:59PM +0200, Christoffer Dall wrote: > diff --git a/arch/arm/include/asm/psci.h b/arch/arm/include/asm/psci.h > index c4ae171..b93e34a 100644 > --- a/arch/arm/include/asm/psci.h > +++ b/arch/arm/include/asm/psci.h > @@ -29,16 +29,19 @@ struct psci_operations { > int (*cpu_off)(struct psci_power_state state); > int (*cpu_on)(unsigned long cpuid, unsigned long entry_point); > int (*migrate)(unsigned long cpuid); > + int (*affinity_info)(unsigned long target_affinity, > + unsigned long lowest_affinity_level); > + int (*migrate_info_type)(void); > }; > > extern struct psci_operations psci_ops; > extern struct smp_operations psci_smp_ops; > > #ifdef CONFIG_ARM_PSCI > -void psci_init(void); > +int psci_init(void); > bool psci_smp_available(void); > #else > -static inline void psci_init(void) { } > +static inline int psci_init(void) { } The change introduces the following compile warning on imx_v6_v7_defconfig build. In file included from ../arch/arm/kernel/setup.c:40:0: ../arch/arm/include/asm/psci.h: In function ‘psci_init’: ../arch/arm/include/asm/psci.h:44:1: warning: no return statement in function returning non-void [-Wreturn-type] Shawn > static inline bool psci_smp_available(void) { return false; } > #endif
On Tue, May 27, 2014 at 03:44:55PM +0800, Shawn Guo wrote: > On Sun, May 25, 2014 at 08:18:59PM +0200, Christoffer Dall wrote: > > diff --git a/arch/arm/include/asm/psci.h b/arch/arm/include/asm/psci.h > > index c4ae171..b93e34a 100644 > > --- a/arch/arm/include/asm/psci.h > > +++ b/arch/arm/include/asm/psci.h > > @@ -29,16 +29,19 @@ struct psci_operations { > > int (*cpu_off)(struct psci_power_state state); > > int (*cpu_on)(unsigned long cpuid, unsigned long entry_point); > > int (*migrate)(unsigned long cpuid); > > + int (*affinity_info)(unsigned long target_affinity, > > + unsigned long lowest_affinity_level); > > + int (*migrate_info_type)(void); > > }; > > > > extern struct psci_operations psci_ops; > > extern struct smp_operations psci_smp_ops; > > > > #ifdef CONFIG_ARM_PSCI > > -void psci_init(void); > > +int psci_init(void); > > bool psci_smp_available(void); > > #else > > -static inline void psci_init(void) { } > > +static inline int psci_init(void) { } > > The change introduces the following compile warning on > imx_v6_v7_defconfig build. > > In file included from ../arch/arm/kernel/setup.c:40:0: > ../arch/arm/include/asm/psci.h: In function ‘psci_init’: > ../arch/arm/include/asm/psci.h:44:1: warning: no return statement in function returning non-void [-Wreturn-type] > Thanks for noticing, I just sent a fixup patch. -Christoffer
Il 27/05/2014 11:18, Christoffer Dall ha scritto: > On Tue, May 27, 2014 at 03:44:55PM +0800, Shawn Guo wrote: >> On Sun, May 25, 2014 at 08:18:59PM +0200, Christoffer Dall wrote: >>> diff --git a/arch/arm/include/asm/psci.h b/arch/arm/include/asm/psci.h >>> index c4ae171..b93e34a 100644 >>> --- a/arch/arm/include/asm/psci.h >>> +++ b/arch/arm/include/asm/psci.h >>> @@ -29,16 +29,19 @@ struct psci_operations { >>> int (*cpu_off)(struct psci_power_state state); >>> int (*cpu_on)(unsigned long cpuid, unsigned long entry_point); >>> int (*migrate)(unsigned long cpuid); >>> + int (*affinity_info)(unsigned long target_affinity, >>> + unsigned long lowest_affinity_level); >>> + int (*migrate_info_type)(void); >>> }; >>> >>> extern struct psci_operations psci_ops; >>> extern struct smp_operations psci_smp_ops; >>> >>> #ifdef CONFIG_ARM_PSCI >>> -void psci_init(void); >>> +int psci_init(void); >>> bool psci_smp_available(void); >>> #else >>> -static inline void psci_init(void) { } >>> +static inline int psci_init(void) { } >> >> The change introduces the following compile warning on >> imx_v6_v7_defconfig build. >> >> In file included from ../arch/arm/kernel/setup.c:40:0: >> ../arch/arm/include/asm/psci.h: In function ‘psci_init’: >> ../arch/arm/include/asm/psci.h:44:1: warning: no return statement in function returning non-void [-Wreturn-type] >> > Thanks for noticing, I just sent a fixup patch. Since this is not a KVM file, I'd rather get a new pull request. Paolo
On Tue, May 27, 2014 at 11:27:10AM +0200, Paolo Bonzini wrote: > Il 27/05/2014 11:18, Christoffer Dall ha scritto: > >On Tue, May 27, 2014 at 03:44:55PM +0800, Shawn Guo wrote: > >>On Sun, May 25, 2014 at 08:18:59PM +0200, Christoffer Dall wrote: > >>>diff --git a/arch/arm/include/asm/psci.h b/arch/arm/include/asm/psci.h > >>>index c4ae171..b93e34a 100644 > >>>--- a/arch/arm/include/asm/psci.h > >>>+++ b/arch/arm/include/asm/psci.h > >>>@@ -29,16 +29,19 @@ struct psci_operations { > >>> int (*cpu_off)(struct psci_power_state state); > >>> int (*cpu_on)(unsigned long cpuid, unsigned long entry_point); > >>> int (*migrate)(unsigned long cpuid); > >>>+ int (*affinity_info)(unsigned long target_affinity, > >>>+ unsigned long lowest_affinity_level); > >>>+ int (*migrate_info_type)(void); > >>> }; > >>> > >>> extern struct psci_operations psci_ops; > >>> extern struct smp_operations psci_smp_ops; > >>> > >>> #ifdef CONFIG_ARM_PSCI > >>>-void psci_init(void); > >>>+int psci_init(void); > >>> bool psci_smp_available(void); > >>> #else > >>>-static inline void psci_init(void) { } > >>>+static inline int psci_init(void) { } > >> > >>The change introduces the following compile warning on > >>imx_v6_v7_defconfig build. > >> > >>In file included from ../arch/arm/kernel/setup.c:40:0: > >>../arch/arm/include/asm/psci.h: In function ‘psci_init’: > >>../arch/arm/include/asm/psci.h:44:1: warning: no return statement in function returning non-void [-Wreturn-type] > >> > >Thanks for noticing, I just sent a fixup patch. > > Since this is not a KVM file, I'd rather get a new pull request. > Not sure I see why that makes a difference for a one-line fix patch, but ok. You want a new pull request with just this patch or a new pull request instead of the existing one containing all the PSCI patches + this fixup patch? -Christoffer
Il 27/05/2014 11:46, Christoffer Dall ha scritto: > Not sure I see why that makes a difference for a one-line fix patch, but > ok. > > You want a new pull request with just this patch or a new pull request > instead of the existing one containing all the PSCI patches + this fixup > patch? I prefer this patch squashed into the existing series. Paolo
On 25 May 2014 19:18, Christoffer Dall <christoffer.dall@linaro.org> wrote: > From: Anup Patel <anup.patel@linaro.org> > > Currently, we don't have an exit reason to notify user space about > a system-level event (for e.g. system reset or shutdown) triggered > by the VCPU. This patch adds exit reason KVM_EXIT_SYSTEM_EVENT for > this purpose. We can also inform user space about the 'type' and > architecture specific 'flags' of a system-level event using the > kvm_run structure. > > This newly added KVM_EXIT_SYSTEM_EVENT will be used by KVM ARM/ARM64 > in-kernel PSCI v0.2 support to reset/shutdown VMs. > --- a/Documentation/virtual/kvm/api.txt > +++ b/Documentation/virtual/kvm/api.txt > @@ -2740,6 +2740,21 @@ It gets triggered whenever both KVM_CAP_PPC_EPR are enabled and an > external interrupt has just been delivered into the guest. User space > should put the acknowledged interrupt vector into the 'epr' field. > > + /* KVM_EXIT_SYSTEM_EVENT */ > + struct { > +#define KVM_SYSTEM_EVENT_SHUTDOWN 1 > +#define KVM_SYSTEM_EVENT_RESET 2 > + __u32 type; > + __u64 flags; > + } system_event; > + > +If exit_reason is KVM_EXIT_SYSTEM_EVENT then the vcpu has triggered > +a system-level event using some architecture specific mechanism (hypercall > +or some special instruction). In case of ARM/ARM64, this is triggered using > +HVC instruction based PSCI call from the vcpu. The 'type' field describes > +the system-level event type. The 'flags' field describes architecture > +specific flags for the system-level event. Talking with Ard I realised that there's actually a hole in the specification of this new ABI. Did we intend these shutdown and reset exits to be: (1) requests from the guest for the shutdown/reset to be scheduled in the near future (and we'll continue to execute the guest until the shutdown actually happens) (2) requests for shutdown/reset right now, with no further guest instructions to be executed ? As currently implemented in QEMU we get behaviour (1), but I think the kernel PSCI implementation assumes behaviour (2). Who's right? thanks -- PMM
On Fri, Aug 29, 2014 at 06:39:09PM +0100, Peter Maydell wrote: > On 25 May 2014 19:18, Christoffer Dall <christoffer.dall@linaro.org> wrote: > > From: Anup Patel <anup.patel@linaro.org> > > > > Currently, we don't have an exit reason to notify user space about > > a system-level event (for e.g. system reset or shutdown) triggered > > by the VCPU. This patch adds exit reason KVM_EXIT_SYSTEM_EVENT for > > this purpose. We can also inform user space about the 'type' and > > architecture specific 'flags' of a system-level event using the > > kvm_run structure. > > > > This newly added KVM_EXIT_SYSTEM_EVENT will be used by KVM ARM/ARM64 > > in-kernel PSCI v0.2 support to reset/shutdown VMs. > > > --- a/Documentation/virtual/kvm/api.txt > > +++ b/Documentation/virtual/kvm/api.txt > > @@ -2740,6 +2740,21 @@ It gets triggered whenever both KVM_CAP_PPC_EPR are enabled and an > > external interrupt has just been delivered into the guest. User space > > should put the acknowledged interrupt vector into the 'epr' field. > > > > + /* KVM_EXIT_SYSTEM_EVENT */ > > + struct { > > +#define KVM_SYSTEM_EVENT_SHUTDOWN 1 > > +#define KVM_SYSTEM_EVENT_RESET 2 > > + __u32 type; > > + __u64 flags; > > + } system_event; > > + > > +If exit_reason is KVM_EXIT_SYSTEM_EVENT then the vcpu has triggered > > +a system-level event using some architecture specific mechanism (hypercall > > +or some special instruction). In case of ARM/ARM64, this is triggered using > > +HVC instruction based PSCI call from the vcpu. The 'type' field describes > > +the system-level event type. The 'flags' field describes architecture > > +specific flags for the system-level event. > > Talking with Ard I realised that there's actually a hole in the > specification of this new ABI. Did we intend these shutdown > and reset exits to be: > (1) requests from the guest for the shutdown/reset to be > scheduled in the near future (and we'll continue to execute > the guest until the shutdown actually happens) > (2) requests for shutdown/reset right now, with no further > guest instructions to be executed > > ? > > As currently implemented in QEMU we get behaviour (1), > but I think the kernel PSCI implementation assumes > behaviour (2). Who's right? > For the arm/arm64 use of this API (currently the only one?) the host would not break or anything like that if you keep executing the VM, but the guest will expect that no other instructions are executed after this call. The PSCI spec states that it's the responsibility of the PSCI implementation (here KVM), that "Implementation must ensure that all cores are in a known state with caches cleaned". I guess we don't need to worry about the latter, but we could handle the former by pausing all VCPUs prior to exiting with the SHUTDOWN system event. In that scenario, user space could choose to do either (1) or (2), but it gets a little fishy with a reset if we set the pause flag, because we would then at least need to specify in this ABI that this happens for ARM/ARM64 on reset. We could clarify this ABI to the fact that user space should not run any VCPUs after receiving this event, but the above change should probably be made anyhow, to make sure KVM implements PSCI as much as it can in the kernel? -Christoffer
On 1 September 2014 10:20, Christoffer Dall <christoffer.dall@linaro.org> wrote: > On Fri, Aug 29, 2014 at 06:39:09PM +0100, Peter Maydell wrote: >> Talking with Ard I realised that there's actually a hole in the >> specification of this new ABI. Did we intend these shutdown >> and reset exits to be: >> (1) requests from the guest for the shutdown/reset to be >> scheduled in the near future (and we'll continue to execute >> the guest until the shutdown actually happens) >> (2) requests for shutdown/reset right now, with no further >> guest instructions to be executed >> >> ? >> >> As currently implemented in QEMU we get behaviour (1), >> but I think the kernel PSCI implementation assumes >> behaviour (2). Who's right? >> > For the arm/arm64 use of this API (currently the only one?) the host > would not break or anything like that if you keep executing the VM, but > the guest will expect that no other instructions are executed after this > call. Well, if we do that then between QEMU and KVM we've violated the PSCI ABI we're supposed to provide, so somebody is wrong :-) I guess that since the kernel already implements "assume userspace won't resume the guest vcpu" the path of least resistance is to make userspace follow that. What does kvmtool do here (if it implements PSCI shutdown and reset at all)? thanks -- PMM
On Mon, Sep 01, 2014 at 10:30:17AM +0100, Peter Maydell wrote: > On 1 September 2014 10:20, Christoffer Dall <christoffer.dall@linaro.org> wrote: > > On Fri, Aug 29, 2014 at 06:39:09PM +0100, Peter Maydell wrote: > >> Talking with Ard I realised that there's actually a hole in the > >> specification of this new ABI. Did we intend these shutdown > >> and reset exits to be: > >> (1) requests from the guest for the shutdown/reset to be > >> scheduled in the near future (and we'll continue to execute > >> the guest until the shutdown actually happens) > >> (2) requests for shutdown/reset right now, with no further > >> guest instructions to be executed > >> > >> ? > >> > >> As currently implemented in QEMU we get behaviour (1), > >> but I think the kernel PSCI implementation assumes > >> behaviour (2). Who's right? > >> > > For the arm/arm64 use of this API (currently the only one?) the host > > would not break or anything like that if you keep executing the VM, but > > the guest will expect that no other instructions are executed after this > > call. > > Well, if we do that then between QEMU and KVM we've > violated the PSCI ABI we're supposed to provide, so somebody > is wrong :-) > > I guess that since the kernel already implements "assume > userspace won't resume the guest vcpu" the path of least > resistance is to make userspace follow that. The thing is that we're not exposing PSCI to user space, we're just exposing a system event, so it feels a bit weird to rely on user space's correct interpretation of a more generic API, to correctly implement PSCI in the kernel. On the other hand, user space can always break the guest as it sees fit... -Christoffer
On 1 September 2014 10:56, Christoffer Dall <christoffer.dall@linaro.org> wrote: > The thing is that we're not exposing PSCI to user space, we're just > exposing a system event, so it feels a bit weird to rely on user space's > correct interpretation of a more generic API, to correctly implement > PSCI in the kernel. Yeah; if somebody wants to argue that the other set of semantics make more sense considered purely as a KVM kernel-to-user API I have no objection. (QEMU's current "we'll do that at some point in the future" implementation follows the typical semantics for reset/shutdown triggered by a device register write I think: the write-to-device-register instruction will generally complete and CPU execution continue before the prodded device can get the system reset process done. But we don't necessarily need to be bound by that idea.) -- PMM
Hi Paolo and Gleb, The following changes since commit 198c74f43f0f5473f99967aead30ddc622804bc1: KVM: MMU: flush tlb out of mmu lock when write-protect the sptes (2014-04-23 17:49:52 -0300) are available in the git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/kvmarm/kvmarm.git tags/kvm-arm-for-3.16 for you to fetch changes up to 1252b3313642c3d0dff5b951b625468bf0dcd059: arm64: KVM: Enable minimalistic support for Cortex-A53 (2014-05-25 20:05:30 +0200) ---------------------------------------------------------------- Changed for the 3.16 merge window. This includes KVM support for PSCI v0.2 and also includes generic Linux support for PSCI v0.2 (on hosts that advertise that feature via their DT), since the latter depends on headers introduced by the former. Finally there's a small patch from Marc that enables Cortex-A53 support. ---------------------------------------------------------------- Anup Patel (12): KVM: Add capability to advertise PSCI v0.2 support ARM/ARM64: KVM: Add common header for PSCI related defines ARM/ARM64: KVM: Add base for PSCI v0.2 emulation KVM: Documentation: Add info regarding KVM_ARM_VCPU_PSCI_0_2 feature ARM/ARM64: KVM: Make kvm_psci_call() return convention more flexible KVM: Add KVM_EXIT_SYSTEM_EVENT to user space API header ARM/ARM64: KVM: Emulate PSCI v0.2 SYSTEM_OFF and SYSTEM_RESET ARM/ARM64: KVM: Emulate PSCI v0.2 AFFINITY_INFO ARM/ARM64: KVM: Emulate PSCI v0.2 MIGRATE_INFO_TYPE and related functions ARM/ARM64: KVM: Fix CPU_ON emulation for PSCI v0.2 ARM/ARM64: KVM: Emulate PSCI v0.2 CPU_SUSPEND ARM/ARM64: KVM: Advertise KVM_CAP_ARM_PSCI_0_2 to user space Ashwin Chaugule (3): PSCI: Add initial support for PSCIv0.2 functions Documentation: devicetree: Add new binding for PSCIv0.2 ARM: Check if a CPU has gone offline Marc Zyngier (1): arm64: KVM: Enable minimalistic support for Cortex-A53 Documentation/devicetree/bindings/arm/psci.txt | 37 +++- Documentation/virtual/kvm/api.txt | 17 ++ arch/arm/include/asm/kvm_host.h | 2 +- arch/arm/include/asm/kvm_psci.h | 6 +- arch/arm/include/asm/psci.h | 7 +- arch/arm/include/uapi/asm/kvm.h | 10 +- arch/arm/kernel/psci.c | 196 +++++++++++++++++---- arch/arm/kernel/psci_smp.c | 33 ++++ arch/arm/kvm/arm.c | 1 + arch/arm/kvm/handle_exit.c | 10 +- arch/arm/kvm/psci.c | 235 +++++++++++++++++++++++-- arch/arm64/include/asm/cpu_ops.h | 2 + arch/arm64/include/asm/cputype.h | 1 + arch/arm64/include/asm/kvm_host.h | 2 +- arch/arm64/include/asm/kvm_psci.h | 6 +- arch/arm64/include/asm/psci.h | 2 +- arch/arm64/include/uapi/asm/kvm.h | 13 +- arch/arm64/kernel/psci.c | 231 ++++++++++++++++++++---- arch/arm64/kernel/smp.c | 22 +++ arch/arm64/kvm/guest.c | 2 + arch/arm64/kvm/handle_exit.c | 10 +- arch/arm64/kvm/sys_regs_generic_v8.c | 2 + include/uapi/linux/Kbuild | 1 + include/uapi/linux/kvm.h | 9 + include/uapi/linux/psci.h | 90 ++++++++++ 25 files changed, 831 insertions(+), 116 deletions(-) create mode 100644 include/uapi/linux/psci.h