Re: [PATCH v4 2/5] arm64: KVM: use mutex_trylock_nest_lock when locking all vCPUs

[Marc Zyngier <maz@kernel.org>]

On Thu, 01 May 2025 14:41:26 +0100,
Peter Zijlstra <peterz@infradead.org> wrote:
> 
> On Thu, May 01, 2025 at 01:44:28PM +0100, Marc Zyngier wrote:
> > On Thu, 01 May 2025 12:15:52 +0100,
> > Peter Zijlstra <peterz@infradead.org> wrote:
> > > 
> > > > > + */
> > > > > +int kvm_trylock_all_vcpus(struct kvm *kvm)
> > > > > +{
> > > > > +	struct kvm_vcpu *vcpu;
> > > > > +	unsigned long i, j;
> > > > > +
> > > > > +	kvm_for_each_vcpu(i, vcpu, kvm)
> > > > > +		if (!mutex_trylock_nest_lock(&vcpu->mutex, &kvm->lock))
> > > 
> > > This one includes an assertion that kvm->lock is actually held.
> > 
> > Ah, cunning. Thanks.
> > 
> > > That said, I'm not at all sure what the purpose of all this trylock
> > > stuff is here.
> > > 
> > > Can someone explain? Last time I asked someone said something about
> > > multiple VMs, but I don't know enough about kvm to know what that means.
> > 
> > Multiple VMs? That'd be real fun. Not.
> > 
> > > Are those vcpu->mutex another class for other VMs? Or what gives?
> > 
> > Nah. This is firmly single VM.
> > 
> > The purpose of this contraption is that there are some rare cases
> > where we need to make sure that if we update some global state, all
> > the vcpus of a VM need to see, or none of them.
> > 
> > For these cases, the guarantee comes from luserspace, and it gives the
> > pinky promise that none of the vcpus are running at that point. But
> > being of a suspicious nature, we assert that this is true by trying to
> > take all the vcpu mutexes in one go. This will fail if a vcpu is
> > running, as KVM itself takes the vcpu mutex before doing anything.
> > 
> > Similar requirement exists if we need to synthesise some state for
> > userspace from all the individual vcpu states.
> 
> Ah, okay. Because x86 is simply doing mutex_lock() instead of
> mutex_trylock() -- which would end up waiting for this activity to
> subside I suppose.
> 
> Hence the use of the killable variant I suppose, for when they get tired
> of waiting.

Yeah, I remember some debate around that when this refactoring was
first posted. I quickly paged it out.

> If all the architectures are basically doing the same thing, it might
> make sense to unify this particular behaviour. But what do I know.

I don't know either. The trylock behaviour has been there since day-1
on the arm side, and changing it would have userspace visible effects.
So I'm pretty keen on preserving it, warts and all. The last thing I
need is a VMM person hitting my inbox on the grounds that their toy is
broken.

On the other hand, we're talking about virtualisation, so everything
is more or less broken by design...

	M.

-- 
Without deviation from the norm, progress is not possible.