[PATCH v2 0/6] Improve PSCI system events and fix reboot bugs

[PATCH v2 0/6] Improve PSCI system events and fix reboot bugs

[Christoffer Dall]

Several people have reported problems with rebooting ARM VMs, especially
on 32-bit ARM.  This is mainly due to the same reason we were seeing
boot errors in the past, namely that the ram, dcache, and icache weren't
coherent on guest boot with the guest (stage-1) MMU disabled.  We solved
this by ensuring coherency when we fault in pages, but since most memory
is already mapped after a reboot, we don't do anything.

The solution is to unmap the regular RAM on VCPU init, but we must
take care to not unmap the GIC or other IO regions, hence the somehwat
complicated solution.

As part of figuring this out, it became clear that some semantics around
the KVM_ARM_VCPU_INIT ABI and system event ABI was unclear (what is
userspace expected to do when it receives a system event).  This series
also clarifies the ABI and changes the kernel functionality to do what
userspace expects (turn off VCPUs on a system shutdown event).

The code is avaliable here as well:
http://git.linaro.org/people/christoffer.dall/linux-kvm-arm.git vcpu_init_fixes-v2

There is an alternative version with more code-reuse for the unmapping
implementation for the previous version of this patch series available
in the following git repo:

http://git.linaro.org/people/christoffer.dall/linux-kvm-arm.git vcpu_init_fixes-alternative

Testing
-------
This has been tested on CubieBoard, Arndale, TC2, and Juno.  On Arndale
and TC2 it was extremely easy to reproduce the problem (just start a VM
that runs reboot from /etc/rc.local or similar) and this series clearly
fixes the behavior.  For the previous version of this series, I was
seeing some problems on Juno, but it turned out to be because I wasn't
limiting my testing to one of the clusters, and since we don't support
re-initing a VCPU on a different physical host CPU (big.LITTLE), it was
failing.  For this version of the patch series, it has been running a
reboot loop on Juno for hours.

Changelog
---------
Changes v1->v2:
 - New patch to not clear the VCPU_POWER_OFF flag
 - Fixed spelling error in commit message
 - Adapted ABI texts based on Peter's feedback
 - Check for changed parameters to KVM_ARM_VCPU_INIT
 - Now unmap the Stage-2 RAM mappings at VCPU init instead of at PSCI
   system event time.

Christoffer Dall (6):
  arm/arm64: KVM: Don't clear the VCPU_POWER_OFF flag
  arm/arm64: KVM: Correct KVM_ARM_VCPU_INIT power off option
  arm/arm64: KVM: Reset the HCR on each vcpu when resetting the vcpu
  arm/arm64: KVM: Clarify KVM_ARM_VCPU_INIT ABI
  arm/arm64: KVM: Turn off vcpus on PSCI shutdown/reboot
  arm/arm64: KVM: Introduce stage2_unmap_vm

 Documentation/virtual/kvm/api.txt    | 17 +++++++++-
 arch/arm/include/asm/kvm_emulate.h   |  5 +++
 arch/arm/include/asm/kvm_host.h      |  2 --
 arch/arm/include/asm/kvm_mmu.h       |  1 +
 arch/arm/kvm/arm.c                   | 56 ++++++++++++++++++++++++++++++-
 arch/arm/kvm/guest.c                 | 26 ---------------
 arch/arm/kvm/mmu.c                   | 65 ++++++++++++++++++++++++++++++++++++
 arch/arm/kvm/psci.c                  | 19 +++++++++++
 arch/arm64/include/asm/kvm_emulate.h |  5 +++
 arch/arm64/include/asm/kvm_host.h    |  3 +-
 arch/arm64/include/asm/kvm_mmu.h     |  1 +
 arch/arm64/kvm/guest.c               | 26 ---------------
 12 files changed, 168 insertions(+), 58 deletions(-)

-- 
2.1.2.330.g565301e.dirty

[PATCH v2 1/6] arm/arm64: KVM: Don't clear the VCPU_POWER_OFF flag

[Christoffer Dall]

If a VCPU was originally started with power off (typically to be brought
up by PSCI in SMP configurations), there is no need to clear the
POWER_OFF flag in the kernel, as this flag is only tested during the
init ioctl itself.

Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
---
 arch/arm/kvm/arm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
