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

[Joao Martins]

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;
}

>      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,