| Message ID | 1407891099-24641-4-git-send-email-mitchelh@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Aug 13, 2014 at 01:51:36AM +0100, Mitchel Humpherys wrote: > Currently, we provide the iommu_ops.iova_to_phys service by doing a > table walk in software to translate IO virtual addresses to physical > addresses. On SMMUs that support it, it can be useful to ask the SMMU > itself to do the translation. This can be used to warm the TLBs for an > SMMU. It can also be useful for testing and hardware validation. I'm not really sold on the usefulness of this feature. If you want hardware validation features, I'd rather do something through debugfs, but your use-case for warming the TLBs is intriguing. Do you have an example use-case with performance figures? > Since the address translation registers are optional on SMMUv2, only > enable hardware translations when using SMMUv1 or when SMMU_IDR0.S1TS=1 > and SMMU_IDR0.ATOSNS=0, as described in the ARM SMMU v1-v2 spec. [...] > +static phys_addr_t arm_smmu_iova_to_phys_hard(struct iommu_domain *domain, > + dma_addr_t iova) > +{ > + struct arm_smmu_domain *smmu_domain = domain->priv; > + struct arm_smmu_device *smmu = smmu_domain->smmu; > + struct arm_smmu_cfg *cfg = &smmu_domain->cfg; > + struct device *dev = smmu->dev; > + void __iomem *cb_base; > + int count = 0; > + u64 phys; > + > + arm_smmu_enable_clocks(smmu); > + > + cb_base = ARM_SMMU_CB_BASE(smmu) + ARM_SMMU_CB(smmu, cfg->cbndx); > + > + if (smmu->version == 1) { > + u32 reg = iova & 0xFFFFF000; > + writel_relaxed(reg, cb_base + ARM_SMMU_CB_ATS1PR_LO); > + } else { > + u64 reg = iova & 0xfffffffffffff000; > + writeq_relaxed(reg, cb_base + ARM_SMMU_CB_ATS1PR_LO); We don't have writeq for arch/arm/. > + } > + > + mb(); Why? > + while (readl_relaxed(cb_base + ARM_SMMU_CB_ATSR) & ATSR_ACTIVE) { > + if (++count == ATSR_LOOP_TIMEOUT) { > + dev_err(dev, > + "iova to phys timed out on 0x%pa for %s. Falling back to software table walk.\n", > + &iova, dev_name(dev)); > + arm_smmu_disable_clocks(smmu); > + return arm_smmu_iova_to_phys_soft(domain, iova); > + } > + cpu_relax(); > + } Do you know what happened to Olav's patches to make this sort of code generic? > @@ -2005,6 +2073,11 @@ int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu) > return -ENODEV; > } > > + if (smmu->version == 1 || (!(id & ID0_ATOSNS) && (id & ID0_S1TS))) { Are you sure about this? The v2 spec says that is ATOSNS is clear then S1TS is also clear. Will
On Tue, Aug 19 2014 at 05:44:32 AM, Will Deacon <will.deacon@arm.com> wrote: > On Wed, Aug 13, 2014 at 01:51:36AM +0100, Mitchel Humpherys wrote: >> Currently, we provide the iommu_ops.iova_to_phys service by doing a >> table walk in software to translate IO virtual addresses to physical >> addresses. On SMMUs that support it, it can be useful to ask the SMMU >> itself to do the translation. This can be used to warm the TLBs for an >> SMMU. It can also be useful for testing and hardware validation. > > I'm not really sold on the usefulness of this feature. If you want hardware > validation features, I'd rather do something through debugfs, but your > use-case for warming the TLBs is intriguing. Do you have an example use-case > with performance figures? I'm afraid I don't have an example use case or performance numbers at the moment... >> Since the address translation registers are optional on SMMUv2, only >> enable hardware translations when using SMMUv1 or when SMMU_IDR0.S1TS=1 >> and SMMU_IDR0.ATOSNS=0, as described in the ARM SMMU v1-v2 spec. > > [...] > >> +static phys_addr_t arm_smmu_iova_to_phys_hard(struct iommu_domain *domain, >> + dma_addr_t iova) >> +{ >> + struct arm_smmu_domain *smmu_domain = domain->priv; >> + struct arm_smmu_device *smmu = smmu_domain->smmu; >> + struct arm_smmu_cfg *cfg = &smmu_domain->cfg; >> + struct device *dev = smmu->dev; >> + void __iomem *cb_base; >> + int count = 0; >> + u64 phys; >> + >> + arm_smmu_enable_clocks(smmu); >> + >> + cb_base = ARM_SMMU_CB_BASE(smmu) + ARM_SMMU_CB(smmu, cfg->cbndx); >> + >> + if (smmu->version == 1) { >> + u32 reg = iova & 0xFFFFF000; >> + writel_relaxed(reg, cb_base + ARM_SMMU_CB_ATS1PR_LO); >> + } else { >> + u64 reg = iova & 0xfffffffffffff000; >> + writeq_relaxed(reg, cb_base + ARM_SMMU_CB_ATS1PR_LO); > > We don't have writeq for arch/arm/. Ah yes looks like this is an MSM-ism that never made it upstream since it wouldn't be guaranteed to be atomic. I'll make sure to do arm32 compiles on upstream kernels for future patches, sorry! I guess we could use <asm-generic/io-64-nonatomic-lo-hi.h> but I can also re-work this to be two separate writel's. >> + } >> + >> + mb(); > > Why? My thought was that if we start polling ATSR_ACTIVE prematurely (before the write to ATS1PR actually finishes) all heck could break loose? Not sure if that's a bogus assumption due to device memory being strongly ordered? >> + while (readl_relaxed(cb_base + ARM_SMMU_CB_ATSR) & ATSR_ACTIVE) { >> + if (++count == ATSR_LOOP_TIMEOUT) { >> + dev_err(dev, >> + "iova to phys timed out on 0x%pa for %s. Falling back to software table walk.\n", >> + &iova, dev_name(dev)); >> + arm_smmu_disable_clocks(smmu); >> + return arm_smmu_iova_to_phys_soft(domain, iova); >> + } >> + cpu_relax(); >> + } > > Do you know what happened to Olav's patches to make this sort of code > generic? I assume you're talking about this, right? http://lists.infradead.org/pipermail/linux-arm-kernel/2014-June/267943.html Yeah looks like he never sent an update since it was part of a series that wasn't going to make it in (the qsmmu driver). I can always bring that patch (actually Matt Wagantall's patch) in here and rework this to use that. > >> @@ -2005,6 +2073,11 @@ int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu) >> return -ENODEV; >> } >> >> + if (smmu->version == 1 || (!(id & ID0_ATOSNS) && (id & ID0_S1TS))) { > > Are you sure about this? The v2 spec says that is ATOSNS is clear then S1TS > is also clear. I was looking at Section 4.1.1 of ARM IHI 0062C ID091613 which states: In SMMUv2, the address translation registers are OPTIONAL. The address translation registers are implemented only when both: o The SMMU_IDR0.S1TS bit is set to 1. o The SMMU_IDR0.ATOSNS bit is set to 0. I assume you're referring to section 9.6.1 of the same document: ATOSNS, bit[26] Address Translation Operations Not Supported. The possible values of this bit are: 0 Address translation operations are supported. Stage 1 translation is not supported, that is, the S1TS bit is set to 0. 1 Address translation operations are not supported. Stage 1 translation is supported, that is, the S1TS bit is set to 1. If that really means that S1TS and ATOSNS always have the same value then Section 4.1.1 doesn't make any sense. Or am I missing something? -Mitch
Hi Mitch, On Tue, Aug 19, 2014 at 07:12:41PM +0100, Mitchel Humpherys wrote: > On Tue, Aug 19 2014 at 05:44:32 AM, Will Deacon <will.deacon@arm.com> wrote: > > We don't have writeq for arch/arm/. > > Ah yes looks like this is an MSM-ism that never made it upstream since > it wouldn't be guaranteed to be atomic. I'll make sure to do arm32 > compiles on upstream kernels for future patches, sorry! > > I guess we could use <asm-generic/io-64-nonatomic-lo-hi.h> but I can > also re-work this to be two separate writel's. Yeah, just do two writels. > >> + } > >> + > >> + mb(); > > > > Why? > > My thought was that if we start polling ATSR_ACTIVE prematurely (before > the write to ATS1PR actually finishes) all heck could break loose? Not > sure if that's a bogus assumption due to device memory being strongly > ordered? I think the device-memory guarantees should be enough. If not, we need a comment explaining why. > >> + while (readl_relaxed(cb_base + ARM_SMMU_CB_ATSR) & ATSR_ACTIVE) { > >> + if (++count == ATSR_LOOP_TIMEOUT) { > >> + dev_err(dev, > >> + "iova to phys timed out on 0x%pa for %s. Falling back to software table walk.\n", > >> + &iova, dev_name(dev)); > >> + arm_smmu_disable_clocks(smmu); > >> + return arm_smmu_iova_to_phys_soft(domain, iova); > >> + } > >> + cpu_relax(); > >> + } > > > > Do you know what happened to Olav's patches to make this sort of code > > generic? > > I assume you're talking about this, right? > > http://lists.infradead.org/pipermail/linux-arm-kernel/2014-June/267943.html > > Yeah looks like he never sent an update since it was part of a series > that wasn't going to make it in (the qsmmu driver). I can always bring > that patch (actually Matt Wagantall's patch) in here and rework this to > use that. Yup, I think it would be useful to revive that as a separate series. > > > >> @@ -2005,6 +2073,11 @@ int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu) > >> return -ENODEV; > >> } > >> > >> + if (smmu->version == 1 || (!(id & ID0_ATOSNS) && (id & ID0_S1TS))) { > > > > Are you sure about this? The v2 spec says that is ATOSNS is clear then S1TS > > is also clear. > > I was looking at Section 4.1.1 of ARM IHI 0062C ID091613 which states: > > In SMMUv2, the address translation registers are OPTIONAL. The > address translation registers are implemented only when both: > > o The SMMU_IDR0.S1TS bit is set to 1. > o The SMMU_IDR0.ATOSNS bit is set to 0. > > I assume you're referring to section 9.6.1 of the same document: > > ATOSNS, bit[26] > Address Translation Operations Not Supported. The possible values of > this bit are: > > 0 Address translation operations are supported. Stage 1 > translation is not supported, that is, the S1TS bit is set to 0. > > 1 Address translation operations are not supported. Stage 1 > translation is supported, that is, the S1TS bit is set to 1. > > If that really means that S1TS and ATOSNS always have the same value > then Section 4.1.1 doesn't make any sense. Or am I missing something? I'll get this checked, as those two paragraphs don't make an awful lot of sense together. Will
On Tue, Aug 26, 2014 at 02:54:51PM +0100, Will Deacon wrote: > On Tue, Aug 19, 2014 at 07:12:41PM +0100, Mitchel Humpherys wrote: > > On Tue, Aug 19 2014 at 05:44:32 AM, Will Deacon <will.deacon@arm.com> wrote: > > >> @@ -2005,6 +2073,11 @@ int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu) > > >> return -ENODEV; > > >> } > > >> > > >> + if (smmu->version == 1 || (!(id & ID0_ATOSNS) && (id & ID0_S1TS))) { > > > > > > Are you sure about this? The v2 spec says that is ATOSNS is clear then S1TS > > > is also clear. > > > > I was looking at Section 4.1.1 of ARM IHI 0062C ID091613 which states: > > > > In SMMUv2, the address translation registers are OPTIONAL. The > > address translation registers are implemented only when both: > > > > o The SMMU_IDR0.S1TS bit is set to 1. > > o The SMMU_IDR0.ATOSNS bit is set to 0. > > > > I assume you're referring to section 9.6.1 of the same document: > > > > ATOSNS, bit[26] > > Address Translation Operations Not Supported. The possible values of > > this bit are: > > > > 0 Address translation operations are supported. Stage 1 > > translation is not supported, that is, the S1TS bit is set to 0. > > > > 1 Address translation operations are not supported. Stage 1 > > translation is supported, that is, the S1TS bit is set to 1. > > > > If that really means that S1TS and ATOSNS always have the same value > > then Section 4.1.1 doesn't make any sense. Or am I missing something? > > I'll get this checked, as those two paragraphs don't make an awful lot of > sense together. Right, word from above says that ATOS is implemented iff: IDR0.S1TS == 1 && IDR0.ATOSNS == 0 Basically, ATOS only works for stage-1 when it's present, so that explains the dependency. Looking at the piece of diff at the top of this mail, I think that means your code is correct Will
diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c index 7fdc58d8f8..63c6707fad 100644 --- a/drivers/iommu/arm-smmu.c +++ b/drivers/iommu/arm-smmu.c @@ -246,11 +246,17 @@ #define ARM_SMMU_CB_TTBR0_HI 0x24 #define ARM_SMMU_CB_TTBCR 0x30 #define ARM_SMMU_CB_S1_MAIR0 0x38 +#define ARM_SMMU_CB_PAR_LO 0x50 +#define ARM_SMMU_CB_PAR_HI 0x54 #define ARM_SMMU_CB_FSR 0x58 #define ARM_SMMU_CB_FAR_LO 0x60 #define ARM_SMMU_CB_FAR_HI 0x64 #define ARM_SMMU_CB_FSYNR0 0x68 #define ARM_SMMU_CB_S1_TLBIASID 0x610 +#define ARM_SMMU_CB_ATS1PR_LO 0x800 +#define ARM_SMMU_CB_ATS1PR_HI 0x804 +#define ARM_SMMU_CB_ATSR 0x8f0 +#define ATSR_LOOP_TIMEOUT 1000000 /* 1s! */ #define SCTLR_S1_ASIDPNE (1 << 12) #define SCTLR_CFCFG (1 << 7) @@ -262,6 +268,10 @@ #define SCTLR_M (1 << 0) #define SCTLR_EAE_SBOP (SCTLR_AFE | SCTLR_TRE) +#define CB_PAR_F (1 << 0) + +#define ATSR_ACTIVE (1 << 0) + #define RESUME_RETRY (0 << 0) #define RESUME_TERMINATE (1 << 0) @@ -375,6 +385,7 @@ struct arm_smmu_device { #define ARM_SMMU_FEAT_TRANS_S1 (1 << 2) #define ARM_SMMU_FEAT_TRANS_S2 (1 << 3) #define ARM_SMMU_FEAT_TRANS_NESTED (1 << 4) +#define ARM_SMMU_FEAT_TRANS_OPS (1 << 5) u32 features; #define ARM_SMMU_OPT_SECURE_CFG_ACCESS (1 << 0) @@ -1653,7 +1664,7 @@ static size_t arm_smmu_unmap(struct iommu_domain *domain, unsigned long iova, return ret ? 0 : size; } -static phys_addr_t arm_smmu_iova_to_phys(struct iommu_domain *domain, +static phys_addr_t arm_smmu_iova_to_phys_soft(struct iommu_domain *domain, dma_addr_t iova) { pgd_t *pgdp, pgd; @@ -1686,6 +1697,63 @@ static phys_addr_t arm_smmu_iova_to_phys(struct iommu_domain *domain, return __pfn_to_phys(pte_pfn(pte)) | (iova & ~PAGE_MASK); } +static phys_addr_t arm_smmu_iova_to_phys_hard(struct iommu_domain *domain, + dma_addr_t iova) +{ + struct arm_smmu_domain *smmu_domain = domain->priv; + struct arm_smmu_device *smmu = smmu_domain->smmu; + struct arm_smmu_cfg *cfg = &smmu_domain->cfg; + struct device *dev = smmu->dev; + void __iomem *cb_base; + int count = 0; + u64 phys; + + arm_smmu_enable_clocks(smmu); + + cb_base = ARM_SMMU_CB_BASE(smmu) + ARM_SMMU_CB(smmu, cfg->cbndx); + + if (smmu->version == 1) { + u32 reg = iova & 0xFFFFF000; + writel_relaxed(reg, cb_base + ARM_SMMU_CB_ATS1PR_LO); + } else { + u64 reg = iova & 0xfffffffffffff000; + writeq_relaxed(reg, cb_base + ARM_SMMU_CB_ATS1PR_LO); + } + + mb(); + while (readl_relaxed(cb_base + ARM_SMMU_CB_ATSR) & ATSR_ACTIVE) { + if (++count == ATSR_LOOP_TIMEOUT) { + dev_err(dev, + "iova to phys timed out on 0x%pa for %s. Falling back to software table walk.\n", + &iova, dev_name(dev)); + arm_smmu_disable_clocks(smmu); + return arm_smmu_iova_to_phys_soft(domain, iova); + } + cpu_relax(); + } + + phys = readl_relaxed(cb_base + ARM_SMMU_CB_PAR_LO); + phys |= ((u64) readl_relaxed(cb_base + ARM_SMMU_CB_PAR_HI)) << 32; + + if (phys & CB_PAR_F) { + dev_err(dev, "translation fault on %s!\n", dev_name(dev)); + dev_err(dev, "PAR = 0x%llx\n", phys); + } + phys = (phys & 0xFFFFFFF000ULL) | (iova & 0x00000FFF); + + arm_smmu_disable_clocks(smmu); + return phys; +} + +static phys_addr_t arm_smmu_iova_to_phys(struct iommu_domain *domain, + dma_addr_t iova) +{ + struct arm_smmu_domain *smmu_domain = domain->priv; + if (smmu_domain->smmu->features & ARM_SMMU_FEAT_TRANS_OPS) + return arm_smmu_iova_to_phys_hard(domain, iova); + return arm_smmu_iova_to_phys_soft(domain, iova); +} + static int arm_smmu_domain_has_cap(struct iommu_domain *domain, unsigned long cap) { @@ -2005,6 +2073,11 @@ int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu) return -ENODEV; } + if (smmu->version == 1 || (!(id & ID0_ATOSNS) && (id & ID0_S1TS))) { + smmu->features |= ARM_SMMU_FEAT_TRANS_OPS; + dev_notice(smmu->dev, "\taddress translation ops\n"); + } + if (id & ID0_CTTW) { smmu->features |= ARM_SMMU_FEAT_COHERENT_WALK; dev_notice(smmu->dev, "\tcoherent table walk\n");
Currently, we provide the iommu_ops.iova_to_phys service by doing a table walk in software to translate IO virtual addresses to physical addresses. On SMMUs that support it, it can be useful to ask the SMMU itself to do the translation. This can be used to warm the TLBs for an SMMU. It can also be useful for testing and hardware validation. Since the address translation registers are optional on SMMUv2, only enable hardware translations when using SMMUv1 or when SMMU_IDR0.S1TS=1 and SMMU_IDR0.ATOSNS=0, as described in the ARM SMMU v1-v2 spec. Signed-off-by: Mitchel Humpherys <mitchelh@codeaurora.org> --- drivers/iommu/arm-smmu.c | 75 +++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 74 insertions(+), 1 deletion(-)