The IOMMU subsystem has undergone some changes, including the removal
of iommu_ops from the bus structure. Consequently, the existing domain
allocation interface, which relies on a bus type argument, is no longer
relevant:
struct iommu_domain *iommu_domain_alloc(struct bus_type *bus)
This series is designed to refactor the use of this interface. It
proposes two new interfaces to replace iommu_domain_alloc():
- iommu_user_domain_alloc(): This interface is intended for allocating
iommu domains managed by userspace for device passthrough scenarios,
such as those used by iommufd, vfio, and vdpa. It clearly indicates
that the domain is for user-managed device DMA.
If an IOMMU driver does not implement iommu_ops->domain_alloc_user,
this interface will rollback to the generic paging domain allocation.
- iommu_paging_domain_alloc(): This interface is for allocating iommu
domains managed by kernel drivers for kernel DMA purposes. It takes a
device pointer as a parameter, which better reflects the current
design of the IOMMU subsystem.
The majority of device drivers currently using iommu_domain_alloc() do
so to allocate a domain for a specific device and then attach that
domain to the device. These cases can be straightforwardly migrated to
the new interfaces.
However, there are some drivers with more complex use cases that do
not fit neatly into this new scheme. For example:
$ git grep "= iommu_domain_alloc"
arch/arm/mm/dma-mapping.c: mapping->domain = iommu_domain_alloc(bus);
drivers/gpu/drm/rockchip/rockchip_drm_drv.c: private->domain = iommu_domain_alloc(private->iommu_dev->bus);
drivers/gpu/drm/tegra/drm.c: tegra->domain = iommu_domain_alloc(&platform_bus_type);
drivers/infiniband/hw/usnic/usnic_uiom.c: pd->domain = domain = iommu_domain_alloc(dev->bus);
This series leave those cases unchanged and keep iommu_domain_alloc()
for their usage. But new drivers should not use it anymore.
The whole series is also available on GitHub:
https://github.com/LuBaolu/intel-iommu/commits/iommu-domain-allocation-refactor-v1
Lu Baolu (20):
iommu: Add iommu_user_domain_alloc() interface
iommufd: Use iommu_user_domain_alloc()
vfio/type1: Use iommu_paging_domain_alloc()
vhost-vdpa: Use iommu_user_domain_alloc()
iommu: Add iommu_paging_domain_alloc() interface
drm/msm: Use iommu_paging_domain_alloc()
drm/nouveau/tegra: Use iommu_paging_domain_alloc()
gpu: host1x: Use iommu_paging_domain_alloc()
media: nvidia: tegra: Use iommu_paging_domain_alloc()
media: venus: firmware: Use iommu_paging_domain_alloc()
ath10k: Use iommu_paging_domain_alloc()
wifi: ath11k: Use iommu_paging_domain_alloc()
remoteproc: Use iommu_paging_domain_alloc()
soc/fsl/qbman: Use iommu_paging_domain_alloc()
iommu/vt-d: Add helper to allocate paging domain
iommu/vt-d: Add domain_alloc_paging support
iommu/vt-d: Simplify compatibility check for identity domain
iommu/vt-d: Enhance compatibility check for paging domain attach
iommu/vt-d: Remove domain_update_iommu_cap()
iommu/vt-d: Remove domain_update_iommu_superpage()
include/linux/iommu.h | 12 +
drivers/gpu/drm/msm/msm_iommu.c | 8 +-
.../drm/nouveau/nvkm/engine/device/tegra.c | 4 +-
drivers/gpu/host1x/dev.c | 6 +-
drivers/iommu/intel/iommu.c | 319 ++++++++----------
drivers/iommu/intel/pasid.c | 28 +-
drivers/iommu/iommu.c | 62 ++++
drivers/iommu/iommufd/hw_pagetable.c | 20 +-
.../media/platform/nvidia/tegra-vde/iommu.c | 6 +-
drivers/media/platform/qcom/venus/firmware.c | 6 +-
drivers/net/wireless/ath/ath10k/snoc.c | 6 +-
drivers/net/wireless/ath/ath11k/ahb.c | 6 +-
drivers/remoteproc/remoteproc_core.c | 6 +-
drivers/soc/fsl/qbman/qman_portal.c | 4 +-
drivers/vfio/vfio_iommu_type1.c | 7 +-
drivers/vhost/vdpa.c | 11 +-
16 files changed, 253 insertions(+), 258 deletions(-)
--
2.34.1
Replace iommu_domain_alloc() with iommu_user_domain_alloc().
Signed-off-by: Lu Baolu <[email protected]>
---
drivers/vhost/vdpa.c | 11 +++--------
1 file changed, 3 insertions(+), 8 deletions(-)
diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
index 63a53680a85c..7784218fd9d2 100644
--- a/drivers/vhost/vdpa.c
+++ b/drivers/vhost/vdpa.c
@@ -1312,26 +1312,21 @@ static int vhost_vdpa_alloc_domain(struct vhost_vdpa *v)
struct vdpa_device *vdpa = v->vdpa;
const struct vdpa_config_ops *ops = vdpa->config;
struct device *dma_dev = vdpa_get_dma_dev(vdpa);
- const struct bus_type *bus;
int ret;
/* Device want to do DMA by itself */
if (ops->set_map || ops->dma_map)
return 0;
- bus = dma_dev->bus;
- if (!bus)
- return -EFAULT;
-
if (!device_iommu_capable(dma_dev, IOMMU_CAP_CACHE_COHERENCY)) {
dev_warn_once(&v->dev,
"Failed to allocate domain, device is not IOMMU cache coherent capable\n");
return -ENOTSUPP;
}
- v->domain = iommu_domain_alloc(bus);
- if (!v->domain)
- return -EIO;
+ v->domain = iommu_user_domain_alloc(dma_dev, 0);
+ if (IS_ERR(v->domain))
+ return PTR_ERR(v->domain);
ret = iommu_attach_device(v->domain, dma_dev);
if (ret)
--
2.34.1
The domain allocated in msm_iommu_new() is for the @dev. Replace
iommu_domain_alloc() with iommu_paging_domain_alloc() to make it explicit.
Update msm_iommu_new() to always return ERR_PTR in failure cases instead
of NULL.
Signed-off-by: Lu Baolu <[email protected]>
---
drivers/gpu/drm/msm/msm_iommu.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/msm/msm_iommu.c b/drivers/gpu/drm/msm/msm_iommu.c
index d5512037c38b..f7e28d4b5f62 100644
--- a/drivers/gpu/drm/msm/msm_iommu.c
+++ b/drivers/gpu/drm/msm/msm_iommu.c
@@ -407,9 +407,9 @@ struct msm_mmu *msm_iommu_new(struct device *dev, unsigned long quirks)
struct msm_iommu *iommu;
int ret;
- domain = iommu_domain_alloc(dev->bus);
- if (!domain)
- return NULL;
+ domain = iommu_paging_domain_alloc(dev);
+ if (IS_ERR(domain))
+ return ERR_CAST(domain);
iommu_set_pgtable_quirks(domain, quirks);
@@ -441,7 +441,7 @@ struct msm_mmu *msm_iommu_gpu_new(struct device *dev, struct msm_gpu *gpu, unsig
struct msm_mmu *mmu;
mmu = msm_iommu_new(dev, quirks);
- if (IS_ERR_OR_NULL(mmu))
+ if (IS_ERR(mmu))
return mmu;
iommu = to_msm_iommu(mmu);
--
2.34.1
An iommu domain is allocated in host1x_iommu_attach() and is attached to
host->dev. Use iommu_paging_domain_alloc() to make it explicit.
Signed-off-by: Lu Baolu <[email protected]>
---
drivers/gpu/host1x/dev.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/host1x/dev.c b/drivers/gpu/host1x/dev.c
index 3a0aaa68ac8d..46a2447c1124 100644
--- a/drivers/gpu/host1x/dev.c
+++ b/drivers/gpu/host1x/dev.c
@@ -404,9 +404,9 @@ static struct iommu_domain *host1x_iommu_attach(struct host1x *host)
if (err < 0)
goto put_group;
- host->domain = iommu_domain_alloc(&platform_bus_type);
- if (!host->domain) {
- err = -ENOMEM;
+ host->domain = iommu_paging_domain_alloc(host->dev);
+ if (IS_ERR(host->domain)) {
+ err = PTR_ERR(host->domain);
goto put_cache;
}
--
2.34.1
An iommu domain is allocated in tegra_vde_iommu_init() and is attached to
vde->dev. Use iommu_paging_domain_alloc() to make it explicit.
Signed-off-by: Lu Baolu <[email protected]>
---
drivers/media/platform/nvidia/tegra-vde/iommu.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/media/platform/nvidia/tegra-vde/iommu.c b/drivers/media/platform/nvidia/tegra-vde/iommu.c
index 5521ed3e465f..5a41b5364238 100644
--- a/drivers/media/platform/nvidia/tegra-vde/iommu.c
+++ b/drivers/media/platform/nvidia/tegra-vde/iommu.c
@@ -78,9 +78,9 @@ int tegra_vde_iommu_init(struct tegra_vde *vde)
arm_iommu_release_mapping(mapping);
}
#endif
- vde->domain = iommu_domain_alloc(&platform_bus_type);
- if (!vde->domain) {
- err = -ENOMEM;
+ vde->domain = iommu_paging_domain_alloc(dev);
+ if (IS_ERR(vde->domain)) {
+ err = PTR_ERR(vde->domain);
goto put_group;
}
--
2.34.1
An iommu domain is allocated in ath10k_fw_init() and is attached to
ar_snoc->fw.dev in the same function. Use iommu_paging_domain_alloc() to
make it explicit.
Signed-off-by: Lu Baolu <[email protected]>
---
drivers/net/wireless/ath/ath10k/snoc.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/net/wireless/ath/ath10k/snoc.c b/drivers/net/wireless/ath/ath10k/snoc.c
index 8530550cf5df..0fe47d51013c 100644
--- a/drivers/net/wireless/ath/ath10k/snoc.c
+++ b/drivers/net/wireless/ath/ath10k/snoc.c
@@ -1635,10 +1635,10 @@ static int ath10k_fw_init(struct ath10k *ar)
ar_snoc->fw.dev = &pdev->dev;
- iommu_dom = iommu_domain_alloc(&platform_bus_type);
- if (!iommu_dom) {
+ iommu_dom = iommu_paging_domain_alloc(ar_snoc->fw.dev);
+ if (IS_ERR(iommu_dom)) {
ath10k_err(ar, "failed to allocate iommu domain\n");
- ret = -ENOMEM;
+ ret = PTR_ERR(iommu_dom);
goto err_unregister;
}
--
2.34.1
An iommu domain is allocated in ath11k_ahb_fw_resources_init() and is
attached to ab_ahb->fw.dev in the same function.
Use iommu_paging_domain_alloc() to make it explicit.
Signed-off-by: Lu Baolu <[email protected]>
---
drivers/net/wireless/ath/ath11k/ahb.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/net/wireless/ath/ath11k/ahb.c b/drivers/net/wireless/ath/ath11k/ahb.c
index ca0f17ddebba..a469647719f9 100644
--- a/drivers/net/wireless/ath/ath11k/ahb.c
+++ b/drivers/net/wireless/ath/ath11k/ahb.c
@@ -1001,10 +1001,10 @@ static int ath11k_ahb_fw_resources_init(struct ath11k_base *ab)
ab_ahb->fw.dev = &pdev->dev;
- iommu_dom = iommu_domain_alloc(&platform_bus_type);
- if (!iommu_dom) {
+ iommu_dom = iommu_paging_domain_alloc(ab_ahb->fw.dev);
+ if (IS_ERR(iommu_dom)) {
ath11k_err(ab, "failed to allocate iommu domain\n");
- ret = -ENOMEM;
+ ret = PTR_ERR(iommu_dom);
goto err_unregister;
}
--
2.34.1
An iommu domain is allocated in rproc_enable_iommu() and is attached to
rproc->dev.parent in the same function.
Use iommu_paging_domain_alloc() to make it explicit.
Signed-off-by: Lu Baolu <[email protected]>
---
drivers/remoteproc/remoteproc_core.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index f276956f2c5c..eb66f78ec8b7 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -109,10 +109,10 @@ static int rproc_enable_iommu(struct rproc *rproc)
return 0;
}
- domain = iommu_domain_alloc(dev->bus);
- if (!domain) {
+ domain = iommu_paging_domain_alloc(dev);
+ if (IS_ERR(domain)) {
dev_err(dev, "can't alloc iommu domain\n");
- return -ENOMEM;
+ return PTR_ERR(domain);
}
iommu_set_fault_handler(domain, rproc_iommu_fault, rproc);
--
2.34.1
Move paging domain allocation code out from intel_iommu_domain_alloc().
The intel_iommu_domain_alloc() is still remaining to allocate an identity
domain. However, it will soon disappear as we are about to convert the
identity domain to a global static one.
Signed-off-by: Lu Baolu <[email protected]>
---
drivers/iommu/intel/iommu.c | 50 ++++++++++++++++---------------------
1 file changed, 22 insertions(+), 28 deletions(-)
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index ccde5f5972e4..eb8e08699b80 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -3708,35 +3708,8 @@ static struct dmar_domain *paging_domain_alloc(struct device *dev, bool first_st
static struct iommu_domain *intel_iommu_domain_alloc(unsigned type)
{
- struct dmar_domain *dmar_domain;
- struct iommu_domain *domain;
-
- switch (type) {
- case IOMMU_DOMAIN_DMA:
- case IOMMU_DOMAIN_UNMANAGED:
- dmar_domain = alloc_domain(type);
- if (!dmar_domain) {
- pr_err("Can't allocate dmar_domain\n");
- return NULL;
- }
- if (md_domain_init(dmar_domain, DEFAULT_DOMAIN_ADDRESS_WIDTH)) {
- pr_err("Domain initialization failed\n");
- domain_exit(dmar_domain);
- return NULL;
- }
-
- domain = &dmar_domain->domain;
- domain->geometry.aperture_start = 0;
- domain->geometry.aperture_end =
- __DOMAIN_MAX_ADDR(dmar_domain->gaw);
- domain->geometry.force_aperture = true;
-
- return domain;
- case IOMMU_DOMAIN_IDENTITY:
+ if (type == IOMMU_DOMAIN_IDENTITY)
return &si_domain->domain;
- default:
- return NULL;
- }
return NULL;
}
@@ -3791,6 +3764,26 @@ intel_iommu_domain_alloc_user(struct device *dev, u32 flags,
return domain;
}
+static struct iommu_domain *intel_iommu_domain_alloc_paging(struct device *dev)
+{
+ struct dmar_domain *dmar_domain;
+ struct device_domain_info *info;
+ struct intel_iommu *iommu;
+
+ /* Do not support the legacy iommu_domain_alloc() interface. */
+ if (!dev)
+ return ERR_PTR(-ENODEV);
+
+ info = dev_iommu_priv_get(dev);
+ iommu = info->iommu;
+ dmar_domain = paging_domain_alloc(dev,
+ sm_supported(iommu) && ecap_flts(iommu->ecap));
+ if (IS_ERR(dmar_domain))
+ return ERR_CAST(dmar_domain);
+
+ return &dmar_domain->domain;
+}
+
static void intel_iommu_domain_free(struct iommu_domain *domain)
{
struct dmar_domain *dmar_domain = to_dmar_domain(domain);
@@ -4650,6 +4643,7 @@ const struct iommu_ops intel_iommu_ops = {
.domain_alloc = intel_iommu_domain_alloc,
.domain_alloc_user = intel_iommu_domain_alloc_user,
.domain_alloc_sva = intel_svm_domain_alloc,
+ .domain_alloc_paging = intel_iommu_domain_alloc_paging,
.probe_device = intel_iommu_probe_device,
.release_device = intel_iommu_release_device,
.get_resv_regions = intel_iommu_get_resv_regions,
--
2.34.1
Currently, the identity domain attachment follows the same path as the
paging domain attachment and is subject to the same compatibility checks
as a normal paging domain. However, this level of check is unnecessary
for the identity domain since it only requires the hardware to support
passthrough mode, which is a given for modern hardware.
On the early VT-d platforms, where hardware passthrough mode is not yet
supported, the identity domain is supported by a makeshift paging domain
with the entire system memory 1:1 mapped. For such early hardware, the
appropriate domain type should be returned in device_def_domain_type(),
and the identity domain should be simplified in compatibility checks.
The identity domain workaround in prepare_domain_attach_device() is just
temporary and should be removed once the identity domain is converted to
have its own dedicated attachment path.
Signed-off-by: Lu Baolu <[email protected]>
---
drivers/iommu/intel/iommu.c | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index eb8e08699b80..693a6d7c79ed 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -2190,6 +2190,16 @@ static bool device_rmrr_is_relaxable(struct device *dev)
*/
static int device_def_domain_type(struct device *dev)
{
+ struct device_domain_info *info = dev_iommu_priv_get(dev);
+ struct intel_iommu *iommu = info->iommu;
+
+ /*
+ * Hardware does not support the passthrough translation mode.
+ * Always use a dynamaic mapping domain.
+ */
+ if (!ecap_pass_through(iommu->ecap))
+ return IOMMU_DOMAIN_DMA;
+
if (dev_is_pci(dev)) {
struct pci_dev *pdev = to_pci_dev(dev);
@@ -3802,6 +3812,14 @@ int prepare_domain_attach_device(struct iommu_domain *domain,
struct intel_iommu *iommu = info->iommu;
int addr_width;
+ /*
+ * This is a temporary solution as the identity domain attachment
+ * goes through this path as well. It should be removed once the
+ * identity domain has its own attach path.
+ */
+ if (domain->type == IOMMU_DOMAIN_IDENTITY)
+ return ecap_pass_through(iommu->ecap) ? 0 : -EOPNOTSUPP;
+
if (dmar_domain->force_snooping && !ecap_sc_support(iommu->ecap))
return -EINVAL;
--
2.34.1
The attributes of a paging domain are initialized during the allocation
process, and any attempt to attach a domain that is not compatible will
result in a failure. Therefore, there is no need to update the domain
attributes at the time of domain attachment.
Signed-off-by: Lu Baolu <[email protected]>
---
drivers/iommu/intel/iommu.c | 86 +------------------------------------
1 file changed, 2 insertions(+), 84 deletions(-)
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index e9393f5c2c50..74e005b1c4b4 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -366,36 +366,6 @@ static bool iommu_paging_structure_coherency(struct intel_iommu *iommu)
ecap_smpwc(iommu->ecap) : ecap_coherent(iommu->ecap);
}
-static void domain_update_iommu_coherency(struct dmar_domain *domain)
-{
- struct iommu_domain_info *info;
- struct dmar_drhd_unit *drhd;
- struct intel_iommu *iommu;
- bool found = false;
- unsigned long i;
-
- domain->iommu_coherency = true;
- xa_for_each(&domain->iommu_array, i, info) {
- found = true;
- if (!iommu_paging_structure_coherency(info->iommu)) {
- domain->iommu_coherency = false;
- break;
- }
- }
- if (found)
- return;
-
- /* No hardware attached; use lowest common denominator */
- rcu_read_lock();
- for_each_active_iommu(iommu, drhd) {
- if (!iommu_paging_structure_coherency(iommu)) {
- domain->iommu_coherency = false;
- break;
- }
- }
- rcu_read_unlock();
-}
-
static int domain_update_iommu_superpage(struct dmar_domain *domain,
struct intel_iommu *skip)
{
@@ -426,29 +396,6 @@ static int domain_update_iommu_superpage(struct dmar_domain *domain,
return fls(mask);
}
-static int domain_update_device_node(struct dmar_domain *domain)
-{
- struct device_domain_info *info;
- int nid = NUMA_NO_NODE;
- unsigned long flags;
-
- spin_lock_irqsave(&domain->lock, flags);
- list_for_each_entry(info, &domain->devices, link) {
- /*
- * There could possibly be multiple device numa nodes as devices
- * within the same domain may sit behind different IOMMUs. There
- * isn't perfect answer in such situation, so we select first
- * come first served policy.
- */
- nid = dev_to_node(info->dev);
- if (nid != NUMA_NO_NODE)
- break;
- }
- spin_unlock_irqrestore(&domain->lock, flags);
-
- return nid;
-}
-
/* Return the super pagesize bitmap if supported. */
static unsigned long domain_super_pgsize_bitmap(struct dmar_domain *domain)
{
@@ -466,35 +413,6 @@ static unsigned long domain_super_pgsize_bitmap(struct dmar_domain *domain)
return bitmap;
}
-/* Some capabilities may be different across iommus */
-void domain_update_iommu_cap(struct dmar_domain *domain)
-{
- domain_update_iommu_coherency(domain);
- domain->iommu_superpage = domain_update_iommu_superpage(domain, NULL);
-
- /*
- * If RHSA is missing, we should default to the device numa domain
- * as fall back.
- */
- if (domain->nid == NUMA_NO_NODE)
- domain->nid = domain_update_device_node(domain);
-
- /*
- * First-level translation restricts the input-address to a
- * canonical address (i.e., address bits 63:N have the same
- * value as address bit [N-1], where N is 48-bits with 4-level
- * paging and 57-bits with 5-level paging). Hence, skip bit
- * [N-1].
- */
- if (domain->use_first_level)
- domain->domain.geometry.aperture_end = __DOMAIN_MAX_ADDR(domain->gaw - 1);
- else
- domain->domain.geometry.aperture_end = __DOMAIN_MAX_ADDR(domain->gaw);
-
- domain->domain.pgsize_bitmap |= domain_super_pgsize_bitmap(domain);
- domain_update_iotlb(domain);
-}
-
struct context_entry *iommu_context_addr(struct intel_iommu *iommu, u8 bus,
u8 devfn, int alloc)
{
@@ -1589,7 +1507,7 @@ int domain_attach_iommu(struct dmar_domain *domain, struct intel_iommu *iommu)
ret = xa_err(curr) ? : -EBUSY;
goto err_clear;
}
- domain_update_iommu_cap(domain);
+ domain_update_iotlb(domain);
spin_unlock(&iommu->lock);
return 0;
@@ -1615,7 +1533,7 @@ void domain_detach_iommu(struct dmar_domain *domain, struct intel_iommu *iommu)
clear_bit(info->did, iommu->domain_ids);
xa_erase(&domain->iommu_array, iommu->seq_id);
domain->nid = NUMA_NO_NODE;
- domain_update_iommu_cap(domain);
+ domain_update_iotlb(domain);
kfree(info);
}
spin_unlock(&iommu->lock);
--
2.34.1
The requirement for consistent super page support across all the IOMMU
hardware in the system has been removed. In the past, if a new IOMMU
was hot-added and lacked consistent super page capability, the hot-add
process would be aborted. However, with the updated attachment semantics,
it is now permissible for the super page capability to vary among
different IOMMU hardware units.
Signed-off-by: Lu Baolu <[email protected]>
---
drivers/iommu/intel/iommu.c | 39 +------------------------------------
1 file changed, 1 insertion(+), 38 deletions(-)
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 74e005b1c4b4..660d2b6c531b 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -366,36 +366,6 @@ static bool iommu_paging_structure_coherency(struct intel_iommu *iommu)
ecap_smpwc(iommu->ecap) : ecap_coherent(iommu->ecap);
}
-static int domain_update_iommu_superpage(struct dmar_domain *domain,
- struct intel_iommu *skip)
-{
- struct dmar_drhd_unit *drhd;
- struct intel_iommu *iommu;
- int mask = 0x3;
-
- if (!intel_iommu_superpage)
- return 0;
-
- /* set iommu_superpage to the smallest common denominator */
- rcu_read_lock();
- for_each_active_iommu(iommu, drhd) {
- if (iommu != skip) {
- if (domain && domain->use_first_level) {
- if (!cap_fl1gp_support(iommu->cap))
- mask = 0x1;
- } else {
- mask &= cap_super_page_val(iommu->cap);
- }
-
- if (!mask)
- break;
- }
- }
- rcu_read_unlock();
-
- return fls(mask);
-}
-
/* Return the super pagesize bitmap if supported. */
static unsigned long domain_super_pgsize_bitmap(struct dmar_domain *domain)
{
@@ -2845,8 +2815,8 @@ int dmar_parse_one_satc(struct acpi_dmar_header *hdr, void *arg)
static int intel_iommu_add(struct dmar_drhd_unit *dmaru)
{
- int sp, ret;
struct intel_iommu *iommu = dmaru->iommu;
+ int ret;
ret = intel_cap_audit(CAP_AUDIT_HOTPLUG_DMAR, iommu);
if (ret)
@@ -2858,13 +2828,6 @@ static int intel_iommu_add(struct dmar_drhd_unit *dmaru)
return -ENXIO;
}
- sp = domain_update_iommu_superpage(NULL, iommu) - 1;
- if (sp >= 0 && !(cap_super_page_val(iommu->cap) & (1 << sp))) {
- pr_warn("%s: Doesn't support large page.\n",
- iommu->name);
- return -ENXIO;
- }
-
/*
* Disable translation if already enabled prior to OS handover.
*/
--
2.34.1
An iommu domain is allocated in portal_set_cpu() and is attached to
pcfg->dev in the same function.
Use iommu_paging_domain_alloc() to make it explicit.
Signed-off-by: Lu Baolu <[email protected]>
---
drivers/soc/fsl/qbman/qman_portal.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/soc/fsl/qbman/qman_portal.c b/drivers/soc/fsl/qbman/qman_portal.c
index e23b60618c1a..91f17cea3744 100644
--- a/drivers/soc/fsl/qbman/qman_portal.c
+++ b/drivers/soc/fsl/qbman/qman_portal.c
@@ -48,8 +48,8 @@ static void portal_set_cpu(struct qm_portal_config *pcfg, int cpu)
struct device *dev = pcfg->dev;
int ret;
- pcfg->iommu_domain = iommu_domain_alloc(&platform_bus_type);
- if (!pcfg->iommu_domain) {
+ pcfg->iommu_domain = iommu_paging_domain_alloc(dev);
+ if (IS_ERR(pcfg->iommu_domain)) {
dev_err(dev, "%s(): iommu_domain_alloc() failed", __func__);
goto no_iommu;
}
--
2.34.1
Replace iommu_domain_alloc() with iommu_user_domain_alloc().
Signed-off-by: Lu Baolu <[email protected]>
---
drivers/iommu/iommufd/hw_pagetable.c | 20 +++++---------------
1 file changed, 5 insertions(+), 15 deletions(-)
diff --git a/drivers/iommu/iommufd/hw_pagetable.c b/drivers/iommu/iommufd/hw_pagetable.c
index 33d142f8057d..ada05fccb36a 100644
--- a/drivers/iommu/iommufd/hw_pagetable.c
+++ b/drivers/iommu/iommufd/hw_pagetable.c
@@ -127,21 +127,11 @@ iommufd_hwpt_paging_alloc(struct iommufd_ctx *ictx, struct iommufd_ioas *ioas,
hwpt_paging->ioas = ioas;
hwpt_paging->nest_parent = flags & IOMMU_HWPT_ALLOC_NEST_PARENT;
- if (ops->domain_alloc_user) {
- hwpt->domain = ops->domain_alloc_user(idev->dev, flags, NULL,
- user_data);
- if (IS_ERR(hwpt->domain)) {
- rc = PTR_ERR(hwpt->domain);
- hwpt->domain = NULL;
- goto out_abort;
- }
- hwpt->domain->owner = ops;
- } else {
- hwpt->domain = iommu_domain_alloc(idev->dev->bus);
- if (!hwpt->domain) {
- rc = -ENOMEM;
- goto out_abort;
- }
+ hwpt->domain = iommu_user_domain_alloc(idev->dev, flags);
+ if (IS_ERR(hwpt->domain)) {
+ rc = PTR_ERR(hwpt->domain);
+ hwpt->domain = NULL;
+ goto out_abort;
}
/*
--
2.34.1
Commit <17de3f5fdd35> ("iommu: Retire bus ops") removes iommu ops from
bus. The iommu subsystem no longer relies on bus for operations. So the
bus parameter in iommu_domain_alloc() is no longer relevant.
Add a new interface named iommu_paging_domain_alloc(), which explicitly
indicates the allocation of a paging domain for DMA managed by a kernel
driver. The new interface takes a device pointer as its parameter, that
better aligns with the current iommu subsystem.
Signed-off-by: Lu Baolu <[email protected]>
---
include/linux/iommu.h | 6 ++++++
drivers/iommu/iommu.c | 20 ++++++++++++++++++++
2 files changed, 26 insertions(+)
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 6648b2415474..16401de7802d 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -781,6 +781,7 @@ extern bool device_iommu_capable(struct device *dev, enum iommu_cap cap);
extern bool iommu_group_has_isolated_msi(struct iommu_group *group);
extern struct iommu_domain *iommu_domain_alloc(const struct bus_type *bus);
struct iommu_domain *iommu_user_domain_alloc(struct device *dev, u32 flags);
+struct iommu_domain *iommu_paging_domain_alloc(struct device *dev);
extern void iommu_domain_free(struct iommu_domain *domain);
extern int iommu_attach_device(struct iommu_domain *domain,
struct device *dev);
@@ -1092,6 +1093,11 @@ static inline struct iommu_domain *iommu_user_domain_alloc(struct device *dev, u
return ERR_PTR(-ENODEV);
}
+static inline struct iommu_domain *iommu_paging_domain_alloc(struct device *dev)
+{
+ return ERR_PTR(-ENODEV);
+}
+
static inline void iommu_domain_free(struct iommu_domain *domain)
{
}
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index f1416892ef8e..7df4a021b040 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -2016,6 +2016,10 @@ static int __iommu_domain_alloc_dev(struct device *dev, void *data)
return 0;
}
+/*
+ * The iommu ops in bus has been retired. Do not use this interface in
+ * new drivers.
+ */
struct iommu_domain *iommu_domain_alloc(const struct bus_type *bus)
{
const struct iommu_ops *ops = NULL;
@@ -2074,6 +2078,22 @@ struct iommu_domain *iommu_user_domain_alloc(struct device *dev, u32 flags)
}
EXPORT_SYMBOL_GPL(iommu_user_domain_alloc);
+/**
+ * iommu_paging_domain_alloc() - Allocate a paging domain
+ * @dev: device for which the domain is allocated
+ *
+ * Allocate a paging domain which will be managed by a kernel driver. Return
+ * allocated domain if successful, or a ERR pointer for failure.
+ */
+struct iommu_domain *iommu_paging_domain_alloc(struct device *dev)
+{
+ if (!dev_has_iommu(dev))
+ return ERR_PTR(-ENODEV);
+
+ return __iommu_domain_alloc(dev_iommu_ops(dev), dev, IOMMU_DOMAIN_UNMANAGED);
+}
+EXPORT_SYMBOL_GPL(iommu_paging_domain_alloc);
+
void iommu_domain_free(struct iommu_domain *domain)
{
if (domain->type == IOMMU_DOMAIN_SVA)
--
2.34.1
In nvkm_device_tegra_probe_iommu(), a paging domain is allocated for @dev
and attached to it on success. Use iommu_paging_domain_alloc() to make it
explicit.
Signed-off-by: Lu Baolu <[email protected]>
---
drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c b/drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c
index 87caa4a72921..763c4c2925f9 100644
--- a/drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c
+++ b/drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c
@@ -120,8 +120,8 @@ nvkm_device_tegra_probe_iommu(struct nvkm_device_tegra *tdev)
mutex_init(&tdev->iommu.mutex);
if (device_iommu_mapped(dev)) {
- tdev->iommu.domain = iommu_domain_alloc(&platform_bus_type);
- if (!tdev->iommu.domain)
+ tdev->iommu.domain = iommu_paging_domain_alloc(dev);
+ if (IS_ERR(tdev->iommu.domain))
goto error;
/*
--
2.34.1
On Wed, May 29, 2024 at 01:32:36PM +0800, Lu Baolu wrote:
> The domain allocated in msm_iommu_new() is for the @dev. Replace
> iommu_domain_alloc() with iommu_paging_domain_alloc() to make it explicit.
>
> Update msm_iommu_new() to always return ERR_PTR in failure cases instead
> of NULL.
Please don't mix unrelated changes, because ...
>
> Signed-off-by: Lu Baolu <[email protected]>
> ---
> drivers/gpu/drm/msm/msm_iommu.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/msm_iommu.c b/drivers/gpu/drm/msm/msm_iommu.c
> index d5512037c38b..f7e28d4b5f62 100644
> --- a/drivers/gpu/drm/msm/msm_iommu.c
> +++ b/drivers/gpu/drm/msm/msm_iommu.c
> @@ -407,9 +407,9 @@ struct msm_mmu *msm_iommu_new(struct device *dev, unsigned long quirks)
> struct msm_iommu *iommu;
> int ret;
>
> - domain = iommu_domain_alloc(dev->bus);
> - if (!domain)
> - return NULL;
> + domain = iommu_paging_domain_alloc(dev);
> + if (IS_ERR(domain))
> + return ERR_CAST(domain);
>
> iommu_set_pgtable_quirks(domain, quirks);
>
> @@ -441,7 +441,7 @@ struct msm_mmu *msm_iommu_gpu_new(struct device *dev, struct msm_gpu *gpu, unsig
> struct msm_mmu *mmu;
>
> mmu = msm_iommu_new(dev, quirks);
> - if (IS_ERR_OR_NULL(mmu))
> + if (IS_ERR(mmu))
> return mmu;
NAK, not having an IOMMU is a poor but legit usecase for some of devices
which don't have IOMMU support yet (for example because of the buggy
implementation for which we were not able to get all the hooks in).
Please don't break compatibility for existing platforms.
>
> iommu = to_msm_iommu(mmu);
> --
> 2.34.1
>
--
With best wishes
Dmitry
An iommu domain is allocated in venus_firmware_init() and is attached to
core->fw.dev in the same function. Use iommu_paging_domain_alloc() to
make it explicit.
Signed-off-by: Lu Baolu <[email protected]>
---
drivers/media/platform/qcom/venus/firmware.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/media/platform/qcom/venus/firmware.c b/drivers/media/platform/qcom/venus/firmware.c
index fe7da2b30482..66a18830e66d 100644
--- a/drivers/media/platform/qcom/venus/firmware.c
+++ b/drivers/media/platform/qcom/venus/firmware.c
@@ -316,10 +316,10 @@ int venus_firmware_init(struct venus_core *core)
core->fw.dev = &pdev->dev;
- iommu_dom = iommu_domain_alloc(&platform_bus_type);
- if (!iommu_dom) {
+ iommu_dom = iommu_paging_domain_alloc(core->fw.dev);
+ if (IS_ERR(iommu_dom)) {
dev_err(core->fw.dev, "Failed to allocate iommu domain\n");
- ret = -ENOMEM;
+ ret = PTR_ERR(iommu_dom);
goto err_unregister;
}
--
2.34.1
Replace iommu_domain_alloc() with iommu_user_domain_alloc().
Signed-off-by: Lu Baolu <[email protected]>
---
drivers/vfio/vfio_iommu_type1.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index 3a0218171cfa..1d553f7f7c26 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -2135,7 +2135,7 @@ static int vfio_iommu_domain_alloc(struct device *dev, void *data)
{
struct iommu_domain **domain = data;
- *domain = iommu_domain_alloc(dev->bus);
+ *domain = iommu_user_domain_alloc(dev, 0);
return 1; /* Don't iterate */
}
@@ -2192,11 +2192,12 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
* us a representative device for the IOMMU API call. We don't actually
* want to iterate beyond the first device (if any).
*/
- ret = -EIO;
iommu_group_for_each_dev(iommu_group, &domain->domain,
vfio_iommu_domain_alloc);
- if (!domain->domain)
+ if (IS_ERR(domain->domain)) {
+ ret = PTR_ERR(domain->domain);
goto out_free_domain;
+ }
if (iommu->nesting) {
ret = iommu_enable_nesting(domain->domain);
--
2.34.1
The domain_alloc_user operation is currently implemented by allocating a
paging domain using iommu_domain_alloc(). This is because it needs to fully
initialize the domain before return. Add a helper to do this to avoid using
iommu_domain_alloc().
Signed-off-by: Lu Baolu <[email protected]>
---
drivers/iommu/intel/iommu.c | 87 +++++++++++++++++++++++++++++++++----
1 file changed, 78 insertions(+), 9 deletions(-)
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 2e9811bf2a4e..ccde5f5972e4 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -3633,6 +3633,79 @@ static struct iommu_domain blocking_domain = {
}
};
+static int iommu_superpage_capability(struct intel_iommu *iommu, bool first_stage)
+{
+ if (!intel_iommu_superpage)
+ return 0;
+
+ if (first_stage)
+ return cap_fl1gp_support(iommu->cap) ? 2 : 1;
+
+ return fls(cap_super_page_val(iommu->cap));
+}
+
+static struct dmar_domain *paging_domain_alloc(struct device *dev, bool first_stage)
+{
+ struct device_domain_info *info = dev_iommu_priv_get(dev);
+ struct intel_iommu *iommu = info->iommu;
+ struct dmar_domain *domain;
+ int addr_width;
+
+ domain = kzalloc(sizeof(*domain), GFP_KERNEL);
+ if (!domain)
+ return ERR_PTR(-ENOMEM);
+
+ INIT_LIST_HEAD(&domain->devices);
+ INIT_LIST_HEAD(&domain->dev_pasids);
+ INIT_LIST_HEAD(&domain->cache_tags);
+ spin_lock_init(&domain->lock);
+ spin_lock_init(&domain->cache_lock);
+ xa_init(&domain->iommu_array);
+
+ domain->nid = dev_to_node(dev);
+ domain->has_iotlb_device = info->ats_enabled;
+ domain->use_first_level = first_stage;
+
+ /* calculate the address width */
+ addr_width = agaw_to_width(iommu->agaw);
+ if (addr_width > cap_mgaw(iommu->cap))
+ addr_width = cap_mgaw(iommu->cap);
+ domain->gaw = addr_width;
+ domain->agaw = iommu->agaw;
+ domain->max_addr = __DOMAIN_MAX_ADDR(addr_width);
+
+ /* iommu memory access coherency */
+ domain->iommu_coherency = iommu_paging_structure_coherency(iommu);
+
+ /* pagesize bitmap */
+ domain->domain.pgsize_bitmap = SZ_4K;
+ domain->iommu_superpage = iommu_superpage_capability(iommu, first_stage);
+ domain->domain.pgsize_bitmap |= domain_super_pgsize_bitmap(domain);
+
+ /*
+ * IOVA aperture: First-level translation restricts the input-address
+ * to a canonical address (i.e., address bits 63:N have the same value
+ * as address bit [N-1], where N is 48-bits with 4-level paging and
+ * 57-bits with 5-level paging). Hence, skip bit [N-1].
+ */
+ domain->domain.geometry.force_aperture = true;
+ domain->domain.geometry.aperture_start = 0;
+ if (first_stage)
+ domain->domain.geometry.aperture_end = __DOMAIN_MAX_ADDR(domain->gaw - 1);
+ else
+ domain->domain.geometry.aperture_end = __DOMAIN_MAX_ADDR(domain->gaw);
+
+ /* always allocate the top pgd */
+ domain->pgd = iommu_alloc_page_node(domain->nid, GFP_KERNEL);
+ if (!domain->pgd) {
+ kfree(domain);
+ return ERR_PTR(-ENOMEM);
+ }
+ domain_flush_cache(domain, domain->pgd, PAGE_SIZE);
+
+ return domain;
+}
+
static struct iommu_domain *intel_iommu_domain_alloc(unsigned type)
{
struct dmar_domain *dmar_domain;
@@ -3695,15 +3768,11 @@ intel_iommu_domain_alloc_user(struct device *dev, u32 flags,
if (user_data || (dirty_tracking && !ssads_supported(iommu)))
return ERR_PTR(-EOPNOTSUPP);
- /*
- * domain_alloc_user op needs to fully initialize a domain before
- * return, so uses iommu_domain_alloc() here for simple.
- */
- domain = iommu_domain_alloc(dev->bus);
- if (!domain)
- return ERR_PTR(-ENOMEM);
-
- dmar_domain = to_dmar_domain(domain);
+ /* Do not use first stage for user domain translation. */
+ dmar_domain = paging_domain_alloc(dev, false);
+ if (IS_ERR(dmar_domain))
+ return ERR_CAST(dmar_domain);
+ domain = &dmar_domain->domain;
if (nested_parent) {
dmar_domain->nested_parent = true;
--
2.34.1
On 2024/5/29 13:32, Lu Baolu wrote:
> Commit <17de3f5fdd35> ("iommu: Retire bus ops") removes iommu ops from
> bus. The iommu subsystem no longer relies on bus for operations. So the
> bus parameter in iommu_domain_alloc() is no longer relevant.
>
> Add a new interface named iommu_paging_domain_alloc(), which explicitly
> indicates the allocation of a paging domain for DMA managed by a kernel
> driver. The new interface takes a device pointer as its parameter, that
> better aligns with the current iommu subsystem.
you may want to move this patch before patch 03/04.
> Signed-off-by: Lu Baolu <[email protected]>
> ---
> include/linux/iommu.h | 6 ++++++
> drivers/iommu/iommu.c | 20 ++++++++++++++++++++
> 2 files changed, 26 insertions(+)
>
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index 6648b2415474..16401de7802d 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -781,6 +781,7 @@ extern bool device_iommu_capable(struct device *dev, enum iommu_cap cap);
> extern bool iommu_group_has_isolated_msi(struct iommu_group *group);
> extern struct iommu_domain *iommu_domain_alloc(const struct bus_type *bus);
> struct iommu_domain *iommu_user_domain_alloc(struct device *dev, u32 flags);
> +struct iommu_domain *iommu_paging_domain_alloc(struct device *dev);
> extern void iommu_domain_free(struct iommu_domain *domain);
> extern int iommu_attach_device(struct iommu_domain *domain,
> struct device *dev);
> @@ -1092,6 +1093,11 @@ static inline struct iommu_domain *iommu_user_domain_alloc(struct device *dev, u
> return ERR_PTR(-ENODEV);
> }
>
> +static inline struct iommu_domain *iommu_paging_domain_alloc(struct device *dev)
> +{
> + return ERR_PTR(-ENODEV);
> +}
> +
> static inline void iommu_domain_free(struct iommu_domain *domain)
> {
> }
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index f1416892ef8e..7df4a021b040 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -2016,6 +2016,10 @@ static int __iommu_domain_alloc_dev(struct device *dev, void *data)
> return 0;
> }
>
> +/*
> + * The iommu ops in bus has been retired. Do not use this interface in
> + * new drivers.
> + */
> struct iommu_domain *iommu_domain_alloc(const struct bus_type *bus)
> {
> const struct iommu_ops *ops = NULL;
> @@ -2074,6 +2078,22 @@ struct iommu_domain *iommu_user_domain_alloc(struct device *dev, u32 flags)
> }
> EXPORT_SYMBOL_GPL(iommu_user_domain_alloc);
>
> +/**
> + * iommu_paging_domain_alloc() - Allocate a paging domain
> + * @dev: device for which the domain is allocated
> + *
> + * Allocate a paging domain which will be managed by a kernel driver. Return
> + * allocated domain if successful, or a ERR pointer for failure.
> + */
> +struct iommu_domain *iommu_paging_domain_alloc(struct device *dev)
> +{
> + if (!dev_has_iommu(dev))
> + return ERR_PTR(-ENODEV);
> +
> + return __iommu_domain_alloc(dev_iommu_ops(dev), dev, IOMMU_DOMAIN_UNMANAGED);
> +}
> +EXPORT_SYMBOL_GPL(iommu_paging_domain_alloc);
> +
> void iommu_domain_free(struct iommu_domain *domain)
> {
> if (domain->type == IOMMU_DOMAIN_SVA)
--
Regards,
Yi Liu
On 2024/5/29 13:32, Lu Baolu wrote:
> The IOMMU subsystem has undergone some changes, including the removal
> of iommu_ops from the bus structure. Consequently, the existing domain
> allocation interface, which relies on a bus type argument, is no longer
> relevant:
>
> struct iommu_domain *iommu_domain_alloc(struct bus_type *bus)
>
> This series is designed to refactor the use of this interface. It
> proposes two new interfaces to replace iommu_domain_alloc():
>
> - iommu_user_domain_alloc(): This interface is intended for allocating
> iommu domains managed by userspace for device passthrough scenarios,
> such as those used by iommufd, vfio, and vdpa. It clearly indicates
> that the domain is for user-managed device DMA.
user paging domain? It looks to me user domain includes the nested domains
as well.
> If an IOMMU driver does not implement iommu_ops->domain_alloc_user,
> this interface will rollback to the generic paging domain allocation.
>
> - iommu_paging_domain_alloc(): This interface is for allocating iommu
> domains managed by kernel drivers for kernel DMA purposes. It takes a
> device pointer as a parameter, which better reflects the current
> design of the IOMMU subsystem.
>
> The majority of device drivers currently using iommu_domain_alloc() do
> so to allocate a domain for a specific device and then attach that
> domain to the device. These cases can be straightforwardly migrated to
> the new interfaces.
>
> However, there are some drivers with more complex use cases that do
> not fit neatly into this new scheme. For example:
>
> $ git grep "= iommu_domain_alloc"
> arch/arm/mm/dma-mapping.c: mapping->domain = iommu_domain_alloc(bus);
> drivers/gpu/drm/rockchip/rockchip_drm_drv.c: private->domain = iommu_domain_alloc(private->iommu_dev->bus);
> drivers/gpu/drm/tegra/drm.c: tegra->domain = iommu_domain_alloc(&platform_bus_type);
> drivers/infiniband/hw/usnic/usnic_uiom.c: pd->domain = domain = iommu_domain_alloc(dev->bus);
>
> This series leave those cases unchanged and keep iommu_domain_alloc()
> for their usage. But new drivers should not use it anymore.
does it mean there is still domains allocated via iommu_domain_alloc()
on VT-d platform?
> The whole series is also available on GitHub:
> https://github.com/LuBaolu/intel-iommu/commits/iommu-domain-allocation-refactor-v1
>
> Lu Baolu (20):
> iommu: Add iommu_user_domain_alloc() interface
> iommufd: Use iommu_user_domain_alloc()
> vfio/type1: Use iommu_paging_domain_alloc()
> vhost-vdpa: Use iommu_user_domain_alloc()
> iommu: Add iommu_paging_domain_alloc() interface
> drm/msm: Use iommu_paging_domain_alloc()
> drm/nouveau/tegra: Use iommu_paging_domain_alloc()
> gpu: host1x: Use iommu_paging_domain_alloc()
> media: nvidia: tegra: Use iommu_paging_domain_alloc()
> media: venus: firmware: Use iommu_paging_domain_alloc()
> ath10k: Use iommu_paging_domain_alloc()
> wifi: ath11k: Use iommu_paging_domain_alloc()
> remoteproc: Use iommu_paging_domain_alloc()
> soc/fsl/qbman: Use iommu_paging_domain_alloc()
> iommu/vt-d: Add helper to allocate paging domain
> iommu/vt-d: Add domain_alloc_paging support
> iommu/vt-d: Simplify compatibility check for identity domain
> iommu/vt-d: Enhance compatibility check for paging domain attach
> iommu/vt-d: Remove domain_update_iommu_cap()
> iommu/vt-d: Remove domain_update_iommu_superpage()
>
> include/linux/iommu.h | 12 +
> drivers/gpu/drm/msm/msm_iommu.c | 8 +-
> .../drm/nouveau/nvkm/engine/device/tegra.c | 4 +-
> drivers/gpu/host1x/dev.c | 6 +-
> drivers/iommu/intel/iommu.c | 319 ++++++++----------
> drivers/iommu/intel/pasid.c | 28 +-
> drivers/iommu/iommu.c | 62 ++++
> drivers/iommu/iommufd/hw_pagetable.c | 20 +-
> .../media/platform/nvidia/tegra-vde/iommu.c | 6 +-
> drivers/media/platform/qcom/venus/firmware.c | 6 +-
> drivers/net/wireless/ath/ath10k/snoc.c | 6 +-
> drivers/net/wireless/ath/ath11k/ahb.c | 6 +-
> drivers/remoteproc/remoteproc_core.c | 6 +-
> drivers/soc/fsl/qbman/qman_portal.c | 4 +-
> drivers/vfio/vfio_iommu_type1.c | 7 +-
> drivers/vhost/vdpa.c | 11 +-
> 16 files changed, 253 insertions(+), 258 deletions(-)
>
--
Regards,
Yi Liu
The driver now supports domain_alloc_paging, ensuring that a valid device
pointer is provided whenever a paging domain is allocated. Additionally,
the dmar_domain attributes are set up at the time of allocation.
Consistent with the established semantics in the IOMMU core, if a domain is
attached to a device and found to be incompatible with the IOMMU hardware
capabilities, the operation will return an -EINVAL error. This implicitly
advises the caller to allocate a new domain for the device and attempt the
domain attachment again.
Signed-off-by: Lu Baolu <[email protected]>
---
drivers/iommu/intel/iommu.c | 39 ++++++++++++++++++-------------------
drivers/iommu/intel/pasid.c | 28 +-------------------------
2 files changed, 20 insertions(+), 47 deletions(-)
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 693a6d7c79ed..e9393f5c2c50 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -3826,27 +3826,26 @@ int prepare_domain_attach_device(struct iommu_domain *domain,
if (domain->dirty_ops && !ssads_supported(iommu))
return -EINVAL;
- /* check if this iommu agaw is sufficient for max mapped address */
- addr_width = agaw_to_width(iommu->agaw);
- if (addr_width > cap_mgaw(iommu->cap))
- addr_width = cap_mgaw(iommu->cap);
-
- if (dmar_domain->max_addr > (1LL << addr_width))
+ if (dmar_domain->iommu_coherency !=
+ iommu_paging_structure_coherency(iommu))
return -EINVAL;
- dmar_domain->gaw = addr_width;
-
- /*
- * Knock out extra levels of page tables if necessary
- */
- while (iommu->agaw < dmar_domain->agaw) {
- struct dma_pte *pte;
-
- pte = dmar_domain->pgd;
- if (dma_pte_present(pte)) {
- dmar_domain->pgd = phys_to_virt(dma_pte_addr(pte));
- iommu_free_page(pte);
- }
- dmar_domain->agaw--;
+
+ if (domain->type & __IOMMU_DOMAIN_PAGING) {
+ if (dmar_domain->iommu_superpage !=
+ iommu_superpage_capability(iommu, dmar_domain->use_first_level))
+ return -EINVAL;
+
+ if (dmar_domain->use_first_level &&
+ (!sm_supported(iommu) || !ecap_flts(iommu->ecap)))
+ return -EINVAL;
+
+ /* check if this iommu agaw is sufficient for max mapped address */
+ addr_width = agaw_to_width(iommu->agaw);
+ if (addr_width > cap_mgaw(iommu->cap))
+ addr_width = cap_mgaw(iommu->cap);
+
+ if (dmar_domain->gaw > addr_width || dmar_domain->agaw > iommu->agaw)
+ return -EINVAL;
}
if (sm_supported(iommu) && !dev_is_real_dma_subdevice(dev) &&
diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c
index abce19e2ad6f..573e1b8e3cfb 100644
--- a/drivers/iommu/intel/pasid.c
+++ b/drivers/iommu/intel/pasid.c
@@ -345,25 +345,6 @@ int intel_pasid_setup_first_level(struct intel_iommu *iommu,
return 0;
}
-/*
- * Skip top levels of page tables for iommu which has less agaw
- * than default. Unnecessary for PT mode.
- */
-static int iommu_skip_agaw(struct dmar_domain *domain,
- struct intel_iommu *iommu,
- struct dma_pte **pgd)
-{
- int agaw;
-
- for (agaw = domain->agaw; agaw > iommu->agaw; agaw--) {
- *pgd = phys_to_virt(dma_pte_addr(*pgd));
- if (!dma_pte_present(*pgd))
- return -EINVAL;
- }
-
- return agaw;
-}
-
/*
* Set up the scalable mode pasid entry for second only translation type.
*/
@@ -374,7 +355,6 @@ int intel_pasid_setup_second_level(struct intel_iommu *iommu,
struct pasid_entry *pte;
struct dma_pte *pgd;
u64 pgd_val;
- int agaw;
u16 did;
/*
@@ -388,12 +368,6 @@ int intel_pasid_setup_second_level(struct intel_iommu *iommu,
}
pgd = domain->pgd;
- agaw = iommu_skip_agaw(domain, iommu, &pgd);
- if (agaw < 0) {
- dev_err(dev, "Invalid domain page table\n");
- return -EINVAL;
- }
-
pgd_val = virt_to_phys(pgd);
did = domain_id_iommu(domain, iommu);
@@ -412,7 +386,7 @@ int intel_pasid_setup_second_level(struct intel_iommu *iommu,
pasid_clear_entry(pte);
pasid_set_domain_id(pte, did);
pasid_set_slptr(pte, pgd_val);
- pasid_set_address_width(pte, agaw);
+ pasid_set_address_width(pte, domain->agaw);
pasid_set_translation_type(pte, PASID_ENTRY_PGTT_SL_ONLY);
pasid_set_fault_enable(pte);
pasid_set_page_snoop(pte, !!ecap_smpwc(iommu->ecap));
--
2.34.1
On 2024/5/29 17:03, Yi Liu wrote:
> On 2024/5/29 13:32, Lu Baolu wrote:
>> The IOMMU subsystem has undergone some changes, including the removal
>> of iommu_ops from the bus structure. Consequently, the existing domain
>> allocation interface, which relies on a bus type argument, is no longer
>> relevant:
>>
>> struct iommu_domain *iommu_domain_alloc(struct bus_type *bus)
>>
>> This series is designed to refactor the use of this interface. It
>> proposes two new interfaces to replace iommu_domain_alloc():
>>
>> - iommu_user_domain_alloc(): This interface is intended for allocating
>> iommu domains managed by userspace for device passthrough scenarios,
>> such as those used by iommufd, vfio, and vdpa. It clearly indicates
>> that the domain is for user-managed device DMA.
>
> user paging domain? It looks to me user domain includes the nested domains
> as well.
Yes, nested domain is a user domain. The iommu driver should implement
iommu_ops->domain_alloc_user for nested domain allocation.
>
>> If an IOMMU driver does not implement iommu_ops->domain_alloc_user,
>> this interface will rollback to the generic paging domain allocation.
>>
>> - iommu_paging_domain_alloc(): This interface is for allocating iommu
>> domains managed by kernel drivers for kernel DMA purposes. It takes a
>> device pointer as a parameter, which better reflects the current
>> design of the IOMMU subsystem.
>>
>> The majority of device drivers currently using iommu_domain_alloc() do
>> so to allocate a domain for a specific device and then attach that
>> domain to the device. These cases can be straightforwardly migrated to
>> the new interfaces.
>>
>> However, there are some drivers with more complex use cases that do
>> not fit neatly into this new scheme. For example:
>>
>> $ git grep "= iommu_domain_alloc"
>> arch/arm/mm/dma-mapping.c: mapping->domain =
>> iommu_domain_alloc(bus);
>> drivers/gpu/drm/rockchip/rockchip_drm_drv.c: private->domain =
>> iommu_domain_alloc(private->iommu_dev->bus);
>> drivers/gpu/drm/tegra/drm.c: tegra->domain =
>> iommu_domain_alloc(&platform_bus_type);
>> drivers/infiniband/hw/usnic/usnic_uiom.c: pd->domain = domain =
>> iommu_domain_alloc(dev->bus);
>>
>> This series leave those cases unchanged and keep iommu_domain_alloc()
>> for their usage. But new drivers should not use it anymore.
>
> does it mean there is still domains allocated via iommu_domain_alloc()
> on VT-d platform?
I think the drivers mentioned above do not run on x86 platforms, or do
they?
Best regards,
baolu
On 5/29/24 4:21 PM, Dmitry Baryshkov wrote:
> On Wed, May 29, 2024 at 01:32:36PM +0800, Lu Baolu wrote:
>> The domain allocated in msm_iommu_new() is for the @dev. Replace
>> iommu_domain_alloc() with iommu_paging_domain_alloc() to make it explicit.
>>
>> Update msm_iommu_new() to always return ERR_PTR in failure cases instead
>> of NULL.
> Please don't mix unrelated changes, because ...
>
>> Signed-off-by: Lu Baolu<[email protected]>
>> ---
>> drivers/gpu/drm/msm/msm_iommu.c | 8 ++++----
>> 1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/msm_iommu.c b/drivers/gpu/drm/msm/msm_iommu.c
>> index d5512037c38b..f7e28d4b5f62 100644
>> --- a/drivers/gpu/drm/msm/msm_iommu.c
>> +++ b/drivers/gpu/drm/msm/msm_iommu.c
>> @@ -407,9 +407,9 @@ struct msm_mmu *msm_iommu_new(struct device *dev, unsigned long quirks)
>> struct msm_iommu *iommu;
>> int ret;
>>
>> - domain = iommu_domain_alloc(dev->bus);
>> - if (!domain)
>> - return NULL;
>> + domain = iommu_paging_domain_alloc(dev);
>> + if (IS_ERR(domain))
>> + return ERR_CAST(domain);
>>
>> iommu_set_pgtable_quirks(domain, quirks);
>>
>> @@ -441,7 +441,7 @@ struct msm_mmu *msm_iommu_gpu_new(struct device *dev, struct msm_gpu *gpu, unsig
>> struct msm_mmu *mmu;
>>
>> mmu = msm_iommu_new(dev, quirks);
>> - if (IS_ERR_OR_NULL(mmu))
>> + if (IS_ERR(mmu))
>> return mmu;
> NAK, not having an IOMMU is a poor but legit usecase for some of devices
> which don't have IOMMU support yet (for example because of the buggy
> implementation for which we were not able to get all the hooks in).
>
> Please don't break compatibility for existing platforms.
Sure. I will remove this line of change. Though I have no idea in which
case msm_iommu_new() could return NULL after this patch.
Best regards,
baolu
On 5/29/24 5:04 PM, Yi Liu wrote:
> On 2024/5/29 13:32, Lu Baolu wrote:
>> Commit <17de3f5fdd35> ("iommu: Retire bus ops") removes iommu ops from
>> bus. The iommu subsystem no longer relies on bus for operations. So the
>> bus parameter in iommu_domain_alloc() is no longer relevant.
>>
>> Add a new interface named iommu_paging_domain_alloc(), which explicitly
>> indicates the allocation of a paging domain for DMA managed by a kernel
>> driver. The new interface takes a device pointer as its parameter, that
>> better aligns with the current iommu subsystem.
>
> you may want to move this patch before patch 03/04.
Emm, why?
Best regards,
baolu
On 5/30/24 9:59 AM, Baolu Lu wrote:
> On 5/29/24 5:04 PM, Yi Liu wrote:
>> On 2024/5/29 13:32, Lu Baolu wrote:
>>> Commit <17de3f5fdd35> ("iommu: Retire bus ops") removes iommu ops from
>>> bus. The iommu subsystem no longer relies on bus for operations. So the
>>> bus parameter in iommu_domain_alloc() is no longer relevant.
>>>
>>> Add a new interface named iommu_paging_domain_alloc(), which explicitly
>>> indicates the allocation of a paging domain for DMA managed by a kernel
>>> driver. The new interface takes a device pointer as its parameter, that
>>> better aligns with the current iommu subsystem.
>>
>> you may want to move this patch before patch 03/04.
>
> Emm, why?
I see. The commit subject is misleading. It should be "vfio/type1: Use
iommu_user_domain_alloc()".
Best regards,
baolu
On Thu, 30 May 2024 at 04:59, Baolu Lu <[email protected]> wrote:
>
> On 5/29/24 4:21 PM, Dmitry Baryshkov wrote:
> > On Wed, May 29, 2024 at 01:32:36PM +0800, Lu Baolu wrote:
> >> The domain allocated in msm_iommu_new() is for the @dev. Replace
> >> iommu_domain_alloc() with iommu_paging_domain_alloc() to make it explicit.
> >>
> >> Update msm_iommu_new() to always return ERR_PTR in failure cases instead
> >> of NULL.
> > Please don't mix unrelated changes, because ...
> >
> >> Signed-off-by: Lu Baolu<[email protected]>
> >> ---
> >> drivers/gpu/drm/msm/msm_iommu.c | 8 ++++----
> >> 1 file changed, 4 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/msm/msm_iommu.c b/drivers/gpu/drm/msm/msm_iommu.c
> >> index d5512037c38b..f7e28d4b5f62 100644
> >> --- a/drivers/gpu/drm/msm/msm_iommu.c
> >> +++ b/drivers/gpu/drm/msm/msm_iommu.c
> >> @@ -407,9 +407,9 @@ struct msm_mmu *msm_iommu_new(struct device *dev, unsigned long quirks)
> >> struct msm_iommu *iommu;
> >> int ret;
> >>
> >> - domain = iommu_domain_alloc(dev->bus);
> >> - if (!domain)
> >> - return NULL;
> >> + domain = iommu_paging_domain_alloc(dev);
> >> + if (IS_ERR(domain))
> >> + return ERR_CAST(domain);
> >>
> >> iommu_set_pgtable_quirks(domain, quirks);
> >>
> >> @@ -441,7 +441,7 @@ struct msm_mmu *msm_iommu_gpu_new(struct device *dev, struct msm_gpu *gpu, unsig
> >> struct msm_mmu *mmu;
> >>
> >> mmu = msm_iommu_new(dev, quirks);
> >> - if (IS_ERR_OR_NULL(mmu))
> >> + if (IS_ERR(mmu))
> >> return mmu;
> > NAK, not having an IOMMU is a poor but legit usecase for some of devices
> > which don't have IOMMU support yet (for example because of the buggy
> > implementation for which we were not able to get all the hooks in).
> >
> > Please don't break compatibility for existing platforms.
>
> Sure. I will remove this line of change. Though I have no idea in which
> case msm_iommu_new() could return NULL after this patch.
So, even without this chunk you are going to break the no-IOMMU case.
Please don't. This will result in a regression report and a revert.
Instead please provide a way for the existing drivers to continue
working. For example, something like:
if (IS_ERR(mmu) && ERR_PTR(mmu) == -ENODEV))
return NULL;
--
With best wishes
Dmitry
On 29/05/2024 6:32 am, Lu Baolu wrote:
> The IOMMU subsystem has undergone some changes, including the removal
> of iommu_ops from the bus structure. Consequently, the existing domain
> allocation interface, which relies on a bus type argument, is no longer
> relevant:
>
> struct iommu_domain *iommu_domain_alloc(struct bus_type *bus)
>
> This series is designed to refactor the use of this interface. It
> proposes two new interfaces to replace iommu_domain_alloc():
>
> - iommu_user_domain_alloc(): This interface is intended for allocating
> iommu domains managed by userspace for device passthrough scenarios,
> such as those used by iommufd, vfio, and vdpa. It clearly indicates
> that the domain is for user-managed device DMA.
>
> If an IOMMU driver does not implement iommu_ops->domain_alloc_user,
> this interface will rollback to the generic paging domain allocation.
>
> - iommu_paging_domain_alloc(): This interface is for allocating iommu
> domains managed by kernel drivers for kernel DMA purposes. It takes a
> device pointer as a parameter, which better reflects the current
> design of the IOMMU subsystem.
>
> The majority of device drivers currently using iommu_domain_alloc() do
> so to allocate a domain for a specific device and then attach that
> domain to the device. These cases can be straightforwardly migrated to
> the new interfaces.
Ooh, nice! This was rising back up my to-do list as well, but I concur
it's rather more straightforward than my version that did devious things
to keep the iommu_domain_alloc() name...
> However, there are some drivers with more complex use cases that do
> not fit neatly into this new scheme. For example:
>
> $ git grep "= iommu_domain_alloc"
> arch/arm/mm/dma-mapping.c: mapping->domain = iommu_domain_alloc(bus);
This one's simple enough, the refactor just needs to go one step deeper.
I've just rebased and pushed my old patch for that, if you'd like it [1].
> drivers/gpu/drm/rockchip/rockchip_drm_drv.c: private->domain = iommu_domain_alloc(private->iommu_dev->bus);
Both this one and usnic_uiom_alloc_pd() should be OK - back when I did
all the figuring out to clean up iommu_present(), I specifically
reworked them into "dev->bus" style as a reminder that it *is* supposed
to be the right device for doing this with, even if the attach is a bit
more distant.
> drivers/gpu/drm/tegra/drm.c: tegra->domain = iommu_domain_alloc(&platform_bus_type);
This is the tricky one, where the device to hand may *not* be the right
device for IOMMU API use [2]. FWIW my plan was to pull the "walk the
platform bus to find any IOMMU-mapped device" trick into this code and
use it both to remove the final iommu_present() and for a device-based
domain allocation.
> drivers/infiniband/hw/usnic/usnic_uiom.c: pd->domain = domain = iommu_domain_alloc(dev->bus);
>
> This series leave those cases unchanged and keep iommu_domain_alloc()
> for their usage. But new drivers should not use it anymore.
I'd certainly be keen for it to be gone ASAP, since I'm seeing
increasing demand for supporting multiple IOMMU drivers, and this is the
last bus-based thing standing in the way of that.
Thanks,
Robin.
[1]
https://gitlab.arm.com/linux-arm/linux-rm/-/commit/f048cc6a323d8641898025ca96071df7cbe8bd52
[2]
https://lore.kernel.org/linux-iommu/[email protected]/
> The whole series is also available on GitHub:
> https://github.com/LuBaolu/intel-iommu/commits/iommu-domain-allocation-refactor-v1
>
> Lu Baolu (20):
> iommu: Add iommu_user_domain_alloc() interface
> iommufd: Use iommu_user_domain_alloc()
> vfio/type1: Use iommu_paging_domain_alloc()
> vhost-vdpa: Use iommu_user_domain_alloc()
> iommu: Add iommu_paging_domain_alloc() interface
> drm/msm: Use iommu_paging_domain_alloc()
> drm/nouveau/tegra: Use iommu_paging_domain_alloc()
> gpu: host1x: Use iommu_paging_domain_alloc()
> media: nvidia: tegra: Use iommu_paging_domain_alloc()
> media: venus: firmware: Use iommu_paging_domain_alloc()
> ath10k: Use iommu_paging_domain_alloc()
> wifi: ath11k: Use iommu_paging_domain_alloc()
> remoteproc: Use iommu_paging_domain_alloc()
> soc/fsl/qbman: Use iommu_paging_domain_alloc()
> iommu/vt-d: Add helper to allocate paging domain
> iommu/vt-d: Add domain_alloc_paging support
> iommu/vt-d: Simplify compatibility check for identity domain
> iommu/vt-d: Enhance compatibility check for paging domain attach
> iommu/vt-d: Remove domain_update_iommu_cap()
> iommu/vt-d: Remove domain_update_iommu_superpage()
>
> include/linux/iommu.h | 12 +
> drivers/gpu/drm/msm/msm_iommu.c | 8 +-
> .../drm/nouveau/nvkm/engine/device/tegra.c | 4 +-
> drivers/gpu/host1x/dev.c | 6 +-
> drivers/iommu/intel/iommu.c | 319 ++++++++----------
> drivers/iommu/intel/pasid.c | 28 +-
> drivers/iommu/iommu.c | 62 ++++
> drivers/iommu/iommufd/hw_pagetable.c | 20 +-
> .../media/platform/nvidia/tegra-vde/iommu.c | 6 +-
> drivers/media/platform/qcom/venus/firmware.c | 6 +-
> drivers/net/wireless/ath/ath10k/snoc.c | 6 +-
> drivers/net/wireless/ath/ath11k/ahb.c | 6 +-
> drivers/remoteproc/remoteproc_core.c | 6 +-
> drivers/soc/fsl/qbman/qman_portal.c | 4 +-
> drivers/vfio/vfio_iommu_type1.c | 7 +-
> drivers/vhost/vdpa.c | 11 +-
> 16 files changed, 253 insertions(+), 258 deletions(-)
>
On 2024/5/29 20:02, Baolu Lu wrote:
> On 2024/5/29 17:03, Yi Liu wrote:
>> On 2024/5/29 13:32, Lu Baolu wrote:
>>> The IOMMU subsystem has undergone some changes, including the removal
>>> of iommu_ops from the bus structure. Consequently, the existing domain
>>> allocation interface, which relies on a bus type argument, is no longer
>>> relevant:
>>>
>>> struct iommu_domain *iommu_domain_alloc(struct bus_type *bus)
>>>
>>> This series is designed to refactor the use of this interface. It
>>> proposes two new interfaces to replace iommu_domain_alloc():
>>>
>>> - iommu_user_domain_alloc(): This interface is intended for allocating
>>> iommu domains managed by userspace for device passthrough scenarios,
>>> such as those used by iommufd, vfio, and vdpa. It clearly indicates
>>> that the domain is for user-managed device DMA.
>>
>> user paging domain? It looks to me user domain includes the nested domains
>> as well.
>
> Yes, nested domain is a user domain. The iommu driver should implement
> iommu_ops->domain_alloc_user for nested domain allocation.
will it be more clear to name iommu_user_domain_alloc() be
iommu_user_paging_domain_alloc() as it is mainly for paging domain
allocation?
>>
>>> If an IOMMU driver does not implement iommu_ops->domain_alloc_user,
>>> this interface will rollback to the generic paging domain allocation.
>>>
>>> - iommu_paging_domain_alloc(): This interface is for allocating iommu
>>> domains managed by kernel drivers for kernel DMA purposes. It takes a
>>> device pointer as a parameter, which better reflects the current
>>> design of the IOMMU subsystem.
>>>
>>> The majority of device drivers currently using iommu_domain_alloc() do
>>> so to allocate a domain for a specific device and then attach that
>>> domain to the device. These cases can be straightforwardly migrated to
>>> the new interfaces.
>>>
>>> However, there are some drivers with more complex use cases that do
>>> not fit neatly into this new scheme. For example:
>>>
>>> $ git grep "= iommu_domain_alloc"
>>> arch/arm/mm/dma-mapping.c: mapping->domain = iommu_domain_alloc(bus);
>>> drivers/gpu/drm/rockchip/rockchip_drm_drv.c: private->domain =
>>> iommu_domain_alloc(private->iommu_dev->bus);
>>> drivers/gpu/drm/tegra/drm.c: tegra->domain =
>>> iommu_domain_alloc(&platform_bus_type);
>>> drivers/infiniband/hw/usnic/usnic_uiom.c: pd->domain = domain =
>>> iommu_domain_alloc(dev->bus);
>>>
>>> This series leave those cases unchanged and keep iommu_domain_alloc()
>>> for their usage. But new drivers should not use it anymore.
>>
>> does it mean there is still domains allocated via iommu_domain_alloc()
>> on VT-d platform?
>
> I think the drivers mentioned above do not run on x86 platforms, or do
> they?
cool. BTW. I know out-of-tree drivers are not counted in upstream review.
Just out of curious, is there a formal way to let such drivers know it is
no longer allowed to use iommu_domain_alloc() on VT-d?
--
Regards,
Yi Liu
On 5/30/24 3:58 PM, Dmitry Baryshkov wrote:
> On Thu, 30 May 2024 at 04:59, Baolu Lu<[email protected]> wrote:
>> On 5/29/24 4:21 PM, Dmitry Baryshkov wrote:
>>> On Wed, May 29, 2024 at 01:32:36PM +0800, Lu Baolu wrote:
>>>> The domain allocated in msm_iommu_new() is for the @dev. Replace
>>>> iommu_domain_alloc() with iommu_paging_domain_alloc() to make it explicit.
>>>>
>>>> Update msm_iommu_new() to always return ERR_PTR in failure cases instead
>>>> of NULL.
>>> Please don't mix unrelated changes, because ...
>>>
>>>> Signed-off-by: Lu Baolu<[email protected]>
>>>> ---
>>>> drivers/gpu/drm/msm/msm_iommu.c | 8 ++++----
>>>> 1 file changed, 4 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/msm/msm_iommu.c b/drivers/gpu/drm/msm/msm_iommu.c
>>>> index d5512037c38b..f7e28d4b5f62 100644
>>>> --- a/drivers/gpu/drm/msm/msm_iommu.c
>>>> +++ b/drivers/gpu/drm/msm/msm_iommu.c
>>>> @@ -407,9 +407,9 @@ struct msm_mmu *msm_iommu_new(struct device *dev, unsigned long quirks)
>>>> struct msm_iommu *iommu;
>>>> int ret;
>>>>
>>>> - domain = iommu_domain_alloc(dev->bus);
>>>> - if (!domain)
>>>> - return NULL;
>>>> + domain = iommu_paging_domain_alloc(dev);
>>>> + if (IS_ERR(domain))
>>>> + return ERR_CAST(domain);
>>>>
>>>> iommu_set_pgtable_quirks(domain, quirks);
>>>>
>>>> @@ -441,7 +441,7 @@ struct msm_mmu *msm_iommu_gpu_new(struct device *dev, struct msm_gpu *gpu, unsig
>>>> struct msm_mmu *mmu;
>>>>
>>>> mmu = msm_iommu_new(dev, quirks);
>>>> - if (IS_ERR_OR_NULL(mmu))
>>>> + if (IS_ERR(mmu))
>>>> return mmu;
>>> NAK, not having an IOMMU is a poor but legit usecase for some of devices
>>> which don't have IOMMU support yet (for example because of the buggy
>>> implementation for which we were not able to get all the hooks in).
>>>
>>> Please don't break compatibility for existing platforms.
>> Sure. I will remove this line of change. Though I have no idea in which
>> case msm_iommu_new() could return NULL after this patch.
> So, even without this chunk you are going to break the no-IOMMU case.
> Please don't. This will result in a regression report and a revert.
>
> Instead please provide a way for the existing drivers to continue
> working. For example, something like:
>
> if (IS_ERR(mmu) && ERR_PTR(mmu) == -ENODEV))
> return NULL;
Oh I see. msm_iommu_new() returning NULL indicates a no-IOMMU case,
right? So perhaps we can make it explicit like below?
if (!device_iommu_mapped(dev))
return NULL;
domain = iommu_paging_domain_alloc(dev);
if (IS_ERR(domain))
return ERR_CAST(domain);
Best regards,
baolu
On 5/31/24 1:59 AM, Robin Murphy wrote:
> On 29/05/2024 6:32 am, Lu Baolu wrote:
>> The IOMMU subsystem has undergone some changes, including the removal
>> of iommu_ops from the bus structure. Consequently, the existing domain
>> allocation interface, which relies on a bus type argument, is no longer
>> relevant:
>>
>> struct iommu_domain *iommu_domain_alloc(struct bus_type *bus)
>>
>> This series is designed to refactor the use of this interface. It
>> proposes two new interfaces to replace iommu_domain_alloc():
>>
>> - iommu_user_domain_alloc(): This interface is intended for allocating
>> iommu domains managed by userspace for device passthrough scenarios,
>> such as those used by iommufd, vfio, and vdpa. It clearly indicates
>> that the domain is for user-managed device DMA.
>>
>> If an IOMMU driver does not implement iommu_ops->domain_alloc_user,
>> this interface will rollback to the generic paging domain allocation.
>>
>> - iommu_paging_domain_alloc(): This interface is for allocating iommu
>> domains managed by kernel drivers for kernel DMA purposes. It takes a
>> device pointer as a parameter, which better reflects the current
>> design of the IOMMU subsystem.
>>
>> The majority of device drivers currently using iommu_domain_alloc() do
>> so to allocate a domain for a specific device and then attach that
>> domain to the device. These cases can be straightforwardly migrated to
>> the new interfaces.
>
> Ooh, nice! This was rising back up my to-do list as well, but I concur
> it's rather more straightforward than my version that did devious things
> to keep the iommu_domain_alloc() name...
>
>> However, there are some drivers with more complex use cases that do
>> not fit neatly into this new scheme. For example:
>>
>> $ git grep "= iommu_domain_alloc"
>> arch/arm/mm/dma-mapping.c: mapping->domain =
>> iommu_domain_alloc(bus);
>
> This one's simple enough, the refactor just needs to go one step deeper.
> I've just rebased and pushed my old patch for that, if you'd like it [1].
Great! With this change, we can safely replace iommu_domain_alloc().
diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
index 52f9c56cc3cb..88c2d68a69c9 100644
--- a/arch/arm/mm/dma-mapping.c
+++ b/arch/arm/mm/dma-mapping.c
@@ -1585,9 +1585,11 @@ arm_iommu_create_mapping(struct device *dev,
dma_addr_t base, u64 size)
spin_lock_init(&mapping->lock);
- mapping->domain = iommu_domain_alloc(dev->bus);
- if (!mapping->domain)
+ mapping->domain = iommu_paging_domain_alloc(dev);
+ if (IS_ERR(mapping->domain)) {
+ err = PTR_ERR(mapping->domain);
goto err4;
+ }
kref_init(&mapping->kref);
return mapping;
>
>> drivers/gpu/drm/rockchip/rockchip_drm_drv.c: private->domain =
>> iommu_domain_alloc(private->iommu_dev->bus);
>
> Both this one and usnic_uiom_alloc_pd() should be OK - back when I did
> all the figuring out to clean up iommu_present(), I specifically
> reworked them into "dev->bus" style as a reminder that it *is* supposed
> to be the right device for doing this with, even if the attach is a bit
> more distant.
Yeah! I will cleanup these two in the next version.
>
>> drivers/gpu/drm/tegra/drm.c: tegra->domain =
>> iommu_domain_alloc(&platform_bus_type);
>
> This is the tricky one, where the device to hand may *not* be the right
> device for IOMMU API use [2]. FWIW my plan was to pull the "walk the
> platform bus to find any IOMMU-mapped device" trick into this code and
> use it both to remove the final iommu_present() and for a device-based
> domain allocation.
I am not familiar with this driver, so the solution you mentioned above
is the best option I can think of for now. I will incorporate this into
the next version.
>
>> drivers/infiniband/hw/usnic/usnic_uiom.c: pd->domain = domain =
>> iommu_domain_alloc(dev->bus);
>>
>> This series leave those cases unchanged and keep iommu_domain_alloc()
>> for their usage. But new drivers should not use it anymore.
>
> I'd certainly be keen for it to be gone ASAP, since I'm seeing
> increasing demand for supporting multiple IOMMU drivers, and this is the
> last bus-based thing standing in the way of that.
Agreed. With all iommu_domain_alloc() removed, iommu_domain_alloc()
could be dropped.
>
> Thanks,
> Robin.
>
> [1]
> https://gitlab.arm.com/linux-arm/linux-rm/-/commit/f048cc6a323d8641898025ca96071df7cbe8bd52
> [2]
> https://lore.kernel.org/linux-iommu/[email protected]/
Best regards,
baolu
On 5/31/24 11:16 AM, Yi Liu wrote:
> On 2024/5/29 20:02, Baolu Lu wrote:
>> On 2024/5/29 17:03, Yi Liu wrote:
>>> On 2024/5/29 13:32, Lu Baolu wrote:
>>>> The IOMMU subsystem has undergone some changes, including the removal
>>>> of iommu_ops from the bus structure. Consequently, the existing domain
>>>> allocation interface, which relies on a bus type argument, is no longer
>>>> relevant:
>>>>
>>>> struct iommu_domain *iommu_domain_alloc(struct bus_type *bus)
>>>>
>>>> This series is designed to refactor the use of this interface. It
>>>> proposes two new interfaces to replace iommu_domain_alloc():
>>>>
>>>> - iommu_user_domain_alloc(): This interface is intended for allocating
>>>> iommu domains managed by userspace for device passthrough scenarios,
>>>> such as those used by iommufd, vfio, and vdpa. It clearly indicates
>>>> that the domain is for user-managed device DMA.
>>>
>>> user paging domain? It looks to me user domain includes the nested
>>> domains
>>> as well.
>>
>> Yes, nested domain is a user domain. The iommu driver should implement
>> iommu_ops->domain_alloc_user for nested domain allocation.
>
> will it be more clear to name iommu_user_domain_alloc() be
> iommu_user_paging_domain_alloc() as it is mainly for paging domain
> allocation?
That might be better; let's wait and see if there's another option.
>
>>>
>>>> If an IOMMU driver does not implement iommu_ops->domain_alloc_user,
>>>> this interface will rollback to the generic paging domain
>>>> allocation.
>>>>
>>>> - iommu_paging_domain_alloc(): This interface is for allocating iommu
>>>> domains managed by kernel drivers for kernel DMA purposes. It
>>>> takes a
>>>> device pointer as a parameter, which better reflects the current
>>>> design of the IOMMU subsystem.
>>>>
>>>> The majority of device drivers currently using iommu_domain_alloc() do
>>>> so to allocate a domain for a specific device and then attach that
>>>> domain to the device. These cases can be straightforwardly migrated to
>>>> the new interfaces.
>>>>
>>>> However, there are some drivers with more complex use cases that do
>>>> not fit neatly into this new scheme. For example:
>>>>
>>>> $ git grep "= iommu_domain_alloc"
>>>> arch/arm/mm/dma-mapping.c: mapping->domain =
>>>> iommu_domain_alloc(bus);
>>>> drivers/gpu/drm/rockchip/rockchip_drm_drv.c: private->domain =
>>>> iommu_domain_alloc(private->iommu_dev->bus);
>>>> drivers/gpu/drm/tegra/drm.c: tegra->domain =
>>>> iommu_domain_alloc(&platform_bus_type);
>>>> drivers/infiniband/hw/usnic/usnic_uiom.c: pd->domain = domain
>>>> = iommu_domain_alloc(dev->bus);
>>>>
>>>> This series leave those cases unchanged and keep iommu_domain_alloc()
>>>> for their usage. But new drivers should not use it anymore.
>>>
>>> does it mean there is still domains allocated via iommu_domain_alloc()
>>> on VT-d platform?
>>
>> I think the drivers mentioned above do not run on x86 platforms, or do
>> they?
>
> cool. BTW. I know out-of-tree drivers are not counted in upstream review.
> Just out of curious, is there a formal way to let such drivers know it is
> no longer allowed to use iommu_domain_alloc() on VT-d?
As Robin suggested, we should try to remove iommu_domain_alloc() from
the tree in this series.
Best regards,
baolu
On 2024/5/31 14:00, Baolu Lu wrote:
> On 5/31/24 11:16 AM, Yi Liu wrote:
>> On 2024/5/29 20:02, Baolu Lu wrote:
>>> On 2024/5/29 17:03, Yi Liu wrote:
>>>> On 2024/5/29 13:32, Lu Baolu wrote:
>>>>> The IOMMU subsystem has undergone some changes, including the removal
>>>>> of iommu_ops from the bus structure. Consequently, the existing domain
>>>>> allocation interface, which relies on a bus type argument, is no longer
>>>>> relevant:
>>>>>
>>>>> struct iommu_domain *iommu_domain_alloc(struct bus_type *bus)
>>>>>
>>>>> This series is designed to refactor the use of this interface. It
>>>>> proposes two new interfaces to replace iommu_domain_alloc():
>>>>>
>>>>> - iommu_user_domain_alloc(): This interface is intended for allocating
>>>>> iommu domains managed by userspace for device passthrough scenarios,
>>>>> such as those used by iommufd, vfio, and vdpa. It clearly indicates
>>>>> that the domain is for user-managed device DMA.
>>>>
>>>> user paging domain? It looks to me user domain includes the nested domains
>>>> as well.
>>>
>>> Yes, nested domain is a user domain. The iommu driver should implement
>>> iommu_ops->domain_alloc_user for nested domain allocation.
>>
>> will it be more clear to name iommu_user_domain_alloc() be
>> iommu_user_paging_domain_alloc() as it is mainly for paging domain
>> allocation?
>
> That might be better; let's wait and see if there's another option.
>
>>
>>>>
>>>>> If an IOMMU driver does not implement iommu_ops->domain_alloc_user,
>>>>> this interface will rollback to the generic paging domain allocation.
>>>>>
>>>>> - iommu_paging_domain_alloc(): This interface is for allocating iommu
>>>>> domains managed by kernel drivers for kernel DMA purposes. It takes a
>>>>> device pointer as a parameter, which better reflects the current
>>>>> design of the IOMMU subsystem.
>>>>>
>>>>> The majority of device drivers currently using iommu_domain_alloc() do
>>>>> so to allocate a domain for a specific device and then attach that
>>>>> domain to the device. These cases can be straightforwardly migrated to
>>>>> the new interfaces.
>>>>>
>>>>> However, there are some drivers with more complex use cases that do
>>>>> not fit neatly into this new scheme. For example:
>>>>>
>>>>> $ git grep "= iommu_domain_alloc"
>>>>> arch/arm/mm/dma-mapping.c: mapping->domain =
>>>>> iommu_domain_alloc(bus);
>>>>> drivers/gpu/drm/rockchip/rockchip_drm_drv.c: private->domain =
>>>>> iommu_domain_alloc(private->iommu_dev->bus);
>>>>> drivers/gpu/drm/tegra/drm.c: tegra->domain =
>>>>> iommu_domain_alloc(&platform_bus_type);
>>>>> drivers/infiniband/hw/usnic/usnic_uiom.c: pd->domain = domain =
>>>>> iommu_domain_alloc(dev->bus);
>>>>>
>>>>> This series leave those cases unchanged and keep iommu_domain_alloc()
>>>>> for their usage. But new drivers should not use it anymore.
>>>>
>>>> does it mean there is still domains allocated via iommu_domain_alloc()
>>>> on VT-d platform?
>>>
>>> I think the drivers mentioned above do not run on x86 platforms, or do
>>> they?
>>
>> cool. BTW. I know out-of-tree drivers are not counted in upstream review.
>> Just out of curious, is there a formal way to let such drivers know it is
>> no longer allowed to use iommu_domain_alloc() on VT-d?
>
> As Robin suggested, we should try to remove iommu_domain_alloc() from
> the tree in this series.
If it's completely dropped, that's fine. OOT drivers should fail in the
time of compiling.
--
Regards,
Yi Liu
On Fri, May 31, 2024 at 09:57:54AM +0800, Baolu Lu wrote:
> On 5/30/24 3:58 PM, Dmitry Baryshkov wrote:
> > On Thu, 30 May 2024 at 04:59, Baolu Lu<[email protected]> wrote:
> > > On 5/29/24 4:21 PM, Dmitry Baryshkov wrote:
> > > > On Wed, May 29, 2024 at 01:32:36PM +0800, Lu Baolu wrote:
> > > > > The domain allocated in msm_iommu_new() is for the @dev. Replace
> > > > > iommu_domain_alloc() with iommu_paging_domain_alloc() to make it explicit.
> > > > >
> > > > > Update msm_iommu_new() to always return ERR_PTR in failure cases instead
> > > > > of NULL.
> > > > Please don't mix unrelated changes, because ...
> > > >
> > > > > Signed-off-by: Lu Baolu<[email protected]>
> > > > > ---
> > > > > drivers/gpu/drm/msm/msm_iommu.c | 8 ++++----
> > > > > 1 file changed, 4 insertions(+), 4 deletions(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/msm/msm_iommu.c b/drivers/gpu/drm/msm/msm_iommu.c
> > > > > index d5512037c38b..f7e28d4b5f62 100644
> > > > > --- a/drivers/gpu/drm/msm/msm_iommu.c
> > > > > +++ b/drivers/gpu/drm/msm/msm_iommu.c
> > > > > @@ -407,9 +407,9 @@ struct msm_mmu *msm_iommu_new(struct device *dev, unsigned long quirks)
> > > > > struct msm_iommu *iommu;
> > > > > int ret;
> > > > >
> > > > > - domain = iommu_domain_alloc(dev->bus);
> > > > > - if (!domain)
> > > > > - return NULL;
> > > > > + domain = iommu_paging_domain_alloc(dev);
> > > > > + if (IS_ERR(domain))
> > > > > + return ERR_CAST(domain);
> > > > >
> > > > > iommu_set_pgtable_quirks(domain, quirks);
> > > > >
> > > > > @@ -441,7 +441,7 @@ struct msm_mmu *msm_iommu_gpu_new(struct device *dev, struct msm_gpu *gpu, unsig
> > > > > struct msm_mmu *mmu;
> > > > >
> > > > > mmu = msm_iommu_new(dev, quirks);
> > > > > - if (IS_ERR_OR_NULL(mmu))
> > > > > + if (IS_ERR(mmu))
> > > > > return mmu;
> > > > NAK, not having an IOMMU is a poor but legit usecase for some of devices
> > > > which don't have IOMMU support yet (for example because of the buggy
> > > > implementation for which we were not able to get all the hooks in).
> > > >
> > > > Please don't break compatibility for existing platforms.
> > > Sure. I will remove this line of change. Though I have no idea in which
> > > case msm_iommu_new() could return NULL after this patch.
> > So, even without this chunk you are going to break the no-IOMMU case.
> > Please don't. This will result in a regression report and a revert.
> >
> > Instead please provide a way for the existing drivers to continue
> > working. For example, something like:
> >
> > if (IS_ERR(mmu) && ERR_PTR(mmu) == -ENODEV))
> > return NULL;
>
> Oh I see. msm_iommu_new() returning NULL indicates a no-IOMMU case,
> right? So perhaps we can make it explicit like below?
>
> if (!device_iommu_mapped(dev))
> return NULL;
>
> domain = iommu_paging_domain_alloc(dev);
> if (IS_ERR(domain))
> return ERR_CAST(domain);
Yes, this should work, thank you.
--
With best wishes
Dmitry
On Wed, May 29, 2024 at 08:02:12PM +0800, Baolu Lu wrote:
> > > drivers/infiniband/hw/usnic/usnic_uiom.c: pd->domain = domain
> > > = iommu_domain_alloc(dev->bus);
> > >
> > > This series leave those cases unchanged and keep iommu_domain_alloc()
> > > for their usage. But new drivers should not use it anymore.
> >
> > does it mean there is still domains allocated via iommu_domain_alloc()
> > on VT-d platform?
>
> I think the drivers mentioned above do not run on x86 platforms, or do
> they?
usnic does.. What was preventing converting it?
Jason
On 6/3/24 9:35 PM, Jason Gunthorpe wrote:
> On Wed, May 29, 2024 at 08:02:12PM +0800, Baolu Lu wrote:
>>>> drivers/infiniband/hw/usnic/usnic_uiom.c: pd->domain = domain
>>>> = iommu_domain_alloc(dev->bus);
>>>>
>>>> This series leave those cases unchanged and keep iommu_domain_alloc()
>>>> for their usage. But new drivers should not use it anymore.
>>>
>>> does it mean there is still domains allocated via iommu_domain_alloc()
>>> on VT-d platform?
>>
>> I think the drivers mentioned above do not run on x86 platforms, or do
>> they?
>
> usnic does.. What was preventing converting it?
Nothing, just because it wasn't that obvious. :-) I will convert it in
the next version.
Best regards,
baolu