Re: [PATCH v2 06/10] arm/arm-powerctl: rebuild hflags after setting CP15 bits in arm_set_cpu_on()

[Richard Henderson <richard.henderson@linaro.org>]

On 12/17/19 6:12 AM, Peter Maydell wrote:
> Cc'ing Richard : this is one for you I think... (surely we
> need to rebuild the hflags from scratch when we power up
> a CPU anyway?)

We do compute hflags from scratch in reset.

It has also turned out that there were a few board models that poked at the
contents of the cpu and needed special help.  Some of that I would imagine
would be fixed properly with the multi-phase reset patches, where we could
rebuild hflags when *leaving* reset.

In arm_set_cpu_on_async_work, we start by resetting the cpu and then start
poking at the contents of some system registers.  So, yes, we do need to
rebuild after doing that.  Also, I'm not sure how this function should fit into
the multi-phase reset future.

So:

>> On Tue, Dec 17, 2019 at 12:36 AM Niek Linnenbank <nieklinnenbank@gmail.com> wrote:
>>>
>>> After setting CP15 bits in arm_set_cpu_on() the cached hflags must
>>> be rebuild to reflect the changed processor state. Without rebuilding,
>>> the cached hflags would be inconsistent until the next call to
>>> arm_rebuild_hflags(). When QEMU is compiled with debugging enabled
>>> (--enable-debug), this problem is captured shortly after the first
>>> call to arm_set_cpu_on() for CPUs running in ARM 32-bit non-secure mode:
>>>
>>>   qemu-system-arm: target/arm/helper.c:11359: cpu_get_tb_cpu_state:
>>>   Assertion `flags == rebuild_hflags_internal(env)' failed.
>>>   Aborted (core dumped)
>>>
>>> Fixes: 0c7f8c43daf65
>>> Signed-off-by: Niek Linnenbank <nieklinnenbank@gmail.com>
>>> ---
>>>  target/arm/arm-powerctl.c | 3 +++
>>>  1 file changed, 3 insertions(+)
>>>
>>> diff --git a/target/arm/arm-powerctl.c b/target/arm/arm-powerctl.c
>>> index b064513d44..b75f813b40 100644
>>> --- a/target/arm/arm-powerctl.c
>>> +++ b/target/arm/arm-powerctl.c
>>> @@ -127,6 +127,9 @@ static void arm_set_cpu_on_async_work(CPUState *target_cpu_state,
>>>          target_cpu->env.regs[0] = info->context_id;
>>>      }
>>>
>>> +    /* CP15 update requires rebuilding hflags */
>>> +    arm_rebuild_hflags(&target_cpu->env);
>>> +
>>>      /* Start the new CPU at the requested address */
>>>      cpu_set_pc(target_cpu_state, info->entry);
>>>

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~