Re: [PATCH v5 00/13] hw/iommufd: IOMMUFD Dirty Tracking

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

On 7/19/24 14:04, Joao Martins wrote:
> This small series adds support for IOMMU dirty tracking support via the
> IOMMUFD backend. The hardware capability is available on most recent x86
> hardware. The series is divided organized as follows:
> 
> * Patch 1-2: Fixes a regression into mdev support with IOMMUFD. This
>               one is independent of the series but happened to cross it
>               while testing mdev with this series
> 
> * Patch 3: Adds a support to iommufd_get_device_info() for capabilities
> 
> * Patches 4 - 11: IOMMUFD backend support for dirty tracking;
> 
> Introduce auto domains -- Patch 5 goes into more detail, but the gist is that
> we will find and attach a device to a compatible IOMMU domain, or allocate a new
> hardware pagetable *or* rely on kernel IOAS attach (for mdevs). Afterwards the
> workflow is relatively simple:
> 
> 1) Probe device and allow dirty tracking in the HWPT
> 2) Toggling dirty tracking on/off
> 3) Read-and-clear of Dirty IOVAs
> 
> The heuristics selected for (1) were to always request the HWPT for
> dirty tracking if supported, or rely on device dirty page tracking. This
> is a little simplistic and we aren't necessarily utilizing IOMMU dirty
> tracking even if we ask during hwpt allocation.
> 
> The unmap case is deferred until further vIOMMU support with migration
> is added[3] which will then introduce the usage of
> IOMMU_HWPT_GET_DIRTY_BITMAP_NO_CLEAR in GET_DIRTY_BITMAP ioctl in the
> dma unmap bitmap flow.
> 
> * Patches 12-13: Don't block live migration where there's no VF dirty
> tracker, considering that we have IOMMU dirty tracking.
> 
> Comments and feedback appreciated. Thanks for the review so far and
> apologies in advance if I missed any comment.
> 
> Cheers,
>      Joao
> 
> P.S. Suggest linux-next (or future v6.11) as hypervisor kernel as there's
> some bugs fixed there with regards to IOMMU hugepage dirty tracking.
> 
> Changes since v4[6]:
> * Add various Reviewed-by in patches 2,3,4,6,8,11
> * Change error messages to mention IOMMU (Zhenzhong)
> * Better improve the checking of dirty page tracking in
>    vfio_migration_realize() to detect per-device IOMMU instead of using
>    container dirty_page_supported().
> * Improve/Cleanup various commit messages to be clear (Eric)
> * Extract the caps::hw_caps into its own patch as it was miosleading to
> be hidden in another patch (new patch 7)
> * Restructure patch 1 helper to be vfio_device_is_mdev() and use
> vfio::mdev directly in rest of patches (Cedric)
> * Improve error messages of set,query dirty tracking (Cedric)
> * Add missing casts to uintptr and uint64_t* (Cedric)
> * Improve terciary check in set_dirty_Tracking (Cedric)
> * Add missing commens to struct doc from aw_bits removal (and hw_caps
> addition) (Eric)
> * Fix the detach flow in auto domains (Eric)
> * Add new helper vfio_device_hiod_realize() and use it in backends
> * (Cedric)
> * Move introduction of iommufd_hwpt_dirty_tracking() in the predecessor
> * patch (Cedric)
> * Set hwpt to NULL on detach (Eric)
> * Spurious line (Eric)
> 
> Changes since v3[5]:
> * Skip HostIOMMUDevice::realize for mdev, and introduce a helper to check if the VFIO
>    device is mdev. (Zhenzhong)
> * Skip setting IOMMU device for mdev (Zhenzhong)
> * Add Zhenzhong review tag in patch 3
> * Utilize vbasedev::bcontainer::dirty_pages_supported instead of introducing
>    a new HostIOMMUDevice capability and thus remove the cap patch from the series (Zhenzhong)
> * Move the HostIOMMUDevice::realize() to be part of VFIODevice initialization in attach_device()
> while skipping it all together for mdev. (Cedric)
> * Due to the previous item, had to remove aw_bits because it depends on device attach being
> finished, instead defer it to when get_cap() gets called.
> * Skip auto domains for mdev instead of purposedly erroring out (Zhenzhong)
> * Pass errp in all cases, and instead just free the error in case of -EINVAL
>    in most of all patches, and also pass Error* in iommufd_backend_alloc_hwpt() amd
>    set/query dirty. This is made better thanks in part to skipping auto domains for mdev (Cedric)
> 
> Changes since RFCv2[4]:
> * Always allocate hwpt with IOMMU_HWPT_ALLOC_DIRTY_TRACKING even if
> we end up not actually toggling dirty tracking. (Avihai)
> * Fix error handling widely in auto domains logic and all patches (Avihai)
> * Reuse iommufd_backend_get_device_info() for capabilities (Zhenzhong)
> * New patches 1 and 2 taking into consideration previous comments.
> * Store hwpt::flags to know if we have dirty tracking (Avihai)
> * New patch 8, that allows to query dirty tracking support after
> provisioning. This is a cleaner way to check IOMMU dirty tracking support
> when vfio::migration is iniitalized, as opposed to RFCv2 via device caps.
> device caps way is still used because at vfio attach we aren't yet with
> a fully initialized migration state.
> * Adopt error propagation in query,set dirty tracking
> * Misc improvements overall broadly and Avihai
> * Drop hugepages as it's a bit unrelated; I can pursue that patch
> * separately. The main motivation is to provide a way to test
> without hugepages similar to what vfio_type1_iommu.disable_hugepages=1
> does.
> 
> Changes since RFCv1[2]:
> * Remove intel/amd dirty tracking emulation enabling
> * Remove the dirtyrate improvement for VF/IOMMU dirty tracking
> [Will pursue these two in separate series]
> * Introduce auto domains support
> * Enforce dirty tracking following the IOMMUFD UAPI for this
> * Add support for toggling hugepages in IOMMUFD
> * Auto enable support when VF supports migration to use IOMMU
> when it doesn't have VF dirty tracking
> * Add a parameter to toggle VF dirty tracking
> 
> [0] https://lore.kernel.org/qemu-devel/20240201072818.327930-1-zhenzhong.duan@intel.com/
> [1] https://lore.kernel.org/qemu-devel/20240201072818.327930-10-zhenzhong.duan@intel.com/
> [2] https://lore.kernel.org/qemu-devel/20220428211351.3897-1-joao.m.martins@oracle.com/
> [3] https://lore.kernel.org/qemu-devel/20230622214845.3980-1-joao.m.martins@oracle.com/
> [4] https://lore.kernel.org/qemu-devel/20240212135643.5858-1-joao.m.martins@oracle.com/
> [5] https://lore.kernel.org/qemu-devel/20240708143420.16953-1-joao.m.martins@oracle.com/
> [6] https://lore.kernel.org/qemu-devel/20240712114704.8708-1-joao.m.martins@oracle.com/#t
> 
> Joao Martins (13):
>    vfio/pci: Extract mdev check into an helper
>    vfio/iommufd: Don't initialize nor set a HOST_IOMMU_DEVICE with mdev
>    backends/iommufd: Extend iommufd_backend_get_device_info() to fetch HW
>      capabilities
>    vfio/iommufd: Return errno in iommufd_cdev_attach_ioas_hwpt()
>    vfio/iommufd: Introduce auto domain creation
>    vfio/{iommufd,container}: Remove caps::aw_bits
>    vfio/iommufd: Add hw_caps field to HostIOMMUDeviceCaps
>    vfio/{iommufd,container}: Invoke HostIOMMUDevice::realize() during
>      attach_device()
>    vfio/iommufd: Probe and request hwpt dirty tracking capability
>    vfio/iommufd: Implement VFIOIOMMUClass::set_dirty_tracking support
>    vfio/iommufd: Implement VFIOIOMMUClass::query_dirty_bitmap support
>    vfio/migration: Don't block migration device dirty tracking is
>      unsupported
>    vfio/common: Allow disabling device dirty page tracking
> 
>   include/hw/vfio/vfio-common.h      |  15 +++
>   include/sysemu/host_iommu_device.h |   5 +-
>   include/sysemu/iommufd.h           |  13 ++-
>   backends/iommufd.c                 |  89 +++++++++++++-
>   hw/vfio/common.c                   |  17 +--
>   hw/vfio/container.c                |   9 +-
>   hw/vfio/helpers.c                  |  25 ++++
>   hw/vfio/iommufd.c                  | 181 ++++++++++++++++++++++++++++-
>   hw/vfio/migration.c                |  12 +-
>   hw/vfio/pci.c                      |  26 +++--
>   backends/trace-events              |   3 +
>   11 files changed, 356 insertions(+), 39 deletions(-)
  
Applied 1-4 to vfio-next.

Still looking at the rest. We have ~24h for the last reviews.

Thanks,

C.