arm-smmu: Implementation and context format differentiation
Subsystems (3)
Robin Murphy
reviewercover arm-smmu: Implementation and context format differentiation
Hi all,
This is almost 3 separate sub-series, but there's enough interdependency and horrid merge conflicts that I've been keeping it all together.
- Patches 1-3 rationalise implementation-specific details and errata, in order to start shovelling in yet more wonderful workarounds.
- Patches 3-6 do a broadly similar thing for context formats, breaking the effective hard-coding to allow it to become a runtime choice. (With the bonus of unblocking short-descriptor support, once I find and revive the hac^Wpatches I was using for that.)
- Patch 7 then takes advantage of the previous changes to support the hilarious raison d'etre of MMU-401, in all its glory.
This is all based on top of the ThunderX patches in Will's iommu/devel branch. It also conflicts somewhat with my per-domain page size series; since this is probably less contentious, I expect I'll optimistically rebase that on top of this one before I post it again.
Robin.
---
Robin Murphy (7):
iommu/arm-smmu: Differentiate specific implementations iommu/arm-smmu: Convert ThunderX workaround to new method iommu/arm-smmu: Work around MMU-500 prefetch errata
io-64-nonatomic: Add relaxed accessor variants iommu/arm-smmu: Tidy up 64-bit/atomic I/O accesses iommu/arm-smmu: Decouple context format from kernel config iommu/arm-smmu: Support SMMUv1 64KB supplement
Documentation/arm64/silicon-errata.txt | 1 + drivers/iommu/arm-smmu.c | 238 +++++++++++++++++++++++---------- include/linux/io-64-nonatomic-hi-lo.h | 25 ++++ include/linux/io-64-nonatomic-lo-hi.h | 25 ++++ 4 files changed, 216 insertions(+), 73 deletions(-)
Signature · 1 line
2.7.3.dirty
Robin Murphy
reviewerPATCH 1/7 iommu/arm-smmu: Differentiate specific implementations
As the inevitable reality of implementation-specific errata workarounds begin to accrue alongside our integration quirk handling, it's about time the driver had a decent way of keeping track. Extend the per-SMMU data so we can identify specific implementations in an efficient and firmware-agnostic manner.
--- drivers/iommu/arm-smmu.c | 33 ++++++++++++++++++++++++++------- 1 file changed, 26 insertions(+), 7 deletions(-)
drivers/iommu/arm-smmu.c +26 -7
@@ -278,6 +278,10 @@ enum arm_smmu_arch_version { ARM_SMMU_V2, }; +enum arm_smmu_implementation { +GENERIC_SMMU , +}; +struct arm_smmu_smr {u8 idx;u16 mask;
@@ -315,6 +319,7 @@ struct arm_smmu_device {#define ARM_SMMU_OPT_SECURE_CFG_ACCESS (1 << 0)u32 options;enum arm_smmu_arch_version version; +enum arm_smmu_implementation model;u32 num_context_banks;u32 num_s2_context_banks;
@@ -1735,13 +1740,24 @@ static int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu)return 0 ; } +struct arm_smmu_match_data { +enum arm_smmu_arch_version version; +enum arm_smmu_implementation model; +}; + +#define ARM_SMMU_MATCH_DATA (name, ver, imp) \ +static struct arm_smmu_match_data name = { .version = ver, .model = imp } + +ARM_SMMU_MATCH_DATA (smmu_generic_v1,ARM_SMMU_V1 ,GENERIC_SMMU ); +ARM_SMMU_MATCH_DATA (smmu_generic_v2,ARM_SMMU_V2 ,GENERIC_SMMU ); +static const struct of_device_id arm_smmu_of_match[] = { - { .compatible ="arm,smmu-v1" , .data = (void *)ARM_SMMU_V1 }, - { .compatible ="arm,smmu-v2" , .data = (void *)ARM_SMMU_V2 }, - { .compatible ="arm,mmu-400" , .data = (void *)ARM_SMMU_V1 }, - { .compatible ="arm,mmu-401" , .data = (void *)ARM_SMMU_V1 }, - { .compatible ="arm,mmu-500" , .data = (void *)ARM_SMMU_V2 }, - { .compatible ="cavium,smmu-v2" , .data = (void *)ARM_SMMU_V2 }, + { .compatible ="arm,smmu-v1" , .data = &smmu_generic_v1 }, + { .compatible ="arm,smmu-v2" , .data = &smmu_generic_v2 }, + { .compatible ="arm,mmu-400" , .data = &smmu_generic_v1 }, + { .compatible ="arm,mmu-401" , .data = &smmu_generic_v1 }, + { .compatible ="arm,mmu-500" , .data = &smmu_generic_v2 }, + { .compatible ="cavium,smmu-v2" , .data = &smmu_generic_v2 }, { }, };MODULE_DEVICE_TABLE (of, arm_smmu_of_match);
@@ -1749,6 +1765,7 @@ MODULE_DEVICE_TABLE(of, arm_smmu_of_match);static int arm_smmu_device_dt_probe (struct platform_device *pdev) {const struct of_device_id *of_id; +const struct arm_smmu_match_data *data;struct resource *res;struct arm_smmu_device *smmu;struct device *dev = &pdev->dev;
@@ -1764,7 +1781,9 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev) smmu->dev = dev; of_id =of_match_node (arm_smmu_of_match, dev->of_node); - smmu->version = (enum arm_smmu_arch_version )of_id->data; + data = of_id->data; + smmu->version = data->version; + smmu->model = data->model; res =platform_get_resource (pdev,IORESOURCE_MEM ,0 ); smmu->base =devm_ioremap_resource (dev, res);
Signature · 1 line
2.7.3.dirty
PATCH 1/7 iommu/arm-smmu: Differentiate specific implementations
Quoted 86 lines · Robin Murphy
As the inevitable reality of implementation-specific errata workarounds begin to accrue alongside our integration quirk handling, it's about time the driver had a decent way of keeping track. Extend the per-SMMU data so we can identify specific implementations in an efficient and firmware-agnostic manner.
Signed-off-by Robin Murphy <[email protected]>---
drivers/iommu/arm-smmu.c| 33 ++++++++++++++++++++++++++-------1 file changed, 26 insertions(+), 7 deletions(-)drivers/iommu/arm-smmu.c +26
-7@@ -278,6 +278,10 @@ enum arm_smmu_arch_version { ARM_SMMU_V2, }; +enum arm_smmu_implementation { +GENERIC_SMMU , +}; +struct arm_smmu_smr {u8 idx;u16 mask;@@ -315,6 +319,7 @@ struct arm_smmu_device {#define ARM_SMMU_OPT_SECURE_CFG_ACCESS (1 << 0)u32 options;enum arm_smmu_arch_version version; +enum arm_smmu_implementation model;u32 num_context_banks;u32 num_s2_context_banks;@@ -1735,13 +1740,24 @@ static int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu)return 0 ; } +struct arm_smmu_match_data { +enum arm_smmu_arch_version version; +enum arm_smmu_implementation model; +}; + +#define ARM_SMMU_MATCH_DATA (name, ver, imp) \ +static struct arm_smmu_match_data name = { .version = ver, .model = imp } + +ARM_SMMU_MATCH_DATA (smmu_generic_v1,ARM_SMMU_V1 ,GENERIC_SMMU ); +ARM_SMMU_MATCH_DATA (smmu_generic_v2,ARM_SMMU_V2 ,GENERIC_SMMU ); +static const struct of_device_id arm_smmu_of_match[] = { - { .compatible ="arm,smmu-v1" , .data = (void *)ARM_SMMU_V1 }, - { .compatible ="arm,smmu-v2" , .data = (void *)ARM_SMMU_V2 }, - { .compatible ="arm,mmu-400" , .data = (void *)ARM_SMMU_V1 }, - { .compatible ="arm,mmu-401" , .data = (void *)ARM_SMMU_V1 }, - { .compatible ="arm,mmu-500" , .data = (void *)ARM_SMMU_V2 }, - { .compatible ="cavium,smmu-v2" , .data = (void *)ARM_SMMU_V2 }, + { .compatible ="arm,smmu-v1" , .data = &smmu_generic_v1 }, + { .compatible ="arm,smmu-v2" , .data = &smmu_generic_v2 }, + { .compatible ="arm,mmu-400" , .data = &smmu_generic_v1 }, + { .compatible ="arm,mmu-401" , .data = &smmu_generic_v1 }, + { .compatible ="arm,mmu-500" , .data = &smmu_generic_v2 }, + { .compatible ="cavium,smmu-v2" , .data = &smmu_generic_v2 }, { }, };MODULE_DEVICE_TABLE (of, arm_smmu_of_match);@@ -1749,6 +1765,7 @@ MODULE_DEVICE_TABLE(of, arm_smmu_of_match);static int arm_smmu_device_dt_probe (struct platform_device *pdev) {const struct of_device_id *of_id; +const struct arm_smmu_match_data *data;struct resource *res;struct arm_smmu_device *smmu;struct device *dev = &pdev->dev;@@ -1764,7 +1781,9 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev) smmu->dev = dev; of_id =of_match_node (arm_smmu_of_match, dev->of_node); - smmu->version = (enum arm_smmu_arch_version )of_id->data; + data = of_id->data; + smmu->version = data->version; + smmu->model = data->model; res =platform_get_resource (pdev,IORESOURCE_MEM ,0 ); smmu->base =devm_ioremap_resource (dev, res);Signature · 1 line
2.7.3.dirty
Looks good to me.
Quoted 1 line
Robin Murphy
reviewerPATCH 2/7 iommu/arm-smmu: Convert ThunderX workaround to new method
In reply to Robin MurphyWith a framework for implementation-specific funtionality in place, the currently-FDT-dependent ThunderX workaround gets to be the first user.
--- drivers/iommu/arm-smmu.c | 27 ++++++++++++++------------- 1 file changed, 14 insertions(+), 13 deletions(-)
drivers/iommu/arm-smmu.c +14 -13
@@ -280,6 +280,7 @@ enum arm_smmu_arch_version {enum arm_smmu_implementation {GENERIC_SMMU , +CAVIUM_SMMUV2 , };struct arm_smmu_smr {
@@ -1686,6 +1687,17 @@ static int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu) }dev_notice (smmu->dev,"\t%u context banks (%u stage-2 only)\n" , smmu->num_context_banks, smmu->num_s2_context_banks); +/* +* Cavium CN88xx erratum #27704. +* Ensure ASID and VMID allocation is unique across all SMMUs in +* the system. +*/ +if (smmu->model ==CAVIUM_SMMUV2 ) { + smmu->cavium_id_base = +atomic_add_return (smmu->num_context_banks, + &cavium_smmu_context_count); + smmu->cavium_id_base -= smmu->num_context_banks; + }/* ID2 */ id =readl_relaxed (gr0_base +ARM_SMMU_GR0_ID2 );
@@ -1750,6 +1762,7 @@ static struct arm_smmu_match_data name = { .version = ver, .model = imp }ARM_SMMU_MATCH_DATA (smmu_generic_v1,ARM_SMMU_V1 ,GENERIC_SMMU );ARM_SMMU_MATCH_DATA (smmu_generic_v2,ARM_SMMU_V2 ,GENERIC_SMMU ); +ARM_SMMU_MATCH_DATA (cavium_smmuv2,ARM_SMMU_V2 ,CAVIUM_SMMUV2 );static const struct of_device_id arm_smmu_of_match[] = { { .compatible ="arm,smmu-v1" , .data = &smmu_generic_v1 },
@@ -1757,7 +1770,7 @@ static const struct of_device_id arm_smmu_of_match[] = { { .compatible = "arm,mmu-400", .data = &smmu_generic_v1 }, { .compatible = "arm,mmu-401", .data = &smmu_generic_v1 }, { .compatible = "arm,mmu-500", .data = &smmu_generic_v2 }, - { .compatible = "cavium,smmu-v2", .data = &smmu_generic_v2 }, + { .compatible = "cavium,smmu-v2", .data = &cavium_smmuv2 }, { }, };MODULE_DEVICE_TABLE (of, arm_smmu_of_match);
@@ -1871,18 +1884,6 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev) } } -/* -* Cavium CN88xx erratum #27704. -* Ensure ASID and VMID allocation is unique across all SMMUs in -* the system. -*/ -if (of_device_is_compatible (dev->of_node,"cavium,smmu-v2" )) { - smmu->cavium_id_base = -atomic_add_return (smmu->num_context_banks, - &cavium_smmu_context_count); - smmu->cavium_id_base -= smmu->num_context_banks; - } -INIT_LIST_HEAD (&smmu->list);spin_lock (&arm_smmu_devices_lock);list_add (&smmu->list, &arm_smmu_devices);
Signature · 1 line
2.7.3.dirty
PATCH 2/7 iommu/arm-smmu: Convert ThunderX workaround to new method
Quoted 76 lines · Robin Murphy
With a framework for implementation-specific funtionality in place, the currently-FDT-dependent ThunderX workaround gets to be the first user.
Signed-off-by Robin Murphy <[email protected]>---
drivers/iommu/arm-smmu.c| 27 ++++++++++++++-------------1 file changed, 14 insertions(+), 13 deletions(-)drivers/iommu/arm-smmu.c +14
-13@@ -280,6 +280,7 @@ enum arm_smmu_arch_version {enum arm_smmu_implementation {GENERIC_SMMU , +CAVIUM_SMMUV2 , };struct arm_smmu_smr {@@ -1686,6 +1687,17 @@ static int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu) }dev_notice (smmu->dev,"\t%u context banks (%u stage-2 only)\n" , smmu->num_context_banks, smmu->num_s2_context_banks); +/* +* Cavium CN88xx erratum #27704. +* Ensure ASID and VMID allocation is unique across all SMMUs in +* the system. +*/ +if (smmu->model ==CAVIUM_SMMUV2 ) { + smmu->cavium_id_base = +atomic_add_return (smmu->num_context_banks, + &cavium_smmu_context_count); + smmu->cavium_id_base -= smmu->num_context_banks; + }/* ID2 */ id =readl_relaxed (gr0_base +ARM_SMMU_GR0_ID2 );@@ -1750,6 +1762,7 @@ static struct arm_smmu_match_data name = { .version = ver, .model = imp }ARM_SMMU_MATCH_DATA (smmu_generic_v1,ARM_SMMU_V1 ,GENERIC_SMMU );ARM_SMMU_MATCH_DATA (smmu_generic_v2,ARM_SMMU_V2 ,GENERIC_SMMU ); +ARM_SMMU_MATCH_DATA (cavium_smmuv2,ARM_SMMU_V2 ,CAVIUM_SMMUV2 );static const struct of_device_id arm_smmu_of_match[] = { { .compatible ="arm,smmu-v1" , .data = &smmu_generic_v1 },@@ -1757,7 +1770,7 @@ static const struct of_device_id arm_smmu_of_match[] = { { .compatible = "arm,mmu-400", .data = &smmu_generic_v1 }, { .compatible = "arm,mmu-401", .data = &smmu_generic_v1 }, { .compatible = "arm,mmu-500", .data = &smmu_generic_v2 }, - { .compatible = "cavium,smmu-v2", .data = &smmu_generic_v2 }, + { .compatible = "cavium,smmu-v2", .data = &cavium_smmuv2 }, { }, };MODULE_DEVICE_TABLE (of, arm_smmu_of_match);@@ -1871,18 +1884,6 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev) } } -/* -* Cavium CN88xx erratum #27704. -* Ensure ASID and VMID allocation is unique across all SMMUs in -* the system. -*/ -if (of_device_is_compatible (dev->of_node,"cavium,smmu-v2" )) { - smmu->cavium_id_base = -atomic_add_return (smmu->num_context_banks, - &cavium_smmu_context_count); - smmu->cavium_id_base -= smmu->num_context_banks; - } -INIT_LIST_HEAD (&smmu->list);spin_lock (&arm_smmu_devices_lock);list_add (&smmu->list, &arm_smmu_devices);Signature · 1 line
2.7.3.dirty
This looks fine too.
Quoted 1 line
Robin Murphy
reviewerPATCH 3/7 iommu/arm-smmu: Work around MMU-500 prefetch errata
In reply to Robin MurphyMMU-500 erratum #841119 is tickled by a particular set of circumstances interacting with the next-page prefetcher. Since said prefetcher is quite dumb and actually detrimental to performance in some cases (by causing unwanted TLB evictions for non-sequential access patterns), we lose very little by turning it off, and what we gain is a guarantee that the erratum is never hit.
As a bonus, the same workaround will also prevent erratum #826419 once v7 short descriptor support is implemented.
--- Documentation/arm64/silicon-errata.txt | 1 + drivers/iommu/arm-smmu.c | 16 +++++++++++++++- 2 files changed, 16 insertions(+), 1 deletion(-)
Documentation/arm64/silicon-errata.txt +1 -0
@@ -53,6 +53,7 @@ stable kernels. | ARM | Cortex-A57 | #832075 | ARM64_ERRATUM_832075 | | ARM | Cortex-A57 | #852523 | N/A | | ARM | Cortex-A57 | #834220 | ARM64_ERRATUM_834220 | +| ARM | MMU-500 | #841119,#826419 | N/A | | | | | | | Cavium | ThunderX ITS | #22375, #24313 | CAVIUM_ERRATUM_22375 | | Cavium | ThunderX GICv3 | #23154 | CAVIUM_ERRATUM_23154 |
drivers/iommu/arm-smmu.c +15 -1
@@ -203,6 +203,7 @@#define ARM_SMMU_CB (smmu, n) ((n) * (1 << (smmu)->pgshift))#define ARM_SMMU_CB_SCTLR 0x0 +#define ARM_SMMU_CB_ACTLR 0x4#define ARM_SMMU_CB_RESUME 0x8#define ARM_SMMU_CB_TTBCR2 0x10#define ARM_SMMU_CB_TTBR0 0x20
@@ -234,6 +235,8 @@#define SCTLR_M (1 << 0)#define SCTLR_EAE_SBOP (SCTLR_AFE | SCTLR_TRE) +#define ARM_MMU500_ACTLR_CPRE (1 << 1) +#define CB_PAR_F (1 << 0)#define ATSR_ACTIVE (1 << 0)
@@ -280,6 +283,7 @@ enum arm_smmu_arch_version {enum arm_smmu_implementation {GENERIC_SMMU , +ARM_MMU500 ,CAVIUM_SMMUV2 , };
@@ -1517,6 +1521,15 @@ static void arm_smmu_device_reset(struct arm_smmu_device *smmu) cb_base =ARM_SMMU_CB_BASE (smmu) +ARM_SMMU_CB (smmu, i);writel_relaxed (0 , cb_base +ARM_SMMU_CB_SCTLR );writel_relaxed (FSR_FAULT , cb_base +ARM_SMMU_CB_FSR ); +/* +* Disable MMU-500's not-particularly-beneficial next-page +* prefetcher for the sake of errata #841119 and #826419. +*/ +if (smmu->model ==ARM_MMU500 ) { + reg =readl_relaxed (cb_base +ARM_SMMU_CB_ACTLR ); + reg &= ~ARM_MMU500_ACTLR_CPRE ; +writel_relaxed (reg, cb_base +ARM_SMMU_CB_ACTLR ); + } }/* Invalidate the TLB, just in case */
@@ -1762,6 +1775,7 @@ static struct arm_smmu_match_data name = { .version = ver, .model = imp }ARM_SMMU_MATCH_DATA (smmu_generic_v1,ARM_SMMU_V1 ,GENERIC_SMMU );ARM_SMMU_MATCH_DATA (smmu_generic_v2,ARM_SMMU_V2 ,GENERIC_SMMU ); +ARM_SMMU_MATCH_DATA (arm_mmu500,ARM_SMMU_V2 ,ARM_MMU500 );ARM_SMMU_MATCH_DATA (cavium_smmuv2,ARM_SMMU_V2 ,CAVIUM_SMMUV2 );static const struct of_device_id arm_smmu_of_match[] = {
@@ -1769,7 +1783,7 @@ static const struct of_device_id arm_smmu_of_match[] = { { .compatible = "arm,smmu-v2", .data = &smmu_generic_v2 }, { .compatible = "arm,mmu-400", .data = &smmu_generic_v1 }, { .compatible = "arm,mmu-401", .data = &smmu_generic_v1 }, - { .compatible = "arm,mmu-500", .data = &smmu_generic_v2 }, + { .compatible = "arm,mmu-500", .data = &arm_mmu500 }, { .compatible = "cavium,smmu-v2", .data = &cavium_smmuv2 }, { }, };
Signature · 1 line
2.7.3.dirty
PATCH 3/7 iommu/arm-smmu: Work around MMU-500 prefetch errata
Quoted 10 lines · Robin Murphy
MMU-500 erratum #841119 is tickled by a particular set of circumstances interacting with the next-page prefetcher. Since said prefetcher is quite dumb and actually detrimental to performance in some cases (by causing unwanted TLB evictions for non-sequential access patterns), we lose very little by turning it off, and what we gain is a guarantee that the erratum is never hit.
As a bonus, the same workaround will also prevent erratum #826419 once v7 short descriptor support is implemented.
Quoted 7 lines
CC Catalin Marinas <[email protected]>CC Will Deacon <[email protected]>Signed-off-by Robin Murphy <[email protected]>--- Documentation/arm64/silicon-errata.txt | 1 + drivers/iommu/arm-smmu.c | 16 +++++++++++++++- 2 files changed, 16 insertions(+), 1 deletion(-)
Looks fine, but can you update silicon-errata.txt too, please? It's a handy reference for "what is Linux actively working around" and this one is somewhat hidden away.
Will
PATCH 3/7 iommu/arm-smmu: Work around MMU-500 prefetch errata
In reply to Robin MurphyQuoted 23 lines · Robin Murphy
MMU-500 erratum #841119 is tickled by a particular set of circumstances interacting with the next-page prefetcher. Since said prefetcher is quite dumb and actually detrimental to performance in some cases (by causing unwanted TLB evictions for non-sequential access patterns), we lose very little by turning it off, and what we gain is a guarantee that the erratum is never hit.
As a bonus, the same workaround will also prevent erratum #826419 once v7 short descriptor support is implemented.
CC Catalin Marinas <[email protected]>CC Will Deacon <[email protected]>Signed-off-by Robin Murphy <[email protected]>--- Documentation/arm64/silicon-errata.txt | 1 + drivers/iommu/arm-smmu.c | 16 +++++++++++++++- 2 files changed, 16 insertions(+), 1 deletion(-)Documentation/arm64/silicon-errata.txt +0
-0
Documentation/arm64/silicon-errata.txt stable kernels. · line 56
Quoted 4 lines
Documentation/arm64/silicon-errata.txt +1
-0@@ -53,6 +53,7 @@ stable kernels. | ARM | Cortex-A57 | #832075 | ARM64_ERRATUM_832075 | | ARM | Cortex-A57 | #852523 | N/A | | ARM | Cortex-A57 | #834220 | ARM64_ERRATUM_834220 | +| ARM | MMU-500 | #841119,#826419 | N/A |
Found it :)
Will
Robin Murphy
reviewerPATCH 4/7 io-64-nonatomic: Add relaxed accessor variants
In reply to Robin MurphyWhilst commit 9439eb3ab9d1 ("asm-generic: io: implement relaxed accessor macros as conditional wrappers") makes the *_relaxed forms of I/O accessors universally available to drivers, in cases where writeq() is implemented via the io-64-nonatomic helpers, writeq_relaxed() will end up falling back to writel() regardless of whether writel_relaxed() is available (identically for s/write/read/).
Add corresponding relaxed forms of the nonatomic helpers to delegate to the equivalent 32-bit accessors as appropriate.
--- include/linux/io-64-nonatomic-hi-lo.h | 25 +++++++++++++++++++++++++ include/linux/io-64-nonatomic-lo-hi.h | 25 +++++++++++++++++++++++++ 2 files changed, 50 insertions(+)
include/linux/io-64-nonatomic-hi-lo.h +25 -0
@@ -21,6 +21,23 @@ static inline void hi_lo_writeq(__u64 val, volatile void __iomem *addr)writel (val, addr); } +static inline __u64 hi_lo_readq_relaxed (const volatile void __iomem *addr) +{ +const volatile u32 __iomem *p = addr; +u32 low, high; + + high =readl_relaxed (p +1 ); + low =readl_relaxed (p); + +return low + ((u64 )high <<32 ); +} + +static inline void hi_lo_writeq_relaxed (__u64 val,volatile void __iomem *addr) +{ +writel_relaxed (val >>32 , addr +4 ); +writel_relaxed (val, addr); +} +#ifndef readq#define readq hi_lo_readq#endif
@@ -29,4 +46,12 @@ static inline void hi_lo_writeq(__u64 val, volatile void __iomem *addr)#define writeq hi_lo_writeq#endif +#ifndef readq_relaxed +#define readq_relaxed hi_lo_readq_relaxed +#endif + +#ifndef writeq_relaxed +#define writeq hi_lo_writeq_relaxed +#endif +#endif /* _LINUX_IO_64_NONATOMIC_HI_LO_H_ */
include/linux/io-64-nonatomic-lo-hi.h +25 -0
@@ -21,6 +21,23 @@ static inline void lo_hi_writeq(__u64 val, volatile void __iomem *addr)writel (val >>32 , addr +4 ); } +static inline __u64 lo_hi_readq_relaxed (const volatile void __iomem *addr) +{ +const volatile u32 __iomem *p = addr; +u32 low, high; + + low =readl_relaxed (p); + high =readl_relaxed (p +1 ); + +return low + ((u64 )high <<32 ); +} + +static inline void lo_hi_writeq_relaxed (__u64 val,volatile void __iomem *addr) +{ +writel_relaxed (val, addr); +writel_relaxed (val >>32 , addr +4 ); +} +#ifndef readq#define readq lo_hi_readq#endif
@@ -29,4 +46,12 @@ static inline void lo_hi_writeq(__u64 val, volatile void __iomem *addr)#define writeq lo_hi_writeq#endif +#ifndef readq_relaxed +#define readq_relaxed hi_lo_readq_relaxed +#endif + +#ifndef writeq_relaxed +#define writeq hi_lo_writeq_relaxed +#endif +#endif /* _LINUX_IO_64_NONATOMIC_LO_HI_H_ */
Signature · 1 line
2.7.3.dirty
PATCH 4/7 io-64-nonatomic: Add relaxed accessor variants
Quoted 34 lines · Robin Murphy
Whilst commit
9439eb3ab9d1("asm-generic: io: implement relaxed accessor macros as conditional wrappers") makes the*_relaxedforms of I/O accessors universally available to drivers, in cases wherewriteq()is implemented via the io-64-nonatomic helpers,writeq_relaxed()will end up falling back towritel()regardless of whetherwritel_relaxed()is available (identically for s/write/read/).Add corresponding relaxed forms of the nonatomic helpers to delegate to the equivalent 32-bit accessors as appropriate.
CC Arnd Bergmann <[email protected]>CC Christoph Hellwig <[email protected]>CC Darren Hart <[email protected]>CC Hitoshi Mitake <[email protected]>Signed-off-by Robin Murphy <[email protected]>--- include/linux/io-64-nonatomic-hi-lo.h | 25 +++++++++++++++++++++++++ include/linux/io-64-nonatomic-lo-hi.h | 25 +++++++++++++++++++++++++ 2 files changed, 50 insertions(+)include/linux/io-64-nonatomic-hi-lo.h +6
-0@@ -21,6 +21,23 @@ static inline void hi_lo_writeq(__u64 val, volatile void __iomem *addr)writel (val, addr); } +static inline __u64 hi_lo_readq_relaxed (const volatile void __iomem *addr) +{ +const volatile u32 __iomem *p = addr; +u32 low, high; + + high =readl_relaxed (p +1 );
include/linux/io-64-nonatomic-hi-lo.h static inline void hi_lo_writeq(__u64 val, volatile void __iomem *addr) · line 39
Quoted 10 lines
include/linux/io-64-nonatomic-hi-lo.h +10
-0+ low =readl_relaxed (p); + +return low + ((u64 )high <<32 ); +} + +static inline void hi_lo_writeq_relaxed (__u64 val,volatile void __iomem *addr) +{ +writel_relaxed (val >>32 , addr +4 ); +writel_relaxed (val, addr); +}
Could we not generate the _relaxed variants with some macro magic?
Will
Robin Murphy
reviewerPATCH 4/7 io-64-nonatomic: Add relaxed accessor variants
Quoted 39 lines · Will Deacon
Quoted 38 lines · Robin Murphy
Whilst commit
9439eb3ab9d1("asm-generic: io: implement relaxed accessor macros as conditional wrappers") makes the*_relaxedforms of I/O accessors universally available to drivers, in cases wherewriteq()is implemented via the io-64-nonatomic helpers,writeq_relaxed()will end up falling back towritel()regardless of whetherwritel_relaxed()is available (identically for s/write/read/).Add corresponding relaxed forms of the nonatomic helpers to delegate to the equivalent 32-bit accessors as appropriate.
CC Arnd Bergmann <[email protected]>CC Christoph Hellwig <[email protected]>CC Darren Hart <[email protected]>CC Hitoshi Mitake <[email protected]>Signed-off-by Robin Murphy <[email protected]>--- include/linux/io-64-nonatomic-hi-lo.h | 25 +++++++++++++++++++++++++ include/linux/io-64-nonatomic-lo-hi.h | 25 +++++++++++++++++++++++++ 2 files changed, 50 insertions(+)include/linux/io-64-nonatomic-hi-lo.h +10
-0@@ -21,6 +21,23 @@ static inline void hi_lo_writeq(__u64 val, volatile void __iomem *addr)writel (val, addr); } +static inline __u64 hi_lo_readq_relaxed (const volatile void __iomem *addr) +{ +const volatile u32 __iomem *p = addr; +u32 low, high; + + high =readl_relaxed (p +1 ); + low =readl_relaxed (p); + +return low + ((u64 )high <<32 ); +}
Quoted 8 lines
include/linux/io-64-nonatomic-hi-lo.h static inline void hi_lo_writeq(__u64 val, volatile void __iomem *addr) · line 39
Quoted 6 lines
include/linux/io-64-nonatomic-hi-lo.h +6
-0+ +static inline void hi_lo_writeq_relaxed (__u64 val,volatile void __iomem *addr) +{ +writel_relaxed (val >>32 , addr +4 ); +writel_relaxed (val, addr); +}Could we not generate the
_relaxedvariants with some macro magic?
We _could_ - indeed I started doing that, but then decided that the obfuscation of horrible macro-templated functions wasn't worth saving a couple of hundred bytes in some code that isn't exactly difficult to maintain and has needed touching once in 4 years.
If you did want to go down the macro route, I may as well also generate both lo-hi and hi-lo headers all from a single template, it'd be really clever... <alarm bells> ;)
Robin.
Quoted 3 lines
Will
PATCH 4/7 io-64-nonatomic: Add relaxed accessor variants
Quoted 49 lines · Robin Murphy
Quoted 39 lines · Will Deacon
Quoted 38 lines · Robin Murphy
Whilst commit
9439eb3ab9d1("asm-generic: io: implement relaxed accessor macros as conditional wrappers") makes the*_relaxedforms of I/O accessors universally available to drivers, in cases wherewriteq()is implemented via the io-64-nonatomic helpers,writeq_relaxed()will end up falling back towritel()regardless of whetherwritel_relaxed()is available (identically for s/write/read/).Add corresponding relaxed forms of the nonatomic helpers to delegate to the equivalent 32-bit accessors as appropriate.
CC Arnd Bergmann <[email protected]>CC Christoph Hellwig <[email protected]>CC Darren Hart <[email protected]>CC Hitoshi Mitake <[email protected]>Signed-off-by Robin Murphy <[email protected]>--- include/linux/io-64-nonatomic-hi-lo.h | 25 +++++++++++++++++++++++++ include/linux/io-64-nonatomic-lo-hi.h | 25 +++++++++++++++++++++++++ 2 files changed, 50 insertions(+)include/linux/io-64-nonatomic-hi-lo.h +10
-0@@ -21,6 +21,23 @@ static inline void hi_lo_writeq(__u64 val, volatile void __iomem *addr)writel (val, addr); } +static inline __u64 hi_lo_readq_relaxed (const volatile void __iomem *addr) +{ +const volatile u32 __iomem *p = addr; +u32 low, high; + + high =readl_relaxed (p +1 ); + low =readl_relaxed (p); + +return low + ((u64 )high <<32 ); +}Quoted 8 lines
include/linux/io-64-nonatomic-hi-lo.h static inline void hi_lo_writeq(__u64 val, volatile void __iomem *addr) · line 39
Quoted 6 lines
include/linux/io-64-nonatomic-hi-lo.h +6
-0+ +static inline void hi_lo_writeq_relaxed (__u64 val,volatile void __iomem *addr) +{ +writel_relaxed (val >>32 , addr +4 ); +writel_relaxed (val, addr); +}Could we not generate the
_relaxedvariants with some macro magic?
Quoted 8 lines
We _could_ - indeed I started doing that, but then decided that the obfuscation of horrible macro-templated functions wasn't worth saving a couple of hundred bytes in some code that isn't exactly difficult to maintain and has needed touching once in 4 years.
If you did want to go down the macro route, I may as well also generate both lo-hi and hi-lo headers all from a single template, it'd be really clever... <alarm bells> ;)
I certainly wasn't suggesting any more than the obvious macroisation, but I'll leave it up to Arnd, as I think this falls on his lap.
Will
Arnd Bergmann
maintainerPATCH 4/7 io-64-nonatomic: Add relaxed accessor variants
Quoted 25 lines · On Monday 25 April 2016 14:32:42 Will Deacon wrote:
Quoted 25 lines
Quoted 11 lines
Quoted 11 lines
include/linux/io-64-nonatomic-hi-lo.h +10
-0+static inline __u64 hi_lo_readq_relaxed (const volatile void __iomem *addr) +{ +const volatile u32 __iomem *p = addr; +u32 low, high; + + high =readl_relaxed (p +1 ); + low =readl_relaxed (p); + +return low + ((u64 )high <<32 ); +}Quoted 8 lines
include/linux/io-64-nonatomic-hi-lo.h static inline void hi_lo_writeq(__u64 val, volatile void __iomem *addr) · line 39
Quoted 6 lines
include/linux/io-64-nonatomic-hi-lo.h +6
-0+ +static inline void hi_lo_writeq_relaxed (__u64 val,volatile void __iomem *addr) +{ +writel_relaxed (val >>32 , addr +4 ); +writel_relaxed (val, addr); +}Could we not generate the
_relaxedvariants with some macro magic?We _could_ - indeed I started doing that, but then decided that the obfuscation of horrible macro-templated functions wasn't worth saving a couple of hundred bytes in some code that isn't exactly difficult to maintain and has needed touching once in 4 years.
Quoted 6 lines
Quoted 3 lines
If you did want to go down the macro route, I may as well also generate both lo-hi and hi-lo headers all from a single template, it'd be really clever... <alarm bells>
I certainly wasn't suggesting any more than the obvious macroisation, but I'll leave it up to Arnd, as I think this falls on his lap.
I'd prefer the open-coded variant as well.
Arnd
Robin Murphy
reviewerPATCH 4/7 io-64-nonatomic: Add relaxed accessor variants
Hi Arnd,
Quoted 26 lines · Arnd Bergmann
Quoted 25 lines · On Monday 25 April 2016 14:32:42 Will Deacon wrote:
Quoted 25 lines
Quoted 11 lines
Quoted 11 lines
include/linux/io-64-nonatomic-hi-lo.h +10
-0+static inline __u64 hi_lo_readq_relaxed (const volatile void __iomem *addr) +{ +const volatile u32 __iomem *p = addr; +u32 low, high; + + high =readl_relaxed (p +1 ); + low =readl_relaxed (p); + +return low + ((u64 )high <<32 ); +}Quoted 8 lines
include/linux/io-64-nonatomic-hi-lo.h static inline void hi_lo_writeq(__u64 val, volatile void __iomem *addr) · line 39
Quoted 6 lines
include/linux/io-64-nonatomic-hi-lo.h +6
-0+ +static inline void hi_lo_writeq_relaxed (__u64 val,volatile void __iomem *addr) +{ +writel_relaxed (val >>32 , addr +4 ); +writel_relaxed (val, addr); +}Could we not generate the
_relaxedvariants with some macro magic?We _could_ - indeed I started doing that, but then decided that the obfuscation of horrible macro-templated functions wasn't worth saving a couple of hundred bytes in some code that isn't exactly difficult to maintain and has needed touching once in 4 years.
Quoted 8 lines
Quoted 6 lines
Quoted 3 lines
If you did want to go down the macro route, I may as well also generate both lo-hi and hi-lo headers all from a single template, it'd be really clever... <alarm bells>
I certainly wasn't suggesting any more than the obvious macroisation, but I'll leave it up to Arnd, as I think this falls on his lap.
I'd prefer the open-coded variant as well.
By that, do you mean sticking with the smmu_writeq() implementation in the driver and dropping this patch, or merging this patch as-is without further macro-magic?
Thanks,
Robin.
Quoted 3 lines
Arnd
Arnd Bergmann
maintainerPATCH 4/7 io-64-nonatomic: Add relaxed accessor variants
Quoted 10 lines · On Monday 25 April 2016 16:28:01 Robin Murphy wrote:
Quoted 10 lines
Quoted 10 lines
Quoted 9 lines
We _could_ - indeed I started doing that, but then decided that the obfuscation of horrible macro-templated functions wasn't worth saving a couple of hundred bytes in some code that isn't exactly difficult to maintain and has needed touching once in 4 years.
If you did want to go down the macro route, I may as well also generate both lo-hi and hi-lo headers all from a single template, it'd be really clever... <alarm bells>
Quoted 9 lines
Quoted 4 lines
Quoted 2 lines
I certainly wasn't suggesting any more than the obvious macroisation, but I'll leave it up to Arnd, as I think this falls on his lap.
I'd prefer the open-coded variant as well.
By that, do you mean sticking with the
smmu_writeq()implementation in the driver and dropping this patch, or merging this patch as-is without further macro-magic?
Sorry, that was really ambiguous on my end. I meant leaving patch 4/7 as it is in the version you posted.
However, leaving the open-coded writel_relaxed() in the driver or just using the non-relaxed hi_lo_readq() would be totally fine too.
Arnd
PATCH 4/7 io-64-nonatomic: Add relaxed accessor variants
Quoted 16 lines · Arnd Bergmann
Quoted 15 lines · On Monday 25 April 2016 16:28:01 Robin Murphy wrote:
Quoted 14 lines
Quoted 6 lines
Quoted 6 lines
We _could_ - indeed I started doing that, but then decided that the obfuscation of horrible macro-templated functions wasn't worth saving a couple of hundred bytes in some code that isn't exactly difficult to maintain and has needed touching once in 4 years.
Quoted 6 lines
Quoted 3 lines
If you did want to go down the macro route, I may as well also generate both lo-hi and hi-lo headers all from a single template, it'd be really clever... <alarm bells>
I certainly wasn't suggesting any more than the obvious macroisation, but I'll leave it up to Arnd, as I think this falls on his lap.
I'd prefer the open-coded variant as well.
Quoted 7 lines
Quoted 4 lines
By that, do you mean sticking with the
smmu_writeq()implementation in the driver and dropping this patch, or merging this patch as-is without further macro-magic?Sorry, that was really ambiguous on my end. I meant leaving patch 4/7 as it is in the version you posted.
I'm happy with that. Could I have your ack, so that I can queue it with the related SMMU patches, please?
Will
Arnd Bergmann
maintainerPATCH 4/7 io-64-nonatomic: Add relaxed accessor variants
In reply to Robin MurphyQuoted 10 lines · On Wednesday 13 April 2016 18:13:00 Robin Murphy wrote:
Whilst commit
9439eb3ab9d1("asm-generic: io: implement relaxed accessor macros as conditional wrappers") makes the*_relaxedforms of I/O accessors universally available to drivers, in cases wherewriteq()is implemented via the io-64-nonatomic helpers,writeq_relaxed()will end up falling back towritel()regardless of whetherwritel_relaxed()is available (identically for s/write/read/).Add corresponding relaxed forms of the nonatomic helpers to delegate to the equivalent 32-bit accessors as appropriate.
Quoted 7 lines
CC Arnd Bergmann <[email protected]>CC Christoph Hellwig <[email protected]>CC Darren Hart <[email protected]>CC Hitoshi Mitake <[email protected]>Signed-off-by Robin Murphy <[email protected]>---
Robin Murphy
reviewerPATCH v2 1/1 io-64-nonatomic: Add relaxed accessor variants
In reply to Robin MurphyWhilst commit 9439eb3ab9d1 ("asm-generic: io: implement relaxed accessor macros as conditional wrappers") makes the *_relaxed forms of I/O accessors universally available to drivers, in cases where writeq() is implemented via the io-64-nonatomic helpers, writeq_relaxed() will end up falling back to writel() regardless of whether writel_relaxed() is available (identically for s/write/read/).
Add corresponding relaxed forms of the nonatomic helpers to delegate to the equivalent 32-bit accessors as appropriate. We also need to fix io.h to avoid defining default relaxed variants if the basic accessors themselves don't exist.
---
Changes since v1:
- Fix the embarassing copy-paste errors which we all managed to miss.
- Fix io.h so that the nonatomic relaxed definitions are actually used.
Arnd, I've taken the liberty of leaving your ack in place for the additional io.h tweak - please yell at me if that's not OK.
Robin.
include/asm-generic/io.h | 4 ++-- include/linux/io-64-nonatomic-hi-lo.h | 25 +++++++++++++++++++++++++ include/linux/io-64-nonatomic-lo-hi.h | 25 +++++++++++++++++++++++++ 3 files changed, 52 insertions(+), 2 deletions(-)
include/asm-generic/io.h +2 -2
@@ -191,7 +191,7 @@ static inline void writeq(u64 value, volatile void __iomem *addr)#define readl_relaxed readl#endif -#ifndef readq_relaxed +#if defined(readq) && !defined(readq_relaxed)#define readq_relaxed readq#endif
@@ -207,7 +207,7 @@ static inline void writeq(u64 value, volatile void __iomem *addr)#define writel_relaxed writel#endif -#ifndef writeq_relaxed +#if defined(writeq) && !defined(writeq_relaxed)#define writeq_relaxed writeq#endif
include/linux/io-64-nonatomic-hi-lo.h +25 -0
@@ -21,6 +21,23 @@ static inline void hi_lo_writeq(__u64 val, volatile void __iomem *addr)writel (val, addr); } +static inline __u64 hi_lo_readq_relaxed (const volatile void __iomem *addr) +{ +const volatile u32 __iomem *p = addr; +u32 low, high; + + high =readl_relaxed (p +1 ); + low =readl_relaxed (p); + +return low + ((u64 )high <<32 ); +} + +static inline void hi_lo_writeq_relaxed (__u64 val,volatile void __iomem *addr) +{ +writel_relaxed (val >>32 , addr +4 ); +writel_relaxed (val, addr); +} +#ifndef readq#define readq hi_lo_readq#endif
@@ -29,4 +46,12 @@ static inline void hi_lo_writeq(__u64 val, volatile void __iomem *addr)#define writeq hi_lo_writeq#endif +#ifndef readq_relaxed +#define readq_relaxed hi_lo_readq_relaxed +#endif + +#ifndef writeq_relaxed +#define writeq_relaxed hi_lo_writeq_relaxed +#endif +#endif /* _LINUX_IO_64_NONATOMIC_HI_LO_H_ */
include/linux/io-64-nonatomic-lo-hi.h +25 -0
@@ -21,6 +21,23 @@ static inline void lo_hi_writeq(__u64 val, volatile void __iomem *addr)writel (val >>32 , addr +4 ); } +static inline __u64 lo_hi_readq_relaxed (const volatile void __iomem *addr) +{ +const volatile u32 __iomem *p = addr; +u32 low, high; + + low =readl_relaxed (p); + high =readl_relaxed (p +1 ); + +return low + ((u64 )high <<32 ); +} + +static inline void lo_hi_writeq_relaxed (__u64 val,volatile void __iomem *addr) +{ +writel_relaxed (val, addr); +writel_relaxed (val >>32 , addr +4 ); +} +#ifndef readq#define readq lo_hi_readq#endif
@@ -29,4 +46,12 @@ static inline void lo_hi_writeq(__u64 val, volatile void __iomem *addr)#define writeq lo_hi_writeq#endif +#ifndef readq_relaxed +#define readq_relaxed lo_hi_readq_relaxed +#endif + +#ifndef writeq_relaxed +#define writeq_relaxed lo_hi_writeq_relaxed +#endif +#endif /* _LINUX_IO_64_NONATOMIC_LO_HI_H_ */
Signature · 1 line
2.8.1.dirty
Robin Murphy
reviewerPATCH 5/7 iommu/arm-smmu: Tidy up 64-bit/atomic I/O accesses
In reply to Robin MurphyWith {read,write}q_relaxed now able to fall back to the common nonatomic-hi-lo helper, make use of that so that we don't have to open-code our own. In the process, also convert the other remaining split accesses, and repurpose the custom accessor to smooth out the couple of troublesome instances where we really want to avoid nonatomic writes (and a 64-bit access is unnecessary in the 32-bit context formats we would use on a 32-bit CPU).
This paves the way for getting rid of some of the assumptions currently baked into the driver which make it really awkward to use 32-bit context formats with SMMUv2 under a 64-bit kernel.
--- drivers/iommu/arm-smmu.c | 48 +++++++++++++++++++----------------------------- 1 file changed, 19 insertions(+), 29 deletions(-)
drivers/iommu/arm-smmu.c +19 -29
@@ -34,6 +34,7 @@#include <linux/err.h> #include <linux/interrupt.h> #include <linux/io.h> +#include <linux/io-64-nonatomic-hi-lo.h> #include <linux/iommu.h> #include <linux/iopoll.h> #include <linux/module.h>
@@ -71,16 +72,15 @@ ((smmu->options & ARM_SMMU_OPT_SECURE_CFG_ACCESS) \ ? 0x400 : 0)) +/* +* Some 64-bit registers only make sense to write atomically, but in such +* cases all the data relevant to AArch32 formats lies within the lower word, +* therefore this actually makes more sense than it might first appear. +*/ #ifdef CONFIG_64BIT -#define smmu_writeq writeq_relaxed +#define smmu_write_atomic_lq writeq_relaxed#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) +#define smmu_write_atomic_lq writel_relaxed#endif /* Configuration registers */
@@ -211,11 +211,9 @@#define ARM_SMMU_CB_TTBCR 0x30#define ARM_SMMU_CB_S1_MAIR0 0x38#define ARM_SMMU_CB_S1_MAIR1 0x3c -#define ARM_SMMU_CB_PAR_LO 0x50 -#define ARM_SMMU_CB_PAR_HI 0x54 +#define ARM_SMMU_CB_PAR 0x50#define ARM_SMMU_CB_FSR 0x58 -#define ARM_SMMU_CB_FAR_LO 0x60 -#define ARM_SMMU_CB_FAR_HI 0x64 +#define ARM_SMMU_CB_FAR 0x60#define ARM_SMMU_CB_FSYNR0 0x68#define ARM_SMMU_CB_S1_TLBIVA 0x600#define ARM_SMMU_CB_S1_TLBIASID 0x610
@@ -645,7 +643,7 @@ static void arm_smmu_tlb_inv_range_nosync(unsigned long iova, size_t size,ARM_SMMU_CB_S2_TLBIIPAS2 ; iova >>=12 ;do { -writeq_relaxed (iova, reg); +smmu_write_atomic_lq (iova, reg); iova += granule >>12 ; }while (size -= granule);#endif
@@ -664,7 +662,7 @@ static struct iommu_gather_ops arm_smmu_gather_ops = {static irqreturn_t arm_smmu_context_fault (int irq,void *dev) {int flags, ret; -u32 fsr, far, fsynr, resume; +u32 fsr, fsynr, resume;unsigned long iova;struct iommu_domain *domain = dev;struct arm_smmu_domain *smmu_domain =to_smmu_domain (domain);
@@ -686,13 +684,7 @@ static irqreturn_t arm_smmu_context_fault(int irq, void *dev) fsynr =readl_relaxed (cb_base +ARM_SMMU_CB_FSYNR0 ); flags = fsynr &FSYNR0_WNR ?IOMMU_FAULT_WRITE :IOMMU_FAULT_READ ; - far =readl_relaxed (cb_base +ARM_SMMU_CB_FAR_LO ); - iova = far; -#ifdef CONFIG_64BIT - far =readl_relaxed (cb_base +ARM_SMMU_CB_FAR_HI ); - iova |= ((unsigned long )far <<32 ); -#endif - + iova =readq_relaxed (cb_base +ARM_SMMU_CB_FAR );if (!report_iommu_fault (domain, smmu->dev, iova, flags)) { ret =IRQ_HANDLED ; resume =RESUME_RETRY ;
@@ -788,14 +780,14 @@ static void arm_smmu_init_context_bank(struct arm_smmu_domain *smmu_domain, reg64 = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[0 ]; reg64 |= ((u64 )ARM_SMMU_CB_ASID (smmu, cfg)) << TTBRn_ASID_SHIFT; -smmu_writeq (reg64, cb_base +ARM_SMMU_CB_TTBR0 ); +writeq_relaxed (reg64, cb_base +ARM_SMMU_CB_TTBR0 ); reg64 = pgtbl_cfg->arm_lpae_s1_cfg.ttbr[1 ]; reg64 |= ((u64 )ARM_SMMU_CB_ASID (smmu, cfg)) << TTBRn_ASID_SHIFT; -smmu_writeq (reg64, cb_base +ARM_SMMU_CB_TTBR1 ); +writeq_relaxed (reg64, cb_base +ARM_SMMU_CB_TTBR1 ); }else { reg64 = pgtbl_cfg->arm_lpae_s2_cfg.vttbr; -smmu_writeq (reg64, cb_base +ARM_SMMU_CB_TTBR0 ); +writeq_relaxed (reg64, cb_base +ARM_SMMU_CB_TTBR0 ); }/* TTBCR */
@@ -1263,8 +1255,8 @@ static phys_addr_t arm_smmu_iova_to_phys_hard(struct iommu_domain *domain,/* ATS1 registers can only be written atomically */ va = iova & ~0xfffUL ;if (smmu->version ==ARM_SMMU_V2 ) -smmu_writeq (va, cb_base +ARM_SMMU_CB_ATS1PR ); -else +smmu_write_atomic_lq (va, cb_base +ARM_SMMU_CB_ATS1PR ); +else /* Register is only 32-bit in v1 */ writel_relaxed (va, cb_base +ARM_SMMU_CB_ATS1PR ); if (readl_poll_timeout_atomic(cb_base + ARM_SMMU_CB_ATSR, tmp,
@@ -1275,9 +1267,7 @@ static phys_addr_t arm_smmu_iova_to_phys_hard(struct iommu_domain *domain,return ops->iova_to_phys (ops, iova); } - phys =readl_relaxed (cb_base +ARM_SMMU_CB_PAR_LO ); - phys |= ((u64 )readl_relaxed (cb_base +ARM_SMMU_CB_PAR_HI )) <<32 ; - + phys =readq_relaxed (cb_base +ARM_SMMU_CB_PAR );if (phys &CB_PAR_F ) {dev_err (dev,"translation fault!\n" );dev_err (dev,"PAR = 0x%llx\n" , phys);
Signature · 1 line
2.7.3.dirty
Robin Murphy
reviewerPATCH 6/7 iommu/arm-smmu: Decouple context format from kernel config
In reply to Robin MurphyThe way the driver currently forces an AArch32 or AArch64 context format based on the kernel config and SMMU architecture version is suboptimal, in that it makes it very hard to support oddball mix-and-match cases like the SMMUv1 64KB supplement, or situations where the reduced table depth of an AArch32 short descriptor context may be desirable under an AArch64 kernel. It also only happens to work on current implementations which do support all the relevant formats.
Introduce an explicit notion of context format, so we can manage that independently and get rid of the inflexible #ifdeffery.
--- drivers/iommu/arm-smmu.c | 98 +++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 80 insertions(+), 18 deletions(-)
drivers/iommu/arm-smmu.c +80 -18
@@ -117,6 +117,8 @@#define ID0_NTS (1 << 28)#define ID0_SMS (1 << 27)#define ID0_ATOSNS (1 << 26) +#define ID0_PTFS_NO_AARCH32 (1 << 25) +#define ID0_PTFS_NO_AARCH32S (1 << 24)#define ID0_CTTW (1 << 14)#define ID0_NUMIRPT_SHIFT 16#define ID0_NUMIRPT_MASK 0xff
@@ -317,6 +319,11 @@ struct arm_smmu_device {#define ARM_SMMU_FEAT_TRANS_NESTED (1 << 4)#define ARM_SMMU_FEAT_TRANS_OPS (1 << 5)#define ARM_SMMU_FEAT_VMID16 (1 << 6) +#define ARM_SMMU_FEAT_FMT_AARCH64_4K (1 << 7) +#define ARM_SMMU_FEAT_FMT_AARCH64_16K (1 << 8) +#define ARM_SMMU_FEAT_FMT_AARCH64_64K (1 << 9) +#define ARM_SMMU_FEAT_FMT_AARCH32_L (1 << 10) +#define ARM_SMMU_FEAT_FMT_AARCH32_S (1 << 11)u32 features;#define ARM_SMMU_OPT_SECURE_CFG_ACCESS (1 << 0)
@@ -346,10 +353,18 @@ struct arm_smmu_device {u32 cavium_id_base;/* Specific to Cavium */ }; +enum arm_smmu_context_fmt { +ARM_SMMU_CTX_FMT_NONE , +ARM_SMMU_CTX_FMT_AARCH64 , +ARM_SMMU_CTX_FMT_AARCH32_L , +ARM_SMMU_CTX_FMT_AARCH32_S , +}; +struct arm_smmu_cfg {u8 cbndx;u8 irptndx;u32 cbar; +enum arm_smmu_context_fmt fmt; };#define INVALID_IRPTNDX 0xff
@@ -619,14 +634,13 @@ static void arm_smmu_tlb_inv_range_nosync(unsigned long iova, size_t size, reg =ARM_SMMU_CB_BASE (smmu) +ARM_SMMU_CB (smmu, cfg->cbndx); reg += leaf ?ARM_SMMU_CB_S1_TLBIVAL :ARM_SMMU_CB_S1_TLBIVA ; -if (!IS_ENABLED (CONFIG_64BIT ) || smmu->version ==ARM_SMMU_V1 ) { +if (cfg->fmt !=ARM_SMMU_CTX_FMT_AARCH64 ) { iova &= ~12UL ; iova |=ARM_SMMU_CB_ASID (smmu, cfg);do {writel_relaxed (iova, reg); iova += granule; }while (size -= granule); -#ifdef CONFIG_64BIT }else { iova >>=12 ; iova |= (u64 )ARM_SMMU_CB_ASID (smmu, cfg) <<48 ;
@@ -634,9 +648,7 @@ static void arm_smmu_tlb_inv_range_nosync(unsigned long iova, size_t size,writeq_relaxed (iova, reg); iova += granule >>12 ; }while (size -= granule); -#endif } -#ifdef CONFIG_64BIT }else if (smmu->version ==ARM_SMMU_V2 ) { reg =ARM_SMMU_CB_BASE (smmu) +ARM_SMMU_CB (smmu, cfg->cbndx); reg += leaf ? ARM_SMMU_CB_S2_TLBIIPAS2L :
@@ -646,7 +658,6 @@ static void arm_smmu_tlb_inv_range_nosync(unsigned long iova, size_t size,smmu_write_atomic_lq (iova, reg); iova += granule >>12 ; }while (size -= granule); -#endif }else { reg =ARM_SMMU_GR0 (smmu) +ARM_SMMU_GR0_TLBIVMID ;writel_relaxed (ARM_SMMU_CB_VMID (smmu, cfg), reg);
@@ -745,11 +756,10 @@ static void arm_smmu_init_context_bank(struct arm_smmu_domain *smmu_domain, cb_base =ARM_SMMU_CB_BASE (smmu) +ARM_SMMU_CB (smmu, cfg->cbndx);if (smmu->version >ARM_SMMU_V1 ) { -#ifdef CONFIG_64BIT - reg =CBA2R_RW64_64BIT ; -#else - reg =CBA2R_RW64_32BIT ; -#endif +if (cfg->fmt ==ARM_SMMU_CTX_FMT_AARCH64 ) + reg =CBA2R_RW64_64BIT ; +else + reg =CBA2R_RW64_32BIT ;/* 16-bit VMIDs live in CBA2R */ if (smmu->features &ARM_SMMU_FEAT_VMID16 ) reg |=ARM_SMMU_CB_VMID (smmu, cfg) <<CBA2R_VMID_SHIFT ;
@@ -860,16 +870,48 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,if (!(smmu->features &ARM_SMMU_FEAT_TRANS_S2 )) smmu_domain->stage =ARM_SMMU_DOMAIN_S1 ; +/* +* Choosing a suitable context format is even more fiddly. Until we +* grow some way for the caller to express a preference, just aim for +* the closest match to the CPU format out of what we might have. +*/ +if (smmu->features &ARM_SMMU_FEAT_FMT_AARCH32_L ) + cfg->fmt =ARM_SMMU_CTX_FMT_AARCH32_L ; +if (IS_ENABLED (CONFIG_64BIT ) || cfg->fmt ==ARM_SMMU_CTX_FMT_NONE ) { +switch (PAGE_SIZE ) { +case SZ_64K : +if (smmu->features &ARM_SMMU_FEAT_FMT_AARCH64_64K ) { + cfg->fmt =ARM_SMMU_CTX_FMT_AARCH64 ; +break ; + }/* else fall through */ +case SZ_16K : +if (smmu->features &ARM_SMMU_FEAT_FMT_AARCH64_16K ) { + cfg->fmt =ARM_SMMU_CTX_FMT_AARCH64 ; +break ; + }/* else fall through */ +case SZ_4K : +if (smmu->features &ARM_SMMU_FEAT_FMT_AARCH64_4K ) + cfg->fmt =ARM_SMMU_CTX_FMT_AARCH64 ; + } + } +if (cfg->fmt ==ARM_SMMU_CTX_FMT_NONE ) { + ret = -EINVAL ; + goto out_unlock; + } +switch (smmu_domain->stage) {case ARM_SMMU_DOMAIN_S1 : cfg->cbar =CBAR_TYPE_S1_TRANS_S2_BYPASS ; start = smmu->num_s2_context_banks; ias = smmu->va_size; oas = smmu->ipa_size; -if (IS_ENABLED (CONFIG_64BIT )) +if (cfg->fmt ==ARM_SMMU_CTX_FMT_AARCH64 ) { fmt =ARM_64_LPAE_S1 ; -else + }else { fmt =ARM_32_LPAE_S1 ; + ias =min (ias,32UL ); + oas =min (oas,40UL ); + }break ;case ARM_SMMU_DOMAIN_NESTED :/*
@@ -881,10 +923,13 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain, start =0 ; ias = smmu->ipa_size; oas = smmu->pa_size; -if (IS_ENABLED (CONFIG_64BIT )) +if (cfg->fmt ==ARM_SMMU_CTX_FMT_AARCH64 ) { fmt =ARM_64_LPAE_S2 ; -else + }else { fmt =ARM_32_LPAE_S2 ; + ias =min (ias,40UL ); + oas =min (oas,40UL ); + }break ;default : ret = -EINVAL ;
@@ -1670,6 +1715,12 @@ static int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu)ID0_NUMSIDB_MASK ; } +if (smmu->version <ARM_SMMU_V2 || !(id &ID0_PTFS_NO_AARCH32 )) { + smmu->features |=ARM_SMMU_FEAT_FMT_AARCH32_L ; +if (!(id &ID0_PTFS_NO_AARCH32S )) + smmu->features |=ARM_SMMU_FEAT_FMT_AARCH32_S ; + } +/* ID1 */ id =readl_relaxed (gr0_base +ARM_SMMU_GR0_ID1 ); smmu->pgshift = (id &ID1_PAGESIZE ) ?16 :12 ;
@@ -1725,7 +1776,6 @@ static int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu)if (smmu->version ==ARM_SMMU_V1 ) { smmu->va_size = smmu->ipa_size; - size =SZ_4K |SZ_2M |SZ_1G ; }else { size = (id >>ID2_UBS_SHIFT ) &ID2_UBS_MASK ; smmu->va_size =arm_smmu_id_size_to_bits (size);
@@ -1734,13 +1784,25 @@ static int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu)#endif size =0 ;if (id &ID2_PTFS_4K ) - size |=SZ_4K |SZ_2M |SZ_1G ; + smmu->features |=ARM_SMMU_FEAT_FMT_AARCH64_4K ;if (id &ID2_PTFS_16K ) - size |=SZ_16K |SZ_32M ; + smmu->features |=ARM_SMMU_FEAT_FMT_AARCH64_16K ;if (id &ID2_PTFS_64K ) - size |=SZ_64K |SZ_512M ; + smmu->features |=ARM_SMMU_FEAT_FMT_AARCH64_64K ; } +/* Now we've corralled the various formats, what'll it do? */ + size =0 ; +if (smmu->features &ARM_SMMU_FEAT_FMT_AARCH32_S ) + size |=SZ_4K |SZ_64K |SZ_1M |SZ_16M ; +if (smmu->features & + (ARM_SMMU_FEAT_FMT_AARCH32_L |ARM_SMMU_FEAT_FMT_AARCH64_4K )) + size |=SZ_4K |SZ_2M |SZ_1G ; +if (smmu->features &ARM_SMMU_FEAT_FMT_AARCH64_16K ) + size |=SZ_16K |SZ_32M ; +if (smmu->features &ARM_SMMU_FEAT_FMT_AARCH64_64K ) + size |=SZ_64K |SZ_512M ; + arm_smmu_ops.pgsize_bitmap &= size;dev_notice (smmu->dev,"\tSupported page sizes: 0x%08lx\n" , size);
Signature · 1 line
2.7.3.dirty
PATCH 6/7 iommu/arm-smmu: Decouple context format from kernel config
Hi Robin,
Quoted 10 lines · Robin Murphy
The way the driver currently forces an AArch32 or AArch64 context format based on the kernel config and SMMU architecture version is suboptimal, in that it makes it very hard to support oddball mix-and-match cases like the SMMUv1 64KB supplement, or situations where the reduced table depth of an AArch32 short descriptor context may be desirable under an AArch64 kernel. It also only happens to work on current implementations which do support all the relevant formats.
Introduce an explicit notion of context format, so we can manage that independently and get rid of the inflexible #ifdeffery.
Thanks for doing all of this! One comment on the page size stuff:
Quoted 7 lines
drivers/iommu/arm-smmu.c +7
-0+if (IS_ENABLED (CONFIG_64BIT ) || cfg->fmt ==ARM_SMMU_CTX_FMT_NONE ) { +switch (PAGE_SIZE ) { +case SZ_64K : +if (smmu->features &ARM_SMMU_FEAT_FMT_AARCH64_64K ) { + cfg->fmt =ARM_SMMU_CTX_FMT_AARCH64 ; +break ; + }/* else fall through */
drivers/iommu/arm-smmu.c static int arm_smmu_init_domain_context(struct iommu_domain *domain, · line 896
Quoted 10 lines
drivers/iommu/arm-smmu.c +10
-0+case SZ_16K : +if (smmu->features &ARM_SMMU_FEAT_FMT_AARCH64_16K ) { + cfg->fmt =ARM_SMMU_CTX_FMT_AARCH64 ; +break ; + }/* else fall through */ +case SZ_4K : +if (smmu->features &ARM_SMMU_FEAT_FMT_AARCH64_4K ) + cfg->fmt =ARM_SMMU_CTX_FMT_AARCH64 ; + } + }
The io-pgtable code (arm_lpae_restrict_pgsizes) already does something *very* similar to this, using the pgsize_bitmap as input. Can you not just choose the ARM_SMMU_CTX_FMT_AARCH64 here and set the pgsize_bitmap to represent all page sizes supported by the hardware? That way, you should end up with the best option coming back from the pgtable code (I do this in the v3 driver, fwiw).
Will
Robin Murphy
reviewerPATCH 6/7 iommu/arm-smmu: Decouple context format from kernel config
Quoted 32 lines · Will Deacon
Hi Robin,
Quoted 10 lines · Robin Murphy
The way the driver currently forces an AArch32 or AArch64 context format based on the kernel config and SMMU architecture version is suboptimal, in that it makes it very hard to support oddball mix-and-match cases like the SMMUv1 64KB supplement, or situations where the reduced table depth of an AArch32 short descriptor context may be desirable under an AArch64 kernel. It also only happens to work on current implementations which do support all the relevant formats.
Introduce an explicit notion of context format, so we can manage that independently and get rid of the inflexible #ifdeffery.
Thanks for doing all of this! One comment on the page size stuff:
Quoted 16 lines
drivers/iommu/arm-smmu.c +16
-0+if (IS_ENABLED (CONFIG_64BIT ) || cfg->fmt ==ARM_SMMU_CTX_FMT_NONE ) { +switch (PAGE_SIZE ) { +case SZ_64K : +if (smmu->features &ARM_SMMU_FEAT_FMT_AARCH64_64K ) { + cfg->fmt =ARM_SMMU_CTX_FMT_AARCH64 ; +break ; + }/* else fall through */ +case SZ_16K : +if (smmu->features &ARM_SMMU_FEAT_FMT_AARCH64_16K ) { + cfg->fmt =ARM_SMMU_CTX_FMT_AARCH64 ; +break ; + }/* else fall through */ +case SZ_4K : +if (smmu->features &ARM_SMMU_FEAT_FMT_AARCH64_4K ) + cfg->fmt =ARM_SMMU_CTX_FMT_AARCH64 ; + }
Quoted 8 lines
Review
Quoted 1 line
(quoted diff) +1
-0+ }The io-pgtable code (
arm_lpae_restrict_pgsizes) already does something *very* similar to this, using thepgsize_bitmapas input. Can you not just choose theARM_SMMU_CTX_FMT_AARCH64here and set thepgsize_bitmapto represent all page sizes supported by the hardware? That way, you should end up with the best option coming back from the pgtable code (I do this in the v3 driver, fwiw).
It took a while to come back to me, but the problem is that we can't call alloc_io_pgtable_ops(fmt...) before choosing either ARM_64_LPAE_Sx or ARM_32_LPAE_Sx for fmt, but if we commit to 64-bit we'll get stuck later without the possibility of falling back to AArch32 if it turns out we don't have a viable AArch64 granule. Plus we'd have to remove the LPAE page sizes from the bitmap beforehand in the case we don't support AARCH64_4K lest we end up with a phantom format the hardware doesn't actually do, and it all starts getting rather horrible...
Robin.
Quoted 2 lines
Will
PATCH 6/7 iommu/arm-smmu: Decouple context format from kernel config
Quoted 42 lines · Robin Murphy
Quoted 32 lines · Will Deacon
Hi Robin,
Quoted 10 lines · Robin Murphy
The way the driver currently forces an AArch32 or AArch64 context format based on the kernel config and SMMU architecture version is suboptimal, in that it makes it very hard to support oddball mix-and-match cases like the SMMUv1 64KB supplement, or situations where the reduced table depth of an AArch32 short descriptor context may be desirable under an AArch64 kernel. It also only happens to work on current implementations which do support all the relevant formats.
Introduce an explicit notion of context format, so we can manage that independently and get rid of the inflexible #ifdeffery.
Thanks for doing all of this! One comment on the page size stuff:
Quoted 16 lines
drivers/iommu/arm-smmu.c +16
-0+if (IS_ENABLED (CONFIG_64BIT ) || cfg->fmt ==ARM_SMMU_CTX_FMT_NONE ) { +switch (PAGE_SIZE ) { +case SZ_64K : +if (smmu->features &ARM_SMMU_FEAT_FMT_AARCH64_64K ) { + cfg->fmt =ARM_SMMU_CTX_FMT_AARCH64 ; +break ; + }/* else fall through */ +case SZ_16K : +if (smmu->features &ARM_SMMU_FEAT_FMT_AARCH64_16K ) { + cfg->fmt =ARM_SMMU_CTX_FMT_AARCH64 ; +break ; + }/* else fall through */ +case SZ_4K : +if (smmu->features &ARM_SMMU_FEAT_FMT_AARCH64_4K ) + cfg->fmt =ARM_SMMU_CTX_FMT_AARCH64 ; + }Quoted 8 lines
Review
Quoted 1 line
(quoted diff) +1
-0+ }The io-pgtable code (
arm_lpae_restrict_pgsizes) already does something *very* similar to this, using thepgsize_bitmapas input. Can you not just choose theARM_SMMU_CTX_FMT_AARCH64here and set thepgsize_bitmapto represent all page sizes supported by the hardware? That way, you should end up with the best option coming back from the pgtable code (I do this in the v3 driver, fwiw).
Quoted 5 lines
It took a while to come back to me, but the problem is that we can't call
alloc_io_pgtable_ops(fmt...)before choosing eitherARM_64_LPAE_SxorARM_32_LPAE_Sxfor fmt, but if we commit to 64-bit we'll get stuck later without the possibility of falling back to AArch32 if it turns out we don't have a viable AArch64 granule.
In what case would you not have a viable AArch64 granule, but the option of falling back to AArch32 makes things work?
Quoted 4 lines
Plus we'd have to remove the LPAE page sizes
from the bitmap beforehand in the case we don't supportAARCH64_4Klest we end up with a phantom format the hardware doesn't actually do, and it all starts getting rather horrible...
I'm not following. The io-pgtable code shouldn't give you back a phanton format.
Will
Robin Murphy
reviewerPATCH 6/7 iommu/arm-smmu: Decouple context format from kernel config
Quoted 43 lines · Will Deacon
Quoted 42 lines · Robin Murphy
Quoted 40 lines · Will Deacon
Hi Robin,
Quoted 10 lines · Robin Murphy
The way the driver currently forces an AArch32 or AArch64 context format based on the kernel config and SMMU architecture version is suboptimal, in that it makes it very hard to support oddball mix-and-match cases like the SMMUv1 64KB supplement, or situations where the reduced table depth of an AArch32 short descriptor context may be desirable under an AArch64 kernel. It also only happens to work on current implementations which do support all the relevant formats.
Introduce an explicit notion of context format, so we can manage that independently and get rid of the inflexible #ifdeffery.
Thanks for doing all of this! One comment on the page size stuff:
Quoted 7 lines
drivers/iommu/arm-smmu.c +7
-0+if (IS_ENABLED (CONFIG_64BIT ) || cfg->fmt ==ARM_SMMU_CTX_FMT_NONE ) { +switch (PAGE_SIZE ) { +case SZ_64K : +if (smmu->features &ARM_SMMU_FEAT_FMT_AARCH64_64K ) { + cfg->fmt =ARM_SMMU_CTX_FMT_AARCH64 ; +break ; + }/* else fall through */ drivers/iommu/arm-smmu.c static int arm_smmu_init_domain_context(struct iommu_domain *domain, · line 896
Quoted 10 lines
drivers/iommu/arm-smmu.c +10
-0+case SZ_16K : +if (smmu->features &ARM_SMMU_FEAT_FMT_AARCH64_16K ) { + cfg->fmt =ARM_SMMU_CTX_FMT_AARCH64 ; +break ; + }/* else fall through */ +case SZ_4K : +if (smmu->features &ARM_SMMU_FEAT_FMT_AARCH64_4K ) + cfg->fmt =ARM_SMMU_CTX_FMT_AARCH64 ; + } + }The io-pgtable code (
arm_lpae_restrict_pgsizes) already does something *very* similar to this, using thepgsize_bitmapas input. Can you not just choose theARM_SMMU_CTX_FMT_AARCH64here and set thepgsize_bitmapto represent all page sizes supported by the hardware? That way, you should end up with the best option coming back from the pgtable code (I do this in the v3 driver, fwiw).
Quoted 8 lines
Quoted 5 lines
It took a while to come back to me, but the problem is that we can't call
alloc_io_pgtable_ops(fmt...)before choosing eitherARM_64_LPAE_SxorARM_32_LPAE_Sxfor fmt, but if we commit to 64-bit we'll get stuck later without the possibility of falling back to AArch32 if it turns out we don't have a viable AArch64 granule.In what case would you not have a viable AArch64 granule, but the option of falling back to AArch32 makes things work?
A 4KB page kernel with MMU-401, whose only supported AArch64 granule is supposedly 64KB.
Quoted 7 lines
Quoted 4 lines
Plus we'd have to remove the LPAE page sizes
from the bitmap beforehand in the case we don't supportAARCH64_4Klest we end up with a phantom format the hardware doesn't actually do, and it all starts getting rather horrible...I'm not following. The io-pgtable code shouldn't give you back a phanton format.
If you call alloc_io_pgtable_ops() with ARM_64_LPAE_S2 and an unmolested MMU-401 pgsize_bitmap, you get back a 4K granule v8 pagetable, which per a strict reading of the TRM isn't supported (although does appear to work in practice...)
Consider also a stage 1 SMMU supporting all formats but only with 4K
granules: with a 64K page arm64 kernel, the presence of AARCH64_4K support would lead us into arm_64_lpae_alloc_pgtable_s1(), which would be tricked into giving us back a 64K granule v8 format by the short descriptor large page size matching PAGE_SIZE, and things would definitely go downhill from there.
Given that, I think the only truly safe thing to do is to pass an explicit granule in the io_pgtable_cfg and just get rid of the bitmap-based guessing in arm_lpae_restrict_pgsizes() - otherwise, we'd still have to pre-restrict the bitmap, making it pretty much redundant anyway. I'll have a hack at that - in the meantime please feel free to queue patches 1-3 if you're happy with them, as that part is still feature-complete without all this context stuff.
Robin.
Quoted 3 lines
Will
Will Deacon
AppliedPATCH 6/7 iommu/arm-smmu: Decouple context format from kernel config
Quoted 6 lines · Robin Murphy
Quoted 2 lines · Will Deacon
In what case would you not have a viable AArch64 granule, but the option of falling back to AArch32 makes things work?
A 4KB page kernel with MMU-401, whose only supported AArch64 granule is supposedly 64KB.
That doesn't sound right at all!
Quoted 5 lines
Quoted 5 lines
Quoted 4 lines
Plus we'd have to remove the LPAE page sizes
from the bitmap beforehand in the case we don't supportAARCH64_4Klest we end up with a phantom format the hardware doesn't actually do, and it all starts getting rather horrible...
Quoted 7 lines
Quoted 2 lines
I'm not following. The io-pgtable code shouldn't give you back a phanton format.
If you call
alloc_io_pgtable_ops()withARM_64_LPAE_S2and an unmolested MMU-401pgsize_bitmap, you get back a 4K granule v8 pagetable, which per a strict reading of the TRM isn't supported (although does appear to work in practice...)
I suspect that's a badly worded/incorrect TRM. In reality, I'd be happy to assume that a given granule is supported across all formats (in as much as it can be) if it's supported by one of them, but acknowledge that's not guaranteed by the architecture.
Quoted 6 lines
Consider also a stage 1 SMMU supporting all formats but only with 4K
granules: with a 64K page arm64 kernel, the presence ofAARCH64_4Ksupport would lead us intoarm_64_lpae_alloc_pgtable_s1(), which would be tricked into giving us back a 64K granule v8 format by the short descriptor large page size matchingPAGE_SIZE, and things would definitely go downhill from there.
We could improve this by having a separate pgsize_bitmap per format, and passing the relevant one to the relevant page table constructor.
Quoted 4 lines
Given that, I think the only truly safe thing to do is to pass an explicit granule in the
io_pgtable_cfgand just get rid of the bitmap-based guessing inarm_lpae_restrict_pgsizes()- otherwise, we'd still have to pre-restrict the bitmap, making it pretty much redundant anyway. I'll have a hack at that
Actually, I think I prefer to go the other way. Let's try moving more of this common logic into the io-pgtable code. For example, an IOMMU driver could say "I support these formats, and these options (page sizes, quirks, etc) for each format" and the io-pgtable code can choose the most appropriate thing.
Quoted 2 lines
- in the meantime please feel free to queue patches 1-3 if you're happy with them, as that part is still feature-complete without all this context stuff.
Queued those, thanks!
Will
Robin Murphy
reviewerPATCH 6/7 iommu/arm-smmu: Decouple context format from kernel config
Quoted 43 lines · Will Deacon
Quoted 6 lines · Robin Murphy
Quoted 2 lines · Will Deacon
In what case would you not have a viable AArch64 granule, but the option of falling back to AArch32 makes things work?
A 4KB page kernel with MMU-401, whose only supported AArch64 granule is supposedly 64KB.
That doesn't sound right at all!
Quoted 5 lines
Quoted 5 lines
Quoted 4 lines
Plus we'd have to remove the LPAE page sizes
from the bitmap beforehand in the case we don't supportAARCH64_4Klest we end up with a phantom format the hardware doesn't actually do, and it all starts getting rather horrible...Quoted 7 lines
Quoted 2 lines
I'm not following. The io-pgtable code shouldn't give you back a phanton format.
If you call
alloc_io_pgtable_ops()withARM_64_LPAE_S2and an unmolested MMU-401pgsize_bitmap, you get back a 4K granule v8 pagetable, which per a strict reading of the TRM isn't supported (although does appear to work in practice...)I suspect that's a badly worded/incorrect TRM. In reality, I'd be happy to assume that a given granule is supported across all formats (in as much as it can be) if it's supported by one of them, but acknowledge that's not guaranteed by the architecture.
Quoted 6 lines
Consider also a stage 1 SMMU supporting all formats but only with 4K
granules: with a 64K page arm64 kernel, the presence ofAARCH64_4Ksupport would lead us intoarm_64_lpae_alloc_pgtable_s1(), which would be tricked into giving us back a 64K granule v8 format by the short descriptor large page size matchingPAGE_SIZE, and things would definitely go downhill from there.We could improve this by having a separate
pgsize_bitmapper format, and passing the relevant one to the relevant page table constructor.Quoted 4 lines
Given that, I think the only truly safe thing to do is to pass an explicit granule in the
io_pgtable_cfgand just get rid of the bitmap-based guessing inarm_lpae_restrict_pgsizes()- otherwise, we'd still have to pre-restrict the bitmap, making it pretty much redundant anyway. I'll have a hack at that
Quoted 5 lines
Actually, I think I prefer to go the other way. Let's try moving more of this common logic into the io-pgtable code. For example, an IOMMU driver could say "I support these formats, and these options (page sizes, quirks, etc) for each format" and the io-pgtable code can choose the most appropriate thing.
Agreed - that sounds like the best approach in the long run, and I'd be happy to put some thought into designing it once the to-do list shrinks a bit. Right now, though, how about a quick fix like I just hacked up below? (diff on top of this series because laziness)
Robin.
--->8---
drivers/iommu/arm-smmu.c +0 -0
@@ -784,28 +784,20 @@ static int arm_smmu_init_domain_context(struct
iommu_domain *domain,
/*
* Choosing a suitable context format is even more fiddly. Until we
- * grow some way for the caller to express a preference, just aim for
- * the closest match to the CPU format out of what we might have.
+ * grow some way for the caller to express a preference, and/or move
+ * the decision into the io-pgtable code where it arguably belongs,
+ * just aim for the closest thing to the rest of the system, and hope
+ * that the hardware isn't mental enough that we can't assume AArch64
+ * support to be a superset of AArch32 support...
*/
if (smmu->features & ARM_SMMU_FEAT_FMT_AARCH32_L)
cfg->fmt = ARM_SMMU_CTX_FMT_AARCH32_L;
- if (IS_ENABLED(CONFIG_64BIT) || cfg->fmt == ARM_SMMU_CTX_FMT_NONE) {
- switch (PAGE_SIZE) {
- case SZ_64K:
- if (smmu->features & ARM_SMMU_FEAT_FMT_AARCH64_64K) {
- cfg->fmt = ARM_SMMU_CTX_FMT_AARCH64;
- break;
- } /* else fall through */
- case SZ_16K:
- if (smmu->features & ARM_SMMU_FEAT_FMT_AARCH64_16K) {
- cfg->fmt = ARM_SMMU_CTX_FMT_AARCH64;
- break;
- } /* else fall through */
- case SZ_4K:
- if (smmu->features & ARM_SMMU_FEAT_FMT_AARCH64_4K)
- cfg->fmt = ARM_SMMU_CTX_FMT_AARCH64;
- }
- }
+ if ((IS_ENABLED(CONFIG_64BIT) || cfg->fmt == ARM_SMMU_CTX_FMT_NONE) &&
+ (smmu->features & (ARM_SMMU_FEAT_FMT_AARCH64_64K |
+ ARM_SMMU_FEAT_FMT_AARCH64_16K |
+ ARM_SMMU_FEAT_FMT_AARCH64_4K)))
+ cfg->fmt = ARM_SMMU_CTX_FMT_AARCH64;
+
if (cfg->fmt == ARM_SMMU_CTX_FMT_NONE) {
ret = -EINVAL;
goto out_unlock;
Robin Murphy
reviewerPATCH v2 1/1 iommu/arm-smmu: Decouple context format from kernel config
In reply to Robin MurphyThe way the driver currently forces an AArch32 or AArch64 context format based on the kernel config and SMMU architecture version is suboptimal, in that it makes it very hard to support oddball mix-and-match cases like the SMMUv1 64KB supplement, or situations where the reduced table depth of an AArch32 short descriptor context may be desirable under an AArch64 kernel. It also only happens to work on current implementations which do support all the relevant formats.
Introduce an explicit notion of context format, so we can manage that independently and get rid of the inflexible #ifdeffery.
---
Changes since v1:
- Simplify the format decision to let the io-pgtable code pick AArch64 granules itself, since (pl330 shenanigans aside) MMU-401 actually works better than it claims to. I'll look into abstracting this away completely into io-pgtable so it can be shared with SMMUv3 and others.
- Remove a couple more now-dead lines from arm_smmu_device_cfg_probe() which I missed first time around.
Robin.
drivers/iommu/arm-smmu.c | 94 ++++++++++++++++++++++++++++++++++++------------ 1 file changed, 72 insertions(+), 22 deletions(-)
drivers/iommu/arm-smmu.c +72 -22
@@ -117,6 +117,8 @@#define ID0_NTS (1 << 28)#define ID0_SMS (1 << 27)#define ID0_ATOSNS (1 << 26) +#define ID0_PTFS_NO_AARCH32 (1 << 25) +#define ID0_PTFS_NO_AARCH32S (1 << 24)#define ID0_CTTW (1 << 14)#define ID0_NUMIRPT_SHIFT 16#define ID0_NUMIRPT_MASK 0xff
@@ -317,6 +319,11 @@ struct arm_smmu_device {#define ARM_SMMU_FEAT_TRANS_NESTED (1 << 4)#define ARM_SMMU_FEAT_TRANS_OPS (1 << 5)#define ARM_SMMU_FEAT_VMID16 (1 << 6) +#define ARM_SMMU_FEAT_FMT_AARCH64_4K (1 << 7) +#define ARM_SMMU_FEAT_FMT_AARCH64_16K (1 << 8) +#define ARM_SMMU_FEAT_FMT_AARCH64_64K (1 << 9) +#define ARM_SMMU_FEAT_FMT_AARCH32_L (1 << 10) +#define ARM_SMMU_FEAT_FMT_AARCH32_S (1 << 11)u32 features;#define ARM_SMMU_OPT_SECURE_CFG_ACCESS (1 << 0)
@@ -346,10 +353,18 @@ struct arm_smmu_device {u32 cavium_id_base;/* Specific to Cavium */ }; +enum arm_smmu_context_fmt { +ARM_SMMU_CTX_FMT_NONE , +ARM_SMMU_CTX_FMT_AARCH64 , +ARM_SMMU_CTX_FMT_AARCH32_L , +ARM_SMMU_CTX_FMT_AARCH32_S , +}; +struct arm_smmu_cfg {u8 cbndx;u8 irptndx;u32 cbar; +enum arm_smmu_context_fmt fmt; };#define INVALID_IRPTNDX 0xff
@@ -619,14 +634,13 @@ static void arm_smmu_tlb_inv_range_nosync(unsigned long iova, size_t size, reg =ARM_SMMU_CB_BASE (smmu) +ARM_SMMU_CB (smmu, cfg->cbndx); reg += leaf ?ARM_SMMU_CB_S1_TLBIVAL :ARM_SMMU_CB_S1_TLBIVA ; -if (!IS_ENABLED (CONFIG_64BIT ) || smmu->version ==ARM_SMMU_V1 ) { +if (cfg->fmt !=ARM_SMMU_CTX_FMT_AARCH64 ) { iova &= ~12UL ; iova |=ARM_SMMU_CB_ASID (smmu, cfg);do {writel_relaxed (iova, reg); iova += granule; }while (size -= granule); -#ifdef CONFIG_64BIT }else { iova >>=12 ; iova |= (u64 )ARM_SMMU_CB_ASID (smmu, cfg) <<48 ;
@@ -634,9 +648,7 @@ static void arm_smmu_tlb_inv_range_nosync(unsigned long iova, size_t size,writeq_relaxed (iova, reg); iova += granule >>12 ; }while (size -= granule); -#endif } -#ifdef CONFIG_64BIT }else if (smmu->version ==ARM_SMMU_V2 ) { reg =ARM_SMMU_CB_BASE (smmu) +ARM_SMMU_CB (smmu, cfg->cbndx); reg += leaf ? ARM_SMMU_CB_S2_TLBIIPAS2L :
@@ -646,7 +658,6 @@ static void arm_smmu_tlb_inv_range_nosync(unsigned long iova, size_t size,smmu_write_atomic_lq (iova, reg); iova += granule >>12 ; }while (size -= granule); -#endif }else { reg =ARM_SMMU_GR0 (smmu) +ARM_SMMU_GR0_TLBIVMID ;writel_relaxed (ARM_SMMU_CB_VMID (smmu, cfg), reg);
@@ -745,11 +756,10 @@ static void arm_smmu_init_context_bank(struct arm_smmu_domain *smmu_domain, cb_base =ARM_SMMU_CB_BASE (smmu) +ARM_SMMU_CB (smmu, cfg->cbndx);if (smmu->version >ARM_SMMU_V1 ) { -#ifdef CONFIG_64BIT - reg =CBA2R_RW64_64BIT ; -#else - reg =CBA2R_RW64_32BIT ; -#endif +if (cfg->fmt ==ARM_SMMU_CTX_FMT_AARCH64 ) + reg =CBA2R_RW64_64BIT ; +else + reg =CBA2R_RW64_32BIT ;/* 16-bit VMIDs live in CBA2R */ if (smmu->features &ARM_SMMU_FEAT_VMID16 ) reg |=ARM_SMMU_CB_VMID (smmu, cfg) <<CBA2R_VMID_SHIFT ;
@@ -860,16 +870,40 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,if (!(smmu->features &ARM_SMMU_FEAT_TRANS_S2 )) smmu_domain->stage =ARM_SMMU_DOMAIN_S1 ; +/* +* Choosing a suitable context format is even more fiddly. Until we +* grow some way for the caller to express a preference, and/or move +* the decision into the io-pgtable code where it arguably belongs, +* just aim for the closest thing to the rest of the system, and hope +* that the hardware isn't mental enough that we can't assume AArch64 +* support to be a superset of AArch32 support... +*/ +if (smmu->features &ARM_SMMU_FEAT_FMT_AARCH32_L ) + cfg->fmt =ARM_SMMU_CTX_FMT_AARCH32_L ; +if ((IS_ENABLED (CONFIG_64BIT ) || cfg->fmt ==ARM_SMMU_CTX_FMT_NONE ) && + (smmu->features & (ARM_SMMU_FEAT_FMT_AARCH64_64K | +ARM_SMMU_FEAT_FMT_AARCH64_16K | +ARM_SMMU_FEAT_FMT_AARCH64_4K ))) + cfg->fmt =ARM_SMMU_CTX_FMT_AARCH64 ; + +if (cfg->fmt ==ARM_SMMU_CTX_FMT_NONE ) { + ret = -EINVAL ; + goto out_unlock; + } +switch (smmu_domain->stage) {case ARM_SMMU_DOMAIN_S1 : cfg->cbar =CBAR_TYPE_S1_TRANS_S2_BYPASS ; start = smmu->num_s2_context_banks; ias = smmu->va_size; oas = smmu->ipa_size; -if (IS_ENABLED (CONFIG_64BIT )) +if (cfg->fmt ==ARM_SMMU_CTX_FMT_AARCH64 ) { fmt =ARM_64_LPAE_S1 ; -else + }else { fmt =ARM_32_LPAE_S1 ; + ias =min (ias,32UL ); + oas =min (oas,40UL ); + }break ;case ARM_SMMU_DOMAIN_NESTED :/*
@@ -881,10 +915,13 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain, start =0 ; ias = smmu->ipa_size; oas = smmu->pa_size; -if (IS_ENABLED (CONFIG_64BIT )) +if (cfg->fmt ==ARM_SMMU_CTX_FMT_AARCH64 ) { fmt =ARM_64_LPAE_S2 ; -else + }else { fmt =ARM_32_LPAE_S2 ; + ias =min (ias,40UL ); + oas =min (oas,40UL ); + }break ;default : ret = -EINVAL ;
@@ -1670,6 +1707,12 @@ static int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu)ID0_NUMSIDB_MASK ; } +if (smmu->version <ARM_SMMU_V2 || !(id &ID0_PTFS_NO_AARCH32 )) { + smmu->features |=ARM_SMMU_FEAT_FMT_AARCH32_L ; +if (!(id &ID0_PTFS_NO_AARCH32S )) + smmu->features |=ARM_SMMU_FEAT_FMT_AARCH32_S ; + } +/* ID1 */ id =readl_relaxed (gr0_base +ARM_SMMU_GR0_ID1 ); smmu->pgshift = (id &ID1_PAGESIZE ) ?16 :12 ;
@@ -1725,22 +1768,29 @@ static int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu)if (smmu->version ==ARM_SMMU_V1 ) { smmu->va_size = smmu->ipa_size; - size =SZ_4K |SZ_2M |SZ_1G ; }else { size = (id >>ID2_UBS_SHIFT ) &ID2_UBS_MASK ; smmu->va_size =arm_smmu_id_size_to_bits (size); -#ifndef CONFIG_64BIT - smmu->va_size =min (32UL , smmu->va_size); -#endif - size =0 ;if (id &ID2_PTFS_4K ) - size |=SZ_4K |SZ_2M |SZ_1G ; + smmu->features |=ARM_SMMU_FEAT_FMT_AARCH64_4K ;if (id &ID2_PTFS_16K ) - size |=SZ_16K |SZ_32M ; + smmu->features |=ARM_SMMU_FEAT_FMT_AARCH64_16K ;if (id &ID2_PTFS_64K ) - size |=SZ_64K |SZ_512M ; + smmu->features |=ARM_SMMU_FEAT_FMT_AARCH64_64K ; } +/* Now we've corralled the various formats, what'll it do? */ + size =0 ; +if (smmu->features &ARM_SMMU_FEAT_FMT_AARCH32_S ) + size |=SZ_4K |SZ_64K |SZ_1M |SZ_16M ; +if (smmu->features & + (ARM_SMMU_FEAT_FMT_AARCH32_L |ARM_SMMU_FEAT_FMT_AARCH64_4K )) + size |=SZ_4K |SZ_2M |SZ_1G ; +if (smmu->features &ARM_SMMU_FEAT_FMT_AARCH64_16K ) + size |=SZ_16K |SZ_32M ; +if (smmu->features &ARM_SMMU_FEAT_FMT_AARCH64_64K ) + size |=SZ_64K |SZ_512M ; + arm_smmu_ops.pgsize_bitmap &= size;dev_notice (smmu->dev,"\tSupported page sizes: 0x%08lx\n" , size);
Signature · 1 line
2.8.1.dirty
Robin Murphy
reviewerPATCH 7/7 iommu/arm-smmu: Support SMMUv1 64KB supplement
In reply to Robin MurphyThe 64KB Translation Granule Supplement to the SMMUv1 architecture allows an SMMUv1 implementation to support 64KB pages for stage 2 translations, using a constrained VMSAv8 descriptor format limited to 40-bit addresses. Now that we can freely mix and match context formats, we can actually handle having 4KB pages via an AArch32 context but 64KB pages via an AArch64 context, so plumb it in.
It is assumed that any implementations will have hardware capabilities matching the format constraints, thus obviating the need for excessive sanity-checking; this is the case for MMU-401, the only ARM Ltd. implementation.
--- drivers/iommu/arm-smmu.c | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-)
drivers/iommu/arm-smmu.c +14 -8
@@ -277,7 +277,8 @@ MODULE_PARM_DESC(disable_bypass, "Disable bypass streams such that incoming transactions from devices that are not attached to an iommu domain will report an abort back to the device and will not be allowed to pass through the SMMU.");enum arm_smmu_arch_version { -ARM_SMMU_V1 =1 , +ARM_SMMU_V1 , +ARM_SMMU_V1_64K ,ARM_SMMU_V2 , };
@@ -769,7 +770,7 @@ static void arm_smmu_init_context_bank(struct arm_smmu_domain *smmu_domain,/* CBAR */ reg = cfg->cbar; -if (smmu->version ==ARM_SMMU_V1 ) +if (smmu->version <ARM_SMMU_V2 ) reg |= cfg->irptndx <<CBAR_IRPTNDX_SHIFT ;/*
@@ -942,7 +943,7 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain, goto out_unlock; cfg->cbndx = ret; -if (smmu->version ==ARM_SMMU_V1 ) { +if (smmu->version <ARM_SMMU_V2 ) { cfg->irptndx =atomic_inc_return (&smmu->irptndx); cfg->irptndx %= smmu->num_context_irqs; }else {
@@ -1627,7 +1628,8 @@ static int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu)bool cttw_dt, cttw_reg;dev_notice (smmu->dev,"probing hardware configuration...\n" ); -dev_notice (smmu->dev,"SMMUv%d with:\n" , smmu->version); +dev_notice (smmu->dev,"SMMUv%d with:\n" , + smmu->version ==ARM_SMMU_V2 ?2 :1 );/* ID0 */ id =readl_relaxed (gr0_base +ARM_SMMU_GR0_ID0 );
@@ -1659,7 +1661,8 @@ static int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu)return -ENODEV ; } -if ((id &ID0_S1TS ) && ((smmu->version ==1 ) || !(id &ID0_ATOSNS ))) { +if ((id &ID0_S1TS ) && + ((smmu->version <ARM_SMMU_V2 ) || !(id &ID0_ATOSNS ))) { smmu->features |=ARM_SMMU_FEAT_TRANS_OPS ;dev_notice (smmu->dev,"\taddress translation ops\n" ); }
@@ -1774,8 +1777,10 @@ static int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu)dev_warn (smmu->dev,"failed to set DMA mask for table walker\n" ); -if (smmu->version ==ARM_SMMU_V1 ) { +if (smmu->version <ARM_SMMU_V2 ) { smmu->va_size = smmu->ipa_size; +if (smmu->version ==ARM_SMMU_V1_64K ) + smmu->features |=ARM_SMMU_FEAT_FMT_AARCH64_64K ; }else { size = (id >>ID2_UBS_SHIFT ) &ID2_UBS_MASK ; smmu->va_size =arm_smmu_id_size_to_bits (size);
@@ -1827,6 +1832,7 @@ static struct arm_smmu_match_data name = { .version = ver, .model = imp }ARM_SMMU_MATCH_DATA (smmu_generic_v1,ARM_SMMU_V1 ,GENERIC_SMMU );ARM_SMMU_MATCH_DATA (smmu_generic_v2,ARM_SMMU_V2 ,GENERIC_SMMU ); +ARM_SMMU_MATCH_DATA (arm_mmu401,ARM_SMMU_V1_64K ,GENERIC_SMMU );ARM_SMMU_MATCH_DATA (arm_mmu500,ARM_SMMU_V2 ,ARM_MMU500 );ARM_SMMU_MATCH_DATA (cavium_smmuv2,ARM_SMMU_V2 ,CAVIUM_SMMUV2 );
@@ -1834,7 +1840,7 @@ static const struct of_device_id arm_smmu_of_match[] = { { .compatible = "arm,smmu-v1", .data = &smmu_generic_v1 }, { .compatible = "arm,smmu-v2", .data = &smmu_generic_v2 }, { .compatible = "arm,mmu-400", .data = &smmu_generic_v1 }, - { .compatible = "arm,mmu-401", .data = &smmu_generic_v1 }, + { .compatible = "arm,mmu-401", .data = &arm_mmu401 }, { .compatible = "arm,mmu-500", .data = &arm_mmu500 }, { .compatible = "cavium,smmu-v2", .data = &cavium_smmuv2 }, { },
@@ -1928,7 +1934,7 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev)parse_driver_options (smmu); -if (smmu->version >ARM_SMMU_V1 && +if (smmu->version ==ARM_SMMU_V2 && smmu->num_context_banks != smmu->num_context_irqs) { dev_err(dev, "found only %d context interrupt(s) but %d required\n",
Signature · 1 line
2.7.3.dirty