Re: [PATCH 3/5] KVM: arm/arm64: factor out common wfe/wfi emulation code

[Christoffer Dall <cdall@linaro.org>]

On Fri, Sep 29, 2017 at 01:30:39PM +0200, Andrew Jones wrote:
> Before we add more common code to the wfi emulation, let's first
> factor out everything common.
> 
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> ---
>  arch/arm/include/asm/kvm_host.h   |  2 ++
>  arch/arm/kvm/handle_exit.c        | 14 +++++---------
>  arch/arm64/include/asm/kvm_host.h |  2 ++
>  arch/arm64/kvm/handle_exit.c      | 14 +++++---------
>  virt/kvm/arm/arm.c                | 13 +++++++++++++
>  virt/kvm/arm/psci.c               | 15 +++++++--------
>  6 files changed, 34 insertions(+), 26 deletions(-)
> 
> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
> index 85f3c20b9759..964320825372 100644
> --- a/arch/arm/include/asm/kvm_host.h
> +++ b/arch/arm/include/asm/kvm_host.h
> @@ -231,6 +231,8 @@ void kvm_arm_vcpu_power_off(struct kvm_vcpu *vcpu);
>  void kvm_arm_vcpu_power_on(struct kvm_vcpu *vcpu);
>  void kvm_arm_halt_guest(struct kvm *kvm);
>  void kvm_arm_resume_guest(struct kvm *kvm);
> +void kvm_arm_emulate_wfe(struct kvm_vcpu *vcpu);
> +void kvm_arm_emulate_wfi(struct kvm_vcpu *vcpu);
>  
>  int kvm_arm_copy_coproc_indices(struct kvm_vcpu *vcpu, u64 __user *uindices);
>  unsigned long kvm_arm_num_coproc_regs(struct kvm_vcpu *vcpu);
> diff --git a/arch/arm/kvm/handle_exit.c b/arch/arm/kvm/handle_exit.c
> index cf8bf6bf87c4..e40466577c87 100644
> --- a/arch/arm/kvm/handle_exit.c
> +++ b/arch/arm/kvm/handle_exit.c
> @@ -57,22 +57,18 @@ static int handle_smc(struct kvm_vcpu *vcpu, struct kvm_run *run)
>   * @run:	the kvm_run structure pointer
>   *
>   * WFE: Yield the CPU and come back to this vcpu when the scheduler
> - * decides to.
> - * WFI: Simply call kvm_vcpu_block(), which will halt execution of
> - * world-switches and schedule other host processes until there is an
> - * incoming IRQ or FIQ to the VM.
> + *      decides to.
> + * WFI: Halt execution of world-switches and schedule other host
> + *      processes until there is an incoming IRQ or FIQ to the VM.

s/VM/VCPU/

