Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.34 as permitted sender) client-ip=23.128.96.34;
Message-ID: <999f01f4-4f3f-4a55-ae43-7316c46581c5@arm.com>
Date:   Thu, 12 Oct 2023 16:28:28 +0530
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] driver: perf: arm_pmuv3: Read PMMIR_EL1 unconditionally
To:     Mark Rutland <mark.rutland@arm.com>,
        James Clark <james.clark@arm.com>
Cc:     linux-arm-kernel@lists.infradead.org, zhangshaokun@hisilicon.com,
        Will Deacon <will@kernel.org>, linux-kernel@vger.kernel.org
References: <20231009075631.193208-1-anshuman.khandual@arm.com>
 <ffb41c00-1df8-e4bb-deff-c2d1cfb15ec0@arm.com>
 <ZSe7XnMPOpIO-VIF@FVFF77S0Q05N>
Content-Language: en-US
From:   Anshuman Khandual <anshuman.khandual@arm.com>
In-Reply-To: <ZSe7XnMPOpIO-VIF@FVFF77S0Q05N>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk


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@kernel.org>
>>> Cc: Mark Rutland <mark.rutland@arm.com>
>>> Cc: linux-arm-kernel@lists.infradead.org
>>> Cc: linux-kernel@vger.kernel.org
>>> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.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/