On Wed, 19 Apr 2023 03:18:51 +0100,
Reiji Watanabe <reijiw@xxxxxxxxxx> wrote:
>
> kvm_arch_vcpu_ioctl_vcpu_init() doesn't acquire mp_state_lock
> when setting the mp_state to KVM_MP_STATE_RUNNABLE. Fix the
> code to acquire the lock.
>
> Signed-off-by: Reiji Watanabe <reijiw@xxxxxxxxxx>
> ---
> arch/arm64/kvm/arm.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> index fbafcbbcc463..388aa4f18f21 100644
> --- a/arch/arm64/kvm/arm.c
> +++ b/arch/arm64/kvm/arm.c
> @@ -1244,8 +1244,11 @@ static int kvm_arch_vcpu_ioctl_vcpu_init(struct kvm_vcpu *vcpu,
> */
> if (test_bit(KVM_ARM_VCPU_POWER_OFF, vcpu->arch.features))
> kvm_arm_vcpu_power_off(vcpu);
> - else
> + else {
> + spin_lock(&vcpu->arch.mp_state_lock);
> WRITE_ONCE(vcpu->arch.mp_state.mp_state, KVM_MP_STATE_RUNNABLE);
> + spin_unlock(&vcpu->arch.mp_state_lock);
> + }
>
> return 0;
> }
I'm not entirely convinced that this fixes anything. What does the
lock hazard against given that the write is atomic? But maybe a
slightly more readable of this would be to expand the critical section
this way:
diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
index 4ec888fdd4f7..bb21d0c25de7 100644
--- a/arch/arm64/kvm/arm.c
+++ b/arch/arm64/kvm/arm.c
@@ -1246,11 +1246,15 @@ static int kvm_arch_vcpu_ioctl_vcpu_init(struct kvm_vcpu *vcpu,
/*
* Handle the "start in power-off" case.
*/
+ spin_lock(&vcpu->arch.mp_state_lock);
+
if (test_bit(KVM_ARM_VCPU_POWER_OFF, vcpu->arch.features))
- kvm_arm_vcpu_power_off(vcpu);
+ __kvm_arm_vcpu_power_off(vcpu);
else
WRITE_ONCE(vcpu->arch.mp_state.mp_state, KVM_MP_STATE_RUNNABLE);
+ spin_unlock(&vcpu->arch.mp_state_lock);
+
return 0;
}
Thoughts?
M.
--
Without deviation from the norm, progress is not possible.
