Re: [PATCH v2 12/12] iommu/vt-d: Add set_dev_pasid callback for nested domain

[Jason Gunthorpe <jgg@nvidia.com>]

On Mon, May 06, 2024 at 03:42:21PM +0800, Baolu Lu wrote:
> On 2024/4/30 17:19, Yi Liu wrote:
> > On 2024/4/17 17:25, Tian, Kevin wrote:
> > > > From: Liu, Yi L <yi.l.liu@intel.com>
> > > > Sent: Friday, April 12, 2024 4:15 PM
> > > > 
> > > > From: Lu Baolu <baolu.lu@linux.intel.com>
> > > > 
> > > > This allows the upper layers to set a nested type domain to a PASID of a
> > > > device if the PASID feature is supported by the IOMMU hardware.
> > > > 
> > > > The set_dev_pasid callback for non-nested domain has already be
> > > > there, so
> > > > this only needs to add it for nested domains. Note that the S2
> > > > domain with
> > > > dirty tracking capability is not supported yet as no user for now.
> > > 
> > > S2 domain does support dirty tracking. Do you mean the specific
> > > check in intel_iommu_set_dev_pasid() i.e. pasid-granular dirty
> > > tracking is not supported yet?
> > 
> > yes. We may remove this check when real usage comes. e.g. SIOV.
> > 
> > > > +static int intel_nested_set_dev_pasid(struct iommu_domain *domain,
> > > > +                      struct device *dev, ioasid_t pasid,
> > > > +                      struct iommu_domain *old)
> > > > +{
> > > > +    struct device_domain_info *info = dev_iommu_priv_get(dev);
> > > > +    struct dmar_domain *dmar_domain = to_dmar_domain(domain);
> > > > +    struct intel_iommu *iommu = info->iommu;
> > > > +
> > > > +    if (iommu->agaw < dmar_domain->s2_domain->agaw)
> > > > +        return -EINVAL;
> > > > +
> > > 
> > > this check is covered by prepare_domain_attach_device() already.
> > 
> > This was added to avoid modifying the s2_domain's agaw. I'm fine to remove
> > it personally as the existing attach path also needs to update domain's
> > agaw per device attachment. @Baolu, how about your opinion?
> 
> We still need something to do before we can safely remove this check.
> All the domain allocation interfaces should eventually have the device
> pointer as the input, and all domain attributions could be initialized
> during domain allocation. In the attach paths, it should return -EINVAL
> directly if the domain is not compatible with the iommu for the device.

Yes, and this is already true for PASID.

I feel we could reasonably insist that domanis used with PASID are
allocated with a non-NULL dev.

If so it means we need to fixup the domain allocation in iommufd as
part of the pasid series, and Intel will have to implement
alloc_domain_paging().

Jason