Skip to contentSkip to conversation

iommu/arm-smmu-v2: ThunderX(errata-23399) mis-extends 64bit registers

PATCH Superseded tchalamarla at caviumnetworks.com 5 participants 7 messages Jul 30, 2015 – Jul 31, 2015 2 messages not in the archive IOMMU

v1 of 4 Superseded Latest is v4 →

lore
Share & tools
lore

tchalamarla at caviumnetworks.com

author
#0 lore raw

PATCH 1/1 iommu/arm-smmu-v2: ThunderX(errata-23399) mis-extends 64bit registers

From: Tirumalesh Chalamarla <[email protected]>

The SMMU architecture defines two different behaviors when 64-bit registers are written with 32-bit writes. The first behavior causes zero extension into the upper 32-bits. The second behavior splits a 64-bit register into "normal" 32-bit register pairs.

On some passes of ThunderX,
the following registers incorrectly zero extended when they should instead behave as normal 32-bit register pairs:

SMMU()_(S)GFAR
SMMU()_NSGFAR
SMMU()_CB()_TTBR0
SMMU()_CB()_TTBR1
SMMU()_CB()_FAR

Signed-off-by Tirumalesh Chalamarla <[email protected]>
---
 drivers/iommu/arm-smmu.c | 51 ++++++++++++++++++++++++++++++++++--------------
 1 file changed, 36 insertions(+), 15 deletions(-)
drivers/iommu/arm-smmu.c +36 -15
index 66a803b..7a3cf7f 100644
@@ -290,6 +290,7 @@ struct arm_smmu_device {
 	u32				features;
 
 #define ARM_SMMU_OPT_SECURE_CFG_ACCESS (1 << 0)
+#define ARM_SMMU_OPT_64BIT_WRITES_ONLY (1 << 1)
 	u32				options;
 	enum arm_smmu_arch_version	version;
 

@@ -351,6 +352,8 @@ struct arm_smmu_option_prop {
 
 static struct arm_smmu_option_prop arm_smmu_options[] = {
 	{ ARM_SMMU_OPT_SECURE_CFG_ACCESS, "calxeda,smmu-secure-config-access" },
+	/* ThunderX errata 23399 */
+	{ ARM_SMMU_OPT_64BIT_WRITES_ONLY, "thunderx,smmu-64-bit-writes-only" },
 	{ 0, NULL},
 };
 

@@ -719,6 +722,7 @@ static void arm_smmu_init_context_bank(struct arm_smmu_domain *smmu_domain,
 				       struct io_pgtable_cfg *pgtbl_cfg)
 {
 	u32 reg;
+	u64 reg64;
 	bool stage1;
 	struct arm_smmu_cfg *cfg = &smmu_domain->cfg;
 	struct arm_smmu_device *smmu = smmu_domain->smmu;

@@ -762,22 +766,39 @@ static void arm_smmu_init_context_bank(struct arm_smmu_domain *smmu_domain,
 
 	/* TTBRs */
 	if (stage1) {
-		reg = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[0];
-		writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBR0_LO);
-		reg = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[0] >> 32;
-		reg |= ARM_SMMU_CB_ASID(cfg) << TTBRn_HI_ASID_SHIFT;
-		writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBR0_HI);
-
-		reg = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[1];
-		writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBR1_LO);
-		reg = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[1] >> 32;
-		reg |= ARM_SMMU_CB_ASID(cfg) << TTBRn_HI_ASID_SHIFT;
-		writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBR1_HI);
+		if (smmu->options & ARM_SMMU_OPT_64BIT_WRITES_ONLY) {
+			reg64 = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[0];
+			reg64 |= ((u64) ARM_SMMU_CB_ASID(cfg)) <<
+						(TTBRn_HI_ASID_SHIFT + 32);
+			writeq_relaxed(reg, cb_base + ARM_SMMU_CB_TTBR0_LO);
+
+			reg64 = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[1];
+			reg64 |= ((u64)ARM_SMMU_CB_ASID(cfg)) <<
+						(TTBRn_HI_ASID_SHIFT + 32);
+			writeq_relaxed(reg, cb_base + ARM_SMMU_CB_TTBR1_LO);
+		} else {
+			reg = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[0];
+			writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBR0_LO);
+			reg = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[0] >> 32;
+			reg |= ARM_SMMU_CB_ASID(cfg) << TTBRn_HI_ASID_SHIFT;
+			writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBR0_HI);
+
+			reg = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[1];
+			writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBR1_LO);
+			reg = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[1] >> 32;
+			reg |= ARM_SMMU_CB_ASID(cfg) << TTBRn_HI_ASID_SHIFT;
+			writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBR1_HI);
+		}
 	} else {
-		reg = pgtbl_cfg->arm_lpae_s2_cfg.vttbr;
-		writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBR0_LO);
-		reg = pgtbl_cfg->arm_lpae_s2_cfg.vttbr >> 32;
-		writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBR0_HI);
+		if (smmu->options & ARM_SMMU_OPT_64BIT_WRITES_ONLY) {
+			reg64 = pgtbl_cfg->arm_lpae_s2_cfg.vttbr;
+			writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBR0_LO);
+		} else {
+			reg = pgtbl_cfg->arm_lpae_s2_cfg.vttbr;
+			writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBR0_LO);
+			reg = pgtbl_cfg->arm_lpae_s2_cfg.vttbr >> 32;
+			writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBR0_HI);
+		}
 	}
 
 	/* TTBCR */
