On Tue, 28 May 2019 14:17:11 +1000
Paul Mackerras <paulus@xxxxxxxxxx> wrote:
> 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?
>
Hi Paul,
No I'm not. As you point out below, it is just a matter of rationalizing the
arguments of xive_check_provisioning().
> > 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.
>
I certainly don't have such reason :)
Reviewed-by: Greg Kurz <groug@xxxxxxxx>
> Paul.
