On 2023-08-17 18:06, Michael Shavit wrote:
> On Fri, Aug 18, 2023 at 12:35 AM Robin Murphy <[email protected]> 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 <[email protected]> wrote:
>
> On 2023-08-17 18:06, Michael Shavit wrote:
> > On Fri, Aug 18, 2023 at 12:35 AM Robin Murphy <[email protected]> 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.