Signature · 1 line
2.1.0
#1 lore raw

PATCH 1/1 iommu/arm-smmu-v2: ThunderX(errata-23399) mis-extends 64bit registers

Hello,

Quoted 21 lines · tchalamarla at caviumnetworks.com

From: Tirumalesh Chalamarla <[email protected]>

The SMMU architecture defines two different behaviors when 64-bit registers are written with 32-bit writes. The first behavior causes zero extension into the upper 32-bits. The second behavior splits a 64-bit register into "normal" 32-bit register pairs.

On some passes of ThunderX,
the following registers incorrectly zero extended when they should instead behave as normal 32-bit register pairs:

SMMU()_(S)GFAR
SMMU()_NSGFAR
SMMU()_CB()_TTBR0
SMMU()_CB()_TTBR1
SMMU()_CB()_FAR

Signed-off-by Tirumalesh Chalamarla <[email protected]>
---
 drivers/iommu/arm-smmu.c | 51 ++++++++++++++++++++++++++++++++++--------------
 1 file changed, 36 insertions(+), 15 deletions(-)

[...]

Quoted 28 lines
drivers/iommu/arm-smmu.c +13 -11
@@ -762,22 +766,39 @@ static void arm_smmu_init_context_bank(struct arm_smmu_domain *smmu_domain,
 
 	/* TTBRs */
 	if (stage1) {
-		reg = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[0];
-		writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBR0_LO);
-		reg = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[0] >> 32;
-		reg |= ARM_SMMU_CB_ASID(cfg) << TTBRn_HI_ASID_SHIFT;
-		writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBR0_HI);
-
-		reg = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[1];
-		writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBR1_LO);
-		reg = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[1] >> 32;
-		reg |= ARM_SMMU_CB_ASID(cfg) << TTBRn_HI_ASID_SHIFT;
-		writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBR1_HI);
+		if (smmu->options & ARM_SMMU_OPT_64BIT_WRITES_ONLY) {
+			reg64 = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[0];
+			reg64 |= ((u64) ARM_SMMU_CB_ASID(cfg)) <<
+						(TTBRn_HI_ASID_SHIFT + 32);
+			writeq_relaxed(reg, cb_base + ARM_SMMU_CB_TTBR0_LO);
+
+			reg64 = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[1];
+			reg64 |= ((u64)ARM_SMMU_CB_ASID(cfg)) <<
+						(TTBRn_HI_ASID_SHIFT + 32);
+			writeq_relaxed(reg, cb_base + ARM_SMMU_CB_TTBR1_LO);
+		} else {
+			reg = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[0];
+			writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBR0_LO);

drivers/iommu/arm-smmu.c static void arm_smmu_init_context_bank(struct arm_smmu_domain *smmu_domain, · line 791

Quoted 10 lines
drivers/iommu/arm-smmu.c +10 -0
+			reg = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[0] >> 32;
+			reg |= ARM_SMMU_CB_ASID(cfg) << TTBRn_HI_ASID_SHIFT;
+			writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBR0_HI);
+
+			reg = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[1];
+			writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBR1_LO);
+			reg = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[1] >> 32;
+			reg |= ARM_SMMU_CB_ASID(cfg) << TTBRn_HI_ASID_SHIFT;
+			writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBR1_HI);
+		}

