On Mon, 25 Jan 2021 12:06:51 +0100
Jean-Philippe Brucker 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
> Signed-off-by: Jean-Philippe Brucker
Hi Jean-Phillipe,
Just one query below. Either fix that or tell me why you don't need it and
then I'm happy. With that resolved
Reviewed-by: Jonathan Cameron
> ---
> 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 bb251cab61f3..ee66d1f4cb81 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
> @@ -435,9 +435,13 @@ bool arm_smmu_sva_supported(struct arm_smmu_device *smmu)
> return true;
> }
>
> -static bool arm_smmu_iopf_supported(struct arm_smmu_master *master)
> +bool arm_smmu_master_iopf_supported(struct arm_smmu_master *master)
> {
> - return false;
> + /* We're not keeping track of SIDs in fault events */
> + if (master->num_streams != 1)
> + return false;
> +
> + return master->stall_enabled;
> }
>
> bool arm_smmu_master_sva_supported(struct arm_smmu_master *master)
> @@ -445,8 +449,8 @@ bool arm_smmu_master_sva_supported(struct arm_smmu_master
> *master)
> if (!(master->smmu->features & ARM_SMMU_FEAT_SVA))
> return false;
>
> - /* SSID and IOPF support are mandatory for the moment */
> - return master->ssid_bits && arm_smmu_iopf_supported(master);
> + /* SSID support is mandatory for the moment */
> + return master->ssid_bits;
> }
>
> bool arm_smmu_master_sva_enabled(struct arm_smmu_master *master)
> @@ -459,13 +463,55 @@ bool arm_smmu_master_sva_enabled(struct arm_smmu_master
> *master)
> return enabled;
> }
>
> +static int arm_smmu_master_sva_enable_iopf(struct arm_smmu_master *master)
> +{
> + int ret;
> + struct device *dev = master->dev;
> +
> + /*
> + * Drivers for devices supporting PRI or stall should enable IOPF first.
> + * Others have device-specific fault handlers and don't need IOPF.
> + */
> + if (!arm_smmu_master_iopf_supported(master))
So if we have master->iopf_enabled and this happens. Then I'm not totally sure
what prevents the disable below running its cleanup on stuff that was never
configured.
> + return 0;
> +
> + if (!master->iopf_enabled)
> + return -EINVAL;
> +
> + ret = iopf_queue_add_device(master->smmu->evtq.iopf, dev);
> + if (ret)
> + return ret;
> +
> + ret = iommu_register_device_fault_handler(dev, iommu_queue_iopf, dev);
> + if (ret) {
> + iopf_queue_remove_device(master->smmu->evtq.iopf, dev);
> + return ret;
> + }
> + return 0;
> +}
> +
> +static void arm_smmu_master_sva_disable_iopf(struct arm_smmu_master *master)
> +{
> + struct device *dev = master->dev;
> +
> + if (!master->iopf_enabled)
> + return;
As above, I think you need a sanity check on
!arm_smmu_master_iopf_supported(master) before clearing the following.
I may well be missing something that stops us getting here though.
Alternative is probably to sanity check iopf_enabled = true is supported
before letting a driver set it.
> +
> + iommu_unregister_device_fault_handler(dev);
> + iopf_queue_remove_device(master->smmu->evtq.iopf, dev);
> +}
> +
> int arm_smmu_master_enable_sva(struct arm_smmu_master *master)
> {
> + int ret;
> +
> mutex_lock(_lock);
> - master->sva_enabled = true;
> + ret = arm_smmu_master_sva_enable_iopf(master);
> + if (!ret)
> + master->sva_enabled = true;
> mutex_unlock(_lock);
>
> - return 0;
> + return ret;
> }
>
> int arm_smmu_master_disable_sva(struct arm_smmu_master *master)
> @@ -476,6 +522,7 @@ int arm_smmu_master_disable_sva(struct arm_smmu_master
> *master)
> mutex_unlock(_lock);
> return -EBUSY;
> }
> + arm_smmu_master_sva_disable_iopf(master);
> master->sva_enabled = false;
> mutex_unlock(_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 3afec6ed8075..39a59d297c3c 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 @@
> #include
>
> #include
