[PATCH v1 0/2] Add arm64 vcpu exit reasons and tracepoint

[PATCH v1 0/2] Add arm64 vcpu exit reasons and tracepoint

[Jing Zhang]

This patch adds a field in arm64 arch vcpu structure to save the last exit
reason, which could be poked by the hook provided by the tracepoint.
A previous solution adding vcpu exits stats was discussed here:
https://lore.kernel.org/all/20210922010851.2312845-3-jingzhangos@google.com.
As Marc suggested, a tracepoint is preferred for those heavy arm64 vcpu exit
reason stats.

Jing Zhang (2):
  KVM: arm64: Add arch specific exit reasons
  KVM: arm64: Add debug tracepoint for vcpu exits

 arch/arm64/include/asm/kvm_emulate.h |  5 +++
 arch/arm64/include/asm/kvm_host.h    | 36 ++++++++++++++++
 arch/arm64/kvm/arm.c                 |  2 +
 arch/arm64/kvm/handle_exit.c         | 62 +++++++++++++++++++++++++---
 arch/arm64/kvm/mmu.c                 |  4 ++
 arch/arm64/kvm/sys_regs.c            |  6 +++
 arch/arm64/kvm/trace_arm.h           |  8 ++++
 7 files changed, 118 insertions(+), 5 deletions(-)


base-commit: 9872e6bc08d6ef6de79717ff6bbff0f297c134ef
-- 
2.35.1.723.g4982287a31-goog

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

[Jing Zhang]

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;
 };
 
 /* Pointer to the vcpu's SVE FFR for sve_{save,load}_state() */
diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
index e3140abd2e2e..cee0edaacded 100644
--- a/arch/arm64/kvm/handle_exit.c
+++ b/arch/arm64/kvm/handle_exit.c
@@ -49,6 +49,18 @@ static int handle_hvc(struct kvm_vcpu *vcpu)
 	return ret;
 }
 
+static int handle_hvc32(struct kvm_vcpu *vcpu)
+{
+	vcpu->arch.exit_reason = ARM_EXIT_HVC32;
+	return handle_hvc(vcpu);
+}
+
+static int handle_hvc64(struct kvm_vcpu *vcpu)
+{
+	vcpu->arch.exit_reason = ARM_EXIT_HVC64;
+	return handle_hvc(vcpu);
+}
+
 static int handle_smc(struct kvm_vcpu *vcpu)
 {
 	/*
@@ -64,12 +76,25 @@ static int handle_smc(struct kvm_vcpu *vcpu)
 	return 1;
 }
 
+static int handle_smc32(struct kvm_vcpu *vcpu)
+{
+	vcpu->arch.exit_reason = ARM_EXIT_SMC32;
+	return handle_smc(vcpu);
+}
+
+static int handle_smc64(struct kvm_vcpu *vcpu)
+{
+	vcpu->arch.exit_reason = ARM_EXIT_SMC64;
+	return handle_smc(vcpu);
+}
+
 /*
  * Guest access to FP/ASIMD registers are routed to this handler only
  * when the system doesn't support FP/ASIMD.
  */
 static int handle_no_fpsimd(struct kvm_vcpu *vcpu)
 {
+	vcpu->arch.exit_reason = ARM_EXIT_FP_ASIMD;
 	kvm_inject_undefined(vcpu);
 	return 1;
 }
@@ -91,11 +116,13 @@ static int kvm_handle_wfx(struct kvm_vcpu *vcpu)
 	if (kvm_vcpu_get_esr(vcpu) & ESR_ELx_WFx_ISS_WFE) {
 		trace_kvm_wfx_arm64(*vcpu_pc(vcpu), true);
 		vcpu->stat.wfe_exit_stat++;
+		vcpu->arch.exit_reason = ARM_EXIT_WFE;
 		kvm_vcpu_on_spin(vcpu, vcpu_mode_priv(vcpu));
 	} else {
 		trace_kvm_wfx_arm64(*vcpu_pc(vcpu), false);
 		vcpu->stat.wfi_exit_stat++;
 		kvm_vcpu_wfi(vcpu);
+		vcpu->arch.exit_reason = ARM_EXIT_WFI;
 	}
 
 	kvm_incr_pc(vcpu);
@@ -118,12 +145,29 @@ static int kvm_handle_guest_debug(struct kvm_vcpu *vcpu)
 {
 	struct kvm_run *run = vcpu->run;
 	u32 esr = kvm_vcpu_get_esr(vcpu);
+	u8 esr_ec = ESR_ELx_EC(esr);
 
 	run->exit_reason = KVM_EXIT_DEBUG;
 	run->debug.arch.hsr = esr;
 
-	if (ESR_ELx_EC(esr) == ESR_ELx_EC_WATCHPT_LOW)
+	switch (esr_ec) {
+	case ESR_ELx_EC_SOFTSTP_LOW:
+		vcpu->arch.exit_reason = ARM_EXIT_SOFTSTP_LOW;
+		break;
+	case ESR_ELx_EC_WATCHPT_LOW:
 		run->debug.arch.far = vcpu->arch.fault.far_el2;
+		vcpu->arch.exit_reason = ARM_EXIT_WATCHPT_LOW;
+		break;
+	case ESR_ELx_EC_BREAKPT_LOW:
+		vcpu->arch.exit_reason = ARM_EXIT_BREAKPT_LOW;
+		break;
+	case ESR_ELx_EC_BKPT32:
+		vcpu->arch.exit_reason = ARM_EXIT_BKPT32;
+		break;
+	case ESR_ELx_EC_BRK64:
+		vcpu->arch.exit_reason = ARM_EXIT_BRK64;
+		break;
+	}
 
 	return 0;
 }
@@ -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;
 	kvm_inject_undefined(vcpu);
 	return 1;
 }
