Re: [PATCH v5 14/27] iommu/arm-smmu-v3: Make changing domains be hitless for ATS

[Jason Gunthorpe <jgg@nvidia.com>]

On Thu, Mar 21, 2024 at 08:26:43PM +0800, Michael Shavit wrote:
> Overall I think the patch works, but it took me a while to really
> digest the big picture. Just to make sure I fully understand:
> 
> We're trying to satisfy the following invariants for correctness:
> 1. Devices cannot get translations from a domain that was detached
> after arm_smmu_attach_dev() returns.

Yes, regardless of ATS this is the API requirement.

> 2. Devices cannot get cached translations from a domain after
> arm_smmu_atc_inv_domain() returns, regardless of whether the domain is
> simultaneously being attached/detached from a device.

There is no simultaneously here since the group lock is held by the
core code.
 
> Apart from note point 2. above, the behaviour of translations while in
> the middle of an arm_smmu_attach_dev doesn't have well defined
> requirements:
> 1. Before this patch, devices may get translations belonging to the
> old domain, then aborts or identity translations, and then the new
> domain while arm_smmu_attach_dev() is in progress.

Yes

> 2. Ater this patch, devices may get an arbitrary mix of translations
> belonging to the old domain and the new domain while
> arm_smmu_attach_dev() is in progress.

Yes, but no aborts.

> While disabling and re-enabling ATS inside arm_smmu_attach_dev() would
> meet the same requirements as this patch, it's not optimal since the
> device may still have traffic on other pasids than the one being
> manipulated.

Yes, we can't just disable ATS due to the PASIDs. A SVA PASID might be
present and disabling ATS to change the RID domain would completely
wreck it. Today this scenario is prevented by sva_enable, which is
removed in following patches.

I added this note for context to the commit message:

 This is part of the bigger picture to allow changing the RID domain while
 a PASID is in use. If a SVA PASID is relying on ATS to function then
 changing the RID domain cannot just temporarily toggle ATS off without
 also wrecking the SVA PASID. The new infrastructure here is organized so
 that the PASID attach/detach flows will make use of it as well in
 following patches.

Thanks,
Jason