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

[Alexander Graf <agraf@csgraf.de>]

On 27.11.20 05:41, Paolo Bonzini wrote:
> 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?


Mostly because there is a lot of super fragile logic all over resets 
atm. Init setts dirty, post-init clears it. Then the arch reset handlers 
assume that state is not dirty and fiddle with KVM reset ioctls and KVM 
register modification ioctls directly. Mostly because KVM for the most 
part implements its own reset logic.

I'm fairy sure I'd break someone unintentionally if I just throw this 
into the common reset handler.

However, if we're happy to fix the fallout when it arises, I'm happy to 
do so.


Alex