Re: [PATCH RFC 16/19] iommu/arm-smmu-v3: Enable HTTU for stage1 with io-pgtable mapping

Robin Murphy
2022-04-29T12:47:00Z

On 2022-04-29 13:10, Joao Martins wrote:
> On 4/29/22 12:35, Robin Murphy wrote:
>> On 2022-04-28 22:09, Joao Martins wrote:
>>> From: Kunkun Jiang <jiangkunkun@huawei.com>
>>>
>>> As nested mode is not upstreamed now, we just aim to support dirty
>>> log tracking for stage1 with io-pgtable mapping (means not support
>>> SVA mapping). If HTTU is supported, we enable HA/HD bits in the SMMU
>>> CD and transfer ARM_HD quirk to io-pgtable.
>>>
>>> We additionally filter out HD|HA if not supportted. The CD.HD bit
>>> is not particularly useful unless we toggle the DBM bit in the PTE
>>> entries.
>>>
>>> Co-developed-by: Keqian Zhu <zhukeqian1@huawei.com>
>>> Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com>
>>> Signed-off-by: Kunkun Jiang <jiangkunkun@huawei.com>
>>> [joaomart:Convey HD|HA bits over to the context descriptor
>>> and update commit message]
>>> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
>>> ---
>>> drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 11 +++++++++++
>>> drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 3 +++
>>> include/linux/io-pgtable.h | 1 +
>>> 3 files changed, 15 insertions(+)
>>>
>>> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>>> index 1ca72fcca930..5f728f8f20a2 100644
>>> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>>> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>>> @@ -1077,10 +1077,18 @@ int arm_smmu_write_ctx_desc(struct arm_smmu_domain *smmu_domain, int ssid,
>>> * this substream's traffic
>>> */
>>> } else { /* (1) and (2) */
>>> + struct arm_smmu_device *smmu = smmu_domain->smmu;
>>> + u64 tcr = cd->tcr;
>>> +
>>> cdptr[1] = cpu_to_le64(cd->ttbr & CTXDESC_CD_1_TTB0_MASK);
>>> cdptr[2] = 0;
>>> cdptr[3] = cpu_to_le64(cd->mair);
>>>
>>> + if (!(smmu->features & ARM_SMMU_FEAT_HD))
>>> + tcr &= ~CTXDESC_CD_0_TCR_HD;
>>> + if (!(smmu->features & ARM_SMMU_FEAT_HA))
>>> + tcr &= ~CTXDESC_CD_0_TCR_HA;
>>
>> This is very backwards...
>>
> Yes.
>
>>> +
>>> /*
>>> * STE is live, and the SMMU might read dwords of this CD in any
>>> * order. Ensure that it observes valid values before reading
>>> @@ -2100,6 +2108,7 @@ static int arm_smmu_domain_finalise_s1(struct arm_smmu_domain *smmu_domain,
>>> FIELD_PREP(CTXDESC_CD_0_TCR_ORGN0, tcr->orgn) |
>>> FIELD_PREP(CTXDESC_CD_0_TCR_SH0, tcr->sh) |
>>> FIELD_PREP(CTXDESC_CD_0_TCR_IPS, tcr->ips) |
>>> + CTXDESC_CD_0_TCR_HA | CTXDESC_CD_0_TCR_HD |
>>
>> ...these should be set in io-pgtable's TCR value *if* io-pgatble is
>> using DBM, then propagated through from there like everything else.
>>
>
> So the DBM bit superseedes the TCR bit -- that's strage? say if you mark a PTE as
> writeable-clean with DBM set but TCR.HD unset .. then won't trigger a perm-fault?
> I need to re-read that section of the manual, as I didn't get the impression above.

No, architecturally, the {TCR,CD}.HD bit is still the "master switch"
for whether the DBM field in PTEs is interpreted or not, but in terms of
our abstraction, we only need to care about setting HD if io-pgtable is
actually going to want to use DBM, so we may as well leave it to
io-pgtable to tell us canonically. The logical interface here in general
is that we use the initial io_pgtable_cfg to tell it what it *can* use,
but then we read back afterwards to see exactly what it has chosen to
do, and I think HA/HD also fit perfectly into that paradigm.

Robin.

>>> CTXDESC_CD_0_TCR_EPD1 | CTXDESC_CD_0_AA64;
>>> cfg->cd.mair = pgtbl_cfg->arm_lpae_s1_cfg.mair;
>>>
>>> @@ -2203,6 +2212,8 @@ static int arm_smmu_domain_finalise(struct iommu_domain *domain,
>>> .iommu_dev = smmu->dev,
>>> };
>>>
>>> + if (smmu->features & ARM_SMMU_FEAT_HD)
>>> + pgtbl_cfg.quirks |= IO_PGTABLE_QUIRK_ARM_HD;
>>
>> You need to depend on ARM_SMMU_FEAT_COHERENCY for this as well, not
>> least because you don't have any of the relevant business for
>> synchronising non-coherent PTEs in your walk functions, but it's also
>> implementation-defined whether HTTU even operates on non-cacheable
>> pagetables, and frankly you just don't want to go there ;)
>>
> /me nods OK.
>
>> Robin.
>>
>>> if (smmu->features & ARM_SMMU_FEAT_BBML1)
>>> pgtbl_cfg.quirks |= IO_PGTABLE_QUIRK_ARM_BBML1;
>>> else if (smmu->features & ARM_SMMU_FEAT_BBML2)
>>> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
>>> index e15750be1d95..ff32242f2fdb 100644
>>> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
>>> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
>>> @@ -292,6 +292,9 @@
>>> #define CTXDESC_CD_0_TCR_IPS GENMASK_ULL(34, 32)
>>> #define CTXDESC_CD_0_TCR_TBI0 (1ULL << 38)
>>>
>>> +#define CTXDESC_CD_0_TCR_HA (1UL << 43)
>>> +#define CTXDESC_CD_0_TCR_HD (1UL << 42)
>>> +
>>> #define CTXDESC_CD_0_AA64 (1UL << 41)
>>> #define CTXDESC_CD_0_S (1UL << 44)
>>> #define CTXDESC_CD_0_R (1UL << 45)
>>> diff --git a/include/linux/io-pgtable.h b/include/linux/io-pgtable.h
>>> index d7626ca67dbf..a11902ae9cf1 100644
>>> --- a/include/linux/io-pgtable.h
>>> +++ b/include/linux/io-pgtable.h
>>> @@ -87,6 +87,7 @@ struct io_pgtable_cfg {
>>> #define IO_PGTABLE_QUIRK_ARM_OUTER_WBWA BIT(6)
>>> #define IO_PGTABLE_QUIRK_ARM_BBML1 BIT(7)
>>> #define IO_PGTABLE_QUIRK_ARM_BBML2 BIT(8)
>>> + #define IO_PGTABLE_QUIRK_ARM_HD BIT(9)
>>>
>>> unsigned long quirks;
>>> unsigned long pgsize_bitmap;