| Message ID | 156925341736.974393.18379970954169086891.stgit@bahia.lan |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | KVM: PPC: Book3S: HV: XIVE: Allocate less VPs in OPAL | expand |
On Mon, Sep 23, 2019 at 05:43:37PM +0200, Greg Kurz wrote: > From: Cédric Le Goater <clg@kaod.org> > > Do not assign the device private pointer before making sure the XIVE > VPs are allocated in OPAL and test pointer validity when releasing > the device. > > Fixes: 5422e95103cf ("KVM: PPC: Book3S HV: XIVE: Replace the 'destroy' method by a 'release' method") > Signed-off-by: Cédric Le Goater <clg@kaod.org> > Signed-off-by: Greg Kurz <groug@kaod.org> What happens in the case where the OPAL allocation fails? Does the host crash, or hang, or leak resources? I presume that users can trigger the allocation failure just by starting a suitably large number of guests - is that right? Is there an easier way? I'm trying to work out whether this is urgently needed in 5.4 and the stable trees or not. Paul.
On Tue, 24 Sep 2019 15:28:55 +1000 Paul Mackerras <paulus@ozlabs.org> wrote: > On Mon, Sep 23, 2019 at 05:43:37PM +0200, Greg Kurz wrote: > > From: Cédric Le Goater <clg@kaod.org> > > > > Do not assign the device private pointer before making sure the XIVE > > VPs are allocated in OPAL and test pointer validity when releasing > > the device. > > > > Fixes: 5422e95103cf ("KVM: PPC: Book3S HV: XIVE: Replace the 'destroy' method by a 'release' method") > > Signed-off-by: Cédric Le Goater <clg@kaod.org> > > Signed-off-by: Greg Kurz <groug@kaod.org> > > What happens in the case where the OPAL allocation fails? Does the > host crash, or hang, or leak resources? I presume that users can > trigger the allocation failure just by starting a suitably large > number of guests - is that right? Is there an easier way? I'm trying > to work out whether this is urgently needed in 5.4 and the stable > trees or not. > Wait... I don't quite remember how this patch landed in my tree but when I look at it again I have the impression it tries to fix something that cannot happen. It is indeed easy to trigger the allocation failure, eg. start more than 127 guests on a Witherspoon system. But if this happens, the create function returns an error and the device isn't created. I don't see how the release function could hence get called with a "partially initialized" device. Please ignore this patch. Unfortunately the rest of the series doesn't apply cleanly without it... I'll rebase and post a v2. Sorry for the noise :-\ > Paul.
diff --git a/arch/powerpc/kvm/book3s_xive.c b/arch/powerpc/kvm/book3s_xive.c index 591bfb4bfd0f..cd2006bfcd3e 100644 --- a/arch/powerpc/kvm/book3s_xive.c +++ b/arch/powerpc/kvm/book3s_xive.c @@ -1897,12 +1897,21 @@ void kvmppc_xive_free_sources(struct kvmppc_xive_src_block *sb) static void kvmppc_xive_release(struct kvm_device *dev) { struct kvmppc_xive *xive = dev->private; - struct kvm *kvm = xive->kvm; + struct kvm *kvm; struct kvm_vcpu *vcpu; int i; pr_devel("Releasing xive device\n"); + /* + * In case of failure (OPAL) at device creation, the device + * can be partially initialized. + */ + if (!xive) + return; + + kvm = xive->kvm; + /* * Since this is the device release function, we know that * userspace does not have any open fd referring to the @@ -2001,7 +2010,6 @@ static int kvmppc_xive_create(struct kvm_device *dev, u32 type) if (!xive) return -ENOMEM; - dev->private = xive; xive->dev = dev; xive->kvm = kvm; mutex_init(&xive->lock); @@ -2031,6 +2039,7 @@ static int kvmppc_xive_create(struct kvm_device *dev, u32 type) if (ret) return ret; + dev->private = xive; return 0; } diff --git a/arch/powerpc/kvm/book3s_xive_native.c b/arch/powerpc/kvm/book3s_xive_native.c index 248c1ea9e788..e9cbb42de424 100644 --- a/arch/powerpc/kvm/book3s_xive_native.c +++ b/arch/powerpc/kvm/book3s_xive_native.c @@ -987,12 +987,21 @@ static int kvmppc_xive_native_has_attr(struct kvm_device *dev, static void kvmppc_xive_native_release(struct kvm_device *dev) { struct kvmppc_xive *xive = dev->private; - struct kvm *kvm = xive->kvm; + struct kvm *kvm; struct kvm_vcpu *vcpu; int i; pr_devel("Releasing xive native device\n"); + /* + * In case of failure (OPAL) at device creation, the device + * can be partially initialized. + */ + if (!xive) + return; + + kvm = xive->kvm; + /* * Clear the KVM device file address_space which is used to * unmap the ESB pages when a device is passed-through. @@ -1076,7 +1085,6 @@ static int kvmppc_xive_native_create(struct kvm_device *dev, u32 type) if (!xive) return -ENOMEM; - dev->private = xive; xive->dev = dev; xive->kvm = kvm; kvm->arch.xive = xive; @@ -1100,6 +1108,7 @@ static int kvmppc_xive_native_create(struct kvm_device *dev, u32 type) if (ret) return ret; + dev->private = xive; return 0; }