linux-kernel - Re: [PATCH V2 3/7] arm64/perf: Update struct pmu_hw

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <84683aa7-58ad-85f8-327b-daed2f704834@arm.com>
Date:   Tue, 13 Sep 2022 11:03:45 +0530
From:   Anshuman Khandual <anshuman.khandual@....com>
To:     Mark Brown <broonie@...nel.org>
Cc:     linux-kernel@...r.kernel.org, linux-perf-users@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org, peterz@...radead.org,
        acme@...nel.org, mark.rutland@....com, will@...nel.org,
        catalin.marinas@....com, James Clark <james.clark@....com>,
        Rob Herring <robh@...nel.org>, Marc Zyngier <maz@...nel.org>,
        Ingo Molnar <mingo@...hat.com>
Subject: Re: [PATCH V2 3/7] arm64/perf: Update struct pmu_hw_events for BRBE



On 9/12/22 15:42, Mark Brown wrote:
> On Thu, Sep 08, 2022 at 10:40:42AM +0530, Anshuman Khandual wrote:
> 
>> +	/* Captured BRBE buffer - copied as is into perf_sample_data */
>> +	struct perf_branch_stack	brbe_stack;
>> +	struct perf_branch_entry	brbe_entries[BRBE_MAX_ENTRIES];
> 
> It looks like perf_branch_entry is intended to be the variably
> sized entries array at the end of perf_branch_stack?  That could

That is right. Because max number of entries for brbe_entries[] array
is platform dependent i.e BHRB_MAX_ENTRIES on powerpc, MAX_LBR_ENTRIES
on x86 and BRBE_MAX_ENTRIES on arm64.

The generic definition

struct perf_branch_stack {
        __u64                           nr;
        __u64                           hw_idx;
        struct perf_branch_entry        entries[];
};

On x86 platform

#define MAX_LBR_ENTRIES         32

struct cpu_hw_events {
	....
	struct perf_branch_stack        lbr_stack;
        struct perf_branch_entry        lbr_entries[MAX_LBR_ENTRIES];
	....
}

On powerpc platform

#define BHRB_MAX_ENTRIES        32

struct cpu_hw_events {
	....
        struct  perf_branch_stack       bhrb_stack;
        struct  perf_branch_entry       bhrb_entries[BHRB_MAX_ENTRIES];
	....
}

Followed same format on arm64 platform as well

#define BRBE_MAX_ENTRIES	64

struct pmu_hw_events {
	....
	....
	struct perf_branch_stack	brbe_stack;
	struct perf_branch_entry	brbe_entries[BRBE_MAX_ENTRIES];
	....
	....
}

> probably do with being called out if it's the case.  It feels

Right, we could add a comment in this regard.

> like it would be clearer and safer to allocate these dynamically
> when BRBE is used if that's possible, I'd expect that should also
> deal with the stack frame size issues as well.

That might not be possible because the generic 'struct perf_branch_stack'
expects 'perf_branch_stack.entries' to be a variable array which is also
contiguous in memory, with other elements in 'perf_branch_stack'. Besides
that will be a deviation from similar implementations on x86 and powerpc
platforms.

The stack frame size came up because BRBE_MAX_ENTRIES is 64 compared to
just 32 on other platforms, which follow the exact same method.