>-----Original Message----- >From: Liu, Yi L <[email protected]> >Subject: Re: [PATCH 2/5] backends/iommufd: Extend >iommufd_backend_alloc_hwpt() with fault_id > >On 3/27/26 10:52, Zhenzhong Duan wrote: >> No need to force caller to set IOMMU_HWPT_FAULT_ID_VALID, we take it >> by checking fault_id nonzero. > >this is not reliable. There is no doc that claims fault_id should be >non-zero.
Yes, from kernel code, 0 is never allocated as an object id but it's not documented clearly. Eric ever asked the same question before. https://lore.kernel.org/all/[email protected]/ @Nicolin, @JasonG, just want to ask if you have plan to doc it in kernel. If no, I'll update this patch following Yi's suggestion. Thanks Zhenzhong > >> >> Signed-off-by: Zhenzhong Duan <[email protected]> >> --- >> include/system/iommufd.h | 4 ++-- >> backends/iommufd.c | 13 +++++++++---- >> hw/arm/smmuv3-accel.c | 6 +++--- >> hw/i386/intel_iommu_accel.c | 2 +- >> hw/vfio/iommufd.c | 2 +- >> backends/trace-events | 2 +- >> 6 files changed, 17 insertions(+), 12 deletions(-) >> >> diff --git a/include/system/iommufd.h b/include/system/iommufd.h >> index d4ba8434a5..f753bfaf69 100644 >> --- a/include/system/iommufd.h >> +++ b/include/system/iommufd.h >> @@ -85,8 +85,8 @@ bool >iommufd_backend_get_device_info(IOMMUFDBackend *be, uint32_t devid, >> 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); >> + void *data_ptr, uint32_t fault_id, >> + uint32_t *out_hwpt, Error **errp); >> bool iommufd_backend_alloc_viommu(IOMMUFDBackend *be, uint32_t >dev_id, >> uint32_t viommu_type, uint32_t hwpt_id, >> uint32_t *out_hwpt, Error **errp); >> diff --git a/backends/iommufd.c b/backends/iommufd.c >> index 9496377a25..fe64badea0 100644 >> --- a/backends/iommufd.c >> +++ b/backends/iommufd.c >> @@ -310,23 +310,28 @@ int >iommufd_backend_unmap_dma(IOMMUFDBackend *be, uint32_t ioas_id, >> 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) >> + void *data_ptr, uint32_t fault_id, >> + 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, >> + .fault_id = fault_id, >> }; >> >> + if (fault_id) { >> + flags |= IOMMU_HWPT_FAULT_ID_VALID; >> + } >> + alloc_hwpt.flags = flags; >> + >> 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, >> + data_len, (uintptr_t)data_ptr, >> fault_id, >> alloc_hwpt.out_hwpt_id, ret); >> if (ret) { >> error_setg_errno(errp, errno, "Failed to allocate hwpt"); >> diff --git a/hw/arm/smmuv3-accel.c b/hw/arm/smmuv3-accel.c >> index 0af6b3296d..1e2fb1e748 100644 >> --- a/hw/arm/smmuv3-accel.c >> +++ b/hw/arm/smmuv3-accel.c >> @@ -224,7 +224,7 @@ >smmuv3_accel_dev_alloc_translate(SMMUv3AccelDevice *accel_dev, STE *ste, >> accel->viommu->viommu_id, flags, >> IOMMU_HWPT_DATA_ARM_SMMUV3, >> sizeof(nested_data), &nested_data, >> - &hwpt_id, errp)) { >> + 0, &hwpt_id, errp)) { > >same remark. passing 0 is not reliable. :( > >> return NULL; >> } >> >> @@ -558,14 +558,14 @@ smmuv3_accel_alloc_viommu(SMMUv3State *s, >HostIOMMUDeviceIOMMUFD *idev, >> if (!iommufd_backend_alloc_hwpt(idev->iommufd, idev->devid, viommu_id, >> 0, IOMMU_HWPT_DATA_ARM_SMMUV3, >> sizeof(abort_data), &abort_data, >> - &accel->abort_hwpt_id, errp)) { >> + 0, &accel->abort_hwpt_id, errp)) { >> goto free_viommu; >> } >> >> if (!iommufd_backend_alloc_hwpt(idev->iommufd, idev->devid, viommu_id, >> 0, IOMMU_HWPT_DATA_ARM_SMMUV3, >> sizeof(bypass_data), &bypass_data, >> - &accel->bypass_hwpt_id, errp)) { >> + 0, &accel->bypass_hwpt_id, errp)) { >> goto free_abort_hwpt; >> } >> >> diff --git a/hw/i386/intel_iommu_accel.c b/hw/i386/intel_iommu_accel.c >> index e73695ff83..32cca7672a 100644 >> --- a/hw/i386/intel_iommu_accel.c >> +++ b/hw/i386/intel_iommu_accel.c >> @@ -115,7 +115,7 @@ static bool vtd_create_fs_hwpt(VTDHostIOMMUDevice >*vtd_hiod, >> >> return iommufd_backend_alloc_hwpt(idev->iommufd, idev->devid, idev- >>hwpt_id, >> flags, IOMMU_HWPT_DATA_VTD_S1, >> - sizeof(vtd), &vtd, fs_hwpt_id, errp); >> + sizeof(vtd), &vtd, 0, fs_hwpt_id, >> errp); >> } >> >> static void vtd_destroy_old_fs_hwpt(VTDHostIOMMUDevice *vtd_hiod, >> diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c >> index dce4e4ce72..e4e8b266ab 100644 >> --- a/hw/vfio/iommufd.c >> +++ b/hw/vfio/iommufd.c >> @@ -444,7 +444,7 @@ static bool >iommufd_cdev_autodomains_get(VFIODevice *vbasedev, >> if (!iommufd_backend_alloc_hwpt(iommufd, vbasedev->devid, >> container->ioas_id, flags, >> IOMMU_HWPT_DATA_NONE, 0, NULL, >> - &hwpt_id, errp)) { >> + 0, &hwpt_id, errp)) { >> return false; >> } >> >> diff --git a/backends/trace-events b/backends/trace-events >> index 6820a9939e..8a204fcb73 100644 >> --- a/backends/trace-events >> +++ b/backends/trace-events >> @@ -16,7 +16,7 @@ iommufd_backend_map_file_dma(int iommufd, uint32_t >ioas, uint64_t iova, uint64_t >> 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_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 >fault_id, 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" fault_id=%u >out_hwpt=%u (%d)" >> iommufd_backend_alloc_faultq(int iommufd, uint32_t fault_id, uint32_t >fault_fd, int ret) " iommufd=%d fault_id=%d fault_fd=%d (%d)" >> iommufd_backend_free_id(int iommufd, uint32_t id, int ret) " iommufd=%d >id=%d (%d)" >> iommufd_backend_set_dirty(int iommufd, uint32_t hwpt_id, bool start, int >> ret) >" iommufd=%d hwpt=%u enable=%d (%d)"
