Re: [PATCH V16 5/8] KVM: arm64: nvhe: Disable branch generation in nVHE guests

[Anshuman Khandual]

On 3/1/24 00:10, Marc Zyngier wrote:
> On Thu, 25 Jan 2024 09:41:16 +0000,
> Anshuman Khandual <anshuman.khandual@xxxxxxx> wrote:
>>
>> Disable the BRBE before we enter the guest, saving the status and enable it
>> back once we get out of the guest. This avoids capturing branch records in
>> the guest kernel or userspace, which would be confusing the host samples.
>>
>> Cc: Marc Zyngier <maz@xxxxxxxxxx>
>> Cc: Oliver Upton <oliver.upton@xxxxxxxxx>
>> Cc: James Morse <james.morse@xxxxxxx>
>> Cc: Suzuki K Poulose <suzuki.poulose@xxxxxxx>
>> Cc: Catalin Marinas <catalin.marinas@xxxxxxx>
>> Cc: Will Deacon <will@xxxxxxxxxx>
>> Cc: kvmarm@xxxxxxxxxxxxxxx
>> Cc: linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
>> CC: linux-kernel@xxxxxxxxxxxxxxx
>> Signed-off-by: Anshuman Khandual <anshuman.khandual@xxxxxxx>
>> ---
>> Changes in V16:
>>
>> - Dropped BRBCR_EL1 and BRBFCR_EL1 from enum vcpu_sysreg
>> - Reverted back the KVM NVHE patch - used host_debug_state based 'brbcr_el1'
>>   element, and dropped the previous dependency on Jame's coresight series
>>
>>  arch/arm64/include/asm/kvm_host.h  |  5 ++++-
>>  arch/arm64/kvm/debug.c             |  5 +++++
>>  arch/arm64/kvm/hyp/nvhe/debug-sr.c | 33 ++++++++++++++++++++++++++++++
>>  3 files changed, 42 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
>> index 21c57b812569..bce8792092af 100644
>> --- a/arch/arm64/include/asm/kvm_host.h
>> +++ b/arch/arm64/include/asm/kvm_host.h
>> @@ -569,7 +569,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;
>> @@ -610,6 +610,7 @@ struct kvm_vcpu_arch {
>>  		u64 pmscr_el1;
>>  		/* Self-hosted trace */
>>  		u64 trfcr_el1;
>> +		u64 brbcr_el1;
>>  	} host_debug_state;
>>  
>>  	/* VGIC state */
>> @@ -779,6 +780,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 BRBE context if active  */
>> +#define DEBUG_STATE_SAVE_BRBE	__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..99f85d8acbf3 100644
>> --- a/arch/arm64/kvm/debug.c
>> +++ b/arch/arm64/kvm/debug.c
>> @@ -335,10 +335,15 @@ 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);
>> +
>> +	/* Check if we have BRBE implemented and available at the host */
>> +	if (cpuid_feature_extract_unsigned_field(dfr0, ID_AA64DFR0_EL1_BRBE_SHIFT))
>> +		vcpu_set_flag(vcpu, DEBUG_STATE_SAVE_BRBE);
>>  }
>>  
>>  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_BRBE);
>>  }
>> diff --git a/arch/arm64/kvm/hyp/nvhe/debug-sr.c b/arch/arm64/kvm/hyp/nvhe/debug-sr.c
>> index 4558c02eb352..79bcf0fb1326 100644
>> --- a/arch/arm64/kvm/hyp/nvhe/debug-sr.c
>> +++ b/arch/arm64/kvm/hyp/nvhe/debug-sr.c
>> @@ -79,6 +79,34 @@ static void __debug_restore_trace(u64 trfcr_el1)
>>  	write_sysreg_s(trfcr_el1, SYS_TRFCR_EL1);
>>  }
>>  
>> +static void __debug_save_brbe(u64 *brbcr_el1)
>> +{
>> +	*brbcr_el1 = 0;
>> +
>> +	/* Check if the BRBE is enabled */
>> +	if (!(read_sysreg_s(SYS_BRBCR_EL1) & (BRBCR_ELx_E0BRE | BRBCR_ELx_ExBRE)))
>> +		return;
>> +
>> +	/*
>> +	 * Prohibit branch record generation while we are in guest.
>> +	 * Since access to BRBCR_EL1 is trapped, the guest can't
>> +	 * modify the filtering set by the host.
>> +	 */
>> +	*brbcr_el1 = read_sysreg_s(SYS_BRBCR_EL1);
>> +	write_sysreg_s(0, SYS_BRBCR_EL1);
> 
> As for TRFCR and PMSCR, this is broken on hVHE.
> 
> Please see [1]
> 
> 	M.
> 
> [1] https://lore.kernel.org/r/20240229145417.3606279-1-maz@xxxxxxxxxx
> 

Ahh I see, so the unified accessors read_sysreg_el1()/write_sysreg_el1()
need to be used here - which will choose between BRBCR_EL1 & BRBCR_EL12
as required. Will do the changes, thanks for pointing out.