Hi Robin,
On 2021/2/6 0:11, Robin Murphy wrote:
> On 2021-02-05 11:48, Robin Murphy wrote:
>> On 2021-02-05 09:13, Keqian Zhu wrote:
>>> Hi Robin and Jean,
>>>
>>> On 2021/2/5 3:50, Robin Murphy wrote:
>>>> On 2021-01-28 15:17, Keqian Zhu wrote:
>>>>> From: jiangkunkun <jiangkun...@huawei.com>
>>>>>
>>>>> The SMMU which supports HTTU (Hardware Translation Table Update) can
>>>>> update the access flag and the dirty state of TTD by hardware. It is
>>>>> essential to track dirty pages of DMA.
>>>>>
>>>>> This adds feature detection, none functional change.
>>>>>
>>>>> Co-developed-by: Keqian Zhu <zhukeqi...@huawei.com>
>>>>> Signed-off-by: Kunkun Jiang <jiangkun...@huawei.com>
>>>>> ---
>>>>> drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 16 ++++++++++++++++
>>>>> drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 8 ++++++++
>>>>> include/linux/io-pgtable.h | 1 +
>>>>> 3 files changed, 25 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 8ca7415d785d..0f0fe71cc10d 100644
>>>>> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>>>>> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>>>>> @@ -1987,6 +1987,7 @@ static int arm_smmu_domain_finalise(struct
>>>>> iommu_domain *domain,
>>>>> .pgsize_bitmap = smmu->pgsize_bitmap,
>>>>> .ias = ias,
>>>>> .oas = oas,
>>>>> + .httu_hd = smmu->features & ARM_SMMU_FEAT_HTTU_HD,
>>>>> .coherent_walk = smmu->features & ARM_SMMU_FEAT_COHERENCY,
>>>>> .tlb = &arm_smmu_flush_ops,
>>>>> .iommu_dev = smmu->dev,
>>>>> @@ -3224,6 +3225,21 @@ static int arm_smmu_device_hw_probe(struct
>>>>> arm_smmu_device *smmu)
>>>>> if (reg & IDR0_HYP)
>>>>> smmu->features |= ARM_SMMU_FEAT_HYP;
>>>>> + switch (FIELD_GET(IDR0_HTTU, reg)) {
>>>>
>>>> We need to accommodate the firmware override as well if we need this to be
>>>> meaningful. Jean-Philippe is already carrying a suitable patch in the SVA
>>>> stack[1].
>>> Robin, Thanks for pointing it out.
>>>
>>> Jean, I see that the IORT HTTU flag overrides the hardware register info
>>> unconditionally. I have some concern about it:
>>>
>>> If the override flag has HTTU but hardware doesn't support it, then driver
>>> will use this feature but receive access fault or permission fault from
>>> SMMU unexpectedly.
>>> 1) If IOPF is not supported, then kernel can not work normally.
>>> 2) If IOPF is supported, kernel will perform useless actions, such as HTTU
>>> based dma dirty tracking (this series).
>>
>> Yes, if the IORT describes the SMMU incorrectly, things will not work well.
>> Just like if it describes the wrong base address or the wrong interrupt
>> numbers, things will also not work well. The point is that incorrect
>> firmware can be updated in the field fairly easily; incorrect hardware can
>> not.
>>
>> Say the SMMU designer hard-codes the ID register field to 0x2 because the
>> SMMU itself is capable of HTTU, and they assume it's always going to be
>> wired up coherently, but then a customer integrates it to a non-coherent
>> interconnect. Firmware needs to override that value to prevent an OS
>> thinking that the claimed HTTU capability is ever going to work.
>>
>> Or say the SMMU *is* integrated correctly, but due to an erratum discovered
>> later in the interconnect or SMMU itself, it turns out DBM doesn't always
>> work reliably, but AF is still OK. Firmware needs to downgrade the indicated
>> level of support from that which was intended to that which works reliably.
>>
>> Or say someone forgets to set an integration tieoff so their SMMU reports
>> 0x0 even though it and the interconnect *can* happily support HTTU. In that
>> case, firmware may want to upgrade the value to *allow* an OS to use HTTU
>> despite the ID register being wrong.
>>
>>> As the IORT spec doesn't give an explicit explanation for HTTU override,
>>> can we comprehend it as a mask for HTTU related hardware register?
>>> So the logic becomes: smmu->feature = HTTU override & IDR0_HTTU;
>>
>> No, it literally states that the OS must use the value of the firmware field
>> *instead* of the value from the hardware field.
>
> Oops, apologies for an oversight there - I've been reviewing IORT spec
> updates lately so naturally had the newest version open already. Turns out
> these descriptions were only clarified in the most recent release, so if you
> were looking at an older document they *were* horribly vague.
Yep, my local version is E which was released at July 2020. I download the
version E.a just now, thanks. ;-)
Thanks,
Keqian
