IOMMU: arm-smmu-v3: Fix broken STE.VALID check in Broadcom Vulcan

PATCH New Prem Mallappa 4 participants 7 messages Dec 17, 2015 – Dec 21, 2015

Subsystems (3)

Device tree bindings IOMMU Arm SMMU drivers

lore

Share & tools

lore

Prem Mallappa

author

2015-12-17 09:59 UTC #0 lore raw

PATCH v2 1/2 IOMMU: arm-smmu-v3: Fix broken STE.VALID check in Broadcom Vulcan

Vulcan SMMUv3 looks into more bits than necessary to validate the STE entry (including EATS, S2PS, AARCH64), which is a overkill, but without the proper encoding; the SMMU stops processing PCIe read/write requests. Giving the h/w what it wants.

Signed-off-by Prem Mallappa <[email protected]>

---
 drivers/iommu/arm-smmu-v3.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

drivers/iommu/arm-smmu-v3.c +10 -0

index 359190b..9886d2d 100644
@@ -260,6 +260,7 @@
 #define STRTAB_STE_2_S2VMID_MASK	0xffffUL
 #define STRTAB_STE_2_VTCR_SHIFT		32
 #define STRTAB_STE_2_VTCR_MASK		0x7ffffUL
+#define STRTAB_STE_2_S2PS_SHIFT        48
 #define STRTAB_STE_2_S2AA64		(1UL << 51)
 #define STRTAB_STE_2_S2ENDI		(1UL << 52)
 #define STRTAB_STE_2_S2PTW		(1UL << 54)

