[PATCH RESEND] arm64: KVM: Add Kconfig option for max VCPUs per-Guest

[PATCH RESEND] arm64: KVM: Add Kconfig option for max VCPUs per-Guest

[Anup Patel]

Current max VCPUs per-Guest is set to 4 which is preventing
us from creating a Guest (or VM) with 8 VCPUs on Host (e.g.
X-Gene Storm SOC) with 8 Host CPUs.

The correct value of max VCPUs per-Guest should be same as
the max CPUs supported by GICv2 which is 8 but, increasing
value of max VCPUs per-Guest can make things slower hence
we add Kconfig option to let KVM users select appropriate
max VCPUs per-Guest.

Signed-off-by: Anup Patel <anup.patel@linaro.org>
Signed-off-by: Pranavkumar Sawargaonkar <pranavkumar@linaro.org>
---
 arch/arm64/include/asm/kvm_host.h |    7 ++++++-
 arch/arm64/kvm/Kconfig            |   11 +++++++++++
 2 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 5d85a02..0a1d697 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -26,7 +26,12 @@
 #include <asm/kvm_asm.h>
 #include <asm/kvm_mmio.h>
 
-#define KVM_MAX_VCPUS 4
+#if defined(CONFIG_KVM_ARM_MAX_VCPUS)
+#define KVM_MAX_VCPUS CONFIG_KVM_ARM_MAX_VCPUS
+#else
+#define KVM_MAX_VCPUS 0
+#endif
+
 #define KVM_USER_MEM_SLOTS 32
 #define KVM_PRIVATE_MEM_SLOTS 4
 #define KVM_COALESCED_MMIO_PAGE_OFFSET 1
diff --git a/arch/arm64/kvm/Kconfig b/arch/arm64/kvm/Kconfig
index 4480ab3..8ba85e9 100644
--- a/arch/arm64/kvm/Kconfig
+++ b/arch/arm64/kvm/Kconfig
@@ -36,6 +36,17 @@ config KVM_ARM_HOST
 	---help---
 	  Provides host support for ARM processors.
 
+config KVM_ARM_MAX_VCPUS
+	int "Number maximum supported virtual CPUs per VM"
+	depends on KVM_ARM_HOST
+	default 4
+	help
+	  Static number of max supported virtual CPUs per VM.
+
+	  If you choose a high number, the vcpu structures will be quite
+	  large, so only choose a reasonable number that you expect to
+	  actually use.
+
 config KVM_ARM_VGIC
 	bool
 	depends on KVM_ARM_HOST && OF
-- 
1.7.9.5

[PATCH] arm64: KVM: Force undefined exception for Guest SMC intructions

[Anup Patel]

The SMC-based PSCI emulation for Guest is going to be very different
from the in-kernel HVC-based PSCI emulation hence for now just inject
undefined exception when Guest executes SMC instruction.

Signed-off-by: Anup Patel <anup.patel@linaro.org>
Signed-off-by: Pranavkumar Sawargaonkar <pranavkumar@linaro.org>
---
 arch/arm64/kvm/handle_exit.c |    3 ---
 1 file changed, 3 deletions(-)

diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
index 8da5606..df84d7b 100644
--- a/arch/arm64/kvm/handle_exit.c
+++ b/arch/arm64/kvm/handle_exit.c
@@ -39,9 +39,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] KVM: Documentation: Fix typo for KVM_ARM_VCPU_INIT ioctl

[Anup Patel]

Fix minor typo in "Parameters:" of KVM_ARM_VCPU_INIT documentation.

Signed-off-by: Anup Patel <anup.patel@linaro.org>
Signed-off-by: Pranavkumar Sawargaonkar <pranavkumar@linaro.org>
---
 Documentation/virtual/kvm/api.txt |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
index a30035d..aad3244 100644
--- a/Documentation/virtual/kvm/api.txt
+++ b/Documentation/virtual/kvm/api.txt
@@ -2327,7 +2327,7 @@ current state.  "addr" is ignored.
 Capability: basic
 Architectures: arm, arm64
 Type: vcpu ioctl
