Re: [RFC PATCH 01/11] iommu/arm-smmu-v3: Add feature detection for HTTU
Hi Robin,
I am going to send v2 at next week, to addresses these issues reported by you.
Many thanks!
And do you have any further comments on patch #4 #5 and #6?
Thanks,
Keqian
On 2021/2/5 3:50, Robin Murphy wrote:
> On 2021-01-28 15:17, Keqian Zhu wrote:
>> From: jiangkunkun
>>
>> 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
>> Signed-off-by: Kunkun Jiang
>> ---
>> 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= _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].
>
>> +case IDR0_HTTU_NONE:
>> +break;
>> +case IDR0_HTTU_HA:
>> +smmu->features |= ARM_SMMU_FEAT_HTTU_HA;
>> +break;
>> +case IDR0_HTTU_HAD:
>> +smmu->features |= ARM_SMMU_FEAT_HTTU_HA;
>> +smmu->features |= ARM_SMMU_FEAT_HTTU_HD;
>> +break;
>> +default:
>> +dev_err(smmu->dev, "unknown/unsupported HTTU!\n");
>> +return -ENXIO;
>> +}
>> +
>> /*
>>* The coherency feature as set by FW is used in preference to the ID
>>* register, but warn on mismatch.
>> 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 96c2e9565e00..e91bea44519e 100644
>> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
>> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
>> @@ -33,6 +33,10 @@
>> #define IDR0_ASID16(1 << 12)
>> #define IDR0_ATS(1 << 10)
>> #define IDR0_HYP(1 << 9)
>> +#define IDR0_HTTUGENMASK(7, 6)
>> +#define IDR0_HTTU_NONE0
>> +#define IDR0_HTTU_HA1
>> +#define IDR0_HTTU_HAD2
>> #define IDR0_COHACC(1 << 4)
>> #define IDR0_TTFGENMASK(3, 2)
>> #define IDR0_TTF_AARCH642
>> @@ -286,6 +290,8 @@
>> #define CTXDESC_CD_0_TCR_TBI0(1ULL << 38)
>> #define CTXDESC_CD_0_AA64(1UL << 41)
>> +#define CTXDESC_CD_0_HD(1UL << 42)
>> +#define CTXDESC_CD_0_HA(1UL << 43)
>> #define CTXDESC_CD_0_S(1UL << 44)
>> #define CTXDESC_CD_0_R(1UL << 45)
>> #define CTXDESC_CD_0_A(1UL << 46)
>> @@ -604,6 +610,8 @@ struct arm_smmu_device {
>> #define ARM_SMMU_FEAT_RANGE_INV(1 << 15)
>> #define ARM_SMMU_FEAT_BTM(1 << 16)
>> #define ARM_SMMU_FEAT_SVA(1 << 17)
>> +#define ARM_SMMU_FEAT_HTTU_HA(1 << 18)
>> +#define ARM_SMMU_FEAT_HTTU_HD(1 << 19)
>> u32features;
>> #define ARM_SMMU_OPT_SKIP_PREFETCH(1 << 0)
>> diff --git a/include/linux/io-pgtable.h b/include/linux/io-pgtable.h
>> index ea727eb1a1a9..1a00ea8562c7 100644
>> --- a/include/linux/io-pgtable.h
>> +++ b/include/linux/io-pgtable.h
>> @@ -97,6 +97,7 @@ struct io_pgtable_cfg {
>> unsigned longpgsize_bitmap;
>> unsigned intias;
>> unsigned intoas;
>> +boolhttu_hd;
>
> This is very specific to the AArch64 stage 1 format, not a generic capability
> - I think it should be a quirk flag rather than a common field.
>
> Robin.
>
> [1]
> https://jpbrucker.net/git/linux/commit/?h=sva/current=1ef7d512fb9082450dfe0d22ca4f7e35625a097b
>
>> boolcoherent_walk;
>> const struct iommu_flush_ops*tlb;
>> struct device*iommu_dev;
>>
> .
>
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [RFC PATCH 01/11] iommu/arm-smmu-v3: Add feature detection for HTTU
Hi Robin,
On 2021/2/5 19: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
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
Signed-off-by: Kunkun Jiang
---
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= _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.
Agree.
>
> 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.
Fair enough. Mask can realize "downgrade", but not "upgrade". You give a
reasonable point for upgrade.
BTW, my original intention is that mask can provide some convenience for BIOS
maker, as the override flag can keep same for SMMUs regardless they support
HTTU or not. But it shows that mask cannot cover all scenario.
>
>> 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.
Yep, I just get the latest version and see it.
>
+case IDR0_HTTU_NONE:
+break;
+case IDR0_HTTU_HA:
+smmu->features |= ARM_SMMU_FEAT_HTTU_HA;
+break;
+case IDR0_HTTU_HAD:
+smmu->features |= ARM_SMMU_FEAT_HTTU_HA;
+smmu->features |= ARM_SMMU_FEAT_HTTU_HD;
+break;
+default:
+dev_err(smmu->dev, "unknown/unsupported HTTU!\n");
+return -ENXIO;
+}
+
/*
* The coherency
Re: [RFC PATCH 01/11] iommu/arm-smmu-v3: Add feature detection for HTTU
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
>
> 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
> Signed-off-by: Kunkun Jiang
> ---
>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= _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
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [RFC PATCH 01/11] iommu/arm-smmu-v3: Add feature detection for HTTU
Hi Jean, On 2021/2/5 17:51, Jean-Philippe Brucker wrote: > Hi Keqian, > > On Fri, Feb 05, 2021 at 05:13:50PM +0800, Keqian Zhu wrote: >>> 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). >> >> 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? > > To me "Overrides the value of SMMU_IDR0.HTTU" is clear enough: disregard > the value of SMMU_IDR0.HTTU and use the one specified by IORT instead. And > that's both ways, since there is no validity mask for the IORT value: if > there is an IORT table, always ignore SMMU_IDR0.HTTU. > > That's how the SMMU driver implements the COHACC bit, which has the same > wording in IORT. So I think we should implement HTTU the same way. OK, and Robin said that the latest IORT spec literally states it. > > One complication is that there is no equivalent override for device tree. > I think it can be added later if necessary, because unlike IORT it can be > tri state (property not present, overriden positive, overridden negative). Yeah, that would be more flexible. ;-) > > Thanks, > Jean > > . > Thanks, Keqian ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [RFC PATCH 01/11] iommu/arm-smmu-v3: Add feature detection for HTTU
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
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
Signed-off-by: Kunkun Jiang
---
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 = _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.
Robin.
+ case IDR0_HTTU_NONE:
+ break;
+ case IDR0_HTTU_HA:
+ smmu->features |= ARM_SMMU_FEAT_HTTU_HA;
+ break;
+ case IDR0_HTTU_HAD:
+ smmu->features |= ARM_SMMU_FEAT_HTTU_HA;
+ smmu->features |= ARM_SMMU_FEAT_HTTU_HD;
+ break;
+ default:
+ dev_err(smmu->dev, "unknown/unsupported HTTU!\n");
+ return -ENXIO;
+ }
+
/*
* The coherency feature as set by FW is used in preference
to the ID
* register, but warn on mismatch.
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 96c2e9565e00..e91bea44519e 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
@@ -33,6 +33,10 @@
#define IDR0_ASID16 (1 << 12)
#define IDR0_ATS (1 <<
Re: [RFC PATCH 01/11] iommu/arm-smmu-v3: Add feature detection for HTTU
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
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
Signed-off-by: Kunkun Jiang
---
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= _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.
+case IDR0_HTTU_NONE:
+break;
+case IDR0_HTTU_HA:
+smmu->features |= ARM_SMMU_FEAT_HTTU_HA;
+break;
+case IDR0_HTTU_HAD:
+smmu->features |= ARM_SMMU_FEAT_HTTU_HA;
+smmu->features |= ARM_SMMU_FEAT_HTTU_HD;
+break;
+default:
+dev_err(smmu->dev, "unknown/unsupported HTTU!\n");
+return -ENXIO;
+}
+
/*
* The coherency feature as set by FW is used in preference to the ID
* register, but warn on mismatch.
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 96c2e9565e00..e91bea44519e 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
@@ -33,6 +33,10 @@
#define IDR0_ASID16(1 << 12)
#define IDR0_ATS(1 << 10)
#define IDR0_HYP(1 << 9)
+#define IDR0_HTTUGENMASK(7, 6)
+#define IDR0_HTTU_NONE0
+#define IDR0_HTTU_HA1
+#define IDR0_HTTU_HAD2
#define IDR0_COHACC(1 << 4)
#define IDR0_TTFGENMASK(3, 2)
#define IDR0_TTF_AARCH642
@@ -286,6 +290,8 @@
#define
Re: [RFC PATCH 01/11] iommu/arm-smmu-v3: Add feature detection for HTTU
Hi Keqian, On Fri, Feb 05, 2021 at 05:13:50PM +0800, Keqian Zhu wrote: > > 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). > > 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? To me "Overrides the value of SMMU_IDR0.HTTU" is clear enough: disregard the value of SMMU_IDR0.HTTU and use the one specified by IORT instead. And that's both ways, since there is no validity mask for the IORT value: if there is an IORT table, always ignore SMMU_IDR0.HTTU. That's how the SMMU driver implements the COHACC bit, which has the same wording in IORT. So I think we should implement HTTU the same way. One complication is that there is no equivalent override for device tree. I think it can be added later if necessary, because unlike IORT it can be tri state (property not present, overriden positive, overridden negative). Thanks, Jean ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [RFC PATCH 01/11] iommu/arm-smmu-v3: Add feature detection for HTTU
Hi Robin and Jean,
On 2021/2/5 3:50, Robin Murphy wrote:
> On 2021-01-28 15:17, Keqian Zhu wrote:
>> From: jiangkunkun
>>
>> 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
>> Signed-off-by: Kunkun Jiang
>> ---
>> 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= _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).
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;
>
>> +case IDR0_HTTU_NONE:
>> +break;
>> +case IDR0_HTTU_HA:
>> +smmu->features |= ARM_SMMU_FEAT_HTTU_HA;
>> +break;
>> +case IDR0_HTTU_HAD:
>> +smmu->features |= ARM_SMMU_FEAT_HTTU_HA;
>> +smmu->features |= ARM_SMMU_FEAT_HTTU_HD;
>> +break;
>> +default:
>> +dev_err(smmu->dev, "unknown/unsupported HTTU!\n");
>> +return -ENXIO;
>> +}
>> +
>> /*
>>* The coherency feature as set by FW is used in preference to the ID
>>* register, but warn on mismatch.
>> 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 96c2e9565e00..e91bea44519e 100644
>> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
>> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
>> @@ -33,6 +33,10 @@
>> #define IDR0_ASID16(1 << 12)
>> #define IDR0_ATS(1 << 10)
>> #define IDR0_HYP(1 << 9)
>> +#define IDR0_HTTUGENMASK(7, 6)
>> +#define IDR0_HTTU_NONE0
>> +#define IDR0_HTTU_HA1
>> +#define IDR0_HTTU_HAD2
>> #define IDR0_COHACC(1 << 4)
>> #define IDR0_TTFGENMASK(3, 2)
>> #define IDR0_TTF_AARCH642
>> @@ -286,6 +290,8 @@
>> #define CTXDESC_CD_0_TCR_TBI0(1ULL << 38)
>> #define CTXDESC_CD_0_AA64(1UL << 41)
>> +#define CTXDESC_CD_0_HD(1UL << 42)
>> +#define CTXDESC_CD_0_HA(1UL << 43)
>> #define CTXDESC_CD_0_S(1UL << 44)
>> #define CTXDESC_CD_0_R(1UL << 45)
>> #define CTXDESC_CD_0_A(1UL << 46)
>> @@ -604,6 +610,8 @@ struct arm_smmu_device {
>> #define ARM_SMMU_FEAT_RANGE_INV(1 << 15)
>> #define ARM_SMMU_FEAT_BTM(1 << 16)
>> #define ARM_SMMU_FEAT_SVA(1 << 17)
>> +#define ARM_SMMU_FEAT_HTTU_HA(1 << 18)
>> +#define ARM_SMMU_FEAT_HTTU_HD(1 << 19)
>> u32features;
>> #define ARM_SMMU_OPT_SKIP_PREFETCH(1 << 0)
>> diff --git a/include/linux/io-pgtable.h b/include/linux/io-pgtable.h
>> index ea727eb1a1a9..1a00ea8562c7 100644
>> --- a/include/linux/io-pgtable.h
>> +++ b/include/linux/io-pgtable.h
>> @@ -97,6 +97,7 @@ struct io_pgtable_cfg {
>> unsigned longpgsize_bitmap;
>> unsigned intias;
>> unsigned intoas;
>> +boolhttu_hd;
>
> This is very specific to the AArch64 stage 1 format, not a generic capability
Re: [RFC PATCH 01/11] iommu/arm-smmu-v3: Add feature detection for HTTU
On 2021-01-28 15:17, Keqian Zhu wrote:
From: jiangkunkun
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
Signed-off-by: Kunkun Jiang
---
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= _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].
+ case IDR0_HTTU_NONE:
+ break;
+ case IDR0_HTTU_HA:
+ smmu->features |= ARM_SMMU_FEAT_HTTU_HA;
+ break;
+ case IDR0_HTTU_HAD:
+ smmu->features |= ARM_SMMU_FEAT_HTTU_HA;
+ smmu->features |= ARM_SMMU_FEAT_HTTU_HD;
+ break;
+ default:
+ dev_err(smmu->dev, "unknown/unsupported HTTU!\n");
+ return -ENXIO;
+ }
+
/*
* The coherency feature as set by FW is used in preference to the ID
* register, but warn on mismatch.
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 96c2e9565e00..e91bea44519e 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
@@ -33,6 +33,10 @@
#define IDR0_ASID16 (1 << 12)
#define IDR0_ATS (1 << 10)
#define IDR0_HYP (1 << 9)
+#define IDR0_HTTU GENMASK(7, 6)
+#define IDR0_HTTU_NONE 0
+#define IDR0_HTTU_HA 1
+#define IDR0_HTTU_HAD 2
#define IDR0_COHACC (1 << 4)
#define IDR0_TTF GENMASK(3, 2)
#define IDR0_TTF_AARCH64 2
@@ -286,6 +290,8 @@
#define CTXDESC_CD_0_TCR_TBI0 (1ULL << 38)
#define CTXDESC_CD_0_AA64 (1UL << 41)
+#define CTXDESC_CD_0_HD(1UL << 42)
+#define CTXDESC_CD_0_HA(1UL << 43)
#define CTXDESC_CD_0_S(1UL << 44)
#define CTXDESC_CD_0_R(1UL << 45)
#define CTXDESC_CD_0_A(1UL << 46)
@@ -604,6 +610,8 @@ struct arm_smmu_device {
#define ARM_SMMU_FEAT_RANGE_INV (1 << 15)
#define ARM_SMMU_FEAT_BTM (1 << 16)
#define ARM_SMMU_FEAT_SVA (1 << 17)
+#define ARM_SMMU_FEAT_HTTU_HA (1 << 18)
+#define ARM_SMMU_FEAT_HTTU_HD (1 << 19)
u32 features;
#define ARM_SMMU_OPT_SKIP_PREFETCH (1 << 0)
diff --git a/include/linux/io-pgtable.h b/include/linux/io-pgtable.h
index ea727eb1a1a9..1a00ea8562c7 100644
--- a/include/linux/io-pgtable.h
+++ b/include/linux/io-pgtable.h
@@ -97,6 +97,7 @@ struct io_pgtable_cfg {
unsigned long pgsize_bitmap;
unsigned intias;
unsigned intoas;
+ boolhttu_hd;
This is very specific to the AArch64 stage 1 format, not a generic
capability - I think it should be a quirk flag rather than a common field.
Robin.
[1]
https://jpbrucker.net/git/linux/commit/?h=sva/current=1ef7d512fb9082450dfe0d22ca4f7e35625a097b
boolcoherent_walk;
const struct iommu_flush_ops*tlb;
struct device *iommu_dev;
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
