Hi Marc,
On Wed, Apr 19, 2023 at 08:12:45AM +0100, Marc Zyngier wrote:
> 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
It appears that kvm_psci_vcpu_on() expects the vCPU's mp_state
to not be changed by holding the lock. Although I don't think this
code practically causes any real issues now, I am a little concerned
about leaving one instance that updates mpstate without acquiring the
lock, in terms of future maintenance, as holding the lock won't prevent
mp_state from being updated.
What do you think ?
> 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?
Yes, it looks better!
Thank you,
Reiji
