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

[Anshuman Khandual <anshuman.khandual@arm.com>]

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