@@ -145,6 +190,7 @@ static int kvm_handle_unknown_ec(struct kvm_vcpu *vcpu)
  */
 static int handle_sve(struct kvm_vcpu *vcpu)
 {
+	vcpu->arch.exit_reason = ARM_EXIT_SVE;
 	kvm_inject_undefined(vcpu);
 	return 1;
 }
@@ -156,6 +202,7 @@ static int handle_sve(struct kvm_vcpu *vcpu)
  */
 static int kvm_handle_ptrauth(struct kvm_vcpu *vcpu)
 {
+	vcpu->arch.exit_reason = ARM_EXIT_PAC;
 	kvm_inject_undefined(vcpu);
 	return 1;
 }
@@ -168,10 +215,10 @@ static exit_handle_fn arm_exit_handlers[] = {
 	[ESR_ELx_EC_CP14_MR]	= kvm_handle_cp14_32,
 	[ESR_ELx_EC_CP14_LS]	= kvm_handle_cp14_load_store,
 	[ESR_ELx_EC_CP14_64]	= kvm_handle_cp14_64,
-	[ESR_ELx_EC_HVC32]	= handle_hvc,
-	[ESR_ELx_EC_SMC32]	= handle_smc,
-	[ESR_ELx_EC_HVC64]	= handle_hvc,
-	[ESR_ELx_EC_SMC64]	= handle_smc,
+	[ESR_ELx_EC_HVC32]	= handle_hvc32,
+	[ESR_ELx_EC_SMC32]	= handle_smc32,
+	[ESR_ELx_EC_HVC64]	= handle_hvc64,
+	[ESR_ELx_EC_SMC64]	= handle_smc64,
 	[ESR_ELx_EC_SYS64]	= kvm_handle_sys_reg,
 	[ESR_ELx_EC_SVE]	= handle_sve,
 	[ESR_ELx_EC_IABT_LOW]	= kvm_handle_guest_abort,
@@ -240,8 +287,10 @@ int handle_exit(struct kvm_vcpu *vcpu, int exception_index)
 
 	switch (exception_index) {
 	case ARM_EXCEPTION_IRQ:
+		vcpu->arch.exit_reason = ARM_EXIT_IRQ;
 		return 1;
 	case ARM_EXCEPTION_EL1_SERROR:
+		vcpu->arch.exit_reason = ARM_EXIT_EL1_SERROR;
 		return 1;
 	case ARM_EXCEPTION_TRAP:
 		return handle_trap_exceptions(vcpu);
@@ -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;
 		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;
 		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;
 		run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
 		return 0;
 	}
diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
index 1623abc56af2..657babe7857c 100644
--- a/arch/arm64/kvm/mmu.c
+++ b/arch/arm64/kvm/mmu.c
@@ -1333,6 +1333,10 @@ int kvm_handle_guest_abort(struct kvm_vcpu *vcpu)
 
 	fault_ipa = kvm_vcpu_get_fault_ipa(vcpu);
 	is_iabt = kvm_vcpu_trap_is_iabt(vcpu);
+	if (is_iabt)
+		vcpu->arch.exit_reason = ARM_EXIT_IABT_LOW;
+	else if (kvm_vcpu_trap_is_dabt(vcpu))
+		vcpu->arch.exit_reason = ARM_EXIT_DABT_LOW;
 
 	/* Synchronous External Abort? */
 	if (kvm_vcpu_abt_issea(vcpu)) {
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index dd34b5ab51d4..156b6213b17d 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -2205,6 +2205,7 @@ static int check_sysreg_table(const struct sys_reg_desc *table, unsigned int n,
 
 int kvm_handle_cp14_load_store(struct kvm_vcpu *vcpu)
 {
+	vcpu->arch.exit_reason = ARM_EXIT_CP14_LS;
 	kvm_inject_undefined(vcpu);
 	return 1;
 }
@@ -2372,21 +2373,25 @@ static int kvm_handle_cp_32(struct kvm_vcpu *vcpu,
 
 int kvm_handle_cp15_64(struct kvm_vcpu *vcpu)
 {
+	vcpu->arch.exit_reason = ARM_EXIT_CP15_64;
 	return kvm_handle_cp_64(vcpu, cp15_64_regs, ARRAY_SIZE(cp15_64_regs));
 }
 
 int kvm_handle_cp15_32(struct kvm_vcpu *vcpu)
 {
+	vcpu->arch.exit_reason = ARM_EXIT_CP15_32;
 	return kvm_handle_cp_32(vcpu, cp15_regs, ARRAY_SIZE(cp15_regs));
 }
 
 int kvm_handle_cp14_64(struct kvm_vcpu *vcpu)
 {
+	vcpu->arch.exit_reason = ARM_EXIT_CP14_64;
 	return kvm_handle_cp_64(vcpu, cp14_64_regs, ARRAY_SIZE(cp14_64_regs));
 }
 
 int kvm_handle_cp14_32(struct kvm_vcpu *vcpu)
 {
+	vcpu->arch.exit_reason = ARM_EXIT_CP14_32;
 	return kvm_handle_cp_32(vcpu, cp14_regs, ARRAY_SIZE(cp14_regs));
 }
 
@@ -2444,6 +2449,7 @@ int kvm_handle_sys_reg(struct kvm_vcpu *vcpu)
 	int ret;
 
 	trace_kvm_handle_sys_reg(esr);
+	vcpu->arch.exit_reason = ARM_EXIT_SYS64;
 
 	params = esr_sys64_to_params(esr);
 	params.regval = vcpu_get_reg(vcpu, Rt);
-- 
2.35.1.723.g4982287a31-goog

[PATCH v1 2/2] KVM: arm64: Add debug tracepoint for vcpu exits

[Jing Zhang]

This tracepoint only provides a hook for poking vcpu exits information,
not exported to tracefs.
A timestamp is added for the last vcpu exit, which would be useful for
analysis for vcpu exits.

Signed-off-by: Jing Zhang <jingzhangos@google.com>
---
 arch/arm64/include/asm/kvm_host.h | 3 +++
 arch/arm64/kvm/arm.c              | 2 ++
 arch/arm64/kvm/trace_arm.h        | 8 ++++++++
 3 files changed, 13 insertions(+)

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index daa68b053bdc..576f2c18d008 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -415,6 +415,9 @@ struct kvm_vcpu_arch {
 
 	/* Arch specific exit reason */
 	enum arm_exit_reason exit_reason;
+
+	/* Timestamp for the last vcpu exit */
+	u64 last_exit_time;
 };
 
 /* Pointer to the vcpu's SVE FFR for sve_{save,load}_state() */
diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
index f49ebdd9c990..98631f79c182 100644
--- a/arch/arm64/kvm/arm.c
+++ b/arch/arm64/kvm/arm.c
@@ -783,6 +783,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu)
 	ret = 1;
 	run->exit_reason = KVM_EXIT_UNKNOWN;
 	while (ret > 0) {
+		trace_kvm_vcpu_exits(vcpu);
 		/*
 		 * Check conditions before entering the guest
 		 */
@@ -898,6 +899,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu)
 		local_irq_enable();
 
 		trace_kvm_exit(ret, kvm_vcpu_trap_get_class(vcpu), *vcpu_pc(vcpu));
+		vcpu->arch.last_exit_time = ktime_to_ns(ktime_get());
 
 		/* Exit types that need handling before we can be preempted */
 		handle_exit_early(vcpu, ret);
diff --git a/arch/arm64/kvm/trace_arm.h b/arch/arm64/kvm/trace_arm.h
index 33e4e7dd2719..3e7dfd640e23 100644
--- a/arch/arm64/kvm/trace_arm.h
+++ b/arch/arm64/kvm/trace_arm.h
@@ -301,6 +301,14 @@ TRACE_EVENT(kvm_timer_emulate,
 		  __entry->timer_idx, __entry->should_fire)
 );
 
+/*
+ * Following tracepoints are not exported in tracefs and provide hooking
+ * mechanisms only for testing and debugging purposes.
+ */
+DECLARE_TRACE(kvm_vcpu_exits,
+	TP_PROTO(struct kvm_vcpu *vcpu),
+	TP_ARGS(vcpu));
+
 #endif /* _TRACE_ARM_ARM64_KVM_H */
 
 #undef TRACE_INCLUDE_PATH
-- 
2.35.1.723.g4982287a31-goog

Re: [PATCH v1 2/2] KVM: arm64: Add debug tracepoint for vcpu exits

[Oliver Upton]

Hi Jing,

On Thu, Mar 17, 2022 at 12:56:30AM +0000, Jing Zhang wrote:
> This tracepoint only provides a hook for poking vcpu exits information,
> not exported to tracefs.
> A timestamp is added for the last vcpu exit, which would be useful for
> analysis for vcpu exits.
> 
> Signed-off-by: Jing Zhang <jingzhangos@google.com>
> ---
>  arch/arm64/include/asm/kvm_host.h | 3 +++
>  arch/arm64/kvm/arm.c              | 2 ++
>  arch/arm64/kvm/trace_arm.h        | 8 ++++++++
>  3 files changed, 13 insertions(+)
> 
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index daa68b053bdc..576f2c18d008 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -415,6 +415,9 @@ struct kvm_vcpu_arch {
>  
>  	/* Arch specific exit reason */
>  	enum arm_exit_reason exit_reason;
> +
> +	/* Timestamp for the last vcpu exit */
> +	u64 last_exit_time;
>  };
>  
>  /* Pointer to the vcpu's SVE FFR for sve_{save,load}_state() */
> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> index f49ebdd9c990..98631f79c182 100644
> --- a/arch/arm64/kvm/arm.c
> +++ b/arch/arm64/kvm/arm.c
> @@ -783,6 +783,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu)
>  	ret = 1;
>  	run->exit_reason = KVM_EXIT_UNKNOWN;
>  	while (ret > 0) {
> +		trace_kvm_vcpu_exits(vcpu);
>  		/*
>  		 * Check conditions before entering the guest
>  		 */
> @@ -898,6 +899,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu)
>  		local_irq_enable();
>  
>  		trace_kvm_exit(ret, kvm_vcpu_trap_get_class(vcpu), *vcpu_pc(vcpu));
> +		vcpu->arch.last_exit_time = ktime_to_ns(ktime_get());
>  
>  		/* Exit types that need handling before we can be preempted */
>  		handle_exit_early(vcpu, ret);
> diff --git a/arch/arm64/kvm/trace_arm.h b/arch/arm64/kvm/trace_arm.h
> index 33e4e7dd2719..3e7dfd640e23 100644
> --- a/arch/arm64/kvm/trace_arm.h
> +++ b/arch/arm64/kvm/trace_arm.h
> @@ -301,6 +301,14 @@ TRACE_EVENT(kvm_timer_emulate,
>  		  __entry->timer_idx, __entry->should_fire)
>  );
>  
> +/*
> + * Following tracepoints are not exported in tracefs and provide hooking
> + * mechanisms only for testing and debugging purposes.
> + */
> +DECLARE_TRACE(kvm_vcpu_exits,
> +	TP_PROTO(struct kvm_vcpu *vcpu),
> +	TP_ARGS(vcpu));
> +

