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

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

[Anup Patel]

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_SHUTDOWN and KVM_EXIT_RESET exit reasons.

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)

Anup Patel (2):
  KVM: Add KVM_EXIT_RESET to user space API header
  ARM/ARM64: KVM: Forward PSCI SYSTEM_OFF and SYSTEM_RESET to user
    space

 arch/arm/include/asm/kvm_psci.h   |    2 +-
 arch/arm/include/uapi/asm/kvm.h   |    2 ++
 arch/arm/kvm/handle_exit.c        |    7 ++++++-
 arch/arm/kvm/psci.c               |   38 +++++++++++++++++++++++++++++--------
 arch/arm64/include/asm/kvm_psci.h |    2 +-
 arch/arm64/include/uapi/asm/kvm.h |    2 ++
 arch/arm64/kvm/handle_exit.c      |   10 ++++++----
 include/uapi/linux/kvm.h          |    1 +
 8 files changed, 49 insertions(+), 15 deletions(-)

-- 
1.7.9.5

[PATCH 1/2] KVM: Add KVM_EXIT_RESET to user space API header

[Anup Patel]

Currently, we don't have an exit reason for VM reset emulation
in user space hence this patch adds exit reason KVM_EXIT_RESET
for this purpose.

This newly added KVM_EXIT_RESET will be used by KVM arm/arm64
in-kernel PSCI support to reset VMs.

Signed-off-by: Anup Patel <anup.patel@linaro.org>
Signed-off-by: Pranavkumar Sawargaonkar <pranavkumar@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 902f124..64a04cc 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_RESET            24
 
 /* For KVM_EXIT_INTERNAL_ERROR */
 /* Emulate instruction failed. */
-- 
1.7.9.5

[PATCH 2/2] ARM/ARM64: KVM: Forward PSCI SYSTEM_OFF and SYSTEM_RESET to user space

[Anup Patel]

The PSCI SYSTEM_OFF and SYSTEM_RESET functions are system-level
functions hence cannot be fully emulated by the in-kernel PSCI
emulation code.

To tackle this, we forward PSCI SYSTEM_OFF and SYSTEM_RESET function
calls from guest to user space (i.e. QEMU or KVMTOOL) via KVM run
structure using KVM_EXIT_SHUTDOWN and KVM_EXIT_RESET exit reasons.

Signed-off-by: Anup Patel <anup.patel@linaro.org>
Signed-off-by: Pranavkumar Sawargaonkar <pranavkumar@linaro.org>
---
 arch/arm/include/asm/kvm_psci.h   |    2 +-
 arch/arm/include/uapi/asm/kvm.h   |    2 ++
 arch/arm/kvm/handle_exit.c        |    7 ++++++-
 arch/arm/kvm/psci.c               |   38 +++++++++++++++++++++++++++++--------
 arch/arm64/include/asm/kvm_psci.h |    2 +-
 arch/arm64/include/uapi/asm/kvm.h |    2 ++
 arch/arm64/kvm/handle_exit.c      |   10 ++++++----
 7 files changed, 48 insertions(+), 15 deletions(-)

diff --git a/arch/arm/include/asm/kvm_psci.h b/arch/arm/include/asm/kvm_psci.h
index 9a83d98..992d7f1 100644
--- a/arch/arm/include/asm/kvm_psci.h
+++ b/arch/arm/include/asm/kvm_psci.h
@@ -18,6 +18,6 @@
 #ifndef __ARM_KVM_PSCI_H__
 #define __ARM_KVM_PSCI_H__
 
-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/include/uapi/asm/kvm.h b/arch/arm/include/uapi/asm/kvm.h
index c498b60..f4de20c 100644
--- a/arch/arm/include/uapi/asm/kvm.h
+++ b/arch/arm/include/uapi/asm/kvm.h
@@ -172,6 +172,8 @@ struct kvm_arch_memory_slot {
 #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_SYSTEM_OFF		KVM_PSCI_FN(4)
+#define KVM_PSCI_FN_SYSTEM_RESET	KVM_PSCI_FN(5)
 
 #define KVM_PSCI_RET_SUCCESS		0
 #define KVM_PSCI_RET_NI			((unsigned long)-1)
diff --git a/arch/arm/kvm/handle_exit.c b/arch/arm/kvm/handle_exit.c
index a920790..c96c7e8 100644
--- a/arch/arm/kvm/handle_exit.c
+++ b/arch/arm/kvm/handle_exit.c
@@ -40,11 +40,16 @@ 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 == 0)
 		return 1;
+	else if (ret > 0)
+		return 0;
 
 	kvm_inject_undefined(vcpu);
 	return 1;
diff --git a/arch/arm/kvm/psci.c b/arch/arm/kvm/psci.c
index 0881bf1..c8d3fcd 100644
--- a/arch/arm/kvm/psci.c
+++ b/arch/arm/kvm/psci.c
@@ -84,18 +84,31 @@ static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu)
 	return KVM_PSCI_RET_SUCCESS;
 }
 
+static void kvm_psci_system_off(struct kvm_vcpu *vcpu)
+{
+	vcpu->run->exit_reason = KVM_EXIT_SHUTDOWN;
+}
+
+static void kvm_psci_system_reset(struct kvm_vcpu *vcpu)
+{
+	vcpu->run->exit_reason = KVM_EXIT_RESET;
+}
+
 /**
  * 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.
+ * The calling convention is similar to SMC calls to the secure world
+ * where the function number is placed in r0 and function number
+ * specified in r0 is withing the PSCI range.
+ *
+ * This function returns: 0 (success), > 0 (success but exit to user
+ * space), and < 0 (failure)
  */
-bool kvm_psci_call(struct kvm_vcpu *vcpu)
+int kvm_psci_call(struct kvm_vcpu *vcpu)
 {
+	int ret = 0;
 	unsigned long psci_fn = *vcpu_reg(vcpu, 0) & ~((u32) 0);
 	unsigned long val;
 
@@ -111,11 +124,20 @@ bool kvm_psci_call(struct kvm_vcpu *vcpu)
 	case KVM_PSCI_FN_MIGRATE:
 		val = KVM_PSCI_RET_NI;
 		break;
-
+	case KVM_PSCI_FN_SYSTEM_OFF:
+		kvm_psci_system_off(vcpu);
+		val = KVM_PSCI_RET_SUCCESS;
+		ret = 1;
+		break;
+	case KVM_PSCI_FN_SYSTEM_RESET:
+		kvm_psci_system_reset(vcpu);
+		val = KVM_PSCI_RET_SUCCESS;
+		ret = 1;
+		break;
 	default:
-		return false;
+		return -EINVAL;
 	}
 
 	*vcpu_reg(vcpu, 0) = val;
-	return true;
+	return ret;
 }
diff --git a/arch/arm64/include/asm/kvm_psci.h b/arch/arm64/include/asm/kvm_psci.h
index e301a48..9bd0ee8 100644
--- a/arch/arm64/include/asm/kvm_psci.h
+++ b/arch/arm64/include/asm/kvm_psci.h
@@ -18,6 +18,6 @@
 #ifndef __ARM64_KVM_PSCI_H__
 #define __ARM64_KVM_PSCI_H__
 
-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/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
index d9f026b..f678902 100644
--- a/arch/arm64/include/uapi/asm/kvm.h
+++ b/arch/arm64/include/uapi/asm/kvm.h
@@ -158,6 +158,8 @@ struct kvm_arch_memory_slot {
 #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_SYSTEM_OFF		KVM_PSCI_FN(4)
+#define KVM_PSCI_FN_SYSTEM_RESET	KVM_PSCI_FN(5)
 
 #define KVM_PSCI_RET_SUCCESS		0
 #define KVM_PSCI_RET_NI			((unsigned long)-1)
diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
index 8da5606..a5b5b6a 100644
--- a/arch/arm64/kvm/handle_exit.c
+++ b/arch/arm64/kvm/handle_exit.c
@@ -30,8 +30,13 @@ 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 == 0)
 		return 1;
+	else if (ret > 0)
+		return 0;
 
 	kvm_inject_undefined(vcpu);
 	return 1;
@@ -39,9 +44,6 @@ static int handle_hvc(struct kvm_vcpu *vcpu, struct kvm_run *run)
 
 static int handle_smc(struct kvm_vcpu *vcpu, struct kvm_run *run)
 {
-	if (kvm_psci_call(vcpu))
-		return 1;
-
 	kvm_inject_undefined(vcpu);
 	return 1;
 }
-- 
1.7.9.5

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

[Anup Patel]

On 25 November 2013 21:19, Anup Patel <anup.patel@linaro.org> 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_SHUTDOWN and KVM_EXIT_RESET exit reasons.
>
> 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)
>
> Anup Patel (2):
>   KVM: Add KVM_EXIT_RESET to user space API header
>   ARM/ARM64: KVM: Forward PSCI SYSTEM_OFF and SYSTEM_RESET to user
>     space
>
>  arch/arm/include/asm/kvm_psci.h   |    2 +-
>  arch/arm/include/uapi/asm/kvm.h   |    2 ++
>  arch/arm/kvm/handle_exit.c        |    7 ++++++-
>  arch/arm/kvm/psci.c               |   38 +++++++++++++++++++++++++++++--------
>  arch/arm64/include/asm/kvm_psci.h |    2 +-
>  arch/arm64/include/uapi/asm/kvm.h |    2 ++
>  arch/arm64/kvm/handle_exit.c      |   10 ++++++----
>  include/uapi/linux/kvm.h          |    1 +
>  8 files changed, 49 insertions(+), 15 deletions(-)
>
> --
> 1.7.9.5
>

Hi All,

If anyone wants to try this patches using KVMTOOL then they
can find guest kernel side PSCI patches and KVMTOOL patch
attached here.

