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

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

[tchalamarla at caviumnetworks.com]

From: Tirumalesh Chalamarla <tchalamarla@caviumnetworks.com>

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 <tchalamarla@caviumnetworks.com>
---
 drivers/iommu/arm-smmu.c | 51 ++++++++++++++++++++++++++++++++++--------------
 1 file changed, 36 insertions(+), 15 deletions(-)

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 66a803b..7a3cf7f 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -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 */
-- 
2.1.0

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

[Will Deacon]

Hello,

On Thu, Jul 30, 2015 at 06:55:06PM +0100, tchalamarla at caviumnetworks.com wrote:
> From: Tirumalesh Chalamarla <tchalamarla@caviumnetworks.com>
> 
> 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 <tchalamarla@caviumnetworks.com>
> ---
>  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?

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

Will

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

[Robin Murphy]

On 30/07/15 19:45, Will Deacon wrote:
> Hello,
>
> On Thu, Jul 30, 2015 at 06:55:06PM +0100, tchalamarla at caviumnetworks.com wrote:
>> From: Tirumalesh Chalamarla <tchalamarla@caviumnetworks.com>
>>
>> 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 <tchalamarla@caviumnetworks.com>
>> ---
>>   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?

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.

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

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

[Will Deacon]

On Thu, Jul 30, 2015 at 09:54:04PM +0100, Chalamarla, Tirumalesh wrote:
> 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

> +#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,

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

[Russell King - ARM Linux]

On Thu, Jul 30, 2015 at 08:54:04PM +0000, Chalamarla, Tirumalesh wrote:
> 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))

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);

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.