Re: [PATCH 4/8] arm: Synchronize CPU on PSCI on

[Paolo Bonzini <pbonzini@redhat.com>]

On 27/11/20 00:32, Alexander Graf wrote:
> 
> On 26.11.20 23:26, Peter Maydell wrote:
>> On Thu, 26 Nov 2020 at 22:16, Alexander Graf <agraf@csgraf.de> wrote:
>>> cpu_synchronize_state() sets the CPU registers into "dirty" state which
>>> means that env now holds the current copy. On the next entry, we then
>>> sync them back into HVF.
>>>
>>> Without the cpu_synchronize_state() call, HVF never knows that the CPU
>>> state is actually dirty. I guess it could as well live in cpu_reset()
>>> somewhere, but we have to get the state switched over to dirty one way
>>> or another.
>>>
>>> One interesting thing to note here is that the CPU actually comes up in
>>> "dirty" after init. But init is done on realization already. I'm not
>>> sure why we lose the dirty state in between that and the reset.
>> Yeah, it sounds like you need to figure out where the dirty
>> to not-dirty transitions ought to be happening rather than
>> just fudging things here...
> 
> 
> When init is complete (system is ready to launch), the CPU state is 
> pushed to HVF and dirty is set to false. So by design, a normal 
> cpu_reset doesn't have vcpu_dirty set.
> 
> How about this patch instead?
> 
> Alex
> 
> 
> 
> commit 8c61bc4d613b01e251b6b2f892d1a55a333c6e37
> Author: Alexander Graf <agraf@csgraf.de>
> Date:   Thu Nov 26 02:47:09 2020 +0100
> 
>      hvf: arm: Mark CPU as dirty on reset
> 
>      When clearing internal state of a CPU, we should also make sure 
> that HVF
>      knows about it and can push the new values down to vcpu state.
> 
>      Make sure that with HVF enabled, we tell it that it should synchronize
>      CPU state on next entry after a reset.
> 
>      This fixes PSCI handling, because now newly pushed state such as X0 
> and
>      PC on remote CPU enablement also get pushed into HVF.
> 
>      Signed-off-by: Alexander Graf <agraf@csgraf.de>
> 
> diff --git a/target/arm/arm-powerctl.c b/target/arm/arm-powerctl.c
> index b75f813b40..a49a5b32e6 100644
> --- a/target/arm/arm-powerctl.c
> +++ b/target/arm/arm-powerctl.c
> @@ -15,6 +15,7 @@
>   #include "arm-powerctl.h"
>   #include "qemu/log.h"
>   #include "qemu/main-loop.h"
> +#include "sysemu/hw_accel.h"
> 
>   #ifndef DEBUG_ARM_POWERCTL
>   #define DEBUG_ARM_POWERCTL 0
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index db6f7c34ed..9a501ea4bd 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -411,6 +411,8 @@ static void arm_cpu_reset(DeviceState *dev)
>   #ifndef CONFIG_USER_ONLY
>       if (kvm_enabled()) {
>           kvm_arm_reset_vcpu(cpu);
> +    } else if (hvf_enabled()) {
> +        s->vcpu_dirty = true;
>       }
>   #endif
> 

Why only for HVF and only for ARM?  For example hax_init_vcpu and 
whpx_init_vcpu both set s->vcpu_dirty; should you just set it 
unconditionally in cpu_common_reset?

Thanks,

Paolo