Re: [PATCH 4/5] KVM: arm/arm64: improve kvm_arch_vcpu_runnable

[Christoffer Dall <cdall@linaro.org>]

On Fri, Sep 29, 2017 at 01:30:40PM +0200, Andrew Jones wrote:
> Conceptually, kvm_arch_vcpu_runnable() should be "not waiting,
> or waiting for interrupts and interrupts are pending",
> 
>   !waiting-uninterruptable &&
>   (!waiting-for-interrupts || interrupts-pending)
> 
> but the implementation was only
> 
>   !waiting-uninterruptable && interrupts-pending
> 
> Thanks to the context of the two callers there wasn't an issue,
> however, to future-proof the function, this patch implements the
> conceptual condition by applying mp_state to track waiting-
> uninterruptable vs. waiting-for-interrupts.
> 
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> ---
>  Documentation/virtual/kvm/api.txt | 10 ++++++----
>  virt/kvm/arm/arm.c                | 13 +++++++++----
>  2 files changed, 15 insertions(+), 8 deletions(-)
> 
> diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
> index e63a35fafef0..7c0bb1ae10a2 100644
> --- a/Documentation/virtual/kvm/api.txt
> +++ b/Documentation/virtual/kvm/api.txt
> @@ -1071,7 +1071,7 @@ Possible values are:
>   - KVM_MP_STATE_INIT_RECEIVED:   the vcpu has received an INIT signal, and is
>                                   now ready for a SIPI [x86]
>   - KVM_MP_STATE_HALTED:          the vcpu has executed a HLT instruction and
> -                                 is waiting for an interrupt [x86]
> +                                 is waiting for an interrupt [x86,arm/arm64]
>   - KVM_MP_STATE_SIPI_RECEIVED:   the vcpu has just received a SIPI (vector
>                                   accessible via KVM_GET_VCPU_EVENTS) [x86]
>   - KVM_MP_STATE_STOPPED:         the vcpu is stopped [s390,arm/arm64]
> @@ -1087,8 +1087,9 @@ these architectures.
>  
>  For arm/arm64:
>  
> -The only states that are valid are KVM_MP_STATE_STOPPED and
> -KVM_MP_STATE_RUNNABLE which reflect if the vcpu is paused or not.
> +The only states that are valid are KVM_MP_STATE_STOPPED, KVM_MP_STATE_HALTED,
> +and KVM_MP_STATE_RUNNABLE which reflect if the vcpu is powered-off, waiting
> +for interrupts, or powered-on and not waiting for interrupts.
>  

Is it valid to introduce another value after we've publicly declared
that it's not valid?  What if userspace has an assert(state ==
KVM_MP_STATE_STOPPED || state == KVM_MP_STATE_RUNNABLE) ?

At least we should make that is not the case.

>  4.39 KVM_SET_MP_STATE
>  
> @@ -1108,7 +1109,8 @@ these architectures.
>  For arm/arm64:
>  
>  The only states that are valid are KVM_MP_STATE_STOPPED and
> -KVM_MP_STATE_RUNNABLE which reflect if the vcpu should be paused or not.
> +KVM_MP_STATE_RUNNABLE which reflect if the vcpu should be
> +powered-off or not.
>  

It looks pretty dodgy to me that we can return an MP state which we
cannot set again.  Feels like this could break migration.

>  4.40 KVM_SET_IDENTITY_MAP_ADDR
>  
> diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
> index 220a3dbda76c..5bc9b0d2fd0f 100644
> --- a/virt/kvm/arm/arm.c
> +++ b/virt/kvm/arm/arm.c
> @@ -414,13 +414,16 @@ static bool vcpu_should_sleep(struct kvm_vcpu *vcpu)
>   * kvm_arch_vcpu_runnable - determine if the vcpu can be scheduled
>   * @vcpu:	The VCPU pointer
>   *
> - * If the guest CPU is not waiting for interrupts or an interrupt line is
> - * asserted, the CPU is by definition runnable.
> + * If the VCPU is not waiting at all (including sleeping, which is waiting
> + * uninterruptably), or it's waiting for interrupts but interrupts are
> + * pending, then it is by definition runnable.
>   */
>  int kvm_arch_vcpu_runnable(struct kvm_vcpu *vcpu)
>  {
> -	return (!!vcpu->arch.irq_lines || kvm_vgic_vcpu_pending_irq(vcpu))
> -		&& !vcpu_should_sleep(vcpu);
> +	return !vcpu_should_sleep(vcpu) &&
> +	       (vcpu->arch.mp_state != KVM_MP_STATE_HALTED ||
> +		(!!vcpu->arch.irq_lines ||
> +		 kvm_vgic_vcpu_pending_irq(vcpu)));

This is hard to read.  How about:

	bool irq_pending = !!vcpu->arch.irq_lines ||
			   kvm_vgic_vcpu_pending_irq(vcpu);
	
	if (vcpu_should_sleep(vcpu)
		return false;
	else if (vcpu->arch.mp_state == KVM_MP_STATE_HALTED && !irq_pending)
		return false;
	return true;

>  }
>  
>  bool kvm_arch_vcpu_in_kernel(struct kvm_vcpu *vcpu)
> @@ -582,7 +585,9 @@ void kvm_arm_emulate_wfe(struct kvm_vcpu *vcpu)
>  void kvm_arm_emulate_wfi(struct kvm_vcpu *vcpu)
>  {
>  	vcpu->stat.wfi_exit_stat++;
> +	vcpu->arch.mp_state = KVM_MP_STATE_HALTED;
>  	kvm_vcpu_block(vcpu);
> +	vcpu->arch.mp_state = KVM_MP_STATE_RUNNABLE;
>  	kvm_clear_request(KVM_REQ_UNHALT, vcpu);
>  }
>  
> -- 
> 2.13.5
> 

Thanks,
-Christoffer