I'm fine with making this sort of change if you need it, but this is pretty ugly. Worse, it won't compile for 32-bit ARM.

How about we add a wrapper around these, say smmu_writeq(...), which can then either expand to 2x writel_relaxed or 1x writeq_relaxed depending on CONFIG_64BIT and your erratum workaround?

Don't forgot to update the ATOS code too (so you need to write the high word first).

Will

A message that is not in the archive (sent off-list or not yet received): [email protected]

#3 lore raw

On Jul 30, 2015, at 11:45 AM, Will Deacon <[email protected]<mailto:[email protected]>> wrote:

Hello,

On Thu, Jul 30, 2015 at 06:55:06PM +0100, [email protected]<mailto:[email protected]> wrote:
From: Tirumalesh Chalamarla <[email protected]<mailto:[email protected]>>

The SMMU architecture defines two different behaviors when 64-bit registers are written with 32-bit writes. The first behavior causes zero extension into the upper 32-bits. The second behavior splits a 64-bit register into "normal" 32-bit register pairs.

On some passes of ThunderX,
the following registers incorrectly zero extended when they should instead behave as normal 32-bit register pairs:

SMMU()_(S)GFAR
SMMU()_NSGFAR
SMMU()_CB()_TTBR0
SMMU()_CB()_TTBR1
SMMU()_CB()_FAR

Signed-off-by Tirumalesh Chalamarla <[email protected]<mailto:[email protected]>>

---
drivers/iommu/arm-smmu.c | 51 ++++++++++++++++++++++++++++++++++--------------

1 file changed, 36 insertions(+), 15 deletions(-)

[...]

@@ -762,22 +766,39 @@ static void arm_smmu_init_context_bank(struct arm_smmu_domain *smmu_domain,

/* TTBRs */
if (stage1) {
- reg = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[0];
- writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBR0_LO);
- reg = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[0] >> 32;
- reg |= ARM_SMMU_CB_ASID(cfg) << TTBRn_HI_ASID_SHIFT;
- writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBR0_HI); -
- reg = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[1];
- writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBR1_LO);
- reg = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[1] >> 32;
- reg |= ARM_SMMU_CB_ASID(cfg) << TTBRn_HI_ASID_SHIFT;
- writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBR1_HI);
+ if (smmu->options & ARM_SMMU_OPT_64BIT_WRITES_ONLY) {
+ reg64 = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[0];
+ reg64 |= ((u64) ARM_SMMU_CB_ASID(cfg)) <<
+ (TTBRn_HI_ASID_SHIFT + 32);
+ writeq_relaxed(reg, cb_base + ARM_SMMU_CB_TTBR0_LO); +
+ reg64 = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[1];
+ reg64 |= ((u64)ARM_SMMU_CB_ASID(cfg)) <<
+ (TTBRn_HI_ASID_SHIFT + 32);
+ writeq_relaxed(reg, cb_base + ARM_SMMU_CB_TTBR1_LO);
+ } else {
+ reg = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[0];
+ writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBR0_LO);
+ reg = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[0] >> 32;
+ reg |= ARM_SMMU_CB_ASID(cfg) << TTBRn_HI_ASID_SHIFT;
+ writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBR0_HI); +
+ reg = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[1];
+ writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBR1_LO);
+ reg = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[1] >> 32;
+ reg |= ARM_SMMU_CB_ASID(cfg) << TTBRn_HI_ASID_SHIFT;
+ writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBR1_HI);
+ }

I'm fine with making this sort of change if you need it, but this is pretty ugly. Worse, it won't compile for 32-bit ARM.

How about we add a wrapper around these, say smmu_writeq(...), which can then either expand to 2x writel_relaxed or 1x writeq_relaxed depending on CONFIG_64BIT and your erratum workaround?

i think we don’t even need to restrict it for work around.
we can just use CONFIG_64BIT and smmu_writeq.

if thats fine i will repost the patch.

Thanks,
Tirumalesh.
Don't forgot to update the ATOS code too (so you need to write the high word first).

Will

A message that is not in the archive (sent off-list or not yet received): 96AD8EF9-D416-43AB-BA6F-2C25098BB8BB-M3mlKVOIwJVv6pq1l3V1OdBPR1lH4CV8@public.gmane.org

