On Mon, Oct 10, 2016 at 02:36:28PM +0200, Markus Armbruster wrote: > Auger Eric <eric.au...@redhat.com> writes: > > > 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 <eric.au...@redhat.com> 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 level, > >>>>> 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 whereas > >>>>> it was silent before. > >>>>> > >>>>> Signed-off-by: Eric Auger <eric.au...@redhat.com> > >>>>> Reviewed-by: Markus Armbruster <arm...@redhat.com> > >>>>> > >>>>> --- > >>>>> > >>>>> 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 = g_malloc0(sizeof(*container)); > container->space = space; > container->fd = 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_IOMMU) || > ioctl(fd, VFIO_CHECK_EXTENSION, > VFIO_SPAPR_TCE_v2_IOMMU)) { > struct vfio_iommu_spapr_tce_info info; > bool v2 = !!ioctl(fd, VFIO_CHECK_EXTENSION, > VFIO_SPAPR_TCE_v2_IOMMU); > > ret = ioctl(group->fd, VFIO_GROUP_SET_CONTAINER, &fd); > if (ret) { > error_setg_errno(errp, errno, "failed to set group > container"); > ret = -errno; > goto free_container_exit; > } > container->iommu_type = > v2 ? VFIO_SPAPR_TCE_v2_IOMMU : VFIO_SPAPR_TCE_IOMMU; > ret = ioctl(fd, VFIO_SET_IOMMU, container->iommu_type); > if (ret) { > error_setg_errno(errp, errno, "failed to set iommu for > container"); > ret = -errno; > goto free_container_exit; > } > > /* > * The host kernel code implementing VFIO_IOMMU_DISABLE is > called > * when container fd is closed so we do not call it explicitly > * in this file. > */ > if (!v2) { > ret = ioctl(fd, VFIO_IOMMU_ENABLE); > if (ret) { > error_setg_errno(errp, errno, "failed to enable > container"); > ret = -errno; > goto free_container_exit; > } > } else { > container->prereg_listener = vfio_prereg_listener; > > memory_listener_register(&container->prereg_listener, > >>>>> &address_space_memory); > >>>>> if (container->error) { > >>>> > >>>> I tried to see where non-zero container->error comes from, but failed. > >>>> 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_listener); > >>>>> - error_report("vfio: RAM memory listener initialization > >>>>> failed for container"); > >>>>> + ret = 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. > > 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.
So, the only thing that uses container->error right now is for the return code from the VFIO_IOMMU_SPAPR_REGISTER_MEMORY ioctl(). However in vfio_preref_listener_region_add() there's also one error_report() that would be a little more graceful to propagate up by the same mechanism. It probably makes more sense as a follow up change, though. > > 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? > -- 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
signature.asc
Description: PGP signature