[PATCH v1 1/2] KVM: arm64: Acquire mp_state_lock in kvm_arch_vcpu_ioctl_vcpu_init()

[Reiji Watanabe]

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 at google.com> 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 at google.com>
> > ---
> >  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