Re: [PATCH v1 1/2] KVM: arm64: Add arch specific exit reasons

[Marc Zyngier <maz@kernel.org>]

Hi Jing,

On Thu, 17 Mar 2022 00:56:29 +0000,
Jing Zhang <jingzhangos@google.com> wrote:
> 
> Arch specific exit reasons have been available for other architectures.
> Add arch specific exit reason support for ARM64, which would be used in
> KVM stats for monitoring VCPU status.
> 
> Signed-off-by: Jing Zhang <jingzhangos@google.com>
> ---
>  arch/arm64/include/asm/kvm_emulate.h |  5 +++
>  arch/arm64/include/asm/kvm_host.h    | 33 +++++++++++++++
>  arch/arm64/kvm/handle_exit.c         | 62 +++++++++++++++++++++++++---
>  arch/arm64/kvm/mmu.c                 |  4 ++
>  arch/arm64/kvm/sys_regs.c            |  6 +++
>  5 files changed, 105 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
> index d62405ce3e6d..f73c8d900642 100644
> --- a/arch/arm64/include/asm/kvm_emulate.h
> +++ b/arch/arm64/include/asm/kvm_emulate.h
> @@ -321,6 +321,11 @@ static inline bool kvm_vcpu_trap_is_iabt(const struct kvm_vcpu *vcpu)
>  	return kvm_vcpu_trap_get_class(vcpu) == ESR_ELx_EC_IABT_LOW;
>  }
>  
> +static inline bool kvm_vcpu_trap_is_dabt(const struct kvm_vcpu *vcpu)
> +{
> +	return kvm_vcpu_trap_get_class(vcpu) == ESR_ELx_EC_DABT_LOW;
> +}
> +
>  static inline bool kvm_vcpu_trap_is_exec_fault(const struct kvm_vcpu *vcpu)
>  {
>  	return kvm_vcpu_trap_is_iabt(vcpu) && !kvm_vcpu_abt_iss1tw(vcpu);
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index 76f795b628f1..daa68b053bdc 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -282,6 +282,36 @@ struct vcpu_reset_state {
>  	bool		reset;
>  };
>  
> +enum arm_exit_reason {
> +	ARM_EXIT_UNKNOWN,
> +	ARM_EXIT_IRQ,
> +	ARM_EXIT_EL1_SERROR,
> +	ARM_EXIT_HYP_GONE,
> +	ARM_EXIT_IL,
> +	ARM_EXIT_WFI,
> +	ARM_EXIT_WFE,
> +	ARM_EXIT_CP15_32,
> +	ARM_EXIT_CP15_64,
> +	ARM_EXIT_CP14_32,
> +	ARM_EXIT_CP14_LS,
> +	ARM_EXIT_CP14_64,
> +	ARM_EXIT_HVC32,
> +	ARM_EXIT_SMC32,
> +	ARM_EXIT_HVC64,
> +	ARM_EXIT_SMC64,
> +	ARM_EXIT_SYS64,
> +	ARM_EXIT_SVE,
> +	ARM_EXIT_IABT_LOW,
> +	ARM_EXIT_DABT_LOW,
> +	ARM_EXIT_SOFTSTP_LOW,
> +	ARM_EXIT_WATCHPT_LOW,
> +	ARM_EXIT_BREAKPT_LOW,
> +	ARM_EXIT_BKPT32,
> +	ARM_EXIT_BRK64,
> +	ARM_EXIT_FP_ASIMD,
> +	ARM_EXIT_PAC,
> +};
> +
>  struct kvm_vcpu_arch {
>  	struct kvm_cpu_context ctxt;
>  	void *sve_state;
> @@ -382,6 +412,9 @@ struct kvm_vcpu_arch {
>  		u64 last_steal;
>  		gpa_t base;
>  	} steal;
> +
> +	/* Arch specific exit reason */
> +	enum arm_exit_reason exit_reason;

We already have a copy of ESR_EL2. Together with the exit code, this
gives you everything you need. Why add another piece of state?

[...]

> @@ -135,6 +179,7 @@ static int kvm_handle_unknown_ec(struct kvm_vcpu *vcpu)
>  	kvm_pr_unimpl("Unknown exception class: esr: %#08x -- %s\n",
>  		      esr, esr_get_class_string(esr));
>  
> +	vcpu->arch.exit_reason = ARM_EXIT_UNKNOWN;

If anything, this should say "either CPU out of spec, or KVM bug". And
I don't see the point of tracking these. This should be reported in a
completely different manner, because this has nothing to do with the
normal exits a vcpu does.

> @@ -250,6 +299,7 @@ int handle_exit(struct kvm_vcpu *vcpu, int exception_index)
>  		 * EL2 has been reset to the hyp-stub. This happens when a guest
>  		 * is pre-empted by kvm_reboot()'s shutdown call.
>  		 */
> +		vcpu->arch.exit_reason = ARM_EXIT_HYP_GONE;

Same thing here: the machine is *rebooting*. Who cares?

>  		run->exit_reason = KVM_EXIT_FAIL_ENTRY;
>  		return 0;
>  	case ARM_EXCEPTION_IL:
> @@ -257,11 +307,13 @@ int handle_exit(struct kvm_vcpu *vcpu, int exception_index)
>  		 * We attempted an illegal exception return.  Guest state must
>  		 * have been corrupted somehow.  Give up.
>  		 */
> +		vcpu->arch.exit_reason = ARM_EXIT_IL;

This is another reason why I dislike this patch. It mixes
architectural state (ESR) and KVM gunk (exit code). Why not spit these
two bits of information in the trace, and let whoever deals with it to
infer what they want from it?

>  		run->exit_reason = KVM_EXIT_FAIL_ENTRY;
>  		return -EINVAL;
>  	default:
>  		kvm_pr_unimpl("Unsupported exception type: %d",
>  			      exception_index);
> +		vcpu->arch.exit_reason = ARM_EXIT_UNKNOWN;

See? Now you have UNKNOWN covering two really different concepts.
That's broken. Overall, this patch is reinventing the wheel (and
slightly square one), and I don't see a good reason for the state
duplication.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm