iommu/arm-smmu: don't touch the secure STLBIALL register

PATCH New Mitchel Humpherys 3 participants 11 messages Dec 23, 2014 – Jan 8, 2015 IOMMU

Share & tools

Mitchel Humpherys

author

2014-12-23 17:39 UTC #0 lore raw

PATCH 1/1 iommu/arm-smmu: don't touch the secure STLBIALL register

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 <[email protected]>

---
 drivers/iommu/arm-smmu.c | 2 --
 1 file changed, 2 deletions(-)

drivers/iommu/arm-smmu.c +0 -2

index 60558f794922..9170bbced5e5 100644
@@ -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);

Signature · 3 lines

Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Will Deacon

2015-01-06 14:15 UTC #1 lore raw

PATCH 1/1 iommu/arm-smmu: don't touch the secure STLBIALL register

Hi Mitch,

Quoted 22 lines · Mitchel Humpherys

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 <[email protected]>
---
 drivers/iommu/arm-smmu.c | 2 --
 1 file changed, 2 deletions(-)
drivers/iommu/arm-smmu.c +0 -1
index 60558f794922..9170bbced5e5 100644
@@ -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

drivers/iommu/arm-smmu.c static void arm_smmu_device_reset(struct arm_smmu_device *smmu) · line 1689

Quoted 6 lines

drivers/iommu/arm-smmu.c +0 -1

@@ -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

Mitchel Humpherys

author

2015-01-06 20:16 UTC #2 lore raw

PATCH 1/1 iommu/arm-smmu: don't touch the secure STLBIALL register

Quoted 9 lines · Will Deacon

