On 08/08/2023 12:04, Marc Zyngier wrote:
On Fri, 04 Aug 2023 11:13:12 +0100, James Clark james.clark@arm.com wrote:
Currently trace will always be generated in nVHE as long as TRBE isn't being used. To allow filtering out guest trace, re-apply the filter rules before switching to the guest.
The TRFCR restore function remains the same.
Signed-off-by: James Clark james.clark@arm.com
arch/arm64/kvm/debug.c | 7 ++++ arch/arm64/kvm/hyp/nvhe/debug-sr.c | 56 +++++++++++++++++++++++++++--- 2 files changed, 59 insertions(+), 4 deletions(-)
diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c index 8725291cb00a..ebb4db20a859 100644 --- a/arch/arm64/kvm/debug.c +++ b/arch/arm64/kvm/debug.c @@ -335,10 +335,17 @@ void kvm_arch_vcpu_load_debug_state_flags(struct kvm_vcpu *vcpu) if (cpuid_feature_extract_unsigned_field(dfr0, ID_AA64DFR0_EL1_TraceBuffer_SHIFT) && !(read_sysreg_s(SYS_TRBIDR_EL1) & TRBIDR_EL1_P)) vcpu_set_flag(vcpu, DEBUG_STATE_SAVE_TRBE);
- /*
* Save TRFCR on nVHE if FEAT_TRF exists. This will be done in cases* where DEBUG_STATE_SAVE_TRBE doesn't completely disable trace.*/- if (cpuid_feature_extract_unsigned_field(dfr0, ID_AA64DFR0_EL1_TraceFilt_SHIFT))
vcpu_set_flag(vcpu, DEBUG_STATE_SAVE_TRFCR);} void kvm_arch_vcpu_put_debug_state_flags(struct kvm_vcpu *vcpu) { vcpu_clear_flag(vcpu, DEBUG_STATE_SAVE_SPE); vcpu_clear_flag(vcpu, DEBUG_STATE_SAVE_TRBE);
- vcpu_clear_flag(vcpu, DEBUG_STATE_SAVE_TRFCR);
} diff --git a/arch/arm64/kvm/hyp/nvhe/debug-sr.c b/arch/arm64/kvm/hyp/nvhe/debug-sr.c index 4558c02eb352..0e8c85b29b92 100644 --- a/arch/arm64/kvm/hyp/nvhe/debug-sr.c +++ b/arch/arm64/kvm/hyp/nvhe/debug-sr.c @@ -51,13 +51,17 @@ static void __debug_restore_spe(u64 pmscr_el1) write_sysreg_s(pmscr_el1, SYS_PMSCR_EL1); } -static void __debug_save_trace(u64 *trfcr_el1) +/*
- Save TRFCR and disable trace completely if TRBE is being used. Return true
- if trace was disabled.
- */
+static bool __debug_save_trace(u64 *trfcr_el1) { *trfcr_el1 = 0; /* Check if the TRBE is enabled */ if (!(read_sysreg_s(SYS_TRBLIMITR_EL1) & TRBLIMITR_EL1_E))
return;
return false;While you're refactoring this code, please move the zeroing of *trfcr_el1 under the if statement.
/* * Prohibit trace generation while we are in guest. * Since access to TRFCR_EL1 is trapped, the guest can't @@ -68,6 +72,8 @@ static void __debug_save_trace(u64 *trfcr_el1) isb(); /* Drain the trace buffer to memory */ tsb_csync();
- return true;
} static void __debug_restore_trace(u64 trfcr_el1) @@ -79,14 +85,55 @@ static void __debug_restore_trace(u64 trfcr_el1) write_sysreg_s(trfcr_el1, SYS_TRFCR_EL1); } +#if IS_ENABLED(CONFIG_PERF_EVENTS)
As previously stated, just always compile this. There shouldn't be anything here that's so large that it becomes a candidate for exclusion. Hell, even the whole of NV+pKVM are permanent features, even of most people won't use *any* of that.
+static inline void __debug_save_trfcr(struct kvm_vcpu *vcpu) +{
- u64 trfcr;
- struct kvm_etm_event etm_event = vcpu->arch.host_debug_state.etm_event;
- /* No change if neither are excluded */
- if (!etm_event.exclude_guest && !etm_event.exclude_host) {
/* Zeroing prevents restoring a stale value */vcpu->arch.host_debug_state.trfcr_el1 = 0;I find this "zero means do nothing" part very odd. I can see it is already done, but I really dislike this sort of assumption to avoid writing to a register.
I'd really prefer we track another version of TRFCR_EL1, compare host and guest, and decide to avoid writing if they are equal. At least, it would be readable.
And in the end, expressing *everything* in terms of the register would really help, instead of the exclude_* stuff that has no place in the low-level arch code.
Yep, I agree with all of the above, I can make these changes for the next version. I just want to clarify your point about disabling trace for protected guests when not in debug mode that I asked about in the review on patch 1.
Thanks,
M.