linux-kernel - Re: [PATCH] driver: perf: arm_pmuv3: Read PMMIR

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: <999f01f4-4f3f-4a55-ae43-7316c46581c5@arm.com>
Date:   Thu, 12 Oct 2023 16:28:28 +0530
From:   Anshuman Khandual <anshuman.khandual@....com>
To:     Mark Rutland <mark.rutland@....com>,
        James Clark <james.clark@....com>
Cc:     linux-arm-kernel@...ts.infradead.org, zhangshaokun@...ilicon.com,
        Will Deacon <will@...nel.org>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] driver: perf: arm_pmuv3: Read PMMIR_EL1 unconditionally



On 10/12/23 14:54, Mark Rutland wrote:
> On Mon, Oct 09, 2023 at 09:59:19AM +0100, James Clark wrote:
>> On 09/10/2023 08:56, Anshuman Khandual wrote:
>>> PMMIR_EL1 needs to be captured in 'armpmu->reg_pmmir', for all appropriate
>>> PMU version implementations where the register is available and reading it
>>> is valid . Hence checking for bus slot event presence is redundant and can
>>> be dropped.
>>>
>>> Cc: Will Deacon <will@...nel.org>
>>> Cc: Mark Rutland <mark.rutland@....com>
>>> Cc: linux-arm-kernel@...ts.infradead.org
>>> Cc: linux-kernel@...r.kernel.org
>>> Signed-off-by: Anshuman Khandual <anshuman.khandual@....com>
>>> ---
>>> This applies on v6.6-rc5.
>>>  
>>>  drivers/perf/arm_pmuv3.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/perf/arm_pmuv3.c b/drivers/perf/arm_pmuv3.c
>>> index 1e72b486c033..9fc1b6da5106 100644
>>> --- a/drivers/perf/arm_pmuv3.c
>>> +++ b/drivers/perf/arm_pmuv3.c
>>> @@ -1129,7 +1129,7 @@ static void __armv8pmu_probe_pmu(void *info)
>>>  			     pmceid, ARMV8_PMUV3_MAX_COMMON_EVENTS);
>>>  
>>>  	/* store PMMIR register for sysfs */
>>> -	if (is_pmuv3p4(pmuver) && (pmceid_raw[1] & BIT(31)))
>>> +	if (is_pmuv3p4(pmuver))
>>>  		cpu_pmu->reg_pmmir = read_pmmir();
>>>  	else
>>>  		cpu_pmu->reg_pmmir = 0;
>>
>>
>> This does have the side effect of showing non-zero values in caps/slots
>> even when the STALL_SLOT event isn't implemented. I think that's the
>> scenario that the original commit (f5be3a61fd) was trying to avoid:
>>
>>   /sys/bus/event_source/devices/armv8_pmuv3_0/caps/slots is exposed
>>   under sysfs. [If] Both ARMv8.4-PMU and STALL_SLOT event are
>>   implemented, it returns the slots from PMMIR_EL1, otherwise it will
>>   return 0.
> 
> We check for the STALL_SLOT event becuase (at the time) the ARM ARM said:
> 
> | If STALL_SLOT is not implemented, it is IMPLEMENTATION DEFINED whether the
> | PMMIR System registers are implemented.
> 
> ... and this was necessary to avoid triggering an UNDEFINED exception if we
> attempted to read PMMIR on a CPU which didn't actually implement it.
> 
> See: 
> 
>   https://lore.kernel.org/linux-arm-kernel/20200720101518.GA11516@willie-the-truck/
>   https://lore.kernel.org/linux-arm-kernel/20200720105019.GA54220@C02TD0UTHF1T.local/
>   https://lore.kernel.org/linux-arm-kernel/20200720105410.GD11516@willie-the-truck/
> 
> As I promised in that thread, I did raise that with our architects. According
> to the bug I filed against the architecture, this was tightened such that
> ARMv8.4-PMU gauaranteed the presence of PMMIR, and that should have changed
> between the G.a and G.b releases of the ARM ARM.
> 
> Anshuman, can you go and check that the wording did chaange between G.a and G.b?

G.a was the last ARM ARM version to have that STALL_SLOT event dependency for
PMMIR_EL1 system register which got dropped in G.b version ARM ARM.

> 
> Assuming it did (and the wording in the latest J.a release is also fine),
> please update the commit message to describe the history above.

Sure, will update the commit message as follows.

    driver: perf: arm_pmuv3: Read PMMIR_EL1 unconditionally
    
    PMMIR_EL1 needs to be captured in 'armpmu->reg_pmmir', for all appropriate
    PMU version implementations i.e FEAT_PMUv3p4 onward, where the register is
    available and reading it is valid. However STALL_SLOT event dependency was
    included earlier as per previous version ARM ARM wordings [1]. But later
    i.e ARM ARM version G.a onward, STALL_SLOT event dependency for PMMIR_EL1
    register has been dropped. Checking for that dependency is now redundant,
    and should be dropped.
    
    [1] https://lore.kernel.org/linux-arm-kernel/20200720105410.GD11516@willie-the-truck/