> From: Baolu Lu <baolu.lu@xxxxxxxxxxxxxxx>
> Sent: Wednesday, October 9, 2024 9:09 AM
>
> 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?
No. Looks I was confused by this patch and misunderstood the
existing code.
