From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:40357) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1btbF4-0007Rb-Ps for qemu-devel@nongnu.org; Mon, 10 Oct 2016 10:07:51 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1btbEz-0003ZB-VB for qemu-devel@nongnu.org; Mon, 10 Oct 2016 10:07:45 -0400 Received: from ozlabs.org ([103.22.144.67]:57559) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1btbEz-0003YP-Cu for qemu-devel@nongnu.org; Mon, 10 Oct 2016 10:07:41 -0400 Date: Tue, 11 Oct 2016 00:36:12 +1100 From: David Gibson Message-ID: <20161010133612.GI22498@umbus.fritz.box> References: <1475770058-20409-1-git-send-email-eric.auger@redhat.com> <1475770058-20409-9-git-send-email-eric.auger@redhat.com> <87vax4abi8.fsf@dusky.pond.sub.org> <8e31bba2-5659-0796-8efd-8b66da46a232@redhat.com> <20161010053407.GH22498@umbus.fritz.box> <59293d68-cadc-a370-2532-b1a2310d07dd@redhat.com> <874m4kxtwz.fsf@dusky.pond.sub.org> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="jQIvE3yXcK9X9HBh" Content-Disposition: inline In-Reply-To: Subject: Re: [Qemu-devel] [PATCH v5 08/17] vfio: Pass an Error object to vfio_connect_container List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Auger Eric Cc: Markus Armbruster , aik@ozlabs.ru, alex.williamson@redhat.com, eric.auger.pro@gmail.com, qemu-devel@nongnu.org --jQIvE3yXcK9X9HBh Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Mon, Oct 10, 2016 at 03:21:24PM +0200, Auger Eric wrote: > Hi Markus, > On 10/10/2016 14:36, Markus Armbruster wrote: > > Auger Eric writes: > >=20 > >> Hi, > >> > >> On 10/10/2016 07:34, David Gibson wrote: > >>> On Fri, Oct 07, 2016 at 09:36:09AM +0200, Auger Eric wrote: > >>>> Hi, > >>>> > >>>> On 07/10/2016 09:01, Markus Armbruster wrote: > >>>>> Eric Auger writes: > >>>>> > >>>>>> The error is currently simply reported in vfio_get_group. Don't > >>>>>> bother too much with the prefix which will be handled at upper lev= el, > >>>>>> later on. > >>>>>> > >>>>>> Also return an error value in case container->error is not 0 and > >>>>>> the container is teared down. > >>>>> > >>>>> "torn down", I think. > >>>> > >>>> Sure. I had a wrong feeling when writing this ... > >>>>> > >>>>> Is this a bug fix? See also below. > >>>>> > >>>>>> On vfio_spapr_remove_window failure, we also report an error where= as > >>>>>> it was silent before. > >>>>>> > >>>>>> Signed-off-by: Eric Auger > >>>>>> Reviewed-by: Markus Armbruster > >>>>>> > >>>>>> --- > >>>>>> > >>>>>> v4 -> v5: > >>>>>> - set ret to container->error > >>>>>> - mention error report on vfio_spapr_remove_window failure in the = commit > >>>>>> message > >>>>>> --- > >>>>>> hw/vfio/common.c | 40 +++++++++++++++++++++++++--------------- > >>>>>> 1 file changed, 25 insertions(+), 15 deletions(-) > >>>>>> > >>>>>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c > >>>>>> index 29188a1..85a7759 100644 > >>>>>> --- a/hw/vfio/common.c > >>>>>> +++ b/hw/vfio/common.c > > [...] > >>>>>> @@ -1008,7 +1010,9 @@ static int vfio_connect_container(VFIOGroup = *group, AddressSpace *as) > > container =3D g_malloc0(sizeof(*container)); > > container->space =3D space; > > container->fd =3D fd; > > if (ioctl(fd, VFIO_CHECK_EXTENSION, VFIO_TYPE1_IOMMU) || > > ioctl(fd, VFIO_CHECK_EXTENSION, VFIO_TYPE1v2_IOMMU)) { > > [...] > > } else if (ioctl(fd, VFIO_CHECK_EXTENSION, VFIO_SPAPR_TCE_IO= MMU) || > > ioctl(fd, VFIO_CHECK_EXTENSION, VFIO_SPAPR_TCE_v2= _IOMMU)) { > > struct vfio_iommu_spapr_tce_info info; > > bool v2 =3D !!ioctl(fd, VFIO_CHECK_EXTENSION, VFIO_SPAPR= _TCE_v2_IOMMU); > >=20 > > ret =3D ioctl(group->fd, VFIO_GROUP_SET_CONTAINER, &fd); > > if (ret) { > > error_setg_errno(errp, errno, "failed to set group c= ontainer"); > > ret =3D -errno; > > goto free_container_exit; > > } > > container->iommu_type =3D > > v2 ? VFIO_SPAPR_TCE_v2_IOMMU : VFIO_SPAPR_TCE_IOMMU; > > ret =3D ioctl(fd, VFIO_SET_IOMMU, container->iommu_type); > > if (ret) { > > error_setg_errno(errp, errno, "failed to set iommu f= or container"); > > ret =3D -errno; > > goto free_container_exit; > > } > >=20 > > /* > > * The host kernel code implementing VFIO_IOMMU_DISABLE = is called > > * when container fd is closed so we do not call it expl= icitly > > * in this file. > > */ > > if (!v2) { > > ret =3D ioctl(fd, VFIO_IOMMU_ENABLE); > > if (ret) { > > error_setg_errno(errp, errno, "failed to enable = container"); > > ret =3D -errno; > > goto free_container_exit; > > } > > } else { > > container->prereg_listener =3D vfio_prereg_listener; > >=20 > > memory_listener_register(&container->prereg_listener, > >>>>>> &address_space_memory); > >>>>>> if (container->error) { > >>>>> > >>>>> I tried to see where non-zero container->error comes from, but fail= ed. > >>>>> Can you help? > >>>> > >>>> Added Alexey in CC > >>>> > >>>> It is set in vfio_prereg_listener_region_add (spapr.c) > >>>> There is a comment there saying: > >>>> /* > >>>> * On the initfn path, store the first error in the container so we > >>>> * can gracefully fail. Runtime, there's not much we can do other > >>>> * than throw a hardware error. > >>>> */ > >>>> 1) by the way I should also s/initfn/realize now. > >>>> 2) by gracefully fail I understand the error should be properly > >>>> cascaded. Also when looking at the other vfio_memory_listener > >>>> registration below, ret is set to container->error. > >>>> 3) I could use error_setg_errno ... > >>>> > >>>> David, Alexey, could you confirm we should set the returned value to= the > >>>> container->error below? > >>> > >>> I think the right approach is to change container->error from an int > >>> to an Error *. As now, we stash the first error from the listener in > >>> there. > >>> > >>> realize() would check for a non-NULL error in the container after > >>> registering the listener, and if present, propagate it up to the > >>> caller. > >>> > >>>> > >>>> Thanks > >>>> > >>>> Eric > >>>> > >>>> > >>>>> > >>>>>> memory_listener_unregister(&container->prereg_lis= tener); > >>>>>> - error_report("vfio: RAM memory listener initializ= ation failed for container"); > >>>>>> + ret =3D container->error; > >> Thank you for your answers. OK to change container->error from an int > >> to an Error *. > >> > >> So I understand the fix just above is correct, ie. consider a non-NULL > >> container->error as an error that should be cascaded to the caller. > >> Currently I understand it is not since ret was left to 0. > >=20 > > If whatever sets container->error now can provide more useful error > > information by setting an Error, then replacing VFIOContainer member int > > error by Error *err makes sense. Else, I recommend to keep it simple > > and stick to errno codes. > >=20 > > My original question was about something else: I can't see what could > > have set container->error here. Have a look at the additional context I > > quoted above. Initially, container->error is zero. The ioctl()'s can't > > change it. That leaves memory_listener_register(). How can > > container->error be set? > My understanding is on > memory_listener_register(&container->prereg_listener, > &address_space_memory); >=20 > you get the vfio_prereg_listener_region_add called which is likely to > set container->error with the returned value of > VFIO_IOMMU_SPAPR_REGISTER_MEMORY ioctl. >=20 > Do I miss something? That's correct. memory_listener_register() will call the registered listener on all existing memory regions. The listener we've registered can set container->error in some circumstances, so that's how it can be set here. --=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 --jQIvE3yXcK9X9HBh Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBCAAGBQJX+5lKAAoJEGw4ysog2bOSWlgP/i3A1UFZRc9XE/GggUasj+0b XJeXrVbs5zRFEl3hOrY4sxkh/bPvUNqum4XesbxgIIVVjkfpAliRXZt56dN6Slc8 dFtCWQOdbNk1WrXd13JqV6I+NLRiUUynuM6zm69F3S863rJmO9qiT12HEYJapSw8 dNE7XeBKr1h5C5jpyjBvIEKh0N5ezzYPqZRtAncCfcAm4Vs8Cm/X0X9c1Feh5SA2 aMl6LR8AgJXCi0sMrz5YrlD/dskoH7gDsbI5bVnFqbjwxXrGUmm2Jgzhb5zfW1j1 Wlqb2zLRbG1sNV2z+RnKJFZMbo94kPgjJMPXOUzikESOYN8kG0G8NmA96b7hN5uv tDdaidetnhWd1IHy0JCVF6S4bZfclCAcjmU92x4XyZ4970cafgbKv/Kw443X1f3O EQdAt1d7wc2gEMvye3StQp53IOIMAXAwcNHIFXhPWuq42glxqENrDUfvqe9e0yI1 cwYvDVH88MgG0Nm/9Dld4m450yECg5Dhxs8hVyCPvtR4dkK/lpcqIymtMQCK6ndR 15zUoF7ugB89C1Hq9DYB7GaHqNRoYAB+j9SYHmtba56IXO3b/QVCIrdVov+N6fHY jLELBNVPeEWWNsa8Lgs+gZVdyxttXICOi38WjKQcXtSA9W+GEPeAb1POe1xMrq+4 Ut1orTCZpiFNol2MLJCy =MR3I -----END PGP SIGNATURE----- --jQIvE3yXcK9X9HBh--