Re: [RFC PATCH 2/3] arm64: KVM: Support exclude_guest for Coresight trace in nVHE

[James Clark <james.clark@arm.com>]

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.
>