Re: [PATCH 09/15] KVM: arm64: Remove debug tracepoints

[Marc Zyngier <maz@kernel.org>]

On Fri, 08 Nov 2024 22:24:13 +0000,
Oliver Upton <oliver.upton@linux.dev> wrote:
> 
> The debug tracepoints are a useless firehose of information that track
> implementation detail rather than well-defined events. These are going
> to be rather difficult to uphold now that the implementation is getting
> redone, so throw them out instead of bending over backwards.
> 
> Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
> ---
>  arch/arm64/kvm/debug.c | 29 -----------------------------
>  1 file changed, 29 deletions(-)
> 
> diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c
> index c2bf1b296b14..66cf4e843b47 100644
> --- a/arch/arm64/kvm/debug.c
> +++ b/arch/arm64/kvm/debug.c
> @@ -35,10 +35,6 @@ static void save_guest_debug_regs(struct kvm_vcpu *vcpu)
>  	u64 val = vcpu_read_sys_reg(vcpu, MDSCR_EL1);
>  
>  	vcpu->arch.guest_debug_preserved.mdscr_el1 = val;
> -
> -	trace_kvm_arm_set_dreg32("Saved MDSCR_EL1",
> -				vcpu->arch.guest_debug_preserved.mdscr_el1);
> -
>  	vcpu->arch.guest_debug_preserved.pstate_ss =
>  					(*vcpu_cpsr(vcpu) & DBG_SPSR_SS);
>  }
> @@ -49,9 +45,6 @@ static void restore_guest_debug_regs(struct kvm_vcpu *vcpu)
>  
>  	vcpu_write_sys_reg(vcpu, val, MDSCR_EL1);
>  
> -	trace_kvm_arm_set_dreg32("Restored MDSCR_EL1",
> -				vcpu_read_sys_reg(vcpu, MDSCR_EL1));
> -
>  	if (vcpu->arch.guest_debug_preserved.pstate_ss)
>  		*vcpu_cpsr(vcpu) |= DBG_SPSR_SS;
>  	else
> @@ -102,8 +95,6 @@ static void kvm_arm_setup_mdcr_el2(struct kvm_vcpu *vcpu)
>  	    !vcpu_get_flag(vcpu, DEBUG_DIRTY) ||
>  	    kvm_vcpu_os_lock_enabled(vcpu))
>  		vcpu->arch.mdcr_el2 |= MDCR_EL2_TDA;
> -
> -	trace_kvm_arm_set_dreg32("MDCR_EL2", vcpu->arch.mdcr_el2);
>  }
>  
>  /**
> @@ -201,8 +192,6 @@ void kvm_arm_setup_debug(struct kvm_vcpu *vcpu)
>  			vcpu_write_sys_reg(vcpu, mdscr, MDSCR_EL1);
>  		}
>  
> -		trace_kvm_arm_set_dreg32("SPSR_EL2", *vcpu_cpsr(vcpu));
> -
>  		/*
>  		 * HW Breakpoints and watchpoints
>  		 *
> @@ -220,14 +209,6 @@ void kvm_arm_setup_debug(struct kvm_vcpu *vcpu)
>  			vcpu->arch.debug_ptr = &vcpu->arch.external_debug_state;
>  			vcpu_set_flag(vcpu, DEBUG_DIRTY);
>  
> -			trace_kvm_arm_set_regset("BKPTS", get_num_brps(),
> -						&vcpu->arch.debug_ptr->dbg_bcr[0],
> -						&vcpu->arch.debug_ptr->dbg_bvr[0]);
> -
> -			trace_kvm_arm_set_regset("WAPTS", get_num_wrps(),
> -						&vcpu->arch.debug_ptr->dbg_wcr[0],
> -						&vcpu->arch.debug_ptr->dbg_wvr[0]);
> -
>  		/*
>  		 * The OS Lock blocks debug exceptions in all ELs when it is
>  		 * enabled. If the guest has enabled the OS Lock, constrain its
> @@ -253,8 +234,6 @@ void kvm_arm_setup_debug(struct kvm_vcpu *vcpu)
>  	/* Write mdcr_el2 changes since vcpu_load on VHE systems */
>  	if (has_vhe() && orig_mdcr_el2 != vcpu->arch.mdcr_el2)
>  		write_sysreg(vcpu->arch.mdcr_el2, mdcr_el2);
> -
> -	trace_kvm_arm_set_dreg32("MDSCR_EL1", vcpu_read_sys_reg(vcpu, MDSCR_EL1));
>  }
>  
>  void kvm_arm_clear_debug(struct kvm_vcpu *vcpu)
> @@ -282,14 +261,6 @@ void kvm_arm_clear_debug(struct kvm_vcpu *vcpu)
>  		 */
>  		if (vcpu->guest_debug & KVM_GUESTDBG_USE_HW) {
>  			kvm_arm_reset_debug_ptr(vcpu);
> -
> -			trace_kvm_arm_set_regset("BKPTS", get_num_brps(),
> -						&vcpu->arch.debug_ptr->dbg_bcr[0],
> -						&vcpu->arch.debug_ptr->dbg_bvr[0]);
> -
> -			trace_kvm_arm_set_regset("WAPTS", get_num_wrps(),
> -						&vcpu->arch.debug_ptr->dbg_wcr[0],
> -						&vcpu->arch.debug_ptr->dbg_wvr[0]);
>  		}
>  	}
>  }

Whilst you're at it, how about:

diff --git a/arch/arm64/kvm/trace_handle_exit.h b/arch/arm64/kvm/trace_handle_exit.h
index 064a58c19f481..fa1e48a36d3f9 100644
--- a/arch/arm64/kvm/trace_handle_exit.h
+++ b/arch/arm64/kvm/trace_handle_exit.h
@@ -101,26 +101,6 @@ TRACE_EVENT(kvm_arm_set_dreg32,
 
 TRACE_DEFINE_SIZEOF(__u64);
 
-TRACE_EVENT(kvm_arm_set_regset,
-	TP_PROTO(const char *type, int len, __u64 *control, __u64 *value),
-	TP_ARGS(type, len, control, value),
-	TP_STRUCT__entry(
-		__field(const char *, name)
-		__field(int, len)
-		__array(u64, ctrls, 16)
-		__array(u64, values, 16)
-	),
-	TP_fast_assign(
-		__entry->name = type;
-		__entry->len = len;
-		memcpy(__entry->ctrls, control, len << 3);
-		memcpy(__entry->values, value, len << 3);
-	),
-	TP_printk("%d %s CTRL:%s VALUE:%s", __entry->len, __entry->name,
-		__print_array(__entry->ctrls, __entry->len, sizeof(__u64)),
-		__print_array(__entry->values, __entry->len, sizeof(__u64)))
-);
-
 TRACE_EVENT(trap_reg,
 	TP_PROTO(const char *fn, int reg, bool is_write, u64 write_value),
 	TP_ARGS(fn, reg, is_write, write_value),

It's not like we're going to miss that.

Thanks,

	M.

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