| Message ID | 20230804101317.460697-3-james.clark@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | coresight: Support exclude_guest with Feat_TRF and nVHE | expand |
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. Thanks, M.
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. >
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; /* * 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) +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; + return; + } + + trfcr = read_sysreg_s(SYS_TRFCR_EL1); + vcpu->arch.host_debug_state.trfcr_el1 = trfcr; + + if (etm_event.exclude_guest) { + trfcr &= ~(TRFCR_ELx_ExTRE | TRFCR_ELx_E0TRE); + } else { + /* + * If host was excluded then EL0 and ELx tracing bits will + * already be cleared so they need to be set now for the guest. + */ + trfcr |= etm_event.exclude_kernel ? 0 : TRFCR_ELx_ExTRE; + trfcr |= etm_event.exclude_user ? 0 : TRFCR_ELx_E0TRE; + } + write_sysreg_s(trfcr, SYS_TRFCR_EL1); +} +#else +static inline void __debug_save_trfcr(struct kvm_vcpu *vcpu) {} +#endif + void __debug_save_host_buffers_nvhe(struct kvm_vcpu *vcpu) { + bool trc_disabled = false; + /* Disable and flush SPE data generation */ if (vcpu_get_flag(vcpu, DEBUG_STATE_SAVE_SPE)) __debug_save_spe(&vcpu->arch.host_debug_state.pmscr_el1); /* Disable and flush Self-Hosted Trace generation */ if (vcpu_get_flag(vcpu, DEBUG_STATE_SAVE_TRBE)) - __debug_save_trace(&vcpu->arch.host_debug_state.trfcr_el1); + trc_disabled = __debug_save_trace(&vcpu->arch.host_debug_state.trfcr_el1); + + /* + * As long as trace wasn't completely disabled due to use of TRBE, + * TRFCR can be saved and the exclude_guest rules applied. + */ + if (!trc_disabled && vcpu_get_flag(vcpu, DEBUG_STATE_SAVE_TRFCR)) + __debug_save_trfcr(vcpu); } void __debug_switch_to_guest(struct kvm_vcpu *vcpu) @@ -98,7 +145,8 @@ void __debug_restore_host_buffers_nvhe(struct kvm_vcpu *vcpu) { if (vcpu_get_flag(vcpu, DEBUG_STATE_SAVE_SPE)) __debug_restore_spe(vcpu->arch.host_debug_state.pmscr_el1); - if (vcpu_get_flag(vcpu, DEBUG_STATE_SAVE_TRBE)) + if (vcpu_get_flag(vcpu, DEBUG_STATE_SAVE_TRBE) || + vcpu_get_flag(vcpu, DEBUG_STATE_SAVE_TRFCR)) __debug_restore_trace(vcpu->arch.host_debug_state.trfcr_el1); }
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(-)