Regards,
Anup
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0006-ARM-psci-Add-support-for-system-reboot-and-poweroff.patch
Type: text/x-diff
Size: 2340 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20131125/a3142151/attachment-0003.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0007-ARM64-psci-Add-support-for-system-reboot-and-powerof.patch
Type: text/x-diff
Size: 2381 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20131125/a3142151/attachment-0004.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: kvmtool_psci_emulation.patch
Type: text/x-diff
Size: 4544 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20131125/a3142151/attachment-0005.bin>

[PATCH 2/2] ARM/ARM64: KVM: Forward PSCI SYSTEM_OFF and SYSTEM_RESET to user space

[Anup Patel]

(Adding Christoffer, forgot to add him to CC)

On Mon, Nov 25, 2013 at 9:19 PM, Anup Patel <anup.patel@linaro.org> wrote:
> The PSCI SYSTEM_OFF and SYSTEM_RESET functions are system-level
> functions hence cannot be fully emulated by the in-kernel PSCI
> emulation code.
>
> To tackle this, we forward PSCI SYSTEM_OFF and SYSTEM_RESET function
> calls from guest to user space (i.e. QEMU or KVMTOOL) via KVM run
> structure using KVM_EXIT_SHUTDOWN and KVM_EXIT_RESET exit reasons.
>
> Signed-off-by: Anup Patel <anup.patel@linaro.org>
> Signed-off-by: Pranavkumar Sawargaonkar <pranavkumar@linaro.org>
> ---
>  arch/arm/include/asm/kvm_psci.h   |    2 +-
>  arch/arm/include/uapi/asm/kvm.h   |    2 ++
>  arch/arm/kvm/handle_exit.c        |    7 ++++++-
>  arch/arm/kvm/psci.c               |   38 +++++++++++++++++++++++++++++--------
>  arch/arm64/include/asm/kvm_psci.h |    2 +-
>  arch/arm64/include/uapi/asm/kvm.h |    2 ++
>  arch/arm64/kvm/handle_exit.c      |   10 ++++++----
>  7 files changed, 48 insertions(+), 15 deletions(-)
>
> diff --git a/arch/arm/include/asm/kvm_psci.h b/arch/arm/include/asm/kvm_psci.h
> index 9a83d98..992d7f1 100644
> --- a/arch/arm/include/asm/kvm_psci.h
> +++ b/arch/arm/include/asm/kvm_psci.h
> @@ -18,6 +18,6 @@
>  #ifndef __ARM_KVM_PSCI_H__
>  #define __ARM_KVM_PSCI_H__
>
> -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/include/uapi/asm/kvm.h b/arch/arm/include/uapi/asm/kvm.h
> index c498b60..f4de20c 100644
> --- a/arch/arm/include/uapi/asm/kvm.h
> +++ b/arch/arm/include/uapi/asm/kvm.h
> @@ -172,6 +172,8 @@ struct kvm_arch_memory_slot {
>  #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_SYSTEM_OFF         KVM_PSCI_FN(4)
> +#define KVM_PSCI_FN_SYSTEM_RESET       KVM_PSCI_FN(5)
>
>  #define KVM_PSCI_RET_SUCCESS           0
>  #define KVM_PSCI_RET_NI                        ((unsigned long)-1)
> diff --git a/arch/arm/kvm/handle_exit.c b/arch/arm/kvm/handle_exit.c
> index a920790..c96c7e8 100644
> --- a/arch/arm/kvm/handle_exit.c
> +++ b/arch/arm/kvm/handle_exit.c
> @@ -40,11 +40,16 @@ 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 == 0)
>                 return 1;
> +       else if (ret > 0)
> +               return 0;
>
>         kvm_inject_undefined(vcpu);
>         return 1;
> diff --git a/arch/arm/kvm/psci.c b/arch/arm/kvm/psci.c
> index 0881bf1..c8d3fcd 100644
> --- a/arch/arm/kvm/psci.c
> +++ b/arch/arm/kvm/psci.c
> @@ -84,18 +84,31 @@ static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu)
>         return KVM_PSCI_RET_SUCCESS;
>  }
>
> +static void kvm_psci_system_off(struct kvm_vcpu *vcpu)
> +{
> +       vcpu->run->exit_reason = KVM_EXIT_SHUTDOWN;
> +}
> +
> +static void kvm_psci_system_reset(struct kvm_vcpu *vcpu)
> +{
> +       vcpu->run->exit_reason = KVM_EXIT_RESET;
> +}
> +
>  /**
>   * 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.
> + * The calling convention is similar to SMC calls to the secure world
> + * where the function number is placed in r0 and function number
> + * specified in r0 is withing the PSCI range.
> + *
> + * This function returns: 0 (success), > 0 (success but exit to user
> + * space), and < 0 (failure)
>   */
> -bool kvm_psci_call(struct kvm_vcpu *vcpu)
> +int kvm_psci_call(struct kvm_vcpu *vcpu)
>  {
> +       int ret = 0;
>         unsigned long psci_fn = *vcpu_reg(vcpu, 0) & ~((u32) 0);
>         unsigned long val;
>
> @@ -111,11 +124,20 @@ bool kvm_psci_call(struct kvm_vcpu *vcpu)
>         case KVM_PSCI_FN_MIGRATE:
>                 val = KVM_PSCI_RET_NI;
>                 break;
> -
> +       case KVM_PSCI_FN_SYSTEM_OFF:
> +               kvm_psci_system_off(vcpu);
> +               val = KVM_PSCI_RET_SUCCESS;
> +               ret = 1;
> +               break;
> +       case KVM_PSCI_FN_SYSTEM_RESET:
> +               kvm_psci_system_reset(vcpu);
> +               val = KVM_PSCI_RET_SUCCESS;
> +               ret = 1;
> +               break;
>         default:
> -               return false;
> +               return -EINVAL;
>         }
>
>         *vcpu_reg(vcpu, 0) = val;
> -       return true;
> +       return ret;
>  }
> diff --git a/arch/arm64/include/asm/kvm_psci.h b/arch/arm64/include/asm/kvm_psci.h
> index e301a48..9bd0ee8 100644
> --- a/arch/arm64/include/asm/kvm_psci.h
> +++ b/arch/arm64/include/asm/kvm_psci.h
> @@ -18,6 +18,6 @@
>  #ifndef __ARM64_KVM_PSCI_H__
>  #define __ARM64_KVM_PSCI_H__
>
> -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/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
> index d9f026b..f678902 100644
> --- a/arch/arm64/include/uapi/asm/kvm.h
> +++ b/arch/arm64/include/uapi/asm/kvm.h
> @@ -158,6 +158,8 @@ struct kvm_arch_memory_slot {
>  #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_SYSTEM_OFF         KVM_PSCI_FN(4)
> +#define KVM_PSCI_FN_SYSTEM_RESET       KVM_PSCI_FN(5)
>
>  #define KVM_PSCI_RET_SUCCESS           0
>  #define KVM_PSCI_RET_NI                        ((unsigned long)-1)
> diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
> index 8da5606..a5b5b6a 100644
> --- a/arch/arm64/kvm/handle_exit.c
> +++ b/arch/arm64/kvm/handle_exit.c
> @@ -30,8 +30,13 @@ 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 == 0)
>                 return 1;
> +       else if (ret > 0)
> +               return 0;
>
>         kvm_inject_undefined(vcpu);
>         return 1;
> @@ -39,9 +44,6 @@ static int handle_hvc(struct kvm_vcpu *vcpu, struct kvm_run *run)
>
>  static int handle_smc(struct kvm_vcpu *vcpu, struct kvm_run *run)
>  {
> -       if (kvm_psci_call(vcpu))
> -               return 1;
> -
>         kvm_inject_undefined(vcpu);
>         return 1;
>  }
> --
> 1.7.9.5
>
> _______________________________________________
> kvmarm mailing list
> kvmarm at lists.cs.columbia.edu
> https://lists.cs.columbia.edu/cucslists/listinfo/kvmarm

[PATCH 1/2] KVM: Add KVM_EXIT_RESET to user space API header

[Anup Patel]

(Adding Christoffer, forgot to add him to CC)

On Mon, Nov 25, 2013 at 9:19 PM, Anup Patel <anup.patel@linaro.org> wrote:
> Currently, we don't have an exit reason for VM reset emulation
> in user space hence this patch adds exit reason KVM_EXIT_RESET
> for this purpose.
>
> This newly added KVM_EXIT_RESET will be used by KVM arm/arm64
> in-kernel PSCI support to reset VMs.
>
> Signed-off-by: Anup Patel <anup.patel@linaro.org>
> Signed-off-by: Pranavkumar Sawargaonkar <pranavkumar@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 902f124..64a04cc 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_RESET            24
>
>  /* For KVM_EXIT_INTERNAL_ERROR */
>  /* Emulate instruction failed. */
> --
> 1.7.9.5
>
> _______________________________________________
> kvmarm mailing list
> kvmarm at lists.cs.columbia.edu
> https://lists.cs.columbia.edu/cucslists/listinfo/kvmarm

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

[Anup Patel]

(Adding Christoffer, forgot to add him to CC)

