Messages in this thread | | | Date | Thu, 3 Aug 2023 14:44:00 -0300 | From | Jason Gunthorpe <> | Subject | Re: [PATCH v5 2/6] iommu/arm-smmu-v3: Keep track of attached ssids |
| |
On Fri, Aug 04, 2023 at 12:32:08AM +0800, Michael Shavit wrote: > On Thu, Aug 3, 2023 at 11:42 PM Jason Gunthorpe <jgg@nvidia.com> wrote: > > > > On Thu, Aug 03, 2023 at 06:12:22PM +0800, Michael Shavit wrote: > > > The arm-smmu-v3 driver keeps track of all masters that a domain is > > > attached to so that it can re-write their STEs when the domain's ASID is > > > upated by SVA. > > > > Wah? > > > > A domain's ASID shouldn't change, why does it change for SVA? Doesn't > > SVA use CDTE's only? Why would it ever change a STE? I'm confused what > > you are trying to explain here. > > Urh right, I mixed up CD entry and STE here. Before this patch, SVA > keeps tracks of all the masters attached to a CD domain, and updates > the CD entry 0 in their CD table.
Because it assumes that if a domain is returned from the ASID lookup it is a RID domain.
> Now that a CD domain can be attached on non-zero SSIDs, SVA can't > simply update slot 0 in the table; it must know which slot(s) this > domain is attached to.
Yes, so why are you passing in 0 as the ssid argument to arm_smmu_write_ctx_desc_devices() for the ASID change?
I think your commit message is trying to say:
The SMMUv3 driver keeps track of all the iommu_domains that are assigned to an ASID in an xarray. The SVA code needs to re-use the same ASID as the CPU's ASID (presumably only for BTM mode?) so it has a mechanism to reclaim an already used ASID from an existing domain.
This is currently hardwired with the assumption that a domain using an ASID is only on SSID 0.
Add a list to the struct arm_smmu_domain recording each master and SSID that the domain is attached to and update it when any domain is attached/detached.
Make arm_smmu_write_ctx_desc_devices() follow this list when storing the CD table entries for the domain.
Remove 'ssid' as an argument to arm_smmu_write_ctx_desc_devices() since it is always found in the attached_ssids.
> > What is a "primary domain"? Why can't we fix SVA first so it doesn't > > have this weird "piggybacks" or: > > > ... > > > > This patch is not making sense to me, the goal in the commit message > > seems logical, but why is tracking CD entries introducing this concept > > of a primary domain and doing special stuff for SSID=0? > > I'd argue this patch isn't introducing anything the driver isn't > already doing.
So this I don't understand:
+ if (ssid && attached_domain->ssid == 0) { + ret = arm_smmu_write_ctx_desc(master, ssid, cd); + } else { + ret = arm_smmu_write_ctx_desc( + master, attached_domain->ssid, cd); + }
Fix this patch so attached_domain->ssid is never wrong?
Remove ssid as an input to the function.
(I'd ultimately expect to remove CD too)
> it. I do have a patch series in the works to properly fix SVA, but > it's growing quite large and I was trying to get this feature > out first. At a high level, the subsequent series: > 1. Nests the list of attached masters in a list of SMMUs the domain is > installed in. Updates SMMU-level operations (such as invalidations) to > iterate over said list. > 2. Checks the compatibility of a domain when attaching to a new SMMU > instead of outright rejecting, to allow attaching a domain to multiple > SMMUs. > 3. Thus allowing SVA to alloc a single domain for the MM struct (which > the series maps from multiple SVA domains internally, pending support > at the iommu core layer)
This should not be hard for the core code
> 4. And allowing SVA to use the same set_dev_pasid implementation used > here on that domain.
This list all makes alot of sense to me
> Now having said that, it might be possible to get rid of piggybacking > sooner if we create an MM per master instead of per "primary-domain", > but I'm not too sure about performance implications. AFAICT, the only > downside might be for invalidate_range commands that could no longer > be sent as a batched command to the SMMU (since each mmu notifier > would be called independently).
I'm not sure this series leaves things in a better state than before, now we have two parallel domain attachment paths for PASID :(
Jason
| |