-Parameters: struct struct kvm_vcpu_init (in)
+Parameters: struct kvm_vcpu_init (in)
 Returns: 0 on success; -1 on error
 Errors:
  ?EINVAL: ???the target is unknown, or the combination of features is invalid.
-- 
1.7.9.5

[PATCH v2 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_SYSTEM_EVENT exit reason.

To try this patch from guest kernel, we will need PSCI-based restart and
poweroff support in the guest kenel for both ARM and ARM64.

Rob Herring has already submitted patches for PSCI-based restart and
poweroff in ARM kernel but these are not merged yet due unstable device
tree bindings of kernel PSCI support. We will be having similar patches
for PSCI-based restart and poweroff in ARM64 kernel.
(Refer http://www.spinics.net/lists/arm-kernel/msg262217.html)
(Refer http://www.spinics.net/lists/devicetree/msg05348.html)

Change Log:

V2:
 - Replace KVM_EXIT_RESET with KVM_EXIT_SYSTEM_EVENT
 - Make kvm_psci_call() return convention to match 
   KVM ARM return convention

V1:
 - Initial revised patch after RFC PATCH.

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

 Documentation/virtual/kvm/api.txt |   15 ++++++++++++
 arch/arm/include/asm/kvm_psci.h   |    2 +-
 arch/arm/include/uapi/asm/kvm.h   |    2 ++
 arch/arm/kvm/handle_exit.c        |   13 +++++++---
 arch/arm/kvm/psci.c               |   48 ++++++++++++++++++++++++++++++-------
 arch/arm64/include/asm/kvm_psci.h |    2 +-
 arch/arm64/include/uapi/asm/kvm.h |    2 ++
 arch/arm64/kvm/handle_exit.c      |   12 +++++++---
 include/uapi/linux/kvm.h          |    8 +++++++
 9 files changed, 88 insertions(+), 16 deletions(-)

-- 
1.7.9.5

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

[Anup Patel]

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 support to reset/shutdown VMs.

Signed-off-by: Anup Patel <anup.patel@linaro.org>
Signed-off-by: Pranavkumar Sawargaonkar <pranavkumar@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 aad3244..4ef180e 100644
--- a/Documentation/virtual/kvm/api.txt
+++ b/Documentation/virtual/kvm/api.txt
@@ -2699,6 +2699,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 902f124..bb13faa 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];
 	};
-- 
1.7.9.5

[PATCH v2 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 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>
---
 arch/arm/include/asm/kvm_psci.h   |    2 +-
 arch/arm/include/uapi/asm/kvm.h   |    2 ++
 arch/arm/kvm/handle_exit.c        |   13 +++++++---
 arch/arm/kvm/psci.c               |   48 ++++++++++++++++++++++++++++++-------
 arch/arm64/include/asm/kvm_psci.h |    2 +-
 arch/arm64/include/uapi/asm/kvm.h |    2 ++
 arch/arm64/kvm/handle_exit.c      |   12 +++++++---
 7 files changed, 65 insertions(+), 16 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..c3f0e72 100644
--- a/arch/arm/kvm/handle_exit.c
+++ b/arch/arm/kvm/handle_exit.c
@@ -40,14 +40,21 @@ 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 ret;
+	else if (ret == -EINVAL) {
+		kvm_inject_undefined(vcpu);
 		return 1;
+	}
+
+	return ret;
 
-	kvm_inject_undefined(vcpu);
-	return 1;
 }
 
 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 0881bf1..8e246d6 100644
--- a/arch/arm/kvm/psci.c
+++ b/arch/arm/kvm/psci.c
@@ -84,18 +84,41 @@ static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu)
 	return KVM_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);
+}
+
 /**
  * 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 (errors)
+ *
+ * Errors:
+ * -EINVAL: Unrecognized PSCI function
  */