index 9e193c8..b160bea 100644
--- a/arch/arm/kvm/arm.c
+++ b/arch/arm/kvm/arm.c
@@ -661,7 +661,7 @@ static int kvm_arch_vcpu_ioctl_vcpu_init(struct kvm_vcpu *vcpu,
 	/*
 	 * Handle the "start in power-off" case by marking the VCPU as paused.
 	 */
-	if (__test_and_clear_bit(KVM_ARM_VCPU_POWER_OFF, vcpu->arch.features))
+	if (test_bit(KVM_ARM_VCPU_POWER_OFF, vcpu->arch.features))
 		vcpu->arch.pause = true;
 
 	return 0;
-- 
2.1.2.330.g565301e.dirty

[PATCH v2 1/6] arm/arm64: KVM: Don't clear the VCPU_POWER_OFF flag

[Marc Zyngier]

On 03/12/14 21:18, Christoffer Dall wrote:
> If a VCPU was originally started with power off (typically to be brought
> up by PSCI in SMP configurations), there is no need to clear the
> POWER_OFF flag in the kernel, as this flag is only tested during the
> init ioctl itself.
> 
> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> ---
>  arch/arm/kvm/arm.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
> index 9e193c8..b160bea 100644
> --- a/arch/arm/kvm/arm.c
> +++ b/arch/arm/kvm/arm.c
> @@ -661,7 +661,7 @@ static int kvm_arch_vcpu_ioctl_vcpu_init(struct kvm_vcpu *vcpu,
>  	/*
>  	 * Handle the "start in power-off" case by marking the VCPU as paused.
>  	 */
> -	if (__test_and_clear_bit(KVM_ARM_VCPU_POWER_OFF, vcpu->arch.features))
> +	if (test_bit(KVM_ARM_VCPU_POWER_OFF, vcpu->arch.features))
>  		vcpu->arch.pause = true;
>  
>  	return 0;
> 

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

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

[PATCH v2 2/6] arm/arm64: KVM: Correct KVM_ARM_VCPU_INIT power off option

[Christoffer Dall]

The implementation of KVM_ARM_VCPU_INIT is currently not doing what
userspace expects, namely making sure that a vcpu which may have been
turned off using PSCI is returned to its initial state, which would be
powered on if userspace does not set the KVM_ARM_VCPU_POWER_OFF flag.

Implement the expected functionality and clarify the ABI.

Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
---
 Documentation/virtual/kvm/api.txt | 3 ++-
 arch/arm/kvm/arm.c                | 2 ++
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
index 7610eaa..bb82a90 100644
--- a/Documentation/virtual/kvm/api.txt
+++ b/Documentation/virtual/kvm/api.txt
@@ -2455,7 +2455,8 @@ should be created before this ioctl is invoked.
 
 Possible features:
 	- KVM_ARM_VCPU_POWER_OFF: Starts the CPU in a power-off state.
-	  Depends on KVM_CAP_ARM_PSCI.
+	  Depends on KVM_CAP_ARM_PSCI.  If not set, the CPU will be powered on
+	  and execute guest code when KVM_RUN is called.
 	- KVM_ARM_VCPU_EL1_32BIT: Starts the CPU in a 32bit mode.
 	  Depends on KVM_CAP_ARM_EL1_32BIT (arm64 only).
 	- KVM_ARM_VCPU_PSCI_0_2: Emulate PSCI v0.2 for the CPU.
diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
index b160bea..edc1964 100644
--- a/arch/arm/kvm/arm.c
+++ b/arch/arm/kvm/arm.c
@@ -663,6 +663,8 @@ static int kvm_arch_vcpu_ioctl_vcpu_init(struct kvm_vcpu *vcpu,
 	 */
 	if (test_bit(KVM_ARM_VCPU_POWER_OFF, vcpu->arch.features))
 		vcpu->arch.pause = true;
+	else
+		vcpu->arch.pause = false;
 
 	return 0;
 }
-- 
2.1.2.330.g565301e.dirty

[PATCH v2 2/6] arm/arm64: KVM: Correct KVM_ARM_VCPU_INIT power off option

[Marc Zyngier]

On 03/12/14 21:18, Christoffer Dall wrote:
> The implementation of KVM_ARM_VCPU_INIT is currently not doing what
> userspace expects, namely making sure that a vcpu which may have been
> turned off using PSCI is returned to its initial state, which would be
> powered on if userspace does not set the KVM_ARM_VCPU_POWER_OFF flag.
> 
> Implement the expected functionality and clarify the ABI.
> 
> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> ---
>  Documentation/virtual/kvm/api.txt | 3 ++-
>  arch/arm/kvm/arm.c                | 2 ++
>  2 files changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
> index 7610eaa..bb82a90 100644
> --- a/Documentation/virtual/kvm/api.txt
> +++ b/Documentation/virtual/kvm/api.txt
> @@ -2455,7 +2455,8 @@ should be created before this ioctl is invoked.
>  
>  Possible features:
>  	- KVM_ARM_VCPU_POWER_OFF: Starts the CPU in a power-off state.
> -	  Depends on KVM_CAP_ARM_PSCI.
> +	  Depends on KVM_CAP_ARM_PSCI.  If not set, the CPU will be powered on
> +	  and execute guest code when KVM_RUN is called.
>  	- KVM_ARM_VCPU_EL1_32BIT: Starts the CPU in a 32bit mode.
>  	  Depends on KVM_CAP_ARM_EL1_32BIT (arm64 only).
>  	- KVM_ARM_VCPU_PSCI_0_2: Emulate PSCI v0.2 for the CPU.
> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
> index b160bea..edc1964 100644
> --- a/arch/arm/kvm/arm.c
> +++ b/arch/arm/kvm/arm.c
> @@ -663,6 +663,8 @@ static int kvm_arch_vcpu_ioctl_vcpu_init(struct kvm_vcpu *vcpu,
>  	 */
>  	if (test_bit(KVM_ARM_VCPU_POWER_OFF, vcpu->arch.features))
>  		vcpu->arch.pause = true;
> +	else
> +		vcpu->arch.pause = false;
>  
>  	return 0;
>  }
> 

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

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

[PATCH v2 3/6] arm/arm64: KVM: Reset the HCR on each vcpu when resetting the vcpu

[Christoffer Dall]

When userspace resets the vcpu using KVM_ARM_VCPU_INIT, we should also
reset the HCR, because we now modify the HCR dynamically to
enable/disable trapping of guest accesses to the VM registers.

This is crucial for reboot of VMs working since otherwise we will not be
doing the necessary cache maintenance operations when faulting in pages
with the guest MMU off.

Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
---
 arch/arm/include/asm/kvm_emulate.h   | 5 +++++
 arch/arm/kvm/arm.c                   | 2 ++
 arch/arm/kvm/guest.c                 | 1 -
 arch/arm64/include/asm/kvm_emulate.h | 5 +++++
 arch/arm64/kvm/guest.c               | 1 -
 5 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/arch/arm/include/asm/kvm_emulate.h b/arch/arm/include/asm/kvm_emulate.h
index b9db269..66ce176 100644
--- a/arch/arm/include/asm/kvm_emulate.h
+++ b/arch/arm/include/asm/kvm_emulate.h
@@ -33,6 +33,11 @@ void kvm_inject_undefined(struct kvm_vcpu *vcpu);
 void kvm_inject_dabt(struct kvm_vcpu *vcpu, unsigned long addr);
 void kvm_inject_pabt(struct kvm_vcpu *vcpu, unsigned long addr);
 
+static inline void vcpu_reset_hcr(struct kvm_vcpu *vcpu)
+{
+	vcpu->arch.hcr = HCR_GUEST_MASK;
+}
+
 static inline bool vcpu_mode_is_32bit(struct kvm_vcpu *vcpu)
 {
 	return 1;
diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
index edc1964..24c9ca4 100644
--- a/arch/arm/kvm/arm.c
+++ b/arch/arm/kvm/arm.c
@@ -658,6 +658,8 @@ static int kvm_arch_vcpu_ioctl_vcpu_init(struct kvm_vcpu *vcpu,
 	if (ret)
 		return ret;
 
+	vcpu_reset_hcr(vcpu);
+
 	/*
 	 * Handle the "start in power-off" case by marking the VCPU as paused.
 	 */
diff --git a/arch/arm/kvm/guest.c b/arch/arm/kvm/guest.c
index cc0b787..8c97208 100644
--- a/arch/arm/kvm/guest.c
+++ b/arch/arm/kvm/guest.c
@@ -38,7 +38,6 @@ struct kvm_stats_debugfs_item debugfs_entries[] = {
 
 int kvm_arch_vcpu_setup(struct kvm_vcpu *vcpu)
 {
-	vcpu->arch.hcr = HCR_GUEST_MASK;
 	return 0;
 }
 
diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
index 5674a55..8127e45 100644
--- a/arch/arm64/include/asm/kvm_emulate.h
+++ b/arch/arm64/include/asm/kvm_emulate.h
@@ -38,6 +38,11 @@ void kvm_inject_undefined(struct kvm_vcpu *vcpu);
 void kvm_inject_dabt(struct kvm_vcpu *vcpu, unsigned long addr);
 void kvm_inject_pabt(struct kvm_vcpu *vcpu, unsigned long addr);
 
+static inline void vcpu_reset_hcr(struct kvm_vcpu *vcpu)
+{
+	vcpu->arch.hcr_el2 = HCR_GUEST_FLAGS;
+}
+
 static inline unsigned long *vcpu_pc(const struct kvm_vcpu *vcpu)
 {
 	return (unsigned long *)&vcpu_gp_regs(vcpu)->regs.pc;
diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c
index 7679469..84d5959 100644
--- a/arch/arm64/kvm/guest.c
+++ b/arch/arm64/kvm/guest.c
@@ -38,7 +38,6 @@ struct kvm_stats_debugfs_item debugfs_entries[] = {
 
 int kvm_arch_vcpu_setup(struct kvm_vcpu *vcpu)
 {
-	vcpu->arch.hcr_el2 = HCR_GUEST_FLAGS;
 	return 0;
 }
 
-- 
2.1.2.330.g565301e.dirty

[PATCH v2 3/6] arm/arm64: KVM: Reset the HCR on each vcpu when resetting the vcpu

[Marc Zyngier]

On 03/12/14 21:18, Christoffer Dall wrote:
> When userspace resets the vcpu using KVM_ARM_VCPU_INIT, we should also
> reset the HCR, because we now modify the HCR dynamically to
> enable/disable trapping of guest accesses to the VM registers.
> 
> This is crucial for reboot of VMs working since otherwise we will not be
> doing the necessary cache maintenance operations when faulting in pages
> with the guest MMU off.
> 
> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> ---
>  arch/arm/include/asm/kvm_emulate.h   | 5 +++++
>  arch/arm/kvm/arm.c                   | 2 ++
>  arch/arm/kvm/guest.c                 | 1 -
>  arch/arm64/include/asm/kvm_emulate.h | 5 +++++
>  arch/arm64/kvm/guest.c               | 1 -
>  5 files changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/include/asm/kvm_emulate.h b/arch/arm/include/asm/kvm_emulate.h
> index b9db269..66ce176 100644
> --- a/arch/arm/include/asm/kvm_emulate.h
> +++ b/arch/arm/include/asm/kvm_emulate.h
> @@ -33,6 +33,11 @@ void kvm_inject_undefined(struct kvm_vcpu *vcpu);
>  void kvm_inject_dabt(struct kvm_vcpu *vcpu, unsigned long addr);
>  void kvm_inject_pabt(struct kvm_vcpu *vcpu, unsigned long addr);
>  
> +static inline void vcpu_reset_hcr(struct kvm_vcpu *vcpu)
> +{
> +	vcpu->arch.hcr = HCR_GUEST_MASK;
> +}
> +
>  static inline bool vcpu_mode_is_32bit(struct kvm_vcpu *vcpu)
>  {
>  	return 1;
> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
> index edc1964..24c9ca4 100644
> --- a/arch/arm/kvm/arm.c
> +++ b/arch/arm/kvm/arm.c
> @@ -658,6 +658,8 @@ static int kvm_arch_vcpu_ioctl_vcpu_init(struct kvm_vcpu *vcpu,
>  	if (ret)
>  		return ret;
>  
> +	vcpu_reset_hcr(vcpu);
> +
>  	/*
>  	 * Handle the "start in power-off" case by marking the VCPU as paused.
>  	 */
> diff --git a/arch/arm/kvm/guest.c b/arch/arm/kvm/guest.c
> index cc0b787..8c97208 100644
> --- a/arch/arm/kvm/guest.c
> +++ b/arch/arm/kvm/guest.c
> @@ -38,7 +38,6 @@ struct kvm_stats_debugfs_item debugfs_entries[] = {
>  
>  int kvm_arch_vcpu_setup(struct kvm_vcpu *vcpu)
>  {
> -	vcpu->arch.hcr = HCR_GUEST_MASK;
>  	return 0;
>  }
>  
> diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
> index 5674a55..8127e45 100644
> --- a/arch/arm64/include/asm/kvm_emulate.h
> +++ b/arch/arm64/include/asm/kvm_emulate.h
> @@ -38,6 +38,11 @@ void kvm_inject_undefined(struct kvm_vcpu *vcpu);
>  void kvm_inject_dabt(struct kvm_vcpu *vcpu, unsigned long addr);
>  void kvm_inject_pabt(struct kvm_vcpu *vcpu, unsigned long addr);
>  
> +static inline void vcpu_reset_hcr(struct kvm_vcpu *vcpu)
> +{
> +	vcpu->arch.hcr_el2 = HCR_GUEST_FLAGS;
> +}
> +
>  static inline unsigned long *vcpu_pc(const struct kvm_vcpu *vcpu)
>  {
>  	return (unsigned long *)&vcpu_gp_regs(vcpu)->regs.pc;
> diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c
> index 7679469..84d5959 100644
> --- a/arch/arm64/kvm/guest.c
> +++ b/arch/arm64/kvm/guest.c
> @@ -38,7 +38,6 @@ struct kvm_stats_debugfs_item debugfs_entries[] = {
>  
>  int kvm_arch_vcpu_setup(struct kvm_vcpu *vcpu)
>  {
> -	vcpu->arch.hcr_el2 = HCR_GUEST_FLAGS;
>  	return 0;
>  }
>  
> 

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

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

[PATCH v2 4/6] arm/arm64: KVM: Clarify KVM_ARM_VCPU_INIT ABI

[Christoffer Dall]

It is not clear that this ioctl can be called multiple times for a given
vcpu.  Userspace already does this, so clarify the ABI.

Also specify that userspace is expected to always make secondary and
subsequent calls to the ioctl with the same parameters for the VCPU as
the initial call (which userspace also already does).

Add code to check that userspace doesn't violate that ABI in the future,
and move the kvm_vcpu_set_target() function which is currently
duplicated between the 32-bit and 64-bit versions in guest.c to a common
static function in arm.c, shared between both architectures.

Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
---
 Documentation/virtual/kvm/api.txt |  5 +++++
 arch/arm/include/asm/kvm_host.h   |  2 --
 arch/arm/kvm/arm.c                | 43 +++++++++++++++++++++++++++++++++++++++
 arch/arm/kvm/guest.c              | 25 -----------------------
 arch/arm64/include/asm/kvm_host.h |  2 --
 arch/arm64/kvm/guest.c            | 25 -----------------------
 6 files changed, 48 insertions(+), 54 deletions(-)

diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
index bb82a90..81f1b97 100644
--- a/Documentation/virtual/kvm/api.txt
+++ b/Documentation/virtual/kvm/api.txt
@@ -2453,6 +2453,11 @@ return ENOEXEC for that vcpu.
 Note that because some registers reflect machine topology, all vcpus
 should be created before this ioctl is invoked.
 
+Userspace can call this function multiple times for a given vcpu, including
+after the vcpu has been run. This will reset the vcpu to its initial
+state. All calls to this function after the initial call must use the same
+target and same set of feature flags, otherwise EINVAL will be returned.
+
 Possible features:
 	- KVM_ARM_VCPU_POWER_OFF: Starts the CPU in a power-off state.
 	  Depends on KVM_CAP_ARM_PSCI.  If not set, the CPU will be powered on
diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
index 53036e2..254e065 100644
--- a/arch/arm/include/asm/kvm_host.h
+++ b/arch/arm/include/asm/kvm_host.h
@@ -150,8 +150,6 @@ struct kvm_vcpu_stat {
 	u32 halt_wakeup;
 };
 
-int kvm_vcpu_set_target(struct kvm_vcpu *vcpu,
-			const struct kvm_vcpu_init *init);
 int kvm_vcpu_preferred_target(struct kvm_vcpu_init *init);
 unsigned long kvm_arm_num_regs(struct kvm_vcpu *vcpu);
 int kvm_arm_copy_reg_indices(struct kvm_vcpu *vcpu, u64 __user *indices);
diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
index 24c9ca4..4043769 100644
--- a/arch/arm/kvm/arm.c
+++ b/arch/arm/kvm/arm.c
@@ -263,6 +263,7 @@ int kvm_arch_vcpu_init(struct kvm_vcpu *vcpu)
 {
 	/* Force users to call KVM_ARM_VCPU_INIT */
 	vcpu->arch.target = -1;
+	bitmap_zero(vcpu->arch.features, KVM_VCPU_MAX_FEATURES);
 
 	/* Set up the timer */
 	kvm_timer_vcpu_init(vcpu);
@@ -649,6 +650,48 @@ int kvm_vm_ioctl_irq_line(struct kvm *kvm, struct kvm_irq_level *irq_level,
 	return -EINVAL;
 }
 
+static int kvm_vcpu_set_target(struct kvm_vcpu *vcpu,
+			       const struct kvm_vcpu_init *init)
+{
+	unsigned int i;
+	int phys_target = kvm_target_cpu();
+
+	if (init->target != phys_target)
+		return -EINVAL;
+
+	/*
+	 * Secondary and subsequent calls to KVM_ARM_VCPU_INIT must
+	 * use the same target.
+	 */
+	if (vcpu->arch.target != -1 && vcpu->arch.target != init->target)
+		return -EINVAL;
+
+	/* -ENOENT for unknown features, -EINVAL for invalid combinations. */
+	for (i = 0; i < sizeof(init->features) * 8; i++) {
+		bool set = (init->features[i / 32] & (1 << (i % 32)));
+
+		if (set && i >= KVM_VCPU_MAX_FEATURES)
+			return -ENOENT;
+
+		/*
+		 * Secondary and subsequent calls to KVM_ARM_VCPU_INIT must
+		 * use the same feature set.
+		 */
+		if (vcpu->arch.target != -1 && i < KVM_VCPU_MAX_FEATURES &&
+		    test_bit(i, vcpu->arch.features) != set)
+			return -EINVAL;
+
+		if (set)
+			set_bit(i, vcpu->arch.features);
+	}
+
+	vcpu->arch.target = phys_target;
+
+	/* Now we know what it is, we can reset it. */
+	return kvm_reset_vcpu(vcpu);
+}
+
+
 static int kvm_arch_vcpu_ioctl_vcpu_init(struct kvm_vcpu *vcpu,
 					 struct kvm_vcpu_init *init)
 {
diff --git a/arch/arm/kvm/guest.c b/arch/arm/kvm/guest.c
index 8c97208..384bab6 100644
--- a/arch/arm/kvm/guest.c
+++ b/arch/arm/kvm/guest.c
@@ -273,31 +273,6 @@ int __attribute_const__ kvm_target_cpu(void)
 	}
 }
 
-int kvm_vcpu_set_target(struct kvm_vcpu *vcpu,
-			const struct kvm_vcpu_init *init)
-{
-	unsigned int i;
-
-	/* We can only cope with guest==host and only on A15/A7 (for now). */
-	if (init->target != kvm_target_cpu())
-		return -EINVAL;
-
-	vcpu->arch.target = init->target;
-	bitmap_zero(vcpu->arch.features, KVM_VCPU_MAX_FEATURES);
-
-	/* -ENOENT for unknown features, -EINVAL for invalid combinations. */
-	for (i = 0; i < sizeof(init->features) * 8; i++) {
-		if (test_bit(i, (void *)init->features)) {
-			if (i >= KVM_VCPU_MAX_FEATURES)
-				return -ENOENT;
-			set_bit(i, vcpu->arch.features);
-		}
-	}
-
-	/* Now we know what it is, we can reset it. */
-	return kvm_reset_vcpu(vcpu);
-}
-
 int kvm_vcpu_preferred_target(struct kvm_vcpu_init *init)
 {
 	int target = kvm_target_cpu();
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 2012c4b..65c6152 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -165,8 +165,6 @@ struct kvm_vcpu_stat {
 	u32 halt_wakeup;
 };
 
-int kvm_vcpu_set_target(struct kvm_vcpu *vcpu,
-			const struct kvm_vcpu_init *init);
 int kvm_vcpu_preferred_target(struct kvm_vcpu_init *init);
 unsigned long kvm_arm_num_regs(struct kvm_vcpu *vcpu);
 int kvm_arm_copy_reg_indices(struct kvm_vcpu *vcpu, u64 __user *indices);
diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c
index 84d5959..9535bd5 100644
--- a/arch/arm64/kvm/guest.c
+++ b/arch/arm64/kvm/guest.c
@@ -296,31 +296,6 @@ int __attribute_const__ kvm_target_cpu(void)
 	return -EINVAL;
 }
 
-int kvm_vcpu_set_target(struct kvm_vcpu *vcpu,
-			const struct kvm_vcpu_init *init)
-{
-	unsigned int i;
-	int phys_target = kvm_target_cpu();
-
-	if (init->target != phys_target)
-		return -EINVAL;
-
-	vcpu->arch.target = phys_target;
-	bitmap_zero(vcpu->arch.features, KVM_VCPU_MAX_FEATURES);
-
-	/* -ENOENT for unknown features, -EINVAL for invalid combinations. */
-	for (i = 0; i < sizeof(init->features) * 8; i++) {
-		if (init->features[i / 32] & (1 << (i % 32))) {
-			if (i >= KVM_VCPU_MAX_FEATURES)
-				return -ENOENT;
-			set_bit(i, vcpu->arch.features);
-		}
-	}
-
-	/* Now we know what it is, we can reset it. */
-	return kvm_reset_vcpu(vcpu);
-}
-
 int kvm_vcpu_preferred_target(struct kvm_vcpu_init *init)
 {
 	int target = kvm_target_cpu();
-- 
2.1.2.330.g565301e.dirty

[PATCH v2 4/6] arm/arm64: KVM: Clarify KVM_ARM_VCPU_INIT ABI

[Marc Zyngier]

On 03/12/14 21:18, Christoffer Dall wrote:
> It is not clear that this ioctl can be called multiple times for a given
> vcpu.  Userspace already does this, so clarify the ABI.
> 
> Also specify that userspace is expected to always make secondary and
> subsequent calls to the ioctl with the same parameters for the VCPU as
> the initial call (which userspace also already does).
> 
> Add code to check that userspace doesn't violate that ABI in the future,
> and move the kvm_vcpu_set_target() function which is currently
> duplicated between the 32-bit and 64-bit versions in guest.c to a common
> static function in arm.c, shared between both architectures.
> 
> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> ---
>  Documentation/virtual/kvm/api.txt |  5 +++++
>  arch/arm/include/asm/kvm_host.h   |  2 --
>  arch/arm/kvm/arm.c                | 43 +++++++++++++++++++++++++++++++++++++++
>  arch/arm/kvm/guest.c              | 25 -----------------------
>  arch/arm64/include/asm/kvm_host.h |  2 --
>  arch/arm64/kvm/guest.c            | 25 -----------------------
>  6 files changed, 48 insertions(+), 54 deletions(-)
> 
> diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
> index bb82a90..81f1b97 100644
> --- a/Documentation/virtual/kvm/api.txt
> +++ b/Documentation/virtual/kvm/api.txt
> @@ -2453,6 +2453,11 @@ return ENOEXEC for that vcpu.
>  Note that because some registers reflect machine topology, all vcpus
>  should be created before this ioctl is invoked.
>  
> +Userspace can call this function multiple times for a given vcpu, including
> +after the vcpu has been run. This will reset the vcpu to its initial
> +state. All calls to this function after the initial call must use the same
> +target and same set of feature flags, otherwise EINVAL will be returned.
> +
>  Possible features:
>  	- KVM_ARM_VCPU_POWER_OFF: Starts the CPU in a power-off state.
>  	  Depends on KVM_CAP_ARM_PSCI.  If not set, the CPU will be powered on
> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
> index 53036e2..254e065 100644
> --- a/arch/arm/include/asm/kvm_host.h
> +++ b/arch/arm/include/asm/kvm_host.h
> @@ -150,8 +150,6 @@ struct kvm_vcpu_stat {
>  	u32 halt_wakeup;
>  };
>  
> -int kvm_vcpu_set_target(struct kvm_vcpu *vcpu,
> -			const struct kvm_vcpu_init *init);
>  int kvm_vcpu_preferred_target(struct kvm_vcpu_init *init);
>  unsigned long kvm_arm_num_regs(struct kvm_vcpu *vcpu);
>  int kvm_arm_copy_reg_indices(struct kvm_vcpu *vcpu, u64 __user *indices);
> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
> index 24c9ca4..4043769 100644
> --- a/arch/arm/kvm/arm.c
> +++ b/arch/arm/kvm/arm.c
> @@ -263,6 +263,7 @@ int kvm_arch_vcpu_init(struct kvm_vcpu *vcpu)
>  {
>  	/* Force users to call KVM_ARM_VCPU_INIT */
>  	vcpu->arch.target = -1;
> +	bitmap_zero(vcpu->arch.features, KVM_VCPU_MAX_FEATURES);
>  
>  	/* Set up the timer */
>  	kvm_timer_vcpu_init(vcpu);
> @@ -649,6 +650,48 @@ int kvm_vm_ioctl_irq_line(struct kvm *kvm, struct kvm_irq_level *irq_level,
>  	return -EINVAL;
>  }
>  
> +static int kvm_vcpu_set_target(struct kvm_vcpu *vcpu,
> +			       const struct kvm_vcpu_init *init)
> +{
> +	unsigned int i;
> +	int phys_target = kvm_target_cpu();
> +
> +	if (init->target != phys_target)
> +		return -EINVAL;
> +
> +	/*
> +	 * Secondary and subsequent calls to KVM_ARM_VCPU_INIT must
> +	 * use the same target.
> +	 */
> +	if (vcpu->arch.target != -1 && vcpu->arch.target != init->target)
> +		return -EINVAL;
> +
> +	/* -ENOENT for unknown features, -EINVAL for invalid combinations. */
> +	for (i = 0; i < sizeof(init->features) * 8; i++) {
> +		bool set = (init->features[i / 32] & (1 << (i % 32)));
> +
> +		if (set && i >= KVM_VCPU_MAX_FEATURES)
> +			return -ENOENT;
> +
> +		/*
> +		 * Secondary and subsequent calls to KVM_ARM_VCPU_INIT must
> +		 * use the same feature set.
> +		 */
> +		if (vcpu->arch.target != -1 && i < KVM_VCPU_MAX_FEATURES &&
> +		    test_bit(i, vcpu->arch.features) != set)
> +			return -EINVAL;
> +
> +		if (set)
> +			set_bit(i, vcpu->arch.features);
> +	}
> +
> +	vcpu->arch.target = phys_target;
> +
> +	/* Now we know what it is, we can reset it. */
> +	return kvm_reset_vcpu(vcpu);
> +}
> +
> +
>  static int kvm_arch_vcpu_ioctl_vcpu_init(struct kvm_vcpu *vcpu,
>  					 struct kvm_vcpu_init *init)
>  {
> diff --git a/arch/arm/kvm/guest.c b/arch/arm/kvm/guest.c
> index 8c97208..384bab6 100644
> --- a/arch/arm/kvm/guest.c
> +++ b/arch/arm/kvm/guest.c
> @@ -273,31 +273,6 @@ int __attribute_const__ kvm_target_cpu(void)
>  	}
>  }
>  
> -int kvm_vcpu_set_target(struct kvm_vcpu *vcpu,
> -			const struct kvm_vcpu_init *init)
> -{
> -	unsigned int i;
> -
> -	/* We can only cope with guest==host and only on A15/A7 (for now). */
> -	if (init->target != kvm_target_cpu())
> -		return -EINVAL;
> -
> -	vcpu->arch.target = init->target;
> -	bitmap_zero(vcpu->arch.features, KVM_VCPU_MAX_FEATURES);
> -
> -	/* -ENOENT for unknown features, -EINVAL for invalid combinations. */
> -	for (i = 0; i < sizeof(init->features) * 8; i++) {
> -		if (test_bit(i, (void *)init->features)) {
> -			if (i >= KVM_VCPU_MAX_FEATURES)
> -				return -ENOENT;
> -			set_bit(i, vcpu->arch.features);
> -		}
> -	}
> -
> -	/* Now we know what it is, we can reset it. */
> -	return kvm_reset_vcpu(vcpu);
> -}
> -
>  int kvm_vcpu_preferred_target(struct kvm_vcpu_init *init)
>  {
>  	int target = kvm_target_cpu();
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index 2012c4b..65c6152 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -165,8 +165,6 @@ struct kvm_vcpu_stat {
>  	u32 halt_wakeup;
>  };
>  
> -int kvm_vcpu_set_target(struct kvm_vcpu *vcpu,
> -			const struct kvm_vcpu_init *init);
>  int kvm_vcpu_preferred_target(struct kvm_vcpu_init *init);
>  unsigned long kvm_arm_num_regs(struct kvm_vcpu *vcpu);
>  int kvm_arm_copy_reg_indices(struct kvm_vcpu *vcpu, u64 __user *indices);
> diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c
> index 84d5959..9535bd5 100644
> --- a/arch/arm64/kvm/guest.c
> +++ b/arch/arm64/kvm/guest.c
> @@ -296,31 +296,6 @@ int __attribute_const__ kvm_target_cpu(void)
>  	return -EINVAL;
>  }
>  
> -int kvm_vcpu_set_target(struct kvm_vcpu *vcpu,
> -			const struct kvm_vcpu_init *init)
> -{
> -	unsigned int i;
> -	int phys_target = kvm_target_cpu();
> -
> -	if (init->target != phys_target)
> -		return -EINVAL;
> -
> -	vcpu->arch.target = phys_target;
> -	bitmap_zero(vcpu->arch.features, KVM_VCPU_MAX_FEATURES);
> -
> -	/* -ENOENT for unknown features, -EINVAL for invalid combinations. */
> -	for (i = 0; i < sizeof(init->features) * 8; i++) {
> -		if (init->features[i / 32] & (1 << (i % 32))) {
> -			if (i >= KVM_VCPU_MAX_FEATURES)
> -				return -ENOENT;
> -			set_bit(i, vcpu->arch.features);
> -		}
> -	}
> -
> -	/* Now we know what it is, we can reset it. */
> -	return kvm_reset_vcpu(vcpu);
> -}
> -
>  int kvm_vcpu_preferred_target(struct kvm_vcpu_init *init)
>  {
>  	int target = kvm_target_cpu();
> 

Very nice cleanup.

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

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

[PATCH v2 5/6] arm/arm64: KVM: Turn off vcpus on PSCI shutdown/reboot

[Christoffer Dall]

When a vcpu calls SYSTEM_OFF or SYSTEM_RESET with PSCI v0.2, the vcpus
should really be turned off for the VM adhering to the suggestions in
the PSCI spec, and it's the sane thing to do.

Also, clarify the behavior and expectations for exits to user space with
the KVM_EXIT_SYSTEM_EVENT case.

Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
---
 Documentation/virtual/kvm/api.txt |  9 +++++++++
 arch/arm/kvm/psci.c               | 19 +++++++++++++++++++
 arch/arm64/include/asm/kvm_host.h |  1 +
 3 files changed, 29 insertions(+)

diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
index 81f1b97..228f9cf 100644
--- a/Documentation/virtual/kvm/api.txt
+++ b/Documentation/virtual/kvm/api.txt
@@ -2957,6 +2957,15 @@ 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.
 
+Valid values for 'type' are:
+  KVM_SYSTEM_EVENT_SHUTDOWN -- the guest has requested a shutdown of the
+   VM. Userspace is not obliged to honour this, and if it does honour
+   this does not need to destroy the VM synchronously (ie it may call
+   KVM_RUN again before shutdown finally occurs).
+  KVM_SYSTEM_EVENT_RESET -- the guest has requested a reset of the VM.
+   As with SHUTDOWN, userspace can choose to ignore the request, or
+   to schedule the reset to occur in the future and may call KVM_RUN again.
+
 		/* Fix the size of the union. */
 		char padding[256];
 	};
diff --git a/arch/arm/kvm/psci.c b/arch/arm/kvm/psci.c
index 09cf377..ae0bb91 100644
--- a/arch/arm/kvm/psci.c
+++ b/arch/arm/kvm/psci.c
@@ -15,6 +15,7 @@
  * along with this program.  If not, see <http://www.gnu.org/licenses/>.
  */
 
+#include <linux/preempt.h>
 #include <linux/kvm_host.h>
 #include <linux/wait.h>
 
@@ -166,6 +167,24 @@ static unsigned long kvm_psci_vcpu_affinity_info(struct kvm_vcpu *vcpu)
 
 static void kvm_prepare_system_event(struct kvm_vcpu *vcpu, u32 type)
 {
+	int i;
+	struct kvm_vcpu *tmp;
+
+	/*
+	 * The KVM ABI specifies that a system event exit may call KVM_RUN
+	 * again and may perform shutdown/reboot at a later time that when the
+	 * actual request is made.  Since we are implementing PSCI and a
+	 * caller of PSCI reboot and shutdown expects that the system shuts
+	 * down or reboots immediately, let's make sure that VCPUs are not run
+	 * after this call is handled and before the VCPUs have been
+	 * re-initialized.
+	 */
+	kvm_for_each_vcpu(i, tmp, vcpu->kvm)
+		tmp->arch.pause = true;
+	preempt_disable();
+	force_vm_exit(cpu_all_mask);
+	preempt_enable();
+
 	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;
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 65c6152..0b7dfdb 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -198,6 +198,7 @@ struct kvm_vcpu *kvm_arm_get_running_vcpu(void);
 struct kvm_vcpu * __percpu *kvm_get_running_vcpus(void);
 
 u64 kvm_call_hyp(void *hypfn, ...);
+void force_vm_exit(const cpumask_t *mask);
 
 int handle_exit(struct kvm_vcpu *vcpu, struct kvm_run *run,
 		int exception_index);
-- 
2.1.2.330.g565301e.dirty

[PATCH v2 5/6] arm/arm64: KVM: Turn off vcpus on PSCI shutdown/reboot

[Marc Zyngier]

On 03/12/14 21:18, Christoffer Dall wrote:
> When a vcpu calls SYSTEM_OFF or SYSTEM_RESET with PSCI v0.2, the vcpus
> should really be turned off for the VM adhering to the suggestions in
> the PSCI spec, and it's the sane thing to do.
> 
> Also, clarify the behavior and expectations for exits to user space with
> the KVM_EXIT_SYSTEM_EVENT case.
> 
> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> ---
>  Documentation/virtual/kvm/api.txt |  9 +++++++++
>  arch/arm/kvm/psci.c               | 19 +++++++++++++++++++
>  arch/arm64/include/asm/kvm_host.h |  1 +
>  3 files changed, 29 insertions(+)
> 
> diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
> index 81f1b97..228f9cf 100644
> --- a/Documentation/virtual/kvm/api.txt
> +++ b/Documentation/virtual/kvm/api.txt
> @@ -2957,6 +2957,15 @@ 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.
>  
> +Valid values for 'type' are:
> +  KVM_SYSTEM_EVENT_SHUTDOWN -- the guest has requested a shutdown of the
> +   VM. Userspace is not obliged to honour this, and if it does honour
> +   this does not need to destroy the VM synchronously (ie it may call
> +   KVM_RUN again before shutdown finally occurs).
> +  KVM_SYSTEM_EVENT_RESET -- the guest has requested a reset of the VM.
> +   As with SHUTDOWN, userspace can choose to ignore the request, or
> +   to schedule the reset to occur in the future and may call KVM_RUN again.
> +
>  		/* Fix the size of the union. */
>  		char padding[256];
>  	};
> diff --git a/arch/arm/kvm/psci.c b/arch/arm/kvm/psci.c
> index 09cf377..ae0bb91 100644
> --- a/arch/arm/kvm/psci.c
> +++ b/arch/arm/kvm/psci.c
> @@ -15,6 +15,7 @@
>   * along with this program.  If not, see <http://www.gnu.org/licenses/>.
>   */
>  
> +#include <linux/preempt.h>
>  #include <linux/kvm_host.h>
>  #include <linux/wait.h>
>  
> @@ -166,6 +167,24 @@ static unsigned long kvm_psci_vcpu_affinity_info(struct kvm_vcpu *vcpu)
>  
>  static void kvm_prepare_system_event(struct kvm_vcpu *vcpu, u32 type)
>  {
> +	int i;
> +	struct kvm_vcpu *tmp;
> +
> +	/*
> +	 * The KVM ABI specifies that a system event exit may call KVM_RUN
> +	 * again and may perform shutdown/reboot at a later time that when the
> +	 * actual request is made.  Since we are implementing PSCI and a
> +	 * caller of PSCI reboot and shutdown expects that the system shuts
> +	 * down or reboots immediately, let's make sure that VCPUs are not run
> +	 * after this call is handled and before the VCPUs have been
> +	 * re-initialized.
> +	 */
> +	kvm_for_each_vcpu(i, tmp, vcpu->kvm)
> +		tmp->arch.pause = true;
> +	preempt_disable();
> +	force_vm_exit(cpu_all_mask);
> +	preempt_enable();
> +

I'm slightly uneasy about this force_vm_exit, as this is something that
is directly triggered by the guest. I suppose it is almost impossible to
find out which CPUs we're actually using...

>  	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;
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index 65c6152..0b7dfdb 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -198,6 +198,7 @@ struct kvm_vcpu *kvm_arm_get_running_vcpu(void);
>  struct kvm_vcpu * __percpu *kvm_get_running_vcpus(void);
>  
>  u64 kvm_call_hyp(void *hypfn, ...);
> +void force_vm_exit(const cpumask_t *mask);
>  
>  int handle_exit(struct kvm_vcpu *vcpu, struct kvm_run *run,
>  		int exception_index);
> 

Other than that,

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

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

[PATCH v2 5/6] arm/arm64: KVM: Turn off vcpus on PSCI shutdown/reboot

[Christoffer Dall]

On Mon, Dec 08, 2014 at 12:04:53PM +0000, Marc Zyngier wrote:
> On 03/12/14 21:18, Christoffer Dall wrote:
> > When a vcpu calls SYSTEM_OFF or SYSTEM_RESET with PSCI v0.2, the vcpus
> > should really be turned off for the VM adhering to the suggestions in
> > the PSCI spec, and it's the sane thing to do.
> > 
> > Also, clarify the behavior and expectations for exits to user space with
> > the KVM_EXIT_SYSTEM_EVENT case.
> > 
> > Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> > ---
> >  Documentation/virtual/kvm/api.txt |  9 +++++++++
> >  arch/arm/kvm/psci.c               | 19 +++++++++++++++++++
> >  arch/arm64/include/asm/kvm_host.h |  1 +
> >  3 files changed, 29 insertions(+)
> > 
> > diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
> > index 81f1b97..228f9cf 100644
> > --- a/Documentation/virtual/kvm/api.txt
> > +++ b/Documentation/virtual/kvm/api.txt
> > @@ -2957,6 +2957,15 @@ 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.
> >  
> > +Valid values for 'type' are:
> > +  KVM_SYSTEM_EVENT_SHUTDOWN -- the guest has requested a shutdown of the
> > +   VM. Userspace is not obliged to honour this, and if it does honour
> > +   this does not need to destroy the VM synchronously (ie it may call
> > +   KVM_RUN again before shutdown finally occurs).
> > +  KVM_SYSTEM_EVENT_RESET -- the guest has requested a reset of the VM.
> > +   As with SHUTDOWN, userspace can choose to ignore the request, or
> > +   to schedule the reset to occur in the future and may call KVM_RUN again.
> > +
> >  		/* Fix the size of the union. */
> >  		char padding[256];
> >  	};
> > diff --git a/arch/arm/kvm/psci.c b/arch/arm/kvm/psci.c
> > index 09cf377..ae0bb91 100644
> > --- a/arch/arm/kvm/psci.c
> > +++ b/arch/arm/kvm/psci.c
> > @@ -15,6 +15,7 @@
> >   * along with this program.  If not, see <http://www.gnu.org/licenses/>.
> >   */
> >  
> > +#include <linux/preempt.h>
> >  #include <linux/kvm_host.h>
> >  #include <linux/wait.h>
> >  
> > @@ -166,6 +167,24 @@ static unsigned long kvm_psci_vcpu_affinity_info(struct kvm_vcpu *vcpu)
> >  
> >  static void kvm_prepare_system_event(struct kvm_vcpu *vcpu, u32 type)
> >  {
> > +	int i;
> > +	struct kvm_vcpu *tmp;
> > +
> > +	/*
> > +	 * The KVM ABI specifies that a system event exit may call KVM_RUN
> > +	 * again and may perform shutdown/reboot at a later time that when the
> > +	 * actual request is made.  Since we are implementing PSCI and a
> > +	 * caller of PSCI reboot and shutdown expects that the system shuts
> > +	 * down or reboots immediately, let's make sure that VCPUs are not run
> > +	 * after this call is handled and before the VCPUs have been
> > +	 * re-initialized.
> > +	 */
> > +	kvm_for_each_vcpu(i, tmp, vcpu->kvm)
> > +		tmp->arch.pause = true;
> > +	preempt_disable();
> > +	force_vm_exit(cpu_all_mask);
> > +	preempt_enable();
> > +
> 
> I'm slightly uneasy about this force_vm_exit, as this is something that
> is directly triggered by the guest. I suppose it is almost impossible to
> find out which CPUs we're actually using...
> 
Ah, you mean we should only IPI the CPUs that are actually running a
VCPU belonging to this VM?

I guess I could replace it with:

	kvm_for_each_vcpu(i, tmp, vcpu->kvm) {
		tmp->arch.pause = true;
		kvm_vcpu_kick(tmp);
	}

or a slightly more optimized "half-open-coded-kvm_vcpu_kick":

	me = get_cpu();
	kvm_for_each_vcpu(i, tmp, vcpu->kvm) {
		tmp->arch.pause = true;
		if (tmp->cpu != me && (unsigned)tmp->cpu < nr_cpu_ids &&
		    cpu_online(tmp->cpu)  && kvm_arch_vcpu_should_kick(tmp))
			smp_send_reschedule(tmp->cpu);
	}

which should save us waking up vcpu threads that are parked on
waitqueues.  Not sure it's worth it, maybe it is for 100s of vcpu
systems?

Can we actually replace force_vm_exit() with the more optimized
open-coded version?  That messes with VMID allocation so it really needs
a lot of testing though...

Preferences?

-Christoffer

[PATCH v2 5/6] arm/arm64: KVM: Turn off vcpus on PSCI shutdown/reboot

[Marc Zyngier]

On 08/12/14 12:58, Christoffer Dall wrote:
> On Mon, Dec 08, 2014 at 12:04:53PM +0000, Marc Zyngier wrote:
>> On 03/12/14 21:18, Christoffer Dall wrote:
>>> When a vcpu calls SYSTEM_OFF or SYSTEM_RESET with PSCI v0.2, the vcpus
>>> should really be turned off for the VM adhering to the suggestions in
>>> the PSCI spec, and it's the sane thing to do.
>>>
>>> Also, clarify the behavior and expectations for exits to user space with
>>> the KVM_EXIT_SYSTEM_EVENT case.
>>>
>>> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
>>> ---
>>>  Documentation/virtual/kvm/api.txt |  9 +++++++++
>>>  arch/arm/kvm/psci.c               | 19 +++++++++++++++++++
>>>  arch/arm64/include/asm/kvm_host.h |  1 +
>>>  3 files changed, 29 insertions(+)
>>>
>>> diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
>>> index 81f1b97..228f9cf 100644
>>> --- a/Documentation/virtual/kvm/api.txt
>>> +++ b/Documentation/virtual/kvm/api.txt
>>> @@ -2957,6 +2957,15 @@ 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.
>>>  
>>> +Valid values for 'type' are:
>>> +  KVM_SYSTEM_EVENT_SHUTDOWN -- the guest has requested a shutdown of the
>>> +   VM. Userspace is not obliged to honour this, and if it does honour
>>> +   this does not need to destroy the VM synchronously (ie it may call
>>> +   KVM_RUN again before shutdown finally occurs).
>>> +  KVM_SYSTEM_EVENT_RESET -- the guest has requested a reset of the VM.
>>> +   As with SHUTDOWN, userspace can choose to ignore the request, or
>>> +   to schedule the reset to occur in the future and may call KVM_RUN again.
>>> +
>>>  		/* Fix the size of the union. */
>>>  		char padding[256];
>>>  	};
>>> diff --git a/arch/arm/kvm/psci.c b/arch/arm/kvm/psci.c
>>> index 09cf377..ae0bb91 100644
>>> --- a/arch/arm/kvm/psci.c
>>> +++ b/arch/arm/kvm/psci.c
>>> @@ -15,6 +15,7 @@
>>>   * along with this program.  If not, see <http://www.gnu.org/licenses/>.
>>>   */
>>>  
>>> +#include <linux/preempt.h>
>>>  #include <linux/kvm_host.h>
>>>  #include <linux/wait.h>
>>>  
>>> @@ -166,6 +167,24 @@ static unsigned long kvm_psci_vcpu_affinity_info(struct kvm_vcpu *vcpu)
>>>  
>>>  static void kvm_prepare_system_event(struct kvm_vcpu *vcpu, u32 type)
>>>  {
>>> +	int i;
>>> +	struct kvm_vcpu *tmp;
>>> +
>>> +	/*
>>> +	 * The KVM ABI specifies that a system event exit may call KVM_RUN
>>> +	 * again and may perform shutdown/reboot at a later time that when the
>>> +	 * actual request is made.  Since we are implementing PSCI and a
>>> +	 * caller of PSCI reboot and shutdown expects that the system shuts
>>> +	 * down or reboots immediately, let's make sure that VCPUs are not run
>>> +	 * after this call is handled and before the VCPUs have been
>>> +	 * re-initialized.
>>> +	 */
>>> +	kvm_for_each_vcpu(i, tmp, vcpu->kvm)
>>> +		tmp->arch.pause = true;
>>> +	preempt_disable();
>>> +	force_vm_exit(cpu_all_mask);
>>> +	preempt_enable();
>>> +
>>
>> I'm slightly uneasy about this force_vm_exit, as this is something that
>> is directly triggered by the guest. I suppose it is almost impossible to
>> find out which CPUs we're actually using...
>>
> Ah, you mean we should only IPI the CPUs that are actually running a
> VCPU belonging to this VM?
> 
> I guess I could replace it with:
> 
> 	kvm_for_each_vcpu(i, tmp, vcpu->kvm) {
> 		tmp->arch.pause = true;
> 		kvm_vcpu_kick(tmp);
> 	}

Ah, that's even simpler than I thought. Yeah, looks good to me.

> 
> or a slightly more optimized "half-open-coded-kvm_vcpu_kick":
> 
> 	me = get_cpu();
> 	kvm_for_each_vcpu(i, tmp, vcpu->kvm) {
> 		tmp->arch.pause = true;
> 		if (tmp->cpu != me && (unsigned)tmp->cpu < nr_cpu_ids &&
> 		    cpu_online(tmp->cpu)  && kvm_arch_vcpu_should_kick(tmp))
> 			smp_send_reschedule(tmp->cpu);
> 	}
> 
> which should save us waking up vcpu threads that are parked on
> waitqueues.  Not sure it's worth it, maybe it is for 100s of vcpu
> systems?

Probably not worth it at the moment.

> Can we actually replace force_vm_exit() with the more optimized
> open-coded version?  That messes with VMID allocation so it really needs
> a lot of testing though...

VMID reallocation almost never occurs, and that's a system-wide event,
not triggered by a guest. I'd rather not mess with that just yet.

> Preferences?

I think your first version is very nice, provided that it doesn't
introduce any unforeseen regression.

Thanks,

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

[PATCH v2 5/6] arm/arm64: KVM: Turn off vcpus on PSCI shutdown/reboot

[Christoffer Dall]

On Mon, Dec 08, 2014 at 01:19:15PM +0000, Marc Zyngier wrote:
> On 08/12/14 12:58, Christoffer Dall wrote:
> > On Mon, Dec 08, 2014 at 12:04:53PM +0000, Marc Zyngier wrote:
> >> On 03/12/14 21:18, Christoffer Dall wrote:
> >>> When a vcpu calls SYSTEM_OFF or SYSTEM_RESET with PSCI v0.2, the vcpus
> >>> should really be turned off for the VM adhering to the suggestions in
> >>> the PSCI spec, and it's the sane thing to do.
> >>>
> >>> Also, clarify the behavior and expectations for exits to user space with
> >>> the KVM_EXIT_SYSTEM_EVENT case.
> >>>
> >>> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> >>> ---
> >>>  Documentation/virtual/kvm/api.txt |  9 +++++++++
> >>>  arch/arm/kvm/psci.c               | 19 +++++++++++++++++++
> >>>  arch/arm64/include/asm/kvm_host.h |  1 +
> >>>  3 files changed, 29 insertions(+)
> >>>
> >>> diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
> >>> index 81f1b97..228f9cf 100644
> >>> --- a/Documentation/virtual/kvm/api.txt
> >>> +++ b/Documentation/virtual/kvm/api.txt
> >>> @@ -2957,6 +2957,15 @@ 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.
> >>>  
> >>> +Valid values for 'type' are:
> >>> +  KVM_SYSTEM_EVENT_SHUTDOWN -- the guest has requested a shutdown of the
> >>> +   VM. Userspace is not obliged to honour this, and if it does honour
> >>> +   this does not need to destroy the VM synchronously (ie it may call
> >>> +   KVM_RUN again before shutdown finally occurs).
> >>> +  KVM_SYSTEM_EVENT_RESET -- the guest has requested a reset of the VM.
> >>> +   As with SHUTDOWN, userspace can choose to ignore the request, or
> >>> +   to schedule the reset to occur in the future and may call KVM_RUN again.
> >>> +
> >>>  		/* Fix the size of the union. */
> >>>  		char padding[256];
> >>>  	};
> >>> diff --git a/arch/arm/kvm/psci.c b/arch/arm/kvm/psci.c
> >>> index 09cf377..ae0bb91 100644
> >>> --- a/arch/arm/kvm/psci.c
> >>> +++ b/arch/arm/kvm/psci.c
> >>> @@ -15,6 +15,7 @@
> >>>   * along with this program.  If not, see <http://www.gnu.org/licenses/>.
> >>>   */
> >>>  
> >>> +#include <linux/preempt.h>
> >>>  #include <linux/kvm_host.h>
> >>>  #include <linux/wait.h>
> >>>  
> >>> @@ -166,6 +167,24 @@ static unsigned long kvm_psci_vcpu_affinity_info(struct kvm_vcpu *vcpu)
> >>>  
> >>>  static void kvm_prepare_system_event(struct kvm_vcpu *vcpu, u32 type)
> >>>  {
> >>> +	int i;
> >>> +	struct kvm_vcpu *tmp;
> >>> +
> >>> +	/*
> >>> +	 * The KVM ABI specifies that a system event exit may call KVM_RUN
> >>> +	 * again and may perform shutdown/reboot at a later time that when the
> >>> +	 * actual request is made.  Since we are implementing PSCI and a
> >>> +	 * caller of PSCI reboot and shutdown expects that the system shuts
> >>> +	 * down or reboots immediately, let's make sure that VCPUs are not run
> >>> +	 * after this call is handled and before the VCPUs have been
> >>> +	 * re-initialized.
> >>> +	 */
> >>> +	kvm_for_each_vcpu(i, tmp, vcpu->kvm)
> >>> +		tmp->arch.pause = true;
> >>> +	preempt_disable();
> >>> +	force_vm_exit(cpu_all_mask);
> >>> +	preempt_enable();
> >>> +
> >>
> >> I'm slightly uneasy about this force_vm_exit, as this is something that
> >> is directly triggered by the guest. I suppose it is almost impossible to
> >> find out which CPUs we're actually using...
> >>
> > Ah, you mean we should only IPI the CPUs that are actually running a
> > VCPU belonging to this VM?
> > 
> > I guess I could replace it with:
> > 
> > 	kvm_for_each_vcpu(i, tmp, vcpu->kvm) {
> > 		tmp->arch.pause = true;
> > 		kvm_vcpu_kick(tmp);
> > 	}
> 
> Ah, that's even simpler than I thought. Yeah, looks good to me.
> 
> > 
> > or a slightly more optimized "half-open-coded-kvm_vcpu_kick":
> > 
> > 	me = get_cpu();
> > 	kvm_for_each_vcpu(i, tmp, vcpu->kvm) {
> > 		tmp->arch.pause = true;
> > 		if (tmp->cpu != me && (unsigned)tmp->cpu < nr_cpu_ids &&
> > 		    cpu_online(tmp->cpu)  && kvm_arch_vcpu_should_kick(tmp))
> > 			smp_send_reschedule(tmp->cpu);
> > 	}
> > 
> > which should save us waking up vcpu threads that are parked on
> > waitqueues.  Not sure it's worth it, maybe it is for 100s of vcpu
> > systems?
> 
> Probably not worth it at the moment.
> 
> > Can we actually replace force_vm_exit() with the more optimized
> > open-coded version?  That messes with VMID allocation so it really needs
> > a lot of testing though...
> 
> VMID reallocation almost never occurs, and that's a system-wide event,
> not triggered by a guest. I'd rather not mess with that just yet.
> 
> > Preferences?
> 
> I think your first version is very nice, provided that it doesn't
> introduce any unforeseen regression.
> 

ok, will respin with option #1.

Thanks,
-Christoffer

[PATCH v2 5/6] arm/arm64: KVM: Turn off vcpus on PSCI shutdown/reboot

[Christoffer Dall]

On Mon, Dec 08, 2014 at 01:19:15PM +0000, Marc Zyngier wrote:
> On 08/12/14 12:58, Christoffer Dall wrote:
> > On Mon, Dec 08, 2014 at 12:04:53PM +0000, Marc Zyngier wrote:
> >> On 03/12/14 21:18, Christoffer Dall wrote:
> >>> When a vcpu calls SYSTEM_OFF or SYSTEM_RESET with PSCI v0.2, the vcpus
> >>> should really be turned off for the VM adhering to the suggestions in
> >>> the PSCI spec, and it's the sane thing to do.
> >>>
> >>> Also, clarify the behavior and expectations for exits to user space with
> >>> the KVM_EXIT_SYSTEM_EVENT case.
> >>>
> >>> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> >>> ---
> >>>  Documentation/virtual/kvm/api.txt |  9 +++++++++
> >>>  arch/arm/kvm/psci.c               | 19 +++++++++++++++++++
> >>>  arch/arm64/include/asm/kvm_host.h |  1 +
> >>>  3 files changed, 29 insertions(+)
> >>>
> >>> diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
> >>> index 81f1b97..228f9cf 100644
> >>> --- a/Documentation/virtual/kvm/api.txt
> >>> +++ b/Documentation/virtual/kvm/api.txt
> >>> @@ -2957,6 +2957,15 @@ 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.
> >>>  
> >>> +Valid values for 'type' are:
> >>> +  KVM_SYSTEM_EVENT_SHUTDOWN -- the guest has requested a shutdown of the
> >>> +   VM. Userspace is not obliged to honour this, and if it does honour
> >>> +   this does not need to destroy the VM synchronously (ie it may call
> >>> +   KVM_RUN again before shutdown finally occurs).
> >>> +  KVM_SYSTEM_EVENT_RESET -- the guest has requested a reset of the VM.
> >>> +   As with SHUTDOWN, userspace can choose to ignore the request, or
> >>> +   to schedule the reset to occur in the future and may call KVM_RUN again.
> >>> +
> >>>  		/* Fix the size of the union. */
> >>>  		char padding[256];
> >>>  	};
> >>> diff --git a/arch/arm/kvm/psci.c b/arch/arm/kvm/psci.c
> >>> index 09cf377..ae0bb91 100644
> >>> --- a/arch/arm/kvm/psci.c
> >>> +++ b/arch/arm/kvm/psci.c
> >>> @@ -15,6 +15,7 @@
> >>>   * along with this program.  If not, see <http://www.gnu.org/licenses/>.
> >>>   */
> >>>  
> >>> +#include <linux/preempt.h>
> >>>  #include <linux/kvm_host.h>
> >>>  #include <linux/wait.h>
> >>>  
> >>> @@ -166,6 +167,24 @@ static unsigned long kvm_psci_vcpu_affinity_info(struct kvm_vcpu *vcpu)
> >>>  
> >>>  static void kvm_prepare_system_event(struct kvm_vcpu *vcpu, u32 type)
> >>>  {
> >>> +	int i;
> >>> +	struct kvm_vcpu *tmp;
> >>> +
> >>> +	/*
> >>> +	 * The KVM ABI specifies that a system event exit may call KVM_RUN
> >>> +	 * again and may perform shutdown/reboot at a later time that when the
> >>> +	 * actual request is made.  Since we are implementing PSCI and a
> >>> +	 * caller of PSCI reboot and shutdown expects that the system shuts
> >>> +	 * down or reboots immediately, let's make sure that VCPUs are not run
> >>> +	 * after this call is handled and before the VCPUs have been
> >>> +	 * re-initialized.
> >>> +	 */
> >>> +	kvm_for_each_vcpu(i, tmp, vcpu->kvm)
> >>> +		tmp->arch.pause = true;
> >>> +	preempt_disable();
> >>> +	force_vm_exit(cpu_all_mask);
> >>> +	preempt_enable();
> >>> +
> >>
> >> I'm slightly uneasy about this force_vm_exit, as this is something that
> >> is directly triggered by the guest. I suppose it is almost impossible to
> >> find out which CPUs we're actually using...
> >>
> > Ah, you mean we should only IPI the CPUs that are actually running a
> > VCPU belonging to this VM?
> > 
> > I guess I could replace it with:
> > 
> > 	kvm_for_each_vcpu(i, tmp, vcpu->kvm) {
> > 		tmp->arch.pause = true;
> > 		kvm_vcpu_kick(tmp);
> > 	}
> 
> Ah, that's even simpler than I thought. Yeah, looks good to me.
> 
Can I take this as an ack and apply this series?

-Christoffer

[PATCH v2 5/6] arm/arm64: KVM: Turn off vcpus on PSCI shutdown/reboot

[Marc Zyngier]

On 2014-12-12 19:49, Christoffer Dall wrote:
> On Mon, Dec 08, 2014 at 01:19:15PM +0000, Marc Zyngier wrote:
>> On 08/12/14 12:58, Christoffer Dall wrote:
>> > On Mon, Dec 08, 2014 at 12:04:53PM +0000, Marc Zyngier wrote:
>> >> On 03/12/14 21:18, Christoffer Dall wrote:
>> >>> When a vcpu calls SYSTEM_OFF or SYSTEM_RESET with PSCI v0.2, the 
>> vcpus
>> >>> should really be turned off for the VM adhering to the 
>> suggestions in
>> >>> the PSCI spec, and it's the sane thing to do.
>> >>>
>> >>> Also, clarify the behavior and expectations for exits to user 
>> space with
>> >>> the KVM_EXIT_SYSTEM_EVENT case.
>> >>>
>> >>> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
>> >>> ---
>> >>>  Documentation/virtual/kvm/api.txt |  9 +++++++++
>> >>>  arch/arm/kvm/psci.c               | 19 +++++++++++++++++++
>> >>>  arch/arm64/include/asm/kvm_host.h |  1 +
>> >>>  3 files changed, 29 insertions(+)
>> >>>
>> >>> diff --git a/Documentation/virtual/kvm/api.txt 
>> b/Documentation/virtual/kvm/api.txt
>> >>> index 81f1b97..228f9cf 100644
>> >>> --- a/Documentation/virtual/kvm/api.txt
>> >>> +++ b/Documentation/virtual/kvm/api.txt
>> >>> @@ -2957,6 +2957,15 @@ 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.
>> >>>
>> >>> +Valid values for 'type' are:
>> >>> +  KVM_SYSTEM_EVENT_SHUTDOWN -- the guest has requested a 
>> shutdown of the
>> >>> +   VM. Userspace is not obliged to honour this, and if it does 
>> honour
>> >>> +   this does not need to destroy the VM synchronously (ie it 
>> may call
>> >>> +   KVM_RUN again before shutdown finally occurs).
>> >>> +  KVM_SYSTEM_EVENT_RESET -- the guest has requested a reset of 
>> the VM.
>> >>> +   As with SHUTDOWN, userspace can choose to ignore the 
>> request, or
>> >>> +   to schedule the reset to occur in the future and may call 
>> KVM_RUN again.
>> >>> +
>> >>>  		/* Fix the size of the union. */
>> >>>  		char padding[256];
>> >>>  	};
>> >>> diff --git a/arch/arm/kvm/psci.c b/arch/arm/kvm/psci.c
>> >>> index 09cf377..ae0bb91 100644
>> >>> --- a/arch/arm/kvm/psci.c
>> >>> +++ b/arch/arm/kvm/psci.c
>> >>> @@ -15,6 +15,7 @@
>> >>>   * along with this program.  If not, see 
>> <http://www.gnu.org/licenses/>.
>> >>>   */
>> >>>
>> >>> +#include <linux/preempt.h>
>> >>>  #include <linux/kvm_host.h>
>> >>>  #include <linux/wait.h>
>> >>>
>> >>> @@ -166,6 +167,24 @@ static unsigned long 
>> kvm_psci_vcpu_affinity_info(struct kvm_vcpu *vcpu)
>> >>>
>> >>>  static void kvm_prepare_system_event(struct kvm_vcpu *vcpu, u32 
>> type)
>> >>>  {
>> >>> +	int i;
>> >>> +	struct kvm_vcpu *tmp;
>> >>> +
>> >>> +	/*
>> >>> +	 * The KVM ABI specifies that a system event exit may call 
>> KVM_RUN
>> >>> +	 * again and may perform shutdown/reboot at a later time that 
>> when the
>> >>> +	 * actual request is made.  Since we are implementing PSCI and 
>> a
>> >>> +	 * caller of PSCI reboot and shutdown expects that the system 
>> shuts
>> >>> +	 * down or reboots immediately, let's make sure that VCPUs are 
>> not run
>> >>> +	 * after this call is handled and before the VCPUs have been
>> >>> +	 * re-initialized.
>> >>> +	 */
>> >>> +	kvm_for_each_vcpu(i, tmp, vcpu->kvm)
>> >>> +		tmp->arch.pause = true;
>> >>> +	preempt_disable();
>> >>> +	force_vm_exit(cpu_all_mask);
>> >>> +	preempt_enable();
>> >>> +
>> >>
>> >> I'm slightly uneasy about this force_vm_exit, as this is 
>> something that
>> >> is directly triggered by the guest. I suppose it is almost 
>> impossible to
>> >> find out which CPUs we're actually using...
>> >>
>> > Ah, you mean we should only IPI the CPUs that are actually running 
>> a
>> > VCPU belonging to this VM?
>> >
>> > I guess I could replace it with:
>> >
>> > 	kvm_for_each_vcpu(i, tmp, vcpu->kvm) {
>> > 		tmp->arch.pause = true;
>> > 		kvm_vcpu_kick(tmp);
>> > 	}
>>
>> Ah, that's even simpler than I thought. Yeah, looks good to me.
>>
> Can I take this as an ack and apply this series?

Yes, please go ahead.

         M.
-- 
Fast, cheap, reliable. Pick two.

[PATCH v2 6/6] arm/arm64: KVM: Introduce stage2_unmap_vm

[Christoffer Dall]

Introduce a new function to unmap user RAM regions in the stage2 page
tables.  This is needed on reboot (or when the guest turns off the MMU)
to ensure we fault in pages again and make the dcache, RAM, and icache
coherent.

Using unmap_stage2_range for the whole guest physical range does not
work, because that unmaps IO regions (such as the GIC) which will not be
recreated or in the best case faulted in on a page-by-page basis.

Call this function on secondary and subsequent calls to the
KVM_ARM_VCPU_INIT ioctl so that a reset VCPU will detect the guest
Stage-1 MMU is off when faulting in pages and make the caches coherent.

Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
---
 arch/arm/include/asm/kvm_mmu.h   |  1 +
 arch/arm/kvm/arm.c               |  7 +++++
 arch/arm/kvm/mmu.c               | 65 ++++++++++++++++++++++++++++++++++++++++
 arch/arm64/include/asm/kvm_mmu.h |  1 +
 4 files changed, 74 insertions(+)

diff --git a/arch/arm/include/asm/kvm_mmu.h b/arch/arm/include/asm/kvm_mmu.h
index acb0d57..4654c42 100644
--- a/arch/arm/include/asm/kvm_mmu.h
+++ b/arch/arm/include/asm/kvm_mmu.h
@@ -52,6 +52,7 @@ int create_hyp_io_mappings(void *from, void *to, phys_addr_t);
 void free_boot_hyp_pgd(void);
 void free_hyp_pgds(void);
 
+void stage2_unmap_vm(struct kvm *kvm);
 int kvm_alloc_stage2_pgd(struct kvm *kvm);
 void kvm_free_stage2_pgd(struct kvm *kvm);
 int kvm_phys_addr_ioremap(struct kvm *kvm, phys_addr_t guest_ipa,
diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
index 4043769..da87c07 100644
--- a/arch/arm/kvm/arm.c
+++ b/arch/arm/kvm/arm.c
@@ -701,6 +701,13 @@ static int kvm_arch_vcpu_ioctl_vcpu_init(struct kvm_vcpu *vcpu,
 	if (ret)
 		return ret;
 
+	/*
+	 * Ensure a rebooted VM will fault in RAM pages and detect if the
+	 * guest MMU is turned off and flush the caches as needed.
+	 */
+	if (vcpu->arch.has_run_once)
+		stage2_unmap_vm(vcpu->kvm);
+
 	vcpu_reset_hcr(vcpu);
 
 	/*
diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
index 57a403a..b1f3c9a 100644
--- a/arch/arm/kvm/mmu.c
+++ b/arch/arm/kvm/mmu.c
@@ -611,6 +611,71 @@ static void unmap_stage2_range(struct kvm *kvm, phys_addr_t start, u64 size)
 	unmap_range(kvm, kvm->arch.pgd, start, size);
 }
 
+static void stage2_unmap_memslot(struct kvm *kvm,
+				 struct kvm_memory_slot *memslot)
+{
+	hva_t hva = memslot->userspace_addr;
+	phys_addr_t addr = memslot->base_gfn << PAGE_SHIFT;
+	phys_addr_t size = PAGE_SIZE * memslot->npages;
+	hva_t reg_end = hva + size;
+
+	/*
+	 * A memory region could potentially cover multiple VMAs, and any holes
+	 * between them, so iterate over all of them to find out if we should
+	 * unmap any of them.
+	 *
+	 *     +--------------------------------------------+
+	 * +---------------+----------------+   +----------------+
+	 * |   : VMA 1     |      VMA 2     |   |    VMA 3  :    |
+	 * +---------------+----------------+   +----------------+
+	 *     |               memory region                |
+	 *     +--------------------------------------------+
+	 */
+	do {
+		struct vm_area_struct *vma = find_vma(current->mm, hva);
+		hva_t vm_start, vm_end;
+
+		if (!vma || vma->vm_start >= reg_end)
+			break;
+
+		/*
+		 * Take the intersection of this VMA with the memory region
+		 */
+		vm_start = max(hva, vma->vm_start);
+		vm_end = min(reg_end, vma->vm_end);
+
+		if (!(vma->vm_flags & VM_PFNMAP)) {
+			gpa_t gpa = addr + (vm_start - memslot->userspace_addr);
+			unmap_stage2_range(kvm, gpa, vm_end - vm_start);
+		}
+		hva = vm_end;
+	} while (hva < reg_end);
+}
+
+/**
+ * stage2_unmap_vm - Unmap Stage-2 RAM mappings
+ * @kvm: The struct kvm pointer
+ *
+ * Go through the memregions and unmap any reguler RAM
+ * backing memory already mapped to the VM.
+ */
+void stage2_unmap_vm(struct kvm *kvm)
+{
+	struct kvm_memslots *slots;
+	struct kvm_memory_slot *memslot;
+	int idx;
+
+	idx = srcu_read_lock(&kvm->srcu);
+	spin_lock(&kvm->mmu_lock);
+
+	slots = kvm_memslots(kvm);
+	kvm_for_each_memslot(memslot, slots)
+		stage2_unmap_memslot(kvm, memslot);
+
+	spin_unlock(&kvm->mmu_lock);
+	srcu_read_unlock(&kvm->srcu, idx);
+}
+
 /**
  * kvm_free_stage2_pgd - free all stage-2 tables
  * @kvm:	The KVM struct pointer for the VM.
diff --git a/arch/arm64/include/asm/kvm_mmu.h b/arch/arm64/include/asm/kvm_mmu.h
index 0caf7a5..061fed7 100644
--- a/arch/arm64/include/asm/kvm_mmu.h
+++ b/arch/arm64/include/asm/kvm_mmu.h
@@ -83,6 +83,7 @@ int create_hyp_io_mappings(void *from, void *to, phys_addr_t);
 void free_boot_hyp_pgd(void);
 void free_hyp_pgds(void);
 
+void stage2_unmap_vm(struct kvm *kvm);
 int kvm_alloc_stage2_pgd(struct kvm *kvm);
 void kvm_free_stage2_pgd(struct kvm *kvm);
 int kvm_phys_addr_ioremap(struct kvm *kvm, phys_addr_t guest_ipa,
-- 
2.1.2.330.g565301e.dirty

[PATCH v2 6/6] arm/arm64: KVM: Introduce stage2_unmap_vm

[Marc Zyngier]

On 03/12/14 21:18, Christoffer Dall wrote:
> Introduce a new function to unmap user RAM regions in the stage2 page
> tables.  This is needed on reboot (or when the guest turns off the MMU)
> to ensure we fault in pages again and make the dcache, RAM, and icache
> coherent.
> 
> Using unmap_stage2_range for the whole guest physical range does not
> work, because that unmaps IO regions (such as the GIC) which will not be
> recreated or in the best case faulted in on a page-by-page basis.
> 
> Call this function on secondary and subsequent calls to the
> KVM_ARM_VCPU_INIT ioctl so that a reset VCPU will detect the guest
> Stage-1 MMU is off when faulting in pages and make the caches coherent.
> 
> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> ---
>  arch/arm/include/asm/kvm_mmu.h   |  1 +
>  arch/arm/kvm/arm.c               |  7 +++++
>  arch/arm/kvm/mmu.c               | 65 ++++++++++++++++++++++++++++++++++++++++
>  arch/arm64/include/asm/kvm_mmu.h |  1 +
>  4 files changed, 74 insertions(+)
> 
> diff --git a/arch/arm/include/asm/kvm_mmu.h b/arch/arm/include/asm/kvm_mmu.h
> index acb0d57..4654c42 100644
> --- a/arch/arm/include/asm/kvm_mmu.h
> +++ b/arch/arm/include/asm/kvm_mmu.h
> @@ -52,6 +52,7 @@ int create_hyp_io_mappings(void *from, void *to, phys_addr_t);
>  void free_boot_hyp_pgd(void);
>  void free_hyp_pgds(void);
>  
> +void stage2_unmap_vm(struct kvm *kvm);
>  int kvm_alloc_stage2_pgd(struct kvm *kvm);
>  void kvm_free_stage2_pgd(struct kvm *kvm);
>  int kvm_phys_addr_ioremap(struct kvm *kvm, phys_addr_t guest_ipa,
> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
> index 4043769..da87c07 100644
> --- a/arch/arm/kvm/arm.c
> +++ b/arch/arm/kvm/arm.c
> @@ -701,6 +701,13 @@ static int kvm_arch_vcpu_ioctl_vcpu_init(struct kvm_vcpu *vcpu,
>  	if (ret)
>  		return ret;
>  
> +	/*
> +	 * Ensure a rebooted VM will fault in RAM pages and detect if the
> +	 * guest MMU is turned off and flush the caches as needed.
> +	 */
> +	if (vcpu->arch.has_run_once)
> +		stage2_unmap_vm(vcpu->kvm);
> +
>  	vcpu_reset_hcr(vcpu);
>  
>  	/*
> diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
> index 57a403a..b1f3c9a 100644
> --- a/arch/arm/kvm/mmu.c
> +++ b/arch/arm/kvm/mmu.c
> @@ -611,6 +611,71 @@ static void unmap_stage2_range(struct kvm *kvm, phys_addr_t start, u64 size)
>  	unmap_range(kvm, kvm->arch.pgd, start, size);
>  }
>  
> +static void stage2_unmap_memslot(struct kvm *kvm,
> +				 struct kvm_memory_slot *memslot)
> +{
> +	hva_t hva = memslot->userspace_addr;
> +	phys_addr_t addr = memslot->base_gfn << PAGE_SHIFT;
> +	phys_addr_t size = PAGE_SIZE * memslot->npages;
> +	hva_t reg_end = hva + size;
> +
> +	/*
> +	 * A memory region could potentially cover multiple VMAs, and any holes
> +	 * between them, so iterate over all of them to find out if we should
> +	 * unmap any of them.
> +	 *
> +	 *     +--------------------------------------------+
> +	 * +---------------+----------------+   +----------------+
> +	 * |   : VMA 1     |      VMA 2     |   |    VMA 3  :    |
> +	 * +---------------+----------------+   +----------------+
> +	 *     |               memory region                |
> +	 *     +--------------------------------------------+
> +	 */
> +	do {
> +		struct vm_area_struct *vma = find_vma(current->mm, hva);
> +		hva_t vm_start, vm_end;
> +
> +		if (!vma || vma->vm_start >= reg_end)
> +			break;
> +
> +		/*
> +		 * Take the intersection of this VMA with the memory region
> +		 */
> +		vm_start = max(hva, vma->vm_start);
> +		vm_end = min(reg_end, vma->vm_end);
> +
> +		if (!(vma->vm_flags & VM_PFNMAP)) {
> +			gpa_t gpa = addr + (vm_start - memslot->userspace_addr);
> +			unmap_stage2_range(kvm, gpa, vm_end - vm_start);
> +		}
> +		hva = vm_end;
> +	} while (hva < reg_end);
> +}
> +
> +/**
> + * stage2_unmap_vm - Unmap Stage-2 RAM mappings
> + * @kvm: The struct kvm pointer
> + *
> + * Go through the memregions and unmap any reguler RAM
> + * backing memory already mapped to the VM.
> + */
> +void stage2_unmap_vm(struct kvm *kvm)
> +{
> +	struct kvm_memslots *slots;
> +	struct kvm_memory_slot *memslot;
> +	int idx;
> +
> +	idx = srcu_read_lock(&kvm->srcu);
> +	spin_lock(&kvm->mmu_lock);
> +
> +	slots = kvm_memslots(kvm);
> +	kvm_for_each_memslot(memslot, slots)
> +		stage2_unmap_memslot(kvm, memslot);
> +
> +	spin_unlock(&kvm->mmu_lock);
> +	srcu_read_unlock(&kvm->srcu, idx);
> +}
> +
>  /**
>   * kvm_free_stage2_pgd - free all stage-2 tables
>   * @kvm:	The KVM struct pointer for the VM.
> diff --git a/arch/arm64/include/asm/kvm_mmu.h b/arch/arm64/include/asm/kvm_mmu.h
> index 0caf7a5..061fed7 100644
> --- a/arch/arm64/include/asm/kvm_mmu.h
> +++ b/arch/arm64/include/asm/kvm_mmu.h
> @@ -83,6 +83,7 @@ int create_hyp_io_mappings(void *from, void *to, phys_addr_t);
>  void free_boot_hyp_pgd(void);
>  void free_hyp_pgds(void);
>  
> +void stage2_unmap_vm(struct kvm *kvm);
>  int kvm_alloc_stage2_pgd(struct kvm *kvm);
>  void kvm_free_stage2_pgd(struct kvm *kvm);
>  int kvm_phys_addr_ioremap(struct kvm *kvm, phys_addr_t guest_ipa,
> 

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

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

[PATCH v2 0/6] Improve PSCI system events and fix reboot bugs

[Andrew Jones]

On Wed, Dec 03, 2014 at 10:18:36PM +0100, Christoffer Dall wrote:
> Several people have reported problems with rebooting ARM VMs, especially
> on 32-bit ARM.  This is mainly due to the same reason we were seeing
> boot errors in the past, namely that the ram, dcache, and icache weren't
> coherent on guest boot with the guest (stage-1) MMU disabled.  We solved
> this by ensuring coherency when we fault in pages, but since most memory
> is already mapped after a reboot, we don't do anything.
> 
> The solution is to unmap the regular RAM on VCPU init, but we must
> take care to not unmap the GIC or other IO regions, hence the somehwat
> complicated solution.
> 
> As part of figuring this out, it became clear that some semantics around
> the KVM_ARM_VCPU_INIT ABI and system event ABI was unclear (what is
> userspace expected to do when it receives a system event).  This series
> also clarifies the ABI and changes the kernel functionality to do what
> userspace expects (turn off VCPUs on a system shutdown event).
> 
> The code is avaliable here as well:
> http://git.linaro.org/people/christoffer.dall/linux-kvm-arm.git vcpu_init_fixes-v2
> 
> There is an alternative version with more code-reuse for the unmapping
> implementation for the previous version of this patch series available
> in the following git repo:
> 
> http://git.linaro.org/people/christoffer.dall/linux-kvm-arm.git vcpu_init_fixes-alternative
> 
> Testing
> -------
> This has been tested on CubieBoard, Arndale, TC2, and Juno.  On Arndale
> and TC2 it was extremely easy to reproduce the problem (just start a VM
> that runs reboot from /etc/rc.local or similar) and this series clearly
> fixes the behavior.  For the previous version of this series, I was
> seeing some problems on Juno, but it turned out to be because I wasn't
> limiting my testing to one of the clusters, and since we don't support
> re-initing a VCPU on a different physical host CPU (big.LITTLE), it was
> failing.  For this version of the patch series, it has been running a
> reboot loop on Juno for hours.

Just tested this version. Looks good. No problems after install nor
after many, many reboots.

drew

> 
> Changelog
> ---------
> Changes v1->v2:
>  - New patch to not clear the VCPU_POWER_OFF flag
>  - Fixed spelling error in commit message
>  - Adapted ABI texts based on Peter's feedback
>  - Check for changed parameters to KVM_ARM_VCPU_INIT
>  - Now unmap the Stage-2 RAM mappings at VCPU init instead of at PSCI
>    system event time.
> 
> Christoffer Dall (6):
>   arm/arm64: KVM: Don't clear the VCPU_POWER_OFF flag
>   arm/arm64: KVM: Correct KVM_ARM_VCPU_INIT power off option
>   arm/arm64: KVM: Reset the HCR on each vcpu when resetting the vcpu
>   arm/arm64: KVM: Clarify KVM_ARM_VCPU_INIT ABI
>   arm/arm64: KVM: Turn off vcpus on PSCI shutdown/reboot
>   arm/arm64: KVM: Introduce stage2_unmap_vm
> 
>  Documentation/virtual/kvm/api.txt    | 17 +++++++++-
>  arch/arm/include/asm/kvm_emulate.h   |  5 +++
>  arch/arm/include/asm/kvm_host.h      |  2 --
>  arch/arm/include/asm/kvm_mmu.h       |  1 +
>  arch/arm/kvm/arm.c                   | 56 ++++++++++++++++++++++++++++++-
>  arch/arm/kvm/guest.c                 | 26 ---------------
>  arch/arm/kvm/mmu.c                   | 65 ++++++++++++++++++++++++++++++++++++
>  arch/arm/kvm/psci.c                  | 19 +++++++++++
>  arch/arm64/include/asm/kvm_emulate.h |  5 +++
>  arch/arm64/include/asm/kvm_host.h    |  3 +-
>  arch/arm64/include/asm/kvm_mmu.h     |  1 +
>  arch/arm64/kvm/guest.c               | 26 ---------------
>  12 files changed, 168 insertions(+), 58 deletions(-)
> 
> -- 
> 2.1.2.330.g565301e.dirty
> 

[PATCH v2 0/6] Improve PSCI system events and fix reboot bugs

[Peter Maydell]

On 3 December 2014 at 21:18, Christoffer Dall
<christoffer.dall@linaro.org> wrote:
> Several people have reported problems with rebooting ARM VMs, especially
> on 32-bit ARM.  This is mainly due to the same reason we were seeing
> boot errors in the past, namely that the ram, dcache, and icache weren't
> coherent on guest boot with the guest (stage-1) MMU disabled.  We solved
> this by ensuring coherency when we fault in pages, but since most memory
> is already mapped after a reboot, we don't do anything.
>
> The solution is to unmap the regular RAM on VCPU init, but we must
> take care to not unmap the GIC or other IO regions, hence the somehwat
> complicated solution.
>
> As part of figuring this out, it became clear that some semantics around
> the KVM_ARM_VCPU_INIT ABI and system event ABI was unclear (what is
> userspace expected to do when it receives a system event).  This series
> also clarifies the ABI and changes the kernel functionality to do what
> userspace expects (turn off VCPUs on a system shutdown event).

Userspace ABI documentation clarifications:
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

-- PMM