On Tue, Sep 30 2014 at 03:23:34 AM, Will Deacon <will.dea...@arm.com> wrote:
> Hi Mitch,
>
> On Sun, Sep 28, 2014 at 04:27:29AM +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.
>>
>> 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;
>> + u32 tmp;
>> + u64 phys;
>> +
>> + cb_base = ARM_SMMU_CB_BASE(smmu) + ARM_SMMU_CB(smmu, cfg->cbndx);
>> +
>> + if (smmu->version == 1) {
>> + u32 reg = iova & ~0xFFF;
>
> Cosmetic comment, but hex constants are lowercase everywhere else in the
> file.
Ah, woops. Let me fix that.
>
>> + writel_relaxed(reg, cb_base + ARM_SMMU_CB_ATS1PR_LO);
>> + } else {
>> + u32 reg = iova & ~0xFFF;
>> + writel_relaxed(reg, cb_base + ARM_SMMU_CB_ATS1PR_LO);
>> + reg = (iova & ~0xFFF) >> 32;
>> + writel_relaxed(reg, cb_base + ARM_SMMU_CB_ATS1PR_HI);
>> + }
>> +
>> + if (readl_poll_timeout(cb_base + ARM_SMMU_CB_ATSR, tmp,
>> + !(tmp & ATSR_ACTIVE), 10, ATSR_LOOP_TIMEOUT)) {
>> + dev_err(dev,
>> + "iova to phys timed out on 0x%pa. Falling back to
>> software table walk.\n",
>> + &iova);
>> + return arm_smmu_iova_to_phys_soft(domain, iova);
>> + }
>> +
>> + phys = readl_relaxed(cb_base + ARM_SMMU_CB_PAR_LO);
>> + phys |= ((u64) readl_relaxed(cb_base + ARM_SMMU_CB_PAR_HI)) << 32;
>
> The absence of locking in this function concerns me a bit. For the software
> implementation, we're just reading page tables, but here we're writing ATS
> registers and I think we need to ensure serialisation against another
> iova_to_phys on the same domain.
Good catch, let me take the domain lock here. I'll also have to move to
readl_poll_timeout_atomic since the domain lock is a spinlock.
>
>> + if (phys & CB_PAR_F) {
>> + dev_err(dev, "translation fault!\n");
>> + dev_err(dev, "PAR = 0x%llx\n", phys);
>> + }
>> + phys = (phys & 0xFFFFFFF000ULL) | (iova & 0x00000FFF);
>> +
>> + return phys;
>
> You can return phys == 0 on failure (at least, the callers in kvm and vfio
> treat this as an error).
Ah yes, I agree that a 0 return value from iommu_iova_to_phys appears to
be treated as an error. Let me fix that.
-Mitch
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
