[PATCH 0/2] KVM: arm/arm64: Guest synchronous halt/resume

[PATCH 0/2] KVM: arm/arm64: Guest synchronous halt/resume

[Eric Auger]

This series introduces the capability to synchronously exit the guest
and prevent it from being re-entered.

This functionality is requested for IRQ forwarding where we need to make
sure the guest has exited and is prevented from being rescheduled until
we complete the forwarding state change.

This series does not introduce any user of the functionality. First user
will be IRQ forwarding. The split is done to ease the review process.

can be found at:
https://git.linaro.org/people/eric.auger/linux.git/shortlog/refs/heads/v4.2-rc1-bypass-fwd-v2

Best Regards

Eric

History:

PATCH:
- just added __maybe_unused following Marc's advice

RFC life before:
- originally part of [RFC 00/17] ARM IRQ forward control based on IRQ
  bypass manager (https://lkml.org/lkml/2015/7/2/268) and isolated in
  this series.

Eric Auger (2):
  KVM: arm: rename pause into power_off
  kvm: arm/arm64: implement kvm_arm_[halt,resume]_guest

 arch/arm/include/asm/kvm_host.h   |  5 ++++-
 arch/arm/kvm/arm.c                | 41 +++++++++++++++++++++++++++++++++------
 arch/arm/kvm/psci.c               | 10 +++++-----
 arch/arm64/include/asm/kvm_host.h |  3 +++
 4 files changed, 47 insertions(+), 12 deletions(-)

-- 
1.9.1

[PATCH 0/2] KVM: arm/arm64: Guest synchronous halt/resume

[Eric Auger]

This series introduces the capability to synchronously exit the guest
and prevent it from being re-entered.

This functionality is requested for IRQ forwarding where we need to make
sure the guest has exited and is prevented from being rescheduled until
we complete the forwarding state change.

This series does not introduce any user of the functionality. First user
will be IRQ forwarding. The split is done to ease the review process.

can be found at:
https://git.linaro.org/people/eric.auger/linux.git/shortlog/refs/heads/v4.2-rc1-bypass-fwd-v2

Best Regards

Eric

History:

PATCH:
- just added __maybe_unused following Marc's advice

RFC life before:
- originally part of [RFC 00/17] ARM IRQ forward control based on IRQ
  bypass manager (https://lkml.org/lkml/2015/7/2/268) and isolated in
  this series.

Eric Auger (2):
  KVM: arm: rename pause into power_off
  kvm: arm/arm64: implement kvm_arm_[halt,resume]_guest

 arch/arm/include/asm/kvm_host.h   |  5 ++++-
 arch/arm/kvm/arm.c                | 41 +++++++++++++++++++++++++++++++++------
 arch/arm/kvm/psci.c               | 10 +++++-----
 arch/arm64/include/asm/kvm_host.h |  3 +++
 4 files changed, 47 insertions(+), 12 deletions(-)

-- 
1.9.1

[PATCH 1/2] KVM: arm: rename pause into power_off

[Eric Auger]

The kvm_vcpu_arch pause field is renamed into power_off to prepare
for the introduction of a new pause field.

Signed-off-by: Eric Auger <eric.auger@linaro.org>

v4 -> v5:
- fix compilation issue on arm64 (add power_off field in kvm_host.h)
---
 arch/arm/include/asm/kvm_host.h   |  4 ++--
 arch/arm/kvm/arm.c                | 10 +++++-----
 arch/arm/kvm/psci.c               | 10 +++++-----
 arch/arm64/include/asm/kvm_host.h |  4 ++--
 4 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
index e896d2c..304004d 100644
--- a/arch/arm/include/asm/kvm_host.h
+++ b/arch/arm/include/asm/kvm_host.h
@@ -129,8 +129,8 @@ struct kvm_vcpu_arch {
 	 * here.
 	 */
 
-	/* Don't run the guest on this vcpu */
-	bool pause;
+	/* vcpu power-off state */
+	bool power_off;
 
 	/* IO related fields */
 	struct kvm_decode mmio_decode;
diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
index bcdf799..7537e68 100644
--- a/arch/arm/kvm/arm.c
+++ b/arch/arm/kvm/arm.c
@@ -475,7 +475,7 @@ static void vcpu_pause(struct kvm_vcpu *vcpu)
 {
 	wait_queue_head_t *wq = kvm_arch_vcpu_wq(vcpu);
 
-	wait_event_interruptible(*wq, !vcpu->arch.pause);
+	wait_event_interruptible(*wq, !vcpu->arch.power_off);
 }
 
 static int kvm_vcpu_initialized(struct kvm_vcpu *vcpu)
@@ -525,7 +525,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
 
 		update_vttbr(vcpu->kvm);
 
-		if (vcpu->arch.pause)
+		if (vcpu->arch.power_off)
 			vcpu_pause(vcpu);
 
 		/*
@@ -766,12 +766,12 @@ static int kvm_arch_vcpu_ioctl_vcpu_init(struct kvm_vcpu *vcpu,
 	vcpu_reset_hcr(vcpu);
 
 	/*
-	 * Handle the "start in power-off" case by marking the VCPU as paused.
+	 * Handle the "start in power-off" case.
 	 */
 	if (test_bit(KVM_ARM_VCPU_POWER_OFF, vcpu->arch.features))
-		vcpu->arch.pause = true;
+		vcpu->arch.power_off = true;
 	else
-		vcpu->arch.pause = false;
+		vcpu->arch.power_off = false;
 
 	return 0;
 }
diff --git a/arch/arm/kvm/psci.c b/arch/arm/kvm/psci.c
index 4b94b51..134971a 100644
--- a/arch/arm/kvm/psci.c
+++ b/arch/arm/kvm/psci.c
@@ -63,7 +63,7 @@ static unsigned long kvm_psci_vcpu_suspend(struct kvm_vcpu *vcpu)
 
 static void kvm_psci_vcpu_off(struct kvm_vcpu *vcpu)
 {
-	vcpu->arch.pause = true;
+	vcpu->arch.power_off = true;
 }
 
 static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu)
@@ -87,7 +87,7 @@ static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu)
 	 */
 	if (!vcpu)
 		return PSCI_RET_INVALID_PARAMS;
-	if (!vcpu->arch.pause) {
+	if (!vcpu->arch.power_off) {
 		if (kvm_psci_version(source_vcpu) != KVM_ARM_PSCI_0_1)
 			return PSCI_RET_ALREADY_ON;
 		else
@@ -115,7 +115,7 @@ static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu)
 	 * the general puspose registers are undefined upon CPU_ON.
 	 */
 	*vcpu_reg(vcpu, 0) = context_id;
-	vcpu->arch.pause = false;
+	vcpu->arch.power_off = false;
 	smp_mb();		/* Make sure the above is visible */
 
 	wq = kvm_arch_vcpu_wq(vcpu);
@@ -152,7 +152,7 @@ static unsigned long kvm_psci_vcpu_affinity_info(struct kvm_vcpu *vcpu)
 	kvm_for_each_vcpu(i, tmp, kvm) {
 		mpidr = kvm_vcpu_get_mpidr_aff(tmp);
 		if (((mpidr & target_affinity_mask) == target_affinity) &&
-		    !tmp->arch.pause) {
+		    !tmp->arch.power_off) {
 			return PSCI_0_2_AFFINITY_LEVEL_ON;
 		}
 	}
@@ -175,7 +175,7 @@ static void kvm_prepare_system_event(struct kvm_vcpu *vcpu, u32 type)
 	 * re-initialized.
 	 */
 	kvm_for_each_vcpu(i, tmp, vcpu->kvm) {
-		tmp->arch.pause = true;
+		tmp->arch.power_off = true;
 		kvm_vcpu_kick(tmp);
 	}
 
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 2709db2..009da6b 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -122,8 +122,8 @@ struct kvm_vcpu_arch {
 	 * here.
 	 */
 
-	/* Don't run the guest */
-	bool pause;
+	/* vcpu power-off state */
+	bool power_off;
 
 	/* IO related fields */
 	struct kvm_decode mmio_decode;
-- 
1.9.1

[PATCH 1/2] KVM: arm: rename pause into power_off

[Eric Auger]

The kvm_vcpu_arch pause field is renamed into power_off to prepare
for the introduction of a new pause field.

Signed-off-by: Eric Auger <eric.auger@linaro.org>

v4 -> v5:
- fix compilation issue on arm64 (add power_off field in kvm_host.h)
---
 arch/arm/include/asm/kvm_host.h   |  4 ++--
 arch/arm/kvm/arm.c                | 10 +++++-----
 arch/arm/kvm/psci.c               | 10 +++++-----
 arch/arm64/include/asm/kvm_host.h |  4 ++--
 4 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
index e896d2c..304004d 100644
--- a/arch/arm/include/asm/kvm_host.h
+++ b/arch/arm/include/asm/kvm_host.h
@@ -129,8 +129,8 @@ struct kvm_vcpu_arch {
 	 * here.
 	 */
 
-	/* Don't run the guest on this vcpu */
-	bool pause;
+	/* vcpu power-off state */
+	bool power_off;
 
 	/* IO related fields */
 	struct kvm_decode mmio_decode;
diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
index bcdf799..7537e68 100644
--- a/arch/arm/kvm/arm.c
+++ b/arch/arm/kvm/arm.c
@@ -475,7 +475,7 @@ static void vcpu_pause(struct kvm_vcpu *vcpu)
 {
 	wait_queue_head_t *wq = kvm_arch_vcpu_wq(vcpu);
 
-	wait_event_interruptible(*wq, !vcpu->arch.pause);
+	wait_event_interruptible(*wq, !vcpu->arch.power_off);
 }
 
 static int kvm_vcpu_initialized(struct kvm_vcpu *vcpu)
@@ -525,7 +525,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
 
 		update_vttbr(vcpu->kvm);
 
-		if (vcpu->arch.pause)
+		if (vcpu->arch.power_off)
 			vcpu_pause(vcpu);
 
 		/*
@@ -766,12 +766,12 @@ static int kvm_arch_vcpu_ioctl_vcpu_init(struct kvm_vcpu *vcpu,
 	vcpu_reset_hcr(vcpu);
 
 	/*
-	 * Handle the "start in power-off" case by marking the VCPU as paused.
+	 * Handle the "start in power-off" case.
 	 */
 	if (test_bit(KVM_ARM_VCPU_POWER_OFF, vcpu->arch.features))
-		vcpu->arch.pause = true;
+		vcpu->arch.power_off = true;
 	else
-		vcpu->arch.pause = false;
+		vcpu->arch.power_off = false;
 
 	return 0;
 }
diff --git a/arch/arm/kvm/psci.c b/arch/arm/kvm/psci.c
index 4b94b51..134971a 100644
--- a/arch/arm/kvm/psci.c
+++ b/arch/arm/kvm/psci.c
@@ -63,7 +63,7 @@ static unsigned long kvm_psci_vcpu_suspend(struct kvm_vcpu *vcpu)
 
 static void kvm_psci_vcpu_off(struct kvm_vcpu *vcpu)
 {
-	vcpu->arch.pause = true;
+	vcpu->arch.power_off = true;
 }
 
 static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu)
@@ -87,7 +87,7 @@ static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu)
 	 */
 	if (!vcpu)
 		return PSCI_RET_INVALID_PARAMS;
