* [PATCH] iommu/arm-smmu-v3: Set GBPA to abort all transactions @ 2018-03-28 14:39 Timur Tabi 2018-03-28 15:00 ` Marc Zyngier 2018-04-05 11:26 ` Will Deacon 0 siblings, 2 replies; 11+ messages in thread From: Timur Tabi @ 2018-03-28 14:39 UTC (permalink / raw) To: linux-arm-kernel From: Sameer Goel <sgoel@codeaurora.org> 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 | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c index 3f2f1fc68b52..c04a89310c59 100644 --- a/drivers/iommu/arm-smmu-v3.c +++ b/drivers/iommu/arm-smmu-v3.c @@ -2458,6 +2458,18 @@ 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. + */ + 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, Inc. 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] 11+ messages in thread
* [PATCH] iommu/arm-smmu-v3: Set GBPA to abort all transactions 2018-03-28 14:39 [PATCH] iommu/arm-smmu-v3: Set GBPA to abort all transactions Timur Tabi @ 2018-03-28 15:00 ` Marc Zyngier 2018-04-11 15:58 ` Goel, Sameer 2018-04-05 11:26 ` Will Deacon 1 sibling, 1 reply; 11+ messages in thread From: Marc Zyngier @ 2018-03-28 15:00 UTC (permalink / raw) To: linux-arm-kernel On 2018-03-28 15:39, Timur Tabi wrote: > From: Sameer Goel <sgoel@codeaurora.org> > > 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 | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/drivers/iommu/arm-smmu-v3.c > b/drivers/iommu/arm-smmu-v3.c > index 3f2f1fc68b52..c04a89310c59 100644 > --- a/drivers/iommu/arm-smmu-v3.c > +++ b/drivers/iommu/arm-smmu-v3.c > @@ -2458,6 +2458,18 @@ 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. > + */ > + 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; A tangential question: can we reliably detect that the SMMU already has valid mappings, which would indicate that we're in a pretty bad shape already by the time we set that bit? For all we know, memory could have been corrupted long before we hit this point, and this patch barely narrows the window of opportunity. At the very least, we should emit a warning and taint the kernel (we've recently added such measures to the GICv3 driver). Thanks, M. -- Fast, cheap, reliable. Pick two. ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH] iommu/arm-smmu-v3: Set GBPA to abort all transactions 2018-03-28 15:00 ` Marc Zyngier @ 2018-04-11 15:58 ` Goel, Sameer 2018-04-11 16:54 ` Marc Zyngier 0 siblings, 1 reply; 11+ messages in thread From: Goel, Sameer @ 2018-04-11 15:58 UTC (permalink / raw) To: linux-arm-kernel On 3/28/2018 9:00 AM, Marc Zyngier wrote: > On 2018-03-28 15:39, Timur Tabi wrote: >> From: Sameer Goel <sgoel@codeaurora.org> >> >> 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 | 12 ++++++++++++ >> ?1 file changed, 12 insertions(+) >> >> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c >> index 3f2f1fc68b52..c04a89310c59 100644 >> --- a/drivers/iommu/arm-smmu-v3.c >> +++ b/drivers/iommu/arm-smmu-v3.c >> @@ -2458,6 +2458,18 @@ 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. >> +???? */ >> +??? 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; > > A tangential question: can we reliably detect that the SMMU already has valid mappings, which would indicate that we're in a pretty bad shape already by the time we set that bit? For all we know, memory could have been corrupted long before we hit this point, and this patch barely narrows the window of opportunity. :) Yes that is correct. This only covers the kdump scenario. Trying to get some reliability when booting up the crash kernel. The system is already in a bad state. I don't think that this will happen in a normal scenario. But please point me to the GICv3 change and I'll have a look. Thanks, Sameer > > At the very least, we should emit a warning and taint the kernel (we've recently added such measures to the GICv3 driver). > > Thanks, > > ??????? M. -- 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 [flat|nested] 11+ messages in thread
* [PATCH] iommu/arm-smmu-v3: Set GBPA to abort all transactions 2018-04-11 15:58 ` Goel, Sameer @ 2018-04-11 16:54 ` Marc Zyngier 2018-04-12 10:17 ` Robin Murphy 0 siblings, 1 reply; 11+ messages in thread From: Marc Zyngier @ 2018-04-11 16:54 UTC (permalink / raw) To: linux-arm-kernel Hi Sammer, On 11/04/18 16:58, Goel, Sameer wrote: > > > On 3/28/2018 9:00 AM, Marc Zyngier wrote: >> On 2018-03-28 15:39, Timur Tabi wrote: >>> From: Sameer Goel <sgoel@codeaurora.org> >>> >>> 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 | 12 ++++++++++++ >>> ?1 file changed, 12 insertions(+) >>> >>> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c >>> index 3f2f1fc68b52..c04a89310c59 100644 >>> --- a/drivers/iommu/arm-smmu-v3.c >>> +++ b/drivers/iommu/arm-smmu-v3.c >>> @@ -2458,6 +2458,18 @@ 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. >>> +???? */ >>> +??? 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; >> >> A tangential question: can we reliably detect that the SMMU already >> has valid mappings, which would indicate that we're in a pretty bad >> shape already by the time we set that bit? For all we know, memory >> could have been corrupted long before we hit this point, and this >> patch barely narrows the window of opportunity. > > :) Yes that is correct. This only covers the kdump scenario. Trying > to get some reliability when booting up the crash kernel. The system > is already in a bad state. I don't think that this will happen in a > normal scenario. But please point me to the GICv3 change and I'll > have a look. See this: https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/tree/drivers/irqchip/irq-gic-v3-its.c?h=irq/irqchip-4.17&id=6eb486b66a3094cdcd68dc39c9df3a29d6a51dd5#n3407 M. -- Jazz is not dead. It just smells funny... ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH] iommu/arm-smmu-v3: Set GBPA to abort all transactions 2018-04-11 16:54 ` Marc Zyngier @ 2018-04-12 10:17 ` Robin Murphy 2018-04-12 10:55 ` Will Deacon 2018-04-12 11:56 ` Marc Zyngier 0 siblings, 2 replies; 11+ messages in thread From: Robin Murphy @ 2018-04-12 10:17 UTC (permalink / raw) To: linux-arm-kernel On 11/04/18 17:54, Marc Zyngier wrote: > Hi Sammer, > > On 11/04/18 16:58, Goel, Sameer wrote: >> >> >> On 3/28/2018 9:00 AM, Marc Zyngier wrote: >>> On 2018-03-28 15:39, Timur Tabi wrote: >>>> From: Sameer Goel <sgoel@codeaurora.org> >>>> >>>> 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 | 12 ++++++++++++ >>>> ?1 file changed, 12 insertions(+) >>>> >>>> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c >>>> index 3f2f1fc68b52..c04a89310c59 100644 >>>> --- a/drivers/iommu/arm-smmu-v3.c >>>> +++ b/drivers/iommu/arm-smmu-v3.c >>>> @@ -2458,6 +2458,18 @@ 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. >>>> +???? */ >>>> +??? 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; >>> >>> A tangential question: can we reliably detect that the SMMU already >>> has valid mappings, which would indicate that we're in a pretty bad >>> shape already by the time we set that bit? For all we know, memory >>> could have been corrupted long before we hit this point, and this >>> patch barely narrows the window of opportunity. >> >> :) Yes that is correct. This only covers the kdump scenario. Trying >> to get some reliability when booting up the crash kernel. The system >> is already in a bad state. I don't think that this will happen in a >> normal scenario. But please point me to the GICv3 change and I'll >> have a look. > > See this: > https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/tree/drivers/irqchip/irq-gic-v3-its.c?h=irq/irqchip-4.17&id=6eb486b66a3094cdcd68dc39c9df3a29d6a51dd5#n3407 The nearest equivalent to that is probably the top-level SMMUEN check that we already have (see the diff context above). To go beyond that you'd have to chase the old stream table pointer and scan the whole thing looking for valid contexts, then potentially walk page tables within those contexts to check for live translations if you really wanted to be sure. That would be a hell of a lot of work to do in the boot path. Robin. ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH] iommu/arm-smmu-v3: Set GBPA to abort all transactions 2018-04-12 10:17 ` Robin Murphy @ 2018-04-12 10:55 ` Will Deacon 2018-04-12 11:56 ` Marc Zyngier 1 sibling, 0 replies; 11+ messages in thread From: Will Deacon @ 2018-04-12 10:55 UTC (permalink / raw) To: linux-arm-kernel On Thu, Apr 12, 2018 at 11:17:24AM +0100, Robin Murphy wrote: > On 11/04/18 17:54, Marc Zyngier wrote: > >On 11/04/18 16:58, Goel, Sameer wrote: > >>On 3/28/2018 9:00 AM, Marc Zyngier wrote: > >>>A tangential question: can we reliably detect that the SMMU already > >>>has valid mappings, which would indicate that we're in a pretty bad > >>>shape already by the time we set that bit? For all we know, memory > >>>could have been corrupted long before we hit this point, and this > >>>patch barely narrows the window of opportunity. > >> > >>:) Yes that is correct. This only covers the kdump scenario. Trying > >>to get some reliability when booting up the crash kernel. The system > >>is already in a bad state. I don't think that this will happen in a > >>normal scenario. But please point me to the GICv3 change and I'll > >>have a look. > > > >See this: > >https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/tree/drivers/irqchip/irq-gic-v3-its.c?h=irq/irqchip-4.17&id=6eb486b66a3094cdcd68dc39c9df3a29d6a51dd5#n3407 > > The nearest equivalent to that is probably the top-level SMMUEN check that > we already have (see the diff context above). To go beyond that you'd have > to chase the old stream table pointer and scan the whole thing looking for > valid contexts, then potentially walk page tables within those contexts to > check for live translations if you really wanted to be sure. That would be a > hell of a lot of work to do in the boot path. Yeah, please don't waste time writing a patch to do that! ;) Will ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH] iommu/arm-smmu-v3: Set GBPA to abort all transactions 2018-04-12 10:17 ` Robin Murphy 2018-04-12 10:55 ` Will Deacon @ 2018-04-12 11:56 ` Marc Zyngier 2018-05-11 16:15 ` Goel, Sameer 2018-05-11 20:52 ` Nate Watterson 1 sibling, 2 replies; 11+ messages in thread From: Marc Zyngier @ 2018-04-12 11:56 UTC (permalink / raw) To: linux-arm-kernel On 12/04/18 11:17, Robin Murphy wrote: > On 11/04/18 17:54, Marc Zyngier wrote: >> Hi Sammer, >> >> On 11/04/18 16:58, Goel, Sameer wrote: >>> >>> >>> On 3/28/2018 9:00 AM, Marc Zyngier wrote: >>>> On 2018-03-28 15:39, Timur Tabi wrote: >>>>> From: Sameer Goel <sgoel@codeaurora.org> >>>>> >>>>> 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 | 12 ++++++++++++ >>>>> ?1 file changed, 12 insertions(+) >>>>> >>>>> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c >>>>> index 3f2f1fc68b52..c04a89310c59 100644 >>>>> --- a/drivers/iommu/arm-smmu-v3.c >>>>> +++ b/drivers/iommu/arm-smmu-v3.c >>>>> @@ -2458,6 +2458,18 @@ 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. >>>>> +???? */ >>>>> +??? 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; >>>> >>>> A tangential question: can we reliably detect that the SMMU already >>>> has valid mappings, which would indicate that we're in a pretty bad >>>> shape already by the time we set that bit? For all we know, memory >>>> could have been corrupted long before we hit this point, and this >>>> patch barely narrows the window of opportunity. >>> >>> :) Yes that is correct. This only covers the kdump scenario. Trying >>> to get some reliability when booting up the crash kernel. The system >>> is already in a bad state. I don't think that this will happen in a >>> normal scenario. But please point me to the GICv3 change and I'll >>> have a look. >> >> See this: >> https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/tree/drivers/irqchip/irq-gic-v3-its.c?h=irq/irqchip-4.17&id=6eb486b66a3094cdcd68dc39c9df3a29d6a51dd5#n3407 > > The nearest equivalent to that is probably the top-level SMMUEN check > that we already have (see the diff context above). To go beyond that > you'd have to chase the old stream table pointer and scan the whole > thing looking for valid contexts, then potentially walk page tables > within those contexts to check for live translations if you really > wanted to be sure. That would be a hell of a lot of work to do in the > boot path. Yeah, feels a bit too involved for sanity. I'd simply suggest you taint the kernel if you find the SMMU enabled, as you're already on shaky ground. Thanks, M. -- Jazz is not dead. It just smells funny... ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH] iommu/arm-smmu-v3: Set GBPA to abort all transactions 2018-04-12 11:56 ` Marc Zyngier @ 2018-05-11 16:15 ` Goel, Sameer 2018-05-11 20:52 ` Nate Watterson 1 sibling, 0 replies; 11+ messages in thread From: Goel, Sameer @ 2018-05-11 16:15 UTC (permalink / raw) To: linux-arm-kernel On 4/12/2018 5:56 AM, Marc Zyngier wrote: > On 12/04/18 11:17, Robin Murphy wrote: >> On 11/04/18 17:54, Marc Zyngier wrote: >>> Hi Sammer, >>> >>> On 11/04/18 16:58, Goel, Sameer wrote: >>>> >>>> >>>> On 3/28/2018 9:00 AM, Marc Zyngier wrote: >>>>> On 2018-03-28 15:39, Timur Tabi wrote: >>>>>> From: Sameer Goel <sgoel@codeaurora.org> >>>>>> >>>>>> 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 | 12 ++++++++++++ >>>>>> ?1 file changed, 12 insertions(+) >>>>>> >>>>>> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c >>>>>> index 3f2f1fc68b52..c04a89310c59 100644 >>>>>> --- a/drivers/iommu/arm-smmu-v3.c >>>>>> +++ b/drivers/iommu/arm-smmu-v3.c >>>>>> @@ -2458,6 +2458,18 @@ 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. >>>>>> +???? */ >>>>>> +??? 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; >>>>> >>>>> A tangential question: can we reliably detect that the SMMU already >>>>> has valid mappings, which would indicate that we're in a pretty bad >>>>> shape already by the time we set that bit? For all we know, memory >>>>> could have been corrupted long before we hit this point, and this >>>>> patch barely narrows the window of opportunity. >>>> >>>> :) Yes that is correct. This only covers the kdump scenario. Trying >>>> to get some reliability when booting up the crash kernel. The system >>>> is already in a bad state. I don't think that this will happen in a >>>> normal scenario. But please point me to the GICv3 change and I'll >>>> have a look. >>> >>> See this: >>> https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/tree/drivers/irqchip/irq-gic-v3-its.c?h=irq/irqchip-4.17&id=6eb486b66a3094cdcd68dc39c9df3a29d6a51dd5#n3407 >> >> The nearest equivalent to that is probably the top-level SMMUEN check >> that we already have (see the diff context above). To go beyond that >> you'd have to chase the old stream table pointer and scan the whole >> thing looking for valid contexts, then potentially walk page tables >> within those contexts to check for live translations if you really >> wanted to be sure. That would be a hell of a lot of work to do in the >> boot path. > Yeah, feels a bit too involved for sanity. I'd simply suggest you taint > the kernel if you find the SMMU enabled, as you're already on shaky ground. Ok. I think since this is a kdump kernel a taint is not necessary? > > Thanks, > > M. > -- 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 [flat|nested] 11+ messages in thread
* [PATCH] iommu/arm-smmu-v3: Set GBPA to abort all transactions 2018-04-12 11:56 ` Marc Zyngier 2018-05-11 16:15 ` Goel, Sameer @ 2018-05-11 20:52 ` Nate Watterson 1 sibling, 0 replies; 11+ messages in thread From: Nate Watterson @ 2018-05-11 20:52 UTC (permalink / raw) To: linux-arm-kernel Hi Mark, On 4/12/2018 7:56 AM, Marc Zyngier wrote: > On 12/04/18 11:17, Robin Murphy wrote: >> On 11/04/18 17:54, Marc Zyngier wrote: >>> Hi Sammer, >>> >>> On 11/04/18 16:58, Goel, Sameer wrote: >>>> >>>> >>>> On 3/28/2018 9:00 AM, Marc Zyngier wrote: >>>>> On 2018-03-28 15:39, Timur Tabi wrote: >>>>>> From: Sameer Goel <sgoel@codeaurora.org> >>>>>> >>>>>> 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 | 12 ++++++++++++ >>>>>> ?1 file changed, 12 insertions(+) >>>>>> >>>>>> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c >>>>>> index 3f2f1fc68b52..c04a89310c59 100644 >>>>>> --- a/drivers/iommu/arm-smmu-v3.c >>>>>> +++ b/drivers/iommu/arm-smmu-v3.c >>>>>> @@ -2458,6 +2458,18 @@ 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. >>>>>> +???? */ >>>>>> +??? 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; >>>>> >>>>> A tangential question: can we reliably detect that the SMMU already >>>>> has valid mappings, which would indicate that we're in a pretty bad >>>>> shape already by the time we set that bit? For all we know, memory >>>>> could have been corrupted long before we hit this point, and this >>>>> patch barely narrows the window of opportunity. >>>> >>>> :) Yes that is correct. This only covers the kdump scenario. Trying >>>> to get some reliability when booting up the crash kernel. The system >>>> is already in a bad state. I don't think that this will happen in a >>>> normal scenario. But please point me to the GICv3 change and I'll >>>> have a look. >>> >>> See this: >>> https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/tree/drivers/irqchip/irq-gic-v3-its.c?h=irq/irqchip-4.17&id=6eb486b66a3094cdcd68dc39c9df3a29d6a51dd5#n3407 >> >> The nearest equivalent to that is probably the top-level SMMUEN check >> that we already have (see the diff context above). To go beyond that >> you'd have to chase the old stream table pointer and scan the whole >> thing looking for valid contexts, then potentially walk page tables >> within those contexts to check for live translations if you really >> wanted to be sure. That would be a hell of a lot of work to do in the >> boot path. > Yeah, feels a bit too involved for sanity. I'd simply suggest you taint > the kernel if you find the SMMU enabled, as you're already on shaky ground. Finding the SMMU already enabled does not necessarily indicate that anything catastrophic has occurred. For instance, to support OSes without an SMMUv3 driver, boot FW may have enabled the SMMU and installed 1-to-1 mappings for DDR and MSI target addr(s) to compensate for a MSI-capable master whose default DMA attrs needed tweaking (ex: non-coherent -> coherent). If such a configuration warrants tainting the kernel, then we should similarly check GBPA for attr overrides and taint the kernel if any are found there. > > Thanks, > > M. > -- 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 [flat|nested] 11+ messages in thread
* [PATCH] iommu/arm-smmu-v3: Set GBPA to abort all transactions 2018-03-28 14:39 [PATCH] iommu/arm-smmu-v3: Set GBPA to abort all transactions Timur Tabi 2018-03-28 15:00 ` Marc Zyngier @ 2018-04-05 11:26 ` Will Deacon 2018-04-11 15:54 ` Goel, Sameer 1 sibling, 1 reply; 11+ messages in thread From: Will Deacon @ 2018-04-05 11:26 UTC (permalink / raw) To: linux-arm-kernel On Wed, Mar 28, 2018 at 09:39:40AM -0500, Timur Tabi wrote: > From: Sameer Goel <sgoel@codeaurora.org> > > 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 | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c > index 3f2f1fc68b52..c04a89310c59 100644 > --- a/drivers/iommu/arm-smmu-v3.c > +++ b/drivers/iommu/arm-smmu-v3.c > @@ -2458,6 +2458,18 @@ 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. > + */ > + ret = arm_smmu_update_gbpa(smmu, 1, GBPA_ABORT); > + if (ret) { > + dev_err(smmu->dev, "GBPA not responding to update\n"); > + return ret; > + } This needs to be predicated on the disable_bypass option, otherwise I think it will cause regressions for systems that rely on passthrough. Will ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH] iommu/arm-smmu-v3: Set GBPA to abort all transactions 2018-04-05 11:26 ` Will Deacon @ 2018-04-11 15:54 ` Goel, Sameer 0 siblings, 0 replies; 11+ messages in thread From: Goel, Sameer @ 2018-04-11 15:54 UTC (permalink / raw) To: linux-arm-kernel On 4/5/2018 5:26 AM, Will Deacon wrote: > On Wed, Mar 28, 2018 at 09:39:40AM -0500, Timur Tabi wrote: >> From: Sameer Goel <sgoel@codeaurora.org> >> >> 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 | 12 ++++++++++++ >> 1 file changed, 12 insertions(+) >> >> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c >> index 3f2f1fc68b52..c04a89310c59 100644 >> --- a/drivers/iommu/arm-smmu-v3.c >> +++ b/drivers/iommu/arm-smmu-v3.c >> @@ -2458,6 +2458,18 @@ 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. >> + */ >> + ret = arm_smmu_update_gbpa(smmu, 1, GBPA_ABORT); >> + if (ret) { >> + dev_err(smmu->dev, "GBPA not responding to update\n"); >> + return ret; >> + } > > This needs to be predicated on the disable_bypass option, otherwise I think > it will cause regressions for systems that rely on passthrough. Ok, I'll make the change. > > Will > -- 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 [flat|nested] 11+ messages in thread
end of thread, other threads:[~2018-05-11 20:52 UTC | newest] Thread overview: 11+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2018-03-28 14:39 [PATCH] iommu/arm-smmu-v3: Set GBPA to abort all transactions Timur Tabi 2018-03-28 15:00 ` Marc Zyngier 2018-04-11 15:58 ` Goel, Sameer 2018-04-11 16:54 ` Marc Zyngier 2018-04-12 10:17 ` Robin Murphy 2018-04-12 10:55 ` Will Deacon 2018-04-12 11:56 ` Marc Zyngier 2018-05-11 16:15 ` Goel, Sameer 2018-05-11 20:52 ` Nate Watterson 2018-04-05 11:26 ` Will Deacon 2018-04-11 15:54 ` Goel, Sameer
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).