| Message ID | 1475770058-20409-9-git-send-email-eric.auger@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Eric Auger <eric.auger@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. 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.auger@redhat.com> > Reviewed-by: Markus Armbruster <armbru@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? > memory_listener_unregister(&container->prereg_listener); > - error_report("vfio: RAM memory listener initialization failed for container"); > + ret = container->error; > + 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; > }
Hi, On 07/10/2016 09:01, Markus Armbruster wrote: > Eric Auger <eric.auger@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.auger@redhat.com> >> Reviewed-by: Markus Armbruster <armbru@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? Thanks Eric > >> memory_listener_unregister(&container->prereg_listener); >> - error_report("vfio: RAM memory listener initialization failed for container"); >> + ret = container->error; >> + 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; >> } >
On 07/10/16 18:36, Auger Eric wrote: > Hi, > > On 07/10/2016 09:01, Markus Armbruster wrote: >> Eric Auger <eric.auger@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.auger@redhat.com> >>> Reviewed-by: Markus Armbruster <armbru@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 ... Use on what Error* exactly? It is a listener, no return codes from there... > David, Alexey, could you confirm we should set the returned value to the > container->error below? The hw/vfio/common.c's ret is -errno but the hw/vfio/spapr.c's ret is ioctl()'s ret which is not a huge difference (as that specific ioctl() does not return successful non-zero codes) but yes, I should have used -errno in hw/vfio/spapr.c, not just ret. > > Thanks > > Eric > > >> >>> memory_listener_unregister(&container->prereg_listener); >>> - error_report("vfio: RAM memory listener initialization failed for container"); >>> + ret = container->error; >>> + 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; >>> } >>
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.auger@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.auger@redhat.com> > >> Reviewed-by: Markus Armbruster <armbru@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; > >> + 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; > >> } > > >
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.auger@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.auger@redhat.com> >>>> Reviewed-by: Markus Armbruster <armbru@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; >>>> } >>> >> >
Auger Eric <eric.auger@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.auger@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.auger@redhat.com> >>>>> Reviewed-by: Markus Armbruster <armbru@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?
Hi Markus, On 10/10/2016 14:36, Markus Armbruster wrote: > Auger Eric <eric.auger@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.auger@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.auger@redhat.com> >>>>>> Reviewed-by: Markus Armbruster <armbru@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? My understanding is on memory_listener_register(&container->prereg_listener, &address_space_memory); 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. Do I miss something? Thanks Eric >
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 <eric.auger@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.auger@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.auger@redhat.com> > >>>>>> Reviewed-by: Markus Armbruster <armbru@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? > My understanding is on > memory_listener_register(&container->prereg_listener, > &address_space_memory); > > 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. > > 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.
David Gibson <david@gibson.dropbear.id.au> writes: > 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 <eric.auger@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.auger@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.auger@redhat.com> >> >>>>>> Reviewed-by: Markus Armbruster <armbru@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? >> My understanding is on >> memory_listener_register(&container->prereg_listener, >> &address_space_memory); >> >> 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. >> >> 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. Okay. I think this series is fine as it is. If you want to change container->error, you can do that on top.
On Mon, Oct 10, 2016 at 02:36:28PM +0200, Markus Armbruster wrote: > Auger Eric <eric.auger@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.auger@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.auger@redhat.com> > >>>>> Reviewed-by: Markus Armbruster <armbru@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? >
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) { memory_listener_unregister(&container->prereg_listener); - error_report("vfio: RAM memory listener initialization failed for container"); + ret = container->error; + 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; }