On Mon, Nov 25, 2013 at 9:27 PM, Anup Patel <anup.patel@linaro.org> wrote:
> On 25 November 2013 21:19, Anup Patel <anup.patel@linaro.org> 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_SHUTDOWN and KVM_EXIT_RESET exit reasons.
>>
>> 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)
>>
>> Anup Patel (2):
>>   KVM: Add KVM_EXIT_RESET to user space API header
>>   ARM/ARM64: KVM: Forward PSCI SYSTEM_OFF and SYSTEM_RESET to user
>>     space
>>
>>  arch/arm/include/asm/kvm_psci.h   |    2 +-
>>  arch/arm/include/uapi/asm/kvm.h   |    2 ++
>>  arch/arm/kvm/handle_exit.c        |    7 ++++++-
>>  arch/arm/kvm/psci.c               |   38 +++++++++++++++++++++++++++++--------
>>  arch/arm64/include/asm/kvm_psci.h |    2 +-
>>  arch/arm64/include/uapi/asm/kvm.h |    2 ++
>>  arch/arm64/kvm/handle_exit.c      |   10 ++++++----
>>  include/uapi/linux/kvm.h          |    1 +
>>  8 files changed, 49 insertions(+), 15 deletions(-)
>>
>> --
>> 1.7.9.5
>>
>
> Hi All,
>
> If anyone wants to try this patches using KVMTOOL then they
> can find guest kernel side PSCI patches and KVMTOOL patch
> attached here.
>
> Regards,
> Anup
>
> _______________________________________________
> kvmarm mailing list
> kvmarm at lists.cs.columbia.edu
> https://lists.cs.columbia.edu/cucslists/listinfo/kvmarm

[PATCH 2/2] ARM/ARM64: KVM: Forward PSCI SYSTEM_OFF and SYSTEM_RESET to user space

[Christoffer Dall]

On Mon, Nov 25, 2013 at 09:19:59PM +0530, Anup Patel wrote:
> The PSCI SYSTEM_OFF and SYSTEM_RESET functions are system-level
> functions hence cannot be fully emulated by the in-kernel PSCI
> emulation code.
> 
> To tackle this, we forward PSCI SYSTEM_OFF and SYSTEM_RESET function
> calls from guest to user space (i.e. QEMU or KVMTOOL) via KVM run
> structure using KVM_EXIT_SHUTDOWN and KVM_EXIT_RESET exit reasons.
> 
> Signed-off-by: Anup Patel <anup.patel@linaro.org>
> Signed-off-by: Pranavkumar Sawargaonkar <pranavkumar@linaro.org>
> ---
>  arch/arm/include/asm/kvm_psci.h   |    2 +-
>  arch/arm/include/uapi/asm/kvm.h   |    2 ++
>  arch/arm/kvm/handle_exit.c        |    7 ++++++-
>  arch/arm/kvm/psci.c               |   38 +++++++++++++++++++++++++++++--------
>  arch/arm64/include/asm/kvm_psci.h |    2 +-
>  arch/arm64/include/uapi/asm/kvm.h |    2 ++
>  arch/arm64/kvm/handle_exit.c      |   10 ++++++----
>  7 files changed, 48 insertions(+), 15 deletions(-)
> 
> diff --git a/arch/arm/include/asm/kvm_psci.h b/arch/arm/include/asm/kvm_psci.h
> index 9a83d98..992d7f1 100644
> --- a/arch/arm/include/asm/kvm_psci.h
> +++ b/arch/arm/include/asm/kvm_psci.h
> @@ -18,6 +18,6 @@
>  #ifndef __ARM_KVM_PSCI_H__
>  #define __ARM_KVM_PSCI_H__
>  
> -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/include/uapi/asm/kvm.h b/arch/arm/include/uapi/asm/kvm.h
> index c498b60..f4de20c 100644
> --- a/arch/arm/include/uapi/asm/kvm.h
> +++ b/arch/arm/include/uapi/asm/kvm.h
> @@ -172,6 +172,8 @@ struct kvm_arch_memory_slot {
>  #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_SYSTEM_OFF		KVM_PSCI_FN(4)
> +#define KVM_PSCI_FN_SYSTEM_RESET	KVM_PSCI_FN(5)
>  
>  #define KVM_PSCI_RET_SUCCESS		0
>  #define KVM_PSCI_RET_NI			((unsigned long)-1)
> diff --git a/arch/arm/kvm/handle_exit.c b/arch/arm/kvm/handle_exit.c
> index a920790..c96c7e8 100644
> --- a/arch/arm/kvm/handle_exit.c
> +++ b/arch/arm/kvm/handle_exit.c
> @@ -40,11 +40,16 @@ 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 == 0)
>  		return 1;
> +	else if (ret > 0)
> +		return 0;
>  
>  	kvm_inject_undefined(vcpu);
>  	return 1;
> diff --git a/arch/arm/kvm/psci.c b/arch/arm/kvm/psci.c
> index 0881bf1..c8d3fcd 100644
> --- a/arch/arm/kvm/psci.c
> +++ b/arch/arm/kvm/psci.c
> @@ -84,18 +84,31 @@ static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu)
>  	return KVM_PSCI_RET_SUCCESS;
>  }
>  
> +static void kvm_psci_system_off(struct kvm_vcpu *vcpu)
> +{
> +	vcpu->run->exit_reason = KVM_EXIT_SHUTDOWN;
> +}
> +
> +static void kvm_psci_system_reset(struct kvm_vcpu *vcpu)
> +{
> +	vcpu->run->exit_reason = KVM_EXIT_RESET;
> +}
> +
>  /**
>   * 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.
> + * The calling convention is similar to SMC calls to the secure world
> + * where the function number is placed in r0 and function number
> + * specified in r0 is withing the PSCI range.
> + *
> + * This function returns: 0 (success), > 0 (success but exit to user
> + * space), and < 0 (failure)

Hmmm, I think this is a bit confusing because it essentially does the
reverse of all the other kvm internal return convention.  Wouldn't it
make more sense to have 0 (success, but exit to user space), 1 (success,
resume VM), and < 0 (failure).

Also note that you are general-casing all error codes that may happen to
mean that it did not match the PSCI function to anything well-known.
In this case, that's sort of ok, because we don't expect the PSCI code
to randomly fail (if it does, it reports it to the caller in the return
register), but not fail, as in, I'm out of memory or anything like that.
But I think you can clarify this in the comment, by saying something
like:

Errors:
 -EINVAL: Unrecognized PSCI function

in the comment for the function.

>   */
> -bool kvm_psci_call(struct kvm_vcpu *vcpu)
> +int kvm_psci_call(struct kvm_vcpu *vcpu)
>  {
> +	int ret = 0;
>  	unsigned long psci_fn = *vcpu_reg(vcpu, 0) & ~((u32) 0);
>  	unsigned long val;
>  
> @@ -111,11 +124,20 @@ bool kvm_psci_call(struct kvm_vcpu *vcpu)
>  	case KVM_PSCI_FN_MIGRATE:
>  		val = KVM_PSCI_RET_NI;
>  		break;
> -
> +	case KVM_PSCI_FN_SYSTEM_OFF:
> +		kvm_psci_system_off(vcpu);
> +		val = KVM_PSCI_RET_SUCCESS;
> +		ret = 1;
> +		break;
> +	case KVM_PSCI_FN_SYSTEM_RESET:
> +		kvm_psci_system_reset(vcpu);
> +		val = KVM_PSCI_RET_SUCCESS;
> +		ret = 1;
> +		break;
>  	default:
> -		return false;
> +		return -EINVAL;
>  	}
>  
>  	*vcpu_reg(vcpu, 0) = val;
> -	return true;
> +	return ret;
>  }
> diff --git a/arch/arm64/include/asm/kvm_psci.h b/arch/arm64/include/asm/kvm_psci.h
> index e301a48..9bd0ee8 100644
> --- a/arch/arm64/include/asm/kvm_psci.h
> +++ b/arch/arm64/include/asm/kvm_psci.h
> @@ -18,6 +18,6 @@
>  #ifndef __ARM64_KVM_PSCI_H__
>  #define __ARM64_KVM_PSCI_H__
>  
> -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/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
> index d9f026b..f678902 100644
> --- a/arch/arm64/include/uapi/asm/kvm.h
> +++ b/arch/arm64/include/uapi/asm/kvm.h
> @@ -158,6 +158,8 @@ struct kvm_arch_memory_slot {
>  #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_SYSTEM_OFF		KVM_PSCI_FN(4)
> +#define KVM_PSCI_FN_SYSTEM_RESET	KVM_PSCI_FN(5)
>  
>  #define KVM_PSCI_RET_SUCCESS		0
>  #define KVM_PSCI_RET_NI			((unsigned long)-1)
> diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
> index 8da5606..a5b5b6a 100644
> --- a/arch/arm64/kvm/handle_exit.c
> +++ b/arch/arm64/kvm/handle_exit.c
> @@ -30,8 +30,13 @@ 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 == 0)
>  		return 1;
> +	else if (ret > 0)
> +		return 0;
>  
>  	kvm_inject_undefined(vcpu);
>  	return 1;
> @@ -39,9 +44,6 @@ static int handle_hvc(struct kvm_vcpu *vcpu, struct kvm_run *run)
>  
>  static int handle_smc(struct kvm_vcpu *vcpu, struct kvm_run *run)
>  {
> -	if (kvm_psci_call(vcpu))
> -		return 1;
> -

Isn't this an unrelated change, which should go in a separate patch with
some justification text for the change?

>  	kvm_inject_undefined(vcpu);
>  	return 1;
>  }
> -- 
> 1.7.9.5
> 

Otherwise, it looks overall to go in the right direction.

-Christoffer

[PATCH 1/2] KVM: Add KVM_EXIT_RESET to user space API header

[Christoffer Dall]

On Mon, Nov 25, 2013 at 09:19:58PM +0530, Anup Patel wrote:
> Currently, we don't have an exit reason for VM reset emulation
> in user space hence this patch adds exit reason KVM_EXIT_RESET
> for this purpose.
> 
> This newly added KVM_EXIT_RESET will be used by KVM arm/arm64
> in-kernel PSCI support to reset VMs.

Acked-by: Christoffer Dall <christoffer.dall@linaro.org>

