| Message ID | 20240514153130.394307-9-clg@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vfio: Improve error reporting (part 2 | expand |
On 5/14/24 17:31, Cédric Le Goater wrote: > Let the callers do the error reporting. Add documentation while at it. > > Signed-off-by: Cédric Le Goater <clg@redhat.com> > --- > > Changes in v6: > > - Fixed the line wrapping (Avihai) > - Fixed query_dirty_bitmap documentation (Avihai) > > Changes in v5: > > - Replaced error_setg() by error_setg_errno() in > vfio_devices_query_dirty_bitmap() and vfio_legacy_query_dirty_bitmap() > - ':' -> '-' in vfio_iommu_map_dirty_notify() > > include/hw/vfio/vfio-common.h | 4 +- > include/hw/vfio/vfio-container-base.h | 21 +++++++-- > hw/vfio/common.c | 61 ++++++++++++++++++--------- > hw/vfio/container-base.c | 7 +-- > hw/vfio/container.c | 14 +++--- > 5 files changed, 72 insertions(+), 35 deletions(-) > > diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h > index 46f88493634b5634a9c14a5caa33a463fbf2c50d..68911d36676667352e94a97895828aff4b194b57 100644 > --- a/include/hw/vfio/vfio-common.h > +++ b/include/hw/vfio/vfio-common.h > @@ -274,9 +274,9 @@ bool > vfio_devices_all_device_dirty_tracking(const VFIOContainerBase *bcontainer); > int vfio_devices_query_dirty_bitmap(const VFIOContainerBase *bcontainer, > VFIOBitmap *vbmap, hwaddr iova, > - hwaddr size); > + hwaddr size, Error **errp); > int vfio_get_dirty_bitmap(const VFIOContainerBase *bcontainer, uint64_t iova, > - uint64_t size, ram_addr_t ram_addr); > + uint64_t size, ram_addr_t ram_addr, Error **errp); > > /* Returns 0 on success, or a negative errno. */ > int vfio_device_get_name(VFIODevice *vbasedev, Error **errp); > diff --git a/include/hw/vfio/vfio-container-base.h b/include/hw/vfio/vfio-container-base.h > index 326ceea52a2030eec9dad289a9845866c4a8c090..42cfbf32dc737606c3f620d126f35d85d4833534 100644 > --- a/include/hw/vfio/vfio-container-base.h > +++ b/include/hw/vfio/vfio-container-base.h > @@ -84,8 +84,8 @@ void vfio_container_del_section_window(VFIOContainerBase *bcontainer, > int vfio_container_set_dirty_page_tracking(VFIOContainerBase *bcontainer, > bool start, Error **errp); > int vfio_container_query_dirty_bitmap(const VFIOContainerBase *bcontainer, > - VFIOBitmap *vbmap, > - hwaddr iova, hwaddr size); > + VFIOBitmap *vbmap, hwaddr iova, > + hwaddr size, Error **errp); > > void vfio_container_init(VFIOContainerBase *bcontainer, > VFIOAddressSpace *space, > @@ -138,9 +138,22 @@ struct VFIOIOMMUClass { > */ > int (*set_dirty_page_tracking)(const VFIOContainerBase *bcontainer, > bool start, Error **errp); > + /** > + * @query_dirty_bitmap > + * > + * Get bitmap of dirty pages from container > + * > + * @bcontainer: #VFIOContainerBase from which to get dirty pages > + * @vbmap: #VFIOBitmap internal bitmap structure > + * @iova: iova base address > + * @size: size of iova range > + * @errp: pointer to Error*, to store an error if it happens. > + * > + * Returns zero to indicate success and negative for error > + */ > int (*query_dirty_bitmap)(const VFIOContainerBase *bcontainer, > - VFIOBitmap *vbmap, > - hwaddr iova, hwaddr size); > + VFIOBitmap *vbmap, hwaddr iova, hwaddr size, > + Error **errp); > /* PCI specific */ > int (*pci_hot_reset)(VFIODevice *vbasedev, bool single); > > diff --git a/hw/vfio/common.c b/hw/vfio/common.c > index 919c4c52bc1590fd6c0bda37ba5881f58ff2ffff..9b5123d45fc1f9d5be4bbbf92898f89cd11e1363 100644 > --- a/hw/vfio/common.c > +++ b/hw/vfio/common.c > @@ -1140,8 +1140,8 @@ static int vfio_device_dma_logging_report(VFIODevice *vbasedev, hwaddr iova, > } > > int vfio_devices_query_dirty_bitmap(const VFIOContainerBase *bcontainer, > - VFIOBitmap *vbmap, hwaddr iova, > - hwaddr size) > + VFIOBitmap *vbmap, hwaddr iova, hwaddr size, > + Error **errp) > { > VFIODevice *vbasedev; > int ret; > @@ -1150,10 +1150,10 @@ int vfio_devices_query_dirty_bitmap(const VFIOContainerBase *bcontainer, > ret = vfio_device_dma_logging_report(vbasedev, iova, size, > vbmap->bitmap); > if (ret) { > - error_report("%s: Failed to get DMA logging report, iova: " > - "0x%" HWADDR_PRIx ", size: 0x%" HWADDR_PRIx > - ", err: %d (%s)", > - vbasedev->name, iova, size, ret, strerror(-ret)); > + error_setg_errno(errp, -ret, > + "%s: Failed to get DMA logging report, iova: " > + "0x%" HWADDR_PRIx ", size: 0x%" HWADDR_PRIx, > + vbasedev->name, iova, size); > > return ret; > } > @@ -1163,7 +1163,7 @@ int vfio_devices_query_dirty_bitmap(const VFIOContainerBase *bcontainer, > } > > int vfio_get_dirty_bitmap(const VFIOContainerBase *bcontainer, uint64_t iova, > - uint64_t size, ram_addr_t ram_addr) > + uint64_t size, ram_addr_t ram_addr, Error **errp) > { > bool all_device_dirty_tracking = > vfio_devices_all_device_dirty_tracking(bcontainer); > @@ -1180,13 +1180,17 @@ int vfio_get_dirty_bitmap(const VFIOContainerBase *bcontainer, uint64_t iova, > > ret = vfio_bitmap_alloc(&vbmap, size); > if (ret) { > + error_setg_errno(errp, -ret, > + "Failed to allocate dirty tracking bitmap"); > return ret; > } > > if (all_device_dirty_tracking) { > - ret = vfio_devices_query_dirty_bitmap(bcontainer, &vbmap, iova, size); > + ret = vfio_devices_query_dirty_bitmap(bcontainer, &vbmap, iova, size, > + errp); > } else { > - ret = vfio_container_query_dirty_bitmap(bcontainer, &vbmap, iova, size); > + ret = vfio_container_query_dirty_bitmap(bcontainer, &vbmap, iova, size, > + errp); > } > > if (ret) { > @@ -1234,12 +1238,13 @@ static void vfio_iommu_map_dirty_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb) > } > > ret = vfio_get_dirty_bitmap(bcontainer, iova, iotlb->addr_mask + 1, > - translated_addr); > + translated_addr, &local_err); > if (ret) { > - error_report("vfio_iommu_map_dirty_notify(%p, 0x%"HWADDR_PRIx", " > - "0x%"HWADDR_PRIx") = %d (%s)", > - bcontainer, iova, iotlb->addr_mask + 1, ret, > - strerror(-ret)); > + error_prepend(&local_err, > + "vfio_iommu_map_dirty_notify(%p, 0x%"HWADDR_PRIx", " > + "0x%"HWADDR_PRIx") failed - ", bcontainer, iova, > + iotlb->addr_mask + 1); > + error_report_err(local_err); > } > > out_unlock: > @@ -1259,12 +1264,19 @@ static int vfio_ram_discard_get_dirty_bitmap(MemoryRegionSection *section, > const ram_addr_t ram_addr = memory_region_get_ram_addr(section->mr) + > section->offset_within_region; > VFIORamDiscardListener *vrdl = opaque; > + Error *local_err = NULL; > + int ret; > > /* > * Sync the whole mapped region (spanning multiple individual mappings) > * in one go. > */ > - return vfio_get_dirty_bitmap(vrdl->bcontainer, iova, size, ram_addr); > + ret = vfio_get_dirty_bitmap(vrdl->bcontainer, iova, size, ram_addr, > + &local_err); > + if (ret) { > + error_report_err(local_err); > + } > + return ret; > } > > static int > @@ -1296,7 +1308,7 @@ vfio_sync_ram_discard_listener_dirty_bitmap(VFIOContainerBase *bcontainer, > } > > static int vfio_sync_dirty_bitmap(VFIOContainerBase *bcontainer, > - MemoryRegionSection *section) > + MemoryRegionSection *section, Error **errp) > { > ram_addr_t ram_addr; > > @@ -1327,7 +1339,14 @@ static int vfio_sync_dirty_bitmap(VFIOContainerBase *bcontainer, > } > return 0; > } else if (memory_region_has_ram_discard_manager(section->mr)) { > - return vfio_sync_ram_discard_listener_dirty_bitmap(bcontainer, section); > + int ret; > + > + ret = vfio_sync_ram_discard_listener_dirty_bitmap(bcontainer, section); > + if (ret) { > + error_setg(errp, > + "Failed to sync dirty bitmap with RAM discard listener"); > + return ret; > + } > } > > ram_addr = memory_region_get_ram_addr(section->mr) + > @@ -1335,7 +1354,7 @@ static int vfio_sync_dirty_bitmap(VFIOContainerBase *bcontainer, > > return vfio_get_dirty_bitmap(bcontainer, > REAL_HOST_PAGE_ALIGN(section->offset_within_address_space), > - int128_get64(section->size), ram_addr); > + int128_get64(section->size), ram_addr, errp); > } > > static void vfio_listener_log_sync(MemoryListener *listener, > @@ -1344,16 +1363,16 @@ static void vfio_listener_log_sync(MemoryListener *listener, > VFIOContainerBase *bcontainer = container_of(listener, VFIOContainerBase, > listener); > int ret; > + Error *local_err = NULL; > > if (vfio_listener_skipped_section(section)) { > return; > } > > if (vfio_devices_all_dirty_tracking(bcontainer)) { > - ret = vfio_sync_dirty_bitmap(bcontainer, section); > + ret = vfio_sync_dirty_bitmap(bcontainer, section, &local_err); > if (ret) { > - error_report("vfio: Failed to sync dirty bitmap, err: %d (%s)", ret, > - strerror(-ret)); > + error_report_err(local_err); > vfio_set_migration_error(ret); > } > } > diff --git a/hw/vfio/container-base.c b/hw/vfio/container-base.c > index 7c0764121d24b02b6c4e66e368d7dff78a6d65aa..4fa250c46fd16333d2e2358ede8b0a0afdb185b3 100644 > --- a/hw/vfio/container-base.c > +++ b/hw/vfio/container-base.c > @@ -64,11 +64,12 @@ int vfio_container_set_dirty_page_tracking(VFIOContainerBase *bcontainer, > } > > int vfio_container_query_dirty_bitmap(const VFIOContainerBase *bcontainer, > - VFIOBitmap *vbmap, > - hwaddr iova, hwaddr size) > + VFIOBitmap *vbmap, hwaddr iova, > + hwaddr size, Error **errp) > { > g_assert(bcontainer->ops->query_dirty_bitmap); > - return bcontainer->ops->query_dirty_bitmap(bcontainer, vbmap, iova, size); > + return bcontainer->ops->query_dirty_bitmap(bcontainer, vbmap, iova, size, > + errp); > } > > void vfio_container_init(VFIOContainerBase *bcontainer, VFIOAddressSpace *space, > diff --git a/hw/vfio/container.c b/hw/vfio/container.c > index c35221fbe7dc5453050f97cd186fc958e24f28f7..e00db6546c652c61a352f8e4cd65b212ecfb65bc 100644 > --- a/hw/vfio/container.c > +++ b/hw/vfio/container.c > @@ -130,6 +130,7 @@ static int vfio_legacy_dma_unmap(const VFIOContainerBase *bcontainer, > }; > bool need_dirty_sync = false; > int ret; > + Error *local_err = NULL; > > if (iotlb && vfio_devices_all_running_and_mig_active(bcontainer)) { > if (!vfio_devices_all_device_dirty_tracking(bcontainer) && > @@ -165,8 +166,9 @@ static int vfio_legacy_dma_unmap(const VFIOContainerBase *bcontainer, > > if (need_dirty_sync) { > ret = vfio_get_dirty_bitmap(bcontainer, iova, size, > - iotlb->translated_addr); > + iotlb->translated_addr, &local_err); > if (ret) { > + error_report_err(local_err); > return ret; > } > } > @@ -236,7 +238,8 @@ vfio_legacy_set_dirty_page_tracking(const VFIOContainerBase *bcontainer, > > static int vfio_legacy_query_dirty_bitmap(const VFIOContainerBase *bcontainer, > VFIOBitmap *vbmap, > - hwaddr iova, hwaddr size) > + hwaddr iova, hwaddr size, > + Error **errp) > { > const VFIOContainer *container = container_of(bcontainer, VFIOContainer, > bcontainer); > @@ -264,9 +267,10 @@ static int vfio_legacy_query_dirty_bitmap(const VFIOContainerBase *bcontainer, > ret = ioctl(container->fd, VFIO_IOMMU_DIRTY_PAGES, dbitmap); > if (ret) { > ret = -errno; > - error_report("Failed to get dirty bitmap for iova: 0x%"PRIx64 > - " size: 0x%"PRIx64" err: %d", (uint64_t)range->iova, > - (uint64_t)range->size, errno); > + error_setg_errno(errp, errno, > + "Failed to get dirty bitmap for iova: 0x%"PRIx64 > + " size: 0x%"PRIx64, (uint64_t)range->iova, > + (uint64_t)range->size); > } > > g_free(dbitmap); Reviewed-by: Eric Auger <eric.auger@redhat.com> Eric
On 14/05/2024 18:31, Cédric Le Goater wrote: > External email: Use caution opening links or attachments > > > Let the callers do the error reporting. Add documentation while at it. > > Signed-off-by: Cédric Le Goater <clg@redhat.com> > --- > > Changes in v6: > > - Fixed the line wrapping (Avihai) > - Fixed query_dirty_bitmap documentation (Avihai) > > Changes in v5: > > - Replaced error_setg() by error_setg_errno() in > vfio_devices_query_dirty_bitmap() and vfio_legacy_query_dirty_bitmap() > - ':' -> '-' in vfio_iommu_map_dirty_notify() > > include/hw/vfio/vfio-common.h | 4 +- > include/hw/vfio/vfio-container-base.h | 21 +++++++-- > hw/vfio/common.c | 61 ++++++++++++++++++--------- > hw/vfio/container-base.c | 7 +-- > hw/vfio/container.c | 14 +++--- > 5 files changed, 72 insertions(+), 35 deletions(-) > > diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h > index 46f88493634b5634a9c14a5caa33a463fbf2c50d..68911d36676667352e94a97895828aff4b194b57 100644 > --- a/include/hw/vfio/vfio-common.h > +++ b/include/hw/vfio/vfio-common.h > @@ -274,9 +274,9 @@ bool > vfio_devices_all_device_dirty_tracking(const VFIOContainerBase *bcontainer); > int vfio_devices_query_dirty_bitmap(const VFIOContainerBase *bcontainer, > VFIOBitmap *vbmap, hwaddr iova, > - hwaddr size); > + hwaddr size, Error **errp); [*] Can be wrapped like: int vfio_devices_query_dirty_bitmap(const VFIOContainerBase *bcontainer, VFIOBitmap *vbmap, hwaddr iova, hwaddr size, Error **errp); > int vfio_get_dirty_bitmap(const VFIOContainerBase *bcontainer, uint64_t iova, > - uint64_t size, ram_addr_t ram_addr); > + uint64_t size, ram_addr_t ram_addr, Error **errp); > > /* Returns 0 on success, or a negative errno. */ > int vfio_device_get_name(VFIODevice *vbasedev, Error **errp); > diff --git a/include/hw/vfio/vfio-container-base.h b/include/hw/vfio/vfio-container-base.h > index 326ceea52a2030eec9dad289a9845866c4a8c090..42cfbf32dc737606c3f620d126f35d85d4833534 100644 > --- a/include/hw/vfio/vfio-container-base.h > +++ b/include/hw/vfio/vfio-container-base.h > @@ -84,8 +84,8 @@ void vfio_container_del_section_window(VFIOContainerBase *bcontainer, > int vfio_container_set_dirty_page_tracking(VFIOContainerBase *bcontainer, > bool start, Error **errp); > int vfio_container_query_dirty_bitmap(const VFIOContainerBase *bcontainer, > - VFIOBitmap *vbmap, > - hwaddr iova, hwaddr size); > + VFIOBitmap *vbmap, hwaddr iova, > + hwaddr size, Error **errp); > > void vfio_container_init(VFIOContainerBase *bcontainer, > VFIOAddressSpace *space, > @@ -138,9 +138,22 @@ struct VFIOIOMMUClass { > */ > int (*set_dirty_page_tracking)(const VFIOContainerBase *bcontainer, > bool start, Error **errp); > + /** > + * @query_dirty_bitmap > + * > + * Get bitmap of dirty pages from container > + * > + * @bcontainer: #VFIOContainerBase from which to get dirty pages > + * @vbmap: #VFIOBitmap internal bitmap structure > + * @iova: iova base address > + * @size: size of iova range > + * @errp: pointer to Error*, to store an error if it happens. > + * > + * Returns zero to indicate success and negative for error > + */ > int (*query_dirty_bitmap)(const VFIOContainerBase *bcontainer, > - VFIOBitmap *vbmap, > - hwaddr iova, hwaddr size); > + VFIOBitmap *vbmap, hwaddr iova, hwaddr size, > + Error **errp); > /* PCI specific */ > int (*pci_hot_reset)(VFIODevice *vbasedev, bool single); > > diff --git a/hw/vfio/common.c b/hw/vfio/common.c > index 919c4c52bc1590fd6c0bda37ba5881f58ff2ffff..9b5123d45fc1f9d5be4bbbf92898f89cd11e1363 100644 > --- a/hw/vfio/common.c > +++ b/hw/vfio/common.c > @@ -1140,8 +1140,8 @@ static int vfio_device_dma_logging_report(VFIODevice *vbasedev, hwaddr iova, > } > > int vfio_devices_query_dirty_bitmap(const VFIOContainerBase *bcontainer, > - VFIOBitmap *vbmap, hwaddr iova, > - hwaddr size) > + VFIOBitmap *vbmap, hwaddr iova, hwaddr size, > + Error **errp) > { > VFIODevice *vbasedev; > int ret; > @@ -1150,10 +1150,10 @@ int vfio_devices_query_dirty_bitmap(const VFIOContainerBase *bcontainer, > ret = vfio_device_dma_logging_report(vbasedev, iova, size, > vbmap->bitmap); > if (ret) { > - error_report("%s: Failed to get DMA logging report, iova: " > - "0x%" HWADDR_PRIx ", size: 0x%" HWADDR_PRIx > - ", err: %d (%s)", > - vbasedev->name, iova, size, ret, strerror(-ret)); > + error_setg_errno(errp, -ret, > + "%s: Failed to get DMA logging report, iova: " > + "0x%" HWADDR_PRIx ", size: 0x%" HWADDR_PRIx, > + vbasedev->name, iova, size); > > return ret; > } > @@ -1163,7 +1163,7 @@ int vfio_devices_query_dirty_bitmap(const VFIOContainerBase *bcontainer, > } > > int vfio_get_dirty_bitmap(const VFIOContainerBase *bcontainer, uint64_t iova, > - uint64_t size, ram_addr_t ram_addr) > + uint64_t size, ram_addr_t ram_addr, Error **errp) > { > bool all_device_dirty_tracking = > vfio_devices_all_device_dirty_tracking(bcontainer); > @@ -1180,13 +1180,17 @@ int vfio_get_dirty_bitmap(const VFIOContainerBase *bcontainer, uint64_t iova, > > ret = vfio_bitmap_alloc(&vbmap, size); > if (ret) { > + error_setg_errno(errp, -ret, > + "Failed to allocate dirty tracking bitmap"); > return ret; > } > > if (all_device_dirty_tracking) { > - ret = vfio_devices_query_dirty_bitmap(bcontainer, &vbmap, iova, size); > + ret = vfio_devices_query_dirty_bitmap(bcontainer, &vbmap, iova, size, > + errp); > } else { > - ret = vfio_container_query_dirty_bitmap(bcontainer, &vbmap, iova, size); > + ret = vfio_container_query_dirty_bitmap(bcontainer, &vbmap, iova, size, > + errp); > } > > if (ret) { > @@ -1234,12 +1238,13 @@ static void vfio_iommu_map_dirty_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb) > } > > ret = vfio_get_dirty_bitmap(bcontainer, iova, iotlb->addr_mask + 1, > - translated_addr); > + translated_addr, &local_err); > if (ret) { > - error_report("vfio_iommu_map_dirty_notify(%p, 0x%"HWADDR_PRIx", " > - "0x%"HWADDR_PRIx") = %d (%s)", > - bcontainer, iova, iotlb->addr_mask + 1, ret, > - strerror(-ret)); > + error_prepend(&local_err, > + "vfio_iommu_map_dirty_notify(%p, 0x%"HWADDR_PRIx", " > + "0x%"HWADDR_PRIx") failed - ", bcontainer, iova, > + iotlb->addr_mask + 1); > + error_report_err(local_err); > } > > out_unlock: > @@ -1259,12 +1264,19 @@ static int vfio_ram_discard_get_dirty_bitmap(MemoryRegionSection *section, > const ram_addr_t ram_addr = memory_region_get_ram_addr(section->mr) + > section->offset_within_region; > VFIORamDiscardListener *vrdl = opaque; > + Error *local_err = NULL; > + int ret; > > /* > * Sync the whole mapped region (spanning multiple individual mappings) > * in one go. > */ > - return vfio_get_dirty_bitmap(vrdl->bcontainer, iova, size, ram_addr); > + ret = vfio_get_dirty_bitmap(vrdl->bcontainer, iova, size, ram_addr, > + &local_err); > + if (ret) { > + error_report_err(local_err); > + } > + return ret; > } > > static int > @@ -1296,7 +1308,7 @@ vfio_sync_ram_discard_listener_dirty_bitmap(VFIOContainerBase *bcontainer, > } > > static int vfio_sync_dirty_bitmap(VFIOContainerBase *bcontainer, > - MemoryRegionSection *section) > + MemoryRegionSection *section, Error **errp) > { > ram_addr_t ram_addr; > > @@ -1327,7 +1339,14 @@ static int vfio_sync_dirty_bitmap(VFIOContainerBase *bcontainer, > } > return 0; > } else if (memory_region_has_ram_discard_manager(section->mr)) { > - return vfio_sync_ram_discard_listener_dirty_bitmap(bcontainer, section); > + int ret; > + > + ret = vfio_sync_ram_discard_listener_dirty_bitmap(bcontainer, section); > + if (ret) { > + error_setg(errp, > + "Failed to sync dirty bitmap with RAM discard listener"); > + return ret; > + } Need to move "return ret" out of the "if (ret)" branch -- we need to immediately return from the else if branch also on success. With that, Reviewed-by: Avihai Horon <avihaih@nvidia.com> (Nit: there are 2 more places where line wrapping can be improved, if you want. Marked them with [*]) > } > > ram_addr = memory_region_get_ram_addr(section->mr) + > @@ -1335,7 +1354,7 @@ static int vfio_sync_dirty_bitmap(VFIOContainerBase *bcontainer, > > return vfio_get_dirty_bitmap(bcontainer, > REAL_HOST_PAGE_ALIGN(section->offset_within_address_space), > - int128_get64(section->size), ram_addr); > + int128_get64(section->size), ram_addr, errp); > } > > static void vfio_listener_log_sync(MemoryListener *listener, > @@ -1344,16 +1363,16 @@ static void vfio_listener_log_sync(MemoryListener *listener, > VFIOContainerBase *bcontainer = container_of(listener, VFIOContainerBase, > listener); > int ret; > + Error *local_err = NULL; > > if (vfio_listener_skipped_section(section)) { > return; > } > > if (vfio_devices_all_dirty_tracking(bcontainer)) { > - ret = vfio_sync_dirty_bitmap(bcontainer, section); > + ret = vfio_sync_dirty_bitmap(bcontainer, section, &local_err); > if (ret) { > - error_report("vfio: Failed to sync dirty bitmap, err: %d (%s)", ret, > - strerror(-ret)); > + error_report_err(local_err); > vfio_set_migration_error(ret); > } > } > diff --git a/hw/vfio/container-base.c b/hw/vfio/container-base.c > index 7c0764121d24b02b6c4e66e368d7dff78a6d65aa..4fa250c46fd16333d2e2358ede8b0a0afdb185b3 100644 > --- a/hw/vfio/container-base.c > +++ b/hw/vfio/container-base.c > @@ -64,11 +64,12 @@ int vfio_container_set_dirty_page_tracking(VFIOContainerBase *bcontainer, > } > > int vfio_container_query_dirty_bitmap(const VFIOContainerBase *bcontainer, > - VFIOBitmap *vbmap, > - hwaddr iova, hwaddr size) > + VFIOBitmap *vbmap, hwaddr iova, > + hwaddr size, Error **errp) > { > g_assert(bcontainer->ops->query_dirty_bitmap); > - return bcontainer->ops->query_dirty_bitmap(bcontainer, vbmap, iova, size); > + return bcontainer->ops->query_dirty_bitmap(bcontainer, vbmap, iova, size, > + errp); > } > > void vfio_container_init(VFIOContainerBase *bcontainer, VFIOAddressSpace *space, > diff --git a/hw/vfio/container.c b/hw/vfio/container.c > index c35221fbe7dc5453050f97cd186fc958e24f28f7..e00db6546c652c61a352f8e4cd65b212ecfb65bc 100644 > --- a/hw/vfio/container.c > +++ b/hw/vfio/container.c > @@ -130,6 +130,7 @@ static int vfio_legacy_dma_unmap(const VFIOContainerBase *bcontainer, > }; > bool need_dirty_sync = false; > int ret; > + Error *local_err = NULL; > > if (iotlb && vfio_devices_all_running_and_mig_active(bcontainer)) { > if (!vfio_devices_all_device_dirty_tracking(bcontainer) && > @@ -165,8 +166,9 @@ static int vfio_legacy_dma_unmap(const VFIOContainerBase *bcontainer, > > if (need_dirty_sync) { > ret = vfio_get_dirty_bitmap(bcontainer, iova, size, > - iotlb->translated_addr); > + iotlb->translated_addr, &local_err); > if (ret) { > + error_report_err(local_err); > return ret; > } > } > @@ -236,7 +238,8 @@ vfio_legacy_set_dirty_page_tracking(const VFIOContainerBase *bcontainer, > > static int vfio_legacy_query_dirty_bitmap(const VFIOContainerBase *bcontainer, > VFIOBitmap *vbmap, > - hwaddr iova, hwaddr size) > + hwaddr iova, hwaddr size, > + Error **errp) [*] Can be wrapped like: static int vfio_legacy_query_dirty_bitmap(const VFIOContainerBase *bcontainer, VFIOBitmap *vbmap, hwaddr iova, hwaddr size, Error **errp) Thanks. > { > const VFIOContainer *container = container_of(bcontainer, VFIOContainer, > bcontainer); > @@ -264,9 +267,10 @@ static int vfio_legacy_query_dirty_bitmap(const VFIOContainerBase *bcontainer, > ret = ioctl(container->fd, VFIO_IOMMU_DIRTY_PAGES, dbitmap); > if (ret) { > ret = -errno; > - error_report("Failed to get dirty bitmap for iova: 0x%"PRIx64 > - " size: 0x%"PRIx64" err: %d", (uint64_t)range->iova, > - (uint64_t)range->size, errno); > + error_setg_errno(errp, errno, > + "Failed to get dirty bitmap for iova: 0x%"PRIx64 > + " size: 0x%"PRIx64, (uint64_t)range->iova, > + (uint64_t)range->size); > } > > g_free(dbitmap); > -- > 2.45.0 >
On 5/16/24 10:42, Avihai Horon wrote: > > On 14/05/2024 18:31, Cédric Le Goater wrote: >> External email: Use caution opening links or attachments >> >> >> Let the callers do the error reporting. Add documentation while at it. >> >> Signed-off-by: Cédric Le Goater <clg@redhat.com> >> --- >> >> Changes in v6: >> >> - Fixed the line wrapping (Avihai) >> - Fixed query_dirty_bitmap documentation (Avihai) >> >> Changes in v5: >> >> - Replaced error_setg() by error_setg_errno() in >> vfio_devices_query_dirty_bitmap() and vfio_legacy_query_dirty_bitmap() >> - ':' -> '-' in vfio_iommu_map_dirty_notify() >> >> include/hw/vfio/vfio-common.h | 4 +- >> include/hw/vfio/vfio-container-base.h | 21 +++++++-- >> hw/vfio/common.c | 61 ++++++++++++++++++--------- >> hw/vfio/container-base.c | 7 +-- >> hw/vfio/container.c | 14 +++--- >> 5 files changed, 72 insertions(+), 35 deletions(-) >> >> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h >> index 46f88493634b5634a9c14a5caa33a463fbf2c50d..68911d36676667352e94a97895828aff4b194b57 100644 >> --- a/include/hw/vfio/vfio-common.h >> +++ b/include/hw/vfio/vfio-common.h >> @@ -274,9 +274,9 @@ bool >> vfio_devices_all_device_dirty_tracking(const VFIOContainerBase *bcontainer); >> int vfio_devices_query_dirty_bitmap(const VFIOContainerBase *bcontainer, >> VFIOBitmap *vbmap, hwaddr iova, >> - hwaddr size); >> + hwaddr size, Error **errp); > > [*] Can be wrapped like: > > int vfio_devices_query_dirty_bitmap(const VFIOContainerBase *bcontainer, > VFIOBitmap *vbmap, hwaddr iova, hwaddr size, > Error **errp); OK. So I changed all *_dirty_bitmap() prototypes. >> int vfio_get_dirty_bitmap(const VFIOContainerBase *bcontainer, uint64_t iova, >> - uint64_t size, ram_addr_t ram_addr); >> + uint64_t size, ram_addr_t ram_addr, Error **errp); >> >> /* Returns 0 on success, or a negative errno. */ >> int vfio_device_get_name(VFIODevice *vbasedev, Error **errp); >> diff --git a/include/hw/vfio/vfio-container-base.h b/include/hw/vfio/vfio-container-base.h >> index 326ceea52a2030eec9dad289a9845866c4a8c090..42cfbf32dc737606c3f620d126f35d85d4833534 100644 >> --- a/include/hw/vfio/vfio-container-base.h >> +++ b/include/hw/vfio/vfio-container-base.h >> @@ -84,8 +84,8 @@ void vfio_container_del_section_window(VFIOContainerBase *bcontainer, >> int vfio_container_set_dirty_page_tracking(VFIOContainerBase *bcontainer, >> bool start, Error **errp); >> int vfio_container_query_dirty_bitmap(const VFIOContainerBase *bcontainer, >> - VFIOBitmap *vbmap, >> - hwaddr iova, hwaddr size); >> + VFIOBitmap *vbmap, hwaddr iova, >> + hwaddr size, Error **errp); >> >> void vfio_container_init(VFIOContainerBase *bcontainer, >> VFIOAddressSpace *space, >> @@ -138,9 +138,22 @@ struct VFIOIOMMUClass { >> */ >> int (*set_dirty_page_tracking)(const VFIOContainerBase *bcontainer, >> bool start, Error **errp); >> + /** >> + * @query_dirty_bitmap >> + * >> + * Get bitmap of dirty pages from container >> + * >> + * @bcontainer: #VFIOContainerBase from which to get dirty pages >> + * @vbmap: #VFIOBitmap internal bitmap structure >> + * @iova: iova base address >> + * @size: size of iova range >> + * @errp: pointer to Error*, to store an error if it happens. >> + * >> + * Returns zero to indicate success and negative for error >> + */ >> int (*query_dirty_bitmap)(const VFIOContainerBase *bcontainer, >> - VFIOBitmap *vbmap, >> - hwaddr iova, hwaddr size); >> + VFIOBitmap *vbmap, hwaddr iova, hwaddr size, >> + Error **errp); >> /* PCI specific */ >> int (*pci_hot_reset)(VFIODevice *vbasedev, bool single); >> >> diff --git a/hw/vfio/common.c b/hw/vfio/common.c >> index 919c4c52bc1590fd6c0bda37ba5881f58ff2ffff..9b5123d45fc1f9d5be4bbbf92898f89cd11e1363 100644 >> --- a/hw/vfio/common.c >> +++ b/hw/vfio/common.c >> @@ -1140,8 +1140,8 @@ static int vfio_device_dma_logging_report(VFIODevice *vbasedev, hwaddr iova, >> } >> >> int vfio_devices_query_dirty_bitmap(const VFIOContainerBase *bcontainer, >> - VFIOBitmap *vbmap, hwaddr iova, >> - hwaddr size) >> + VFIOBitmap *vbmap, hwaddr iova, hwaddr size, >> + Error **errp) >> { >> VFIODevice *vbasedev; >> int ret; >> @@ -1150,10 +1150,10 @@ int vfio_devices_query_dirty_bitmap(const VFIOContainerBase *bcontainer, >> ret = vfio_device_dma_logging_report(vbasedev, iova, size, >> vbmap->bitmap); >> if (ret) { >> - error_report("%s: Failed to get DMA logging report, iova: " >> - "0x%" HWADDR_PRIx ", size: 0x%" HWADDR_PRIx >> - ", err: %d (%s)", >> - vbasedev->name, iova, size, ret, strerror(-ret)); >> + error_setg_errno(errp, -ret, >> + "%s: Failed to get DMA logging report, iova: " >> + "0x%" HWADDR_PRIx ", size: 0x%" HWADDR_PRIx, >> + vbasedev->name, iova, size); >> >> return ret; >> } >> @@ -1163,7 +1163,7 @@ int vfio_devices_query_dirty_bitmap(const VFIOContainerBase *bcontainer, >> } >> >> int vfio_get_dirty_bitmap(const VFIOContainerBase *bcontainer, uint64_t iova, >> - uint64_t size, ram_addr_t ram_addr) >> + uint64_t size, ram_addr_t ram_addr, Error **errp) >> { >> bool all_device_dirty_tracking = >> vfio_devices_all_device_dirty_tracking(bcontainer); >> @@ -1180,13 +1180,17 @@ int vfio_get_dirty_bitmap(const VFIOContainerBase *bcontainer, uint64_t iova, >> >> ret = vfio_bitmap_alloc(&vbmap, size); >> if (ret) { >> + error_setg_errno(errp, -ret, >> + "Failed to allocate dirty tracking bitmap"); >> return ret; >> } >> >> if (all_device_dirty_tracking) { >> - ret = vfio_devices_query_dirty_bitmap(bcontainer, &vbmap, iova, size); >> + ret = vfio_devices_query_dirty_bitmap(bcontainer, &vbmap, iova, size, >> + errp); >> } else { >> - ret = vfio_container_query_dirty_bitmap(bcontainer, &vbmap, iova, size); >> + ret = vfio_container_query_dirty_bitmap(bcontainer, &vbmap, iova, size, >> + errp); >> } >> >> if (ret) { >> @@ -1234,12 +1238,13 @@ static void vfio_iommu_map_dirty_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb) >> } >> >> ret = vfio_get_dirty_bitmap(bcontainer, iova, iotlb->addr_mask + 1, >> - translated_addr); >> + translated_addr, &local_err); >> if (ret) { >> - error_report("vfio_iommu_map_dirty_notify(%p, 0x%"HWADDR_PRIx", " >> - "0x%"HWADDR_PRIx") = %d (%s)", >> - bcontainer, iova, iotlb->addr_mask + 1, ret, >> - strerror(-ret)); >> + error_prepend(&local_err, >> + "vfio_iommu_map_dirty_notify(%p, 0x%"HWADDR_PRIx", " >> + "0x%"HWADDR_PRIx") failed - ", bcontainer, iova, >> + iotlb->addr_mask + 1); >> + error_report_err(local_err); >> } >> >> out_unlock: >> @@ -1259,12 +1264,19 @@ static int vfio_ram_discard_get_dirty_bitmap(MemoryRegionSection *section, >> const ram_addr_t ram_addr = memory_region_get_ram_addr(section->mr) + >> section->offset_within_region; >> VFIORamDiscardListener *vrdl = opaque; >> + Error *local_err = NULL; >> + int ret; >> >> /* >> * Sync the whole mapped region (spanning multiple individual mappings) >> * in one go. >> */ >> - return vfio_get_dirty_bitmap(vrdl->bcontainer, iova, size, ram_addr); >> + ret = vfio_get_dirty_bitmap(vrdl->bcontainer, iova, size, ram_addr, >> + &local_err); >> + if (ret) { >> + error_report_err(local_err); >> + } >> + return ret; >> } >> >> static int >> @@ -1296,7 +1308,7 @@ vfio_sync_ram_discard_listener_dirty_bitmap(VFIOContainerBase *bcontainer, >> } >> >> static int vfio_sync_dirty_bitmap(VFIOContainerBase *bcontainer, >> - MemoryRegionSection *section) >> + MemoryRegionSection *section, Error **errp) >> { >> ram_addr_t ram_addr; >> >> @@ -1327,7 +1339,14 @@ static int vfio_sync_dirty_bitmap(VFIOContainerBase *bcontainer, >> } >> return 0; >> } else if (memory_region_has_ram_discard_manager(section->mr)) { >> - return vfio_sync_ram_discard_listener_dirty_bitmap(bcontainer, section); >> + int ret; >> + >> + ret = vfio_sync_ram_discard_listener_dirty_bitmap(bcontainer, section); >> + if (ret) { >> + error_setg(errp, >> + "Failed to sync dirty bitmap with RAM discard listener"); >> + return ret; >> + } > > Need to move "return ret" out of the "if (ret)" branch -- we need to immediately return from the else if branch also on success. drat. Good catch. This deserves a v7. Thanks, C. > With that, > Reviewed-by: Avihai Horon <avihaih@nvidia.com> > > (Nit: there are 2 more places where line wrapping can be improved, if you want. Marked them with [*]) > >> } >> >> ram_addr = memory_region_get_ram_addr(section->mr) + >> @@ -1335,7 +1354,7 @@ static int vfio_sync_dirty_bitmap(VFIOContainerBase *bcontainer, >> >> return vfio_get_dirty_bitmap(bcontainer, >> REAL_HOST_PAGE_ALIGN(section->offset_within_address_space), >> - int128_get64(section->size), ram_addr); >> + int128_get64(section->size), ram_addr, errp); >> } >> >> static void vfio_listener_log_sync(MemoryListener *listener, >> @@ -1344,16 +1363,16 @@ static void vfio_listener_log_sync(MemoryListener *listener, >> VFIOContainerBase *bcontainer = container_of(listener, VFIOContainerBase, >> listener); >> int ret; >> + Error *local_err = NULL; >> >> if (vfio_listener_skipped_section(section)) { >> return; >> } >> >> if (vfio_devices_all_dirty_tracking(bcontainer)) { >> - ret = vfio_sync_dirty_bitmap(bcontainer, section); >> + ret = vfio_sync_dirty_bitmap(bcontainer, section, &local_err); >> if (ret) { >> - error_report("vfio: Failed to sync dirty bitmap, err: %d (%s)", ret, >> - strerror(-ret)); >> + error_report_err(local_err); >> vfio_set_migration_error(ret); >> } >> } >> diff --git a/hw/vfio/container-base.c b/hw/vfio/container-base.c >> index 7c0764121d24b02b6c4e66e368d7dff78a6d65aa..4fa250c46fd16333d2e2358ede8b0a0afdb185b3 100644 >> --- a/hw/vfio/container-base.c >> +++ b/hw/vfio/container-base.c >> @@ -64,11 +64,12 @@ int vfio_container_set_dirty_page_tracking(VFIOContainerBase *bcontainer, >> } >> >> int vfio_container_query_dirty_bitmap(const VFIOContainerBase *bcontainer, >> - VFIOBitmap *vbmap, >> - hwaddr iova, hwaddr size) >> + VFIOBitmap *vbmap, hwaddr iova, >> + hwaddr size, Error **errp) >> { >> g_assert(bcontainer->ops->query_dirty_bitmap); >> - return bcontainer->ops->query_dirty_bitmap(bcontainer, vbmap, iova, size); >> + return bcontainer->ops->query_dirty_bitmap(bcontainer, vbmap, iova, size, >> + errp); >> } >> >> void vfio_container_init(VFIOContainerBase *bcontainer, VFIOAddressSpace *space, >> diff --git a/hw/vfio/container.c b/hw/vfio/container.c >> index c35221fbe7dc5453050f97cd186fc958e24f28f7..e00db6546c652c61a352f8e4cd65b212ecfb65bc 100644 >> --- a/hw/vfio/container.c >> +++ b/hw/vfio/container.c >> @@ -130,6 +130,7 @@ static int vfio_legacy_dma_unmap(const VFIOContainerBase *bcontainer, >> }; >> bool need_dirty_sync = false; >> int ret; >> + Error *local_err = NULL; >> >> if (iotlb && vfio_devices_all_running_and_mig_active(bcontainer)) { >> if (!vfio_devices_all_device_dirty_tracking(bcontainer) && >> @@ -165,8 +166,9 @@ static int vfio_legacy_dma_unmap(const VFIOContainerBase *bcontainer, >> >> if (need_dirty_sync) { >> ret = vfio_get_dirty_bitmap(bcontainer, iova, size, >> - iotlb->translated_addr); >> + iotlb->translated_addr, &local_err); >> if (ret) { >> + error_report_err(local_err); >> return ret; >> } >> } >> @@ -236,7 +238,8 @@ vfio_legacy_set_dirty_page_tracking(const VFIOContainerBase *bcontainer, >> >> static int vfio_legacy_query_dirty_bitmap(const VFIOContainerBase *bcontainer, >> VFIOBitmap *vbmap, >> - hwaddr iova, hwaddr size) >> + hwaddr iova, hwaddr size, >> + Error **errp) > > [*] Can be wrapped like: > > static int vfio_legacy_query_dirty_bitmap(const VFIOContainerBase *bcontainer, > VFIOBitmap *vbmap, hwaddr iova, > hwaddr size, Error **errp) > > Thanks. > >> { >> const VFIOContainer *container = container_of(bcontainer, VFIOContainer, >> bcontainer); >> @@ -264,9 +267,10 @@ static int vfio_legacy_query_dirty_bitmap(const VFIOContainerBase *bcontainer, >> ret = ioctl(container->fd, VFIO_IOMMU_DIRTY_PAGES, dbitmap); >> if (ret) { >> ret = -errno; >> - error_report("Failed to get dirty bitmap for iova: 0x%"PRIx64 >> - " size: 0x%"PRIx64" err: %d", (uint64_t)range->iova, >> - (uint64_t)range->size, errno); >> + error_setg_errno(errp, errno, >> + "Failed to get dirty bitmap for iova: 0x%"PRIx64 >> + " size: 0x%"PRIx64, (uint64_t)range->iova, >> + (uint64_t)range->size); >> } >> >> g_free(dbitmap); >> -- >> 2.45.0 >> >
diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h index 46f88493634b5634a9c14a5caa33a463fbf2c50d..68911d36676667352e94a97895828aff4b194b57 100644 --- a/include/hw/vfio/vfio-common.h +++ b/include/hw/vfio/vfio-common.h @@ -274,9 +274,9 @@ bool vfio_devices_all_device_dirty_tracking(const VFIOContainerBase *bcontainer); int vfio_devices_query_dirty_bitmap(const VFIOContainerBase *bcontainer, VFIOBitmap *vbmap, hwaddr iova, - hwaddr size); + hwaddr size, Error **errp); int vfio_get_dirty_bitmap(const VFIOContainerBase *bcontainer, uint64_t iova, - uint64_t size, ram_addr_t ram_addr); + uint64_t size, ram_addr_t ram_addr, Error **errp); /* Returns 0 on success, or a negative errno. */ int vfio_device_get_name(VFIODevice *vbasedev, Error **errp); diff --git a/include/hw/vfio/vfio-container-base.h b/include/hw/vfio/vfio-container-base.h index 326ceea52a2030eec9dad289a9845866c4a8c090..42cfbf32dc737606c3f620d126f35d85d4833534 100644 --- a/include/hw/vfio/vfio-container-base.h +++ b/include/hw/vfio/vfio-container-base.h @@ -84,8 +84,8 @@ void vfio_container_del_section_window(VFIOContainerBase *bcontainer, int vfio_container_set_dirty_page_tracking(VFIOContainerBase *bcontainer, bool start, Error **errp); int vfio_container_query_dirty_bitmap(const VFIOContainerBase *bcontainer, - VFIOBitmap *vbmap, - hwaddr iova, hwaddr size); + VFIOBitmap *vbmap, hwaddr iova, + hwaddr size, Error **errp); void vfio_container_init(VFIOContainerBase *bcontainer, VFIOAddressSpace *space, @@ -138,9 +138,22 @@ struct VFIOIOMMUClass { */ int (*set_dirty_page_tracking)(const VFIOContainerBase *bcontainer, bool start, Error **errp); + /** + * @query_dirty_bitmap + * + * Get bitmap of dirty pages from container + * + * @bcontainer: #VFIOContainerBase from which to get dirty pages + * @vbmap: #VFIOBitmap internal bitmap structure + * @iova: iova base address + * @size: size of iova range + * @errp: pointer to Error*, to store an error if it happens. + * + * Returns zero to indicate success and negative for error + */ int (*query_dirty_bitmap)(const VFIOContainerBase *bcontainer, - VFIOBitmap *vbmap, - hwaddr iova, hwaddr size); + VFIOBitmap *vbmap, hwaddr iova, hwaddr size, + Error **errp); /* PCI specific */ int (*pci_hot_reset)(VFIODevice *vbasedev, bool single); diff --git a/hw/vfio/common.c b/hw/vfio/common.c index 919c4c52bc1590fd6c0bda37ba5881f58ff2ffff..9b5123d45fc1f9d5be4bbbf92898f89cd11e1363 100644 --- a/hw/vfio/common.c +++ b/hw/vfio/common.c @@ -1140,8 +1140,8 @@ static int vfio_device_dma_logging_report(VFIODevice *vbasedev, hwaddr iova, } int vfio_devices_query_dirty_bitmap(const VFIOContainerBase *bcontainer, - VFIOBitmap *vbmap, hwaddr iova, - hwaddr size) + VFIOBitmap *vbmap, hwaddr iova, hwaddr size, + Error **errp) { VFIODevice *vbasedev; int ret; @@ -1150,10 +1150,10 @@ int vfio_devices_query_dirty_bitmap(const VFIOContainerBase *bcontainer, ret = vfio_device_dma_logging_report(vbasedev, iova, size, vbmap->bitmap); if (ret) { - error_report("%s: Failed to get DMA logging report, iova: " - "0x%" HWADDR_PRIx ", size: 0x%" HWADDR_PRIx - ", err: %d (%s)", - vbasedev->name, iova, size, ret, strerror(-ret)); + error_setg_errno(errp, -ret, + "%s: Failed to get DMA logging report, iova: " + "0x%" HWADDR_PRIx ", size: 0x%" HWADDR_PRIx, + vbasedev->name, iova, size); return ret; } @@ -1163,7 +1163,7 @@ int vfio_devices_query_dirty_bitmap(const VFIOContainerBase *bcontainer, } int vfio_get_dirty_bitmap(const VFIOContainerBase *bcontainer, uint64_t iova, - uint64_t size, ram_addr_t ram_addr) + uint64_t size, ram_addr_t ram_addr, Error **errp) { bool all_device_dirty_tracking = vfio_devices_all_device_dirty_tracking(bcontainer); @@ -1180,13 +1180,17 @@ int vfio_get_dirty_bitmap(const VFIOContainerBase *bcontainer, uint64_t iova, ret = vfio_bitmap_alloc(&vbmap, size); if (ret) { + error_setg_errno(errp, -ret, + "Failed to allocate dirty tracking bitmap"); return ret; } if (all_device_dirty_tracking) { - ret = vfio_devices_query_dirty_bitmap(bcontainer, &vbmap, iova, size); + ret = vfio_devices_query_dirty_bitmap(bcontainer, &vbmap, iova, size, + errp); } else { - ret = vfio_container_query_dirty_bitmap(bcontainer, &vbmap, iova, size); + ret = vfio_container_query_dirty_bitmap(bcontainer, &vbmap, iova, size, + errp); } if (ret) { @@ -1234,12 +1238,13 @@ static void vfio_iommu_map_dirty_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb) } ret = vfio_get_dirty_bitmap(bcontainer, iova, iotlb->addr_mask + 1, - translated_addr); + translated_addr, &local_err); if (ret) { - error_report("vfio_iommu_map_dirty_notify(%p, 0x%"HWADDR_PRIx", " - "0x%"HWADDR_PRIx") = %d (%s)", - bcontainer, iova, iotlb->addr_mask + 1, ret, - strerror(-ret)); + error_prepend(&local_err, + "vfio_iommu_map_dirty_notify(%p, 0x%"HWADDR_PRIx", " + "0x%"HWADDR_PRIx") failed - ", bcontainer, iova, + iotlb->addr_mask + 1); + error_report_err(local_err); } out_unlock: @@ -1259,12 +1264,19 @@ static int vfio_ram_discard_get_dirty_bitmap(MemoryRegionSection *section, const ram_addr_t ram_addr = memory_region_get_ram_addr(section->mr) + section->offset_within_region; VFIORamDiscardListener *vrdl = opaque; + Error *local_err = NULL; + int ret; /* * Sync the whole mapped region (spanning multiple individual mappings) * in one go. */ - return vfio_get_dirty_bitmap(vrdl->bcontainer, iova, size, ram_addr); + ret = vfio_get_dirty_bitmap(vrdl->bcontainer, iova, size, ram_addr, + &local_err); + if (ret) { + error_report_err(local_err); + } + return ret; } static int @@ -1296,7 +1308,7 @@ vfio_sync_ram_discard_listener_dirty_bitmap(VFIOContainerBase *bcontainer, } static int vfio_sync_dirty_bitmap(VFIOContainerBase *bcontainer, - MemoryRegionSection *section) + MemoryRegionSection *section, Error **errp) { ram_addr_t ram_addr; @@ -1327,7 +1339,14 @@ static int vfio_sync_dirty_bitmap(VFIOContainerBase *bcontainer, } return 0; } else if (memory_region_has_ram_discard_manager(section->mr)) { - return vfio_sync_ram_discard_listener_dirty_bitmap(bcontainer, section); + int ret; + + ret = vfio_sync_ram_discard_listener_dirty_bitmap(bcontainer, section); + if (ret) { + error_setg(errp, + "Failed to sync dirty bitmap with RAM discard listener"); + return ret; + } } ram_addr = memory_region_get_ram_addr(section->mr) + @@ -1335,7 +1354,7 @@ static int vfio_sync_dirty_bitmap(VFIOContainerBase *bcontainer, return vfio_get_dirty_bitmap(bcontainer, REAL_HOST_PAGE_ALIGN(section->offset_within_address_space), - int128_get64(section->size), ram_addr); + int128_get64(section->size), ram_addr, errp); } static void vfio_listener_log_sync(MemoryListener *listener, @@ -1344,16 +1363,16 @@ static void vfio_listener_log_sync(MemoryListener *listener, VFIOContainerBase *bcontainer = container_of(listener, VFIOContainerBase, listener); int ret; + Error *local_err = NULL; if (vfio_listener_skipped_section(section)) { return; } if (vfio_devices_all_dirty_tracking(bcontainer)) { - ret = vfio_sync_dirty_bitmap(bcontainer, section); + ret = vfio_sync_dirty_bitmap(bcontainer, section, &local_err); if (ret) { - error_report("vfio: Failed to sync dirty bitmap, err: %d (%s)", ret, - strerror(-ret)); + error_report_err(local_err); vfio_set_migration_error(ret); } } diff --git a/hw/vfio/container-base.c b/hw/vfio/container-base.c index 7c0764121d24b02b6c4e66e368d7dff78a6d65aa..4fa250c46fd16333d2e2358ede8b0a0afdb185b3 100644 --- a/hw/vfio/container-base.c +++ b/hw/vfio/container-base.c @@ -64,11 +64,12 @@ int vfio_container_set_dirty_page_tracking(VFIOContainerBase *bcontainer, } int vfio_container_query_dirty_bitmap(const VFIOContainerBase *bcontainer, - VFIOBitmap *vbmap, - hwaddr iova, hwaddr size) + VFIOBitmap *vbmap, hwaddr iova, + hwaddr size, Error **errp) { g_assert(bcontainer->ops->query_dirty_bitmap); - return bcontainer->ops->query_dirty_bitmap(bcontainer, vbmap, iova, size); + return bcontainer->ops->query_dirty_bitmap(bcontainer, vbmap, iova, size, + errp); } void vfio_container_init(VFIOContainerBase *bcontainer, VFIOAddressSpace *space, diff --git a/hw/vfio/container.c b/hw/vfio/container.c index c35221fbe7dc5453050f97cd186fc958e24f28f7..e00db6546c652c61a352f8e4cd65b212ecfb65bc 100644 --- a/hw/vfio/container.c +++ b/hw/vfio/container.c @@ -130,6 +130,7 @@ static int vfio_legacy_dma_unmap(const VFIOContainerBase *bcontainer, }; bool need_dirty_sync = false; int ret; + Error *local_err = NULL; if (iotlb && vfio_devices_all_running_and_mig_active(bcontainer)) { if (!vfio_devices_all_device_dirty_tracking(bcontainer) && @@ -165,8 +166,9 @@ static int vfio_legacy_dma_unmap(const VFIOContainerBase *bcontainer, if (need_dirty_sync) { ret = vfio_get_dirty_bitmap(bcontainer, iova, size, - iotlb->translated_addr); + iotlb->translated_addr, &local_err); if (ret) { + error_report_err(local_err); return ret; } } @@ -236,7 +238,8 @@ vfio_legacy_set_dirty_page_tracking(const VFIOContainerBase *bcontainer, static int vfio_legacy_query_dirty_bitmap(const VFIOContainerBase *bcontainer, VFIOBitmap *vbmap, - hwaddr iova, hwaddr size) + hwaddr iova, hwaddr size, + Error **errp) { const VFIOContainer *container = container_of(bcontainer, VFIOContainer, bcontainer); @@ -264,9 +267,10 @@ static int vfio_legacy_query_dirty_bitmap(const VFIOContainerBase *bcontainer, ret = ioctl(container->fd, VFIO_IOMMU_DIRTY_PAGES, dbitmap); if (ret) { ret = -errno; - error_report("Failed to get dirty bitmap for iova: 0x%"PRIx64 - " size: 0x%"PRIx64" err: %d", (uint64_t)range->iova, - (uint64_t)range->size, errno); + error_setg_errno(errp, errno, + "Failed to get dirty bitmap for iova: 0x%"PRIx64 + " size: 0x%"PRIx64, (uint64_t)range->iova, + (uint64_t)range->size); } g_free(dbitmap);
Let the callers do the error reporting. Add documentation while at it. Signed-off-by: Cédric Le Goater <clg@redhat.com> --- Changes in v6: - Fixed the line wrapping (Avihai) - Fixed query_dirty_bitmap documentation (Avihai) Changes in v5: - Replaced error_setg() by error_setg_errno() in vfio_devices_query_dirty_bitmap() and vfio_legacy_query_dirty_bitmap() - ':' -> '-' in vfio_iommu_map_dirty_notify() include/hw/vfio/vfio-common.h | 4 +- include/hw/vfio/vfio-container-base.h | 21 +++++++-- hw/vfio/common.c | 61 ++++++++++++++++++--------- hw/vfio/container-base.c | 7 +-- hw/vfio/container.c | 14 +++--- 5 files changed, 72 insertions(+), 35 deletions(-)