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

["Cédric Le Goater" <clg@redhat.com>]

On 3/8/24 08:39, Eric Auger wrote:
> 
> 
> 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!

Took me a while do understand where it was come from. So you educated
me also :)

>>
>>> +     * @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

ok. Let's get the initial migration part in first and then I will resend the
VFIO part.

Thanks for your time.

C.