> 
> Signed-off-by: Anup Patel <anup.patel@linaro.org>
> Signed-off-by: Pranavkumar Sawargaonkar <pranavkumar@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 902f124..64a04cc 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_RESET            24
>  
>  /* For KVM_EXIT_INTERNAL_ERROR */
>  /* Emulate instruction failed. */
> -- 
> 1.7.9.5
> 

[PATCH 1/2] KVM: Add KVM_EXIT_RESET to user space API header

[Alexander Graf]

On 25.11.2013, at 16:49, Anup Patel <anup.patel@linaro.org> wrote:

> Currently, we don't have an exit reason for VM reset emulation
> in user space hence this patch adds exit reason KVM_EXIT_RESET
> for this purpose.
> 
> This newly added KVM_EXIT_RESET will be used by KVM arm/arm64
> in-kernel PSCI support to reset VMs.
> 
> Signed-off-by: Anup Patel <anup.patel@linaro.org>
> Signed-off-by: Pranavkumar Sawargaonkar <pranavkumar@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 902f124..64a04cc 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_RESET            24

I have to admit that I'm not particularly happy with the exit name. It's not obvious from the name under which circumstances it gets triggered. Does it get triggered when a core level reset happens? Does it get triggered when a system level reset happened? When the guest requests one?

I know what it does, but I find the name too generic for what it is. What you're really doing is introduce a new communication channel in parallel to MMIO / PIO / HCALL which is only used for system level reset / shutdown today.

Can we treat it as such? Could you please make this a common exit number that's called something like

  KVM_EXIT_SYSTEM_EVENT

with a parameter that can either be TRIGGER_SHUTDOWN or TRIGGER_RESET.

That way it's obvious what's going on and people don't get confused.


Alex

[PATCH 1/2] KVM: Add KVM_EXIT_RESET to user space API header

[Anup Patel]

On Tue, Dec 10, 2013 at 7:43 AM, Alexander Graf <agraf@suse.de> wrote:
>
> On 25.11.2013, at 16:49, Anup Patel <anup.patel@linaro.org> wrote:
>
>> Currently, we don't have an exit reason for VM reset emulation
>> in user space hence this patch adds exit reason KVM_EXIT_RESET
>> for this purpose.
>>
>> This newly added KVM_EXIT_RESET will be used by KVM arm/arm64
>> in-kernel PSCI support to reset VMs.
>>
>> Signed-off-by: Anup Patel <anup.patel@linaro.org>
>> Signed-off-by: Pranavkumar Sawargaonkar <pranavkumar@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 902f124..64a04cc 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_RESET            24
>
> I have to admit that I'm not particularly happy with the exit name. It's not obvious from the name under which circumstances it gets triggered. Does it get triggered when a core level reset happens? Does it get triggered when a system level reset happened? When the guest requests one?

The KVM_EXIT_RESET gets triggered when system level reset is
initiated by VCPU. For arm/arm64, this is through SYSTEM_RESET
PSCI call. In KVM x86 SVM/VMX, we have KVM_EXIT_SHUTDOWN
being used for system shutdown which we have re-used for arm/arm64.

>
> I know what it does, but I find the name too generic for what it is. What you're really doing is introduce a new communication channel in parallel to MMIO / PIO / HCALL which is only used for system level reset / shutdown today.
>
> Can we treat it as such? Could you please make this a common exit number that's called something like
>
>   KVM_EXIT_SYSTEM_EVENT
>
> with a parameter that can either be TRIGGER_SHUTDOWN or TRIGGER_RESET.
>
> That way it's obvious what's going on and people don't get confused.

I don't foresee any system level operations other than SHUTDOWN
and RESET to be handled from KVM in-kernel code but I might be
wrong.

May be we can rename KVM_EXIT_SHUTDOWN and KVM_EXIT_RESET
to KVM_EXIT_SYSTEM_SHUTDOWN and KVM_EXIT_SYSTEM_RESET ??

--
Anup

>
>
> Alex
>
>
> _______________________________________________
> kvmarm mailing list
> kvmarm at lists.cs.columbia.edu
> https://lists.cs.columbia.edu/cucslists/listinfo/kvmarm

[PATCH 2/2] ARM/ARM64: KVM: Forward PSCI SYSTEM_OFF and SYSTEM_RESET to user space

[Anup Patel]

On Tue, Dec 10, 2013 at 4:21 AM, Christoffer Dall
<christoffer.dall@linaro.org> wrote:
> On Mon, Nov 25, 2013 at 09:19:59PM +0530, Anup Patel wrote:
>> The PSCI SYSTEM_OFF and SYSTEM_RESET functions are system-level
>> functions hence cannot be fully emulated by the in-kernel PSCI
>> emulation code.
>>
>> To tackle this, we forward PSCI SYSTEM_OFF and SYSTEM_RESET function
>> calls from guest to user space (i.e. QEMU or KVMTOOL) via KVM run
>> structure using KVM_EXIT_SHUTDOWN and KVM_EXIT_RESET exit reasons.
>>
>> Signed-off-by: Anup Patel <anup.patel@linaro.org>
>> Signed-off-by: Pranavkumar Sawargaonkar <pranavkumar@linaro.org>
>> ---
>>  arch/arm/include/asm/kvm_psci.h   |    2 +-
>>  arch/arm/include/uapi/asm/kvm.h   |    2 ++
>>  arch/arm/kvm/handle_exit.c        |    7 ++++++-
>>  arch/arm/kvm/psci.c               |   38 +++++++++++++++++++++++++++++--------
>>  arch/arm64/include/asm/kvm_psci.h |    2 +-
>>  arch/arm64/include/uapi/asm/kvm.h |    2 ++
>>  arch/arm64/kvm/handle_exit.c      |   10 ++++++----
>>  7 files changed, 48 insertions(+), 15 deletions(-)
>>
>> diff --git a/arch/arm/include/asm/kvm_psci.h b/arch/arm/include/asm/kvm_psci.h
>> index 9a83d98..992d7f1 100644
>> --- a/arch/arm/include/asm/kvm_psci.h
>> +++ b/arch/arm/include/asm/kvm_psci.h
>> @@ -18,6 +18,6 @@
>>  #ifndef __ARM_KVM_PSCI_H__
>>  #define __ARM_KVM_PSCI_H__
>>
>> -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/include/uapi/asm/kvm.h b/arch/arm/include/uapi/asm/kvm.h
>> index c498b60..f4de20c 100644
>> --- a/arch/arm/include/uapi/asm/kvm.h
>> +++ b/arch/arm/include/uapi/asm/kvm.h
>> @@ -172,6 +172,8 @@ struct kvm_arch_memory_slot {
>>  #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_SYSTEM_OFF               KVM_PSCI_FN(4)
>> +#define KVM_PSCI_FN_SYSTEM_RESET     KVM_PSCI_FN(5)
>>
>>  #define KVM_PSCI_RET_SUCCESS         0
>>  #define KVM_PSCI_RET_NI                      ((unsigned long)-1)
>> diff --git a/arch/arm/kvm/handle_exit.c b/arch/arm/kvm/handle_exit.c
>> index a920790..c96c7e8 100644
>> --- a/arch/arm/kvm/handle_exit.c
>> +++ b/arch/arm/kvm/handle_exit.c
>> @@ -40,11 +40,16 @@ 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 == 0)
>>               return 1;
>> +     else if (ret > 0)
>> +             return 0;
>>
>>       kvm_inject_undefined(vcpu);
>>       return 1;
>> diff --git a/arch/arm/kvm/psci.c b/arch/arm/kvm/psci.c
>> index 0881bf1..c8d3fcd 100644
>> --- a/arch/arm/kvm/psci.c
>> +++ b/arch/arm/kvm/psci.c
>> @@ -84,18 +84,31 @@ static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu)
>>       return KVM_PSCI_RET_SUCCESS;
>>  }
>>
>> +static void kvm_psci_system_off(struct kvm_vcpu *vcpu)
>> +{
>> +     vcpu->run->exit_reason = KVM_EXIT_SHUTDOWN;
>> +}
>> +
>> +static void kvm_psci_system_reset(struct kvm_vcpu *vcpu)
>> +{
>> +     vcpu->run->exit_reason = KVM_EXIT_RESET;
>> +}
>> +
>>  /**
>>   * 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.
>> + * The calling convention is similar to SMC calls to the secure world
>> + * where the function number is placed in r0 and function number
>> + * specified in r0 is withing the PSCI range.
>> + *
>> + * This function returns: 0 (success), > 0 (success but exit to user
>> + * space), and < 0 (failure)
>
> Hmmm, I think this is a bit confusing because it essentially does the
> reverse of all the other kvm internal return convention.  Wouldn't it
> make more sense to have 0 (success, but exit to user space), 1 (success,
> resume VM), and < 0 (failure).

OK, I will change kvm_psci_call() return convention as-per your suggestion.

>
> Also note that you are general-casing all error codes that may happen to
> mean that it did not match the PSCI function to anything well-known.
> In this case, that's sort of ok, because we don't expect the PSCI code
> to randomly fail (if it does, it reports it to the caller in the return
> register), but not fail, as in, I'm out of memory or anything like that.
> But I think you can clarify this in the comment, by saying something
> like:
>
> Errors:
>  -EINVAL: Unrecognized PSCI function

Sure, I will update the comments.

>
> in the comment for the function.
>
>>   */
>> -bool kvm_psci_call(struct kvm_vcpu *vcpu)
>> +int kvm_psci_call(struct kvm_vcpu *vcpu)
>>  {
>> +     int ret = 0;
>>       unsigned long psci_fn = *vcpu_reg(vcpu, 0) & ~((u32) 0);
>>       unsigned long val;
>>
>> @@ -111,11 +124,20 @@ bool kvm_psci_call(struct kvm_vcpu *vcpu)
>>       case KVM_PSCI_FN_MIGRATE:
>>               val = KVM_PSCI_RET_NI;
>>               break;
>> -
>> +     case KVM_PSCI_FN_SYSTEM_OFF:
>> +             kvm_psci_system_off(vcpu);
>> +             val = KVM_PSCI_RET_SUCCESS;
>> +             ret = 1;
>> +             break;
>> +     case KVM_PSCI_FN_SYSTEM_RESET:
>> +             kvm_psci_system_reset(vcpu);
>> +             val = KVM_PSCI_RET_SUCCESS;
>> +             ret = 1;
>> +             break;
>>       default:
>> -             return false;
>> +             return -EINVAL;
>>       }
>>
>>       *vcpu_reg(vcpu, 0) = val;
>> -     return true;
>> +     return ret;
>>  }
>> diff --git a/arch/arm64/include/asm/kvm_psci.h b/arch/arm64/include/asm/kvm_psci.h
>> index e301a48..9bd0ee8 100644
>> --- a/arch/arm64/include/asm/kvm_psci.h
>> +++ b/arch/arm64/include/asm/kvm_psci.h
>> @@ -18,6 +18,6 @@
>>  #ifndef __ARM64_KVM_PSCI_H__
>>  #define __ARM64_KVM_PSCI_H__
>>
>> -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/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
>> index d9f026b..f678902 100644
>> --- a/arch/arm64/include/uapi/asm/kvm.h
>> +++ b/arch/arm64/include/uapi/asm/kvm.h
>> @@ -158,6 +158,8 @@ struct kvm_arch_memory_slot {
>>  #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_SYSTEM_OFF               KVM_PSCI_FN(4)
>> +#define KVM_PSCI_FN_SYSTEM_RESET     KVM_PSCI_FN(5)
>>
>>  #define KVM_PSCI_RET_SUCCESS         0
>>  #define KVM_PSCI_RET_NI                      ((unsigned long)-1)
>> diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
>> index 8da5606..a5b5b6a 100644
>> --- a/arch/arm64/kvm/handle_exit.c
>> +++ b/arch/arm64/kvm/handle_exit.c
>> @@ -30,8 +30,13 @@ 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 == 0)
>>               return 1;
>> +     else if (ret > 0)
>> +             return 0;
>>
>>       kvm_inject_undefined(vcpu);
>>       return 1;
>> @@ -39,9 +44,6 @@ static int handle_hvc(struct kvm_vcpu *vcpu, struct kvm_run *run)
>>
>>  static int handle_smc(struct kvm_vcpu *vcpu, struct kvm_run *run)
>>  {
>> -     if (kvm_psci_call(vcpu))
>> -             return 1;
>> -
>
> Isn't this an unrelated change, which should go in a separate patch with
> some justification text for the change?

Actually, handle_smc() was already using kvm_psci_call() so I had to change
it anyway because of changes in kvm_psic_call(). Also, user space emulation
of SMC-based PSCI is going to be different from in-kernel HVC-based PSCI
emulation hence, for now it seemed more appropriate to inject undefined
exception for KVM arm64 (just like KVM arm).

--
Anup

>
>>       kvm_inject_undefined(vcpu);
>>       return 1;
>>  }
>> --
>> 1.7.9.5
>>
>
> Otherwise, it looks overall to go in the right direction.
>
> -Christoffer
> _______________________________________________
> kvmarm mailing list
> kvmarm at lists.cs.columbia.edu
> https://lists.cs.columbia.edu/cucslists/listinfo/kvmarm

