From: Jean-Philippe Brucker <jean-philippe@linaro.org>

When a KVM vCPU is reset following a PSCI CPU_ON call, its power state
is not synchronized with KVM at the moment. Because the vCPU is not
marked dirty, we miss the call to kvm_arch_put_registers() that writes
to KVM's MP_STATE. Force mp_state synchronization.

Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
Signed-off-by: Salil Mehta <salil.mehta@huawei.com>
---
 target/arm/kvm.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/target/arm/kvm.c b/target/arm/kvm.c
index 1121771c4a..7acd83ce64 100644
--- a/target/arm/kvm.c
+++ b/target/arm/kvm.c
@@ -980,6 +980,7 @@ void kvm_arm_cpu_post_load(ARMCPU *cpu)
 void kvm_arm_reset_vcpu(ARMCPU *cpu)
 {
     int ret;
+    CPUState *cs = CPU(cpu);
 
     /* Re-init VCPU so that all registers are set to
      * their respective reset values.
@@ -1001,6 +1002,12 @@ void kvm_arm_reset_vcpu(ARMCPU *cpu)
      * for the same reason we do so in kvm_arch_get_registers().
      */
     write_list_to_cpustate(cpu);
+
+    /*
+     * Ensure we call kvm_arch_put_registers(). The vCPU isn't marked dirty if
+     * it was parked in KVM and is now booting from a PSCI CPU_ON call.
+     */
+    cs->vcpu_dirty = true;
 }
 
 void kvm_arm_create_host_vcpu(ARMCPU *cpu)
-- 
2.34.1

On Fri Jun 14, 2024 at 9:36 AM AEST, Salil Mehta wrote:
> From: Jean-Philippe Brucker <jean-philippe@linaro.org>
>
> When a KVM vCPU is reset following a PSCI CPU_ON call, its power state
> is not synchronized with KVM at the moment. Because the vCPU is not
> marked dirty, we miss the call to kvm_arch_put_registers() that writes
> to KVM's MP_STATE. Force mp_state synchronization.

Hmm. Is this a bug fix for upstream? arm does respond to CPU_ON calls
by the look, but maybe it's not doing KVM parking until your series?
Maybe just a slight change to say "When KVM parking is implemented for
ARM..." if so.

>
> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> Signed-off-by: Salil Mehta <salil.mehta@huawei.com>
> ---
>  target/arm/kvm.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/target/arm/kvm.c b/target/arm/kvm.c
> index 1121771c4a..7acd83ce64 100644
> --- a/target/arm/kvm.c
> +++ b/target/arm/kvm.c
> @@ -980,6 +980,7 @@ void kvm_arm_cpu_post_load(ARMCPU *cpu)
>  void kvm_arm_reset_vcpu(ARMCPU *cpu)
>  {
>      int ret;
> +    CPUState *cs = CPU(cpu);
>  
>      /* Re-init VCPU so that all registers are set to
>       * their respective reset values.
> @@ -1001,6 +1002,12 @@ void kvm_arm_reset_vcpu(ARMCPU *cpu)
>       * for the same reason we do so in kvm_arch_get_registers().
>       */
>      write_list_to_cpustate(cpu);
> +
> +    /*
> +     * Ensure we call kvm_arch_put_registers(). The vCPU isn't marked dirty if
> +     * it was parked in KVM and is now booting from a PSCI CPU_ON call.
> +     */
> +    cs->vcpu_dirty = true;
>  }
>  
>  void kvm_arm_create_host_vcpu(ARMCPU *cpu)

Also above my pay grade, but arm_set_cpu_on_async_work() which seems
to be what calls the CPU reset you refer to does a bunch of CPU register
and state setting including the power state setting that you mention.
Would the vcpu_dirty be better to go there?

Thanks,
Nick

Hi Nick,

>  From: Nicholas Piggin <npiggin@gmail.com>
>  Sent: Thursday, July 4, 2024 4:28 AM
>  To: Salil Mehta <salil.mehta@huawei.com>; qemu-devel@nongnu.org;
>  qemu-arm@nongnu.org; mst@redhat.com
>  
>  On Fri Jun 14, 2024 at 9:36 AM AEST, Salil Mehta wrote:
>  > From: Jean-Philippe Brucker <jean-philippe@linaro.org>
>  >
>  > When a KVM vCPU is reset following a PSCI CPU_ON call, its power state
>  > is not synchronized with KVM at the moment. Because the vCPU is not
>  > marked dirty, we miss the call to kvm_arch_put_registers() that writes
>  > to KVM's MP_STATE. Force mp_state synchronization.
>  
>  Hmm. Is this a bug fix for upstream? arm does respond to CPU_ON calls by
>  the look, but maybe it's not doing KVM parking until your series?


Yes, this is required we now park and un-park the vCPUs. We must ensure the
KVM resets the KVM VCPU state as well. Hence, not a fix but a change which
is required in context to this patch-set.


>  Maybe just a slight change to say "When KVM parking is implemented for
>  ARM..." if so.

Sure.

>  
>  >
>  > Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
>  > Signed-off-by: Salil Mehta <salil.mehta@huawei.com>
>  > ---
>  >  target/arm/kvm.c | 7 +++++++
>  >  1 file changed, 7 insertions(+)
>  >
>  > diff --git a/target/arm/kvm.c b/target/arm/kvm.c index
>  > 1121771c4a..7acd83ce64 100644
>  > --- a/target/arm/kvm.c
>  > +++ b/target/arm/kvm.c
>  > @@ -980,6 +980,7 @@ void kvm_arm_cpu_post_load(ARMCPU *cpu)
>  void
>  > kvm_arm_reset_vcpu(ARMCPU *cpu)  {
>  >      int ret;
>  > +    CPUState *cs = CPU(cpu);
>  >
>  >      /* Re-init VCPU so that all registers are set to
>  >       * their respective reset values.
>  > @@ -1001,6 +1002,12 @@ void kvm_arm_reset_vcpu(ARMCPU *cpu)
>  >       * for the same reason we do so in kvm_arch_get_registers().
>  >       */
>  >      write_list_to_cpustate(cpu);
>  > +
>  > +    /*
>  > +     * Ensure we call kvm_arch_put_registers(). The vCPU isn't marked dirty if
>  > +     * it was parked in KVM and is now booting from a PSCI CPU_ON call.
>  > +     */
>  > +    cs->vcpu_dirty = true;
>  >  }
>  >
>  >  void kvm_arm_create_host_vcpu(ARMCPU *cpu)
>  
>  Also above my pay grade, but arm_set_cpu_on_async_work() which seems
>  to be what calls the CPU reset you refer to does a bunch of CPU register and
>  state setting including the power state setting that you mention.
>  Would the vcpu_dirty be better to go there?


Maybe we can. Let me cross verify this.


Thanks
Salil.

>  
>  Thanks,
>  Nick