Re: [PATCH 1/9] vfio/container: Add dirty tracking started flag

[Avihai Horon]

On 16/12/2024 14:15, Joao Martins wrote:
> External email: Use caution opening links or attachments
>
>
> On 16/12/2024 09:46, Avihai Horon wrote:
> > Add a flag to VFIOContainerBase that indicates whether dirty tracking
> > has been started for the container or not.
> >
> > This will be used in the following patches to allow dirty page syncs
> > only if dirty tracking has been started.
> >
> > Signed-off-by: Avihai Horon <avihaih@nvidia.com>
> Reviewed-by: Joao Martins <joao.m.martins@oracle.com>
>
> Just a nit below, but it's optional.
>
> > ---
> >   include/hw/vfio/vfio-container-base.h | 1 +
> >   hw/vfio/container-base.c              | 8 +++++++-
> >   2 files changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/hw/vfio/vfio-container-base.h 
> > b/include/hw/vfio/vfio-container-base.h
> > index 62a8b60d87..4cff9943ab 100644
> > --- a/include/hw/vfio/vfio-container-base.h
> > +++ b/include/hw/vfio/vfio-container-base.h
> > @@ -44,6 +44,7 @@ typedef struct VFIOContainerBase {
> >       unsigned long pgsizes;
> >       unsigned int dma_max_mappings;
> >       bool dirty_pages_supported;
> > +    bool dirty_pages_started; /* Protected by BQL */
> >       QLIST_HEAD(, VFIOGuestIOMMU) giommu_list;
> >       QLIST_HEAD(, VFIORamDiscardListener) vrdl_list;
> >       QLIST_ENTRY(VFIOContainerBase) next;
> > diff --git a/hw/vfio/container-base.c b/hw/vfio/container-base.c
> > index 6f86c37d97..48fc75cd62 100644
> > --- a/hw/vfio/container-base.c
> > +++ b/hw/vfio/container-base.c
> > @@ -64,13 +64,19 @@ int 
> > vfio_container_set_dirty_page_tracking(VFIOContainerBase *bcontainer,
> >                                              bool start, Error **errp)
> >   {
> >       VFIOIOMMUClass *vioc = VFIO_IOMMU_GET_CLASS(bcontainer);
> > +    int ret;
> >
> >       if (!bcontainer->dirty_pages_supported) {
> >           return 0;
> >       }
> It's a nop when no state is changed; don't know if you wanna capture that here.
> Something like this below now that you track container dirty tracking status:
>
> if (!(bcontainer->dirty_pages_started == start)) {
>          return 0;
> }

I guess you mean:

if (bcontainer->dirty_pages_started == start) {
    return 0;
}

Yes, I can add it.

Thanks!

> >       g_assert(vioc->set_dirty_page_tracking);
> > -    return vioc->set_dirty_page_tracking(bcontainer, start, errp);
> > +    ret = vioc->set_dirty_page_tracking(bcontainer, start, errp);
> > +    if (!ret) {
> > +        bcontainer->dirty_pages_started = start;
> > +    }
> > +
> > +    return ret;
> >   }
>
> >   int vfio_container_query_dirty_bitmap(const VFIOContainerBase *bcontainer,