#5 lore raw

is some thing like this looks good

+#ifdef CONFIG_64BIT
+#define smmu_writeq(reg64, addr) writeq_relaxed((reg64), (addr))
+#else
+#define smmu_writeq(reg64, addr) \
+ writel_relaxed(((reg64) >> 32), ((addr) + 4)); \
+ writel_relaxed((reg64), (addr))
+
+
/* Configuration registers */
#define ARM_SMMU_GR0_sCR0 0x0
#define sCR0_CLIENTPD (1 << 0)
@@ -226,7 +234,7 @@
#define TTBCR2_SEP_SHIFT 15
#define TTBCR2_SEP_UPSTREAM (0x7 << TTBCR2_SEP_SHIFT)

-#define TTBRn_HI_ASID_SHIFT 16
+#define TTBRn_ASID_SHIFT 48

#define FSR_MULTI (1 << 31)
#define FSR_SS (1 << 30)
@@ -719,6 +727,7 @@ static void arm_smmu_init_context_bank(struct arm_smmu_domain *smmu_domain,
struct io_pgtable_cfg *pgtbl_cfg)

 {
        u32 reg;
+       u64 reg64;
        bool stage1;
        struct arm_smmu_cfg *cfg = &smmu_domain->cfg;
        struct arm_smmu_device *smmu = smmu_domain->smmu;

@@ -762,22 +771,16 @@ static void arm_smmu_init_context_bank(struct arm_smmu_domain *smmu_domain,

        /* TTBRs */
        if (stage1) {
-               reg = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[0];
-               writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBR0_LO);
-               reg = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[0] >> 32;
-               reg |= ARM_SMMU_CB_ASID(cfg) << TTBRn_HI_ASID_SHIFT;
-               writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBR0_HI);
-
-               reg = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[1];
-               writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBR1_LO);
-               reg = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[1] >> 32;
-               reg |= ARM_SMMU_CB_ASID(cfg) << TTBRn_HI_ASID_SHIFT;
-               writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBR1_HI);
+               reg64 = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[0];
+               reg64 |= ((u64)ARM_SMMU_CB_ASID(cfg)) << TTBRn_ASID_SHIFT;
+               smmu_writeq(reg64, cb_base + ARM_SMMU_CB_TTBR0_LO);
+
+               reg64 = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[1];
+               reg64 |= ARM_SMMU_CB_ASID(cfg) << TTBRn_ASID_SHIFT;
+               smmu_writeq(reg, cb_base + ARM_SMMU_CB_TTBR1_LO);
        } else {
-               reg = pgtbl_cfg->arm_lpae_s2_cfg.vttbr;
-               writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBR0_LO);
-               reg = pgtbl_cfg->arm_lpae_s2_cfg.vttbr >> 32;
-               writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBR0_HI);
+               reg64 = pgtbl_cfg->arm_lpae_s2_cfg.vttbr;
+               smmu_writeq(reg, cb_base + ARM_SMMU_CB_TTBR0_LO);
        }

        /* TTBCR */
@@ -1236,10 +1239,8 @@ static phys_addr_t arm_smmu_iova_to_phys_hard(struct iommu_domain *domain,
                u32 reg = iova & ~0xfff;
                writel_relaxed(reg, cb_base + ARM_SMMU_CB_ATS1PR_LO);
        } else {
-               u32 reg = iova & ~0xfff;
-               writel_relaxed(reg, cb_base + ARM_SMMU_CB_ATS1PR_LO);
-               reg = ((u64)iova & ~0xfff) >> 32;
-               writel_relaxed(reg, cb_base + ARM_SMMU_CB_ATS1PR_HI);
+               u64 reg = iova & ~0xfff;
+               smmu_writeq(reg, cb_base + ARM_SMMU_CB_ATS1PR_LO);

}