drivers/iommu/arm-smmu.c static void arm_smmu_device_reset(struct arm_smmu_device *smmu) · line 1689
Quoted 4 lines
drivers/iommu/arm-smmu.c +0 -1
 	/* 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.

Quoted 4 lines

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

Signature · 3 lines

Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Rob Herring

2015-01-06 22:35 UTC #3 lore raw

PATCH 1/1 iommu/arm-smmu: don't touch the secure STLBIALL register

Quoted 13 lines · Mitchel Humpherys

Quoted 12 lines · Will Deacon
Quoted 4 lines
     /* 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?

Quoted 6 lines

Quoted 1 line
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

Mitchel Humpherys

author

2015-01-06 23:30 UTC #4 lore raw

PATCH 1/1 iommu/arm-smmu: don't touch the secure STLBIALL register

Quoted 22 lines · Rob Herring

Quoted 13 lines · Mitchel Humpherys
Quoted 12 lines · Will Deacon
Quoted 4 lines
     /* 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?
Quoted 6 lines
Quoted 1 line
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).

Quoted 7 lines

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

Signature · 3 lines

Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Will Deacon

2015-01-07 10:13 UTC #5 lore raw

PATCH 1/1 iommu/arm-smmu: don't touch the secure STLBIALL register

Quoted 31 lines · Mitchel Humpherys

Quoted 22 lines · Rob Herring
Quoted 13 lines · Mitchel Humpherys
Quoted 12 lines · Will Deacon
Quoted 4 lines
     /* 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?
Quoted 6 lines
Quoted 1 line
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).
Quoted 7 lines
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.

Quoted 8 lines

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

Mitchel Humpherys

author

2015-01-07 17:52 UTC #6 lore raw

PATCH 1/1 iommu/arm-smmu: don't touch the secure STLBIALL register

Quoted 41 lines · Will Deacon

Quoted 31 lines · Mitchel Humpherys
Quoted 22 lines · Rob Herring
Quoted 13 lines · Mitchel Humpherys
Quoted 12 lines · Will Deacon
Quoted 4 lines
     /* 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?
Quoted 6 lines
Quoted 1 line
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).
Quoted 7 lines
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.
Quoted 8 lines
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.

Quoted 3 lines

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.

Quoted 3 lines

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

Signature · 3 lines

Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Will Deacon

2015-01-07 18:04 UTC #7 lore raw

PATCH 1/1 iommu/arm-smmu: don't touch the secure STLBIALL register

Quoted 51 lines · Mitchel Humpherys

Quoted 41 lines · Will Deacon
Quoted 31 lines · Mitchel Humpherys
Quoted 22 lines · Rob Herring
Quoted 13 lines · Mitchel Humpherys
Quoted 12 lines · Will Deacon
Quoted 4 lines
     /* 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?
Quoted 6 lines
Quoted 1 line
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).
Quoted 7 lines
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.
Quoted 8 lines
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.
Quoted 3 lines
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.

Quoted 8 lines

Quoted 3 lines
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

Mitchel Humpherys

author

2015-01-07 18:35 UTC #8 lore raw

PATCH 1/1 iommu/arm-smmu: don't touch the secure STLBIALL register

Quoted 56 lines · Will Deacon

Quoted 55 lines · Mitchel Humpherys
Quoted 41 lines · Will Deacon
Quoted 31 lines · Mitchel Humpherys
Quoted 22 lines · Rob Herring
Quoted 13 lines · Mitchel Humpherys
Quoted 12 lines · Will Deacon
Quoted 4 lines
     /* 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?
Quoted 6 lines
Quoted 1 line
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).
Quoted 7 lines
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.
Quoted 8 lines
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.
Quoted 3 lines
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.
Quoted 3 lines
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.

Quoted 8 lines

Quoted 4 lines
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 <[email protected]>

---
 drivers/iommu/arm-smmu.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

drivers/iommu/arm-smmu.c +6 -3

index 60558f794922..d4c149d83f3d 100644
@@ -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);

Signature · 3 lines

Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Will Deacon

2015-01-07 18:53 UTC #9 lore raw

PATCH 1/1 iommu/arm-smmu: don't touch the secure STLBIALL register

Quoted 12 lines · Mitchel Humpherys

Quoted 11 lines · Will Deacon
Quoted 9 lines · Mitchel Humpherys
Quoted 3 lines · Will Deacon
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?

Quoted 7 lines

Quoted 3 lines
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!

Quoted 21 lines

-- >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 <[email protected]>
---
 drivers/iommu/arm-smmu.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)
drivers/iommu/arm-smmu.c +0 -0
index 60558f794922..d4c149d83f3d 100644

drivers/iommu/arm-smmu.c static void arm_smmu_device_reset(struct arm_smmu_device *smmu) · line 1690

Quoted 8 lines

drivers/iommu/arm-smmu.c +2 -3

@@ -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.

drivers/iommu/arm-smmu.c static void arm_smmu_device_reset(struct arm_smmu_device *smmu) · line 1694

Quoted 4 lines

drivers/iommu/arm-smmu.c +4 -0

+	} 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

Rob Herring

2015-01-08 20:58 UTC #10 lore raw

PATCH 1/1 iommu/arm-smmu: don't touch the secure STLBIALL register

+Andreas

Quoted 46 lines · Will Deacon

Quoted 12 lines · Mitchel Humpherys
Quoted 11 lines · Will Deacon
Quoted 9 lines · Mitchel Humpherys
Quoted 3 lines · Will Deacon
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?
Quoted 7 lines
Quoted 3 lines
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!
Quoted 23 lines
-- >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 <[email protected]>
---
 drivers/iommu/arm-smmu.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)
drivers/iommu/arm-smmu.c +0 -0
index 60558f794922..d4c149d83f3d 100644
@@ -1686,9 +1686,12 @@ static void arm_smmu_device_reset(struct arm_smmu_device *smmu)
      }
 

Quoted 9 lines

drivers/iommu/arm-smmu.c static void arm_smmu_device_reset(struct arm_smmu_device *smmu) · line 1690
Quoted 6 lines
drivers/iommu/arm-smmu.c +2 -3
      /* 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