[PATCH V1] arm64: perf: Change PMCR write to read-modify-write

[PATCH V1] arm64: perf: Change PMCR write to read-modify-write

[Srinivasarao Pathipati]

Preserve the bitfields of PMCR_EL0 during PMU reset.
Reset routine should set only PMCR.C, PMCR.P and PMCR.LC fields only
to reset the counters. Other fields should not be changed
as they could be set before PMU initialization and their value must
be preserved even after reset.

Signed-off-by: Srinivasarao Pathipati <quic_spathi@quicinc.com>
---
 arch/arm64/kernel/perf_event.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c
index cb69ff1..9e22326 100644
--- a/arch/arm64/kernel/perf_event.c
+++ b/arch/arm64/kernel/perf_event.c
@@ -1047,7 +1047,7 @@ static void armv8pmu_reset(void *info)
 	if (armv8pmu_has_long_event(cpu_pmu))
 		pmcr |= ARMV8_PMU_PMCR_LP;
 
-	armv8pmu_pmcr_write(pmcr);
+	armv8pmu_pmcr_write(armv8pmu_pmcr_read() | pmcr);
 }
 
 static int __armv8_pmuv3_map_event(struct perf_event *event,
-- 
2.7.4

Re: [PATCH V1] arm64: perf: Change PMCR write to read-modify-write

[Robin Murphy]

On 2022-04-27 10:51, Srinivasarao Pathipati wrote:
> Preserve the bitfields of PMCR_EL0 during PMU reset.
> Reset routine should set only PMCR.C, PMCR.P and PMCR.LC fields only
> to reset the counters. Other fields should not be changed
> as they could be set before PMU initialization and their value must
> be preserved even after reset.

No. We also want to ensure PMCR.E and PMCR.D are set to 0, for example. 
Given that nearly all the writeable fields in PMCR reset to an 
architecturally UNKNOWN value, preserving that is clearly nonsense. 
What's your *real* motivation here?

Thanks,
Robin.

> Signed-off-by: Srinivasarao Pathipati <quic_spathi@quicinc.com>
> ---
>   arch/arm64/kernel/perf_event.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c
> index cb69ff1..9e22326 100644
> --- a/arch/arm64/kernel/perf_event.c
> +++ b/arch/arm64/kernel/perf_event.c
> @@ -1047,7 +1047,7 @@ static void armv8pmu_reset(void *info)
>   	if (armv8pmu_has_long_event(cpu_pmu))
>   		pmcr |= ARMV8_PMU_PMCR_LP;
>   
> -	armv8pmu_pmcr_write(pmcr);
> +	armv8pmu_pmcr_write(armv8pmu_pmcr_read() | pmcr);
>   }
>   
>   static int __armv8_pmuv3_map_event(struct perf_event *event,

Re: [PATCH V1] arm64: perf: Change PMCR write to read-modify-write

[Srinivasarao Pathipati (Consultant)]

>
>On 2022-04-27 10:51, Srinivasarao Pathipati wrote:
>> Preserve the bitfields of PMCR_EL0 during PMU reset.
>> Reset routine should set only PMCR.C, PMCR.P and PMCR.LC fields only
>> to reset the counters. Other fields should not be changed
>> as they could be set before PMU initialization and their value must
>> be preserved even after reset.
>
>No. We also want to ensure PMCR.E and PMCR.D are set to 0, for example.
>Given that nearly all the writeable fields in PMCR reset to an
>architecturally UNKNOWN value, preserving that is clearly nonsense.
>What's your *real* motivation here?

Thanks Robin for reviewing this patch.
The X bit is set by firmware on Qualcomm chipsets 
same is getting cleared by kernel from armv8pmu_reset().
We are trying to retain this bit with this patch.

If it is wrong to retaining all bits? can we just retain X bit?

Re: [PATCH V1] arm64: perf: Change PMCR write to read-modify-write

[Robin Murphy]

On 2022-04-27 12:09, Srinivasarao Pathipati (Consultant) wrote:
>>
>> On 2022-04-27 10:51, Srinivasarao Pathipati wrote:
>>> Preserve the bitfields of PMCR_EL0 during PMU reset.
>>> Reset routine should set only PMCR.C, PMCR.P and PMCR.LC fields only
>>> to reset the counters. Other fields should not be changed
>>> as they could be set before PMU initialization and their value must
>>> be preserved even after reset.
>>
>> No. We also want to ensure PMCR.E and PMCR.D are set to 0, for example.
>> Given that nearly all the writeable fields in PMCR reset to an
>> architecturally UNKNOWN value, preserving that is clearly nonsense.
>> What's your *real* motivation here?
> 
> Thanks Robin for reviewing this patch.
> The X bit is set by firmware on Qualcomm chipsets
> same is getting cleared by kernel from armv8pmu_reset().
> We are trying to retain this bit with this patch.

OK, that's fair enough, but anything firmware may have done is not 
really relevant once Linux has taken over, since firmware is no longer 
in control of the PMU. As mentioned, at this point the driver doesn't 
know *why* any bits are set the way they are - maybe they were 
deliberately configured by platform firmware; maybe the hardware just 
randomly reset to that value; maybe another OS kexec'ed with the PMU 
still running and configured in who-knows-what state; we simply can't 
reason about it, so we have to configure the PMU into a known state for 
how we're going to use it.

> If it is wrong to retaining all bits? can we just retain X bit?

If it's useful for PMU events under Linux to be exported where 
applicable, then it seems reasonable for Linux to enable PMCR.X for 
itself. Certainly there doesn't seem to be any obvious reason *not* to, 
other than perhaps some small power cost, but I guess it could always be 
user-controlled if we wanted to be particularly cautious.

Thanks,
Robin.