Re: [Qemu-devel] [PATCH 05/13] target/arm: Allow AArch32 access for PMCCFILTR

[Peter Maydell]

On 30 September 2017 at 03:08, Aaron Lindsay <address@hidden> wrote:
> Also fix the existing bitmask for writes.

"Also" in a commit message can often be translated as
"This should really go in a separate commit, but" :-)

> Signed-off-by: Aaron Lindsay <address@hidden>
> ---
>  target/arm/helper.c | 23 ++++++++++++++++++++++-
>  1 file changed, 22 insertions(+), 1 deletion(-)
>
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 54070a3..fcc2fcf 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -1059,10 +1059,25 @@ static void pmccfiltr_write(CPUARMState *env, const 
> ARMCPRegInfo *ri,
>                              uint64_t value)
>  {
>      pmccntr_sync(env);
> -    env->cp15.pmccfiltr_el0 = value & 0x7E000000;
> +    env->cp15.pmccfiltr_el0 = value & 0xfc000000;
>      pmccntr_sync(env);
>  }
>
> +static void pmccfiltr_write_a32(CPUARMState *env, const ARMCPRegInfo *ri,
> +                            uint64_t value)
> +{
> +    pmccntr_sync(env);
> +    env->cp15.pmccfiltr_el0 = (env->cp15.pmccfiltr_el0 & 0x04000000) |
> +        (value & 0xf8000000); /* M is not visible in AArch32 */

I think the comment would look better on its own line.

> +    pmccntr_sync(env);
> +}

I think we could probably benefit from defining constants for
the bit names in this register.

> +
> +static uint64_t pmccfiltr_read_a32(CPUARMState *env, const ARMCPRegInfo *ri)
> +{
> +    /* M is not visible in AArch32 */
> +    return env->cp15.pmccfiltr_el0 & 0xf8000000;
> +}
> +
>  static void pmcntenset_write(CPUARMState *env, const ARMCPRegInfo *ri,
>                              uint64_t value)
>  {
> @@ -1280,6 +1295,12 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
>        .type = ARM_CP_IO,
>        .readfn = pmccntr_read, .writefn = pmccntr_write, },
>  #endif
> +    { .name = "PMCCFILTR", .cp = 15, .opc1 = 0, .crn = 14, .crm = 15, .opc2 
> = 7,
> +      .writefn = pmccfiltr_write_a32, .readfn = pmccfiltr_read_a32,
> +      .access = PL0_RW, .accessfn = pmreg_access,
> +      .type = ARM_CP_ALIAS,
> +      .fieldoffset = offsetoflow32(CPUARMState, cp15.pmccfiltr_el0),
> +      .resetvalue = 0, },
>      { .name = "PMCCFILTR_EL0", .state = ARM_CP_STATE_AA64,
>        .opc0 = 3, .opc1 = 3, .crn = 14, .crm = 15, .opc2 = 7,
>        .writefn = pmccfiltr_write,

If you're defining both a .readfn and a .writefn for a register
you don't need to provide a .fieldoffset.

thanks
-- PMM