When we were discussing this earlier, I wasn't aware of the kvm_exit
tracepoint which I think encapsulates what you're looking for.
ESR_EL2.EC is the critical piece to determine what caused the exit.

It is probably also important to call out that this trace point only
will fire for a 'full' KVM exit (i.e. out of hyp and back to the
kernel). There are several instances where the exit is handled in hyp
and we immediately resume the guest.

Now -- I am bordering on clueless with tracepoints, but it isn't
immediately obvious how the attached program can determine the vCPU that
triggered the TP. If we are going to propose modularizing certain KVM
metrics with tracepoints then that would be a rather critical piece of
information.

Apologies for any confusion I added to the whole situation, but
hopefully we can still engage in a broader conversation regarding
how to package up optional KVM metrics.

--
Thanks,
Oliver

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

[Marc Zyngier]

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.

Re: [PATCH v1 2/2] KVM: arm64: Add debug tracepoint for vcpu exits

[Marc Zyngier]

Hi Jing,

On Thu, 17 Mar 2022 00:56:30 +0000,
Jing Zhang <jingzhangos@google.com> wrote:
> 
> This tracepoint only provides a hook for poking vcpu exits information,
> not exported to tracefs.
> A timestamp is added for the last vcpu exit, which would be useful for
> analysis for vcpu exits.

