Re: [PATCH v6 0/9] hw/iommufd: IOMMUFD Dirty Tracking

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

On 7/23/24 10:56, Joao Martins wrote:
> On 23/07/2024 09:35, Cédric Le Goater wrote:
>> On 7/22/24 23:13, 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 (and these SMMUv3 in upcoming v6.11). The series is divided
>>> organized as follows:
>>>
>>> * Patches 1 - 7: IOMMUFD backend support for dirty tracking;
>>>
>>> Introduce auto domains -- Patch 3 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 8 - 9: Don't block live migration where there's no VF dirty
>>> tracker, considering that we have IOMMU dirty tracking.
>>>
>>> Comments and feedback appreciated (on patches 1, 5, 8, 9)
>>>
>>> Cheers,
>>>       Joao
>>>
>>> P.S. Suggest v6.11-rc as hypervisor kernel as there's
>>> some bugs fixed there with regards to IOMMU hugepage dirty tracking.
>>>
>>> Changes since v5[6]:
>>> * Remove patches 1-4 as these were commited to vfio-next
>>> * Add the Rb by Cedric and Zhenzhong (previously patches 7, 8, 10, 11)
>>> * Introduce VFIODevice::iommu_dirty_tracking and use it on patch 5, 8
>>> to store whether we can use IOMMU dirty tracking.
>>>
>>> Changes since v4[5]:
>>> * 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 various commit messages (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)
>>> * Add missing commens to struct doc from aw_bits removal (and hw_caps
>>> addition) (Eric)
>>> * Fix the detach flow in auto domains (Eric)
>>> * 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/20240719120501.81279-1-joao.m.martins@oracle.com/
>>>
>>> Joao Martins (9):
>>>     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      |  13 +++
>>>    include/sysemu/host_iommu_device.h |   5 +-
>>>    include/sysemu/iommufd.h           |  11 ++
>>>    backends/iommufd.c                 |  85 ++++++++++++++-
>>>    hw/vfio/common.c                   |  19 ++--
>>>    hw/vfio/container.c                |   9 +-
>>>    hw/vfio/helpers.c                  |  11 ++
>>>    hw/vfio/iommufd.c                  | 170 ++++++++++++++++++++++++++++-
>>>    hw/vfio/migration.c                |  12 +-
>>>    hw/vfio/pci.c                      |   3 +
>>>    backends/trace-events              |   3 +
>>>    11 files changed, 318 insertions(+), 23 deletions(-)
>>
>> Applied to vfio-next with the changes that were discussed this morning.
>> Please check.
>>
> 
> I think the only thing missing is in the fourth patch to add the comment Eric
> suggested (see below). Other than that, looks good to me.
> 
> diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
> index 50ffa4b77090..abb6f1a4b4a8 100644
> --- a/hw/vfio/iommufd.c
> +++ b/hw/vfio/iommufd.c
> @@ -488,6 +488,13 @@ static bool iommufd_cdev_attach(const char *name,
> VFIODevice *vbasedev,
> 
>       space = vfio_get_address_space(as);
> 
> +    /*
> +     * The HostIOMMUDevice data from legacy backend is static and doesn't need
> +     * any information from the (type1-iommu) backend to be initialized. In
> +     * contrast however, the IOMMUFD HostIOMMUDevice data requires the iommufd
> +     * FD to be connected and having a devid to be able to successfully call
> +     * iommufd_backend_get_device_info().
> +     */
>       if (!vfio_device_hiod_realize(vbasedev, errp)) {
>           goto err_alloc_ioas;
>       }
> 

Yep. This is fixed now. I will send a vfio PR in a couple of hours.

Thanks,

C.