iommu/arm-smmu-v3: Set GBPA to abort all transactions
PATCH v2 1/1 iommu/arm-smmu-v3: Set GBPA to abort all transactions
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.
--- drivers/iommu/arm-smmu-v3.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)
drivers/iommu/arm-smmu-v3.c +14 -0
@@ -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;
Signature · 1 line
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
Hi Sameer,
Quoted 18 lines · Sameer Goel
Set
SMMU_GBPAto 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 <[email protected]>--- drivers/iommu/arm-smmu-v3.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)drivers/iommu/arm-smmu-v3.c +0
-0@@ -2395,6 +2395,20 @@ static int arm_smmu_device_reset(struct arm_smmu_device *smmu, bool bypass)if (reg &CR0_SMMUEN )
drivers/iommu/arm-smmu-v3.c static int arm_smmu_device_reset(struct arm_smmu_device *smmu, bool bypass) · line 2405
Quoted 10 lines
drivers/iommu/arm-smmu-v3.c +8
-0dev_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
drivers/iommu/arm-smmu-v3.c +15 -6
@@ -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
Quoted 28 lines · Will Deacon
drivers/iommu/arm-smmu-v3.c +7
-2@@ -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)
drivers/iommu/arm-smmu-v3.c static int arm_smmu_device_reset(struct arm_smmu_device *smmu, bool bypass) · line 2403
Quoted 7 lines
drivers/iommu/arm-smmu-v3.c +3
-1@@ -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