Re: [PATCH V9 00/10] arm64/perf: Enable branch stack sampling

From: Anshuman Khandual
Date: Thu Mar 23 2023 - 00:26:03 EST


Hello Mark,

On 3/22/23 00:32, Mark Brown wrote:
> On Wed, Mar 15, 2023 at 10:44:34AM +0530, Anshuman Khandual wrote:
>> This series enables perf branch stack sampling support on arm64 platform
>> via a new arch feature called Branch Record Buffer Extension (BRBE). All
>> relevant register definitions could be accessed here.
>>
>> https://developer.arm.com/documentation/ddi0601/2021-12/AArch64-Registers
>
> While looking at another feature I noticed that HFGITR_EL2 has two traps
> for BRBE instructions, nBRBINJ and nBRBIALL which trap BRB INJ and BRB
> IALL. Even if we don't use those right now does it make sense to

Right, current branch stack sampling experiments have been on EL2 host itself.

> document a requirement for those traps to be disabled now in case we
> need them later, and do so during EL2 setup for KVM guests? That could
> always be done incrementally.
Unlike all other instruction trap enable fields in SYS_HFGITR_EL2, these BRBE
instructions ones are actually inverted in semantics i.e the particular fields
need to be set for these traps to be disabled in EL2.

SYS_HFGITR_EL2.nBRBIALL
SYS_HFGITR_EL2.nBRBINJ

By default entire SYS_HFGITR_EL2 is set as cleared during init and that would
prevent a guest from using BRBE.

init_kernel_el()
init_el2()
init_el2_state()
__init_el2_fgt()
........
msr_s SYS_HFGITR_EL2, xzr
........

I guess something like the following (untested) needs to be done, to enable
BRBE in guests.

diff --git a/arch/arm64/include/asm/el2_setup.h b/arch/arm64/include/asm/el2_setup.h
index 037724b19c5c..309708127a2a 100644
--- a/arch/arm64/include/asm/el2_setup.h
+++ b/arch/arm64/include/asm/el2_setup.h
@@ -161,6 +161,15 @@
msr_s SYS_HFGWTR_EL2, x0
msr_s SYS_HFGITR_EL2, xzr

+ mrs x1, id_aa64dfr0_el1
+ ubfx x1, x1, #ID_AA64DFR0_EL1_BRBE_SHIFT, #4
+ cbz x1, .Lskip_brbe_\@
+ mov x0, xzr
+ orr x0, x0, #HFGITR_EL2_nBRBIALL
+ orr x0, x0, #HFGITR_EL2_nBRBINJ
+ msr_s SYS_HFGITR_EL2, x0
+
+.Lskip_brbe_\@:
mrs x1, id_aa64pfr0_el1 // AMU traps UNDEF without AMU
ubfx x1, x1, #ID_AA64PFR0_EL1_AMU_SHIFT, #4
cbz x1, .Lskip_fgt_\@
diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
index b3bc03ee22bd..3b939c42f3b8 100644
--- a/arch/arm64/include/asm/sysreg.h
+++ b/arch/arm64/include/asm/sysreg.h
@@ -527,6 +527,9 @@
#define SYS_HFGITR_EL2 sys_reg(3, 4, 1, 1, 6)
#define SYS_HACR_EL2 sys_reg(3, 4, 1, 1, 7)

+#define HFGITR_EL2_nBRBIALL (BIT(56))
+#define HFGITR_EL2_nBRBINJ (BIT(55))
+
#define SYS_TTBR0_EL2 sys_reg(3, 4, 2, 0, 0)
#define SYS_TTBR1_EL2 sys_reg(3, 4, 2, 0, 1)
#define SYS_TCR_EL2 sys_reg(3, 4, 2, 0, 2)


>
> I've got a patch adding the definition of that register to sysreg which
> I should be sending shortly, no need to duplicate that effort.

Sure, I assume you are moving the existing definition for SYS_HFGITR_EL2 along
with all its fields from ../include/asm/sysreg.h to ../tools/sysreg. Right, it
makes sense.

- Anshuman