Message ID | cover.1749884998.git.nicolinc@nvidia.com |
---|---|
Headers | show |
Series | iommufd: Add vIOMMU infrastructure (Part-4 HW QUEUE) | expand |
On 6/14/2025 3:14 PM, Nicolin Chen wrote: > The iommu_hw_info can output via the out_data_type field the vendor data > type from a driver, but this only allows driver to report one data type. > > Now, with SMMUv3 having a Tegra241 CMDQV implementation, it has two sets > of types and data structs to report. > > One way to support that is to use the same type field bidirectionally. > > Reuse the same field by adding an "in_data_type", allowing user space to > request for a specific type and to get the corresponding data. > > For backward compatibility, since the ioctl handler has never checked an > input value, add an IOMMU_HW_INFO_FLAG_INPUT_TYPE to switch between the > old output-only field and the new bidirectional field. > > Reviewed-by: Kevin Tian<kevin.tian@intel.com> > Signed-off-by: Nicolin Chen<nicolinc@nvidia.com> Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>
On Sat, Jun 14, 2025 at 12:14:32AM -0700, Nicolin Chen wrote: > Now, access->ops can be NULL, to support an internal use case for the new > HW queue object. Since an access object in this case will be allocated by > an inernal iommufd object, the refcount on the ictx should be skipped, so > as not to deadlock the release of the ictx as it would otherwise wait for > the release of the access first during the release of the internal object > that could wait for the release of ictx: > ictx --releases--> hw_queue --releases--> access > ^ | > |_________________releases________________v > > Add a set of lightweight internal APIs to unlink access and ictx: > ictx --releases--> hw_queue --releases--> access > > Signed-off-by: Nicolin Chen <nicolinc@nvidia.com> > --- > drivers/iommu/iommufd/iommufd_private.h | 8 ++++ > drivers/iommu/iommufd/device.c | 59 +++++++++++++++++++++---- > 2 files changed, 58 insertions(+), 9 deletions(-) > > diff --git a/drivers/iommu/iommufd/iommufd_private.h b/drivers/iommu/iommufd/iommufd_private.h > index 4a375a8c9216..468717d5e5bc 100644 > --- a/drivers/iommu/iommufd/iommufd_private.h > +++ b/drivers/iommu/iommufd/iommufd_private.h > @@ -484,6 +484,14 @@ void iopt_remove_access(struct io_pagetable *iopt, > struct iommufd_access *access, u32 iopt_access_list_id); > void iommufd_access_destroy_object(struct iommufd_object *obj); > > +/* iommufd_access for internal use */ > +struct iommufd_access *iommufd_access_create_internal(struct iommufd_ctx *ictx); > +#define iommufd_access_destroy_internal(ictx, access) \ > + iommufd_object_destroy_user(ictx, &(access)->obj) Use a static inline please > +int iommufd_access_attach_internal(struct iommufd_access *access, > + struct iommufd_ioas *ioas); > +#define iommufd_access_detach_internal(access) iommufd_access_detach(access) > struct iommufd_eventq { > struct iommufd_object obj; > struct iommufd_ctx *ictx; > diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c > index 9293722b9cff..ad33f1e41a24 100644 > --- a/drivers/iommu/iommufd/device.c > +++ b/drivers/iommu/iommufd/device.c > @@ -1084,7 +1084,39 @@ void iommufd_access_destroy_object(struct iommufd_object *obj) > if (access->ioas) > WARN_ON(iommufd_access_change_ioas(access, NULL)); > mutex_unlock(&access->ioas_lock); > - iommufd_ctx_put(access->ictx); > + if (access->ops) > + iommufd_ctx_put(access->ictx); I was hoping we could null the ictx to signal internal? That didn't work out? I would at least add a comment here this is filtering internal that doesn't have ictx. Maybe a little inline 'iommufd_access_is_internal' is appropriate. We'll be sad down the road if we need ops for internal. Jason
On Sat, Jun 14, 2025 at 12:14:38AM -0700, Nicolin Chen wrote: > +/* Entry for iommufd_ctx::mt_mmap */ > +struct iommufd_mmap { > + struct iommufd_object *owner; > + > + /* Allocated start position in mt_mmap tree */ > + unsigned long startp; pgoff_t, looks like this is already in PAGE_SIZE units. > + /* Physical range for io_remap_pfn_range() */ > + unsigned long mmio_pfn; physaddr_t and maybe don't use pfn? > + unsigned long num_pfns; size_t Rest looks OK Reviewed-by: Jason Gunthorpe <jgg@nvidia.com> Jason
On Sat, Jun 14, 2025 at 12:14:44AM -0700, Nicolin Chen wrote: > An impl driver might want to allocate its own type of vIOMMU object or the > standard IOMMU_VIOMMU_TYPE_ARM_SMMUV3 by setting up its own SW/HW bits, as > the tegra241-cmdqv driver will add IOMMU_VIOMMU_TYPE_TEGRA241_CMDQV. > > Add vsmmu_size/type and vsmmu_init to struct arm_smmu_impl_ops. Prioritize > them in arm_smmu_get_viommu_size() and arm_vsmmu_init(). > > Reviewed-by: Pranjal Shrivastava <praan@google.com> > Reviewed-by: Kevin Tian <kevin.tian@intel.com> > Signed-off-by: Nicolin Chen <nicolinc@nvidia.com> > --- > drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 5 +++++ > drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c | 8 ++++++++ > 2 files changed, 13 insertions(+) Reviewed-by: Jason Gunthorpe <jgg@nvidia.com> Jason
On Sat, Jun 14, 2025 at 12:14:48AM -0700, Nicolin Chen wrote: > To simplify the mappings from global VCMDQs to VINTFs' LVCMDQs, the design > chose to do static allocations and mappings in the global reset function. > > However, with the user-owned VINTF support, it exposes a security concern: > if user space VM only wants one LVCMDQ for a VINTF, statically mapping two > or more LVCMDQs creates a hidden VCMDQ that user space could DoS attack by > writing random stuff to overwhelm the kernel with unhandleable IRQs. > > Thus, to support the user-owned VINTF feature, a LVCMDQ mapping has to be > done dynamically. > > HW allows pre-assigning global VCMDQs in the CMDQ_ALLOC registers, without > finalizing the mappings by keeping CMDQV_CMDQ_ALLOCATED=0. So, add a pair > of map/unmap helper that simply sets/clears that bit. > > For kernel-owned VINTF0, move LVCMDQ mappings to tegra241_vintf_hw_init(), > and the unmappings to tegra241_vintf_hw_deinit(). > > For user-owned VINTFs that will be added, the mappings/unmappings will be > on demand upon an LVCMDQ allocation from the user space. > > However, the dynamic LVCMDQ mapping/unmapping can complicate the timing of > calling tegra241_vcmdq_hw_init/deinit(), which write LVCMDQ address space, > i.e. requiring LVCMDQ to be mapped. Highlight that with a note to the top > of either of them. > > Acked-by: Pranjal Shrivastava <praan@google.com> > Signed-off-by: Nicolin Chen <nicolinc@nvidia.com> > --- > .../iommu/arm/arm-smmu-v3/tegra241-cmdqv.c | 37 +++++++++++++++++-- > 1 file changed, 33 insertions(+), 4 deletions(-) Reviewed-by: Jason Gunthorpe <jgg@nvidia.com> Jason