[PATCH] iommu/arm-smmu-v3: Simplify stage selection logic

[PATCH] iommu/arm-smmu-v3: Simplify stage selection logic

[Michael Shavit]

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(-)

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
-- 
2.41.0.694.ge786442a9b-goog


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

Re: [PATCH] iommu/arm-smmu-v3: Simplify stage selection logic

[Nicolin Chen]

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>

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

Re: [PATCH] iommu/arm-smmu-v3: Simplify stage selection logic

[Robin Murphy]

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

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

Re: [PATCH] iommu/arm-smmu-v3: Simplify stage selection logic

[Michael Shavit]

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.

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

Re: [PATCH] iommu/arm-smmu-v3: Simplify stage selection logic

[Robin Murphy]

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.

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

Re: [PATCH] iommu/arm-smmu-v3: Simplify stage selection logic

[Michael Shavit]

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.

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