Re: [PATCH V5 6/7] arm64/perf: Add BRBE driver

[James Clark]

On 30/11/2022 04:49, Anshuman Khandual wrote:
> 
> 
> On 11/29/22 21:23, James Clark wrote:
[...]
> 
> The latest code (not posted), disables TRBE completely while reading the
> branch records during PMU interrupt. Could you please apply those changes
> as well, or rather just use the branch instead.
> 
> https://gitlab.arm.com/linux-arm/linux-anshuman/-/commit/ab17879711f0e61c280ed52400ccde172b67e04a
> 
>>
>> armv8pmu_brbe_enable+0xc/arm64_pmu_brbe_enable+0x0/P/-/-/0/CALL
>> armpmu_start+0xe0/armv8pmu_brbe_enable+0x0/P/-/-/0/IND_CALL
>> armv8pmu_brbe_reset+0x18/armpmu_start+0xd0/P/-/-/0/RET
>> arm64_pmu_brbe_reset+0x18/armv8pmu_brbe_reset+0x10/P/-/-/0/RET
> 
> I am wondering how the different privilege branch record leak is possible.
> Because arm64_pmu_brbe_enable() should start BRBE from a clean state with
> respect to privilege level filters.
> 
> void arm64_pmu_brbe_enable(struct pmu_hw_events *cpuc)
> {
>         u64 brbfcr, brbcr;
> 
>         if (brbe_disabled(cpuc))
>                 return;
> 
>         brbfcr = read_sysreg_s(SYS_BRBFCR_EL1);
>         brbfcr &= ~BRBFCR_EL1_BANK_MASK;
>         brbfcr &= ~(BRBFCR_EL1_EnI | BRBFCR_EL1_PAUSED | BRBE_FCR_MASK);
>         brbfcr |= (cpuc->brbfcr & BRBE_FCR_MASK);
>         write_sysreg_s(brbfcr, SYS_BRBFCR_EL1);
>         isb();
> 
>         brbcr = read_sysreg_s(SYS_BRBCR_EL1);
>         brbcr &= ~BRBE_CR_MASK;			--> Contains BRBCR_EL1_E1BRE and BRBCR_EL1_E0BRE
>         brbcr |= BRBCR_EL1_FZP;
>         brbcr |= (BRBCR_EL1_TS_PHYSICAL << BRBCR_EL1_TS_SHIFT);
>         brbcr |= (cpuc->brbcr & BRBE_CR_MASK);
>         write_sysreg_s(brbcr, SYS_BRBCR_EL1);
>         isb();

Yes I tracked down the problem to here as well. I added a
arm64_pmu_brbe_reset(cpuc) to the end of arm64_pmu_brbe_enable() and it
fixes the issue.

The problem is, without ensuring that BRBFCR_EL1_PAUSED is set, there is
no way to write to both BRBFCR and BRBCR in either order without new
records being produced based on a partial configuration.

BRBFCR_EL1_PAUSED isn't set from the previous session, and there is no
obvious place to add a paused at the end of the session with the current
callbacks. So the easiest fix is to flush the old kernel samples after
configuring both registers.

I'm not sure what the BRBFCR_EL1_PAUSED value is at power on either, so
the issue might also be present with the very first brbe session, but
less obvious than a userspace one following a kernel one. But the flush
solves that problem too.

> }
> 
> Could these samples are from a previous session ? But they should have been
> flushed in armpmu_start().
> 
> static void armpmu_start(struct perf_event *event, int flags)
> {
> 	.........
>         if (has_branch_stack(event)) {
>                 if (event->ctx->task && hw_events->brbe_context != event->ctx) {
>                         armpmu->brbe_reset(hw_events);
>                         hw_events->brbe_context = event->ctx;
>                 }
>                 armpmu->brbe_enable(hw_events);
>                 hw_events->brbe_users++;
>         }
> 	.........
> }
> 
>>
>>
>> [1]:
>> https://gitlab.arm.com/linux-arm/linux-jc/-/commit/7260b7bef06ac161eac88d05266e8c5c303d9881