-bool kvm_psci_call(struct kvm_vcpu *vcpu)
+int kvm_psci_call(struct kvm_vcpu *vcpu)
 {
+	int ret = 1;
 	unsigned long psci_fn = *vcpu_reg(vcpu, 0) & ~((u32) 0);
 	unsigned long val;
 
@@ -111,11 +134,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 = 0;
+		break;
+	case KVM_PSCI_FN_SYSTEM_RESET:
+		kvm_psci_system_reset(vcpu);
+		val = KVM_PSCI_RET_SUCCESS;
+		ret = 0;
+		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 df84d7b..e382eb8 100644
--- a/arch/arm64/kvm/handle_exit.c
+++ b/arch/arm64/kvm/handle_exit.c
@@ -30,11 +30,17 @@ 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 ret;
+	else 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)
-- 
1.7.9.5

[PATCH RESEND] arm64: KVM: Add Kconfig option for max VCPUs per-Guest

[Marc Zyngier]

On 12/12/13 16:12, Anup Patel wrote:
> Current max VCPUs per-Guest is set to 4 which is preventing
> us from creating a Guest (or VM) with 8 VCPUs on Host (e.g.
> X-Gene Storm SOC) with 8 Host CPUs.
> 
> The correct value of max VCPUs per-Guest should be same as
> the max CPUs supported by GICv2 which is 8 but, increasing
> value of max VCPUs per-Guest can make things slower hence
> we add Kconfig option to let KVM users select appropriate
> max VCPUs per-Guest.
> 
> Signed-off-by: Anup Patel <anup.patel@linaro.org>
> Signed-off-by: Pranavkumar Sawargaonkar <pranavkumar@linaro.org>

Acked-by: Marc Zyngier <marc.zyngier@arm.com>

	M.

> ---
>  arch/arm64/include/asm/kvm_host.h |    7 ++++++-
>  arch/arm64/kvm/Kconfig            |   11 +++++++++++
>  2 files changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index 5d85a02..0a1d697 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -26,7 +26,12 @@
>  #include <asm/kvm_asm.h>
>  #include <asm/kvm_mmio.h>
>  
> -#define KVM_MAX_VCPUS 4
> +#if defined(CONFIG_KVM_ARM_MAX_VCPUS)
> +#define KVM_MAX_VCPUS CONFIG_KVM_ARM_MAX_VCPUS
> +#else
> +#define KVM_MAX_VCPUS 0
> +#endif
> +
>  #define KVM_USER_MEM_SLOTS 32
>  #define KVM_PRIVATE_MEM_SLOTS 4
>  #define KVM_COALESCED_MMIO_PAGE_OFFSET 1
> diff --git a/arch/arm64/kvm/Kconfig b/arch/arm64/kvm/Kconfig
> index 4480ab3..8ba85e9 100644
> --- a/arch/arm64/kvm/Kconfig
> +++ b/arch/arm64/kvm/Kconfig
> @@ -36,6 +36,17 @@ config KVM_ARM_HOST
>  	---help---
>  	  Provides host support for ARM processors.
>  
> +config KVM_ARM_MAX_VCPUS
> +	int "Number maximum supported virtual CPUs per VM"
> +	depends on KVM_ARM_HOST
> +	default 4
> +	help
> +	  Static number of max supported virtual CPUs per VM.
> +
> +	  If you choose a high number, the vcpu structures will be quite
> +	  large, so only choose a reasonable number that you expect to
> +	  actually use.
> +
>  config KVM_ARM_VGIC
>  	bool
>  	depends on KVM_ARM_HOST && OF
> 


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

[PATCH] arm64: KVM: Force undefined exception for Guest SMC intructions

[Marc Zyngier]

On 12/12/13 16:12, Anup Patel wrote:
> The SMC-based PSCI emulation for Guest is going to be very different
> from the in-kernel HVC-based PSCI emulation hence for now just inject
> undefined exception when Guest executes SMC instruction.
> 
> Signed-off-by: Anup Patel <anup.patel@linaro.org>
> Signed-off-by: Pranavkumar Sawargaonkar <pranavkumar@linaro.org>

