* [PATCH] iommu/arm-smmu: don't touch the secure STLBIALL register @ 2014-12-23 17:39 Mitchel Humpherys 2015-01-06 14:15 ` Will Deacon 0 siblings, 1 reply; 11+ messages in thread From: Mitchel Humpherys @ 2014-12-23 17:39 UTC (permalink / raw) To: linux-arm-kernel Currently we do a STLBIALL when we initialize the SMMU. However, in some configurations that register is not supposed to be touched and is marked as "Secure only" in the spec. Rip it out. Signed-off-by: Mitchel Humpherys <mitchelh@codeaurora.org> --- drivers/iommu/arm-smmu.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c index 60558f794922..9170bbced5e5 100644 --- a/drivers/iommu/arm-smmu.c +++ b/drivers/iommu/arm-smmu.c @@ -173,7 +173,6 @@ #define PIDR2_ARCH_MASK 0xf /* Global TLB invalidation */ -#define ARM_SMMU_GR0_STLBIALL 0x60 #define ARM_SMMU_GR0_TLBIVMID 0x64 #define ARM_SMMU_GR0_TLBIALLNSNH 0x68 #define ARM_SMMU_GR0_TLBIALLH 0x6c @@ -1686,7 +1685,6 @@ static void arm_smmu_device_reset(struct arm_smmu_device *smmu) } /* Invalidate the TLB, just in case */ - writel_relaxed(0, gr0_base + ARM_SMMU_GR0_STLBIALL); writel_relaxed(0, gr0_base + ARM_SMMU_GR0_TLBIALLH); writel_relaxed(0, gr0_base + ARM_SMMU_GR0_TLBIALLNSNH); -- Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, 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: don't touch the secure STLBIALL register 2014-12-23 17:39 [PATCH] iommu/arm-smmu: don't touch the secure STLBIALL register Mitchel Humpherys @ 2015-01-06 14:15 ` Will Deacon 2015-01-06 20:16 ` Mitchel Humpherys 0 siblings, 1 reply; 11+ messages in thread From: Will Deacon @ 2015-01-06 14:15 UTC (permalink / raw) To: linux-arm-kernel Hi Mitch, On Tue, Dec 23, 2014 at 05:39:22PM +0000, Mitchel Humpherys wrote: > Currently we do a STLBIALL when we initialize the SMMU. However, in > some configurations that register is not supposed to be touched and is > marked as "Secure only" in the spec. Rip it out. > > Signed-off-by: Mitchel Humpherys <mitchelh@codeaurora.org> > --- > drivers/iommu/arm-smmu.c | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c > index 60558f794922..9170bbced5e5 100644 > --- a/drivers/iommu/arm-smmu.c > +++ b/drivers/iommu/arm-smmu.c > @@ -173,7 +173,6 @@ > #define PIDR2_ARCH_MASK 0xf > > /* Global TLB invalidation */ > -#define ARM_SMMU_GR0_STLBIALL 0x60 > #define ARM_SMMU_GR0_TLBIVMID 0x64 > #define ARM_SMMU_GR0_TLBIALLNSNH 0x68 > #define ARM_SMMU_GR0_TLBIALLH 0x6c > @@ -1686,7 +1685,6 @@ static void arm_smmu_device_reset(struct arm_smmu_device *smmu) > } > > /* Invalidate the TLB, just in case */ > - writel_relaxed(0, gr0_base + ARM_SMMU_GR0_STLBIALL); > writel_relaxed(0, gr0_base + ARM_SMMU_GR0_TLBIALLH); > writel_relaxed(0, gr0_base + ARM_SMMU_GR0_TLBIALLNSNH); I was slightly worried that this would break the Calxeda implementation with ARM_SMMU_OPT_SECURE_CFG_ACCESS, but actually these registers aren't even aliased there so I think there's a bigger bug for them. Anyway, given that their hardware has gone the way of the dodo, I'll take the patch as-is unless you have any further comments? Will ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH] iommu/arm-smmu: don't touch the secure STLBIALL register 2015-01-06 14:15 ` Will Deacon @ 2015-01-06 20:16 ` Mitchel Humpherys 2015-01-06 22:35 ` Rob Herring 0 siblings, 1 reply; 11+ messages in thread From: Mitchel Humpherys @ 2015-01-06 20:16 UTC (permalink / raw) To: linux-arm-kernel On Tue, Jan 06 2015 at 06:15:07 AM, Will Deacon <will.deacon@arm.com> wrote: >> /* Invalidate the TLB, just in case */ >> - writel_relaxed(0, gr0_base + ARM_SMMU_GR0_STLBIALL); >> writel_relaxed(0, gr0_base + ARM_SMMU_GR0_TLBIALLH); >> writel_relaxed(0, gr0_base + ARM_SMMU_GR0_TLBIALLNSNH); > > I was slightly worried that this would break the Calxeda implementation > with ARM_SMMU_OPT_SECURE_CFG_ACCESS, but actually these registers aren't > even aliased there so I think there's a bigger bug for them. > > Anyway, given that their hardware has gone the way of the dodo, I'll take > the patch as-is unless you have any further comments? > > Will Yeah I agree that this shouldn't affect the (now defunct) Calxeda implementation. I've tested this on some hardware here and we crash when we touch that register since it's secure-only (not banked, as you mentioned). -Mitch -- Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, 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: don't touch the secure STLBIALL register 2015-01-06 20:16 ` Mitchel Humpherys @ 2015-01-06 22:35 ` Rob Herring 2015-01-06 23:30 ` Mitchel Humpherys 0 siblings, 1 reply; 11+ messages in thread From: Rob Herring @ 2015-01-06 22:35 UTC (permalink / raw) To: linux-arm-kernel On Tue, Jan 6, 2015 at 2:16 PM, Mitchel Humpherys <mitchelh@codeaurora.org> wrote: > On Tue, Jan 06 2015 at 06:15:07 AM, Will Deacon <will.deacon@arm.com> wrote: >>> /* Invalidate the TLB, just in case */ >>> - writel_relaxed(0, gr0_base + ARM_SMMU_GR0_STLBIALL); >>> writel_relaxed(0, gr0_base + ARM_SMMU_GR0_TLBIALLH); >>> writel_relaxed(0, gr0_base + ARM_SMMU_GR0_TLBIALLNSNH); >> >> I was slightly worried that this would break the Calxeda implementation >> with ARM_SMMU_OPT_SECURE_CFG_ACCESS, but actually these registers aren't >> even aliased there so I think there's a bigger bug for them. >> >> Anyway, given that their hardware has gone the way of the dodo, I'll take >> the patch as-is unless you have any further comments? >> >> Will > > Yeah I agree that this shouldn't affect the (now defunct) Calxeda > implementation. I've tested this on some hardware here and we crash > when we touch that register since it's secure-only (not banked, as you > mentioned). It's not quite dead: http://www.eweek.com/servers/calxedas-arm-based-server-chips-re-emerge-with-new-company.html But AFAIK, production systems don't enable the SMMU, but someone could still want to at some point. A note in the commit log here would be nice so it gets recorded. Rob ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH] iommu/arm-smmu: don't touch the secure STLBIALL register 2015-01-06 22:35 ` Rob Herring @ 2015-01-06 23:30 ` Mitchel Humpherys 2015-01-07 10:13 ` Will Deacon 0 siblings, 1 reply; 11+ messages in thread From: Mitchel Humpherys @ 2015-01-06 23:30 UTC (permalink / raw) To: linux-arm-kernel On Tue, Jan 06 2015 at 02:35:28 PM, Rob Herring <robherring2@gmail.com> wrote: > On Tue, Jan 6, 2015 at 2:16 PM, Mitchel Humpherys > <mitchelh@codeaurora.org> wrote: >> On Tue, Jan 06 2015 at 06:15:07 AM, Will Deacon <will.deacon@arm.com> wrote: >>>> /* Invalidate the TLB, just in case */ >>>> - writel_relaxed(0, gr0_base + ARM_SMMU_GR0_STLBIALL); >>>> writel_relaxed(0, gr0_base + ARM_SMMU_GR0_TLBIALLH); >>>> writel_relaxed(0, gr0_base + ARM_SMMU_GR0_TLBIALLNSNH); >>> >>> I was slightly worried that this would break the Calxeda implementation >>> with ARM_SMMU_OPT_SECURE_CFG_ACCESS, but actually these registers aren't >>> even aliased there so I think there's a bigger bug for them. >>> >>> Anyway, given that their hardware has gone the way of the dodo, I'll take >>> the patch as-is unless you have any further comments? >>> >>> Will >> >> Yeah I agree that this shouldn't affect the (now defunct) Calxeda >> implementation. I've tested this on some hardware here and we crash >> when we touch that register since it's secure-only (not banked, as you >> mentioned). > > It's not quite dead: > > http://www.eweek.com/servers/calxedas-arm-based-server-chips-re-emerge-with-new-company.html > > But AFAIK, production systems don't enable the SMMU, but someone could > still want to at some point. A note in the commit log here would be > nice so it gets recorded. Actually, as Will mentioned this shouldn't affect Calxeda since this isn't a banked register. I think the confusion is from the `S' prefix in the spec. The /s/ (lower-case, italic) prefix means that there are secure and non-secure versions of the register, while the S (upper-case, non-italic) prefix means "this is a secure register" (which may or may not have a banked non-secure counterpart). This particular register is an S-only register (there's no non-secure counterpart) so the Calxeda workaround isn't relevant here, AFAICT. -Mitch -- Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, 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: don't touch the secure STLBIALL register 2015-01-06 23:30 ` Mitchel Humpherys @ 2015-01-07 10:13 ` Will Deacon 2015-01-07 17:52 ` Mitchel Humpherys 0 siblings, 1 reply; 11+ messages in thread From: Will Deacon @ 2015-01-07 10:13 UTC (permalink / raw) To: linux-arm-kernel On Tue, Jan 06, 2015 at 11:30:49PM +0000, Mitchel Humpherys wrote: > On Tue, Jan 06 2015 at 02:35:28 PM, Rob Herring <robherring2@gmail.com> wrote: > > On Tue, Jan 6, 2015 at 2:16 PM, Mitchel Humpherys > > <mitchelh@codeaurora.org> wrote: > >> On Tue, Jan 06 2015 at 06:15:07 AM, Will Deacon <will.deacon@arm.com> wrote: > >>>> /* Invalidate the TLB, just in case */ > >>>> - writel_relaxed(0, gr0_base + ARM_SMMU_GR0_STLBIALL); > >>>> writel_relaxed(0, gr0_base + ARM_SMMU_GR0_TLBIALLH); > >>>> writel_relaxed(0, gr0_base + ARM_SMMU_GR0_TLBIALLNSNH); > >>> > >>> I was slightly worried that this would break the Calxeda implementation > >>> with ARM_SMMU_OPT_SECURE_CFG_ACCESS, but actually these registers aren't > >>> even aliased there so I think there's a bigger bug for them. > >>> > >>> Anyway, given that their hardware has gone the way of the dodo, I'll take > >>> the patch as-is unless you have any further comments? > >>> > >>> Will > >> > >> Yeah I agree that this shouldn't affect the (now defunct) Calxeda > >> implementation. I've tested this on some hardware here and we crash > >> when we touch that register since it's secure-only (not banked, as you > >> mentioned). > > > > It's not quite dead: > > > > http://www.eweek.com/servers/calxedas-arm-based-server-chips-re-emerge-with-new-company.html > > > > But AFAIK, production systems don't enable the SMMU, but someone could > > still want to at some point. A note in the commit log here would be > > nice so it gets recorded. > > Actually, as Will mentioned this shouldn't affect Calxeda since this > isn't a banked register. I think the confusion is from the `S' prefix > in the spec. The /s/ (lower-case, italic) prefix means that there are > secure and non-secure versions of the register, while the S (upper-case, > non-italic) prefix means "this is a secure register" (which may or may > not have a banked non-secure counterpart). This particular register is > an S-only register (there's no non-secure counterpart) so the Calxeda > workaround isn't relevant here, AFAICT. Right, but I think the problem is that we go and write zero to ARM_SMMU_GR0_TLBIALLH and ARM_SMMU_GR0_TLBIALLNSNH at what *would be* their non-secure aliases for the secure side (i.e. + 0x400). If would be better to check for the ARM_SMMU_OPT_SECURE_CFG_ACCESS feature and, if it's set then zero ARM_SMMU_GR0_STLBIALL at the correct address otherwise do the ARM_SMMU_GR0_TLBIALLH and ARM_SMMU_GR0_TLBIALLNSNH. Will ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH] iommu/arm-smmu: don't touch the secure STLBIALL register 2015-01-07 10:13 ` Will Deacon @ 2015-01-07 17:52 ` Mitchel Humpherys 2015-01-07 18:04 ` Will Deacon 0 siblings, 1 reply; 11+ messages in thread From: Mitchel Humpherys @ 2015-01-07 17:52 UTC (permalink / raw) To: linux-arm-kernel On Wed, Jan 07 2015 at 02:13:00 AM, Will Deacon <will.deacon@arm.com> wrote: > On Tue, Jan 06, 2015 at 11:30:49PM +0000, Mitchel Humpherys wrote: >> On Tue, Jan 06 2015 at 02:35:28 PM, Rob Herring <robherring2@gmail.com> wrote: >> > On Tue, Jan 6, 2015 at 2:16 PM, Mitchel Humpherys >> > <mitchelh@codeaurora.org> wrote: >> >> On Tue, Jan 06 2015 at 06:15:07 AM, Will Deacon <will.deacon@arm.com> wrote: >> >>>> /* Invalidate the TLB, just in case */ >> >>>> - writel_relaxed(0, gr0_base + ARM_SMMU_GR0_STLBIALL); >> >>>> writel_relaxed(0, gr0_base + ARM_SMMU_GR0_TLBIALLH); >> >>>> writel_relaxed(0, gr0_base + ARM_SMMU_GR0_TLBIALLNSNH); >> >>> >> >>> I was slightly worried that this would break the Calxeda implementation >> >>> with ARM_SMMU_OPT_SECURE_CFG_ACCESS, but actually these registers aren't >> >>> even aliased there so I think there's a bigger bug for them. >> >>> >> >>> Anyway, given that their hardware has gone the way of the dodo, I'll take >> >>> the patch as-is unless you have any further comments? >> >>> >> >>> Will >> >> >> >> Yeah I agree that this shouldn't affect the (now defunct) Calxeda >> >> implementation. I've tested this on some hardware here and we crash >> >> when we touch that register since it's secure-only (not banked, as you >> >> mentioned). >> > >> > It's not quite dead: >> > >> > http://www.eweek.com/servers/calxedas-arm-based-server-chips-re-emerge-with-new-company.html >> > >> > But AFAIK, production systems don't enable the SMMU, but someone could >> > still want to at some point. A note in the commit log here would be >> > nice so it gets recorded. >> >> Actually, as Will mentioned this shouldn't affect Calxeda since this >> isn't a banked register. I think the confusion is from the `S' prefix >> in the spec. The /s/ (lower-case, italic) prefix means that there are >> secure and non-secure versions of the register, while the S (upper-case, >> non-italic) prefix means "this is a secure register" (which may or may >> not have a banked non-secure counterpart). This particular register is >> an S-only register (there's no non-secure counterpart) so the Calxeda >> workaround isn't relevant here, AFAICT. > > Right, but I think the problem is that we go and write zero to > ARM_SMMU_GR0_TLBIALLH and ARM_SMMU_GR0_TLBIALLNSNH at what *would be* their > non-secure aliases for the secure side (i.e. + 0x400). This sounds like a separate problem. Since these GR0 registers aren't banked the calxeda workaround doesn't work... SMMU_STLBIALL, on the other hand, is not only not banked but it's also "secure only" so I don't think we have any business touching it ever. > If would be better to check for the ARM_SMMU_OPT_SECURE_CFG_ACCESS feature > and, if it's set then zero ARM_SMMU_GR0_STLBIALL at the correct address > otherwise do the ARM_SMMU_GR0_TLBIALLH and ARM_SMMU_GR0_TLBIALLNSNH. I'm confused. The problem I'm addressing here is that we're touching a register that's marked as "secure only", which causes our system to crash. Why would we ever want to touch a secure only register, calxeda workaround or not? -Mitch -- Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, 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: don't touch the secure STLBIALL register 2015-01-07 17:52 ` Mitchel Humpherys @ 2015-01-07 18:04 ` Will Deacon 2015-01-07 18:35 ` Mitchel Humpherys 0 siblings, 1 reply; 11+ messages in thread From: Will Deacon @ 2015-01-07 18:04 UTC (permalink / raw) To: linux-arm-kernel On Wed, Jan 07, 2015 at 05:52:46PM +0000, Mitchel Humpherys wrote: > On Wed, Jan 07 2015 at 02:13:00 AM, Will Deacon <will.deacon@arm.com> wrote: > > On Tue, Jan 06, 2015 at 11:30:49PM +0000, Mitchel Humpherys wrote: > >> On Tue, Jan 06 2015 at 02:35:28 PM, Rob Herring <robherring2@gmail.com> wrote: > >> > On Tue, Jan 6, 2015 at 2:16 PM, Mitchel Humpherys > >> > <mitchelh@codeaurora.org> wrote: > >> >> On Tue, Jan 06 2015 at 06:15:07 AM, Will Deacon <will.deacon@arm.com> wrote: > >> >>>> /* Invalidate the TLB, just in case */ > >> >>>> - writel_relaxed(0, gr0_base + ARM_SMMU_GR0_STLBIALL); > >> >>>> writel_relaxed(0, gr0_base + ARM_SMMU_GR0_TLBIALLH); > >> >>>> writel_relaxed(0, gr0_base + ARM_SMMU_GR0_TLBIALLNSNH); > >> >>> > >> >>> I was slightly worried that this would break the Calxeda implementation > >> >>> with ARM_SMMU_OPT_SECURE_CFG_ACCESS, but actually these registers aren't > >> >>> even aliased there so I think there's a bigger bug for them. > >> >>> > >> >>> Anyway, given that their hardware has gone the way of the dodo, I'll take > >> >>> the patch as-is unless you have any further comments? > >> >>> > >> >>> Will > >> >> > >> >> Yeah I agree that this shouldn't affect the (now defunct) Calxeda > >> >> implementation. I've tested this on some hardware here and we crash > >> >> when we touch that register since it's secure-only (not banked, as you > >> >> mentioned). > >> > > >> > It's not quite dead: > >> > > >> > http://www.eweek.com/servers/calxedas-arm-based-server-chips-re-emerge-with-new-company.html > >> > > >> > But AFAIK, production systems don't enable the SMMU, but someone could > >> > still want to at some point. A note in the commit log here would be > >> > nice so it gets recorded. > >> > >> Actually, as Will mentioned this shouldn't affect Calxeda since this > >> isn't a banked register. I think the confusion is from the `S' prefix > >> in the spec. The /s/ (lower-case, italic) prefix means that there are > >> secure and non-secure versions of the register, while the S (upper-case, > >> non-italic) prefix means "this is a secure register" (which may or may > >> not have a banked non-secure counterpart). This particular register is > >> an S-only register (there's no non-secure counterpart) so the Calxeda > >> workaround isn't relevant here, AFAICT. > > > > Right, but I think the problem is that we go and write zero to > > ARM_SMMU_GR0_TLBIALLH and ARM_SMMU_GR0_TLBIALLNSNH at what *would be* their > > non-secure aliases for the secure side (i.e. + 0x400). > > This sounds like a separate problem. Since these GR0 registers aren't > banked the calxeda workaround doesn't work... SMMU_STLBIALL, on the > other hand, is not only not banked but it's also "secure only" so I > don't think we have any business touching it ever. > > > If would be better to check for the ARM_SMMU_OPT_SECURE_CFG_ACCESS feature > > and, if it's set then zero ARM_SMMU_GR0_STLBIALL at the correct address > > otherwise do the ARM_SMMU_GR0_TLBIALLH and ARM_SMMU_GR0_TLBIALLNSNH. > > I'm confused. The problem I'm addressing here is that we're touching a > register that's marked as "secure only", which causes our system to > crash. Why would we ever want to touch a secure only register, calxeda > workaround or not? Because I think the way the SMMU is wired for Calxeda is that the CPU can only see the secure side of the register interface, so the only way to nuke the whole TLB would be to use ARM_SMMU_GR0_STLBIALL. Will ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH] iommu/arm-smmu: don't touch the secure STLBIALL register 2015-01-07 18:04 ` Will Deacon @ 2015-01-07 18:35 ` Mitchel Humpherys 2015-01-07 18:53 ` Will Deacon 0 siblings, 1 reply; 11+ messages in thread From: Mitchel Humpherys @ 2015-01-07 18:35 UTC (permalink / raw) To: linux-arm-kernel On Wed, Jan 07 2015 at 10:04:20 AM, Will Deacon <will.deacon@arm.com> wrote: > On Wed, Jan 07, 2015 at 05:52:46PM +0000, Mitchel Humpherys wrote: >> On Wed, Jan 07 2015 at 02:13:00 AM, Will Deacon <will.deacon@arm.com> wrote: >> > On Tue, Jan 06, 2015 at 11:30:49PM +0000, Mitchel Humpherys wrote: >> >> On Tue, Jan 06 2015 at 02:35:28 PM, Rob Herring <robherring2@gmail.com> wrote: >> >> > On Tue, Jan 6, 2015 at 2:16 PM, Mitchel Humpherys >> >> > <mitchelh@codeaurora.org> wrote: >> >> >> On Tue, Jan 06 2015 at 06:15:07 AM, Will Deacon <will.deacon@arm.com> wrote: >> >> >>>> /* Invalidate the TLB, just in case */ >> >> >>>> - writel_relaxed(0, gr0_base + ARM_SMMU_GR0_STLBIALL); >> >> >>>> writel_relaxed(0, gr0_base + ARM_SMMU_GR0_TLBIALLH); >> >> >>>> writel_relaxed(0, gr0_base + ARM_SMMU_GR0_TLBIALLNSNH); >> >> >>> >> >> >>> I was slightly worried that this would break the Calxeda implementation >> >> >>> with ARM_SMMU_OPT_SECURE_CFG_ACCESS, but actually these registers aren't >> >> >>> even aliased there so I think there's a bigger bug for them. >> >> >>> >> >> >>> Anyway, given that their hardware has gone the way of the dodo, I'll take >> >> >>> the patch as-is unless you have any further comments? >> >> >>> >> >> >>> Will >> >> >> >> >> >> Yeah I agree that this shouldn't affect the (now defunct) Calxeda >> >> >> implementation. I've tested this on some hardware here and we crash >> >> >> when we touch that register since it's secure-only (not banked, as you >> >> >> mentioned). >> >> > >> >> > It's not quite dead: >> >> > >> >> > http://www.eweek.com/servers/calxedas-arm-based-server-chips-re-emerge-with-new-company.html >> >> > >> >> > But AFAIK, production systems don't enable the SMMU, but someone could >> >> > still want to at some point. A note in the commit log here would be >> >> > nice so it gets recorded. >> >> >> >> Actually, as Will mentioned this shouldn't affect Calxeda since this >> >> isn't a banked register. I think the confusion is from the `S' prefix >> >> in the spec. The /s/ (lower-case, italic) prefix means that there are >> >> secure and non-secure versions of the register, while the S (upper-case, >> >> non-italic) prefix means "this is a secure register" (which may or may >> >> not have a banked non-secure counterpart). This particular register is >> >> an S-only register (there's no non-secure counterpart) so the Calxeda >> >> workaround isn't relevant here, AFAICT. >> > >> > Right, but I think the problem is that we go and write zero to >> > ARM_SMMU_GR0_TLBIALLH and ARM_SMMU_GR0_TLBIALLNSNH at what *would be* their >> > non-secure aliases for the secure side (i.e. + 0x400). >> >> This sounds like a separate problem. Since these GR0 registers aren't >> banked the calxeda workaround doesn't work... SMMU_STLBIALL, on the >> other hand, is not only not banked but it's also "secure only" so I >> don't think we have any business touching it ever. >> >> > If would be better to check for the ARM_SMMU_OPT_SECURE_CFG_ACCESS feature >> > and, if it's set then zero ARM_SMMU_GR0_STLBIALL at the correct address >> > otherwise do the ARM_SMMU_GR0_TLBIALLH and ARM_SMMU_GR0_TLBIALLNSNH. >> >> I'm confused. The problem I'm addressing here is that we're touching a >> register that's marked as "secure only", which causes our system to >> crash. Why would we ever want to touch a secure only register, calxeda >> workaround or not? > > Because I think the way the SMMU is wired for Calxeda is that the CPU can > only see the secure side of the register interface, so the only way to nuke > the whole TLB would be to use ARM_SMMU_GR0_STLBIALL. Still not sure I understand what "the correct address" is for STLBIALL on Calxeda (i.e. whether or not we need to use ARM_SMMU_GR0_NS), but something like: -- >8 -- Subject: [PATCH v2] iommu/arm-smmu: don't touch the secure STLBIALL register Currently we do a STLBIALL when we initialize the SMMU. However, on systems with sane secure configurations (i.e. !ARM_SMMU_OPT_SECURE_CFG_ACCESS) that register is not supposed to be touched and is marked as "Secure only" in the spec. Touching it results in a crash on those platforms. However, on platforms with ARM_SMMU_OPT_SECURE_CFG_ACCESS it's the only way to nuke the whole TLB, so leave it in for them but rip it out for everyone else. Signed-off-by: Mitchel Humpherys <mitchelh@codeaurora.org> --- drivers/iommu/arm-smmu.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c index 60558f794922..d4c149d83f3d 100644 --- a/drivers/iommu/arm-smmu.c +++ b/drivers/iommu/arm-smmu.c @@ -1686,9 +1686,12 @@ static void arm_smmu_device_reset(struct arm_smmu_device *smmu) } /* Invalidate the TLB, just in case */ - writel_relaxed(0, gr0_base + ARM_SMMU_GR0_STLBIALL); - writel_relaxed(0, gr0_base + ARM_SMMU_GR0_TLBIALLH); - writel_relaxed(0, gr0_base + ARM_SMMU_GR0_TLBIALLNSNH); + if (smmu->options & ARM_SMMU_OPT_SECURE_CFG_ACCESS) { + writel_relaxed(0, gr0_base + ARM_SMMU_GR0_STLBIALL); + } else { + writel_relaxed(0, gr0_base + ARM_SMMU_GR0_TLBIALLH); + writel_relaxed(0, gr0_base + ARM_SMMU_GR0_TLBIALLNSNH); + } reg = readl_relaxed(ARM_SMMU_GR0_NS(smmu) + ARM_SMMU_GR0_sCR0); -- Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, 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: don't touch the secure STLBIALL register 2015-01-07 18:35 ` Mitchel Humpherys @ 2015-01-07 18:53 ` Will Deacon 2015-01-08 20:58 ` Rob Herring 0 siblings, 1 reply; 11+ messages in thread From: Will Deacon @ 2015-01-07 18:53 UTC (permalink / raw) To: linux-arm-kernel On Wed, Jan 07, 2015 at 06:35:41PM +0000, Mitchel Humpherys wrote: > On Wed, Jan 07 2015 at 10:04:20 AM, Will Deacon <will.deacon@arm.com> wrote: > > On Wed, Jan 07, 2015 at 05:52:46PM +0000, Mitchel Humpherys wrote: > >> On Wed, Jan 07 2015 at 02:13:00 AM, Will Deacon <will.deacon@arm.com> wrote: > >> > If would be better to check for the ARM_SMMU_OPT_SECURE_CFG_ACCESS feature > >> > and, if it's set then zero ARM_SMMU_GR0_STLBIALL at the correct address > >> > otherwise do the ARM_SMMU_GR0_TLBIALLH and ARM_SMMU_GR0_TLBIALLNSNH. > >> > >> I'm confused. The problem I'm addressing here is that we're touching a > >> register that's marked as "secure only", which causes our system to > >> crash. Why would we ever want to touch a secure only register, calxeda > >> workaround or not? > > > > Because I think the way the SMMU is wired for Calxeda is that the CPU can > > only see the secure side of the register interface, so the only way to nuke > > the whole TLB would be to use ARM_SMMU_GR0_STLBIALL. > > Still not sure I understand what "the correct address" is for STLBIALL > on Calxeda (i.e. whether or not we need to use ARM_SMMU_GR0_NS), but > something like: Hehe, I wasn't actually expecting a patch, but thanks! > -- >8 -- > Subject: [PATCH v2] iommu/arm-smmu: don't touch the secure STLBIALL register > > Currently we do a STLBIALL when we initialize the SMMU. However, on > systems with sane secure > configurations (i.e. !ARM_SMMU_OPT_SECURE_CFG_ACCESS) that register is > not supposed to be touched and is marked as "Secure only" in the spec. > Touching it results in a crash on those platforms. However, on > platforms with ARM_SMMU_OPT_SECURE_CFG_ACCESS it's the only way to nuke > the whole TLB, so leave it in for them but rip it out for everyone else. > > Signed-off-by: Mitchel Humpherys <mitchelh@codeaurora.org> > --- > drivers/iommu/arm-smmu.c | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c > index 60558f794922..d4c149d83f3d 100644 > --- a/drivers/iommu/arm-smmu.c > +++ b/drivers/iommu/arm-smmu.c > @@ -1686,9 +1686,12 @@ static void arm_smmu_device_reset(struct arm_smmu_device *smmu) > } > > /* Invalidate the TLB, just in case */ > - writel_relaxed(0, gr0_base + ARM_SMMU_GR0_STLBIALL); > - writel_relaxed(0, gr0_base + ARM_SMMU_GR0_TLBIALLH); > - writel_relaxed(0, gr0_base + ARM_SMMU_GR0_TLBIALLNSNH); > + if (smmu->options & ARM_SMMU_OPT_SECURE_CFG_ACCESS) { > + writel_relaxed(0, gr0_base + ARM_SMMU_GR0_STLBIALL); Right, so this is the bit where we'd need some Calxeda information about whether or not to subtract 0x400 from gr0_base or not. > + } else { > + writel_relaxed(0, gr0_base + ARM_SMMU_GR0_TLBIALLH); > + writel_relaxed(0, gr0_base + ARM_SMMU_GR0_TLBIALLNSNH); > + } For now, I've applied your original patch pending any insight on the above. Cheers, Will ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH] iommu/arm-smmu: don't touch the secure STLBIALL register 2015-01-07 18:53 ` Will Deacon @ 2015-01-08 20:58 ` Rob Herring 0 siblings, 0 replies; 11+ messages in thread From: Rob Herring @ 2015-01-08 20:58 UTC (permalink / raw) To: linux-arm-kernel +Andreas On Wed, Jan 7, 2015 at 12:53 PM, Will Deacon <will.deacon@arm.com> wrote: > On Wed, Jan 07, 2015 at 06:35:41PM +0000, Mitchel Humpherys wrote: >> On Wed, Jan 07 2015 at 10:04:20 AM, Will Deacon <will.deacon@arm.com> wrote: >> > On Wed, Jan 07, 2015 at 05:52:46PM +0000, Mitchel Humpherys wrote: >> >> On Wed, Jan 07 2015 at 02:13:00 AM, Will Deacon <will.deacon@arm.com> wrote: >> >> > If would be better to check for the ARM_SMMU_OPT_SECURE_CFG_ACCESS feature >> >> > and, if it's set then zero ARM_SMMU_GR0_STLBIALL at the correct address >> >> > otherwise do the ARM_SMMU_GR0_TLBIALLH and ARM_SMMU_GR0_TLBIALLNSNH. >> >> >> >> I'm confused. The problem I'm addressing here is that we're touching a >> >> register that's marked as "secure only", which causes our system to >> >> crash. Why would we ever want to touch a secure only register, calxeda >> >> workaround or not? >> > >> > Because I think the way the SMMU is wired for Calxeda is that the CPU can >> > only see the secure side of the register interface, so the only way to nuke >> > the whole TLB would be to use ARM_SMMU_GR0_STLBIALL. >> >> Still not sure I understand what "the correct address" is for STLBIALL >> on Calxeda (i.e. whether or not we need to use ARM_SMMU_GR0_NS), but >> something like: > > Hehe, I wasn't actually expecting a patch, but thanks! > >> -- >8 -- >> Subject: [PATCH v2] iommu/arm-smmu: don't touch the secure STLBIALL register >> >> Currently we do a STLBIALL when we initialize the SMMU. However, on >> systems with sane secure >> configurations (i.e. !ARM_SMMU_OPT_SECURE_CFG_ACCESS) that register is >> not supposed to be touched and is marked as "Secure only" in the spec. >> Touching it results in a crash on those platforms. However, on >> platforms with ARM_SMMU_OPT_SECURE_CFG_ACCESS it's the only way to nuke >> the whole TLB, so leave it in for them but rip it out for everyone else. >> >> Signed-off-by: Mitchel Humpherys <mitchelh@codeaurora.org> >> --- >> drivers/iommu/arm-smmu.c | 9 ++++++--- >> 1 file changed, 6 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c >> index 60558f794922..d4c149d83f3d 100644 >> --- a/drivers/iommu/arm-smmu.c >> +++ b/drivers/iommu/arm-smmu.c >> @@ -1686,9 +1686,12 @@ static void arm_smmu_device_reset(struct arm_smmu_device *smmu) >> } >> >> /* Invalidate the TLB, just in case */ >> - writel_relaxed(0, gr0_base + ARM_SMMU_GR0_STLBIALL); >> - writel_relaxed(0, gr0_base + ARM_SMMU_GR0_TLBIALLH); >> - writel_relaxed(0, gr0_base + ARM_SMMU_GR0_TLBIALLNSNH); >> + if (smmu->options & ARM_SMMU_OPT_SECURE_CFG_ACCESS) { >> + writel_relaxed(0, gr0_base + ARM_SMMU_GR0_STLBIALL); > > Right, so this is the bit where we'd need some Calxeda information about > whether or not to subtract 0x400 from gr0_base or not. Beats me. I don't recall any configuration option here. The binding only has a single address. Maybe Andreas remembers. Rob ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2015-01-08 20:58 UTC | newest] Thread overview: 11+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-12-23 17:39 [PATCH] iommu/arm-smmu: don't touch the secure STLBIALL register Mitchel Humpherys 2015-01-06 14:15 ` Will Deacon 2015-01-06 20:16 ` Mitchel Humpherys 2015-01-06 22:35 ` Rob Herring 2015-01-06 23:30 ` Mitchel Humpherys 2015-01-07 10:13 ` Will Deacon 2015-01-07 17:52 ` Mitchel Humpherys 2015-01-07 18:04 ` Will Deacon 2015-01-07 18:35 ` Mitchel Humpherys 2015-01-07 18:53 ` Will Deacon 2015-01-08 20:58 ` Rob Herring
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).