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

[Cédric Le Goater]

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.