>-----Original Message-----
>From: Cédric Le Goater <c...@redhat.com>
>Subject: Re: [PATCH v5 05/13] vfio/iommufd: Introduce auto domain
>creation
>
>On 7/22/24 10:50, Joao Martins wrote:
>> On 22/07/2024 06:16, Duan, Zhenzhong wrote:
>>>> -----Original Message-----
>>>> From: Joao Martins <joao.m.mart...@oracle.com>
>>>> Subject: [PATCH v5 05/13] vfio/iommufd: Introduce auto domain
>creation
>>>>
>>>> There's generally two modes of operation for IOMMUFD:
>>>>
>>>> 1) The simple user API which intends to perform relatively simple things
>>>> with IOMMUs e.g. DPDK. The process generally creates an IOAS and
>attaches
>>>> to VFIO and mainly performs IOAS_MAP and UNMAP.
>>>>
>>>> 2) The native IOMMUFD API where you have fine grained control of the
>>>> IOMMU domain and model it accordingly. This is where most new
>feature
>>>> are being steered to.
>>>>
>>>> For dirty tracking 2) is required, as it needs to ensure that
>>>> the stage-2/parent IOMMU domain will only attach devices
>>>> that support dirty tracking (so far it is all homogeneous in x86, likely
>>>> not the case for smmuv3). Such invariant on dirty tracking provides a
>>>> useful guarantee to VMMs that will refuse incompatible device
>>>> attachments for IOMMU domains.
>>>>
>>>> Dirty tracking insurance is enforced via HWPT_ALLOC, which is
>>>> responsible for creating an IOMMU domain. This is contrast to the
>>>> 'simple API' where the IOMMU domain is created by IOMMUFD
>>>> automatically
>>>> when it attaches to VFIO (usually referred as autodomains) but it has
>>>> the needed handling for mdevs.
>>>>
>>>> To support dirty tracking with the advanced IOMMUFD API, it needs
>>>> similar logic, where IOMMU domains are created and devices attached
>to
>>>> compatible domains. Essentially mimicking kernel
>>>> iommufd_device_auto_get_domain(). With mdevs given there's no
>IOMMU
>>>> domain
>>>> it falls back to IOAS attach.
>>>>
>>>> The auto domain logic allows different IOMMU domains to be created
>when
>>>> DMA dirty tracking is not desired (and VF can provide it), and others
>where
>>>> it is. Here it is not used in this way given how VFIODevice migration
>>>> state is initialized after the device attachment. But such mixed mode of
>>>> IOMMU dirty tracking + device dirty tracking is an improvement that
>can
>>>> be added on. Keep the 'all of nothing' of type1 approach that we have
>>>> been using so far between container vs device dirty tracking.
>>>>
>>>> Signed-off-by: Joao Martins <joao.m.mart...@oracle.com>
>>>> ---
>>>> include/hw/vfio/vfio-common.h | 9 ++++
>>>> include/sysemu/iommufd.h | 5 +++
>>>> backends/iommufd.c | 30 +++++++++++++
>>>> hw/vfio/iommufd.c | 84
>>>> +++++++++++++++++++++++++++++++++++
>>>> backends/trace-events | 1 +
>>>> 5 files changed, 129 insertions(+)
>>>>
>>>> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-
>>>> common.h
>>>> index 98acae8c1c97..1a96678f8c38 100644
>>>> --- a/include/hw/vfio/vfio-common.h
>>>> +++ b/include/hw/vfio/vfio-common.h
>>>> @@ -95,10 +95,17 @@ typedef struct VFIOHostDMAWindow {
>>>>
>>>> typedef struct IOMMUFDBackend IOMMUFDBackend;
>>>>
>>>> +typedef struct VFIOIOASHwpt {
>>>> + uint32_t hwpt_id;
>>>> + QLIST_HEAD(, VFIODevice) device_list;
>>>> + QLIST_ENTRY(VFIOIOASHwpt) next;
>>>> +} VFIOIOASHwpt;
>>>> +
>>>> typedef struct VFIOIOMMUFDContainer {
>>>> VFIOContainerBase bcontainer;
>>>> IOMMUFDBackend *be;
>>>> uint32_t ioas_id;
>>>> + QLIST_HEAD(, VFIOIOASHwpt) hwpt_list;
>>>> } VFIOIOMMUFDContainer;
>>>>
>>>> OBJECT_DECLARE_SIMPLE_TYPE(VFIOIOMMUFDContainer,
>>>> VFIO_IOMMU_IOMMUFD);
>>>> @@ -135,6 +142,8 @@ typedef struct VFIODevice {
>>>> HostIOMMUDevice *hiod;
>>>> int devid;
>>>> IOMMUFDBackend *iommufd;
>>>> + VFIOIOASHwpt *hwpt;
>>>> + QLIST_ENTRY(VFIODevice) hwpt_next;
>>>> } VFIODevice;
>>>>
>>>> struct VFIODeviceOps {
>>>> diff --git a/include/sysemu/iommufd.h b/include/sysemu/iommufd.h
>>>> index 57d502a1c79a..e917e7591d05 100644
>>>> --- a/include/sysemu/iommufd.h
>>>> +++ b/include/sysemu/iommufd.h
>>>> @@ -50,6 +50,11 @@ int
>>>> iommufd_backend_unmap_dma(IOMMUFDBackend *be, uint32_t
>ioas_id,
>>>> bool iommufd_backend_get_device_info(IOMMUFDBackend *be,
>uint32_t
>>>> devid,
>>>> uint32_t *type, void *data, uint32_t
>>>> len,
>>>> uint64_t *caps, Error **errp);
>>>> +bool iommufd_backend_alloc_hwpt(IOMMUFDBackend *be, uint32_t
>>>> dev_id,
>>>> + uint32_t pt_id, uint32_t flags,
>>>> + uint32_t data_type, uint32_t data_len,
>>>> + void *data_ptr, uint32_t *out_hwpt,
>>>> + Error **errp);
>>>>
>>>> #define TYPE_HOST_IOMMU_DEVICE_IOMMUFD
>>>> TYPE_HOST_IOMMU_DEVICE "-iommufd"
>>>> #endif
>>>> diff --git a/backends/iommufd.c b/backends/iommufd.c
>>>> index 2b3d51af26d2..a94d3b90c05c 100644
>>>> --- a/backends/iommufd.c
>>>> +++ b/backends/iommufd.c
>>>> @@ -208,6 +208,36 @@ int
>>>> iommufd_backend_unmap_dma(IOMMUFDBackend *be, uint32_t
>ioas_id,
>>>> return ret;
>>>> }
>>>>
>>>> +bool iommufd_backend_alloc_hwpt(IOMMUFDBackend *be, uint32_t
>>>> dev_id,
>>>> + uint32_t pt_id, uint32_t flags,
>>>> + uint32_t data_type, uint32_t data_len,
>>>> + void *data_ptr, uint32_t *out_hwpt,
>>>> + Error **errp)
>>>> +{
>>>> + int ret, fd = be->fd;
>>>> + struct iommu_hwpt_alloc alloc_hwpt = {
>>>> + .size = sizeof(struct iommu_hwpt_alloc),
>>>> + .flags = flags,
>>>> + .dev_id = dev_id,
>>>> + .pt_id = pt_id,
>>>> + .data_type = data_type,
>>>> + .data_len = data_len,
>>>> + .data_uptr = (uintptr_t)data_ptr,
>>>> + };
>>>> +
>>>> + ret = ioctl(fd, IOMMU_HWPT_ALLOC, &alloc_hwpt);
>>>> + trace_iommufd_backend_alloc_hwpt(fd, dev_id, pt_id, flags,
>data_type,
>>>> + data_len, (uintptr_t)data_ptr,
>>>> + alloc_hwpt.out_hwpt_id, ret);
>>>> + if (ret) {
>>>> + error_setg_errno(errp, errno, "Failed to allocate hwpt");
>>>> + return false;
>>>> + }
>>>> +
>>>> + *out_hwpt = alloc_hwpt.out_hwpt_id;
>>>> + return true;
>>>> +}
>>>> +
>>>> bool iommufd_backend_get_device_info(IOMMUFDBackend *be,
>uint32_t
>>>> devid,
>>>> uint32_t *type, void *data, uint32_t
>>>> len,
>>>> uint64_t *caps, Error **errp)
>>>> diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
>>>> index 077dea8f1b64..545f4a404125 100644
>>>> --- a/hw/vfio/iommufd.c
>>>> +++ b/hw/vfio/iommufd.c
>>>> @@ -212,10 +212,88 @@ static bool
>>>> iommufd_cdev_detach_ioas_hwpt(VFIODevice *vbasedev, Error **errp)
>>>> return true;
>>>> }
>>>>
>>>> +static bool iommufd_cdev_autodomains_get(VFIODevice *vbasedev,
>>>> + VFIOIOMMUFDContainer *container,
>>>> + Error **errp)
>>>> +{
>>>> + IOMMUFDBackend *iommufd = vbasedev->iommufd;
>>>> + uint32_t flags = 0;
>>>> + VFIOIOASHwpt *hwpt;
>>>> + uint32_t hwpt_id;
>>>> + int ret;
>>>> +
>>>> + /* Try to find a domain */
>>>> + QLIST_FOREACH(hwpt, &container->hwpt_list, next) {
>>>> + ret = iommufd_cdev_attach_ioas_hwpt(vbasedev, hwpt->hwpt_id,
>>>> errp);
>>>> + if (ret) {
>>>> + /* -EINVAL means the domain is incompatible with the device.
>*/
>>>> + if (ret == -EINVAL) {
>>>> + /*
>>>> + * It is an expected failure and it just means we will try
>>>> + * another domain, or create one if no existing compatible
>>>> + * domain is found. Hence why the error is discarded
>>>> below.
>>>> + */
>>>> + error_free(*errp);
>>>> + *errp = NULL;
>>>> + continue;
>>>> + }
>>>> +
>>>> + return false;
>>>> + } else {
>>>> + vbasedev->hwpt = hwpt;
>>>> + QLIST_INSERT_HEAD(&hwpt->device_list, vbasedev, hwpt_next);
>>>> + return true;
>>>> + }
>>>> + }
>>>> +
>>>> + if (!iommufd_backend_alloc_hwpt(iommufd, vbasedev->devid,
>>>> + container->ioas_id, flags,
>>>> + IOMMU_HWPT_DATA_NONE, 0, NULL,
>>>> + &hwpt_id, errp)) {
>>>> + return false;
>>>> + }
>>>> +
>>>> + hwpt = g_malloc0(sizeof(*hwpt));
>>>> + hwpt->hwpt_id = hwpt_id;
>>>> + QLIST_INIT(&hwpt->device_list);
>>>> +
>>>> + ret = iommufd_cdev_attach_ioas_hwpt(vbasedev, hwpt->hwpt_id,
>errp);
>>>> + if (ret) {
>>>> + iommufd_backend_free_id(container->be, hwpt->hwpt_id);
>>>> + g_free(hwpt);
>>>> + return false;
>>>> + }
>>>> +
>>>> + vbasedev->hwpt = hwpt;
>>>> + QLIST_INSERT_HEAD(&hwpt->device_list, vbasedev, hwpt_next);
>>>> + QLIST_INSERT_HEAD(&container->hwpt_list, hwpt, next);
>>>> + return true;
>>>> +}
>>>> +
>>>> +static void iommufd_cdev_autodomains_put(VFIODevice *vbasedev,
>>>> + VFIOIOMMUFDContainer *container)
>>>> +{
>>>> + VFIOIOASHwpt *hwpt = vbasedev->hwpt;
>>>> +
>>>> + QLIST_REMOVE(vbasedev, hwpt_next);
>>>> + vbasedev->hwpt = NULL;
>>>> +
>>>> + if (QLIST_EMPTY(&hwpt->device_list)) {
>>>> + QLIST_REMOVE(hwpt, next);
>>>> + iommufd_backend_free_id(container->be, hwpt->hwpt_id);
>>>> + g_free(hwpt);
>>>> + }
>>>> +}
>>>
>>> Looks the detach flow is still missed?
>>>
>>
>>
>> I don't think so. The iommufd_backend_free_id() pairs with alloc_hwpt call
>and
>> is there for when there's no device attached to the hwpt to actually free
>the
>> hwpt. Besides setting to NULL the device hwpt, the detach flow was fixed
>below (...)
>>
>>>> +
>>>> static bool iommufd_cdev_attach_container(VFIODevice *vbasedev,
>>>> VFIOIOMMUFDContainer *container,
>>>> Error **errp)
>>>> {
>>>> + /* mdevs aren't physical devices and will fail with auto domains */
>>>> + if (!vbasedev->mdev) {
>>>> + return iommufd_cdev_autodomains_get(vbasedev, container,
>errp);
>>>> + }
>>>> +
>>>> return !iommufd_cdev_attach_ioas_hwpt(vbasedev, container-
>>ioas_id,
>>>> errp);
>>>> }
>>>>
>>>> @@ -227,6 +305,11 @@ static void
>>>> iommufd_cdev_detach_container(VFIODevice *vbasedev,
>>>> if (!iommufd_cdev_detach_ioas_hwpt(vbasedev, &err)) {
>>>
>>> Shouldn't we check mdev before calling this?
>>>
>> (...) here. Detach needs to be called for both, and keep in mind that this
>> doesn't a pt_id, as the ioctl detaches from whatever domain or emulated
>idea of
>> it (for mdev) that it has previously been called IOMMUFD_ATTACH with.
>>
>> We also call this with mdev we just don't call it with a hwpt_id but rather
>use
>> autodomains (and it doesn't actually allocate a hw domain)
>>
>>>> error_report_err(err);
>>>> }
>>>> +
>>>> + if (vbasedev->hwpt) {
>>>> + iommufd_cdev_autodomains_put(vbasedev, container);
>>>> + }
>>>> +
>>>> }
>>>>
>>>> static void iommufd_cdev_container_destroy(VFIOIOMMUFDContainer
>>>> *container)
>>>> @@ -354,6 +437,7 @@ static bool iommufd_cdev_attach(const char
>*name,
>>>> VFIODevice *vbasedev,
>>>> container =
>>>> VFIO_IOMMU_IOMMUFD(object_new(TYPE_VFIO_IOMMU_IOMMUFD));
>>>> container->be = vbasedev->iommufd;
>>>> container->ioas_id = ioas_id;
>>>> + QLIST_INIT(&container->hwpt_list);
>>>
>>> This can be in ::instance_init().
>>>
>> But there's no instance_init() for TYPE_VFIO_IOMMU_IOMMUFD. This is
>where all
>> IOMMUFD container stuff is taking place aiui.
>
>We can add an .instance_init() handler later on. It would be cleaner I agree
>but it shouldn't be a reason to block the series.
Yes, it's minor.
>
>Zhenzhong,
>
>Did Joao address your concerns ?
Sure.
Thanks
Zhenzhong
>
>Thanks,
>
>C.
>
>
>
>
>>> Thanks
>>> Zhenzhong
>>>
>>>>
>>>> bcontainer = &container->bcontainer;
>>>> vfio_address_space_insert(space, bcontainer);
>>>> diff --git a/backends/trace-events b/backends/trace-events
>>>> index 211e6f374adc..4d8ac02fe7d6 100644
>>>> --- a/backends/trace-events
>>>> +++ b/backends/trace-events
>>>> @@ -14,4 +14,5 @@ iommufd_backend_map_dma(int iommufd,
>uint32_t
>>>> ioas, uint64_t iova, uint64_t size
>>>> iommufd_backend_unmap_dma_non_exist(int iommufd, uint32_t ioas,
>>>> uint64_t iova, uint64_t size, int ret) " Unmap nonexistent mapping:
>>>> iommufd=%d ioas=%d iova=0x%"PRIx64" size=0x%"PRIx64" (%d)"
>>>> iommufd_backend_unmap_dma(int iommufd, uint32_t ioas, uint64_t
>iova,
>>>> uint64_t size, int ret) " iommufd=%d ioas=%d iova=0x%"PRIx64"
>>>> size=0x%"PRIx64" (%d)"
>>>> iommufd_backend_alloc_ioas(int iommufd, uint32_t ioas) "
>iommufd=%d
>>>> ioas=%d"
>>>> +iommufd_backend_alloc_hwpt(int iommufd, uint32_t dev_id,
>uint32_t
>>>> pt_id, uint32_t flags, uint32_t hwpt_type, uint32_t len, uint64_t
>data_ptr,
>>>> uint32_t out_hwpt_id, int ret) " iommufd=%d dev_id=%u pt_id=%u
>>>> flags=0x%x hwpt_type=%u len=%u data_ptr=0x%"PRIx64"
>out_hwpt=%u
>>>> (%d)"
>>>> iommufd_backend_free_id(int iommufd, uint32_t id, int ret) "
>iommufd=%d
>>>> id=%d (%d)"
>>>> --
>>>> 2.17.2
>>>
>>
