Re: [Qemu-arm] [PATCH] Correct CPACR reset value for v7 cores

[Cédric Le Goater]

On 05/22/2018 07:37 PM, Peter Maydell wrote:
> In commit f0aff255700 we made cpacr_write() enforce that some CPACR
> bits are RAZ/WI and some are RAO/WI for ARMv7 cores. Unfortunately
> we forgot to also update the register's reset value. The effect
> was that (a) a guest that read CPACR on reset would not see ones in
> the RAO bits, and (b) if you did a migration before the guest did
> a write to the CPACR then the migration would fail because the
> destination would enforce the RAO bits and then complain that they
> didn't match the zero value from the source.
> 
> Implement reset for the CPACR using a custom reset function
> that just calls cpacr_write(), to avoid having to duplicate
> the logic for which bits are RAO.
> 
> This bug would affect migration for TCG CPUs which are ARMv7
> with VFP but without one of Neon or VFPv3.
> 
> Reported-by: Cédric Le Goater <address@hidden>
> Signed-off-by: Peter Maydell <address@hidden>

Tested-by: Cédric Le Goater <address@hidden>

> ---
> This is sufficient that a save-and-reload while the romulus-bmc
> machine is in the bootloader will work. On the other hand if
> I do a save-and-reload after the kernel has started booting
> then we get the classic "guest hang after reload", so some
> state is still not being transferred somewhere (probably in
> a device in the machine model?)

I see the problem. Bizarre.

Thanks,

C.  

> ---
>  target/arm/helper.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index c0f739972e..6023bf6046 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -863,6 +863,14 @@ static void cpacr_write(CPUARMState *env, const 
> ARMCPRegInfo *ri,
>      env->cp15.cpacr_el1 = value;
>  }
>  
> +static void cpacr_reset(CPUARMState *env, const ARMCPRegInfo *ri)
> +{
> +    /* Call cpacr_write() so that we reset with the correct RAO bits set
> +     * for our CPU features.
> +     */
> +    cpacr_write(env, ri, 0);
> +}
> +
>  static CPAccessResult cpacr_access(CPUARMState *env, const ARMCPRegInfo *ri,
>                                     bool isread)
>  {
> @@ -920,7 +928,7 @@ static const ARMCPRegInfo v6_cp_reginfo[] = {
>      { .name = "CPACR", .state = ARM_CP_STATE_BOTH, .opc0 = 3,
>        .crn = 1, .crm = 0, .opc1 = 0, .opc2 = 2, .accessfn = cpacr_access,
>        .access = PL1_RW, .fieldoffset = offsetof(CPUARMState, cp15.cpacr_el1),
> -      .resetvalue = 0, .writefn = cpacr_write },
> +      .resetfn = cpacr_reset, .writefn = cpacr_write },
>      REGINFO_SENTINEL
>  };
>  
>