The trace itself gives you a timestamp. Why do you need an extra one?

> 
> Signed-off-by: Jing Zhang <jingzhangos@google.com>
> ---
>  arch/arm64/include/asm/kvm_host.h | 3 +++
>  arch/arm64/kvm/arm.c              | 2 ++
>  arch/arm64/kvm/trace_arm.h        | 8 ++++++++
>  3 files changed, 13 insertions(+)
> 
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index daa68b053bdc..576f2c18d008 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -415,6 +415,9 @@ struct kvm_vcpu_arch {
>  
>  	/* Arch specific exit reason */
>  	enum arm_exit_reason exit_reason;
> +
> +	/* Timestamp for the last vcpu exit */
> +	u64 last_exit_time;
>  };
>  
>  /* Pointer to the vcpu's SVE FFR for sve_{save,load}_state() */
> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> index f49ebdd9c990..98631f79c182 100644
> --- a/arch/arm64/kvm/arm.c
> +++ b/arch/arm64/kvm/arm.c
> @@ -783,6 +783,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu)
>  	ret = 1;
>  	run->exit_reason = KVM_EXIT_UNKNOWN;
>  	while (ret > 0) {
> +		trace_kvm_vcpu_exits(vcpu);

Exit? We haven't entered the guest yet!

>  		/*
>  		 * Check conditions before entering the guest
>  		 */
> @@ -898,6 +899,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu)
>  		local_irq_enable();
>  
>  		trace_kvm_exit(ret, kvm_vcpu_trap_get_class(vcpu), *vcpu_pc(vcpu));
> +		vcpu->arch.last_exit_time = ktime_to_ns(ktime_get());

