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