Acked-by: Marc Zyngier <marc.zyngier@arm.com>

	M.

> ---
>  arch/arm64/kvm/handle_exit.c |    3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
> index 8da5606..df84d7b 100644
> --- a/arch/arm64/kvm/handle_exit.c
> +++ b/arch/arm64/kvm/handle_exit.c
> @@ -39,9 +39,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;
>  }
> 


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

[PATCH] arm64: KVM: Force undefined exception for Guest SMC intructions

[Christoffer Dall]

On Thu, Dec 12, 2013 at 09:42:23PM +0530, Anup Patel wrote:
> The SMC-based PSCI emulation for Guest is going to be very different
> from the in-kernel HVC-based PSCI emulation hence for now just inject
> undefined exception when Guest executes SMC instruction.

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>
> ---
>  arch/arm64/kvm/handle_exit.c |    3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
> index 8da5606..df84d7b 100644
> --- a/arch/arm64/kvm/handle_exit.c
> +++ b/arch/arm64/kvm/handle_exit.c
> @@ -39,9 +39,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
> 

-- 
Christoffer

[PATCH RESEND] arm64: KVM: Add Kconfig option for max VCPUs per-Guest

[Christoffer Dall]

On Thu, Dec 12, 2013 at 09:42:22PM +0530, Anup Patel wrote:
> Current max VCPUs per-Guest is set to 4 which is preventing
> us from creating a Guest (or VM) with 8 VCPUs on Host (e.g.
> X-Gene Storm SOC) with 8 Host CPUs.
> 
> The correct value of max VCPUs per-Guest should be same as
> the max CPUs supported by GICv2 which is 8 but, increasing
> value of max VCPUs per-Guest can make things slower hence
> we add Kconfig option to let KVM users select appropriate
> max VCPUs per-Guest.
> 

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>
> ---
>  arch/arm64/include/asm/kvm_host.h |    7 ++++++-
>  arch/arm64/kvm/Kconfig            |   11 +++++++++++
>  2 files changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index 5d85a02..0a1d697 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -26,7 +26,12 @@
>  #include <asm/kvm_asm.h>
>  #include <asm/kvm_mmio.h>
>  
> -#define KVM_MAX_VCPUS 4
> +#if defined(CONFIG_KVM_ARM_MAX_VCPUS)
> +#define KVM_MAX_VCPUS CONFIG_KVM_ARM_MAX_VCPUS
> +#else
> +#define KVM_MAX_VCPUS 0
> +#endif
> +
>  #define KVM_USER_MEM_SLOTS 32
>  #define KVM_PRIVATE_MEM_SLOTS 4
>  #define KVM_COALESCED_MMIO_PAGE_OFFSET 1
> diff --git a/arch/arm64/kvm/Kconfig b/arch/arm64/kvm/Kconfig
> index 4480ab3..8ba85e9 100644
> --- a/arch/arm64/kvm/Kconfig
> +++ b/arch/arm64/kvm/Kconfig
> @@ -36,6 +36,17 @@ config KVM_ARM_HOST
>  	---help---
>  	  Provides host support for ARM processors.
>  
> +config KVM_ARM_MAX_VCPUS
> +	int "Number maximum supported virtual CPUs per VM"
> +	depends on KVM_ARM_HOST
> +	default 4
> +	help
> +	  Static number of max supported virtual CPUs per VM.
> +
> +	  If you choose a high number, the vcpu structures will be quite
> +	  large, so only choose a reasonable number that you expect to
> +	  actually use.
> +
>  config KVM_ARM_VGIC
>  	bool
>  	depends on KVM_ARM_HOST && OF
> -- 
> 1.7.9.5
> 

-- 
Christoffer

[PATCH] KVM: Documentation: Fix typo for KVM_ARM_VCPU_INIT ioctl

[Christoffer Dall]

