From: Rui Zhu <[email protected]>
This reverts commit 6833b8f2e19945a41e4d5efd8c6d9f4cae9a5b7d.
This constraint violates the protocol. When tg is not 0 but ttl, scale,
and num are 0, the hardware reports the CERROR_IL gerror. In the
protocol, leaf is not a prerequisite for TTL.
Cc: Will Deacon <[email protected]>
Cc: Robin Murphy <[email protected]>
Cc: Joerg Roedel <[email protected]>
Cc: Lu Baolu <[email protected]>
Cc: Jason Gunthorpe <[email protected]>
Cc: Yicong Yang <[email protected]>
Cc: Tomas Krcka <[email protected]>
Cc: Jean-Philippe Brucker <[email protected]>
Cc: Nicolin Chen <[email protected]>
Cc: Rui Zhu <[email protected]>
Signed-off-by: Rui Zhu <[email protected]>
---
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 9 ++-------
1 file changed, 2 insertions(+), 7 deletions(-)
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 9b0dc3505601..098e84cfa82f 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -1898,13 +1898,8 @@ static void __arm_smmu_tlb_inv_range(struct arm_smmu_cmdq_ent *cmd,
/* Convert page size of 12,14,16 (log2) to 1,2,3 */
cmd->tlbi.tg = (tg - 10) / 2;
- /*
- * Determine what level the granule is at. For non-leaf, io-pgtable
- * assumes .tlb_flush_walk can invalidate multiple levels at once,
- * so ignore the nominal last-level granule and leave TTL=0.
- */
- if (cmd->tlbi.leaf)
- cmd->tlbi.ttl = 4 - ((ilog2(granule) - 3) / (tg - 3));
+ /* Determine what level the granule is at */
+ cmd->tlbi.ttl = 4 - ((ilog2(granule) - 3) / (tg - 3));
num_pages = size >> tg;
}
--
1.8.3.1
On Mon, Jul 31, 2023 at 02:21:22PM +0800, wangwudi wrote:
> From: Rui Zhu <[email protected]>
>
> This reverts commit 6833b8f2e19945a41e4d5efd8c6d9f4cae9a5b7d.
>
> This constraint violates the protocol. When tg is not 0 but ttl, scale,
> and num are 0, the hardware reports the CERROR_IL gerror. In the
> protocol, leaf is not a prerequisite for TTL.
>
> Cc: Will Deacon <[email protected]>
> Cc: Robin Murphy <[email protected]>
> Cc: Joerg Roedel <[email protected]>
> Cc: Lu Baolu <[email protected]>
> Cc: Jason Gunthorpe <[email protected]>
> Cc: Yicong Yang <[email protected]>
> Cc: Tomas Krcka <[email protected]>
> Cc: Jean-Philippe Brucker <[email protected]>
> Cc: Nicolin Chen <[email protected]>
> Cc: Rui Zhu <[email protected]>
>
> Signed-off-by: Rui Zhu <[email protected]>
> ---
> drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 9 ++-------
> 1 file changed, 2 insertions(+), 7 deletions(-)
>
> 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 9b0dc3505601..098e84cfa82f 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -1898,13 +1898,8 @@ static void __arm_smmu_tlb_inv_range(struct arm_smmu_cmdq_ent *cmd,
> /* Convert page size of 12,14,16 (log2) to 1,2,3 */
> cmd->tlbi.tg = (tg - 10) / 2;
>
> - /*
> - * Determine what level the granule is at. For non-leaf, io-pgtable
> - * assumes .tlb_flush_walk can invalidate multiple levels at once,
> - * so ignore the nominal last-level granule and leave TTL=0.
> - */
> - if (cmd->tlbi.leaf)
> - cmd->tlbi.ttl = 4 - ((ilog2(granule) - 3) / (tg - 3));
> + /* Determine what level the granule is at */
> + cmd->tlbi.ttl = 4 - ((ilog2(granule) - 3) / (tg - 3));
Doesn't this reintroduce the bug that 6833b8f2e199 tried to fix?
afaict, we should only hit the problematic case of tg != 0 but ttl, scale
and num all 0 if we're invalidating a single page, so shouldn't we just
zap tg in that case, since it's not doing anything useful?
I hesitate to say we should avoid range invalidation altogether for
single-page invalidations because I think some errata workarounds might
need that to work.
Will
On 2023/8/1 16:55, Will Deacon wrote:
> On Mon, Jul 31, 2023 at 02:21:22PM +0800, wangwudi wrote:
>> From: Rui Zhu <[email protected]>
>>
>> This reverts commit 6833b8f2e19945a41e4d5efd8c6d9f4cae9a5b7d.
>>
>> This constraint violates the protocol. When tg is not 0 but ttl, scale,
>> and num are 0, the hardware reports the CERROR_IL gerror. In the
>> protocol, leaf is not a prerequisite for TTL.
>>
>> Cc: Will Deacon <[email protected]>
>> Cc: Robin Murphy <[email protected]>
>> Cc: Joerg Roedel <[email protected]>
>> Cc: Lu Baolu <[email protected]>
>> Cc: Jason Gunthorpe <[email protected]>
>> Cc: Yicong Yang <[email protected]>
>> Cc: Tomas Krcka <[email protected]>
>> Cc: Jean-Philippe Brucker <[email protected]>
>> Cc: Nicolin Chen <[email protected]>
>> Cc: Rui Zhu <[email protected]>
>>
>> Signed-off-by: Rui Zhu <[email protected]>
>> ---
>> drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 9 ++-------
>> 1 file changed, 2 insertions(+), 7 deletions(-)
>>
>> 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 9b0dc3505601..098e84cfa82f 100644
>> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>> @@ -1898,13 +1898,8 @@ static void __arm_smmu_tlb_inv_range(struct arm_smmu_cmdq_ent *cmd,
>> /* Convert page size of 12,14,16 (log2) to 1,2,3 */
>> cmd->tlbi.tg = (tg - 10) / 2;
>>
>> - /*
>> - * Determine what level the granule is at. For non-leaf, io-pgtable
>> - * assumes .tlb_flush_walk can invalidate multiple levels at once,
>> - * so ignore the nominal last-level granule and leave TTL=0.
>> - */
>> - if (cmd->tlbi.leaf)
>> - cmd->tlbi.ttl = 4 - ((ilog2(granule) - 3) / (tg - 3));
>> + /* Determine what level the granule is at */
>> + cmd->tlbi.ttl = 4 - ((ilog2(granule) - 3) / (tg - 3));
>
> Doesn't this reintroduce the bug that 6833b8f2e199 tried to fix?
>
> afaict, we should only hit the problematic case of tg != 0 but ttl, scale
> and num all 0 if we're invalidating a single page, so shouldn't we just
> zap tg in that case, since it's not doing anything useful?
You're right. I'm sorry I missed. I just need to handle the problematic
case by assigning 0 to tg. It's better to add this following code before
each tlbi cmd batch add.
if (num_pages == 1) {
cmd->tlbi.tg = 0;
}
I'll resubmit a new patch. Thanks for your correction.
>
> I hesitate to say we should avoid range invalidation altogether for
> single-page invalidations because I think some errata workarounds might
> need that to work.
>
> Will
> .
>
When tg != 0 but ttl, scale, num all 0 in a range tlbi command, it
is reserved and will cause the CERROR_ILL error. This case means
that the size to be invalidated is only one page size, and the
range invalidation is meaningless here. So we set tg to 0 in this
case to do an non-range invalidation instead.
Cc: Will Deacon <[email protected]>
Cc: Robin Murphy <[email protected]>
Cc: Joerg Roedel <[email protected]>
Cc: Lu Baolu <[email protected]>
Cc: Jason Gunthorpe <[email protected]>
Cc: Yicong Yang <[email protected]>
Cc: Tomas Krcka <[email protected]>
Cc: Jean-Philippe Brucker <[email protected]>
Cc: Nicolin Chen <[email protected]>
Cc: Rui Zhu <[email protected]>
Signed-off-by: Rui Zhu <[email protected]>
---
ChangeLog:
v1-->v2:
1. Change from "Revert" to modify the problematic case
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
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 9b0dc3505601..5e56c7e85819 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -1895,9 +1895,6 @@ static void __arm_smmu_tlb_inv_range(struct arm_smmu_cmdq_ent *cmd,
/* Get the leaf page size */
tg = __ffs(smmu_domain->domain.pgsize_bitmap);
- /* Convert page size of 12,14,16 (log2) to 1,2,3 */
- cmd->tlbi.tg = (tg - 10) / 2;
-
/*
* Determine what level the granule is at. For non-leaf, io-pgtable
* assumes .tlb_flush_walk can invalidate multiple levels at once,
@@ -1930,6 +1927,12 @@ static void __arm_smmu_tlb_inv_range(struct arm_smmu_cmdq_ent *cmd,
num = (num_pages >> scale) & CMDQ_TLBI_RANGE_NUM_MAX;
cmd->tlbi.num = num - 1;
+ /* Prevent error caused by one page tlbi with leaf 0 */
+ if (scale == 0 && num == 1 && cmd->tlbi.leaf == 0)
+ cmd->tlbi.tg = 0;
+ else /* Convert page size of 12,14,16 (log2) to 1,2,3 */
+ cmd->tlbi.tg = (tg - 10) / 2;
+
/* range is num * 2^scale * pgsize */
inv_range = num << (scale + tg);
--
1.8.3.1
On Fri, Aug 04, 2023 at 05:31:20PM +0800, zhurui wrote:
> When tg != 0 but ttl, scale, num all 0 in a range tlbi command, it
> is reserved and will cause the CERROR_ILL error. This case means
> that the size to be invalidated is only one page size, and the
> range invalidation is meaningless here. So we set tg to 0 in this
> case to do an non-range invalidation instead.
>
> Cc: Will Deacon <[email protected]>
> Cc: Robin Murphy <[email protected]>
> Cc: Joerg Roedel <[email protected]>
> Cc: Lu Baolu <[email protected]>
> Cc: Jason Gunthorpe <[email protected]>
> Cc: Yicong Yang <[email protected]>
> Cc: Tomas Krcka <[email protected]>
> Cc: Jean-Philippe Brucker <[email protected]>
> Cc: Nicolin Chen <[email protected]>
> Cc: Rui Zhu <[email protected]>
>
> Signed-off-by: Rui Zhu <[email protected]>
> ---
> ChangeLog:
> v1-->v2:
> 1. Change from "Revert" to modify the problematic case
>
> drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> 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 9b0dc3505601..5e56c7e85819 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -1895,9 +1895,6 @@ static void __arm_smmu_tlb_inv_range(struct arm_smmu_cmdq_ent *cmd,
> /* Get the leaf page size */
> tg = __ffs(smmu_domain->domain.pgsize_bitmap);
>
> - /* Convert page size of 12,14,16 (log2) to 1,2,3 */
> - cmd->tlbi.tg = (tg - 10) / 2;
> -
> /*
> * Determine what level the granule is at. For non-leaf, io-pgtable
> * assumes .tlb_flush_walk can invalidate multiple levels at once,
> @@ -1930,6 +1927,12 @@ static void __arm_smmu_tlb_inv_range(struct arm_smmu_cmdq_ent *cmd,
> num = (num_pages >> scale) & CMDQ_TLBI_RANGE_NUM_MAX;
> cmd->tlbi.num = num - 1;
>
> + /* Prevent error caused by one page tlbi with leaf 0 */
> + if (scale == 0 && num == 1 && cmd->tlbi.leaf == 0)
> + cmd->tlbi.tg = 0;
This should only be true for the last iteration, right (i.e. when num_pages
== 1)? In which case, I'd prefer to leave the old code as-is and just add:
/* Single-page leaf invalidation requires a TG field of 0 */
if (num_pages == 1 && !cmd->tlbi.leaf)
cmd->tlbi.tg = 0;
here.
Will
On Fri, Aug 04, 2023 at 05:52:25PM +0100, Will Deacon wrote:
> On Fri, Aug 04, 2023 at 05:31:20PM +0800, zhurui wrote:
> > When tg != 0 but ttl, scale, num all 0 in a range tlbi command, it
> > is reserved and will cause the CERROR_ILL error. This case means
> > that the size to be invalidated is only one page size, and the
> > range invalidation is meaningless here. So we set tg to 0 in this
> > case to do an non-range invalidation instead.
> > @@ -1930,6 +1927,12 @@ static void __arm_smmu_tlb_inv_range(struct arm_smmu_cmdq_ent *cmd,
> > num = (num_pages >> scale) & CMDQ_TLBI_RANGE_NUM_MAX;
> > cmd->tlbi.num = num - 1;
> >
> > + /* Prevent error caused by one page tlbi with leaf 0 */
> > + if (scale == 0 && num == 1 && cmd->tlbi.leaf == 0)
> > + cmd->tlbi.tg = 0;
>
> This should only be true for the last iteration, right (i.e. when num_pages
> == 1)? In which case, I'd prefer to leave the old code as-is and just add:
>
> /* Single-page leaf invalidation requires a TG field of 0 */
> if (num_pages == 1 && !cmd->tlbi.leaf)
> cmd->tlbi.tg = 0;
Is "!cmd->tlbi.leaf" to be "leaf" or "non-leaf"?
IIUIC, this "num_pages == 1" implies "NUM == 0, SCALE == 0" while
the "!cmd->tlbi.leaf" implies "TTL = 0b00", which in combination
would result in a CERROR_ILL mentioned by the spec?
I feel this could be more clear by just checking the three fields
following the spec...
Thanks
Nicolin
On 2023/8/5 2:30, Nicolin Chen wrote:
> On Fri, Aug 04, 2023 at 05:52:25PM +0100, Will Deacon wrote:
>> On Fri, Aug 04, 2023 at 05:31:20PM +0800, zhurui wrote:
>>> When tg != 0 but ttl, scale, num all 0 in a range tlbi command, it
>>> is reserved and will cause the CERROR_ILL error. This case means
>>> that the size to be invalidated is only one page size, and the
>>> range invalidation is meaningless here. So we set tg to 0 in this
>>> case to do an non-range invalidation instead.
>
>>> @@ -1930,6 +1927,12 @@ static void __arm_smmu_tlb_inv_range(struct arm_smmu_cmdq_ent *cmd,
>>> num = (num_pages >> scale) & CMDQ_TLBI_RANGE_NUM_MAX;
>>> cmd->tlbi.num = num - 1;
>>>
>>> + /* Prevent error caused by one page tlbi with leaf 0 */
>>> + if (scale == 0 && num == 1 && cmd->tlbi.leaf == 0)
>>> + cmd->tlbi.tg = 0;
>>
>> This should only be true for the last iteration, right (i.e. when num_pages
>> == 1)? In which case, I'd prefer to leave the old code as-is and just add:
>>
>> /* Single-page leaf invalidation requires a TG field of 0 */
>> if (num_pages == 1 && !cmd->tlbi.leaf)
>> cmd->tlbi.tg = 0;To Will and Nicolin,
Not only the last iteration, it's the result of __ffs function. For example, if
numpages is 33, then the value of __ffs(num_pages) is 0, so the value of scale
is also 0. The value of num depends on CMDQ_TLBI_RANGE_NUM_MAX. That is, the
maximum value of num is 31. Therefore, the final value of num is 1.
So, if consider CMDQ_TLBI_RANGE_NUM_MAX, there will be some case not the last
one page but the beginning pages. That's why I use scale and num as conditions,
not num_pages. Then I should reassign tg based on the result.
>
> Is "!cmd->tlbi.leaf" to be "leaf" or "non-leaf"?
>
> IIUIC, this "num_pages == 1" implies "NUM == 0, SCALE == 0" while
> the "!cmd->tlbi.leaf" implies "TTL = 0b00", which in combination
> would result in a CERROR_ILL mentioned by the spec?
>
> I feel this could be more clear by just checking the three fields
> following the spec...>
> Thanks
> Nicolin
> .
>
Yes, based on spec 4.4.1.1 for ARM IHI 0070, after the TLL and TG table, there is a
description for TG != 0b00, and you can check it in the last point.
Thanks.
ZhuRui
.
On 2023-08-06 06:28, zhurui wrote:
> On 2023/8/5 2:30, Nicolin Chen wrote:
>> On Fri, Aug 04, 2023 at 05:52:25PM +0100, Will Deacon wrote:
>>> On Fri, Aug 04, 2023 at 05:31:20PM +0800, zhurui wrote:
>>>> When tg != 0 but ttl, scale, num all 0 in a range tlbi command, it
>>>> is reserved and will cause the CERROR_ILL error. This case means
>>>> that the size to be invalidated is only one page size, and the
>>>> range invalidation is meaningless here. So we set tg to 0 in this
>>>> case to do an non-range invalidation instead.
>>
>>>> @@ -1930,6 +1927,12 @@ static void __arm_smmu_tlb_inv_range(struct arm_smmu_cmdq_ent *cmd,
>>>> num = (num_pages >> scale) & CMDQ_TLBI_RANGE_NUM_MAX;
>>>> cmd->tlbi.num = num - 1;
>>>>
>>>> + /* Prevent error caused by one page tlbi with leaf 0 */
>>>> + if (scale == 0 && num == 1 && cmd->tlbi.leaf == 0)
>>>> + cmd->tlbi.tg = 0;
>>>
>>> This should only be true for the last iteration, right (i.e. when num_pages
>>> == 1)? In which case, I'd prefer to leave the old code as-is and just add:
>>>
>>> /* Single-page leaf invalidation requires a TG field of 0 */
>>> if (num_pages == 1 && !cmd->tlbi.leaf)
>>> cmd->tlbi.tg = 0;To Will and Nicolin,
>
> Not only the last iteration, it's the result of __ffs function. For example, if
> numpages is 33, then the value of __ffs(num_pages) is 0, so the value of scale
> is also 0. The value of num depends on CMDQ_TLBI_RANGE_NUM_MAX. That is, the
> maximum value of num is 31. Therefore, the final value of num is 1.
> So, if consider CMDQ_TLBI_RANGE_NUM_MAX, there will be some case not the last
> one page but the beginning pages. That's why I use scale and num as conditions,
> not num_pages. Then I should reassign tg based on the result.
Yeah, I'd rather not downgrade to a non-range invalidate since that
complicates the reasoning for the errata affecting those. If the size of
the invalidation is equal to TG then it can only represent a single
last-level page, i.e. TTL=3, thus if it does warrant handling here then
indeed rearranging to base the condition on num_pages as well ought to
suffice. However, this is all still begging the question of where and
why we're doing a *non-leaf* invalidation that isn't aligned to the size
of a table, because that in itself doesn't make a whole heap of sense -
my hunch is that that wants figuring out and could probably be fixed at
the source.
Thanks,
Robin.
>
>>
>> Is "!cmd->tlbi.leaf" to be "leaf" or "non-leaf"?
>>
>> IIUIC, this "num_pages == 1" implies "NUM == 0, SCALE == 0" while
>> the "!cmd->tlbi.leaf" implies "TTL = 0b00", which in combination
>> would result in a CERROR_ILL mentioned by the spec?
>>
>> I feel this could be more clear by just checking the three fields
>> following the spec...>
>> Thanks
>> Nicolin
>> .
>>
> Yes, based on spec 4.4.1.1 for ARM IHI 0070, after the TLL and TG table, there is a
> description for TG != 0b00, and you can check it in the last point.
>
> Thanks.
> ZhuRui
> .
On Sun, Aug 06, 2023 at 01:28:04PM +0800, zhurui wrote:
> On 2023/8/5 2:30, Nicolin Chen wrote:
> > On Fri, Aug 04, 2023 at 05:52:25PM +0100, Will Deacon wrote:
> >> On Fri, Aug 04, 2023 at 05:31:20PM +0800, zhurui wrote:
> >>> When tg != 0 but ttl, scale, num all 0 in a range tlbi command, it
> >>> is reserved and will cause the CERROR_ILL error. This case means
> >>> that the size to be invalidated is only one page size, and the
> >>> range invalidation is meaningless here. So we set tg to 0 in this
> >>> case to do an non-range invalidation instead.
> >
> >>> @@ -1930,6 +1927,12 @@ static void __arm_smmu_tlb_inv_range(struct arm_smmu_cmdq_ent *cmd,
> >>> num = (num_pages >> scale) & CMDQ_TLBI_RANGE_NUM_MAX;
> >>> cmd->tlbi.num = num - 1;
> >>>
> >>> + /* Prevent error caused by one page tlbi with leaf 0 */
> >>> + if (scale == 0 && num == 1 && cmd->tlbi.leaf == 0)
> >>> + cmd->tlbi.tg = 0;
> >>
> >> This should only be true for the last iteration, right (i.e. when num_pages
> >> == 1)? In which case, I'd prefer to leave the old code as-is and just add:
> >>
> >> /* Single-page leaf invalidation requires a TG field of 0 */
> >> if (num_pages == 1 && !cmd->tlbi.leaf)
> >> cmd->tlbi.tg = 0;To Will and Nicolin,
>
> Not only the last iteration, it's the result of __ffs function. For example, if
> numpages is 33, then the value of __ffs(num_pages) is 0, so the value of scale
> is also 0. The value of num depends on CMDQ_TLBI_RANGE_NUM_MAX. That is, the
> maximum value of num is 31. Therefore, the final value of num is 1.
> So, if consider CMDQ_TLBI_RANGE_NUM_MAX, there will be some case not the last
> one page but the beginning pages. That's why I use scale and num as conditions,
> not num_pages. Then I should reassign tg based on the result.
Yes, thanks, you're quite right. I'm not sure what I was thinking!
Will
Hi Robin,
On Mon, Aug 07, 2023 at 08:20:45PM +0100, Robin Murphy wrote:
> On 2023-08-06 06:28, zhurui wrote:
> > On 2023/8/5 2:30, Nicolin Chen wrote:
> > > On Fri, Aug 04, 2023 at 05:52:25PM +0100, Will Deacon wrote:
> > > > On Fri, Aug 04, 2023 at 05:31:20PM +0800, zhurui wrote:
> > > > > When tg != 0 but ttl, scale, num all 0 in a range tlbi command, it
> > > > > is reserved and will cause the CERROR_ILL error. This case means
> > > > > that the size to be invalidated is only one page size, and the
> > > > > range invalidation is meaningless here. So we set tg to 0 in this
> > > > > case to do an non-range invalidation instead.
> > >
> > > > > @@ -1930,6 +1927,12 @@ static void __arm_smmu_tlb_inv_range(struct arm_smmu_cmdq_ent *cmd,
> > > > > num = (num_pages >> scale) & CMDQ_TLBI_RANGE_NUM_MAX;
> > > > > cmd->tlbi.num = num - 1;
> > > > >
> > > > > + /* Prevent error caused by one page tlbi with leaf 0 */
> > > > > + if (scale == 0 && num == 1 && cmd->tlbi.leaf == 0)
> > > > > + cmd->tlbi.tg = 0;
> > > >
> > > > This should only be true for the last iteration, right (i.e. when num_pages
> > > > == 1)? In which case, I'd prefer to leave the old code as-is and just add:
> > > >
> > > > /* Single-page leaf invalidation requires a TG field of 0 */
> > > > if (num_pages == 1 && !cmd->tlbi.leaf)
> > > > cmd->tlbi.tg = 0;To Will and Nicolin,
> >
> > Not only the last iteration, it's the result of __ffs function. For example, if
> > numpages is 33, then the value of __ffs(num_pages) is 0, so the value of scale
> > is also 0. The value of num depends on CMDQ_TLBI_RANGE_NUM_MAX. That is, the
> > maximum value of num is 31. Therefore, the final value of num is 1.
> > So, if consider CMDQ_TLBI_RANGE_NUM_MAX, there will be some case not the last
> > one page but the beginning pages. That's why I use scale and num as conditions,
> > not num_pages. Then I should reassign tg based on the result.
>
> Yeah, I'd rather not downgrade to a non-range invalidate since that
> complicates the reasoning for the errata affecting those. If the size of the
> invalidation is equal to TG then it can only represent a single last-level
> page, i.e. TTL=3, thus if it does warrant handling here then indeed
> rearranging to base the condition on num_pages as well ought to suffice.
> However, this is all still begging the question of where and why we're doing
> a *non-leaf* invalidation that isn't aligned to the size of a table, because
> that in itself doesn't make a whole heap of sense - my hunch is that that
> wants figuring out and could probably be fixed at the source.
Isn't that described above because we're using CMDQ_TLBI_RANGE_NUM_MAX
to break up the range into separate commands?
Do you mind if I queue the patch as-is for now? I don't think the driver
should be emitting illegal commands, and v2 of the patch does seem like
the obvious thing to do.
Will
On 08/08/2023 5:24 pm, Will Deacon wrote:
> Hi Robin,
>
> On Mon, Aug 07, 2023 at 08:20:45PM +0100, Robin Murphy wrote:
>> On 2023-08-06 06:28, zhurui wrote:
>>> On 2023/8/5 2:30, Nicolin Chen wrote:
>>>> On Fri, Aug 04, 2023 at 05:52:25PM +0100, Will Deacon wrote:
>>>>> On Fri, Aug 04, 2023 at 05:31:20PM +0800, zhurui wrote:
>>>>>> When tg != 0 but ttl, scale, num all 0 in a range tlbi command, it
>>>>>> is reserved and will cause the CERROR_ILL error. This case means
>>>>>> that the size to be invalidated is only one page size, and the
>>>>>> range invalidation is meaningless here. So we set tg to 0 in this
>>>>>> case to do an non-range invalidation instead.
>>>>
>>>>>> @@ -1930,6 +1927,12 @@ static void __arm_smmu_tlb_inv_range(struct arm_smmu_cmdq_ent *cmd,
>>>>>> num = (num_pages >> scale) & CMDQ_TLBI_RANGE_NUM_MAX;
>>>>>> cmd->tlbi.num = num - 1;
>>>>>>
>>>>>> + /* Prevent error caused by one page tlbi with leaf 0 */
>>>>>> + if (scale == 0 && num == 1 && cmd->tlbi.leaf == 0)
>>>>>> + cmd->tlbi.tg = 0;
>>>>>
>>>>> This should only be true for the last iteration, right (i.e. when num_pages
>>>>> == 1)? In which case, I'd prefer to leave the old code as-is and just add:
>>>>>
>>>>> /* Single-page leaf invalidation requires a TG field of 0 */
>>>>> if (num_pages == 1 && !cmd->tlbi.leaf)
>>>>> cmd->tlbi.tg = 0;To Will and Nicolin,
>>>
>>> Not only the last iteration, it's the result of __ffs function. For example, if
>>> numpages is 33, then the value of __ffs(num_pages) is 0, so the value of scale
>>> is also 0. The value of num depends on CMDQ_TLBI_RANGE_NUM_MAX. That is, the
>>> maximum value of num is 31. Therefore, the final value of num is 1.
>>> So, if consider CMDQ_TLBI_RANGE_NUM_MAX, there will be some case not the last
>>> one page but the beginning pages. That's why I use scale and num as conditions,
>>> not num_pages. Then I should reassign tg based on the result.
>>
>> Yeah, I'd rather not downgrade to a non-range invalidate since that
>> complicates the reasoning for the errata affecting those. If the size of the
>> invalidation is equal to TG then it can only represent a single last-level
>> page, i.e. TTL=3, thus if it does warrant handling here then indeed
>> rearranging to base the condition on num_pages as well ought to suffice.
>> However, this is all still begging the question of where and why we're doing
>> a *non-leaf* invalidation that isn't aligned to the size of a table, because
>> that in itself doesn't make a whole heap of sense - my hunch is that that
>> wants figuring out and could probably be fixed at the source.
>
> Isn't that described above because we're using CMDQ_TLBI_RANGE_NUM_MAX
> to break up the range into separate commands?
Not really, because if we're doing a genuine non-leaf invalidation of a
table then it should be a block-aligned range that ought to fit in a
single command and should certainly never involve a single-granule
remainder. If we're doing non-leaf invalidations of things that
logically don't need to be non-leaf, making them leaf would be the even
better option.
> Do you mind if I queue the patch as-is for now? I don't think the driver
> should be emitting illegal commands, and v2 of the patch does seem like
> the obvious thing to do.
TBH I'd rather you just drop my patch if it's proven problematic, and
I'll take another crack at it soon. The potential problems we introduce
by using non-range invalidates on errata-affected MMU-700 revisions are
worse than the almost-entirely-theoretical one I was trying to address.
Cheers,
Robin.
On 2023/8/9 0:43, Robin Murphy wrote:
> On 08/08/2023 5:24 pm, Will Deacon wrote:
>> Hi Robin,
>>
>> On Mon, Aug 07, 2023 at 08:20:45PM +0100, Robin Murphy wrote:
>>> On 2023-08-06 06:28, zhurui wrote:
>>>> On 2023/8/5 2:30, Nicolin Chen wrote:
>>>>> On Fri, Aug 04, 2023 at 05:52:25PM +0100, Will Deacon wrote:
>>>>>> On Fri, Aug 04, 2023 at 05:31:20PM +0800, zhurui wrote:
>>>>>>> When tg != 0 but ttl, scale, num all 0 in a range tlbi command, it
>>>>>>> is reserved and will cause the CERROR_ILL error. This case means
>>>>>>> that the size to be invalidated is only one page size, and the
>>>>>>> range invalidation is meaningless here. So we set tg to 0 in this
>>>>>>> case to do an non-range invalidation instead.
>>>>>
>>>>>>> @@ -1930,6 +1927,12 @@ static void __arm_smmu_tlb_inv_range(struct arm_smmu_cmdq_ent *cmd,
>>>>>>> num = (num_pages >> scale) & CMDQ_TLBI_RANGE_NUM_MAX;
>>>>>>> cmd->tlbi.num = num - 1;
>>>>>>>
>>>>>>> + /* Prevent error caused by one page tlbi with leaf 0 */
>>>>>>> + if (scale == 0 && num == 1 && cmd->tlbi.leaf == 0)
>>>>>>> + cmd->tlbi.tg = 0;
>>>>>>
>>>>>> This should only be true for the last iteration, right (i.e. when num_pages
>>>>>> == 1)? In which case, I'd prefer to leave the old code as-is and just add:
>>>>>>
>>>>>> /* Single-page leaf invalidation requires a TG field of 0 */
>>>>>> if (num_pages == 1 && !cmd->tlbi.leaf)
>>>>>> cmd->tlbi.tg = 0;To Will and Nicolin,
>>>>
>>>> Not only the last iteration, it's the result of __ffs function. For example, if
>>>> numpages is 33, then the value of __ffs(num_pages) is 0, so the value of scale
>>>> is also 0. The value of num depends on CMDQ_TLBI_RANGE_NUM_MAX. That is, the
>>>> maximum value of num is 31. Therefore, the final value of num is 1.
>>>> So, if consider CMDQ_TLBI_RANGE_NUM_MAX, there will be some case not the last
>>>> one page but the beginning pages. That's why I use scale and num as conditions,
>>>> not num_pages. Then I should reassign tg based on the result.
>>>
>>> Yeah, I'd rather not downgrade to a non-range invalidate since that
>>> complicates the reasoning for the errata affecting those. If the size of the
>>> invalidation is equal to TG then it can only represent a single last-level
>>> page, i.e. TTL=3, thus if it does warrant handling here then indeed
>>> rearranging to base the condition on num_pages as well ought to suffice.
>>> However, this is all still begging the question of where and why we're doing
>>> a *non-leaf* invalidation that isn't aligned to the size of a table, because
>>> that in itself doesn't make a whole heap of sense - my hunch is that that
>>> wants figuring out and could probably be fixed at the source.
>>
>> Isn't that described above because we're using CMDQ_TLBI_RANGE_NUM_MAX
>> to break up the range into separate commands?
>
> Not really, because if we're doing a genuine non-leaf invalidation of a table then it should be a block-aligned range that ought to fit in a single command and should certainly never involve a single-granule remainder. If we're doing non-leaf invalidations of things that logically don't need to be non-leaf, making them leaf would be the even better option.
>
I agree with Robin that if the caller is doing a genuine non-leaf invalidation
of a table, it should not involve a single-granule tlbi. It seems that the
caller only filter the block size, but not the address aligned or not maybe.
>> Do you mind if I queue the patch as-is for now? I don't think the driver
>> should be emitting illegal commands, and v2 of the patch does seem like
>> the obvious thing to do.
>
> TBH I'd rather you just drop my patch if it's proven problematic, and I'll take another crack at it soon. The potential problems we introduce by using non-range invalidates on errata-affected MMU-700 revisions are worse than the almost-entirely-theoretical one I was trying to address.
>
If you all agree to roll back the problematic code, is the first patch be OK?
Should I need to add some more descriptions to clarify this?
Thanks,
Zhurui.
On Wed, Aug 09, 2023 at 05:22:06PM +0800, zhurui wrote:
> On 2023/8/9 0:43, Robin Murphy wrote:
> > On 08/08/2023 5:24 pm, Will Deacon wrote:
> >> On Mon, Aug 07, 2023 at 08:20:45PM +0100, Robin Murphy wrote:
> >>> Yeah, I'd rather not downgrade to a non-range invalidate since that
> >>> complicates the reasoning for the errata affecting those. If the size of the
> >>> invalidation is equal to TG then it can only represent a single last-level
> >>> page, i.e. TTL=3, thus if it does warrant handling here then indeed
> >>> rearranging to base the condition on num_pages as well ought to suffice.
> >>> However, this is all still begging the question of where and why we're doing
> >>> a *non-leaf* invalidation that isn't aligned to the size of a table, because
> >>> that in itself doesn't make a whole heap of sense - my hunch is that that
> >>> wants figuring out and could probably be fixed at the source.
> >>
> >> Isn't that described above because we're using CMDQ_TLBI_RANGE_NUM_MAX
> >> to break up the range into separate commands?
> >
> > Not really, because if we're doing a genuine non-leaf invalidation of a
> > table then it should be a block-aligned range that ought to fit in a
> > single command and should certainly never involve a single-granule
> > remainder. If we're doing non-leaf invalidations of things that
> > logically don't need to be non-leaf, making them leaf would be the even
> > better option.
> >
>
> I agree with Robin that if the caller is doing a genuine non-leaf invalidation
> of a table, it should not involve a single-granule tlbi. It seems that the
> caller only filter the block size, but not the address aligned or not maybe.
There's only one caller though, right? That's the
io_pgtable_tlb_flush_walk() call in io-pgtable-arm.c which shouldn't trigger
this problem.
Do you have a backtrace for the case when we generate the illegal command?
> >> Do you mind if I queue the patch as-is for now? I don't think the driver
> >> should be emitting illegal commands, and v2 of the patch does seem like
> >> the obvious thing to do.
> >
> > TBH I'd rather you just drop my patch if it's proven problematic, and
> > I'll take another crack at it soon. The potential problems we introduce
> > by using non-range invalidates on errata-affected MMU-700 revisions are
> > worse than the almost-entirely-theoretical one I was trying to address.
> >
>
> If you all agree to roll back the problematic code, is the first patch be OK?
> Should I need to add some more descriptions to clarify this?
I can just go and revert Robin's original patch, but I'd like to see your
backtrace first so that we understand how this is occurring.
Thanks,
Will
On 2023-08-09 10:22, zhurui wrote:
> On 2023/8/9 0:43, Robin Murphy wrote:
>> On 08/08/2023 5:24 pm, Will Deacon wrote:
>>> Hi Robin,
>>>
>>> On Mon, Aug 07, 2023 at 08:20:45PM +0100, Robin Murphy wrote:
>>>> On 2023-08-06 06:28, zhurui wrote:
>>>>> On 2023/8/5 2:30, Nicolin Chen wrote:
>>>>>> On Fri, Aug 04, 2023 at 05:52:25PM +0100, Will Deacon wrote:
>>>>>>> On Fri, Aug 04, 2023 at 05:31:20PM +0800, zhurui wrote:
>>>>>>>> When tg != 0 but ttl, scale, num all 0 in a range tlbi command, it
>>>>>>>> is reserved and will cause the CERROR_ILL error. This case means
>>>>>>>> that the size to be invalidated is only one page size, and the
>>>>>>>> range invalidation is meaningless here. So we set tg to 0 in this
>>>>>>>> case to do an non-range invalidation instead.
>>>>>>
>>>>>>>> @@ -1930,6 +1927,12 @@ static void __arm_smmu_tlb_inv_range(struct arm_smmu_cmdq_ent *cmd,
>>>>>>>> num = (num_pages >> scale) & CMDQ_TLBI_RANGE_NUM_MAX;
>>>>>>>> cmd->tlbi.num = num - 1;
>>>>>>>>
>>>>>>>> + /* Prevent error caused by one page tlbi with leaf 0 */
>>>>>>>> + if (scale == 0 && num == 1 && cmd->tlbi.leaf == 0)
>>>>>>>> + cmd->tlbi.tg = 0;
>>>>>>>
>>>>>>> This should only be true for the last iteration, right (i.e. when num_pages
>>>>>>> == 1)? In which case, I'd prefer to leave the old code as-is and just add:
>>>>>>>
>>>>>>> /* Single-page leaf invalidation requires a TG field of 0 */
>>>>>>> if (num_pages == 1 && !cmd->tlbi.leaf)
>>>>>>> cmd->tlbi.tg = 0;To Will and Nicolin,
>>>>>
>>>>> Not only the last iteration, it's the result of __ffs function. For example, if
>>>>> numpages is 33, then the value of __ffs(num_pages) is 0, so the value of scale
>>>>> is also 0. The value of num depends on CMDQ_TLBI_RANGE_NUM_MAX. That is, the
>>>>> maximum value of num is 31. Therefore, the final value of num is 1.
>>>>> So, if consider CMDQ_TLBI_RANGE_NUM_MAX, there will be some case not the last
>>>>> one page but the beginning pages. That's why I use scale and num as conditions,
>>>>> not num_pages. Then I should reassign tg based on the result.
>>>>
>>>> Yeah, I'd rather not downgrade to a non-range invalidate since that
>>>> complicates the reasoning for the errata affecting those. If the size of the
>>>> invalidation is equal to TG then it can only represent a single last-level
>>>> page, i.e. TTL=3, thus if it does warrant handling here then indeed
>>>> rearranging to base the condition on num_pages as well ought to suffice.
>>>> However, this is all still begging the question of where and why we're doing
>>>> a *non-leaf* invalidation that isn't aligned to the size of a table, because
>>>> that in itself doesn't make a whole heap of sense - my hunch is that that
>>>> wants figuring out and could probably be fixed at the source.
>>>
>>> Isn't that described above because we're using CMDQ_TLBI_RANGE_NUM_MAX
>>> to break up the range into separate commands?
>>
>> Not really, because if we're doing a genuine non-leaf invalidation of a table then it should be a block-aligned range that ought to fit in a single command and should certainly never involve a single-granule remainder. If we're doing non-leaf invalidations of things that logically don't need to be non-leaf, making them leaf would be the even better option.
>>
>
> I agree with Robin that if the caller is doing a genuine non-leaf invalidation
> of a table, it should not involve a single-granule tlbi. It seems that the
> caller only filter the block size, but not the address aligned or not maybe.
Oh, did you hit this with SVA by any chance? AFAICS the only place
io-pgtable can cause a non-leaf invalidation is from tlb_flush_walk,
which absolutely should be table-aligned, but then there is another path
from arm_smmu_mm_invalidate_range(), where I guess we use non-leaf
unconditionally because the MMU notifier doesn't have visibility of
exactly how the pagetable structure may have changed. However, by the
same token that path really does actually need my fix, since it's also
hard-coding PAGE_SIZE as the invalidation granule. I was saying it's a
largely theoretical issue from the io-pgtable point of view, since the
chance of unmapping a level 1 table full of level 2 block entries via
the IOMMU API is minimal (other than cases like VFIO teardown where
precise invalidation doesn't matter since the whole domain is about to
be destroyed anyway), but in this SVA path which I hadn't considered
before, I think it means that unmapping *any* block entry may not be
invalidated correctly, and unmapping blocks in general is definitely
something that processes can and will do :(
>>> Do you mind if I queue the patch as-is for now? I don't think the driver
>>> should be emitting illegal commands, and v2 of the patch does seem like
>>> the obvious thing to do.
>>
>> TBH I'd rather you just drop my patch if it's proven problematic, and I'll take another crack at it soon. The potential problems we introduce by using non-range invalidates on errata-affected MMU-700 revisions are worse than the almost-entirely-theoretical one I was trying to address.
Does the patch below work for you?
Thanks,
Robin.
----->8-----
Subject: [PATCH] iommu/arm-smmu-v3: Avoid constructing invalid range commands
Although io-pgtable's non-leaf invalidations are always for full tables,
I missed that SVA also uses non-leaf invalidations, while being at the
mercy of whatever range the MMU notifier throws at it. This means it
definitely wants the previous TTL fix as well, since it also doesn't
know exactly which leaf level(s) may need invalidating, but it can also
give us less-aligned ranges wherein certain corners may lead to building
an invalid command where TTL, Num and Scale are all 0. It should be fine
to handle this by over-invalidating an extra page, since falling back to
a non-range command opens up a whole can of errata-flavoured worms.
Fixes: 6833b8f2e199 ("iommu/arm-smmu-v3: Set TTL invalidation hint better")
Reported-by: Rui Zhu <[email protected]>
Signed-off-by: Robin Murphy <[email protected]>
---
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 15 ++++++++++-----
1 file changed, 10 insertions(+), 5 deletions(-)
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 9b0dc3505601..6ccbae9b93a1 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -1895,18 +1895,23 @@ static void __arm_smmu_tlb_inv_range(struct arm_smmu_cmdq_ent *cmd,
/* Get the leaf page size */
tg = __ffs(smmu_domain->domain.pgsize_bitmap);
+ num_pages = size >> tg;
+
/* Convert page size of 12,14,16 (log2) to 1,2,3 */
cmd->tlbi.tg = (tg - 10) / 2;
/*
- * Determine what level the granule is at. For non-leaf, io-pgtable
- * assumes .tlb_flush_walk can invalidate multiple levels at once,
- * so ignore the nominal last-level granule and leave TTL=0.
+ * Determine what level the granule is at. For non-leaf, both
+ * io-pgtable and SVA pass a nominal last-level granule because
+ * they don't know what level(s) actually apply, so ignore that
+ * and leave TTL=0. However for various errata reasons we still
+ * want to use a range command, so avoid the SVA corner case
+ * where both scale and num could be 0 as well.
*/
if (cmd->tlbi.leaf)
cmd->tlbi.ttl = 4 - ((ilog2(granule) - 3) / (tg - 3));
-
- num_pages = size >> tg;
+ else if ((num_pages & CMDQ_TLBI_RANGE_NUM_MAX) == 1)
+ num_pages++;
}
cmds.num = 0;
On 2023-08-09 14:48, Robin Murphy wrote:
[...]
> Does the patch below work for you?
Any comments on this? Just noticed this commit on a local dev branch and
realised I'd totally forgotten about it already. I'm pretty confident it
ought to be right, but then it *was* also me who missed the original bug
to begin with... ;)
Thanks,
Robin.
> ----->8-----
> Subject: [PATCH] iommu/arm-smmu-v3: Avoid constructing invalid range
> commands
>
> Although io-pgtable's non-leaf invalidations are always for full tables,
> I missed that SVA also uses non-leaf invalidations, while being at the
> mercy of whatever range the MMU notifier throws at it. This means it
> definitely wants the previous TTL fix as well, since it also doesn't
> know exactly which leaf level(s) may need invalidating, but it can also
> give us less-aligned ranges wherein certain corners may lead to building
> an invalid command where TTL, Num and Scale are all 0. It should be fine
> to handle this by over-invalidating an extra page, since falling back to
> a non-range command opens up a whole can of errata-flavoured worms.
>
> Fixes: 6833b8f2e199 ("iommu/arm-smmu-v3: Set TTL invalidation hint better")
> Reported-by: Rui Zhu <[email protected]>
> Signed-off-by: Robin Murphy <[email protected]>
> ---
> drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 15 ++++++++++-----
> 1 file changed, 10 insertions(+), 5 deletions(-)
>
> 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 9b0dc3505601..6ccbae9b93a1 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -1895,18 +1895,23 @@ static void __arm_smmu_tlb_inv_range(struct
> arm_smmu_cmdq_ent *cmd,
> /* Get the leaf page size */
> tg = __ffs(smmu_domain->domain.pgsize_bitmap);
>
> + num_pages = size >> tg;
> +
> /* Convert page size of 12,14,16 (log2) to 1,2,3 */
> cmd->tlbi.tg = (tg - 10) / 2;
>
> /*
> - * Determine what level the granule is at. For non-leaf,
> io-pgtable
> - * assumes .tlb_flush_walk can invalidate multiple levels at once,
> - * so ignore the nominal last-level granule and leave TTL=0.
> + * Determine what level the granule is at. For non-leaf, both
> + * io-pgtable and SVA pass a nominal last-level granule because
> + * they don't know what level(s) actually apply, so ignore that
> + * and leave TTL=0. However for various errata reasons we still
> + * want to use a range command, so avoid the SVA corner case
> + * where both scale and num could be 0 as well.
> */
> if (cmd->tlbi.leaf)
> cmd->tlbi.ttl = 4 - ((ilog2(granule) - 3) / (tg - 3));
> -
> - num_pages = size >> tg;
> + else if ((num_pages & CMDQ_TLBI_RANGE_NUM_MAX) == 1)
> + num_pages++;
> }
>
> cmds.num = 0;
>
On 2023/8/19 0:21, Will Deacon wrote:
> On Fri, Aug 18, 2023 at 05:19:31PM +0100, Robin Murphy wrote:
>> On 2023-08-09 14:48, Robin Murphy wrote:
>> [...]
>>> Does the patch below work for you?
>>
>> Any comments on this? Just noticed this commit on a local dev branch and
>> realised I'd totally forgotten about it already. I'm pretty confident it
>> ought to be right, but then it *was* also me who missed the original bug to
>> begin with... ;)
>
> I'm happy to take it if zhurui can confirm that it fixes their issue...
>
> Will (had also forgotten about this)
>
>>> ----->8-----
>>> Subject: [PATCH] iommu/arm-smmu-v3: Avoid constructing invalid range
>>> commands
>>>
>>> Although io-pgtable's non-leaf invalidations are always for full tables,
>>> I missed that SVA also uses non-leaf invalidations, while being at the
>>> mercy of whatever range the MMU notifier throws at it. This means it
>>> definitely wants the previous TTL fix as well, since it also doesn't
>>> know exactly which leaf level(s) may need invalidating, but it can also
>>> give us less-aligned ranges wherein certain corners may lead to building
>>> an invalid command where TTL, Num and Scale are all 0. It should be fine
>>> to handle this by over-invalidating an extra page, since falling back to
>>> a non-range command opens up a whole can of errata-flavoured worms.
>>>
>>> Fixes: 6833b8f2e199 ("iommu/arm-smmu-v3: Set TTL invalidation hint better")
>>> Reported-by: Rui Zhu <[email protected]>
>>> Signed-off-by: Robin Murphy <[email protected]>
>>> ---
>>> drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 15 ++++++++++-----
>>> 1 file changed, 10 insertions(+), 5 deletions(-)
>>>
>>> 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 9b0dc3505601..6ccbae9b93a1 100644
>>> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>>> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>>> @@ -1895,18 +1895,23 @@ static void __arm_smmu_tlb_inv_range(struct
>>> arm_smmu_cmdq_ent *cmd,
>>> /* Get the leaf page size */
>>> tg = __ffs(smmu_domain->domain.pgsize_bitmap);
>>>
>>> + num_pages = size >> tg;
>>> +
>>> /* Convert page size of 12,14,16 (log2) to 1,2,3 */
>>> cmd->tlbi.tg = (tg - 10) / 2;
>>>
>>> /*
>>> - * Determine what level the granule is at. For non-leaf,
>>> io-pgtable
>>> - * assumes .tlb_flush_walk can invalidate multiple levels at once,
>>> - * so ignore the nominal last-level granule and leave TTL=0.
>>> + * Determine what level the granule is at. For non-leaf, both
>>> + * io-pgtable and SVA pass a nominal last-level granule because
>>> + * they don't know what level(s) actually apply, so ignore that
>>> + * and leave TTL=0. However for various errata reasons we still
>>> + * want to use a range command, so avoid the SVA corner case
>>> + * where both scale and num could be 0 as well.
>>> */
>>> if (cmd->tlbi.leaf)
>>> cmd->tlbi.ttl = 4 - ((ilog2(granule) - 3) / (tg - 3));
>>> -
>>> - num_pages = size >> tg;
>>> + else if ((num_pages & CMDQ_TLBI_RANGE_NUM_MAX) == 1)
>>> + num_pages++;
>>> }
>>>
>>> cmds.num = 0;
>>>
Hi, Will and Robin,
Sorry for taking so long to reply you. We have some problems with our machine these days. It's
solved just today. I give a test with Robin's patch for our testcase, everything is ok. I think
the problem has been solved.
Thanks,
ZhuRui.
On Wed, Sep 06, 2023 at 01:59:55PM +0100, Robin Murphy wrote:
> On 2023-09-06 06:05, Easwar Hariharan wrote:
> > Could you please send out this patch since ZhuRui has confirmed it fixes
> > their issue and CC it to stable for v5.15+? Or if Will is willing to
> > pick it up off this thread, I can do the backport to stable.
>
> I can resend after -rc1 if Will would prefer that. It's tagged as a fix so
> should hopefully get picked for stable automatically once it hits mainline.
Yes, please! Then I can queue it next week and send it to Joerg as a fix.
Will