| Message ID | d5fc1f72-7428-4fef-d868-d06b85add635@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/1] iommu/arm-smmu-v3: Fix error case of range command | expand |
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
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 <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;
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; >
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; > >
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.
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.
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.
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
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);
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(-) -- 1.8.3.1