On Thu, Dec 12, 2013 at 09:42:24PM +0530, Anup Patel wrote:
> Fix minor typo in "Parameters:" of KVM_ARM_VCPU_INIT documentation.

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>
> ---
>  Documentation/virtual/kvm/api.txt |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
> index a30035d..aad3244 100644
> --- a/Documentation/virtual/kvm/api.txt
> +++ b/Documentation/virtual/kvm/api.txt
> @@ -2327,7 +2327,7 @@ current state.  "addr" is ignored.
>  Capability: basic
>  Architectures: arm, arm64
>  Type: vcpu ioctl
> -Parameters: struct struct kvm_vcpu_init (in)
> +Parameters: struct kvm_vcpu_init (in)
>  Returns: 0 on success; -1 on error
>  Errors:
>   ?EINVAL: ???the target is unknown, or the combination of features is invalid.
> -- 
> 1.7.9.5
> 

-- 
Christoffer

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

[Christoffer Dall]

On Thu, Dec 12, 2013 at 09:42:25PM +0530, Anup Patel wrote:
> The Power State and Coordination Interface (PSCI) specification defines
> SYSTEM_OFF and SYSTEM_RESET functions for system poweroff and reboot.
> 
> This patchset adds emulation of PSCI SYSTEM_OFF and SYSTEM_RESET functions
> in KVM ARM/ARM64 by forwarding them to user space (QEMU or KVMTOOL) using
> KVM_EXIT_SYSTEM_EVENT exit reason.
> 
> To try this patch from guest kernel, we will need PSCI-based restart and
> poweroff support in the guest kenel for both ARM and ARM64.
> 
> Rob Herring has already submitted patches for PSCI-based restart and
> poweroff in ARM kernel but these are not merged yet due unstable device
> tree bindings of kernel PSCI support. We will be having similar patches
> for PSCI-based restart and poweroff in ARM64 kernel.
> (Refer http://www.spinics.net/lists/arm-kernel/msg262217.html)
> (Refer http://www.spinics.net/lists/devicetree/msg05348.html)

There's something really funky about the threading and reply-to settings
of all of these patches, please check your e-mail sending tool config.

> 
> Change Log:
> 
> V2:
>  - Replace KVM_EXIT_RESET with KVM_EXIT_SYSTEM_EVENT
>  - Make kvm_psci_call() return convention to match 
>    KVM ARM return convention
> 
> V1:
>  - Initial revised patch after RFC PATCH.
> 
> Anup Patel (2):
>   KVM: Add KVM_EXIT_SYSTEM_EVENT to user space API header
>   ARM/ARM64: KVM: Forward PSCI SYSTEM_OFF and SYSTEM_RESET to user
>     space
> 
>  Documentation/virtual/kvm/api.txt |   15 ++++++++++++
>  arch/arm/include/asm/kvm_psci.h   |    2 +-
>  arch/arm/include/uapi/asm/kvm.h   |    2 ++
>  arch/arm/kvm/handle_exit.c        |   13 +++++++---
>  arch/arm/kvm/psci.c               |   48 ++++++++++++++++++++++++++++++-------
>  arch/arm64/include/asm/kvm_psci.h |    2 +-
>  arch/arm64/include/uapi/asm/kvm.h |    2 ++
>  arch/arm64/kvm/handle_exit.c      |   12 +++++++---
>  include/uapi/linux/kvm.h          |    8 +++++++
>  9 files changed, 88 insertions(+), 16 deletions(-)
> 
> -- 
> 1.7.9.5
> 

-- 
Christoffer

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

[Christoffer Dall]

On Thu, Dec 12, 2013 at 09:42:26PM +0530, Anup Patel wrote:
> 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 support to reset/shutdown VMs.
> 
> Signed-off-by: Anup Patel <anup.patel@linaro.org>
> Signed-off-by: Pranavkumar Sawargaonkar <pranavkumar@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 aad3244..4ef180e 100644
> --- a/Documentation/virtual/kvm/api.txt
> +++ b/Documentation/virtual/kvm/api.txt
> @@ -2699,6 +2699,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 902f124..bb13faa 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];
>  	};
> -- 
> 1.7.9.5
> 

