Messages in this thread | | | Date | Tue, 21 Nov 2023 16:42:18 +0530 | Subject | Re: [V14 5/8] KVM: arm64: nvhe: Disable branch generation in nVHE guests | From | Anshuman Khandual <> |
| |
On 11/14/23 14:46, James Clark wrote: > > > On 14/11/2023 05:13, 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 is just to avoid capturing records >> in the guest kernel/userspace, which would be confusing the 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 V14: >> >> - This is a new patch in the series >> >> arch/arm64/include/asm/kvm_host.h | 4 ++++ >> arch/arm64/kvm/debug.c | 6 +++++ >> arch/arm64/kvm/hyp/nvhe/debug-sr.c | 38 ++++++++++++++++++++++++++++++ >> 3 files changed, 48 insertions(+) >> >> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h >> index 68421c74283a..1faa0430d8dd 100644 >> --- a/arch/arm64/include/asm/kvm_host.h >> +++ b/arch/arm64/include/asm/kvm_host.h >> @@ -449,6 +449,8 @@ enum vcpu_sysreg { >> CNTHV_CVAL_EL2, >> PMSCR_EL1, /* Statistical profiling extension */ >> TRFCR_EL1, /* Self-hosted trace filters */ >> + BRBCR_EL1, /* Branch Record Buffer Control Register */ >> + BRBFCR_EL1, /* Branch Record Buffer Function Control Register */ >> >> NR_SYS_REGS /* Nothing after this line! */ >> }; >> @@ -753,6 +755,8 @@ struct kvm_vcpu_arch { >> #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)) >> +/* Save BRBE context if active */ >> +#define DEBUG_STATE_SAVE_BRBE __vcpu_single_flag(iflags, BIT(9)) >> >> /* 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 2ab41b954512..4055783c3d34 100644 >> --- a/arch/arm64/kvm/debug.c >> +++ b/arch/arm64/kvm/debug.c >> @@ -354,6 +354,11 @@ void kvm_arch_vcpu_load_debug_state_flags(struct kvm_vcpu *vcpu) >> !(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) && >> + (read_sysreg_s(SYS_BRBCR_EL1) & (BRBCR_ELx_E0BRE | BRBCR_ELx_ExBRE))) >> + vcpu_set_flag(vcpu, DEBUG_STATE_SAVE_BRBE); > > Isn't this supposed to just be the feature check? Whether BRBE is > enabled or not is checked later in __debug_save_brbe() anyway.
Okay, will make it just a feature check via ID_AA64DFR0_EL1_BRBE_SHIFT.
> > It seems like it's possible to become enabled after this flag load part.
Agreed.
> >> } >> >> void kvm_arch_vcpu_put_debug_state_flags(struct kvm_vcpu *vcpu) >> @@ -361,6 +366,7 @@ 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); >> + vcpu_clear_flag(vcpu, DEBUG_STATE_SAVE_BRBE); >> } >> >> void kvm_etm_set_guest_trfcr(u64 trfcr_guest) >> diff --git a/arch/arm64/kvm/hyp/nvhe/debug-sr.c b/arch/arm64/kvm/hyp/nvhe/debug-sr.c >> index 6174f710948e..e44a1f71a0f8 100644 >> --- a/arch/arm64/kvm/hyp/nvhe/debug-sr.c >> +++ b/arch/arm64/kvm/hyp/nvhe/debug-sr.c >> @@ -93,6 +93,38 @@ static void __debug_restore_trace(struct kvm_cpu_context *host_ctxt, >> write_sysreg_s(ctxt_sys_reg(host_ctxt, TRFCR_EL1), SYS_TRFCR_EL1); >> } >> >> +static void __debug_save_brbe(struct kvm_cpu_context *host_ctxt) >> +{ >> + ctxt_sys_reg(host_ctxt, BRBCR_EL1) = 0; >> + ctxt_sys_reg(host_ctxt, BRBFCR_EL1) = 0; >> + >> + /* Check if the BRBE is enabled */ >> + if (!(ctxt_sys_reg(host_ctxt, BRBCR_EL1) & (BRBCR_ELx_E0BRE | BRBCR_ELx_ExBRE))) >> + return; > > Doesn't this always fail, the host BRBCR_EL1 value was just cleared on > the line above.
Agreed, this error might have slipped in while converting to ctxt_sys_reg().
> > Also, you need to read the register to determine if it was enabled or
Right
> not, so you might as well always store the real value, rather than 0 in > the not enabled case.
But if it is not enabled - why store the real value ?
> >> + >> + /* >> + * Prohibit branch record generation while we are in guest. >> + * Since access to BRBCR_EL1 and BRBFCR_EL1 is trapped, the >> + * guest can't modify the filtering set by the host. >> + */ >> + ctxt_sys_reg(host_ctxt, BRBCR_EL1) = read_sysreg_s(SYS_BRBCR_EL1); >> + ctxt_sys_reg(host_ctxt, BRBFCR_EL1) = read_sysreg_s(SYS_BRBFCR_EL1) >> + write_sysreg_s(0, SYS_BRBCR_EL1); >> + write_sysreg_s(0, SYS_BRBFCR_EL1); > > Why does SYS_BRBFCR_EL1 need to be saved and restored? Only > BRBCR_ELx_E0BRE and BRBCR_ELx_ExBRE need to be cleared to disable BRBE.
Right, just thought both brbcr, and brbfcr system registers represent current BRBE state (besides branch records), in a more comprehensive manner, although none would be changed from inside the guest.
> >> + isb(); >> +} >> + >> +static void __debug_restore_brbe(struct kvm_cpu_context *host_ctxt) >> +{ >> + if (!ctxt_sys_reg(host_ctxt, BRBCR_EL1) || !ctxt_sys_reg(host_ctxt, BRBFCR_EL1)) >> + return; >> + >> + /* Restore BRBE controls */ >> + write_sysreg_s(ctxt_sys_reg(host_ctxt, BRBCR_EL1), SYS_BRBCR_EL1); >> + write_sysreg_s(ctxt_sys_reg(host_ctxt, BRBFCR_EL1), SYS_BRBFCR_EL1); >> + isb(); >> +} >> + >> void __debug_save_host_buffers_nvhe(struct kvm_cpu_context *host_ctxt, >> struct kvm_cpu_context *guest_ctxt) >> { >> @@ -102,6 +134,10 @@ void __debug_save_host_buffers_nvhe(struct kvm_cpu_context *host_ctxt, >> >> if (vcpu_get_flag(host_ctxt->__hyp_running_vcpu, DEBUG_STATE_SAVE_TRFCR)) >> __debug_save_trace(host_ctxt, guest_ctxt); >> + >> + /* Disable BRBE branch records */ >> + if (vcpu_get_flag(host_ctxt->__hyp_running_vcpu, DEBUG_STATE_SAVE_BRBE)) >> + __debug_save_brbe(host_ctxt); >> } >> >> void __debug_switch_to_guest(struct kvm_vcpu *vcpu) >> @@ -116,6 +152,8 @@ void __debug_restore_host_buffers_nvhe(struct kvm_cpu_context *host_ctxt, >> __debug_restore_spe(host_ctxt); >> if (vcpu_get_flag(host_ctxt->__hyp_running_vcpu, DEBUG_STATE_SAVE_TRFCR)) >> __debug_restore_trace(host_ctxt, guest_ctxt); >> + if (vcpu_get_flag(host_ctxt->__hyp_running_vcpu, DEBUG_STATE_SAVE_BRBE)) >> + __debug_restore_brbe(host_ctxt); >> } >> >> void __debug_switch_to_host(struct kvm_vcpu *vcpu)
| |