linux-kernel - Re: [PATCH V6] arm64: perf: Make exporting of pmu events configurable

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] [day] [month] [year] [list]

Message-ID: <9477d392-aedd-3187-762d-8e19fdc41c29@quicinc.com>
Date:   Wed, 8 Mar 2023 12:29:57 +0530
From:   Srinivasarao Pathipati <quic_c_spathi@...cinc.com>
To:     Will Deacon <will@...nel.org>
CC:     <mark.rutland@....com>, <peterz@...radead.org>, <mingo@...hat.com>,
        <acme@...nel.org>, <alexander.shishkin@...ux.intel.com>,
        <jolsa@...nel.org>, <namhyung@...nel.org>,
        <catalin.marinas@....com>, <linux-arm-kernel@...ts.infradead.org>,
        <linux-perf-users@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH V6] arm64: perf: Make exporting of pmu events configurable


On 7/13/2022 9:35 AM, Srinivasarao Pathipati wrote:
>
> On 6/24/2022 5:48 PM, Will Deacon wrote:
>> On Thu, Jun 09, 2022 at 06:29:49PM +0530, Srinivasarao Pathipati wrote:
>>> The PMU export bit (PMCR_EL0.X) is getting reset during pmu reset,
>>> Make it configurable using sysctls to enable/disable at runtime.
>>>
>>> Signed-off-by: Srinivasarao Pathipati <quic_c_spathi@...cinc.com>
>>> ---
>>> Changes since V5:
>>>     - removed configuring with kernel parameters.
>>> Changes since V4:
>>>     - Registering sysctls dynamically for only arm64 as suggested by 
>>> Will
>>>     - Not removed the code to configure with kernel parameters
>>>       as the sysctl's kernel parameter(sysctl.kernel.export_pmu_events)
>>>       is not working at early bootup. pmu_reset() getting called before
>>>       sysctl's kernel parameter is set.
>>> Changes since V3:
>>>     - export bit is now configurable with sysctl
>>>     - enabling export bit on reset instead of retaining
>>>
>>> Changes since V2:
>>>     Done below changes as per Will's comments
>>>     - enabling pmcr_x now configurable with kernel parameters and
>>>       by default it is disabled.
>>>
>>> Changes since V1:
>>>     - Preserving only PMCR_X bit as per Robin Murphy's comment.
>>>
>>> ---
>>>   Documentation/admin-guide/sysctl/kernel.rst | 11 +++++++++++
>>>   arch/arm64/kernel/perf_event.c              | 13 +++++++++++++
>>>   2 files changed, 24 insertions(+)
>>>
>>> diff --git a/Documentation/admin-guide/sysctl/kernel.rst 
>>> b/Documentation/admin-guide/sysctl/kernel.rst
>>> index ddccd10..c2ecd84 100644
>>> --- a/Documentation/admin-guide/sysctl/kernel.rst
>>> +++ b/Documentation/admin-guide/sysctl/kernel.rst
>>> @@ -267,6 +267,17 @@ domain names are in general different. For a 
>>> detailed discussion
>>>   see the ``hostname(1)`` man page.
>>>     +export_pmu_events (arm64 only)
>>> +==============================
>>> +
>>> +Controls the PMU export bit (PMCR_EL0.X), which enables the 
>>> exporting of
>>> +events over an IMPLEMENTATION DEFINED PMU event export bus to 
>>> another device.
>>> +
>>> +0: disables exporting of events (default).
>>> +
>>> +1: enables exporting of events.
>>> +
>>> +
>>>   firmware_config
>>>   ===============
>>>   diff --git a/arch/arm64/kernel/perf_event.c 
>>> b/arch/arm64/kernel/perf_event.c
>>> index cb69ff1..a8c32a0 100644
>>> --- a/arch/arm64/kernel/perf_event.c
>>> +++ b/arch/arm64/kernel/perf_event.c
>>> @@ -298,6 +298,7 @@ PMU_FORMAT_ATTR(long, "config1:0");
>>>   PMU_FORMAT_ATTR(rdpmc, "config1:1");
>>>     static int sysctl_perf_user_access __read_mostly;
>>> +static int sysctl_export_pmu_events __read_mostly;
>>>     static inline bool armv8pmu_event_is_64bit(struct perf_event 
>>> *event)
>>>   {
>>> @@ -1047,6 +1048,9 @@ static void armv8pmu_reset(void *info)
>>>       if (armv8pmu_has_long_event(cpu_pmu))
>>>           pmcr |= ARMV8_PMU_PMCR_LP;
>>>   +    if (sysctl_export_pmu_events)
>>> +        pmcr |= ARMV8_PMU_PMCR_X;
>> I think we need to do this in armv8pmu_start() rather than 
>> armv8pmu_reset(),
>> otherwise any changes to the sysctl at runtime won't take effect 
>> unless you
>> do something like re-online the CPU.
>>
>> Will
>
> Hi Will ,
>
> We think it may not work.   Say for example pmu->reset is called which 
> disables the exporting of events,
> and pmu->start not called. This would lead to missing of events.
>
> How about enabling it in both reset and start functions? do you see 
> any issue?
>
> --Srinivas

Hi Will ,

Could you please suggest ,better way to fix this.