| Message ID | 20240613233639.202896-26-salil.mehta@huawei.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | None | expand |
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
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)