[PATCH 2/2] ARM/ARM64: KVM: Forward PSCI SYSTEM_OFF and SYSTEM_RESET to user space

[Marc Zyngier]

On 10/12/13 05:05, Anup Patel wrote:
> On Tue, Dec 10, 2013 at 4:21 AM, Christoffer Dall
> <christoffer.dall@linaro.org> wrote:
>> On Mon, Nov 25, 2013 at 09:19:59PM +0530, Anup Patel wrote:
>>> @@ -39,9 +44,6 @@ static int handle_hvc(struct kvm_vcpu *vcpu, struct kvm_run *run)
>>>
>>>  static int handle_smc(struct kvm_vcpu *vcpu, struct kvm_run *run)
>>>  {
>>> -     if (kvm_psci_call(vcpu))
>>> -             return 1;
>>> -
>>
>> Isn't this an unrelated change, which should go in a separate patch with
>> some justification text for the change?
> 
> Actually, handle_smc() was already using kvm_psci_call() so I had to change
> it anyway because of changes in kvm_psic_call(). Also, user space emulation
> of SMC-based PSCI is going to be different from in-kernel HVC-based PSCI
> emulation hence, for now it seemed more appropriate to inject undefined
> exception for KVM arm64 (just like KVM arm).

Doh. I though I got rid of that.

That chunk should have been been removed ages ago (probably before
merging the arm64 code), as per 24a7f6757.

Please make it a separate patch, and I'll merge that independently.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

[PATCH 2/2] ARM/ARM64: KVM: Forward PSCI SYSTEM_OFF and SYSTEM_RESET to user space

[Anup Patel]

On Tue, Dec 10, 2013 at 4:27 PM, Marc Zyngier <marc.zyngier@arm.com> wrote:
> On 10/12/13 05:05, Anup Patel wrote:
>> On Tue, Dec 10, 2013 at 4:21 AM, Christoffer Dall
>> <christoffer.dall@linaro.org> wrote:
>>> On Mon, Nov 25, 2013 at 09:19:59PM +0530, Anup Patel wrote:
>>>> @@ -39,9 +44,6 @@ static int handle_hvc(struct kvm_vcpu *vcpu, struct kvm_run *run)
>>>>
>>>>  static int handle_smc(struct kvm_vcpu *vcpu, struct kvm_run *run)
>>>>  {
>>>> -     if (kvm_psci_call(vcpu))
>>>> -             return 1;
>>>> -
>>>
>>> Isn't this an unrelated change, which should go in a separate patch with
>>> some justification text for the change?
>>
>> Actually, handle_smc() was already using kvm_psci_call() so I had to change
>> it anyway because of changes in kvm_psic_call(). Also, user space emulation
>> of SMC-based PSCI is going to be different from in-kernel HVC-based PSCI
>> emulation hence, for now it seemed more appropriate to inject undefined
>> exception for KVM arm64 (just like KVM arm).
>
> Doh. I though I got rid of that.
>
> That chunk should have been been removed ages ago (probably before
> merging the arm64 code), as per 24a7f6757.
>
> Please make it a separate patch, and I'll merge that independently.

Sure, I'll make this a separate patch and send it soon.

--
Anup

>
> Thanks,
>
>         M.
> --
> Jazz is not dead. It just smells funny...

[PATCH 1/2] KVM: Add KVM_EXIT_RESET to user space API header

[Alexander Graf]

On 10.12.2013, at 05:23, Anup Patel <anup@brainfault.org> wrote:

> On Tue, Dec 10, 2013 at 7:43 AM, Alexander Graf <agraf@suse.de> wrote:
>> 
>> On 25.11.2013, at 16:49, Anup Patel <anup.patel@linaro.org> wrote:
>> 
>>> Currently, we don't have an exit reason for VM reset emulation
>>> in user space hence this patch adds exit reason KVM_EXIT_RESET
>>> for this purpose.
>>> 
>>> This newly added KVM_EXIT_RESET will be used by KVM arm/arm64
>>> in-kernel PSCI support to reset VMs.
>>> 
>>> Signed-off-by: Anup Patel <anup.patel@linaro.org>
>>> Signed-off-by: Pranavkumar Sawargaonkar <pranavkumar@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 902f124..64a04cc 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_RESET            24
>> 
>> I have to admit that I'm not particularly happy with the exit name. It's not obvious from the name under which circumstances it gets triggered. Does it get triggered when a core level reset happens? Does it get triggered when a system level reset happened? When the guest requests one?
> 
> The KVM_EXIT_RESET gets triggered when system level reset is
> initiated by VCPU. For arm/arm64, this is through SYSTEM_RESET
> PSCI call. In KVM x86 SVM/VMX, we have KVM_EXIT_SHUTDOWN
> being used for system shutdown which we have re-used for arm/arm64.

Yeah, that name already did mislead you once :).

KVM_EXIT_SHUTDOWN happens on

  * triple fault
  * CPU internal severe problems

the latter is defined as:

In contrast, an error that cannot be contained and is of such severity that it has compromised the continued operation of a processor core requires immediate action to terminate system processing and may result in a hardware-enforced shutdown. In the shutdown state, the execution of instructions by that processor core is halted. See Section 8.2.9 ?#DF?Double-Fault Exception (Vector 8)? on page 220 for a description of the shutdown processor state.

Triple faults are used commonly in 286 code to switch from PG to real mode. So they _have_ to be emulated as core reset. Otherwise you break old guests.

However, the scope of this exit is definitely vcpu wide. What you are looking for is a system wide notification. Commonly this happens through MMIO, but I can see why you wouldn't want that with PSCI interpreted in the kernel. That's why I asked you to create a completely new one to not add up the the confusion.

> 
>> 
>> I know what it does, but I find the name too generic for what it is. What you're really doing is introduce a new communication channel in parallel to MMIO / PIO / HCALL which is only used for system level reset / shutdown today.
>> 
>> Can we treat it as such? Could you please make this a common exit number that's called something like
>> 
>>  KVM_EXIT_SYSTEM_EVENT
>> 
>> with a parameter that can either be TRIGGER_SHUTDOWN or TRIGGER_RESET.
>> 
>> That way it's obvious what's going on and people don't get confused.
> 
> I don't foresee any system level operations other than SHUTDOWN
> and RESET to be handled from KVM in-kernel code but I might be
> wrong.

The good but about the EXIT_SYSTEM_EVENT is that it's immediately obvious that we're not talking about a vcpu local event. But I'm open to better names.

> May be we can rename KVM_EXIT_SHUTDOWN and KVM_EXIT_RESET
> to KVM_EXIT_SYSTEM_SHUTDOWN and KVM_EXIT_SYSTEM_RESET ??

You definitely can not rename KVM_EXIT_SHUTDOWN. It's part of the KVM API. In fact, I think it's a bad idea to even reuse the name as it clearly works on vcpu level.


Alex

[PATCH 1/2] KVM: Add KVM_EXIT_RESET to user space API header

[Anup Patel]

On Tue, Dec 10, 2013 at 9:19 PM, Alexander Graf <agraf@suse.de> wrote:
>
> On 10.12.2013, at 05:23, Anup Patel <anup@brainfault.org> wrote:
>
>> On Tue, Dec 10, 2013 at 7:43 AM, Alexander Graf <agraf@suse.de> wrote:
>>>
>>> On 25.11.2013, at 16:49, Anup Patel <anup.patel@linaro.org> wrote:
>>>
>>>> Currently, we don't have an exit reason for VM reset emulation
>>>> in user space hence this patch adds exit reason KVM_EXIT_RESET
>>>> for this purpose.
>>>>
>>>> This newly added KVM_EXIT_RESET will be used by KVM arm/arm64
>>>> in-kernel PSCI support to reset VMs.
>>>>
>>>> Signed-off-by: Anup Patel <anup.patel@linaro.org>
>>>> Signed-off-by: Pranavkumar Sawargaonkar <pranavkumar@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 902f124..64a04cc 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_RESET            24
>>>
>>> I have to admit that I'm not particularly happy with the exit name. It's not obvious from the name under which circumstances it gets triggered. Does it get triggered when a core level reset happens? Does it get triggered when a system level reset happened? When the guest requests one?
>>
>> The KVM_EXIT_RESET gets triggered when system level reset is
>> initiated by VCPU. For arm/arm64, this is through SYSTEM_RESET
>> PSCI call. In KVM x86 SVM/VMX, we have KVM_EXIT_SHUTDOWN
>> being used for system shutdown which we have re-used for arm/arm64.
>
> Yeah, that name already did mislead you once :).
>
> KVM_EXIT_SHUTDOWN happens on
>
>   * triple fault
>   * CPU internal severe problems
>
> the latter is defined as:
>
> In contrast, an error that cannot be contained and is of such severity that it has compromised the continued operation of a processor core requires immediate action to terminate system processing and may result in a hardware-enforced shutdown. In the shutdown state, the execution of instructions by that processor core is halted. See Section 8.2.9 ?#DF?Double-Fault Exception (Vector 8)? on page 220 for a description of the shutdown processor state.
>
> Triple faults are used commonly in 286 code to switch from PG to real mode. So they _have_ to be emulated as core reset. Otherwise you break old guests.
>
> However, the scope of this exit is definitely vcpu wide. What you are looking for is a system wide notification. Commonly this happens through MMIO, but I can see why you wouldn't want that with PSCI interpreted in the kernel. That's why I asked you to create a completely new one to not add up the the confusion.

