[PATCH 1/1] Revert "iommu/arm-smmu-v3: Set TTL invalidation hint better"

[PATCH 1/1] Revert "iommu/arm-smmu-v3: Set TTL invalidation hint better"

[wangwudi]

From: Rui Zhu <zhurui3@huawei.com>

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 <will@kernel.org>
Cc: Robin Murphy <robin.murphy@arm.com>
Cc: Joerg Roedel <joro@8bytes.org>
Cc: Lu Baolu <baolu.lu@linux.intel.com>
Cc: Jason Gunthorpe <jgg@ziepe.ca>
Cc: Yicong Yang <yangyicong@hisilicon.com>
Cc: Tomas Krcka <krckatom@amazon.de>
Cc: Jean-Philippe Brucker <jean-philippe@linaro.org>
Cc: Nicolin Chen <nicolinc@nvidia.com>
Cc: Rui Zhu <zhurui3@huawei.com>

Signed-off-by: Rui Zhu <zhurui3@huawei.com>
---
 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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/1] Revert "iommu/arm-smmu-v3: Set TTL invalidation hint better"

[Will Deacon]

On Mon, Jul 31, 2023 at 02:21:22PM +0800, wangwudi wrote:
> From: Rui Zhu <zhurui3@huawei.com>
> 
> 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 <will@kernel.org>
> Cc: Robin Murphy <robin.murphy@arm.com>
> Cc: Joerg Roedel <joro@8bytes.org>
> Cc: Lu Baolu <baolu.lu@linux.intel.com>
> Cc: Jason Gunthorpe <jgg@ziepe.ca>
> Cc: Yicong Yang <yangyicong@hisilicon.com>
> Cc: Tomas Krcka <krckatom@amazon.de>
> Cc: Jean-Philippe Brucker <jean-philippe@linaro.org>
> Cc: Nicolin Chen <nicolinc@nvidia.com>
> Cc: Rui Zhu <zhurui3@huawei.com>
> 
> Signed-off-by: Rui Zhu <zhurui3@huawei.com>
> ---
>  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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/1] Revert "iommu/arm-smmu-v3: Set TTL invalidation hint better"

[zhurui]

On 2023/8/1 16:55, Will Deacon wrote:
> On Mon, Jul 31, 2023 at 02:21:22PM +0800, wangwudi wrote:
>> From: Rui Zhu <zhurui3@huawei.com>
>>
>> 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 <will@kernel.org>
>> Cc: Robin Murphy <robin.murphy@arm.com>
>> Cc: Joerg Roedel <joro@8bytes.org>
>> Cc: Lu Baolu <baolu.lu@linux.intel.com>
>> Cc: Jason Gunthorpe <jgg@ziepe.ca>
>> Cc: Yicong Yang <yangyicong@hisilicon.com>
>> Cc: Tomas Krcka <krckatom@amazon.de>
>> Cc: Jean-Philippe Brucker <jean-philippe@linaro.org>
>> Cc: Nicolin Chen <nicolinc@nvidia.com>
>> Cc: Rui Zhu <zhurui3@huawei.com>
>>
>> Signed-off-by: Rui Zhu <zhurui3@huawei.com>
>> ---
>>  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
> .
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 1/1] iommu/arm-smmu-v3: Fix error case of range command

[zhurui]

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 <will@kernel.org>
Cc: Robin Murphy <robin.murphy@arm.com>
Cc: Joerg Roedel <joro@8bytes.org>
Cc: Lu Baolu <baolu.lu@linux.intel.com>
Cc: Jason Gunthorpe <jgg@ziepe.ca>
Cc: Yicong Yang <yangyicong@hisilicon.com>
Cc: Tomas Krcka <krckatom@amazon.de>
Cc: Jean-Philippe Brucker <jean-philippe@linaro.org>
Cc: Nicolin Chen <nicolinc@nvidia.com>
Cc: Rui Zhu <zhurui3@huawei.com>

Signed-off-by: Rui Zhu <zhurui3@huawei.com>
---
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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/1] iommu/arm-smmu-v3: Fix error case of range command

[Will Deacon]

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 <will@kernel.org>
> Cc: Robin Murphy <robin.murphy@arm.com>
> Cc: Joerg Roedel <joro@8bytes.org>
> Cc: Lu Baolu <baolu.lu@linux.intel.com>
> Cc: Jason Gunthorpe <jgg@ziepe.ca>
> Cc: Yicong Yang <yangyicong@hisilicon.com>
> Cc: Tomas Krcka <krckatom@amazon.de>
> Cc: Jean-Philippe Brucker <jean-philippe@linaro.org>
> Cc: Nicolin Chen <nicolinc@nvidia.com>
> Cc: Rui Zhu <zhurui3@huawei.com>
> 
> Signed-off-by: Rui Zhu <zhurui3@huawei.com>
> ---
> 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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/1] iommu/arm-smmu-v3: Fix error case of range command

[Nicolin Chen]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/1] iommu/arm-smmu-v3: Fix error case of range command

[zhurui]

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
.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/1] iommu/arm-smmu-v3: Fix error case of range command

[Robin Murphy]

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
> .

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/1] iommu/arm-smmu-v3: Fix error case of range command

[Will Deacon]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/1] iommu/arm-smmu-v3: Fix error case of range command

[Robin Murphy]

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.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/1] iommu/arm-smmu-v3: Fix error case of range command

[zhurui]

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.






_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/1] iommu/arm-smmu-v3: Fix error case of range command

[Will Deacon]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/1] iommu/arm-smmu-v3: Fix error case of range command

[Robin Murphy]

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 <zhurui3@huawei.com>
Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
  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;

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/1] iommu/arm-smmu-v3: Fix error case of range command

[Robin Murphy]

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 <zhurui3@huawei.com>
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> ---
>   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;
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/1] iommu/arm-smmu-v3: Fix error case of range command

[Will Deacon]

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 <zhurui3@huawei.com>
> > Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> > ---
> >   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;
> > 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/1] iommu/arm-smmu-v3: Fix error case of range command

[zhurui]

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 <zhurui3@huawei.com>
>>> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
>>> ---
>>>   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.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/1] iommu/arm-smmu-v3: Fix error case of range command

[Easwar Hariharan]

On 8/25/23 01:12, zhurui wrote:
> 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 <zhurui3@huawei.com>
>>>> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
>>>> ---
>>>>    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.
>

Hi Robin,

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.

-- 
Thanks,
Easwar


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/1] iommu/arm-smmu-v3: Fix error case of range command

[Robin Murphy]

On 2023-09-06 06:05, Easwar Hariharan wrote:
> On 8/25/23 01:12, zhurui wrote:
>> 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 <zhurui3@huawei.com>
>>>>> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
>>>>> ---
>>>>>    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.
>>
> 
> Hi Robin,
> 
> 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.

Thanks,
Robin.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/1] iommu/arm-smmu-v3: Fix error case of range command

[Will Deacon]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/1] iommu/arm-smmu-v3: Fix error case of range command

[Will Deacon]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel