Messages in this thread | ![/](/images/icornerl.gif) | | Date | Tue, 30 Jan 2024 09:11:30 +0530 | Subject | Re: [PATCH V16 5/8] KVM: arm64: nvhe: Disable branch generation in nVHE guests | From | 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@kernel.org> >> Cc: Oliver Upton <oliver.upton@linux.dev> >> Cc: James Morse <james.morse@arm.com> >> Cc: Suzuki K Poulose <suzuki.poulose@arm.com> >> Cc: Catalin Marinas <catalin.marinas@arm.com> >> Cc: Will Deacon <will@kernel.org> >> Cc: kvmarm@lists.linux.dev >> Cc: linux-arm-kernel@lists.infradead.org >> CC: linux-kernel@vger.kernel.org >> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com> >> --- >> 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.
| ![\](/images/icornerr.gif) |