Why isn't the above tracepoint sufficient? It gives you the EC, and
comes with a timestamp for free. And why should *everyone* pay the
price of this timestamp update if not tracing?

>  
>  		/* Exit types that need handling before we can be preempted */
>  		handle_exit_early(vcpu, ret);
> diff --git a/arch/arm64/kvm/trace_arm.h b/arch/arm64/kvm/trace_arm.h
> index 33e4e7dd2719..3e7dfd640e23 100644
> --- a/arch/arm64/kvm/trace_arm.h
> +++ b/arch/arm64/kvm/trace_arm.h
> @@ -301,6 +301,14 @@ TRACE_EVENT(kvm_timer_emulate,
>  		  __entry->timer_idx, __entry->should_fire)
>  );
>  
> +/*
> + * Following tracepoints are not exported in tracefs and provide hooking
> + * mechanisms only for testing and debugging purposes.
> + */
> +DECLARE_TRACE(kvm_vcpu_exits,
> +	TP_PROTO(struct kvm_vcpu *vcpu),
> +	TP_ARGS(vcpu));
> +
>  #endif /* _TRACE_ARM_ARM64_KVM_H */
>  
>  #undef TRACE_INCLUDE_PATH

I guess this is the only bit I actually like about this series: a
generic, out of the way mechanism to let people hook whatever they
want and dump the state they need.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH v1 2/2] KVM: arm64: Add debug tracepoint for vcpu exits

[David Matlack]

