| Message ID | 20230818000321.1.Ibca43cc8d1bcad3ac3deef5726b9745128aea634@changeid (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iommu/arm-smmu-v3: Simplify stage selection logic | expand |
On Fri, Aug 18, 2023 at 12:03:30AM +0800, Michael Shavit wrote: > It is invalid for an arm-smmu-v3 to have neither FEAT_TRANS_S1 nor > FEAT_TRANS_S2 bits set, and this is even checked in the probe. > > Only set ARM_SMMU_DOMAIN_S2 if FEAT_TRANS_S1 isn't supported, otherwise set > ARM_SMMU_DOMAIN_S1. This is clearer as the existing code implies that > something more sophisticated is going on with the stage selection logic. > > Signed-off-by: Michael Shavit <mshavit@google.com> Reviewed-by: Nicolin Chen <nicolinc@nvidia.com>
On 2023-08-17 17:03, Michael Shavit wrote: > It is invalid for an arm-smmu-v3 to have neither FEAT_TRANS_S1 nor > FEAT_TRANS_S2 bits set, and this is even checked in the probe. > > Only set ARM_SMMU_DOMAIN_S2 if FEAT_TRANS_S1 isn't supported, otherwise set > ARM_SMMU_DOMAIN_S1. This is clearer as the existing code implies that > something more sophisticated is going on with the stage selection logic. The reason it's like this is because of arm_smmu_enable_nesting(), which *is* the additional thing that's going on with the stage selection logic. Thanks, Robin. > Signed-off-by: Michael Shavit <mshavit@google.com> > --- > > drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > 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 9b0dc35056019..e74c8c630adfa 100644 > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > @@ -2206,7 +2206,7 @@ static int arm_smmu_domain_finalise(struct iommu_domain *domain, > /* Restrict the stage to what we can actually support */ > if (!(smmu->features & ARM_SMMU_FEAT_TRANS_S1)) > smmu_domain->stage = ARM_SMMU_DOMAIN_S2; > - if (!(smmu->features & ARM_SMMU_FEAT_TRANS_S2)) > + else > smmu_domain->stage = ARM_SMMU_DOMAIN_S1; > > switch (smmu_domain->stage) { > > base-commit: 6eaae198076080886b9e7d57f4ae06fa782f90ef
On Fri, Aug 18, 2023 at 12:35 AM Robin Murphy <robin.murphy@arm.com> wrote: > > The reason it's like this is because of arm_smmu_enable_nesting(), which > *is* the additional thing that's going on with the stage selection logic. > > Thanks, > Robin. Right, but arm_smmu_enable_nesting isn't involved in this computation at this point in the flow. arm_smmu_enable_nesting returns early if smmu_domain->smmu isn't set, and smmu_domain->smmu is only set after arm_smmu_domain_finalise. So at this point, smmu_domain->stage is being initialized for the first time. If this code is responsible for handling some special nesting case, then it's probably not working as intended.
On 2023-08-17 18:06, Michael Shavit wrote: > On Fri, Aug 18, 2023 at 12:35 AM Robin Murphy <robin.murphy@arm.com> wrote: >> >> The reason it's like this is because of arm_smmu_enable_nesting(), which >> *is* the additional thing that's going on with the stage selection logic. >> >> Thanks, >> Robin. > > Right, but arm_smmu_enable_nesting isn't involved in this computation > at this point in the flow. > > arm_smmu_enable_nesting returns early if smmu_domain->smmu isn't set, > and smmu_domain->smmu is only set after arm_smmu_domain_finalise. > So at this point, smmu_domain->stage is being initialized for the > first time. If this code is responsible for handling some special > nesting case, then it's probably not working as intended. I think you may have misread that code... Anyway, the point of the logic here is that it is not "selection", it is, as the comment says, "restriction" - i.e. it is checking that the already-selected stage is actually supported, and coercing it if not. The default selection for a newly-allocated domain is always implicitly ARM_SMMU_DOMAIN_S1 (which is explicitly defined as 0 to convey that significance), but it may be set to ARM_SMMU_DOMAIN_NESTED before the first attach finalises the pagetable format. Obviously this could be clearer, especially for anyone not so familiar with all the history, but at this point I honestly don't think it's worth doing anything without completely ripping out arm_smmu_enable_nesting() as well. Jason already had a patch a while ago, and my bus rework is now also very close to the point of finally fixing iommu_domain_alloc() to be able to return working domains, such that all the "domain_finalise" bodges go away and that whole "modify the domain between allocation and attach" paradigm is no longer valid at all. By this point I'm not too fussed about breaking the current meaning of ARM_SMMU_DOMAIN_NESTED any more. But what I definitely don't want to do is have a change like this which subtly but decisively breaks it while still leaving all the now-dead code in place ;) Thanks, Robin.
On Fri, Aug 18, 2023 at 2:19 AM Robin Murphy <robin.murphy@arm.com> wrote: > > On 2023-08-17 18:06, Michael Shavit wrote: > > On Fri, Aug 18, 2023 at 12:35 AM Robin Murphy <robin.murphy@arm.com> wrote: > >> > >> The reason it's like this is because of arm_smmu_enable_nesting(), which > >> *is* the additional thing that's going on with the stage selection logic. > >> > >> Thanks, > >> Robin. > > > > Right, but arm_smmu_enable_nesting isn't involved in this computation > > at this point in the flow. > > > > arm_smmu_enable_nesting returns early if smmu_domain->smmu isn't set, > > and smmu_domain->smmu is only set after arm_smmu_domain_finalise. > > So at this point, smmu_domain->stage is being initialized for the > > first time. If this code is responsible for handling some special > > nesting case, then it's probably not working as intended. > > I think you may have misread that code... oof, yes, I did indeed misread. > > Anyway, the point of the logic here is that it is not "selection", it > is, as the comment says, "restriction" - i.e. it is checking that the > already-selected stage is actually supported, and coercing it if not. > The default selection for a newly-allocated domain is always implicitly > ARM_SMMU_DOMAIN_S1 (which is explicitly defined as 0 to convey that > significance), but it may be set to ARM_SMMU_DOMAIN_NESTED before the > first attach finalises the pagetable format. Thanks for the explanation, that does make sense :) . > Obviously this could be clearer, especially for anyone not so familiar > with all the history, but at this point I honestly don't think it's > worth doing anything without completely ripping out > arm_smmu_enable_nesting() as well. Jason already had a patch a while > ago, and my bus rework is now also very close to the point of finally > fixing iommu_domain_alloc() to be able to return working domains, such > that all the "domain_finalise" bodges go away and that whole "modify the > domain between allocation and attach" paradigm is no longer valid at all. > > By this point I'm not too fussed about breaking the current meaning of > ARM_SMMU_DOMAIN_NESTED any more. But what I definitely don't want to do > is have a change like this which subtly but decisively breaks it while > still leaving all the now-dead code in place ;) Ack, will drop this change.
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 9b0dc35056019..e74c8c630adfa 100644 --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c @@ -2206,7 +2206,7 @@ static int arm_smmu_domain_finalise(struct iommu_domain *domain, /* Restrict the stage to what we can actually support */ if (!(smmu->features & ARM_SMMU_FEAT_TRANS_S1)) smmu_domain->stage = ARM_SMMU_DOMAIN_S2; - if (!(smmu->features & ARM_SMMU_FEAT_TRANS_S2)) + else smmu_domain->stage = ARM_SMMU_DOMAIN_S1; switch (smmu_domain->stage) {
It is invalid for an arm-smmu-v3 to have neither FEAT_TRANS_S1 nor FEAT_TRANS_S2 bits set, and this is even checked in the probe. Only set ARM_SMMU_DOMAIN_S2 if FEAT_TRANS_S1 isn't supported, otherwise set ARM_SMMU_DOMAIN_S1. This is clearer as the existing code implies that something more sophisticated is going on with the stage selection logic. Signed-off-by: Michael Shavit <mshavit@google.com> --- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) base-commit: 6eaae198076080886b9e7d57f4ae06fa782f90ef