RE: [PATCH v3 1/1] target/arm: Trap PMCR when MDCR_EL2.TPMCR is set

[Smail AIDER via <qemu-devel@nongnu.org>]

Hi Philippe,

It is not just some refactoring. The last patch v3 is a squash of two previous patches v1 and v2.
Maybe I need to change the history description if not clear (I was talking from v3 point of view).
The purpose of the series is the main description itself. Please check the v1 below:

https://patchew.org/QEMU/20250722131925.2119169-1-smail.aider@huawei.com/

Other than that, the argument (is_pmcr) is correct. "isread" is not used in this case.

--
Best Regards,
Smail AIDER
E-Mail: smail.aider@huawei.com
Operating System Researcher/Developer
Dresden Research Center, OS Kernel Lab
Huawei Technologies Co., Ltd

-----Original Message-----
From: Philippe Mathieu-Daudé <philmd@linaro.org> 
Sent: Monday, August 11, 2025 2:36 PM
To: Smail AIDER <smail.aider@huawei.com>; qemu-devel@nongnu.org
Cc: Alexander Spyridakis <alexander.spyridakis@huawei.com>; zhangyue (BA) <zhangyue165@huawei.com>; Liuyutao(DRC) <liuyutao2@huawei.com>; mjt@tls.msk.ru; Peter Maydell <peter.maydell@linaro.org>; qemu-arm@nongnu.org; richard.henderson@linaro.org
Subject: Re: [PATCH v3 1/1] target/arm: Trap PMCR when MDCR_EL2.TPMCR is set

Hi Smail,

(no need to Cc qemu-stable with this patch, it is a simple refactor)

On 11/8/25 13:21, Smail AIDER via wrote:
> From: Smail AIDER via <qemu-devel@nongnu.org>
> 
> Trap PMCR_EL0 or PMCR accesses to EL2 when MDCR_EL2.TPMCR is set.
> Similar to MDCR_EL2.TPM, MDCR_EL2.TPMCR allows trapping EL0 and EL1
> accesses to the PMCR register to EL2.
> 
> Signed-off-by: Smail AIDER <smail.aider@huawei.com>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> Message-Id: <20250722131925.2119169-1-smail.aider@huawei.com>
> ---
>   target/arm/cpregs-pmu.c | 33 +++++++++++++++++++++++++--------
>   1 file changed, 25 insertions(+), 8 deletions(-)
> 
> diff --git a/target/arm/cpregs-pmu.c b/target/arm/cpregs-pmu.c
> index 9c4431c18b..13392ddc4c 100644
> --- a/target/arm/cpregs-pmu.c
> +++ b/target/arm/cpregs-pmu.c
> @@ -228,22 +228,27 @@ static bool event_supported(uint16_t number)
>       return supported_event_map[number] != UNSUPPORTED_EVENT;
>   }
>   
> -static CPAccessResult pmreg_access(CPUARMState *env, const ARMCPRegInfo *ri,
> -                                   bool isread)
> +static CPAccessResult do_pmreg_access(CPUARMState *env, bool is_pmcr)

"bool is_pmcr" vs ...

> +static CPAccessResult pmreg_access(CPUARMState *env, const ARMCPRegInfo *ri,
> +                                   bool isread)

... "bool isread".

I suppose we want to use "is_pmcr" here instead of "isread".

> +{
> +    return do_pmreg_access(env, false);
> +}
> +
> +static CPAccessResult pmreg_access_pmcr(CPUARMState *env, const ARMCPRegInfo *ri,
> +                                   bool isread)
> +{
> +    return do_pmreg_access(env, true);
> +}
> +
>   static CPAccessResult pmreg_access_xevcntr(CPUARMState *env,
>                                              const ARMCPRegInfo *ri,
>                                              bool isread)
> @@ -1187,14 +1204,14 @@ void define_pm_cpregs(ARMCPU *cpu)
>               .fgt = FGT_PMCR_EL0,
>               .type = ARM_CP_IO | ARM_CP_ALIAS,
>               .fieldoffset = offsetoflow32(CPUARMState, cp15.c9_pmcr),
> -            .accessfn = pmreg_access,
> +            .accessfn = pmreg_access_pmcr,
>               .readfn = pmcr_read, .raw_readfn = raw_read,
>               .writefn = pmcr_write, .raw_writefn = raw_write,
>           };
>           const ARMCPRegInfo pmcr64 = {
>               .name = "PMCR_EL0", .state = ARM_CP_STATE_AA64,
>               .opc0 = 3, .opc1 = 3, .crn = 9, .crm = 12, .opc2 = 0,
> -            .access = PL0_RW, .accessfn = pmreg_access,
> +            .access = PL0_RW, .accessfn = pmreg_access_pmcr,
>               .fgt = FGT_PMCR_EL0,
>               .type = ARM_CP_IO,
>               .fieldoffset = offsetof(CPUARMState, cp15.c9_pmcr),