Re: [PATCH 04/19] iommu/arm-smmu-v3: Make STE programming independent of the callers

[Jason Gunthorpe <jgg@nvidia.com>]

On Thu, Oct 12, 2023 at 04:10:50PM +0800, Michael Shavit wrote:

> This sounds pretty complicated....Is this complexity really required
> here?

It is first needed before 'iommu/arm-smmu-v3: Do not change the STE
twice during arm_smmu_attach_dev()' which is a couple of patches
further on.

Then it keeps getting relied on.

I don't think there is any simple answer here, the HW has this complex
requirement. The current code is also complex - arguably more complex
because of how leaky/fragile it is. There is even a couple of pages of
text in the spec describing how to do this, and it doesn't discuss the
hitless cases!

At the end this is only 32 lines and it replaces both
arm_smmu_write_ctx_desc() and arm_smmu_write_strtab_ent().

FWIW, I found the most difficult part the used bit calculation, not
the update algorithm. Difficult because it is hard to read and find in
the spec when things are INGORED, but it is a "straightforward" job of
finding INGORED cases and making the used bits 0.

> Specifically, can we start with a naive version that always first
> nukes `V=0` before writing the STE?

I'm a little worried doing so will subtly break things that are
currently working as the current code does have cases which are
hitless.

Then we'd just need to change it anyhow in 5 patches or so

> This still allows you to remove requirements that callers must have
> first set the STE to abort (supposedly to get rid of the
> arm_smmu_detach_dev call currently made from arm_smmu_attach_dev)
> while being easier to digest.  The more sophisticated version can
> then be closer in the series to the patch that requires it
> (supposedly this is to support replacing a fully blocking/bypass STE
> with one that uses
> STRTAB_STE_1_S1DSS_TERMINATE/STRTAB_STE_1_S1DSS_BYPASS when a pasid
> domain is first attached?) at which point it's easier to reason
> about its benefits and alternatives.

From memory there are many cases that use the full functionality:

 - IDENTIY -> DMA -> IDENTITY hitless with RESV_DIRECT
 - STE -> S1DSS -> STE hitless (PASID upgrade)
 - S1 -> BLOCKING -> S1 with active PASID hitless (iommufd case)
 - CD ASID change hitless (BTM S1 replacement)
 - CD quiet_cd hitless (SVA mm release)

Some of this are fragile and open coded today, eg the CD quiet_cd and
ASID changes both just edit the STE in place. At the end we always
build full target STE/CDs and always consistently store it.

This is a nice tool because we don't have to specially think about the
above 5 case and painfully open code a FSM across several layers. We
just do and it works. Then everything else that can be hitless also
just becomes hitless, even if we don't have a use case for it..

> Can we get rid of this line and use target.data[0] everywhere above?
> 'val' isn't exactly a great name to describe the first word of the STE
> and there's no need to defer writing data[0] anymore since this isn't
> directly writing to the register.
> (Feel free to ignore this if it's already addressed by subsequent patches)

Subsequent patches erase this function :)

Thanks,
Jason