On Wed, Mar 16, 2022 at 10:37 PM Oliver Upton <oupton@google.com> wrote:
>
> Hi Jing,
>
> On Thu, Mar 17, 2022 at 12:56:30AM +0000, Jing Zhang wrote:
> > This tracepoint only provides a hook for poking vcpu exits information,
> > not exported to tracefs.
> > A timestamp is added for the last vcpu exit, which would be useful for
> > analysis for vcpu exits.
> >
> > Signed-off-by: Jing Zhang <jingzhangos@google.com>
> > ---
> >  arch/arm64/include/asm/kvm_host.h | 3 +++
> >  arch/arm64/kvm/arm.c              | 2 ++
> >  arch/arm64/kvm/trace_arm.h        | 8 ++++++++
> >  3 files changed, 13 insertions(+)
> >
> > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> > index daa68b053bdc..576f2c18d008 100644
> > --- a/arch/arm64/include/asm/kvm_host.h
> > +++ b/arch/arm64/include/asm/kvm_host.h
> > @@ -415,6 +415,9 @@ struct kvm_vcpu_arch {
> >
> >       /* Arch specific exit reason */
> >       enum arm_exit_reason exit_reason;
> > +
> > +     /* Timestamp for the last vcpu exit */
> > +     u64 last_exit_time;
> >  };
> >
> >  /* Pointer to the vcpu's SVE FFR for sve_{save,load}_state() */
> > diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> > index f49ebdd9c990..98631f79c182 100644
> > --- a/arch/arm64/kvm/arm.c
> > +++ b/arch/arm64/kvm/arm.c
> > @@ -783,6 +783,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu)
> >       ret = 1;
> >       run->exit_reason = KVM_EXIT_UNKNOWN;
> >       while (ret > 0) {
> > +             trace_kvm_vcpu_exits(vcpu);
> >               /*
> >                * Check conditions before entering the guest
> >                */
> > @@ -898,6 +899,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu)
> >               local_irq_enable();
> >
> >               trace_kvm_exit(ret, kvm_vcpu_trap_get_class(vcpu), *vcpu_pc(vcpu));
> > +             vcpu->arch.last_exit_time = ktime_to_ns(ktime_get());
> >
> >               /* Exit types that need handling before we can be preempted */
> >               handle_exit_early(vcpu, ret);
> > diff --git a/arch/arm64/kvm/trace_arm.h b/arch/arm64/kvm/trace_arm.h
> > index 33e4e7dd2719..3e7dfd640e23 100644
> > --- a/arch/arm64/kvm/trace_arm.h
> > +++ b/arch/arm64/kvm/trace_arm.h
> > @@ -301,6 +301,14 @@ TRACE_EVENT(kvm_timer_emulate,
> >                 __entry->timer_idx, __entry->should_fire)
> >  );
> >
> > +/*
> > + * Following tracepoints are not exported in tracefs and provide hooking
> > + * mechanisms only for testing and debugging purposes.
> > + */
> > +DECLARE_TRACE(kvm_vcpu_exits,
> > +     TP_PROTO(struct kvm_vcpu *vcpu),
> > +     TP_ARGS(vcpu));
> > +
>
> When we were discussing this earlier, I wasn't aware of the kvm_exit
> tracepoint which I think encapsulates what you're looking for.
> ESR_EL2.EC is the critical piece to determine what caused the exit.
>
> It is probably also important to call out that this trace point only
> will fire for a 'full' KVM exit (i.e. out of hyp and back to the
> kernel). There are several instances where the exit is handled in hyp
> and we immediately resume the guest.
>
> Now -- I am bordering on clueless with tracepoints, but it isn't
> immediately obvious how the attached program can determine the vCPU that
> triggered the TP. If we are going to propose modularizing certain KVM
> metrics with tracepoints then that would be a rather critical piece of
> information.
>
> Apologies for any confusion I added to the whole situation, but
> hopefully we can still engage in a broader conversation regarding
> how to package up optional KVM metrics.

These are all good questions.

For context to non-Google folks on the mailing list, we are interested
in exploring Marc's idea of using tracepoint hooking as a way for e.g.
cloud providers to implement proprietary stats without having to
modify KVM.

Adding specific tracepoints (like this series does) is probably
premature until we have figured out the broader design for how
out-of-module stats will work end-to-end and get that infrastructure
merged upstream.

>
> --
> Thanks,
> Oliver