Looks good to me:

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

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

[Christoffer Dall]

On Thu, Dec 12, 2013 at 09:42:27PM +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 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>
> ---
>  arch/arm/include/asm/kvm_psci.h   |    2 +-
>  arch/arm/include/uapi/asm/kvm.h   |    2 ++
>  arch/arm/kvm/handle_exit.c        |   13 +++++++---
>  arch/arm/kvm/psci.c               |   48 ++++++++++++++++++++++++++++++-------
>  arch/arm64/include/asm/kvm_psci.h |    2 +-
>  arch/arm64/include/uapi/asm/kvm.h |    2 ++
>  arch/arm64/kvm/handle_exit.c      |   12 +++++++---
>  7 files changed, 65 insertions(+), 16 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..c3f0e72 100644
> --- a/arch/arm/kvm/handle_exit.c
> +++ b/arch/arm/kvm/handle_exit.c
> @@ -40,14 +40,21 @@ 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 ret;
> +	else if (ret == -EINVAL) {
> +		kvm_inject_undefined(vcpu);
>  		return 1;
> +	}
> +
> +	return ret;

Wouldn't

ret = kvm_psci_call(vcpu);
if (ret == -EINVAL) {
	kvm_inject_undefined(vcpu);
	return 1;
}

return ret;

be simpler?

also, minor nit, but I think according to the CodingStyle if you have
single statements in if cases, but they are part of a larger if
statement with alternative clauses that use curly-brackets to
encapsulate multiple statements, then you should use curly braces around
the single-line statement as well...

>  
> -	kvm_inject_undefined(vcpu);
> -	return 1;
>  }
>  
>  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 0881bf1..8e246d6 100644
> --- a/arch/arm/kvm/psci.c
> +++ b/arch/arm/kvm/psci.c
> @@ -84,18 +84,41 @@ static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu)
>  	return KVM_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);
> +}
> +
>  /**
>   * 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

the function number

> + * specified in r0 is withing the PSCI range.

this reads a little funny now.  I think you can get rid of everything
after the first mention of r0 (cut from "and function number...") since
you explain that in the error codes below.

> + *
> + * This function returns: > 0 (success), 0 (success but exit to user
> + * space), and < 0 (errors)

you could use @return: instead of "This function returns", but I'm not
sure if anyone actually cares.

> + *
> + * Errors:
> + * -EINVAL: Unrecognized PSCI function
>   */
> -bool kvm_psci_call(struct kvm_vcpu *vcpu)
> +int kvm_psci_call(struct kvm_vcpu *vcpu)
>  {
> +	int ret = 1;
>  	unsigned long psci_fn = *vcpu_reg(vcpu, 0) & ~((u32) 0);
>  	unsigned long val;
>  
> @@ -111,11 +134,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 = 0;
> +		break;
> +	case KVM_PSCI_FN_SYSTEM_RESET:
> +		kvm_psci_system_reset(vcpu);
> +		val = KVM_PSCI_RET_SUCCESS;
> +		ret = 0;
> +		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 df84d7b..e382eb8 100644
> --- a/arch/arm64/kvm/handle_exit.c
> +++ b/arch/arm64/kvm/handle_exit.c
> @@ -30,11 +30,17 @@ 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 ret;
> +	else if (ret == -EINVAL) {
> +		kvm_inject_undefined(vcpu);
>  		return 1;
> +	}
>  
> -	kvm_inject_undefined(vcpu);
> -	return 1;
> +	return ret;

same as above.

>  }
>  
>  static int handle_smc(struct kvm_vcpu *vcpu, struct kvm_run *run)
> -- 
> 1.7.9.5
> 

Aside from the code formatting and commenting above, it looks pretty
good to me.

-Christoffer

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

[Anup Patel]

