* [PATCH v2 1/1] iommu/arm-smmu-v3: Set GBPA to abort all transactions @ 2018-06-26 2:30 Sameer Goel 2018-07-25 14:27 ` Will Deacon 0 siblings, 1 reply; 3+ messages in thread From: Sameer Goel @ 2018-06-26 2:30 UTC (permalink / raw) To: linux-arm-kernel 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. ^ permalink raw reply related [flat|nested] 3+ messages in thread
* [PATCH v2 1/1] iommu/arm-smmu-v3: Set GBPA to abort all transactions 2018-06-26 2:30 [PATCH v2 1/1] iommu/arm-smmu-v3: Set GBPA to abort all transactions Sameer Goel @ 2018-07-25 14:27 ` Will Deacon 2018-07-25 15:11 ` Will Deacon 0 siblings, 1 reply; 3+ messages in thread From: Will Deacon @ 2018-07-25 14:27 UTC (permalink / raw) To: linux-arm-kernel 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); ^ permalink raw reply related [flat|nested] 3+ messages in thread
* [PATCH v2 1/1] iommu/arm-smmu-v3: Set GBPA to abort all transactions 2018-07-25 14:27 ` Will Deacon @ 2018-07-25 15:11 ` Will Deacon 0 siblings, 0 replies; 3+ messages in thread From: Will Deacon @ 2018-07-25 15:11 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2018-07-25 15:11 UTC | newest] Thread overview: 3+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2018-06-26 2:30 [PATCH v2 1/1] iommu/arm-smmu-v3: Set GBPA to abort all transactions Sameer Goel 2018-07-25 14:27 ` Will Deacon 2018-07-25 15:11 ` Will Deacon
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).