Thanks for the info on the x86 part.

I think all this info should have been part of KVM api documentation
for KVM_EXIT_SHUTDOWN.

>
>>
>>>
>>> I know what it does, but I find the name too generic for what it is. What you're really doing is introduce a new communication channel in parallel to MMIO / PIO / HCALL which is only used for system level reset / shutdown today.
>>>
>>> Can we treat it as such? Could you please make this a common exit number that's called something like
>>>
>>>  KVM_EXIT_SYSTEM_EVENT
>>>
>>> with a parameter that can either be TRIGGER_SHUTDOWN or TRIGGER_RESET.
>>>
>>> That way it's obvious what's going on and people don't get confused.
>>
>> I don't foresee any system level operations other than SHUTDOWN
>> and RESET to be handled from KVM in-kernel code but I might be
>> wrong.
>
> The good but about the EXIT_SYSTEM_EVENT is that it's immediately obvious that we're not talking about a vcpu local event. But I'm open to better names.
>
>> May be we can rename KVM_EXIT_SHUTDOWN and KVM_EXIT_RESET
>> to KVM_EXIT_SYSTEM_SHUTDOWN and KVM_EXIT_SYSTEM_RESET ??
>
> You definitely can not rename KVM_EXIT_SHUTDOWN. It's part of the KVM API. In fact, I think it's a bad idea to even reuse the name as it clearly works on vcpu level.

Sure, it makes sense to avoid use of KVM_EXIT_SHUTDOWN
in arm/arm64.

How about adding exit reasons KVM_EXIT_SYSTEM_RESET
and KVM_EXIT_SYSTEM_SHUTDOWN ?

These exit reasons will be used for arm/arm64 but can also be
used by other architectures if they want.

--
Anup

>
>
> Alex
>

[PATCH 1/2] KVM: Add KVM_EXIT_RESET to user space API header

[Alexander Graf]

On 10.12.2013, at 17:07, Anup Patel <anup@brainfault.org> wrote:

> On Tue, Dec 10, 2013 at 9:19 PM, Alexander Graf <agraf@suse.de> wrote:
>> 
>> On 10.12.2013, at 05:23, Anup Patel <anup@brainfault.org> wrote:
>> 
>>> On Tue, Dec 10, 2013 at 7:43 AM, Alexander Graf <agraf@suse.de> wrote:
>>>> 
>>>> On 25.11.2013, at 16:49, Anup Patel <anup.patel@linaro.org> wrote:
>>>> 
>>>>> Currently, we don't have an exit reason for VM reset emulation
>>>>> in user space hence this patch adds exit reason KVM_EXIT_RESET
>>>>> for this purpose.
>>>>> 
>>>>> This newly added KVM_EXIT_RESET will be used by KVM arm/arm64
>>>>> in-kernel PSCI support to reset VMs.
>>>>> 
>>>>> Signed-off-by: Anup Patel <anup.patel@linaro.org>
>>>>> Signed-off-by: Pranavkumar Sawargaonkar <pranavkumar@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 902f124..64a04cc 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_RESET            24
>>>> 
>>>> I have to admit that I'm not particularly happy with the exit name. It's not obvious from the name under which circumstances it gets triggered. Does it get triggered when a core level reset happens? Does it get triggered when a system level reset happened? When the guest requests one?
>>> 
>>> The KVM_EXIT_RESET gets triggered when system level reset is
>>> initiated by VCPU. For arm/arm64, this is through SYSTEM_RESET
>>> PSCI call. In KVM x86 SVM/VMX, we have KVM_EXIT_SHUTDOWN
>>> being used for system shutdown which we have re-used for arm/arm64.
>> 
>> Yeah, that name already did mislead you once :).
>> 
>> KVM_EXIT_SHUTDOWN happens on
>> 
>>  * triple fault
>>  * CPU internal severe problems
>> 
>> the latter is defined as:
>> 
>> In contrast, an error that cannot be contained and is of such severity that it has compromised the continued operation of a processor core requires immediate action to terminate system processing and may result in a hardware-enforced shutdown. In the shutdown state, the execution of instructions by that processor core is halted. See Section 8.2.9 ?#DF?Double-Fault Exception (Vector 8)? on page 220 for a description of the shutdown processor state.
>> 
>> Triple faults are used commonly in 286 code to switch from PG to real mode. So they _have_ to be emulated as core reset. Otherwise you break old guests.
>> 
>> However, the scope of this exit is definitely vcpu wide. What you are looking for is a system wide notification. Commonly this happens through MMIO, but I can see why you wouldn't want that with PSCI interpreted in the kernel. That's why I asked you to create a completely new one to not add up the the confusion.
> 
> Thanks for the info on the x86 part.
> 
> I think all this info should have been part of KVM api documentation
> for KVM_EXIT_SHUTDOWN.
> 
>> 
>>> 
>>>> 
>>>> I know what it does, but I find the name too generic for what it is. What you're really doing is introduce a new communication channel in parallel to MMIO / PIO / HCALL which is only used for system level reset / shutdown today.
>>>> 
>>>> Can we treat it as such? Could you please make this a common exit number that's called something like
>>>> 
>>>> KVM_EXIT_SYSTEM_EVENT
>>>> 
>>>> with a parameter that can either be TRIGGER_SHUTDOWN or TRIGGER_RESET.
>>>> 
>>>> That way it's obvious what's going on and people don't get confused.
>>> 
>>> I don't foresee any system level operations other than SHUTDOWN
>>> and RESET to be handled from KVM in-kernel code but I might be
>>> wrong.
>> 
>> The good but about the EXIT_SYSTEM_EVENT is that it's immediately obvious that we're not talking about a vcpu local event. But I'm open to better names.
>> 
>>> May be we can rename KVM_EXIT_SHUTDOWN and KVM_EXIT_RESET
>>> to KVM_EXIT_SYSTEM_SHUTDOWN and KVM_EXIT_SYSTEM_RESET ??
>> 
>> You definitely can not rename KVM_EXIT_SHUTDOWN. It's part of the KVM API. In fact, I think it's a bad idea to even reuse the name as it clearly works on vcpu level.
> 
> Sure, it makes sense to avoid use of KVM_EXIT_SHUTDOWN
> in arm/arm64.
> 
> How about adding exit reasons KVM_EXIT_SYSTEM_RESET
> and KVM_EXIT_SYSTEM_SHUTDOWN ?
> 
> These exit reasons will be used for arm/arm64 but can also be
> used by other architectures if they want.

I don't think we'll be able to reuse anything for other archs. For hcall PPC for example, we want to keep the hcall number scheme as the same between guest <-> kvm and kvm <-> qemu. That makes the overall logic easier, as the hcall number space is already properly standardized.


Alex