if (readl_poll_timeout_atomic(cb_base + ARM_SMMU_CB_ATSR, tmp,
~

On Jul 30, 2015, at 12:07 PM, Chalamarla, Tirumalesh <[email protected]<mailto:[email protected]>> wrote:

On Jul 30, 2015, at 11:45 AM, Will Deacon <[email protected]<mailto:[email protected]>> wrote:

Hello,

On Thu, Jul 30, 2015 at 06:55:06PM +0100, [email protected]<mailto:[email protected]> wrote:
From: Tirumalesh Chalamarla <[email protected]<mailto:[email protected]>>

The SMMU architecture defines two different behaviors when 64-bit registers are written with 32-bit writes. The first behavior causes zero extension into the upper 32-bits. The second behavior splits a 64-bit register into "normal" 32-bit register pairs.

On some passes of ThunderX,
the following registers incorrectly zero extended when they should instead behave as normal 32-bit register pairs:

SMMU()_(S)GFAR
SMMU()_NSGFAR
SMMU()_CB()_TTBR0
SMMU()_CB()_TTBR1
SMMU()_CB()_FAR

Signed-off-by Tirumalesh Chalamarla <[email protected]<mailto:[email protected]>>

---
drivers/iommu/arm-smmu.c | 51 ++++++++++++++++++++++++++++++++++--------------

1 file changed, 36 insertions(+), 15 deletions(-)

[...]

@@ -762,22 +766,39 @@ static void arm_smmu_init_context_bank(struct arm_smmu_domain *smmu_domain,

/* TTBRs */
if (stage1) {
- reg = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[0];
- writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBR0_LO);
- reg = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[0] >> 32;
- reg |= ARM_SMMU_CB_ASID(cfg) << TTBRn_HI_ASID_SHIFT;
- writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBR0_HI); -
- reg = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[1];
- writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBR1_LO);
- reg = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[1] >> 32;
- reg |= ARM_SMMU_CB_ASID(cfg) << TTBRn_HI_ASID_SHIFT;
- writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBR1_HI);
+ if (smmu->options & ARM_SMMU_OPT_64BIT_WRITES_ONLY) {
+ reg64 = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[0];
+ reg64 |= ((u64) ARM_SMMU_CB_ASID(cfg)) <<
+ (TTBRn_HI_ASID_SHIFT + 32);
+ writeq_relaxed(reg, cb_base + ARM_SMMU_CB_TTBR0_LO); +
+ reg64 = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[1];
+ reg64 |= ((u64)ARM_SMMU_CB_ASID(cfg)) <<
+ (TTBRn_HI_ASID_SHIFT + 32);
+ writeq_relaxed(reg, cb_base + ARM_SMMU_CB_TTBR1_LO);
+ } else {
+ reg = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[0];
+ writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBR0_LO);
+ reg = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[0] >> 32;
+ reg |= ARM_SMMU_CB_ASID(cfg) << TTBRn_HI_ASID_SHIFT;
+ writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBR0_HI); +
+ reg = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[1];
+ writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBR1_LO);
+ reg = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[1] >> 32;
+ reg |= ARM_SMMU_CB_ASID(cfg) << TTBRn_HI_ASID_SHIFT;
+ writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBR1_HI);
+ }

I'm fine with making this sort of change if you need it, but this is pretty ugly. Worse, it won't compile for 32-bit ARM.

How about we add a wrapper around these, say smmu_writeq(...), which can then either expand to 2x writel_relaxed or 1x writeq_relaxed depending on CONFIG_64BIT and your erratum workaround?

i think we don’t even need to restrict it for work around.
we can just use CONFIG_64BIT and smmu_writeq.

if thats fine i will repost the patch.

Thanks,
Tirumalesh.
Don't forgot to update the ATOS code too (so you need to write the high word first).

Will

#6 lore raw

PATCH 1/1 iommu/arm-smmu-v2: ThunderX(errata-23399) mis-extends 64bit registers

Quoted 1 line · Tirumalesh

is some thing like this looks good

That's the right sort of idea, but please send a proper patch that you've actually tested. The diff below mixes up reg64 and reg.

Will

Quoted 73 lines
drivers/iommu/arm-smmu.c +21 -20
+#ifdef CONFIG_64BIT
+#define smmu_writeq(reg64, addr)       writeq_relaxed((reg64), (addr))
+#else
+#define smmu_writeq(reg64, addr)                       \
+               writel_relaxed(((reg64) >> 32), ((addr) + 4));  \
+               writel_relaxed((reg64), (addr))
+
+
 /* Configuration registers */
 #define ARM_SMMU_GR0_sCR0              0x0
 #define sCR0_CLIENTPD                  (1 << 0)

