On 07/10/16 18:36, 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 ...
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; >>> } >> -- Alexey