@@ -579,6 +580,7 @@ struct arm_smmu_device {
 	u32				features;
 
 #define ARM_SMMU_OPT_SKIP_PREFETCH	(1 << 0)
+#define ARM_SMMU_OPT_BRCM_BROKEN_STE_VALID	(1 << 1)
 	u32				options;
 
 	struct arm_smmu_cmdq		cmdq;

@@ -643,6 +645,7 @@ struct arm_smmu_option_prop {
 
 static struct arm_smmu_option_prop arm_smmu_options[] = {
 	{ ARM_SMMU_OPT_SKIP_PREFETCH, "hisilicon,broken-prefetch-cmd" },
+	{ ARM_SMMU_OPT_BRCM_BROKEN_STE_VALID, "broadcom,broken-ste-valid-check" },
 	{ 0, NULL},
 };

@@ -1053,6 +1056,13 @@ static void arm_smmu_write_strtab_ent(struct arm_smmu_device *smmu, u32 sid,
 				      : STRTAB_STE_0_CFG_BYPASS;
 		dst[0] = cpu_to_le64(val);
 		dst[2] = 0; /* Nuke the VMID */
+
+		if (smmu && (smmu->options & ARM_SMMU_OPT_BRCM_BROKEN_STE_VALID)) {
+			WARN_ON(smmu->oas != 44);
+			dst[1] = STRTAB_STE_1_EATS_TRANS << STRTAB_STE_1_EATS_SHIFT;
+			dst[2] |= cpu_to_le64(STRTAB_STE_2_S2AA64);
+		}
+
 		if (ste_live)
 			arm_smmu_sync_ste_for_sid(smmu, sid);
 		return;

Signature · 1 line

2.6.0

Prem Mallappa

author

2015-12-17 09:59 UTC #1 lore raw

PATCH v2 2/2 Documentation: dt-bindings: Add option to workaround STE.VALID in Broadcom Vulcan

Signed-off-by Prem Mallappa <[email protected]>

---
 Documentation/devicetree/bindings/iommu/arm,smmu-v3.txt | 5 +++++
 1 file changed, 5 insertions(+)

Documentation/devicetree/bindings/iommu/arm,smmu-v3.txt +5 -0

index 947863a..9c1218e 100644
@@ -43,6 +43,11 @@ the PCIe specification.
 - hisilicon,broken-prefetch-cmd
                     : Avoid sending CMD_PREFETCH_* commands to the SMMU.
 
+- broadcom,broken-ste-valid-check
+                    : Broadcom specific, h/w peeks into more than just 
+		      STE.VALID bit in "Bypass" mode. Enabling this, 
+		      populates needed bits.
+
 ** Example
 
         smmu at 2b400000 {

Signature · 1 line

2.6.0

Will Deacon

2015-12-17 11:51 UTC #2 lore raw

PATCH v2 2/2 Documentation: dt-bindings: Add option to workaround STE.VALID in Broadcom Vulcan

[adding devicetree since I'd like an Ack from them if possible]

Quoted 1 line · Prem Mallappa

Signed-off-by Prem Mallappa <[email protected]>

Please can you add a commit message for this?

Quoted 9 lines

---
 Documentation/devicetree/bindings/iommu/arm,smmu-v3.txt | 5 +++++
 1 file changed, 5 insertions(+)
Documentation/devicetree/bindings/iommu/arm,smmu-v3.txt +0 -0
index 947863a..9c1218e 100644

Documentation/devicetree/bindings/iommu/arm,smmu-v3.txt the PCIe specification. · line 49

Quoted 7 lines

Documentation/devicetree/bindings/iommu/arm,smmu-v3.txt +4 -0

@@ -43,6 +43,11 @@ the PCIe specification.
 - hisilicon,broken-prefetch-cmd
                     : Avoid sending CMD_PREFETCH_* commands to the SMMU.
 
+- broadcom,broken-ste-valid-check
+                    : Broadcom specific, h/w peeks into more than just 
+		      STE.VALID bit in "Bypass" mode. Enabling this, 
+		      populates needed bits.

This should really describe more about what happens, I think. For example,

"Enable ATS and Stage-2 AArch64 translation in bypass STEs, since some
hardware requires this in order for an STE to be treated as valid."

Will

Mark Rutland

2015-12-17 12:00 UTC #3 lore raw

PATCH v2 2/2 Documentation: dt-bindings: Add option to workaround STE.VALID in Broadcom Vulcan

Quoted 26 lines · Will Deacon

[adding devicetree since I'd like an Ack from them if possible]
Quoted 1 line · Prem Mallappa
Signed-off-by Prem Mallappa <[email protected]>
Please can you add a commit message for this?
Quoted 9 lines
---
 Documentation/devicetree/bindings/iommu/arm,smmu-v3.txt | 5 +++++
 1 file changed, 5 insertions(+)
Documentation/devicetree/bindings/iommu/arm,smmu-v3.txt +0 -0
index 947863a..9c1218e 100644
Documentation/devicetree/bindings/iommu/arm,smmu-v3.txt the PCIe specification. · line 49
Quoted 7 lines
Documentation/devicetree/bindings/iommu/arm,smmu-v3.txt +4 -0
@@ -43,6 +43,11 @@ the PCIe specification.
 - hisilicon,broken-prefetch-cmd
                     : Avoid sending CMD_PREFETCH_* commands to the SMMU.
 
+- broadcom,broken-ste-valid-check
+                    : Broadcom specific, h/w peeks into more than just 
+		      STE.VALID bit in "Bypass" mode. Enabling this, 
+		      populates needed bits.
This should really describe more about what happens, I think. For example,

Quoted 2 lines

"Enable ATS and Stage-2 AArch64 translation in bypass STEs, since some
hardware requires this in order for an STE to be treated as valid."

We certainly need to define the "needed bits".

Assuming your example cover all of those, it sounds fine to me.

Mark.

Will Deacon

2015-12-17 11:42 UTC #4 lore raw

PATCH v2 1/2 IOMMU: arm-smmu-v3: Fix broken STE.VALID check in Broadcom Vulcan

In reply to Prem Mallappa

Hi Prem,

Thanks for the update. I still have a few questions though.

Quoted 39 lines · Prem Mallappa

Signed-off-by Prem Mallappa <[email protected]>

---
 drivers/iommu/arm-smmu-v3.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

drivers/iommu/arm-smmu-v3.c +3 -0

index 359190b..9886d2d 100644
@@ -260,6 +260,7 @@
 #define STRTAB_STE_2_S2VMID_MASK	0xffffUL
 #define STRTAB_STE_2_VTCR_SHIFT		32
 #define STRTAB_STE_2_VTCR_MASK		0x7ffffUL
+#define STRTAB_STE_2_S2PS_SHIFT        48
 #define STRTAB_STE_2_S2AA64		(1UL << 51)
 #define STRTAB_STE_2_S2ENDI		(1UL << 52)
 #define STRTAB_STE_2_S2PTW		(1UL << 54)

@@ -579,6 +580,7 @@ struct arm_smmu_device {
 	u32				features;
 
 #define ARM_SMMU_OPT_SKIP_PREFETCH	(1 << 0)
+#define ARM_SMMU_OPT_BRCM_BROKEN_STE_VALID	(1 << 1)
 	u32				options;
 
 	struct arm_smmu_cmdq		cmdq;

@@ -643,6 +645,7 @@ struct arm_smmu_option_prop {
 
 static struct arm_smmu_option_prop arm_smmu_options[] = {
 	{ ARM_SMMU_OPT_SKIP_PREFETCH, "hisilicon,broken-prefetch-cmd" },
+	{ ARM_SMMU_OPT_BRCM_BROKEN_STE_VALID, "broadcom,broken-ste-valid-check" },
 	{ 0, NULL},
 };

drivers/iommu/arm-smmu-v3.c static void arm_smmu_write_strtab_ent(struct arm_smmu_device *smmu, u32 sid, · line 1061

Quoted 6 lines

drivers/iommu/arm-smmu-v3.c +3 -0

@@ -1053,6 +1056,13 @@ static void arm_smmu_write_strtab_ent(struct arm_smmu_device *smmu, u32 sid,
 				      : STRTAB_STE_0_CFG_BYPASS;
 		dst[0] = cpu_to_le64(val);
 		dst[2] = 0; /* Nuke the VMID */
+
+		if (smmu && (smmu->options & ARM_SMMU_OPT_BRCM_BROKEN_STE_VALID)) {
+			WARN_ON(smmu->oas != 44);

Do we really need this WARN_ON?

drivers/iommu/arm-smmu-v3.c static void arm_smmu_write_strtab_ent(struct arm_smmu_device *smmu, u32 sid, · line 1062

Quoted 1 line

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

+			dst[1] = STRTAB_STE_1_EATS_TRANS << STRTAB_STE_1_EATS_SHIFT;

cpu_to_le64 here too.

drivers/iommu/arm-smmu-v3.c static void arm_smmu_write_strtab_ent(struct arm_smmu_device *smmu, u32 sid, · line 1065

Quoted 3 lines

drivers/iommu/arm-smmu-v3.c +3 -0
+			dst[2] |= cpu_to_le64(STRTAB_STE_2_S2AA64);
+		}
+

Is this workaround only needed for bypass STEs? If not, we have a problem when we install a stage-1 entry, because we'll clear the EATS bits as it stands.

Will

Prem (Premachandra) Mallappa

2015-12-19 04:47 UTC #5 lore raw

PATCH v2 1/2 IOMMU: arm-smmu-v3: Fix broken STE.VALID check in Broadcom Vulcan

Hi Will,

Quoted 4 lines

Is this workaround only needed for bypass STEs? If not, we have a problem when we install a stage-1 entry, because we'll clear the EATS bits as it stands.

Yes, this _was_ needed only in Bypass STEs, AFAIK.
However, the h/w fixes that were committed recently has this worked out.
I have tested and confirmed this patch is no longer necessary. We'll drop this series as STE.VALID is not broken anymore :).

Cheers,
/Prem

Will Deacon

2015-12-21 09:55 UTC #6 lore raw

PATCH v2 1/2 IOMMU: arm-smmu-v3: Fix broken STE.VALID check in Broadcom Vulcan

Quoted 1 line · Prem (Premachandra) Mallappa

Hi Will,

Hello Prem,

Quoted 9 lines

Quoted 4 lines
Is this workaround only needed for bypass STEs? If not, we have a problem when we install a stage-1 entry, because we'll clear the EATS bits as it stands.
Yes, this _was_ needed only in Bypass STEs, AFAIK.
However, the h/w fixes that were committed recently has this worked out. I have tested and confirmed this patch is no longer necessary. We'll drop this series as STE.VALID is not broken anymore :).

Hey, that's an early Christmas present! :)

Will