Skip to contentSkip to conversation

IOMMU: arm-smmu-v3: fix broken S2PS and AARCH64 in Broadcom Vulcan

PATCH Superseded Prem Mallappa 3 participants 6 messages Dec 14, 2015 – Dec 16, 2015 IOMMU

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Prem Mallappa

author
#0 lore raw

PATCH 1/1 IOMMU: arm-smmu-v3: fix broken S2PS and AARCH64 in Broadcom Vulcan

Vulcan SMMUv3 looks for AARCH64 and S2PS inroder to validate the STE entry, which is a overkill, but when proper encoding not found; 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 | 12 ++++++++++++
 1 file changed, 12 insertions(+)
drivers/iommu/arm-smmu-v3.c +12 -0
index d4af50d..dfda564 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)

@@ -577,6 +578,7 @@ struct arm_smmu_device {
 	u32				features;
 
 #define ARM_SMMU_OPT_SKIP_PREFETCH	(1 << 0)
+#define ARM_SMMU_OPT_BROKEN_STE_VALID	(1 << 1)
 	u32				options;
 
 	struct arm_smmu_cmdq		cmdq;

@@ -641,6 +643,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_BROKEN_STE_VALID, "broadcom,broken-ste-valid-check" },
 	{ 0, NULL},
 };
 

@@ -1046,6 +1049,15 @@ 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_BROKEN_STE_VALID)) {
+#define SMMU_STE_OAS_44_BITS 0x4UL
+			WARN_ON(smmu->oas != 44);
+			dst[1] = STRTAB_STE_1_EATS_TRANS << STRTAB_STE_1_EATS_SHIFT;
+			dst[2] |= cpu_to_le64((SMMU_STE_OAS_44_BITS << STRTAB_STE_2_S2PS_SHIFT) |
+					STRTAB_STE_2_S2AA64);
+		}
+
 		if (ste_live)
 			arm_smmu_sync_ste_for_sid(smmu, sid);
 		return;
Signature · 1 line
2.6.0
#1 lore raw

PATCH 1/1 IOMMU: arm-smmu-v3: fix broken S2PS and AARCH64 in Broadcom Vulcan

Hi Prem,

Quoted 1 line · Prem Mallappa

Vulcan SMMUv3 looks for AARCH64 and S2PS inroder to validate the STE entry,

'inroder' ?

Quoted 29 lines

which is a overkill, but when proper encoding not found; 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 | 12 ++++++++++++
 1 file changed, 12 insertions(+)
drivers/iommu/arm-smmu-v3.c +2 -0
index d4af50d..dfda564 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)

@@ -577,6 +578,7 @@ struct arm_smmu_device {
 	u32				features;
 
 #define ARM_SMMU_OPT_SKIP_PREFETCH	(1 << 0)
+#define ARM_SMMU_OPT_BROKEN_STE_VALID	(1 << 1)
 	u32				options;
 
 	struct arm_smmu_cmdq		cmdq;

drivers/iommu/arm-smmu-v3.c struct arm_smmu_option_prop { · line 646

Quoted 4 lines
drivers/iommu/arm-smmu-v3.c +1 -0
@@ -641,6 +643,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_BROKEN_STE_VALID, "broadcom,broken-ste-valid-check" },

This looks like a more specific problem than "broken ste valid check". Maybe we could call the prop ARM_SMMU_OPT_BCOM_BROKEN_STE_VALID?

You also need to update the devicetree binding in Documentation/devicetree/bindings/iommu/arm,smmu-v3.txt.

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

Quoted 10 lines
drivers/iommu/arm-smmu-v3.c +3 -0
 	{ 0, NULL},
 };
 

@@ -1046,6 +1049,15 @@ 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_BROKEN_STE_VALID)) {
+#define SMMU_STE_OAS_44_BITS 0x4UL

Please don't add a #define here. Can we instead use the oas field that we've extracted from IDR5? I think we need to be doing that anyway when we're using stage-2 translation, looking at the spec...

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

Quoted 2 lines
drivers/iommu/arm-smmu-v3.c +2 -0
+			WARN_ON(smmu->oas != 44);
+			dst[1] = STRTAB_STE_1_EATS_TRANS << STRTAB_STE_1_EATS_SHIFT;

Why are you enabling ATS?

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

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

Will

#2 lore raw

PATCH 1/1 IOMMU: arm-smmu-v3: fix broken S2PS and AARCH64 in Broadcom Vulcan

Quoted 12 lines · Will Deacon

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

Quoted 7 lines · Prem Mallappa
drivers/iommu/arm-smmu-v3.c +3 -0
@@ -1046,6 +1049,15 @@ 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_BROKEN_STE_VALID)) {
+#define SMMU_STE_OAS_44_BITS 0x4UL

Please don't add a #define here. Can we instead use the oas field that we've extracted from IDR5? I think we need to be doing that anyway when we're using stage-2 translation, looking at the spec...

... which we already are doing, thanks to the vtcr initialisation in the io-pgtable code, which populates the PS field based on the OAS.

Will

#3 lore raw

PATCH 1/1 IOMMU: arm-smmu-v3: fix broken S2PS and AARCH64 in Broadcom Vulcan

In reply to Will Deacon

Hi Will, Thanks for the review.

Quoted 6 lines · Prem Mallappa
Quoted 2 lines

Vulcan SMMUv3 looks for AARCH64 and S2PS inroder to validate the STE entry,

'inroder' ?

In order

Quoted 36 lines

drivers/iommu/arm-smmu-

Quoted 9 lines

which is a overkill, but when proper encoding not found; 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 | 12 ++++++++++++
 1 file changed, 12 insertions(+)
drivers/iommu/arm-smmu- renamed from drivers/iommu/arm-smmu-v3.c +0 -0

v3.c

Quoted 20 lines

index d4af50d..dfda564 100644

drivers/iommu/arm-smmu-v3.c +2 -0
@@ -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)

