Re: [PATCH v10 10/10] iommu/arm-smmu-v3: Add stall support for platform devices
From: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
Date: 2021-01-21 19:15:09
Also in:
linux-acpi, linux-arm-kernel, linux-iommu
On Thu, 21 Jan 2021 13:36:24 +0100 Jean-Philippe Brucker [off-list ref] wrote:
The SMMU provides a Stall model for handling page faults in platform devices. It is similar to PCIe PRI, but doesn't require devices to have their own translation cache. Instead, faulting transactions are parked and the OS is given a chance to fix the page tables and retry the transaction. Enable stall for devices that support it (opt-in by firmware). When an event corresponds to a translation error, call the IOMMU fault handler. If the fault is recoverable, it will call us back to terminate or continue the stall. To use stall device drivers need to enable IOMMU_DEV_FEAT_IOPF, which initializes the fault queue for the device. Tested-by: Zhangfei Gao <zhangfei.gao@linaro.org> Signed-off-by: Jean-Philippe Brucker <redacted>
One thing inline + one comment which was mostly a case of I ran out of time to walk through why probe and release aren't symmetric...
--- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 43 ++++ .../iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c | 59 +++++- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 185 +++++++++++++++++- 3 files changed, 273 insertions(+), 14 deletions(-)
...
quoted hunk ↗ jump to hunk
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 db5d6aa76c3a..af6982aca42e 100644 --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c@@ -32,6 +32,7 @@
...
quoted hunk ↗ jump to hunk
master->domain = smmu_domain;@@ -2484,6 +2624,11 @@ static struct iommu_device *arm_smmu_probe_device(struct device *dev) master->ssid_bits = min_t(u8, master->ssid_bits, CTXDESC_LINEAR_CDMAX); + if ((smmu->features & ARM_SMMU_FEAT_STALLS && + device_property_read_bool(dev, "dma-can-stall")) || + smmu->features & ARM_SMMU_FEAT_STALL_FORCE) + master->stall_enabled = true; + return &smmu->iommu; err_free_master:@@ -2502,6 +2647,7 @@ static void arm_smmu_release_device(struct device *dev) master = dev_iommu_priv_get(dev); WARN_ON(arm_smmu_master_sva_enabled(master)); + iopf_queue_remove_device(master->smmu->evtq.iopf, dev); arm_smmu_detach_dev(master); arm_smmu_disable_pasid(master); arm_smmu_remove_master(master);
The lack of symmetry here bothers me a bit, but it's already true, so I guess this case is fine as well. ...
quoted hunk ↗ jump to hunk
@@ -2785,6 +2946,7 @@ static int arm_smmu_cmdq_init(struct arm_smmu_device *smmu) static int arm_smmu_init_queues(struct arm_smmu_device *smmu) { int ret; + bool sva = smmu->features & ARM_SMMU_FEAT_STALLS;
FEAT_SVA?
quoted hunk ↗ jump to hunk
/* cmdq */ ret = arm_smmu_init_one_queue(smmu, &smmu->cmdq.q, ARM_SMMU_CMDQ_PROD,@@ -2804,6 +2966,12 @@ static int arm_smmu_init_queues(struct arm_smmu_device *smmu) if (ret) return ret; + if (sva && smmu->features & ARM_SMMU_FEAT_STALLS) {
Isn't this checking same thing twice?
quoted hunk ↗ jump to hunk
+ smmu->evtq.iopf = iopf_queue_alloc(dev_name(smmu->dev)); + if (!smmu->evtq.iopf) + return -ENOMEM; + } + /* priq */ if (!(smmu->features & ARM_SMMU_FEAT_PRI)) return 0;@@ -3718,6 +3886,7 @@ static int arm_smmu_device_remove(struct platform_device *pdev) iommu_device_unregister(&smmu->iommu); iommu_device_sysfs_remove(&smmu->iommu); arm_smmu_device_disable(smmu); + iopf_queue_free(smmu->evtq.iopf); return 0; }