| Message ID | cover.1746757630.git.nicolinc@nvidia.com |
|---|---|
| Headers | show |
| Series | iommufd: Add vIOMMU infrastructure (Part-4 HW QUEUE) | expand |
On Thu, May 08, 2025 at 08:02:27PM -0700, Nicolin Chen wrote: > An IOMMU driver that allocated a vIOMMU may want to revert the allocation, > if it encounters an internal error after the allocation. So, there needs a > destroy helper for drivers to use. For instance: > > static my_viommu_alloc() > { > ... > my_viommu = iommufd_viommu_alloc(viomm, struct my_viommu, core); > ... > ret = init_my_viommu(); > if (ret) { > /* Need to destroy the my_viommu->core */ > return ERR_PTR(ret); > } > return &my_viommu->core; > } > > Move iommufd_object_abort() to the driver.c file and the public header, to > introduce common iommufd_struct_destroy() helper that will abort all kinds > of driver structures, not confined to iommufd_viommu but also the new ones > being added in the future. > > Reviewed-by: Jason Gunthorpe <jgg@nvidia.com> > Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com> > Signed-off-by: Nicolin Chen <nicolinc@nvidia.com> > --- > drivers/iommu/iommufd/iommufd_private.h | 1 - > include/linux/iommufd.h | 16 ++++++++++++++++ > drivers/iommu/iommufd/driver.c | 14 ++++++++++++++ > drivers/iommu/iommufd/main.c | 13 ------------- > 4 files changed, 30 insertions(+), 14 deletions(-) One idea that struck me when I was looking at this was to copy the technique from rdma. When an object is allocated we keep track of it in the struct iommufd_ucmd. Then when the command is over the core code either aborts or finalizes the objects in the iommufd_ucmd. This way you don't have to expose abort and related to drivers. Something like this: diff --git a/drivers/iommu/iommufd/driver.c b/drivers/iommu/iommufd/driver.c index 2d2695e2562d02..289dd19eec90f1 100644 --- a/drivers/iommu/iommufd/driver.c +++ b/drivers/iommu/iommufd/driver.c @@ -36,6 +36,16 @@ struct iommufd_object *_iommufd_object_alloc(struct iommufd_ctx *ictx, } EXPORT_SYMBOL_NS_GPL(_iommufd_object_alloc, "IOMMUFD"); +struct iommufd_object *_iommufd_object_alloc_ucmd(struct iommufd_ucmd *ucmd, + size_t size, + enum iommufd_object_type type) +{ + if (ucmd->alloced_obj) + return ERR_PTR(-EBUSY); + ucmd->alloced_obj = _iommufd_object_alloc(ucmd->ictx, size, type); + return ucmd->alloced_obj; +} + /* Undo _iommufd_object_alloc() if iommufd_object_finalize() was not called */ void iommufd_object_abort(struct iommufd_ctx *ictx, struct iommufd_object *obj) { diff --git a/drivers/iommu/iommufd/eventq.c b/drivers/iommu/iommufd/eventq.c index f39cf079734769..f109948a81ac8c 100644 --- a/drivers/iommu/iommufd/eventq.c +++ b/drivers/iommu/iommufd/eventq.c @@ -473,7 +473,7 @@ int iommufd_fault_alloc(struct iommufd_ucmd *ucmd) if (cmd->flags) return -EOPNOTSUPP; - fault = __iommufd_object_alloc(ucmd->ictx, fault, IOMMUFD_OBJ_FAULT, + fault = __iommufd_object_alloc_ucmd(ucmd, fault, IOMMUFD_OBJ_FAULT, common.obj); if (IS_ERR(fault)) return PTR_ERR(fault); @@ -483,10 +483,8 @@ int iommufd_fault_alloc(struct iommufd_ucmd *ucmd) fdno = iommufd_eventq_init(&fault->common, "[iommufd-pgfault]", ucmd->ictx, &iommufd_fault_fops); - if (fdno < 0) { - rc = fdno; - goto out_abort; - } + if (fdno < 0) + return fdno; cmd->out_fault_id = fault->common.obj.id; cmd->out_fault_fd = fdno; @@ -494,7 +492,6 @@ int iommufd_fault_alloc(struct iommufd_ucmd *ucmd) rc = iommufd_ucmd_respond(ucmd, sizeof(*cmd)); if (rc) goto out_put_fdno; - iommufd_object_finalize(ucmd->ictx, &fault->common.obj); fd_install(fdno, fault->common.filep); @@ -502,9 +499,6 @@ int iommufd_fault_alloc(struct iommufd_ucmd *ucmd) out_put_fdno: put_unused_fd(fdno); fput(fault->common.filep); -out_abort: - iommufd_object_abort_and_destroy(ucmd->ictx, &fault->common.obj); - return rc; } diff --git a/drivers/iommu/iommufd/iommufd_private.h b/drivers/iommu/iommufd/iommufd_private.h index c87326d7ecfccb..94cafae86d271c 100644 --- a/drivers/iommu/iommufd/iommufd_private.h +++ b/drivers/iommu/iommufd/iommufd_private.h @@ -152,6 +152,7 @@ struct iommufd_ucmd { void __user *ubuffer; u32 user_size; void *cmd; + struct iommufd_object *alloced_obj; }; int iommufd_vfio_ioctl(struct iommufd_ctx *ictx, unsigned int cmd, @@ -258,6 +259,15 @@ iommufd_object_put_and_try_destroy(struct iommufd_ctx *ictx, #define iommufd_object_alloc(ictx, ptr, type) \ __iommufd_object_alloc(ictx, ptr, type, obj) +#define __iommufd_object_alloc_uctx(uctx, ptr, type, obj) \ + container_of(_iommufd_object_alloc_ucmd( \ + uctx, \ + sizeof(*(ptr)) + BUILD_BUG_ON_ZERO( \ + offsetof(typeof(*(ptr)), \ + obj) != 0), \ + type), \ + typeof(*(ptr)), obj) + /* * The IO Address Space (IOAS) pagetable is a virtual page table backed by the * io_pagetable object. It is a user controlled mapping of IOVA -> PFNs. The diff --git a/drivers/iommu/iommufd/main.c b/drivers/iommu/iommufd/main.c index 1d7f3584aea0f7..0bc9c02f6bfc4f 100644 --- a/drivers/iommu/iommufd/main.c +++ b/drivers/iommu/iommufd/main.c @@ -408,6 +408,14 @@ static long iommufd_fops_ioctl(struct file *filp, unsigned int cmd, if (ret) return ret; ret = op->execute(&ucmd); + + if (ucmd.alloced_obj) { + if (ret) + iommufd_object_abort_and_destroy(ictx, + ucmd.alloced_obj); + else + iommufd_object_finalize(ictx, ucmd.alloced_obj); + } return ret; }
On Thu, May 08, 2025 at 08:02:37PM -0700, Nicolin Chen wrote: > With the introduction of the new object and its infrastructure, update the > doc to reflect that. > > Signed-off-by: Nicolin Chen <nicolinc@nvidia.com> > --- > Documentation/userspace-api/iommufd.rst | 12 ++++++++++++ > 1 file changed, 12 insertions(+) Reviewed-by: Jason Gunthorpe <jgg@nvidia.com> Jason
> From: Jason Gunthorpe <jgg@nvidia.com> > Sent: Friday, May 16, 2025 12:47 AM > > On Thu, May 08, 2025 at 08:02:35PM -0700, Nicolin Chen wrote: > > + /* vma->vm_pgoff carries an index to an mtree entry (immap) */ > > + immap = mtree_load(&ictx->mt_mmap, vma->vm_pgoff); > > + if (!immap) > > + return -ENXIO; > > + if (length >> PAGE_SHIFT != immap->num_pfns) > > + return -ENXIO; > > This needs to validate that vm_pgoff is at the start of the immap or > num_pfns is the wrong thing to validate length against. > vm_pgoff is the index into mtree. If it's wrong mtree_load() will fail already?
On Fri, May 16, 2025 at 10:29:56AM -0300, Jason Gunthorpe wrote: > On Fri, May 16, 2025 at 04:08:25AM +0000, Tian, Kevin wrote: > > > From: Jason Gunthorpe <jgg@nvidia.com> > > > Sent: Friday, May 16, 2025 12:47 AM > > > > > > On Thu, May 08, 2025 at 08:02:35PM -0700, Nicolin Chen wrote: > > > > + /* vma->vm_pgoff carries an index to an mtree entry (immap) */ > > > > + immap = mtree_load(&ictx->mt_mmap, vma->vm_pgoff); > > > > + if (!immap) > > > > + return -ENXIO; > > > > + if (length >> PAGE_SHIFT != immap->num_pfns) > > > > + return -ENXIO; > > > > > > This needs to validate that vm_pgoff is at the start of the immap or > > > num_pfns is the wrong thing to validate length against. > > > > > > > vm_pgoff is the index into mtree. If it's wrong mtree_load() will > > fail already? > > I'm not sure? I thought mtree_load will return any range that > intersects with the given index? > > Otherwise what is the point of having a range based datastructure? Yea, I can confirm that providing a vm_pgoff that's in the range (though not the startp) could get immap too. I am adding negative test coverage for the vm_pgoff/length input for the following ifs: + /* vma->vm_pgoff carries an index to an mtree entry (immap) */ + immap = mtree_load(&ictx->mt_mmap, vma->vm_pgoff); + if (!immap) + return -ENXIO; + /* Validate the vm_pgoff and length against the registered region */ + if (vma->vm_pgoff != immap->startp) + return -ENXIO; + if (length != immap->num_pfns << PAGE_SHIFT) + return -ENXIO; Thanks Nicolin
Kevin, Nicolin, On 5/16/2025 8:29 AM, Tian, Kevin wrote: >> From: Nicolin Chen <nicolinc@nvidia.com> >> Sent: Friday, May 16, 2025 10:30 AM >> >> On Thu, May 15, 2025 at 05:58:41AM +0000, Tian, Kevin wrote: >>>> From: Nicolin Chen <nicolinc@nvidia.com> >>>> Sent: Friday, May 9, 2025 11:03 AM >>>> >>>> Add IOMMUFD_OBJ_HW_QUEUE with an iommufd_hw_queue structure, >>>> representing >>>> a HW-accelerated queue type of IOMMU's physical queue that can be >> passed >>>> through to a user space VM for direct hardware control, such as: >>>> - NVIDIA's Virtual Command Queue >>>> - AMD vIOMMU's Command Buffer, Event Log Buffer, and PPR Log Buffer >>>> >>>> Introduce an allocator iommufd_hw_queue_alloc(). And add a pair of >>>> viommu >>>> ops for iommufd to forward user space ioctls to IOMMU drivers. >>>> >>>> Given that the first user of this HW QUEUE (tegra241-cmdqv) will need to >>>> ensure the queue memory to be physically contiguous, add a flag >> property >>>> in iommufd_viommu_ops and >>>> IOMMUFD_VIOMMU_FLAG_HW_QUEUE_READS_PA to allow >>>> driver to flag it so that the core will validate the physical pages of a >>>> given guest queue. >>> >>> 'READS' is confusing here. What about xxx_CONTIG_PAS? >> >> Combining Jason's first comments here: >> https://lore.kernel.org/linux- >> iommu/20250515160620.GJ382960@nvidia.com/ >> >> So, pinning should be optional too. And I think there would be >> unlikely a case where HW needs contiguous physical pages while >> not requiring to pin the pages, right? AMD IOMMU needs contiguous GPA space for buffer (like command buffer), not contiguous physical address. >> >> So, we need an flag that could indicate to do both tests. Yet, >> "xxx_CONTIG_PAS" doesn't sound very fitting, compared to this >> "IOMMUFD_VIOMMU_FLAG_HW_QUEUE_READS_PA". >> >> Perhaps, we should just add some comments to clarify a bit. Or >> do you have some better naming? >> > > let's wait until that open is closed, i.e. whether we still let the core > manage it and whether AMD requires pinning even when IOVA > is used. I think we may still want to pin those buffer address. -Vasant
The vIOMMU object is designed to represent a slice of an IOMMU HW for its virtualization features shared with or passed to user space (a VM mostly) in a way of HW acceleration. This extended the HWPT-based design for more advanced virtualization feature. HW QUEUE introduced by this series as a part of the vIOMMU infrastructure represents a HW accelerated queue/buffer for VM to use exclusively, e.g. - NVIDIA's Virtual Command Queue - AMD vIOMMU's Command Buffer, Event Log Buffer, and PPR Log Buffer each of which allows its IOMMU HW to directly access a queue memory owned by a guest VM and allows a guest OS to control the HW queue direclty, to avoid VM Exit overheads to improve the performance. Introduce IOMMUFD_OBJ_HW_QUEUE and its pairing IOMMUFD_CMD_HW_QUEUE_ALLOC allowing VMM to forward the IOMMU-specific queue info, such as queue base address, size, and etc. Meanwhile, a guest-owned queue needs the guest kernel to control the queue by reading/writing its consumer and producer indexes, via MMIO acceses to the hardware MMIO registers. Introduce an mmap infrastructure for iommufd to support passing through a piece of MMIO region from the host physical address space to the guest physical address space. The mmap info (offset/ length) used by an mmap syscall must be pre-allocated and returned to the user space via an output driver-data during an IOMMUFD_CMD_HW_QUEUE_ALLOC call. Thus, it requires a driver-specific user data support in the vIOMMU allocation flow. As a real-world use case, this series implements a HW QUEUE support in the tegra241-cmdqv driver for VCMDQs on NVIDIA Grace CPU. In another word, it is also the Tegra CMDQV series Part-2 (user-space support), reworked from Previous RFCv1: https://lore.kernel.org/all/cover.1712978212.git.nicolinc@nvidia.com/ This enables the HW accelerated feature for NVIDIA Grace CPU. Compared to the standard SMMUv3 operating in the nested translation mode trapping CMDQ for TLBI and ATC_INV commands, this gives a huge performance improvement: 70% to 90% reductions of invalidation time were measured by various DMA unmap tests running in a guest OS. // Unmap latencies from "dma_map_benchmark -g @granule -t @threads", // by toggling "/sys/kernel/debug/iommu/tegra241_cmdqv/bypass_vcmdq" @granule | @threads | bypass_vcmdq=1 | bypass_vcmdq=0 4KB 1 35.7 us 5.3 us 16KB 1 41.8 us 6.8 us 64KB 1 68.9 us 9.9 us 128KB 1 109.0 us 12.6 us 256KB 1 187.1 us 18.0 us 4KB 2 96.9 us 6.8 us 16KB 2 97.8 us 7.5 us 64KB 2 151.5 us 10.7 us 128KB 2 257.8 us 12.7 us 256KB 2 443.0 us 17.9 us This is on Github: https://github.com/nicolinc/iommufd/commits/iommufd_hw_queue-v4 Paring QEMU branch for testing: https://github.com/nicolinc/qemu/commits/wip/for_iommufd_hw_queue-v4 Changelog v4 * Rebase on v6.15-rc5 * Add Reviewed-by from Vasant * Rename "vQUEUE" to "HW QUEUE" * Use "offset" and "length" for all mmap-related variables * [iommufd] Use u64 for guest PA * [iommufd] Fix typo in uAPI doc * [iommufd] Rename immap_id to offset * [iommufd] Drop the partial-size mmap support * [iommufd] Do not replace WARN_ON with WARN_ON_ONCE * [iommufd] Use "u64 base_addr" for queue base address * [iommufd] Use u64 base_pfn/num_pfns for immap structure * [iommufd] Correct the size passed in to mtree_alloc_range() * [iommufd] Add IOMMUFD_VIOMMU_FLAG_HW_QUEUE_READS_PA to viommu_ops v3 https://lore.kernel.org/all/cover.1746139811.git.nicolinc@nvidia.com/ * Add Reviewed-by from Baolu, Pranjal, and Alok * Revise kdocs, uAPI docs, and commit logs * Rename "vCMDQ" back to "vQUEUE" for AMD cases * [tegra] Add tegra241_vcmdq_hw_flush_timeout() * [tegra] Rename vsmmu_alloc to alloc_vintf_user * [tegra] Use writel for SID replacement registers * [tegra] Move mmap removal call to vsmmu_destroy op * [tegra] Fix revert in tegra241_vintf_alloc_lvcmdq_user() * [iommufd] Replace "& ~PAGE_MASK" with PAGE_ALIGNED() * [iommufd] Add an object-type "owner" to immap structure * [iommufd] Drop the ictx input in the new for-driver APIs * [iommufd] Add iommufd_vma_ops to keep track of mmap lifecycle * [iommufd] Add viommu-based iommufd_viommu_alloc/destroy_mmap helpers * [iommufd] Rename iommufd_ctx_alloc/free_mmap to _iommufd_alloc/destroy_mmap v2 https://lore.kernel.org/all/cover.1745646960.git.nicolinc@nvidia.com/ * Add Reviewed-by from Jason * [smmu] Fix vsmmu initial value * [smmu] Support impl for hw_info * [tegra] Rename "slot" to "vsid" * [tegra] Update kdocs and commit logs * [tegra] Map/unmap LVCMDQ dynamically * [tegra] Refcount the previous LVCMDQ * [tegra] Return -EEXIST if LVCMDQ exists * [tegra] Simplify VINTF cleanup routine * [tegra] Use vmid and s2_domain in vsmmu * [tegra] Rename "mmap_pgoff" to "immap_id" * [tegra] Add more addr and length validation * [iommufd] Add more narrative to mmap's kdoc * [iommufd] Add iommufd_struct_depend/undepend() * [iommufd] Rename vcmdq_free op to vcmdq_destroy * [iommufd] Fix bug in iommu_copy_struct_to_user() * [iommufd] Drop is_io from iommufd_ctx_alloc_mmap() * [iommufd] Test the queue memory for its contiguity * [iommufd] Return -ENXIO if address or length fails * [iommufd] Do not change @min_last in mock_viommu_alloc() * [iommufd] Generalize TEGRA241_VCMDQ data in core structure * [iommufd] Add selftest coverage for IOMMUFD_CMD_VCMDQ_ALLOC * [iommufd] Add iopt_pin_pages() to prevent queue memory from unmapping v1 https://lore.kernel.org/all/cover.1744353300.git.nicolinc@nvidia.com/ Thanks Nicolin Nicolin Chen (23): iommufd/viommu: Add driver-allocated vDEVICE support iommu: Pass in a driver-level user data structure to viommu_alloc op iommufd/viommu: Allow driver-specific user data for a vIOMMU object iommu: Add iommu_copy_struct_to_user helper iommufd/driver: Let iommufd_viommu_alloc helper save ictx to viommu->ictx iommufd/driver: Add iommufd_struct_destroy to revert iommufd_viommu_alloc iommufd/selftest: Support user_data in mock_viommu_alloc iommufd/selftest: Add covearge for viommu data iommufd: Abstract iopt_pin_pages and iopt_unpin_pages helpers iommufd/viommu: Introduce IOMMUFD_OBJ_HW_QUEUE and its related struct iommufd/viommu: Add IOMMUFD_CMD_HW_QUEUE_ALLOC ioctl iommufd/driver: Add iommufd_hw_queue_depend/undepend() helpers iommufd/selftest: Add coverage for IOMMUFD_CMD_HW_QUEUE_ALLOC iommufd: Add mmap interface iommufd/selftest: Add coverage for the new mmap interface Documentation: userspace-api: iommufd: Update HW QUEUE iommu/arm-smmu-v3-iommufd: Add vsmmu_alloc impl op iommu/arm-smmu-v3-iommufd: Support implementation-defined hw_info iommu/tegra241-cmdqv: Use request_threaded_irq iommu/tegra241-cmdqv: Simplify deinit flow in tegra241_cmdqv_remove_vintf() iommu/tegra241-cmdqv: Do not statically map LVCMDQs iommu/tegra241-cmdqv: Add user-space use support iommu/tegra241-cmdqv: Add IOMMU_VEVENTQ_TYPE_TEGRA241_CMDQV support drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 25 +- drivers/iommu/iommufd/io_pagetable.h | 8 + drivers/iommu/iommufd/iommufd_private.h | 28 +- drivers/iommu/iommufd/iommufd_test.h | 20 + include/linux/iommu.h | 43 +- include/linux/iommufd.h | 186 ++++++- include/uapi/linux/iommufd.h | 116 ++++- tools/testing/selftests/iommu/iommufd_utils.h | 52 +- .../arm/arm-smmu-v3/arm-smmu-v3-iommufd.c | 43 +- .../iommu/arm/arm-smmu-v3/tegra241-cmdqv.c | 490 +++++++++++++++++- drivers/iommu/iommufd/device.c | 117 +---- drivers/iommu/iommufd/driver.c | 94 ++++ drivers/iommu/iommufd/io_pagetable.c | 95 ++++ drivers/iommu/iommufd/main.c | 80 ++- drivers/iommu/iommufd/selftest.c | 133 ++++- drivers/iommu/iommufd/viommu.c | 121 ++++- tools/testing/selftests/iommu/iommufd.c | 97 +++- .../selftests/iommu/iommufd_fail_nth.c | 11 +- Documentation/userspace-api/iommufd.rst | 12 + 19 files changed, 1577 insertions(+), 194 deletions(-) base-commit: 92a09c47464d040866cf2b4cd052bc60555185fb