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 >>>> @@ -34,6 +34,7 @@ >>>> #include "qemu/range.h" >>>> #include "sysemu/kvm.h" >>>> #include "trace.h" >>>> +#include "qapi/error.h" >>>> >>>> struct vfio_group_head vfio_group_list = >>>> QLIST_HEAD_INITIALIZER(vfio_group_list); >>>> @@ -900,7 +901,8 @@ static void vfio_put_address_space(VFIOAddressSpace >>>> *space) >>>> } >>>> } >>>> >>>> -static int vfio_connect_container(VFIOGroup *group, AddressSpace *as) >>>> +static int vfio_connect_container(VFIOGroup *group, AddressSpace *as, >>>> + Error **errp) >>>> { >>>> VFIOContainer *container; >>>> int ret, fd; >>>> @@ -918,15 +920,15 @@ static int vfio_connect_container(VFIOGroup *group, >>>> AddressSpace *as) >>>> >>>> fd = qemu_open("/dev/vfio/vfio", O_RDWR); >>>> if (fd < 0) { >>>> - error_report("vfio: failed to open /dev/vfio/vfio: %m"); >>>> + error_setg_errno(errp, errno, "failed to open /dev/vfio/vfio"); >>>> ret = -errno; >>>> goto put_space_exit; >>>> } >>>> >>>> ret = ioctl(fd, VFIO_GET_API_VERSION); >>>> if (ret != VFIO_API_VERSION) { >>>> - error_report("vfio: supported vfio version: %d, " >>>> - "reported version: %d", VFIO_API_VERSION, ret); >>>> + error_setg(errp, "supported vfio version: %d, " >>>> + "reported version: %d", VFIO_API_VERSION, ret); >>>> ret = -EINVAL; >>>> goto close_fd_exit; >>>> } >>>> @@ -941,7 +943,7 @@ static int vfio_connect_container(VFIOGroup *group, >>>> AddressSpace *as) >>>> >>>> ret = ioctl(group->fd, VFIO_GROUP_SET_CONTAINER, &fd); >>>> if (ret) { >>>> - error_report("vfio: failed to set group container: %m"); >>>> + error_setg_errno(errp, errno, "failed to set group >>>> container"); >>>> ret = -errno; >>>> goto free_container_exit; >>>> } >>>> @@ -949,7 +951,7 @@ static int vfio_connect_container(VFIOGroup *group, >>>> AddressSpace *as) >>>> container->iommu_type = v2 ? VFIO_TYPE1v2_IOMMU : >>>> VFIO_TYPE1_IOMMU; >>>> ret = ioctl(fd, VFIO_SET_IOMMU, container->iommu_type); >>>> if (ret) { >>>> - error_report("vfio: failed to set iommu for container: %m"); >>>> + error_setg_errno(errp, errno, "failed to set iommu for >>>> container"); >>>> ret = -errno; >>>> goto free_container_exit; >>>> } >>>> @@ -976,7 +978,7 @@ static int vfio_connect_container(VFIOGroup *group, >>>> AddressSpace *as) >>>> >>>> ret = ioctl(group->fd, VFIO_GROUP_SET_CONTAINER, &fd); >>>> if (ret) { >>>> - error_report("vfio: failed to set group container: %m"); >>>> + error_setg_errno(errp, errno, "failed to set group >>>> container"); >>>> ret = -errno; >>>> goto free_container_exit; >>>> } >>>> @@ -984,7 +986,7 @@ static int vfio_connect_container(VFIOGroup *group, >>>> AddressSpace *as) >>>> v2 ? VFIO_SPAPR_TCE_v2_IOMMU : VFIO_SPAPR_TCE_IOMMU; >>>> ret = ioctl(fd, VFIO_SET_IOMMU, container->iommu_type); >>>> if (ret) { >>>> - error_report("vfio: failed to set iommu for container: %m"); >>>> + error_setg_errno(errp, errno, "failed to set iommu for >>>> container"); >>>> ret = -errno; >>>> goto free_container_exit; >>>> } >>>> @@ -997,7 +999,7 @@ static int vfio_connect_container(VFIOGroup *group, >>>> AddressSpace *as) >>>> if (!v2) { >>>> ret = ioctl(fd, VFIO_IOMMU_ENABLE); >>>> if (ret) { >>>> - error_report("vfio: failed to enable container: %m"); >>>> + error_setg_errno(errp, errno, "failed to enable >>>> container"); >>>> ret = -errno; >>>> goto free_container_exit; >>>> } >>>> @@ -1008,7 +1010,9 @@ static int vfio_connect_container(VFIOGroup *group, >>>> AddressSpace *as) >>>> &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. Thanks Eric >>>> + error_setg(errp, >>>> + "RAM memory listener initialization failed for >>>> container"); >>>> goto free_container_exit; >>>> } >>>> } >>>> @@ -1016,7 +1020,8 @@ static int vfio_connect_container(VFIOGroup *group, >>>> AddressSpace *as) >>>> info.argsz = sizeof(info); >>>> ret = ioctl(fd, VFIO_IOMMU_SPAPR_TCE_GET_INFO, &info); >>>> if (ret) { >>>> - error_report("vfio: VFIO_IOMMU_SPAPR_TCE_GET_INFO failed: >>>> %m"); >>>> + error_setg_errno(errp, errno, >>>> + "VFIO_IOMMU_SPAPR_TCE_GET_INFO failed"); >>>> ret = -errno; >>>> if (v2) { >>>> memory_listener_unregister(&container->prereg_listener); >>>> @@ -1033,6 +1038,8 @@ static int vfio_connect_container(VFIOGroup *group, >>>> AddressSpace *as) >>>> */ >>>> ret = vfio_spapr_remove_window(container, >>>> info.dma32_window_start); >>>> if (ret) { >>>> + error_setg_errno(errp, -ret, >>>> + "failed to remove existing window"); >>>> goto free_container_exit; >>>> } >>>> } else { >>>> @@ -1043,7 +1050,7 @@ static int vfio_connect_container(VFIOGroup *group, >>>> AddressSpace *as) >>>> 0x1000); >>>> } >>>> } else { >>>> - error_report("vfio: No available IOMMU models"); >>>> + error_setg(errp, "No available IOMMU models"); >>>> ret = -EINVAL; >>>> goto free_container_exit; >>>> } >>>> @@ -1054,7 +1061,8 @@ static int vfio_connect_container(VFIOGroup *group, >>>> AddressSpace *as) >>>> >>>> if (container->error) { >>>> ret = container->error; >>>> - error_report("vfio: memory listener initialization failed for >>>> container"); >>>> + error_setg_errno(errp, -ret, >>>> + "memory listener initialization failed for >>>> container"); >>>> goto listener_release_exit; >>>> } >>>> >>>> @@ -1120,6 +1128,7 @@ VFIOGroup *vfio_get_group(int groupid, AddressSpace >>>> *as) >>>> VFIOGroup *group; >>>> char path[32]; >>>> struct vfio_group_status status = { .argsz = sizeof(status) }; >>>> + Error *err = NULL; >>>> >>>> QLIST_FOREACH(group, &vfio_group_list, next) { >>>> if (group->groupid == groupid) { >>>> @@ -1158,8 +1167,9 @@ VFIOGroup *vfio_get_group(int groupid, AddressSpace >>>> *as) >>>> group->groupid = groupid; >>>> QLIST_INIT(&group->device_list); >>>> >>>> - if (vfio_connect_container(group, as)) { >>>> - error_report("vfio: failed to setup container for group %d", >>>> groupid); >>>> + if (vfio_connect_container(group, as, &err)) { >>>> + error_reportf_err(err, "vfio: failed to setup container for group >>>> %d", >>>> + groupid); >>>> goto close_fd_exit; >>>> } >>> >> >