Currently, KVM ARM/ARM64 only provides in-kernel emulation of Power State and Coordination Interface (PSCI) v0.1.
This patchset aims at providing newer PSCI v0.2 for KVM ARM/ARM64 VCPUs such that it does not break current KVM ARM/ARM64 ABI.
The user space tools (i.e. QEMU or KVMTOOL) will have to explicitly enable KVM_ARM_VCPU_PSCI_0_2 feature using KVM_ARM_VCPU_INIT ioctl for providing PSCI v0.2 to VCPUs.
Changlog:
V6: - Introduce uapi/linux/psci.h for sharing PSCI defines between ARM kernel, ARM64 kernel, KVM ARM/ARM64 and user space - Make CPU_SUSPEND emulation similar to WFI emulation
V5: - Have separate last patch to advertise KVM_CAP_ARM_PSCI_0_2 - Use kvm_psci_version() in kvm_psci_vcpu_on() - Return ALREADY_ON for PSCI v0.2 CPU_ON if VCPU is not paused - Remove per-VCPU suspend context - As-per PSCI v0.2 spec, only current CPU can suspend itself
V4: - Implement all mandatory functions required by PSCI v0.2
V3: - Make KVM_ARM_VCPU_PSCI_0_2 feature experiementatl for now so that it fails for user space till all mandatory PSCI v0.2 functions are emulated by KVM ARM/ARM64 - Have separate patch for making KVM_ARM_VCPU_PSCI_0_2 feature available to user space. This patch can be defferred for now
V2: - Don't rename PSCI return values KVM_PSCI_RET_NI and KVM_PSCI_RET_INVAL - Added kvm_psci_version() to get PSCI version available to VCPU - Fixed grammer in Documentation/virtual/kvm/api.txt
V1: - Initial RFC PATCH
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
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/uapi/asm/kvm.h | 19 +-- arch/arm/kvm/arm.c | 1 + arch/arm/kvm/handle_exit.c | 10 +- arch/arm/kvm/psci.c | 233 ++++++++++++++++++++++++++++++++++--- arch/arm64/include/asm/kvm_host.h | 2 +- arch/arm64/include/asm/kvm_psci.h | 6 +- arch/arm64/include/uapi/asm/kvm.h | 21 ++-- arch/arm64/kvm/handle_exit.c | 10 +- include/uapi/linux/Kbuild | 1 + include/uapi/linux/kvm.h | 9 ++ include/uapi/linux/psci.h | 55 +++++++++ 14 files changed, 344 insertions(+), 48 deletions(-) create mode 100644 include/uapi/linux/psci.h
User space (i.e. QEMU or KVMTOOL) should be able to check whether KVM ARM/ARM64 supports in-kernel PSCI v0.2 emulation. For this purpose, we define KVM_CAP_ARM_PSCI_0_2 in KVM user space interface header.
Signed-off-by: Anup Patel anup.patel@linaro.org Signed-off-by: Pranavkumar Sawargaonkar pranavkumar@linaro.org Acked-by: Christoffer Dall christoffer.dall@linaro.org --- include/uapi/linux/kvm.h | 1 + 1 file changed, 1 insertion(+)
diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h index a7518be..260d3ae 100644 --- a/include/uapi/linux/kvm.h +++ b/include/uapi/linux/kvm.h @@ -741,6 +741,7 @@ struct kvm_ppc_smmu_info { #define KVM_CAP_EXT_EMUL_CPUID 95 #define KVM_CAP_HYPERV_TIME 96 #define KVM_CAP_IOAPIC_POLARITY_IGNORED 97 +#define KVM_CAP_ARM_PSCI_0_2 98
#ifdef KVM_CAP_IRQ_ROUTING
We need a common place to share PSCI related defines among ARM kernel, ARM64 kernel, KVM ARM/ARM64 PSCI emulation, and user space.
We introduce uapi/linux/psci.h for this purpose. This newly added header will be first used by KVM ARM/ARM64 in-kernel PSCI emulation and user space (i.e. QEMU or KVMTOOL).
Signed-off-by: Anup Patel anup.patel@linaro.org Signed-off-by: Pranavkumar Sawargaonkar pranavkumar@linaro.org --- include/uapi/linux/Kbuild | 1 + include/uapi/linux/psci.h | 55 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 56 insertions(+) create mode 100644 include/uapi/linux/psci.h
diff --git a/include/uapi/linux/Kbuild b/include/uapi/linux/Kbuild index 6929571..24e9033 100644 --- a/include/uapi/linux/Kbuild +++ b/include/uapi/linux/Kbuild @@ -317,6 +317,7 @@ header-y += ppp-ioctl.h header-y += ppp_defs.h header-y += pps.h header-y += prctl.h +header-y += psci.h header-y += ptp_clock.h header-y += ptrace.h header-y += qnx4_fs.h diff --git a/include/uapi/linux/psci.h b/include/uapi/linux/psci.h new file mode 100644 index 0000000..41f727e --- /dev/null +++ b/include/uapi/linux/psci.h @@ -0,0 +1,55 @@ +/* + * ARM Power State and Coordination Interface (PSCI) header + * + * This header holds common PSCI defines and macros shared by: + * ARM kernel, ARM64 kernel, KVM ARM/ARM64 and user space. + * + * Copyright (C) 2014 Anup Patel anup.patel@linaro.org + */ + +#ifndef _UAPI_LINUX_PSCI_H +#define _UAPI_LINUX_PSCI_H + +/* PSCI v0.1 interface */ +#define PSCI_FN(base, n) ((base) + (n)) + +#define PSCI_FN_CPU_SUSPEND(base) PSCI_FN(base, 0) +#define PSCI_FN_CPU_OFF(base) PSCI_FN(base, 1) +#define PSCI_FN_CPU_ON(base) PSCI_FN(base, 2) +#define PSCI_FN_MIGRATE(base) PSCI_FN(base, 3) + +/* PSCI v0.2 interface */ +#define PSCI_0_2_FN_BASE 0x84000000 +#define PSCI_0_2_FN(n) (PSCI_0_2_FN_BASE + (n)) +#define PSCI_0_2_FN64_BASE 0xC4000000 +#define PSCI_0_2_FN64(n) (PSCI_0_2_FN64_BASE + (n)) + +#define PSCI_0_2_FN_PSCI_VERSION PSCI_0_2_FN(0) +#define PSCI_0_2_FN_CPU_SUSPEND PSCI_0_2_FN(1) +#define PSCI_0_2_FN_CPU_OFF PSCI_0_2_FN(2) +#define PSCI_0_2_FN_CPU_ON PSCI_0_2_FN(3) +#define PSCI_0_2_FN_AFFINITY_INFO PSCI_0_2_FN(4) +#define PSCI_0_2_FN_MIGRATE PSCI_0_2_FN(5) +#define PSCI_0_2_FN_MIGRATE_INFO_TYPE PSCI_0_2_FN(6) +#define PSCI_0_2_FN_MIGRATE_INFO_UP_CPU PSCI_0_2_FN(7) +#define PSCI_0_2_FN_SYSTEM_OFF PSCI_0_2_FN(8) +#define PSCI_0_2_FN_SYSTEM_RESET PSCI_0_2_FN(9) + +#define PSCI_0_2_FN64_CPU_SUSPEND PSCI_0_2_FN64(1) +#define PSCI_0_2_FN64_CPU_ON PSCI_0_2_FN64(3) +#define PSCI_0_2_FN64_AFFINITY_INFO PSCI_0_2_FN64(4) +#define PSCI_0_2_FN64_MIGRATE PSCI_0_2_FN64(5) +#define PSCI_0_2_FN64_MIGRATE_INFO_UP_CPU PSCI_0_2_FN64(7) + +/* PSCI return values */ +#define PSCI_RET_SUCCESS 0 +#define PSCI_RET_NOT_SUPPORTED ((unsigned long)-1) +#define PSCI_RET_INVALID_PARAMS ((unsigned long)-2) +#define PSCI_RET_DENIED ((unsigned long)-3) +#define PSCI_RET_ALREADY_ON ((unsigned long)-4) +#define PSCI_RET_ON_PENDING ((unsigned long)-5) +#define PSCI_RET_INTERNAL_FAILURE ((unsigned long)-6) +#define PSCI_RET_NOT_PRESENT ((unsigned long)-7) +#define PSCI_RET_DISABLED ((unsigned long)-8) + +#endif /* _UAPI_LINUX_PSCI_H */
On Mon, Mar 31, 2014 at 04:16:03PM +0530, Anup Patel wrote:
We need a common place to share PSCI related defines among ARM kernel, ARM64 kernel, KVM ARM/ARM64 PSCI emulation, and user space.
We introduce uapi/linux/psci.h for this purpose. This newly added header will be first used by KVM ARM/ARM64 in-kernel PSCI emulation and user space (i.e. QEMU or KVMTOOL).
Signed-off-by: Anup Patel anup.patel@linaro.org Signed-off-by: Pranavkumar Sawargaonkar pranavkumar@linaro.org
include/uapi/linux/Kbuild | 1 + include/uapi/linux/psci.h | 55 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 56 insertions(+) create mode 100644 include/uapi/linux/psci.h
You should coordinate with Ashwin's work so you don't step on each other's definitions and one patch set should depend on the other one.
-Christoffer
Hello,
On 31 March 2014 14:07, Christoffer Dall christoffer.dall@linaro.org wrote:
On Mon, Mar 31, 2014 at 04:16:03PM +0530, Anup Patel wrote:
We need a common place to share PSCI related defines among ARM kernel, ARM64 kernel, KVM ARM/ARM64 PSCI emulation, and user space.
We introduce uapi/linux/psci.h for this purpose. This newly added header will be first used by KVM ARM/ARM64 in-kernel PSCI emulation and user space (i.e. QEMU or KVMTOOL).
Signed-off-by: Anup Patel anup.patel@linaro.org Signed-off-by: Pranavkumar Sawargaonkar pranavkumar@linaro.org
include/uapi/linux/Kbuild | 1 + include/uapi/linux/psci.h | 55 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 56 insertions(+) create mode 100644 include/uapi/linux/psci.h
You should coordinate with Ashwin's work so you don't step on each other's definitions and one patch set should depend on the other one.
So, apart from some extra #defines we mostly differ on style. Whats the general recommendation here? Mine looks simpler [1] ;)
Cheers, Ashwin
On Mon, Mar 31, 2014 at 02:18:20PM -0400, Ashwin Chaugule wrote:
Hello,
On 31 March 2014 14:07, Christoffer Dall christoffer.dall@linaro.org wrote:
On Mon, Mar 31, 2014 at 04:16:03PM +0530, Anup Patel wrote:
We need a common place to share PSCI related defines among ARM kernel, ARM64 kernel, KVM ARM/ARM64 PSCI emulation, and user space.
We introduce uapi/linux/psci.h for this purpose. This newly added header will be first used by KVM ARM/ARM64 in-kernel PSCI emulation and user space (i.e. QEMU or KVMTOOL).
Signed-off-by: Anup Patel anup.patel@linaro.org Signed-off-by: Pranavkumar Sawargaonkar pranavkumar@linaro.org
include/uapi/linux/Kbuild | 1 + include/uapi/linux/psci.h | 55 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 56 insertions(+) create mode 100644 include/uapi/linux/psci.h
You should coordinate with Ashwin's work so you don't step on each other's definitions and one patch set should depend on the other one.
So, apart from some extra #defines we mostly differ on style. Whats the general recommendation here? Mine looks simpler [1] ;)
I think Marc brought up the point previously that we may get to PSCI spec v42.17 and the defines should perhaps encompass the version we are defining things for right now...
That being said, assuming new spec versions don't break backwards compatibility in the numbering, the wording proposed by your patch does look simpler.
I do like the hardcoded IDs because that's how it's presented in the spec itself.
-Christoffer
Hi Christoffer,
On 31 March 2014 14:36, Christoffer Dall christoffer.dall@linaro.org wrote:
I think Marc brought up the point previously that we may get to PSCI spec v42.17 and the defines should perhaps encompass the version we are defining things for right now...
That being said, assuming new spec versions don't break backwards compatibility in the numbering, the wording proposed by your patch does look simpler.
I do like the hardcoded IDs because that's how it's presented in the spec itself.
Cool. I'll just add the following line in kbuild to this patch and resend.
include/uapi/linux/Kbuild | 1 +
+header-y += psci.h
Meanwhile, if there are any strong preferences to using the uapi header in Anup's patch, then I can rebase later.
Thanks, Ashwin
On 31 March 2014 15:26, Ashwin Chaugule ashwin.chaugule@linaro.org wrote:
Hi Christoffer,
On 31 March 2014 14:36, Christoffer Dall christoffer.dall@linaro.org wrote:
I think Marc brought up the point previously that we may get to PSCI spec v42.17 and the defines should perhaps encompass the version we are defining things for right now...
That being said, assuming new spec versions don't break backwards compatibility in the numbering, the wording proposed by your patch does look simpler.
I do like the hardcoded IDs because that's how it's presented in the spec itself.
Cool. I'll just add the following line in kbuild to this patch and resend.
by "this" patch, I meant my version. Sorry if I caused any confusion. :)
include/uapi/linux/Kbuild | 1 +
+header-y += psci.h
Meanwhile, if there are any strong preferences to using the uapi header in Anup's patch, then I can rebase later.
Thanks, Ashwin
Hi Ashwin,
On Tue, Apr 1, 2014 at 1:13 AM, Ashwin Chaugule ashwin.chaugule@linaro.org wrote:
On 31 March 2014 15:26, Ashwin Chaugule ashwin.chaugule@linaro.org wrote:
Hi Christoffer,
On 31 March 2014 14:36, Christoffer Dall christoffer.dall@linaro.org wrote:
I think Marc brought up the point previously that we may get to PSCI spec v42.17 and the defines should perhaps encompass the version we are defining things for right now...
That being said, assuming new spec versions don't break backwards compatibility in the numbering, the wording proposed by your patch does look simpler.
I do like the hardcoded IDs because that's how it's presented in the spec itself.
Cool. I'll just add the following line in kbuild to this patch and resend.
by "this" patch, I meant my version. Sorry if I caused any confusion. :)
include/uapi/linux/Kbuild | 1 +
+header-y += psci.h
Meanwhile, if there are any strong preferences to using the uapi header in Anup's patch, then I can rebase later.
Yes, the only difference between headers introduced by your patch and this patch would be styling.
I was thinking of quickly revising this patch with following changes pickedup from your patch:
+#define PSCI_POWER_STATE_ID_MASK 0xffff +#define PSCI_POWER_STATE_ID_SHIFT 0 +#define PSCI_POWER_STATE_TYPE_MASK 0x1 +#define PSCI_POWER_STATE_TYPE_SHIFT 16 +#define PSCI_POWER_STATE_AFFL_MASK 0x3 +#define PSCI_POWER_STATE_AFFL_SHIFT 24 + +#define PSCI_VER_MAJOR_MASK 0xffff0000 +#define PSCI_VER_MINOR_MASK 0x0000ffff +#define PSCI_VER_MAJOR_SHIFT 16 +#define PSCI_VER_MAJOR(ver) \ + ((ver & PSCI_VER_MAJOR_MASK) >> PSCI_VER_MAJOR_SHIFT) +#define PSCI_VER_MINOR(ver) (ver & PSCI_VER_MINOR_MASK)
We also have another RFC v2 patchset for QEMU side changes (being tested currently) which uses the PSCI defines introduced by this patch.
It would be very helpful if you could rebase your patch with v7 of this patch which will include some changes from your patch. (Does this sound OK to you?)
Regards, Anup
Thanks, Ashwin
kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
On Mon, Mar 31, 2014 at 5:46 AM, Anup Patel anup.patel@linaro.org wrote:
We need a common place to share PSCI related defines among ARM kernel, ARM64 kernel, KVM ARM/ARM64 PSCI emulation, and user space.
We introduce uapi/linux/psci.h for this purpose. This newly added header will be first used by KVM ARM/ARM64 in-kernel PSCI emulation and user space (i.e. QEMU or KVMTOOL).
Signed-off-by: Anup Patel anup.patel@linaro.org Signed-off-by: Pranavkumar Sawargaonkar pranavkumar@linaro.org
include/uapi/linux/Kbuild | 1 + include/uapi/linux/psci.h | 55 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 56 insertions(+) create mode 100644 include/uapi/linux/psci.h
diff --git a/include/uapi/linux/Kbuild b/include/uapi/linux/Kbuild index 6929571..24e9033 100644 --- a/include/uapi/linux/Kbuild +++ b/include/uapi/linux/Kbuild @@ -317,6 +317,7 @@ header-y += ppp-ioctl.h header-y += ppp_defs.h header-y += pps.h header-y += prctl.h +header-y += psci.h header-y += ptp_clock.h header-y += ptrace.h header-y += qnx4_fs.h diff --git a/include/uapi/linux/psci.h b/include/uapi/linux/psci.h new file mode 100644 index 0000000..41f727e --- /dev/null +++ b/include/uapi/linux/psci.h @@ -0,0 +1,55 @@ +/*
- ARM Power State and Coordination Interface (PSCI) header
- This header holds common PSCI defines and macros shared by:
- ARM kernel, ARM64 kernel, KVM ARM/ARM64 and user space.
- Copyright (C) 2014 Anup Patel anup.patel@linaro.org
Like I told Ashwin, fix the copyright. You can be the author, but Linaro is the copyright holder.
- */
+#ifndef _UAPI_LINUX_PSCI_H +#define _UAPI_LINUX_PSCI_H
+/* PSCI v0.1 interface */ +#define PSCI_FN(base, n) ((base) + (n))
+#define PSCI_FN_CPU_SUSPEND(base) PSCI_FN(base, 0) +#define PSCI_FN_CPU_OFF(base) PSCI_FN(base, 1) +#define PSCI_FN_CPU_ON(base) PSCI_FN(base, 2) +#define PSCI_FN_MIGRATE(base) PSCI_FN(base, 3)
+/* PSCI v0.2 interface */ +#define PSCI_0_2_FN_BASE 0x84000000 +#define PSCI_0_2_FN(n) (PSCI_0_2_FN_BASE + (n)) +#define PSCI_0_2_FN64_BASE 0xC4000000 +#define PSCI_0_2_FN64(n) (PSCI_0_2_FN64_BASE + (n))
This could be something like:
#define PSCI_0_2_64BIT 0x40000000 #define PSCI_0_2_FN64(n) (PSCI_0_2_FN_BASE + PSCI_0_2_64BIT + (n))
It's also useful as you may want to be able to check for 64-bit calls.
+#define PSCI_0_2_FN_PSCI_VERSION PSCI_0_2_FN(0) +#define PSCI_0_2_FN_CPU_SUSPEND PSCI_0_2_FN(1) +#define PSCI_0_2_FN_CPU_OFF PSCI_0_2_FN(2) +#define PSCI_0_2_FN_CPU_ON PSCI_0_2_FN(3) +#define PSCI_0_2_FN_AFFINITY_INFO PSCI_0_2_FN(4) +#define PSCI_0_2_FN_MIGRATE PSCI_0_2_FN(5) +#define PSCI_0_2_FN_MIGRATE_INFO_TYPE PSCI_0_2_FN(6) +#define PSCI_0_2_FN_MIGRATE_INFO_UP_CPU PSCI_0_2_FN(7) +#define PSCI_0_2_FN_SYSTEM_OFF PSCI_0_2_FN(8) +#define PSCI_0_2_FN_SYSTEM_RESET PSCI_0_2_FN(9)
+#define PSCI_0_2_FN64_CPU_SUSPEND PSCI_0_2_FN64(1) +#define PSCI_0_2_FN64_CPU_ON PSCI_0_2_FN64(3) +#define PSCI_0_2_FN64_AFFINITY_INFO PSCI_0_2_FN64(4) +#define PSCI_0_2_FN64_MIGRATE PSCI_0_2_FN64(5) +#define PSCI_0_2_FN64_MIGRATE_INFO_UP_CPU PSCI_0_2_FN64(7)
+/* PSCI return values */ +#define PSCI_RET_SUCCESS 0 +#define PSCI_RET_NOT_SUPPORTED ((unsigned long)-1) +#define PSCI_RET_INVALID_PARAMS ((unsigned long)-2) +#define PSCI_RET_DENIED ((unsigned long)-3) +#define PSCI_RET_ALREADY_ON ((unsigned long)-4) +#define PSCI_RET_ON_PENDING ((unsigned long)-5) +#define PSCI_RET_INTERNAL_FAILURE ((unsigned long)-6) +#define PSCI_RET_NOT_PRESENT ((unsigned long)-7) +#define PSCI_RET_DISABLED ((unsigned long)-8)
These should not be cast. The return values are defined to be int32 independent of 64-bit or 32-bit calls.
Rob
On Wed, Apr 2, 2014 at 9:52 AM, Rob Herring rob.herring@linaro.org wrote:
On Mon, Mar 31, 2014 at 5:46 AM, Anup Patel anup.patel@linaro.org wrote:
We need a common place to share PSCI related defines among ARM kernel, ARM64 kernel, KVM ARM/ARM64 PSCI emulation, and user space.
We introduce uapi/linux/psci.h for this purpose. This newly added header will be first used by KVM ARM/ARM64 in-kernel PSCI emulation and user space (i.e. QEMU or KVMTOOL).
Signed-off-by: Anup Patel anup.patel@linaro.org Signed-off-by: Pranavkumar Sawargaonkar pranavkumar@linaro.org
include/uapi/linux/Kbuild | 1 + include/uapi/linux/psci.h | 55 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 56 insertions(+) create mode 100644 include/uapi/linux/psci.h
diff --git a/include/uapi/linux/Kbuild b/include/uapi/linux/Kbuild index 6929571..24e9033 100644 --- a/include/uapi/linux/Kbuild +++ b/include/uapi/linux/Kbuild @@ -317,6 +317,7 @@ header-y += ppp-ioctl.h header-y += ppp_defs.h header-y += pps.h header-y += prctl.h +header-y += psci.h header-y += ptp_clock.h header-y += ptrace.h header-y += qnx4_fs.h diff --git a/include/uapi/linux/psci.h b/include/uapi/linux/psci.h new file mode 100644 index 0000000..41f727e --- /dev/null +++ b/include/uapi/linux/psci.h @@ -0,0 +1,55 @@ +/*
- ARM Power State and Coordination Interface (PSCI) header
- This header holds common PSCI defines and macros shared by:
- ARM kernel, ARM64 kernel, KVM ARM/ARM64 and user space.
- Copyright (C) 2014 Anup Patel anup.patel@linaro.org
Like I told Ashwin, fix the copyright. You can be the author, but Linaro is the copyright holder.
Sure, I will update the copyright as-per your suggestion.
- */
+#ifndef _UAPI_LINUX_PSCI_H +#define _UAPI_LINUX_PSCI_H
+/* PSCI v0.1 interface */ +#define PSCI_FN(base, n) ((base) + (n))
+#define PSCI_FN_CPU_SUSPEND(base) PSCI_FN(base, 0) +#define PSCI_FN_CPU_OFF(base) PSCI_FN(base, 1) +#define PSCI_FN_CPU_ON(base) PSCI_FN(base, 2) +#define PSCI_FN_MIGRATE(base) PSCI_FN(base, 3)
+/* PSCI v0.2 interface */ +#define PSCI_0_2_FN_BASE 0x84000000 +#define PSCI_0_2_FN(n) (PSCI_0_2_FN_BASE + (n)) +#define PSCI_0_2_FN64_BASE 0xC4000000 +#define PSCI_0_2_FN64(n) (PSCI_0_2_FN64_BASE + (n))
This could be something like:
#define PSCI_0_2_64BIT 0x40000000 #define PSCI_0_2_FN64(n) (PSCI_0_2_FN_BASE + PSCI_0_2_64BIT + (n))
It's also useful as you may want to be able to check for 64-bit calls.
Good point.
From PSCI emulation perspective, I did not face this situation but we might
need this in future.
+#define PSCI_0_2_FN_PSCI_VERSION PSCI_0_2_FN(0) +#define PSCI_0_2_FN_CPU_SUSPEND PSCI_0_2_FN(1) +#define PSCI_0_2_FN_CPU_OFF PSCI_0_2_FN(2) +#define PSCI_0_2_FN_CPU_ON PSCI_0_2_FN(3) +#define PSCI_0_2_FN_AFFINITY_INFO PSCI_0_2_FN(4) +#define PSCI_0_2_FN_MIGRATE PSCI_0_2_FN(5) +#define PSCI_0_2_FN_MIGRATE_INFO_TYPE PSCI_0_2_FN(6) +#define PSCI_0_2_FN_MIGRATE_INFO_UP_CPU PSCI_0_2_FN(7) +#define PSCI_0_2_FN_SYSTEM_OFF PSCI_0_2_FN(8) +#define PSCI_0_2_FN_SYSTEM_RESET PSCI_0_2_FN(9)
+#define PSCI_0_2_FN64_CPU_SUSPEND PSCI_0_2_FN64(1) +#define PSCI_0_2_FN64_CPU_ON PSCI_0_2_FN64(3) +#define PSCI_0_2_FN64_AFFINITY_INFO PSCI_0_2_FN64(4) +#define PSCI_0_2_FN64_MIGRATE PSCI_0_2_FN64(5) +#define PSCI_0_2_FN64_MIGRATE_INFO_UP_CPU PSCI_0_2_FN64(7)
+/* PSCI return values */ +#define PSCI_RET_SUCCESS 0 +#define PSCI_RET_NOT_SUPPORTED ((unsigned long)-1) +#define PSCI_RET_INVALID_PARAMS ((unsigned long)-2) +#define PSCI_RET_DENIED ((unsigned long)-3) +#define PSCI_RET_ALREADY_ON ((unsigned long)-4) +#define PSCI_RET_ON_PENDING ((unsigned long)-5) +#define PSCI_RET_INTERNAL_FAILURE ((unsigned long)-6) +#define PSCI_RET_NOT_PRESENT ((unsigned long)-7) +#define PSCI_RET_DISABLED ((unsigned long)-8)
These should not be cast. The return values are defined to be int32 independent of 64-bit or 32-bit calls.
The cast is for ease of use in kernel and emulation code.
With the cast in place, we can assign PSCI return values to signed as well as unsigned variables.
Rob _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
-- Anup
On Wed, Apr 2, 2014 at 10:01 AM, Anup Patel anup@brainfault.org wrote:
On Wed, Apr 2, 2014 at 9:52 AM, Rob Herring rob.herring@linaro.org wrote:
On Mon, Mar 31, 2014 at 5:46 AM, Anup Patel anup.patel@linaro.org wrote:
We need a common place to share PSCI related defines among ARM kernel, ARM64 kernel, KVM ARM/ARM64 PSCI emulation, and user space.
We introduce uapi/linux/psci.h for this purpose. This newly added header will be first used by KVM ARM/ARM64 in-kernel PSCI emulation and user space (i.e. QEMU or KVMTOOL).
Signed-off-by: Anup Patel anup.patel@linaro.org Signed-off-by: Pranavkumar Sawargaonkar pranavkumar@linaro.org
include/uapi/linux/Kbuild | 1 + include/uapi/linux/psci.h | 55 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 56 insertions(+) create mode 100644 include/uapi/linux/psci.h
diff --git a/include/uapi/linux/Kbuild b/include/uapi/linux/Kbuild index 6929571..24e9033 100644 --- a/include/uapi/linux/Kbuild +++ b/include/uapi/linux/Kbuild @@ -317,6 +317,7 @@ header-y += ppp-ioctl.h header-y += ppp_defs.h header-y += pps.h header-y += prctl.h +header-y += psci.h header-y += ptp_clock.h header-y += ptrace.h header-y += qnx4_fs.h diff --git a/include/uapi/linux/psci.h b/include/uapi/linux/psci.h new file mode 100644 index 0000000..41f727e --- /dev/null +++ b/include/uapi/linux/psci.h @@ -0,0 +1,55 @@ +/*
- ARM Power State and Coordination Interface (PSCI) header
- This header holds common PSCI defines and macros shared by:
- ARM kernel, ARM64 kernel, KVM ARM/ARM64 and user space.
- Copyright (C) 2014 Anup Patel anup.patel@linaro.org
Like I told Ashwin, fix the copyright. You can be the author, but Linaro is the copyright holder.
Sure, I will update the copyright as-per your suggestion.
- */
+#ifndef _UAPI_LINUX_PSCI_H +#define _UAPI_LINUX_PSCI_H
+/* PSCI v0.1 interface */ +#define PSCI_FN(base, n) ((base) + (n))
+#define PSCI_FN_CPU_SUSPEND(base) PSCI_FN(base, 0) +#define PSCI_FN_CPU_OFF(base) PSCI_FN(base, 1) +#define PSCI_FN_CPU_ON(base) PSCI_FN(base, 2) +#define PSCI_FN_MIGRATE(base) PSCI_FN(base, 3)
+/* PSCI v0.2 interface */ +#define PSCI_0_2_FN_BASE 0x84000000 +#define PSCI_0_2_FN(n) (PSCI_0_2_FN_BASE + (n)) +#define PSCI_0_2_FN64_BASE 0xC4000000 +#define PSCI_0_2_FN64(n) (PSCI_0_2_FN64_BASE + (n))
This could be something like:
#define PSCI_0_2_64BIT 0x40000000 #define PSCI_0_2_FN64(n) (PSCI_0_2_FN_BASE + PSCI_0_2_64BIT + (n))
It's also useful as you may want to be able to check for 64-bit calls.
Good point.
From PSCI emulation perspective, I did not face this situation but we might need this in future.
+#define PSCI_0_2_FN_PSCI_VERSION PSCI_0_2_FN(0) +#define PSCI_0_2_FN_CPU_SUSPEND PSCI_0_2_FN(1) +#define PSCI_0_2_FN_CPU_OFF PSCI_0_2_FN(2) +#define PSCI_0_2_FN_CPU_ON PSCI_0_2_FN(3) +#define PSCI_0_2_FN_AFFINITY_INFO PSCI_0_2_FN(4) +#define PSCI_0_2_FN_MIGRATE PSCI_0_2_FN(5) +#define PSCI_0_2_FN_MIGRATE_INFO_TYPE PSCI_0_2_FN(6) +#define PSCI_0_2_FN_MIGRATE_INFO_UP_CPU PSCI_0_2_FN(7) +#define PSCI_0_2_FN_SYSTEM_OFF PSCI_0_2_FN(8) +#define PSCI_0_2_FN_SYSTEM_RESET PSCI_0_2_FN(9)
+#define PSCI_0_2_FN64_CPU_SUSPEND PSCI_0_2_FN64(1) +#define PSCI_0_2_FN64_CPU_ON PSCI_0_2_FN64(3) +#define PSCI_0_2_FN64_AFFINITY_INFO PSCI_0_2_FN64(4) +#define PSCI_0_2_FN64_MIGRATE PSCI_0_2_FN64(5) +#define PSCI_0_2_FN64_MIGRATE_INFO_UP_CPU PSCI_0_2_FN64(7)
+/* PSCI return values */ +#define PSCI_RET_SUCCESS 0 +#define PSCI_RET_NOT_SUPPORTED ((unsigned long)-1) +#define PSCI_RET_INVALID_PARAMS ((unsigned long)-2) +#define PSCI_RET_DENIED ((unsigned long)-3) +#define PSCI_RET_ALREADY_ON ((unsigned long)-4) +#define PSCI_RET_ON_PENDING ((unsigned long)-5) +#define PSCI_RET_INTERNAL_FAILURE ((unsigned long)-6) +#define PSCI_RET_NOT_PRESENT ((unsigned long)-7) +#define PSCI_RET_DISABLED ((unsigned long)-8)
These should not be cast. The return values are defined to be int32 independent of 64-bit or 32-bit calls.
The cast is for ease of use in kernel and emulation code.
With the cast in place, we can assign PSCI return values to signed as well as unsigned variables.
Never mind, I will update in-kernel PSCI emulation to do the typecasting wherever required. This way we can keep this header closer to spec.
Rob _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
-- Anup
-- Anup
Currently, the in-kernel PSCI emulation provides PSCI v0.1 interface to VCPUs. This patch extends current in-kernel PSCI emulation to provide PSCI v0.2 interface to VCPUs.
By default, ARM/ARM64 KVM will always provide PSCI v0.1 interface for keeping the ABI backward-compatible.
To select PSCI v0.2 interface for VCPUs, the user space (i.e. QEMU or KVMTOOL) will have to set KVM_ARM_VCPU_PSCI_0_2 feature when doing VCPU init using KVM_ARM_VCPU_INIT ioctl.
Signed-off-by: Anup Patel anup.patel@linaro.org Signed-off-by: Pranavkumar Sawargaonkar pranavkumar@linaro.org --- arch/arm/include/asm/kvm_host.h | 2 +- arch/arm/include/asm/kvm_psci.h | 4 ++ arch/arm/include/uapi/asm/kvm.h | 19 ++++---- arch/arm/kvm/psci.c | 93 ++++++++++++++++++++++++++++++------- arch/arm64/include/asm/kvm_host.h | 2 +- arch/arm64/include/asm/kvm_psci.h | 4 ++ arch/arm64/include/uapi/asm/kvm.h | 21 +++++---- 7 files changed, 108 insertions(+), 37 deletions(-)
diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h index 09af149..193ceaf 100644 --- a/arch/arm/include/asm/kvm_host.h +++ b/arch/arm/include/asm/kvm_host.h @@ -36,7 +36,7 @@ #define KVM_COALESCED_MMIO_PAGE_OFFSET 1 #define KVM_HAVE_ONE_REG
-#define KVM_VCPU_MAX_FEATURES 1 +#define KVM_VCPU_MAX_FEATURES 2
#include <kvm/arm_vgic.h>
diff --git a/arch/arm/include/asm/kvm_psci.h b/arch/arm/include/asm/kvm_psci.h index 9a83d98..4c0e3e1 100644 --- a/arch/arm/include/asm/kvm_psci.h +++ b/arch/arm/include/asm/kvm_psci.h @@ -18,6 +18,10 @@ #ifndef __ARM_KVM_PSCI_H__ #define __ARM_KVM_PSCI_H__
+#define KVM_ARM_PSCI_0_1 1 +#define KVM_ARM_PSCI_0_2 2 + +int kvm_psci_version(struct kvm_vcpu *vcpu); bool kvm_psci_call(struct kvm_vcpu *vcpu);
#endif /* __ARM_KVM_PSCI_H__ */ diff --git a/arch/arm/include/uapi/asm/kvm.h b/arch/arm/include/uapi/asm/kvm.h index ef0c878..6574ddf 100644 --- a/arch/arm/include/uapi/asm/kvm.h +++ b/arch/arm/include/uapi/asm/kvm.h @@ -20,6 +20,7 @@ #define __ARM_KVM_H__
#include <linux/types.h> +#include <linux/psci.h> #include <asm/ptrace.h>
#define __KVM_HAVE_GUEST_DEBUG @@ -83,6 +84,7 @@ struct kvm_regs { #define KVM_VGIC_V2_CPU_SIZE 0x2000
#define KVM_ARM_VCPU_POWER_OFF 0 /* CPU is started in OFF state */ +#define KVM_ARM_VCPU_PSCI_0_2 1 /* CPU uses PSCI v0.2 */
struct kvm_vcpu_init { __u32 target; @@ -194,16 +196,15 @@ struct kvm_arch_memory_slot {
/* PSCI interface */ #define KVM_PSCI_FN_BASE 0x95c1ba5e -#define KVM_PSCI_FN(n) (KVM_PSCI_FN_BASE + (n))
-#define KVM_PSCI_FN_CPU_SUSPEND KVM_PSCI_FN(0) -#define KVM_PSCI_FN_CPU_OFF KVM_PSCI_FN(1) -#define KVM_PSCI_FN_CPU_ON KVM_PSCI_FN(2) -#define KVM_PSCI_FN_MIGRATE KVM_PSCI_FN(3) +#define KVM_PSCI_FN_CPU_SUSPEND PSCI_FN(KVM_PSCI_FN_BASE, 0) +#define KVM_PSCI_FN_CPU_OFF PSCI_FN(KVM_PSCI_FN_BASE, 1) +#define KVM_PSCI_FN_CPU_ON PSCI_FN(KVM_PSCI_FN_BASE, 2) +#define KVM_PSCI_FN_MIGRATE PSCI_FN(KVM_PSCI_FN_BASE, 3)
-#define KVM_PSCI_RET_SUCCESS 0 -#define KVM_PSCI_RET_NI ((unsigned long)-1) -#define KVM_PSCI_RET_INVAL ((unsigned long)-2) -#define KVM_PSCI_RET_DENIED ((unsigned long)-3) +#define KVM_PSCI_RET_SUCCESS PSCI_RET_SUCCESS +#define KVM_PSCI_RET_NI PSCI_RET_NOT_SUPPORTED +#define KVM_PSCI_RET_INVAL PSCI_RET_INVALID_PARAMS +#define KVM_PSCI_RET_DENIED PSCI_RET_DENIED
#endif /* __ARM_KVM_H__ */ diff --git a/arch/arm/kvm/psci.c b/arch/arm/kvm/psci.c index 448f60e..8c42596c 100644 --- a/arch/arm/kvm/psci.c +++ b/arch/arm/kvm/psci.c @@ -59,7 +59,7 @@ static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu) * turned off. */ if (!vcpu || !vcpu->arch.pause) - return KVM_PSCI_RET_INVAL; + return PSCI_RET_INVALID_PARAMS;
target_pc = *vcpu_reg(source_vcpu, 2);
@@ -82,20 +82,60 @@ static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu) wq = kvm_arch_vcpu_wq(vcpu); wake_up_interruptible(wq);
- return KVM_PSCI_RET_SUCCESS; + return PSCI_RET_SUCCESS; }
-/** - * kvm_psci_call - handle PSCI call if r0 value is in range - * @vcpu: Pointer to the VCPU struct - * - * Handle PSCI calls from guests through traps from HVC instructions. - * The calling convention is similar to SMC calls to the secure world where - * the function number is placed in r0 and this function returns true if the - * function number specified in r0 is withing the PSCI range, and false - * otherwise. - */ -bool kvm_psci_call(struct kvm_vcpu *vcpu) +int kvm_psci_version(struct kvm_vcpu *vcpu) +{ + if (test_bit(KVM_ARM_VCPU_PSCI_0_2, vcpu->arch.features)) + return KVM_ARM_PSCI_0_2; + + return KVM_ARM_PSCI_0_1; +} + +static bool kvm_psci_0_2_call(struct kvm_vcpu *vcpu) +{ + unsigned long psci_fn = *vcpu_reg(vcpu, 0) & ~((u32) 0); + unsigned long val; + + switch (psci_fn) { + case PSCI_0_2_FN_PSCI_VERSION: + /* + * Bits[31:16] = Major Version = 0 + * Bits[15:0] = Minor Version = 2 + */ + val = 2; + break; + case PSCI_0_2_FN_CPU_OFF: + kvm_psci_vcpu_off(vcpu); + val = PSCI_RET_SUCCESS; + break; + case PSCI_0_2_FN_CPU_ON: + case PSCI_0_2_FN64_CPU_ON: + val = kvm_psci_vcpu_on(vcpu); + break; + case PSCI_0_2_FN_CPU_SUSPEND: + case PSCI_0_2_FN_AFFINITY_INFO: + case PSCI_0_2_FN_MIGRATE: + case PSCI_0_2_FN_MIGRATE_INFO_TYPE: + case PSCI_0_2_FN_MIGRATE_INFO_UP_CPU: + case PSCI_0_2_FN_SYSTEM_OFF: + case PSCI_0_2_FN_SYSTEM_RESET: + case PSCI_0_2_FN64_CPU_SUSPEND: + case PSCI_0_2_FN64_AFFINITY_INFO: + case PSCI_0_2_FN64_MIGRATE: + case PSCI_0_2_FN64_MIGRATE_INFO_UP_CPU: + val = PSCI_RET_NOT_SUPPORTED; + break; + default: + return false; + } + + *vcpu_reg(vcpu, 0) = val; + return true; +} + +static bool kvm_psci_0_1_call(struct kvm_vcpu *vcpu) { unsigned long psci_fn = *vcpu_reg(vcpu, 0) & ~((u32) 0); unsigned long val; @@ -103,16 +143,15 @@ bool kvm_psci_call(struct kvm_vcpu *vcpu) switch (psci_fn) { case KVM_PSCI_FN_CPU_OFF: kvm_psci_vcpu_off(vcpu); - val = KVM_PSCI_RET_SUCCESS; + val = PSCI_RET_SUCCESS; break; case KVM_PSCI_FN_CPU_ON: val = kvm_psci_vcpu_on(vcpu); break; case KVM_PSCI_FN_CPU_SUSPEND: case KVM_PSCI_FN_MIGRATE: - val = KVM_PSCI_RET_NI; + val = PSCI_RET_NOT_SUPPORTED; break; - default: return false; } @@ -120,3 +159,25 @@ bool kvm_psci_call(struct kvm_vcpu *vcpu) *vcpu_reg(vcpu, 0) = val; return true; } + +/** + * kvm_psci_call - handle PSCI call if r0 value is in range + * @vcpu: Pointer to the VCPU struct + * + * Handle PSCI calls from guests through traps from HVC instructions. + * The calling convention is similar to SMC calls to the secure world where + * the function number is placed in r0 and this function returns true if the + * function number specified in r0 is withing the PSCI range, and false + * otherwise. + */ +bool kvm_psci_call(struct kvm_vcpu *vcpu) +{ + switch (kvm_psci_version(vcpu)) { + case KVM_ARM_PSCI_0_2: + return kvm_psci_0_2_call(vcpu); + case KVM_ARM_PSCI_0_1: + return kvm_psci_0_1_call(vcpu); + default: + return false; + }; +} diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h index 0a1d697..92242ce 100644 --- a/arch/arm64/include/asm/kvm_host.h +++ b/arch/arm64/include/asm/kvm_host.h @@ -39,7 +39,7 @@ #include <kvm/arm_vgic.h> #include <kvm/arm_arch_timer.h>
-#define KVM_VCPU_MAX_FEATURES 2 +#define KVM_VCPU_MAX_FEATURES 3
struct kvm_vcpu; int kvm_target_cpu(void); diff --git a/arch/arm64/include/asm/kvm_psci.h b/arch/arm64/include/asm/kvm_psci.h index e301a48..e25c658 100644 --- a/arch/arm64/include/asm/kvm_psci.h +++ b/arch/arm64/include/asm/kvm_psci.h @@ -18,6 +18,10 @@ #ifndef __ARM64_KVM_PSCI_H__ #define __ARM64_KVM_PSCI_H__
+#define KVM_ARM_PSCI_0_1 1 +#define KVM_ARM_PSCI_0_2 2 + +int kvm_psci_version(struct kvm_vcpu *vcpu); bool kvm_psci_call(struct kvm_vcpu *vcpu);
#endif /* __ARM64_KVM_PSCI_H__ */ diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h index eaf54a3..9b67161 100644 --- a/arch/arm64/include/uapi/asm/kvm.h +++ b/arch/arm64/include/uapi/asm/kvm.h @@ -31,6 +31,7 @@ #define KVM_NR_SPSR 5
#ifndef __ASSEMBLY__ +#include <linux/psci.h> #include <asm/types.h> #include <asm/ptrace.h>
@@ -77,6 +78,7 @@ struct kvm_regs {
#define KVM_ARM_VCPU_POWER_OFF 0 /* CPU is started in OFF state */ #define KVM_ARM_VCPU_EL1_32BIT 1 /* CPU running a 32bit VM */ +#define KVM_ARM_VCPU_PSCI_0_2 2 /* CPU uses PSCI v0.2 */
struct kvm_vcpu_init { __u32 target; @@ -177,19 +179,18 @@ struct kvm_arch_memory_slot { /* Highest supported SPI, from VGIC_NR_IRQS */ #define KVM_ARM_IRQ_GIC_MAX 127
-/* PSCI interface */ +/* PSCI v0.1 interface */ #define KVM_PSCI_FN_BASE 0x95c1ba5e -#define KVM_PSCI_FN(n) (KVM_PSCI_FN_BASE + (n))
-#define KVM_PSCI_FN_CPU_SUSPEND KVM_PSCI_FN(0) -#define KVM_PSCI_FN_CPU_OFF KVM_PSCI_FN(1) -#define KVM_PSCI_FN_CPU_ON KVM_PSCI_FN(2) -#define KVM_PSCI_FN_MIGRATE KVM_PSCI_FN(3) +#define KVM_PSCI_FN_CPU_SUSPEND PSCI_FN(KVM_PSCI_FN_BASE, 0) +#define KVM_PSCI_FN_CPU_OFF PSCI_FN(KVM_PSCI_FN_BASE, 1) +#define KVM_PSCI_FN_CPU_ON PSCI_FN(KVM_PSCI_FN_BASE, 2) +#define KVM_PSCI_FN_MIGRATE PSCI_FN(KVM_PSCI_FN_BASE, 3)
-#define KVM_PSCI_RET_SUCCESS 0 -#define KVM_PSCI_RET_NI ((unsigned long)-1) -#define KVM_PSCI_RET_INVAL ((unsigned long)-2) -#define KVM_PSCI_RET_DENIED ((unsigned long)-3) +#define KVM_PSCI_RET_SUCCESS PSCI_RET_SUCCESS +#define KVM_PSCI_RET_NI PSCI_RET_NOT_SUPPORTED +#define KVM_PSCI_RET_INVAL PSCI_RET_INVALID_PARAMS +#define KVM_PSCI_RET_DENIED PSCI_RET_DENIED
#endif
On Mon, Mar 31, 2014 at 04:16:04PM +0530, Anup Patel wrote:
Currently, the in-kernel PSCI emulation provides PSCI v0.1 interface to VCPUs. This patch extends current in-kernel PSCI emulation to provide PSCI v0.2 interface to VCPUs.
By default, ARM/ARM64 KVM will always provide PSCI v0.1 interface for keeping the ABI backward-compatible.
To select PSCI v0.2 interface for VCPUs, the user space (i.e. QEMU or KVMTOOL) will have to set KVM_ARM_VCPU_PSCI_0_2 feature when doing VCPU init using KVM_ARM_VCPU_INIT ioctl.
Signed-off-by: Anup Patel anup.patel@linaro.org Signed-off-by: Pranavkumar Sawargaonkar pranavkumar@linaro.org
arch/arm/include/asm/kvm_host.h | 2 +- arch/arm/include/asm/kvm_psci.h | 4 ++ arch/arm/include/uapi/asm/kvm.h | 19 ++++---- arch/arm/kvm/psci.c | 93 ++++++++++++++++++++++++++++++------- arch/arm64/include/asm/kvm_host.h | 2 +- arch/arm64/include/asm/kvm_psci.h | 4 ++ arch/arm64/include/uapi/asm/kvm.h | 21 +++++---- 7 files changed, 108 insertions(+), 37 deletions(-)
diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h index 09af149..193ceaf 100644 --- a/arch/arm/include/asm/kvm_host.h +++ b/arch/arm/include/asm/kvm_host.h @@ -36,7 +36,7 @@ #define KVM_COALESCED_MMIO_PAGE_OFFSET 1 #define KVM_HAVE_ONE_REG -#define KVM_VCPU_MAX_FEATURES 1 +#define KVM_VCPU_MAX_FEATURES 2 #include <kvm/arm_vgic.h> diff --git a/arch/arm/include/asm/kvm_psci.h b/arch/arm/include/asm/kvm_psci.h index 9a83d98..4c0e3e1 100644 --- a/arch/arm/include/asm/kvm_psci.h +++ b/arch/arm/include/asm/kvm_psci.h @@ -18,6 +18,10 @@ #ifndef __ARM_KVM_PSCI_H__ #define __ARM_KVM_PSCI_H__ +#define KVM_ARM_PSCI_0_1 1 +#define KVM_ARM_PSCI_0_2 2
+int kvm_psci_version(struct kvm_vcpu *vcpu); bool kvm_psci_call(struct kvm_vcpu *vcpu); #endif /* __ARM_KVM_PSCI_H__ */ diff --git a/arch/arm/include/uapi/asm/kvm.h b/arch/arm/include/uapi/asm/kvm.h index ef0c878..6574ddf 100644 --- a/arch/arm/include/uapi/asm/kvm.h +++ b/arch/arm/include/uapi/asm/kvm.h @@ -20,6 +20,7 @@ #define __ARM_KVM_H__ #include <linux/types.h> +#include <linux/psci.h> #include <asm/ptrace.h> #define __KVM_HAVE_GUEST_DEBUG @@ -83,6 +84,7 @@ struct kvm_regs { #define KVM_VGIC_V2_CPU_SIZE 0x2000 #define KVM_ARM_VCPU_POWER_OFF 0 /* CPU is started in OFF state */ +#define KVM_ARM_VCPU_PSCI_0_2 1 /* CPU uses PSCI v0.2 */ struct kvm_vcpu_init { __u32 target; @@ -194,16 +196,15 @@ struct kvm_arch_memory_slot { /* PSCI interface */ #define KVM_PSCI_FN_BASE 0x95c1ba5e -#define KVM_PSCI_FN(n) (KVM_PSCI_FN_BASE + (n)) -#define KVM_PSCI_FN_CPU_SUSPEND KVM_PSCI_FN(0) -#define KVM_PSCI_FN_CPU_OFF KVM_PSCI_FN(1) -#define KVM_PSCI_FN_CPU_ON KVM_PSCI_FN(2) -#define KVM_PSCI_FN_MIGRATE KVM_PSCI_FN(3) +#define KVM_PSCI_FN_CPU_SUSPEND PSCI_FN(KVM_PSCI_FN_BASE, 0) +#define KVM_PSCI_FN_CPU_OFF PSCI_FN(KVM_PSCI_FN_BASE, 1) +#define KVM_PSCI_FN_CPU_ON PSCI_FN(KVM_PSCI_FN_BASE, 2) +#define KVM_PSCI_FN_MIGRATE PSCI_FN(KVM_PSCI_FN_BASE, 3) -#define KVM_PSCI_RET_SUCCESS 0 -#define KVM_PSCI_RET_NI ((unsigned long)-1) -#define KVM_PSCI_RET_INVAL ((unsigned long)-2) -#define KVM_PSCI_RET_DENIED ((unsigned long)-3) +#define KVM_PSCI_RET_SUCCESS PSCI_RET_SUCCESS +#define KVM_PSCI_RET_NI PSCI_RET_NOT_SUPPORTED +#define KVM_PSCI_RET_INVAL PSCI_RET_INVALID_PARAMS +#define KVM_PSCI_RET_DENIED PSCI_RET_DENIED #endif /* __ARM_KVM_H__ */ diff --git a/arch/arm/kvm/psci.c b/arch/arm/kvm/psci.c index 448f60e..8c42596c 100644 --- a/arch/arm/kvm/psci.c +++ b/arch/arm/kvm/psci.c @@ -59,7 +59,7 @@ static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu) * turned off. */ if (!vcpu || !vcpu->arch.pause)
return KVM_PSCI_RET_INVAL;
return PSCI_RET_INVALID_PARAMS;target_pc = *vcpu_reg(source_vcpu, 2); @@ -82,20 +82,60 @@ static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu) wq = kvm_arch_vcpu_wq(vcpu); wake_up_interruptible(wq);
- return KVM_PSCI_RET_SUCCESS;
- return PSCI_RET_SUCCESS;
} -/**
- kvm_psci_call - handle PSCI call if r0 value is in range
- @vcpu: Pointer to the VCPU struct
- Handle PSCI calls from guests through traps from HVC instructions.
- The calling convention is similar to SMC calls to the secure world where
- the function number is placed in r0 and this function returns true if the
- function number specified in r0 is withing the PSCI range, and false
- otherwise.
- */
-bool kvm_psci_call(struct kvm_vcpu *vcpu) +int kvm_psci_version(struct kvm_vcpu *vcpu) +{
- if (test_bit(KVM_ARM_VCPU_PSCI_0_2, vcpu->arch.features))
return KVM_ARM_PSCI_0_2;- return KVM_ARM_PSCI_0_1;
+}
+static bool kvm_psci_0_2_call(struct kvm_vcpu *vcpu) +{
- unsigned long psci_fn = *vcpu_reg(vcpu, 0) & ~((u32) 0);
- unsigned long val;
- switch (psci_fn) {
- case PSCI_0_2_FN_PSCI_VERSION:
/** Bits[31:16] = Major Version = 0* Bits[15:0] = Minor Version = 2*/val = 2;break;- case PSCI_0_2_FN_CPU_OFF:
kvm_psci_vcpu_off(vcpu);val = PSCI_RET_SUCCESS;break;- case PSCI_0_2_FN_CPU_ON:
- case PSCI_0_2_FN64_CPU_ON:
val = kvm_psci_vcpu_on(vcpu);break;- case PSCI_0_2_FN_CPU_SUSPEND:
- case PSCI_0_2_FN_AFFINITY_INFO:
- case PSCI_0_2_FN_MIGRATE:
- case PSCI_0_2_FN_MIGRATE_INFO_TYPE:
- case PSCI_0_2_FN_MIGRATE_INFO_UP_CPU:
- case PSCI_0_2_FN_SYSTEM_OFF:
- case PSCI_0_2_FN_SYSTEM_RESET:
- case PSCI_0_2_FN64_CPU_SUSPEND:
- case PSCI_0_2_FN64_AFFINITY_INFO:
- case PSCI_0_2_FN64_MIGRATE:
- case PSCI_0_2_FN64_MIGRATE_INFO_UP_CPU:
val = PSCI_RET_NOT_SUPPORTED;break;- default:
return false;- }
- *vcpu_reg(vcpu, 0) = val;
- return true;
+}
+static bool kvm_psci_0_1_call(struct kvm_vcpu *vcpu) { unsigned long psci_fn = *vcpu_reg(vcpu, 0) & ~((u32) 0); unsigned long val; @@ -103,16 +143,15 @@ bool kvm_psci_call(struct kvm_vcpu *vcpu) switch (psci_fn) { case KVM_PSCI_FN_CPU_OFF: kvm_psci_vcpu_off(vcpu);
val = KVM_PSCI_RET_SUCCESS;
val = PSCI_RET_SUCCESS;
val = KVM_PSCI_RET_NI;
val = PSCI_RET_NOT_SUPPORTED;
- default: return false; }
@@ -120,3 +159,25 @@ bool kvm_psci_call(struct kvm_vcpu *vcpu) *vcpu_reg(vcpu, 0) = val; return true; }
+/**
- kvm_psci_call - handle PSCI call if r0 value is in range
- @vcpu: Pointer to the VCPU struct
- Handle PSCI calls from guests through traps from HVC instructions.
- The calling convention is similar to SMC calls to the secure world where
- the function number is placed in r0 and this function returns true if the
- function number specified in r0 is withing the PSCI range, and false
- otherwise.
- */
+bool kvm_psci_call(struct kvm_vcpu *vcpu) +{
- switch (kvm_psci_version(vcpu)) {
- case KVM_ARM_PSCI_0_2:
return kvm_psci_0_2_call(vcpu);- case KVM_ARM_PSCI_0_1:
return kvm_psci_0_1_call(vcpu);- default:
return false;- };
+} diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h index 0a1d697..92242ce 100644 --- a/arch/arm64/include/asm/kvm_host.h +++ b/arch/arm64/include/asm/kvm_host.h @@ -39,7 +39,7 @@ #include <kvm/arm_vgic.h> #include <kvm/arm_arch_timer.h> -#define KVM_VCPU_MAX_FEATURES 2 +#define KVM_VCPU_MAX_FEATURES 3 struct kvm_vcpu; int kvm_target_cpu(void); diff --git a/arch/arm64/include/asm/kvm_psci.h b/arch/arm64/include/asm/kvm_psci.h index e301a48..e25c658 100644 --- a/arch/arm64/include/asm/kvm_psci.h +++ b/arch/arm64/include/asm/kvm_psci.h @@ -18,6 +18,10 @@ #ifndef __ARM64_KVM_PSCI_H__ #define __ARM64_KVM_PSCI_H__ +#define KVM_ARM_PSCI_0_1 1 +#define KVM_ARM_PSCI_0_2 2
+int kvm_psci_version(struct kvm_vcpu *vcpu); bool kvm_psci_call(struct kvm_vcpu *vcpu); #endif /* __ARM64_KVM_PSCI_H__ */ diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h index eaf54a3..9b67161 100644 --- a/arch/arm64/include/uapi/asm/kvm.h +++ b/arch/arm64/include/uapi/asm/kvm.h @@ -31,6 +31,7 @@ #define KVM_NR_SPSR 5 #ifndef __ASSEMBLY__ +#include <linux/psci.h> #include <asm/types.h> #include <asm/ptrace.h> @@ -77,6 +78,7 @@ struct kvm_regs { #define KVM_ARM_VCPU_POWER_OFF 0 /* CPU is started in OFF state */ #define KVM_ARM_VCPU_EL1_32BIT 1 /* CPU running a 32bit VM */ +#define KVM_ARM_VCPU_PSCI_0_2 2 /* CPU uses PSCI v0.2 */ struct kvm_vcpu_init { __u32 target; @@ -177,19 +179,18 @@ struct kvm_arch_memory_slot { /* Highest supported SPI, from VGIC_NR_IRQS */ #define KVM_ARM_IRQ_GIC_MAX 127 -/* PSCI interface */ +/* PSCI v0.1 interface */ #define KVM_PSCI_FN_BASE 0x95c1ba5e -#define KVM_PSCI_FN(n) (KVM_PSCI_FN_BASE + (n)) -#define KVM_PSCI_FN_CPU_SUSPEND KVM_PSCI_FN(0) -#define KVM_PSCI_FN_CPU_OFF KVM_PSCI_FN(1) -#define KVM_PSCI_FN_CPU_ON KVM_PSCI_FN(2) -#define KVM_PSCI_FN_MIGRATE KVM_PSCI_FN(3) +#define KVM_PSCI_FN_CPU_SUSPEND PSCI_FN(KVM_PSCI_FN_BASE, 0) +#define KVM_PSCI_FN_CPU_OFF PSCI_FN(KVM_PSCI_FN_BASE, 1) +#define KVM_PSCI_FN_CPU_ON PSCI_FN(KVM_PSCI_FN_BASE, 2) +#define KVM_PSCI_FN_MIGRATE PSCI_FN(KVM_PSCI_FN_BASE, 3) -#define KVM_PSCI_RET_SUCCESS 0 -#define KVM_PSCI_RET_NI ((unsigned long)-1) -#define KVM_PSCI_RET_INVAL ((unsigned long)-2) -#define KVM_PSCI_RET_DENIED ((unsigned long)-3) +#define KVM_PSCI_RET_SUCCESS PSCI_RET_SUCCESS +#define KVM_PSCI_RET_NI PSCI_RET_NOT_SUPPORTED +#define KVM_PSCI_RET_INVAL PSCI_RET_INVALID_PARAMS +#define KVM_PSCI_RET_DENIED PSCI_RET_DENIED #endif -- 1.7.9.5
Acked-by: Christoffer Dall christoffer.dall@linaro.org
We have in-kernel emulation of PSCI v0.2 in KVM ARM/ARM64. To provide PSCI v0.2 interface to VCPUs, we have to enable KVM_ARM_VCPU_PSCI_0_2 feature when doing KVM_ARM_VCPU_INIT ioctl.
The patch updates documentation of KVM_ARM_VCPU_INIT ioctl to provide info regarding KVM_ARM_VCPU_PSCI_0_2 feature.
Signed-off-by: Anup Patel anup.patel@linaro.org Signed-off-by: Pranavkumar Sawargaonkar pranavkumar@linaro.org Acked-by: Christoffer Dall christoffer.dall@linaro.org --- Documentation/virtual/kvm/api.txt | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt index 4714f28..0114d5b 100644 --- a/Documentation/virtual/kvm/api.txt +++ b/Documentation/virtual/kvm/api.txt @@ -2361,6 +2361,8 @@ Possible features: Depends on KVM_CAP_ARM_PSCI. - KVM_ARM_VCPU_EL1_32BIT: Starts the CPU in a 32bit mode. Depends on KVM_CAP_ARM_EL1_32BIT (arm64 only). + - KVM_ARM_VCPU_PSCI_0_2: Emulate PSCI v0.2 for the CPU. + Depends on KVM_CAP_ARM_PSCI_0_2.
4.83 KVM_ARM_PREFERRED_TARGET
Currently, the kvm_psci_call() returns 'true' or 'false' based on whether the PSCI function call was handled successfully or not. This does not help us emulate system-level PSCI functions where the actual emulation work will be done by user space (QEMU or KVMTOOL). Examples of such system-level PSCI functions are: PSCI v0.2 SYSTEM_OFF and SYSTEM_RESET.
This patch updates kvm_psci_call() to return three types of values: 1) > 0 (success) 2) = 0 (success but exit to user space) 3) < 0 (errors)
Signed-off-by: Anup Patel anup.patel@linaro.org Signed-off-by: Pranavkumar Sawargaonkar pranavkumar@linaro.org Reviewed-by: Christoffer Dall christoffer.dall@linaro.org --- arch/arm/include/asm/kvm_psci.h | 2 +- arch/arm/kvm/handle_exit.c | 10 +++++++--- arch/arm/kvm/psci.c | 28 ++++++++++++++++------------ arch/arm64/include/asm/kvm_psci.h | 2 +- arch/arm64/kvm/handle_exit.c | 10 +++++++--- 5 files changed, 32 insertions(+), 20 deletions(-)
diff --git a/arch/arm/include/asm/kvm_psci.h b/arch/arm/include/asm/kvm_psci.h index 4c0e3e1..6bda945 100644 --- a/arch/arm/include/asm/kvm_psci.h +++ b/arch/arm/include/asm/kvm_psci.h @@ -22,6 +22,6 @@ #define KVM_ARM_PSCI_0_2 2
int kvm_psci_version(struct kvm_vcpu *vcpu); -bool kvm_psci_call(struct kvm_vcpu *vcpu); +int kvm_psci_call(struct kvm_vcpu *vcpu);
#endif /* __ARM_KVM_PSCI_H__ */ diff --git a/arch/arm/kvm/handle_exit.c b/arch/arm/kvm/handle_exit.c index 0de91fc..1270095 100644 --- a/arch/arm/kvm/handle_exit.c +++ b/arch/arm/kvm/handle_exit.c @@ -38,14 +38,18 @@ static int handle_svc_hyp(struct kvm_vcpu *vcpu, struct kvm_run *run)
static int handle_hvc(struct kvm_vcpu *vcpu, struct kvm_run *run) { + int ret; + trace_kvm_hvc(*vcpu_pc(vcpu), *vcpu_reg(vcpu, 0), kvm_vcpu_hvc_get_imm(vcpu));
- if (kvm_psci_call(vcpu)) + ret = kvm_psci_call(vcpu); + if (ret == -EINVAL) { + kvm_inject_undefined(vcpu); return 1; + }
- kvm_inject_undefined(vcpu); - return 1; + return ret; }
static int handle_smc(struct kvm_vcpu *vcpu, struct kvm_run *run) diff --git a/arch/arm/kvm/psci.c b/arch/arm/kvm/psci.c index 8c42596c..14e6fa6 100644 --- a/arch/arm/kvm/psci.c +++ b/arch/arm/kvm/psci.c @@ -93,7 +93,7 @@ int kvm_psci_version(struct kvm_vcpu *vcpu) return KVM_ARM_PSCI_0_1; }
-static bool kvm_psci_0_2_call(struct kvm_vcpu *vcpu) +static int kvm_psci_0_2_call(struct kvm_vcpu *vcpu) { unsigned long psci_fn = *vcpu_reg(vcpu, 0) & ~((u32) 0); unsigned long val; @@ -128,14 +128,14 @@ static bool kvm_psci_0_2_call(struct kvm_vcpu *vcpu) val = PSCI_RET_NOT_SUPPORTED; break; default: - return false; + return -EINVAL; }
*vcpu_reg(vcpu, 0) = val; - return true; + return 1; }
-static bool kvm_psci_0_1_call(struct kvm_vcpu *vcpu) +static int kvm_psci_0_1_call(struct kvm_vcpu *vcpu) { unsigned long psci_fn = *vcpu_reg(vcpu, 0) & ~((u32) 0); unsigned long val; @@ -153,11 +153,11 @@ static bool kvm_psci_0_1_call(struct kvm_vcpu *vcpu) val = PSCI_RET_NOT_SUPPORTED; break; default: - return false; + return -EINVAL; }
*vcpu_reg(vcpu, 0) = val; - return true; + return 1; }
/** @@ -165,12 +165,16 @@ static bool kvm_psci_0_1_call(struct kvm_vcpu *vcpu) * @vcpu: Pointer to the VCPU struct * * Handle PSCI calls from guests through traps from HVC instructions. - * The calling convention is similar to SMC calls to the secure world where - * the function number is placed in r0 and this function returns true if the - * function number specified in r0 is withing the PSCI range, and false - * otherwise. + * The calling convention is similar to SMC calls to the secure world + * where the function number is placed in r0. + * + * This function returns: > 0 (success), 0 (success but exit to user + * space), and < 0 (errors) + * + * Errors: + * -EINVAL: Unrecognized PSCI function */ -bool kvm_psci_call(struct kvm_vcpu *vcpu) +int kvm_psci_call(struct kvm_vcpu *vcpu) { switch (kvm_psci_version(vcpu)) { case KVM_ARM_PSCI_0_2: @@ -178,6 +182,6 @@ bool kvm_psci_call(struct kvm_vcpu *vcpu) case KVM_ARM_PSCI_0_1: return kvm_psci_0_1_call(vcpu); default: - return false; + return -EINVAL; }; } diff --git a/arch/arm64/include/asm/kvm_psci.h b/arch/arm64/include/asm/kvm_psci.h index e25c658..bc39e55 100644 --- a/arch/arm64/include/asm/kvm_psci.h +++ b/arch/arm64/include/asm/kvm_psci.h @@ -22,6 +22,6 @@ #define KVM_ARM_PSCI_0_2 2
int kvm_psci_version(struct kvm_vcpu *vcpu); -bool kvm_psci_call(struct kvm_vcpu *vcpu); +int kvm_psci_call(struct kvm_vcpu *vcpu);
#endif /* __ARM64_KVM_PSCI_H__ */ diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c index 7bc41ea..743a74d 100644 --- a/arch/arm64/kvm/handle_exit.c +++ b/arch/arm64/kvm/handle_exit.c @@ -30,11 +30,15 @@ typedef int (*exit_handle_fn)(struct kvm_vcpu *, struct kvm_run *);
static int handle_hvc(struct kvm_vcpu *vcpu, struct kvm_run *run) { - if (kvm_psci_call(vcpu)) + int ret; + + ret = kvm_psci_call(vcpu); + if (ret == -EINVAL) { + kvm_inject_undefined(vcpu); return 1; + }
- kvm_inject_undefined(vcpu); - return 1; + return ret; }
static int handle_smc(struct kvm_vcpu *vcpu, struct kvm_run *run)
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.
Signed-off-by: Anup Patel anup.patel@linaro.org Signed-off-by: Pranavkumar Sawargaonkar pranavkumar@linaro.org Reviewed-by: Christoffer Dall christoffer.dall@linaro.org --- Documentation/virtual/kvm/api.txt | 15 +++++++++++++++ include/uapi/linux/kvm.h | 8 ++++++++ 2 files changed, 23 insertions(+)
diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt index 0114d5b..e675ec2 100644 --- a/Documentation/virtual/kvm/api.txt +++ b/Documentation/virtual/kvm/api.txt @@ -2721,6 +2721,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. + /* Fix the size of the union. */ char padding[256]; }; diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h index 260d3ae..0cf2882 100644 --- a/include/uapi/linux/kvm.h +++ b/include/uapi/linux/kvm.h @@ -171,6 +171,7 @@ struct kvm_pit_config { #define KVM_EXIT_WATCHDOG 21 #define KVM_EXIT_S390_TSCH 22 #define KVM_EXIT_EPR 23 +#define KVM_EXIT_SYSTEM_EVENT 24
/* For KVM_EXIT_INTERNAL_ERROR */ /* Emulate instruction failed. */ @@ -301,6 +302,13 @@ struct kvm_run { struct { __u32 epr; } epr; + /* KVM_EXIT_SYSTEM_EVENT */ + struct { +#define KVM_SYSTEM_EVENT_SHUTDOWN 1 +#define KVM_SYSTEM_EVENT_RESET 2 + __u32 type; + __u64 flags; + } system_event; /* Fix the size of the union. */ char padding[256]; };
The PSCI v0.2 SYSTEM_OFF and SYSTEM_RESET functions are system-level functions hence cannot be fully emulated by in-kernel PSCI emulation code.
To tackle this, we forward PSCI v0.2 SYSTEM_OFF and SYSTEM_RESET function calls from vcpu to user space (i.e. QEMU or KVMTOOL) via kvm_run structure using KVM_EXIT_SYSTEM_EVENT exit reasons.
Signed-off-by: Anup Patel anup.patel@linaro.org Signed-off-by: Pranavkumar Sawargaonkar pranavkumar@linaro.org Reviewed-by: Christoffer Dall christoffer.dall@linaro.org --- arch/arm/kvm/psci.c | 32 +++++++++++++++++++++++++++++--- 1 file changed, 29 insertions(+), 3 deletions(-)
diff --git a/arch/arm/kvm/psci.c b/arch/arm/kvm/psci.c index 14e6fa6..b964aa4 100644 --- a/arch/arm/kvm/psci.c +++ b/arch/arm/kvm/psci.c @@ -85,6 +85,23 @@ static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu) return PSCI_RET_SUCCESS; }
+static inline void kvm_prepare_system_event(struct kvm_vcpu *vcpu, u32 type) +{ + memset(&vcpu->run->system_event, 0, sizeof(vcpu->run->system_event)); + vcpu->run->system_event.type = type; + vcpu->run->exit_reason = KVM_EXIT_SYSTEM_EVENT; +} + +static void kvm_psci_system_off(struct kvm_vcpu *vcpu) +{ + kvm_prepare_system_event(vcpu, KVM_SYSTEM_EVENT_SHUTDOWN); +} + +static void kvm_psci_system_reset(struct kvm_vcpu *vcpu) +{ + kvm_prepare_system_event(vcpu, KVM_SYSTEM_EVENT_RESET); +} + int kvm_psci_version(struct kvm_vcpu *vcpu) { if (test_bit(KVM_ARM_VCPU_PSCI_0_2, vcpu->arch.features)) @@ -95,6 +112,7 @@ int kvm_psci_version(struct kvm_vcpu *vcpu)
static int kvm_psci_0_2_call(struct kvm_vcpu *vcpu) { + int ret = 1; unsigned long psci_fn = *vcpu_reg(vcpu, 0) & ~((u32) 0); unsigned long val;
@@ -114,13 +132,21 @@ static int kvm_psci_0_2_call(struct kvm_vcpu *vcpu) case PSCI_0_2_FN64_CPU_ON: val = kvm_psci_vcpu_on(vcpu); break; + case PSCI_0_2_FN_SYSTEM_OFF: + kvm_psci_system_off(vcpu); + val = PSCI_RET_SUCCESS; + ret = 0; + break; + case PSCI_0_2_FN_SYSTEM_RESET: + kvm_psci_system_reset(vcpu); + val = PSCI_RET_SUCCESS; + ret = 0; + break; case PSCI_0_2_FN_CPU_SUSPEND: case PSCI_0_2_FN_AFFINITY_INFO: case PSCI_0_2_FN_MIGRATE: case PSCI_0_2_FN_MIGRATE_INFO_TYPE: case PSCI_0_2_FN_MIGRATE_INFO_UP_CPU: - case PSCI_0_2_FN_SYSTEM_OFF: - case PSCI_0_2_FN_SYSTEM_RESET: case PSCI_0_2_FN64_CPU_SUSPEND: case PSCI_0_2_FN64_AFFINITY_INFO: case PSCI_0_2_FN64_MIGRATE: @@ -132,7 +158,7 @@ static int kvm_psci_0_2_call(struct kvm_vcpu *vcpu) }
*vcpu_reg(vcpu, 0) = val; - return 1; + return ret; }
static int kvm_psci_0_1_call(struct kvm_vcpu *vcpu)
This patch adds emulation of PSCI v0.2 AFFINITY_INFO function call for KVM ARM/ARM64. This is a VCPU-level function call which will be used to determine current state of given affinity level.
Signed-off-by: Anup Patel anup.patel@linaro.org Signed-off-by: Pranavkumar Sawargaonkar pranavkumar@linaro.org Reviewed-by: Christoffer Dall christoffer.dall@linaro.org --- arch/arm/kvm/psci.c | 67 +++++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 65 insertions(+), 2 deletions(-)
diff --git a/arch/arm/kvm/psci.c b/arch/arm/kvm/psci.c index b964aa4..c80bd45 100644 --- a/arch/arm/kvm/psci.c +++ b/arch/arm/kvm/psci.c @@ -27,6 +27,31 @@ * as described in ARM document number ARM DEN 0022A. */
+static unsigned long psci_affinity_mask(unsigned long affinity_level) +{ + unsigned long affinity_mask = MPIDR_HWID_BITMASK; + + switch (affinity_level) { + case 0: /* All affinity levels are valid */ + affinity_mask &= ~0x0UL; + break; + case 1: /* Aff0 ignored */ + affinity_mask &= ~0xFFUL; + break; + case 2: /* Aff0 and Aff1 ignored */ + affinity_mask &= ~0xFFFFUL; + break; + case 3: /* Aff0, Aff1, and Aff2 ignored */ + affinity_mask &= ~0xFFFFFFUL; + break; + default: + affinity_mask = 0; + break; + }; + + return affinity_mask; +} + static void kvm_psci_vcpu_off(struct kvm_vcpu *vcpu) { vcpu->arch.pause = true; @@ -85,6 +110,42 @@ static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu) return PSCI_RET_SUCCESS; }
+static unsigned long kvm_psci_vcpu_affinity_info(struct kvm_vcpu *vcpu) +{ + int i; + unsigned long mpidr; + unsigned long target_affinity; + unsigned long target_affinity_mask; + unsigned long lowest_affinity_level; + struct kvm *kvm = vcpu->kvm; + struct kvm_vcpu *tmp; + + target_affinity = *vcpu_reg(vcpu, 1); + lowest_affinity_level = *vcpu_reg(vcpu, 2); + + /* Determine target affinity mask */ + target_affinity_mask = psci_affinity_mask(lowest_affinity_level); + if (!target_affinity_mask) + return KVM_PSCI_RET_INVAL; + + /* Ignore other bits of target affinity */ + target_affinity &= target_affinity_mask; + + /* + * If one or more VCPU matching target affinity are running + * then return 0 (ON) else return 1 (OFF) + */ + kvm_for_each_vcpu(i, tmp, kvm) { + mpidr = kvm_vcpu_get_mpidr(tmp); + if (((mpidr & target_affinity_mask) == target_affinity) && + !tmp->arch.pause) { + return 0; + } + } + + return 1; +} + static inline void kvm_prepare_system_event(struct kvm_vcpu *vcpu, u32 type) { memset(&vcpu->run->system_event, 0, sizeof(vcpu->run->system_event)); @@ -132,6 +193,10 @@ static int kvm_psci_0_2_call(struct kvm_vcpu *vcpu) case PSCI_0_2_FN64_CPU_ON: val = kvm_psci_vcpu_on(vcpu); break; + case PSCI_0_2_FN_AFFINITY_INFO: + case PSCI_0_2_FN64_AFFINITY_INFO: + val = kvm_psci_vcpu_affinity_info(vcpu); + break; case PSCI_0_2_FN_SYSTEM_OFF: kvm_psci_system_off(vcpu); val = PSCI_RET_SUCCESS; @@ -143,12 +208,10 @@ static int kvm_psci_0_2_call(struct kvm_vcpu *vcpu) ret = 0; break; case PSCI_0_2_FN_CPU_SUSPEND: - case PSCI_0_2_FN_AFFINITY_INFO: case PSCI_0_2_FN_MIGRATE: case PSCI_0_2_FN_MIGRATE_INFO_TYPE: case PSCI_0_2_FN_MIGRATE_INFO_UP_CPU: case PSCI_0_2_FN64_CPU_SUSPEND: - case PSCI_0_2_FN64_AFFINITY_INFO: case PSCI_0_2_FN64_MIGRATE: case PSCI_0_2_FN64_MIGRATE_INFO_UP_CPU: val = PSCI_RET_NOT_SUPPORTED;
This patch adds emulation of PSCI v0.2 MIGRATE, MIGRATE_INFO_TYPE, and MIGRATE_INFO_UP_CPU function calls for KVM ARM/ARM64.
KVM ARM/ARM64 being a hypervisor (and not a Trusted OS), we cannot provide this functions hence we emulate these functions in following way: 1. MIGRATE - Returns "Not Supported" 2. MIGRATE_INFO_TYPE - Return 2 i.e. Trusted OS is either not present or does not require migration 3. MIGRATE_INFO_UP_CPU - Returns "Not Supported"
Signed-off-by: Anup Patel anup.patel@linaro.org Signed-off-by: Pranavkumar Sawargaonkar pranavkumar@linaro.org Reviewed-by: Christoffer Dall christoffer.dall@linaro.org --- arch/arm/kvm/psci.c | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-)
diff --git a/arch/arm/kvm/psci.c b/arch/arm/kvm/psci.c index c80bd45..961c124 100644 --- a/arch/arm/kvm/psci.c +++ b/arch/arm/kvm/psci.c @@ -197,6 +197,21 @@ static int kvm_psci_0_2_call(struct kvm_vcpu *vcpu) case PSCI_0_2_FN64_AFFINITY_INFO: val = kvm_psci_vcpu_affinity_info(vcpu); break; + case PSCI_0_2_FN_MIGRATE: + case PSCI_0_2_FN64_MIGRATE: + val = PSCI_RET_NOT_SUPPORTED; + break; + case PSCI_0_2_FN_MIGRATE_INFO_TYPE: + /* + * Trusted OS is either not present or + * does not require migration + */ + val = 2; + break; + case PSCI_0_2_FN_MIGRATE_INFO_UP_CPU: + case PSCI_0_2_FN64_MIGRATE_INFO_UP_CPU: + val = PSCI_RET_NOT_SUPPORTED; + break; case PSCI_0_2_FN_SYSTEM_OFF: kvm_psci_system_off(vcpu); val = PSCI_RET_SUCCESS; @@ -208,12 +223,7 @@ static int kvm_psci_0_2_call(struct kvm_vcpu *vcpu) ret = 0; break; case PSCI_0_2_FN_CPU_SUSPEND: - case PSCI_0_2_FN_MIGRATE: - case PSCI_0_2_FN_MIGRATE_INFO_TYPE: - case PSCI_0_2_FN_MIGRATE_INFO_UP_CPU: case PSCI_0_2_FN64_CPU_SUSPEND: - case PSCI_0_2_FN64_MIGRATE: - case PSCI_0_2_FN64_MIGRATE_INFO_UP_CPU: val = PSCI_RET_NOT_SUPPORTED; break; default:
As-per PSCI v0.2, the source CPU provides physical address of "entry point" and "context id" for starting a target CPU. Also, if target CPU is already running then we should return ALREADY_ON.
Current emulation of CPU_ON function does not consider physical address of "context id" and returns INVALID_PARAMETERS if target CPU is already running.
This patch updates kvm_psci_vcpu_on() such that it works for both PSCI v0.1 and PSCI v0.2.
Signed-off-by: Anup Patel anup.patel@linaro.org Signed-off-by: Pranavkumar Sawargaonkar pranavkumar@linaro.org Reviewed-by: Christoffer Dall christoffer.dall@linaro.org --- arch/arm/kvm/psci.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/arch/arm/kvm/psci.c b/arch/arm/kvm/psci.c index 961c124..85bf896 100644 --- a/arch/arm/kvm/psci.c +++ b/arch/arm/kvm/psci.c @@ -63,6 +63,7 @@ static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu) struct kvm_vcpu *vcpu = NULL, *tmp; wait_queue_head_t *wq; unsigned long cpu_id; + unsigned long context_id; unsigned long mpidr; phys_addr_t target_pc; int i; @@ -83,10 +84,17 @@ static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu) * Make sure the caller requested a valid CPU and that the CPU is * turned off. */ - if (!vcpu || !vcpu->arch.pause) + if (!vcpu) return PSCI_RET_INVALID_PARAMS; + if (!vcpu->arch.pause) { + if (kvm_psci_version(source_vcpu) != KVM_ARM_PSCI_0_1) + return PSCI_RET_ALREADY_ON; + else + return PSCI_RET_INVALID_PARAMS; + }
target_pc = *vcpu_reg(source_vcpu, 2); + context_id = *vcpu_reg(source_vcpu, 3);
kvm_reset_vcpu(vcpu);
@@ -101,6 +109,8 @@ static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu) kvm_vcpu_set_be(vcpu);
*vcpu_pc(vcpu) = target_pc; + if (kvm_psci_version(source_vcpu) != KVM_ARM_PSCI_0_1) + *vcpu_reg(vcpu, 0) = context_id; vcpu->arch.pause = false; smp_mb(); /* Make sure the above is visible */
This patch adds emulation of PSCI v0.2 CPU_SUSPEND function call for KVM ARM/ARM64. This is a CPU-level function call which can suspend current CPU or current CPU cluster. We don't have VCPU clusters in KVM so for KVM we simply suspend the current VCPU.
The CPU_SUSPEND emulation is not tested much because currently there is no CPUIDLE driver in Linux kernel that uses PSCI CPU_SUSPEND. The PSCI CPU_SUSPEND implementation in ARM64 kernel was tested using a Simple CPUIDLE driver which is not published due to unstable DT-bindings for PSCI. (For more info, http://lwn.net/Articles/574950/)
Even if we had stable DT-bindings for PSCI and CPUIDLE driver that uses PSCI CPU_SUSPEND then still we need to define SUSPEND states and WAKEUP events for KVM ARM/ARM64.
Due to this, we implement CPU_SUSPEND emulation similar to WFI (Wait-for-interrupt) emulation.
Signed-off-by: Anup Patel anup.patel@linaro.org Signed-off-by: Pranavkumar Sawargaonkar pranavkumar@linaro.org --- arch/arm/kvm/psci.c | 29 +++++++++++++++++++++++++---- 1 file changed, 25 insertions(+), 4 deletions(-)
diff --git a/arch/arm/kvm/psci.c b/arch/arm/kvm/psci.c index 85bf896..f414fd3 100644 --- a/arch/arm/kvm/psci.c +++ b/arch/arm/kvm/psci.c @@ -52,6 +52,27 @@ static unsigned long psci_affinity_mask(unsigned long affinity_level) return affinity_mask; }
+static unsigned long kvm_psci_vcpu_suspend(struct kvm_vcpu *vcpu) +{ + /* + * NOTE: Currently, we don't have any wakeup events for KVM + * so for simplicity we make VCPU suspend emulation same-as + * WFI (Wait-for-interrupt) emulation. + * + * To do this we simply update VCPU registers as-per state + * info provided via r1 - r3 (or x1 - x3) and block the + * VCPU for irqs. + */ + if (*vcpu_reg(vcpu, 1) & (0x1UL << 16)) { + /* Update return pc and r0 for power-down state. */ + *vcpu_pc(vcpu) = *vcpu_reg(vcpu, 2); + *vcpu_reg(vcpu, 0) = *vcpu_reg(vcpu, 3); + } + kvm_vcpu_block(vcpu); + + return PSCI_RET_SUCCESS; +} + static void kvm_psci_vcpu_off(struct kvm_vcpu *vcpu) { vcpu->arch.pause = true; @@ -195,6 +216,10 @@ static int kvm_psci_0_2_call(struct kvm_vcpu *vcpu) */ val = 2; break; + case PSCI_0_2_FN_CPU_SUSPEND: + case PSCI_0_2_FN64_CPU_SUSPEND: + val = kvm_psci_vcpu_suspend(vcpu); + break; case PSCI_0_2_FN_CPU_OFF: kvm_psci_vcpu_off(vcpu); val = PSCI_RET_SUCCESS; @@ -232,10 +257,6 @@ static int kvm_psci_0_2_call(struct kvm_vcpu *vcpu) val = PSCI_RET_SUCCESS; ret = 0; break; - case PSCI_0_2_FN_CPU_SUSPEND: - case PSCI_0_2_FN64_CPU_SUSPEND: - val = PSCI_RET_NOT_SUPPORTED; - break; default: return -EINVAL; }
On Mon, Mar 31, 2014 at 04:16:12PM +0530, Anup Patel wrote:
This patch adds emulation of PSCI v0.2 CPU_SUSPEND function call for KVM ARM/ARM64. This is a CPU-level function call which can suspend current CPU or current CPU cluster. We don't have VCPU clusters in KVM so for KVM we simply suspend the current VCPU.
The CPU_SUSPEND emulation is not tested much because currently there is no CPUIDLE driver in Linux kernel that uses PSCI CPU_SUSPEND. The PSCI CPU_SUSPEND implementation in ARM64 kernel was tested using a Simple CPUIDLE driver which is not published due to unstable DT-bindings for PSCI. (For more info, http://lwn.net/Articles/574950/)
Even if we had stable DT-bindings for PSCI and CPUIDLE driver that uses PSCI CPU_SUSPEND then still we need to define SUSPEND states and WAKEUP events for KVM ARM/ARM64.
Due to this, we implement CPU_SUSPEND emulation similar to WFI (Wait-for-interrupt) emulation.
Signed-off-by: Anup Patel anup.patel@linaro.org Signed-off-by: Pranavkumar Sawargaonkar pranavkumar@linaro.org
arch/arm/kvm/psci.c | 29 +++++++++++++++++++++++++---- 1 file changed, 25 insertions(+), 4 deletions(-)
diff --git a/arch/arm/kvm/psci.c b/arch/arm/kvm/psci.c index 85bf896..f414fd3 100644 --- a/arch/arm/kvm/psci.c +++ b/arch/arm/kvm/psci.c @@ -52,6 +52,27 @@ static unsigned long psci_affinity_mask(unsigned long affinity_level) return affinity_mask; } +static unsigned long kvm_psci_vcpu_suspend(struct kvm_vcpu *vcpu) +{
- /*
* NOTE: Currently, we don't have any wakeup events for KVM* so for simplicity we make VCPU suspend emulation same-as* WFI (Wait-for-interrupt) emulation.
If you implement it like WFI, we do have wake-up events: Namely interrupts.
** To do this we simply update VCPU registers as-per state* info provided via r1 - r3 (or x1 - x3) and block the* VCPU for irqs.*/- if (*vcpu_reg(vcpu, 1) & (0x1UL << 16)) {
/* Update return pc and r0 for power-down state. */*vcpu_pc(vcpu) = *vcpu_reg(vcpu, 2);*vcpu_reg(vcpu, 0) = *vcpu_reg(vcpu, 3);- }
Hmm, this looks wrong. This looks like you're respecting the power-down state request but not resetting the CPU. What I was saying before was that if you implement this as kvm_vcpu_block(), just like WFI, then you need to preserve all state, ignore power-down state requests and treat them as suspend states, implement them as WFI, and put a big fat comment here explaining why this is architecturally valid (by referring to the PSCI 0.2 spec) and what the semantics of doing that is.
-Christoffer
- kvm_vcpu_block(vcpu);
- return PSCI_RET_SUCCESS;
+}
static void kvm_psci_vcpu_off(struct kvm_vcpu *vcpu) { vcpu->arch.pause = true; @@ -195,6 +216,10 @@ static int kvm_psci_0_2_call(struct kvm_vcpu *vcpu) */ val = 2; break;
- case PSCI_0_2_FN_CPU_SUSPEND:
- case PSCI_0_2_FN64_CPU_SUSPEND:
val = kvm_psci_vcpu_suspend(vcpu);break;@@ -232,10 +257,6 @@ static int kvm_psci_0_2_call(struct kvm_vcpu *vcpu) val = PSCI_RET_SUCCESS; ret = 0; break;
- case PSCI_0_2_FN_CPU_SUSPEND:
- case PSCI_0_2_FN64_CPU_SUSPEND:
val = PSCI_RET_NOT_SUPPORTED;break;-- 1.7.9.5
On Mon, Mar 31, 2014 at 11:10 PM, Christoffer Dall christoffer.dall@linaro.org wrote:
On Mon, Mar 31, 2014 at 04:16:12PM +0530, Anup Patel wrote:
This patch adds emulation of PSCI v0.2 CPU_SUSPEND function call for KVM ARM/ARM64. This is a CPU-level function call which can suspend current CPU or current CPU cluster. We don't have VCPU clusters in KVM so for KVM we simply suspend the current VCPU.
The CPU_SUSPEND emulation is not tested much because currently there is no CPUIDLE driver in Linux kernel that uses PSCI CPU_SUSPEND. The PSCI CPU_SUSPEND implementation in ARM64 kernel was tested using a Simple CPUIDLE driver which is not published due to unstable DT-bindings for PSCI. (For more info, http://lwn.net/Articles/574950/)
Even if we had stable DT-bindings for PSCI and CPUIDLE driver that uses PSCI CPU_SUSPEND then still we need to define SUSPEND states and WAKEUP events for KVM ARM/ARM64.
Due to this, we implement CPU_SUSPEND emulation similar to WFI (Wait-for-interrupt) emulation.
Signed-off-by: Anup Patel anup.patel@linaro.org Signed-off-by: Pranavkumar Sawargaonkar pranavkumar@linaro.org
arch/arm/kvm/psci.c | 29 +++++++++++++++++++++++++---- 1 file changed, 25 insertions(+), 4 deletions(-)
diff --git a/arch/arm/kvm/psci.c b/arch/arm/kvm/psci.c index 85bf896..f414fd3 100644 --- a/arch/arm/kvm/psci.c +++ b/arch/arm/kvm/psci.c @@ -52,6 +52,27 @@ static unsigned long psci_affinity_mask(unsigned long affinity_level) return affinity_mask; }
+static unsigned long kvm_psci_vcpu_suspend(struct kvm_vcpu *vcpu) +{
/** NOTE: Currently, we don't have any wakeup events for KVM* so for simplicity we make VCPU suspend emulation same-as* WFI (Wait-for-interrupt) emulation.If you implement it like WFI, we do have wake-up events: Namely interrupts.
** To do this we simply update VCPU registers as-per state* info provided via r1 - r3 (or x1 - x3) and block the* VCPU for irqs.*/if (*vcpu_reg(vcpu, 1) & (0x1UL << 16)) {/* Update return pc and r0 for power-down state. */*vcpu_pc(vcpu) = *vcpu_reg(vcpu, 2);*vcpu_reg(vcpu, 0) = *vcpu_reg(vcpu, 3);}Hmm, this looks wrong. This looks like you're respecting the power-down state request but not resetting the CPU. What I was saying before was that if you implement this as kvm_vcpu_block(), just like WFI, then you need to preserve all state, ignore power-down state requests and treat them as suspend states, implement them as WFI, and put a big fat comment here explaining why this is architecturally valid (by referring to the PSCI 0.2 spec) and what the semantics of doing that is.
Actually, I was more inclined towards preserving the VCPU context for power-down request but as-per section 5.4.2 clause 3 we can treat power-down request to be same as suspend request.
I update this patch accordingly.
Regards, Anup
-Christoffer
kvm_vcpu_block(vcpu);return PSCI_RET_SUCCESS;+}
static void kvm_psci_vcpu_off(struct kvm_vcpu *vcpu) { vcpu->arch.pause = true; @@ -195,6 +216,10 @@ static int kvm_psci_0_2_call(struct kvm_vcpu *vcpu) */ val = 2; break;
case PSCI_0_2_FN_CPU_SUSPEND:case PSCI_0_2_FN64_CPU_SUSPEND:val = kvm_psci_vcpu_suspend(vcpu);break; case PSCI_0_2_FN_CPU_OFF: kvm_psci_vcpu_off(vcpu); val = PSCI_RET_SUCCESS;@@ -232,10 +257,6 @@ static int kvm_psci_0_2_call(struct kvm_vcpu *vcpu) val = PSCI_RET_SUCCESS; ret = 0; break;
case PSCI_0_2_FN_CPU_SUSPEND:case PSCI_0_2_FN64_CPU_SUSPEND:val = PSCI_RET_NOT_SUPPORTED;break; default: return -EINVAL; }-- 1.7.9.5
kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
We have PSCI v0.2 emulation available in KVM ARM/ARM64 hence advertise this to user space (i.e. QEMU or KVMTOOL) via KVM_CHECK_EXTENSION ioctl.
Signed-off-by: Anup Patel anup.patel@linaro.org Signed-off-by: Pranavkumar Sawargaonkar pranavkumar@linaro.org Acked-by: Christoffer Dall christoffer.dall@linaro.org --- arch/arm/kvm/arm.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c index bd18bb8..2076605 100644 --- a/arch/arm/kvm/arm.c +++ b/arch/arm/kvm/arm.c @@ -197,6 +197,7 @@ int kvm_dev_ioctl_check_extension(long ext) case KVM_CAP_DESTROY_MEMORY_REGION_WORKS: case KVM_CAP_ONE_REG: case KVM_CAP_ARM_PSCI: + case KVM_CAP_ARM_PSCI_0_2: r = 1; break; case KVM_CAP_COALESCED_MMIO:
linaro-kernel@lists.linaro.org
-
Anup Patel -
Anup Patel -
Ashwin Chaugule -
Christoffer Dall -
Rob Herring