On Sat, Dec 14, 2013 at 1:38 AM, Christoffer Dall
<christoffer.dall@linaro.org> wrote:
> On Thu, Dec 12, 2013 at 09:42:27PM +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 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>
>> ---
>>  arch/arm/include/asm/kvm_psci.h   |    2 +-
>>  arch/arm/include/uapi/asm/kvm.h   |    2 ++
>>  arch/arm/kvm/handle_exit.c        |   13 +++++++---
>>  arch/arm/kvm/psci.c               |   48 ++++++++++++++++++++++++++++++-------
>>  arch/arm64/include/asm/kvm_psci.h |    2 +-
>>  arch/arm64/include/uapi/asm/kvm.h |    2 ++
>>  arch/arm64/kvm/handle_exit.c      |   12 +++++++---
>>  7 files changed, 65 insertions(+), 16 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..c3f0e72 100644
>> --- a/arch/arm/kvm/handle_exit.c
>> +++ b/arch/arm/kvm/handle_exit.c
>> @@ -40,14 +40,21 @@ 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 ret;
>> +     else if (ret == -EINVAL) {
>> +             kvm_inject_undefined(vcpu);
>>               return 1;
>> +     }
>> +
>> +     return ret;
>
> Wouldn't
>
> ret = kvm_psci_call(vcpu);
> if (ret == -EINVAL) {
>         kvm_inject_undefined(vcpu);
>         return 1;
> }
>
> return ret;
>
> be simpler?

Yes, I will make this simpler.

>
> also, minor nit, but I think according to the CodingStyle if you have
> single statements in if cases, but they are part of a larger if
> statement with alternative clauses that use curly-brackets to
> encapsulate multiple statements, then you should use curly braces around
> the single-line statement as well...
>
>>
>> -     kvm_inject_undefined(vcpu);
>> -     return 1;
>>  }
>>
>>  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 0881bf1..8e246d6 100644
>> --- a/arch/arm/kvm/psci.c
>> +++ b/arch/arm/kvm/psci.c
>> @@ -84,18 +84,41 @@ static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu)
>>       return KVM_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);
>> +}
>> +
>>  /**
>>   * 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
>
> the function number
>
>> + * specified in r0 is withing the PSCI range.
>
> this reads a little funny now.  I think you can get rid of everything
> after the first mention of r0 (cut from "and function number...") since
> you explain that in the error codes below.

Sure, I will update this comment.

>
>> + *
>> + * This function returns: > 0 (success), 0 (success but exit to user
>> + * space), and < 0 (errors)
>
> you could use @return: instead of "This function returns", but I'm not
> sure if anyone actually cares.

Actually, "@returns" is correct way to specify as-per doxygen
comment style for C but I have seen "Returns:" or "Returns" being
used commonly in other parts of kernel. I will replace "The function
returns:" with "Returns:".

>
>> + *
>> + * Errors:
>> + * -EINVAL: Unrecognized PSCI function
>>   */
>> -bool kvm_psci_call(struct kvm_vcpu *vcpu)
>> +int kvm_psci_call(struct kvm_vcpu *vcpu)
>>  {
>> +     int ret = 1;
>>       unsigned long psci_fn = *vcpu_reg(vcpu, 0) & ~((u32) 0);
>>       unsigned long val;
>>
>> @@ -111,11 +134,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 = 0;
>> +             break;
>> +     case KVM_PSCI_FN_SYSTEM_RESET:
>> +             kvm_psci_system_reset(vcpu);
>> +             val = KVM_PSCI_RET_SUCCESS;
>> +             ret = 0;
>> +             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 df84d7b..e382eb8 100644
>> --- a/arch/arm64/kvm/handle_exit.c
>> +++ b/arch/arm64/kvm/handle_exit.c
>> @@ -30,11 +30,17 @@ 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 ret;
>> +     else if (ret == -EINVAL) {
>> +             kvm_inject_undefined(vcpu);
>>               return 1;
>> +     }
>>
>> -     kvm_inject_undefined(vcpu);
>> -     return 1;
>> +     return ret;
>
> same as above.

OK.

>
>>  }
>>
>>  static int handle_smc(struct kvm_vcpu *vcpu, struct kvm_run *run)
>> --
>> 1.7.9.5
>>
>
> Aside from the code formatting and commenting above, it looks pretty
> good to me.
>
> -Christoffer
> _______________________________________________
> kvmarm mailing list
> kvmarm at lists.cs.columbia.edu
> https://lists.cs.columbia.edu/cucslists/listinfo/kvmarm

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

