iommu/arm-smmu-v2: ThunderX mis-extends 64bit registers
PATCH v2 1/1 iommu/arm-smmu-v2: ThunderX mis-extends 64bit registers
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 buggy implementations, registers incorrectly zero extended when they should instead behave as normal 32-bit register pairs.
---
Changes from V1:
- Introduced smmu_writeq
drivers/iommu/arm-smmu.c | 45 +++++++++++++++++++++++++-------------------- 1 file changed, 25 insertions(+), 20 deletions(-)
drivers/iommu/arm-smmu.c +25 -20
@@ -69,6 +69,18 @@ ((smmu->options & ARM_SMMU_OPT_SECURE_CFG_ACCESS) \ ? 0x400 : 0)) +#ifdef CONFIG_64BIT +#define smmu_writeq (reg64, addr) writeq_relaxed((reg64), (addr)) +#else +#define smmu_writeq (reg64, addr) \ + do { \ + u64 __val = (reg64); \ + void __iomem *__addr = (addr); \ + writel_relaxed(__val >> 32, __addr + 4); \ + writel_relaxed(__val, __addr); \ + } while (0) +#endif +/* Configuration registers */ #define ARM_SMMU_GR0_sCR0 0x0#define sCR0_CLIENTPD (1 << 0)
@@ -226,7 +238,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)
@@ -762,22 +774,17 @@ 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 ); +u64 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 |= ((u64 )ARM_SMMU_CB_ASID (cfg)) << TTBRn_ASID_SHIFT; +smmu_writeq (reg64, 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 ); +u64 reg64 = pgtbl_cfg->arm_lpae_s2_cfg.vttbr; +smmu_writeq (reg64, cb_base +ARM_SMMU_CB_TTBR0_LO ); }/* TTBCR */
@@ -1236,10 +1243,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 reg64 = iova & ~0xfff ; +smmu_writeq (reg64, cb_base +ARM_SMMU_CB_ATS1PR_LO ); } if (readl_poll_timeout_atomic(cb_base + ARM_SMMU_CB_ATSR, tmp,
Signature · 1 line
2.1.0
PATCH v2 1/1 iommu/arm-smmu-v2: ThunderX mis-extends 64bit registers
Hi Tirumalesh,
I think this looks pretty good now, just one small comment below.
Quoted 55 lines · Tirumalesh Chalamarla
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 buggy implementations, registers incorrectly zero extended when they should instead behave as normal 32-bit register pairs.
Signed-off-by Tirumalesh Chalamarla <[email protected]>---
Changes from V1:
- Introducedsmmu_writeqdrivers/iommu/arm-smmu.c | 45 +++++++++++++++++++++++++-------------------- 1 file changed, 25 insertions(+), 20 deletions(-)drivers/iommu/arm-smmu.c +13
-3@@ -69,6 +69,18 @@ ((smmu->options & ARM_SMMU_OPT_SECURE_CFG_ACCESS) \ ? 0x400 : 0)) +#ifdef CONFIG_64BIT +#define smmu_writeq (reg64, addr) writeq_relaxed((reg64), (addr)) +#else +#define smmu_writeq (reg64, addr) \ + do { \ + u64 __val = (reg64); \ + void __iomem *__addr = (addr); \ + writel_relaxed(__val >> 32, __addr + 4); \ + writel_relaxed(__val, __addr); \ + } while (0) +#endif +/* Configuration registers */ #define ARM_SMMU_GR0_sCR0 0x0#define sCR0_CLIENTPD (1 << 0)@@ -226,7 +238,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)@@ -762,22 +774,17 @@ 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 );
drivers/iommu/arm-smmu.c static void arm_smmu_init_context_bank(struct arm_smmu_domain *smmu_domain, · line 777
Quoted 10 lines
drivers/iommu/arm-smmu.c +1
-9- 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 ); +u64 reg64 = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[0 ];
Can we move this declaration to the start of the function along with u32 reg, please? It's used in both cases of the if/else block so it might be a tad cleaner.
Will
Robin Murphy
reviewerPATCH v2 1/1 iommu/arm-smmu-v2: ThunderX mis-extends 64bit registers
Quoted 5 lines · Will Deacon
Hi Tirumalesh,
I think this looks pretty good now, just one small comment below.
On Wed, Aug 05, 2015 at 05:54:28PM +0100, Tirumalesh Chalamarla wrote:
[...]
Quoted 17 lines
Quoted 17 lines
drivers/iommu/arm-smmu.c +1
-9-#define TTBRn_HI_ASID_SHIFT 16 +#define TTBRn_ASID_SHIFT 48#define FSR_MULTI (1 << 31)#define FSR_SS (1 << 30)@@ -762,22 +774,17 @@ 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 );
Quoted 8 lines
drivers/iommu/arm-smmu.c static void arm_smmu_init_context_bank(struct arm_smmu_domain *smmu_domain, · line 777
Quoted 4 lines
drivers/iommu/arm-smmu.c +1
-3- 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 ); +u64 reg64 = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[0 ];Can we move this declaration to the start of the function along with u32 reg, please? It's used in both cases of the if/else block so it might be a tad cleaner.
We could also drop the _LO suffix from the relevant #defines (so they match the architectural names) and remove the now-unused _HI versions entirely.
Robin.
Quoted 7 lines
Will
_______________________________________________
iommu mailing list
iommu at lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu