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 <[email protected]>
---
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;
/*
* 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);
}
--
2.34.1
On Fri, 04 Aug 2023 11:13:12 +0100,
James Clark <[email protected]> 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 <[email protected]>
> ---
> 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.
--
Without deviation from the norm, progress is not possible.
On 08/08/2023 12:04, Marc Zyngier wrote:
> On Fri, 04 Aug 2023 11:13:12 +0100,
> James Clark <[email protected]> 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 <[email protected]>
>> ---
>> 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.
>