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

[Anshuman Khandual]

On 1/29/24 17:50, Suzuki K Poulose wrote:
> On 25/01/2024 09:41, Anshuman Khandual 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);
>> +    isb();
> 
> Is this isb() required here ? This can be synchronised with the Guest entry ?
> 
>> +}
>> +
>> +static void __debug_restore_brbe(u64 brbcr_el1)
>> +{
>> +    if (!brbcr_el1)
>> +        return;
>> +
>> +    /* Restore BRBE controls */
>> +    write_sysreg_s(brbcr_el1, SYS_BRBCR_EL1);
>> +    isb();
> 
> Similarly here, exit back to EL1 host can synchronise the setting ?

Sure, will drop both the isb() here.