Re: [PATCH] target/arm: mark the 32bit alias of PAR when LPAE enabled

[Peter Maydell]

On Tue, 7 Nov 2023 at 10:51, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> We also mark it ARM_CP_NO_GDB so we avoid duplicate PAR's in the
> system register XML we send to gdb.
>
> Suggested-by: <richard.henderson@linaro.org>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>
> ---
> v2
>   - only set ARM_CP_NO_GDB when no LPAE enabled
>   - also mark as AP_CP_ALIAS
> ---
>  target/arm/helper.c | 32 ++++++++++++++++++--------------
>  1 file changed, 18 insertions(+), 14 deletions(-)
>
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 5dc0d20a84..80a35b29bb 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -3722,20 +3722,6 @@ static void ats_write64(CPUARMState *env, const 
> ARMCPRegInfo *ri,
>  }
>  #endif
>
> -static const ARMCPRegInfo vapa_cp_reginfo[] = {
> -    { .name = "PAR", .cp = 15, .crn = 7, .crm = 4, .opc1 = 0, .opc2 = 0,
> -      .access = PL1_RW, .resetvalue = 0,
> -      .bank_fieldoffsets = { offsetoflow32(CPUARMState, cp15.par_s),
> -                             offsetoflow32(CPUARMState, cp15.par_ns) },
> -      .writefn = par_write },
> -#ifndef CONFIG_USER_ONLY
> -    /* This underdecoding is safe because the reginfo is NO_RAW. */
> -    { .name = "ATS", .cp = 15, .crn = 7, .crm = 8, .opc1 = 0, .opc2 = CP_ANY,
> -      .access = PL1_W, .accessfn = ats_access,
> -      .writefn = ats_write, .type = ARM_CP_NO_RAW | ARM_CP_RAISES_EXC },
> -#endif
> -};
> -
>  /* Return basic MPU access permission bits.  */
>  static uint32_t simple_mpu_ap_bits(uint32_t val)
>  {
> @@ -8904,6 +8890,24 @@ void register_cp_regs_for_features(ARMCPU *cpu)
>          define_arm_cp_regs(cpu, generic_timer_cp_reginfo);
>      }
>      if (arm_feature(env, ARM_FEATURE_VAPA)) {
> +        ARMCPRegInfo vapa_cp_reginfo[] = {
> +            { .name = "PAR", .cp = 15, .crn = 7, .crm = 4, .opc1 = 0, .opc2 
> = 0,
> +              .access = PL1_RW, .resetvalue = 0,
> +              .bank_fieldoffsets = { offsetoflow32(CPUARMState, cp15.par_s),
> +                                     offsetoflow32(CPUARMState, cp15.par_ns) 
> },
> +              .writefn = par_write},
> +#ifndef CONFIG_USER_ONLY
> +            /* This underdecoding is safe because the reginfo is NO_RAW. */
> +            { .name = "ATS", .cp = 15, .crn = 7, .crm = 8, .opc1 = 0, .opc2 
> = CP_ANY,
> +              .access = PL1_W, .accessfn = ats_access,
> +              .writefn = ats_write, .type = ARM_CP_NO_RAW | 
> ARM_CP_RAISES_EXC },
> +#endif
> +        };
> +
> +        /* When LPAE exists the PAR register is an alias */

We could be a little more explicit:
 /*
  * When LPAE exists this 32-bit PAR register is an alias of the
  * 64-bit AArch32 PAR register defined in lpae_cp_reginfo[]
  */

> +        if (arm_feature(env, ARM_FEATURE_LPAE)) {
> +            vapa_cp_reginfo[0].type = ARM_CP_ALIAS | ARM_CP_NO_GDB;
> +        }
>          define_arm_cp_regs(cpu, vapa_cp_reginfo);
>      }
>      if (arm_feature(env, ARM_FEATURE_CACHE_TEST_CLEAN)) {

Otherwise
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM