Re: [PATCH RFCv1 07/14] iommufd: Add viommu set/unset_dev_id ops

[Jason Gunthorpe <jgg@nvidia.com>]

On Tue, May 21, 2024 at 03:27:02PM -0700, Nicolin Chen wrote:
> On Tue, May 21, 2024 at 03:24:48PM -0300, Jason Gunthorpe wrote:
> > On Tue, May 14, 2024 at 06:59:07PM -0700, Nicolin Chen wrote:
> > > So, you want a proxy S1 domain for a device to attach, in case
> > > of a stage-2 only setup, because an S2 domain will no longer has
> > > a VMID, since it's shared among viommus. In the SMMU driver case,
> > > an arm_smmu_domain won't have an smmu pointer, so a device can't
> > > attach to an S2 domain but always an nested S1 domain, right?
> > 
> > That seems like a simple solution to the VMID lifetime, but it means
> > the kernel has to decode more types of vSTE.
> 
> Yea. For vSTE=abort, likely we need a nested block domain too?

Sure, it is easy to do
 
> > I don't know if there is merit one way or the other. A more specific
> > API surface is nice, but the two APIs are completely duplicating.
> > 
> > So maybe:
> > 
> > #define IOMMU_VIOMMU_INVALIDATE IOMMU_HWPT_INVALIDATE
> > 
> > As documentation and have the kernel just detect based on the type of
> > the passed ID?
> 
> Yea, the only difference is viommu_id v.s. hwpt_id that we can
> document.
> 
> Then in this case, we have two mostly identical uAPIs for the
> SMMU driver to use. Should we implement both?

I suspect it will turn out nicely naturally, lets try and see
 
> > > > We can add ATS invalidation after either as an enhancement as part of
> > > > adding the VIOMMU either as DEV_INVALIDATE or VIOMMU_INVALIDATE (or
> > > > both)
> > > 
> > > Yea, maybe step by step like this:
> > > 
> > > Part-1 VIOMMU_ALLOC and VIOMMU_ATTACH
> > > Part-2 VIOMMU_SET/UNSET_VDEV_ID
> > > Part-3 VIOMMU_INVALIDATE
> > > Part-4 VQUEUE_ALLOC
> > > ...
> > 
> > So we have this stuff still open:
> >  - Identity STE with PASID (part 2b)
> >  - IOMMU_GET_HW_INFO (part 3)
> >  - IOMMU_HWPT_ALLOC_NEST_PARENT (part 3)
> >  - IOMMU_HWPT_DATA_ARM_SMMUV3 (part 3)
> >  - IOMMU_HWPT_INVALIDATE_DATA_ARM_SMMUV3
> >  - VIOMMU_ALLOC, VIOMMU_ATTACH
> >  - VIOMMU_INVALIDATE
> >  - VIOMMU_SET/UNSET_VDEV_ID
> >  - VQUEUE_ALLOC / vCMDQ
> > 
> > I feel like IOMMU_HWPT_INVALIDATE_DATA_ARM_SMMUV3 is a reasonable fit
> > to part 3. Then part 4 would be VIOMMU_ALLOC -> VIOMMU_SET/UNSET_VDEV_ID
> > which brings ATS support the API.
> 
> There is some conflict at passing in viommu_id/viommu v.s. parent
> hwpt_id/domain for a nested domain allocation. Do you think that
> should be addressed later in VIOMMU series v.s. part3?
>
> More specifically, I have two drafts in my viommu series:
> 87a659e65229 WAR: iommufd: Allow pt_it to carry viommu_id
> 7c5fd8f50bc9 WAR pass in viommu pointer to domain_alloc_user op

It would be good for viommu to come with all the uAPI changes in one
shot, so all the pt_ids should be updated to accept viommu to pass the
S2 HWPT.

Then whatever driver changes are needed to make ATS work should come
together too.

> I know that these two only make sense with VIOMMU_ALOC. Yet, will
> there be a problem, if we establish nested domain allocation with
> parent domain/hwpt by part3, in the uAPI, and then change later?
> Will we end up with supporting two for backward compatibility?

I think this is fairly minor compatability, let's see.

Jason