@@ -577,6 +578,7 @@ struct arm_smmu_device {
 	u32				features;
 
 #define ARM_SMMU_OPT_SKIP_PREFETCH	(1 << 0)
+#define ARM_SMMU_OPT_BROKEN_STE_VALID	(1 << 1)
 	u32				options;
 
 	struct arm_smmu_cmdq		cmdq;

drivers/iommu/arm-smmu-v3.c struct arm_smmu_option_prop { · line 645

Quoted 3 lines
drivers/iommu/arm-smmu-v3.c +0 -0
@@ -641,6 +643,7 @@ struct arm_smmu_option_prop {
 
 static struct arm_smmu_option_prop arm_smmu_options[] = {
 	{ ARM_SMMU_OPT_SKIP_PREFETCH, "hisilicon,broken-prefetch-

cmd" },

drivers/iommu/arm-smmu-v3.c struct arm_smmu_option_prop { · line 646

Quoted 1 line
drivers/iommu/arm-smmu-v3.c +1 -0
+	{ ARM_SMMU_OPT_BROKEN_STE_VALID, "broadcom,broken-ste-

valid-check"

Quoted 9 lines

drivers/iommu/arm-smmu-v3.c struct arm_smmu_option_prop { · line 647

Quoted 1 line
drivers/iommu/arm-smmu-v3.c +1 -0
+},

This looks like a more specific problem than "broken ste valid check". Maybe we could call the prop ARM_SMMU_OPT_BCOM_BROKEN_STE_VALID?

You also need to update the devicetree binding in Documentation/devicetree/bindings/iommu/arm,smmu-v3.txt.

Sure I'll send out v2.

Quoted 11 lines

drivers/iommu/arm-smmu-v3.c static void arm_smmu_write_strtab_ent(struct

Quoted 4 lines
drivers/iommu/arm-smmu-v3.c +0 -0
 	{ 0, NULL},
 };

@@ -1046,6 +1049,15 @@ static void arm_smmu_write_strtab_ent(struct

arm_smmu_device *smmu, u32 sid,

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

Quoted 5 lines
drivers/iommu/arm-smmu-v3.c +2 -0
 				      : STRTAB_STE_0_CFG_BYPASS;
 		dst[0] = cpu_to_le64(val);
 		dst[2] = 0; /* Nuke the VMID */
+
+		if (smmu && (smmu->options &

ARM_SMMU_OPT_BROKEN_STE_VALID)) {

Quoted 6 lines

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

Quoted 1 line
drivers/iommu/arm-smmu-v3.c +1 -0
+#define SMMU_STE_OAS_44_BITS 0x4UL

Please don't add a #define here. Can we instead use the oas field that we've extracted from IDR5? I think we need to be doing that anyway when we're using stage-2 translation, looking at the spec...

As you mentioned in other email, "OAS" is already taken care by io-pgtable vtcr.

Quoted 6 lines

drivers/iommu/arm-smmu-v3.c static void arm_smmu_write_strtab_ent(struct · line 1056

Quoted 2 lines
drivers/iommu/arm-smmu-v3.c +2 -0
+			WARN_ON(smmu->oas != 44);
+			dst[1] = STRTAB_STE_1_EATS_TRANS <<

STRTAB_STE_1_EATS_SHIFT;

Why are you enabling ATS?

Again, based on feedback from h/w team which expects these fields populated (hence broken :) ), I am not aware of the h/w doing anything beyond checking this for STE.VALID() (i.o.w, if STE is illegal).

/Prem

#4 lore raw

PATCH 1/1 IOMMU: arm-smmu-v3: fix broken S2PS and AARCH64 in Broadcom Vulcan

Quoted 7 lines · Prem (Premachandra) Mallappa
Quoted 6 lines · Prem Mallappa
Quoted 2 lines

Vulcan SMMUv3 looks for AARCH64 and S2PS inroder to validate the STE entry,

'inroder' ?

In order

Got it!

Quoted 58 lines
Quoted 36 lines

drivers/iommu/arm-smmu-

Quoted 9 lines

which is a overkill, but when proper encoding not found; 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 | 12 ++++++++++++
 1 file changed, 12 insertions(+)
drivers/iommu/arm-smmu- renamed from drivers/iommu/arm-smmu-v3.c +0 -0

v3.c

Quoted 20 lines

index d4af50d..dfda564 100644

drivers/iommu/arm-smmu-v3.c +2 -0
@@ -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)

@@ -577,6 +578,7 @@ struct arm_smmu_device {
 	u32				features;
 
 #define ARM_SMMU_OPT_SKIP_PREFETCH	(1 << 0)
+#define ARM_SMMU_OPT_BROKEN_STE_VALID	(1 << 1)
 	u32				options;
 
 	struct arm_smmu_cmdq		cmdq;

drivers/iommu/arm-smmu-v3.c struct arm_smmu_option_prop { · line 645

Quoted 3 lines
drivers/iommu/arm-smmu-v3.c +0 -0
@@ -641,6 +643,7 @@ struct arm_smmu_option_prop {
 
 static struct arm_smmu_option_prop arm_smmu_options[] = {
 	{ ARM_SMMU_OPT_SKIP_PREFETCH, "hisilicon,broken-prefetch-

cmd" },

drivers/iommu/arm-smmu-v3.c struct arm_smmu_option_prop { · line 646

Quoted 1 line
drivers/iommu/arm-smmu-v3.c +1 -0
+	{ ARM_SMMU_OPT_BROKEN_STE_VALID, "broadcom,broken-ste-

valid-check"

Quoted 9 lines

drivers/iommu/arm-smmu-v3.c struct arm_smmu_option_prop { · line 647

Quoted 1 line
drivers/iommu/arm-smmu-v3.c +1 -0
+},

This looks like a more specific problem than "broken ste valid check". Maybe we could call the prop ARM_SMMU_OPT_BCOM_BROKEN_STE_VALID?

You also need to update the devicetree binding in Documentation/devicetree/bindings/iommu/arm,smmu-v3.txt.

Sure I'll send out v2.

Quoted 11 lines

drivers/iommu/arm-smmu-v3.c static void arm_smmu_write_strtab_ent(struct

Quoted 4 lines
drivers/iommu/arm-smmu-v3.c +0 -0
 	{ 0, NULL},
 };

@@ -1046,6 +1049,15 @@ static void arm_smmu_write_strtab_ent(struct

arm_smmu_device *smmu, u32 sid,

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

Quoted 5 lines
drivers/iommu/arm-smmu-v3.c +2 -0
 				      : STRTAB_STE_0_CFG_BYPASS;
 		dst[0] = cpu_to_le64(val);
 		dst[2] = 0; /* Nuke the VMID */
+
+		if (smmu && (smmu->options &

ARM_SMMU_OPT_BROKEN_STE_VALID)) {

Quoted 8 lines
Quoted 6 lines

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

Quoted 1 line
drivers/iommu/arm-smmu-v3.c +1 -0
+#define SMMU_STE_OAS_44_BITS 0x4UL

Please don't add a #define here. Can we instead use the oas field that we've extracted from IDR5? I think we need to be doing that anyway when we're using stage-2 translation, looking at the spec...

As you mentioned in other email, "OAS" is already taken care by io-pgtable vtcr.

Ok -- so what is the value reported by your IDR5?

Quoted 8 lines
Quoted 6 lines

drivers/iommu/arm-smmu-v3.c static void arm_smmu_write_strtab_ent(struct · line 1056

Quoted 2 lines
drivers/iommu/arm-smmu-v3.c +2 -0
+			WARN_ON(smmu->oas != 44);
+			dst[1] = STRTAB_STE_1_EATS_TRANS <<

STRTAB_STE_1_EATS_SHIFT;

Why are you enabling ATS?

Again, based on feedback from h/w team which expects these fields populated (hence broken :) ),

Ok, I just didn't see any mention of that in the commit message and wanted to make sure it wasn't an accidental unrelated change.

Will

#5 lore raw

PATCH 1/1 IOMMU: arm-smmu-v3: fix broken S2PS and AARCH64 in Broadcom Vulcan

Quoted 1 line

Ok -- so what is the value reported by your IDR5?

Our IDR5, OAS=0x4 (44bits)

Quoted 4 lines

Ok, I just didn't see any mention of that in the commit message and wanted to make sure it wasn't an accidental unrelated change.

Got it.

/Prem