On 11/14/23 15:28, James Clark wrote:
>
>
> On 14/11/2023 05:13, Anshuman Khandual wrote:
>> Branch stack sampling support i.e capturing branch records during execution
>> in core perf, rides along with normal HW events being scheduled on the PMU.
>> This prepares ARMV8 PMU framework for branch stack support on relevant PMUs
>> with required HW implementation.
>>
>
> [...]
>
>> - All armv8pmu_branch_xxxx() stub definitions have been moved inside
>> include/linux/perf/arm_pmuv3.h for easy access from both arm32 and
>> arm64 platforms
>>
>
> This causes lots of W=1 build errors because the prototypes are in
> arm_pmuv3.h, but arm_brbe.c doesn't include it.
I guess these are the W=1 warnings you mentioned above.
drivers/perf/arm_brbe.c:11:6: warning: no previous prototype for ‘armv8pmu_branch_reset’ [-Wmissing-prototypes]
11 | void armv8pmu_branch_reset(void)
| ^~~~~~~~~~~~~~~~~~~~~
drivers/perf/arm_brbe.c:190:6: warning: no previous prototype for ‘armv8pmu_branch_save’ [-Wmissing-prototypes]
190 | void armv8pmu_branch_save(struct arm_pmu *arm_pmu, void *ctx)
| ^~~~~~~~~~~~~~~~~~~~
drivers/perf/arm_brbe.c:236:6: warning: no previous prototype for ‘armv8pmu_branch_attr_valid’ [-Wmissing-prototypes]
236 | bool armv8pmu_branch_attr_valid(struct perf_event *event)
| ^~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/perf/arm_brbe.c:269:5: warning: no previous prototype for ‘armv8pmu_task_ctx_cache_alloc’ [-Wmissing-prototypes]
269 | int armv8pmu_task_ctx_cache_alloc(struct arm_pmu *arm_pmu)
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/perf/arm_brbe.c:279:6: warning: no previous prototype for ‘armv8pmu_task_ctx_cache_free’ [-Wmissing-prototypes]
279 | void armv8pmu_task_ctx_cache_free(struct arm_pmu *arm_pmu)
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/perf/arm_brbe.c:303:6: warning: no previous prototype for ‘armv8pmu_branch_probe’ [-Wmissing-prototypes]
303 | void armv8pmu_branch_probe(struct arm_pmu *armpmu)
| ^~~~~~~~~~~~~~~~~~~~~
drivers/perf/arm_brbe.c:449:6: warning: no previous prototype for ‘armv8pmu_branch_enable’ [-Wmissing-prototypes]
449 | void armv8pmu_branch_enable(struct arm_pmu *arm_pmu)
| ^~~~~~~~~~~~~~~~~~~~~~
drivers/perf/arm_brbe.c:474:6: warning: no previous prototype for ‘armv8pmu_branch_disable’ [-Wmissing-prototypes]
474 | void armv8pmu_branch_disable(void)
| ^~~~~~~~~~~~~~~~~~~~~~~
drivers/perf/arm_brbe.c:717:6: warning: no previous prototype for ‘armv8pmu_branch_read’ [-Wmissing-prototypes]
717 | void armv8pmu_branch_read(struct pmu_hw_events *cpuc, struct perf_event *event)
Branch helpers are used in ARM PMU V3 driver i.e drivers/perf/arm_pmuv3.c.
Whether the actual BRBE helper definitions, or their fallback stubs (when
CONFIG_ARM64_BRBE is not enabled), need to be accessible from arm_pmuv3.c
driver not from brbe.c implementations itself.
>
> It seems like the main reason you can't include arm_brbe.h in arm32 code
> is because there are a load of inline functions and references to
> registers in there. But these are only used in arm_brbe.c, so they don't
Right, arm32 should not be exposed to BRBE internals via arm_brbe.h header.
> need to be in the header anyway.
Right, these are only used in arm_brbe.c
>
> If you removed the code from the header and moved it to the source file
> you could move the brbe prototypes to the brbe header and it would be a
> bit cleaner and more idiomatic.
Alight, how about the following changes - build tested on arm32 and arm64.
- Move BRBE helpers from arm_brbe.h into arm_brbe.c
- Move armv8_pmu_xxx() declaration inside arm_brbe.h for arm64 (CONFIG_ARM64_BRBE)
- Move armv8_pmu_xxx() stub definitions inside arm_pmuv3.c for arm32 (!CONFIG_ARM64_BRBE)
- Include arm_brbe.h header both in arm_pmuv3.c and arm_brbe.c
On 15/11/2023 05:44, Anshuman Khandual wrote:
> On 11/14/23 15:28, James Clark wrote:
>>
>>
>> On 14/11/2023 05:13, Anshuman Khandual wrote:
>>> Branch stack sampling support i.e capturing branch records during execution
>>> in core perf, rides along with normal HW events being scheduled on the PMU.
>>> This prepares ARMV8 PMU framework for branch stack support on relevant PMUs
>>> with required HW implementation.
>>>
>>
>> [...]
>>
>>> - All armv8pmu_branch_xxxx() stub definitions have been moved inside
>>> include/linux/perf/arm_pmuv3.h for easy access from both arm32 and
>>> arm64 platforms
>>>
>>
>> This causes lots of W=1 build errors because the prototypes are in
>> arm_pmuv3.h, but arm_brbe.c doesn't include it.
>
> I guess these are the W=1 warnings you mentioned above.
>
> drivers/perf/arm_brbe.c:11:6: warning: no previous prototype for ‘armv8pmu_branch_reset’ [-Wmissing-prototypes]
> 11 | void armv8pmu_branch_reset(void)
> | ^~~~~~~~~~~~~~~~~~~~~
> drivers/perf/arm_brbe.c:190:6: warning: no previous prototype for ‘armv8pmu_branch_save’ [-Wmissing-prototypes]
> 190 | void armv8pmu_branch_save(struct arm_pmu *arm_pmu, void *ctx)
> | ^~~~~~~~~~~~~~~~~~~~
> drivers/perf/arm_brbe.c:236:6: warning: no previous prototype for ‘armv8pmu_branch_attr_valid’ [-Wmissing-prototypes]
> 236 | bool armv8pmu_branch_attr_valid(struct perf_event *event)
> | ^~~~~~~~~~~~~~~~~~~~~~~~~~
> drivers/perf/arm_brbe.c:269:5: warning: no previous prototype for ‘armv8pmu_task_ctx_cache_alloc’ [-Wmissing-prototypes]
> 269 | int armv8pmu_task_ctx_cache_alloc(struct arm_pmu *arm_pmu)
> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> drivers/perf/arm_brbe.c:279:6: warning: no previous prototype for ‘armv8pmu_task_ctx_cache_free’ [-Wmissing-prototypes]
> 279 | void armv8pmu_task_ctx_cache_free(struct arm_pmu *arm_pmu)
> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
> drivers/perf/arm_brbe.c:303:6: warning: no previous prototype for ‘armv8pmu_branch_probe’ [-Wmissing-prototypes]
> 303 | void armv8pmu_branch_probe(struct arm_pmu *armpmu)
> | ^~~~~~~~~~~~~~~~~~~~~
> drivers/perf/arm_brbe.c:449:6: warning: no previous prototype for ‘armv8pmu_branch_enable’ [-Wmissing-prototypes]
> 449 | void armv8pmu_branch_enable(struct arm_pmu *arm_pmu)
> | ^~~~~~~~~~~~~~~~~~~~~~
> drivers/perf/arm_brbe.c:474:6: warning: no previous prototype for ‘armv8pmu_branch_disable’ [-Wmissing-prototypes]
> 474 | void armv8pmu_branch_disable(void)
> | ^~~~~~~~~~~~~~~~~~~~~~~
> drivers/perf/arm_brbe.c:717:6: warning: no previous prototype for ‘armv8pmu_branch_read’ [-Wmissing-prototypes]
> 717 | void armv8pmu_branch_read(struct pmu_hw_events *cpuc, struct perf_event *event)
>
> Branch helpers are used in ARM PMU V3 driver i.e drivers/perf/arm_pmuv3.c.
> Whether the actual BRBE helper definitions, or their fallback stubs (when
> CONFIG_ARM64_BRBE is not enabled), need to be accessible from arm_pmuv3.c
> driver not from brbe.c implementations itself.
>
>>
>> It seems like the main reason you can't include arm_brbe.h in arm32 code
>> is because there are a load of inline functions and references to
>> registers in there. But these are only used in arm_brbe.c, so they don't
>
> Right, arm32 should not be exposed to BRBE internals via arm_brbe.h header.
>
>> need to be in the header anyway.
>
> Right, these are only used in arm_brbe.c
>
>>
>> If you removed the code from the header and moved it to the source file
>> you could move the brbe prototypes to the brbe header and it would be a
>> bit cleaner and more idiomatic.
>
> Alight, how about the following changes - build tested on arm32 and arm64.
>
> - Move BRBE helpers from arm_brbe.h into arm_brbe.c
> - Move armv8_pmu_xxx() declaration inside arm_brbe.h for arm64 (CONFIG_ARM64_BRBE)
> - Move armv8_pmu_xxx() stub definitions inside arm_pmuv3.c for arm32 (!CONFIG_ARM64_BRBE)
> - Include arm_brbe.h header both in arm_pmuv3.c and arm_brbe.c
Agree to them all except:
- Move armv8_pmu_xxx() stub definitions inside arm_pmuv3.c for arm32
(!CONFIG_ARM64_BRBE)
Normally you put the stubs right next to the prototypes with #else, so
in this case both would be in arm_brbe.h. Not sure what the reason for
splitting them here is? You already said "include arm_brbe.h in
arm_pmuv3.c", so that covers arm32 too.
On 11/15/23 15:07, James Clark wrote:
>
>
> On 15/11/2023 05:44, Anshuman Khandual wrote:
>> On 11/14/23 15:28, James Clark wrote:
>>>
>>>
>>> On 14/11/2023 05:13, Anshuman Khandual wrote:
>>>> Branch stack sampling support i.e capturing branch records during execution
>>>> in core perf, rides along with normal HW events being scheduled on the PMU.
>>>> This prepares ARMV8 PMU framework for branch stack support on relevant PMUs
>>>> with required HW implementation.
>>>>
>>>
>>> [...]
>>>
>>>> - All armv8pmu_branch_xxxx() stub definitions have been moved inside
>>>> include/linux/perf/arm_pmuv3.h for easy access from both arm32 and
>>>> arm64 platforms
>>>>
>>>
>>> This causes lots of W=1 build errors because the prototypes are in
>>> arm_pmuv3.h, but arm_brbe.c doesn't include it.
>>
>> I guess these are the W=1 warnings you mentioned above.
>>
>> drivers/perf/arm_brbe.c:11:6: warning: no previous prototype for ‘armv8pmu_branch_reset’ [-Wmissing-prototypes]
>> 11 | void armv8pmu_branch_reset(void)
>> | ^~~~~~~~~~~~~~~~~~~~~
>> drivers/perf/arm_brbe.c:190:6: warning: no previous prototype for ‘armv8pmu_branch_save’ [-Wmissing-prototypes]
>> 190 | void armv8pmu_branch_save(struct arm_pmu *arm_pmu, void *ctx)
>> | ^~~~~~~~~~~~~~~~~~~~
>> drivers/perf/arm_brbe.c:236:6: warning: no previous prototype for ‘armv8pmu_branch_attr_valid’ [-Wmissing-prototypes]
>> 236 | bool armv8pmu_branch_attr_valid(struct perf_event *event)
>> | ^~~~~~~~~~~~~~~~~~~~~~~~~~
>> drivers/perf/arm_brbe.c:269:5: warning: no previous prototype for ‘armv8pmu_task_ctx_cache_alloc’ [-Wmissing-prototypes]
>> 269 | int armv8pmu_task_ctx_cache_alloc(struct arm_pmu *arm_pmu)
>> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> drivers/perf/arm_brbe.c:279:6: warning: no previous prototype for ‘armv8pmu_task_ctx_cache_free’ [-Wmissing-prototypes]
>> 279 | void armv8pmu_task_ctx_cache_free(struct arm_pmu *arm_pmu)
>> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> drivers/perf/arm_brbe.c:303:6: warning: no previous prototype for ‘armv8pmu_branch_probe’ [-Wmissing-prototypes]
>> 303 | void armv8pmu_branch_probe(struct arm_pmu *armpmu)
>> | ^~~~~~~~~~~~~~~~~~~~~
>> drivers/perf/arm_brbe.c:449:6: warning: no previous prototype for ‘armv8pmu_branch_enable’ [-Wmissing-prototypes]
>> 449 | void armv8pmu_branch_enable(struct arm_pmu *arm_pmu)
>> | ^~~~~~~~~~~~~~~~~~~~~~
>> drivers/perf/arm_brbe.c:474:6: warning: no previous prototype for ‘armv8pmu_branch_disable’ [-Wmissing-prototypes]
>> 474 | void armv8pmu_branch_disable(void)
>> | ^~~~~~~~~~~~~~~~~~~~~~~
>> drivers/perf/arm_brbe.c:717:6: warning: no previous prototype for ‘armv8pmu_branch_read’ [-Wmissing-prototypes]
>> 717 | void armv8pmu_branch_read(struct pmu_hw_events *cpuc, struct perf_event *event)
>>
>> Branch helpers are used in ARM PMU V3 driver i.e drivers/perf/arm_pmuv3.c.
>> Whether the actual BRBE helper definitions, or their fallback stubs (when
>> CONFIG_ARM64_BRBE is not enabled), need to be accessible from arm_pmuv3.c
>> driver not from brbe.c implementations itself.
>>
>>>
>>> It seems like the main reason you can't include arm_brbe.h in arm32 code
>>> is because there are a load of inline functions and references to
>>> registers in there. But these are only used in arm_brbe.c, so they don't
>>
>> Right, arm32 should not be exposed to BRBE internals via arm_brbe.h header.
>>
>>> need to be in the header anyway.
>>
>> Right, these are only used in arm_brbe.c
>>
>>>
>>> If you removed the code from the header and moved it to the source file
>>> you could move the brbe prototypes to the brbe header and it would be a
>>> bit cleaner and more idiomatic.
>>
>> Alight, how about the following changes - build tested on arm32 and arm64.
>>
>> - Move BRBE helpers from arm_brbe.h into arm_brbe.c
>> - Move armv8_pmu_xxx() declaration inside arm_brbe.h for arm64 (CONFIG_ARM64_BRBE)
>> - Move armv8_pmu_xxx() stub definitions inside arm_pmuv3.c for arm32 (!CONFIG_ARM64_BRBE)
>> - Include arm_brbe.h header both in arm_pmuv3.c and arm_brbe.c
>
> Agree to them all except:
>
> - Move armv8_pmu_xxx() stub definitions inside arm_pmuv3.c for arm32
> (!CONFIG_ARM64_BRBE)
>
> Normally you put the stubs right next to the prototypes with #else, so
> in this case both would be in arm_brbe.h. Not sure what the reason for
> splitting them here is? You already said "include arm_brbe.h in
> arm_pmuv3.c", so that covers arm32 too.
Not any particular strong reason for the split as such, will move these
stubs to the header as well. BRBE header includes <linux/perf/arm_pmu.h>
which causes the following redefinition warning for the pr_fmt().
drivers/perf/arm_brbe.c:11: warning: "pr_fmt" redefined
11 | #define pr_fmt(fmt) "brbe: " fmt
|
In file included from ./include/linux/kernel.h:31,
from ./include/linux/interrupt.h:6,
from ./include/linux/perf/arm_pmu.h:11,
from drivers/perf/arm_brbe.h:10,
from drivers/perf/arm_brbe.c:9:
./include/linux/printk.h:345: note: this is the location of the previous definition
345 | #define pr_fmt(fmt) fmt
Although it should be okay to just drop this custom pr_fmt() from BRBE.