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

[Eric Auger]

On 3/7/24 13:06, Cédric Le Goater wrote:
> On 3/7/24 09:09, Eric Auger wrote:
>> 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?
> 
> yep
> 
>> for my education is the "#"VFIOContainerBase formalized somewhere?
> 
> It's QEMU specific. See 4cf41794411f ("docs: tweak kernel-doc for QEMU
> coding standards").
OK thank you for the education!
> 
>> +     * @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)
> 
> When I started this series, I was trying to find a way to introduce
> progressively the changes and this patch is preparing ground for
> what is coming next. It could be merged with the following if you prefer.
up to you or tweek the commit msg

Eric
> 
> 
> Thanks,
> 
> C.
> 
> 
> 
> 
>>>       }
>>>         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
>>
>