@@ -226,7 +234,7 @@
 #define TTBCR2_SEP_SHIFT               15
 #define TTBCR2_SEP_UPSTREAM            (0x7 << TTBCR2_SEP_SHIFT)
 
-#define TTBRn_HI_ASID_SHIFT            16
+#define TTBRn_ASID_SHIFT               48
 
 #define FSR_MULTI                      (1 << 31)
 #define FSR_SS                         (1 << 30)

@@ -719,6 +727,7 @@ static void arm_smmu_init_context_bank(struct arm_smmu_domain *smmu_domain,
                                       struct io_pgtable_cfg *pgtbl_cfg)
 {
        u32 reg;
+       u64 reg64;
        bool stage1;
        struct arm_smmu_cfg *cfg = &smmu_domain->cfg;
        struct arm_smmu_device *smmu = smmu_domain->smmu;

@@ -762,22 +771,16 @@ static void arm_smmu_init_context_bank(struct arm_smmu_domain *smmu_domain,
 
        /* TTBRs */
        if (stage1) {
-               reg = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[0];
-               writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBR0_LO);
-               reg = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[0] >> 32;
-               reg |= ARM_SMMU_CB_ASID(cfg) << TTBRn_HI_ASID_SHIFT;
-               writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBR0_HI);
-
-               reg = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[1];
-               writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBR1_LO);
-               reg = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[1] >> 32;
-               reg |= ARM_SMMU_CB_ASID(cfg) << TTBRn_HI_ASID_SHIFT;
-               writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBR1_HI);
+               reg64 = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[0];
+               reg64 |= ((u64)ARM_SMMU_CB_ASID(cfg)) << TTBRn_ASID_SHIFT;
+               smmu_writeq(reg64, cb_base + ARM_SMMU_CB_TTBR0_LO);
+
+               reg64 = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[1];
+               reg64 |= ARM_SMMU_CB_ASID(cfg) << TTBRn_ASID_SHIFT;
+               smmu_writeq(reg, cb_base + ARM_SMMU_CB_TTBR1_LO);
        } else {
-               reg = pgtbl_cfg->arm_lpae_s2_cfg.vttbr;
-               writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBR0_LO);
-               reg = pgtbl_cfg->arm_lpae_s2_cfg.vttbr >> 32;
-               writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBR0_HI);
+               reg64 = pgtbl_cfg->arm_lpae_s2_cfg.vttbr;
+               smmu_writeq(reg, cb_base + ARM_SMMU_CB_TTBR0_LO);
        }
 
        /* TTBCR */

