Re: [PATCH v1 19/28] target/arm: don't bother with id_aa64pfr0_read for USER_ONLY

[Philippe Mathieu-Daudé]

On 3/16/20 6:21 PM, Alex Bennée wrote:
> For system emulation we need to check the state of the GIC before we
> report the value. However this isn't relevant to exporting of the
> value to linux-user and indeed breaks the exported value as set by
> modify_arm_cp_regs.
>
> Signed-off-by: Alex Bennée <address@hidden>
> Reviewed-by: Richard Henderson <address@hidden>
>
> ---
> v2
>    - extend the ifdef and make type CONST with no accessfn
> ---
>   target/arm/helper.c | 20 +++++++++++++++-----
>   1 file changed, 15 insertions(+), 5 deletions(-)
>
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 7e560ea7db6..d2ec2c53510 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -6697,6 +6697,7 @@ static uint64_t id_pfr1_read(CPUARMState *env, const 
> ARMCPRegInfo *ri)
>       return pfr1;
>   }
>   
> +#ifndef CONFIG_USER_ONLY
>   static uint64_t id_aa64pfr0_read(CPUARMState *env, const ARMCPRegInfo *ri)
>   {
>       ARMCPU *cpu = env_archcpu(env);
> @@ -6707,6 +6708,7 @@ static uint64_t id_aa64pfr0_read(CPUARMState *env, const 
> ARMCPRegInfo *ri)
>       }
>       return pfr0;
>   }
> +#endif
>   
>   /* Shared logic between LORID and the rest of the LOR* registers.
>    * Secure state has already been delt with.
> @@ -7280,16 +7282,24 @@ void register_cp_regs_for_features(ARMCPU *cpu)
>            * define new registers here.
>            */
>           ARMCPRegInfo v8_idregs[] = {
> -            /* ID_AA64PFR0_EL1 is not a plain ARM_CP_CONST because we don't
> -             * know the right value for the GIC field until after we
> -             * define these regs.
> +            /*
> +             * ID_AA64PFR0_EL1 is not a plain ARM_CP_CONST in system
> +             * emulation because we don't know the right value for the
> +             * GIC field until after we define these regs.
>                */
>               { .name = "ID_AA64PFR0_EL1", .state = ARM_CP_STATE_AA64,
>                 .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 4, .opc2 = 0,
> -              .access = PL1_R, .type = ARM_CP_NO_RAW,
> +              .access = PL1_R,
> +#ifdef CONFIG_USER_ONLY
> +              .type = ARM_CP_CONST,
> +              .resetvalue = cpu->isar.id_aa64pfr0
> +#else
> +              .type = ARM_CP_NO_RAW,
>                 .accessfn = access_aa64_tid3,
>                 .readfn = id_aa64pfr0_read,
> -              .writefn = arm_cp_write_ignore },
> +              .writefn = arm_cp_write_ignore

I guess this is the implementation of what Peter described here:
https://www.mail-archive.com/address@hidden/msg689388.html

Reviewed-by: Philippe Mathieu-Daudé <address@hidden>

> +#endif
> +            },
>               { .name = "ID_AA64PFR1_EL1", .state = ARM_CP_STATE_AA64,
>                 .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 4, .opc2 = 1,
>                 .access = PL1_R, .type = ARM_CP_CONST,