* [PATCH v5 00/12] iommu: SVA and IOPF refactoring
@ 2022-05-02 1:48 Lu Baolu
2022-05-02 1:48 ` [PATCH v5 01/12] dmaengine: idxd: Separate user and kernel pasid enabling Lu Baolu
` (11 more replies)
0 siblings, 12 replies; 30+ messages in thread
From: Lu Baolu @ 2022-05-02 1:48 UTC (permalink / raw)
To: Joerg Roedel, Jason Gunthorpe, Christoph Hellwig, Kevin Tian,
Ashok Raj, Will Deacon, Robin Murphy, Jean-Philippe Brucker,
Dave Jiang, Vinod Koul
Cc: Eric Auger, Liu Yi L, Jacob jun Pan, iommu, linux-kernel, Lu Baolu
Hi folks,
The former part of this series refactors the IOMMU SVA code by assigning
an SVA type of iommu_domain to a shared virtual address and replacing
sva_bind/unbind iommu ops with attach/detach_dev_pasid domain ops.
The latter part changes the existing I/O page fault handling framework
from only serving SVA to a generic one. Any driver or component could
handle the I/O page faults for its domain in its own way by installing
an I/O page fault handler.
This series has been functionally tested on an x86 machine and compile
tested for other architectures.
This series is also available on github:
[2] https://github.com/LuBaolu/intel-iommu/commits/iommu-sva-refactoring-v5
Please review and suggest.
Best regards,
baolu
Change log:
v5:
- Address review comments from Jean-Philippe Brucker. Very appreciated!
- Remove redundant pci aliases check in
device_group_immutable_singleton().
- Treat all buses exept PCI as static in immutable singleton check.
- As the sva_bind/unbind() have already guaranteed sva domain free only
after iopf_queue_flush_dev(), remove the unnecessary domain refcount.
- Move domain get() out of the list iteration in iopf_handle_group().
v4:
- https://lore.kernel.org/linux-iommu/20220421052121.3464100-1-baolu.lu@linux.intel.com/
- Solve the overlap with another series and make this series
self-contained.
- No objection to the abstraction of data structure during v3 review.
Hence remove the RFC subject prefix.
- Refine the immutable singleton group code according to Kevin's
comments.
v3:
- https://lore.kernel.org/linux-iommu/20220410102443.294128-1-baolu.lu@linux.intel.com/
- Rework iommu_group_singleton_lockdown() by adding a flag to the group
that positively indicates the group can never have more than one
member, even after hot plug.
- Abstract the data structs used for iommu sva in a separated patches to
make it easier for review.
- I still keep the RFC prefix in this series as above two significant
changes need at least another round review to be finalized.
- Several misc refinements.
v2:
- https://lore.kernel.org/linux-iommu/20220329053800.3049561-1-baolu.lu@linux.intel.com/
- Add sva domain life cycle management to avoid race between unbind and
page fault handling.
- Use a single domain for each mm.
- Return a single sva handler for the same binding.
- Add a new helper to meet singleton group requirement.
- Rework the SVA domain allocation for arm smmu v3 driver and move the
pasid_bit initialization to device probe.
- Drop the patch "iommu: Handle IO page faults directly".
- Add mmget_not_zero(mm) in SVA page fault handler.
v1:
- https://lore.kernel.org/linux-iommu/20220320064030.2936936-1-baolu.lu@linux.intel.com/
- Initial post.
Dave Jiang (1):
dmaengine: idxd: Separate user and kernel pasid enabling
Lu Baolu (11):
iommu: Add pasid_bits field in struct dev_iommu
iommu: Add attach/detach_dev_pasid domain ops
iommu/sva: Basic data structures for SVA
iommu/vt-d: Remove SVM_FLAG_SUPERVISOR_MODE support
iommu/vt-d: Add SVA domain support
arm-smmu-v3/sva: Add SVA domain support
iommu/sva: Use attach/detach_pasid_dev in SVA interfaces
iommu: Remove SVA related callbacks from iommu ops
iommu: Prepare IOMMU domain for IOPF
iommu: Per-domain I/O page fault handling
iommu: Rename iommu-sva-lib.{c,h}
include/linux/intel-iommu.h | 5 +-
include/linux/iommu.h | 100 ++++--
drivers/dma/idxd/idxd.h | 6 +
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 25 +-
.../iommu/{iommu-sva-lib.h => iommu-sva.h} | 3 +-
drivers/dma/idxd/cdev.c | 4 +-
drivers/dma/idxd/init.c | 30 +-
.../iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c | 85 ++---
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 28 +-
drivers/iommu/intel/iommu.c | 20 +-
drivers/iommu/intel/svm.c | 135 +++----
drivers/iommu/io-pgfault.c | 66 +---
drivers/iommu/iommu-sva-lib.c | 71 ----
drivers/iommu/iommu-sva.c | 328 ++++++++++++++++++
drivers/iommu/iommu.c | 189 +++++-----
drivers/iommu/Makefile | 2 +-
16 files changed, 672 insertions(+), 425 deletions(-)
rename drivers/iommu/{iommu-sva-lib.h => iommu-sva.h} (91%)
delete mode 100644 drivers/iommu/iommu-sva-lib.c
create mode 100644 drivers/iommu/iommu-sva.c
--
2.25.1
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v5 01/12] dmaengine: idxd: Separate user and kernel pasid enabling
2022-05-02 1:48 [PATCH v5 00/12] iommu: SVA and IOPF refactoring Lu Baolu
@ 2022-05-02 1:48 ` Lu Baolu
2022-05-02 1:48 ` [PATCH v5 02/12] iommu: Add pasid_bits field in struct dev_iommu Lu Baolu
` (10 subsequent siblings)
11 siblings, 0 replies; 30+ messages in thread
From: Lu Baolu @ 2022-05-02 1:48 UTC (permalink / raw)
To: Joerg Roedel, Jason Gunthorpe, Christoph Hellwig, Kevin Tian,
Ashok Raj, Will Deacon, Robin Murphy, Jean-Philippe Brucker,
Dave Jiang, Vinod Koul
Cc: Eric Auger, Liu Yi L, Jacob jun Pan, iommu, linux-kernel,
Jacob Pan, Lu Baolu
From: Dave Jiang <dave.jiang@intel.com>
The idxd driver always gated the pasid enabling under a single knob and
this assumption is incorrect. The pasid used for kernel operation can be
independently toggled and has no dependency on the user pasid (and vice
versa). Split the two so they are independent "enabled" flags.
Cc: Vinod Koul <vkoul@kernel.org>
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
Link: https://lore.kernel.org/linux-iommu/20220315050713.2000518-10-jacob.jun.pan@linux.intel.com/
Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
drivers/dma/idxd/idxd.h | 6 ++++++
drivers/dma/idxd/cdev.c | 4 ++--
drivers/dma/idxd/init.c | 30 ++++++++++++++++++------------
3 files changed, 26 insertions(+), 14 deletions(-)
diff --git a/drivers/dma/idxd/idxd.h b/drivers/dma/idxd/idxd.h
index da72eb15f610..ccbefd0be617 100644
--- a/drivers/dma/idxd/idxd.h
+++ b/drivers/dma/idxd/idxd.h
@@ -239,6 +239,7 @@ enum idxd_device_flag {
IDXD_FLAG_CONFIGURABLE = 0,
IDXD_FLAG_CMD_RUNNING,
IDXD_FLAG_PASID_ENABLED,
+ IDXD_FLAG_USER_PASID_ENABLED,
};
struct idxd_dma_dev {
@@ -469,6 +470,11 @@ static inline bool device_pasid_enabled(struct idxd_device *idxd)
return test_bit(IDXD_FLAG_PASID_ENABLED, &idxd->flags);
}
+static inline bool device_user_pasid_enabled(struct idxd_device *idxd)
+{
+ return test_bit(IDXD_FLAG_USER_PASID_ENABLED, &idxd->flags);
+}
+
static inline bool device_swq_supported(struct idxd_device *idxd)
{
return (support_enqcmd && device_pasid_enabled(idxd));
diff --git a/drivers/dma/idxd/cdev.c b/drivers/dma/idxd/cdev.c
index b9b2b4a4124e..7df996deffbe 100644
--- a/drivers/dma/idxd/cdev.c
+++ b/drivers/dma/idxd/cdev.c
@@ -99,7 +99,7 @@ static int idxd_cdev_open(struct inode *inode, struct file *filp)
ctx->wq = wq;
filp->private_data = ctx;
- if (device_pasid_enabled(idxd)) {
+ if (device_user_pasid_enabled(idxd)) {
sva = iommu_sva_bind_device(dev, current->mm, NULL);
if (IS_ERR(sva)) {
rc = PTR_ERR(sva);
@@ -152,7 +152,7 @@ static int idxd_cdev_release(struct inode *node, struct file *filep)
if (wq_shared(wq)) {
idxd_device_drain_pasid(idxd, ctx->pasid);
} else {
- if (device_pasid_enabled(idxd)) {
+ if (device_user_pasid_enabled(idxd)) {
/* The wq disable in the disable pasid function will drain the wq */
rc = idxd_wq_disable_pasid(wq);
if (rc < 0)
diff --git a/drivers/dma/idxd/init.c b/drivers/dma/idxd/init.c
index 993a5dcca24f..e1b5d1e4a949 100644
--- a/drivers/dma/idxd/init.c
+++ b/drivers/dma/idxd/init.c
@@ -513,16 +513,19 @@ static int idxd_probe(struct idxd_device *idxd)
if (IS_ENABLED(CONFIG_INTEL_IDXD_SVM) && sva) {
rc = iommu_dev_enable_feature(dev, IOMMU_DEV_FEAT_SVA);
- if (rc == 0) {
- rc = idxd_enable_system_pasid(idxd);
- if (rc < 0) {
- iommu_dev_disable_feature(dev, IOMMU_DEV_FEAT_SVA);
- dev_warn(dev, "Failed to enable PASID. No SVA support: %d\n", rc);
- } else {
- set_bit(IDXD_FLAG_PASID_ENABLED, &idxd->flags);
- }
- } else {
+ if (rc) {
+ /*
+ * Do not bail here since legacy DMA is still
+ * supported, both user and in-kernel DMA with
+ * PASID rely on SVA feature.
+ */
dev_warn(dev, "Unable to turn on SVA feature.\n");
+ } else {
+ set_bit(IDXD_FLAG_USER_PASID_ENABLED, &idxd->flags);
+ if (idxd_enable_system_pasid(idxd))
+ dev_warn(dev, "No in-kernel DMA with PASID.\n");
+ else
+ set_bit(IDXD_FLAG_PASID_ENABLED, &idxd->flags);
}
} else if (!sva) {
dev_warn(dev, "User forced SVA off via module param.\n");
@@ -561,7 +564,8 @@ static int idxd_probe(struct idxd_device *idxd)
err:
if (device_pasid_enabled(idxd))
idxd_disable_system_pasid(idxd);
- iommu_dev_disable_feature(dev, IOMMU_DEV_FEAT_SVA);
+ if (device_user_pasid_enabled(idxd) || device_pasid_enabled(idxd))
+ iommu_dev_disable_feature(dev, IOMMU_DEV_FEAT_SVA);
return rc;
}
@@ -574,7 +578,8 @@ static void idxd_cleanup(struct idxd_device *idxd)
idxd_cleanup_internals(idxd);
if (device_pasid_enabled(idxd))
idxd_disable_system_pasid(idxd);
- iommu_dev_disable_feature(dev, IOMMU_DEV_FEAT_SVA);
+ if (device_user_pasid_enabled(idxd) || device_pasid_enabled(idxd))
+ iommu_dev_disable_feature(dev, IOMMU_DEV_FEAT_SVA);
}
static int idxd_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
@@ -691,7 +696,8 @@ static void idxd_remove(struct pci_dev *pdev)
free_irq(irq_entry->vector, irq_entry);
pci_free_irq_vectors(pdev);
pci_iounmap(pdev, idxd->reg_base);
- iommu_dev_disable_feature(&pdev->dev, IOMMU_DEV_FEAT_SVA);
+ if (device_user_pasid_enabled(idxd) || device_pasid_enabled(idxd))
+ iommu_dev_disable_feature(&pdev->dev, IOMMU_DEV_FEAT_SVA);
pci_disable_device(pdev);
destroy_workqueue(idxd->wq);
perfmon_pmu_remove(idxd);
--
2.25.1
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v5 02/12] iommu: Add pasid_bits field in struct dev_iommu
2022-05-02 1:48 [PATCH v5 00/12] iommu: SVA and IOPF refactoring Lu Baolu
2022-05-02 1:48 ` [PATCH v5 01/12] dmaengine: idxd: Separate user and kernel pasid enabling Lu Baolu
@ 2022-05-02 1:48 ` Lu Baolu
2022-05-03 18:02 ` Jean-Philippe Brucker
2022-05-02 1:48 ` [PATCH v5 03/12] iommu: Add attach/detach_dev_pasid domain ops Lu Baolu
` (9 subsequent siblings)
11 siblings, 1 reply; 30+ messages in thread
From: Lu Baolu @ 2022-05-02 1:48 UTC (permalink / raw)
To: Joerg Roedel, Jason Gunthorpe, Christoph Hellwig, Kevin Tian,
Ashok Raj, Will Deacon, Robin Murphy, Jean-Philippe Brucker,
Dave Jiang, Vinod Koul
Cc: Eric Auger, Liu Yi L, Jacob jun Pan, iommu, linux-kernel, Lu Baolu
Use this field to save the pasid/ssid bits that a device is able to
support with its IOMMU hardware. It is a generic attribute of a device
and lifting it into the per-device dev_iommu struct makes it possible
to allocate a PASID for device without calls into the IOMMU drivers.
Any iommu driver which suports PASID related features should set this
field before features are enabled on the devices.
For initialization of this field in the VT-d driver, the
info->pasid_supported is only set for PCI devices. So the status is
that non-PCI SVA hasn't been supported yet. Setting this field only for
PCI devices has no functional change.
Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
include/linux/iommu.h | 1 +
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 2 ++
drivers/iommu/intel/iommu.c | 5 ++++-
3 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 5e1afe169549..b8ffaf2cb1d0 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -373,6 +373,7 @@ struct dev_iommu {
struct iommu_fwspec *fwspec;
struct iommu_device *iommu_dev;
void *priv;
+ unsigned int pasid_bits;
};
int iommu_device_register(struct iommu_device *iommu,
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
index 627a3ed5ee8f..afc63fce6107 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -2681,6 +2681,8 @@ static struct iommu_device *arm_smmu_probe_device(struct device *dev)
smmu->features & ARM_SMMU_FEAT_STALL_FORCE)
master->stall_enabled = true;
+ dev->iommu->pasid_bits = master->ssid_bits;
+
return &smmu->iommu;
err_free_master:
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index cf43e8f9091b..170eb777d57b 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -4611,8 +4611,11 @@ static struct iommu_device *intel_iommu_probe_device(struct device *dev)
if (pasid_supported(iommu)) {
int features = pci_pasid_features(pdev);
- if (features >= 0)
+ if (features >= 0) {
info->pasid_supported = features | 1;
+ dev->iommu->pasid_bits =
+ fls(pci_max_pasids(pdev)) - 1;
+ }
}
if (info->ats_supported && ecap_prs(iommu->ecap) &&
--
2.25.1
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v5 03/12] iommu: Add attach/detach_dev_pasid domain ops
2022-05-02 1:48 [PATCH v5 00/12] iommu: SVA and IOPF refactoring Lu Baolu
2022-05-02 1:48 ` [PATCH v5 01/12] dmaengine: idxd: Separate user and kernel pasid enabling Lu Baolu
2022-05-02 1:48 ` [PATCH v5 02/12] iommu: Add pasid_bits field in struct dev_iommu Lu Baolu
@ 2022-05-02 1:48 ` Lu Baolu
2022-05-03 18:07 ` Jean-Philippe Brucker
2022-05-02 1:48 ` [PATCH v5 04/12] iommu/sva: Basic data structures for SVA Lu Baolu
` (8 subsequent siblings)
11 siblings, 1 reply; 30+ messages in thread
From: Lu Baolu @ 2022-05-02 1:48 UTC (permalink / raw)
To: Joerg Roedel, Jason Gunthorpe, Christoph Hellwig, Kevin Tian,
Ashok Raj, Will Deacon, Robin Murphy, Jean-Philippe Brucker,
Dave Jiang, Vinod Koul
Cc: Eric Auger, Liu Yi L, Jacob jun Pan, iommu, linux-kernel, Lu Baolu
Attaching an IOMMU domain to a PASID of a device is a generic operation
for modern IOMMU drivers which support PASID-granular DMA address
translation. Currently visible usage scenarios include (but not limited):
- SVA (Shared Virtual Address)
- kernel DMA with PASID
- hardware-assist mediated device
This adds a pair of common domain ops for this purpose and adds helpers
to attach/detach a domain to/from a {device, PASID}. Some buses, like
PCI, route packets without considering the PASID value. Thus a DMA target
address with PASID might be treated as P2P if the address falls into the
MMIO BAR of other devices in the group. To make things simple, these
interfaces only apply to devices belonging to the singleton groups, and
the singleton is immutable in fabric i.e. not affected by hotplug.
Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
include/linux/iommu.h | 21 ++++++++++++
drivers/iommu/iommu.c | 76 +++++++++++++++++++++++++++++++++++++++++++
2 files changed, 97 insertions(+)
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index b8ffaf2cb1d0..ab36244d4e94 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -263,6 +263,8 @@ struct iommu_ops {
* struct iommu_domain_ops - domain specific operations
* @attach_dev: attach an iommu domain to a device
* @detach_dev: detach an iommu domain from a device
+ * @attach_dev_pasid: attach an iommu domain to a pasid of device
+ * @detach_dev_pasid: detach an iommu domain from a pasid of device
* @map: map a physically contiguous memory region to an iommu domain
* @map_pages: map a physically contiguous set of pages of the same size to
* an iommu domain.
@@ -283,6 +285,10 @@ struct iommu_ops {
struct iommu_domain_ops {
int (*attach_dev)(struct iommu_domain *domain, struct device *dev);
void (*detach_dev)(struct iommu_domain *domain, struct device *dev);
+ int (*attach_dev_pasid)(struct iommu_domain *domain,
+ struct device *dev, ioasid_t pasid);
+ void (*detach_dev_pasid)(struct iommu_domain *domain,
+ struct device *dev, ioasid_t pasid);
int (*map)(struct iommu_domain *domain, unsigned long iova,
phys_addr_t paddr, size_t size, int prot, gfp_t gfp);
@@ -678,6 +684,10 @@ int iommu_group_claim_dma_owner(struct iommu_group *group, void *owner);
void iommu_group_release_dma_owner(struct iommu_group *group);
bool iommu_group_dma_owner_claimed(struct iommu_group *group);
+int iommu_attach_device_pasid(struct iommu_domain *domain,
+ struct device *dev, ioasid_t pasid);
+void iommu_detach_device_pasid(struct iommu_domain *domain,
+ struct device *dev, ioasid_t pasid);
#else /* CONFIG_IOMMU_API */
struct iommu_ops {};
@@ -1051,6 +1061,17 @@ static inline bool iommu_group_dma_owner_claimed(struct iommu_group *group)
{
return false;
}
+
+static inline int iommu_attach_device_pasid(struct iommu_domain *domain,
+ struct device *dev, ioasid_t pasid)
+{
+ return -ENODEV;
+}
+
+static inline void iommu_detach_device_pasid(struct iommu_domain *domain,
+ struct device *dev, ioasid_t pasid)
+{
+}
#endif /* CONFIG_IOMMU_API */
/**
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 29906bc16371..89c9d19ddb28 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -38,6 +38,7 @@ struct iommu_group {
struct kobject kobj;
struct kobject *devices_kobj;
struct list_head devices;
+ struct xarray pasid_array;
struct mutex mutex;
void *iommu_data;
void (*iommu_data_release)(void *iommu_data);
@@ -630,6 +631,7 @@ struct iommu_group *iommu_group_alloc(void)
mutex_init(&group->mutex);
INIT_LIST_HEAD(&group->devices);
INIT_LIST_HEAD(&group->entry);
+ xa_init(&group->pasid_array);
ret = ida_simple_get(&iommu_group_ida, 0, 0, GFP_KERNEL);
if (ret < 0) {
@@ -3190,3 +3192,77 @@ bool iommu_group_dma_owner_claimed(struct iommu_group *group)
return user;
}
EXPORT_SYMBOL_GPL(iommu_group_dma_owner_claimed);
+
+/*
+ * Use standard PCI bus topology and isolation features to check immutable
+ * singleton. Otherwise, assume the bus is static and then singleton can
+ * know from the device count in the group.
+ */
+static bool device_group_immutable_singleton(struct device *dev)
+{
+ struct iommu_group *group = iommu_group_get(dev);
+ int count;
+
+ if (!group)
+ return false;
+
+ mutex_lock(&group->mutex);
+ count = iommu_group_device_count(group);
+ mutex_unlock(&group->mutex);
+ iommu_group_put(group);
+
+ if (count != 1)
+ return false;
+
+ /*
+ * The PCI device could be considered to be fully isolated if all
+ * devices on the path from the device to the host-PCI bridge are
+ * protected from peer-to-peer DMA by ACS.
+ */
+ if (dev_is_pci(dev))
+ return pci_acs_path_enabled(to_pci_dev(dev), NULL,
+ REQ_ACS_FLAGS);
+
+ return true;
+}
+
+int iommu_attach_device_pasid(struct iommu_domain *domain,
+ struct device *dev, ioasid_t pasid)
+{
+ struct iommu_group *group;
+ int ret = -EINVAL;
+ void *curr;
+
+ if (!domain->ops->attach_dev_pasid)
+ return -EOPNOTSUPP;
+
+ if (!device_group_immutable_singleton(dev))
+ return -EINVAL;
+
+ group = iommu_group_get(dev);
+ mutex_lock(&group->mutex);
+ curr = xa_cmpxchg(&group->pasid_array, pasid, NULL, domain, GFP_KERNEL);
+ if (curr)
+ goto out_unlock;
+ ret = domain->ops->attach_dev_pasid(domain, dev, pasid);
+ if (ret)
+ xa_erase(&group->pasid_array, pasid);
+out_unlock:
+ mutex_unlock(&group->mutex);
+ iommu_group_put(group);
+
+ return ret;
+}
+
+void iommu_detach_device_pasid(struct iommu_domain *domain,
+ struct device *dev, ioasid_t pasid)
+{
+ struct iommu_group *group = iommu_group_get(dev);
+
+ mutex_lock(&group->mutex);
+ domain->ops->detach_dev_pasid(domain, dev, pasid);
+ xa_erase(&group->pasid_array, pasid);
+ mutex_unlock(&group->mutex);
+
+ iommu_group_put(group);
+}
--
2.25.1
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v5 04/12] iommu/sva: Basic data structures for SVA
2022-05-02 1:48 [PATCH v5 00/12] iommu: SVA and IOPF refactoring Lu Baolu
` (2 preceding siblings ...)
2022-05-02 1:48 ` [PATCH v5 03/12] iommu: Add attach/detach_dev_pasid domain ops Lu Baolu
@ 2022-05-02 1:48 ` Lu Baolu
2022-05-03 18:09 ` Jean-Philippe Brucker
2022-05-02 1:48 ` [PATCH v5 05/12] iommu/vt-d: Remove SVM_FLAG_SUPERVISOR_MODE support Lu Baolu
` (7 subsequent siblings)
11 siblings, 1 reply; 30+ messages in thread
From: Lu Baolu @ 2022-05-02 1:48 UTC (permalink / raw)
To: Joerg Roedel, Jason Gunthorpe, Christoph Hellwig, Kevin Tian,
Ashok Raj, Will Deacon, Robin Murphy, Jean-Philippe Brucker,
Dave Jiang, Vinod Koul
Cc: Eric Auger, Liu Yi L, Jacob jun Pan, iommu, linux-kernel, Lu Baolu
Use below data structures for SVA implementation in the IOMMU core:
- struct iommu_sva_ioas
Represent the I/O address space shared with an application CPU address
space. This structure has a 1:1 relationship with an mm_struct. It
grabs a "mm->mm_count" refcount during creation and drop it on release.
- struct iommu_domain (IOMMU_DOMAIN_SVA type)
Represent a hardware pagetable that the IOMMU hardware could use for
SVA translation. Multiple iommu domains could be bound with an SVA ioas
and each grabs a refcount from ioas in order to make sure ioas could
only be freed after all domains have been unbound.
- struct iommu_sva
Represent a bond relationship between an SVA ioas and an iommu domain.
If a bond already exists, it's reused and a reference is taken.
Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
include/linux/iommu.h | 14 +++++++++++++-
drivers/iommu/iommu-sva-lib.h | 1 +
drivers/iommu/iommu-sva-lib.c | 18 ++++++++++++++++++
3 files changed, 32 insertions(+), 1 deletion(-)
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index ab36244d4e94..f582f434c513 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -42,6 +42,7 @@ struct notifier_block;
struct iommu_sva;
struct iommu_fault_event;
struct iommu_dma_cookie;
+struct iommu_sva_ioas;
/* iommu fault flags */
#define IOMMU_FAULT_READ 0x0
@@ -64,6 +65,9 @@ struct iommu_domain_geometry {
#define __IOMMU_DOMAIN_PT (1U << 2) /* Domain is identity mapped */
#define __IOMMU_DOMAIN_DMA_FQ (1U << 3) /* DMA-API uses flush queue */
+#define __IOMMU_DOMAIN_SHARED (1U << 4) /* Page table shared from CPU */
+#define __IOMMU_DOMAIN_HOST_VA (1U << 5) /* Host CPU virtual address */
+
/*
* This are the possible domain-types
*
@@ -86,6 +90,8 @@ struct iommu_domain_geometry {
#define IOMMU_DOMAIN_DMA_FQ (__IOMMU_DOMAIN_PAGING | \
__IOMMU_DOMAIN_DMA_API | \
__IOMMU_DOMAIN_DMA_FQ)
+#define IOMMU_DOMAIN_SVA (__IOMMU_DOMAIN_SHARED | \
+ __IOMMU_DOMAIN_HOST_VA)
struct iommu_domain {
unsigned type;
@@ -95,6 +101,7 @@ struct iommu_domain {
void *handler_token;
struct iommu_domain_geometry geometry;
struct iommu_dma_cookie *iova_cookie;
+ struct iommu_sva_ioas *sva_ioas;
};
static inline bool iommu_is_dma_domain(struct iommu_domain *domain)
@@ -628,7 +635,12 @@ struct iommu_fwspec {
* struct iommu_sva - handle to a device-mm bond
*/
struct iommu_sva {
- struct device *dev;
+ struct device *dev;
+ struct iommu_sva_ioas *sva_ioas;
+ struct iommu_domain *domain;
+ /* Link to sva ioas's bonds list */
+ struct list_head node;
+ refcount_t users;
};
int iommu_fwspec_init(struct device *dev, struct fwnode_handle *iommu_fwnode,
diff --git a/drivers/iommu/iommu-sva-lib.h b/drivers/iommu/iommu-sva-lib.h
index 8909ea1094e3..9c5e108e2c8a 100644
--- a/drivers/iommu/iommu-sva-lib.h
+++ b/drivers/iommu/iommu-sva-lib.h
@@ -10,6 +10,7 @@
int iommu_sva_alloc_pasid(struct mm_struct *mm, ioasid_t min, ioasid_t max);
struct mm_struct *iommu_sva_find(ioasid_t pasid);
+struct mm_struct *iommu_sva_domain_mm(struct iommu_domain *domain);
/* I/O Page fault */
struct device;
diff --git a/drivers/iommu/iommu-sva-lib.c b/drivers/iommu/iommu-sva-lib.c
index 106506143896..d524a402be3b 100644
--- a/drivers/iommu/iommu-sva-lib.c
+++ b/drivers/iommu/iommu-sva-lib.c
@@ -3,6 +3,8 @@
* Helpers for IOMMU drivers implementing SVA
*/
#include <linux/mutex.h>
+#include <linux/iommu.h>
+#include <linux/slab.h>
#include <linux/sched/mm.h>
#include "iommu-sva-lib.h"
@@ -10,6 +12,22 @@
static DEFINE_MUTEX(iommu_sva_lock);
static DECLARE_IOASID_SET(iommu_sva_pasid);
+struct iommu_sva_ioas {
+ struct mm_struct *mm;
+ ioasid_t pasid;
+
+ /* Counter of domains attached to this ioas. */
+ refcount_t users;
+
+ /* All bindings are linked here. */
+ struct list_head bonds;
+};
+
+struct mm_struct *iommu_sva_domain_mm(struct iommu_domain *domain)
+{
+ return domain->sva_ioas->mm;
+}
+
/**
* iommu_sva_alloc_pasid - Allocate a PASID for the mm
* @mm: the mm
--
2.25.1
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v5 05/12] iommu/vt-d: Remove SVM_FLAG_SUPERVISOR_MODE support
2022-05-02 1:48 [PATCH v5 00/12] iommu: SVA and IOPF refactoring Lu Baolu
` (3 preceding siblings ...)
2022-05-02 1:48 ` [PATCH v5 04/12] iommu/sva: Basic data structures for SVA Lu Baolu
@ 2022-05-02 1:48 ` Lu Baolu
2022-05-02 1:48 ` [PATCH v5 06/12] iommu/vt-d: Add SVA domain support Lu Baolu
` (6 subsequent siblings)
11 siblings, 0 replies; 30+ messages in thread
From: Lu Baolu @ 2022-05-02 1:48 UTC (permalink / raw)
To: Joerg Roedel, Jason Gunthorpe, Christoph Hellwig, Kevin Tian,
Ashok Raj, Will Deacon, Robin Murphy, Jean-Philippe Brucker,
Dave Jiang, Vinod Koul
Cc: Eric Auger, Liu Yi L, Jacob jun Pan, iommu, linux-kernel,
Lu Baolu, Jacob Pan
The current kernel DMA with PASID support is based on the SVA with a flag
SVM_FLAG_SUPERVISOR_MODE. The IOMMU driver binds the kernel memory address
space to a PASID of the device. The device driver programs the device with
kernel virtual address (KVA) for DMA access. There have been security and
functional issues with this approach:
- The lack of IOTLB synchronization upon kernel page table updates.
(vmalloc, module/BPF loading, CONFIG_DEBUG_PAGEALLOC etc.)
- Other than slight more protection, using kernel virtual address (KVA)
has little advantage over physical address. There are also no use
cases yet where DMA engines need kernel virtual addresses for in-kernel
DMA.
This removes SVM_FLAG_SUPERVISOR_MODE support in the Intel IOMMU driver.
The device driver is suggested to handle kernel DMA with PASID through
the kernel DMA APIs.
Link: https://lore.kernel.org/linux-iommu/20210511194726.GP1002214@nvidia.com/
Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
drivers/iommu/intel/svm.c | 53 +++++++++------------------------------
1 file changed, 12 insertions(+), 41 deletions(-)
diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c
index 7ee37d996e15..574ddddaa33a 100644
--- a/drivers/iommu/intel/svm.c
+++ b/drivers/iommu/intel/svm.c
@@ -313,8 +313,7 @@ static int pasid_to_svm_sdev(struct device *dev, unsigned int pasid,
return 0;
}
-static int intel_svm_alloc_pasid(struct device *dev, struct mm_struct *mm,
- unsigned int flags)
+static int intel_svm_alloc_pasid(struct device *dev, struct mm_struct *mm)
{
ioasid_t max_pasid = dev_is_pci(dev) ?
pci_max_pasids(to_pci_dev(dev)) : intel_pasid_max_id;
@@ -324,8 +323,7 @@ static int intel_svm_alloc_pasid(struct device *dev, struct mm_struct *mm,
static struct iommu_sva *intel_svm_bind_mm(struct intel_iommu *iommu,
struct device *dev,
- struct mm_struct *mm,
- unsigned int flags)
+ struct mm_struct *mm)
{
struct device_domain_info *info = dev_iommu_priv_get(dev);
unsigned long iflags, sflags;
@@ -341,22 +339,18 @@ static struct iommu_sva *intel_svm_bind_mm(struct intel_iommu *iommu,
svm->pasid = mm->pasid;
svm->mm = mm;
- svm->flags = flags;
INIT_LIST_HEAD_RCU(&svm->devs);
- if (!(flags & SVM_FLAG_SUPERVISOR_MODE)) {
- svm->notifier.ops = &intel_mmuops;
- ret = mmu_notifier_register(&svm->notifier, mm);
- if (ret) {
- kfree(svm);
- return ERR_PTR(ret);
- }
+ svm->notifier.ops = &intel_mmuops;
+ ret = mmu_notifier_register(&svm->notifier, mm);
+ if (ret) {
+ kfree(svm);
+ return ERR_PTR(ret);
}
ret = pasid_private_add(svm->pasid, svm);
if (ret) {
- if (svm->notifier.ops)
- mmu_notifier_unregister(&svm->notifier, mm);
+ mmu_notifier_unregister(&svm->notifier, mm);
kfree(svm);
return ERR_PTR(ret);
}
@@ -391,9 +385,7 @@ static struct iommu_sva *intel_svm_bind_mm(struct intel_iommu *iommu,
}
/* Setup the pasid table: */
- sflags = (flags & SVM_FLAG_SUPERVISOR_MODE) ?
- PASID_FLAG_SUPERVISOR_MODE : 0;
- sflags |= cpu_feature_enabled(X86_FEATURE_LA57) ? PASID_FLAG_FL5LP : 0;
+ sflags = cpu_feature_enabled(X86_FEATURE_LA57) ? PASID_FLAG_FL5LP : 0;
spin_lock_irqsave(&iommu->lock, iflags);
ret = intel_pasid_setup_first_level(iommu, dev, mm->pgd, mm->pasid,
FLPT_DEFAULT_DID, sflags);
@@ -410,8 +402,7 @@ static struct iommu_sva *intel_svm_bind_mm(struct intel_iommu *iommu,
kfree(sdev);
free_svm:
if (list_empty(&svm->devs)) {
- if (svm->notifier.ops)
- mmu_notifier_unregister(&svm->notifier, mm);
+ mmu_notifier_unregister(&svm->notifier, mm);
pasid_private_remove(mm->pasid);
kfree(svm);
}
@@ -821,37 +812,17 @@ static irqreturn_t prq_event_thread(int irq, void *d)
struct iommu_sva *intel_svm_bind(struct device *dev, struct mm_struct *mm, void *drvdata)
{
struct intel_iommu *iommu = device_to_iommu(dev, NULL, NULL);
- unsigned int flags = 0;
struct iommu_sva *sva;
int ret;
- if (drvdata)
- flags = *(unsigned int *)drvdata;
-
- if (flags & SVM_FLAG_SUPERVISOR_MODE) {
- if (!ecap_srs(iommu->ecap)) {
- dev_err(dev, "%s: Supervisor PASID not supported\n",
- iommu->name);
- return ERR_PTR(-EOPNOTSUPP);
- }
-
- if (mm) {
- dev_err(dev, "%s: Supervisor PASID with user provided mm\n",
- iommu->name);
- return ERR_PTR(-EINVAL);
- }
-
- mm = &init_mm;
- }
-
mutex_lock(&pasid_mutex);
- ret = intel_svm_alloc_pasid(dev, mm, flags);
+ ret = intel_svm_alloc_pasid(dev, mm);
if (ret) {
mutex_unlock(&pasid_mutex);
return ERR_PTR(ret);
}
- sva = intel_svm_bind_mm(iommu, dev, mm, flags);
+ sva = intel_svm_bind_mm(iommu, dev, mm);
mutex_unlock(&pasid_mutex);
return sva;
--
2.25.1
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v5 06/12] iommu/vt-d: Add SVA domain support
2022-05-02 1:48 [PATCH v5 00/12] iommu: SVA and IOPF refactoring Lu Baolu
` (4 preceding siblings ...)
2022-05-02 1:48 ` [PATCH v5 05/12] iommu/vt-d: Remove SVM_FLAG_SUPERVISOR_MODE support Lu Baolu
@ 2022-05-02 1:48 ` Lu Baolu
2022-05-02 1:48 ` [PATCH v5 07/12] arm-smmu-v3/sva: " Lu Baolu
` (5 subsequent siblings)
11 siblings, 0 replies; 30+ messages in thread
From: Lu Baolu @ 2022-05-02 1:48 UTC (permalink / raw)
To: Joerg Roedel, Jason Gunthorpe, Christoph Hellwig, Kevin Tian,
Ashok Raj, Will Deacon, Robin Murphy, Jean-Philippe Brucker,
Dave Jiang, Vinod Koul
Cc: Eric Auger, Liu Yi L, Jacob jun Pan, iommu, linux-kernel, Lu Baolu
Add support for SVA domain allocation and provide an SVA-specific
iommu_domain_ops.
Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
include/linux/intel-iommu.h | 1 +
drivers/iommu/intel/iommu.c | 10 ++++++++++
drivers/iommu/intel/svm.c | 37 +++++++++++++++++++++++++++++++++++++
3 files changed, 48 insertions(+)
diff --git a/include/linux/intel-iommu.h b/include/linux/intel-iommu.h
index 72e5d7900e71..3b4ca16f53e2 100644
--- a/include/linux/intel-iommu.h
+++ b/include/linux/intel-iommu.h
@@ -744,6 +744,7 @@ void intel_svm_unbind(struct iommu_sva *handle);
u32 intel_svm_get_pasid(struct iommu_sva *handle);
int intel_svm_page_response(struct device *dev, struct iommu_fault_event *evt,
struct iommu_page_response *msg);
+extern const struct iommu_domain_ops intel_svm_domain_ops;
struct intel_svm_dev {
struct list_head list;
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 170eb777d57b..5d1d3e325afa 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -4330,6 +4330,16 @@ static struct iommu_domain *intel_iommu_domain_alloc(unsigned type)
return domain;
case IOMMU_DOMAIN_IDENTITY:
return &si_domain->domain;
+#ifdef CONFIG_INTEL_IOMMU_SVM
+ case IOMMU_DOMAIN_SVA:
+ dmar_domain = alloc_domain(type);
+ if (!dmar_domain)
+ return NULL;
+ domain = &dmar_domain->domain;
+ domain->ops = &intel_svm_domain_ops;
+
+ return domain;
+#endif /* CONFIG_INTEL_IOMMU_SVM */
default:
return NULL;
}
diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c
index 574ddddaa33a..ec684d883014 100644
--- a/drivers/iommu/intel/svm.c
+++ b/drivers/iommu/intel/svm.c
@@ -931,3 +931,40 @@ int intel_svm_page_response(struct device *dev,
mutex_unlock(&pasid_mutex);
return ret;
}
+
+static int intel_svm_attach_dev_pasid(struct iommu_domain *domain,
+ struct device *dev, ioasid_t pasid)
+{
+ struct device_domain_info *info = dev_iommu_priv_get(dev);
+ struct mm_struct *mm = iommu_sva_domain_mm(domain);
+ struct intel_iommu *iommu = info->iommu;
+ struct iommu_sva *sva;
+ int ret = 0;
+
+ mutex_lock(&pasid_mutex);
+ sva = intel_svm_bind_mm(iommu, dev, mm);
+ if (IS_ERR(sva))
+ ret = PTR_ERR(sva);
+ mutex_unlock(&pasid_mutex);
+
+ return ret;
+}
+
+static void intel_svm_detach_dev_pasid(struct iommu_domain *domain,
+ struct device *dev, ioasid_t pasid)
+{
+ mutex_lock(&pasid_mutex);
+ intel_svm_unbind_mm(dev, pasid);
+ mutex_unlock(&pasid_mutex);
+}
+
+static void intel_svm_domain_free(struct iommu_domain *domain)
+{
+ kfree(to_dmar_domain(domain));
+}
+
+const struct iommu_domain_ops intel_svm_domain_ops = {
+ .attach_dev_pasid = intel_svm_attach_dev_pasid,
+ .detach_dev_pasid = intel_svm_detach_dev_pasid,
+ .free = intel_svm_domain_free,
+};
--
2.25.1
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v5 07/12] arm-smmu-v3/sva: Add SVA domain support
2022-05-02 1:48 [PATCH v5 00/12] iommu: SVA and IOPF refactoring Lu Baolu
` (5 preceding siblings ...)
2022-05-02 1:48 ` [PATCH v5 06/12] iommu/vt-d: Add SVA domain support Lu Baolu
@ 2022-05-02 1:48 ` Lu Baolu
2022-05-03 18:12 ` Jean-Philippe Brucker
2022-05-02 1:48 ` [PATCH v5 08/12] iommu/sva: Use attach/detach_pasid_dev in SVA interfaces Lu Baolu
` (4 subsequent siblings)
11 siblings, 1 reply; 30+ messages in thread
From: Lu Baolu @ 2022-05-02 1:48 UTC (permalink / raw)
To: Joerg Roedel, Jason Gunthorpe, Christoph Hellwig, Kevin Tian,
Ashok Raj, Will Deacon, Robin Murphy, Jean-Philippe Brucker,
Dave Jiang, Vinod Koul
Cc: Eric Auger, Liu Yi L, Jacob jun Pan, iommu, linux-kernel, Lu Baolu
Add support for SVA domain allocation and provide an SVA-specific
iommu_domain_ops.
Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 14 +++++++
.../iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c | 42 +++++++++++++++++++
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 21 ++++++++++
3 files changed, 77 insertions(+)
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
index cd48590ada30..7631c00fdcbd 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
@@ -759,6 +759,10 @@ struct iommu_sva *arm_smmu_sva_bind(struct device *dev, struct mm_struct *mm,
void arm_smmu_sva_unbind(struct iommu_sva *handle);
u32 arm_smmu_sva_get_pasid(struct iommu_sva *handle);
void arm_smmu_sva_notifier_synchronize(void);
+int arm_smmu_sva_attach_dev_pasid(struct iommu_domain *domain,
+ struct device *dev, ioasid_t id);
+void arm_smmu_sva_detach_dev_pasid(struct iommu_domain *domain,
+ struct device *dev, ioasid_t id);
#else /* CONFIG_ARM_SMMU_V3_SVA */
static inline bool arm_smmu_sva_supported(struct arm_smmu_device *smmu)
{
@@ -804,5 +808,15 @@ static inline u32 arm_smmu_sva_get_pasid(struct iommu_sva *handle)
}
static inline void arm_smmu_sva_notifier_synchronize(void) {}
+
+static inline int arm_smmu_sva_attach_dev_pasid(struct iommu_domain *domain,
+ struct device *dev, ioasid_t id)
+{
+ return -ENODEV;
+}
+
+static inline void arm_smmu_sva_detach_dev_pasid(struct iommu_domain *domain,
+ struct device *dev,
+ ioasid_t id) {}
#endif /* CONFIG_ARM_SMMU_V3_SVA */
#endif /* _ARM_SMMU_V3_H */
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
index c623dae1e115..3b843cd3ed67 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
@@ -541,3 +541,45 @@ void arm_smmu_sva_notifier_synchronize(void)
*/
mmu_notifier_synchronize();
}
+
+int arm_smmu_sva_attach_dev_pasid(struct iommu_domain *domain,
+ struct device *dev, ioasid_t id)
+{
+ int ret = 0;
+ struct iommu_sva *handle;
+ struct mm_struct *mm = iommu_sva_domain_mm(domain);
+
+ if (domain->type != IOMMU_DOMAIN_SVA || !mm)
+ return -EINVAL;
+
+ mutex_lock(&sva_lock);
+ handle = __arm_smmu_sva_bind(dev, mm);
+ if (IS_ERR(handle))
+ ret = PTR_ERR(handle);
+ mutex_unlock(&sva_lock);
+
+ return ret;
+}
+
+void arm_smmu_sva_detach_dev_pasid(struct iommu_domain *domain,
+ struct device *dev, ioasid_t id)
+{
+ struct arm_smmu_bond *bond = NULL, *t;
+ struct mm_struct *mm = iommu_sva_domain_mm(domain);
+ struct arm_smmu_master *master = dev_iommu_priv_get(dev);
+
+ mutex_lock(&sva_lock);
+ list_for_each_entry(t, &master->bonds, list) {
+ if (t->mm == mm) {
+ bond = t;
+ break;
+ }
+ }
+
+ if (!WARN_ON(!bond) && refcount_dec_and_test(&bond->refs)) {
+ list_del(&bond->list);
+ arm_smmu_mmu_notifier_put(bond->smmu_mn);
+ kfree(bond);
+ }
+ mutex_unlock(&sva_lock);
+}
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
index afc63fce6107..bd80de0bad98 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -1995,10 +1995,31 @@ static bool arm_smmu_capable(enum iommu_cap cap)
}
}
+static void arm_smmu_sva_domain_free(struct iommu_domain *domain)
+{
+ kfree(domain);
+}
+
+static const struct iommu_domain_ops arm_smmu_sva_domain_ops = {
+ .attach_dev_pasid = arm_smmu_sva_attach_dev_pasid,
+ .detach_dev_pasid = arm_smmu_sva_detach_dev_pasid,
+ .free = arm_smmu_sva_domain_free,
+};
+
static struct iommu_domain *arm_smmu_domain_alloc(unsigned type)
{
struct arm_smmu_domain *smmu_domain;
+ if (type == IOMMU_DOMAIN_SVA) {
+ struct iommu_domain *domain;
+
+ domain = kzalloc(sizeof(*domain), GFP_KERNEL);
+ if (domain)
+ domain->ops = &arm_smmu_sva_domain_ops;
+
+ return domain;
+ }
+
if (type != IOMMU_DOMAIN_UNMANAGED &&
type != IOMMU_DOMAIN_DMA &&
type != IOMMU_DOMAIN_DMA_FQ &&
--
2.25.1
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v5 08/12] iommu/sva: Use attach/detach_pasid_dev in SVA interfaces
2022-05-02 1:48 [PATCH v5 00/12] iommu: SVA and IOPF refactoring Lu Baolu
` (6 preceding siblings ...)
2022-05-02 1:48 ` [PATCH v5 07/12] arm-smmu-v3/sva: " Lu Baolu
@ 2022-05-02 1:48 ` Lu Baolu
2022-05-02 1:48 ` [PATCH v5 09/12] iommu: Remove SVA related callbacks from iommu ops Lu Baolu
` (3 subsequent siblings)
11 siblings, 0 replies; 30+ messages in thread
From: Lu Baolu @ 2022-05-02 1:48 UTC (permalink / raw)
To: Joerg Roedel, Jason Gunthorpe, Christoph Hellwig, Kevin Tian,
Ashok Raj, Will Deacon, Robin Murphy, Jean-Philippe Brucker,
Dave Jiang, Vinod Koul
Cc: Eric Auger, Liu Yi L, Jacob jun Pan, iommu, linux-kernel, Lu Baolu
The existing iommu SVA interfaces are implemented by calling the SVA
specific iommu ops provided by the IOMMU drivers. There's no need for
any SVA specific ops in iommu_ops vector anymore as we can achieve
this through the generic attach/detach_dev_pasid domain ops.
This refactors the IOMMU SVA interfaces implementation by using the
attach/detach_pasid_dev ops and align them with the concept of the
iommu domain. Put the new SVA code in the sva related file in order
to make it self-contained.
Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
include/linux/iommu.h | 44 ++++----
drivers/iommu/iommu-sva-lib.c | 194 ++++++++++++++++++++++++++++++++++
drivers/iommu/iommu.c | 92 ----------------
3 files changed, 217 insertions(+), 113 deletions(-)
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index f582f434c513..c5a16b47cae8 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -683,12 +683,6 @@ int iommu_dev_enable_feature(struct device *dev, enum iommu_dev_features f);
int iommu_dev_disable_feature(struct device *dev, enum iommu_dev_features f);
bool iommu_dev_feature_enabled(struct device *dev, enum iommu_dev_features f);
-struct iommu_sva *iommu_sva_bind_device(struct device *dev,
- struct mm_struct *mm,
- void *drvdata);
-void iommu_sva_unbind_device(struct iommu_sva *handle);
-u32 iommu_sva_get_pasid(struct iommu_sva *handle);
-
int iommu_device_use_default_domain(struct device *dev);
void iommu_device_unuse_default_domain(struct device *dev);
@@ -1030,21 +1024,6 @@ iommu_dev_disable_feature(struct device *dev, enum iommu_dev_features feat)
return -ENODEV;
}
-static inline struct iommu_sva *
-iommu_sva_bind_device(struct device *dev, struct mm_struct *mm, void *drvdata)
-{
- return NULL;
-}
-
-static inline void iommu_sva_unbind_device(struct iommu_sva *handle)
-{
-}
-
-static inline u32 iommu_sva_get_pasid(struct iommu_sva *handle)
-{
- return IOMMU_PASID_INVALID;
-}
-
static inline struct iommu_fwspec *dev_iommu_fwspec_get(struct device *dev)
{
return NULL;
@@ -1086,6 +1065,29 @@ static inline void iommu_detach_device_pasid(struct iommu_domain *domain,
}
#endif /* CONFIG_IOMMU_API */
+#ifdef CONFIG_IOMMU_SVA
+struct iommu_sva *iommu_sva_bind_device(struct device *dev,
+ struct mm_struct *mm,
+ void *drvdata);
+void iommu_sva_unbind_device(struct iommu_sva *handle);
+u32 iommu_sva_get_pasid(struct iommu_sva *handle);
+#else /* CONFIG_IOMMU_SVA */
+static inline struct iommu_sva *
+iommu_sva_bind_device(struct device *dev, struct mm_struct *mm, void *drvdata)
+{
+ return NULL;
+}
+
+static inline void iommu_sva_unbind_device(struct iommu_sva *handle)
+{
+}
+
+static inline u32 iommu_sva_get_pasid(struct iommu_sva *handle)
+{
+ return IOMMU_PASID_INVALID;
+}
+#endif /* CONFIG_IOMMU_SVA */
+
/**
* iommu_map_sgtable - Map the given buffer to the IOMMU domain
* @domain: The IOMMU domain to perform the mapping
diff --git a/drivers/iommu/iommu-sva-lib.c b/drivers/iommu/iommu-sva-lib.c
index d524a402be3b..0524659a4c0c 100644
--- a/drivers/iommu/iommu-sva-lib.c
+++ b/drivers/iommu/iommu-sva-lib.c
@@ -11,6 +11,7 @@
static DEFINE_MUTEX(iommu_sva_lock);
static DECLARE_IOASID_SET(iommu_sva_pasid);
+static DEFINE_XARRAY_ALLOC(iommu_sva_ioas_array);
struct iommu_sva_ioas {
struct mm_struct *mm;
@@ -87,3 +88,196 @@ struct mm_struct *iommu_sva_find(ioasid_t pasid)
return ioasid_find(&iommu_sva_pasid, pasid, __mmget_not_zero);
}
EXPORT_SYMBOL_GPL(iommu_sva_find);
+
+/*
+ * Get or put an ioas for a shared memory.
+ */
+static struct iommu_sva_ioas *iommu_sva_ioas_get(struct mm_struct *mm,
+ ioasid_t pasid)
+{
+ struct iommu_sva_ioas *ioas;
+ int ret;
+
+ ioas = xa_load(&iommu_sva_ioas_array, pasid);
+ if (ioas) {
+ if (WARN_ON(ioas->mm != mm))
+ return ERR_PTR(-EINVAL);
+ refcount_inc(&ioas->users);
+ return ioas;
+ }
+
+ ioas = kzalloc(sizeof(*ioas), GFP_KERNEL);
+ if (!ioas)
+ return ERR_PTR(-ENOMEM);
+
+ ioas->mm = mm;
+ ioas->pasid = pasid;
+ refcount_set(&ioas->users, 1);
+ INIT_LIST_HEAD(&ioas->bonds);
+ ret = xa_err(xa_store(&iommu_sva_ioas_array, pasid, ioas, GFP_KERNEL));
+ if (ret) {
+ kfree(ioas);
+ return ERR_PTR(ret);
+ }
+
+ mmgrab(mm);
+
+ return ioas;
+}
+
+static void iommu_sva_ioas_put(struct iommu_sva_ioas *ioas)
+{
+ if (refcount_dec_and_test(&ioas->users)) {
+ WARN_ON(!list_empty(&ioas->bonds));
+ xa_erase(&iommu_sva_ioas_array, ioas->pasid);
+ mmdrop(ioas->mm);
+ kfree(ioas);
+ }
+}
+
+/*
+ * IOMMU SVA driver-oriented interfaces
+ */
+static struct iommu_domain *
+iommu_sva_alloc_domain(struct device *dev, struct iommu_sva_ioas *ioas)
+{
+ struct bus_type *bus = dev->bus;
+ struct iommu_domain *domain;
+
+ if (!bus || !bus->iommu_ops)
+ return NULL;
+
+ domain = bus->iommu_ops->domain_alloc(IOMMU_DOMAIN_SVA);
+ if (!domain)
+ return NULL;
+
+ /* The caller must hold a reference to ioas. */
+ domain->sva_ioas = ioas;
+ domain->type = IOMMU_DOMAIN_SVA;
+
+ return domain;
+}
+
+/**
+ * iommu_sva_bind_device() - Bind a process address space to a device
+ * @dev: the device
+ * @mm: the mm to bind, caller must hold a reference to it
+ * @drvdata: opaque data pointer to pass to bind callback
+ *
+ * Create a bond between device and address space, allowing the device to access
+ * the mm using the returned PASID. If a bond already exists between @device and
+ * @mm, it is returned and an additional reference is taken. Caller must call
+ * iommu_sva_unbind_device() to release each reference.
+ *
+ * iommu_dev_enable_feature(dev, IOMMU_DEV_FEAT_SVA) must be called first, to
+ * initialize the required SVA features.
+ *
+ * On error, returns an ERR_PTR value.
+ */
+struct iommu_sva *
+iommu_sva_bind_device(struct device *dev, struct mm_struct *mm, void *drvdata)
+{
+ int ret = -EINVAL;
+ struct iommu_sva *handle;
+ struct iommu_domain *domain;
+ struct iommu_sva_ioas *ioas;
+
+ /*
+ * TODO: Remove the drvdata parameter after kernel PASID support is
+ * enabled for the idxd driver.
+ */
+ if (drvdata)
+ return ERR_PTR(-EOPNOTSUPP);
+
+ /* Allocate mm->pasid if necessary. */
+ ret = iommu_sva_alloc_pasid(mm, 1, (1U << dev->iommu->pasid_bits) - 1);
+ if (ret)
+ return ERR_PTR(ret);
+
+ mutex_lock(&iommu_sva_lock);
+ ioas = iommu_sva_ioas_get(mm, mm->pasid);
+ if (IS_ERR(ioas)) {
+ ret = PTR_ERR(ioas);
+ goto out_unlock;
+ }
+
+ /* Search for an existing bond. */
+ list_for_each_entry(handle, &ioas->bonds, node) {
+ if (handle->dev == dev) {
+ refcount_inc(&handle->users);
+ /* No new bond, drop the counter. */
+ iommu_sva_ioas_put(ioas);
+ goto out_success;
+ }
+ }
+
+ handle = kzalloc(sizeof(*handle), GFP_KERNEL);
+ if (!handle) {
+ ret = -ENOMEM;
+ goto out_put_ioas;
+ }
+
+ /* The reference to ioas will be kept until domain free. */
+ domain = iommu_sva_alloc_domain(dev, ioas);
+ if (!domain) {
+ ret = -ENODEV;
+ goto out_free_handle;
+ }
+
+ ret = iommu_attach_device_pasid(domain, dev, mm->pasid);
+ if (ret)
+ goto out_free_domain;
+
+ handle->dev = dev;
+ handle->domain = domain;
+ handle->sva_ioas = ioas;
+ refcount_set(&handle->users, 1);
+ list_add_tail(&handle->node, &ioas->bonds);
+
+out_success:
+ mutex_unlock(&iommu_sva_lock);
+ return handle;
+
+out_free_domain:
+ iommu_domain_free(domain);
+out_free_handle:
+ kfree(handle);
+out_put_ioas:
+ iommu_sva_ioas_put(ioas);
+out_unlock:
+ mutex_unlock(&iommu_sva_lock);
+ return ERR_PTR(ret);
+}
+EXPORT_SYMBOL_GPL(iommu_sva_bind_device);
+
+/**
+ * iommu_sva_unbind_device() - Remove a bond created with iommu_sva_bind_device
+ * @handle: the handle returned by iommu_sva_bind_device()
+ *
+ * Put reference to a bond between device and address space. The device should
+ * not be issuing any more transaction for this PASID. All outstanding page
+ * requests for this PASID must have been flushed to the IOMMU.
+ */
+void iommu_sva_unbind_device(struct iommu_sva *handle)
+{
+ struct device *dev = handle->dev;
+ struct iommu_domain *domain = handle->domain;
+ struct iommu_sva_ioas *ioas = handle->sva_ioas;
+
+ mutex_lock(&iommu_sva_lock);
+ if (refcount_dec_and_test(&handle->users)) {
+ list_del(&handle->node);
+ iommu_detach_device_pasid(domain, dev, ioas->pasid);
+ iommu_domain_free(domain);
+ iommu_sva_ioas_put(ioas);
+ kfree(handle);
+ }
+ mutex_unlock(&iommu_sva_lock);
+}
+EXPORT_SYMBOL_GPL(iommu_sva_unbind_device);
+
+u32 iommu_sva_get_pasid(struct iommu_sva *handle)
+{
+ return handle->sva_ioas->pasid;
+}
+EXPORT_SYMBOL_GPL(iommu_sva_get_pasid);
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 89c9d19ddb28..7cae631c1baa 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -2731,98 +2731,6 @@ bool iommu_dev_feature_enabled(struct device *dev, enum iommu_dev_features feat)
}
EXPORT_SYMBOL_GPL(iommu_dev_feature_enabled);
-/**
- * iommu_sva_bind_device() - Bind a process address space to a device
- * @dev: the device
- * @mm: the mm to bind, caller must hold a reference to it
- * @drvdata: opaque data pointer to pass to bind callback
- *
- * Create a bond between device and address space, allowing the device to access
- * the mm using the returned PASID. If a bond already exists between @device and
- * @mm, it is returned and an additional reference is taken. Caller must call
- * iommu_sva_unbind_device() to release each reference.
- *
- * iommu_dev_enable_feature(dev, IOMMU_DEV_FEAT_SVA) must be called first, to
- * initialize the required SVA features.
- *
- * On error, returns an ERR_PTR value.
- */
-struct iommu_sva *
-iommu_sva_bind_device(struct device *dev, struct mm_struct *mm, void *drvdata)
-{
- struct iommu_group *group;
- struct iommu_sva *handle = ERR_PTR(-EINVAL);
- const struct iommu_ops *ops = dev_iommu_ops(dev);
-
- if (!ops->sva_bind)
- return ERR_PTR(-ENODEV);
-
- group = iommu_group_get(dev);
- if (!group)
- return ERR_PTR(-ENODEV);
-
- /* Ensure device count and domain don't change while we're binding */
- mutex_lock(&group->mutex);
-
- /*
- * To keep things simple, SVA currently doesn't support IOMMU groups
- * with more than one device. Existing SVA-capable systems are not
- * affected by the problems that required IOMMU groups (lack of ACS
- * isolation, device ID aliasing and other hardware issues).
- */
- if (iommu_group_device_count(group) != 1)
- goto out_unlock;
-
- handle = ops->sva_bind(dev, mm, drvdata);
-
-out_unlock:
- mutex_unlock(&group->mutex);
- iommu_group_put(group);
-
- return handle;
-}
-EXPORT_SYMBOL_GPL(iommu_sva_bind_device);
-
-/**
- * iommu_sva_unbind_device() - Remove a bond created with iommu_sva_bind_device
- * @handle: the handle returned by iommu_sva_bind_device()
- *
- * Put reference to a bond between device and address space. The device should
- * not be issuing any more transaction for this PASID. All outstanding page
- * requests for this PASID must have been flushed to the IOMMU.
- */
-void iommu_sva_unbind_device(struct iommu_sva *handle)
-{
- struct iommu_group *group;
- struct device *dev = handle->dev;
- const struct iommu_ops *ops = dev_iommu_ops(dev);
-
- if (!ops->sva_unbind)
- return;
-
- group = iommu_group_get(dev);
- if (!group)
- return;
-
- mutex_lock(&group->mutex);
- ops->sva_unbind(handle);
- mutex_unlock(&group->mutex);
-
- iommu_group_put(group);
-}
-EXPORT_SYMBOL_GPL(iommu_sva_unbind_device);
-
-u32 iommu_sva_get_pasid(struct iommu_sva *handle)
-{
- const struct iommu_ops *ops = dev_iommu_ops(handle->dev);
-
- if (!ops->sva_get_pasid)
- return IOMMU_PASID_INVALID;
-
- return ops->sva_get_pasid(handle);
-}
-EXPORT_SYMBOL_GPL(iommu_sva_get_pasid);
-
/*
* Changes the default domain of an iommu group that has *only* one device
*
--
2.25.1
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v5 09/12] iommu: Remove SVA related callbacks from iommu ops
2022-05-02 1:48 [PATCH v5 00/12] iommu: SVA and IOPF refactoring Lu Baolu
` (7 preceding siblings ...)
2022-05-02 1:48 ` [PATCH v5 08/12] iommu/sva: Use attach/detach_pasid_dev in SVA interfaces Lu Baolu
@ 2022-05-02 1:48 ` Lu Baolu
2022-05-03 18:14 ` Jean-Philippe Brucker
2022-05-02 1:48 ` [PATCH v5 10/12] iommu: Prepare IOMMU domain for IOPF Lu Baolu
` (2 subsequent siblings)
11 siblings, 1 reply; 30+ messages in thread
From: Lu Baolu @ 2022-05-02 1:48 UTC (permalink / raw)
To: Joerg Roedel, Jason Gunthorpe, Christoph Hellwig, Kevin Tian,
Ashok Raj, Will Deacon, Robin Murphy, Jean-Philippe Brucker,
Dave Jiang, Vinod Koul
Cc: Eric Auger, Liu Yi L, Jacob jun Pan, iommu, linux-kernel, Lu Baolu
These ops'es have been replaced with the dev_attach/detach_pasid domain
ops'es. There's no need for them anymore. Remove them to avoid dead
code.
Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
include/linux/intel-iommu.h | 4 --
include/linux/iommu.h | 8 ---
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 17 -------
drivers/iommu/iommu-sva-lib.h | 1 -
.../iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c | 41 ----------------
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 3 --
drivers/iommu/intel/iommu.c | 3 --
drivers/iommu/intel/svm.c | 49 -------------------
drivers/iommu/iommu-sva-lib.c | 4 +-
9 files changed, 2 insertions(+), 128 deletions(-)
diff --git a/include/linux/intel-iommu.h b/include/linux/intel-iommu.h
index 3b4ca16f53e2..5af24befc9f1 100644
--- a/include/linux/intel-iommu.h
+++ b/include/linux/intel-iommu.h
@@ -738,10 +738,6 @@ struct intel_iommu *device_to_iommu(struct device *dev, u8 *bus, u8 *devfn);
extern void intel_svm_check(struct intel_iommu *iommu);
extern int intel_svm_enable_prq(struct intel_iommu *iommu);
extern int intel_svm_finish_prq(struct intel_iommu *iommu);
-struct iommu_sva *intel_svm_bind(struct device *dev, struct mm_struct *mm,
- void *drvdata);
-void intel_svm_unbind(struct iommu_sva *handle);
-u32 intel_svm_get_pasid(struct iommu_sva *handle);
int intel_svm_page_response(struct device *dev, struct iommu_fault_event *evt,
struct iommu_page_response *msg);
extern const struct iommu_domain_ops intel_svm_domain_ops;
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index c5a16b47cae8..19718939d9df 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -214,9 +214,6 @@ struct iommu_iotlb_gather {
* @dev_has/enable/disable_feat: per device entries to check/enable/disable
* iommu specific features.
* @dev_feat_enabled: check enabled feature
- * @sva_bind: Bind process address space to device
- * @sva_unbind: Unbind process address space from device
- * @sva_get_pasid: Get PASID associated to a SVA handle
* @page_response: handle page request response
* @def_domain_type: device default domain type, return value:
* - IOMMU_DOMAIN_IDENTITY: must use an identity domain
@@ -250,11 +247,6 @@ struct iommu_ops {
int (*dev_enable_feat)(struct device *dev, enum iommu_dev_features f);
int (*dev_disable_feat)(struct device *dev, enum iommu_dev_features f);
- struct iommu_sva *(*sva_bind)(struct device *dev, struct mm_struct *mm,
- void *drvdata);
- void (*sva_unbind)(struct iommu_sva *handle);
- u32 (*sva_get_pasid)(struct iommu_sva *handle);
-
int (*page_response)(struct device *dev,
struct iommu_fault_event *evt,
struct iommu_page_response *msg);
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
index 7631c00fdcbd..2513309ec0db 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
@@ -754,10 +754,6 @@ bool arm_smmu_master_sva_enabled(struct arm_smmu_master *master);
int arm_smmu_master_enable_sva(struct arm_smmu_master *master);
int arm_smmu_master_disable_sva(struct arm_smmu_master *master);
bool arm_smmu_master_iopf_supported(struct arm_smmu_master *master);
-struct iommu_sva *arm_smmu_sva_bind(struct device *dev, struct mm_struct *mm,
- void *drvdata);
-void arm_smmu_sva_unbind(struct iommu_sva *handle);
-u32 arm_smmu_sva_get_pasid(struct iommu_sva *handle);
void arm_smmu_sva_notifier_synchronize(void);
int arm_smmu_sva_attach_dev_pasid(struct iommu_domain *domain,
struct device *dev, ioasid_t id);
@@ -794,19 +790,6 @@ static inline bool arm_smmu_master_iopf_supported(struct arm_smmu_master *master
return false;
}
-static inline struct iommu_sva *
-arm_smmu_sva_bind(struct device *dev, struct mm_struct *mm, void *drvdata)
-{
- return ERR_PTR(-ENODEV);
-}
-
-static inline void arm_smmu_sva_unbind(struct iommu_sva *handle) {}
-
-static inline u32 arm_smmu_sva_get_pasid(struct iommu_sva *handle)
-{
- return IOMMU_PASID_INVALID;
-}
-
static inline void arm_smmu_sva_notifier_synchronize(void) {}
static inline int arm_smmu_sva_attach_dev_pasid(struct iommu_domain *domain,
diff --git a/drivers/iommu/iommu-sva-lib.h b/drivers/iommu/iommu-sva-lib.h
index 9c5e108e2c8a..5776b4c80cc1 100644
--- a/drivers/iommu/iommu-sva-lib.h
+++ b/drivers/iommu/iommu-sva-lib.h
@@ -8,7 +8,6 @@
#include <linux/ioasid.h>
#include <linux/mm_types.h>
-int iommu_sva_alloc_pasid(struct mm_struct *mm, ioasid_t min, ioasid_t max);
struct mm_struct *iommu_sva_find(ioasid_t pasid);
struct mm_struct *iommu_sva_domain_mm(struct iommu_domain *domain);
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
index 3b843cd3ed67..0ace04b27d4b 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
@@ -335,11 +335,6 @@ __arm_smmu_sva_bind(struct device *dev, struct mm_struct *mm)
if (!bond)
return ERR_PTR(-ENOMEM);
- /* Allocate a PASID for this mm if necessary */
- ret = iommu_sva_alloc_pasid(mm, 1, (1U << master->ssid_bits) - 1);
- if (ret)
- goto err_free_bond;
-
bond->mm = mm;
bond->sva.dev = dev;
refcount_set(&bond->refs, 1);
@@ -358,42 +353,6 @@ __arm_smmu_sva_bind(struct device *dev, struct mm_struct *mm)
return ERR_PTR(ret);
}
-struct iommu_sva *
-arm_smmu_sva_bind(struct device *dev, struct mm_struct *mm, void *drvdata)
-{
- struct iommu_sva *handle;
- struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
- struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
-
- if (smmu_domain->stage != ARM_SMMU_DOMAIN_S1)
- return ERR_PTR(-EINVAL);
-
- mutex_lock(&sva_lock);
- handle = __arm_smmu_sva_bind(dev, mm);
- mutex_unlock(&sva_lock);
- return handle;
-}
-
-void arm_smmu_sva_unbind(struct iommu_sva *handle)
-{
- struct arm_smmu_bond *bond = sva_to_bond(handle);
-
- mutex_lock(&sva_lock);
- if (refcount_dec_and_test(&bond->refs)) {
- list_del(&bond->list);
- arm_smmu_mmu_notifier_put(bond->smmu_mn);
- kfree(bond);
- }
- mutex_unlock(&sva_lock);
-}
-
-u32 arm_smmu_sva_get_pasid(struct iommu_sva *handle)
-{
- struct arm_smmu_bond *bond = sva_to_bond(handle);
-
- return bond->mm->pasid;
-}
-
bool arm_smmu_sva_supported(struct arm_smmu_device *smmu)
{
unsigned long reg, fld;
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
index bd80de0bad98..543d3ef1c102 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -2875,9 +2875,6 @@ static struct iommu_ops arm_smmu_ops = {
.dev_feat_enabled = arm_smmu_dev_feature_enabled,
.dev_enable_feat = arm_smmu_dev_enable_feature,
.dev_disable_feat = arm_smmu_dev_disable_feature,
- .sva_bind = arm_smmu_sva_bind,
- .sva_unbind = arm_smmu_sva_unbind,
- .sva_get_pasid = arm_smmu_sva_get_pasid,
.page_response = arm_smmu_page_response,
.pgsize_bitmap = -1UL, /* Restricted during device attach */
.owner = THIS_MODULE,
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 5d1d3e325afa..46e2eb15197b 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -4914,9 +4914,6 @@ const struct iommu_ops intel_iommu_ops = {
.def_domain_type = device_def_domain_type,
.pgsize_bitmap = SZ_4K,
#ifdef CONFIG_INTEL_IOMMU_SVM
- .sva_bind = intel_svm_bind,
- .sva_unbind = intel_svm_unbind,
- .sva_get_pasid = intel_svm_get_pasid,
.page_response = intel_svm_page_response,
#endif
.default_domain_ops = &(const struct iommu_domain_ops) {
diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c
index ec684d883014..6084f960ba27 100644
--- a/drivers/iommu/intel/svm.c
+++ b/drivers/iommu/intel/svm.c
@@ -313,14 +313,6 @@ static int pasid_to_svm_sdev(struct device *dev, unsigned int pasid,
return 0;
}
-static int intel_svm_alloc_pasid(struct device *dev, struct mm_struct *mm)
-{
- ioasid_t max_pasid = dev_is_pci(dev) ?
- pci_max_pasids(to_pci_dev(dev)) : intel_pasid_max_id;
-
- return iommu_sva_alloc_pasid(mm, PASID_MIN, max_pasid - 1);
-}
-
static struct iommu_sva *intel_svm_bind_mm(struct intel_iommu *iommu,
struct device *dev,
struct mm_struct *mm)
@@ -809,47 +801,6 @@ static irqreturn_t prq_event_thread(int irq, void *d)
return IRQ_RETVAL(handled);
}
-struct iommu_sva *intel_svm_bind(struct device *dev, struct mm_struct *mm, void *drvdata)
-{
- struct intel_iommu *iommu = device_to_iommu(dev, NULL, NULL);
- struct iommu_sva *sva;
- int ret;
-
- mutex_lock(&pasid_mutex);
- ret = intel_svm_alloc_pasid(dev, mm);
- if (ret) {
- mutex_unlock(&pasid_mutex);
- return ERR_PTR(ret);
- }
-
- sva = intel_svm_bind_mm(iommu, dev, mm);
- mutex_unlock(&pasid_mutex);
-
- return sva;
-}
-
-void intel_svm_unbind(struct iommu_sva *sva)
-{
- struct intel_svm_dev *sdev = to_intel_svm_dev(sva);
-
- mutex_lock(&pasid_mutex);
- intel_svm_unbind_mm(sdev->dev, sdev->pasid);
- mutex_unlock(&pasid_mutex);
-}
-
-u32 intel_svm_get_pasid(struct iommu_sva *sva)
-{
- struct intel_svm_dev *sdev;
- u32 pasid;
-
- mutex_lock(&pasid_mutex);
- sdev = to_intel_svm_dev(sva);
- pasid = sdev->pasid;
- mutex_unlock(&pasid_mutex);
-
- return pasid;
-}
-
int intel_svm_page_response(struct device *dev,
struct iommu_fault_event *evt,
struct iommu_page_response *msg)
diff --git a/drivers/iommu/iommu-sva-lib.c b/drivers/iommu/iommu-sva-lib.c
index 0524659a4c0c..992388106da0 100644
--- a/drivers/iommu/iommu-sva-lib.c
+++ b/drivers/iommu/iommu-sva-lib.c
@@ -41,7 +41,8 @@ struct mm_struct *iommu_sva_domain_mm(struct iommu_domain *domain)
*
* Returns 0 on success and < 0 on error.
*/
-int iommu_sva_alloc_pasid(struct mm_struct *mm, ioasid_t min, ioasid_t max)
+static int iommu_sva_alloc_pasid(struct mm_struct *mm,
+ ioasid_t min, ioasid_t max)
{
int ret = 0;
ioasid_t pasid;
@@ -67,7 +68,6 @@ int iommu_sva_alloc_pasid(struct mm_struct *mm, ioasid_t min, ioasid_t max)
mutex_unlock(&iommu_sva_lock);
return ret;
}
-EXPORT_SYMBOL_GPL(iommu_sva_alloc_pasid);
/* ioasid_find getter() requires a void * argument */
static bool __mmget_not_zero(void *mm)
--
2.25.1
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v5 10/12] iommu: Prepare IOMMU domain for IOPF
2022-05-02 1:48 [PATCH v5 00/12] iommu: SVA and IOPF refactoring Lu Baolu
` (8 preceding siblings ...)
2022-05-02 1:48 ` [PATCH v5 09/12] iommu: Remove SVA related callbacks from iommu ops Lu Baolu
@ 2022-05-02 1:48 ` Lu Baolu
2022-05-03 18:20 ` Jean-Philippe Brucker
2022-05-02 1:48 ` [PATCH v5 11/12] iommu: Per-domain I/O page fault handling Lu Baolu
2022-05-02 1:48 ` [PATCH v5 12/12] iommu: Rename iommu-sva-lib.{c,h} Lu Baolu
11 siblings, 1 reply; 30+ messages in thread
From: Lu Baolu @ 2022-05-02 1:48 UTC (permalink / raw)
To: Joerg Roedel, Jason Gunthorpe, Christoph Hellwig, Kevin Tian,
Ashok Raj, Will Deacon, Robin Murphy, Jean-Philippe Brucker,
Dave Jiang, Vinod Koul
Cc: Eric Auger, Liu Yi L, Jacob jun Pan, iommu, linux-kernel, Lu Baolu
This adds some mechanisms around the iommu_domain so that the I/O page
fault handling framework could route a page fault to the domain and
call the fault handler from it.
Add pointers to the page fault handler and its private data in struct
iommu_domain. The fault handler will be called with the private data
as a parameter once a page fault is routed to the domain. Any kernel
component which owns an iommu domain could install handler and its
private parameter so that the page fault could be further routed and
handled.
A new helper iommu_get_domain_for_dev_pasid() which retrieves attached
domain for a {device, PASID} is added. It will be used by the page fault
handling framework which knows {device, PASID} reported from the iommu
driver. We have a guarantee that the SVA domain doesn't go away during
IOPF handling, because unbind() waits for pending faults with
iopf_queue_flush_dev() before freeing the domain. Hence, there's no need
to synchronize life cycle of the iommu domains between the unbind() and
the interrupt threads.
This also prepares the SVA implementation to be the first consumer of
the per-domain page fault handling model.
Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
include/linux/iommu.h | 12 +++++++
drivers/iommu/iommu-sva-lib.c | 65 +++++++++++++++++++++++++++++++++++
drivers/iommu/iommu.c | 21 +++++++++++
3 files changed, 98 insertions(+)
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 19718939d9df..1164524814cb 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -102,6 +102,9 @@ struct iommu_domain {
struct iommu_domain_geometry geometry;
struct iommu_dma_cookie *iova_cookie;
struct iommu_sva_ioas *sva_ioas;
+ enum iommu_page_response_code (*iopf_handler)(struct iommu_fault *fault,
+ void *data);
+ void *fault_data;
};
static inline bool iommu_is_dma_domain(struct iommu_domain *domain)
@@ -686,6 +689,9 @@ int iommu_attach_device_pasid(struct iommu_domain *domain,
struct device *dev, ioasid_t pasid);
void iommu_detach_device_pasid(struct iommu_domain *domain,
struct device *dev, ioasid_t pasid);
+struct iommu_domain *
+iommu_get_domain_for_dev_pasid(struct device *dev, ioasid_t pasid);
+
#else /* CONFIG_IOMMU_API */
struct iommu_ops {};
@@ -1055,6 +1061,12 @@ static inline void iommu_detach_device_pasid(struct iommu_domain *domain,
struct device *dev, ioasid_t pasid)
{
}
+
+static inline struct iommu_domain *
+iommu_get_domain_for_dev_pasid(struct device *dev, ioasid_t pasid)
+{
+ return NULL;
+}
#endif /* CONFIG_IOMMU_API */
#ifdef CONFIG_IOMMU_SVA
diff --git a/drivers/iommu/iommu-sva-lib.c b/drivers/iommu/iommu-sva-lib.c
index 992388106da0..05a7d2f0e46f 100644
--- a/drivers/iommu/iommu-sva-lib.c
+++ b/drivers/iommu/iommu-sva-lib.c
@@ -135,6 +135,69 @@ static void iommu_sva_ioas_put(struct iommu_sva_ioas *ioas)
}
}
+/*
+ * I/O page fault handler for SVA
+ *
+ * Copied from io-pgfault.c with mmget_not_zero() added before
+ * mmap_read_lock().
+ */
+static enum iommu_page_response_code
+iommu_sva_handle_iopf(struct iommu_fault *fault, void *data)
+{
+ vm_fault_t ret;
+ struct mm_struct *mm;
+ struct vm_area_struct *vma;
+ unsigned int access_flags = 0;
+ struct iommu_domain *domain = data;
+ unsigned int fault_flags = FAULT_FLAG_REMOTE;
+ struct iommu_fault_page_request *prm = &fault->prm;
+ enum iommu_page_response_code status = IOMMU_PAGE_RESP_INVALID;
+
+ if (!(prm->flags & IOMMU_FAULT_PAGE_REQUEST_PASID_VALID))
+ return status;
+
+ mm = iommu_sva_domain_mm(domain);
+ if (IS_ERR_OR_NULL(mm) || !mmget_not_zero(mm))
+ return status;
+
+ mmap_read_lock(mm);
+
+ vma = find_extend_vma(mm, prm->addr);
+ if (!vma)
+ /* Unmapped area */
+ goto out_put_mm;
+
+ if (prm->perm & IOMMU_FAULT_PERM_READ)
+ access_flags |= VM_READ;
+
+ if (prm->perm & IOMMU_FAULT_PERM_WRITE) {
+ access_flags |= VM_WRITE;
+ fault_flags |= FAULT_FLAG_WRITE;
+ }
+
+ if (prm->perm & IOMMU_FAULT_PERM_EXEC) {
+ access_flags |= VM_EXEC;
+ fault_flags |= FAULT_FLAG_INSTRUCTION;
+ }
+
+ if (!(prm->perm & IOMMU_FAULT_PERM_PRIV))
+ fault_flags |= FAULT_FLAG_USER;
+
+ if (access_flags & ~vma->vm_flags)
+ /* Access fault */
+ goto out_put_mm;
+
+ ret = handle_mm_fault(vma, prm->addr, fault_flags, NULL);
+ status = ret & VM_FAULT_ERROR ? IOMMU_PAGE_RESP_INVALID :
+ IOMMU_PAGE_RESP_SUCCESS;
+
+out_put_mm:
+ mmap_read_unlock(mm);
+ mmput(mm);
+
+ return status;
+}
+
/*
* IOMMU SVA driver-oriented interfaces
*/
@@ -154,6 +217,8 @@ iommu_sva_alloc_domain(struct device *dev, struct iommu_sva_ioas *ioas)
/* The caller must hold a reference to ioas. */
domain->sva_ioas = ioas;
domain->type = IOMMU_DOMAIN_SVA;
+ domain->iopf_handler = iommu_sva_handle_iopf;
+ domain->fault_data = domain;
return domain;
}
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 7cae631c1baa..33449523afbe 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -3174,3 +3174,24 @@ void iommu_detach_device_pasid(struct iommu_domain *domain,
iommu_group_put(group);
}
+
+struct iommu_domain *iommu_get_domain_for_dev_pasid(struct device *dev,
+ ioasid_t pasid)
+{
+ struct iommu_domain *domain;
+ struct iommu_group *group;
+
+ if (!pasid_valid(pasid))
+ return NULL;
+
+ group = iommu_group_get(dev);
+ if (!group)
+ return NULL;
+
+ mutex_lock(&group->mutex);
+ domain = xa_load(&group->pasid_array, pasid);
+ mutex_unlock(&group->mutex);
+ iommu_group_put(group);
+
+ return domain;
+}
--
2.25.1
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v5 11/12] iommu: Per-domain I/O page fault handling
2022-05-02 1:48 [PATCH v5 00/12] iommu: SVA and IOPF refactoring Lu Baolu
` (9 preceding siblings ...)
2022-05-02 1:48 ` [PATCH v5 10/12] iommu: Prepare IOMMU domain for IOPF Lu Baolu
@ 2022-05-02 1:48 ` Lu Baolu
2022-05-03 18:27 ` Jean-Philippe Brucker
2022-05-02 1:48 ` [PATCH v5 12/12] iommu: Rename iommu-sva-lib.{c,h} Lu Baolu
11 siblings, 1 reply; 30+ messages in thread
From: Lu Baolu @ 2022-05-02 1:48 UTC (permalink / raw)
To: Joerg Roedel, Jason Gunthorpe, Christoph Hellwig, Kevin Tian,
Ashok Raj, Will Deacon, Robin Murphy, Jean-Philippe Brucker,
Dave Jiang, Vinod Koul
Cc: Eric Auger, Liu Yi L, Jacob jun Pan, iommu, linux-kernel, Lu Baolu
Tweak the I/O page fault handling framework to route the page faults to
the domain and call the page fault handler retrieved from the domain.
This makes the I/O page fault handling framework possible to serve more
usage scenarios as long as they have an IOMMU domain and install a page
fault handler in it. Some unused functions are also removed to avoid
dead code.
Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
drivers/iommu/iommu-sva-lib.h | 1 -
drivers/iommu/io-pgfault.c | 64 ++++-------------------------------
drivers/iommu/iommu-sva-lib.c | 20 -----------
3 files changed, 7 insertions(+), 78 deletions(-)
diff --git a/drivers/iommu/iommu-sva-lib.h b/drivers/iommu/iommu-sva-lib.h
index 5776b4c80cc1..e7813c6706fb 100644
--- a/drivers/iommu/iommu-sva-lib.h
+++ b/drivers/iommu/iommu-sva-lib.h
@@ -8,7 +8,6 @@
#include <linux/ioasid.h>
#include <linux/mm_types.h>
-struct mm_struct *iommu_sva_find(ioasid_t pasid);
struct mm_struct *iommu_sva_domain_mm(struct iommu_domain *domain);
/* I/O Page fault */
diff --git a/drivers/iommu/io-pgfault.c b/drivers/iommu/io-pgfault.c
index 1df8c1dcae77..8a2bb56e1474 100644
--- a/drivers/iommu/io-pgfault.c
+++ b/drivers/iommu/io-pgfault.c
@@ -69,69 +69,18 @@ static int iopf_complete_group(struct device *dev, struct iopf_fault *iopf,
return iommu_page_response(dev, &resp);
}
-static enum iommu_page_response_code
-iopf_handle_single(struct iopf_fault *iopf)
-{
- vm_fault_t ret;
- struct mm_struct *mm;
- struct vm_area_struct *vma;
- unsigned int access_flags = 0;
- unsigned int fault_flags = FAULT_FLAG_REMOTE;
- struct iommu_fault_page_request *prm = &iopf->fault.prm;
- enum iommu_page_response_code status = IOMMU_PAGE_RESP_INVALID;
-
- if (!(prm->flags & IOMMU_FAULT_PAGE_REQUEST_PASID_VALID))
- return status;
-
- mm = iommu_sva_find(prm->pasid);
- if (IS_ERR_OR_NULL(mm))
- return status;
-
- mmap_read_lock(mm);
-
- vma = find_extend_vma(mm, prm->addr);
- if (!vma)
- /* Unmapped area */
- goto out_put_mm;
-
- if (prm->perm & IOMMU_FAULT_PERM_READ)
- access_flags |= VM_READ;
-
- if (prm->perm & IOMMU_FAULT_PERM_WRITE) {
- access_flags |= VM_WRITE;
- fault_flags |= FAULT_FLAG_WRITE;
- }
-
- if (prm->perm & IOMMU_FAULT_PERM_EXEC) {
- access_flags |= VM_EXEC;
- fault_flags |= FAULT_FLAG_INSTRUCTION;
- }
-
- if (!(prm->perm & IOMMU_FAULT_PERM_PRIV))
- fault_flags |= FAULT_FLAG_USER;
-
- if (access_flags & ~vma->vm_flags)
- /* Access fault */
- goto out_put_mm;
-
- ret = handle_mm_fault(vma, prm->addr, fault_flags, NULL);
- status = ret & VM_FAULT_ERROR ? IOMMU_PAGE_RESP_INVALID :
- IOMMU_PAGE_RESP_SUCCESS;
-
-out_put_mm:
- mmap_read_unlock(mm);
- mmput(mm);
-
- return status;
-}
-
static void iopf_handle_group(struct work_struct *work)
{
struct iopf_group *group;
+ struct iommu_domain *domain;
struct iopf_fault *iopf, *next;
enum iommu_page_response_code status = IOMMU_PAGE_RESP_SUCCESS;
group = container_of(work, struct iopf_group, work);
+ domain = iommu_get_domain_for_dev_pasid(group->dev,
+ group->last_fault.fault.prm.pasid);
+ if (!domain || !domain->iopf_handler)
+ status = IOMMU_PAGE_RESP_INVALID;
list_for_each_entry_safe(iopf, next, &group->faults, list) {
/*
@@ -139,7 +88,8 @@ static void iopf_handle_group(struct work_struct *work)
* faults in the group if there is an error.
*/
if (status == IOMMU_PAGE_RESP_SUCCESS)
- status = iopf_handle_single(iopf);
+ status = domain->iopf_handler(&iopf->fault,
+ domain->fault_data);
if (!(iopf->fault.prm.flags &
IOMMU_FAULT_PAGE_REQUEST_LAST_PAGE))
diff --git a/drivers/iommu/iommu-sva-lib.c b/drivers/iommu/iommu-sva-lib.c
index 05a7d2f0e46f..ae3595d60f38 100644
--- a/drivers/iommu/iommu-sva-lib.c
+++ b/drivers/iommu/iommu-sva-lib.c
@@ -69,26 +69,6 @@ static int iommu_sva_alloc_pasid(struct mm_struct *mm,
return ret;
}
-/* ioasid_find getter() requires a void * argument */
-static bool __mmget_not_zero(void *mm)
-{
- return mmget_not_zero(mm);
-}
-
-/**
- * iommu_sva_find() - Find mm associated to the given PASID
- * @pasid: Process Address Space ID assigned to the mm
- *
- * On success a reference to the mm is taken, and must be released with mmput().
- *
- * Returns the mm corresponding to this PASID, or an error if not found.
- */
-struct mm_struct *iommu_sva_find(ioasid_t pasid)
-{
- return ioasid_find(&iommu_sva_pasid, pasid, __mmget_not_zero);
-}
-EXPORT_SYMBOL_GPL(iommu_sva_find);
-
/*
* Get or put an ioas for a shared memory.
*/
--
2.25.1
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v5 12/12] iommu: Rename iommu-sva-lib.{c,h}
2022-05-02 1:48 [PATCH v5 00/12] iommu: SVA and IOPF refactoring Lu Baolu
` (10 preceding siblings ...)
2022-05-02 1:48 ` [PATCH v5 11/12] iommu: Per-domain I/O page fault handling Lu Baolu
@ 2022-05-02 1:48 ` Lu Baolu
2022-05-03 18:28 ` Jean-Philippe Brucker
11 siblings, 1 reply; 30+ messages in thread
From: Lu Baolu @ 2022-05-02 1:48 UTC (permalink / raw)
To: Joerg Roedel, Jason Gunthorpe, Christoph Hellwig, Kevin Tian,
Ashok Raj, Will Deacon, Robin Murphy, Jean-Philippe Brucker,
Dave Jiang, Vinod Koul
Cc: Eric Auger, Liu Yi L, Jacob jun Pan, iommu, linux-kernel, Lu Baolu
Rename iommu-sva-lib.c[h] to iommu-sva.c[h] as it contains all code
for SVA implementation in iommu core.
Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
drivers/iommu/{iommu-sva-lib.h => iommu-sva.h} | 0
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c | 2 +-
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 2 +-
drivers/iommu/intel/iommu.c | 2 +-
drivers/iommu/intel/svm.c | 2 +-
drivers/iommu/io-pgfault.c | 2 +-
drivers/iommu/{iommu-sva-lib.c => iommu-sva.c} | 2 +-
drivers/iommu/Makefile | 2 +-
8 files changed, 7 insertions(+), 7 deletions(-)
rename drivers/iommu/{iommu-sva-lib.h => iommu-sva.h} (100%)
rename drivers/iommu/{iommu-sva-lib.c => iommu-sva.c} (99%)
diff --git a/drivers/iommu/iommu-sva-lib.h b/drivers/iommu/iommu-sva.h
similarity index 100%
rename from drivers/iommu/iommu-sva-lib.h
rename to drivers/iommu/iommu-sva.h
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
index 0ace04b27d4b..73a336e17dc8 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
@@ -9,7 +9,7 @@
#include <linux/slab.h>
#include "arm-smmu-v3.h"
-#include "../../iommu-sva-lib.h"
+#include "../../iommu-sva.h"
#include "../../io-pgtable-arm.h"
struct arm_smmu_mmu_notifier {
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
index 543d3ef1c102..ca2bd17eec41 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -31,7 +31,7 @@
#include <linux/amba/bus.h>
#include "arm-smmu-v3.h"
-#include "../../iommu-sva-lib.h"
+#include "../../iommu-sva.h"
static bool disable_bypass = true;
module_param(disable_bypass, bool, 0444);
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 46e2eb15197b..b38f50810459 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -27,7 +27,7 @@
#include <linux/tboot.h>
#include "../irq_remapping.h"
-#include "../iommu-sva-lib.h"
+#include "../iommu-sva.h"
#include "pasid.h"
#include "cap_audit.h"
diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c
index 6084f960ba27..38c33cde177e 100644
--- a/drivers/iommu/intel/svm.c
+++ b/drivers/iommu/intel/svm.c
@@ -25,7 +25,7 @@
#include "pasid.h"
#include "perf.h"
-#include "../iommu-sva-lib.h"
+#include "../iommu-sva.h"
static irqreturn_t prq_event_thread(int irq, void *d);
static void intel_svm_drain_prq(struct device *dev, u32 pasid);
diff --git a/drivers/iommu/io-pgfault.c b/drivers/iommu/io-pgfault.c
index 8a2bb56e1474..a9ecf6bf5500 100644
--- a/drivers/iommu/io-pgfault.c
+++ b/drivers/iommu/io-pgfault.c
@@ -11,7 +11,7 @@
#include <linux/slab.h>
#include <linux/workqueue.h>
-#include "iommu-sva-lib.h"
+#include "iommu-sva.h"
/**
* struct iopf_queue - IO Page Fault queue
diff --git a/drivers/iommu/iommu-sva-lib.c b/drivers/iommu/iommu-sva.c
similarity index 99%
rename from drivers/iommu/iommu-sva-lib.c
rename to drivers/iommu/iommu-sva.c
index ae3595d60f38..b631765fa8c0 100644
--- a/drivers/iommu/iommu-sva-lib.c
+++ b/drivers/iommu/iommu-sva.c
@@ -7,7 +7,7 @@
#include <linux/slab.h>
#include <linux/sched/mm.h>
-#include "iommu-sva-lib.h"
+#include "iommu-sva.h"
static DEFINE_MUTEX(iommu_sva_lock);
static DECLARE_IOASID_SET(iommu_sva_pasid);
diff --git a/drivers/iommu/Makefile b/drivers/iommu/Makefile
index 44475a9b3eea..c1763476162b 100644
--- a/drivers/iommu/Makefile
+++ b/drivers/iommu/Makefile
@@ -27,6 +27,6 @@ obj-$(CONFIG_FSL_PAMU) += fsl_pamu.o fsl_pamu_domain.o
obj-$(CONFIG_S390_IOMMU) += s390-iommu.o
obj-$(CONFIG_HYPERV_IOMMU) += hyperv-iommu.o
obj-$(CONFIG_VIRTIO_IOMMU) += virtio-iommu.o
-obj-$(CONFIG_IOMMU_SVA) += iommu-sva-lib.o io-pgfault.o
+obj-$(CONFIG_IOMMU_SVA) += iommu-sva.o io-pgfault.o
obj-$(CONFIG_SPRD_IOMMU) += sprd-iommu.o
obj-$(CONFIG_APPLE_DART) += apple-dart.o
--
2.25.1
^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH v5 02/12] iommu: Add pasid_bits field in struct dev_iommu
2022-05-02 1:48 ` [PATCH v5 02/12] iommu: Add pasid_bits field in struct dev_iommu Lu Baolu
@ 2022-05-03 18:02 ` Jean-Philippe Brucker
2022-05-05 6:25 ` Baolu Lu
0 siblings, 1 reply; 30+ messages in thread
From: Jean-Philippe Brucker @ 2022-05-03 18:02 UTC (permalink / raw)
To: Lu Baolu
Cc: Joerg Roedel, Jason Gunthorpe, Christoph Hellwig, Kevin Tian,
Ashok Raj, Will Deacon, Robin Murphy, Jean-Philippe Brucker,
Dave Jiang, Vinod Koul, Eric Auger, Liu Yi L, Jacob jun Pan,
iommu, linux-kernel
On Mon, May 02, 2022 at 09:48:32AM +0800, Lu Baolu wrote:
> Use this field to save the pasid/ssid bits that a device is able to
> support with its IOMMU hardware. It is a generic attribute of a device
> and lifting it into the per-device dev_iommu struct makes it possible
> to allocate a PASID for device without calls into the IOMMU drivers.
> Any iommu driver which suports PASID related features should set this
> field before features are enabled on the devices.
>
> For initialization of this field in the VT-d driver, the
> info->pasid_supported is only set for PCI devices. So the status is
> that non-PCI SVA hasn't been supported yet. Setting this field only for
> PCI devices has no functional change.
>
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
Reviewed-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> ---
> include/linux/iommu.h | 1 +
> drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 2 ++
> drivers/iommu/intel/iommu.c | 5 ++++-
> 3 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index 5e1afe169549..b8ffaf2cb1d0 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -373,6 +373,7 @@ struct dev_iommu {
> struct iommu_fwspec *fwspec;
> struct iommu_device *iommu_dev;
> void *priv;
> + unsigned int pasid_bits;
> };
>
> int iommu_device_register(struct iommu_device *iommu,
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> index 627a3ed5ee8f..afc63fce6107 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -2681,6 +2681,8 @@ static struct iommu_device *arm_smmu_probe_device(struct device *dev)
> smmu->features & ARM_SMMU_FEAT_STALL_FORCE)
> master->stall_enabled = true;
>
> + dev->iommu->pasid_bits = master->ssid_bits;
> +
> return &smmu->iommu;
>
> err_free_master:
> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> index cf43e8f9091b..170eb777d57b 100644
> --- a/drivers/iommu/intel/iommu.c
> +++ b/drivers/iommu/intel/iommu.c
> @@ -4611,8 +4611,11 @@ static struct iommu_device *intel_iommu_probe_device(struct device *dev)
> if (pasid_supported(iommu)) {
> int features = pci_pasid_features(pdev);
>
> - if (features >= 0)
> + if (features >= 0) {
> info->pasid_supported = features | 1;
> + dev->iommu->pasid_bits =
> + fls(pci_max_pasids(pdev)) - 1;
> + }
> }
>
> if (info->ats_supported && ecap_prs(iommu->ecap) &&
> --
> 2.25.1
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v5 03/12] iommu: Add attach/detach_dev_pasid domain ops
2022-05-02 1:48 ` [PATCH v5 03/12] iommu: Add attach/detach_dev_pasid domain ops Lu Baolu
@ 2022-05-03 18:07 ` Jean-Philippe Brucker
2022-05-05 6:28 ` Baolu Lu
0 siblings, 1 reply; 30+ messages in thread
From: Jean-Philippe Brucker @ 2022-05-03 18:07 UTC (permalink / raw)
To: Lu Baolu
Cc: Joerg Roedel, Jason Gunthorpe, Christoph Hellwig, Kevin Tian,
Ashok Raj, Will Deacon, Robin Murphy, Jean-Philippe Brucker,
Dave Jiang, Vinod Koul, Eric Auger, Liu Yi L, Jacob jun Pan,
iommu, linux-kernel
On Mon, May 02, 2022 at 09:48:33AM +0800, Lu Baolu wrote:
> Attaching an IOMMU domain to a PASID of a device is a generic operation
> for modern IOMMU drivers which support PASID-granular DMA address
> translation. Currently visible usage scenarios include (but not limited):
>
> - SVA (Shared Virtual Address)
> - kernel DMA with PASID
> - hardware-assist mediated device
>
> This adds a pair of common domain ops for this purpose and adds helpers
> to attach/detach a domain to/from a {device, PASID}. Some buses, like
> PCI, route packets without considering the PASID value. Thus a DMA target
> address with PASID might be treated as P2P if the address falls into the
> MMIO BAR of other devices in the group. To make things simple, these
> interfaces only apply to devices belonging to the singleton groups, and
> the singleton is immutable in fabric i.e. not affected by hotplug.
>
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
Reviewed-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
just a nit below
> ---
> include/linux/iommu.h | 21 ++++++++++++
> drivers/iommu/iommu.c | 76 +++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 97 insertions(+)
>
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index b8ffaf2cb1d0..ab36244d4e94 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -263,6 +263,8 @@ struct iommu_ops {
> * struct iommu_domain_ops - domain specific operations
> * @attach_dev: attach an iommu domain to a device
> * @detach_dev: detach an iommu domain from a device
> + * @attach_dev_pasid: attach an iommu domain to a pasid of device
> + * @detach_dev_pasid: detach an iommu domain from a pasid of device
> * @map: map a physically contiguous memory region to an iommu domain
> * @map_pages: map a physically contiguous set of pages of the same size to
> * an iommu domain.
> @@ -283,6 +285,10 @@ struct iommu_ops {
> struct iommu_domain_ops {
> int (*attach_dev)(struct iommu_domain *domain, struct device *dev);
> void (*detach_dev)(struct iommu_domain *domain, struct device *dev);
> + int (*attach_dev_pasid)(struct iommu_domain *domain,
> + struct device *dev, ioasid_t pasid);
> + void (*detach_dev_pasid)(struct iommu_domain *domain,
> + struct device *dev, ioasid_t pasid);
>
> int (*map)(struct iommu_domain *domain, unsigned long iova,
> phys_addr_t paddr, size_t size, int prot, gfp_t gfp);
> @@ -678,6 +684,10 @@ int iommu_group_claim_dma_owner(struct iommu_group *group, void *owner);
> void iommu_group_release_dma_owner(struct iommu_group *group);
> bool iommu_group_dma_owner_claimed(struct iommu_group *group);
>
> +int iommu_attach_device_pasid(struct iommu_domain *domain,
> + struct device *dev, ioasid_t pasid);
> +void iommu_detach_device_pasid(struct iommu_domain *domain,
> + struct device *dev, ioasid_t pasid);
> #else /* CONFIG_IOMMU_API */
>
> struct iommu_ops {};
> @@ -1051,6 +1061,17 @@ static inline bool iommu_group_dma_owner_claimed(struct iommu_group *group)
> {
> return false;
> }
> +
> +static inline int iommu_attach_device_pasid(struct iommu_domain *domain,
> + struct device *dev, ioasid_t pasid)
> +{
> + return -ENODEV;
> +}
> +
> +static inline void iommu_detach_device_pasid(struct iommu_domain *domain,
> + struct device *dev, ioasid_t pasid)
> +{
> +}
> #endif /* CONFIG_IOMMU_API */
>
> /**
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 29906bc16371..89c9d19ddb28 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -38,6 +38,7 @@ struct iommu_group {
> struct kobject kobj;
> struct kobject *devices_kobj;
> struct list_head devices;
> + struct xarray pasid_array;
> struct mutex mutex;
> void *iommu_data;
> void (*iommu_data_release)(void *iommu_data);
> @@ -630,6 +631,7 @@ struct iommu_group *iommu_group_alloc(void)
> mutex_init(&group->mutex);
> INIT_LIST_HEAD(&group->devices);
> INIT_LIST_HEAD(&group->entry);
> + xa_init(&group->pasid_array);
>
> ret = ida_simple_get(&iommu_group_ida, 0, 0, GFP_KERNEL);
> if (ret < 0) {
> @@ -3190,3 +3192,77 @@ bool iommu_group_dma_owner_claimed(struct iommu_group *group)
> return user;
> }
> EXPORT_SYMBOL_GPL(iommu_group_dma_owner_claimed);
> +
> +/*
> + * Use standard PCI bus topology and isolation features to check immutable
> + * singleton. Otherwise, assume the bus is static and then singleton can
> + * know from the device count in the group.
> + */
The comment doesn't really add anything that can't be directly understood
from the code.
> +static bool device_group_immutable_singleton(struct device *dev)
> +{
> + struct iommu_group *group = iommu_group_get(dev);
> + int count;
> +
> + if (!group)
> + return false;
> +
> + mutex_lock(&group->mutex);
> + count = iommu_group_device_count(group);
> + mutex_unlock(&group->mutex);
> + iommu_group_put(group);
> +
> + if (count != 1)
> + return false;
> +
> + /*
> + * The PCI device could be considered to be fully isolated if all
> + * devices on the path from the device to the host-PCI bridge are
> + * protected from peer-to-peer DMA by ACS.
> + */
> + if (dev_is_pci(dev))
> + return pci_acs_path_enabled(to_pci_dev(dev), NULL,
> + REQ_ACS_FLAGS);
> +
> + return true;
> +}
> +
> +int iommu_attach_device_pasid(struct iommu_domain *domain,
> + struct device *dev, ioasid_t pasid)
> +{
> + struct iommu_group *group;
> + int ret = -EINVAL;
> + void *curr;
> +
> + if (!domain->ops->attach_dev_pasid)
> + return -EOPNOTSUPP;
> +
> + if (!device_group_immutable_singleton(dev))
> + return -EINVAL;
> +
> + group = iommu_group_get(dev);
> + mutex_lock(&group->mutex);
> + curr = xa_cmpxchg(&group->pasid_array, pasid, NULL, domain, GFP_KERNEL);
> + if (curr)
> + goto out_unlock;
> + ret = domain->ops->attach_dev_pasid(domain, dev, pasid);
> + if (ret)
> + xa_erase(&group->pasid_array, pasid);
> +out_unlock:
> + mutex_unlock(&group->mutex);
> + iommu_group_put(group);
> +
> + return ret;
> +}
> +
> +void iommu_detach_device_pasid(struct iommu_domain *domain,
> + struct device *dev, ioasid_t pasid)
> +{
> + struct iommu_group *group = iommu_group_get(dev);
> +
> + mutex_lock(&group->mutex);
> + domain->ops->detach_dev_pasid(domain, dev, pasid);
> + xa_erase(&group->pasid_array, pasid);
> + mutex_unlock(&group->mutex);
> +
> + iommu_group_put(group);
> +}
> --
> 2.25.1
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v5 04/12] iommu/sva: Basic data structures for SVA
2022-05-02 1:48 ` [PATCH v5 04/12] iommu/sva: Basic data structures for SVA Lu Baolu
@ 2022-05-03 18:09 ` Jean-Philippe Brucker
2022-05-05 6:42 ` Baolu Lu
0 siblings, 1 reply; 30+ messages in thread
From: Jean-Philippe Brucker @ 2022-05-03 18:09 UTC (permalink / raw)
To: Lu Baolu
Cc: Joerg Roedel, Jason Gunthorpe, Christoph Hellwig, Kevin Tian,
Ashok Raj, Will Deacon, Robin Murphy, Jean-Philippe Brucker,
Dave Jiang, Vinod Koul, Eric Auger, Liu Yi L, Jacob jun Pan,
iommu, linux-kernel
On Mon, May 02, 2022 at 09:48:34AM +0800, Lu Baolu wrote:
> Use below data structures for SVA implementation in the IOMMU core:
>
> - struct iommu_sva_ioas
> Represent the I/O address space shared with an application CPU address
> space. This structure has a 1:1 relationship with an mm_struct. It
> grabs a "mm->mm_count" refcount during creation and drop it on release.
Do we actually need this structure? At the moment it only keeps track of
bonds, which we can move to struct dev_iommu. Replacing it by a mm pointer
in struct iommu_domain simplifies the driver and seems to work
Thanks,
Jean
>
> - struct iommu_domain (IOMMU_DOMAIN_SVA type)
> Represent a hardware pagetable that the IOMMU hardware could use for
> SVA translation. Multiple iommu domains could be bound with an SVA ioas
> and each grabs a refcount from ioas in order to make sure ioas could
> only be freed after all domains have been unbound.
>
> - struct iommu_sva
> Represent a bond relationship between an SVA ioas and an iommu domain.
> If a bond already exists, it's reused and a reference is taken.
>
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> ---
> include/linux/iommu.h | 14 +++++++++++++-
> drivers/iommu/iommu-sva-lib.h | 1 +
> drivers/iommu/iommu-sva-lib.c | 18 ++++++++++++++++++
> 3 files changed, 32 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index ab36244d4e94..f582f434c513 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -42,6 +42,7 @@ struct notifier_block;
> struct iommu_sva;
> struct iommu_fault_event;
> struct iommu_dma_cookie;
> +struct iommu_sva_ioas;
>
> /* iommu fault flags */
> #define IOMMU_FAULT_READ 0x0
> @@ -64,6 +65,9 @@ struct iommu_domain_geometry {
> #define __IOMMU_DOMAIN_PT (1U << 2) /* Domain is identity mapped */
> #define __IOMMU_DOMAIN_DMA_FQ (1U << 3) /* DMA-API uses flush queue */
>
> +#define __IOMMU_DOMAIN_SHARED (1U << 4) /* Page table shared from CPU */
> +#define __IOMMU_DOMAIN_HOST_VA (1U << 5) /* Host CPU virtual address */
> +
> /*
> * This are the possible domain-types
> *
> @@ -86,6 +90,8 @@ struct iommu_domain_geometry {
> #define IOMMU_DOMAIN_DMA_FQ (__IOMMU_DOMAIN_PAGING | \
> __IOMMU_DOMAIN_DMA_API | \
> __IOMMU_DOMAIN_DMA_FQ)
> +#define IOMMU_DOMAIN_SVA (__IOMMU_DOMAIN_SHARED | \
> + __IOMMU_DOMAIN_HOST_VA)
>
> struct iommu_domain {
> unsigned type;
> @@ -95,6 +101,7 @@ struct iommu_domain {
> void *handler_token;
> struct iommu_domain_geometry geometry;
> struct iommu_dma_cookie *iova_cookie;
> + struct iommu_sva_ioas *sva_ioas;
> };
>
> static inline bool iommu_is_dma_domain(struct iommu_domain *domain)
> @@ -628,7 +635,12 @@ struct iommu_fwspec {
> * struct iommu_sva - handle to a device-mm bond
> */
> struct iommu_sva {
> - struct device *dev;
> + struct device *dev;
> + struct iommu_sva_ioas *sva_ioas;
> + struct iommu_domain *domain;
> + /* Link to sva ioas's bonds list */
> + struct list_head node;
> + refcount_t users;
> };
>
> int iommu_fwspec_init(struct device *dev, struct fwnode_handle *iommu_fwnode,
> diff --git a/drivers/iommu/iommu-sva-lib.h b/drivers/iommu/iommu-sva-lib.h
> index 8909ea1094e3..9c5e108e2c8a 100644
> --- a/drivers/iommu/iommu-sva-lib.h
> +++ b/drivers/iommu/iommu-sva-lib.h
> @@ -10,6 +10,7 @@
>
> int iommu_sva_alloc_pasid(struct mm_struct *mm, ioasid_t min, ioasid_t max);
> struct mm_struct *iommu_sva_find(ioasid_t pasid);
> +struct mm_struct *iommu_sva_domain_mm(struct iommu_domain *domain);
>
> /* I/O Page fault */
> struct device;
> diff --git a/drivers/iommu/iommu-sva-lib.c b/drivers/iommu/iommu-sva-lib.c
> index 106506143896..d524a402be3b 100644
> --- a/drivers/iommu/iommu-sva-lib.c
> +++ b/drivers/iommu/iommu-sva-lib.c
> @@ -3,6 +3,8 @@
> * Helpers for IOMMU drivers implementing SVA
> */
> #include <linux/mutex.h>
> +#include <linux/iommu.h>
> +#include <linux/slab.h>
> #include <linux/sched/mm.h>
>
> #include "iommu-sva-lib.h"
> @@ -10,6 +12,22 @@
> static DEFINE_MUTEX(iommu_sva_lock);
> static DECLARE_IOASID_SET(iommu_sva_pasid);
>
> +struct iommu_sva_ioas {
> + struct mm_struct *mm;
> + ioasid_t pasid;
> +
> + /* Counter of domains attached to this ioas. */
> + refcount_t users;
> +
> + /* All bindings are linked here. */
> + struct list_head bonds;
> +};
> +
> +struct mm_struct *iommu_sva_domain_mm(struct iommu_domain *domain)
> +{
> + return domain->sva_ioas->mm;
> +}
> +
> /**
> * iommu_sva_alloc_pasid - Allocate a PASID for the mm
> * @mm: the mm
> --
> 2.25.1
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v5 07/12] arm-smmu-v3/sva: Add SVA domain support
2022-05-02 1:48 ` [PATCH v5 07/12] arm-smmu-v3/sva: " Lu Baolu
@ 2022-05-03 18:12 ` Jean-Philippe Brucker
2022-05-05 7:09 ` Baolu Lu
0 siblings, 1 reply; 30+ messages in thread
From: Jean-Philippe Brucker @ 2022-05-03 18:12 UTC (permalink / raw)
To: Lu Baolu
Cc: Joerg Roedel, Jason Gunthorpe, Christoph Hellwig, Kevin Tian,
Ashok Raj, Will Deacon, Robin Murphy, Jean-Philippe Brucker,
Dave Jiang, Vinod Koul, Eric Auger, Liu Yi L, Jacob jun Pan,
iommu, linux-kernel
On Mon, May 02, 2022 at 09:48:37AM +0800, Lu Baolu wrote:
> Add support for SVA domain allocation and provide an SVA-specific
> iommu_domain_ops.
>
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> ---
> drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 14 +++++++
> .../iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c | 42 +++++++++++++++++++
> drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 21 ++++++++++
> 3 files changed, 77 insertions(+)
>
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> index cd48590ada30..7631c00fdcbd 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> @@ -759,6 +759,10 @@ struct iommu_sva *arm_smmu_sva_bind(struct device *dev, struct mm_struct *mm,
> void arm_smmu_sva_unbind(struct iommu_sva *handle);
> u32 arm_smmu_sva_get_pasid(struct iommu_sva *handle);
> void arm_smmu_sva_notifier_synchronize(void);
> +int arm_smmu_sva_attach_dev_pasid(struct iommu_domain *domain,
> + struct device *dev, ioasid_t id);
> +void arm_smmu_sva_detach_dev_pasid(struct iommu_domain *domain,
> + struct device *dev, ioasid_t id);
> #else /* CONFIG_ARM_SMMU_V3_SVA */
> static inline bool arm_smmu_sva_supported(struct arm_smmu_device *smmu)
> {
> @@ -804,5 +808,15 @@ static inline u32 arm_smmu_sva_get_pasid(struct iommu_sva *handle)
> }
>
> static inline void arm_smmu_sva_notifier_synchronize(void) {}
> +
> +static inline int arm_smmu_sva_attach_dev_pasid(struct iommu_domain *domain,
> + struct device *dev, ioasid_t id)
> +{
> + return -ENODEV;
> +}
> +
> +static inline void arm_smmu_sva_detach_dev_pasid(struct iommu_domain *domain,
> + struct device *dev,
> + ioasid_t id) {}
> #endif /* CONFIG_ARM_SMMU_V3_SVA */
> #endif /* _ARM_SMMU_V3_H */
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
> index c623dae1e115..3b843cd3ed67 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
> @@ -541,3 +541,45 @@ void arm_smmu_sva_notifier_synchronize(void)
> */
> mmu_notifier_synchronize();
> }
> +
> +int arm_smmu_sva_attach_dev_pasid(struct iommu_domain *domain,
> + struct device *dev, ioasid_t id)
> +{
> + int ret = 0;
> + struct iommu_sva *handle;
> + struct mm_struct *mm = iommu_sva_domain_mm(domain);
> +
> + if (domain->type != IOMMU_DOMAIN_SVA || !mm)
We wouldn't get that far with a non-SVA domain since iommu_sva_domain_mm()
would dereference a NULL pointer. Could you move it after the domain->type
check, and maybe add a WARN_ON()? It could help catch issues in future
API changes.
> + return -EINVAL;
> +
> + mutex_lock(&sva_lock);
> + handle = __arm_smmu_sva_bind(dev, mm);
> + if (IS_ERR(handle))
> + ret = PTR_ERR(handle);
> + mutex_unlock(&sva_lock);
> +
> + return ret;
> +}
> +
> +void arm_smmu_sva_detach_dev_pasid(struct iommu_domain *domain,
> + struct device *dev, ioasid_t id)
> +{
> + struct arm_smmu_bond *bond = NULL, *t;
> + struct mm_struct *mm = iommu_sva_domain_mm(domain);
> + struct arm_smmu_master *master = dev_iommu_priv_get(dev);
> +
> + mutex_lock(&sva_lock);
> + list_for_each_entry(t, &master->bonds, list) {
> + if (t->mm == mm) {
> + bond = t;
> + break;
> + }
> + }
> +
> + if (!WARN_ON(!bond) && refcount_dec_and_test(&bond->refs)) {
> + list_del(&bond->list);
> + arm_smmu_mmu_notifier_put(bond->smmu_mn);
> + kfree(bond);
> + }
> + mutex_unlock(&sva_lock);
> +}
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> index afc63fce6107..bd80de0bad98 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -1995,10 +1995,31 @@ static bool arm_smmu_capable(enum iommu_cap cap)
> }
> }
>
> +static void arm_smmu_sva_domain_free(struct iommu_domain *domain)
> +{
> + kfree(domain);
> +}
> +
> +static const struct iommu_domain_ops arm_smmu_sva_domain_ops = {
> + .attach_dev_pasid = arm_smmu_sva_attach_dev_pasid,
> + .detach_dev_pasid = arm_smmu_sva_detach_dev_pasid,
> + .free = arm_smmu_sva_domain_free,
> +};
> +
> static struct iommu_domain *arm_smmu_domain_alloc(unsigned type)
> {
> struct arm_smmu_domain *smmu_domain;
>
> + if (type == IOMMU_DOMAIN_SVA) {
> + struct iommu_domain *domain;
> +
> + domain = kzalloc(sizeof(*domain), GFP_KERNEL);
> + if (domain)
> + domain->ops = &arm_smmu_sva_domain_ops;
> +
> + return domain;
> + }
> +
I'd prefer moving all of this to arm-smmu-v3-sva.c and just call
arm_smmu_sva_domain_alloc() here
Otherwise the patch looks fine. I'll rework the driver when I find some
time, because we can now remove arm_smmu_bond and move smmu_mn to the SVA
domain, maybe also remove sva_lock but I haven't thought it through.
Thanks,
Jean
> if (type != IOMMU_DOMAIN_UNMANAGED &&
> type != IOMMU_DOMAIN_DMA &&
> type != IOMMU_DOMAIN_DMA_FQ &&
> --
> 2.25.1
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v5 09/12] iommu: Remove SVA related callbacks from iommu ops
2022-05-02 1:48 ` [PATCH v5 09/12] iommu: Remove SVA related callbacks from iommu ops Lu Baolu
@ 2022-05-03 18:14 ` Jean-Philippe Brucker
0 siblings, 0 replies; 30+ messages in thread
From: Jean-Philippe Brucker @ 2022-05-03 18:14 UTC (permalink / raw)
To: Lu Baolu
Cc: Joerg Roedel, Jason Gunthorpe, Christoph Hellwig, Kevin Tian,
Ashok Raj, Will Deacon, Robin Murphy, Jean-Philippe Brucker,
Dave Jiang, Vinod Koul, Eric Auger, Liu Yi L, Jacob jun Pan,
iommu, linux-kernel
On Mon, May 02, 2022 at 09:48:39AM +0800, Lu Baolu wrote:
> These ops'es have been replaced with the dev_attach/detach_pasid domain
> ops'es. There's no need for them anymore. Remove them to avoid dead
> code.
>
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
Reviewed-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> ---
> include/linux/intel-iommu.h | 4 --
> include/linux/iommu.h | 8 ---
> drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 17 -------
> drivers/iommu/iommu-sva-lib.h | 1 -
> .../iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c | 41 ----------------
> drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 3 --
> drivers/iommu/intel/iommu.c | 3 --
> drivers/iommu/intel/svm.c | 49 -------------------
> drivers/iommu/iommu-sva-lib.c | 4 +-
> 9 files changed, 2 insertions(+), 128 deletions(-)
>
> diff --git a/include/linux/intel-iommu.h b/include/linux/intel-iommu.h
> index 3b4ca16f53e2..5af24befc9f1 100644
> --- a/include/linux/intel-iommu.h
> +++ b/include/linux/intel-iommu.h
> @@ -738,10 +738,6 @@ struct intel_iommu *device_to_iommu(struct device *dev, u8 *bus, u8 *devfn);
> extern void intel_svm_check(struct intel_iommu *iommu);
> extern int intel_svm_enable_prq(struct intel_iommu *iommu);
> extern int intel_svm_finish_prq(struct intel_iommu *iommu);
> -struct iommu_sva *intel_svm_bind(struct device *dev, struct mm_struct *mm,
> - void *drvdata);
> -void intel_svm_unbind(struct iommu_sva *handle);
> -u32 intel_svm_get_pasid(struct iommu_sva *handle);
> int intel_svm_page_response(struct device *dev, struct iommu_fault_event *evt,
> struct iommu_page_response *msg);
> extern const struct iommu_domain_ops intel_svm_domain_ops;
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index c5a16b47cae8..19718939d9df 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -214,9 +214,6 @@ struct iommu_iotlb_gather {
> * @dev_has/enable/disable_feat: per device entries to check/enable/disable
> * iommu specific features.
> * @dev_feat_enabled: check enabled feature
> - * @sva_bind: Bind process address space to device
> - * @sva_unbind: Unbind process address space from device
> - * @sva_get_pasid: Get PASID associated to a SVA handle
> * @page_response: handle page request response
> * @def_domain_type: device default domain type, return value:
> * - IOMMU_DOMAIN_IDENTITY: must use an identity domain
> @@ -250,11 +247,6 @@ struct iommu_ops {
> int (*dev_enable_feat)(struct device *dev, enum iommu_dev_features f);
> int (*dev_disable_feat)(struct device *dev, enum iommu_dev_features f);
>
> - struct iommu_sva *(*sva_bind)(struct device *dev, struct mm_struct *mm,
> - void *drvdata);
> - void (*sva_unbind)(struct iommu_sva *handle);
> - u32 (*sva_get_pasid)(struct iommu_sva *handle);
> -
> int (*page_response)(struct device *dev,
> struct iommu_fault_event *evt,
> struct iommu_page_response *msg);
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> index 7631c00fdcbd..2513309ec0db 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> @@ -754,10 +754,6 @@ bool arm_smmu_master_sva_enabled(struct arm_smmu_master *master);
> int arm_smmu_master_enable_sva(struct arm_smmu_master *master);
> int arm_smmu_master_disable_sva(struct arm_smmu_master *master);
> bool arm_smmu_master_iopf_supported(struct arm_smmu_master *master);
> -struct iommu_sva *arm_smmu_sva_bind(struct device *dev, struct mm_struct *mm,
> - void *drvdata);
> -void arm_smmu_sva_unbind(struct iommu_sva *handle);
> -u32 arm_smmu_sva_get_pasid(struct iommu_sva *handle);
> void arm_smmu_sva_notifier_synchronize(void);
> int arm_smmu_sva_attach_dev_pasid(struct iommu_domain *domain,
> struct device *dev, ioasid_t id);
> @@ -794,19 +790,6 @@ static inline bool arm_smmu_master_iopf_supported(struct arm_smmu_master *master
> return false;
> }
>
> -static inline struct iommu_sva *
> -arm_smmu_sva_bind(struct device *dev, struct mm_struct *mm, void *drvdata)
> -{
> - return ERR_PTR(-ENODEV);
> -}
> -
> -static inline void arm_smmu_sva_unbind(struct iommu_sva *handle) {}
> -
> -static inline u32 arm_smmu_sva_get_pasid(struct iommu_sva *handle)
> -{
> - return IOMMU_PASID_INVALID;
> -}
> -
> static inline void arm_smmu_sva_notifier_synchronize(void) {}
>
> static inline int arm_smmu_sva_attach_dev_pasid(struct iommu_domain *domain,
> diff --git a/drivers/iommu/iommu-sva-lib.h b/drivers/iommu/iommu-sva-lib.h
> index 9c5e108e2c8a..5776b4c80cc1 100644
> --- a/drivers/iommu/iommu-sva-lib.h
> +++ b/drivers/iommu/iommu-sva-lib.h
> @@ -8,7 +8,6 @@
> #include <linux/ioasid.h>
> #include <linux/mm_types.h>
>
> -int iommu_sva_alloc_pasid(struct mm_struct *mm, ioasid_t min, ioasid_t max);
> struct mm_struct *iommu_sva_find(ioasid_t pasid);
> struct mm_struct *iommu_sva_domain_mm(struct iommu_domain *domain);
>
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
> index 3b843cd3ed67..0ace04b27d4b 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
> @@ -335,11 +335,6 @@ __arm_smmu_sva_bind(struct device *dev, struct mm_struct *mm)
> if (!bond)
> return ERR_PTR(-ENOMEM);
>
> - /* Allocate a PASID for this mm if necessary */
> - ret = iommu_sva_alloc_pasid(mm, 1, (1U << master->ssid_bits) - 1);
> - if (ret)
> - goto err_free_bond;
> -
> bond->mm = mm;
> bond->sva.dev = dev;
> refcount_set(&bond->refs, 1);
> @@ -358,42 +353,6 @@ __arm_smmu_sva_bind(struct device *dev, struct mm_struct *mm)
> return ERR_PTR(ret);
> }
>
> -struct iommu_sva *
> -arm_smmu_sva_bind(struct device *dev, struct mm_struct *mm, void *drvdata)
> -{
> - struct iommu_sva *handle;
> - struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
> - struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
> -
> - if (smmu_domain->stage != ARM_SMMU_DOMAIN_S1)
> - return ERR_PTR(-EINVAL);
> -
> - mutex_lock(&sva_lock);
> - handle = __arm_smmu_sva_bind(dev, mm);
> - mutex_unlock(&sva_lock);
> - return handle;
> -}
> -
> -void arm_smmu_sva_unbind(struct iommu_sva *handle)
> -{
> - struct arm_smmu_bond *bond = sva_to_bond(handle);
> -
> - mutex_lock(&sva_lock);
> - if (refcount_dec_and_test(&bond->refs)) {
> - list_del(&bond->list);
> - arm_smmu_mmu_notifier_put(bond->smmu_mn);
> - kfree(bond);
> - }
> - mutex_unlock(&sva_lock);
> -}
> -
> -u32 arm_smmu_sva_get_pasid(struct iommu_sva *handle)
> -{
> - struct arm_smmu_bond *bond = sva_to_bond(handle);
> -
> - return bond->mm->pasid;
> -}
> -
> bool arm_smmu_sva_supported(struct arm_smmu_device *smmu)
> {
> unsigned long reg, fld;
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> index bd80de0bad98..543d3ef1c102 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -2875,9 +2875,6 @@ static struct iommu_ops arm_smmu_ops = {
> .dev_feat_enabled = arm_smmu_dev_feature_enabled,
> .dev_enable_feat = arm_smmu_dev_enable_feature,
> .dev_disable_feat = arm_smmu_dev_disable_feature,
> - .sva_bind = arm_smmu_sva_bind,
> - .sva_unbind = arm_smmu_sva_unbind,
> - .sva_get_pasid = arm_smmu_sva_get_pasid,
> .page_response = arm_smmu_page_response,
> .pgsize_bitmap = -1UL, /* Restricted during device attach */
> .owner = THIS_MODULE,
> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> index 5d1d3e325afa..46e2eb15197b 100644
> --- a/drivers/iommu/intel/iommu.c
> +++ b/drivers/iommu/intel/iommu.c
> @@ -4914,9 +4914,6 @@ const struct iommu_ops intel_iommu_ops = {
> .def_domain_type = device_def_domain_type,
> .pgsize_bitmap = SZ_4K,
> #ifdef CONFIG_INTEL_IOMMU_SVM
> - .sva_bind = intel_svm_bind,
> - .sva_unbind = intel_svm_unbind,
> - .sva_get_pasid = intel_svm_get_pasid,
> .page_response = intel_svm_page_response,
> #endif
> .default_domain_ops = &(const struct iommu_domain_ops) {
> diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c
> index ec684d883014..6084f960ba27 100644
> --- a/drivers/iommu/intel/svm.c
> +++ b/drivers/iommu/intel/svm.c
> @@ -313,14 +313,6 @@ static int pasid_to_svm_sdev(struct device *dev, unsigned int pasid,
> return 0;
> }
>
> -static int intel_svm_alloc_pasid(struct device *dev, struct mm_struct *mm)
> -{
> - ioasid_t max_pasid = dev_is_pci(dev) ?
> - pci_max_pasids(to_pci_dev(dev)) : intel_pasid_max_id;
> -
> - return iommu_sva_alloc_pasid(mm, PASID_MIN, max_pasid - 1);
> -}
> -
> static struct iommu_sva *intel_svm_bind_mm(struct intel_iommu *iommu,
> struct device *dev,
> struct mm_struct *mm)
> @@ -809,47 +801,6 @@ static irqreturn_t prq_event_thread(int irq, void *d)
> return IRQ_RETVAL(handled);
> }
>
> -struct iommu_sva *intel_svm_bind(struct device *dev, struct mm_struct *mm, void *drvdata)
> -{
> - struct intel_iommu *iommu = device_to_iommu(dev, NULL, NULL);
> - struct iommu_sva *sva;
> - int ret;
> -
> - mutex_lock(&pasid_mutex);
> - ret = intel_svm_alloc_pasid(dev, mm);
> - if (ret) {
> - mutex_unlock(&pasid_mutex);
> - return ERR_PTR(ret);
> - }
> -
> - sva = intel_svm_bind_mm(iommu, dev, mm);
> - mutex_unlock(&pasid_mutex);
> -
> - return sva;
> -}
> -
> -void intel_svm_unbind(struct iommu_sva *sva)
> -{
> - struct intel_svm_dev *sdev = to_intel_svm_dev(sva);
> -
> - mutex_lock(&pasid_mutex);
> - intel_svm_unbind_mm(sdev->dev, sdev->pasid);
> - mutex_unlock(&pasid_mutex);
> -}
> -
> -u32 intel_svm_get_pasid(struct iommu_sva *sva)
> -{
> - struct intel_svm_dev *sdev;
> - u32 pasid;
> -
> - mutex_lock(&pasid_mutex);
> - sdev = to_intel_svm_dev(sva);
> - pasid = sdev->pasid;
> - mutex_unlock(&pasid_mutex);
> -
> - return pasid;
> -}
> -
> int intel_svm_page_response(struct device *dev,
> struct iommu_fault_event *evt,
> struct iommu_page_response *msg)
> diff --git a/drivers/iommu/iommu-sva-lib.c b/drivers/iommu/iommu-sva-lib.c
> index 0524659a4c0c..992388106da0 100644
> --- a/drivers/iommu/iommu-sva-lib.c
> +++ b/drivers/iommu/iommu-sva-lib.c
> @@ -41,7 +41,8 @@ struct mm_struct *iommu_sva_domain_mm(struct iommu_domain *domain)
> *
> * Returns 0 on success and < 0 on error.
> */
> -int iommu_sva_alloc_pasid(struct mm_struct *mm, ioasid_t min, ioasid_t max)
> +static int iommu_sva_alloc_pasid(struct mm_struct *mm,
> + ioasid_t min, ioasid_t max)
> {
> int ret = 0;
> ioasid_t pasid;
> @@ -67,7 +68,6 @@ int iommu_sva_alloc_pasid(struct mm_struct *mm, ioasid_t min, ioasid_t max)
> mutex_unlock(&iommu_sva_lock);
> return ret;
> }
> -EXPORT_SYMBOL_GPL(iommu_sva_alloc_pasid);
>
> /* ioasid_find getter() requires a void * argument */
> static bool __mmget_not_zero(void *mm)
> --
> 2.25.1
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v5 10/12] iommu: Prepare IOMMU domain for IOPF
2022-05-02 1:48 ` [PATCH v5 10/12] iommu: Prepare IOMMU domain for IOPF Lu Baolu
@ 2022-05-03 18:20 ` Jean-Philippe Brucker
2022-05-05 8:31 ` Baolu Lu
0 siblings, 1 reply; 30+ messages in thread
From: Jean-Philippe Brucker @ 2022-05-03 18:20 UTC (permalink / raw)
To: Lu Baolu
Cc: Joerg Roedel, Jason Gunthorpe, Christoph Hellwig, Kevin Tian,
Ashok Raj, Will Deacon, Robin Murphy, Jean-Philippe Brucker,
Dave Jiang, Vinod Koul, Eric Auger, Liu Yi L, Jacob jun Pan,
iommu, linux-kernel
On Mon, May 02, 2022 at 09:48:40AM +0800, Lu Baolu wrote:
> This adds some mechanisms around the iommu_domain so that the I/O page
> fault handling framework could route a page fault to the domain and
> call the fault handler from it.
>
> Add pointers to the page fault handler and its private data in struct
> iommu_domain. The fault handler will be called with the private data
> as a parameter once a page fault is routed to the domain. Any kernel
> component which owns an iommu domain could install handler and its
> private parameter so that the page fault could be further routed and
> handled.
>
> A new helper iommu_get_domain_for_dev_pasid() which retrieves attached
> domain for a {device, PASID} is added. It will be used by the page fault
> handling framework which knows {device, PASID} reported from the iommu
> driver. We have a guarantee that the SVA domain doesn't go away during
> IOPF handling, because unbind() waits for pending faults with
> iopf_queue_flush_dev() before freeing the domain. Hence, there's no need
> to synchronize life cycle of the iommu domains between the unbind() and
> the interrupt threads.
>
> This also prepares the SVA implementation to be the first consumer of
> the per-domain page fault handling model.
>
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> ---
> include/linux/iommu.h | 12 +++++++
> drivers/iommu/iommu-sva-lib.c | 65 +++++++++++++++++++++++++++++++++++
> drivers/iommu/iommu.c | 21 +++++++++++
> 3 files changed, 98 insertions(+)
>
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index 19718939d9df..1164524814cb 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -102,6 +102,9 @@ struct iommu_domain {
> struct iommu_domain_geometry geometry;
> struct iommu_dma_cookie *iova_cookie;
> struct iommu_sva_ioas *sva_ioas;
> + enum iommu_page_response_code (*iopf_handler)(struct iommu_fault *fault,
> + void *data);
> + void *fault_data;
> };
>
> static inline bool iommu_is_dma_domain(struct iommu_domain *domain)
> @@ -686,6 +689,9 @@ int iommu_attach_device_pasid(struct iommu_domain *domain,
> struct device *dev, ioasid_t pasid);
> void iommu_detach_device_pasid(struct iommu_domain *domain,
> struct device *dev, ioasid_t pasid);
> +struct iommu_domain *
> +iommu_get_domain_for_dev_pasid(struct device *dev, ioasid_t pasid);
> +
> #else /* CONFIG_IOMMU_API */
>
> struct iommu_ops {};
> @@ -1055,6 +1061,12 @@ static inline void iommu_detach_device_pasid(struct iommu_domain *domain,
> struct device *dev, ioasid_t pasid)
> {
> }
> +
> +static inline struct iommu_domain *
> +iommu_get_domain_for_dev_pasid(struct device *dev, ioasid_t pasid)
> +{
> + return NULL;
> +}
> #endif /* CONFIG_IOMMU_API */
>
> #ifdef CONFIG_IOMMU_SVA
> diff --git a/drivers/iommu/iommu-sva-lib.c b/drivers/iommu/iommu-sva-lib.c
> index 992388106da0..05a7d2f0e46f 100644
> --- a/drivers/iommu/iommu-sva-lib.c
> +++ b/drivers/iommu/iommu-sva-lib.c
> @@ -135,6 +135,69 @@ static void iommu_sva_ioas_put(struct iommu_sva_ioas *ioas)
> }
> }
>
> +/*
> + * I/O page fault handler for SVA
> + *
> + * Copied from io-pgfault.c with mmget_not_zero() added before
> + * mmap_read_lock().
> + */
> +static enum iommu_page_response_code
> +iommu_sva_handle_iopf(struct iommu_fault *fault, void *data)
> +{
> + vm_fault_t ret;
> + struct mm_struct *mm;
> + struct vm_area_struct *vma;
> + unsigned int access_flags = 0;
> + struct iommu_domain *domain = data;
> + unsigned int fault_flags = FAULT_FLAG_REMOTE;
> + struct iommu_fault_page_request *prm = &fault->prm;
> + enum iommu_page_response_code status = IOMMU_PAGE_RESP_INVALID;
> +
> + if (!(prm->flags & IOMMU_FAULT_PAGE_REQUEST_PASID_VALID))
> + return status;
> +
> + mm = iommu_sva_domain_mm(domain);
> + if (IS_ERR_OR_NULL(mm) || !mmget_not_zero(mm))
> + return status;
> +
> + mmap_read_lock(mm);
> +
> + vma = find_extend_vma(mm, prm->addr);
> + if (!vma)
> + /* Unmapped area */
> + goto out_put_mm;
> +
> + if (prm->perm & IOMMU_FAULT_PERM_READ)
> + access_flags |= VM_READ;
> +
> + if (prm->perm & IOMMU_FAULT_PERM_WRITE) {
> + access_flags |= VM_WRITE;
> + fault_flags |= FAULT_FLAG_WRITE;
> + }
> +
> + if (prm->perm & IOMMU_FAULT_PERM_EXEC) {
> + access_flags |= VM_EXEC;
> + fault_flags |= FAULT_FLAG_INSTRUCTION;
> + }
> +
> + if (!(prm->perm & IOMMU_FAULT_PERM_PRIV))
> + fault_flags |= FAULT_FLAG_USER;
> +
> + if (access_flags & ~vma->vm_flags)
> + /* Access fault */
> + goto out_put_mm;
> +
> + ret = handle_mm_fault(vma, prm->addr, fault_flags, NULL);
> + status = ret & VM_FAULT_ERROR ? IOMMU_PAGE_RESP_INVALID :
> + IOMMU_PAGE_RESP_SUCCESS;
> +
> +out_put_mm:
> + mmap_read_unlock(mm);
> + mmput(mm);
> +
> + return status;
> +}
> +
> /*
> * IOMMU SVA driver-oriented interfaces
> */
> @@ -154,6 +217,8 @@ iommu_sva_alloc_domain(struct device *dev, struct iommu_sva_ioas *ioas)
> /* The caller must hold a reference to ioas. */
> domain->sva_ioas = ioas;
> domain->type = IOMMU_DOMAIN_SVA;
> + domain->iopf_handler = iommu_sva_handle_iopf;
> + domain->fault_data = domain;
>
> return domain;
> }
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 7cae631c1baa..33449523afbe 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -3174,3 +3174,24 @@ void iommu_detach_device_pasid(struct iommu_domain *domain,
>
> iommu_group_put(group);
> }
> +
> +struct iommu_domain *iommu_get_domain_for_dev_pasid(struct device *dev,
> + ioasid_t pasid)
> +{
> + struct iommu_domain *domain;
> + struct iommu_group *group;
> +
> + if (!pasid_valid(pasid))
> + return NULL;
> +
> + group = iommu_group_get(dev);
> + if (!group)
> + return NULL;
> +
> + mutex_lock(&group->mutex);
Unfortunately this still causes the deadlock when unbind() flushes the
IOPF queue while holding the group mutex.
If we make this function private to IOPF, then we can get rid of this
mutex_lock(). It's OK because:
* xarray protects its internal state with RCU, so we can call
xa_load() outside the lock.
* The domain obtained from xa_load is finalized. Its content is valid
because xarray stores the domain using rcu_assign_pointer(), which has a
release memory barrier, which pairs with data dependencies in IOPF
(domain->sva_ioas etc).
We'll need to be careful about this when allowing other users to install
a fault handler. Should be fine as long as the handler and data are
installed before the domain is added to pasid_array.
* We know the domain is valid the whole time IOPF is using it, because
unbind() waits for pending faults.
We just need a comment explaining the last point, something like:
/*
* Safe to fetch outside the group mutex because:
* - xarray protects its internal state with RCU
* - the domain obtained is either NULL or fully formed
* - the IOPF work is the only caller and is flushed before the
* domain is freed.
*/
Thanks,
Jean
> + domain = xa_load(&group->pasid_array, pasid);
> + mutex_unlock(&group->mutex);
> + iommu_group_put(group);
> +
> + return domain;
> +}
> --
> 2.25.1
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v5 11/12] iommu: Per-domain I/O page fault handling
2022-05-02 1:48 ` [PATCH v5 11/12] iommu: Per-domain I/O page fault handling Lu Baolu
@ 2022-05-03 18:27 ` Jean-Philippe Brucker
0 siblings, 0 replies; 30+ messages in thread
From: Jean-Philippe Brucker @ 2022-05-03 18:27 UTC (permalink / raw)
To: Lu Baolu
Cc: Joerg Roedel, Jason Gunthorpe, Christoph Hellwig, Kevin Tian,
Ashok Raj, Will Deacon, Robin Murphy, Jean-Philippe Brucker,
Dave Jiang, Vinod Koul, Eric Auger, Liu Yi L, Jacob jun Pan,
iommu, linux-kernel
On Mon, May 02, 2022 at 09:48:41AM +0800, Lu Baolu wrote:
> Tweak the I/O page fault handling framework to route the page faults to
> the domain and call the page fault handler retrieved from the domain.
> This makes the I/O page fault handling framework possible to serve more
> usage scenarios as long as they have an IOMMU domain and install a page
> fault handler in it. Some unused functions are also removed to avoid
> dead code.
>
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
Reviewed-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> ---
> drivers/iommu/iommu-sva-lib.h | 1 -
> drivers/iommu/io-pgfault.c | 64 ++++-------------------------------
> drivers/iommu/iommu-sva-lib.c | 20 -----------
> 3 files changed, 7 insertions(+), 78 deletions(-)
>
> diff --git a/drivers/iommu/iommu-sva-lib.h b/drivers/iommu/iommu-sva-lib.h
> index 5776b4c80cc1..e7813c6706fb 100644
> --- a/drivers/iommu/iommu-sva-lib.h
> +++ b/drivers/iommu/iommu-sva-lib.h
> @@ -8,7 +8,6 @@
> #include <linux/ioasid.h>
> #include <linux/mm_types.h>
>
> -struct mm_struct *iommu_sva_find(ioasid_t pasid);
> struct mm_struct *iommu_sva_domain_mm(struct iommu_domain *domain);
>
> /* I/O Page fault */
> diff --git a/drivers/iommu/io-pgfault.c b/drivers/iommu/io-pgfault.c
> index 1df8c1dcae77..8a2bb56e1474 100644
> --- a/drivers/iommu/io-pgfault.c
> +++ b/drivers/iommu/io-pgfault.c
> @@ -69,69 +69,18 @@ static int iopf_complete_group(struct device *dev, struct iopf_fault *iopf,
> return iommu_page_response(dev, &resp);
> }
>
> -static enum iommu_page_response_code
> -iopf_handle_single(struct iopf_fault *iopf)
> -{
> - vm_fault_t ret;
> - struct mm_struct *mm;
> - struct vm_area_struct *vma;
> - unsigned int access_flags = 0;
> - unsigned int fault_flags = FAULT_FLAG_REMOTE;
> - struct iommu_fault_page_request *prm = &iopf->fault.prm;
> - enum iommu_page_response_code status = IOMMU_PAGE_RESP_INVALID;
> -
> - if (!(prm->flags & IOMMU_FAULT_PAGE_REQUEST_PASID_VALID))
> - return status;
> -
> - mm = iommu_sva_find(prm->pasid);
> - if (IS_ERR_OR_NULL(mm))
> - return status;
> -
> - mmap_read_lock(mm);
> -
> - vma = find_extend_vma(mm, prm->addr);
> - if (!vma)
> - /* Unmapped area */
> - goto out_put_mm;
> -
> - if (prm->perm & IOMMU_FAULT_PERM_READ)
> - access_flags |= VM_READ;
> -
> - if (prm->perm & IOMMU_FAULT_PERM_WRITE) {
> - access_flags |= VM_WRITE;
> - fault_flags |= FAULT_FLAG_WRITE;
> - }
> -
> - if (prm->perm & IOMMU_FAULT_PERM_EXEC) {
> - access_flags |= VM_EXEC;
> - fault_flags |= FAULT_FLAG_INSTRUCTION;
> - }
> -
> - if (!(prm->perm & IOMMU_FAULT_PERM_PRIV))
> - fault_flags |= FAULT_FLAG_USER;
> -
> - if (access_flags & ~vma->vm_flags)
> - /* Access fault */
> - goto out_put_mm;
> -
> - ret = handle_mm_fault(vma, prm->addr, fault_flags, NULL);
> - status = ret & VM_FAULT_ERROR ? IOMMU_PAGE_RESP_INVALID :
> - IOMMU_PAGE_RESP_SUCCESS;
> -
> -out_put_mm:
> - mmap_read_unlock(mm);
> - mmput(mm);
> -
> - return status;
> -}
> -
> static void iopf_handle_group(struct work_struct *work)
> {
> struct iopf_group *group;
> + struct iommu_domain *domain;
> struct iopf_fault *iopf, *next;
> enum iommu_page_response_code status = IOMMU_PAGE_RESP_SUCCESS;
>
> group = container_of(work, struct iopf_group, work);
> + domain = iommu_get_domain_for_dev_pasid(group->dev,
> + group->last_fault.fault.prm.pasid);
> + if (!domain || !domain->iopf_handler)
> + status = IOMMU_PAGE_RESP_INVALID;
>
> list_for_each_entry_safe(iopf, next, &group->faults, list) {
> /*
> @@ -139,7 +88,8 @@ static void iopf_handle_group(struct work_struct *work)
> * faults in the group if there is an error.
> */
> if (status == IOMMU_PAGE_RESP_SUCCESS)
> - status = iopf_handle_single(iopf);
> + status = domain->iopf_handler(&iopf->fault,
> + domain->fault_data);
>
> if (!(iopf->fault.prm.flags &
> IOMMU_FAULT_PAGE_REQUEST_LAST_PAGE))
> diff --git a/drivers/iommu/iommu-sva-lib.c b/drivers/iommu/iommu-sva-lib.c
> index 05a7d2f0e46f..ae3595d60f38 100644
> --- a/drivers/iommu/iommu-sva-lib.c
> +++ b/drivers/iommu/iommu-sva-lib.c
> @@ -69,26 +69,6 @@ static int iommu_sva_alloc_pasid(struct mm_struct *mm,
> return ret;
> }
>
> -/* ioasid_find getter() requires a void * argument */
> -static bool __mmget_not_zero(void *mm)
> -{
> - return mmget_not_zero(mm);
> -}
> -
> -/**
> - * iommu_sva_find() - Find mm associated to the given PASID
> - * @pasid: Process Address Space ID assigned to the mm
> - *
> - * On success a reference to the mm is taken, and must be released with mmput().
> - *
> - * Returns the mm corresponding to this PASID, or an error if not found.
> - */
> -struct mm_struct *iommu_sva_find(ioasid_t pasid)
> -{
> - return ioasid_find(&iommu_sva_pasid, pasid, __mmget_not_zero);
> -}
> -EXPORT_SYMBOL_GPL(iommu_sva_find);
> -
> /*
> * Get or put an ioas for a shared memory.
> */
> --
> 2.25.1
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v5 12/12] iommu: Rename iommu-sva-lib.{c,h}
2022-05-02 1:48 ` [PATCH v5 12/12] iommu: Rename iommu-sva-lib.{c,h} Lu Baolu
@ 2022-05-03 18:28 ` Jean-Philippe Brucker
0 siblings, 0 replies; 30+ messages in thread
From: Jean-Philippe Brucker @ 2022-05-03 18:28 UTC (permalink / raw)
To: Lu Baolu
Cc: Joerg Roedel, Jason Gunthorpe, Christoph Hellwig, Kevin Tian,
Ashok Raj, Will Deacon, Robin Murphy, Jean-Philippe Brucker,
Dave Jiang, Vinod Koul, Eric Auger, Liu Yi L, Jacob jun Pan,
iommu, linux-kernel
On Mon, May 02, 2022 at 09:48:42AM +0800, Lu Baolu wrote:
> Rename iommu-sva-lib.c[h] to iommu-sva.c[h] as it contains all code
> for SVA implementation in iommu core.
>
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
Reviewed-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> ---
> drivers/iommu/{iommu-sva-lib.h => iommu-sva.h} | 0
> drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c | 2 +-
> drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 2 +-
> drivers/iommu/intel/iommu.c | 2 +-
> drivers/iommu/intel/svm.c | 2 +-
> drivers/iommu/io-pgfault.c | 2 +-
> drivers/iommu/{iommu-sva-lib.c => iommu-sva.c} | 2 +-
> drivers/iommu/Makefile | 2 +-
> 8 files changed, 7 insertions(+), 7 deletions(-)
> rename drivers/iommu/{iommu-sva-lib.h => iommu-sva.h} (100%)
> rename drivers/iommu/{iommu-sva-lib.c => iommu-sva.c} (99%)
>
> diff --git a/drivers/iommu/iommu-sva-lib.h b/drivers/iommu/iommu-sva.h
> similarity index 100%
> rename from drivers/iommu/iommu-sva-lib.h
> rename to drivers/iommu/iommu-sva.h
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
> index 0ace04b27d4b..73a336e17dc8 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
> @@ -9,7 +9,7 @@
> #include <linux/slab.h>
>
> #include "arm-smmu-v3.h"
> -#include "../../iommu-sva-lib.h"
> +#include "../../iommu-sva.h"
> #include "../../io-pgtable-arm.h"
>
> struct arm_smmu_mmu_notifier {
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> index 543d3ef1c102..ca2bd17eec41 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -31,7 +31,7 @@
> #include <linux/amba/bus.h>
>
> #include "arm-smmu-v3.h"
> -#include "../../iommu-sva-lib.h"
> +#include "../../iommu-sva.h"
>
> static bool disable_bypass = true;
> module_param(disable_bypass, bool, 0444);
> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> index 46e2eb15197b..b38f50810459 100644
> --- a/drivers/iommu/intel/iommu.c
> +++ b/drivers/iommu/intel/iommu.c
> @@ -27,7 +27,7 @@
> #include <linux/tboot.h>
>
> #include "../irq_remapping.h"
> -#include "../iommu-sva-lib.h"
> +#include "../iommu-sva.h"
> #include "pasid.h"
> #include "cap_audit.h"
>
> diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c
> index 6084f960ba27..38c33cde177e 100644
> --- a/drivers/iommu/intel/svm.c
> +++ b/drivers/iommu/intel/svm.c
> @@ -25,7 +25,7 @@
>
> #include "pasid.h"
> #include "perf.h"
> -#include "../iommu-sva-lib.h"
> +#include "../iommu-sva.h"
>
> static irqreturn_t prq_event_thread(int irq, void *d);
> static void intel_svm_drain_prq(struct device *dev, u32 pasid);
> diff --git a/drivers/iommu/io-pgfault.c b/drivers/iommu/io-pgfault.c
> index 8a2bb56e1474..a9ecf6bf5500 100644
> --- a/drivers/iommu/io-pgfault.c
> +++ b/drivers/iommu/io-pgfault.c
> @@ -11,7 +11,7 @@
> #include <linux/slab.h>
> #include <linux/workqueue.h>
>
> -#include "iommu-sva-lib.h"
> +#include "iommu-sva.h"
>
> /**
> * struct iopf_queue - IO Page Fault queue
> diff --git a/drivers/iommu/iommu-sva-lib.c b/drivers/iommu/iommu-sva.c
> similarity index 99%
> rename from drivers/iommu/iommu-sva-lib.c
> rename to drivers/iommu/iommu-sva.c
> index ae3595d60f38..b631765fa8c0 100644
> --- a/drivers/iommu/iommu-sva-lib.c
> +++ b/drivers/iommu/iommu-sva.c
> @@ -7,7 +7,7 @@
> #include <linux/slab.h>
> #include <linux/sched/mm.h>
>
> -#include "iommu-sva-lib.h"
> +#include "iommu-sva.h"
>
> static DEFINE_MUTEX(iommu_sva_lock);
> static DECLARE_IOASID_SET(iommu_sva_pasid);
> diff --git a/drivers/iommu/Makefile b/drivers/iommu/Makefile
> index 44475a9b3eea..c1763476162b 100644
> --- a/drivers/iommu/Makefile
> +++ b/drivers/iommu/Makefile
> @@ -27,6 +27,6 @@ obj-$(CONFIG_FSL_PAMU) += fsl_pamu.o fsl_pamu_domain.o
> obj-$(CONFIG_S390_IOMMU) += s390-iommu.o
> obj-$(CONFIG_HYPERV_IOMMU) += hyperv-iommu.o
> obj-$(CONFIG_VIRTIO_IOMMU) += virtio-iommu.o
> -obj-$(CONFIG_IOMMU_SVA) += iommu-sva-lib.o io-pgfault.o
> +obj-$(CONFIG_IOMMU_SVA) += iommu-sva.o io-pgfault.o
> obj-$(CONFIG_SPRD_IOMMU) += sprd-iommu.o
> obj-$(CONFIG_APPLE_DART) += apple-dart.o
> --
> 2.25.1
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v5 02/12] iommu: Add pasid_bits field in struct dev_iommu
2022-05-03 18:02 ` Jean-Philippe Brucker
@ 2022-05-05 6:25 ` Baolu Lu
0 siblings, 0 replies; 30+ messages in thread
From: Baolu Lu @ 2022-05-05 6:25 UTC (permalink / raw)
To: Jean-Philippe Brucker
Cc: Joerg Roedel, Jason Gunthorpe, Christoph Hellwig, Kevin Tian,
Ashok Raj, Will Deacon, Robin Murphy, Jean-Philippe Brucker,
Dave Jiang, Vinod Koul, Eric Auger, Liu Yi L, Jacob jun Pan,
iommu, linux-kernel
On 2022/5/4 02:02, Jean-Philippe Brucker wrote:
> On Mon, May 02, 2022 at 09:48:32AM +0800, Lu Baolu wrote:
>> Use this field to save the pasid/ssid bits that a device is able to
>> support with its IOMMU hardware. It is a generic attribute of a device
>> and lifting it into the per-device dev_iommu struct makes it possible
>> to allocate a PASID for device without calls into the IOMMU drivers.
>> Any iommu driver which suports PASID related features should set this
>> field before features are enabled on the devices.
>>
>> For initialization of this field in the VT-d driver, the
>> info->pasid_supported is only set for PCI devices. So the status is
>> that non-PCI SVA hasn't been supported yet. Setting this field only for
>> PCI devices has no functional change.
>>
>> Signed-off-by: Lu Baolu<baolu.lu@linux.intel.com>
> Reviewed-by: Jean-Philippe Brucker<jean-philippe@linaro.org>
Thank you! Very appreciated for reviewing my patches.
Best regards,
baolu
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v5 03/12] iommu: Add attach/detach_dev_pasid domain ops
2022-05-03 18:07 ` Jean-Philippe Brucker
@ 2022-05-05 6:28 ` Baolu Lu
0 siblings, 0 replies; 30+ messages in thread
From: Baolu Lu @ 2022-05-05 6:28 UTC (permalink / raw)
To: Jean-Philippe Brucker
Cc: Joerg Roedel, Jason Gunthorpe, Christoph Hellwig, Kevin Tian,
Ashok Raj, Will Deacon, Robin Murphy, Jean-Philippe Brucker,
Dave Jiang, Vinod Koul, Eric Auger, Liu Yi L, Jacob jun Pan,
iommu, linux-kernel
On 2022/5/4 02:07, Jean-Philippe Brucker wrote:
> On Mon, May 02, 2022 at 09:48:33AM +0800, Lu Baolu wrote:
>> Attaching an IOMMU domain to a PASID of a device is a generic operation
>> for modern IOMMU drivers which support PASID-granular DMA address
>> translation. Currently visible usage scenarios include (but not limited):
>>
>> - SVA (Shared Virtual Address)
>> - kernel DMA with PASID
>> - hardware-assist mediated device
>>
>> This adds a pair of common domain ops for this purpose and adds helpers
>> to attach/detach a domain to/from a {device, PASID}. Some buses, like
>> PCI, route packets without considering the PASID value. Thus a DMA target
>> address with PASID might be treated as P2P if the address falls into the
>> MMIO BAR of other devices in the group. To make things simple, these
>> interfaces only apply to devices belonging to the singleton groups, and
>> the singleton is immutable in fabric i.e. not affected by hotplug.
>>
>> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
>
> Reviewed-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
>
> just a nit below
>
>> ---
>> include/linux/iommu.h | 21 ++++++++++++
>> drivers/iommu/iommu.c | 76 +++++++++++++++++++++++++++++++++++++++++++
>> 2 files changed, 97 insertions(+)
>>
>> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
>> index b8ffaf2cb1d0..ab36244d4e94 100644
>> --- a/include/linux/iommu.h
>> +++ b/include/linux/iommu.h
>> @@ -263,6 +263,8 @@ struct iommu_ops {
>> * struct iommu_domain_ops - domain specific operations
>> * @attach_dev: attach an iommu domain to a device
>> * @detach_dev: detach an iommu domain from a device
>> + * @attach_dev_pasid: attach an iommu domain to a pasid of device
>> + * @detach_dev_pasid: detach an iommu domain from a pasid of device
>> * @map: map a physically contiguous memory region to an iommu domain
>> * @map_pages: map a physically contiguous set of pages of the same size to
>> * an iommu domain.
>> @@ -283,6 +285,10 @@ struct iommu_ops {
>> struct iommu_domain_ops {
>> int (*attach_dev)(struct iommu_domain *domain, struct device *dev);
>> void (*detach_dev)(struct iommu_domain *domain, struct device *dev);
>> + int (*attach_dev_pasid)(struct iommu_domain *domain,
>> + struct device *dev, ioasid_t pasid);
>> + void (*detach_dev_pasid)(struct iommu_domain *domain,
>> + struct device *dev, ioasid_t pasid);
>>
>> int (*map)(struct iommu_domain *domain, unsigned long iova,
>> phys_addr_t paddr, size_t size, int prot, gfp_t gfp);
>> @@ -678,6 +684,10 @@ int iommu_group_claim_dma_owner(struct iommu_group *group, void *owner);
>> void iommu_group_release_dma_owner(struct iommu_group *group);
>> bool iommu_group_dma_owner_claimed(struct iommu_group *group);
>>
>> +int iommu_attach_device_pasid(struct iommu_domain *domain,
>> + struct device *dev, ioasid_t pasid);
>> +void iommu_detach_device_pasid(struct iommu_domain *domain,
>> + struct device *dev, ioasid_t pasid);
>> #else /* CONFIG_IOMMU_API */
>>
>> struct iommu_ops {};
>> @@ -1051,6 +1061,17 @@ static inline bool iommu_group_dma_owner_claimed(struct iommu_group *group)
>> {
>> return false;
>> }
>> +
>> +static inline int iommu_attach_device_pasid(struct iommu_domain *domain,
>> + struct device *dev, ioasid_t pasid)
>> +{
>> + return -ENODEV;
>> +}
>> +
>> +static inline void iommu_detach_device_pasid(struct iommu_domain *domain,
>> + struct device *dev, ioasid_t pasid)
>> +{
>> +}
>> #endif /* CONFIG_IOMMU_API */
>>
>> /**
>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>> index 29906bc16371..89c9d19ddb28 100644
>> --- a/drivers/iommu/iommu.c
>> +++ b/drivers/iommu/iommu.c
>> @@ -38,6 +38,7 @@ struct iommu_group {
>> struct kobject kobj;
>> struct kobject *devices_kobj;
>> struct list_head devices;
>> + struct xarray pasid_array;
>> struct mutex mutex;
>> void *iommu_data;
>> void (*iommu_data_release)(void *iommu_data);
>> @@ -630,6 +631,7 @@ struct iommu_group *iommu_group_alloc(void)
>> mutex_init(&group->mutex);
>> INIT_LIST_HEAD(&group->devices);
>> INIT_LIST_HEAD(&group->entry);
>> + xa_init(&group->pasid_array);
>>
>> ret = ida_simple_get(&iommu_group_ida, 0, 0, GFP_KERNEL);
>> if (ret < 0) {
>> @@ -3190,3 +3192,77 @@ bool iommu_group_dma_owner_claimed(struct iommu_group *group)
>> return user;
>> }
>> EXPORT_SYMBOL_GPL(iommu_group_dma_owner_claimed);
>> +
>> +/*
>> + * Use standard PCI bus topology and isolation features to check immutable
>> + * singleton. Otherwise, assume the bus is static and then singleton can
>> + * know from the device count in the group.
>> + */
>
> The comment doesn't really add anything that can't be directly understood
> from the code.
Yes. It's fine to remove it.
Best regards,
baolu
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v5 04/12] iommu/sva: Basic data structures for SVA
2022-05-03 18:09 ` Jean-Philippe Brucker
@ 2022-05-05 6:42 ` Baolu Lu
2022-05-07 8:32 ` Baolu Lu
0 siblings, 1 reply; 30+ messages in thread
From: Baolu Lu @ 2022-05-05 6:42 UTC (permalink / raw)
To: Jean-Philippe Brucker
Cc: Joerg Roedel, Jason Gunthorpe, Christoph Hellwig, Kevin Tian,
Ashok Raj, Will Deacon, Robin Murphy, Jean-Philippe Brucker,
Dave Jiang, Vinod Koul, Eric Auger, Liu Yi L, Jacob jun Pan,
iommu, linux-kernel
On 2022/5/4 02:09, Jean-Philippe Brucker wrote:
> On Mon, May 02, 2022 at 09:48:34AM +0800, Lu Baolu wrote:
>> Use below data structures for SVA implementation in the IOMMU core:
>>
>> - struct iommu_sva_ioas
>> Represent the I/O address space shared with an application CPU address
>> space. This structure has a 1:1 relationship with an mm_struct. It
>> grabs a "mm->mm_count" refcount during creation and drop it on release.
>
> Do we actually need this structure? At the moment it only keeps track of
> bonds, which we can move to struct dev_iommu. Replacing it by a mm pointer
> in struct iommu_domain simplifies the driver and seems to work
Fair enough.
+struct iommu_sva_ioas {
+ struct mm_struct *mm;
+ ioasid_t pasid;
+
+ /* Counter of domains attached to this ioas. */
+ refcount_t users;
+
+ /* All bindings are linked here. */
+ struct list_head bonds;
+};
By moving @mm to struct iommu_domain and @bonds to struct dev_iommu, the
code looks simpler. The mm, sva domain and per-device dev_iommu are
guaranteed to be valid during bind() and unbind().
Will head this direction in the next version.
>
> Thanks,
> Jean
Best regards,
baolu
>
>>
>> - struct iommu_domain (IOMMU_DOMAIN_SVA type)
>> Represent a hardware pagetable that the IOMMU hardware could use for
>> SVA translation. Multiple iommu domains could be bound with an SVA ioas
>> and each grabs a refcount from ioas in order to make sure ioas could
>> only be freed after all domains have been unbound.
>>
>> - struct iommu_sva
>> Represent a bond relationship between an SVA ioas and an iommu domain.
>> If a bond already exists, it's reused and a reference is taken.
>>
>> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
>> ---
>> include/linux/iommu.h | 14 +++++++++++++-
>> drivers/iommu/iommu-sva-lib.h | 1 +
>> drivers/iommu/iommu-sva-lib.c | 18 ++++++++++++++++++
>> 3 files changed, 32 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
>> index ab36244d4e94..f582f434c513 100644
>> --- a/include/linux/iommu.h
>> +++ b/include/linux/iommu.h
>> @@ -42,6 +42,7 @@ struct notifier_block;
>> struct iommu_sva;
>> struct iommu_fault_event;
>> struct iommu_dma_cookie;
>> +struct iommu_sva_ioas;
>>
>> /* iommu fault flags */
>> #define IOMMU_FAULT_READ 0x0
>> @@ -64,6 +65,9 @@ struct iommu_domain_geometry {
>> #define __IOMMU_DOMAIN_PT (1U << 2) /* Domain is identity mapped */
>> #define __IOMMU_DOMAIN_DMA_FQ (1U << 3) /* DMA-API uses flush queue */
>>
>> +#define __IOMMU_DOMAIN_SHARED (1U << 4) /* Page table shared from CPU */
>> +#define __IOMMU_DOMAIN_HOST_VA (1U << 5) /* Host CPU virtual address */
>> +
>> /*
>> * This are the possible domain-types
>> *
>> @@ -86,6 +90,8 @@ struct iommu_domain_geometry {
>> #define IOMMU_DOMAIN_DMA_FQ (__IOMMU_DOMAIN_PAGING | \
>> __IOMMU_DOMAIN_DMA_API | \
>> __IOMMU_DOMAIN_DMA_FQ)
>> +#define IOMMU_DOMAIN_SVA (__IOMMU_DOMAIN_SHARED | \
>> + __IOMMU_DOMAIN_HOST_VA)
>>
>> struct iommu_domain {
>> unsigned type;
>> @@ -95,6 +101,7 @@ struct iommu_domain {
>> void *handler_token;
>> struct iommu_domain_geometry geometry;
>> struct iommu_dma_cookie *iova_cookie;
>> + struct iommu_sva_ioas *sva_ioas;
>> };
>>
>> static inline bool iommu_is_dma_domain(struct iommu_domain *domain)
>> @@ -628,7 +635,12 @@ struct iommu_fwspec {
>> * struct iommu_sva - handle to a device-mm bond
>> */
>> struct iommu_sva {
>> - struct device *dev;
>> + struct device *dev;
>> + struct iommu_sva_ioas *sva_ioas;
>> + struct iommu_domain *domain;
>> + /* Link to sva ioas's bonds list */
>> + struct list_head node;
>> + refcount_t users;
>> };
>>
>> int iommu_fwspec_init(struct device *dev, struct fwnode_handle *iommu_fwnode,
>> diff --git a/drivers/iommu/iommu-sva-lib.h b/drivers/iommu/iommu-sva-lib.h
>> index 8909ea1094e3..9c5e108e2c8a 100644
>> --- a/drivers/iommu/iommu-sva-lib.h
>> +++ b/drivers/iommu/iommu-sva-lib.h
>> @@ -10,6 +10,7 @@
>>
>> int iommu_sva_alloc_pasid(struct mm_struct *mm, ioasid_t min, ioasid_t max);
>> struct mm_struct *iommu_sva_find(ioasid_t pasid);
>> +struct mm_struct *iommu_sva_domain_mm(struct iommu_domain *domain);
>>
>> /* I/O Page fault */
>> struct device;
>> diff --git a/drivers/iommu/iommu-sva-lib.c b/drivers/iommu/iommu-sva-lib.c
>> index 106506143896..d524a402be3b 100644
>> --- a/drivers/iommu/iommu-sva-lib.c
>> +++ b/drivers/iommu/iommu-sva-lib.c
>> @@ -3,6 +3,8 @@
>> * Helpers for IOMMU drivers implementing SVA
>> */
>> #include <linux/mutex.h>
>> +#include <linux/iommu.h>
>> +#include <linux/slab.h>
>> #include <linux/sched/mm.h>
>>
>> #include "iommu-sva-lib.h"
>> @@ -10,6 +12,22 @@
>> static DEFINE_MUTEX(iommu_sva_lock);
>> static DECLARE_IOASID_SET(iommu_sva_pasid);
>>
>> +struct iommu_sva_ioas {
>> + struct mm_struct *mm;
>> + ioasid_t pasid;
>> +
>> + /* Counter of domains attached to this ioas. */
>> + refcount_t users;
>> +
>> + /* All bindings are linked here. */
>> + struct list_head bonds;
>> +};
>> +
>> +struct mm_struct *iommu_sva_domain_mm(struct iommu_domain *domain)
>> +{
>> + return domain->sva_ioas->mm;
>> +}
>> +
>> /**
>> * iommu_sva_alloc_pasid - Allocate a PASID for the mm
>> * @mm: the mm
>> --
>> 2.25.1
>>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v5 07/12] arm-smmu-v3/sva: Add SVA domain support
2022-05-03 18:12 ` Jean-Philippe Brucker
@ 2022-05-05 7:09 ` Baolu Lu
0 siblings, 0 replies; 30+ messages in thread
From: Baolu Lu @ 2022-05-05 7:09 UTC (permalink / raw)
To: Jean-Philippe Brucker
Cc: Joerg Roedel, Jason Gunthorpe, Christoph Hellwig, Kevin Tian,
Ashok Raj, Will Deacon, Robin Murphy, Jean-Philippe Brucker,
Dave Jiang, Vinod Koul, Eric Auger, Liu Yi L, Jacob jun Pan,
iommu, linux-kernel
On 2022/5/4 02:12, Jean-Philippe Brucker wrote:
> On Mon, May 02, 2022 at 09:48:37AM +0800, Lu Baolu wrote:
>> Add support for SVA domain allocation and provide an SVA-specific
>> iommu_domain_ops.
>>
>> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
>> ---
>> drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 14 +++++++
>> .../iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c | 42 +++++++++++++++++++
>> drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 21 ++++++++++
>> 3 files changed, 77 insertions(+)
>>
>> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
>> index cd48590ada30..7631c00fdcbd 100644
>> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
>> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
>> @@ -759,6 +759,10 @@ struct iommu_sva *arm_smmu_sva_bind(struct device *dev, struct mm_struct *mm,
>> void arm_smmu_sva_unbind(struct iommu_sva *handle);
>> u32 arm_smmu_sva_get_pasid(struct iommu_sva *handle);
>> void arm_smmu_sva_notifier_synchronize(void);
>> +int arm_smmu_sva_attach_dev_pasid(struct iommu_domain *domain,
>> + struct device *dev, ioasid_t id);
>> +void arm_smmu_sva_detach_dev_pasid(struct iommu_domain *domain,
>> + struct device *dev, ioasid_t id);
>> #else /* CONFIG_ARM_SMMU_V3_SVA */
>> static inline bool arm_smmu_sva_supported(struct arm_smmu_device *smmu)
>> {
>> @@ -804,5 +808,15 @@ static inline u32 arm_smmu_sva_get_pasid(struct iommu_sva *handle)
>> }
>>
>> static inline void arm_smmu_sva_notifier_synchronize(void) {}
>> +
>> +static inline int arm_smmu_sva_attach_dev_pasid(struct iommu_domain *domain,
>> + struct device *dev, ioasid_t id)
>> +{
>> + return -ENODEV;
>> +}
>> +
>> +static inline void arm_smmu_sva_detach_dev_pasid(struct iommu_domain *domain,
>> + struct device *dev,
>> + ioasid_t id) {}
>> #endif /* CONFIG_ARM_SMMU_V3_SVA */
>> #endif /* _ARM_SMMU_V3_H */
>> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
>> index c623dae1e115..3b843cd3ed67 100644
>> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
>> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
>> @@ -541,3 +541,45 @@ void arm_smmu_sva_notifier_synchronize(void)
>> */
>> mmu_notifier_synchronize();
>> }
>> +
>> +int arm_smmu_sva_attach_dev_pasid(struct iommu_domain *domain,
>> + struct device *dev, ioasid_t id)
>> +{
>> + int ret = 0;
>> + struct iommu_sva *handle;
>> + struct mm_struct *mm = iommu_sva_domain_mm(domain);
>> +
>> + if (domain->type != IOMMU_DOMAIN_SVA || !mm)
>
> We wouldn't get that far with a non-SVA domain since iommu_sva_domain_mm()
> would dereference a NULL pointer. Could you move it after the domain->type
> check, and maybe add a WARN_ON()? It could help catch issues in future
> API changes.
Sure. I will make it like this,
int arm_smmu_sva_attach_dev_pasid(struct iommu_domain *domain,
struct device *dev, ioasid_t id)
{
int ret = 0;
struct mm_struct *mm;
struct iommu_sva *handle;
if (domain->type != IOMMU_DOMAIN_SVA)
return -EINVAL;
mm = iommu_sva_domain_mm(domain);
if (WARN_ON(!mm))
return -ENODEV;
... ...
>
>> + return -EINVAL;
>> +
>> + mutex_lock(&sva_lock);
>> + handle = __arm_smmu_sva_bind(dev, mm);
>> + if (IS_ERR(handle))
>> + ret = PTR_ERR(handle);
>> + mutex_unlock(&sva_lock);
>> +
>> + return ret;
>> +}
>> +
>> +void arm_smmu_sva_detach_dev_pasid(struct iommu_domain *domain,
>> + struct device *dev, ioasid_t id)
>> +{
>> + struct arm_smmu_bond *bond = NULL, *t;
>> + struct mm_struct *mm = iommu_sva_domain_mm(domain);
>> + struct arm_smmu_master *master = dev_iommu_priv_get(dev);
>> +
>> + mutex_lock(&sva_lock);
>> + list_for_each_entry(t, &master->bonds, list) {
>> + if (t->mm == mm) {
>> + bond = t;
>> + break;
>> + }
>> + }
>> +
>> + if (!WARN_ON(!bond) && refcount_dec_and_test(&bond->refs)) {
>> + list_del(&bond->list);
>> + arm_smmu_mmu_notifier_put(bond->smmu_mn);
>> + kfree(bond);
>> + }
>> + mutex_unlock(&sva_lock);
>> +}
>> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>> index afc63fce6107..bd80de0bad98 100644
>> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>> @@ -1995,10 +1995,31 @@ static bool arm_smmu_capable(enum iommu_cap cap)
>> }
>> }
>>
>> +static void arm_smmu_sva_domain_free(struct iommu_domain *domain)
>> +{
>> + kfree(domain);
>> +}
>> +
>> +static const struct iommu_domain_ops arm_smmu_sva_domain_ops = {
>> + .attach_dev_pasid = arm_smmu_sva_attach_dev_pasid,
>> + .detach_dev_pasid = arm_smmu_sva_detach_dev_pasid,
>> + .free = arm_smmu_sva_domain_free,
>> +};
>> +
>> static struct iommu_domain *arm_smmu_domain_alloc(unsigned type)
>> {
>> struct arm_smmu_domain *smmu_domain;
>>
>> + if (type == IOMMU_DOMAIN_SVA) {
>> + struct iommu_domain *domain;
>> +
>> + domain = kzalloc(sizeof(*domain), GFP_KERNEL);
>> + if (domain)
>> + domain->ops = &arm_smmu_sva_domain_ops;
>> +
>> + return domain;
>> + }
>> +
>
> I'd prefer moving all of this to arm-smmu-v3-sva.c and just call
> arm_smmu_sva_domain_alloc() here
Sure.
>
> Otherwise the patch looks fine. I'll rework the driver when I find some
> time, because we can now remove arm_smmu_bond and move smmu_mn to the SVA
> domain, maybe also remove sva_lock but I haven't thought it through.
Yes. Intel SVA code also needs further cleanup. It's in my non-urgent
task list.
>
> Thanks,
> Jean
>
>> if (type != IOMMU_DOMAIN_UNMANAGED &&
>> type != IOMMU_DOMAIN_DMA &&
>> type != IOMMU_DOMAIN_DMA_FQ &&
>> --
>> 2.25.1
>>
Best regards,
baolu
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v5 10/12] iommu: Prepare IOMMU domain for IOPF
2022-05-03 18:20 ` Jean-Philippe Brucker
@ 2022-05-05 8:31 ` Baolu Lu
2022-05-05 13:38 ` Jean-Philippe Brucker
0 siblings, 1 reply; 30+ messages in thread
From: Baolu Lu @ 2022-05-05 8:31 UTC (permalink / raw)
To: Jean-Philippe Brucker
Cc: Joerg Roedel, Jason Gunthorpe, Christoph Hellwig, Kevin Tian,
Ashok Raj, Will Deacon, Robin Murphy, Jean-Philippe Brucker,
Dave Jiang, Vinod Koul, Eric Auger, Liu Yi L, Jacob jun Pan,
iommu, linux-kernel
On 2022/5/4 02:20, Jean-Philippe Brucker wrote:
>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>> index 7cae631c1baa..33449523afbe 100644
>> --- a/drivers/iommu/iommu.c
>> +++ b/drivers/iommu/iommu.c
>> @@ -3174,3 +3174,24 @@ void iommu_detach_device_pasid(struct iommu_domain *domain,
>>
>> iommu_group_put(group);
>> }
>> +
>> +struct iommu_domain *iommu_get_domain_for_dev_pasid(struct device *dev,
>> + ioasid_t pasid)
>> +{
>> + struct iommu_domain *domain;
>> + struct iommu_group *group;
>> +
>> + if (!pasid_valid(pasid))
>> + return NULL;
>> +
>> + group = iommu_group_get(dev);
>> + if (!group)
>> + return NULL;
>> +
>> + mutex_lock(&group->mutex);
> Unfortunately this still causes the deadlock when unbind() flushes the
> IOPF queue while holding the group mutex.
Sorry, I didn't get your point here.
Do you mean unbind() could hold group mutex before calling this helper?
The group mutex is only available in iommu.c. The unbind() has no means
to hold this lock. Or, I missed anything?
Best regards,
baolu
>
> If we make this function private to IOPF, then we can get rid of this
> mutex_lock(). It's OK because:
>
> * xarray protects its internal state with RCU, so we can call
> xa_load() outside the lock.
>
> * The domain obtained from xa_load is finalized. Its content is valid
> because xarray stores the domain using rcu_assign_pointer(), which has a
> release memory barrier, which pairs with data dependencies in IOPF
> (domain->sva_ioas etc).
>
> We'll need to be careful about this when allowing other users to install
> a fault handler. Should be fine as long as the handler and data are
> installed before the domain is added to pasid_array.
>
> * We know the domain is valid the whole time IOPF is using it, because
> unbind() waits for pending faults.
>
> We just need a comment explaining the last point, something like:
>
> /*
> * Safe to fetch outside the group mutex because:
> * - xarray protects its internal state with RCU
> * - the domain obtained is either NULL or fully formed
> * - the IOPF work is the only caller and is flushed before the
> * domain is freed.
> */
>
> Thanks,
> Jean
>
>> + domain = xa_load(&group->pasid_array, pasid);
>> + mutex_unlock(&group->mutex);
>> + iommu_group_put(group);
>> +
>> + return domain;
>> +}
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v5 10/12] iommu: Prepare IOMMU domain for IOPF
2022-05-05 8:31 ` Baolu Lu
@ 2022-05-05 13:38 ` Jean-Philippe Brucker
2022-05-06 5:40 ` Baolu Lu
0 siblings, 1 reply; 30+ messages in thread
From: Jean-Philippe Brucker @ 2022-05-05 13:38 UTC (permalink / raw)
To: Baolu Lu
Cc: Joerg Roedel, Jason Gunthorpe, Christoph Hellwig, Kevin Tian,
Ashok Raj, Will Deacon, Robin Murphy, Jean-Philippe Brucker,
Dave Jiang, Vinod Koul, Eric Auger, Liu Yi L, Jacob jun Pan,
iommu, linux-kernel
Hi Baolu,
On Thu, May 05, 2022 at 04:31:38PM +0800, Baolu Lu wrote:
> On 2022/5/4 02:20, Jean-Philippe Brucker wrote:
> > > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> > > index 7cae631c1baa..33449523afbe 100644
> > > --- a/drivers/iommu/iommu.c
> > > +++ b/drivers/iommu/iommu.c
> > > @@ -3174,3 +3174,24 @@ void iommu_detach_device_pasid(struct iommu_domain *domain,
> > > iommu_group_put(group);
> > > }
> > > +
> > > +struct iommu_domain *iommu_get_domain_for_dev_pasid(struct device *dev,
> > > + ioasid_t pasid)
> > > +{
> > > + struct iommu_domain *domain;
> > > + struct iommu_group *group;
> > > +
> > > + if (!pasid_valid(pasid))
> > > + return NULL;
> > > +
> > > + group = iommu_group_get(dev);
> > > + if (!group)
> > > + return NULL;
> > > +
> > > + mutex_lock(&group->mutex);
> > Unfortunately this still causes the deadlock when unbind() flushes the
> > IOPF queue while holding the group mutex.
>
> Sorry, I didn't get your point here.
>
> Do you mean unbind() could hold group mutex before calling this helper?
> The group mutex is only available in iommu.c. The unbind() has no means
> to hold this lock. Or, I missed anything?
I wasn't clear, it's iommu_detach_device_pasid() that holds the
group->mutex:
iommu_sva_unbind_device() |
iommu_detach_device_pasid() |
mutex_lock(&group->mutex) |
domain->ops->detach_dev_pasid() | iopf_handle_group()
iopf_queue_flush_dev() | iommu_get_domain_for_dev_pasid()
... wait for IOPF work | mutex_lock(&group->mutex)
| ... deadlock
Thanks,
Jean
>
> Best regards,
> baolu
>
> >
> > If we make this function private to IOPF, then we can get rid of this
> > mutex_lock(). It's OK because:
> >
> > * xarray protects its internal state with RCU, so we can call
> > xa_load() outside the lock.
> >
> > * The domain obtained from xa_load is finalized. Its content is valid
> > because xarray stores the domain using rcu_assign_pointer(), which has a
> > release memory barrier, which pairs with data dependencies in IOPF
> > (domain->sva_ioas etc).
> >
> > We'll need to be careful about this when allowing other users to install
> > a fault handler. Should be fine as long as the handler and data are
> > installed before the domain is added to pasid_array.
> >
> > * We know the domain is valid the whole time IOPF is using it, because
> > unbind() waits for pending faults.
> >
> > We just need a comment explaining the last point, something like:
> >
> > /*
> > * Safe to fetch outside the group mutex because:
> > * - xarray protects its internal state with RCU
> > * - the domain obtained is either NULL or fully formed
> > * - the IOPF work is the only caller and is flushed before the
> > * domain is freed.
> > */
> >
> > Thanks,
> > Jean
> >
> > > + domain = xa_load(&group->pasid_array, pasid);
> > > + mutex_unlock(&group->mutex);
> > > + iommu_group_put(group);
> > > +
> > > + return domain;
> > > +}
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v5 10/12] iommu: Prepare IOMMU domain for IOPF
2022-05-05 13:38 ` Jean-Philippe Brucker
@ 2022-05-06 5:40 ` Baolu Lu
0 siblings, 0 replies; 30+ messages in thread
From: Baolu Lu @ 2022-05-06 5:40 UTC (permalink / raw)
To: Jean-Philippe Brucker
Cc: Joerg Roedel, Jason Gunthorpe, Christoph Hellwig, Kevin Tian,
Ashok Raj, Will Deacon, Robin Murphy, Jean-Philippe Brucker,
Dave Jiang, Vinod Koul, Eric Auger, Liu Yi L, Jacob jun Pan,
iommu, linux-kernel
On 2022/5/5 21:38, Jean-Philippe Brucker wrote:
> Hi Baolu,
>
> On Thu, May 05, 2022 at 04:31:38PM +0800, Baolu Lu wrote:
>> On 2022/5/4 02:20, Jean-Philippe Brucker wrote:
>>>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>>>> index 7cae631c1baa..33449523afbe 100644
>>>> --- a/drivers/iommu/iommu.c
>>>> +++ b/drivers/iommu/iommu.c
>>>> @@ -3174,3 +3174,24 @@ void iommu_detach_device_pasid(struct iommu_domain *domain,
>>>> iommu_group_put(group);
>>>> }
>>>> +
>>>> +struct iommu_domain *iommu_get_domain_for_dev_pasid(struct device *dev,
>>>> + ioasid_t pasid)
>>>> +{
>>>> + struct iommu_domain *domain;
>>>> + struct iommu_group *group;
>>>> +
>>>> + if (!pasid_valid(pasid))
>>>> + return NULL;
>>>> +
>>>> + group = iommu_group_get(dev);
>>>> + if (!group)
>>>> + return NULL;
>>>> +
>>>> + mutex_lock(&group->mutex);
>>> Unfortunately this still causes the deadlock when unbind() flushes the
>>> IOPF queue while holding the group mutex.
>>
>> Sorry, I didn't get your point here.
>>
>> Do you mean unbind() could hold group mutex before calling this helper?
>> The group mutex is only available in iommu.c. The unbind() has no means
>> to hold this lock. Or, I missed anything?
>
> I wasn't clear, it's iommu_detach_device_pasid() that holds the
> group->mutex:
>
> iommu_sva_unbind_device() |
> iommu_detach_device_pasid() |
> mutex_lock(&group->mutex) |
> domain->ops->detach_dev_pasid() | iopf_handle_group()
> iopf_queue_flush_dev() | iommu_get_domain_for_dev_pasid()
> ... wait for IOPF work | mutex_lock(&group->mutex)
> | ... deadlock
Ah! Yes. Thank you for the clarification.
>
> Thanks,
> Jean
>
>>
>> Best regards,
>> baolu
>>
>>>
>>> If we make this function private to IOPF, then we can get rid of this
>>> mutex_lock(). It's OK because:
>>>
>>> * xarray protects its internal state with RCU, so we can call
>>> xa_load() outside the lock.
>>>
>>> * The domain obtained from xa_load is finalized. Its content is valid
>>> because xarray stores the domain using rcu_assign_pointer(), which has a
>>> release memory barrier, which pairs with data dependencies in IOPF
>>> (domain->sva_ioas etc).
>>>
>>> We'll need to be careful about this when allowing other users to install
>>> a fault handler. Should be fine as long as the handler and data are
>>> installed before the domain is added to pasid_array.
>>>
>>> * We know the domain is valid the whole time IOPF is using it, because
>>> unbind() waits for pending faults.
>>>
>>> We just need a comment explaining the last point, something like:
>>>
>>> /*
>>> * Safe to fetch outside the group mutex because:
>>> * - xarray protects its internal state with RCU
>>> * - the domain obtained is either NULL or fully formed
>>> * - the IOPF work is the only caller and is flushed before the
>>> * domain is freed.
>>> */
Agreed. The mutex is needed only when domain could possibly be freed
before unbind(). In that case, we need this mutex and get a reference
from the domain. As we have dropped the domain user reference, this lock
is unnecessary.
>>>
>>> Thanks,
>>> Jean
>>>
>>>> + domain = xa_load(&group->pasid_array, pasid);
>>>> + mutex_unlock(&group->mutex);
>>>> + iommu_group_put(group);
>>>> +
>>>> + return domain;
>>>> +}
>>
Best regards,
baolu
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v5 04/12] iommu/sva: Basic data structures for SVA
2022-05-05 6:42 ` Baolu Lu
@ 2022-05-07 8:32 ` Baolu Lu
2022-05-07 12:39 ` Baolu Lu
0 siblings, 1 reply; 30+ messages in thread
From: Baolu Lu @ 2022-05-07 8:32 UTC (permalink / raw)
To: Jean-Philippe Brucker
Cc: Kevin Tian, Dave Jiang, Ashok Raj, Robin Murphy, iommu,
linux-kernel, Christoph Hellwig, Jean-Philippe Brucker,
Vinod Koul, Jacob jun Pan, Jason Gunthorpe, Will Deacon
Hi Jean,
On 2022/5/5 14:42, Baolu Lu wrote:
> On 2022/5/4 02:09, Jean-Philippe Brucker wrote:
>> On Mon, May 02, 2022 at 09:48:34AM +0800, Lu Baolu wrote:
>>> Use below data structures for SVA implementation in the IOMMU core:
>>>
>>> - struct iommu_sva_ioas
>>> Represent the I/O address space shared with an application CPU
>>> address
>>> space. This structure has a 1:1 relationship with an mm_struct. It
>>> grabs a "mm->mm_count" refcount during creation and drop it on
>>> release.
>>
>> Do we actually need this structure? At the moment it only keeps track of
>> bonds, which we can move to struct dev_iommu. Replacing it by a mm
>> pointer
>> in struct iommu_domain simplifies the driver and seems to work
>
> Fair enough.
>
> +struct iommu_sva_ioas {
> + struct mm_struct *mm;
> + ioasid_t pasid;
> +
> + /* Counter of domains attached to this ioas. */
> + refcount_t users;
> +
> + /* All bindings are linked here. */
> + struct list_head bonds;
> +};
>
> By moving @mm to struct iommu_domain and @bonds to struct dev_iommu, the
> code looks simpler. The mm, sva domain and per-device dev_iommu are
> guaranteed to be valid during bind() and unbind().
>
> Will head this direction in the next version.
I'm trying to implement this idea in real code. It seems that we need
additional fields in struct iommu_domain to track which devices the mm
was bound to. It doesn't simplify the code much. Any thoughts?
Best regards,
baolu
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v5 04/12] iommu/sva: Basic data structures for SVA
2022-05-07 8:32 ` Baolu Lu
@ 2022-05-07 12:39 ` Baolu Lu
0 siblings, 0 replies; 30+ messages in thread
From: Baolu Lu @ 2022-05-07 12:39 UTC (permalink / raw)
To: Jean-Philippe Brucker
Cc: Vinod Koul, Kevin Tian, Dave Jiang, Ashok Raj, Will Deacon,
Jean-Philippe Brucker, linux-kernel, Christoph Hellwig, iommu,
Jacob jun Pan, Jason Gunthorpe, Robin Murphy
On 2022/5/7 16:32, Baolu Lu wrote:
> Hi Jean,
>
> On 2022/5/5 14:42, Baolu Lu wrote:
>> On 2022/5/4 02:09, Jean-Philippe Brucker wrote:
>>> On Mon, May 02, 2022 at 09:48:34AM +0800, Lu Baolu wrote:
>>>> Use below data structures for SVA implementation in the IOMMU core:
>>>>
>>>> - struct iommu_sva_ioas
>>>> Represent the I/O address space shared with an application CPU
>>>> address
>>>> space. This structure has a 1:1 relationship with an mm_struct. It
>>>> grabs a "mm->mm_count" refcount during creation and drop it on
>>>> release.
>>>
>>> Do we actually need this structure? At the moment it only keeps
>>> track of
>>> bonds, which we can move to struct dev_iommu. Replacing it by a mm
>>> pointer
>>> in struct iommu_domain simplifies the driver and seems to work
>>
>> Fair enough.
>>
>> +struct iommu_sva_ioas {
>> + struct mm_struct *mm;
>> + ioasid_t pasid;
>> +
>> + /* Counter of domains attached to this ioas. */
>> + refcount_t users;
>> +
>> + /* All bindings are linked here. */
>> + struct list_head bonds;
>> +};
>>
>> By moving @mm to struct iommu_domain and @bonds to struct dev_iommu, the
>> code looks simpler. The mm, sva domain and per-device dev_iommu are
>> guaranteed to be valid during bind() and unbind().
>>
>> Will head this direction in the next version.
>
> I'm trying to implement this idea in real code. It seems that we need
> additional fields in struct iommu_domain to track which devices the mm
> was bound to. It doesn't simplify the code much. Any thoughts?
Sorry, Jean. This has been discussed. We don't need to share sva domain
among devices at this stage. It's not a big issue to sva domain as it's
a dumb domain which has no support for map()/unmap() and the cache
manipulation.
I will still head this direction. Sorry for the noise.
Best regards,
baolu
^ permalink raw reply [flat|nested] 30+ messages in thread
end of thread, other threads:[~2022-05-07 12:40 UTC | newest]
Thread overview: 30+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-05-02 1:48 [PATCH v5 00/12] iommu: SVA and IOPF refactoring Lu Baolu
2022-05-02 1:48 ` [PATCH v5 01/12] dmaengine: idxd: Separate user and kernel pasid enabling Lu Baolu
2022-05-02 1:48 ` [PATCH v5 02/12] iommu: Add pasid_bits field in struct dev_iommu Lu Baolu
2022-05-03 18:02 ` Jean-Philippe Brucker
2022-05-05 6:25 ` Baolu Lu
2022-05-02 1:48 ` [PATCH v5 03/12] iommu: Add attach/detach_dev_pasid domain ops Lu Baolu
2022-05-03 18:07 ` Jean-Philippe Brucker
2022-05-05 6:28 ` Baolu Lu
2022-05-02 1:48 ` [PATCH v5 04/12] iommu/sva: Basic data structures for SVA Lu Baolu
2022-05-03 18:09 ` Jean-Philippe Brucker
2022-05-05 6:42 ` Baolu Lu
2022-05-07 8:32 ` Baolu Lu
2022-05-07 12:39 ` Baolu Lu
2022-05-02 1:48 ` [PATCH v5 05/12] iommu/vt-d: Remove SVM_FLAG_SUPERVISOR_MODE support Lu Baolu
2022-05-02 1:48 ` [PATCH v5 06/12] iommu/vt-d: Add SVA domain support Lu Baolu
2022-05-02 1:48 ` [PATCH v5 07/12] arm-smmu-v3/sva: " Lu Baolu
2022-05-03 18:12 ` Jean-Philippe Brucker
2022-05-05 7:09 ` Baolu Lu
2022-05-02 1:48 ` [PATCH v5 08/12] iommu/sva: Use attach/detach_pasid_dev in SVA interfaces Lu Baolu
2022-05-02 1:48 ` [PATCH v5 09/12] iommu: Remove SVA related callbacks from iommu ops Lu Baolu
2022-05-03 18:14 ` Jean-Philippe Brucker
2022-05-02 1:48 ` [PATCH v5 10/12] iommu: Prepare IOMMU domain for IOPF Lu Baolu
2022-05-03 18:20 ` Jean-Philippe Brucker
2022-05-05 8:31 ` Baolu Lu
2022-05-05 13:38 ` Jean-Philippe Brucker
2022-05-06 5:40 ` Baolu Lu
2022-05-02 1:48 ` [PATCH v5 11/12] iommu: Per-domain I/O page fault handling Lu Baolu
2022-05-03 18:27 ` Jean-Philippe Brucker
2022-05-02 1:48 ` [PATCH v5 12/12] iommu: Rename iommu-sva-lib.{c,h} Lu Baolu
2022-05-03 18:28 ` Jean-Philippe Brucker
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).