Messages in this thread | | | Date | Tue, 13 Jun 2023 11:14:30 +0800 | Subject | Re: [PATCH 1/2] iommu: Prevent RESV_DIRECT devices from blocking domains | From | Baolu Lu <> |
| |
On 6/12/23 4:28 PM, Liu, Jingqi wrote: > On 6/7/2023 11:51 AM, Lu Baolu wrote: >> The IOMMU_RESV_DIRECT flag indicates that a memory region must be mapped >> 1:1 at all times. This means that the region must always be accessible to >> the device, even if the device is attached to a blocking domain. This is >> equal to saying that IOMMU_RESV_DIRECT flag prevents devices from being >> attached to blocking domains. >> >> This also implies that devices that implement RESV_DIRECT regions will be >> prevented from being assigned to user space since taking the DMA >> ownership >> immediately switches to a blocking domain. >> >> The rule of preventing devices with the IOMMU_RESV_DIRECT regions from >> being assigned to user space has existed in the Intel IOMMU driver for >> a long time. Now, this rule is being lifted up to a general core rule, >> as other architectures like AMD and ARM also have RMRR-like reserved >> regions. This has been discussed in the community mailing list and refer >> to below link for more details. >> >> Other places using unmanaged domains for kernel DMA must follow the >> iommu_get_resv_regions() and setup IOMMU_RESV_DIRECT - we do not restrict >> them in the core code. >> >> Cc: Robin Murphy <robin.murphy@arm.com> >> Cc: Alex Williamson <alex.williamson@redhat.com> >> Cc: Kevin Tian <kevin.tian@intel.com> >> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> >> Link: >> https://lore.kernel.org/linux-iommu/BN9PR11MB5276E84229B5BD952D78E9598C639@BN9PR11MB5276.namprd11.prod.outlook.com >> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com> >> --- >> include/linux/iommu.h | 2 ++ >> drivers/iommu/iommu.c | 39 +++++++++++++++++++++++++++++---------- >> 2 files changed, 31 insertions(+), 10 deletions(-) >> >> diff --git a/include/linux/iommu.h b/include/linux/iommu.h >> index d31642596675..fd18019ac951 100644 >> --- a/include/linux/iommu.h >> +++ b/include/linux/iommu.h >> @@ -409,6 +409,7 @@ struct iommu_fault_param { >> * @priv: IOMMU Driver private data >> * @max_pasids: number of PASIDs this device can consume >> * @attach_deferred: the dma domain attachment is deferred >> + * @requires_direct: The driver requested IOMMU_RESV_DIRECT >> * >> * TODO: migrate other per device data pointers under >> iommu_dev_data, e.g. >> * struct iommu_group *iommu_group; >> @@ -422,6 +423,7 @@ struct dev_iommu { >> void *priv; >> u32 max_pasids; >> u32 attach_deferred:1; >> + u32 requires_direct:1; >> }; >> int iommu_device_register(struct iommu_device *iommu, >> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c >> index 9e0228ef612b..e59de7852067 100644 >> --- a/drivers/iommu/iommu.c >> +++ b/drivers/iommu/iommu.c >> @@ -959,12 +959,7 @@ static int >> iommu_create_device_direct_mappings(struct iommu_domain *domain, >> unsigned long pg_size; >> int ret = 0; >> - if (!iommu_is_dma_domain(domain)) >> - return 0; >> - >> - BUG_ON(!domain->pgsize_bitmap); >> - >> - pg_size = 1UL << __ffs(domain->pgsize_bitmap); >> + pg_size = domain->pgsize_bitmap ? 1UL << >> __ffs(domain->pgsize_bitmap) : 0; > Would it be better to add the following check here? > if (WARN_ON(!pg_size)) > return -EINVAL; > > Instead of checking latter in the loop as follows. > if (WARN_ON_ONCE(!pg_size)) { > ret = -EINVAL; > goto out; > }
I am afraid no. Only the paging domains need a valid pg_size. That's the reason why I put it after the iommu_is_dma_domain() check. The previous code has the same behavior too.
Best regards, baolu
| |