>   */
>  static int kvm_handle_wfx(struct kvm_vcpu *vcpu, struct kvm_run *run)
>  {
>  	if (kvm_vcpu_get_hsr(vcpu) & HSR_WFI_IS_WFE) {
>  		trace_kvm_wfx(*vcpu_pc(vcpu), true);
> -		vcpu->stat.wfe_exit_stat++;
> -		kvm_vcpu_on_spin(vcpu, vcpu_mode_priv(vcpu));
> +		kvm_arm_emulate_wfe(vcpu);
>  	} else {
>  		trace_kvm_wfx(*vcpu_pc(vcpu), false);
> -		vcpu->stat.wfi_exit_stat++;
> -		kvm_vcpu_block(vcpu);
> -		kvm_clear_request(KVM_REQ_UNHALT, vcpu);
> +		kvm_arm_emulate_wfi(vcpu);
>  	}
>  
>  	kvm_skip_instr(vcpu, kvm_vcpu_trap_il_is32bit(vcpu));
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index 25545a87de11..a774f6b30474 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -332,6 +332,8 @@ void kvm_arm_vcpu_power_off(struct kvm_vcpu *vcpu);
>  void kvm_arm_vcpu_power_on(struct kvm_vcpu *vcpu);
>  void kvm_arm_halt_guest(struct kvm *kvm);
>  void kvm_arm_resume_guest(struct kvm *kvm);
> +void kvm_arm_emulate_wfe(struct kvm_vcpu *vcpu);
> +void kvm_arm_emulate_wfi(struct kvm_vcpu *vcpu);
>  
>  u64 __kvm_call_hyp(void *hypfn, ...);
>  #define kvm_call_hyp(f, ...) __kvm_call_hyp(kvm_ksym_ref(f), ##__VA_ARGS__)
> diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
> index 7debb74843a0..7ba50a296d10 100644
> --- a/arch/arm64/kvm/handle_exit.c
> +++ b/arch/arm64/kvm/handle_exit.c
> @@ -74,22 +74,18 @@ static int handle_no_fpsimd(struct kvm_vcpu *vcpu, struct kvm_run *run)
>   * @vcpu:	the vcpu pointer
>   *
>   * WFE: Yield the CPU and come back to this vcpu when the scheduler
> - * decides to.
> - * WFI: Simply call kvm_vcpu_block(), which will halt execution of
> - * world-switches and schedule other host processes until there is an
> - * incoming IRQ or FIQ to the VM.
> + *      decides to.
> + * WFI: Halt execution of world-switches and schedule other host
> + *      processes until there is an incoming IRQ or FIQ to the VM.
>   */
>  static int kvm_handle_wfx(struct kvm_vcpu *vcpu, struct kvm_run *run)
>  {
>  	if (kvm_vcpu_get_hsr(vcpu) & ESR_ELx_WFx_ISS_WFE) {
>  		trace_kvm_wfx_arm64(*vcpu_pc(vcpu), true);
> -		vcpu->stat.wfe_exit_stat++;
> -		kvm_vcpu_on_spin(vcpu, vcpu_mode_priv(vcpu));
> +		kvm_arm_emulate_wfe(vcpu);
>  	} else {
>  		trace_kvm_wfx_arm64(*vcpu_pc(vcpu), false);
> -		vcpu->stat.wfi_exit_stat++;
> -		kvm_vcpu_block(vcpu);
> -		kvm_clear_request(KVM_REQ_UNHALT, vcpu);
> +		kvm_arm_emulate_wfi(vcpu);
>  	}
>  
>  	kvm_skip_instr(vcpu, kvm_vcpu_trap_il_is32bit(vcpu));
> diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
> index 954e77608d29..220a3dbda76c 100644
> --- a/virt/kvm/arm/arm.c
> +++ b/virt/kvm/arm/arm.c
> @@ -573,6 +573,19 @@ void kvm_arm_resume_guest(struct kvm *kvm)
>  	}
>  }
>  
> +void kvm_arm_emulate_wfe(struct kvm_vcpu *vcpu)
> +{
> +	vcpu->stat.wfe_exit_stat++;
> +	kvm_vcpu_on_spin(vcpu, vcpu_mode_priv(vcpu));
> +}
> +
> +void kvm_arm_emulate_wfi(struct kvm_vcpu *vcpu)
> +{
> +	vcpu->stat.wfi_exit_stat++;
> +	kvm_vcpu_block(vcpu);
> +	kvm_clear_request(KVM_REQ_UNHALT, vcpu);
> +}
> +
>  static void vcpu_req_sleep(struct kvm_vcpu *vcpu)
>  {
>  	struct swait_queue_head *wq = kvm_arch_vcpu_wq(vcpu);
> diff --git a/virt/kvm/arm/psci.c b/virt/kvm/arm/psci.c
> index a7bf152f1247..755c415304ea 100644
> --- a/virt/kvm/arm/psci.c
> +++ b/virt/kvm/arm/psci.c
> @@ -50,20 +50,19 @@ static unsigned long kvm_psci_vcpu_suspend(struct kvm_vcpu *vcpu)
>  	 * This means for KVM the wakeup events are interrupts and
>  	 * this is consistent with intended use of StateID as described
>  	 * in section 5.4.1 of PSCI v0.2 specification (ARM DEN 0022A).
> -	 *
> -	 * Further, we also treat power-down request to be same as
> -	 * stand-by request as-per section 5.4.2 clause 3 of PSCI v0.2
> -	 * specification (ARM DEN 0022A). This means all suspend states
> -	 * for KVM will preserve the register state.
>  	 */
> -	kvm_vcpu_block(vcpu);
> -	kvm_clear_request(KVM_REQ_UNHALT, vcpu);
> -
> +	kvm_arm_emulate_wfi(vcpu);
>  	return PSCI_RET_SUCCESS;
>  }
>  
>  static void kvm_psci_vcpu_off(struct kvm_vcpu *vcpu)
>  {
> +	/*
> +	 * We treat a power-off request the same as a stand-by request,
> +	 * as-per section 5.4.2 clause 3 of PSCI v0.2* specification
> +	 * (ARM DEN 0022A). This means all suspend states for KVM will
> +	 * preserve the register state.
> +	 */

I'm not actually convinced that this part of the comment was about
power-off.  I think what it was trying to say was simply that a suspend
operation should preverse the register state, and therefore the
commentary doesn't belong in this function.  I agree that the comment is
potentially more confusing (I lost the exact version of the document
referred to in the original comment so can't be sure if there was
something unclear in the original document or if the comment is just
vaguely written), than it helps.  Therefore, I think you should either
(a) keep the comment as it is, (b) rewrite it to make sense, or (c) just
delete it.

>  	kvm_arm_vcpu_power_off(vcpu);
>  }
>  
> -- 
> 2.13.5
> 

Thanks,
-Christoffer