Re: [PATCH v2 3/6] iommu/vt-d: Make intel_iommu_set_dev_pasid() to handle domain replacement

[Baolu Lu <baolu.lu@xxxxxxxxxxxxxxx>]

On 2024/9/30 15:19, Tian, Kevin wrote:
> > From: Baolu Lu <baolu.lu@xxxxxxxxxxxxxxx>
> > Sent: Friday, September 13, 2024 10:17 AM
> >
> > On 9/13/24 9:35 AM, Baolu Lu wrote:
> > > On 9/12/24 9:04 PM, Yi Liu wrote:
> > > > +static void intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t
> > > > pasid,
> > > > +                     struct iommu_domain *domain)
> > > > +{
> > > > +    struct device_domain_info *info = dev_iommu_priv_get(dev);
> > > > +    struct intel_iommu *iommu = info->iommu;
> > > > +
> > > >        intel_pasid_tear_down_entry(iommu, dev, pasid,
> > > >                        INTEL_PASID_TEARDOWN_DRAIN_PRQ);
> > > > +    if (domain->type == IOMMU_DOMAIN_IDENTITY)
> > > > +        return;
> > >
> > > The static identity domain is not capable of handling page requests.
> > > Therefore there is no need to drain PRQ for an identity domain removal.
> > >
> > > So it probably should be something like this:
> > >
> > >       if (domain->type == IOMMU_DOMAIN_IDENTITY) {
> > >           intel_pasid_tear_down_entry(iommu, dev, pasid, 0);
> > >           return;
> > >       }
> > >
> > >       intel_pasid_tear_down_entry(iommu, dev, pasid,
> > >                                       INTEL_PASID_TEARDOWN_DRAIN_PRQ);
> >
> > Just revisited this. It seems that we just need to drain PRQ if the
> > attached domain is iopf-capable. Therefore, how about making it like
> > this?
> >
> > 	unsigned int flags = 0;
> >
> > 	if (domain->iopf_handler)
> > 		flags |= INTEL_PASID_TEARDOWN_DRAIN_PRQ;
> >
> > 	intel_pasid_tear_down_entry(iommu, dev, pasid, flags);
> >
> > 	/* Identity domain has no meta data for pasid. */
> > 	if (domain->type == IOMMU_DOMAIN_IDENTITY)
> > 		return;
>
> this is the right thing to do, but also suggesting a bug in existing
> code. intel_pasid_tear_down_entry() is not just for PRQ drain.
> It's also about iotlb/devtlb invalidation. From device p.o.v it
> has no idea about the translation mode in the IOMMU side and
> always caches the valid mappings in devtlb when ATS is enabled.

Yes. You are right.

intel_pasid_tear_down_entry() takes care of iotlb/devtlb invalidation.
So it's fine as long as intel_pasid_tear_down_entry() is called for the
IDENTITY domain path, right?

> Existing code skips all those housekeeping for identify domain
> by early return before intel_pasid_tear_down_entry(). We need
> a separate fix for it before this series?

Existing code doesn't skip intel_pasid_tear_down_entry().

        if (domain->type == IOMMU_DOMAIN_IDENTITY) {
                intel_pasid_tear_down_entry(iommu, dev, pasid, false);
                return;
        }

Or anything I overlooked?

Thanks,
baolu