[PATCH 1/2] KVM: Add KVM_EXIT_RESET to user space API header

[Christoffer Dall]

On Tue, Dec 10, 2013 at 03:13:34AM +0100, Alexander Graf wrote:
> 
> On 25.11.2013, at 16:49, Anup Patel <anup.patel@linaro.org> wrote:
> 
> > Currently, we don't have an exit reason for VM reset emulation
> > in user space hence this patch adds exit reason KVM_EXIT_RESET
> > for this purpose.
> > 
> > This newly added KVM_EXIT_RESET will be used by KVM arm/arm64
> > in-kernel PSCI support to reset VMs.
> > 
> > Signed-off-by: Anup Patel <anup.patel@linaro.org>
> > Signed-off-by: Pranavkumar Sawargaonkar <pranavkumar@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 902f124..64a04cc 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_RESET            24
> 
> I have to admit that I'm not particularly happy with the exit name. It's not obvious from the name under which circumstances it gets triggered. Does it get triggered when a core level reset happens? Does it get triggered when a system level reset happened? When the guest requests one?
> 
> I know what it does, but I find the name too generic for what it is. What you're really doing is introduce a new communication channel in parallel to MMIO / PIO / HCALL which is only used for system level reset / shutdown today.
> 
> Can we treat it as such? Could you please make this a common exit number that's called something like
> 
>   KVM_EXIT_SYSTEM_EVENT
> 
> with a parameter that can either be TRIGGER_SHUTDOWN or TRIGGER_RESET.
> 
> That way it's obvious what's going on and people don't get confused.
> 
I didn't realize what the KVM_EXIT_SHUTDOWN really was, thanks for
explaining that.  In that case, the SYSTEM_EVENT sounds good to me.

How do you propose the parameter gets passed?  As a new struct to the
untion in kvm_run ?

-Christoffer

[PATCH 1/2] KVM: Add KVM_EXIT_RESET to user space API header

[Christoffer Dall]

On Tue, Dec 10, 2013 at 04:49:19PM +0100, Alexander Graf wrote:
> 
> On 10.12.2013, at 05:23, Anup Patel <anup@brainfault.org> wrote:
> 
> > On Tue, Dec 10, 2013 at 7:43 AM, Alexander Graf <agraf@suse.de> wrote:
> >> 
> >> On 25.11.2013, at 16:49, Anup Patel <anup.patel@linaro.org> wrote:
> >> 
> >>> Currently, we don't have an exit reason for VM reset emulation
> >>> in user space hence this patch adds exit reason KVM_EXIT_RESET
> >>> for this purpose.
> >>> 
> >>> This newly added KVM_EXIT_RESET will be used by KVM arm/arm64
> >>> in-kernel PSCI support to reset VMs.
> >>> 
> >>> Signed-off-by: Anup Patel <anup.patel@linaro.org>
> >>> Signed-off-by: Pranavkumar Sawargaonkar <pranavkumar@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 902f124..64a04cc 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_RESET            24
> >> 
> >> I have to admit that I'm not particularly happy with the exit name. It's not obvious from the name under which circumstances it gets triggered. Does it get triggered when a core level reset happens? Does it get triggered when a system level reset happened? When the guest requests one?
> > 
> > The KVM_EXIT_RESET gets triggered when system level reset is
> > initiated by VCPU. For arm/arm64, this is through SYSTEM_RESET
> > PSCI call. In KVM x86 SVM/VMX, we have KVM_EXIT_SHUTDOWN
> > being used for system shutdown which we have re-used for arm/arm64.
> 
> Yeah, that name already did mislead you once :).
> 
> KVM_EXIT_SHUTDOWN happens on
> 
>   * triple fault
>   * CPU internal severe problems
> 
> the latter is defined as:
> 
> In contrast, an error that cannot be contained and is of such severity that it has compromised the continued operation of a processor core requires immediate action to terminate system processing and may result in a hardware-enforced shutdown. In the shutdown state, the execution of instructions by that processor core is halted. See Section 8.2.9 ?#DF?Double-Fault Exception (Vector 8)? on page 220 for a description of the shutdown processor state.
> 
> Triple faults are used commonly in 286 code to switch from PG to real mode. So they _have_ to be emulated as core reset. Otherwise you break old guests.
> 
> However, the scope of this exit is definitely vcpu wide. What you are looking for is a system wide notification. Commonly this happens through MMIO, but I can see why you wouldn't want that with PSCI interpreted in the kernel. That's why I asked you to create a completely new one to not add up the the confusion.
> 

Did you grab this documentation from somewhere that I can't find with
grep, or did you just come up with it?

Shouldn't we have all exit reasons documented in
Documentation/virtual/kvm/... ?

> > 
> >> 
> >> I know what it does, but I find the name too generic for what it is. What you're really doing is introduce a new communication channel in parallel to MMIO / PIO / HCALL which is only used for system level reset / shutdown today.
> >> 
> >> Can we treat it as such? Could you please make this a common exit number that's called something like
> >> 
> >>  KVM_EXIT_SYSTEM_EVENT
> >> 
> >> with a parameter that can either be TRIGGER_SHUTDOWN or TRIGGER_RESET.
> >> 
> >> That way it's obvious what's going on and people don't get confused.
> > 
> > I don't foresee any system level operations other than SHUTDOWN
> > and RESET to be handled from KVM in-kernel code but I might be
> > wrong.
> 
> The good but about the EXIT_SYSTEM_EVENT is that it's immediately obvious that we're not talking about a vcpu local event. But I'm open to better names.
> 
> > May be we can rename KVM_EXIT_SHUTDOWN and KVM_EXIT_RESET
> > to KVM_EXIT_SYSTEM_SHUTDOWN and KVM_EXIT_SYSTEM_RESET ??
> 
> You definitely can not rename KVM_EXIT_SHUTDOWN. It's part of the KVM API. In fact, I think it's a bad idea to even reuse the name as it clearly works on vcpu level.
> 
> 

-Christoffer

[PATCH 1/2] KVM: Add KVM_EXIT_RESET to user space API header

[Christoffer Dall]

On Tue, Dec 10, 2013 at 05:45:31PM +0100, Alexander Graf wrote:
> 
> On 10.12.2013, at 17:07, Anup Patel <anup@brainfault.org> wrote:
> 
> > On Tue, Dec 10, 2013 at 9:19 PM, Alexander Graf <agraf@suse.de> wrote:
> >> 
> >> On 10.12.2013, at 05:23, Anup Patel <anup@brainfault.org> wrote:
> >> 
> >>> On Tue, Dec 10, 2013 at 7:43 AM, Alexander Graf <agraf@suse.de> wrote:
> >>>> 
> >>>> On 25.11.2013, at 16:49, Anup Patel <anup.patel@linaro.org> wrote:
> >>>> 
> >>>>> Currently, we don't have an exit reason for VM reset emulation
> >>>>> in user space hence this patch adds exit reason KVM_EXIT_RESET
> >>>>> for this purpose.
> >>>>> 
> >>>>> This newly added KVM_EXIT_RESET will be used by KVM arm/arm64
> >>>>> in-kernel PSCI support to reset VMs.
> >>>>> 
> >>>>> Signed-off-by: Anup Patel <anup.patel@linaro.org>
> >>>>> Signed-off-by: Pranavkumar Sawargaonkar <pranavkumar@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 902f124..64a04cc 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_RESET            24
> >>>> 
> >>>> I have to admit that I'm not particularly happy with the exit name. It's not obvious from the name under which circumstances it gets triggered. Does it get triggered when a core level reset happens? Does it get triggered when a system level reset happened? When the guest requests one?
> >>> 
> >>> The KVM_EXIT_RESET gets triggered when system level reset is
> >>> initiated by VCPU. For arm/arm64, this is through SYSTEM_RESET
> >>> PSCI call. In KVM x86 SVM/VMX, we have KVM_EXIT_SHUTDOWN
> >>> being used for system shutdown which we have re-used for arm/arm64.
> >> 
> >> Yeah, that name already did mislead you once :).
> >> 
> >> KVM_EXIT_SHUTDOWN happens on
> >> 
> >>  * triple fault
> >>  * CPU internal severe problems
> >> 
> >> the latter is defined as:
> >> 
> >> In contrast, an error that cannot be contained and is of such severity that it has compromised the continued operation of a processor core requires immediate action to terminate system processing and may result in a hardware-enforced shutdown. In the shutdown state, the execution of instructions by that processor core is halted. See Section 8.2.9 ?#DF?Double-Fault Exception (Vector 8)? on page 220 for a description of the shutdown processor state.
> >> 
> >> Triple faults are used commonly in 286 code to switch from PG to real mode. So they _have_ to be emulated as core reset. Otherwise you break old guests.
> >> 
> >> However, the scope of this exit is definitely vcpu wide. What you are looking for is a system wide notification. Commonly this happens through MMIO, but I can see why you wouldn't want that with PSCI interpreted in the kernel. That's why I asked you to create a completely new one to not add up the the confusion.
> > 
> > Thanks for the info on the x86 part.
> > 
> > I think all this info should have been part of KVM api documentation
> > for KVM_EXIT_SHUTDOWN.
> > 
> >> 
> >>> 
> >>>> 
> >>>> I know what it does, but I find the name too generic for what it is. What you're really doing is introduce a new communication channel in parallel to MMIO / PIO / HCALL which is only used for system level reset / shutdown today.
> >>>> 
> >>>> Can we treat it as such? Could you please make this a common exit number that's called something like
> >>>> 
> >>>> KVM_EXIT_SYSTEM_EVENT
> >>>> 
> >>>> with a parameter that can either be TRIGGER_SHUTDOWN or TRIGGER_RESET.
> >>>> 
> >>>> That way it's obvious what's going on and people don't get confused.
> >>> 
> >>> I don't foresee any system level operations other than SHUTDOWN
> >>> and RESET to be handled from KVM in-kernel code but I might be
> >>> wrong.
> >> 
> >> The good but about the EXIT_SYSTEM_EVENT is that it's immediately obvious that we're not talking about a vcpu local event. But I'm open to better names.
> >> 
> >>> May be we can rename KVM_EXIT_SHUTDOWN and KVM_EXIT_RESET
> >>> to KVM_EXIT_SYSTEM_SHUTDOWN and KVM_EXIT_SYSTEM_RESET ??
> >> 
> >> You definitely can not rename KVM_EXIT_SHUTDOWN. It's part of the KVM API. In fact, I think it's a bad idea to even reuse the name as it clearly works on vcpu level.
> > 
> > Sure, it makes sense to avoid use of KVM_EXIT_SHUTDOWN
> > in arm/arm64.
> > 
> > How about adding exit reasons KVM_EXIT_SYSTEM_RESET
> > and KVM_EXIT_SYSTEM_SHUTDOWN ?
> > 
> > These exit reasons will be used for arm/arm64 but can also be
> > used by other architectures if they want.
> 
> I don't think we'll be able to reuse anything for other archs. For hcall PPC for example, we want to keep the hcall number scheme as the same between guest <-> kvm and kvm <-> qemu. That makes the overall logic easier, as the hcall number space is already properly standardized.
> 
> 
It should also make the first switch in userspace on the exit reason
easier, as the same code is likely to handle all sorts of system events.