[Christoffer Dall]

On Sat, Dec 14, 2013 at 06:54:58PM +0530, Anup Patel wrote:
> On Sat, Dec 14, 2013 at 1:38 AM, Christoffer Dall
> <christoffer.dall@linaro.org> wrote:
> > On Thu, Dec 12, 2013 at 09:42:27PM +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 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>
> >> ---
> >>  arch/arm/include/asm/kvm_psci.h   |    2 +-
> >>  arch/arm/include/uapi/asm/kvm.h   |    2 ++
> >>  arch/arm/kvm/handle_exit.c        |   13 +++++++---
> >>  arch/arm/kvm/psci.c               |   48 ++++++++++++++++++++++++++++++-------
> >>  arch/arm64/include/asm/kvm_psci.h |    2 +-
> >>  arch/arm64/include/uapi/asm/kvm.h |    2 ++
> >>  arch/arm64/kvm/handle_exit.c      |   12 +++++++---
> >>  7 files changed, 65 insertions(+), 16 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..c3f0e72 100644
> >> --- a/arch/arm/kvm/handle_exit.c
> >> +++ b/arch/arm/kvm/handle_exit.c
> >> @@ -40,14 +40,21 @@ 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 ret;
> >> +     else if (ret == -EINVAL) {
> >> +             kvm_inject_undefined(vcpu);
> >>               return 1;
> >> +     }
> >> +
> >> +     return ret;
> >
> > Wouldn't
> >
> > ret = kvm_psci_call(vcpu);
> > if (ret == -EINVAL) {
> >         kvm_inject_undefined(vcpu);
> >         return 1;
> > }
> >
> > return ret;
> >
> > be simpler?
> 
> Yes, I will make this simpler.
> 
> >
> > also, minor nit, but I think according to the CodingStyle if you have
> > single statements in if cases, but they are part of a larger if
> > statement with alternative clauses that use curly-brackets to
> > encapsulate multiple statements, then you should use curly braces around
> > the single-line statement as well...
> >
> >>
> >> -     kvm_inject_undefined(vcpu);
> >> -     return 1;
> >>  }
> >>
> >>  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 0881bf1..8e246d6 100644
> >> --- a/arch/arm/kvm/psci.c
> >> +++ b/arch/arm/kvm/psci.c
> >> @@ -84,18 +84,41 @@ static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu)
> >>       return KVM_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);
> >> +}
> >> +
> >>  /**
> >>   * 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
> >
> > the function number
> >
> >> + * specified in r0 is withing the PSCI range.
> >
> > this reads a little funny now.  I think you can get rid of everything
> > after the first mention of r0 (cut from "and function number...") since
> > you explain that in the error codes below.
> 
> Sure, I will update this comment.
> 
> >
> >> + *
> >> + * This function returns: > 0 (success), 0 (success but exit to user
> >> + * space), and < 0 (errors)
> >
> > you could use @return: instead of "This function returns", but I'm not
> > sure if anyone actually cares.
> 
> Actually, "@returns" is correct way to specify as-per doxygen
> comment style for C but I have seen "Returns:" or "Returns" being
> used commonly in other parts of kernel. I will replace "The function
> returns:" with "Returns:".
> 

Yes, but it doesn't seem that's what the kernel preferred style is
following:

$ grep -sInR '@return' * | wc -l
583
$ grep -sInR '@returns' * | wc -l
82
$ grep -sInR '@Returns' * | wc -l
1
$ grep -sInR '@Return' * | wc -l
1

Anyway, it's not a big deal in any case.

[...]

-Christoffer