| Message ID | 20240613061731.3109448-7-anshuman.khandual@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64/perf: Enable branch stack sampling | expand |
On Thu, Jun 13, 2024 at 11:47:28AM +0530, 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. It'd be good to explain why we need to do this for nVHE, but not for VHE. I *think* that you're relying on BRBCR_EL2.EL0HBRE being ignored when HCR_EL2.TGE == 0, and BRBCR_EL1.E{1,0}BRE being initialized to 0 out-of-reset. What should a user do if they *want* samples from a guest? Is that possible to do on other architectures, or do is that always prevented? Mark. > > 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 V18: > > - Used host_data_ptr() to access host_debug_state.brbcr_el1 register > - Changed DEBUG_STATE_SAVE_BRBE to use BIT(7) > - Reverted back iflags as u8 > > arch/arm64/include/asm/kvm_host.h | 3 +++ > arch/arm64/kvm/debug.c | 5 +++++ > arch/arm64/kvm/hyp/nvhe/debug-sr.c | 31 ++++++++++++++++++++++++++++++ > 3 files changed, 39 insertions(+) > > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h > index 36b8e97bf49e..db922c10bd2a 100644 > --- a/arch/arm64/include/asm/kvm_host.h > +++ b/arch/arm64/include/asm/kvm_host.h > @@ -579,6 +579,7 @@ struct kvm_host_data { > u64 trfcr_el1; > /* Values of trap registers for the host before guest entry. */ > u64 mdcr_el2; > + u64 brbcr_el1; > } host_debug_state; > }; > > @@ -842,6 +843,8 @@ struct kvm_vcpu_arch { > #define DEBUG_STATE_SAVE_SPE __vcpu_single_flag(iflags, BIT(5)) > /* Save TRBE context if active */ > #define DEBUG_STATE_SAVE_TRBE __vcpu_single_flag(iflags, BIT(6)) > +/* Save BRBE context if active */ > +#define DEBUG_STATE_SAVE_BRBE __vcpu_single_flag(iflags, BIT(7)) > > /* 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 ce8886122ed3..8fa648943f0f 100644 > --- a/arch/arm64/kvm/debug.c > +++ b/arch/arm64/kvm/debug.c > @@ -336,10 +336,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 53efda0235cf..97e861df1b45 100644 > --- a/arch/arm64/kvm/hyp/nvhe/debug-sr.c > +++ b/arch/arm64/kvm/hyp/nvhe/debug-sr.c > @@ -79,6 +79,32 @@ static void __debug_restore_trace(u64 trfcr_el1) > write_sysreg_el1(trfcr_el1, SYS_TRFCR); > } > > +static void __debug_save_brbe(u64 *brbcr_el1) > +{ > + *brbcr_el1 = 0; > + > + /* Check if the BRBE is enabled */ > + if (!(read_sysreg_el1(SYS_BRBCR) & (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_el1(SYS_BRBCR); > + write_sysreg_el1(0, SYS_BRBCR); > +} > + > +static void __debug_restore_brbe(u64 brbcr_el1) > +{ > + if (!brbcr_el1) > + return; > + > + /* Restore BRBE controls */ > + write_sysreg_el1(brbcr_el1, SYS_BRBCR); > +} > + > void __debug_save_host_buffers_nvhe(struct kvm_vcpu *vcpu) > { > /* Disable and flush SPE data generation */ > @@ -87,6 +113,9 @@ void __debug_save_host_buffers_nvhe(struct kvm_vcpu *vcpu) > /* Disable and flush Self-Hosted Trace generation */ > if (vcpu_get_flag(vcpu, DEBUG_STATE_SAVE_TRBE)) > __debug_save_trace(host_data_ptr(host_debug_state.trfcr_el1)); > + /* Disable BRBE branch records */ > + if (vcpu_get_flag(vcpu, DEBUG_STATE_SAVE_BRBE)) > + __debug_save_brbe(host_data_ptr(host_debug_state.brbcr_el1)); > } > > void __debug_switch_to_guest(struct kvm_vcpu *vcpu) > @@ -100,6 +129,8 @@ void __debug_restore_host_buffers_nvhe(struct kvm_vcpu *vcpu) > __debug_restore_spe(*host_data_ptr(host_debug_state.pmscr_el1)); > if (vcpu_get_flag(vcpu, DEBUG_STATE_SAVE_TRBE)) > __debug_restore_trace(*host_data_ptr(host_debug_state.trfcr_el1)); > + if (vcpu_get_flag(vcpu, DEBUG_STATE_SAVE_BRBE)) > + __debug_restore_brbe(*host_data_ptr(host_debug_state.brbcr_el1)); > } > > void __debug_switch_to_host(struct kvm_vcpu *vcpu) > -- > 2.25.1 >
On 6/14/24 20:53, Mark Rutland wrote: > On Thu, Jun 13, 2024 at 11:47:28AM +0530, 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. > > It'd be good to explain why we need to do this for nVHE, but not for > VHE. I *think* that you're relying on BRBCR_EL2.EL0HBRE being ignored > when HCR_EL2.TGE == 0, and BRBCR_EL1.E{1,0}BRE being initialized to 0 > out-of-reset. That's right, there is no possibility for the host and guest BRBE config to overlap. > > What should a user do if they *want* samples from a guest? Is that That is not supported currently. But in order to enable capturing guest branch samples from inside the host - BRBCR_EL2 configs need to migrate into BRBCR_EL1 when the guest runs on the cpu. > possible to do on other architectures, or do is that always prevented? I am not sure about other architectures, but for now this falls within guest support which might be looked into later. But is not the proposed patch complete in itself without any further guest support ? > > Mark. > >> >> 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 V18: >> >> - Used host_data_ptr() to access host_debug_state.brbcr_el1 register >> - Changed DEBUG_STATE_SAVE_BRBE to use BIT(7) >> - Reverted back iflags as u8 >> >> arch/arm64/include/asm/kvm_host.h | 3 +++ >> arch/arm64/kvm/debug.c | 5 +++++ >> arch/arm64/kvm/hyp/nvhe/debug-sr.c | 31 ++++++++++++++++++++++++++++++ >> 3 files changed, 39 insertions(+) >> >> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h >> index 36b8e97bf49e..db922c10bd2a 100644 >> --- a/arch/arm64/include/asm/kvm_host.h >> +++ b/arch/arm64/include/asm/kvm_host.h >> @@ -579,6 +579,7 @@ struct kvm_host_data { >> u64 trfcr_el1; >> /* Values of trap registers for the host before guest entry. */ >> u64 mdcr_el2; >> + u64 brbcr_el1; >> } host_debug_state; >> }; >> >> @@ -842,6 +843,8 @@ struct kvm_vcpu_arch { >> #define DEBUG_STATE_SAVE_SPE __vcpu_single_flag(iflags, BIT(5)) >> /* Save TRBE context if active */ >> #define DEBUG_STATE_SAVE_TRBE __vcpu_single_flag(iflags, BIT(6)) >> +/* Save BRBE context if active */ >> +#define DEBUG_STATE_SAVE_BRBE __vcpu_single_flag(iflags, BIT(7)) >> >> /* 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 ce8886122ed3..8fa648943f0f 100644 >> --- a/arch/arm64/kvm/debug.c >> +++ b/arch/arm64/kvm/debug.c >> @@ -336,10 +336,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 53efda0235cf..97e861df1b45 100644 >> --- a/arch/arm64/kvm/hyp/nvhe/debug-sr.c >> +++ b/arch/arm64/kvm/hyp/nvhe/debug-sr.c >> @@ -79,6 +79,32 @@ static void __debug_restore_trace(u64 trfcr_el1) >> write_sysreg_el1(trfcr_el1, SYS_TRFCR); >> } >> >> +static void __debug_save_brbe(u64 *brbcr_el1) >> +{ >> + *brbcr_el1 = 0; >> + >> + /* Check if the BRBE is enabled */ >> + if (!(read_sysreg_el1(SYS_BRBCR) & (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_el1(SYS_BRBCR); >> + write_sysreg_el1(0, SYS_BRBCR); >> +} >> + >> +static void __debug_restore_brbe(u64 brbcr_el1) >> +{ >> + if (!brbcr_el1) >> + return; >> + >> + /* Restore BRBE controls */ >> + write_sysreg_el1(brbcr_el1, SYS_BRBCR); >> +} >> + >> void __debug_save_host_buffers_nvhe(struct kvm_vcpu *vcpu) >> { >> /* Disable and flush SPE data generation */ >> @@ -87,6 +113,9 @@ void __debug_save_host_buffers_nvhe(struct kvm_vcpu *vcpu) >> /* Disable and flush Self-Hosted Trace generation */ >> if (vcpu_get_flag(vcpu, DEBUG_STATE_SAVE_TRBE)) >> __debug_save_trace(host_data_ptr(host_debug_state.trfcr_el1)); >> + /* Disable BRBE branch records */ >> + if (vcpu_get_flag(vcpu, DEBUG_STATE_SAVE_BRBE)) >> + __debug_save_brbe(host_data_ptr(host_debug_state.brbcr_el1)); >> } >> >> void __debug_switch_to_guest(struct kvm_vcpu *vcpu) >> @@ -100,6 +129,8 @@ void __debug_restore_host_buffers_nvhe(struct kvm_vcpu *vcpu) >> __debug_restore_spe(*host_data_ptr(host_debug_state.pmscr_el1)); >> if (vcpu_get_flag(vcpu, DEBUG_STATE_SAVE_TRBE)) >> __debug_restore_trace(*host_data_ptr(host_debug_state.trfcr_el1)); >> + if (vcpu_get_flag(vcpu, DEBUG_STATE_SAVE_BRBE)) >> + __debug_restore_brbe(*host_data_ptr(host_debug_state.brbcr_el1)); >> } >> >> void __debug_switch_to_host(struct kvm_vcpu *vcpu) >> -- >> 2.25.1 >>
On Mon, Jun 17, 2024 at 12:15:15PM +0530, Anshuman Khandual wrote: > > > On 6/14/24 20:53, Mark Rutland wrote: > > On Thu, Jun 13, 2024 at 11:47:28AM +0530, 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. > > > > It'd be good to explain why we need to do this for nVHE, but not for > > VHE. I *think* that you're relying on BRBCR_EL2.EL0HBRE being ignored > > when HCR_EL2.TGE == 0, and BRBCR_EL1.E{1,0}BRE being initialized to 0 > > out-of-reset. > > That's right, there is no possibility for the host and guest BRBE config > to overlap. > > > What should a user do if they *want* samples from a guest? Is that > > That is not supported currently. But in order to enable capturing guest > branch samples from inside the host - BRBCR_EL2 configs need to migrate > into BRBCR_EL1 when the guest runs on the cpu. > > > possible to do on other architectures, or do is that always prevented? > > I am not sure about other architectures, but for now this falls within > guest support which might be looked into later. But is not the proposed > patch complete in itself without any further guest support ? My concern here is ABI rather than actual support. It's not clear to me how this works across architectures, and we should have some idea of how this would work (e.g. if we're going to require new ABI or not), so that we don't have to break ABI later on. Mark. > > > > > Mark. > > > >> > >> 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 V18: > >> > >> - Used host_data_ptr() to access host_debug_state.brbcr_el1 register > >> - Changed DEBUG_STATE_SAVE_BRBE to use BIT(7) > >> - Reverted back iflags as u8 > >> > >> arch/arm64/include/asm/kvm_host.h | 3 +++ > >> arch/arm64/kvm/debug.c | 5 +++++ > >> arch/arm64/kvm/hyp/nvhe/debug-sr.c | 31 ++++++++++++++++++++++++++++++ > >> 3 files changed, 39 insertions(+) > >> > >> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h > >> index 36b8e97bf49e..db922c10bd2a 100644 > >> --- a/arch/arm64/include/asm/kvm_host.h > >> +++ b/arch/arm64/include/asm/kvm_host.h > >> @@ -579,6 +579,7 @@ struct kvm_host_data { > >> u64 trfcr_el1; > >> /* Values of trap registers for the host before guest entry. */ > >> u64 mdcr_el2; > >> + u64 brbcr_el1; > >> } host_debug_state; > >> }; > >> > >> @@ -842,6 +843,8 @@ struct kvm_vcpu_arch { > >> #define DEBUG_STATE_SAVE_SPE __vcpu_single_flag(iflags, BIT(5)) > >> /* Save TRBE context if active */ > >> #define DEBUG_STATE_SAVE_TRBE __vcpu_single_flag(iflags, BIT(6)) > >> +/* Save BRBE context if active */ > >> +#define DEBUG_STATE_SAVE_BRBE __vcpu_single_flag(iflags, BIT(7)) > >> > >> /* 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 ce8886122ed3..8fa648943f0f 100644 > >> --- a/arch/arm64/kvm/debug.c > >> +++ b/arch/arm64/kvm/debug.c > >> @@ -336,10 +336,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 53efda0235cf..97e861df1b45 100644 > >> --- a/arch/arm64/kvm/hyp/nvhe/debug-sr.c > >> +++ b/arch/arm64/kvm/hyp/nvhe/debug-sr.c > >> @@ -79,6 +79,32 @@ static void __debug_restore_trace(u64 trfcr_el1) > >> write_sysreg_el1(trfcr_el1, SYS_TRFCR); > >> } > >> > >> +static void __debug_save_brbe(u64 *brbcr_el1) > >> +{ > >> + *brbcr_el1 = 0; > >> + > >> + /* Check if the BRBE is enabled */ > >> + if (!(read_sysreg_el1(SYS_BRBCR) & (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_el1(SYS_BRBCR); > >> + write_sysreg_el1(0, SYS_BRBCR); > >> +} > >> + > >> +static void __debug_restore_brbe(u64 brbcr_el1) > >> +{ > >> + if (!brbcr_el1) > >> + return; > >> + > >> + /* Restore BRBE controls */ > >> + write_sysreg_el1(brbcr_el1, SYS_BRBCR); > >> +} > >> + > >> void __debug_save_host_buffers_nvhe(struct kvm_vcpu *vcpu) > >> { > >> /* Disable and flush SPE data generation */ > >> @@ -87,6 +113,9 @@ void __debug_save_host_buffers_nvhe(struct kvm_vcpu *vcpu) > >> /* Disable and flush Self-Hosted Trace generation */ > >> if (vcpu_get_flag(vcpu, DEBUG_STATE_SAVE_TRBE)) > >> __debug_save_trace(host_data_ptr(host_debug_state.trfcr_el1)); > >> + /* Disable BRBE branch records */ > >> + if (vcpu_get_flag(vcpu, DEBUG_STATE_SAVE_BRBE)) > >> + __debug_save_brbe(host_data_ptr(host_debug_state.brbcr_el1)); > >> } > >> > >> void __debug_switch_to_guest(struct kvm_vcpu *vcpu) > >> @@ -100,6 +129,8 @@ void __debug_restore_host_buffers_nvhe(struct kvm_vcpu *vcpu) > >> __debug_restore_spe(*host_data_ptr(host_debug_state.pmscr_el1)); > >> if (vcpu_get_flag(vcpu, DEBUG_STATE_SAVE_TRBE)) > >> __debug_restore_trace(*host_data_ptr(host_debug_state.trfcr_el1)); > >> + if (vcpu_get_flag(vcpu, DEBUG_STATE_SAVE_BRBE)) > >> + __debug_restore_brbe(*host_data_ptr(host_debug_state.brbcr_el1)); > >> } > >> > >> void __debug_switch_to_host(struct kvm_vcpu *vcpu) > >> -- > >> 2.25.1 > >>
On 6/17/24 15:09, Mark Rutland wrote: > On Mon, Jun 17, 2024 at 12:15:15PM +0530, Anshuman Khandual wrote: >> >> >> On 6/14/24 20:53, Mark Rutland wrote: >>> On Thu, Jun 13, 2024 at 11:47:28AM +0530, 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. >>> >>> It'd be good to explain why we need to do this for nVHE, but not for >>> VHE. I *think* that you're relying on BRBCR_EL2.EL0HBRE being ignored >>> when HCR_EL2.TGE == 0, and BRBCR_EL1.E{1,0}BRE being initialized to 0 >>> out-of-reset. >> >> That's right, there is no possibility for the host and guest BRBE config >> to overlap. >> >>> What should a user do if they *want* samples from a guest? Is that >> >> That is not supported currently. But in order to enable capturing guest >> branch samples from inside the host - BRBCR_EL2 configs need to migrate >> into BRBCR_EL1 when the guest runs on the cpu. >> >>> possible to do on other architectures, or do is that always prevented? >> >> I am not sure about other architectures, but for now this falls within >> guest support which might be looked into later. But is not the proposed >> patch complete in itself without any further guest support ? > > My concern here is ABI rather than actual support. I am trying to understand how this is an ABI problem. Because perf debug tools could be described as - a best effort based sample collection. All samples that could be collected for a given perf_event_attr request might change if the underlying assumptions change later on. AFAICT semantics of expectations for a given attribute request is not a hard ABI requirement. > > It's not clear to me how this works across architectures, and we should > have some idea of how this would work (e.g. if we're going to require > new ABI or not), so that we don't have to break ABI later on. BRBE HW does not have any guest filter in itself, unless BRBCR_EL2 gets migrated across BRBCR_EL1 during guest transition, guest branch records would not be captured. event->attr.exclude_guest = 0 could have been denied during armpmu_add() for preventing events with guest branch sample requests being scheduled on the PMU. But it turns out to be not a very reliable parameter in that sense as well. event->attr.exclude_guest = 0 remains clear even for a standard session. ./perf record -e instructions:k -j any_call,save_type ls perf tools will need some changes in order to avoid the above scenarios as a default behaviour which would not be desirable as well. These semantics could be worked out later on when BRBE guest support gets included, and the current proposal would not prevent any potential future changes in this regard. > > Mark.
On Thu, Jun 20, 2024 at 09:52:05AM +0530, Anshuman Khandual wrote: > On 6/17/24 15:09, Mark Rutland wrote: > > On Mon, Jun 17, 2024 at 12:15:15PM +0530, Anshuman Khandual wrote: > >> On 6/14/24 20:53, Mark Rutland wrote: > >>> On Thu, Jun 13, 2024 at 11:47:28AM +0530, 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. > >>> > >>> It'd be good to explain why we need to do this for nVHE, but not for > >>> VHE. I *think* that you're relying on BRBCR_EL2.EL0HBRE being ignored > >>> when HCR_EL2.TGE == 0, and BRBCR_EL1.E{1,0}BRE being initialized to 0 > >>> out-of-reset. > >> > >> That's right, there is no possibility for the host and guest BRBE config > >> to overlap. > >> > >>> What should a user do if they *want* samples from a guest? Is that > >> > >> That is not supported currently. But in order to enable capturing guest > >> branch samples from inside the host - BRBCR_EL2 configs need to migrate > >> into BRBCR_EL1 when the guest runs on the cpu. > >> > >>> possible to do on other architectures, or do is that always prevented? > >> > >> I am not sure about other architectures, but for now this falls within > >> guest support which might be looked into later. But is not the proposed > >> patch complete in itself without any further guest support ? > > > > My concern here is ABI rather than actual support. > I am trying to understand how this is an ABI problem. Because perf debug > tools could be described as - a best effort based sample collection. All > samples that could be collected for a given perf_event_attr request might > change if the underlying assumptions change later on. AFAICT semantics of > expectations for a given attribute request is not a hard ABI requirement. The ABI requirements are certainly unclear, but people get *very* upset when behaviour changes, so I think we need to have some certainty that we're not backing ourselves into a corner where we have to make substantial behavioural changes later. Surely we can figure out how this works on other architectures today? There's a substantial argument for aligning with x86, so can we figure out under which conditions x86 would provide guest samples? e.g. is that always, never, or when certain attr options are configured? > > It's not clear to me how this works across architectures, and we should > > have some idea of how this would work (e.g. if we're going to require > > new ABI or not), so that we don't have to break ABI later on. > > BRBE HW does not have any guest filter in itself, unless BRBCR_EL2 gets > migrated across BRBCR_EL1 during guest transition, guest branch records > would not be captured. > > event->attr.exclude_guest = 0 could have been denied during armpmu_add() > for preventing events with guest branch sample requests being scheduled > on the PMU. But it turns out to be not a very reliable parameter in that > sense as well. > > event->attr.exclude_guest = 0 remains clear even for a standard session. > > ./perf record -e instructions:k -j any_call,save_type ls > > perf tools will need some changes in order to avoid the above scenarios > as a default behaviour which would not be desirable as well. If we're liable to need perf tool changes, then we *definitely* need to understand this better. > These semantics could be worked out later on when BRBE guest support gets > included, and the current proposal would not prevent any potential future > changes in this regard. That depends entirely on what changes we'd expect would be necessary in the perf tools. We need to be certain that we don't enable some use case that subseuqent changes break. Mark.
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h index 36b8e97bf49e..db922c10bd2a 100644 --- a/arch/arm64/include/asm/kvm_host.h +++ b/arch/arm64/include/asm/kvm_host.h @@ -579,6 +579,7 @@ struct kvm_host_data { u64 trfcr_el1; /* Values of trap registers for the host before guest entry. */ u64 mdcr_el2; + u64 brbcr_el1; } host_debug_state; }; @@ -842,6 +843,8 @@ struct kvm_vcpu_arch { #define DEBUG_STATE_SAVE_SPE __vcpu_single_flag(iflags, BIT(5)) /* Save TRBE context if active */ #define DEBUG_STATE_SAVE_TRBE __vcpu_single_flag(iflags, BIT(6)) +/* Save BRBE context if active */ +#define DEBUG_STATE_SAVE_BRBE __vcpu_single_flag(iflags, BIT(7)) /* 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 ce8886122ed3..8fa648943f0f 100644 --- a/arch/arm64/kvm/debug.c +++ b/arch/arm64/kvm/debug.c @@ -336,10 +336,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 53efda0235cf..97e861df1b45 100644 --- a/arch/arm64/kvm/hyp/nvhe/debug-sr.c +++ b/arch/arm64/kvm/hyp/nvhe/debug-sr.c @@ -79,6 +79,32 @@ static void __debug_restore_trace(u64 trfcr_el1) write_sysreg_el1(trfcr_el1, SYS_TRFCR); } +static void __debug_save_brbe(u64 *brbcr_el1) +{ + *brbcr_el1 = 0; + + /* Check if the BRBE is enabled */ + if (!(read_sysreg_el1(SYS_BRBCR) & (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_el1(SYS_BRBCR); + write_sysreg_el1(0, SYS_BRBCR); +} + +static void __debug_restore_brbe(u64 brbcr_el1) +{ + if (!brbcr_el1) + return; + + /* Restore BRBE controls */ + write_sysreg_el1(brbcr_el1, SYS_BRBCR); +} + void __debug_save_host_buffers_nvhe(struct kvm_vcpu *vcpu) { /* Disable and flush SPE data generation */ @@ -87,6 +113,9 @@ void __debug_save_host_buffers_nvhe(struct kvm_vcpu *vcpu) /* Disable and flush Self-Hosted Trace generation */ if (vcpu_get_flag(vcpu, DEBUG_STATE_SAVE_TRBE)) __debug_save_trace(host_data_ptr(host_debug_state.trfcr_el1)); + /* Disable BRBE branch records */ + if (vcpu_get_flag(vcpu, DEBUG_STATE_SAVE_BRBE)) + __debug_save_brbe(host_data_ptr(host_debug_state.brbcr_el1)); } void __debug_switch_to_guest(struct kvm_vcpu *vcpu) @@ -100,6 +129,8 @@ void __debug_restore_host_buffers_nvhe(struct kvm_vcpu *vcpu) __debug_restore_spe(*host_data_ptr(host_debug_state.pmscr_el1)); if (vcpu_get_flag(vcpu, DEBUG_STATE_SAVE_TRBE)) __debug_restore_trace(*host_data_ptr(host_debug_state.trfcr_el1)); + if (vcpu_get_flag(vcpu, DEBUG_STATE_SAVE_BRBE)) + __debug_restore_brbe(*host_data_ptr(host_debug_state.brbcr_el1)); } void __debug_switch_to_host(struct kvm_vcpu *vcpu)
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 V18: - Used host_data_ptr() to access host_debug_state.brbcr_el1 register - Changed DEBUG_STATE_SAVE_BRBE to use BIT(7) - Reverted back iflags as u8 arch/arm64/include/asm/kvm_host.h | 3 +++ arch/arm64/kvm/debug.c | 5 +++++ arch/arm64/kvm/hyp/nvhe/debug-sr.c | 31 ++++++++++++++++++++++++++++++ 3 files changed, 39 insertions(+)