Hi Cédric, On 3/6/24 14:34, Cédric Le Goater wrote: > We will use the Error object to improve error reporting in the > .log_global*() handlers of VFIO. Add documentation while at it. > > Reviewed-by: Philippe Mathieu-Daudé <phi...@linaro.org> > Signed-off-by: Cédric Le Goater <c...@redhat.com> > --- > > Changes in v3: > > - Use error_setg_errno() in vfio_legacy_set_dirty_page_tracking() > > include/hw/vfio/vfio-container-base.h | 18 ++++++++++++++++-- > hw/vfio/common.c | 4 ++-- > hw/vfio/container-base.c | 4 ++-- > hw/vfio/container.c | 6 +++--- > 4 files changed, 23 insertions(+), 9 deletions(-) > > diff --git a/include/hw/vfio/vfio-container-base.h > b/include/hw/vfio/vfio-container-base.h > index > 3582d5f97a37877b2adfc0d0b06996c82403f8b7..c76984654a596e3016a8cf833e10143eb872e102 > 100644 > --- a/include/hw/vfio/vfio-container-base.h > +++ b/include/hw/vfio/vfio-container-base.h > @@ -82,7 +82,7 @@ int vfio_container_add_section_window(VFIOContainerBase > *bcontainer, > void vfio_container_del_section_window(VFIOContainerBase *bcontainer, > MemoryRegionSection *section); > int vfio_container_set_dirty_page_tracking(VFIOContainerBase *bcontainer, > - bool start); > + bool start, Error **errp); > int vfio_container_query_dirty_bitmap(const VFIOContainerBase *bcontainer, > VFIOBitmap *vbmap, > hwaddr iova, hwaddr size); > @@ -121,9 +121,23 @@ struct VFIOIOMMUClass { > int (*attach_device)(const char *name, VFIODevice *vbasedev, > AddressSpace *as, Error **errp); > void (*detach_device)(VFIODevice *vbasedev); > + > /* migration feature */ > + > + /** > + * @set_dirty_page_tracking > + * > + * Start or stop dirty pages tracking on VFIO container > + * > + * @bcontainer: #VFIOContainerBase on which to de/activate dirty > + * pages tracking s/pages/page? for my education is the "#"VFIOContainerBase formalized somewhere? > + * @start: indicates whether to start or stop dirty pages tracking > + * @errp: pointer to Error*, to store an error if it happens. > + * > + * Returns zero to indicate success and negative for error > + */ > int (*set_dirty_page_tracking)(const VFIOContainerBase *bcontainer, > - bool start); > + bool start, Error **errp); > int (*query_dirty_bitmap)(const VFIOContainerBase *bcontainer, > VFIOBitmap *vbmap, > hwaddr iova, hwaddr size); > diff --git a/hw/vfio/common.c b/hw/vfio/common.c > index > 800ba0aeac84b8dcc83b042bb70c37b4bf78d3f4..5598a508399a6c0b3a20ba17311cbe83d84250c5 > 100644 > --- a/hw/vfio/common.c > +++ b/hw/vfio/common.c > @@ -1085,7 +1085,7 @@ static bool > vfio_listener_log_global_start(MemoryListener *listener, > if (vfio_devices_all_device_dirty_tracking(bcontainer)) { > ret = vfio_devices_dma_logging_start(bcontainer); > } else { > - ret = vfio_container_set_dirty_page_tracking(bcontainer, true); > + ret = vfio_container_set_dirty_page_tracking(bcontainer, true, NULL); It is not obvious why we don't pass errp here. Also there is ana error_report below. Why isn't the error propagated? (not related to your patch though) > } > > if (ret) { > @@ -1105,7 +1105,7 @@ static void > vfio_listener_log_global_stop(MemoryListener *listener) > if (vfio_devices_all_device_dirty_tracking(bcontainer)) { > vfio_devices_dma_logging_stop(bcontainer); > } else { > - ret = vfio_container_set_dirty_page_tracking(bcontainer, false); > + ret = vfio_container_set_dirty_page_tracking(bcontainer, false, > NULL); > } > > if (ret) { > diff --git a/hw/vfio/container-base.c b/hw/vfio/container-base.c > index > 913ae49077c4f09b7b27517c1231cfbe4befb7fb..7c0764121d24b02b6c4e66e368d7dff78a6d65aa > 100644 > --- a/hw/vfio/container-base.c > +++ b/hw/vfio/container-base.c > @@ -53,14 +53,14 @@ void vfio_container_del_section_window(VFIOContainerBase > *bcontainer, > } > > int vfio_container_set_dirty_page_tracking(VFIOContainerBase *bcontainer, > - bool start) > + bool start, Error **errp) > { > if (!bcontainer->dirty_pages_supported) { > return 0; > } > > g_assert(bcontainer->ops->set_dirty_page_tracking); > - return bcontainer->ops->set_dirty_page_tracking(bcontainer, start); > + return bcontainer->ops->set_dirty_page_tracking(bcontainer, start, errp); > } > > int vfio_container_query_dirty_bitmap(const VFIOContainerBase *bcontainer, > diff --git a/hw/vfio/container.c b/hw/vfio/container.c > index > 096d77eac3946a9c38fc2a98116b93353f71f06e..6524575aeddcea8470b5fd10caf57475088d1813 > 100644 > --- a/hw/vfio/container.c > +++ b/hw/vfio/container.c > @@ -210,7 +210,7 @@ static int vfio_legacy_dma_map(const VFIOContainerBase > *bcontainer, hwaddr iova, > > static int > vfio_legacy_set_dirty_page_tracking(const VFIOContainerBase *bcontainer, > - bool start) > + bool start, Error **errp) > { > const VFIOContainer *container = container_of(bcontainer, VFIOContainer, > bcontainer); > @@ -228,8 +228,8 @@ vfio_legacy_set_dirty_page_tracking(const > VFIOContainerBase *bcontainer, > ret = ioctl(container->fd, VFIO_IOMMU_DIRTY_PAGES, &dirty); > if (ret) { > ret = -errno; > - error_report("Failed to set dirty tracking flag 0x%x errno: %d", > - dirty.flags, errno); > + error_setg_errno(errp, errno, "Failed to set dirty tracking flag > 0x%x", > + dirty.flags); > } > > return ret;
Thanks Eric