Re: [PATCH] KVM: PPC: Book3S HV: XIVE: introduce a KVM device lock

[Paul Mackerras <paulus@ozlabs.org>]

Greg,

On Fri, May 24, 2019 at 08:16:21PM +0200, Greg Kurz wrote:
> On Fri, 24 May 2019 15:20:30 +0200
> Cédric Le Goater <clg@kaod.org> wrote:
> 
> > The XICS-on-XIVE KVM device needs to allocate XIVE event queues when a
> > priority is used by the OS. This is referred as EQ provisioning and it
> > is done under the hood when :
> > 
> >   1. a CPU is hot-plugged in the VM
> >   2. the "set-xive" is called at VM startup
> >   3. sources are restored at VM restore
> > 
> > The kvm->lock mutex is used to protect the different XIVE structures
> > being modified but in some contextes, kvm->lock is taken under the
> > vcpu->mutex which is a forbidden sequence by KVM.
> > 
> > Introduce a new mutex 'lock' for the KVM devices for them to
> > synchronize accesses to the XIVE device structures.
> > 
> > Signed-off-by: Cédric Le Goater <clg@kaod.org>
> > ---
> >  arch/powerpc/kvm/book3s_xive.h        |  1 +
> >  arch/powerpc/kvm/book3s_xive.c        | 23 +++++++++++++----------
> >  arch/powerpc/kvm/book3s_xive_native.c | 15 ++++++++-------
> >  3 files changed, 22 insertions(+), 17 deletions(-)
> > 
> > diff --git a/arch/powerpc/kvm/book3s_xive.h b/arch/powerpc/kvm/book3s_xive.h
> > index 426146332984..862c2c9650ae 100644
> > --- a/arch/powerpc/kvm/book3s_xive.h
> > +++ b/arch/powerpc/kvm/book3s_xive.h
> > @@ -141,6 +141,7 @@ struct kvmppc_xive {
> >  	struct kvmppc_xive_ops *ops;
> >  	struct address_space   *mapping;
> >  	struct mutex mapping_lock;
> > +	struct mutex lock;
> >  };
> >  
> >  #define KVMPPC_XIVE_Q_COUNT	8
> > diff --git a/arch/powerpc/kvm/book3s_xive.c b/arch/powerpc/kvm/book3s_xive.c
> > index f623451ec0a3..12c8a36dd980 100644
> > --- a/arch/powerpc/kvm/book3s_xive.c
> > +++ b/arch/powerpc/kvm/book3s_xive.c
> > @@ -271,14 +271,14 @@ static int xive_provision_queue(struct kvm_vcpu *vcpu, u8 prio)
> >  	return rc;
> >  }
> >  
> > -/* Called with kvm_lock held */
> > +/* Called with xive->lock held */
> >  static int xive_check_provisioning(struct kvm *kvm, u8 prio)
> >  {
> >  	struct kvmppc_xive *xive = kvm->arch.xive;
> 
> Since the kvm_lock isn't protecting kvm->arch anymore, this looks weird.

Are you suggesting that something that was protected before now isn't
with Cédric's patch?

> Passing xive instead of kvm and using xive->kvm would make more sense IMHO.
> 
> Maybe fold the following into your patch ?

As far as I can see your delta patch doesn't actually change any
locking but just rationalizes the parameters for an internal
function.  That being so, for 5.2 I am intending to put Cédric's
original patch in, unless someone comes up with a good reason not to.

Paul.