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

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:

quoted

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 <redacted>

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.

quoted

 }
 
 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