Re: [PATCH v4 16/25] vfio: Add Error** argument to .set_dirty_page_tracking() handler

[Eric Auger]

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é <philmd@linaro.org>
> Signed-off-by: Cédric Le Goater <clg@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