[PATCH v2 1/1] iommu/arm-smmu-v3: Set GBPA to abort all transactions

[PATCH v2 1/1] iommu/arm-smmu-v3: Set GBPA to abort all transactions

[Sameer Goel]

Set SMMU_GBPA to abort all incoming translations during the SMMU reset
when SMMUEN==0.

This prevents a race condition where a stray DMA from the crashed primary
kernel can try to access an IOVA address as an invalid PA when SMMU is
disabled during reset in the crash kernel.

Signed-off-by: Sameer Goel <sgoel@codeaurora.org>
---
 drivers/iommu/arm-smmu-v3.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index 1d64710..5fedb8e 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -2395,6 +2395,20 @@ static int arm_smmu_device_reset(struct arm_smmu_device *smmu, bool bypass)
 	if (reg & CR0_SMMUEN)
 		dev_warn(smmu->dev, "SMMU currently enabled! Resetting...\n");
 
+	/*
+	 * Abort all incoming translations. This can happen in a kdump case
+	 * where SMMU is initialized when a prior DMA is pending. Just
+	 * disabling the SMMU in this case might result in writes to invalid
+	 * PAs. Do this only if bypass is not set.
+	 */
+	if(!bypass || disable_bypass) {
+		ret = arm_smmu_update_gbpa(smmu, 1, GBPA_ABORT);
+		if (ret) {
+			dev_err(smmu->dev, "GBPA not responding to update\n");
+			return ret;
+		}
+	}
+
 	ret = arm_smmu_device_disable(smmu);
 	if (ret)
 		return ret;
-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH v2 1/1] iommu/arm-smmu-v3: Set GBPA to abort all transactions

[Will Deacon]

Hi Sameer,

On Mon, Jun 25, 2018 at 08:30:39PM -0600, Sameer Goel wrote:
> Set SMMU_GBPA to abort all incoming translations during the SMMU reset
> when SMMUEN==0.
> 
> This prevents a race condition where a stray DMA from the crashed primary
> kernel can try to access an IOVA address as an invalid PA when SMMU is
> disabled during reset in the crash kernel.
> 
> Signed-off-by: Sameer Goel <sgoel@codeaurora.org>
> ---
>  drivers/iommu/arm-smmu-v3.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
> index 1d64710..5fedb8e 100644
> --- a/drivers/iommu/arm-smmu-v3.c
> +++ b/drivers/iommu/arm-smmu-v3.c
> @@ -2395,6 +2395,20 @@ static int arm_smmu_device_reset(struct arm_smmu_device *smmu, bool bypass)
>  	if (reg & CR0_SMMUEN)
>  		dev_warn(smmu->dev, "SMMU currently enabled! Resetting...\n");
>  
> +	/*
> +	 * Abort all incoming translations. This can happen in a kdump case
> +	 * where SMMU is initialized when a prior DMA is pending. Just
> +	 * disabling the SMMU in this case might result in writes to invalid
> +	 * PAs. Do this only if bypass is not set.
> +	 */
> +	if(!bypass || disable_bypass) {
> +		ret = arm_smmu_update_gbpa(smmu, 1, GBPA_ABORT);

I really don't understand what this is trying to accomplish!

As far as I understood, the problem you're trying to solve is when we
kexec a crash kernel without cleanly shutting down the SMMU. In this case,
we want to avoid resetting the SMMU when we find it enabled, because doing
so would put it into bypass and any ongoing DMA could corrupt memory being
uses by the crash kernel. Is that correct? If so, how about the diff below
instead of this patch?

Cheers,

Will

--->8

diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index 7fb5230cd145..3ce841c5cae3 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -24,6 +24,7 @@
 #include <linux/acpi_iort.h>
 #include <linux/bitfield.h>
 #include <linux/bitops.h>
+#include <linux/crash_dump.h>
 #include <linux/delay.h>
 #include <linux/dma-iommu.h>
 #include <linux/err.h>
@@ -2212,8 +2213,12 @@ static int arm_smmu_update_gbpa(struct arm_smmu_device *smmu, u32 set, u32 clr)
 	reg &= ~clr;
 	reg |= set;
 	writel_relaxed(reg | GBPA_UPDATE, gbpa);
-	return readl_relaxed_poll_timeout(gbpa, reg, !(reg & GBPA_UPDATE),
-					  1, ARM_SMMU_POLL_TIMEOUT_US);
+	ret = readl_relaxed_poll_timeout(gbpa, reg, !(reg & GBPA_UPDATE),
+					 1, ARM_SMMU_POLL_TIMEOUT_US);
+
+	if (ret)
+		dev_err(smmu->dev, "GBPA not responding to update\n");
+	return ret;
 }
 
 static void arm_smmu_free_msis(void *data)