@@ -1236,10 +1239,8 @@ static phys_addr_t arm_smmu_iova_to_phys_hard(struct iommu_domain *domain,
                u32 reg = iova & ~0xfff;
                writel_relaxed(reg, cb_base + ARM_SMMU_CB_ATS1PR_LO);
        } else {
-               u32 reg = iova & ~0xfff;
-               writel_relaxed(reg, cb_base + ARM_SMMU_CB_ATS1PR_LO);
-               reg = ((u64)iova & ~0xfff) >> 32;
-               writel_relaxed(reg, cb_base + ARM_SMMU_CB_ATS1PR_HI);
+               u64 reg = iova & ~0xfff;
+               smmu_writeq(reg, cb_base + ARM_SMMU_CB_ATS1PR_LO);
        }
 
        if (readl_poll_timeout_atomic(cb_base + ARM_SMMU_CB_ATSR, tmp,
#7 lore raw

PATCH 1/1 iommu/arm-smmu-v2: ThunderX(errata-23399) mis-extends 64bit registers

In reply to Chalamarla, Tirumalesh

Review

Quoted 8 lines · Tirumalesh

is some thing like this looks good

(quoted diff) +6 -0
+#ifdef CONFIG_64BIT
+#define smmu_writeq(reg64, addr)       writeq_relaxed((reg64), (addr))
+#else
+#define smmu_writeq(reg64, addr)                       \
+               writel_relaxed(((reg64) >> 32), ((addr) + 4));  \
+               writel_relaxed((reg64), (addr))

It's missing a #endif.

This also suffers from multiple argument evaluation, and it hides that there's two expressions here - which makes future maintanence harder.

#define smmu_writeq(reg64, addr)					\
	do {								\
		u64 __val = (reg64);					\
		volatile void __iomem *__addr = (addr);			\
		writel_relaxed(__val >> 32, __addr + 4);		\
		writel_relaxed(__val, __addr);				\
	} while (0)

is longer but is much preferred as it won't suffer side effects from stuff like:

	if (...)
		smmu_writeq(val++, addr);
Signature · 2 lines
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
reviewer
#8 lore raw

PATCH 1/1 iommu/arm-smmu-v2: ThunderX(errata-23399) mis-extends 64bit registers

In reply to Will Deacon
Quoted 69 lines · Will Deacon

Hello,

Quoted 21 lines · tchalamarla at caviumnetworks.com

From: Tirumalesh Chalamarla <[email protected]>

The SMMU architecture defines two different behaviors when 64-bit registers are written with 32-bit writes. The first behavior causes zero extension into the upper 32-bits. The second behavior splits a 64-bit register into "normal" 32-bit register pairs.

On some passes of ThunderX,
the following registers incorrectly zero extended when they should instead behave as normal 32-bit register pairs:

   SMMU()_(S)GFAR
   SMMU()_NSGFAR
   SMMU()_CB()_TTBR0
   SMMU()_CB()_TTBR1
   SMMU()_CB()_FAR
Signed-off-by Tirumalesh Chalamarla <[email protected]>
---
  drivers/iommu/arm-smmu.c | 51 ++++++++++++++++++++++++++++++++++--------------
  1 file changed, 36 insertions(+), 15 deletions(-)

[...]

Quoted 28 lines
drivers/iommu/arm-smmu.c +13 -11
@@ -762,22 +766,39 @@ static void arm_smmu_init_context_bank(struct arm_smmu_domain *smmu_domain,
 
  	/* TTBRs */
  	if (stage1) {
-		reg = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[0];
-		writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBR0_LO);
-		reg = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[0] >> 32;
-		reg |= ARM_SMMU_CB_ASID(cfg) << TTBRn_HI_ASID_SHIFT;
-		writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBR0_HI);
-
-		reg = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[1];
-		writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBR1_LO);
-		reg = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[1] >> 32;
-		reg |= ARM_SMMU_CB_ASID(cfg) << TTBRn_HI_ASID_SHIFT;
-		writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBR1_HI);
+		if (smmu->options & ARM_SMMU_OPT_64BIT_WRITES_ONLY) {
+			reg64 = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[0];
+			reg64 |= ((u64) ARM_SMMU_CB_ASID(cfg)) <<
+						(TTBRn_HI_ASID_SHIFT + 32);
+			writeq_relaxed(reg, cb_base + ARM_SMMU_CB_TTBR0_LO);
+
+			reg64 = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[1];
+			reg64 |= ((u64)ARM_SMMU_CB_ASID(cfg)) <<
+						(TTBRn_HI_ASID_SHIFT + 32);
+			writeq_relaxed(reg, cb_base + ARM_SMMU_CB_TTBR1_LO);
+		} else {
+			reg = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[0];
+			writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBR0_LO);

drivers/iommu/arm-smmu.c static void arm_smmu_init_context_bank(struct arm_smmu_domain *smmu_domain, · line 791

Quoted 10 lines
drivers/iommu/arm-smmu.c +10 -0
+			reg = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[0] >> 32;
+			reg |= ARM_SMMU_CB_ASID(cfg) << TTBRn_HI_ASID_SHIFT;
+			writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBR0_HI);
+
+			reg = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[1];
+			writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBR1_LO);
+			reg = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[1] >> 32;
+			reg |= ARM_SMMU_CB_ASID(cfg) << TTBRn_HI_ASID_SHIFT;
+			writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBR1_HI);
+		}

I'm fine with making this sort of change if you need it, but this is pretty ugly. Worse, it won't compile for 32-bit ARM.

Quoted 3 lines

How about we add a wrapper around these, say smmu_writeq(...), which can then either expand to 2x writel_relaxed or 1x writeq_relaxed depending on CONFIG_64BIT and your erratum workaround?

Would we even need a specific erratum workaround then? I don't see an issue with always using writeq on 64-bit, and there's not much we can do for 32-bit if it has no way to write the upper half of the TTBR on this thing.

Robin.

Quoted 10 lines

Don't forgot to update the ATOS code too (so you need to write the high word first).

Will
_______________________________________________
iommu mailing list
iommu at lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu