On 19/02/2024 08:58, Avihai Horon wrote:
> Hi Joao,
>
> On 12/02/2024 15:56, Joao Martins wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> There's generally two modes of operation for IOMMUFD:
>>
>> * The simple user API which intends to perform relatively simple things
>> with IOMMUs e.g. DPDK. It generally creates an IOAS and attach to VFIO
>> and mainly performs IOAS_MAP and UNMAP.
>>
>> * 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.
>>
>> For dirty tracking such property is enabled/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)
>>
>> 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 mimmicing kernel
>> iommufd_device_auto_get_domain(). If this fails (i.e. mdevs) it falls back
>> to IOAS attach.
>>
>> Signed-off-by: Joao Martins <joao.m.mart...@oracle.com>
>> ---
>> Right now the only alternative to a userspace autodomains implementation
>> is to mimmicing all the flags being added to HWPT_ALLOC but into VFIO
>> IOAS attach. So opted for autodomains userspace approach to avoid the
>> duplication of hwpt-alloc flags vs attach-ioas flags. I lack mdev real
>> drivers atm, so testing with those is still TBD.
>>
>> Opinions, comments, welcome!
>> ---
>> backends/iommufd.c | 29 +++++++++++++
>> backends/trace-events | 1 +
>> hw/vfio/iommufd.c | 78 +++++++++++++++++++++++++++++++++++
>> include/hw/vfio/vfio-common.h | 9 ++++
>> include/sysemu/iommufd.h | 4 ++
>> 5 files changed, 121 insertions(+)
>>
>> diff --git a/backends/iommufd.c b/backends/iommufd.c
>> index 8486894f1b3f..2970135af4b9 100644
>> --- a/backends/iommufd.c
>> +++ b/backends/iommufd.c
>> @@ -211,6 +211,35 @@ int iommufd_backend_unmap_dma(IOMMUFDBackend *be,
>> uint32_t ioas_id,
>> return ret;
>> }
>>
>> +int iommufd_backend_alloc_hwpt(int iommufd, 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)
>> +{
>> + int ret;
>> + 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 = (uint64_t)data_ptr,
>> + .__reserved = 0,
>> + };
>> +
>> + ret = ioctl(iommufd, IOMMU_HWPT_ALLOC, &alloc_hwpt);
>> + trace_iommufd_backend_alloc_hwpt(iommufd, dev_id, pt_id, flags,
>> data_type,
>> + data_len, (uint64_t)data_ptr,
>> + alloc_hwpt.out_hwpt_id, ret);
>> + if (ret) {
>> + error_report("IOMMU_HWPT_ALLOC failed: %m");
>> + } else {
>> + *out_hwpt = alloc_hwpt.out_hwpt_id;
>> + }
>> + return !ret ? 0 : -errno;
>> +}
>> +
>> static const TypeInfo iommufd_backend_info = {
>> .name = TYPE_IOMMUFD_BACKEND,
>> .parent = TYPE_OBJECT,
>> diff --git a/backends/trace-events b/backends/trace-events
>> index d45c6e31a67e..f83a276a4253 100644
>> --- a/backends/trace-events
>> +++ b/backends/trace-events
>> @@ -13,5 +13,6 @@ iommu_backend_set_fd(int fd) "pre-opened /dev/iommu fd=%d"
>> iommufd_backend_map_dma(int iommufd, uint32_t ioas, uint64_t iova, uint64_t
>> size, void *vaddr, bool readonly, int ret) " iommufd=%d ioas=%d
>> iova=0x%"PRIx64" size=0x%"PRIx64" addr=%p readonly=%d (%d)"
>> 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_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_alloc_ioas(int iommufd, uint32_t ioas, int ret) "
>> iommufd=%d
>> ioas=%d (%d)"
>> iommufd_backend_free_id(int iommufd, uint32_t id, int ret) " iommufd=%d
>> id=%d (%d)"
>> diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
>> index 7d39d7a5fa51..ca7ec45e725c 100644
>> --- a/hw/vfio/iommufd.c
>> +++ b/hw/vfio/iommufd.c
>> @@ -219,10 +219,82 @@ static int iommufd_cdev_detach_ioas_hwpt(VFIODevice
>> *vbasedev, Error **errp)
>> return ret;
>> }
>>
>> +static int iommufd_cdev_autodomains_get(VFIODevice *vbasedev,
>> + VFIOIOMMUFDContainer *container,
>> + Error **errp)
>> +{
>> + int iommufd = vbasedev->iommufd_dev.iommufd->fd;
>> + VFIOIOASHwpt *hwpt;
>> + Error *err = NULL;
>> + int ret = -EINVAL;
>
> The -EINVAL initialization is not necessary.
>
/me nods
>> + uint32_t hwpt_id;
>> +
>> + /* Try to find a domain */
>> + QLIST_FOREACH(hwpt, &container->hwpt_list, next) {
>> + ret = iommufd_cdev_attach_ioas_hwpt(vbasedev, hwpt->hwpt_id, &err);
>> + if (ret) {
>> + /* -EINVAL means the domain is incompatible with the device. */
>> + if (ret == -EINVAL) {
>
> On error iommufd_cdev_attach_ioas_hwpt() returns -1 and not -errno, so I guess
> we need to change it.
>
/me facepalm yes indeed
>> + continue;
>> + }
>> + return ret;
>> + } else {
>> + vbasedev->hwpt = hwpt;
>> + QLIST_INSERT_HEAD(&hwpt->device_list, vbasedev, hwpt_next);
>> + return 0;
>> + }
>> + }
>> +
>> + ret = iommufd_backend_alloc_hwpt(iommufd,
>> + vbasedev->iommufd_dev.devid,
>> + container->ioas_id, 0, 0, 0,
>
> Should we explicitly pass IOMMU_HWPT_DATA_NONE instead of 0?
>
That's better yes.
>> + NULL, &hwpt_id);
>> + if (ret) {
>> + error_append_hint(&err,
>> + "Failed to allocate HWPT for device %s. Fallback to IOAS
>> attach\n",
>> + vbasedev->name);
>> + warn_report_err(err);
>> + return ret;
>> + }
>> +
>> + 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, &err);
>> + if (ret) {
>> + iommufd_backend_free_id(container->be, hwpt->hwpt_id);
>> + g_free(hwpt);
>> + return ret;
>> + }
>> +
>> + vbasedev->hwpt = hwpt;
>> + QLIST_INSERT_HEAD(&hwpt->device_list, vbasedev, hwpt_next);
>> + QLIST_INSERT_HEAD(&container->hwpt_list, hwpt, next);
>> + return 0;
>
> I think we need to improve error handling in this function.
> There are various places where err is not freed/reported, not NULL-ed before
> re-used, or used in error_append_hint() although it can be NULL.
> If there are places where Error can be ignored, we can pass NULL instead of
> &err.
> Plus, errp parameter passed to this function is never used.
>
Let me fix all those for the non-RFC v3.
>> +}
>> +
>> +static void iommufd_cdev_autodomains_put(VFIODevice *vbasedev,
>> + VFIOIOMMUFDContainer *container)
>> +{
>> + VFIOIOASHwpt *hwpt = vbasedev->hwpt;
>> +
>> + QLIST_REMOVE(vbasedev, hwpt_next);
>> + QLIST_REMOVE(hwpt, next);
>
> Shouldn't QLIST_REMOVE(hwpt, next) be moved inside the if?
>
yes
>> + if (QLIST_EMPTY(&hwpt->device_list)) {
>> + iommufd_backend_free_id(container->be, hwpt->hwpt_id);
>> + g_free(hwpt);
>> + }
>> +}
>> +
>> static int iommufd_cdev_attach_container(VFIODevice *vbasedev,
>> VFIOIOMMUFDContainer *container,
>> Error **errp)
>> {
>> + if (!iommufd_cdev_autodomains_get(vbasedev, container, errp)) {
>> + return 0;
>> + }
>
> If errp is used in iommufd_cdev_autodomains_get() eventually, we need to make
> sure it doens't contain an Error object before using it again below.
>
yes