-Christoffer

[PATCH 1/2] KVM: Add KVM_EXIT_RESET to user space API header

[Alexander Graf]

On 10.12.2013, at 23:30, Christoffer Dall <christoffer.dall@linaro.org> wrote:

> On Tue, Dec 10, 2013 at 04:49:19PM +0100, Alexander Graf wrote:
>> 
>> On 10.12.2013, at 05:23, Anup Patel <anup@brainfault.org> wrote:
>> 
>>> On Tue, Dec 10, 2013 at 7:43 AM, Alexander Graf <agraf@suse.de> wrote:
>>>> 
>>>> On 25.11.2013, at 16:49, Anup Patel <anup.patel@linaro.org> wrote:
>>>> 
>>>>> Currently, we don't have an exit reason for VM reset emulation
>>>>> in user space hence this patch adds exit reason KVM_EXIT_RESET
>>>>> for this purpose.
>>>>> 
>>>>> This newly added KVM_EXIT_RESET will be used by KVM arm/arm64
>>>>> in-kernel PSCI support to reset VMs.
>>>>> 
>>>>> Signed-off-by: Anup Patel <anup.patel@linaro.org>
>>>>> Signed-off-by: Pranavkumar Sawargaonkar <pranavkumar@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 902f124..64a04cc 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_RESET            24
>>>> 
>>>> I have to admit that I'm not particularly happy with the exit name. It's not obvious from the name under which circumstances it gets triggered. Does it get triggered when a core level reset happens? Does it get triggered when a system level reset happened? When the guest requests one?
>>> 
>>> The KVM_EXIT_RESET gets triggered when system level reset is
>>> initiated by VCPU. For arm/arm64, this is through SYSTEM_RESET
>>> PSCI call. In KVM x86 SVM/VMX, we have KVM_EXIT_SHUTDOWN
>>> being used for system shutdown which we have re-used for arm/arm64.
>> 
>> Yeah, that name already did mislead you once :).
>> 
>> KVM_EXIT_SHUTDOWN happens on
>> 
>>  * triple fault
>>  * CPU internal severe problems
>> 
>> the latter is defined as:
>> 
>> In contrast, an error that cannot be contained and is of such severity that it has compromised the continued operation of a processor core requires immediate action to terminate system processing and may result in a hardware-enforced shutdown. In the shutdown state, the execution of instructions by that processor core is halted. See Section 8.2.9 ?#DF?Double-Fault Exception (Vector 8)? on page 220 for a description of the shutdown processor state.
>> 
>> Triple faults are used commonly in 286 code to switch from PG to real mode. So they _have_ to be emulated as core reset. Otherwise you break old guests.
>> 
>> However, the scope of this exit is definitely vcpu wide. What you are looking for is a system wide notification. Commonly this happens through MMIO, but I can see why you wouldn't want that with PSCI interpreted in the kernel. That's why I asked you to create a completely new one to not add up the the confusion.
>> 
> 
> Did you grab this documentation from somewhere that I can't find with
> grep, or did you just come up with it?

That was from the AMD64 architecture reference manual :).

> Shouldn't we have all exit reasons documented in
> Documentation/virtual/kvm/... ?

Yes, that would be great. The only reason I knew something was fishy was that there's no way for a guest to trigger a non-MMIO/PIO/HCALL event that would end up in a genuine shutdown request ;).


Alex

[PATCH 1/2] KVM: Add KVM_EXIT_RESET to user space API header

[Alexander Graf]

On 10.12.2013, at 23:27, Christoffer Dall <christoffer.dall@linaro.org> wrote:

> On Tue, Dec 10, 2013 at 03:13:34AM +0100, Alexander Graf wrote:
>> 
>> On 25.11.2013, at 16:49, Anup Patel <anup.patel@linaro.org> wrote:
>> 
>>> Currently, we don't have an exit reason for VM reset emulation
>>> in user space hence this patch adds exit reason KVM_EXIT_RESET
>>> for this purpose.
>>> 
>>> This newly added KVM_EXIT_RESET will be used by KVM arm/arm64
>>> in-kernel PSCI support to reset VMs.
>>> 
>>> Signed-off-by: Anup Patel <anup.patel@linaro.org>
>>> Signed-off-by: Pranavkumar Sawargaonkar <pranavkumar@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 902f124..64a04cc 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_RESET            24
>> 
>> I have to admit that I'm not particularly happy with the exit name. It's not obvious from the name under which circumstances it gets triggered. Does it get triggered when a core level reset happens? Does it get triggered when a system level reset happened? When the guest requests one?
>> 
>> I know what it does, but I find the name too generic for what it is. What you're really doing is introduce a new communication channel in parallel to MMIO / PIO / HCALL which is only used for system level reset / shutdown today.
>> 
>> Can we treat it as such? Could you please make this a common exit number that's called something like
>> 
>>  KVM_EXIT_SYSTEM_EVENT
>> 
>> with a parameter that can either be TRIGGER_SHUTDOWN or TRIGGER_RESET.
>> 
>> That way it's obvious what's going on and people don't get confused.
>> 
> I didn't realize what the KVM_EXIT_SHUTDOWN really was, thanks for
> explaining that.  In that case, the SYSTEM_EVENT sounds good to me.
> 
> How do you propose the parameter gets passed?  As a new struct to the
> untion in kvm_run ?

Yup, that's how we usually pass information along with an exit reason. Make sure to add a flags field so it can be extended if we need to pass in more later for future unknown events. PPC for example has fun system wide hypercalls like "enable little endian mode on all virtual cpus". I don't even want to start imagining what people will come up with next.


Alex

[PATCH 1/2] KVM: Add KVM_EXIT_RESET to user space API header

[Anup Patel]

On Wed, Dec 11, 2013 at 4:06 AM, Alexander Graf <agraf@suse.de> wrote:
>
> On 10.12.2013, at 23:27, Christoffer Dall <christoffer.dall@linaro.org> wrote:
>
>> On Tue, Dec 10, 2013 at 03:13:34AM +0100, Alexander Graf wrote:
>>>
>>> On 25.11.2013, at 16:49, Anup Patel <anup.patel@linaro.org> wrote:
>>>
>>>> Currently, we don't have an exit reason for VM reset emulation
>>>> in user space hence this patch adds exit reason KVM_EXIT_RESET
>>>> for this purpose.
>>>>
>>>> This newly added KVM_EXIT_RESET will be used by KVM arm/arm64
>>>> in-kernel PSCI support to reset VMs.
>>>>
>>>> Signed-off-by: Anup Patel <anup.patel@linaro.org>
>>>> Signed-off-by: Pranavkumar Sawargaonkar <pranavkumar@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 902f124..64a04cc 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_RESET            24
>>>
>>> I have to admit that I'm not particularly happy with the exit name. It's not obvious from the name under which circumstances it gets triggered. Does it get triggered when a core level reset happens? Does it get triggered when a system level reset happened? When the guest requests one?
>>>
>>> I know what it does, but I find the name too generic for what it is. What you're really doing is introduce a new communication channel in parallel to MMIO / PIO / HCALL which is only used for system level reset / shutdown today.
>>>
>>> Can we treat it as such? Could you please make this a common exit number that's called something like
>>>
>>>  KVM_EXIT_SYSTEM_EVENT
>>>
>>> with a parameter that can either be TRIGGER_SHUTDOWN or TRIGGER_RESET.
>>>
>>> That way it's obvious what's going on and people don't get confused.
>>>
>> I didn't realize what the KVM_EXIT_SHUTDOWN really was, thanks for
>> explaining that.  In that case, the SYSTEM_EVENT sounds good to me.
>>
>> How do you propose the parameter gets passed?  As a new struct to the
>> untion in kvm_run ?
>
> Yup, that's how we usually pass information along with an exit reason. Make sure to add a flags field so it can be extended if we need to pass in more later for future unknown events. PPC for example has fun system wide hypercalls like "enable little endian mode on all virtual cpus". I don't even want to start imagining what people will come up with next.

Sure, I will add KVM_EXIT_SYSTEM_EVENT exit reason and related documentation.

--
Anup

>
>
> Alex
>
>
> _______________________________________________
> kvmarm mailing list
> kvmarm at lists.cs.columbia.edu
> https://lists.cs.columbia.edu/cucslists/listinfo/kvmarm