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. 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?