-	if (!vcpu->arch.pause) {
+	if (!vcpu->arch.power_off) {
 		if (kvm_psci_version(source_vcpu) != KVM_ARM_PSCI_0_1)
 			return PSCI_RET_ALREADY_ON;
 		else
@@ -115,7 +115,7 @@ static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu)
 	 * the general puspose registers are undefined upon CPU_ON.
 	 */
 	*vcpu_reg(vcpu, 0) = context_id;
-	vcpu->arch.pause = false;
+	vcpu->arch.power_off = false;
 	smp_mb();		/* Make sure the above is visible */
 
 	wq = kvm_arch_vcpu_wq(vcpu);
@@ -152,7 +152,7 @@ static unsigned long kvm_psci_vcpu_affinity_info(struct kvm_vcpu *vcpu)
 	kvm_for_each_vcpu(i, tmp, kvm) {
 		mpidr = kvm_vcpu_get_mpidr_aff(tmp);
 		if (((mpidr & target_affinity_mask) == target_affinity) &&
-		    !tmp->arch.pause) {
+		    !tmp->arch.power_off) {
 			return PSCI_0_2_AFFINITY_LEVEL_ON;
 		}
 	}
@@ -175,7 +175,7 @@ static void kvm_prepare_system_event(struct kvm_vcpu *vcpu, u32 type)
 	 * re-initialized.
 	 */
 	kvm_for_each_vcpu(i, tmp, vcpu->kvm) {
-		tmp->arch.pause = true;
+		tmp->arch.power_off = true;
 		kvm_vcpu_kick(tmp);
 	}
 
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 2709db2..009da6b 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -122,8 +122,8 @@ struct kvm_vcpu_arch {
 	 * here.
 	 */
 
-	/* Don't run the guest */
-	bool pause;
+	/* vcpu power-off state */
+	bool power_off;
 
 	/* IO related fields */
 	struct kvm_decode mmio_decode;
-- 
1.9.1

[PATCH 2/2] kvm: arm/arm64: implement kvm_arm_[halt,resume]_guest

[Eric Auger]

On halt, the guest is forced to exit and prevented from being
re-entered. This is synchronous.

Those two operations will be needed for IRQ forwarding setting.

Signed-off-by: Eric Auger <eric.auger@linaro.org>

---
RFC v1 -> v2:
- add __maybe_unused

RFC:
- rename the function and this latter becomes static
- remove __KVM_HAVE_ARCH_HALT_GUEST

v4 -> v5: add arm64 support
- also defines __KVM_HAVE_ARCH_HALT_GUEST for arm64
- add pause field
---
 arch/arm/include/asm/kvm_host.h   |  3 +++
 arch/arm/kvm/arm.c                | 35 ++++++++++++++++++++++++++++++++---
 arch/arm64/include/asm/kvm_host.h |  3 +++
 3 files changed, 38 insertions(+), 3 deletions(-)

diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
index 304004d..899ae27 100644
--- a/arch/arm/include/asm/kvm_host.h
+++ b/arch/arm/include/asm/kvm_host.h
@@ -132,6 +132,9 @@ struct kvm_vcpu_arch {
 	/* vcpu power-off state */
 	bool power_off;
 
+	/* Don't run the guest */
+	bool pause;
+
 	/* IO related fields */
 	struct kvm_decode mmio_decode;
 
diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
index 7537e68..46d4ef6 100644
--- a/arch/arm/kvm/arm.c
+++ b/arch/arm/kvm/arm.c
@@ -471,11 +471,39 @@ bool kvm_arch_intc_initialized(struct kvm *kvm)
 	return vgic_initialized(kvm);
 }
 
+static void kvm_arm_halt_guest(struct kvm *kvm) __maybe_unused;
+static void kvm_arm_resume_guest(struct kvm *kvm) __maybe_unused;
+
+static void kvm_arm_halt_guest(struct kvm *kvm)
+{
+	int i;
+	struct kvm_vcpu *vcpu;
+
+	kvm_for_each_vcpu(i, vcpu, kvm)
+		vcpu->arch.pause = true;
+	force_vm_exit(cpu_all_mask);
+}
+
+static void kvm_arm_resume_guest(struct kvm *kvm)
+{
+	int i;
+	struct kvm_vcpu *vcpu;
+
+	kvm_for_each_vcpu(i, vcpu, kvm) {
+		wait_queue_head_t *wq = kvm_arch_vcpu_wq(vcpu);
+
+		vcpu->arch.pause = false;
+		wake_up_interruptible(wq);
+	}
+}
+
+
 static void vcpu_pause(struct kvm_vcpu *vcpu)
 {
 	wait_queue_head_t *wq = kvm_arch_vcpu_wq(vcpu);
 
-	wait_event_interruptible(*wq, !vcpu->arch.power_off);
+	wait_event_interruptible(*wq, ((!vcpu->arch.power_off) &&
+				       (!vcpu->arch.pause)));
 }
 
 static int kvm_vcpu_initialized(struct kvm_vcpu *vcpu)
@@ -525,7 +553,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
 
 		update_vttbr(vcpu->kvm);
 
-		if (vcpu->arch.power_off)
+		if (vcpu->arch.power_off || vcpu->arch.pause)
 			vcpu_pause(vcpu);
 
 		/*
@@ -551,7 +579,8 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
 			run->exit_reason = KVM_EXIT_INTR;
 		}
 
-		if (ret <= 0 || need_new_vmid_gen(vcpu->kvm)) {
+		if (ret <= 0 || need_new_vmid_gen(vcpu->kvm) ||
+		    vcpu->arch.pause) {
 			local_irq_enable();
 			preempt_enable();
 			kvm_vgic_sync_hwstate(vcpu);
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 009da6b..69e3785 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -125,6 +125,9 @@ struct kvm_vcpu_arch {
 	/* vcpu power-off state */
 	bool power_off;
 
+	/* Don't run the guest */
+	bool pause;
+
 	/* IO related fields */
 	struct kvm_decode mmio_decode;
 
-- 
1.9.1

[PATCH 2/2] kvm: arm/arm64: implement kvm_arm_[halt,resume]_guest

[Eric Auger]

On halt, the guest is forced to exit and prevented from being
re-entered. This is synchronous.

Those two operations will be needed for IRQ forwarding setting.

Signed-off-by: Eric Auger <eric.auger@linaro.org>

---
RFC v1 -> v2:
- add __maybe_unused

RFC:
- rename the function and this latter becomes static
- remove __KVM_HAVE_ARCH_HALT_GUEST

v4 -> v5: add arm64 support
- also defines __KVM_HAVE_ARCH_HALT_GUEST for arm64
- add pause field
---
 arch/arm/include/asm/kvm_host.h   |  3 +++
 arch/arm/kvm/arm.c                | 35 ++++++++++++++++++++++++++++++++---
 arch/arm64/include/asm/kvm_host.h |  3 +++
 3 files changed, 38 insertions(+), 3 deletions(-)

diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
index 304004d..899ae27 100644
--- a/arch/arm/include/asm/kvm_host.h
+++ b/arch/arm/include/asm/kvm_host.h
@@ -132,6 +132,9 @@ struct kvm_vcpu_arch {
 	/* vcpu power-off state */
 	bool power_off;
 
+	/* Don't run the guest */
+	bool pause;
+
 	/* IO related fields */
 	struct kvm_decode mmio_decode;
 
diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
index 7537e68..46d4ef6 100644
--- a/arch/arm/kvm/arm.c
+++ b/arch/arm/kvm/arm.c
@@ -471,11 +471,39 @@ bool kvm_arch_intc_initialized(struct kvm *kvm)
 	return vgic_initialized(kvm);
 }
 
+static void kvm_arm_halt_guest(struct kvm *kvm) __maybe_unused;
+static void kvm_arm_resume_guest(struct kvm *kvm) __maybe_unused;
+
+static void kvm_arm_halt_guest(struct kvm *kvm)
+{
+	int i;
+	struct kvm_vcpu *vcpu;
+
+	kvm_for_each_vcpu(i, vcpu, kvm)
+		vcpu->arch.pause = true;
+	force_vm_exit(cpu_all_mask);
+}
+
+static void kvm_arm_resume_guest(struct kvm *kvm)
+{
+	int i;
+	struct kvm_vcpu *vcpu;
+
+	kvm_for_each_vcpu(i, vcpu, kvm) {
+		wait_queue_head_t *wq = kvm_arch_vcpu_wq(vcpu);
+
+		vcpu->arch.pause = false;
+		wake_up_interruptible(wq);
+	}
+}
+
+
 static void vcpu_pause(struct kvm_vcpu *vcpu)
 {
 	wait_queue_head_t *wq = kvm_arch_vcpu_wq(vcpu);
 
-	wait_event_interruptible(*wq, !vcpu->arch.power_off);
+	wait_event_interruptible(*wq, ((!vcpu->arch.power_off) &&
+				       (!vcpu->arch.pause)));
 }
 
 static int kvm_vcpu_initialized(struct kvm_vcpu *vcpu)
@@ -525,7 +553,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
 
 		update_vttbr(vcpu->kvm);
 
-		if (vcpu->arch.power_off)
+		if (vcpu->arch.power_off || vcpu->arch.pause)
 			vcpu_pause(vcpu);
 
 		/*
@@ -551,7 +579,8 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
 			run->exit_reason = KVM_EXIT_INTR;
 		}
 
-		if (ret <= 0 || need_new_vmid_gen(vcpu->kvm)) {
+		if (ret <= 0 || need_new_vmid_gen(vcpu->kvm) ||
+		    vcpu->arch.pause) {
 			local_irq_enable();
 			preempt_enable();
 			kvm_vgic_sync_hwstate(vcpu);
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 009da6b..69e3785 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -125,6 +125,9 @@ struct kvm_vcpu_arch {
 	/* vcpu power-off state */
 	bool power_off;
 
+	/* Don't run the guest */
+	bool pause;
+
 	/* IO related fields */
 	struct kvm_decode mmio_decode;
 
-- 
1.9.1

Re: [PATCH 1/2] KVM: arm: rename pause into power_off

[Andrew Jones]

On Mon, Jul 06, 2015 at 02:49:55PM +0200, Eric Auger wrote:
> The kvm_vcpu_arch pause field is renamed into power_off to prepare
> for the introduction of a new pause field.
> 
> Signed-off-by: Eric Auger <eric.auger@linaro.org>
> 
> v4 -> v5:
> - fix compilation issue on arm64 (add power_off field in kvm_host.h)
> ---
>  arch/arm/include/asm/kvm_host.h   |  4 ++--
>  arch/arm/kvm/arm.c                | 10 +++++-----
>  arch/arm/kvm/psci.c               | 10 +++++-----
>  arch/arm64/include/asm/kvm_host.h |  4 ++--
>  4 files changed, 14 insertions(+), 14 deletions(-)
> 
> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
> index e896d2c..304004d 100644
> --- a/arch/arm/include/asm/kvm_host.h
> +++ b/arch/arm/include/asm/kvm_host.h
> @@ -129,8 +129,8 @@ struct kvm_vcpu_arch {
>  	 * here.
>  	 */
>  
> -	/* Don't run the guest on this vcpu */
> -	bool pause;

This patch should leave pause in. kvm_arch_vcpu_ioctl_set_mpstate still
references it, compilation is broken until the next patch comes.

> +	/* vcpu power-off state */
> +	bool power_off;
>  
>  	/* IO related fields */
>  	struct kvm_decode mmio_decode;
> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
> index bcdf799..7537e68 100644
> --- a/arch/arm/kvm/arm.c
> +++ b/arch/arm/kvm/arm.c
> @@ -475,7 +475,7 @@ static void vcpu_pause(struct kvm_vcpu *vcpu)
>  {
>  	wait_queue_head_t *wq = kvm_arch_vcpu_wq(vcpu);
>  
> -	wait_event_interruptible(*wq, !vcpu->arch.pause);
> +	wait_event_interruptible(*wq, !vcpu->arch.power_off);

Leaving pause in would allow this to already be

+       wait_event_interruptible(*wq, ((!vcpu->arch.power_off) &&
+                                      (!vcpu->arch.pause)));

which feels better. The function name is vcpu_pause, so it's nice
to see state named 'pause' in there.

>  }
>  
>  static int kvm_vcpu_initialized(struct kvm_vcpu *vcpu)
> @@ -525,7 +525,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
>  
>  		update_vttbr(vcpu->kvm);
>  
> -		if (vcpu->arch.pause)
> +		if (vcpu->arch.power_off)
>  			vcpu_pause(vcpu);
>  
>  		/*
> @@ -766,12 +766,12 @@ static int kvm_arch_vcpu_ioctl_vcpu_init(struct kvm_vcpu *vcpu,
>  	vcpu_reset_hcr(vcpu);
>  
>  	/*
> -	 * Handle the "start in power-off" case by marking the VCPU as paused.
> +	 * Handle the "start in power-off" case.
>  	 */
>  	if (test_bit(KVM_ARM_VCPU_POWER_OFF, vcpu->arch.features))
> -		vcpu->arch.pause = true;
> +		vcpu->arch.power_off = true;
>  	else
> -		vcpu->arch.pause = false;
> +		vcpu->arch.power_off = false;
>  
>  	return 0;
>  }
> diff --git a/arch/arm/kvm/psci.c b/arch/arm/kvm/psci.c
> index 4b94b51..134971a 100644
> --- a/arch/arm/kvm/psci.c
> +++ b/arch/arm/kvm/psci.c
> @@ -63,7 +63,7 @@ static unsigned long kvm_psci_vcpu_suspend(struct kvm_vcpu *vcpu)
>  
>  static void kvm_psci_vcpu_off(struct kvm_vcpu *vcpu)
>  {
> -	vcpu->arch.pause = true;
> +	vcpu->arch.power_off = true;
>  }
>  
>  static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu)
> @@ -87,7 +87,7 @@ static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu)
>  	 */
>  	if (!vcpu)
>  		return PSCI_RET_INVALID_PARAMS;
> -	if (!vcpu->arch.pause) {
> +	if (!vcpu->arch.power_off) {
>  		if (kvm_psci_version(source_vcpu) != KVM_ARM_PSCI_0_1)
>  			return PSCI_RET_ALREADY_ON;
>  		else
> @@ -115,7 +115,7 @@ static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu)
>  	 * the general puspose registers are undefined upon CPU_ON.
>  	 */
>  	*vcpu_reg(vcpu, 0) = context_id;
> -	vcpu->arch.pause = false;
> +	vcpu->arch.power_off = false;
>  	smp_mb();		/* Make sure the above is visible */
>  
>  	wq = kvm_arch_vcpu_wq(vcpu);
> @@ -152,7 +152,7 @@ static unsigned long kvm_psci_vcpu_affinity_info(struct kvm_vcpu *vcpu)
>  	kvm_for_each_vcpu(i, tmp, kvm) {
>  		mpidr = kvm_vcpu_get_mpidr_aff(tmp);
>  		if (((mpidr & target_affinity_mask) == target_affinity) &&
> -		    !tmp->arch.pause) {
> +		    !tmp->arch.power_off) {
>  			return PSCI_0_2_AFFINITY_LEVEL_ON;
>  		}
>  	}
> @@ -175,7 +175,7 @@ static void kvm_prepare_system_event(struct kvm_vcpu *vcpu, u32 type)
>  	 * re-initialized.
>  	 */
>  	kvm_for_each_vcpu(i, tmp, vcpu->kvm) {
> -		tmp->arch.pause = true;
> +		tmp->arch.power_off = true;
>  		kvm_vcpu_kick(tmp);
>  	}
>  
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index 2709db2..009da6b 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -122,8 +122,8 @@ struct kvm_vcpu_arch {
>  	 * here.
>  	 */
>  
> -	/* Don't run the guest */
> -	bool pause;
> +	/* vcpu power-off state */
> +	bool power_off;
>  
>  	/* IO related fields */
>  	struct kvm_decode mmio_decode;
> -- 
> 1.9.1

Thanks,
drew

[PATCH 1/2] KVM: arm: rename pause into power_off

[Andrew Jones]

On Mon, Jul 06, 2015 at 02:49:55PM +0200, Eric Auger wrote:
> The kvm_vcpu_arch pause field is renamed into power_off to prepare
> for the introduction of a new pause field.
> 
> Signed-off-by: Eric Auger <eric.auger@linaro.org>
> 
> v4 -> v5:
> - fix compilation issue on arm64 (add power_off field in kvm_host.h)
> ---
>  arch/arm/include/asm/kvm_host.h   |  4 ++--
>  arch/arm/kvm/arm.c                | 10 +++++-----
>  arch/arm/kvm/psci.c               | 10 +++++-----
>  arch/arm64/include/asm/kvm_host.h |  4 ++--
>  4 files changed, 14 insertions(+), 14 deletions(-)
> 
> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
> index e896d2c..304004d 100644
> --- a/arch/arm/include/asm/kvm_host.h
> +++ b/arch/arm/include/asm/kvm_host.h
> @@ -129,8 +129,8 @@ struct kvm_vcpu_arch {
>  	 * here.
>  	 */
>  
> -	/* Don't run the guest on this vcpu */
> -	bool pause;

This patch should leave pause in. kvm_arch_vcpu_ioctl_set_mpstate still
references it, compilation is broken until the next patch comes.

> +	/* vcpu power-off state */
> +	bool power_off;
>  
>  	/* IO related fields */
>  	struct kvm_decode mmio_decode;
> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
> index bcdf799..7537e68 100644
> --- a/arch/arm/kvm/arm.c
> +++ b/arch/arm/kvm/arm.c
> @@ -475,7 +475,7 @@ static void vcpu_pause(struct kvm_vcpu *vcpu)
>  {
>  	wait_queue_head_t *wq = kvm_arch_vcpu_wq(vcpu);
>  
> -	wait_event_interruptible(*wq, !vcpu->arch.pause);
> +	wait_event_interruptible(*wq, !vcpu->arch.power_off);

Leaving pause in would allow this to already be

+       wait_event_interruptible(*wq, ((!vcpu->arch.power_off) &&
+                                      (!vcpu->arch.pause)));

which feels better. The function name is vcpu_pause, so it's nice
to see state named 'pause' in there.

>  }
>  
>  static int kvm_vcpu_initialized(struct kvm_vcpu *vcpu)
> @@ -525,7 +525,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
>  
>  		update_vttbr(vcpu->kvm);
>  
> -		if (vcpu->arch.pause)
> +		if (vcpu->arch.power_off)
>  			vcpu_pause(vcpu);
>  
>  		/*
> @@ -766,12 +766,12 @@ static int kvm_arch_vcpu_ioctl_vcpu_init(struct kvm_vcpu *vcpu,
>  	vcpu_reset_hcr(vcpu);
>  
>  	/*
> -	 * Handle the "start in power-off" case by marking the VCPU as paused.
> +	 * Handle the "start in power-off" case.
>  	 */
>  	if (test_bit(KVM_ARM_VCPU_POWER_OFF, vcpu->arch.features))
> -		vcpu->arch.pause = true;
> +		vcpu->arch.power_off = true;
>  	else
> -		vcpu->arch.pause = false;
> +		vcpu->arch.power_off = false;
>  
>  	return 0;
>  }
> diff --git a/arch/arm/kvm/psci.c b/arch/arm/kvm/psci.c
> index 4b94b51..134971a 100644
> --- a/arch/arm/kvm/psci.c
> +++ b/arch/arm/kvm/psci.c
> @@ -63,7 +63,7 @@ static unsigned long kvm_psci_vcpu_suspend(struct kvm_vcpu *vcpu)
>  
>  static void kvm_psci_vcpu_off(struct kvm_vcpu *vcpu)
>  {
> -	vcpu->arch.pause = true;
> +	vcpu->arch.power_off = true;
>  }
>  
>  static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu)
> @@ -87,7 +87,7 @@ static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu)
>  	 */
>  	if (!vcpu)
>  		return PSCI_RET_INVALID_PARAMS;
> -	if (!vcpu->arch.pause) {
> +	if (!vcpu->arch.power_off) {
>  		if (kvm_psci_version(source_vcpu) != KVM_ARM_PSCI_0_1)
>  			return PSCI_RET_ALREADY_ON;
>  		else
> @@ -115,7 +115,7 @@ static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu)
>  	 * the general puspose registers are undefined upon CPU_ON.
>  	 */
>  	*vcpu_reg(vcpu, 0) = context_id;
> -	vcpu->arch.pause = false;
> +	vcpu->arch.power_off = false;
>  	smp_mb();		/* Make sure the above is visible */
>  
>  	wq = kvm_arch_vcpu_wq(vcpu);
> @@ -152,7 +152,7 @@ static unsigned long kvm_psci_vcpu_affinity_info(struct kvm_vcpu *vcpu)
>  	kvm_for_each_vcpu(i, tmp, kvm) {
>  		mpidr = kvm_vcpu_get_mpidr_aff(tmp);
>  		if (((mpidr & target_affinity_mask) == target_affinity) &&
> -		    !tmp->arch.pause) {
> +		    !tmp->arch.power_off) {
>  			return PSCI_0_2_AFFINITY_LEVEL_ON;
>  		}
>  	}
> @@ -175,7 +175,7 @@ static void kvm_prepare_system_event(struct kvm_vcpu *vcpu, u32 type)
>  	 * re-initialized.
>  	 */
>  	kvm_for_each_vcpu(i, tmp, vcpu->kvm) {
> -		tmp->arch.pause = true;
> +		tmp->arch.power_off = true;
>  		kvm_vcpu_kick(tmp);
>  	}
>  
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index 2709db2..009da6b 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -122,8 +122,8 @@ struct kvm_vcpu_arch {
>  	 * here.
>  	 */
>  
> -	/* Don't run the guest */
> -	bool pause;
> +	/* vcpu power-off state */
> +	bool power_off;
>  
>  	/* IO related fields */
>  	struct kvm_decode mmio_decode;
> -- 
> 1.9.1

Thanks,
drew

Re: [PATCH 2/2] kvm: arm/arm64: implement kvm_arm_[halt, resume]_guest

[Andrew Jones]

On Mon, Jul 06, 2015 at 02:49:56PM +0200, Eric Auger wrote:
> On halt, the guest is forced to exit and prevented from being
> re-entered. This is synchronous.
> 
> Those two operations will be needed for IRQ forwarding setting.
> 
> Signed-off-by: Eric Auger <eric.auger@linaro.org>
> 
> ---
> RFC v1 -> v2:
> - add __maybe_unused
> 
> RFC:
> - rename the function and this latter becomes static
> - remove __KVM_HAVE_ARCH_HALT_GUEST
> 
> v4 -> v5: add arm64 support
> - also defines __KVM_HAVE_ARCH_HALT_GUEST for arm64
> - add pause field
> ---
>  arch/arm/include/asm/kvm_host.h   |  3 +++
>  arch/arm/kvm/arm.c                | 35 ++++++++++++++++++++++++++++++++---
>  arch/arm64/include/asm/kvm_host.h |  3 +++
>  3 files changed, 38 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
> index 304004d..899ae27 100644
> --- a/arch/arm/include/asm/kvm_host.h
> +++ b/arch/arm/include/asm/kvm_host.h
> @@ -132,6 +132,9 @@ struct kvm_vcpu_arch {
>  	/* vcpu power-off state */
>  	bool power_off;
>  
> +	/* Don't run the guest */
> +	bool pause;
> +
>  	/* IO related fields */
>  	struct kvm_decode mmio_decode;
>  
> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
> index 7537e68..46d4ef6 100644
> --- a/arch/arm/kvm/arm.c
> +++ b/arch/arm/kvm/arm.c
> @@ -471,11 +471,39 @@ bool kvm_arch_intc_initialized(struct kvm *kvm)
>  	return vgic_initialized(kvm);
>  }
>  
> +static void kvm_arm_halt_guest(struct kvm *kvm) __maybe_unused;
> +static void kvm_arm_resume_guest(struct kvm *kvm) __maybe_unused;
> +
> +static void kvm_arm_halt_guest(struct kvm *kvm)
> +{
> +	int i;
> +	struct kvm_vcpu *vcpu;
> +
> +	kvm_for_each_vcpu(i, vcpu, kvm)
> +		vcpu->arch.pause = true;
> +	force_vm_exit(cpu_all_mask);
> +}
> +
> +static void kvm_arm_resume_guest(struct kvm *kvm)
> +{
> +	int i;
> +	struct kvm_vcpu *vcpu;
> +
> +	kvm_for_each_vcpu(i, vcpu, kvm) {
> +		wait_queue_head_t *wq = kvm_arch_vcpu_wq(vcpu);
> +
> +		vcpu->arch.pause = false;
> +		wake_up_interruptible(wq);
> +	}
> +}
> +
> +
>  static void vcpu_pause(struct kvm_vcpu *vcpu)
>  {
>  	wait_queue_head_t *wq = kvm_arch_vcpu_wq(vcpu);
>  
> -	wait_event_interruptible(*wq, !vcpu->arch.power_off);
> +	wait_event_interruptible(*wq, ((!vcpu->arch.power_off) &&
> +				       (!vcpu->arch.pause)));
>  }
>  
>  static int kvm_vcpu_initialized(struct kvm_vcpu *vcpu)
> @@ -525,7 +553,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
>  
>  		update_vttbr(vcpu->kvm);
>  
> -		if (vcpu->arch.power_off)
> +		if (vcpu->arch.power_off || vcpu->arch.pause)
>  			vcpu_pause(vcpu);
>  
>  		/*
> @@ -551,7 +579,8 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
>  			run->exit_reason = KVM_EXIT_INTR;
>  		}
>  
> -		if (ret <= 0 || need_new_vmid_gen(vcpu->kvm)) {
> +		if (ret <= 0 || need_new_vmid_gen(vcpu->kvm) ||
> +		    vcpu->arch.pause) {
>  			local_irq_enable();
>  			preempt_enable();
>  			kvm_vgic_sync_hwstate(vcpu);
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index 009da6b..69e3785 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -125,6 +125,9 @@ struct kvm_vcpu_arch {
>  	/* vcpu power-off state */
>  	bool power_off;
>  
> +	/* Don't run the guest */
> +	bool pause;
> +
>  	/* IO related fields */
>  	struct kvm_decode mmio_decode;

So in the last patch, for cleanliness, maybe it would be better to
change the pause reference to a power_off reference in
kvm_arch_vcpu_ioctl_set_mpstate, and keep removing it. Or, maybe the
patch wouldn't look too bad if we just squashed this one into the
last one?

drew

[PATCH 2/2] kvm: arm/arm64: implement kvm_arm_[halt, resume]_guest

[Andrew Jones]

On Mon, Jul 06, 2015 at 02:49:56PM +0200, Eric Auger wrote:
> On halt, the guest is forced to exit and prevented from being
> re-entered. This is synchronous.
> 
> Those two operations will be needed for IRQ forwarding setting.
> 
> Signed-off-by: Eric Auger <eric.auger@linaro.org>
> 
> ---
> RFC v1 -> v2:
> - add __maybe_unused
> 
> RFC:
> - rename the function and this latter becomes static
> - remove __KVM_HAVE_ARCH_HALT_GUEST
> 
> v4 -> v5: add arm64 support
> - also defines __KVM_HAVE_ARCH_HALT_GUEST for arm64
> - add pause field
> ---
>  arch/arm/include/asm/kvm_host.h   |  3 +++
>  arch/arm/kvm/arm.c                | 35 ++++++++++++++++++++++++++++++++---
>  arch/arm64/include/asm/kvm_host.h |  3 +++
>  3 files changed, 38 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
> index 304004d..899ae27 100644
> --- a/arch/arm/include/asm/kvm_host.h
> +++ b/arch/arm/include/asm/kvm_host.h
> @@ -132,6 +132,9 @@ struct kvm_vcpu_arch {
>  	/* vcpu power-off state */
>  	bool power_off;
>  
> +	/* Don't run the guest */
> +	bool pause;
> +
>  	/* IO related fields */
>  	struct kvm_decode mmio_decode;
>  
> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
> index 7537e68..46d4ef6 100644
> --- a/arch/arm/kvm/arm.c
> +++ b/arch/arm/kvm/arm.c
> @@ -471,11 +471,39 @@ bool kvm_arch_intc_initialized(struct kvm *kvm)
>  	return vgic_initialized(kvm);
>  }
>  
> +static void kvm_arm_halt_guest(struct kvm *kvm) __maybe_unused;
> +static void kvm_arm_resume_guest(struct kvm *kvm) __maybe_unused;
> +
> +static void kvm_arm_halt_guest(struct kvm *kvm)
> +{
> +	int i;
> +	struct kvm_vcpu *vcpu;
> +
> +	kvm_for_each_vcpu(i, vcpu, kvm)
> +		vcpu->arch.pause = true;
> +	force_vm_exit(cpu_all_mask);
> +}
> +
> +static void kvm_arm_resume_guest(struct kvm *kvm)
> +{
> +	int i;
> +	struct kvm_vcpu *vcpu;
> +
> +	kvm_for_each_vcpu(i, vcpu, kvm) {
> +		wait_queue_head_t *wq = kvm_arch_vcpu_wq(vcpu);
> +
> +		vcpu->arch.pause = false;
> +		wake_up_interruptible(wq);
> +	}
> +}
> +
> +
>  static void vcpu_pause(struct kvm_vcpu *vcpu)
>  {
>  	wait_queue_head_t *wq = kvm_arch_vcpu_wq(vcpu);
>  
> -	wait_event_interruptible(*wq, !vcpu->arch.power_off);
> +	wait_event_interruptible(*wq, ((!vcpu->arch.power_off) &&
> +				       (!vcpu->arch.pause)));
>  }
>  
>  static int kvm_vcpu_initialized(struct kvm_vcpu *vcpu)
> @@ -525,7 +553,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
>  
>  		update_vttbr(vcpu->kvm);
>  
> -		if (vcpu->arch.power_off)
> +		if (vcpu->arch.power_off || vcpu->arch.pause)
>  			vcpu_pause(vcpu);
>  
>  		/*
> @@ -551,7 +579,8 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
>  			run->exit_reason = KVM_EXIT_INTR;
>  		}
>  
> -		if (ret <= 0 || need_new_vmid_gen(vcpu->kvm)) {
> +		if (ret <= 0 || need_new_vmid_gen(vcpu->kvm) ||
> +		    vcpu->arch.pause) {
>  			local_irq_enable();
>  			preempt_enable();
>  			kvm_vgic_sync_hwstate(vcpu);
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index 009da6b..69e3785 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -125,6 +125,9 @@ struct kvm_vcpu_arch {
>  	/* vcpu power-off state */
>  	bool power_off;
>  
> +	/* Don't run the guest */
> +	bool pause;
> +
>  	/* IO related fields */
>  	struct kvm_decode mmio_decode;

So in the last patch, for cleanliness, maybe it would be better to
change the pause reference to a power_off reference in
kvm_arch_vcpu_ioctl_set_mpstate, and keep removing it. Or, maybe the
patch wouldn't look too bad if we just squashed this one into the
last one?

drew

Re: [PATCH 1/2] KVM: arm: rename pause into power_off

[Eric Auger]

Hi Andrew,
On 07/07/2015 03:36 PM, Andrew Jones wrote:
> On Mon, Jul 06, 2015 at 02:49:55PM +0200, Eric Auger wrote:
>> The kvm_vcpu_arch pause field is renamed into power_off to prepare
>> for the introduction of a new pause field.
>>
>> Signed-off-by: Eric Auger <eric.auger@linaro.org>
>>
>> v4 -> v5:
>> - fix compilation issue on arm64 (add power_off field in kvm_host.h)
>> ---
>>  arch/arm/include/asm/kvm_host.h   |  4 ++--
>>  arch/arm/kvm/arm.c                | 10 +++++-----
>>  arch/arm/kvm/psci.c               | 10 +++++-----
>>  arch/arm64/include/asm/kvm_host.h |  4 ++--
>>  4 files changed, 14 insertions(+), 14 deletions(-)
>>
>> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
>> index e896d2c..304004d 100644
>> --- a/arch/arm/include/asm/kvm_host.h
>> +++ b/arch/arm/include/asm/kvm_host.h
>> @@ -129,8 +129,8 @@ struct kvm_vcpu_arch {
>>  	 * here.
>>  	 */
>>  
>> -	/* Don't run the guest on this vcpu */
>> -	bool pause;
> 
> This patch should leave pause in. kvm_arch_vcpu_ioctl_set_mpstate still
> references it, compilation is broken until the next patch comes.
Effectively that's definitively wrong.
> 
>> +	/* vcpu power-off state */
>> +	bool power_off;
>>  
>>  	/* IO related fields */
>>  	struct kvm_decode mmio_decode;
>> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
>> index bcdf799..7537e68 100644
>> --- a/arch/arm/kvm/arm.c
>> +++ b/arch/arm/kvm/arm.c
>> @@ -475,7 +475,7 @@ static void vcpu_pause(struct kvm_vcpu *vcpu)
>>  {
>>  	wait_queue_head_t *wq = kvm_arch_vcpu_wq(vcpu);
>>  
>> -	wait_event_interruptible(*wq, !vcpu->arch.pause);
>> +	wait_event_interruptible(*wq, !vcpu->arch.power_off);
> 
> Leaving pause in would allow this to already be
> 
> +       wait_event_interruptible(*wq, ((!vcpu->arch.power_off) &&
> +                                      (!vcpu->arch.pause)));
> 
> which feels better. The function name is vcpu_pause, so it's nice
> to see state named 'pause' in there.
agreed

Thanks for the review!

Eric
> 
>>  }
>>  
>>  static int kvm_vcpu_initialized(struct kvm_vcpu *vcpu)
>> @@ -525,7 +525,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
>>  
>>  		update_vttbr(vcpu->kvm);
>>  
>> -		if (vcpu->arch.pause)
>> +		if (vcpu->arch.power_off)
>>  			vcpu_pause(vcpu);
>>  
>>  		/*
>> @@ -766,12 +766,12 @@ static int kvm_arch_vcpu_ioctl_vcpu_init(struct kvm_vcpu *vcpu,
>>  	vcpu_reset_hcr(vcpu);
>>  
>>  	/*
>> -	 * Handle the "start in power-off" case by marking the VCPU as paused.
>> +	 * Handle the "start in power-off" case.
>>  	 */
>>  	if (test_bit(KVM_ARM_VCPU_POWER_OFF, vcpu->arch.features))
>> -		vcpu->arch.pause = true;
>> +		vcpu->arch.power_off = true;
>>  	else
>> -		vcpu->arch.pause = false;
>> +		vcpu->arch.power_off = false;
>>  
>>  	return 0;
>>  }
>> diff --git a/arch/arm/kvm/psci.c b/arch/arm/kvm/psci.c
>> index 4b94b51..134971a 100644
>> --- a/arch/arm/kvm/psci.c
>> +++ b/arch/arm/kvm/psci.c
>> @@ -63,7 +63,7 @@ static unsigned long kvm_psci_vcpu_suspend(struct kvm_vcpu *vcpu)
>>  
>>  static void kvm_psci_vcpu_off(struct kvm_vcpu *vcpu)
>>  {
>> -	vcpu->arch.pause = true;
>> +	vcpu->arch.power_off = true;
>>  }
>>  
>>  static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu)
>> @@ -87,7 +87,7 @@ static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu)
>>  	 */
>>  	if (!vcpu)
>>  		return PSCI_RET_INVALID_PARAMS;
>> -	if (!vcpu->arch.pause) {
>> +	if (!vcpu->arch.power_off) {
>>  		if (kvm_psci_version(source_vcpu) != KVM_ARM_PSCI_0_1)
>>  			return PSCI_RET_ALREADY_ON;
>>  		else
>> @@ -115,7 +115,7 @@ static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu)
>>  	 * the general puspose registers are undefined upon CPU_ON.
>>  	 */
>>  	*vcpu_reg(vcpu, 0) = context_id;
>> -	vcpu->arch.pause = false;
>> +	vcpu->arch.power_off = false;
>>  	smp_mb();		/* Make sure the above is visible */
>>  
>>  	wq = kvm_arch_vcpu_wq(vcpu);
>> @@ -152,7 +152,7 @@ static unsigned long kvm_psci_vcpu_affinity_info(struct kvm_vcpu *vcpu)
>>  	kvm_for_each_vcpu(i, tmp, kvm) {
>>  		mpidr = kvm_vcpu_get_mpidr_aff(tmp);
>>  		if (((mpidr & target_affinity_mask) == target_affinity) &&
>> -		    !tmp->arch.pause) {
>> +		    !tmp->arch.power_off) {
>>  			return PSCI_0_2_AFFINITY_LEVEL_ON;
>>  		}
>>  	}
>> @@ -175,7 +175,7 @@ static void kvm_prepare_system_event(struct kvm_vcpu *vcpu, u32 type)
>>  	 * re-initialized.
>>  	 */
>>  	kvm_for_each_vcpu(i, tmp, vcpu->kvm) {
>> -		tmp->arch.pause = true;
>> +		tmp->arch.power_off = true;
>>  		kvm_vcpu_kick(tmp);
>>  	}
>>  
>> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
>> index 2709db2..009da6b 100644
>> --- a/arch/arm64/include/asm/kvm_host.h
>> +++ b/arch/arm64/include/asm/kvm_host.h
>> @@ -122,8 +122,8 @@ struct kvm_vcpu_arch {
>>  	 * here.
>>  	 */
>>  
>> -	/* Don't run the guest */
>> -	bool pause;
>> +	/* vcpu power-off state */
>> +	bool power_off;
>>  
>>  	/* IO related fields */
>>  	struct kvm_decode mmio_decode;
>> -- 
>> 1.9.1
> 
> Thanks,
> drew
> 

[PATCH 1/2] KVM: arm: rename pause into power_off

[Eric Auger]

Hi Andrew,
On 07/07/2015 03:36 PM, Andrew Jones wrote:
> On Mon, Jul 06, 2015 at 02:49:55PM +0200, Eric Auger wrote:
>> The kvm_vcpu_arch pause field is renamed into power_off to prepare
>> for the introduction of a new pause field.
>>
>> Signed-off-by: Eric Auger <eric.auger@linaro.org>
>>
>> v4 -> v5:
>> - fix compilation issue on arm64 (add power_off field in kvm_host.h)
>> ---
>>  arch/arm/include/asm/kvm_host.h   |  4 ++--
>>  arch/arm/kvm/arm.c                | 10 +++++-----
>>  arch/arm/kvm/psci.c               | 10 +++++-----
>>  arch/arm64/include/asm/kvm_host.h |  4 ++--
>>  4 files changed, 14 insertions(+), 14 deletions(-)
>>
>> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
>> index e896d2c..304004d 100644
>> --- a/arch/arm/include/asm/kvm_host.h
>> +++ b/arch/arm/include/asm/kvm_host.h
>> @@ -129,8 +129,8 @@ struct kvm_vcpu_arch {
>>  	 * here.
>>  	 */
>>  
>> -	/* Don't run the guest on this vcpu */
>> -	bool pause;
> 
> This patch should leave pause in. kvm_arch_vcpu_ioctl_set_mpstate still
> references it, compilation is broken until the next patch comes.
Effectively that's definitively wrong.
> 
>> +	/* vcpu power-off state */
>> +	bool power_off;
>>  
>>  	/* IO related fields */
>>  	struct kvm_decode mmio_decode;
>> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
>> index bcdf799..7537e68 100644
>> --- a/arch/arm/kvm/arm.c
>> +++ b/arch/arm/kvm/arm.c
>> @@ -475,7 +475,7 @@ static void vcpu_pause(struct kvm_vcpu *vcpu)
>>  {
>>  	wait_queue_head_t *wq = kvm_arch_vcpu_wq(vcpu);
>>  
>> -	wait_event_interruptible(*wq, !vcpu->arch.pause);
>> +	wait_event_interruptible(*wq, !vcpu->arch.power_off);
> 
> Leaving pause in would allow this to already be
> 
> +       wait_event_interruptible(*wq, ((!vcpu->arch.power_off) &&
> +                                      (!vcpu->arch.pause)));
> 
> which feels better. The function name is vcpu_pause, so it's nice
> to see state named 'pause' in there.
agreed

Thanks for the review!

Eric
> 
>>  }
>>  
>>  static int kvm_vcpu_initialized(struct kvm_vcpu *vcpu)
>> @@ -525,7 +525,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
>>  
>>  		update_vttbr(vcpu->kvm);
>>  
>> -		if (vcpu->arch.pause)
>> +		if (vcpu->arch.power_off)
>>  			vcpu_pause(vcpu);
>>  
>>  		/*
>> @@ -766,12 +766,12 @@ static int kvm_arch_vcpu_ioctl_vcpu_init(struct kvm_vcpu *vcpu,
>>  	vcpu_reset_hcr(vcpu);
>>  
>>  	/*
>> -	 * Handle the "start in power-off" case by marking the VCPU as paused.
>> +	 * Handle the "start in power-off" case.
>>  	 */
>>  	if (test_bit(KVM_ARM_VCPU_POWER_OFF, vcpu->arch.features))
>> -		vcpu->arch.pause = true;
>> +		vcpu->arch.power_off = true;
>>  	else
>> -		vcpu->arch.pause = false;
>> +		vcpu->arch.power_off = false;
>>  
>>  	return 0;
>>  }
>> diff --git a/arch/arm/kvm/psci.c b/arch/arm/kvm/psci.c
>> index 4b94b51..134971a 100644
>> --- a/arch/arm/kvm/psci.c
>> +++ b/arch/arm/kvm/psci.c
>> @@ -63,7 +63,7 @@ static unsigned long kvm_psci_vcpu_suspend(struct kvm_vcpu *vcpu)
>>  
>>  static void kvm_psci_vcpu_off(struct kvm_vcpu *vcpu)
>>  {
>> -	vcpu->arch.pause = true;
>> +	vcpu->arch.power_off = true;
>>  }
>>  
>>  static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu)
>> @@ -87,7 +87,7 @@ static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu)
>>  	 */
>>  	if (!vcpu)
>>  		return PSCI_RET_INVALID_PARAMS;
>> -	if (!vcpu->arch.pause) {
>> +	if (!vcpu->arch.power_off) {
>>  		if (kvm_psci_version(source_vcpu) != KVM_ARM_PSCI_0_1)
>>  			return PSCI_RET_ALREADY_ON;
>>  		else
>> @@ -115,7 +115,7 @@ static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu)
>>  	 * the general puspose registers are undefined upon CPU_ON.
>>  	 */
>>  	*vcpu_reg(vcpu, 0) = context_id;
>> -	vcpu->arch.pause = false;
>> +	vcpu->arch.power_off = false;
>>  	smp_mb();		/* Make sure the above is visible */
>>  
>>  	wq = kvm_arch_vcpu_wq(vcpu);
>> @@ -152,7 +152,7 @@ static unsigned long kvm_psci_vcpu_affinity_info(struct kvm_vcpu *vcpu)
>>  	kvm_for_each_vcpu(i, tmp, kvm) {
>>  		mpidr = kvm_vcpu_get_mpidr_aff(tmp);
>>  		if (((mpidr & target_affinity_mask) == target_affinity) &&
>> -		    !tmp->arch.pause) {
>> +		    !tmp->arch.power_off) {
>>  			return PSCI_0_2_AFFINITY_LEVEL_ON;
>>  		}
>>  	}
>> @@ -175,7 +175,7 @@ static void kvm_prepare_system_event(struct kvm_vcpu *vcpu, u32 type)
>>  	 * re-initialized.
>>  	 */
>>  	kvm_for_each_vcpu(i, tmp, vcpu->kvm) {
>> -		tmp->arch.pause = true;
>> +		tmp->arch.power_off = true;
>>  		kvm_vcpu_kick(tmp);
>>  	}
>>  
>> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
>> index 2709db2..009da6b 100644
>> --- a/arch/arm64/include/asm/kvm_host.h
>> +++ b/arch/arm64/include/asm/kvm_host.h
>> @@ -122,8 +122,8 @@ struct kvm_vcpu_arch {
>>  	 * here.
>>  	 */
>>  
>> -	/* Don't run the guest */
>> -	bool pause;
>> +	/* vcpu power-off state */
>> +	bool power_off;
>>  
>>  	/* IO related fields */
>>  	struct kvm_decode mmio_decode;
>> -- 
>> 1.9.1
> 
> Thanks,
> drew
> 

Re: [PATCH 2/2] kvm: arm/arm64: implement kvm_arm_[halt, resume]_guest

[Eric Auger]

On 07/07/2015 03:41 PM, Andrew Jones wrote:
> On Mon, Jul 06, 2015 at 02:49:56PM +0200, Eric Auger wrote:
>> On halt, the guest is forced to exit and prevented from being
>> re-entered. This is synchronous.
>>
>> Those two operations will be needed for IRQ forwarding setting.
>>
>> Signed-off-by: Eric Auger <eric.auger@linaro.org>
>>
>> ---
>> RFC v1 -> v2:
>> - add __maybe_unused
>>
>> RFC:
>> - rename the function and this latter becomes static
>> - remove __KVM_HAVE_ARCH_HALT_GUEST
>>
>> v4 -> v5: add arm64 support
>> - also defines __KVM_HAVE_ARCH_HALT_GUEST for arm64
>> - add pause field
>> ---
>>  arch/arm/include/asm/kvm_host.h   |  3 +++
>>  arch/arm/kvm/arm.c                | 35 ++++++++++++++++++++++++++++++++---
>>  arch/arm64/include/asm/kvm_host.h |  3 +++
>>  3 files changed, 38 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
>> index 304004d..899ae27 100644
>> --- a/arch/arm/include/asm/kvm_host.h
>> +++ b/arch/arm/include/asm/kvm_host.h
>> @@ -132,6 +132,9 @@ struct kvm_vcpu_arch {
>>  	/* vcpu power-off state */
>>  	bool power_off;
>>  
>> +	/* Don't run the guest */
>> +	bool pause;
>> +
>>  	/* IO related fields */
>>  	struct kvm_decode mmio_decode;
>>  
>> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
>> index 7537e68..46d4ef6 100644
>> --- a/arch/arm/kvm/arm.c
>> +++ b/arch/arm/kvm/arm.c
>> @@ -471,11 +471,39 @@ bool kvm_arch_intc_initialized(struct kvm *kvm)
>>  	return vgic_initialized(kvm);
>>  }
>>  
>> +static void kvm_arm_halt_guest(struct kvm *kvm) __maybe_unused;
>> +static void kvm_arm_resume_guest(struct kvm *kvm) __maybe_unused;
>> +
>> +static void kvm_arm_halt_guest(struct kvm *kvm)
>> +{
>> +	int i;
>> +	struct kvm_vcpu *vcpu;
>> +
>> +	kvm_for_each_vcpu(i, vcpu, kvm)
>> +		vcpu->arch.pause = true;
>> +	force_vm_exit(cpu_all_mask);
>> +}
>> +
>> +static void kvm_arm_resume_guest(struct kvm *kvm)
>> +{
>> +	int i;
>> +	struct kvm_vcpu *vcpu;
>> +
>> +	kvm_for_each_vcpu(i, vcpu, kvm) {
>> +		wait_queue_head_t *wq = kvm_arch_vcpu_wq(vcpu);
>> +
>> +		vcpu->arch.pause = false;
>> +		wake_up_interruptible(wq);
>> +	}
>> +}
>> +
>> +
>>  static void vcpu_pause(struct kvm_vcpu *vcpu)
>>  {
>>  	wait_queue_head_t *wq = kvm_arch_vcpu_wq(vcpu);
>>  
>> -	wait_event_interruptible(*wq, !vcpu->arch.power_off);
>> +	wait_event_interruptible(*wq, ((!vcpu->arch.power_off) &&
>> +				       (!vcpu->arch.pause)));
>>  }
>>  
>>  static int kvm_vcpu_initialized(struct kvm_vcpu *vcpu)
>> @@ -525,7 +553,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
>>  
>>  		update_vttbr(vcpu->kvm);
>>  
>> -		if (vcpu->arch.power_off)
>> +		if (vcpu->arch.power_off || vcpu->arch.pause)
>>  			vcpu_pause(vcpu);
>>  
>>  		/*
>> @@ -551,7 +579,8 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
>>  			run->exit_reason = KVM_EXIT_INTR;
>>  		}
>>  
>> -		if (ret <= 0 || need_new_vmid_gen(vcpu->kvm)) {
>> +		if (ret <= 0 || need_new_vmid_gen(vcpu->kvm) ||
>> +		    vcpu->arch.pause) {
>>  			local_irq_enable();
>>  			preempt_enable();
>>  			kvm_vgic_sync_hwstate(vcpu);
>> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
>> index 009da6b..69e3785 100644
>> --- a/arch/arm64/include/asm/kvm_host.h
>> +++ b/arch/arm64/include/asm/kvm_host.h
>> @@ -125,6 +125,9 @@ struct kvm_vcpu_arch {
>>  	/* vcpu power-off state */
>>  	bool power_off;
>>  
>> +	/* Don't run the guest */
>> +	bool pause;
>> +
>>  	/* IO related fields */
>>  	struct kvm_decode mmio_decode;
> 
> So in the last patch, for cleanliness, maybe it would be better to
> change the pause reference to a power_off reference in
> kvm_arch_vcpu_ioctl_set_mpstate, and keep removing it. Or, maybe the
> patch wouldn't look too bad if we just squashed this one into the
> last one?
yeah I need to think again about the patch structure and/or maybe adopt
the lazy approach, putting both together.

Thanks again

Eric
> 
> drew
> 

[PATCH 2/2] kvm: arm/arm64: implement kvm_arm_[halt, resume]_guest

[Eric Auger]

On 07/07/2015 03:41 PM, Andrew Jones wrote:
> On Mon, Jul 06, 2015 at 02:49:56PM +0200, Eric Auger wrote:
>> On halt, the guest is forced to exit and prevented from being
>> re-entered. This is synchronous.
>>
>> Those two operations will be needed for IRQ forwarding setting.
>>
>> Signed-off-by: Eric Auger <eric.auger@linaro.org>
>>
>> ---
>> RFC v1 -> v2:
>> - add __maybe_unused
>>
>> RFC:
>> - rename the function and this latter becomes static
>> - remove __KVM_HAVE_ARCH_HALT_GUEST
>>
>> v4 -> v5: add arm64 support
>> - also defines __KVM_HAVE_ARCH_HALT_GUEST for arm64
>> - add pause field
>> ---
>>  arch/arm/include/asm/kvm_host.h   |  3 +++
>>  arch/arm/kvm/arm.c                | 35 ++++++++++++++++++++++++++++++++---
>>  arch/arm64/include/asm/kvm_host.h |  3 +++
>>  3 files changed, 38 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
>> index 304004d..899ae27 100644
>> --- a/arch/arm/include/asm/kvm_host.h
>> +++ b/arch/arm/include/asm/kvm_host.h
>> @@ -132,6 +132,9 @@ struct kvm_vcpu_arch {
>>  	/* vcpu power-off state */
>>  	bool power_off;
>>  
>> +	/* Don't run the guest */
>> +	bool pause;
>> +
>>  	/* IO related fields */
>>  	struct kvm_decode mmio_decode;
>>  
>> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
>> index 7537e68..46d4ef6 100644
>> --- a/arch/arm/kvm/arm.c
>> +++ b/arch/arm/kvm/arm.c
>> @@ -471,11 +471,39 @@ bool kvm_arch_intc_initialized(struct kvm *kvm)
>>  	return vgic_initialized(kvm);
>>  }
>>  
>> +static void kvm_arm_halt_guest(struct kvm *kvm) __maybe_unused;
>> +static void kvm_arm_resume_guest(struct kvm *kvm) __maybe_unused;
>> +
>> +static void kvm_arm_halt_guest(struct kvm *kvm)
>> +{
>> +	int i;
>> +	struct kvm_vcpu *vcpu;
>> +
>> +	kvm_for_each_vcpu(i, vcpu, kvm)
>> +		vcpu->arch.pause = true;
>> +	force_vm_exit(cpu_all_mask);
>> +}
>> +
>> +static void kvm_arm_resume_guest(struct kvm *kvm)
>> +{
>> +	int i;
>> +	struct kvm_vcpu *vcpu;
>> +
>> +	kvm_for_each_vcpu(i, vcpu, kvm) {
>> +		wait_queue_head_t *wq = kvm_arch_vcpu_wq(vcpu);
>> +
>> +		vcpu->arch.pause = false;
>> +		wake_up_interruptible(wq);
>> +	}
>> +}
>> +
>> +
>>  static void vcpu_pause(struct kvm_vcpu *vcpu)
>>  {
>>  	wait_queue_head_t *wq = kvm_arch_vcpu_wq(vcpu);
>>  
>> -	wait_event_interruptible(*wq, !vcpu->arch.power_off);
>> +	wait_event_interruptible(*wq, ((!vcpu->arch.power_off) &&
>> +				       (!vcpu->arch.pause)));
>>  }
>>  
>>  static int kvm_vcpu_initialized(struct kvm_vcpu *vcpu)
>> @@ -525,7 +553,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
>>  
>>  		update_vttbr(vcpu->kvm);
>>  
>> -		if (vcpu->arch.power_off)
>> +		if (vcpu->arch.power_off || vcpu->arch.pause)
>>  			vcpu_pause(vcpu);
>>  
>>  		/*
>> @@ -551,7 +579,8 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
>>  			run->exit_reason = KVM_EXIT_INTR;
>>  		}
>>  
>> -		if (ret <= 0 || need_new_vmid_gen(vcpu->kvm)) {
>> +		if (ret <= 0 || need_new_vmid_gen(vcpu->kvm) ||
>> +		    vcpu->arch.pause) {
>>  			local_irq_enable();
>>  			preempt_enable();
>>  			kvm_vgic_sync_hwstate(vcpu);
>> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
>> index 009da6b..69e3785 100644
>> --- a/arch/arm64/include/asm/kvm_host.h
>> +++ b/arch/arm64/include/asm/kvm_host.h
>> @@ -125,6 +125,9 @@ struct kvm_vcpu_arch {
>>  	/* vcpu power-off state */
>>  	bool power_off;
>>  
>> +	/* Don't run the guest */
>> +	bool pause;
>> +
>>  	/* IO related fields */
>>  	struct kvm_decode mmio_decode;
> 
> So in the last patch, for cleanliness, maybe it would be better to
> change the pause reference to a power_off reference in
> kvm_arch_vcpu_ioctl_set_mpstate, and keep removing it. Or, maybe the
> patch wouldn't look too bad if we just squashed this one into the
> last one?
yeah I need to think again about the patch structure and/or maybe adopt
the lazy approach, putting both together.

Thanks again

Eric
> 
> drew
> 

Re: [PATCH 1/2] KVM: arm: rename pause into power_off

[Christoffer Dall]

On Mon, Jul 06, 2015 at 02:49:55PM +0200, Eric Auger wrote:
> The kvm_vcpu_arch pause field is renamed into power_off to prepare
> for the introduction of a new pause field.
> 
> Signed-off-by: Eric Auger <eric.auger@linaro.org>
> 
> v4 -> v5:
> - fix compilation issue on arm64 (add power_off field in kvm_host.h)
> ---
>  arch/arm/include/asm/kvm_host.h   |  4 ++--
>  arch/arm/kvm/arm.c                | 10 +++++-----
>  arch/arm/kvm/psci.c               | 10 +++++-----
>  arch/arm64/include/asm/kvm_host.h |  4 ++--
>  4 files changed, 14 insertions(+), 14 deletions(-)
> 
> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
> index e896d2c..304004d 100644
> --- a/arch/arm/include/asm/kvm_host.h
> +++ b/arch/arm/include/asm/kvm_host.h
> @@ -129,8 +129,8 @@ struct kvm_vcpu_arch {
>  	 * here.
>  	 */
>  
> -	/* Don't run the guest on this vcpu */
> -	bool pause;
> +	/* vcpu power-off state */
> +	bool power_off;
>  
>  	/* IO related fields */
>  	struct kvm_decode mmio_decode;
> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
> index bcdf799..7537e68 100644
> --- a/arch/arm/kvm/arm.c
> +++ b/arch/arm/kvm/arm.c
> @@ -475,7 +475,7 @@ static void vcpu_pause(struct kvm_vcpu *vcpu)
>  {
>  	wait_queue_head_t *wq = kvm_arch_vcpu_wq(vcpu);
>  
> -	wait_event_interruptible(*wq, !vcpu->arch.pause);
> +	wait_event_interruptible(*wq, !vcpu->arch.power_off);

would there be any benefit to simply calling kvm_vcpu_block() instead of
vcpu_pause, and rewrite kvm_arch_vcpu_runnable to:

int kvm_arch_vcpu_runnable(struct kvm_vcpu *v)
{
▸       return !vcpu->arch.power_off &&
		(!!v->arch.irq_lines || kvm_vgic_vcpu_pending_irq(v));
}

Not sure really, certainly the runnable function does not become more
readable.

>  }
>  
>  static int kvm_vcpu_initialized(struct kvm_vcpu *vcpu)
> @@ -525,7 +525,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
>  
>  		update_vttbr(vcpu->kvm);
>  
> -		if (vcpu->arch.pause)
> +		if (vcpu->arch.power_off)
>  			vcpu_pause(vcpu);

looking back over this code, how does this actually guarantee that we
don't run a powered-off cpu?

vcpu_pause() just does a wait_event_interruptible(), so if we get
scheduled again, we'll just proceed running.  Is there any case where we
could get scheduled without signal_pending() being true and therefore
inadvertedly run the vcpu?

if so, we should change the line below like this:

diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
index bc738d2..98f31e6 100644
--- a/arch/arm/kvm/arm.c
+++ b/arch/arm/kvm/arm.c
@@ -542,7 +542,8 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
 			run->exit_reason = KVM_EXIT_INTR;
 		}
 
-		if (ret <= 0 || need_new_vmid_gen(vcpu->kvm)) {
+		if (ret <= 0 || need_new_vmid_gen(vcpu->kvm) ||
+		    vcpu->arch.power_off) {
 			local_irq_enable();
 			preempt_enable();
 			kvm_timer_sync_hwstate(vcpu);


Sorry for polluting your patch with these questions, I'm otherwise fine
with the rename.

Thanks,
-Christoffer

>  
>  		/*
> @@ -766,12 +766,12 @@ static int kvm_arch_vcpu_ioctl_vcpu_init(struct kvm_vcpu *vcpu,
>  	vcpu_reset_hcr(vcpu);
>  
>  	/*
> -	 * Handle the "start in power-off" case by marking the VCPU as paused.
> +	 * Handle the "start in power-off" case.
>  	 */
>  	if (test_bit(KVM_ARM_VCPU_POWER_OFF, vcpu->arch.features))
> -		vcpu->arch.pause = true;
> +		vcpu->arch.power_off = true;
>  	else
> -		vcpu->arch.pause = false;
> +		vcpu->arch.power_off = false;
>  
>  	return 0;
>  }
> diff --git a/arch/arm/kvm/psci.c b/arch/arm/kvm/psci.c
> index 4b94b51..134971a 100644
> --- a/arch/arm/kvm/psci.c
> +++ b/arch/arm/kvm/psci.c
> @@ -63,7 +63,7 @@ static unsigned long kvm_psci_vcpu_suspend(struct kvm_vcpu *vcpu)
>  
>  static void kvm_psci_vcpu_off(struct kvm_vcpu *vcpu)
>  {
> -	vcpu->arch.pause = true;
> +	vcpu->arch.power_off = true;
>  }
>  
>  static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu)
> @@ -87,7 +87,7 @@ static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu)
>  	 */
>  	if (!vcpu)
>  		return PSCI_RET_INVALID_PARAMS;
> -	if (!vcpu->arch.pause) {
> +	if (!vcpu->arch.power_off) {
>  		if (kvm_psci_version(source_vcpu) != KVM_ARM_PSCI_0_1)
>  			return PSCI_RET_ALREADY_ON;
>  		else
> @@ -115,7 +115,7 @@ static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu)
>  	 * the general puspose registers are undefined upon CPU_ON.
>  	 */
>  	*vcpu_reg(vcpu, 0) = context_id;
> -	vcpu->arch.pause = false;
> +	vcpu->arch.power_off = false;
>  	smp_mb();		/* Make sure the above is visible */
>  
>  	wq = kvm_arch_vcpu_wq(vcpu);
> @@ -152,7 +152,7 @@ static unsigned long kvm_psci_vcpu_affinity_info(struct kvm_vcpu *vcpu)
>  	kvm_for_each_vcpu(i, tmp, kvm) {
>  		mpidr = kvm_vcpu_get_mpidr_aff(tmp);
>  		if (((mpidr & target_affinity_mask) == target_affinity) &&
> -		    !tmp->arch.pause) {
> +		    !tmp->arch.power_off) {
>  			return PSCI_0_2_AFFINITY_LEVEL_ON;
>  		}
>  	}
> @@ -175,7 +175,7 @@ static void kvm_prepare_system_event(struct kvm_vcpu *vcpu, u32 type)
>  	 * re-initialized.
>  	 */
>  	kvm_for_each_vcpu(i, tmp, vcpu->kvm) {
> -		tmp->arch.pause = true;
> +		tmp->arch.power_off = true;
>  		kvm_vcpu_kick(tmp);
>  	}
>  
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index 2709db2..009da6b 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -122,8 +122,8 @@ struct kvm_vcpu_arch {
>  	 * here.
>  	 */
>  
> -	/* Don't run the guest */
> -	bool pause;
> +	/* vcpu power-off state */
> +	bool power_off;
>  
>  	/* IO related fields */
>  	struct kvm_decode mmio_decode;
> -- 
> 1.9.1
> 
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH 1/2] KVM: arm: rename pause into power_off

[Christoffer Dall]

On Mon, Jul 06, 2015 at 02:49:55PM +0200, Eric Auger wrote:
> The kvm_vcpu_arch pause field is renamed into power_off to prepare
> for the introduction of a new pause field.
> 
> Signed-off-by: Eric Auger <eric.auger@linaro.org>
> 
> v4 -> v5:
> - fix compilation issue on arm64 (add power_off field in kvm_host.h)
> ---
>  arch/arm/include/asm/kvm_host.h   |  4 ++--
>  arch/arm/kvm/arm.c                | 10 +++++-----
>  arch/arm/kvm/psci.c               | 10 +++++-----
>  arch/arm64/include/asm/kvm_host.h |  4 ++--
>  4 files changed, 14 insertions(+), 14 deletions(-)
> 
> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
> index e896d2c..304004d 100644
> --- a/arch/arm/include/asm/kvm_host.h
> +++ b/arch/arm/include/asm/kvm_host.h
> @@ -129,8 +129,8 @@ struct kvm_vcpu_arch {
>  	 * here.
>  	 */
>  
> -	/* Don't run the guest on this vcpu */
> -	bool pause;
> +	/* vcpu power-off state */
> +	bool power_off;
>  
>  	/* IO related fields */
>  	struct kvm_decode mmio_decode;
> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
> index bcdf799..7537e68 100644
> --- a/arch/arm/kvm/arm.c
> +++ b/arch/arm/kvm/arm.c
> @@ -475,7 +475,7 @@ static void vcpu_pause(struct kvm_vcpu *vcpu)
>  {
>  	wait_queue_head_t *wq = kvm_arch_vcpu_wq(vcpu);
>  
> -	wait_event_interruptible(*wq, !vcpu->arch.pause);
> +	wait_event_interruptible(*wq, !vcpu->arch.power_off);

would there be any benefit to simply calling kvm_vcpu_block() instead of
vcpu_pause, and rewrite kvm_arch_vcpu_runnable to:

int kvm_arch_vcpu_runnable(struct kvm_vcpu *v)
{
?       return !vcpu->arch.power_off &&
		(!!v->arch.irq_lines || kvm_vgic_vcpu_pending_irq(v));
}

Not sure really, certainly the runnable function does not become more
readable.

>  }
>  
>  static int kvm_vcpu_initialized(struct kvm_vcpu *vcpu)
> @@ -525,7 +525,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
>  
>  		update_vttbr(vcpu->kvm);
>  
> -		if (vcpu->arch.pause)
> +		if (vcpu->arch.power_off)
>  			vcpu_pause(vcpu);

looking back over this code, how does this actually guarantee that we
don't run a powered-off cpu?

vcpu_pause() just does a wait_event_interruptible(), so if we get
scheduled again, we'll just proceed running.  Is there any case where we
could get scheduled without signal_pending() being true and therefore
inadvertedly run the vcpu?

if so, we should change the line below like this:

diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
index bc738d2..98f31e6 100644
--- a/arch/arm/kvm/arm.c
+++ b/arch/arm/kvm/arm.c
@@ -542,7 +542,8 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
 			run->exit_reason = KVM_EXIT_INTR;
 		}
 
-		if (ret <= 0 || need_new_vmid_gen(vcpu->kvm)) {
+		if (ret <= 0 || need_new_vmid_gen(vcpu->kvm) ||
+		    vcpu->arch.power_off) {
 			local_irq_enable();
 			preempt_enable();
 			kvm_timer_sync_hwstate(vcpu);


Sorry for polluting your patch with these questions, I'm otherwise fine
with the rename.

Thanks,
-Christoffer

>  
>  		/*
> @@ -766,12 +766,12 @@ static int kvm_arch_vcpu_ioctl_vcpu_init(struct kvm_vcpu *vcpu,
>  	vcpu_reset_hcr(vcpu);
>  
>  	/*
> -	 * Handle the "start in power-off" case by marking the VCPU as paused.
> +	 * Handle the "start in power-off" case.
>  	 */
>  	if (test_bit(KVM_ARM_VCPU_POWER_OFF, vcpu->arch.features))
> -		vcpu->arch.pause = true;
> +		vcpu->arch.power_off = true;
>  	else
> -		vcpu->arch.pause = false;
> +		vcpu->arch.power_off = false;
>  
>  	return 0;
>  }
> diff --git a/arch/arm/kvm/psci.c b/arch/arm/kvm/psci.c
> index 4b94b51..134971a 100644
> --- a/arch/arm/kvm/psci.c
> +++ b/arch/arm/kvm/psci.c
> @@ -63,7 +63,7 @@ static unsigned long kvm_psci_vcpu_suspend(struct kvm_vcpu *vcpu)
>  
>  static void kvm_psci_vcpu_off(struct kvm_vcpu *vcpu)
>  {
> -	vcpu->arch.pause = true;
> +	vcpu->arch.power_off = true;
>  }
>  
>  static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu)
> @@ -87,7 +87,7 @@ static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu)
>  	 */
>  	if (!vcpu)
>  		return PSCI_RET_INVALID_PARAMS;
> -	if (!vcpu->arch.pause) {
> +	if (!vcpu->arch.power_off) {
>  		if (kvm_psci_version(source_vcpu) != KVM_ARM_PSCI_0_1)
>  			return PSCI_RET_ALREADY_ON;
>  		else
> @@ -115,7 +115,7 @@ static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu)
>  	 * the general puspose registers are undefined upon CPU_ON.
>  	 */
>  	*vcpu_reg(vcpu, 0) = context_id;
> -	vcpu->arch.pause = false;
> +	vcpu->arch.power_off = false;
>  	smp_mb();		/* Make sure the above is visible */
>  
>  	wq = kvm_arch_vcpu_wq(vcpu);
> @@ -152,7 +152,7 @@ static unsigned long kvm_psci_vcpu_affinity_info(struct kvm_vcpu *vcpu)
>  	kvm_for_each_vcpu(i, tmp, kvm) {
>  		mpidr = kvm_vcpu_get_mpidr_aff(tmp);
>  		if (((mpidr & target_affinity_mask) == target_affinity) &&
> -		    !tmp->arch.pause) {
> +		    !tmp->arch.power_off) {
>  			return PSCI_0_2_AFFINITY_LEVEL_ON;
>  		}
>  	}
> @@ -175,7 +175,7 @@ static void kvm_prepare_system_event(struct kvm_vcpu *vcpu, u32 type)
>  	 * re-initialized.
>  	 */
>  	kvm_for_each_vcpu(i, tmp, vcpu->kvm) {
> -		tmp->arch.pause = true;
> +		tmp->arch.power_off = true;
>  		kvm_vcpu_kick(tmp);
>  	}
>  
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index 2709db2..009da6b 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -122,8 +122,8 @@ struct kvm_vcpu_arch {
>  	 * here.
>  	 */
>  
> -	/* Don't run the guest */
> -	bool pause;
> +	/* vcpu power-off state */
> +	bool power_off;
>  
>  	/* IO related fields */
>  	struct kvm_decode mmio_decode;
> -- 
> 1.9.1
> 

Re: [PATCH 2/2] kvm: arm/arm64: implement kvm_arm_[halt, resume]_guest

[Christoffer Dall]

On Mon, Jul 06, 2015 at 02:49:56PM +0200, Eric Auger wrote:
> On halt, the guest is forced to exit and prevented from being
> re-entered. This is synchronous.
> 
> Those two operations will be needed for IRQ forwarding setting.
> 
> Signed-off-by: Eric Auger <eric.auger@linaro.org>
> 
> ---
> RFC v1 -> v2:
> - add __maybe_unused
> 
> RFC:
> - rename the function and this latter becomes static
> - remove __KVM_HAVE_ARCH_HALT_GUEST
> 
> v4 -> v5: add arm64 support
> - also defines __KVM_HAVE_ARCH_HALT_GUEST for arm64
> - add pause field
> ---
>  arch/arm/include/asm/kvm_host.h   |  3 +++
>  arch/arm/kvm/arm.c                | 35 ++++++++++++++++++++++++++++++++---
>  arch/arm64/include/asm/kvm_host.h |  3 +++
>  3 files changed, 38 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
> index 304004d..899ae27 100644
> --- a/arch/arm/include/asm/kvm_host.h
> +++ b/arch/arm/include/asm/kvm_host.h
> @@ -132,6 +132,9 @@ struct kvm_vcpu_arch {
>  	/* vcpu power-off state */
>  	bool power_off;
>  
> +	/* Don't run the guest */
> +	bool pause;
> +
>  	/* IO related fields */
>  	struct kvm_decode mmio_decode;
>  
> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
> index 7537e68..46d4ef6 100644
> --- a/arch/arm/kvm/arm.c
> +++ b/arch/arm/kvm/arm.c
> @@ -471,11 +471,39 @@ bool kvm_arch_intc_initialized(struct kvm *kvm)
>  	return vgic_initialized(kvm);
>  }
>  
> +static void kvm_arm_halt_guest(struct kvm *kvm) __maybe_unused;
> +static void kvm_arm_resume_guest(struct kvm *kvm) __maybe_unused;
> +
> +static void kvm_arm_halt_guest(struct kvm *kvm)
> +{
> +	int i;
> +	struct kvm_vcpu *vcpu;
> +
> +	kvm_for_each_vcpu(i, vcpu, kvm)
> +		vcpu->arch.pause = true;
> +	force_vm_exit(cpu_all_mask);
> +}
> +
> +static void kvm_arm_resume_guest(struct kvm *kvm)
> +{
> +	int i;
> +	struct kvm_vcpu *vcpu;
> +
> +	kvm_for_each_vcpu(i, vcpu, kvm) {
> +		wait_queue_head_t *wq = kvm_arch_vcpu_wq(vcpu);
> +
> +		vcpu->arch.pause = false;
> +		wake_up_interruptible(wq);
> +	}
> +}
> +
> +
>  static void vcpu_pause(struct kvm_vcpu *vcpu)
>  {
>  	wait_queue_head_t *wq = kvm_arch_vcpu_wq(vcpu);
>  
> -	wait_event_interruptible(*wq, !vcpu->arch.power_off);
> +	wait_event_interruptible(*wq, ((!vcpu->arch.power_off) &&
> +				       (!vcpu->arch.pause)));
>  }
>  
>  static int kvm_vcpu_initialized(struct kvm_vcpu *vcpu)
> @@ -525,7 +553,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
>  
>  		update_vttbr(vcpu->kvm);
>  
> -		if (vcpu->arch.power_off)
> +		if (vcpu->arch.power_off || vcpu->arch.pause)
>  			vcpu_pause(vcpu);

These two changes really make me feel like kvm_arch_vcpu_runnable()
should be changed to check for both flags and all thes eplaces should
check for runnable instead.

To make the runnable function more robust, perhaps it should check:

1. The vcpu is neither paused nor powered off
2. The vcpu is not in a WFI state (vcpu->arch.wfi_request) OR there are
   pending interrupts (the current check).

Then the logic would fit the name of the function, instead of being a
specific check only called through handle_wfx.

>  
>  		/*
> @@ -551,7 +579,8 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
>  			run->exit_reason = KVM_EXIT_INTR;
>  		}
>  
> -		if (ret <= 0 || need_new_vmid_gen(vcpu->kvm)) {
> +		if (ret <= 0 || need_new_vmid_gen(vcpu->kvm) ||
> +		    vcpu->arch.pause) {

so why do we need to re-check the pause flag, but not the power_off
flag?  That is non-trivial for sure, so if it's correct, deserves a
comment.  Also see my comment on the last patch.

>  			local_irq_enable();
>  			preempt_enable();
>  			kvm_vgic_sync_hwstate(vcpu);
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index 009da6b..69e3785 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -125,6 +125,9 @@ struct kvm_vcpu_arch {
>  	/* vcpu power-off state */
>  	bool power_off;
>  
> +	/* Don't run the guest */

Probably need to be more clear about this being an implementation
requirement rather than being guest visible or related to any
architectural concept.

> +	bool pause;
> +
>  	/* IO related fields */
>  	struct kvm_decode mmio_decode;
>  
> -- 
> 1.9.1
> 

Thanks,
-Christoffer

[PATCH 2/2] kvm: arm/arm64: implement kvm_arm_[halt, resume]_guest

[Christoffer Dall]

On Mon, Jul 06, 2015 at 02:49:56PM +0200, Eric Auger wrote:
> On halt, the guest is forced to exit and prevented from being
> re-entered. This is synchronous.
> 
> Those two operations will be needed for IRQ forwarding setting.
> 
> Signed-off-by: Eric Auger <eric.auger@linaro.org>
> 
> ---
> RFC v1 -> v2:
> - add __maybe_unused
> 
> RFC:
> - rename the function and this latter becomes static
> - remove __KVM_HAVE_ARCH_HALT_GUEST
> 
> v4 -> v5: add arm64 support
> - also defines __KVM_HAVE_ARCH_HALT_GUEST for arm64
> - add pause field
> ---
>  arch/arm/include/asm/kvm_host.h   |  3 +++
>  arch/arm/kvm/arm.c                | 35 ++++++++++++++++++++++++++++++++---
>  arch/arm64/include/asm/kvm_host.h |  3 +++
>  3 files changed, 38 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
> index 304004d..899ae27 100644
> --- a/arch/arm/include/asm/kvm_host.h
> +++ b/arch/arm/include/asm/kvm_host.h
> @@ -132,6 +132,9 @@ struct kvm_vcpu_arch {
>  	/* vcpu power-off state */
>  	bool power_off;
>  
> +	/* Don't run the guest */
> +	bool pause;
> +
>  	/* IO related fields */
>  	struct kvm_decode mmio_decode;
>  
> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
> index 7537e68..46d4ef6 100644
> --- a/arch/arm/kvm/arm.c
> +++ b/arch/arm/kvm/arm.c
> @@ -471,11 +471,39 @@ bool kvm_arch_intc_initialized(struct kvm *kvm)
>  	return vgic_initialized(kvm);
>  }
>  
> +static void kvm_arm_halt_guest(struct kvm *kvm) __maybe_unused;
> +static void kvm_arm_resume_guest(struct kvm *kvm) __maybe_unused;
> +
> +static void kvm_arm_halt_guest(struct kvm *kvm)
> +{
> +	int i;
> +	struct kvm_vcpu *vcpu;
> +
> +	kvm_for_each_vcpu(i, vcpu, kvm)
> +		vcpu->arch.pause = true;
> +	force_vm_exit(cpu_all_mask);
> +}
> +
> +static void kvm_arm_resume_guest(struct kvm *kvm)
> +{
> +	int i;
> +	struct kvm_vcpu *vcpu;
> +
> +	kvm_for_each_vcpu(i, vcpu, kvm) {
> +		wait_queue_head_t *wq = kvm_arch_vcpu_wq(vcpu);
> +
> +		vcpu->arch.pause = false;
> +		wake_up_interruptible(wq);
> +	}
> +}
> +
> +
>  static void vcpu_pause(struct kvm_vcpu *vcpu)
>  {
>  	wait_queue_head_t *wq = kvm_arch_vcpu_wq(vcpu);
>  
> -	wait_event_interruptible(*wq, !vcpu->arch.power_off);
> +	wait_event_interruptible(*wq, ((!vcpu->arch.power_off) &&
> +				       (!vcpu->arch.pause)));
>  }
>  
>  static int kvm_vcpu_initialized(struct kvm_vcpu *vcpu)
> @@ -525,7 +553,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
>  
>  		update_vttbr(vcpu->kvm);
>  
> -		if (vcpu->arch.power_off)
> +		if (vcpu->arch.power_off || vcpu->arch.pause)
>  			vcpu_pause(vcpu);

These two changes really make me feel like kvm_arch_vcpu_runnable()
should be changed to check for both flags and all thes eplaces should
check for runnable instead.

To make the runnable function more robust, perhaps it should check:

1. The vcpu is neither paused nor powered off
2. The vcpu is not in a WFI state (vcpu->arch.wfi_request) OR there are
   pending interrupts (the current check).

Then the logic would fit the name of the function, instead of being a
specific check only called through handle_wfx.

>  
>  		/*
> @@ -551,7 +579,8 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
>  			run->exit_reason = KVM_EXIT_INTR;
>  		}
>  
> -		if (ret <= 0 || need_new_vmid_gen(vcpu->kvm)) {
> +		if (ret <= 0 || need_new_vmid_gen(vcpu->kvm) ||
> +		    vcpu->arch.pause) {

so why do we need to re-check the pause flag, but not the power_off
flag?  That is non-trivial for sure, so if it's correct, deserves a
comment.  Also see my comment on the last patch.

>  			local_irq_enable();
>  			preempt_enable();
>  			kvm_vgic_sync_hwstate(vcpu);
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index 009da6b..69e3785 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -125,6 +125,9 @@ struct kvm_vcpu_arch {
>  	/* vcpu power-off state */
>  	bool power_off;
>  
> +	/* Don't run the guest */

Probably need to be more clear about this being an implementation
requirement rather than being guest visible or related to any
architectural concept.

> +	bool pause;
> +
>  	/* IO related fields */
>  	struct kvm_decode mmio_decode;
>  
> -- 
> 1.9.1
> 

Thanks,
-Christoffer

Re: [PATCH 1/2] KVM: arm: rename pause into power_off

[Eric Auger]

Hi Christoffer,
On 07/18/2015 11:09 AM, Christoffer Dall wrote:
> On Mon, Jul 06, 2015 at 02:49:55PM +0200, Eric Auger wrote:
>> The kvm_vcpu_arch pause field is renamed into power_off to prepare
>> for the introduction of a new pause field.
>>
>> Signed-off-by: Eric Auger <eric.auger@linaro.org>
>>
>> v4 -> v5:
>> - fix compilation issue on arm64 (add power_off field in kvm_host.h)
>> ---
>>  arch/arm/include/asm/kvm_host.h   |  4 ++--
>>  arch/arm/kvm/arm.c                | 10 +++++-----
>>  arch/arm/kvm/psci.c               | 10 +++++-----
>>  arch/arm64/include/asm/kvm_host.h |  4 ++--
>>  4 files changed, 14 insertions(+), 14 deletions(-)
>>
>> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
>> index e896d2c..304004d 100644
>> --- a/arch/arm/include/asm/kvm_host.h
>> +++ b/arch/arm/include/asm/kvm_host.h
>> @@ -129,8 +129,8 @@ struct kvm_vcpu_arch {
>>  	 * here.
>>  	 */
>>  
>> -	/* Don't run the guest on this vcpu */
>> -	bool pause;
>> +	/* vcpu power-off state */
>> +	bool power_off;
>>  
>>  	/* IO related fields */
>>  	struct kvm_decode mmio_decode;
>> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
>> index bcdf799..7537e68 100644
>> --- a/arch/arm/kvm/arm.c
>> +++ b/arch/arm/kvm/arm.c
>> @@ -475,7 +475,7 @@ static void vcpu_pause(struct kvm_vcpu *vcpu)
>>  {
>>  	wait_queue_head_t *wq = kvm_arch_vcpu_wq(vcpu);
>>  
>> -	wait_event_interruptible(*wq, !vcpu->arch.pause);
>> +	wait_event_interruptible(*wq, !vcpu->arch.power_off);
> 
> would there be any benefit to simply calling kvm_vcpu_block() instead of
> vcpu_pause, and rewrite kvm_arch_vcpu_runnable to:
Wouldn't it somehow change the known behavior or kvm_vcpu_block which is
expected/used to exit on IRQ/FIQ (WFI). Here it would exit when
power_off changes to false (or maybe you meant pause below in the new
context?).
> 
> int kvm_arch_vcpu_runnable(struct kvm_vcpu *v)
> {
> ▸       return !vcpu->arch.power_off &&
> 		(!!v->arch.irq_lines || kvm_vgic_vcpu_pending_irq(v));
> }
> 
> Not sure really, certainly the runnable function does not become more
> readable.
To me the usage of kvm_vcpu_block looks more complex than this code and
I would prefer keeping that version if you don't mind.
> 
>>  }
>>  
>>  static int kvm_vcpu_initialized(struct kvm_vcpu *vcpu)
>> @@ -525,7 +525,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
>>  
>>  		update_vttbr(vcpu->kvm);
>>  
>> -		if (vcpu->arch.pause)
>> +		if (vcpu->arch.power_off)
>>  			vcpu_pause(vcpu);
> 
> looking back over this code, how does this actually guarantee that we
> don't run a powered-off cpu?
> 
> vcpu_pause() just does a wait_event_interruptible(), so if we get
> scheduled again, we'll just proceed running. 
actually it also checks the !vcpu->arch.power_off condition, right?
 Is there any case where we
> could get scheduled without signal_pending() being true and therefore
> inadvertedly run the vcpu?
kvm_arm_halt_guest can happen at any time, including after the execution
of above vcpu_pause(vcpu) call. This is the reason why I added the
second check below, once we entered the critical section and just before
running the vcpu.

With regard to renamed power_off boolean my understanding is:

power_off is set
- on kvm_arch_vcpu_ioctl_vcpu_init/KVM_ARM_VCPU_POWER_OFF
- on PSCI calls from guests through traps from HVC instructions
in that case I don't think this can happen

- on KVM_SET_MP_STATE ioctl: I think in that case, what you describe can
happen.

Do you share the same understanding?

Best Regards

Eric

> 
> if so, we should change the line below like this:
> 
> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
> index bc738d2..98f31e6 100644
> --- a/arch/arm/kvm/arm.c
> +++ b/arch/arm/kvm/arm.c
> @@ -542,7 +542,8 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
>  			run->exit_reason = KVM_EXIT_INTR;
>  		}
>  
> -		if (ret <= 0 || need_new_vmid_gen(vcpu->kvm)) {
> +		if (ret <= 0 || need_new_vmid_gen(vcpu->kvm) ||
> +		    vcpu->arch.power_off) {
>  			local_irq_enable();
>  			preempt_enable();
>  			kvm_timer_sync_hwstate(vcpu);
> 
> 
> Sorry for polluting your patch with these questions, I'm otherwise fine
> with the rename.
> 
> Thanks,
> -Christoffer
> 
>>  
>>  		/*
>> @@ -766,12 +766,12 @@ static int kvm_arch_vcpu_ioctl_vcpu_init(struct kvm_vcpu *vcpu,
>>  	vcpu_reset_hcr(vcpu);
>>  
>>  	/*
>> -	 * Handle the "start in power-off" case by marking the VCPU as paused.
>> +	 * Handle the "start in power-off" case.
>>  	 */
>>  	if (test_bit(KVM_ARM_VCPU_POWER_OFF, vcpu->arch.features))
>> -		vcpu->arch.pause = true;
>> +		vcpu->arch.power_off = true;
>>  	else
>> -		vcpu->arch.pause = false;
>> +		vcpu->arch.power_off = false;
>>  
>>  	return 0;
>>  }
>> diff --git a/arch/arm/kvm/psci.c b/arch/arm/kvm/psci.c
>> index 4b94b51..134971a 100644
>> --- a/arch/arm/kvm/psci.c
>> +++ b/arch/arm/kvm/psci.c
>> @@ -63,7 +63,7 @@ static unsigned long kvm_psci_vcpu_suspend(struct kvm_vcpu *vcpu)
>>  
>>  static void kvm_psci_vcpu_off(struct kvm_vcpu *vcpu)
>>  {
>> -	vcpu->arch.pause = true;
>> +	vcpu->arch.power_off = true;
>>  }
>>  
>>  static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu)
>> @@ -87,7 +87,7 @@ static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu)
>>  	 */
>>  	if (!vcpu)
>>  		return PSCI_RET_INVALID_PARAMS;
>> -	if (!vcpu->arch.pause) {
>> +	if (!vcpu->arch.power_off) {
>>  		if (kvm_psci_version(source_vcpu) != KVM_ARM_PSCI_0_1)
>>  			return PSCI_RET_ALREADY_ON;
>>  		else
>> @@ -115,7 +115,7 @@ static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu)
>>  	 * the general puspose registers are undefined upon CPU_ON.
>>  	 */
>>  	*vcpu_reg(vcpu, 0) = context_id;
>> -	vcpu->arch.pause = false;
>> +	vcpu->arch.power_off = false;
>>  	smp_mb();		/* Make sure the above is visible */
>>  
>>  	wq = kvm_arch_vcpu_wq(vcpu);
>> @@ -152,7 +152,7 @@ static unsigned long kvm_psci_vcpu_affinity_info(struct kvm_vcpu *vcpu)
>>  	kvm_for_each_vcpu(i, tmp, kvm) {
>>  		mpidr = kvm_vcpu_get_mpidr_aff(tmp);
>>  		if (((mpidr & target_affinity_mask) == target_affinity) &&
>> -		    !tmp->arch.pause) {
>> +		    !tmp->arch.power_off) {
>>  			return PSCI_0_2_AFFINITY_LEVEL_ON;
>>  		}
>>  	}
>> @@ -175,7 +175,7 @@ static void kvm_prepare_system_event(struct kvm_vcpu *vcpu, u32 type)
>>  	 * re-initialized.
>>  	 */
>>  	kvm_for_each_vcpu(i, tmp, vcpu->kvm) {
>> -		tmp->arch.pause = true;
>> +		tmp->arch.power_off = true;
>>  		kvm_vcpu_kick(tmp);
>>  	}
>>  
>> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
>> index 2709db2..009da6b 100644
>> --- a/arch/arm64/include/asm/kvm_host.h
>> +++ b/arch/arm64/include/asm/kvm_host.h
>> @@ -122,8 +122,8 @@ struct kvm_vcpu_arch {
>>  	 * here.
>>  	 */
>>  
>> -	/* Don't run the guest */
>> -	bool pause;
>> +	/* vcpu power-off state */
>> +	bool power_off;
>>  
>>  	/* IO related fields */
>>  	struct kvm_decode mmio_decode;
>> -- 
>> 1.9.1
>>

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH 1/2] KVM: arm: rename pause into power_off

[Eric Auger]

Hi Christoffer,
On 07/18/2015 11:09 AM, Christoffer Dall wrote:
> On Mon, Jul 06, 2015 at 02:49:55PM +0200, Eric Auger wrote:
>> The kvm_vcpu_arch pause field is renamed into power_off to prepare
>> for the introduction of a new pause field.
>>
>> Signed-off-by: Eric Auger <eric.auger@linaro.org>
>>
>> v4 -> v5:
>> - fix compilation issue on arm64 (add power_off field in kvm_host.h)
>> ---
>>  arch/arm/include/asm/kvm_host.h   |  4 ++--
>>  arch/arm/kvm/arm.c                | 10 +++++-----
>>  arch/arm/kvm/psci.c               | 10 +++++-----
>>  arch/arm64/include/asm/kvm_host.h |  4 ++--
>>  4 files changed, 14 insertions(+), 14 deletions(-)
>>
>> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
>> index e896d2c..304004d 100644
>> --- a/arch/arm/include/asm/kvm_host.h
>> +++ b/arch/arm/include/asm/kvm_host.h
>> @@ -129,8 +129,8 @@ struct kvm_vcpu_arch {
>>  	 * here.
>>  	 */
>>  
>> -	/* Don't run the guest on this vcpu */
>> -	bool pause;
>> +	/* vcpu power-off state */
>> +	bool power_off;
>>  
>>  	/* IO related fields */
>>  	struct kvm_decode mmio_decode;
>> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
>> index bcdf799..7537e68 100644
>> --- a/arch/arm/kvm/arm.c
>> +++ b/arch/arm/kvm/arm.c
>> @@ -475,7 +475,7 @@ static void vcpu_pause(struct kvm_vcpu *vcpu)
>>  {
>>  	wait_queue_head_t *wq = kvm_arch_vcpu_wq(vcpu);
>>  
>> -	wait_event_interruptible(*wq, !vcpu->arch.pause);
>> +	wait_event_interruptible(*wq, !vcpu->arch.power_off);
> 
> would there be any benefit to simply calling kvm_vcpu_block() instead of
> vcpu_pause, and rewrite kvm_arch_vcpu_runnable to:
Wouldn't it somehow change the known behavior or kvm_vcpu_block which is
expected/used to exit on IRQ/FIQ (WFI). Here it would exit when
power_off changes to false (or maybe you meant pause below in the new
context?).
> 
> int kvm_arch_vcpu_runnable(struct kvm_vcpu *v)
> {
> ?       return !vcpu->arch.power_off &&
> 		(!!v->arch.irq_lines || kvm_vgic_vcpu_pending_irq(v));
> }
> 
> Not sure really, certainly the runnable function does not become more
> readable.
To me the usage of kvm_vcpu_block looks more complex than this code and
I would prefer keeping that version if you don't mind.
> 
>>  }
>>  
>>  static int kvm_vcpu_initialized(struct kvm_vcpu *vcpu)
>> @@ -525,7 +525,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
>>  
>>  		update_vttbr(vcpu->kvm);
>>  
>> -		if (vcpu->arch.pause)
>> +		if (vcpu->arch.power_off)
>>  			vcpu_pause(vcpu);
> 
> looking back over this code, how does this actually guarantee that we
> don't run a powered-off cpu?
> 
> vcpu_pause() just does a wait_event_interruptible(), so if we get
> scheduled again, we'll just proceed running. 
actually it also checks the !vcpu->arch.power_off condition, right?
 Is there any case where we
> could get scheduled without signal_pending() being true and therefore
> inadvertedly run the vcpu?
kvm_arm_halt_guest can happen at any time, including after the execution
of above vcpu_pause(vcpu) call. This is the reason why I added the
second check below, once we entered the critical section and just before
running the vcpu.

With regard to renamed power_off boolean my understanding is:

power_off is set
- on kvm_arch_vcpu_ioctl_vcpu_init/KVM_ARM_VCPU_POWER_OFF
- on PSCI calls from guests through traps from HVC instructions
in that case I don't think this can happen

- on KVM_SET_MP_STATE ioctl: I think in that case, what you describe can
happen.

Do you share the same understanding?

Best Regards

Eric

> 
> if so, we should change the line below like this:
> 
> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
> index bc738d2..98f31e6 100644
> --- a/arch/arm/kvm/arm.c
> +++ b/arch/arm/kvm/arm.c
> @@ -542,7 +542,8 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
>  			run->exit_reason = KVM_EXIT_INTR;
>  		}
>  
> -		if (ret <= 0 || need_new_vmid_gen(vcpu->kvm)) {
> +		if (ret <= 0 || need_new_vmid_gen(vcpu->kvm) ||
> +		    vcpu->arch.power_off) {
>  			local_irq_enable();
>  			preempt_enable();
>  			kvm_timer_sync_hwstate(vcpu);
> 
> 
> Sorry for polluting your patch with these questions, I'm otherwise fine
> with the rename.
> 
> Thanks,
> -Christoffer
> 
>>  
>>  		/*
>> @@ -766,12 +766,12 @@ static int kvm_arch_vcpu_ioctl_vcpu_init(struct kvm_vcpu *vcpu,
>>  	vcpu_reset_hcr(vcpu);
>>  
>>  	/*
>> -	 * Handle the "start in power-off" case by marking the VCPU as paused.
>> +	 * Handle the "start in power-off" case.
>>  	 */
>>  	if (test_bit(KVM_ARM_VCPU_POWER_OFF, vcpu->arch.features))
>> -		vcpu->arch.pause = true;
>> +		vcpu->arch.power_off = true;
>>  	else
>> -		vcpu->arch.pause = false;
>> +		vcpu->arch.power_off = false;
>>  
>>  	return 0;
>>  }
>> diff --git a/arch/arm/kvm/psci.c b/arch/arm/kvm/psci.c
>> index 4b94b51..134971a 100644
>> --- a/arch/arm/kvm/psci.c
>> +++ b/arch/arm/kvm/psci.c
>> @@ -63,7 +63,7 @@ static unsigned long kvm_psci_vcpu_suspend(struct kvm_vcpu *vcpu)
>>  
>>  static void kvm_psci_vcpu_off(struct kvm_vcpu *vcpu)
>>  {
>> -	vcpu->arch.pause = true;
>> +	vcpu->arch.power_off = true;
>>  }
>>  
>>  static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu)
>> @@ -87,7 +87,7 @@ static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu)
>>  	 */
>>  	if (!vcpu)
>>  		return PSCI_RET_INVALID_PARAMS;
>> -	if (!vcpu->arch.pause) {
>> +	if (!vcpu->arch.power_off) {
>>  		if (kvm_psci_version(source_vcpu) != KVM_ARM_PSCI_0_1)
>>  			return PSCI_RET_ALREADY_ON;
>>  		else
>> @@ -115,7 +115,7 @@ static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu)
>>  	 * the general puspose registers are undefined upon CPU_ON.
>>  	 */
>>  	*vcpu_reg(vcpu, 0) = context_id;
>> -	vcpu->arch.pause = false;
>> +	vcpu->arch.power_off = false;
>>  	smp_mb();		/* Make sure the above is visible */
>>  
>>  	wq = kvm_arch_vcpu_wq(vcpu);
>> @@ -152,7 +152,7 @@ static unsigned long kvm_psci_vcpu_affinity_info(struct kvm_vcpu *vcpu)
>>  	kvm_for_each_vcpu(i, tmp, kvm) {
>>  		mpidr = kvm_vcpu_get_mpidr_aff(tmp);
>>  		if (((mpidr & target_affinity_mask) == target_affinity) &&
>> -		    !tmp->arch.pause) {
>> +		    !tmp->arch.power_off) {
>>  			return PSCI_0_2_AFFINITY_LEVEL_ON;
>>  		}
>>  	}
>> @@ -175,7 +175,7 @@ static void kvm_prepare_system_event(struct kvm_vcpu *vcpu, u32 type)
>>  	 * re-initialized.
>>  	 */
>>  	kvm_for_each_vcpu(i, tmp, vcpu->kvm) {
>> -		tmp->arch.pause = true;
>> +		tmp->arch.power_off = true;
>>  		kvm_vcpu_kick(tmp);
>>  	}
>>  
>> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
>> index 2709db2..009da6b 100644
>> --- a/arch/arm64/include/asm/kvm_host.h
>> +++ b/arch/arm64/include/asm/kvm_host.h
>> @@ -122,8 +122,8 @@ struct kvm_vcpu_arch {
>>  	 * here.
>>  	 */
>>  
>> -	/* Don't run the guest */
>> -	bool pause;
>> +	/* vcpu power-off state */
>> +	bool power_off;
>>  
>>  	/* IO related fields */
>>  	struct kvm_decode mmio_decode;
>> -- 
>> 1.9.1
>>

Re: [PATCH 2/2] kvm: arm/arm64: implement kvm_arm_[halt, resume]_guest

[Eric Auger]

On 07/18/2015 11:18 AM, Christoffer Dall wrote:
> On Mon, Jul 06, 2015 at 02:49:56PM +0200, Eric Auger wrote:
>> On halt, the guest is forced to exit and prevented from being
>> re-entered. This is synchronous.
>>
>> Those two operations will be needed for IRQ forwarding setting.
>>
>> Signed-off-by: Eric Auger <eric.auger@linaro.org>
>>
>> ---
>> RFC v1 -> v2:
>> - add __maybe_unused
>>
>> RFC:
>> - rename the function and this latter becomes static
>> - remove __KVM_HAVE_ARCH_HALT_GUEST
>>
>> v4 -> v5: add arm64 support
>> - also defines __KVM_HAVE_ARCH_HALT_GUEST for arm64
>> - add pause field
>> ---
>>  arch/arm/include/asm/kvm_host.h   |  3 +++
>>  arch/arm/kvm/arm.c                | 35 ++++++++++++++++++++++++++++++++---
>>  arch/arm64/include/asm/kvm_host.h |  3 +++
>>  3 files changed, 38 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
>> index 304004d..899ae27 100644
>> --- a/arch/arm/include/asm/kvm_host.h
>> +++ b/arch/arm/include/asm/kvm_host.h
>> @@ -132,6 +132,9 @@ struct kvm_vcpu_arch {
>>  	/* vcpu power-off state */
>>  	bool power_off;
>>  
>> +	/* Don't run the guest */
>> +	bool pause;
>> +
>>  	/* IO related fields */
>>  	struct kvm_decode mmio_decode;
>>  
>> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
>> index 7537e68..46d4ef6 100644
>> --- a/arch/arm/kvm/arm.c
>> +++ b/arch/arm/kvm/arm.c
>> @@ -471,11 +471,39 @@ bool kvm_arch_intc_initialized(struct kvm *kvm)
>>  	return vgic_initialized(kvm);
>>  }
>>  
>> +static void kvm_arm_halt_guest(struct kvm *kvm) __maybe_unused;
>> +static void kvm_arm_resume_guest(struct kvm *kvm) __maybe_unused;
>> +
>> +static void kvm_arm_halt_guest(struct kvm *kvm)
>> +{
>> +	int i;
>> +	struct kvm_vcpu *vcpu;
>> +
>> +	kvm_for_each_vcpu(i, vcpu, kvm)
>> +		vcpu->arch.pause = true;
>> +	force_vm_exit(cpu_all_mask);
>> +}
>> +
>> +static void kvm_arm_resume_guest(struct kvm *kvm)
>> +{
>> +	int i;
>> +	struct kvm_vcpu *vcpu;
>> +
>> +	kvm_for_each_vcpu(i, vcpu, kvm) {
>> +		wait_queue_head_t *wq = kvm_arch_vcpu_wq(vcpu);
>> +
>> +		vcpu->arch.pause = false;
>> +		wake_up_interruptible(wq);
>> +	}
>> +}
>> +
>> +
>>  static void vcpu_pause(struct kvm_vcpu *vcpu)
>>  {
>>  	wait_queue_head_t *wq = kvm_arch_vcpu_wq(vcpu);
>>  
>> -	wait_event_interruptible(*wq, !vcpu->arch.power_off);
>> +	wait_event_interruptible(*wq, ((!vcpu->arch.power_off) &&
>> +				       (!vcpu->arch.pause)));
>>  }
>>  
>>  static int kvm_vcpu_initialized(struct kvm_vcpu *vcpu)
>> @@ -525,7 +553,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
>>  
>>  		update_vttbr(vcpu->kvm);
>>  
>> -		if (vcpu->arch.power_off)
>> +		if (vcpu->arch.power_off || vcpu->arch.pause)
>>  			vcpu_pause(vcpu);
> 
> These two changes really make me feel like kvm_arch_vcpu_runnable()
> should be changed to check for both flags and all thes eplaces should
> check for runnable instead.
> 
> To make the runnable function more robust, perhaps it should check:
> 
> 1. The vcpu is neither paused nor powered off
> 2. The vcpu is not in a WFI state (vcpu->arch.wfi_request) OR there are
>    pending interrupts (the current check).
> 
> Then the logic would fit the name of the function, instead of being a
> specific check only called through handle_wfx.

Reading this again, I reconsider what I said in previous comment answer
and this definitively makes sense to update the runnable function. I
was/am afraid by this vcpu_block function and I need to better undertand
it - I think what I said previously is not correct - .

I will investigate that direction.

Eric
> 
>>  
>>  		/*
>> @@ -551,7 +579,8 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
>>  			run->exit_reason = KVM_EXIT_INTR;
>>  		}
>>  
>> -		if (ret <= 0 || need_new_vmid_gen(vcpu->kvm)) {
>> +		if (ret <= 0 || need_new_vmid_gen(vcpu->kvm) ||
>> +		    vcpu->arch.pause) {
> 
> so why do we need to re-check the pause flag, but not the power_off
> flag?  That is non-trivial for sure, so if it's correct, deserves a
> comment.  Also see my comment on the last patch.
> 
>>  			local_irq_enable();
>>  			preempt_enable();
>>  			kvm_vgic_sync_hwstate(vcpu);
>> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
>> index 009da6b..69e3785 100644
>> --- a/arch/arm64/include/asm/kvm_host.h
>> +++ b/arch/arm64/include/asm/kvm_host.h
>> @@ -125,6 +125,9 @@ struct kvm_vcpu_arch {
>>  	/* vcpu power-off state */
>>  	bool power_off;
>>  
>> +	/* Don't run the guest */
> 
> Probably need to be more clear about this being an implementation
> requirement rather than being guest visible or related to any
> architectural concept.
> 
>> +	bool pause;
>> +
>>  	/* IO related fields */
>>  	struct kvm_decode mmio_decode;
>>  
>> -- 
>> 1.9.1
>>
> 
> Thanks,
> -Christoffer
> 

[PATCH 2/2] kvm: arm/arm64: implement kvm_arm_[halt, resume]_guest

[Eric Auger]

On 07/18/2015 11:18 AM, Christoffer Dall wrote:
> On Mon, Jul 06, 2015 at 02:49:56PM +0200, Eric Auger wrote:
>> On halt, the guest is forced to exit and prevented from being
>> re-entered. This is synchronous.
>>
>> Those two operations will be needed for IRQ forwarding setting.
>>
>> Signed-off-by: Eric Auger <eric.auger@linaro.org>
>>
>> ---
>> RFC v1 -> v2:
>> - add __maybe_unused
>>
>> RFC:
>> - rename the function and this latter becomes static
>> - remove __KVM_HAVE_ARCH_HALT_GUEST
>>
>> v4 -> v5: add arm64 support
>> - also defines __KVM_HAVE_ARCH_HALT_GUEST for arm64
>> - add pause field
>> ---
>>  arch/arm/include/asm/kvm_host.h   |  3 +++
>>  arch/arm/kvm/arm.c                | 35 ++++++++++++++++++++++++++++++++---
>>  arch/arm64/include/asm/kvm_host.h |  3 +++
>>  3 files changed, 38 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
>> index 304004d..899ae27 100644
>> --- a/arch/arm/include/asm/kvm_host.h
>> +++ b/arch/arm/include/asm/kvm_host.h
>> @@ -132,6 +132,9 @@ struct kvm_vcpu_arch {
>>  	/* vcpu power-off state */
>>  	bool power_off;
>>  
>> +	/* Don't run the guest */
>> +	bool pause;
>> +
>>  	/* IO related fields */
>>  	struct kvm_decode mmio_decode;
>>  
>> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
>> index 7537e68..46d4ef6 100644
>> --- a/arch/arm/kvm/arm.c
>> +++ b/arch/arm/kvm/arm.c
>> @@ -471,11 +471,39 @@ bool kvm_arch_intc_initialized(struct kvm *kvm)
>>  	return vgic_initialized(kvm);
>>  }
>>  
>> +static void kvm_arm_halt_guest(struct kvm *kvm) __maybe_unused;
>> +static void kvm_arm_resume_guest(struct kvm *kvm) __maybe_unused;
>> +
>> +static void kvm_arm_halt_guest(struct kvm *kvm)
>> +{
>> +	int i;
>> +	struct kvm_vcpu *vcpu;
>> +
>> +	kvm_for_each_vcpu(i, vcpu, kvm)
>> +		vcpu->arch.pause = true;
>> +	force_vm_exit(cpu_all_mask);
>> +}
>> +
>> +static void kvm_arm_resume_guest(struct kvm *kvm)
>> +{
>> +	int i;
>> +	struct kvm_vcpu *vcpu;
>> +
>> +	kvm_for_each_vcpu(i, vcpu, kvm) {
>> +		wait_queue_head_t *wq = kvm_arch_vcpu_wq(vcpu);
>> +
>> +		vcpu->arch.pause = false;
>> +		wake_up_interruptible(wq);
>> +	}
>> +}
>> +
>> +
>>  static void vcpu_pause(struct kvm_vcpu *vcpu)
>>  {
>>  	wait_queue_head_t *wq = kvm_arch_vcpu_wq(vcpu);
>>  
>> -	wait_event_interruptible(*wq, !vcpu->arch.power_off);
>> +	wait_event_interruptible(*wq, ((!vcpu->arch.power_off) &&
>> +				       (!vcpu->arch.pause)));
>>  }
>>  
>>  static int kvm_vcpu_initialized(struct kvm_vcpu *vcpu)
>> @@ -525,7 +553,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
>>  
>>  		update_vttbr(vcpu->kvm);
>>  
>> -		if (vcpu->arch.power_off)
>> +		if (vcpu->arch.power_off || vcpu->arch.pause)
>>  			vcpu_pause(vcpu);
> 
> These two changes really make me feel like kvm_arch_vcpu_runnable()
> should be changed to check for both flags and all thes eplaces should
> check for runnable instead.
> 
> To make the runnable function more robust, perhaps it should check:
> 
> 1. The vcpu is neither paused nor powered off
> 2. The vcpu is not in a WFI state (vcpu->arch.wfi_request) OR there are
>    pending interrupts (the current check).
> 
> Then the logic would fit the name of the function, instead of being a
> specific check only called through handle_wfx.

Reading this again, I reconsider what I said in previous comment answer
and this definitively makes sense to update the runnable function. I
was/am afraid by this vcpu_block function and I need to better undertand
it - I think what I said previously is not correct - .

I will investigate that direction.

Eric
> 
>>  
>>  		/*
>> @@ -551,7 +579,8 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
>>  			run->exit_reason = KVM_EXIT_INTR;
>>  		}
>>  
>> -		if (ret <= 0 || need_new_vmid_gen(vcpu->kvm)) {
>> +		if (ret <= 0 || need_new_vmid_gen(vcpu->kvm) ||
>> +		    vcpu->arch.pause) {
> 
> so why do we need to re-check the pause flag, but not the power_off
> flag?  That is non-trivial for sure, so if it's correct, deserves a
> comment.  Also see my comment on the last patch.
> 
>>  			local_irq_enable();
>>  			preempt_enable();
>>  			kvm_vgic_sync_hwstate(vcpu);
>> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
>> index 009da6b..69e3785 100644
>> --- a/arch/arm64/include/asm/kvm_host.h
>> +++ b/arch/arm64/include/asm/kvm_host.h
>> @@ -125,6 +125,9 @@ struct kvm_vcpu_arch {
>>  	/* vcpu power-off state */
>>  	bool power_off;
>>  
>> +	/* Don't run the guest */
> 
> Probably need to be more clear about this being an implementation
> requirement rather than being guest visible or related to any
> architectural concept.
> 
>> +	bool pause;
>> +
>>  	/* IO related fields */
>>  	struct kvm_decode mmio_decode;
>>  
>> -- 
>> 1.9.1
>>
> 
> Thanks,
> -Christoffer
> 

Re: [PATCH 2/2] kvm: arm/arm64: implement kvm_arm_[halt, resume]_guest

[Eric Auger]

Hi Chistoffer,
On 07/18/2015 11:18 AM, Christoffer Dall wrote:
> On Mon, Jul 06, 2015 at 02:49:56PM +0200, Eric Auger wrote:
>> On halt, the guest is forced to exit and prevented from being
>> re-entered. This is synchronous.
>>
>> Those two operations will be needed for IRQ forwarding setting.
>>
>> Signed-off-by: Eric Auger <eric.auger@linaro.org>
>>
>> ---
>> RFC v1 -> v2:
>> - add __maybe_unused
>>
>> RFC:
>> - rename the function and this latter becomes static
>> - remove __KVM_HAVE_ARCH_HALT_GUEST
>>
>> v4 -> v5: add arm64 support
>> - also defines __KVM_HAVE_ARCH_HALT_GUEST for arm64
>> - add pause field
>> ---
>>  arch/arm/include/asm/kvm_host.h   |  3 +++
>>  arch/arm/kvm/arm.c                | 35 ++++++++++++++++++++++++++++++++---
>>  arch/arm64/include/asm/kvm_host.h |  3 +++
>>  3 files changed, 38 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
>> index 304004d..899ae27 100644
>> --- a/arch/arm/include/asm/kvm_host.h
>> +++ b/arch/arm/include/asm/kvm_host.h
>> @@ -132,6 +132,9 @@ struct kvm_vcpu_arch {
>>  	/* vcpu power-off state */
>>  	bool power_off;
>>  
>> +	/* Don't run the guest */
>> +	bool pause;
>> +
>>  	/* IO related fields */
>>  	struct kvm_decode mmio_decode;
>>  
>> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
>> index 7537e68..46d4ef6 100644
>> --- a/arch/arm/kvm/arm.c
>> +++ b/arch/arm/kvm/arm.c
>> @@ -471,11 +471,39 @@ bool kvm_arch_intc_initialized(struct kvm *kvm)
>>  	return vgic_initialized(kvm);
>>  }
>>  
>> +static void kvm_arm_halt_guest(struct kvm *kvm) __maybe_unused;
>> +static void kvm_arm_resume_guest(struct kvm *kvm) __maybe_unused;
>> +
>> +static void kvm_arm_halt_guest(struct kvm *kvm)
>> +{
>> +	int i;
>> +	struct kvm_vcpu *vcpu;
>> +
>> +	kvm_for_each_vcpu(i, vcpu, kvm)
>> +		vcpu->arch.pause = true;
>> +	force_vm_exit(cpu_all_mask);
>> +}
>> +
>> +static void kvm_arm_resume_guest(struct kvm *kvm)
>> +{
>> +	int i;
>> +	struct kvm_vcpu *vcpu;
>> +
>> +	kvm_for_each_vcpu(i, vcpu, kvm) {
>> +		wait_queue_head_t *wq = kvm_arch_vcpu_wq(vcpu);
>> +
>> +		vcpu->arch.pause = false;
>> +		wake_up_interruptible(wq);
>> +	}
>> +}
>> +
>> +
>>  static void vcpu_pause(struct kvm_vcpu *vcpu)
>>  {
>>  	wait_queue_head_t *wq = kvm_arch_vcpu_wq(vcpu);
>>  
>> -	wait_event_interruptible(*wq, !vcpu->arch.power_off);
>> +	wait_event_interruptible(*wq, ((!vcpu->arch.power_off) &&
>> +				       (!vcpu->arch.pause)));
>>  }
>>  
>>  static int kvm_vcpu_initialized(struct kvm_vcpu *vcpu)
>> @@ -525,7 +553,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
>>  
>>  		update_vttbr(vcpu->kvm);
>>  
>> -		if (vcpu->arch.power_off)
>> +		if (vcpu->arch.power_off || vcpu->arch.pause)
>>  			vcpu_pause(vcpu);
> 
> These two changes really make me feel like kvm_arch_vcpu_runnable()
> should be changed to check for both flags and all thes eplaces should
> check for runnable instead.
> 
> To make the runnable function more robust, perhaps it should check:
> 
> 1. The vcpu is neither paused nor powered off
> 2. The vcpu is not in a WFI state (vcpu->arch.wfi_request) OR there are
>    pending interrupts (the current check).

I did not understand what you meant by this vcpu->arch.wfi_request,
which I could not find. In case we call runnable from
kvm_vcpu_block/kvm_vcpu_check_block we sometimes are in WFI
(kvm_handle_wfx)?

Thanks

Eric
> 
> Then the logic would fit the name of the function, instead of being a
> specific check only called through handle_wfx.
> 
>>  
>>  		/*
>> @@ -551,7 +579,8 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
>>  			run->exit_reason = KVM_EXIT_INTR;
>>  		}
>>  
>> -		if (ret <= 0 || need_new_vmid_gen(vcpu->kvm)) {
>> +		if (ret <= 0 || need_new_vmid_gen(vcpu->kvm) ||
>> +		    vcpu->arch.pause) {
> 
> so why do we need to re-check the pause flag, but not the power_off
> flag?  That is non-trivial for sure, so if it's correct, deserves a
> comment.  Also see my comment on the last patch.
> 
>>  			local_irq_enable();
>>  			preempt_enable();
>>  			kvm_vgic_sync_hwstate(vcpu);
>> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
>> index 009da6b..69e3785 100644
>> --- a/arch/arm64/include/asm/kvm_host.h
>> +++ b/arch/arm64/include/asm/kvm_host.h
>> @@ -125,6 +125,9 @@ struct kvm_vcpu_arch {
>>  	/* vcpu power-off state */
>>  	bool power_off;
>>  
>> +	/* Don't run the guest */
> 
> Probably need to be more clear about this being an implementation
> requirement rather than being guest visible or related to any
> architectural concept.
> 
>> +	bool pause;
>> +
>>  	/* IO related fields */
>>  	struct kvm_decode mmio_decode;
>>  
>> -- 
>> 1.9.1
>>
> 
> Thanks,
> -Christoffer
> 

[PATCH 2/2] kvm: arm/arm64: implement kvm_arm_[halt, resume]_guest

[Eric Auger]

Hi Chistoffer,
On 07/18/2015 11:18 AM, Christoffer Dall wrote:
> On Mon, Jul 06, 2015 at 02:49:56PM +0200, Eric Auger wrote:
>> On halt, the guest is forced to exit and prevented from being
>> re-entered. This is synchronous.
>>
>> Those two operations will be needed for IRQ forwarding setting.
>>
>> Signed-off-by: Eric Auger <eric.auger@linaro.org>
>>
>> ---
>> RFC v1 -> v2:
>> - add __maybe_unused
>>
>> RFC:
>> - rename the function and this latter becomes static
>> - remove __KVM_HAVE_ARCH_HALT_GUEST
>>
>> v4 -> v5: add arm64 support
>> - also defines __KVM_HAVE_ARCH_HALT_GUEST for arm64
>> - add pause field
>> ---
>>  arch/arm/include/asm/kvm_host.h   |  3 +++
>>  arch/arm/kvm/arm.c                | 35 ++++++++++++++++++++++++++++++++---
>>  arch/arm64/include/asm/kvm_host.h |  3 +++
>>  3 files changed, 38 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
>> index 304004d..899ae27 100644
>> --- a/arch/arm/include/asm/kvm_host.h
>> +++ b/arch/arm/include/asm/kvm_host.h
>> @@ -132,6 +132,9 @@ struct kvm_vcpu_arch {
>>  	/* vcpu power-off state */
>>  	bool power_off;
>>  
>> +	/* Don't run the guest */
>> +	bool pause;
>> +
>>  	/* IO related fields */
>>  	struct kvm_decode mmio_decode;
>>  
>> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
>> index 7537e68..46d4ef6 100644
>> --- a/arch/arm/kvm/arm.c
>> +++ b/arch/arm/kvm/arm.c
>> @@ -471,11 +471,39 @@ bool kvm_arch_intc_initialized(struct kvm *kvm)
>>  	return vgic_initialized(kvm);
>>  }
>>  
>> +static void kvm_arm_halt_guest(struct kvm *kvm) __maybe_unused;
>> +static void kvm_arm_resume_guest(struct kvm *kvm) __maybe_unused;
>> +
>> +static void kvm_arm_halt_guest(struct kvm *kvm)
>> +{
>> +	int i;
>> +	struct kvm_vcpu *vcpu;
>> +
>> +	kvm_for_each_vcpu(i, vcpu, kvm)
>> +		vcpu->arch.pause = true;
>> +	force_vm_exit(cpu_all_mask);
>> +}
>> +
>> +static void kvm_arm_resume_guest(struct kvm *kvm)
>> +{
>> +	int i;
>> +	struct kvm_vcpu *vcpu;
>> +
>> +	kvm_for_each_vcpu(i, vcpu, kvm) {
>> +		wait_queue_head_t *wq = kvm_arch_vcpu_wq(vcpu);
>> +
>> +		vcpu->arch.pause = false;
>> +		wake_up_interruptible(wq);
>> +	}
>> +}
>> +
>> +
>>  static void vcpu_pause(struct kvm_vcpu *vcpu)
>>  {
>>  	wait_queue_head_t *wq = kvm_arch_vcpu_wq(vcpu);
>>  
>> -	wait_event_interruptible(*wq, !vcpu->arch.power_off);
>> +	wait_event_interruptible(*wq, ((!vcpu->arch.power_off) &&
>> +				       (!vcpu->arch.pause)));
>>  }
>>  
>>  static int kvm_vcpu_initialized(struct kvm_vcpu *vcpu)
>> @@ -525,7 +553,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
>>  
>>  		update_vttbr(vcpu->kvm);
>>  
>> -		if (vcpu->arch.power_off)
>> +		if (vcpu->arch.power_off || vcpu->arch.pause)
>>  			vcpu_pause(vcpu);
> 
> These two changes really make me feel like kvm_arch_vcpu_runnable()
> should be changed to check for both flags and all thes eplaces should
> check for runnable instead.
> 
> To make the runnable function more robust, perhaps it should check:
> 
> 1. The vcpu is neither paused nor powered off
> 2. The vcpu is not in a WFI state (vcpu->arch.wfi_request) OR there are
>    pending interrupts (the current check).

I did not understand what you meant by this vcpu->arch.wfi_request,
which I could not find. In case we call runnable from
kvm_vcpu_block/kvm_vcpu_check_block we sometimes are in WFI
(kvm_handle_wfx)?

Thanks

Eric
> 
> Then the logic would fit the name of the function, instead of being a
> specific check only called through handle_wfx.
> 
>>  
>>  		/*
>> @@ -551,7 +579,8 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
>>  			run->exit_reason = KVM_EXIT_INTR;
>>  		}
>>  
>> -		if (ret <= 0 || need_new_vmid_gen(vcpu->kvm)) {
>> +		if (ret <= 0 || need_new_vmid_gen(vcpu->kvm) ||
>> +		    vcpu->arch.pause) {
> 
> so why do we need to re-check the pause flag, but not the power_off
> flag?  That is non-trivial for sure, so if it's correct, deserves a
> comment.  Also see my comment on the last patch.
> 
>>  			local_irq_enable();
>>  			preempt_enable();
>>  			kvm_vgic_sync_hwstate(vcpu);
>> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
>> index 009da6b..69e3785 100644
>> --- a/arch/arm64/include/asm/kvm_host.h
>> +++ b/arch/arm64/include/asm/kvm_host.h
>> @@ -125,6 +125,9 @@ struct kvm_vcpu_arch {
>>  	/* vcpu power-off state */
>>  	bool power_off;
>>  
>> +	/* Don't run the guest */
> 
> Probably need to be more clear about this being an implementation
> requirement rather than being guest visible or related to any
> architectural concept.
> 
>> +	bool pause;
>> +
>>  	/* IO related fields */
>>  	struct kvm_decode mmio_decode;
>>  
>> -- 
>> 1.9.1
>>
> 
> Thanks,
> -Christoffer
>