From mboxrd@z Thu Jan 1 00:00:00 1970 From: David Gibson Subject: Re: [PATCH v2 03/16] KVM: PPC: Book3S HV: XIVE: introduce a new capability KVM_CAP_PPC_IRQ_XIVE Date: Wed, 13 Mar 2019 15:05:42 +1100 Message-ID: <20190313040542.GL9881@umbus.fritz.box> References: <20190222112840.25000-1-clg@kaod.org> <20190222112840.25000-4-clg@kaod.org> <20190225003527.GG7668@umbus.fritz.box> <2f029721-0b20-2706-6627-0bac36afd03e@kaod.org> Mime-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="c8JyeaiReRNoiMDS" Cc: kvm@vger.kernel.org, kvm-ppc@vger.kernel.org, Paul Mackerras , linuxppc-dev@lists.ozlabs.org To: =?iso-8859-1?Q?C=E9dric?= Le Goater Return-path: Content-Disposition: inline In-Reply-To: <2f029721-0b20-2706-6627-0bac36afd03e@kaod.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: linuxppc-dev-bounces+glppe-linuxppc-embedded-2=m.gmane.org@lists.ozlabs.org Sender: "Linuxppc-dev" List-Id: kvm.vger.kernel.org --c8JyeaiReRNoiMDS Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, Mar 12, 2019 at 03:03:25PM +0100, C=E9dric Le Goater wrote: > On 2/25/19 1:35 AM, David Gibson wrote: > > On Fri, Feb 22, 2019 at 12:28:27PM +0100, C=E9dric Le Goater wrote: [snip] > >> +int kvmppc_xive_native_connect_vcpu(struct kvm_device *dev, > >> + struct kvm_vcpu *vcpu, u32 cpu) > >> +{ > >> + struct kvmppc_xive *xive =3D dev->private; > >> + struct kvmppc_xive_vcpu *xc; > >> + int rc; > >> + > >> + pr_devel("native_connect_vcpu(cpu=3D%d)\n", cpu); > >> + > >> + if (dev->ops !=3D &kvm_xive_native_ops) { > >> + pr_devel("Wrong ops !\n"); > >> + return -EPERM; > >> + } > >> + if (xive->kvm !=3D vcpu->kvm) > >> + return -EPERM; > >> + if (vcpu->arch.irq_type !=3D KVMPPC_IRQ_DEFAULT) > >> + return -EBUSY; > >> + if (kvmppc_xive_find_server(vcpu->kvm, cpu)) { > >=20 > > You haven't taken the kvm->lock yet, so couldn't a race mean a > > duplicate server gets inserted after you make this check? > >=20 > >> + pr_devel("Duplicate !\n"); > >> + return -EEXIST; > >> + } > >> + if (cpu >=3D KVM_MAX_VCPUS) { > >> + pr_devel("Out of bounds !\n"); > >> + return -EINVAL; > >> + } > >> + xc =3D kzalloc(sizeof(*xc), GFP_KERNEL); > >> + if (!xc) > >> + return -ENOMEM; > >> + > >> + mutex_lock(&vcpu->kvm->lock); > >> + vcpu->arch.xive_vcpu =3D xc; > >=20 > > Similarly you don't verify this is NULL after taking the lock, so > > couldn't another thread race and make a connect which gets clobbered > > here? >=20 > Yes. this is not very safe ... We need to clean up all the KVM device=20 > methods doing the connection of the presenter to the vCPU AFAICT.=20 > I will fix the XIVE native one for now.=20 >=20 > And also, this CPU parameter is useless. There is no reason to connect=20 > a vCPU from another vCPU. Hmm.. I thought the point of the 'cpu' parameter (not a great name) is that it lets userspace chose the guest visible irq server ID. I think that's preferable to tying it to an existing cpu id, if possible. --=20 David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson --c8JyeaiReRNoiMDS Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEdfRlhq5hpmzETofcbDjKyiDZs5IFAlyIgZYACgkQbDjKyiDZ s5JZshAAlJTbgUEzm/PuGQSZ8a7JqBhImKIbcZgeM6ZMcPpWOMWluJ6PWM9qDRDl Wh9lGWxEneoPGbES7TWp5vVd43ro1ncHyE6WbyTOHFgSfjKjargr70pdW0vQTXz2 sPV/5D3RfiuTs2kz4aOLF7KuVJH3ktCnSLIYSHDdSRTo38e5NVL06giRFRUugQvm lwQng7UOmkQji1EZ9wTtbjQ3DVOKdrMoRZSdqHxICmYREVuF17oDhRPHwkh6lMku FoC5SvPRWejI4PjDT3sxWwvezRgGInrW9oLiH/Y9hmf96uS+i+VqqvNcvuaPF+7v 5FpafHUxEEyag4s6IvHl2jC6OGnVgbA6qtYm+o75whsEeohQNokRwbLzPuEgudi5 XkuOkHkxf2lfHe0vXm3ivsG9LGVSePBJ2eVQ38Dzg5ZWu/Hs5ZiMXJTafb3BoTBq WknL/p48rxUcUM+r/pPzrx4pVD4AmQ5qvHS5rrMz+C5FhWV68p64UZWgCovUlzEB 2dks3I6V5CQOKuQykxYP1kmMTQ/nReiInLHHsS4LzkGRmiiMQcVZGyANOT36L59j H9zOR/1rLV2KrvITrXloTpxz/YWWjmGBQBLLOVn/nn032aEYQiLtvYHAlJBSixAO KK40uB9XvfD6fBNXgsswax6cLidEBw/RWzNwj6WUVY0DCiiGhRE= =YaVr -----END PGP SIGNATURE----- --c8JyeaiReRNoiMDS-- From mboxrd@z Thu Jan 1 00:00:00 1970 From: David Gibson Date: Wed, 13 Mar 2019 04:05:42 +0000 Subject: Re: [PATCH v2 03/16] KVM: PPC: Book3S HV: XIVE: introduce a new capability KVM_CAP_PPC_IRQ_XIVE Message-Id: <20190313040542.GL9881@umbus.fritz.box> MIME-Version: 1 Content-Type: multipart/mixed; boundary="c8JyeaiReRNoiMDS" List-Id: References: <20190222112840.25000-1-clg@kaod.org> <20190222112840.25000-4-clg@kaod.org> <20190225003527.GG7668@umbus.fritz.box> <2f029721-0b20-2706-6627-0bac36afd03e@kaod.org> In-Reply-To: <2f029721-0b20-2706-6627-0bac36afd03e@kaod.org> To: =?iso-8859-1?Q?C=E9dric?= Le Goater Cc: kvm@vger.kernel.org, kvm-ppc@vger.kernel.org, Paul Mackerras , linuxppc-dev@lists.ozlabs.org --c8JyeaiReRNoiMDS Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, Mar 12, 2019 at 03:03:25PM +0100, C=E9dric Le Goater wrote: > On 2/25/19 1:35 AM, David Gibson wrote: > > On Fri, Feb 22, 2019 at 12:28:27PM +0100, C=E9dric Le Goater wrote: [snip] > >> +int kvmppc_xive_native_connect_vcpu(struct kvm_device *dev, > >> + struct kvm_vcpu *vcpu, u32 cpu) > >> +{ > >> + struct kvmppc_xive *xive =3D dev->private; > >> + struct kvmppc_xive_vcpu *xc; > >> + int rc; > >> + > >> + pr_devel("native_connect_vcpu(cpu=3D%d)\n", cpu); > >> + > >> + if (dev->ops !=3D &kvm_xive_native_ops) { > >> + pr_devel("Wrong ops !\n"); > >> + return -EPERM; > >> + } > >> + if (xive->kvm !=3D vcpu->kvm) > >> + return -EPERM; > >> + if (vcpu->arch.irq_type !=3D KVMPPC_IRQ_DEFAULT) > >> + return -EBUSY; > >> + if (kvmppc_xive_find_server(vcpu->kvm, cpu)) { > >=20 > > You haven't taken the kvm->lock yet, so couldn't a race mean a > > duplicate server gets inserted after you make this check? > >=20 > >> + pr_devel("Duplicate !\n"); > >> + return -EEXIST; > >> + } > >> + if (cpu >=3D KVM_MAX_VCPUS) { > >> + pr_devel("Out of bounds !\n"); > >> + return -EINVAL; > >> + } > >> + xc =3D kzalloc(sizeof(*xc), GFP_KERNEL); > >> + if (!xc) > >> + return -ENOMEM; > >> + > >> + mutex_lock(&vcpu->kvm->lock); > >> + vcpu->arch.xive_vcpu =3D xc; > >=20 > > Similarly you don't verify this is NULL after taking the lock, so > > couldn't another thread race and make a connect which gets clobbered > > here? >=20 > Yes. this is not very safe ... We need to clean up all the KVM device=20 > methods doing the connection of the presenter to the vCPU AFAICT.=20 > I will fix the XIVE native one for now.=20 >=20 > And also, this CPU parameter is useless. There is no reason to connect=20 > a vCPU from another vCPU. Hmm.. I thought the point of the 'cpu' parameter (not a great name) is that it lets userspace chose the guest visible irq server ID. I think that's preferable to tying it to an existing cpu id, if possible. --=20 David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson --c8JyeaiReRNoiMDS Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEdfRlhq5hpmzETofcbDjKyiDZs5IFAlyIgZYACgkQbDjKyiDZ s5JZshAAlJTbgUEzm/PuGQSZ8a7JqBhImKIbcZgeM6ZMcPpWOMWluJ6PWM9qDRDl Wh9lGWxEneoPGbES7TWp5vVd43ro1ncHyE6WbyTOHFgSfjKjargr70pdW0vQTXz2 sPV/5D3RfiuTs2kz4aOLF7KuVJH3ktCnSLIYSHDdSRTo38e5NVL06giRFRUugQvm lwQng7UOmkQji1EZ9wTtbjQ3DVOKdrMoRZSdqHxICmYREVuF17oDhRPHwkh6lMku FoC5SvPRWejI4PjDT3sxWwvezRgGInrW9oLiH/Y9hmf96uS+i+VqqvNcvuaPF+7v 5FpafHUxEEyag4s6IvHl2jC6OGnVgbA6qtYm+o75whsEeohQNokRwbLzPuEgudi5 XkuOkHkxf2lfHe0vXm3ivsG9LGVSePBJ2eVQ38Dzg5ZWu/Hs5ZiMXJTafb3BoTBq WknL/p48rxUcUM+r/pPzrx4pVD4AmQ5qvHS5rrMz+C5FhWV68p64UZWgCovUlzEB 2dks3I6V5CQOKuQykxYP1kmMTQ/nReiInLHHsS4LzkGRmiiMQcVZGyANOT36L59j H9zOR/1rLV2KrvITrXloTpxz/YWWjmGBQBLLOVn/nn032aEYQiLtvYHAlJBSixAO KK40uB9XvfD6fBNXgsswax6cLidEBw/RWzNwj6WUVY0DCiiGhRE= =YaVr -----END PGP SIGNATURE----- --c8JyeaiReRNoiMDS--