I had a look at this before I realized it's already in. I'm sending
this out not to demand any change, but only to point out an issue to be
avoided in future work.
Cédric Le Goater <c...@redhat.com> writes:
> 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>
> Reviewed-by: Avihai Horon <avih...@nvidia.com>
> Reviewed-by: Eric Auger <eric.au...@redhat.com>
> Signed-off-by: Cédric Le Goater <c...@redhat.com>
> ---
> 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..326ceea52a2030eec9dad289a9845866c4a8c090
> 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
> + * page tracking
> + * @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);
Note for later: this is always called via
vfio_container_set_dirty_page_tracking().
> 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
> 8f9cbdc0264044ce587877a7d19d14b28527291b..485e53916491f1164d29e739fb7106c0c77df737
> 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -1076,7 +1076,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);
> }
>
> if (ret) {
> @@ -1096,7 +1096,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) {
Note for later: all callers pass NULL to ignore the new Error.
> 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
> 77bdec276ec49cb9cd767c0de42ec801b4421572..c35221fbe7dc5453050f97cd186fc958e24f28f7
> 100644
> --- a/hw/vfio/container.c
> +++ b/hw/vfio/container.c
> @@ -209,7 +209,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);
> @@ -227,8 +227,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);
Silent improvement: errno is now reported symbolically (like "Operation
not permitted") instead of numerically (like "1"). Worth mentioning in
the commit message.
Since the callers pass NULL to ignore the Error, this error condition is
now silent, I believe.
I figure you correct this in later patches. If we accept temporary loss
of error reporting, the commit message should mention it. Loss of error
reporting is easy enough to avoid, though: have the callers pass a
pointer to a local @err, and on failure report it with
error_report_err().
> }
>
> return ret;
