Re: [PATCH v3 3/6] arm64: KVM: Add iflag for FEAT_TRF

[Suzuki K Poulose <suzuki.poulose@arm.com>]

On 04/12/2023 09:48, Marc Zyngier wrote:
> On Thu, 19 Oct 2023 17:55:01 +0100,
> James Clark <james.clark@arm.com> wrote:
>>
>> Add an extra iflag to signify if the TRFCR register is accessible.
>> Because TRBE requires FEAT_TRF, DEBUG_STATE_SAVE_TRBE still has the same
>> behavior even though it's only set when FEAT_TRF is present.
>>
>> The following holes are left in struct kvm_vcpu_arch, but there aren't
>> enough other 8 bit fields to rearrange it to leave any hole smaller than
>> 7 bytes:
>>
>>    u8                         cflags;               /*  2292     1 */
>>    /* XXX 1 byte hole, try to pack */
>>    u16                        iflags;               /*  2294     2 */
>>    u8                         sflags;               /*  2296     1 */
>>    bool                       pause;                /*  2297     1 */
>>    /* XXX 6 bytes hole, try to pack */
>>
>> Signed-off-by: James Clark <james.clark@arm.com>
>> ---
>>   arch/arm64/include/asm/kvm_host.h |  4 +++-
>>   arch/arm64/kvm/debug.c            | 22 ++++++++++++++++++----
>>   2 files changed, 21 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
>> index 7c82927ddaf2..0f0bf8e641bd 100644
>> --- a/arch/arm64/include/asm/kvm_host.h
>> +++ b/arch/arm64/include/asm/kvm_host.h
>> @@ -535,7 +535,7 @@ struct kvm_vcpu_arch {
>>   	u8 cflags;
>>   
>>   	/* Input flags to the hypervisor code, potentially cleared after use */
>> -	u8 iflags;
>> +	u16 iflags;
>>   
>>   	/* State flags for kernel bookkeeping, unused by the hypervisor code */
>>   	u8 sflags;
>> @@ -741,6 +741,8 @@ struct kvm_vcpu_arch {
>>   #define DEBUG_STATE_SAVE_TRBE	__vcpu_single_flag(iflags, BIT(6))
>>   /* vcpu running in HYP context */
>>   #define VCPU_HYP_CONTEXT	__vcpu_single_flag(iflags, BIT(7))
>> +/* Save trace filter controls */
>> +#define DEBUG_STATE_SAVE_TRFCR	__vcpu_single_flag(iflags, BIT(8))
>>   
>>   /* SVE enabled for host EL0 */
>>   #define HOST_SVE_ENABLED	__vcpu_single_flag(sflags, BIT(0))
>> diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c
>> index 8725291cb00a..20cdd40b3c42 100644
>> --- a/arch/arm64/kvm/debug.c
>> +++ b/arch/arm64/kvm/debug.c
>> @@ -331,14 +331,28 @@ void kvm_arch_vcpu_load_debug_state_flags(struct kvm_vcpu *vcpu)
>>   	    !(read_sysreg_s(SYS_PMBIDR_EL1) & BIT(PMBIDR_EL1_P_SHIFT)))
>>   		vcpu_set_flag(vcpu, DEBUG_STATE_SAVE_SPE);
>>   
>> -	/* Check if we have TRBE implemented and available at the host */
>> -	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 (TraceFilt) exists. This will be
>> +	 * done in cases where use of TRBE doesn't completely disable trace and
>> +	 * handles the exclude_host/exclude_guest rules of the trace session.
> 
> This comment provides zero information. What will be done? Under which
> conditions? What are the rules?
> 
>> +	 */
>> +	if (cpuid_feature_extract_unsigned_field(dfr0, ID_AA64DFR0_EL1_TraceFilt_SHIFT)) {
>> +		vcpu_set_flag(vcpu, DEBUG_STATE_SAVE_TRFCR);
>> +		/*
>> +		 * Check if we have TRBE implemented and available at the host. If it's
>> +		 * in use at the time of guest switch it will need to be disabled and
>> +		 * then restored. The architecture mandates FEAT_TRF with TRBE, so we
>> +		 * only need to check for TRBE after TRF.
>> +		 */
>> +		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);
>> +	}
> 
> Multiple questions:
> 
> - Why is it safe to trust the local CPU's capability rather than the
>    consolidated view from the cpufeature infrastructure?

The coresight driver is capable of handling heterogeneous systems. i.e., 
some CPUs may not have FEAT_TRF or FEAT_TRBE. This could be for various
reasons (e.g., CPU Erratum disabling TRBE, though not used now). It 
already needs to treat each CPU differently (due to the capabilities
of the ETM).

That said, we could reject events with exclude_guest/exclude_host flags
on CPUs that do not support FEAT_TRF. But that doesn't solve checking
the local capability.

Suzuki


> 
> - Why defer the saving of the registers if there are no changes made
>    to them in the interval?
> 
> Thanks,
> 
> 	M.
>