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@xxxxxxxx> 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@xxxxxxxx>
> > ---
> > 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.