@@ -2393,8 +2398,14 @@ static int arm_smmu_device_reset(struct arm_smmu_device *smmu, bool bypass)
 
 	/* Clear CR0 and sync (disables SMMU and queue processing) */
 	reg = readl_relaxed(smmu->base + ARM_SMMU_CR0);
-	if (reg & CR0_SMMUEN)
+	if (reg & CR0_SMMUEN) {
+		if (is_kdump_kernel()) {
+			arm_smmu_update_gbpa(smmu, GBPA_ABORT, 0);
+			return -EBUSY;
+		}
+
 		dev_warn(smmu->dev, "SMMU currently enabled! Resetting...\n");
+	}
 
 	ret = arm_smmu_device_disable(smmu);
 	if (ret)
@@ -2492,10 +2503,8 @@ static int arm_smmu_device_reset(struct arm_smmu_device *smmu, bool bypass)
 		enables |= CR0_SMMUEN;
 	} else {
 		ret = arm_smmu_update_gbpa(smmu, 0, GBPA_ABORT);
-		if (ret) {
-			dev_err(smmu->dev, "GBPA not responding to update\n");
+		if (ret)
 			return ret;
-		}
 	}
 	ret = arm_smmu_write_reg_sync(smmu, enables, ARM_SMMU_CR0,
 				      ARM_SMMU_CR0ACK);

[PATCH v2 1/1] iommu/arm-smmu-v3: Set GBPA to abort all transactions

[Will Deacon]

On Wed, Jul 25, 2018 at 03:27:37PM +0100, Will Deacon wrote:
> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
> index 7fb5230cd145..3ce841c5cae3 100644
> --- a/drivers/iommu/arm-smmu-v3.c
> +++ b/drivers/iommu/arm-smmu-v3.c
> @@ -24,6 +24,7 @@
>  #include <linux/acpi_iort.h>
>  #include <linux/bitfield.h>
>  #include <linux/bitops.h>
> +#include <linux/crash_dump.h>
>  #include <linux/delay.h>
>  #include <linux/dma-iommu.h>
>  #include <linux/err.h>
> @@ -2212,8 +2213,12 @@ static int arm_smmu_update_gbpa(struct arm_smmu_device *smmu, u32 set, u32 clr)
>  	reg &= ~clr;
>  	reg |= set;
>  	writel_relaxed(reg | GBPA_UPDATE, gbpa);
> -	return readl_relaxed_poll_timeout(gbpa, reg, !(reg & GBPA_UPDATE),
> -					  1, ARM_SMMU_POLL_TIMEOUT_US);
> +	ret = readl_relaxed_poll_timeout(gbpa, reg, !(reg & GBPA_UPDATE),
> +					 1, ARM_SMMU_POLL_TIMEOUT_US);
> +
> +	if (ret)
> +		dev_err(smmu->dev, "GBPA not responding to update\n");
> +	return ret;
>  }
>  
>  static void arm_smmu_free_msis(void *data)
> @@ -2393,8 +2398,14 @@ static int arm_smmu_device_reset(struct arm_smmu_device *smmu, bool bypass)
>  
>  	/* Clear CR0 and sync (disables SMMU and queue processing) */
>  	reg = readl_relaxed(smmu->base + ARM_SMMU_CR0);
> -	if (reg & CR0_SMMUEN)
> +	if (reg & CR0_SMMUEN) {
> +		if (is_kdump_kernel()) {
> +			arm_smmu_update_gbpa(smmu, GBPA_ABORT, 0);

Oops, I forgot to call arm_smmu_device_disable(smmu); here after updating
the GBPA register.

Anyway, I'd be interested in your thoughts on this approach.

Will